[PATCH/RFC,1/3] dt-bindings: i2c: i2c-gpio: Add support for named gpios

Message ID 1503566474-5335-2-git-send-email-geert+renesas@glider.be
State New
Headers show

Commit Message

Geert Uytterhoeven Aug. 24, 2017, 9:21 a.m.
The current i2c-gpio DT bindings use a single unnamed "gpios" property
to refer to the SDA and SCL signal lines by index.  This is error-prone
for the casual DT writer and reviewer, as one has to look up the order
in the DT bindings.

Fix this by amending the DT bindings to use two separate named gpios
properties, and deprecate the old unnamed variant.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/devicetree/bindings/i2c/i2c-gpio.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Simon Horman Aug. 28, 2017, 8:16 a.m. | #1
On Thu, Aug 24, 2017 at 11:21:12AM +0200, Geert Uytterhoeven wrote:
> The current i2c-gpio DT bindings use a single unnamed "gpios" property
> to refer to the SDA and SCL signal lines by index.  This is error-prone
> for the casual DT writer and reviewer, as one has to look up the order
> in the DT bindings.
> 
> Fix this by amending the DT bindings to use two separate named gpios
> properties, and deprecate the old unnamed variant.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-gpio.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> index 4f8ec947c6bd9cad..61033f73577dd70e 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> @@ -2,8 +2,9 @@ Device-Tree bindings for i2c gpio driver
>  
>  Required properties:
>  	- compatible = "i2c-gpio";
> -	- gpios: sda and scl gpio
> -
> +	- sda-gpios: gpio used for the sda signal
> +	- scl-gpios: gpio used for the scl signal
> +	- gpios: sda and scl gpio, alternative for {sda,scl}-gpios (deprecated)
>  
>  Optional properties:
>  	- i2c-gpio,sda-open-drain: sda as open drain

Maybe it would be best to update the example too?
Rob Herring Aug. 31, 2017, 5:55 p.m. | #2
On Thu, Aug 24, 2017 at 11:21:12AM +0200, Geert Uytterhoeven wrote:
> The current i2c-gpio DT bindings use a single unnamed "gpios" property
> to refer to the SDA and SCL signal lines by index.  This is error-prone
> for the casual DT writer and reviewer, as one has to look up the order
> in the DT bindings.
> 
> Fix this by amending the DT bindings to use two separate named gpios
> properties, and deprecate the old unnamed variant.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-gpio.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> index 4f8ec947c6bd9cad..61033f73577dd70e 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> @@ -2,8 +2,9 @@ Device-Tree bindings for i2c gpio driver
>  
>  Required properties:
>  	- compatible = "i2c-gpio";
> -	- gpios: sda and scl gpio
> -
> +	- sda-gpios: gpio used for the sda signal
> +	- scl-gpios: gpio used for the scl signal
> +	- gpios: sda and scl gpio, alternative for {sda,scl}-gpios (deprecated)
>  
>  Optional properties:
>  	- i2c-gpio,sda-open-drain: sda as open drain

Well, if we're going to change things, we should drop these open-drain 
properties. We have open-drain flags in gpio cells, so we don't need 
these I think.

Rob
Geert Uytterhoeven Sept. 7, 2017, 11:42 a.m. | #3
Hi Rob,

On Thu, Aug 31, 2017 at 7:55 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Aug 24, 2017 at 11:21:12AM +0200, Geert Uytterhoeven wrote:
>> The current i2c-gpio DT bindings use a single unnamed "gpios" property
>> to refer to the SDA and SCL signal lines by index.  This is error-prone
>> for the casual DT writer and reviewer, as one has to look up the order
>> in the DT bindings.
>>
>> Fix this by amending the DT bindings to use two separate named gpios
>> properties, and deprecate the old unnamed variant.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-gpio.txt | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
>> index 4f8ec947c6bd9cad..61033f73577dd70e 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
>> @@ -2,8 +2,9 @@ Device-Tree bindings for i2c gpio driver
>>
>>  Required properties:
>>       - compatible = "i2c-gpio";
>> -     - gpios: sda and scl gpio
>> -
>> +     - sda-gpios: gpio used for the sda signal
>> +     - scl-gpios: gpio used for the scl signal
>> +     - gpios: sda and scl gpio, alternative for {sda,scl}-gpios (deprecated)
>>
>>  Optional properties:
>>       - i2c-gpio,sda-open-drain: sda as open drain
>
> Well, if we're going to change things, we should drop these open-drain
> properties. We have open-drain flags in gpio cells, so we don't need
> these I think.

It depends. Some GPIO controllers use #gpio-cells = 1, lacking flags.
Furthermore, the gpio-specifiers are controller-specific, and not all GPIO
controllers supporting flags follow the semi-standard flag definitions in
<dt-bindings/gpio/gpio.h>.

