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

[Wolfram Sang]

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.

> 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

> 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

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

> 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.

> 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.

> 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?

Attachment: signature.asc
Description: Digital signature