Re: [PATCH v1] HID: cp2112: support i2c write-read transfers in hid-cp2112

[Ellen Wang <ellen@xxxxxxxxxxxxxxxxxxx>]

In-line replies below:

On 06/20/2015 08:41 AM, Antonio Borneo wrote:
> 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.

I know.  It's a matter of taste whether to use a natural type or a 
size-specific size.  I perfectly fine using u8 to save 4 bytes.

> 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

Thanks!

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