[PATCH v2 0/3] em28xx: improve I2C code

[PATCH v2 0/3] em28xx: improve I2C code

[Mauro Carvalho Chehab]

This is a series of cleanup patches for em28xx I2C. It was originally
part of a series of patches meant to split em28xx V4L2 module, but it
made sense to submit as a separate patch set.

Part of the original series were already merged, as the patches there
were ok.

The first patch on this series caused lots of discussions. I think we
finally got an agreement on it.

The other two patches were just rebased, as the context lines change
due to the changes on the first patch.

Mauro Carvalho Chehab (3):
  [media] em28xx-i2c: Fix error code for I2C error transfers
  [media] em28xx: cleanup I2C debug messages
  [media] em28xx: add timeout debug information if i2c_debug enabled

 drivers/media/usb/em28xx/em28xx-i2c.c | 138 ++++++++++++++++++++--------------
 1 file changed, 83 insertions(+), 55 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/3] [media] em28xx-i2c: Fix error code for I2C error transfers

[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.chehab@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;
 }
 
 /*
@@ -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",
@@ -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;
 }
 
 /*
@@ -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;
 }
 
 /*
@@ -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 ret;
 	/*
@@ -356,7 +355,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	 * 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.
 	 */
 
@@ -369,7 +368,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	if (!ret)
 		return len;
 	else if (ret > 0)
-		return -ENODEV;
+		return -ENXIO;
 
 	return ret;
 	/*
@@ -410,7 +409,7 @@ static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
 		rc = em2800_i2c_check_for_device(dev, addr);
 	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
 		rc = em25xx_bus_B_check_for_device(dev, addr);
-	if (rc == -ENODEV) {
+	if (rc == -ENXIO) {
 		if (i2c_debug)
 			printk(" no device\n");
 	}
@@ -498,11 +497,15 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
 			       i == num - 1 ? "stop" : "nonstop",
 			       addr, msgs[i].len);
-		if (!msgs[i].len) { /* no len: check only for device presence */
+		if (!msgs[i].len) {
+			/*
+			 * no len: check only for device presence
+			 * This code is only called during device probe.
+			 */
 			rc = i2c_check_for_device(i2c_bus, addr);
-			if (rc == -ENODEV) {
+			if (rc == -ENXIO) {
 				rt_mutex_unlock(&dev->i2c_bus_lock);
-				return rc;
+				return -ENODEV;
 			}
 		} else if (msgs[i].flags & I2C_M_RD) {
 			/* read bytes */
-- 
1.8.3.1

[PATCH v2 2/3] [media] em28xx: cleanup I2C debug messages

[Mauro Carvalho Chehab]

The I2C output messages is too polluted. Clean it a little
bit, by:
	- use the proper core support for memory dumps;
	- hide most stuff under the i2c_debug umbrella;
	- add the missing KERN_CONT where needed;
	- use 2 levels or verbosity. Only the second one
	  will show the I2C transfer data.

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c | 84 ++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 40 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 76f956635bd9..e8eb83160d36 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -41,7 +41,7 @@ MODULE_PARM_DESC(i2c_scan, "scan i2c bus at insmod time");
 
 static unsigned int i2c_debug;
 module_param(i2c_debug, int, 0644);
-MODULE_PARM_DESC(i2c_debug, "enable debug messages [i2c]");
+MODULE_PARM_DESC(i2c_debug, "i2c debug message level (1: normal debug, 2: show I2C transfers)");
 
 /*
  * em2800_i2c_send_bytes()
@@ -89,7 +89,8 @@ 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);
+	if (i2c_debug)
+		em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
 	return -ETIMEDOUT;
 }
 
@@ -132,8 +133,11 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 		}
 		msleep(5);
 	}
-	if (ret != 0x84 + len - 1)
-		em28xx_warn("read from i2c device at 0x%x timed out\n", addr);
+	if (ret != 0x84 + len - 1) {
+		if (i2c_debug)
+			em28xx_warn("read from i2c device at 0x%x timed out\n",
+				    addr);
+	}
 
 	/* get the received message */
 	ret = dev->em28xx_read_reg_req_len(dev, 0x00, 4-len, buf2, len);
@@ -213,7 +217,9 @@ 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 (status=%i)\n", addr, ret);
+	if (i2c_debug)
+		em28xx_warn("write to i2c device at 0x%x timed out (status=%i)\n",
+			    addr, ret);
 	return -ETIMEDOUT;
 }
 
@@ -409,10 +415,6 @@ static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
 		rc = em2800_i2c_check_for_device(dev, addr);
 	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
 		rc = em25xx_bus_B_check_for_device(dev, addr);
-	if (rc == -ENXIO) {
-		if (i2c_debug)
-			printk(" no device\n");
-	}
 	return rc;
 }
 
@@ -421,7 +423,7 @@ static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
 {
 	struct em28xx *dev = i2c_bus->dev;
 	u16 addr = msg.addr << 1;
-	int byte, rc = -EOPNOTSUPP;
+	int rc = -EOPNOTSUPP;
 
 	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX)
 		rc = em28xx_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
@@ -429,10 +431,6 @@ static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
 		rc = em2800_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
 	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
 		rc = em25xx_bus_B_recv_bytes(dev, addr, msg.buf, msg.len);
-	if (i2c_debug) {
-		for (byte = 0; byte < msg.len; byte++)
-			printk(" %02x", msg.buf[byte]);
-	}
 	return rc;
 }
 
@@ -441,12 +439,8 @@ static inline int i2c_send_bytes(struct em28xx_i2c_bus *i2c_bus,
 {
 	struct em28xx *dev = i2c_bus->dev;
 	u16 addr = msg.addr << 1;
-	int byte, rc = -EOPNOTSUPP;
+	int rc = -EOPNOTSUPP;
 
-	if (i2c_debug) {
-		for (byte = 0; byte < msg.len; byte++)
-			printk(" %02x", msg.buf[byte]);
-	}
 	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX)
 		rc = em28xx_i2c_send_bytes(dev, addr, msg.buf, msg.len, stop);
 	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800)
@@ -491,7 +485,7 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 	}
 	for (i = 0; i < num; i++) {
 		addr = msgs[i].addr << 1;
-		if (i2c_debug)
+		if (i2c_debug > 1)
 			printk(KERN_DEBUG "%s at %s: %s %s addr=%02x len=%d:",
 			       dev->name, __func__ ,
 			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
@@ -503,25 +497,41 @@ 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 == -ENXIO) {
+			if (rc < 0) {
+				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);
+				}
 				rt_mutex_unlock(&dev->i2c_bus_lock);
-				return -ENODEV;
+				return rc;
 			}
 		} else if (msgs[i].flags & I2C_M_RD) {
 			/* read bytes */
 			rc = i2c_recv_bytes(i2c_bus, msgs[i]);
+
+			if (i2c_debug > 1 && rc >= 0)
+				printk(KERN_CONT " %*ph",
+				       msgs[i].len, msgs[i].buf);
 		} else {
+			if (i2c_debug > 1)
+				printk(KERN_CONT " %*ph",
+				       msgs[i].len, msgs[i].buf);
+
 			/* write bytes */
 			rc = i2c_send_bytes(i2c_bus, msgs[i], i == num - 1);
 		}
 		if (rc < 0) {
-			if (i2c_debug)
-				printk(" ERROR: %i\n", rc);
+			if (i2c_debug > 1)
+				printk(KERN_CONT " ERROR: %i\n", rc);
 			rt_mutex_unlock(&dev->i2c_bus_lock);
 			return rc;
 		}
