[PATCH v2 0/5] em28xx: add support for the em2765 bridge

[PATCH v2 0/5] em28xx: add support for the em2765 bridge

[Frank Schäfer]

This patch series adds basic support for the em25xx/276x/7x/8x camera bridges.
These devices differ from the em2710/2750 and em28xx bridges in several points:
1) a second i2c bus is provided which has to be accessed with a different 
   read/write algorithm (=> patch 1)
2) a different frame data format is used (=> patch 3)
3) additional output formats (e.g. mpeg) are provided. This patch series does
   not (yet) add support for them, but it fixes the output format selection 
   for these bridges (the current code sets bit 5 of the output format register,
   which has a different meaning for the other bridges and breaks capturing
   with em25xx family sdevices). (=> patch 4)
4) registers 0x34+0x35 (VBI_START_H/V for em28xx devices) are used for a 
   different (unknown) purpose. This needs to be investigated further (could be 
   zooming, cropping, image statistics or AWB/AE window selection).
   At normal operation, these registers are set to capturing (input) 
   width/height / 16. (=> patch 5)

Patch 2 add the chip id of the em2765 as found in the "SpeedLink Vicious And 
Devine Laplace" webcam. The changes have also been tested with this device.

Changes since v1:
- rebased on the recent em28xx i2c bus changes (real support for 2 busses)
- moved i2c algorithm depending transfer function calls to separate functions

Frank Schäfer (5):
  em28xx: add support for em25xx i2c bus B read/write/check device
    operations
  em28xx: add chip id of the em2765
  em28xx: add support for em25xx/em276x/em277x/em278x frame data
    processing
  em28xx: make em28xx_set_outfmt() working with EM25xx family bridges
  em28xx: write output frame resolution to regs 0x34+0x35 for em25xx
    family bridges

 drivers/media/usb/em28xx/em28xx-cards.c |   17 ++-
 drivers/media/usb/em28xx/em28xx-core.c  |   27 +++-
 drivers/media/usb/em28xx/em28xx-i2c.c   |  229 +++++++++++++++++++++++++------
 drivers/media/usb/em28xx/em28xx-reg.h   |    7 +
 drivers/media/usb/em28xx/em28xx-video.c |   72 +++++++++-
 drivers/media/usb/em28xx/em28xx.h       |   11 +-
 6 Dateien geändert, 315 Zeilen hinzugefügt(+), 48 Zeilen entfernt(-)

-- 
1.7.10.4

[PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Frank Schäfer]

The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm
for i2c communication with the sensor, which is connected to a second i2c bus.

We don't know yet how to find out which devices support/use it.
It's very likely used by all em25xx and em276x+ bridges.
Tests with other em28xx chips (em2820, em2882/em2883) show, that this
algorithm always succeeds there although no slave device is connected.

The algorithm likely also works for real i2c client devices (OV2640 uses SCCB),
because the Windows driver seems to use it for probing Samsung and Kodak
sensors.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-cards.c |    8 +-
 drivers/media/usb/em28xx/em28xx-i2c.c   |  229 +++++++++++++++++++++++++------
 drivers/media/usb/em28xx/em28xx.h       |   10 +-
 3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index cb7cdd3..033b6cb 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 	rt_mutex_init(&dev->i2c_bus_lock);
 
 	/* register i2c bus 0 */
-	retval = em28xx_i2c_register(dev, 0);
+	if (dev->board.is_em2800)
+		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM2800);
+	else
+		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM28XX);
 	if (retval < 0) {
 		em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
 			__func__, retval);
 		goto unregister_dev;
 	}
 
+	/* register i2c bus 1 */
 	if (dev->def_i2c_bus) {
-		retval = em28xx_i2c_register(dev, 1);
+		retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
 		if (retval < 0) {
 			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
 				__func__, retval);
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 9e2fa41..ab14ac3 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -5,6 +5,7 @@
 		      Markus Rechberger <mrechberger@gmail.com>
 		      Mauro Carvalho Chehab <mchehab@infradead.org>
 		      Sascha Sommer <saschasommer@freenet.de>
+   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
 }
 
 /*
+ * em25xx_bus_B_send_bytes
+ * write bytes to the i2c device
+ */
+static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
+				   u16 len)
+{
+	int ret;
+
+	if (len < 1 || len > 64)
+		return -EOPNOTSUPP;
+	/* NOTE: limited by the USB ctrl message constraints
+	 * Zero length reads always succeed, even if no device is connected */
+
+	/* Set register and write value */
+	ret = dev->em28xx_write_regs_req(dev, 0x06, addr, buf, len);
+	/* NOTE:
+	 * 0 byte writes always succeed, even if no device is connected. */
+	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;
+		}
+	}
+	/* Check success */
+	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
+	/* NOTE: the only error we've seen so far is
+	 * 0x01 when the slave device is not present */
+	if (ret == 0x00) {
+		return len;
+	} else if (ret > 0) {
+		return -ENODEV;
+	}
+
+	return ret;
+	/* NOTE: With chips which do not support this operation,
+	 * it seems to succeed ALWAYS ! (even if no device connected) */
+}
+
+/*
+ * em25xx_bus_B_recv_bytes
+ * read bytes from the i2c device
+ */
+static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
+				   u16 len)
+{
+	int ret;
+
+	if (len < 1 || len > 64)
+		return -EOPNOTSUPP;
+	/* NOTE: limited by the USB ctrl message constraints
+	 * Zero length reads always succeed, even if no device is connected */
+
+	/* Read value */
+	ret = dev->em28xx_read_reg_req_len(dev, 0x06, addr, buf, len);
+	/* NOTE:
+	 * 0 byte reads always succeed, even if no device is connected. */
+	if (ret < 0) {
+		em28xx_warn("reading from i2c device at 0x%x failed (error=%i)\n",
+			    addr, ret);
+		return ret;
+	}
+	/* NOTE: some devices with two i2c busses have the bad habit to return 0
+	 * 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
+	 * spamming the system log on device probing and do nothing here.
+	 */
+
+	/* Check success */
+	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
+	/* NOTE: the only error we've seen so far is
+	 * 0x01 when the slave device is not present */
+	if (ret == 0x00) {
+		return len;
+	} else if (ret > 0) {
+		return -ENODEV;
+	}
+
+	return ret;
+	/* NOTE: With chips which do not support this operation,
+	 * it seems to succeed ALWAYS ! (even if no device connected) */
+}
+
+/*
+ * em25xx_bus_B_check_for_device()
+ * check if there is a i2c device at the supplied address
+ */
+static int em25xx_bus_B_check_for_device(struct em28xx *dev, u16 addr)
+{
+	u8 buf;
+	int ret;
+
+	ret = em25xx_bus_B_recv_bytes(dev, addr, &buf, 1);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+	/* NOTE: With chips which do not support this operation,
+	 * it seems to succeed ALWAYS ! (even if no device connected) */
+}
+
+static inline int i2c_check_for_device(struct em28xx_i2c_bus *i2c_bus, u16 addr)
+{
+	struct em28xx *dev = i2c_bus->dev;
+	int rc = -EOPNOTSUPP;
+
+	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
+		rc = em28xx_i2c_check_for_device(dev, addr);
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
+		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 (i2c_debug)
+			printk(" no device\n");
+	}
+	return rc;
+}
+
+static inline int i2c_recv_bytes(struct em28xx_i2c_bus *i2c_bus,
+				 struct i2c_msg msg)
+{
+	struct em28xx *dev = i2c_bus->dev;
+	u16 addr = msg.addr << 1;
+	int byte, rc = -EOPNOTSUPP;
+
+	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
+		rc = em28xx_i2c_recv_bytes(dev, addr, msg.buf, msg.len);
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
+		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;
+}
+
+static inline int i2c_send_bytes(struct em28xx_i2c_bus *i2c_bus,
+				 struct i2c_msg msg, int stop)
+{
+	struct em28xx *dev = i2c_bus->dev;
+	u16 addr = msg.addr << 1;
+	int byte, 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) {
+		rc = em2800_i2c_send_bytes(dev, addr, msg.buf, msg.len);
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
+		rc = em25xx_bus_B_send_bytes(dev, addr, msg.buf, msg.len);
+	}
+	return rc;
+}
+
+/*
  * em28xx_i2c_xfer()
  * the main i2c transfer function
  */
@@ -283,7 +454,7 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 	struct em28xx_i2c_bus *i2c_bus = i2c_adap->algo_data;
 	struct em28xx *dev = i2c_bus->dev;
 	unsigned bus = i2c_bus->bus;
-	int addr, rc, i, byte;
+	int addr, rc, i;
 	u8 reg;
 
 	rc = rt_mutex_trylock(&dev->i2c_bus_lock);
@@ -291,7 +462,8 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 		return rc;
 
 	/* Switch I2C bus if needed */
-	if (bus != dev->cur_i2c_bus) {
+	if (bus != dev->cur_i2c_bus &&
+	    i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
 		if (bus == 1)
 			reg = EM2874_I2C_SECONDARY_BUS_SELECT;
 		else
@@ -314,45 +486,17 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 			       i == num - 1 ? "stop" : "nonstop",
 			       addr, msgs[i].len);
 		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);
+			rc = i2c_check_for_device(i2c_bus, addr);
 			if (rc == -ENODEV) {
-				if (i2c_debug)
-					printk(" no device\n");
 				rt_mutex_unlock(&dev->i2c_bus_lock);
 				return rc;
 			}
 		} else if (msgs[i].flags & I2C_M_RD) {
 			/* read bytes */
-			if (dev->board.is_em2800)
-				rc = em2800_i2c_recv_bytes(dev, addr,
-							   msgs[i].buf,
-							   msgs[i].len);
-			else
-				rc = em28xx_i2c_recv_bytes(dev, addr,
-							   msgs[i].buf,
-							   msgs[i].len);
-			if (i2c_debug) {
-				for (byte = 0; byte < msgs[i].len; byte++)
-					printk(" %02x", msgs[i].buf[byte]);
-			}
+			rc = i2c_recv_bytes(i2c_bus, msgs[i]);
 		} else {
 			/* write bytes */
-			if (i2c_debug) {
-				for (byte = 0; byte < msgs[i].len; byte++)
-					printk(" %02x", msgs[i].buf[byte]);
-			}
-			if (dev->board.is_em2800)
-				rc = em2800_i2c_send_bytes(dev, addr,
-							   msgs[i].buf,
-							   msgs[i].len);
-			else
-				rc = em28xx_i2c_send_bytes(dev, addr,
-							   msgs[i].buf,
-							   msgs[i].len,
-							   i == num - 1);
+			rc = i2c_send_bytes(i2c_bus, msgs[i], i == num - 1);
 		}
 		if (rc < 0) {
 			if (i2c_debug)
@@ -622,12 +766,17 @@ error:
 static u32 functionality(struct i2c_adapter *i2c_adap)
 {
 	struct em28xx_i2c_bus *i2c_bus = i2c_adap->algo_data;
-	struct em28xx *dev = i2c_bus->dev;
 
-	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;
+	if ((i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) ||
+	    (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)) {
+		return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800)  {
+		return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL) &
+			~I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
+	}
+
+	WARN(1, "Unknown i2c bus algorithm.\n");
+	return 0;
 }
 
 static struct i2c_algorithm em28xx_algo = {
@@ -701,7 +850,8 @@ void em28xx_do_i2c_scan(struct em28xx *dev, unsigned bus)
  * em28xx_i2c_register()
  * register i2c bus
  */
-int em28xx_i2c_register(struct em28xx *dev, unsigned bus)
+int em28xx_i2c_register(struct em28xx *dev, unsigned bus,
+			enum em28xx_i2c_algo_type algo_type)
 {
 	int retval;
 
@@ -716,6 +866,7 @@ int em28xx_i2c_register(struct em28xx *dev, unsigned bus)
 	strcpy(dev->i2c_adap[bus].name, dev->name);
 
 	dev->i2c_bus[bus].bus = bus;
+	dev->i2c_bus[bus].algo_type = algo_type;
 	dev->i2c_bus[bus].dev = dev;
 	dev->i2c_adap[bus].algo_data = &dev->i2c_bus[bus];
 	i2c_set_adapdata(&dev->i2c_adap[bus], &dev->v4l2_dev);
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 4c667fd..aeee896 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -461,10 +461,17 @@ struct em28xx_fh {
 	enum v4l2_buf_type           type;
 };
 
+enum em28xx_i2c_algo_type {
+	EM28XX_I2C_ALGO_EM28XX = 0,
+	EM28XX_I2C_ALGO_EM2800,
+	EM28XX_I2C_ALGO_EM25XX_BUS_B,
+};
+
 struct em28xx_i2c_bus {
 	struct em28xx *dev;
 
 	unsigned bus;
+	enum em28xx_i2c_algo_type algo_type;
 };
 
 
@@ -651,7 +658,8 @@ struct em28xx_ops {
 
 /* Provided by em28xx-i2c.c */
 void em28xx_do_i2c_scan(struct em28xx *dev, unsigned bus);
-int  em28xx_i2c_register(struct em28xx *dev, unsigned bus);
+int  em28xx_i2c_register(struct em28xx *dev, unsigned bus,
+			 enum em28xx_i2c_algo_type algo_type);
 int  em28xx_i2c_unregister(struct em28xx *dev, unsigned bus);
 
 /* Provided by em28xx-core.c */
-- 
1.7.10.4

[PATCH v2 2/5] em28xx: add chip id of the em2765

[Frank Schäfer]

This chip can be found in the SpeedLink VAD Laplace webcam (1ae7:9003 and 1ae7:9004).

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

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 033b6cb..37c2173 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3041,6 +3041,12 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 		case CHIP_ID_EM2750:
 			chip_name = "em2750";
 			break;
+		case CHIP_ID_EM2765:
+			chip_name = "em2765";
+			dev->wait_after_write = 0;
+			dev->is_em25xx = 1;
+			dev->eeprom_addrwidth_16bit = 1;
+			break;
 		case CHIP_ID_EM2820:
 			chip_name = "em2710/2820";
 			break;
@@ -3151,7 +3157,10 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 
 	/* register i2c bus 1 */
 	if (dev->def_i2c_bus) {
-		retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
+		if (dev->is_em25xx)
+			retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM25XX_BUS_B);
+		else
+			retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
 		if (retval < 0) {
 			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
 				__func__, retval);
diff --git a/drivers/media/usb/em28xx/em28xx-reg.h b/drivers/media/usb/em28xx/em28xx-reg.h
index 8fd3c7f..1b0ecd6 100644
--- a/drivers/media/usb/em28xx/em28xx-reg.h
+++ b/drivers/media/usb/em28xx/em28xx-reg.h
@@ -219,6 +219,7 @@ enum em28xx_chip_id {
 	CHIP_ID_EM2860 = 34,
 	CHIP_ID_EM2870 = 35,
 	CHIP_ID_EM2883 = 36,
+	CHIP_ID_EM2765 = 54,
 	CHIP_ID_EM2874 = 65,
 	CHIP_ID_EM2884 = 68,
 	CHIP_ID_EM28174 = 113,
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index aeee896..7be008f 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -482,6 +482,7 @@ struct em28xx {
 	int model;		/* index in the device_data struct */
 	int devno;		/* marks the number of this device */
 	enum em28xx_chip_id chip_id;
+	unsigned int is_em25xx:1;	/* em25xx/em276x/7x/8x family bridge */
 
 	unsigned char disconnected:1;	/* device has been diconnected */
 
-- 
1.7.10.4

[PATCH v2 3/5] em28xx: add support for em25xx/em276x/em277x/em278x frame data processing

[Frank Schäfer]

The em25xx/em276x/em277x/em278x frame data format is different to the one used
by the em2710/em2750/em28xx chips.
With the recent cleanups and reorganization of the frame data processing code it
can be easily extended to support these devices.

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

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index d585c19..c1bd18d 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -76,6 +76,16 @@ MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL");
 MODULE_VERSION(EM28XX_VERSION);
 
+
+#define EM25XX_FRMDATAHDR_BYTE1			0x02
+#define EM25XX_FRMDATAHDR_BYTE2_STILL_IMAGE	0x20
+#define EM25XX_FRMDATAHDR_BYTE2_FRAME_END	0x02
+#define EM25XX_FRMDATAHDR_BYTE2_FRAME_ID	0x01
+#define EM25XX_FRMDATAHDR_BYTE2_MASK	(EM25XX_FRMDATAHDR_BYTE2_STILL_IMAGE | \
+					 EM25XX_FRMDATAHDR_BYTE2_FRAME_END |   \
+					 EM25XX_FRMDATAHDR_BYTE2_FRAME_ID)
+
+
 static unsigned int video_nr[] = {[0 ... (EM28XX_MAXBOARDS - 1)] = -1U };
 static unsigned int vbi_nr[]   = {[0 ... (EM28XX_MAXBOARDS - 1)] = -1U };
 static unsigned int radio_nr[] = {[0 ... (EM28XX_MAXBOARDS - 1)] = -1U };
@@ -408,6 +418,62 @@ static inline void process_frame_data_em28xx(struct em28xx *dev,
 		em28xx_copy_video(dev, buf, data_pkt, data_len);
 }
 
+/*
+ * Process data packet according to the em25xx/em276x/7x/8x frame data format
+ */
+static inline void process_frame_data_em25xx(struct em28xx *dev,
+					     unsigned char *data_pkt,
+					     unsigned int  data_len)
+{
+	struct em28xx_buffer    *buf = dev->usb_ctl.vid_buf;
+	struct em28xx_dmaqueue  *dmaq = &dev->vidq;
+	bool frame_end = 0;
+
+	/* Check for header */
+	/* NOTE: at least with bulk transfers, only the first packet
+	 * has a header and has always set the FRAME_END bit         */
+	if (data_len >= 2) {	/* em25xx header is only 2 bytes long */
+		if ((data_pkt[0] == EM25XX_FRMDATAHDR_BYTE1) &&
+		    ((data_pkt[1] & ~EM25XX_FRMDATAHDR_BYTE2_MASK) == 0x00)) {
+			dev->top_field = !(data_pkt[1] &
+					   EM25XX_FRMDATAHDR_BYTE2_FRAME_ID);
+			frame_end = data_pkt[1] &
+				    EM25XX_FRMDATAHDR_BYTE2_FRAME_END;
+			data_pkt += 2;
+			data_len -= 2;
+		}
+
+		/* Finish field and prepare next (BULK only) */
+		if (dev->analog_xfer_bulk && frame_end) {
+			buf = finish_field_prepare_next(dev, buf, dmaq);
+			dev->usb_ctl.vid_buf = buf;
+		}
+		/* NOTE: in ISOC mode when a new frame starts and buf==NULL,
+		 * we COULD already prepare a buffer here to avoid skipping the
+		 * first frame.
+		 */
+	}
+
+	/* Copy data */
+	if (buf != NULL && data_len > 0)
+		em28xx_copy_video(dev, buf, data_pkt, data_len);
+
+	/* Finish frame (ISOC only) => avoids lag of 1 frame */
+	if (!dev->analog_xfer_bulk && frame_end) {
+		buf = finish_field_prepare_next(dev, buf, dmaq);
+		dev->usb_ctl.vid_buf = buf;
+	}
+
+	/* NOTE: Tested with USB bulk transfers only !
+	 * The wording in the datasheet suggests that isoc might work different.
+	 * The current code assumes that with isoc transfers each packet has a
+	 * header like with the other em28xx devices.
+	 */
+	/* NOTE: Support for interlaced mode is pure theory. It has not been
+	 * tested and it is unknown if these devices actually support it. */
+	/* NOTE: No VBI support yet (these chips likely do not support VBI). */
+}
+
 /* Processes and copies the URB data content (video and VBI data) */
 static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 {
@@ -460,7 +526,11 @@ static inline int em28xx_urb_data_copy(struct em28xx *dev, struct urb *urb)
 			continue;
 		}
 
-		process_frame_data_em28xx(dev, usb_data_pkt, usb_data_len);
+		if (dev->is_em25xx)
+			process_frame_data_em25xx(dev, usb_data_pkt, usb_data_len);
+		else
+			process_frame_data_em28xx(dev, usb_data_pkt, usb_data_len);
+
 	}
 	return 1;
 }
-- 
1.7.10.4

[PATCH v2 4/5] em28xx: make em28xx_set_outfmt() working with EM25xx family bridges

[Frank Schäfer]

Streaming doesn't work with the EM2765 if bit 5 of the output format register
0x27 is set.
It's actually not clear if really has to be set for the other chips, but for
now let's keep it to avoid regressions and add a comment to the code.

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

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index b2dcb3d..7b9f76b 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -697,12 +697,22 @@ int em28xx_vbi_supported(struct em28xx *dev)
 int em28xx_set_outfmt(struct em28xx *dev)
 {
 	int ret;
-	u8 vinctrl;
-
-	ret = em28xx_write_reg_bits(dev, EM28XX_R27_OUTFMT,
-				dev->format->reg | 0x20, 0xff);
+	u8 fmt, vinctrl;
+
+	fmt = dev->format->reg;
+	if (!dev->is_em25xx)
+		fmt |= 0x20;
+	/* NOTE: it's not clear if this is really needed !
+	 * The datasheets say bit 5 is a reserved bit and devices seem to work
+	 * fine without it. But the Windows driver sets it for em2710/50+em28xx
+	 * devices and we've always been setting it, too.
+	 *
+	 * em2765 (em25xx, em276x/7x/8x ?) devices do NOT work with this bit set,
+	 * it's likely used for an additional (compressed ?) format there.
+	 */
+	ret = em28xx_write_reg(dev, EM28XX_R27_OUTFMT, fmt);
 	if (ret < 0)
-			return ret;
+		return ret;
 
 	ret = em28xx_write_reg(dev, EM28XX_R10_VINMODE, dev->vinmode);
 	if (ret < 0)
-- 
1.7.10.4

[PATCH v2 5/5] em28xx: write output frame resolution to regs 0x34+0x35 for em25xx family bridges

[Frank Schäfer]

The Windows driver writes the output resolution to registers 0x34 (width / 16)
and 0x35 (height / 16) always.
We don't know yet what these registers are used for.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c |    7 +++++++
 drivers/media/usb/em28xx/em28xx-reg.h  |    6 ++++++
 2 Dateien geändert, 13 Zeilen hinzugefügt(+)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index 7b9f76b..0ce6b0f 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -766,6 +766,13 @@ static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
 	em28xx_write_regs(dev, EM28XX_R1E_CWIDTH, &cwidth, 1);
 	em28xx_write_regs(dev, EM28XX_R1F_CHEIGHT, &cheight, 1);
 	em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
+
+	if (dev->is_em25xx) {
+		em28xx_write_reg(dev, 0x34, width >> 4);
+		em28xx_write_reg(dev, 0x35, height >> 4);
+	}
+	/* FIXME: function/meaning of these registers ? */
+	/* FIXME: align width+height to multiples of 4 ?! */
 }
 
 static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v)
diff --git a/drivers/media/usb/em28xx/em28xx-reg.h b/drivers/media/usb/em28xx/em28xx-reg.h
index 1b0ecd6..e08982a 100644
--- a/drivers/media/usb/em28xx/em28xx-reg.h
+++ b/drivers/media/usb/em28xx/em28xx-reg.h
@@ -167,6 +167,12 @@
 
 #define EM28XX_R34_VBI_START_H	0x34
 #define EM28XX_R35_VBI_START_V	0x35
+/* NOTE: the EM276x (and EM25xx, EM277x/8x ?) (camera bridges) use these
+ * registers for a different unknown purpose.
+ *   => register 0x34 is set to capture width / 16
+ *   => register 0x35 is set to capture height / 16
+ */
+
 #define EM28XX_R36_VBI_WIDTH	0x36
 #define EM28XX_R37_VBI_HEIGHT	0x37
 
-- 
1.7.10.4

Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Mauro Carvalho Chehab]

Em Sat, 23 Mar 2013 18:27:08 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm
> for i2c communication with the sensor, which is connected to a second i2c bus.
> 
> We don't know yet how to find out which devices support/use it.
> It's very likely used by all em25xx and em276x+ bridges.
> Tests with other em28xx chips (em2820, em2882/em2883) show, that this
> algorithm always succeeds there although no slave device is connected.
> 
> The algorithm likely also works for real i2c client devices (OV2640 uses SCCB),
> because the Windows driver seems to use it for probing Samsung and Kodak
> sensors.
> 
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-cards.c |    8 +-
>  drivers/media/usb/em28xx/em28xx-i2c.c   |  229 +++++++++++++++++++++++++------
>  drivers/media/usb/em28xx/em28xx.h       |   10 +-
>  3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index cb7cdd3..033b6cb 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>  	rt_mutex_init(&dev->i2c_bus_lock);
>  
>  	/* register i2c bus 0 */
> -	retval = em28xx_i2c_register(dev, 0);
> +	if (dev->board.is_em2800)
> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM2800);
> +	else
> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM28XX);
>  	if (retval < 0) {
>  		em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
>  			__func__, retval);
>  		goto unregister_dev;
>  	}
>  
> +	/* register i2c bus 1 */
>  	if (dev->def_i2c_bus) {
> -		retval = em28xx_i2c_register(dev, 1);
> +		retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
>  		if (retval < 0) {
>  			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
>  				__func__, retval);
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 9e2fa41..ab14ac3 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -5,6 +5,7 @@
>  		      Markus Rechberger <mrechberger@gmail.com>
>  		      Mauro Carvalho Chehab <mchehab@infradead.org>
>  		      Sascha Sommer <saschasommer@freenet.de>
> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
>  
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
> @@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
>  }
>  
>  /*
> + * em25xx_bus_B_send_bytes
> + * write bytes to the i2c device
> + */
> +static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> +				   u16 len)
> +{
> +	int ret;
> +
> +	if (len < 1 || len > 64)
> +		return -EOPNOTSUPP;
> +	/* NOTE: limited by the USB ctrl message constraints
> +	 * Zero length reads always succeed, even if no device is connected */
> +
> +	/* Set register and write value */
> +	ret = dev->em28xx_write_regs_req(dev, 0x06, addr, buf, len);
> +	/* NOTE:
> +	 * 0 byte writes always succeed, even if no device is connected. */
> +	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;
> +		}
> +	}
> +	/* Check success */
> +	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
> +	/* NOTE: the only error we've seen so far is
> +	 * 0x01 when the slave device is not present */
> +	if (ret == 0x00) {
> +		return len;
> +	} else if (ret > 0) {
> +		return -ENODEV;
> +	}
> +
> +	return ret;
> +	/* NOTE: With chips which do not support this operation,
> +	 * it seems to succeed ALWAYS ! (even if no device connected) */
> +}
> +
> +/*
> + * em25xx_bus_B_recv_bytes
> + * read bytes from the i2c device
> + */
> +static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> +				   u16 len)
> +{
> +	int ret;
> +
> +	if (len < 1 || len > 64)
> +		return -EOPNOTSUPP;
> +	/* NOTE: limited by the USB ctrl message constraints
> +	 * Zero length reads always succeed, even if no device is connected */


Please stick with Kernel's coding style, as described on
Documentation/CodingStyle and on the common practices.

Multi-line comments are like:
	/*
	 * Foo
	 * bar
	 */

There are also a bunch of scripts/checkpatch.pl complains for this patch: 

WARNING: please, no spaces at the start of a line
#69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>$

WARNING: space prohibited between function name and open parenthesis '('
#69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>

WARNING: Avoid CamelCase: <Copyright>
#69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>

WARNING: Avoid CamelCase: <Frank>
#69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>

WARNING: Avoid CamelCase: <Sch>
#69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
+   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>

WARNING: quoted string split across lines
#97: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:298:
+			em28xx_warn("writing to i2c device at 0x%x failed "
+				    "(error=%i)\n", addr, ret);

WARNING: quoted string split across lines
#101: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:302:
+			em28xx_warn("%i bytes write to i2c device at 0x%x "
+				    "requested, but %i bytes written\n",

WARNING: braces {} are not necessary for any arm of this statement
#110: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:311:
+	if (ret == 0x00) {
[...]
+	} else if (ret > 0) {
[...]

WARNING: braces {} are not necessary for any arm of this statement
#156: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:357:
+	if (ret == 0x00) {
[...]
+	} else if (ret > 0) {
[...]

WARNING: braces {} are not necessary for any arm of this statement
#190: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:391:
+	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
[...]
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
[...]
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
[...]

WARNING: printk() should include KERN_ facility level
#199: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:400:
+			printk(" no device\n");

WARNING: braces {} are not necessary for any arm of this statement
#211: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:412:
+	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
[...]
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
[...]
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
[...]

WARNING: printk() should include KERN_ facility level
#220: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:421:
+			printk(" %02x", msg.buf[byte]);

WARNING: braces {} are not necessary for any arm of this statement
#236: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:437:
+	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
[...]
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
[...]
+	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
[...]

total: 0 errors, 14 warnings, 333 lines checked

Your patch has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

PS.: I'll write a separate email if I find any non-coding style issue on
this patch series. Won't comment anymore about coding style, as I'm
assuming that you'll be fixing it on the other patches of this series
if needed.

Regards,
Mauro

Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Mauro Carvalho Chehab]

Em Sat, 23 Mar 2013 18:27:08 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm
> for i2c communication with the sensor, which is connected to a second i2c bus.
> 
> We don't know yet how to find out which devices support/use it.
> It's very likely used by all em25xx and em276x+ bridges.
> Tests with other em28xx chips (em2820, em2882/em2883) show, that this
> algorithm always succeeds there although no slave device is connected.
> 
> The algorithm likely also works for real i2c client devices (OV2640 uses SCCB),
> because the Windows driver seems to use it for probing Samsung and Kodak
> sensors.
> 
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-cards.c |    8 +-
>  drivers/media/usb/em28xx/em28xx-i2c.c   |  229 +++++++++++++++++++++++++------
>  drivers/media/usb/em28xx/em28xx.h       |   10 +-
>  3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index cb7cdd3..033b6cb 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>  	rt_mutex_init(&dev->i2c_bus_lock);
>  
>  	/* register i2c bus 0 */
> -	retval = em28xx_i2c_register(dev, 0);
> +	if (dev->board.is_em2800)
> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM2800);
> +	else
> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM28XX);
>  	if (retval < 0) {
>  		em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
>  			__func__, retval);
>  		goto unregister_dev;
>  	}
>  
> +	/* register i2c bus 1 */
>  	if (dev->def_i2c_bus) {
> -		retval = em28xx_i2c_register(dev, 1);
> +		retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
>  		if (retval < 0) {
>  			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
>  				__func__, retval);
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 9e2fa41..ab14ac3 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -5,6 +5,7 @@
>  		      Markus Rechberger <mrechberger@gmail.com>
>  		      Mauro Carvalho Chehab <mchehab@infradead.org>
>  		      Sascha Sommer <saschasommer@freenet.de>
> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
>  
>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
> @@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
>  }
>  
>  /*
> + * em25xx_bus_B_send_bytes
> + * write bytes to the i2c device
> + */
> +static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> +				   u16 len)
> +{
> +	int ret;
> +
> +	if (len < 1 || len > 64)
> +		return -EOPNOTSUPP;
> +	/* NOTE: limited by the USB ctrl message constraints
> +	 * Zero length reads always succeed, even if no device is connected */
> +
> +	/* Set register and write value */
> +	ret = dev->em28xx_write_regs_req(dev, 0x06, addr, buf, len);
> +	/* NOTE:
> +	 * 0 byte writes always succeed, even if no device is connected. */

You already noticed it on the previous note.

> +	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;
> +		}
> +	}
> +	/* Check success */
> +	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
> +	/* NOTE: the only error we've seen so far is
> +	 * 0x01 when the slave device is not present */
> +	if (ret == 0x00) {

	Please simplify. just use:
		if (!ret)

> +		return len;
> +	} else if (ret > 0) {
> +		return -ENODEV;
> +	}
> +
> +	return ret;
> +	/* NOTE: With chips which do not support this operation,
> +	 * it seems to succeed ALWAYS ! (even if no device connected) */

Sorry, but I didn't get what you're trying to explain here. What are those
em25xx chips that don't support this operation?

> +}
> +
> +/*
> + * em25xx_bus_B_recv_bytes
> + * read bytes from the i2c device
> + */
> +static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> +				   u16 len)
> +{
> +	int ret;
> +
> +	if (len < 1 || len > 64)
> +		return -EOPNOTSUPP;
> +	/* NOTE: limited by the USB ctrl message constraints
> +	 * Zero length reads always succeed, even if no device is connected */

You already explained about the zero length before.

> +
> +	/* Read value */
> +	ret = dev->em28xx_read_reg_req_len(dev, 0x06, addr, buf, len);
> +	/* NOTE:
> +	 * 0 byte reads always succeed, even if no device is connected. */

You're insistent on that, won't you? ;)

> +	if (ret < 0) {
> +		em28xx_warn("reading from i2c device at 0x%x failed (error=%i)\n",
> +			    addr, ret);
> +		return ret;
> +	}
> +	/* NOTE: some devices with two i2c busses have the bad habit to return 0
> +	 * 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
> +	 * spamming the system log on device probing and do nothing here.
> +	 */
> +
> +	/* Check success */
> +	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
> +	/* NOTE: the only error we've seen so far is
> +	 * 0x01 when the slave device is not present */
> +	if (ret == 0x00) {

Same as already noticed.

> +		return len;
> +	} else if (ret > 0) {
> +		return -ENODEV;
> +	}
> +
> +	return ret;
> +	/* NOTE: With chips which do not support this operation,
> +	 * it seems to succeed ALWAYS ! (even if no device connected) */

Again, didn't understand what you're meaning.

Regards,
Mauro

Re: [PATCH v2 5/5] em28xx: write output frame resolution to regs 0x34+0x35 for em25xx family bridges

[Mauro Carvalho Chehab]

Em Sat, 23 Mar 2013 18:27:12 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> The Windows driver writes the output resolution to registers 0x34 (width / 16)
> and 0x35 (height / 16) always.
> We don't know yet what these registers are used for.
> 
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-core.c |    7 +++++++
>  drivers/media/usb/em28xx/em28xx-reg.h  |    6 ++++++
>  2 Dateien geändert, 13 Zeilen hinzugefügt(+)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
> index 7b9f76b..0ce6b0f 100644
> --- a/drivers/media/usb/em28xx/em28xx-core.c
> +++ b/drivers/media/usb/em28xx/em28xx-core.c
> @@ -766,6 +766,13 @@ static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
>  	em28xx_write_regs(dev, EM28XX_R1E_CWIDTH, &cwidth, 1);
>  	em28xx_write_regs(dev, EM28XX_R1F_CHEIGHT, &cheight, 1);
>  	em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
> +
> +	if (dev->is_em25xx) {
> +		em28xx_write_reg(dev, 0x34, width >> 4);
> +		em28xx_write_reg(dev, 0x35, height >> 4);
> +	}
> +	/* FIXME: function/meaning of these registers ? */
> +	/* FIXME: */

Please move those comments to be _before_ the code you're commenting.

E. g. something like:

	if (dev->is_em25xx) {
		/*
		 * FIXME:
 		 *	- function/meaning of these registers are unknown;
		 *	- align width+height to multiples of 4 ?! 
		 */
		em28xx_write_reg(dev, 0x34, width >> 4);
		em28xx_write_reg(dev, 0x35, height >> 4);
	}


>  }
>  
>  static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v)
> diff --git a/drivers/media/usb/em28xx/em28xx-reg.h b/drivers/media/usb/em28xx/em28xx-reg.h
> index 1b0ecd6..e08982a 100644
> --- a/drivers/media/usb/em28xx/em28xx-reg.h
> +++ b/drivers/media/usb/em28xx/em28xx-reg.h
> @@ -167,6 +167,12 @@
>  
>  #define EM28XX_R34_VBI_START_H	0x34
>  #define EM28XX_R35_VBI_START_V	0x35
> +/* NOTE: the EM276x (and EM25xx, EM277x/8x ?) (camera bridges) use these
> + * registers for a different unknown purpose.
> + *   => register 0x34 is set to capture width / 16
> + *   => register 0x35 is set to capture height / 16
> + */
> +
>  #define EM28XX_R36_VBI_WIDTH	0x36
>  #define EM28XX_R37_VBI_HEIGHT	0x37
>  


