gpio-pxa: getting GPIOs by devicetree phandle broken

gpio-pxa: getting GPIOs by devicetree phandle broken

[Tyler Hall]

Hi,

Commit 7b8792b ("gpiolib: of: Correct error handling in
of_get_named_gpiod_flags") seems to break the ability to use DT
bindings to reference this driver's GPIOs by phandle for banks above
the first.

The issue is that gpio-pxa registers multiple gpio chips - one for
each bank - but they're all associated with the same DT node. The new
behavior in of_gpiochip_find_and_xlate() causes gpiochip_find() to
bail after the first chip matches and its xlate function fails.
Previously it would try all chips associated with the phandle and
pxa_gpio_of_xlate() would fail until it was called with the correct
gpiochip.

I think the new behavior of of_gpiochip_find_and_xlate() is reasonable,
so I see a couple ways of fixing gpio-pxa.

1. Require child nodes in DT for each bank
2. Refactor gpio-pxa to only register one gpiochip

There was a previously undocumented binding that required child nodes
but it was removed by 5dbb7c6 ("gpio: pxa: remove dead code"). It's
still present in the DT files that use "marvell,mmp-gpio." Here's an
example from pxa168.dtsi.

            gpio@d4019000 {
                compatible = "marvell,mmp-gpio";
                #address-cells = <1>;
                #size-cells = <1>;

                ...

                gcb0: gpio@d4019000 {
                    reg = <0xd4019000 0x4>;
                };
                ...
            };

I may proceed with option 1 since there's some precedent, but I'd
appreciate any input.

Thanks,
Tyler

Re: gpio-pxa: getting GPIOs by devicetree phandle broken

[Alexandre Courbot]

Adding Robert who reported the same thing.

On Sat, Feb 7, 2015 at 6:28 AM, Tyler Hall <tylerwhall@gmail.com> wrote:
> Hi,
>
> Commit 7b8792b ("gpiolib: of: Correct error handling in
> of_get_named_gpiod_flags") seems to break the ability to use DT
> bindings to reference this driver's GPIOs by phandle for banks above
> the first.
>
> The issue is that gpio-pxa registers multiple gpio chips - one for
> each bank - but they're all associated with the same DT node. The new
> behavior in of_gpiochip_find_and_xlate() causes gpiochip_find() to
> bail after the first chip matches and its xlate function fails.
> Previously it would try all chips associated with the phandle and
> pxa_gpio_of_xlate() would fail until it was called with the correct
> gpiochip.
>
> I think the new behavior of of_gpiochip_find_and_xlate() is reasonable,
> so I see a couple ways of fixing gpio-pxa.
>
> 1. Require child nodes in DT for each bank

This would break DT compatibility.

> 2. Refactor gpio-pxa to only register one gpiochip

Sounds better, especially since this would reflect the hardware more
accurately. One DT node should translate into one GPIO chip. The
problem is that I'm afraid several other drivers are doing the same
thing and thus are now similarly broken.

The following is also likely to work as a workaround, but I would not
go as far as saying this should be taken as a fix. Hans, since you
wrote 7b8792b, could we have your input on this?

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 08261f2b3a82..a09095531b00 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -45,14 +45,16 @@ static int of_gpiochip_find_and_xlate(struct
gpio_chip *gc, void *data)
                return false;

        ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
-       if (ret < 0) {
+       if (ret == -EPROBE_DEFER) {
                /* We've found the gpio chip, but the translation failed.
                 * Return true to stop looking and return the translation
                 * error via out_gpio
                 */
                gg_data->out_gpio = ERR_PTR(ret);
                return true;
-        }
+       } else if (ret < 0) {
+               return false;
+       }

        gg_data->out_gpio = gpiochip_get_desc(gc, ret);
        return true;

Re: gpio-pxa: getting GPIOs by devicetree phandle broken

[Robert Jarzmik]

Alexandre Courbot <gnurou@gmail.com> writes:

> Adding Robert who reported the same thing.
>
> On Sat, Feb 7, 2015 at 6:28 AM, Tyler Hall <tylerwhall@gmail.com> wrote:
>> 1. Require child nodes in DT for each bank
>
> This would break DT compatibility.
Agreed.

>
>> 2. Refactor gpio-pxa to only register one gpiochip
>
> Sounds better, especially since this would reflect the hardware more
> accurately. One DT node should translate into one GPIO chip.
Could I know where "should translate into _one_ GPIO chip" comes from ? Is it a
specification, or is it a new rule we're creating ?

And if it's a new rule, I'd like to know what backs it up .

> problem is that I'm afraid several other drivers are doing the same
> thing and thus are now similarly broken.
That's also what I'm afraid of.

> The following is also likely to work as a workaround, but I would not
> go as far as saying this should be taken as a fix. Hans, since you
> wrote 7b8792b, could we have your input on this?
I was going to suggest the very same approach. If this one (or a similar one)
doesn't meet success, then my weekend of revert queries is not over ... and I
don't like reverts.

Cheers.

-- 
Robert

RE: gpio-pxa: getting GPIOs by devicetree phandle broken

[Holmberg, Hans]

> > 1. Require child nodes in DT for each bank
> 
> This would break DT compatibility.
> 
> > 2. Refactor gpio-pxa to only register one gpiochip
> 
> Sounds better, especially since this would reflect the hardware more
> accurately. One DT node should translate into one GPIO chip. The problem is
> that I'm afraid several other drivers are doing the same thing and thus are
> now similarly broken.

This sounds reasonable to me.

> The following is also likely to work as a workaround, but I would not go as
> far as saying this should be taken as a fix. Hans, since you wrote 7b8792b,
> could we have your input on this?

> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index
> 08261f2b3a82..a09095531b00 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -45,14 +45,16 @@ static int of_gpiochip_find_and_xlate(struct
> gpio_chip *gc, void *data)
>                 return false;
> 
>         ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
> -       if (ret < 0) {
> +       if (ret == -EPROBE_DEFER) {
>                 /* We've found the gpio chip, but the translation failed.
>                  * Return true to stop looking and return the translation
>                  * error via out_gpio
>                  */
>                 gg_data->out_gpio = ERR_PTR(ret);
>                 return true;
> -        }
> +       } else if (ret < 0) {
> +               return false;
> +       }
> 
>         gg_data->out_gpio = gpiochip_get_desc(gc, ret);
>         return true;

The problem my patch solved was that -EPROBE_DEFER was returned regardless of lookup-error in of_get_named_gpiod_flags, not that -EPROBE_DEFER error was not passed on.

The issue with multiple gpiochips per of-node could be worked around as followed I believe, comments?

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 08261f2..43984ab 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -47,11 +47,12 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
        ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
        if (ret < 0) {
                /* We've found the gpio chip, but the translation failed.
-                * Return true to stop looking and return the translation
-                * error via out_gpio
+                * Store translation error in out_gpio.
+                * Return false to keep looking, as more than one GPIO chip
+                * could be registered per of-node.
                 */
                gg_data->out_gpio = ERR_PTR(ret);
-               return true;
+               return false;
         }
 
        gg_data->out_gpio = gpiochip_get_desc(gc, ret);
--

Best regards,
Hans

Re: gpio-pxa: getting GPIOs by devicetree phandle broken

[Tyler Hall]

> The issue with multiple gpiochips per of-node could be worked around as followed I believe, comments?
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 08261f2..43984ab 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -47,11 +47,12 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
>         ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
>         if (ret < 0) {
>                 /* We've found the gpio chip, but the translation failed.
> -                * Return true to stop looking and return the translation
> -                * error via out_gpio
> +                * Store translation error in out_gpio.
> +                * Return false to keep looking, as more than one GPIO chip
> +                * could be registered per of-node.
>                  */
>                 gg_data->out_gpio = ERR_PTR(ret);
> -               return true;
> +               return false;
>          }
>
>         gg_data->out_gpio = gpiochip_get_desc(gc, ret);

As long as we're ok with multiple gpiochips per of-node, this would
work for me. It'll change the preference of which chip returns the
error in the case of multiple chips, but that's already undefined
behavior.

Re: gpio-pxa: getting GPIOs by devicetree phandle broken

[Robert Jarzmik]

Tyler Hall <tylerwhall@gmail.com> writes:

>> The issue with multiple gpiochips per of-node could be worked around as followed I believe, comments?
>>
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> index 08261f2..43984ab 100644
>> --- a/drivers/gpio/gpiolib-of.c
>> +++ b/drivers/gpio/gpiolib-of.c
>> @@ -47,11 +47,12 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
>>         ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
>>         if (ret < 0) {
>>                 /* We've found the gpio chip, but the translation failed.
>> -                * Return true to stop looking and return the translation
>> -                * error via out_gpio
>> +                * Store translation error in out_gpio.
>> +                * Return false to keep looking, as more than one GPIO chip
>> +                * could be registered per of-node.
>>                  */
>>                 gg_data->out_gpio = ERR_PTR(ret);
>> -               return true;
>> +               return false;
>>          }
>>
>>         gg_data->out_gpio = gpiochip_get_desc(gc, ret);
>
> As long as we're ok with multiple gpiochips per of-node, this would
> work for me. It'll change the preference of which chip returns the
> error in the case of multiple chips, but that's already undefined
> behavior.

Looks good to me too, this will solve my issue, and the global behavior would be
consistent with the former one.

Would you care submitting a proper patch so that we can apply our Reviewed-by,
Tested-by etc ... ?

Cheers.

-- 
Robert

Re: gpio-pxa: getting GPIOs by devicetree phandle broken

[Alexandre Courbot]

On Tue, Feb 10, 2015 at 2:38 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Tyler Hall <tylerwhall@gmail.com> writes:
>
>>> The issue with multiple gpiochips per of-node could be worked around as followed I believe, comments?
>>>
>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>>> index 08261f2..43984ab 100644
>>> --- a/drivers/gpio/gpiolib-of.c
>>> +++ b/drivers/gpio/gpiolib-of.c
>>> @@ -47,11 +47,12 @@ static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
>>>         ret = gc->of_xlate(gc, &gg_data->gpiospec, gg_data->flags);
>>>         if (ret < 0) {
>>>                 /* We've found the gpio chip, but the translation failed.
>>> -                * Return true to stop looking and return the translation
>>> -                * error via out_gpio
>>> +                * Store translation error in out_gpio.
>>> +                * Return false to keep looking, as more than one GPIO chip
>>> +                * could be registered per of-node.
>>>                  */
>>>                 gg_data->out_gpio = ERR_PTR(ret);
>>> -               return true;
>>> +               return false;
>>>          }
>>>
>>>         gg_data->out_gpio = gpiochip_get_desc(gc, ret);
>>
>> As long as we're ok with multiple gpiochips per of-node, this would
>> work for me. It'll change the preference of which chip returns the
>> error in the case of multiple chips, but that's already undefined
>> behavior.
>
> Looks good to me too, this will solve my issue, and the global behavior would be
> consistent with the former one.
>
> Would you care submitting a proper patch so that we can apply our Reviewed-by,
> Tested-by etc ... ?

Looks ok to me too, if only a little bit hackish. A patch would be
appreciated so we can send it for -fixes as well.