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

[PATCH v3 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>
---
Changes in v3:
- Rewrote the commit message, to properly describe the problem being
  resolved.
- Link to v2: https://lore.kernel.org/r/20240126-x13s-touchscreen-v2-0-5374ccc9e10d@quicinc.com

Changes in v2:
- Dropped output-high from &ts0_default, to avoid bouncing the reset
  line unnecessarily
- Link to v1: https://lore.kernel.org/r/20240125-x13s-touchscreen-v1-0-ab8c882def9c@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 | 7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)
---
base-commit: 8bf1262c53f50fa91fe15d01e5ef5629db55313c
change-id: 20240125-x13s-touchscreen-48012ff3c24e

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

[PATCH v3 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.

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
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

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

[Johan Hovold]

On Mon, Jan 29, 2024 at 08:47:47AM -0800, Bjorn Andersson wrote:
> 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.

> +  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

Hmm, for the third time, it seems you ignored my comment that you need
to remove the comment about these properties from the driver as part of
this series.

	/*
	 * 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.
	 */
	if (!device_property_read_u32(dev, "post-reset-deassert-delay-ms", &val))
		ihid_of->post_reset_delay_ms = val;

> +
>    touchscreen-inverted-x: true
>  
>    touchscreen-inverted-y: true

Johan

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

[Rob Herring]

On Mon, Jan 29, 2024 at 05:56:47PM +0100, Johan Hovold wrote:
> On Mon, Jan 29, 2024 at 08:47:47AM -0800, Bjorn Andersson wrote:
> > 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.
> 
> > +  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
> 
> Hmm, for the third time, it seems you ignored my comment that you need
> to remove the comment about these properties from the driver as part of
> this series.
> 
> 	/*
> 	 * 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.
> 	 */
> 	if (!device_property_read_u32(dev, "post-reset-deassert-delay-ms", &val))
> 		ihid_of->post_reset_delay_ms = val;

DT devices should have a specific compatible that gives enough detail to 
handle this delay or *any* other power sequencing requirement.

OTOH, we've already got one other delay property, what's one more. Sigh.

Acked-by: Rob Herring <robh@kernel.org>

Rob

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

[Johan Hovold]

On Wed, Jan 31, 2024 at 02:22:39PM -0600, Rob Herring wrote:
> On Mon, Jan 29, 2024 at 05:56:47PM +0100, Johan Hovold wrote:
> > On Mon, Jan 29, 2024 at 08:47:47AM -0800, Bjorn Andersson wrote:
> > > 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.
> > 
> > > +  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
> > 
> > Hmm, for the third time, it seems you ignored my comment that you need
> > to remove the comment about these properties from the driver as part of
> > this series.
> > 
> > 	/*
> > 	 * 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.
> > 	 */
> > 	if (!device_property_read_u32(dev, "post-reset-deassert-delay-ms", &val))
> > 		ihid_of->post_reset_delay_ms = val;
> 
> DT devices should have a specific compatible that gives enough detail to 
> handle this delay or *any* other power sequencing requirement.
> 
> OTOH, we've already got one other delay property, what's one more. Sigh.
> 
> Acked-by: Rob Herring <robh@kernel.org>

Right, and I guess we could do this now before this new property gets
more use. Who knows if those delays are actually correct or may need to
be tweaked down the line.

Apparently we only have one specific i2c-hid compatible in the kernel
(and it's not yet used by the driver).

Johan

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

[Bjorn Andersson]

The touchscreen present on some SKUs of Lenovo Thinkpad X13s is never
detected by Linux. Power is applied and the device is brought out of
reset using the pinconfig in DeviceTree, but the read-test in
__i2c_hid_core_probe() fails to access the device, which result in probe
being aborted.

Some users have reported success after rebinding the device.

Looking to the ACPI tables, there's a 5ms after-power and a 200ms
after-reset delay. The power-supply is shared with other components, so
this is active all the way through boot. The reset GPIO, on the other
hand, is low (reset asserted) at boot, so this is first deasserted by
the implicit application of the pinconf state.

This means the time between reset deassert and __i2c_hid_core_probe() is
significantly below the value documented in the ACPI tables.

As the I2C HID binding and driver support specifying a reset gpio,
replace the pinconf-based scheme to pull the device out of reset. Then
specify the after-reset time.

The shared power rail is currently always on, but in case this ever
change, the after-power delay is added as well, to not violate the
power-on to reset-deassert timing requirement.

Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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..33731b95ad51 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>;
 	};
@@ -1450,7 +1454,6 @@ int-n-pins {
 		reset-n-pins {
 			pins = "gpio99";
 			function = "gpio";
-			output-high;
 			drive-strength = <16>;
 		};
 	};

-- 
2.25.1

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

[Daniel Thompson]

On Mon, Jan 29, 2024 at 08:47:48AM -0800, Bjorn Andersson wrote:
> The touchscreen present on some SKUs of Lenovo Thinkpad X13s is never
> detected by Linux. Power is applied and the device is brought out of
> reset using the pinconfig in DeviceTree, but the read-test in
> __i2c_hid_core_probe() fails to access the device, which result in probe
> being aborted.
>
> Some users have reported success after rebinding the device.
>
> Looking to the ACPI tables, there's a 5ms after-power and a 200ms
> after-reset delay. The power-supply is shared with other components, so
> this is active all the way through boot. The reset GPIO, on the other
> hand, is low (reset asserted) at boot, so this is first deasserted by
> the implicit application of the pinconf state.
>
> This means the time between reset deassert and __i2c_hid_core_probe() is
> significantly below the value documented in the ACPI tables.
>
> As the I2C HID binding and driver support specifying a reset gpio,
> replace the pinconf-based scheme to pull the device out of reset. Then
> specify the after-reset time.
>
> The shared power rail is currently always on, but in case this ever
> change, the after-power delay is added as well, to not violate the
> power-on to reset-deassert timing requirement.
>
> Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>

For the sake of completeness (and to make sure I don't forget if anyone
asks me in the future) I also (successfully) tested with the output
high removed from the touchscreen pinctrl.


Daniel.


PS Strictly speaking I tested v2 and am replying to v3... but the DT
   should be the same for both versions ;-).

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

[Johan Hovold]

On Mon, Jan 29, 2024 at 08:47:48AM -0800, Bjorn Andersson wrote:
> The touchscreen present on some SKUs of Lenovo Thinkpad X13s is never
> detected by Linux. Power is applied and the device is brought out of
> reset using the pinconfig in DeviceTree, but the read-test in
> __i2c_hid_core_probe() fails to access the device, which result in probe
> being aborted.
> 
> Some users have reported success after rebinding the device.
> 
> Looking to the ACPI tables, there's a 5ms after-power and a 200ms
> after-reset delay. The power-supply is shared with other components, so
> this is active all the way through boot. The reset GPIO, on the other
> hand, is low (reset asserted) at boot, so this is first deasserted by
> the implicit application of the pinconf state.
> 
> This means the time between reset deassert and __i2c_hid_core_probe() is
> significantly below the value documented in the ACPI tables.
> 
> As the I2C HID binding and driver support specifying a reset gpio,
> replace the pinconf-based scheme to pull the device out of reset. Then
> specify the after-reset time.
> 
> The shared power rail is currently always on, but in case this ever
> change, the after-power delay is added as well, to not violate the
> power-on to reset-deassert timing requirement.
> 
> Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

Thanks for the update. 

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

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

[Rob Herring]

On Mon, Jan 29, 2024 at 08:47:48AM -0800, Bjorn Andersson wrote:
> The touchscreen present on some SKUs of Lenovo Thinkpad X13s is never
> detected by Linux. Power is applied and the device is brought out of
> reset using the pinconfig in DeviceTree, but the read-test in
> __i2c_hid_core_probe() fails to access the device, which result in probe
> being aborted.
> 
> Some users have reported success after rebinding the device.
> 
> Looking to the ACPI tables, there's a 5ms after-power and a 200ms
> after-reset delay. The power-supply is shared with other components, so
> this is active all the way through boot. The reset GPIO, on the other
> hand, is low (reset asserted) at boot, so this is first deasserted by
> the implicit application of the pinconf state.
> 
> This means the time between reset deassert and __i2c_hid_core_probe() is
> significantly below the value documented in the ACPI tables.
> 
> As the I2C HID binding and driver support specifying a reset gpio,
> replace the pinconf-based scheme to pull the device out of reset. Then
> specify the after-reset time.
> 
> The shared power rail is currently always on, but in case this ever
> change, the after-power delay is added as well, to not violate the
> power-on to reset-deassert timing requirement.
> 
> Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> 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..33731b95ad51 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";

Please add a real device compatible before you need the next power 
sequencing property I'll reject.

>  		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>;
>  	};
> @@ -1450,7 +1454,6 @@ int-n-pins {
>  		reset-n-pins {
>  			pins = "gpio99";
>  			function = "gpio";
> -			output-high;
>  			drive-strength = <16>;
>  		};
>  	};
> 
> -- 
> 2.25.1
>