[PATCH] pinctrl: at91: allow use of of gpio-line-names property

[PATCH] pinctrl: at91: allow use of of gpio-line-names property

[Peter Rosin]

If no line name is given (by not having a gpio-line-names property,
or by having empty "" strings for some lines), fall back to the
existing pioC12-style line name scheme.

It is useful to be able to explicitly name lines from the schematics
or its function, rather than having the MCU names forced upon every
user.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/pinctrl/pinctrl-at91.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

I don't know if it's sane to fall back to the pioC12-style on empty
strings, or if someone adding a gpio-line-names property should be
responsible for filling in those names "by hand". I generally don't
care what "unused" pins are named, so either is fine by me...

Cheers,
Peter

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 6022496bb6a9..4f108d07e6ad 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1821,7 +1821,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	int irq, i;
 	int alias_idx = of_alias_get_id(np, "gpio");
 	uint32_t ngpio;
-	char **names;
+	const char **names;
 
 	BUG_ON(alias_idx >= ARRAY_SIZE(gpio_chips));
 	if (gpio_chips[alias_idx]) {
@@ -1890,8 +1890,15 @@ static int at91_gpio_probe(struct platform_device *pdev)
 		goto clk_enable_err;
 	}
 
-	for (i = 0; i < chip->ngpio; i++)
-		names[i] = kasprintf(GFP_KERNEL, "pio%c%d", alias_idx + 'A', i);
+	if (of_property_read_string_array(np, "gpio-line-names",
+					  names, chip->ngpio) != chip->ngpio)
+		memset(names, 0, chip->ngpio * sizeof(char *));
+
+	for (i = 0; i < chip->ngpio; i++) {
+		if (!names[i] || !names[i][0])
+			names[i] = kasprintf(GFP_KERNEL,
+					     "pio%c%d", alias_idx + 'A', i);
+	}
 
 	chip->names = (const char *const *)names;
 
-- 
2.20.1

Re: [PATCH] pinctrl: at91: allow use of of gpio-line-names property

[Alexander Dahl]

Hello Peter,

Am Tue, Dec 07, 2021 at 12:32:03AM +0100 schrieb Peter Rosin:
> If no line name is given (by not having a gpio-line-names property,
> or by having empty "" strings for some lines), fall back to the
> existing pioC12-style line name scheme.
> 
> It is useful to be able to explicitly name lines from the schematics
> or its function, rather than having the MCU names forced upon every
> user.

+1 from me. 

I asked about this some months ago, but I saw no clear
direction in the discussion. So for reference:

https://lore.kernel.org/linux-gpio/946021874.11132.1615900079722@seven.thorsis.com/

HTH & Greets
Alex

> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/pinctrl/pinctrl-at91.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> I don't know if it's sane to fall back to the pioC12-style on empty
> strings, or if someone adding a gpio-line-names property should be
> responsible for filling in those names "by hand". I generally don't
> care what "unused" pins are named, so either is fine by me...
> 
> Cheers,
> Peter
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 6022496bb6a9..4f108d07e6ad 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -1821,7 +1821,7 @@ static int at91_gpio_probe(struct platform_device *pdev)
>  	int irq, i;
>  	int alias_idx = of_alias_get_id(np, "gpio");
>  	uint32_t ngpio;
> -	char **names;
> +	const char **names;
>  
>  	BUG_ON(alias_idx >= ARRAY_SIZE(gpio_chips));
>  	if (gpio_chips[alias_idx]) {
> @@ -1890,8 +1890,15 @@ static int at91_gpio_probe(struct platform_device *pdev)
>  		goto clk_enable_err;
>  	}
>  
> -	for (i = 0; i < chip->ngpio; i++)
> -		names[i] = kasprintf(GFP_KERNEL, "pio%c%d", alias_idx + 'A', i);
> +	if (of_property_read_string_array(np, "gpio-line-names",
> +					  names, chip->ngpio) != chip->ngpio)
> +		memset(names, 0, chip->ngpio * sizeof(char *));
> +
> +	for (i = 0; i < chip->ngpio; i++) {
> +		if (!names[i] || !names[i][0])
> +			names[i] = kasprintf(GFP_KERNEL,
> +					     "pio%c%d", alias_idx + 'A', i);
> +	}
>  
>  	chip->names = (const char *const *)names;
>  
> -- 
> 2.20.1
> 

Re: [PATCH] pinctrl: at91: allow use of of gpio-line-names property

[Peter Rosin]

