Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Kai-Heng Feng]

> On Jan 17, 2019, at 20:35, Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote:
> 
> On Thu, Jan 17, 2019 at 12:41 PM Kai-Heng Feng
> <kai.heng.feng@xxxxxxxxxxxxx> wrote:
>> 
>> 
>> 
>>> On Jan 17, 2019, at 16:06, Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote:
>>> 
>>> On Thu, Jan 17, 2019 at 6:02 AM Kai-Heng Feng
>>> <kai.heng.feng@xxxxxxxxxxxxx> wrote:
>> [snipped]
>>>> Goodix says the firmware needs at least 60ms to fully respond ON and
>>>> SLEEP command.
>>> 
>>> I was about to say that this is not conformant to the specification.
>>> But looking at 7.2.8 SET_POWER of
>>> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
>>> 
>>> They say:
>>> "The DEVICE must ensure that it transitions to the HOST specified
>>> Power State in under 1 second. "
>>> But in the RESPONSE paragraph: "The DEVICE shall not respond back
>>> after receiving the command. The DEVICE is mandated to enter that
>>> power state imminently."
>>> 
>>> My interpretation was always that the device has to be in a ready
>>> state for new commands "immediately".
>>> However, the first sentence may suggest that the driver would block
>>> any command to the device before the 1 second timestamp.
>>> 
>>>> 
>>>> Iâll see if an 200ms autosuspend can solve all Goodix, LG and Raydium
>>>> touchpanels.
>>> 
>>> We already have a I2C_HID_QUIRK_DELAY_AFTER_SLEEP quirk that delay
>>> operations after sleep by 20ms. Maybe we can keep the runtime PM but
>>> use the same timers to not confuse the hardware.
>> 
>> Yes, but wouldnât use pm_*_autosuspend() helpers can both solve the issue
>> and make the code more cleaner?
> 
> You are probably correct :)
> 
> I am not too familiar with the details of the runtime PM API, but if
> you can make this a barrier to prevent the host to call too many
> suspends/resumes in a row, that would be nice.
> And we might be able to ditch I2C_HID_QUIRK_DELAY_AFTER_SLEEP and
> I2C_HID_QUIRK_NO_RUNTIME_PM.

Ok, using autosuspend helpers doesnât really solve the issue.

Although it can cover the case like fast ON/SLEEP triggered by
quick transition of display manager -> desktop session, but once
it gets runtime suspended, we can never sure when itâll get 
runtime resumed again. So a mleep() between each powering
commands is still needed.

So I think we should stick to I2C_HID_QUIRK_NO_RUNTIME_PM
for now.

Kai-Heng

> 
> Adding the involved people in the thread.
> 
> Cheers,
> Benjamin