[PATCH 0/2] arm64: dts: qcom: sc8280xp-x13s: Enable touchscreen

[PATCH 0/2] arm64: dts: qcom: sc8280xp-x13s: Enable touchscreen

[Bjorn Andersson]

This documents and defines the necessary properties for the I2C
HID-based touchscreen found in some SKUs of the Lenovo Thinkpad X13s to
work.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Bjorn Andersson (2):
      dt-bindings: HID: i2c-hid: Document reset-related properties
      arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

 Documentation/devicetree/bindings/input/hid-over-i2c.yaml  | 6 ++++++
 arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 +++++-
 2 files changed, 11 insertions(+), 1 deletion(-)
---
base-commit: 8bf1262c53f50fa91fe15d01e5ef5629db55313c
change-id: 20240125-x13s-touchscreen-48012ff3c24e

Best regards,
-- 
Bjorn Andersson <quic_bjorande@quicinc.com>

[PATCH 1/2] dt-bindings: HID: i2c-hid: Document reset-related properties

[Bjorn Andersson]

Some I2C HID devices has a reset pin and requires that some specified
time elapses after this reset pin is deasserted, before communication
with the device is attempted.

The Linux implementation is looking for these in the "reset-gpios" and
"post-reset-deassert-delay-ms" properties already, so use these property
names.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
index 138caad96a29..f07ff4cb3d26 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
@@ -50,6 +50,12 @@ properties:
     description: Time required by the device after enabling its regulators
       or powering it on, before it is ready for communication.
 
+  post-reset-deassert-delay-ms:
+    description: Time required by the device after reset has been deasserted,
+      before it is ready for communication.
+
+  reset-gpios: true
+
   touchscreen-inverted-x: true
 
   touchscreen-inverted-y: true

-- 
2.25.1

[PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Bjorn Andersson]

The failing read-test in __i2c_hid_core_probe() determines that there's
nothing connected at the documented address of the touchscreen.

