Re: [PATCH v1 1/1] gpio: altera: Add missed base and label initialisations

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, Nov 16, 2024 at 3:12 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> On Fri, Nov 15, 2024 at 1:30 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > During conversion driver to modern APIs the base field initial value
> > of the GPIO chip was moved from -1 to 0, which triggers a warning.
> > Add missed base initialisation as it was in the original code.
> >
> > Also initialise the GPIO chip label correctly.
> >
> > Fixes: 50dded8d9d62 ("gpio: altera: Drop legacy-of-mm-gpiochip.h header")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > ---
> >
> > Can be folded into the mentioned change, up to you and sorry for missing
> > these in the first place.
> >
> >  drivers/gpio/gpio-altera.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpio/gpio-altera.c b/drivers/gpio/gpio-altera.c
> > index 039fef26546e..73e660c5e38a 100644
> > --- a/drivers/gpio/gpio-altera.c
> > +++ b/drivers/gpio/gpio-altera.c
> > @@ -261,6 +261,11 @@ static int altera_gpio_probe(struct platform_device *pdev)
> >         altera_gc->gc.set               = altera_gpio_set;
> >         altera_gc->gc.owner             = THIS_MODULE;
> >         altera_gc->gc.parent            = &pdev->dev;
> > +       altera_gc->gc.base              = -1;
>
> This does in fact fix commit 50dded8d9d62 ("gpio: altera: Drop
> legacy-of-mm-gpiochip.h header") and should go together with it
> upstream.
>
> > +
> > +       altera_gc->gc.label = devm_kasprintf(dev, GFP_KERNEL, "%pfw", dev_fwnode(dev));
> > +       if (!altera_gc->gc.label)
> > +               return -ENOMEM;
>
> This is a separate thing and I would argue it's not even a fix per se.
> Lack of label doesn't make it entirely impossible to use the chip. I'm
> willing to send it as a fix but please make it a separate commit as it
> doesn't fix anything from 50dded8d9d62.
>
> The commit message for it could also use some explanation why it's
> better to set label (so that the chip will not figure as "unknown" in
> the kernel).
>
> Bart

Ah scratch that... of_mm_gpiochip_add_data() would set the label from
the OF node's name.

In that case this can be a single commit but please explain it better
in the commit message to avoid confusion. Say what happened and why we
need to set the label in the driver now.

Bart

>
> >
> >         altera_gc->regs = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(altera_gc->regs))
> > --
> > 2.43.0.rc1.1336.g36b5255a03ac
> >