[PATCH V5 1/2] i2c/adapter: Add bus recovery infrastructure

[PATCH V5 1/2] i2c/adapter: Add bus recovery infrastructure

[Viresh Kumar]

From: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>

Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c
protocol Rev. 03 section 3.1.16 titled "Bus clear".

http://www.nxp.com/documents/user_manual/UM10204.pdf

Sometimes during operation i2c bus hangs and we need to give dummy clocks to
slave device to start the transfer again. Now we may have capability in the bus
controller to generate these clocks or platform may have gpio pins which can be
toggled to generate dummy clocks. This patch supports both.

This patch also adds in generic bus recovery routines gpio or scl line based
which can be used by bus controller. In addition controller driver may provide
its own version of the bus recovery routine.

This doesn't support multi-master recovery for now.

Signed-off-by: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
---
V4->V5:
- section name corrected to 3.1.16
- merged gpio and non-gpio recovery routines to remove code redundancy
- Changed types of gpio and gpio-flags to unsigned and unsigned long
- Checking return value of get_gpio() now
- using DIV_ROUND_UP for calculating delay, to get more correct value

 drivers/i2c/i2c-core.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |  52 +++++++++++++++++
 2 files changed, 205 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a7edf98..bdc249a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -27,7 +27,9 @@
 
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/delay.h>
 #include <linux/errno.h>
+#include <linux/gpio.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
@@ -104,6 +106,116 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 #define i2c_device_uevent	NULL
 #endif	/* CONFIG_HOTPLUG */
 
+/* i2c bus recovery routines */
+static inline void set_scl_value(struct i2c_adapter *adap, int val)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+
+	if (bri->is_gpio_recovery)
+		gpio_set_value(bri->scl_gpio, val);
+	else
+		bri->set_scl(adap, val);
+}
+
+static inline int get_sda_value(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+
+	if (bri->is_gpio_recovery)
+		return gpio_get_value(bri->sda_gpio);
+	else
+		return bri->get_sda(adap);
+}
+
+static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	struct device *dev = &adap->dev;
+	int ret = 0;
+
+	if (bri->get_gpio) {
+		ret = bri->get_gpio(bri->scl_gpio);
+		if (ret) {
+			dev_warn(dev, "scl get_gpio: %d\n", bri->scl_gpio);
+			return ret;
+		}
+	}
+
+	ret = gpio_request_one(bri->scl_gpio, bri->scl_gpio_flags, "i2c-scl");
+	if (ret) {
+		dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
+		goto scl_put_gpio;
+	}
+
+	if (!bri->skip_sda_polling) {
+		if (bri->get_gpio)
+			ret = bri->get_gpio(bri->sda_gpio);
+
+		if (unlikely(ret ||
+			gpio_request_one(bri->sda_gpio, bri->sda_gpio_flags,
+				"i2c-sda"))) {
+			/* work without sda polling */
+			dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
+					bri->sda_gpio);
+			bri->skip_sda_polling = true;
+			if (!ret && bri->put_gpio)
+				bri->put_gpio(bri->sda_gpio);
+
+			ret = 0;
+		}
+	}
+
+scl_put_gpio:
+	if (bri->put_gpio)
+		bri->put_gpio(bri->scl_gpio);
+
+	return ret;
+}
+
+static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+
+	gpio_free(bri->scl_gpio);
+
+	if (!bri->skip_sda_polling) {
+		gpio_free(bri->sda_gpio);
+
+		if (bri->put_gpio)
+			bri->put_gpio(bri->sda_gpio);
+	}
+}
+
+static int i2c_recover_bus(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	unsigned long delay = 1000000;
+	int i, ret, val = 1;
+
+	if (bri->is_gpio_recovery) {
+		ret = i2c_get_gpios_for_recovery(adap);
+		if (ret)
+			return ret;
+	}
+
+	delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);
+
+	for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
+		set_scl_value(adap, val);
+		ndelay(delay);
+
+		/* break if sda got high, check only when scl line is high */
+		if (!bri->skip_sda_polling && val)
+			if (unlikely(get_sda_value(adap)))
+				break;
+	}
+
+	if (bri->is_gpio_recovery)
+		i2c_put_gpios_for_recovery(adap);
+
+	return 0;
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -896,6 +1008,47 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 			 "Failed to create compatibility class link\n");
 #endif
 
+	/* bus recovery specific initialization */
+	if (adap->bus_recovery_info) {
+		struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+
+		if (bri->recover_bus) {
+			dev_info(&adap->dev,
+				"registered for non-generic bus recovery\n");
+		} else {
+			/* Use generic recovery routines */
+			if (!bri->clock_rate_khz) {
+				dev_warn(&adap->dev,
+					"doesn't have valid recovery clock rate\n");
+				goto exit_recovery;
+			}
+
+			/* Most controller need 9 clocks at max */
+			if (!bri->clock_cnt)
+				bri->clock_cnt = 9;
+
+			bri->recover_bus = i2c_recover_bus;
+
+			if (bri->is_gpio_recovery) {
+				dev_info(&adap->dev,
+					"registered for gpio bus recovery\n");
+			} else if (bri->set_scl) {
+				if (!bri->skip_sda_polling && !bri->get_sda) {
+					dev_warn(&adap->dev,
+						"!get_sda. skip sda polling\n");
+					bri->skip_sda_polling = true;
+				}
+
+				dev_info(&adap->dev,
+					"registered for scl bus recovery\n");
+			} else {
+				dev_warn(&adap->dev,
+					"doesn't have valid recovery type\n");
+			}
+		}
+	}
+
+exit_recovery:
 	/* create pre-declared device nodes */
 	if (adap->nr < __i2c_first_dynamic_bus_num)
 		i2c_scan_static_board_info(adap);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5970266..c43e5c4 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -370,6 +370,55 @@ struct i2c_algorithm {
 	u32 (*functionality) (struct i2c_adapter *);
 };
 
