Re: [PATCH v2] HID: support i2c write-read and large transfers in hid-cp2112
I'll do that, and use the quirks mechanism, too. Stay tuned. On 06/18/2015 01:26 AM, Wolfram Sang wrote: Mine is rev 1 !!! Found also that in the datasheet chapter 12 reports the differences between the two revisions. In 12.1 it states that revision 1 does not implement repeated start. Perfect! I will try to replace my device with a newer one. Can this be checked in the code? -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] HID: support i2c write-read and large transfers in hid-cp2112
On Thu, Jun 18, 2015 at 4:26 PM, Wolfram Sang w...@the-dreams.de wrote: Mine is rev 1 !!! Found also that in the datasheet chapter 12 reports the differences between the two revisions. In 12.1 it states that revision 1 does not implement repeated start. Perfect! I will try to replace my device with a newer one. Can this be checked in the code? You are right! The code should check the revision and allow read-after-write for rev2 only. Or better, return error on rev1; we can expect any further revision will implement it correctly. Definitevy the revision can be retrieved by SW and used for the check. Today rev# is already retrieved during probe() and printed. Should be enough to keep it in a new field of the private date struct. Thanks for the hint. Antonio -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] HID: support i2c write-read and large transfers in hid-cp2112
On Thu, Jun 18, 2015 at 8:55 AM, Ellen Wang el...@cumulusnetworks.com wrote: ... Since the the long-read fix is less controversial, I've split it out and sent it as a separate patch. Hi Ellen, got it, will check soon As for repeated start, it's not that easy for me to check it. The chip is in a rack-mounted network switch. However, if you really want me to verify it, I can pull the box out and hook it up on a bench. The Silabs datasheet clearly states that the device is supposed to issue a repeated start (rev 1.2, http://www.silabs.com/Support%20Documents/TechnicalDocs/CP2112.pdf, figure 8 on page 15). Is your chip an older rev? Mine is rev 2 (but as you said, there would have been an erratum if they fixed a hardware bug). usb 1-1.2.3: New USB device found, idVendor=10c4, idProduct=ea90 usb 1-1.2.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 usb 1-1.2.3: Product: CP2112 HID USB-to-SMBus Bridge usb 1-1.2.3: Manufacturer: Silicon Laboratories usb 1-1.2.3: SerialNumber: 00343E9A cp2112 0003:10C4:EA90.0001: hidraw0: USB HID v1.01 Device [Silicon Laboratories CP2112 HID USB-to-SMBus Bridge] on usb-:00:16.0-1.2.3/input0 cp2112 0003:10C4:EA90.0001: Part Number: 0x0C Device Version: 0x02 Mine is rev 1 !!! Found also that in the datasheet chapter 12 reports the differences between the two revisions. In 12.1 it states that revision 1 does not implement repeated start. Perfect! I will try to replace my device with a newer one. In mean time, feel free to send the patch for write-read. I will check and test it ignoring the lack of repeated start on my obsolete HW. Thanks, Antonio -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] HID: support i2c write-read and large transfers in hid-cp2112
Mine is rev 1 !!! Found also that in the datasheet chapter 12 reports the differences between the two revisions. In 12.1 it states that revision 1 does not implement repeated start. Perfect! I will try to replace my device with a newer one. Can this be checked in the code? signature.asc Description: Digital signature
Re: [PATCH v2] HID: support i2c write-read and large transfers in hid-cp2112
On 06/15/2015 02:10 AM, Antonio Borneo wrote: On Sat, Jun 13, 2015 at 4:26 PM, Ellen Wang el...@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 breaks certain important cases. For example, the at24 eeprom driver generates paired write and read messages (for eeprom address and data). And the reads can be larger than 61 bytes. Since the device doesn't support i2c repeated starts in general, but does support a single write-repeated-start-read pair (as CP2112_DATA_WRITE_READ_REQUEST), we recognize the latter case and implement only that. To support large reads, we wrap a loop around cp2112_read() to pick up all the returned data. Hi Ellen, to keep git log consistent, the subject should change to something like HID: cp2112: ... OK. Will fix. Actually you are adding two features. I think should be better to split them in two independent patches. ... The part above goes in a patch for write-read, the part below in another patch for large transfers. I have tested you patch and works fine; I can read an I2C eeprom. But I also check with an oscilloscope the I2C signals and I got disappointed. There is no repeated START. I can clearly see cp2112 generating a STOP immediately followed by a START. Datasheet of cp2112 in figure 8 reports the expected time diagram with repeated START, but it's not what I get on my HW. Your code seam ok. Maybe my device is outdated or broken. The errata document from Silabs does not report anythink relevant. Can you verify with a scope on your HW too? Thanks, Antonio ... Since the the long-read fix is less controversial, I've split it out and sent it as a separate patch. As for repeated start, it's not that easy for me to check it. The chip is in a rack-mounted network switch. However, if you really want me to verify it, I can pull the box out and hook it up on a bench. The Silabs datasheet clearly states that the device is supposed to issue a repeated start (rev 1.2, http://www.silabs.com/Support%20Documents/TechnicalDocs/CP2112.pdf, figure 8 on page 15). Is your chip an older rev? Mine is rev 2 (but as you said, there would have been an erratum if they fixed a hardware bug). usb 1-1.2.3: New USB device found, idVendor=10c4, idProduct=ea90 usb 1-1.2.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 usb 1-1.2.3: Product: CP2112 HID USB-to-SMBus Bridge usb 1-1.2.3: Manufacturer: Silicon Laboratories usb 1-1.2.3: SerialNumber: 00343E9A cp2112 0003:10C4:EA90.0001: hidraw0: USB HID v1.01 Device [Silicon Laboratories CP2112 HID USB-to-SMBus Bridge] on usb-:00:16.0-1.2.3/input0 cp2112 0003:10C4:EA90.0001: Part Number: 0x0C Device Version: 0x02 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] HID: support i2c write-read and large transfers in hid-cp2112
On Sat, Jun 13, 2015 at 4:26 PM, Ellen Wang el...@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 breaks certain
important cases. For example, the at24 eeprom driver generates
paired write and read messages (for eeprom address and data).
And the reads can be larger than 61 bytes.
Since the device doesn't support i2c repeated starts in general,
but does support a single write-repeated-start-read pair
(as CP2112_DATA_WRITE_READ_REQUEST), we recognize the latter
case and implement only that.
To support large reads, we wrap a loop around cp2112_read()
to pick up all the returned data.
Hi Ellen,
to keep git log consistent, the subject should change to something like
HID: cp2112: ...
Actually you are adding two features.
I think should be better to split them in two independent patches.
Signed-off-by: Ellen Wang el...@cumulusnetworks.com
---
As Antonio Borneo and I discussed previously, this is the updated
patch that preserves the repeated start semantics (by not incorrectly
implementing cases that don't work).
Thank you for your time!
---
drivers/hid/hid-cp2112.c | 79
+-
1 file changed, 57 insertions(+), 22 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 3318de6..2c10a45 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -444,6 +444,24 @@ 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)
{
@@ -451,26 +469,45 @@ static int cp2112_i2c_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs,
struct hid_device *hdev = dev-hdev;
u8 buf[64];
ssize_t count;
+ ssize_t read_length = 0;
+ u8 *read_buf = NULL;
unsigned int retries;
int ret;
hid_dbg(hdev, I2C %d messages\n, num);
- if (num != 1) {
+ if (num == 1) {
+ if (msgs-flags I2C_M_RD) {
+ hid_dbg(hdev, I2C read %#04x len %d\n,
+ msgs-addr, msgs-len);
+ read_length = msgs-len;
+ read_buf = msgs-buf;
+ count = cp2112_read_req(buf, msgs-addr, msgs-len);
+ } else {
+ hid_dbg(hdev, I2C write %#04x len %d\n,
+ msgs-addr, msgs-len);
+ count = cp2112_i2c_write_req(buf, msgs-addr,
+msgs-buf, msgs-len);
+ }
+ if (count 0)
+ return count;
+ } else if (num == 2
+ msgs[0].addr == msgs[1].addr
+ !(msgs[0].flags I2C_M_RD) (msgs[1].flags I2C_M_RD))
{
+ hid_dbg(hdev, I2C write-read %#04x wlen %d rlen %d\n,
+ msgs[0].addr, msgs[0].len, msgs[1].len);
+ read_length = msgs[1].len;
+ read_buf = msgs[1].buf;
+ count = cp2112_i2c_write_read_req(buf, msgs[0].addr,
+ msgs[0].buf, msgs[0].len, msgs[1].len);
+ if (count 0)
+ return count;
+ } else {
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);
The part above goes in a patch for write-read, the part below in
another patch for large transfers.
I have tested you patch and works fine; I can read an I2C eeprom.
But I also check with an
[PATCH v2] HID: support i2c write-read and large transfers in hid-cp2112
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 breaks certain
important cases. For example, the at24 eeprom driver generates
paired write and read messages (for eeprom address and data).
And the reads can be larger than 61 bytes.
Since the device doesn't support i2c repeated starts in general,
but does support a single write-repeated-start-read pair
(as CP2112_DATA_WRITE_READ_REQUEST), we recognize the latter
case and implement only that.
To support large reads, we wrap a loop around cp2112_read()
to pick up all the returned data.
Signed-off-by: Ellen Wang el...@cumulusnetworks.com
---
As Antonio Borneo and I discussed previously, this is the updated
patch that preserves the repeated start semantics (by not incorrectly
implementing cases that don't work).
Thank you for your time!
---
drivers/hid/hid-cp2112.c | 79 +-
1 file changed, 57 insertions(+), 22 deletions(-)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 3318de6..2c10a45 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -444,6 +444,24 @@ 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)
{
@@ -451,26 +469,45 @@ static int cp2112_i2c_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs,
struct hid_device *hdev = dev-hdev;
u8 buf[64];
ssize_t count;
+ ssize_t read_length = 0;
+ u8 *read_buf = NULL;
unsigned int retries;
int ret;
hid_dbg(hdev, I2C %d messages\n, num);
- if (num != 1) {
+ if (num == 1) {
+ if (msgs-flags I2C_M_RD) {
+ hid_dbg(hdev, I2C read %#04x len %d\n,
+ msgs-addr, msgs-len);
+ read_length = msgs-len;
+ read_buf = msgs-buf;
+ count = cp2112_read_req(buf, msgs-addr, msgs-len);
+ } else {
+ hid_dbg(hdev, I2C write %#04x len %d\n,
+ msgs-addr, msgs-len);
+ count = cp2112_i2c_write_req(buf, msgs-addr,
+msgs-buf, msgs-len);
+ }
+ if (count 0)
+ return count;
+ } else if (num == 2
+ msgs[0].addr == msgs[1].addr
+ !(msgs[0].flags I2C_M_RD) (msgs[1].flags I2C_M_RD)) {
+ hid_dbg(hdev, I2C write-read %#04x wlen %d rlen %d\n,
+ msgs[0].addr, msgs[0].len, msgs[1].len);
+ read_length = msgs[1].len;
+ read_buf = msgs[1].buf;
+ count = cp2112_i2c_write_read_req(buf, msgs[0].addr,
+ msgs[0].buf, msgs[0].len, msgs[1].len);
+ if (count 0)
+ return count;
+ } else {
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);
@@ -506,21 +543,19 @@ static int cp2112_i2c_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs,
goto power_normal;
}
- if (!(msgs-flags I2C_M_RD))
- goto finish;
-
- 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;
+ for (count = 0; count read_length;) {
+ ret = cp2112_read(dev, read_buf +
