Message-ID: <38cb3527-8480-bdb9-a5d9-b601bc494a5f@redhat.com>
Date:   Tue, 5 Jun 2018 09:37:32 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Chris Chiu <chiu@...lessm.com>, Darren Hart <dvhart@...radead.org>
Cc:     Daniel Drake <drake@...lessm.com>,
        Corentin Chary <corentin.chary@...il.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        acpi4asus-user <acpi4asus-user@...ts.sourceforge.net>,
        Linux Upstreaming Team <linux@...lessm.com>,
        Bastien Nocera <hadess@...ess.net>,
        Benjamin Berg <benjamin@...solutions.net>
Subject: Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API
 on kbd brightness change

Hi,

On 05-06-18 05:18, Chris Chiu wrote:
> On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart <dvhart@...radead.org> 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@...hat.com> 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

Powered by blists - more mailing lists