On Tue, 2018-06-05 at 09:37 +0200, Hans de Goede wrote: > 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@redha > > > > > t.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. It isn't on Macs, at least. Toggle is a toggle, not a cycle key. It turns the keyboard backlight off and on, restoring the backlight level when turned back on. > 2) Add a new KEY_KBDILLUMCYCLE event Which won't be accessible to Xorg. > 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).