-- 

Cheers,
Mauro

Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Frank Schäfer]

Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
> Em Sat, 23 Mar 2013 18:27:08 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm
>> for i2c communication with the sensor, which is connected to a second i2c bus.
>>
>> We don't know yet how to find out which devices support/use it.
>> It's very likely used by all em25xx and em276x+ bridges.
>> Tests with other em28xx chips (em2820, em2882/em2883) show, that this
>> algorithm always succeeds there although no slave device is connected.
>>
>> The algorithm likely also works for real i2c client devices (OV2640 uses SCCB),
>> because the Windows driver seems to use it for probing Samsung and Kodak
>> sensors.
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-cards.c |    8 +-
>>  drivers/media/usb/em28xx/em28xx-i2c.c   |  229 +++++++++++++++++++++++++------
>>  drivers/media/usb/em28xx/em28xx.h       |   10 +-
>>  3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>> index cb7cdd3..033b6cb 100644
>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>> @@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>>  	rt_mutex_init(&dev->i2c_bus_lock);
>>  
>>  	/* register i2c bus 0 */
>> -	retval = em28xx_i2c_register(dev, 0);
>> +	if (dev->board.is_em2800)
>> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM2800);
>> +	else
>> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM28XX);
>>  	if (retval < 0) {
>>  		em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
>>  			__func__, retval);
>>  		goto unregister_dev;
>>  	}
>>  
>> +	/* register i2c bus 1 */
>>  	if (dev->def_i2c_bus) {
>> -		retval = em28xx_i2c_register(dev, 1);
>> +		retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
>>  		if (retval < 0) {
>>  			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
>>  				__func__, retval);
>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>> index 9e2fa41..ab14ac3 100644
>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>> @@ -5,6 +5,7 @@
>>  		      Markus Rechberger <mrechberger@gmail.com>
>>  		      Mauro Carvalho Chehab <mchehab@infradead.org>
>>  		      Sascha Sommer <saschasommer@freenet.de>
>> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
>>  
>>     This program is free software; you can redistribute it and/or modify
>>     it under the terms of the GNU General Public License as published by
>> @@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
>>  }
>>  
>>  /*
>> + * em25xx_bus_B_send_bytes
>> + * write bytes to the i2c device
>> + */
>> +static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>> +				   u16 len)
>> +{
>> +	int ret;
>> +
>> +	if (len < 1 || len > 64)
>> +		return -EOPNOTSUPP;
>> +	/* NOTE: limited by the USB ctrl message constraints
>> +	 * Zero length reads always succeed, even if no device is connected */
>> +
>> +	/* Set register and write value */
>> +	ret = dev->em28xx_write_regs_req(dev, 0x06, addr, buf, len);
>> +	/* NOTE:
>> +	 * 0 byte writes always succeed, even if no device is connected. */
> You already noticed it on the previous note.

Yes. ;)

>
>> +	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;
>> +		}
>> +	}
>> +	/* Check success */
>> +	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
>> +	/* NOTE: the only error we've seen so far is
>> +	 * 0x01 when the slave device is not present */
>> +	if (ret == 0x00) {
> 	Please simplify. just use:
> 		if (!ret)

I would like to keep it as is because I think it better expresses the
purposes of this check. I also used 0x00 instead of 0 on purpose.
ret is a mixed value which is negative on errors and contains the data
bytes (0x00 to 0xff) on success.
Ok, in this specific case all other values are covered with a single
(ret > 0) check, but take a look at the comment and the em28xx-algo
functions where we check for 0x10, too.

>
>> +		return len;
>> +	} else if (ret > 0) {
>> +		return -ENODEV;
>> +	}
>> +
>> +	return ret;
>> +	/* NOTE: With chips which do not support this operation,
>> +	 * it seems to succeed ALWAYS ! (even if no device connected) */
> Sorry, but I didn't get what you're trying to explain here. What are those
> em25xx chips that don't support this operation?

Hmm... I don't know how to explain it better...
The thing is, that this algo _seems_ to work also (at least with some)
chips which actually don't support it (even if they don't provide a
second i2c bus).

>
>> +}
>> +
>> +/*
>> + * em25xx_bus_B_recv_bytes
>> + * read bytes from the i2c device
>> + */
>> +static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>> +				   u16 len)
>> +{
>> +	int ret;
>> +
>> +	if (len < 1 || len > 64)
>> +		return -EOPNOTSUPP;
>> +	/* NOTE: limited by the USB ctrl message constraints
>> +	 * Zero length reads always succeed, even if no device is connected */
> You already explained about the zero length before.

Yepp, but in another function. ;)

>> +
>> +	/* Read value */
>> +	ret = dev->em28xx_read_reg_req_len(dev, 0x06, addr, buf, len);
>> +	/* NOTE:
>> +	 * 0 byte reads always succeed, even if no device is connected. */
> You're insistent on that, won't you? ;)

Yeah, I like comments and I think there should be much more ! :)

But in this case I don't want to be insistent, I just want to make sure
that important issues are expalined at all relevant code places.
The alternative would be to reference comments at other places, which
IMHO doesn't work well.

Regards,
Frank

Re: [PATCH v2 5/5] em28xx: write output frame resolution to regs 0x34+0x35 for em25xx family bridges

[Frank Schäfer]

Am 24.03.2013 12:44, schrieb Mauro Carvalho Chehab:
> Em Sat, 23 Mar 2013 18:27:12 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> The Windows driver writes the output resolution to registers 0x34 (width / 16)
>> and 0x35 (height / 16) always.
>> We don't know yet what these registers are used for.
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-core.c |    7 +++++++
>>  drivers/media/usb/em28xx/em28xx-reg.h  |    6 ++++++
>>  2 Dateien geändert, 13 Zeilen hinzugefügt(+)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>> index 7b9f76b..0ce6b0f 100644
>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>> @@ -766,6 +766,13 @@ static void em28xx_capture_area_set(struct em28xx *dev, u8 hstart, u8 vstart,
>>  	em28xx_write_regs(dev, EM28XX_R1E_CWIDTH, &cwidth, 1);
>>  	em28xx_write_regs(dev, EM28XX_R1F_CHEIGHT, &cheight, 1);
>>  	em28xx_write_regs(dev, EM28XX_R1B_OFLOW, &overflow, 1);
>> +
>> +	if (dev->is_em25xx) {
>> +		em28xx_write_reg(dev, 0x34, width >> 4);
>> +		em28xx_write_reg(dev, 0x35, height >> 4);
>> +	}
>> +	/* FIXME: function/meaning of these registers ? */
>> +	/* FIXME: align width+height to multiples of 4 ?! */
> Please move those comments to be _before_ the code you're commenting.
>
> E. g. something like:
>
> 	if (dev->is_em25xx) {
> 		/*
> 		 * FIXME:
>  		 *	- function/meaning of these registers are unknown;
> 		 *	- align width+height to multiples of 4 ?! 
> 		 */
> 		em28xx_write_reg(dev, 0x34, width >> 4);
> 		em28xx_write_reg(dev, 0x35, height >> 4);
> 	}

Ok, no problem.

Frank

>
>>  }
>>  
>>  static int em28xx_scaler_set(struct em28xx *dev, u16 h, u16 v)
>> diff --git a/drivers/media/usb/em28xx/em28xx-reg.h b/drivers/media/usb/em28xx/em28xx-reg.h
>> index 1b0ecd6..e08982a 100644
>> --- a/drivers/media/usb/em28xx/em28xx-reg.h
>> +++ b/drivers/media/usb/em28xx/em28xx-reg.h
>> @@ -167,6 +167,12 @@
>>  
>>  #define EM28XX_R34_VBI_START_H	0x34
>>  #define EM28XX_R35_VBI_START_V	0x35
>> +/* NOTE: the EM276x (and EM25xx, EM277x/8x ?) (camera bridges) use these
>> + * registers for a different unknown purpose.
>> + *   => register 0x34 is set to capture width / 16
>> + *   => register 0x35 is set to capture height / 16
>> + */
>> +
>>  #define EM28XX_R36_VBI_WIDTH	0x36
>>  #define EM28XX_R37_VBI_HEIGHT	0x37
>>  
>

Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Frank Schäfer]

...

Am 24.03.2013 12:22, schrieb Mauro Carvalho Chehab:
> Please stick with Kernel's coding style, as described on
> Documentation/CodingStyle and on the common practices.
>
> Multi-line comments are like:
> 	/*
> 	 * Foo
> 	 * bar
> 	 */
>
> There are also a bunch of scripts/checkpatch.pl complains for this patch: 
>
> WARNING: please, no spaces at the start of a line
> #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>$
>
> WARNING: space prohibited between function name and open parenthesis '('
> #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
>
> WARNING: Avoid CamelCase: <Copyright>
> #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
>
> WARNING: Avoid CamelCase: <Frank>
> #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
>
> WARNING: Avoid CamelCase: <Sch>
> #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>

