[PATCH 0/5] em28xx: i2c bug fixes and cleanups

[PATCH 0/5] em28xx: i2c bug fixes and cleanups

[Frank Schäfer]

This patch series contains some I2C bug fixes / cleanups / unifications and 
improvements for the em28xx driver, which I've made while working on adding 
support for the em25xx/em276x i2c bus B support and playing with the 
Terratec Cinergy 200 USB which I've got recently.

Patches 1 and 5 are cleanup/unification patches, patches 2, 3, 4 fix some bugs.
Patch 3 actually fixes 2 bugs, but separate commits didn't make sense, because 
more or less the whole function had to be rewritten.


Frank Schäfer (5):
  em28xx: clean up the data type mess of the i2c transfer function
    parameters
  em28xx: respect the message size constraints for i2c transfers
  em28xx: fix two severe bugs in function em2800_i2c_recv_bytes()
  em28xx: fix the i2c adapter functionality flags
  em28xx: fix+improve+unify i2c error handling, debug messages and code
    comments

 drivers/media/usb/em28xx/em28xx-core.c |    5 +-
 drivers/media/usb/em28xx/em28xx-i2c.c  |  280 +++++++++++++++++++-------------
 drivers/media/usb/em28xx/em28xx.h      |    2 +-
 3 Dateien geändert, 172 Zeilen hinzugefügt(+), 115 Zeilen entfernt(-)

-- 
1.7.10.4

[PATCH 1/5] em28xx: clean up the data type mess of the i2c transfer function parameters

