On Thu, Jun 18, 2015 at 5:55 PM, Ellen Wang <ellen@xxxxxxxxxxxxxxxxxxx> wrote:
> cp2112_i2c_xfer() only supports a single i2c_msg. More than
> one message at a time just returns EIO. This breaks certain
> important cases. For example, the at24 eeprom driver generates
> paired write and read messages (for eeprom address and data).
>
> Since the device doesn't support i2c repeated starts in general,
> but does support a single write-repeated-start-read pair
> (since hardware rev 1), we recognize the latter case and
> implement only that.
Hi Ellen,
I have ordered some device rev2, but shipment will take 2 weeks.
Do you mind if I delay testing this patch till I get them?
In mean time, one comment below.
> Signed-off-by: Ellen Wang <ellen@xxxxxxxxxxxxxxxxxxx>
> ---
> drivers/hid/hid-cp2112.c | 74 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 55 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 5a72819..00d062a 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -153,6 +153,7 @@ MODULE_DEVICE_TABLE(hid, cp2112_devices);
> struct cp2112_device {
> struct i2c_adapter adap;
> struct hid_device *hdev;
> + int hwversion;
No need for int; u8 is enough (value is copyed from buf[2] that is u8).
Put the new u8 field few lines below, together with the other u8, to
avoid extra padding.
No need to send immediately a new version. Let's see if there is any
other comment and if someone can test it before me.
Thanks,
Antonio
> wait_queue_head_t wait;
> u8 read_data[61];
> u8 read_length;
> @@ -444,6 +445,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 +470,46 @@ 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 (dev->hwversion > 1 && /* no repeated start in rev 1 */
> + 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,15 +545,12 @@ static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> goto power_normal;
> }
>
> - if (!(msgs->flags & I2C_M_RD))
> - goto finish;
> -
> - for (count = 0; count < msgs->len;) {
> - ret = cp2112_read(dev, msgs->buf + count, msgs->len - count);
> + for (count = 0; count < read_length;) {
> + ret = cp2112_read(dev, read_buf + count, read_length - count);
> if (ret < 0)
> goto power_normal;
> count += ret;
> - if (count > msgs->len) {
> + if (count > read_length) {
> /*
> * The hardware returned too much data.
> * This is mostly harmless because cp2112_read()
> @@ -524,15 +560,14 @@ static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> * it shouldn't go unnoticed.
> */
> hid_err(hdev, "long read: %d > %zd\n",
> - ret, msgs->len - count + ret);
> + ret, read_length - count + ret);
> ret = -EIO;
> goto power_normal;
> }
> }
>
> -finish:
> /* return the number of transferred messages */
> - ret = 1;
> + ret = num;
>
> power_normal:
> hid_hw_power(hdev, PM_HINT_NORMAL);
> @@ -1040,6 +1075,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> dev->adap.dev.parent = &hdev->dev;
> snprintf(dev->adap.name, sizeof(dev->adap.name),
> "CP2112 SMBus Bridge on hiddev%d", hdev->minor);
> + dev->hwversion = buf[2];
> init_waitqueue_head(&dev->wait);
>
> hid_device_io_start(hdev);
> --
> 1.7.10.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
