[PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

[PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

[Johan Hovold]

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

[PATCH 1/6] dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema

[Johan Hovold]

The Ilitek ILI2901 touch screen controller was apparently incorrectly
added to the Elan eKTH6915 schema simply because it also has a reset
gpio and is currently managed by the Elan driver in Linux.

The two controllers are not related even if an unfortunate wording in
the commit message adding the Ilitek compatible made it sound like they
were.

Add a dedicated schema for the ILI2901 which does not specify the I2C
address (which is likely 0x41 rather than 0x10 as for other Ilitek touch
controllers) to avoid cluttering the Elan schema with unrelated devices
and to make it easier to find the correct schema when adding further
Ilitek controllers.

Fixes: d74ac6f60a7e ("dt-bindings: HID: i2c-hid: elan: Introduce Ilitek ili2901")
Cc: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../bindings/input/elan,ekth6915.yaml         |  5 +-
 .../bindings/input/ilitek,ili2901.yaml        | 66 +++++++++++++++++++
 2 files changed, 68 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml

diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index dc4ac41f2441..3e2d216c6432 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -18,9 +18,8 @@ allOf:
 
 properties:
   compatible:
-    enum:
-      - elan,ekth6915
-      - ilitek,ili2901
+    items:
+      - const: elan,ekth6915
 
   reg:
     const: 0x10
diff --git a/Documentation/devicetree/bindings/input/ilitek,ili2901.yaml b/Documentation/devicetree/bindings/input/ilitek,ili2901.yaml
new file mode 100644
index 000000000000..1abeec768d79
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/ilitek,ili2901.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/ilitek,ili2901.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ilitek ILI2901 touchscreen controller
+
+maintainers:
+  - Jiri Kosina <jkosina@suse.com>
+
+description:
+  Supports the Ilitek ILI2901 touchscreen controller.
+  This touchscreen controller uses the i2c-hid protocol with a reset GPIO.
+
+allOf:
+  - $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
+properties:
+  compatible:
+    enum:
+      - ilitek,ili2901
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  panel: true
+
+  reset-gpios:
+    maxItems: 1
+
+  vcc33-supply: true
+
+  vccio-supply: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - vcc33-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen@41 {
+        compatible = "ilitek,ili2901";
+        reg = <0x41>;
+
+        interrupt-parent = <&tlmm>;
+        interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
+
+        reset-gpios = <&tlmm 8 GPIO_ACTIVE_LOW>;
+        vcc33-supply = <&pp3300_ts>;
+      };
+    };
-- 
2.43.2

[PATCH 2/6] dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M

[Johan Hovold]

Add a compatible string for the Elan eKTH5015M touch controller.

Judging from the current binding and commit bd3cba00dcc6 ("HID: i2c-hid:
elan: Add support for Elan eKTH6915 i2c-hid touchscreens"), eKTH5015M
appears to be compatible with eKTH6915. Notably the power-on sequence is
the same.

While at it, drop a redundant label from the example.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../devicetree/bindings/input/elan,ekth6915.yaml    | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index 3e2d216c6432..c3a6f901ff45 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -18,8 +18,13 @@ allOf:
 
 properties:
   compatible:
-    items:
-      - const: elan,ekth6915
+    oneOf:
+      - items:
+          - enum:
+              - elan,ekth5015m
+          - const: elan,ekth6915
+      - items:
+          - const: elan,ekth6915
 
   reg:
     const: 0x10
@@ -57,8 +62,8 @@ examples:
       #address-cells = <1>;
       #size-cells = <0>;
 
-      ap_ts: touchscreen@10 {
-        compatible = "elan,ekth6915";
+      touchscreen@10 {
+        compatible = "elan,ekth5015m", "elan,ekth6915";
         reg = <0x10>;
 
         interrupt-parent = <&tlmm>;
-- 
2.43.2

[PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Johan Hovold]

When the power supply is shared with other peripherals the reset line
can be wired in such a way that it can remain deasserted regardless of
whether the supply is on or not.

This is important as it can be used to avoid holding the controller in
reset for extended periods of time when it remains powered, something
which can lead to increased power consumption. Leaving reset deasserted
also avoids leaking current through the reset circuitry pull-up
resistors.

Add 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.

Note that this property can also be used when the supply cannot be
turned off by the OS at all.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
index c3a6f901ff45..3d20673f10b2 100644
--- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
+++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
@@ -37,6 +37,12 @@ properties:
   reset-gpios:
     description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
 
+  no-reset-on-power-off:
+    type: boolean
+    description:
+      Reset line is wired so that it can be left deasserted when the power
+      supply is off.
+
   vcc33-supply:
     description: The 3.3V supply to the touchscreen.
 
-- 
2.43.2

[PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage

[Johan Hovold]

The Elan eKTH5015M touch controller found on the Lenovo ThinkPad X13s
shares the VCC33 supply with other peripherals that may remain powered
during suspend (e.g. when enabled as wakeup sources).

The reset line is also wired so that it can be left deasserted when the
supply is off.

This is important as it avoids holding the controller in reset for
extended periods of time when it remains powered, which can lead to
increased power consumption, and also avoids leaking current through the
X13s reset circuitry during suspend (and after driver unbind).

Use the new 'no-reset-on-power-off' devicetree property to determine
when reset needs to be asserted on power down.

Notably this also avoids wasting power on machine variants without a
touchscreen for which the driver would otherwise exit probe with reset
asserted.

Fixes: bd3cba00dcc6 ("HID: i2c-hid: elan: Add support for Elan eKTH6915 i2c-hid touchscreens")
Cc: stable@vger.kernel.org	# 6.0
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/hid/i2c-hid/i2c-hid-of-elan.c | 37 ++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of-elan.c b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
index 5b91fb106cfc..8a905027d5e9 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-elan.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-elan.c
@@ -31,6 +31,7 @@ struct i2c_hid_of_elan {
 	struct regulator *vcc33;
 	struct regulator *vccio;
 	struct gpio_desc *reset_gpio;
+	bool no_reset_on_power_off;
 	const struct elan_i2c_hid_chip_data *chip_data;
 };
 
@@ -40,17 +41,17 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
 		container_of(ops, struct i2c_hid_of_elan, ops);
 	int ret;
 
+	gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+
 	if (ihid_elan->vcc33) {
 		ret = regulator_enable(ihid_elan->vcc33);
 		if (ret)
-			return ret;
+			goto err_deassert_reset;
 	}
 
 	ret = regulator_enable(ihid_elan->vccio);
-	if (ret) {
-		regulator_disable(ihid_elan->vcc33);
-		return ret;
-	}
+	if (ret)
+		goto err_disable_vcc33;
 
 	if (ihid_elan->chip_data->post_power_delay_ms)
 		msleep(ihid_elan->chip_data->post_power_delay_ms);
@@ -60,6 +61,15 @@ static int elan_i2c_hid_power_up(struct i2chid_ops *ops)
 		msleep(ihid_elan->chip_data->post_gpio_reset_on_delay_ms);
 
 	return 0;
+
+err_disable_vcc33:
+	if (ihid_elan->vcc33)
+		regulator_disable(ihid_elan->vcc33);
+err_deassert_reset:
+	if (ihid_elan->no_reset_on_power_off)
+		gpiod_set_value_cansleep(ihid_elan->reset_gpio, 0);
+
+	return ret;
 }
 
 static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
@@ -67,7 +77,14 @@ static void elan_i2c_hid_power_down(struct i2chid_ops *ops)
 	struct i2c_hid_of_elan *ihid_elan =
 		container_of(ops, struct i2c_hid_of_elan, ops);
 
-	gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+	/*
+	 * Do not assert reset when the hardware allows for it to remain
+	 * deasserted regardless of the state of the (shared) power supply to
+	 * avoid wasting power when the supply is left on.
+	 */
+	if (!ihid_elan->no_reset_on_power_off)
+		gpiod_set_value_cansleep(ihid_elan->reset_gpio, 1);
+
 	if (ihid_elan->chip_data->post_gpio_reset_off_delay_ms)
 		msleep(ihid_elan->chip_data->post_gpio_reset_off_delay_ms);
 
@@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
 	ihid_elan->ops.power_up = elan_i2c_hid_power_up;
 	ihid_elan->ops.power_down = elan_i2c_hid_power_down;
 
-	/* Start out with reset asserted */
-	ihid_elan->reset_gpio =
-		devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+							GPIOD_ASIS);
 	if (IS_ERR(ihid_elan->reset_gpio))
 		return PTR_ERR(ihid_elan->reset_gpio);
 
+	ihid_elan->no_reset_on_power_off = of_property_read_bool(client->dev.of_node,
+						"no-reset-on-power-off");
+
 	ihid_elan->vccio = devm_regulator_get(&client->dev, "vccio");
 	if (IS_ERR(ihid_elan->vccio))
 		return PTR_ERR(ihid_elan->vccio);
-- 
2.43.2

[PATCH 5/6] arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

[Johan Hovold]

The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay
before sending commands after having deasserted reset during power on.

Switch to 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.

Note that the always-on 1.8 V supply (s10b) is not used by the
controller directly and should not be described.

Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
Cc: stable@vger.kernel.org	# 6.0
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 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 b220ba4fba23..e27c8a21125c 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -674,15 +674,16 @@ &i2c4 {
 
 	status = "okay";
 
-	/* FIXME: verify */
 	touchscreen@10 {
-		compatible = "hid-over-i2c";
+		compatible = "elan,ekth5015m", "elan,ekth6915";
 		reg = <0x10>;
 
-		hid-descr-addr = <0x1>;
 		interrupts-extended = <&tlmm 175 IRQ_TYPE_LEVEL_LOW>;
-		vdd-supply = <&vreg_misc_3p3>;
-		vddl-supply = <&vreg_s10b>;
+		reset-gpios = <&tlmm 99 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;
+		no-reset-on-power-off;
+
+		vcc33-supply = <&vreg_misc_3p3>;
+		vccio-supply = <&vreg_misc_3p3>;
 
 		pinctrl-names = "default";
 		pinctrl-0 = <&ts0_default>;
@@ -1637,8 +1638,8 @@ int-n-pins {
 		reset-n-pins {
 			pins = "gpio99";
 			function = "gpio";
-			output-high;
-			drive-strength = <16>;
+			drive-strength = <2>;
+			bias-disable;
 		};
 	};
 
-- 
2.43.2

[PATCH 6/6] arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset

[Johan Hovold]

The touch controller reset line is currently not described by the
devicetree except in the pin configuration which is used to deassert
reset.

As the reset line has an external pull up to an always-on rail there is
no need to drive the pin high so just leave it configured as an input
and disable the internal pull down.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index 08b3627049bc..68e70c983c94 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -1014,8 +1014,7 @@ int-n-pins {
 		reset-n-pins {
 			pins = "gpio99";
 			function = "gpio";
-			output-high;
-			drive-strength = <16>;
+			bias-disable;
 		};
 	};
 
-- 
2.43.2

Re: [PATCH 1/6] dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema

[Krzysztof Kozlowski]

On 23/04/2024 15:46, Johan Hovold wrote:
> The Ilitek ILI2901 touch screen controller was apparently incorrectly
> added to the Elan eKTH6915 schema simply because it also has a reset
> gpio and is currently managed by the Elan driver in Linux.
> 
> The two controllers are not related even if an unfortunate wording in
> the commit message adding the Ilitek compatible made it sound like they
> were.
> 
> Add a dedicated schema for the ILI2901 which does not specify the I2C
> address (which is likely 0x41 rather than 0x10 as for other Ilitek touch
> controllers) to avoid cluttering the Elan schema with unrelated devices
> and to make it easier to find the correct schema when adding further
> Ilitek controllers.
> 
> Fixes: d74ac6f60a7e ("dt-bindings: HID: i2c-hid: elan: Introduce Ilitek ili2901")
> Cc: Zhengqiao Xia <xiazhengqiao@huaqin.corp-partner.google.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../bindings/input/elan,ekth6915.yaml         |  5 +-
>  .../bindings/input/ilitek,ili2901.yaml        | 66 +++++++++++++++++++
>  2 files changed, 68 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/ilitek,ili2901.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> index dc4ac41f2441..3e2d216c6432 100644
> --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> @@ -18,9 +18,8 @@ allOf:
>  
>  properties:
>    compatible:
> -    enum:
> -      - elan,ekth6915
> -      - ilitek,ili2901
> +    items:

Drop items, that's just const. Or keep it as enum, which makes patch
diff smaller here.

> +      - const: elan,ekth6915

With items dropped:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/6] dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M

[Krzysztof Kozlowski]

On 23/04/2024 15:46, Johan Hovold wrote:
> Add a compatible string for the Elan eKTH5015M touch controller.
> 
> Judging from the current binding and commit bd3cba00dcc6 ("HID: i2c-hid:
> elan: Add support for Elan eKTH6915 i2c-hid touchscreens"), eKTH5015M
> appears to be compatible with eKTH6915. Notably the power-on sequence is
> the same.
> 
> While at it, drop a redundant label from the example.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../devicetree/bindings/input/elan,ekth6915.yaml    | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> index 3e2d216c6432..c3a6f901ff45 100644
> --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> @@ -18,8 +18,13 @@ allOf:
>  
>  properties:
>    compatible:
> -    items:
> -      - const: elan,ekth6915
> +    oneOf:
> +      - items:
> +          - enum:
> +              - elan,ekth5015m
> +          - const: elan,ekth6915
> +      - items:

Don't re-add the items for this entry. Just const.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Krzysztof Kozlowski]

On 23/04/2024 15:46, Johan Hovold wrote:
> When the power supply is shared with other peripherals the reset line
> can be wired in such a way that it can remain deasserted regardless of
> whether the supply is on or not.

To clarify: the reset line is still present and working in such case?

> 
> This is important as it can be used to avoid holding the controller in
> reset for extended periods of time when it remains powered, something
> which can lead to increased power consumption. Leaving reset deasserted
> also avoids leaking current through the reset circuitry pull-up
> resistors.
> 
> Add 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.
> 
> Note that this property can also be used when the supply cannot be
> turned off by the OS at all.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  Documentation/devicetree/bindings/input/elan,ekth6915.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> index c3a6f901ff45..3d20673f10b2 100644
> --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> @@ -37,6 +37,12 @@ properties:
>    reset-gpios:
>      description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
>  
> +  no-reset-on-power-off:

Missing vendor prefix. Unless you want to re-use existing property
"keep-power-in-suspend", but the case here mentions power off, not suspend.

Anyway, the property sounds like what the OS should be doing, which is
not what we want. You basically instruct driver what to do. We want a
described hardware configuration or hardware specifics.

Reset is pulled to something? What is exactly different in this hardware
configuration comparing to other hardware setup (regular)?

> +    type: boolean
> +    description:
> +      Reset line is wired so that it can be left deasserted when the power
> +      supply is off.

Best regards,
Krzysztof

Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

[Steev Klimaszewski]

Hi Johan,

On Tue, Apr 23, 2024 at 8:47 AM Johan Hovold <johan+linaro@kernel.org> 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 <steev@kali.org>

Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> 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

Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage

[Doug Anderson]

Hi,

On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
>         ihid_elan->ops.power_up = elan_i2c_hid_power_up;
>         ihid_elan->ops.power_down = elan_i2c_hid_power_down;
>
> -       /* Start out with reset asserted */
> -       ihid_elan->reset_gpio =
> -               devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> +       ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +                                                       GPIOD_ASIS);

I'm not a huge fan of this part of the change. It feels like the GPIO
state should be initialized by the probe function. Right before we
call i2c_hid_core_probe() we should be in the state of "powered off"
and the reset line should be in a consistent state. If
"no_reset_on_power_off" then it should be de-asserted. Else it should
be asserted.

I think GPIOD_ASIS doesn't actually do anything useful for you, right?
i2c_hid_core_probe() will power on and the first thing that'll happen
there is that the reset line will be unconditionally asserted.

Having this as "GPIOD_ASIS" makes it feel like the kernel is somehow
able to maintain continuity of this GPIO line from the BIOS state to
the kernel, but I don't think it can. I've looked at the "GPIOD_ASIS"
property before because I've always wanted the ability to have GPIOs
that could more seamlessly transition their firmware state to their
kernel state. I don't think the API actually allows it. The fact that
GPIO regulators don't support this seamless transition (even though it
would be an obvious feature to add) supports my theory that the API
doesn't currently allow it. It may be possible to make something work
on some implementations but I think it's not guaranteed.

Specifically, the docs say:

* GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must be set
  later with one of the dedicated functions.

So that means that you can't read the pin without making it an input
(which might change the state if it was previously driving a value)
and you can't write the pin without making it an output and choosing a
value to set it to. Basically grabbing a pin with "asis" doesn't allow
you to do anything with it--it just claims it and doesn't let anyone
else have it.

-Doug

Re: [PATCH 2/6] dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M

[Johan Hovold]

On Tue, Apr 23, 2024 at 06:24:39PM +0200, Krzysztof Kozlowski wrote:
> On 23/04/2024 15:46, Johan Hovold wrote:
 
> >  properties:
> >    compatible:
> > -    items:
> > -      - const: elan,ekth6915
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - elan,ekth5015m
> > +          - const: elan,ekth6915
> > +      - items:
> 
> Don't re-add the items for this entry. Just const.

Sure. But note that the example schema uses 'items' like this (e.g. for
'compatible' and 'clock-names'):

	https://docs.kernel.org/devicetree/bindings/writing-schema.html#annotated-example-schema

Johan

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Johan Hovold]

On Tue, Apr 23, 2024 at 06:29:44PM +0200, Krzysztof Kozlowski wrote:
> On 23/04/2024 15:46, Johan Hovold wrote:
> > When the power supply is shared with other peripherals the reset line
> > can be wired in such a way that it can remain deasserted regardless of
> > whether the supply is on or not.
> 
> To clarify: the reset line is still present and working in such case?

Yes.

> > This is important as it can be used to avoid holding the controller in
> > reset for extended periods of time when it remains powered, something
> > which can lead to increased power consumption. Leaving reset deasserted
> > also avoids leaking current through the reset circuitry pull-up
> > resistors.
> > 
> > Add 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.
> > 
> > Note that this property can also be used when the supply cannot be
> > turned off by the OS at all.

> >    reset-gpios:
> >      description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
> >  
> > +  no-reset-on-power-off:
> 
> Missing vendor prefix. Unless you want to re-use existing property
> "keep-power-in-suspend", but the case here mentions power off, not suspend.

No, I left out the prefix on purpose as I mentioned in the cover letter.
There is nothing vendor specific about this property and I expect it to
be reused for other devices.

And "keep-power-in-suspend" is too specific and indeed looks like
instruction to the OS rather than hw description (more below), but
importantly it is not related to the problem here (which is about
reset, not power).
 
> Anyway, the property sounds like what the OS should be doing, which is
> not what we want. You basically instruct driver what to do. We want a
> described hardware configuration or hardware specifics.

Right, and this was why I at first rejected a property name like this in
favour of 'reset-pulled-to-supply' in my first draft. That name
obviously does not work as the 'supply' suffix is already claimed, but I
also realised that it doesn't really describe the hardware property that
allows the reset line to remain asserted.

The key feature in this hardware design is that the reset line will not
just be pulled to the supply voltage (what other voltage would it be
pulled to), but that it is also pulled to ground when the supply is
disabled.

Rather than trying to encode this in the property name, I settled on the
descriptive 'no-reset-on-power-off' after the seeing the prior art in
'goodix,no-reset-during-suspend' property. The latter is too specific
and encodes policy, but the former could still be considered hardware
description and would also apply to other designs which have the
property that the reset line should be left deasserted.

One such example is when the supply can not be disabled at all (e.g. the
Goodix case), but I can imagine there being more than one way to design
such reset circuits.

> Reset is pulled to something? What is exactly different in this hardware
> configuration comparing to other hardware setup (regular)?

The power supply is shared with other peripherals and the hardware
designers have made provisions so that the reset line can remain
deasserted regardless of the state of the supply in order to save power.
 
> > +    type: boolean
> > +    description:
> > +      Reset line is wired so that it can be left deasserted when the power
> > +      supply is off.

Johan

Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

[Johan Hovold]

On Tue, Apr 23, 2024 at 02:34:20PM -0500, Steev Klimaszewski wrote:
> On Tue, Apr 23, 2024 at 8:47 AM Johan Hovold <johan+linaro@kernel.org> 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 <steev@kali.org>

Thanks for testing, Steev.

Johan

Re: [PATCH 2/6] dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M

[Krzysztof Kozlowski]

On 24/04/2024 09:03, Johan Hovold wrote:
> On Tue, Apr 23, 2024 at 06:24:39PM +0200, Krzysztof Kozlowski wrote:
>> On 23/04/2024 15:46, Johan Hovold wrote:
>  
>>>  properties:
>>>    compatible:
>>> -    items:
>>> -      - const: elan,ekth6915
>>> +    oneOf:
>>> +      - items:
>>> +          - enum:
>>> +              - elan,ekth5015m
>>> +          - const: elan,ekth6915
>>> +      - items:
>>
>> Don't re-add the items for this entry. Just const.
> 
> Sure. But note that the example schema uses 'items' like this (e.g. for
> 'compatible' and 'clock-names'):
> 
> 	https://docs.kernel.org/devicetree/bindings/writing-schema.html#annotated-example-schema
> 

Yes, that's the inconsistency we keep. The point is that clocks usually
have just one list, so one "items:". For compatible there can be many
and it leads to less readable code, e.g.:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml?h=v6.9-rc5#n15


Best regards,
Krzysztof

Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

[Johan Hovold]

On Tue, Apr 23, 2024 at 01:36:18PM -0700, Doug Anderson wrote:
> On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> 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

Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage

[Johan Hovold]

On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote:
> On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:

> > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> >         ihid_elan->ops.power_up = elan_i2c_hid_power_up;
> >         ihid_elan->ops.power_down = elan_i2c_hid_power_down;
> >
> > -       /* Start out with reset asserted */
> > -       ihid_elan->reset_gpio =
> > -               devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> > +       ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > +                                                       GPIOD_ASIS);
> 
> I'm not a huge fan of this part of the change. It feels like the GPIO
> state should be initialized by the probe function. Right before we
> call i2c_hid_core_probe() we should be in the state of "powered off"
> and the reset line should be in a consistent state. If
> "no_reset_on_power_off" then it should be de-asserted. Else it should
> be asserted.

First, the reset gpio will be set before probe() returns, just not
immediately when it is requested.

[ Sure, your panel follower implementation may defer the actual probe of
the touchscreen even further but I think that's a design flaw in the
current implementation. ]

Second, the device is not necessarily in the "powered off" state as the
driver leaves the power supplies in whatever state that the boot
firmware left them in.

Not immediately asserting reset and instead leaving it in the state that
the boot firmware left it in is also no different from what happens when
a probe function bails out before requesting the reset line.

> I think GPIOD_ASIS doesn't actually do anything useful for you, right?
> i2c_hid_core_probe() will power on and the first thing that'll happen
> there is that the reset line will be unconditionally asserted.

It avoids asserting reset before we need to and thus also avoid the need
to deassert it on early probe failures (e.g. if one of the regulator
lookups fails).

We also don't need to worry about timing requirements, which can all be
handled in one place (i.e. in the power up and power down callbacks).
 
> Having this as "GPIOD_ASIS" makes it feel like the kernel is somehow
> able to maintain continuity of this GPIO line from the BIOS state to
> the kernel, but I don't think it can. I've looked at the "GPIOD_ASIS"
> property before because I've always wanted the ability to have GPIOs
> that could more seamlessly transition their firmware state to their
> kernel state. I don't think the API actually allows it. The fact that
> GPIO regulators don't support this seamless transition (even though it
> would be an obvious feature to add) supports my theory that the API
> doesn't currently allow it. It may be possible to make something work
> on some implementations but I think it's not guaranteed.
> 
> Specifically, the docs say:
> 
> * GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must be set
>   later with one of the dedicated functions.
> 
> So that means that you can't read the pin without making it an input
> (which might change the state if it was previously driving a value)
> and you can't write the pin without making it an output and choosing a
> value to set it to. Basically grabbing a pin with "asis" doesn't allow
> you to do anything with it--it just claims it and doesn't let anyone
> else have it.

