[PATCH] gpio: omap: Honor "aliases" node

[PATCH] gpio: omap: Honor "aliases" node

[Alexander Sverdlin]

Currently the naming of the GPIO chips depends on their order in the DT,
but also on the kernel version (I've noticed the change from v5.10.x to
v5.11). Honor the persistent enumeration in the "aliases" node like other
GPIO drivers do.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
---
Yes, I noticed checkpatch "WARNING: DT binding docs and includes should be
a separate patch."
However, the parts below are tiny and barely make sense separately.

 Documentation/devicetree/bindings/gpio/gpio-omap.txt | 6 ++++++
 drivers/gpio/gpio-omap.c                             | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
index e57b2cb28f6c..6050db3fd84e 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-omap.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
@@ -30,9 +30,15 @@ OMAP specific properties:
 - ti,gpio-always-on: 	Indicates if a GPIO bank is always powered and
 			so will never lose its logic state.
 
+Note: GPIO ports can have an alias correctly numbered in "aliases" node for
+persistent enumeration.
 
 Example:
 
+aliases {
+	gpio0 = &gpio0;
+};
+
 gpio0: gpio@44e07000 {
     compatible = "ti,omap4-gpio";
     reg = <0x44e07000 0x1000>;
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 41952bb818ad..dd2a8f6d920f 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1014,6 +1014,11 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
 			bank->chip.parent = &omap_mpuio_device.dev;
 		bank->chip.base = OMAP_MPUIO(0);
 	} else {
+#ifdef CONFIG_OF_GPIO
+		ret = of_alias_get_id(bank->chip.of_node, "gpio");
+		if (ret >= 0)
+			gpio = ret * bank->width;
+#endif
 		label = devm_kasprintf(bank->chip.parent, GFP_KERNEL, "gpio-%d-%d",
 				       gpio, gpio + bank->width - 1);
 		if (!label)
-- 
2.30.1

Re: [PATCH] gpio: omap: Honor "aliases" node

[Grygorii Strashko]

On 02/03/2021 03:18, Alexander Sverdlin wrote:
> Currently the naming of the GPIO chips depends on their order in the DT,
> but also on the kernel version (I've noticed the change from v5.10.x to
> v5.11). Honor the persistent enumeration in the "aliases" node like other
> GPIO drivers do.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> ---
> Yes, I noticed checkpatch "WARNING: DT binding docs and includes should be
> a separate patch."
> However, the parts below are tiny and barely make sense separately.
> 
>   Documentation/devicetree/bindings/gpio/gpio-omap.txt | 6 ++++++
>   drivers/gpio/gpio-omap.c                             | 5 +++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
> index e57b2cb28f6c..6050db3fd84e 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-omap.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
> @@ -30,9 +30,15 @@ OMAP specific properties:
>   - ti,gpio-always-on: 	Indicates if a GPIO bank is always powered and
>   			so will never lose its logic state.
>   
> +Note: GPIO ports can have an alias correctly numbered in "aliases" node for
> +persistent enumeration.
>   
>   Example:
>   
> +aliases {
> +	gpio0 = &gpio0;
> +};
> +
>   gpio0: gpio@44e07000 {
>       compatible = "ti,omap4-gpio";
>       reg = <0x44e07000 0x1000>;
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 41952bb818ad..dd2a8f6d920f 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1014,6 +1014,11 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc)
>   			bank->chip.parent = &omap_mpuio_device.dev;
>   		bank->chip.base = OMAP_MPUIO(0);
>   	} else {
> +#ifdef CONFIG_OF_GPIO
> +		ret = of_alias_get_id(bank->chip.of_node, "gpio");
> +		if (ret >= 0)
> +			gpio = ret * bank->width;
> +#endif
>   		label = devm_kasprintf(bank->chip.parent, GFP_KERNEL, "gpio-%d-%d",
>   				       gpio, gpio + bank->width - 1);
>   		if (!label)
> 

You're not the first one, this was not accepted. See [1]
[1] https://patchwork.kernel.org/project/linux-omap/patch/1465898604-16294-1-git-send-email-u.kleine-koenig@pengutronix.de/


-- 
Best regards,
grygorii

Re: [PATCH] gpio: omap: Honor "aliases" node

[Linus Walleij]

On Tue, Mar 2, 2021 at 2:18 AM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:

> Currently the naming of the GPIO chips depends on their order in the DT,
> but also on the kernel version (I've noticed the change from v5.10.x to
> v5.11). Honor the persistent enumeration in the "aliases" node like other
> GPIO drivers do.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> ---
> Yes, I noticed checkpatch "WARNING: DT binding docs and includes should be
> a separate patch."
> However, the parts below are tiny and barely make sense separately.

I've shut it down in the past because the instance ordering is a
linuxism and the needs are in the Linux userspace somehow.
It is different from a UART for example, which always need to
be at the same place on any operating system, hence it has an
alias.

For kernelspace the instance order should not matter, since
all resources are obtained from the device tree anyway
by phandle.

For userspace:
The way to determine topology in Linux userspace is to use sysfs,
and combined with the GPIO character device this provides a
unique ID for each GPIO chip and line on the system.

/sys/bus/gpio/devices/gpiochip0/
/sys/bus/gpio/devices/gpiochip1/

etc can change, but these appear as PCI, I2C, SPI, platform
etc nodes as well. On my PC:

/sys/devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.5/1-1.5:1.0/gpiochip0

It's pretty clear where that gpiochip sits.

Yours,
Linus Walleij

Re: [PATCH] gpio: omap: Honor "aliases" node

[Rob Herring]

On Tue, Mar 02, 2021 at 05:21:23PM +0100, Linus Walleij wrote:
> On Tue, Mar 2, 2021 at 2:18 AM Alexander Sverdlin
> <alexander.sverdlin@gmail.com> wrote:
> 
> > Currently the naming of the GPIO chips depends on their order in the DT,
> > but also on the kernel version (I've noticed the change from v5.10.x to
> > v5.11). Honor the persistent enumeration in the "aliases" node like other
> > GPIO drivers do.
> >
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> > ---
> > Yes, I noticed checkpatch "WARNING: DT binding docs and includes should be
> > a separate patch."
> > However, the parts below are tiny and barely make sense separately.
> 
> I've shut it down in the past because the instance ordering is a
> linuxism and the needs are in the Linux userspace somehow.
> It is different from a UART for example, which always need to
> be at the same place on any operating system, hence it has an
> alias.
> 
> For kernelspace the instance order should not matter, since
> all resources are obtained from the device tree anyway
> by phandle.

Thank you!

Can we remove the ones we have already for GPIO? 

BTW, It's been on my todo list for a while to start requiring 
documentation of alias names so we can reject new ones and get rid of 
some of the unused existing ones. Some platforms have numbered 
everything...

Rob

Re: [PATCH] gpio: omap: Honor "aliases" node

[Linus Walleij]

On Mon, Mar 8, 2021 at 7:37 PM Rob Herring <robh@kernel.org> wrote:

> Can we remove the ones we have already for GPIO?

I think we would get pretty hard pushback if we attempt that.
We have all these drivers that utilize it:

gpio-clps711x.c:        id = of_alias_get_id(np, "gpio");
gpio-mvebu.c:   id = of_alias_get_id(pdev->dev.of_node, "gpio");
gpio-mxc.c:     port->gc.base = (pdev->id < 0) ? of_alias_get_id(np,
"gpio") * 32 :
gpio-mxs.c:     port->id = of_alias_get_id(np, "gpio");
gpio-vf610.c:   gc->base = of_alias_get_id(np, "gpio") * VF610_GPIO_PER_PORT;
gpio-zynq.c:    chip->base = of_alias_get_id(pdev->dev.of_node, "gpio");
pinctrl-at91.c: int alias_idx = of_alias_get_id(np, "gpio");
pinctrl-st.c:   int bank_num = of_alias_get_id(np, "gpio");
samsung/pinctrl-samsung.c:      id = of_alias_get_id(node, "pinctrl");

Predictably it is so many bad examples that new driver authors will claim
something along the line of
"why can't I have a lollipop when all other kids got one".

Several of those have this by a claim one way or another that
the DT boot need to look like the boardfile boot. Some of these
have been migrated from board files so could possible drop
this id/base coding.

I don't know what the maintainers would say, should we send
attack patches? :D At least some kind of motivation would come
out of it.

Yours,
Linus Walleij