Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

On Sun, Jul 4, 2021 at 12:05 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Sun, Jul 4, 2021 at 11:06 AM Sergio Paracuellos
> <sergio.paracuellos@xxxxxxxxx> wrote:
> > On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
> > <sergio.paracuellos@xxxxxxxxx> wrote:
> > > On Sat, Jul 3, 2021 at 9:36 PM Andy Shevchenko
> > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > On Sat, Jul 3, 2021 at 3:51 PM Sergio Paracuellos
> > > > <sergio.paracuellos@xxxxxxxxx> wrote:
> > > > > On Sat, Jul 3, 2021 at 2:05 PM Sergio Paracuellos
> > > > > <sergio.paracuellos@xxxxxxxxx> wrote:
>
> ...
>
> > > > The below is closer to what I meant, yes. I have not much time to look
> > > > into the details, but I don't have objections about what you suggested
> > > > below. Additional comments there as well.
> > >
> > > Thanks for your time and review, Andy. Let's wait to see if Linus and
> > > Bartosz are also ok with this approach.
> > >
> > > > > How about something like this?
> > > > >
> > > > > diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c
> > > > > index 82fb20dca53a..5854a9343491 100644
> > > > > --- a/drivers/gpio/gpio-mt7621.c
> > > > > +++ b/drivers/gpio/gpio-mt7621.c
> > > > > @@ -241,6 +241,7 @@ mediatek_gpio_bank_probe(struct device *dev,
> > > > >         if (!rg->chip.label)
> > > > >                 return -ENOMEM;
> > > > >
> > > > > +       rg->chip.offset = bank * MTK_BANK_WIDTH;
> > > > >         rg->irq_chip.name = dev_name(dev);
> > > > >         rg->irq_chip.parent_device = dev;
> > > > >         rg->irq_chip.irq_unmask = mediatek_gpio_irq_unmask;
> > > >
> > > > Obviously it should be a separate patch :-)
> > >
> > > Of course :). I will include one separate patch per driver using the
> > > custom set names stuff: gpio-mt7621 and gpio-brcmstb. I don't know if
> > > any other one is also following that wrong pattern.
> >
> > What if each gpiochip inside the same driver has a different width? In
> > such a case (looking into the code seems to be the case for
> > 'gpio-brcmstb', since driver's calculations per base are aligned with
> > this code changes but when it is assigned every line name is taking
> > into account gpio bank's width variable... If the only "client" of
> > this code would be gpio-mt7621 (or those where base and width of the
> > banks is the same) I don't know if changing core code makes sense...
>
> As far as I understood the problem, the driver (either broadcom one or
> mediatek) uses one GPIO description from which it internally splits to
> a few GPIO chips. GPIO chips are kinda independent in that sense,
> correct? So, if you put the index / offset field per GPIO chip before
> creation, the problem is solved.  What did I miss?

Should be, yes. But my concern is about why the broadcom driver
calculate base as:

base = bank->id * MAX_GPIO_PER_BANK;

and then fill names using:

/*
 * Make sure to not index beyond the end of the number of descriptors
 * of the GPIO device.
 */
for (i = 0; i < bank->width; i++) {
 ...

It looks like each gpio chip is separated MAX_GPIO_PER_BANK but the
width of each of some of them may be different. So in my understanding
assume for example there are four banks with widths 32,32, 24, 32 and
if you want to provide friendly names for all of them, in the third
one you have to create empty strings until 32 or you will get wrong to
the starting of the fourth bank and the code is getting care of not
going out of index in the for loop and assign only those needed. So
technically you are providing 8 empty strings even though the width of
the third bank is only 24 which sounds also bad... But maybe I am
misunderstanding the code itself and I need a bit more sleep :)

Best regards,
    Sergio Paracuellos

>
> --
> With Best Regards,
> Andy Shevchenko