| 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 |
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() ?
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
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",
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", >
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", > >
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 --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",