Messages in this thread | | | Date | Thu, 28 Sep 2023 22:06:24 +0100 | From | Jamie Lentin <> | Subject | Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd() |
| |
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; >> > }
| |