[Patch V5] i2c: imx: implement bus recovery

[Patch V5] 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>
---
V2:
As Uwe Kleine-König's suggestion, the version do below changes:
 -replace of_get_named_gpio() with devm_gpiod_get_optional()
 -move gpio_direction_input() and gpio_direction_output() call to the
  prepare callback
 -use 0 and 1 as return value for the get_scl and get_sda callbacks

V3:
 -replace "...-gpio" with "...-gpios" in i2c binding doc
 -document the requirement of using sleep state to configure
  the pins as gpio in i2c binding doc
 -remove i2c_recover_bus() in i2c_imx_trx_complete()
 -use GPIOD_OUT_HIGH as the parameter of devm_gpiod_get_optional to
  config the gpios as output high
 -add error disposal as devm_gpiod_get_optional meets error

V4:
 -remove include <linux/of_gpio.h>
 -call i2c_recover_bus under the condition of the existing of
  both sda and scl. Drop the sda and scl check in i2c_imx_get_scl,
  i2c_imx_get_sda, i2c_imx_set_scl, i2c_imx_prepare_recovery and
  i2c_imx_prepare_recovery
 -use GPIOD_OUT_IN as the parameter of devm_gpiod_get_optional
 -remove documenting the requirement of using sleep state to configure
  the pins as gpio in i2c binding doc

V5:
 -introduce a dedicated gpio state for bus recovery.
 -assign adapter.bus_recovery_info after the two gpios were aquired successfully.

 Documentation/devicetree/bindings/i2c/i2c-imx.txt |   9 ++
 drivers/i2c/busses/i2c-imx.c                      | 121 ++++++++++++++++++++--
 2 files changed, 123 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
index ce4311d..eab5836 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
@@ -14,6 +14,10 @@ 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".
+- scl-gpios: specify the gpio related to SCL pin
+- sda-gpios: specify the gpio related to SDA pin
+- pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
+  bus recovery, call it "gpio" state
 
 Examples:
 
@@ -37,4 +41,9 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */
 	dmas = <&edma0 0 50>,
 		<&edma0 0 51>;
 	dma-names = "rx","tx";
+	pinctrl-names = "default", "gpio";
+	pinctrl-0 = <&pinctrl_i2c1>;
+	pinctrl-1 = <&pinctrl_i2c1_gpio>;
+	scl-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH>;
+	sda-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;
 };
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index d7c823b..90c0e0f 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>
@@ -64,6 +65,8 @@
 /* Default value */
 #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
 
+#define I2C_PINCTRL_STATE_GPIO	"gpio"
+
 /*
  * Enable DMA if transfer byte size is bigger than this threshold.
  * As the hardware request, it must bigger than 4 bytes.\
@@ -178,6 +181,11 @@ enum imx_i2c_type {
 	VF610_I2C,
 };
 
+struct imx_i2c_pinctrl {
+	struct gpio_desc *sda;
+	struct gpio_desc *scl;
+};
+
 struct imx_i2c_hwdata {
 	enum imx_i2c_type	devtype;
 	unsigned		regshift;
@@ -209,8 +217,13 @@ 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 pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_pins_default;
+	struct pinctrl_state *pinctrl_pins_gpio;
+
 	struct imx_i2c_dma	*dma;
 };
 
@@ -439,7 +452,10 @@ 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;
+			if (i2c_imx->adapter.bus_recovery_info)
+				return i2c_recover_bus(&i2c_imx->adapter);
+			else
+				return -ETIMEDOUT;
 		}
 		schedule();
 	}
@@ -963,6 +979,62 @@ out:
 	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);
+
+	return gpiod_get_value(i2c_imx->pins.scl);
+}
+
+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);
+
+	return gpiod_get_value(i2c_imx->pins.sda);
+}
+
+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);
+
+	gpiod_set_value(i2c_imx->pins.scl, 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);
+
+	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_gpio);
+	gpiod_direction_input(i2c_imx->pins.sda);
+	gpiod_direction_output(i2c_imx->pins.scl, 1);
+}
+
+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);
+
+	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);
+}
+
+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
@@ -1011,12 +1083,12 @@ static int i2c_imx_probe(struct platform_device *pdev)
 
 	/* Setup i2c_imx driver structure */
 	strlcpy(i2c_imx->adapter.name, pdev->name, sizeof(i2c_imx->adapter.name));
