[v2,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 | 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(-)
Comments
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
--
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
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
--
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 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
--
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 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
--
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
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
>
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
>
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
--
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
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
>
>
>
>
>
@@ -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);
@@ -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);
@@ -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 */