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

[Sergio Paracuellos <sergio.paracuellos@gmail.com>]

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:
> > > > On Sat, Jul 3, 2021 at 1:32 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Sat, Jul 3, 2021 at 2:06 PM Sergio Paracuellos
> > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > On Fri, Jul 2, 2021 at 1:30 PM Sergio Paracuellos
> > > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > -               ret = devprop_gpiochip_set_names(gc);
> > > > > > +               ret = devprop_gpiochip_set_names(gc, 0);
> > > > >
> > > > > I had been expecting that this parameter would be in the field of the gpiochip.
> > > > >
> > > > > ...
> > > >
> > > > If doing it in that way is preferred, I have no problem at all. But in
> > > > that case I think there is no need for a new
> > > > 'devprop_gpiochip_set_names_base' and we can assume for all drivers to
> > > > be zero and if is set taking it into account directly in
> > > > devprop_gpiochip_set_names function? Is this what you mean by having
> > > > this field added there??
> >
> > 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...

Best regards,
    Sergio Paracuellos

>
> >
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 6e3c4d7a7d14..0587f46b7c22 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -380,10 +380,10 @@ static int devprop_gpiochip_set_names(struct
> > > gpio_chip *chip)
> > >                 return 0;
> > >
> > >         count = device_property_string_array_count(dev, "gpio-line-names");
> > > -       if (count < 0)
> >
> > > +       if (count < 0 || count <= chip->offset)
> >
> > Please, split it into two conditionals and add a comment to the second one.
>
> For sure I will do, thanks.
>
> >
> > >                 return 0;
> > >
> > > -       if (count > gdev->ngpio) {
> > > +       if (count > gdev->ngpio && chip->offset == 0) {
> > >                 dev_warn(&gdev->dev, "gpio-line-names is length %d but
> > > should be at most length %d",
> > >                          count, gdev->ngpio);
> > >                 count = gdev->ngpio;
> > > @@ -401,8 +401,9 @@ static int devprop_gpiochip_set_names(struct
> > > gpio_chip *chip)
> > >                 return ret;
> > >         }
> > >
> > > +       count = (chip->offset >= count) ? (chip->offset - count) : count;
> >
> > Too many parentheses.
>
> Ok, I will also change this.
>
> >
> > >         for (i = 0; i < count; i++)
> > > -               gdev->descs[i].name = names[i];
> > > +               gdev->descs[i].name = names[chip->offset + i];
> > >
> > >         kfree(names);
> > >
> > > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> > > index 4a7e295c3640..39e0786586f6 100644
> > > --- a/include/linux/gpio/driver.h
> > > +++ b/include/linux/gpio/driver.h
> > > @@ -312,6 +312,9 @@ struct gpio_irq_chip {
> > >   *     get rid of the static GPIO number space in the long run.
> > >   * @ngpio: the number of GPIOs handled by this controller; the last GPIO
> > >   *     handled is (base + ngpio - 1).
> > > + * @offset: when multiple gpio chips belong to the same device this
> > > + *     can be used as offset within the device so friendly names can
> > > + *     be properly assigned.
> > >   * @names: if set, must be an array of strings to use as alternative
> > >   *      names for the GPIOs in this chip. Any entry in the array
> > >   *      may be NULL if there is no alias for the GPIO, however the
> > > @@ -398,6 +401,7 @@ struct gpio_chip {
> > >
> > >         int                     base;
> > >         u16                     ngpio;
> > > +       int                     offset;
> >
> > u16 (as ngpio has that type)
> >
> > >         const char              *const *names;
> > >         bool                    can_sleep;
> > >
> > >
> > > Does this sound reasonable?
>
> So the gpiolib related patch updated code with your proposed changes
> looks as follows:
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6e3c4d7a7d14..0c773d9ef292 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -383,7 +383,18 @@ static int devprop_gpiochip_set_names(struct
> gpio_chip *chip)
>         if (count < 0)
>                 return 0;
>
> -       if (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;
> +
> +       if (count > gdev->ngpio && chip->offset == 0) {
>                 dev_warn(&gdev->dev, "gpio-line-names is length %d but
> should be at most length %d",
>                          count, gdev->ngpio);
>                 count = gdev->ngpio;
> @@ -401,8 +412,9 @@ static int devprop_gpiochip_set_names(struct
> gpio_chip *chip)
>                 return ret;
>         }
>
> +       count = (chip->offset >= count) ? chip->offset - count : count;
>         for (i = 0; i < count; i++)
> -               gdev->descs[i].name = names[i];
> +               gdev->descs[i].name = names[chip->offset + i];
>
>         kfree(names);
>
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 4a7e295c3640..7a77f533d8fe 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -312,6 +312,9 @@ struct gpio_irq_chip {
>   *     get rid of the static GPIO number space in the long run.
>   * @ngpio: the number of GPIOs handled by this controller; the last GPIO
>   *     handled is (base + ngpio - 1).
> + * @offset: when multiple gpio chips belong to the same device this
> + *     can be used as offset within the device so friendly names can
> + *     be properly assigned.
>   * @names: if set, must be an array of strings to use as alternative
>   *      names for the GPIOs in this chip. Any entry in the array
>   *      may be NULL if there is no alias for the GPIO, however the
> @@ -398,6 +401,7 @@ struct gpio_chip {
>
>         int                     base;
>         u16                     ngpio;
> +       u16                     offset;
>         const char              *const *names;
>         bool                    can_sleep;
>
> Best regards,
>     Sergio Paracuellos
> >
> > > > > > The problem I see with this approach is that
> > > > > > 'devprop_gpiochip_set_names' already trusts in gpio_device already
> > > > > > created and this happens in 'gpiochip_add_data_with_key'. So doing in
> > > > > > this way force "broken drivers" to call this new
> > > > > > 'devprop_gpiochip_set_names_base' function after
> > > > > > 'devm_gpiochip_add_data' is called so the core code has already set up
> > > > > > the friendly names repeated for all gpio chip banks and the approach
> > > > > > would be to "overwrite" those in a second pass which sounds more like
> > > > > > a hack than a solution.
> > > > > >
> > > > > > But maybe I am missing something in what you were pointing out here.
> > > > >
> > > > > Would the above work?
> >
> > --
> > With Best Regards,
> > Andy Shevchenko