-	i2c_imx->adapter.owner		= THIS_MODULE;
-	i2c_imx->adapter.algo		= &i2c_imx_algo;
-	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->base			= base;
+	i2c_imx->adapter.owner = THIS_MODULE;
+	i2c_imx->adapter.algo = &i2c_imx_algo;
+	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->base = base;
 
 	/* Get I2C clock */
 	i2c_imx->clk = devm_clk_get(&pdev->dev, NULL);
@@ -1031,6 +1103,23 @@ static int i2c_imx_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(i2c_imx->pinctrl)) {
+		ret = PTR_ERR(i2c_imx->pinctrl);
+		goto clk_disable;
+	}
+
+	i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
+						PINCTRL_STATE_DEFAULT);
+	if (IS_ERR(i2c_imx->pinctrl_pins_default))
+		dev_warn(&pdev->dev, "could not get default state\n");
+	else {
+		i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
+						I2C_PINCTRL_STATE_GPIO);
+		if (IS_ERR(i2c_imx->pinctrl_pins_gpio))
+			dev_warn(&pdev->dev, "could not get gpio state\n");
+	}
+
 	/* Request IRQ */
 	ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
 				pdev->name, i2c_imx);
@@ -1057,6 +1146,24 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto rpm_disable;
 
+	/* Init recover pins */
+	i2c_imx->pins.sda =
+		devm_gpiod_get_optional(&pdev->dev, "sda-gpios", GPIOD_IN);
+	i2c_imx->pins.scl =
+		devm_gpiod_get_optional(&pdev->dev, "scl-gpios", GPIOD_IN);
+
+	if (IS_ERR(i2c_imx->pins.sda)) {
+		ret = PTR_ERR(i2c_imx->pins.sda);
+		goto clk_disable;
+	}
+
+	if (IS_ERR(i2c_imx->pins.scl)) {
+		ret = PTR_ERR(i2c_imx->pins.scl);
+		goto clk_disable;
+	}
+
+	i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
+
 	/* Set up clock divider */
 	i2c_imx->bitrate = IMX_I2C_BIT_RATE;
 	ret = of_property_read_u32(pdev->dev.of_node,
-- 
1.9.1

[Patch V5] ARM: dts: imx6qdl-sabresd: add i2c pinctrl gpio state for bus recovery

[Gao Pan]

When i2c bus SCL/SDA are stuck low during transfer, the i2c
pinctrl should be configured to gpio mode to do bus recovery.

Genenally, we can use sleep state for bus recovery. But it's
not a good choice to use sleep state here because it will be
confused. This patch introduces a dedicated gpio state.

Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Signed-off-by: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 944eb81..6e97018 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -172,8 +172,11 @@
 
 &i2c1 {
 	clock-frequency = <100000>;
-	pinctrl-names = "default";
+	pinctrl-names = "default", "gpio";
 	pinctrl-0 = <&pinctrl_i2c1>;
+	pinctrl-1 = <&pinctrl_i2c1_gpio>;
+	scl-gpios = <&gpio5 26 0>;
+	sda-gpios = <&gpio5 27 0>;
 	status = "okay";
 
 	codec: wm8962@1a {
@@ -393,6 +396,13 @@
 			>;
 		};
 
+		pinctrl_i2c1_gpio: i2c1grp_gpio {
+			fsl,pins = <
+				MX6QDL_PAD_CSI0_DAT8__GPIO5_IO26        0x1b0b0
+				MX6QDL_PAD_CSI0_DAT9__GPIO5_IO27        0x1b0b0
+			>
+		};
+
 		pinctrl_i2c2: i2c2grp {
 			fsl,pins = <
 				MX6QDL_PAD_KEY_COL3__I2C2_SCL		0x4001b8b1
-- 
1.9.1

RE: [Patch V5] ARM: dts: imx6qdl-sabresd: add i2c pinctrl gpio state for bus recovery

[Gao Pandy]

Ping...
From: Gao Pan <mailto:b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org> Sent: Friday, September 11, 2015 6:43 PM
> To: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org;
> shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Li Frank-B20596; Duan Fugang-B38611; Gao
> Pan-B54642; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Subject: [Patch V5] ARM: dts: imx6qdl-sabresd: add i2c pinctrl gpio state
> for bus recovery
> 
> When i2c bus SCL/SDA are stuck low during transfer, the i2c pinctrl
> should be configured to gpio mode to do bus recovery.
> 
> Genenally, we can use sleep state for bus recovery. But it's not a good
> choice to use sleep state here because it will be confused. This patch
> introduces a dedicated gpio state.
> 
> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Signed-off-by: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index 944eb81..6e97018 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -172,8 +172,11 @@
> 
>  &i2c1 {
>  	clock-frequency = <100000>;
> -	pinctrl-names = "default";
> +	pinctrl-names = "default", "gpio";
>  	pinctrl-0 = <&pinctrl_i2c1>;
> +	pinctrl-1 = <&pinctrl_i2c1_gpio>;
> +	scl-gpios = <&gpio5 26 0>;
> +	sda-gpios = <&gpio5 27 0>;
>  	status = "okay";
> 
>  	codec: wm8962@1a {
> @@ -393,6 +396,13 @@
>  			>;
>  		};
> 
> +		pinctrl_i2c1_gpio: i2c1grp_gpio {
> +			fsl,pins = <
> +				MX6QDL_PAD_CSI0_DAT8__GPIO5_IO26        0x1b0b0
> +				MX6QDL_PAD_CSI0_DAT9__GPIO5_IO27        0x1b0b0
> +			>
> +		};
> +
>  		pinctrl_i2c2: i2c2grp {
>  			fsl,pins = <
>  				MX6QDL_PAD_KEY_COL3__I2C2_SCL		0x4001b8b1
> --
> 1.9.1

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

[Gao Pandy]

Ping...
From: Gao Pan <mailto:b54642@freescale.com> Sent: Friday, September 11, 2015 6:43 PM
> To: wsa@the-dreams.de; u.kleine-koenig@pengutronix.de;
> shawnguo@kernel.org; kernel@pengutronix.de
> Cc: linux-i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; Gao
> Pan-B54642; linux-arm-kernel@lists.infradead.org
> Subject: [Patch V5] i2c: imx: implement bus recovery
> 
> 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@freescale.com>
> Signed-off-by: Gao Pan <b54642@freescale.com>
> ---
> V2:
> As Uwe Kleine-König's suggestion, the version do below changes:
>  -replace of_get_named_gpio() with devm_gpiod_get_optional()  -move
> gpio_direction_input() and gpio_direction_output() call to the
>   prepare callback
>  -use 0 and 1 as return value for the get_scl and get_sda callbacks
> 
> V3:
>  -replace "...-gpio" with "...-gpios" in i2c binding doc  -document the
> requirement of using sleep state to configure
>   the pins as gpio in i2c binding doc
>  -remove i2c_recover_bus() in i2c_imx_trx_complete()  -use GPIOD_OUT_HIGH
> as the parameter of devm_gpiod_get_optional to
>   config the gpios as output high
>  -add error disposal as devm_gpiod_get_optional meets error
> 
> V4:
>  -remove include <linux/of_gpio.h>
>  -call i2c_recover_bus under the condition of the existing of
>   both sda and scl. Drop the sda and scl check in i2c_imx_get_scl,
>   i2c_imx_get_sda, i2c_imx_set_scl, i2c_imx_prepare_recovery and
>   i2c_imx_prepare_recovery
>  -use GPIOD_OUT_IN as the parameter of devm_gpiod_get_optional  -remove
> documenting the requirement of using sleep state to configure
>   the pins as gpio in i2c binding doc
> 
> V5:
>  -introduce a dedicated gpio state for bus recovery.
>  -assign adapter.bus_recovery_info after the two gpios were aquired
> successfully.
> 
>  Documentation/devicetree/bindings/i2c/i2c-imx.txt |   9 ++
>  drivers/i2c/busses/i2c-imx.c                      | 121
> ++++++++++++++++++++--
>  2 files changed, 123 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> index ce4311d..eab5836 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
> @@ -14,6 +14,10 @@ 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".
> +- scl-gpios: specify the gpio related to SCL pin
> +- sda-gpios: specify the gpio related to SDA pin
> +- pinctrl: add extra pinctrl to configure i2c pins to gpio function for
> +i2c
> +  bus recovery, call it "gpio" state
> 
>  Examples:
> 
> @@ -37,4 +41,9 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */
>  	dmas = <&edma0 0 50>,
>  		<&edma0 0 51>;
>  	dma-names = "rx","tx";
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&pinctrl_i2c1>;
> +	pinctrl-1 = <&pinctrl_i2c1_gpio>;
> +	scl-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH>;
> +	sda-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;
>  };
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index d7c823b..90c0e0f 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>
> @@ -64,6 +65,8 @@
>  /* Default value */
>  #define IMX_I2C_BIT_RATE	100000	/* 100kHz */
> 
> +#define I2C_PINCTRL_STATE_GPIO	"gpio"
> +
>  /*
>   * Enable DMA if transfer byte size is bigger than this threshold.
>   * As the hardware request, it must bigger than 4 bytes.\ @@ -178,6
> +181,11 @@ enum imx_i2c_type {
>  	VF610_I2C,
>  };
> 
> +struct imx_i2c_pinctrl {
> +	struct gpio_desc *sda;
> +	struct gpio_desc *scl;
> +};
> +
>  struct imx_i2c_hwdata {
>  	enum imx_i2c_type	devtype;
>  	unsigned		regshift;
> @@ -209,8 +217,13 @@ 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 pinctrl *pinctrl;
> +	struct pinctrl_state *pinctrl_pins_default;
> +	struct pinctrl_state *pinctrl_pins_gpio;
> +
>  	struct imx_i2c_dma	*dma;
>  };
> 
> @@ -439,7 +452,10 @@ 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;
> +			if (i2c_imx->adapter.bus_recovery_info)
> +				return i2c_recover_bus(&i2c_imx->adapter);
> +			else
> +				return -ETIMEDOUT;
>  		}
>  		schedule();
>  	}
> @@ -963,6 +979,62 @@ out:
>  	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);
> +
> +	return gpiod_get_value(i2c_imx->pins.scl);
> +}
> +
> +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);
> +
> +	return gpiod_get_value(i2c_imx->pins.sda);
> +}
> +
> +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);
> +
> +	gpiod_set_value(i2c_imx->pins.scl, 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);
> +
> +	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_gpio);
> +	gpiod_direction_input(i2c_imx->pins.sda);
> +	gpiod_direction_output(i2c_imx->pins.scl, 1); }
> +
> +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);
> +
> +	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx-
> >pinctrl_pins_default);
> +}
> +
> +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 @@ -1011,12 +1083,12 @@
> static int i2c_imx_probe(struct platform_device *pdev)
> 
>  	/* Setup i2c_imx driver structure */
>  	strlcpy(i2c_imx->adapter.name, pdev->name, sizeof(i2c_imx-
> >adapter.name));
> -	i2c_imx->adapter.owner		= THIS_MODULE;
> -	i2c_imx->adapter.algo		= &i2c_imx_algo;
> -	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->base			= base;
> +	i2c_imx->adapter.owner = THIS_MODULE;
> +	i2c_imx->adapter.algo = &i2c_imx_algo;
> +	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->base = base;
> 
>  	/* Get I2C clock */
>  	i2c_imx->clk = devm_clk_get(&pdev->dev, NULL); @@ -1031,6 +1103,23
> @@ static int i2c_imx_probe(struct platform_device *pdev)
>  		return ret;
>  	}
> 
> +	i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(i2c_imx->pinctrl)) {
> +		ret = PTR_ERR(i2c_imx->pinctrl);
> +		goto clk_disable;
> +	}
> +
> +	i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx-
> >pinctrl,
> +						PINCTRL_STATE_DEFAULT);
> +	if (IS_ERR(i2c_imx->pinctrl_pins_default))
> +		dev_warn(&pdev->dev, "could not get default state\n");
> +	else {
> +		i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx-
> >pinctrl,
> +						I2C_PINCTRL_STATE_GPIO);
> +		if (IS_ERR(i2c_imx->pinctrl_pins_gpio))
> +			dev_warn(&pdev->dev, "could not get gpio state\n");
> +	}
> +
>  	/* Request IRQ */
>  	ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
>  				pdev->name, i2c_imx);
> @@ -1057,6 +1146,24 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
>  	if (ret < 0)
>  		goto rpm_disable;
> 
> +	/* Init recover pins */
> +	i2c_imx->pins.sda =
> +		devm_gpiod_get_optional(&pdev->dev, "sda-gpios", GPIOD_IN);
> +	i2c_imx->pins.scl =
> +		devm_gpiod_get_optional(&pdev->dev, "scl-gpios", GPIOD_IN);
> +
> +	if (IS_ERR(i2c_imx->pins.sda)) {
> +		ret = PTR_ERR(i2c_imx->pins.sda);
> +		goto clk_disable;
> +	}
> +
> +	if (IS_ERR(i2c_imx->pins.scl)) {
> +		ret = PTR_ERR(i2c_imx->pins.scl);
> +		goto clk_disable;
> +	}
> +
> +	i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
> +
>  	/* 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 V5] i2c: imx: implement bus recovery

[Uwe Kleine-König]

Hello,

On Fri, Sep 11, 2015 at 06:42:34PM +0800, Gao Pan wrote:
> V5:
>  -introduce a dedicated gpio state for bus recovery.
>  -assign adapter.bus_recovery_info after the two gpios were aquired successfully.

You also do this if the gpios were not acquired successfully.

> +	if (IS_ERR(i2c_imx->pinctrl_pins_default))
> +		dev_warn(&pdev->dev, "could not get default state\n");
> +	else {
> +		i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
> +						I2C_PINCTRL_STATE_GPIO);
> +		if (IS_ERR(i2c_imx->pinctrl_pins_gpio))
> +			dev_warn(&pdev->dev, "could not get gpio state\n");

IMHO pinctrl_lookup_state returning an error is enough to not try a
recovery.

> +	}
> +
>  	/* Request IRQ */
>  	ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
>  				pdev->name, i2c_imx);
> @@ -1057,6 +1146,24 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto rpm_disable;
>  
> +	/* Init recover pins */
> +	i2c_imx->pins.sda =
> +		devm_gpiod_get_optional(&pdev->dev, "sda-gpios", GPIOD_IN);
> +	i2c_imx->pins.scl =
> +		devm_gpiod_get_optional(&pdev->dev, "scl-gpios", GPIOD_IN);
> +
> +	if (IS_ERR(i2c_imx->pins.sda)) {
> +		ret = PTR_ERR(i2c_imx->pins.sda);
> +		goto clk_disable;
> +	}
> +
> +	if (IS_ERR(i2c_imx->pins.scl)) {
> +		ret = PTR_ERR(i2c_imx->pins.scl);
> +		goto clk_disable;
> +	}
> +

