diff mbox

Davinci gpio devicetree

Message ID CA+V-a8sncfVq6x4iQxAo=te+rXJ25-mOD6rU+CFpbcypdPA_oA@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lad, Prabhakar Feb. 28, 2014, 1:51 p.m. UTC
Hi Alexander,

On Fri, Feb 28, 2014 at 2:56 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> + Prabhakar
>
> On Tuesday 25 February 2014 08:54 PM, Alexander Holler wrote:
>> Hello,
>>
>> I've seen kernel 3.14-rc contains support for gpios using devicetree.
>>
>> I've two comments:
>>
>> 1. #gpio-cells seems to be missing,
>> 2. a small usage example (e.g. with gpio-leds or gpio-keys) would be nice.
>>
Yes #gpio-cells is missing, you can refer following patch fixing it,

Regards,
--Prabhakar Lad

-----X------------X

>From e8e96492926fe74012bb8ae8163411405a12057c Mon Sep 17 00:00:00 2001
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Date: Fri, 28 Feb 2014 19:15:22 +0530
Subject: [PATCH] devicetree: bindings: gpio-davinci: fix documentation

This patch adds missing #gpio-cells and also adds a
usage example.

Reported-by: Alexander Holler <holler@ahsoftware.de>
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
 .../devicetree/bindings/gpio/gpio-davinci.txt      |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Alexander Holler March 1, 2014, 1:10 p.m. UTC | #1
Am 28.02.2014 14:51, schrieb Prabhakar Lad:
> Hi Alexander,
>
> On Fri, Feb 28, 2014 at 2:56 PM, Sekhar Nori <nsekhar@ti.com> wrote:
>> + Prabhakar
>>
>> On Tuesday 25 February 2014 08:54 PM, Alexander Holler wrote:
>>> Hello,
>>>
>>> I've seen kernel 3.14-rc contains support for gpios using devicetree.
>>>
>>> I've two comments:
>>>
>>> 1. #gpio-cells seems to be missing,
>>> 2. a small usage example (e.g. with gpio-leds or gpio-keys) would be nice.
>>>
> Yes #gpio-cells is missing, you can refer following patch fixing it,

Thanks, thats just what I've used.

I've mentioned the usage example because there should be already at 
least one example, the one used to test the patch. ;)

Maybe it makes sense to add the example to one of the provided dts too. 
I've used the da850-evm.dts as a template, and just had to add

#include <dt-bindings/gpio/gpio.h>

besides that the &gpio0 in your binding description there just has to be 
&gpio.

Regards,

Alexander Holler


