On 25/05/17 13:41, Mason wrote:

On 25/05/2017 14:23, Marc Zyngier wrote:

quoted

On 25/05/17 13:00, Mason wrote:

quoted

On 25/05/2017 10:48, Marc Zyngier wrote:

quoted

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.

quoted

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