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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, May 30, 2015 at 1:32 AM, Ellen Wang <ellen@xxxxxxxxxxxxxxxxxxx> wrote:
>
>
> On 05/29/2015 09:28 AM, Antonio Borneo wrote:
>>
>> On Fri, Apr 24, 2015 at 5:49 AM, Ellen Wang <ellen@xxxxxxxxxxxxxxxxxxx>
>> wrote:
>>>
>>> cp2112_i2c_xfer() only supports a single i2c_msg and only
>>> reads up to 61 bytes.  More than one message at a time
>>> and longers reads just return errors.
>>>
>>> This is a serious limitation.  For example, the at24 eeprom driver
>>> generates paired write and read messages (for eeprom address and data).
>>> And the reads can be quite large.  The fix consists of a loop
>>> to go through all the messages, and a loop around cp2112_read()
>>> to pick up all the returned data.  For extra credit, it now also
>>> supports write-read pairs and implements them as
>>> CP2112_DATA_WRITE_READ_REQUEST.
>>>
>>> Signed-off-by: Ellen Wang <ellen@xxxxxxxxxxxxxxxxxxx>
>>
>>
>> Hi Ellen,
>>
>> the patch is partially wrong. See below.
>>
>> I added Wolfram, I2C subsystem maintainer, in copy.
>>
>>> ---
>>>   drivers/hid/hid-cp2112.c |  136
>>> +++++++++++++++++++++++++++++-----------------
>>>   1 file changed, 87 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
>>> index 3318de6..e7e72a4 100644
>>> --- a/drivers/hid/hid-cp2112.c
>>> +++ b/drivers/hid/hid-cp2112.c
>>> @@ -444,11 +444,30 @@ static int cp2112_i2c_write_req(void *buf, u8
>>> slave_address, u8 *data,
>>>          return data_length + 3;
>>>   }
>>>
>>> +static int cp2112_i2c_write_read_req(void *buf, u8 slave_address,
>>> +                                    u8 *addr, int addr_length,
>>> +                                    int read_length)
>>> +{
>>> +       struct cp2112_write_read_req_report *report = buf;
>>> +
>>> +       if (read_length < 1 || read_length > 512 ||
>>> +           addr_length > sizeof(report->target_address))
>>> +               return -EINVAL;
>>> +
>>> +       report->report = CP2112_DATA_WRITE_READ_REQUEST;
>>> +       report->slave_address = slave_address << 1;
>>> +       report->length = cpu_to_be16(read_length);
>>> +       report->target_address_length = addr_length;
>>> +       memcpy(report->target_address, addr, addr_length);
>>> +       return addr_length + 5;
>>> +}
>>> +
>>>   static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
>>> *msgs,
>>>                             int num)
>>>   {
>>>          struct cp2112_device *dev = (struct cp2112_device
>>> *)adap->algo_data;
>>>          struct hid_device *hdev = dev->hdev;
>>> +       struct i2c_msg *m;
>>>          u8 buf[64];
>>>          ssize_t count;
>>>          unsigned int retries;
>>> @@ -456,71 +475,90 @@ static int cp2112_i2c_xfer(struct i2c_adapter
>>> *adap, struct i2c_msg *msgs,
>>>
>>>          hid_dbg(hdev, "I2C %d messages\n", num);
>>>
>>> -       if (num != 1) {
>>> -               hid_err(hdev,
>>> -                       "Multi-message I2C transactions not
>>> supported\n");
>>> -               return -EOPNOTSUPP;
>>> -       }
>>> -
>>> -       if (msgs->flags & I2C_M_RD)
>>> -               count = cp2112_read_req(buf, msgs->addr, msgs->len);
>>> -       else
>>> -               count = cp2112_i2c_write_req(buf, msgs->addr, msgs->buf,
>>> -                                            msgs->len);
>>> -
>>> -       if (count < 0)
>>> -               return count;
>>> -
>>>          ret = hid_hw_power(hdev, PM_HINT_FULLON);
>>>          if (ret < 0) {
>>>                  hid_err(hdev, "power management error: %d\n", ret);
>>>                  return ret;
>>>          }
>>>
>>> -       ret = cp2112_hid_output(hdev, buf, count, HID_OUTPUT_REPORT);
>>> -       if (ret < 0) {
>>> -               hid_warn(hdev, "Error starting transaction: %d\n", ret);
>>> -               goto power_normal;
>>> -       }
>>> +       for (m = msgs; m < msgs + num; m++) {
>>
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux