[Patch v1] i2c: imx: implement bus recovery

[Patch v1] i2c: imx: implement bus recovery

[Gao Pan]

Implement bus recovery methods for i2c-imx so we can recover from
situations where SCL/SDA are stuck low.

Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
pinctrl to gpio mode by calling pinctrl sleep set function, and then
use GPIO to emulate the i2c protocol to send nine dummy clock to recover
i2c device. After recovery, set i2c pinctrl to default group setting.

Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-imx.txt |   4 +
 drivers/i2c/busses/i2c-imx.c                      | 102 +++++++++++++++++++++-
 2 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
index ce4311d..0a0848f5 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
@@ -14,6 +14,8 @@ Optional properties:
   The absence of the propoerty indicates the default frequency 100 kHz.
 - dmas: A list of two dma specifiers, one for each entry in dma-names.
 - dma-names: should contain "tx" and "rx".
+- recover-scl: specify the gpio related to SCL pin
+- recover-sda: specify the gpio related to SDA pin
 
 Examples:
 
@@ -37,4 +39,6 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */
 	dmas = <&edma0 0 50>,
 		<&edma0 0 51>;
 	dma-names = "rx","tx";
+	recover-scl = <&gpio5 26 0>;
+	recover-sda = <&gpio5 27 0>;
 };
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 785aa67..eb4f95c 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -40,6 +40,7 @@
 #include <linux/dmapool.h>
 #include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -48,6 +49,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/of_dma.h>
 #include <linux/platform_data/i2c-imx.h>
 #include <linux/platform_device.h>
@@ -175,6 +177,16 @@ enum imx_i2c_type {
 	VF610_I2C,
 };
 
+enum imx_i2c_vol_level {
+	I2C_IMX_LOW_VOL_LEVEL,
+	I2C_IMX_HIGH_VOL_LEVEL,
+};
+
+struct imx_i2c_pinctrl {
+	unsigned int	sda_pin;
+	unsigned int	scl_pin;
+};
+
 struct imx_i2c_hwdata {
 	enum imx_i2c_type	devtype;
 	unsigned		regshift;
@@ -206,6 +218,7 @@ struct imx_i2c_struct {
 	unsigned int		ifdr; /* IMX_I2C_IFDR */
 	unsigned int		cur_clk;
 	unsigned int		bitrate;
+	struct imx_i2c_pinctrl	pins;
 	const struct imx_i2c_hwdata	*hwdata;
 
 	struct imx_i2c_dma	*dma;
@@ -436,7 +449,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
 		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
 			dev_dbg(&i2c_imx->adapter.dev,
 				"<%s> I2C bus is busy\n", __func__);
-			return -ETIMEDOUT;
+			return i2c_recover_bus(&i2c_imx->adapter);
 		}
 		schedule();
 	}
@@ -450,6 +463,7 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
 
 	if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
 		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
+		i2c_recover_bus(&i2c_imx->adapter);
 		return -ETIMEDOUT;
 	}
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__);
@@ -956,6 +970,67 @@ fail0:
 	return (result < 0) ? result : num;
 }
 
+static int i2c_imx_get_scl(struct i2c_adapter *adap)
+{
+	struct imx_i2c_struct *i2c_imx;
+
+	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
+	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
+		return gpio_get_value(i2c_imx->pins.scl_pin);
+
+	return I2C_IMX_HIGH_VOL_LEVEL;
+}
+
+static int i2c_imx_get_sda(struct i2c_adapter *adap)
+{
+	struct imx_i2c_struct *i2c_imx;
+
+	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
+	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin) {
+		gpio_direction_input(i2c_imx->pins.sda_pin);
+		return gpio_get_value(i2c_imx->pins.sda_pin);
+	}
+
+	return I2C_IMX_HIGH_VOL_LEVEL;
+}
+
+static void i2c_imx_set_scl(struct i2c_adapter *adap, int val)
+{
+	struct imx_i2c_struct *i2c_imx;
+
+	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
+	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
+		gpio_direction_output(i2c_imx->pins.scl_pin, val);
+}
+
+static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct imx_i2c_struct *i2c_imx;
+
+	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
+	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
+		pinctrl_pm_select_sleep_state(&adap->dev);
+
+}
+
+static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct imx_i2c_struct *i2c_imx;
+
+	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
+	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
+		pinctrl_pm_select_default_state(&adap->dev);
+}
+
+static struct i2c_bus_recovery_info i2c_imx_bus_recovery_info = {
+	.get_scl                = i2c_imx_get_scl,
+	.get_sda                = i2c_imx_get_sda,
+	.set_scl                = i2c_imx_set_scl,
+	.prepare_recovery	= i2c_imx_prepare_recovery,
+	.unprepare_recovery	= i2c_imx_unprepare_recovery,
+	.recover_bus            = i2c_generic_scl_recovery,
+};
+
 static u32 i2c_imx_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
