* [PATCH regression fix 1/2] Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
From: Hans de Goede @ 2024-01-26 16:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Paul Menzel, stable, regressions, linux-input
In-Reply-To: <20240126160724.13278-1-hdegoede@redhat.com>
After commit 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in
translated mode") the keyboard on Dell XPS 13 9350 / 9360 / 9370 models
has stopped working after a suspend/resume.
The problem appears to be that atkbd_probe() fails when called
from atkbd_reconnect() on resume, which on systems where
ATKBD_CMD_GETID is skipped can only happen by ATKBD_CMD_SETLEDS
failing. ATKBD_CMD_SETLEDS failing because ATKBD_CMD_GETID was
skipped is weird, but apparently that is what is happening.
Fix this by also skipping ATKBD_CMD_SETLEDS when skipping
ATKBD_CMD_GETID.
Fixes: 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/linux-input/0aa4a61f-c939-46fe-a572-08022e8931c7@molgen.mpg.de/
Closes: https://bbs.archlinux.org/viewtopic.php?pid=2146300
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218424
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2260517
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/keyboard/atkbd.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index 13ef6284223d..c229bd6b3f7f 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -811,7 +811,6 @@ static int atkbd_probe(struct atkbd *atkbd)
{
struct ps2dev *ps2dev = &atkbd->ps2dev;
unsigned char param[2];
- bool skip_getid;
/*
* Some systems, where the bit-twiddling when testing the io-lines of the
@@ -825,6 +824,11 @@ static int atkbd_probe(struct atkbd *atkbd)
"keyboard reset failed on %s\n",
ps2dev->serio->phys);
+ if (atkbd_skip_getid(atkbd)) {
+ atkbd->id = 0xab83;
+ return 0;
+ }
+
/*
* Then we check the keyboard ID. We should get 0xab83 under normal conditions.
* Some keyboards report different values, but the first byte is always 0xab or
@@ -833,18 +837,17 @@ static int atkbd_probe(struct atkbd *atkbd)
*/
param[0] = param[1] = 0xa5; /* initialize with invalid values */
- skip_getid = atkbd_skip_getid(atkbd);
- if (skip_getid || ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
+ if (ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
/*
- * If the get ID command was skipped or failed, we check if we can at least set
+ * If the get ID command failed, we check if we can at least set
* the LEDs on the keyboard. This should work on every keyboard out there.
* It also turns the LEDs off, which we want anyway.
*/
param[0] = 0;
if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
return -1;
- atkbd->id = skip_getid ? 0xab83 : 0xabba;
+ atkbd->id = 0xabba;
return 0;
}
--
2.43.0
^ permalink raw reply related
* [PATCH regression fix 0/2] Input: atkbd - Fix Dell XPS 13 line suspend/resume regression
From: Hans de Goede @ 2024-01-26 16:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Paul Menzel, stable, regressions, linux-input
Hi Dmitry,
There have been multiple reports that the keyboard on
Dell XPS 13 9350 / 9360 / 9370 models has stopped working after
a suspend/resume after the merging of commit 936e4d49ecbc ("Input:
atkbd - skip ATKBD_CMD_GETID in translated mode").
See the 4 closes tags in the first patch for 4 reports of this.
I have been working with the first reporter on resolving this
and testing on his Dell XPS 13 9360 confirms that these patches
fix things.
Unfortunately the commit causing the issue has also been picked
up by multiple stable kernel series now. Can you please send
these fixes to Linus ASAP, so that they can also be backported
to the stable series ASAP ?
Alternatively we could revert the commit causing this, but that
commit is know to fix issues on a whole bunch of other laptops
so I would rather not revert it.
Regards,
Hans
Hans de Goede (2):
Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
Input: atkbd - Do not skip atkbd_deactivate() when skipping
ATKBD_CMD_GETID
drivers/input/keyboard/atkbd.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH regression fix 2/2] Input: atkbd - Do not skip atkbd_deactivate() when skipping ATKBD_CMD_GETID
From: Hans de Goede @ 2024-01-26 16:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Paul Menzel, stable, regressions, linux-input
In-Reply-To: <20240126160724.13278-1-hdegoede@redhat.com>
After commit 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in
translated mode") not only the getid command is skipped, but also
the de-activating of the keyboard at the end of atkbd_probe(), potentially
re-introducing the problem fixed by commit be2d7e4233a4 ("Input: atkbd -
fix multi-byte scancode handling on reconnect").
Make sure multi-byte scancode handling on reconnect is still handled
correctly by not skipping the atkbd_deactivate() call.
Fixes: 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/keyboard/atkbd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
index c229bd6b3f7f..7f67f9f2946b 100644
--- a/drivers/input/keyboard/atkbd.c
+++ b/drivers/input/keyboard/atkbd.c
@@ -826,7 +826,7 @@ static int atkbd_probe(struct atkbd *atkbd)
if (atkbd_skip_getid(atkbd)) {
atkbd->id = 0xab83;
- return 0;
+ goto deactivate_kbd;
}
/*
@@ -863,6 +863,7 @@ static int atkbd_probe(struct atkbd *atkbd)
return -1;
}
+deactivate_kbd:
/*
* Make sure nothing is coming from the keyboard and disturbs our
* internal state.
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Johan Hovold @ 2024-01-26 16:23 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Daniel Thompson, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Benjamin Tissoires,
Jiri Kosina, Bjorn Andersson, Konrad Dybcio, Johan Hovold,
linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <20240126145346.GN2936378@hu-bjorande-lv.qualcomm.com>
On Fri, Jan 26, 2024 at 06:53:46AM -0800, Bjorn Andersson wrote:
> On Fri, Jan 26, 2024 at 03:31:02PM +0100, Johan Hovold wrote:
> > On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> > > In short it looks like the delays make the difference and, even a short
> > > delay, can fix the problem.
> >
> > Right, but since the suppliers are left enabled by the bootloader (and
> > never disabled by the kernel), that only begs the question of why this
> > makes a difference.
>
> You're right, the supply is kept on by other things, so this isn't the
> problem.
>
> > Without the delay, the other HID devices are probing (successfully)
> > slightly before, but essentially in parallel with the touchscreen while
> > using the same resources. Is that causing trouble somehow?
>
> The difference to those other HID devices is GPIO 99 - the reset pin,
> which is configured pull down input from boot - i.e. the chip is held in
> reset.
>
> When the HID device is being probed, pinctrl applies &ts0_default starts
> driving it high, bringing the device out of reset. But insufficient time
> is given for the chip to come up so the I2C read fails.
Ah, that's it.
You should drop that 'output-high' from the pin config as part of this
patch to avoid toggling the reset line twice at boot.
Looks like we have the same problem on the CRD as well. There the
touchscreen still works, possibly because it has been enabled by the
boot firmware or simply because that touchscreen can handle a shorter
delay.
Where exactly did you find those delay values in the ACPI tables? I
couldn't seem to find anything in the decompiled DSDT.
> If you later try to probe again, 200ms has elapsed since the reset was
> deasserted (driven high).
Right.
Johan
^ permalink raw reply
* Re: [PATCH regression fix 1/2] Input: atkbd - Skip ATKBD_CMD_SETLEDS when skipping ATKBD_CMD_GETID
From: Hans de Goede @ 2024-01-26 16:25 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov
Cc: Paul Menzel, stable, regressions, linux-input
In-Reply-To: <20240126160724.13278-2-hdegoede@redhat.com>
Hi,
On 1/26/24 17:07, Hans de Goede wrote:
> After commit 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in
> translated mode") the keyboard on Dell XPS 13 9350 / 9360 / 9370 models
> has stopped working after a suspend/resume.
>
> The problem appears to be that atkbd_probe() fails when called
> from atkbd_reconnect() on resume, which on systems where
> ATKBD_CMD_GETID is skipped can only happen by ATKBD_CMD_SETLEDS
> failing. ATKBD_CMD_SETLEDS failing because ATKBD_CMD_GETID was
> skipped is weird, but apparently that is what is happening.
Thinking more about it, what is likely happening here is that
ATKBD_CMD_SETLEDS is being send from atkbd_probe() where as
before atkbd_probe() would call ATKBD_CMD_GETID() and if that
succeeded (which it likely did) atkbd_probe() would continue with
calling atkbd_deactivate() and then exit, never calling
ATKBD_CMD_SETLEDS (at least not from atkbd_probe()).
So the problem seems to be that the embedded controller
does not like receiving ending ATKBD_CMD_SETLEDS as
the first command after resume and that being the first
command after resume is new behavior introduced by
936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
After applying both patches from this set (which is what
Paul tested), the ATKBD_CMD_GETID will still be skipped
but instead of replacing it with a ATKBD_CMD_SETLEDS
atkbd_probe() now continues with calling atkbd_deactivate()
and then exits as before the recent changes.
So after applying both patches here the behavior change
compared to before 936e4d49ecbc is limited to just
skipping ATKBD_CMD_GETID rather then effectively
replacing it with ATKBD_CMD_SETLEDS.
Regards,
Hans
>
> Fix this by also skipping ATKBD_CMD_SETLEDS when skipping
> ATKBD_CMD_GETID.
>
> Fixes: 936e4d49ecbc ("Input: atkbd - skip ATKBD_CMD_GETID in translated mode")
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Closes: https://lore.kernel.org/linux-input/0aa4a61f-c939-46fe-a572-08022e8931c7@molgen.mpg.de/
> Closes: https://bbs.archlinux.org/viewtopic.php?pid=2146300
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218424
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2260517
> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/input/keyboard/atkbd.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 13ef6284223d..c229bd6b3f7f 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -811,7 +811,6 @@ static int atkbd_probe(struct atkbd *atkbd)
> {
> struct ps2dev *ps2dev = &atkbd->ps2dev;
> unsigned char param[2];
> - bool skip_getid;
>
> /*
> * Some systems, where the bit-twiddling when testing the io-lines of the
> @@ -825,6 +824,11 @@ static int atkbd_probe(struct atkbd *atkbd)
> "keyboard reset failed on %s\n",
> ps2dev->serio->phys);
>
> + if (atkbd_skip_getid(atkbd)) {
> + atkbd->id = 0xab83;
> + return 0;
> + }
> +
> /*
> * Then we check the keyboard ID. We should get 0xab83 under normal conditions.
> * Some keyboards report different values, but the first byte is always 0xab or
> @@ -833,18 +837,17 @@ static int atkbd_probe(struct atkbd *atkbd)
> */
>
> param[0] = param[1] = 0xa5; /* initialize with invalid values */
> - skip_getid = atkbd_skip_getid(atkbd);
> - if (skip_getid || ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
> + if (ps2_command(ps2dev, param, ATKBD_CMD_GETID)) {
>
> /*
> - * If the get ID command was skipped or failed, we check if we can at least set
> + * If the get ID command failed, we check if we can at least set
> * the LEDs on the keyboard. This should work on every keyboard out there.
> * It also turns the LEDs off, which we want anyway.
> */
> param[0] = 0;
> if (ps2_command(ps2dev, param, ATKBD_CMD_SETLEDS))
> return -1;
> - atkbd->id = skip_getid ? 0xab83 : 0xabba;
> + atkbd->id = 0xabba;
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Bjorn Andersson @ 2024-01-26 16:36 UTC (permalink / raw)
To: Johan Hovold
Cc: Daniel Thompson, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Benjamin Tissoires,
Jiri Kosina, Bjorn Andersson, Konrad Dybcio, Johan Hovold,
linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <ZbPcgqr9gBByqV7Q@hovoldconsulting.com>
On Fri, Jan 26, 2024 at 05:23:30PM +0100, Johan Hovold wrote:
> On Fri, Jan 26, 2024 at 06:53:46AM -0800, Bjorn Andersson wrote:
> > On Fri, Jan 26, 2024 at 03:31:02PM +0100, Johan Hovold wrote:
> > > On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
>
> > > > In short it looks like the delays make the difference and, even a short
> > > > delay, can fix the problem.
> > >
> > > Right, but since the suppliers are left enabled by the bootloader (and
> > > never disabled by the kernel), that only begs the question of why this
> > > makes a difference.
> >
> > You're right, the supply is kept on by other things, so this isn't the
> > problem.
> >
> > > Without the delay, the other HID devices are probing (successfully)
> > > slightly before, but essentially in parallel with the touchscreen while
> > > using the same resources. Is that causing trouble somehow?
> >
> > The difference to those other HID devices is GPIO 99 - the reset pin,
> > which is configured pull down input from boot - i.e. the chip is held in
> > reset.
> >
> > When the HID device is being probed, pinctrl applies &ts0_default starts
> > driving it high, bringing the device out of reset. But insufficient time
> > is given for the chip to come up so the I2C read fails.
>
> Ah, that's it.
>
> You should drop that 'output-high' from the pin config as part of this
> patch to avoid toggling the reset line twice at boot.
>
Sounds reasonable, let's fix that while we're at it...
> Looks like we have the same problem on the CRD as well. There the
> touchscreen still works, possibly because it has been enabled by the
> boot firmware or simply because that touchscreen can handle a shorter
> delay.
>
I only poke the CRD remotely, forgot that it had touchscreen. Let's
bake a patch for that as well...
> Where exactly did you find those delay values in the ACPI tables? I
> couldn't seem to find anything in the decompiled DSDT.
>
The PEP sequence for the touchscreen device.
Regards,
Bjorn
> > If you later try to probe again, 200ms has elapsed since the reset was
> > deasserted (driven high).
>
> Right.
>
> Johan
^ permalink raw reply
* [PATCH v2 0/2] arm64: dts: qcom: sc8280xp-x13s: Enable touchscreen
From: Bjorn Andersson @ 2024-01-26 16:41 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson,
Daniel Thompson
This documents and defines the necessary properties for the I2C
HID-based touchscreen found in some SKUs of the Lenovo Thinkpad X13s to
work.
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Changes in v2:
- Dropped output-high from &ts0_default, to avoid bouncing the reset
line unnecessarily
- Link to v1: https://lore.kernel.org/r/20240125-x13s-touchscreen-v1-0-ab8c882def9c@quicinc.com
---
Bjorn Andersson (2):
dt-bindings: HID: i2c-hid: Document reset-related properties
arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 7 +++++--
2 files changed, 11 insertions(+), 2 deletions(-)
---
base-commit: 8bf1262c53f50fa91fe15d01e5ef5629db55313c
change-id: 20240125-x13s-touchscreen-48012ff3c24e
Best regards,
--
Bjorn Andersson <quic_bjorande@quicinc.com>
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: HID: i2c-hid: Document reset-related properties
From: Bjorn Andersson @ 2024-01-26 16:41 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson
In-Reply-To: <20240126-x13s-touchscreen-v2-0-5374ccc9e10d@quicinc.com>
Some I2C HID devices has a reset pin and requires that some specified
time elapses after this reset pin is deasserted, before communication
with the device is attempted.
The Linux implementation is looking for these in the "reset-gpios" and
"post-reset-deassert-delay-ms" properties already, so use these property
names.
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
index 138caad96a29..f07ff4cb3d26 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
@@ -50,6 +50,12 @@ properties:
description: Time required by the device after enabling its regulators
or powering it on, before it is ready for communication.
+ post-reset-deassert-delay-ms:
+ description: Time required by the device after reset has been deasserted,
+ before it is ready for communication.
+
+ reset-gpios: true
+
touchscreen-inverted-x: true
touchscreen-inverted-y: true
--
2.25.1
^ permalink raw reply related
* [PATCH v2 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Bjorn Andersson @ 2024-01-26 16:41 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson,
Daniel Thompson
In-Reply-To: <20240126-x13s-touchscreen-v2-0-5374ccc9e10d@quicinc.com>
The failing read-test in __i2c_hid_core_probe() determines that there's
nothing connected at the documented address of the touchscreen.
Introduce the 5ms after-power and 200ms after-reset delays found in the
ACPI tables. Also wire up the reset-gpio, for good measure.
Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index def3976bd5bb..33731b95ad51 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -620,7 +620,6 @@ &i2c4 {
status = "okay";
- /* FIXME: verify */
touchscreen@10 {
compatible = "hid-over-i2c";
reg = <0x10>;
@@ -630,6 +629,11 @@ touchscreen@10 {
vdd-supply = <&vreg_misc_3p3>;
vddl-supply = <&vreg_s10b>;
+ reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+ post-power-on-delay-ms = <5>;
+ post-reset-deassert-delay-ms = <200>;
+
pinctrl-names = "default";
pinctrl-0 = <&ts0_default>;
};
@@ -1450,7 +1454,6 @@ int-n-pins {
reset-n-pins {
pins = "gpio99";
function = "gpio";
- output-high;
drive-strength = <16>;
};
};
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] dt-bindings: HID: i2c-hid: Document reset-related properties
From: Johan Hovold @ 2024-01-26 16:52 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold, linux-arm-msm, linux-input, devicetree,
linux-kernel, Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <20240126-x13s-touchscreen-v2-1-5374ccc9e10d@quicinc.com>
On Fri, Jan 26, 2024 at 08:41:38AM -0800, Bjorn Andersson wrote:
> Some I2C HID devices has a reset pin and requires that some specified
> time elapses after this reset pin is deasserted, before communication
> with the device is attempted.
>
> The Linux implementation is looking for these in the "reset-gpios" and
> "post-reset-deassert-delay-ms" properties already, so use these property
> names.
>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
> Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> index 138caad96a29..f07ff4cb3d26 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
> @@ -50,6 +50,12 @@ properties:
> description: Time required by the device after enabling its regulators
> or powering it on, before it is ready for communication.
>
> + post-reset-deassert-delay-ms:
> + description: Time required by the device after reset has been deasserted,
> + before it is ready for communication.
You forgot to remove the now obsolete comment from the HID driver that I
mentioned.
> +
> + reset-gpios: true
> +
> touchscreen-inverted-x: true
>
> touchscreen-inverted-y: true
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Johan Hovold @ 2024-01-26 16:56 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold, linux-arm-msm, linux-input, devicetree,
linux-kernel, Konrad Dybcio, Krzysztof Kozlowski, Daniel Thompson
In-Reply-To: <20240126-x13s-touchscreen-v2-2-5374ccc9e10d@quicinc.com>
On Fri, Jan 26, 2024 at 08:41:39AM -0800, Bjorn Andersson wrote:
> The failing read-test in __i2c_hid_core_probe() determines that there's
> nothing connected at the documented address of the touchscreen.
>
> Introduce the 5ms after-power and 200ms after-reset delays found in the
> ACPI tables. Also wire up the reset-gpio, for good measure.
Please amend the commit message so that it reflects the discussion we
just had (e.g. wiring up the reset-gpio is not just for good measure).
Johan
^ permalink raw reply
* [PATCH] HID: i2c-hid-of: fix NULL-deref on failed power up
From: Johan Hovold @ 2024-01-26 17:09 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Johan Hovold, stable, Douglas Anderson
A while back the I2C HID implementation was split in an ACPI and OF
part, but the new OF driver never initialises the client pointer which
is dereferenced on power-up failures.
Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
Cc: stable@vger.kernel.org # 5.12
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/hid/i2c-hid/i2c-hid-of.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index c4e1fa0273c8..8be4d576da77 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -87,6 +87,7 @@ static int i2c_hid_of_probe(struct i2c_client *client)
if (!ihid_of)
return -ENOMEM;
+ ihid_of->client = client;
ihid_of->ops.power_up = i2c_hid_of_power_up;
ihid_of->ops.power_down = i2c_hid_of_power_down;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] HID: i2c-hid-of: fix NULL-deref on failed power up
From: Doug Anderson @ 2024-01-26 17:47 UTC (permalink / raw)
To: Johan Hovold
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
stable
In-Reply-To: <20240126170901.893-1-johan+linaro@kernel.org>
Hi,
On Fri, Jan 26, 2024 at 9:10 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> A while back the I2C HID implementation was split in an ACPI and OF
> part, but the new OF driver never initialises the client pointer which
> is dereferenced on power-up failures.
>
> Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
> Cc: stable@vger.kernel.org # 5.12
> Cc: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> drivers/hid/i2c-hid/i2c-hid-of.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> index c4e1fa0273c8..8be4d576da77 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -87,6 +87,7 @@ static int i2c_hid_of_probe(struct i2c_client *client)
> if (!ihid_of)
> return -ENOMEM;
>
> + ihid_of->client = client;
Good catch and thanks for the fix. FWIW, I'd be OK w/
Reviewed-by: Douglas Anderson <dianders@chromium.org>
That being said, I'd be even happier if you simply removed the
"client" from the structure and removed the error printout.
regulator_bulk_enable() already prints error messages when a failure
happens and thus the error printout is redundant and wastes space.
-Doug
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid-of: fix NULL-deref on failed power up
From: Jiri Kosina @ 2024-01-26 17:53 UTC (permalink / raw)
To: Doug Anderson
Cc: Johan Hovold, Benjamin Tissoires, linux-input, linux-kernel,
stable
In-Reply-To: <CAD=FV=UzGcneoL1d-DDXVugAeq2+YLCKrq8-5B7TfVAAKgF=SQ@mail.gmail.com>
On Fri, 26 Jan 2024, Doug Anderson wrote:
> > A while back the I2C HID implementation was split in an ACPI and OF
> > part, but the new OF driver never initialises the client pointer which
> > is dereferenced on power-up failures.
> >
> > Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
> > Cc: stable@vger.kernel.org # 5.12
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> > drivers/hid/i2c-hid/i2c-hid-of.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> > index c4e1fa0273c8..8be4d576da77 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> > @@ -87,6 +87,7 @@ static int i2c_hid_of_probe(struct i2c_client *client)
> > if (!ihid_of)
> > return -ENOMEM;
> >
> > + ihid_of->client = client;
>
> Good catch and thanks for the fix. FWIW, I'd be OK w/
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
I've now queued this as a fix for 6.8 ....
> That being said, I'd be even happier if you simply removed the "client"
> from the structure and removed the error printout.
> regulator_bulk_enable() already prints error messages when a failure
> happens and thus the error printout is redundant and wastes space.
... and this can be done for 6.9.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] drivers/platform/x86/touchscreen_dmi.c: Add touch config
From: Hans de Goede @ 2024-01-26 19:25 UTC (permalink / raw)
To: Phoenix Chen
Cc: ilpo.jarvinen, linux-input, platform-driver-x86, linux-kernel
In-Reply-To: <20240126095308.5042-1-asbeltogf@gmail.com>
Hi,
On 1/26/24 10:53, Phoenix Chen wrote:
> Added touch screen info for TECLAST X16 Plus tablet.
>
> Signed-off-by: Phoenix Chen <asbeltogf@gmail.com>
Thank you for your patch/series, I've applied this patch
(series) to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
And thank you for also adding the embedded_fw data so that is
will work out of the box for end users.
I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.
Regards,
Hans
> ---
> drivers/platform/x86/touchscreen_dmi.c | 35 ++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
> index 0c6733772698..7aee5e9ff2b8 100644
> --- a/drivers/platform/x86/touchscreen_dmi.c
> +++ b/drivers/platform/x86/touchscreen_dmi.c
> @@ -944,6 +944,32 @@ static const struct ts_dmi_data teclast_tbook11_data = {
> .properties = teclast_tbook11_props,
> };
>
> +static const struct property_entry teclast_x16_plus_props[] = {
> + PROPERTY_ENTRY_U32("touchscreen-min-x", 8),
> + PROPERTY_ENTRY_U32("touchscreen-min-y", 14),
> + PROPERTY_ENTRY_U32("touchscreen-size-x", 1916),
> + PROPERTY_ENTRY_U32("touchscreen-size-y", 1264),
> + PROPERTY_ENTRY_BOOL("touchscreen-inverted-y"),
> + PROPERTY_ENTRY_STRING("firmware-name", "gsl3692-teclast-x16-plus.fw"),
> + PROPERTY_ENTRY_U32("silead,max-fingers", 10),
> + PROPERTY_ENTRY_BOOL("silead,home-button"),
> + { }
> +};
> +
> +static const struct ts_dmi_data teclast_x16_plus_data = {
> + .embedded_fw = {
> + .name = "silead/gsl3692-teclast-x16-plus.fw",
> + .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
> + .length = 43560,
> + .sha256 = { 0x9d, 0xb0, 0x3d, 0xf1, 0x00, 0x3c, 0xb5, 0x25,
> + 0x62, 0x8a, 0xa0, 0x93, 0x4b, 0xe0, 0x4e, 0x75,
> + 0xd1, 0x27, 0xb1, 0x65, 0x3c, 0xba, 0xa5, 0x0f,
> + 0xcd, 0xb4, 0xbe, 0x00, 0xbb, 0xf6, 0x43, 0x29 },
> + },
> + .acpi_name = "MSSL1680:00",
> + .properties = teclast_x16_plus_props,
> +};
> +
> static const struct property_entry teclast_x3_plus_props[] = {
> PROPERTY_ENTRY_U32("touchscreen-size-x", 1980),
> PROPERTY_ENTRY_U32("touchscreen-size-y", 1500),
> @@ -1612,6 +1638,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
> DMI_MATCH(DMI_PRODUCT_SKU, "E5A6_A1"),
> },
> },
> + {
> + /* Teclast X16 Plus */
> + .driver_data = (void *)&teclast_x16_plus_data,
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "TECLAST"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Default string"),
> + DMI_MATCH(DMI_PRODUCT_SKU, "D3A5_A1"),
> + },
> + },
> {
> /* Teclast X3 Plus */
> .driver_data = (void *)&teclast_x3_plus_data,
^ permalink raw reply
* Re: PS/2 keyboard of laptop Dell XPS 13 9360 goes missing after S3
From: Linux regression tracking #update (Thorsten Leemhuis) @ 2024-01-27 8:12 UTC (permalink / raw)
To: LKML; +Cc: linux-input, linux-pm, regressions
In-Reply-To: <0aa4a61f-c939-46fe-a572-08022e8931c7@molgen.mpg.de>
On 18.01.24 13:57, Paul Menzel wrote:
> #regzbot introduced v6.6.11..v6.7
#regzbot introduced 936e4d49ecbc
#regzbot ignore-activity
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
That page also explains what to do if mails like this annoy you.
^ permalink raw reply
* Re: [PATCH v1 3/7] HID: playstation: DS4: Don't fail on FW/HW version request
From: Max Staudt @ 2024-01-27 8:56 UTC (permalink / raw)
To: Roderick Colenbrander
Cc: Roderick Colenbrander, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel
In-Reply-To: <CAEc3jaDD76e+Lkwc8WSac30-kXBYKP6_R2+kM4+Z_RVOinD9Kg@mail.gmail.com>
On 1/25/24 09:43, Roderick Colenbrander wrote:
> On Mon, Jan 15, 2024 at 6:51 AM Max Staudt <max@enpas.org> wrote:
>>
>> ret = dualshock4_get_firmware_info(ds4);
>> if (ret) {
>> hid_err(hdev, "Failed to get firmware info from DualShock4\n");
>> - return ERR_PTR(ret);
>> + hid_err(hdev, "HW/FW version data in sysfs will be invalid.\n");
>> }
>
> This looks good. Perhaps could have been a hid_warn then, but err is
> probably fine.
I didn't think about it and kept the hid_err that was already there :)
I agree, and maybe we can take this further: If this code block is not a fatal error anymore, maybe it makes sense to change *both* lines to hid_warn?
If you prefer this, then I'll update the other patches accordingly.
Max
^ permalink raw reply
* Re: [PATCH v1 5/7] HID: playstation: DS4: Parse minimal report 0x01
From: Max Staudt @ 2024-01-27 9:00 UTC (permalink / raw)
To: Roderick Colenbrander
Cc: Roderick Colenbrander, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel
In-Reply-To: <CAEc3jaCYWpXX=YgeNzXJd0Pg4yn4WKqL=GBSAzaWNzo=3RSw1A@mail.gmail.com>
On 1/25/24 09:54, Roderick Colenbrander wrote:
> I would say let's turn this into a 'return 0'. The goto is not useful
> since there is no need for any common cleanup or some other common
> logic later.
Oops, that one slipped through the cracks.
Thank you, will do!
Max
^ permalink raw reply
* Re: [PATCH v1 2/7] HID: playstation: DS4: Don't fail on MAC address request
From: Max Staudt @ 2024-01-27 8:51 UTC (permalink / raw)
To: Roderick Colenbrander
Cc: Roderick Colenbrander, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel
In-Reply-To: <CAEc3jaD9qXp=F6Xfg8bdeC1Hv5pHiKA16SBd=-ac74ibE1ELyw@mail.gmail.com>
On 1/25/24 09:39, Roderick Colenbrander wrote:
> On Mon, Jan 15, 2024 at 6:58 AM Max Staudt <max@enpas.org> wrote:
>>
>> Some third-party controllers can't report their MAC address.
>>
> For what type of devices is this not working? This one example of this
> request which is very foundational for a controller working on even
> the game console. Are this perhaps USB-only devices? If the case maybe
> some kind of error is only needed for USB connections.
IIRC I've only seen this quirk with the oddball VID/PID 7545:0104 that my patch 7/7 adds. It is indeed a USB-only controller. I have not tried this device (or any, really) with an actual console, my focus is just on Linux. Admittedly, I'd also like to know what happens on a real PS4 :)
My intention was to keep the error message and the solution as universal as possible, in case a controller comes along that has both this quirk *and* Bluetooth. It's a bit of an ugly workaround though - if you (or anyone else) have an idea for a nicer solution, I'd be really glad.
Max
^ permalink raw reply
* Re: [PATCH v1 7/7] HID: playstation: DS4: Add VID/PID for SZ-MYPOWER controllers
From: Max Staudt @ 2024-01-27 11:16 UTC (permalink / raw)
To: Roderick Colenbrander
Cc: Roderick Colenbrander, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel
In-Reply-To: <CAEc3jaBU3M0Zce2pdFvdBSG50a7Ky=GY4gLO3dkYdDrkYtiO0Q@mail.gmail.com>
On 1/25/24 10:03, Roderick Colenbrander wrote:
> I'm not familiar with this device, but if it indeed works. Then I'm
> okay with it.
Thanks!
I've just tried this patch on real hardware again, and there's a tradeoff here - it improves the situation for one 7545:0104 controller, and worsens it for another.
Up to you, and if you don't want to think about it, then let's shelve this patch :)
Details follow, if you're curious.
I have two controllers with VID/PID 7545:0104, and they're both very quirky multi-emulation devices. One is shaped like a PS4 controller, the other like a hybrid between a PS4 and a Switch controller. Since these controllers exhibit all of the USB related quirks in this series, I've kept them as reproducers. Other controllers that passed through my hands only had a subset of the quirks.
Up until now, both controllers worked with hid-sony as PS3 controllers. With this patch, the PS4 controller gains LED support and fine-grained control of the weak rumble motor. The "Switch (?) controller" on the other hand errors out, becomes 0079:181c, and loses the Home key and the accelerometer. This is a user facing change, and the question is how much we really care about these controllers.
More details, if you're still reading:
Both are "multi-purpose" controllers, appearing as PS4/PS3/Switch/other controllers in sequence. They advertise themselves as one USB device, and if there is no driver sending whatever init sequence they expect, they disconnect and try emulating a different controller.
The PS4 controller has rumble and an RGB LED, and this patch series improves its functionality. It cannot emulate a Switch controller.
The Switch (?) controller has no rumble and four multicolour player LEDs, but it adds Switch compatibility including accelerometer and gyro.
For the PS4 mode, which is the first that they try, and which would unify most functions, they use 7545:0104 instead of cloning a DS4 VID/PID. So I took a guess and found that it works fine with hid-playstation if I add the VID/PID and the init quirks in patches 2/3/4. Well, to be precise, I've only made the DS4 shaped one work in PS4 mode, the Switch controller isn't happy and errors out, see below.
On the PS4 controller, this makes the RGB LED work, rumble works, but the gyro and touchpad don't send HID updates. The touchpad can click though, so maybe the controller I have has a hardware defect.
The Switch (?) controller is where things get weird. It disconnects, even though it is initialised by hid-playstation, and transitions into a generic controller with VID/PID 0079:181c. This mode is *not* on the list of emulations it usually tries. It's as if the "unfinished" PS4 initialisation transitions it into a hidden fifth emulation mode. In this mode, the home key does not send any HID event, and there are no accelerometer updates that hid-sony would receive in PS3 mode.
So, with this patch, the PS4 controller works better on Linux, while the Switch controller works worse. Both were seen as PS3 controllers up until now. I see no way to discern them at driver probe time.
Any preference on what to do...?
Max
^ permalink raw reply
* Re: [PATCH v1 0/7] HID: playstation: DS4: LED bugfix, third-party gamepad support
From: Max Staudt @ 2024-01-27 11:52 UTC (permalink / raw)
To: Roderick Colenbrander, Jiri Kosina
Cc: Roderick Colenbrander, Benjamin Tissoires, linux-input,
linux-kernel
In-Reply-To: <CAEc3jaATA-Z86jh=njR6yctguQBRp1BQEFcBQhYT0rYKcA4f2w@mail.gmail.com>
On 1/25/24 07:24, Roderick Colenbrander wrote:
> Sorry for the late reply. I had glanced over them, but didn't have an
> opportunity for a detailed review yet.
No worries, thank you for giving the patches a chance.
> My general fear is a balance between supporting clone devices vs
> reliability. This driver is heavily used in devices (phones, tablets,
> TVs, cars). There have been bug reports in the past and just getting
> the fixes downstream takes a lot of time (e.g. Android devices).
I understand this very well, which is why I hope I've kept the patches small enough to be alright to follow. If you have a bad feeling about something in particular, please let me know, and maybe we can find a better solution or alleviate concerns.
On my end, I am working (currently as a hobby) on an appliance that aims to support as many controllers as possible, hence I know the choices you face in order to keep user expectations in check, and user experience consistent. See my comment on patch 7/7 - what if someone uses a third-party controller that happens to work by chance, and a kernel update breaks stuff?
So I am with you on focusing on the original devices, which have predictable behaviour and quality. That said, hopefully some or all of these patches are trivial enough to be included upstream. I feel that it's beautiful to plug random stuff into a Linux box and to find that it just works :)
> One of the key things I really would like to see enhanced are the unit
> tests (hid-tools / kernel side now). To really make sure we emulate
> behavior of these other devices well. The tricky part is that they
> don't always support all the HID requests of the real device (which is
> weird as the game console does use those HID reports and others and I
> don't know how it would have worked there).
I've been wondering the same, but without a PS4 of my own, I didn't try such controllers on a real console. That would be interesting indeed!
> That's in general the key feedback about the tests. A question for
> Max: do you have access to all the devices being added?
I don't have all devices I've ever tested, but I do have at least one reproducer for each patch in this series.
Thanks,
Max
^ permalink raw reply
* [PATCH] Input: bma150: remove I2C_CLASS_HWMON from drivers w/o detect and address_list
From: Heiner Kallweit @ 2024-01-27 15:18 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER, linux-i2c@vger.kernel.org
Class-based I2C probing requires detect() and address_list to be
set in the I2C client driver, see checks in i2c_detect().
It's misleading to declare I2C_CLASS_HWMON support if this
precondition isn't met.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/input/misc/bma150.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
index 0fb4cc628..22feaa8c1 100644
--- a/drivers/input/misc/bma150.c
+++ b/drivers/input/misc/bma150.c
@@ -549,7 +549,6 @@ static struct i2c_driver bma150_driver = {
.name = BMA150_DRIVER,
.pm = &bma150_pm,
},
- .class = I2C_CLASS_HWMON,
.id_table = bma150_id,
.probe = bma150_probe,
.remove = bma150_remove,
--
2.43.0
^ permalink raw reply related
* Re: [RFC PATCH 2/5] mfd: add 88pm88x driver
From: Karel Balej @ 2024-01-28 9:38 UTC (permalink / raw)
To: Lee Jones
Cc: Karel Balej, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20240125122634.GE74950@google.com>
Lee,
thank you for your feedback.
On Thu Jan 25, 2024 at 1:26 PM CET, Lee Jones wrote:
[...]
> > +#define PM88X_REG_INT_STATUS1 0x05
> > +
> > +#define PM88X_REG_INT_ENA_1 0x0a
> > +#define PM88X_INT_ENA1_ONKEY BIT(0)
> > +
> > +enum pm88x_irq_number {
> > + PM88X_IRQ_ONKEY,
> > +
> > + PM88X_MAX_IRQ
> > +};
>
> An enum for a single IRQ?
There will be a lot more IRQs but I have only added this one so far as
it is the only one used by this series -- is that OK?
> > +static struct reg_sequence pm886_presets[] = {
> > + /* disable watchdog */
> > + REG_SEQ0(PM88X_REG_WDOG, 0x01),
>
> Easier to read if you place spaces between them.
>
> > + /* GPIO1: DVC, GPIO0: input */
> > + REG_SEQ0(PM88X_REG_GPIO_CTRL1, 0x40),
>
> Shouldn't you set these up using Pintrl?
You mean to add a new MFD cell for the pins and write the respective
driver? The downstream implementation has no such thing so I'm not sure
if I would be able to do that from scratch.
> > + /* GPIO2: input */
> > + REG_SEQ0(PM88X_REG_GPIO_CTRL2, 0x00),
> > + /* DVC2, DVC1 */
>
> Please unify all of the comments.
>
> They all use a different structure.
>
> > + REG_SEQ0(PM88X_REG_GPIO_CTRL3, 0x44),
> > + /* GPIO5V_1:input, GPIO5V_2: input */
> > + REG_SEQ0(PM88X_REG_GPIO_CTRL4, 0x00),
> > + /* output 32 kHz from XO */
> > + REG_SEQ0(PM88X_REG_AON_CTRL2, 0x2a),
> > + /* OSC_FREERUN = 1, to lock FLL */
> > + REG_SEQ0(PM88X_REG_BK_OSC_CTRL1, 0x0f),
> > + /* XO_LJ = 1, enable low jitter for 32 kHz */
> > + REG_SEQ0(PM88X_REG_LOWPOWER2, 0x20),
> > + /* OV_VSYS and UV_VSYS1 comparators on VSYS disabled, VSYS_OVER_TH : 5.6V */
> > + REG_SEQ0(PM88X_REG_LOWPOWER4, 0xc8),
> > + /* set the duty cycle of charger DC/DC to max */
> > + REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0),
>
> These all looks like they should be handled in their respective drivers?
>
> "patch"ing these in seems like a hack.
To be honest, I don't really know why these are required and what effect
they have -- the comments above taken from the downstream version are
the only thing I have to go by. I might try removing them to see if
there is any noticable change and whether they could be added only later
with the respective drivers.
>
> > +};
>
> Why this instead of
What are you refering to here please?
> > +static struct resource onkey_resources[] = {
> > + DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"),
> > +};
> > +
> > +static struct mfd_cell pm88x_devs[] = {
> > + {
> > + .name = "88pm88x-onkey",
> > + .num_resources = ARRAY_SIZE(onkey_resources),
> > + .resources = onkey_resources,
> > + .id = -1,
> > + },
> > +};
>
> It's not an MFD if it only supports a single device.
As I have noted above with respect to the IRQ enum and also in the
commit message, I have so far only added the parts which there is
already use for. I intend to add the other parts along with the
respective subdevice drivers, please see my regulator series [1] for an
example.
I thought this approach would make for shorter and simpler patches and
also would allow me to make more informed decisions as I familiarize
myself with the downstream subdevice drivers more closely one by one.
> > + i2c_set_clientdata(client, chip);
> > +
> > + device_init_wakeup(&client->dev, 1);
> > +
> > + chip->regmaps[PM88X_REGMAP_BASE] = devm_regmap_init_i2c(client, &pm88x_i2c_regmap);
> > + if (IS_ERR(chip->regmaps[PM88X_REGMAP_BASE])) {
>
> Just define different regmaps if you really need them.
You mean not to use an array of regmaps but add new struct members
instead? One for each regmap?
> > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h
> > new file mode 100644
> > index 000000000000..a34c57447827
> > --- /dev/null
> > +++ b/include/linux/mfd/88pm88x.h
[...]
> > +#define PM88X_REG_ID 0x00
> > +
> > +#define PM88X_REG_STATUS1 0x01
> > +#define PM88X_ONKEY_STS1 BIT(0)
> > +
> > +#define PM88X_REG_MISC_CONFIG1 0x14
> > +#define PM88X_SW_PDOWN BIT(5)
> > +
> > +#define PM88X_REG_MISC_CONFIG2 0x15
> > +#define PM88X_INT_INV BIT(0)
> > +#define PM88X_INT_CLEAR BIT(1)
> > +#define PM88X_INT_RC 0x00
> > +#define PM88X_INT_WC BIT(1)
> > +#define PM88X_INT_MASK_MODE BIT(2)
> > +
> > +#define PM88X_REG_WDOG 0x1d
> > +
> > +#define PM88X_REG_LOWPOWER2 0x21
> > +#define PM88X_REG_LOWPOWER4 0x23
> > +
> > +#define PM88X_REG_GPIO_CTRL1 0x30
>
> These don't really need to be spaced out, do they?
I have spaced them out already as I expect to add some related
definitions to each of these in the future and thought it would then
perhaps be more easily readable like this.
[1] https://lore.kernel.org/all/20231228100208.2932-1-karelb@gimli.ms.mff.cuni.cz/
Kind regards,
K. B.
^ permalink raw reply
* [PATCH] HID: logitech-dj: allow mice to use all types of reports
From: Yaraslau Furman @ 2024-01-28 21:49 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: yaro330, Filipe Laíns, Jiri Kosina,
open list:HID LOGITECH DRIVERS, open list
You can bind whatever action you want to the mouse's reprogrammable
buttons using Windows application. Allow Linux to receive those keycodes.
Signed-off-by: Yaraslau Furman <yaro330@gmail.com>
---
drivers/hid/hid-logitech-dj.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index e6a8b6d8eab7..5730be1aa41c 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -948,11 +948,12 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
HIDPP_DEVICE_TYPE_MASK;
workitem->quad_id_msb = hidpp_report->params[HIDPP_PARAM_EQUAD_MSB];
workitem->quad_id_lsb = hidpp_report->params[HIDPP_PARAM_EQUAD_LSB];
+ workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA |
+ POWER_KEYS | MEDIA_CENTER |
+ HIDPP;
+
switch (workitem->device_type) {
case REPORT_TYPE_KEYBOARD:
- workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA |
- POWER_KEYS | MEDIA_CENTER |
- HIDPP;
id = (workitem->quad_id_msb << 8) | workitem->quad_id_lsb;
for (i = 0; i < ARRAY_SIZE(kbd_builtin_touchpad_ids); i++) {
if (id == kbd_builtin_touchpad_ids[i]) {
@@ -965,9 +966,6 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
}
break;
case REPORT_TYPE_MOUSE:
- workitem->reports_supported |= STD_MOUSE | HIDPP;
- if (djrcv_dev->type == recvr_type_mouse_only)
- workitem->reports_supported |= MULTIMEDIA;
break;
}
}
--
2.43.0
^ permalink raw reply related
* [PATCH bpf-next v4 0/3] Annotate kfuncs in .BTF_ids section
From: Daniel Xu @ 2024-01-29 1:24 UTC (permalink / raw)
To: linux-trace-kernel, coreteam, bpf, linux-input, cgroups, netdev,
linux-stm32, linux-kselftest, linux-doc, fsverity, linux-kernel,
linux-arm-kernel, netfilter-devel, alexei.starovoitov, olsajiri,
quentin, alan.maguire, memxor
=== Description ===
This is a bpf-treewide change that annotates all kfuncs as such inside
.BTF_ids. This annotation eventually allows us to automatically generate
kfunc prototypes from bpftool.
We store this metadata inside a yet-unused flags field inside struct
btf_id_set8 (thanks Kumar!). pahole will be taught where to look.
More details about the full chain of events are available in commit 3's
description.
The accompanying pahole and bpftool changes can be viewed
here on these "frozen" branches [0][1].
[0]: https://github.com/danobi/pahole/tree/kfunc_btf-v3-mailed
[1]: https://github.com/danobi/linux/tree/kfunc_bpftool-mailed
=== Changelog ===
Changes from v3:
* Rebase to bpf-next and add missing annotation on new kfunc
Changes from v2:
* Only WARN() for vmlinux kfuncs
Changes from v1:
* Move WARN_ON() up a call level
* Also return error when kfunc set is not properly tagged
* Use BTF_KFUNCS_START/END instead of flags
* Rename BTF_SET8_KFUNC to BTF_SET8_KFUNCS
Daniel Xu (3):
bpf: btf: Support flags for BTF_SET8 sets
bpf: btf: Add BTF_KFUNCS_START/END macro pair
bpf: treewide: Annotate BPF kfuncs in BTF
Documentation/bpf/kfuncs.rst | 8 +++----
drivers/hid/bpf/hid_bpf_dispatch.c | 8 +++----
fs/verity/measure.c | 4 ++--
include/linux/btf_ids.h | 21 +++++++++++++++----
kernel/bpf/btf.c | 8 +++++++
kernel/bpf/cpumask.c | 4 ++--
kernel/bpf/helpers.c | 8 +++----
kernel/bpf/map_iter.c | 4 ++--
kernel/cgroup/rstat.c | 4 ++--
kernel/trace/bpf_trace.c | 8 +++----
net/bpf/test_run.c | 8 +++----
net/core/filter.c | 20 +++++++++---------
net/core/xdp.c | 4 ++--
net/ipv4/bpf_tcp_ca.c | 4 ++--
net/ipv4/fou_bpf.c | 4 ++--
net/ipv4/tcp_bbr.c | 4 ++--
net/ipv4/tcp_cubic.c | 4 ++--
net/ipv4/tcp_dctcp.c | 4 ++--
net/netfilter/nf_conntrack_bpf.c | 4 ++--
net/netfilter/nf_nat_bpf.c | 4 ++--
net/xfrm/xfrm_interface_bpf.c | 4 ++--
net/xfrm/xfrm_state_bpf.c | 4 ++--
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 8 +++----
23 files changed, 87 insertions(+), 66 deletions(-)
--
2.42.1
^ 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