From mboxrd@z Thu Jan 1 00:00:00 1970 From: Srinivas Pandruvada Subject: Re: [PATCH] HID: i2c-hid: Support batch read of input events from fifo Date: Sat, 10 May 2014 11:48:19 -0700 Message-ID: <536E7473.4080502@linux.intel.com> References: <1399555929-10870-1-git-send-email-archana.patni@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com ([134.134.136.20]:41432 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752029AbaEJSsV (ORCPT ); Sat, 10 May 2014 14:48:21 -0400 In-Reply-To: Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Benjamin Tissoires , Archana Patni Cc: Jiri Kosina , jic23@kernel.org, linux-input , Mika Westerberg , "Pandruvada, Srinivas" , Archana Patni , Subramony Sesha Hi Benjamin, On 05/08/2014 07:28 AM, Benjamin Tissoires wrote: > On Thu, May 8, 2014 at 9:32 AM, Archana Patni > 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 >> Signed-off-by: Subramony Sesha >> Reviewed-by: Mika Westerberg >> Reviewed-by: Srinivas Pandruvada >> --- >> 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 >