[U-Boot] [PATCH] Introduce BIT macro

[Matthias Kaehlcke <matthias@kaehlcke.net>]

Hi Wolfgang,

El Thu, Feb 11, 2010 at 09:46:22PM +0100 Wolfgang Denk ha dit:

> Dear Matthias Kaehlcke,
> 
> In message <20100211200302.GE15905@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@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 for your explications. i now understand that the BIT macro
rather can obscure what is going on, though at the first glance it
seems to improve readability.

-- 
Matthias Kaehlcke
Embedded Linux Developer
Barcelona

        They that can give up essential liberty to obtain a little
           temporary safety deserve neither liberty nor safety
                         (Benjamin Franklin)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-