* [PATCH v3 3/3] HID: i2c-hid: Align i2c_hid_set_power() retry with HID descriptor read
From: Kenny Levinsen @ 2024-04-26 22:47 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: <20240426225739.2166-1-kl@kl.wtf>
The retry for HID descriptor and for power commands deals with the same
device quirk, so align the two.
Tested-by: Lukasz Majczak <lma@chromium.org>
Reviewed-by: Lukasz Majczak <lma@chromium.org>
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 6ac1b11fb675..4ec12c083714 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -385,25 +385,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 on a bus error. 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 && 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
* [PATCH v3 2/3] HID: i2c-hid: Retry HID descriptor read to wake up STM devices
From: Kenny Levinsen @ 2024-04-26 22:47 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: <20240426225739.2166-1-kl@kl.wtf>
Some STM microcontrollers need 400µs after rising clock edge in order to
come out of their deep sleep state. This in turn means that the first
command sent to them will fail on a bus error.
Retry once on bus error to see if the device came alive, otherwise treat
the error as if no device was present like before.
Link: https://lore.kernel.org/all/20240405102436.3479210-1-lma@chromium.org/#t
Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
Co-developed-by: Lukasz Majczak <lma@chromium.org>
Tested-by: Lukasz Majczak <lma@chromium.org>
Reviewed-by: Lukasz Majczak <lma@chromium.org>
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 6ffa43d245b4..6ac1b11fb675 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -991,8 +991,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
struct hid_device *hid = ihid->hid;
int ret;
+ /*
+ * 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 on a bus error. Retry the command in this case.
+ */
ret = i2c_hid_fetch_hid_descriptor(ihid);
- if (ret < 0) {
+ if (ret == -ENXIO) {
+ usleep_range(400, 500);
+ ret = i2c_hid_fetch_hid_descriptor(ihid);
+ }
+ if (ret) {
i2c_hid_dbg(ihid, "failed to fetch HID descriptor: %d\n", ret);
return ret;
}
--
2.44.0
^ permalink raw reply related
* [PATCH v3 1/3] HID: i2c-hid: Rely on HID descriptor fetch to probe
From: Kenny Levinsen @ 2024-04-26 22:47 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: <20240426225739.2166-1-kl@kl.wtf>
To avoid error messages when a device is not present, b3a81b6c4fc6 added
an initial bus probe using a dummy i2c_smbus_read_byte() call.
Without this probe, i2c_hid_fetch_hid_descriptor() will fail internally
on a bus error and log. Treat the bus error as a missing device and
remove the error log so we can do away with the probe.
Tested-by: Lukasz Majczak <lma@chromium.org>
Reviewed-by: Lukasz Majczak <lma@chromium.org>
Signed-off-by: Kenny Levinsen <kl@kl.wtf>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index d965382196c6..6ffa43d245b4 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -872,12 +872,11 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
ihid->wHIDDescRegister,
&ihid->hdesc,
sizeof(ihid->hdesc));
- if (error) {
- dev_err(&ihid->client->dev,
- "failed to fetch HID descriptor: %d\n",
- error);
- return -ENODEV;
- }
+
+ /* The i2c drivers are a bit inconsistent with their error
+ * codes, so treat everything as -ENXIO for now. */
+ if (error)
+ return -ENXIO;
}
/* Validate the length of HID descriptor, the 4 first bytes:
@@ -992,17 +991,9 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
struct hid_device *hid = ihid->hid;
int ret;
- /* Make sure there is something at this address */
- ret = i2c_smbus_read_byte(client);
- if (ret < 0) {
- i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
- return -ENXIO;
- }
-
ret = i2c_hid_fetch_hid_descriptor(ihid);
if (ret < 0) {
- dev_err(&client->dev,
- "Failed to fetch the HID Descriptor\n");
+ i2c_hid_dbg(ihid, "failed to fetch HID descriptor: %d\n", ret);
return ret;
}
--
2.44.0
^ permalink raw reply related
* [PATCH v3 0/3] HID: i2c-hid: Probe and wake device with HID descriptor fetch
From: Kenny Levinsen @ 2024-04-26 22:47 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
The previous iteration of this was a bit naïve about bus error code
consistency from i2c drivers. Instead of trying to differentiate between
them, we now handle all bus errors during HID descriptor fetch the same
with a single debug log, as suggested by Doug[0].
As the change is relatively minor, I have carried over Łukasz' Tested-by
and Reviewed-by tags.
Third time's the charm?
[1] https://lore.kernel.org/all/CAD=FV=Xr6NsW085Sc+NhVmGDOn-zCCQ65CMNce_DsHxtXUgm9w@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Andreas Kemnade @ 2024-04-26 18:53 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Felix Kaechele, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, o.rempel, u.kleine-koenig, hdegoede, ye.xingchen,
p.puschmann, linux-input, devicetree, linux-kernel,
caleb.connolly
In-Reply-To: <CAHp75VdA_peJBWXFWSs-vDxA86y6MG0J7ixzXExqDxJJzfJ7mA@mail.gmail.com>
Hi,
On Fri, 26 Apr 2024 16:51:59 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > I think focaltech,ft5406 is better because it is consistent with other drivers/
> > bindings. We do not specify the display it is used on on other touchscreen
> > bindings. We do not specify the actual LEDs behind a LED interface chip.
> > And often the chip name is more easily to find out than the name of a
> > display.
>
> I'm _not_ a DT person, you should clarify this with them. I just
> pointed out the possible mistake (from Linux / DT perspective), and
> not hardware / business related one. If they are fine with focaltech
> prefix, then it's fine to me as well!
as 1/2 is Acked-by DT, I think the compatible is fine, so I will probably
just send a v3 with a sorted 2/2.
Regards,
Andreas
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
From: Rob Herring @ 2024-04-26 15:36 UTC (permalink / raw)
To: James McGregor
Cc: linux-input, Hans de Goede, Krzysztof Kozlowski, linux-sunxi,
devicetree, linux-arm-kernel, Jernej Skrabec, Conor Dooley,
Samuel Holland, Andre Przywara, Chen-Yu Tsai, Dmitry Torokhov
In-Reply-To: <20240426092924.15489-2-jamcgregor@protonmail.com>
On Fri, 26 Apr 2024 09:29:41 +0000, James McGregor wrote:
> 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>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> ---
> .../bindings/input/allwinner,sun4i-a10-lradc-keys.yaml | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.
If a tag was not added on purpose, please state why and what changed.
Missing tags:
Acked-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 2/2] ARM: dts: sun50i: Add LRADC node
From: Jernej Škrabec @ 2024-04-26 14:20 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Samuel Holland, Andre Przywara,
James McGregor
Cc: linux-input, devicetree, linux-arm-kernel, linux-sunxi
In-Reply-To: <20240426092924.15489-3-jamcgregor@protonmail.com>
Dne petek, 26. april 2024 ob 11:29:48 GMT +2 je James McGregor napisal(a):
> Add a DT node for the Allwinner H616 LRADC describing the base address,
> interrupt, reset and clock gates.
>
> Signed-off-by: James McGregor <jamcgregor@protonmail.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Jernej Škrabec <jernej.skrabec@gmail.com>
Best regards,
Jernej
> ---
> V1 -> V2: Moved DT node to correct place in tree order
>
> arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> index a061b69c07c2..1e8538ca7db0 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> @@ -594,6 +594,16 @@ ths: thermal-sensor@5070400 {
> #thermal-sensor-cells = <1>;
> };
>
> + lradc: lradc@5070800 {
> + compatible = "allwinner,sun50i-h616-lradc",
> + "allwinner,sun50i-r329-lradc";
> + reg = <0x05070800 0x400>;
> + interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&ccu CLK_BUS_KEYADC>;
> + resets = <&ccu RST_BUS_KEYADC>;
> + status = "disabled";
> + };
> +
> usbotg: usb@5100000 {
> compatible = "allwinner,sun50i-h616-musb",
> "allwinner,sun8i-h3-musb";
>
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Andy Shevchenko @ 2024-04-26 13:51 UTC (permalink / raw)
To: Andreas Kemnade
Cc: Felix Kaechele, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, o.rempel, u.kleine-koenig, hdegoede, ye.xingchen,
p.puschmann, linux-input, devicetree, linux-kernel,
caleb.connolly
In-Reply-To: <20240426095617.4e442681@aktux>
On Fri, Apr 26, 2024 at 10:56 AM Andreas Kemnade <andreas@kemnade.info> wrote:
> On Thu, 25 Apr 2024 22:54:13 -0400
> Felix Kaechele <felix@kaechele.ca> wrote:
> > On 2024-04-25 12:54, Andreas Kemnade wrote:
> > > On Fri, 5 Apr 2024 20:21:19 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >> On Fri, Apr 5, 2024 at 7:28 PM Andreas Kemnade <andreas@kemnade.info> wrote: >>> On Fri, 5 Apr 2024 18:13:45 +0300
> > >>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
...
> > >>>> Why a different vendor prefix?
> >
> > >>> I sorted by the numbers. Looking at datasheets for other controllers I see >>> https://www.displayfuture.com/Display/datasheet/controller/FT5x06.pdf
> > >>> it only mentions FocalTech Systems Co., Ltd.
> > >>
> > >> But does the driver use that? AFAICS it uses edt. Perhaps it's due to
> > >> a business split, not to my knowledge anyway.
> >
> > I've been looking into this over the past few weeks as I was working on
> > mainline support for an Android device.
> > And please forgive me if any of the following is not fully accurate, I'm
> > not an industry expert.
> >
> > After some research, my understanding of this is as follows:
> >
> > - There are companies that make touch ICs, LCD driver ICs and sometimes
> > even ICs that are both. Focaltech or Himax are examples of such companies.
> >
> > - There are companies that make LCMs. These are complete assemblies of
> > panel, backlight, touch layer and driver circuitry PCBs. This is what
> > OEMs generally purchase when they design a consumer device. Emerging
> > Display Technologies Corp. (EDT) is such a LCM manufacturing company.
> > More often than not LCM manufacturers do not make their own driver ICs.
> >
> > LCM manufacturers include ICs from Focaltech in their LCMs.
> > To my knowledge Focaltech is not a manufacturer of LCMs.
> >
> > As such, an interpretation of the compatible string "edt,edt-ft5406"
> > could be: Unspecified EDT LCM with Focaltech FT5406 IC.
> >
> > From my perspective, more correct would either be something like
> > "edt,etm070001bdh6" (the LCM by EDT that contains this IC, especially if
> > it had model specific quirks) or "focaltech,ft5406".
> > But "edt,edt-ft5406" is incorrect if being specific is the goal here.
> > Given that the driver predates much of the DT binding rigour it's what
> > we have now though.
> >
> I think focaltech,ft5406 is better because it is consistent with other drivers/
> bindings. We do not specify the display it is used on on other touchscreen
> bindings. We do not specify the actual LEDs behind a LED interface chip.
> And often the chip name is more easily to find out than the name of a
> display.
I'm _not_ a DT person, you should clarify this with them. I just
pointed out the possible mistake (from Linux / DT perspective), and
not hardware / business related one. If they are fine with focaltech
prefix, then it's fine to me as well!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v2 2/2] ARM: dts: sun50i: Add LRADC node
From: James McGregor @ 2024-04-26 9:29 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Andre Przywara
Cc: linux-input, devicetree, linux-arm-kernel, linux-sunxi
In-Reply-To: <20240426092924.15489-1-jamcgregor@protonmail.com>
Add a DT node for the Allwinner H616 LRADC describing the base address,
interrupt, reset and clock gates.
Signed-off-by: James McGregor <jamcgregor@protonmail.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
V1 -> V2: Moved DT node to correct place in tree order
arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
index a061b69c07c2..1e8538ca7db0 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
@@ -594,6 +594,16 @@ ths: thermal-sensor@5070400 {
#thermal-sensor-cells = <1>;
};
+ lradc: lradc@5070800 {
+ compatible = "allwinner,sun50i-h616-lradc",
+ "allwinner,sun50i-r329-lradc";
+ reg = <0x05070800 0x400>;
+ interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&ccu CLK_BUS_KEYADC>;
+ resets = <&ccu RST_BUS_KEYADC>;
+ status = "disabled";
+ };
+
usbotg: usb@5100000 {
compatible = "allwinner,sun50i-h616-musb",
"allwinner,sun8i-h3-musb";
--
2.34.1
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
From: James McGregor @ 2024-04-26 9:29 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Andre Przywara
Cc: linux-input, devicetree, linux-arm-kernel, linux-sunxi
In-Reply-To: <20240426092924.15489-1-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>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
.../bindings/input/allwinner,sun4i-a10-lradc-keys.yaml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml b/Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml
index c384bf0bb25d..6bdb8040be65 100644
--- a/Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml
+++ b/Documentation/devicetree/bindings/input/allwinner,sun4i-a10-lradc-keys.yaml
@@ -22,7 +22,9 @@ properties:
- const: allwinner,sun8i-a83t-r-lradc
- const: allwinner,sun50i-r329-lradc
- items:
- - const: allwinner,sun20i-d1-lradc
+ - enum:
+ - allwinner,sun50i-h616-lradc
+ - allwinner,sun20i-d1-lradc
- const: allwinner,sun50i-r329-lradc
reg:
--
2.34.1
^ permalink raw reply related
* [PATCH v2 0/2] arm64: dts: allwinner: H616: Add LRADC
From: James McGregor @ 2024-04-26 9:29 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Andre Przywara
Cc: linux-input, devicetree, linux-arm-kernel, linux-sunxi
Version 2 moves the LRADC DT node to the right place. It was out of
order before.
The Allwinner H616 series of SoCs have a low-rate ADC (LRADC) with
6-bit resolution and one input channel. They're compatible with the
existing drivers, so it only needs to be enabled in the DT.
Add an LRADC node to the H616 .dtsi, so board DTs can use them by
adding 'status = "okay";'.
This was tested on an OrangePi Zero 2W by attaching an expansion board
with two key buttons connected to the LRADC, and adding them to the DT.
/dev/input/event0 then properly reported the button presses. The patches
are based off sunxi/for-next.
James McGregor (2):
dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
ARM: dts: sun50i: Add LRADC node
.../bindings/input/allwinner,sun4i-a10-lradc-keys.yaml | 4 +++-
arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 10 ++++++++++
2 files changed, 13 insertions(+), 1 deletion(-)
--
2.34.1
^ permalink raw reply
* Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage
From: Johan Hovold @ 2024-04-26 9:29 UTC (permalink / raw)
To: Doug Anderson
Cc: Johan Hovold, 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: <CAD=FV=Vxgu==8Cv3sDydFpEdd6ws2stkZvxvajE1OAFm2BgmXw@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Andreas Kemnade @ 2024-04-26 7:56 UTC (permalink / raw)
To: Felix Kaechele
Cc: Andy Shevchenko, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
conor+dt, o.rempel, u.kleine-koenig, hdegoede, ye.xingchen,
p.puschmann, linux-input, devicetree, linux-kernel,
caleb.connolly
In-Reply-To: <7dd1eb70-b011-4247-aea9-173ddcd17dc7@kaechele.ca>
On Thu, 25 Apr 2024 22:54:13 -0400
Felix Kaechele <felix@kaechele.ca> wrote:
> On 2024-04-25 12:54, Andreas Kemnade wrote:
> > On Fri, 5 Apr 2024 20:21:19 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> >
> >> On Fri, Apr 5, 2024 at 7:28 PM Andreas Kemnade <andreas@kemnade.info> wrote: >>> On Fri, 5 Apr 2024 18:13:45 +0300
> >>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> ...
>
> >>>> Why a different vendor prefix?
>
> ...
>
> >>> I sorted by the numbers. Looking at datasheets for other controllers I see >>> https://www.displayfuture.com/Display/datasheet/controller/FT5x06.pdf
> >>> it only mentions FocalTech Systems Co., Ltd.
> >>
> >> But does the driver use that? AFAICS it uses edt. Perhaps it's due to
> >> a business split, not to my knowledge anyway.
>
> I've been looking into this over the past few weeks as I was working on
> mainline support for an Android device.
> And please forgive me if any of the following is not fully accurate, I'm
> not an industry expert.
>
> After some research, my understanding of this is as follows:
>
> - There are companies that make touch ICs, LCD driver ICs and sometimes
> even ICs that are both. Focaltech or Himax are examples of such companies.
>
> - There are companies that make LCMs. These are complete assemblies of
> panel, backlight, touch layer and driver circuitry PCBs. This is what
> OEMs generally purchase when they design a consumer device. Emerging
> Display Technologies Corp. (EDT) is such a LCM manufacturing company.
> More often than not LCM manufacturers do not make their own driver ICs.
>
> LCM manufacturers include ICs from Focaltech in their LCMs.
> To my knowledge Focaltech is not a manufacturer of LCMs.
>
> As such, an interpretation of the compatible string "edt,edt-ft5406"
> could be: Unspecified EDT LCM with Focaltech FT5406 IC.
>
> From my perspective, more correct would either be something like
> "edt,etm070001bdh6" (the LCM by EDT that contains this IC, especially if
> it had model specific quirks) or "focaltech,ft5406".
> But "edt,edt-ft5406" is incorrect if being specific is the goal here.
> Given that the driver predates much of the DT binding rigour it's what
> we have now though.
>
I think focaltech,ft5406 is better because it is consistent with other drivers/
bindings. We do not specify the display it is used on on other touchscreen
bindings. We do not specify the actual LEDs behind a LED interface chip.
And often the chip name is more easily to find out than the name of a
display.
[...]
> I don't think that's how the compatible strings are used today, but it
> is what would make sense in my opinion.
>
> > Looking around I found this:
> > if (tsdata->version == EV_FT)
> > swap(x, y);
> > ...
> > case 0x59: /* Evervision Display with FT5xx6 TS */
> > tsdata->version = EV_FT;
> >
> > I need swap(x.y), I am using touchscreen-swapped-x-y property now.
> > So evervision prefix?
>
> The compatible string doesn't have any bearing on whether x and y are
> swapped. The driver relies on its device detection heuristic for that
> determination.
Well, I think, yes, it could have a bearing, on some models, maybe x is
swapped with some pressure value in some record.
> Ideally, the driver would allow describing this property
>
> 1. in the devicetree using the "touchscreen-swapped-x-y" property
> from the common touchscreen bindings
>
It allows that right now.
Well, the devicetree should be driver/os independent, so it describes the
hardware. so the driver should spit out the records specified as x
in the chip manual as x and the ones specified as y as y.
If the display is then wired up to the controller in an unusual way,
it can be compensated by the devicetree specification (if we go the
focaltech,ftxxxx road instead of edt,etm070001bdh6).
But datasheets... where are you? So we need a heuristic. I guess
the order used most is probably the one used in the data sheet.
> 2. by extending the edt_i2c_chip_data struct to hold that property
> and set it based on the compatible string if it is, in fact, a property
> of that specific IC
>
yes, agreed.
Regards,
Andreas
^ permalink raw reply
* [syzbot] [input?] [ext4?] possible deadlock in uinput_request_submit
From: syzbot @ 2024-04-26 4:42 UTC (permalink / raw)
To: dmitry.torokhov, linux-ext4, linux-fsdevel, linux-input,
linux-kernel, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: 7b4f2bc91c15 Add linux-next specific files for 20240418
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=12a07f67180000
kernel config: https://syzkaller.appspot.com/x/.config?x=ae644165a243bf62
dashboard link: https://syzkaller.appspot.com/bug?extid=159077b1355b8cd72757
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1273c763180000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14b59430980000
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/524a18e6c5be/disk-7b4f2bc9.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/029f1b84d653/vmlinux-7b4f2bc9.xz
kernel image: https://storage.googleapis.com/syzbot-assets/c02d1542e886/bzImage-7b4f2bc9.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/95b3c106e235/mount_6.gz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+159077b1355b8cd72757@syzkaller.appspotmail.com
WARNING: possible circular locking dependency detected
6.9.0-rc4-next-20240418-syzkaller #0 Not tainted
------------------------------------------------------
syz-executor109/5116 is trying to acquire lock:
ffff8880117e3870 (&newdev->mutex){+.+.}-{3:3}, at: uinput_request_send drivers/input/misc/uinput.c:151 [inline]
ffff8880117e3870 (&newdev->mutex){+.+.}-{3:3}, at: uinput_request_submit+0x19c/0x740 drivers/input/misc/uinput.c:182
but task is already holding lock:
ffff888015fb60b0
(&ff->mutex
){+.+.}-{3:3}
, at: input_ff_upload+0x3e4/0xb00 drivers/input/ff-core.c:120
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #3 (
&ff->mutex){+.+.}-{3:3}
:
lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5754
__mutex_lock_common kernel/locking/mutex.c:608 [inline]
__mutex_lock+0x136/0xd70 kernel/locking/mutex.c:752
input_ff_flush+0x5e/0x140 drivers/input/ff-core.c:240
input_flush_device+0x9c/0xc0 drivers/input/input.c:686
evdev_release+0xf9/0x7d0 drivers/input/evdev.c:444
__fput+0x406/0x8b0 fs/file_table.c:422
__do_sys_close fs/open.c:1555 [inline]
__se_sys_close fs/open.c:1540 [inline]
__x64_sys_close+0x7f/0x110 fs/open.c:1540
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
-> #2 (
&dev->mutex
#2){+.+.}-{3:3}
:
lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5754
__mutex_lock_common kernel/locking/mutex.c:608 [inline]
__mutex_lock+0x136/0xd70 kernel/locking/mutex.c:752
input_register_handle+0x6d/0x3b0 drivers/input/input.c:2555
kbd_connect+0xbf/0x130 drivers/tty/vt/keyboard.c:1589
input_attach_handler drivers/input/input.c:1064 [inline]
input_register_device+0xcfa/0x1090 drivers/input/input.c:2396
acpi_button_add+0x6c6/0xb90 drivers/acpi/button.c:604
acpi_device_probe+0xa5/0x2b0 drivers/acpi/bus.c:1063
really_probe+0x2b8/0xad0 drivers/base/dd.c:656
__driver_probe_device+0x1a2/0x390 drivers/base/dd.c:798
driver_probe_device+0x50/0x430 drivers/base/dd.c:828
__driver_attach+0x45f/0x710 drivers/base/dd.c:1214
bus_for_each_dev+0x239/0x2b0 drivers/base/bus.c:368
bus_add_driver+0x346/0x670 drivers/base/bus.c:673
driver_register+0x23a/0x320 drivers/base/driver.c:246
do_one_initcall+0x248/0x880 init/main.c:1263
do_initcall_level+0x157/0x210 init/main.c:1325
do_initcalls+0x3f/0x80 init/main.c:1341
kernel_init_freeable+0x435/0x5d0 init/main.c:1574
kernel_init+0x1d/0x2b0 init/main.c:1463
ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
-> #1 (
input_mutex){+.+.}-{3:3}
:
lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5754
__mutex_lock_common kernel/locking/mutex.c:608 [inline]
__mutex_lock+0x136/0xd70 kernel/locking/mutex.c:752
input_register_device+0xae5/0x1090 drivers/input/input.c:2389
uinput_create_device+0x40e/0x630 drivers/input/misc/uinput.c:365
uinput_ioctl_handler+0x48b/0x1770 drivers/input/misc/uinput.c:904
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:907 [inline]
__se_sys_ioctl+0xfc/0x170 fs/ioctl.c:893
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
-> #0
(&newdev->mutex
){+.+.}-{3:3}
:
check_prev_add kernel/locking/lockdep.c:3134 [inline]
check_prevs_add kernel/locking/lockdep.c:3253 [inline]
validate_chain+0x18cb/0x58e0 kernel/locking/lockdep.c:3869
__lock_acquire+0x1346/0x1fd0 kernel/locking/lockdep.c:5137
lock_acquire+0x1ed/0x550 kernel/locking/lockdep.c:5754
__mutex_lock_common kernel/locking/mutex.c:608 [inline]
__mutex_lock+0x136/0xd70 kernel/locking/mutex.c:752
uinput_request_send drivers/input/misc/uinput.c:151 [inline]
uinput_request_submit+0x19c/0x740 drivers/input/misc/uinput.c:182
uinput_dev_upload_effect+0x199/0x240 drivers/input/misc/uinput.c:257
input_ff_upload+0x5df/0xb00 drivers/input/ff-core.c:150
evdev_do_ioctl drivers/input/evdev.c:1183 [inline]
evdev_ioctl_handler+0x17d0/0x21b0 drivers/input/evdev.c:1272
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:907 [inline]
__se_sys_ioctl+0xfc/0x170 fs/ioctl.c:893
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xf5/0x240 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
other info that might help us debug this:
Chain exists of:
&newdev->mutex
--> &dev->mutex
#2 -->
&ff->mutex
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(
&ff->mutex);
lock(&dev->mutex
#2);
lock(&ff->mutex
);
lock(&newdev->mutex
);
*** DEADLOCK ***
2 locks held by syz-executor109/5116:
#0: ffff88801cac6110
(&evdev->mutex
){+.+.}-{3:3}
, at: evdev_ioctl_handler+0x125/0x21b0 drivers/input/evdev.c:1263
#1: ffff888015fb60b0
(&ff->mutex
){+.+.}-{3:3}
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.
If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Felix Kaechele @ 2024-04-26 2:54 UTC (permalink / raw)
To: Andreas Kemnade, Andy Shevchenko
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
o.rempel, u.kleine-koenig, hdegoede, ye.xingchen, p.puschmann,
linux-input, devicetree, linux-kernel, caleb.connolly
In-Reply-To: <20240425185417.0a5f9c19@aktux>
On 2024-04-25 12:54, Andreas Kemnade wrote:
> On Fri, 5 Apr 2024 20:21:19 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Fri, Apr 5, 2024 at 7:28 PM Andreas Kemnade <andreas@kemnade.info> wrote: >>> On Fri, 5 Apr 2024 18:13:45 +0300
>>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
...
>>>> Why a different vendor prefix?
...
>>> I sorted by the numbers. Looking at datasheets for other controllers I see >>> https://www.displayfuture.com/Display/datasheet/controller/FT5x06.pdf
>>> it only mentions FocalTech Systems Co., Ltd.
>>
>> But does the driver use that? AFAICS it uses edt. Perhaps it's due to
>> a business split, not to my knowledge anyway.
I've been looking into this over the past few weeks as I was working on
mainline support for an Android device.
And please forgive me if any of the following is not fully accurate, I'm
not an industry expert.
After some research, my understanding of this is as follows:
- There are companies that make touch ICs, LCD driver ICs and sometimes
even ICs that are both. Focaltech or Himax are examples of such companies.
- There are companies that make LCMs. These are complete assemblies of
panel, backlight, touch layer and driver circuitry PCBs. This is what
OEMs generally purchase when they design a consumer device. Emerging
Display Technologies Corp. (EDT) is such a LCM manufacturing company.
More often than not LCM manufacturers do not make their own driver ICs.
LCM manufacturers include ICs from Focaltech in their LCMs.
To my knowledge Focaltech is not a manufacturer of LCMs.
As such, an interpretation of the compatible string "edt,edt-ft5406"
could be: Unspecified EDT LCM with Focaltech FT5406 IC.
From my perspective, more correct would either be something like
"edt,etm070001bdh6" (the LCM by EDT that contains this IC, especially if
it had model specific quirks) or "focaltech,ft5406".
But "edt,edt-ft5406" is incorrect if being specific is the goal here.
Given that the driver predates much of the DT binding rigour it's what
we have now though.
> Well, lets cite edt-ft5x06.rst:
>
> "The edt-ft5x06 driver is useful for the EDT "Polytouch" family of capacitive > touch screens. Note that it is *not* suitable for other devices based
on the
> focaltec ft5x06 devices, since they contain vendor-specific firmware. In
> particular this driver is not suitable for the Nook tablet."
That contradicts my experience with this driver. It works fine on a BOE
TV080WXM-LL4 LCM with a FT8201 without modifications.
> So chips from focaltech which can be equipped with different firmware?
Firmware can change, the vendor drivers support handling firmware transfers.
Apparently firmware is handled differently, depending on how a
manufacturer designs their LCM.
They can be shipped without flash, with flash but no firmware programmed
or complete with firmware.
In all but the last scenario the driver would have to load (and possibly
program to flash) the firmware for the LCM to be fully operational to
specifications. Today, this driver doesn't do that. As such the
behaviour is unlikely to change and could as well be described in the
devicetree instead of having the driver try guessing.
I personally would rather trust whoever is performing the integration
work to properly describe the hardware in the devicetree if the firmware
doesn't allow for a definitive way to identify how it would like to be
treated.
If the driver ever gained firmware handling features I'd expect the
filename for the firmware to be defined in the devicetree together with
the properties describing the treatment it expects.
> So edt prefix means EDT firmware?
I don't think that's how the compatible strings are used today, but it
is what would make sense in my opinion.
> Looking around I found this:
> if (tsdata->version == EV_FT)
> swap(x, y);
> ...
> case 0x59: /* Evervision Display with FT5xx6 TS */
> tsdata->version = EV_FT;
>
> I need swap(x.y), I am using touchscreen-swapped-x-y property now.
> So evervision prefix?
The compatible string doesn't have any bearing on whether x and y are
swapped. The driver relies on its device detection heuristic for that
determination.
Ideally, the driver would allow describing this property
1. in the devicetree using the "touchscreen-swapped-x-y" property
from the common touchscreen bindings
2. by extending the edt_i2c_chip_data struct to hold that property
and set it based on the compatible string if it is, in fact, a property
of that specific IC
The only reason I created this wall of text is because I am intending to
submit a very similar patch, using the "focaltech,ft8201" string. So
getting a decision on this would be helpful for my efforts as well.
Regards,
Felix
^ permalink raw reply
* [dtor-input:for-linus] BUILD SUCCESS be81415a32ef6d8a8a85529fcfac03d05b3e757d
From: kernel test robot @ 2024-04-25 23:48 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
branch HEAD: be81415a32ef6d8a8a85529fcfac03d05b3e757d Input: xpad - add support for ASUS ROG RAIKIRI
elapsed time: 1466m
configs tested: 163
configs skipped: 3
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allnoconfig gcc
alpha allyesconfig gcc
alpha defconfig gcc
arc allnoconfig gcc
arc axs101_defconfig gcc
arc axs103_defconfig gcc
arc defconfig gcc
arc haps_hs_defconfig gcc
arc hsdk_defconfig gcc
arm allnoconfig clang
arm collie_defconfig gcc
arm defconfig clang
arm moxart_defconfig gcc
arm randconfig-001-20240425 clang
arm randconfig-002-20240425 clang
arm randconfig-003-20240425 clang
arm randconfig-004-20240425 clang
arm s3c6400_defconfig gcc
arm shmobile_defconfig gcc
arm sp7021_defconfig gcc
arm vexpress_defconfig gcc
arm64 allmodconfig clang
arm64 allnoconfig gcc
arm64 allyesconfig clang
arm64 defconfig gcc
csky allnoconfig gcc
csky defconfig gcc
hexagon allmodconfig clang
hexagon allnoconfig clang
hexagon allyesconfig clang
hexagon defconfig clang
hexagon randconfig-001-20240425 clang
hexagon randconfig-002-20240425 clang
i386 allmodconfig gcc
i386 allnoconfig gcc
i386 allyesconfig gcc
i386 buildonly-randconfig-001-20240425 gcc
i386 buildonly-randconfig-002-20240426 clang
i386 buildonly-randconfig-003-20240425 gcc
i386 buildonly-randconfig-005-20240426 clang
i386 buildonly-randconfig-006-20240425 gcc
i386 buildonly-randconfig-006-20240426 clang
i386 defconfig clang
i386 randconfig-002-20240426 clang
i386 randconfig-004-20240425 gcc
i386 randconfig-006-20240426 clang
i386 randconfig-012-20240426 clang
i386 randconfig-013-20240425 gcc
i386 randconfig-013-20240426 clang
i386 randconfig-014-20240425 gcc
i386 randconfig-014-20240426 clang
i386 randconfig-015-20240425 gcc
i386 randconfig-015-20240426 clang
i386 randconfig-016-20240425 gcc
i386 randconfig-016-20240426 clang
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch allyesconfig gcc
loongarch defconfig gcc
m68k allmodconfig gcc
m68k allnoconfig gcc
m68k allyesconfig gcc
m68k defconfig gcc
m68k hp300_defconfig gcc
m68k m5249evb_defconfig gcc
m68k q40_defconfig gcc
m68k stmark2_defconfig gcc
microblaze allmodconfig gcc
microblaze allnoconfig gcc
microblaze allyesconfig gcc
microblaze defconfig gcc
mips allmodconfig gcc
mips allnoconfig gcc
mips allyesconfig gcc
mips ip27_defconfig gcc
mips lemote2f_defconfig gcc
mips loongson1c_defconfig gcc
mips maltaup_xpa_defconfig gcc
mips pic32mzda_defconfig gcc
nios2 allmodconfig gcc
nios2 allnoconfig gcc
nios2 allyesconfig gcc
nios2 defconfig gcc
openrisc alldefconfig gcc
openrisc allmodconfig gcc
openrisc allnoconfig gcc
openrisc allyesconfig gcc
openrisc defconfig gcc
parisc allmodconfig gcc
parisc allnoconfig gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc64 alldefconfig gcc
parisc64 defconfig gcc
powerpc allmodconfig gcc
powerpc allnoconfig gcc
powerpc allyesconfig clang
powerpc randconfig-003-20240425 clang
powerpc sam440ep_defconfig gcc
powerpc tqm8540_defconfig gcc
powerpc walnut_defconfig gcc
riscv allmodconfig clang
riscv allnoconfig gcc
riscv allyesconfig clang
riscv defconfig clang
riscv rv32_defconfig clang
s390 allmodconfig clang
s390 allnoconfig clang
s390 allyesconfig gcc
s390 debug_defconfig gcc
s390 defconfig clang
sh allmodconfig gcc
sh allnoconfig gcc
sh allyesconfig gcc
sh defconfig gcc
sh kfr2r09-romimage_defconfig gcc
sh rsk7269_defconfig gcc
sh sh7757lcr_defconfig gcc
sparc allmodconfig gcc
sparc allnoconfig gcc
sparc allyesconfig gcc
sparc defconfig gcc
sparc64 allmodconfig gcc
sparc64 allyesconfig gcc
sparc64 defconfig gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig gcc
um defconfig clang
um i386_defconfig gcc
um randconfig-002-20240425 clang
um x86_64_defconfig clang
x86_64 allnoconfig clang
x86_64 allyesconfig clang
x86_64 buildonly-randconfig-002-20240425 gcc
x86_64 buildonly-randconfig-002-20240426 gcc
x86_64 buildonly-randconfig-005-20240426 gcc
x86_64 buildonly-randconfig-006-20240425 gcc
x86_64 buildonly-randconfig-006-20240426 gcc
x86_64 defconfig gcc
x86_64 randconfig-001-20240426 gcc
x86_64 randconfig-002-20240426 gcc
x86_64 randconfig-003-20240426 gcc
x86_64 randconfig-004-20240425 gcc
x86_64 randconfig-005-20240425 gcc
x86_64 randconfig-011-20240425 gcc
x86_64 randconfig-012-20240426 gcc
x86_64 randconfig-013-20240426 gcc
x86_64 randconfig-014-20240425 gcc
x86_64 randconfig-014-20240426 gcc
x86_64 randconfig-015-20240426 gcc
x86_64 randconfig-072-20240425 gcc
x86_64 randconfig-073-20240425 gcc
x86_64 randconfig-074-20240426 gcc
x86_64 randconfig-075-20240426 gcc
x86_64 randconfig-076-20240425 gcc
x86_64 randconfig-076-20240426 gcc
x86_64 rhel-8.3-func gcc
x86_64 rhel-8.3-rust clang
x86_64 rhel-8.3 gcc
xtensa allnoconfig gcc
xtensa allyesconfig gcc
xtensa virt_defconfig gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 02/11] HID: hexLIN: Add support for USB LIN bus adapter
From: Christoph Fritz @ 2024-04-25 20:49 UTC (permalink / raw)
To: Benjamin Tissoires
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,
Greg Kroah-Hartman, Jiri Slaby, Andreas Lauser, Jonathan Corbet,
linux-can, netdev, devicetree, linux-input, linux-serial
In-Reply-To: <5w4fhdfplmaowyiu7i327pziniwqnftgpn3ei6uttuezwgfgql@xnxikjjv6fob>
Hi Benjamin,
thanks for your review, please see my answers below.
...
> > +
> > +static int hexlin_tx_req_status(struct hexlin_priv_data *priv,
> > + const void *out_report, int len)
> > +{
> > + int ret;
> > + unsigned long t;
> > +
> > + mutex_lock(&priv->tx_lock);
>
> AFAICT, any operation using the device will use this function and
> therefore this is enforcing a single user at the same time.
>
> Is this a bus or a hw limitation?
It's a hw limitation.
> > +
> > + reinit_completion(&priv->wait_in_report);
> > +
> > + ret = hexlin_tx_report(priv, out_report, len);
> > + if (ret)
> > + goto tx_exit;
> > +
> > + t = wait_for_completion_killable_timeout(&priv->wait_in_report,
> > + msecs_to_jiffies(1000));
> > + if (!t)
> > + ret = -ETIMEDOUT;
> > +
> > + if (priv->is_error)
> > + ret = -EINVAL;
> > +
> > +tx_exit:
> > + mutex_unlock(&priv->tx_lock);
> > +
> > + return ret;
> > +}
...
> > +static int hexlin_raw_event(struct hid_device *hdev,
> > + struct hid_report *report, u8 *data, int sz)
> > +{
> > + struct hexlin_priv_data *priv;
> > + int ret;
> > +
> > + if (sz < 1 || sz > HEXLIN_PKGLEN_MAX)
> > + return -EREMOTEIO;
> > +
> > + priv = hid_get_drvdata(hdev);
> > +
> > + hid_dbg(hdev, "%s, size:%i, data[0]: 0x%02x\n", __func__, sz, data[0]);
> > +
> > + priv->is_error = false;
> > +
> > + switch (data[0]) {
> > + case HEXLIN_SUCCESS:
> > + if (sz != 1)
> > + return -EREMOTEIO;
>
> Could we have some #define for all of these sizes (here and in all of
> the other branches)?
OK
>
> > + hid_dbg(hdev, "HEXLIN_SUCCESS: 0x%02x\n", data[0]);
> > + complete(&priv->wait_in_report);
>
> Shouldn't you ensure that you currently have a request pending?
> This works as long as no-one opens the hidraw node (see my remark
> below).
Thanks for the heads up, there is no need for hidraw.
> > + break;
> > + case HEXLIN_FAIL:
> > + if (sz != 1)
> > + return -EREMOTEIO;
> > + hid_err(hdev, "HEXLIN_FAIL: 0x%02x\n", data[0]);
> > + priv->is_error = true;
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_VERSION:
> > + if (sz != 2)
> > + return -EREMOTEIO;
> > + priv->fw_version = data[1];
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_RESPONDER_ANSWER_ID:
> > + if (sz != 20)
> > + return -EREMOTEIO;
> > + BUILD_BUG_ON(sizeof(priv->rar) != 20);
>
> magical constants again
OK
>
> > + memcpy(&priv->rar, data, sizeof(priv->rar));
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_BAUDRATE:
> > + if (sz != 3)
> > + return -EREMOTEIO;
> > + BUILD_BUG_ON(sizeof(priv->baudrate) != 2);
> > + memcpy(&priv->baudrate, &data[1], sizeof(priv->baudrate));
> > + le16_to_cpus(priv->baudrate);
> > + complete(&priv->wait_in_report);
> > + break;
> > + /* following cases not initiated by us, so no complete() */
> > + case HEXLIN_FRAME:
> > + if (sz != 17) {
> > + hid_err_once(hdev, "frame size mismatch: %i\n", sz);
> > + return -EREMOTEIO;
> > + }
> > + ret = hexlin_queue_frames_insert(priv, &data[1], sz-1);
> > + if (ret) {
> > + hid_err(hdev, "failed to add frame: %i\n", ret);
> > + return ret;
> > + }
> > + break;
> > + case HEXLIN_ERROR:
> > + hid_err(hdev, "error from adapter\n");
> > + break;
> > + default:
> > + hid_err(hdev, "unknown event: 0x%02x\n", data[0]);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int init_hw(struct hexlin_priv_data *priv)
> > +{
> > + int ret;
> > +
> > + ret = hexlin_reset_dev(priv);
> > + if (ret)
> > + return ret;
> > +
> > + ret = hexlin_get_version(priv);
> > + if (ret)
> > + return ret;
> > +
> > + priv->baudrate = LIN_DEFAULT_BAUDRATE;
> > + ret = hexlin_set_baudrate(priv, priv->baudrate);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int hexlin_probe(struct hid_device *hdev,
> > + const struct hid_device_id *id)
> > +{
> > + struct hexlin_priv_data *priv;
> > + int ret;
> > +
> > + priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->hid_dev = hdev;
> > + hid_set_drvdata(hdev, priv);
> > +
> > + mutex_init(&priv->tx_lock);
> > +
> > + ret = hid_parse(hdev);
> > + if (ret) {
> > + hid_err(hdev, "hid parse failed with %d\n", ret);
> > + goto fail_and_free;
> > + }
> > +
> > + ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>
> Are you sure you want HID_CONNECT_HIDRAW?
>
> Given that your whole driver relies on the assumption that any command
> sent to the device is guarded by the mutex, if one client opens the
> hidraw node and starts sending commands behind your back you are
> screwed...
>
> Maybe use HID_CONNECT_DRIVER instead.
HID_CONNECT_DRIVER it is
>
> > + if (ret) {
> > + hid_err(hdev, "hid hw start failed with %d\n", ret);
> > + goto fail_and_stop;
> > + }
> > +
> > + ret = hid_hw_open(hdev);
> > + if (ret) {
> > + hid_err(hdev, "hid hw open failed with %d\n", ret);
> > + goto fail_and_close;
> > + }
> > +
> > + init_completion(&priv->wait_in_report);
> > +
> > + hid_device_io_start(hdev);
> > +
> > + ret = init_hw(priv);
> > + if (ret)
> > + goto fail_and_close;
> > +
> > + priv->ldev = register_lin(&hdev->dev, &hexlin_ldo);
> > + if (IS_ERR_OR_NULL(priv->ldev)) {
> > + ret = PTR_ERR(priv->ldev);
> > + goto fail_and_close;
> > + }
> > +
> > + hid_info(hdev, "hexLIN (fw-version: %u) probed\n", priv->fw_version);
>
> you are not calling hid_hw_close(hdev) here (on purpose I guess).
>
> However, this prevents the device to enter any sleep mode as the kernel
> will always consider it to be in use.
> Is there some open/close mechanism in LIN or in CAN that can tell the
> device that it needs to be opened or do we assume that the device needs
> to be powered on all the time?
One can bring the LIN device up and down, just like any other Ethernet
or CAN device. So, for revision 2 of this patchset, I added open/stop
handling. This allows for hid_hw_close(hdev) here and also makes
remove() handling way easier. Thanks for the heads up.
>
> > +
> > + return 0;
> > +
> > +fail_and_close:
> > + hid_hw_close(hdev);
> > +fail_and_stop:
> > + hid_hw_stop(hdev);
> > +fail_and_free:
> > + mutex_destroy(&priv->tx_lock);
> > + return ret;
> > +}
> > +
> > +static void hexlin_remove(struct hid_device *hdev)
> > +{
> > + struct hexlin_priv_data *priv = hid_get_drvdata(hdev);
> > +
> > + complete(&priv->wait_in_report);
>
> what if you get one LIN request just now, between those 2 calls?
>
> You should probably disable the ability to take the mutex before sending
> the complete call above or you might still have the mutex taken here.
>
> Also shouldn't you set priv->is_error = true before the complete?
>
> > + unregister_lin(priv->ldev);
> > + hid_hw_close(hdev);
> > + hid_hw_stop(hdev);
> > + mutex_destroy(&priv->tx_lock);
>
> Given how the device works, I think it would be safer to do this in the
> following order:
>
> // prevent any incoming event (assuming hidraw is not available)
> hid_hw_close(hdev);
> // ensure the device is powered off
> hid_hw_stop(hdev);
> // mark any pending request as failed
> priv->is_error = true;
> // mark the device as unusable
> priv->removed = true;
> complete(&priv->wait_in_report);
> // unregister
> unregister_lin(priv->ldev);
> // mutex is not used anymore
> mutex_destroy(&priv->tx_lock);
>
> (I might be wrong but this seems more sensible to me).
>
> Actually, instead of having a priv->removed boolean, you could also take
> and release the mutex before releasing it, this way you are sure to not
> be in the critical code section. This should work because you are using
> wait_for_completion_killable_timeout() and so after 1 s you are
> guaranteed to exit the mutex.
Thanks for the great explanation.
> > +}
> > +
> > +static const struct hid_device_id hexlin_table[] = {
> > + { HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_HEXLIN) },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, hexlin_table);
> > +
> > +static struct hid_driver hexlin_driver = {
> > + .name = "hexLIN",
> > + .id_table = hexlin_table,
> > + .probe = hexlin_probe,
> > + .remove = hexlin_remove,
> > + .raw_event = hexlin_raw_event,
> > +};
> > +
> > +static int __init hexlin_init(void)
> > +{
> > + return hid_register_driver(&hexlin_driver);
> > +}
> > +
> > +static void __exit hexlin_exit(void)
> > +{
> > + hid_unregister_driver(&hexlin_driver);
> > +}
> > +
> > +/*
> > + * When compiled into the kernel, initialize after the hid bus.
> > + */
> > +late_initcall(hexlin_init);
> > +module_exit(hexlin_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Christoph Fritz <christoph.fritz@hexdev.de>");
> > +MODULE_DESCRIPTION("LIN bus driver for hexLIN USB adapter");
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index 8376fb5e2d0b4..157d234e1d400 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -903,6 +903,7 @@
> > #define USB_DEVICE_ID_MCC_PMD1208LS 0x007a
> >
> > #define USB_VENDOR_ID_MCS 0x16d0
> > +#define USB_DEVICE_ID_MCS_HEXLIN 0x0648
> > #define USB_DEVICE_ID_MCS_GAMEPADBLOCK 0x0bcc
> >
> > #define USB_VENDOR_MEGAWORLD 0x07b5
> > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> > index e0bbf0c6345d6..328fcc61303f3 100644
> > --- a/drivers/hid/hid-quirks.c
> > +++ b/drivers/hid/hid-quirks.c
> > @@ -436,6 +436,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
> > { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_2) },
> > { HID_USB_DEVICE(USB_VENDOR_ID_GYRATION, USB_DEVICE_ID_GYRATION_REMOTE_3) },
> > #endif
> > +#if IS_ENABLED(CONFIG_HID_HEXLIN)
> > + { HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_HEXLIN) },
>
> Generally, the pattern for drivers in the HID subsystem is to rely on
> the vendor name, not the product, in order to be able to extend it to
> more than one product.
>
> Is your vendor name MCS? Or Hexdev?
>
> If so, the driver should likely be hid-hexdev.c...
We got the PID from MCS online shop here:
https://www.mcselec.com/index.php?page=shop.product_details&product_id=92&option=com_phpshop
Our vendor name is hexDEV, but the USB VID is MCS, and the product name
is hexLIN...
So is 'hid-hexlin.c' or 'hid-hexdev-hexlin.c' okay or does it need to
be named 'hid-mcs-hexlin.c' in spite MCS has nearly nothing to do with
it?
Cheers
-- Christoph
^ permalink raw reply
* Re: [PATCH v2 1/3] HID: i2c-hid: Rely on HID descriptor fetch to probe
From: Kenny Levinsen @ 2024-04-25 19:36 UTC (permalink / raw)
To: Doug Anderson
Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires, Hans de Goede,
Maxime Ripard, Kai-Heng Feng, Johan Hovold, linux-input,
linux-kernel, Radoslaw Biernacki, Lukasz Majczak
In-Reply-To: <CAD=FV=Xr6NsW085Sc+NhVmGDOn-zCCQ65CMNce_DsHxtXUgm9w@mail.gmail.com>
On 4/24/24 7:00 PM, Doug Anderson wrote:
> I worry a little bit about keying just off of -EREMOTEIO. If I'm
> skimming the code properly it's up to the different i2c bus controller
> to decide which error code to return here. Looking at, for instance,
> "i2c-qcom-geni.c", I see:
>
> [NACK] = {-ENXIO, "NACK: slv unresponsive, check its power/reset-ln"},
Hmm, good point. I decided to go through the drivers and check their
behavior on NACK, and based on my quick glance I found (insert accuracy
disclaimer):
- 52 drivers emitting ENXIO
- 14 drivers emitting EREMOTEIO
- 11 driver emitting EIO
- 5 drivers emitting ETIMEDOUT
- 1 driver emitting EAGAIN
- 1 driver emitting I2C_ERR_BERR (???)
So just EREMOTEIO is definitely not good enough. Looking at the drivers,
it seems like the majority of drivers emitting generic error codes could
just as well emit ENXIO on NACK. Room for improvement.
> Maybe we should just use dev_dbg() in all cases here when we fail to
> fetch the descriptor? ...otherwise I think some boards will start
> getting a noisy error message.
I'm okay with that. I don't like hiding a useful error message, but the
smbus probe would also have hidden bus errors.
I'll send a v3 with just dev_dbg, then if I (or someone else) end up
aligning more i2c drivers on their NACK error we can go to the stricter
check and incentivize the drivers to give meaningful error values...
^ permalink raw reply
* Re: [PATCH v9 1/8] x86/vmware: Correct macro names
From: Alexey Makhalov @ 2024-04-25 17:27 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-kernel, virtualization, hpa, dave.hansen, mingo, tglx, x86,
netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, timothym, akaher,
dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov
In-Reply-To: <20240425152130.GJZip0-l040XCyUapN@fat_crate.local>
On 4/25/24 8:21 AM, Borislav Petkov wrote:
> On Wed, Apr 24, 2024 at 04:14:06PM -0700, Alexey Makhalov wrote:
>> VCPU_RESERVED and LEGACY_X2APIC are not VMware hypercall commands.
>> These are bits in return value of VMWARE_CMD_GETVCPU_INFO command.
>> Change VMWARE_CMD_ prefix to GETVCPU_INFO_ one. And move bit-shift
>> operation to the macro body.
>
> I don't understand:
>
> $ git grep GETVCPU_INFO
> arch/x86/kernel/cpu/vmware.c:51:#define VMWARE_CMD_GETVCPU_INFO 68
> arch/x86/kernel/cpu/vmware.c:478: VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
>
> so that's a VMWARE_CMD 68, at least the prefix says so.
>
> And those two are *bits* in that eax which that hypercall returns.
>
> Or are those two bits generic but defined in a vmware-specific
> hypercall?
>
> Hm.
>
These are VMware hypercall commands:
#define VMWARE_CMD_GETVERSION 10
#define VMWARE_CMD_GETHZ 45
#define VMWARE_CMD_GETVCPU_INFO 68
#define VMWARE_CMD_STEALCLOCK 91
These are VMware-specific macros to analyze return values of
corresponding commands. They are prefixed with command name.
#define GETVCPU_INFO_LEGACY_X2APIC BIT(3)
#define GETVCPU_INFO_VCPU_RESERVED BIT(31)
#define STEALCLOCK_NOT_AVAILABLE (-1)
#define STEALCLOCK_DISABLED 0
#define STEALCLOCK_ENABLED 1
Name VMWARE_CMD_LEGACY_X2APIC was not correct as LEGACY_X2APIC is not a
command but the meaning of 3rd bit of a return value of
VMWARE_CMD_GETVCPU_INFO. So, change it to GETVCPU_INFO_LEGACY_X2APIC.
The same change with GETVCPU_INFO_VCPU_RESERVED.
Both these bits are not generic.
--Alexey
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: edt-ft5x06 - add ft5426
From: Andreas Kemnade @ 2024-04-25 16:54 UTC (permalink / raw)
To: Andy Shevchenko
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
o.rempel, u.kleine-koenig, hdegoede, ye.xingchen, p.puschmann,
linux-input, devicetree, linux-kernel, caleb.connolly
In-Reply-To: <CAHp75VckoDheCN-KQ0KcSk9rE_-cXFUujurtA4sK6KAixDttQQ@mail.gmail.com>
On Fri, 5 Apr 2024 20:21:19 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Fri, Apr 5, 2024 at 7:28 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> > On Fri, 5 Apr 2024 18:13:45 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Fri, Apr 5, 2024 at 1:20 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> ...
>
> > > > @@ -1484,6 +1484,7 @@ static const struct of_device_id edt_ft5x06_of_match[] = {
> > > > { .compatible = "edt,edt-ft5206", .data = &edt_ft5x06_data },
> > > > { .compatible = "edt,edt-ft5306", .data = &edt_ft5x06_data },
> > > > { .compatible = "edt,edt-ft5406", .data = &edt_ft5x06_data },
> > > > + { .compatible = "focaltech,ft5426", .data = &edt_ft5506_data },
> > >
> > > Why a different vendor prefix?
> > > In case you need to use this one, keep the list sorted, currently this
> > > splits the edt,* ones.
> > >
> > How do I know whether to use evervision or edt instead?
>
> Ask DT people, the vendor-prefixes lists both...
>
> > I sorted by the numbers. Looking at datasheets for other controllers I see
> > https://www.displayfuture.com/Display/datasheet/controller/FT5x06.pdf
> > it only mentions FocalTech Systems Co., Ltd.
>
> But does the driver use that? AFAICS it uses edt. Perhaps it's due to
> a business split, not to my knowledge anyway.
>
Well, lets cite edt-ft5x06.rst:
"The edt-ft5x06 driver is useful for the EDT "Polytouch" family of capacitive
touch screens. Note that it is *not* suitable for other devices based on the
focaltec ft5x06 devices, since they contain vendor-specific firmware. In
particular this driver is not suitable for the Nook tablet."
So chips from focaltech which can be equipped with different firmware? So
edt prefix means EDT firmware?
Looking around I found this:
if (tsdata->version == EV_FT)
swap(x, y);
...
case 0x59: /* Evervision Display with FT5xx6 TS */
tsdata->version = EV_FT;
I need swap(x.y), I am using touchscreen-swapped-x-y property now.
So evervision prefix?
Regards,
Andreas
^ permalink raw reply
* Re: [PATCH] Input: cyapa - add missing input core locking to suspend/resume functions
From: Andrzej Pietrasiewicz @ 2024-04-25 17:04 UTC (permalink / raw)
To: Marek Szyprowski, linux-input, linux-kernel; +Cc: Dmitry Torokhov
In-Reply-To: <5f9052f0-ef5f-4c4f-85f9-f07fffd2b5ef@samsung.com>
Hi Marek,
W dniu 25.04.2024 o 12:02, Marek Szyprowski pisze:
> On 09.10.2023 14:10, Marek Szyprowski wrote:
>> Grab input->mutex during suspend/resume functions like it is done in
>> other input drivers. This fixes the following warning during system
>> suspend/resume cycle on Samsung Exynos5250-based Snow Chromebook:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 1 PID: 1680 at drivers/input/input.c:2291 input_device_enabled+0x68/0x6c
>> Modules linked in: ...
>> CPU: 1 PID: 1680 Comm: kworker/u4:12 Tainted: G W 6.6.0-rc5-next-20231009 #14109
>> Hardware name: Samsung Exynos (Flattened Device Tree)
>> Workqueue: events_unbound async_run_entry_fn
>> unwind_backtrace from show_stack+0x10/0x14
>> show_stack from dump_stack_lvl+0x58/0x70
>> dump_stack_lvl from __warn+0x1a8/0x1cc
>> __warn from warn_slowpath_fmt+0x18c/0x1b4
>> warn_slowpath_fmt from input_device_enabled+0x68/0x6c
>> input_device_enabled from cyapa_gen3_set_power_mode+0x13c/0x1dc
>> cyapa_gen3_set_power_mode from cyapa_reinitialize+0x10c/0x15c
>> cyapa_reinitialize from cyapa_resume+0x48/0x98
>> cyapa_resume from dpm_run_callback+0x90/0x298
>> dpm_run_callback from device_resume+0xb4/0x258
>> device_resume from async_resume+0x20/0x64
>> async_resume from async_run_entry_fn+0x40/0x15c
>> async_run_entry_fn from process_scheduled_works+0xbc/0x6a8
>> process_scheduled_works from worker_thread+0x188/0x454
>> worker_thread from kthread+0x108/0x140
>> kthread from ret_from_fork+0x14/0x28
>> Exception stack(0xf1625fb0 to 0xf1625ff8)
>> ...
>> ---[ end trace 0000000000000000 ]---
>> ...
>> ------------[ cut here ]------------
>> WARNING: CPU: 1 PID: 1680 at drivers/input/input.c:2291 input_device_enabled+0x68/0x6c
>> Modules linked in: ...
>> CPU: 1 PID: 1680 Comm: kworker/u4:12 Tainted: G W 6.6.0-rc5-next-20231009 #14109
>> Hardware name: Samsung Exynos (Flattened Device Tree)
>> Workqueue: events_unbound async_run_entry_fn
>> unwind_backtrace from show_stack+0x10/0x14
>> show_stack from dump_stack_lvl+0x58/0x70
>> dump_stack_lvl from __warn+0x1a8/0x1cc
>> __warn from warn_slowpath_fmt+0x18c/0x1b4
>> warn_slowpath_fmt from input_device_enabled+0x68/0x6c
>> input_device_enabled from cyapa_gen3_set_power_mode+0x13c/0x1dc
>> cyapa_gen3_set_power_mode from cyapa_reinitialize+0x10c/0x15c
>> cyapa_reinitialize from cyapa_resume+0x48/0x98
>> cyapa_resume from dpm_run_callback+0x90/0x298
>> dpm_run_callback from device_resume+0xb4/0x258
>> device_resume from async_resume+0x20/0x64
>> async_resume from async_run_entry_fn+0x40/0x15c
>> async_run_entry_fn from process_scheduled_works+0xbc/0x6a8
>> process_scheduled_works from worker_thread+0x188/0x454
>> worker_thread from kthread+0x108/0x140
>> kthread from ret_from_fork+0x14/0x28
>> Exception stack(0xf1625fb0 to 0xf1625ff8)
>> ...
>> ---[ end trace 0000000000000000 ]---
>>
>> Fixes: d69f0a43c677 ("Input: use input_device_enabled()")
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> Gentle ping?
Thanks for pinging this gently ;)
Unfortunately I am unable to test it now, but I assume this
fixes the problem for you. If so, you can add my
Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>
>> ---
>> drivers/input/mouse/cyapa.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
>> index a84098448f5b..cf23f95b5f11 100644
>> --- a/drivers/input/mouse/cyapa.c
>> +++ b/drivers/input/mouse/cyapa.c
>> @@ -1347,10 +1347,16 @@ static int cyapa_suspend(struct device *dev)
>> u8 power_mode;
>> int error;
>>
>> - error = mutex_lock_interruptible(&cyapa->state_sync_lock);
>> + error = mutex_lock_interruptible(&cyapa->input->mutex);
>> if (error)
>> return error;
>>
>> + error = mutex_lock_interruptible(&cyapa->state_sync_lock);
>> + if (error) {
>> + mutex_unlock(&cyapa->input->mutex);
>> + return error;
>> + }
>> +
>> /*
>> * Runtime PM is enable only when device is in operational mode and
>> * users in use, so need check it before disable it to
>> @@ -1385,6 +1391,8 @@ static int cyapa_suspend(struct device *dev)
>> cyapa->irq_wake = (enable_irq_wake(client->irq) == 0);
>>
>> mutex_unlock(&cyapa->state_sync_lock);
>> + mutex_unlock(&cyapa->input->mutex);
>> +
>> return 0;
>> }
>>
>> @@ -1394,6 +1402,7 @@ static int cyapa_resume(struct device *dev)
>> struct cyapa *cyapa = i2c_get_clientdata(client);
>> int error;
>>
>> + mutex_lock(&cyapa->input->mutex);
>> mutex_lock(&cyapa->state_sync_lock);
>>
>> if (device_may_wakeup(dev) && cyapa->irq_wake) {
>> @@ -1412,6 +1421,7 @@ static int cyapa_resume(struct device *dev)
>> enable_irq(client->irq);
>>
>> mutex_unlock(&cyapa->state_sync_lock);
>> + mutex_unlock(&cyapa->input->mutex);
>> return 0;
>> }
>>
>
> Best regards
^ permalink raw reply
* Re: [PATCH v9 1/8] x86/vmware: Correct macro names
From: Borislav Petkov @ 2024-04-25 15:21 UTC (permalink / raw)
To: Alexey Makhalov
Cc: linux-kernel, virtualization, hpa, dave.hansen, mingo, tglx, x86,
netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, timothym, akaher,
dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov
In-Reply-To: <20240424231407.14098-1-alexey.makhalov@broadcom.com>
On Wed, Apr 24, 2024 at 04:14:06PM -0700, Alexey Makhalov wrote:
> VCPU_RESERVED and LEGACY_X2APIC are not VMware hypercall commands.
> These are bits in return value of VMWARE_CMD_GETVCPU_INFO command.
> Change VMWARE_CMD_ prefix to GETVCPU_INFO_ one. And move bit-shift
> operation to the macro body.
I don't understand:
$ git grep GETVCPU_INFO
arch/x86/kernel/cpu/vmware.c:51:#define VMWARE_CMD_GETVCPU_INFO 68
arch/x86/kernel/cpu/vmware.c:478: VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
so that's a VMWARE_CMD 68, at least the prefix says so.
And those two are *bits* in that eax which that hypercall returns.
Or are those two bits generic but defined in a vmware-specific
hypercall?
Hm.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH 3/3] HID: bpf: lazy load the hid_tail_call entrypoint
From: Benjamin Tissoires @ 2024-04-25 12:52 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild, Jiri Kosina, Benjamin Tissoires, lkp, oe-kbuild-all,
linux-input, linux-kernel, stable
In-Reply-To: <x7ffovagmqjazr6jhr6urkctcj7ozn6poakvjluorhzuxmyyg2@dxw7ucw3qc6z>
On Apr 24 2024, Benjamin Tissoires wrote:
> On Apr 23 2024, Dan Carpenter wrote:
> > Hi Benjamin,
> >
> > kernel test robot noticed the following build warnings:
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Tissoires/HID-bpf-fix-a-comment-in-a-define/20240419-225110
> > base: b912cf042072e12e93faa874265b30cc0aa521b9
> > patch link: https://lore.kernel.org/r/20240419-hid_bpf_lazy_skel-v1-3-9210bcd4b61c%40kernel.org
> > patch subject: [PATCH 3/3] HID: bpf: lazy load the hid_tail_call entrypoint
> > config: i386-randconfig-141-20240423 (https://download.01.org/0day-ci/archive/20240423/202404231109.h2IRrMMD-lkp@intel.com/config)
> > compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > | Closes: https://lore.kernel.org/r/202404231109.h2IRrMMD-lkp@intel.com/
> >
> > smatch warnings:
> > drivers/hid/bpf/hid_bpf_jmp_table.c:478 __hid_bpf_attach_prog() error: uninitialized symbol 'link'.
> >
> > vim +/link +478 drivers/hid/bpf/hid_bpf_jmp_table.c
> >
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 396 noinline int
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 397 __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type,
> > 7cdd2108903a4e3 Benjamin Tissoires 2024-01-24 398 int prog_fd, struct bpf_prog *prog, __u32 flags)
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 399 {
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 400 struct bpf_link_primer link_primer;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 401 struct hid_bpf_link *link;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 402 struct hid_bpf_prog_entry *prog_entry;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 403 int cnt, err = -EINVAL, prog_table_idx = -1;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 404
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 405 mutex_lock(&hid_bpf_attach_lock);
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 406
> > 60caa381da7dc38 Benjamin Tissoires 2024-04-19 407 if (!jmp_table.map) {
> > 60caa381da7dc38 Benjamin Tissoires 2024-04-19 408 err = hid_bpf_preload_skel();
> > 60caa381da7dc38 Benjamin Tissoires 2024-04-19 409 WARN_ONCE(err, "error while preloading HID BPF dispatcher: %d", err);
> > 60caa381da7dc38 Benjamin Tissoires 2024-04-19 410 if (err)
> > 60caa381da7dc38 Benjamin Tissoires 2024-04-19 411 goto err_unlock;
> > ^^^^^^^^^^^^^^^^
> > link isn't initialized.
>
> Well spotted! Thanks
> I'll send a v2 soon.
FWIW, this patch prevents the proper unloading of the HID-BPF programs.
So not sure we'll want it as such just now.
Cheers,
Benjamin
> >
> > 60caa381da7dc38 Benjamin Tissoires 2024-04-19 412 }
> > 60caa381da7dc38 Benjamin Tissoires 2024-04-19 413
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 414 link = kzalloc(sizeof(*link), GFP_USER);
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 415 if (!link) {
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 416 err = -ENOMEM;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 417 goto err_unlock;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 418 }
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 419
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 420 bpf_link_init(&link->link, BPF_LINK_TYPE_UNSPEC,
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 421 &hid_bpf_link_lops, prog);
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 422
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 423 /* do not attach too many programs to a given HID device */
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 424 cnt = hid_bpf_program_count(hdev, NULL, prog_type);
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 425 if (cnt < 0) {
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 426 err = cnt;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 427 goto err_unlock;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 428 }
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 429
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 430 if (cnt >= hid_bpf_max_programs(prog_type)) {
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 431 err = -E2BIG;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 432 goto err_unlock;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 433 }
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 434
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 435 prog_table_idx = hid_bpf_insert_prog(prog_fd, prog);
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 436 /* if the jmp table is full, abort */
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 437 if (prog_table_idx < 0) {
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 438 err = prog_table_idx;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 439 goto err_unlock;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 440 }
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 441
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 442 if (flags & HID_BPF_FLAG_INSERT_HEAD) {
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 443 /* take the previous prog_entry slot */
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 444 jmp_table.tail = PREV(jmp_table.tail);
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 445 prog_entry = &jmp_table.entries[jmp_table.tail];
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 446 } else {
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 447 /* take the next prog_entry slot */
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 448 prog_entry = &jmp_table.entries[jmp_table.head];
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 449 jmp_table.head = NEXT(jmp_table.head);
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 450 }
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 451
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 452 /* we steal the ref here */
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 453 prog_entry->prog = prog;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 454 prog_entry->idx = prog_table_idx;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 455 prog_entry->hdev = hdev;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 456 prog_entry->type = prog_type;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 457
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 458 /* finally store the index in the device list */
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 459 err = hid_bpf_populate_hdev(hdev, prog_type);
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 460 if (err) {
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 461 hid_bpf_release_prog_at(prog_table_idx);
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 462 goto err_unlock;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 463 }
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 464
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 465 link->hid_table_index = prog_table_idx;
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 466
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 467 err = bpf_link_prime(&link->link, &link_primer);
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 468 if (err)
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 469 goto err_unlock;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 470
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 471 mutex_unlock(&hid_bpf_attach_lock);
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 472
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 473 return bpf_link_settle(&link_primer);
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 474
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 475 err_unlock:
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 476 mutex_unlock(&hid_bpf_attach_lock);
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 477
> > 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 @478 kfree(link);
> > ^^^^
> >
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 479
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 480 return err;
> > f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 481 }
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests/wiki
> >
^ permalink raw reply
* Re: [PATCH] Input: cyapa - add missing input core locking to suspend/resume functions
From: Marek Szyprowski @ 2024-04-25 10:02 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: Dmitry Torokhov, Andrzej Pietrasiewicz
In-Reply-To: <20231009121018.1075318-1-m.szyprowski@samsung.com>
On 09.10.2023 14:10, Marek Szyprowski wrote:
> Grab input->mutex during suspend/resume functions like it is done in
> other input drivers. This fixes the following warning during system
> suspend/resume cycle on Samsung Exynos5250-based Snow Chromebook:
>
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1680 at drivers/input/input.c:2291 input_device_enabled+0x68/0x6c
> Modules linked in: ...
> CPU: 1 PID: 1680 Comm: kworker/u4:12 Tainted: G W 6.6.0-rc5-next-20231009 #14109
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Workqueue: events_unbound async_run_entry_fn
> unwind_backtrace from show_stack+0x10/0x14
> show_stack from dump_stack_lvl+0x58/0x70
> dump_stack_lvl from __warn+0x1a8/0x1cc
> __warn from warn_slowpath_fmt+0x18c/0x1b4
> warn_slowpath_fmt from input_device_enabled+0x68/0x6c
> input_device_enabled from cyapa_gen3_set_power_mode+0x13c/0x1dc
> cyapa_gen3_set_power_mode from cyapa_reinitialize+0x10c/0x15c
> cyapa_reinitialize from cyapa_resume+0x48/0x98
> cyapa_resume from dpm_run_callback+0x90/0x298
> dpm_run_callback from device_resume+0xb4/0x258
> device_resume from async_resume+0x20/0x64
> async_resume from async_run_entry_fn+0x40/0x15c
> async_run_entry_fn from process_scheduled_works+0xbc/0x6a8
> process_scheduled_works from worker_thread+0x188/0x454
> worker_thread from kthread+0x108/0x140
> kthread from ret_from_fork+0x14/0x28
> Exception stack(0xf1625fb0 to 0xf1625ff8)
> ...
> ---[ end trace 0000000000000000 ]---
> ...
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1680 at drivers/input/input.c:2291 input_device_enabled+0x68/0x6c
> Modules linked in: ...
> CPU: 1 PID: 1680 Comm: kworker/u4:12 Tainted: G W 6.6.0-rc5-next-20231009 #14109
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Workqueue: events_unbound async_run_entry_fn
> unwind_backtrace from show_stack+0x10/0x14
> show_stack from dump_stack_lvl+0x58/0x70
> dump_stack_lvl from __warn+0x1a8/0x1cc
> __warn from warn_slowpath_fmt+0x18c/0x1b4
> warn_slowpath_fmt from input_device_enabled+0x68/0x6c
> input_device_enabled from cyapa_gen3_set_power_mode+0x13c/0x1dc
> cyapa_gen3_set_power_mode from cyapa_reinitialize+0x10c/0x15c
> cyapa_reinitialize from cyapa_resume+0x48/0x98
> cyapa_resume from dpm_run_callback+0x90/0x298
> dpm_run_callback from device_resume+0xb4/0x258
> device_resume from async_resume+0x20/0x64
> async_resume from async_run_entry_fn+0x40/0x15c
> async_run_entry_fn from process_scheduled_works+0xbc/0x6a8
> process_scheduled_works from worker_thread+0x188/0x454
> worker_thread from kthread+0x108/0x140
> kthread from ret_from_fork+0x14/0x28
> Exception stack(0xf1625fb0 to 0xf1625ff8)
> ...
> ---[ end trace 0000000000000000 ]---
>
> Fixes: d69f0a43c677 ("Input: use input_device_enabled()")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Gentle ping?
> ---
> drivers/input/mouse/cyapa.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> index a84098448f5b..cf23f95b5f11 100644
> --- a/drivers/input/mouse/cyapa.c
> +++ b/drivers/input/mouse/cyapa.c
> @@ -1347,10 +1347,16 @@ static int cyapa_suspend(struct device *dev)
> u8 power_mode;
> int error;
>
> - error = mutex_lock_interruptible(&cyapa->state_sync_lock);
> + error = mutex_lock_interruptible(&cyapa->input->mutex);
> if (error)
> return error;
>
> + error = mutex_lock_interruptible(&cyapa->state_sync_lock);
> + if (error) {
> + mutex_unlock(&cyapa->input->mutex);
> + return error;
> + }
> +
> /*
> * Runtime PM is enable only when device is in operational mode and
> * users in use, so need check it before disable it to
> @@ -1385,6 +1391,8 @@ static int cyapa_suspend(struct device *dev)
> cyapa->irq_wake = (enable_irq_wake(client->irq) == 0);
>
> mutex_unlock(&cyapa->state_sync_lock);
> + mutex_unlock(&cyapa->input->mutex);
> +
> return 0;
> }
>
> @@ -1394,6 +1402,7 @@ static int cyapa_resume(struct device *dev)
> struct cyapa *cyapa = i2c_get_clientdata(client);
> int error;
>
> + mutex_lock(&cyapa->input->mutex);
> mutex_lock(&cyapa->state_sync_lock);
>
> if (device_may_wakeup(dev) && cyapa->irq_wake) {
> @@ -1412,6 +1421,7 @@ static int cyapa_resume(struct device *dev)
> enable_irq(client->irq);
>
> mutex_unlock(&cyapa->state_sync_lock);
> + mutex_unlock(&cyapa->input->mutex);
> return 0;
> }
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ 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-25 9:39 UTC (permalink / raw)
To: Johan Hovold
Cc: Johan Hovold, 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: <Zii2CUeIyBwxzrBu@hovoldconsulting.com>
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
^ permalink raw reply
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