[v2,07/11] gpiolib: replace find_chip_by_name() with gpio_device_find_by_label()
Commit Message
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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpiolib.c | 33 ++++++++++++---------------------
1 file changed, 12 insertions(+), 21 deletions(-)
Comments
On Tue, Sep 12, 2023 at 12:07:23PM +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.
...
> for (p = &table->table[0]; p->key; p++) {
> - struct gpio_chip *gc;
> + struct gpio_device *gdev __free(gpio_device_put) = NULL;
> + gc = gpio_device_get_chip(gdev);
What the heck is this, btw? You have gdev NULL here.
> /* idx must always match exactly */
> if (p->idx != idx)
> @@ -4004,9 +3996,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) {
...
> if (gc->ngpio <= p->chip_hwnum) {
> dev_err(dev,
> "requested GPIO %u (%u) is out of range [0..%u] for chip %s\n",
> - idx, p->chip_hwnum, gc->ngpio - 1,
> + idx, p->chip_hwnum, gdev->chip->ngpio - 1,
In other patch you use wrapper to get gdev->chip, why not here?
> gc->label);
Is this gc is different to gdev->chip?
> return ERR_PTR(-EINVAL);
> }
...
Sorry, but this patch seems to me as WIP. Please, revisit it, make sure all
things are done consistently.
On Tue, Sep 12, 2023 at 1:08 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 12, 2023 at 12:07:23PM +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.
>
> ...
>
> > for (hog = &hogs[0]; hog->chip_label; hog++) {
> > + struct gpio_device *gdev __free(gpio_device_put) = NULL;
>
> In the loop?! How does it work when loop goes second iteration and so on?
>
This works fine, the variable goes out of scope (reference is put) on
every iteration.
Bart
> > 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);
>
> So, do we expect the chip_label be different between hogs? Ah, seems so
> as it covers _all_ hogs in the system.
>
> > }
>
> Even if the __free() scope works fine, I think this algorithm should be
> revisited to make sure we have iterating only on hogs of the same chip.
> Hence, the hogs should be placed into tree structure with a label being
> the key in it.
>
> ...
>
> > + struct gpio_device *gdev __free(gpio_device_put) = NULL;
>
> > + gc = gpio_device_get_chip(gdev);
>
> Similar wish here, perhaps maple tree can be utilized in the future for both of them.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Tue, Sep 12, 2023 at 1:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 12, 2023 at 12:07:23PM +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.
>
> ...
>
> > for (p = &table->table[0]; p->key; p++) {
> > - struct gpio_chip *gc;
> > + struct gpio_device *gdev __free(gpio_device_put) = NULL;
>
> > + gc = gpio_device_get_chip(gdev);
>
> What the heck is this, btw? You have gdev NULL here.
>
Gah! Thanks. I relied on tests succeeding and no KASAN warnings, I
need to go through this line-by-line again.
Bart
> > /* idx must always match exactly */
> > if (p->idx != idx)
> > @@ -4004,9 +3996,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) {
>
> ...
>
> > if (gc->ngpio <= p->chip_hwnum) {
> > dev_err(dev,
> > "requested GPIO %u (%u) is out of range [0..%u] for chip %s\n",
> > - idx, p->chip_hwnum, gc->ngpio - 1,
> > + idx, p->chip_hwnum, gdev->chip->ngpio - 1,
>
> In other patch you use wrapper to get gdev->chip, why not here?
>
> > gc->label);
>
> Is this gc is different to gdev->chip?
>
> > return ERR_PTR(-EINVAL);
> > }
>
> ...
>
> Sorry, but this patch seems to me as WIP. Please, revisit it, make sure all
> things are done consistently.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Tue, Sep 12, 2023 at 1:35 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Sep 12, 2023 at 1:16 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Sep 12, 2023 at 12:07:23PM +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.
> >
> > ...
> >
> > > for (p = &table->table[0]; p->key; p++) {
> > > - struct gpio_chip *gc;
> > > + struct gpio_device *gdev __free(gpio_device_put) = NULL;
> >
> > > + gc = gpio_device_get_chip(gdev);
> >
> > What the heck is this, btw? You have gdev NULL here.
> >
>
> Gah! Thanks. I relied on tests succeeding and no KASAN warnings, I
> need to go through this line-by-line again.
>
Fortunately, this was just an unused leftover. I fixed it for v3.
Bart
> Bart
>
> > > /* idx must always match exactly */
> > > if (p->idx != idx)
> > > @@ -4004,9 +3996,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) {
> >
> > ...
> >
> > > if (gc->ngpio <= p->chip_hwnum) {
> > > dev_err(dev,
> > > "requested GPIO %u (%u) is out of range [0..%u] for chip %s\n",
> > > - idx, p->chip_hwnum, gc->ngpio - 1,
> > > + idx, p->chip_hwnum, gdev->chip->ngpio - 1,
> >
> > In other patch you use wrapper to get gdev->chip, why not here?
> >
> > > gc->label);
> >
> > Is this gc is different to gdev->chip?
> >
> > > return ERR_PTR(-EINVAL);
> > > }
> >
> > ...
> >
> > Sorry, but this patch seems to me as WIP. Please, revisit it, make sure all
> > things are done consistently.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
@@ -1144,18 +1144,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
@@ -3907,21 +3895,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);
@@ -3976,13 +3965,16 @@ 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;
+
+ gc = gpio_device_get_chip(gdev);
/* idx must always match exactly */
if (p->idx != idx)
@@ -4004,9 +3996,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
@@ -4022,7 +4013,7 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
if (gc->ngpio <= p->chip_hwnum) {
dev_err(dev,
"requested GPIO %u (%u) is out of range [0..%u] for chip %s\n",
- idx, p->chip_hwnum, gc->ngpio - 1,
+ idx, p->chip_hwnum, gdev->chip->ngpio - 1,
gc->label);
return ERR_PTR(-EINVAL);
}