Skip to contentSkip to conversation

BIT macro cleanup

PATCH In review Milind Arun Choudhary 3 participants 5 messages Mar 28, 2007 – Mar 30, 2007

Subsystems (13)
Network drivers VFS SCSI Kernel janitors +9 more subsystems
lore
Share & tools
lore

Milind Arun Choudhary

author
#0 lore raw

PATCH 1/1 BIT macro cleanup

BIT macro cleanup,now in bitops.h

Signed-off-by Milind Arun Choudhary <[email protected]>

---

 arch/ppc/platforms/chestnut.c               |    1 
 drivers/edac/edac_mc.h                      |    2 -
 drivers/i2c/busses/i2c-pxa.c                |   55 ++++++++++++++--------------
 drivers/net/eth16i.c                        |    1 
 drivers/net/meth.h                          |    3 -
 drivers/net/s2io.h                          |    1 
 drivers/net/wireless/hostap/hostap_common.h |    3 -
 drivers/scsi/nsp32.h                        |    4 --
 drivers/scsi/pcmcia/nsp_cs.h                |    1 
 drivers/serial/amba-pl011.c                 |   37 ++++++++----------
 drivers/video/cyber2000fb.c                 |   44 +++++++++++-----------
 fs/select.c                                 |    1 
 include/asm-mips/ip32/crime.h               |    3 -
 include/asm-mips/ip32/mace.h                |    3 -
 include/linux/bitops.h                      |    3 +
 include/linux/input.h                       |    2 -
 include/video/sstfb.h                       |    1 
 include/video/tdfx.h                        |    3 -
 18 files changed, 73 insertions(+), 95 deletions(-)
arch/ppc/platforms/chestnut.c +0 -1
index a764ae7..248bfdd 100644
@@ -48,7 +48,6 @@ extern void gen550_progress(char *, unsigned short);
 extern void gen550_init(int, struct uart_port *);
 extern void mv64360_pcibios_fixup(mv64x60_handle_t *bh);
 
-#define BIT(x) (1<<x)
 #define CHESTNUT_PRESERVE_MASK (BIT(MV64x60_CPU2DEV_0_WIN) | \
 				BIT(MV64x60_CPU2DEV_1_WIN) | \
 				BIT(MV64x60_CPU2DEV_2_WIN) | \
drivers/edac/edac_mc.h +0 -2
index 713444c..1d8c495 100644
@@ -79,8 +79,6 @@ extern int edac_debug_level;
 
 #endif  /* !CONFIG_EDAC_DEBUG */
 
-#define BIT(x) (1 << (x))
-
 #define PCI_VEND_DEV(vend, dev) PCI_VENDOR_ID_ ## vend, \
 	PCI_DEVICE_ID_ ## vend ## _ ## dev
drivers/i2c/busses/i2c-pxa.c +28 -27

73 lines not on this page · raw message

drivers/net/eth16i.c +0 -1
index 93283e3..41853b5 100644
@@ -170,7 +170,6 @@ static char *version =
 
 
 /* Few macros */
-#define BIT(a)		       ( (1 << (a)) )
 #define BITSET(ioaddr, bnum)   ((outb(((inb(ioaddr)) | (bnum)), ioaddr)))
 #define BITCLR(ioaddr, bnum)   ((outb(((inb(ioaddr)) & (~(bnum))), ioaddr)))
drivers/net/meth.h +0 -3
index 84960da..da92943 100644
@@ -28,9 +28,6 @@
 #define RX_BUFFER_OFFSET (sizeof(rx_status_vector)+2) /* staus vector + 2 bytes of padding */
 #define RX_BUCKET_SIZE 256
 
-#undef BIT
-#define BIT(x)	(1UL << (x))
-
 /* For more detailed explanations of what each field menas,
    see Nick's great comments to #defines below (or docs, if
    you are lucky enough toget hold of them :)*/
drivers/net/s2io.h +1 -0
index 0de0c65..5aa3be5 100644
@@ -14,6 +14,7 @@
 #define _S2IO_H
 
 #define TBD 0
+#undef BIT
 #define BIT(loc)		(0x8000000000000000ULL >> (loc))
 #define vBIT(val, loc, sz)	(((u64)val) << (64-loc-sz))
 #define INV(d)  ((d&0xff)<<24) | (((d>>8)&0xff)<<16) | (((d>>16)&0xff)<<8)| ((d>>24)&0xff)
drivers/net/wireless/hostap/hostap_common.h +1 -2
index 0162400..429c9dd 100644
@@ -3,8 +3,7 @@
 
 #include <linux/types.h>
 #include <linux/if_ether.h>
-
-#define BIT(x) (1 << (x))
+#include <linux/bitops.h>
 
 #define MAC2STR(a) (a)[0], (a)[1], (a)[2], (a)[3], (a)[4], (a)[5]
 #define MACSTR "%02x:%02x:%02x:%02x:%02x:%02x"
