gpio: omap: make gpio numbering deterministical by using of aliases

Traditionally the n-th gpio device probed by the omap gpio driver got the gpio number range [n*32 .. n*32+31].
When order of the devices probed by the driver changes (which can happen already now when some devices have a pinctrl and so the first probe attempt returns -ENODEV) the numbering changes.

To ensure a deterministical numbering use of_alias_get_id to determine the number base for a given device. If no respective alias exists fall back to the traditional numbering.

For the unusual case where only a part of the gpio devices have a matching alias some of them might fail to probe. But if none of them has an alias or all, there is no conflict which should be good enough to maintain backward compatibility.

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
Hello,

if you're happy with this patch I can follow up and add the respective aliases to the device trees.

  drivers/gpio/gpio-omap.c | 12 ++++++++++++
  1 file changed, 12 insertions(+)

drivers/gpio/gpio-omap.c +1 -0

index b98ede78c9d8..6814245a54aa 100644
@@ -1034,6 +1034,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
  	static int gpio;
  	int irq_base = 0;
  	int ret;
+	int gpio_alias_id;
 
  	/*
  	 * REVISIT eventually switch from OMAP-specific gpio structs

drivers/gpio/gpio-omap.c +0 -0

  	bank->chip.ngpio = bank->width;
 
  	ret = gpiochip_add_data(&bank->chip, bank);

Quoted 10 lines · Uwe Kleine-K?nig

drivers/gpio/gpio-omap.c +1 -0

index b98ede78c9d8..6814245a54aa 100644
@@ -1034,6 +1034,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
  	static int gpio;
  	int irq_base = 0;
  	int ret;
+	int gpio_alias_id;
 

drivers/gpio/gpio-omap.c static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) · line 1058

Quoted 5 lines

drivers/gpio/gpio-omap.c +0 -0

  	/*
  	 * REVISIT eventually switch from OMAP-specific gpio structs

@@ -1056,6 +1057,17 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
  		bank->chip.label = "gpio";
  		bank->chip.base = gpio;

I think, the gpio base correction should be done here

Quoted 7 lines

drivers/gpio/gpio-omap.c +6 -0

  	}
+
+	/*
+	 * Traditionally the base is given out in first-come-first-serve order.
+	 * This might shuffle the numbering of gpios if the probe order changes.
+	 * So make the base deterministical if the device tree specifies alias
+	 * ids.

drivers/gpio/gpio-omap.c static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) · line 1070

Quoted 5 lines

drivers/gpio/gpio-omap.c +5 -0

+	 */
+	gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
+	if (gpio_alias_id >= 0)
+		bank->chip.base = bank->width * gpio_alias_id;
+

Unfortunately, this driver is still used by non-DT platforms, so above code will
break build if !OF && !OF_GPIO

Hello Grygorii,

Quoted 11 lines · Grygorii Strashko

Quoted 10 lines · Uwe Kleine-K?nig

drivers/gpio/gpio-omap.c +1 -0

index b98ede78c9d8..6814245a54aa 100644
@@ -1034,6 +1034,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
   	static int gpio;
   	int irq_base = 0;
   	int ret;
+	int gpio_alias_id;
 

Quoted 7 lines

drivers/gpio/gpio-omap.c static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) · line 1058

Quoted 5 lines

drivers/gpio/gpio-omap.c +0 -0

   	/*
   	 * REVISIT eventually switch from OMAP-specific gpio structs

@@ -1056,6 +1057,17 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
   		bank->chip.label = "gpio";
   		bank->chip.base = gpio;

I think, the gpio base correction should be done here

What is the upside of doing it here? It has the downside that it needs one more indention level. The only difference I see is that it doesn't apply for mpuio devices then and wonder if that is relevant.

Quoted 16 lines

drivers/gpio/gpio-omap.c static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) · line 1070

Quoted 13 lines

drivers/gpio/gpio-omap.c +11 -0

   	}
+
+	/*
+	 * Traditionally the base is given out in first-come-first-serve order.
+	 * This might shuffle the numbering of gpios if the probe order changes.
+	 * So make the base deterministical if the device tree specifies alias
+	 * ids.
+	 */
+	gpio_alias_id = of_alias_get_id(bank->chip.of_node, "gpio");
+	if (gpio_alias_id >= 0)
+		bank->chip.base = bank->width * gpio_alias_id;
+

Unfortunately, this driver is still used by non-DT platforms, so above code will
break build if !OF && !OF_GPIO

Without CONFIG_OF of_alias_get_id still exists and returns -ENOSYS which should make the added code a nop. Does your compiler agree with you that my patch breaks building the driver?

