diff mbox series

[2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example

Message ID 20230529074302.3612294-2-carlos.song@nxp.com
State New
Headers
Series [1/2] i2c: imx-lpi2c: add bus recovery feature |

Commit Message

Carlos Song May 29, 2023, 7:43 a.m. UTC 
  From: Clark Wang <xiaoning.wang@nxp.com>

Add i2c bus recovery configuration example.

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
 .../devicetree/bindings/i2c/i2c-imx-lpi2c.yaml   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Krzysztof Kozlowski May 30, 2023, 2:58 p.m. UTC | #1 
On 29/05/2023 09:43, carlos.song@nxp.com wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
> 
> Add i2c bus recovery configuration example.

Why? That's just example... also with coding style issue.

> 
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
>  .../devicetree/bindings/i2c/i2c-imx-lpi2c.yaml   | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
> index 4656f5112b84..62ee457496e4 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
> @@ -58,6 +58,16 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  pinctrl-names:
> +    minItems: 1
> +    maxItems: 3

What's the benefit of this? Entries should be defined but without it is
not really helpful. Anyway not explained in commit msg.

> +
> +  scl-gpios:
> +    maxItems: 1
> +
> +  sda-gpios:
> +    maxItems: 1

You don't need these two. Anyway not explained in commit msg.

> +
>  required:
>    - compatible
>    - reg
> @@ -70,6 +80,7 @@ examples:
>    - |
>      #include <dt-bindings/clock/imx7ulp-clock.h>
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/gpio/gpio.h>
>  
>      i2c@40a50000 {
>          compatible = "fsl,imx7ulp-lpi2c";
> @@ -78,4 +89,9 @@ examples:
>          interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
>          clocks = <&clks IMX7ULP_CLK_LPI2C7>,
>                   <&clks IMX7ULP_CLK_NIC1_BUS_DIV>;
> +        pinctrl-names = "default","gpio";

Missing space.

> +        pinctrl-0 = <&pinctrl_i2c>;
> +        pinctrl-1 = <&pinctrl_i2c_recovery>;
> +        scl-gpios = <&gpio5 14 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +        sda-gpios = <&gpio5 15 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>      };

Best regards,
Krzysztof
Carlos Song May 31, 2023, 10:22 a.m. UTC | #2 
Hi,
	Thanks for you reply. 
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, May 30, 2023 10:59 PM
> To: Carlos Song <carlos.song@nxp.com>; Aisheng Dong
> <aisheng.dong@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> Anson.Huang@nxp.com
> Cc: Clark Wang <xiaoning.wang@nxp.com>; Bough Chen
> <haibo.chen@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> linux-i2c@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery
> example
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> On 29/05/2023 09:43, carlos.song@nxp.com wrote:
> > From: Clark Wang <xiaoning.wang@nxp.com>
> >
> > Add i2c bus recovery configuration example.
> 
> Why? That's just example... also with coding style issue.
> 
> >
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > ---
> >  .../devicetree/bindings/i2c/i2c-imx-lpi2c.yaml   | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
> > b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
> > index 4656f5112b84..62ee457496e4 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
> > @@ -58,6 +58,16 @@ properties:
> >    power-domains:
> >      maxItems: 1
> >
> > +  pinctrl-names:
> > +    minItems: 1
> > +    maxItems: 3
> 
> What's the benefit of this? Entries should be defined but without it is not really
> helpful. Anyway not explained in commit msg.
> 
> > +
> > +  scl-gpios:
> > +    maxItems: 1
> > +
> > +  sda-gpios:
> > +    maxItems: 1
> 
> You don't need these two. Anyway not explained in commit msg.
> 

Sorry for confusing you with the poor commit log and without
full description.

The reason why we need sending the patch for dt-binding is :
We sent out a patch for I.MX LPI2C bus support recovery function.
When LPI2C use recovery function, lpi2c controller need to switch the 
SCL pin and SDA pin to their GPIO function.  So I think the scl-gpio and
sda-gpio property need to be added in the dt-bindings.

And alternative pinmux settings are described in a separate pinctrl state "gpio". 
So maybe "gpio" pinctrl item need to be added.

I would like to know whether the above changes are really unnecessary according to above case?
Or because of the vague commit log, you are misled and think that our patch is not necessary to add examples.

Is there no need to add sda/scl-gpios property or no need to add maxItems: 1?
We also find the sci-gpio and sda-gpio have been defined in the ref: /schemas/i2c/i2c-controller.yaml. 
So is this the root cause of no need to add these properties?

Thanks!
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -70,6 +80,7 @@ examples:
> >    - |
> >      #include <dt-bindings/clock/imx7ulp-clock.h>
> >      #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> >
> >      i2c@40a50000 {
> >          compatible = "fsl,imx7ulp-lpi2c"; @@ -78,4 +89,9 @@ examples:
> >          interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> >          clocks = <&clks IMX7ULP_CLK_LPI2C7>,
> >                   <&clks IMX7ULP_CLK_NIC1_BUS_DIV>;
> > +        pinctrl-names = "default","gpio";
> 
> Missing space.
> 
> > +        pinctrl-0 = <&pinctrl_i2c>;
> > +        pinctrl-1 = <&pinctrl_i2c_recovery>;
> > +        scl-gpios = <&gpio5 14 (GPIO_ACTIVE_HIGH |
> GPIO_OPEN_DRAIN)>;
> > +        sda-gpios = <&gpio5 15 (GPIO_ACTIVE_HIGH |
> GPIO_OPEN_DRAIN)>;
> >      };
> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski May 31, 2023, 7:50 p.m. UTC | #3 
On 31/05/2023 12:22, Carlos Song wrote:
> Hi,
> 	Thanks for you reply. 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Tuesday, May 30, 2023 10:59 PM
>> To: Carlos Song <carlos.song@nxp.com>; Aisheng Dong
>> <aisheng.dong@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de;
>> kernel@pengutronix.de; festevam@gmail.com; robh+dt@kernel.org;
>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>> Anson.Huang@nxp.com
>> Cc: Clark Wang <xiaoning.wang@nxp.com>; Bough Chen
>> <haibo.chen@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>> linux-i2c@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Subject: [EXT] Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery
>> example
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report this
>> email' button
>>
>>
>> On 29/05/2023 09:43, carlos.song@nxp.com wrote:
>>> From: Clark Wang <xiaoning.wang@nxp.com>
>>>
>>> Add i2c bus recovery configuration example.
>>
>> Why? That's just example... also with coding style issue.
>>
>>>
>>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
>>> Signed-off-by: Carlos Song <carlos.song@nxp.com>
>>> ---
>>>  .../devicetree/bindings/i2c/i2c-imx-lpi2c.yaml   | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>>> b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>>> index 4656f5112b84..62ee457496e4 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>>> @@ -58,6 +58,16 @@ properties:
>>>    power-domains:
>>>      maxItems: 1
>>>
>>> +  pinctrl-names:
>>> +    minItems: 1
>>> +    maxItems: 3
>>
>> What's the benefit of this? Entries should be defined but without it is not really
>> helpful. Anyway not explained in commit msg.
>>
>>> +
>>> +  scl-gpios:
>>> +    maxItems: 1
>>> +
>>> +  sda-gpios:
>>> +    maxItems: 1
>>
>> You don't need these two. Anyway not explained in commit msg.
>>
> 
> Sorry for confusing you with the poor commit log and without
> full description.
> 
> The reason why we need sending the patch for dt-binding is :
> We sent out a patch for I.MX LPI2C bus support recovery function.
> When LPI2C use recovery function, lpi2c controller need to switch the 
> SCL pin and SDA pin to their GPIO function.  So I think the scl-gpio and
> sda-gpio property need to be added in the dt-bindings.

Why do you think they are not in the bindings already?

> 
> And alternative pinmux settings are described in a separate pinctrl state "gpio". 
> So maybe "gpio" pinctrl item need to be added.
> 
> I would like to know whether the above changes are really unnecessary according to above case?
> Or because of the vague commit log, you are misled and think that our patch is not necessary to add examples.


I claim your patch has zero effect. Can you prove otherwise?

Proof is with DTS example and result of dtbs_check.

> 
> Is there no need to add sda/scl-gpios property or no need to add maxItems: 1?

I think entire patch can be dropped.

> We also find the sci-gpio and sda-gpio have been defined in the ref: /schemas/i2c/i2c-controller.yaml. 
> So is this the root cause of no need to add these properties?

Yes.


Best regards,
Krzysztof
Krzysztof Kozlowski June 2, 2023, 1:18 p.m. UTC | #4 
Resending as my previous email probably got lost. If you got it twice,
apologies.

On 31/05/2023 12:22, Carlos Song wrote:
> Hi,
> 	Thanks for you reply. 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Tuesday, May 30, 2023 10:59 PM
>> To: Carlos Song <carlos.song@nxp.com>; Aisheng Dong
>> <aisheng.dong@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de;
>> kernel@pengutronix.de; festevam@gmail.com; robh+dt@kernel.org;
>> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
>> Anson.Huang@nxp.com
>> Cc: Clark Wang <xiaoning.wang@nxp.com>; Bough Chen
>> <haibo.chen@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
>> linux-i2c@vger.kernel.org; devicetree@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Subject: [EXT] Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery
>> example
>>
>> Caution: This is an external email. Please take care when clicking links or
>> opening attachments. When in doubt, report the message using the 'Report this
>> email' button
>>
>>
>> On 29/05/2023 09:43, carlos.song@nxp.com wrote:
>>> From: Clark Wang <xiaoning.wang@nxp.com>
>>>
>>> Add i2c bus recovery configuration example.
>>
>> Why? That's just example... also with coding style issue.
>>
>>>
>>> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
>>> Signed-off-by: Carlos Song <carlos.song@nxp.com>
>>> ---
>>>  .../devicetree/bindings/i2c/i2c-imx-lpi2c.yaml   | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>>> b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>>> index 4656f5112b84..62ee457496e4 100644
>>> --- a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
>>> @@ -58,6 +58,16 @@ properties:
>>>    power-domains:
>>>      maxItems: 1
>>>
>>> +  pinctrl-names:
>>> +    minItems: 1
>>> +    maxItems: 3
>>
>> What's the benefit of this? Entries should be defined but without it is not really
>> helpful. Anyway not explained in commit msg.
>>
>>> +
>>> +  scl-gpios:
>>> +    maxItems: 1
>>> +
>>> +  sda-gpios:
>>> +    maxItems: 1
>>
>> You don't need these two. Anyway not explained in commit msg.
>>
> 
> Sorry for confusing you with the poor commit log and without
> full description.
> 
> The reason why we need sending the patch for dt-binding is :
> We sent out a patch for I.MX LPI2C bus support recovery function.
> When LPI2C use recovery function, lpi2c controller need to switch the 
> SCL pin and SDA pin to their GPIO function.  So I think the scl-gpio and
> sda-gpio property need to be added in the dt-bindings.

Why do you think they are not in the bindings already?

> 
> And alternative pinmux settings are described in a separate pinctrl state "gpio". 
> So maybe "gpio" pinctrl item need to be added.
> 
> I would like to know whether the above changes are really unnecessary according to above case?
> Or because of the vague commit log, you are misled and think that our patch is not necessary to add examples.


I claim your patch has zero effect. Can you prove otherwise?

Proof is with DTS example and result of dtbs_check.

> 
> Is there no need to add sda/scl-gpios property or no need to add maxItems: 1?

I think entire patch can be dropped.

> We also find the sci-gpio and sda-gpio have been defined in the ref: /schemas/i2c/i2c-controller.yaml. 
> So is this the root cause of no need to add these properties?

Yes.


Best regards,
Krzysztof
Andi Shyti June 13, 2023, 10:42 p.m. UTC | #5 
Hi,

> > We also find the sci-gpio and sda-gpio have been defined in the ref: /schemas/i2c/i2c-controller.yaml. 
> > So is this the root cause of no need to add these properties?
> 
> Yes.

is some cleanup needed also in i2c-imx.yaml?

Andi
Carlos Song July 24, 2023, 9:48 a.m. UTC | #6 
Hi, Andy

Thank you very much for your advice!
I have re-made my patches based on your suggestion.

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Wednesday, June 14, 2023 6:42 AM
> To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Cc: Carlos Song <carlos.song@nxp.com>; Aisheng Dong
> <aisheng.dong@nxp.com>; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; festevam@gmail.com; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> Anson.Huang@nxp.com; Clark Wang <xiaoning.wang@nxp.com>; Bough Chen
> <haibo.chen@nxp.com>; dl-linux-imx <linux-imx@nxp.com>;
> linux-i2c@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery
> example
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> Hi,
> 
> > > We also find the sci-gpio and sda-gpio have been defined in the ref:
> /schemas/i2c/i2c-controller.yaml.
> > > So is this the root cause of no need to add these properties?
> >
> > Yes.
> 
> is some cleanup needed also in i2c-imx.yaml?
> 

Carlos: I will not upstream any patch for i2c-imx.yaml. I will drop it.

> Andi
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
index 4656f5112b84..62ee457496e4 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx-lpi2c.yaml
@@ -58,6 +58,16 @@  properties:
   power-domains:
     maxItems: 1
 
+  pinctrl-names:
+    minItems: 1
+    maxItems: 3
+
+  scl-gpios:
+    maxItems: 1
+
+  sda-gpios:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -70,6 +80,7 @@  examples:
   - |
     #include <dt-bindings/clock/imx7ulp-clock.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/gpio/gpio.h>
 
     i2c@40a50000 {
         compatible = "fsl,imx7ulp-lpi2c";
@@ -78,4 +89,9 @@  examples:
         interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
         clocks = <&clks IMX7ULP_CLK_LPI2C7>,
                  <&clks IMX7ULP_CLK_NIC1_BUS_DIV>;
+        pinctrl-names = "default","gpio";
+        pinctrl-0 = <&pinctrl_i2c>;
+        pinctrl-1 = <&pinctrl_i2c_recovery>;
+        scl-gpios = <&gpio5 14 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+        sda-gpios = <&gpio5 15 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
     };