[PATCH v2] gpio: mt7621: support gpio-line-names property

[PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

The default handling of the gpio-line-names property by the
gpiolib-of implementation does not work with the multiple
gpiochip banks per device structure used by the gpio-mt7621
driver.

This commit adds driver level support for the device tree
property so that GPIO lines can be assigned friendly names.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
Hi,

This driver has three gpiochips with 32 gpios each. Core implmentation
got gpio's repeated along each gpio chip if chip.names is not assigned.
To avoid this behaviour driver will set this names as empty or
with desired friendly line names. Consider the following sample with
minimal entries for the first chip with this patch changes applied:

&gpio {
    gpio-line-names = "", "", "", "",
                      "", "", "SFP LOS", "extcon port5 PoE compat",
                      "SFP module def0", "LED blue SFP", "SFP tx disable", "",
                      "switch USB power", "mode", "", "buzzer",
                      "LED blue pwr", "switch port5 PoE out", "reset";
};

gpioinfo
gpiochip0 - 32 lines:
  line   0:      unnamed       unused  output  active-high
  line   1:      unnamed       unused   input  active-high
  line   2:      unnamed       unused   input  active-high
  line   3:      unnamed       unused   input  active-high
  line   4:      unnamed       unused   input  active-high
  line   5:      unnamed       unused   input  active-high
  line   6:    "SFP LOS"        "los"   input  active-high [used]
  line   7: "extcon port5 PoE compat" unused input active-high
  line   8: "SFP module def0" "mod-def0" input active-low [used]
  line   9: "LED blue SFP" "blue:sfp" output active-high [used]
  line  10: "SFP tx disable" "tx-disable" output active-high [used]
  line  11:      unnamed       unused  output  active-high
  line  12: "switch USB power" "usb_power" output active-high [used]
  line  13:       "mode"       "mode"   input  active-high [used]
  line  14:      unnamed       unused   input  active-high
  line  15:     "buzzer"     "buzzer"  output  active-high [used]
  line  16: "LED blue pwr" "blue:pwr" output active-high [used]
  line  17: "switch port5 PoE out" "sysfs" input active-high [used]
  line  18:      "reset"      "reset"   input  active-high [used]
  line  19:      unnamed       unused   input  active-high
  line  20:      unnamed       unused   input  active-high
  line  21:      unnamed       unused   input  active-high
  line  22:      unnamed       unused   input  active-high
  line  23:      unnamed       unused   input  active-high
  line  24:      unnamed       unused   input  active-high
  line  25:      unnamed       unused   input  active-high
  line  26:      unnamed       unused   input  active-high
  line  27:      unnamed       unused   input  active-high
  line  28:      unnamed       unused   input  active-high
  line  29:      unnamed       unused   input  active-high
  line  30:      unnamed       unused   input  active-high
  line  31:      unnamed       unused   input  active-high 
gpiochip1 - 32 lines:
  line   0:      unnamed       unused   input  active-high
  line   1:      unnamed       unused   input  active-high 
  ...
  line  31:      unnamed       unused   input  active-high 
gpiochip2 - 32 lines:
  line   0:      unnamed       unused   input  active-high
  line   1:      unnamed       unused   input  active-high 
  ...
  line  31:      unnamed       unused   input  active-high 

To avoid gpiochip1 and gpiochip2 entries repeated with this
minimal lines definition change, we assign empty reserved
'names' in driver code.

Note that we also don't want to to prevent the driver from
succeeding at probe due to an error in the gpio-line-names
property and the ENODATA error is considered a valid result
to terminate any further labeling so there is no need for
an error message in that case. Other error results are
unexpected so an error message indicating the consequence
of the error is appropriate here.

Changes in v2:
 - 'mediatek_gpio_set_names' returns -ENOMEM in case of OOM.
 - Caller checks 'mediatek_gpio_set_names' return value.

Thanks in advance for your time.

Best regards,
    Sergio Paracuellos

 drivers/gpio/gpio-mt7621.c | 45 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c
index 82fb20dca53a..7fc9e3c1ce10 100644
--- a/drivers/gpio/gpio-mt7621.c
+++ b/drivers/gpio/gpio-mt7621.c
@@ -206,6 +206,47 @@ mediatek_gpio_xlate(struct gpio_chip *chip,
 	return gpio % MTK_BANK_WIDTH;
 }
 
+static int
+mediatek_gpio_set_names(struct device *dev, struct mtk_gc *rg)
+{
+	struct device_node *np = dev->of_node;
+	const char **names;
+	int nstrings, base;
+	unsigned int i;
+
+	names = devm_kcalloc(dev, MTK_BANK_WIDTH, sizeof(*names),
+			     GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	base = rg->bank * MTK_BANK_WIDTH;
+	nstrings = of_property_count_strings(np, "gpio-line-names");
+	if (nstrings <= base)
+		goto assign_names;
+
+	for (i = 0; i < MTK_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;
+	}
+
+assign_names:
+	rg->chip.names = names;
+
+	return 0;
+}
+
 static int
 mediatek_gpio_bank_probe(struct device *dev,
 			 struct device_node *node, int bank)
@@ -241,6 +282,10 @@ mediatek_gpio_bank_probe(struct device *dev,
 	if (!rg->chip.label)
 		return -ENOMEM;
 
+	ret = mediatek_gpio_set_names(dev, rg);
+	if (ret)
+		return ret;
+
 	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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

On Sat, Jun 26, 2021 at 7:18 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> The default handling of the gpio-line-names property by the
> gpiolib-of implementation does not work with the multiple
> gpiochip banks per device structure used by the gpio-mt7621
> driver.
>
> This commit adds driver level support for the device tree
> property so that GPIO lines can be assigned friendly names.



> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
> Hi,
>
> This driver has three gpiochips with 32 gpios each. Core implementation

implementation


> got gpio's repeated along each gpio chip if chip.names is not assigned.
> To avoid this behaviour driver will set this names as empty or

the driver
these names

> with desired friendly line names. Consider the following sample with
> minimal entries for the first chip with this patch changes applied:

The same comment as per v1:

Any idea why it's not a duplicate of
https://elixir.bootlin.com/linux/v5.13-rc7/C/ident/devprop_gpiochip_set_names,
and why the latter is not called in your case?


--
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

Hi Andy,

On Sun, Jun 27, 2021 at 11:33 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Jun 26, 2021 at 7:18 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > The default handling of the gpio-line-names property by the
> > gpiolib-of implementation does not work with the multiple
> > gpiochip banks per device structure used by the gpio-mt7621
> > driver.
> >
> > This commit adds driver level support for the device tree
> > property so that GPIO lines can be assigned friendly names.
>
>
>
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> > Hi,
> >
> > This driver has three gpiochips with 32 gpios each. Core implementation
>
> implementation
>
>
> > got gpio's repeated along each gpio chip if chip.names is not assigned.
> > To avoid this behaviour driver will set this names as empty or
>
> the driver
> these names
>
> > with desired friendly line names. Consider the following sample with
> > minimal entries for the first chip with this patch changes applied:
>
> The same comment as per v1:
>
> Any idea why it's not a duplicate of
> https://elixir.bootlin.com/linux/v5.13-rc7/C/ident/devprop_gpiochip_set_names,
> and why the latter is not called in your case?

The core properly calls this function but not in the way expected.
This driver implements three banks of 32 gpios each internally using
one gpiochip per bank, all of them in the same device. So the core
code you are pointing out here duplicates the same names along the
three gpiochips which is not the expected behaviour. So implementing
in this way and setting names at least reserved avoids the core code
to be run and also avoids the duplication getting expected behaviour
for all the banks and each line friendly name.

Best regards,
    Sergio Paracuellos

>
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

On Sun, Jun 27, 2021 at 12:47 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> On Sun, Jun 27, 2021 at 11:33 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sat, Jun 26, 2021 at 7:18 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > >
> > > The default handling of the gpio-line-names property by the
> > > gpiolib-of implementation does not work with the multiple
> > > gpiochip banks per device structure used by the gpio-mt7621
> > > driver.
> > >
> > > This commit adds driver level support for the device tree
> > > property so that GPIO lines can be assigned friendly names.

> > > This driver has three gpiochips with 32 gpios each. Core implementation
> >
> > implementation
> >
> >
> > > got gpio's repeated along each gpio chip if chip.names is not assigned.
> > > To avoid this behaviour driver will set this names as empty or
> >
> > the driver
> > these names
> >
> > > with desired friendly line names. Consider the following sample with
> > > minimal entries for the first chip with this patch changes applied:
> >
> > The same comment as per v1:
> >
> > Any idea why it's not a duplicate of
> > https://elixir.bootlin.com/linux/v5.13-rc7/C/ident/devprop_gpiochip_set_names,
> > and why the latter is not called in your case?
>
> The core properly calls this function but not in the way expected.
> This driver implements three banks of 32 gpios each internally using
> one gpiochip per bank, all of them in the same device. So the core
> code you are pointing out here duplicates the same names along the
> three gpiochips which is not the expected behaviour. So implementing
> in this way and setting names at least reserved avoids the core code
> to be run and also avoids the duplication getting expected behaviour
> for all the banks and each line friendly name.

Isn't it the problem of how we supply fwnode in that case?
Another possibility is to fix DT (although I'm not sure it's now possible).

Have you considered the above?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

On Sun, Jun 27, 2021 at 12:51 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 27, 2021 at 12:47 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Sun, Jun 27, 2021 at 11:33 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Sat, Jun 26, 2021 at 7:18 PM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > >
> > > > The default handling of the gpio-line-names property by the
> > > > gpiolib-of implementation does not work with the multiple
> > > > gpiochip banks per device structure used by the gpio-mt7621
> > > > driver.
> > > >
> > > > This commit adds driver level support for the device tree
> > > > property so that GPIO lines can be assigned friendly names.
>
> > > > This driver has three gpiochips with 32 gpios each. Core implementation
> > >
> > > implementation
> > >
> > >
> > > > got gpio's repeated along each gpio chip if chip.names is not assigned.
> > > > To avoid this behaviour driver will set this names as empty or
> > >
> > > the driver
> > > these names
> > >
> > > > with desired friendly line names. Consider the following sample with
> > > > minimal entries for the first chip with this patch changes applied:
> > >
> > > The same comment as per v1:
> > >
> > > Any idea why it's not a duplicate of
> > > https://elixir.bootlin.com/linux/v5.13-rc7/C/ident/devprop_gpiochip_set_names,
> > > and why the latter is not called in your case?
> >
> > The core properly calls this function but not in the way expected.
> > This driver implements three banks of 32 gpios each internally using
> > one gpiochip per bank, all of them in the same device. So the core
> > code you are pointing out here duplicates the same names along the
> > three gpiochips which is not the expected behaviour. So implementing
> > in this way and setting names at least reserved avoids the core code
> > to be run and also avoids the duplication getting expected behaviour
> > for all the banks and each line friendly name.
>
> Isn't it the problem of how we supply fwnode in that case?
> Another possibility is to fix DT (although I'm not sure it's now possible).

Since the fwnode is the same for all banks of the same device, each bank
repeats the first MTK_BANK_WIDTH label names in each bank.

This commit populates the gc.names member of each bank from the
device-tree node within the driver. This overrides the default behavior
since devprop_gpiochip_set_names() will only be called if names is NULL.

Best regards,
    Sergio Paracuellos

>
> Have you considered the above?
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

On Sun, Jun 27, 2021 at 1:56 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> On Sun, Jun 27, 2021 at 12:51 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jun 27, 2021 at 12:47 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Sun, Jun 27, 2021 at 11:33 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sat, Jun 26, 2021 at 7:18 PM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:
> > > > >
> > > > > The default handling of the gpio-line-names property by the
> > > > > gpiolib-of implementation does not work with the multiple
> > > > > gpiochip banks per device structure used by the gpio-mt7621
> > > > > driver.
> > > > >
> > > > > This commit adds driver level support for the device tree
> > > > > property so that GPIO lines can be assigned friendly names.
> >
> > > > > This driver has three gpiochips with 32 gpios each. Core implementation
> > > >
> > > > implementation
> > > >
> > > >
> > > > > got gpio's repeated along each gpio chip if chip.names is not assigned.
> > > > > To avoid this behaviour driver will set this names as empty or
> > > >
> > > > the driver
> > > > these names
> > > >
> > > > > with desired friendly line names. Consider the following sample with
> > > > > minimal entries for the first chip with this patch changes applied:
> > > >
> > > > The same comment as per v1:
> > > >
> > > > Any idea why it's not a duplicate of
> > > > https://elixir.bootlin.com/linux/v5.13-rc7/C/ident/devprop_gpiochip_set_names,
> > > > and why the latter is not called in your case?
> > >
> > > The core properly calls this function but not in the way expected.
> > > This driver implements three banks of 32 gpios each internally using
> > > one gpiochip per bank, all of them in the same device. So the core
> > > code you are pointing out here duplicates the same names along the
> > > three gpiochips which is not the expected behaviour. So implementing
> > > in this way and setting names at least reserved avoids the core code
> > > to be run and also avoids the duplication getting expected behaviour
> > > for all the banks and each line friendly name.
> >
> > Isn't it the problem of how we supply fwnode in that case?
> > Another possibility is to fix DT (although I'm not sure it's now possible).
>
> Since the fwnode is the same for all banks of the same device, each bank
> repeats the first MTK_BANK_WIDTH label names in each bank.

Can you point out the DT in question?

> This commit populates the gc.names member of each bank from the
> device-tree node within the driver. This overrides the default behavior
> since devprop_gpiochip_set_names() will only be called if names is NULL.

I believe this commit is not needed in the proposed (i.e. duplication) shape.
The fwnode supports primary and secondary ones. Thus, we may create a
pair of fwnodes when they will unify properties per device with
properties per child together (child is primary and device, i.e.
parent, is secondary).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

Hi Andy,

On Sun, Jun 27, 2021 at 3:01 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 27, 2021 at 1:56 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Sun, Jun 27, 2021 at 12:51 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Jun 27, 2021 at 12:47 PM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > > On Sun, Jun 27, 2021 at 11:33 AM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Sat, Jun 26, 2021 at 7:18 PM Sergio Paracuellos
> > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > >
> > > > > > The default handling of the gpio-line-names property by the
> > > > > > gpiolib-of implementation does not work with the multiple
> > > > > > gpiochip banks per device structure used by the gpio-mt7621
> > > > > > driver.
> > > > > >
> > > > > > This commit adds driver level support for the device tree
> > > > > > property so that GPIO lines can be assigned friendly names.
> > >
> > > > > > This driver has three gpiochips with 32 gpios each. Core implementation
> > > > >
> > > > > implementation
> > > > >
> > > > >
> > > > > > got gpio's repeated along each gpio chip if chip.names is not assigned.
> > > > > > To avoid this behaviour driver will set this names as empty or
> > > > >
> > > > > the driver
> > > > > these names
> > > > >
> > > > > > with desired friendly line names. Consider the following sample with
> > > > > > minimal entries for the first chip with this patch changes applied:
> > > > >
> > > > > The same comment as per v1:
> > > > >
> > > > > Any idea why it's not a duplicate of
> > > > > https://elixir.bootlin.com/linux/v5.13-rc7/C/ident/devprop_gpiochip_set_names,
> > > > > and why the latter is not called in your case?
> > > >
> > > > The core properly calls this function but not in the way expected.
> > > > This driver implements three banks of 32 gpios each internally using
> > > > one gpiochip per bank, all of them in the same device. So the core
> > > > code you are pointing out here duplicates the same names along the
> > > > three gpiochips which is not the expected behaviour. So implementing
> > > > in this way and setting names at least reserved avoids the core code
> > > > to be run and also avoids the duplication getting expected behaviour
> > > > for all the banks and each line friendly name.
> > >
> > > Isn't it the problem of how we supply fwnode in that case?
> > > Another possibility is to fix DT (although I'm not sure it's now possible).
> >
> > Since the fwnode is the same for all banks of the same device, each bank
> > repeats the first MTK_BANK_WIDTH label names in each bank.
>
> Can you point out the DT in question?

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-next

Gpio node:

 gpio: gpio@600 {
              #gpio-cells = <2>;
              #interrupt-cells = <2>;
              compatible = "mediatek,mt7621-gpio";
              gpio-controller;
              gpio-ranges = <&pinctrl 0 0 95>;
              interrupt-controller;
              reg = <0x600 0x100>;
              interrupt-parent = <&gic>;
              interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
   };

My overlay:

&gpio {
    gpio-line-names = "", "", "", "",
                      "", "", "SFP LOS", "extcon port5 PoE compat",
                      "SFP module def0", "LED blue SFP", "SFP tx disable", "",
                      "switch USB power", "mode", "", "buzzer",
                      "LED blue pwr", "switch port5 PoE out", "reset";
};



>
> > This commit populates the gc.names member of each bank from the
> > device-tree node within the driver. This overrides the default behavior
> > since devprop_gpiochip_set_names() will only be called if names is NULL.
>
> I believe this commit is not needed in the proposed (i.e. duplication) shape.
> The fwnode supports primary and secondary ones. Thus, we may create a
> pair of fwnodes when they will unify properties per device with
> properties per child together (child is primary and device, i.e.
> parent, is secondary).

There are no child nodes, all the stuff is in the same parent node
and, as I said, belongs to the same device but internally uses three
gpiochips. This case is pretty much the same as the following already
added commit for gpio-brcmstb:

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/gpio/gpio-brcmstb.c?id=5eefcaed501dd9e3933dbff58720244bd75ed90f

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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

On Sun, Jun 27, 2021 at 4:13 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> On Sun, Jun 27, 2021 at 3:01 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jun 27, 2021 at 1:56 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Sun, Jun 27, 2021 at 12:51 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sun, Jun 27, 2021 at 12:47 PM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > On Sun, Jun 27, 2021 at 11:33 AM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > On Sat, Jun 26, 2021 at 7:18 PM Sergio Paracuellos
> > > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > >
> > > > > > > The default handling of the gpio-line-names property by the
> > > > > > > gpiolib-of implementation does not work with the multiple
> > > > > > > gpiochip banks per device structure used by the gpio-mt7621
> > > > > > > driver.
> > > > > > >
> > > > > > > This commit adds driver level support for the device tree
> > > > > > > property so that GPIO lines can be assigned friendly names.
> > > >
> > > > > > > This driver has three gpiochips with 32 gpios each. Core implementation
> > > > > >
> > > > > > implementation
> > > > > >
> > > > > >
> > > > > > > got gpio's repeated along each gpio chip if chip.names is not assigned.
> > > > > > > To avoid this behaviour driver will set this names as empty or
> > > > > >
> > > > > > the driver
> > > > > > these names
> > > > > >
> > > > > > > with desired friendly line names. Consider the following sample with
> > > > > > > minimal entries for the first chip with this patch changes applied:
> > > > > >
> > > > > > The same comment as per v1:
> > > > > >
> > > > > > Any idea why it's not a duplicate of
> > > > > > https://elixir.bootlin.com/linux/v5.13-rc7/C/ident/devprop_gpiochip_set_names,
> > > > > > and why the latter is not called in your case?
> > > > >
> > > > > The core properly calls this function but not in the way expected.
> > > > > This driver implements three banks of 32 gpios each internally using
> > > > > one gpiochip per bank, all of them in the same device. So the core
> > > > > code you are pointing out here duplicates the same names along the
> > > > > three gpiochips which is not the expected behaviour. So implementing
> > > > > in this way and setting names at least reserved avoids the core code
> > > > > to be run and also avoids the duplication getting expected behaviour
> > > > > for all the banks and each line friendly name.
> > > >
> > > > Isn't it the problem of how we supply fwnode in that case?
> > > > Another possibility is to fix DT (although I'm not sure it's now possible).
> > >
> > > Since the fwnode is the same for all banks of the same device, each bank
> > > repeats the first MTK_BANK_WIDTH label names in each bank.
> >
> > Can you point out the DT in question?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-next
>
> Gpio node:
>
>  gpio: gpio@600 {
>               #gpio-cells = <2>;
>               #interrupt-cells = <2>;
>               compatible = "mediatek,mt7621-gpio";
>               gpio-controller;
>               gpio-ranges = <&pinctrl 0 0 95>;
>               interrupt-controller;
>               reg = <0x600 0x100>;
>               interrupt-parent = <&gic>;
>               interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
>    };
>
> My overlay:
>
> &gpio {
>     gpio-line-names = "", "", "", "",
>                       "", "", "SFP LOS", "extcon port5 PoE compat",
>                       "SFP module def0", "LED blue SFP", "SFP tx disable", "",
>                       "switch USB power", "mode", "", "buzzer",
>                       "LED blue pwr", "switch port5 PoE out", "reset";
> };
>
>
>
> >
> > > This commit populates the gc.names member of each bank from the
> > > device-tree node within the driver. This overrides the default behavior
> > > since devprop_gpiochip_set_names() will only be called if names is NULL.
> >
> > I believe this commit is not needed in the proposed (i.e. duplication) shape.
> > The fwnode supports primary and secondary ones. Thus, we may create a
> > pair of fwnodes when they will unify properties per device with
> > properties per child together (child is primary and device, i.e.
> > parent, is secondary).
>
> There are no child nodes, all the stuff is in the same parent node
> and, as I said, belongs to the same device but internally uses three
> gpiochips.

And it can't be split into three children in the overlay?
Let's assume it can't, then the GPIO library function should be
refactored in a way that it takes parameters like base index for the
names and tries to satisfy the caller.

> This case is pretty much the same as the following already
> added commit for gpio-brcmstb:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/gpio/gpio-brcmstb.c?id=5eefcaed501dd9e3933dbff58720244bd75ed90f

This should be fixed accordingly.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

Hi Andy,

On Fri, Jul 2, 2021 at 11:27 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jun 27, 2021 at 4:13 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Sun, Jun 27, 2021 at 3:01 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Jun 27, 2021 at 1:56 PM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > > On Sun, Jun 27, 2021 at 12:51 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Sun, Jun 27, 2021 at 12:47 PM Sergio Paracuellos
> > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > On Sun, Jun 27, 2021 at 11:33 AM Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com> wrote:
> > > > > > > On Sat, Jun 26, 2021 at 7:18 PM Sergio Paracuellos
> > > > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > > >
> > > > > > > > The default handling of the gpio-line-names property by the
> > > > > > > > gpiolib-of implementation does not work with the multiple
> > > > > > > > gpiochip banks per device structure used by the gpio-mt7621
> > > > > > > > driver.
> > > > > > > >
> > > > > > > > This commit adds driver level support for the device tree
> > > > > > > > property so that GPIO lines can be assigned friendly names.
> > > > >
> > > > > > > > This driver has three gpiochips with 32 gpios each. Core implementation
> > > > > > >
> > > > > > > implementation
> > > > > > >
> > > > > > >
> > > > > > > > got gpio's repeated along each gpio chip if chip.names is not assigned.
> > > > > > > > To avoid this behaviour driver will set this names as empty or
> > > > > > >
> > > > > > > the driver
> > > > > > > these names
> > > > > > >
> > > > > > > > with desired friendly line names. Consider the following sample with
> > > > > > > > minimal entries for the first chip with this patch changes applied:
> > > > > > >
> > > > > > > The same comment as per v1:
> > > > > > >
> > > > > > > Any idea why it's not a duplicate of
> > > > > > > https://elixir.bootlin.com/linux/v5.13-rc7/C/ident/devprop_gpiochip_set_names,
> > > > > > > and why the latter is not called in your case?
> > > > > >
> > > > > > The core properly calls this function but not in the way expected.
> > > > > > This driver implements three banks of 32 gpios each internally using
> > > > > > one gpiochip per bank, all of them in the same device. So the core
> > > > > > code you are pointing out here duplicates the same names along the
> > > > > > three gpiochips which is not the expected behaviour. So implementing
> > > > > > in this way and setting names at least reserved avoids the core code
> > > > > > to be run and also avoids the duplication getting expected behaviour
> > > > > > for all the banks and each line friendly name.
> > > > >
> > > > > Isn't it the problem of how we supply fwnode in that case?
> > > > > Another possibility is to fix DT (although I'm not sure it's now possible).
> > > >
> > > > Since the fwnode is the same for all banks of the same device, each bank
> > > > repeats the first MTK_BANK_WIDTH label names in each bank.
> > >
> > > Can you point out the DT in question?
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-next
> >
> > Gpio node:
> >
> >  gpio: gpio@600 {
> >               #gpio-cells = <2>;
> >               #interrupt-cells = <2>;
> >               compatible = "mediatek,mt7621-gpio";
> >               gpio-controller;
> >               gpio-ranges = <&pinctrl 0 0 95>;
> >               interrupt-controller;
> >               reg = <0x600 0x100>;
> >               interrupt-parent = <&gic>;
> >               interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
> >    };
> >
> > My overlay:
> >
> > &gpio {
> >     gpio-line-names = "", "", "", "",
> >                       "", "", "SFP LOS", "extcon port5 PoE compat",
> >                       "SFP module def0", "LED blue SFP", "SFP tx disable", "",
> >                       "switch USB power", "mode", "", "buzzer",
> >                       "LED blue pwr", "switch port5 PoE out", "reset";
> > };
> >
> >
> >
> > >
> > > > This commit populates the gc.names member of each bank from the
> > > > device-tree node within the driver. This overrides the default behavior
> > > > since devprop_gpiochip_set_names() will only be called if names is NULL.
> > >
> > > I believe this commit is not needed in the proposed (i.e. duplication) shape.
> > > The fwnode supports primary and secondary ones. Thus, we may create a
> > > pair of fwnodes when they will unify properties per device with
> > > properties per child together (child is primary and device, i.e.
> > > parent, is secondary).
> >
> > There are no child nodes, all the stuff is in the same parent node
> > and, as I said, belongs to the same device but internally uses three
> > gpiochips.
>
> And it can't be split into three children in the overlay?

Original code before this being mainlined was using three children and
I was told in the review that three children were not allowed:

See https://patchwork.ozlabs.org/project/linux-gpio/patch/1527924610-13135-3-git-send-email-sergio.paracuellos@gmail.com/#1932827

> Let's assume it can't, then the GPIO library function should be
> refactored in a way that it takes parameters like base index for the
> names and tries to satisfy the caller.

Bartosz, Linus, any thoughts on this?

>
> > This case is pretty much the same as the following already
> > added commit for gpio-brcmstb:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/gpio/gpio-brcmstb.c?id=5eefcaed501dd9e3933dbff58720244bd75ed90f
>
> This should be fixed accordingly.

Obviously, the treatment should be the same, yes :)

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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

On Fri, Jul 2, 2021 at 12:40 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> On Fri, Jul 2, 2021 at 11:27 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jun 27, 2021 at 4:13 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Sun, Jun 27, 2021 at 3:01 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sun, Jun 27, 2021 at 1:56 PM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:

...

> > > > Can you point out the DT in question?
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-next
> > >
> > > Gpio node:
> > >
> > >  gpio: gpio@600 {
> > >               #gpio-cells = <2>;
> > >               #interrupt-cells = <2>;
> > >               compatible = "mediatek,mt7621-gpio";
> > >               gpio-controller;
> > >               gpio-ranges = <&pinctrl 0 0 95>;
> > >               interrupt-controller;
> > >               reg = <0x600 0x100>;
> > >               interrupt-parent = <&gic>;
> > >               interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
> > >    };
> > >
> > > My overlay:
> > >
> > > &gpio {
> > >     gpio-line-names = "", "", "", "",
> > >                       "", "", "SFP LOS", "extcon port5 PoE compat",
> > >                       "SFP module def0", "LED blue SFP", "SFP tx disable", "",
> > >                       "switch USB power", "mode", "", "buzzer",
> > >                       "LED blue pwr", "switch port5 PoE out", "reset";
> > > };
> > >
> > > > > This commit populates the gc.names member of each bank from the
> > > > > device-tree node within the driver. This overrides the default behavior
> > > > > since devprop_gpiochip_set_names() will only be called if names is NULL.
> > > >
> > > > I believe this commit is not needed in the proposed (i.e. duplication) shape.
> > > > The fwnode supports primary and secondary ones. Thus, we may create a
> > > > pair of fwnodes when they will unify properties per device with
> > > > properties per child together (child is primary and device, i.e.
> > > > parent, is secondary).
> > >
> > > There are no child nodes, all the stuff is in the same parent node
> > > and, as I said, belongs to the same device but internally uses three
> > > gpiochips.
> >
> > And it can't be split into three children in the overlay?
>
> Original code before this being mainlined was using three children and
> I was told in the review that three children were not allowed:
>
> See https://patchwork.ozlabs.org/project/linux-gpio/patch/1527924610-13135-3-git-send-email-sergio.paracuellos@gmail.com/#1932827

Thanks for the pointer!

> > Let's assume it can't, then the GPIO library function should be
> > refactored in a way that it takes parameters like base index for the
> > names and tries to satisfy the caller.
>
> Bartosz, Linus, any thoughts on this?

Reading that discussion and seeing Linus there, I guess he himself
signed (implicitly) for my idea :-)
Otherwise we will have duplication of the code for almost no benefit
when we can avoid it completely.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Linus Walleij]

