2013-08-07 09:06:29

by Linus Walleij

[permalink] [raw]
Subject: [PATCH] bitops: move BITS() macro to the bitops file

This macro was invented by Mattias Nilsson for the usecase
where you want to set a sequence of bits inside a n-bit
word, while leaving the head and tail of the sequence all
zeroes. For example:

#include <linux/bitops.h>

u16 mask = BITS(4, 12);

Yields a mask like this:

0001111111110000

This patch moves the construct out of the MFD PRCMU driver
and make it available for common use, after noticing in a
review or two that it would be useful for others.

Cc: Akinobu Mita <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
---
drivers/mfd/dbx500-prcmu-regs.h | 2 --
include/linux/bitops.h | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mfd/dbx500-prcmu-regs.h b/drivers/mfd/dbx500-prcmu-regs.h
index 4f6f0fa..906e75e 100644
--- a/drivers/mfd/dbx500-prcmu-regs.h
+++ b/drivers/mfd/dbx500-prcmu-regs.h
@@ -13,8 +13,6 @@
#ifndef __DB8500_PRCMU_REGS_H
#define __DB8500_PRCMU_REGS_H

-#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
-
#define PRCM_ACLK_MGT (0x004)
#define PRCM_SVAMMCSPCLK_MGT (0x008)
#define PRCM_SIAMMDSPCLK_MGT (0x00C)
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index a3b6b82..1f9d78b 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -6,6 +6,7 @@
#define BIT(nr) (1UL << (nr))
#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
+#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
#define BITS_PER_BYTE 8
#define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
#endif
--
1.8.1.4


2013-08-07 10:12:50

by anish singh

[permalink] [raw]
Subject: Re: [PATCH] bitops: move BITS() macro to the bitops file

On Wed, Aug 7, 2013 at 2:36 PM, Linus Walleij <[email protected]> wrote:
> This macro was invented by Mattias Nilsson for the usecase
> where you want to set a sequence of bits inside a n-bit
> word, while leaving the head and tail of the sequence all
> zeroes. For example:
>
> #include <linux/bitops.h>
>
> u16 mask = BITS(4, 12);

nice.I don't have anything against this patch but couldn't understand
how this works.So i wrote what i knew as below:

#include <stdio.h>

#define BIT(nr) (1UL << (nr))
#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
#define BITS_MY(_start, _end) (((BIT(_end-_start+1))-1)<<_start)
int main()
{
printf("%x\n", BITS(4,12));
printf("%x\n", BITS_MY(4,12));
return 0;
}

>
> Yields a mask like this:
>
> 0001111111110000
>
> This patch moves the construct out of the MFD PRCMU driver
> and make it available for common use, after noticing in a
> review or two that it would be useful for others.
>
> Cc: Akinobu Mita <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> ---
> drivers/mfd/dbx500-prcmu-regs.h | 2 --
> include/linux/bitops.h | 1 +
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/dbx500-prcmu-regs.h b/drivers/mfd/dbx500-prcmu-regs.h
> index 4f6f0fa..906e75e 100644
> --- a/drivers/mfd/dbx500-prcmu-regs.h
> +++ b/drivers/mfd/dbx500-prcmu-regs.h
> @@ -13,8 +13,6 @@
> #ifndef __DB8500_PRCMU_REGS_H
> #define __DB8500_PRCMU_REGS_H
>
> -#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
> -
> #define PRCM_ACLK_MGT (0x004)
> #define PRCM_SVAMMCSPCLK_MGT (0x008)
> #define PRCM_SIAMMDSPCLK_MGT (0x00C)
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index a3b6b82..1f9d78b 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -6,6 +6,7 @@
> #define BIT(nr) (1UL << (nr))
> #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
> #define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
> +#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))

Shouldn't there be compile time check here for checking _end > _start?
BUILD_BUG_ON(_end<_start);

> #define BITS_PER_BYTE 8
> #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
> #endif
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2013-08-07 10:33:36

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] bitops: move BITS() macro to the bitops file

On Wed, 2013-08-07 at 11:06 +0200, Linus Walleij wrote:
> This macro was invented by Mattias Nilsson for the usecase
> where you want to set a sequence of bits inside a n-bit
> word, while leaving the head and tail of the sequence all
> zeroes. For example:
>
> #include <linux/bitops.h>

BITS is a name that's not easily reused because it's
already in use in a few other places.

There are a few conflicts.

$ git grep -E "^\s*#\s*define\s+\bBITS\b"
arch/arc/include/asm/disasm.h:#define BITS(word, s, e) (((word) >> (s)) & (~((-2) << ((e) - (s)))))
drivers/input/keyboard/tnetv107x-keypad.c:#define BITS(x) (BIT(x) - 1)
drivers/mfd/dbx500-prcmu-regs.h:#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))
drivers/net/wireless/iwlwifi/mvm/fw-api-bt-coex.h:#define BITS(nb) (BIT(nb) - 1)
fs/select.c:#define BITS(fds, n) (*FDS_IN(fds, n)|*FDS_OUT(fds, n)|*FDS_EX(fds, n))
lib/zlib_inflate/inflate.c:#define BITS(n) \
sound/core/oss/rate.c:#define BITS (1<<SHIFT)

> u16 mask = BITS(4, 12);
>
> Yields a mask like this:
>
> 0001111111110000
[]
> diff --git a/drivers/mfd/dbx500-prcmu-regs.h b/drivers/mfd/dbx500-prcmu-regs.h
[]
> @@ -13,8 +13,6 @@
[]
> -#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))

> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
[]
> @@ -6,6 +6,7 @@
[]
> +#define BITS(_start, _end) ((BIT(_end) - BIT(_start)) + BIT(_end))

Maybe use a statement expression to make sure
the start and end accesses are done only once.

#define BITS(start, end) \
({ \
unsigned long high = BIT(end); \
unsigned long low = BIT(start); \
unsigned long rtn = high + (high - low); \
rtn; \
})

I suggest | instead of + too.

Maybe add a WARN when low > high too.
Maybe make it a function to consolidate
any WARN instead of inline expansion.

2013-08-07 10:54:12

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH] bitops: move BITS() macro to the bitops file

On Wed, Aug 07, 2013 at 11:06:19AM +0200, Linus Walleij wrote:
> This macro was invented by Mattias Nilsson for the usecase
> where you want to set a sequence of bits inside a n-bit
> word, while leaving the head and tail of the sequence all
> zeroes. For example:
>
> #include <linux/bitops.h>
>
> u16 mask = BITS(4, 12);
>
> Yields a mask like this:
>
> 0001111111110000
>
> This patch moves the construct out of the MFD PRCMU driver
> and make it available for common use, after noticing in a
> review or two that it would be useful for others.

Well, we have similar GEN_MASK and GENMASK macros around the tree. Maybe
we could unify them all?

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--