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; 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. Anyway, I'll add the macros, if that improves review and maintenance. Regards.