-		if (i2c_debug)
-			printk("\n");
+		if (i2c_debug > 1)
+			printk(KERN_CONT "\n");
 	}
 
 	rt_mutex_unlock(&dev->i2c_bus_lock);
@@ -604,7 +614,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned bus,
 	 * calculation and returned device dataset. Simplifies the code a lot,
 	 * but we might have to deal with multiple sizes in the future !
 	 */
-	int i, err;
+	int err;
 	struct em28xx_eeprom *dev_config;
 	u8 buf, *data;
 
@@ -635,20 +645,14 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned bus,
 		goto error;
 	}
 
-	/* Display eeprom content */
-	for (i = 0; i < len; i++) {
-		if (0 == (i % 16)) {
-			if (dev->eeprom_addrwidth_16bit)
-				em28xx_info("i2c eeprom %04x:", i);
-			else
-				em28xx_info("i2c eeprom %02x:", i);
-		}
-		printk(" %02x", data[i]);
-		if (15 == (i % 16))
-			printk("\n");
+	if (i2c_debug) {
+		/* Display eeprom content */
+		print_hex_dump(KERN_INFO, "eeprom ", DUMP_PREFIX_OFFSET,
+			       16, 1, data, len, true);
+
+		if (dev->eeprom_addrwidth_16bit)
+			em28xx_info("eeprom %06x: ... (skipped)\n", 256);
 	}
-	if (dev->eeprom_addrwidth_16bit)
-		em28xx_info("i2c eeprom %04x: ... (skipped)\n", i);
 
 	if (dev->eeprom_addrwidth_16bit &&
 	    data[0] == 0x26 && data[3] == 0x00) {
-- 
1.8.3.1

[PATCH v2 3/3] [media] em28xx: add timeout debug information if i2c_debug enabled

[Mauro Carvalho Chehab]

If i2c_debug is enabled, we splicitly want to know when a
device fails with timeout.

If i2c_debug==2, this is already provided, for each I2C transfer
that fails.

However, most of the time, we don't need to go that far. We just
want to know that I2C transfers fail.

So, add such errors for normal (ret == 0x10) I2C aborted timeouts.

Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index e8eb83160d36..7e1724076ac4 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -80,6 +80,9 @@ 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) {
+			if (i2c_debug == 1)
+				em28xx_warn("R05 returned 0x%02x: I2C timeout",
+					    ret);
 			return -ENXIO;
 		}
 		if (ret < 0) {
@@ -124,6 +127,9 @@ 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) {
+			if (i2c_debug == 1)
+				em28xx_warn("R05 returned 0x%02x: I2C timeout",
+					    ret);
 			return -ENXIO;
 		}
 		if (ret < 0) {
@@ -203,6 +209,9 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 		if (ret == 0) /* success */
 			return len;
 		if (ret == 0x10) {
+			if (i2c_debug == 1)
+				em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
+					    addr);
 			return -ENXIO;
 		}
 		if (ret < 0) {
@@ -263,8 +272,12 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 			    ret);
 		return ret;
 	}
-	if (ret == 0x10)
+	if (ret == 0x10) {
+		if (i2c_debug == 1)
+			em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
+				    addr);
 		return -ENXIO;
+	}
 
 	em28xx_warn("unknown i2c error (status=%i)\n", ret);
 	return -ETIMEDOUT;
@@ -322,8 +335,12 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	 */
 	if (!ret)
 		return len;
-	else if (ret > 0)
+	else if (ret > 0) {
+		if (i2c_debug == 1)
+			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
+				    ret);
 		return -ENXIO;
+	}
 
 	return ret;
 	/*
@@ -373,8 +390,12 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	 */
 	if (!ret)
 		return len;
-	else if (ret > 0)
+	else if (ret > 0) {
+		if (i2c_debug == 1)
+			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
+				    ret);
 		return -ENXIO;
+	}
 
 	return ret;
 	/*
-- 
1.8.3.1

Re: [PATCH v2 1/3] [media] em28xx-i2c: Fix error code for I2C error transfers

[Frank Schäfer]

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.chehab@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 ret;
>  	/*
> @@ -356,7 +355,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>  	 * 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.
>  	 */
>  
> @@ -369,7 +368,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>  	if (!ret)
>  		return len;
>  	else if (ret > 0)
> -		return -ENODEV;
> +		return -ENXIO;
>  
>  	return ret;
>  	/*
> @@ -410,7 +409,7 @@ static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
>  		rc = em2800_i2c_check_for_device(dev, addr);
>  	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
>  		rc = em25xx_bus_B_check_for_device(dev, addr);
> -	if (rc == -ENODEV) {
> +	if (rc == -ENXIO) {
>  		if (i2c_debug)
>  			printk(" no device\n");
>  	}
> @@ -498,11 +497,15 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>  			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>  			       i == num - 1 ? "stop" : "nonstop",
>  			       addr, msgs[i].len);
> -		if (!msgs[i].len) { /* no len: check only for device presence */
> +		if (!msgs[i].len) {
> +			/*
> +			 * no len: check only for device presence
> +			 * This code is only called during device probe.
> +			 */
>  			rc = i2c_check_for_device(i2c_bus, addr);
> -			if (rc == -ENODEV) {
> +			if (rc == -ENXIO) {
>  				rt_mutex_unlock(&dev->i2c_bus_lock);
> -				return rc;
> +				return -ENODEV;
I assume this is a small mistake ? ;)

>  			}
>  		} else if (msgs[i].flags & I2C_M_RD) {
>  			/* read bytes */

Re: [PATCH v2 2/3] [media] em28xx: cleanup I2C debug messages

[Frank Schäfer]

Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
> The I2C output messages is too polluted. Clean it a little
> bit, by:
> 	- use the proper core support for memory dumps;
> 	- hide most stuff under the i2c_debug umbrella;
> 	- add the missing KERN_CONT where needed;
> 	- use 2 levels or verbosity. Only the second one
> 	  will show the I2C transfer data.
>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> ---
>  drivers/media/usb/em28xx/em28xx-i2c.c | 84 ++++++++++++++++++-----------------
>  1 file changed, 44 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 76f956635bd9..e8eb83160d36 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -41,7 +41,7 @@ MODULE_PARM_DESC(i2c_scan, "scan i2c bus at insmod time");
>  
>  static unsigned int i2c_debug;
>  module_param(i2c_debug, int, 0644);
> -MODULE_PARM_DESC(i2c_debug, "enable debug messages [i2c]");
> +MODULE_PARM_DESC(i2c_debug, "i2c debug message level (1: normal debug, 2: show I2C transfers)");
>  
>  /*
>   * em2800_i2c_send_bytes()
> @@ -89,7 +89,8 @@ 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);
> +	if (i2c_debug)
> +		em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
>  	return -ETIMEDOUT;
>  }
>  
> @@ -132,8 +133,11 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
>  		}
>  		msleep(5);
>  	}
> -	if (ret != 0x84 + len - 1)
> -		em28xx_warn("read from i2c device at 0x%x timed out\n", addr);
> +	if (ret != 0x84 + len - 1) {
> +		if (i2c_debug)
> +			em28xx_warn("read from i2c device at 0x%x timed out\n",
> +				    addr);
> +	}
>  
>  	/* get the received message */
>  	ret = dev->em28xx_read_reg_req_len(dev, 0x00, 4-len, buf2, len);
> @@ -213,7 +217,9 @@ 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 (status=%i)\n", addr, ret);
> +	if (i2c_debug)
> +		em28xx_warn("write to i2c device at 0x%x timed out (status=%i)\n",
> +			    addr, ret);
>  	return -ETIMEDOUT;
>  }
>  
> @@ -409,10 +415,6 @@ static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
>  		rc = em2800_i2c_check_for_device(dev, addr);
>  	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
>  		rc = em25xx_bus_B_check_for_device(dev, addr);
> -	if (rc == -ENXIO) {
> -		if (i2c_debug)
> -			printk(" no device\n");
> -	}
>  	return rc;
>  }
>  
> @@ -421,7 +423,7 @@ static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
>  {
>  	struct em28xx *dev = i2c_bus->dev;
>  	u16 addr = msg.addr << 1;
> -	int byte, rc = -EOPNOTSUPP;
> +	int rc = -EOPNOTSUPP;
>  
>  	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX)
>  		rc = em28xx_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
> @@ -429,10 +431,6 @@ static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
>  		rc = em2800_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
>  	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
>  		rc = em25xx_bus_B_recv_bytes(dev, addr, msg.buf, msg.len);
> -	if (i2c_debug) {
> -		for (byte = 0; byte < msg.len; byte++)
> -			printk(" %02x", msg.buf[byte]);
> -	}
>  	return rc;
>  }
>  
> @@ -441,12 +439,8 @@ static inline int i2c_send_bytes(struct em28xx_i2c_bus *i2c_bus,
>  {
>  	struct em28xx *dev = i2c_bus->dev;
>  	u16 addr = msg.addr << 1;
> -	int byte, rc = -EOPNOTSUPP;
> +	int rc = -EOPNOTSUPP;
>  
> -	if (i2c_debug) {
> -		for (byte = 0; byte < msg.len; byte++)
> -			printk(" %02x", msg.buf[byte]);
> -	}
>  	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX)
>  		rc = em28xx_i2c_send_bytes(dev, addr, msg.buf, msg.len, stop);
>  	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800)
> @@ -491,7 +485,7 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>  	}
>  	for (i = 0; i < num; i++) {
>  		addr = msgs[i].addr << 1;
> -		if (i2c_debug)
> +		if (i2c_debug > 1)
>  			printk(KERN_DEBUG "%s at %s: %s %s addr=%02x len=%d:",
>  			       dev->name, __func__ ,
>  			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
> @@ -503,25 +497,41 @@ 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 == -ENXIO) {
> +			if (rc < 0) {
> +				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);
> +				}
This introduces unnecessary double warnings.

>  				rt_mutex_unlock(&dev->i2c_bus_lock);
> -				return -ENODEV;
> +				return rc;
>  			}
>  		} else if (msgs[i].flags & I2C_M_RD) {
>  			/* read bytes */
>  			rc = i2c_recv_bytes(i2c_bus, msgs[i]);
> +
> +			if (i2c_debug > 1 && rc >= 0)
> +				printk(KERN_CONT " %*ph",
> +				       msgs[i].len, msgs[i].buf);
>  		} else {
> +			if (i2c_debug > 1)
> +				printk(KERN_CONT " %*ph",
> +				       msgs[i].len, msgs[i].buf);
> +
>  			/* write bytes */
>  			rc = i2c_send_bytes(i2c_bus, msgs[i], i == num - 1);
>  		}
>  		if (rc < 0) {
> -			if (i2c_debug)
> -				printk(" ERROR: %i\n", rc);
> +			if (i2c_debug > 1)
> +				printk(KERN_CONT " ERROR: %i\n", rc);
>  			rt_mutex_unlock(&dev->i2c_bus_lock);
>  			return rc;
>  		}
> -		if (i2c_debug)
> -			printk("\n");
> +		if (i2c_debug > 1)
> +			printk(KERN_CONT "\n");
>  	}
>  
>  	rt_mutex_unlock(&dev->i2c_bus_lock);