Introduce the 5ms after-power and 200ms after-reset delays found in the
ACPI tables. Also wire up the reset-gpio, for good measure.

Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index def3976bd5bb..d64d0e76c1ea 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -620,7 +620,6 @@ &i2c4 {
 
 	status = "okay";
 
-	/* FIXME: verify */
 	touchscreen@10 {
 		compatible = "hid-over-i2c";
 		reg = <0x10>;
@@ -630,6 +629,11 @@ touchscreen@10 {
 		vdd-supply = <&vreg_misc_3p3>;
 		vddl-supply = <&vreg_s10b>;
 
+		reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+		post-power-on-delay-ms = <5>;
+		post-reset-deassert-delay-ms = <200>;
+
 		pinctrl-names = "default";
 		pinctrl-0 = <&ts0_default>;
 	};

-- 
2.25.1

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Johan Hovold]

On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> The failing read-test in __i2c_hid_core_probe() determines that there's
> nothing connected at the documented address of the touchscreen.
> 
> Introduce the 5ms after-power and 200ms after-reset delays found in the
> ACPI tables. Also wire up the reset-gpio, for good measure.

As the supplies for the touchscreen are always on (and left on by the
bootloader) it would seem that it is really the addition of the reset
gpio which makes things work here. Unless the delay is needed for some
other reason.

(The power-on delay also looks a bit short compared to what is used for
other devices.)

Reset support was only recently added with commit 2be404486c05 ("HID:
i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
backport this one before first determining that.

That commit also added a comment in the HID driver about the
'post-reset-deassert-delay-ms' to the driver which should now be
removed:

	/*
	 * Note this is a kernel internal device-property set by x86 platform code,
	 * this MUST not be used in devicetree files without first adding it to
	 * the DT bindings.
	 */

Johan

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Daniel Thompson]

On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > The failing read-test in __i2c_hid_core_probe() determines that there's
> > nothing connected at the documented address of the touchscreen.
> >
> > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > ACPI tables. Also wire up the reset-gpio, for good measure.
>
> As the supplies for the touchscreen are always on (and left on by the
> bootloader) it would seem that it is really the addition of the reset
> gpio which makes things work here. Unless the delay is needed for some
> other reason.
>
> (The power-on delay also looks a bit short compared to what is used for
> other devices.)
>
> Reset support was only recently added with commit 2be404486c05 ("HID:
> i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> backport this one before first determining that.

This comment attracted my attention so I tried booting with each of the
three lines individually.


On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> +             reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;

This is not enough, on it's own, to get the touch screen running.

I guess that's not so much of a surprise since the rebind-the-driver
from userspace trick wouldn't have been touching this reset.


> +             post-power-on-delay-ms = <5>;

This line alone is enough (in v6.7.1).


> +             post-reset-deassert-delay-ms = <200>;

This line alone is also enough!

In short it looks like the delays make the difference and, even a short
delay, can fix the problem.

Of course, regardless of the line-by-line results I also ran with all
the changes so, FWIW:
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Johan Hovold]

On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > The failing read-test in __i2c_hid_core_probe() determines that there's
> > > nothing connected at the documented address of the touchscreen.
> > >
> > > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > > ACPI tables. Also wire up the reset-gpio, for good measure.
> >
> > As the supplies for the touchscreen are always on (and left on by the
> > bootloader) it would seem that it is really the addition of the reset
> > gpio which makes things work here. Unless the delay is needed for some
> > other reason.
> >
> > (The power-on delay also looks a bit short compared to what is used for
> > other devices.)
> >
> > Reset support was only recently added with commit 2be404486c05 ("HID:
> > i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> > backport this one before first determining that.
> 
> This comment attracted my attention so I tried booting with each of the
> three lines individually.
> 
> On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > +             reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> 
> This is not enough, on it's own, to get the touch screen running.
> 
> I guess that's not so much of a surprise since the rebind-the-driver
> from userspace trick wouldn't have been touching this reset.

Right, I realised that after hitting send.

For the record, people have successfully been using the touchpad after
forcing the driver to reprobe through sysfs:

	echo 4-0010 >/sys/bus/i2c/drivers/i2c_hid_of/bind

> > +             post-power-on-delay-ms = <5>;
> 
> This line alone is enough (in v6.7.1).

Thanks for confirming.

> > +             post-reset-deassert-delay-ms = <200>;
> 
> This line alone is also enough!

Yes, the driver honours this delay regardless of whether a reset gpio is
defined currently, so this is expected.

> In short it looks like the delays make the difference and, even a short
> delay, can fix the problem.

Right, but since the suppliers are left enabled by the bootloader (and
never disabled by the kernel), that only begs the question of why this
makes a difference.

Without the delay, the other HID devices are probing (successfully)
slightly before, but essentially in parallel with the touchscreen while
using the same resources. Is that causing trouble somehow?

Or is there a bug in the i2c controller driver affecting only this
device that can be worked around by adding a delay before the first
transfer?

Johan

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Bjorn Andersson]

On Fri, Jan 26, 2024 at 03:31:02PM +0100, Johan Hovold wrote:
> On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> > On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> > > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > > The failing read-test in __i2c_hid_core_probe() determines that there's
> > > > nothing connected at the documented address of the touchscreen.
> > > >
> > > > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > > > ACPI tables. Also wire up the reset-gpio, for good measure.
> > >
> > > As the supplies for the touchscreen are always on (and left on by the
> > > bootloader) it would seem that it is really the addition of the reset
> > > gpio which makes things work here. Unless the delay is needed for some
> > > other reason.
> > >
> > > (The power-on delay also looks a bit short compared to what is used for
> > > other devices.)
> > >
> > > Reset support was only recently added with commit 2be404486c05 ("HID:
> > > i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> > > backport this one before first determining that.
> > 
> > This comment attracted my attention so I tried booting with each of the
> > three lines individually.
> > 
> > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > +             reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> > 
> > This is not enough, on it's own, to get the touch screen running.
> > 
> > I guess that's not so much of a surprise since the rebind-the-driver
> > from userspace trick wouldn't have been touching this reset.
> 
> Right, I realised that after hitting send.
> 
> For the record, people have successfully been using the touchpad after
> forcing the driver to reprobe through sysfs:
> 
> 	echo 4-0010 >/sys/bus/i2c/drivers/i2c_hid_of/bind
> 
> > > +             post-power-on-delay-ms = <5>;
> > 
> > This line alone is enough (in v6.7.1).
> 
> Thanks for confirming.
> 
> > > +             post-reset-deassert-delay-ms = <200>;
> > 
> > This line alone is also enough!
> 
> Yes, the driver honours this delay regardless of whether a reset gpio is
> defined currently, so this is expected.
> 
> > In short it looks like the delays make the difference and, even a short
> > delay, can fix the problem.
> 
> Right, but since the suppliers are left enabled by the bootloader (and
> never disabled by the kernel), that only begs the question of why this
> makes a difference.
> 

You're right, the supply is kept on by other things, so this isn't the
problem.

> Without the delay, the other HID devices are probing (successfully)
> slightly before, but essentially in parallel with the touchscreen while
> using the same resources. Is that causing trouble somehow?
> 

The difference to those other HID devices is GPIO 99 - the reset pin,
which is configured pull down input from boot - i.e. the chip is held in
reset.

When the HID device is being probed, pinctrl applies &ts0_default starts
driving it high, bringing the device out of reset. But insufficient time
is given for the chip to come up so the I2C read fails.

If you later try to probe again, 200ms has elapsed since the reset was
deasserted (driven high).

Regards,
Bjorn

> Or is there a bug in the i2c controller driver affecting only this
> device that can be worked around by adding a delay before the first
> transfer?
> 
> Johan

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Johan Hovold]

On Fri, Jan 26, 2024 at 06:53:46AM -0800, Bjorn Andersson wrote:
> On Fri, Jan 26, 2024 at 03:31:02PM +0100, Johan Hovold wrote:
> > On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:

> > > In short it looks like the delays make the difference and, even a short
> > > delay, can fix the problem.
> > 
> > Right, but since the suppliers are left enabled by the bootloader (and
> > never disabled by the kernel), that only begs the question of why this
> > makes a difference.
> 
> You're right, the supply is kept on by other things, so this isn't the
> problem.
> 
> > Without the delay, the other HID devices are probing (successfully)
> > slightly before, but essentially in parallel with the touchscreen while
> > using the same resources. Is that causing trouble somehow?
> 
> The difference to those other HID devices is GPIO 99 - the reset pin,
> which is configured pull down input from boot - i.e. the chip is held in
> reset.
> 
> When the HID device is being probed, pinctrl applies &ts0_default starts
> driving it high, bringing the device out of reset. But insufficient time
> is given for the chip to come up so the I2C read fails.

Ah, that's it.

You should drop that 'output-high' from the pin config as part of this
patch to avoid toggling the reset line twice at boot.

Looks like we have the same problem on the CRD as well. There the
touchscreen still works, possibly because it has been enabled by the
boot firmware or simply because that touchscreen can handle a shorter
delay.

Where exactly did you find those delay values in the ACPI tables? I
couldn't seem to find anything in the decompiled DSDT.

> If you later try to probe again, 200ms has elapsed since the reset was
> deasserted (driven high).

Right.

Johan

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Bjorn Andersson]

