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

[PATCH V4 Resend 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.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.

Signed-off-by: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h    |  52 ++++++++++++++++++++
 2 files changed, 177 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 96ef3d8..7ede75d 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,88 @@ 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 int i2c_gpio_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->get_gpio)
+		bri->get_gpio(bri->scl_gpio);
+
+	ret = gpio_request_one(bri->scl_gpio, bri->scl_gpio_flags, "i2c-scl");
+	if (ret < 0) {
+		dev_warn(&adap->dev, "gpio request fail: %d\n", bri->scl_gpio);
+		return ret;
+	}
+
+	if (!bri->skip_sda_polling) {
+		if (bri->get_gpio)
+			bri->get_gpio(bri->sda_gpio);
+
+		ret = gpio_request_one(bri->sda_gpio, bri->sda_gpio_flags,
+				"i2c-sda");
+		if (ret < 0) {
+			/* work without sda polling */
+			dev_warn(&adap->dev,
+				"sda_gpio request fail: %d. Skip sda polling\n",
+				bri->scl_gpio);
+			bri->skip_sda_polling = true;
+			if (bri->put_gpio)
+				bri->put_gpio(bri->sda_gpio);
+		}
+	}
+
+	delay /= bri->clock_rate_khz * 2;
+
+	for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
+		ndelay(delay);
+		gpio_set_value(bri->scl_gpio, val);
+
+		/* break if sda got high, check only when scl line is high */
+		if (!bri->skip_sda_polling && val)
+			if (gpio_get_value(bri->sda_gpio))
+				break;
+	}
+
+	gpio_free(bri->scl_gpio);
+
+	if (bri->put_gpio)
+		bri->put_gpio(bri->scl_gpio);
+
+	if (!bri->skip_sda_polling) {
+		gpio_free(bri->sda_gpio);
+
+		if (bri->put_gpio)
+			bri->put_gpio(bri->sda_gpio);
+	}
+
+	return 0;
+}
+
+static int i2c_scl_recover_bus(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	int i, val = 0;
+	unsigned long delay = 1000000;
+
+	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 (bri->get_sda(adap))
+				break;
+	}
+
+	return 0;
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -879,6 +963,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;
+
+			if (bri->is_gpio_recovery) {
+				bri->recover_bus = i2c_gpio_recover_bus;
+				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;
+				}
+
+				bri->recover_bus = i2c_scl_recover_bus;
+				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..9e0a14a 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.
+ * @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);
+	int (*put_gpio)(unsigned gpio);
+	u32 scl_gpio;
+	u32 sda_gpio;
+	u32 scl_gpio_flags;
+	u32 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 V4 Resend 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 | 24 ++++++++++++++
 include/linux/i2c/i2c-designware.h          | 49 +++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 1 deletion(-)
 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 1e48bec..f29f283 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -536,7 +536,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..3a7a4e8 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,11 @@ static struct i2c_algorithm i2c_dw_algo = {
 	.master_xfer	= i2c_dw_xfer,
 	.functionality	= i2c_dw_func,
 };
+
+static struct i2c_bus_recovery_info dw_recovery_info = {
+	.is_gpio_recovery = true,
+};
+
 static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 {
 	return clk_get_rate(dev->clk)/1000;
@@ -55,6 +61,7 @@ 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;
 	int irq, r;
 
 	/* NOTE: driver uses the static register mapping */
@@ -141,6 +148,23 @@ 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) {
+		dw_recovery_info.get_gpio = pdata->get_gpio;
+		dw_recovery_info.put_gpio = pdata->put_gpio;
+		dw_recovery_info.scl_gpio = pdata->scl_gpio;
+		dw_recovery_info.scl_gpio_flags = pdata->scl_gpio_flags;
+		dw_recovery_info.clock_rate_khz = clk_get_rate(dev->clk) / 1000;
+
+		if (!pdata->skip_sda_polling) {
+			dw_recovery_info.sda_gpio = pdata->sda_gpio;
+			dw_recovery_info.sda_gpio_flags = pdata->sda_gpio_flags;
+		}
+
+		adap->bus_recovery_info = &dw_recovery_info;
+	}
+
 	adap->nr = pdev->id;
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
diff --git a/include/linux/i2c/i2c-designware.h b/include/linux/i2c/i2c-designware.h
new file mode 100644
index 0000000..341bd4c
--- /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);
+	int (*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 V4 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Uwe Kleine-König]

