Message-ID: <CAMhs-H8QeRcY+EiCrN2Rw1A1-xDdpuDqr-knoQEAcQiTXvdBDw@mail.gmail.com>
Date:   Mon, 19 Jul 2021 10:31:12 +0200
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Gregory Fong <gregory.0xf0@...il.com>
Cc:     "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        René van Dorst <opensource@...rst.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        John Thomson <git@...nthomson.fastmail.com.au>,
        NeilBrown <neil@...wn.name>,
        Nicholas Mc Guire <hofrat@...dl.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to
 support multiple gpiochip baks per device

Hi Gregory,

Thanks for the feedback.

On Mon, Jul 19, 2021 at 9:57 AM Gregory Fong <gregory.0xf0@...il.com> wrote:
>
> Hi Sergio,
>
> On Thu, Jul 08, 2021 at 09:04:27AM +0200, Sergio Paracuellos wrote:
> > The default gpiolib-of implementation does not work with the multiple
> > gpiochip banks per device structure used for example by the gpio-mt7621
> > and gpio-brcmstb drivers. To fix these kind of situations driver code
> > is forced to fill the names to avoid the gpiolib code to set names
> > repeated along the banks. Instead of continue with that antipattern
> > fix the gpiolib core function to get expected behaviour for every
> > single situation adding a field 'offset' in the gpiochip structure.
> > Doing in this way, we can assume this offset will be zero for normal
> > driver code where only one gpiochip bank per device is used but
> > can be set explicitly in those drivers that really need more than
> > one gpiochip.
>
> This is a nice improvement, thanks for putting this together!  A few
> remarks below:
>
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@...il.com>
> > ---
> >  drivers/gpio/gpiolib.c      | 34 ++++++++++++++++++++++++++++------
> >  include/linux/gpio/driver.h |  4 ++++
> >  2 files changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 27c07108496d..f3f45b804542 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -382,11 +382,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;
>
> This case needs a descriptive warning message.  Silent failure to assign
> names here will leave someone confused about what they're doing wrong.

Ok, I will add something like "All line names are not defined for bank
X.". Or any other suggestion would be also ok :).

>
> >
> >       names = kcalloc(count, sizeof(*names), GFP_KERNEL);
> >       if (!names)
> > @@ -400,8 +405,25 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
> >               return ret;
> >       }
> >
> > +     /*
> > +      * When more that one gpiochip per device is used, 'count' can
> > +      * contain at most number gpiochips x chip->ngpio. We have to
> > +      * correctly distribute all defined lines taking into account
> > +      * chip->offset as starting point from where we will assign
> > +      * the names to pins from the 'names' array. Since property
> > +      * 'gpio-line-names' cannot contains gaps, we have to be sure
> > +      * we only assign those pins that really exists since chip->ngpio
> > +      * can be different of the chip->offset.
> > +      */
> > +     count = (count > chip->offset) ? count - chip->offset : count;
> > +     if (count > chip->ngpio) {
>
> In the multiple gpiochip case, if there are 3+ gpiochips this seems like
> it will yield an invalid warning. For example, if there are 3 gpiochips
> (banks 0, 1, and 2), and all gpios are given names in gpio-line-names,
> isn't this condition going to always evaluate to true for bank 1,
> resulting in an invalid warning?  In that case I would think setting
> count to chip->ngpio is the *expected* behavior.
>
> Since that's a "normal" behavior in the multiple gpiochip case, I'm not
> sure there's a simple way to detect an over-long gpio-line-names here
> in this function anymore.

Yes, in case of multiple chips with all lines names defined this
warning will be displayed but I wanted to maintain the warning for
normal cases and I was not able to find an easy way of distinc that
cases with those having multiple gpiochips internally. So I ended up
in "ok, will be displayed for those special cases and interpreted as
we are just assigning names within an offset along the gpiochips in
the device.". Any other suggestion of course is always welcome :)

Thanks,
    Sergio Paracuellos

>
> > +             dev_warn(&gdev->dev, "gpio-line-names is length %d but
> > should be at most length %d", +                        count,
> > chip->ngpio);
> > +             count = chip->ngpio; +  } + for (i = 0; i < count; i++)
> > -             gdev->descs[i].name = names[i]; +
> > gdev->descs[i].name = names[chip->offset + i];
> >
> >       kfree(names);
> >
> > [snip]
>
> Best regards,
> Gregory

Powered by blists - more mailing lists