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

["Leo L. Schwab" <ewhac@ewhac.org>]

On Wed, Sep 17, 2025 at 12:33:36PM +0200, Hans de Goede wrote:
> On 16-Sep-25 00:18, Leo L. Schwab wrote:
> > 	What do you want to happen to `brightness_hw_changed` when
> > `brightness` is changed in sysfs while the backlight is on?  As it stands,
> > the current behavior is:
> > 	* Driver loads and probes; `brightness` and `brightness_hw_changed`
> > 	  both set to 255.
> 
> Ack, except that as mentioned above I would not touch brightness_hw_changed
> and just leave it at -1.
> 
> > 	* sysfs `brightness` changed to 128.  `brightness_hw_changed`
> > 	  remains at 255.
> > 	* Toggle backilght off.  `brightness_hw_changed` changed to 0.
> > 	  `brightness` remains at 128.
> > 	* Toggle backlight back on.  `brightness_hw_changed` gets a copy of
> > 	  `brightness`, and both are now 128.
> 
> Ack this is all correct.
>
	...Oy.

	Okay, I can give you that.

> > 	This seems inconsistent to me.
> 
> This is working as intended / how the API was designed as
> Documentation/ABI/testing/sysfs-class-led says:
> 
>                 Reading this file will return the last brightness level set
>                 by the hardware, this may be different from the current
>                 brightness. Reading this file when no hw brightness change
>                 event has happened will return an ENODATA error.
>
	First: Why isn't this mentioned in Documentation/leds/leds_class.rst?

	And second: This doesn't really clarify anything.  That paragraph
may be legitimately interpreted to mean that `brightness_hw_changed` is
completely independent of `brightness`, as it was in my original
implementation.

> >  Hence my earlier suggestion that
> > `brightness_hw_changed` should track all changes to `brightness`, except
> > when the backlight is toggled off.
> 
> Then it also would be reporting values coming from sysfs writes,
> which it explicitly should not do.
>
	Okay, fair, but having `brightness_hw_changed` read as 255, then
later as 128 after hitting the toggle button a couple of times strikes me as
inconsistent behavior.

> Summarizing in my view the following changes are necessary on v4:
> 
> 1. Add backlight_disabled (or backlight_enabled) flag to struct lg_g15_data
> 2. Init that flag from prope()
> 3. Use that flag on receiving input reports to see if notify()
>    should be called
> 4. Replace the LED_FULL passed to notify() (for off->on)
>    with g15_led->brightness
> 
	Will do; will post v6 shortly.  And someone should update the docs
describing the expected interaction between `brightness` and
`brightness_hw_changed`.

					Schwab