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

From: Jamie Lentin <jm@lentin.co.uk>
To: Martin Kepplinger <martink@posteo.de>
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: Thu, 28 Sep 2023 22:06:24 +0100	[thread overview]
Message-ID: <6adc3e66402f38258eae3a044db9ee11@lentin.co.uk> (raw)
In-Reply-To: <137ee9ed434fe98fd773cd27895afc564f92a23c.camel@posteo.de>

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