Re: [PATCH] hid: cp2112: support i2c_transfer() when num > 1

[Antonio Borneo]

On Sun, Mar 15, 2015 at 8:10 PM, Wolfram Sang <wsa@xxxxxxxxxxxxx> wrote:
> On Sun, Mar 15, 2015 at 07:43:18PM +0800, Antonio Borneo wrote:
>> The device cp2112 does not support i2c combined transactions,
>> since unable to suppress the stop bit between adjacent i2c
>> transactions.
>
> Let's keep the precise terminology: a 'transfer' is anything between the
> start and stop bit. It can consist of multiple 'messages' which are
> combined with repeated start within one transfer.

Hi Wolfram,

thanks for your review and the clarification.

>
>> For this reason, in commit b9029345ed6483fcadadc4834b44a5656dd56d70
>> ("HID: cp2112: add I2C mode") I have omitted the support for
>> num > 1 in i2c_transfer().
>
> Good. You should make use of the quirk framework soon in linux-next or
> here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/quirks

Nice improvement. Informing the upper driver about max R/W length is
really welcome.

>> 1) in few kernel drivers i2c_transfer() has been used to
>> simplify the code by replacing a sequence of i2c_master_send()
>> and i2c_master_recv(), e.g. in i2c-hid.c and iio subsystem.
>> Those drivers will fail if used with current cp2112 driver.
>
> I see two options for those:
>
> 1) revert the simplifications and sacrifice a bit of performance
>    to support the widest number of adapters
> 2) use the quirk infrastructure from above to query the
>    quirks of the adapter to chose between fast or compatible

Could this be an extension of your quirks?
I mean, moving in i2c-core the switch between fast or compatible?
The caller should only state if combined transfer is strictly required
by the device on I2C bus.

> Keep in mind that some devices do *require* that messages are combined
> with repeated start because they will reset their state machine after
> stop.

Agree!

>> 2) in drivers/i2c/busses/ there are already drivers that
>> implement i2c_transfer() as a sequence of elementary (not
>> combined) i2c transactions, e.g. i2c-bcm2835.c, i2c-puv3.c,
>> i2c-qup.c, i2c-robotfuzz-osif.c, i2c-viperboard.c, i2c-xlr.c.
>
> You misread that, most probably. They are iterating over the messages,
> yes, but they are expected to connect them via repeated start. If there
> is a driver sending stop after every message and accepting more than one
> message per transfer, this is a bug.

I have check most of the I2C adapter drivers. At least for the 6
drivers above, reading the code I don't see anything that implements
the repeated start. But I do not have the HW to run a test.
It's definitively possible I misread them.

>> To fix 1) and considering 2), rewrite i2c_transfer() in case
>> of num > 1 as a loop of non-combined i2c transactions.
>
> For the above reasons, NAK.

Ok, agree to drop this patch.

>> In [1] we had a talk about implementing i2c_transfer() as a
>> sequence of elementary (not combined) i2c transactions.
>> After Jonathan's reply in [2], I went to check better the
>> existing I2C drivers and I changed opinion.
>
> And why is this driver in hid? This is clearly an I2C master driver?

Actually it should be a MFD, since it implements I2C/SMB master and GPIO.
It uses HID over USB, that is probably the reason it is here.

Best Regards,
Antonio
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/