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

[Andy Shevchenko]

On Sun, Jul 4, 2021 at 2:25 PM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
> 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...

While I might agree on this, it sounds quite well correct and should
be done that way in such cases. The fundamental fix would be (but will
never appear due to ABI backward compatibility) to allow gaps in the
DT property arrays.

The workaround may be the amount of lines per bank in another property
(gpio-ranges?). In either case the GPIO bindings and drivers that
split hardware per bank seems to me unaligned and that is the root
cause, but it seems it was the initial desire to have like this.

Anyway, I have an opinion that at some point either workaround or
other means will be enforced on the GPIO library level in the core
code and your approach seems a good first step towards that.

> But maybe I am
> misunderstanding the code itself and I need a bit more sleep :)

Also possible :-)

-- 
With Best Regards,
Andy Shevchenko