Traditionally the n-th gpio device probed by the omap gpio driver got the gpio number range [n*32 .. n*32+31].
When order of the devices probed by the driver changes (which can happen already now when some devices have a pinctrl and so the first probe attempt returns -ENODEV) the numbering changes.

To ensure a deterministical numbering use of_alias_get_id to determine the number base for a given device. If no respective alias exists fall back to the traditional numbering.

Quoted 6 lines · Uwe Kleine-K?nig

Traditionally the n-th gpio device probed by the omap gpio driver got the gpio number range [n*32 .. n*32+31].
When order of the devices probed by the driver changes (which can happen already now when some devices have a pinctrl and so the first probe attempt returns -ENODEV) the numbering changes.

Quoted 3 lines

To ensure a deterministical numbering use of_alias_get_id to determine the number base for a given device. If no respective alias exists fall back to the traditional numbering.

I'm not too happy about this approach.

The patch doesn't mention what practical problems it is trying to solve.

The GPIO numbering scheme is a matter of Linux internals and not about hardware description IMO.

The way forward is to use the character device and use gpiochip devices with offset indexes and look up GPIOs by name from the character devices. If nothing substantial happens I am merging the final pieces of the GPIO chardev ABI for v4.8 and that is doing all that sysfs was doing and then some. I just need to change a small thing before sending the final version for review.

Quoted 6 lines · Linus Walleij

The way forward is to use the character device and use gpiochip devices with offset indexes and look up GPIOs by name from the character devices. If nothing substantial happens I am merging the final pieces of the GPIO chardev ABI for v4.8 and that is doing all that sysfs was doing and then some. I just need to change a small thing before sending the final version for review.

Hmm, so /sys/class/gpio was obsoleted before the substitution was ready? I'd say an overlapping of several kernel versions would be good as we cannot expect that userspace changes as fast as the kernel.

Quoted 8 lines · Uwe Kleine-K?nig

Quoted 6 lines · Linus Walleij

The way forward is to use the character device and use gpiochip devices with offset indexes and look up GPIOs by name from the character devices. If nothing substantial happens I am merging the final pieces of the GPIO chardev ABI for v4.8 and that is doing all that sysfs was doing and then some. I just need to change a small thing before sending the final version for review.

Quoted 3 lines

Hmm, so /sys/class/gpio was obsoleted before the substitution was ready? I'd say an overlapping of several kernel versions would be good as we cannot expect that userspace changes as fast as the kernel.

Well the /sys/class/gpio is an interface, and we'll have to maintain it basically forever as we all know. It also works just fine for simple things, so let's make sure it is usable. Having it inconsistent can cause nasty side effects upgrading kernels.

Wouldn't gpio-line-names be helpful for such cases as mentioned in [1],[2] But I don't know it these properties were exposed in legacy GPIO sysfs interface also.

Quoted 8 lines · Uwe Kleine-K?nig

Quoted 6 lines · Linus Walleij

The way forward is to use the character device and use gpiochip devices with offset indexes and look up GPIOs by name from the character devices. If nothing substantial happens I am merging the final pieces of the GPIO chardev ABI for v4.8 and that is doing all that sysfs was doing and then some. I just need to change a small thing before sending the final version for review.

Quoted 3 lines

Hmm, so /sys/class/gpio was obsoleted before the substitution was ready? I'd say an overlapping of several kernel versions would be good as we cannot expect that userspace changes as fast as the kernel.

Well the /sys/class/gpio is an interface, and we'll have to maintain it basically forever as we all know. It also works just fine for simple things, so let's make sure it is usable. Having it inconsistent can cause nasty side effects upgrading kernels.

Quoted 2 lines · Linus Walleij

The GPIO numbering scheme is a matter of Linux internals and not about hardware description IMO.

Not sure if I should agree here or not. It's very usual that the "internal" gpio numbers match the hardware reference manual. I know this from imx, at91, all pre-dt platforms, I'm sure there are more, and I bet I'm not the only one relying on this for omap.

And this is very usual in the dt world, too:

$ git grep -El 'gpio. = \&gpio' arch/arm/boot/dts | wc -l
37

Quoted 6 lines

The way forward is to use the character device and use gpiochip devices with offset indexes and look up GPIOs by name from the character devices. If nothing substantial happens I am merging the final pieces of the GPIO chardev ABI for v4.8 and that is doing all that sysfs was doing and then some. I just need to change a small thing before sending the final version for review.

Hmm, so /sys/class/gpio was obsoleted before the substitution was ready?

I'd say an overlapping of several kernel versions would be good as we cannot expect that userspace changes as fast as the kernel.