@@ -1009,6 +1084,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	i2c_imx->adapter.dev.parent	= &pdev->dev;
 	i2c_imx->adapter.nr		= pdev->id;
 	i2c_imx->adapter.dev.of_node	= pdev->dev.of_node;
+	i2c_imx->adapter.bus_recovery_info	= &i2c_imx_bus_recovery_info;
 	i2c_imx->base			= base;
 
 	/* Get I2C clock */
@@ -1037,6 +1113,30 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	/* Set up adapter data */
 	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
 
+	/* Init recover pins */
+	i2c_imx->pins.sda_pin =
+		of_get_named_gpio(pdev->dev.of_node, "recover-sda", 0);
+	i2c_imx->pins.scl_pin =
+		of_get_named_gpio(pdev->dev.of_node, "recover-scl", 0);
+	if (gpio_is_valid(i2c_imx->pins.sda_pin) &&
+	    gpio_is_valid(i2c_imx->pins.scl_pin)) {
+		ret = devm_gpio_request_one(&pdev->dev, i2c_imx->pins.sda_pin,
+					    GPIOF_OUT_INIT_HIGH, "recover-sda");
+		if (ret) {
+			i2c_imx->pins.sda_pin = 0;
+			dev_err(&pdev->dev, "request sda failed\n");
+		}
+		ret = devm_gpio_request_one(&pdev->dev, i2c_imx->pins.scl_pin,
+					    GPIOF_OUT_INIT_HIGH, "recover-scl");
+		if (ret) {
+			i2c_imx->pins.scl_pin = 0;
+			dev_err(&pdev->dev, "request scl failed\n");
+		}
+	} else {
+		i2c_imx->pins.sda_pin = 0;
+		i2c_imx->pins.scl_pin = 0;
+	}
+
 	/* Set up clock divider */
 	i2c_imx->bitrate = IMX_I2C_BIT_RATE;
 	ret = of_property_read_u32(pdev->dev.of_node,
-- 
1.9.1

Re: [Patch v1] i2c: imx: implement bus recovery

[Uwe Kleine-König]

Hello Gao,

On Tue, Jul 14, 2015 at 10:04:46AM +0800, Gao Pan wrote:
> Implement bus recovery methods for i2c-imx so we can recover from
> situations where SCL/SDA are stuck low.
> 
> Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
> pinctrl to gpio mode by calling pinctrl sleep set function, and then
> use GPIO to emulate the i2c protocol to send nine dummy clock to recover
> i2c device. After recovery, set i2c pinctrl to default group setting.
> 
> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Signed-off-by: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-imx.txt |   4 +
>  drivers/i2c/busses/i2c-imx.c                      | 102 +++++++++++++++++++++-
>  2 files changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> index ce4311d..0a0848f5 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> @@ -14,6 +14,8 @@ Optional properties:
>    The absence of the propoerty indicates the default frequency 100 kHz.
>  - dmas: A list of two dma specifiers, one for each entry in dma-names.
>  - dma-names: should contain "tx" and "rx".
> +- recover-scl: specify the gpio related to SCL pin
> +- recover-sda: specify the gpio related to SDA pin
I don't like the naming here. That the gpios are used for recovery isn't
a hardware description. What about "scl-gpio" and "sda-gpio"?
 
>  Examples:
>  
> @@ -37,4 +39,6 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */
>  	dmas = <&edma0 0 50>,
>  		<&edma0 0 51>;
>  	dma-names = "rx","tx";
> +	recover-scl = <&gpio5 26 0>;
> +	recover-sda = <&gpio5 27 0>;
>  };
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 785aa67..eb4f95c 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -40,6 +40,7 @@
>  #include <linux/dmapool.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
> +#include <linux/gpio.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> @@ -48,6 +49,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>  #include <linux/of_dma.h>
>  #include <linux/platform_data/i2c-imx.h>
>  #include <linux/platform_device.h>
> @@ -175,6 +177,16 @@ enum imx_i2c_type {
>  	VF610_I2C,
>  };
>  
> +enum imx_i2c_vol_level {
> +	I2C_IMX_LOW_VOL_LEVEL,
> +	I2C_IMX_HIGH_VOL_LEVEL,
> +};
These are used as return value for the get_scl and get_sda callbacks
which should return an int. I'd use plain 0 and 1 here.

