* [PATCH v2 3/3] HID: i2c-hid: Align i2c_hid_set_power() retry with HID descriptor read
From: Kenny Levinsen @ 2024-04-23 12:07 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki,
Lukasz Majczak
Cc: Kenny Levinsen
In-Reply-To: <20240423122518.34811-1-kl@kl.wtf>
The retry for HID descriptor and for power commands deals with the same
device quirk, so align the two.
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 252ccb3b71d1..749c0c036adb 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -391,25 +391,21 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
i2c_hid_dbg(ihid, "%s\n", __func__);
/*
- * Some devices require to send a command to wakeup before power on.
- * The call will get a return value (EREMOTEIO) but device will be
- * triggered and activated. After that, it goes like a normal device.
+ * Some STM-based devices need 400µs after a rising clock edge to wake
+ * from deep sleep, which in turn means that our first command will
+ * fail EREMOTEIO. Certain Weida Tech devices also need this wake-up.
+ * Retry the command in this case.
*/
- if (power_state == I2C_HID_PWR_ON) {
+ ret = i2c_hid_set_power_command(ihid, power_state);
+ if (ret == -EREMOTEIO && power_state == I2C_HID_PWR_ON) {
+ usleep_range(400, 500);
ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON);
-
- /* Device was already activated */
- if (!ret)
- goto set_pwr_exit;
}
- ret = i2c_hid_set_power_command(ihid, power_state);
if (ret)
dev_err(&ihid->client->dev,
"failed to change power setting.\n");
-set_pwr_exit:
-
/*
* The HID over I2C specification states that if a DEVICE needs time
* after the PWR_ON request, it should utilise CLOCK stretching.
--
2.44.0
^ permalink raw reply related
* Re: [PATCH 1/2] dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
From: Andre Przywara @ 2024-04-23 12:51 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-input, devicetree, linux-arm-kernel, linux-sunxi,
James McGregor
In-Reply-To: <f2021c5d-25fa-4bdf-8f8c-b0bc271eb54e@linaro.org>
On Tue, 23 Apr 2024 14:18:23 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
Hi,
> On 23/04/2024 12:15, Andre Przywara wrote:
> > On Mon, 22 Apr 2024 17:45:10 +0100
> > Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > Hi,
> >
> >> From: James McGregor <jamcgregor@protonmail.com>
> >>
> >> The Allwinner H616 SoC has an LRADC which is compatible with the
> >> versions in existing SoCs.
> >> Add a compatible string for H616, with the R329 fallback. This is the
> >> same as the D1, so put them into an enum.
> >>
> >> Signed-off-by: James McGregor <jamcgregor@protonmail.com>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >
> > Compared the descriptions in the manual between the R392 and the H616, they
> > look the same:
> >
> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Why do you review your own patches? Does it mean that you contribute
> code which you did not review before?
I just merely sent the code on behalf of James, because he had trouble
with the email setup (Protonmail has no SMTP), but didn't want to delay
the post any longer.
> This is odd process.
I agree, I would have liked it more if James would have sent it himself,
and then my review would look more natural, but with my review I
wanted to explicitly point out the technical correctness. Besides: I found
this ordering issue in the other patch only after sending, so needed to
somehow respond anyway.
Also I wanted to make the process transparent: someone posts a patch (in
this case via a proxy), then it gets reviewed.
> Your Review is implied by sending the patch.
Is that really true? I was under the impression that sending is
independent from review. I mean I doubt that every maintainer sending
patches up the chain (when they add their SoB) implies a *review*? Surely
they do agree on the patch (also typically expressed by an Ack), otherwise
they wouldn't send it, but a "review" is still a different thing.
The Linux history has both Rb + SoB from the same person and just SoB
signatures, so I assume that it's not implied.
> And you have there SoB which indicates you sent it...
Yes, but SoB just means I sign off on the legal aspects: that I got the
patches legally, compliant with the GPL, and that I am fine with and
allowed to release them under GPL conditions.
That does not include any code review aspect, AFAICT.
Happy to hear other opinions, but this is my interpretation.
Cheers,
Andre
^ permalink raw reply
* Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition
From: Kenny Levinsen @ 2024-04-23 13:01 UTC (permalink / raw)
To: Łukasz Majczak
Cc: Johan Hovold, benjamin.tissoires, dianders, dtor, hdegoede, jikos,
johan+linaro, kai.heng.feng, linux-input, linux-kernel, mripard,
rad
In-Reply-To: <CAE5UKNqPA4SnnXyaB7Hwk0kcKMMQ_DUuxogDphnnvSGP8g1nAQ@mail.gmail.com>
On 4/23/24 1:32 PM, Łukasz Majczak wrote:
> Unfortunately, your fix doesn't help in our case as there is no sleep
> before the second call to
> i2c_hid_fetch_hid_descriptor().
Yeah, I checked with a logic analyzer and only see ~50µs delay from the
I2C timeout, and 50 is according to my quick math less than the 400 you
mention is the requirement. That means that the current resume path also
lacked a sleep, as it tried power commands in immediate succession.
I have made a v2 with your sleeps added, and added you as Co-developed-by.
Link: https://lore.kernel.org/all/20240423122518.34811-1-kl@kl.wtf/
> Saying more, this STM exposes two i2c addresses (connected physically
> to the same bus)
> one is the HID interface and the other one gives an access to the base
> firmware and is
> served by cros_ec_i2c driver and actually thanks to this driver,
> touchpad is woken up because
> In the resume path cros_ec_i2c "starts talking" through the i2c bus
> thus generating a wakeup
> interrupt.
Ah, that explains why you did not find an issue with the resume path. In
the patch-series I sent, the boot (hid descriptor fetch) and resume
(power on) wake-up paths are fully aligned so neither have to rely on
such "adjacent drivers" waking up the i2c-hid device.
^ permalink raw reply
* [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property
From: Johan Hovold @ 2024-04-23 13:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel,
Johan Hovold
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
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
^ permalink raw reply related
* [PATCH 1/6] dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema
From: Johan Hovold @ 2024-04-23 13:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel,
Johan Hovold, Zhengqiao Xia
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
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
^ permalink raw reply related
* [PATCH 2/6] dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M
From: Johan Hovold @ 2024-04-23 13:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel,
Johan Hovold
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
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
^ permalink raw reply related
* [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage
From: Johan Hovold @ 2024-04-23 13:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel,
Johan Hovold, stable
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
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
^ permalink raw reply related
* [PATCH 5/6] arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
From: Johan Hovold @ 2024-04-23 13:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel,
Johan Hovold, stable
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
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
^ permalink raw reply related
* [PATCH 6/6] arm64: dts: qcom: sc8280xp-crd: use external pull up for touch reset
From: Johan Hovold @ 2024-04-23 13:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel,
Johan Hovold
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
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
^ permalink raw reply related
* [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
From: Johan Hovold @ 2024-04-23 13:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel,
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
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
From: Rob Herring @ 2024-04-23 14:32 UTC (permalink / raw)
To: Andre Przywara
Cc: devicetree, James McGregor, Dmitry Torokhov, Hans de Goede,
Chen-Yu Tsai, Conor Dooley, linux-input, linux-sunxi,
Krzysztof Kozlowski, linux-arm-kernel, Jernej Skrabec,
Samuel Holland
In-Reply-To: <20240422164511.2488261-2-andre.przywara@arm.com>
On Mon, 22 Apr 2024 17:45:10 +0100, Andre Przywara wrote:
> From: James McGregor <jamcgregor@protonmail.com>
>
> The Allwinner H616 SoC has an LRADC which is compatible with the
> versions in existing SoCs.
> Add a compatible string for H616, with the R329 fallback. This is the
> same as the D1, so put them into an enum.
>
> Signed-off-by: James McGregor <jamcgregor@protonmail.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> .../bindings/input/allwinner,sun4i-a10-lradc-keys.yaml | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
While the Reviewed-by from Andre was a bit unusual, the explanation
makes sense.
Also note that b4 can proxy send patches for people with broken (or
non-existent) SMTP servers.
Rob
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
From: Krzysztof Kozlowski @ 2024-04-23 14:59 UTC (permalink / raw)
To: Andre Przywara
Cc: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-input, devicetree, linux-arm-kernel, linux-sunxi,
James McGregor
In-Reply-To: <20240423135106.02ab4473@donnerap.manchester.arm.com>
On 23/04/2024 14:51, Andre Przywara wrote:
> On Tue, 23 Apr 2024 14:18:23 +0200
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>
> Hi,
>
>> On 23/04/2024 12:15, Andre Przywara wrote:
>>> On Mon, 22 Apr 2024 17:45:10 +0100
>>> Andre Przywara <andre.przywara@arm.com> wrote:
>>>
>>> Hi,
>>>
>>>> From: James McGregor <jamcgregor@protonmail.com>
>>>>
>>>> The Allwinner H616 SoC has an LRADC which is compatible with the
>>>> versions in existing SoCs.
>>>> Add a compatible string for H616, with the R329 fallback. This is the
>>>> same as the D1, so put them into an enum.
>>>>
>>>> Signed-off-by: James McGregor <jamcgregor@protonmail.com>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> Compared the descriptions in the manual between the R392 and the H616, they
>>> look the same:
>>>
>>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>
>> Why do you review your own patches? Does it mean that you contribute
>> code which you did not review before?
>
> I just merely sent the code on behalf of James, because he had trouble
> with the email setup (Protonmail has no SMTP), but didn't want to delay
> the post any longer.
OK, thanks, I suggest using b4 relay in the future.
>
>> This is odd process.
>
> I agree, I would have liked it more if James would have sent it himself,
> and then my review would look more natural, but with my review I
> wanted to explicitly point out the technical correctness. Besides: I found
> this ordering issue in the other patch only after sending, so needed to
> somehow respond anyway.
> Also I wanted to make the process transparent: someone posts a patch (in
> this case via a proxy), then it gets reviewed.
>
>> Your Review is implied by sending the patch.
>
> Is that really true? I was under the impression that sending is
For authorship, both tested and review are implied. You cannot send code
which you do not think is correct, therefore your authorship fulfills
entire Reviewer's statement of oversight. There is nothing new said in
statement of oversight comparing to what authorship says.
Now for testing, I think it is also kind of obvious that whenever we can
test our own code, we test it.
For sending other people patches, we could disagree. I stand that I
would not ever send incorrect patch intentionally. Therefore reviewer's
statement of oversight is entirely redundant as well. I just cannot send
someone's patch without reviewing, thus without adhering to points
expressed by statement of oversight.
> independent from review. I mean I doubt that every maintainer sending
> patches up the chain (when they add their SoB) implies a *review*? Surely
Yes, every. This applies to mass-maintainers, like netdev, Greg, Andrew etc.
Every patch I apply to my subsystems is reviewed by me. I cannot do
else, because that is the requirement of maintainership.
There are however maintainers (see i2c patches or Intel DRM) who accept
patches and do not review them. When they review, they provide
additional Rb tag + Sob. This is weird because it means when they accept
patch, they take it unreviewed! Their SoB does not imply reviewing patch
and this is in contrast to kernel process.
BTW, Stephen Rothwell mentions this to every maintainer on adding their
tree to linux-next ("You will need to ensure that ... reviewed by you
(or another maintainer of your subsystem tree)").
> they do agree on the patch (also typically expressed by an Ack), otherwise
> they wouldn't send it, but a "review" is still a different thing.
IMO, this would mean such maintainers accept code which they do not
understand/review/care. They are just patch juggling monkeys who take
something and push it further without doing actual work.
That's not how maintainership should look like. Maintainer must take
reviewed code and, if other maintainers do not review, then they must
perform it.
> The Linux history has both Rb + SoB from the same person and just SoB
> signatures, so I assume that it's not implied.
It depends on people. As I said, I2C and DRM provide Review tag. For me
this is silly and suggest that all my work, that 1000 patches I took,
was not reviewed.
>
>> And you have there SoB which indicates you sent it...
>
> Yes, but SoB just means I sign off on the legal aspects: that I got the
> patches legally, compliant with the GPL, and that I am fine with and
> allowed to release them under GPL conditions.
> That does not include any code review aspect, AFAICT.
So you want to say, that you are fine in sending intentionally buggy
code, knowingly incorrect, because your SoB and your "git send-email"
does not mean you reviewed it?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading report descriptor
From: Benjamin Tissoires @ 2024-04-23 14:59 UTC (permalink / raw)
To: Linux regressions mailing list
Cc: Jiri Kosina, Douglas Anderson, Hans de Goede, linux-input,
linux-kernel, Kenny Levinsen
In-Reply-To: <14d1b38e-0f11-4852-9c52-92b4bedb0a77@leemhuis.info>
On Mon, Apr 22, 2024 at 7:11 PM Linux regression tracking (Thorsten
Leemhuis) <regressions@leemhuis.info> wrote:
>
> On 31.03.24 20:24, Kenny Levinsen wrote:
> > In af93a167eda9, i2c_hid_parse was changed to continue with reading the
> > report descriptor before waiting for reset to be acknowledged.
> >
> > This has lead to two regressions:
>
> Lo! Jiri, Benjamin, quick question: is there a reason why this fix for a
> 6.8-rc1 regression after more than two and half weeks is not yet
> mainlined? Or is there some good reason why we should be should be extra
> cautious?
No special reasons I guess. Neither Jiri nor I have sent a HID update
for this rc cycle, so it's still there, waiting to be pushed.
I've been quite busy with BPF lately and dropped the ball slightly on
the HID maintainer side, but I'm sure we'll send the PR to Linus this
week or the next.
Cheers,
Benjamin
>
>
> Side note: I noticed this due to the tracking today, but I also saw a
> user that recently ran into the problem the quoted fix is supposed to
> resolve: https://social.lol/@major/112294923280815017
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>
> #regzbot poke
>
> > 1. We fail to handle reset acknowledgement if it happens while reading
> > the report descriptor. The transfer sets I2C_HID_READ_PENDING, which
> > causes the IRQ handler to return without doing anything.
> >
> > This affects both a Wacom touchscreen and a Sensel touchpad.
> >
> > 2. On a Sensel touchpad, reading the report descriptor this quickly
> > after reset results in all zeroes or partial zeroes.
> >
> > The issues were observed on the Lenovo Thinkpad Z16 Gen 2.
> >
> > The change in question was made based on a Microsoft article[0] stating
> > that Windows 8 *may* read the report descriptor in parallel with
> > awaiting reset acknowledgement, intended as a slight reset performance
> > optimization. Perhaps they only do this if reset is not completing
> > quickly enough for their tastes?
> >
> > As the code is not currently ready to read registers in parallel with a
> > pending reset acknowledgement, and as reading quickly breaks the report
> > descriptor on the Sensel touchpad, revert to waiting for reset
> > acknowledgement before proceeding to read the report descriptor.
> >
> > [0]: https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management
> >
> > Fixes: af93a167eda9 ("HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor")
> > Signed-off-by: Kenny Levinsen <kl@kl.wtf>
> > ---
> > drivers/hid/i2c-hid/i2c-hid-core.c | 13 ++++---------
> > 1 file changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > index 2df1ab3c31cc..72d2bccf5621 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -735,9 +735,12 @@ static int i2c_hid_parse(struct hid_device *hid)
> > mutex_lock(&ihid->reset_lock);
> > do {
> > ret = i2c_hid_start_hwreset(ihid);
> > - if (ret)
> > + if (ret == 0)
> > + ret = i2c_hid_finish_hwreset(ihid);
> > + else
> > msleep(1000);
> > } while (tries-- > 0 && ret);
> > + mutex_unlock(&ihid->reset_lock);
> >
> > if (ret)
> > goto abort_reset;
> > @@ -767,16 +770,8 @@ static int i2c_hid_parse(struct hid_device *hid)
> > }
> > }
> >
> > - /*
> > - * Windows directly reads the report-descriptor after sending reset
> > - * and then waits for resets completion afterwards. Some touchpads
> > - * actually wait for the report-descriptor to be read before signalling
> > - * reset completion.
> > - */
> > - ret = i2c_hid_finish_hwreset(ihid);
> > abort_reset:
> > clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> > - mutex_unlock(&ihid->reset_lock);
> > if (ret)
> > goto out;
> >
>
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
From: Krzysztof Kozlowski @ 2024-04-23 15:14 UTC (permalink / raw)
To: Andre Przywara
Cc: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-input, devicetree, linux-arm-kernel, linux-sunxi,
James McGregor
In-Reply-To: <1714205b-39cf-4803-b251-a35f6b9ab3e9@linaro.org>
On 23/04/2024 16:59, Krzysztof Kozlowski wrote:
>>>>
>>>> Compared the descriptions in the manual between the R392 and the H616, they
>>>> look the same:
>>>>
>>>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> Why do you review your own patches? Does it mean that you contribute
>>> code which you did not review before?
>>
>> I just merely sent the code on behalf of James, because he had trouble
>> with the email setup (Protonmail has no SMTP), but didn't want to delay
>> the post any longer.
>
> OK, thanks, I suggest using b4 relay in the future.
>
>>
>>> This is odd process.
>>
>> I agree, I would have liked it more if James would have sent it himself,
>> and then my review would look more natural, but with my review I
>> wanted to explicitly point out the technical correctness. Besides: I found
>> this ordering issue in the other patch only after sending, so needed to
>> somehow respond anyway.
>> Also I wanted to make the process transparent: someone posts a patch (in
>> this case via a proxy), then it gets reviewed.
>>
>>> Your Review is implied by sending the patch.
>>
>> Is that really true? I was under the impression that sending is
>
> For authorship, both tested and review are implied. You cannot send code
> which you do not think is correct, therefore your authorship fulfills
> entire Reviewer's statement of oversight. There is nothing new said in
> statement of oversight comparing to what authorship says.
>
> Now for testing, I think it is also kind of obvious that whenever we can
> test our own code, we test it.
>
> For sending other people patches, we could disagree. I stand that I
> would not ever send incorrect patch intentionally. Therefore reviewer's
> statement of oversight is entirely redundant as well. I just cannot send
> someone's patch without reviewing, thus without adhering to points
> expressed by statement of oversight.
>
>> independent from review. I mean I doubt that every maintainer sending
>> patches up the chain (when they add their SoB) implies a *review*? Surely
>
> Yes, every. This applies to mass-maintainers, like netdev, Greg, Andrew etc.
>
> Every patch I apply to my subsystems is reviewed by me. I cannot do
> else, because that is the requirement of maintainership.
>
> There are however maintainers (see i2c patches or Intel DRM) who accept
> patches and do not review them. When they review, they provide
> additional Rb tag + Sob. This is weird because it means when they accept
> patch, they take it unreviewed! Their SoB does not imply reviewing patch
> and this is in contrast to kernel process.
Above is my quote and I understood that my sentence is misleading. I
wanted to say that such maintainers, when accepting code reviewed by
other maintainer, do not perform their own review. They just blindly
take it. That would be the meaning for me, that in other cases they add
explicit Review-tag. If they add it explicit, then when it is missing it
means "No review". And this is odd.
>
> BTW, Stephen Rothwell mentions this to every maintainer on adding their
> tree to linux-next ("You will need to ensure that ... reviewed by you
> (or another maintainer of your subsystem tree)").
>
>
>> they do agree on the patch (also typically expressed by an Ack), otherwise
>> they wouldn't send it, but a "review" is still a different thing.
>
> IMO, this would mean such maintainers accept code which they do not
> understand/review/care. They are just patch juggling monkeys who take
> something and push it further without doing actual work.
>
> That's not how maintainership should look like. Maintainer must take
> reviewed code and, if other maintainers do not review, then they must
> perform it.
>
>> The Linux history has both Rb + SoB from the same person and just SoB
>> signatures, so I assume that it's not implied.
>
> It depends on people. As I said, I2C and DRM provide Review tag. For me
> this is silly and suggest that all my work, that 1000 patches I took,
> was not reviewed.
Also, quoting maintainer's handbook in Linux kernel:
"Maintainers must review *all* patches touching exclusively their
drivers, no matter how trivial."
This cannot be written simpler. Every patch maintainer accepts, which is
not reviewed by co-maintainers or other maintainers, must be reviewed by
him, thus Review is implied by his SoB and commit.
>
>>
>>> And you have there SoB which indicates you sent it...
>>
>> Yes, but SoB just means I sign off on the legal aspects: that I got the
>> patches legally, compliant with the GPL, and that I am fine with and
>> allowed to release them under GPL conditions.
>> That does not include any code review aspect, AFAICT.
>
> So you want to say, that you are fine in sending intentionally buggy
> code, knowingly incorrect, because your SoB and your "git send-email"
> does not mean you reviewed it?
>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: HID: i2c-hid: add dedicated Ilitek ILI2901 schema
From: Krzysztof Kozlowski @ 2024-04-23 16:23 UTC (permalink / raw)
To: Johan Hovold, Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel,
Zhengqiao Xia
In-Reply-To: <20240423134611.31979-2-johan+linaro@kernel.org>
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
^ permalink raw reply
* Re: [PATCH 2/6] dt-bindings: HID: i2c-hid: elan: add Elan eKTH5015M
From: Krzysztof Kozlowski @ 2024-04-23 16:24 UTC (permalink / raw)
To: Johan Hovold, Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel
In-Reply-To: <20240423134611.31979-3-johan+linaro@kernel.org>
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
^ permalink raw reply
* Re: [PATCH 3/6] dt-bindings: HID: i2c-hid: elan: add 'no-reset-on-power-off' property
From: Krzysztof Kozlowski @ 2024-04-23 16:29 UTC (permalink / raw)
To: Johan Hovold, Jiri Kosina, Benjamin Tissoires
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Linus Walleij, Douglas Anderson,
linux-input, devicetree, linux-arm-msm, linux-kernel
In-Reply-To: <20240423134611.31979-4-johan+linaro@kernel.org>
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
^ permalink raw reply
* Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
From: Steev Klimaszewski @ 2024-04-23 19:34 UTC (permalink / raw)
To: Johan Hovold
Cc: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Linus Walleij, Douglas Anderson, linux-input, devicetree,
linux-arm-msm, linux-kernel
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
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>
^ permalink raw reply
* Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
From: Doug Anderson @ 2024-04-23 20:36 UTC (permalink / raw)
To: Johan Hovold
Cc: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Linus Walleij, linux-input, devicetree, linux-arm-msm,
linux-kernel
In-Reply-To: <20240423134611.31979-1-johan+linaro@kernel.org>
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
^ permalink raw reply
* Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage
From: Doug Anderson @ 2024-04-23 20:37 UTC (permalink / raw)
To: Johan Hovold
Cc: Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Linus Walleij, linux-input, devicetree, linux-arm-msm,
linux-kernel, stable
In-Reply-To: <20240423134611.31979-5-johan+linaro@kernel.org>
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
^ permalink raw reply
* Re: [PATCH v2] HID: hid-steam: Add Deck IMU support
From: Vicki Pfau @ 2024-04-24 2:45 UTC (permalink / raw)
To: Max Maisel, jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20240420123418.4938-1-mmm-1@posteo.net>
On 4/20/24 5:34 AM, Max Maisel wrote:
> The Deck's controller features an accelerometer and gyroscope which
> send their measurement values by default in the main HID input report.
> Expose both sensors to userspace through a separate evdev node as it
> is done by the hid-nintendo and hid-playstation drivers.
>
> Signed-off-by: Max Maisel <mmm-1@posteo.net>
> ---
>
> Changes in v2:
> * Increased gyroscope range to 32768.
> * Removed comment about factory calibration of sensor values.
> * Removed STEAM_QUIRK_DECK check in steam_raw_event function.
> * Silenced the IMU when gamepad mode is disabled.
> * Added improved fuzz values for the input device.
> The new values are based on the average deviation from the average
> sensor values at rest.
> * Rebased onto kernel v6.9-rc4.
> * Improved the test procedure below.
>
> Test procedure:
>
> This patch was tested on a Steam Deck running Arch Linux. With it,
> applications using latest SDL2/3 git libraries will pick up the sensors
> without hidraw access. This was tested against the antimicrox gamepad
> mapper.
>
> Measurement value scaling was tested by logging and comparing sensors
> values between the deck and a dualsense controller.
> For the accelerometer, both controllers were aligned to gravity on all axes
> and the reported values were compared.
> For the gyroscope, both controllers were placed on a makeshift
> rotational plate and the reported absolute angular velocity was compared.
> Furthermore, it was tested that the axes have the same orientation
> between the two controller types.
> Finally, it was tested that the full scale values for both sensor types
> can be reached by doing jerky movements with the deck.
> All observed values matched within a few percent error range.
>
> drivers/hid/hid-steam.c | 155 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 147 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index b08a5ab58528..f166188c21ec 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -66,6 +66,14 @@ static LIST_HEAD(steam_devices);
> #define STEAM_DECK_TRIGGER_RESOLUTION 5461
> /* Joystick runs are about 5 mm and 32768 units */
> #define STEAM_DECK_JOYSTICK_RESOLUTION 6553
> +/* Accelerometer has 16 bit resolution and a range of +/- 2g */
> +#define STEAM_DECK_ACCEL_RES_PER_G 16384
> +#define STEAM_DECK_ACCEL_RANGE 32768
> +#define STEAM_DECK_ACCEL_FUZZ 32
> +/* Gyroscope has 16 bit resolution and a range of +/- 2000 dps */
> +#define STEAM_DECK_GYRO_RES_PER_DPS 16
> +#define STEAM_DECK_GYRO_RANGE 32768
> +#define STEAM_DECK_GYRO_FUZZ 1
>
> #define STEAM_PAD_FUZZ 256
>
> @@ -288,6 +296,7 @@ struct steam_device {
> struct mutex report_mutex;
> unsigned long client_opened;
> struct input_dev __rcu *input;
> + struct input_dev __rcu *sensors;
> unsigned long quirks;
> struct work_struct work_connect;
> bool connected;
> @@ -302,6 +311,7 @@ struct steam_device {
> struct work_struct rumble_work;
> u16 rumble_left;
> u16 rumble_right;
> + unsigned int sensor_timestamp_us;
> };
>
> static int steam_recv_report(struct steam_device *steam,
> @@ -825,6 +835,74 @@ static int steam_input_register(struct steam_device *steam)
> return ret;
> }
>
> +static int steam_sensors_register(struct steam_device *steam)
> +{
> + struct hid_device *hdev = steam->hdev;
> + struct input_dev *sensors;
> + int ret;
> +
> + if (!(steam->quirks & STEAM_QUIRK_DECK))
> + return 0;
> +
> + rcu_read_lock();
> + sensors = rcu_dereference(steam->sensors);
> + rcu_read_unlock();
> + if (sensors) {
> + dbg_hid("%s: already connected\n", __func__);
> + return 0;
> + }
> +
> + sensors = input_allocate_device();
> + if (!sensors)
> + return -ENOMEM;
> +
> + input_set_drvdata(sensors, steam);
> + sensors->dev.parent = &hdev->dev;
> +
> + sensors->name = "Steam Deck Motion Sensors";
> + sensors->phys = hdev->phys;
> + sensors->uniq = steam->serial_no;
> + sensors->id.bustype = hdev->bus;
> + sensors->id.vendor = hdev->vendor;
> + sensors->id.product = hdev->product;
> + sensors->id.version = hdev->version;
> +
> + __set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
> + __set_bit(EV_MSC, sensors->evbit);
> + __set_bit(MSC_TIMESTAMP, sensors->mscbit);
> +
> + input_set_abs_params(sensors, ABS_X, -STEAM_DECK_ACCEL_RANGE,
> + STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
> + input_set_abs_params(sensors, ABS_Y, -STEAM_DECK_ACCEL_RANGE,
> + STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
> + input_set_abs_params(sensors, ABS_Z, -STEAM_DECK_ACCEL_RANGE,
> + STEAM_DECK_ACCEL_RANGE, STEAM_DECK_ACCEL_FUZZ, 0);
> + input_abs_set_res(sensors, ABS_X, STEAM_DECK_ACCEL_RES_PER_G);
> + input_abs_set_res(sensors, ABS_Y, STEAM_DECK_ACCEL_RES_PER_G);
> + input_abs_set_res(sensors, ABS_Z, STEAM_DECK_ACCEL_RES_PER_G);
> +
> + input_set_abs_params(sensors, ABS_RX, -STEAM_DECK_GYRO_RANGE,
> + STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
> + input_set_abs_params(sensors, ABS_RY, -STEAM_DECK_GYRO_RANGE,
> + STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
> + input_set_abs_params(sensors, ABS_RZ, -STEAM_DECK_GYRO_RANGE,
> + STEAM_DECK_GYRO_RANGE, STEAM_DECK_GYRO_FUZZ, 0);
> + input_abs_set_res(sensors, ABS_RX, STEAM_DECK_GYRO_RES_PER_DPS);
> + input_abs_set_res(sensors, ABS_RY, STEAM_DECK_GYRO_RES_PER_DPS);
> + input_abs_set_res(sensors, ABS_RZ, STEAM_DECK_GYRO_RES_PER_DPS);
> +
> + ret = input_register_device(sensors);
> + if (ret)
> + goto sensors_register_fail;
> +
> + rcu_assign_pointer(steam->sensors, sensors);
> + return 0;
> +
> +sensors_register_fail:
> + input_free_device(sensors);
> + return ret;
> +}
> +
> static void steam_input_unregister(struct steam_device *steam)
> {
> struct input_dev *input;
> @@ -838,6 +916,24 @@ static void steam_input_unregister(struct steam_device *steam)
> input_unregister_device(input);
> }
>
> +static void steam_sensors_unregister(struct steam_device *steam)
> +{
> + struct input_dev *sensors;
> +
> + if (!(steam->quirks & STEAM_QUIRK_DECK))
> + return;
> +
> + rcu_read_lock();
> + sensors = rcu_dereference(steam->sensors);
> + rcu_read_unlock();
> +
> + if (!sensors)
> + return;
> + RCU_INIT_POINTER(steam->sensors, NULL);
> + synchronize_rcu();
> + input_unregister_device(sensors);
> +}
> +
> static void steam_battery_unregister(struct steam_device *steam)
> {
> struct power_supply *battery;
> @@ -890,18 +986,28 @@ static int steam_register(struct steam_device *steam)
> spin_lock_irqsave(&steam->lock, flags);
> client_opened = steam->client_opened;
> spin_unlock_irqrestore(&steam->lock, flags);
> +
> if (!client_opened) {
> steam_set_lizard_mode(steam, lizard_mode);
> ret = steam_input_register(steam);
> - } else
> - ret = 0;
> + if (ret != 0)
> + goto steam_register_input_fail;
> + ret = steam_sensors_register(steam);
> + if (ret != 0)
> + goto steam_register_sensors_fail;
> + }
> + return 0;
>
> +steam_register_sensors_fail:
> + steam_input_unregister(steam);
> +steam_register_input_fail:
> return ret;
> }
>
> static void steam_unregister(struct steam_device *steam)
> {
> steam_battery_unregister(steam);
> + steam_sensors_unregister(steam);
> steam_input_unregister(steam);
> if (steam->serial_no[0]) {
> hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> @@ -1010,6 +1116,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
> steam->client_opened++;
> spin_unlock_irqrestore(&steam->lock, flags);
>
> + steam_sensors_unregister(steam);
> steam_input_unregister(steam);
>
> return 0;
> @@ -1030,6 +1137,7 @@ static void steam_client_ll_close(struct hid_device *hdev)
> if (connected) {
> steam_set_lizard_mode(steam, lizard_mode);
> steam_input_register(steam);
> + steam_sensors_register(steam);
> }
> }
>
> @@ -1121,6 +1229,7 @@ static int steam_probe(struct hid_device *hdev,
> INIT_DELAYED_WORK(&steam->mode_switch, steam_mode_switch_cb);
> INIT_LIST_HEAD(&steam->list);
> INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
> + steam->sensor_timestamp_us = 0;
>
> /*
> * With the real steam controller interface, do not connect hidraw.
> @@ -1380,12 +1489,12 @@ static void steam_do_input_event(struct steam_device *steam,
> * 18-19 | s16 | ABS_HAT0Y | left-pad Y value
> * 20-21 | s16 | ABS_HAT1X | right-pad X value
> * 22-23 | s16 | ABS_HAT1Y | right-pad Y value
> - * 24-25 | s16 | -- | accelerometer X value
> - * 26-27 | s16 | -- | accelerometer Y value
> - * 28-29 | s16 | -- | accelerometer Z value
> - * 30-31 | s16 | -- | gyro X value
> - * 32-33 | s16 | -- | gyro Y value
> - * 34-35 | s16 | -- | gyro Z value
> + * 24-25 | s16 | IMU ABS_X | accelerometer X value
> + * 26-27 | s16 | IMU ABS_Z | accelerometer Y value
> + * 28-29 | s16 | IMU ABS_Y | accelerometer Z value
> + * 30-31 | s16 | IMU ABS_RX | gyro X value
> + * 32-33 | s16 | IMU ABS_RZ | gyro Y value
> + * 34-35 | s16 | IMU ABS_RY | gyro Z value
> * 36-37 | s16 | -- | quaternion W value
> * 38-39 | s16 | -- | quaternion X value
> * 40-41 | s16 | -- | quaternion Y value
> @@ -1546,6 +1655,32 @@ static void steam_do_deck_input_event(struct steam_device *steam,
> input_sync(input);
> }
>
> +static void steam_do_deck_sensors_event(struct steam_device *steam,
> + struct input_dev *sensors, u8 *data)
> +{
> + /*
> + * The deck input report is received every 4 ms on average,
> + * with a jitter of +/- 4 ms even though the USB descriptor claims
> + * that it uses 1 kHz.
> + * Since the HID report does not include a sensor timestamp,
> + * use a fixed increment here.
> + */
> + steam->sensor_timestamp_us += 4000;
> +
> + if (!steam->gamepad_mode)
> + return;
> +
> + input_event(sensors, EV_MSC, MSC_TIMESTAMP, steam->sensor_timestamp_us);
> + input_report_abs(sensors, ABS_X, steam_le16(data + 24));
> + input_report_abs(sensors, ABS_Z, -steam_le16(data + 26));
> + input_report_abs(sensors, ABS_Y, steam_le16(data + 28));
> + input_report_abs(sensors, ABS_RX, steam_le16(data + 30));
> + input_report_abs(sensors, ABS_RZ, -steam_le16(data + 32));
> + input_report_abs(sensors, ABS_RY, steam_le16(data + 34));
> +
> + input_sync(sensors);
> +}
> +
> /*
> * The size for this message payload is 11.
> * The known values are:
> @@ -1583,6 +1718,7 @@ static int steam_raw_event(struct hid_device *hdev,
> {
> struct steam_device *steam = hid_get_drvdata(hdev);
> struct input_dev *input;
> + struct input_dev *sensors;
> struct power_supply *battery;
>
> if (!steam)
> @@ -1628,6 +1764,9 @@ static int steam_raw_event(struct hid_device *hdev,
> input = rcu_dereference(steam->input);
> if (likely(input))
> steam_do_deck_input_event(steam, input, data);
> + sensors = rcu_dereference(steam->sensors);
> + if (likely(sensors))
> + steam_do_deck_sensors_event(steam, sensors, data);
> rcu_read_unlock();
> break;
> case ID_CONTROLLER_WIRELESS:
>
> base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680
Looks good.
Reviewed-by: Vicki Pfau <vi@endrift.com>
^ permalink raw reply
* Re: [PATCH] Input: xpad - add support for ASUS ROG RAIKIRI
From: Vicki Pfau @ 2024-04-24 2:48 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
In-Reply-To: <20240404035345.159643-1-vi@endrift.com>
Hello Dmitry,
On 4/3/24 8:53 PM, Vicki Pfau wrote:
> Add the VID/PID for ASUS ROG RAIKIRI to xpad_device and the VID to xpad_table
>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
> ---
> drivers/input/joystick/xpad.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index bf3a7c1824f8..a7ce6258a2fa 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -202,6 +202,7 @@ static const struct xpad_device {
> { 0x0738, 0xcb29, "Saitek Aviator Stick AV8R02", 0, XTYPE_XBOX360 },
> { 0x0738, 0xf738, "Super SFIV FightStick TE S", 0, XTYPE_XBOX360 },
> { 0x07ff, 0xffff, "Mad Catz GamePad", 0, XTYPE_XBOX360 },
> + { 0x0b05, 0x1a38, "ASUS ROG RAIKIRI", 0, XTYPE_XBOXONE },
> { 0x0c12, 0x0005, "Intec wireless", 0, XTYPE_XBOX },
> { 0x0c12, 0x8801, "Nyko Xbox Controller", 0, XTYPE_XBOX },
> { 0x0c12, 0x8802, "Zeroplus Xbox Controller", 0, XTYPE_XBOX },
> @@ -474,6 +475,7 @@ static const struct usb_device_id xpad_table[] = {
> { USB_DEVICE(0x0738, 0x4540) }, /* Mad Catz Beat Pad */
> XPAD_XBOXONE_VENDOR(0x0738), /* Mad Catz FightStick TE 2 */
> XPAD_XBOX360_VENDOR(0x07ff), /* Mad Catz Gamepad */
> + XPAD_XBOXONE_VENDOR(0x0b05), /* ASUS controllers */
> XPAD_XBOX360_VENDOR(0x0c12), /* Zeroplus X-Box 360 controllers */
> XPAD_XBOX360_VENDOR(0x0e6f), /* 0x0e6f Xbox 360 controllers */
> XPAD_XBOXONE_VENDOR(0x0e6f), /* 0x0e6f Xbox One controllers */
Can you please review this patch?
^ permalink raw reply
* Re: [PATCH 01/11] can: Add LIN bus as CAN abstraction
From: Jiri Slaby @ 2024-04-24 6:15 UTC (permalink / raw)
To: christoph.fritz
Cc: Oliver Hartkopp, Marc Kleine-Budde, Vincent Mailhol,
David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jiri Kosina,
Benjamin Tissoires, Greg Kroah-Hartman, Andreas Lauser,
Jonathan Corbet, linux-can, netdev, devicetree, linux-input,
linux-serial
In-Reply-To: <43a8b0484e5b4e7d550a665aa4f7b37186d030f7.camel@hexdev.de>
On 23. 04. 24, 11:33, Christoph Fritz wrote:
>>> --- /dev/null
>>> +++ b/include/net/lin.h
>>> @@ -0,0 +1,97 @@
...
>>> +/* special ID descriptions for LIN */
>>> +#define LIN_ENHANCED_CKSUM_FLAG 0x00000100U
>>> +
>>> +static const unsigned char lin_id_parity_tbl[] = {
>>
>> This ends up in every translation unit you include lin.h into. Bad.
>
> This is also being used by a serial lin driver. But I guess most of the drivers have no need for this. Mhm, ... any ideas?
If needs be, put it to a .c and keep an extern here.
thanks,
--
js
suse labs
^ permalink raw reply
* [PATCH v2 00/19] backlight: Constify lcd_ops
From: Krzysztof Kozlowski @ 2024-04-24 6:33 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
Bruno Prémont, Jiri Kosina, Benjamin Tissoires,
Alexander Shiyan, Sascha Hauer, Pengutronix Kernel Team,
Shawn Guo, Fabio Estevam
Cc: dri-devel, linux-fbdev, linux-kernel, linux-input,
linux-arm-kernel, imx, linux-omap, Krzysztof Kozlowski,
Thomas Zimmermann, Thomas Weißschuh
Hi,
Changes in v2:
- Collect tags, including wrongly places Thomas' tag (which requires me
to manually edit 15 other patches to drop it).
- Combine here checkpatch patch:
https://lore.kernel.org/all/20240414185440.288812-1-krzk@kernel.org/
- Link to v1: https://lore.kernel.org/r/20240414-video-backlight-lcd-ops-v1-0-9b37fcbf546a@kernel.org
Dependencies
============
All further patches depend on the first patch. Therefore everything
could go via backlight tree (please ack) or via cross-tree pulls. Or
whatever maintainer choose, just coordinate this with backlight.
Best regards,
Krzysztof
---
Krzysztof Kozlowski (19):
backlight: Constify lcd_ops
backlight: ams369fg06: Constify lcd_ops
backlight: corgi_lcd: Constify lcd_ops
backlight: hx8357: Constify lcd_ops
backlight: ili922x: Constify lcd_ops
backlight: ili9320: Constify lcd_ops
backlight: jornada720_lcd: Constify lcd_ops
backlight: l4f00242t03: Constify lcd_ops
backlight: lms283gf05: Constify lcd_ops
backlight: lms501kf03: Constify lcd_ops
backlight: ltv350qv: Constify lcd_ops
backlight: otm3225a: Constify lcd_ops
backlight: platform_lcd: Constify lcd_ops
backlight: tdo24m: Constify lcd_ops
HID: picoLCD: Constify lcd_ops
fbdev: clps711x: Constify lcd_ops
fbdev: imx: Constify lcd_ops
fbdev: omap: lcd_ams_delta: Constify lcd_ops
const_structs.checkpatch: add lcd_ops
drivers/hid/hid-picolcd_lcd.c | 2 +-
drivers/video/backlight/ams369fg06.c | 2 +-
drivers/video/backlight/corgi_lcd.c | 2 +-
drivers/video/backlight/hx8357.c | 2 +-
drivers/video/backlight/ili922x.c | 2 +-
drivers/video/backlight/ili9320.c | 2 +-
drivers/video/backlight/jornada720_lcd.c | 2 +-
drivers/video/backlight/l4f00242t03.c | 2 +-
drivers/video/backlight/lcd.c | 4 ++--
drivers/video/backlight/lms283gf05.c | 2 +-
drivers/video/backlight/lms501kf03.c | 2 +-
drivers/video/backlight/ltv350qv.c | 2 +-
drivers/video/backlight/otm3225a.c | 2 +-
drivers/video/backlight/platform_lcd.c | 2 +-
drivers/video/backlight/tdo24m.c | 2 +-
drivers/video/fbdev/clps711x-fb.c | 2 +-
drivers/video/fbdev/imxfb.c | 2 +-
drivers/video/fbdev/omap/lcd_ams_delta.c | 2 +-
include/linux/lcd.h | 6 +++---
scripts/const_structs.checkpatch | 1 +
20 files changed, 23 insertions(+), 22 deletions(-)
---
base-commit: a59668a9397e7245b26e9be85d23f242ff757ae8
change-id: 20240414-video-backlight-lcd-ops-276d8439ffb8
Best regards,
--
Krzysztof Kozlowski <krzk@kernel.org>
^ permalink raw reply
* [PATCH v2 01/19] backlight: Constify lcd_ops
From: Krzysztof Kozlowski @ 2024-04-24 6:33 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Helge Deller,
Bruno Prémont, Jiri Kosina, Benjamin Tissoires,
Alexander Shiyan, Sascha Hauer, Pengutronix Kernel Team,
Shawn Guo, Fabio Estevam
Cc: dri-devel, linux-fbdev, linux-kernel, linux-input,
linux-arm-kernel, imx, linux-omap, Krzysztof Kozlowski
In-Reply-To: <20240424-video-backlight-lcd-ops-v2-0-1aaa82b07bc6@kernel.org>
'struct lcd_ops' passed in lcd_device_register() is not modified by core
backlight code, so it can be made const for code safety. This allows
drivers to also define the structure as const.
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
drivers/video/backlight/lcd.c | 4 ++--
include/linux/lcd.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
index ba4771cbd781..ceec90ca758b 100644
--- a/drivers/video/backlight/lcd.c
+++ b/drivers/video/backlight/lcd.c
@@ -191,7 +191,7 @@ static const struct class lcd_class = {
* or a pointer to the newly allocated device.
*/
struct lcd_device *lcd_device_register(const char *name, struct device *parent,
- void *devdata, struct lcd_ops *ops)
+ void *devdata, const struct lcd_ops *ops)
{
struct lcd_device *new_ld;
int rc;
@@ -279,7 +279,7 @@ static int devm_lcd_device_match(struct device *dev, void *res, void *data)
*/
struct lcd_device *devm_lcd_device_register(struct device *dev,
const char *name, struct device *parent,
- void *devdata, struct lcd_ops *ops)
+ void *devdata, const struct lcd_ops *ops)
{
struct lcd_device **ptr, *lcd;
diff --git a/include/linux/lcd.h b/include/linux/lcd.h
index 238fb1dfed98..68703a51dc53 100644
--- a/include/linux/lcd.h
+++ b/include/linux/lcd.h
@@ -61,7 +61,7 @@ struct lcd_device {
points to something in the body of that driver, it is also invalid. */
struct mutex ops_lock;
/* If this is NULL, the backing module is unloaded */
- struct lcd_ops *ops;
+ const struct lcd_ops *ops;
/* Serialise access to set_power method */
struct mutex update_lock;
/* The framebuffer notifier block */
@@ -102,10 +102,10 @@ static inline void lcd_set_power(struct lcd_device *ld, int power)
}
extern struct lcd_device *lcd_device_register(const char *name,
- struct device *parent, void *devdata, struct lcd_ops *ops);
+ struct device *parent, void *devdata, const struct lcd_ops *ops);
extern struct lcd_device *devm_lcd_device_register(struct device *dev,
const char *name, struct device *parent,
- void *devdata, struct lcd_ops *ops);
+ void *devdata, const struct lcd_ops *ops);
extern void lcd_device_unregister(struct lcd_device *ld);
extern void devm_lcd_device_unregister(struct device *dev,
struct lcd_device *ld);
--
2.43.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox