diff mbox series

[19/21] gpio: swnode: replace gpiochip_find() with gpio_device_find_by_label()

Message ID 20230905185309.131295-20-brgl@bgdev.pl
State Superseded
Headers show
Series gpio: convert users to gpio_device_find() and remove gpiochip_find() | expand

Commit Message

Bartosz Golaszewski Sept. 5, 2023, 6:53 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We're porting all users of gpiochip_find() to using gpio_device_find().
Update the swnode GPIO code.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

Comments

Andy Shevchenko Sept. 6, 2023, 2:52 p.m. UTC | #1
On Tue, Sep 05, 2023 at 08:53:07PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're porting all users of gpiochip_find() to using gpio_device_find().
> Update the swnode GPIO code.

...

> -static int swnode_gpiochip_match_name(struct gpio_chip *chip, void *data)
> +static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
>  {
> -	return !strcmp(chip->label, data);
> -}
> +	const struct software_node *gdev_node;
> +	struct gpio_device *gdev;
>  
> -static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
> -{

Not sure if you used --patience. Patches might look better.

> -	const struct software_node *chip_node;
> -	struct gpio_chip *chip;
> -
> -	chip_node = to_software_node(fwnode);
> -	if (!chip_node || !chip_node->name)
> +	gdev_node = to_software_node(fwnode);
> +	if (!gdev_node || !gdev_node->name)
>  		return ERR_PTR(-EINVAL);
>  
> -	chip = gpiochip_find((void *)chip_node->name, swnode_gpiochip_match_name);
> -	return chip ?: ERR_PTR(-EPROBE_DEFER);
> +	gdev = gpio_device_find_by_label((void *)gdev_node->name);
> +	return gdev ?: ERR_PTR(-EPROBE_DEFER);
>  }

...

> -	desc = gpiochip_get_desc(chip, args.args[0]);
> +	desc = gpiochip_get_desc(gdev->chip, args.args[0]);

gpio_device_get_desc() ?
Linus Walleij Sept. 7, 2023, 7:40 a.m. UTC | #2
On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We're porting all users of gpiochip_find() to using gpio_device_find().
> Update the swnode GPIO code.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With Andy's comment addressed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Paul Cercueil Jan. 24, 2024, 2:59 p.m. UTC | #3
Hi Bartosz,

Le mardi 05 septembre 2023 à 20:53 +0200, Bartosz Golaszewski a écrit :
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We're porting all users of gpiochip_find() to using
> gpio_device_find().
> Update the swnode GPIO code.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-
> swnode.c
> index b5a6eaf3729b..56c8519be538 100644
> --- a/drivers/gpio/gpiolib-swnode.c
> +++ b/drivers/gpio/gpiolib-swnode.c
> @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> *con_id, char *propname,
>  		strscpy(propname, "gpios", max_size);
>  }
>  
> -static int swnode_gpiochip_match_name(struct gpio_chip *chip, void
> *data)
> +static struct gpio_device *swnode_get_gpio_device(struct
> fwnode_handle *fwnode)
>  {
> -	return !strcmp(chip->label, data);
> -}
> +	const struct software_node *gdev_node;
> +	struct gpio_device *gdev;
>  
> -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> *fwnode)
> -{
> -	const struct software_node *chip_node;
> -	struct gpio_chip *chip;
> -
> -	chip_node = to_software_node(fwnode);
> -	if (!chip_node || !chip_node->name)
> +	gdev_node = to_software_node(fwnode);
> +	if (!gdev_node || !gdev_node->name)
>  		return ERR_PTR(-EINVAL);
>  
> -	chip = gpiochip_find((void *)chip_node->name,
> swnode_gpiochip_match_name);
> -	return chip ?: ERR_PTR(-EPROBE_DEFER);
> +	gdev = gpio_device_find_by_label((void *)gdev_node->name);
> +	return gdev ?: ERR_PTR(-EPROBE_DEFER);
>  }
>  
>  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
>  				   const char *con_id, unsigned int
> idx,
>  				   unsigned long *flags)
>  {
> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;
>  	const struct software_node *swnode;
>  	struct fwnode_reference_args args;
> -	struct gpio_chip *chip;
>  	struct gpio_desc *desc;
>  	char propname[32]; /* 32 is max size of property name */
>  	int error;
> @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> fwnode_handle *fwnode,
>  		return ERR_PTR(error);
>  	}
>  
> -	chip = swnode_get_chip(args.fwnode);
> +	gdev = swnode_get_gpio_device(args.fwnode);
>  	fwnode_handle_put(args.fwnode);
> -	if (IS_ERR(chip))
> -		return ERR_CAST(chip);
> +	if (IS_ERR(gdev))
> +		return ERR_CAST(gdev);