if (i2c_imx->pins.sda && i2c_imx->pins.scl)

> +	i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
> +
>  	/* Set up clock divider */
>  	i2c_imx->bitrate = IMX_I2C_BIT_RATE;
>  	ret = of_property_read_u32(pdev->dev.of_node,

Best regards
Uwe

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

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

[Sascha Hauer]

Hi,

On Fri, Sep 11, 2015 at 06:42:34PM +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.

If SCL/SDA a stuck low *during* a transfer that transfer went wrong and
we have to return an error and the caller must repeat the transfer. We
can't just recover the bus in the middle of a transfer and claim the
transfer worked.
We can only do bus recovery on the start of a transfer. 

> @@ -439,7 +452,10 @@ 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;
> +			if (i2c_imx->adapter.bus_recovery_info)
> +				return i2c_recover_bus(&i2c_imx->adapter);
> +			else
> +				return -ETIMEDOUT;

In my tests with a stuck i2c bus this didn't work because this code
is not executed due to the arbitration lost check done before. The
I2SR_IAL bit was always set when the bus was stuck.

Also as mentioned above I think we should only execute the bus recovery
once in a transfer, when the controller fails to start. i2c_imx_bus_busy()
is called from far too many places.

>  		}
>  		schedule();
>  	}
> @@ -963,6 +979,62 @@ out:
>  	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);
> +
> +	return gpiod_get_value(i2c_imx->pins.scl);
> +}

