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

[Benjamin Berg]

Hey,

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:
> > > > [SNIP]
> > > 
> > > 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
> > > itself).
> > > 
> > > 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.

> I guess alternatively I could live with hardcoding this in the
> kernel as these 2 patches do, but that solves it just for *this*
> laptop, I've a feeling that if we do that we end up with similar
> code in all laptop vendor drivers under drivers/platform/x86
> soon.  Which really is the acpi_video.brightness_event thing
> again, where the kernel would handle brightness key-presses
> but only if the acpi_video backlight interface was in use
> and not on models with a vendor specific or native-hardware
> backlight driver.  Hmm, so writing this, I'm still quite sure
> the kernel approach is actually a bad idea.

Benjamin

Attachment: signature.asc
Description: This is a digitally signed message part