These properties may prevent it from being used by the regulator
framework, but GPIOD_ASIS works well in the case of a reset gpio where
we simply leave it in whatever state the firmware left it in if probe
fails before we get to powering on the device.

Johan

Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage

[Doug Anderson]

Hi,

On Wed, Apr 24, 2024 at 3:56 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote:
> > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > >         ihid_elan->ops.power_up = elan_i2c_hid_power_up;
> > >         ihid_elan->ops.power_down = elan_i2c_hid_power_down;
> > >
> > > -       /* Start out with reset asserted */
> > > -       ihid_elan->reset_gpio =
> > > -               devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> > > +       ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > > +                                                       GPIOD_ASIS);
> >
> > I'm not a huge fan of this part of the change. It feels like the GPIO
> > state should be initialized by the probe function. Right before we
> > call i2c_hid_core_probe() we should be in the state of "powered off"
> > and the reset line should be in a consistent state. If
> > "no_reset_on_power_off" then it should be de-asserted. Else it should
> > be asserted.
>
> First, the reset gpio will be set before probe() returns, just not
> immediately when it is requested.
>
> [ Sure, your panel follower implementation may defer the actual probe of
> the touchscreen even further but I think that's a design flaw in the
> current implementation. ]
>
> Second, the device is not necessarily in the "powered off" state

