| Message ID | 1392665727-5734-2-git-send-email-gsi@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Dear Gerhard Sittig, In message <1392665727-5734-2-git-send-email-gsi@denx.de> you wrote: > several compile units and local header files re-invented the > BIT() macro, move BIT() and BITMASK() declarations into the > common.h header file and adjust call sites ... > diff --git a/include/common.h b/include/common.h > index 221b7768c5be..49885f3c01bf 100644 > --- a/include/common.h > +++ b/include/common.h > @@ -967,6 +967,9 @@ static inline phys_addr_t map_to_sysmem(const void *ptr) > #define ALIGN(x,a) __ALIGN_MASK((x),(typeof(x))(a)-1) > #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask)) > > +#define BIT(n) (1U << (n)) > +#define BITMASK(bits) (BIT(bits) - 1) > + I'm sorry, but these macros are ugly and make the code harder to read. Also, they are inherently non-portable. I guess you are not aware that "bit 0" is the MSB on Power architecture, not the LSB as you expect in your definition. I strongly reommend NOT to use any such macros. Adding these to common.h could be considered as an invitation to use them, which is the opposite of what I want. So sorry, but this is a clear NAK. Best regards, Wolfgang Denk
On Mon, Feb 17, 2014 at 22:01 +0100, Wolfgang Denk wrote: > > Dear Gerhard Sittig, > > In message <1392665727-5734-2-git-send-email-gsi@denx.de> you wrote: > > several compile units and local header files re-invented the > > BIT() macro, move BIT() and BITMASK() declarations into the > > common.h header file and adjust call sites > ... > > diff --git a/include/common.h b/include/common.h > > index 221b7768c5be..49885f3c01bf 100644 > > --- a/include/common.h > > +++ b/include/common.h > > @@ -967,6 +967,9 @@ static inline phys_addr_t map_to_sysmem(const void *ptr) > > #define ALIGN(x,a) __ALIGN_MASK((x),(typeof(x))(a)-1) > > #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask)) > > > > +#define BIT(n) (1U << (n)) > > +#define BITMASK(bits) (BIT(bits) - 1) > > + > > I'm sorry, but these macros are ugly and make the code harder to read. > Also, they are inherently non-portable. I guess you are not aware > that "bit 0" is the MSB on Power architecture, not the LSB as you > expect in your definition. > > I strongly reommend NOT to use any such macros. Adding these to > common.h could be considered as an invitation to use them, which is > the opposite of what I want. > > So sorry, but this is a clear NAK. That's fine with me. So this patch can get dropped and nothing changes for the existing code base. The MCS7830 driver will then need a respin (after I took in some more feedback, of course). There was the question of whether to use the BIT(5) macro or re-invented (1 << 5) or 0x20 numbers. The feedback now allows a clear answer. :) virtually yours Gerhard Sittig
diff --git a/arch/arm/cpu/arm1176/tnetv107x/clock.c b/arch/arm/cpu/arm1176/tnetv107x/clock.c index 3708b6f59f49..a30b666b4d76 100644 --- a/arch/arm/cpu/arm1176/tnetv107x/clock.c +++ b/arch/arm/cpu/arm1176/tnetv107x/clock.c @@ -13,8 +13,6 @@ #define CLOCK_BASE TNETV107X_CLOCK_CONTROL_BASE #define PSC_BASE TNETV107X_PSC_BASE -#define BIT(x) (1 << (x)) - #define MAX_PREDIV 64 #define MAX_POSTDIV 8 #define MAX_MULT 512 diff --git a/arch/arm/cpu/arm926ejs/davinci/cpu.c b/arch/arm/cpu/arm926ejs/davinci/cpu.c index ff6114775728..74c3d5d936c2 100644 --- a/arch/arm/cpu/arm926ejs/davinci/cpu.c +++ b/arch/arm/cpu/arm926ejs/davinci/cpu.c @@ -28,8 +28,6 @@ DECLARE_GLOBAL_DATA_PTR; #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/arch/arm/include/asm/arch-am33xx/cpu.h b/arch/arm/include/asm/arch-am33xx/cpu.h index 9febfa2719a9..4df504a12939 100644 --- a/arch/arm/include/asm/arch-am33xx/cpu.h +++ b/arch/arm/include/asm/arch-am33xx/cpu.h @@ -11,13 +11,14 @@ #ifndef _AM33XX_CPU_H #define _AM33XX_CPU_H +#include <common.h> + #if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__)) #include <asm/types.h> #endif /* !(__KERNEL_STRICT_NAMES || __ASSEMBLY__) */ #include <asm/arch/hardware.h> -#define BIT(x) (1 << x) #define CL_BIT(x) (0 << x) /* Timer register bits */ diff --git a/arch/arm/include/asm/arch-ixp/ixp425.h b/arch/arm/include/asm/arch-ixp/ixp425.h index c2e9c820495a..3bceb445b0ff 100644 --- a/arch/arm/include/asm/arch-ixp/ixp425.h +++ b/arch/arm/include/asm/arch-ixp/ixp425.h @@ -14,7 +14,7 @@ #ifndef _ASM_ARM_IXP425_H_ #define _ASM_ARM_IXP425_H_ -#define BIT(x) (1<<(x)) +#include <common.h> /* FIXME: Only this does work for u-boot... find out why... [RS] */ #define UBOOT_REG_FIX 1 diff --git a/arch/arm/include/asm/arch-omap5/cpu.h b/arch/arm/include/asm/arch-omap5/cpu.h index fb5a568b698b..7cd245bca915 100644 --- a/arch/arm/include/asm/arch-omap5/cpu.h +++ b/arch/arm/include/asm/arch-omap5/cpu.h @@ -10,6 +10,8 @@ #ifndef _CPU_H #define _CPU_H +#include <common.h> + #if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__)) #include <asm/types.h> #endif /* !(__KERNEL_STRICT_NAMES || __ASSEMBLY__) */ @@ -99,8 +101,6 @@ struct watchdog { #endif /* __ASSEMBLY__ */ #endif /* __KERNEL_STRICT_NAMES */ -#define BIT(x) (1 << (x)) - #define WD_UNLOCK1 0xAAAA #define WD_UNLOCK2 0x5555 diff --git a/arch/arm/include/asm/arch-tegra20/dc.h b/arch/arm/include/asm/arch-tegra20/dc.h index 20790b6c0e90..f1a67e8b63d9 100644 --- a/arch/arm/include/asm/arch-tegra20/dc.h +++ b/arch/arm/include/asm/arch-tegra20/dc.h @@ -8,6 +8,8 @@ #ifndef __ASM_ARCH_TEGRA_DC_H #define __ASM_ARCH_TEGRA_DC_H +#include <common.h> + /* Register definitions for the Tegra display controller */ /* CMD register 0x000 ~ 0x43 */ @@ -351,8 +353,6 @@ struct dc_ctlr { struct dc_winbuf_reg winbuf; /* WINBUF A/B/C 0x800 ~ 0x80a */ }; -#define BIT(pos) (1U << pos) - /* DC_CMD_DISPLAY_COMMAND 0x032 */ #define CTRL_MODE_SHIFT 5 #define CTRL_MODE_MASK (0x3 << CTRL_MODE_SHIFT) diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c index 7a62cc33613c..abcedc210211 100644 --- a/drivers/mtd/nand/jz4740_nand.c +++ b/drivers/mtd/nand/jz4740_nand.c @@ -16,7 +16,6 @@ #define JZ_NAND_CMD_ADDR (JZ_NAND_DATA_ADDR + 0x8000) #define JZ_NAND_ADDR_ADDR (JZ_NAND_DATA_ADDR + 0x10000) -#define BIT(x) (1 << (x)) #define JZ_NAND_ECC_CTRL_ENCODING BIT(3) #define JZ_NAND_ECC_CTRL_RS BIT(2) #define JZ_NAND_ECC_CTRL_RESET BIT(1) diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c index 50167aab63a8..94b9b9e71086 100644 --- a/drivers/net/cpsw.c +++ b/drivers/net/cpsw.c @@ -26,7 +26,6 @@ #include <phy.h> #include <asm/arch/cpu.h> -#define BITMASK(bits) (BIT(bits) - 1) #define PHY_REG_MASK 0x1f #define PHY_ID_MASK 0x1f #define NUM_DESCS (PKTBUFSRX * 2) diff --git a/drivers/net/npe/include/IxOsalOsIxp400.h b/drivers/net/npe/include/IxOsalOsIxp400.h index 104c733e3b84..dec8ebb00224 100644 --- a/drivers/net/npe/include/IxOsalOsIxp400.h +++ b/drivers/net/npe/include/IxOsalOsIxp400.h @@ -23,7 +23,7 @@ #ifndef IxOsalOsIxp400_H #define IxOsalOsIxp400_H -#define BIT(x) (1<<(x)) +#include <common.h> #define IXP425_EthA_BASE 0xc8009000 #define IXP425_EthB_BASE 0xc800a000 diff --git a/drivers/spi/andes_spi.h b/drivers/spi/andes_spi.h index b7d294599a17..26c6250d9a90 100644 --- a/drivers/spi/andes_spi.h +++ b/drivers/spi/andes_spi.h @@ -10,6 +10,8 @@ #ifndef __ANDES_SPI_H #define __ANDES_SPI_H +#include <common.h> + struct andes_spi_regs { unsigned int apb; /* 0x00 - APB SPI interface setting */ unsigned int pio; /* 0x04 - PIO reg */ @@ -23,8 +25,6 @@ struct andes_spi_regs { unsigned int ver; /* 0x3c - SPI version reg */ }; -#define BIT(x) (1 << (x)) - /* 0x00 - APB SPI interface setting register */ #define ANDES_SPI_APB_BAUD(x) (((x) & 0xff) < 0) #define ANDES_SPI_APB_CSHT(x) (((x) & 0xf) < 16) diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h index 33f69b5ca98a..1901faefc0e7 100644 --- a/drivers/spi/davinci_spi.h +++ b/drivers/spi/davinci_spi.h @@ -9,6 +9,8 @@ #ifndef _DAVINCI_SPI_H_ #define _DAVINCI_SPI_H_ +#include <common.h> + struct davinci_spi_regs { dv_reg gcr0; /* 0x00 */ dv_reg gcr1; /* 0x04 */ @@ -36,8 +38,6 @@ struct davinci_spi_regs { dv_reg intvec1; /* 0x64 */ }; -#define BIT(x) (1 << (x)) - /* SPIGCR0 */ #define SPIGCR0_SPIENA_MASK 0x1 #define SPIGCR0_SPIRST_MASK 0x0 diff --git a/include/common.h b/include/common.h index 221b7768c5be..49885f3c01bf 100644 --- a/include/common.h +++ b/include/common.h @@ -967,6 +967,9 @@ static inline phys_addr_t map_to_sysmem(const void *ptr) #define ALIGN(x,a) __ALIGN_MASK((x),(typeof(x))(a)-1) #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask)) +#define BIT(n) (1U << (n)) +#define BITMASK(bits) (BIT(bits) - 1) + /* * ARCH_DMA_MINALIGN is defined in asm/cache.h for each architecture. It * is used to align DMA buffers.
several compile units and local header files re-invented the BIT() macro, move BIT() and BITMASK() declarations into the common.h header file and adjust call sites Cc: Cyril Chemparathy <cyril@ti.com> Cc: David Brownell <dbrownell@users.sourceforge.net> Cc: Chandan Nath <chandan.nath@ti.com> Cc: Wolfgang Denk <wd@denx.de> Cc: Mugunthan V N <mugunthanvnm@ti.com> Cc: Wei Ni <wni@nvidia.com> Cc: Xiangfu Liu <xiangfu@openmobilefree.net> Cc: Macpaul Lin <macpaul@andestech.com> Cc: Sekhar Nori <nsekhar@ti.com> Signed-off-by: Gerhard Sittig <gsi@denx.de> --- arch/arm/cpu/arm1176/tnetv107x/clock.c | 2 -- arch/arm/cpu/arm926ejs/davinci/cpu.c | 2 -- arch/arm/include/asm/arch-am33xx/cpu.h | 3 ++- arch/arm/include/asm/arch-ixp/ixp425.h | 2 +- arch/arm/include/asm/arch-omap5/cpu.h | 4 ++-- arch/arm/include/asm/arch-tegra20/dc.h | 4 ++-- drivers/mtd/nand/jz4740_nand.c | 1 - drivers/net/cpsw.c | 1 - drivers/net/npe/include/IxOsalOsIxp400.h | 2 +- drivers/spi/andes_spi.h | 4 ++-- drivers/spi/davinci_spi.h | 4 ++-- include/common.h | 3 +++ 12 files changed, 15 insertions(+), 17 deletions(-)