[PATCH v2 0/2] Keyboard backlight for the Lenovo Legion Pro 7 Gen 10

[PATCH v2 0/2] Keyboard backlight for the Lenovo Legion Pro 7 Gen 10

[Ciro Pedrini via B4 Relay]

Hi,

This is v2 of the Legion keyboard lighting series. After Rong's comments
it's a lot smaller: it only handles the keyboard controller (048d:c197)
now, and exposes its brightness as a normal kbd_backlight LED. The
LampArray (048d:c193) is left for a generic LampArray driver.

Some background in case you missed v1: on my laptop the keyboard
backlight never turned on under Linux. The controller's brightness was 0
from the factory and nothing on Linux could change it. The controller
has a vendor feature report (0x07) with a stored brightness from 0 to 9,
and that brightness dims every LED of the machine. Fn+Up/Down/Space are
handled by the firmware, which also sends the new brightness on a vendor
input report.

Patch 1 stops ideapad-laptop from sending KEY_UNKNOWN for those three
keys. Patch 2 is the driver: the brightness as kbd_backlight, plus
brightness_hw_changed for the hotkeys so the desktop OSD works.

Derek, Mark: I put the driver under your "LENOVO HID drivers" entry, and
I already did that in v1 without asking you, sorry. Is that OK with you?
If not, I'll add a separate entry.

I tested it on my 16IAX10H (83F5, BIOS Q7CN77WW) with 7.2: brightness
from sysfs and KDE, the hotkeys and the OSD, suspend/resume, unbind and
rebind. It's based on pdx86 for-next and builds with W=1.

About how this was made: I found the protocol with usbmon captures and
by looking at Lenovo's Windows software. I wrote the code with help from
Claude Code (Claude Opus 5.5). I went through all of it and tested it
myself, and each patch has a short note about it below the ---.

Ilpo, Rong: thanks a lot for taking the time to review v1 so carefully.
Looking back, it was rougher than it should have been. If you see
anything else that needs fixing, I'm happy to keep working on it.

Thanks,
Ciro

---
Changes in v2:
- Only the keyboard controller (c197) now, as Rong suggested. No
  LampArray, no colours, no lighting_mode, just a kbd_backlight LED.
- Ilpo's comments: struct with __le16 for the request, guard(),
  devm_mutex_init(), no magic numbers, includes.
- Removed the "ready" flag, its spinlock and the work item. raw_event
  can't run during probe/remove, so it notifies the LED directly.
- Keep led_cdev.brightness in sync with the hotkeys (from the Sashiko
  review).
- Moved the note about the AI tool out of the changelogs, below the ---.
- Link to v1: https://lore.kernel.org/r/20261109-legion-lighting-rfc-v1-0-a87e1db8c5e3@gmail.com

---
Ciro Pedrini (2):
      platform/x86: ideapad-laptop: Ignore keyboard backlight hotkeys
      HID: lenovo-legion: Add keyboard backlight driver for Legion Gen 10

 MAINTAINERS                                  |   1 +
 drivers/hid/Kconfig                          |  13 ++
 drivers/hid/Makefile                         |   1 +
 drivers/hid/hid-ids.h                        |   1 +
 drivers/hid/hid-lenovo-legion.c              | 261 +++++++++++++++++++++++++++
 drivers/platform/x86/lenovo/ideapad-laptop.c |   4 +
 6 files changed, 281 insertions(+)
---
base-commit: a514d723595bd8e5ce926a7fea148ceb3c26915c
change-id: 20261109-legion-lighting-rfc-dc32165df52d

Best regards,
-- 
Ciro Pedrini <ciro.pedrini@gmail.com>

[PATCH v2 1/2] platform/x86: ideapad-laptop: Ignore keyboard backlight hotkeys

[Ciro Pedrini via B4 Relay]

From: Ciro Pedrini <ciro.pedrini@gmail.com>

On the Legion Pro 7 16IAX10H, Fn+Space (backlight effect), Fn+Up and
Fn+Down (backlight brightness) emit WMI fn-key events 0x21, 0x1a and
0x19, which the keymap does not cover, so they reach userspace as
KEY_UNKNOWN. The keyboard's USB controller handles these keys itself: it
switches the effect or changes the brightness, and reports the new level
on its own HID interface. Ignore the three events, as is already done for
FnLock and Esc, rather than mapping them to KEY_KBDILLUM*, which would
make userspace change the brightness a second time.

Tested on the 16IAX10H with 7.2: with the change, evtest shows no events
for the three keys, and the keyboard still switches the effect and the
brightness.

Signed-off-by: Ciro Pedrini <ciro.pedrini@gmail.com>
---
Prepared with Claude Code (Claude Opus 5.5) from evtest on the laptop's
"Ideapad extra buttons" device; I reviewed and tested it.
---
 drivers/platform/x86/lenovo/ideapad-laptop.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
index 91a8d11c79c5..8b6c08b21c8d 100644
--- a/drivers/platform/x86/lenovo/ideapad-laptop.c
+++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
@@ -1336,6 +1336,10 @@ static const struct key_entry ideapad_keymap[] = {
 	{ KE_IGNORE,	0x02 | IDEAPAD_WMI_KEY },
 	/* Esc (handled by the firmware) */
 	{ KE_IGNORE,	0x03 | IDEAPAD_WMI_KEY },
+	/* Keyboard backlight down, up and effect (handled by the keyboard) */
+	{ KE_IGNORE,	0x19 | IDEAPAD_WMI_KEY },
+	{ KE_IGNORE,	0x1a | IDEAPAD_WMI_KEY },
+	{ KE_IGNORE,	0x21 | IDEAPAD_WMI_KEY },
 	/* Customizable Lenovo Hotkey ("star" with 'S' inside) */
 	{ KE_KEY,	0x01 | IDEAPAD_WMI_KEY, { KEY_FAVORITES } },
 	{ KE_KEY,	0x04 | IDEAPAD_WMI_KEY, { KEY_SELECTIVE_SCREENSHOT } },

-- 
2.56.0

[PATCH v2 2/2] HID: lenovo-legion: Add keyboard backlight driver for Legion Gen 10

[Ciro Pedrini via B4 Relay]

From: Ciro Pedrini <ciro.pedrini@gmail.com>

The keyboard backlight of the Lenovo Legion Pro 7 16IAX10H (2025) has no
Linux support: on the test unit nothing ever lit, because the keyboard
controller's brightness was 0 from the factory and nothing exposes it.

The ITE 8258 keyboard controller (048d:c197) is the built-in keyboard and
carries a vendor protocol on feature report 0x07 (960 bytes): a request,
answered by reading the report back once the controller has prepared it.
Its hardware brightness, 0..9, is stored in the controller and applies to
every LED of the machine (keys, front strip, rear vents, lid logo). The
firmware handles Fn+Up and Fn+Down itself and reports the new level on
vendor input report 5.

Expose the brightness as a kbd_backlight LED, legion::kbd_backlight, with
brightness_hw_changed for the hotkeys so that desktops show their OSD.
Unregistering keeps the level (LED_RETAIN_AT_SHUTDOWN): switching it off
would be stored by the controller.

The colours stay the controller's own effects, which Fn+Space cycles. The
machine's HID LampArray (048d:c193) is left to hid-generic, or to a
generic LampArray driver.

Tested on the 16IAX10H (83F5) with 7.2 as a module: brightness from sysfs
and KDE, the hotkeys with KDE's OSD, suspend/resume, unbinding and
rebinding, and a userspace LampArray client alongside the driver.

Signed-off-by: Ciro Pedrini <ciro.pedrini@gmail.com>
---
Prepared with Claude Code (Claude Opus 5.5) from measurements on the
laptop (usbmon captures) and analysis of the vendor's Windows software
for interoperability; I reviewed and tested it.
---
 MAINTAINERS                     |   1 +
 drivers/hid/Kconfig             |  13 ++
 drivers/hid/Makefile            |   1 +
 drivers/hid/hid-ids.h           |   1 +
 drivers/hid/hid-lenovo-legion.c | 261 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 277 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 222b7fc2c3aa..843906a1f9d5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15187,6 +15187,7 @@ F:	Documentation/ABI/testing/sysfs-driver-hid-lenovo-go
 F:	Documentation/ABI/testing/sysfs-driver-hid-lenovo-go-s
 F:	drivers/hid/hid-lenovo-go-s.c
 F:	drivers/hid/hid-lenovo-go.c
+F:	drivers/hid/hid-lenovo-legion.c
 F:	drivers/hid/hid-lenovo.c
 
 LENOVO YOGA BOOK 9 KEYBOARD DOCK DRIVER
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 2481bc01c435..55d25afddb69 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -652,6 +652,19 @@ config HID_LENOVO_GO_S
 	S. Say M here to compile this driver as a module. The module will be called
 	hid-lenovo-go-s.
 
+config HID_LENOVO_LEGION
+	tristate "Lenovo Legion laptop keyboard backlight"
+	depends on USB_HID
+	depends on LEDS_CLASS
+	help
+	Support for the keyboard backlight of 2025 Lenovo Legion laptops
+	(Legion Pro 7 Gen 10): the hardware brightness of the ITE keyboard
+	controller as a kbd_backlight LED, with the Fn+Up/Down changes
+	reported to userspace.
+
+	Say M here to compile this driver as a module. The module will be called
+	hid-lenovo-legion.
+
 config HID_LETSKETCH
 	tristate "Letsketch WP9620N tablets"
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 43a8eb0f389b..713bb1c89861 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_HID_LCPOWER)	+= hid-lcpower.o
 obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
 obj-$(CONFIG_HID_LENOVO_GO)	+= hid-lenovo-go.o
 obj-$(CONFIG_HID_LENOVO_GO_S)	+= hid-lenovo-go-s.o
