lkml.org 
[lkml]   [2021]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2] gpio: mt7621: support gpio-line-names property
On Sun, Jul 4, 2021 at 10:07 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> On Sun, Jul 4, 2021 at 1:36 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sun, Jul 4, 2021 at 2:25 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Sun, Jul 4, 2021 at 12:05 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sun, Jul 4, 2021 at 11:06 AM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
> > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > On Sat, Jul 3, 2021 at 9:36 PM Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > > On Sat, Jul 3, 2021 at 3:51 PM Sergio Paracuellos
> > > > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > > > On Sat, Jul 3, 2021 at 2:05 PM Sergio Paracuellos
> > > > > > > > <sergio.paracuellos@gmail.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;
> + }

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

\
 
 \ /
  Last update: 2021-07-04 22:16    [W:0.113 / U:0.760 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site