There's a generic i2c_generic_gpio_recovery() which handles the bus
recovery using gpios. We should use it.

> @@ -1057,6 +1146,24 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto rpm_disable;
>  
> +	/* Init recover pins */
> +	i2c_imx->pins.sda =
> +		devm_gpiod_get_optional(&pdev->dev, "sda-gpios", GPIOD_IN);
> +	i2c_imx->pins.scl =
> +		devm_gpiod_get_optional(&pdev->dev, "scl-gpios", GPIOD_IN);

This cannot work. devm_gpiod_get_optional already appends "-gpios".

See my alternative approach below based on your code.

Sascha

---------------------------8<---------------------------

>From 32dcaeb4488d8311f68179adf261a909adc660a0 Mon Sep 17 00:00:00 2001
From: Gao Pan <b54642-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
Date: Fri, 11 Sep 2015 18:42:34 +0800
Subject: [PATCH] i2c: imx: implement bus recovery

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>
Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-imx.txt |  9 +++
 drivers/i2c/busses/i2c-imx.c                      | 69 +++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
index ce4311d..eab5836 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
@@ -14,6 +14,10 @@ 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".
+- scl-gpios: specify the gpio related to SCL pin
+- sda-gpios: specify the gpio related to SDA pin
+- pinctrl: add extra pinctrl to configure i2c pins to gpio function for i2c
+  bus recovery, call it "gpio" state
 
 Examples:
 
