[U-Boot] [PATCH] Introduce BIT macro

[Wolfgang Denk]

Dear Matthias Kaehlcke,

In message <20100211200302.GE15905 at darwin> you wrote:
> Most code defines constants for bit positions by means of "(1 << n)". The Linux

Most code does? I disagree. Only minor parts of the code do, and I
generally tend to consider this bad style.

> kernel defines the BIT macro for this purpose, providing a uniform and more
> readable way to define these constants. This patch adds the BIT macro to
> linux/bitops.h, and removes its local definitions from davinci and ixp code.

I am strongly tempted to reject this patch, as it does not improve
anything. On contrary, it makes things worse. I'll try to explain
why.

> Signed-off-by: Matthias Kaehlcke <matthias at kaehlcke.net>
> ---
>  cpu/arm926ejs/davinci/cpu.c          |    2 --
>  cpu/ixp/npe/include/IxOsalOsIxp400.h |    2 --
>  include/asm-arm/arch-ixp/ixp425.h    |    2 --
>  include/linux/bitops.h               |    1 +
>  4 files changed, 1 insertions(+), 6 deletions(-)

With the local definitions in ARM related header files you could at
least make an edicated guess what the code might mean.

By moving the definition into a globally valid and visible location
the inherent problems of sucha definition become obvious.


If you see a line of code like

	value |= 0x0020;	or	value |= (1 << 5)
or
	vlaue &= ~0x0020;	or	vlaue &= ~(1 << 5)

you know immediately and exactly what that means - there is not the
slightest bit of doubt about the meaning of the code.

You probably thing that

	value |= BIT(5);

is easier to read. But is this really so?

Why do you prefer to write (or read) "BIT(5)" instead of "1 << 5"?
Because some of your user manuals talk about "bit5" etc.?

Guess why there is not a single use of BIT(x) in PowerPC code in
U-Boot?  For the Power Architecture, bit 0 is by definition the most
significant bit. What does this mean for a simple value as "(1 << 5)"?


Size of object		(1 << 5) is ...
     8 bit		bit 2
    16 bit		bit 10
    32 bit		bit 26
    64 bit		bit 58

You see? Even if you are aware that bit 0 is the MSB, the notation of
BIT(x) makes no sense, as it at least also depends on the object size.


Forget BIT(x). It's unportable and misleading at best.

If you want to improve U-Boot code, then please come up with cleanup
patches to remove this construct from U-Boot all together.

Thanks.

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
The thing is, as you progress in the Craft,  you'll  learn  there  is
another rule... When you break rules, break 'em good and hard.
                                    - Terry Pratchett, _Wyrd Sisters_