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

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


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   |  164 +++++++++++++++++++++++++++----
 drivers/media/usb/em28xx/em28xx-reg.h   |    7 ++
 drivers/media/usb/em28xx/em28xx-video.c |   72 +++++++++++++-
 drivers/media/usb/em28xx/em28xx.h       |    8 ++
 6 Dateien geändert, 271 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)

-- 
1.7.10.4

[PATCH 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 |    6 +-
 drivers/media/usb/em28xx/em28xx-i2c.c   |  164 +++++++++++++++++++++++++++----
 drivers/media/usb/em28xx/em28xx.h       |    7 ++
 3 Dateien geändert, 159 Zeilen hinzugefügt(+), 18 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
index 5a5b637..75d4aef 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3103,7 +3103,11 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 		return retval;
 	}
 	/* Configure i2c bus */
-	if (!dev->board.is_em2800) {
+	if (dev->board.is_em2800) {
+		dev->i2c_algo_type = EM28XX_I2C_ALGO_EM2800;
+	} else {
+		dev->i2c_algo_type = EM28XX_I2C_ALGO_EM28XX;
+
 		retval = em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, dev->board.i2c_speed);
 		if (retval < 0) {
 			em28xx_errdev("%s: em28xx_write_reg failed!"
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 44bef43..6e86ffc 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
@@ -270,6 +271,114 @@ 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 != 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 */
+	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) */
+}
+
+/*
  * em28xx_i2c_xfer()
  * the main i2c transfer function
  */
@@ -277,7 +386,9 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 			   struct i2c_msg msgs[], int num)
 {
 	struct em28xx *dev = i2c_adap->algo_data;
-	int addr, rc, i, byte;
+	int addr, i, byte;
+	int rc = -EOPNOTSUPP;
+	enum em28xx_i2c_algo_type algo_type = dev->i2c_algo_type;
 
 	if (num <= 0)
 		return 0;
@@ -290,10 +401,13 @@ 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
+			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
 				rc = em28xx_i2c_check_for_device(dev, addr);
+			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
+				rc = em2800_i2c_check_for_device(dev, addr);
+			} else if (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");
@@ -301,14 +415,19 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 			}
 		} else if (msgs[i].flags & I2C_M_RD) {
 			/* read bytes */
-			if (dev->board.is_em2800)
-				rc = em2800_i2c_recv_bytes(dev, addr,
+			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
+				rc = em28xx_i2c_recv_bytes(dev, addr,
 							   msgs[i].buf,
 							   msgs[i].len);
-			else
-				rc = em28xx_i2c_recv_bytes(dev, addr,
+			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
+				rc = em2800_i2c_recv_bytes(dev, addr,
 							   msgs[i].buf,
 							   msgs[i].len);
+			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
+				rc = em25xx_bus_B_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]);
@@ -319,15 +438,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 				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
+			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
 				rc = em28xx_i2c_send_bytes(dev, addr,
 							   msgs[i].buf,
 							   msgs[i].len,
 							   i == num - 1);
+			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
+				rc = em2800_i2c_send_bytes(dev, addr,
+							   msgs[i].buf,
+							   msgs[i].len);
+			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
+				rc = em25xx_bus_B_send_bytes(dev, addr,
+							    msgs[i].buf,
+							    msgs[i].len);
+			}
 		}
 		if (rc < 0) {
 			if (i2c_debug)
@@ -589,10 +713,16 @@ error:
 static u32 functionality(struct i2c_adapter *adap)
 {
 	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;
+
+	if ((dev->i2c_algo_type == EM28XX_I2C_ALGO_EM28XX) ||
+	    (dev->i2c_algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)) {
+		return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	} else if (dev->i2c_algo_type == EM28XX_I2C_ALGO_EM2800)  {
+		return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL) &
+			~I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
+	}
+
+	return 0; /* BUG */
 }
 
 static struct i2c_algorithm em28xx_algo = {
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 2d6d31a..b7c8134 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -175,6 +175,12 @@
 /* time in msecs to wait for i2c writes to finish */
 #define EM2800_I2C_XFER_TIMEOUT		20
 
+enum em28xx_i2c_algo_type {
+	EM28XX_I2C_ALGO_EM28XX,
+	EM28XX_I2C_ALGO_EM2800,
+	EM28XX_I2C_ALGO_EM25XX_BUS_B,
+};
+
 enum em28xx_mode {
 	EM28XX_SUSPEND,
 	EM28XX_ANALOG_MODE,
@@ -510,6 +516,7 @@ struct em28xx {
 	/* i2c i/o */
 	struct i2c_adapter i2c_adap;
 	struct i2c_client i2c_client;
+	enum em28xx_i2c_algo_type i2c_algo_type;
 	unsigned char eeprom_addrwidth_16bit:1;
 	/* video for linux */
 	int users;		/* user count for exclusive use */
-- 
1.7.10.4

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

[Mauro Carvalho Chehab]

Em Sun,  3 Mar 2013 20:40:57 +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 |    6 +-
>  drivers/media/usb/em28xx/em28xx-i2c.c   |  164 +++++++++++++++++++++++++++----
>  drivers/media/usb/em28xx/em28xx.h       |    7 ++
>  3 Dateien geändert, 159 Zeilen hinzugefügt(+), 18 Zeilen entfernt(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index 5a5b637..75d4aef 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3103,7 +3103,11 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>  		return retval;
>  	}
>  	/* Configure i2c bus */
> -	if (!dev->board.is_em2800) {
> +	if (dev->board.is_em2800) {
> +		dev->i2c_algo_type = EM28XX_I2C_ALGO_EM2800;
> +	} else {
> +		dev->i2c_algo_type = EM28XX_I2C_ALGO_EM28XX;
> +
>  		retval = em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, dev->board.i2c_speed);
>  		if (retval < 0) {
>  			em28xx_errdev("%s: em28xx_write_reg failed!"
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 44bef43..6e86ffc 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
> @@ -270,6 +271,114 @@ 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 != 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 */
> +	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) */
> +}
> +
> +/*
>   * em28xx_i2c_xfer()
>   * the main i2c transfer function
>   */
> @@ -277,7 +386,9 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>  			   struct i2c_msg msgs[], int num)
>  {
>  	struct em28xx *dev = i2c_adap->algo_data;
> -	int addr, rc, i, byte;
> +	int addr, i, byte;
> +	int rc = -EOPNOTSUPP;
> +	enum em28xx_i2c_algo_type algo_type = dev->i2c_algo_type;
>  
>  	if (num <= 0)
>  		return 0;
> @@ -290,10 +401,13 @@ 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
> +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
>  				rc = em28xx_i2c_check_for_device(dev, addr);
> +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
> +				rc = em2800_i2c_check_for_device(dev, addr);
> +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> +				rc = em25xx_bus_B_check_for_device(dev, addr);
> +			}

That seems too messy. 

IMO, the best is to create 3 different em28xx_i2c_xfer() routines:
one for em2800, one for "standard" I2c protocol, and another one for
this em25xx one. Then, all you need to do is to embed 
static struct i2c_algorithm into struct em28xx and fill
em28xx_algo.master_transfer to point to the correct xfer.

That makes the code more readable and remove a little faster by removing
the unneeded ifs from the code.

>  			if (rc == -ENODEV) {
>  				if (i2c_debug)
>  					printk(" no device\n");
> @@ -301,14 +415,19 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>  			}
>  		} else if (msgs[i].flags & I2C_M_RD) {
>  			/* read bytes */
> -			if (dev->board.is_em2800)
> -				rc = em2800_i2c_recv_bytes(dev, addr,
> +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
> +				rc = em28xx_i2c_recv_bytes(dev, addr,
>  							   msgs[i].buf,
>  							   msgs[i].len);
> -			else
> -				rc = em28xx_i2c_recv_bytes(dev, addr,
> +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
> +				rc = em2800_i2c_recv_bytes(dev, addr,
>  							   msgs[i].buf,
>  							   msgs[i].len);
> +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> +				rc = em25xx_bus_B_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]);
> @@ -319,15 +438,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>  				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
> +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
>  				rc = em28xx_i2c_send_bytes(dev, addr,
>  							   msgs[i].buf,
>  							   msgs[i].len,
>  							   i == num - 1);
> +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
> +				rc = em2800_i2c_send_bytes(dev, addr,
> +							   msgs[i].buf,
> +							   msgs[i].len);
> +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> +				rc = em25xx_bus_B_send_bytes(dev, addr,
> +							    msgs[i].buf,
> +							    msgs[i].len);
> +			}
>  		}
>  		if (rc < 0) {
>  			if (i2c_debug)
> @@ -589,10 +713,16 @@ error:
>  static u32 functionality(struct i2c_adapter *adap)
>  {
>  	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;
> +
> +	if ((dev->i2c_algo_type == EM28XX_I2C_ALGO_EM28XX) ||
> +	    (dev->i2c_algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)) {
> +		return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +	} else if (dev->i2c_algo_type == EM28XX_I2C_ALGO_EM2800)  {
> +		return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL) &
> +			~I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
> +	}
> +
> +	return 0; /* BUG */

Please create a separate I2C register logic for bus B. It was a mistake
to reuse the existing I2C bus for logic for both buses (ok, actually,
no device currently uses 2 buses, as they simply don't read eeproms on
2 bus devices). 

Btw, cx231xx has a similar issue: it has 3 buses. See how it was solved there:

drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[0]);
drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[1]);
drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[2]);

