Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

["Frank Schäfer" <fschaefer.oss@googlemail.com>]

Hi Antti,

Am 02.01.2013 20:29, schrieb Antti Palosaari:
> On 12/24/2012 01:09 PM, Frank Schäfer wrote:
>> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab:
>>> Em Sun, 23 Dec 2012 14:58:12 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> 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@googlemail.com> escreveu:
>>
>>>>> 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
>
> Maybe you already have seen, but I did some initial stuff year or two
> ago for implementing that but left it unimplemented as there was so
> much stuff to check and discuss in order to agree correct solution.
>
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html
>
> There is regmap which maybe could do stuff like that, I am not sure as
> I never tested it. At least it could do some stuff top of I2C bus.

Yes, I've read this discussion, but didn't have time to take a deeper
look into the regmap stuff yet.

For the em28xx driver itself, there is no real need for i2c block
read/write emulation at the moment. We could save only a few lines.
I'm also burried with lots of other stuff at the moment which has a
higher priority for me.

Please note that the whole discussion has nothing to do with this patch.
It just removes code which isn't and has never been working.

>
> Also I heavily disagree you what goes to I2C subsystem integration.
> That is clearly stuff which resides top of I2C bus and it is *not bus
> dependent*. There is many other buses too having similar splitting
> logic like SPI?
>

I don't understand you. In which points do we disagree ??


Regards,
Frank