Hi Finn, Geert,
In the interest of making minimal changes between the Mac and Amiga
versions, I'd leave the macros as they are, and add a comment to the
macro definitions stating that both addr and fifo are local-scope
variables in the only scope the macro is used in, to address reviewer's
concerns. Can you both live with that?
Placing the two macros in a suitable header in arch/m68k/include/asm/ so
Mac and Amiga can share the same code without duplicating it in two
files would be another option (that forces use of addr and fifo as
parameters), but let's not overengineer things. I don't expect any other
driver would need to share this code, or the PDMA macros also in the Mac
driver...
Other than that, I've implemented and tested all the suggested changes
and could post v4 of this patch now.
Cheers,
Michael
Am 16.03.18 um 00:17 schrieb Finn Thain:
On Wed, 14 Mar 2018, Michael Schmitz wrote:
Please pass "addr" and "fifo" as macro parameters, too, so it's easier
for the reviewer to notice they are used.
Yes, I can do that (meaning Finn would need to make the same change to
keep our versions in sync).
Personally, I wouldn't want to change it. This wasn't an oversight. Maybe
if you (Geert) take a look at MAC_ESP_PDMA_LOOP, etc. this style might
make more sense.
These are not macros in a header file that might get used in any random
scope. There is exactly one scope in which the macro appears, and the
variables that appear in the macro are simply the variables from that
scope.
If you apply the rule, "the macro's parameters should exhaustively list
all the macro's symbols", then (in this case) you'd violate the rule
"Don't Repeat Yourself". And if you'd adhere to the latter rule then you'd
violate the former. So I chose the more readable option.
The preprocessor allows us to pretend we are doing symbolic code
transformation, but that's not needed here. This was meant to be a textual
device.
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