Independent of the inclusion of my patch series in the mainline kernel I'll have to use it on my custom kernel to keep the hardware do what it should.

Quoted 5 lines · Uwe Kleine-K?nig

Quoted 2 lines · Linus Walleij

The GPIO numbering scheme is a matter of Linux internals and not about hardware description IMO.

Quoted 4 lines

Not sure if I should agree here or not. It's very usual that the "internal" gpio numbers match the hardware reference manual. I know this from imx, at91, all pre-dt platforms, I'm sure there are more, and I bet I'm not the only one relying on this for omap.

I think it will still match nicely against the chip-local offsets of the primary gpiochip so it'll be fine with a chardev too. The same was/is

the case of the first interrupts on x86 I think, but with the plethora of irqchips and dependency on probe order etc the assumption is nowadays to dangerous.

Quoted 1 line

Quoted 4 lines

And this is very usual in the dt world, too:

$ git grep -El 'gpio. = \&gpio' arch/arm/boot/dts | wc -l
37

Aha I didn't even know. Well I guess I could allow it for OMAP too then, but I want an ACK from one of the DT binding maintainers.

Hello Linus,

Quoted 12 lines · Linus Walleij

Quoted 9 lines · Uwe Kleine-K?nig

Quoted 2 lines · Linus Walleij

The GPIO numbering scheme is a matter of Linux internals and not about hardware description IMO.

Not sure if I should agree here or not. It's very usual that the "internal" gpio numbers match the hardware reference manual. I know this from imx, at91, all pre-dt platforms, I'm sure there are more, and I bet I'm not the only one relying on this for omap.

Quoted 2 lines

I think it will still match nicely against the chip-local offsets of the primary gpiochip so it'll be fine with a chardev too. The same was/is

I cannot follow. What is the primary gpiochip? The first one? What is a "chip-local offset". Just 3 for the fourth gpio of a given gpio bank? I guess the problem is that I didn't follow development of the gpio chardev.

Quoted 11 lines

the case of the first interrupts on x86 I think, but with the plethora of irqchips and dependency on probe order etc the assumption is nowadays to dangerous.

Quoted 4 lines

And this is very usual in the dt world, too:

$ git grep -El 'gpio. = \&gpio' arch/arm/boot/dts | wc -l
37

Aha I didn't even know. Well I guess I could allow it for OMAP too then, but I want an ACK from one of the DT binding maintainers.

In general, our use of aliases is rather ill-defined. It would be nicer if we could address devices in a similar manner to disks or partitions, e.g. by path or uuid, but I don't think we have anything sensible we can use there.

Given that, I can see the use of an alias to provide a consistent way of referring to a particular gpiochip (and maybe we need to expose the alises information somehow to userspace), but IMO that's independent of any global ID space, probe ordering, etc.

From the kernel point of view the way forward to identify and refer to

Quoted 9 lines · Mark Rutland

In general, our use of aliases is rather ill-defined. It would be nicer if we could address devices in a similar manner to disks or partitions, e.g. by path or uuid, but I don't think we have anything sensible we can use there.

Given that, I can see the use of an alias to provide a consistent way of referring to a particular gpiochip (and maybe we need to expose the alises information somehow to userspace), but IMO that's independent of any global ID space, probe ordering, etc.

From the kernel point of view the way forward to identify and refer to
a particular gpiochip is using /dev/gpiochipN the character device.

If complete topology of the bus placement etc is needed, userspace can traverse /sys/bus/gpio/*

This solves the big problem with the current global numbering system in /sys/class/gpio/*

So what this alias should address would be two things:

- Solve the immediate issue of the global number space for the legacy sysfs ABI, but also:

- Determine which chip is gpiochip0, gpiochip1, .. etc in the new ABI, so the devices get consistent numbering.

The latter is lightly frowned upon by the udev people: they think it is more proper to traverse /sys to get topological information about the devices.

I would appreciate if a patch to add alias handling would take care of both these things if we apply it.

Quoted 2 lines

This solves the big problem with the current global numbering system in /sys/class/gpio/*

Hm. May be i misunderstood samthing (and sry, if my following question is dummy as I've not followed closely new GPIO ABI development), but..

- from above description it seems that global numbering system is not really solved :( instead it's been moved one level up and now the same happens with gpiocipX devices :(

Wouldn't it be reasonable to add possibility to create named/labeled
gpiocipX devices from the very beginning, like: "/dev/gpiocipX[_name|label], or "/dev/gpiocip[_name|label]", or ..

Actually, struct gpio_chip has label field already.

PS. From my experience, the worst case with dev numbering usually happens after few insmod/rmmod (or sometimes after suspend/resume) iterations - ttyX ;..(.