@@ -37,4 +41,9 @@ i2c0: i2c@40066000 { /* i2c0 on vf610 */
 	dmas = <&edma0 0 50>,
 		<&edma0 0 51>;
 	dma-names = "rx","tx";
+	pinctrl-names = "default", "gpio";
+	pinctrl-0 = <&pinctrl_i2c1>;
+	pinctrl-1 = <&pinctrl_i2c1_gpio>;
+	scl-gpios = <&gpio5 26 GPIO_ACTIVE_HIGH>;
+	sda-gpios = <&gpio5 27 GPIO_ACTIVE_HIGH>;
 };
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index a53a7dd..0aa5c42 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -49,6 +49,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_dma.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_data/i2c-imx.h>
 #include <linux/platform_device.h>
 #include <linux/sched.h>
@@ -207,6 +208,11 @@ struct imx_i2c_struct {
 	unsigned int		cur_clk;
 	unsigned int		bitrate;
 	const struct imx_i2c_hwdata	*hwdata;
+	struct i2c_bus_recovery_info rinfo;
+
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_pins_default;
+	struct pinctrl_state *pinctrl_pins_gpio;
 
 	struct imx_i2c_dma	*dma;
 };
@@ -896,6 +902,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 
 	/* Start I2C transfer */
 	result = i2c_imx_start(i2c_imx);