I'm a bit late to the party, sorry.

I'm looking at how __free() should be used to use it in my own
patchset, and I was wondering if this code actually works.

What happens if swnode_get_gpio_device() returns an error pointer?
Won't that cause a call to gpio_device_put() with the invalid pointer?

Cheers,
-Paul

>  
> -	desc = gpiochip_get_desc(chip, args.args[0]);
> +	desc = gpiochip_get_desc(gdev->chip, args.args[0]);
>  	*flags = args.args[1]; /* We expect native GPIO flags */
>  
>  	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> status (%d)\n",
Bartosz Golaszewski Jan. 24, 2024, 3:04 p.m. UTC | #4
On Wed, Jan 24, 2024 at 3:59 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi Bartosz,
>
> Le mardi 05 septembre 2023 à 20:53 +0200, Bartosz Golaszewski a écrit :
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We're porting all users of gpiochip_find() to using
> > gpio_device_find().
> > Update the swnode GPIO code.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
> >  1 file changed, 12 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-
> > swnode.c
> > index b5a6eaf3729b..56c8519be538 100644
> > --- a/drivers/gpio/gpiolib-swnode.c
> > +++ b/drivers/gpio/gpiolib-swnode.c
> > @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> > *con_id, char *propname,
> >               strscpy(propname, "gpios", max_size);
> >  }
> >
> > -static int swnode_gpiochip_match_name(struct gpio_chip *chip, void
> > *data)
> > +static struct gpio_device *swnode_get_gpio_device(struct
> > fwnode_handle *fwnode)
> >  {
> > -     return !strcmp(chip->label, data);
> > -}
> > +     const struct software_node *gdev_node;
> > +     struct gpio_device *gdev;
> >
> > -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> > *fwnode)
> > -{
> > -     const struct software_node *chip_node;
> > -     struct gpio_chip *chip;
> > -
> > -     chip_node = to_software_node(fwnode);
> > -     if (!chip_node || !chip_node->name)
> > +     gdev_node = to_software_node(fwnode);
> > +     if (!gdev_node || !gdev_node->name)
> >               return ERR_PTR(-EINVAL);
> >
> > -     chip = gpiochip_find((void *)chip_node->name,
> > swnode_gpiochip_match_name);
> > -     return chip ?: ERR_PTR(-EPROBE_DEFER);
> > +     gdev = gpio_device_find_by_label((void *)gdev_node->name);
> > +     return gdev ?: ERR_PTR(-EPROBE_DEFER);
> >  }
> >
> >  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> >                                  const char *con_id, unsigned int
> > idx,
> >                                  unsigned long *flags)
> >  {
> > +     struct gpio_device *gdev __free(gpio_device_put) = NULL;
> >       const struct software_node *swnode;
> >       struct fwnode_reference_args args;
> > -     struct gpio_chip *chip;
> >       struct gpio_desc *desc;
> >       char propname[32]; /* 32 is max size of property name */
> >       int error;
> > @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> > fwnode_handle *fwnode,
> >               return ERR_PTR(error);
> >       }
> >
> > -     chip = swnode_get_chip(args.fwnode);
> > +     gdev = swnode_get_gpio_device(args.fwnode);
> >       fwnode_handle_put(args.fwnode);
> > -     if (IS_ERR(chip))
> > -             return ERR_CAST(chip);
> > +     if (IS_ERR(gdev))
> > +             return ERR_CAST(gdev);
>
> I'm a bit late to the party, sorry.
>
> I'm looking at how __free() should be used to use it in my own
> patchset, and I was wondering if this code actually works.
>
> What happens if swnode_get_gpio_device() returns an error pointer?
> Won't that cause a call to gpio_device_put() with the invalid pointer?
>
> Cheers,
> -Paul
>

