Skip to contentSkip to conversation

Introduce the for_each_set_clump8 macro

PATCH Superseded William Breathitt Gray 5 participants 39 messages Oct 9, 2019 – Oct 10, 2019

Subsystems (13)

v17 of 18 Superseded Latest is v18 →

lore
Share & tools
lore
#0 lore raw

v17 cover Introduce the for_each_set_clump8 macro

Changes in v17:
- Move bitmap_get_value8/bitmap_set_value8 to include/linux/bitmap.h
- add style changes suggested by Andy Shevchenko to intel_soc_dts_iosf

While adding GPIO get_multiple/set_multiple callback support for various drivers, I noticed a pattern of looping manifesting that would be useful standardized as a macro.

This patchset introduces the for_each_set_clump8 macro and utilizes it in several GPIO drivers. The for_each_set_clump macro8 facilitates a for-loop syntax that iterates over a memory region entire groups of set bits at a time.

For example, suppose you would like to iterate over a 32-bit integer 8 bits at a time, skipping over 8-bit groups with no set bit, where XXXXXXXX represents the current 8-bit group:

    Example:        10111110 00000000 11111111 00110011
    First loop:     10111110 00000000 11111111 XXXXXXXX
    Second loop:    10111110 00000000 XXXXXXXX 00110011
    Third loop:     XXXXXXXX 00000000 11111111 00110011

Each iteration of the loop returns the next 8-bit group that has at least one set bit.

The for_each_set_clump8 macro has four parameters:

* start: set to the bit offset of the current clump
* clump: set to the current clump value
* bits: bitmap to search within
* size: bitmap size in number of bits

In this version of the patchset, the for_each_set_clump macro has been reimplemented and simplified based on the suggestions provided by Rasmus Villemoes and Andy Shevchenko in the version 4 submission.

In particular, the function of the for_each_set_clump macro has been restricted to handle only 8-bit clumps; the drivers that use the for_each_set_clump macro only handle 8-bit ports so a generic for_each_set_clump implementation is not necessary. Thus, a solution for large clumps (i.e. those larger than the width of a bitmap word) can be postponed until a driver appears that actually requires such a generic for_each_set_clump implementation.

For what it's worth, a semi-generic for_each_set_clump (i.e. for clumps smaller than the width of a bitmap word) can be implemented by simply replacing the hardcoded '8' and '0xFF' instances with respective variables. I have not yet had a need for such an implementation, and since it falls short of a true generic for_each_set_clump function, I have decided to forgo such an implementation for now.

In addition, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to get and set 8-bit values respectively. Their use is based on the behavior suggested in the patchset version 4 review.

William Breathitt Gray (14):
bitops: Introduce the for_each_set_clump8 macro lib/test_bitmap.c: Add for_each_set_clump8 test cases
gpio: 104-dio-48e: Utilize for_each_set_clump8 macro
gpio: 104-idi-48: Utilize for_each_set_clump8 macro
gpio: gpio-mm: Utilize for_each_set_clump8 macro
gpio: ws16c48: Utilize for_each_set_clump8 macro
gpio: pci-idio-16: Utilize for_each_set_clump8 macro
gpio: pcie-idio-24: Utilize for_each_set_clump8 macro
gpio: uniphier: Utilize for_each_set_clump8 macro
gpio: 74x164: Utilize the for_each_set_clump8 macro
thermal: intel: intel_soc_dts_iosf: Utilize for_each_set_clump8 macro
gpio: pisosr: Utilize the for_each_set_clump8 macro
gpio: max3191x: Utilize the for_each_set_clump8 macro
gpio: pca953x: Utilize the for_each_set_clump8 macro

 drivers/gpio/gpio-104-dio-48e.c            |  73 ++++----------
 drivers/gpio/gpio-104-idi-48.c             |  36 ++-----
 drivers/gpio/gpio-74x164.c                 |  19 ++--
 drivers/gpio/gpio-gpio-mm.c                |  73 ++++----------
 drivers/gpio/gpio-max3191x.c               |  19 ++--
 drivers/gpio/gpio-pca953x.c                |  17 ++--
 drivers/gpio/gpio-pci-idio-16.c            |  75 +++++---------
 drivers/gpio/gpio-pcie-idio-24.c           | 109 ++++++++-------------
 drivers/gpio/gpio-pisosr.c                 |  12 +--
 drivers/gpio/gpio-uniphier.c               |  16 ++-
 drivers/gpio/gpio-ws16c48.c                |  73 ++++----------
 drivers/thermal/intel/intel_soc_dts_iosf.c |  31 +++---
 drivers/thermal/intel/intel_soc_dts_iosf.h |   2 -
 include/asm-generic/bitops/find.h          |  17 ++++
 include/linux/bitmap.h                     |  35 +++++++
 include/linux/bitops.h                     |   5 +
 lib/find_bit.c                             |  14 +++
 lib/test_bitmap.c                          |  65 ++++++++++++
 18 files changed, 328 insertions(+), 363 deletions(-)

base-commit: 8c550e94b8835170593169a45b5ba30d3fc72a70

Signature · 1 line
2.23.0
#1 lore raw

PATCH v17 1/14 bitops: Introduce the for_each_set_clump8 macro

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

---
 include/asm-generic/bitops/find.h | 17 +++++++++++++++
 include/linux/bitmap.h            | 35 +++++++++++++++++++++++++++++++
 include/linux/bitops.h            |  5 +++++
 lib/find_bit.c                    | 14 +++++++++++++
 4 files changed, 71 insertions(+)
include/asm-generic/bitops/find.h +17 -0
index 8a1ee10014de..9fdf21302fdf 100644
@@ -80,4 +80,21 @@ extern unsigned long find_first_zero_bit(const unsigned long *addr,
 
 #endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
 
+/**
+ * find_next_clump8 - find next 8-bit clump with set bits in a memory region
+ * @clump: location to store copy of found clump
+ * @addr: address to base the search on
+ * @size: bitmap size in number of bits
+ * @offset: bit offset at which to start searching
+ *
+ * Returns the bit offset for the next set clump; the found clump value is
+ * copied to the location pointed by @clump. If no bits are set, returns @size.
+ */
+extern unsigned long find_next_clump8(unsigned long *clump,
+				      const unsigned long *addr,
+				      unsigned long size, unsigned long offset);
+
+#define find_first_clump8(clump, bits, size) \
+	find_next_clump8((clump), (bits), (size), 0)
+
 #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
include/linux/bitmap.h +35 -0
index 90528f12bdfa..761fab5b60a7 100644
@@ -66,6 +66,8 @@
  *  bitmap_allocate_region(bitmap, pos, order)  Allocate specified bit region
  *  bitmap_from_arr32(dst, buf, nbits)          Copy nbits from u32[] buf to dst
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
+ *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
  *
  * Note, bitmap_zero() and bitmap_fill() operate over the region of
  * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -488,6 +490,39 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
 		dst[1] = mask >> 32;
 }
 
+/**
+ * bitmap_get_value8 - get an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ *
+ * Returns the 8-bit value located at the @start bit offset within the @src
+ * memory region.
+ */
+static inline unsigned long bitmap_get_value8(const unsigned long *map,
+					      unsigned long start)
+{
+	const size_t index = BIT_WORD(start);
+	const unsigned long offset = start % BITS_PER_LONG;
+
+	return (map[index] >> offset) & 0xFF;
+}
+
+/**
+ * bitmap_set_value8 - set an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: the 8-bit value; values wider than 8 bits may clobber bitmap
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ */
+static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
+				     unsigned long start)
+{
+	const size_t index = BIT_WORD(start);
+	const unsigned long offset = start % BITS_PER_LONG;
+
+	map[index] &= ~(0xFF << offset);
+	map[index] |= value << offset;
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LINUX_BITMAP_H */
include/linux/bitops.h +5 -0
index cf074bce3eb3..fb94a10f7853 100644
@@ -40,6 +40,11 @@ extern unsigned long __sw_hweight64(__u64 w);
 	     (bit) < (size);					\
 	     (bit) = find_next_zero_bit((addr), (size), (bit) + 1))
 
