Message ID | 20230925102302.13094-1-martink@posteo.de |
---|---|
State | Superseded |
Headers | show |
Series | [RFC,1/2] hid: lenovo: Resend all settings on reset_resume for compact keyboards | expand |
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 <jm@lentin.co.uk> > Signed-off-by: Martin Kepplinger <martink@posteo.de> > --- > 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 <jm@lentin.co.uk> > > Signed-off-by: Martin Kepplinger <martink@posteo.de> > > --- > > 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 <jm@lentin.co.uk> >> > Signed-off-by: Martin Kepplinger <martink@posteo.de> >> > --- >> > 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 <jm@lentin.co.uk> > > > > Signed-off-by: Martin Kepplinger <martink@posteo.de> > > > > --- > > > > 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
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);