On Mon, Aug 27, 2012 at 10:41:26AM +0530, Viresh Kumar wrote:
> 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.16 titled "Bus clear".
It's 3.1.16, not 3.16.

> 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.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    |  52 ++++++++++++++++++++
>  2 files changed, 177 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 96ef3d8..7ede75d 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,88 @@ 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 int i2c_gpio_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->get_gpio)
> +		bri->get_gpio(bri->scl_gpio);
get_gpio returns an int. That should be checked.

> +	ret = gpio_request_one(bri->scl_gpio, bri->scl_gpio_flags, "i2c-scl");
> +	if (ret < 0) {
> +		dev_warn(&adap->dev, "gpio request fail: %d\n", bri->scl_gpio);
> +		return ret;
> +	}
> +
> +	if (!bri->skip_sda_polling) {
> +		if (bri->get_gpio)
> +			bri->get_gpio(bri->sda_gpio);
ditto.

> +
> +		ret = gpio_request_one(bri->sda_gpio, bri->sda_gpio_flags,
> +				"i2c-sda");
> +		if (ret < 0) {
> +			/* work without sda polling */
> +			dev_warn(&adap->dev,
> +				"sda_gpio request fail: %d. Skip sda polling\n",
> +				bri->scl_gpio);
> +			bri->skip_sda_polling = true;
> +			if (bri->put_gpio)
> +				bri->put_gpio(bri->sda_gpio);
> +		}
> +	}
> +
> +	delay /= bri->clock_rate_khz * 2;
For maximal correctness you should use DIV_ROUND_UP here.

> +
> +	for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
When do you want clock_cnt != 9?

> +		ndelay(delay);
> +		gpio_set_value(bri->scl_gpio, val);
> +
> +		/* break if sda got high, check only when scl line is high */
> +		if (!bri->skip_sda_polling && val)
> +			if (gpio_get_value(bri->sda_gpio))
> +				break;
I'm not sure it's worth to conditionally shortcut here. Either always or
never shortcut.

In a document by Atmel describing on of their at24 eeproms they suggest
to send an start and stop after the nine clocks. Yesterday I considered
that sensible but today I don't remember why.

Also it might be worth pointing out that this procedure isn't
multi-master save because it does neither clock synchronization (3.1.7)
nor arbitration (3.1.8). I'm not sure how relevant multi-master is, but
if you don't consider it maybe still make it explicit.

> +	}
> +
> +	gpio_free(bri->scl_gpio);
> +
> +	if (bri->put_gpio)
> +		bri->put_gpio(bri->scl_gpio);
> +
> +	if (!bri->skip_sda_polling) {
> +		gpio_free(bri->sda_gpio);
> +
> +		if (bri->put_gpio)
> +			bri->put_gpio(bri->sda_gpio);
> +	}
> +
> +	return 0;
> +}
> +
> +static int i2c_scl_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +	int i, val = 0;
> +	unsigned long delay = 1000000;
> +
> +	delay /= bri->clock_rate_khz * 2;
> +
> +	for (i = 0; i < bri->clock_cnt * 2; i++,
> +			val = !val) {
> +		bri->set_scl(adap, val);
again for multi-master set_scl can fail and so if this scenario is
considered worth to be supported it should return an error indication
that needs handling.

> +		ndelay(delay);
> +
> +		/* break if sda got high, check only when scl line is high */
> +		if (!bri->skip_sda_polling && val)
> +			if (bri->get_sda(adap))
> +				break;
> +	}
> +
> +	return 0;
> +}
i2c_scl_recover_bus and i2c_gpio_recover_bus have a common pattern. Both
bitbang SCL and SDA. Is it worth to factor out the common stuff?

> +
>  static int i2c_device_probe(struct device *dev)
>  {
>  	struct i2c_client	*client = i2c_verify_client(dev);
> @@ -879,6 +963,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;
> +
> +			if (bri->is_gpio_recovery) {
> +				bri->recover_bus = i2c_gpio_recover_bus;
> +				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;
> +				}
> +
> +				bri->recover_bus = i2c_scl_recover_bus;
> +				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..9e0a14a 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.
This is used for both, scl and sda. I don't know if there is a more
modern approach available using the pinmux framework? (Added Linus
Walleij to Cc.)

> + * @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.
IMHO you should not make this configurable but use

	GPIOF_OUT_INIT_HIGH | GPIOF_OPEN_DRAIN

> + * @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);
> +	int (*put_gpio)(unsigned gpio);
> +	u32 scl_gpio;
> +	u32 sda_gpio;
> +	u32 scl_gpio_flags;
> +	u32 sda_gpio_flags;
These are not the right types. gpio_request_one expects gpio to be
unsigned and flags to be unsigned long.

As scl/sda recovery and gpio recovery are mutually exclusive you could
use a union here. But maybe it's not worth the added complexity?

> +};
> +
>  /*
>   * 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)

Best regards
Uwe

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

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

[viresh kumar]

Finally somebody had a look at this poor fellow :)

On Thu, Sep 27, 2012 at 7:58 PM, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Mon, Aug 27, 2012 at 10:41:26AM +0530, Viresh Kumar wrote:
>> 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.16 titled "Bus clear".
> It's 3.1.16, not 3.16.

Thanks for pointing out.

>> 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.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
>> ---
>>  drivers/i2c/i2c-core.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/i2c.h    |  52 ++++++++++++++++++++
>>  2 files changed, 177 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 96ef3d8..7ede75d 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,88 @@ 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 int i2c_gpio_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->get_gpio)
>> +             bri->get_gpio(bri->scl_gpio);
> get_gpio returns an int. That should be checked.

Yes.

>> +
>> +             ret = gpio_request_one(bri->sda_gpio, bri->sda_gpio_flags,
>> +                             "i2c-sda");
>> +             if (ret < 0) {
>> +                     /* work without sda polling */
>> +                     dev_warn(&adap->dev,
>> +                             "sda_gpio request fail: %d. Skip sda polling\n",
>> +                             bri->scl_gpio);
>> +                     bri->skip_sda_polling = true;
>> +                     if (bri->put_gpio)
>> +                             bri->put_gpio(bri->sda_gpio);
>> +             }
>> +     }
>> +
>> +     delay /= bri->clock_rate_khz * 2;
> For maximal correctness you should use DIV_ROUND_UP here.

Good point.

>> +     for (i = 0; i < bri->clock_cnt * 2; i++, val = !val) {
> When do you want clock_cnt != 9?

I don't remember well, but this is result of discussions on the
earlier patchsets.
So, its just an option that somebody may use.

>> +             ndelay(delay);
>> +             gpio_set_value(bri->scl_gpio, val);
>> +
>> +             /* break if sda got high, check only when scl line is high */
>> +             if (!bri->skip_sda_polling && val)
>> +                     if (gpio_get_value(bri->sda_gpio))
>> +                             break;
> I'm not sure it's worth to conditionally shortcut here. Either always or
> never shortcut.

This was done, because few platforms may not have configuration bits to read
status of sda line.. For them skip_sda_polling was required.

Whereas, others would need this to see if we can return early.

> In a document by Atmel describing on of their at24 eeproms they suggest
> to send an start and stop after the nine clocks. Yesterday I considered
> that sensible but today I don't remember why.
>
> Also it might be worth pointing out that this procedure isn't
> multi-master save because it does neither clock synchronization (3.1.7)
> nor arbitration (3.1.8). I'm not sure how relevant multi-master is, but
> if you don't consider it maybe still make it explicit.

Not considering it for this basic version, will mention that.

>> +     }
>> +
>> +     gpio_free(bri->scl_gpio);
>> +
>> +     if (bri->put_gpio)
>> +             bri->put_gpio(bri->scl_gpio);
>> +
>> +     if (!bri->skip_sda_polling) {
>> +             gpio_free(bri->sda_gpio);
>> +
>> +             if (bri->put_gpio)
>> +                     bri->put_gpio(bri->sda_gpio);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int i2c_scl_recover_bus(struct i2c_adapter *adap)
>> +{
>> +     struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +     int i, val = 0;
>> +     unsigned long delay = 1000000;
>> +
>> +     delay /= bri->clock_rate_khz * 2;
>> +
>> +     for (i = 0; i < bri->clock_cnt * 2; i++,
>> +                     val = !val) {
>> +             bri->set_scl(adap, val);
> again for multi-master set_scl can fail and so if this scenario is
> considered worth to be supported it should return an error indication
> that needs handling.

ditto.

>> +             ndelay(delay);
>> +
>> +             /* break if sda got high, check only when scl line is high */
>> +             if (!bri->skip_sda_polling && val)
>> +                     if (bri->get_sda(adap))
>> +                             break;
>> +     }
>> +
>> +     return 0;
>> +}
> i2c_scl_recover_bus and i2c_gpio_recover_bus have a common pattern. Both
> bitbang SCL and SDA. Is it worth to factor out the common stuff?

Will surely have a look at it again, before sending another version.

>>  static int i2c_device_probe(struct device *dev)
>>  {
>>       struct i2c_client       *client = i2c_verify_client(dev);
>> @@ -879,6 +963,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;
>> +
>> +                     if (bri->is_gpio_recovery) {
>> +                             bri->recover_bus = i2c_gpio_recover_bus;
>> +                             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;
>> +                             }
>> +
>> +                             bri->recover_bus = i2c_scl_recover_bus;
>> +                             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..9e0a14a 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.
> This is used for both, scl and sda. I don't know if there is a more
> modern approach available using the pinmux framework? (Added Linus
> Walleij to Cc.)
>
>> + * @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.
> IMHO you should not make this configurable but use
>
>         GPIOF_OUT_INIT_HIGH | GPIOF_OPEN_DRAIN

Discussed here:
http://www.spinics.net/lists/linux-i2c/msg07325.html

>> + * @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);
>> +     int (*put_gpio)(unsigned gpio);
>> +     u32 scl_gpio;
>> +     u32 sda_gpio;
>> +     u32 scl_gpio_flags;
>> +     u32 sda_gpio_flags;
> These are not the right types. gpio_request_one expects gpio to be
> unsigned and flags to be unsigned long.

Will fix it.

> As scl/sda recovery and gpio recovery are mutually exclusive you could
> use a union here. But maybe it's not worth the added complexity?

Hmm.

Thanks for your valuable comments Uwe.

But before i send another version of this patchset, need some inputs from
Wolfram.

@Wolfram and other maintainers: I understand that you have been very
busy all the time, but this patchset is worth having some feedback. Isn't it?

Please share your comments about it, whatever they are...

Many platforms want to use this infrastructure but they are just not
able to do it,
because it hasn't got your acceptance till now.

--
viresh

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

[Wolfram Sang]

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

Hi,

> But before i send another version of this patchset, need some inputs from
> Wolfram.

I am trying to have another I2C weekend this weekend. And your patches
have been scheduled for that. The generic feeling is:

Very useful but the interface could probably be simplified. This is why
it takes me so long, working on interfaces needs more thought than the
average patch review :/

Thanks,

   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 V4 Resend 1/2] i2c/adapter: Add bus recovery infrastructure

[Viresh Kumar]

On 28 September 2012 12:30, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> I am trying to have another I2C weekend this weekend. And your patches
> have been scheduled for that. The generic feeling is:
>
> Very useful but the interface could probably be simplified. This is why
> it takes me so long, working on interfaces needs more thought than the
> average patch review :/