On Fri, Jul 2, 2021 at 11:40 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> On Fri, Jul 2, 2021 at 11:27 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

> > > There are no child nodes, all the stuff is in the same parent node
> > > and, as I said, belongs to the same device but internally uses three
> > > gpiochips.
> >
> > And it can't be split into three children in the overlay?
>
> Original code before this being mainlined was using three children and
> I was told in the review that three children were not allowed:
>
> See https://patchwork.ozlabs.org/project/linux-gpio/patch/1527924610-13135-3-git-send-email-sergio.paracuellos@gmail.com/#1932827

Yeah this is one of those unfortunate cases where the DT representation
(or ACPI for that matter) of the device and the Linux internal representation
differs.

> > Let's assume it can't, then the GPIO library function should be
> > refactored in a way that it takes parameters like base index for the
> > names and tries to satisfy the caller.
>
> Bartosz, Linus, any thoughts on this?

This would be ideal, is there a reasonably simple way to get to this helper?

In gpiolib.c devprop_gpiochip_set_names() need to be refactored to
take a base number, devprop_gpiochip_set_names_base() that
function exposed in <linux/gpio/driver.h> and then the old function
devprop_gpiochip_set_names() wrapped in the new
one so all old users continue to work without modification.
Sprinkle some kerneldoc on top so we do not make mistakes
in the future.