+	if (result) {
+		i2c_recover_bus(&i2c_imx->adapter);
+		result = i2c_imx_start(i2c_imx);
+	}
+
 	if (result)
 		goto fail0;
 
@@ -956,6 +967,55 @@ fail0:
 	return (result < 0) ? result : num;
 }
 
+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);
+
+	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_gpio);
+}
+
+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);
+
+	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);
+}
+
+static void i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
+		struct platform_device *pdev)
+{
+	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
+
+	i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
+			PINCTRL_STATE_DEFAULT);
+	i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
+			"gpio");
+	rinfo->sda_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+			"sda-gpios", 0, NULL);
+	rinfo->scl_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+			"scl-gpios", 0, NULL);
+
+	if (!gpio_is_valid(rinfo->sda_gpio) ||
+	    !gpio_is_valid(rinfo->scl_gpio) ||
+	    IS_ERR(i2c_imx->pinctrl_pins_default) ||
+	    IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
+		dev_dbg(&pdev->dev, "recovery information incomplete\n");
+		return;
+	}
+
+	dev_dbg(&pdev->dev, "using scl-gpio %d and sda-gpio %d for recovery\n",
+			rinfo->sda_gpio, rinfo->scl_gpio);
+
+	rinfo->prepare_recovery = i2c_imx_prepare_recovery;
+	rinfo->unprepare_recovery = i2c_imx_unprepare_recovery;
+	rinfo->recover_bus = i2c_generic_gpio_recovery;
+	i2c_imx->adapter.bus_recovery_info = rinfo;
+}
+
 static u32 i2c_imx_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
@@ -1023,6 +1083,13 @@ static int i2c_imx_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "can't enable I2C clock\n");
 		return ret;
 	}
+
+	i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(i2c_imx->pinctrl)) {
+		ret = PTR_ERR(i2c_imx->pinctrl);
+		goto clk_disable;
+	}
+
 	/* Request IRQ */
 	ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
 				pdev->name, i2c_imx);
@@ -1037,6 +1104,8 @@ static int i2c_imx_probe(struct platform_device *pdev)
 	/* Set up adapter data */
 	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
 
+	i2c_imx_init_recovery_info(i2c_imx, pdev);
+
 	/* Set up clock divider */
 	i2c_imx->bitrate = IMX_I2C_BIT_RATE;
 	ret = of_property_read_u32(pdev->dev.of_node,
-- 
2.5.1


-- 
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 V5] i2c: imx: implement bus recovery

[Gao Pandy]

From: linux-i2c-owner@vger.kernel.org <mailto:linux-i2c-owner@vger.kernel.org> Sent: Friday, September 18, 2015 3:55 PM
> To: Gao Pan-B54642
> Cc: wsa@the-dreams.de; shawnguo@kernel.org; kernel@pengutronix.de; linux-
> i2c@vger.kernel.org; Li Frank-B20596; Duan Fugang-B38611; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [Patch V5] i2c: imx: implement bus recovery
> 
> Hello,
> 
> On Fri, Sep 11, 2015 at 06:42:34PM +0800, Gao Pan wrote:
> > V5:
> >  -introduce a dedicated gpio state for bus recovery.
> >  -assign adapter.bus_recovery_info after the two gpios were aquired
> successfully.
> 
> You also do this if the gpios were not acquired successfully.
 
