[PATCH 1/1] gpiolib: allow device numbering using OF alias

[PATCH 1/1] gpiolib: allow device numbering using OF alias

[Alexander Stein]

From: Markus Niebel <Markus.Niebel@ew.tq-group.com>

This is useful e.g. for the following cases

- GPIO IP name order is not aligned with SOC addresses
  (i.MX93 from NXP)
- reproducible naming for GPIO expander chips

The implementation is a mix of the one found for MMC and RTC.

Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
imx93 specifies alias for 4 on-chip GPIO controllers. But they are
ignored:
$ ls -o -g /sys/bus/gpio/devices/
total 0
lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip0 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0071/gpiochip0
lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip1 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0072/gpiochip1
lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip2 -> ../../../devices/platform/soc@0/43810080.gpio/gpiochip2
lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip3 -> ../../../devices/platform/soc@0/43820080.gpio/gpiochip3
lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip4 -> ../../../devices/platform/soc@0/43830080.gpio/gpiochip4
lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip5 -> ../../../devices/platform/soc@0/47400080.gpio/gpiochip5
lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip6 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0070/gpiochip6

With this patch this becomes:
$ ls -o -g /sys/bus/gpio/devices/
total 0
lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip0 -> ../../../devices/platform/soc@0/47400080.gpio/gpiochip0
lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip1 -> ../../../devices/platform/soc@0/43810080.gpio/gpiochip1
lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip2 -> ../../../devices/platform/soc@0/43820080.gpio/gpiochip2
lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip3 -> ../../../devices/platform/soc@0/43830080.gpio/gpiochip3
lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip4 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0071/gpiochip4
lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip5 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0072/gpiochip5
lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip6 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0070/gpiochip6

 drivers/gpio/gpiolib.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 19bd23044b01..4d606ad522ac 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -663,10 +663,25 @@ static void gpiochip_setup_devs(void)
 	}
 }
 
