Messages in this thread | | | Subject | Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change | From | Hans de Goede <> | Date | Tue, 5 Jun 2018 09:37:32 +0200 |
| |
Hi,
On 05-06-18 05:18, Chris Chiu wrote: > On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart <dvhart@infradead.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@redhat.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
| |