>
> Regards,
> --Prabhakar Lad
>
> -----X------------X
>
>  From e8e96492926fe74012bb8ae8163411405a12057c Mon Sep 17 00:00:00 2001
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> Date: Fri, 28 Feb 2014 19:15:22 +0530
> Subject: [PATCH] devicetree: bindings: gpio-davinci: fix documentation
>
> This patch adds missing #gpio-cells and also adds a
> usage example.
>
> Reported-by: Alexander Holler <holler@ahsoftware.de>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
>   .../devicetree/bindings/gpio/gpio-davinci.txt      |   18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> index a2e839d..fb96b94 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> @@ -8,6 +8,10 @@ Required Properties:
>
>   - gpio-controller : Marks the device node as a gpio controller.
>
> +- #gpio-cells : Should be two.
> +  - first cell is the pin number
> +  - second cell is used to specify optional parameters (unused)
> +
>   - interrupt-parent: phandle of the parent interrupt controller.
>
>   - interrupts: Array of GPIO interrupt number. Only banked or unbanked IRQs are
> @@ -27,6 +31,7 @@ Example:
>   gpio: gpio@1e26000 {
>       compatible = "ti,dm6441-gpio";
>       gpio-controller;
> +    #gpio-cells = <2>;
>       reg = <0x226000 0x1000>;
>       interrupt-parent = <&intc>;
>       interrupts = <42 IRQ_TYPE_EDGE_BOTH 43 IRQ_TYPE_EDGE_BOTH
> @@ -39,3 +44,16 @@ gpio: gpio@1e26000 {
>       interrupt-controller;
>       #interrupt-cells = <2>;
>   };
> +
> +leds {
> +    compatible = "gpio-leds";
> +    led1 {
> +        label = "davinci:green:usr1";
> +        gpios = <&gpio0 10 GPIO_ACTIVE_HIGH>;
> +    };
> +
> +    led2 {
> +        label = "davinci:red:debug1";
> +        gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>;
> +    };
> +};
>
Alexander Holler March 1, 2014, 1:28 p.m. UTC | #2
Hello,

Having had a second look at your example (comparing with what I've used 
here), I think it might make sense to change it a bit:

Am 01.03.2014 14:10, schrieb Alexander Holler:
 > Am 28.02.2014 14:51, schrieb Prabhakar Lad:

 >> +leds {
         pinctrl-names = "default";
         pinctrl-0 = <&led_pins>;
 >> +    compatible = "gpio-leds";
 >> +    led1 {
 >> +        label = "davinci:green:usr1";
 >> +        gpios = <&gpio0 10 GPIO_ACTIVE_HIGH>;
             linux,default-trigger = "heartbeat";
 >> +    };
 >> +
 >> +    led2 {
 >> +        label = "davinci:red:debug1";
 >> +        gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>;
 >> +    };
 >> +};

or just add "..." to denote that there should/might be some additional 
stuff which doesn't really belong to the description of the gpio-binding 
(like pinctrl).

Regards,

Alexander Holler
Lad, Prabhakar March 3, 2014, 10:23 a.m. UTC | #3
Hi Alexander,

On Sat, Mar 1, 2014 at 6:58 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Hello,
>
> Having had a second look at your example (comparing with what I've used
> here), I think it might make sense to change it a bit:
>
> Am 01.03.2014 14:10, schrieb Alexander Holler:
>
>> Am 28.02.2014 14:51, schrieb Prabhakar Lad:
>
>>> +leds {
>         pinctrl-names = "default";
>         pinctrl-0 = <&led_pins>;
>
I think this can be dropped or else one might also feel led_pins are missing.

>>> +    compatible = "gpio-leds";
>>> +    led1 {
>>> +        label = "davinci:green:usr1";
>>> +        gpios = <&gpio0 10 GPIO_ACTIVE_HIGH>;
>             linux,default-trigger = "heartbeat";
>
>>> +    };
>>> +
>>> +    led2 {
>>> +        label = "davinci:red:debug1";
>>> +        gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>;
>>> +    };
>>> +};
>
> or just add "..." to denote that there should/might be some additional stuff
> which doesn't really belong to the description of the gpio-binding (like
> pinctrl).
>
I would prefer "..." instead


Sekhar If you are OK with the above changes I'll post a updated patch to DT list
aswell let me know your comments on this.

Thanks,
--Prabhakar Lad
Sekhar Nori March 3, 2014, 10:31 a.m. UTC | #4
On Monday 03 March 2014 03:53 PM, Prabhakar Lad wrote:
> Hi Alexander,
> 
> On Sat, Mar 1, 2014 at 6:58 PM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Hello,
>>
>> Having had a second look at your example (comparing with what I've used
>> here), I think it might make sense to change it a bit:
>>
>> Am 01.03.2014 14:10, schrieb Alexander Holler:
>>
>>> Am 28.02.2014 14:51, schrieb Prabhakar Lad:
>>
>>>> +leds {
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&led_pins>;
>>
> I think this can be dropped or else one might also feel led_pins are missing.
> 
>>>> +    compatible = "gpio-leds";
>>>> +    led1 {
>>>> +        label = "davinci:green:usr1";
>>>> +        gpios = <&gpio0 10 GPIO_ACTIVE_HIGH>;
>>             linux,default-trigger = "heartbeat";
>>
>>>> +    };
>>>> +
>>>> +    led2 {
>>>> +        label = "davinci:red:debug1";
>>>> +        gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>;
>>>> +    };
>>>> +};
>>
>> or just add "..." to denote that there should/might be some additional stuff
>> which doesn't really belong to the description of the gpio-binding (like
>> pinctrl).
>>
> I would prefer "..." instead
> 
> 
> Sekhar If you are OK with the above changes I'll post a updated patch to DT list
> aswell let me know your comments on this.

