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
Hi Johan,
On Tue, Apr 23, 2024 at 8:47 AM Johan Hovold <[email protected]> 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 <[email protected]>
Hi,
On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <[email protected]> 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 <[email protected]> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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