gpio: mt7621: support gpio-line-names property

Message ID 20210619073551.13703-1-sergio.paracuellos@gmail.com
State New
Headers show
Series
  • gpio: mt7621: support gpio-line-names property
Related show

Commit Message

Sergio Paracuellos June 19, 2021, 7:35 a.m.
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.

Thanks in advance for your time.

Best regards,
    Sergio Paracuellos

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

Comments

Bartosz Golaszewski June 25, 2021, 11:53 a.m. | #1
On Sat, Jun 19, 2021 at 9:35 AM 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 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.

>

> Thanks in advance for your time.

>

> Best regards,

>     Sergio Paracuellos

>

>  drivers/gpio/gpio-mt7621.c | 41 ++++++++++++++++++++++++++++++++++++++

>  1 file changed, 41 insertions(+)

>

> diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c

> index 82fb20dca53a..b5f8fd8e928a 100644

> --- a/drivers/gpio/gpio-mt7621.c

> +++ b/drivers/gpio/gpio-mt7621.c

> @@ -206,6 +206,45 @@ mediatek_gpio_xlate(struct gpio_chip *chip,

>         return gpio % MTK_BANK_WIDTH;

>  }

>

> +static void

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


While the ENODATA bit makes sense, not failing after an OOM in a
driver is wrong. Please return the error code here.

Bartosz

> +

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

> +}

> +

>  static int

>  mediatek_gpio_bank_probe(struct device *dev,

>                          struct device_node *node, int bank)

> @@ -241,6 +280,8 @@ mediatek_gpio_bank_probe(struct device *dev,

>         if (!rg->chip.label)

>                 return -ENOMEM;

>

> +       mediatek_gpio_set_names(dev, rg);

> +

>         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

>
Sergio Paracuellos June 25, 2021, 12:40 p.m. | #2
Hi Bartosz,

On Fri, Jun 25, 2021 at 1:53 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>

> On Sat, Jun 19, 2021 at 9:35 AM 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 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.

> >

> > Thanks in advance for your time.

> >

> > Best regards,

> >     Sergio Paracuellos

> >

> >  drivers/gpio/gpio-mt7621.c | 41 ++++++++++++++++++++++++++++++++++++++

> >  1 file changed, 41 insertions(+)

> >

> > diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c

> > index 82fb20dca53a..b5f8fd8e928a 100644

> > --- a/drivers/gpio/gpio-mt7621.c

> > +++ b/drivers/gpio/gpio-mt7621.c

> > @@ -206,6 +206,45 @@ mediatek_gpio_xlate(struct gpio_chip *chip,

> >         return gpio % MTK_BANK_WIDTH;

> >  }

> >

> > +static void

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

>

> While the ENODATA bit makes sense, not failing after an OOM in a

> driver is wrong. Please return the error code here.


Ok, I see your point. I didn't want the driver to fail in its setup
completion because of this property being failing but what you are
saying makes sense, so I will return -ENOMEM for this case and check
for it in 'mediatek_gpio_bank_probe'. Will send v2 with these changes
hopefully tomorrow.

Thanks,
    Sergio Paracuellos

>

> Bartosz

>

> > +

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

> > +}

> > +

> >  static int

> >  mediatek_gpio_bank_probe(struct device *dev,

> >                          struct device_node *node, int bank)

> > @@ -241,6 +280,8 @@ mediatek_gpio_bank_probe(struct device *dev,

> >         if (!rg->chip.label)

> >                 return -ENOMEM;

> >

> > +       mediatek_gpio_set_names(dev, rg);

> > +

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

> >
Andy Shevchenko June 27, 2021, 9:31 a.m. | #3
On Sat, Jun 19, 2021 at 1:02 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.


...

> +static void

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

> +

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

> +}


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

Patch

diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c
index 82fb20dca53a..b5f8fd8e928a 100644
--- a/drivers/gpio/gpio-mt7621.c
+++ b/drivers/gpio/gpio-mt7621.c
@@ -206,6 +206,45 @@  mediatek_gpio_xlate(struct gpio_chip *chip,
 	return gpio % MTK_BANK_WIDTH;
 }
 
+static void
+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;
+
+	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;
+}
+
 static int
 mediatek_gpio_bank_probe(struct device *dev,
 			 struct device_node *node, int bank)
@@ -241,6 +280,8 @@  mediatek_gpio_bank_probe(struct device *dev,
 	if (!rg->chip.label)
 		return -ENOMEM;
 
+	mediatek_gpio_set_names(dev, rg);
+
 	rg->irq_chip.name = dev_name(dev);
 	rg->irq_chip.parent_device = dev;
 	rg->irq_chip.irq_unmask = mediatek_gpio_irq_unmask;