+#define for_each_set_clump8(start, clump, bits, size) \
+	for ((start) = find_first_clump8(&(clump), (bits), (size)); \
+	     (start) < (size); \
+	     (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
+
 static inline int get_bitmask_order(unsigned int count)
 {
 	int order;
lib/find_bit.c +14 -0
index 5c51eb45178a..e35a76b291e6 100644
@@ -214,3 +214,17 @@ EXPORT_SYMBOL(find_next_bit_le);
 #endif
 
 #endif /* __BIG_ENDIAN */
+
+unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,
+			       unsigned long size, unsigned long offset)
+{
+	offset = find_next_bit(addr, size, offset);
+	if (offset == size)
+		return size;
+
+	offset = round_down(offset, 8);
+	*clump = bitmap_get_value8(addr, offset);
+
+	return offset;
+}
+EXPORT_SYMBOL(find_next_clump8);
Signature · 1 line
2.23.0
#2 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Quoted 66 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

---
 include/asm-generic/bitops/find.h | 17 +++++++++++++++
 include/linux/bitmap.h            | 35 +++++++++++++++++++++++++++++++
 include/linux/bitops.h            |  5 +++++
 lib/find_bit.c                    | 14 +++++++++++++
 4 files changed, 71 insertions(+)
include/asm-generic/bitops/find.h +17 -0
index 8a1ee10014de..9fdf21302fdf 100644
@@ -80,4 +80,21 @@ extern unsigned long find_first_zero_bit(const unsigned long *addr,
 
 #endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
 
+/**
+ * find_next_clump8 - find next 8-bit clump with set bits in a memory region
+ * @clump: location to store copy of found clump
+ * @addr: address to base the search on
+ * @size: bitmap size in number of bits
+ * @offset: bit offset at which to start searching
+ *
+ * Returns the bit offset for the next set clump; the found clump value is
+ * copied to the location pointed by @clump. If no bits are set, returns @size.
+ */
+extern unsigned long find_next_clump8(unsigned long *clump,
+                                     const unsigned long *addr,
+                                     unsigned long size, unsigned long offset);
+
+#define find_first_clump8(clump, bits, size) \
+       find_next_clump8((clump), (bits), (size), 0)
+
 #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
include/linux/bitmap.h +2 -0
index 90528f12bdfa..761fab5b60a7 100644
@@ -66,6 +66,8 @@
  *  bitmap_allocate_region(bitmap, pos, order)  Allocate specified bit region
  *  bitmap_from_arr32(dst, buf, nbits)          Copy nbits from u32[] buf to dst
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
+ *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
  *
  * Note, bitmap_zero() and bitmap_fill() operate over the region of
  * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -488,6 +490,39 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
                dst[1] = mask >> 32;
 }
 

include/linux/bitmap.h static inline void bitmap_from_u64(unsigned long *dst, u64 mask) · line 502

Quoted 10 lines
include/linux/bitmap.h +10 -0
+/**
+ * bitmap_get_value8 - get an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ *
+ * Returns the 8-bit value located at the @start bit offset within the @src
+ * memory region.
+ */
+static inline unsigned long bitmap_get_value8(const unsigned long *map,
+                                             unsigned long start)

Why is the return type "unsigned long" where you know it return the 8-bit value ?

u8?

Quoted 4 lines
include/linux/bitmap.h +4 -0
+{
+       const size_t index = BIT_WORD(start);
+       const unsigned long offset = start % BITS_PER_LONG;
+

include/linux/bitmap.h static inline void bitmap_from_u64(unsigned long *dst, u64 mask) · line 516

Quoted 10 lines
include/linux/bitmap.h +10 -0
+       return (map[index] >> offset) & 0xFF;
+}
+
+/**
+ * bitmap_set_value8 - set an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: the 8-bit value; values wider than 8 bits may clobber bitmap
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ */
+static inline void bitmap_set_value8(unsigned long *map, unsigned long value,

Same here, "u8 value"

Quoted 33 lines
include/linux/bitmap.h +9 -0
+                                    unsigned long start)
+{
+       const size_t index = BIT_WORD(start);
+       const unsigned long offset = start % BITS_PER_LONG;
+
+       map[index] &= ~(0xFF << offset);
+       map[index] |= value << offset;
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LINUX_BITMAP_H */
include/linux/bitops.h +5 -0
index cf074bce3eb3..fb94a10f7853 100644
@@ -40,6 +40,11 @@ extern unsigned long __sw_hweight64(__u64 w);
             (bit) < (size);                                    \
             (bit) = find_next_zero_bit((addr), (size), (bit) + 1))
 
+#define for_each_set_clump8(start, clump, bits, size) \
+       for ((start) = find_first_clump8(&(clump), (bits), (size)); \
+            (start) < (size); \
+            (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
+
 static inline int get_bitmask_order(unsigned int count)
 {
        int order;
lib/find_bit.c +0 -0
index 5c51eb45178a..e35a76b291e6 100644

lib/find_bit.c EXPORT_SYMBOL(find_next_bit_le); · line 218

Quoted 5 lines
lib/find_bit.c +2 -0
@@ -214,3 +214,17 @@ EXPORT_SYMBOL(find_next_bit_le);
 #endif
 
 #endif /* __BIG_ENDIAN */
+
+unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,

Ditto. "u8 *clump"

Quoted 13 lines
lib/find_bit.c +12 -1
+                              unsigned long size, unsigned long offset)
+{
+       offset = find_next_bit(addr, size, offset);
+       if (offset == size)
+               return size;
+
+       offset = round_down(offset, 8);
+       *clump = bitmap_get_value8(addr, offset);
+
+       return offset;
+}
+EXPORT_SYMBOL(find_next_clump8);
--
Quoted 2 lines

2.23.0

--
Best Regards

Masahiro Yamada

#3 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Quoted 82 lines · Masahiro Yamada
Quoted 66 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

---
 include/asm-generic/bitops/find.h | 17 +++++++++++++++
 include/linux/bitmap.h            | 35 +++++++++++++++++++++++++++++++
 include/linux/bitops.h            |  5 +++++
 lib/find_bit.c                    | 14 +++++++++++++
 4 files changed, 71 insertions(+)
include/asm-generic/bitops/find.h +17 -0
index 8a1ee10014de..9fdf21302fdf 100644
@@ -80,4 +80,21 @@ extern unsigned long find_first_zero_bit(const unsigned long *addr,
 
 #endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
 
+/**
+ * find_next_clump8 - find next 8-bit clump with set bits in a memory region
+ * @clump: location to store copy of found clump
+ * @addr: address to base the search on
+ * @size: bitmap size in number of bits
+ * @offset: bit offset at which to start searching
+ *
+ * Returns the bit offset for the next set clump; the found clump value is
+ * copied to the location pointed by @clump. If no bits are set, returns @size.
+ */
+extern unsigned long find_next_clump8(unsigned long *clump,
+                                     const unsigned long *addr,
+                                     unsigned long size, unsigned long offset);
+
+#define find_first_clump8(clump, bits, size) \
+       find_next_clump8((clump), (bits), (size), 0)
+
 #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
include/linux/bitmap.h +2 -0
index 90528f12bdfa..761fab5b60a7 100644
@@ -66,6 +66,8 @@
  *  bitmap_allocate_region(bitmap, pos, order)  Allocate specified bit region
  *  bitmap_from_arr32(dst, buf, nbits)          Copy nbits from u32[] buf to dst
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
+ *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
  *
  * Note, bitmap_zero() and bitmap_fill() operate over the region of
  * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -488,6 +490,39 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
                dst[1] = mask >> 32;
 }
 

include/linux/bitmap.h static inline void bitmap_from_u64(unsigned long *dst, u64 mask) · line 502

Quoted 10 lines
include/linux/bitmap.h +10 -0
+/**
+ * bitmap_get_value8 - get an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ *
+ * Returns the 8-bit value located at the @start bit offset within the @src
+ * memory region.
+ */
+static inline unsigned long bitmap_get_value8(const unsigned long *map,
+                                             unsigned long start)

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Quoted 1 line

u8?

The primary reason is to be consistent with the datatype of the bitmap:
https://lkml.org/lkml/2019/1/12/26

This should also make it easier to extent to other sizes in the future since we won't have to change the interface in order to support 16-bit or 32-bit values -- they should easily fit within an unsigned long.

William Breathitt Gray

Quoted 89 lines
Quoted 7 lines
include/linux/bitmap.h +4 -0
+{
+       const size_t index = BIT_WORD(start);
+       const unsigned long offset = start % BITS_PER_LONG;
+

include/linux/bitmap.h static inline void bitmap_from_u64(unsigned long *dst, u64 mask) · line 516

Quoted 10 lines
include/linux/bitmap.h +10 -0
+       return (map[index] >> offset) & 0xFF;
+}
+
+/**
+ * bitmap_set_value8 - set an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: the 8-bit value; values wider than 8 bits may clobber bitmap
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ */
+static inline void bitmap_set_value8(unsigned long *map, unsigned long value,

Same here, "u8 value"

Quoted 33 lines
include/linux/bitmap.h +9 -0
+                                    unsigned long start)
+{
+       const size_t index = BIT_WORD(start);
+       const unsigned long offset = start % BITS_PER_LONG;
+
+       map[index] &= ~(0xFF << offset);
+       map[index] |= value << offset;
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LINUX_BITMAP_H */
include/linux/bitops.h +5 -0
index cf074bce3eb3..fb94a10f7853 100644
@@ -40,6 +40,11 @@ extern unsigned long __sw_hweight64(__u64 w);
             (bit) < (size);                                    \
             (bit) = find_next_zero_bit((addr), (size), (bit) + 1))
 
+#define for_each_set_clump8(start, clump, bits, size) \
+       for ((start) = find_first_clump8(&(clump), (bits), (size)); \
+            (start) < (size); \
+            (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
+
 static inline int get_bitmask_order(unsigned int count)
 {
        int order;
lib/find_bit.c +0 -0
index 5c51eb45178a..e35a76b291e6 100644

lib/find_bit.c EXPORT_SYMBOL(find_next_bit_le); · line 218

Quoted 5 lines
lib/find_bit.c +2 -0
@@ -214,3 +214,17 @@ EXPORT_SYMBOL(find_next_bit_le);
 #endif
 
 #endif /* __BIG_ENDIAN */
+
+unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,

Ditto. "u8 *clump"

Quoted 13 lines
lib/find_bit.c +12 -1
+                              unsigned long size, unsigned long offset)
+{
+       offset = find_next_bit(addr, size, offset);
+       if (offset == size)
+               return size;
+
+       offset = round_down(offset, 8);
+       *clump = bitmap_get_value8(addr, offset);
+
+       return offset;
+}
+EXPORT_SYMBOL(find_next_clump8);
--
Quoted 2 lines

2.23.0

--
Best Regards

Masahiro Yamada

#4 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

In reply to Masahiro Yamada
Quoted 10 lines · Masahiro Yamada
Quoted 7 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

Quoted 2 lines

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Same comment for the rest.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#5 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Hi Andy,

Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Masahiro Yamada
Quoted 7 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

Quoted 6 lines
Quoted 2 lines

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#6 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Quoted 23 lines · Geert Uytterhoeven

Hi Andy,

Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Masahiro Yamada
Quoted 7 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

Quoted 6 lines
Quoted 2 lines

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Quoted 8 lines

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

+1

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

If you want to see more examples from other parts,

int of_property_read_u8(const struct device_node *np,
const char *propname, u8 *out_value)

int of_property_read_u16(const struct device_node *np,
const char *propname, u16 *out_value)

Signature · 2 lines
Best Regards
Masahiro Yamada
#7 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Quoted 34 lines · Masahiro Yamada
Quoted 20 lines · Geert Uytterhoeven
Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Masahiro Yamada
Quoted 7 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

Quoted 6 lines
Quoted 2 lines

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Quoted 8 lines

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

+1

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

Quoted 8 lines

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

If you want to see more examples from other parts,

Geert's and yours examples both are not related. They are about fixed-width properies when we know that is the part of protocol. Here we have no protocol which stricts us to the mentioned fixed-width types.

So, I can tell an opposite, your arguments didn't convince me.

Imagine the function which does an or / and / xor operation on bitmap. Now, when I supply unsigned long, I will see
operations on one type in _one_ function independently of the size. Your proposal will make an unneded churn.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#8 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Hi Andy,

Quoted 43 lines · Andy Shevchenko
Quoted 41 lines · Masahiro Yamada
Quoted 20 lines · Geert Uytterhoeven
Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Masahiro Yamada
Quoted 7 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

Quoted 6 lines
Quoted 2 lines

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Quoted 8 lines

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

+1

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

Quoted 5 lines
Quoted 1 line

If you want to see more examples from other parts,

Geert's and yours examples both are not related. They are about fixed-width properies when we know that is the part of protocol. Here we have no protocol which stricts us to the mentioned fixed-width types.

Yes you have: they are functions to store/retrieve an 8-bit value from the middle of the bitmap, which is reflected in their names ("clump8", "value8").
The input/output value is clearly separated from the actual bitmap, which is referenced by the "unsigned long *".

If you add new "value16" functions, they will be intended to store/retrieve 16-bit values.

Besides, if retrieving an 8-bit value requires passing an "unsigned long *", the caller needs two variables: one unsigned long to pass the address of, and one u8 to copy the returned value into.

Quoted 6 lines

So, I can tell an opposite, your arguments didn't convince me.

Imagine the function which does an or / and / xor operation on bitmap. Now, when I supply unsigned long, I will see
operations on one type in _one_ function independently of the size. Your proposal will make an unneded churn.

Depends on what kind of value you will use to do the logical operation with the bitmap:
- Full bitmap => unsigned long * + size,
- Single bitmap "word" => unsigned long,
- 8-bit value => u8,
- 16-bit value => u16

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#9 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Quoted 51 lines · Geert Uytterhoeven
Quoted 43 lines · Andy Shevchenko
Quoted 41 lines · Masahiro Yamada
Quoted 20 lines · Geert Uytterhoeven
Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Masahiro Yamada
Quoted 7 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

Quoted 6 lines
Quoted 2 lines

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Quoted 8 lines

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

+1

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

Quoted 5 lines
Quoted 1 line

If you want to see more examples from other parts,

Geert's and yours examples both are not related. They are about fixed-width properies when we know that is the part of protocol. Here we have no protocol which stricts us to the mentioned fixed-width types.

Quoted 8 lines

Yes you have: they are functions to store/retrieve an 8-bit value from the middle of the bitmap, which is reflected in their names ("clump8", "value8").
The input/output value is clearly separated from the actual bitmap, which is referenced by the "unsigned long *".

If you add new "value16" functions, they will be intended to store/retrieve 16-bit values.

And if I add 4-bit, 12-bit or 24-bit values, what should I use?

Quoted 3 lines

Besides, if retrieving an 8-bit value requires passing an "unsigned long *", the caller needs two variables: one unsigned long to pass the address of, and one u8 to copy the returned value into.

Why do you need a temporary variable? In some cases it might make sense, but in general simple cases I don't see what you may achieve with it.

I looked at bitmap.h and see few functions may have benefited of actually eliminating a use of long -> u8 -> long conversion.

Here is the question what we are mostly doing after we got a clump out of bitmap.

Quoted 13 lines
Quoted 6 lines

So, I can tell an opposite, your arguments didn't convince me.

Imagine the function which does an or / and / xor operation on bitmap. Now, when I supply unsigned long, I will see
operations on one type in _one_ function independently of the size. Your proposal will make an unneded churn.

Depends on what kind of value you will use to do the logical operation with the bitmap:
- Full bitmap => unsigned long * + size,
- Single bitmap "word" => unsigned long,
- 8-bit value => u8,
- 16-bit value => u16

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#10 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Hi Andy,

Quoted 58 lines · Andy Shevchenko
Quoted 57 lines · Geert Uytterhoeven
Quoted 43 lines · Andy Shevchenko
Quoted 41 lines · Masahiro Yamada
Quoted 20 lines · Geert Uytterhoeven
Quoted 11 lines · Andy Shevchenko
Quoted 9 lines · Masahiro Yamada
Quoted 7 lines · William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

Quoted 6 lines
Quoted 2 lines

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Quoted 8 lines

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

+1

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

Quoted 5 lines
Quoted 1 line

If you want to see more examples from other parts,

Geert's and yours examples both are not related. They are about fixed-width properies when we know that is the part of protocol. Here we have no protocol which stricts us to the mentioned fixed-width types.

Yes you have: they are functions to store/retrieve an 8-bit value from the middle of the bitmap, which is reflected in their names ("clump8", "value8").
The input/output value is clearly separated from the actual bitmap, which is referenced by the "unsigned long *".

Quoted 4 lines
Quoted 2 lines

If you add new "value16" functions, they will be intended to store/retrieve 16-bit values.

And if I add 4-bit, 12-bit or 24-bit values, what should I use?

Whatever is needed to store that?
I agree "unsigned long" is appropriate for a generic function to extract a bit field of 1 to BITS_PER_LONG bits.

Quoted 7 lines
Quoted 3 lines

Besides, if retrieving an 8-bit value requires passing an "unsigned long *", the caller needs two variables: one unsigned long to pass the address of, and one u8 to copy the returned value into.

Why do you need a temporary variable? In some cases it might make sense, but in general simple cases I don't see what you may achieve with it.

Because find_next_clump8() takes a pointer to store the output value.

Quoted 5 lines

I looked at bitmap.h and see few functions may have benefited of actually eliminating a use of long -> u8 -> long conversion.

Here is the question what we are mostly doing after we got a clump out of bitmap.

If I call find_next_clump8() to extract a byte, I guess I want to process an u8 aftwerwards?

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#11 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Quoted 61 lines · Geert Uytterhoeven
Quoted 46 lines · Andy Shevchenko
Quoted 45 lines · Geert Uytterhoeven
Quoted 31 lines · Andy Shevchenko
Quoted 29 lines · Masahiro Yamada
Quoted 13 lines · Geert Uytterhoeven
Quoted 10 lines · Andy Shevchenko
Quoted 5 lines · Masahiro Yamada

On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray <[email protected]> wrote:

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Quoted 7 lines

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

Quoted 5 lines
Quoted 1 line

If you want to see more examples from other parts,

Geert's and yours examples both are not related. They are about fixed-width properies when we know that is the part of protocol. Here we have no protocol which stricts us to the mentioned fixed-width types.

Yes you have: they are functions to store/retrieve an 8-bit value from the middle of the bitmap, which is reflected in their names ("clump8", "value8").
The input/output value is clearly separated from the actual bitmap, which is referenced by the "unsigned long *".

Quoted 4 lines
Quoted 2 lines

If you add new "value16" functions, they will be intended to store/retrieve 16-bit values.

And if I add 4-bit, 12-bit or 24-bit values, what should I use?

Whatever is needed to store that?
I agree "unsigned long" is appropriate for a generic function to extract a bit field of 1 to BITS_PER_LONG bits.

Quoted 4 lines
Quoted 3 lines

Besides, if retrieving an 8-bit value requires passing an "unsigned long *", the caller needs two variables: one unsigned long to pass the address of, and one u8 to copy the returned value into.

Quoted 5 lines
Quoted 3 lines

Why do you need a temporary variable? In some cases it might make sense, but in general simple cases I don't see what you may achieve with it.

Because find_next_clump8() takes a pointer to store the output value.

So does regmap_read().

8 appeared there during review when it has been proposed to optimize to 8-bit clumps as most of the current users utilize it. The initial idea was to be bit-width agnostic. And with current API it's possible to easy convert to other formats later if we need.

Quoted 8 lines
Quoted 5 lines

I looked at bitmap.h and see few functions may have benefited of actually eliminating a use of long -> u8 -> long conversion.

Here is the question what we are mostly doing after we got a clump out of bitmap.

If I call find_next_clump8() to extract a byte, I guess I want to process an u8 aftwerwards?

Some functions may expect a width-(semi-)dependent types, like regmap_write(). Yes, it's possible to supply u8 there and have an implicit type cast.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#12 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Hi Andy,

Quoted 62 lines · Andy Shevchenko
Quoted 61 lines · Geert Uytterhoeven
Quoted 46 lines · Andy Shevchenko
Quoted 45 lines · Geert Uytterhoeven
Quoted 31 lines · Andy Shevchenko
Quoted 29 lines · Masahiro Yamada
Quoted 13 lines · Geert Uytterhoeven
Quoted 10 lines · Andy Shevchenko
Quoted 5 lines · Masahiro Yamada

On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray <[email protected]> wrote:

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Quoted 7 lines

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

Quoted 5 lines
Quoted 1 line

If you want to see more examples from other parts,

Geert's and yours examples both are not related. They are about fixed-width properies when we know that is the part of protocol. Here we have no protocol which stricts us to the mentioned fixed-width types.

Yes you have: they are functions to store/retrieve an 8-bit value from the middle of the bitmap, which is reflected in their names ("clump8", "value8").
The input/output value is clearly separated from the actual bitmap, which is referenced by the "unsigned long *".

Quoted 4 lines
Quoted 2 lines

If you add new "value16" functions, they will be intended to store/retrieve 16-bit values.

And if I add 4-bit, 12-bit or 24-bit values, what should I use?

Whatever is needed to store that?
I agree "unsigned long" is appropriate for a generic function to extract a bit field of 1 to BITS_PER_LONG bits.

Quoted 4 lines
Quoted 3 lines

Besides, if retrieving an 8-bit value requires passing an "unsigned long *", the caller needs two variables: one unsigned long to pass the address of, and one u8 to copy the returned value into.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Why do you need a temporary variable? In some cases it might make sense, but in general simple cases I don't see what you may achieve with it.

Because find_next_clump8() takes a pointer to store the output value.

So does regmap_read().

I believe that one is different, as it is a generic function, and the width of the
returned value depends on the regmap config.

Quoted 4 lines

8 appeared there during review when it has been proposed to optimize to 8-bit clumps as most of the current users utilize it. The initial idea was to be bit-width agnostic. And with current API it's possible to easy convert to other formats later if we need.

"optimized for 8-bit clumps" and "out-of-line function that takes an unsigned long pointer for an output parameter" don't match well, IMHO.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
#13 lore raw

Re: [PATCH v17 01/14] bitops: Introduce the for_each_set_clump8 macro

Quoted 83 lines · Geert Uytterhoeven

Hi Andy,

Quoted 62 lines · Andy Shevchenko
Quoted 61 lines · Geert Uytterhoeven
Quoted 46 lines · Andy Shevchenko
Quoted 45 lines · Geert Uytterhoeven
Quoted 31 lines · Andy Shevchenko
Quoted 29 lines · Masahiro Yamada
Quoted 13 lines · Geert Uytterhoeven
Quoted 10 lines · Andy Shevchenko
Quoted 5 lines · Masahiro Yamada

On Thu, Oct 10, 2019 at 12:27 AM William Breathitt Gray <[email protected]> wrote:

Why is the return type "unsigned long" where you know it return the 8-bit value ?

Because bitmap API operates on unsigned long type. This is not only consistency, but for sake of flexibility in case we would like to introduce more calls like clump16 or so.

Quoted 7 lines

TBH, that doesn't convince me: those functions explicitly take/return an 8-bit value, and have "8" in their name. The 8-bit value is never really related to, retrieved from, or stored in a full "unsigned long" element of a bitmap, only to/from/in a part (byte) of it.

Following your rationale, all of iowrite{8,16,32,64}*() should take an "unsigned long" value, too.

Using u8/u16/u32/u64 looks more consistent with other bitmap helpers.

void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf, unsigned int nbits);
void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits); static inline void bitmap_from_u64(unsigned long *dst, u64 mask);

Quoted 5 lines
Quoted 1 line

If you want to see more examples from other parts,

Geert's and yours examples both are not related. They are about fixed-width properies when we know that is the part of protocol. Here we have no protocol which stricts us to the mentioned fixed-width types.

Yes you have: they are functions to store/retrieve an 8-bit value from the middle of the bitmap, which is reflected in their names ("clump8", "value8").
The input/output value is clearly separated from the actual bitmap, which is referenced by the "unsigned long *".

Quoted 4 lines
Quoted 2 lines

If you add new "value16" functions, they will be intended to store/retrieve 16-bit values.

And if I add 4-bit, 12-bit or 24-bit values, what should I use?

Whatever is needed to store that?
I agree "unsigned long" is appropriate for a generic function to extract a bit field of 1 to BITS_PER_LONG bits.

Quoted 4 lines
Quoted 3 lines

Besides, if retrieving an 8-bit value requires passing an "unsigned long *", the caller needs two variables: one unsigned long to pass the address of, and one u8 to copy the returned value into.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Why do you need a temporary variable? In some cases it might make sense, but in general simple cases I don't see what you may achieve with it.

Because find_next_clump8() takes a pointer to store the output value.

So does regmap_read().

I believe that one is different, as it is a generic function, and the width of the
returned value depends on the regmap config.

Quoted 4 lines

8 appeared there during review when it has been proposed to optimize to 8-bit clumps as most of the current users utilize it. The initial idea was to be bit-width agnostic. And with current API it's possible to easy convert to other formats later if we need.

Quoted 13 lines

"optimized for 8-bit clumps" and "out-of-line function that takes an unsigned long pointer for an output parameter" don't match well, IMHO.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

"Optimize" may not be the best way of describing it. I conceded to introducing a restricted implementation (i.e. for_each_set_clump8) since there were disagreements on the best approach for an implementation a generic for_each_set_clump macro that could support any bit size. So I settled for introducing just for_each_set_clump8 since it has an implementation everyone could agree on and I didn't want to stall the patchset for this introduction.

I'm hoping to propose the generic for_each_set_clump macro again in the future after for_each_set_clump8 has had time to be utilized. There are some files that I think might benefit from such a generic implementation (e.g. gpio-thunderx with 64-bit ports and gpio-xilinx with variable size channels). In such case, for_each_set_clump8 would likely be reimplemented as a macro hardcoding an 8 passed to for_each_set_clump -- or perhaps just eliminated and replaced with for_each_set_clump directly -- so maintaining clump as unsigned long pointer is useful since we won't need to worry about redeclaring variables to match the datatype.

Though I admit that there are advantages in specifying the datatype as u8 (or u16, u32, etc.). If we know the size then it's reasonable to expect that the implementation can be optimized to not worry about variable sizes and boundaries -- as exemplified by the simplicity of the for_each_set_clump8 implementation. So that may be an argument for keeping the for_each_set_clump8 implementation separate from the generic for_each_set_clump implementation.

William Breathitt Gray

#14 lore raw

PATCH v17 1/14 bitops: Introduce the for_each_set_clump8 macro

In reply to William Breathitt Gray

This macro iterates for each 8-bit group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value8 and bitmap_set_value8 functions are introduced to respectively get and set an 8-bit value in a bitmap memory region.

---
 include/asm-generic/bitops/find.h | 17 +++++++++++++++
 include/linux/bitmap.h            | 35 +++++++++++++++++++++++++++++++
 include/linux/bitops.h            |  5 +++++
 lib/find_bit.c                    | 14 +++++++++++++
 4 files changed, 71 insertions(+)
include/asm-generic/bitops/find.h +17 -0
index 8a1ee10014de..9fdf21302fdf 100644
@@ -80,4 +80,21 @@ extern unsigned long find_first_zero_bit(const unsigned long *addr,
 
 #endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
 
+/**
+ * find_next_clump8 - find next 8-bit clump with set bits in a memory region
+ * @clump: location to store copy of found clump
+ * @addr: address to base the search on
+ * @size: bitmap size in number of bits
+ * @offset: bit offset at which to start searching
+ *
+ * Returns the bit offset for the next set clump; the found clump value is
+ * copied to the location pointed by @clump. If no bits are set, returns @size.
+ */
+extern unsigned long find_next_clump8(unsigned long *clump,
+				      const unsigned long *addr,
+				      unsigned long size, unsigned long offset);
+
+#define find_first_clump8(clump, bits, size) \
+	find_next_clump8((clump), (bits), (size), 0)
+
 #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
include/linux/bitmap.h +35 -0
index 90528f12bdfa..761fab5b60a7 100644
@@ -66,6 +66,8 @@
  *  bitmap_allocate_region(bitmap, pos, order)  Allocate specified bit region
  *  bitmap_from_arr32(dst, buf, nbits)          Copy nbits from u32[] buf to dst
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
+ *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
  *
  * Note, bitmap_zero() and bitmap_fill() operate over the region of
  * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -488,6 +490,39 @@ static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
 		dst[1] = mask >> 32;
 }
 
+/**
+ * bitmap_get_value8 - get an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ *
+ * Returns the 8-bit value located at the @start bit offset within the @src
+ * memory region.
+ */
+static inline unsigned long bitmap_get_value8(const unsigned long *map,
+					      unsigned long start)
+{
+	const size_t index = BIT_WORD(start);
+	const unsigned long offset = start % BITS_PER_LONG;
+
+	return (map[index] >> offset) & 0xFF;
+}
+
+/**
+ * bitmap_set_value8 - set an 8-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: the 8-bit value; values wider than 8 bits may clobber bitmap
+ * @start: bit offset of the 8-bit value; must be a multiple of 8
+ */
+static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
+				     unsigned long start)
+{
+	const size_t index = BIT_WORD(start);
+	const unsigned long offset = start % BITS_PER_LONG;
+
+	map[index] &= ~(0xFF << offset);
+	map[index] |= value << offset;
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LINUX_BITMAP_H */
include/linux/bitops.h +5 -0
index cf074bce3eb3..fb94a10f7853 100644
@@ -40,6 +40,11 @@ extern unsigned long __sw_hweight64(__u64 w);
 	     (bit) < (size);					\
 	     (bit) = find_next_zero_bit((addr), (size), (bit) + 1))
 
+#define for_each_set_clump8(start, clump, bits, size) \
+	for ((start) = find_first_clump8(&(clump), (bits), (size)); \
+	     (start) < (size); \
+	     (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
+
 static inline int get_bitmask_order(unsigned int count)
 {
 	int order;
lib/find_bit.c +14 -0
index 5c51eb45178a..e35a76b291e6 100644
@@ -214,3 +214,17 @@ EXPORT_SYMBOL(find_next_bit_le);
 #endif
 
 #endif /* __BIG_ENDIAN */
+
+unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,
+			       unsigned long size, unsigned long offset)
+{
+	offset = find_next_bit(addr, size, offset);
+	if (offset == size)
+		return size;
+
+	offset = round_down(offset, 8);
+	*clump = bitmap_get_value8(addr, offset);
+
+	return offset;
+}
+EXPORT_SYMBOL(find_next_clump8);
Signature · 1 line
2.23.0
#15 lore raw

PATCH v17 2/14 lib/test_bitmap.c: Add for_each_set_clump8 test cases

In reply to William Breathitt Gray

The introduction of the for_each_set_clump8 macro warrants test cases to verify the implementation. This patch adds test case checks for whether an out-of-bounds clump index is returned, a zero clump is returned, or the returned clump value differs from the expected clump value.

---
 lib/test_bitmap.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)
lib/test_bitmap.c +65 -0
index 51a98f7ee79e..dc167c13eb39 100644
@@ -92,6 +92,36 @@ __check_eq_u32_array(const char *srcfile, unsigned int line,
 	return true;
 }
 
+static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,
+				    const unsigned int offset,
+				    const unsigned int size,
+				    const unsigned char *const clump_exp,
+				    const unsigned long *const clump)
+{
+	unsigned long exp;
+
+	if (offset >= size) {
+		pr_warn("[%s:%u] bit offset for clump out-of-bounds: expected less than %u, got %u\n",
+			srcfile, line, size, offset);
+		return false;
+	}
+
+	exp = clump_exp[offset / 8];
+	if (!exp) {
+		pr_warn("[%s:%u] bit offset for zero clump: expected nonzero clump, got bit offset %u with clump value 0",
+			srcfile, line, offset);
+		return false;
+	}
+
+	if (*clump != exp) {
+		pr_warn("[%s:%u] expected clump value of 0x%lX, got clump value of 0x%lX",
+			srcfile, line, exp, *clump);
+		return false;
+	}
+
+	return true;
+}
+
 #define __expect_eq(suffix, ...)					\
 	({								\
 		int result = 0;						\
@@ -108,6 +138,7 @@ __check_eq_u32_array(const char *srcfile, unsigned int line,
 #define expect_eq_bitmap(...)		__expect_eq(bitmap, ##__VA_ARGS__)
 #define expect_eq_pbl(...)		__expect_eq(pbl, ##__VA_ARGS__)
 #define expect_eq_u32_array(...)	__expect_eq(u32_array, ##__VA_ARGS__)
+#define expect_eq_clump8(...)		__expect_eq(clump8, ##__VA_ARGS__)
 
 static void __init test_zero_clear(void)
 {
@@ -404,6 +435,39 @@ static void noinline __init test_mem_optimisations(void)
 	}
 }
 
+static const unsigned char clump_exp[] __initconst = {
+	0x01,	/* 1 bit set */
+	0x02,	/* non-edge 1 bit set */
+	0x00,	/* zero bits set */
+	0x38,	/* 3 bits set across 4-bit boundary */
+	0x38,	/* Repeated clump */
+	0x0F,	/* 4 bits set */
+	0xFF,	/* all bits set */
+	0x05,	/* non-adjacent 2 bits set */
+};
+
+static void __init test_for_each_set_clump8(void)
+{
+#define CLUMP_EXP_NUMBITS 64
+	DECLARE_BITMAP(bits, CLUMP_EXP_NUMBITS);
+	unsigned int start;
+	unsigned long clump;
+
+	/* set bitmap to test case */
+	bitmap_zero(bits, CLUMP_EXP_NUMBITS);
+	bitmap_set(bits, 0, 1);		/* 0x01 */
+	bitmap_set(bits, 9, 1);		/* 0x02 */
+	bitmap_set(bits, 27, 3);	/* 0x28 */
+	bitmap_set(bits, 35, 3);	/* 0x28 */
+	bitmap_set(bits, 40, 4);	/* 0x0F */
+	bitmap_set(bits, 48, 8);	/* 0xFF */
+	bitmap_set(bits, 56, 1);	/* 0x05 - part 1 */
+	bitmap_set(bits, 58, 1);	/* 0x05 - part 2 */
+
+	for_each_set_clump8(start, clump, bits, CLUMP_EXP_NUMBITS)
+		expect_eq_clump8(start, CLUMP_EXP_NUMBITS, clump_exp, &clump);
+}
+
 static void __init selftest(void)
 {
 	test_zero_clear();
@@ -413,6 +477,7 @@ static void __init selftest(void)
 	test_bitmap_parselist();
 	test_bitmap_parselist_user();
 	test_mem_optimisations();
+	test_for_each_set_clump8();
 }
 
 KSTM_MODULE_LOADERS(test_bitmap);
Signature · 1 line
2.23.0
#16 lore raw

PATCH v17 2/14 lib/test_bitmap.c: Add for_each_set_clump8 test cases

In reply to William Breathitt Gray

The introduction of the for_each_set_clump8 macro warrants test cases to verify the implementation. This patch adds test case checks for whether an out-of-bounds clump index is returned, a zero clump is returned, or the returned clump value differs from the expected clump value.

---
 lib/test_bitmap.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)
lib/test_bitmap.c +65 -0
index 51a98f7ee79e..dc167c13eb39 100644
@@ -92,6 +92,36 @@ __check_eq_u32_array(const char *srcfile, unsigned int line,
 	return true;
 }
 
+static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,
+				    const unsigned int offset,
+				    const unsigned int size,
+				    const unsigned char *const clump_exp,
+				    const unsigned long *const clump)
+{
+	unsigned long exp;
+
+	if (offset >= size) {
+		pr_warn("[%s:%u] bit offset for clump out-of-bounds: expected less than %u, got %u\n",
+			srcfile, line, size, offset);
+		return false;
+	}
+
+	exp = clump_exp[offset / 8];
+	if (!exp) {
+		pr_warn("[%s:%u] bit offset for zero clump: expected nonzero clump, got bit offset %u with clump value 0",
+			srcfile, line, offset);
+		return false;
+	}
+
+	if (*clump != exp) {
+		pr_warn("[%s:%u] expected clump value of 0x%lX, got clump value of 0x%lX",
+			srcfile, line, exp, *clump);
+		return false;
+	}
+
+	return true;
+}
+
 #define __expect_eq(suffix, ...)					\
 	({								\
 		int result = 0;						\
@@ -108,6 +138,7 @@ __check_eq_u32_array(const char *srcfile, unsigned int line,
 #define expect_eq_bitmap(...)		__expect_eq(bitmap, ##__VA_ARGS__)
 #define expect_eq_pbl(...)		__expect_eq(pbl, ##__VA_ARGS__)
 #define expect_eq_u32_array(...)	__expect_eq(u32_array, ##__VA_ARGS__)
+#define expect_eq_clump8(...)		__expect_eq(clump8, ##__VA_ARGS__)
 
 static void __init test_zero_clear(void)
 {
@@ -404,6 +435,39 @@ static void noinline __init test_mem_optimisations(void)
 	}
 }
 
+static const unsigned char clump_exp[] __initconst = {
+	0x01,	/* 1 bit set */
+	0x02,	/* non-edge 1 bit set */
+	0x00,	/* zero bits set */
+	0x38,	/* 3 bits set across 4-bit boundary */
+	0x38,	/* Repeated clump */
+	0x0F,	/* 4 bits set */
+	0xFF,	/* all bits set */
+	0x05,	/* non-adjacent 2 bits set */
+};
+
+static void __init test_for_each_set_clump8(void)
+{
+#define CLUMP_EXP_NUMBITS 64
+	DECLARE_BITMAP(bits, CLUMP_EXP_NUMBITS);
+	unsigned int start;
+	unsigned long clump;
+
+	/* set bitmap to test case */
+	bitmap_zero(bits, CLUMP_EXP_NUMBITS);
+	bitmap_set(bits, 0, 1);		/* 0x01 */
+	bitmap_set(bits, 9, 1);		/* 0x02 */
+	bitmap_set(bits, 27, 3);	/* 0x28 */
+	bitmap_set(bits, 35, 3);	/* 0x28 */
+	bitmap_set(bits, 40, 4);	/* 0x0F */
+	bitmap_set(bits, 48, 8);	/* 0xFF */
+	bitmap_set(bits, 56, 1);	/* 0x05 - part 1 */
+	bitmap_set(bits, 58, 1);	/* 0x05 - part 2 */
+
+	for_each_set_clump8(start, clump, bits, CLUMP_EXP_NUMBITS)
+		expect_eq_clump8(start, CLUMP_EXP_NUMBITS, clump_exp, &clump);
+}
+
 static void __init selftest(void)
 {
 	test_zero_clear();
@@ -413,6 +477,7 @@ static void __init selftest(void)
 	test_bitmap_parselist();
 	test_bitmap_parselist_user();
 	test_mem_optimisations();
+	test_for_each_set_clump8();
 }
 
 KSTM_MODULE_LOADERS(test_bitmap);
Signature · 1 line
2.23.0
#17 lore raw

PATCH v17 3/14 gpio: 104-dio-48e: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-104-dio-48e.c | 73 ++++++++++-----------------------
 1 file changed, 21 insertions(+), 52 deletions(-)
drivers/gpio/gpio-104-dio-48e.c +21 -52
index a44fa8af5b0d..977f0f6eb1ba 100644
@@ -175,46 +175,25 @@ static int dio48e_gpio_get(struct gpio_chip *chip, unsigned offset)
 	return !!(port_state & mask);
 }
 
+static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
+
 static int dio48e_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long *bits)
 {
 	struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
-	size_t i;
-	static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
-	const unsigned int gpio_reg_size = 8;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
+	unsigned long offset;
+	unsigned long gpio_mask;
+	unsigned int port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports); i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
-
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
-
-		/* read bits from current gpio port */
-		port_state = inb(dio48egpio->base + ports[i]);
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		port_addr = dio48egpio->base + ports[offset / 8];
+		port_state = inb(port_addr) & gpio_mask;
 
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
@@ -244,37 +223,27 @@ static void dio48e_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
-	unsigned int i;
-	const unsigned int gpio_reg_size = 8;
-	unsigned int port;
-	unsigned int out_port;
-	unsigned int bitmask;
+	unsigned long offset;
+	unsigned long gpio_mask;
+	size_t index;
+	unsigned int port_addr;
+	unsigned long bitmask;
 	unsigned long flags;
 
-	/* set bits are evaluated a gpio register size at a time */
-	for (i = 0; i < chip->ngpio; i += gpio_reg_size) {
-		/* no more set bits in this mask word; skip to the next word */
-		if (!mask[BIT_WORD(i)]) {
-			i = (BIT_WORD(i) + 1) * BITS_PER_LONG - gpio_reg_size;
-			continue;
-		}
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		index = offset / 8;
+		port_addr = dio48egpio->base + ports[index];
 
-		port = i / gpio_reg_size;
-		out_port = (port > 2) ? port + 1 : port;
-		bitmask = mask[BIT_WORD(i)] & bits[BIT_WORD(i)];
+		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
 
 		raw_spin_lock_irqsave(&dio48egpio->lock, flags);
 
 		/* update output state data and set device gpio register */
-		dio48egpio->out_state[port] &= ~mask[BIT_WORD(i)];
-		dio48egpio->out_state[port] |= bitmask;
-		outb(dio48egpio->out_state[port], dio48egpio->base + out_port);
+		dio48egpio->out_state[index] &= ~gpio_mask;
+		dio48egpio->out_state[index] |= bitmask;
+		outb(dio48egpio->out_state[index], port_addr);
 
 		raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
-
-		/* prepare for next gpio register set */
-		mask[BIT_WORD(i)] >>= gpio_reg_size;
-		bits[BIT_WORD(i)] >>= gpio_reg_size;
 	}
 }
 
Signature · 1 line
2.23.0
#18 lore raw

PATCH v17 4/14 gpio: 104-idi-48: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-104-idi-48.c | 36 +++++++---------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)
drivers/gpio/gpio-104-idi-48.c +7 -29
index ff53887bdaa8..bf67040cbbbb 100644
@@ -85,42 +85,20 @@ static int idi_48_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long *bits)
 {
 	struct idi_48_gpio *const idi48gpio = gpiochip_get_data(chip);
-	size_t i;
+	unsigned long offset;
+	unsigned long gpio_mask;
 	static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
-	const unsigned int gpio_reg_size = 8;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
+	unsigned int port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports); i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		port_addr = idi48gpio->base + ports[offset / 8];
+		port_state = inb(port_addr) & gpio_mask;
 
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
-
-		/* read bits from current gpio port */
-		port_state = inb(idi48gpio->base + ports[i]);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
Signature · 1 line
2.23.0
#19 lore raw

