linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature
@ 2023-05-29  7:43 carlos.song
  2023-05-29  7:43 ` [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example carlos.song
  2023-06-13 23:19 ` [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature Andi Shyti
  0 siblings, 2 replies; 9+ messages in thread
From: carlos.song @ 2023-05-29  7:43 UTC (permalink / raw)
  To: aisheng.dong, shawnguo, s.hauer, kernel, festevam, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, Anson.Huang
  Cc: carlos.song, xiaoning.wang, haibo.chen, linux-imx, linux-i2c,
	devicetree, linux-arm-kernel, linux-kernel

From: Clark Wang <xiaoning.wang@nxp.com>

Add bus recovery feature for LPI2C.
Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.

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

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 62111fe9f207..40a4420d4c12 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -107,6 +108,11 @@ struct lpi2c_imx_struct {
 	unsigned int		txfifosize;
 	unsigned int		rxfifosize;
 	enum lpi2c_imx_mode	mode;
+
+	struct i2c_bus_recovery_info rinfo;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_pins_default;
+	struct pinctrl_state *pinctrl_pins_gpio;
 };
 
 static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx,
@@ -134,6 +140,8 @@ static int lpi2c_imx_bus_busy(struct lpi2c_imx_struct *lpi2c_imx)
 
 		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
 			dev_dbg(&lpi2c_imx->adapter.dev, "bus not work\n");
+			if (lpi2c_imx->adapter.bus_recovery_info)
+				i2c_recover_bus(&lpi2c_imx->adapter);
 			return -ETIMEDOUT;
 		}
 		schedule();
@@ -191,6 +199,8 @@ static void lpi2c_imx_stop(struct lpi2c_imx_struct *lpi2c_imx)
 
 		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
 			dev_dbg(&lpi2c_imx->adapter.dev, "stop timeout\n");
+			if (lpi2c_imx->adapter.bus_recovery_info)
+				i2c_recover_bus(&lpi2c_imx->adapter);
 			break;
 		}
 		schedule();
@@ -328,6 +338,8 @@ static int lpi2c_imx_txfifo_empty(struct lpi2c_imx_struct *lpi2c_imx)
 
 		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
 			dev_dbg(&lpi2c_imx->adapter.dev, "txfifo empty timeout\n");
+			if (lpi2c_imx->adapter.bus_recovery_info)
+				i2c_recover_bus(&lpi2c_imx->adapter);
 			return -ETIMEDOUT;
 		}
 		schedule();
@@ -533,6 +545,71 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void lpi2c_imx_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct lpi2c_imx_struct *lpi2c_imx;
+
+	lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter);
+
+	pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_gpio);
+}
+
+static void lpi2c_imx_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct lpi2c_imx_struct *lpi2c_imx;
+
+	lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter);
+
+	pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_default);
+}
+
+/*
+ * We switch SCL and SDA to their GPIO function and do some bitbanging
+ * for bus recovery. These alternative pinmux settings can be
+ * described in the device tree by a separate pinctrl state "gpio". If
+ * this is missing this is not a big problem, the only implication is
+ * that we can't do bus recovery.
+ */
+static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx,
+		struct platform_device *pdev)
+{
+	struct i2c_bus_recovery_info *rinfo = &lpi2c_imx->rinfo;
+
+	lpi2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!lpi2c_imx->pinctrl || IS_ERR(lpi2c_imx->pinctrl)) {
+		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
+		return PTR_ERR(lpi2c_imx->pinctrl);
+	}
+
+	lpi2c_imx->pinctrl_pins_default = pinctrl_lookup_state(lpi2c_imx->pinctrl,
+			PINCTRL_STATE_DEFAULT);
+	lpi2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(lpi2c_imx->pinctrl,
+			"gpio");
+	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
+	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
+
+	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
+	    PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (IS_ERR(rinfo->sda_gpiod) ||
+		   IS_ERR(rinfo->scl_gpiod) ||
+		   IS_ERR(lpi2c_imx->pinctrl_pins_default) ||
+		   IS_ERR(lpi2c_imx->pinctrl_pins_gpio)) {
+		dev_dbg(&pdev->dev, "recovery information incomplete\n");
+		return 0;
+	}
+
+	dev_info(&pdev->dev, "using scl%s for recovery\n",
+		 rinfo->sda_gpiod ? ",sda" : "");
+
+	rinfo->prepare_recovery = lpi2c_imx_prepare_recovery;
+	rinfo->unprepare_recovery = lpi2c_imx_unprepare_recovery;
+	rinfo->recover_bus = i2c_generic_scl_recovery;
+	lpi2c_imx->adapter.bus_recovery_info = rinfo;
+
+	return 0;
+}
+
 static u32 lpi2c_imx_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
