Message-ID: <aLdWZJwSrpvgXPFL@ewhac.org>
Date: Tue, 2 Sep 2025 13:41:08 -0700
From: "Leo L. Schwab" <ewhac@...ac.org>
To: Hans de Goede <hansg@...nel.org>
Cc: Kate Hsuan <hpa@...hat.com>, Jiri Kosina <jikos@...nel.org>,
	Benjamin Tissoires <bentiss@...nel.org>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] HID: lg-g15 - Add support for Logitech G13.

	Didn't directly receive the intermediate reply:

On Tue, Sep 02, 2025 at 11:14:09AM +0200, Hans de Goede wrote:
> On 2-Sep-25 11:07 AM, Hans de Goede wrote:
> > Ah I see. Yes if you do need to do a CONFIG check then using IS_ENABLED()
> > is good.
> > 
> > But I'm afraid that the underlying problem here is the use of
> > cdev.brightness_hw_changed this is really only meant for led-class.c
> > internal use.
> > 
	Then there should be a comment in the include file to that effect.

> > The idea of cdev.brightness_hw_changed is that it stores the last
> > value set by the hw.
> > 
> > But in the mean time that value may have been overwritten by software.
> > 
> > I think that you will fail to call led_classdev_notify_brightness_hw_changed()
> > (you can add a debug print to check) if the following happens:
> > 
> > 1. Brightness set to 255 (RGB 255,255,255) through sysfs
> > 2. State toggled to off by backlight control button, brightness is now 0
> > 3. Brightness set to 255 (RGB 255,255,255) through sysfs
> > 4. State toggled to off by backlight control button, brightness is now 0
> > 
	This does not happen.  The G13 accepts and remembers backlight color
settings even when the LEDs have been toggled off locally.

```
#### Initial state: Backlight on, full green:
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
255
0 255 0
root:/sys/class/leds/g13:rgb:kbd_backlight# echo 255 0 0 > multi_intensity 
#### Backlight is on, full red.
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
255
255 0 0
#### Backlight toggle button pressed; backlight is now off.
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
0
255 0 0
root:/sys/class/leds/g13:rgb:kbd_backlight# echo 0 0 255 > multi_intensity 
#### Backlight color set to full blue, but is still off.
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
0
0 0 255
#### Backlight toggle button pressed; backlight is now on, and blue.
root:/sys/class/leds/g13:rgb:kbd_backlight# cat brightness brightness_hw_changed multi_intensity 
255
255
0 0 255
```

	This also works if you alter `brightness` while the backlight is
toggled off.  IMHO, this is correct, principle-of-least-surprise behavior.

	Further (at least on my machine), `brightness_hw_changed` is
read-only in sysfs, and therefore can't be altered by host software.
Therefore, it would seem that using `cdev.brightness_hw_changed` as a cache
value is valid.  Otherwise, as you ouline below, we'd have to go through all
the workqueue gymnastics.

> > I also see that you use TEST_BIT(rep->keybits, 23) ? LED_FULL : LED_OFF
> > for the brightness value send to led_classdev_notify_brightness_hw_changed()
> > but I would expect the hw to restore the previous brightness on a toggle
> > from off -> on through the button? So then that should be send.
> > 
> > And you also never update cdev.brightness and use the cached 
> > struct lg_g15_led.brightness in lg_g13_kbd_led_get(). This means that
> > after a hw toggle of the backlight reading the brightness from sysfs
> > will show the wrong (old) value.
> > 
	This prompts the question:  What is the full intensity calculation
formula intended to be?  The docs appear to be rather quiet on this point.
If we assume all intensity/brightness values (0-255) are essentially mapped
to the range [0.0, 1.0], then it seems to me the calculation is:

	out = intensity * brightness * brightness_hw_changed

	i.e., turning either brightness value down to zero will turn the LED
"off" without affecting the other values, so when you turn it back on again,
you'll get back the color/other brightness you started with.  (Corollary:
For software to know the current output value, it must consider all three
input values.)

	I'm further assuming that brightness_hw_changed should have the same
range as brightness, as there is no separate `max_brightness_hw_changed`
sysfs value.

> > I think that instead what you need to do is create a
> > lg_g13_leds_changed_work() mirroring lg_g15_leds_changed_work()

	I dissent.  But then it's entirely possible I'm still missing
something...

	The only edge case I'm immediately aware of is:

	* Plug in G13.
	* Toggle backlight off.
	* Unload kernel module.
	* Reload kernel module.

	The backlight is now toggled off, but the newly loaded driver
doesn't know this.  Attempting to read `brightness_hw_changed` from sysfs at
this point will result in ENODATA (essentially reporting, "I don't know").
AFAIK, there is no way to probe the G13 for the current state of the
backlight HW toggle.  However, the moment the user generates any event on
the G13, the correct state will be obtained, and `brightness_hw_changed`
will be updated accordingly.  Not ideal, but seemed the most honest
approach.

> p.s.
> 
> Hmm, I wonder if this device is maybe more like the G510, where
> once the BL is turned off it simply ignores any updates send
> from the PC?  [ ... ]

	Nope.  As per my experiments above, the G13 accepts and remembers
backlight color updates even when the backlight is locally toggled off.

					Schwab

Powered by blists - more mailing lists