mbox series

[v2,0/7] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

Message ID 20240507144821.12275-1-johan+linaro@kernel.org
Headers show
Series HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on | expand

Message

Johan Hovold May 7, 2024, 2:48 p.m. UTC
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.

Changes in v2
 - drop redundant 'items' from binding
 - include a "should" in description of 'no-reset-on-power-off' property
 - always assert reset on probe
 - enable elan i2c-hid driver in arm64 defconfig

Johan Hovold (7):
  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
  arm64: defconfig: enable Elan i2c-hid driver

 .../bindings/input/elan,ekth6915.yaml         | 19 ++++--
 .../bindings/input/ilitek,ili2901.yaml        | 66 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts     |  3 +-
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 15 +++--
 arch/arm64/configs/defconfig                  |  1 +
 drivers/hid/i2c-hid/i2c-hid-of-elan.c         | 59 +++++++++++++----
 6 files changed, 137 insertions(+), 26 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml

Comments

Johan Hovold June 5, 2024, 12:25 p.m. UTC | #1
Hi Jiri and Benjamin,

On Tue, May 07, 2024 at 04:48:14PM +0200, Johan Hovold 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 devicetree changes are expected to go in through the Qualcomm tree
> once the binding and driver updates have been merged.

> [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 (7):
>   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

Could you consider picking the first four patches up for 6.10-rc3 so
that Bjorn can take the devicetree changes?

>   arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
>   arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset
>   arm64: defconfig: enable Elan i2c-hid driver

Johan
Benjamin Tissoires June 6, 2024, 6:57 a.m. UTC | #2
On Jun 05 2024, Johan Hovold wrote:
> Hi Jiri and Benjamin,
> 
> On Tue, May 07, 2024 at 04:48:14PM +0200, Johan Hovold 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 devicetree changes are expected to go in through the Qualcomm tree
> > once the binding and driver updates have been merged.
> 
> > [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 (7):
> >   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
> 
> Could you consider picking the first four patches up for 6.10-rc3 so
> that Bjorn can take the devicetree changes?

We definitely can. But if it makes things easier, Bjorn can also take
the whole series through his tree with my Acked-by.

If I don't get answer by tomorrow I'll apply the first 4 in the hid
tree.

Cheers,
Benjamin

> 
> >   arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
> >   arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset
> >   arm64: defconfig: enable Elan i2c-hid driver
> 
> Johan
Johan Hovold June 6, 2024, 7:10 a.m. UTC | #3
On Thu, Jun 06, 2024 at 08:57:47AM +0200, Benjamin Tissoires wrote:
> On Jun 05 2024, Johan Hovold wrote:
> > Hi Jiri and Benjamin,
> > 
> > On Tue, May 07, 2024 at 04:48:14PM +0200, Johan Hovold wrote:

> > > Johan Hovold (7):
> > >   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
> > 
> > Could you consider picking the first four patches up for 6.10-rc3 so
> > that Bjorn can take the devicetree changes?
> 
> We definitely can. But if it makes things easier, Bjorn can also take
> the whole series through his tree with my Acked-by.

Thanks, but it should be fine to take this through two different trees.

It will probably take a little longer to get the DT changes into
mainline anyway as they will also go through the SoC tree.

> > >   arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
> > >   arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset
> > >   arm64: defconfig: enable Elan i2c-hid driver

Johan
Benjamin Tissoires June 7, 2024, 9:19 a.m. UTC | #4
On Tue, 07 May 2024 16:48:14 +0200, Johan Hovold 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]
> 
> [...]

Applied to hid/hid.git (for-6.10/upstream-fixes), thanks!

[1/7] dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema
      https://git.kernel.org/hid/hid/c/8d3ae46c6433
[2/7] dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M
      https://git.kernel.org/hid/hid/c/07fc16fa5552
[3/7] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property
      https://git.kernel.org/hid/hid/c/e538d4b85b8f
[4/7] HID: i2c-hid: elan: fix reset suspend current leakage
      https://git.kernel.org/hid/hid/c/0eafc58f2194

Cheers,
Benjamin Tissoires June 7, 2024, 9:24 a.m. UTC | #5
On Jun 06 2024, Johan Hovold wrote:
> On Thu, Jun 06, 2024 at 08:57:47AM +0200, Benjamin Tissoires wrote:
> > On Jun 05 2024, Johan Hovold wrote:
> > > Hi Jiri and Benjamin,
> > > 
> > > On Tue, May 07, 2024 at 04:48:14PM +0200, Johan Hovold wrote:
> 
> > > > Johan Hovold (7):
> > > >   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
> > > 
> > > Could you consider picking the first four patches up for 6.10-rc3 so
> > > that Bjorn can take the devicetree changes?
> > 
> > We definitely can. But if it makes things easier, Bjorn can also take
> > the whole series through his tree with my Acked-by.
> 
> Thanks, but it should be fine to take this through two different trees.
> 
> It will probably take a little longer to get the DT changes into
> mainline anyway as they will also go through the SoC tree.

Alright, fair enough. I've applied them, and will let them sink in
for-next for 24h. I'll send the PR to Linus tomorrow evening. No
guarantees they'll make -rc3 (but they should be applied early next week
unless I messed something big).

Cheers,
Benjamin
Bjorn Andersson June 7, 2024, 6:48 p.m. UTC | #6
On Tue, 07 May 2024 16:48:14 +0200, Johan Hovold 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]
> 
> [...]

Applied, thanks!

[7/7] arm64: defconfig: enable Elan i2c-hid driver
      commit: e706474d8428f420bba11f2c49c3083fd1b31d88

Best regards,