+obj-$(CONFIG_HID_LENOVO_LEGION)	+= hid-lenovo-legion.o
 obj-$(CONFIG_HID_LETSKETCH)	+= hid-letsketch.o
 obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
 obj-$(CONFIG_HID_LOGITECH)	+= hid-lg-g15.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 91319aa6516d..992b9b147e95 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -773,6 +773,7 @@
 #define USB_DEVICE_ID_ITE_LENOVO_YOGA2  0x8350
 #define I2C_DEVICE_ID_ITE_LENOVO_LEGION_Y720	0x837a
 #define USB_DEVICE_ID_ITE_LENOVO_YOGA900	0x8396
+#define USB_DEVICE_ID_ITE_LENOVO_LEGION_GEN10_KBD	0xc197
 #define I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_KEYBOARD	0x8987
 #define I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_G11_KEYBOARD	0x83db
 #define USB_DEVICE_ID_ITE8595		0x8595
diff --git a/drivers/hid/hid-lenovo-legion.c b/drivers/hid/hid-lenovo-legion.c
new file mode 100644
index 000000000000..6d0b4adc6f49
--- /dev/null
+++ b/drivers/hid/hid-lenovo-legion.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for the keyboard backlight of Lenovo Legion laptops (2025).
+ *
+ * The ITE 8258 keyboard controller (048d:c197) is the built-in keyboard and
+ * carries Lenovo's vendor lighting protocol: feature report 0x07, 960 bytes,
+ * a request (struct legion_msg) answered by reading the report back with a
+ * GET_FEATURE once the controller has prepared it (up to ~50 ms).
+ *
+ * The backlight brightness is op 0xCE (set) / 0xCD (get), 0..9, stored in the
+ * controller; it applies to every LED of the machine (keys, front strip, rear
+ * vents, lid logo). The firmware handles Fn+Up / Fn+Down itself and reports
+ * the new level on vendor input report 5 (struct legion_event).
+ *
+ * The colours are the controller's own effects. The LEDs can also be handed
+ * to the HID LampArray of the "Lenovo Lighting" device (048d:c193), which
+ * this driver leaves to others.
+ */
+
+#include <linux/cleanup.h>
+#include <linux/compiler.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/leds.h>
+#include <linux/lockdep.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include <asm/byteorder.h>
+
+#include "hid-ids.h"
+
+#define LEGION_REPORT_ID		0x07
+#define LEGION_REPORT_SIZE		960
+#define LEGION_ANSWER_DELAY_MS		100
+
+#define LEGION_OP_GET_BRIGHTNESS	0xcd
+#define LEGION_OP_SET_BRIGHTNESS	0xce
+#define LEGION_MAX_BRIGHTNESS		9
+
+#define LEGION_EVENT_REPORT_ID		0x05
+#define LEGION_EVENT_BRIGHTNESS_UP	0x02
+#define LEGION_EVENT_BRIGHTNESS_DOWN	0x03
+
+/* Request, and its answer: the value is params[0]. */
+struct legion_msg {
+	u8 report_id;
+	u8 op;
+	__le16 len;			/* of params */
+	u8 params[];
+} __packed;
+
+struct legion_event {
+	u8 report_id;
+	u8 event;
+	u8 level;			/* brightness after the change */
+} __packed;
+
+struct legion_kbd {
+	struct hid_device *hdev;
+	struct mutex lock;		/* one request/answer exchange at a time */
+	struct legion_msg *msg;		/* LEGION_REPORT_SIZE, DMA-safe */
+	struct led_classdev led;
+};
+
+/*
+ * Send one request. When @answer is non-NULL, wait for the controller to
+ * prepare its answer and return its value there.
+ */
+static int legion_exchange(struct legion_kbd *kbd, u8 op, const u8 *params,
+			   u16 len, u8 *answer)
+{
+	struct hid_device *hdev = kbd->hdev;
+	struct legion_msg *msg = kbd->msg;
+	int ret;
+
+	lockdep_assert_held(&kbd->lock);
+
+	memset(msg, 0, LEGION_REPORT_SIZE);
+	msg->report_id = LEGION_REPORT_ID;
+	msg->op = op;
+	msg->len = cpu_to_le16(len);
+	memcpy(msg->params, params, len);
+
+	ret = hid_hw_raw_request(hdev, LEGION_REPORT_ID, (u8 *)msg,
+				 LEGION_REPORT_SIZE, HID_FEATURE_REPORT,
+				 HID_REQ_SET_REPORT);
+	if (ret < 0)
+		return ret;
+	if (!answer)
+		return 0;
+
+	msleep(LEGION_ANSWER_DELAY_MS);
+
+	memset(msg, 0, LEGION_REPORT_SIZE);
+	ret = hid_hw_raw_request(hdev, LEGION_REPORT_ID, (u8 *)msg,
+				 LEGION_REPORT_SIZE, HID_FEATURE_REPORT,
+				 HID_REQ_GET_REPORT);
+	if (ret < 0)
+		return ret;
+	if (ret <= (int)sizeof(*msg) || msg->report_id != LEGION_REPORT_ID ||
+	    msg->op != op) {
+		hid_warn(hdev, "unexpected answer to 0x%02x: %*ph\n", op,
+			 min(ret, 8), msg);
+		return -EIO;
+	}
+
+	*answer = msg->params[0];
+	return 0;
+}
+
+static int legion_brightness_set(struct led_classdev *led,
+				 enum led_brightness value)
+{
+	struct legion_kbd *kbd = container_of(led, struct legion_kbd, led);
+	u8 level = min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);
+
+	guard(mutex)(&kbd->lock);
+	return legion_exchange(kbd, LEGION_OP_SET_BRIGHTNESS, &level,
+			       sizeof(level), NULL);
+}
+
+static enum led_brightness legion_brightness_get(struct led_classdev *led)
+{
+	struct legion_kbd *kbd = container_of(led, struct legion_kbd, led);
+	u8 zero = 0, value;
+	int ret;
+
+	guard(mutex)(&kbd->lock);
+	ret = legion_exchange(kbd, LEGION_OP_GET_BRIGHTNESS, &zero,
+			      sizeof(zero), &value);
+	if (ret)
+		return led->brightness;	/* keep the last known level */
+
+	return min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);
+}
+
+static int legion_raw_event(struct hid_device *hdev, struct hid_report *report,
+			    u8 *data, int size)
+{
+	const struct legion_event *ev = (const struct legion_event *)data;
+	struct legion_kbd *kbd = hid_get_drvdata(hdev);
+	unsigned int level;
+
+	if (size < (int)sizeof(*ev) || ev->report_id != LEGION_EVENT_REPORT_ID)
+		return 0;
+
+	if (ev->event != LEGION_EVENT_BRIGHTNESS_UP &&
+	    ev->event != LEGION_EVENT_BRIGHTNESS_DOWN)
+		return 0;
+
+	/*
+	 * hid-core calls this only after probe() has returned and not once
+	 * remove() has started, so the LED is registered. The controller has
+	 * already changed the level: keep led_cdev.brightness in step with it.
+	 */
+	level = min_t(unsigned int, ev->level, LEGION_MAX_BRIGHTNESS);
+	WRITE_ONCE(kbd->led.brightness, level);
+	led_classdev_notify_brightness_hw_changed(&kbd->led, level);
+
+	return 0;	/* let hid-core handle the report as usual */
+}
+
+static int legion_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	struct legion_kbd *kbd;
+	int ret;
+
+	/* As hid-generic: one input device per application collection. */
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	kbd = devm_kzalloc_obj(&hdev->dev, *kbd);
+	if (!kbd)
+		return -ENOMEM;
+
+	kbd->msg = devm_kzalloc(&hdev->dev, LEGION_REPORT_SIZE, GFP_KERNEL);
+	if (!kbd->msg)
+		return -ENOMEM;
+
+	ret = devm_mutex_init(&hdev->dev, &kbd->lock);
+	if (ret)
+		return ret;
+
+	kbd->hdev = hdev;
+	hid_set_drvdata(hdev, kbd);
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret)
+		return ret;
+
+	/* Keep the interrupt endpoint polled so hotkey reports always arrive. */
+	ret = hid_hw_open(hdev);
+	if (ret)
+		goto err_stop;
+
+	kbd->led.name = "legion::kbd_backlight";
+	kbd->led.max_brightness = LEGION_MAX_BRIGHTNESS;
+	kbd->led.brightness_set_blocking = legion_brightness_set;
+	kbd->led.brightness_get = legion_brightness_get;
+	/*
+	 * LED_RETAIN_AT_SHUTDOWN: unregistering (unload, unbind) must not
+	 * switch the backlight off, the controller would store 0.
+	 * LED_BRIGHT_HW_CHANGED: Fn+Up/Down are reported to userspace.
+	 */
+	kbd->led.flags = LED_RETAIN_AT_SHUTDOWN | LED_BRIGHT_HW_CHANGED;
+
+	/*
+	 * Not devm: the LED must be gone before remove() stops the device,
+	 * devm would release it only after that.
+	 */
+	ret = led_classdev_register(&hdev->dev, &kbd->led);
+	if (ret)
+		goto err_close;
+
+	return 0;
+
+err_close:
+	hid_hw_close(hdev);
+err_stop:
+	hid_hw_stop(hdev);
+	return ret;
+}
+
+static void legion_remove(struct hid_device *hdev)
+{
+	struct legion_kbd *kbd = hid_get_drvdata(hdev);
+
+	/* hid-core no longer calls legion_raw_event(): the LED can go. */
+	led_classdev_unregister(&kbd->led);
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id legion_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE_LENOVO_LEGION_GEN10_KBD) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, legion_devices);
+
+static struct hid_driver legion_driver = {
+	.name = "hid-lenovo-legion",
+	.id_table = legion_devices,
+	.probe = legion_probe,
+	.remove = legion_remove,
+	.raw_event = legion_raw_event,
+};
+module_hid_driver(legion_driver);
+
+MODULE_AUTHOR("Ciro Pedrini <ciro.pedrini@gmail.com>");
+MODULE_DESCRIPTION("Keyboard backlight of Lenovo Legion laptops (2025)");
+MODULE_LICENSE("GPL");

