Date:   Sun, 4 Jul 2021 22:07:16 +0200
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        John Thomson <git@...nthomson.fastmail.com.au>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        NeilBrown <neil@...wn.name>,
        René van Dorst <opensource@...rst.com>,
        Nicholas Mc Guire <hofrat@...dl.org>
Subject: Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

On Sun, Jul 4, 2021 at 1:36 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Sun, Jul 4, 2021 at 2:25 PM Sergio Paracuellos
> <sergio.paracuellos@...il.com> wrote:
> > On Sun, Jul 4, 2021 at 12:05 PM Andy Shevchenko
> > <andy.shevchenko@...il.com> wrote:
> > > On Sun, Jul 4, 2021 at 11:06 AM Sergio Paracuellos
> > > <sergio.paracuellos@...il.com> wrote:
> > > > On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
> > > > <sergio.paracuellos@...il.com> wrote:
> > > > > On Sat, Jul 3, 2021 at 9:36 PM Andy Shevchenko
> > > > > <andy.shevchenko@...il.com> wrote:
> > > > > > On Sat, Jul 3, 2021 at 3:51 PM Sergio Paracuellos
> > > > > > <sergio.paracuellos@...il.com> wrote:
> > > > > > > On Sat, Jul 3, 2021 at 2:05 PM Sergio Paracuellos
> > > > > > > <sergio.paracuellos@...il.com> 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;
+       }
+
        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

Powered by blists - more mailing lists