em28xx: resolve the remaining issues with the i2c code

PATCH In review Frank Schäfer 3 participants 10 messages Jan 19, 2014 – Feb 11, 2014

Subsystems (2)

Media (V4L/DVB) EM28XX VIDEO4LINUX driver

lore

Share & tools

lore

Frank Schäfer

author

2014-01-19 21:47 UTC #0 lore raw

cover em28xx: resolve the remaining issues with the i2c code

This small patch series resolves the remaing issues with the em82xx i2c code which have been introduced with the recent changes.

Frank Schäfer (4):
em28xx-i2c: fix the i2c error description strings for -ENXIO
em28xx-i2c: fix the error code for unknown errors
em28xx-i2c: do not map -ENXIO errors to -ENODEV for empty i2c
transfers
em28xx-i2c: remove duplicate error printing code from

    em28xx_i2c_xfer()

 drivers/media/usb/em28xx/em28xx-i2c.c |   53 ++++++++++++++++++++-------------

1 Datei geändert, 32 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-)

Signature · 1 line

1.7.10.4

Frank Schäfer

author

2014-01-19 21:47 UTC #1 lore raw

PATCH 1/4 em28xx-i2c: fix the i2c error description strings for -ENXIO

Commit d845fb3ae5 "em28xx-i2c: add timeout debug information if i2c_debug enabled" has added wrong error descriptions for -ENXIO.
The strings are also missing terminating newline characters, which breaks the output format.

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-i2c.c |   12 ++++++------

1 Datei geändert, 6 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +6 -6

index 7e17240..bd8101d 100644
@@ -81,7 +81,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 			return len;
 		if (ret == 0x94 + len - 1) {
 			if (i2c_debug == 1)
-				em28xx_warn("R05 returned 0x%02x: I2C timeout",
+				em28xx_warn("R05 returned 0x%02x: I2C ACK error\n",
 					    ret);
 			return -ENXIO;
 		}

@@ -128,7 +128,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 			break;
 		if (ret == 0x94 + len - 1) {
 			if (i2c_debug == 1)
-				em28xx_warn("R05 returned 0x%02x: I2C timeout",
+				em28xx_warn("R05 returned 0x%02x: I2C ACK error\n",
 					    ret);
 			return -ENXIO;
 		}

@@ -210,7 +210,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 			return len;
 		if (ret == 0x10) {
 			if (i2c_debug == 1)
-				em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
+				em28xx_warn("I2C ACK error on writing to addr 0x%02x\n",
 					    addr);
 			return -ENXIO;
 		}

@@ -274,7 +274,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 	}
 	if (ret == 0x10) {
 		if (i2c_debug == 1)
-			em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
+			em28xx_warn("I2C ACK error on writing to addr 0x%02x\n",
 				    addr);
 		return -ENXIO;
 	}

@@ -337,7 +337,7 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 		return len;
 	else if (ret > 0) {
 		if (i2c_debug == 1)
-			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
+			em28xx_warn("Bus B R08 returned 0x%02x: I2C ACK error\n",
 				    ret);
 		return -ENXIO;
 	}

@@ -392,7 +392,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 		return len;
 	else if (ret > 0) {
 		if (i2c_debug == 1)
-			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
+			em28xx_warn("Bus B R08 returned 0x%02x: I2C ACK error\n",
 				    ret);
 		return -ENXIO;
 	}

Signature · 1 line

1.7.10.4

Frank Schäfer

author

2014-01-19 21:47 UTC #2 lore raw

PATCH 2/4 em28xx-i2c: fix the error code for unknown errors

In reply to Frank Schäfer

Commit e63b009d6e "em28xx-i2c: Fix error code for I2C error transfers" changed the code to return -ETIMEDOUT on all unknown errors.
But the proper error code for unknown errors is -EIO.
So only report -ETIMEDOUT in case of the errors 0x02 and 0x04, which are according to Mauro Carvalho Chehabs tests related to i2c clock stretching and return -EIO for the rest.

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-i2c.c |   29 +++++++++++++++++++++++------

1 Datei geändert, 23 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +23 -6

index bd8101d..ba6433c 100644
@@ -226,10 +226,18 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 		 * (even with high payload) ...
 		 */
 	}
