Re: [PATCH 2/2] gpio: of: Allow -gpio suffix for property names

Re: [PATCH 2/2] gpio: of: Allow -gpio suffix for property names

[Linus Walleij]

On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> From: Thierry Reding <treding@nvidia.com>
>
> Many bindings use the -gpio suffix in property names. Support this in
> addition to the -gpios suffix when requesting GPIOs using the new
> descriptor-based API.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Are the DT bindings really full of such ambiguity between
singular and plural? Examples?

What happens in affected drivers today? It just doesn't work?

Does that mean these bindings are not actively used by any
drivers yet so we could augment the bindings instead, or are
they already deployed so we must implement this?

Would like a word from the DT people here...

>  drivers/gpio/gpiolib.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7a0b97076374..b991462c22fb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2594,17 +2594,23 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>                                       unsigned int idx,
>                                       enum gpio_lookup_flags *flags)
>  {
> +       static const char *suffixes[] = { "gpios", "gpio" };
>         char prop_name[32]; /* 32 is max size of property name */
>         enum of_gpio_flags of_flags;
>         struct gpio_desc *desc;
> +       unsigned int i;
>
> -       if (con_id)
> -               snprintf(prop_name, 32, "%s-gpios", con_id);
> -       else
> -               snprintf(prop_name, 32, "gpios");
> +       for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
> +               if (con_id)
> +                       snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]);
> +               else
> +                       snprintf(prop_name, 32, "%s", suffixes[i]);
>
> -       desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
> -                                       &of_flags);
> +               desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
> +                                               &of_flags);
> +               if (!IS_ERR(desc))
> +                       break;
> +       }

Code snippet left for reference on devicetree ML.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: of: Allow -gpio suffix for property names

[Rob Herring]

On Thu, Apr 24, 2014 at 7:47 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>
>> From: Thierry Reding <treding@nvidia.com>
>>
>> Many bindings use the -gpio suffix in property names. Support this in
>> addition to the -gpios suffix when requesting GPIOs using the new
>> descriptor-based API.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>
> Are the DT bindings really full of such ambiguity between
> singular and plural? Examples?
>
> What happens in affected drivers today? It just doesn't work?

They mostly seem to use of_get_named_gpio.

>
> Does that mean these bindings are not actively used by any
> drivers yet so we could augment the bindings instead, or are
> they already deployed so we must implement this?
>
> Would like a word from the DT people here...

Interestingly, there is not a single occurrence of '-gpio ' in
powerpc, but a bunch in ARM. In hindsight, we should have perhaps
enforced using -gpios only, but that doesn't really matter now. We
have -gpio in use, so we need to support it. That doesn't necessarily
mean this function has to support it. For example, this function could
a legacy method and some other function should be used instead
(of_get_named_gpio perhaps).

>>  drivers/gpio/gpiolib.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 7a0b97076374..b991462c22fb 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2594,17 +2594,23 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>>                                       unsigned int idx,
>>                                       enum gpio_lookup_flags *flags)
>>  {
>> +       static const char *suffixes[] = { "gpios", "gpio" };
>>         char prop_name[32]; /* 32 is max size of property name */
>>         enum of_gpio_flags of_flags;
>>         struct gpio_desc *desc;
>> +       unsigned int i;
>>
>> -       if (con_id)
>> -               snprintf(prop_name, 32, "%s-gpios", con_id);
>> -       else
>> -               snprintf(prop_name, 32, "gpios");
>> +       for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
>> +               if (con_id)
>> +                       snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]);
>> +               else
>> +                       snprintf(prop_name, 32, "%s", suffixes[i]);

This has the side effect of searching for "gpio" as property name
which I don't think we want to allow. It also allows a DT with either
suffix to work. While I don't necessarily think the kernel's job
should be DT validation, we don't have any other validation today and
I don't see how this change simplifies the code. Between stricter DT
checking (in the kernel) and simpler code, I'd pick the latter.

Rob

Re: [PATCH 2/2] gpio: of: Allow -gpio suffix for property names

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4249 bytes --]

On Thu, Apr 24, 2014 at 09:06:24AM -0500, Rob Herring wrote:
> On Thu, Apr 24, 2014 at 7:47 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> >
> >> From: Thierry Reding <treding@nvidia.com>
> >>
> >> Many bindings use the -gpio suffix in property names. Support this in
> >> addition to the -gpios suffix when requesting GPIOs using the new
> >> descriptor-based API.
> >>
> >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >
> > Are the DT bindings really full of such ambiguity between
> > singular and plural? Examples?
> >
> > What happens in affected drivers today? It just doesn't work?
> 
> They mostly seem to use of_get_named_gpio.

Indeed. That has the downside of requiring manual parsing and handling
of the GPIO polarity, though.

> > Does that mean these bindings are not actively used by any
> > drivers yet so we could augment the bindings instead, or are
> > they already deployed so we must implement this?
> >
> > Would like a word from the DT people here...
> 
> Interestingly, there is not a single occurrence of '-gpio ' in
> powerpc, but a bunch in ARM. In hindsight, we should have perhaps
> enforced using -gpios only, but that doesn't really matter now. We
> have -gpio in use, so we need to support it.

I think I also saw a proposal only recently to add support for a
gpios/gpio-names type of binding

> That doesn't necessarily mean this function has to support it. For
> example, this function could a legacy method and some other function
> should be used instead (of_get_named_gpio perhaps).

The reason why I posted this is precisely because I wanted to convert
over some drivers to use the new helpers (because they make things like
polarity handling much easier). My first attempt was to fix the binding
because I was under the impression that -gpio usage was discouraged, but
people didn't like that because, you know, DT bindings being a stable
ABI and all that.

