On Jan 17, 2019, at 20:35, Benjamin Tissoires [off-list ref] wrote:

On Thu, Jan 17, 2019 at 12:41 PM Kai-Heng Feng
[off-list ref] wrote:

quoted

quoted

On Jan 17, 2019, at 16:06, Benjamin Tissoires [off-list ref] wrote:

On Thu, Jan 17, 2019 at 6:02 AM Kai-Heng Feng
[off-list ref] wrote:

[snipped]

quoted

quoted

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.

quoted

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