PATCH v17 4/14 gpio: 104-idi-48: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-104-idi-48.c | 36 +++++++---------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)
drivers/gpio/gpio-104-idi-48.c +7 -29
index ff53887bdaa8..bf67040cbbbb 100644
@@ -85,42 +85,20 @@ static int idi_48_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long *bits)
 {
 	struct idi_48_gpio *const idi48gpio = gpiochip_get_data(chip);
-	size_t i;
+	unsigned long offset;
+	unsigned long gpio_mask;
 	static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
-	const unsigned int gpio_reg_size = 8;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
+	unsigned int port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports); i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		port_addr = idi48gpio->base + ports[offset / 8];
+		port_state = inb(port_addr) & gpio_mask;
 
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
-
-		/* read bits from current gpio port */
-		port_state = inb(idi48gpio->base + ports[i]);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
Signature · 1 line
2.23.0
#20 lore raw

PATCH v17 5/14 gpio: gpio-mm: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-gpio-mm.c | 73 +++++++++++--------------------------
 1 file changed, 21 insertions(+), 52 deletions(-)
drivers/gpio/gpio-gpio-mm.c +21 -52
index 78a1db24e931..72196ea36358 100644
@@ -164,46 +164,25 @@ static int gpiomm_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	return !!(port_state & mask);
 }
 
