[U-Boot] [PATCH v2 3/7] usb: eth: introduce support for Moschip USB ethernet

[Marek Vasut]

Hello Wolfgang,

> Dear Marek,
> 
> In message <201402172222.38911.marex at denx.de> you wrote:
> > > > +#define PHY_CMD1_READ		BIT(6)
> > > > +#define PHY_CMD1_WRITE		BIT(5)
> > > > +#define PHY_CMD1_PHYADDR	BIT(0)
> > > > +
> > > > +#define PHY_CMD2_PEND		BIT(7)
> > > > +#define PHY_CMD2_READY		BIT(6)
> > > 
> > > ...
> > > 
> > > As mentioned in patch # 1, I object against the use of these
> > > obfuscating BIT() macros.  Please do not use these; use plain
> > > readable code, that leaves no ambiguities to the reader.
> > 
> > Just to chime in real quick, Linux uses these 'BIT()' macros, but I
> > personally have no hard feelings about them either way.
> 
> Yes, certain developers have been using this style before.  This does
> not make it any better.  Fact is, that I have no way to tell what the
> code means.  BIT(0) can be expected to have any of the following
> meanings: 0x01, 0x80, 0x8000, 0x80000000, ... So I always have to look
> up the macro definition first, before I can unerstand it.  And then I
> have to compute in my head what the number actually means.
> 
> Compare: BIT(6) or 0x40 - what is easier to write, to read, and to
> understand?

(1 << 6) is easier for me to read honestly, because then I can quickly 
crosscheck it with the datasheet.

> You dump a register - either with the BDI, or with some printf().
> You get 0x27051956.  Is BIT(17) set?  Is bit 0x00020000 set?
> Which of these questions is easier to answer (even when you are sure
> that this is on a system where bit no. 0 is the LSB)?

You have a point with the endianness here.

Best regards,
Marek Vasut