> +struct imx_i2c_pinctrl {
> +	unsigned int	sda_pin;
> +	unsigned int	scl_pin;
> +};
> +
>  struct imx_i2c_hwdata {
>  	enum imx_i2c_type	devtype;
>  	unsigned		regshift;
> @@ -206,6 +218,7 @@ struct imx_i2c_struct {
>  	unsigned int		ifdr; /* IMX_I2C_IFDR */
>  	unsigned int		cur_clk;
>  	unsigned int		bitrate;
> +	struct imx_i2c_pinctrl	pins;
>  	const struct imx_i2c_hwdata	*hwdata;
>  
>  	struct imx_i2c_dma	*dma;
> @@ -436,7 +449,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
>  		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
>  			dev_dbg(&i2c_imx->adapter.dev,
>  				"<%s> I2C bus is busy\n", __func__);
> -			return -ETIMEDOUT;
> +			return i2c_recover_bus(&i2c_imx->adapter);

This looks wrong. What if the bus is still busy after the call to
i2c_recover_bus?  Also it doesn't feel right to call i2c_recover_bus
just because the bus is busy.

>  		}
>  		schedule();
>  	}
> @@ -450,6 +463,7 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
>  
>  	if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
>  		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
> +		i2c_recover_bus(&i2c_imx->adapter);
>  		return -ETIMEDOUT;
ditto.

>  	}
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__);
> @@ -956,6 +970,67 @@ fail0:
>  	return (result < 0) ? result : num;
>  }
>  
> +static int i2c_imx_get_scl(struct i2c_adapter *adap)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
> +		return gpio_get_value(i2c_imx->pins.scl_pin);
> +
> +	return I2C_IMX_HIGH_VOL_LEVEL;
> +}
> +
> +static int i2c_imx_get_sda(struct i2c_adapter *adap)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin) {
> +		gpio_direction_input(i2c_imx->pins.sda_pin);
Don't you want that call in the prepare callback?

> +		return gpio_get_value(i2c_imx->pins.sda_pin);
> +	}
> +
> +	return I2C_IMX_HIGH_VOL_LEVEL;
> +}
> +
> +static void i2c_imx_set_scl(struct i2c_adapter *adap, int val)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
> +		gpio_direction_output(i2c_imx->pins.scl_pin, val);
Please move the gpio_direction_output call to the prepare callback and
only use gpio_set_value here.

> +}
> +
> +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
> +		pinctrl_pm_select_sleep_state(&adap->dev);
So the sleep state is expected to mux the pins into their gpio function?

> +
spare empty line