Yes, please post a formal patch.

Thanks,
Sekhar
Alexander Holler March 3, 2014, 10:49 p.m. UTC | #5
BTW., I wonder how to use e.g. GP6_7 (which I calc as no 103 by using 
6*16+7)?

Using e.g.

	gpios = <&gpio 103 GPIO_ACTIVE_HIGH>;

in a led definition (for da850-evm) fails because 103 is greater than 
the max. limit of 32 as set for every chip in davinci_gpio_probe() in 
gpio-davinci.c.

Regards,

Alexander Holler
Alexander Holler March 4, 2014, 11:54 a.m. UTC | #6
Am 03.03.2014 23:49, schrieb Alexander Holler:
> BTW., I wonder how to use e.g. GP6_7 (which I calc as no 103 by using
> 6*16+7)?
> 
> Using e.g.
> 
>     gpios = <&gpio 103 GPIO_ACTIVE_HIGH>;
> 
> in a led definition (for da850-evm) fails because 103 is greater than
> the max. limit of 32 as set for every chip in davinci_gpio_probe() in
> gpio-davinci.c.

To be a bit more precise, I wonder how one is able to use one of those 4
other gpio_chips gpio-davinci registers through an entry with ti,ngpio
set to 144, like it's done in da850.dtsi. With such an entry,
gpio-davinci registers 5 gpio_chips (4 with 32 gpios and one with 16
gpios), but I think currently one is only able to use the first 32 gpios
from within a dt.

Maybe I'm missing some magic somewhere.

I've added two other people to cc from which I think they might be
interested in that topic.

Regards,

Alexander Holler
Alexander Holler March 4, 2014, 2:41 p.m. UTC | #7
Am 04.03.2014 16:12, schrieb Grygorii Strashko:
> On 03/04/2014 01:54 PM, Alexander Holler wrote:
>> Am 03.03.2014 23:49, schrieb Alexander Holler:
>>> BTW., I wonder how to use e.g. GP6_7 (which I calc as no 103 by using
>>> 6*16+7)?
>>>
>>> Using e.g.
>>>
>>>      gpios = <&gpio 103 GPIO_ACTIVE_HIGH>;
>>>
>>> in a led definition (for da850-evm) fails because 103 is greater than
>>> the max. limit of 32 as set for every chip in davinci_gpio_probe() in
>>> gpio-davinci.c.
>>
>> To be a bit more precise, I wonder how one is able to use one of those 4
>> other gpio_chips gpio-davinci registers through an entry with ti,ngpio
>> set to 144, like it's done in da850.dtsi. With such an entry,
>> gpio-davinci registers 5 gpio_chips (4 with 32 gpios and one with 16
>> gpios), but I think currently one is only able to use the first 32 gpios
>> from within a dt.
>>
>> Maybe I'm missing some magic somewhere.
>>
>> I've added two other people to cc from which I think they might be
>> interested in that topic.
>
> The gpio_chip of_xlate staff has to be added to handle this.
> (gpio-pxa.c can be used as ref)

Sounds like what I've thought. I've debugged it yesterday evening down 
to of_gpio_simple_xlate() where it ends because the gpio number 103 is 
greater than ngpio which is 32. The result is an always defered probe.

I haven't noticed it before, because I wasn't sure until when it is 
supposed to be defered, as I first had to solve some other problems 
while trying to boot a dt-davinci-kernel here.

Regards,

