Re: [PATCH] gpiolib: allow line names from device props to override driver names

[Alexander Dahl]

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@xxxxxxxxxx>

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@xxxxxxxxxxx>

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@xxxxxxxxxx/
> 
> 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
>