[U-Boot] [PATCH] Introduce BIT macro

[U-Boot] [PATCH] Introduce BIT macro

[Matthias Kaehlcke]

Most code defines constants for bit positions by means of "(1 << n)". The Linux
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.

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

diff --git a/cpu/arm926ejs/davinci/cpu.c b/cpu/arm926ejs/davinci/cpu.c
index fc3551c..c274ddd 100644
--- a/cpu/arm926ejs/davinci/cpu.c
+++ b/cpu/arm926ejs/davinci/cpu.c
@@ -40,8 +40,6 @@
 #define PLLC_PLLDIV8	0x170
 #define PLLC_PLLDIV9	0x174
 
-#define BIT(x)		(1 << (x))
-
 /* SOC-specific pll info */
 #ifdef CONFIG_SOC_DM355
 #define ARM_PLLDIV	PLLC_PLLDIV1
diff --git a/cpu/ixp/npe/include/IxOsalOsIxp400.h b/cpu/ixp/npe/include/IxOsalOsIxp400.h
index 44a94fb..5e09651 100644
--- a/cpu/ixp/npe/include/IxOsalOsIxp400.h
+++ b/cpu/ixp/npe/include/IxOsalOsIxp400.h
@@ -47,8 +47,6 @@
 #ifndef IxOsalOsIxp400_H
 #define IxOsalOsIxp400_H
 
-#define BIT(x) (1<<(x))
-
 #define IXP425_EthA_BASE	0xc8009000
 #define IXP425_EthB_BASE	0xc800a000
 
diff --git a/include/asm-arm/arch-ixp/ixp425.h b/include/asm-arm/arch-ixp/ixp425.h
index 2114437..689e1ac 100644
--- a/include/asm-arm/arch-ixp/ixp425.h
+++ b/include/asm-arm/arch-ixp/ixp425.h
@@ -14,8 +14,6 @@
 #ifndef _ASM_ARM_IXP425_H_
 #define _ASM_ARM_IXP425_H_
 
-#define BIT(x)	(1<<(x))
-
 /* FIXME: Only this does work for u-boot... find out why... [RS] */
 #define UBOOT_REG_FIX 1
 #ifdef UBOOT_REG_FIX
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index e724310..c098c9a 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -104,6 +104,7 @@ static inline unsigned int generic_hweight8(unsigned int w)
 	return (res & 0x0F) + ((res >> 4) & 0x0F);
 }
 
+#define BIT(nr)			(1UL << (nr))
 #define BIT_MASK(nr)		(1UL << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)		((nr) / BITS_PER_LONG)
 
-- 
1.6.5

[U-Boot] [PATCH] Introduce BIT macro

[Wolfgang Denk]

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.

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@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_

[U-Boot] [PATCH] Introduce BIT macro

[Matthias Kaehlcke]

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                  `-