| Message ID | 20211111202506.19459-2-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpiolib: improve coding style for local variables | expand |
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
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. > }
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
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.
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)
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(-)