[PATCH v2 0/3] gpiolib: refactor for_each_hwgpio()

[PATCH v2 0/3] gpiolib: refactor for_each_hwgpio()

[Andy Shevchenko]

Some of the for_each_*() macros use the very similar piece of code
that may be deduplicated (by introducing a new helper macro in patch 1).
On top of that the implementation of the low-level for_each_hwgpio_in_range()
looks too verbose. Try to simplify it (patches 2 and 3).

Changelog v2:
- added kernel-doc for a new helper (Bart)
- added two more patches

Andy Shevchenko (3):
  gpiolib: Deduplicate some code in for_each_requested_gpio_in_range()
  gpiolib: Simplify implementation of for_each_hwgpio_in_range()
  gpiolib: Switch to use for_each_if() helper

 include/linux/gpio/driver.h | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/3] gpiolib: Deduplicate some code in for_each_requested_gpio_in_range()

[Andy Shevchenko]

Refactor for_each_requested_gpio_in_range() to deduplicate some code
which is basically repeats the for_each_hwgpio(). In order to achieve
this, split the latter to two, for_each_hwgpio_in_range() and
for_each_hwgpio().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/gpio/driver.h | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a307d8ed9c51..024112ef5a78 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -548,6 +548,23 @@ DEFINE_CLASS(_gpiochip_for_each_data,
 	     }),
 	     const char **label, int *i)
 
+/**
+ * for_each_hwgpio_in_range - Iterates over all GPIOs in a given range
+ * @_chip: Chip to iterate over.
+ * @_i: Loop counter.
+ * @_base: First GPIO in the ranger.
+ * @_size: Amount of GPIOs to check starting from @base.
+ * @_label: Place to store the address of the label if the GPIO is requested.
+ *          Set to NULL for unused GPIOs.
+ */
+#define for_each_hwgpio_in_range(_chip, _i, _base, _size, _label)			\
+	for (CLASS(_gpiochip_for_each_data, _data)(&_label, &_i);			\
+	     *_data.i < _size;								\
+	     (*_data.i)++, kfree(*(_data.label)), *_data.label = NULL)			\
+		if (IS_ERR(*_data.label =						\
+			gpiochip_dup_line_label(_chip, _base + *_data.i))) {}		\
+		else
+
 /**
  * for_each_hwgpio - Iterates over all GPIOs for given chip.
  * @_chip: Chip to iterate over.
@@ -555,13 +572,8 @@ DEFINE_CLASS(_gpiochip_for_each_data,
  * @_label: Place to store the address of the label if the GPIO is requested.
  *          Set to NULL for unused GPIOs.
  */
-#define for_each_hwgpio(_chip, _i, _label) \
-	for (CLASS(_gpiochip_for_each_data, _data)(&_label, &_i); \
-	     *_data.i < _chip->ngpio; \
-	     (*_data.i)++, kfree(*(_data.label)), *_data.label = NULL) \
-		if (IS_ERR(*_data.label = \
-			gpiochip_dup_line_label(_chip, *_data.i))) {} \
-		else
+#define for_each_hwgpio(_chip, _i, _label)						\
+	for_each_hwgpio_in_range(_chip, _i, 0, _chip->ngpio, _label)
 
 /**
  * for_each_requested_gpio_in_range - iterates over requested GPIOs in a given range
@@ -572,13 +584,8 @@ DEFINE_CLASS(_gpiochip_for_each_data,
  * @_label:	label of current GPIO
  */
 #define for_each_requested_gpio_in_range(_chip, _i, _base, _size, _label)		\
-	for (CLASS(_gpiochip_for_each_data, _data)(&_label, &_i);			\
-	     *_data.i < _size;								\
-	     (*_data.i)++, kfree(*(_data.label)), *_data.label = NULL)			\
-		if ((*_data.label =							\
-			gpiochip_dup_line_label(_chip, _base + *_data.i)) == NULL) {}	\
-		else if (IS_ERR(*_data.label)) {}					\
-		else
+	for_each_hwgpio_in_range(_chip, _i, _base, _size, _label)			\
+		if (_label == NULL) {} else
 
 /* Iterates over all requested GPIO of the given @chip */
 #define for_each_requested_gpio(chip, i, label)						\
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 2/3] gpiolib: Simplify implementation of for_each_hwgpio_in_range()

[Andy Shevchenko]