drivers/scsi/nsp32.h +0 -4
index a976e81..9779c5a 100644
@@ -68,10 +68,6 @@ static char * nsp32_model[] = {
 typedef u32 u32_le;
 typedef u16 u16_le;
 
-/*
- * MACRO
- */
-#define BIT(x)      (1UL << (x))
 
 /*
  * BASIC Definitions
drivers/scsi/pcmcia/nsp_cs.h +0 -1
index 9102cbd..0a3e2d1 100644
@@ -26,7 +26,6 @@
 /************************************
  * Some useful macros...
  */
-#define BIT(x)      (1L << (x))
 
 /* SCSI initiator must be ID 7 */
 #define NSP_INITIATOR_ID  7
drivers/serial/amba-pl011.c +17 -20
index 44639e7..dfff378 100644
@@ -53,6 +53,9 @@
 #include <asm/io.h>
 #include <asm/sizes.h>
 
+#define SETBITS(data,mask)    (data |= mask)
+#define CLRBITS(data,mask)    (data &= ~mask)
+
 #define UART_NR			14
 
 #define SERIAL_AMBA_MAJOR	204

@@ -262,15 +265,11 @@ static unsigned int pl01x_get_mctrl(struct uart_port *port)
 	unsigned int result = 0;
 	unsigned int status = readw(uap->port.membase + UART01x_FR);
 
-#define BIT(uartbit, tiocmbit)		\
-	if (status & uartbit)		\
-		result |= tiocmbit
+	result |= (status & UART01x_FR_DCD) ? TIOCM_CAR  : 0 ;
+	result |= (status & UART01x_FR_DSR) ? TIOCM_DSR  : 0 ;
+	result |= (status & UART01x_FR_CTS) ? TIOCM_CTS  : 0 ;
+	result |= (status & UART011_FR_RI)  ? TIOCM_RNG  : 0 ;
 
-	BIT(UART01x_FR_DCD, TIOCM_CAR);
-	BIT(UART01x_FR_DSR, TIOCM_DSR);
-	BIT(UART01x_FR_CTS, TIOCM_CTS);
-	BIT(UART011_FR_RI, TIOCM_RNG);
-#undef BIT
 	return result;
 }
 

@@ -281,18 +280,16 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
 
 	cr = readw(uap->port.membase + UART011_CR);
 
-#define	BIT(tiocmbit, uartbit)		\
-	if (mctrl & tiocmbit)		\
-		cr |= uartbit;		\
-	else				\
-		cr &= ~uartbit
-
-	BIT(TIOCM_RTS, UART011_CR_RTS);
-	BIT(TIOCM_DTR, UART011_CR_DTR);
-	BIT(TIOCM_OUT1, UART011_CR_OUT1);
-	BIT(TIOCM_OUT2, UART011_CR_OUT2);
-	BIT(TIOCM_LOOP, UART011_CR_LBE);
-#undef BIT
+       (mctrl & TIOCM_RTS)  ?
+               SETBITS(cr, UART011_CR_DTR)  : CLRBITS(cr, UART011_CR_DTR);
+       (mctrl & TIOCM_DTR)  ?
+               SETBITS(cr, UART011_CR_DTR)  : CLRBITS(cr, UART011_CR_DTR);
+       (mctrl & TIOCM_OUT1) ?
+               SETBITS(cr, UART011_CR_OUT1) : CLRBITS(cr, UART011_CR_OUT1);
+       (mctrl & TIOCM_OUT2) ?
+               SETBITS(cr, UART011_CR_OUT2) : CLRBITS(cr, UART011_CR_OUT2);
+       (mctrl & TIOCM_LOOP) ?
+               SETBITS(cr, UART011_CR_LBE)  : CLRBITS(cr, UART011_CR_LBE);
 
 	writew(cr, uap->port.membase + UART011_CR);
 }
drivers/video/cyber2000fb.c +22 -22

72 lines not on this page · raw message

fs/select.c +0 -1
index fe0893a..4bbe8ed 100644
@@ -180,7 +180,6 @@ get_max:
 	return max;
 }
 
-#define BIT(i)		(1UL << ((i)&(__NFDBITS-1)))
 #define MEM(i,m)	((m)+(unsigned)(i)/__NFDBITS)
 #define ISSET(i,m)	(((i)&*(m)) != 0)
 #define SET(i,m)	(*(m) |= (i))
include/asm-mips/ip32/crime.h +0 -3
index a13702f..7c36b0e 100644
@@ -17,9 +17,6 @@
  */
 #define CRIME_BASE	0x14000000	/* physical */
 
