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-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
