| Message ID | 4d17866a-d9a4-a3d7-189a-781d18dbea00@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: at91: allow use of of gpio-line-names property | expand |
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 >
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
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.
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
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;
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