+/**
+ * struct i2c_bus_recovery_info - I2c bus recovery information
+ * @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
+ *	pass it NULL to use generic ones, i.e. gpio or scl based.
+ * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
+ *	it became high or not. Only required if recover_bus == NULL.
+ * @is_gpio_recovery: true, select gpio type else scl type. Only required if
+ *	recover_bus == NULL.
+ * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
+ *	scl type recovery.
+ * @clock_cnt: count of max clocks to be generated. Required for both gpio and
+ *	scl type recovery.
+ * @set_scl: controller specific scl configuration routine. Only required if
+ *	is_gpio_recovery == false
+ * @get_sda: controller specific sda read routine. Only required if
+ *	is_gpio_recovery == false and skip_sda_polling == false.
+ * @get_gpio: called before recover_bus() to get padmux configured for scl line.
+ *	as gpio. Only required if is_gpio_recovery == true. Return 0 on success.
+ * @put_gpio: called after recover_bus() to get padmux configured for scl line
+ *	as scl. Only required if is_gpio_recovery == true.
+ * @scl_gpio: gpio number of the scl line. Only required if is_gpio_recovery ==
+ *	true.
+ * @sda_gpio: gpio number of the sda line. Only required if is_gpio_recovery ==
+ *	true and skip_sda_polling == false.
+ * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
+ *	GPIOF_OUT_INIT_LOW.
+ * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
+ *	GPIOF_OUT_INIT_LOW.
+ */
+struct i2c_bus_recovery_info {
+	int (*recover_bus)(struct i2c_adapter *);
+	bool skip_sda_polling;
+	bool is_gpio_recovery;
+	u32 clock_rate_khz;
+	u8 clock_cnt;
+
+	/* scl/sda recovery */
+	void (*set_scl)(struct i2c_adapter *, int val);
+	int (*get_sda)(struct i2c_adapter *);
+
+	/* gpio recovery */
+	int (*get_gpio)(unsigned gpio);
+	void (*put_gpio)(unsigned gpio);
+	unsigned scl_gpio;
+	unsigned sda_gpio;
+	unsigned long scl_gpio_flags;
+	unsigned long sda_gpio_flags;
+};
+
 /*
  * i2c_adapter is the structure used to identify a physical i2c bus along
  * with the access algorithms necessary to access it.
@@ -393,6 +442,9 @@ struct i2c_adapter {
 
 	struct mutex userspace_clients_lock;
 	struct list_head userspace_clients;
+
+	/* Pass valid pointer if recovery infrastructure is required */
+	struct i2c_bus_recovery_info *bus_recovery_info;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V5 2/2] i2c/designware: Provide i2c bus recovery support

[Viresh Kumar]

From: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>

Add bus recovery support for designware_i2c controller. It uses generic gpio
based i2c_gpio_recover_bus() routine.

Signed-off-by: Vincenzo Frascino <vincenzo.frascino-qxv4g6HH51o@public.gmane.org>
Signed-off-by: Shiraz Hashim <shiraz.hashim-qxv4g6HH51o@public.gmane.org>
Signed-off-by: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-core.c    |  7 ++++-
 drivers/i2c/busses/i2c-designware-platdrv.c | 40 +++++++++++++++++++++--
 include/linux/i2c/i2c-designware.h          | 49 +++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/i2c/i2c-designware.h

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 7b8ebbe..3073178 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -538,7 +538,12 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
 	if (ret == 0) {
 		dev_err(dev->dev, "controller timed out\n");
-		i2c_dw_init(dev);
+		if (adap->bus_recovery_info &&
+				adap->bus_recovery_info->recover_bus) {
+			dev_dbg(dev->dev, "try i2c bus recovery\n");
+			adap->bus_recovery_info->recover_bus(adap);
+		}
+
 		ret = -ETIMEDOUT;
 		goto done;
 	} else if (ret < 0)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0506fef..9e8b7e3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/i2c/i2c-designware.h>
 #include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/sched.h>
@@ -45,6 +46,7 @@ static struct i2c_algorithm i2c_dw_algo = {
 	.master_xfer	= i2c_dw_xfer,
 	.functionality	= i2c_dw_func,
 };
