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

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

Hi Leo,

On 16-Sep-25 00:18, Leo L. Schwab wrote:
> On Wed, Sep 10, 2025 at 09:16:45PM +0200, Hans de Goede wrote:
>> Since the driver writes any new values to the G13 and the G13 accepts
>> those and remembers them even when the backlight is off,
>> the notify() should be passed g15_led->brightness when an
>> off -> on transition happens (and 0 or LED_OFF for the on -> off
>> transition).
>>
>> Since g15_led->brightness gets initialized by reading the actual
>> setting from the G13 at probe time and then gets updated on
>> any successful completion if writing a new brightness value
>> to the G13, it should always reflect the value which the backlight
>> will be set at by the G13 after an off -> on transition.
>>
>> Or am I missing something ?
>>
> 	If I'm understanding you correctly:


> 	You want `brightness` to be copied to `brightness_hw_changed` on
> probe, and on every backlight off->on transition (cool so far).

Just to clarify, there are 2 things here

1: brightness as in the actual rgb/brightness values the backlight will
   be lit with when it is on
2. A backlight_disabled flag to indicate if the backlight is disabled
   in hw by the button on the G13

I would suggest to track those separately by adding a backlight_disabled
(or backlight_enabled) flag to struct lg_g15_data like I do in the
g510 patch I send earlier in the thread.

So wrt copying things on probe() I would copy both the brightness
to g15_led->brightness which is already done in v3 as well as 
use something like:

        ret = hid_hw_raw_request(g15->hdev, LG_G510_INPUT_KBD_BACKLIGHT,
                                 g15->transfer_buf, 2,
                                 HID_INPUT_REPORT, HID_REQ_GET_REPORT);

to get the input-report with the backlight_enabled/disabled flag and
initialize backlight_disabled based on that.

I would not touch mcled.cdev.brightness_hw_changed directly,
not touching this will also avoid the build issues when
support for it is disabled.

Ack to on detecting a backlight off->on transition based on comparing
the input-report bit to the cached backlight_disabled flag pass
the cached g15_led->brightness to notify()

> 	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.

> 	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.

>  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.

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

and that is it, with those changes I believe that we should be
good to go.

Regards,

Hans