Am 24.03.2013 12:38, schrieb Mauro Carvalho Chehab:
> Em Sat, 23 Mar 2013 18:27:08 +0100
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxx>
>> Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx>
>> Sascha Sommer <saschasommer@xxxxxxxxxx>
>> + Copyright (C) 2013 Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