+static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
+
 static int gpiomm_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long *bits)
 {
 	struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
-	size_t i;
-	static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
-	const unsigned int gpio_reg_size = 8;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
+	unsigned long offset;
+	unsigned long gpio_mask;
+	unsigned int port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports); i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
-
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
-
-		/* read bits from current gpio port */
-		port_state = inb(gpiommgpio->base + ports[i]);
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		port_addr = gpiommgpio->base + ports[offset / 8];
+		port_state = inb(port_addr) & gpio_mask;
 
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
@@ -234,37 +213,27 @@ static void gpiomm_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
-	unsigned int i;
-	const unsigned int gpio_reg_size = 8;
-	unsigned int port;
-	unsigned int out_port;
-	unsigned int bitmask;
+	unsigned long offset;
+	unsigned long gpio_mask;
+	size_t index;
+	unsigned int port_addr;
+	unsigned long bitmask;
 	unsigned long flags;
 
-	/* set bits are evaluated a gpio register size at a time */
-	for (i = 0; i < chip->ngpio; i += gpio_reg_size) {
-		/* no more set bits in this mask word; skip to the next word */
-		if (!mask[BIT_WORD(i)]) {
-			i = (BIT_WORD(i) + 1) * BITS_PER_LONG - gpio_reg_size;
-			continue;
-		}
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		index = offset / 8;
+		port_addr = gpiommgpio->base + ports[index];
 
-		port = i / gpio_reg_size;
-		out_port = (port > 2) ? port + 1 : port;
-		bitmask = mask[BIT_WORD(i)] & bits[BIT_WORD(i)];
+		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
 
 		spin_lock_irqsave(&gpiommgpio->lock, flags);
 
 		/* update output state data and set device gpio register */
-		gpiommgpio->out_state[port] &= ~mask[BIT_WORD(i)];
-		gpiommgpio->out_state[port] |= bitmask;
-		outb(gpiommgpio->out_state[port], gpiommgpio->base + out_port);
+		gpiommgpio->out_state[index] &= ~gpio_mask;
+		gpiommgpio->out_state[index] |= bitmask;
+		outb(gpiommgpio->out_state[index], port_addr);
 
 		spin_unlock_irqrestore(&gpiommgpio->lock, flags);
-
-		/* prepare for next gpio register set */
-		mask[BIT_WORD(i)] >>= gpio_reg_size;
-		bits[BIT_WORD(i)] >>= gpio_reg_size;
 	}
 }
 