-- 
2.56.0

Re: [PATCH v2 1/2] platform/x86: ideapad-laptop: Ignore keyboard backlight hotkeys

[Ciro Pedrini]

Hi,

Rong acked this patch in v1, I missed carrying it over:
https://lore.kernel.org/r/4d0153a209a7c54ff513eabd3fd87445269279f0.camel@rong.moe

Acked-by: Rong Zhang <i@rong.moe>

Thanks,
Ciro

Re: [PATCH v2 2/2] HID: lenovo-legion: Add keyboard backlight driver for Legion Gen 10

[Rong Zhang]

Hi Ciro,

On Tue, 2026-11-10 at 20:29 +0000, Ciro Pedrini via B4 Relay wrote:
> From: Ciro Pedrini <ciro.pedrini@gmail.com>
> 
> The keyboard backlight of the Lenovo Legion Pro 7 16IAX10H (2025) has no
> Linux support: on the test unit nothing ever lit, because the keyboard
> controller's brightness was 0 from the factory and nothing exposes it.
> 
> The ITE 8258 keyboard controller (048d:c197) is the built-in keyboard and
> carries a vendor protocol on feature report 0x07 (960 bytes): a request,
> answered by reading the report back once the controller has prepared it.
> Its hardware brightness, 0..9, is stored in the controller and applies to
> every LED of the machine (keys, front strip, rear vents, lid logo).
> 

I don't know if it's appropriate to represent so many things with the
name "legion::kbd_backlight".

>  The
> firmware handles Fn+Up and Fn+Down itself and reports the new level on
> vendor input report 5.
> 
> Expose the brightness as a kbd_backlight LED, legion::kbd_backlight, with
> brightness_hw_changed for the hotkeys so that desktops show their OSD.
> Unregistering keeps the level (LED_RETAIN_AT_SHUTDOWN): switching it off
> would be stored by the controller.
> 
> The colours stay the controller's own effects, which Fn+Space cycles. 
> 

Just curious, can the software select which effect to use, or listen to
an event when Fn+Space is pressed?

> The
> machine's HID LampArray (048d:c193) is left to hid-generic, or to a
> generic LampArray driver.
> 
> Tested on the 16IAX10H (83F5) with 7.2 as a module: brightness from sysfs
> and KDE, the hotkeys with KDE's OSD, suspend/resume, unbinding and
> rebinding, and a userspace LampArray client alongside the driver.
> 
> Signed-off-by: Ciro Pedrini <ciro.pedrini@gmail.com>
> ---
> Prepared with Claude Code (Claude Opus 5.5) from measurements on the
> laptop (usbmon captures) and analysis of the vendor's Windows software
> for interoperability; I reviewed and tested it.
> ---
>  MAINTAINERS                     |   1 +
>  drivers/hid/Kconfig             |  13 ++
>  drivers/hid/Makefile            |   1 +
>  drivers/hid/hid-ids.h           |   1 +
>  drivers/hid/hid-lenovo-legion.c | 261 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 277 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 222b7fc2c3aa..843906a1f9d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15187,6 +15187,7 @@ F:	Documentation/ABI/testing/sysfs-driver-hid-lenovo-go
>  F:	Documentation/ABI/testing/sysfs-driver-hid-lenovo-go-s
>  F:	drivers/hid/hid-lenovo-go-s.c
>  F:	drivers/hid/hid-lenovo-go.c
> +F:	drivers/hid/hid-lenovo-legion.c
>  F:	drivers/hid/hid-lenovo.c
>  
>  LENOVO YOGA BOOK 9 KEYBOARD DOCK DRIVER
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 2481bc01c435..55d25afddb69 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -652,6 +652,19 @@ config HID_LENOVO_GO_S
>  	S. Say M here to compile this driver as a module. The module will be called
>  	hid-lenovo-go-s.
>  
> +config HID_LENOVO_LEGION
> +	tristate "Lenovo Legion laptop keyboard backlight"
> +	depends on USB_HID
> +	depends on LEDS_CLASS
> +	help
> +	Support for the keyboard backlight of 2025 Lenovo Legion laptops
> +	(Legion Pro 7 Gen 10): the hardware brightness of the ITE keyboard
> +	controller as a kbd_backlight LED, with the Fn+Up/Down changes
> +	reported to userspace.

Try not to mention too many hardware details in case it becomes
inapplicable for future devices. Instead, focus on the functionality. It
should be something like:

   Support for the LEDs of some Lenovo Legion laptops since 2025, with
   the brightness changes initiated by predefined shortcuts (e.g.,
   Fn+Up/Down) reported via brightness_hw_changed.

> +
> +	Say M here to compile this driver as a module. The module will be called
> +	hid-lenovo-legion.
> +
>  config HID_LETSKETCH
>  	tristate "Letsketch WP9620N tablets"
>  	depends on USB_HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 43a8eb0f389b..713bb1c89861 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_HID_LCPOWER)	+= hid-lcpower.o
>  obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
>  obj-$(CONFIG_HID_LENOVO_GO)	+= hid-lenovo-go.o
>  obj-$(CONFIG_HID_LENOVO_GO_S)	+= hid-lenovo-go-s.o
> +obj-$(CONFIG_HID_LENOVO_LEGION)	+= hid-lenovo-legion.o
>  obj-$(CONFIG_HID_LETSKETCH)	+= hid-letsketch.o
>  obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
>  obj-$(CONFIG_HID_LOGITECH)	+= hid-lg-g15.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 91319aa6516d..992b9b147e95 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -773,6 +773,7 @@
>  #define USB_DEVICE_ID_ITE_LENOVO_YOGA2  0x8350
>  #define I2C_DEVICE_ID_ITE_LENOVO_LEGION_Y720	0x837a
>  #define USB_DEVICE_ID_ITE_LENOVO_YOGA900	0x8396
> +#define USB_DEVICE_ID_ITE_LENOVO_LEGION_GEN10_KBD	0xc197
>  #define I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_KEYBOARD	0x8987
>  #define I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_G11_KEYBOARD	0x83db
>  #define USB_DEVICE_ID_ITE8595		0x8595
> diff --git a/drivers/hid/hid-lenovo-legion.c b/drivers/hid/hid-lenovo-legion.c
> new file mode 100644
> index 000000000000..6d0b4adc6f49
> --- /dev/null
> +++ b/drivers/hid/hid-lenovo-legion.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HID driver for the keyboard backlight of Lenovo Legion laptops (2025).
> + *
> + * The ITE 8258 keyboard controller (048d:c197) is the built-in keyboard and
> + * carries Lenovo's vendor lighting protocol: feature report 0x07, 960 bytes,
> + * a request (struct legion_msg) answered by reading the report back with a
> + * GET_FEATURE once the controller has prepared it (up to ~50 ms).
> + *
> + * The backlight brightness is op 0xCE (set) / 0xCD (get), 0..9, stored in the
> + * controller; it applies to every LED of the machine (keys, front strip, rear
> + * vents, lid logo). The firmware handles Fn+Up / Fn+Down itself and reports
> + * the new level on vendor input report 5 (struct legion_event).
> + *
> + * The colours are the controller's own effects. The LEDs can also be handed
> + * to the HID LampArray of the "Lenovo Lighting" device (048d:c193), which
> + * this driver leaves to others.
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/compiler.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/leds.h>
> +#include <linux/lockdep.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#include <asm/byteorder.h>
> +
> +#include "hid-ids.h"
> +
> +#define LEGION_REPORT_ID		0x07
> +#define LEGION_REPORT_SIZE		960

Move it below struct legion_msg {}...

