diff mbox series

[v2,07/11] gpiolib: replace find_chip_by_name() with gpio_device_find_by_label()

Message ID 20230912100727.23197-8-brgl@bgdev.pl (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series gpiolib: work towards removing gpiochip_find() | expand

Commit Message

Bartosz Golaszewski Sept. 12, 2023, 10:07 a.m. UTC 
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

Andy Shevchenko Sept. 12, 2023, 11:08 a.m. UTC | #1 
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?

>  		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.
Andy Shevchenko Sept. 12, 2023, 11:16 a.m. UTC | #2 
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.
Bartosz Golaszewski Sept. 12, 2023, 11:34 a.m. UTC | #3 
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
>
>
Bartosz Golaszewski Sept. 12, 2023, 11:35 a.m. UTC | #4 
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
>
>
Bartosz Golaszewski Sept. 15, 2023, 9:44 a.m. UTC | #5 
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
> >
> >
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 224e0d330009..a10d1d663524 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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);
 		}