> +}
> +
> +static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct imx_i2c_struct *i2c_imx;
> +
> +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
> +		pinctrl_pm_select_default_state(&adap->dev);
> +}
> +
> +static struct i2c_bus_recovery_info i2c_imx_bus_recovery_info = {
> +	.get_scl                = i2c_imx_get_scl,
> +	.get_sda                = i2c_imx_get_sda,
> +	.set_scl                = i2c_imx_set_scl,
> +	.prepare_recovery	= i2c_imx_prepare_recovery,
> +	.unprepare_recovery	= i2c_imx_unprepare_recovery,
> +	.recover_bus            = i2c_generic_scl_recovery,
Mixed tab and spaces used for indention (if you ask me, use a single
space before the '=' ...

> +};
> +
>  static u32 i2c_imx_func(struct i2c_adapter *adapter)
>  {
>  	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> @@ -1009,6 +1084,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	i2c_imx->adapter.dev.parent	= &pdev->dev;
>  	i2c_imx->adapter.nr		= pdev->id;
>  	i2c_imx->adapter.dev.of_node	= pdev->dev.of_node;
> +	i2c_imx->adapter.bus_recovery_info	= &i2c_imx_bus_recovery_info;
>  	i2c_imx->base			= base;
... which prevents uglyness as introduce here.)

>  
>  	/* Get I2C clock */
> @@ -1037,6 +1113,30 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	/* Set up adapter data */
>  	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
>  
> +	/* Init recover pins */
> +	i2c_imx->pins.sda_pin =
> +		of_get_named_gpio(pdev->dev.of_node, "recover-sda", 0);
> +	i2c_imx->pins.scl_pin =
> +		of_get_named_gpio(pdev->dev.of_node, "recover-scl", 0);
I'd use devm_gpiod_get_optional here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [Patch v1] i2c: imx: implement bus recovery

[Gao Pandy]

From: Uwe Kleine-König <mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Sent: Tuesday, July 14, 2015 2:49 PM
> To: Gao Pan-B54642
> Cc: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Li Frank-B20596; Duan
> Fugang-B38611
> Subject: Re: [Patch v1] i2c: imx: implement bus recovery
> 
> Hello Gao,
> 
> On Tue, Jul 14, 2015 at 10:04:46AM +0800, Gao Pan wrote:
> > Implement bus recovery methods for i2c-imx so we can recover from
> > situations where SCL/SDA are stuck low.
> >
> > Once i2c bus SCL/SDA are stuck low during transfer, config the i2c
> > pinctrl to gpio mode by calling pinctrl sleep set function, and then
> > use GPIO to emulate the i2c protocol to send nine dummy clock to
> > recover i2c device. After recovery, set i2c pinctrl to default group
> setting.
> >
> > Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > Signed-off-by: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  Documentation/devicetree/bindings/i2c/i2c-imx.txt |   4 +
> >  drivers/i2c/busses/i2c-imx.c                      | 102
> +++++++++++++++++++++-
> >  2 files changed, 105 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > index ce4311d..0a0848f5 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > @@ -14,6 +14,8 @@ Optional properties:
> >    The absence of the propoerty indicates the default frequency 100 kHz.
> >  - dmas: A list of two dma specifiers, one for each entry in dma-names.
> >  - dma-names: should contain "tx" and "rx".
> > +- recover-scl: specify the gpio related to SCL pin
> > +- recover-sda: specify the gpio related to SDA pin
> I don't like the naming here. That the gpios are used for recovery isn't
> a hardware description. What about "scl-gpio" and "sda-gpio"?

Thanks. I will change it in the next version. 

> >  Examples:
> >
> > @@ -37,4 +39,6 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */
> >  	dmas = <&edma0 0 50>,
> >  		<&edma0 0 51>;
> >  	dma-names = "rx","tx";
> > +	recover-scl = <&gpio5 26 0>;
> > +	recover-sda = <&gpio5 27 0>;
> >  };
> > diff --git a/drivers/i2c/busses/i2c-imx.c
> > b/drivers/i2c/busses/i2c-imx.c index 785aa67..eb4f95c 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -40,6 +40,7 @@
> >  #include <linux/dmapool.h>
> >  #include <linux/err.h>
> >  #include <linux/errno.h>
> > +#include <linux/gpio.h>
> >  #include <linux/i2c.h>
> >  #include <linux/init.h>
> >  #include <linux/interrupt.h>
> > @@ -48,6 +49,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> >  #include <linux/of_dma.h>
> >  #include <linux/platform_data/i2c-imx.h>  #include
> > <linux/platform_device.h> @@ -175,6 +177,16 @@ enum imx_i2c_type {
> >  	VF610_I2C,
> >  };
> >
> > +enum imx_i2c_vol_level {
> > +	I2C_IMX_LOW_VOL_LEVEL,
> > +	I2C_IMX_HIGH_VOL_LEVEL,
> > +};
> These are used as return value for the get_scl and get_sda callbacks
> which should return an int. I'd use plain 0 and 1 here.

