| Message ID | 20230915150327.81918-5-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | gpiolib: work towards removing gpiochip_find() | expand |
On Fri, Sep 15, 2023 at 05:03:19PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > By far the most common way of looking up GPIO devices is using their > label. Provide a helpers for that to avoid every user implementing their > own matching function. ... > +static int gpio_chip_match_by_label(struct gpio_chip *gc, void *label) > +{ > + return gc->label && !strcmp(gc->label, label); > +} I am still wondering if we can oblige providers to have label to be non-empty.
On Mon, Sep 18, 2023 at 9:19 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 15, 2023 at 05:03:19PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > By far the most common way of looking up GPIO devices is using their > > label. Provide a helpers for that to avoid every user implementing their > > own matching function. > > ... > > > +static int gpio_chip_match_by_label(struct gpio_chip *gc, void *label) > > +{ > > + return gc->label && !strcmp(gc->label, label); > > +} > > I am still wondering if we can oblige providers to have label to be non-empty. > Of course we can. Just bail out of gpiochip_add_data_with_key() if it is. But that's material for a different patch. Bart
On Wed, Sep 27, 2023 at 01:22:36PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 18, 2023 at 9:19 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Sep 15, 2023 at 05:03:19PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > By far the most common way of looking up GPIO devices is using their > > > label. Provide a helpers for that to avoid every user implementing their > > > own matching function. ... > > > +static int gpio_chip_match_by_label(struct gpio_chip *gc, void *label) > > > +{ > > > + return gc->label && !strcmp(gc->label, label); > > > +} > > > > I am still wondering if we can oblige providers to have label to be non-empty. > > Of course we can. Just bail out of gpiochip_add_data_with_key() if it > is. But that's material for a different patch. Yes, but my point here is that 1) the current users are already following this requirement; 2) the enforcement can be done explicitly somewhere (in the register function). Is the 1) incorrect assumption?
On Wed, Sep 27, 2023 at 2:33 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Sep 27, 2023 at 01:22:36PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 18, 2023 at 9:19 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Fri, Sep 15, 2023 at 05:03:19PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > By far the most common way of looking up GPIO devices is using their > > > > label. Provide a helpers for that to avoid every user implementing their > > > > own matching function. > > ... > > > > > +static int gpio_chip_match_by_label(struct gpio_chip *gc, void *label) > > > > +{ > > > > + return gc->label && !strcmp(gc->label, label); > > > > +} > > > > > > I am still wondering if we can oblige providers to have label to be non-empty. > > > > Of course we can. Just bail out of gpiochip_add_data_with_key() if it > > is. But that's material for a different patch. > > Yes, but my point here is that > 1) the current users are already following this requirement; > 2) the enforcement can be done explicitly somewhere (in the register function). > > Is the 1) incorrect assumption? > I remember doing a quick glance over GPIO providers and it looks like ALL of them set the label. But I may have missed something. I would start with a warning. Bart
On Wed, Sep 27, 2023 at 02:42:28PM +0200, Bartosz Golaszewski wrote: > On Wed, Sep 27, 2023 at 2:33 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Sep 27, 2023 at 01:22:36PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 18, 2023 at 9:19 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Sep 15, 2023 at 05:03:19PM +0200, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> ... > > > > > +static int gpio_chip_match_by_label(struct gpio_chip *gc, void *label) > > > > > +{ > > > > > + return gc->label && !strcmp(gc->label, label); > > > > > +} > > > > > > > > I am still wondering if we can oblige providers to have label to be non-empty. > > > > > > Of course we can. Just bail out of gpiochip_add_data_with_key() if it > > > is. But that's material for a different patch. > > > > Yes, but my point here is that > > 1) the current users are already following this requirement; > > 2) the enforcement can be done explicitly somewhere (in the register function). > > > > Is the 1) incorrect assumption? > > I remember doing a quick glance over GPIO providers and it looks like > ALL of them set the label. But I may have missed something. I would > start with a warning. For now I would drop the NULL check. We will have a few weeks to see if somebody screams about. Meanwhile we can add the real error message patch if no-one complains.
On Wed, Sep 27, 2023 at 3:48 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Sep 27, 2023 at 02:42:28PM +0200, Bartosz Golaszewski wrote: > > On Wed, Sep 27, 2023 at 2:33 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, Sep 27, 2023 at 01:22:36PM +0200, Bartosz Golaszewski wrote: > > > > On Mon, Sep 18, 2023 at 9:19 AM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Fri, Sep 15, 2023 at 05:03:19PM +0200, Bartosz Golaszewski wrote: > > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > ... > > > > > > > +static int gpio_chip_match_by_label(struct gpio_chip *gc, void *label) > > > > > > +{ > > > > > > + return gc->label && !strcmp(gc->label, label); > > > > > > +} > > > > > > > > > > I am still wondering if we can oblige providers to have label to be non-empty. > > > > > > > > Of course we can. Just bail out of gpiochip_add_data_with_key() if it > > > > is. But that's material for a different patch. > > > > > > Yes, but my point here is that > > > 1) the current users are already following this requirement; > > > 2) the enforcement can be done explicitly somewhere (in the register function). > > > > > > Is the 1) incorrect assumption? > > > > I remember doing a quick glance over GPIO providers and it looks like > > ALL of them set the label. But I may have missed something. I would > > start with a warning. > > For now I would drop the NULL check. We will have a few weeks to see > if somebody screams about. Meanwhile we can add the real error message > patch if no-one complains. No, I'm not going to potentially break stuff like that as a way to detect bugs. That's not a hot path, we're not gaining much. Let's add a warning first, wait for some time, make it an error and then remove the check. Bart
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 0371d23f0a46..9f20311e4c1a 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -20,6 +20,7 @@ #include <linux/seq_file.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/string.h> #include <linux/gpio.h> #include <linux/gpio/driver.h> @@ -1081,6 +1082,26 @@ struct gpio_device *gpio_device_find(void *data, } EXPORT_SYMBOL_GPL(gpio_device_find); +static int gpio_chip_match_by_label(struct gpio_chip *gc, void *label) +{ + return gc->label && !strcmp(gc->label, label); +} + +/** + * gpio_device_find_by_label() - wrapper around gpio_device_find() finding the + * GPIO device by its backing chip's label + * @label: Label to lookup + * + * Returns: + * Reference to the GPIO device or NULL. Reference must be released with + * gpio_device_put(). + */ +struct gpio_device *gpio_device_find_by_label(const char *label) +{ + return gpio_device_find((void *)label, gpio_chip_match_by_label); +} +EXPORT_SYMBOL_GPL(gpio_device_find_by_label); + static int gpiochip_match_name(struct gpio_chip *gc, void *data) { const char *name = data; diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 6ad1f1a8ef2e..24996cba6465 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -610,6 +610,7 @@ struct gpio_chip *gpiochip_find(void *data, struct gpio_device *gpio_device_find(void *data, int (*match)(struct gpio_chip *gc, void *data)); +struct gpio_device *gpio_device_find_by_label(const char *label); struct gpio_device *gpio_device_get(struct gpio_device *gdev); void gpio_device_put(struct gpio_device *gdev);