Hi JJ,

On Wed, Oct 3, 2012 at 5:08 AM, Jian-Jhong Ding [off-list ref] wrote:

Hi Benjamin,

I have one little question about __i2chid_command(), please see below.

"benjamin.tissoires" [off-list ref] writes:

quoted

From: Benjamin Tissoires <redacted>

Microsoft published the protocol specification of HID over i2c:
http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx

This patch introduces an implementation of this protocol.

This implementation does not includes the ACPI part of the specification.
This will come when ACPI 5.0 devices will be available.

Once the ACPI part will be done, OEM will not have to declare HID over I2C
devices in their platform specific driver.

Signed-off-by: Benjamin Tissoires <redacted>
---

[...] snipped

quoted

+     if (wait) {
+             spin_lock_irq(&ihid->flock);
+             set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+             spin_unlock_irq(&ihid->flock);
+     }
+
+     do {
+             ret = i2c_transfer(client->adapter, msg, msg_num);
+             if (ret > 0)
+                     break;
+             tries--;
+             dev_dbg(&client->dev, "retrying i2chid_command:%d (%d)\n",
+                     command, tries);
+     } while (tries > 0);

Do we have to do this retry here?  In i2c_transfer() it already does
retry automatically on arbitration loss (please see __i2c_transfer() in
drivers/i2c/i2c-core.c) that's defined by individual bus driver.  Maybe
we don't have to retry here and make the code a bit simpler.

Indeed, this retry is not necessary. I'll remove it in v2.
Thanks for the review.

Cheers,
Benjamin

quoted

+     if (wait && ret > 0) {
+             if (debug)
+                     dev_err(&client->dev, "%s: waiting....\n", __func__);
+             if (!wait_event_timeout(ihid->wait,
+                             !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
+                             msecs_to_jiffies(5000)))
+                     ret = -ENODATA;
+             if (debug)
+                     dev_err(&client->dev, "%s: finished.\n", __func__);
+     }
+
+     kfree(cmd);
+
+     return ret > 0 ? ret != msg_num : ret;
+}
+
+static int i2chid_command(struct i2c_client *client, int command,
+             unsigned char *buf_recv, int data_len)
+{
+     return __i2chid_command(client, command, 0, 0, NULL, 0,
+                             buf_recv, data_len);
+}