The downside of not allowing the gpiod API to support the -gpio suffix
is that we'll never be able to convert drivers that use such a binding
and will forever have a hodgepodge of GPIO APIs that we need to support.

> >>  drivers/gpio/gpiolib.c | 18 ++++++++++++------
> >>  1 file changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index 7a0b97076374..b991462c22fb 100644
> >> --- a/drivers/gpio/gpiolib.c
> >> +++ b/drivers/gpio/gpiolib.c
> >> @@ -2594,17 +2594,23 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
> >>                                       unsigned int idx,
> >>                                       enum gpio_lookup_flags *flags)
> >>  {
> >> +       static const char *suffixes[] = { "gpios", "gpio" };
> >>         char prop_name[32]; /* 32 is max size of property name */
> >>         enum of_gpio_flags of_flags;
> >>         struct gpio_desc *desc;
> >> +       unsigned int i;
> >>
> >> -       if (con_id)
> >> -               snprintf(prop_name, 32, "%s-gpios", con_id);
> >> -       else
> >> -               snprintf(prop_name, 32, "gpios");
> >> +       for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
> >> +               if (con_id)
> >> +                       snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]);
> >> +               else
> >> +                       snprintf(prop_name, 32, "%s", suffixes[i]);
> 
> This has the side effect of searching for "gpio" as property name
> which I don't think we want to allow.

Why don't we want to allow a "gpio" property when we already allow
"gpios"?

> It also allows a DT with either suffix to work. While I don't
> necessarily think the kernel's job should be DT validation, we don't
> have any other validation today and I don't see how this change
> simplifies the code. Between stricter DT checking (in the kernel) and
> simpler code, I'd pick the latter.

I had briefly considered adding more validation here as well, such as
refusing to hand out any GPIO with idx > 0 for the -gpio suffix, but
then opted not to do that in favour of code simplicity.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] gpio: of: Allow -gpio suffix for property names

[Stephen Warren]

On 04/24/2014 12:22 PM, Thierry Reding wrote:
...
> The downside of not allowing the gpiod API to support the -gpio suffix
> is that we'll never be able to convert drivers that use such a binding
> and will forever have a hodgepodge of GPIO APIs that we need to support.

Perhaps rather than making the existing gpiod API automatically search
for both -gpios and -gpio, we could make a new API for the other suffix,
so that driver indicate explicitly which property name they want. That
way, someone can't accidentally write -gpio in the DT and have it still
work. Or, add a parameter to the existing API, but that's probably a lot
more churn.

Re: [PATCH 2/2] gpio: of: Allow -gpio suffix for property names

[Linus Walleij]

On Fri, Apr 25, 2014 at 8:24 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/24/2014 12:22 PM, Thierry Reding wrote:
> ...
>> The downside of not allowing the gpiod API to support the -gpio suffix
>> is that we'll never be able to convert drivers that use such a binding
>> and will forever have a hodgepodge of GPIO APIs that we need to support.
>
> Perhaps rather than making the existing gpiod API automatically search
> for both -gpios and -gpio, we could make a new API for the other suffix,
> so that driver indicate explicitly which property name they want. That
> way, someone can't accidentally write -gpio in the DT and have it still
> work. Or, add a parameter to the existing API, but that's probably a lot
> more churn.

Hm, that is possible, I just worry that this will lead the DT and ACPI
semantics to diverge even more, and the present patch make things
more coherent from the framework side of things instead of even more
elaborate per-HW-info-method :-/

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: of: Allow -gpio suffix for property names

[Alexandre Courbot]

On Thu, Apr 24, 2014 at 11:06 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Apr 24, 2014 at 7:47 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Wed, Apr 23, 2014 at 5:28 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>>
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Many bindings use the -gpio suffix in property names. Support this in
>>> addition to the -gpios suffix when requesting GPIOs using the new
>>> descriptor-based API.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>
>> Are the DT bindings really full of such ambiguity between
>> singular and plural? Examples?
>>
>> What happens in affected drivers today? It just doesn't work?
>
> They mostly seem to use of_get_named_gpio.

In an idea world of_get_named_gpio() would be gpiolib-private so
people cannot come with their own custom-named DT GPIO properties.
Given its broad usage this is not possible, but maybe we can at least
do it for of_get_named_gpiod().

>
>>
>> Does that mean these bindings are not actively used by any
>> drivers yet so we could augment the bindings instead, or are
>> they already deployed so we must implement this?
>>
>> Would like a word from the DT people here...
>
> Interestingly, there is not a single occurrence of '-gpio ' in
> powerpc, but a bunch in ARM. In hindsight, we should have perhaps
> enforced using -gpios only, but that doesn't really matter now. We
> have -gpio in use, so we need to support it. That doesn't necessarily
> mean this function has to support it. For example, this function could
> a legacy method and some other function should be used instead
> (of_get_named_gpio perhaps).

It seems like we have to support that use-case indeed (many instances
in arch/arm). The incentive for handling this in that function vs.
user code is that having support here would allow drivers to directly
use gpiod_get() and having it automatically handle GPIO properties
like active-low instead of requiring user code to handle it by itself
every time.

Without this many drivers for devices using "-gpio" properties could
not switch to the new gpiod interface.

So as far as I'm concerned this code makes GPIO user-code easier. This
is not to say that we should allow that "-gpio" suffix for new
bindings.

Acked-by: Alexandre Courbot <acourbot@nvidia.com>