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. >
