Re: [PATCH v1] HID: support multiple and large i2c transfers in hid-cp2112

On 05/29/2015 08:38 PM, Antonio Borneo wrote:

On Sat, May 30, 2015 at 1:32 AM, Ellen Wang [off-list ref] wrote:

quoted

On 05/29/2015 09:28 AM, Antonio Borneo wrote:

quoted

CP2112 is not able to combine messages since unable to repeat the start
bits.
It can only send simple transfers or a combined read-after-write.

Here you cannot simply loop on all the messages and send them one by
one because there is no way to force the repeated start bit in
between.
You can find more details in this thread about CP2112
https://lkml.org/lkml/2015/3/15/64
and here info about the repeated start bit under "Combined transactions"
Documentation/i2c/i2c-protocol

Anyway your idea to introduce read-after-write is valid.
I suggest you to check the drivers i2c-opal.c and i2c-pmcmsp.c ; at a
quick check I think they have similar limitation as CP2112.
You could replicate the same check, supporting only num==1 (always)
and num==2 (only if msg[0] is write and msg[1] is read).

Best Regards,
Antonio

It don't think the code attempts to generate repeated start.  It simply
issues and completes each message as separate transfers.  It's not different
from calling cp2112_i2c_xfer() repeatedly with single messages, except in
the bracketing hid_hw_power().

cp2112_i2c_xfer() is the lower level side of i2c_transfer().
i2c_transfer() requires that only one Stop bit is sent at the end of
last message. Between messages a repeated start is mandatory.
In general case, CP2112 cannot do that, that's why the original code
returns error if num != 1

It's not correct to issue each message as separate transfer inside
cp2112_i2c_xfer(); the caller of i2c_transfer() expects the messages
to be sent with repeated start.

With you patch you highlight that there is another case that should be
implemented.
CP2112 can handle the case of num==2 when msg[0] is write and msg[1] is read.
There are other devices with similar limitations, as I pointed above.

Extending cp2112_i2c_xfer() to support a combined read-after-write is
ok, while issuing each message without forcing repeated start is
incorrect.

Best Regards,
Antonio

OK.  I neglected to read the later messages in the thread you pointed me to.  Oddly, I submitted my patch weeks before that thread, and was given exactly the opposite feedback, which was that the fix was fine in principle (but I should separate out some unrelated bug fixes).  And I agreed to using the quirks mechanism in a later patch.

Do you already have a patch that addresses the whole issue?  I just rewrote my code to handle single messages and the write-read case, as you suggested.

Thanks