From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 009CBE77362 for ; Sat, 30 Sep 2023 09:26:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231401AbjI3J0n (ORCPT ); Sat, 30 Sep 2023 05:26:43 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48360 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229645AbjI3J0m (ORCPT ); Sat, 30 Sep 2023 05:26:42 -0400 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 67C85CA for ; Sat, 30 Sep 2023 02:26:39 -0700 (PDT) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id EF3F6240028 for ; Sat, 30 Sep 2023 11:26:35 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1696065996; bh=bhsTAvwHD9YkPK3+W0x6nsCnMuRURoezjRHkCLlCP8E=; h=Message-ID:Subject:From:To:Cc:Date:Content-Transfer-Encoding: MIME-Version:From; b=NCjwOitqw9g7HnOGc35Ok1lhZO3w6iatMclH66TXV31fi/mn4U40fGcJE/Ni5m7s+ abkSD3HDm61eEReBvm0Q/ddMqSRucRi2EB0pBR8fn+yzFwf68OL644Y+AGurKVgjyq emIIUDCGfIN+sv2MZ75Hinq7n8cZwJOGo8JzYIuhbLVJtx48bRyzgNy6QagRa1I0rZ 2IMGA0AUFnmIxdkYoe2qXBkdqF7AfkJ78m8KZ9MWKSsD2gW3yDJjdlGAvdhFjObjRI HrpoFSDY0bo0d1SnKSpm5eJpQCGFEWp+MZTZkljaFLcYF5NBu3F5S4lUgo17H1FohX Ovij8dGH8+QHQ== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4RyMKl2KCXz6tvk; Sat, 30 Sep 2023 11:26:35 +0200 (CEST) Message-ID: <2d1e21cc9677a5cfe828decd8cbd5e930237e76d.camel@posteo.de> Subject: Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd() From: Martin Kepplinger To: Jamie Lentin Cc: jikos@kernel.org, benjamin.tissoires@redhat.com, linux-kernel@vger.kernel.org, linux-input@vger.kernel.org Date: Sat, 30 Sep 2023 09:26:34 +0000 In-Reply-To: <6adc3e66402f38258eae3a044db9ee11@lentin.co.uk> References: <140b721bc345a846863a37ebf17c3174@lentin.co.uk> <20230925102302.13094-1-martink@posteo.de> <20230925102302.13094-2-martink@posteo.de> <137ee9ed434fe98fd773cd27895afc564f92a23c.camel@posteo.de> <6adc3e66402f38258eae3a044db9ee11@lentin.co.uk> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org 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. > > > >=20 > > > > Suggested-by: Jamie Lentin > > > > Signed-off-by: Martin Kepplinger > > > > --- > > > > =C2=A0drivers/hid/hid-lenovo.c | 27 +++++++++------------------ > > > > =C2=A01 file changed, 9 insertions(+), 18 deletions(-) > > > >=20 > > > > 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) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct lenovo_drvda= ta *cptkbd_data =3D > > > > hid_get_drvdata(hdev); > > > >=20 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* All the custom action= happens on the USBMOUSE device > > > > for > > > > USB */ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (((hdev->product =3D= =3D USB_DEVICE_ID_LENOVO_CUSBKBD) || > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (hdev= ->product =3D=3D USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) > > > > && > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 hdev-= >type !=3D HID_TYPE_USBMOUSE) { > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0hid_dbg(hdev, "Ignoring keyboard half of > > > > device\n"); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0return; > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > + > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * Tell the keyboar= d a driver understands it, and turn > > > > F7, > > > > F9, F11 > > > > into > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * regular keys > > > > @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct > > > > hid_device > > > > *hdev) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int ret; > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct lenovo_drvda= ta *cptkbd_data; > > > >=20 > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* All the custom action= happens on the USBMOUSE device > > > > for > > > > USB */ > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (((hdev->product =3D= =3D USB_DEVICE_ID_LENOVO_CUSBKBD) || > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (hdev= ->product =3D=3D USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) > > > > && > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 hdev-= >type !=3D HID_TYPE_USBMOUSE) { > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0hid_dbg(hdev, "Ignoring keyboard half of > > > > device\n"); > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0return 0; > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > - > > >=20 > > > 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:- > > >=20 > > > # evtest > > > =C2=A0=C2=A0 . . . > > > /dev/input/event10:=C2=A0=C2=A0=C2=A0=C2=A0 ThinkPad Compact Bluetoot= h Keyboard with > > > TrackPoint > > > /dev/input/event11:=C2=A0=C2=A0=C2=A0=C2=A0 Lenovo ThinkPad Compact U= SB Keyboard with > > > TrackPoint > > > /dev/input/event12:=C2=A0=C2=A0=C2=A0=C2=A0 Lenovo ThinkPad Compact U= SB Keyboard with > > > TrackPoint > > >=20 > > > # 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 > > >=20 > > > (note 11 is missing.) > > >=20 > > > 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, > > >=20 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0cptkbd_data =3D dev= m_kzalloc(&hdev->dev, > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0sizeof(*cptkbd_data), > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0GFP_KERNEL); > > > > @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct > > > > hid_device > > > > *hdev, > > > > =C2=A0#ifdef CONFIG_PM > > > > =C2=A0static int lenovo_reset_resume(struct hid_device *hdev) > > > > =C2=A0{ > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0switch (hdev->product) { > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case USB_DEVICE_ID_LENOV= O_CUSBKBD: > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0if (hdev->type =3D=3D HID_TYPE_USBMOUSE) { > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0len= ovo_features_set_cptkbd(hdev); > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0} > > > > - > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0break; > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0default: > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0break; > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0lenovo_features_set_cptk= bd(hdev); > >=20 > > 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? >=20 > Correct, this switch statement() that you're removing in this patch=20 > already does exactly this, so replacing it with the=20 > if()-and-return-early block would result in equivalent code (ignoring > the Trackpoint keyboard II). That suggestion wasn't the most helpful > of=20 > mine, sorry! >=20 > The reason I originally used a switch here is for symmetry with=20 > lenovo_probe(), lenovo_remove(), etc. It might some day be useful to > add=20 > something like: >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0case USB_DEVICE_ID_LENOVO= _X1_TAB: > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0lenovo_reset_resume_tp10ubkbd(hdev); > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0break; >=20 > ...to the switch. For completeness, lenovo_reset_resume() should=20 > probably call a separate lenovo_reset_resume_cptkbd() that does the=20 > work. For just 3 lines of code it didn't seem worth it at the time=20 > though. >=20 > 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