Thanks. It can improve the readability to use plain 0 and 1 here.
 
> > +struct imx_i2c_pinctrl {
> > +	unsigned int	sda_pin;
> > +	unsigned int	scl_pin;
> > +};
> > +
> >  struct imx_i2c_hwdata {
> >  	enum imx_i2c_type	devtype;
> >  	unsigned		regshift;
> > @@ -206,6 +218,7 @@ struct imx_i2c_struct {
> >  	unsigned int		ifdr; /* IMX_I2C_IFDR */
> >  	unsigned int		cur_clk;
> >  	unsigned int		bitrate;
> > +	struct imx_i2c_pinctrl	pins;
> >  	const struct imx_i2c_hwdata	*hwdata;
> >
> >  	struct imx_i2c_dma	*dma;
> > @@ -436,7 +449,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct
> *i2c_imx, int for_busy)
> >  		if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500)))
> {
> >  			dev_dbg(&i2c_imx->adapter.dev,
> >  				"<%s> I2C bus is busy\n", __func__);
> > -			return -ETIMEDOUT;
> > +			return i2c_recover_bus(&i2c_imx->adapter);
> 
> This looks wrong. What if the bus is still busy after the call to
> i2c_recover_bus?  Also it doesn't feel right to call i2c_recover_bus just
> because the bus is busy.

In there, maybe i2c device sda is pulled low and bus is dead, so I add the bus recovery in here.
Because there wait 500ms, it is enough long.

If the bus is still busy after recovery, it return the -EBUSY, and gpio also cannot recover the bus, so this transfer is failed.
What do you suggest for the position to call i2c_recover_bus() ? Thanks.

> >  		}
> >  		schedule();
> >  	}
> > @@ -450,6 +463,7 @@ static int i2c_imx_trx_complete(struct
> > imx_i2c_struct *i2c_imx)
> >
> >  	if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
> >  		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
> > +		i2c_recover_bus(&i2c_imx->adapter);
> >  		return -ETIMEDOUT;
> ditto.

The same as above. But it is better:
return i2c_recover_bus(&i2c_imx->adapter);

