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

Marek Vasut
2014-02-18T11:24:06Z

Hello Wolfgang,

> Dear Marek,
>
> In message <201402172222.38911.marex@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