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: Sat, 3 Jul 2021 14:51:25 +0200 [thread overview]
Message-ID: <CAMhs-H8Y0txwcqRTxpsB_GEoOYbhHWO81EANMxMSybzWPS=HTA@mail.gmail.com> (raw)
In-Reply-To: <CAMhs-H_e2U7nUav8h+Q0w-aZXvD6VM6wpg857WbFgw6x3z1ufA@mail.gmail.com>
On Sat, Jul 3, 2021 at 2:05 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Andy,
>
> 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??
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;
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)
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;
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;
const char *const *names;
bool can_sleep;
Does this sound reasonable?
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?
>
> The following works for me, but the overwritten part of the
> 'gdev->descs[i].name' because this has been already called once by the
> core code is hacky and dirty, IMHO :)
>
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 4a7e295c3640..ad145ab0794c 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -537,6 +537,8 @@ extern int gpiochip_add_data_with_key(struct
> gpio_chip *gc, void *data,
> devm_gpiochip_add_data_with_key(dev, gc, data, NULL, NULL)
> #endif /* CONFIG_LOCKDEP */
>
> +extern int devprop_gpiochip_set_names_base(struct gpio_chip *gc, int base);
> +
> static inline int gpiochip_add(struct gpio_chip *gc)
> {
> return gpiochip_add_data(gc, NULL);
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6e3c4d7a7d14..f9942d5d2f2a 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -361,13 +361,14 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc)
> /*
> * devprop_gpiochip_set_names - Set GPIO line names using device properties
> * @chip: GPIO chip whose lines should be named, if possible
> + * @base: starting index in names array to start copying from
> *
> * Looks for device property "gpio-line-names" and if it exists assigns
> * GPIO line names for the chip. The memory allocated for the assigned
> * names belong to the underlying software node and should not be released
> * by the caller.
> */
> -static int devprop_gpiochip_set_names(struct gpio_chip *chip)
> +static int devprop_gpiochip_set_names(struct gpio_chip *chip, int base)
> {
> struct gpio_device *gdev = chip->gpiodev;
> struct device *dev = chip->parent;
> @@ -383,12 +384,18 @@ static int devprop_gpiochip_set_names(struct
> gpio_chip *chip)
> if (count < 0)
> return 0;
>
> - if (count > gdev->ngpio) {
> + if (count > gdev->ngpio && base == 0) {
> dev_warn(&gdev->dev, "gpio-line-names is length %d but
> should be at most length %d",
> count, gdev->ngpio);
> count = gdev->ngpio;
> }
>
> + if (count <= base) {
> + for (i = 0; i < count; i++)
> + gdev->descs[i].name = "";
> + return 0;
> + }
> +
> names = kcalloc(count, sizeof(*names), GFP_KERNEL);
> if (!names)
> return -ENOMEM;
> @@ -401,14 +408,21 @@ static int devprop_gpiochip_set_names(struct
> gpio_chip *chip)
> return ret;
> }
>
> + count = (base >= count) ? (base - count) : count;
> for (i = 0; i < count; i++)
> - gdev->descs[i].name = names[i];
> + gdev->descs[i].name = names[base + i];
>
> kfree(names);
>
> return 0;
> }
>
> +int devprop_gpiochip_set_names_base(struct gpio_chip *gc, int base)
> +{
> + return devprop_gpiochip_set_names(gc, base);
> +}
> +EXPORT_SYMBOL_GPL(devprop_gpiochip_set_names_base);
> +
> static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
> {
> unsigned long *p;
> @@ -684,7 +698,7 @@ int gpiochip_add_data_with_key(struct gpio_chip
> *gc, void *data,
> if (gc->names)
> ret = gpiochip_set_desc_names(gc);
> else
> - ret = devprop_gpiochip_set_names(gc);
> + ret = devprop_gpiochip_set_names(gc, 0);
> if (ret)
> goto err_remove_from_list;
>
> diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c
> index 82fb20dca53a..d4f19ab726b2 100644
> --- a/drivers/gpio/gpio-mt7621.c
> +++ b/drivers/gpio/gpio-mt7621.c
> @@ -282,6 +282,12 @@ mediatek_gpio_bank_probe(struct device *dev,
> return ret;
> }
>
> + ret = devprop_gpiochip_set_names_base(&rg->chip, bank * MTK_BANK_WIDTH);
> + if (ret) {
> + dev_err(dev, "Error setting line names for bank %d", bank);
> + return ret;
> + }
> +
> /* set polarity to low for all gpios */
> mtk_gpio_w32(rg, GPIO_REG_POL, 0);
>
> Best regards,
> Sergio Paracuellos
>
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
next prev parent reply other threads:[~2021-07-03 12:51 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 [this message]
2021-07-03 19:36 ` Andy Shevchenko
2021-07-04 5:57 ` Sergio Paracuellos
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-H8Y0txwcqRTxpsB_GEoOYbhHWO81EANMxMSybzWPS=HTA@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).