Re: [PATCH v3] gpio: mmio: handle "ngpios" properly in bgpio_init()

[Andy Shevchenko]

On Fri, Mar 3, 2023 at 11:58 PM Asmaa Mnebhi <asmaa@xxxxxxxxxx> wrote:

Thanks for an update!
My comments below.

> 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.

You missed adding the () to the function name here.

...

First of all there should be two patches:
1) splitting out the new helper;
2) using it in the mmio driver.

...

> +       ret = gpiochip_get_ngpios(gc, dev);
> +       if (ret)
> +               gc->ngpio = gc->bgpio_bits;

But this doesn't update bgpio_bits in the success case. Can you
explain why it's not a problem (should be at least in the code as a
comment).

...

> +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) {
> +                       chip_err(gc, "Failed to get ngpios property\n");
> +                       return -EINVAL;
> +               }

This is not an equivalent to what was in the GPIO library. Why is it so?

> +               gc->ngpio = ngpios;
> +       }
> +
> +       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;
> +}

...

>                 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);

AFAIU this will give a different result to what was previous in one of
the error cases.

-- 
With Best Regards,
Andy Shevchenko