Re: [PATCH 1/3] i2c: at91: move i2c_recover_bus() outside of at91_do_twi_transfer()

[Wolfram Sang <wsa@kernel.org>]

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

Hi Codrin,

sorry for the super-long delay. There is an issue here with regard to
bus recovery which affetcs more drivers and I can't make up my mind how
to handle it...

> Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")

Sidenote: I don't think this is a fix.

> +	if (ret < 0) {
> +		/*
> +		 * some faulty I2C slave devices might hold SDA down;
> +		 * we can send a bus clear command, hoping that the pins will be
> +		 * released
> +		 */
> +		i2c_recover_bus(&dev->adapter);
> +	} else {
> +		ret = num;
> +	}

So, one issue is more straightforward. Bus recovery is applied on all
errors. It should only be called when SDA is stuck.

The other issue is that bus recovery is applied after a transfer. The
I2C specs mention bus recovery only at the beginning of a transfer when
SDA is detected low. I think it also makes more sense because the bus
may also be stuck because of a misbehaving bootloader etc. This will be
caught when the check is done at the beginning.

However, moving the detection to the beginning leaves room for a
regression, because your driver already does it at the end of a
transfer. However, I'd think all regressions coming up need seperate
fixing anyhow. Unless I overlooked something, of course.

So, I think it should be moved to the beginning of a transfer, but I am
open for discussion, so we get the best possible bus recovery in Linux.

Happy hacking,

   Wolfram


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