diff mbox

gpio-pxa: getting GPIOs by devicetree phandle broken

Message ID CAAVeFu+yf6Wf6MspiDLiWwQkMwMvimNa0SpMK_yO_9X07UecEw@mail.gmail.com
State New
Headers show

Commit Message

Alexandre Courbot Feb. 9, 2015, 6:11 a.m. UTC 
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?

        return true;
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Robert Jarzmik Feb. 9, 2015, 8:02 a.m. UTC | #1 
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.
Hans Holmberg Feb. 9, 2015, 9:42 a.m. UTC | #2 
DQo+ID4gMS4gUmVxdWlyZSBjaGlsZCBub2RlcyBpbiBEVCBmb3IgZWFjaCBiYW5rDQo+IA0KPiBU
aGlzIHdvdWxkIGJyZWFrIERUIGNvbXBhdGliaWxpdHkuDQo+IA0KPiA+IDIuIFJlZmFjdG9yIGdw
aW8tcHhhIHRvIG9ubHkgcmVnaXN0ZXIgb25lIGdwaW9jaGlwDQo+IA0KPiBTb3VuZHMgYmV0dGVy
LCBlc3BlY2lhbGx5IHNpbmNlIHRoaXMgd291bGQgcmVmbGVjdCB0aGUgaGFyZHdhcmUgbW9yZQ0K
PiBhY2N1cmF0ZWx5LiBPbmUgRFQgbm9kZSBzaG91bGQgdHJhbnNsYXRlIGludG8gb25lIEdQSU8g
Y2hpcC4gVGhlIHByb2JsZW0gaXMNCj4gdGhhdCBJJ20gYWZyYWlkIHNldmVyYWwgb3RoZXIgZHJp
dmVycyBhcmUgZG9pbmcgdGhlIHNhbWUgdGhpbmcgYW5kIHRodXMgYXJlDQo+IG5vdyBzaW1pbGFy
bHkgYnJva2VuLg0KDQpUaGlzIHNvdW5kcyByZWFzb25hYmxlIHRvIG1lLg0KDQo+IFRoZSBmb2xs
b3dpbmcgaXMgYWxzbyBsaWtlbHkgdG8gd29yayBhcyBhIHdvcmthcm91bmQsIGJ1dCBJIHdvdWxk
IG5vdCBnbyBhcw0KPiBmYXIgYXMgc2F5aW5nIHRoaXMgc2hvdWxkIGJlIHRha2VuIGFzIGEgZml4
LiBIYW5zLCBzaW5jZSB5b3Ugd3JvdGUgN2I4NzkyYiwNCj4gY291bGQgd2UgaGF2ZSB5b3VyIGlu
cHV0IG9uIHRoaXM/DQoNCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3Bpby9ncGlvbGliLW9mLmMg
Yi9kcml2ZXJzL2dwaW8vZ3Bpb2xpYi1vZi5jIGluZGV4DQo+IDA4MjYxZjJiM2E4Mi4uYTA5MDk1
NTMxYjAwIDEwMDY0NA0KPiAtLS0gYS9kcml2ZXJzL2dwaW8vZ3Bpb2xpYi1vZi5jDQo+ICsrKyBi
L2RyaXZlcnMvZ3Bpby9ncGlvbGliLW9mLmMNCj4gQEAgLTQ1LDE0ICs0NSwxNiBAQCBzdGF0aWMg
aW50IG9mX2dwaW9jaGlwX2ZpbmRfYW5kX3hsYXRlKHN0cnVjdA0KPiBncGlvX2NoaXAgKmdjLCB2
b2lkICpkYXRhKQ0KPiAgICAgICAgICAgICAgICAgcmV0dXJuIGZhbHNlOw0KPiANCj4gICAgICAg
ICByZXQgPSBnYy0+b2ZfeGxhdGUoZ2MsICZnZ19kYXRhLT5ncGlvc3BlYywgZ2dfZGF0YS0+Zmxh
Z3MpOw0KPiAtICAgICAgIGlmIChyZXQgPCAwKSB7DQo+ICsgICAgICAgaWYgKHJldCA9PSAtRVBS
T0JFX0RFRkVSKSB7DQo+ICAgICAgICAgICAgICAgICAvKiBXZSd2ZSBmb3VuZCB0aGUgZ3BpbyBj
aGlwLCBidXQgdGhlIHRyYW5zbGF0aW9uIGZhaWxlZC4NCj4gICAgICAgICAgICAgICAgICAqIFJl
dHVybiB0cnVlIHRvIHN0b3AgbG9va2luZyBhbmQgcmV0dXJuIHRoZSB0cmFuc2xhdGlvbg0KPiAg
ICAgICAgICAgICAgICAgICogZXJyb3IgdmlhIG91dF9ncGlvDQo+ICAgICAgICAgICAgICAgICAg
Ki8NCj4gICAgICAgICAgICAgICAgIGdnX2RhdGEtPm91dF9ncGlvID0gRVJSX1BUUihyZXQpOw0K
PiAgICAgICAgICAgICAgICAgcmV0dXJuIHRydWU7DQo+IC0gICAgICAgIH0NCj4gKyAgICAgICB9
IGVsc2UgaWYgKHJldCA8IDApIHsNCj4gKyAgICAgICAgICAgICAgIHJldHVybiBmYWxzZTsNCj4g
KyAgICAgICB9DQo+IA0KPiAgICAgICAgIGdnX2RhdGEtPm91dF9ncGlvID0gZ3Bpb2NoaXBfZ2V0
X2Rlc2MoZ2MsIHJldCk7DQo+ICAgICAgICAgcmV0dXJuIHRydWU7DQoNClRoZSBwcm9ibGVtIG15
IHBhdGNoIHNvbHZlZCB3YXMgdGhhdCAtRVBST0JFX0RFRkVSIHdhcyByZXR1cm5lZCByZWdhcmRs
ZXNzIG9mIGxvb2t1cC1lcnJvciBpbiBvZl9nZXRfbmFtZWRfZ3Bpb2RfZmxhZ3MsIG5vdCB0aGF0
IC1FUFJPQkVfREVGRVIgZXJyb3Igd2FzIG5vdCBwYXNzZWQgb24uDQoNClRoZSBpc3N1ZSB3aXRo
IG11bHRpcGxlIGdwaW9jaGlwcyBwZXIgb2Ytbm9kZSBjb3VsZCBiZSB3b3JrZWQgYXJvdW5kIGFz
IGZvbGxvd2VkIEkgYmVsaWV2ZSwgY29tbWVudHM/DQoNCmRpZmYgLS1naXQgYS9kcml2ZXJzL2dw
aW8vZ3Bpb2xpYi1vZi5jIGIvZHJpdmVycy9ncGlvL2dwaW9saWItb2YuYw0KaW5kZXggMDgyNjFm
Mi4uNDM5ODRhYiAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvZ3Bpby9ncGlvbGliLW9mLmMNCisrKyBi
L2RyaXZlcnMvZ3Bpby9ncGlvbGliLW9mLmMNCkBAIC00NywxMSArNDcsMTIgQEAgc3RhdGljIGlu
dCBvZl9ncGlvY2hpcF9maW5kX2FuZF94bGF0ZShzdHJ1Y3QgZ3Bpb19jaGlwICpnYywgdm9pZCAq
ZGF0YSkNCiAgICAgICAgcmV0ID0gZ2MtPm9mX3hsYXRlKGdjLCAmZ2dfZGF0YS0+Z3Bpb3NwZWMs
IGdnX2RhdGEtPmZsYWdzKTsNCiAgICAgICAgaWYgKHJldCA8IDApIHsNCiAgICAgICAgICAgICAg
ICAvKiBXZSd2ZSBmb3VuZCB0aGUgZ3BpbyBjaGlwLCBidXQgdGhlIHRyYW5zbGF0aW9uIGZhaWxl
ZC4NCi0gICAgICAgICAgICAgICAgKiBSZXR1cm4gdHJ1ZSB0byBzdG9wIGxvb2tpbmcgYW5kIHJl
dHVybiB0aGUgdHJhbnNsYXRpb24NCi0gICAgICAgICAgICAgICAgKiBlcnJvciB2aWEgb3V0X2dw
aW8NCisgICAgICAgICAgICAgICAgKiBTdG9yZSB0cmFuc2xhdGlvbiBlcnJvciBpbiBvdXRfZ3Bp
by4NCisgICAgICAgICAgICAgICAgKiBSZXR1cm4gZmFsc2UgdG8ga2VlcCBsb29raW5nLCBhcyBt
b3JlIHRoYW4gb25lIEdQSU8gY2hpcA0KKyAgICAgICAgICAgICAgICAqIGNvdWxkIGJlIHJlZ2lz
dGVyZWQgcGVyIG9mLW5vZGUuDQogICAgICAgICAgICAgICAgICovDQogICAgICAgICAgICAgICAg
Z2dfZGF0YS0+b3V0X2dwaW8gPSBFUlJfUFRSKHJldCk7DQotICAgICAgICAgICAgICAgcmV0dXJu
IHRydWU7DQorICAgICAgICAgICAgICAgcmV0dXJuIGZhbHNlOw0KICAgICAgICAgfQ0KIA0KICAg
ICAgICBnZ19kYXRhLT5vdXRfZ3BpbyA9IGdwaW9jaGlwX2dldF9kZXNjKGdjLCByZXQpOw0KLS0N
Cg0KQmVzdCByZWdhcmRzLA0KSGFucw0K
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tyler Hall Feb. 9, 2015, 2:41 p.m. UTC | #3 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robert Jarzmik Feb. 9, 2015, 5:38 p.m. UTC | #4 
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.
Alexandre Courbot Feb. 10, 2015, 5:33 a.m. UTC | #5 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);