diff mbox series

hid: lenovo: Resend all settings on reset_resume for compact keyboards

Message ID 20231002150914.22101-1-martink@posteo.de (mailing list archive)
State Mainlined
Commit 2f2bd7cbd1d1548137b351040dc4e037d18cdfdc
Delegated to: Jiri Kosina
Headers show
Series hid: lenovo: Resend all settings on reset_resume for compact keyboards | expand

Commit Message

Martin Kepplinger-Novakovic Oct. 2, 2023, 3:09 p.m. UTC 
From: Jamie Lentin <jm@lentin.co.uk>

The USB Compact Keyboard variant requires a reset_resume function to
restore keyboard configuration after a suspend in some situations. Move
configuration normally done on probe to lenovo_features_set_cptkbd(), then
recycle this for use on reset_resume.

Without, the keyboard and driver would end up in an inconsistent state,
breaking middle-button scrolling amongst other problems, and twiddling
sysfs values wouldn't help as the middle-button mode won't be set until
the driver is reloaded.

Tested on a USB and Bluetooth Thinkpad Compact Keyboard.

CC: stable@vger.kernel.org
Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for compact keyboards")
Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/hid/hid-lenovo.c | 50 +++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 16 deletions(-)

Comments

Martin Kepplinger-Novakovic Oct. 12, 2023, 7:51 a.m. UTC | #1 
Am Montag, dem 02.10.2023 um 15:09 +0000 schrieb Martin Kepplinger:
> From: Jamie Lentin <jm@lentin.co.uk>
> 
> The USB Compact Keyboard variant requires a reset_resume function to
> restore keyboard configuration after a suspend in some situations.
> Move
> configuration normally done on probe to lenovo_features_set_cptkbd(),
> then
> recycle this for use on reset_resume.
> 
> Without, the keyboard and driver would end up in an inconsistent
> state,
> breaking middle-button scrolling amongst other problems, and
> twiddling
> sysfs values wouldn't help as the middle-button mode won't be set
> until
> the driver is reloaded.
> 
> Tested on a USB and Bluetooth Thinkpad Compact Keyboard.
> 
> CC: stable@vger.kernel.org
> Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for
> compact keyboards")
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---

ok who could review and possibly queue this? This fixes a pretty
annoying bug and makes the Keyboard usable after resuming from system
suspend. Jiri or Benjamin? Should I add Dmitry?

thanks a lot,

                         martin


