| Message ID | 20210518083339.23416-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] gpiolib: Introduce for_each_gpio_desc_if() macro | expand |
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
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
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
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
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
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
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
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
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,
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(-)