This should work I think.

Any similar drivers (several gpio_chip per FW node) that want to
set line names need to do the same thing.

Sorry that you ran into this, I hate it when I'm first at hairy stuff
like this.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

Hi Linus,

On Fri, Jul 2, 2021 at 12:18 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Jul 2, 2021 at 11:40 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Fri, Jul 2, 2021 at 11:27 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
>
> > > > There are no child nodes, all the stuff is in the same parent node
> > > > and, as I said, belongs to the same device but internally uses three
> > > > gpiochips.
> > >
> > > And it can't be split into three children in the overlay?
> >
> > Original code before this being mainlined was using three children and
> > I was told in the review that three children were not allowed:
> >
> > See https://patchwork.ozlabs.org/project/linux-gpio/patch/1527924610-13135-3-git-send-email-sergio.paracuellos@gmail.com/#1932827
>
> Yeah this is one of those unfortunate cases where the DT representation
> (or ACPI for that matter) of the device and the Linux internal representation
> differs.
>
> > > Let's assume it can't, then the GPIO library function should be
> > > refactored in a way that it takes parameters like base index for the
> > > names and tries to satisfy the caller.
> >
> > Bartosz, Linus, any thoughts on this?
>
> This would be ideal, is there a reasonably simple way to get to this helper?
>
> In gpiolib.c devprop_gpiochip_set_names() need to be refactored to
> take a base number, devprop_gpiochip_set_names_base() that
> function exposed in <linux/gpio/driver.h> and then the old function
> devprop_gpiochip_set_names() wrapped in the new
> one so all old users continue to work without modification.
> Sprinkle some kerneldoc on top so we do not make mistakes
> in the future.
>
> This should work I think.
>
> Any similar drivers (several gpio_chip per FW node) that want to
> set line names need to do the same thing.

Thanks for the advices. I'll try to make a bit of time to try to
handle this in the way you are pointing out here.

Best regards,
    Sergio Paracuellos

>
> Sorry that you ran into this, I hate it when I'm first at hairy stuff
> like this.
>
> Yours,
> Linus Walleij

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

Hi Linus,

On Fri, Jul 2, 2021 at 1:30 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Linus,
>
> On Fri, Jul 2, 2021 at 12:18 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Fri, Jul 2, 2021 at 11:40 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Fri, Jul 2, 2021 at 11:27 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> >
> > > > > There are no child nodes, all the stuff is in the same parent node
> > > > > and, as I said, belongs to the same device but internally uses three
> > > > > gpiochips.
> > > >
> > > > And it can't be split into three children in the overlay?
> > >
> > > Original code before this being mainlined was using three children and
> > > I was told in the review that three children were not allowed:
> > >
> > > See https://patchwork.ozlabs.org/project/linux-gpio/patch/1527924610-13135-3-git-send-email-sergio.paracuellos@gmail.com/#1932827
> >
> > Yeah this is one of those unfortunate cases where the DT representation
> > (or ACPI for that matter) of the device and the Linux internal representation
> > differs.
> >
> > > > Let's assume it can't, then the GPIO library function should be
> > > > refactored in a way that it takes parameters like base index for the
> > > > names and tries to satisfy the caller.
> > >
> > > Bartosz, Linus, any thoughts on this?
> >
> > This would be ideal, is there a reasonably simple way to get to this helper?
> >
> > In gpiolib.c devprop_gpiochip_set_names() need to be refactored to
> > take a base number, devprop_gpiochip_set_names_base() that
> > function exposed in <linux/gpio/driver.h> and then the old function
> > devprop_gpiochip_set_names() wrapped in the new
> > one so all old users continue to work without modification.
> > Sprinkle some kerneldoc on top so we do not make mistakes
> > in the future.
> >
> > This should work I think.