Thanks. If the gpios are not acquired successfully, the context goes to label clk_disable. So the assignment is skipped.
Please see the following code.

      ......
  
	if (IS_ERR(i2c_imx->pins.sda)) {
		ret = PTR_ERR(i2c_imx->pins.sda);
		goto clk_disable;
	}
	if (IS_ERR(i2c_imx->pins.scl)) {
		ret = PTR_ERR(i2c_imx->pins.scl);
		goto clk_disable;
	}
	
	i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;

	......


> 
> > +	if (IS_ERR(i2c_imx->pinctrl_pins_default))
> > +		dev_warn(&pdev->dev, "could not get default state\n");
> > +	else {
> > +		i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx-
> >pinctrl,
> > +						I2C_PINCTRL_STATE_GPIO);
> > +		if (IS_ERR(i2c_imx->pinctrl_pins_gpio))
> > +			dev_warn(&pdev->dev, "could not get gpio state\n");
> 
> IMHO pinctrl_lookup_state returning an error is enough to not try a
> recovery.
 
Thanks, you are right. How about the following logic.

	if (!(IS_ERR(i2c_imx->pinctrl_pins_default)) && !(IS_ERR(i2c_imx->pinctrl_pins_gpio))) {
                i2c_imx->pins.sda =
                        devm_gpiod_get_optional(&pdev->dev, "sda-gpios", GPIOD_IN);
                i2c_imx->pins.scl =
                        devm_gpiod_get_optional(&pdev->dev, "scl-gpios", GPIOD_IN);

                if (IS_ERR(i2c_imx->pins.sda)) {
                        ret = PTR_ERR(i2c_imx->pins.sda);
                        goto clk_disable;
                }

                if (IS_ERR(i2c_imx->pins.scl)) {
                        ret = PTR_ERR(i2c_imx->pins.scl);
                        goto clk_disable;
                }

                i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
	} 


> 
> > +	}
> > +
> >  	/* Request IRQ */
> >  	ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
> >  				pdev->name, i2c_imx);
> > @@ -1057,6 +1146,24 @@ static int i2c_imx_probe(struct platform_device
> *pdev)
> >  	if (ret < 0)
> >  		goto rpm_disable;
> >
> > +	/* Init recover pins */
> > +	i2c_imx->pins.sda =
> > +		devm_gpiod_get_optional(&pdev->dev, "sda-gpios", GPIOD_IN);
> > +	i2c_imx->pins.scl =
> > +		devm_gpiod_get_optional(&pdev->dev, "scl-gpios", GPIOD_IN);
> > +
> > +	if (IS_ERR(i2c_imx->pins.sda)) {
> > +		ret = PTR_ERR(i2c_imx->pins.sda);
> > +		goto clk_disable;
> > +	}
> > +
> > +	if (IS_ERR(i2c_imx->pins.scl)) {
> > +		ret = PTR_ERR(i2c_imx->pins.scl);
> > +		goto clk_disable;
> > +	}
> > +
> 
> if (i2c_imx->pins.sda && i2c_imx->pins.scl)
> 
> > +	i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
> > +
> >  	/* Set up clock divider */
> >  	i2c_imx->bitrate = IMX_I2C_BIT_RATE;
> >  	ret = of_property_read_u32(pdev->dev.of_node,
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 | http://www.pengutronix.de/
> |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

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

[Uwe Kleine-König]

Hello,

On Mon, Sep 21, 2015 at 04:29:20AM +0000, Gao Pandy wrote:
> > On Fri, Sep 11, 2015 at 06:42:34PM +0800, Gao Pan wrote:
> > > V5:
> > >  -introduce a dedicated gpio state for bus recovery.
> > >  -assign adapter.bus_recovery_info after the two gpios were aquired
> > >   successfully.
> > 
> > You also do this if the gpios were not acquired successfully.
>  
> Thanks. If the gpios are not acquired successfully, the context goes
> to label clk_disable. So the assignment is skipped.
> Please see the following code.
> 
>       ......
>   
> 	if (IS_ERR(i2c_imx->pins.sda)) {
> 		ret = PTR_ERR(i2c_imx->pins.sda);
> 		goto clk_disable;
> 	}
> 	if (IS_ERR(i2c_imx->pins.scl)) {
> 		ret = PTR_ERR(i2c_imx->pins.scl);
> 		goto clk_disable;
> 	}
> 	
> 	i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
> 
> 	......

Right, if devm_gpiod_get_optional returns an error probing fails and
everything is right. If however devm_gpiod_get_optional returns NULL
(i.e. the dt doesn't contain an scl-gpios property) you happily use them
even though gpio_set_value are stubs in this case.
 
> > IMHO pinctrl_lookup_state returning an error is enough to not try a
> > recovery.
>  
> Thanks, you are right. How about the following logic.
> 
> 	if (!(IS_ERR(i2c_imx->pinctrl_pins_default)) && !(IS_ERR(i2c_imx->pinctrl_pins_gpio))) {

After the ! you don't need a (, but apart from this the logic is fine.

>                 i2c_imx->pins.sda =
>                         devm_gpiod_get_optional(&pdev->dev, "sda-gpios", GPIOD_IN);
>                 i2c_imx->pins.scl =
>                         devm_gpiod_get_optional(&pdev->dev, "scl-gpios", GPIOD_IN);
> 
>                 if (IS_ERR(i2c_imx->pins.sda)) {
>                         ret = PTR_ERR(i2c_imx->pins.sda);
>                         goto clk_disable;
>                 }
> 
>                 if (IS_ERR(i2c_imx->pins.scl)) {
>                         ret = PTR_ERR(i2c_imx->pins.scl);
>                         goto clk_disable;
>                 }

As said above, here you need an

		if (i2c_imx->pins.scl && i2c_imx->pins.sda)
>                 i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
> 	} 

Best regards
Uwe

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

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

[Gao Pandy]

From: Uwe Kleine-König <mailto:u.kleine-koenig@pengutronix.de> Sent: Monday, September 21, 2015 2:33 PM
> To: Gao Pan-B54642
> Cc: Li Frank-B20596; wsa@the-dreams.de; linux-i2c@vger.kernel.org;
> kernel@pengutronix.de; Duan Fugang-B38611; shawnguo@kernel.org; linux-
> arm-kernel@lists.infradead.org
> Subject: Re: [Patch V5] i2c: imx: implement bus recovery
> 
> Hello,
> 
> On Mon, Sep 21, 2015 at 04:29:20AM +0000, Gao Pandy wrote:
> > > On Fri, Sep 11, 2015 at 06:42:34PM +0800, Gao Pan wrote:
> > > > V5:
> > > >  -introduce a dedicated gpio state for bus recovery.
> > > >  -assign adapter.bus_recovery_info after the two gpios were aquired
> > > >   successfully.
> > >
> > > You also do this if the gpios were not acquired successfully.
> >
> > Thanks. If the gpios are not acquired successfully, the context goes
> > to label clk_disable. So the assignment is skipped.
> > Please see the following code.
> >
> >       ......
> >
> > 	if (IS_ERR(i2c_imx->pins.sda)) {
> > 		ret = PTR_ERR(i2c_imx->pins.sda);
> > 		goto clk_disable;
> > 	}
> > 	if (IS_ERR(i2c_imx->pins.scl)) {
> > 		ret = PTR_ERR(i2c_imx->pins.scl);
> > 		goto clk_disable;
> > 	}
> >
> > 	i2c_imx->adapter.bus_recovery_info = &i2c_imx_bus_recovery_info;
> >
> > 	......
> 
> Right, if devm_gpiod_get_optional returns an error probing fails and
> everything is right. If however devm_gpiod_get_optional returns NULL (i.e.
> the dt doesn't contain an scl-gpios property) you happily use them even
> though gpio_set_value are stubs in this case.
 
Thanks, you are right.

> > > IMHO pinctrl_lookup_state returning an error is enough to not try a
> > > recovery.
> >
> > Thanks, you are right. How about the following logic.
> >
> > 	if (!(IS_ERR(i2c_imx->pinctrl_pins_default)) &&
> > !(IS_ERR(i2c_imx->pinctrl_pins_gpio))) {
> 
> After the ! you don't need a (, but apart from this the logic is fine.
> 
> >                 i2c_imx->pins.sda =
> >                         devm_gpiod_get_optional(&pdev->dev, "sda-gpios",
> GPIOD_IN);
> >                 i2c_imx->pins.scl =
> >                         devm_gpiod_get_optional(&pdev->dev,
> > "scl-gpios", GPIOD_IN);
> >
> >                 if (IS_ERR(i2c_imx->pins.sda)) {
> >                         ret = PTR_ERR(i2c_imx->pins.sda);
> >                         goto clk_disable;
> >                 }
> >
> >                 if (IS_ERR(i2c_imx->pins.scl)) {
> >                         ret = PTR_ERR(i2c_imx->pins.scl);
> >                         goto clk_disable;
> >                 }
> 
> As said above, here you need an
> 
> 		if (i2c_imx->pins.scl && i2c_imx->pins.sda)
> >                 i2c_imx->adapter.bus_recovery_info =
> &i2c_imx_bus_recovery_info;
> > 	}
 
Thank you very much, will fix it in next version.

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