| Message ID | 20210727111554.1338832-2-codrin.ciubotariu@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: at91: Fixes and updates | expand |
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
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c index 1cceb6866689..0352dc09d697 100644 --- a/drivers/i2c/busses/i2c-at91-master.c +++ b/drivers/i2c/busses/i2c-at91-master.c @@ -639,13 +639,6 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) AT91_TWI_THRCLR | AT91_TWI_LOCKCLR); } - /* - * 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); - return ret; } @@ -705,7 +698,17 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num) ret = at91_do_twi_transfer(dev); - ret = (ret < 0) ? ret : num; + 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; + } + out: pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);
This patch doesn't add a functional change, it just separates the recovery from the transfer itself. Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery") Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> --- drivers/i2c/busses/i2c-at91-master.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)