Re: [2/3] i2c: exynos5: implement bus recovery functionality

From: Wolfram Sang <wsa@the-dreams.de>
To: Andrzej Hajda <a.hajda@samsung.com>
Cc: Andi Shyti <andi.shyti@samsung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	"open list:I2C SUBSYSTEM" <linux-i2c@vger.kernel.org>,
	"moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES"
	<linux-samsung-soc@vger.kernel.org>
Subject: Re: [2/3] i2c: exynos5: implement bus recovery functionality
Date: Mon, 15 Jan 2018 21:52:50 +0100	[thread overview]
Message-ID: <20180115205250.p5mbn3ib2paj53pv@ninjato> (raw)
In-Reply-To: <20171130143007.30258-3-a.hajda@samsung.com>

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

Hi,

On Thu, Nov 30, 2017 at 03:30:06PM +0100, Andrzej Hajda wrote:
> In some circumstances I2C clients can be in abnormal state, to allow
> recover them from it I2C master can pulse SCL line until SDA line
> goes up and then generate STOP condition. Exynos driver does not have
> possibility to manipulate I2C lines directly, but it can provide similar
> functionality using manual commands. Replacing I2C adapter reset with

Really? READ_DATA looks like 8 clock pulses, but you need 9.

> bus recovery significantly incereases I2C bus robustness in case of timeouts.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

> @@ -642,7 +666,7 @@ static int exynos5_i2c_xfer_msg(struct exynos5_i2c *i2c,
>  		ret = exynos5_i2c_wait_bus_idle(i2c);
>  
>  	if (ret < 0) {
> -		exynos5_i2c_reset(i2c);
> +		exynos5_i2c_recovery(i2c);

Bus recovery is only defined for stalled busses, i.e. when SDA is stuck
low. It is not a suitable method to deal with general timeouts.

> +int exynos5_i2c_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct exynos5_i2c *i2c = adap->algo_data;
> +
> +	i2c_lock_adapter(adap);
> +	clk_enable(i2c->clk);
> +	exynos5_i2c_recovery(i2c);
> +	clk_disable(i2c->clk);
> +	i2c_unlock_adapter(adap);

Why do you need to lock? Aren't you protected by the call to master_xfer
which then detected a problem?

Regards,

   Wolfram

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]