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

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

[Viresh Kumar]

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->V6:
- Removed sda_gpio_flags
- Make scl_gpio_flags as GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH by default
- update bri->set_scl and bri->get_sda for gpio recovery case in i2c core
- Guaranteed to generate 9 falling-rising edges for bus recovery

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 | 156 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |  55 +++++++++++++++++
 2 files changed, 211 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a7edf98..e78033b 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,111 @@ 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 void set_scl_gpio_value(struct i2c_adapter *adap, int val)
+{
+	gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
+}
+
+static int get_sda_gpio_value(struct i2c_adapter *adap)
+{
+	return gpio_get_value(adap->bus_recovery_info->sda_gpio);
+}
+
+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, GPIOF_IN, "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 = 0;
+
+	if (bri->is_gpio_recovery) {
+		ret = i2c_get_gpios_for_recovery(adap);
+		if (ret)
+			return ret;
+	} else {
+		bri->set_scl(adap, 1);
+	}
+
+	/*
+	 * By this time SCL is high, as we need to give 9 falling-rising edges
+	 */
+
+	delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);
+
+	for (i = 0; i < bri->clock_cnt * 2; i++, val = !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(bri->get_sda(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 +1003,55 @@ 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) {
+				if (!bri->scl_gpio_flags)
+					bri->scl_gpio_flags = GPIOF_OPEN_DRAIN;
+
+				/* We always start by making GPIO HIGH */
+				bri->scl_gpio_flags |= 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) {
+				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..13eeb2e 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -370,6 +370,58 @@ 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 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
+ *	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. If passed as 0,
+ *      (GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH) is used instead. Otherwise, it
+ *      is ORRED with GPIOF_OUT_INIT_HIGH.
+ *      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 *);
+	bool skip_sda_polling;
+	bool is_gpio_recovery;
+	u32 clock_rate_khz;
+	u8 clock_cnt;
+
+	/*
+	 * 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 *);
+
+	/* gpio recovery */
+	int (*get_gpio)(unsigned gpio);
+	void (*put_gpio)(unsigned gpio);
+	unsigned scl_gpio;
+	unsigned sda_gpio;
+	unsigned long scl_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 +445,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 V6 2/2] i2c/designware: Provide i2c bus recovery support

[Viresh Kumar]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
V5->V6:
- removed sda_gpio_flags

 drivers/i2c/busses/i2c-designware-core.c    |  7 ++++-
 drivers/i2c/busses/i2c-designware-platdrv.c | 38 ++++++++++++++++++++++--
 include/linux/i2c/i2c-designware.h          | 46 +++++++++++++++++++++++++++++
 3 files changed, 88 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 cbba7db..9b7c9bf 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..afa0df9 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,45 @@ 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;
+
+		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 +206,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..522dec0
--- /dev/null
+++ b/include/linux/i2c/i2c-designware.h
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+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;
+};
+
+#endif /* I2C_DESIGNWARE_H */
-- 
1.7.12.rc2.18.g61b472e

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 6995 bytes --]

On Thu, Oct 04, 2012 at 04:34:54PM +0530, Viresh Kumar wrote:
> 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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> V5->V6:
> - removed sda_gpio_flags
> 
>  drivers/i2c/busses/i2c-designware-core.c    |  7 ++++-
>  drivers/i2c/busses/i2c-designware-platdrv.c | 38 ++++++++++++++++++++++--
>  include/linux/i2c/i2c-designware.h          | 46 +++++++++++++++++++++++++++++
>  3 files changed, 88 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 cbba7db..9b7c9bf 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);
> +		}
> +

This should be in the core?

>  		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..afa0df9 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,45 @@ 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;

It is probably easier to define the whole bri structure in the platform
and simply pass it on here? Hmm, now devicetree also comes to my mind.
Will think about that a little, too, but main implementation should be
left for someone needing that.

> +
> +		if (!pdata->skip_sda_polling)
> +			recovery_info->sda_gpio = pdata->sda_gpio;
> +
> +		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 +206,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..522dec0
> --- /dev/null
> +++ b/include/linux/i2c/i2c-designware.h
> @@ -0,0 +1,46 @@
> +/*
> + * 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.
> + */
> +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;
> +};
> +
> +#endif /* I2C_DESIGNWARE_H */
> -- 
> 1.7.12.rc2.18.g61b472e
> 
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Viresh Kumar]

On 25 November 2012 02:33, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>> diff --git 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);
>> +             }
>> +
>
> This should be in the core?

Because wait_for_completion() would fail in controllers, so i kept this
code here. How will we come to know about xfer failure in core?

Though i realize that check for adap->bus_recovery_info->recover_bus()
is just not required.

>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> +     /* 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;
>
> It is probably easier to define the whole bri structure in the platform
> and simply pass it on here? Hmm, now devicetree also comes to my mind.
> Will think about that a little, too, but main implementation should be
> left for someone needing that.

Ok. I didn't went for DT as recovery_info also has some routines, for which
we need special implementation for platforms.

--
viresh

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

> >> +             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);
> >> +             }
> >> +
> >
> > This should be in the core?
> 
> Because wait_for_completion() would fail in controllers, so i kept this
> code here. How will we come to know about xfer failure in core?

Somehow true. We need the clarification of .timeout and .retries in the
i2c subsystem first, then we can think of returning a specific
-Esomething here which could indicate that a recovery might help. Might
require another thinking if that should be different from -ETIMEDOUT.
Still, we need the cleanup first, and because this may take a while, it
is not your problem.

So, we could start like above and fix users later.


Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Viresh Kumar]

On 4 October 2012 16:34, 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 Wolfram,

I haven't pinged you earlier as i knew merge window is around. Can you please
share your opinion for this now?

--
viresh

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

[Viresh Kumar]

On 19 October 2012 19:03, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 4 October 2012 16:34, 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 Wolfram,
>
> I haven't pinged you earlier as i knew merge window is around. Can you please
> share your opinion for this now?

Ping!!

--
viresh

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 11715 bytes --]

On Thu, Oct 04, 2012 at 04:34:53PM +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>

As mentioned before, I still have issues with the API and have most
comments to that for now.

> ---
> V5->V6:
> - Removed sda_gpio_flags
> - Make scl_gpio_flags as GPIOF_OPEN_DRAIN | GPIOF_OUT_INIT_HIGH by default
> - update bri->set_scl and bri->get_sda for gpio recovery case in i2c core
> - Guaranteed to generate 9 falling-rising edges for bus recovery
> 
> 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 | 156 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    |  55 +++++++++++++++++
>  2 files changed, 211 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index a7edf98..e78033b 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,111 @@ 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 void set_scl_gpio_value(struct i2c_adapter *adap, int val)
> +{
> +	gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
> +}
> +
> +static int get_sda_gpio_value(struct i2c_adapter *adap)
> +{
> +	return gpio_get_value(adap->bus_recovery_info->sda_gpio);
> +}
> +
> +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);

This warning is probably not very helpful to a user.

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

Since the unlikely() are not in hot-paths, you probably better skip
them.

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

What is this magic value?

> +	int i, ret, val = 0;
> +
> +	if (bri->is_gpio_recovery) {
> +		ret = i2c_get_gpios_for_recovery(adap);
> +		if (ret)
> +			return ret;
> +	} else {
> +		bri->set_scl(adap, 1);
> +	}
> +
> +	/*
> +	 * By this time SCL is high, as we need to give 9 falling-rising edges
> +	 */
> +
> +	delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);
> +
> +	for (i = 0; i < bri->clock_cnt * 2; i++, val = !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(bri->get_sda(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 +1003,55 @@ 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) {
> +				if (!bri->scl_gpio_flags)
> +					bri->scl_gpio_flags = GPIOF_OPEN_DRAIN;
> +
> +				/* We always start by making GPIO HIGH */
> +				bri->scl_gpio_flags |= 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) {
> +				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..13eeb2e 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -370,6 +370,58 @@ 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.

What about having those options?

NULL or custom_pointer or i2c_generic_scl_recovery or i2c_generic_gpio_recovery

where i2c_generic_gpio_recovery is probably:

	get_gpios_for_recovery
	i2c_generic_scl_recovery
	put_gpios_for_recovery

and i2c_generic_scl_recovery is basically your current
i2c_generic_recovery. That makes it easier to add other generic routines
if that should ever become necessary.


> + * @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.

Does a user really need to set this?

> + * @is_gpio_recovery: true, select gpio type else scl type. Only required if
> + *	recover_bus == NULL.

This could be dropped in favor of i2c_generic_*_recovery in recover_bus

> + * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
> + *	scl type recovery.

Does a user really need this? We could probably use something close to
100kHz always?

> + * @clock_cnt: count of max clocks to be generated. Required for both gpio and
> + *	scl type recovery.

Don't think this should be something else than 9. If so, it should be
increased generally in the core and not inside some platform data.

> + * @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

Basically OK, documentation should be more user-centric not
implementation centric :)

> + * @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.

I wonder if it makes sense to have those more generic like
prepare_recovery and unprepare_recovery?

> + * @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.

OK.

> + * @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. Otherwise, it
> + *      is ORRED with GPIOF_OUT_INIT_HIGH.

Is this needed? I'd say we drop it until somebody with a need can add
something like this.

> + *      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 *);
> +	bool skip_sda_polling;
> +	bool is_gpio_recovery;
> +	u32 clock_rate_khz;
> +	u8 clock_cnt;
> +
> +	/*
> +	 * 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 *);
> +
> +	/* gpio recovery */
> +	int (*get_gpio)(unsigned gpio);
> +	void (*put_gpio)(unsigned gpio);
> +	unsigned scl_gpio;
> +	unsigned sda_gpio;
> +	unsigned long scl_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 +445,9 @@ struct i2c_adapter {
>  
>  	struct mutex userspace_clients_lock;
>  	struct list_head userspace_clients;
> +
> +	/* Pass valid pointer if recovery infrastructure is required */

This comment can be left out.

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

See also the next mail...


-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Viresh Kumar]

Hi Wolfram,

Thanks for sharing your opinion.

On 25 November 2012 02:29, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Thu, Oct 04, 2012 at 04:34:53PM +0530, Viresh Kumar wrote:

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

>> +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);
>
> This warning is probably not very helpful to a user.

This is what i thought about it: This is a platform specific routine and
most probably it will fail the first time this code is ever hit and so a
warning would be very helpful for them.

But, now i think platforms should have these prints in their get_gpio
implementations. And we can make it have return type void.

>> +                     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 ||
>
> Since the unlikely() are not in hot-paths, you probably better skip
> them.

It will be removed as get_gpio() would have return type void.

>> +                     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);
>> +                     bri->skip_sda_polling = true;
>> +                     if (!ret && bri->put_gpio)
>> +                             bri->put_gpio(bri->sda_gpio);
>> +
>> +                     ret = 0;
>> +             }
>> +     }
>> +static int i2c_recover_bus(struct i2c_adapter *adap)
>> +{
>> +     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +     unsigned long delay = 1000000;
>
> What is this magic value?

10 ^ 6 For time conversion to ns. clock duration is given by:
1/ (1000 * KHz) sec = 10^6 /(10^6 * 1000 * KHz) sec = 10^6/KHz nsec

>> +/**
>> + * 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.
>
> What about having those options?
>
> NULL or custom_pointer or i2c_generic_scl_recovery or i2c_generic_gpio_recovery

Looks good. But NULL should be an error then ?

> where i2c_generic_gpio_recovery is probably:
>
>         get_gpios_for_recovery
>         i2c_generic_scl_recovery
>         put_gpios_for_recovery

Ok.

> and i2c_generic_scl_recovery is basically your current
> i2c_generic_recovery. That makes it easier to add other generic routines
> if that should ever become necessary.

yes, that a good point.

>> + * @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.
>
> Does a user really need to set this?

This was done for platforms and i2c controllers which don't have configuration
to control scl line. So i still feel we need it.

>> + * @is_gpio_recovery: true, select gpio type else scl type. Only required if
>> + *   recover_bus == NULL.
>
> This could be dropped in favor of i2c_generic_*_recovery in recover_bus

Yes.

>> + * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
>> + *   scl type recovery.
>
> Does a user really need this? We could probably use something close to
> 100kHz always?

Not sure. Doesn't it depend on the current clk rate of controller ?
If not then yes it can be 100 KHz

>> + * @clock_cnt: count of max clocks to be generated. Required for both gpio and
>> + *   scl type recovery.
>
> Don't think this should be something else than 9. If so, it should be
> increased generally in the core and not inside some platform data.

I don't remember well, but somebody pointed it out in earlier version as
they require more of these. But will drop it for now and make it 9.

>> + * @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
>
> Basically OK, documentation should be more user-centric not
> implementation centric :)

Yes, i realize it now.

>> + * @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.
>
> I wonder if it makes sense to have those more generic like
> prepare_recovery and unprepare_recovery?

Yes.

>> + * @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. Otherwise, it
>> + *      is ORRED with GPIOF_OUT_INIT_HIGH.
>
> Is this needed? I'd say we drop it until somebody with a need can add
> something like this.

Ok.

>> +     /* Pass valid pointer if recovery infrastructure is required */
>
> This comment can be left out.

Ok.

I will float another version as soon as we close open points here. I really
want to get this in 3.8 :)