-	if (i2c_debug)
-		em28xx_warn("write to i2c device at 0x%x timed out (status=%i)\n",
-			    addr, ret);
-	return -ETIMEDOUT;
+
+	if (ret == 0x02 || ret == 0x04) {
+		/* NOTE: these errors seem to be related to clock stretching */
+		if (i2c_debug)
+			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;
 }
 
 /*

@@ -279,8 +287,17 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 		return -ENXIO;
 	}
 
-	em28xx_warn("unknown i2c error (status=%i)\n", ret);
-	return -ETIMEDOUT;
+	if (ret == 0x02 || ret == 0x04) {
+		/* NOTE: these errors seem to be related to clock stretching */
+		if (i2c_debug)
+			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;
 }
 
 /*

Signature · 1 line

1.7.10.4

Frank Schäfer

author

2014-01-19 21:47 UTC #3 lore raw

PATCH 3/4 em28xx-i2c: do not map -ENXIO errors to -ENODEV for empty i2c transfers

In reply to Frank Schäfer

Commit e63b009d6e "" changed the error codes i2c ACK errors from -ENODEV to -ENXIO. But it also introduced a line that maps -ENXIO back to -ENODEV in case of empty i2c messages, which makes no sense, because
1.) an ACK error is an ACK error no matter what the i2c message content is 2.) -ENXIO is perfectly suited for probing, too
3.) we are loosing the ability to distinguish USB device disconnects

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-i2c.c |    1 -

1 Datei geändert, 1 Zeile entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +0 -1

index ba6433c..a26d7d4 100644
@@ -539,7 +539,6 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 				if (rc == -ENXIO) {
 					if (i2c_debug > 1)
 						printk(KERN_CONT " no device\n");
-					rc = -ENODEV;
 				} else {
 					if (i2c_debug > 1)
 						printk(KERN_CONT " ERROR: %i\n", rc);

Signature · 1 line

1.7.10.4

Mauro Carvalho Chehab

2014-02-04 18:47 UTC #4 lore raw

Re: [PATCH 3/4] em28xx-i2c: do not map -ENXIO errors to -ENODEV for empty i2c transfers

Em Sun, 19 Jan 2014 22:48:36 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 5 lines

Commit e63b009d6e "" changed the error codes i2c ACK errors from -ENODEV to -ENXIO. But it also introduced a line that maps -ENXIO back to -ENODEV in case of empty i2c messages, which makes no sense, because
1.) an ACK error is an ACK error no matter what the i2c message content is 2.) -ENXIO is perfectly suited for probing, too

I don't agree with this patch. 0-byte messages are only usin during device probe.

Quoted 1 line

3.) we are loosing the ability to distinguish USB device disconnects

Huh?

Quoted 18 lines

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-i2c.c |    1 -

1 Datei geändert, 1 Zeile entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +0 -1

index ba6433c..a26d7d4 100644
@@ -539,7 +539,6 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 				if (rc == -ENXIO) {
 					if (i2c_debug > 1)
 						printk(KERN_CONT " no device\n");
-					rc = -ENODEV;
 				} else {
 					if (i2c_debug > 1)
 						printk(KERN_CONT " ERROR: %i\n", rc);

Signature · 2 lines

Cheers,
Mauro

Frank Schäfer

author

2014-02-09 18:33 UTC #5 lore raw

Re: [PATCH 3/4] em28xx-i2c: do not map -ENXIO errors to -ENODEV for empty i2c transfers

Am 04.02.2014 19:47, schrieb Mauro Carvalho Chehab:

Quoted 10 lines

Em Sun, 19 Jan 2014 22:48:36 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 5 lines
Commit e63b009d6e "" changed the error codes i2c ACK errors from -ENODEV to -ENXIO. But it also introduced a line that maps -ENXIO back to -ENODEV in case of empty i2c messages, which makes no sense, because
1.) an ACK error is an ACK error no matter what the i2c message content is 2.) -ENXIO is perfectly suited for probing, too
I don't agree with this patch. 0-byte messages are only usin during device probe.

???

The error handling is inconsistent for no good reason.

The old code always returned -ENODEV.
Then you came to the conclusion that -ENODEV isn't good and we both agreed that -ENXIO is appropriate.
But then you decided to keep -ENODEV for 0-Byte messages only. Why ?
According to the i2c error code description, -ENXIO and -ENODEV are both suited for probing.
AFAICS there are zero reasons for returning different error codes in case of the same i2c ack error.
So please, either -ENODEV or -ENXIO instead of such inconsistencies.

Quoted 2 lines

Quoted 1 line
3.) we are loosing the ability to distinguish USB device disconnects
Huh?

Maybe (like me) you didn't notice that before.
This is probably the most cogent argument for changing -ENODEV to -ENXIO for i2c ack errors in case of USB devices. ;-)

Regards,
Frank

Quoted 17 lines

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-i2c.c |    1 -

1 Datei geändert, 1 Zeile entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +0 -1

index ba6433c..a26d7d4 100644
@@ -539,7 +539,6 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 				if (rc == -ENXIO) {
 					if (i2c_debug > 1)
 						printk(KERN_CONT " no device\n");
-					rc = -ENODEV;
 				} else {
 					if (i2c_debug > 1)
 						printk(KERN_CONT " ERROR: %i\n", rc);

Antti Palosaari

2014-02-11 23:51 UTC #6 lore raw

Re: [PATCH 3/4] em28xx-i2c: do not map -ENXIO errors to -ENODEV for empty i2c transfers

Quoted 27 lines · Frank Schäfer

Am 04.02.2014 19:47, schrieb Mauro Carvalho Chehab:
Quoted 10 lines
Em Sun, 19 Jan 2014 22:48:36 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 5 lines
Commit e63b009d6e "" changed the error codes i2c ACK errors from -ENODEV to -ENXIO. But it also introduced a line that maps -ENXIO back to -ENODEV in case of empty i2c messages, which makes no sense, because
1.) an ACK error is an ACK error no matter what the i2c message content is 2.) -ENXIO is perfectly suited for probing, too
I don't agree with this patch. 0-byte messages are only usin during device probe.
???
The error handling is inconsistent for no good reason.
The old code always returned -ENODEV.
Then you came to the conclusion that -ENODEV isn't good and we both agreed that -ENXIO is appropriate.
But then you decided to keep -ENODEV for 0-Byte messages only. Why ?
According to the i2c error code description, -ENXIO and -ENODEV are both suited for probing.
AFAICS there are zero reasons for returning different error codes in case of the same i2c ack error.
So please, either -ENODEV or -ENXIO instead of such inconsistencies.

Quoted 5 lines

Quoted 2 lines
Quoted 1 line
3.) we are loosing the ability to distinguish USB device disconnects
Huh?
Maybe (like me) you didn't notice that before.
This is probably the most cogent argument for changing -ENODEV to -ENXIO for i2c ack errors in case of USB devices. ;-)

I looked the I2C spec and there seems to be *not* restriction I2C message len, nor any mention zero len is not valid. If that is the case, adapter should just do the zero len xfer if requested and return success if it is success (slave answers).

If adapter does not support zero len messages it should return EOPNOTSUPP according to kernel i2c fault codes document. I think em28xx supports, so that is not case here.

I think Frank is correct.

I2C spec is here:
http://www.nxp.com/documents/user_manual/UM10204.pdf

Hope this helps.

regards
Antti

Signature · 1 line

http://palosaari.fi/

Frank Schäfer

author

2014-01-19 21:47 UTC #7 lore raw

PATCH 4/4 em28xx-i2c: remove duplicate error printing code from em28xx_i2c_xfer()

In reply to Frank Schäfer

Signed-off-by Frank Schäfer <[email protected]>

---
 drivers/media/usb/em28xx/em28xx-i2c.c |   11 +++--------

1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)

drivers/media/usb/em28xx/em28xx-i2c.c +3 -8

index a26d7d4..1a514ca 100644
@@ -535,14 +535,9 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 			 * This code is only called during device probe.
 			 */
 			rc = i2c_check_for_device(i2c_bus, addr);
