[1/5] em28xx: add support for em25xx i2c bus B read/write/check device operations
Commit Message
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(-)
Comments
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 */
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -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!"
@@ -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 = {
@@ -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 */