+
 static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 {
 	return clk_get_rate(dev->clk)/1000;
@@ -55,6 +57,8 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	struct dw_i2c_dev *dev;
 	struct i2c_adapter *adap;
 	struct resource *mem, *ioarea;
+	struct i2c_dw_pdata *pdata;
+	struct i2c_bus_recovery_info *recovery_info = NULL;
 	int irq, r;
 
 	/* NOTE: driver uses the static register mapping */
@@ -141,17 +145,47 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;
 
+	/* Bus recovery support */
+	pdata = dev_get_platdata(&pdev->dev);
+	if (pdata) {
+		recovery_info = kzalloc(sizeof(*recovery_info), GFP_KERNEL);
+		if (!recovery_info) {
+			adap->bus_recovery_info = NULL;
+			dev_err(&pdev->dev,
+				"failure to allocate memory for bus recovery\n");
+			goto skip_recovery;
+		}
+
+		recovery_info->is_gpio_recovery = true;
+		recovery_info->get_gpio = pdata->get_gpio;
+		recovery_info->put_gpio = pdata->put_gpio;
+		recovery_info->scl_gpio = pdata->scl_gpio;
+		recovery_info->scl_gpio_flags = pdata->scl_gpio_flags;
+		recovery_info->clock_rate_khz = clk_get_rate(dev->clk) / 1000;
+
+		if (!pdata->skip_sda_polling) {
+			recovery_info->sda_gpio = pdata->sda_gpio;
+			recovery_info->sda_gpio_flags = pdata->sda_gpio_flags;
+		}
+
+		adap->bus_recovery_info = recovery_info;
+	} else {
+		adap->bus_recovery_info = NULL;
+	}
+
+skip_recovery:
 	adap->nr = pdev->id;
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
 		dev_err(&pdev->dev, "failure adding adapter\n");
-		goto err_free_irq;
+		goto err_free_recovery_info;
 	}
 	of_i2c_register_devices(adap);
 
 	return 0;
 
-err_free_irq:
+err_free_recovery_info:
+	kfree(recovery_info);
 	free_irq(dev->irq, dev);
 err_iounmap:
 	iounmap(dev->base);
@@ -174,6 +208,8 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
 	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
 	struct resource *mem;
 
+	kfree(dev->adapter.bus_recovery_info);
+
 	platform_set_drvdata(pdev, NULL);
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
diff --git a/include/linux/i2c/i2c-designware.h b/include/linux/i2c/i2c-designware.h
new file mode 100644
index 0000000..d60cb61c
--- /dev/null
+++ b/include/linux/i2c/i2c-designware.h
@@ -0,0 +1,49 @@
+/*
+ * Synopsys DesignWare I2C adapter driver's platform data
+ *
+ * Copyright (C) 2012 ST Microelectronics.
+ * Author: Vincenzo Frascino <vincenzo.frascino-qxv4g6HH51o@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#ifndef I2C_DESIGNWARE_H
+#define I2C_DESIGNWARE_H
+
+#include <linux/platform_device.h>
+
+/*
+ * struct i2c_dw_pdata - Designware I2c platform data
+ * @scl_gpio: gpio number of the scl line.
+ * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
+ *	GPIOF_OUT_INIT_LOW.
+ * @get_gpio: called before recover_bus() to get padmux configured for scl line
+ *	as gpio. Only required if is_gpio_recovery == true
+ * @put_gpio: called after recover_bus() to get padmux configured for scl line
+ *	as scl. Only required if is_gpio_recovery == true
+ * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
+ *	it became high or not. Below are required only if this is false.
+ * @sda_gpio: gpio number of the sda line.
+ * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
+ *	GPIOF_OUT_INIT_LOW.
+ */
+struct i2c_dw_pdata {
+	int scl_gpio;
+	int scl_gpio_flags;
+	int (*get_gpio)(unsigned gpio);
+	void (*put_gpio)(unsigned gpio);
+
+	/* sda polling specific */
+	bool skip_sda_polling;
+	int sda_gpio;
+	int sda_gpio_flags;
+};
+
+#endif /* I2C_DESIGNWARE_H */
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Viresh Kumar]

On 28 September 2012 16:58, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c
> protocol Rev. 03 section 3.1.16 titled "Bus clear".
>
> http://www.nxp.com/documents/user_manual/UM10204.pdf
>
> Sometimes during operation i2c bus hangs and we need to give dummy clocks to
> slave device to start the transfer again. Now we may have capability in the bus
> controller to generate these clocks or platform may have gpio pins which can be
> toggled to generate dummy clocks. This patch supports both.
>
> This patch also adds in generic bus recovery routines gpio or scl line based
> which can be used by bus controller. In addition controller driver may provide
> its own version of the bus recovery routine.
>
> This doesn't support multi-master recovery for now.

Hi Uwe,

If you have gone through V5 and have no noticeable issues with it, can it have a
reviewed-by: from you?

--
viresh

Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Uwe Kleine-König]

Hello Viresh,

On Fri, Sep 28, 2012 at 04:58:43PM +0530, Viresh Kumar wrote:
> Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c
> protocol Rev. 03 section 3.1.16 titled "Bus clear".
> 
> http://www.nxp.com/documents/user_manual/UM10204.pdf
> 
> Sometimes during operation i2c bus hangs and we need to give dummy clocks to
> slave device to start the transfer again. Now we may have capability in the bus
> controller to generate these clocks or platform may have gpio pins which can be
> toggled to generate dummy clocks. This patch supports both.
> 
> This patch also adds in generic bus recovery routines gpio or scl line based
> which can be used by bus controller. In addition controller driver may provide
> its own version of the bus recovery routine.
> 
> This doesn't support multi-master recovery for now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> V5 Resend:
> - Fixed my email-id, removed st.com and added linaro.org
> V4->V5:
> - section name corrected to 3.1.16
> - merged gpio and non-gpio recovery routines to remove code redundancy
> - Changed types of gpio and gpio-flags to unsigned and unsigned long
> - Checking return value of get_gpio() now
> - using DIV_ROUND_UP for calculating delay, to get more correct value
> 
>  drivers/i2c/i2c-core.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    |  52 +++++++++++++++++
>  2 files changed, 205 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index a7edf98..bdc249a 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -27,7 +27,9 @@
>  
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> +#include <linux/delay.h>
>  #include <linux/errno.h>
> +#include <linux/gpio.h>
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> @@ -104,6 +106,116 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>  #define i2c_device_uevent	NULL
>  #endif	/* CONFIG_HOTPLUG */
>  
> +/* i2c bus recovery routines */
> +static inline void set_scl_value(struct i2c_adapter *adap, int val)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> +	if (bri->is_gpio_recovery)
> +		gpio_set_value(bri->scl_gpio, val);
> +	else
> +		bri->set_scl(adap, val);
I would have done this in a different way (with function pointers
instead of an if for each bang). Well, I don't care much.

> +}
> +
> +static inline int get_sda_value(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> +	if (bri->is_gpio_recovery)
> +		return gpio_get_value(bri->sda_gpio);
> +	else
> +		return bri->get_sda(adap);
> +}
> +
> +static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +	struct device *dev = &adap->dev;
> +	int ret = 0;
> +
> +	if (bri->get_gpio) {
> +		ret = bri->get_gpio(bri->scl_gpio);
> +		if (ret) {
> +			dev_warn(dev, "scl get_gpio: %d\n", bri->scl_gpio);
> +			return ret;
> +		}
> +	}
> +
> +	ret = gpio_request_one(bri->scl_gpio, bri->scl_gpio_flags, "i2c-scl");
> +	if (ret) {
> +		dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
> +		goto scl_put_gpio;
> +	}
> +
> +	if (!bri->skip_sda_polling) {
> +		if (bri->get_gpio)
> +			ret = bri->get_gpio(bri->sda_gpio);
> +
> +		if (unlikely(ret ||
> +			gpio_request_one(bri->sda_gpio, bri->sda_gpio_flags,
> +				"i2c-sda"))) {
> +			/* work without sda polling */
> +			dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
> +					bri->sda_gpio);
> +			bri->skip_sda_polling = true;
> +			if (!ret && bri->put_gpio)
> +				bri->put_gpio(bri->sda_gpio);
> +
> +			ret = 0;
> +		}
> +	}
> +
> +scl_put_gpio:
> +	if (bri->put_gpio)
> +		bri->put_gpio(bri->scl_gpio);
> +
> +	return ret;
> +}
> +
> +static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> +	gpio_free(bri->scl_gpio);
> +
> +	if (!bri->skip_sda_polling) {
> +		gpio_free(bri->sda_gpio);
> +
> +		if (bri->put_gpio)
> +			bri->put_gpio(bri->sda_gpio);
> +	}
> +}
> +
> +static int i2c_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +	unsigned long delay = 1000000;
> +	int i, ret, val = 1;
> +
> +	if (bri->is_gpio_recovery) {
> +		ret = i2c_get_gpios_for_recovery(adap);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);
> +
> +	for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
> +		set_scl_value(adap, val);
> +		ndelay(delay);
> +
> +		/* break if sda got high, check only when scl line is high */
> +		if (!bri->skip_sda_polling && val)
> +			if (unlikely(get_sda_value(adap)))
> +				break;
> +	}
With clock_cnt usually being 9 (and skipping sda polling) assume a
device pulls down SDA because it was just addressed but the last clock
pulse to release SDA didn't occur:

	SDAout ¯\_/¯¯¯\___/¯¯¯\___________________/¯¯¯¯¯¯
        SCL    ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯¯¯¯¯
	        S   1   2   3   4   5   6   7   8   9

This adresses an eeprom with address 0x50. When SCL is released for the
9th clock the eeprom pulls down SDA to ack being addressed. After that
the eeprom expects the master to write a byte.

Now you do write 0xff:

	 i      0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17
	SDAin  ___/¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯\______
	SCL    ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯¯\__/¯¯\__/¯¯\__/¯¯\______/
	        9   1   2   3   4    5     6     7     8

(assuming the SCL line goes up some time later to its idle state).
That is you leave the bus in exactly the same state as before: The 9th
clock isn't complete and the eeprom asserts SDA low to ack the byte just
written. So the bus is still stalled. The problem is BTW the exact same
one I introduced first in my bus clear routine (for barebox though).
Even though you count to 2*9 you only do 8 real clock pulses because you
have a half cycle at both the start and the end.

> +
> +	if (bri->is_gpio_recovery)
> +		i2c_put_gpios_for_recovery(adap);
> +
> +	return 0;
> +}
> +
>  static int i2c_device_probe(struct device *dev)
>  {
>  	struct i2c_client	*client = i2c_verify_client(dev);
> @@ -896,6 +1008,47 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  			 "Failed to create compatibility class link\n");
>  #endif
>  
> +	/* bus recovery specific initialization */
> +	if (adap->bus_recovery_info) {
> +		struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> +		if (bri->recover_bus) {
> +			dev_info(&adap->dev,
> +				"registered for non-generic bus recovery\n");
> +		} else {
> +			/* Use generic recovery routines */
> +			if (!bri->clock_rate_khz) {
> +				dev_warn(&adap->dev,
> +					"doesn't have valid recovery clock rate\n");
> +				goto exit_recovery;
> +			}
> +
> +			/* Most controller need 9 clocks at max */
> +			if (!bri->clock_cnt)
> +				bri->clock_cnt = 9;
> +
> +			bri->recover_bus = i2c_recover_bus;
> +
> +			if (bri->is_gpio_recovery) {
> +				dev_info(&adap->dev,
> +					"registered for gpio bus recovery\n");
> +			} else if (bri->set_scl) {
> +				if (!bri->skip_sda_polling && !bri->get_sda) {
> +					dev_warn(&adap->dev,
> +						"!get_sda. skip sda polling\n");
> +					bri->skip_sda_polling = true;
> +				}
> +
> +				dev_info(&adap->dev,
> +					"registered for scl bus recovery\n");
> +			} else {
> +				dev_warn(&adap->dev,
> +					"doesn't have valid recovery type\n");
> +			}
> +		}
> +	}
> +
> +exit_recovery:
>  	/* create pre-declared device nodes */
>  	if (adap->nr < __i2c_first_dynamic_bus_num)
>  		i2c_scan_static_board_info(adap);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 5970266..c43e5c4 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -370,6 +370,55 @@ struct i2c_algorithm {
>  	u32 (*functionality) (struct i2c_adapter *);
>  };
>  
> +/**
> + * struct i2c_bus_recovery_info - I2c bus recovery information
> + * @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
> + *	pass it NULL to use generic ones, i.e. gpio or scl based.
> + * @skip_sda_polling: if true, bus recovery will not poll sda line to check if
> + *	it became high or not. Only required if recover_bus == NULL.
> + * @is_gpio_recovery: true, select gpio type else scl type. Only required if
> + *	recover_bus == NULL.
> + * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
> + *	scl type recovery.
> + * @clock_cnt: count of max clocks to be generated. Required for both gpio and
> + *	scl type recovery.
> + * @set_scl: controller specific scl configuration routine. Only required if
> + *	is_gpio_recovery == false
> + * @get_sda: controller specific sda read routine. Only required if
> + *	is_gpio_recovery == false and skip_sda_polling == false.
> + * @get_gpio: called before recover_bus() to get padmux configured for scl line.
> + *	as gpio. Only required if is_gpio_recovery == true. Return 0 on success.
> + * @put_gpio: called after recover_bus() to get padmux configured for scl line
> + *	as scl. Only required if is_gpio_recovery == true.
> + * @scl_gpio: gpio number of the scl line. Only required if is_gpio_recovery ==
> + *	true.
> + * @sda_gpio: gpio number of the sda line. Only required if is_gpio_recovery ==
> + *	true and skip_sda_polling == false.
> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
> + *	GPIOF_OUT_INIT_LOW.
> + * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
> + *	GPIOF_OUT_INIT_LOW.
Didn't you want to change this to GPIOF_OUT_INIT_HIGH |
GPIOF_OPEN_DRAIN? Hmm, I just wonder how to distinguish
GPIOF_OUT_INIT_LOW from unset. Hmm, maybe assume unset because _LOW is
wrong?

Best regards
Uwe

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

Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Viresh Kumar]

On 4 October 2012 13:30, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Fri, Sep 28, 2012 at 04:58:43PM +0530, Viresh Kumar wrote:

>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c

>> +static inline void set_scl_value(struct i2c_adapter *adap, int val)
>> +{
>> +     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +
>> +     if (bri->is_gpio_recovery)
>> +             gpio_set_value(bri->scl_gpio, val);
>> +     else
>> +             bri->set_scl(adap, val);
> I would have done this in a different way (with function pointers
> instead of an if for each bang). Well, I don't care much.

That would be better. Will be fixed :)

>> +static int i2c_recover_bus(struct i2c_adapter *adap)
>> +{
>> +     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +     unsigned long delay = 1000000;
>> +     int i, ret, val = 1;
>> +
>> +     if (bri->is_gpio_recovery) {
>> +             ret = i2c_get_gpios_for_recovery(adap);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);
>> +
>> +     for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
>> +             set_scl_value(adap, val);
>> +             ndelay(delay);
>> +
>> +             /* break if sda got high, check only when scl line is high */
>> +             if (!bri->skip_sda_polling && val)
>> +                     if (unlikely(get_sda_value(adap)))
>> +                             break;
>> +     }
> With clock_cnt usually being 9 (and skipping sda polling) assume a
> device pulls down SDA because it was just addressed but the last clock
> pulse to release SDA didn't occur:
>
>         SDAout ¯\_/¯¯¯\___/¯¯¯\___________________/¯¯¯¯¯¯
>         SCL    ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯¯¯¯¯
>                 S   1   2   3   4   5   6   7   8   9
>
> This adresses an eeprom with address 0x50. When SCL is released for the
> 9th clock the eeprom pulls down SDA to ack being addressed. After that
> the eeprom expects the master to write a byte.
>
> Now you do write 0xff:
>
>          i      0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17
>         SDAin  ___/¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯\______
>         SCL    ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯¯\__/¯¯\__/¯¯\__/¯¯\______/
>                 9   1   2   3   4    5     6     7     8
>
> (assuming the SCL line goes up some time later to its idle state).
> That is you leave the bus in exactly the same state as before: The 9th
> clock isn't complete and the eeprom asserts SDA low to ack the byte just
> written. So the bus is still stalled. The problem is BTW the exact same
> one I introduced first in my bus clear routine (for barebox though).
> Even though you count to 2*9 you only do 8 real clock pulses because you
> have a half cycle at both the start and the end.

Fantastic explanation!!
So, we actually need to do "Low-High" 9 times instead of "High-Low".
So, initializing val to 0 should fix it?

>> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
>> + *   GPIOF_OUT_INIT_LOW.
>> + * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
>> + *   GPIOF_OUT_INIT_LOW.

> Didn't you want to change this to GPIOF_OUT_INIT_HIGH |
> GPIOF_OPEN_DRAIN? Hmm, I just wonder how to distinguish
> GPIOF_OUT_INIT_LOW from unset. Hmm, maybe assume unset because _LOW is
> wrong?

Hmmm.... Hmmmm... Hmmm... :)
Two things:
- Why should we default it to GPIOF_OPEN_DRAIN? Open drain would mean,
gpio_set_value() will not write one for it, but would put it in input mode.
I don't think that would be correct, as an generic case.
- Obviously _LOW seems to be incorrect. So we can actually do something as
simple as: pass (sda_gpio_flags | GPIOF_OUT_INIT_HIGH). That will work
in both cases, user passed a value or not.

--
viresh

Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Uwe Kleine-König]

Hello,

> >> +     for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
> >> +             set_scl_value(adap, val);
> >> +             ndelay(delay);
> >> +
> >> +             /* break if sda got high, check only when scl line is high */
> >> +             if (!bri->skip_sda_polling && val)
> >> +                     if (unlikely(get_sda_value(adap)))
> >> +                             break;
> >> +     }
> > With clock_cnt usually being 9 (and skipping sda polling) assume a
> > device pulls down SDA because it was just addressed but the last clock
> > pulse to release SDA didn't occur:
> >
> >         SDAout ¯\_/¯¯¯\___/¯¯¯\___________________/¯¯¯¯¯¯
> >         SCL    ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯\_/¯¯¯¯¯
> >                 S   1   2   3   4   5   6   7   8   9
> >
> > This adresses an eeprom with address 0x50. When SCL is released for the
> > 9th clock the eeprom pulls down SDA to ack being addressed. After that
> > the eeprom expects the master to write a byte.
> >
> > Now you do write 0xff:
> >
> >          i      0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17
> >         SDAin  ___/¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯\______
> >         SCL    ¯¯\_/¯\_/¯\_/¯\_/¯\_/¯¯\__/¯¯\__/¯¯\__/¯¯\______/
> >                 9   1   2   3   4    5     6     7     8
> >
> > (assuming the SCL line goes up some time later to its idle state).
> > That is you leave the bus in exactly the same state as before: The 9th
> > clock isn't complete and the eeprom asserts SDA low to ack the byte just
> > written. So the bus is still stalled. The problem is BTW the exact same
> > one I introduced first in my bus clear routine (for barebox though).
> > Even though you count to 2*9 you only do 8 real clock pulses because you
> > have a half cycle at both the start and the end.
> 
> Fantastic explanation!!
Your luck that I just had the same problem ;-)

> So, we actually need to do "Low-High" 9 times instead of "High-Low".
> So, initializing val to 0 should fix it?
I don't think this is enough. If you cut off the last half clock of the
first sequence above doing 9 times low-high isn't enough. So you have to
do high + 9x low-high to assert 9 full cycles.

> >> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
> >> + *   GPIOF_OUT_INIT_LOW.
> >> + * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
> >> + *   GPIOF_OUT_INIT_LOW.
> 
> > Didn't you want to change this to GPIOF_OUT_INIT_HIGH |
> > GPIOF_OPEN_DRAIN? Hmm, I just wonder how to distinguish
> > GPIOF_OUT_INIT_LOW from unset. Hmm, maybe assume unset because _LOW is
> > wrong?
> 
> Hmmm.... Hmmmm... Hmmm... :)
> Two things:
> - Why should we default it to GPIOF_OPEN_DRAIN? Open drain would mean,
> gpio_set_value() will not write one for it, but would put it in input mode.
> I don't think that would be correct, as an generic case.
As the i2c-bus is open drain and multi-master capable it's wrong to pull
it to 1 because this can result in a short circuit. Even in a
single-master scenario (where you can pull SCL in both directions) you
must not drive SDA to 1.

Best regards
Uwe

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

Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Viresh Kumar]

On 4 October 2012 14:50, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:

>> So, we actually need to do "Low-High" 9 times instead of "High-Low".
>> So, initializing val to 0 should fix it?
> I don't think this is enough. If you cut off the last half clock of the
> first sequence above doing 9 times low-high isn't enough. So you have to
> do high + 9x low-high to assert 9 full cycles.

I am not cutting the last half clock. val is the variable which keeps
track of value to be
set on the line. I am asking to start from zero.

You mean, we should do a high first and then 9 low-high? But this line was high
initially.. what difference will it make by making it high again?

Sorry, i am not a I2C expert, so need some help :)

>> >> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
>> >> + *   GPIOF_OUT_INIT_LOW.
>> >> + * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
>> >> + *   GPIOF_OUT_INIT_LOW.
>>
>> > Didn't you want to change this to GPIOF_OUT_INIT_HIGH |
>> > GPIOF_OPEN_DRAIN? Hmm, I just wonder how to distinguish
>> > GPIOF_OUT_INIT_LOW from unset. Hmm, maybe assume unset because _LOW is
>> > wrong?
>>
>> Hmmm.... Hmmmm... Hmmm... :)
>> Two things:
>> - Why should we default it to GPIOF_OPEN_DRAIN? Open drain would mean,
>> gpio_set_value() will not write one for it, but would put it in input mode.
>> I don't think that would be correct, as an generic case.
> As the i2c-bus is open drain and multi-master capable it's wrong to pull
> it to 1 because this can result in a short circuit. Even in a
> single-master scenario (where you can pull SCL in both directions) you
> must not drive SDA to 1.

Hmm... Hopefully i understood it well this time.
- We actually don't need these flags then.

Always pass:
- scl_flags: GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH
- sda_flags: GPIOF_IN

Why would anybody else require something different for any SoC?

--
viresh

Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Uwe Kleine-König]

On Thu, Oct 04, 2012 at 03:02:26PM +0530, Viresh Kumar wrote:
> On 4 October 2012 14:50, Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> 
> >> So, we actually need to do "Low-High" 9 times instead of "High-Low".
> >> So, initializing val to 0 should fix it?
> > I don't think this is enough. If you cut off the last half clock of the
> > first sequence above doing 9 times low-high isn't enough. So you have to
> > do high + 9x low-high to assert 9 full cycles.
> 
> I am not cutting the last half clock. val is the variable which keeps
> track of value to be
> set on the line. I am asking to start from zero.
I meant the sequence that created the stall, not the one intending to
clear it. If you remove the last rising edge from that the SCL line is
initially low.
 
> You mean, we should do a high first and then 9 low-high? But this line was high
> initially.. what difference will it make by making it high again?
> 
> Sorry, i am not a I2C expert, so need some help :)
:(

> >> >> + * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
> >> >> + *   GPIOF_OUT_INIT_LOW.
> >> >> + * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
> >> >> + *   GPIOF_OUT_INIT_LOW.
> >>
> >> > Didn't you want to change this to GPIOF_OUT_INIT_HIGH |
> >> > GPIOF_OPEN_DRAIN? Hmm, I just wonder how to distinguish
> >> > GPIOF_OUT_INIT_LOW from unset. Hmm, maybe assume unset because _LOW is
> >> > wrong?
> >>
> >> Hmmm.... Hmmmm... Hmmm... :)
> >> Two things:
> >> - Why should we default it to GPIOF_OPEN_DRAIN? Open drain would mean,
> >> gpio_set_value() will not write one for it, but would put it in input mode.
> >> I don't think that would be correct, as an generic case.
> > As the i2c-bus is open drain and multi-master capable it's wrong to pull
> > it to 1 because this can result in a short circuit. Even in a
> > single-master scenario (where you can pull SCL in both directions) you
> > must not drive SDA to 1.
> 
> Hmm... Hopefully i understood it well this time.
> - We actually don't need these flags then.
> 
> Always pass:
> - scl_flags: GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH
> - sda_flags: GPIOF_IN
> 
> Why would anybody else require something different for any SoC?
IMHO this should work, yes. (At least conceptually, not necessary for
all implementations.)

Best regards
Uwe

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

Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Viresh Kumar]

On 4 October 2012 15:17, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Thu, Oct 04, 2012 at 03:02:26PM +0530, Viresh Kumar wrote:
>> On 4 October 2012 14:50, Uwe Kleine-König
>> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>>
>> >> So, we actually need to do "Low-High" 9 times instead of "High-Low".
>> >> So, initializing val to 0 should fix it?
>> > I don't think this is enough. If you cut off the last half clock of the
>> > first sequence above doing 9 times low-high isn't enough. So you have to
>> > do high + 9x low-high to assert 9 full cycles.
>>
>> I am not cutting the last half clock. val is the variable which keeps
>> track of value to be
>> set on the line. I am asking to start from zero.
> I meant the sequence that created the stall, not the one intending to
> clear it. If you remove the last rising edge from that the SCL line is
> initially low.

But then, wouldn't only 8.5 cycles are enough? As with 8.5 cycles we will
achieve 9 low-high cycles?

I can't find you on IRC (#linaro on freenode). Want to finish this up quickly,
so that i can send a fixup patch ASAP and get your reviewed-by :)

--
viresh

--
viresh

Re: [PATCH V5 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Viresh Kumar]

On 4 October 2012 13:30, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:

Hi Uwe,

Please see if following fixup looks fine to you:


    fixup! i2c/adapter: Add bus recovery infrastructure
---
 drivers/i2c/i2c-core.c | 33 ++++++++++++++-------------------
 include/linux/i2c.h    | 22 ++++++++++++----------
 2 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index bdc249a..393d5f7 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -107,24 +107,14 @@ static int i2c_device_uevent(struct device *dev,
struct kobj_uevent_env *env)
 #endif /* CONFIG_HOTPLUG */

 /* i2c bus recovery routines */
-static inline void set_scl_value(struct i2c_adapter *adap, int val)
+static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
 {
-       struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
-
-       if (bri->is_gpio_recovery)
-               gpio_set_value(bri->scl_gpio, val);
-       else
-               bri->set_scl(adap, val);
+       gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
 }

-static inline int get_sda_value(struct i2c_adapter *adap)
+static int get_sda_gpio_value(struct i2c_adapter *adap)
 {
-       struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
-
-       if (bri->is_gpio_recovery)
-               return gpio_get_value(bri->sda_gpio);
-       else
-               return bri->get_sda(adap);
+       return gpio_get_value(adap->bus_recovery_info->sda_gpio);
 }

 static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
@@ -152,8 +142,7 @@ static int i2c_get_gpios_for_recovery(struct
i2c_adapter *adap)
                        ret = bri->get_gpio(bri->sda_gpio);

                if (unlikely(ret ||
-                       gpio_request_one(bri->sda_gpio, bri->sda_gpio_flags,
-                               "i2c-sda"))) {
+                       gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda"))) {
                        /* work without sda polling */
                        dev_warn(dev, "can't get sda: %d. Skip sda polling\n",
                                        bri->sda_gpio);
@@ -190,7 +179,7 @@ static int i2c_recover_bus(struct i2c_adapter *adap)
 {
        struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
        unsigned long delay = 1000000;
-       int i, ret, val = 1;
+       int i, ret, val = 0;

        if (bri->is_gpio_recovery) {
                ret = i2c_get_gpios_for_recovery(adap);
@@ -201,12 +190,12 @@ static int i2c_recover_bus(struct i2c_adapter *adap)
        delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);

        for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
-               set_scl_value(adap, val);
+               bri->set_scl(adap, val);
                ndelay(delay);

                /* break if sda got high, check only when scl line is high */
                if (!bri->skip_sda_polling && val)
-                       if (unlikely(get_sda_value(adap)))
+                       if (unlikely(bri->get_sda(adap)))
                                break;
        }

@@ -1030,6 +1019,12 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
                        bri->recover_bus = i2c_recover_bus;

                         if (bri->is_gpio_recovery) {
+                               if (!bri->scl_gpio_flags)
+                                       bri->scl_gpio_flags = GPIOF_OPEN_DRAIN |
+                                               GPIOF_OUT_INIT_HIGH;
+
+                               bri->set_scl = set_scl_gpio_value;
+                               bri->get_sda = get_sda_gpio_value;
                                dev_info(&adap->dev,
                                        "registered for gpio bus recovery\n");
                        } else if (bri->set_scl) {
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index c43e5c4..dd470a1 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -382,10 +382,10 @@ struct i2c_algorithm {
  *     scl type recovery.
  * @clock_cnt: count of max clocks to be generated. Required for both gpio and
  *     scl type recovery.
- * @set_scl: controller specific scl configuration routine. Only required if
- *     is_gpio_recovery == false
- * @get_sda: controller specific sda read routine. Only required if
- *     is_gpio_recovery == false and skip_sda_polling == false.
+ * @set_scl: controller specific routine, if is_gpio_recovery == false.
+ *       set_scl_gpio_value otherwise
+ * @get_sda: controller specific routine, if is_gpio_recovery == false.
+ *       get_sda_gpio_value otherwise
  * @get_gpio: called before recover_bus() to get padmux configured
for scl line.
  *     as gpio. Only required if is_gpio_recovery == true. Return 0 on success.
  * @put_gpio: called after recover_bus() to get padmux configured for scl line
@@ -394,10 +394,9 @@ struct i2c_algorithm {
  *     true.
  * @sda_gpio: gpio number of the sda line. Only required if is_gpio_recovery ==
  *     true and skip_sda_polling == false.
- * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. 0 implies
- *     GPIOF_OUT_INIT_LOW.
- * @sda_gpio_flags: flag for gpio_request_one of sda_gpio. 0 implies
- *     GPIOF_OUT_INIT_LOW.
+ * @scl_gpio_flags: flag for gpio_request_one of scl_gpio. If passed as 0,
+ *      (GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH) is used instead.
+ *      These is no need of sda_gpio_flags, as we always read it in input mode.
  */
 struct i2c_bus_recovery_info {
        int (*recover_bus)(struct i2c_adapter *);
@@ -406,7 +405,11 @@ struct i2c_bus_recovery_info {
        u32 clock_rate_khz;
        u8 clock_cnt;

-       /* scl/sda recovery */
+       /*
+        * Fn pointers for recovery, will point either to:
+        * - set_scl_gpio_value and get_sda_gpio_value for gpio recovery
+        * - Controller specific routines, otherwise
+        */
        void (*set_scl)(struct i2c_adapter *, int val);
        int (*get_sda)(struct i2c_adapter *);

@@ -416,7 +419,6 @@ struct i2c_bus_recovery_info {
        unsigned scl_gpio;
        unsigned sda_gpio;
        unsigned long scl_gpio_flags;
-       unsigned long sda_gpio_flags;
 };

 /*