diff mbox series

gpiolib: allow line names from device props to override driver names

Message ID 70b1de02-b674-ca17-9219-61fa8e1c00db@axentia.se
State New
Headers show
Series gpiolib: allow line names from device props to override driver names | expand

Commit Message

Peter Rosin Dec. 9, 2021, 11:32 a.m. UTC 
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/

Comments

Andy Shevchenko Dec. 9, 2021, 12:12 p.m. UTC | #1 
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
>
Alexander Dahl Dec. 10, 2021, 12:24 p.m. UTC | #2 
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
>
Bartosz Golaszewski Dec. 13, 2021, 2:04 p.m. UTC | #3 
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
diff mbox series

Patch

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;