Logically, the driver treats it as being in "powered off" state,
though. That's why the i2c-hid core makes the call to power it on. IMO
we should strive to make it more of a consistent state, not less of
one.


> as the
> driver leaves the power supplies in whatever state that the boot
> firmware left them in.

I guess it depends on the regulator. ;-) For GPIO-regulators they
aren't in whatever state the boot firmware left them in. For non-GPIO
regulators we (usually) do preserve the state that the boot firmware
left them in.


> Not immediately asserting reset and instead leaving it in the state that
> the boot firmware left it in is also no different from what happens when
> a probe function bails out before requesting the reset line.
>
> > I think GPIOD_ASIS doesn't actually do anything useful for you, right?
> > i2c_hid_core_probe() will power on and the first thing that'll happen
> > there is that the reset line will be unconditionally asserted.
>
> It avoids asserting reset before we need to and thus also avoid the need
> to deassert it on early probe failures (e.g. if one of the regulator
> lookups fails).

I guess so, though I'm of the opinion that we should be robust against
the state that firmware left things in. The firmware's job is to boot
the kernel and make sure that the system is running in a safe/reliable
way, not to optimize the power consumption of the board. If the
firmware left the line configured as "output low" then you'd let that
stand. If it's important for the line to be left in a certain state,
isn't it better to make that explicit?

