Hello Ondřej, On 1/2/23 11:57, Ondřej Jirman wrote: [...] >> >> You tell me, it is your patch :) I just cherry-picked this from your tree: > > I have other patches to goodix driver that do power off the touch sensor chip > during sleep, so that it doesn't consume excessinve amounts of power when > the phone is suspended. Mainline doesn't. You have to adapt this to mainline, > because you're not upstreaming the required Goodix patches, for regulator-off-in-suspend > to not break things. > >> https://github.com/megous/linux/commit/11f8da60d6a5 >> >> But if that is not correct, then I can drop the regulator-off-in-suspend. >> Ah, I see. Missed that. Then I guess that's better to drop the regulator-off-in-suspend until the goodix driver patches are upstreamed. >> [...] >> >>>> + >>>> + touchscreen@14 { >>>> + compatible = "goodix,gt917s"; >>> >>> This is not the correct compatible. Pinephone Pro uses Goodix GT1158: >>> >>> Goodix-TS 3-0014: ID 1158, version: 0100 >>> Goodix-TS 3-0014: Direct firmware load for goodix_1158_cfg.bin failed with error -2 >>> >>> >> >> Same thing. I wasn't aware of this since your patch was using this compatible >> string. If "goodix,gt1158" is the correct compatible string, then I agree we >> should have that instead even when the firmware is missing. Because the DT is >> supposed to describe the hardware. The FW issue can be tackled as a follow-up. >> >> [...] > > Yes, compatible string is sort of irrelevant, because the driver does runtime > auto-detection based on chip ID. I didn't bother with superficial issues in the > original code from Martijn/Kamil. Now that you're mainlining the code, this > should be sorted out, though. > > There's no FW issue, I was just using the log to show you the actual chip ID the > driver detects. > Gotcha. > (You should probably put my SoB after Kamil/Martijn, since I took the > maintenance/development of the driver after they wrote the base support > initially in secret. I'm not the original author of the code.) > I wasn't aware of that. I just kept the author field as it's in your tree. [...] >> https://github.com/megous/linux/commit/f19ce7bb7d72 > > Yes, and test the driver more thoroughly: > > - look at clk_summary to verify clock rate the kernel thinks it's using > - test refresh rate, somehow, to again verify the actual clock rate (kernel can > lie in debugfs) > - test power cycling the panel (eg. via system suspend/resume or other means) > Agreed that the more testing the better. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