The "space prohibited between function name and open parenthesis" and
"CamelCase" warnings are pure nonsense.
The "spaces at start of a line thing" applies to the whole and
licences/copyright headers of the em28xx driver.
If you think we should change that - fine - but it really doesn't make
sense to do this as part of this patch series.


> WARNING: quoted string split across lines
> #97: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:298:
> +			em28xx_warn("writing to i2c device at 0x%x failed "
> +				    "(error=%i)\n", addr, ret);
>
> WARNING: quoted string split across lines
> #101: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:302:
> +			em28xx_warn("%i bytes write to i2c device at 0x%x "
> +				    "requested, but %i bytes written\n",

Yes, these two are discussible.
AFAIK, strings should not be split across lines to avoid breaking
grepping for strings.
In this case I decided for the "80 characters per line rule" instead,
because grepping for strings containing placeholders IMHO doesn't make
much sense.
We do exactly the same in the existing i2c functions.

> WARNING: braces {} are not necessary for any arm of this statement
> #110: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:311:
> +	if (ret == 0x00) {
> [...]
> +	} else if (ret > 0) {
> [...]
>
> WARNING: braces {} are not necessary for any arm of this statement
> #156: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:357:
> +	if (ret == 0x00) {
> [...]
> +	} else if (ret > 0) {
> [...]
>
> WARNING: braces {} are not necessary for any arm of this statement
> #190: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:391:
> +	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
> [...]
> +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
> [...]
> +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> [...]

The patch perfectly matches the kernel coding style rules here !?
What do you want me to change ?
Do you really think the code looks better without some of these braces ?

> WARNING: printk() should include KERN_ facility level
> #199: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:400:
> +			printk(" no device\n");

I only moved this piece of code around, but yes, that should really be
fixed !

> WARNING: braces {} are not necessary for any arm of this statement
> #211: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:412:
> +	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
> [...]
> +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
> [...]
> +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> [...]

See above.

> WARNING: printk() should include KERN_ facility level
> #220: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:421:
> +			printk(" %02x", msg.buf[byte]);

Same here, should indeed be fixed.

> WARNING: braces {} are not necessary for any arm of this statement
> #236: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:437:
> +	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
> [...]
> +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
> [...]
> +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> [...]

See above.

Except for the two printk warnings, please tell me which changes you
would like to see exactly.
I will send an updated version of this series then.

Regards,
Frank

> total: 0 errors, 14 warnings, 333 lines checked
>
> Your patch has style problems, please review.
>
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
> PS.: I'll write a separate email if I find any non-coding style issue on
> this patch series. Won't comment anymore about coding style, as I'm
> assuming that you'll be fixing it on the other patches of this series
> if needed.
>
> Regards,
> Mauro

Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Mauro Carvalho Chehab]

Em Sun, 24 Mar 2013 14:04:47 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> ...
> 
> Am 24.03.2013 12:22, schrieb Mauro Carvalho Chehab:
> > Please stick with Kernel's coding style, as described on
> > Documentation/CodingStyle and on the common practices.
> >
> > Multi-line comments are like:
> > 	/*
> > 	 * Foo
> > 	 * bar
> > 	 */
> >
> > There are also a bunch of scripts/checkpatch.pl complains for this patch: 
> >
> > WARNING: please, no spaces at the start of a line
> > #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> > +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>$
> >
> > WARNING: space prohibited between function name and open parenthesis '('
> > #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> > +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
> >
> > WARNING: Avoid CamelCase: <Copyright>
> > #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> > +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
> >
> > WARNING: Avoid CamelCase: <Frank>
> > #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> > +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
> >
> > WARNING: Avoid CamelCase: <Sch>
> > #69: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:8:
> > +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
> 
> The "space prohibited between function name and open parenthesis" and
> "CamelCase" warnings are pure nonsense.
> The "spaces at start of a line thing" applies to the whole and
> licences/copyright headers of the em28xx driver.
> If you think we should change that - fine - but it really doesn't make
> sense to do this as part of this patch series.

The above complaints are because checkpatch doesn't like comments like:

/*
 FOO
 */

As it should be, instead:

 /*
  * FOO
  */

You can safely ignore all the above. It is not worth to fix the comments
there.

> 
> 
> > WARNING: quoted string split across lines
> > #97: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:298:
> > +			em28xx_warn("writing to i2c device at 0x%x failed "
> > +				    "(error=%i)\n", addr, ret);
> >
> > WARNING: quoted string split across lines
> > #101: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:302:
> > +			em28xx_warn("%i bytes write to i2c device at 0x%x "
> > +				    "requested, but %i bytes written\n",
> 
> Yes, these two are discussible.
> AFAIK, strings should not be split across lines to avoid breaking
> grepping for strings.
> In this case I decided for the "80 characters per line rule" instead,
> because grepping for strings containing placeholders IMHO doesn't make
> much sense.
> We do exactly the same in the existing i2c functions.

The "not break strings" rule is stronger. When the code was written, 
80 cols were a mandatory rule (and there was no checkpatch.pl). After
lots of discussions (with took years), we're all set about two points:

	- the 80 cols is a soft limit;

	- strings should not be broken. Linus explicitly pointed on that
time that he doesn't want to see strings broken, as it makes harder to 
grep for the printed messages when needed.

> 
> > WARNING: braces {} are not necessary for any arm of this statement
> > #110: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:311:
> > +	if (ret == 0x00) {
> > [...]
> > +	} else if (ret > 0) {
> > [...]
> >
> > WARNING: braces {} are not necessary for any arm of this statement
> > #156: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:357:
> > +	if (ret == 0x00) {
> > [...]
> > +	} else if (ret > 0) {
> > [...]
> >
> > WARNING: braces {} are not necessary for any arm of this statement
> > #190: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:391:
> > +	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
> > [...]
> > +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
> > [...]
> > +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> > [...]
> 
> The patch perfectly matches the kernel coding style rules here !?
> What do you want me to change ?
> Do you really think the code looks better without some of these braces ?

For sure it looks better without the braces. Braces should be used only
if there are multiple lines at the if chain.

> > WARNING: printk() should include KERN_ facility level
> > #199: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:400:
> > +			printk(" no device\n");
> 
> I only moved this piece of code around, but yes, that should really be
> fixed !

Where you're just moving the code, no.

> 
> > WARNING: braces {} are not necessary for any arm of this statement
> > #211: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:412:
> > +	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
> > [...]
> > +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
> > [...]
> > +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> > [...]
> 
> See above.
> 
> > WARNING: printk() should include KERN_ facility level
> > #220: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:421:
> > +			printk(" %02x", msg.buf[byte]);
> 
> Same here, should indeed be fixed.
> 
> > WARNING: braces {} are not necessary for any arm of this statement
> > #236: FILE: drivers/media/usb/em28xx/em28xx-i2c.c:437:
> > +	if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM28XX) {
> > [...]
> > +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM2800) {
> > [...]
> > +	} else if (i2c_bus->algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> > [...]
> 
> See above.
> 
> Except for the two printk warnings, please tell me which changes you
> would like to see exactly.
> I will send an updated version of this series then.
> 
> Regards,
> Frank
> 
> > total: 0 errors, 14 warnings, 333 lines checked
> >
> > Your patch has style problems, please review.
> >
> > If any of these errors are false positives, please report
> > them to the maintainer, see CHECKPATCH in MAINTAINERS.
> >
> > PS.: I'll write a separate email if I find any non-coding style issue on
> > this patch series. Won't comment anymore about coding style, as I'm
> > assuming that you'll be fixing it on the other patches of this series
> > if needed.
> >
> > Regards,
> > Mauro
> 


-- 

Cheers,
Mauro

Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Mauro Carvalho Chehab]

Em Sun, 24 Mar 2013 13:53:40 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
> > Em Sat, 23 Mar 2013 18:27:08 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm
> >> for i2c communication with the sensor, which is connected to a second i2c bus.
> >>
> >> We don't know yet how to find out which devices support/use it.
> >> It's very likely used by all em25xx and em276x+ bridges.
> >> Tests with other em28xx chips (em2820, em2882/em2883) show, that this
> >> algorithm always succeeds there although no slave device is connected.
> >>
> >> The algorithm likely also works for real i2c client devices (OV2640 uses SCCB),
> >> because the Windows driver seems to use it for probing Samsung and Kodak
> >> sensors.
> >>
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> ---
> >>  drivers/media/usb/em28xx/em28xx-cards.c |    8 +-
> >>  drivers/media/usb/em28xx/em28xx-i2c.c   |  229 +++++++++++++++++++++++++------
> >>  drivers/media/usb/em28xx/em28xx.h       |   10 +-
> >>  3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
> >>
> >> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >> index cb7cdd3..033b6cb 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >> @@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> >>  	rt_mutex_init(&dev->i2c_bus_lock);
> >>  
> >>  	/* register i2c bus 0 */
> >> -	retval = em28xx_i2c_register(dev, 0);
> >> +	if (dev->board.is_em2800)
> >> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM2800);
> >> +	else
> >> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM28XX);
> >>  	if (retval < 0) {
> >>  		em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
> >>  			__func__, retval);
> >>  		goto unregister_dev;
> >>  	}
> >>  
> >> +	/* register i2c bus 1 */
> >>  	if (dev->def_i2c_bus) {
> >> -		retval = em28xx_i2c_register(dev, 1);
> >> +		retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
> >>  		if (retval < 0) {
> >>  			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
> >>  				__func__, retval);
> >> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> >> index 9e2fa41..ab14ac3 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> >> @@ -5,6 +5,7 @@
> >>  		      Markus Rechberger <mrechberger@gmail.com>
> >>  		      Mauro Carvalho Chehab <mchehab@infradead.org>
> >>  		      Sascha Sommer <saschasommer@freenet.de>
> >> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
> >>  
> >>     This program is free software; you can redistribute it and/or modify
> >>     it under the terms of the GNU General Public License as published by
> >> @@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
> >>  }
> >>  
> >>  /*
> >> + * em25xx_bus_B_send_bytes
> >> + * write bytes to the i2c device
> >> + */
> >> +static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >> +				   u16 len)
> >> +{
> >> +	int ret;
> >> +
> >> +	if (len < 1 || len > 64)
> >> +		return -EOPNOTSUPP;
> >> +	/* NOTE: limited by the USB ctrl message constraints
> >> +	 * Zero length reads always succeed, even if no device is connected */
> >> +
> >> +	/* Set register and write value */
> >> +	ret = dev->em28xx_write_regs_req(dev, 0x06, addr, buf, len);
> >> +	/* NOTE:
> >> +	 * 0 byte writes always succeed, even if no device is connected. */
> > You already noticed it on the previous note.
> 
> Yes. ;)