--
viresh

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2764 bytes --]

On Sun, Nov 25, 2012 at 09:34:46AM +0530, Viresh Kumar wrote:
> Hi Wolfram,
> 
> Thanks for sharing your opinion.
> 
> On 25 November 2012 02:29, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > On Thu, Oct 04, 2012 at 04:34:53PM +0530, Viresh Kumar wrote:
> 
> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> 
> >> +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);
> >
> > This warning is probably not very helpful to a user.
> 
> This is what i thought about it: This is a platform specific routine and
> most probably it will fail the first time this code is ever hit and so a
> warning would be very helpful for them.
> 
> But, now i think platforms should have these prints in their get_gpio
> implementations. And we can make it have return type void.

The warning could be made helpful if properly rephrased. Actually, I
think returning an err is OK here, but if there is a warning it should
be a proper human readable sentence.

> >> +             if (unlikely(ret ||
> >
> > Since the unlikely() are not in hot-paths, you probably better skip
> > them.
> 
> It will be removed as get_gpio() would have return type void.

Keep in mind that it was not the only one. I think V7 had still one
left.

> >> + * @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.
> >
> > Does a user really need to set this?
> 
> This was done for platforms and i2c controllers which don't have configuration
> to control scl line. So i still feel we need it.

It might be needed internally, still not sure if a user needs to set it.
If so, there should be more documentation when she wants to use this.

> >> + * @clock_rate_khz: clock rate of dummy clock in khz. Required for both gpio and
> >> + *   scl type recovery.
> >
> > Does a user really need this? We could probably use something close to
> > 100kHz always?
> 
> Not sure. Doesn't it depend on the current clk rate of controller ?
> If not then yes it can be 100 KHz

It should be OK to drive the bus at x kHz and recover from it at y kHz
(as long as y < 100).

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Viresh Kumar]

Hi Wolfram/Paul,

I have tried to fix my patches as per review comments from you.
Following is diff of the new changes i have made, I am sending a
new complete version separately:

commit 62cf40359c63ac028b3e8a6395f9440f6a3b0162
Author: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date:   Mon Dec 3 08:12:55 2012 +0530

    fixup! i2c/adapter: Add bus recovery infrastructure
---
 drivers/i2c/i2c-core.c | 42 +++++++++++++++++++++++++++---------------
 include/linux/i2c.h    | 19 +++++++++++--------
 2 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d100276..9b6a1a6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -109,15 +109,22 @@ static int i2c_device_uevent(struct device *dev,
struct kobj_uevent_env *env)
 /* i2c bus recovery routines */
 #define RECOVERY_CLK_CNT       9
 #define DEFAULT_CLK_RATE       100     /* KHz */
+/* 10^6/KHz for delay in ns */
+unsigned long clk_delay = DIV_ROUND_UP(1000000, DEFAULT_CLK_RATE * 2);

-static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
+static int get_sda_gpio_value(struct i2c_adapter *adap)
 {
-       gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
+       return gpio_get_value(adap->bus_recovery_info->sda_gpio);
 }

-static int get_sda_gpio_value(struct i2c_adapter *adap)
+static int get_scl_gpio_value(struct i2c_adapter *adap)
 {
-       return gpio_get_value(adap->bus_recovery_info->sda_gpio);
+       return gpio_get_value(adap->bus_recovery_info->scl_gpio);
+}
+
+static void set_scl_gpio_value(struct i2c_adapter *adap, int val)
+{
+       gpio_set_value(adap->bus_recovery_info->scl_gpio, val);
 }

 static int i2c_get_gpios_for_recovery(struct i2c_adapter *adap)
@@ -158,28 +165,37 @@ static void i2c_put_gpios_for_recovery(struct
i2c_adapter *adap)
 static int i2c_generic_recovery(struct i2c_adapter *adap)
 {
        struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
-       unsigned long delay = 1000000; /* 10^6/KHz for delay in ns */
        int i, val = 0;

        if (bri->prepare_recovery)
                bri->prepare_recovery(bri);

+       /* SCL shouldn't be low here */
+       if (!bri->get_scl(adap)) {
+               dev_err(&adap->dev, "SCL is stuck Low, exiting recovery.\n");
+               goto unprepare;
+       }
+
        /*
         * By this time SCL is high, as we need to give 9 falling-rising edges
         */
-
-       delay = DIV_ROUND_UP(delay, bri->clock_rate_khz * 2);
-
        for (i = 0; i < RECOVERY_CLK_CNT * 2; i++, val = !val) {
                bri->set_scl(adap, val);
-               ndelay(delay);
+               ndelay(clk_delay);

                /* break if sda got high, check only when scl line is high */
                if (!bri->skip_sda_polling && val)
-                       if (unlikely(bri->get_sda(adap)))
+                       /* Check SCL again to see fault condition */
+                       if (!bri->get_scl(adap)) {
+                               dev_err(&adap->dev, "SCL is stuck Low
during recovery, exiting recovery.\n");
+                               goto unprepare;
+                       }
+
+                       if (bri->get_sda(adap))
                                break;
        }

+unprepare:
        if (bri->unprepare_recovery)
                bri->unprepare_recovery(bri);

@@ -1008,13 +1024,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
                        goto exit_recovery;
                }

-               if (!bri->clock_rate_khz) {
-                       dev_warn(&adap->dev, "default clk rate used: 100KHz\n");
-                       bri->clock_rate_khz = DEFAULT_CLK_RATE;
-               }
-
                /* GPIO recovery */
                if (bri->recover_bus == i2c_generic_gpio_recovery) {
+                       bri->get_scl = get_scl_gpio_value;
                        bri->set_scl = set_scl_gpio_value;

                        if (!bri->skip_sda_polling)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 63acd9d..165282f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -373,14 +373,17 @@ struct i2c_algorithm {
  * @recover_bus: Recover routine. Either pass driver's recover_bus()
routine, or
  *     i2c_generic_scl_recovery() or i2c_generic_gpio_recovery().
  * @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.
- * @clock_rate_khz: clock rate of dummy clock in khz. Required for
both gpio and
- *     scl type recovery.
- * @set_scl: This sets/clears scl line. For GPIO recovery set_scl_gpio_value()
- *     is used here otherwise controller specific routine must be passed.
+ *     it became high or not. Platforms/controllers which don't have
+ *     configuration registers to control sda line must set this. Only required
+ *     if recover_bus == NULL.
  * @get_sda: This gets current value of sda line. For GPIO recovery
  *     get_sda_gpio_value() is used here otherwise controller specific routine
  *     must be passed.
+ * @get_scl: This gets current value of scl line. For GPIO recovery
+ *     get_scl_gpio_value() is used here otherwise controller specific routine
+ *     must be passed.
+ * @set_scl: This sets/clears scl line. For GPIO recovery set_scl_gpio_value()
+ *     is used here otherwise controller specific routine must be passed.
  * @prepare_recovery: This will be called before starting recovery.
Platform may
  *     configure padmux here for sda/scl line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform
@@ -391,15 +394,15 @@ struct i2c_algorithm {
 struct i2c_bus_recovery_info {
        int (*recover_bus)(struct i2c_adapter *);
        bool skip_sda_polling;
-       u32 clock_rate_khz;

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

        void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
        void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);