[PATCH 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device

[PATCH 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device

[Sergio Paracuellos]

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

[0]: https://lkml.org/lkml/2021/6/26/198

Sergio Paracuellos (3):
  gpiolib: convert 'devprop_gpiochip_set_names' to support multiple
    gpiochip baks 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      | 34 +++++++++++++++++++++++-----
 include/linux/gpio/driver.h |  4 ++++
 4 files changed, 34 insertions(+), 50 deletions(-)

-- 
2.25.1

[PATCH 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip baks per device

[Sergio Paracuellos]

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.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.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;
 
 	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) {
+		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);
 
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

[PATCH 2/3] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

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

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

[PATCH 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names'

[Sergio Paracuellos]

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.

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

Re: [PATCH 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device

[Andy Shevchenko]

On Thu, Jul 8, 2021 at 10:04 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> 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.

Thanks for doing this!
LGTM,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Best regards,
>     Sergio Paracuellos
>
> [0]: https://lkml.org/lkml/2021/6/26/198
>
> Sergio Paracuellos (3):
>   gpiolib: convert 'devprop_gpiochip_set_names' to support multiple
>     gpiochip baks 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      | 34 +++++++++++++++++++++++-----
>  include/linux/gpio/driver.h |  4 ++++
>  4 files changed, 34 insertions(+), 50 deletions(-)
>
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device

[Sergio Paracuellos]

On Thu, Jul 8, 2021 at 10:02 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jul 8, 2021 at 10:04 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > 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.
>
> Thanks for doing this!
> LGTM,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks for your feedback and support!

>
> > Best regards,
> >     Sergio Paracuellos
> >
> > [0]: https://lkml.org/lkml/2021/6/26/198
> >
> > Sergio Paracuellos (3):
> >   gpiolib: convert 'devprop_gpiochip_set_names' to support multiple
> >     gpiochip baks 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      | 34 +++++++++++++++++++++++-----
> >  include/linux/gpio/driver.h |  4 ++++
> >  4 files changed, 34 insertions(+), 50 deletions(-)
> >
> > --
> > 2.25.1
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip baks per device

[Gregory Fong]

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@gmail.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.

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

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

Re: [PATCH 3/3] gpio: brcmstb: remove custom 'brcmstb_gpio_set_names'

[Gregory Fong]

On Thu, Jul 08, 2021 at 09:04:29AM +0200, Sergio Paracuellos 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.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

After the first patch is cleaned up,

Acked-by: Gregory Fong <gregory.0xf0@gmail.com>

Best regards,
Gregory

Re: [PATCH 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip baks per device

[Sergio Paracuellos]

Hi Gregory,

Thanks for the feedback.

On Mon, Jul 19, 2021 at 9:57 AM Gregory Fong <gregory.0xf0@gmail.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@gmail.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

Re: [PATCH 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device

[Sergio Paracuellos]

On Thu, Jul 8, 2021 at 9:04 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> 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
>
> [0]: https://lkml.org/lkml/2021/6/26/198
>
> Sergio Paracuellos (3):
>   gpiolib: convert 'devprop_gpiochip_set_names' to support multiple
>     gpiochip baks 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      | 34 +++++++++++++++++++++++-----
>  include/linux/gpio/driver.h |  4 ++++
>  4 files changed, 34 insertions(+), 50 deletions(-)

Hi!

Linus, Bartosz, any comments on this series?

Best regards,
    Sergio Paracuellos
>
> --
> 2.25.1
>

Re: [PATCH 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip baks per device

[Gregory Fong]

On Mon, Jul 19, 2021 at 1:31 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> On Mon, Jul 19, 2021 at 9:57 AM Gregory Fong <gregory.0xf0@gmail.com> wrote:
> > On Thu, Jul 08, 2021 at 09:04:27AM +0200, Sergio Paracuellos wrote:
> [snip]
> > > 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 :).

I'd like this to name the gpio-line-names property like the other
warning does.  Not sure there's a good way to generically determine
what the bank number is, since some driver might not populate at
regular offsets.

We do have the count and offset available, so maybe something like
"gpio-line-names too short (length <count>), cannot map names for the
gpiochip at offset <offset>"?

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

There are millions of parts with this gpio hardware in the wild; I'd
much prefer we didn't issue a warning for every chip using it.

If there is a good way to detect the multiple gpiochip case, then that
could be used to determine whether to issue the warning.  Otherwise,
it seems like it would be better to remove the warning altogether.

Best regards,
Gregory

Re: [PATCH 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device

[Bartosz Golaszewski]

On Tue, Jul 27, 2021 at 8:02 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> On Thu, Jul 8, 2021 at 9:04 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > 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
> >
> > [0]: https://lkml.org/lkml/2021/6/26/198
> >
> > Sergio Paracuellos (3):
> >   gpiolib: convert 'devprop_gpiochip_set_names' to support multiple
> >     gpiochip baks 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      | 34 +++++++++++++++++++++++-----
> >  include/linux/gpio/driver.h |  4 ++++
> >  4 files changed, 34 insertions(+), 50 deletions(-)
>
> Hi!
>
> Linus, Bartosz, any comments on this series?
>

Looks good, but I was thinking you were going to address Gregory's
points first and resend a v2?

Bartosz

Re: [PATCH 0/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip per device

[Sergio Paracuellos]

Hi,

On Tue, Jul 27, 2021 at 1:35 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> On Tue, Jul 27, 2021 at 8:02 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > On Thu, Jul 8, 2021 at 9:04 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > >
> > > 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
> > >
> > > [0]: https://lkml.org/lkml/2021/6/26/198
> > >
> > > Sergio Paracuellos (3):
> > >   gpiolib: convert 'devprop_gpiochip_set_names' to support multiple
> > >     gpiochip baks 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      | 34 +++++++++++++++++++++++-----
> > >  include/linux/gpio/driver.h |  4 ++++
> > >  4 files changed, 34 insertions(+), 50 deletions(-)
> >
> > Hi!
> >
> > Linus, Bartosz, any comments on this series?
> >
>
> Looks good, but I was thinking you were going to address Gregory's
> points first and resend a v2?

I was waiting for your opinion about the last warning stuff Gregory
commented on PATCH 1 since it is not a good way to distinguish between
normal (1 gpiochip) and special cases (multiple gpiochips). I think
since that can happen normally we can just remove the warning. With
that clear I properly fix it up and resend v2.

>
> Bartosz

Best regards,
    Sergio Paracuellos

Re: [PATCH 1/3] gpiolib: convert 'devprop_gpiochip_set_names' to support multiple gpiochip baks per device

[Sergio Paracuellos]

Hi Gregory,

On Tue, Jul 27, 2021 at 9:39 AM Gregory Fong <gregory.0xf0@gmail.com> wrote:
>
> On Mon, Jul 19, 2021 at 1:31 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Mon, Jul 19, 2021 at 9:57 AM Gregory Fong <gregory.0xf0@gmail.com> wrote:
> > > On Thu, Jul 08, 2021 at 09:04:27AM +0200, Sergio Paracuellos wrote:
> > [snip]
> > > > 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 :).
>
> I'd like this to name the gpio-line-names property like the other
> warning does.  Not sure there's a good way to generically determine
> what the bank number is, since some driver might not populate at
> regular offsets.
>
> We do have the count and offset available, so maybe something like
> "gpio-line-names too short (length <count>), cannot map names for the
> gpiochip at offset <offset>"?

Ok, sounds ok to me to have this warning in this way, thanks.
>
> >
> > >
> > > >
> > > >       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 :)
>
> There are millions of parts with this gpio hardware in the wild; I'd
> much prefer we didn't issue a warning for every chip using it.
>
> If there is a good way to detect the multiple gpiochip case, then that
> could be used to determine whether to issue the warning.  Otherwise,
> it seems like it would be better to remove the warning altogether.

I think since this might be kind of "normal scenery" now, and there is
not an easy way to distinguish between normal and special cases (it
is?) warning can be safely removed.

>
> Best regards,
> Gregory

Best regards,
    Sergio Paracuellos