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

["Uwe Kleine-König" <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hello,

On Thu, Jan 24, 2013 at 04:06:37PM +0530, Viresh Kumar wrote:
> Most number of versions for any patchset i submitted :)
So let's see if you do a v10 ... :-)
 
> +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);
> +
Do we want to break out here if sda is high?

> +	/*
> +	 * 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");
> +			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 */
Above you wrote "SCL", here "scl". I suggest to use one of them
consistently and use the same capitalisation for sda.

> +		if (bri->get_sda && val)
> +			if (bri->get_sda(adap))
> +				break;
Maybe better:
		/* break if sda got high */
		if (bri->get_sda && bri->get_sda(adap)) {
			/* don't leave with scl low */
			if (!val)
				bri->set_scl(adap, 1);
			break;
		}

> +	}
> +
> +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);
Why this?

Best regards
Uwe

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