Also note: if we really end up keeping GPIOD_ASIS, which I'm still not
convinced is the right move, the docs seem to imply that you need to
explicitly set a direction before using it. Your current patch doesn't
do that.

-Doug

Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on

[Doug Anderson]

Hi,

On Wed, Apr 24, 2024 at 1:50 AM Johan Hovold <johan@kernel.org> 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 <johan+linaro@kernel.org> 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

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Krzysztof Kozlowski]

On 24/04/2024 09:34, Johan Hovold wrote:
> On Tue, Apr 23, 2024 at 06:29:44PM +0200, Krzysztof Kozlowski wrote:
>> On 23/04/2024 15:46, Johan Hovold wrote:
>>> When the power supply is shared with other peripherals the reset line
>>> can be wired in such a way that it can remain deasserted regardless of
>>> whether the supply is on or not.
>>
>> To clarify: the reset line is still present and working in such case?
> 
> Yes.
> 
>>> This is important as it can be used to avoid holding the controller in
>>> reset for extended periods of time when it remains powered, something
>>> which can lead to increased power consumption. Leaving reset deasserted
>>> also avoids leaking current through the reset circuitry pull-up
>>> resistors.
>>>
>>> Add 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.
>>>
>>> Note that this property can also be used when the supply cannot be
>>> turned off by the OS at all.
> 
>>>    reset-gpios:
>>>      description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
>>>  
>>> +  no-reset-on-power-off:
>>
>> Missing vendor prefix. Unless you want to re-use existing property
>> "keep-power-in-suspend", but the case here mentions power off, not suspend.
> 
> No, I left out the prefix on purpose as I mentioned in the cover letter.
> There is nothing vendor specific about this property and I expect it to
> be reused for other devices.
> 
> And "keep-power-in-suspend" is too specific and indeed looks like
> instruction to the OS rather than hw description (more below), but
> importantly it is not related to the problem here (which is about
> reset, not power).
>  
>> Anyway, the property sounds like what the OS should be doing, which is
>> not what we want. You basically instruct driver what to do. We want a
>> described hardware configuration or hardware specifics.
> 
> Right, and this was why I at first rejected a property name like this in
> favour of 'reset-pulled-to-supply' in my first draft. That name
> obviously does not work as the 'supply' suffix is already claimed, but I
> also realised that it doesn't really describe the hardware property that
> allows the reset line to remain asserted.
> 
> The key feature in this hardware design is that the reset line will not
> just be pulled to the supply voltage (what other voltage would it be
> pulled to), but that it is also pulled to ground when the supply is
> disabled.