Alexander Holler
Grygorii Strashko March 4, 2014, 3:12 p.m. UTC | #8
On 03/04/2014 01:54 PM, Alexander Holler wrote:
> Am 03.03.2014 23:49, schrieb Alexander Holler:
>> BTW., I wonder how to use e.g. GP6_7 (which I calc as no 103 by using
>> 6*16+7)?
>>
>> Using e.g.
>>
>>      gpios = <&gpio 103 GPIO_ACTIVE_HIGH>;
>>
>> in a led definition (for da850-evm) fails because 103 is greater than
>> the max. limit of 32 as set for every chip in davinci_gpio_probe() in
>> gpio-davinci.c.
>
> To be a bit more precise, I wonder how one is able to use one of those 4
> other gpio_chips gpio-davinci registers through an entry with ti,ngpio
> set to 144, like it's done in da850.dtsi. With such an entry,
> gpio-davinci registers 5 gpio_chips (4 with 32 gpios and one with 16
> gpios), but I think currently one is only able to use the first 32 gpios
> from within a dt.
>
> Maybe I'm missing some magic somewhere.
>
> I've added two other people to cc from which I think they might be
> interested in that topic.

The gpio_chip of_xlate staff has to be added to handle this.
(gpio-pxa.c can be used as ref)

>
> Regards,
>
> Alexander Holler
>
Lad, Prabhakar March 4, 2014, 6:30 p.m. UTC | #9
Hi Alexander,

On Tue, Mar 4, 2014 at 5:24 PM, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 03.03.2014 23:49, schrieb Alexander Holler:
>> BTW., I wonder how to use e.g. GP6_7 (which I calc as no 103 by using
>> 6*16+7)?
>>
>> Using e.g.
>>
>>     gpios = <&gpio 103 GPIO_ACTIVE_HIGH>;
>>
>> in a led definition (for da850-evm) fails because 103 is greater than
>> the max. limit of 32 as set for every chip in davinci_gpio_probe() in
>> gpio-davinci.c.
>
> To be a bit more precise, I wonder how one is able to use one of those 4
> other gpio_chips gpio-davinci registers through an entry with ti,ngpio
> set to 144, like it's done in da850.dtsi. With such an entry,
> gpio-davinci registers 5 gpio_chips (4 with 32 gpios and one with 16
> gpios), but I think currently one is only able to use the first 32 gpios
> from within a dt.
>
You got it correct it creates 5 gpio chips, with each gpio group containing
32 gpio as per  Davinci GPIO register architecture (GPIO control register)
Currently from all the GPIO's can be used, for example the dip switch (S7-8 gpio
pin 116 ) you can use the patch at [1] for testing on da850 evm.

[1]  http://git.linuxtv.org/mhadli/v4l-dvb-davinci_devices.git/commitdiff/75575f7b34d094677546f7ed094af107ebdc1e7d

> Maybe I'm missing some magic somewhere.
>
> I've added two other people to cc from which I think they might be
> interested in that topic.
>
> Regards,
>
> Alexander Holler
>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
index a2e839d..fb96b94 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
@@ -8,6 +8,10 @@  Required Properties:

 - gpio-controller : Marks the device node as a gpio controller.

+- #gpio-cells : Should be two.
+  - first cell is the pin number
+  - second cell is used to specify optional parameters (unused)
+
 - interrupt-parent: phandle of the parent interrupt controller.

 - interrupts: Array of GPIO interrupt number. Only banked or unbanked IRQs are
@@ -27,6 +31,7 @@  Example:
 gpio: gpio@1e26000 {
     compatible = "ti,dm6441-gpio";
     gpio-controller;
+    #gpio-cells = <2>;
     reg = <0x226000 0x1000>;
     interrupt-parent = <&intc>;
     interrupts = <42 IRQ_TYPE_EDGE_BOTH 43 IRQ_TYPE_EDGE_BOTH
@@ -39,3 +44,16 @@  gpio: gpio@1e26000 {
     interrupt-controller;
     #interrupt-cells = <2>;
 };
+
+leds {
+    compatible = "gpio-leds";
+    led1 {
+        label = "davinci:green:usr1";
+        gpios = <&gpio0 10 GPIO_ACTIVE_HIGH>;
+    };
+
+    led2 {
+        label = "davinci:red:debug1";
+        gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>;
+    };
+};