linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device
@ 2021-07-27 14:48 Sergio Paracuellos
  2021-07-27 14:48 ` [PATCH v2 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks " Sergio Paracuellos
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2021-07-27 14:48 UTC (permalink / raw)
  To: linux-gpio
  Cc: linus.walleij, gregory.0xf0, bgolaszewski, f.fainelli,
	matthias.bgg, opensource, andy.shevchenko, git, neil, hofrat,
	linux-kernel

There are some unfortunate cases where the DT representation
of the device and the Linux internal representation differs.
Such drivers for devices are forced to implement a custom function
to avoid the core code 'devprop_gpiochip_set_names' to be executed
since in any other case every gpiochip inside will got repeated
names through its internal gpiochip banks. To avoid this antipattern
this changes are introduced trying to adapt core 'devprop_gpiochip_set_names'
to get a correct behaviour for every single situation.

This series introduces a new 'offset' field in the gpiochip structure
that can be used for those unfortunate drivers that must define multiple
gpiochips per device.

Drivers affected by this situation are also updated. These are
'gpio-mt7621' and 'gpio-brcmstb'.

Motivation for this series available at [0].

Thanks in advance for your feedback.

Best regards,
    Sergio Paracuellos

Changes in v2:
 - Address Gregory Fong comments in v1 of the series [1].
 - Collect Andy Shevchenko Reviewed-by for the series.
 - Collect Gregory Fong Acked-by for PATCH 3/3.

[0]: https://lkml.org/lkml/2021/6/26/198
[1]: https://lkml.org/lkml/2021/7/8/47

Sergio Paracuellos (3):
  gpiolib: convert 'devprop_gpiochip_set_names' to support multiple
    gpiochip banks per device
  gpio: mt7621: support gpio-line-names property
  gpio: brcmstb: remove custom 'brcmstb_gpio_set_names'

 drivers/gpio/gpio-brcmstb.c | 45 +------------------------------------
 drivers/gpio/gpio-mt7621.c  |  1 +
 drivers/gpio/gpiolib.c      | 33 ++++++++++++++++++++++-----
 include/linux/gpio/driver.h |  4 ++++
 4 files changed, 34 insertions(+), 49 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks per device
  2021-07-27 14:48 [PATCH v2 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device Sergio Paracuellos
@ 2021-07-27 14:48 ` Sergio Paracuellos
  2021-07-27 15:04   ` Andy Shevchenko
  2021-07-27 14:48 ` [PATCH v2 2/3] gpio: mt7621: support gpio-line-names property Sergio Paracuellos
  2021-07-27 14:48 ` [PATCH v2 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names' Sergio Paracuellos
  2 siblings, 1 reply; 8+ messages in thread
From: Sergio Paracuellos @ 2021-07-27 14:48 UTC (permalink / raw)
  To: linux-gpio
  Cc: linus.walleij, gregory.0xf0, bgolaszewski, f.fainelli,
	matthias.bgg, opensource, andy.shevchenko, git, neil, hofrat,
	linux-kernel

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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/gpio/gpiolib.c      | 33 ++++++++++++++++++++++++++++-----
 include/linux/gpio/driver.h |  4 ++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 27c07108496d..bdd96891e848 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -382,10 +382,19 @@ 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) {
+		dev_warn(&gdev->dev, "gpio-line-names too short (length %d) "
+			 "cannot map names for the gpiochip at offset %u\n",
+			 count, chip->offset);
+		return 0;
 	}
 
 	names = kcalloc(count, sizeof(*names), GFP_KERNEL);
@@ -400,8 +409,22 @@ 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)
+		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);
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 3a268781fcec..a0f9901dcae6 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;
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] gpio: mt7621: support gpio-line-names property
  2021-07-27 14:48 [PATCH v2 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device Sergio Paracuellos
  2021-07-27 14:48 ` [PATCH v2 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks " Sergio Paracuellos
@ 2021-07-27 14:48 ` Sergio Paracuellos
  2021-07-27 14:48 ` [PATCH v2 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names' Sergio Paracuellos
  2 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2021-07-27 14:48 UTC (permalink / raw)
  To: linux-gpio
  Cc: linus.walleij, gregory.0xf0, bgolaszewski, f.fainelli,
	matthias.bgg, opensource, andy.shevchenko, git, neil, hofrat,
	linux-kernel

This driver uses multiple gpiochip banks per device.
To support 'gpio-line-names' along the banks 'offset'
for each bank must be set explicitly.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/gpio/gpio-mt7621.c | 1 +
 1 file changed, 1 insertion(+)

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names'
  2021-07-27 14:48 [PATCH v2 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device Sergio Paracuellos
  2021-07-27 14:48 ` [PATCH v2 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks " Sergio Paracuellos
  2021-07-27 14:48 ` [PATCH v2 2/3] gpio: mt7621: support gpio-line-names property Sergio Paracuellos
@ 2021-07-27 14:48 ` Sergio Paracuellos
  2021-07-27 15:06   ` Andy Shevchenko
  2 siblings, 1 reply; 8+ messages in thread
From: Sergio Paracuellos @ 2021-07-27 14:48 UTC (permalink / raw)
  To: linux-gpio
  Cc: linus.walleij, gregory.0xf0, bgolaszewski, f.fainelli,
	matthias.bgg, opensource, andy.shevchenko, git, neil, hofrat,
	linux-kernel

Gpiolib core code has been updated to support setting
friendly names through properly 'gpio-line-names'.
Instead of redefine behaviour here to skip the core
to be executed, just properly assign the desired offset
per bank to get in the core the expected behaviour.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Gregory Fong <gregory.0xf0@gmail.com>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/gpio/gpio-brcmstb.c | 45 +------------------------------------
 1 file changed, 1 insertion(+), 44 deletions(-)

diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
index fcfc1a1f1a5c..a7275159052e 100644
--- a/drivers/gpio/gpio-brcmstb.c
+++ b/drivers/gpio/gpio-brcmstb.c
@@ -603,49 +603,6 @@ static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
 	.resume_noirq = brcmstb_gpio_resume,
 };
 
-static void brcmstb_gpio_set_names(struct device *dev,
-				   struct brcmstb_gpio_bank *bank)
-{
-	struct device_node *np = dev->of_node;
-	const char **names;
-	int nstrings, base;
-	unsigned int i;
-
-	base = bank->id * MAX_GPIO_PER_BANK;
-
-	nstrings = of_property_count_strings(np, "gpio-line-names");
-	if (nstrings <= base)
-		/* Line names not present */
-		return;
-
-	names = devm_kcalloc(dev, MAX_GPIO_PER_BANK, sizeof(*names),
-			     GFP_KERNEL);
-	if (!names)
-		return;
-
-	/*
-	 * Make sure to not index beyond the end of the number of descriptors
-	 * of the GPIO device.
-	 */
-	for (i = 0; i < bank->width; i++) {
-		const char *name;
-		int ret;
-
-		ret = of_property_read_string_index(np, "gpio-line-names",
-						    base + i, &name);
-		if (ret) {
-			if (ret != -ENODATA)
-				dev_err(dev, "unable to name line %d: %d\n",
-					base + i, ret);
-			break;
-		}
-		if (*name)
-			names[i] = name;
-	}
-
-	bank->gc.names = names;
-}
-
 static int brcmstb_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -759,6 +716,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		gc->of_xlate = brcmstb_gpio_of_xlate;
 		/* not all ngpio lines are valid, will use bank width later */
 		gc->ngpio = MAX_GPIO_PER_BANK;
+		gc->offset = bank->id * MAX_GPIO_PER_BANK;
 		if (priv->parent_irq > 0)
 			gc->to_irq = brcmstb_gpio_to_irq;
 
@@ -769,7 +727,6 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
 		need_wakeup_event |= !!__brcmstb_gpio_get_active_irqs(bank);
 		gc->write_reg(reg_base + GIO_MASK(bank->id), 0);
 
-		brcmstb_gpio_set_names(dev, bank);
 		err = gpiochip_add_data(gc, bank);
 		if (err) {
 			dev_err(dev, "Could not add gpiochip for bank %d\n",
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks per device
  2021-07-27 14:48 ` [PATCH v2 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks " Sergio Paracuellos
@ 2021-07-27 15:04   ` Andy Shevchenko
  2021-07-27 15:06     ` Sergio Paracuellos
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2021-07-27 15:04 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: open list:GPIO SUBSYSTEM, Linus Walleij, gregory.0xf0,
	Bartosz Golaszewski, Florian Fainelli, Matthias Brugger,
	René van Dorst, John Thomson, NeilBrown, Nicholas Mc Guire,
	Linux Kernel Mailing List

On Tue, Jul 27, 2021 at 5:48 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> 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.

...

> +               dev_warn(&gdev->dev, "gpio-line-names too short (length %d) "
> +                        "cannot map names for the gpiochip at offset %u\n",

Reflow this that string literal will be on one line (it's fine to be
over even 100).

> +                        count, chip->offset);

--
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names'
  2021-07-27 14:48 ` [PATCH v2 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names' Sergio Paracuellos
@ 2021-07-27 15:06   ` Andy Shevchenko
  2021-07-27 15:08     ` Sergio Paracuellos
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2021-07-27 15:06 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: open list:GPIO SUBSYSTEM, Linus Walleij, gregory.0xf0,
	Bartosz Golaszewski, Florian Fainelli, Matthias Brugger,
	René van Dorst, John Thomson, NeilBrown, Nicholas Mc Guire,
	Linux Kernel Mailing List

On Tue, Jul 27, 2021 at 5:48 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Gpiolib core code has been updated to support setting
> friendly names through properly 'gpio-line-names'.
> Instead of redefine behaviour here to skip the core
> to be executed, just properly assign the desired offset
> per bank to get in the core the expected behaviour.

Since it will be a v3, reflow commit messages to occupy a little bit
more available space (per line). Something ~72-75 characters per line
is good balance.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks per device
  2021-07-27 15:04   ` Andy Shevchenko
@ 2021-07-27 15:06     ` Sergio Paracuellos
  0 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2021-07-27 15:06 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, Linus Walleij, Gregory Fong,
	Bartosz Golaszewski, Florian Fainelli, Matthias Brugger,
	René van Dorst, John Thomson, NeilBrown, Nicholas Mc Guire,
	Linux Kernel Mailing List

On Tue, Jul 27, 2021 at 5:05 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jul 27, 2021 at 5:48 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> 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.
>
> ...
>
> > +               dev_warn(&gdev->dev, "gpio-line-names too short (length %d) "
> > +                        "cannot map names for the gpiochip at offset %u\n",
>
> Reflow this that string literal will be on one line (it's fine to be
> over even 100).

Ah, ok. Will fix this and send v3.

Thanks,
    Sergio Paracuellos

>
> > +                        count, chip->offset);
>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names'
  2021-07-27 15:06   ` Andy Shevchenko
@ 2021-07-27 15:08     ` Sergio Paracuellos
  0 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2021-07-27 15:08 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: open list:GPIO SUBSYSTEM, Linus Walleij, Gregory Fong,
	Bartosz Golaszewski, Florian Fainelli, Matthias Brugger,
	René van Dorst, John Thomson, NeilBrown, Nicholas Mc Guire,
	Linux Kernel Mailing List

On Tue, Jul 27, 2021 at 5:07 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jul 27, 2021 at 5:48 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > Gpiolib core code has been updated to support setting
> > friendly names through properly 'gpio-line-names'.
> > Instead of redefine behaviour here to skip the core
> > to be executed, just properly assign the desired offset
> > per bank to get in the core the expected behaviour.
>
> Since it will be a v3, reflow commit messages to occupy a little bit
> more available space (per line). Something ~72-75 characters per line
> is good balance.

Sure.

Thanks,
    Sergio Paracuellos
>
> --
> With Best Regards,
> Andy Shevchenko

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-07-27 15:09 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27 14:48 [PATCH v2 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device Sergio Paracuellos
2021-07-27 14:48 ` [PATCH v2 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip banks " Sergio Paracuellos
2021-07-27 15:04   ` Andy Shevchenko
2021-07-27 15:06     ` Sergio Paracuellos
2021-07-27 14:48 ` [PATCH v2 2/3] gpio: mt7621: support gpio-line-names property Sergio Paracuellos
2021-07-27 14:48 ` [PATCH v2 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names' Sergio Paracuellos
2021-07-27 15:06   ` Andy Shevchenko
2021-07-27 15:08     ` Sergio Paracuellos

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