OK, if the property was specific to the hardware, then I would propose
something more hardware-related, e.g. "reset-supply-tied". However :


> Rather than trying to encode this in the property name, I settled on the
> descriptive 'no-reset-on-power-off' after the seeing the prior art in
> 'goodix,no-reset-during-suspend' property. The latter is too specific
> and encodes policy, but the former could still be considered hardware
> description and would also apply to other designs which have the
> property that the reset line should be left deasserted.
> 
> One such example is when the supply can not be disabled at all (e.g. the
> Goodix case), but I can imagine there being more than one way to design
> such reset circuits.

It seems it is common problem. LEDs have property
"retain-state-shutdown", to indicate that during system shutdown we
should not touch them (like power off). Would some variant be applicable
here? First, do we talk here about power off like system shutdown or
runtime PM, thus suspend?


Best regards,
Krzysztof

Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage

[Johan Hovold]

On Wed, Apr 24, 2024 at 09:24:33AM -0700, Doug Anderson wrote:
> On Wed, Apr 24, 2024 at 3:56 AM Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote:
> > > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:

> > > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > >         ihid_elan->ops.power_up = elan_i2c_hid_power_up;
> > > >         ihid_elan->ops.power_down = elan_i2c_hid_power_down;
> > > >
> > > > -       /* Start out with reset asserted */
> > > > -       ihid_elan->reset_gpio =
> > > > -               devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> > > > +       ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > > > +                                                       GPIOD_ASIS);
> > >
> > > I'm not a huge fan of this part of the change. It feels like the GPIO
> > > state should be initialized by the probe function. Right before we
> > > call i2c_hid_core_probe() we should be in the state of "powered off"
> > > and the reset line should be in a consistent state. If
> > > "no_reset_on_power_off" then it should be de-asserted. Else it should
> > > be asserted.