-			if (rc < 0) {
-				if (rc == -ENXIO) {
-					if (i2c_debug > 1)
-						printk(KERN_CONT " no device\n");
-				} else {
-					if (i2c_debug > 1)
-						printk(KERN_CONT " ERROR: %i\n", rc);
-				}
+			if (rc == -ENXIO) {
+				if (i2c_debug > 1)
+					printk(KERN_CONT " no device\n");
 				rt_mutex_unlock(&dev->i2c_bus_lock);
 				return rc;
 			}

Signature · 1 line

1.7.10.4

Mauro Carvalho Chehab

2014-02-04 18:50 UTC #8 lore raw

Re: [PATCH 4/4] em28xx-i2c: remove duplicate error printing code from em28xx_i2c_xfer()

Em Sun, 19 Jan 2014 22:48:37 +0100
Frank Schäfer <[email protected]> escreveu:

Quoted 14 lines

Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |   11 +++--------
1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-i2c.c +0 -1
index a26d7d4..1a514ca 100644
@@ -535,14 +535,9 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 			 * This code is only called during device probe.
 			 */
 			rc = i2c_check_for_device(i2c_bus, addr);
-			if (rc < 0) {

drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap, · line 540

Quoted 10 lines

drivers/media/usb/em28xx/em28xx-i2c.c +3 -7

-				if (rc == -ENXIO) {
-					if (i2c_debug > 1)
-						printk(KERN_CONT " no device\n");
-				} else {
-					if (i2c_debug > 1)
-						printk(KERN_CONT " ERROR: %i\n", rc);
-				}
+			if (rc == -ENXIO) {
+				if (i2c_debug > 1)
+					printk(KERN_CONT " no device\n");

Even if the previous patch were accepted, this one is wrong, as -ENXIO doesn't always mean that there's no device. Also, other return codes may happen here (like -EIO).

Quoted 3 lines

drivers/media/usb/em28xx/em28xx-i2c.c +0 -0

 				rt_mutex_unlock(&dev->i2c_bus_lock);
 				return rc;
 			}

Signature · 2 lines

Cheers,
Mauro

Frank Schäfer

author

2014-02-09 18:37 UTC #9 lore raw

Re: [PATCH 4/4] em28xx-i2c: remove duplicate error printing code from em28xx_i2c_xfer()

Am 04.02.2014 19:50, schrieb Mauro Carvalho Chehab:

Quoted 22 lines

Em Sun, 19 Jan 2014 22:48:37 +0100
Frank Schäfer <[email protected]> escreveu:
Quoted 19 lines
Signed-off-by Frank Schäfer <[email protected]>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |   11 +++--------
1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
drivers/media/usb/em28xx/em28xx-i2c.c +0 -6
index a26d7d4..1a514ca 100644
@@ -535,14 +535,9 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 			 * This code is only called during device probe.
 			 */
 			rc = i2c_check_for_device(i2c_bus, addr);
-			if (rc < 0) {
-				if (rc == -ENXIO) {
-					if (i2c_debug > 1)
-						printk(KERN_CONT " no device\n");
-				} else {
-					if (i2c_debug > 1)

Quoted 8 lines

drivers/media/usb/em28xx/em28xx-i2c.c static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap, · line 540
Quoted 5 lines
drivers/media/usb/em28xx/em28xx-i2c.c +3 -2
-						printk(KERN_CONT " ERROR: %i\n", rc);
-				}
+			if (rc == -ENXIO) {
+				if (i2c_debug > 1)
+					printk(KERN_CONT " no device\n");
Even if the previous patch were accepted, this one is wrong, as -ENXIO doesn't always mean that there's no device. Also, other return codes may happen here (like -EIO).

Mauro... please read the patch and the corresponding code again. You will notice that there is absolutely no functional change. The patch just removes duplicate code.
Hence your comment makes no sense.

If you insist on the weird -ENODEV/-ENXIO inconsistency you've introduced, then of course the patch needs to be rebased.

Quoted 3 lines

Quoted 3 lines
drivers/media/usb/em28xx/em28xx-i2c.c +0 -0
 				rt_mutex_unlock(&dev->i2c_bus_lock);
 				return rc;
 			}