@@ -611,6 +688,12 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
 	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
 
+	/* Init optional bus recovery function */
+	ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev);
+	/* Give it another chance if pinctrl used is not ready yet */
+	if (ret == -EPROBE_DEFER)
+		goto rpm_disable;
+
 	ret = i2c_add_adapter(&lpi2c_imx->adapter);
 	if (ret)
 		goto rpm_disable;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example
  2023-05-29  7:43 [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature carlos.song
@ 2023-05-29  7:43 ` carlos.song
  2023-05-30 14:58   ` Krzysztof Kozlowski
  2023-06-13 23:19 ` [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature Andi Shyti
  1 sibling, 1 reply; 9+ messages in thread
From: carlos.song @ 2023-05-29  7:43 UTC (permalink / raw)
  To: aisheng.dong, shawnguo, s.hauer, kernel, festevam, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, Anson.Huang
  Cc: carlos.song, xiaoning.wang, haibo.chen, linux-imx, linux-i2c,
	devicetree, linux-arm-kernel, linux-kernel

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(+)

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example
  2023-05-29  7:43 ` [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example carlos.song
@ 2023-05-30 14:58   ` Krzysztof Kozlowski
  2023-05-31 10:22     ` [EXT] " Carlos Song
  0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-30 14:58 UTC (permalink / raw)
  To: carlos.song, aisheng.dong, shawnguo, s.hauer, kernel, festevam,
	robh+dt, krzysztof.kozlowski+dt, conor+dt, Anson.Huang
  Cc: xiaoning.wang, haibo.chen, linux-imx, linux-i2c, devicetree,
	linux-arm-kernel, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example
  2023-05-30 14:58   ` Krzysztof Kozlowski
@ 2023-05-31 10:22     ` Carlos Song
  2023-06-02 13:18       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 9+ messages in thread
From: Carlos Song @ 2023-05-31 10:22 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Aisheng Dong, shawnguo, s.hauer, kernel,
	festevam, robh+dt, krzysztof.kozlowski+dt, conor+dt, Anson.Huang
  Cc: Clark Wang, Bough Chen, dl-linux-imx, linux-i2c, devicetree,
	linux-arm-kernel, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [EXT] Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example
  2023-05-31 10:22     ` [EXT] " Carlos Song
@ 2023-06-02 13:18       ` Krzysztof Kozlowski
  2023-06-13 22:42         ` Andi Shyti
  0 siblings, 1 reply; 9+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-02 13:18 UTC (permalink / raw)
  To: Carlos Song, Aisheng Dong, shawnguo, s.hauer, kernel, festevam,
	robh+dt, krzysztof.kozlowski+dt, conor+dt, Anson.Huang
  Cc: Clark Wang, Bough Chen, dl-linux-imx, linux-i2c, devicetree,
	linux-arm-kernel, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [EXT] Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example
  2023-06-02 13:18       ` Krzysztof Kozlowski
@ 2023-06-13 22:42         ` Andi Shyti
  2023-07-24  9:48           ` Carlos Song
  0 siblings, 1 reply; 9+ messages in thread
From: Andi Shyti @ 2023-06-13 22:42 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Carlos Song, Aisheng Dong, shawnguo, s.hauer, kernel, festevam,
	robh+dt, krzysztof.kozlowski+dt, conor+dt, Anson.Huang,
	Clark Wang, Bough Chen, dl-linux-imx, linux-i2c, devicetree,
	linux-arm-kernel, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature
  2023-05-29  7:43 [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature carlos.song
  2023-05-29  7:43 ` [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example carlos.song
@ 2023-06-13 23:19 ` Andi Shyti
  2023-07-24 10:11   ` [EXT] " Carlos Song
  1 sibling, 1 reply; 9+ messages in thread
From: Andi Shyti @ 2023-06-13 23:19 UTC (permalink / raw)
  To: carlos.song
  Cc: aisheng.dong, shawnguo, s.hauer, kernel, festevam, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, Anson.Huang, xiaoning.wang,
	haibo.chen, linux-imx, linux-i2c, devicetree, linux-arm-kernel,
	linux-kernel

Hi,

On Mon, May 29, 2023 at 03:43:01PM +0800, carlos.song@nxp.com wrote:
> From: Clark Wang <xiaoning.wang@nxp.com>
> 
> Add bus recovery feature for LPI2C.
> Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.

please update the commit message according to the dts changes, as
well.

[...]

> +static void lpi2c_imx_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct lpi2c_imx_struct *lpi2c_imx;
> +
> +	lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter);
> +
> +	pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_gpio);
> +}
> +
> +static void lpi2c_imx_unprepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct lpi2c_imx_struct *lpi2c_imx;
> +
> +	lpi2c_imx = container_of(adap, struct lpi2c_imx_struct, adapter);
> +
> +	pinctrl_select_state(lpi2c_imx->pinctrl, lpi2c_imx->pinctrl_pins_default);
> +}
> +
> +/*
> + * We switch SCL and SDA to their GPIO function and do some bitbanging
> + * for bus recovery. These alternative pinmux settings can be
> + * described in the device tree by a separate pinctrl state "gpio". If

is this still true?

> + * this is missing this is not a big problem, the only implication is
> + * that we can't do bus recovery.
> + */
> +static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx,
> +		struct platform_device *pdev)
> +{
> +	struct i2c_bus_recovery_info *rinfo = &lpi2c_imx->rinfo;
> +
> +	lpi2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (!lpi2c_imx->pinctrl || IS_ERR(lpi2c_imx->pinctrl)) {
> +		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
> +		return PTR_ERR(lpi2c_imx->pinctrl);
> +	}
> +
> +	lpi2c_imx->pinctrl_pins_default = pinctrl_lookup_state(lpi2c_imx->pinctrl,
> +			PINCTRL_STATE_DEFAULT);
> +	lpi2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(lpi2c_imx->pinctrl,
> +			"gpio");
> +	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> +	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
> +
> +	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
> +	    PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;
> +	} else if (IS_ERR(rinfo->sda_gpiod) ||
> +		   IS_ERR(rinfo->scl_gpiod) ||
> +		   IS_ERR(lpi2c_imx->pinctrl_pins_default) ||
> +		   IS_ERR(lpi2c_imx->pinctrl_pins_gpio)) {
> +		dev_dbg(&pdev->dev, "recovery information incomplete\n");
> +		return 0;
> +	}

Why not use these assignement from the default
i2c_init_recovery()? Is there anything you are doing I am not
seeing?

> +
> +	dev_info(&pdev->dev, "using scl%s for recovery\n",
> +		 rinfo->sda_gpiod ? ",sda" : "");

is there any case when sda_gpiod is NULL?

> +
> +	rinfo->prepare_recovery = lpi2c_imx_prepare_recovery;
> +	rinfo->unprepare_recovery = lpi2c_imx_unprepare_recovery;
> +	rinfo->recover_bus = i2c_generic_scl_recovery;
> +	lpi2c_imx->adapter.bus_recovery_info = rinfo;

do you need also the set_scl() function? It should be mandatory.

> +
> +	return 0;
> +}

Besides, this is a copy/paste from i2c-imx.c, any chance to put
the two things together?

Andi

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example
  2023-06-13 22:42         ` Andi Shyti
@ 2023-07-24  9:48           ` Carlos Song
  0 siblings, 0 replies; 9+ messages in thread
From: Carlos Song @ 2023-07-24  9:48 UTC (permalink / raw)
  To: Andi Shyti
  Cc: Aisheng Dong, shawnguo, s.hauer, kernel, festevam, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, Anson.Huang, Clark Wang,
	Bough Chen, dl-linux-imx, linux-i2c, devicetree,
	linux-arm-kernel, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [EXT] Re: [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature
  2023-06-13 23:19 ` [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature Andi Shyti
@ 2023-07-24 10:11   ` Carlos Song
  0 siblings, 0 replies; 9+ messages in thread
From: Carlos Song @ 2023-07-24 10:11 UTC (permalink / raw)
  To: Andi Shyti
  Cc: Aisheng Dong, shawnguo, s.hauer, kernel, festevam, robh+dt,
	krzysztof.kozlowski+dt, conor+dt, Anson.Huang, Clark Wang,
	Bough Chen, dl-linux-imx, linux-i2c, devicetree,
	linux-arm-kernel, linux-kernel

Hi, Andi

Sorry for the long time to reply. According to your advice, I found the patch is too redundant!
so I will send V2 patch. 
I find gpio and pinctrl assignement from the default i2c_init_recovery() have been defined very well.
Lpi2c have special initialization conditions for i2c recovery and I have added a comment in V2.

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Wednesday, June 14, 2023 7:20 AM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: 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: [EXT] Re: [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature
> 
> 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,
> 
> On Mon, May 29, 2023 at 03:43:01PM +0800, carlos.song@nxp.com wrote:
> > From: Clark Wang <xiaoning.wang@nxp.com>
> >
> > Add bus recovery feature for LPI2C.
> > Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.
> 
> please update the commit message according to the dts changes, as well.
> 
Carlos: There is still a need to add gpio pinctrl to set i2c sda/scl pin to gpio
> [...]
> 
> > +static void lpi2c_imx_prepare_recovery(struct i2c_adapter *adap) {
> > +     struct lpi2c_imx_struct *lpi2c_imx;
> > +
> > +     lpi2c_imx = container_of(adap, struct lpi2c_imx_struct,
> > + adapter);
> > +
> > +     pinctrl_select_state(lpi2c_imx->pinctrl,
> > +lpi2c_imx->pinctrl_pins_gpio); }
> > +
> > +static void lpi2c_imx_unprepare_recovery(struct i2c_adapter *adap) {
> > +     struct lpi2c_imx_struct *lpi2c_imx;
> > +
> > +     lpi2c_imx = container_of(adap, struct lpi2c_imx_struct,
> > + adapter);
> > +
> > +     pinctrl_select_state(lpi2c_imx->pinctrl,
> > +lpi2c_imx->pinctrl_pins_default); }
> > +
> > +/*
> > + * We switch SCL and SDA to their GPIO function and do some
> > +bitbanging
> > + * for bus recovery. These alternative pinmux settings can be
> > + * described in the device tree by a separate pinctrl state "gpio".
> > +If
> 
> is this still true?
> 

Carlos: Yes it is true.

> > + * this is missing this is not a big problem, the only implication is
> > + * that we can't do bus recovery.
> > + */
> > +static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx,
> > +             struct platform_device *pdev) {
> > +     struct i2c_bus_recovery_info *rinfo = &lpi2c_imx->rinfo;
> > +
> > +     lpi2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +     if (!lpi2c_imx->pinctrl || IS_ERR(lpi2c_imx->pinctrl)) {
> > +             dev_info(&pdev->dev, "can't get pinctrl, bus recovery not
> supported\n");
> > +             return PTR_ERR(lpi2c_imx->pinctrl);
> > +     }
> > +
> > +     lpi2c_imx->pinctrl_pins_default =
> pinctrl_lookup_state(lpi2c_imx->pinctrl,
> > +                     PINCTRL_STATE_DEFAULT);
> > +     lpi2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(lpi2c_imx->pinctrl,
> > +                     "gpio");
> > +     rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> > +     rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
> > + GPIOD_OUT_HIGH_OPEN_DRAIN);
> > +
> > +     if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
> > +         PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
> > +             return -EPROBE_DEFER;
> > +     } else if (IS_ERR(rinfo->sda_gpiod) ||
> > +                IS_ERR(rinfo->scl_gpiod) ||
> > +                IS_ERR(lpi2c_imx->pinctrl_pins_default) ||
> > +                IS_ERR(lpi2c_imx->pinctrl_pins_gpio)) {
> > +             dev_dbg(&pdev->dev, "recovery information incomplete\n");
> > +             return 0;
> > +     }
> 
> Why not use these assignement from the default i2c_init_recovery()? Is there
> anything you are doing I am not seeing?
> 

Carlos: these assignements are too redundant and I will fix it in V2 patch.

> > +
> > +     dev_info(&pdev->dev, "using scl%s for recovery\n",
> > +              rinfo->sda_gpiod ? ",sda" : "");
> 
> is there any case when sda_gpiod is NULL?
> 
Carlos: I will delete it in V2.
> > +
> > +     rinfo->prepare_recovery = lpi2c_imx_prepare_recovery;
> > +     rinfo->unprepare_recovery = lpi2c_imx_unprepare_recovery;
> > +     rinfo->recover_bus = i2c_generic_scl_recovery;
> > +     lpi2c_imx->adapter.bus_recovery_info = rinfo;
> 
> do you need also the set_scl() function? It should be mandatory.
> 
Carlos: I will use the default setting in V2 patch.
> > +
> > +     return 0;
> > +}
> 
> Besides, this is a copy/paste from i2c-imx.c, any chance to put the two things
> together?
> 
Carlos: I hope to apply the new recovery patch for lpi2c.
> Andi

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-07-24 10:20 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-29  7:43 [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature carlos.song
2023-05-29  7:43 ` [PATCH 2/2] dt-bindings: i2c: imx-lpi2c: Add bus recovery example carlos.song
2023-05-30 14:58   ` Krzysztof Kozlowski
2023-05-31 10:22     ` [EXT] " Carlos Song
2023-06-02 13:18       ` Krzysztof Kozlowski
2023-06-13 22:42         ` Andi Shyti
2023-07-24  9:48           ` Carlos Song
2023-06-13 23:19 ` [PATCH 1/2] i2c: imx-lpi2c: add bus recovery feature Andi Shyti
2023-07-24 10:11   ` [EXT] " Carlos Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).