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

[Sergio Paracuellos]

On Sun, Jul 4, 2021 at 10:07 PM Sergio Paracuellos
<sergio.paracuellos@xxxxxxxxx> wrote:
>
> On Sun, Jul 4, 2021 at 1:36 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> >
> > 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.
>
> I see, so I guess I'll update my current patch to take this also into
> account so I can move the check against ngpio after count is
> calculated for both cases. Doing it that way should cover current
> behaviour, AFAICS.
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6e3c4d7a7d14..44321ac175d4 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -383,11 +383,16 @@ static int devprop_gpiochip_set_names(struct
> gpio_chip *chip)
>         if (count < 0)
>                 return 0;
>
> -       if (count > gdev->ngpio) {
> -               dev_warn(&gdev->dev, "gpio-line-names is length %d but
> should be at most length %d",
> -                        count, gdev->ngpio);
> -               count = gdev->ngpio;
> -       }
> +       /*
> +        * When offset is set in the driver side we assume the driver internally
> +        * is using more than one gpiochip per the same device. We have to stop
> +        * setting friendly names if the specified ones with 'gpio-line-names'
> +        * are less than the offset in the device itself. This means all the
> +        * lines are not present for every single pin within all the internal
> +        * gpiochips.
> +        */
> +       if (count <= chip->offset)
> +               return 0;
>
>         names = kcalloc(count, sizeof(*names), GFP_KERNEL);
>         if (!names)
> @@ -401,8 +406,15 @@ static int devprop_gpiochip_set_names(struct
> gpio_chip *chip)
>                 return ret;
>         }
>
> +       count = (chip->offset > count) ? chip->offset - count : count;
> +       if (count > gdev->ngpio) {
> +               dev_warn(&gdev->dev, "gpio-line-names is length %d but
> should be at most length %d",
> +                        count, gdev->ngpio);
> +               count = gdev->ngpio;
> +       }

I meant 'chip->ngpio' here in both if clause and assignment.

> +
>         for (i = 0; i < count; i++)
> -               gdev->descs[i].name = names[i];
> +               gdev->descs[i].name = names[chip->offset + i];
>
>         kfree(names);
>
> >
> > 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.
>
> ngpio should have that for the gpiochip that has just called this code, right?
>
> >
> > 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.
>
> Thanks, I will properly send this patchset hopefully during this week.
>
> Best regards,
>     Sergio Paracuellos
>
> >
> > > But maybe I am
> > > misunderstanding the code itself and I need a bit more sleep :)
> >
> > Also possible :-)
> >
> > --
> > With Best Regards,
> > Andy Shevchenko