> +#define LEGION_ANSWER_DELAY_MS		100
> +
> +#define LEGION_OP_GET_BRIGHTNESS	0xcd
> +#define LEGION_OP_SET_BRIGHTNESS	0xce
> +#define LEGION_MAX_BRIGHTNESS		9
> +
> +#define LEGION_EVENT_REPORT_ID		0x05
> +#define LEGION_EVENT_BRIGHTNESS_UP	0x02
> +#define LEGION_EVENT_BRIGHTNESS_DOWN	0x03
> +
> +/* Request, and its answer: the value is params[0]. */
> +struct legion_msg {
> +	u8 report_id;
> +	u8 op;
> +	__le16 len;			/* of params */
> +	u8 params[];
> +} __packed;

#define LEGION_REPORT_PARAM_SIZE	956
#define LEGION_REPORT_SIZE		960

static_assert(struct_size_t(struct legion_msg, params, LEGION_REPORT_PARAM_SIZE) ==
	      LEGION_REPORT_SIZE);

> +
> +struct legion_event {
> +	u8 report_id;
> +	u8 event;
> +	u8 level;			/* brightness after the change */
> +} __packed;
> +
> +struct legion_kbd {
> +	struct hid_device *hdev;
> +	struct mutex lock;		/* one request/answer exchange at a time */
> +	struct legion_msg *msg;		/* LEGION_REPORT_SIZE, DMA-safe */
> +	struct led_classdev led;
> +};
> +
> +/*
> + * Send one request. When @answer is non-NULL, wait for the controller to
> + * prepare its answer and return its value there.
> + */
> +static int legion_exchange(struct legion_kbd *kbd, u8 op, const u8 *params,
> +			   u16 len, u8 *answer)

Weird naming. Why not call it legion_raw_command()?

> +{
> +	struct hid_device *hdev = kbd->hdev;
> +	struct legion_msg *msg = kbd->msg;
> +	int ret;
> +
> +	lockdep_assert_held(&kbd->lock);

if (len > LEGION_REPORT_PARAM_SIZE)
	...

> +
> +	memset(msg, 0, LEGION_REPORT_SIZE);
> +	msg->report_id = LEGION_REPORT_ID;
> +	msg->op = op;
> +	msg->len = cpu_to_le16(len);
> +	memcpy(msg->params, params, len);
> +
> +	ret = hid_hw_raw_request(hdev, LEGION_REPORT_ID, (u8 *)msg,
> +				 LEGION_REPORT_SIZE, HID_FEATURE_REPORT,
> +				 HID_REQ_SET_REPORT);

I wonder, what if we set the report with a short report size, say,
struct_size(msg, params, len)? Could the device handle it properly in
this case? If so, there is a chance that we don't need to sleep for 100ms
later (I guess the device is processing too much data, causing the
massive delay).

> +	if (ret < 0)
> +		return ret;
> +	if (!answer)
> +		return 0;
> +
> +	msleep(LEGION_ANSWER_DELAY_MS);

What a cursed device! 100ms sounds quite horrible.

Fortunately this code path is unreachable in brightness_set_blocking(),
or else we could be in trouble (holding triggers_list_lock for too long)
when removing a trigger.

This is just my random thought. Nothing needs to be fixed here.

> +
> +	memset(msg, 0, LEGION_REPORT_SIZE);
> +	ret = hid_hw_raw_request(hdev, LEGION_REPORT_ID, (u8 *)msg,
> +				 LEGION_REPORT_SIZE, HID_FEATURE_REPORT,
> +				 HID_REQ_GET_REPORT);
> +	if (ret < 0)
> +		return ret;
> +	if (ret <= (int)sizeof(*msg) || msg->report_id != LEGION_REPORT_ID ||
> +	    msg->op != op) {
> +		hid_warn(hdev, "unexpected answer to 0x%02x: %*ph\n", op,
> +			 min(ret, 8), msg);
> +		return -EIO;
> +	}
> +
> +	*answer = msg->params[0];

Ditto, what if we get the report with struct_size(msg, params, 1)?

> +	return 0;
> +}

Since it's frequently used with len==1, let's add a additional wrapper
like:

   static int legion_simple_command(struct legion_kbd *kbd, u8 op, u8 param, u8 *answer)

so that we can simplify the following code.

> +
> +static int legion_brightness_set(struct led_classdev *led,
> +				 enum led_brightness value)
> +{
> +	struct legion_kbd *kbd = container_of(led, struct legion_kbd, led);
> +	u8 level = min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);

An insane value should cause -EINVAL. Why clamp it?

Besides, don't check LEGION_MAX_BRIGHTNESS, but check led->max_brightness
instead. The latter is scalable when adding support for other devices.

> +
> +	guard(mutex)(&kbd->lock);
> +	return legion_exchange(kbd, LEGION_OP_SET_BRIGHTNESS, &level,
> +			       sizeof(level), NULL);
> +}
> +
> +static enum led_brightness legion_brightness_get(struct led_classdev *led)
> +{
> +	struct legion_kbd *kbd = container_of(led, struct legion_kbd, led);
> +	u8 zero = 0, value;
> +	int ret;
> +
> +	guard(mutex)(&kbd->lock);
> +	ret = legion_exchange(kbd, LEGION_OP_GET_BRIGHTNESS, &zero,
> +			      sizeof(zero), &value);
> +	if (ret)
> +		return led->brightness;	/* keep the last known level */

Please return the error as is. The LED core has already done the same
thing for you.

> +
> +	return min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);

An insane value should cause an error, too.

Meanwhile, I'd prefer to add a dev_warn() when ret is an error or when
value is out of range, so that we can identify faulty devices.

> +}
> +
> +static int legion_raw_event(struct hid_device *hdev, struct hid_report *report,
> +			    u8 *data, int size)
> +{
> +	const struct legion_event *ev = (const struct legion_event *)data;
> +	struct legion_kbd *kbd = hid_get_drvdata(hdev);
> +	unsigned int level;
> +
> +	if (size < (int)sizeof(*ev) || ev->report_id != LEGION_EVENT_REPORT_ID)
> +		return 0;
> +
> +	if (ev->event != LEGION_EVENT_BRIGHTNESS_UP &&
> +	    ev->event != LEGION_EVENT_BRIGHTNESS_DOWN)
> +		return 0;
> +
> +	/*
> +	 * hid-core calls this only after probe() has returned and not once
> +	 * remove() has started, so the LED is registered. The controller has
> +	 * already changed the level: keep led_cdev.brightness in step with it.
> +	 */
> +	level = min_t(unsigned int, ev->level, LEGION_MAX_BRIGHTNESS);

Don't clamp. Either ignore insane values silently or add a dev_warn(),
then return early.

> +	WRITE_ONCE(kbd->led.brightness, level);

Usually this is needless as long as the driver provides brightness_get().

> +	led_classdev_notify_brightness_hw_changed(&kbd->led, level);
> +
> +	return 0;	/* let hid-core handle the report as usual */
> +}
> +
> +static int legion_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct legion_kbd *kbd;
> +	int ret;
> +
> +	/* As hid-generic: one input device per application collection. */

Typical LLM tautology. Please explain why the driver needs this instead
of repeating what the flag does.

> +	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return ret;
> +
> +	kbd = devm_kzalloc_obj(&hdev->dev, *kbd);
> +	if (!kbd)
> +		return -ENOMEM;
> +
> +	kbd->msg = devm_kzalloc(&hdev->dev, LEGION_REPORT_SIZE, GFP_KERNEL);
> +	if (!kbd->msg)
> +		return -ENOMEM;
> +
> +	ret = devm_mutex_init(&hdev->dev, &kbd->lock);
> +	if (ret)
> +		return ret;
> +
> +	kbd->hdev = hdev;
> +	hid_set_drvdata(hdev, kbd);
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret)
> +		return ret;
> +
> +	/* Keep the interrupt endpoint polled so hotkey reports always arrive. */
> +	ret = hid_hw_open(hdev);
> +	if (ret)
> +		goto err_stop;
> +
> +	kbd->led.name = "legion::kbd_backlight";
> +	kbd->led.max_brightness = LEGION_MAX_BRIGHTNESS;
> +	kbd->led.brightness_set_blocking = legion_brightness_set;
> +	kbd->led.brightness_get = legion_brightness_get;
> +	/*
> +	 * LED_RETAIN_AT_SHUTDOWN: unregistering (unload, unbind) must not
> +	 * switch the backlight off, the controller would store 0.
> +	 * LED_BRIGHT_HW_CHANGED: Fn+Up/Down are reported to userspace.
> +	 */

Usually you don't need the comment. The intent is very obvious.

> +	kbd->led.flags = LED_RETAIN_AT_SHUTDOWN | LED_BRIGHT_HW_CHANGED;
> +
> +	/*
> +	 * Not devm: the LED must be gone before remove() stops the device,
> +	 * devm would release it only after that.
> +	 */

Why? The comment doesn't answer anything at all.

A lot of HID drivers have been using devm to manage LEDs for long, and
there are also some drivers manually call led_classdev_unregister() after
hid_hw_stop().