No. because the __free() callback is defined as:

DEFINE_FREE(gpio_device_put, struct gpio_device *,
    if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))

Bart

> >
> > -     desc = gpiochip_get_desc(chip, args.args[0]);
> > +     desc = gpiochip_get_desc(gdev->chip, args.args[0]);
> >       *flags = args.args[1]; /* We expect native GPIO flags */
> >
> >       pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> > status (%d)\n",
>
Paul Cercueil Jan. 24, 2024, 3:11 p.m. UTC | #5
Le mercredi 24 janvier 2024 à 16:04 +0100, Bartosz Golaszewski a
écrit :
> On Wed, Jan 24, 2024 at 3:59 PM Paul Cercueil <paul@crapouillou.net>
> wrote:
> > 
> > Hi Bartosz,
> > 
> > Le mardi 05 septembre 2023 à 20:53 +0200, Bartosz Golaszewski a
> > écrit :
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > 
> > > We're porting all users of gpiochip_find() to using
> > > gpio_device_find().
> > > Update the swnode GPIO code.
> > > 
> > > Signed-off-by: Bartosz Golaszewski
> > > <bartosz.golaszewski@linaro.org>
> > > ---
> > >  drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
> > >  1 file changed, 12 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib-swnode.c
> > > b/drivers/gpio/gpiolib-
> > > swnode.c
> > > index b5a6eaf3729b..56c8519be538 100644
> > > --- a/drivers/gpio/gpiolib-swnode.c
> > > +++ b/drivers/gpio/gpiolib-swnode.c
> > > @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> > > *con_id, char *propname,
> > >               strscpy(propname, "gpios", max_size);
> > >  }
> > > 
> > > -static int swnode_gpiochip_match_name(struct gpio_chip *chip,
> > > void
> > > *data)
> > > +static struct gpio_device *swnode_get_gpio_device(struct
> > > fwnode_handle *fwnode)
> > >  {
> > > -     return !strcmp(chip->label, data);
> > > -}
> > > +     const struct software_node *gdev_node;
> > > +     struct gpio_device *gdev;
> > > 
> > > -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> > > *fwnode)
> > > -{
> > > -     const struct software_node *chip_node;
> > > -     struct gpio_chip *chip;
> > > -
> > > -     chip_node = to_software_node(fwnode);
> > > -     if (!chip_node || !chip_node->name)
> > > +     gdev_node = to_software_node(fwnode);
> > > +     if (!gdev_node || !gdev_node->name)
> > >               return ERR_PTR(-EINVAL);
> > > 
> > > -     chip = gpiochip_find((void *)chip_node->name,
> > > swnode_gpiochip_match_name);
> > > -     return chip ?: ERR_PTR(-EPROBE_DEFER);
> > > +     gdev = gpio_device_find_by_label((void *)gdev_node->name);
> > > +     return gdev ?: ERR_PTR(-EPROBE_DEFER);
> > >  }
> > > 
> > >  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> > >                                  const char *con_id, unsigned int
> > > idx,
> > >                                  unsigned long *flags)
> > >  {
> > > +     struct gpio_device *gdev __free(gpio_device_put) = NULL;
> > >       const struct software_node *swnode;
> > >       struct fwnode_reference_args args;
> > > -     struct gpio_chip *chip;
> > >       struct gpio_desc *desc;
> > >       char propname[32]; /* 32 is max size of property name */
> > >       int error;
> > > @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> > > fwnode_handle *fwnode,
> > >               return ERR_PTR(error);
> > >       }
> > > 
> > > -     chip = swnode_get_chip(args.fwnode);
> > > +     gdev = swnode_get_gpio_device(args.fwnode);
> > >       fwnode_handle_put(args.fwnode);
> > > -     if (IS_ERR(chip))
> > > -             return ERR_CAST(chip);
> > > +     if (IS_ERR(gdev))
> > > +             return ERR_CAST(gdev);
> > 
> > I'm a bit late to the party, sorry.
> > 
> > I'm looking at how __free() should be used to use it in my own
> > patchset, and I was wondering if this code actually works.
> > 
> > What happens if swnode_get_gpio_device() returns an error pointer?
> > Won't that cause a call to gpio_device_put() with the invalid
> > pointer?
> > 
> > Cheers,
> > -Paul
> > 
> 
> No. because the __free() callback is defined as:
> 
> DEFINE_FREE(gpio_device_put, struct gpio_device *,
>     if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))

