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

[Hans de Goede]

Hi,

On 05-06-18 05:18, Chris Chiu wrote:
> 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?

As I mentioned in my reply to Darren, there are 2 proper solutions to this:

1) Make userspace treat KEY_KBDILLUMTOGGLE as a cycle key, this is
what the kbd-backlight on most laptops with a single hotkey (*) does
in cases where this is handled in firmware, rather then left to the
OS. The handled in firmware is the case which I created the
led_classdev_notify_brightness_hw_changed() API for. This would be
my preferred solution and I believe that Benjamin is discussing this
with Bastien ATM.

2) Add a new KEY_KBDILLUMCYCLE event and send that for the TOGGLE code
on these laptops.

Yes both will take time to get into end-users hand, but so will a
kernel-only solution. In the mean time endless can always carry
downstream patches to make things work right now (while waiting for
the changes to trickle down from upstream).

Regards,

Hans