Of course, the em28xx-cards will need to tell on what bus the tuner and
demods are. The em28xx I2C logic will also need to write to EM28XX_R06_I2C_CLK
if the bus is different - the windows driver is just stupid: it just precedes all
I2C writes by a write at reg 0x06 - we can certainly do better than that ;).

If you're in doubt on how to do it, I can seek for some time to address it
properly.

>  }
>  
>  static struct i2c_algorithm em28xx_algo = {
> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
> index 2d6d31a..b7c8134 100644
> --- a/drivers/media/usb/em28xx/em28xx.h
> +++ b/drivers/media/usb/em28xx/em28xx.h
> @@ -175,6 +175,12 @@
>  /* time in msecs to wait for i2c writes to finish */
>  #define EM2800_I2C_XFER_TIMEOUT		20
>  
> +enum em28xx_i2c_algo_type {
> +	EM28XX_I2C_ALGO_EM28XX,
> +	EM28XX_I2C_ALGO_EM2800,
> +	EM28XX_I2C_ALGO_EM25XX_BUS_B,
> +};
> +
>  enum em28xx_mode {
>  	EM28XX_SUSPEND,
>  	EM28XX_ANALOG_MODE,
> @@ -510,6 +516,7 @@ struct em28xx {
>  	/* i2c i/o */
>  	struct i2c_adapter i2c_adap;
>  	struct i2c_client i2c_client;
> +	enum em28xx_i2c_algo_type i2c_algo_type;
>  	unsigned char eeprom_addrwidth_16bit:1;
>  	/* video for linux */
>  	int users;		/* user count for exclusive use */

-- 

Cheers,
Mauro

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

[Mauro Carvalho Chehab]

Em Mon, 4 Mar 2013 17:20:05 -0300
Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:

> Em Sun,  3 Mar 2013 20:40:57 +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 |    6 +-
> >  drivers/media/usb/em28xx/em28xx-i2c.c   |  164 +++++++++++++++++++++++++++----
> >  drivers/media/usb/em28xx/em28xx.h       |    7 ++
> >  3 Dateien geändert, 159 Zeilen hinzugefügt(+), 18 Zeilen entfernt(-)
> > 
> > diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> > index 5a5b637..75d4aef 100644
> > --- a/drivers/media/usb/em28xx/em28xx-cards.c
> > +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> > @@ -3103,7 +3103,11 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
> >  		return retval;
> >  	}
> >  	/* Configure i2c bus */
> > -	if (!dev->board.is_em2800) {
> > +	if (dev->board.is_em2800) {
> > +		dev->i2c_algo_type = EM28XX_I2C_ALGO_EM2800;
> > +	} else {
> > +		dev->i2c_algo_type = EM28XX_I2C_ALGO_EM28XX;
> > +
> >  		retval = em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, dev->board.i2c_speed);
> >  		if (retval < 0) {
> >  			em28xx_errdev("%s: em28xx_write_reg failed!"
> > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> > index 44bef43..6e86ffc 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
> > @@ -270,6 +271,114 @@ 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 != 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 */
> > +	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) */
> > +}
> > +
> > +/*
> >   * em28xx_i2c_xfer()
> >   * the main i2c transfer function
> >   */
> > @@ -277,7 +386,9 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
> >  			   struct i2c_msg msgs[], int num)
> >  {
> >  	struct em28xx *dev = i2c_adap->algo_data;
> > -	int addr, rc, i, byte;
> > +	int addr, i, byte;
> > +	int rc = -EOPNOTSUPP;
> > +	enum em28xx_i2c_algo_type algo_type = dev->i2c_algo_type;
> >  
> >  	if (num <= 0)
> >  		return 0;
> > @@ -290,10 +401,13 @@ 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
> > +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
> >  				rc = em28xx_i2c_check_for_device(dev, addr);
> > +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
> > +				rc = em2800_i2c_check_for_device(dev, addr);
> > +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> > +				rc = em25xx_bus_B_check_for_device(dev, addr);
> > +			}
> 
> That seems too messy. 
> 
> IMO, the best is to create 3 different em28xx_i2c_xfer() routines:
> one for em2800, one for "standard" I2c protocol, and another one for
> this em25xx one. Then, all you need to do is to embed 
> static struct i2c_algorithm into struct em28xx and fill
> em28xx_algo.master_transfer to point to the correct xfer.
> 
> That makes the code more readable and remove a little faster by removing
> the unneeded ifs from the code.
> 
> >  			if (rc == -ENODEV) {
> >  				if (i2c_debug)
> >  					printk(" no device\n");
> > @@ -301,14 +415,19 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
> >  			}
> >  		} else if (msgs[i].flags & I2C_M_RD) {
> >  			/* read bytes */
> > -			if (dev->board.is_em2800)
> > -				rc = em2800_i2c_recv_bytes(dev, addr,
> > +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
> > +				rc = em28xx_i2c_recv_bytes(dev, addr,
> >  							   msgs[i].buf,
> >  							   msgs[i].len);
> > -			else
> > -				rc = em28xx_i2c_recv_bytes(dev, addr,
> > +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
> > +				rc = em2800_i2c_recv_bytes(dev, addr,
> >  							   msgs[i].buf,
> >  							   msgs[i].len);
> > +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> > +				rc = em25xx_bus_B_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]);
> > @@ -319,15 +438,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
> >  				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
> > +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
> >  				rc = em28xx_i2c_send_bytes(dev, addr,
> >  							   msgs[i].buf,
> >  							   msgs[i].len,
> >  							   i == num - 1);
> > +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
> > +				rc = em2800_i2c_send_bytes(dev, addr,
> > +							   msgs[i].buf,
> > +							   msgs[i].len);
> > +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
> > +				rc = em25xx_bus_B_send_bytes(dev, addr,
> > +							    msgs[i].buf,
> > +							    msgs[i].len);
> > +			}
> >  		}
> >  		if (rc < 0) {
> >  			if (i2c_debug)
> > @@ -589,10 +713,16 @@ error:
> >  static u32 functionality(struct i2c_adapter *adap)
> >  {
> >  	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;
> > +
> > +	if ((dev->i2c_algo_type == EM28XX_I2C_ALGO_EM28XX) ||
> > +	    (dev->i2c_algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)) {
> > +		return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> > +	} else if (dev->i2c_algo_type == EM28XX_I2C_ALGO_EM2800)  {
> > +		return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL) &
> > +			~I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
> > +	}
> > +
> > +	return 0; /* BUG */
> 
> Please create a separate I2C register logic for bus B. It was a mistake
> to reuse the existing I2C bus for logic for both buses (ok, actually,
> no device currently uses 2 buses, as they simply don't read eeproms on
> 2 bus devices). 
> 
> Btw, cx231xx has a similar issue: it has 3 buses. See how it was solved there:
> 
> drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[0]);
> drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[1]);
> drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[2]);
> 
> Of course, the em28xx-cards will need to tell on what bus the tuner and
> demods are. The em28xx I2C logic will also need to write to EM28XX_R06_I2C_CLK
> if the bus is different - the windows driver is just stupid: it just precedes all
> I2C writes by a write at reg 0x06 - we can certainly do better than that ;).
> 
> If you're in doubt on how to do it, I can seek for some time to address it
> properly.

