linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sergio Paracuellos <sergio.paracuellos@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "Linus Walleij" <linus.walleij@linaro.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	"Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"John Thomson" <git@johnthomson.fastmail.com.au>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	NeilBrown <neil@brown.name>,
	"René van Dorst" <opensource@vdorst.com>,
	"Nicholas Mc Guire" <hofrat@osadl.org>
Subject: Re: [PATCH v2] gpio: mt7621: support gpio-line-names property
Date: Sun, 4 Jul 2021 07:57:43 +0200	[thread overview]
Message-ID: <CAMhs-H8g8c047DSw2ObX7xS=YuPrXNRMecuV1TnKT--gnDdDOw@mail.gmail.com> (raw)
In-Reply-To: <CAHp75VfRYZn5uuPgQHJ5Hm3p3XVrfs=ReZXxEPm+dqLNb5QtGA@mail.gmail.com>

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.

>
> > 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

  reply	other threads:[~2021-07-04  5:58 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-26 16:18 [PATCH v2] gpio: mt7621: support gpio-line-names property Sergio Paracuellos
2021-06-27  9:33 ` Andy Shevchenko
2021-06-27  9:47   ` Sergio Paracuellos
2021-06-27 10:51     ` Andy Shevchenko
2021-06-27 10:56       ` Sergio Paracuellos
2021-06-27 13:00         ` Andy Shevchenko
2021-06-27 13:12           ` Sergio Paracuellos
2021-07-02  9:26             ` Andy Shevchenko
2021-07-02  9:40               ` Sergio Paracuellos
2021-07-02  9:56                 ` Andy Shevchenko
2021-07-02 10:18                 ` Linus Walleij
2021-07-02 11:30                   ` Sergio Paracuellos
2021-07-03 11:06                     ` Sergio Paracuellos
2021-07-03 11:32                       ` Andy Shevchenko
2021-07-03 12:05                         ` Sergio Paracuellos
2021-07-03 12:51                           ` Sergio Paracuellos
2021-07-03 19:36                             ` Andy Shevchenko
2021-07-04  5:57                               ` Sergio Paracuellos [this message]
2021-07-04  8:06                                 ` Sergio Paracuellos
2021-07-04 10:04                                   ` Andy Shevchenko
2021-07-04 11:25                                     ` Sergio Paracuellos
2021-07-04 11:35                                       ` Andy Shevchenko
2021-07-04 20:07                                         ` Sergio Paracuellos
2021-07-04 20:15                                           ` Sergio Paracuellos

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMhs-H8g8c047DSw2ObX7xS=YuPrXNRMecuV1TnKT--gnDdDOw@mail.gmail.com' \
    --to=sergio.paracuellos@gmail.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=git@johnthomson.fastmail.com.au \
    --cc=hofrat@osadl.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthias.bgg@gmail.com \
    --cc=neil@brown.name \
    --cc=opensource@vdorst.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).