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

From: Hans de Goede
Date: Tue Jun 05 2018 - 07:06:11 EST


On 05-06-18 12:46, Benjamin Berg wrote:

On Tue, 2018-06-05 at 12:31 +0200, Hans de Goede wrote:
On 05-06-18 12:14, Bastien Nocera wrote:
On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote:
On 05-06-18 11:58, Bastien Nocera wrote:

Ok, so what are you suggestion, do you really want to hardcode
the cycle behavior in the kernel as these 2 patches are doing,
without any option to intervene from userspace?

As mentioned before in the thread there are several example
of the kernel deciding to handle key-presses itself, putting
policy in the kernel and they have all ended poorly (think
e.g. rfkill, acpi-video dealing with LC brightnesskey presses

I guess one thing we could do here is code out both solutions,
have a module option which controls if we:

1) Handle this in the kernel as these patches do
2) Or send a new KEY_KBDILLUMCYCLE event

Combined with a Kconfig option to select which is the default
behavior. Then Endless can select 1 for now and then in
Fedora (which defaults to Wayland now) we could default to
2. once all the code for handling 2 is in place.

This is ugly (on the kernel side) but it might be the best
compromise we can do.

I don't really mind which option is used, I'm listing the problems with
the different options. If you don't care about Xorg, then definitely go
for adding a new key. Otherwise, processing it in the kernel is the
least ugly, especially given that the key goes through the same driver
that controls the brightness anyway. There's no crazy cross driver
interaction as there was in the other cases you listed.

Unfortunately not caring about Xorg is not really an option.

Ok, new idea, how about we make g-s-d behavior upon detecting a
KEY_KBDILLUMTOGGLE event configurable, if we're on a Mac do a
toggle, otherwise do a cycle.

Or we could do this through hwdb, then we could add a hwdb entry
for this laptop setting the udev property to do a cycle instead of
a toggle on receiving the keypress.

If we are adding hwdb entries anyway to control the userspace
interpretation of the TOGGLE key, then we could also add the new CYCLE
key and explicitly re-map it to TOGGLE. That requires slightly more
logic in hwdb, but it does mean that we could theoretically just drop
the workaround if we ever stop caring about Xorg.

Hmm, interesting proposal, I say go for it :)