FYI, The other patches in this series are OK. I'm tagging them as
changes_requested just because they depend on this one.

Regards,
Mauro

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

[Frank Schäfer]

Am 04.03.2013 21:23, schrieb Mauro Carvalho Chehab:
> Em Mon, 4 Mar 2013 17:20:05 -0300
> Mauro Carvalho Chehab <mchehab@redhat.com> escreveu:
>
>> Em Sun,  3 Mar 2013 20:40:57 +0100
>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
...
>>> @@ -277,7 +386,9 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>>>  			   struct i2c_msg msgs[], int num)
>>>  {
>>>  	struct em28xx *dev = i2c_adap->algo_data;
>>> -	int addr, rc, i, byte;
>>> +	int addr, i, byte;
>>> +	int rc = -EOPNOTSUPP;
>>> +	enum em28xx_i2c_algo_type algo_type = dev->i2c_algo_type;
>>>  
>>>  	if (num <= 0)
>>>  		return 0;
>>> @@ -290,10 +401,13 @@ 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
>>> +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
>>>  				rc = em28xx_i2c_check_for_device(dev, addr);
>>> +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
>>> +				rc = em2800_i2c_check_for_device(dev, addr);
>>> +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
>>> +				rc = em25xx_bus_B_check_for_device(dev, addr);
>>> +			}
>> That seems too messy. 
>>
>> IMO, the best is to create 3 different em28xx_i2c_xfer() routines:
>> one for em2800, one for "standard" I2c protocol, and another one for
>> this em25xx one. Then, all you need to do is to embed 
>> static struct i2c_algorithm into struct em28xx and fill
>> em28xx_algo.master_transfer to point to the correct xfer.
>>
>> That makes the code more readable and remove a little faster by removing
>> the unneeded ifs from the code.