-#undef BIT
-#define BIT(x)	(1UL << (x))
-
 struct sgi_crime {
 	volatile unsigned long id;
 #define CRIME_ID_MASK			0xff
include/asm-mips/ip32/mace.h +0 -3
index 990082c..d08d7c6 100644
@@ -17,9 +17,6 @@
  */
 #define MACE_BASE	0x1f000000	/* physical */
 
-#undef BIT
-#define BIT(x) (1UL << (x))
-
 /*
  * PCI interface
  */
include/linux/bitops.h +3 -0
index 638165f..1db7815 100644
@@ -8,6 +8,9 @@
  */
 #include <asm/bitops.h>
 
+#define BIT(nr)	(1UL << ((nr) % BITS_PER_LONG))
+#define LLBIT(nr) (1ULL << (nr))
+
 static __inline__ int get_bitmask_order(unsigned int count)
 {
 	int order;
include/linux/input.h +1 -1
index bde65c8..5ea8f65 100644
@@ -908,9 +908,9 @@ struct ff_effect {
 #include <linux/fs.h>
 #include <linux/timer.h>
 #include <linux/mod_devicetable.h>
+#include <linux/bitops.h>
 
 #define NBITS(x) (((x)/BITS_PER_LONG)+1)
-#define BIT(x)	(1UL<<((x)%BITS_PER_LONG))
 #define LONG(x) ((x)/BITS_PER_LONG)
 
 #define INPUT_KEYCODE(dev, scancode) ((dev->keycodesize == 1) ? ((u8*)dev->keycode)[scancode] : \
include/video/sstfb.h +0 -1
index baa163f..b52f073 100644
@@ -68,7 +68,6 @@
 #  define print_var(X,Y...)
 #endif
 
-#define BIT(x)		(1ul<<(x))
 #define POW2(x)		(1ul<<(x))
 
 /*
include/video/tdfx.h +0 -3
index c1cc94b..ac6d0f1 100644
@@ -77,9 +77,6 @@
 
 #define COMMAND_3D      (0x00200000 + 0x120)
 
-/* register bitfields (not all, only as needed) */
-
-#define BIT(x) (1UL << (x))
 
 /* COMMAND_2D reg. values */
 #define TDFX_ROP_COPY        0xcc     // src
Signature · 1 line
Milind Arun Choudhary
#2 lore raw
Quoted 8 lines · Alexey Dobriyan
Quoted 6 lines · Milind Arun Choudhary
include/linux/bitops.h +0 -0
@@ -8,6 +8,9 @@
  */
 #include <asm/bitops.h>
Quoted 9 lines

include/linux/bitops.h line 12

Quoted 2 lines
include/linux/bitops.h +1 -0
+#define BIT(nr)	(1UL << ((nr) % BITS_PER_LONG))

I think this would be a disaster because something like

	BIT(123)

would not even generate a warning.

There were a discussion on this, at KJ, when BIT was first used with a modular operation. I said the same thing as you do now, but a big user of BIT is the input-subsystem who defined their BIT as above. Also it was mentioned that the compiler can only find the statical errors, a variable input can break it in runtime.
+ if we _really_ want to check the tree for such warnings, it is easy to remove the modular operation temporarily (and keep away of input/)

I don't say I like this, just that it is a choose between possible errors. Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

author
#3 lore raw
Quoted 20 lines · Richard Knutsson
Quoted 11 lines · Alexey Dobriyan
Quoted 2 lines · Milind Arun Choudhary

+#define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))

I think this would be a disaster because something like

	BIT(123)

would not even generate a warning.

There were a discussion on this, at KJ, when BIT was first used with a modular operation. I said the same thing as you do now, but a big user of BIT is the input-subsystem who defined their BIT as above. Also it was mentioned that the compiler can only find the statical errors, a variable input can break it in runtime.
+ if we _really_ want to check the tree for such warnings, it is easy to remove the modular operation temporarily (and keep away of input/)

Quoted 1 line

I don't say I like this, just that it is a choose between possible errors.

as discussed in earlier thread there are two kinds of users of BIT
1. input sybsystem which needs the "% BITS_PER_LONG"
2. & all other who don't, so that they are warned when the bit no overflows I think a simple macro cant serve both the purposes

so IMHO there should be two macros
+#define BIT(nr) (1UL << (nr))
which server the first case

&
+#define XXXBITXXX(nr) (1UL << ((nr) % BITS_PER_LONG))
for input users
earlier i suggested a name BITWRAP
but some people hate it i think
'm fine with any other name as well
inputs....suggest a name

so that we can still keep these two macros (along with LLBIT) in bitops.h & use apporpriately

only hassle is the whole input tree is to be searched & replaced for BIT is this OK

Signature · 1 line
Milind Arun Choudhary
#4 lore raw
In reply to Milind Arun Choudhary
Quoted 6 lines · Milind Arun Choudhary

BIT macro cleanup,now in bitops.h

Signed-off-by Milind Arun Choudhary <[email protected]>

---

<snip>

drivers/net/s2io.h line 20

Quoted 13 lines
drivers/net/s2io.h +1 -0
index 0de0c65..5aa3be5 100644
@@ -14,6 +14,7 @@
 #define _S2IO_H
 
 #define TBD 0
+#undef BIT
 #define BIT(loc)		(0x8000000000000000ULL >> (loc))
 #define vBIT(val, loc, sz)	(((u64)val) << (64-loc-sz))
 #define INV(d)  ((d&0xff)<<24) | (((d>>8)&0xff)<<16) | (((d>>16)&0xff)<<8)| ((d>>24)&0xff)

Why not use "LLBIT(63 - loc)" instead?

Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors