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

[Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

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

On Thu, Jan 24, 2013 at 04:06:37PM +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.
> 
> Reviewed-by: Paul Carpenter <paul-YHLC2tV1sDlxR4N9A70vTlRxknfHcPLb9dF7HbQ/qKg@public.gmane.org>
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> Most number of versions for any patchset i submitted :)
> 
> V8->V9:
> - removed skip_sda_polling variable
> - simplified/fixed i2c_generic_recovery() routine
> - added new routine i2c_recover_bus()
> - added gpio_is_valid() checks over scl and sda gpios
> - code is rearranged at places (based on Wolfram's comments)
> 
>  drivers/i2c/i2c-core.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    |  39 +++++++++++++
>  2 files changed, 194 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index e388590..8076e52 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>
> @@ -109,6 +111,127 @@ 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 */
> +#define RECOVERY_CLK_CNT	9
> +#define DEFAULT_CLK_RATE	100	/* KHz */

RECOVERY_CLK_RATE

> +/* 10^6/KHz for delay in ns */
> +unsigned long clk_delay = DIV_ROUND_UP(1000000, DEFAULT_CLK_RATE * 2);

Since all inputs are constant values, the result will be a constant as
well. We might even get rid of this variable altogether.

> +
> +static int get_scl_gpio_value(struct i2c_adapter *adap)
> +{
> +	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 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;
> +
> +	ret = gpio_request_one(bri->scl_gpio, GPIOF_OPEN_DRAIN |
> +			GPIOF_OUT_INIT_HIGH, "i2c-scl");
> +	if (ret) {
> +		dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
> +		return ret;
> +	}
> +
> +	if (bri->get_sda) {
> +		if (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->get_sda = NULL;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void i2c_put_gpios_for_recovery(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +
> +	if (bri->get_sda)
> +		gpio_free(bri->sda_gpio);
> +
> +	gpio_free(bri->scl_gpio);
> +}
> +
> +static int i2c_generic_recovery(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +	int i = 0, val = 1, ret = 0;
> +
> +	if (bri->prepare_recovery)
> +		bri->prepare_recovery(bri);
> +
> +	/*
> +	 * By this time SCL is high, as we need to give 9 falling-rising edges
> +	 */
> +	while (i++ < RECOVERY_CLK_CNT * 2) {
> +		/* SCL shouldn't be low here */
> +		if (val && !bri->get_scl(adap)) {
> +			dev_err(&adap->dev, "SCL is stuck Low exit recovery\n");

s/Low/low/

> +			ret = -EBUSY;
> +			goto unprepare;
> +		}
> +
> +		val = !val;
> +		bri->set_scl(adap, val);
> +		ndelay(clk_delay);
> +
> +		/* break if sda got high, check only when scl line is high */
> +		if (bri->get_sda && val)
> +			if (bri->get_sda(adap))
> +				break;

if (val && bri->get_sda && bri->get_sda(adap)
	break;

Didn't we agree to move the SDA check before the SCL setting?

> +	}
> +
> +unprepare:
> +	if (bri->unprepare_recovery)
> +		bri->unprepare_recovery(bri);
> +
> +	return ret;
> +}
> +
> +int i2c_generic_scl_recovery(struct i2c_adapter *adap)
> +{
> +	adap->bus_recovery_info->set_scl(adap, 1);
> +	return i2c_generic_recovery(adap);
> +}
> +
> +int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
> +{
> +	int ret;
> +
> +	ret = i2c_get_gpios_for_recovery(adap);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_generic_recovery(adap);
> +	i2c_put_gpios_for_recovery(adap);
> +
> +	return ret;
> +}
> +
> +int i2c_recover_bus(struct i2c_adapter *adap)
> +{
> +	if (!adap->bus_recovery_info)
> +		return -EOPNOTSUPP;
> +
> +	dev_dbg(&adap->dev, "try i2c bus recovery\n");

s/try/trying/

> +	return adap->bus_recovery_info->recover_bus(adap);
> +}
> +
>  static int i2c_device_probe(struct device *dev)
>  {
>  	struct i2c_client	*client = i2c_verify_client(dev);
> @@ -902,6 +1025,38 @@ 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_err(&adap->dev, "Invalid recover_bus(), skip recovery\n");

s/Invalid/No/
s/skip/not using/
The latter for all later occurences, too.

> +			adap->bus_recovery_info = NULL;
> +			goto exit_recovery;
> +		}
> +
> +		/* GPIO recovery */
> +		if (bri->recover_bus == i2c_generic_gpio_recovery) {
> +			if (!gpio_is_valid(bri->scl_gpio)) {
> +				dev_err(&adap->dev, "Invalid scl gpio, skip recovery\n");
> +				adap->bus_recovery_info = NULL;
> +				goto exit_recovery;
> +			}
> +
> +			if (gpio_is_valid(bri->sda_gpio))
> +				bri->get_sda = get_sda_gpio_value;
> +			else
> +				bri->get_sda = NULL;
> +
> +			bri->get_scl = get_scl_gpio_value;
> +			bri->set_scl = set_scl_gpio_value;
> +		} else if (!bri->set_scl) {
> +			dev_warn(&adap->dev,
> +					"set_scl() is must for scl recovery\n");

What about get_scl. You don't check here, but you don't check when you
use it above.

> +		}
> +	}
> +
> +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 d0c4db7..da9358b 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -370,6 +370,43 @@ 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
> + *	i2c_generic_scl_recovery() or i2c_generic_gpio_recovery().
> + * @get_scl: This gets current value of scl line. Mandatory for generic SCL
> + *      recovery. Used internally for generic GPIO recovery.
> + * @set_scl: This sets/clears scl line. Mandatory for generic SCL recovery. Used
> + *      internally for generic GPIO recovery.
> + * @get_sda: This gets current value of sda line. Optional for generic SCL
> + *      recovery. Used internally, if sda_gpio != -1, for generic GPIO recovery.

s/!= -1/is a valid GPIO/

> + * @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
> + *	may configure padmux here for sda/scl line or something else they want.
> + * @scl_gpio: gpio number of the scl line. Only required for GPIO recovery.
> + * @sda_gpio: gpio number of the sda line. Only required for GPIO recovery.
> + */
> +struct i2c_bus_recovery_info {
> +	int (*recover_bus)(struct i2c_adapter *);
> +
> +	int (*get_scl)(struct i2c_adapter *);
> +	void (*set_scl)(struct i2c_adapter *, int val);
> +	int (*get_sda)(struct i2c_adapter *);
> +
> +	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
> +	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
> +
> +	/* gpio recovery */
> +	unsigned scl_gpio;
> +	unsigned sda_gpio;
> +};
> +
> +/* Generic recovery routines */
> +int i2c_generic_gpio_recovery(struct i2c_adapter *adap);
> +int i2c_generic_scl_recovery(struct i2c_adapter *adap);
> +int i2c_recover_bus(struct i2c_adapter *adap);
> +
>  /*
>   * i2c_adapter is the structure used to identify a physical i2c bus along
>   * with the access algorithms necessary to access it.
> @@ -393,6 +430,8 @@ struct i2c_adapter {
>  
>  	struct mutex userspace_clients_lock;
>  	struct list_head userspace_clients;
> +
> +	struct i2c_bus_recovery_info *bus_recovery_info;
>  };
>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)

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