Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab: > Em Sun, 23 Dec 2012 14:58:12 +0100 > Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu: > >> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab: >>> Em Sun, 16 Dec 2012 19:23:28 +0100 >>> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu: >>> >>>> The em2800 can transfer up to 4 bytes per i2c message. >>>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per message. >>>> >>>> I2C adapters should never split messages transferred via the I2C subsystem >>>> into multiple message transfers, because the result will almost always NOT be >>>> the same as when the whole data is transferred to the I2C client in a single >>>> message. >>>> If the message size exceeds the capabilities of the I2C adapter, -EOPNOTSUPP >>>> should be returned. >>>> >>>> Signed-off-by: Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> >>>> --- >>>> drivers/media/usb/em28xx/em28xx-i2c.c | 44 ++++++++++++++------------------- >>>> 1 Datei geändert, 18 Zeilen hinzugefügt(+), 26 Zeilen entfernt(-) >>>> >>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c >>>> index 44533e4..c508c12 100644 >>>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c >>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c >>>> @@ -50,14 +50,18 @@ do { \ >>>> } while (0) >>>> >>>> /* >>>> - * em2800_i2c_send_max4() >>>> - * send up to 4 bytes to the i2c device >>>> + * em2800_i2c_send_bytes() >>>> + * send up to 4 bytes to the em2800 i2c device >>>> */ >>>> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len) >>>> +static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) >>>> { >>>> int ret; >>>> int write_timeout; >>>> u8 b2[6]; >>>> + >>>> + if (len < 1 || len > 4) >>>> + return -EOPNOTSUPP; >>>> + >>> Except if you actually tested it with all em2800 devices, I think that >>> this change just broke it for em2800. >> Yes, I have tested it. ;) >> But it should be obvious that splitting messages doesn't work, because >> the first byte of each message is treated as register address: >> >> Let's say we want to transfer the 8 bytes 11 22 33 44 55 66 77 88 to >> registers A0-A7 of a client. >> The correspondig i2c message transferred via the i2c subsystem is A0 11 >> 22 33 44 55 66 77 88 >> The current in em28xx splits this message into 3 messages: >> >> message 1: A0 11 22 33 >> message 2: 44 55 66 77 >> message 3: 88 >> >> The result is, that only the first 3 bytes are transferred correctly. >> The data bytes 44 and 88 are interpreted as register index and get lost, >> data bytes 55 66 77 end up in registers 0x44, 0x45 and 0x46 instead of >> 0xA4 to 0xA6. >> Ouch ! > I see your point, but there are devices that allow sending a message > splitted into smaller URB data and do the right thing when the I2C > transfer actually happen. The cx231xx is one of such devices: it has > 4 I2C buses internally; a few of them have a limit of 4 bytes per > transfer. Even so, manufacturers use this limited buses, plugging > I2C devices there that require I2C transfers with more than 4 bytes. > > One of the tricks used on such devices to write long messages is > to disable I2C stop on the first I2C block, and to disable I2C > start+address data on the next messages. Only the I2C bus can do > such things, and such logic avoids to push down to all I2C clients > a complex logic to split messages there. Yes, if the i2c adapter supports this feature, there is no problem with splitting messages. > I'm not sure if this is the case of em2800 though, as I don't have > any device with it. I'm not aware of a possibility to do this with the em2800. > In any case, before dropping it, we should be sure that this won't > break any supported device. Probably the worse case are the TV-based > em2800 boards, with tuners, as they may need to transfer more than 4 > bytes via I2C. I agree. We currently have 6 boards with the em2800. Used i2c clients are TDA9887 SAA711X TUNER_PHILIPS_FCV1236D TUNER_LG_PAL_NEW_TAPC TUNER_LG_TALN and in case of the Terratec Cinergy 200 USB an external IR receiver IC None of these clients tries to transfer more than 4 bytes at once. Terratec Cinergy 200 USB has also been tested by me. Anyway, even if one of those client drivers would writes more than 4 bytes at once to the device, it wouldn't work with the current code. >>> Maybe Sascha could review this patch series on his em2800 devices. >> Comments / reviews are appreciated. > Let's give him some time to review, before merging it. Sure. >>> Those devices are limited, and just like other devices (cx231xx for example), >>> the I2C bus need to split long messages, otherwise the I2C devices will >>> fail. >> I2C adapters are supposed to fail with -EOPNOTSUPP if the message length >> exceeds their capabilities. >> Drivers must be able to handle this error, otherwise they have to be fixed. > Currently, afaikt, no V4L2 I2C client knows how to handle it. Maybe. Fortunately, it seems to cause no trouble. > Ok, returning > -EOPNOTSUPP if the I2C data came from userspace makes sense. > >>> Btw, there was already a long discussion with regards to splitting long >>> I2C messages at the I2C bus or at the I2C adapters. The decision was >>> to do it at the I2C bus logic, as it is simpler than making a code >>> at each I2C client for them to properly handle -EOPNOTSUPP and implement >>> a fallback logic to reduce the transfer window until reach what's >>> supported by the device. >> While letting the i2c bus layer split messages sounds like the right >> thing to do, it is hard to realize that in practice. >> The reason is, that the needed algorithm depends on the capabilities and >> behavior of the i2c adapter _and_ the connected i2c client. >> The three main parameters are: >> - message size limits >> - client register width >> - automatic register index incrementation >> >> I don't know what has been discussed in past, > You'll need to dig into the ML archives. This is a recurrent theme, and, > we have implementations doing I2C split at bus (most cases) and a few > ones doing it at the client side. Yeah, I also have a working implementation of i2c block read/write emulation in my experimental code. ;) >> but I talked to Jean >> Delvare about the message size constraints a few weeks ago. >> He told me that it doesn't make sense to try to handle this at the i2c >> subsystem level. The parameters can be different for reading and >> writing, adapter and client and things are getting complicated quickly. > Jean's opinion is to push it to I2C clients (and we actually do it on a > few cases), but as I explained before, there are several drivers where > this is better done at the I2C bus driver, as the I2C implementation > allows doing it easily at bus level by playing with I2C STOP bits/I2C > start bits. > > We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't > tell the max size of the I2C messages. Adding a complex split logic > for every driver is not a common practice, as just a few I2C bus bridge > drivers suffer from very strict limits. Yes, and even with those who have such a strict limit, it is usually not exceeded because the clients are too 'simple'. ;) > Also, clients that split I2C messages don't actually handle -EOPNOTSUPP. > Instead, they have an init parameter telling the maximum size of the > I2C messages accepted by the bus. > > The logic there is complex, and may require an additional logic at the > bus side, in order to warrant that no I2C stop/start bits will be sent > in the middle of a message, or otherwise the device will fail[1]. > > So, it is generally simpler and more effective to just do it at the bus > side. Maybe. I have no opinion yet. My feeling is, that this should be handled by the i2c subsystem as much as possible, but a) it's complex due to the described reasons b) I have no complete concept yet c) the i2c people seem to be not very interested d) there is lots of other stuff with a higher priority on my TODO list The reason why I started looking into this is, that the em2765 in the SpeedLink VAD Laplace webcam (and likely all similar chip variants, e.g. em2760, em277x, em25xx) can transfer only 1 byte per message to/from the sensor client when using the 2nd i2c bus. I don't know where this restriction comes from, possible explanations are: - because that the 2nd bus is realized using GPIO pins - because the OV2640 uses SCCB Sensor probing for example includes reading 16 bit VID and VID from the client, which means that we require 2 instead of 1 i2c transfers. Sure, for this special case we could live with it, especially because the OV2640 driver uses single byte transfers only... But that's a separate issue, we can talk about later in a different tread. This patch just removes some code which is currently not working (and appearantly not needed). So I think it should be applied. Regards, Frank > [1] not all I2C client drivers have an address sub-address. Also, several > of them require a long I2C message to be handled at once, otherwise the > device fails. > > Regards, > Mauro -- 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