Signature · 1 line
2.23.0
#21 lore raw

PATCH v17 6/14 gpio: ws16c48: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-ws16c48.c | 73 ++++++++++---------------------------
 1 file changed, 20 insertions(+), 53 deletions(-)
drivers/gpio/gpio-ws16c48.c +20 -53
index e0ef66b6a237..51aaa5c17fce 100644
@@ -126,42 +126,19 @@ static int ws16c48_gpio_get_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct ws16c48_gpio *const ws16c48gpio = gpiochip_get_data(chip);
-	const unsigned int gpio_reg_size = 8;
-	size_t i;
-	const size_t num_ports = chip->ngpio / gpio_reg_size;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
+	unsigned long offset;
+	unsigned long gpio_mask;
+	unsigned int port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < num_ports; i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
+		port_addr = ws16c48gpio->base + offset / 8;
+		port_state = inb(port_addr) & gpio_mask;
 
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
-
-		/* read bits from current gpio port */
-		port_state = inb(ws16c48gpio->base + i);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
@@ -195,39 +172,29 @@ static void ws16c48_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct ws16c48_gpio *const ws16c48gpio = gpiochip_get_data(chip);
-	unsigned int i;
-	const unsigned int gpio_reg_size = 8;
-	unsigned int port;
-	unsigned int iomask;
-	unsigned int bitmask;
+	unsigned long offset;
+	unsigned long gpio_mask;
+	size_t index;
+	unsigned int port_addr;
+	unsigned long bitmask;
 	unsigned long flags;
 
-	/* set bits are evaluated a gpio register size at a time */
-	for (i = 0; i < chip->ngpio; i += gpio_reg_size) {
-		/* no more set bits in this mask word; skip to the next word */
-		if (!mask[BIT_WORD(i)]) {
-			i = (BIT_WORD(i) + 1) * BITS_PER_LONG - gpio_reg_size;
-			continue;
-		}
-
-		port = i / gpio_reg_size;
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
+		index = offset / 8;
+		port_addr = ws16c48gpio->base + index;
 
 		/* mask out GPIO configured for input */
-		iomask = mask[BIT_WORD(i)] & ~ws16c48gpio->io_state[port];
-		bitmask = iomask & bits[BIT_WORD(i)];
+		gpio_mask &= ~ws16c48gpio->io_state[index];
+		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
 
 		raw_spin_lock_irqsave(&ws16c48gpio->lock, flags);
 
 		/* update output state data and set device gpio register */
-		ws16c48gpio->out_state[port] &= ~iomask;
-		ws16c48gpio->out_state[port] |= bitmask;
-		outb(ws16c48gpio->out_state[port], ws16c48gpio->base + port);
+		ws16c48gpio->out_state[index] &= ~gpio_mask;
+		ws16c48gpio->out_state[index] |= bitmask;
+		outb(ws16c48gpio->out_state[index], port_addr);
 
 		raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
-
-		/* prepare for next gpio register set */
-		mask[BIT_WORD(i)] >>= gpio_reg_size;
-		bits[BIT_WORD(i)] >>= gpio_reg_size;
 	}
 }
 
Signature · 1 line
2.23.0
#22 lore raw

PATCH v17 7/14 gpio: pci-idio-16: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-pci-idio-16.c | 75 ++++++++++++---------------------
 1 file changed, 27 insertions(+), 48 deletions(-)
drivers/gpio/gpio-pci-idio-16.c +27 -48
index 5aa136a6a3e0..6c117e57078c 100644
@@ -100,45 +100,23 @@ static int idio_16_gpio_get_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip);
-	size_t i;
-	const unsigned int gpio_reg_size = 8;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
-	unsigned long port_state;
+	unsigned long offset;
+	unsigned long gpio_mask;
 	void __iomem *ports[] = {
 		&idio16gpio->reg->out0_7, &idio16gpio->reg->out8_15,
 		&idio16gpio->reg->in0_7, &idio16gpio->reg->in8_15,
 	};
+	void __iomem *port_addr;
+	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports); i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
-
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		port_addr = ports[offset / 8];
+		port_state = ioread8(port_addr) & gpio_mask;
 
-		/* read bits from current gpio port */
-		port_state = ioread8(ports[i]);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
@@ -178,30 +156,31 @@ static void idio_16_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip);
+	unsigned long offset;
+	unsigned long gpio_mask;
+	void __iomem *ports[] = {
+		&idio16gpio->reg->out0_7, &idio16gpio->reg->out8_15,
+	};
+	size_t index;
+	void __iomem *port_addr;
+	unsigned long bitmask;
 	unsigned long flags;
-	unsigned int out_state;
+	unsigned long out_state;
 
-	raw_spin_lock_irqsave(&idio16gpio->lock, flags);
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		index = offset / 8;
+		port_addr = ports[index];
 
-	/* process output lines 0-7 */
-	if (*mask & 0xFF) {
-		out_state = ioread8(&idio16gpio->reg->out0_7) & ~*mask;
-		out_state |= *mask & *bits;
-		iowrite8(out_state, &idio16gpio->reg->out0_7);
-	}
+		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
+
+		raw_spin_lock_irqsave(&idio16gpio->lock, flags);
 
-	/* shift to next output line word */
-	*mask >>= 8;
+		out_state = ioread8(port_addr) & ~gpio_mask;
+		out_state |= bitmask;
+		iowrite8(out_state, port_addr);
 
-	/* process output lines 8-15 */
-	if (*mask & 0xFF) {
-		*bits >>= 8;
-		out_state = ioread8(&idio16gpio->reg->out8_15) & ~*mask;
-		out_state |= *mask & *bits;
-		iowrite8(out_state, &idio16gpio->reg->out8_15);
+		raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
 	}
-
-	raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
 }
 
 static void idio_16_irq_ack(struct irq_data *data)
Signature · 1 line
2.23.0
#23 lore raw

PATCH v17 7/14 gpio: pci-idio-16: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-pci-idio-16.c | 75 ++++++++++++---------------------
 1 file changed, 27 insertions(+), 48 deletions(-)
drivers/gpio/gpio-pci-idio-16.c +27 -48
index 5aa136a6a3e0..6c117e57078c 100644
@@ -100,45 +100,23 @@ static int idio_16_gpio_get_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip);
-	size_t i;
-	const unsigned int gpio_reg_size = 8;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
-	unsigned long port_state;
+	unsigned long offset;
+	unsigned long gpio_mask;
 	void __iomem *ports[] = {
 		&idio16gpio->reg->out0_7, &idio16gpio->reg->out8_15,
 		&idio16gpio->reg->in0_7, &idio16gpio->reg->in8_15,
 	};
+	void __iomem *port_addr;
+	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports); i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
-
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		port_addr = ports[offset / 8];
+		port_state = ioread8(port_addr) & gpio_mask;
 
-		/* read bits from current gpio port */
-		port_state = ioread8(ports[i]);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
@@ -178,30 +156,31 @@ static void idio_16_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip);
+	unsigned long offset;
+	unsigned long gpio_mask;
+	void __iomem *ports[] = {
+		&idio16gpio->reg->out0_7, &idio16gpio->reg->out8_15,
+	};
+	size_t index;
+	void __iomem *port_addr;
+	unsigned long bitmask;
 	unsigned long flags;
-	unsigned int out_state;
+	unsigned long out_state;
 
-	raw_spin_lock_irqsave(&idio16gpio->lock, flags);
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		index = offset / 8;
+		port_addr = ports[index];
 
-	/* process output lines 0-7 */
-	if (*mask & 0xFF) {
-		out_state = ioread8(&idio16gpio->reg->out0_7) & ~*mask;
-		out_state |= *mask & *bits;
-		iowrite8(out_state, &idio16gpio->reg->out0_7);
-	}
+		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
+
+		raw_spin_lock_irqsave(&idio16gpio->lock, flags);
 
-	/* shift to next output line word */
-	*mask >>= 8;
+		out_state = ioread8(port_addr) & ~gpio_mask;
+		out_state |= bitmask;
+		iowrite8(out_state, port_addr);
 
-	/* process output lines 8-15 */
-	if (*mask & 0xFF) {
-		*bits >>= 8;
-		out_state = ioread8(&idio16gpio->reg->out8_15) & ~*mask;
-		out_state |= *mask & *bits;
-		iowrite8(out_state, &idio16gpio->reg->out8_15);
+		raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
 	}