+/**
+ * gpio_first_nonreserved_index() - get the first index that is not reserved
+ */
+static int gpio_first_nonreserved_index(void)
+{
+	int max;
+
+	max = of_alias_get_highest_id("gpio");
+	if (max < 0)
+		return 0;
+
+	return max + 1;
+}
+
 int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			       struct lock_class_key *lock_key,
 			       struct lock_class_key *request_key)
 {
+	int index, alias_id, min_idx;
 	struct fwnode_handle *fwnode = NULL;
 	struct gpio_device *gdev;
 	unsigned long flags;
@@ -696,12 +711,22 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	device_set_node(&gdev->dev, gc->fwnode);
 
-	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
-	if (gdev->id < 0) {
-		ret = gdev->id;
-		goto err_free_gdev;
+	alias_id = of_alias_get_id(to_of_node(gc->fwnode), "gpio");
+	if (alias_id >= 0) {
+		index = ida_simple_get(&gpio_ida, alias_id, alias_id + 1,
+				       GFP_KERNEL);
+	} else {
+		min_idx = gpio_first_nonreserved_index();
+		index = ida_simple_get(&gpio_ida, min_idx, 0,
+				       GFP_KERNEL);
+		if (index < 0) {
+			ret = gdev->id;
+			goto err_free_gdev;
+		}
 	}
 
+	gdev->id = index;
+
 	ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
 	if (ret)
 		goto err_free_ida;
-- 
2.34.1

Re: [PATCH 1/1] gpiolib: allow device numbering using OF alias

[Linus Walleij]

Top-posting because important people are missing from the to:line.

It seems you are trying to enforce topology here,
i.e. hammering down what should come first, second etc, despite the
probe order.

First the DT people need to acknowledge that this is a valid way to use
device tree aliases. I'm not so sure about that. Remember that DT
is mostly OS neutral, but we do have aliases for some use cases that
can be the same tricky in any OS.

Second I want Johan Hovolds input on this from the Linux sysfs side, as
he keeps reminding me that sysfs already has topology and should be
discovered from there (loosely paraphrased from memory). It might
be that you are fixing something that should not be fixed.

Please keep the new respondents on subsequent postings.

Yours,
Linus Walleij

On Wed, Feb 15, 2023 at 10:24 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
>
> This is useful e.g. for the following cases
>
> - GPIO IP name order is not aligned with SOC addresses
>   (i.MX93 from NXP)
> - reproducible naming for GPIO expander chips
>
> The implementation is a mix of the one found for MMC and RTC.
>
> Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> imx93 specifies alias for 4 on-chip GPIO controllers. But they are
> ignored:
> $ ls -o -g /sys/bus/gpio/devices/
> total 0
> lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip0 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0071/gpiochip0
> lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip1 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0072/gpiochip1
> lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip2 -> ../../../devices/platform/soc@0/43810080.gpio/gpiochip2
> lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip3 -> ../../../devices/platform/soc@0/43820080.gpio/gpiochip3
> lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip4 -> ../../../devices/platform/soc@0/43830080.gpio/gpiochip4
> lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip5 -> ../../../devices/platform/soc@0/47400080.gpio/gpiochip5
> lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip6 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0070/gpiochip6
>
> With this patch this becomes:
> $ ls -o -g /sys/bus/gpio/devices/
> total 0
> lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip0 -> ../../../devices/platform/soc@0/47400080.gpio/gpiochip0
> lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip1 -> ../../../devices/platform/soc@0/43810080.gpio/gpiochip1
> lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip2 -> ../../../devices/platform/soc@0/43820080.gpio/gpiochip2
> lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip3 -> ../../../devices/platform/soc@0/43830080.gpio/gpiochip3
> lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip4 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0071/gpiochip4
> lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip5 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0072/gpiochip5
> lrwxrwxrwx 1 0 Feb 15 10:18 gpiochip6 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0070/gpiochip6
>
>  drivers/gpio/gpiolib.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 19bd23044b01..4d606ad522ac 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -663,10 +663,25 @@ static void gpiochip_setup_devs(void)
>         }
>  }
>
> +/**
> + * gpio_first_nonreserved_index() - get the first index that is not reserved
> + */
> +static int gpio_first_nonreserved_index(void)
> +{
> +       int max;
> +
> +       max = of_alias_get_highest_id("gpio");
> +       if (max < 0)
> +               return 0;
> +
> +       return max + 1;
> +}
> +
>  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>                                struct lock_class_key *lock_key,
>                                struct lock_class_key *request_key)
>  {
> +       int index, alias_id, min_idx;
>         struct fwnode_handle *fwnode = NULL;
>         struct gpio_device *gdev;
>         unsigned long flags;
> @@ -696,12 +711,22 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>
>         device_set_node(&gdev->dev, gc->fwnode);
>
> -       gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
> -       if (gdev->id < 0) {
> -               ret = gdev->id;
> -               goto err_free_gdev;
> +       alias_id = of_alias_get_id(to_of_node(gc->fwnode), "gpio");
> +       if (alias_id >= 0) {
> +               index = ida_simple_get(&gpio_ida, alias_id, alias_id + 1,
> +                                      GFP_KERNEL);
> +       } else {
> +               min_idx = gpio_first_nonreserved_index();
> +               index = ida_simple_get(&gpio_ida, min_idx, 0,
> +                                      GFP_KERNEL);
> +               if (index < 0) {
> +                       ret = gdev->id;
> +                       goto err_free_gdev;
> +               }
>         }
>
> +       gdev->id = index;
> +
>         ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
>         if (ret)
>                 goto err_free_ida;
> --
> 2.34.1
>

Re: [PATCH 1/1] gpiolib: allow device numbering using OF alias

[Johan Hovold]

Hi Linus,

and sorry about the late reply on this.

On Wed, Feb 15, 2023 at 03:43:41PM +0100, Linus Walleij wrote:
> Top-posting because important people are missing from the to:line.
> 
> It seems you are trying to enforce topology here,
> i.e. hammering down what should come first, second etc, despite the
> probe order.
> 
> First the DT people need to acknowledge that this is a valid way to use
> device tree aliases. I'm not so sure about that. Remember that DT
> is mostly OS neutral, but we do have aliases for some use cases that
> can be the same tricky in any OS.

Yeah, I believe the idea is that aliases should generally be avoided
expect possibly for the console (or named) serial ports and first
ethernet interface.

> Second I want Johan Hovolds input on this from the Linux sysfs side, as
> he keeps reminding me that sysfs already has topology and should be
> discovered from there (loosely paraphrased from memory). It might
> be that you are fixing something that should not be fixed.

If user space needs to access some gpios directly then you can name
those resources and that should not rely on having stable gpiochip
ids.

> On Wed, Feb 15, 2023 at 10:24 AM Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
> 
> > From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> >
> > This is useful e.g. for the following cases
> >
> > - GPIO IP name order is not aligned with SOC addresses
> >   (i.MX93 from NXP)
> > - reproducible naming for GPIO expander chips
> >
> > The implementation is a mix of the one found for MMC and RTC.
> >
> > Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > imx93 specifies alias for 4 on-chip GPIO controllers. But they are
> > ignored:
> > $ ls -o -g /sys/bus/gpio/devices/
> > total 0
> > lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip0 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0071/gpiochip0
> > lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip1 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0072/gpiochip1
> > lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip2 -> ../../../devices/platform/soc@0/43810080.gpio/gpiochip2
> > lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip3 -> ../../../devices/platform/soc@0/43820080.gpio/gpiochip3
> > lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip4 -> ../../../devices/platform/soc@0/43830080.gpio/gpiochip4
> > lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip5 -> ../../../devices/platform/soc@0/47400080.gpio/gpiochip5
> > lrwxrwxrwx 1 0 Feb 15 10:03 gpiochip6 -> ../../../devices/platform/soc@0/42000000.bus/42530000.i2c/i2c-2/2-0070/gpiochip6

Johan