> +	ret = led_classdev_register(&hdev->dev, &kbd->led);
> +	if (ret)
> +		goto err_close;
> +
> +	return 0;
> +
> +err_close:
> +	hid_hw_close(hdev);
> +err_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static void legion_remove(struct hid_device *hdev)
> +{
> +	struct legion_kbd *kbd = hid_get_drvdata(hdev);
> +
> +	/* hid-core no longer calls legion_raw_event(): the LED can go. */
> +	led_classdev_unregister(&kbd->led);
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id legion_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE_LENOVO_LEGION_GEN10_KBD) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, legion_devices);
> +
> +static struct hid_driver legion_driver = {
> +	.name = "hid-lenovo-legion",
> +	.id_table = legion_devices,
> +	.probe = legion_probe,
> +	.remove = legion_remove,
> +	.raw_event = legion_raw_event,
> +};
> +module_hid_driver(legion_driver);
> +
> +MODULE_AUTHOR("Ciro Pedrini <ciro.pedrini@gmail.com>");
> +MODULE_DESCRIPTION("Keyboard backlight of Lenovo Legion laptops (2025)");

s/ (2025)//

Thanks,
Rong

> +MODULE_LICENSE("GPL");

Re: [PATCH v2 2/2] HID: lenovo-legion: Add keyboard backlight driver for Legion Gen 10

[Ilpo Järvinen]

On Tue, 10 Nov 2026, Ciro Pedrini via B4 Relay wrote:

> From: Ciro Pedrini <ciro.pedrini@gmail.com>
> 
> The keyboard backlight of the Lenovo Legion Pro 7 16IAX10H (2025) has no
> Linux support: on the test unit nothing ever lit, because the keyboard
> controller's brightness was 0 from the factory and nothing exposes it.
> 
> The ITE 8258 keyboard controller (048d:c197) is the built-in keyboard and
> carries a vendor protocol on feature report 0x07 (960 bytes): a request,
> answered by reading the report back once the controller has prepared it.
> Its hardware brightness, 0..9, is stored in the controller and applies to
> every LED of the machine (keys, front strip, rear vents, lid logo). The
> firmware handles Fn+Up and Fn+Down itself and reports the new level on
> vendor input report 5.
> 
> Expose the brightness as a kbd_backlight LED, legion::kbd_backlight, with
> brightness_hw_changed for the hotkeys so that desktops show their OSD.
> Unregistering keeps the level (LED_RETAIN_AT_SHUTDOWN): switching it off
> would be stored by the controller.
> 
> The colours stay the controller's own effects, which Fn+Space cycles. The
> machine's HID LampArray (048d:c193) is left to hid-generic, or to a
> generic LampArray driver.
> 
> Tested on the 16IAX10H (83F5) with 7.2 as a module: brightness from sysfs
> and KDE, the hotkeys with KDE's OSD, suspend/resume, unbinding and
> rebinding, and a userspace LampArray client alongside the driver.
> 
> Signed-off-by: Ciro Pedrini <ciro.pedrini@gmail.com>

Thanks for the update, this one looks much cleaner already. :-)

> ---
> Prepared with Claude Code (Claude Opus 5.5) from measurements on the
> laptop (usbmon captures) and analysis of the vendor's Windows software
> for interoperability; I reviewed and tested it.
> ---
>  MAINTAINERS                     |   1 +
>  drivers/hid/Kconfig             |  13 ++
>  drivers/hid/Makefile            |   1 +
>  drivers/hid/hid-ids.h           |   1 +
>  drivers/hid/hid-lenovo-legion.c | 261 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 277 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 222b7fc2c3aa..843906a1f9d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15187,6 +15187,7 @@ F:	Documentation/ABI/testing/sysfs-driver-hid-lenovo-go
>  F:	Documentation/ABI/testing/sysfs-driver-hid-lenovo-go-s
>  F:	drivers/hid/hid-lenovo-go-s.c
>  F:	drivers/hid/hid-lenovo-go.c
> +F:	drivers/hid/hid-lenovo-legion.c
>  F:	drivers/hid/hid-lenovo.c
>  
>  LENOVO YOGA BOOK 9 KEYBOARD DOCK DRIVER
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 2481bc01c435..55d25afddb69 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -652,6 +652,19 @@ config HID_LENOVO_GO_S
>  	S. Say M here to compile this driver as a module. The module will be called
>  	hid-lenovo-go-s.
>  
> +config HID_LENOVO_LEGION
> +	tristate "Lenovo Legion laptop keyboard backlight"
> +	depends on USB_HID
> +	depends on LEDS_CLASS
> +	help
> +	Support for the keyboard backlight of 2025 Lenovo Legion laptops
> +	(Legion Pro 7 Gen 10): the hardware brightness of the ITE keyboard
> +	controller as a kbd_backlight LED, with the Fn+Up/Down changes
> +	reported to userspace.
> +
> +	Say M here to compile this driver as a module. The module will be called
> +	hid-lenovo-legion.
> +
>  config HID_LETSKETCH
>  	tristate "Letsketch WP9620N tablets"
>  	depends on USB_HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 43a8eb0f389b..713bb1c89861 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_HID_LCPOWER)	+= hid-lcpower.o
>  obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
>  obj-$(CONFIG_HID_LENOVO_GO)	+= hid-lenovo-go.o
>  obj-$(CONFIG_HID_LENOVO_GO_S)	+= hid-lenovo-go-s.o
> +obj-$(CONFIG_HID_LENOVO_LEGION)	+= hid-lenovo-legion.o
>  obj-$(CONFIG_HID_LETSKETCH)	+= hid-letsketch.o
>  obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
>  obj-$(CONFIG_HID_LOGITECH)	+= hid-lg-g15.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 91319aa6516d..992b9b147e95 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -773,6 +773,7 @@
>  #define USB_DEVICE_ID_ITE_LENOVO_YOGA2  0x8350
>  #define I2C_DEVICE_ID_ITE_LENOVO_LEGION_Y720	0x837a
>  #define USB_DEVICE_ID_ITE_LENOVO_YOGA900	0x8396
> +#define USB_DEVICE_ID_ITE_LENOVO_LEGION_GEN10_KBD	0xc197
>  #define I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_KEYBOARD	0x8987
>  #define I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_G11_KEYBOARD	0x83db
>  #define USB_DEVICE_ID_ITE8595		0x8595
> diff --git a/drivers/hid/hid-lenovo-legion.c b/drivers/hid/hid-lenovo-legion.c
> new file mode 100644
> index 000000000000..6d0b4adc6f49
> --- /dev/null
> +++ b/drivers/hid/hid-lenovo-legion.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * HID driver for the keyboard backlight of Lenovo Legion laptops (2025).
> + *
> + * The ITE 8258 keyboard controller (048d:c197) is the built-in keyboard and
> + * carries Lenovo's vendor lighting protocol: feature report 0x07, 960 bytes,
> + * a request (struct legion_msg) answered by reading the report back with a
> + * GET_FEATURE once the controller has prepared it (up to ~50 ms).
> + *
> + * The backlight brightness is op 0xCE (set) / 0xCD (get), 0..9, stored in the
> + * controller; it applies to every LED of the machine (keys, front strip, rear
> + * vents, lid logo). The firmware handles Fn+Up / Fn+Down itself and reports
> + * the new level on vendor input report 5 (struct legion_event).
> + *
> + * The colours are the controller's own effects. The LEDs can also be handed
> + * to the HID LampArray of the "Lenovo Lighting" device (048d:c193), which
> + * this driver leaves to others.
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/compiler.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/leds.h>
> +#include <linux/lockdep.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#include <asm/byteorder.h>
> +
> +#include "hid-ids.h"
> +
> +#define LEGION_REPORT_ID		0x07
> +#define LEGION_REPORT_SIZE		960
> +#define LEGION_ANSWER_DELAY_MS		100
> +
> +#define LEGION_OP_GET_BRIGHTNESS	0xcd
> +#define LEGION_OP_SET_BRIGHTNESS	0xce
> +#define LEGION_MAX_BRIGHTNESS		9
> +
> +#define LEGION_EVENT_REPORT_ID		0x05
> +#define LEGION_EVENT_BRIGHTNESS_UP	0x02
> +#define LEGION_EVENT_BRIGHTNESS_DOWN	0x03
> +
> +/* Request, and its answer: the value is params[0]. */
> +struct legion_msg {
> +	u8 report_id;
> +	u8 op;
> +	__le16 len;			/* of params */
> +	u8 params[];
> +} __packed;
> +
> +struct legion_event {
> +	u8 report_id;
> +	u8 event;
> +	u8 level;			/* brightness after the change */
> +} __packed;
> +
> +struct legion_kbd {
> +	struct hid_device *hdev;
> +	struct mutex lock;		/* one request/answer exchange at a time */
> +	struct legion_msg *msg;		/* LEGION_REPORT_SIZE, DMA-safe */
> +	struct led_classdev led;
> +};
> +
> +/*
> + * Send one request. When @answer is non-NULL, wait for the controller to
> + * prepare its answer and return its value there.
> + */
> +static int legion_exchange(struct legion_kbd *kbd, u8 op, const u8 *params,
> +			   u16 len, u8 *answer)
> +{
> +	struct hid_device *hdev = kbd->hdev;
> +	struct legion_msg *msg = kbd->msg;
> +	int ret;
> +
> +	lockdep_assert_held(&kbd->lock);
> +
> +	memset(msg, 0, LEGION_REPORT_SIZE);
> +	msg->report_id = LEGION_REPORT_ID;
> +	msg->op = op;
> +	msg->len = cpu_to_le16(len);
> +	memcpy(msg->params, params, len);
> +
> +	ret = hid_hw_raw_request(hdev, LEGION_REPORT_ID, (u8 *)msg,
> +				 LEGION_REPORT_SIZE, HID_FEATURE_REPORT,
> +				 HID_REQ_SET_REPORT);
> +	if (ret < 0)
> +		return ret;
> +	if (!answer)
> +		return 0;
> +
> +	msleep(LEGION_ANSWER_DELAY_MS);
> +
> +	memset(msg, 0, LEGION_REPORT_SIZE);
> +	ret = hid_hw_raw_request(hdev, LEGION_REPORT_ID, (u8 *)msg,
> +				 LEGION_REPORT_SIZE, HID_FEATURE_REPORT,
> +				 HID_REQ_GET_REPORT);
> +	if (ret < 0)
> +		return ret;
> +	if (ret <= (int)sizeof(*msg) || msg->report_id != LEGION_REPORT_ID ||

Just remove this cast. It was probably added by llm and doesn't look 
helpful to make those types compatible, everyone even remotely familiar 
with the code will know sizeof() for that struct fits to int anyway.

> +	    msg->op != op) {
> +		hid_warn(hdev, "unexpected answer to 0x%02x: %*ph\n", op,
> +			 min(ret, 8), msg);

Printing data like this looks more like debug level stuff. I don't 
see benefit for normal user from it (non-debug level messages are for 
consumption by normal users so code and too cryptic stuff should be 
avoided).

> +		return -EIO;
> +	}
> +
> +	*answer = msg->params[0];
> +	return 0;
> +}
> +
> +static int legion_brightness_set(struct led_classdev *led,
> +				 enum led_brightness value)
> +{
> +	struct legion_kbd *kbd = container_of(led, struct legion_kbd, led);
> +	u8 level = min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);
> +
> +	guard(mutex)(&kbd->lock);
> +	return legion_exchange(kbd, LEGION_OP_SET_BRIGHTNESS, &level,
> +			       sizeof(level), NULL);
> +}
> +
> +static enum led_brightness legion_brightness_get(struct led_classdev *led)
> +{
> +	struct legion_kbd *kbd = container_of(led, struct legion_kbd, led);
> +	u8 zero = 0, value;
> +	int ret;
> +
> +	guard(mutex)(&kbd->lock);
> +	ret = legion_exchange(kbd, LEGION_OP_GET_BRIGHTNESS, &zero,
> +			      sizeof(zero), &value);
> +	if (ret)
> +		return led->brightness;	/* keep the last known level */
> +
> +	return min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);
> +}
> +
> +static int legion_raw_event(struct hid_device *hdev, struct hid_report *report,
> +			    u8 *data, int size)
> +{
> +	const struct legion_event *ev = (const struct legion_event *)data;
> +	struct legion_kbd *kbd = hid_get_drvdata(hdev);
> +	unsigned int level;
> +
> +	if (size < (int)sizeof(*ev) || ev->report_id != LEGION_EVENT_REPORT_ID)

Remove cast.

> +		return 0;
> +
> +	if (ev->event != LEGION_EVENT_BRIGHTNESS_UP &&
> +	    ev->event != LEGION_EVENT_BRIGHTNESS_DOWN)
> +		return 0;
> +
> +	/*
> +	 * hid-core calls this only after probe() has returned and not once
> +	 * remove() has started, so the LED is registered. The controller has
> +	 * already changed the level: keep led_cdev.brightness in step with it.
> +	 */
> +	level = min_t(unsigned int, ev->level, LEGION_MAX_BRIGHTNESS);
> +	WRITE_ONCE(kbd->led.brightness, level);

Please add include for this. Either rwonce.h or barrier.h, it currently 
comes through compiler.h but I don't see anything that would technically 
require "compiler" to include "rwonce" so it's not a "guaranteed" include 
path in my eye.

> +	led_classdev_notify_brightness_hw_changed(&kbd->led, level);
> +
> +	return 0;	/* let hid-core handle the report as usual */
> +}
> +
> +static int legion_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	struct legion_kbd *kbd;
> +	int ret;
> +
> +	/* As hid-generic: one input device per application collection. */
> +	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> +
> +	ret = hid_parse(hdev);
> +	if (ret)
> +		return ret;
> +
> +	kbd = devm_kzalloc_obj(&hdev->dev, *kbd);
> +	if (!kbd)
> +		return -ENOMEM;
> +
> +	kbd->msg = devm_kzalloc(&hdev->dev, LEGION_REPORT_SIZE, GFP_KERNEL);
> +	if (!kbd->msg)
> +		return -ENOMEM;
> +
> +	ret = devm_mutex_init(&hdev->dev, &kbd->lock);
> +	if (ret)
> +		return ret;
> +
> +	kbd->hdev = hdev;
> +	hid_set_drvdata(hdev, kbd);
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret)
> +		return ret;
> +
> +	/* Keep the interrupt endpoint polled so hotkey reports always arrive. */
> +	ret = hid_hw_open(hdev);
> +	if (ret)
> +		goto err_stop;
> +
> +	kbd->led.name = "legion::kbd_backlight";
> +	kbd->led.max_brightness = LEGION_MAX_BRIGHTNESS;
> +	kbd->led.brightness_set_blocking = legion_brightness_set;
> +	kbd->led.brightness_get = legion_brightness_get;
> +	/*
> +	 * LED_RETAIN_AT_SHUTDOWN: unregistering (unload, unbind) must not
> +	 * switch the backlight off, the controller would store 0.
> +	 * LED_BRIGHT_HW_CHANGED: Fn+Up/Down are reported to userspace.
> +	 */

