Introduce the for_each_set_port_word macro

PATCH Superseded William Breathitt Gray 2 participants 13 messages May 8, 2018 – May 14, 2018

Subsystems (11)

Library code GPIO Generic include/asm header files Bitmap API +7 more subsystems

lore

Share & tools

lore

William Breathitt Gray

author

2018-05-08 13:26 UTC #0 lore raw

v2 cover Introduce the for_each_set_port_word macro

Changes in v2:
- Include linux/types.h file in bitops/find.h to define size_t

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_port_word macro and utilizes it in several GPIO drivers. The for_each_set_port_word macro facilitates a for-loop syntax that iterates over entire groups of set bits at a time.

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

    Example:        1011 1110 0000 1111
    First loop:     1011 1110 0000 XXXX
    Second loop:    1011 XXXX 0000 1111
    Third loop:     XXXX 1110 0000 1111

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

The for_each_set_port_word macro has six parameters:

* port_word: set to current port word index for the iteration
* word_index: set to current bitmap word index for the iteration
* word_offset: bits offset of the found port word in the bitmap word
* bits: bitmap to search within
* size: bitmap size in number of port words
* port_size: port word size in number of bits

The port_size argument can be an arbitrary number of bits and is not required to be a multiple of 2.

I've called the group of bits a "port word" which may be a confusing naming convention; I was afraid calling that them a "group" may be too vague. Should a different name be chosen; what would you suggest?

This patchset was rebased on top of the following three commits:

* commit aaf96e51de11 ("gpio: pci-idio-16: Fix port memory offset for get_multiple callback")
* commit 304440aa96c6 ("gpio: pcie-idio-24: Fix port memory offset for get_multiple/set_multiple callbacks")
* commit e026646c178d ("gpio: pcie-idio-24: Fix off-by-one error in get_multiple loop")

William Breathitt Gray

William Breathitt Gray (7):
bitops: Introduce the for_each_set_port_word macro
gpio: 104-dio-48e: Utilize for_each_set_port_word macro
gpio: 104-idi-48: Utilize for_each_set_port_word macro
gpio: gpio-mm: Utilize for_each_set_port_word macro
gpio: ws16c48: Utilize for_each_set_port_word macro
gpio: pci-idio-16: Utilize for_each_set_port_word macro
gpio: pcie-idio-24: Utilize for_each_set_port_word macro

 drivers/gpio/gpio-104-dio-48e.c   |  67 +++++---------------
 drivers/gpio/gpio-104-idi-48.c    |  32 ++--------
 drivers/gpio/gpio-gpio-mm.c       |  67 +++++---------------
 drivers/gpio/gpio-pci-idio-16.c   |  67 ++++++--------------
 drivers/gpio/gpio-pcie-idio-24.c  | 102 +++++++++++-------------------
 drivers/gpio/gpio-ws16c48.c       |  66 +++++--------------
 include/asm-generic/bitops/find.h |  26 ++++++++
 include/linux/bitops.h            |   9 +++
 lib/find_bit.c                    |  28 ++++++++
 9 files changed, 172 insertions(+), 292 deletions(-)

Signature · 1 line

2.17.0

William Breathitt Gray

author

2018-05-08 13:26 UTC #1 lore raw

PATCH v2 1/7 bitops: Introduce the for_each_set_port_word macro

This macro iterates for each group of bits (port word) with set bits, within a bitmap memory region. For each iteration, "port_word" is set to the found port word index, "word_index" is set to the word index of the bitmap containing the found port word, and "word_offset" is set to the bit offset of the found port word within the respective bitmap word.

Cc Arnd Bergmann <[email protected]>

Signed-off-by William Breathitt Gray <[email protected]>

---
 include/asm-generic/bitops/find.h | 26 ++++++++++++++++++++++++++
 include/linux/bitops.h            |  9 +++++++++
 lib/find_bit.c                    | 28 ++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)

include/asm-generic/bitops/find.h +26 -0

index 8a1ee10014de..c59514547e63 100644
@@ -2,6 +2,8 @@
 #ifndef _ASM_GENERIC_BITOPS_FIND_H_
 #define _ASM_GENERIC_BITOPS_FIND_H_
 
+#include <linux/types.h>
+
 #ifndef find_next_bit
 /**
  * find_next_bit - find the next set bit in a memory region

@@ -80,4 +82,28 @@ extern unsigned long find_first_zero_bit(const unsigned long *addr,
 
 #endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
 
+/**
+ * find_next_port_word - find next port word with set bits in a memory region
+ * @word_index: location to store bitmap word index of found port word
+ * @word_offset: bits offset of the found port word in respective bitmap word
+ * @bits: address to base the search on
+ * @size: bitmap size in number of ports
+ * @offset: port word index to start searching at
+ * @port_size: port word size in bits
+ *
+ * Returns the port index for the next port word with set bits; the respective
+ * bitmap word index is stored at the location pointed by @word_index, and the
+ * bits offset of the found port word within the respective bitmap word is
+ * stored at the location pointed by @word_offset. If no bits are set, returns
+ * @size.
+ */
+size_t find_next_port_word(size_t *const word_index,
+			   unsigned int *const word_offset,
+			   const unsigned long *const bits, const size_t size,
+			   const size_t offset, const unsigned int port_size);
+
+#define find_first_port_word(word_index, word_offset, bits, size, port_size) \
+	find_next_port_word((word_index), (word_offset), (bits), (size), 0, \
+			    (port_size))
+
 #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */

include/linux/bitops.h +9 -0

index 4cac4e1a72ff..c3b7caf4ad2d 100644
@@ -59,6 +59,15 @@ extern unsigned long __sw_hweight64(__u64 w);
 	     (bit) < (size);					\
 	     (bit) = find_next_zero_bit((addr), (size), (bit) + 1))
 