See Documentation/devicetree/bindings/gpio/gpio.txt.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Rob Herring Sept. 18, 2017, 4:52 p.m. | #4
On Thu, Sep 7, 2017 at 6:42 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Rob,
>
> On Thu, Aug 31, 2017 at 7:55 PM, Rob Herring <robh@kernel.org> wrote:
>> On Thu, Aug 24, 2017 at 11:21:12AM +0200, Geert Uytterhoeven wrote:
>>> The current i2c-gpio DT bindings use a single unnamed "gpios" property
>>> to refer to the SDA and SCL signal lines by index.  This is error-prone
>>> for the casual DT writer and reviewer, as one has to look up the order
>>> in the DT bindings.
>>>
>>> Fix this by amending the DT bindings to use two separate named gpios
>>> properties, and deprecate the old unnamed variant.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  Documentation/devicetree/bindings/i2c/i2c-gpio.txt | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
>>> index 4f8ec947c6bd9cad..61033f73577dd70e 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
>>> @@ -2,8 +2,9 @@ Device-Tree bindings for i2c gpio driver
>>>
>>>  Required properties:
>>>       - compatible = "i2c-gpio";
>>> -     - gpios: sda and scl gpio
>>> -
>>> +     - sda-gpios: gpio used for the sda signal
>>> +     - scl-gpios: gpio used for the scl signal
>>> +     - gpios: sda and scl gpio, alternative for {sda,scl}-gpios (deprecated)
>>>
>>>  Optional properties:
>>>       - i2c-gpio,sda-open-drain: sda as open drain
>>
>> Well, if we're going to change things, we should drop these open-drain
>> properties. We have open-drain flags in gpio cells, so we don't need
>> these I think.
>
> It depends. Some GPIO controllers use #gpio-cells = 1, lacking flags.

We should stop allowing that or at least require that anyone using
i2c-gpio have flags. If they are added this new i2c-gpio binding, then
they can add flags too.

> Furthermore, the gpio-specifiers are controller-specific, and not all GPIO
> controllers supporting flags follow the semi-standard flag definitions in
> <dt-bindings/gpio/gpio.h>.

In theory yes, but reality is pretty much everyone follows it. There's
no other way to specify active high vs. low for example. Furthermore,
if someone wanted to do flags in their own custom way, that would
still work. It is still the controller (or GPIO core) that interprets
the flags, not the client.

This is a new binding, so only new DT will have it and we can apply
new standards.

Rob
Linus Walleij Sept. 18, 2017, 7:45 p.m. | #5
On Mon, Sep 18, 2017 at 6:52 PM, Rob Herring <robh@kernel.org> wrote:

> In theory yes, but reality is pretty much everyone follows it. There's
> no other way to specify active high vs. low for example. Furthermore,
> if someone wanted to do flags in their own custom way, that would
> still work. It is still the controller (or GPIO core) that interprets
> the flags, not the client.
>
> This is a new binding, so only new DT will have it and we can apply
> new standards.

I'd like to get a picture of any one-cell GPIO DTS:es and/or drivers
still around.

I would like to deal with them somehow.

When we started the big DT migration this was one of the areas
we made some screwups in, admittedly, but someone just has to
go first, and that was incidentally GPIO controllers.

At this time people were even playing around with DT bindings in
BNF form, which is why the GPIO binding is a bit .. esoteric
at times. I guess I should fix that.

Yours,
Linus Walleij
Rob Herring Sept. 19, 2017, 8:23 p.m. | #6
On Mon, Sep 18, 2017 at 2:45 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Sep 18, 2017 at 6:52 PM, Rob Herring <robh@kernel.org> wrote:
>
>> In theory yes, but reality is pretty much everyone follows it. There's
>> no other way to specify active high vs. low for example. Furthermore,
>> if someone wanted to do flags in their own custom way, that would
>> still work. It is still the controller (or GPIO core) that interprets
>> the flags, not the client.
>>
>> This is a new binding, so only new DT will have it and we can apply
>> new standards.
>
> I'd like to get a picture of any one-cell GPIO DTS:es and/or drivers
> still around.

$ git grep '#gpio-cells.*1'
Documentation/devicetree/bindings/gpio/gpio.txt:
#gpio-cells = <1>;
Documentation/devicetree/bindings/gpio/spear_spics.txt:  *
#gpio-cells: should be 1 and will mention chip select number
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt:
                 #gpio-cells = <1>;
Documentation/devicetree/bindings/mfd/twl6040.txt:- #gpio-cells = <1>:
twl6040 provides GPO lines.
arch/mips/boot/dts/xilfpga/nexys4ddr.dts:               #gpio-cells = <1>;

...and this one may be out of date. All other instances for Xilinx
gpio use 2 cells.

We also have to look for custom translate functions of which there are
a handful, but they all use standard flags.

>
> I would like to deal with them somehow.

Looks like we already warn in the kernel if <2. Might be worth adding
this to dtc, then I can get the warning without booting a kernel on
platforms I don't have.

> When we started the big DT migration this was one of the areas
> we made some screwups in, admittedly, but someone just has to
> go first, and that was incidentally GPIO controllers.

The move to standardize flags started before the Great ARM Conversion,
so I would have expected some old PPC DTs. But it looks like we are
good.

Rob

Patch

diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
index 4f8ec947c6bd9cad..61033f73577dd70e 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
@@ -2,8 +2,9 @@  Device-Tree bindings for i2c gpio driver
 
 Required properties:
 	- compatible = "i2c-gpio";
-	- gpios: sda and scl gpio
-
+	- sda-gpios: gpio used for the sda signal
+	- scl-gpios: gpio used for the scl signal
+	- gpios: sda and scl gpio, alternative for {sda,scl}-gpios (deprecated)
 
 Optional properties:
 	- i2c-gpio,sda-open-drain: sda as open drain