> > Second, the device is not necessarily in the "powered off" state
> 
> Logically, the driver treats it as being in "powered off" state,
> though. That's why the i2c-hid core makes the call to power it on. IMO
> we should strive to make it more of a consistent state, not less of
> one.

That's not really true. The device is often in an undefined power state
and we try to make sure that the hand over is as smooth as possible to
avoid resetting displays and similar unnecessarily.

The power-on sequence is what brings the device into a defined power
state.

> > as the
> > driver leaves the power supplies in whatever state that the boot
> > firmware left them in.
> 
> I guess it depends on the regulator. ;-) For GPIO-regulators they
> aren't in whatever state the boot firmware left them in. For non-GPIO
> regulators we (usually) do preserve the state that the boot firmware
> left them in.

Even for GPIO regulators we have the "regulator-boot-on" devicetree
property which is supposed to be set if the boot firmware has left a
regulator on so that the regulator initialisation can preserve the
state.

> > Not immediately asserting reset and instead leaving it in the state that
> > the boot firmware left it in is also no different from what happens when
> > a probe function bails out before requesting the reset line.
> >
> > > I think GPIOD_ASIS doesn't actually do anything useful for you, right?
> > > i2c_hid_core_probe() will power on and the first thing that'll happen
> > > there is that the reset line will be unconditionally asserted.
> >
> > It avoids asserting reset before we need to and thus also avoid the need
> > to deassert it on early probe failures (e.g. if one of the regulator
> > lookups fails).
> 
> I guess so, though I'm of the opinion that we should be robust against
> the state that firmware left things in. The firmware's job is to boot
> the kernel and make sure that the system is running in a safe/reliable
> way, not to optimize the power consumption of the board.

Agreed.

> If the
> firmware left the line configured as "output low" then you'd let that
> stand. If it's important for the line to be left in a certain state,
> isn't it better to make that explicit?

As I pointed out above we already do this for any error paths before
requesting the reset line. And I also don't think we need to worry too
much about power consumption in case of errors.

But there is one case I had not considered before, and that is your gpio
regulator example but where the boot-on flag does not match the actual
regulator state.

If the supply is on and reset deasserted, but the regulator-boot-on
flag is not set, then we want to make sure that reset is asserted before
disabling the supply when requesting the regulator.

> Also note: if we really end up keeping GPIOD_ASIS, which I'm still not
> convinced is the right move, the docs seem to imply that you need to
> explicitly set a direction before using it. Your current patch doesn't
> do that.

You're right. It will work in my case because of the gpiolib open-drain
implementation, but not generally.

I'll add back the reset during early probe and add error handling for
deasserting reset on machines like the X13s. On these, the touchscreen
may now be reset a couple of times in case of probe deferrals, but
device links should generally prevent that.

Johan

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Johan Hovold]

Hi Krzysztof,

and sorry about the late reply. Got side-tracked.

On Thu, Apr 25, 2024 at 11:39:24AM +0200, Krzysztof Kozlowski wrote:
> On 24/04/2024 09:34, Johan Hovold wrote:
> > On Tue, Apr 23, 2024 at 06:29:44PM +0200, Krzysztof Kozlowski wrote:
> >> On 23/04/2024 15:46, Johan Hovold wrote:
> >>> When the power supply is shared with other peripherals the reset line
> >>> can be wired in such a way that it can remain deasserted regardless of
> >>> whether the supply is on or not.

> >>> This is important as it can be used to avoid holding the controller in
> >>> reset for extended periods of time when it remains powered, something
> >>> which can lead to increased power consumption. Leaving reset deasserted
> >>> also avoids leaking current through the reset circuitry pull-up
> >>> resistors.
> >>>
> >>> Add 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.
> >>>
> >>> Note that this property can also be used when the supply cannot be
> >>> turned off by the OS at all.
> > 
> >>>    reset-gpios:
> >>>      description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
> >>>  
> >>> +  no-reset-on-power-off:
 
> >> Anyway, the property sounds like what the OS should be doing, which is
> >> not what we want. You basically instruct driver what to do. We want a
> >> described hardware configuration or hardware specifics.
> > 
> > Right, and this was why I at first rejected a property name like this in
> > favour of 'reset-pulled-to-supply' in my first draft. That name
> > obviously does not work as the 'supply' suffix is already claimed, but I
> > also realised that it doesn't really describe the hardware property that
> > allows the reset line to remain asserted.
> > 
> > The key feature in this hardware design is that the reset line will not
> > just be pulled to the supply voltage (what other voltage would it be
> > pulled to), but that it is also pulled to ground when the supply is
> > disabled.
> 
> OK, if the property was specific to the hardware, then I would propose
> something more hardware-related, e.g. "reset-supply-tied". However :
> 
> > Rather than trying to encode this in the property name, I settled on the
> > descriptive 'no-reset-on-power-off' after the seeing the prior art in
> > 'goodix,no-reset-during-suspend' property. The latter is too specific
> > and encodes policy, but the former could still be considered hardware
> > description and would also apply to other designs which have the
> > property that the reset line should be left deasserted.
> > 
> > One such example is when the supply can not be disabled at all (e.g. the
> > Goodix case), but I can imagine there being more than one way to design
> > such reset circuits.
> 
> It seems it is common problem. LEDs have property
> "retain-state-shutdown", to indicate that during system shutdown we
> should not touch them (like power off). Would some variant be applicable
> here? First, do we talk here about power off like system shutdown or
> runtime PM, thus suspend?

A name like 'retain-state-shutdown' would also be too specific as what
I'm describing here is that the reset line should be (or can be) left
deasserted whenever the OS wants to power off the device.

That could be during suspend, but more generally whenever the OS
determines that the device does not need to be powered (e.g. when
closing a character device).

Johan

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Linus Walleij]

Hi Johan,

thanks for your patch!

On Tue, Apr 23, 2024 at 3:46 PM Johan Hovold <johan+linaro@kernel.org> wrote:

> When the power supply is shared with other peripherals the reset line
> can be wired in such a way that it can remain deasserted regardless of
> whether the supply is on or not.
>
> This is important as it can be used to avoid holding the controller in
> reset for extended periods of time when it remains powered, something
> which can lead to increased power consumption. Leaving reset deasserted
> also avoids leaking current through the reset circuitry pull-up
> resistors.

So the reset line in this case is a GPIO as seen from the context above.

To me that means that the line should have the GPIO_OPEN_DRAIN flag
set in the device tree node for reset-gpios. As it has pull-up resistors,
setting the line to high impedance takes the device out of reset, and
thus it is effectively open drain.

> Add 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.

If the above holds true, the driver can then just check for the open drain flag
in the reset-gpios phandle, and if that is set, conclude that it should not
actively drive the line low in the poweroff state.

I'd like Krzysztof's input on this though, as he's been all over the reset
code recently and knows it better than me.

Yours,
Linus Walleij

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Johan Hovold]

On Fri, May 03, 2024 at 09:40:43AM +0200, Linus Walleij wrote:

> On Tue, Apr 23, 2024 at 3:46 PM Johan Hovold <johan+linaro@kernel.org> wrote:
> 
> > When the power supply is shared with other peripherals the reset line
> > can be wired in such a way that it can remain deasserted regardless of
> > whether the supply is on or not.
> >
> > This is important as it can be used to avoid holding the controller in
> > reset for extended periods of time when it remains powered, something
> > which can lead to increased power consumption. Leaving reset deasserted
> > also avoids leaking current through the reset circuitry pull-up
> > resistors.
> 
> So the reset line in this case is a GPIO as seen from the context above.
> 
> To me that means that the line should have the GPIO_OPEN_DRAIN flag
> set in the device tree node for reset-gpios. As it has pull-up resistors,
> setting the line to high impedance takes the device out of reset, and
> thus it is effectively open drain.

If you look at the devicetree patch later in the series this is exactly
what is done.
 
> > Add 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.
> 
> If the above holds true, the driver can then just check for the open drain flag
> in the reset-gpios phandle, and if that is set, conclude that it should not
> actively drive the line low in the poweroff state.

That is an alternative I considered but rejected as just knowing that
the gpio is open-drain is not necessarily sufficient, for example, if
the reset line is pulled to always-on rail while power to the device can
be cut.

Perhaps no one would ever construct hardware like that, but it does not
seem like the hardware property I'm trying to encode necessarily follows
from having an open-drain reset line.

And then the OS should probably not make assumptions like that either,
especially since getting it wrong can potentially lead to damaged
hardware.

Johan

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Krzysztof Kozlowski]

On 02/05/2024 11:56, Johan Hovold wrote:
> Hi Krzysztof,
> 
> and sorry about the late reply. Got side-tracked.
> 
> On Thu, Apr 25, 2024 at 11:39:24AM +0200, Krzysztof Kozlowski wrote:
>> On 24/04/2024 09:34, Johan Hovold wrote:
>>> On Tue, Apr 23, 2024 at 06:29:44PM +0200, Krzysztof Kozlowski wrote:
>>>> On 23/04/2024 15:46, Johan Hovold wrote:
>>>>> When the power supply is shared with other peripherals the reset line
>>>>> can be wired in such a way that it can remain deasserted regardless of
>>>>> whether the supply is on or not.
> 
>>>>> This is important as it can be used to avoid holding the controller in
>>>>> reset for extended periods of time when it remains powered, something
>>>>> which can lead to increased power consumption. Leaving reset deasserted
>>>>> also avoids leaking current through the reset circuitry pull-up
>>>>> resistors.
>>>>>
>>>>> Add 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.
>>>>>
>>>>> Note that this property can also be used when the supply cannot be
>>>>> turned off by the OS at all.
>>>
>>>>>    reset-gpios:
>>>>>      description: Reset GPIO; not all touchscreens using eKTH6915 hook this up.
>>>>>  
>>>>> +  no-reset-on-power-off:
>  
>>>> Anyway, the property sounds like what the OS should be doing, which is
>>>> not what we want. You basically instruct driver what to do. We want a
>>>> described hardware configuration or hardware specifics.
>>>
>>> Right, and this was why I at first rejected a property name like this in
>>> favour of 'reset-pulled-to-supply' in my first draft. That name
>>> obviously does not work as the 'supply' suffix is already claimed, but I
>>> also realised that it doesn't really describe the hardware property that
>>> allows the reset line to remain asserted.
>>>
>>> The key feature in this hardware design is that the reset line will not
>>> just be pulled to the supply voltage (what other voltage would it be
>>> pulled to), but that it is also pulled to ground when the supply is
>>> disabled.
>>
>> OK, if the property was specific to the hardware, then I would propose
>> something more hardware-related, e.g. "reset-supply-tied". However :
>>
>>> Rather than trying to encode this in the property name, I settled on the
>>> descriptive 'no-reset-on-power-off' after the seeing the prior art in
>>> 'goodix,no-reset-during-suspend' property. The latter is too specific
>>> and encodes policy, but the former could still be considered hardware
>>> description and would also apply to other designs which have the
>>> property that the reset line should be left deasserted.
>>>
>>> One such example is when the supply can not be disabled at all (e.g. the
>>> Goodix case), but I can imagine there being more than one way to design
>>> such reset circuits.
>>
>> It seems it is common problem. LEDs have property
>> "retain-state-shutdown", to indicate that during system shutdown we
>> should not touch them (like power off). Would some variant be applicable
>> here? First, do we talk here about power off like system shutdown or
>> runtime PM, thus suspend?
> 
> A name like 'retain-state-shutdown' would also be too specific as what
> I'm describing here is that the reset line should be (or can be) left
> deasserted whenever the OS wants to power off the device.

I don't think it is more specific than yours. It is actually more
generic. First, shutdown=poweroff, so that part is the same.
retain-state means keep things enabled, asserted, deasserted, etc, so
multiple cases. Your wording is specific - only one state is kept during
power off: reset deassert.

Best regards,
Krzysztof

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Johan Hovold]

On Fri, May 03, 2024 at 11:11:16AM +0200, Krzysztof Kozlowski wrote:
> On 02/05/2024 11:56, Johan Hovold wrote:
> > On Thu, Apr 25, 2024 at 11:39:24AM +0200, Krzysztof Kozlowski wrote:

> >> It seems it is common problem. LEDs have property
> >> "retain-state-shutdown", to indicate that during system shutdown we
> >> should not touch them (like power off). Would some variant be applicable
> >> here? First, do we talk here about power off like system shutdown or
> >> runtime PM, thus suspend?
> > 
> > A name like 'retain-state-shutdown' would also be too specific as what
> > I'm describing here is that the reset line should be (or can be) left
> > deasserted whenever the OS wants to power off the device.
> 
> I don't think it is more specific than yours. It is actually more
> generic. First, shutdown=poweroff, so that part is the same.

My point is that 'shutdown' is a specific OS concept (i.e. turning the
whole system off), while powering off a *device* can be done for a
number of reasons including closing a character device, suspend and
system-wide shutdown.

The policy decision of when to power off a device is left up to kernel
(e.g. if a device needs to be kept on as it is currently configured as a
wakeup device).

Johan

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Linus Walleij]

On Fri, May 3, 2024 at 10:47 AM Johan Hovold <johan@kernel.org> wrote:

> > If the above holds true, the driver can then just check for the open drain flag
> > in the reset-gpios phandle, and if that is set, conclude that it should not
> > actively drive the line low in the poweroff state.
>
> That is an alternative I considered but rejected as just knowing that
> the gpio is open-drain is not necessarily sufficient, for example, if
> the reset line is pulled to always-on rail while power to the device can
> be cut.
>
> Perhaps no one would ever construct hardware like that, but it does not
> seem like the hardware property I'm trying to encode necessarily follows
> from having an open-drain reset line.
>
> And then the OS should probably not make assumptions like that either,
> especially since getting it wrong can potentially lead to damaged
> hardware.

OK it's a fair point.

I was worried about over-specification of behaviour, as that always
leads to contradictions.

+  no-reset-on-power-off:
+    type: boolean
+    description:
+      Reset line is wired so that it can be left deasserted when the power
+      supply is off.

To be nitpicky: *should* be left deasserted rather than *can* be left
deasserted, right? If the behaviour is desirable but not strictly
required.

Yours,
Linus Walleij

Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property

[Johan Hovold]

On Mon, May 06, 2024 at 08:29:40AM +0200, Linus Walleij wrote:

> +  no-reset-on-power-off:
> +    type: boolean
> +    description:
> +      Reset line is wired so that it can be left deasserted when the power
> +      supply is off.
> 
> To be nitpicky: *should* be left deasserted rather than *can* be left
> deasserted, right? If the behaviour is desirable but not strictly
> required.

I considered that too, but settled on the above description as it is
pure hardware description and leaving the decision to act on it up to
the OS (e.g. if support is implemented).

On the other hand, the "should" is already implied by the property name
so perhaps there's no reason not to include it also in the description:

+  no-reset-on-power-off:
+    type: boolean
+    description:
+      Reset line is wired so that it can (and should) be left
+      deasserted when the power supply is off.

And "should" (unlike "shall") still leaves room for an OS to ignore it
at the cost of increased power consumption.

Johan