[PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Andy Shevchenko]

In a few places we are using a loop against all GPIO descriptors
with a given flag for a given device. Replace it with a consolidated
for_each type of macro.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: fixed compilation issue (LKP), injected if (test_bit) into the loop
 drivers/gpio/gpiolib-of.c    | 10 ++++------
 drivers/gpio/gpiolib-sysfs.c |  7 ++-----
 drivers/gpio/gpiolib.c       |  7 +++----
 drivers/gpio/gpiolib.h       |  7 +++++++
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index bbcc7c073f63..2f8f3f0c8373 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -711,14 +711,12 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
 static void of_gpiochip_remove_hog(struct gpio_chip *chip,
 				   struct device_node *hog)
 {
-	struct gpio_desc *descs = chip->gpiodev->descs;
+	struct gpio_desc *desc;
 	unsigned int i;
 
-	for (i = 0; i < chip->ngpio; i++) {
-		if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
-		    descs[i].hog == hog)
-			gpiochip_free_own_desc(&descs[i]);
-	}
+	for_each_gpio_desc_if(i, chip, desc, FLAG_IS_HOGGED)
+		if (desc->hog == hog)
+			gpiochip_free_own_desc(desc);
 }
 
 static int of_gpiochip_match_node(struct gpio_chip *chip, void *data)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index ae49bb23c6ed..41b3b782bf3f 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -801,11 +801,8 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
 	mutex_unlock(&sysfs_lock);
 
 	/* unregister gpiod class devices owned by sysfs */
