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

[Wolfgang Denk]

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?

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)?

It's all about writing portable and maintainable code.  

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I've finally learned what `upward compatible' means. It means we get
to keep all our old mistakes." - Dennie van Tassel