> 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
