| Message ID | 1399555929-10870-1-git-send-email-archana.patni@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Thu, May 8, 2014 at 9:32 AM, Archana Patni <archana.patni@linux.intel.com> wrote: > Some i2c_hid devices like a sensor hub have built-in fifos which > can accumulate input events in their buffers and trigger an interrupt > at end of a user defined event like fifo full or a timeout. The current > implementation reads one event at a time in the IRQ handler leading to > several hundred interrupts being necessary to flush the fifo. > > This patch changes the threaded IRQ handler to keep input events until > there are no input events left to read. In the normal case, this will > invoke one additional call to fetch an input event to check for no pending > events and terminate the loop. I must say, I don't like this patch. It seems to me that it will solve your problem but will break other devices. Nothing in the spec says what happens if the host tries to read while the interrupt line is *not* asserted. I can easily imagine several scenarios where the device would hang until the next available data, or will just fail. So the proper way according to the spec is to check the status of the interrupt line. This line is the responsibility of the device to be asserted and we should rely on it to know if we can read again. However, a quick check on the interrupt API did not provide me the function I would like, so I guess this is something to be work on. I can not ack this one until we can be sure not breaking other stuff. So: NACK. Cheers, Benjamin > > Signed-off-by: Archana Patni <archana.patni@intel.com> > Signed-off-by: Subramony Sesha <subramony.sesha@intel.com> > Reviewed-by: Mika Westerberg <mika.westerberg@intel.com> > Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@intel.com> > --- > drivers/hid/i2c-hid/i2c-hid.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c > index 21aafc8..3f09a50 100644 > --- a/drivers/hid/i2c-hid/i2c-hid.c > +++ b/drivers/hid/i2c-hid/i2c-hid.c > @@ -52,6 +52,7 @@ > static bool debug; > module_param(debug, bool, 0444); > MODULE_PARM_DESC(debug, "print a lot of debug information"); > +#define MAX_INPUT_EVENT_READ 100 > > #define i2c_hid_dbg(ihid, fmt, arg...) \ > do { \ > @@ -366,7 +367,7 @@ static int i2c_hid_hwreset(struct i2c_client *client) > return 0; > } > > -static void i2c_hid_get_input(struct i2c_hid *ihid) > +static int i2c_hid_get_input(struct i2c_hid *ihid) > { > int ret, ret_size; > int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); > @@ -374,11 +375,11 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) > ret = i2c_master_recv(ihid->client, ihid->inbuf, size); > if (ret != size) { > if (ret < 0) > - return; > + return ret; > > dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n", > __func__, ret, size); > - return; > + return -EIO; > } > > ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8; > @@ -387,13 +388,13 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) > /* host or device initiated RESET completed */ > if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) > wake_up(&ihid->wait); > - return; > + return 0; > } > > if (ret_size > size) { > dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n", > __func__, size, ret_size); > - return; > + return -EIO; > } > > i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf); > @@ -402,17 +403,22 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) > hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2, > ret_size - 2, 1); > > - return; > + return 1; > } > > static irqreturn_t i2c_hid_irq(int irq, void *dev_id) > { > struct i2c_hid *ihid = dev_id; > + int ret; > + int count = 0; > > if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) > return IRQ_HANDLED; > + do { > + count++; > + ret = i2c_hid_get_input(ihid); > > - i2c_hid_get_input(ihid); > + } while (count < MAX_INPUT_EVENT_READ && ret > 0); > > return IRQ_HANDLED; > } > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On May 8, 2014 3:28:23 PM GMT+01:00, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: >On Thu, May 8, 2014 at 9:32 AM, Archana Patni ><archana.patni@linux.intel.com> wrote: >> Some i2c_hid devices like a sensor hub have built-in fifos which >> can accumulate input events in their buffers and trigger an interrupt >> at end of a user defined event like fifo full or a timeout. The >current >> implementation reads one event at a time in the IRQ handler leading >to >> several hundred interrupts being necessary to flush the fifo. >> >> This patch changes the threaded IRQ handler to keep input events >until >> there are no input events left to read. In the normal case, this will >> invoke one additional call to fetch an input event to check for no >pending >> events and terminate the loop. > >I must say, I don't like this patch. >It seems to me that it will solve your problem but will break other >devices. Nothing in the spec says what happens if the host tries to >read while the interrupt line is *not* asserted. I can easily imagine >several scenarios where the device would hang until the next available >data, or will just fail. > >So the proper way according to the spec is to check the status of the >interrupt line. This line is the responsibility of the device to be >asserted and we should rely on it to know if we can read again. > >However, a quick check on the interrupt API did not provide me the >function I would like, so I guess this is something to be work on. In general it is not possible to check the interrupt state. Some interrupt devices simply don't provide any means of reading it. Generally a hybrid of a gpio and an interrupt is needed to do this. They don't technically need to be on the same pin though they often are... > >I can not ack this one until we can be sure not breaking other stuff. > >So: NACK. > >Cheers, >Benjamin > >> >> Signed-off-by: Archana Patni <archana.patni@intel.com> >> Signed-off-by: Subramony Sesha <subramony.sesha@intel.com> >> Reviewed-by: Mika Westerberg <mika.westerberg@intel.com> >> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@intel.com> >> --- >> drivers/hid/i2c-hid/i2c-hid.c | 20 +++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/hid/i2c-hid/i2c-hid.c >b/drivers/hid/i2c-hid/i2c-hid.c >> index 21aafc8..3f09a50 100644 >> --- a/drivers/hid/i2c-hid/i2c-hid.c >> +++ b/drivers/hid/i2c-hid/i2c-hid.c >> @@ -52,6 +52,7 @@ >> static bool debug; >> module_param(debug, bool, 0444); >> MODULE_PARM_DESC(debug, "print a lot of debug information"); >> +#define MAX_INPUT_EVENT_READ 100 >> >> #define i2c_hid_dbg(ihid, fmt, arg...) > \ >> do { > \ >> @@ -366,7 +367,7 @@ static int i2c_hid_hwreset(struct i2c_client >*client) >> return 0; >> } >> >> -static void i2c_hid_get_input(struct i2c_hid *ihid) >> +static int i2c_hid_get_input(struct i2c_hid *ihid) >> { >> int ret, ret_size; >> int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); >> @@ -374,11 +375,11 @@ static void i2c_hid_get_input(struct i2c_hid >*ihid) >> ret = i2c_master_recv(ihid->client, ihid->inbuf, size); >> if (ret != size) { >> if (ret < 0) >> - return; >> + return ret; >> >> dev_err(&ihid->client->dev, "%s: got %d data instead >of %d\n", >> __func__, ret, size); >> - return; >> + return -EIO; >> } >> >> ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8; >> @@ -387,13 +388,13 @@ static void i2c_hid_get_input(struct i2c_hid >*ihid) >> /* host or device initiated RESET completed */ >> if (test_and_clear_bit(I2C_HID_RESET_PENDING, >&ihid->flags)) >> wake_up(&ihid->wait); >> - return; >> + return 0; >> } >> >> if (ret_size > size) { >> dev_err(&ihid->client->dev, "%s: incomplete report >(%d/%d)\n", >> __func__, size, ret_size); >> - return; >> + return -EIO; >> } >> >> i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf); >> @@ -402,17 +403,22 @@ static void i2c_hid_get_input(struct i2c_hid >*ihid) >> hid_input_report(ihid->hid, HID_INPUT_REPORT, >ihid->inbuf + 2, >> ret_size - 2, 1); >> >> - return; >> + return 1; >> } >> >> static irqreturn_t i2c_hid_irq(int irq, void *dev_id) >> { >> struct i2c_hid *ihid = dev_id; >> + int ret; >> + int count = 0; >> >> if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) >> return IRQ_HANDLED; >> + do { >> + count++; >> + ret = i2c_hid_get_input(ihid); >> >> - i2c_hid_get_input(ihid); >> + } while (count < MAX_INPUT_EVENT_READ && ret > 0); >> >> return IRQ_HANDLED; >> } >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe >linux-input" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 8, 2014 at 2:10 PM, Jonathan Cameron <jic23@kernel.org> wrote: > > > On May 8, 2014 3:28:23 PM GMT+01:00, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: >>On Thu, May 8, 2014 at 9:32 AM, Archana Patni >><archana.patni@linux.intel.com> wrote: >>> Some i2c_hid devices like a sensor hub have built-in fifos which >>> can accumulate input events in their buffers and trigger an interrupt >>> at end of a user defined event like fifo full or a timeout. The >>current >>> implementation reads one event at a time in the IRQ handler leading >>to >>> several hundred interrupts being necessary to flush the fifo. >>> >>> This patch changes the threaded IRQ handler to keep input events >>until >>> there are no input events left to read. In the normal case, this will >>> invoke one additional call to fetch an input event to check for no >>pending >>> events and terminate the loop. >> >>I must say, I don't like this patch. >>It seems to me that it will solve your problem but will break other >>devices. Nothing in the spec says what happens if the host tries to >>read while the interrupt line is *not* asserted. I can easily imagine >>several scenarios where the device would hang until the next available >>data, or will just fail. >> >>So the proper way according to the spec is to check the status of the >>interrupt line. This line is the responsibility of the device to be >>asserted and we should rely on it to know if we can read again. >> >>However, a quick check on the interrupt API did not provide me the >>function I would like, so I guess this is something to be work on. > In general it is not possible to check the interrupt state. Some interrupt devices > simply don't provide any means of reading it. Generally a hybrid of a gpio and an > interrupt is needed to do this. They don't technically need to be on the same pin > though they often are... OK, that's what I was afraid of. :( Mika, you mentioned in the early tests that you had some devices not using GPIOs but ACPI IRQ. Are the latest ones still following this? Would it be costly to export the gpio alongside the irq for the devices not using a generic interrupt but a GPIO? If the gpio can be exported, then we can rely on it to get the other events. I don't know how much weight Microsoft put on the GPIO in the spec, but they definitively talk about GPIO. I hope that general interrupts regarding i2c_hid were just a temporary solution until ACPI5 devices were more widely used. So maybe they implemented this optimization for true GPIO interrupts (just guessing). Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 08, 2014 at 05:34:49PM -0400, Benjamin Tissoires wrote: > On Thu, May 8, 2014 at 2:10 PM, Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On May 8, 2014 3:28:23 PM GMT+01:00, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > >>On Thu, May 8, 2014 at 9:32 AM, Archana Patni > >><archana.patni@linux.intel.com> wrote: > >>> Some i2c_hid devices like a sensor hub have built-in fifos which > >>> can accumulate input events in their buffers and trigger an interrupt > >>> at end of a user defined event like fifo full or a timeout. The > >>current > >>> implementation reads one event at a time in the IRQ handler leading > >>to > >>> several hundred interrupts being necessary to flush the fifo. > >>> > >>> This patch changes the threaded IRQ handler to keep input events > >>until > >>> there are no input events left to read. In the normal case, this will > >>> invoke one additional call to fetch an input event to check for no > >>pending > >>> events and terminate the loop. > >> > >>I must say, I don't like this patch. > >>It seems to me that it will solve your problem but will break other > >>devices. Nothing in the spec says what happens if the host tries to > >>read while the interrupt line is *not* asserted. I can easily imagine > >>several scenarios where the device would hang until the next available > >>data, or will just fail. > >> > >>So the proper way according to the spec is to check the status of the > >>interrupt line. This line is the responsibility of the device to be > >>asserted and we should rely on it to know if we can read again. > >> > >>However, a quick check on the interrupt API did not provide me the > >>function I would like, so I guess this is something to be work on. > > In general it is not possible to check the interrupt state. Some interrupt devices > > simply don't provide any means of reading it. Generally a hybrid of a gpio and an > > interrupt is needed to do this. They don't technically need to be on the same pin > > though they often are... > > OK, that's what I was afraid of. :( > > Mika, you mentioned in the early tests that you had some devices not > using GPIOs but ACPI IRQ. Are the latest ones still following this? Yes. Typically it is always GPIO line but you can route it directly to ioapic with some magic done by BIOS. > Would it be costly to export the gpio alongside the irq for the > devices not using a generic interrupt but a GPIO? We have systems where you can select from BIOS menu whether GPIO or IRQ resource is passed. However, looks like vendors are passing IRQ only (just checked Asus T100 and there is no GPIO option at all). > If the gpio can be exported, then we can rely on it to get the other > events. Ideally, you would only have GPIO entry in the table and you could use it as both a GPIO and an IRQ. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benjamin, On 05/08/2014 07:28 AM, Benjamin Tissoires wrote: > On Thu, May 8, 2014 at 9:32 AM, Archana Patni > <archana.patni@linux.intel.com> wrote: >> Some i2c_hid devices like a sensor hub have built-in fifos which >> can accumulate input events in their buffers and trigger an interrupt >> at end of a user defined event like fifo full or a timeout. The current >> implementation reads one event at a time in the IRQ handler leading to >> several hundred interrupts being necessary to flush the fifo. >> >> This patch changes the threaded IRQ handler to keep input events until >> there are no input events left to read. In the normal case, this will >> invoke one additional call to fetch an input event to check for no pending >> events and terminate the loop. > I must say, I don't like this patch. > It seems to me that it will solve your problem but will break other > devices. Nothing in the spec says what happens if the host tries to > read while the interrupt line is *not* asserted. I can easily imagine > several scenarios where the device would hang until the next available > data, or will just fail. > > So the proper way according to the spec is to check the status of the > interrupt line. This line is the responsibility of the device to be > asserted and we should rely on it to know if we can read again. > > However, a quick check on the interrupt API did not provide me the > function I would like, so I guess this is something to be work on. > > I can not ack this one until we can be sure not breaking other stuff. > > So: NACK. I understand your reason, there may be some HID device which will have issue with this change. I am interested in throwing some idea, as I am trying to keep the devices in deepest idle states as long as possible. Sensors are one of the offenders. - This driver can register i2c_driver, command callback. We can define one of the command to set the set the max size dynamically or fire this logic to empty buffers. This way, we can only enable such logic, when a sensor hub has capability and notifies this capability dynamically without affecting any other hid devices. Thanks, Srinivas > Cheers, > Benjamin > >> Signed-off-by: Archana Patni <archana.patni@intel.com> >> Signed-off-by: Subramony Sesha <subramony.sesha@intel.com> >> Reviewed-by: Mika Westerberg <mika.westerberg@intel.com> >> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@intel.com> >> --- >> drivers/hid/i2c-hid/i2c-hid.c | 20 +++++++++++++------- >> 1 file changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c >> index 21aafc8..3f09a50 100644 >> --- a/drivers/hid/i2c-hid/i2c-hid.c >> +++ b/drivers/hid/i2c-hid/i2c-hid.c >> @@ -52,6 +52,7 @@ >> static bool debug; >> module_param(debug, bool, 0444); >> MODULE_PARM_DESC(debug, "print a lot of debug information"); >> +#define MAX_INPUT_EVENT_READ 100 >> >> #define i2c_hid_dbg(ihid, fmt, arg...) \ >> do { \ >> @@ -366,7 +367,7 @@ static int i2c_hid_hwreset(struct i2c_client *client) >> return 0; >> } >> >> -static void i2c_hid_get_input(struct i2c_hid *ihid) >> +static int i2c_hid_get_input(struct i2c_hid *ihid) >> { >> int ret, ret_size; >> int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); >> @@ -374,11 +375,11 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) >> ret = i2c_master_recv(ihid->client, ihid->inbuf, size); >> if (ret != size) { >> if (ret < 0) >> - return; >> + return ret; >> >> dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n", >> __func__, ret, size); >> - return; >> + return -EIO; >> } >> >> ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8; >> @@ -387,13 +388,13 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) >> /* host or device initiated RESET completed */ >> if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) >> wake_up(&ihid->wait); >> - return; >> + return 0; >> } >> >> if (ret_size > size) { >> dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n", >> __func__, size, ret_size); >> - return; >> + return -EIO; >> } >> >> i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf); >> @@ -402,17 +403,22 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) >> hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2, >> ret_size - 2, 1); >> >> - return; >> + return 1; >> } >> >> static irqreturn_t i2c_hid_irq(int irq, void *dev_id) >> { >> struct i2c_hid *ihid = dev_id; >> + int ret; >> + int count = 0; >> >> if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) >> return IRQ_HANDLED; >> + do { >> + count++; >> + ret = i2c_hid_get_input(ihid); >> >> - i2c_hid_get_input(ihid); >> + } while (count < MAX_INPUT_EVENT_READ && ret > 0); >> >> return IRQ_HANDLED; >> } >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-input" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Srinivas, On Sat, May 10, 2014 at 2:48 PM, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > Hi Benjamin, > > On 05/08/2014 07:28 AM, Benjamin Tissoires wrote: >> >> On Thu, May 8, 2014 at 9:32 AM, Archana Patni >> <archana.patni@linux.intel.com> wrote: >>> >>> Some i2c_hid devices like a sensor hub have built-in fifos which >>> can accumulate input events in their buffers and trigger an interrupt >>> at end of a user defined event like fifo full or a timeout. The current >>> implementation reads one event at a time in the IRQ handler leading to >>> several hundred interrupts being necessary to flush the fifo. >>> >>> This patch changes the threaded IRQ handler to keep input events until >>> there are no input events left to read. In the normal case, this will >>> invoke one additional call to fetch an input event to check for no >>> pending >>> events and terminate the loop. >> >> I must say, I don't like this patch. >> It seems to me that it will solve your problem but will break other >> devices. Nothing in the spec says what happens if the host tries to >> read while the interrupt line is *not* asserted. I can easily imagine >> several scenarios where the device would hang until the next available >> data, or will just fail. >> >> So the proper way according to the spec is to check the status of the >> interrupt line. This line is the responsibility of the device to be >> asserted and we should rely on it to know if we can read again. >> >> However, a quick check on the interrupt API did not provide me the >> function I would like, so I guess this is something to be work on. >> >> I can not ack this one until we can be sure not breaking other stuff. >> >> So: NACK. > > I understand your reason, there may be some HID device which will have issue > with this change. > I am interested in throwing some idea, as I am trying to keep the devices in > deepest idle states as > long as possible. Sensors are one of the offenders. > - This driver can register i2c_driver, command callback. We can define one > of the command to set the set the max size dynamically or fire this logic to > empty buffers. This way, we can only enable such logic, when a sensor hub > has capability and notifies this capability dynamically without affecting > any other hid devices. I am OK with the logic as long as the i2c calls are independent of the HID layer. Remember that if the advertised bus is BUS_I2C, it might as well be an uhid device which will not handle the i2c calls. Another solution could be to add a HID quirk which triggers the "empty while input are here" and set this quirk in hid-sensor-hub. This way, you will be in control of it and other devices might use it as well if they support it. I am not a big fan of quirks, so I would still prefer your solution if you can control it without disturbing the HID subsystem. This will all depend where you can enable the functionality on the device. Cheers, Benjamin > > Thanks, > Srinivas > > >> Cheers, >> Benjamin >> >>> Signed-off-by: Archana Patni <archana.patni@intel.com> >>> Signed-off-by: Subramony Sesha <subramony.sesha@intel.com> >>> Reviewed-by: Mika Westerberg <mika.westerberg@intel.com> >>> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@intel.com> >>> --- >>> drivers/hid/i2c-hid/i2c-hid.c | 20 +++++++++++++------- >>> 1 file changed, 13 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c >>> b/drivers/hid/i2c-hid/i2c-hid.c >>> index 21aafc8..3f09a50 100644 >>> --- a/drivers/hid/i2c-hid/i2c-hid.c >>> +++ b/drivers/hid/i2c-hid/i2c-hid.c >>> @@ -52,6 +52,7 @@ >>> static bool debug; >>> module_param(debug, bool, 0444); >>> MODULE_PARM_DESC(debug, "print a lot of debug information"); >>> +#define MAX_INPUT_EVENT_READ 100 >>> >>> #define i2c_hid_dbg(ihid, fmt, arg...) >>> \ >>> do { >>> \ >>> @@ -366,7 +367,7 @@ static int i2c_hid_hwreset(struct i2c_client *client) >>> return 0; >>> } >>> >>> -static void i2c_hid_get_input(struct i2c_hid *ihid) >>> +static int i2c_hid_get_input(struct i2c_hid *ihid) >>> { >>> int ret, ret_size; >>> int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); >>> @@ -374,11 +375,11 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) >>> ret = i2c_master_recv(ihid->client, ihid->inbuf, size); >>> if (ret != size) { >>> if (ret < 0) >>> - return; >>> + return ret; >>> >>> dev_err(&ihid->client->dev, "%s: got %d data instead of >>> %d\n", >>> __func__, ret, size); >>> - return; >>> + return -EIO; >>> } >>> >>> ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8; >>> @@ -387,13 +388,13 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) >>> /* host or device initiated RESET completed */ >>> if (test_and_clear_bit(I2C_HID_RESET_PENDING, >>> &ihid->flags)) >>> wake_up(&ihid->wait); >>> - return; >>> + return 0; >>> } >>> >>> if (ret_size > size) { >>> dev_err(&ihid->client->dev, "%s: incomplete report >>> (%d/%d)\n", >>> __func__, size, ret_size); >>> - return; >>> + return -EIO; >>> } >>> >>> i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf); >>> @@ -402,17 +403,22 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) >>> hid_input_report(ihid->hid, HID_INPUT_REPORT, >>> ihid->inbuf + 2, >>> ret_size - 2, 1); >>> >>> - return; >>> + return 1; >>> } >>> >>> static irqreturn_t i2c_hid_irq(int irq, void *dev_id) >>> { >>> struct i2c_hid *ihid = dev_id; >>> + int ret; >>> + int count = 0; >>> >>> if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) >>> return IRQ_HANDLED; >>> + do { >>> + count++; >>> + ret = i2c_hid_get_input(ihid); >>> >>> - i2c_hid_get_input(ihid); >>> + } while (count < MAX_INPUT_EVENT_READ && ret > 0); >>> >>> return IRQ_HANDLED; >>> } >>> -- >>> 1.7.9.5 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-input" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-input" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 05/12/2014 09:03 AM, Benjamin Tissoires wrote: >> Hi Srinivas, >> >> On Sat, May 10, 2014 at 2:48 PM, Srinivas Pandruvada >> <srinivas.pandruvada@linux.intel.com> wrote: >>> Hi Benjamin, >>> >>> On 05/08/2014 07:28 AM, Benjamin Tissoires wrote: >>>> On Thu, May 8, 2014 at 9:32 AM, Archana Patni >>>> <archana.patni@linux.intel.com> wrote: >>>>> Some i2c_hid devices like a sensor hub have built-in fifos which >>>>> can accumulate input events in their buffers and trigger an interrupt >>>>> at end of a user defined event like fifo full or a timeout. The >>>>> current >>>>> implementation reads one event at a time in the IRQ handler leading >>>>> to >>>>> several hundred interrupts being necessary to flush the fifo. >>>>> >>>>> This patch changes the threaded IRQ handler to keep input events >>>>> until >>>>> there are no input events left to read. In the normal case, this will >>>>> invoke one additional call to fetch an input event to check for no >>>>> pending >>>>> events and terminate the loop. >>>> I must say, I don't like this patch. >>>> It seems to me that it will solve your problem but will break other >>>> devices. Nothing in the spec says what happens if the host tries to >>>> read while the interrupt line is *not* asserted. I can easily imagine >>>> several scenarios where the device would hang until the next available >>>> data, or will just fail. >>>> >>>> So the proper way according to the spec is to check the status of the >>>> interrupt line. This line is the responsibility of the device to be >>>> asserted and we should rely on it to know if we can read again. >>>> >>>> However, a quick check on the interrupt API did not provide me the >>>> function I would like, so I guess this is something to be work on. >>>> >>>> I can not ack this one until we can be sure not breaking other stuff. >>>> >>>> So: NACK. >>> I understand your reason, there may be some HID device which will have >>> issue >>> with this change. >>> I am interested in throwing some idea, as I am trying to keep the >>> devices in >>> deepest idle states as >>> long as possible. Sensors are one of the offenders. >>> - This driver can register i2c_driver, command callback. We can define >>> one >>> of the command to set the set the max size dynamically or fire this >>> logic to >>> empty buffers. This way, we can only enable such logic, when a sensor >>> hub >>> has capability and notifies this capability dynamically without >>> affecting >>> any other hid devices. >> I am OK with the logic as long as the i2c calls are independent of the >> HID layer. Remember that if the advertised bus is BUS_I2C, it might as >> well be an uhid device which will not handle the i2c calls. >> >> Another solution could be to add a HID quirk which triggers the "empty >> while input are here" and set this quirk in hid-sensor-hub. This way, >> you will be in control of it and other devices might use it as well if >> they support it. >> >> I am not a big fan of quirks, so I would still prefer your solution if >> you can control it without disturbing the HID subsystem. This will all >> depend where you can enable the functionality on the device. > I also don't like quirks as they start becoming de facto solutions. > > Archana, > > Can you prototype this using i2c_client_commands by keeping above > constraints? > > Thanks, > Srinivas Benjamin, The i2c_clients_command callback hook seems to be deprecated and not widely used. We had a quick check with Srinivas and he asked us to fall back to a quirk based approach. We will create a device specific quirk (using vendor ID) which sets up a batched read IRQ handler if it sees a device capable of supporting batch reads from the fifo. In the normal case, the current IRQ handler will continue to be used. As more devices support this feature, they can turn on the batched handler via the quirk. Does this look ok to you ? Thanks Archana > > >> Cheers, >> Benjamin >> >>> Thanks, >>> Srinivas >>> >>> >>>> Cheers, >>>> Benjamin >>>> >>>>> Signed-off-by: Archana Patni <archana.patni@intel.com> >>>>> Signed-off-by: Subramony Sesha <subramony.sesha@intel.com> >>>>> Reviewed-by: Mika Westerberg <mika.westerberg@intel.com> >>>>> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@intel.com> >>>>> --- >>>>> drivers/hid/i2c-hid/i2c-hid.c | 20 +++++++++++++------- >>>>> 1 file changed, 13 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c >>>>> b/drivers/hid/i2c-hid/i2c-hid.c >>>>> index 21aafc8..3f09a50 100644 >>>>> --- a/drivers/hid/i2c-hid/i2c-hid.c >>>>> +++ b/drivers/hid/i2c-hid/i2c-hid.c >>>>> @@ -52,6 +52,7 @@ >>>>> static bool debug; >>>>> module_param(debug, bool, 0444); >>>>> MODULE_PARM_DESC(debug, "print a lot of debug information"); >>>>> +#define MAX_INPUT_EVENT_READ 100 >>>>> >>>>> #define i2c_hid_dbg(ihid, fmt, arg...) >>>>> \ >>>>> do { >>>>> \ >>>>> @@ -366,7 +367,7 @@ static int i2c_hid_hwreset(struct i2c_client >>>>> *client) >>>>> return 0; >>>>> } >>>>> >>>>> -static void i2c_hid_get_input(struct i2c_hid *ihid) >>>>> +static int i2c_hid_get_input(struct i2c_hid *ihid) >>>>> { >>>>> int ret, ret_size; >>>>> int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); >>>>> @@ -374,11 +375,11 @@ static void i2c_hid_get_input(struct i2c_hid >>>>> *ihid) >>>>> ret = i2c_master_recv(ihid->client, ihid->inbuf, size); >>>>> if (ret != size) { >>>>> if (ret < 0) >>>>> - return; >>>>> + return ret; >>>>> >>>>> dev_err(&ihid->client->dev, "%s: got %d data >>>>> instead of >>>>> %d\n", >>>>> __func__, ret, size); >>>>> - return; >>>>> + return -EIO; >>>>> } >>>>> >>>>> ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8; >>>>> @@ -387,13 +388,13 @@ static void i2c_hid_get_input(struct i2c_hid >>>>> *ihid) >>>>> /* host or device initiated RESET completed */ >>>>> if (test_and_clear_bit(I2C_HID_RESET_PENDING, >>>>> &ihid->flags)) >>>>> wake_up(&ihid->wait); >>>>> - return; >>>>> + return 0; >>>>> } >>>>> >>>>> if (ret_size > size) { >>>>> dev_err(&ihid->client->dev, "%s: incomplete report >>>>> (%d/%d)\n", >>>>> __func__, size, ret_size); >>>>> - return; >>>>> + return -EIO; >>>>> } >>>>> >>>>> i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf); >>>>> @@ -402,17 +403,22 @@ static void i2c_hid_get_input(struct i2c_hid >>>>> *ihid) >>>>> hid_input_report(ihid->hid, HID_INPUT_REPORT, >>>>> ihid->inbuf + 2, >>>>> ret_size - 2, 1); >>>>> >>>>> - return; >>>>> + return 1; >>>>> } >>>>> >>>>> static irqreturn_t i2c_hid_irq(int irq, void *dev_id) >>>>> { >>>>> struct i2c_hid *ihid = dev_id; >>>>> + int ret; >>>>> + int count = 0; >>>>> >>>>> if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) >>>>> return IRQ_HANDLED; >>>>> + do { >>>>> + count++; >>>>> + ret = i2c_hid_get_input(ihid); >>>>> >>>>> - i2c_hid_get_input(ihid); >>>>> + } while (count < MAX_INPUT_EVENT_READ && ret > 0); >>>>> >>>>> return IRQ_HANDLED; >>>>> } >>>>> -- >>>>> 1.7.9.5 >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe >>>>> linux-input" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-input" >>>> in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-input" >> in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 14, 2014 at 7:04 AM, <archana.patni@linux.intel.com> wrote: >> On 05/12/2014 09:03 AM, Benjamin Tissoires wrote: >>> Hi Srinivas, >>> >>> On Sat, May 10, 2014 at 2:48 PM, Srinivas Pandruvada >>> <srinivas.pandruvada@linux.intel.com> wrote: >>>> Hi Benjamin, >>>> >>>> On 05/08/2014 07:28 AM, Benjamin Tissoires wrote: >>>>> On Thu, May 8, 2014 at 9:32 AM, Archana Patni >>>>> <archana.patni@linux.intel.com> wrote: >>>>>> Some i2c_hid devices like a sensor hub have built-in fifos which >>>>>> can accumulate input events in their buffers and trigger an interrupt >>>>>> at end of a user defined event like fifo full or a timeout. The >>>>>> current >>>>>> implementation reads one event at a time in the IRQ handler leading >>>>>> to >>>>>> several hundred interrupts being necessary to flush the fifo. >>>>>> >>>>>> This patch changes the threaded IRQ handler to keep input events >>>>>> until >>>>>> there are no input events left to read. In the normal case, this will >>>>>> invoke one additional call to fetch an input event to check for no >>>>>> pending >>>>>> events and terminate the loop. >>>>> I must say, I don't like this patch. >>>>> It seems to me that it will solve your problem but will break other >>>>> devices. Nothing in the spec says what happens if the host tries to >>>>> read while the interrupt line is *not* asserted. I can easily imagine >>>>> several scenarios where the device would hang until the next available >>>>> data, or will just fail. >>>>> >>>>> So the proper way according to the spec is to check the status of the >>>>> interrupt line. This line is the responsibility of the device to be >>>>> asserted and we should rely on it to know if we can read again. >>>>> >>>>> However, a quick check on the interrupt API did not provide me the >>>>> function I would like, so I guess this is something to be work on. >>>>> >>>>> I can not ack this one until we can be sure not breaking other stuff. >>>>> >>>>> So: NACK. >>>> I understand your reason, there may be some HID device which will have >>>> issue >>>> with this change. >>>> I am interested in throwing some idea, as I am trying to keep the >>>> devices in >>>> deepest idle states as >>>> long as possible. Sensors are one of the offenders. >>>> - This driver can register i2c_driver, command callback. We can define >>>> one >>>> of the command to set the set the max size dynamically or fire this >>>> logic to >>>> empty buffers. This way, we can only enable such logic, when a sensor >>>> hub >>>> has capability and notifies this capability dynamically without >>>> affecting >>>> any other hid devices. >>> I am OK with the logic as long as the i2c calls are independent of the >>> HID layer. Remember that if the advertised bus is BUS_I2C, it might as >>> well be an uhid device which will not handle the i2c calls. >>> >>> Another solution could be to add a HID quirk which triggers the "empty >>> while input are here" and set this quirk in hid-sensor-hub. This way, >>> you will be in control of it and other devices might use it as well if >>> they support it. >>> >>> I am not a big fan of quirks, so I would still prefer your solution if >>> you can control it without disturbing the HID subsystem. This will all >>> depend where you can enable the functionality on the device. >> I also don't like quirks as they start becoming de facto solutions. >> >> Archana, >> >> Can you prototype this using i2c_client_commands by keeping above >> constraints? >> >> Thanks, >> Srinivas > > Benjamin, > > The i2c_clients_command callback hook seems to be deprecated and not > widely used. We had a quick check with Srinivas and he asked us to fall > back to a quirk based approach. > > We will create a device specific quirk (using vendor ID) which sets up a > batched read IRQ handler if it sees a device capable of supporting batch > reads from the fifo. In the normal case, the current IRQ handler will > continue to be used. As more devices support this feature, they can turn > on the batched handler via the quirk. > > Does this look ok to you ? Yep, fine by me. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 21aafc8..3f09a50 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -52,6 +52,7 @@ static bool debug; module_param(debug, bool, 0444); MODULE_PARM_DESC(debug, "print a lot of debug information"); +#define MAX_INPUT_EVENT_READ 100 #define i2c_hid_dbg(ihid, fmt, arg...) \ do { \ @@ -366,7 +367,7 @@ static int i2c_hid_hwreset(struct i2c_client *client) return 0; } -static void i2c_hid_get_input(struct i2c_hid *ihid) +static int i2c_hid_get_input(struct i2c_hid *ihid) { int ret, ret_size; int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); @@ -374,11 +375,11 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) ret = i2c_master_recv(ihid->client, ihid->inbuf, size); if (ret != size) { if (ret < 0) - return; + return ret; dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n", __func__, ret, size); - return; + return -EIO; } ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8; @@ -387,13 +388,13 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) /* host or device initiated RESET completed */ if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) wake_up(&ihid->wait); - return; + return 0; } if (ret_size > size) { dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n", __func__, size, ret_size); - return; + return -EIO; } i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf); @@ -402,17 +403,22 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2, ret_size - 2, 1); - return; + return 1; } static irqreturn_t i2c_hid_irq(int irq, void *dev_id) { struct i2c_hid *ihid = dev_id; + int ret; + int count = 0; if (test_bit(I2C_HID_READ_PENDING, &ihid->flags)) return IRQ_HANDLED; + do { + count++; + ret = i2c_hid_get_input(ihid); - i2c_hid_get_input(ihid); + } while (count < MAX_INPUT_EVENT_READ && ret > 0); return IRQ_HANDLED; }