Well, there's no need to repeat the same thing twice at the same function ;)

> >
> >> +	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;
> >> +		}
> >> +	}
> >> +	/* Check success */
> >> +	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
> >> +	/* NOTE: the only error we've seen so far is
> >> +	 * 0x01 when the slave device is not present */
> >> +	if (ret == 0x00) {
> > 	Please simplify. just use:
> > 		if (!ret)
> 
> I would like to keep it as is because I think it better expresses the
> purposes of this check. I also used 0x00 instead of 0 on purpose.

Why do you think it better expresses it? It is just a more verbose way
of doing the same thing. 

If you want to better express, then add a comment:
	/* 
	 * Reg 08 value 0 means that the operation succeeded.
	 * different values indicate that the I2C device was not found.
	 */
	if (!ret)
		return len;

> ret is a mixed value which is negative on errors and contains the data
> bytes (0x00 to 0xff) on success.
> Ok, in this specific case all other values are covered with a single
> (ret > 0) check, but take a look at the comment and the em28xx-algo
> functions where we check for 0x10, too.
> 
> >
> >> +		return len;
> >> +	} else if (ret > 0) {
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	return ret;
> >> +	/* NOTE: With chips which do not support this operation,
> >> +	 * it seems to succeed ALWAYS ! (even if no device connected) */
> > Sorry, but I didn't get what you're trying to explain here. What are those
> > em25xx chips that don't support this operation?
> 
> Hmm... I don't know how to explain it better...
> The thing is, that this algo _seems_ to work also (at least with some)
> chips which actually don't support it (even if they don't provide a
> second i2c bus).

Again, what do you mean by "chips which actually don't support it"?

Are you talking about some versions of chips with this ID?
+	CHIP_ID_EM2765 = 54,

Or about something else? How those can be distinguished from the others
that don't support it? Or they can't be distinguished?

> >
> >> +}
> >> +
> >> +/*
> >> + * em25xx_bus_B_recv_bytes
> >> + * read bytes from the i2c device
> >> + */
> >> +static int em25xx_bus_B_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >> +				   u16 len)
> >> +{
> >> +	int ret;
> >> +
> >> +	if (len < 1 || len > 64)
> >> +		return -EOPNOTSUPP;
> >> +	/* NOTE: limited by the USB ctrl message constraints
> >> +	 * Zero length reads always succeed, even if no device is connected */
> > You already explained about the zero length before.
> 
> Yepp, but in another function. ;)
> 
> >> +
> >> +	/* Read value */
> >> +	ret = dev->em28xx_read_reg_req_len(dev, 0x06, addr, buf, len);
> >> +	/* NOTE:
> >> +	 * 0 byte reads always succeed, even if no device is connected. */
> > You're insistent on that, won't you? ;)
> 
> Yeah, I like comments and I think there should be much more ! :)
> 
> But in this case I don't want to be insistent, I just want to make sure
> that important issues are expalined at all relevant code places.
> The alternative would be to reference comments at other places, which
> IMHO doesn't work well.

Well, you don't need to explain the same thing twice at the same function ;)
> 
> Regards,
> Frank
> 


-- 

Cheers,
Mauro

Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Frank Schäfer]

Am 24.03.2013 15:02, schrieb Mauro Carvalho Chehab:
> Em Sun, 24 Mar 2013 13:53:40 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
>>> Em Sat, 23 Mar 2013 18:27:08 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm
>>>> for i2c communication with the sensor, which is connected to a second i2c bus.
>>>>
>>>> We don't know yet how to find out which devices support/use it.
>>>> It's very likely used by all em25xx and em276x+ bridges.
>>>> Tests with other em28xx chips (em2820, em2882/em2883) show, that this
>>>> algorithm always succeeds there although no slave device is connected.
>>>>
>>>> The algorithm likely also works for real i2c client devices (OV2640 uses SCCB),
>>>> because the Windows driver seems to use it for probing Samsung and Kodak
>>>> sensors.
>>>>
>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>> ---
>>>>  drivers/media/usb/em28xx/em28xx-cards.c |    8 +-
>>>>  drivers/media/usb/em28xx/em28xx-i2c.c   |  229 +++++++++++++++++++++++++------
>>>>  drivers/media/usb/em28xx/em28xx.h       |   10 +-
>>>>  3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
>>>>
>>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>>>> index cb7cdd3..033b6cb 100644
>>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>>>> @@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>>>>  	rt_mutex_init(&dev->i2c_bus_lock);
>>>>  
>>>>  	/* register i2c bus 0 */
>>>> -	retval = em28xx_i2c_register(dev, 0);
>>>> +	if (dev->board.is_em2800)
>>>> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM2800);
>>>> +	else
>>>> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM28XX);
>>>>  	if (retval < 0) {
>>>>  		em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
>>>>  			__func__, retval);
>>>>  		goto unregister_dev;
>>>>  	}
>>>>  
>>>> +	/* register i2c bus 1 */
>>>>  	if (dev->def_i2c_bus) {
>>>> -		retval = em28xx_i2c_register(dev, 1);
>>>> +		retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
>>>>  		if (retval < 0) {
>>>>  			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
>>>>  				__func__, retval);
>>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>>>> index 9e2fa41..ab14ac3 100644
>>>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>>>> @@ -5,6 +5,7 @@
>>>>  		      Markus Rechberger <mrechberger@gmail.com>
>>>>  		      Mauro Carvalho Chehab <mchehab@infradead.org>
>>>>  		      Sascha Sommer <saschasommer@freenet.de>
>>>> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
>>>>  
>>>>     This program is free software; you can redistribute it and/or modify
>>>>     it under the terms of the GNU General Public License as published by
>>>> @@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
>>>>  }
>>>>  
>>>>  /*
>>>> + * em25xx_bus_B_send_bytes
>>>> + * write bytes to the i2c device
>>>> + */
>>>> +static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>>> +				   u16 len)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	if (len < 1 || len > 64)
>>>> +		return -EOPNOTSUPP;
>>>> +	/* NOTE: limited by the USB ctrl message constraints
>>>> +	 * Zero length reads always succeed, even if no device is connected */
>>>> +
>>>> +	/* Set register and write value */
>>>> +	ret = dev->em28xx_write_regs_req(dev, 0x06, addr, buf, len);
>>>> +	/* NOTE:
>>>> +	 * 0 byte writes always succeed, even if no device is connected. */
>>> You already noticed it on the previous note.
>> Yes. ;)
> Well, there's no need to repeat the same thing twice at the same function ;)

Uhm yes... WTF... !??
This stuff has definitely been rebased too often... ;)


>>>> +	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;
>>>> +		}
>>>> +	}
>>>> +	/* Check success */
>>>> +	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
>>>> +	/* NOTE: the only error we've seen so far is
>>>> +	 * 0x01 when the slave device is not present */
>>>> +	if (ret == 0x00) {
>>> 	Please simplify. just use:
>>> 		if (!ret)
>> I would like to keep it as is because I think it better expresses the
>> purposes of this check. I also used 0x00 instead of 0 on purpose.
> Why do you think it better expresses it? It is just a more verbose way
> of doing the same thing. 
>
> If you want to better express, then add a comment:
> 	/* 
> 	 * Reg 08 value 0 means that the operation succeeded.
> 	 * different values indicate that the I2C device was not found.
> 	 */
> 	if (!ret)
> 		return len;

:)

I don't care too much. If you prefer it that way, no problem.