If I am understanding correctly what you mean is something like follows:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6e3c4d7a7d14..5a88a305bc59 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)
 {
// WHATEVER CHANGES TO TAKE base into account

+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 +706,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/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);
+

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.

Best regards,
    Sergio Paracuellos

> >
> > Any similar drivers (several gpio_chip per FW node) that want to
> > set line names need to do the same thing.
>
> Thanks for the advices. I'll try to make a bit of time to try to
> handle this in the way you are pointing out here.
>
> Best regards,
>     Sergio Paracuellos
>
> >
> > Sorry that you ran into this, I hate it when I'm first at hairy stuff
> > like this.
> >
> > Yours,
> > Linus Walleij

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

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.

...

> 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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

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

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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

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.

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

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

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

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

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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> 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.

What if each gpiochip inside the same driver has a different width? In
such a case (looking into the code seems to be the case for
'gpio-brcmstb', since driver's calculations per base are aligned with
this code changes but when it is assigned every line name is taking
into account gpio bank's width variable... If the only "client" of
this code would be gpio-mt7621 (or those where base and width of the
banks is the same) I don't know if changing core code makes sense...

Best regards,
    Sergio Paracuellos

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

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

On Sun, Jul 4, 2021 at 11:06 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > 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:

...

> > > 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.
>
> What if each gpiochip inside the same driver has a different width? In
> such a case (looking into the code seems to be the case for
> 'gpio-brcmstb', since driver's calculations per base are aligned with
> this code changes but when it is assigned every line name is taking
> into account gpio bank's width variable... If the only "client" of
> this code would be gpio-mt7621 (or those where base and width of the
> banks is the same) I don't know if changing core code makes sense...

As far as I understood the problem, the driver (either broadcom one or
mediatek) uses one GPIO description from which it internally splits to
a few GPIO chips. GPIO chips are kinda independent in that sense,
correct? So, if you put the index / offset field per GPIO chip before
creation, the problem is solved.  What did I miss?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

On Sun, Jul 4, 2021 at 12:05 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jul 4, 2021 at 11:06 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > 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:
>
> ...
>
> > > > 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.
> >
> > What if each gpiochip inside the same driver has a different width? In
> > such a case (looking into the code seems to be the case for
> > 'gpio-brcmstb', since driver's calculations per base are aligned with
> > this code changes but when it is assigned every line name is taking
> > into account gpio bank's width variable... If the only "client" of
> > this code would be gpio-mt7621 (or those where base and width of the
> > banks is the same) I don't know if changing core code makes sense...
>
> As far as I understood the problem, the driver (either broadcom one or
> mediatek) uses one GPIO description from which it internally splits to
> a few GPIO chips. GPIO chips are kinda independent in that sense,
> correct? So, if you put the index / offset field per GPIO chip before
> creation, the problem is solved.  What did I miss?

Should be, yes. But my concern is about why the broadcom driver
calculate base as:

base = bank->id * MAX_GPIO_PER_BANK;

and then fill names using:

/*
 * Make sure to not index beyond the end of the number of descriptors
 * of the GPIO device.
 */
for (i = 0; i < bank->width; i++) {
 ...

It looks like each gpio chip is separated MAX_GPIO_PER_BANK but the
width of each of some of them may be different. So in my understanding
assume for example there are four banks with widths 32,32, 24, 32 and
if you want to provide friendly names for all of them, in the third
one you have to create empty strings until 32 or you will get wrong to
the starting of the fourth bank and the code is getting care of not
going out of index in the for loop and assign only those needed. So
technically you are providing 8 empty strings even though the width of
the third bank is only 24 which sounds also bad... But maybe I am
misunderstanding the code itself and I need a bit more sleep :)

Best regards,
    Sergio Paracuellos

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Andy Shevchenko]

On Sun, Jul 4, 2021 at 2:25 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> On Sun, Jul 4, 2021 at 12:05 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Jul 4, 2021 at 11:06 AM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > > 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:
> >
> > ...
> >
> > > > > 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.
> > >
> > > What if each gpiochip inside the same driver has a different width? In
> > > such a case (looking into the code seems to be the case for
> > > 'gpio-brcmstb', since driver's calculations per base are aligned with
> > > this code changes but when it is assigned every line name is taking
> > > into account gpio bank's width variable... If the only "client" of
> > > this code would be gpio-mt7621 (or those where base and width of the
> > > banks is the same) I don't know if changing core code makes sense...
> >
> > As far as I understood the problem, the driver (either broadcom one or
> > mediatek) uses one GPIO description from which it internally splits to
> > a few GPIO chips. GPIO chips are kinda independent in that sense,
> > correct? So, if you put the index / offset field per GPIO chip before
> > creation, the problem is solved.  What did I miss?
>
> Should be, yes. But my concern is about why the broadcom driver
> calculate base as:
>
> base = bank->id * MAX_GPIO_PER_BANK;
>
> and then fill names using:
>
> /*
>  * Make sure to not index beyond the end of the number of descriptors
>  * of the GPIO device.
>  */
> for (i = 0; i < bank->width; i++) {
>  ...
>
> It looks like each gpio chip is separated MAX_GPIO_PER_BANK but the
> width of each of some of them may be different. So in my understanding
> assume for example there are four banks with widths 32,32, 24, 32 and
> if you want to provide friendly names for all of them, in the third
> one you have to create empty strings until 32 or you will get wrong to
> the starting of the fourth bank and the code is getting care of not
> going out of index in the for loop and assign only those needed. So
> technically you are providing 8 empty strings even though the width of
> the third bank is only 24 which sounds also bad...

While I might agree on this, it sounds quite well correct and should
be done that way in such cases. The fundamental fix would be (but will
never appear due to ABI backward compatibility) to allow gaps in the
DT property arrays.

The workaround may be the amount of lines per bank in another property
(gpio-ranges?). In either case the GPIO bindings and drivers that
split hardware per bank seems to me unaligned and that is the root
cause, but it seems it was the initial desire to have like this.

Anyway, I have an opinion that at some point either workaround or
other means will be enforced on the GPIO library level in the core
code and your approach seems a good first step towards that.

> But maybe I am
> misunderstanding the code itself and I need a bit more sleep :)

Also possible :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

On Sun, Jul 4, 2021 at 1:36 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Jul 4, 2021 at 2:25 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > On Sun, Jul 4, 2021 at 12:05 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Jul 4, 2021 at 11:06 AM Sergio Paracuellos
> > > <sergio.paracuellos@gmail.com> wrote:
> > > > On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > 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:
> > >
> > > ...
> > >
> > > > > > 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.
> > > >
> > > > What if each gpiochip inside the same driver has a different width? In
> > > > such a case (looking into the code seems to be the case for
> > > > 'gpio-brcmstb', since driver's calculations per base are aligned with
> > > > this code changes but when it is assigned every line name is taking
> > > > into account gpio bank's width variable... If the only "client" of
> > > > this code would be gpio-mt7621 (or those where base and width of the
> > > > banks is the same) I don't know if changing core code makes sense...
> > >
> > > As far as I understood the problem, the driver (either broadcom one or
> > > mediatek) uses one GPIO description from which it internally splits to
> > > a few GPIO chips. GPIO chips are kinda independent in that sense,
> > > correct? So, if you put the index / offset field per GPIO chip before
> > > creation, the problem is solved.  What did I miss?
> >
> > Should be, yes. But my concern is about why the broadcom driver
> > calculate base as:
> >
> > base = bank->id * MAX_GPIO_PER_BANK;
> >
> > and then fill names using:
> >
> > /*
> >  * Make sure to not index beyond the end of the number of descriptors
> >  * of the GPIO device.
> >  */
> > for (i = 0; i < bank->width; i++) {
> >  ...
> >
> > It looks like each gpio chip is separated MAX_GPIO_PER_BANK but the
> > width of each of some of them may be different. So in my understanding
> > assume for example there are four banks with widths 32,32, 24, 32 and
> > if you want to provide friendly names for all of them, in the third
> > one you have to create empty strings until 32 or you will get wrong to
> > the starting of the fourth bank and the code is getting care of not
> > going out of index in the for loop and assign only those needed. So
> > technically you are providing 8 empty strings even though the width of
> > the third bank is only 24 which sounds also bad...
>
> While I might agree on this, it sounds quite well correct and should
> be done that way in such cases. The fundamental fix would be (but will
> never appear due to ABI backward compatibility) to allow gaps in the
> DT property arrays.

I see, so I guess I'll update my current patch to take this also into
account so I can move the check against ngpio after count is
calculated for both cases. Doing it that way should cover current
behaviour, AFAICS.

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6e3c4d7a7d14..44321ac175d4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -383,11 +383,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)
@@ -401,8 +406,15 @@ static int devprop_gpiochip_set_names(struct
gpio_chip *chip)
                return ret;
        }

+       count = (chip->offset > count) ? chip->offset - count : count;
+       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;
+       }
+
        for (i = 0; i < count; i++)
-               gdev->descs[i].name = names[i];
+               gdev->descs[i].name = names[chip->offset + i];

        kfree(names);

>
> The workaround may be the amount of lines per bank in another property
> (gpio-ranges?). In either case the GPIO bindings and drivers that
> split hardware per bank seems to me unaligned and that is the root
> cause, but it seems it was the initial desire to have like this.

ngpio should have that for the gpiochip that has just called this code, right?

>
> Anyway, I have an opinion that at some point either workaround or
> other means will be enforced on the GPIO library level in the core
> code and your approach seems a good first step towards that.

Thanks, I will properly send this patchset hopefully during this week.

Best regards,
    Sergio Paracuellos

>
> > But maybe I am
> > misunderstanding the code itself and I need a bit more sleep :)
>
> Also possible :-)
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] gpio: mt7621: support gpio-line-names property