-
-	raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
 }
 
 static void idio_16_irq_ack(struct irq_data *data)
Signature · 1 line
2.23.0
#24 lore raw

PATCH v17 8/14 gpio: pcie-idio-24: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-pcie-idio-24.c | 109 ++++++++++++-------------------
 1 file changed, 40 insertions(+), 69 deletions(-)
drivers/gpio/gpio-pcie-idio-24.c +40 -69
index 52f1647a46fd..924ec916b358 100644
@@ -198,52 +198,34 @@ static int idio_24_gpio_get_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_24_gpio *const idio24gpio = gpiochip_get_data(chip);
-	size_t i;
-	const unsigned int gpio_reg_size = 8;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
-	unsigned long port_state;
+	unsigned long offset;
+	unsigned long gpio_mask;
 	void __iomem *ports[] = {
 		&idio24gpio->reg->out0_7, &idio24gpio->reg->out8_15,
 		&idio24gpio->reg->out16_23, &idio24gpio->reg->in0_7,
 		&idio24gpio->reg->in8_15, &idio24gpio->reg->in16_23,
 	};
+	size_t index;
+	unsigned long port_state;
 	const unsigned long out_mode_mask = BIT(1);
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports) + 1; i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
-
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		index = offset / 8;
 
 		/* read bits from current gpio port (port 6 is TTL GPIO) */
-		if (i < 6)
-			port_state = ioread8(ports[i]);
+		if (index < 6)
+			port_state = ioread8(ports[index]);
 		else if (ioread8(&idio24gpio->reg->ctl) & out_mode_mask)
 			port_state = ioread8(&idio24gpio->reg->ttl_out0_7);
 		else
 			port_state = ioread8(&idio24gpio->reg->ttl_in0_7);
 
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		port_state &= gpio_mask;
+
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
@@ -294,59 +276,48 @@ static void idio_24_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_24_gpio *const idio24gpio = gpiochip_get_data(chip);
-	size_t i;
-	unsigned long bits_offset;
+	unsigned long offset;
 	unsigned long gpio_mask;
-	const unsigned int gpio_reg_size = 8;
-	const unsigned long port_mask = GENMASK(gpio_reg_size, 0);
-	unsigned long flags;
-	unsigned int out_state;
 	void __iomem *ports[] = {
 		&idio24gpio->reg->out0_7, &idio24gpio->reg->out8_15,
 		&idio24gpio->reg->out16_23
 	};
+	size_t index;
+	unsigned long bitmask;
+	unsigned long flags;
+	unsigned long out_state;
 	const unsigned long out_mode_mask = BIT(1);
-	const unsigned int ttl_offset = 48;
-	const size_t ttl_i = BIT_WORD(ttl_offset);
-	const unsigned int word_offset = ttl_offset % BITS_PER_LONG;
-	const unsigned long ttl_mask = (mask[ttl_i] >> word_offset) & port_mask;
-	const unsigned long ttl_bits = (bits[ttl_i] >> word_offset) & ttl_mask;
-
-	/* set bits are processed a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports); i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
-
-		/* check if any set bits for current port */
-		gpio_mask = (*mask >> bits_offset) & port_mask;
-		if (!gpio_mask) {
-			/* no set bits for this port so move on to next port */
-			continue;
-		}
 
-		raw_spin_lock_irqsave(&idio24gpio->lock, flags);
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		index = offset / 8;
 
-		/* process output lines */
-		out_state = ioread8(ports[i]) & ~gpio_mask;
-		out_state |= (*bits >> bits_offset) & gpio_mask;
-		iowrite8(out_state, ports[i]);
+		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
 
-		raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
-	}
+		raw_spin_lock_irqsave(&idio24gpio->lock, flags);
 
-	/* check if setting TTL lines and if they are in output mode */
-	if (!ttl_mask || !(ioread8(&idio24gpio->reg->ctl) & out_mode_mask))
-		return;
+		/* read bits from current gpio port (port 6 is TTL GPIO) */
+		if (index < 6) {
+			out_state = ioread8(ports[index]);
+		} else if (ioread8(&idio24gpio->reg->ctl) & out_mode_mask) {
+			out_state = ioread8(&idio24gpio->reg->ttl_out0_7);
+		} else {
+			/* skip TTL GPIO if set for input */
+			raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
+			continue;
+		}
 
-	/* handle TTL output */
-	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
+		/* set requested bit states */
+		out_state &= ~gpio_mask;
+		out_state |= bitmask;
 
-	/* process output lines */
-	out_state = ioread8(&idio24gpio->reg->ttl_out0_7) & ~ttl_mask;
-	out_state |= ttl_bits;
-	iowrite8(out_state, &idio24gpio->reg->ttl_out0_7);
+		/* write bits for current gpio port (port 6 is TTL GPIO) */
+		if (index < 6)
+			iowrite8(out_state, ports[index]);
+		else
+			iowrite8(out_state, &idio24gpio->reg->ttl_out0_7);
 
-	raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
+		raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
+	}
 }
 
 static void idio_24_irq_ack(struct irq_data *data)
Signature · 1 line
2.23.0
#25 lore raw

PATCH v17 8/14 gpio: pcie-idio-24: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple/set_multiple callbacks with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-pcie-idio-24.c | 109 ++++++++++++-------------------
 1 file changed, 40 insertions(+), 69 deletions(-)
drivers/gpio/gpio-pcie-idio-24.c +40 -69
index 52f1647a46fd..924ec916b358 100644
@@ -198,52 +198,34 @@ static int idio_24_gpio_get_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_24_gpio *const idio24gpio = gpiochip_get_data(chip);
-	size_t i;
-	const unsigned int gpio_reg_size = 8;
-	unsigned int bits_offset;
-	size_t word_index;
-	unsigned int word_offset;
-	unsigned long word_mask;
-	const unsigned long port_mask = GENMASK(gpio_reg_size - 1, 0);
-	unsigned long port_state;
+	unsigned long offset;
+	unsigned long gpio_mask;
 	void __iomem *ports[] = {
 		&idio24gpio->reg->out0_7, &idio24gpio->reg->out8_15,
 		&idio24gpio->reg->out16_23, &idio24gpio->reg->in0_7,
 		&idio24gpio->reg->in8_15, &idio24gpio->reg->in16_23,
 	};
+	size_t index;
+	unsigned long port_state;
 	const unsigned long out_mode_mask = BIT(1);
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	/* get bits are evaluated a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports) + 1; i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
-
-		/* word index for bits array */
-		word_index = BIT_WORD(bits_offset);
-
-		/* gpio offset within current word of bits array */
-		word_offset = bits_offset % BITS_PER_LONG;
-
-		/* mask of get bits for current gpio within current word */
-		word_mask = mask[word_index] & (port_mask << word_offset);
-		if (!word_mask) {
-			/* no get bits in this port so skip to next one */
-			continue;
-		}
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		index = offset / 8;
 
 		/* read bits from current gpio port (port 6 is TTL GPIO) */
-		if (i < 6)
-			port_state = ioread8(ports[i]);
+		if (index < 6)
+			port_state = ioread8(ports[index]);
 		else if (ioread8(&idio24gpio->reg->ctl) & out_mode_mask)
 			port_state = ioread8(&idio24gpio->reg->ttl_out0_7);
 		else
 			port_state = ioread8(&idio24gpio->reg->ttl_in0_7);
 
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= (port_state << word_offset) & word_mask;
+		port_state &= gpio_mask;
+
+		bitmap_set_value8(bits, port_state, offset);
 	}
 
 	return 0;
@@ -294,59 +276,48 @@ static void idio_24_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_24_gpio *const idio24gpio = gpiochip_get_data(chip);
-	size_t i;
-	unsigned long bits_offset;
+	unsigned long offset;
 	unsigned long gpio_mask;
-	const unsigned int gpio_reg_size = 8;
-	const unsigned long port_mask = GENMASK(gpio_reg_size, 0);
-	unsigned long flags;
-	unsigned int out_state;
 	void __iomem *ports[] = {
 		&idio24gpio->reg->out0_7, &idio24gpio->reg->out8_15,
 		&idio24gpio->reg->out16_23
 	};
+	size_t index;
+	unsigned long bitmask;
+	unsigned long flags;
+	unsigned long out_state;
 	const unsigned long out_mode_mask = BIT(1);
-	const unsigned int ttl_offset = 48;
-	const size_t ttl_i = BIT_WORD(ttl_offset);
-	const unsigned int word_offset = ttl_offset % BITS_PER_LONG;
-	const unsigned long ttl_mask = (mask[ttl_i] >> word_offset) & port_mask;
-	const unsigned long ttl_bits = (bits[ttl_i] >> word_offset) & ttl_mask;
-
-	/* set bits are processed a gpio port register at a time */
-	for (i = 0; i < ARRAY_SIZE(ports); i++) {
-		/* gpio offset in bits array */
-		bits_offset = i * gpio_reg_size;
-
-		/* check if any set bits for current port */
-		gpio_mask = (*mask >> bits_offset) & port_mask;
-		if (!gpio_mask) {
-			/* no set bits for this port so move on to next port */
-			continue;
-		}
 
-		raw_spin_lock_irqsave(&idio24gpio->lock, flags);
+	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+		index = offset / 8;
 
-		/* process output lines */
-		out_state = ioread8(ports[i]) & ~gpio_mask;
-		out_state |= (*bits >> bits_offset) & gpio_mask;
-		iowrite8(out_state, ports[i]);
+		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
 
-		raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
-	}
+		raw_spin_lock_irqsave(&idio24gpio->lock, flags);
 
-	/* check if setting TTL lines and if they are in output mode */
-	if (!ttl_mask || !(ioread8(&idio24gpio->reg->ctl) & out_mode_mask))
-		return;
+		/* read bits from current gpio port (port 6 is TTL GPIO) */
+		if (index < 6) {
+			out_state = ioread8(ports[index]);
+		} else if (ioread8(&idio24gpio->reg->ctl) & out_mode_mask) {
+			out_state = ioread8(&idio24gpio->reg->ttl_out0_7);
+		} else {
+			/* skip TTL GPIO if set for input */
+			raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
+			continue;
+		}
 
-	/* handle TTL output */
-	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
+		/* set requested bit states */
+		out_state &= ~gpio_mask;
+		out_state |= bitmask;
 
-	/* process output lines */
-	out_state = ioread8(&idio24gpio->reg->ttl_out0_7) & ~ttl_mask;
-	out_state |= ttl_bits;
-	iowrite8(out_state, &idio24gpio->reg->ttl_out0_7);
+		/* write bits for current gpio port (port 6 is TTL GPIO) */
+		if (index < 6)
+			iowrite8(out_state, ports[index]);
+		else
+			iowrite8(out_state, &idio24gpio->reg->ttl_out0_7);
 
-	raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
+		raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
+	}
 }
 
 static void idio_24_irq_ack(struct irq_data *data)
Signature · 1 line
2.23.0
#26 lore raw

PATCH v17 9/14 gpio: uniphier: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in set_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity. An improvement in this case is that banks that are not masked will now be skipped.

---
 drivers/gpio/gpio-uniphier.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/gpio/gpio-uniphier.c +6 -10
index 93cdcc41e9fb..3e4b15d0231e 100644
@@ -15,9 +15,6 @@
 #include <linux/spinlock.h>
 #include <dt-bindings/gpio/uniphier-gpio.h>
 
-#define UNIPHIER_GPIO_BANK_MASK		\
-				GENMASK((UNIPHIER_GPIO_LINES_PER_BANK) - 1, 0)
-
 #define UNIPHIER_GPIO_IRQ_MAX_NUM	24
 
 #define UNIPHIER_GPIO_PORT_DATA		0x0	/* data */
@@ -147,15 +144,14 @@ static void uniphier_gpio_set(struct gpio_chip *chip,
 static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
 				       unsigned long *mask, unsigned long *bits)
 {
-	unsigned int bank, shift, bank_mask, bank_bits;
-	int i;
+	unsigned long i;
+	unsigned long bank_mask;
+	unsigned long bank;
+	unsigned long bank_bits;
 
-	for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_LINES_PER_BANK) {
+	for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
 		bank = i / UNIPHIER_GPIO_LINES_PER_BANK;
-		shift = i % BITS_PER_LONG;
-		bank_mask = (mask[BIT_WORD(i)] >> shift) &
-						UNIPHIER_GPIO_BANK_MASK;
-		bank_bits = bits[BIT_WORD(i)] >> shift;
+		bank_bits = bitmap_get_value8(bits, i);
 
 		uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_PORT_DATA,
 					 bank_mask, bank_bits);