>> ret is a mixed value which is negative on errors and contains the data
>> bytes (0x00 to 0xff) on success.
>> Ok, in this specific case all other values are covered with a single
>> (ret > 0) check, but take a look at the comment and the em28xx-algo
>> functions where we check for 0x10, too.
>>
>>>> +		return len;
>>>> +	} else if (ret > 0) {
>>>> +		return -ENODEV;
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +	/* NOTE: With chips which do not support this operation,
>>>> +	 * it seems to succeed ALWAYS ! (even if no device connected) */
>>> Sorry, but I didn't get what you're trying to explain here. What are those
>>> em25xx chips that don't support this operation?
>> Hmm... I don't know how to explain it better...
>> The thing is, that this algo _seems_ to work also (at least with some)
>> chips which actually don't support it (even if they don't provide a
>> second i2c bus).
> Again, what do you mean by "chips which actually don't support it"?
>
> Are you talking about some versions of chips with this ID?
> +	CHIP_ID_EM2765 = 54,

We don't know any other way to distinguish between chips than the chip
ID, right ? ;)
So the same chip ID means the same "chip" or "chip type" for us and
different chip IDs mean different "chips" or "chip types".
And even if there would be several sub-revisions, it's not likely that
they would differ in such a significant feature.

I think the comment should be clear enough, but I could change it to
  "chips with different chip ids which actually don't support it"
Would that make it clear enough for you ? Or do you have a better
suggestion ?


> Or about something else? How those can be distinguished from the others
> that don't support it? Or they can't be distinguished?

That's exactly the reason for this comment. ;)
I don't know, do you ?

Regards,
Frank

Re: [PATCH v2 1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations

[Mauro Carvalho Chehab]

Em Sun, 24 Mar 2013 22:14:58 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 24.03.2013 15:02, schrieb Mauro Carvalho Chehab:
> > Em Sun, 24 Mar 2013 13:53:40 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
> >>> Em Sat, 23 Mar 2013 18:27:08 +0100
> >>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>>
> >>>> The webcam "SpeedLink VAD Laplace" (em2765 + ov2640) uses a special algorithm
> >>>> for i2c communication with the sensor, which is connected to a second i2c bus.
> >>>>
> >>>> We don't know yet how to find out which devices support/use it.
> >>>> It's very likely used by all em25xx and em276x+ bridges.
> >>>> Tests with other em28xx chips (em2820, em2882/em2883) show, that this
> >>>> algorithm always succeeds there although no slave device is connected.
> >>>>
> >>>> The algorithm likely also works for real i2c client devices (OV2640 uses SCCB),
> >>>> because the Windows driver seems to use it for probing Samsung and Kodak
> >>>> sensors.
> >>>>
> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>> ---
> >>>>  drivers/media/usb/em28xx/em28xx-cards.c |    8 +-
> >>>>  drivers/media/usb/em28xx/em28xx-i2c.c   |  229 +++++++++++++++++++++++++------
> >>>>  drivers/media/usb/em28xx/em28xx.h       |   10 +-
> >>>>  3 Dateien geändert, 205 Zeilen hinzugefügt(+), 42 Zeilen entfernt(-)
> >>>>
> >>>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> >>>> index cb7cdd3..033b6cb 100644
> >>>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> >>>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> >>>> @@ -3139,15 +3139,19 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> >>>>  	rt_mutex_init(&dev->i2c_bus_lock);
> >>>>  
> >>>>  	/* register i2c bus 0 */
> >>>> -	retval = em28xx_i2c_register(dev, 0);
> >>>> +	if (dev->board.is_em2800)
> >>>> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM2800);
> >>>> +	else
> >>>> +		retval = em28xx_i2c_register(dev, 0, EM28XX_I2C_ALGO_EM28XX);
> >>>>  	if (retval < 0) {
> >>>>  		em28xx_errdev("%s: em28xx_i2c_register bus 0 - error [%d]!\n",
> >>>>  			__func__, retval);
> >>>>  		goto unregister_dev;
> >>>>  	}
> >>>>  
> >>>> +	/* register i2c bus 1 */
> >>>>  	if (dev->def_i2c_bus) {
> >>>> -		retval = em28xx_i2c_register(dev, 1);
> >>>> +		retval = em28xx_i2c_register(dev, 1, EM28XX_I2C_ALGO_EM28XX);
> >>>>  		if (retval < 0) {
> >>>>  			em28xx_errdev("%s: em28xx_i2c_register bus 1 - error [%d]!\n",
> >>>>  				__func__, retval);
> >>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> >>>> index 9e2fa41..ab14ac3 100644
> >>>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> >>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> >>>> @@ -5,6 +5,7 @@
> >>>>  		      Markus Rechberger <mrechberger@gmail.com>
> >>>>  		      Mauro Carvalho Chehab <mchehab@infradead.org>
> >>>>  		      Sascha Sommer <saschasommer@freenet.de>
> >>>> +   Copyright (C) 2013 Frank Schäfer <fschaefer.oss@googlemail.com>
> >>>>  
> >>>>     This program is free software; you can redistribute it and/or modify
> >>>>     it under the terms of the GNU General Public License as published by
> >>>> @@ -274,6 +275,176 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
> >>>>  }
> >>>>  
> >>>>  /*
> >>>> + * em25xx_bus_B_send_bytes
> >>>> + * write bytes to the i2c device
> >>>> + */
> >>>> +static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >>>> +				   u16 len)
> >>>> +{
> >>>> +	int ret;
> >>>> +
> >>>> +	if (len < 1 || len > 64)
> >>>> +		return -EOPNOTSUPP;
> >>>> +	/* NOTE: limited by the USB ctrl message constraints
> >>>> +	 * Zero length reads always succeed, even if no device is connected */
> >>>> +
> >>>> +	/* Set register and write value */
> >>>> +	ret = dev->em28xx_write_regs_req(dev, 0x06, addr, buf, len);
> >>>> +	/* NOTE:
> >>>> +	 * 0 byte writes always succeed, even if no device is connected. */
> >>> You already noticed it on the previous note.
> >> Yes. ;)
> > Well, there's no need to repeat the same thing twice at the same function ;)
> 
> Uhm yes... WTF... !??
> This stuff has definitely been rebased too often... ;)

:)
> 
> 
> >>>> +	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;
> >>>> +		}
> >>>> +	}
> >>>> +	/* Check success */
> >>>> +	ret = dev->em28xx_read_reg_req(dev, 0x08, 0x0000);
> >>>> +	/* NOTE: the only error we've seen so far is
> >>>> +	 * 0x01 when the slave device is not present */
> >>>> +	if (ret == 0x00) {
> >>> 	Please simplify. just use:
> >>> 		if (!ret)
> >> I would like to keep it as is because I think it better expresses the
> >> purposes of this check. I also used 0x00 instead of 0 on purpose.
> > Why do you think it better expresses it? It is just a more verbose way
> > of doing the same thing. 
> >
> > If you want to better express, then add a comment:
> > 	/* 
> > 	 * Reg 08 value 0 means that the operation succeeded.
> > 	 * different values indicate that the I2C device was not found.
> > 	 */
> > 	if (!ret)
> > 		return len;
> 
> :)
> 
> I don't care too much. If you prefer it that way, no problem.

Ok, thanks!
> 
> >> ret is a mixed value which is negative on errors and contains the data
> >> bytes (0x00 to 0xff) on success.
> >> Ok, in this specific case all other values are covered with a single
> >> (ret > 0) check, but take a look at the comment and the em28xx-algo
> >> functions where we check for 0x10, too.
> >>
> >>>> +		return len;
> >>>> +	} else if (ret > 0) {
> >>>> +		return -ENODEV;
> >>>> +	}
> >>>> +
> >>>> +	return ret;
> >>>> +	/* NOTE: With chips which do not support this operation,
> >>>> +	 * it seems to succeed ALWAYS ! (even if no device connected) */
> >>> Sorry, but I didn't get what you're trying to explain here. What are those
> >>> em25xx chips that don't support this operation?
> >> Hmm... I don't know how to explain it better...
> >> The thing is, that this algo _seems_ to work also (at least with some)
> >> chips which actually don't support it (even if they don't provide a
> >> second i2c bus).
> > Again, what do you mean by "chips which actually don't support it"?
> >
> > Are you talking about some versions of chips with this ID?
> > +	CHIP_ID_EM2765 = 54,
> 
> We don't know any other way to distinguish between chips than the chip
> ID, right ? ;)
> So the same chip ID means the same "chip" or "chip type" for us and
> different chip IDs mean different "chips" or "chip types".
> And even if there would be several sub-revisions, it's not likely that
> they would differ in such a significant feature.
> 
> I think the comment should be clear enough, but I could change it to
>   "chips with different chip ids which actually don't support it"

Ah, now it is clear to me!

> Would that make it clear enough for you ? Or do you have a better
> suggestion ?

> 
> 
> > Or about something else? How those can be distinguished from the others
> > that don't support it? Or they can't be distinguished?
> 
> That's exactly the reason for this comment. ;)
> I don't know, do you ?
> 
> Regards,
> Frank
> 
> 
> 
> 
> 


-- 

Cheers,
Mauro