After system suspend the middle-button mode is being reset to
compatibility mode which simply breaks functionality for the devices
where native mode is configured during probe().
Fix this by setting native mode in reset_resume() for the appropriate
devices.
Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for compact keyboards")
Signed-off-by: Martin Kepplinger <[email protected]>
---
drivers/hid/hid-lenovo.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..d20562b9eca6 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -1344,6 +1344,28 @@ static int lenovo_input_configured(struct hid_device *hdev,
return 0;
}
+static int __maybe_unused lenovo_resume(struct hid_device *hdev)
+{
+ int ret;
+
+ switch (hdev->product) {
+ case USB_DEVICE_ID_LENOVO_CUSBKBD:
+ case USB_DEVICE_ID_LENOVO_CBTKBD:
+ case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
+ case USB_DEVICE_ID_LENOVO_TPIIBTKBD:
+ /* Switch middle button to native mode again */
+ ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
+ if (ret)
+ hid_warn(hdev, "Failed to switch middle button: %d\n",
+ ret);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+
+ return ret;
+}
static const struct hid_device_id lenovo_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
@@ -1380,6 +1402,9 @@ static struct hid_driver lenovo_driver = {
.raw_event = lenovo_raw_event,
.event = lenovo_event,
.report_fixup = lenovo_report_fixup,
+#ifdef CONFIG_PM
+ .reset_resume = lenovo_resume,
+#endif
};
module_hid_driver(lenovo_driver);
--
2.39.2
On 2023-09-21 10:21, Martin Kepplinger wrote:
> After system suspend the middle-button mode is being reset to
> compatibility mode which simply breaks functionality for the devices
> where native mode is configured during probe().
>
> Fix this by setting native mode in reset_resume() for the appropriate
> devices.
>
> Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for
> compact keyboards")
> Signed-off-by: Martin Kepplinger <[email protected]>
> ---
> drivers/hid/hid-lenovo.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 44763c0da444..d20562b9eca6 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -1344,6 +1344,28 @@ static int lenovo_input_configured(struct
> hid_device *hdev,
> return 0;
> }
>
> +static int __maybe_unused lenovo_resume(struct hid_device *hdev)
> +{
> + int ret;
> +
> + switch (hdev->product) {
> + case USB_DEVICE_ID_LENOVO_CUSBKBD:
> + case USB_DEVICE_ID_LENOVO_CBTKBD:
> + case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
> + case USB_DEVICE_ID_LENOVO_TPIIBTKBD:
> + /* Switch middle button to native mode again */
> + ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
This will only reset the middle button state and not the Function key
state, which I believe has similar problems. This was my attempt at
solving this:
https://github.com/lentinj/linux/commit/f1c4e2de780abf8526bcdc9496c463f1ff4fe53b
...which should ensure everything is in a consistent state with what the
kernel expects.
I never submitted this since sending commands was sporadically resulting
in timeouts, although I'm fairly sure it was unrelated to this patch,
and quite possibly a hardware problem with my keyboard. I'd be
interested to know how you get on.
Also, the above will send the command to both the USB keyboard & mouse
devices, only the mouse will respond. So worth prepending something
like:
if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
(hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
hdev->type != HID_TYPE_USBMOUSE) {
hid_dbg(hdev, "Ignoring keyboard half of device\n");
return 0;
}
...to avoid sending known-useless messages.
> + if (ret)
> + hid_warn(hdev, "Failed to switch middle button: %d\n",
> + ret);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> +
> + return ret;
> +}
>
> static const struct hid_device_id lenovo_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
> @@ -1380,6 +1402,9 @@ static struct hid_driver lenovo_driver = {
> .raw_event = lenovo_raw_event,
> .event = lenovo_event,
> .report_fixup = lenovo_report_fixup,
> +#ifdef CONFIG_PM
> + .reset_resume = lenovo_resume,
> +#endif
> };
> module_hid_driver(lenovo_driver);
From: Jamie Lentin <[email protected]>
The USB Compact Keyboard variant requires a reset_resume function to
restore keyboard configuration after a suspend in some situations. Move
configuration normally done on probe to lenovo_features_set_cptkbd(), then
recycle this for use on reset_resume.
Without, the keyboard and driver would end up in an inconsistent state,
breaking middle-button scrolling amongst other problems, and twiddling
sysfs values wouldn't help as the middle-button mode won't be set until
the driver is reloaded.
Tested on a USB and Bluetooth Thinkpad Compact Keyboard.
Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for compact keyboards")
Signed-off-by: Jamie Lentin <[email protected]>
Signed-off-by: Martin Kepplinger <[email protected]>
---
hi Jamie,
thanks for sharing your patch! This works equally well for me. Is the
2nd patch what you had in mind? If so, let's create a new squashed non-RFC
patch.
thanks again,
martin
drivers/hid/hid-lenovo.c | 50 +++++++++++++++++++++++++++-------------
1 file changed, 34 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..29aa6d372bad 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -521,6 +521,19 @@ static void lenovo_features_set_cptkbd(struct hid_device *hdev)
int ret;
struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
+ /*
+ * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
+ * regular keys
+ */
+ ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
+ if (ret)
+ hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n", ret);
+
+ /* Switch middle button to native mode */
+ ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
+ if (ret)
+ hid_warn(hdev, "Failed to switch middle button: %d\n", ret);
+
ret = lenovo_send_cmd_cptkbd(hdev, 0x05, cptkbd_data->fn_lock);
if (ret)
hid_err(hdev, "Fn-lock setting failed: %d\n", ret);
@@ -1126,22 +1139,6 @@ static int lenovo_probe_cptkbd(struct hid_device *hdev)
}
hid_set_drvdata(hdev, cptkbd_data);
- /*
- * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
- * regular keys (Compact only)
- */
- if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD ||
- hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD) {
- ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
- if (ret)
- hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n", ret);
- }
-
- /* Switch middle button to native mode */
- ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
- if (ret)
- hid_warn(hdev, "Failed to switch middle button: %d\n", ret);
-
/* Set keyboard settings to known state */
cptkbd_data->middlebutton_state = 0;
cptkbd_data->fn_lock = true;
@@ -1264,6 +1261,24 @@ static int lenovo_probe(struct hid_device *hdev,
return ret;
}
+#ifdef CONFIG_PM
+static int lenovo_reset_resume(struct hid_device *hdev)
+{
+ switch (hdev->product) {
+ case USB_DEVICE_ID_LENOVO_CUSBKBD:
+ if (hdev->type == HID_TYPE_USBMOUSE) {
+ lenovo_features_set_cptkbd(hdev);
+ }
+
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+#endif
+
static void lenovo_remove_tpkbd(struct hid_device *hdev)
{
struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
@@ -1380,6 +1395,9 @@ static struct hid_driver lenovo_driver = {
.raw_event = lenovo_raw_event,
.event = lenovo_event,
.report_fixup = lenovo_report_fixup,
+#ifdef CONFIG_PM
+ .reset_resume = lenovo_reset_resume,
+#endif
};
module_hid_driver(lenovo_driver);
--
2.39.2
These custom commands will be sent to both the USB keyboard & mouse
devices but only the mouse will respond. Avoid sending known-useless
messages by always prepending the filter before sending them.
Suggested-by: Jamie Lentin <[email protected]>
Signed-off-by: Martin Kepplinger <[email protected]>
---
drivers/hid/hid-lenovo.c | 27 +++++++++------------------
1 file changed, 9 insertions(+), 18 deletions(-)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 29aa6d372bad..922f3e5462f4 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -521,6 +521,14 @@ static void lenovo_features_set_cptkbd(struct hid_device *hdev)
int ret;
struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
+ /* All the custom action happens on the USBMOUSE device for USB */
+ if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
+ (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
+ hdev->type != HID_TYPE_USBMOUSE) {
+ hid_dbg(hdev, "Ignoring keyboard half of device\n");
+ return;
+ }
+
/*
* Tell the keyboard a driver understands it, and turn F7, F9, F11 into
* regular keys
@@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct hid_device *hdev)
int ret;
struct lenovo_drvdata *cptkbd_data;
- /* All the custom action happens on the USBMOUSE device for USB */
- if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
- (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
- hdev->type != HID_TYPE_USBMOUSE) {
- hid_dbg(hdev, "Ignoring keyboard half of device\n");
- return 0;
- }
-
cptkbd_data = devm_kzalloc(&hdev->dev,
sizeof(*cptkbd_data),
GFP_KERNEL);
@@ -1264,16 +1264,7 @@ static int lenovo_probe(struct hid_device *hdev,
#ifdef CONFIG_PM
static int lenovo_reset_resume(struct hid_device *hdev)
{
- switch (hdev->product) {
- case USB_DEVICE_ID_LENOVO_CUSBKBD:
- if (hdev->type == HID_TYPE_USBMOUSE) {
- lenovo_features_set_cptkbd(hdev);
- }
-
- break;
- default:
- break;
- }
+ lenovo_features_set_cptkbd(hdev);
return 0;
}
--
2.39.2
On 2023-09-25 11:23, Martin Kepplinger wrote:
> These custom commands will be sent to both the USB keyboard & mouse
> devices but only the mouse will respond. Avoid sending known-useless
> messages by always prepending the filter before sending them.
>
> Suggested-by: Jamie Lentin <[email protected]>
> Signed-off-by: Martin Kepplinger <[email protected]>
> ---
> drivers/hid/hid-lenovo.c | 27 +++++++++------------------
> 1 file changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 29aa6d372bad..922f3e5462f4 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -521,6 +521,14 @@ static void lenovo_features_set_cptkbd(struct
> hid_device *hdev)
> int ret;
> struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
>
> + /* All the custom action happens on the USBMOUSE device for USB */
> + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
> + hdev->type != HID_TYPE_USBMOUSE) {
> + hid_dbg(hdev, "Ignoring keyboard half of device\n");
> + return;
> + }
> +
> /*
> * Tell the keyboard a driver understands it, and turn F7, F9, F11
> into
> * regular keys
> @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct hid_device
> *hdev)
> int ret;
> struct lenovo_drvdata *cptkbd_data;
>
> - /* All the custom action happens on the USBMOUSE device for USB */
> - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
> - hdev->type != HID_TYPE_USBMOUSE) {
> - hid_dbg(hdev, "Ignoring keyboard half of device\n");
> - return 0;
> - }
> -
I like the idea of doing it once then forgetting about it, but removing
this will mean that the "keyboard half" will have it's own set of
non-functional sysfs parameters I think? Currently:-
# evtest
. . .
/dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
TrackPoint
/dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
TrackPoint
/dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
TrackPoint
# ls -1 /sys/class/input/event*/device/device/fn_lock
/sys/class/input/event10/device/device/fn_lock
/sys/class/input/event12/device/device/fn_lock
(note 11 is missing.)
I think the easiest (but ugly) thing to do is to copy-paste this lump of
code to the top of lenovo_reset_resume.
Cheers,
> cptkbd_data = devm_kzalloc(&hdev->dev,
> sizeof(*cptkbd_data),
> GFP_KERNEL);
> @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct hid_device *hdev,
> #ifdef CONFIG_PM
> static int lenovo_reset_resume(struct hid_device *hdev)
> {
> - switch (hdev->product) {
> - case USB_DEVICE_ID_LENOVO_CUSBKBD:
> - if (hdev->type == HID_TYPE_USBMOUSE) {
> - lenovo_features_set_cptkbd(hdev);
> - }
> -
> - break;
> - default:
> - break;
> - }
> + lenovo_features_set_cptkbd(hdev);
>
> return 0;
> }
Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
> On 2023-09-25 11:23, Martin Kepplinger wrote:
> > These custom commands will be sent to both the USB keyboard & mouse
> > devices but only the mouse will respond. Avoid sending known-
> > useless
> > messages by always prepending the filter before sending them.
> >
> > Suggested-by: Jamie Lentin <[email protected]>
> > Signed-off-by: Martin Kepplinger <[email protected]>
> > ---
> > drivers/hid/hid-lenovo.c | 27 +++++++++------------------
> > 1 file changed, 9 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> > index 29aa6d372bad..922f3e5462f4 100644
> > --- a/drivers/hid/hid-lenovo.c
> > +++ b/drivers/hid/hid-lenovo.c
> > @@ -521,6 +521,14 @@ static void lenovo_features_set_cptkbd(struct
> > hid_device *hdev)
> > int ret;
> > struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
> >
> > + /* All the custom action happens on the USBMOUSE device for
> > USB */
> > + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> > + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
> > + hdev->type != HID_TYPE_USBMOUSE) {
> > + hid_dbg(hdev, "Ignoring keyboard half of
> > device\n");
> > + return;
> > + }
> > +
> > /*
> > * Tell the keyboard a driver understands it, and turn F7,
> > F9, F11
> > into
> > * regular keys
> > @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct
> > hid_device
> > *hdev)
> > int ret;
> > struct lenovo_drvdata *cptkbd_data;
> >
> > - /* All the custom action happens on the USBMOUSE device for
> > USB */
> > - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> > - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
> > - hdev->type != HID_TYPE_USBMOUSE) {
> > - hid_dbg(hdev, "Ignoring keyboard half of
> > device\n");
> > - return 0;
> > - }
> > -
>
> I like the idea of doing it once then forgetting about it, but
> removing
> this will mean that the "keyboard half" will have it's own set of
> non-functional sysfs parameters I think? Currently:-
>
> # evtest
> . . .
> /dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
> TrackPoint
> /dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
> TrackPoint
> /dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
> TrackPoint
>
> # ls -1 /sys/class/input/event*/device/device/fn_lock
> /sys/class/input/event10/device/device/fn_lock
> /sys/class/input/event12/device/device/fn_lock
>
> (note 11 is missing.)
>
> I think the easiest (but ugly) thing to do is to copy-paste this lump
> of
> code to the top of lenovo_reset_resume.
> Cheers,
>
> > cptkbd_data = devm_kzalloc(&hdev->dev,
> > sizeof(*cptkbd_data),
> > GFP_KERNEL);
> > @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct hid_device
> > *hdev,
> > #ifdef CONFIG_PM
> > static int lenovo_reset_resume(struct hid_device *hdev)
> > {
> > - switch (hdev->product) {
> > - case USB_DEVICE_ID_LENOVO_CUSBKBD:
> > - if (hdev->type == HID_TYPE_USBMOUSE) {
> > - lenovo_features_set_cptkbd(hdev);
> > - }
> > -
> > - break;
> > - default:
> > - break;
> > - }
> > + lenovo_features_set_cptkbd(hdev);
ok. ignore my change (this whole patch) and look at your addition here,
don't you already make sure only the mouse-part gets the messages? you
just write switch()case instead of if(); what do you think is missing
here?
thanks,
martin
> >
> > return 0;
> > }
On 2023-09-27 12:20, Martin Kepplinger wrote:
> Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
>> On 2023-09-25 11:23, Martin Kepplinger wrote:
>> > These custom commands will be sent to both the USB keyboard & mouse
>> > devices but only the mouse will respond. Avoid sending known-
>> > useless
>> > messages by always prepending the filter before sending them.
>> >
>> > Suggested-by: Jamie Lentin <[email protected]>
>> > Signed-off-by: Martin Kepplinger <[email protected]>
>> > ---
>> > drivers/hid/hid-lenovo.c | 27 +++++++++------------------
>> > 1 file changed, 9 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
>> > index 29aa6d372bad..922f3e5462f4 100644
>> > --- a/drivers/hid/hid-lenovo.c
>> > +++ b/drivers/hid/hid-lenovo.c
>> > @@ -521,6 +521,14 @@ static void lenovo_features_set_cptkbd(struct
>> > hid_device *hdev)
>> > int ret;
>> > struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
>> >
>> > + /* All the custom action happens on the USBMOUSE device for
>> > USB */
>> > + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
>> > + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
>> > + hdev->type != HID_TYPE_USBMOUSE) {
>> > + hid_dbg(hdev, "Ignoring keyboard half of
>> > device\n");
>> > + return;
>> > + }
>> > +
>> > /*
>> > * Tell the keyboard a driver understands it, and turn F7,
>> > F9, F11
>> > into
>> > * regular keys
>> > @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct
>> > hid_device
>> > *hdev)
>> > int ret;
>> > struct lenovo_drvdata *cptkbd_data;
>> >
>> > - /* All the custom action happens on the USBMOUSE device for
>> > USB */
>> > - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
>> > - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
>> > - hdev->type != HID_TYPE_USBMOUSE) {
>> > - hid_dbg(hdev, "Ignoring keyboard half of
>> > device\n");
>> > - return 0;
>> > - }
>> > -
>>
>> I like the idea of doing it once then forgetting about it, but
>> removing
>> this will mean that the "keyboard half" will have it's own set of
>> non-functional sysfs parameters I think? Currently:-
>>
>> # evtest
>> . . .
>> /dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
>> TrackPoint
>> /dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
>> TrackPoint
>> /dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
>> TrackPoint
>>
>> # ls -1 /sys/class/input/event*/device/device/fn_lock
>> /sys/class/input/event10/device/device/fn_lock
>> /sys/class/input/event12/device/device/fn_lock
>>
>> (note 11 is missing.)
>>
>> I think the easiest (but ugly) thing to do is to copy-paste this lump
>> of
>> code to the top of lenovo_reset_resume.
>> Cheers,
>>
>> > cptkbd_data = devm_kzalloc(&hdev->dev,
>> > sizeof(*cptkbd_data),
>> > GFP_KERNEL);
>> > @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct hid_device
>> > *hdev,
>> > #ifdef CONFIG_PM
>> > static int lenovo_reset_resume(struct hid_device *hdev)
>> > {
>> > - switch (hdev->product) {
>> > - case USB_DEVICE_ID_LENOVO_CUSBKBD:
>> > - if (hdev->type == HID_TYPE_USBMOUSE) {
>> > - lenovo_features_set_cptkbd(hdev);
>> > - }
>> > -
>> > - break;
>> > - default:
>> > - break;
>> > - }
>> > + lenovo_features_set_cptkbd(hdev);
>
> ok. ignore my change (this whole patch) and look at your addition here,
> don't you already make sure only the mouse-part gets the messages? you
> just write switch()case instead of if(); what do you think is missing
> here?
Correct, this switch statement() that you're removing in this patch
already does exactly this, so replacing it with the
if()-and-return-early block would result in equivalent code (ignoring
the Trackpoint keyboard II). That suggestion wasn't the most helpful of
mine, sorry!
The reason I originally used a switch here is for symmetry with
lenovo_probe(), lenovo_remove(), etc. It might some day be useful to add
something like:
case USB_DEVICE_ID_LENOVO_X1_TAB:
lenovo_reset_resume_tp10ubkbd(hdev);
break;
...to the switch. For completeness, lenovo_reset_resume() should
probably call a separate lenovo_reset_resume_cptkbd() that does the
work. For just 3 lines of code it didn't seem worth it at the time
though.
Cheers,
>
> thanks,
> martin
>
>> >
>> > return 0;
>> > }
Am Donnerstag, dem 28.09.2023 um 22:06 +0100 schrieb Jamie Lentin:
> On 2023-09-27 12:20, Martin Kepplinger wrote:
> > Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
> > > On 2023-09-25 11:23, Martin Kepplinger wrote:
> > > > These custom commands will be sent to both the USB keyboard &
> > > > mouse
> > > > devices but only the mouse will respond. Avoid sending known-
> > > > useless
> > > > messages by always prepending the filter before sending them.
> > > >
> > > > Suggested-by: Jamie Lentin <[email protected]>
> > > > Signed-off-by: Martin Kepplinger <[email protected]>
> > > > ---
> > > > drivers/hid/hid-lenovo.c | 27 +++++++++------------------
> > > > 1 file changed, 9 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-
> > > > lenovo.c
> > > > index 29aa6d372bad..922f3e5462f4 100644
> > > > --- a/drivers/hid/hid-lenovo.c
> > > > +++ b/drivers/hid/hid-lenovo.c
> > > > @@ -521,6 +521,14 @@ static void
> > > > lenovo_features_set_cptkbd(struct
> > > > hid_device *hdev)
> > > > int ret;
> > > > struct lenovo_drvdata *cptkbd_data =
> > > > hid_get_drvdata(hdev);
> > > >
> > > > + /* All the custom action happens on the USBMOUSE device
> > > > for
> > > > USB */
> > > > + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> > > > + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD))
> > > > &&
> > > > + hdev->type != HID_TYPE_USBMOUSE) {
> > > > + hid_dbg(hdev, "Ignoring keyboard half of
> > > > device\n");
> > > > + return;
> > > > + }
> > > > +
> > > > /*
> > > > * Tell the keyboard a driver understands it, and turn
> > > > F7,
> > > > F9, F11
> > > > into
> > > > * regular keys
> > > > @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct
> > > > hid_device
> > > > *hdev)
> > > > int ret;
> > > > struct lenovo_drvdata *cptkbd_data;
> > > >
> > > > - /* All the custom action happens on the USBMOUSE device
> > > > for
> > > > USB */
> > > > - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> > > > - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD))
> > > > &&
> > > > - hdev->type != HID_TYPE_USBMOUSE) {
> > > > - hid_dbg(hdev, "Ignoring keyboard half of
> > > > device\n");
> > > > - return 0;
> > > > - }
> > > > -
> > >
> > > I like the idea of doing it once then forgetting about it, but
> > > removing
> > > this will mean that the "keyboard half" will have it's own set of
> > > non-functional sysfs parameters I think? Currently:-
> > >
> > > # evtest
> > > . . .
> > > /dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
> > > TrackPoint
> > > /dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
> > > TrackPoint
> > > /dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
> > > TrackPoint
> > >
> > > # ls -1 /sys/class/input/event*/device/device/fn_lock
> > > /sys/class/input/event10/device/device/fn_lock
> > > /sys/class/input/event12/device/device/fn_lock
> > >
> > > (note 11 is missing.)
> > >
> > > I think the easiest (but ugly) thing to do is to copy-paste this
> > > lump
> > > of
> > > code to the top of lenovo_reset_resume.
> > > Cheers,
> > >
> > > > cptkbd_data = devm_kzalloc(&hdev->dev,
> > > > sizeof(*cptkbd_data),
> > > > GFP_KERNEL);
> > > > @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct
> > > > hid_device
> > > > *hdev,
> > > > #ifdef CONFIG_PM
> > > > static int lenovo_reset_resume(struct hid_device *hdev)
> > > > {
> > > > - switch (hdev->product) {
> > > > - case USB_DEVICE_ID_LENOVO_CUSBKBD:
> > > > - if (hdev->type == HID_TYPE_USBMOUSE) {
> > > > - lenovo_features_set_cptkbd(hdev);
> > > > - }
> > > > -
> > > > - break;
> > > > - default:
> > > > - break;
> > > > - }
> > > > + lenovo_features_set_cptkbd(hdev);
> >
> > ok. ignore my change (this whole patch) and look at your addition
> > here,
> > don't you already make sure only the mouse-part gets the messages?
> > you
> > just write switch()case instead of if(); what do you think is
> > missing
> > here?
>
> Correct, this switch statement() that you're removing in this patch
> already does exactly this, so replacing it with the
> if()-and-return-early block would result in equivalent code (ignoring
> the Trackpoint keyboard II). That suggestion wasn't the most helpful
> of
> mine, sorry!
>
> The reason I originally used a switch here is for symmetry with
> lenovo_probe(), lenovo_remove(), etc. It might some day be useful to
> add
> something like:
>
> case USB_DEVICE_ID_LENOVO_X1_TAB:
> lenovo_reset_resume_tp10ubkbd(hdev);
> break;
>
> ...to the switch. For completeness, lenovo_reset_resume() should
> probably call a separate lenovo_reset_resume_cptkbd() that does the
> work. For just 3 lines of code it didn't seem worth it at the time
> though.
>
> Cheers,
ok your original patch seems to basically be a valid first fix. Should
I send it on your behalf (with you as author) or do you want to send it
yourself? Let's get this fixed :)
thanks,
martin
On 2023-09-30 10:26, Martin Kepplinger wrote:
> Am Donnerstag, dem 28.09.2023 um 22:06 +0100 schrieb Jamie Lentin:
>> On 2023-09-27 12:20, Martin Kepplinger wrote:
>> > Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
>> > > On 2023-09-25 11:23, Martin Kepplinger wrote:
>> > > > These custom commands will be sent to both the USB keyboard &
>> > > > mouse
>> > > > devices but only the mouse will respond. Avoid sending known-
>> > > > useless
>> > > > messages by always prepending the filter before sending them.
>> > > >
>> > > > Suggested-by: Jamie Lentin <[email protected]>
>> > > > Signed-off-by: Martin Kepplinger <[email protected]>
>> > > > ---
>> > > > drivers/hid/hid-lenovo.c | 27 +++++++++------------------
>> > > > 1 file changed, 9 insertions(+), 18 deletions(-)
>> > > >
>> > > > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-
>> > > > lenovo.c
>> > > > index 29aa6d372bad..922f3e5462f4 100644
>> > > > --- a/drivers/hid/hid-lenovo.c
>> > > > +++ b/drivers/hid/hid-lenovo.c
>> > > > @@ -521,6 +521,14 @@ static void
>> > > > lenovo_features_set_cptkbd(struct
>> > > > hid_device *hdev)
>> > > > int ret;
>> > > > struct lenovo_drvdata *cptkbd_data =
>> > > > hid_get_drvdata(hdev);
>> > > >
>> > > > + /* All the custom action happens on the USBMOUSE device
>> > > > for
>> > > > USB */
>> > > > + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
>> > > > + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD))
>> > > > &&
>> > > > + hdev->type != HID_TYPE_USBMOUSE) {
>> > > > + hid_dbg(hdev, "Ignoring keyboard half of
>> > > > device\n");
>> > > > + return;
>> > > > + }
>> > > > +
>> > > > /*
>> > > > * Tell the keyboard a driver understands it, and turn
>> > > > F7,
>> > > > F9, F11
>> > > > into
>> > > > * regular keys
>> > > > @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct
>> > > > hid_device
>> > > > *hdev)
>> > > > int ret;
>> > > > struct lenovo_drvdata *cptkbd_data;
>> > > >
>> > > > - /* All the custom action happens on the USBMOUSE device
>> > > > for
>> > > > USB */
>> > > > - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
>> > > > - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD))
>> > > > &&
>> > > > - hdev->type != HID_TYPE_USBMOUSE) {
>> > > > - hid_dbg(hdev, "Ignoring keyboard half of
>> > > > device\n");
>> > > > - return 0;
>> > > > - }
>> > > > -
>> > >
>> > > I like the idea of doing it once then forgetting about it, but
>> > > removing
>> > > this will mean that the "keyboard half" will have it's own set of
>> > > non-functional sysfs parameters I think? Currently:-
>> > >
>> > > # evtest
>> > > . . .
>> > > /dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
>> > > TrackPoint
>> > > /dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
>> > > TrackPoint
>> > > /dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
>> > > TrackPoint
>> > >
>> > > # ls -1 /sys/class/input/event*/device/device/fn_lock
>> > > /sys/class/input/event10/device/device/fn_lock
>> > > /sys/class/input/event12/device/device/fn_lock
>> > >
>> > > (note 11 is missing.)
>> > >
>> > > I think the easiest (but ugly) thing to do is to copy-paste this
>> > > lump
>> > > of
>> > > code to the top of lenovo_reset_resume.
>> > > Cheers,
>> > >
>> > > > cptkbd_data = devm_kzalloc(&hdev->dev,
>> > > > sizeof(*cptkbd_data),
>> > > > GFP_KERNEL);
>> > > > @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct
>> > > > hid_device
>> > > > *hdev,
>> > > > #ifdef CONFIG_PM
>> > > > static int lenovo_reset_resume(struct hid_device *hdev)
>> > > > {
>> > > > - switch (hdev->product) {
>> > > > - case USB_DEVICE_ID_LENOVO_CUSBKBD:
>> > > > - if (hdev->type == HID_TYPE_USBMOUSE) {
>> > > > - lenovo_features_set_cptkbd(hdev);
>> > > > - }
>> > > > -
>> > > > - break;
>> > > > - default:
>> > > > - break;
>> > > > - }
>> > > > + lenovo_features_set_cptkbd(hdev);
>> >
>> > ok. ignore my change (this whole patch) and look at your addition
>> > here,
>> > don't you already make sure only the mouse-part gets the messages?
>> > you
>> > just write switch()case instead of if(); what do you think is
>> > missing
>> > here?
>>
>> Correct, this switch statement() that you're removing in this patch
>> already does exactly this, so replacing it with the
>> if()-and-return-early block would result in equivalent code (ignoring
>> the Trackpoint keyboard II). That suggestion wasn't the most helpful
>> of
>> mine, sorry!
>>
>> The reason I originally used a switch here is for symmetry with
>> lenovo_probe(), lenovo_remove(), etc. It might some day be useful to
>> add
>> something like:
>>
>> case USB_DEVICE_ID_LENOVO_X1_TAB:
>> lenovo_reset_resume_tp10ubkbd(hdev);
>> break;
>>
>> ...to the switch. For completeness, lenovo_reset_resume() should
>> probably call a separate lenovo_reset_resume_cptkbd() that does the
>> work. For just 3 lines of code it didn't seem worth it at the time
>> though.
>>
>> Cheers,
>
> ok your original patch seems to basically be a valid first fix. Should
> I send it on your behalf (with you as author) or do you want to send it
> yourself? Let's get this fixed :)
If it's working for you and you don't mind, feel free send it on my
behalf. I don't have the hardware to test the patch currently.
Thanks!
>
> thanks,
> martin