[PATCH 1/2] gpiolib: improve coding style for local variables

[PATCH 1/2] gpiolib: improve coding style for local variables

[Bartosz Golaszewski]

Drop unneeded whitespaces and put the variables of the same type
together for consistency with the rest of the code.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpiolib.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d1b9b721218f..4c34c96ef136 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -594,11 +594,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			       struct lock_class_key *request_key)
 {
 	struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;
-	unsigned long	flags;
-	int		ret = 0;
-	unsigned	i;
-	int		base = gc->base;
+	int ret = 0, base = gc->base;
 	struct gpio_device *gdev;
+	unsigned long flags;
+	unsigned int i;
 
 	/*
 	 * First: allocate and populate the internal stat container, and
-- 
2.30.1

[PATCH RFC 2/2] gpiolib: check the 'ngpios' property in core gpiolib code

[Bartosz Golaszewski]

Several drivers read the 'ngpios' device property on their own, but
since it's defined as a standard GPIO property in the device tree bindings
anyway, it's a good candidate for generalization. If the driver didn't
set its gc->ngpio, try to read the 'ngpios' property from the GPIO
device's firmware node before bailing out.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4c34c96ef136..23b0478163d4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -598,6 +598,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	struct gpio_device *gdev;
 	unsigned long flags;
 	unsigned int i;
+	u32 ngpios;
 
 	/*
 	 * First: allocate and populate the internal stat container, and
@@ -646,9 +647,15 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	}
 
 	if (gc->ngpio == 0) {
-		chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
-		ret = -EINVAL;
-		goto err_free_descs;
+		ret = fwnode_property_read_u32(gdev->dev.fwnode, "ngpios",
+					       &ngpios);
+		if (ret == 0) {
+			gc->ngpio = ngpios;
+		} else {
+			chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
+			ret = -EINVAL;
+			goto err_free_descs;
+		}
 	}
 
 	if (gc->ngpio > FASTPATH_NGPIO)
-- 
2.30.1

Re: [PATCH 1/2] gpiolib: improve coding style for local variables

[Linus Walleij]

On Thu, Nov 11, 2021 at 9:25 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Drop unneeded whitespaces and put the variables of the same type
> together for consistency with the rest of the code.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

OK then (also results in inverse christmas-tree shape...)

Yours,
Linus Walleij

Re: [PATCH 1/2] gpiolib: improve coding style for local variables

[Linus Walleij]

On Thu, Nov 11, 2021 at 9:29 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Nov 11, 2021 at 9:25 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > Drop unneeded whitespaces and put the variables of the same type
> > together for consistency with the rest of the code.
> >
> > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
>
> OK then (also results in inverse christmas-tree shape...)

i.e.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH RFC 2/2] gpiolib: check the 'ngpios' property in core gpiolib code

[Linus Walleij]

On Thu, Nov 11, 2021 at 9:25 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Several drivers read the 'ngpios' device property on their own, but
> since it's defined as a standard GPIO property in the device tree bindings
> anyway, it's a good candidate for generalization. If the driver didn't
> set its gc->ngpio, try to read the 'ngpios' property from the GPIO
> device's firmware node before bailing out.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Nice!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH RFC 2/2] gpiolib: check the 'ngpios' property in core gpiolib code

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 10:26 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> Several drivers read the 'ngpios' device property on their own, but
> since it's defined as a standard GPIO property in the device tree bindings
> anyway, it's a good candidate for generalization. If the driver didn't
> set its gc->ngpio, try to read the 'ngpios' property from the GPIO
> device's firmware node before bailing out.

Thanks!

...

> +               ret = fwnode_property_read_u32(gdev->dev.fwnode, "ngpios",
> +                                              &ngpios);

I'm wondering if there is any obstacle to call

               ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios);

?

Rationale (the main one) is to avoid direct dereference of fwnode from
struct device (it might be changed in the future). I really prefer API
calls here.

> +               if (ret == 0) {
> +                       gc->ngpio = ngpios;
> +               } else {
> +                       chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> +                       ret = -EINVAL;
> +                       goto err_free_descs;
> +               }

I would prefer the other way around and without 'else' being involved.

>         }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 2/2] gpiolib: check the 'ngpios' property in core gpiolib code

[Bartosz Golaszewski]

On Thu, Nov 11, 2021 at 9:47 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 10:26 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > Several drivers read the 'ngpios' device property on their own, but
> > since it's defined as a standard GPIO property in the device tree bindings
> > anyway, it's a good candidate for generalization. If the driver didn't
> > set its gc->ngpio, try to read the 'ngpios' property from the GPIO
> > device's firmware node before bailing out.
>
> Thanks!
>
> ...
>
> > +               ret = fwnode_property_read_u32(gdev->dev.fwnode, "ngpios",
> > +                                              &ngpios);
>
> I'm wondering if there is any obstacle to call
>
>                ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios);
>
> ?
>
> Rationale (the main one) is to avoid direct dereference of fwnode from
> struct device (it might be changed in the future). I really prefer API
> calls here.
>
> > +               if (ret == 0) {
> > +                       gc->ngpio = ngpios;
> > +               } else {
> > +                       chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> > +                       ret = -EINVAL;
> > +                       goto err_free_descs;
> > +               }
>
> I would prefer the other way around and without 'else' being involved.
>
> >         }
>
>
> --
> With Best Regards,
> Andy Shevchenko

Will do. Just a note: some drivers that parse the ngpios property will
need to continue doing it themselves as they have additional
requirements from the value (limited max value, needs to be multiple
of 8, etc.) on the read value. For those that are obvious, we can
start converting them after this patch lands.

Bart

Re: [PATCH RFC 2/2] gpiolib: check the 'ngpios' property in core gpiolib code

[Andy Shevchenko]

On Fri, Nov 12, 2021 at 03:48:34PM +0100, Bartosz Golaszewski wrote:
> On Thu, Nov 11, 2021 at 9:47 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> Will do. Just a note: some drivers that parse the ngpios property will
> need to continue doing it themselves as they have additional
> requirements from the value (limited max value, needs to be multiple
> of 8, etc.) on the read value. For those that are obvious, we can
> start converting them after this patch lands.

Sure! No objections from my side.

-- 
With Best Regards,
Andy Shevchenko