[PATCH 0/4] em28xx: resolve the remaining issues with the i2c code

[PATCH 0/4] em28xx: resolve the remaining issues with the i2c code

[Frank Schäfer]

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(-)

-- 
1.7.10.4

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

[Frank Schäfer]

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 <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |   12 ++++++------
 1 Datei geändert, 6 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 7e17240..bd8101d 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -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;
 	}
-- 
1.7.10.4

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

[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 <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |   29 +++++++++++++++++++++++------
 1 Datei geändert, 23 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index bd8101d..ba6433c 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -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;
 }
 
 /*
-- 
1.7.10.4

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

[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 <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |    1 -
 1 Datei geändert, 1 Zeile entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index ba6433c..a26d7d4 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -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);
-- 
1.7.10.4

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

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |   11 +++--------
 1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index a26d7d4..1a514ca 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -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;
 			}
-- 
1.7.10.4

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

[Mauro Carvalho Chehab]

Em Sun, 19 Jan 2014 22:48:36 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> 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.

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

Huh?

> 
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-i2c.c |    1 -
>  1 Datei geändert, 1 Zeile entfernt(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index ba6433c..a26d7d4 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -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);


-- 

Cheers,
Mauro

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

[Mauro Carvalho Chehab]

Em Sun, 19 Jan 2014 22:48:37 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-i2c.c |   11 +++--------
>  1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index a26d7d4..1a514ca 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -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");

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).

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


-- 

Cheers,
Mauro

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

[Frank Schäfer]

Am 04.02.2014 19:47, schrieb Mauro Carvalho Chehab:
> Em Sun, 19 Jan 2014 22:48:36 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> 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.

>> 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

>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-i2c.c |    1 -
>>  1 Datei geändert, 1 Zeile entfernt(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>> index ba6433c..a26d7d4 100644
>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>> @@ -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);

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

[Frank Schäfer]

Am 04.02.2014 19:50, schrieb Mauro Carvalho Chehab:
> Em Sun, 19 Jan 2014 22:48:37 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-i2c.c |   11 +++--------
>>  1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>> index a26d7d4..1a514ca 100644
>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>> @@ -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");
> 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.

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

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

[Antti Palosaari]

On 09.02.2014 20:34, Frank Schäfer wrote:
>
> Am 04.02.2014 19:47, schrieb Mauro Carvalho Chehab:
>> Em Sun, 19 Jan 2014 22:48:36 +0100
>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>
>>> 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.
>
>>> 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

-- 
http://palosaari.fi/