Glad to hear that. Even i would like to make it better :)

Waiting for your comments.

--
Viresh

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

[Uwe Kleine-König]

Hello,

On Fri, Sep 28, 2012 at 09:11:34AM +0530, viresh kumar wrote:
> >> +             ndelay(delay);
> >> +             gpio_set_value(bri->scl_gpio, val);
> >> +
> >> +             /* break if sda got high, check only when scl line is high */
> >> +             if (!bri->skip_sda_polling && val)
> >> +                     if (gpio_get_value(bri->sda_gpio))
> >> +                             break;
> > I'm not sure it's worth to conditionally shortcut here. Either always or
> > never shortcut.
> 
> This was done, because few platforms may not have configuration bits to read
> status of sda line.. For them skip_sda_polling was required.
> 
> Whereas, others would need this to see if we can return early.
What is the upside of returning early? I'd say, just don't do it.

> >> + * @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.
> > IMHO you should not make this configurable but use
> >
> >         GPIOF_OUT_INIT_HIGH | GPIOF_OPEN_DRAIN
> 
> Discussed here:
> http://www.spinics.net/lists/linux-i2c/msg07325.html
Why do you default to GPIOF_OUT_INIT_LOW? The idle state of scl is high.
Using low here introduces an additional clk pulse.

Best regards
Uwe

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

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

[Viresh Kumar]

Hi Uwe,

On 28 September 2012 12:57, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> On Fri, Sep 28, 2012 at 09:11:34AM +0530, viresh kumar wrote:

>> This was done, because few platforms may not have configuration bits to read
>> status of sda line.. For them skip_sda_polling was required.
>>
>> Whereas, others would need this to see if we can return early.

> What is the upside of returning early? I'd say, just don't do it.

Save time. Why give additional clocks when you don't actually need them?
Can use likely/unlikely to make it more efficient for 9 clock pulse
scenario though.

>> >> + * @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.
>> > IMHO you should not make this configurable but use
>> >
>> >         GPIOF_OUT_INIT_HIGH | GPIOF_OPEN_DRAIN
>>
>> Discussed here:
>> http://www.spinics.net/lists/linux-i2c/msg07325.html

> Why do you default to GPIOF_OUT_INIT_LOW? The idle state of scl is high.
> Using low here introduces an additional clk pulse.

We aren't giving any clock pulses to scl line. Are you talking about sda?

--
viresh

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

[Viresh Kumar]

On 28 September 2012 13:06, Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> Why do you default to GPIOF_OUT_INIT_LOW? The idle state of scl is high.
>> Using low here introduces an additional clk pulse.
>
> We aren't giving any clock pulses to scl line. Are you talking about sda?

My fault... we are sending clocks on scl only... Will take care to
optimize it in
next version. If decide to keep it as is, will inform you.

--
viresh

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

[Viresh Kumar]

On 28 September 2012 12:57, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Why do you default to GPIOF_OUT_INIT_LOW? The idle state of scl is high.
> Using low here introduces an additional clk pulse.

Went deep into the code i wrote ages ago to check why i did so :)

My initial idea was, because the idle state of scl is high, giving another high
initially  with gpio would be a waste. As the slave will not notice a change.

So, i will do it low and then following code will run with delay before setting
gpio again. This will ensure, the initial gpio-set is used as clock.

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

                ...
	}

Yes, you are correct in saying that i have generated 9 *2 + 1 = 19
half-clocks...
or 9.5 clocks. Will fix it by making initial value of i as 1.

Also, will take care of this when user sends his own flags :)

--
viresh

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

[Viresh Kumar]

On 28 September 2012 12:30, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> I am trying to have another I2C weekend this weekend. And your patches
> have been scheduled for that. The generic feeling is:
>
> Very useful but the interface could probably be simplified. This is why
> it takes me so long, working on interfaces needs more thought than the
> average patch review :/

Hi,

Did this poor fellow got some attention this weekend? :)

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

--
viresh