Ok. I missed this.

I would argue that it's still not right though - it should probably use
IS_ERR() instead. gpio_device_put() only happens to accept NULL
pointers because the "dev" field is at the very beginning of the
"gpio_device" struct. I'm not sure this works with e.g.
CONFIG_RANDSTRUCT_FULL.

> Bart

Cheers,
-Paul

> > > 
> > > -     desc = gpiochip_get_desc(chip, args.args[0]);
> > > +     desc = gpiochip_get_desc(gdev->chip, args.args[0]);
> > >       *flags = args.args[1]; /* We expect native GPIO flags */
> > > 
> > >       pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> > > status (%d)\n",
> >
Paul Cercueil Jan. 24, 2024, 3:18 p.m. UTC | #6
Le mercredi 24 janvier 2024 à 16:04 +0100, Bartosz Golaszewski a
écrit :
> On Wed, Jan 24, 2024 at 3:59 PM Paul Cercueil <paul@crapouillou.net>
> wrote:
> > 
> > Hi Bartosz,
> > 
> > Le mardi 05 septembre 2023 à 20:53 +0200, Bartosz Golaszewski a
> > écrit :
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > 
> > > We're porting all users of gpiochip_find() to using
> > > gpio_device_find().
> > > Update the swnode GPIO code.
> > > 
> > > Signed-off-by: Bartosz Golaszewski
> > > <bartosz.golaszewski@linaro.org>
> > > ---
> > >  drivers/gpio/gpiolib-swnode.c | 29 ++++++++++++-----------------
> > >  1 file changed, 12 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib-swnode.c
> > > b/drivers/gpio/gpiolib-
> > > swnode.c
> > > index b5a6eaf3729b..56c8519be538 100644
> > > --- a/drivers/gpio/gpiolib-swnode.c
> > > +++ b/drivers/gpio/gpiolib-swnode.c
> > > @@ -31,31 +31,26 @@ static void swnode_format_propname(const char
> > > *con_id, char *propname,
> > >               strscpy(propname, "gpios", max_size);
> > >  }
> > > 
> > > -static int swnode_gpiochip_match_name(struct gpio_chip *chip,
> > > void
> > > *data)
> > > +static struct gpio_device *swnode_get_gpio_device(struct
> > > fwnode_handle *fwnode)
> > >  {
> > > -     return !strcmp(chip->label, data);
> > > -}
> > > +     const struct software_node *gdev_node;
> > > +     struct gpio_device *gdev;
> > > 
> > > -static struct gpio_chip *swnode_get_chip(struct fwnode_handle
> > > *fwnode)
> > > -{
> > > -     const struct software_node *chip_node;
> > > -     struct gpio_chip *chip;
> > > -
> > > -     chip_node = to_software_node(fwnode);
> > > -     if (!chip_node || !chip_node->name)
> > > +     gdev_node = to_software_node(fwnode);
> > > +     if (!gdev_node || !gdev_node->name)
> > >               return ERR_PTR(-EINVAL);
> > > 
> > > -     chip = gpiochip_find((void *)chip_node->name,
> > > swnode_gpiochip_match_name);
> > > -     return chip ?: ERR_PTR(-EPROBE_DEFER);
> > > +     gdev = gpio_device_find_by_label((void *)gdev_node->name);
> > > +     return gdev ?: ERR_PTR(-EPROBE_DEFER);
> > >  }
> > > 
> > >  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
> > >                                  const char *con_id, unsigned int
> > > idx,
> > >                                  unsigned long *flags)
> > >  {
> > > +     struct gpio_device *gdev __free(gpio_device_put) = NULL;
> > >       const struct software_node *swnode;
> > >       struct fwnode_reference_args args;
> > > -     struct gpio_chip *chip;
> > >       struct gpio_desc *desc;
> > >       char propname[32]; /* 32 is max size of property name */
> > >       int error;
> > > @@ -77,12 +72,12 @@ struct gpio_desc *swnode_find_gpio(struct
> > > fwnode_handle *fwnode,
> > >               return ERR_PTR(error);
> > >       }
> > > 
> > > -     chip = swnode_get_chip(args.fwnode);
> > > +     gdev = swnode_get_gpio_device(args.fwnode);
> > >       fwnode_handle_put(args.fwnode);
> > > -     if (IS_ERR(chip))
> > > -             return ERR_CAST(chip);
> > > +     if (IS_ERR(gdev))
> > > +             return ERR_CAST(gdev);
> > 
> > I'm a bit late to the party, sorry.
> > 
> > I'm looking at how __free() should be used to use it in my own
> > patchset, and I was wondering if this code actually works.
> > 
> > What happens if swnode_get_gpio_device() returns an error pointer?
> > Won't that cause a call to gpio_device_put() with the invalid
> > pointer?
> > 
> > Cheers,
> > -Paul
> > 
> 
> No. because the __free() callback is defined as:
> 
> DEFINE_FREE(gpio_device_put, struct gpio_device *,
>     if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))

Disregard my previous email, I'm stupid. This actually checks that the
pointer is non-null.

Cheers,
-Paul

> 
> Bart
> 
> > > 
> > > -     desc = gpiochip_get_desc(chip, args.args[0]);
> > > +     desc = gpiochip_get_desc(gdev->chip, args.args[0]);
> > >       *flags = args.args[1]; /* We expect native GPIO flags */
> > > 
> > >       pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' -
> > > status (%d)\n",
> >
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index b5a6eaf3729b..56c8519be538 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -31,31 +31,26 @@  static void swnode_format_propname(const char *con_id, char *propname,
 		strscpy(propname, "gpios", max_size);
 }
 
-static int swnode_gpiochip_match_name(struct gpio_chip *chip, void *data)
+static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
 {
-	return !strcmp(chip->label, data);
-}
+	const struct software_node *gdev_node;
+	struct gpio_device *gdev;
 
-static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
-{
-	const struct software_node *chip_node;
-	struct gpio_chip *chip;
-
-	chip_node = to_software_node(fwnode);
-	if (!chip_node || !chip_node->name)
+	gdev_node = to_software_node(fwnode);
+	if (!gdev_node || !gdev_node->name)
 		return ERR_PTR(-EINVAL);
 
-	chip = gpiochip_find((void *)chip_node->name, swnode_gpiochip_match_name);
-	return chip ?: ERR_PTR(-EPROBE_DEFER);
+	gdev = gpio_device_find_by_label((void *)gdev_node->name);
+	return gdev ?: ERR_PTR(-EPROBE_DEFER);
 }
 
 struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
 				   const char *con_id, unsigned int idx,
 				   unsigned long *flags)
 {
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
 	const struct software_node *swnode;
 	struct fwnode_reference_args args;
-	struct gpio_chip *chip;
 	struct gpio_desc *desc;
 	char propname[32]; /* 32 is max size of property name */
 	int error;
@@ -77,12 +72,12 @@  struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
 		return ERR_PTR(error);
 	}
 
-	chip = swnode_get_chip(args.fwnode);
+	gdev = swnode_get_gpio_device(args.fwnode);
 	fwnode_handle_put(args.fwnode);
-	if (IS_ERR(chip))
-		return ERR_CAST(chip);
+	if (IS_ERR(gdev))
+		return ERR_CAST(gdev);
 
-	desc = gpiochip_get_desc(chip, args.args[0]);
+	desc = gpiochip_get_desc(gdev->chip, args.args[0]);
 	*flags = args.args[1]; /* We expect native GPIO flags */
 
 	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",