[PATCH v2 1/3] i2c: imx-lpi2c: directly return ISR when detect a NACK

[PATCH v2 1/3] i2c: imx-lpi2c: directly return ISR when detect a NACK

[carlos.song]

From: Gao Pan <pandy.gao@nxp.com>

A NACK flag in ISR means i2c bus error. In such codition,
there is no need to do read/write operation. It's better
to return ISR directly and then stop i2c transfer.

Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
Signed-off-by: Gao Pan <pandy.gao@nxp.com>
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index c3287c887c6f..158de0b7f030 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -514,15 +514,14 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
 	temp = readl(lpi2c_imx->base + LPI2C_MSR);
 	temp &= enabled;
 
-	if (temp & MSR_RDF)
+	if (temp & MSR_NDF) {
+		complete(&lpi2c_imx->complete);
+		return IRQ_HANDLED;
+	} else if (temp & MSR_RDF)
 		lpi2c_imx_read_rxfifo(lpi2c_imx);
-
-	if (temp & MSR_TDF)
+	else if (temp & MSR_TDF)
 		lpi2c_imx_write_txfifo(lpi2c_imx);
 
-	if (temp & MSR_NDF)
-		complete(&lpi2c_imx->complete);
-
 	return IRQ_HANDLED;
 }
 
-- 
2.34.1

[PATCH v2 2/3] i2c: imx-lpi2c: add bus recovery feature

[carlos.song]

From: Carlos Song <carlos.song@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 | 51 ++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 158de0b7f030..e93ff3b5373c 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -107,6 +107,7 @@ struct lpi2c_imx_struct {
 	unsigned int		txfifosize;
 	unsigned int		rxfifosize;
 	enum lpi2c_imx_mode	mode;
+	struct i2c_bus_recovery_info rinfo;
 };
 
 static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx,
@@ -134,6 +135,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 +194,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();
@@ -323,6 +328,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();
@@ -525,6 +532,44 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/*
+ * 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;
+
+	/*
+	 * When there is no pinctrl state "gpio" in device tree, it means i2c
+	 * recovery function is not needed, so it is not a problem even if
+	 * pinctrl state "gpio" is missing. Just do not initialize the I2C bus
+	 * recovery information.
+	 */
+	rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(rinfo->pinctrl)) {
+		if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
+		return PTR_ERR(rinfo->pinctrl);
+	} else if (!rinfo->pinctrl) {
+		return -ENODEV;
+	}
+
+	if (IS_ERR(pinctrl_lookup_state(rinfo->pinctrl, "gpio"))) {
+		dev_dbg(&pdev->dev, "recovery information incomplete\n");
+		return 0;
+	}
+
+	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 |
@@ -603,6 +648,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

Re: [PATCH v2 2/3] i2c: imx-lpi2c: add bus recovery feature

[Andi Shyti]

Hi Carlos,

Quite a different patch this v2.

On Mon, Jul 24, 2023 at 06:55:45PM +0800, carlos.song@nxp.com wrote:
> From: Carlos Song <carlos.song@nxp.com>
> 
> Add bus recovery feature for LPI2C.
> Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.

mmhhh... I already asked you in the previous version to update
the commit log... where is the 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 | 51 ++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 158de0b7f030..e93ff3b5373c 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -107,6 +107,7 @@ struct lpi2c_imx_struct {
>  	unsigned int		txfifosize;
>  	unsigned int		rxfifosize;
>  	enum lpi2c_imx_mode	mode;
> +	struct i2c_bus_recovery_info rinfo;

if this is in the i2c_adapter, why do you also need it here? You
keep this place allocated even if it is optional.

>  };
>  
>  static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx,
> @@ -134,6 +135,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);

what is the recover_bus() function that will get called?

>  			return -ETIMEDOUT;
>  		}
>  		schedule();
> @@ -191,6 +194,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();
> @@ -323,6 +328,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();
> @@ -525,6 +532,44 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * 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

What is the description in the device tree?

