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

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

[Ellen Wang]

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-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
---
 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++) {
+		/*
+		 * If the top two messages are a write followed by a read,
+		 * then we do them together as CP2112_DATA_WRITE_READ_REQUEST.
+		 * Otherwise, process one message.
+		 */
+
+		if (m + 1 < msgs + num && m[0].addr == m[1].addr &&
+		    !(m[0].flags & I2C_M_RD) && (m[1].flags & I2C_M_RD)) {
+			hid_dbg(hdev, "I2C msg %zd write-read %#04x wlen %d rlen %d\n",
+				m - msgs, m[0].addr, m[0].len, m[1].len);
+			count = cp2112_i2c_write_read_req(buf, m[0].addr,
+					m[0].buf, m[0].len, m[1].len);
+			m++;
+		} else if (m->flags & I2C_M_RD) {
+			hid_dbg(hdev, "I2C msg %zd read %#04x len %d\n",
+				m - msgs, m->addr, m->len);
+			count = cp2112_read_req(buf, m->addr, m->len);
+		} else {
+			hid_dbg(hdev, "I2C msg %zd write %#04x len %d\n",
+				m - msgs, m->addr, m->len);
+			count = cp2112_i2c_write_req(buf, m->addr, m->buf,
+						     m->len);
+		}
 
-	for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
-		ret = cp2112_xfer_status(dev);
-		if (-EBUSY == ret)
-			continue;
-		if (ret < 0)
+		if (count < 0) {
+			ret = count;
 			goto power_normal;
-		break;
-	}
+		}
 
-	if (XFER_STATUS_RETRIES <= retries) {
-		hid_warn(hdev, "Transfer timed out, cancelling.\n");
-		buf[0] = CP2112_CANCEL_TRANSFER;
-		buf[1] = 0x01;
+		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;
+		}
 
-		ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);
-		if (ret < 0)
-			hid_warn(hdev, "Error cancelling transaction: %d\n",
-				 ret);
+		for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
+			ret = cp2112_xfer_status(dev);
+			if (-EBUSY == ret)
+				continue;
+			if (ret < 0)
+				goto power_normal;
+			break;
+		}
 
-		ret = -ETIMEDOUT;
-		goto power_normal;
-	}
+		if (XFER_STATUS_RETRIES <= retries) {
+			hid_warn(hdev, "Transfer timed out, cancelling.\n");
+			buf[0] = CP2112_CANCEL_TRANSFER;
+			buf[1] = 0x01;
 
-	if (!(msgs->flags & I2C_M_RD))
-		goto finish;
+			ret = cp2112_hid_output(hdev, buf, 2,
+						HID_OUTPUT_REPORT);
+			if (ret < 0)
+				hid_warn(hdev,
+					 "Error cancelling transaction: %d\n",
+					 ret);
 
-	ret = cp2112_read(dev, msgs->buf, msgs->len);
-	if (ret < 0)
-		goto power_normal;
-	if (ret != msgs->len) {
-		hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len);
-		ret = -EIO;
-		goto power_normal;
+			ret = -ETIMEDOUT;
+			goto power_normal;
+		}
+
+		if (!(m->flags & I2C_M_RD))
+			continue;
+
+		for (count = 0; count < m->len;) {
+			ret = cp2112_read(dev, m->buf + count, m->len - count);
+			if (ret < 0)
+				goto power_normal;
+			count += ret;
+			if (count > m->len) {
+				hid_warn(hdev, "long read: %d > %zd\n",
+					 ret, m->len - count + ret);
+			}
+		}
 	}
 
-finish:
 	/* return the number of transferred messages */
-	ret = 1;
+	ret = num;
 
 power_normal:
 	hid_hw_power(hdev, PM_HINT_NORMAL);
-- 
1.7.10.4

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

[Jiri Kosina]

On Thu, 23 Apr 2015, Ellen Wang 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-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>

David, could you please provide your Reviewed-by: / Acked-by: for this 
patch?

Thanks,

-- 
Jiri Kosina
SUSE Labs

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

[Antonio Borneo]

On Fri, Apr 24, 2015 at 5:49 AM, Ellen Wang <ellen@cumulusnetworks.com> 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@cumulusnetworks.com>

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

