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 <[email protected]>
---
Changes in v4:
- Introduced the patch in the HID driver removing the comment about the
need to update the binding.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Rewrote the commit message, to properly describe the problem being
resolved.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Dropped output-high from &ts0_default, to avoid bouncing the reset
line unnecessarily
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Bjorn Andersson (3):
dt-bindings: HID: i2c-hid: Document reset-related properties
HID: i2c-hid-of: Remove comment about post-reset in DT binding
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 +++++--
drivers/hid/i2c-hid/i2c-hid-of.c | 5 -----
3 files changed, 11 insertions(+), 7 deletions(-)
---
base-commit: 8bf1262c53f50fa91fe15d01e5ef5629db55313c
change-id: 20240125-x13s-touchscreen-48012ff3c24e
Best regards,
--
Bjorn Andersson <[email protected]>
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 <[email protected]>
Reviewed-by: Johan Hovold <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
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
With the "post-reset-deassert-delay-ms" property added to the DeviceTree
binding, the comment is no longer valid, remove it.
Signed-off-by: Bjorn Andersson <[email protected]>
---
drivers/hid/i2c-hid/i2c-hid-of.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index c4e1fa0273c8..bbc2afb86eb6 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -104,11 +104,6 @@ static int i2c_hid_of_probe(struct i2c_client *client)
if (!device_property_read_u32(dev, "post-power-on-delay-ms", &val))
ihid_of->post_power_delay_ms = val;
- /*
- * 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;
--
2.25.1
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
Signed-off-by: Bjorn Andersson <[email protected]>
---
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
On Wed, Jan 31, 2024 at 9:07 PM Bjorn Andersson
<[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> Changes in v4:
> - Introduced the patch in the HID driver removing the comment about the
> need to update the binding.
> - Link to v3: https://lore.kernel.org/r/[email protected]
>
> Changes in v3:
> - Rewrote the commit message, to properly describe the problem being
> resolved.
> - Link to v2: https://lore.kernel.org/r/[email protected]
>
> Changes in v2:
> - Dropped output-high from &ts0_default, to avoid bouncing the reset
> line unnecessarily
> - Link to v1: https://lore.kernel.org/r/[email protected]
>
> ---
> Bjorn Andersson (3):
> dt-bindings: HID: i2c-hid: Document reset-related properties
> HID: i2c-hid-of: Remove comment about post-reset in DT binding
> 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 +++++--
> drivers/hid/i2c-hid/i2c-hid-of.c | 5 -----
> 3 files changed, 11 insertions(+), 7 deletions(-)
> ---
> base-commit: 8bf1262c53f50fa91fe15d01e5ef5629db55313c
> change-id: 20240125-x13s-touchscreen-48012ff3c24e
>
> Best regards,
> --
> Bjorn Andersson <[email protected]>
>
>
Thank you for this work!
Works great on my Thinkpad X13s
Tested-by: Steev Klimaszewski <[email protected]>
On Wed, Jan 31, 2024 at 07:07:27PM -0800, Bjorn Andersson wrote:
> With the "post-reset-deassert-delay-ms" property added to the DeviceTree
> binding, the comment is no longer valid, remove it.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
Reviewed-by: Johan Hovold <[email protected]>
On Wed, Jan 31, 2024 at 07:07:26PM -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.
>
> Reviewed-by: Johan Hovold <[email protected]>
> Acked-by: Rob Herring <[email protected]>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> 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.
I know that Rob reluctantly acked this, but re-reading the commit
message for the commit that added support for the reset gpio to the
driver, and added a comment about this not having been added to the
devicetree binding, it becomes obvious that the latter was done on
purpose and that we probably should not be adding the
'post-reset-deassert-delay-ms' property after all:
For now the new "post-reset-deassert-delay-ms" property is only
used on x86/ACPI (non devicetree) devs. IOW it is not used in
actual devicetree files and the same goes for the reset GPIO.
The devicetree-bindings maintainers have requested properties
like these to not be added to the devicetree-bindings, so the
new property + GPIO are deliberately not added to the existing
devicetree-bindings.
2be404486c05 ("HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of")
So perhaps we should just do this properly and add a new compatible
property for X13s touchscreen which can be used to determine these
delays (e.g. for cases where some default values are insufficient).
Johan
On Thu, Feb 01, 2024 at 11:05:47AM +0100, Johan Hovold wrote:
> On Wed, Jan 31, 2024 at 07:07:26PM -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.
> >
> > Reviewed-by: Johan Hovold <[email protected]>
> > Acked-by: Rob Herring <[email protected]>
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > 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.
>
> I know that Rob reluctantly acked this, but re-reading the commit
> message for the commit that added support for the reset gpio to the
> driver, and added a comment about this not having been added to the
> devicetree binding, it becomes obvious that the latter was done on
> purpose and that we probably should not be adding the
> 'post-reset-deassert-delay-ms' property after all:
>
> For now the new "post-reset-deassert-delay-ms" property is only
> used on x86/ACPI (non devicetree) devs. IOW it is not used in
> actual devicetree files and the same goes for the reset GPIO.
> The devicetree-bindings maintainers have requested properties
> like these to not be added to the devicetree-bindings, so the
> new property + GPIO are deliberately not added to the existing
> devicetree-bindings.
>
> 2be404486c05 ("HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of")
>
> So perhaps we should just do this properly and add a new compatible
> property for X13s touchscreen which can be used to determine these
> delays (e.g. for cases where some default values are insufficient).
>
So we should add a new binding, with a device-specific compatible and
add a reset-gpios only for that (and not the generic hid-over-i2c
binding), and then in the i2c-hid driver encode the two delays?
I can try to rewrite these patches, if you can provide me with a
compatible.
Regards,
Bjorn
On Thu, Feb 01, 2024 at 04:08:05PM -0600, Bjorn Andersson wrote:
> On Thu, Feb 01, 2024 at 11:05:47AM +0100, Johan Hovold wrote:
> > On Wed, Jan 31, 2024 at 07:07:26PM -0800, Bjorn Andersson wrote:
> > > + post-reset-deassert-delay-ms:
> > > + description: Time required by the device after reset has been deasserted,
> > > + before it is ready for communication.
> >
> > I know that Rob reluctantly acked this, but re-reading the commit
> > message for the commit that added support for the reset gpio to the
> > driver, and added a comment about this not having been added to the
> > devicetree binding, it becomes obvious that the latter was done on
> > purpose and that we probably should not be adding the
> > 'post-reset-deassert-delay-ms' property after all:
> >
> > For now the new "post-reset-deassert-delay-ms" property is only
> > used on x86/ACPI (non devicetree) devs. IOW it is not used in
> > actual devicetree files and the same goes for the reset GPIO.
> > The devicetree-bindings maintainers have requested properties
> > like these to not be added to the devicetree-bindings, so the
> > new property + GPIO are deliberately not added to the existing
> > devicetree-bindings.
> >
> > 2be404486c05 ("HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of")
> >
> > So perhaps we should just do this properly and add a new compatible
> > property for X13s touchscreen which can be used to determine these
> > delays (e.g. for cases where some default values are insufficient).
>
> So we should add a new binding, with a device-specific compatible and
> add a reset-gpios only for that (and not the generic hid-over-i2c
> binding), and then in the i2c-hid driver encode the two delays?
Right.
> I can try to rewrite these patches, if you can provide me with a
> compatible.
My X13s doesn't have a touchscreen, but the ACPI tables says "ELAN901C".
You can look at the current binding and work with the HID and DT
maintainers to come up with something appropriate. There is one
device-specific compatible in the DT schema currently:
wacom,w9013
so something like
elan,<product>
where <product> is a name for the device with product id 0x901c (or you
use the HID product id directly somehow).
Johan