+#define for_each_set_port_word(port_word, word_index, word_offset, bits, size, \
+			       port_size) \
+	for ((port_word) = find_first_port_word(&(word_index), &(word_offset), \
+					   (bits), (size), (port_size)); \
+	     (port_word) < (size); \
+	     (port_word) = find_next_port_word(&(word_index), &(word_offset), \
+					  (bits), (size), (port_word) + 1, \
+					  (port_size)))
+
 static inline int get_bitmask_order(unsigned int count)
 {
 	int order;

lib/find_bit.c +28 -0

index ee3df93ba69a..628c94d63d6b 100644
@@ -20,6 +20,7 @@
 #include <linux/bitmap.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
+#include <linux/types.h>
 
 #if !defined(find_next_bit) || !defined(find_next_zero_bit) || \
 		!defined(find_next_and_bit)

@@ -218,3 +219,30 @@ EXPORT_SYMBOL(find_next_bit_le);
 #endif
 
 #endif /* __BIG_ENDIAN */
+
+size_t find_next_port_word(size_t *const word_index,
+			   unsigned int *const word_offset,
+			   const unsigned long *const bits, const size_t size,
+			   const size_t offset, const unsigned int port_size)
+{
+	size_t i;
+	unsigned int bits_offset;
+	unsigned long word_mask;
+	const unsigned long port_mask = GENMASK(port_size - 1, 0);
+
+	for (i = offset; i < size; i++) {
+		bits_offset = i * port_size;
+
+		*word_index = BIT_WORD(bits_offset);
+		*word_offset = bits_offset % BITS_PER_LONG;
+
+		word_mask = bits[*word_index] & (port_mask << *word_offset);
+		if (!word_mask)
+			continue;
+
+		return i;
+	}
+
+	return size;
+}
+EXPORT_SYMBOL(find_next_port_word);

Signature · 1 line

2.17.0

Andy Shevchenko

2018-05-13 15:06 UTC #2 lore raw

Re: [PATCH v2 1/7] bitops: Introduce the for_each_set_port_word macro

Quoted 5 lines · William Breathitt Gray

This macro iterates for each group of bits (port word) with set bits, within a bitmap memory region. For each iteration, "port_word" is set to the found port word index, "word_index" is set to the word index of the bitmap containing the found port word, and "word_offset" is set to the bit offset of the found port word within the respective bitmap word.

Isn't that idea we discussed some time ago?

In any case, part "port" is too specific for a generic function like this. Please, get rid of it completely. No-one knows what port means here. Just makes a lot of confusion.

lib/find_bit.c line 24

Quoted 8 lines

lib/find_bit.c +1 -0

@@ -20,6 +20,7 @@
 #include <linux/bitmap.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
 
+#include <linux/types.h>

No need. It's included by bitmap.h IIRC.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

William Breathitt Gray

author

2018-05-14 13:04 UTC #3 lore raw

Re: [PATCH v2 1/7] bitops: Introduce the for_each_set_port_word macro

Quoted 9 lines · Andy Shevchenko

Quoted 5 lines · William Breathitt Gray
This macro iterates for each group of bits (port word) with set bits, within a bitmap memory region. For each iteration, "port_word" is set to the found port word index, "word_index" is set to the word index of the bitmap containing the found port word, and "word_offset" is set to the bit offset of the found port word within the respective bitmap word.
Isn't that idea we discussed some time ago?

That's right, I found the time to implement the macro suggestion you made during the get_multiple/set_multiple patchset for the PC104 GPIO drivers a while ago.

This macro greatly simplifies the callback function implementations in those drivers and reduces the repeated code that kept appearing among those drivers. Hopefully it can be useful for other drivers as well.

Quoted 4 lines

In any case, part "port" is too specific for a generic function like this. Please, get rid of it completely. No-one knows what port means here. Just makes a lot of confusion.

Okay, I'll come up with a better name and submit a version 3 of this patchset.

Quoted 11 lines

lib/find_bit.c line 24
Quoted 9 lines
lib/find_bit.c +1 -0
@@ -20,6 +20,7 @@
 #include <linux/bitmap.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
 
+#include <linux/types.h>
No need. It's included by bitmap.h IIRC.

Ah, you are correct, I'll remove this line then.

William Breathitt Gray

Quoted 5 lines

Signature · 3 lines
-- 
With Best Regards,
Andy Shevchenko

Andy Shevchenko

2018-05-14 17:08 UTC #4 lore raw

Re: [PATCH v2 1/7] bitops: Introduce the for_each_set_port_word macro

Quoted 9 lines · William Breathitt Gray

Quoted 8 lines · Andy Shevchenko
Quoted 5 lines · William Breathitt Gray
This macro iterates for each group of bits (port word) with set bits, within a bitmap memory region. For each iteration, "port_word" is set to the found port word index, "word_index" is set to the word index of the bitmap containing the found port word, and "word_offset" is set to the bit offset of the found port word within the respective bitmap word.

Quoted 5 lines

Quoted 1 line
Isn't that idea we discussed some time ago?
That's right, I found the time to implement the macro suggestion you made during the get_multiple/set_multiple patchset for the PC104 GPIO drivers a while ago.

So, if you find it appropriate, please add Suggested-by.

Quoted 3 lines

This macro greatly simplifies the callback function implementations in those drivers and reduces the repeated code that kept appearing among those drivers. Hopefully it can be useful for other drivers as well.

Yes, I like the idea!

Quoted 6 lines

Quoted 3 lines
In any case, part "port" is too specific for a generic function like this. Please, get rid of it completely. No-one knows what port means here. Just makes a lot of confusion.
Okay, I'll come up with a better name and submit a version 3 of this patchset.

I also forgot to mention that kernel-doc should accompany function definition (in *.c), and not a declaration (in *.h).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

William Breathitt Gray

author

2018-05-08 13:27 UTC #5 lore raw

PATCH v2 2/7 gpio: 104-dio-48e: Utilize for_each_set_port_word macro

In reply to William Breathitt Gray

Replace boilerplate code in get_multiple/set_multiple callbacks with for_each_set_port_word macro to simplify code and improve clarity.

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-104-dio-48e.c | 67 ++++++++-------------------------
 1 file changed, 16 insertions(+), 51 deletions(-)

drivers/gpio/gpio-104-dio-48e.c +16 ~~-51~~

index 9c4e07fcb74b..843f1c71e814 100644
@@ -183,46 +183,23 @@ 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);
+	size_t word;
+	unsigned int offset;
 	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 */
