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@xxxxxxxxxxxxxxx> wrote:

> From: Markus Niebel <Markus.Niebel@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> ---
> 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
>