On Wed, May 14, 2014 at 7:04 AM, <archana.patni@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