Hmmm.... that's how I did it initially.
But IIRC correctly, it also duplicates lots of code, making it much
bigger...

I will take a look at it again tomorrow evening.


>>>  			if (rc == -ENODEV) {
>>>  				if (i2c_debug)
>>>  					printk(" no device\n");
>>> @@ -301,14 +415,19 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>>>  			}
>>>  		} else if (msgs[i].flags & I2C_M_RD) {
>>>  			/* read bytes */
>>> -			if (dev->board.is_em2800)
>>> -				rc = em2800_i2c_recv_bytes(dev, addr,
>>> +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
>>> +				rc = em28xx_i2c_recv_bytes(dev, addr,
>>>  							   msgs[i].buf,
>>>  							   msgs[i].len);
>>> -			else
>>> -				rc = em28xx_i2c_recv_bytes(dev, addr,
>>> +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
>>> +				rc = em2800_i2c_recv_bytes(dev, addr,
>>>  							   msgs[i].buf,
>>>  							   msgs[i].len);
>>> +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
>>> +				rc = em25xx_bus_B_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]);
>>> @@ -319,15 +438,20 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
>>>  				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
>>> +			if (algo_type == EM28XX_I2C_ALGO_EM28XX) {
>>>  				rc = em28xx_i2c_send_bytes(dev, addr,
>>>  							   msgs[i].buf,
>>>  							   msgs[i].len,
>>>  							   i == num - 1);
>>> +			} else if (algo_type == EM28XX_I2C_ALGO_EM2800) {
>>> +				rc = em2800_i2c_send_bytes(dev, addr,
>>> +							   msgs[i].buf,
>>> +							   msgs[i].len);
>>> +			} else if (algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B) {
>>> +				rc = em25xx_bus_B_send_bytes(dev, addr,
>>> +							    msgs[i].buf,
>>> +							    msgs[i].len);
>>> +			}
>>>  		}
>>>  		if (rc < 0) {
>>>  			if (i2c_debug)
>>> @@ -589,10 +713,16 @@ error:
>>>  static u32 functionality(struct i2c_adapter *adap)
>>>  {
>>>  	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;
>>> +
>>> +	if ((dev->i2c_algo_type == EM28XX_I2C_ALGO_EM28XX) ||
>>> +	    (dev->i2c_algo_type == EM28XX_I2C_ALGO_EM25XX_BUS_B)) {
>>> +		return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>>> +	} else if (dev->i2c_algo_type == EM28XX_I2C_ALGO_EM2800)  {
>>> +		return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL) &
>>> +			~I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
>>> +	}
>>> +
>>> +	return 0; /* BUG */
>> Please create a separate I2C register logic for bus B. It was a mistake
>> to reuse the existing I2C bus for logic for both buses (ok, actually,
>> no device currently uses 2 buses, as they simply don't read eeproms on
>> 2 bus devices). 
>>
>> Btw, cx231xx has a similar issue: it has 3 buses. See how it was solved there:
>>
>> drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[0]);
>> drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[1]);
>> drivers/media/usb/cx231xx/cx231xx-core.c: cx231xx_i2c_register(&dev->i2c_bus[2]);
>>
>> Of course, the em28xx-cards will need to tell on what bus the tuner and
>> demods are. The em28xx I2C logic will also need to write to EM28XX_R06_I2C_CLK
>> if the bus is different - the windows driver is just stupid: it just precedes all
>> I2C writes by a write at reg 0x06 - we can certainly do better than that ;).
>>
>> If you're in doubt on how to do it, I can seek for some time to address it
>> properly.