> + * 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;
> +
> +	/*
> +	 * When there is no pinctrl state "gpio" in device tree, it means i2c
> +	 * recovery function is not needed, so it is not a problem even if
> +	 * pinctrl state "gpio" is missing. Just do not initialize the I2C bus
> +	 * recovery information.
> +	 */
> +	rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(rinfo->pinctrl)) {
> +		if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");

nit: "can't get pinctrl..." sounds like error and this is not an
error. Just "bus recovery not supported" is more a friendly
message.

> +		return PTR_ERR(rinfo->pinctrl);
> +	} else if (!rinfo->pinctrl) {
> +		return -ENODEV;

this is the unsupported case and here I would return '0' as it's
not an error.

> +	}
> +
> +	if (IS_ERR(pinctrl_lookup_state(rinfo->pinctrl, "gpio"))) {
> +		dev_dbg(&pdev->dev, "recovery information incomplete\n");
> +		return 0;
> +	}
> +
> +	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 |
> @@ -603,6 +648,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;

what about other errors like -ENOMEM?

Andi

>  	ret = i2c_add_adapter(&lpi2c_imx->adapter);
>  	if (ret)
>  		goto rpm_disable;
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/3] i2c: imx-lpi2c: add bus recovery feature

[Carlos Song]

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Wednesday, July 26, 2023 10:12 PM
> 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; Clark
> Wang <xiaoning.wang@nxp.com>; Bough Chen <haibo.chen@nxp.com>;
> dl-linux-imx <linux-imx@nxp.com>; linux-i2c@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v2 2/3] 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 Carlos,
> 
> Quite a different patch this v2.
> 

Hi, Andi

hhh... yes, as you see, your advice for V1 guided me.
In i2c_init_recovery, I find the patch: “i2c: core: add generic I2C GPIO recovery“.
Because of it I found i2c driver hasn't needed so many explicit recovery information statements. 
It can help i2c driver to fill incomplete recovery information in i2c_init_recovery().
Based on this patch, any I2C bus drivers will be able to support GPIO recovery just by
providing a pointer to platform's pinctrl and calling i2c_recover_bus() when SDA is
stuck low.

So there are lots of redundant initialization lines in the V1 patch. I have to remove
them and remake the patch V2. 

> On Mon, Jul 24, 2023 at 06:55:45PM +0800, carlos.song@nxp.com wrote:
> > From: Carlos Song <carlos.song@nxp.com>
> >
> > Add bus recovery feature for LPI2C.
> > Need add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.
> 
> mmhhh... I already asked you in the previous version to update the commit log...
> where is the DTS?
> 

Yes, I actually have got you advice in V1. The reason I keep it is that we hope i2c recovery function just be optical. 
In fact the commit log means:
We don’t use i2c recovery function as default. If you want use i2c recovery function, you should
add gpio pinctrl, scl-gpios and sda-gpios configuration in dts.
If you don't add it, it is ok. There is no any error log, of course i2c will not recovery.
(I have added a comment at lpi2c_imx_init_recovery_info())
So I keep itat V2. If there is no need to add it. I also support to remove it or fix it at V3.

> > 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 | 51
> > ++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index 158de0b7f030..e93ff3b5373c 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -107,6 +107,7 @@ struct lpi2c_imx_struct {
> >       unsigned int            txfifosize;
> >       unsigned int            rxfifosize;
> >       enum lpi2c_imx_mode     mode;
> > +     struct i2c_bus_recovery_info rinfo;
> 
> if this is in the i2c_adapter, why do you also need it here? You keep this place
> allocated even if it is optional.
> 

There is a i2c_bus_recovery_info pointer in i2c adapter, so I think I need to allocate
memory space for i2c_bus_recovery_info. How to choose this place allocated also
bother me. I'd also like to know your suggestion about it.

I tried two ways to that:
1. Define a global structure and assign values ​​to its members
+ static struct i2c_bus_recovery_info lpi2c_i2c_recovery_info = {
+	.recover_bus = i2c_generic_scl_recovery,
+}
And in probe(){
+	lpi2c_imx->adapter.bus_recovery_info = &lpi2c_i2c_recovery_info;
}

I2c recovery function will be mandatory. If there is not a gpio-pinctrl configure in dts.
I2c will not output error log "Not using recovery: no {get|set}_scl() found".

That is not what we hope. We hope i2c recovery function is optional. If we do not configure
gpio-pinctrl in dts, it means that we don't use i2c recovery function and should not report an
error. It should be a conditional initialization. We hope check if there is a gpio-pinctrl 
configure in dts. If there is, i2c recovery info begin initialization(This means that i2c recovery
function is needed).

So I choose define i2c_bus_recovery_info in lpi2c_imx_struct(it looks concise and easy to use).
And define a function lpi2c_imx_init_recovery_info to check if i2c recovery info need to be initialized.

> >  };
> >
> >  static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx, @@
> > -134,6 +135,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);
> 
> what is the recover_bus() function that will get called?

It is i2c_generic_scl_recovery. In i2c_init_recovery()-> i2c_gpio_init_generic_recovery(),
if generic GPIO recovery is available, will complete the incomplete recovery information.

> >                       return -ETIMEDOUT;
> >               }
> >               schedule();
> > @@ -191,6 +194,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();
> > @@ -323,6 +328,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();
> > @@ -525,6 +532,44 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
> >       return IRQ_HANDLED;
> >  }
> >
> > +/*
> > + * 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
> 
> What is the description in the device tree?
> 

The configure in dts when we need i2c recovery function:

@@ -410,9 +410,12 @@ &lpi2c1 {
- 		pinctrl-names = "default", "sleep";
+       pinctrl-names = "default", "sleep", "gpio";
+       pinctrl-2 = <&pinctrl_lpi2c1_gpio>;
+       scl-gpios = <&gpio1 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+       sda-gpios = <&gpio1 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
       status = "okay";
@@ -837,6 +840,13 @@ MX93_PAD_I2C1_SDA__LPI2C1_SDA                      0x40000b9e
                >;
        };

+       pinctrl_lpi2c1_gpio: lpi2c1gpiogrp {
+               fsl,pins = <
+                       MX93_PAD_I2C1_SCL__GPIO1_IO00                   0xb9e
+                       MX93_PAD_I2C1_SDA__GPIO1_IO01                   0xb9e
+               >;
+       };

> > + * 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;
> > +
> > +     /*
> > +      * When there is no pinctrl state "gpio" in device tree, it means i2c
> > +      * recovery function is not needed, so it is not a problem even if
> > +      * pinctrl state "gpio" is missing. Just do not initialize the I2C bus
> > +      * recovery information.
> > +      */
> > +     rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +     if (IS_ERR(rinfo->pinctrl)) {
> > +             if (PTR_ERR(rinfo->pinctrl) == -EPROBE_DEFER)
> > +                     return -EPROBE_DEFER;
> > +             dev_info(&pdev->dev, "can't get pinctrl, bus recovery
> > + not supported\n");
> 
> nit: "can't get pinctrl..." sounds like error and this is not an error. Just "bus
> recovery not supported" is more a friendly message.
> 
ok, I will fix it at V3.
> > +             return PTR_ERR(rinfo->pinctrl);
> > +     } else if (!rinfo->pinctrl) {
> > +             return -ENODEV;
> 
> this is the unsupported case and here I would return '0' as it's not an error.
> 
I will fix it at V3.
> > +     }
> > +
> > +     if (IS_ERR(pinctrl_lookup_state(rinfo->pinctrl, "gpio"))) {
> > +             dev_dbg(&pdev->dev, "recovery information incomplete\n");
> > +             return 0;
> > +     }
> > +
> > +     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 | @@ -603,6
> +648,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;
> 
> what about other errors like -ENOMEM?
> 

This judgment cannot cover all error conditions, I will fix it at V3:
+     /* 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)
+             goto rpm_disable;
Is this the judgment expected to be valid?
> Andi
> 
> >       ret = i2c_add_adapter(&lpi2c_imx->adapter);
> >       if (ret)
> >               goto rpm_disable;
> > 
Hope my excessive explanation didn't confuse you. Thanks!
--
> > 2.34.1
> >

[PATCH v2 3/3] i2c: imx-lpi2c: add debug message when i2c peripheral clk doesn't work

[carlos.song]

From: Gao Pan <pandy.gao@nxp.com>

Output error log when i2c peripheral clk rate is 0, then
directly return -EINVAL.

Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
Signed-off-by: Gao Pan <pandy.gao@nxp.com>
Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index e93ff3b5373c..12b4f2a89343 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -214,6 +214,11 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
 	lpi2c_imx_set_mode(lpi2c_imx);
 
 	clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
+	if (!clk_rate) {
+		dev_err(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");
+		return -EINVAL;
+	}
+
 	if (lpi2c_imx->mode == HS || lpi2c_imx->mode == ULTRA_FAST)
 		filt = 0;
 	else
-- 
2.34.1

Re: [PATCH v2 3/3] i2c: imx-lpi2c: add debug message when i2c peripheral clk doesn't work

[Fabio Estevam]

On Mon, Jul 24, 2023 at 7:52 AM <carlos.song@nxp.com> wrote:
>
> From: Gao Pan <pandy.gao@nxp.com>
>
> Output error log when i2c peripheral clk rate is 0, then
> directly return -EINVAL.
>
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index e93ff3b5373c..12b4f2a89343 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -214,6 +214,11 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
>         lpi2c_imx_set_mode(lpi2c_imx);
>
>         clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
> +       if (!clk_rate) {
> +               dev_err(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");

The subject says 'debug message', but this is an error message.

Please adjust the Subject.

RE: [EXT] Re: [PATCH v2 3/3] i2c: imx-lpi2c: add debug message when i2c peripheral clk doesn't work

[Carlos Song]

Thanks! I will adjust it and send V3 patch.

> -----Original Message-----
> From: Fabio Estevam <festevam@gmail.com>
> Sent: Monday, July 24, 2023 8:38 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: andi.shyti@kernel.org; Aisheng Dong <aisheng.dong@nxp.com>;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> Clark Wang <xiaoning.wang@nxp.com>; Bough Chen <haibo.chen@nxp.com>;
> dl-linux-imx <linux-imx@nxp.com>; linux-i2c@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v2 3/3] i2c: imx-lpi2c: add debug message when i2c
> peripheral clk doesn't work
> 
> 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 Mon, Jul 24, 2023 at 7:52 AM <carlos.song@nxp.com> wrote:
> >
> > From: Gao Pan <pandy.gao@nxp.com>
> >
> > Output error log when i2c peripheral clk rate is 0, then directly
> > return -EINVAL.
> >
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > ---
> >  drivers/i2c/busses/i2c-imx-lpi2c.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index e93ff3b5373c..12b4f2a89343 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -214,6 +214,11 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct
> *lpi2c_imx)
> >         lpi2c_imx_set_mode(lpi2c_imx);
> >
> >         clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
> > +       if (!clk_rate) {
> > +               dev_err(&lpi2c_imx->adapter.dev, "clk_per rate is
> > + 0\n");
> 
> The subject says 'debug message', but this is an error message.
> 
> Please adjust the Subject.

Re: [PATCH v2 1/3] i2c: imx-lpi2c: directly return ISR when detect a NACK

[Andi Shyti]

Hi Carlos,

On Mon, Jul 24, 2023 at 06:55:44PM +0800, carlos.song@nxp.com wrote:
> From: Gao Pan <pandy.gao@nxp.com>
> 
> A NACK flag in ISR means i2c bus error. In such codition,
> there is no need to do read/write operation. It's better
> to return ISR directly and then stop i2c transfer.
> 
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index c3287c887c6f..158de0b7f030 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -514,15 +514,14 @@ static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
>  	temp = readl(lpi2c_imx->base + LPI2C_MSR);
>  	temp &= enabled;
>  
> -	if (temp & MSR_RDF)
> +	if (temp & MSR_NDF) {
> +		complete(&lpi2c_imx->complete);
> +		return IRQ_HANDLED;

you can actually remove the return here

	if (temp & MSR_NDF)
		complete();
	else if (temp & MSR_RDF)
		exfifo();
	else if (temp & MSR_TDF)
		txfifo();

	return IRQ_HANDLED;


BTW, the logic here is changing, as well and it's not described
in the commit log. This patch is not only stopping when a nack is
received (MSR_NDF), but it's also making mutually exclusive
read/write (which I guess are MSR_RDF and MSR_TDF).

Is this what you want? If so, can you please describe it in the
commit log or add a comment describing that the three states are
all mutually exclusive.

Thanks,
Andi


> +	} else if (temp & MSR_RDF)
>  		lpi2c_imx_read_rxfifo(lpi2c_imx);
> -
> -	if (temp & MSR_TDF)
> +	else if (temp & MSR_TDF)
>  		lpi2c_imx_write_txfifo(lpi2c_imx);
>  
> -	if (temp & MSR_NDF)
> -		complete(&lpi2c_imx->complete);
> -
>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 2.34.1
> 

RE: [EXT] Re: [PATCH v2 1/3] i2c: imx-lpi2c: directly return ISR when detect a NACK

[Carlos Song]

Hi, Andi

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Wednesday, July 26, 2023 7:55 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; Clark
> Wang <xiaoning.wang@nxp.com>; Bough Chen <haibo.chen@nxp.com>;
> dl-linux-imx <linux-imx@nxp.com>; linux-i2c@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v2 1/3] i2c: imx-lpi2c: directly return ISR when detect
> a NACK
> 
> 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 Carlos,
> 
> On Mon, Jul 24, 2023 at 06:55:44PM +0800, carlos.song@nxp.com wrote:
> > From: Gao Pan <pandy.gao@nxp.com>
> >
> > A NACK flag in ISR means i2c bus error. In such codition, there is no
> > need to do read/write operation. It's better to return ISR directly
> > and then stop i2c transfer.
> >
> > Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> > Signed-off-by: Gao Pan <pandy.gao@nxp.com>
> > Signed-off-by: Carlos Song <carlos.song@nxp.com>
> > ---
> >  drivers/i2c/busses/i2c-imx-lpi2c.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > index c3287c887c6f..158de0b7f030 100644
> > --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> > +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> > @@ -514,15 +514,14 @@ static irqreturn_t lpi2c_imx_isr(int irq, void
> *dev_id)
> >       temp = readl(lpi2c_imx->base + LPI2C_MSR);
> >       temp &= enabled;
> >
> > -     if (temp & MSR_RDF)
> > +     if (temp & MSR_NDF) {
> > +             complete(&lpi2c_imx->complete);
> > +             return IRQ_HANDLED;
> 
> you can actually remove the return here
> 
>         if (temp & MSR_NDF)
>                 complete();
>         else if (temp & MSR_RDF)
>                 exfifo();
>         else if (temp & MSR_TDF)
>                 txfifo();
> 
>         return IRQ_HANDLED;
> 
> 
> BTW, the logic here is changing, as well and it's not described in the commit log.
> This patch is not only stopping when a nack is received (MSR_NDF), but it's also
> making mutually exclusive read/write (which I guess are MSR_RDF and
> MSR_TDF).
> 
> Is this what you want? If so, can you please describe it in the commit log or add
> a comment describing that the three states are all mutually exclusive.
> 
Yes. That is ok. I will fix the commit log and send V3 patch.
> Thanks,
> Andi
> 
> 
> > +     } else if (temp & MSR_RDF)
> >               lpi2c_imx_read_rxfifo(lpi2c_imx);
> > -
> > -     if (temp & MSR_TDF)
> > +     else if (temp & MSR_TDF)
> >               lpi2c_imx_write_txfifo(lpi2c_imx);
> >
> > -     if (temp & MSR_NDF)
> > -             complete(&lpi2c_imx->complete);
> > -
> >       return IRQ_HANDLED;
> >  }
> >
> > --
> > 2.34.1
> >