* Re: [PATCH] gpiolib: allow line names from device props to override driver names
2021-12-09 11:32 [PATCH] gpiolib: allow line names from device props to override driver names Peter Rosin
@ 2021-12-09 12:12 ` Andy Shevchenko
2021-12-10 12:24 ` Alexander Dahl
2021-12-13 14:04 ` Bartosz Golaszewski
2 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2021-12-09 12:12 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Linus Walleij, Bartosz Golaszewski, linux-gpio,
Alexander Dahl, Ludovic Desroches, Nicolas Ferre,
Alexandre Belloni
On Thu, Dec 9, 2021 at 1:32 PM Peter Rosin <peda@axentia.se> wrote:
>
> Some gpio providers set names for gpio lines that match the names of
gpio -> GPIO here and everywhere else in the patch where it is applicable.
> the pins on the SoC, or variations on that theme. These names are
> generally generic, such as pioC12 in the at91 case. These generic names
"generally generic" (reminds me
https://en.wiktionary.org/wiki/%D0%BC%D0%B0%D1%81%D0%BB%D0%BE_%D0%BC%D0%B0%D1%81%D0%BB%D1%8F%D0%BD%D0%BE%D0%B5)
Perhaps
"These names generally speaking are generic, ..." ?
> block the possibility to name gpio lines with in device properties
within ?
> (i.e. gpio-line-names).
>
> Allow overriding a generic name given by the gpio driver if there is
> a name given to the gpio line using device properties, but leave the
> generic name alone if no better name is available.
>
> However, there is a risk. If user space is depending on the above
> mentioned fixed gpio names, AND there are device properties that
> previously did not reach the surface, the name change might cause
> regressions. But hopefully this stays below the radar...
After addressing the mentioned grammar nit-picks, FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Thanks!
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> drivers/gpio/gpiolib.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> Instead of doing this only for pinctrl-at91.c as in my recent patch [1], do
> it for everyone.
>
> Cheers,
> Peter
>
> [1] https://lore.kernel.org/lkml/4d17866a-d9a4-a3d7-189a-781d18dbea00@axentia.se/
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index abfbf546d159..00a2a689c202 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -422,8 +422,15 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
> if (count > chip->ngpio)
> count = chip->ngpio;
>
> - for (i = 0; i < count; i++)
> - gdev->descs[i].name = names[chip->offset + i];
> + for (i = 0; i < count; i++) {
> + /*
> + * Allow overriding "fixed" names provided by the gpio
> + * provider, the "fixed" names are generally generic and less
> + * informative than the names given in device properties.
> + */
> + if (names[chip->offset + i] && names[chip->offset + i][0])
> + gdev->descs[i].name = names[chip->offset + i];
> + }
>
> kfree(names);
>
> @@ -708,10 +715,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> INIT_LIST_HEAD(&gdev->pin_ranges);
> #endif
>
> - if (gc->names)
> + if (gc->names) {
> ret = gpiochip_set_desc_names(gc);
> - else
> - ret = devprop_gpiochip_set_names(gc);
> + if (ret)
> + goto err_remove_from_list;
> + }
> + ret = devprop_gpiochip_set_names(gc);
> if (ret)
> goto err_remove_from_list;
>
> --
> 2.20.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] gpiolib: allow line names from device props to override driver names
2021-12-09 11:32 [PATCH] gpiolib: allow line names from device props to override driver names Peter Rosin
2021-12-09 12:12 ` Andy Shevchenko
@ 2021-12-10 12:24 ` Alexander Dahl
2021-12-13 14:04 ` Bartosz Golaszewski
2 siblings, 0 replies; 4+ messages in thread
From: Alexander Dahl @ 2021-12-10 12:24 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Linus Walleij, Bartosz Golaszewski, linux-gpio,
Andy Shevchenko, Alexander Dahl, Ludovic Desroches,
Nicolas Ferre, Alexandre Belloni, Roelf-Erik Carsjens
Hello Peter,
Am Thu, Dec 09, 2021 at 12:32:29PM +0100 schrieb Peter Rosin:
> Some gpio providers set names for gpio lines that match the names of
> the pins on the SoC, or variations on that theme. These names are
> generally generic, such as pioC12 in the at91 case. These generic names
> block the possibility to name gpio lines with in device properties
> (i.e. gpio-line-names).
>
> Allow overriding a generic name given by the gpio driver if there is
> a name given to the gpio line using device properties, but leave the
> generic name alone if no better name is available.
I think this is a good solution. For example on at91 if someone did
not set gpio-line-names yet, and relied on the generic names (PA0,
PA1, etc.), she won't get new names until she sets line names in dts.
This at least allows for transitioning dts / userspace at the same
time, without the kernel getting in the way.
> However, there is a risk. If user space is depending on the above
> mentioned fixed gpio names, AND there are device properties that
> previously did not reach the surface, the name change might cause
> regressions. But hopefully this stays below the radar...
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
I backported the patch to v5.10.65 and tested, this is part of the
result on a custom board, where line names were set for the sd card
interface and an I²C port:
root@DistroKit:~ gpioinfo | head -n20
gpiochip0 - 128 lines:
line 0: "SDMMC0_CK" unused input active-high
line 1: "SDMMC0_CMD" unused input active-high
line 2: "SDMMC0_DAT0" unused input active-high
line 3: "SDMMC0_DAT1" unused input active-high
line 4: "SDMMC0_DAT2" unused input active-high
line 5: "SDMMC0_DAT3" unused input active-high
line 6: "TWD" unused input active-high
line 7: "TWCK" unused input active-high
line 8: "PA8" unused input active-high
line 9: "PA9" unused input active-high
line 10: "PA10" unused input active-high
line 11: "PA11" unused input active-high
line 12: "PA12" unused input active-high
line 13: "SDMMC0_CD" unused input active-high
line 14: "PA14" unused input active-high
line 15: "PA15" unused input active-high
line 16: "PA16" unused input active-high
line 17: "PA17" unused input active-high
line 18: "PA18" unused input active-high
So if you're okay with testing a backport, you may add my
Tested-by: Alexander Dahl <ada@thorsis.com>
Greets
Alex
> ---
> drivers/gpio/gpiolib.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> Instead of doing this only for pinctrl-at91.c as in my recent patch [1], do
> it for everyone.
>
> Cheers,
> Peter
>
> [1] https://lore.kernel.org/lkml/4d17866a-d9a4-a3d7-189a-781d18dbea00@axentia.se/
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index abfbf546d159..00a2a689c202 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -422,8 +422,15 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
> if (count > chip->ngpio)
> count = chip->ngpio;
>
> - for (i = 0; i < count; i++)
> - gdev->descs[i].name = names[chip->offset + i];
> + for (i = 0; i < count; i++) {
> + /*
> + * Allow overriding "fixed" names provided by the gpio
> + * provider, the "fixed" names are generally generic and less
> + * informative than the names given in device properties.
> + */
> + if (names[chip->offset + i] && names[chip->offset + i][0])
> + gdev->descs[i].name = names[chip->offset + i];
> + }
>
> kfree(names);
>
> @@ -708,10 +715,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> INIT_LIST_HEAD(&gdev->pin_ranges);
> #endif
>
> - if (gc->names)
> + if (gc->names) {
> ret = gpiochip_set_desc_names(gc);
> - else
> - ret = devprop_gpiochip_set_names(gc);
> + if (ret)
> + goto err_remove_from_list;
> + }
> + ret = devprop_gpiochip_set_names(gc);
> if (ret)
> goto err_remove_from_list;
>
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] gpiolib: allow line names from device props to override driver names
2021-12-09 11:32 [PATCH] gpiolib: allow line names from device props to override driver names Peter Rosin
2021-12-09 12:12 ` Andy Shevchenko
2021-12-10 12:24 ` Alexander Dahl
@ 2021-12-13 14:04 ` Bartosz Golaszewski
2 siblings, 0 replies; 4+ messages in thread
From: Bartosz Golaszewski @ 2021-12-13 14:04 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel, Linus Walleij, linux-gpio, Andy Shevchenko,
Alexander Dahl, Ludovic Desroches, Nicolas Ferre,
Alexandre Belloni
On Thu, Dec 9, 2021 at 12:32 PM Peter Rosin <peda@axentia.se> wrote:
>
> Some gpio providers set names for gpio lines that match the names of
> the pins on the SoC, or variations on that theme. These names are
> generally generic, such as pioC12 in the at91 case. These generic names
> block the possibility to name gpio lines with in device properties
> (i.e. gpio-line-names).
>
> Allow overriding a generic name given by the gpio driver if there is
> a name given to the gpio line using device properties, but leave the
> generic name alone if no better name is available.
>
> However, there is a risk. If user space is depending on the above
> mentioned fixed gpio names, AND there are device properties that
> previously did not reach the surface, the name change might cause
> regressions. But hopefully this stays below the radar...
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> drivers/gpio/gpiolib.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> Instead of doing this only for pinctrl-at91.c as in my recent patch [1], do
> it for everyone.
>
> Cheers,
> Peter
>
> [1] https://lore.kernel.org/lkml/4d17866a-d9a4-a3d7-189a-781d18dbea00@axentia.se/
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index abfbf546d159..00a2a689c202 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -422,8 +422,15 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
> if (count > chip->ngpio)
> count = chip->ngpio;
>
> - for (i = 0; i < count; i++)
> - gdev->descs[i].name = names[chip->offset + i];
> + for (i = 0; i < count; i++) {
> + /*
> + * Allow overriding "fixed" names provided by the gpio
> + * provider, the "fixed" names are generally generic and less
> + * informative than the names given in device properties.
> + */
> + if (names[chip->offset + i] && names[chip->offset + i][0])
> + gdev->descs[i].name = names[chip->offset + i];
> + }
>
> kfree(names);
>
> @@ -708,10 +715,12 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> INIT_LIST_HEAD(&gdev->pin_ranges);
> #endif
>
> - if (gc->names)
> + if (gc->names) {
> ret = gpiochip_set_desc_names(gc);
> - else
> - ret = devprop_gpiochip_set_names(gc);
> + if (ret)
> + goto err_remove_from_list;
> + }
> + ret = devprop_gpiochip_set_names(gc);
> if (ret)
> goto err_remove_from_list;
>
> --
> 2.20.1
>
Hi Peter,
This looks good, please resend with Andy's comments addressed so that
I can pick it up.
Bart
^ permalink raw reply [flat|nested] 4+ messages in thread