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

From: Martin Kepplinger <martink@posteo.de>
To: Jamie Lentin <jm@lentin.co.uk>
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
	linux-kernel@vger.kernel.org, linux-input@vger.kernel.org
Subject: Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd()
Date: Wed, 27 Sep 2023 11:20:23 +0000	[thread overview]
Message-ID: <137ee9ed434fe98fd773cd27895afc564f92a23c.camel@posteo.de> (raw)
In-Reply-To: <ef0f15c3b17ebbd58f7481910b3f40ff@lentin.co.uk>

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;
> >  }