Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd()

[Jamie Lentin]

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 <jm@xxxxxxxxxxxx>
> > > > > Signed-off-by: Martin Kepplinger <martink@xxxxxxxxx>
> > > > > ---
> > > > >  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