I've been thinking about this for a while again and why we decided to
clean-up / remove the i2c_debug levels 1 year ago.
The idea was, that any errors except -ENXIO are unusal and should always
be be printed to the system log.
As a result, there was no longer a need for three i2c_debug levels.
The current levels are
i2c_debug = 0: warn on real/severe i2c errors
i2c_debug = 1: also print debug messages about -ENXIO errorr, display
the whole i2c traffic (messages)

What do you think, isn't that still sane ?

The question is now, what to do with clock stretching timeouts.
I would keep them at level 0, too, because it's an error that should not
occur and needs to fixed.


Another thing you're doing with this patch is to move the debugging code
from
i2c_check_for_device(), i2c_recv_device() and i2c_send_device() back to
to em28xx_i2c_xfer().
I have nothing against that, although I think we had decided to do this
opposite in the past.
IIRC, the main reason was to reduce the size of em28xx_i2c_xfer(). And I
think there was also an output formatting reason...

So IMHO, the only thing which should be changed here is to add the
missing KERN_CONT prefix.

> @@ -604,7 +614,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned bus,
>  	 * calculation and returned device dataset. Simplifies the code a lot,
>  	 * but we might have to deal with multiple sizes in the future !
>  	 */
> -	int i, err;
> +	int err;
>  	struct em28xx_eeprom *dev_config;
>  	u8 buf, *data;
>  
> @@ -635,20 +645,14 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned bus,
>  		goto error;
>  	}
>  
> -	/* Display eeprom content */
> -	for (i = 0; i < len; i++) {
> -		if (0 == (i % 16)) {
> -			if (dev->eeprom_addrwidth_16bit)
> -				em28xx_info("i2c eeprom %04x:", i);
> -			else
> -				em28xx_info("i2c eeprom %02x:", i);
> -		}
> -		printk(" %02x", data[i]);
> -		if (15 == (i % 16))
> -			printk("\n");
> +	if (i2c_debug) {
> +		/* Display eeprom content */
> +		print_hex_dump(KERN_INFO, "eeprom ", DUMP_PREFIX_OFFSET,
> +			       16, 1, data, len, true);
> +
> +		if (dev->eeprom_addrwidth_16bit)
> +			em28xx_info("eeprom %06x: ... (skipped)\n", 256);
That's a good change.

>  	}
> -	if (dev->eeprom_addrwidth_16bit)
> -		em28xx_info("i2c eeprom %04x: ... (skipped)\n", i);
The idea here was to signal that 16 bit eeproms are larger than 256 Bytes.
So you may want to keep this line.

>  
>  	if (dev->eeprom_addrwidth_16bit &&
>  	    data[0] == 0x26 && data[3] == 0x00) {

Re: [PATCH v2 3/3] [media] em28xx: add timeout debug information if i2c_debug enabled

[Frank Schäfer]

Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
> If i2c_debug is enabled, we splicitly want to know when a
> device fails with timeout.
>
> If i2c_debug==2, this is already provided, for each I2C transfer
> that fails.
>
> However, most of the time, we don't need to go that far. We just
> want to know that I2C transfers fail.
>
> So, add such errors for normal (ret == 0x10) I2C aborted timeouts.
>
> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> ---
>  drivers/media/usb/em28xx/em28xx-i2c.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index e8eb83160d36..7e1724076ac4 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -80,6 +80,9 @@ 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) {
> +			if (i2c_debug == 1)
> +				em28xx_warn("R05 returned 0x%02x: I2C timeout",
> +					    ret);
>  			return -ENXIO;
>  		}
>  		if (ret < 0) {
> @@ -124,6 +127,9 @@ 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) {
> +			if (i2c_debug == 1)
> +				em28xx_warn("R05 returned 0x%02x: I2C timeout",
> +					    ret);
>  			return -ENXIO;
>  		}
>  		if (ret < 0) {
> @@ -203,6 +209,9 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>  		if (ret == 0) /* success */
>  			return len;
>  		if (ret == 0x10) {
> +			if (i2c_debug == 1)
> +				em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
> +					    addr);
>  			return -ENXIO;
>  		}
>  		if (ret < 0) {
> @@ -263,8 +272,12 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
>  			    ret);
>  		return ret;
>  	}
> -	if (ret == 0x10)
> +	if (ret == 0x10) {
> +		if (i2c_debug == 1)
> +			em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
> +				    addr);
>  		return -ENXIO;
> +	}
>  
>  	em28xx_warn("unknown i2c error (status=%i)\n", ret);
>  	return -ETIMEDOUT;
> @@ -322,8 +335,12 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>  	 */
>  	if (!ret)
>  		return len;
> -	else if (ret > 0)
> +	else if (ret > 0) {
> +		if (i2c_debug == 1)
> +			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
> +				    ret);
>  		return -ENXIO;
> +	}
>  
>  	return ret;
>  	/*
> @@ -373,8 +390,12 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>  	 */
>  	if (!ret)
>  		return len;
> -	else if (ret > 0)
> +	else if (ret > 0) {
> +		if (i2c_debug == 1)
> +			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
> +				    ret);
>  		return -ENXIO;
> +	}
>  
>  	return ret;
>  	/*

The error description should be "I2C ACK error".

You are using (i2c_debug == 1) checks here, which should either be
changed to (i2c_debug > 0) in case of 3 debug levels.

Re: [PATCH v2 1/3] [media] em28xx-i2c: Fix error code for I2C error transfers

[Mauro Carvalho Chehab]

Em Sat, 11 Jan 2014 13:29:49 +0100
Frank Schäfer <fschaefer.oss@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.chehab@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 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 ret;
> >  	/*
> > @@ -356,7 +355,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >  	 * 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.
> >  	 */
> >  
> > @@ -369,7 +368,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >  	if (!ret)
> >  		return len;
> >  	else if (ret > 0)
> > -		return -ENODEV;
> > +		return -ENXIO;
> >  
> >  	return ret;
> >  	/*
> > @@ -410,7 +409,7 @@ static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
> >  		rc = em2800_i2c_check_for_device(dev, addr);
> >  	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
> >  		rc = em25xx_bus_B_check_for_device(dev, addr);
> > -	if (rc == -ENODEV) {
> > +	if (rc == -ENXIO) {
> >  		if (i2c_debug)
> >  			printk(" no device\n");
> >  	}
> > @@ -498,11 +497,15 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
> >  			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
> >  			       i == num - 1 ? "stop" : "nonstop",
> >  			       addr, msgs[i].len);
> > -		if (!msgs[i].len) { /* no len: check only for device presence */
> > +		if (!msgs[i].len) {
> > +			/*
> > +			 * no len: check only for device presence
> > +			 * This code is only called during device probe.
> > +			 */
> >  			rc = i2c_check_for_device(i2c_bus, addr);
> > -			if (rc == -ENODEV) {
> > +			if (rc == -ENXIO) {
> >  				rt_mutex_unlock(&dev->i2c_bus_lock);
> > -				return rc;
> > +				return -ENODEV;
> I assume this is a small mistake ? ;)

No. This is actually the only place where returning -ENODEV makes sense.
Messages with size 0 are used only during device probing.

> 
> >  			}
> >  		} else if (msgs[i].flags & I2C_M_RD) {
> >  			/* read bytes */
> 


-- 

Cheers,
Mauro

Re: [PATCH v2 2/3] [media] em28xx: cleanup I2C debug messages

[Mauro Carvalho Chehab]

Em Sat, 11 Jan 2014 14:09:44 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
> > The I2C output messages is too polluted. Clean it a little
> > bit, by:
> > 	- use the proper core support for memory dumps;
> > 	- hide most stuff under the i2c_debug umbrella;
> > 	- add the missing KERN_CONT where needed;
> > 	- use 2 levels or verbosity. Only the second one
> > 	  will show the I2C transfer data.
> >
> > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > ---
> >  drivers/media/usb/em28xx/em28xx-i2c.c | 84 ++++++++++++++++++-----------------
> >  1 file changed, 44 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> > index 76f956635bd9..e8eb83160d36 100644
> > --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> > +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> > @@ -41,7 +41,7 @@ MODULE_PARM_DESC(i2c_scan, "scan i2c bus at insmod time");
> >  
> >  static unsigned int i2c_debug;
> >  module_param(i2c_debug, int, 0644);
> > -MODULE_PARM_DESC(i2c_debug, "enable debug messages [i2c]");
> > +MODULE_PARM_DESC(i2c_debug, "i2c debug message level (1: normal debug, 2: show I2C transfers)");
> >  
> >  /*
> >   * em2800_i2c_send_bytes()
> > @@ -89,7 +89,8 @@ 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);
> > +	if (i2c_debug)
> > +		em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
> >  	return -ETIMEDOUT;
> >  }
> >  
> > @@ -132,8 +133,11 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
> >  		}
> >  		msleep(5);
> >  	}
> > -	if (ret != 0x84 + len - 1)
> > -		em28xx_warn("read from i2c device at 0x%x timed out\n", addr);
> > +	if (ret != 0x84 + len - 1) {
> > +		if (i2c_debug)
> > +			em28xx_warn("read from i2c device at 0x%x timed out\n",
> > +				    addr);
> > +	}
> >  
> >  	/* get the received message */
> >  	ret = dev->em28xx_read_reg_req_len(dev, 0x00, 4-len, buf2, len);
> > @@ -213,7 +217,9 @@ 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 (status=%i)\n", addr, ret);
> > +	if (i2c_debug)
> > +		em28xx_warn("write to i2c device at 0x%x timed out (status=%i)\n",
> > +			    addr, ret);
> >  	return -ETIMEDOUT;
> >  }
> >  
> > @@ -409,10 +415,6 @@ static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
> >  		rc = em2800_i2c_check_for_device(dev, addr);
> >  	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
> >  		rc = em25xx_bus_B_check_for_device(dev, addr);
> > -	if (rc == -ENXIO) {
> > -		if (i2c_debug)
> > -			printk(" no device\n");
> > -	}
> >  	return rc;
> >  }
> >  
> > @@ -421,7 +423,7 @@ static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
> >  {
> >  	struct em28xx *dev = i2c_bus->dev;
> >  	u16 addr = msg.addr << 1;
> > -	int byte, rc = -EOPNOTSUPP;
> > +	int rc = -EOPNOTSUPP;
> >  
> >  	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX)
> >  		rc = em28xx_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
> > @@ -429,10 +431,6 @@ static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
> >  		rc = em2800_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
> >  	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
> >  		rc = em25xx_bus_B_recv_bytes(dev, addr, msg.buf, msg.len);
> > -	if (i2c_debug) {
> > -		for (byte = 0; byte < msg.len; byte++)
> > -			printk(" %02x", msg.buf[byte]);
> > -	}
> >  	return rc;
> >  }
> >  
> > @@ -441,12 +439,8 @@ static inline int i2c_send_bytes(struct em28xx_i2c_bus *i2c_bus,
> >  {
> >  	struct em28xx *dev = i2c_bus->dev;
> >  	u16 addr = msg.addr << 1;
> > -	int byte, rc = -EOPNOTSUPP;
> > +	int rc = -EOPNOTSUPP;
> >  
> > -	if (i2c_debug) {
> > -		for (byte = 0; byte < msg.len; byte++)
> > -			printk(" %02x", msg.buf[byte]);
> > -	}
> >  	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX)
> >  		rc = em28xx_i2c_send_bytes(dev, addr, msg.buf, msg.len, stop);
> >  	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800)
> > @@ -491,7 +485,7 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
> >  	}
> >  	for (i = 0; i < num; i++) {
> >  		addr = msgs[i].addr << 1;
> > -		if (i2c_debug)
> > +		if (i2c_debug > 1)
> >  			printk(KERN_DEBUG "%s at %s: %s %s addr=%02x len=%d:",
> >  			       dev->name, __func__ ,
> >  			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
> > @@ -503,25 +497,41 @@ 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 == -ENXIO) {
> > +			if (rc < 0) {
> > +				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);
> > +				}
> This introduces unnecessary double warnings.

Again, this is called only during device probing time. The "no device" message
should be the expected one, for an error there, if probing at the wrong I2C
address.

However, a different error code there likely means a bug at the driver
(too short timeouts, lack of a msleep(), ...).

> 
> >  				rt_mutex_unlock(&dev->i2c_bus_lock);
> > -				return -ENODEV;
> > +				return rc;
> >  			}
> >  		} else if (msgs[i].flags & I2C_M_RD) {
> >  			/* read bytes */
> >  			rc = i2c_recv_bytes(i2c_bus, msgs[i]);
> > +
> > +			if (i2c_debug > 1 && rc >= 0)
> > +				printk(KERN_CONT " %*ph",
> > +				       msgs[i].len, msgs[i].buf);
> >  		} else {
> > +			if (i2c_debug > 1)
> > +				printk(KERN_CONT " %*ph",
> > +				       msgs[i].len, msgs[i].buf);
> > +
> >  			/* write bytes */
> >  			rc = i2c_send_bytes(i2c_bus, msgs[i], i == num - 1);
> >  		}
> >  		if (rc < 0) {
> > -			if (i2c_debug)
> > -				printk(" ERROR: %i\n", rc);
> > +			if (i2c_debug > 1)
> > +				printk(KERN_CONT " ERROR: %i\n", rc);
> >  			rt_mutex_unlock(&dev->i2c_bus_lock);
> >  			return rc;
> >  		}
> > -		if (i2c_debug)
> > -			printk("\n");
> > +		if (i2c_debug > 1)
> > +			printk(KERN_CONT "\n");
> >  	}
> >  
> >  	rt_mutex_unlock(&dev->i2c_bus_lock);
> 
> I've been thinking about this for a while again and why we decided to
> clean-up / remove the i2c_debug levels 1 year ago.
> The idea was, that any errors except -ENXIO are unusal and should always
> be be printed to the system log.
> As a result, there was no longer a need for three i2c_debug levels.
> The current levels are
> i2c_debug = 0: warn on real/severe i2c errors
> i2c_debug = 1: also print debug messages about -ENXIO errorr, display
> the whole i2c traffic (messages)
> 
> What do you think, isn't that still sane ?

