| Message ID | 20230915150327.81918-8-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:22PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Remove all remaining uses of find_chip_by_name() (and subsequently: > gpiochip_find()) from gpiolib.c and use the new > gpio_device_find_by_label() instead. ... > -static int gpiochip_match_name(struct gpio_chip *gc, void *data) > -{ > - const char *name = data; > - > - return !strcmp(gc->label, name); And this we had no check for the label being NULL... ... > for (p = &table->table[0]; p->key; p++) { > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > + gdev = gpio_device_find_by_label(p->key); > + if (!gdev) { I haven't got the fix for gpio-sim, shouldn't we have the same here, i.e. definition being done together with the assignment when __free() is in use? > }
On Mon, Sep 18, 2023 at 9:23 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 15, 2023 at 05:03:22PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Remove all remaining uses of find_chip_by_name() (and subsequently: > > gpiochip_find()) from gpiolib.c and use the new > > gpio_device_find_by_label() instead. > > ... > > > -static int gpiochip_match_name(struct gpio_chip *gc, void *data) > > -{ > > - const char *name = data; > > - > > - return !strcmp(gc->label, name); > > And this we had no check for the label being NULL... > Yeah, it was wrong. But maybe all kernel users already do assign it in which case we should be safe just adding a check in gpiochip_add_data_with_key() that would return EINVAL if they don't. > ... > > > for (p = &table->table[0]; p->key; p++) { > > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > > > + gdev = gpio_device_find_by_label(p->key); > > + if (!gdev) { > > I haven't got the fix for gpio-sim, shouldn't we have the same here, i.e. > definition being done together with the assignment when __free() is in use? > It should but I only got yelled at by Linus under the gpio-sim patch after I sent this one. Bart > > } > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Sep 18, 2023 at 10:03 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Mon, Sep 18, 2023 at 9:23 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > for (p = &table->table[0]; p->key; p++) { > > > + struct gpio_device *gdev __free(gpio_device_put) = NULL; > > > > > + gdev = gpio_device_find_by_label(p->key); > > > + if (!gdev) { > > > > I haven't got the fix for gpio-sim, shouldn't we have the same here, i.e. > > definition being done together with the assignment when __free() is in use? > > It should but I only got yelled at by Linus under the gpio-sim patch > after I sent this one. That happens, it's all new. I guess ideally we should patch checkpatch to just moan about this, I wonder how hard that could be (I've only patched it once in my life...) Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e26cbd10a246..4c734bfe6d32 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1145,18 +1145,6 @@ struct gpio_device *gpio_device_find_by_label(const char *label) } EXPORT_SYMBOL_GPL(gpio_device_find_by_label); -static int gpiochip_match_name(struct gpio_chip *gc, void *data) -{ - const char *name = data; - - return !strcmp(gc->label, name); -} - -static struct gpio_chip *find_chip_by_name(const char *name) -{ - return gpiochip_find((void *)name, gpiochip_match_name); -} - /** * gpio_device_get() - Increase the reference count of this GPIO device * @gdev: GPIO device to increase the refcount for @@ -3908,21 +3896,22 @@ EXPORT_SYMBOL_GPL(gpiod_remove_lookup_table); */ void gpiod_add_hogs(struct gpiod_hog *hogs) { - struct gpio_chip *gc; struct gpiod_hog *hog; mutex_lock(&gpio_machine_hogs_mutex); for (hog = &hogs[0]; hog->chip_label; hog++) { + struct gpio_device *gdev __free(gpio_device_put) = NULL; + list_add_tail(&hog->list, &gpio_machine_hogs); /* * The chip may have been registered earlier, so check if it * exists and, if so, try to hog the line now. */ - gc = find_chip_by_name(hog->chip_label); - if (gc) - gpiochip_machine_hog(gc, hog); + gdev = gpio_device_find_by_label(hog->chip_label); + if (gdev) + gpiochip_machine_hog(gpio_device_get_chip(gdev), hog); } mutex_unlock(&gpio_machine_hogs_mutex); @@ -3977,13 +3966,14 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, struct gpio_desc *desc = ERR_PTR(-ENOENT); struct gpiod_lookup_table *table; struct gpiod_lookup *p; + struct gpio_chip *gc; table = gpiod_find_lookup_table(dev); if (!table) return desc; for (p = &table->table[0]; p->key; p++) { - struct gpio_chip *gc; + struct gpio_device *gdev __free(gpio_device_put) = NULL; /* idx must always match exactly */ if (p->idx != idx) @@ -4005,9 +3995,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, return ERR_PTR(-EPROBE_DEFER); } - gc = find_chip_by_name(p->key); - - if (!gc) { + gdev = gpio_device_find_by_label(p->key); + if (!gdev) { /* * As the lookup table indicates a chip with * p->key should exist, assume it may @@ -4020,6 +4009,8 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, return ERR_PTR(-EPROBE_DEFER); } + gc = gpio_device_get_chip(gdev); + if (gc->ngpio <= p->chip_hwnum) { dev_err(dev, "requested GPIO %u (%u) is out of range [0..%u] for chip %s\n", @@ -4028,7 +4019,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id, return ERR_PTR(-EINVAL); } - desc = gpiochip_get_desc(gc, p->chip_hwnum); + desc = gpio_device_get_desc(gdev, p->chip_hwnum); *flags = p->flags; return desc;