The whole purpose of the custom CLASS() is to have possibility
to initialise the counter variable _i to 0. This can't be done
with simple __free() macro as it will be not allowed by C language.
OTOH, the CLASS() operates with the pointers and explicit usage of
the scoped variable _data is not needed, since the pointers are kept
the same over the iterations. Simplify the implementation of
for_each_hwgpio_in_range().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/gpio/driver.h | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 024112ef5a78..ae93f75170f2 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -559,11 +559,9 @@ DEFINE_CLASS(_gpiochip_for_each_data,
  */
 #define for_each_hwgpio_in_range(_chip, _i, _base, _size, _label)			\
 	for (CLASS(_gpiochip_for_each_data, _data)(&_label, &_i);			\
-	     *_data.i < _size;								\
-	     (*_data.i)++, kfree(*(_data.label)), *_data.label = NULL)			\
-		if (IS_ERR(*_data.label =						\
-			gpiochip_dup_line_label(_chip, _base + *_data.i))) {}		\
-		else
+	     _i < _size;								\
+	     _i++, kfree(_label), _label = NULL)					\
+		if (IS_ERR(_label = gpiochip_dup_line_label(_chip, _base + _i))) {} else
 
 /**
  * for_each_hwgpio - Iterates over all GPIOs for given chip.
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 3/3] gpiolib: Switch to use for_each_if() helper

[Andy Shevchenko]

The for_each_*() APIs that are conditional can be written shorter and
less error prone with for_each_if() helper in use. Switch them to use
this helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/gpio/driver.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index ae93f75170f2..a7e9c1d3df71 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -532,6 +532,7 @@ struct gpio_chip {
 
 char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
 
+#define for_each_if(condition) if (!(condition)) {} else
 
 struct _gpiochip_for_each_data {
 	const char **label;
@@ -561,7 +562,7 @@ DEFINE_CLASS(_gpiochip_for_each_data,
 	for (CLASS(_gpiochip_for_each_data, _data)(&_label, &_i);			\
 	     _i < _size;								\
 	     _i++, kfree(_label), _label = NULL)					\
-		if (IS_ERR(_label = gpiochip_dup_line_label(_chip, _base + _i))) {} else
+		for_each_if(!IS_ERR(_label = gpiochip_dup_line_label(_chip, _base + _i)))
 
 /**
  * for_each_hwgpio - Iterates over all GPIOs for given chip.
@@ -583,7 +584,7 @@ DEFINE_CLASS(_gpiochip_for_each_data,
  */
 #define for_each_requested_gpio_in_range(_chip, _i, _base, _size, _label)		\
 	for_each_hwgpio_in_range(_chip, _i, _base, _size, _label)			\
-		if (_label == NULL) {} else
+		for_each_if(_label)
 
 /* Iterates over all requested GPIO of the given @chip */
 #define for_each_requested_gpio(chip, i, label)						\
@@ -869,7 +870,7 @@ static inline void gpiochip_unlock_as_irq(struct gpio_chip *gc,
 
 #define for_each_gpiochip_node(dev, child)					\
 	device_for_each_child_node(dev, child)					\
-		if (!fwnode_property_present(child, "gpio-controller")) {} else
+		for_each_if(fwnode_property_present(child, "gpio-controller"))
 
 static inline unsigned int gpiochip_node_count(struct device *dev)
 {
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 3/3] gpiolib: Switch to use for_each_if() helper

[Bartosz Golaszewski]

On Fri, Feb 7, 2025 at 4:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> The for_each_*() APIs that are conditional can be written shorter and
> less error prone with for_each_if() helper in use. Switch them to use
> this helper.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/gpio/driver.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index ae93f75170f2..a7e9c1d3df71 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -532,6 +532,7 @@ struct gpio_chip {
>
>  char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset);
>
> +#define for_each_if(condition) if (!(condition)) {} else

This is already defined in drm/drm_util.h. Please move it out of there
and make it available globally instead of duplicating.

Bart

Re: (subset) [PATCH v2 0/3] gpiolib: refactor for_each_hwgpio()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Fri, 07 Feb 2025 17:07:33 +0200, Andy Shevchenko wrote:
> Some of the for_each_*() macros use the very similar piece of code
> that may be deduplicated (by introducing a new helper macro in patch 1).
> On top of that the implementation of the low-level for_each_hwgpio_in_range()
> looks too verbose. Try to simplify it (patches 2 and 3).
> 
> Changelog v2:
> - added kernel-doc for a new helper (Bart)
> - added two more patches
> 
> [...]

I applied patches 1 and 2. Please revisit 3/3 in order to not duplicate
the for_each_if() macro.

[1/3] gpiolib: Deduplicate some code in for_each_requested_gpio_in_range()
      commit: 8893516000b247f91fa2cef34f2a77b609e661a4
[2/3] gpiolib: Simplify implementation of for_each_hwgpio_in_range()
      commit: 767412f092fc6e04147305acd70f15770ece47ec

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>