This only documents what these flags are so why a driver needs such a 
comment?

> +	kbd->led.flags = LED_RETAIN_AT_SHUTDOWN | LED_BRIGHT_HW_CHANGED;
> +
> +	/*
> +	 * Not devm: the LED must be gone before remove() stops the device,
> +	 * devm would release it only after that.
> +	 */

Yeah, mixing devm and non-devm can result in this kind of ordering problem 
(sometimes one can get away with it, e.g., freeing allocated space later 
is normally a non-issue but for other things it does break things like 
this).

But could you solve this with devm_add_action_or_reset() (for hid 
teardown) and use devm for everything?

> +	ret = led_classdev_register(&hdev->dev, &kbd->led);
> +	if (ret)
> +		goto err_close;
> +
> +	return 0;
> +
> +err_close:
> +	hid_hw_close(hdev);
> +err_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static void legion_remove(struct hid_device *hdev)
> +{
> +	struct legion_kbd *kbd = hid_get_drvdata(hdev);
> +
> +	/* hid-core no longer calls legion_raw_event(): the LED can go. */
> +	led_classdev_unregister(&kbd->led);
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id legion_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE_LENOVO_LEGION_GEN10_KBD) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, legion_devices);
> +
> +static struct hid_driver legion_driver = {
> +	.name = "hid-lenovo-legion",
> +	.id_table = legion_devices,
> +	.probe = legion_probe,
> +	.remove = legion_remove,
> +	.raw_event = legion_raw_event,
> +};
> +module_hid_driver(legion_driver);
> +
> +MODULE_AUTHOR("Ciro Pedrini <ciro.pedrini@gmail.com>");
> +MODULE_DESCRIPTION("Keyboard backlight of Lenovo Legion laptops (2025)");

I don't think we should put year into this, it will just become obsolete 
pretty fast like all time bound reference. :-)

IMO it's fine even if this wouldn't cover all "Lenovo Legion" laptops 
without explicitly stating that it only is for part of them.

-- 
 i.

Re: [PATCH v2 2/2] HID: lenovo-legion: Add keyboard backlight driver for Legion Gen 10

[Rong Zhang]