[Sergio Paracuellos]

On Sun, Jul 4, 2021 at 10:07 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> On Sun, Jul 4, 2021 at 1:36 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sun, Jul 4, 2021 at 2:25 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > On Sun, Jul 4, 2021 at 12:05 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sun, Jul 4, 2021 at 11:06 AM Sergio Paracuellos
> > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > On Sun, Jul 4, 2021 at 7:57 AM Sergio Paracuellos
> > > > > <sergio.paracuellos@gmail.com> wrote:
> > > > > > 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:
> > > >
> > > > ...
> > > >
> > > > > > > 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.
> > > > >
> > > > > What if each gpiochip inside the same driver has a different width? In
> > > > > such a case (looking into the code seems to be the case for
> > > > > 'gpio-brcmstb', since driver's calculations per base are aligned with
> > > > > this code changes but when it is assigned every line name is taking
> > > > > into account gpio bank's width variable... If the only "client" of
> > > > > this code would be gpio-mt7621 (or those where base and width of the
> > > > > banks is the same) I don't know if changing core code makes sense...
> > > >
> > > > As far as I understood the problem, the driver (either broadcom one or
> > > > mediatek) uses one GPIO description from which it internally splits to
> > > > a few GPIO chips. GPIO chips are kinda independent in that sense,
> > > > correct? So, if you put the index / offset field per GPIO chip before
> > > > creation, the problem is solved.  What did I miss?
> > >
> > > Should be, yes. But my concern is about why the broadcom driver
> > > calculate base as:
> > >
> > > base = bank->id * MAX_GPIO_PER_BANK;
> > >
> > > and then fill names using:
> > >
> > > /*
> > >  * Make sure to not index beyond the end of the number of descriptors
> > >  * of the GPIO device.
> > >  */
> > > for (i = 0; i < bank->width; i++) {
> > >  ...
> > >
> > > It looks like each gpio chip is separated MAX_GPIO_PER_BANK but the
> > > width of each of some of them may be different. So in my understanding
> > > assume for example there are four banks with widths 32,32, 24, 32 and
> > > if you want to provide friendly names for all of them, in the third
> > > one you have to create empty strings until 32 or you will get wrong to
> > > the starting of the fourth bank and the code is getting care of not
> > > going out of index in the for loop and assign only those needed. So
> > > technically you are providing 8 empty strings even though the width of
> > > the third bank is only 24 which sounds also bad...
> >
> > While I might agree on this, it sounds quite well correct and should
> > be done that way in such cases. The fundamental fix would be (but will
> > never appear due to ABI backward compatibility) to allow gaps in the
> > DT property arrays.
>
> I see, so I guess I'll update my current patch to take this also into
> account so I can move the check against ngpio after count is
> calculated for both cases. Doing it that way should cover current
> behaviour, AFAICS.
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6e3c4d7a7d14..44321ac175d4 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -383,11 +383,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)
> @@ -401,8 +406,15 @@ static int devprop_gpiochip_set_names(struct
> gpio_chip *chip)
>                 return ret;
>         }
>
> +       count = (chip->offset > count) ? chip->offset - count : count;
> +       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;
> +       }

I meant 'chip->ngpio' here in both if clause and assignment.

> +
>         for (i = 0; i < count; i++)
> -               gdev->descs[i].name = names[i];
> +               gdev->descs[i].name = names[chip->offset + i];
>
>         kfree(names);
>
> >
> > The workaround may be the amount of lines per bank in another property
> > (gpio-ranges?). In either case the GPIO bindings and drivers that
> > split hardware per bank seems to me unaligned and that is the root
> > cause, but it seems it was the initial desire to have like this.
>
> ngpio should have that for the gpiochip that has just called this code, right?
>
> >
> > Anyway, I have an opinion that at some point either workaround or
> > other means will be enforced on the GPIO library level in the core
> > code and your approach seems a good first step towards that.
>
> Thanks, I will properly send this patchset hopefully during this week.
>
> Best regards,
>     Sergio Paracuellos
>
> >
> > > But maybe I am
> > > misunderstanding the code itself and I need a bit more sleep :)
> >
> > Also possible :-)
> >
> > --
> > With Best Regards,
> > Andy Shevchenko