See my previous mail concerning this.

> FYI, The other patches in this series are OK. I'm tagging them as
> changes_requested just because they depend on this one.

Ok, thanks.
These patches can wait until the VAD Laplace webcam is finally supported
(which I hope will be the case in a few weeks).
I don't expect this chip to appear in one of the devices with the
currently supported generic IDs.
What I'm trying to do is grouping ready patches and sending them as
smaller series to make reviewing easier.

Regards,
Frank

> Regards,
> Mauro

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

[Frank Schäfer]

Am 04.03.2013 22:31, schrieb Frank Schäfer:

> ...
> I don't expect this chip to appear in one of the devices with the
> currently supported generic IDs.

Hmm... maybe I should add:
it doesn't make sense to add the generic USB ID of this chip (eb1a:2765)
to the driver, because most of the devices using it should be UVC compliant.
The same should apply to all other newer Empia camera bridges
(EM276x/7x/8x).

Regards,
Frank

[PATCH 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 |   13 ++++++++++++-
 drivers/media/usb/em28xx/em28xx-reg.h   |    1 +
 drivers/media/usb/em28xx/em28xx.h       |    1 +
 3 Dateien geändert, 14 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 75d4aef..e4d14e7 100644
--- a/drivers/media/usb/em28xx/em28xx-cards.c
+++ b/drivers/media/usb/em28xx/em28xx-cards.c
@@ -3015,6 +3015,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;
@@ -3106,7 +3112,12 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
 	if (dev->board.is_em2800) {
 		dev->i2c_algo_type = EM28XX_I2C_ALGO_EM2800;
 	} else {
-		dev->i2c_algo_type = EM28XX_I2C_ALGO_EM28XX;
+		if (dev->is_em25xx)
+			/* Use i2c bus B */
+			dev->i2c_algo_type = EM28XX_I2C_ALGO_EM25XX_BUS_B;
+			/* FIXME: really do this always ? */
+		else
+			dev->i2c_algo_type = EM28XX_I2C_ALGO_EM28XX;
 
 		retval = em28xx_write_reg(dev, EM28XX_R06_I2C_CLK, dev->board.i2c_speed);
 		if (retval < 0) {
diff --git a/drivers/media/usb/em28xx/em28xx-reg.h b/drivers/media/usb/em28xx/em28xx-reg.h
index 1e369ba..d765d59 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 b7c8134..131fdaa 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -469,6 +469,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 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 93fc620..2e8681f 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 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 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 d765d59..3ec5528 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