Hi JJ, On Wed, Oct 3, 2012 at 5:08 AM, Jian-Jhong Ding <jj_ding@xxxxxxxxxx> wrote: > Hi Benjamin, > > I have one little question about __i2chid_command(), please see below. > > "benjamin.tissoires" <benjamin.tissoires@xxxxxxxxx> writes: >> From: Benjamin Tissoires <benjamin.tissoires@xxxxxxx> >> >> 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 <benjamin.tissoires@xxxxxxx> >> --- >> [...] snipped >> + 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 >> + 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); >> +} -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