> >  	}
> >  	dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__); @@
> > -956,6 +970,67 @@ fail0:
> >  	return (result < 0) ? result : num;
> >  }
> >
> > +static int i2c_imx_get_scl(struct i2c_adapter *adap) {
> > +	struct imx_i2c_struct *i2c_imx;
> > +
> > +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> > +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
> > +		return gpio_get_value(i2c_imx->pins.scl_pin);
> > +
> > +	return I2C_IMX_HIGH_VOL_LEVEL;
> > +}
> > +
> > +static int i2c_imx_get_sda(struct i2c_adapter *adap) {
> > +	struct imx_i2c_struct *i2c_imx;
> > +
> > +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> > +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin) {
> > +		gpio_direction_input(i2c_imx->pins.sda_pin);
> Don't you want that call in the prepare callback?

Thanks for your precise review. I will change it in the next version.
 
> > +		return gpio_get_value(i2c_imx->pins.sda_pin);
> > +	}
> > +
> > +	return I2C_IMX_HIGH_VOL_LEVEL;
> > +}
> > +
> > +static void i2c_imx_set_scl(struct i2c_adapter *adap, int val) {
> > +	struct imx_i2c_struct *i2c_imx;
> > +
> > +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> > +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
> > +		gpio_direction_output(i2c_imx->pins.scl_pin, val);
> Please move the gpio_direction_output call to the prepare callback and
> only use gpio_set_value here. 

Thank you, will change it in next version. 

> > +}
> > +
> > +static void i2c_imx_prepare_recovery(struct i2c_adapter *adap) {
> > +	struct imx_i2c_struct *i2c_imx;
> > +
> > +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> > +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
> > +		pinctrl_pm_select_sleep_state(&adap->dev);
> So the sleep state is expected to mux the pins into their gpio function?

Yes, you are right. In here, we expect the pad is muxed to gpio function.

> > +
> spare empty line

Thanks.

> 
> > +}
> > +
> > +static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap) {
> > +	struct imx_i2c_struct *i2c_imx;
> > +
> > +	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> > +	if (i2c_imx->pins.sda_pin && i2c_imx->pins.scl_pin)
> > +		pinctrl_pm_select_default_state(&adap->dev);
> > +}
> > +
> > +static struct i2c_bus_recovery_info i2c_imx_bus_recovery_info = {
> > +	.get_scl                = i2c_imx_get_scl,
> > +	.get_sda                = i2c_imx_get_sda,
> > +	.set_scl                = i2c_imx_set_scl,
> > +	.prepare_recovery	= i2c_imx_prepare_recovery,
> > +	.unprepare_recovery	= i2c_imx_unprepare_recovery,
> > +	.recover_bus            = i2c_generic_scl_recovery,
> Mixed tab and spaces used for indention (if you ask me, use a single
> space before the '=' ...

Thanks for your carefulness, I will correct it in the next version.

> > +};
> > +
> >  static u32 i2c_imx_func(struct i2c_adapter *adapter)  {
> >  	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL @@ -1009,6 +1084,7 @@
> > static int i2c_imx_probe(struct platform_device *pdev)
> >  	i2c_imx->adapter.dev.parent	= &pdev->dev;
> >  	i2c_imx->adapter.nr		= pdev->id;
> >  	i2c_imx->adapter.dev.of_node	= pdev->dev.of_node;
> > +	i2c_imx->adapter.bus_recovery_info	= &i2c_imx_bus_recovery_info;
> >  	i2c_imx->base			= base;
> ... which prevents uglyness as introduce here.)

Thanks.
 
> >
> >  	/* Get I2C clock */
> > @@ -1037,6 +1113,30 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> >  	/* Set up adapter data */
> >  	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
> >
> > +	/* Init recover pins */
> > +	i2c_imx->pins.sda_pin =
> > +		of_get_named_gpio(pdev->dev.of_node, "recover-sda", 0);
> > +	i2c_imx->pins.scl_pin =
> > +		of_get_named_gpio(pdev->dev.of_node, "recover-scl", 0);
> I'd use devm_gpiod_get_optional here.

Thanks for your good suggestion. I will change it in the next version.

Re: [Patch v1] i2c: imx: implement bus recovery

[Jan Lübbe]

On Di, 2015-07-14 at 08:12 +0000, Gao Pandy wrote:
> > > --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > > @@ -14,6 +14,8 @@ Optional properties:
> > >    The absence of the propoerty indicates the default frequency
> 100 kHz.
> > >  - dmas: A list of two dma specifiers, one for each entry in
> dma-names.
> > >  - dma-names: should contain "tx" and "rx".
> > > +- recover-scl: specify the gpio related to SCL pin
> > > +- recover-sda: specify the gpio related to SDA pin
> > I don't like the naming here. That the gpios are used for recovery
> isn't
> > a hardware description. What about "scl-gpio" and "sda-gpio"?
> 
> Thanks. I will change it in the next version. 

There is already a binding for i2c-gpio:
Documentation/devicetree/bindings/i2c/i2c-gpio.txt

We should be able to reuse those properties as-is, instead of defining
something for each i2c controller binding.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [Patch v1] i2c: imx: implement bus recovery

[Gao Pandy]

From: Jan Lübbe <mailto:jlu@pengutronix.de> Sent: Thursday, July 16, 2015 8:58 PM
> To: Gao Pan-B54642
> Cc: Uwe Kleine-König; wsa@the-dreams.de; linux-i2c@vger.kernel.org; Li
> Frank-B20596; Duan Fugang-B38611
> Subject: Re: [Patch v1] i2c: imx: implement bus recovery
> 
> On Di, 2015-07-14 at 08:12 +0000, Gao Pandy wrote:
> > > > --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > > > @@ -14,6 +14,8 @@ Optional properties:
> > > >    The absence of the propoerty indicates the default frequency
> > 100 kHz.
> > > >  - dmas: A list of two dma specifiers, one for each entry in
> > dma-names.
> > > >  - dma-names: should contain "tx" and "rx".
> > > > +- recover-scl: specify the gpio related to SCL pin
> > > > +- recover-sda: specify the gpio related to SDA pin
> > > I don't like the naming here. That the gpios are used for recovery
> > isn't
> > > a hardware description. What about "scl-gpio" and "sda-gpio"?
> >
> > Thanks. I will change it in the next version.
> 
> There is already a binding for i2c-gpio:
> Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> 
> We should be able to reuse those properties as-is, instead of defining
> something for each i2c controller binding.

Thanks. The hardware description in i2c-gpio.txt describes the case of 
i2c gpio. However, in our patch, we just want the sda and scl pins change
to gpio mode for bus recovery, after recovery, we set the two pins to i2c function,
i2c host controller will take over the transfer. So it's a different case.

Once we use the hardware description in i2c-gpio.txt, additional gpios should
be applied. Correspondingly, it also need plentiful code support.

Re: [Patch v1] i2c: imx: implement bus recovery

[Jan Lübbe]

On Fr, 2015-07-17 at 02:26 +0000, Gao Pandy wrote:
> From: Jan Lübbe <mailto:jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> Sent: Thursday, July 16, 2015 8:58 PM
> > To: Gao Pan-B54642
> > Cc: Uwe Kleine-König; wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; linux-i2c-u79uwXL29TZUIDd8j+nm9g@public.gmane.org.org; Li
> > Frank-B20596; Duan Fugang-B38611
> > Subject: Re: [Patch v1] i2c: imx: implement bus recovery
> > 
> > On Di, 2015-07-14 at 08:12 +0000, Gao Pandy wrote:
> > > > > --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> > > > > @@ -14,6 +14,8 @@ Optional properties:
> > > > >    The absence of the propoerty indicates the default frequency
> > > 100 kHz.
> > > > >  - dmas: A list of two dma specifiers, one for each entry in
> > > dma-names.
> > > > >  - dma-names: should contain "tx" and "rx".
> > > > > +- recover-scl: specify the gpio related to SCL pin
> > > > > +- recover-sda: specify the gpio related to SDA pin
> > > > I don't like the naming here. That the gpios are used for recovery
> > > isn't
> > > > a hardware description. What about "scl-gpio" and "sda-gpio"?
> > >
> > > Thanks. I will change it in the next version.
> > 
> > There is already a binding for i2c-gpio:
> > Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> > 
> > We should be able to reuse those properties as-is, instead of defining
> > something for each i2c controller binding.
> 
> Thanks. The hardware description in i2c-gpio.txt describes the case of 
> i2c gpio. However, in our patch, we just want the sda and scl pins change
> to gpio mode for bus recovery, after recovery, we set the two pins to i2c function,
> i2c host controller will take over the transfer. So it's a different case.

Please note that the devicetree binding should describe the hardware,
not what Linux is doing with the hardware. And on the HW-level I2C
recovery with GPIOs is basically the same as using the GPIOs for normal
I2C.

We don't want to have different DT bindings for every I2C controller
which needs to use GPIOs for recovery. We already know that the existing
i2c-gpio bindings are enough to do normal I2C transfers, so they should
also be good for the recovery case.

You also need to add devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org to CC for patches
changing DT bindings.

> Once we use the hardware description in i2c-gpio.txt, additional gpios should
> be applied. Correspondingly, it also need plentiful code support.

Which additional GPIOs do you mean?

By having a common binding for recovery (based on or identical to
i2c-gpios), we can have common setup code for GPIO recovery instead of
duplicating the DT parsing and setup code in each driver. We probably
should define the pinmux state name which is used for GPIO mode as
well. 

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |