On 25/05/17 13:41, Mason wrote: > On 25/05/2017 14:23, Marc Zyngier wrote: >> On 25/05/17 13:00, Mason wrote: >>> On 25/05/2017 10:48, Marc Zyngier wrote: >>>> Please have some defines for these magic values. >>> >>> Typical driver do >>> #define MUX_OFFSET 0x48 >>> and then access the register's value through >>> readl_relaxed(pcie->base + MUX_OFFSET); >>> >>> I can't do that because the registers were shuffled around >>> between revision 1 and revision 2. Thus, instead of an >>> explicitly-named macro (MUX_OFFSET), I used an explicitly- >>> named field (pcie->mux) and access the register's value >>> through readl_relaxed(pcie->mux); >> >> That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which >> you can supplement with a V2 at some point. >> >>> This is equivalent to providing the offset definitions in the >>> init functions, instead of at the top of the file. >> >> Sorry, my brain parses text far better than hex number. > > Well, the hex numbers do need to show up somewhere :-) > > IIUC, you're saying that > #define MUX_OFFSET 0x48 > is clearer than > pcie->mux = base + 0x48; yes. > > OK, I can accept that. Maybe our brains have been trained > to easily recognize and ingest the macro, or maybe it's > the caps, or maybe the fact that the statement does > several things (addition and assignment and hex). > > Out of curiosity, how would you feel about > pcie->MUX_OFFSET = 0x48; > and then using > readl_relaxed(pcie->base + pcie->MUX_OFFSET); > > It feels weird to me, I think mostly because it is > an unusual pattern. Exactly. Use existing practices help the reviewers quite a lot. > > Anyway, I'll add the macros, if that improves review and > maintenance. Thanks, M. -- Jazz is not dead. It just smells funny...