> +               /*
> +                * If the top two messages are a write followed by a read,
> +                * then we do them together as CP2112_DATA_WRITE_READ_REQUEST.
> +                * Otherwise, process one message.
> +                */
> +
> +               if (m + 1 < msgs + num && m[0].addr == m[1].addr &&
> +                   !(m[0].flags & I2C_M_RD) && (m[1].flags & I2C_M_RD)) {
> +                       hid_dbg(hdev, "I2C msg %zd write-read %#04x wlen %d rlen %d\n",
> +                               m - msgs, m[0].addr, m[0].len, m[1].len);
> +                       count = cp2112_i2c_write_read_req(buf, m[0].addr,
> +                                       m[0].buf, m[0].len, m[1].len);
> +                       m++;
> +               } else if (m->flags & I2C_M_RD) {
> +                       hid_dbg(hdev, "I2C msg %zd read %#04x len %d\n",
> +                               m - msgs, m->addr, m->len);
> +                       count = cp2112_read_req(buf, m->addr, m->len);
> +               } else {
> +                       hid_dbg(hdev, "I2C msg %zd write %#04x len %d\n",
> +                               m - msgs, m->addr, m->len);
> +                       count = cp2112_i2c_write_req(buf, m->addr, m->buf,
> +                                                    m->len);
> +               }
>
> -       for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
> -               ret = cp2112_xfer_status(dev);
> -               if (-EBUSY == ret)
> -                       continue;
> -               if (ret < 0)
> +               if (count < 0) {
> +                       ret = count;
>                         goto power_normal;
> -               break;
> -       }
> +               }
>
> -       if (XFER_STATUS_RETRIES <= retries) {
> -               hid_warn(hdev, "Transfer timed out, cancelling.\n");
> -               buf[0] = CP2112_CANCEL_TRANSFER;
> -               buf[1] = 0x01;
> +               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;
> +               }
>
> -               ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);
> -               if (ret < 0)
> -                       hid_warn(hdev, "Error cancelling transaction: %d\n",
> -                                ret);
> +               for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
> +                       ret = cp2112_xfer_status(dev);
> +                       if (-EBUSY == ret)
> +                               continue;
> +                       if (ret < 0)
> +                               goto power_normal;
> +                       break;
> +               }
>
> -               ret = -ETIMEDOUT;
> -               goto power_normal;
> -       }
> +               if (XFER_STATUS_RETRIES <= retries) {
> +                       hid_warn(hdev, "Transfer timed out, cancelling.\n");
> +                       buf[0] = CP2112_CANCEL_TRANSFER;
> +                       buf[1] = 0x01;
>
> -       if (!(msgs->flags & I2C_M_RD))
> -               goto finish;
> +                       ret = cp2112_hid_output(hdev, buf, 2,
> +                                               HID_OUTPUT_REPORT);
> +                       if (ret < 0)
> +                               hid_warn(hdev,
> +                                        "Error cancelling transaction: %d\n",
> +                                        ret);
>
> -       ret = cp2112_read(dev, msgs->buf, msgs->len);
> -       if (ret < 0)
> -               goto power_normal;
> -       if (ret != msgs->len) {
> -               hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len);
> -               ret = -EIO;
> -               goto power_normal;
> +                       ret = -ETIMEDOUT;
> +                       goto power_normal;
> +               }
> +
> +               if (!(m->flags & I2C_M_RD))
> +                       continue;
> +
> +               for (count = 0; count < m->len;) {
> +                       ret = cp2112_read(dev, m->buf + count, m->len - count);
> +                       if (ret < 0)
> +                               goto power_normal;
> +                       count += ret;
> +                       if (count > m->len) {
> +                               hid_warn(hdev, "long read: %d > %zd\n",
> +                                        ret, m->len - count + ret);
> +                       }
> +               }
>         }
>
> -finish:
>         /* return the number of transferred messages */
> -       ret = 1;
> +       ret = num;
>
>  power_normal:
>         hid_hw_power(hdev, PM_HINT_NORMAL);
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ellen Wang]

On 05/29/2015 09:28 AM, Antonio Borneo wrote:
> On Fri, Apr 24, 2015 at 5:49 AM, Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org> 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-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
>
> 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().

>> +               /*
>> +                * If the top two messages are a write followed by a read,
>> +                * then we do them together as CP2112_DATA_WRITE_READ_REQUEST.
>> +                * Otherwise, process one message.
>> +                */
>> +
>> +               if (m + 1 < msgs + num && m[0].addr == m[1].addr &&
>> +                   !(m[0].flags & I2C_M_RD) && (m[1].flags & I2C_M_RD)) {
>> +                       hid_dbg(hdev, "I2C msg %zd write-read %#04x wlen %d rlen %d\n",
>> +                               m - msgs, m[0].addr, m[0].len, m[1].len);
>> +                       count = cp2112_i2c_write_read_req(buf, m[0].addr,
>> +                                       m[0].buf, m[0].len, m[1].len);
>> +                       m++;
>> +               } else if (m->flags & I2C_M_RD) {
>> +                       hid_dbg(hdev, "I2C msg %zd read %#04x len %d\n",
>> +                               m - msgs, m->addr, m->len);
>> +                       count = cp2112_read_req(buf, m->addr, m->len);
>> +               } else {
>> +                       hid_dbg(hdev, "I2C msg %zd write %#04x len %d\n",
>> +                               m - msgs, m->addr, m->len);
>> +                       count = cp2112_i2c_write_req(buf, m->addr, m->buf,
>> +                                                    m->len);
>> +               }
>>
>> -       for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
>> -               ret = cp2112_xfer_status(dev);
>> -               if (-EBUSY == ret)
>> -                       continue;
>> -               if (ret < 0)
>> +               if (count < 0) {
>> +                       ret = count;
>>                          goto power_normal;
>> -               break;
>> -       }
>> +               }
>>
>> -       if (XFER_STATUS_RETRIES <= retries) {
>> -               hid_warn(hdev, "Transfer timed out, cancelling.\n");
>> -               buf[0] = CP2112_CANCEL_TRANSFER;
>> -               buf[1] = 0x01;
>> +               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;
>> +               }
>>
>> -               ret = cp2112_hid_output(hdev, buf, 2, HID_OUTPUT_REPORT);
>> -               if (ret < 0)
>> -                       hid_warn(hdev, "Error cancelling transaction: %d\n",
>> -                                ret);
>> +               for (retries = 0; retries < XFER_STATUS_RETRIES; ++retries) {
>> +                       ret = cp2112_xfer_status(dev);
>> +                       if (-EBUSY == ret)
>> +                               continue;
>> +                       if (ret < 0)
>> +                               goto power_normal;
>> +                       break;
>> +               }
>>
>> -               ret = -ETIMEDOUT;
>> -               goto power_normal;
>> -       }
>> +               if (XFER_STATUS_RETRIES <= retries) {
>> +                       hid_warn(hdev, "Transfer timed out, cancelling.\n");
>> +                       buf[0] = CP2112_CANCEL_TRANSFER;
>> +                       buf[1] = 0x01;
>>
>> -       if (!(msgs->flags & I2C_M_RD))
>> -               goto finish;
>> +                       ret = cp2112_hid_output(hdev, buf, 2,
>> +                                               HID_OUTPUT_REPORT);
>> +                       if (ret < 0)
>> +                               hid_warn(hdev,
>> +                                        "Error cancelling transaction: %d\n",
>> +                                        ret);
>>
>> -       ret = cp2112_read(dev, msgs->buf, msgs->len);
>> -       if (ret < 0)
>> -               goto power_normal;
>> -       if (ret != msgs->len) {
>> -               hid_warn(hdev, "short read: %d < %d\n", ret, msgs->len);
>> -               ret = -EIO;
>> -               goto power_normal;
>> +                       ret = -ETIMEDOUT;
>> +                       goto power_normal;
>> +               }
>> +
>> +               if (!(m->flags & I2C_M_RD))
>> +                       continue;
>> +
>> +               for (count = 0; count < m->len;) {
>> +                       ret = cp2112_read(dev, m->buf + count, m->len - count);
>> +                       if (ret < 0)
>> +                               goto power_normal;
>> +                       count += ret;
>> +                       if (count > m->len) {
>> +                               hid_warn(hdev, "long read: %d > %zd\n",
>> +                                        ret, m->len - count + ret);
>> +                       }
>> +               }
>>          }
>>
>> -finish:
>>          /* return the number of transferred messages */
>> -       ret = 1;
>> +       ret = num;
>>
>>   power_normal:
>>          hid_hw_power(hdev, PM_HINT_NORMAL);
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Antonio Borneo]

On Sat, May 30, 2015 at 1:32 AM, Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org> wrote:
>
>
> On 05/29/2015 09:28 AM, Antonio Borneo wrote:
>>
>> On Fri, Apr 24, 2015 at 5:49 AM, Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
>> 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-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
>>
>>
>> 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

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

[Ellen Wang]

On 05/29/2015 08:38 PM, Antonio Borneo wrote:
> On Sat, May 30, 2015 at 1:32 AM, Ellen Wang <ellen@cumulusnetworks.com> wrote:
>>
>> On 05/29/2015 09:28 AM, Antonio Borneo wrote:
>>>
>>> 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

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

[Antonio Borneo]

On Sat, May 30, 2015 at 12:58 PM, Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org> wrote:
> On 05/29/2015 08:38 PM, Antonio Borneo wrote:
>>
>> On Sat, May 30, 2015 at 1:32 AM, Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
>> wrote:
>>>
>>>
>>> On 05/29/2015 09:28 AM, Antonio Borneo wrote:
>>>>
>>>>
>>>> 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.

No, I don't have any other patches for cp2112. Would we great to
receive a V2 from you.

Thanks
Antonio

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

[Ellen Wang]

On 05/29/2015 10:23 PM, Antonio Borneo wrote:
> ...
> No, I don't have any other patches for cp2112. Would we great to
> receive a V2 from you.
>
> Thanks
> Antonio

Thank you.  It'll take a bit to test the new code (the hardware was put 
away because I was all done with it).