Re: [PATCH v2 1/1] gpio: mmio: handle "ngpios" properly in bgpio_init()

[Bartosz Golaszewski]

On Tue, Jul 18, 2023 at 8:38 PM Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:
>
>  > bgpio_init() uses "sz" argument to populate ngpio, which is not accurate.
> > Instead, read the "ngpios" property from the DT and if it doesn't exist, use the
> > "sz" argument. With this change, drivers no longer need to overwrite the ngpio
> > variable after calling bgpio_init().
> >
> > If the "ngpios" property is specified, bgpio_bits is calculated as the round up
> > value of ngpio. At the moment, the only requirement specified is that the round
> > up value must be a multiple of 8 but it should also be a power of 2 because we
> > provide accessors based on the bank size in bgpio_setup_accessors().
> >
> > Signed-off-by: Asmaa Mnebhi <asmaa@xxxxxxxxxx>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > ---
> > The following 2 patches were approved in March 2023 but didn't make it into
> > the tree:
> > [PATCH v4] gpio: mmio: handle "ngpios" properly in bgpio_init() [PATCH v1]
> > gpio: mmio: fix calculation of bgpio_bits
> >
> > They needed a rebase and were combined into a single patch since
> > "gpio: mmio: fix calculation of bgpio_bits" fixes a bug in
> > "gpio: mmio: handle "ngpios" properly in bgpio_init()"
> >
> > v1->v2:
> > - Added the tags
> > - Updated the changelog
> >
> >  drivers/gpio/gpio-mmio.c |  9 +++++-
> >  drivers/gpio/gpiolib.c   | 68 ++++++++++++++++++++++------------------
> >  drivers/gpio/gpiolib.h   |  1 +
> >  3 files changed, 46 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c index
> > d9dff3dc92ae..74fdf0d87b2c 100644
> > --- a/drivers/gpio/gpio-mmio.c
> > +++ b/drivers/gpio/gpio-mmio.c
> > @@ -60,6 +60,8 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA
> > is ,.`
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >
> > +#include "gpiolib.h"
> > +
> >  static void bgpio_write8(void __iomem *reg, unsigned long data)  {
> >       writeb(data, reg);
> > @@ -614,10 +616,15 @@ int bgpio_init(struct gpio_chip *gc, struct device
> > *dev,
> >       gc->parent = dev;
> >       gc->label = dev_name(dev);
> >       gc->base = -1;
> > -     gc->ngpio = gc->bgpio_bits;
> >       gc->request = bgpio_request;
> >       gc->be_bits = !!(flags & BGPIOF_BIG_ENDIAN);
> >
> > +     ret = gpiochip_get_ngpios(gc, dev);
> > +     if (ret)
> > +             gc->ngpio = gc->bgpio_bits;
> > +     else
> > +             gc->bgpio_bits = roundup_pow_of_two(round_up(gc->ngpio,
> > 8));
> > +
> >       ret = bgpio_setup_io(gc, dat, set, clr, flags);
> >       if (ret)
> >               return ret;
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index
> > 251c875b5c34..7dac8bb9905a 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -700,6 +700,40 @@ void *gpiochip_get_data(struct gpio_chip *gc)  }
> > EXPORT_SYMBOL_GPL(gpiochip_get_data);
> >
> > +int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev) {
> > +     u32 ngpios = gc->ngpio;
> > +     int ret;
> > +
> > +     if (ngpios == 0) {
> > +             ret = device_property_read_u32(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)
> > +                     return ret;
> > +
> > +             gc->ngpio = ngpios;
> > +     }
> > +
> > +     if (gc->ngpio == 0) {
> > +             chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (gc->ngpio > FASTPATH_NGPIO)
> > +             chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n",
> > +                     gc->ngpio, FASTPATH_NGPIO);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gpiochip_get_ngpios);
> > +
> >  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >                              struct lock_class_key *lock_key,
> >                              struct lock_class_key *request_key) @@ -707,7
> > +741,6 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >       struct gpio_device *gdev;
> >       unsigned long flags;
> >       unsigned int i;
> > -     u32 ngpios = 0;
> >       int base = 0;
> >       int ret = 0;
> >
> > @@ -753,36 +786,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
> > void *data,
> >       else
> >               gdev->owner = THIS_MODULE;
> >
> > -     /*
> > -      * Try the device properties if the driver didn't supply the number
> > -      * of GPIO lines.
> > -      */
> > -     ngpios = gc->ngpio;
> > -     if (ngpios == 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_dev_name;
> > -
> > -             gc->ngpio = ngpios;
> > -     }
> > -
> > -     if (gc->ngpio == 0) {
> > -             chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
> > -             ret = -EINVAL;
> > +     ret = gpiochip_get_ngpios(gc, &gdev->dev);
> > +     if (ret)
> >               goto err_free_dev_name;
> > -     }
> > -
> > -     if (gc->ngpio > FASTPATH_NGPIO)
> > -             chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n",
> > -                       gc->ngpio, FASTPATH_NGPIO);
> >
> >       gdev->descs = kcalloc(gc->ngpio, sizeof(*gdev->descs), GFP_KERNEL);
> >       if (!gdev->descs) {
> > @@ -947,7 +953,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc,
> > void *data,
> >       /* failures here can mean systems won't boot... */
> >       if (ret != -EPROBE_DEFER) {
> >               pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n",
> > __func__,
> > -                    base, base + (int)ngpios - 1,
> > +                    base, base + (int)gc->ngpio - 1,
> >                      gc->label ? : "generic", ret);
> >       }
> >       return ret;
> > diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h index
> > cca81375f127..8de748a16d13 100644
> > --- a/drivers/gpio/gpiolib.h
> > +++ b/drivers/gpio/gpiolib.h
> > @@ -217,6 +217,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const
> > char *con_id,  int gpio_set_debounce_timeout(struct gpio_desc *desc,
> > unsigned int debounce);  int gpiod_hog(struct gpio_desc *desc, const char
> > *name,
> >               unsigned long lflags, enum gpiod_flags dflags);
> > +int gpiochip_get_ngpios(struct gpio_chip *gc, struct device *dev);
> >
> >  /*
> >   * Return the GPIO number of the passed descriptor relative to its chip
> > --
> > 2.30.1
>
> Hi Bart,
>
> This is the final approved patch by both Linus and Andy. Please discard all others.
>

Ok, I applied this one but you need to get your patch versioning in
order next time.

Bart