-	for (i = 0; i < chip->ngpio; i++) {
-		desc = &gdev->descs[i];
-		if (test_and_clear_bit(FLAG_SYSFS, &desc->flags))
-			gpiod_free(desc);
-	}
+	for_each_gpio_desc_if(i, chip, desc, FLAG_SYSFS)
+		gpiod_free(desc);
 }
 
 static int __init gpiolib_sysfs_init(void)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 220a9d8dd4e3..97a69362a584 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4012,12 +4012,11 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
  */
 static void gpiochip_free_hogs(struct gpio_chip *gc)
 {
+	struct gpio_desc *desc;
 	int id;
 
-	for (id = 0; id < gc->ngpio; id++) {
-		if (test_bit(FLAG_IS_HOGGED, &gc->gpiodev->descs[id].flags))
-			gpiochip_free_own_desc(&gc->gpiodev->descs[id]);
-	}
+	for_each_gpio_desc_if(id, gc, desc, FLAG_IS_HOGGED)
+		gpiochip_free_own_desc(desc);
 }
 
 /**
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 30bc3f80f83e..69c96a4276de 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -82,6 +82,13 @@ struct gpio_array {
 };
 
 struct gpio_desc *gpiochip_get_desc(struct gpio_chip *gc, unsigned int hwnum);
+
+#define for_each_gpio_desc_if(i, gc, desc, flag)		\
+	for (i = 0, desc = gpiochip_get_desc(gc, i);		\
+	     i < gc->ngpio;					\
+	     i++, desc = gpiochip_get_desc(gc, i))		\
+		if (!test_bit(flag, &desc->flags)) {} else
+
 int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 				  unsigned int array_size,
 				  struct gpio_desc **desc_array,
-- 
2.30.2

Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Linus Walleij]

On Tue, May 18, 2021 at 10:33 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> In a few places we are using a loop against all GPIO descriptors
> with a given flag for a given device. Replace it with a consolidated
> for_each type of macro.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

This is great for readability.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Johan Hovold]

On Tue, May 18, 2021 at 11:33:39AM +0300, Andy Shevchenko wrote:
> In a few places we are using a loop against all GPIO descriptors
> with a given flag for a given device. Replace it with a consolidated
> for_each type of macro.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: fixed compilation issue (LKP), injected if (test_bit) into the loop
>  drivers/gpio/gpiolib-of.c    | 10 ++++------
>  drivers/gpio/gpiolib-sysfs.c |  7 ++-----
>  drivers/gpio/gpiolib.c       |  7 +++----
>  drivers/gpio/gpiolib.h       |  7 +++++++
>  4 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index bbcc7c073f63..2f8f3f0c8373 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -711,14 +711,12 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  static void of_gpiochip_remove_hog(struct gpio_chip *chip,
>  				   struct device_node *hog)
>  {
> -	struct gpio_desc *descs = chip->gpiodev->descs;
> +	struct gpio_desc *desc;
>  	unsigned int i;
>  
> -	for (i = 0; i < chip->ngpio; i++) {
> -		if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
> -		    descs[i].hog == hog)
> -			gpiochip_free_own_desc(&descs[i]);
> -	}
> +	for_each_gpio_desc_if(i, chip, desc, FLAG_IS_HOGGED)
> +		if (desc->hog == hog)
> +			gpiochip_free_own_desc(desc);

The _if suffix here is too vague.

Please use a more descriptive name so that you don't need to look at the
implementation to understand what the macro does.

Perhaps call it 

	for_each_gpio_desc_with_flag()

or just add the more generic macro 

	for_each_gpio_desc()

and open-code the test so that it's clear what's going on here.

>  struct gpio_desc *gpiochip_get_desc(struct gpio_chip *gc, unsigned int hwnum);
> +
> +#define for_each_gpio_desc_if(i, gc, desc, flag)		\
> +	for (i = 0, desc = gpiochip_get_desc(gc, i);		\
> +	     i < gc->ngpio;					\
> +	     i++, desc = gpiochip_get_desc(gc, i))		\
> +		if (!test_bit(flag, &desc->flags)) {} else
> +
>  int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>  				  unsigned int array_size,
>  				  struct gpio_desc **desc_array,

Johan

Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Andy Shevchenko]

On Thu, May 20, 2021 at 11:07 AM Johan Hovold <johan@kernel.org> wrote:
> On Tue, May 18, 2021 at 11:33:39AM +0300, Andy Shevchenko wrote:

Thank you for the response, my answer below.

...

> The _if suffix here is too vague.
>
> Please use a more descriptive name so that you don't need to look at the
> implementation to understand what the macro does.
>
> Perhaps call it
>
>         for_each_gpio_desc_with_flag()

Haha, I have the same in my internal tree, but then I have changed to
_if and here is why:
- the API is solely for internal use (note, internals of struct
gpio_desc available for the same set of users)
- the current users do only same pattern
- I don't expect that we will have this to be anything else in the future

Thus, _if is a good balance between scope of use and naming.

I prefer to leave it as is.

> or just add the more generic macro
>
>         for_each_gpio_desc()
>
> and open-code the test so that it's clear what's going on here.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Johan Hovold]

On Thu, May 20, 2021 at 11:15:31AM +0300, Andy Shevchenko wrote:
> On Thu, May 20, 2021 at 11:07 AM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, May 18, 2021 at 11:33:39AM +0300, Andy Shevchenko wrote:

> > The _if suffix here is too vague.
> >
> > Please use a more descriptive name so that you don't need to look at the
> > implementation to understand what the macro does.
> >
> > Perhaps call it
> >
> >         for_each_gpio_desc_with_flag()
> 
> Haha, I have the same in my internal tree, but then I have changed to
> _if and here is why:
> - the API is solely for internal use (note, internals of struct
> gpio_desc available for the same set of users)

That's not a valid argument here. You should never make code harder to
read.

There are other ways of marking functions as intended for internal use
(e.g. do not export them and add a _ prefix or whatever).

> - the current users do only same pattern

That's not an argument against using a descriptive name. Possibly
against adding a generic for_each_gpio_desc() macro.

> - I don't expect that we will have this to be anything else in the future

Again, irrelevant. Possibly an argument against adding another helper in
the first place.

> Thus, _if is a good balance between scope of use and naming.

No, no, no. It's never a good idea to obfuscate code.

> I prefer to leave it as is.

I hope you'll reconsider, or that my arguments can convince the
maintainers to step in here.

> > or just add the more generic macro
> >
> >         for_each_gpio_desc()
> >
> > and open-code the test so that it's clear what's going on here.

FWIW, NAK due to the non-descriptive for_each_desc_if() name.

Johan

Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Andy Shevchenko]

On Thu, May 20, 2021 at 10:33:38AM +0200, Johan Hovold wrote:
> On Thu, May 20, 2021 at 11:15:31AM +0300, Andy Shevchenko wrote:

> FWIW, NAK due to the non-descriptive for_each_desc_if() name.

I'm fine without this change, thanks for review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Andy Shevchenko]

On Thu, May 20, 2021 at 12:03:42PM +0300, Andy Shevchenko wrote:
> On Thu, May 20, 2021 at 10:33:38AM +0200, Johan Hovold wrote:
> > On Thu, May 20, 2021 at 11:15:31AM +0300, Andy Shevchenko wrote:
> 
> > FWIW, NAK due to the non-descriptive for_each_desc_if() name.
> 
> I'm fine without this change, thanks for review!

That said, maybe in the future I will reconsider.

And feel free to amend by yourself, you can keep or drop my SoB,
I'm fine either way.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Andy Shevchenko]

On Thu, May 20, 2021 at 10:33:38AM +0200, Johan Hovold wrote:
> On Thu, May 20, 2021 at 11:15:31AM +0300, Andy Shevchenko wrote:
> > On Thu, May 20, 2021 at 11:07 AM Johan Hovold <johan@kernel.org> wrote:
> > > On Tue, May 18, 2021 at 11:33:39AM +0300, Andy Shevchenko wrote:
> 
> > > The _if suffix here is too vague.
> > >
> > > Please use a more descriptive name so that you don't need to look at the
> > > implementation to understand what the macro does.
> > >
> > > Perhaps call it
> > >
> > >         for_each_gpio_desc_with_flag()
> > 
> > Haha, I have the same in my internal tree, but then I have changed to
> > _if and here is why:
> > - the API is solely for internal use (note, internals of struct
> > gpio_desc available for the same set of users)
> 
> That's not a valid argument here. You should never make code harder to
> read.
> 
> There are other ways of marking functions as intended for internal use
> (e.g. do not export them and add a _ prefix or whatever).
> 
> > - the current users do only same pattern
> 
> That's not an argument against using a descriptive name. Possibly
> against adding a generic for_each_gpio_desc() macro.
> 
> > - I don't expect that we will have this to be anything else in the future
> 
> Again, irrelevant. Possibly an argument against adding another helper in
> the first place.
> 
> > Thus, _if is a good balance between scope of use and naming.
> 
> No, no, no. It's never a good idea to obfuscate code.
> 
> > I prefer to leave it as is.
> 
> I hope you'll reconsider, or that my arguments can convince the
> maintainers to step in here.
> 
> > > or just add the more generic macro
> > >
> > >         for_each_gpio_desc()
> > >
> > > and open-code the test so that it's clear what's going on here.
> 
> FWIW, NAK due to the non-descriptive for_each_desc_if() name.

Btw, missed argument

..._with_flag(..., FLAG_...)

breaks the DRY principle. If you read current code it's clear with that

_if(..., FLAG_...)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

[Johan Hovold]

On Thu, May 20, 2021 at 12:16:20PM +0300, Andy Shevchenko wrote:
> On Thu, May 20, 2021 at 10:33:38AM +0200, Johan Hovold wrote:
> > On Thu, May 20, 2021 at 11:15:31AM +0300, Andy Shevchenko wrote:
> > > On Thu, May 20, 2021 at 11:07 AM Johan Hovold <johan@kernel.org> wrote:
> > > > On Tue, May 18, 2021 at 11:33:39AM +0300, Andy Shevchenko wrote:
> > 
> > > > The _if suffix here is too vague.
> > > >
> > > > Please use a more descriptive name so that you don't need to look at the
> > > > implementation to understand what the macro does.
> > > >
> > > > Perhaps call it
> > > >
> > > >         for_each_gpio_desc_with_flag()

> > > > or just add the more generic macro
> > > >
> > > >         for_each_gpio_desc()
> > > >
> > > > and open-code the test so that it's clear what's going on here.
> > 
> > FWIW, NAK due to the non-descriptive for_each_desc_if() name.
> 
> Btw, missed argument
> 
> ..._with_flag(..., FLAG_...)
> 
> breaks the DRY principle. If you read current code it's clear with that
> 
> _if(..., FLAG_...)

That we have precisely zero for_each_ macros with an _if suffix should
also give you a hint that this is not a good idea.

Again, you shouldn't have to look at the implementation to understand
what a helper does.

Johan