+	for_each_set_port_word(i, word, offset, mask, ARRAY_SIZE(ports), 8) {
 		port_state = inb(dio48egpio->base + ports[i]);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= port_state << word_offset;
+		bits[word] |= port_state << offset;
 	}
 
 	return 0;

@@ -252,37 +229,25 @@ 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;
+	size_t i;
+	size_t word;
+	unsigned int offset;
+	unsigned int iomask;
 	unsigned int 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;
-		out_port = (port > 2) ? port + 1 : port;
-		bitmask = mask[BIT_WORD(i)] & bits[BIT_WORD(i)];
+	for_each_set_port_word(i, word, offset, mask, ARRAY_SIZE(ports), 8) {
+		iomask = mask[word] >> offset;
+		bitmask = iomask & (bits[word] >> offset);
 
 		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[i] &= ~iomask;
+		dio48egpio->out_state[i] |= bitmask;
+		outb(dio48egpio->out_state[i], dio48egpio->base + ports[i]);
 
 		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.17.0

William Breathitt Gray

author

2018-05-08 13:27 UTC #6 lore raw

PATCH v2 3/7 gpio: 104-idi-48: Utilize for_each_set_port_word macro

In reply to William Breathitt Gray

Replace boilerplate code in the get_multiple callback with the for_each_set_port_word macro in order to simplify and improve clarity.

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-104-idi-48.c | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

drivers/gpio/gpio-104-idi-48.c +4 ~~-28~~

index 2c9738adb3a6..414dc8de0bd8 100644
@@ -94,41 +94,17 @@ static int idi_48_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 {
 	struct idi_48_gpio *const idi48gpio = gpiochip_get_data(chip);
 	size_t i;
+	size_t word;
+	unsigned int offset;
 	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 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 */
+	for_each_set_port_word(i, word, offset, mask, ARRAY_SIZE(ports), 8) {
 		port_state = inb(idi48gpio->base + ports[i]);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= port_state << word_offset;
+		bits[word] |= port_state << offset;
 	}
 
 	return 0;

Signature · 1 line

2.17.0

William Breathitt Gray

author

2018-05-08 13:27 UTC #7 lore raw

PATCH v2 4/7 gpio: gpio-mm: Utilize for_each_set_port_word macro

In reply to William Breathitt Gray

Replace boilerplate code in get_multiple/set_multiple callbacks with for_each_set_port_word macro to simplify code and improve clarity.

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-gpio-mm.c | 67 +++++++++----------------------------
 1 file changed, 16 insertions(+), 51 deletions(-)

drivers/gpio/gpio-gpio-mm.c +16 ~~-51~~

index b56ff2efbf36..0ede33e7e251 100644
@@ -172,46 +172,23 @@ 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);
+	size_t word;
+	unsigned int offset;
 	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 */
+	for_each_set_port_word(i, word, offset, mask, ARRAY_SIZE(ports), 8) {
 		port_state = inb(gpiommgpio->base + ports[i]);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= port_state << word_offset;
+		bits[word] |= port_state << offset;
 	}
 
 	return 0;

@@ -242,37 +219,25 @@ 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;
+	size_t i;
+	size_t word;
+	unsigned int offset;
+	unsigned int iomask;
 	unsigned int 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;
-		out_port = (port > 2) ? port + 1 : port;
-		bitmask = mask[BIT_WORD(i)] & bits[BIT_WORD(i)];
+	for_each_set_port_word(i, word, offset, mask, ARRAY_SIZE(ports), 8) {
+		iomask = mask[word] >> offset;
+		bitmask = iomask & (bits[word] >> offset);
 
 		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[i] &= ~iomask;
+		gpiommgpio->out_state[i] |= bitmask;
+		outb(gpiommgpio->out_state[i], gpiommgpio->base + ports[i]);
 
 		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.17.0

William Breathitt Gray

author

2018-05-08 13:27 UTC #8 lore raw

PATCH v2 5/7 gpio: ws16c48: Utilize for_each_set_port_word macro

In reply to William Breathitt Gray

Replace boilerplate code in get_multiple/set_multiple callbacks with for_each_set_port_word macro to simplify code and improve clarity.

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-ws16c48.c | 66 +++++++++----------------------------
 1 file changed, 16 insertions(+), 50 deletions(-)

drivers/gpio/gpio-ws16c48.c +16 ~~-50~~

index c7028eb0b8e1..ab8b36f3595c 100644
@@ -134,42 +134,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);
+	size_t port;
+	size_t word;
+	unsigned int offset;
+	const unsigned int port_size = 8;
+	const size_t num_ports = chip->ngpio / port_size;
 	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;
-
-		/* 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;
+	for_each_set_port_word(port, word, offset, mask, num_ports, port_size) {
+		port_state = inb(ws16c48gpio->base + port);
+		bits[word] |= port_state << offset;
 	}
 
 	return 0;

@@ -203,26 +180,19 @@ 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;
+	size_t port;
+	size_t word;
+	unsigned int offset;
+	const unsigned int port_size = 8;
+	const size_t num_ports = chip->ngpio / port_size;
 	unsigned int iomask;
 	unsigned int 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_port_word(port, word, offset, mask, num_ports, port_size) {
 		/* mask out GPIO configured for input */
-		iomask = mask[BIT_WORD(i)] & ~ws16c48gpio->io_state[port];
-		bitmask = iomask & bits[BIT_WORD(i)];
+		iomask = (mask[word] >> offset) & ~ws16c48gpio->io_state[port];
+		bitmask = iomask & (bits[word] >> offset);
 
 		raw_spin_lock_irqsave(&ws16c48gpio->lock, flags);

@@ -232,10 +202,6 @@ static void ws16c48_gpio_set_multiple(struct gpio_chip *chip,
 		outb(ws16c48gpio->out_state[port], ws16c48gpio->base + port);
 
 		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.17.0

William Breathitt Gray

author

2018-05-08 13:28 UTC #9 lore raw

PATCH v2 6/7 gpio: pci-idio-16: Utilize for_each_set_port_word macro

In reply to William Breathitt Gray

Replace boilerplate code in get_multiple/set_multiple callbacks with for_each_set_port_word macro to simplify code and improve clarity.

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-pci-idio-16.c | 67 +++++++++++----------------------
 1 file changed, 21 insertions(+), 46 deletions(-)

drivers/gpio/gpio-pci-idio-16.c +21 ~~-46~~

index 25d16b2af1c3..f5c759d70081 100644
@@ -109,44 +109,20 @@ static int idio_16_gpio_get_multiple(struct gpio_chip *chip,
 {
 	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;
+	size_t word;
+	unsigned int offset;
 	void __iomem *ports[] = {
 		&idio16gpio->reg->out0_7, &idio16gpio->reg->out8_15,
 		&idio16gpio->reg->in0_7, &idio16gpio->reg->in8_15,
 	};
+	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 */
+	for_each_set_port_word(i, word, offset, mask, ARRAY_SIZE(ports), 8) {
 		port_state = ioread8(ports[i]);
-
-		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= port_state << word_offset;
+		bits[word] |= port_state << offset;
 	}
 
 	return 0;

@@ -186,30 +162,29 @@ 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);
+	size_t i;
+	size_t word;
+	unsigned int offset;
+	void __iomem *ports[] = {
+		&idio16gpio->reg->out0_7, &idio16gpio->reg->out8_15,
+	};
+	unsigned int iomask;
+	unsigned int bitmask;
 	unsigned long flags;
 	unsigned int out_state;
 
-	raw_spin_lock_irqsave(&idio16gpio->lock, flags);
+	for_each_set_port_word(i, word, offset, mask, ARRAY_SIZE(ports), 8) {
+		iomask = mask[word] >> offset;
+		bitmask = iomask & (bits[word] >> offset);
 
-	/* 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);
-	}
+		raw_spin_lock_irqsave(&idio16gpio->lock, flags);
 
-	/* shift to next output line word */
-	*mask >>= 8;
+		out_state = ioread8(ports[i]) & ~iomask;
+		out_state |= bitmask;
+		iowrite8(out_state, ports[i]);
 
-	/* 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.17.0

William Breathitt Gray

author

2018-05-08 13:28 UTC #10 lore raw

PATCH v2 7/7 gpio: pcie-idio-24: Utilize for_each_set_port_word macro

In reply to William Breathitt Gray

Replace boilerplate code in get_multiple/set_multiple callbacks with for_each_set_port_word macro to simplify code and improve clarity.

Signed-off-by William Breathitt Gray <[email protected]>

---
 drivers/gpio/gpio-pcie-idio-24.c | 102 +++++++++++--------------------
 1 file changed, 36 insertions(+), 66 deletions(-)

drivers/gpio/gpio-pcie-idio-24.c +36 ~~-66~~

index f953541e7890..a0c81724ce4d 100644
@@ -199,41 +199,21 @@ static int idio_24_gpio_get_multiple(struct gpio_chip *chip,
 {
 	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;
+	size_t word;
+	unsigned int offset;
 	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,
 	};
+	const size_t num_ports = ARRAY_SIZE(ports) + 1;
+	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_port_word(i, word, offset, mask, num_ports, 8) {
 		/* read bits from current gpio port (port 6 is TTL GPIO) */
 		if (i < 6)
 			port_state = ioread8(ports[i]);

@@ -243,7 +223,7 @@ static int idio_24_gpio_get_multiple(struct gpio_chip *chip,
 			port_state = ioread8(&idio24gpio->reg->ttl_in0_7);
 
 		/* store acquired bits at respective bits array offset */
-		bits[word_index] |= port_state << word_offset;
+		bits[word] |= port_state << offset;
 	}
 
 	return 0;

@@ -295,58 +275,48 @@ static void idio_24_gpio_set_multiple(struct gpio_chip *chip,
 {
 	struct idio_24_gpio *const idio24gpio = gpiochip_get_data(chip);
 	size_t i;
-	unsigned long bits_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;
+	size_t word;
+	unsigned int offset;
 	void __iomem *ports[] = {
 		&idio24gpio->reg->out0_7, &idio24gpio->reg->out8_15,
 		&idio24gpio->reg->out16_23
 	};
+	const size_t num_ports = ARRAY_SIZE(ports) + 1;
+	unsigned int iomask;
+	unsigned int bitmask;
+	unsigned long flags;
 	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 */
+	unsigned int out_state;
+
+	for_each_set_port_word(i, word, offset, mask, num_ports, 8) {
+		iomask = mask[word] >> offset;
+		bitmask = iomask & (bits[word] >> offset);
+
+		raw_spin_lock_irqsave(&idio24gpio->lock, flags);
+
+		/* read bits from current gpio port (port 6 is TTL GPIO) */
+		if (i < 6) {
+			out_state = ioread8(ports[i]) & ~iomask;
+		} 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;
 		}
 
-		raw_spin_lock_irqsave(&idio24gpio->lock, flags);
+		/* set requested bit states */
+		out_state &= ~iomask;
+		out_state |= bitmask;
 
-		/* process output lines */
-		out_state = ioread8(ports[i]) & ~gpio_mask;
-		out_state |= (*bits >> bits_offset) & gpio_mask;
-		iowrite8(out_state, ports[i]);
+		/* write bits for current gpio port (port 6 is TTL GPIO) */
+		if (i < 6)
+			iowrite8(out_state, ports[i]);
+		else
+			iowrite8(out_state, &idio24gpio->reg->ttl_out0_7);
 
 		raw_spin_unlock_irqrestore(&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;
-
-	/* handle TTL output */
-	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
-
-	/* 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);
-
-	raw_spin_unlock_irqrestore(&idio24gpio->lock, flags);
 }
 
 static void idio_24_irq_ack(struct irq_data *data)

Signature · 1 line

2.17.0

Andy Shevchenko

2018-05-13 15:11 UTC #11 lore raw

In reply to William Breathitt Gray

Quoted 9 lines · William Breathitt Gray

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_port_word macro and utilizes it in several GPIO drivers. The for_each_set_port_word macro facilitates a for-loop syntax that iterates over entire groups of set bits at a time.

Quoted 8 lines

For example, suppose you would like to iterate over a 16-bit integer 4 bits at a time, skipping over 4-bit groups with no set bit, where XXXX represents the current 4-bit group:
    Example:        1011 1110 0000 1111
    First loop:     1011 1110 0000 XXXX

    Second loop:    1011 XXXX 0000 1111

8-bit itteration. Is it correct?

Quoted 1 line

    Third loop:     XXXX 1110 0000 1111

Looking at the example above I highly recommend to introduce some test cases for your helper function.

Consider extending lib/test_bitmap.c for this purpose.

Quoted 51 lines

Each iteration of the loop returns the next 4-bit group that has at least one set bit.
The for_each_set_port_word macro has six parameters:
* port_word: set to current port word index for the iteration
* word_index: set to current bitmap word index for the iteration
* word_offset: bits offset of the found port word in the bitmap word
* bits: bitmap to search within
* size: bitmap size in number of port words
* port_size: port word size in number of bits
The port_size argument can be an arbitrary number of bits and is not required to be a multiple of 2.
I've called the group of bits a "port word" which may be a confusing naming convention; I was afraid calling that them a "group" may be too vague. Should a different name be chosen; what would you suggest?
This patchset was rebased on top of the following three commits:
* commit aaf96e51de11 ("gpio: pci-idio-16: Fix port memory offset for get_multiple callback")
* commit 304440aa96c6 ("gpio: pcie-idio-24: Fix port memory offset for get_multiple/set_multiple callbacks")
* commit e026646c178d ("gpio: pcie-idio-24: Fix off-by-one error in get_multiple loop")
William Breathitt Gray
William Breathitt Gray (7):
bitops: Introduce the for_each_set_port_word macro
gpio: 104-dio-48e: Utilize for_each_set_port_word macro
gpio: 104-idi-48: Utilize for_each_set_port_word macro
gpio: gpio-mm: Utilize for_each_set_port_word macro
gpio: ws16c48: Utilize for_each_set_port_word macro
gpio: pci-idio-16: Utilize for_each_set_port_word macro
gpio: pcie-idio-24: Utilize for_each_set_port_word macro
 drivers/gpio/gpio-104-dio-48e.c   |  67 +++++---------------
 drivers/gpio/gpio-104-idi-48.c    |  32 ++--------
 drivers/gpio/gpio-gpio-mm.c       |  67 +++++---------------
 drivers/gpio/gpio-pci-idio-16.c   |  67 ++++++--------------
 drivers/gpio/gpio-pcie-idio-24.c  | 102 +++++++++++-------------------
 drivers/gpio/gpio-ws16c48.c       |  66 +++++--------------
 include/asm-generic/bitops/find.h |  26 ++++++++
 include/linux/bitops.h            |   9 +++
 lib/find_bit.c                    |  28 ++++++++
 9 files changed, 172 insertions(+), 292 deletions(-)
--
2.17.0

Signature · 2 lines

With Best Regards,
Andy Shevchenko

William Breathitt Gray

author

2018-05-14 12:57 UTC #12 lore raw

Quoted 16 lines · Andy Shevchenko

Quoted 13 lines · William Breathitt Gray
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_port_word macro and utilizes it in several GPIO drivers. The for_each_set_port_word macro facilitates a for-loop syntax that iterates over entire groups of set bits at a time.
For example, suppose you would like to iterate over a 16-bit integer 4 bits at a time, skipping over 4-bit groups with no set bit, where XXXX represents the current 4-bit group:

Quoted 6 lines

Quoted 4 lines

    Example:        1011 1110 0000 1111
    First loop:     1011 1110 0000 XXXX

    Second loop:    1011 XXXX 0000 1111

8-bit itteration. Is it correct?

This case is 4-bit iteration for the sake of example, but the idea is to have the for_each_set_port_word macro to be capable of 8-bit iteration, as well as any other number of bits, as specified by the port_size parameter.

Quoted 7 lines

Quoted 2 lines
    Third loop:     XXXX 1110 0000 1111
Looking at the example above I highly recommend to introduce some test cases for your helper function.
Consider extending lib/test_bitmap.c for this purpose.

That's a good idea. I'll add in some test cases to lib/test_bitmap.c to help check the implementation of these functions and macro.

William Breathitt Gray

Quoted 58 lines

Quoted 52 lines
Each iteration of the loop returns the next 4-bit group that has at least one set bit.
The for_each_set_port_word macro has six parameters:
* port_word: set to current port word index for the iteration
* word_index: set to current bitmap word index for the iteration
* word_offset: bits offset of the found port word in the bitmap word
* bits: bitmap to search within
* size: bitmap size in number of port words
* port_size: port word size in number of bits
The port_size argument can be an arbitrary number of bits and is not required to be a multiple of 2.
I've called the group of bits a "port word" which may be a confusing naming convention; I was afraid calling that them a "group" may be too vague. Should a different name be chosen; what would you suggest?
This patchset was rebased on top of the following three commits:
* commit aaf96e51de11 ("gpio: pci-idio-16: Fix port memory offset for get_multiple callback")
* commit 304440aa96c6 ("gpio: pcie-idio-24: Fix port memory offset for get_multiple/set_multiple callbacks")
* commit e026646c178d ("gpio: pcie-idio-24: Fix off-by-one error in get_multiple loop")
William Breathitt Gray
William Breathitt Gray (7):
bitops: Introduce the for_each_set_port_word macro
gpio: 104-dio-48e: Utilize for_each_set_port_word macro
gpio: 104-idi-48: Utilize for_each_set_port_word macro
gpio: gpio-mm: Utilize for_each_set_port_word macro
gpio: ws16c48: Utilize for_each_set_port_word macro
gpio: pci-idio-16: Utilize for_each_set_port_word macro
gpio: pcie-idio-24: Utilize for_each_set_port_word macro
 drivers/gpio/gpio-104-dio-48e.c   |  67 +++++---------------
 drivers/gpio/gpio-104-idi-48.c    |  32 ++--------
 drivers/gpio/gpio-gpio-mm.c       |  67 +++++---------------
 drivers/gpio/gpio-pci-idio-16.c   |  67 ++++++--------------
 drivers/gpio/gpio-pcie-idio-24.c  | 102 +++++++++++-------------------
 drivers/gpio/gpio-ws16c48.c       |  66 +++++--------------
 include/asm-generic/bitops/find.h |  26 ++++++++
 include/linux/bitops.h            |   9 +++
 lib/find_bit.c                    |  28 ++++++++
 9 files changed, 172 insertions(+), 292 deletions(-)
--
2.17.0
Signature · 2 lines
With Best Regards,
Andy Shevchenko