On 2021-12-08 13:33, Alexander Dahl wrote:
> Hello Peter,
> 
> Am Tue, Dec 07, 2021 at 12:32:03AM +0100 schrieb Peter Rosin:
>> If no line name is given (by not having a gpio-line-names property,
>> or by having empty "" strings for some lines), fall back to the
>> existing pioC12-style line name scheme.
>>
>> It is useful to be able to explicitly name lines from the schematics
>> or its function, rather than having the MCU names forced upon every
>> user.
> 
> +1 from me. 
> 
> I asked about this some months ago, but I saw no clear
> direction in the discussion. So for reference:
> 
> https://lore.kernel.org/linux-gpio/946021874.11132.1615900079722@seven.thorsis.com/
> 
> HTH & Greets
> Alex

Hi!

The last thing that was said was this, and it was left uncontended.

On 2021-03-20 12:20, Linus Walleij wrote:
> I don't think it's a big deal to change these names.

So, let's focus on that! :-)

It does indeed simplify and clarify userspace to request gpio lines
through some kind of abstraction. It makes it so much easier to
manage userspace across different generations of hardware, where
the HW designers for various reasons move things around. I guess
that makes me guilty of treating the dtb names as ABI (on other
boards), but the take-away is that it is so useful that I even
bothered to write a patch for the boards we are using but where
that scheme did not work.

Sure, I could NIH this abstraction and implement something on my
own, but it does seems like a waste to not make good use of the
gpio-lines-names information.

To me, it seems the risk is low that someone has both added
non-working gpio-line-names properties to gpio controller nodes, and
are then depending on the pioC12 style names. But then again, maybe
I'm just naïve.

Cheers,
Peter

>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/pinctrl/pinctrl-at91.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> I don't know if it's sane to fall back to the pioC12-style on empty
>> strings, or if someone adding a gpio-line-names property should be
>> responsible for filling in those names "by hand". I generally don't
>> care what "unused" pins are named, so either is fine by me...
>>
>> Cheers,
>> Peter

Re: [PATCH] pinctrl: at91: allow use of of gpio-line-names property

[Andy Shevchenko]

On Wed, Dec 8, 2021 at 4:51 PM Alexander Dahl <ada@thorsis.com> wrote:
> Am Tue, Dec 07, 2021 at 12:32:03AM +0100 schrieb Peter Rosin:
> > If no line name is given (by not having a gpio-line-names property,
> > or by having empty "" strings for some lines), fall back to the
> > existing pioC12-style line name scheme.
> >
> > It is useful to be able to explicitly name lines from the schematics
> > or its function, rather than having the MCU names forced upon every
> > user.
>
> +1 from me.
>
> I asked about this some months ago, but I saw no clear
> direction in the discussion. So for reference:
>
> https://lore.kernel.org/linux-gpio/946021874.11132.1615900079722@seven.thorsis.com/

Thanks for the reminder. AFAICS from that discussion we kinda agreed
on the names being excluded from the ABI path. Hence there is good and
bad news.

Bad one: NAK to this patch.
Good one: Please, fix this in the gpiolib respective functions to make
it once for all.

Disclaimer, I'm not a maintainer of this subsystem, I might be
perfectly wrong in my understanding of the state of affairs, let's
hear what Linus and Bart can tell us about the subject. Above are just
my wishes as a contributor to and consumer of this subsystem on how
things should be done.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: at91: allow use of of gpio-line-names property

[Peter Rosin]

Hi!

On 2021-12-08 17:28, Andy Shevchenko wrote:
> On Wed, Dec 8, 2021 at 4:51 PM Alexander Dahl <ada@thorsis.com> wrote:
>> Am Tue, Dec 07, 2021 at 12:32:03AM +0100 schrieb Peter Rosin:
>>> If no line name is given (by not having a gpio-line-names property,
>>> or by having empty "" strings for some lines), fall back to the
>>> existing pioC12-style line name scheme.
>>>
>>> It is useful to be able to explicitly name lines from the schematics
>>> or its function, rather than having the MCU names forced upon every
>>> user.
>>
>> +1 from me.
>>
>> I asked about this some months ago, but I saw no clear
>> direction in the discussion. So for reference:
>>
>> https://lore.kernel.org/linux-gpio/946021874.11132.1615900079722@seven.thorsis.com/
> 
> Thanks for the reminder. AFAICS from that discussion we kinda agreed
> on the names being excluded from the ABI path. Hence there is good and
> bad news.
> 
> Bad one: NAK to this patch.
> Good one: Please, fix this in the gpiolib respective functions to make
> it once for all.
> 
> Disclaimer, I'm not a maintainer of this subsystem, I might be
> perfectly wrong in my understanding of the state of affairs, let's
> hear what Linus and Bart can tell us about the subject. Above are just
> my wishes as a contributor to and consumer of this subsystem on how
> things should be done.

Understood, I'll send a new patch for gpiolib.c instead.

Cheers,
Peter