| Message ID | 20240423134611.31979-1-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on | expand |
Hi Johan, On Tue, Apr 23, 2024 at 8:47 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay > before sending commands after having deasserted reset during power on. > > This series switches the X13s devicetree to use the Elan specific > binding so that the OS can determine the required power-on sequence and > make sure that the controller is always detected during boot. [1] > > The Elan hid-i2c driver currently asserts reset unconditionally during > suspend, which does not work on the X13s where the touch controller > supply is shared with other peripherals that may remain powered. Holding > the controller in reset can increase power consumption and also leaks > current through the reset circuitry pull ups. > > Note that the latter also affects X13s variants where the touchscreen is > not populated as the driver also exits probe() with reset asserted. > > Fix this by adding a new 'no-reset-on-power-off' devicetree property > which can be used by the OS to determine when reset needs to be asserted > on power down and when it safe and desirable to leave it deasserted. > > I tried to look for drivers that had already addressed this but it was > only after I finished implementing this that I noticed Doug's reference > to commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to > true state of the regulator"), which tried to solve a related problem. > > That commit has since been reverted but ultimately resulted in commit > 7607f12ba735 ("HID: i2c-hid: goodix: Add support for > "goodix,no-reset-during-suspend" property") being merged to handle the > related case where the touch controller supply is always on. > > The implementation is very similar, but I decided to use the slightly > more generic 'no-reset-on-power-off' property name after considering a > number of alternatives (including trying to describe the hardware > configuration in the name). (And as this is not vendor specific, I left > out the prefix.) > > Note that my X13s does not have a touchscreen, but I have done partial > verification of the implementation using that machine and the sc8280xp > CRD reference design. Bjorn has promised to help out with final > verification on an X13s with a touchscreen. > > The devicetree changes are expected to go in through the Qualcomm tree > once the binding and driver updates have been merged. > > Johan > > > [1] The reset signal is currently deasserted using the pin configuration > and the controller would be detected if probe is deferred or if user > space triggers a reprobe through sysfs. > > > Johan Hovold (6): > dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema > dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M > dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property > HID: i2c-hid: elan: fix reset suspend current leakage > arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on > arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset > > .../bindings/input/elan,ekth6915.yaml | 20 ++++-- > .../bindings/input/ilitek,ili2901.yaml | 66 +++++++++++++++++++ > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 3 +- > .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 15 +++-- > drivers/hid/i2c-hid/i2c-hid-of-elan.c | 37 ++++++++--- > 5 files changed, 118 insertions(+), 23 deletions(-) > create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml > > -- > 2.43.2 > > I thought that I'd purchased a Thinkpad X13s without touchscreen, but it turns out that I do have one, and since I do, I was able to test this patchset, and it works on mine. Tested-by: Steev Klimaszewski <steev@kali.org>
Hi, On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay > before sending commands after having deasserted reset during power on. > > This series switches the X13s devicetree to use the Elan specific > binding so that the OS can determine the required power-on sequence and > make sure that the controller is always detected during boot. [1] > > The Elan hid-i2c driver currently asserts reset unconditionally during > suspend, which does not work on the X13s where the touch controller > supply is shared with other peripherals that may remain powered. Holding > the controller in reset can increase power consumption and also leaks > current through the reset circuitry pull ups. Can you provide more details about which devices exactly it shares power with? I'm worried that you may be shooting yourself in the foot to avoid shooting yourself in the arm. Specifically, if those other peripherals that may remain powered ever power themselves off then you'll end up back-driving the touchscreen through the reset line, won't you? Since reset is active low then not asserting reset drives the reset line high and, if you power it off, it can leach power backwards through the reset line. The "goodix,no-reset-during-suspend" property that I added earlier specifically worked on systems where the rail was always-on so I could guarantee that didn't happen. From looking at your dts patch it looks like your power _is_ on an always-on rail so you should be OK, but it should be documented that this only works for always-on rails. ...also, from your patch description it sounds as if (maybe?) you intend to eventually let the rail power off if the trackpad isn't a wakeup source. If you eventually plan to do that then you definitely need something more complex here... > Note that the latter also affects X13s variants where the touchscreen is > not populated as the driver also exits probe() with reset asserted. I assume driving against an external pull is _probably_ not a huge deal (should be a pretty small amount of power), but I agree it would be nice to fix. I'm a bit leery of actively driving the reset pin high (deasserting the reset) just to match the pull. It feels like in your case it would be better to make it an input w/ no pulls. It almost feels like something in the pinctrl system should handle this. Something where the pin is default "input no pull" at the board level and when the driver exits it should go back to the pinctrl default... I guess one last thought is: what do we do if/when someone needs the same solution but they want multiple sources of touchscreens, assuming we ever get the second-sourcing problem solved well. In that case the different touchscreen drivers might have a different idea of how the GPIO should be left when the driver exits... -Doug
On Tue, Apr 23, 2024 at 02:34:20PM -0500, Steev Klimaszewski wrote: > On Tue, Apr 23, 2024 at 8:47 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > Note that my X13s does not have a touchscreen, but I have done partial > > verification of the implementation using that machine and the sc8280xp > > CRD reference design. Bjorn has promised to help out with final > > verification on an X13s with a touchscreen. > I thought that I'd purchased a Thinkpad X13s without touchscreen, but > it turns out that I do have one, and since I do, I was able to test > this patchset, and it works on mine. > > Tested-by: Steev Klimaszewski <steev@kali.org> Thanks for testing, Steev. Johan
On Tue, Apr 23, 2024 at 01:36:18PM -0700, Doug Anderson wrote: > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay > > before sending commands after having deasserted reset during power on. > > > > This series switches the X13s devicetree to use the Elan specific > > binding so that the OS can determine the required power-on sequence and > > make sure that the controller is always detected during boot. [1] > > > > The Elan hid-i2c driver currently asserts reset unconditionally during > > suspend, which does not work on the X13s where the touch controller > > supply is shared with other peripherals that may remain powered. Holding > > the controller in reset can increase power consumption and also leaks > > current through the reset circuitry pull ups. > > Can you provide more details about which devices exactly it shares > power with? I'm worried that you may be shooting yourself in the foot > to avoid shooting yourself in the arm. > > Specifically, if those other peripherals that may remain powered ever > power themselves off then you'll end up back-driving the touchscreen > through the reset line, won't you? Since reset is active low then not > asserting reset drives the reset line high and, if you power it off, > it can leach power backwards through the reset line. The > "goodix,no-reset-during-suspend" property that I added earlier > specifically worked on systems where the rail was always-on so I could > guarantee that didn't happen. > > From looking at your dts patch it looks like your power _is_ on an > always-on rail so you should be OK, but it should be documented that > this only works for always-on rails. > > ..also, from your patch description it sounds as if (maybe?) you > intend to eventually let the rail power off if the trackpad isn't a > wakeup source. If you eventually plan to do that then you definitely > need something more complex here... No, that's the whole point: the hardware is designed so that the reset line can be left deasserted by the CPU also when the supply is off. The supply in this case is shared with the keyboard and touchpad, but also some other devices which are not yet fully described. As you rightly noted, the intention is to allow the supply to eventually be disabled when none of these devices are enabled as wakeup sources. I did not want to get in to too much details on exactly how this particular reset circuit is designed, but basically you have a pull up to an always-on 1.8 V rail on the CPU side, a FET level shifter, and a pull up to the supply voltage on the peripheral side. With this design, the reset line can be left deasserted by the CPU (tri-stated or driven high), but the important part is that the reset signal that goes into the controller will be pulled to 3.3 V only when the supply is left on and otherwise it will be connected to ground. > > Note that the latter also affects X13s variants where the touchscreen is > > not populated as the driver also exits probe() with reset asserted. > > I assume driving against an external pull is _probably_ not a huge > deal (should be a pretty small amount of power), but I agree it would > be nice to fix. > > I'm a bit leery of actively driving the reset pin high (deasserting > the reset) just to match the pull. It feels like in your case it would > be better to make it an input w/ no pulls. It almost feels like > something in the pinctrl system should handle this. Something where > the pin is default "input no pull" at the board level and when the > driver exits it should go back to the pinctrl default... If you look at the DT patch that's essentially what I'm doing by describing the reset pin as open-drain so that it will be configured as an input (tristated) when reset is deasserted and only driven low when reset is asserted. > I guess one last thought is: what do we do if/when someone needs the > same solution but they want multiple sources of touchscreens, assuming > we ever get the second-sourcing problem solved well. In that case the > different touchscreen drivers might have a different idea of how the > GPIO should be left when the driver exits... The second-source problem is arguable a separate one, and as we've discussed in the past, the current approach of describing both devices in the devicetree only works when the devices are truly compatible in terms of external resources (supplies, gpios, pinconfig). For anything more complex, we need a more elaborate implementation. In this case it should not be a problem, though, as the reset circuit should have the same properties regardless of which controller you connect (e.g. both nodes would have the 'no-reset-on-power-off' property). Johan
Hi, On Wed, Apr 24, 2024 at 1:50 AM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Apr 23, 2024 at 01:36:18PM -0700, Doug Anderson wrote: > > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote: > > > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay > > > before sending commands after having deasserted reset during power on. > > > > > > This series switches the X13s devicetree to use the Elan specific > > > binding so that the OS can determine the required power-on sequence and > > > make sure that the controller is always detected during boot. [1] > > > > > > The Elan hid-i2c driver currently asserts reset unconditionally during > > > suspend, which does not work on the X13s where the touch controller > > > supply is shared with other peripherals that may remain powered. Holding > > > the controller in reset can increase power consumption and also leaks > > > current through the reset circuitry pull ups. > > > > Can you provide more details about which devices exactly it shares > > power with? I'm worried that you may be shooting yourself in the foot > > to avoid shooting yourself in the arm. > > > > Specifically, if those other peripherals that may remain powered ever > > power themselves off then you'll end up back-driving the touchscreen > > through the reset line, won't you? Since reset is active low then not > > asserting reset drives the reset line high and, if you power it off, > > it can leach power backwards through the reset line. The > > "goodix,no-reset-during-suspend" property that I added earlier > > specifically worked on systems where the rail was always-on so I could > > guarantee that didn't happen. > > > > From looking at your dts patch it looks like your power _is_ on an > > always-on rail so you should be OK, but it should be documented that > > this only works for always-on rails. > > > > ..also, from your patch description it sounds as if (maybe?) you > > intend to eventually let the rail power off if the trackpad isn't a > > wakeup source. If you eventually plan to do that then you definitely > > need something more complex here... > > No, that's the whole point: the hardware is designed so that the reset > line can be left deasserted by the CPU also when the supply is off. > > The supply in this case is shared with the keyboard and touchpad, but > also some other devices which are not yet fully described. As you > rightly noted, the intention is to allow the supply to eventually be > disabled when none of these devices are enabled as wakeup sources. > > I did not want to get in to too much details on exactly how this > particular reset circuit is designed, but basically you have a pull up > to an always-on 1.8 V rail on the CPU side, a FET level shifter, and a > pull up to the supply voltage on the peripheral side. > > With this design, the reset line can be left deasserted by the CPU > (tri-stated or driven high), but the important part is that the reset > signal that goes into the controller will be pulled to 3.3 V only when > the supply is left on and otherwise it will be connected to ground. Ah, got it. The level shifter isolating things makes sense. > > I guess one last thought is: what do we do if/when someone needs the > > same solution but they want multiple sources of touchscreens, assuming > > we ever get the second-sourcing problem solved well. In that case the > > different touchscreen drivers might have a different idea of how the > > GPIO should be left when the driver exits... > > The second-source problem is arguable a separate one, and as we've > discussed in the past, the current approach of describing both devices > in the devicetree only works when the devices are truly compatible in > terms of external resources (supplies, gpios, pinconfig). For anything > more complex, we need a more elaborate implementation. > > In this case it should not be a problem, though, as the reset circuit > should have the same properties regardless of which controller you > connect (e.g. both nodes would have the 'no-reset-on-power-off' > property). The reset circuitry may be the same, but the properties of the touchscreen might not be. It would be easy to imagine a different touchscreen that consumes less power when held in reset. In any case, not a problem we need to solve right now. -Doug
The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay before sending commands after having deasserted reset during power on. This series switches the X13s devicetree to use the Elan specific binding so that the OS can determine the required power-on sequence and make sure that the controller is always detected during boot. [1] The Elan hid-i2c driver currently asserts reset unconditionally during suspend, which does not work on the X13s where the touch controller supply is shared with other peripherals that may remain powered. Holding the controller in reset can increase power consumption and also leaks current through the reset circuitry pull ups. Note that the latter also affects X13s variants where the touchscreen is not populated as the driver also exits probe() with reset asserted. Fix this by adding a new 'no-reset-on-power-off' devicetree property which can be used by the OS to determine when reset needs to be asserted on power down and when it safe and desirable to leave it deasserted. I tried to look for drivers that had already addressed this but it was only after I finished implementing this that I noticed Doug's reference to commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the reset line to true state of the regulator"), which tried to solve a related problem. That commit has since been reverted but ultimately resulted in commit 7607f12ba735 ("HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property") being merged to handle the related case where the touch controller supply is always on. The implementation is very similar, but I decided to use the slightly more generic 'no-reset-on-power-off' property name after considering a number of alternatives (including trying to describe the hardware configuration in the name). (And as this is not vendor specific, I left out the prefix.) Note that my X13s does not have a touchscreen, but I have done partial verification of the implementation using that machine and the sc8280xp CRD reference design. Bjorn has promised to help out with final verification on an X13s with a touchscreen. The devicetree changes are expected to go in through the Qualcomm tree once the binding and driver updates have been merged. Johan [1] The reset signal is currently deasserted using the pin configuration and the controller would be detected if probe is deferred or if user space triggers a reprobe through sysfs. Johan Hovold (6): dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property HID: i2c-hid: elan: fix reset suspend current leakage arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset .../bindings/input/elan,ekth6915.yaml | 20 ++++-- .../bindings/input/ilitek,ili2901.yaml | 66 +++++++++++++++++++ arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 3 +- .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 15 +++-- drivers/hid/i2c-hid/i2c-hid-of-elan.c | 37 ++++++++--- 5 files changed, 118 insertions(+), 23 deletions(-) create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml