Patchwork i2c: tegra: Retry transfer when no_ack status is detected

login
register
mail settings
Submitter Hendrik Lippek
Date April 19, 2013, 11:43 a.m.
Message ID <1366371804-7901-1-git-send-email-hendrik.lippek@avionic-design.de>
Download mbox | patch
Permalink /patch/237924/
State Not Applicable, archived
Headers show

Comments

Hendrik Lippek - April 19, 2013, 11:43 a.m.
In such conditions return -EAGAIN from driver.
This will cause the i2c-core to retry
the transmission as per the retry count and time-out specified by the
platform data of the adapter.

This fix is adapted from chromeos
commit:	16d971dd7ee9571746ff1d352fa3c0092a36478d

Signed-off-by: Hendrik Lippek <hendrik.lippek@avionic-design.de>
---
 drivers/i2c/busses/i2c-tegra.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Thierry Reding - April 22, 2013, 7:02 a.m.
On Fri, Apr 19, 2013 at 01:43:24PM +0200, Hendrik Lippek wrote:
> In such conditions return -EAGAIN from driver.
> This will cause the i2c-core to retry
> the transmission as per the retry count and time-out specified by the
> platform data of the adapter.
> 
> This fix is adapted from chromeos
> commit:	16d971dd7ee9571746ff1d352fa3c0092a36478d
> 
> Signed-off-by: Hendrik Lippek <hendrik.lippek@avionic-design.de>
> ---
>  drivers/i2c/busses/i2c-tegra.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Patches for I2C driver should go to the respective maintainers, not only
the Tegra mailing list. You can use scripts/get_maintainer.pl to obtain
a list of people and mailing lists to Cc (though you may want to apply
common sense and strip that list down a bit). In this case at least
Wolfram Sang and the linux-i2c mailing lists should be Cc'ed. I've added
them now.

Also it would be useful if you could describe the error case that this
patch fixes in more detail. That way people know the context and can
more easily judge whether the fix is appropriate or suggest alternative
solutions to the problem.

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index c1bd68d..81a6170 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -756,7 +756,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_bus *i2c_bus,
>  	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
>  		if (msg->flags & I2C_M_IGNORE_NAK)
>  			return 0;
> -		return -EREMOTEIO;
> +		return -EAGAIN;
>  	}
>  
>  	if (i2c_dev->msg_err & I2C_ERR_UNEXPECTED_STATUS)

I've looked a bit at what other drivers do when receiving a NAK, and
returning -EREMOTEIO is by far the most common way to deal with it.
Furthermore I'm not sure if repeating a transfer when no ACK is received
is the right thing to do. After all there's usually a good reason for a
missing ACK (e.g. slave not present). That also means if this patch is
applied, every access to a slave that isn't present will be retried
several times before failing. It also means that if the transfer finally
fails because the retry count reaches 0, the I2C core will actually
propagate -EAGAIN and hide the original cause of the error.

Perhaps a better alternative would be to use the I2C_M_IGNORE_NAK flag.
There is some useful information in Documentation/i2c/i2c-protocol,
section "Modified transactions". It seems like this isn't a general
problem with the Tegra I2C controller but rather with the specific chip
on this hardware. Can you try if adding the I2C_M_IGNORE_NAK flag to the
transfers makes the problem disappear as well?

Thierry

Patch

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c1bd68d..81a6170 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -756,7 +756,7 @@  static int tegra_i2c_xfer_msg(struct tegra_i2c_bus *i2c_bus,
 	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
-		return -EREMOTEIO;
+		return -EAGAIN;
 	}
 
 	if (i2c_dev->msg_err & I2C_ERR_UNEXPECTED_STATUS)