Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change

[Chris Chiu]

On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart <dvhart@xxxxxxxxxxxxx> wrote:
> On Mon, Jun 04, 2018 at 04:23:04PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 04-06-18 15:51, Daniel Drake wrote:
>> > On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>> > > Is this really a case of the hardware itself processing the
>> > > keypress and then changing the brightness *itself* ?
>> > >
>> > >  From the "[PATCH 2/2] platform/x86: asus-wmi: Add keyboard backlight
>> > > toggle support" patch I get the impression that the driver is
>> > > modifying the brightness from within the kernel rather then the
>> > > keyboard controller are ACPI embeddec-controller doing it itself.
>> > >
>> > > If that is the case then the right fix is for the driver to stop
>> > > mucking with the brighness itself, it should simply report the
>> > > right keyboard events and export a led interface and then userspace
>> > > will do the right thing (and be able to offer flexible policies
>> > > to the user).
>> >
>> > Before this modification, the driver reports the brightness keypresses
>> > to userspace and then userspace can respond by changing the brightness
>> > level, as you describe.
>> >
>> > You are right in that the hardware doesn't change the brightness
>> > directly itself, which is the normal usage of LED_BRIGHT_HW_CHANGED.
>> >
>> > However this approach was suggested by Benjamin Berg and Bastien
>> > Nocera in the thread: Re: [PATCH v2] platform/x86: asus-wmi: Add
>> > keyboard backlight toggle support
>> > https://marc.info/?l=linux-kernel&m=152639169210655&w=2
>> >
>> > The issue is that we need to support a new "keyboard backlight
>> > brightness cycle" key (in the patch that follows this one) which
>> > doesn't fit into any definitions of keys recognised by the kernel and
>> > likewise there's no userspace code to handle it.
>> >
>> > If preferred we could leave the standard brightness keys behaving as
>> > they are (input events) and make the new special key type directly
>> > handled by the kernel?
>>
>> I'm sorry that Benjamin and Bastien steered you in this direction,
>> IMHO none of it should be handled in the kernel.
>>
>> Anytime any sort of input is directly responded to by the kernel
>> it is a huge PITA to deal with from userspace. The kernel will have
>> a simplistic implementation which almost always is wrong.
>>
>> Benjamin, remember the pain we went through with rfkill hotkey
>> presses being handled in the kernel ?
>>
>> And then there is the whole acpi_video.brightness_switch_enabled
>> debacle, which is an option which defaults to true which causes
>> the kernel to handle LCD brightness key presses, which all distros
>> have been patching to default to off for ages.
>>
>> To give a concrete example, we may want to implement software
>> dimming / auto-off of the kbd backlight when the no keys are
>> touched for x seconds. This would seriously get in the way of that.
>>
>> So sorry, but NACK to this series.
>
> So if instead of modifying the LED value, the kernel platform drivers
> converted the TOGGLE into a cycle even by converting to an UP event
> based on awareness of the plaform specific max value and the read
> current value, leaving userspace to act on the TOGGLE/UP events - would
> that be preferable?
>
> Something like:
>
>         if (code == TOGGLE && ledval < ledmax)
>                 code = UP;
>
>         sparse_keymap_report_event(..., code, ...)
>
> }
> --
> Darren Hart
> VMware Open Source Technology Center

That's what I was trying to do in  [PATCH v2] platform/x86: asus-wmi: Add
keyboard backlight toggle support. However, that brought another problem
discussed in the thread.
https://marc.info/?l=linux-kernel&m=152639169210655&w=2

So I moved the brightness change in the driver without passing to userspace.
Per Hans, seems there're some other concerns and I also wonder if the
TOGGLE event happens in ASUS HID (asus-hid.c) which also convert and
pass the keycode to userspace but no TOGGLE key support yet What should
we do then?