Am 24.03.2013 15:02, schrieb Mauro Carvalho Chehab:
> Em Sun, 24 Mar 2013 13:53:40 +0100
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> 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@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. ;)
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
