Message ID | 20211202134034.14048-2-brgl@bgdev.pl |
---|---|
State | New |
Headers | show |
Series | [v4,1/2] gpiolib: improve coding style for local variables | expand |
On Thu, Dec 02, 2021 at 02:40:34PM +0100, Bartosz Golaszewski 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. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> One nit-pick below (you may amend it when applying) > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > v1 -> v2: > - use device_property_read_u32() instead of fwnode_property_read_u32() > - reverse the error check logic > > v2 -> v3: > - don't shadow errors other than -ENODATA in device_property_read_u32() > > v3 -> v4: > - also make sure we return -EINVAL when the device 'ngpios' property is > set to 0 (thanks Andy!) > > drivers/gpio/gpiolib.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index ede8b8a7aa18..bd9b8cb53476 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -599,6 +599,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > int base = gc->base; > unsigned int i; > int ret = 0; > + u32 ngpios; > > /* > * First: allocate and populate the internal stat container, and > @@ -646,6 +647,26 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > goto err_free_dev_name; > } > > + /* > + * Try the device properties if the driver didn't supply the number > + * of GPIO lines. > + */ > + if (gc->ngpio == 0) { > + ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios); > + if (ret == -ENODATA) > + /* > + * -ENODATA means that there is no property found and > + * we want to issue the error message to the user. > + * Besides that, we want to return different error code > + * to state that supplied value is not valid. > + * */ First '* ' is not needed. > + ngpios = 0; > + else if (ret) > + goto err_free_descs; > + > + gc->ngpio = ngpios; > + } > + > if (gc->ngpio == 0) { > chip_err(gc, "tried to insert a GPIO chip with zero lines\n"); > ret = -EINVAL; > -- > 2.25.1 >
On Thu, Dec 2, 2021 at 4:38 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Dec 02, 2021 at 02:40:34PM +0100, Bartosz Golaszewski 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. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > One nit-pick below (you may amend it when applying) > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > v1 -> v2: > > - use device_property_read_u32() instead of fwnode_property_read_u32() > > - reverse the error check logic > > > > v2 -> v3: > > - don't shadow errors other than -ENODATA in device_property_read_u32() > > > > v3 -> v4: > > - also make sure we return -EINVAL when the device 'ngpios' property is > > set to 0 (thanks Andy!) > > > > drivers/gpio/gpiolib.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index ede8b8a7aa18..bd9b8cb53476 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -599,6 +599,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > > int base = gc->base; > > unsigned int i; > > int ret = 0; > > + u32 ngpios; > > > > /* > > * First: allocate and populate the internal stat container, and > > @@ -646,6 +647,26 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > > goto err_free_dev_name; > > } > > > > + /* > > + * Try the device properties if the driver didn't supply the number > > + * of GPIO lines. > > + */ > > + if (gc->ngpio == 0) { > > + ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios); > > + if (ret == -ENODATA) > > + /* > > + * -ENODATA means that there is no property found and > > + * we want to issue the error message to the user. > > + * Besides that, we want to return different error code > > + * to state that supplied value is not valid. > > > + * */ > > First '* ' is not needed. > I'll fix it when applying. Bart
On Thu, Dec 2, 2021 at 2:40 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> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > v1 -> v2: > - use device_property_read_u32() instead of fwnode_property_read_u32() > - reverse the error check logic > > v2 -> v3: > - don't shadow errors other than -ENODATA in device_property_read_u32() > > v3 -> v4: > - also make sure we return -EINVAL when the device 'ngpios' property is > set to 0 (thanks Andy!) > > drivers/gpio/gpiolib.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index ede8b8a7aa18..bd9b8cb53476 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -599,6 +599,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > int base = gc->base; > unsigned int i; > int ret = 0; > + u32 ngpios; > > /* > * First: allocate and populate the internal stat container, and > @@ -646,6 +647,26 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > goto err_free_dev_name; > } > > + /* > + * Try the device properties if the driver didn't supply the number > + * of GPIO lines. > + */ > + if (gc->ngpio == 0) { > + ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios); > + if (ret == -ENODATA) > + /* > + * -ENODATA means that there is no property found and > + * we want to issue the error message to the user. > + * Besides that, we want to return different error code > + * to state that supplied value is not valid. > + * */ > + ngpios = 0; > + else if (ret) > + goto err_free_descs; > + > + gc->ngpio = ngpios; > + } > + > if (gc->ngpio == 0) { > chip_err(gc, "tried to insert a GPIO chip with zero lines\n"); > ret = -EINVAL; > -- > 2.25.1 > Queued for next. Bart
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index ede8b8a7aa18..bd9b8cb53476 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -599,6 +599,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, int base = gc->base; unsigned int i; int ret = 0; + u32 ngpios; /* * First: allocate and populate the internal stat container, and @@ -646,6 +647,26 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, goto err_free_dev_name; } + /* + * Try the device properties if the driver didn't supply the number + * of GPIO lines. + */ + if (gc->ngpio == 0) { + ret = device_property_read_u32(&gdev->dev, "ngpios", &ngpios); + if (ret == -ENODATA) + /* + * -ENODATA means that there is no property found and + * we want to issue the error message to the user. + * Besides that, we want to return different error code + * to state that supplied value is not valid. + * */ + ngpios = 0; + else if (ret) + goto err_free_descs; + + gc->ngpio = ngpios; + } + if (gc->ngpio == 0) { chip_err(gc, "tried to insert a GPIO chip with zero lines\n"); ret = -EINVAL;