Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

[Hans de Goede <hansg@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

Hi Leo,

On 3-Sep-25 9:39 PM, Leo L. Schwab wrote:
> 	For some reason, your replies aren't making it to me directly -- I
> had to find and scrape your reply off the LKML web site:
> 
> On Tue, 2 Sep 2025 23:05:06 +0200, Hans de Goede wrote:
>> On 2-Sep-25 22:41, Leo L. Schwab wrote:
>>> 	This does not happen.  The G13 accepts and remembers backlight color
>>> settings even when the LEDs have been toggled off locally.
>>> [ ... ]
>>
>> I see, interesting.
>>
>> So what happens if you turn off the backlight with the toggle button on the G13
>> and then write 0 to brightness in sysfs and then press the toggle button again?
>>
> 	It's a little difficult to see, but the backlight turns back on with
> minimal brightness.  To my eye, it looks like it's displaying #000001.

Ok.

>> Right it does seem that using cdev.brightness_hw_changed is valid in
>> this case.
>>
>> But the LED API is supposed to have the brightness attribute present
>> the actual current brightness of the device.
>>
>> I'm not sure how upower will react if the poll() on brightness_hw_changed
>> wakes upower up and then the reported brightness is unchanged...
>>
>> I need to think about this a bit and check the upower code, let me
>> get back to you on this in a day or 2 ...
>>
> 	Certainly.

Thank you for waiting. After looking at the upower code + running some
tests with a G510 I think that your hw_brightness_changed support
is pretty good as is.

There are 2 improvements which I would like to see:

1. When the backlight is turned on through the button, you
should pass g15_led->brightness to the notify() call rather
then LED_FULL. GNOME will show an OSD with the new brightness
value shown as a mini progress bar similar to how it shows
speaker volume when doing mute/unmute. This mini progress
bar should show the actual brightness being restored, not
always full brightness.

2. ATM if the backlight is turned off on the G13 when
the driver loads and then one of the buttons gets pressed
then a notify() will happen because the led_cdev.hw_brightness_changed
value of -1 will be different from the value of 0 in the
input-report. This notify will lead to an unwanted OSD
notification in GNOME, so this needs to be fixed.
IMHO the best fix would be to use:

	hid_hw_raw_request(..., HID_INPUT_REPORT, HID_REQ_GET_REPORT);

at probe to get the input-report so that the driver will
actually now the backlight state at probe() time without
needing to wait for the first time the input-report is send.

You have inspired me to add hw_brightness_changed support
to the G510 code, see the attached patch. This patch can
also be used as an example how to get the input report
on the G13 during probe().

Note this also adds a variable at the driver level to
track the backlight state also fixing the compile issue
you hit without needing to use #ifdef-ery.

I'll wait for your G13 support to land first and then
rebase the G510 patch on top.

Regards,

Hans




[-- Attachment #2: 0001-HID-hid-lg-g15-Add-hw_brightness_changed-support-for.patch --]
[-- Type: text/x-patch, Size: 3783 bytes --]

From 1c735e5ddba814acc57a9d268ed7852bd4aa5887 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hansg@kernel.org>
Date: Mon, 8 Sep 2025 22:55:12 +0200
Subject: [PATCH] HID: hid-lg-g15: Add hw_brightness_changed support for the
 G510 keyboard

Add hw_brightness_changed support for the G510 keyboard, so that e.g.
GNOME will show an OSD notification when toggling the backlight on/off
with the button the keyboard.

Note that it is not possible to turn the backlight back on by writing
/sys/class/leds/.../brightness it can only be turned on by pressing
the button on the keyboard. To reflect this /sys/class/leds/.../brightness
will always report the last brightness value independent of the on/off
toggle built into the keyboard.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/hid/hid-lg-g15.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
index f8605656257b..e5be2a5dfa67 100644
--- a/drivers/hid/hid-lg-g15.c
+++ b/drivers/hid/hid-lg-g15.c
@@ -26,6 +26,9 @@
 #define LG_G510_FEATURE_BACKLIGHT_RGB	0x05
 #define LG_G510_FEATURE_POWER_ON_RGB	0x06
 
+#define LG_G510_INPUT_MACRO_KEYS	0x03
+#define LG_G510_INPUT_KBD_BACKLIGHT	0x04
+
 enum lg_g15_model {
 	LG_G15,
 	LG_G15_V2,
@@ -67,6 +70,7 @@ struct lg_g15_data {
 	enum lg_g15_model model;
 	struct lg_g15_led leds[LG_G15_LED_MAX];
 	bool game_mode_enabled;
+	bool backlight_disabled;
 };
 
 /******** G15 and G15 v2 LED functions ********/
@@ -227,6 +231,20 @@ static int lg_g510_get_initial_led_brightness(struct lg_g15_data *g15, int i)
 		g15->leds[i].brightness = 0;
 	}
 
+	if (i)
+		return 0;
+
+	ret = hid_hw_raw_request(g15->hdev, LG_G510_INPUT_KBD_BACKLIGHT,
+				 g15->transfer_buf, 2,
+				 HID_INPUT_REPORT, HID_REQ_GET_REPORT);
+	if (ret != 2) {
+		/* This can happen when a KVM switch is used, so only warn. */
+		hid_warn(g15->hdev, "Error getting backlight state: %d\n", ret);
+		return 0;
+	}
+
+	g15->backlight_disabled = g15->transfer_buf[1] & 0x04;
+
 	return 0;
 }
 
@@ -549,14 +567,24 @@ static int lg_g510_event(struct lg_g15_data *g15, u8 *data)
 
 static int lg_g510_leds_event(struct lg_g15_data *g15, u8 *data)
 {
+	struct lg_g15_led *g15_led = &g15->leds[LG_G15_KBD_BRIGHTNESS];
 	bool backlight_disabled;
 
+	backlight_disabled = data[1] & 0x04;
+	if (backlight_disabled == g15->backlight_disabled)
+		return 0;
+
+	led_classdev_notify_brightness_hw_changed(
+		&g15_led->mcdev.led_cdev,
+		backlight_disabled ? 0 : g15_led->brightness);
+
+	g15->backlight_disabled = backlight_disabled;
+
 	/*
 	 * The G510 ignores backlight updates when the backlight is turned off
 	 * through the light toggle button on the keyboard, to work around this
 	 * we queue a workitem to sync values when the backlight is turned on.
 	 */
-	backlight_disabled = data[1] & 0x04;
 	if (!backlight_disabled)
 		schedule_work(&g15->work);
 
@@ -588,9 +616,9 @@ static int lg_g15_raw_event(struct hid_device *hdev, struct hid_report *report,
 		break;
 	case LG_G510:
 	case LG_G510_USB_AUDIO:
-		if (data[0] == 0x03 && size == 5)
+		if (data[0] == LG_G510_INPUT_MACRO_KEYS && size == 5)
 			return lg_g510_event(g15, data);
-		if (data[0] == 0x04 && size == 2)
+		if (data[0] == LG_G510_INPUT_KBD_BACKLIGHT && size == 2)
 			return lg_g510_leds_event(g15, data);
 		break;
 	}
@@ -624,6 +652,9 @@ static void lg_g15_setup_led_rgb(struct lg_g15_data *g15, int index)
 	g15->leds[index].mcdev.led_cdev.max_brightness = 255;
 	g15->leds[index].mcdev.num_colors = 3;
 
+	if (index == LG_G15_KBD_BRIGHTNESS)
+		g15->leds[index].mcdev.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
+
 	subled_info = devm_kcalloc(&g15->hdev->dev, 3, sizeof(*subled_info), GFP_KERNEL);
 	if (!subled_info)
 		return;
-- 
2.51.0