On Fri, Jan 26, 2024 at 05:23:30PM +0100, Johan Hovold wrote:
> On Fri, Jan 26, 2024 at 06:53:46AM -0800, Bjorn Andersson wrote:
> > On Fri, Jan 26, 2024 at 03:31:02PM +0100, Johan Hovold wrote:
> > > On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> 
> > > > In short it looks like the delays make the difference and, even a short
> > > > delay, can fix the problem.
> > > 
> > > Right, but since the suppliers are left enabled by the bootloader (and
> > > never disabled by the kernel), that only begs the question of why this
> > > makes a difference.
> > 
> > You're right, the supply is kept on by other things, so this isn't the
> > problem.
> > 
> > > Without the delay, the other HID devices are probing (successfully)
> > > slightly before, but essentially in parallel with the touchscreen while
> > > using the same resources. Is that causing trouble somehow?
> > 
> > The difference to those other HID devices is GPIO 99 - the reset pin,
> > which is configured pull down input from boot - i.e. the chip is held in
> > reset.
> > 
> > When the HID device is being probed, pinctrl applies &ts0_default starts
> > driving it high, bringing the device out of reset. But insufficient time
> > is given for the chip to come up so the I2C read fails.
> 
> Ah, that's it.
> 
> You should drop that 'output-high' from the pin config as part of this
> patch to avoid toggling the reset line twice at boot.
> 

Sounds reasonable, let's fix that while we're at it...

> Looks like we have the same problem on the CRD as well. There the
> touchscreen still works, possibly because it has been enabled by the
> boot firmware or simply because that touchscreen can handle a shorter
> delay.
> 

I only poke the CRD remotely, forgot that it had touchscreen. Let's
bake a patch for that as well...

> Where exactly did you find those delay values in the ACPI tables? I
> couldn't seem to find anything in the decompiled DSDT.
> 

The PEP sequence for the touchscreen device.

Regards,
Bjorn

> > If you later try to probe again, 200ms has elapsed since the reset was
> > deasserted (driven high).
> 
> Right.
> 
> Johan

Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Bjorn Andersson]

On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > The failing read-test in __i2c_hid_core_probe() determines that there's
> > > nothing connected at the documented address of the touchscreen.
> > >
> > > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > > ACPI tables. Also wire up the reset-gpio, for good measure.
> >
> > As the supplies for the touchscreen are always on (and left on by the
> > bootloader) it would seem that it is really the addition of the reset
> > gpio which makes things work here. Unless the delay is needed for some
> > other reason.
> >
> > (The power-on delay also looks a bit short compared to what is used for
> > other devices.)
> >
> > Reset support was only recently added with commit 2be404486c05 ("HID:
> > i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> > backport this one before first determining that.
> 
> This comment attracted my attention so I tried booting with each of the
> three lines individually.
> 
> 
> On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > +             reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> 
> This is not enough, on it's own, to get the touch screen running.
> 

No, because pinctrl already brings the chip out of reset without this.

> I guess that's not so much of a surprise since the rebind-the-driver
> from userspace trick wouldn't have been touching this reset.
> 

Right, it would just have been left deasserted from the first attempt.

That said, the addition of the reset means that we're now asserting
reset in such rebind attempts. And as such the
post-reset-deassert-delay-ms is now needed between the explicit deassert
from the driver and the i2c read.

> 
> > +             post-power-on-delay-ms = <5>;
> 
> This line alone is enough (in v6.7.1).
> 

So the delay taken through really_probe() until we reach that i2c read
is almost the entire delay needed, on your specific device.

> 
> > +             post-reset-deassert-delay-ms = <200>;
> 
> This line alone is also enough!
> 
> In short it looks like the delays make the difference and, even a short
> delay, can fix the problem.
> 
> Of course, regardless of the line-by-line results I also ran with all
> the changes so, FWIW:
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
> 

Thanks,
Bjorn

> 
> Daniel.