Re: [PATCH v2 1/3] [media] em28xx-i2c: Fix error code for I2C error transfers
On 11.01.2014 21:10, Mauro Carvalho Chehab wrote:
Em Sat, 11 Jan 2014 13:29:49 +0100
Frank Schäfer fschaefer@googlemail.com escreveu:
Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
Follow the error codes for I2C as described at Documentation/i2c/fault-codes.
In the case of the I2C status register (0x05), this is mapped into:
- ENXIO - when reg 05 returns 0x10
- ETIMEDOUT - when the device is not temporarily not responding
(e. g. reg 05 returning something not 0x10 or 0x00)
- EIO - for generic I/O errors that don't fit into the above.
In the specific case of 0-byte reads, used only during I2C device
probing, it keeps returning -ENODEV.
TODO: return EBUSY when reg 05 returns 0x20 on em2874 and upper.
Signed-off-by: Mauro Carvalho Chehab m.che...@samsung.com
---
drivers/media/usb/em28xx/em28xx-i2c.c | 37 +++
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
b/drivers/media/usb/em28xx/em28xx-i2c.c
index 342f35ad6070..76f956635bd9 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -80,7 +80,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr,
u8 *buf, u16 len)
if (ret == 0x80 + len - 1)
return len;
if (ret == 0x94 + len - 1) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from bridge
register (error=%i)\n,
@@ -90,7 +90,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr,
u8 *buf, u16 len)
msleep(5);
}
em28xx_warn(write to i2c device at 0x%x timed out\n, addr);
- return -EIO;
+ return -ETIMEDOUT;
Hmmm... we don't know anything about these unknown 2800 errors, they
probably do not exist at all.
But as the warning talks about a timeout, yes, let's return ETIMEDOUT
for now.
}
/*
@@ -123,7 +123,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8
addr, u8 *buf, u16 len)
if (ret == 0x84 + len - 1)
break;
if (ret == 0x94 + len - 1) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from bridge
register (error=%i)\n,
Now that I'm looking at this function again, the whole last code section
looks suspicious.
Maybe it is really necessary to make a pseudo read from regs 0x00-0x03,
but I wonder why we return the read data in this error case...
OTOH, I've spend a very long time on these functions making lots of
experiments, so I assume I had a good reason for this. ;)
Except for the return codes, better to not touch on em2800 I2C code.
There are few em2800 devices in the market. I remember that someone
did some cleanup on that code in the past. It took couple years to be
noticed.
Thankfully, the original author of the em2800 driver fixed it.
I have an em2800 device (Terratec Cinergy 200) and the last bigger fixer
for the em2800 i2c functions were made by me (e.g. 2fcc82d8).
I remember that you suspected some of these patches to be wrong, but
Sascha Sommer confirmed that they are right and they were finally applied.
Maybe that's what you remember ?
@@ -199,7 +199,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16
addr, u8 *buf,
if (ret == 0) /* success */
return len;
if (ret == 0x10) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from bridge
register (error=%i)\n,
@@ -213,9 +213,8 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16
addr, u8 *buf,
* (even with high payload) ...
*/
}
-
- em28xx_warn(write to i2c device at 0x%x timed out\n, addr);
- return -EIO;
+ em28xx_warn(write to i2c device at 0x%x timed out (status=%i)\n,
addr, ret);
+ return -ETIMEDOUT;
}
if (ret == 0x02 || ret == 0x04) { /* you may want to narrow this down a
bit more */
em28xx_warn(write to i2c device at 0x%x timed out (status=%i)\n,
addr, ret);
return -ETIMEDOUT;
em28xx_warn(write to i2c device at 0x%x failed with unknown error
(status=%i)\n, addr, ret);
return -EIO;
Let's keep it as-is for now. -ETIMEDOUT is enough to detect that the
error happened at reg 05, with makes easier for anyone to increase the
timeout time and see if this fixes an issue related to timeout.
Unlikley or not, the returned error must be sane.
We don't know anything about them, so EIO is the correct.
I considered adding there ret = 0x20 to return -EBUSY, but it seems
Re: [PATCH v2 1/3] [media] em28xx-i2c: Fix error code for I2C error transfers
Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
Follow the error codes for I2C as described at Documentation/i2c/fault-codes.
In the case of the I2C status register (0x05), this is mapped into:
- ENXIO - when reg 05 returns 0x10
- ETIMEDOUT - when the device is not temporarily not responding
(e. g. reg 05 returning something not 0x10 or 0x00)
- EIO - for generic I/O errors that don't fit into the above.
In the specific case of 0-byte reads, used only during I2C device
probing, it keeps returning -ENODEV.
TODO: return EBUSY when reg 05 returns 0x20 on em2874 and upper.
Signed-off-by: Mauro Carvalho Chehab m.che...@samsung.com
---
drivers/media/usb/em28xx/em28xx-i2c.c | 37
+++
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
b/drivers/media/usb/em28xx/em28xx-i2c.c
index 342f35ad6070..76f956635bd9 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -80,7 +80,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8
addr, u8 *buf, u16 len)
if (ret == 0x80 + len - 1)
return len;
if (ret == 0x94 + len - 1) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from
bridge register (error=%i)\n,
@@ -90,7 +90,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8
addr, u8 *buf, u16 len)
msleep(5);
}
em28xx_warn(write to i2c device at 0x%x timed out\n, addr);
- return -EIO;
+ return -ETIMEDOUT;
Hmmm... we don't know anything about these unknown 2800 errors, they
probably do not exist at all.
But as the warning talks about a timeout, yes, let's return ETIMEDOUT
for now.
}
/*
@@ -123,7 +123,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8
addr, u8 *buf, u16 len)
if (ret == 0x84 + len - 1)
break;
if (ret == 0x94 + len - 1) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from
bridge register (error=%i)\n,
Now that I'm looking at this function again, the whole last code section
looks suspicious.
Maybe it is really necessary to make a pseudo read from regs 0x00-0x03,
but I wonder why we return the read data in this error case...
OTOH, I've spend a very long time on these functions making lots of
experiments, so I assume I had a good reason for this. ;)
@@ -199,7 +199,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16
addr, u8 *buf,
if (ret == 0) /* success */
return len;
if (ret == 0x10) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from
bridge register (error=%i)\n,
@@ -213,9 +213,8 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16
addr, u8 *buf,
* (even with high payload) ...
*/
}
-
- em28xx_warn(write to i2c device at 0x%x timed out\n, addr);
- return -EIO;
+ em28xx_warn(write to i2c device at 0x%x timed out (status=%i)\n,
addr, ret);
+ return -ETIMEDOUT;
}
if (ret == 0x02 || ret == 0x04) { /* you may want to narrow this down a
bit more */
em28xx_warn(write to i2c device at 0x%x timed out (status=%i)\n,
addr, ret);
return -ETIMEDOUT;
em28xx_warn(write to i2c device at 0x%x failed with unknown error
(status=%i)\n, addr, ret);
return -EIO;
/*
@@ -245,7 +244,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16
addr, u8 *buf, u16 len)
* bytes if we are on bus B AND there was no write attempt to the
* specified slave address before AND no device is present at the
* requested slave address.
- * Anyway, the next check will fail with -ENODEV in this case, so avoid
+ * Anyway, the next check will fail with -ENXIO in this case, so avoid
* spamming the system log on device probing and do nothing here.
*/
@@ -259,10 +258,10 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev,
u16 addr, u8 *buf, u16 len)
return ret;
}
if (ret == 0x10)
- return -ENODEV;
+ return -ENXIO;
em28xx_warn(unknown i2c error (status=%i)\n, ret);
- return -EIO;
+ return -ETIMEDOUT;
The same here.
}
/*
@@ -318,7 +317,7 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev,
u16 addr, u8 *buf,
if (!ret)
return len;
else if (ret 0)
- return -ENODEV;
+ return -ENXIO;
return
Re: [PATCH v2 1/3] [media] em28xx-i2c: Fix error code for I2C error transfers
Em Sat, 11 Jan 2014 13:29:49 +0100
Frank Schäfer fschaefer@googlemail.com escreveu:
Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
Follow the error codes for I2C as described at
Documentation/i2c/fault-codes.
In the case of the I2C status register (0x05), this is mapped into:
- ENXIO - when reg 05 returns 0x10
- ETIMEDOUT - when the device is not temporarily not responding
(e. g. reg 05 returning something not 0x10 or 0x00)
- EIO - for generic I/O errors that don't fit into the above.
In the specific case of 0-byte reads, used only during I2C device
probing, it keeps returning -ENODEV.
TODO: return EBUSY when reg 05 returns 0x20 on em2874 and upper.
Signed-off-by: Mauro Carvalho Chehab m.che...@samsung.com
---
drivers/media/usb/em28xx/em28xx-i2c.c | 37
+++
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
b/drivers/media/usb/em28xx/em28xx-i2c.c
index 342f35ad6070..76f956635bd9 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -80,7 +80,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8
addr, u8 *buf, u16 len)
if (ret == 0x80 + len - 1)
return len;
if (ret == 0x94 + len - 1) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from
bridge register (error=%i)\n,
@@ -90,7 +90,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8
addr, u8 *buf, u16 len)
msleep(5);
}
em28xx_warn(write to i2c device at 0x%x timed out\n, addr);
- return -EIO;
+ return -ETIMEDOUT;
Hmmm... we don't know anything about these unknown 2800 errors, they
probably do not exist at all.
But as the warning talks about a timeout, yes, let's return ETIMEDOUT
for now.
}
/*
@@ -123,7 +123,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8
addr, u8 *buf, u16 len)
if (ret == 0x84 + len - 1)
break;
if (ret == 0x94 + len - 1) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from
bridge register (error=%i)\n,
Now that I'm looking at this function again, the whole last code section
looks suspicious.
Maybe it is really necessary to make a pseudo read from regs 0x00-0x03,
but I wonder why we return the read data in this error case...
OTOH, I've spend a very long time on these functions making lots of
experiments, so I assume I had a good reason for this. ;)
Except for the return codes, better to not touch on em2800 I2C code.
There are few em2800 devices in the market. I remember that someone
did some cleanup on that code in the past. It took couple years to be
noticed.
Thankfully, the original author of the em2800 driver fixed it.
@@ -199,7 +199,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev,
u16 addr, u8 *buf,
if (ret == 0) /* success */
return len;
if (ret == 0x10) {
- return -ENODEV;
+ return -ENXIO;
}
if (ret 0) {
em28xx_warn(failed to get i2c transfer status from
bridge register (error=%i)\n,
@@ -213,9 +213,8 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev,
u16 addr, u8 *buf,
* (even with high payload) ...
*/
}
-
- em28xx_warn(write to i2c device at 0x%x timed out\n, addr);
- return -EIO;
+ em28xx_warn(write to i2c device at 0x%x timed out (status=%i)\n,
addr, ret);
+ return -ETIMEDOUT;
}
if (ret == 0x02 || ret == 0x04) { /* you may want to narrow this down a
bit more */
em28xx_warn(write to i2c device at 0x%x timed out (status=%i)\n,
addr, ret);
return -ETIMEDOUT;
em28xx_warn(write to i2c device at 0x%x failed with unknown error
(status=%i)\n, addr, ret);
return -EIO;
Let's keep it as-is for now. -ETIMEDOUT is enough to detect that the
error happened at reg 05, with makes easier for anyone to increase the
timeout time and see if this fixes an issue related to timeout.
I considered adding there ret = 0x20 to return -EBUSY, but it seems
unlikely that this error will ever be detected.
/*
@@ -245,7 +244,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev,
u16 addr, u8 *buf, u16 len)
* bytes if we are on bus B AND there was no write attempt to the
* specified slave address before AND no device is present at the
* requested slave address.
-* Anyway, the next check will fail with -ENODEV in this case, so avoid
+* Anyway, the next check will fail with -ENXIO
