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

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

Hi,

On 10-Sep-25 7:52 AM, Leo L. Schwab wrote:
> On Mon, Sep 08, 2025 at 11:08:29PM +0200, Hans de Goede wrote:
>> 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.
>>
> 	If g15_led->brightness is subsequently changed, should a new
> notify() call also be made with that new brightness, i.e. should
> `hw_brightness_changed` be made to track `brightness`?

No, hw_brightness_changed only track changes done independently
by the hw. sysfs writes should not call notify().

> Indeed, it looks
> like you do this in `lg_g15_leds_changed_work()`.

That is for the original G15 and G15-v2, where the buttion
cycles through a couple of backlight levels (IIRC). That work
only gets queued when we receive a button press notification
and then it *reads* the new brightness from the keyboard and
uses that for the notify().

That work does not get queued/used for normal sysfs writes.

>> 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.
>>
> 	Will give this a try.
> 
>> I'll wait for your G13 support to land first and then
>> rebase the G510 patch on top.
>>
> 	Roger that.

Regards,

Hans