>  drivers/hid/hid-lenovo.c | 50 +++++++++++++++++++++++++++-----------
> --
>  1 file changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 44763c0da444..614320bff39f 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -521,6 +521,19 @@ static void lenovo_features_set_cptkbd(struct
> hid_device *hdev)
>         int ret;
>         struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
>  
> +       /*
> +        * Tell the keyboard a driver understands it, and turn F7,
> F9, F11 into
> +        * regular keys
> +        */
> +       ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
> +       if (ret)
> +               hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n",
> ret);
> +
> +       /* Switch middle button to native mode */
> +       ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
> +       if (ret)
> +               hid_warn(hdev, "Failed to switch middle button:
> %d\n", ret);
> +
>         ret = lenovo_send_cmd_cptkbd(hdev, 0x05, cptkbd_data-
> >fn_lock);
>         if (ret)
>                 hid_err(hdev, "Fn-lock setting failed: %d\n", ret);
> @@ -1126,22 +1139,6 @@ static int lenovo_probe_cptkbd(struct
> hid_device *hdev)
>         }
>         hid_set_drvdata(hdev, cptkbd_data);
>  
> -       /*
> -        * Tell the keyboard a driver understands it, and turn F7,
> F9, F11 into
> -        * regular keys (Compact only)
> -        */
> -       if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD ||
> -           hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD) {
> -               ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
> -               if (ret)
> -                       hid_warn(hdev, "Failed to switch F7/9/11
> mode: %d\n", ret);
> -       }
> -
> -       /* Switch middle button to native mode */
> -       ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
> -       if (ret)
> -               hid_warn(hdev, "Failed to switch middle button:
> %d\n", ret);
> -
>         /* Set keyboard settings to known state */
>         cptkbd_data->middlebutton_state = 0;
>         cptkbd_data->fn_lock = true;
> @@ -1264,6 +1261,24 @@ static int lenovo_probe(struct hid_device
> *hdev,
>         return ret;
>  }
>  
> +#ifdef CONFIG_PM
> +static int lenovo_reset_resume(struct hid_device *hdev)
> +{
> +       switch (hdev->product) {
> +       case USB_DEVICE_ID_LENOVO_CUSBKBD:
> +       case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
> +               if (hdev->type == HID_TYPE_USBMOUSE)
> +                       lenovo_features_set_cptkbd(hdev);
> +
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
>  static void lenovo_remove_tpkbd(struct hid_device *hdev)
>  {
>         struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
> @@ -1380,6 +1395,9 @@ static struct hid_driver lenovo_driver = {
>         .raw_event = lenovo_raw_event,
>         .event = lenovo_event,
>         .report_fixup = lenovo_report_fixup,
> +#ifdef CONFIG_PM
> +       .reset_resume = lenovo_reset_resume,
> +#endif
>  };
>  module_hid_driver(lenovo_driver);
>
Martin Kepplinger-Novakovic Oct. 24, 2023, 8:55 a.m. UTC | #2 
Am Montag, dem 02.10.2023 um 15:09 +0000 schrieb Martin Kepplinger:
> From: Jamie Lentin <jm@lentin.co.uk>
> 
> The USB Compact Keyboard variant requires a reset_resume function to
> restore keyboard configuration after a suspend in some situations.
> Move
> configuration normally done on probe to lenovo_features_set_cptkbd(),
> then
> recycle this for use on reset_resume.
> 
> Without, the keyboard and driver would end up in an inconsistent
> state,
> breaking middle-button scrolling amongst other problems, and
> twiddling
> sysfs values wouldn't help as the middle-button mode won't be set
> until
> the driver is reloaded.
> 
> Tested on a USB and Bluetooth Thinkpad Compact Keyboard.
> 
> CC: stable@vger.kernel.org
> Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for
> compact keyboards")
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>

This is sitting over 3 weeks and I simply add Bernhard and Hans who
wrote big parts of the driver. Maybe more review can help with queuing
this bugfix up? (scrolling and function keys are currently broken after
resuming)

I basically sent Jamie's patch because I have the hardware:
https://lore.kernel.org/all/20231002150914.22101-1-martink@posteo.de/

thanks,
                               martin


> ---
>  drivers/hid/hid-lenovo.c | 50 +++++++++++++++++++++++++++-----------
> --
>  1 file changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 44763c0da444..614320bff39f 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -521,6 +521,19 @@ static void lenovo_features_set_cptkbd(struct
> hid_device *hdev)
>         int ret;
>         struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
>  
> +       /*
> +        * Tell the keyboard a driver understands it, and turn F7,
> F9, F11 into
> +        * regular keys
> +        */
> +       ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
> +       if (ret)
> +               hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n",
> ret);
> +
> +       /* Switch middle button to native mode */
> +       ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
> +       if (ret)
> +               hid_warn(hdev, "Failed to switch middle button:
> %d\n", ret);
> +
>         ret = lenovo_send_cmd_cptkbd(hdev, 0x05, cptkbd_data-
> >fn_lock);
>         if (ret)
>                 hid_err(hdev, "Fn-lock setting failed: %d\n", ret);
> @@ -1126,22 +1139,6 @@ static int lenovo_probe_cptkbd(struct
> hid_device *hdev)
>         }
>         hid_set_drvdata(hdev, cptkbd_data);
>  
> -       /*
> -        * Tell the keyboard a driver understands it, and turn F7,
> F9, F11 into
> -        * regular keys (Compact only)
> -        */
> -       if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD ||
> -           hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD) {
> -               ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
> -               if (ret)
> -                       hid_warn(hdev, "Failed to switch F7/9/11
> mode: %d\n", ret);
> -       }
> -
> -       /* Switch middle button to native mode */
> -       ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
> -       if (ret)
> -               hid_warn(hdev, "Failed to switch middle button:
> %d\n", ret);
> -
>         /* Set keyboard settings to known state */
>         cptkbd_data->middlebutton_state = 0;
>         cptkbd_data->fn_lock = true;
> @@ -1264,6 +1261,24 @@ static int lenovo_probe(struct hid_device
> *hdev,
>         return ret;
>  }
>  
> +#ifdef CONFIG_PM
> +static int lenovo_reset_resume(struct hid_device *hdev)
> +{
> +       switch (hdev->product) {
> +       case USB_DEVICE_ID_LENOVO_CUSBKBD:
> +       case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
> +               if (hdev->type == HID_TYPE_USBMOUSE)
> +                       lenovo_features_set_cptkbd(hdev);
> +
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       return 0;
> +}
> +#endif
> +
>  static void lenovo_remove_tpkbd(struct hid_device *hdev)
>  {
>         struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
> @@ -1380,6 +1395,9 @@ static struct hid_driver lenovo_driver = {
>         .raw_event = lenovo_raw_event,
>         .event = lenovo_event,
>         .report_fixup = lenovo_report_fixup,
> +#ifdef CONFIG_PM
> +       .reset_resume = lenovo_reset_resume,
> +#endif
>  };
>  module_hid_driver(lenovo_driver);
>
Benjamin Tissoires Oct. 25, 2023, 7:01 p.m. UTC | #3 
On Mon, 02 Oct 2023 15:09:14 +0000, Martin Kepplinger wrote:
> The USB Compact Keyboard variant requires a reset_resume function to
> restore keyboard configuration after a suspend in some situations. Move
> configuration normally done on probe to lenovo_features_set_cptkbd(), then
> recycle this for use on reset_resume.
> 
> Without, the keyboard and driver would end up in an inconsistent state,
> breaking middle-button scrolling amongst other problems, and twiddling
> sysfs values wouldn't help as the middle-button mode won't be set until
> the driver is reloaded.
> 
> [...]

Applied to hid/hid.git (for-6.7/lenovo), thanks!

[1/1] hid: lenovo: Resend all settings on reset_resume for compact keyboards
      https://git.kernel.org/hid/hid/c/2f2bd7cbd1d1

Cheers,
diff mbox series

Patch

diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..614320bff39f 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -521,6 +521,19 @@  static void lenovo_features_set_cptkbd(struct hid_device *hdev)
 	int ret;
 	struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
 
+	/*
+	 * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
+	 * regular keys
+	 */
+	ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
+	if (ret)
+		hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n", ret);
+
+	/* Switch middle button to native mode */
+	ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
+	if (ret)
+		hid_warn(hdev, "Failed to switch middle button: %d\n", ret);
+
 	ret = lenovo_send_cmd_cptkbd(hdev, 0x05, cptkbd_data->fn_lock);
 	if (ret)
 		hid_err(hdev, "Fn-lock setting failed: %d\n", ret);
@@ -1126,22 +1139,6 @@  static int lenovo_probe_cptkbd(struct hid_device *hdev)
 	}
 	hid_set_drvdata(hdev, cptkbd_data);
 
-	/*
-	 * Tell the keyboard a driver understands it, and turn F7, F9, F11 into
-	 * regular keys (Compact only)
-	 */
-	if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD ||
-	    hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD) {
-		ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
-		if (ret)
-			hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n", ret);
-	}
-
-	/* Switch middle button to native mode */
-	ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
-	if (ret)
-		hid_warn(hdev, "Failed to switch middle button: %d\n", ret);
-
 	/* Set keyboard settings to known state */
 	cptkbd_data->middlebutton_state = 0;
 	cptkbd_data->fn_lock = true;
@@ -1264,6 +1261,24 @@  static int lenovo_probe(struct hid_device *hdev,
 	return ret;
 }
 
+#ifdef CONFIG_PM
+static int lenovo_reset_resume(struct hid_device *hdev)
+{
+	switch (hdev->product) {
+	case USB_DEVICE_ID_LENOVO_CUSBKBD:
+	case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
+		if (hdev->type == HID_TYPE_USBMOUSE)
+			lenovo_features_set_cptkbd(hdev);
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+#endif
+
 static void lenovo_remove_tpkbd(struct hid_device *hdev)
 {
 	struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
@@ -1380,6 +1395,9 @@  static struct hid_driver lenovo_driver = {
 	.raw_event = lenovo_raw_event,
 	.event = lenovo_event,
 	.report_fixup = lenovo_report_fixup,
+#ifdef CONFIG_PM
+	.reset_resume = lenovo_reset_resume,
+#endif
 };
 module_hid_driver(lenovo_driver);