I don't think so. Well, for sure the highest log level should be to
show all I2C traffic, but, as we're not displaying part of the I2C
errors at the logs anymore, we need an intermediate level.

> The question is now, what to do with clock stretching timeouts.
> I would keep them at level 0, too, because it's an error that should not
> occur and needs to fixed.

I think we should either show all I2C errors or hide everything.

It should be noticed that the I2C client drivers will likely show the
same errors there. So, those logs are actually a way to get more
details of the errors.

> 
> Another thing you're doing with this patch is to move the debugging code
> from
> i2c_check_for_device(), i2c_recv_device() and i2c_send_device() back to
> to em28xx_i2c_xfer().
> I have nothing against that, although I think we had decided to do this
> opposite in the past.
> IIRC, the main reason was to reduce the size of em28xx_i2c_xfer(). And I
> think there was also an output formatting reason...

It is complex to analyze the KERNEL_CONT messages if they're not at the
same function. That's why I decided to move them back.

> So IMHO, the only thing which should be changed here is to add the
> missing KERN_CONT prefix.
> 
> > @@ -604,7 +614,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned bus,
> >  	 * calculation and returned device dataset. Simplifies the code a lot,
> >  	 * but we might have to deal with multiple sizes in the future !
> >  	 */
> > -	int i, err;
> > +	int err;
> >  	struct em28xx_eeprom *dev_config;
> >  	u8 buf, *data;
> >  
> > @@ -635,20 +645,14 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned bus,
> >  		goto error;
> >  	}
> >  
> > -	/* Display eeprom content */
> > -	for (i = 0; i < len; i++) {
> > -		if (0 == (i % 16)) {
> > -			if (dev->eeprom_addrwidth_16bit)
> > -				em28xx_info("i2c eeprom %04x:", i);
> > -			else
> > -				em28xx_info("i2c eeprom %02x:", i);
> > -		}
> > -		printk(" %02x", data[i]);
> > -		if (15 == (i % 16))
> > -			printk("\n");
> > +	if (i2c_debug) {
> > +		/* Display eeprom content */
> > +		print_hex_dump(KERN_INFO, "eeprom ", DUMP_PREFIX_OFFSET,
> > +			       16, 1, data, len, true);
> > +
> > +		if (dev->eeprom_addrwidth_16bit)
> > +			em28xx_info("eeprom %06x: ... (skipped)\n", 256);
> That's a good change.
> 
> >  	}
> > -	if (dev->eeprom_addrwidth_16bit)
> > -		em28xx_info("i2c eeprom %04x: ... (skipped)\n", i);
> The idea here was to signal that 16 bit eeproms are larger than 256 Bytes.
> So you may want to keep this line.

This log is already on the previous change.

> >  
> >  	if (dev->eeprom_addrwidth_16bit &&
> >  	    data[0] == 0x26 && data[3] == 0x00) {
> 


-- 

Cheers,
Mauro

Re: [PATCH v2 3/3] [media] em28xx: add timeout debug information if i2c_debug enabled

[Mauro Carvalho Chehab]

Em Sat, 11 Jan 2014 14:14:38 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
> > If i2c_debug is enabled, we splicitly want to know when a
> > device fails with timeout.
> >
> > If i2c_debug==2, this is already provided, for each I2C transfer
> > that fails.
> >
> > However, most of the time, we don't need to go that far. We just
> > want to know that I2C transfers fail.
> >
> > So, add such errors for normal (ret == 0x10) I2C aborted timeouts.
> >
> > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> > ---
> >  drivers/media/usb/em28xx/em28xx-i2c.c | 27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> > index e8eb83160d36..7e1724076ac4 100644
> > --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> > +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> > @@ -80,6 +80,9 @@ 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) {
> > +			if (i2c_debug == 1)
> > +				em28xx_warn("R05 returned 0x%02x: I2C timeout",
> > +					    ret);
> >  			return -ENXIO;
> >  		}
> >  		if (ret < 0) {
> > @@ -124,6 +127,9 @@ 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) {
> > +			if (i2c_debug == 1)
> > +				em28xx_warn("R05 returned 0x%02x: I2C timeout",
> > +					    ret);
> >  			return -ENXIO;
> >  		}
> >  		if (ret < 0) {
> > @@ -203,6 +209,9 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >  		if (ret == 0) /* success */
> >  			return len;
> >  		if (ret == 0x10) {
> > +			if (i2c_debug == 1)
> > +				em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
> > +					    addr);
> >  			return -ENXIO;
> >  		}
> >  		if (ret < 0) {
> > @@ -263,8 +272,12 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
> >  			    ret);
> >  		return ret;
> >  	}
> > -	if (ret == 0x10)
> > +	if (ret == 0x10) {
> > +		if (i2c_debug == 1)
> > +			em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
> > +				    addr);
> >  		return -ENXIO;
> > +	}
> >  
> >  	em28xx_warn("unknown i2c error (status=%i)\n", ret);
> >  	return -ETIMEDOUT;
> > @@ -322,8 +335,12 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >  	 */
> >  	if (!ret)
> >  		return len;
> > -	else if (ret > 0)
> > +	else if (ret > 0) {
> > +		if (i2c_debug == 1)
> > +			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
> > +				    ret);
> >  		return -ENXIO;
> > +	}
> >  
> >  	return ret;
> >  	/*
> > @@ -373,8 +390,12 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >  	 */
> >  	if (!ret)
> >  		return len;
> > -	else if (ret > 0)
> > +	else if (ret > 0) {
> > +		if (i2c_debug == 1)
> > +			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
> > +				    ret);
> >  		return -ENXIO;
> > +	}
> >  
> >  	return ret;
> >  	/*
> 
> The error description should be "I2C ACK error".

Ok.

> 
> You are using (i2c_debug == 1) checks here, which should either be
> changed to (i2c_debug > 0) in case of 3 debug levels.

Actually, no. If you use i2c_debug = 2, you can't print anything on those
routines, as otherwise it will be printed on a line that it would supposed
to be a KERNEL_CONT message.

> 
> 


-- 

Cheers,
Mauro

Re: [PATCH v2 1/3] [media] em28xx-i2c: Fix error code for I2C error transfers

[Frank Schäfer]

On 11.01.2014 21:10, Mauro Carvalho Chehab wrote:
> Em Sat, 11 Jan 2014 13:29:49 +0100
> Frank Schäfer <fschaefer.oss@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.chehab@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
> 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 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 ret;
>>>   	/*
>>> @@ -356,7 +355,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>>   	 * 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.
>>>   	 */
>>>   
>>> @@ -369,7 +368,7 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>>   	if (!ret)
>>>   		return len;
>>>   	else if (ret > 0)
>>> -		return -ENODEV;
>>> +		return -ENXIO;
>>>   
>>>   	return ret;
>>>   	/*
>>> @@ -410,7 +409,7 @@ static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
>>>   		rc = em2800_i2c_check_for_device(dev, addr);
>>>   	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
>>>   		rc = em25xx_bus_B_check_for_device(dev, addr);
>>> -	if (rc == -ENODEV) {
>>> +	if (rc == -ENXIO) {
>>>   		if (i2c_debug)
>>>   			printk(" no device\n");
>>>   	}
>>> @@ -498,11 +497,15 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>>>   			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>>>   			       i == num - 1 ? "stop" : "nonstop",
>>>   			       addr, msgs[i].len);
>>> -		if (!msgs[i].len) { /* no len: check only for device presence */
>>> +		if (!msgs[i].len) {
>>> +			/*
>>> +			 * no len: check only for device presence
>>> +			 * This code is only called during device probe.
>>> +			 */
>>>   			rc = i2c_check_for_device(i2c_bus, addr);
>>> -			if (rc == -ENODEV) {
>>> +			if (rc == -ENXIO) {
>>>   				rt_mutex_unlock(&dev->i2c_bus_lock);
>>> -				return rc;
>>> +				return -ENODEV;
>> I assume this is a small mistake ? ;)
> No. This is actually the only place where returning -ENODEV makes sense.
> Messages with size 0 are used only during device probing.
That's both is true, but there is really no reason to map ENXIO to ENODEV.
ENXIO is valid for probing, too.

>
>>>   			}
>>>   		} else if (msgs[i].flags & I2C_M_RD) {
>>>   			/* read bytes */
>

Re: [PATCH v2 2/3] [media] em28xx: cleanup I2C debug messages

[Frank Schäfer]

On 11.01.2014 21:22, Mauro Carvalho Chehab wrote:
> Em Sat, 11 Jan 2014 14:09:44 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
>>> The I2C output messages is too polluted. Clean it a little
>>> bit, by:
>>> 	- use the proper core support for memory dumps;
>>> 	- hide most stuff under the i2c_debug umbrella;
>>> 	- add the missing KERN_CONT where needed;
>>> 	- use 2 levels or verbosity. Only the second one
>>> 	  will show the I2C transfer data.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
>>> ---
>>>   drivers/media/usb/em28xx/em28xx-i2c.c | 84 ++++++++++++++++++-----------------
>>>   1 file changed, 44 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>>> index 76f956635bd9..e8eb83160d36 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>>> @@ -41,7 +41,7 @@ MODULE_PARM_DESC(i2c_scan, "scan i2c bus at insmod time");
>>>   
>>>   static unsigned int i2c_debug;
>>>   module_param(i2c_debug, int, 0644);
>>> -MODULE_PARM_DESC(i2c_debug, "enable debug messages [i2c]");
>>> +MODULE_PARM_DESC(i2c_debug, "i2c debug message level (1: normal debug, 2: show I2C transfers)");
>>>   
>>>   /*
>>>    * em2800_i2c_send_bytes()
>>> @@ -89,7 +89,8 @@ 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);
>>> +	if (i2c_debug)
>>> +		em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
>>>   	return -ETIMEDOUT;
>>>   }
>>>   
>>> @@ -132,8 +133,11 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
>>>   		}
>>>   		msleep(5);
>>>   	}
>>> -	if (ret != 0x84 + len - 1)
>>> -		em28xx_warn("read from i2c device at 0x%x timed out\n", addr);
>>> +	if (ret != 0x84 + len - 1) {
>>> +		if (i2c_debug)
>>> +			em28xx_warn("read from i2c device at 0x%x timed out\n",
>>> +				    addr);
>>> +	}
>>>   
>>>   	/* get the received message */
>>>   	ret = dev->em28xx_read_reg_req_len(dev, 0x00, 4-len, buf2, len);
>>> @@ -213,7 +217,9 @@ 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 (status=%i)\n", addr, ret);
>>> +	if (i2c_debug)
>>> +		em28xx_warn("write to i2c device at 0x%x timed out (status=%i)\n",
>>> +			    addr, ret);
>>>   	return -ETIMEDOUT;
>>>   }
>>>   
>>> @@ -409,10 +415,6 @@ static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
>>>   		rc = em2800_i2c_check_for_device(dev, addr);
>>>   	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
>>>   		rc = em25xx_bus_B_check_for_device(dev, addr);
>>> -	if (rc == -ENXIO) {
>>> -		if (i2c_debug)
>>> -			printk(" no device\n");
>>> -	}
>>>   	return rc;
>>>   }
>>>   
>>> @@ -421,7 +423,7 @@ static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
>>>   {
>>>   	struct em28xx *dev = i2c_bus->dev;
>>>   	u16 addr = msg.addr << 1;
>>> -	int byte, rc = -EOPNOTSUPP;
>>> +	int rc = -EOPNOTSUPP;
>>>   
>>>   	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX)
>>>   		rc = em28xx_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
>>> @@ -429,10 +431,6 @@ static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
>>>   		rc = em2800_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
>>>   	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)
>>>   		rc = em25xx_bus_B_recv_bytes(dev, addr, msg.buf, msg.len);
>>> -	if (i2c_debug) {
>>> -		for (byte = 0; byte < msg.len; byte++)
>>> -			printk(" %02x", msg.buf[byte]);
>>> -	}
>>>   	return rc;
>>>   }
>>>   
>>> @@ -441,12 +439,8 @@ static inline int i2c_send_bytes(struct em28xx_i2c_bus *i2c_bus,
>>>   {
>>>   	struct em28xx *dev = i2c_bus->dev;
>>>   	u16 addr = msg.addr << 1;
>>> -	int byte, rc = -EOPNOTSUPP;
>>> +	int rc = -EOPNOTSUPP;
>>>   
>>> -	if (i2c_debug) {
>>> -		for (byte = 0; byte < msg.len; byte++)
>>> -			printk(" %02x", msg.buf[byte]);
>>> -	}
>>>   	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX)
>>>   		rc = em28xx_i2c_send_bytes(dev, addr, msg.buf, msg.len, stop);
>>>   	else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800)
>>> @@ -491,7 +485,7 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>>>   	}
>>>   	for (i = 0; i < num; i++) {
>>>   		addr = msgs[i].addr << 1;
>>> -		if (i2c_debug)
>>> +		if (i2c_debug > 1)
>>>   			printk(KERN_DEBUG "%s at %s: %s %s addr=%02x len=%d:",
>>>   			       dev->name, __func__ ,
>>>   			       (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>>> @@ -503,25 +497,41 @@ 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 == -ENXIO) {
>>> +			if (rc < 0) {
>>> +				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);
>>> +				}
>> This introduces unnecessary double warnings.
> Again, this is called only during device probing time. The "no device" message
> should be the expected one, for an error there, if probing at the wrong I2C
> address.
>
> However, a different error code there likely means a bug at the driver
> (too short timeouts, lack of a msleep(), ...).
???
Why do you want to warn twice about the same error ?

>
>>
>>>   				rt_mutex_unlock(&dev->i2c_bus_lock);
>>> -				return -ENODEV;
>>> +				return rc;
>>>   			}
>>>   		} else if (msgs[i].flags & I2C_M_RD) {
>>>   			/* read bytes */
>>>   			rc = i2c_recv_bytes(i2c_bus, msgs[i]);
>>> +
>>> +			if (i2c_debug > 1 && rc >= 0)
>>> +				printk(KERN_CONT " %*ph",
>>> +				       msgs[i].len, msgs[i].buf);
>>>   		} else {
>>> +			if (i2c_debug > 1)
>>> +				printk(KERN_CONT " %*ph",
>>> +				       msgs[i].len, msgs[i].buf);
>>> +
>>>   			/* write bytes */
>>>   			rc = i2c_send_bytes(i2c_bus, msgs[i], i == num - 1);
>>>   		}
>>>   		if (rc < 0) {
>>> -			if (i2c_debug)
>>> -				printk(" ERROR: %i\n", rc);
>>> +			if (i2c_debug > 1)
>>> +				printk(KERN_CONT " ERROR: %i\n", rc);
>>>   			rt_mutex_unlock(&dev->i2c_bus_lock);
>>>   			return rc;
>>>   		}
>>> -		if (i2c_debug)
>>> -			printk("\n");
>>> +		if (i2c_debug > 1)
>>> +			printk(KERN_CONT "\n");
>>>   	}
>>>   
>>>   	rt_mutex_unlock(&dev->i2c_bus_lock);
>> I've been thinking about this for a while again and why we decided to
>> clean-up / remove the i2c_debug levels 1 year ago.
>> The idea was, that any errors except -ENXIO are unusal and should always
>> be be printed to the system log.
>> As a result, there was no longer a need for three i2c_debug levels.
>> The current levels are
>> i2c_debug = 0: warn on real/severe i2c errors
>> i2c_debug = 1: also print debug messages about -ENXIO errorr, display
>> the whole i2c traffic (messages)
>>
>> What do you think, isn't that still sane ?
> I don't think so. Well, for sure the highest log level should be to
> show all I2C traffic, but, as we're not displaying part of the I2C
> errors at the logs anymore, we need an intermediate level.
...
>
>> The question is now, what to do with clock stretching timeouts.
>> I would keep them at level 0, too, because it's an error that should not
>> occur and needs to fixed.
> I think we should either show all I2C errors or hide everything.
>
> It should be noticed that the I2C client drivers will likely show the
> same errors there. So, those logs are actually a way to get more
> details of the errors.
Ok, at this point I'm changing my mind.
I2c errors should indeed not be reported by both, the adapter and the 
client (at least with debug disabled).
So the actual question here is, who should display them, the i2c adapter 
or the client driver ?
The arguments are the same as with the "automatic retries by the i2c 
adapter - yes or no ?" question,
so I think it should be up to the client drivers to decide whether or 
not an error is normal or critical and if it should be printed to the 
syslog.

Hence, 3 debug levels make sense. :)

>
>> Another thing you're doing with this patch is to move the debugging code
>> from
>> i2c_check_for_device(), i2c_recv_device() and i2c_send_device() back to
>> to em28xx_i2c_xfer().
>> I have nothing against that, although I think we had decided to do this
>> opposite in the past.
>> IIRC, the main reason was to reduce the size of em28xx_i2c_xfer(). And I
>> think there was also an output formatting reason...
> It is complex to analyze the KERNEL_CONT messages if they're not at the
> same function. That's why I decided to move them back.
>
>> So IMHO, the only thing which should be changed here is to add the
>> missing KERN_CONT prefix.
>>
>>> @@ -604,7 +614,7 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned bus,
>>>   	 * calculation and returned device dataset. Simplifies the code a lot,
>>>   	 * but we might have to deal with multiple sizes in the future !
>>>   	 */
>>> -	int i, err;
>>> +	int err;
>>>   	struct em28xx_eeprom *dev_config;
>>>   	u8 buf, *data;
>>>   
>>> @@ -635,20 +645,14 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned bus,
>>>   		goto error;
>>>   	}
>>>   
>>> -	/* Display eeprom content */
>>> -	for (i = 0; i < len; i++) {
>>> -		if (0 == (i % 16)) {
>>> -			if (dev->eeprom_addrwidth_16bit)
>>> -				em28xx_info("i2c eeprom %04x:", i);
>>> -			else
>>> -				em28xx_info("i2c eeprom %02x:", i);
>>> -		}
>>> -		printk(" %02x", data[i]);
>>> -		if (15 == (i % 16))
>>> -			printk("\n");
>>> +	if (i2c_debug) {
>>> +		/* Display eeprom content */
>>> +		print_hex_dump(KERN_INFO, "eeprom ", DUMP_PREFIX_OFFSET,
>>> +			       16, 1, data, len, true);
>>> +
>>> +		if (dev->eeprom_addrwidth_16bit)
>>> +			em28xx_info("eeprom %06x: ... (skipped)\n", 256);
>> That's a good change.
>>
>>>   	}
>>> -	if (dev->eeprom_addrwidth_16bit)
>>> -		em28xx_info("i2c eeprom %04x: ... (skipped)\n", i);
>> The idea here was to signal that 16 bit eeproms are larger than 256 Bytes.
>> So you may want to keep this line.
> This log is already on the previous change.
Yes, beat me ! Sorry for the noise.

>
>>>   
>>>   	if (dev->eeprom_addrwidth_16bit &&
>>>   	    data[0] == 0x26 && data[3] == 0x00) {
>

Re: [PATCH v2 3/3] [media] em28xx: add timeout debug information if i2c_debug enabled

[Frank Schäfer]

On 11.01.2014 21:27, Mauro Carvalho Chehab wrote:
> Em Sat, 11 Jan 2014 14:14:38 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
>>> If i2c_debug is enabled, we splicitly want to know when a
>>> device fails with timeout.
>>>
>>> If i2c_debug==2, this is already provided, for each I2C transfer
>>> that fails.
>>>
>>> However, most of the time, we don't need to go that far. We just
>>> want to know that I2C transfers fail.
>>>
>>> So, add such errors for normal (ret == 0x10) I2C aborted timeouts.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
>>> ---
>>>   drivers/media/usb/em28xx/em28xx-i2c.c | 27 ++++++++++++++++++++++++---
>>>   1 file changed, 24 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>>> index e8eb83160d36..7e1724076ac4 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>>> @@ -80,6 +80,9 @@ 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) {
>>> +			if (i2c_debug == 1)
>>> +				em28xx_warn("R05 returned 0x%02x: I2C timeout",
>>> +					    ret);
>>>   			return -ENXIO;
>>>   		}
>>>   		if (ret < 0) {
>>> @@ -124,6 +127,9 @@ 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) {
>>> +			if (i2c_debug == 1)
>>> +				em28xx_warn("R05 returned 0x%02x: I2C timeout",
>>> +					    ret);
>>>   			return -ENXIO;
>>>   		}
>>>   		if (ret < 0) {
>>> @@ -203,6 +209,9 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>>   		if (ret == 0) /* success */
>>>   			return len;
>>>   		if (ret == 0x10) {
>>> +			if (i2c_debug == 1)
>>> +				em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
>>> +					    addr);
>>>   			return -ENXIO;
>>>   		}
>>>   		if (ret < 0) {
>>> @@ -263,8 +272,12 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
>>>   			    ret);
>>>   		return ret;
>>>   	}
>>> -	if (ret == 0x10)
>>> +	if (ret == 0x10) {
>>> +		if (i2c_debug == 1)
>>> +			em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
>>> +				    addr);
>>>   		return -ENXIO;
>>> +	}
>>>   
>>>   	em28xx_warn("unknown i2c error (status=%i)\n", ret);
>>>   	return -ETIMEDOUT;
>>> @@ -322,8 +335,12 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>>   	 */
>>>   	if (!ret)
>>>   		return len;
>>> -	else if (ret > 0)
>>> +	else if (ret > 0) {
>>> +		if (i2c_debug == 1)
>>> +			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
>>> +				    ret);
>>>   		return -ENXIO;
>>> +	}
>>>   
>>>   	return ret;
>>>   	/*
>>> @@ -373,8 +390,12 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>>   	 */
>>>   	if (!ret)
>>>   		return len;
>>> -	else if (ret > 0)
>>> +	else if (ret > 0) {
>>> +		if (i2c_debug == 1)
>>> +			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
>>> +				    ret);
>>>   		return -ENXIO;
>>> +	}
>>>   
>>>   	return ret;
>>>   	/*
>> The error description should be "I2C ACK error".
> Ok.
>
>> You are using (i2c_debug == 1) checks here, which should either be
>> changed to (i2c_debug > 0) in case of 3 debug levels.
> Actually, no. If you use i2c_debug = 2, you can't print anything on those
> routines, as otherwise it will be printed on a line that it would supposed
> to be a KERNEL_CONT message.
Hmm ok, so this is to avoid output format issues.
I remember that this is a bit tricky...
That's why I was using some ordinary printks without \n linebreaks.

Re: [PATCH v2 3/3] [media] em28xx: add timeout debug information if i2c_debug enabled

[Mauro Carvalho Chehab]

Em Sun, 12 Jan 2014 16:29:26 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> On 11.01.2014 21:27, Mauro Carvalho Chehab wrote:
> > Em Sat, 11 Jan 2014 14:14:38 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 10.01.2014 09:33, schrieb Mauro Carvalho Chehab:
> >>> If i2c_debug is enabled, we splicitly want to know when a
> >>> device fails with timeout.
> >>>
> >>> If i2c_debug==2, this is already provided, for each I2C transfer
> >>> that fails.
> >>>
> >>> However, most of the time, we don't need to go that far. We just
> >>> want to know that I2C transfers fail.
> >>>
> >>> So, add such errors for normal (ret == 0x10) I2C aborted timeouts.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com>
> >>> ---
> >>>   drivers/media/usb/em28xx/em28xx-i2c.c | 27 ++++++++++++++++++++++++---
> >>>   1 file changed, 24 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> index e8eb83160d36..7e1724076ac4 100644
> >>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> @@ -80,6 +80,9 @@ 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) {
> >>> +			if (i2c_debug == 1)
> >>> +				em28xx_warn("R05 returned 0x%02x: I2C timeout",
> >>> +					    ret);
> >>>   			return -ENXIO;
> >>>   		}
> >>>   		if (ret < 0) {
> >>> @@ -124,6 +127,9 @@ 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) {
> >>> +			if (i2c_debug == 1)
> >>> +				em28xx_warn("R05 returned 0x%02x: I2C timeout",
> >>> +					    ret);
> >>>   			return -ENXIO;
> >>>   		}
> >>>   		if (ret < 0) {
> >>> @@ -203,6 +209,9 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >>>   		if (ret == 0) /* success */
> >>>   			return len;
> >>>   		if (ret == 0x10) {
> >>> +			if (i2c_debug == 1)
> >>> +				em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
> >>> +					    addr);
> >>>   			return -ENXIO;
> >>>   		}
> >>>   		if (ret < 0) {
> >>> @@ -263,8 +272,12 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
> >>>   			    ret);
> >>>   		return ret;
> >>>   	}
> >>> -	if (ret == 0x10)
> >>> +	if (ret == 0x10) {
> >>> +		if (i2c_debug == 1)
> >>> +			em28xx_warn("I2C transfer timeout on writing to addr 0x%02x",
> >>> +				    addr);
> >>>   		return -ENXIO;
> >>> +	}
> >>>   
> >>>   	em28xx_warn("unknown i2c error (status=%i)\n", ret);
> >>>   	return -ETIMEDOUT;
> >>> @@ -322,8 +335,12 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >>>   	 */
> >>>   	if (!ret)
> >>>   		return len;
> >>> -	else if (ret > 0)
> >>> +	else if (ret > 0) {
> >>> +		if (i2c_debug == 1)
> >>> +			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
> >>> +				    ret);
> >>>   		return -ENXIO;
> >>> +	}
> >>>   
> >>>   	return ret;
> >>>   	/*
> >>> @@ -373,8 +390,12 @@ static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >>>   	 */
> >>>   	if (!ret)
> >>>   		return len;
> >>> -	else if (ret > 0)
> >>> +	else if (ret > 0) {
> >>> +		if (i2c_debug == 1)
> >>> +			em28xx_warn("Bus B R08 returned 0x%02x: I2C timeout",
> >>> +				    ret);
> >>>   		return -ENXIO;
> >>> +	}
> >>>   
> >>>   	return ret;
> >>>   	/*
> >> The error description should be "I2C ACK error".
> > Ok.
> >
> >> You are using (i2c_debug == 1) checks here, which should either be
> >> changed to (i2c_debug > 0) in case of 3 debug levels.
> > Actually, no. If you use i2c_debug = 2, you can't print anything on those
> > routines, as otherwise it will be printed on a line that it would supposed
> > to be a KERNEL_CONT message.
> Hmm ok, so this is to avoid output format issues.
> I remember that this is a bit tricky...
> That's why I was using some ordinary printks without \n linebreaks.

Yep.

After reading all your 3 comments, it seems that there's no strong reason
why not merging this series.

Ok, improvements can be made, and I'm free to discuss them if you want to
submit some patches improving the I2C debug code, but there are much more
serious issues yet to be fixed, and I want to move on.

So, I'll merge this 3 patch series, plus the 4 patch alsa series, and start
working on the remaining issues, and to test your 8 RFT/RFC patch series.

Regards,
Mauro