[Frank Schäfer]

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

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 1683bd9..44533e4 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -53,12 +53,11 @@ do {							\
  * em2800_i2c_send_max4()
  * send up to 4 bytes to the i2c device
  */
-static int em2800_i2c_send_max4(struct em28xx *dev, unsigned char addr,
-				char *buf, int len)
+static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
 	int ret;
 	int write_timeout;
-	unsigned char b2[6];
+	u8 b2[6];
 	BUG_ON(len < 1 || len > 4);
 	b2[5] = 0x80 + len - 1;
 	b2[4] = addr;
@@ -89,15 +88,13 @@ static int em2800_i2c_send_max4(struct em28xx *dev, unsigned char addr,
 /*
  * em2800_i2c_send_bytes()
  */
-static int em2800_i2c_send_bytes(void *data, unsigned char addr, char *buf,
-				 short len)
+static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
-	char *bufPtr = buf;
+	u8 *bufPtr = buf;
 	int ret;
 	int wrcount = 0;
 	int count;
 	int maxLen = 4;
-	struct em28xx *dev = (struct em28xx *)data;
 	while (len > 0) {
 		count = (len > maxLen) ? maxLen : len;
 		ret = em2800_i2c_send_max4(dev, addr, bufPtr, count);
@@ -115,9 +112,9 @@ static int em2800_i2c_send_bytes(void *data, unsigned char addr, char *buf,
  * em2800_i2c_check_for_device()
  * check if there is a i2c_device at the supplied address
  */
-static int em2800_i2c_check_for_device(struct em28xx *dev, unsigned char addr)
+static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
 {
-	char msg;
+	u8 msg;
 	int ret;
 	int write_timeout;
 	msg = addr;
@@ -150,8 +147,7 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, unsigned char addr)
  * em2800_i2c_recv_bytes()
  * read from the i2c device
  */
-static int em2800_i2c_recv_bytes(struct em28xx *dev, unsigned char addr,
-				 char *buf, int len)
+static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
 	int ret;
 	/* check for the device and set i2c read address */
@@ -174,11 +170,10 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, unsigned char addr,
 /*
  * em28xx_i2c_send_bytes()
  */
-static int em28xx_i2c_send_bytes(void *data, unsigned char addr, char *buf,
-				 short len, int stop)
+static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
+				 u16 len, int stop)
 {
 	int wrcount = 0;
-	struct em28xx *dev = (struct em28xx *)data;
 	int write_timeout, ret;
 
 	wrcount = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
@@ -199,8 +194,7 @@ static int em28xx_i2c_send_bytes(void *data, unsigned char addr, char *buf,
  * em28xx_i2c_recv_bytes()
  * read a byte from the i2c device
  */
-static int em28xx_i2c_recv_bytes(struct em28xx *dev, unsigned char addr,
-				 char *buf, int len)
+static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 {
 	int ret;
 	ret = dev->em28xx_read_reg_req_len(dev, 2, addr, buf, len);
@@ -217,7 +211,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, unsigned char addr,
  * em28xx_i2c_check_for_device()
  * check if there is a i2c_device at the supplied address
  */
-static int em28xx_i2c_check_for_device(struct em28xx *dev, unsigned char addr)
+static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
 {
 	int ret;
 
-- 
1.7.10.4

[PATCH 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

The em2800 can transfer up to 4 bytes per i2c message.
All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per message.

I2C adapters should never split messages transferred via the I2C subsystem
into multiple message transfers, because the result will almost always NOT be
the same as when the whole data is transferred to the I2C client in a single
message.
If the message size exceeds the capabilities of the I2C adapter, -EOPNOTSUPP
should be returned.

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

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 44533e4..c508c12 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -50,14 +50,18 @@ do {							\
 } while (0)
 
 /*
- * em2800_i2c_send_max4()
- * send up to 4 bytes to the i2c device
+ * em2800_i2c_send_bytes()
+ * send up to 4 bytes to the em2800 i2c device
  */
-static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
+static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
 	int ret;
 	int write_timeout;
 	u8 b2[6];
+
+	if (len < 1 || len > 4)
+		return -EOPNOTSUPP;
+
 	BUG_ON(len < 1 || len > 4);
 	b2[5] = 0x80 + len - 1;
 	b2[4] = addr;
@@ -86,29 +90,6 @@ static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 }
 
 /*
- * em2800_i2c_send_bytes()
- */
-static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
-{
-	u8 *bufPtr = buf;
-	int ret;
-	int wrcount = 0;
-	int count;
-	int maxLen = 4;
-	while (len > 0) {
-		count = (len > maxLen) ? maxLen : len;
-		ret = em2800_i2c_send_max4(dev, addr, bufPtr, count);
-		if (ret > 0) {
-			len -= count;
-			bufPtr += count;
-			wrcount += count;
-		} else
-			return (ret < 0) ? ret : -EFAULT;
-	}
-	return wrcount;
-}
-
-/*
  * em2800_i2c_check_for_device()
  * check if there is a i2c_device at the supplied address
  */
@@ -150,6 +131,10 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
 static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
 	int ret;
+
+	if (len < 1 || len > 4)
+		return -EOPNOTSUPP;
+
 	/* check for the device and set i2c read address */
 	ret = em2800_i2c_check_for_device(dev, addr);
 	if (ret) {
@@ -176,6 +161,9 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	int wrcount = 0;
 	int write_timeout, ret;
 
+	if (len < 1 || len > 64)
+		return -EOPNOTSUPP;
+
 	wrcount = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
 
 	/* Seems to be required after a write */
@@ -197,6 +185,10 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 {
 	int ret;
+
+	if (len < 1 || len > 64)
+		return -EOPNOTSUPP;
+
 	ret = dev->em28xx_read_reg_req_len(dev, 2, addr, buf, len);
 	if (ret < 0) {
 		em28xx_warn("reading i2c device failed (error=%i)\n", ret);
-- 
1.7.10.4

[PATCH 3/5] em28xx: fix two severe bugs in function em2800_i2c_recv_bytes()

[Frank Schäfer]

Function em2800_i2c_recv_bytes() has 2 severe bugs:
1) It does not wait for the i2c read to complete before reading the received
   message content from the bridge registers
2) Reading more than 1 byte doesn't work

The former can result in data corruption, the latter always does.

The rewritten code also superseds the content of function
em2800_i2c_check_for_device().

Tested with device "Terratec Cinergy 200 USB".

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |  104 ++++++++++++++++++---------------
 drivers/media/usb/em28xx/em28xx.h     |    2 +-
 2 Dateien geändert, 58 Zeilen hinzugefügt(+), 48 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index c508c12..940ff4d 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -73,12 +73,14 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 	if (len > 3)
 		b2[0] = buf[3];
 
+	/* trigger write */
 	ret = dev->em28xx_write_regs(dev, 4 - len, &b2[4 - len], 2 + len);
 	if (ret != 2 + len) {
 		em28xx_warn("writing to i2c device failed (error=%i)\n", ret);
 		return -EIO;
 	}
-	for (write_timeout = EM2800_I2C_WRITE_TIMEOUT; write_timeout > 0;
+	/* wait for completion */
+	for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
 	     write_timeout -= 5) {
 		ret = dev->em28xx_read_reg(dev, 0x05);
 		if (ret == 0x80 + len - 1)
@@ -90,66 +92,74 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 }
 
 /*
- * em2800_i2c_check_for_device()
- * check if there is a i2c_device at the supplied address
+ * em2800_i2c_recv_bytes()
+ * read up to 4 bytes from the em2800 i2c device
  */
-static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
+static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
-	u8 msg;
+	u8 buf2[4];
 	int ret;
-	int write_timeout;
-	msg = addr;
-	ret = dev->em28xx_write_regs(dev, 0x04, &msg, 1);
-	if (ret < 0) {
-		em28xx_warn("setting i2c device address failed (error=%i)\n",
-			    ret);
-		return ret;
-	}
-	msg = 0x84;
-	ret = dev->em28xx_write_regs(dev, 0x05, &msg, 1);
-	if (ret < 0) {
-		em28xx_warn("preparing i2c read failed (error=%i)\n", ret);
-		return ret;
+	int read_timeout;
+	int i;
+
+	if (len < 1 || len > 4)
+		return -EOPNOTSUPP;
+
+	/* trigger read */
+	buf2[1] = 0x84 + len - 1;
+	buf2[0] = addr;
+	ret = dev->em28xx_write_regs(dev, 0x04, buf2, 2);
+	if (ret != 2) {
+		em28xx_warn("failed to trigger read from i2c address 0x%x "
+			    "(error=%i)\n", addr, ret);
+		return (ret < 0) ? ret : -EIO;
 	}
-	for (write_timeout = EM2800_I2C_WRITE_TIMEOUT; write_timeout > 0;
-	     write_timeout -= 5) {
-		unsigned reg = dev->em28xx_read_reg(dev, 0x5);
 
-		if (reg == 0x94)
+	/* wait for completion */
+	for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
+	     read_timeout -= 5) {
+		ret = dev->em28xx_read_reg(dev, 0x05);
+		if (ret == 0x84 + len - 1) {
+			break;
+		} else if (ret == 0x94 + len - 1) {
 			return -ENODEV;
-		else if (reg == 0x84)
-			return 0;
+		} else if (ret < 0) {
+			em28xx_warn("failed to get i2c transfer status from "
+				    "bridge register (error=%i)\n", ret);
+			return ret;
+		}
 		msleep(5);
 	}
-	return -ENODEV;
+	if (ret != 0x84 + len - 1)
+		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);
+	if (ret != len) {
+		em28xx_warn("reading from i2c device at 0x%x failed: "
+			    "couldn't get the received message from the bridge "
+			    "(error=%i)\n", addr, ret);
+		return (ret < 0) ? ret : -EIO;
+	}
+	for (i=0; i<len; i++)
+		buf[i] = buf2[len-1-i];
+
+	return ret;
 }
 
 /*
- * em2800_i2c_recv_bytes()
- * read from the i2c device
+ * em2800_i2c_check_for_device()
+ * check if there is an i2c device at the supplied address
  */
-static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
+static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
 {
+	u8 buf;
 	int ret;
 
-	if (len < 1 || len > 4)
-		return -EOPNOTSUPP;
-
-	/* check for the device and set i2c read address */
-	ret = em2800_i2c_check_for_device(dev, addr);
-	if (ret) {
-		em28xx_warn
-		    ("preparing read at i2c address 0x%x failed (error=%i)\n",
-		     addr, ret);
-		return ret;
-	}
-	ret = dev->em28xx_read_reg_req_len(dev, 0x0, 0x3, buf, len);
-	if (ret < 0) {
-		em28xx_warn("reading from i2c device at 0x%x failed (error=%i)",
-			    addr, ret);
-		return ret;
-	}
-	return ret;
+	ret = em2800_i2c_recv_bytes(dev, addr, &buf, 1);
+	if (ret == 1)
+		return 0;
+	return (ret < 0) ? ret : -EIO;
 }
 
 /*
@@ -167,7 +177,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	wrcount = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
 
 	/* Seems to be required after a write */
-	for (write_timeout = EM2800_I2C_WRITE_TIMEOUT; write_timeout > 0;
+	for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
 	     write_timeout -= 5) {
 		ret = dev->em28xx_read_reg(dev, 0x05);
 		if (!ret)
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 062841e..dc89aaf 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -195,7 +195,7 @@
 */
 
 /* time in msecs to wait for i2c writes to finish */
-#define EM2800_I2C_WRITE_TIMEOUT 20
+#define EM2800_I2C_XFER_TIMEOUT		20
 
 enum em28xx_mode {
 	EM28XX_SUSPEND,
-- 
1.7.10.4

[PATCH 4/5] em28xx: fix the i2c adapter functionality flags

[Frank Schäfer]

I2C_FUNC_SMBUS_EMUL includes flag I2C_FUNC_SMBUS_WRITE_BLOCK_DATA which signals
that up to 31 data bytes can be written to the ic2 client.
But the EM2800 supports only i2c messages with max. 4 data bytes.
I2C_FUNC_IC2 should be set if a master_xfer fucntion pointer is provided in
struct i2c_algorithm.

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

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 940ff4d..7118535 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -445,7 +445,11 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
  */
 static u32 functionality(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_SMBUS_EMUL;
+	struct em28xx *dev = adap->algo_data;
+	u32 func_flags = I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	if (dev->board.is_em2800)
+		func_flags &= ~I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
+	return func_flags;
 }
 
 static struct i2c_algorithm em28xx_algo = {
-- 
1.7.10.4

[PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Frank Schäfer]

- check i2c slave address range (only 7 bit addresses supported)
- do not pass USB specific error codes to userspace/i2c-subsystem
- unify the returned error codes and make them compliant with
  the i2c subsystem spec
- check number of actually transferred bytes (via USB) everywehere
- fix/improve debug messages
- improve code comments

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c |    5 +-
 drivers/media/usb/em28xx/em28xx-i2c.c  |  116 +++++++++++++++++++++++---------
 2 Dateien geändert, 89 Zeilen hinzugefügt(+), 32 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index c78d38b..cd808bf 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -101,7 +101,7 @@ int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 reg,
 		if (reg_debug)
 			printk(" failed!\n");
 		mutex_unlock(&dev->ctrl_urb_lock);
-		return ret;
+		return usb_translate_errors(ret);
 	}
 
 	if (len)
@@ -182,6 +182,9 @@ int em28xx_write_regs_req(struct em28xx *dev, u8 req, u16 reg, char *buf,
 			      0x0000, reg, dev->urb_buf, len, HZ);
 	mutex_unlock(&dev->ctrl_urb_lock);
 
+	if (ret < 0)
+		return usb_translate_errors(ret);
+
 	if (dev->wait_after_write)
 		msleep(dev->wait_after_write);
 
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 7118535..508c3e1 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -76,18 +76,26 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 	/* trigger write */
 	ret = dev->em28xx_write_regs(dev, 4 - len, &b2[4 - len], 2 + len);
 	if (ret != 2 + len) {
-		em28xx_warn("writing to i2c device failed (error=%i)\n", ret);
-		return -EIO;
+		em28xx_warn("failed to trigger write to i2c address 0x%x "
+			    "(error=%i)\n", addr, ret);
+		return (ret < 0) ? ret : -EIO;
 	}
 	/* wait for completion */
 	for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
 	     write_timeout -= 5) {
 		ret = dev->em28xx_read_reg(dev, 0x05);
-		if (ret == 0x80 + len - 1)
+		if (ret == 0x80 + len - 1) {
 			return len;
+		} else if (ret == 0x94 + len - 1) {
+			return -ENODEV;
+		} else if (ret < 0) {
+			em28xx_warn("failed to get i2c transfer status from "
+				    "bridge register (error=%i)\n", ret);
+			return ret;
+		}
 		msleep(5);
 	}
-	em28xx_warn("i2c write timed out\n");
+	em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
 	return -EIO;
 }
 
@@ -168,24 +176,46 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
 static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 				 u16 len, int stop)
 {
-	int wrcount = 0;
 	int write_timeout, ret;
 
 	if (len < 1 || len > 64)
 		return -EOPNOTSUPP;
 
-	wrcount = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
+	/* Write to i2c device */
+	ret = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
+	if (ret != len) {
+		if (ret < 0) {
+			em28xx_warn("writing to i2c device at 0x%x failed "
+				    "(error=%i)\n", addr, ret);
+			return ret;
+		} else {
+			em28xx_warn("%i bytes write to i2c device at 0x%x "
+				    "requested, but %i bytes written\n",
+				    len, addr, ret);
+			return -EIO;
+		}
+	}
 
-	/* Seems to be required after a write */
+	/* Check success of the i2c operation */
 	for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
 	     write_timeout -= 5) {
 		ret = dev->em28xx_read_reg(dev, 0x05);
-		if (!ret)
-			break;
+		if (ret == 0) { /* success */
+			return len;
+		} else if (ret == 0x10) {
+			return -ENODEV;
+		} else if (ret < 0) {
+			em28xx_warn("failed to read i2c transfer status from "
+				    "bridge (error=%i)\n", ret);
+			return ret;
+		}
 		msleep(5);
+		/* NOTE: do we really have to wait for success ?
+		   Never seen anything else than 0x00 or 0x10
+		   (even with high payload) ...			*/
 	}
-
-	return wrcount;
+	em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
+	return -EIO;
 }
 
 /*
@@ -199,14 +229,37 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 	if (len < 1 || len > 64)
 		return -EOPNOTSUPP;
 
+	/* Read data from i2c device */
 	ret = dev->em28xx_read_reg_req_len(dev, 2, addr, buf, len);
+	if (ret != len) {
+		if (ret < 0) {
+			em28xx_warn("reading from i2c device at 0x%x failed "
+				    "(error=%i)\n", addr, ret);
+			return ret;
+		} else {
+			em28xx_warn("%i bytes requested from i2c device at "
+				    "0x%x, but %i bytes received\n",
+				    len, addr, ret);
+			return -EIO;
+		}
+	}
+
+	/* Check success of the i2c operation */
+	ret = dev->em28xx_read_reg(dev, 0x05);
 	if (ret < 0) {
-		em28xx_warn("reading i2c device failed (error=%i)\n", ret);
+		em28xx_warn("failed to read i2c transfer status from "
+			    "bridge (error=%i)\n", ret);
 		return ret;
 	}
-	if (dev->em28xx_read_reg(dev, 0x5) != 0)
-		return -ENODEV;
-	return ret;
+	if (ret > 0) {
+		if (ret == 0x10) {
+			return -ENODEV;
+		} else {
+			em28xx_warn("unknown i2c error (status=%i)\n", ret);
+			return -EIO;
+		}
+	}
+	return len;
 }
 
 /*
@@ -216,15 +269,12 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
 {
 	int ret;
+	u8 buf;
 
-	ret = dev->em28xx_read_reg_req(dev, 2, addr);
-	if (ret < 0) {
-		em28xx_warn("reading from i2c device failed (error=%i)\n", ret);
-		return ret;
-	}
-	if (dev->em28xx_read_reg(dev, 0x5) != 0)
-		return -ENODEV;
-	return 0;
+	ret = em28xx_i2c_recv_bytes(dev, addr, &buf, 1);
+	if (ret == 1)
+		return 0;
+	return (ret < 0) ? ret : -EIO;
 }
 
 /*
@@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 		dprintk2(2, "%s %s addr=%x len=%d:",
 			 (msgs[i].flags & I2C_M_RD) ? "read" : "write",
 			 i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
+		if (addr > 0xff) {
+			dprintk2(2, " ERROR: 10 bit addresses not supported\n");
+			return -EOPNOTSUPP;
+		}
 		if (!msgs[i].len) { /* no len: check only for device presence */
 			if (dev->board.is_em2800)
 				rc = em2800_i2c_check_for_device(dev, addr);
 			else
 				rc = em28xx_i2c_check_for_device(dev, addr);
-			if (rc < 0) {
-				dprintk2(2, " no device\n");
+			if (rc == -ENODEV) {
+				if (i2c_debug >= 2)
+					printk(" no device\n");
 				return rc;
 			}
-
 		} else if (msgs[i].flags & I2C_M_RD) {
 			/* read bytes */
 			if (dev->board.is_em2800)
@@ -284,16 +338,16 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 							   msgs[i].len,
 							   i == num - 1);
 		}
-		if (rc < 0)
-			goto err;
+		if (rc < 0) {
+			if (i2c_debug >= 2)
+				printk(" ERROR: %i\n", rc);
+			return rc;
+		}
 		if (i2c_debug >= 2)
 			printk("\n");
 	}
 
 	return num;
-err:
-	dprintk2(2, " ERROR: %i\n", rc);
-	return rc;
 }
 
 /* based on linux/sunrpc/svcauth.h and linux/hash.h
-- 
1.7.10.4

Re: [PATCH 2/5] em28xx: respect the message size constraints for i2c transfers

[Antti Palosaari]

On 12/14/2012 06:28 PM, Frank Schäfer wrote:
> The em2800 can transfer up to 4 bytes per i2c message.
> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per message.
>
> I2C adapters should never split messages transferred via the I2C subsystem
> into multiple message transfers, because the result will almost always NOT be
> the same as when the whole data is transferred to the I2C client in a single
> message.
> If the message size exceeds the capabilities of the I2C adapter, -EOPNOTSUPP
> should be returned.
>
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>


> +	if (len < 1 || len > 4)
> +		return -EOPNOTSUPP;

That patch seem to be good for my eyes, but that check for len < 1 is 
something I would like to double checked. Generally len = 0 is OK and is 
used some cases, probing and sometimes when all registers are read for 
example.

Did you test it returns some error for zero len messages?

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Antti Palosaari]

On 12/14/2012 06:28 PM, Frank Schäfer wrote:
> - check i2c slave address range (only 7 bit addresses supported)
> - do not pass USB specific error codes to userspace/i2c-subsystem
> - unify the returned error codes and make them compliant with
>    the i2c subsystem spec
> - check number of actually transferred bytes (via USB) everywehere
> - fix/improve debug messages
> - improve code comments
>
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>


> @@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>   		dprintk2(2, "%s %s addr=%x len=%d:",
>   			 (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>   			 i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
> +		if (addr > 0xff) {
> +			dprintk2(2, " ERROR: 10 bit addresses not supported\n");
> +			return -EOPNOTSUPP;
> +		}

There is own flag for 10bit I2C address. Use it (and likely not compare 
at all addr validly like that). This kind of address validation check is 
quite unnecessary - and after all if it is wanted then correct place is 
somewhere in I2C routines.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

Am 14.12.2012 17:55, schrieb Antti Palosaari:
> On 12/14/2012 06:28 PM, Frank Schäfer wrote:
>> The em2800 can transfer up to 4 bytes per i2c message.
>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per
>> message.
>>
>> I2C adapters should never split messages transferred via the I2C
>> subsystem
>> into multiple message transfers, because the result will almost
>> always NOT be
>> the same as when the whole data is transferred to the I2C client in a
>> single
>> message.
>> If the message size exceeds the capabilities of the I2C adapter,
>> -EOPNOTSUPP
>> should be returned.
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>
>
>> +    if (len < 1 || len > 4)
>> +        return -EOPNOTSUPP;
>
> That patch seem to be good for my eyes, but that check for len < 1 is
> something I would like to double checked. Generally len = 0 is OK and
> is used some cases, probing and sometimes when all registers are read
> for example.
>
> Did you test it returns some error for zero len messages?
>

First of all: thank you for reviewing the patch !

Yeah, messages with zero length could be used for device detection.
But yes, I checked that and it doesn't work.

For the em2800 algorithm, it doesn't work because of the way the mesage
length is encoded in the first two bytes of register 0x05:

118                buf2[1] = 0x84 + len - 1;

Bit 2 is used for read/write, that's why it is

67                b2[5] = 0x80 + len - 1;

for writes.


The more interesting case is the em28xx i2c algorithm...
But zero length messages don't work there, too (at least not with the
em2765 and em2820).
To be more precise: they SEEM to work, but they don't. Even when there
is no i2c device, you get no error (neither from the USB subsystem, nor
from the bridge status register).

Regards,
Frank

Re: [PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Frank Schäfer]

Am 14.12.2012 18:03, schrieb Antti Palosaari:
> On 12/14/2012 06:28 PM, Frank Schäfer wrote:
>> - check i2c slave address range (only 7 bit addresses supported)
>> - do not pass USB specific error codes to userspace/i2c-subsystem
>> - unify the returned error codes and make them compliant with
>>    the i2c subsystem spec
>> - check number of actually transferred bytes (via USB) everywehere
>> - fix/improve debug messages
>> - improve code comments
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>
>
>> @@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter
>> *i2c_adap,
>>           dprintk2(2, "%s %s addr=%x len=%d:",
>>                (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>>                i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
>> +        if (addr > 0xff) {
>> +            dprintk2(2, " ERROR: 10 bit addresses not supported\n");
>> +            return -EOPNOTSUPP;
>> +        }
>
> There is own flag for 10bit I2C address. Use it (and likely not
> compare at all addr validly like that). This kind of address
> validation check is quite unnecessary - and after all if it is wanted
> then correct place is somewhere in I2C routines.

Well, to be 100% sure and strict, we should check both, the flag and the
actual address.
We support 7 bit addresses only, no matter which i2c algo is used. So
doing the address check in each i2c routine seems to be unnecessary code
duplication to me ?

BTW: with the em28xx algorithm, the i2c address is transferred as 16 bit
value. So 10 bit addresses COULD work in theory... ;)

Regards,
Frank

Re: [PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Antti Palosaari]

On 12/15/2012 03:01 PM, Frank Schäfer wrote:
> Am 14.12.2012 18:03, schrieb Antti Palosaari:
>> On 12/14/2012 06:28 PM, Frank Schäfer wrote:
>>> - check i2c slave address range (only 7 bit addresses supported)
>>> - do not pass USB specific error codes to userspace/i2c-subsystem
>>> - unify the returned error codes and make them compliant with
>>>     the i2c subsystem spec
>>> - check number of actually transferred bytes (via USB) everywehere
>>> - fix/improve debug messages
>>> - improve code comments
>>>
>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>
>>
>>> @@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter
>>> *i2c_adap,
>>>            dprintk2(2, "%s %s addr=%x len=%d:",
>>>                 (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>>>                 i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
>>> +        if (addr > 0xff) {
>>> +            dprintk2(2, " ERROR: 10 bit addresses not supported\n");
>>> +            return -EOPNOTSUPP;
>>> +        }
>>
>> There is own flag for 10bit I2C address. Use it (and likely not
>> compare at all addr validly like that). This kind of address
>> validation check is quite unnecessary - and after all if it is wanted
>> then correct place is somewhere in I2C routines.
>
> Well, to be 100% sure and strict, we should check both, the flag and the
> actual address.
> We support 7 bit addresses only, no matter which i2c algo is used. So
> doing the address check in each i2c routine seems to be unnecessary code
> duplication to me ?

I will repeat me, I see it overkill to check address correctness. And as 
I said, that one is general validly could be done easily in I2C core - 
so why the hell you wish make it just only for em28xx ?

I am quite sure if that kind of address validness are saw important they 
are already implemented by I2C core.

Make patch for I2C which does that address validation against client 
10BIT flag and sent it to the mailing list for discussion.

> BTW: with the em28xx algorithm, the i2c address is transferred as 16 bit
> value. So 10 bit addresses COULD work in theory... ;)

Could be, but I think 10bit is never used in real life.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Frank Schäfer]

Am 15.12.2012 14:46, schrieb Antti Palosaari:
> On 12/15/2012 03:01 PM, Frank Schäfer wrote:
>> Am 14.12.2012 18:03, schrieb Antti Palosaari:
>>> On 12/14/2012 06:28 PM, Frank Schäfer wrote:
>>>> - check i2c slave address range (only 7 bit addresses supported)
>>>> - do not pass USB specific error codes to userspace/i2c-subsystem
>>>> - unify the returned error codes and make them compliant with
>>>>     the i2c subsystem spec
>>>> - check number of actually transferred bytes (via USB) everywehere
>>>> - fix/improve debug messages
>>>> - improve code comments
>>>>
>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>
>>>
>>>> @@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter
>>>> *i2c_adap,
>>>>            dprintk2(2, "%s %s addr=%x len=%d:",
>>>>                 (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>>>>                 i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
>>>> +        if (addr > 0xff) {
>>>> +            dprintk2(2, " ERROR: 10 bit addresses not supported\n");
>>>> +            return -EOPNOTSUPP;
>>>> +        }
>>>
>>> There is own flag for 10bit I2C address. Use it (and likely not
>>> compare at all addr validly like that). This kind of address
>>> validation check is quite unnecessary - and after all if it is wanted
>>> then correct place is somewhere in I2C routines.
>>
>> Well, to be 100% sure and strict, we should check both, the flag and the
>> actual address.
>> We support 7 bit addresses only, no matter which i2c algo is used. So
>> doing the address check in each i2c routine seems to be unnecessary code
>> duplication to me ?
>
> I will repeat me, I see it overkill to check address correctness. And
> as I said, that one is general validly could be done easily in I2C
> core - so why the hell you wish make it just only for em28xx ?
>
> I am quite sure if that kind of address validness are saw important
> they are already implemented by I2C core.
>
> Make patch for I2C which does that address validation against client
> 10BIT flag and sent it to the mailing list for discussion.

The I2C core doesn't know about the capabilities of the adapter.
Hence it doesn't know if ten bit addresses will work (the same as with
the message size constraints).
All it does ist to check the client for I2C_CLIENT_TEN && addr > 0x7f
once, when it is instanciated with a call to i2c_new_device().
But we don't use this function in em28xx and the same applies to many
other drivers as well.
Apart from that, the client address and flags can change anytime later
(e.g. when probing devices).

But if you hate the check, I can kick it out.
The risk that it will cause any problems in practice is small.

Regards,
Frank

>
>> BTW: with the em28xx algorithm, the i2c address is transferred as 16 bit
>> value. So 10 bit addresses COULD work in theory... ;)
>
> Could be, but I think 10bit is never used in real life.
>
> regards
> Antti
>

Re: [PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Antti Palosaari]

On 12/15/2012 06:25 PM, Frank Schäfer wrote:
> Am 15.12.2012 14:46, schrieb Antti Palosaari:
>> On 12/15/2012 03:01 PM, Frank Schäfer wrote:
>>> Am 14.12.2012 18:03, schrieb Antti Palosaari:
>>>> On 12/14/2012 06:28 PM, Frank Schäfer wrote:
>>>>> - check i2c slave address range (only 7 bit addresses supported)
>>>>> - do not pass USB specific error codes to userspace/i2c-subsystem
>>>>> - unify the returned error codes and make them compliant with
>>>>>      the i2c subsystem spec
>>>>> - check number of actually transferred bytes (via USB) everywehere
>>>>> - fix/improve debug messages
>>>>> - improve code comments
>>>>>
>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>>
>>>>
>>>>> @@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter
>>>>> *i2c_adap,
>>>>>             dprintk2(2, "%s %s addr=%x len=%d:",
>>>>>                  (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>>>>>                  i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
>>>>> +        if (addr > 0xff) {
>>>>> +            dprintk2(2, " ERROR: 10 bit addresses not supported\n");
>>>>> +            return -EOPNOTSUPP;
>>>>> +        }
>>>>
>>>> There is own flag for 10bit I2C address. Use it (and likely not
>>>> compare at all addr validly like that). This kind of address
>>>> validation check is quite unnecessary - and after all if it is wanted
>>>> then correct place is somewhere in I2C routines.
>>>
>>> Well, to be 100% sure and strict, we should check both, the flag and the
>>> actual address.
>>> We support 7 bit addresses only, no matter which i2c algo is used. So
>>> doing the address check in each i2c routine seems to be unnecessary code
>>> duplication to me ?
>>
>> I will repeat me, I see it overkill to check address correctness. And
>> as I said, that one is general validly could be done easily in I2C
>> core - so why the hell you wish make it just only for em28xx ?
>>
>> I am quite sure if that kind of address validness are saw important
>> they are already implemented by I2C core.
>>
>> Make patch for I2C which does that address validation against client
>> 10BIT flag and sent it to the mailing list for discussion.
>
> The I2C core doesn't know about the capabilities of the adapter.
> Hence it doesn't know if ten bit addresses will work (the same as with
> the message size constraints).
> All it does ist to check the client for I2C_CLIENT_TEN && addr > 0x7f
> once, when it is instanciated with a call to i2c_new_device().
> But we don't use this function in em28xx and the same applies to many
> other drivers as well.
> Apart from that, the client address and flags can change anytime later
> (e.g. when probing devices).

yes, it does not need to know if adapter supports 10 bit or not, or how 
many bytes adapter could send at once. It is up to adapter to check those.

But it could check if client tries to send using invalid address (client 
says it is 7BIT, but address is 10BIT), just situation you are adding to 
em28xx adapter.

If you are worried flags and address could change during operation, I2C 
core could check it too. Every driver I have seen are using I2C routines 
to send messages, and if there is check lets say eg. inside 
i2c_transfer() then it benefits all the others than em28xx. That is NOT 
em28xx *only* issue, it is common for all of our drivers. As it is 
common, adding check for each driver sounds wrong. General check should 
be done in general level, and hw specific issues are for driver.


> But if you hate the check, I can kick it out.
> The risk that it will cause any problems in practice is small.

Mostly, I am trying to explain you are trying to add that check to wrong 
place. Need of whole check is another issue.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Mauro Carvalho Chehab]

Em Sat, 15 Dec 2012 17:25:55 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 15.12.2012 14:46, schrieb Antti Palosaari:
> > On 12/15/2012 03:01 PM, Frank Schäfer wrote:
> >> Am 14.12.2012 18:03, schrieb Antti Palosaari:
> >>> On 12/14/2012 06:28 PM, Frank Schäfer wrote:
> >>>> - check i2c slave address range (only 7 bit addresses supported)
> >>>> - do not pass USB specific error codes to userspace/i2c-subsystem
> >>>> - unify the returned error codes and make them compliant with
> >>>>     the i2c subsystem spec
> >>>> - check number of actually transferred bytes (via USB) everywehere
> >>>> - fix/improve debug messages
> >>>> - improve code comments
> >>>>
> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>
> >>>
> >>>> @@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter
> >>>> *i2c_adap,
> >>>>            dprintk2(2, "%s %s addr=%x len=%d:",
> >>>>                 (msgs[i].flags & I2C_M_RD) ? "read" : "write",
> >>>>                 i == num - 1 ? "stop" : "nonstop", addr, msgs[i].len);
> >>>> +        if (addr > 0xff) {
> >>>> +            dprintk2(2, " ERROR: 10 bit addresses not supported\n");
> >>>> +            return -EOPNOTSUPP;
> >>>> +        }
> >>>
> >>> There is own flag for 10bit I2C address. Use it (and likely not
> >>> compare at all addr validly like that). This kind of address
> >>> validation check is quite unnecessary - and after all if it is wanted
> >>> then correct place is somewhere in I2C routines.
> >>
> >> Well, to be 100% sure and strict, we should check both, the flag and the
> >> actual address.
> >> We support 7 bit addresses only, no matter which i2c algo is used. So
> >> doing the address check in each i2c routine seems to be unnecessary code
> >> duplication to me ?
> >
> > I will repeat me, I see it overkill to check address correctness. And
> > as I said, that one is general validly could be done easily in I2C
> > core - so why the hell you wish make it just only for em28xx ?
> >
> > I am quite sure if that kind of address validness are saw important
> > they are already implemented by I2C core.
> >
> > Make patch for I2C which does that address validation against client
> > 10BIT flag and sent it to the mailing list for discussion.
> 
> The I2C core doesn't know about the capabilities of the adapter.
> Hence it doesn't know if ten bit addresses will work (the same as with
> the message size constraints).
> All it does ist to check the client for I2C_CLIENT_TEN && addr > 0x7f
> once, when it is instanciated with a call to i2c_new_device().
> But we don't use this function in em28xx and the same applies to many
> other drivers as well.
> Apart from that, the client address and flags can change anytime later
> (e.g. when probing devices).
> 
> But if you hate the check, I can kick it out.
> The risk that it will cause any problems in practice is small.

(c/c I2C maintainer)

I agree with Antti: instead of patching tons of drivers, the better is to
put such check inside the I2C core, as there are very few (if any) I2C
drivers with 10bit addresses.

> 
> Regards,
> Frank
> 
> >
> >> BTW: with the em28xx algorithm, the i2c address is transferred as 16 bit
> >> value. So 10 bit addresses COULD work in theory... ;)
> >
> > Could be, but I think 10bit is never used in real life.
> >
> > regards
> > Antti
> >
> 

Regards,
Mauro

Re: [PATCH 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Frank Schäfer]

Am 15.12.2012 18:16, schrieb Antti Palosaari:
> On 12/15/2012 06:25 PM, Frank Schäfer wrote:
>> Am 15.12.2012 14:46, schrieb Antti Palosaari:
>>> On 12/15/2012 03:01 PM, Frank Schäfer wrote:
>>>> Am 14.12.2012 18:03, schrieb Antti Palosaari:
>>>>> On 12/14/2012 06:28 PM, Frank Schäfer wrote:
>>>>>> - check i2c slave address range (only 7 bit addresses supported)
>>>>>> - do not pass USB specific error codes to userspace/i2c-subsystem
>>>>>> - unify the returned error codes and make them compliant with
>>>>>>      the i2c subsystem spec
>>>>>> - check number of actually transferred bytes (via USB) everywehere
>>>>>> - fix/improve debug messages
>>>>>> - improve code comments
>>>>>>
>>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>>>
>>>>>
>>>>>> @@ -244,16 +294,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter
>>>>>> *i2c_adap,
>>>>>>             dprintk2(2, "%s %s addr=%x len=%d:",
>>>>>>                  (msgs[i].flags & I2C_M_RD) ? "read" : "write",
>>>>>>                  i == num - 1 ? "stop" : "nonstop", addr,
>>>>>> msgs[i].len);
>>>>>> +        if (addr > 0xff) {
>>>>>> +            dprintk2(2, " ERROR: 10 bit addresses not
>>>>>> supported\n");
>>>>>> +            return -EOPNOTSUPP;
>>>>>> +        }
>>>>>
>>>>> There is own flag for 10bit I2C address. Use it (and likely not
>>>>> compare at all addr validly like that). This kind of address
>>>>> validation check is quite unnecessary - and after all if it is wanted
>>>>> then correct place is somewhere in I2C routines.
>>>>
>>>> Well, to be 100% sure and strict, we should check both, the flag
>>>> and the
>>>> actual address.
>>>> We support 7 bit addresses only, no matter which i2c algo is used. So
>>>> doing the address check in each i2c routine seems to be unnecessary
>>>> code
>>>> duplication to me ?
>>>
>>> I will repeat me, I see it overkill to check address correctness. And
>>> as I said, that one is general validly could be done easily in I2C
>>> core - so why the hell you wish make it just only for em28xx ?
>>>
>>> I am quite sure if that kind of address validness are saw important
>>> they are already implemented by I2C core.
>>>
>>> Make patch for I2C which does that address validation against client
>>> 10BIT flag and sent it to the mailing list for discussion.
>>
>> The I2C core doesn't know about the capabilities of the adapter.
>> Hence it doesn't know if ten bit addresses will work (the same as with
>> the message size constraints).
>> All it does ist to check the client for I2C_CLIENT_TEN && addr > 0x7f
>> once, when it is instanciated with a call to i2c_new_device().
>> But we don't use this function in em28xx and the same applies to many
>> other drivers as well.
>> Apart from that, the client address and flags can change anytime later
>> (e.g. when probing devices).
>
> yes, it does not need to know if adapter supports 10 bit or not, or
> how many bytes adapter could send at once. It is up to adapter to
> check those.

master_xfer() fcn _is_ the adpater.
You are confusing i2c adapter and client driver code here.

>
> But it could check if client tries to send using invalid address
> (client says it is 7BIT, but address is 10BIT), just situation you are
> adding to em28xx adapter.
>
> If you are worried flags and address could change during operation,
> I2C core could check it too.

Feel free to send patches.

> Every driver I have seen are using I2C routines to send messages, and
> if there is check lets say eg. inside i2c_transfer() then it benefits
> all the others than em28xx.

I agree.

> That is NOT em28xx *only* issue, it is common for all of our drivers. 

Sure.

> As it is common, adding check for each driver sounds wrong. General
> check should be done in general level, and hw specific issues are for
> driver.

Although I wouldn't call it wrong, I agree.
Jean might shed some light on this.

Regards,
Frank