On Wed, 2026-11-11 at 20:09 +0800, Rong Zhang wrote:
> Hi Ciro,
> 
> On Tue, 2026-11-10 at 20:29 +0000, Ciro Pedrini via B4 Relay wrote:
> > From: Ciro Pedrini <ciro.pedrini@gmail.com>
> > 
> > The keyboard backlight of the Lenovo Legion Pro 7 16IAX10H (2025) has no
> > Linux support: on the test unit nothing ever lit, because the keyboard
> > controller's brightness was 0 from the factory and nothing exposes it.
> > 
> > The ITE 8258 keyboard controller (048d:c197) is the built-in keyboard and
> > carries a vendor protocol on feature report 0x07 (960 bytes): a request,
> > answered by reading the report back once the controller has prepared it.
> > Its hardware brightness, 0..9, is stored in the controller and applies to
> > every LED of the machine (keys, front strip, rear vents, lid logo).
> > 
> 
> I don't know if it's appropriate to represent so many things with the
> name "legion::kbd_backlight".
> 
> >  The
> > firmware handles Fn+Up and Fn+Down itself and reports the new level on
> > vendor input report 5.
> > 
> > Expose the brightness as a kbd_backlight LED, legion::kbd_backlight, with
> > brightness_hw_changed for the hotkeys so that desktops show their OSD.
> > Unregistering keeps the level (LED_RETAIN_AT_SHUTDOWN): switching it off
> > would be stored by the controller.
> > 
> > The colours stay the controller's own effects, which Fn+Space cycles. 
> > 
> 
> Just curious, can the software select which effect to use, or listen to
> an event when Fn+Space is pressed?
> 
> > The
> > machine's HID LampArray (048d:c193) is left to hid-generic, or to a
> > generic LampArray driver.
> > 
> > Tested on the 16IAX10H (83F5) with 7.2 as a module: brightness from sysfs
> > and KDE, the hotkeys with KDE's OSD, suspend/resume, unbinding and
> > rebinding, and a userspace LampArray client alongside the driver.
> > 
> > Signed-off-by: Ciro Pedrini <ciro.pedrini@gmail.com>
> > ---
> > Prepared with Claude Code (Claude Opus 5.5) from measurements on the
> > laptop (usbmon captures) and analysis of the vendor's Windows software
> > for interoperability; I reviewed and tested it.
> > ---
> >  MAINTAINERS                     |   1 +
> >  drivers/hid/Kconfig             |  13 ++
> >  drivers/hid/Makefile            |   1 +
> >  drivers/hid/hid-ids.h           |   1 +
> >  drivers/hid/hid-lenovo-legion.c | 261 ++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 277 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 222b7fc2c3aa..843906a1f9d5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15187,6 +15187,7 @@ F:	Documentation/ABI/testing/sysfs-driver-hid-lenovo-go
> >  F:	Documentation/ABI/testing/sysfs-driver-hid-lenovo-go-s
> >  F:	drivers/hid/hid-lenovo-go-s.c
> >  F:	drivers/hid/hid-lenovo-go.c
> > +F:	drivers/hid/hid-lenovo-legion.c
> >  F:	drivers/hid/hid-lenovo.c
> >  
> >  LENOVO YOGA BOOK 9 KEYBOARD DOCK DRIVER
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 2481bc01c435..55d25afddb69 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -652,6 +652,19 @@ config HID_LENOVO_GO_S
> >  	S. Say M here to compile this driver as a module. The module will be called
> >  	hid-lenovo-go-s.
> >  
> > +config HID_LENOVO_LEGION
> > +	tristate "Lenovo Legion laptop keyboard backlight"
> > +	depends on USB_HID
> > +	depends on LEDS_CLASS
> > +	help
> > +	Support for the keyboard backlight of 2025 Lenovo Legion laptops
> > +	(Legion Pro 7 Gen 10): the hardware brightness of the ITE keyboard
> > +	controller as a kbd_backlight LED, with the Fn+Up/Down changes
> > +	reported to userspace.
> 
> Try not to mention too many hardware details in case it becomes
> inapplicable for future devices. Instead, focus on the functionality. It
> should be something like:
> 
>    Support for the LEDs of some Lenovo Legion laptops since 2025, with
>    the brightness changes initiated by predefined shortcuts (e.g.,
>    Fn+Up/Down) reported via brightness_hw_changed.
> 
> > +
> > +	Say M here to compile this driver as a module. The module will be called
> > +	hid-lenovo-legion.
> > +
> >  config HID_LETSKETCH
> >  	tristate "Letsketch WP9620N tablets"
> >  	depends on USB_HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 43a8eb0f389b..713bb1c89861 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -78,6 +78,7 @@ obj-$(CONFIG_HID_LCPOWER)	+= hid-lcpower.o
> >  obj-$(CONFIG_HID_LENOVO)	+= hid-lenovo.o
> >  obj-$(CONFIG_HID_LENOVO_GO)	+= hid-lenovo-go.o
> >  obj-$(CONFIG_HID_LENOVO_GO_S)	+= hid-lenovo-go-s.o
> > +obj-$(CONFIG_HID_LENOVO_LEGION)	+= hid-lenovo-legion.o
> >  obj-$(CONFIG_HID_LETSKETCH)	+= hid-letsketch.o
> >  obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
> >  obj-$(CONFIG_HID_LOGITECH)	+= hid-lg-g15.o
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index 91319aa6516d..992b9b147e95 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -773,6 +773,7 @@
> >  #define USB_DEVICE_ID_ITE_LENOVO_YOGA2  0x8350
> >  #define I2C_DEVICE_ID_ITE_LENOVO_LEGION_Y720	0x837a
> >  #define USB_DEVICE_ID_ITE_LENOVO_YOGA900	0x8396
> > +#define USB_DEVICE_ID_ITE_LENOVO_LEGION_GEN10_KBD	0xc197
> >  #define I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_KEYBOARD	0x8987
> >  #define I2C_DEVICE_ID_ITE_LENOVO_YOGA_SLIM_7X_G11_KEYBOARD	0x83db
> >  #define USB_DEVICE_ID_ITE8595		0x8595
> > diff --git a/drivers/hid/hid-lenovo-legion.c b/drivers/hid/hid-lenovo-legion.c
> > new file mode 100644
> > index 000000000000..6d0b4adc6f49
> > --- /dev/null
> > +++ b/drivers/hid/hid-lenovo-legion.c
> > @@ -0,0 +1,261 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * HID driver for the keyboard backlight of Lenovo Legion laptops (2025).
> > + *
> > + * The ITE 8258 keyboard controller (048d:c197) is the built-in keyboard and
> > + * carries Lenovo's vendor lighting protocol: feature report 0x07, 960 bytes,
> > + * a request (struct legion_msg) answered by reading the report back with a
> > + * GET_FEATURE once the controller has prepared it (up to ~50 ms).
> > + *
> > + * The backlight brightness is op 0xCE (set) / 0xCD (get), 0..9, stored in the
> > + * controller; it applies to every LED of the machine (keys, front strip, rear
> > + * vents, lid logo). The firmware handles Fn+Up / Fn+Down itself and reports
> > + * the new level on vendor input report 5 (struct legion_event).
> > + *
> > + * The colours are the controller's own effects. The LEDs can also be handed
> > + * to the HID LampArray of the "Lenovo Lighting" device (048d:c193), which
> > + * this driver leaves to others.
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/compiler.h>
> > +#include <linux/container_of.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/hid.h>
> > +#include <linux/leds.h>
> > +#include <linux/lockdep.h>
> > +#include <linux/minmax.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +
> > +#include <asm/byteorder.h>
> > +
> > +#include "hid-ids.h"
> > +
> > +#define LEGION_REPORT_ID		0x07
> > +#define LEGION_REPORT_SIZE		960
> 
> Move it below struct legion_msg {}...
> 
> > +#define LEGION_ANSWER_DELAY_MS		100
> > +
> > +#define LEGION_OP_GET_BRIGHTNESS	0xcd
> > +#define LEGION_OP_SET_BRIGHTNESS	0xce
> > +#define LEGION_MAX_BRIGHTNESS		9
> > +
> > +#define LEGION_EVENT_REPORT_ID		0x05
> > +#define LEGION_EVENT_BRIGHTNESS_UP	0x02
> > +#define LEGION_EVENT_BRIGHTNESS_DOWN	0x03
> > +
> > +/* Request, and its answer: the value is params[0]. */
> > +struct legion_msg {
> > +	u8 report_id;
> > +	u8 op;
> > +	__le16 len;			/* of params */
> > +	u8 params[];
> > +} __packed;
> 
> #define LEGION_REPORT_PARAM_SIZE	956
> #define LEGION_REPORT_SIZE		960
> 
> static_assert(struct_size_t(struct legion_msg, params, LEGION_REPORT_PARAM_SIZE) ==
> 	      LEGION_REPORT_SIZE);
> 
> > +
> > +struct legion_event {
> > +	u8 report_id;
> > +	u8 event;
> > +	u8 level;			/* brightness after the change */
> > +} __packed;
> > +
> > +struct legion_kbd {
> > +	struct hid_device *hdev;
> > +	struct mutex lock;		/* one request/answer exchange at a time */
> > +	struct legion_msg *msg;		/* LEGION_REPORT_SIZE, DMA-safe */
> > +	struct led_classdev led;
> > +};
> > +
> > +/*
> > + * Send one request. When @answer is non-NULL, wait for the controller to
> > + * prepare its answer and return its value there.
> > + */
> > +static int legion_exchange(struct legion_kbd *kbd, u8 op, const u8 *params,
> > +			   u16 len, u8 *answer)
> 
> Weird naming. Why not call it legion_raw_command()?
> 
> > +{
> > +	struct hid_device *hdev = kbd->hdev;
> > +	struct legion_msg *msg = kbd->msg;
> > +	int ret;
> > +
> > +	lockdep_assert_held(&kbd->lock);
> 
> if (len > LEGION_REPORT_PARAM_SIZE)
> 	...
> 
> > +
> > +	memset(msg, 0, LEGION_REPORT_SIZE);
> > +	msg->report_id = LEGION_REPORT_ID;
> > +	msg->op = op;
> > +	msg->len = cpu_to_le16(len);
> > +	memcpy(msg->params, params, len);
> > +
> > +	ret = hid_hw_raw_request(hdev, LEGION_REPORT_ID, (u8 *)msg,
> > +				 LEGION_REPORT_SIZE, HID_FEATURE_REPORT,
> > +				 HID_REQ_SET_REPORT);
> 
> I wonder, what if we set the report with a short report size, say,
> struct_size(msg, params, len)? Could the device handle it properly in
> this case? If so, there is a chance that we don't need to sleep for 100ms
> later (I guess the device is processing too much data, causing the
> massive delay).
> 
> > +	if (ret < 0)
> > +		return ret;
> > +	if (!answer)
> > +		return 0;
> > +
> > +	msleep(LEGION_ANSWER_DELAY_MS);
> 
> What a cursed device! 100ms sounds quite horrible.
> 
> Fortunately this code path is unreachable in brightness_set_blocking(),
> or else we could be in trouble (holding triggers_list_lock for too long)
> when removing a trigger.
> 
> This is just my random thought. Nothing needs to be fixed here.
> 
> > +
> > +	memset(msg, 0, LEGION_REPORT_SIZE);
> > +	ret = hid_hw_raw_request(hdev, LEGION_REPORT_ID, (u8 *)msg,
> > +				 LEGION_REPORT_SIZE, HID_FEATURE_REPORT,
> > +				 HID_REQ_GET_REPORT);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret <= (int)sizeof(*msg) || msg->report_id != LEGION_REPORT_ID ||
> > +	    msg->op != op) {
> > +		hid_warn(hdev, "unexpected answer to 0x%02x: %*ph\n", op,
> > +			 min(ret, 8), msg);
> > +		return -EIO;
> > +	}
> > +
> > +	*answer = msg->params[0];
> 
> Ditto, what if we get the report with struct_size(msg, params, 1)?
> 
> > +	return 0;
> > +}
> 
> Since it's frequently used with len==1, let's add a additional wrapper
> like:
> 
>    static int legion_simple_command(struct legion_kbd *kbd, u8 op, u8 param, u8 *answer)
> 
> so that we can simplify the following code.
> 
> > +
> > +static int legion_brightness_set(struct led_classdev *led,
> > +				 enum led_brightness value)
> > +{
> > +	struct legion_kbd *kbd = container_of(led, struct legion_kbd, led);
> > +	u8 level = min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);
> 
> An insane value should cause -EINVAL. Why clamp it?
> 
> Besides, don't check LEGION_MAX_BRIGHTNESS, but check led->max_brightness
> instead. The latter is scalable when adding support for other devices.
> 
> > +
> > +	guard(mutex)(&kbd->lock);
> > +	return legion_exchange(kbd, LEGION_OP_SET_BRIGHTNESS, &level,
> > +			       sizeof(level), NULL);
> > +}
> > +
> > +static enum led_brightness legion_brightness_get(struct led_classdev *led)
> > +{
> > +	struct legion_kbd *kbd = container_of(led, struct legion_kbd, led);
> > +	u8 zero = 0, value;
> > +	int ret;
> > +
> > +	guard(mutex)(&kbd->lock);
> > +	ret = legion_exchange(kbd, LEGION_OP_GET_BRIGHTNESS, &zero,
> > +			      sizeof(zero), &value);
> > +	if (ret)
> > +		return led->brightness;	/* keep the last known level */
> 
> Please return the error as is. The LED core has already done the same
> thing for you.
> 
> > +
> > +	return min_t(unsigned int, value, LEGION_MAX_BRIGHTNESS);
> 
> An insane value should cause an error, too.
> 
> Meanwhile, I'd prefer to add a dev_warn() when ret is an error or when
> value is out of range, so that we can identify faulty devices.

Or dev_dbg() if you prefer to make it less verbose. The whole point of
adding dev_printk here is that the LED core sometimes ignores errors and
prevents it from reaching userspace.

> 
> > +}
> > +
> > +static int legion_raw_event(struct hid_device *hdev, struct hid_report *report,
> > +			    u8 *data, int size)
> > +{
> > +	const struct legion_event *ev = (const struct legion_event *)data;
> > +	struct legion_kbd *kbd = hid_get_drvdata(hdev);
> > +	unsigned int level;
> > +
> > +	if (size < (int)sizeof(*ev) || ev->report_id != LEGION_EVENT_REPORT_ID)
> > +		return 0;
> > +
> > +	if (ev->event != LEGION_EVENT_BRIGHTNESS_UP &&
> > +	    ev->event != LEGION_EVENT_BRIGHTNESS_DOWN)
> > +		return 0;
> > +
> > +	/*
> > +	 * hid-core calls this only after probe() has returned and not once
> > +	 * remove() has started, so the LED is registered. The controller has
> > +	 * already changed the level: keep led_cdev.brightness in step with it.
> > +	 */
> > +	level = min_t(unsigned int, ev->level, LEGION_MAX_BRIGHTNESS);
> 
> Don't clamp. Either ignore insane values silently or add a dev_warn(),
> then return early.

Ditto.

Thanks,
Rong

> 
> > +	WRITE_ONCE(kbd->led.brightness, level);
> 
> Usually this is needless as long as the driver provides brightness_get().
> 
> > +	led_classdev_notify_brightness_hw_changed(&kbd->led, level);
> > +
> > +	return 0;	/* let hid-core handle the report as usual */
> > +}
> > +
> > +static int legion_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > +{
> > +	struct legion_kbd *kbd;
> > +	int ret;
> > +
> > +	/* As hid-generic: one input device per application collection. */
> 
> Typical LLM tautology. Please explain why the driver needs this instead
> of repeating what the flag does.
> 
> > +	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> > +
> > +	ret = hid_parse(hdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	kbd = devm_kzalloc_obj(&hdev->dev, *kbd);
> > +	if (!kbd)
> > +		return -ENOMEM;
> > +
> > +	kbd->msg = devm_kzalloc(&hdev->dev, LEGION_REPORT_SIZE, GFP_KERNEL);
> > +	if (!kbd->msg)
> > +		return -ENOMEM;
> > +
> > +	ret = devm_mutex_init(&hdev->dev, &kbd->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	kbd->hdev = hdev;
> > +	hid_set_drvdata(hdev, kbd);
> > +
> > +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Keep the interrupt endpoint polled so hotkey reports always arrive. */
> > +	ret = hid_hw_open(hdev);
> > +	if (ret)
> > +		goto err_stop;
> > +
> > +	kbd->led.name = "legion::kbd_backlight";
> > +	kbd->led.max_brightness = LEGION_MAX_BRIGHTNESS;
> > +	kbd->led.brightness_set_blocking = legion_brightness_set;
> > +	kbd->led.brightness_get = legion_brightness_get;
> > +	/*
> > +	 * LED_RETAIN_AT_SHUTDOWN: unregistering (unload, unbind) must not
> > +	 * switch the backlight off, the controller would store 0.
> > +	 * LED_BRIGHT_HW_CHANGED: Fn+Up/Down are reported to userspace.
> > +	 */
> 
> Usually you don't need the comment. The intent is very obvious.
> 
> > +	kbd->led.flags = LED_RETAIN_AT_SHUTDOWN | LED_BRIGHT_HW_CHANGED;
> > +
> > +	/*
> > +	 * Not devm: the LED must be gone before remove() stops the device,
> > +	 * devm would release it only after that.
> > +	 */
> 
> Why? The comment doesn't answer anything at all.
> 
> A lot of HID drivers have been using devm to manage LEDs for long, and
> there are also some drivers manually call led_classdev_unregister() after
> hid_hw_stop().
> 
> > +	ret = led_classdev_register(&hdev->dev, &kbd->led);
> > +	if (ret)
> > +		goto err_close;
> > +
> > +	return 0;
> > +
> > +err_close:
> > +	hid_hw_close(hdev);
> > +err_stop:
> > +	hid_hw_stop(hdev);
> > +	return ret;
> > +}
> > +
> > +static void legion_remove(struct hid_device *hdev)
> > +{
> > +	struct legion_kbd *kbd = hid_get_drvdata(hdev);
> > +
> > +	/* hid-core no longer calls legion_raw_event(): the LED can go. */
> > +	led_classdev_unregister(&kbd->led);
> > +	hid_hw_close(hdev);
> > +	hid_hw_stop(hdev);
> > +}
> > +
> > +static const struct hid_device_id legion_devices[] = {
> > +	{ HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE_LENOVO_LEGION_GEN10_KBD) },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(hid, legion_devices);
> > +
> > +static struct hid_driver legion_driver = {
> > +	.name = "hid-lenovo-legion",
> > +	.id_table = legion_devices,
> > +	.probe = legion_probe,
> > +	.remove = legion_remove,
> > +	.raw_event = legion_raw_event,
> > +};
> > +module_hid_driver(legion_driver);
> > +
> > +MODULE_AUTHOR("Ciro Pedrini <ciro.pedrini@gmail.com>");
> > +MODULE_DESCRIPTION("Keyboard backlight of Lenovo Legion laptops (2025)");
> 
> s/ (2025)//
> 
> Thanks,
> Rong
> 
> > +MODULE_LICENSE("GPL");