Hi all
working towards the cleanup of BIT macro,
I've added one to <linux/bitops.h> & cleaned some obvious users.
include/linux/input.h also has a BIT macro
which does a wrap
so currently i've done something like
+#undef BIT
#define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))
Is it advisible to move this macro to bitops.h with some other name
+#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
& make the whole input subsystem use it
The change is huge, more than 125 files using input.h
& almost all use the BIT macro.
discussion on KJ
http://lists.osdl.org/pipermail/kernel-janitors/2007-February/008442.html
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 | 4 ++
include/linux/input.h | 4 +-
include/video/sstfb.h | 1
include/video/tdfx.h | 3 -
18 files changed, 76 insertions(+), 95 deletions(-)
diff --git a/arch/ppc/platforms/chestnut.c b/arch/ppc/platforms/chestnut.c
index a764ae7..248bfdd 100644
--- a/arch/ppc/platforms/chestnut.c
+++ b/arch/ppc/platforms/chestnut.c
@@ -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) | \
diff --git a/drivers/edac/edac_mc.h b/drivers/edac/edac_mc.h
index 713444c..1d8c495 100644
--- a/drivers/edac/edac_mc.h
+++ b/drivers/edac/edac_mc.h
@@ -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
diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 14e83d0..abed806 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -82,7 +82,8 @@ struct bits {
const char *set;
const char *unset;
};
-#define BIT(m, s, u) { .mask = m, .set = s, .unset = u }
+
+#define INIT_BITS(m, s, u) { .mask = m, .set = s, .unset = u }
static inline void
decode_bits(const char *prefix, const struct bits *bits, int num, u32 val)
@@ -97,17 +98,17 @@ decode_bits(const char *prefix, const struct bits
*bits, int num, u32 val)
}
static const struct bits isr_bits[] = {
- BIT(ISR_RWM, "RX", "TX"),
- BIT(ISR_ACKNAK, "NAK", "ACK"),
- BIT(ISR_UB, "Bsy", "Rdy"),
- BIT(ISR_IBB, "BusBsy", "BusRdy"),
- BIT(ISR_SSD, "SlaveStop", NULL),
- BIT(ISR_ALD, "ALD", NULL),
- BIT(ISR_ITE, "TxEmpty", NULL),
- BIT(ISR_IRF, "RxFull", NULL),
- BIT(ISR_GCAD, "GenCall", NULL),
- BIT(ISR_SAD, "SlaveAddr", NULL),
- BIT(ISR_BED, "BusErr", NULL),
+ INIT_BITS(ISR_RWM, "RX", "TX"),
+ INIT_BITS(ISR_ACKNAK, "NAK", "ACK"),
+ INIT_BITS(ISR_UB, "Bsy", "Rdy"),
+ INIT_BITS(ISR_IBB, "BusBsy", "BusRdy"),
+ INIT_BITS(ISR_SSD, "SlaveStop", NULL),
+ INIT_BITS(ISR_ALD, "ALD", NULL),
+ INIT_BITS(ISR_ITE, "TxEmpty", NULL),
+ INIT_BITS(ISR_IRF, "RxFull", NULL),
+ INIT_BITS(ISR_GCAD, "GenCall", NULL),
+ INIT_BITS(ISR_SAD, "SlaveAddr", NULL),
+ INIT_BITS(ISR_BED, "BusErr", NULL),
};
static void decode_ISR(unsigned int val)
@@ -117,21 +118,21 @@ static void decode_ISR(unsigned int val)
}
static const struct bits icr_bits[] = {
- BIT(ICR_START, "START", NULL),
- BIT(ICR_STOP, "STOP", NULL),
- BIT(ICR_ACKNAK, "ACKNAK", NULL),
- BIT(ICR_TB, "TB", NULL),
- BIT(ICR_MA, "MA", NULL),
- BIT(ICR_SCLE, "SCLE", "scle"),
- BIT(ICR_IUE, "IUE", "iue"),
- BIT(ICR_GCD, "GCD", NULL),
- BIT(ICR_ITEIE, "ITEIE", NULL),
- BIT(ICR_IRFIE, "IRFIE", NULL),
- BIT(ICR_BEIE, "BEIE", NULL),
- BIT(ICR_SSDIE, "SSDIE", NULL),
- BIT(ICR_ALDIE, "ALDIE", NULL),
- BIT(ICR_SADIE, "SADIE", NULL),
- BIT(ICR_UR, "UR", "ur"),
+ INIT_BITS(ICR_START, "START", NULL),
+ INIT_BITS(ICR_STOP, "STOP", NULL),
+ INIT_BITS(ICR_ACKNAK, "ACKNAK", NULL),
+ INIT_BITS(ICR_TB, "TB", NULL),
+ INIT_BITS(ICR_MA, "MA", NULL),
+ INIT_BITS(ICR_SCLE, "SCLE", "scle"),
+ INIT_BITS(ICR_IUE, "IUE", "iue"),
+ INIT_BITS(ICR_GCD, "GCD", NULL),
+ INIT_BITS(ICR_ITEIE, "ITEIE", NULL),
+ INIT_BITS(ICR_IRFIE, "IRFIE", NULL),
+ INIT_BITS(ICR_BEIE, "BEIE", NULL),
+ INIT_BITS(ICR_SSDIE, "SSDIE", NULL),
+ INIT_BITS(ICR_ALDIE, "ALDIE", NULL),
+ INIT_BITS(ICR_SADIE, "SADIE", NULL),
+ INIT_BITS(ICR_UR, "UR", "ur"),
};
static void decode_ICR(unsigned int val)
diff --git a/drivers/net/eth16i.c b/drivers/net/eth16i.c
index 93283e3..41853b5 100644
--- a/drivers/net/eth16i.c
+++ b/drivers/net/eth16i.c
@@ -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)))
diff --git a/drivers/net/meth.h b/drivers/net/meth.h
index 84960da..da92943 100644
--- a/drivers/net/meth.h
+++ b/drivers/net/meth.h
@@ -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 :)*/
diff --git a/drivers/net/s2io.h b/drivers/net/s2io.h
index 0de0c65..5aa3be5 100644
--- a/drivers/net/s2io.h
+++ b/drivers/net/s2io.h
@@ -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)
diff --git a/drivers/net/wireless/hostap/hostap_common.h
b/drivers/net/wireless/hostap/hostap_common.h
index 0162400..429c9dd 100644
--- a/drivers/net/wireless/hostap/hostap_common.h
+++ b/drivers/net/wireless/hostap/hostap_common.h
@@ -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"
diff --git a/drivers/scsi/nsp32.h b/drivers/scsi/nsp32.h
index a976e81..9779c5a 100644
--- a/drivers/scsi/nsp32.h
+++ b/drivers/scsi/nsp32.h
@@ -68,10 +68,6 @@ static char * nsp32_model[] = {
typedef u32 u32_le;
typedef u16 u16_le;
-/*
- * MACRO
- */
-#define BIT(x) (1UL << (x))
/*
* BASIC Definitions
diff --git a/drivers/scsi/pcmcia/nsp_cs.h b/drivers/scsi/pcmcia/nsp_cs.h
index 9102cbd..0a3e2d1 100644
--- a/drivers/scsi/pcmcia/nsp_cs.h
+++ b/drivers/scsi/pcmcia/nsp_cs.h
@@ -26,7 +26,6 @@
/************************************
* Some useful macros...
*/
-#define BIT(x) (1L << (x))
/* SCSI initiator must be ID 7 */
#define NSP_INITIATOR_ID 7
diff --git a/drivers/serial/amba-pl011.c b/drivers/serial/amba-pl011.c
index 44639e7..dfff378 100644
--- a/drivers/serial/amba-pl011.c
+++ b/drivers/serial/amba-pl011.c
@@ -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);
}
diff --git a/drivers/video/cyber2000fb.c b/drivers/video/cyber2000fb.c
index 7a6eeda..f21b20f 100644
--- a/drivers/video/cyber2000fb.c
+++ b/drivers/video/cyber2000fb.c
@@ -550,7 +550,7 @@ cyber2000fb_decode_crtc(struct par_info *hw,
struct cfb_info *cfb,
{
u_int Htotal, Hblankend, Hsyncend;
u_int Vtotal, Vdispend, Vblankstart, Vblankend, Vsyncstart, Vsyncend;
-#define BIT(v,b1,m,b2) (((v >> b1) & m) << b2)
+#define BITMASK(v,b1,m,b2) (((v >> b1) & m) << b2)
hw->crtc[13] = hw->pitch;
hw->crtc[17] = 0xe3;
@@ -570,13 +570,13 @@ cyber2000fb_decode_crtc(struct par_info *hw,
struct cfb_info *cfb,
Hblankend = (Htotal - 4*8) >> 3;
- hw->crtc[3] = BIT(Hblankend, 0, 0x1f, 0) |
- BIT(1, 0, 0x01, 7);
+ hw->crtc[3] = BITMASK(Hblankend, 0, 0x1f, 0) |
+ BITMASK(1, 0, 0x01, 7);
Hsyncend = (var->xres + var->right_margin + var->hsync_len) >> 3;
- hw->crtc[5] = BIT(Hsyncend, 0, 0x1f, 0) |
- BIT(Hblankend, 5, 0x01, 7);
+ hw->crtc[5] = BITMASK(Hsyncend, 0, 0x1f, 0) |
+ BITMASK(Hblankend, 5, 0x01, 7);
Vdispend = var->yres - 1;
Vsyncstart = var->yres + var->lower_margin;
@@ -591,20 +591,20 @@ cyber2000fb_decode_crtc(struct par_info *hw,
struct cfb_info *cfb,
Vblankend = Vtotal - 10;
hw->crtc[6] = Vtotal;
- hw->crtc[7] = BIT(Vtotal, 8, 0x01, 0) |
- BIT(Vdispend, 8, 0x01, 1) |
- BIT(Vsyncstart, 8, 0x01, 2) |
- BIT(Vblankstart,8, 0x01, 3) |
- BIT(1, 0, 0x01, 4) |
- BIT(Vtotal, 9, 0x01, 5) |
- BIT(Vdispend, 9, 0x01, 6) |
- BIT(Vsyncstart, 9, 0x01, 7);
- hw->crtc[9] = BIT(0, 0, 0x1f, 0) |
- BIT(Vblankstart,9, 0x01, 5) |
- BIT(1, 0, 0x01, 6);
+ hw->crtc[7] = BITMASK(Vtotal, 8, 0x01, 0) |
+ BITMASK(Vdispend, 8, 0x01, 1) |
+ BITMASK(Vsyncstart, 8, 0x01, 2) |
+ BITMASK(Vblankstart,8, 0x01, 3) |
+ BITMASK(1, 0, 0x01, 4) |
+ BITMASK(Vtotal, 9, 0x01, 5) |
+ BITMASK(Vdispend, 9, 0x01, 6) |
+ BITMASK(Vsyncstart, 9, 0x01, 7);
+ hw->crtc[9] = BITMASK(0, 0, 0x1f, 0) |
+ BITMASK(Vblankstart,9, 0x01, 5) |
+ BITMASK(1, 0, 0x01, 6);
hw->crtc[10] = Vsyncstart;
- hw->crtc[11] = BIT(Vsyncend, 0, 0x0f, 0) |
- BIT(1, 0, 0x01, 7);
+ hw->crtc[11] = BITMASK(Vsyncend, 0, 0x0f, 0) |
+ BITMASK(1, 0, 0x01, 7);
hw->crtc[12] = Vdispend;
hw->crtc[15] = Vblankstart;
hw->crtc[16] = Vblankend;
@@ -616,10 +616,10 @@ cyber2000fb_decode_crtc(struct par_info *hw,
struct cfb_info *cfb,
* 4=LINECOMP:10 5-IVIDEO 6=FIXCNT
*/
hw->crtc_ofl =
- BIT(Vtotal, 10, 0x01, 0) |
- BIT(Vdispend, 10, 0x01, 1) |
- BIT(Vsyncstart, 10, 0x01, 2) |
- BIT(Vblankstart,10, 0x01, 3) |
+ BITMASK(Vtotal, 10, 0x01, 0) |
+ BITMASK(Vdispend, 10, 0x01, 1) |
+ BITMASK(Vsyncstart, 10, 0x01, 2) |
+ BITMASK(Vblankstart,10, 0x01, 3) |
EXT_CRT_VRTOFL_LINECOMP10;
/* woody: set the interlaced bit... */
diff --git a/fs/select.c b/fs/select.c
index fe0893a..4bbe8ed 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -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))
diff --git a/include/asm-mips/ip32/crime.h b/include/asm-mips/ip32/crime.h
index a13702f..7c36b0e 100644
--- a/include/asm-mips/ip32/crime.h
+++ b/include/asm-mips/ip32/crime.h
@@ -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
diff --git a/include/asm-mips/ip32/mace.h b/include/asm-mips/ip32/mace.h
index 990082c..d08d7c6 100644
--- a/include/asm-mips/ip32/mace.h
+++ b/include/asm-mips/ip32/mace.h
@@ -17,9 +17,6 @@
*/
#define MACE_BASE 0x1f000000 /* physical */
-#undef BIT
-#define BIT(x) (1UL << (x))
-
/*
* PCI interface
*/
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 638165f..33ad687 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -8,6 +8,10 @@
*/
#include <asm/bitops.h>
+#define BIT(nr) (1UL << (nr))
+#define LLBIT(nr) (1ULL << (nr))
+#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
+
static __inline__ int get_bitmask_order(unsigned int count)
{
int order;
diff --git a/include/linux/input.h b/include/linux/input.h
index bde65c8..e4203d1 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -908,9 +908,11 @@ 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))
+#undef BIT
+#define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))
#define LONG(x) ((x)/BITS_PER_LONG)
#define INPUT_KEYCODE(dev, scancode) ((dev->keycodesize == 1) ?
((u8*)dev->keycode)[scancode] : \
diff --git a/include/video/sstfb.h b/include/video/sstfb.h
index baa163f..b52f073 100644
--- a/include/video/sstfb.h
+++ b/include/video/sstfb.h
@@ -68,7 +68,6 @@
# define print_var(X,Y...)
#endif
-#define BIT(x) (1ul<<(x))
#define POW2(x) (1ul<<(x))
/*
diff --git a/include/video/tdfx.h b/include/video/tdfx.h
index c1cc94b..ac6d0f1 100644
--- a/include/video/tdfx.h
+++ b/include/video/tdfx.h
@@ -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
Milind Arun Choudhary
Milind Choudhary wrote:
> Hi all
> working towards the cleanup of BIT macro,
> I've added one to <linux/bitops.h> & cleaned some obvious users.
>
> include/linux/input.h also has a BIT macro
> which does a wrap
> so currently i've done something like
>
> +#undef BIT
> #define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))
>
> Is it advisible to move this macro to bitops.h with some other name
>
> +#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
>
> & make the whole input subsystem use it
> The change is huge, more than 125 files using input.h
> & almost all use the BIT macro.
It is as a big of change, but have you dismissed the "BIT(nr %
BITS_PER_LONG)" approach?
I really think this has to be broken down into a patch-set.
<snip>
> diff --git a/fs/select.c b/fs/select.c
> index fe0893a..4bbe8ed 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -180,7 +180,6 @@ get_max:
> return max;
> }
>
> -#define BIT(i) (1UL << ((i)&(__NFDBITS-1)))
Are you sure you can just delete this one?
<snip>
> diff --git a/include/linux/input.h b/include/linux/input.h
> index bde65c8..e4203d1 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -908,9 +908,11 @@ struct ff_effect {
> #include <linux/fs.h>
> #include <linux/timer.h>
> #include <linux/mod_devicetable.h>
> +//#include <linux/bitops.h>
You added and commented it out?
>
> #define NBITS(x) (((x)/BITS_PER_LONG)+1)
> -#define BIT(x) (1UL<<((x)%BITS_PER_LONG))
> +#undef BIT
> +#define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))
Why did you change x to nr? The other defines seems to use x.
> #define LONG(x) ((x)/BITS_PER_LONG)
>
> #define INPUT_KEYCODE(dev, scancode) ((dev->keycodesize == 1) ?
> ((u8*)dev->keycode)[scancode] : \
> diff --git a/include/video/sstfb.h b/include/video/sstfb.h
> index baa163f..b52f073 100644
> --- a/include/video/sstfb.h
> +++ b/include/video/sstfb.h
> @@ -68,7 +68,6 @@
> # define print_var(X,Y...)
> #endif
>
> -#define BIT(x) (1ul<<(x))
> #define POW2(x) (1ul<<(x))
Maybe you can clean up POW2 as well (or define it as "#define POW2(x)
BIT(x)")
Also, it seems your mail-client swapped the tabs to spaces (aka not able
to apply).
Other then what I have commented on, it looks good.
Richard Knutsson
On 2/23/07, Richard Knutsson <[email protected]> wrote:
> > +#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
> >
> > & make the whole input subsystem use it
> > The change is huge, more than 125 files using input.h
> > & almost all use the BIT macro.
> It is as a big of change, but have you dismissed the "BIT(nr %
> BITS_PER_LONG)" approach?
no..
but just looking at the number of places it is being used,
it seems that adding a new macro would be good
which makes it look short n sweet
> I really think this has to be broken down into a patch-set.
yes
> > -#define BIT(i) (1UL << ((i)&(__NFDBITS-1)))
> Are you sure you can just delete this one?
yes...no users in this file
> <snip>
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index bde65c8..e4203d1 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -908,9 +908,11 @@ struct ff_effect {
> > #include <linux/fs.h>
> > #include <linux/timer.h>
> > #include <linux/mod_devicetable.h>
> > +//#include <linux/bitops.h>
> You added and commented it out?
> > #define NBITS(x) (((x)/BITS_PER_LONG)+1)
> > -#define BIT(x) (1UL<<((x)%BITS_PER_LONG))
> > +#undef BIT
> > +#define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))
> Why did you change x to nr? The other defines seems to use x.
just messed it up while testing
would clean it after we decide to add a new macro
& before i send the final version.
> > -#define BIT(x) (1ul<<(x))
> > #define POW2(x) (1ul<<(x))
> Maybe you can clean up POW2 as well (or define it as "#define POW2(x)
> BIT(x)")
yes
but want to go one step at a time
currently just cleaning up places where BIT macro is explicitly defined
the implicit uses [replacing 1UL << (x)] will be handled in another patch series
"use BIT macro wherever appropriate"
>
> Also, it seems your mail-client swapped the tabs to spaces (aka not able
> to apply).
attaching the patch file
bear with me for the time being
--
Milind Arun Choudhary
Milind Choudhary wrote:
> On 2/23/07, Richard Knutsson <[email protected]> wrote:
>> > +#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
>> >
>> > & make the whole input subsystem use it
>> > The change is huge, more than 125 files using input.h
>> > & almost all use the BIT macro.
>> It is as a big of change, but have you dismissed the "BIT(nr %
>> BITS_PER_LONG)" approach?
>
> no..
> but just looking at the number of places it is being used,
> it seems that adding a new macro would be good
> which makes it look short n sweet
You have a point there but I still don't think it should be in bitops.h.
Why should we favor long-wrap before byte-wrap, so what do you think
about doing:
#define BITWRAP(x) BIT((x) % BITS_PER_LONG)
in input.h? Otherwise I think it should be call LBITWRAP (or something)
to both show what kind it is and enable us to add others later.
>> > -#define BIT(i) (1UL << ((i)&(__NFDBITS-1)))
>> Are you sure you can just delete this one?
> yes...no users in this file
Ok
>> > -#define BIT(x) (1ul<<(x))
>> > #define POW2(x) (1ul<<(x))
>> Maybe you can clean up POW2 as well (or define it as "#define POW2(x)
>> BIT(x)")
> yes
> but want to go one step at a time
> currently just cleaning up places where BIT macro is explicitly defined
> the implicit uses [replacing 1UL << (x)] will be handled in another
> patch series
> "use BIT macro wherever appropriate"
Sounds good
>
>>
>> Also, it seems your mail-client swapped the tabs to spaces (aka not able
>> to apply).
> attaching the patch file
> bear with me for the time being
>
No problem :)
It is of course always a good idea to send the patches to yourself and
then trying to apply that patch to see it is alright. But sometimes you
can't get the mailer working, then sendpatchset can be of interest:
http://www.speakeasy.org/~pj99/sgi/sendpatchset
Richard Knutsson
On 2/23/07, Richard Knutsson <[email protected]> wrote:
> Milind Choudhary wrote:
> > On 2/23/07, Richard Knutsson <[email protected]> wrote:
> >> > +#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
> >> >
> >> > & make the whole input subsystem use it
> >> > The change is huge, more than 125 files using input.h
> >> > & almost all use the BIT macro.
> >> It is as a big of change, but have you dismissed the "BIT(nr %
> >> BITS_PER_LONG)" approach?
> >
> > no..
> > but just looking at the number of places it is being used,
> > it seems that adding a new macro would be good
> > which makes it look short n sweet
> You have a point there but I still don't think it should be in bitops.h.
> Why should we favor long-wrap before byte-wrap, so what do you think
> about doing:
>
> #define BITWRAP(x) BIT((x) % BITS_PER_LONG)
>
> in input.h? Otherwise I think it should be call LBITWRAP (or something)
> to both show what kind it is and enable us to add others later.
Why would you not want to have what you call bitwrap as a standard
behavior? Most placed to not use modulus because they know the kind of
data they are working with but should still be fine if generic
implementation did that.
--
Dmitry
Dmitry Torokhov wrote:
> On 2/23/07, Richard Knutsson <[email protected]> wrote:
>> Milind Choudhary wrote:
>> > On 2/23/07, Richard Knutsson <[email protected]> wrote:
>> >> > +#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
>> >> >
>> >> > & make the whole input subsystem use it
>> >> > The change is huge, more than 125 files using input.h
>> >> > & almost all use the BIT macro.
>> >> It is as a big of change, but have you dismissed the "BIT(nr %
>> >> BITS_PER_LONG)" approach?
>> >
>> > no..
>> > but just looking at the number of places it is being used,
>> > it seems that adding a new macro would be good
>> > which makes it look short n sweet
>> You have a point there but I still don't think it should be in bitops.h.
>> Why should we favor long-wrap before byte-wrap, so what do you think
>> about doing:
>>
>> #define BITWRAP(x) BIT((x) % BITS_PER_LONG)
>>
>> in input.h? Otherwise I think it should be call LBITWRAP (or something)
>> to both show what kind it is and enable us to add others later.
>
> Why would you not want to have what you call bitwrap as a standard
> behavior? Most placed to not use modulus because they know the kind of
> data they are working with but should still be fine if generic
> implementation did that.
>
Both because I find the name not as expressive as simple "BIT(x %
something)", but mainly since it only enables wrapping of the long-type.
But that is just my opinion.
Just to test:
> grep -Enr "BIT\(.*\%" *
include/asm-arm/arch-h720x/irqs.h:114:#define IRQ_TO_BIT(irq) (1 << ((irq - NR_GLBL_IRQS) % 32))
include/asm-arm/arch-omap/irqs.h:274:#define OMAP_IRQ_BIT(irq) (1 << ((irq) % 32))
include/asm-i386/mach-visws/piix4.h:17:#define GPIBIT(x) (1 << ((x)%8))
include/linux/netfilter/xt_conntrack.h:11:#define XT_CONNTRACK_STATE_BIT(ctinfo) (1 << ((ctinfo)%IP_CT_IS_REPLY+1))
include/linux/netfilter/xt_state.h:4:#define XT_STATE_BIT(ctinfo) (1 << ((ctinfo)%IP_CT_IS_REPLY+1))
...
So it seems there are some instances who wrap but they don't seem to
like BITSWAP (well, maybe those "% 32" on an appropriate arch).
So I think that if an subsystem uses something like this much (like
input.h), then it is up to that subsystem to provide it. When more then
one sub-system have a need for it, then it should be a common define (as
BIT is now). But as I said, that is just my opinion.
Richard Knutsson
On 2/23/07, Richard Knutsson <[email protected]> wrote:
> Dmitry Torokhov wrote:
> > On 2/23/07, Richard Knutsson <[email protected]> wrote:
> >> Milind Choudhary wrote:
> >> > On 2/23/07, Richard Knutsson <[email protected]> wrote:
> >> >> > +#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
> >> >> >
> >> >> > & make the whole input subsystem use it
> >> >> > The change is huge, more than 125 files using input.h
> >> >> > & almost all use the BIT macro.
> >> >> It is as a big of change, but have you dismissed the "BIT(nr %
> >> >> BITS_PER_LONG)" approach?
> >> >
> >> > no..
> >> > but just looking at the number of places it is being used,
> >> > it seems that adding a new macro would be good
> >> > which makes it look short n sweet
> >> You have a point there but I still don't think it should be in bitops.h.
> >> Why should we favor long-wrap before byte-wrap, so what do you think
> >> about doing:
> >>
> >> #define BITWRAP(x) BIT((x) % BITS_PER_LONG)
> >>
> >> in input.h? Otherwise I think it should be call LBITWRAP (or something)
> >> to both show what kind it is and enable us to add others later.
> >
> > Why would you not want to have what you call bitwrap as a standard
> > behavior? Most placed to not use modulus because they know the kind of
> > data they are working with but should still be fine if generic
> > implementation did that.
> >
> Both because I find the name not as expressive as simple "BIT(x %
> something)",
I was not talking about name (I hate BITWRAP) but behavior.
> but mainly since it only enables wrapping of the long-type.
I'd provde BIT and separate LLBIT for ones who really need long long.
People who intereseted in smaller than BITS_PER_LONG bitmaps shoud use
your proposal - BIT(x % DESIRED_WITH) and BIT should do modulo
BITS_PER_LONG internally.
--
Dmitry
Dmitry Torokhov wrote:
> I was not talking about name (I hate BITWRAP) but behavior.
Oh, my bad :)
>
>> but mainly since it only enables wrapping of the long-type.
>
> I'd provde BIT and separate LLBIT for ones who really need long long.
> People who intereseted in smaller than BITS_PER_LONG bitmaps shoud use
> your proposal - BIT(x % DESIRED_WITH) and BIT should do modulo
> BITS_PER_LONG internally.
I agree that _if_ there is a "BITWRAP" then it should be long, but I
don't see the reason for it to be in bitops.h when it is only input.h
that uses it. + I find it different with BIT since it works as well with
'char' as 'long'.
Also, I think it would be best if the name indicated it is a 'long'.
Am a little bit curious why you would like it in bitops.h, but won't
complain if you do (think you have noticed my view of it ;))
Richard Knutsson
On 2/23/07, Richard Knutsson <[email protected]> wrote:
> Dmitry Torokhov wrote:
> > I was not talking about name (I hate BITWRAP) but behavior.
> Oh, my bad :)
> >
> >> but mainly since it only enables wrapping of the long-type.
> >
> > I'd provde BIT and separate LLBIT for ones who really need long long.
> > People who intereseted in smaller than BITS_PER_LONG bitmaps shoud use
> > your proposal - BIT(x % DESIRED_WITH) and BIT should do modulo
> > BITS_PER_LONG internally.
> I agree that _if_ there is a "BITWRAP" then it should be long, but I
> don't see the reason for it to be in bitops.h when it is only input.h
> that uses it. + I find it different with BIT since it works as well with
> 'char' as 'long'.
> Also, I think it would be best if the name indicated it is a 'long'.
>
> Am a little bit curious why you would like it in bitops.h, but won't
> complain if you do (think you have noticed my view of it ;))
>
Hm, I thought as was clear, but apparently I messed up explaining my position:
1. I don't like BITWRAP name at all and I don't want anything like
that near input code. I think BIT is just fine.
2. I don't want to use BIT(x % BITS_PER_BITLONG) as it will
significantly litter code in the input drivers. You want see whta bits
you are actually setting behind all these "% BITS_PER_BITLONG".
3. I think most of users could use input's implementation of BIT,
possibly using BIT(x % BM_WIDTH) format to further limit width of the
bitmap if needed.
4. LLBIT should be provided to users who really want long long.
--
Dmitry
Dmitry Torokhov wrote:
> On 2/23/07, Richard Knutsson <[email protected]> wrote:
>> Dmitry Torokhov wrote:
>> > I was not talking about name (I hate BITWRAP) but behavior.
>> Oh, my bad :)
>> >
>> >> but mainly since it only enables wrapping of the long-type.
>> >
>> > I'd provde BIT and separate LLBIT for ones who really need long long.
>> > People who intereseted in smaller than BITS_PER_LONG bitmaps shoud use
>> > your proposal - BIT(x % DESIRED_WITH) and BIT should do modulo
>> > BITS_PER_LONG internally.
>> I agree that _if_ there is a "BITWRAP" then it should be long, but I
>> don't see the reason for it to be in bitops.h when it is only input.h
>> that uses it. + I find it different with BIT since it works as well with
>> 'char' as 'long'.
>> Also, I think it would be best if the name indicated it is a 'long'.
>>
>> Am a little bit curious why you would like it in bitops.h, but won't
>> complain if you do (think you have noticed my view of it ;))
>>
>
> Hm, I thought as was clear, but apparently I messed up explaining my
> position:
>
> 1. I don't like BITWRAP name at all and I don't want anything like
> that near input code. I think BIT is just fine.
Oh, I think I understand now. So the (in input.h):
#undef BIT
#define BIT(...
business is what you want to do? Well, that I will not object to. Your
patch with:
+#define BIT(nr) (1UL << (nr))
+#define LLBIT(nr) (1ULL << (nr))
+#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
in bitops.h made me believe the #undef in input.h was just a temporarily
thing.
>
> 2. I don't want to use BIT(x % BITS_PER_BITLONG) as it will
> significantly litter code in the input drivers. You want see whta bits
> you are actually setting behind all these "% BITS_PER_BITLONG".
As I said before, I thought it should be defined as BITSWAP (or
whatever) in input.h and then there is no more "% BITS_PER_LONG" litter.
But redefining BIT seems like an equally good idea;
+ eas(y/ier) to understand and simple to implement
- another definition of BIT.
>
> 3. I think most of users could use input's implementation of BIT,
> possibly using BIT(x % BM_WIDTH) format to further limit width of the
> bitmap if needed.
Agreed.
>
> 4. LLBIT should be provided to users who really want long long.
Agreed. (As in the case of "BIT(x) (0x800...00ULL >> (x)) )
Richard Knutsson
On 2/23/07, Richard Knutsson <[email protected]> wrote:
> Dmitry Torokhov wrote:
> >
> > Hm, I thought as was clear, but apparently I messed up explaining my
> > position:
> >
> > 1. I don't like BITWRAP name at all and I don't want anything like
> > that near input code. I think BIT is just fine.
> Oh, I think I understand now. So the (in input.h):
> #undef BIT
> #define BIT(...
> business is what you want to do? Well, that I will not object to.
No, #undefs may be barely tolerable in .c files but they are not
acceptable in core subsystem interfaces. If you do that you will never
know what version of BIT patricular module is using.
> Your
> patch with:
> +#define BIT(nr) (1UL << (nr))
> +#define LLBIT(nr) (1ULL << (nr))
> +#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
> in bitops.h made me believe the #undef in input.h was just a temporarily
> thing.
No. There is no "my patch". You are confusing me with Milind
Choudhary. I am saying that IMO input's BIT definition should be
adequate for 99% of potential users and that I would be OK with moving
said BIT definition from input.h to bitops.h and maybe supplementing
it with LLBIT. I am also saying that I do not want BITWRAP, BITSWAP
(what swap btw?) nor BIT(x % BITS_PER_LONG) in input drivers.
--
Dmitry
Dmitry Torokhov wrote:
> On 2/23/07, Richard Knutsson <[email protected]> wrote:
>> Dmitry Torokhov wrote:
>> >
>> > Hm, I thought as was clear, but apparently I messed up explaining my
>> > position:
>> >
>> > 1. I don't like BITWRAP name at all and I don't want anything like
>> > that near input code. I think BIT is just fine.
>> Oh, I think I understand now. So the (in input.h):
>> #undef BIT
>> #define BIT(...
>> business is what you want to do? Well, that I will not object to.
>
> No, #undefs may be barely tolerable in .c files but they are not
> acceptable in core subsystem interfaces. If you do that you will never
> know what version of BIT patricular module is using.
Yes, kinda. But wouldn't the compiler complain if you included both
input.h and bitops.h (multiply definitions of BIT)?
>> Your
>> patch with:
>> +#define BIT(nr) (1UL << (nr))
>> +#define LLBIT(nr) (1ULL << (nr))
>> +#define BITWRAP(nr) (1UL << ((nr) % BITS_PER_LONG))
>> in bitops.h made me believe the #undef in input.h was just a temporarily
>> thing.
>
> No. There is no "my patch". You are confusing me with Milind
> Choudhary.
Sorry about that. Am surprised I didn't notice it earlier...
> I am saying that IMO input's BIT definition should be
> adequate for 99% of potential users and that I would be OK with moving
> said BIT definition from input.h to bitops.h and maybe supplementing
> it with LLBIT. I am also saying that I do not want BITWRAP, BITSWAP
> (what swap btw?) nor BIT(x % BITS_PER_LONG) in input drivers.
Is the reason for the modulo to put a bitmask larger then the variable
into an array? I did just a quick 'grep' for "BIT(" in drivers/input/
and from what I saw, most (or all?) of the values are defined constants
and those in input.h were noway near the limits of a 'long'.
The reason I don't like it with modulo is simply because it hides
potential bugs (when x is to big). And what about the "1%"?
IMHO BIT should be as simple as possible.
Richard Knutsson
On Fri, Feb 23, 2007 at 01:44:41PM +0530, Milind Choudhary wrote:
> Hi all
> working towards the cleanup of BIT macro,
> I've added one to <linux/bitops.h> & cleaned some obvious users.
>
> include/linux/input.h also has a BIT macro
> which does a wrap
> so currently i've done something like
>
> +#undef BIT
> #define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))
Since the previous definition of
#define BIT(nr) (1UL << (nr))
gives the same results as the above one for all reasonable usage
scenarios (you don't want to supply nr larger than BITS_PER_LONG),
why not just use the modulo version everywhere?
The only problem I see is that the compiler would not warn where nr IS
too large.
--
Vojtech Pavlik
Director SuSE Labs
On Fri, Feb 23, 2007 at 11:43:44PM +0100, Richard Knutsson wrote:
> >I am saying that IMO input's BIT definition should be
> >adequate for 99% of potential users and that I would be OK with moving
> >said BIT definition from input.h to bitops.h and maybe supplementing
> >it with LLBIT. I am also saying that I do not want BITWRAP, BITSWAP
> >(what swap btw?) nor BIT(x % BITS_PER_LONG) in input drivers.
And I totally agree with Dmitry. The "% BITS_PER_LONG" doesn't hurt
other users, and it's needed for larger-than-single-long bit arrays.
> Is the reason for the modulo to put a bitmask larger then the variable
> into an array?
The complementary LONG() macro will tell you the index of an array of
longs where the bit should be set.
> I did just a quick 'grep' for "BIT(" in drivers/input/
> and from what I saw, most (or all?) of the values are defined constants
> and those in input.h were noway near the limits of a 'long'.
Well, many do not need it, but for example BIT(BTN_LEFT) does, and
that's used in a lot of places.
> The reason I don't like it with modulo is simply because it hides
> potential bugs (when x is to big).
That would be my only concern - losing compiler warnings.
> And what about the "1%"?
The 1% will need either LLBIT or an extra % 8.
> IMHO BIT should be as simple as possible.
--
Vojtech Pavlik
Director SuSE Labs
Vojtech Pavlik wrote:
> On Fri, Feb 23, 2007 at 11:43:44PM +0100, Richard Knutsson wrote:
>
>
>> Is the reason for the modulo to put a bitmask larger then the variable
>> into an array?
>>
>
> The complementary LONG() macro will tell you the index of an array of
> longs where the bit should be set.
>
This may be a little OT, but how come it is not done as an function?
Maybe something like "(set/get)_long_mask(...)".
>
>> The reason I don't like it with modulo is simply because it hides
>> potential bugs (when x is to big).
>>
>
> That would be my only concern - losing compiler warnings.
>
And what bugs me is that this will effect the whole tree for a feature
used in only input, right?
>
>> And what about the "1%"?
>>
>
> The 1% will need either LLBIT or an extra % 8.
>
Oh, that's true
Richard Knutsson
On 12:11 Sat 24 Feb , Vojtech Pavlik wrote:
>
> That would be my only concern - losing compiler warnings.
yes
see
I wanted a single BIT macro which can be used by the whole tree
was looking for a multipurpose one. found it in input.h
so i thought i will put it at a common place
why bitops.h? coz BIT qualifies for a "bitop"
& bitops.h is inclued by kernel.h, hence accessible from every part
of the tree without mucb efforts
now
a> this was written for input user,so they are perfectly happy with it
only change would be now input.h will have
to fetch it from bitops.h..trivial
b> currently almost all other users of BIT are well within the BITS_PER_LONG
limit
c>but it is not sutaible for those who want to go beyond this limit,
as they will not be warned
Now if we have LLBIT which takes care of above case
[& as LLBIT has no wrap it will warn if we go beyond "long long" for
some reason]
So all we need is people to be carefull before passing anything to BIT
& use LLBIT whereever appropriate
so now i think it should be ok to have
#define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))
#define LLBIT(nr) (1ULL << (nr))
thoughts
> > And what about the "1%"?
>
> The 1% will need either LLBIT or an extra % 8.
--
Milind Arun Choudhary
On Saturday 24 February 2007 06:11, Vojtech Pavlik wrote:
> > The reason I don't like it with modulo is simply because it hides
> > potential bugs (when x is to big).
>
> That would be my only concern - losing compiler warnings.
>
I think most dangerous scenario is when both shift operands are not constant
but compiler will not help us in this case...
--
Dmitry
On Saturday 24 February 2007 07:59, Richard Knutsson wrote:
> Vojtech Pavlik wrote:
> > On Fri, Feb 23, 2007 at 11:43:44PM +0100, Richard Knutsson wrote:
> >
> >
> >> Is the reason for the modulo to put a bitmask larger then the variable
> >> into an array?
> >>
> >
> > The complementary LONG() macro will tell you the index of an array of
> > longs where the bit should be set.
> >
> This may be a little OT, but how come it is not done as an function?
> Maybe something like "(set/get)_long_mask(...)".
We have it. Is is called set_bit (or __set_bit) and works wery well for
single bit operations, but sometimes it is nice to be able to write
a = BIT(b) | BIT(c);
--
Dmitry
Milind Arun Choudhary wrote:
> why bitops.h? coz BIT qualifies for a "bitop"
> & bitops.h is inclued by kernel.h, hence accessible from every part
> of the tree without mucb efforts
>
I don't think there is anyone who objects to this
> c>but it is not sutaible for those who want to go beyond this limit,
> as they will not be warned
>
And this is the reason for this overly long thread :)
> So all we need is people to be carefull before passing anything to BIT
>
This is the difficult thing to do
> so now i think it should be ok to have
>
> #define BIT(nr) (1UL << ((nr) % BITS_PER_LONG))
> #define LLBIT(nr) (1ULL << (nr))
>
>
> thoughts
>
Since you guys seems in agreement about the silenced compiler-warnings,
then I will rest my case.
Richard Knutsson