Signature · 1 line
2.23.0
#27 lore raw

Re: [PATCH v17 09/14] gpio: uniphier: Utilize for_each_set_clump8 macro

Quoted 27 lines · William Breathitt Gray

Replace verbose implementation in set_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity. An improvement in this case is that banks that are not masked will now be skipped.

---
 drivers/gpio/gpio-uniphier.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/gpio/gpio-uniphier.c +0 -3
index 93cdcc41e9fb..3e4b15d0231e 100644
@@ -15,9 +15,6 @@
 #include <linux/spinlock.h>
 #include <dt-bindings/gpio/uniphier-gpio.h>
 
-#define UNIPHIER_GPIO_BANK_MASK                \
-                               GENMASK((UNIPHIER_GPIO_LINES_PER_BANK) - 1, 0)
-
 #define UNIPHIER_GPIO_IRQ_MAX_NUM      24
 
 #define UNIPHIER_GPIO_PORT_DATA                0x0     /* data */

drivers/gpio/gpio-uniphier.c static void uniphier_gpio_set(struct gpio_chip *chip, · line 150

Quoted 9 lines
drivers/gpio/gpio-uniphier.c +4 -2
@@ -147,15 +144,14 @@ static void uniphier_gpio_set(struct gpio_chip *chip,
 static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
                                       unsigned long *mask, unsigned long *bits)
 {
-       unsigned int bank, shift, bank_mask, bank_bits;
-       int i;
+       unsigned long i;
+       unsigned long bank_mask;
+       unsigned long bank;
+       unsigned long bank_bits;

Please do not split it into multiple lines
unless you need to do so.

Thanks.

Quoted 14 lines
drivers/gpio/gpio-uniphier.c +2 -6
-       for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_LINES_PER_BANK) {
+       for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
                bank = i / UNIPHIER_GPIO_LINES_PER_BANK;
-               shift = i % BITS_PER_LONG;
-               bank_mask = (mask[BIT_WORD(i)] >> shift) &
-                                               UNIPHIER_GPIO_BANK_MASK;
-               bank_bits = bits[BIT_WORD(i)] >> shift;
+               bank_bits = bitmap_get_value8(bits, i);
 
                uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_PORT_DATA,
                                         bank_mask, bank_bits);
--

2.23.0

Signature · 2 lines
Best Regards
Masahiro Yamada
#28 lore raw

Re: [PATCH v17 09/14] gpio: uniphier: Utilize for_each_set_clump8 macro

Quoted 43 lines · Masahiro Yamada
Quoted 27 lines · William Breathitt Gray

Replace verbose implementation in set_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity. An improvement in this case is that banks that are not masked will now be skipped.

---
 drivers/gpio/gpio-uniphier.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/gpio/gpio-uniphier.c +0 -3
index 93cdcc41e9fb..3e4b15d0231e 100644
@@ -15,9 +15,6 @@
 #include <linux/spinlock.h>
 #include <dt-bindings/gpio/uniphier-gpio.h>
 
-#define UNIPHIER_GPIO_BANK_MASK                \
-                               GENMASK((UNIPHIER_GPIO_LINES_PER_BANK) - 1, 0)
-
 #define UNIPHIER_GPIO_IRQ_MAX_NUM      24
 
 #define UNIPHIER_GPIO_PORT_DATA                0x0     /* data */

drivers/gpio/gpio-uniphier.c static void uniphier_gpio_set(struct gpio_chip *chip, · line 150

Quoted 9 lines
drivers/gpio/gpio-uniphier.c +4 -2
@@ -147,15 +144,14 @@ static void uniphier_gpio_set(struct gpio_chip *chip,
 static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
                                       unsigned long *mask, unsigned long *bits)
 {
-       unsigned int bank, shift, bank_mask, bank_bits;
-       int i;
+       unsigned long i;
+       unsigned long bank_mask;
+       unsigned long bank;
+       unsigned long bank_bits;

Please do not split it into multiple lines
unless you need to do so.

Quoted 1 line

Thanks.

No problem, I'll update this patch to declare them all on the same line.

William Breathitt Gray

Quoted 19 lines
Quoted 14 lines
drivers/gpio/gpio-uniphier.c +2 -6
-       for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_LINES_PER_BANK) {
+       for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
                bank = i / UNIPHIER_GPIO_LINES_PER_BANK;
-               shift = i % BITS_PER_LONG;
-               bank_mask = (mask[BIT_WORD(i)] >> shift) &
-                                               UNIPHIER_GPIO_BANK_MASK;
-               bank_bits = bits[BIT_WORD(i)] >> shift;
+               bank_bits = bitmap_get_value8(bits, i);
 
                uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_PORT_DATA,
                                         bank_mask, bank_bits);
--

2.23.0

Signature · 2 lines
Best Regards
Masahiro Yamada
#29 lore raw

PATCH v17 9/14 gpio: uniphier: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in set_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity. An improvement in this case is that banks that are not masked will now be skipped.

---
 drivers/gpio/gpio-uniphier.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/gpio/gpio-uniphier.c +6 -10
index 93cdcc41e9fb..3e4b15d0231e 100644
@@ -15,9 +15,6 @@
 #include <linux/spinlock.h>
 #include <dt-bindings/gpio/uniphier-gpio.h>
 
-#define UNIPHIER_GPIO_BANK_MASK		\
-				GENMASK((UNIPHIER_GPIO_LINES_PER_BANK) - 1, 0)
-
 #define UNIPHIER_GPIO_IRQ_MAX_NUM	24
 
 #define UNIPHIER_GPIO_PORT_DATA		0x0	/* data */
@@ -147,15 +144,14 @@ static void uniphier_gpio_set(struct gpio_chip *chip,
 static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
 				       unsigned long *mask, unsigned long *bits)
 {
-	unsigned int bank, shift, bank_mask, bank_bits;
-	int i;
+	unsigned long i;
+	unsigned long bank_mask;
+	unsigned long bank;
+	unsigned long bank_bits;
 
-	for (i = 0; i < chip->ngpio; i += UNIPHIER_GPIO_LINES_PER_BANK) {
+	for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
 		bank = i / UNIPHIER_GPIO_LINES_PER_BANK;
-		shift = i % BITS_PER_LONG;
-		bank_mask = (mask[BIT_WORD(i)] >> shift) &
-						UNIPHIER_GPIO_BANK_MASK;
-		bank_bits = bits[BIT_WORD(i)] >> shift;
+		bank_bits = bitmap_get_value8(bits, i);
 
 		uniphier_gpio_bank_write(chip, bank, UNIPHIER_GPIO_PORT_DATA,
 					 bank_mask, bank_bits);
Signature · 1 line
2.23.0
#30 lore raw

PATCH v17 10/14 gpio: 74x164: Utilize the for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in set_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-74x164.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)
drivers/gpio/gpio-74x164.c +9 -10
index e81307f9754e..05637d585152 100644
@@ -6,6 +6,7 @@
  *  Copyright (C) 2010 Miguel Gaio <[email protected]>
  */
 
+#include <linux/bitops.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
@@ -72,20 +73,18 @@ static void gen_74x164_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 				    unsigned long *bits)
 {
 	struct gen_74x164_chip *chip = gpiochip_get_data(gc);
-	unsigned int i, idx, shift;
-	u8 bank, bankmask;
+	unsigned long offset;
+	unsigned long bankmask;
+	size_t bank;
+	unsigned long bitmask;
 
 	mutex_lock(&chip->lock);
-	for (i = 0, bank = chip->registers - 1; i < chip->registers;
-	     i++, bank--) {
-		idx = i / sizeof(*mask);
-		shift = i % sizeof(*mask) * BITS_PER_BYTE;
-		bankmask = mask[idx] >> shift;
-		if (!bankmask)
-			continue;
+	for_each_set_clump8(offset, bankmask, mask, chip->registers * 8) {
+		bank = chip->registers - 1 - offset / 8;
+		bitmask = bitmap_get_value8(bits, offset) & bankmask;
 
 		chip->buffer[bank] &= ~bankmask;
-		chip->buffer[bank] |= bankmask & (bits[idx] >> shift);
+		chip->buffer[bank] |= bitmask;
 	}
 	__gen_74x164_write_config(chip);
 	mutex_unlock(&chip->lock);
Signature · 1 line
2.23.0
#31 lore raw

PATCH v17 10/14 gpio: 74x164: Utilize the for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in set_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-74x164.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)
drivers/gpio/gpio-74x164.c +9 -10
index e81307f9754e..05637d585152 100644
@@ -6,6 +6,7 @@
  *  Copyright (C) 2010 Miguel Gaio <[email protected]>
  */
 
+#include <linux/bitops.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
@@ -72,20 +73,18 @@ static void gen_74x164_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 				    unsigned long *bits)
 {
 	struct gen_74x164_chip *chip = gpiochip_get_data(gc);
-	unsigned int i, idx, shift;
-	u8 bank, bankmask;
+	unsigned long offset;
+	unsigned long bankmask;
+	size_t bank;
+	unsigned long bitmask;
 
 	mutex_lock(&chip->lock);
-	for (i = 0, bank = chip->registers - 1; i < chip->registers;
-	     i++, bank--) {
-		idx = i / sizeof(*mask);
-		shift = i % sizeof(*mask) * BITS_PER_BYTE;
-		bankmask = mask[idx] >> shift;
-		if (!bankmask)
-			continue;
+	for_each_set_clump8(offset, bankmask, mask, chip->registers * 8) {
+		bank = chip->registers - 1 - offset / 8;
+		bitmask = bitmap_get_value8(bits, offset) & bankmask;
 
 		chip->buffer[bank] &= ~bankmask;
-		chip->buffer[bank] |= bankmask & (bits[idx] >> shift);
+		chip->buffer[bank] |= bitmask;
 	}
 	__gen_74x164_write_config(chip);
 	mutex_unlock(&chip->lock);
Signature · 1 line
2.23.0
#32 lore raw

PATCH v17 11/14 thermal: intel: intel_soc_dts_iosf: Utilize for_each_set_clump8 macro

In reply to William Breathitt Gray

Utilize for_each_set_clump8 macro, and the bitmap_set_value8 and bitmap_get_value8 functions, where appropriate. In addition, remove the now unnecessary temp_mask and temp_shift members of the intel_soc_dts_sensor_entry structure.

---
 drivers/thermal/intel/intel_soc_dts_iosf.c | 31 +++++++++++++---------
 drivers/thermal/intel/intel_soc_dts_iosf.h |  2 --
 2 files changed, 18 insertions(+), 15 deletions(-)
drivers/thermal/intel/intel_soc_dts_iosf.c +18 -13
index 5716b62e0f73..f75271b669c6 100644
@@ -6,6 +6,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/bitops.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/interrupt.h>
@@ -103,6 +104,7 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
 	int status;
 	u32 temp_out;
 	u32 out;
+	unsigned long update_ptps;
 	u32 store_ptps;
 	u32 store_ptmc;
 	u32 store_te_out;
@@ -120,8 +122,10 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
 	if (status)
 		return status;
 
-	out = (store_ptps & ~(0xFF << (thres_index * 8)));
-	out |= (temp_out & 0xFF) << (thres_index * 8);
+	update_ptps = store_ptps;
+	bitmap_set_value8(&update_ptps, temp_out & 0xFF, thres_index * 8);
+	out = update_ptps;
+
 	status = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
 				SOC_DTS_OFFSET_PTPS, out);
 	if (status)
@@ -223,6 +227,7 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
 	u32 out;
 	struct intel_soc_dts_sensor_entry *dts;
 	struct intel_soc_dts_sensors *sensors;
+	unsigned long raw;
 
 	dts = tzd->devdata;
 	sensors = dts->sensors;
@@ -231,8 +236,8 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
 	if (status)
 		return status;
 
-	out = (out & dts->temp_mask) >> dts->temp_shift;
-	out -= SOC_DTS_TJMAX_ENCODING;
+	raw = out;
+	out = bitmap_get_value8(&raw, dts->id * 8) - SOC_DTS_TJMAX_ENCODING;
 	*temp = sensors->tj_max - out * 1000;
 
 	return 0;
@@ -280,11 +285,14 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
 				int read_only_trip_cnt)
 {
 	char name[10];
+	unsigned long trip;
 	int trip_count = 0;
 	int trip_mask = 0;
+	int writable_trip_cnt = 0;
+	unsigned long ptps;
 	u32 store_ptps;
+	unsigned long i;
 	int ret;
-	int i;
 
 	/* Store status to restor on exit */
 	ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
@@ -293,11 +301,10 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
 		goto err_ret;
 
 	dts->id = id;
-	dts->temp_mask = 0x00FF << (id * 8);
-	dts->temp_shift = id * 8;
 	if (notification_support) {
 		trip_count = min(SOC_MAX_DTS_TRIPS, trip_cnt);
-		trip_mask = BIT(trip_count - read_only_trip_cnt) - 1;
+		writable_trip_cnt = trip_count - read_only_trip_cnt;
+		trip_mask = GENMASK(writable_trip_cnt - 1, 0);
 	}
 
 	/* Check if the writable trip we provide is not used by BIOS */
@@ -306,11 +313,9 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
 	if (ret)
 		trip_mask = 0;
 	else {
-		for (i = 0; i < trip_count; ++i) {
-			if (trip_mask & BIT(i))
-				if (store_ptps & (0xff << (i * 8)))
-					trip_mask &= ~BIT(i);
-		}
+		ptps = store_ptps;
+		for_each_set_clump8(i, trip, &ptps, writable_trip_cnt * 8)
+			trip_mask &= ~BIT(i / 8);
 	}
 	dts->trip_mask = trip_mask;
 	dts->trip_count = trip_count;
drivers/thermal/intel/intel_soc_dts_iosf.h +0 -2
index adfb09af33fc..c54945748200 100644
@@ -24,8 +24,6 @@ struct intel_soc_dts_sensors;
 
 struct intel_soc_dts_sensor_entry {
 	int id;
-	u32 temp_mask;
-	u32 temp_shift;
 	u32 store_status;
 	u32 trip_mask;
 	u32 trip_count;
Signature · 1 line
2.23.0
#33 lore raw

PATCH v17 12/14 gpio: pisosr: Utilize the for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity.

Cc Morten Hein Tiljeset <[email protected]>
Cc Sean Nyekjaer <[email protected]>
---
 drivers/gpio/gpio-pisosr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/gpio/gpio-pisosr.c +6 -6
index f809a5a8e9eb..9ab2c044ef52 100644
@@ -96,16 +96,16 @@ static int pisosr_gpio_get_multiple(struct gpio_chip *chip,
 				    unsigned long *mask, unsigned long *bits)
 {
 	struct pisosr_gpio *gpio = gpiochip_get_data(chip);
-	unsigned int nbytes = DIV_ROUND_UP(chip->ngpio, 8);
-	unsigned int i, j;
+	unsigned long offset;
+	unsigned long gpio_mask;
+	unsigned long buffer_state;
 
 	pisosr_gpio_refresh(gpio);
 
 	bitmap_zero(bits, chip->ngpio);
-	for (i = 0; i < nbytes; i++) {
-		j = i / sizeof(unsigned long);
-		bits[j] |= ((unsigned long) gpio->buffer[i])
-			   << (8 * (i % sizeof(unsigned long)));
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
+		buffer_state = gpio->buffer[offset / 8] & gpio_mask;
+		bitmap_set_value8(bits, buffer_state, offset);
 	}
 
 	return 0;
Signature · 1 line
2.23.0
#34 lore raw

PATCH v17 12/14 gpio: pisosr: Utilize the for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity.

Cc Morten Hein Tiljeset <[email protected]>
Cc Sean Nyekjaer <[email protected]>
---
 drivers/gpio/gpio-pisosr.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/gpio/gpio-pisosr.c +6 -6
index f809a5a8e9eb..9ab2c044ef52 100644
@@ -96,16 +96,16 @@ static int pisosr_gpio_get_multiple(struct gpio_chip *chip,
 				    unsigned long *mask, unsigned long *bits)
 {
 	struct pisosr_gpio *gpio = gpiochip_get_data(chip);
-	unsigned int nbytes = DIV_ROUND_UP(chip->ngpio, 8);
-	unsigned int i, j;
+	unsigned long offset;
+	unsigned long gpio_mask;
+	unsigned long buffer_state;
 
 	pisosr_gpio_refresh(gpio);
 
 	bitmap_zero(bits, chip->ngpio);
-	for (i = 0; i < nbytes; i++) {
-		j = i / sizeof(unsigned long);
-		bits[j] |= ((unsigned long) gpio->buffer[i])
-			   << (8 * (i % sizeof(unsigned long)));
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
+		buffer_state = gpio->buffer[offset / 8] & gpio_mask;
+		bitmap_set_value8(bits, buffer_state, offset);
 	}
 
 	return 0;
Signature · 1 line
2.23.0
#35 lore raw

PATCH v17 13/14 gpio: max3191x: Utilize the for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity.

Cc Mathias Duckeck <[email protected]>
Cc Lukas Wunner <[email protected]>
---
 drivers/gpio/gpio-max3191x.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
drivers/gpio/gpio-max3191x.c +10 -9
index 4b4b2ceb82fc..0242c6187bf5 100644
@@ -31,6 +31,7 @@
  */
 
 #include <linux/bitmap.h>
+#include <linux/bitops.h>
 #include <linux/crc8.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
@@ -232,16 +233,20 @@ static int max3191x_get_multiple(struct gpio_chip *gpio, unsigned long *mask,
 				 unsigned long *bits)
 {
 	struct max3191x_chip *max3191x = gpiochip_get_data(gpio);
-	int ret, bit = 0, wordlen = max3191x_wordlen(max3191x);
+	const unsigned int wordlen = max3191x_wordlen(max3191x);
+	int ret;
+	unsigned long bit;
+	unsigned long gpio_mask;
+	unsigned long in;
 
 	mutex_lock(&max3191x->lock);
 	ret = max3191x_readout_locked(max3191x);
 	if (ret)
 		goto out_unlock;
 
-	while ((bit = find_next_bit(mask, gpio->ngpio, bit)) != gpio->ngpio) {
+	bitmap_zero(bits, gpio->ngpio);
+	for_each_set_clump8(bit, gpio_mask, mask, gpio->ngpio) {
 		unsigned int chipnum = bit / MAX3191X_NGPIO;
-		unsigned long in, shift, index;
 
 		if (max3191x_chip_is_faulting(max3191x, chipnum)) {
 			ret = -EIO;
@@ -249,12 +254,8 @@ static int max3191x_get_multiple(struct gpio_chip *gpio, unsigned long *mask,
 		}
 
 		in = ((u8 *)max3191x->xfer.rx_buf)[chipnum * wordlen];
-		shift = round_down(bit % BITS_PER_LONG, MAX3191X_NGPIO);
-		index = bit / BITS_PER_LONG;
-		bits[index] &= ~(mask[index] & (0xff << shift));
-		bits[index] |= mask[index] & (in << shift); /* copy bits */
-
-		bit = (chipnum + 1) * MAX3191X_NGPIO; /* go to next chip */
+		in &= gpio_mask;
+		bitmap_set_value8(bits, in, bit);
 	}
 
 out_unlock:
Signature · 1 line
2.23.0
#36 lore raw

PATCH v17 13/14 gpio: max3191x: Utilize the for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in get_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity.

Cc Mathias Duckeck <[email protected]>
Cc Lukas Wunner <[email protected]>
---
 drivers/gpio/gpio-max3191x.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
drivers/gpio/gpio-max3191x.c +10 -9
index 4b4b2ceb82fc..0242c6187bf5 100644
@@ -31,6 +31,7 @@
  */
 
 #include <linux/bitmap.h>
+#include <linux/bitops.h>
 #include <linux/crc8.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
@@ -232,16 +233,20 @@ static int max3191x_get_multiple(struct gpio_chip *gpio, unsigned long *mask,
 				 unsigned long *bits)
 {
 	struct max3191x_chip *max3191x = gpiochip_get_data(gpio);
-	int ret, bit = 0, wordlen = max3191x_wordlen(max3191x);
+	const unsigned int wordlen = max3191x_wordlen(max3191x);
+	int ret;
+	unsigned long bit;
+	unsigned long gpio_mask;
+	unsigned long in;
 
 	mutex_lock(&max3191x->lock);
 	ret = max3191x_readout_locked(max3191x);
 	if (ret)
 		goto out_unlock;
 
-	while ((bit = find_next_bit(mask, gpio->ngpio, bit)) != gpio->ngpio) {
+	bitmap_zero(bits, gpio->ngpio);
+	for_each_set_clump8(bit, gpio_mask, mask, gpio->ngpio) {
 		unsigned int chipnum = bit / MAX3191X_NGPIO;
-		unsigned long in, shift, index;
 
 		if (max3191x_chip_is_faulting(max3191x, chipnum)) {
 			ret = -EIO;
@@ -249,12 +254,8 @@ static int max3191x_get_multiple(struct gpio_chip *gpio, unsigned long *mask,
 		}
 
 		in = ((u8 *)max3191x->xfer.rx_buf)[chipnum * wordlen];
-		shift = round_down(bit % BITS_PER_LONG, MAX3191X_NGPIO);
-		index = bit / BITS_PER_LONG;
-		bits[index] &= ~(mask[index] & (0xff << shift));
-		bits[index] |= mask[index] & (in << shift); /* copy bits */
-
-		bit = (chipnum + 1) * MAX3191X_NGPIO; /* go to next chip */
+		in &= gpio_mask;
+		bitmap_set_value8(bits, in, bit);
 	}
 
 out_unlock:
Signature · 1 line
2.23.0
#37 lore raw

PATCH v17 14/14 gpio: pca953x: Utilize the for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in set_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-pca953x.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
drivers/gpio/gpio-pca953x.c +7 -10
index de5d1383f28d..10b669b8f27d 100644
@@ -10,6 +10,7 @@
 
 #include <linux/acpi.h>
 #include <linux/bits.h>
+#include <linux/bitops.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -456,7 +457,8 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 				      unsigned long *mask, unsigned long *bits)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	unsigned int bank_mask, bank_val;
+	unsigned long offset;
+	unsigned long bank_mask;
 	int bank;
 	u8 reg_val[MAX_BANK];
 	int ret;
@@ -466,15 +468,10 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 	if (ret)
 		goto exit;
 
-	for (bank = 0; bank < NBANK(chip); bank++) {
-		bank_mask = mask[bank / sizeof(*mask)] >>
-			   ((bank % sizeof(*mask)) * 8);
-		if (bank_mask) {
-			bank_val = bits[bank / sizeof(*bits)] >>
-				  ((bank % sizeof(*bits)) * 8);
-			bank_val &= bank_mask;
-			reg_val[bank] = (reg_val[bank] & ~bank_mask) | bank_val;
-		}
+	for_each_set_clump8(offset, bank_mask, mask, gc->ngpio) {
+		bank = offset / 8;
+		reg_val[bank] &= ~bank_mask;
+		reg_val[bank] |= bitmap_get_value8(bits, offset) & bank_mask;
 	}
 
 	pca953x_write_regs(chip, chip->regs->output, reg_val);
Signature · 1 line
2.23.0
#38 lore raw

PATCH v17 14/14 gpio: pca953x: Utilize the for_each_set_clump8 macro

In reply to William Breathitt Gray

Replace verbose implementation in set_multiple callback with for_each_set_clump8 macro to simplify code and improve clarity.

---
 drivers/gpio/gpio-pca953x.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
drivers/gpio/gpio-pca953x.c +7 -10
index de5d1383f28d..10b669b8f27d 100644
@@ -10,6 +10,7 @@
 
 #include <linux/acpi.h>
 #include <linux/bits.h>
+#include <linux/bitops.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -456,7 +457,8 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 				      unsigned long *mask, unsigned long *bits)
 {
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
-	unsigned int bank_mask, bank_val;
+	unsigned long offset;
+	unsigned long bank_mask;
 	int bank;
 	u8 reg_val[MAX_BANK];
 	int ret;
@@ -466,15 +468,10 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc,
 	if (ret)
 		goto exit;
 
-	for (bank = 0; bank < NBANK(chip); bank++) {
-		bank_mask = mask[bank / sizeof(*mask)] >>
-			   ((bank % sizeof(*mask)) * 8);
-		if (bank_mask) {
-			bank_val = bits[bank / sizeof(*bits)] >>
-				  ((bank % sizeof(*bits)) * 8);
-			bank_val &= bank_mask;
-			reg_val[bank] = (reg_val[bank] & ~bank_mask) | bank_val;
-		}
+	for_each_set_clump8(offset, bank_mask, mask, gc->ngpio) {
+		bank = offset / 8;
+		reg_val[bank] &= ~bank_mask;
+		reg_val[bank] |= bitmap_get_value8(bits, offset) & bank_mask;
 	}
 
 	pca953x_write_regs(chip, chip->regs->output, reg_val);
Signature · 1 line
2.23.0