MIME-Version: 1.0
References: <20230303215814.24783-1-asmaa@nvidia.com>
In-Reply-To: <20230303215814.24783-1-asmaa@nvidia.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 4 Mar 2023 00:35:09 +0200
Message-ID: <CAHp75VfpRVf3fzBf76mipu3dWDbwGCBFUOzyR-P=DLab3_y=Kw@mail.gmail.com>
Subject: Re: [PATCH v3] gpio: mmio: handle "ngpios" properly in bgpio_init()
To:     Asmaa Mnebhi <asmaa@nvidia.com>
Cc:     linus.walleij@linaro.org, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Mar 3, 2023 at 11:58=E2=80=AFPM Asmaa Mnebhi <asmaa@nvidia.com> wro=
te:

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 =3D gpiochip_get_ngpios(gc, dev);
> +       if (ret)
> +               gc->ngpio =3D 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 =3D gc->ngpio;
> +       int ret;
> +
> +       if (ngpios =3D=3D 0) {

> +               ret =3D 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 =3D 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.

--=20
With Best Regards,
Andy Shevchenko