Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 09-06-18 02:33, Darren Hart wrote: On Wed, Jun 06, 2018 at 05:32:52PM +0200, Hans de Goede wrote: 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. Hmm, interesting proposal, I say go for it :) So maybe the next stop is that I can follow Darren's suggestion to eliminate the is_kbd_led_event() and send a v2 for review? I believe the best compromise we have right now is to do what Hans suggested in an earlier proposal. That is implementing the two separate behaviours in the kernel 1) handle this in the kernel as if the hardware changed it, and 2) send a new KEY_KBDILLUMCYCLE event [default]. I think you mean or, not and, depending on a module option the code should do either 1) or 2) not both :) Darren, Andy could you live with a module option for this? We are of course strongly opposed to adding module options. I agree we can't ignore Xorg. I agree policy in general should not be in the kernel. I also see many of these drivers as the last mile to getting a platform fully working. If there is a place for one-off fixes, it's in these drivers. I'd love to refactor and use proper abstractions and all that as the patterns make those abstractions clear - but I don't want to delay getting something working waiting for the ideal solution. So I have two questions I'd like to confirm before saying "OK" to a module option. 1) Hans I think you said that doing the code conversion from TOGGLE to UP based on the LED value and the max value was racy with userspace. What is the failure mode here? Is it not easily recoverable? And how do I enter it? g-s-d can currently already auto-dim the brightness of the kbd backlight when idle (if there are enough brightness levels). Lets say that the brightness is at its highest setting and the user wants to cycle the backlight to off. But just before the user hits the cycle key, the timeout expires and userspace dimms the backlight, now the kernel processes the cycle key event, sees it is not max and sends an up, instead of the expected toggle. Note we already have this problem on machines where the cycle behavior is implemented in the firmware/hardware rather then inside the kernel. A bigger problem with sending up-up-up-toggle is that g-s-d saves the current brightness (max) on the first toggle and restores that on the second toggle, so the second up-up-up-toggle sequence we end up restoring the max, going from max to max on the toggle. So the user needs to press toggle twice at max brightness to get to off (and then once for the next cycle, then 2 times again for the cycle after that, etc.). So I think it is fair to say that sending up-up-up-toggle is not a good idea. Do I have to simultaneously modify the software brightness control AND press the keyboard brightness control? How practical is that? If recoverable AND hard to trigger, I think there is value in the very simple 3 level brightness cycle being handled in the kernel. 2) Why is a module option preferable to a compile time option? It seems to me the policy will be largely distro dependent, and the same kernel needing to support both modes seems likely to be pretty rare. Because lets say that we have everything in place in a recent Fedora to handle the cycle-key in userspace, so we have a mapping for the new event at all levels and g-s-d code to handle it. Then this will still only work for GNOME3 and possibly other wayland based desktop environments. While some of our users will keep using the X11 based XFCE or mate desktop environments. So what we actually want is a module option, with a configurable default. So that we can make the default send the cycle event in a future Fedora, while XFCE / mate users can override the default using the module option. ### So typing all of the above has made me think about this once more. Specifically about how most popular brands handle the cycle behavior in firmware/hardware already and that userspace already needs to deal with this and that sofar this does not seem to be a problem. Combine this with the ugliness of adding a module option + adding a new cycle input event requiring a lot of work at various layers to actually work and I think that taking this series as is, is not so bad. Esp. since I don't see anyone doing this work soon. This comes down to faking the cycling being done inside the firmware/ hardware as e.g. Thinkpads and the Dell XPS series actually do, so, as said, userspace already needs to deal with this. If we in the future actually get around to implementing a kbd-illum-cycle input event and have userspace support in place we can always add the module option then. TL;DR: lets just go with this series as is for now, we can always add a module
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 09-06-18 02:33, Darren Hart wrote: On Wed, Jun 06, 2018 at 05:32:52PM +0200, Hans de Goede wrote: 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. Hmm, interesting proposal, I say go for it :) So maybe the next stop is that I can follow Darren's suggestion to eliminate the is_kbd_led_event() and send a v2 for review? I believe the best compromise we have right now is to do what Hans suggested in an earlier proposal. That is implementing the two separate behaviours in the kernel 1) handle this in the kernel as if the hardware changed it, and 2) send a new KEY_KBDILLUMCYCLE event [default]. I think you mean or, not and, depending on a module option the code should do either 1) or 2) not both :) Darren, Andy could you live with a module option for this? We are of course strongly opposed to adding module options. I agree we can't ignore Xorg. I agree policy in general should not be in the kernel. I also see many of these drivers as the last mile to getting a platform fully working. If there is a place for one-off fixes, it's in these drivers. I'd love to refactor and use proper abstractions and all that as the patterns make those abstractions clear - but I don't want to delay getting something working waiting for the ideal solution. So I have two questions I'd like to confirm before saying "OK" to a module option. 1) Hans I think you said that doing the code conversion from TOGGLE to UP based on the LED value and the max value was racy with userspace. What is the failure mode here? Is it not easily recoverable? And how do I enter it? g-s-d can currently already auto-dim the brightness of the kbd backlight when idle (if there are enough brightness levels). Lets say that the brightness is at its highest setting and the user wants to cycle the backlight to off. But just before the user hits the cycle key, the timeout expires and userspace dimms the backlight, now the kernel processes the cycle key event, sees it is not max and sends an up, instead of the expected toggle. Note we already have this problem on machines where the cycle behavior is implemented in the firmware/hardware rather then inside the kernel. A bigger problem with sending up-up-up-toggle is that g-s-d saves the current brightness (max) on the first toggle and restores that on the second toggle, so the second up-up-up-toggle sequence we end up restoring the max, going from max to max on the toggle. So the user needs to press toggle twice at max brightness to get to off (and then once for the next cycle, then 2 times again for the cycle after that, etc.). So I think it is fair to say that sending up-up-up-toggle is not a good idea. Do I have to simultaneously modify the software brightness control AND press the keyboard brightness control? How practical is that? If recoverable AND hard to trigger, I think there is value in the very simple 3 level brightness cycle being handled in the kernel. 2) Why is a module option preferable to a compile time option? It seems to me the policy will be largely distro dependent, and the same kernel needing to support both modes seems likely to be pretty rare. Because lets say that we have everything in place in a recent Fedora to handle the cycle-key in userspace, so we have a mapping for the new event at all levels and g-s-d code to handle it. Then this will still only work for GNOME3 and possibly other wayland based desktop environments. While some of our users will keep using the X11 based XFCE or mate desktop environments. So what we actually want is a module option, with a configurable default. So that we can make the default send the cycle event in a future Fedora, while XFCE / mate users can override the default using the module option. ### So typing all of the above has made me think about this once more. Specifically about how most popular brands handle the cycle behavior in firmware/hardware already and that userspace already needs to deal with this and that sofar this does not seem to be a problem. Combine this with the ugliness of adding a module option + adding a new cycle input event requiring a lot of work at various layers to actually work and I think that taking this series as is, is not so bad. Esp. since I don't see anyone doing this work soon. This comes down to faking the cycling being done inside the firmware/ hardware as e.g. Thinkpads and the Dell XPS series actually do, so, as said, userspace already needs to deal with this. If we in the future actually get around to implementing a kbd-illum-cycle input event and have userspace support in place we can always add the module option then. TL;DR: lets just go with this series as is for now, we can always add a module
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Wed, Jun 06, 2018 at 05:32:52PM +0200, Hans de Goede wrote: > > > > > 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. > > > > > > > > Hmm, interesting proposal, I say go for it :) > > > > > > > > > > So maybe the next stop is that I can follow Darren's suggestion to > > > eliminate > > > the is_kbd_led_event() and send a v2 for review? > > > > I believe the best compromise we have right now is to do what Hans > > suggested in an earlier proposal. That is implementing the two separate > > behaviours in the kernel > > > > 1) handle this in the kernel as if the hardware changed it, and > > 2) send a new KEY_KBDILLUMCYCLE event [default]. > > I think you mean or, not and, depending on a module option the > code should do either 1) or 2) not both :) > > Darren, Andy could you live with a module option for this? We are of course strongly opposed to adding module options. I agree we can't ignore Xorg. I agree policy in general should not be in the kernel. I also see many of these drivers as the last mile to getting a platform fully working. If there is a place for one-off fixes, it's in these drivers. I'd love to refactor and use proper abstractions and all that as the patterns make those abstractions clear - but I don't want to delay getting something working waiting for the ideal solution. So I have two questions I'd like to confirm before saying "OK" to a module option. 1) Hans I think you said that doing the code conversion from TOGGLE to UP based on the LED value and the max value was racy with userspace. What is the failure mode here? Is it not easily recoverable? And how do I enter it? Do I have to simultaneously modify the software brightness control AND press the keyboard brightness control? How practical is that? If recoverable AND hard to trigger, I think there is value in the very simple 3 level brightness cycle being handled in the kernel. 2) Why is a module option preferable to a compile time option? It seems to me the policy will be largely distro dependent, and the same kernel needing to support both modes seems likely to be pretty rare. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Wed, Jun 06, 2018 at 05:32:52PM +0200, Hans de Goede wrote: > > > > > 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. > > > > > > > > Hmm, interesting proposal, I say go for it :) > > > > > > > > > > So maybe the next stop is that I can follow Darren's suggestion to > > > eliminate > > > the is_kbd_led_event() and send a v2 for review? > > > > I believe the best compromise we have right now is to do what Hans > > suggested in an earlier proposal. That is implementing the two separate > > behaviours in the kernel > > > > 1) handle this in the kernel as if the hardware changed it, and > > 2) send a new KEY_KBDILLUMCYCLE event [default]. > > I think you mean or, not and, depending on a module option the > code should do either 1) or 2) not both :) > > Darren, Andy could you live with a module option for this? We are of course strongly opposed to adding module options. I agree we can't ignore Xorg. I agree policy in general should not be in the kernel. I also see many of these drivers as the last mile to getting a platform fully working. If there is a place for one-off fixes, it's in these drivers. I'd love to refactor and use proper abstractions and all that as the patterns make those abstractions clear - but I don't want to delay getting something working waiting for the ideal solution. So I have two questions I'd like to confirm before saying "OK" to a module option. 1) Hans I think you said that doing the code conversion from TOGGLE to UP based on the LED value and the max value was racy with userspace. What is the failure mode here? Is it not easily recoverable? And how do I enter it? Do I have to simultaneously modify the software brightness control AND press the keyboard brightness control? How practical is that? If recoverable AND hard to trigger, I think there is value in the very simple 3 level brightness cycle being handled in the kernel. 2) Why is a module option preferable to a compile time option? It seems to me the policy will be largely distro dependent, and the same kernel needing to support both modes seems likely to be pretty rare. -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 06-06-18 16:27, Benjamin Berg wrote: Hi, On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote: On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede wrote: Hi, On 05-06-18 12:46, Benjamin Berg wrote: 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. Hmm, interesting proposal, I say go for it :) So maybe the next stop is that I can follow Darren's suggestion to eliminate the is_kbd_led_event() and send a v2 for review? I believe the best compromise we have right now is to do what Hans suggested in an earlier proposal. That is implementing the two separate behaviours in the kernel 1) handle this in the kernel as if the hardware changed it, and 2) send a new KEY_KBDILLUMCYCLE event [default]. I think you mean or, not and, depending on a module option the code should do either 1) or 2) not both :) Darren, Andy could you live with a module option for this? Which one is used would be a compile time option for the kernel. Then we have three different choices for handling these devices from a userspace/distribution point of view: 1. Let the kernel handle these devices (quick fix) 2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE (great if Xorg support is not a requirement) Ack, although 2 will require some work in userspace, teach all the layers like xkb about the new KEY_KBDILLUMCYCLE and teach g-s-d to listen to it and do the right thing. But long term 2. is the correct solution, so it would be good to start working towards this. 3. For Xorg support: - Add hwdb entry - remap key to KEY_KBDILLUMTOGGLE - set a flag on the keyboard - detect the flag in userspace and handle KEY_KBDILLUMTOGGLE as if KEY_KBDILLUMCYCLE was pressed (yep, quite ugly) I would just use 1. for Xorg compat and not bother with this mess. Regards, Hans
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 06-06-18 16:27, Benjamin Berg wrote: Hi, On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote: On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede wrote: Hi, On 05-06-18 12:46, Benjamin Berg wrote: 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. Hmm, interesting proposal, I say go for it :) So maybe the next stop is that I can follow Darren's suggestion to eliminate the is_kbd_led_event() and send a v2 for review? I believe the best compromise we have right now is to do what Hans suggested in an earlier proposal. That is implementing the two separate behaviours in the kernel 1) handle this in the kernel as if the hardware changed it, and 2) send a new KEY_KBDILLUMCYCLE event [default]. I think you mean or, not and, depending on a module option the code should do either 1) or 2) not both :) Darren, Andy could you live with a module option for this? Which one is used would be a compile time option for the kernel. Then we have three different choices for handling these devices from a userspace/distribution point of view: 1. Let the kernel handle these devices (quick fix) 2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE (great if Xorg support is not a requirement) Ack, although 2 will require some work in userspace, teach all the layers like xkb about the new KEY_KBDILLUMCYCLE and teach g-s-d to listen to it and do the right thing. But long term 2. is the correct solution, so it would be good to start working towards this. 3. For Xorg support: - Add hwdb entry - remap key to KEY_KBDILLUMTOGGLE - set a flag on the keyboard - detect the flag in userspace and handle KEY_KBDILLUMTOGGLE as if KEY_KBDILLUMCYCLE was pressed (yep, quite ugly) I would just use 1. for Xorg compat and not bother with this mess. Regards, Hans
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote: > On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede > wrote: > > Hi, > > > > > > On 05-06-18 12:46, Benjamin Berg wrote: > > > > > > 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. > > > > Hmm, interesting proposal, I say go for it :) > > > > So maybe the next stop is that I can follow Darren's suggestion to eliminate > the is_kbd_led_event() and send a v2 for review? I believe the best compromise we have right now is to do what Hans suggested in an earlier proposal. That is implementing the two separate behaviours in the kernel 1) handle this in the kernel as if the hardware changed it, and 2) send a new KEY_KBDILLUMCYCLE event [default]. Which one is used would be a compile time option for the kernel. Then we have three different choices for handling these devices from a userspace/distribution point of view: 1. Let the kernel handle these devices (quick fix) 2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE (great if Xorg support is not a requirement) 3. For Xorg support: - Add hwdb entry - remap key to KEY_KBDILLUMTOGGLE - set a flag on the keyboard - detect the flag in userspace and handle KEY_KBDILLUMTOGGLE as if KEY_KBDILLUMCYCLE was pressed (yep, quite ugly) The "beauty" of this approach is that the workaround from option 3 can be simply removed again if we stop caring about Xorg (or should we find a solution to handle high keycodes in Xorg). Benjamin signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On Wed, 2018-06-06 at 10:50 +0800, Chris Chiu wrote: > On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede > wrote: > > Hi, > > > > > > On 05-06-18 12:46, Benjamin Berg wrote: > > > > > > 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. > > > > Hmm, interesting proposal, I say go for it :) > > > > So maybe the next stop is that I can follow Darren's suggestion to eliminate > the is_kbd_led_event() and send a v2 for review? I believe the best compromise we have right now is to do what Hans suggested in an earlier proposal. That is implementing the two separate behaviours in the kernel 1) handle this in the kernel as if the hardware changed it, and 2) send a new KEY_KBDILLUMCYCLE event [default]. Which one is used would be a compile time option for the kernel. Then we have three different choices for handling these devices from a userspace/distribution point of view: 1. Let the kernel handle these devices (quick fix) 2. Assume we are on wayland and handle KEY_KBDILLUMCYCLE (great if Xorg support is not a requirement) 3. For Xorg support: - Add hwdb entry - remap key to KEY_KBDILLUMTOGGLE - set a flag on the keyboard - detect the flag in userspace and handle KEY_KBDILLUMTOGGLE as if KEY_KBDILLUMCYCLE was pressed (yep, quite ugly) The "beauty" of this approach is that the workaround from option 3 can be simply removed again if we stop caring about Xorg (or should we find a solution to handle high keycodes in Xorg). Benjamin signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede wrote: > Hi, > > > On 05-06-18 12:46, Benjamin Berg wrote: >> >> 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. > > > Hmm, interesting proposal, I say go for it :) > > Regards, > > Hans > > > So maybe the next stop is that I can follow Darren's suggestion to eliminate the is_kbd_led_event() and send a v2 for review?
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Tue, Jun 5, 2018 at 7:06 PM, Hans de Goede wrote: > Hi, > > > On 05-06-18 12:46, Benjamin Berg wrote: >> >> 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. > > > Hmm, interesting proposal, I say go for it :) > > Regards, > > Hans > > > So maybe the next stop is that I can follow Darren's suggestion to eliminate the is_kbd_led_event() and send a v2 for review?
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 05-06-18 12:46, Benjamin Berg wrote: 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. Hmm, interesting proposal, I say go for it :) Regards, Hans
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 05-06-18 12:46, Benjamin Berg wrote: 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. Hmm, interesting proposal, I say go for it :) Regards, Hans
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
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 signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 05-06-18 12:14, Bastien Nocera wrote: On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote: Hi, On 05-06-18 11:58, Bastien Nocera wrote: 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 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 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=152639169210655=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=152639169210655=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. 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,
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 05-06-18 12:14, Bastien Nocera wrote: On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote: Hi, On 05-06-18 11:58, Bastien Nocera wrote: 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 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 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=152639169210655=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=152639169210655=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. 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,
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote: > Hi, > > On 05-06-18 11:58, Bastien Nocera wrote: > > 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 > > > 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 > > > > > > edha > > > > > > > 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=152639169210655=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; > > > > > > > > > >
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Tue, 2018-06-05 at 12:05 +0200, Hans de Goede wrote: > Hi, > > On 05-06-18 11:58, Bastien Nocera wrote: > > 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 > > > 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 > > > > > > edha > > > > > > > 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=152639169210655=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; > > > > > > > > > >
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
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 > > 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 > > > > 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=152639169210655=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=152639169210655=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,
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
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 > > 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 > > > > 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=152639169210655=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=152639169210655=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,
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 05-06-18 11:58, Bastien Nocera wrote: 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 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 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=152639169210655=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=152639169210655=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. 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
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 05-06-18 11:58, Bastien Nocera wrote: 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 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 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=152639169210655=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=152639169210655=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. 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
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 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 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=152639169210655=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=152639169210655=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
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 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 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=152639169210655=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=152639169210655=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
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 05-06-18 04:31, Darren Hart 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 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=152639169210655=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? I was thinking about doing that as a workaround myself, but I'm not sure that is a good idea (because of e.g. races with userspace auto-correcting the brightness based on ambient light and/or keyboard activity). As I see it 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. Regards, Hans *) Rather then separate up / down hotkeys Something like: if (code == TOGGLE && ledval < ledmax) code = UP; sparse_keymap_report_event(..., code, ...) }
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 05-06-18 04:31, Darren Hart 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 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=152639169210655=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? I was thinking about doing that as a workaround myself, but I'm not sure that is a good idea (because of e.g. races with userspace auto-correcting the brightness based on ambient light and/or keyboard activity). As I see it 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. Regards, Hans *) Rather then separate up / down hotkeys Something like: if (code == TOGGLE && ledval < ledmax) code = UP; sparse_keymap_report_event(..., code, ...) }
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart 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 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=152639169210655=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=152639169210655=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?
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Tue, Jun 5, 2018 at 10:31 AM, Darren Hart 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 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=152639169210655=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=152639169210655=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?
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
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 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=152639169210655=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
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
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 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=152639169210655=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
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Mon, Jun 04, 2018 at 08:32:37PM +0800, Chris Chiu wrote: > Make asus-wmi notify on hotkey kbd brightness changes, listen for > brightness events and update the brightness directly in the driver. > For this purpose, bound check on brightness in kbd_led_set must be > based on the same data type to prevent illegal value been set. > > Update the brightness by led_classdev_notify_brightness_hw_changed. > This will allow userspace to monitor (poll) for brightness changes > on the LED without reporting via input keymapping. > > Signed-off-by: Chris Chiu > --- > drivers/platform/x86/asus-nb-wmi.c | 2 -- > drivers/platform/x86/asus-wmi.c| 21 +++-- > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/asus-nb-wmi.c > b/drivers/platform/x86/asus-nb-wmi.c > index 136ff2b4cce5..7ce80e4bb5a3 100644 > --- a/drivers/platform/x86/asus-nb-wmi.c > +++ b/drivers/platform/x86/asus-nb-wmi.c > @@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = { > { KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */ > { KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + > HDMI */ > { KE_KEY, 0xB5, { KEY_CALC } }, > - { KE_KEY, 0xC4, { KEY_KBDILLUMUP } }, > - { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } }, > { KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */ > { KE_END, 0}, > }; > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 1f6e68f0b646..b4915b7718c1 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work) > ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F); > > asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL); > + led_classdev_notify_brightness_hw_changed(>kbd_led, > asus->kbd_led_wk); > } > > static int kbd_led_read(struct asus_wmi *asus, int *level, int *env) > @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, > > asus = container_of(led_cdev, struct asus_wmi, kbd_led); > > - if (value > asus->kbd_led.max_brightness) > + if ((int)value > (int)asus->kbd_led.max_brightness) > value = asus->kbd_led.max_brightness; > - else if (value < 0) > + else if ((int)value < 0) > value = 0; > > asus->kbd_led_wk = value; > @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus) > > asus->kbd_led_wk = led_val; > asus->kbd_led.name = "asus::kbd_backlight"; > + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED; > asus->kbd_led.brightness_set = kbd_led_set; > asus->kbd_led.brightness_get = kbd_led_get; > asus->kbd_led.max_brightness = 3; > @@ -1700,6 +1702,13 @@ static int is_display_toggle(int code) > return 0; > } > > +static int is_kbd_led_event(int code) > +{ > + if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN) > + return 1; > + return 0; I don't think this is worth breaking out into a separate function. This isn't exactly a hot path, but I don't think this makes the code any more readable really. The is_display_toggle was comparing 8 values in a complex logic statement, so we don't need to follow that for something this simple. > +} > + > static void asus_wmi_notify(u32 value, void *context) > { > struct asus_wmi *asus = context; > @@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context) > } > } > > + if (is_kbd_led_event(code)) { > + if (code == NOTIFY_KBD_BRTDWN) Seems like we could eliminate the extra function and eliminate a level of indentation by rewriting this as: if (code == NOTIFY_KBD_BRTDWN) kbd_led_set(>kbd_led, asus->kbd_led_wk - 1); else if (code == NOTIFY_KBD_BRTUP) kbd_led_set(>kbd_led, asus->kbd_led_wk + 1); if (code == NOTIFY_KBD_BRTDWN || NOTIFY_KBD_BRTUP) goto exit; Or, just treat them as separate events: if (code == NOTIFY_KBD_BRTDWN) { kbd_led_set(>kbd_led, asus->kbd_led_wk - 1); goto exit; } if (code == NOTIFY_KBD_BRTUP) { kbd_led_set(>kbd_led, asus->kbd_led_wk + 1); goto exit; } > if (is_display_toggle(code) && > asus->driver->quirks->no_display_toggle) > goto exit; > -- > 2.11.0 > > -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Mon, Jun 04, 2018 at 08:32:37PM +0800, Chris Chiu wrote: > Make asus-wmi notify on hotkey kbd brightness changes, listen for > brightness events and update the brightness directly in the driver. > For this purpose, bound check on brightness in kbd_led_set must be > based on the same data type to prevent illegal value been set. > > Update the brightness by led_classdev_notify_brightness_hw_changed. > This will allow userspace to monitor (poll) for brightness changes > on the LED without reporting via input keymapping. > > Signed-off-by: Chris Chiu > --- > drivers/platform/x86/asus-nb-wmi.c | 2 -- > drivers/platform/x86/asus-wmi.c| 21 +++-- > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/asus-nb-wmi.c > b/drivers/platform/x86/asus-nb-wmi.c > index 136ff2b4cce5..7ce80e4bb5a3 100644 > --- a/drivers/platform/x86/asus-nb-wmi.c > +++ b/drivers/platform/x86/asus-nb-wmi.c > @@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = { > { KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */ > { KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + > HDMI */ > { KE_KEY, 0xB5, { KEY_CALC } }, > - { KE_KEY, 0xC4, { KEY_KBDILLUMUP } }, > - { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } }, > { KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */ > { KE_END, 0}, > }; > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 1f6e68f0b646..b4915b7718c1 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work) > ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F); > > asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL); > + led_classdev_notify_brightness_hw_changed(>kbd_led, > asus->kbd_led_wk); > } > > static int kbd_led_read(struct asus_wmi *asus, int *level, int *env) > @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, > > asus = container_of(led_cdev, struct asus_wmi, kbd_led); > > - if (value > asus->kbd_led.max_brightness) > + if ((int)value > (int)asus->kbd_led.max_brightness) > value = asus->kbd_led.max_brightness; > - else if (value < 0) > + else if ((int)value < 0) > value = 0; > > asus->kbd_led_wk = value; > @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus) > > asus->kbd_led_wk = led_val; > asus->kbd_led.name = "asus::kbd_backlight"; > + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED; > asus->kbd_led.brightness_set = kbd_led_set; > asus->kbd_led.brightness_get = kbd_led_get; > asus->kbd_led.max_brightness = 3; > @@ -1700,6 +1702,13 @@ static int is_display_toggle(int code) > return 0; > } > > +static int is_kbd_led_event(int code) > +{ > + if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN) > + return 1; > + return 0; I don't think this is worth breaking out into a separate function. This isn't exactly a hot path, but I don't think this makes the code any more readable really. The is_display_toggle was comparing 8 values in a complex logic statement, so we don't need to follow that for something this simple. > +} > + > static void asus_wmi_notify(u32 value, void *context) > { > struct asus_wmi *asus = context; > @@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context) > } > } > > + if (is_kbd_led_event(code)) { > + if (code == NOTIFY_KBD_BRTDWN) Seems like we could eliminate the extra function and eliminate a level of indentation by rewriting this as: if (code == NOTIFY_KBD_BRTDWN) kbd_led_set(>kbd_led, asus->kbd_led_wk - 1); else if (code == NOTIFY_KBD_BRTUP) kbd_led_set(>kbd_led, asus->kbd_led_wk + 1); if (code == NOTIFY_KBD_BRTDWN || NOTIFY_KBD_BRTUP) goto exit; Or, just treat them as separate events: if (code == NOTIFY_KBD_BRTDWN) { kbd_led_set(>kbd_led, asus->kbd_led_wk - 1); goto exit; } if (code == NOTIFY_KBD_BRTUP) { kbd_led_set(>kbd_led, asus->kbd_led_wk + 1); goto exit; } > if (is_display_toggle(code) && > asus->driver->quirks->no_display_toggle) > goto exit; > -- > 2.11.0 > > -- Darren Hart VMware Open Source Technology Center
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi there Let me add my two cents on the Toshiba side. 2018-06-04 8:23 GMT-06:00 Hans de Goede : > > 1) drivers/platform/x86/toshiba_acpi.c > > I don't know how the key on Toshiba's behaves on models where > it is hardwired / under Windows With Toshiba we have two types of hardware implementations: 1st gen keyboards, supporting AUTO and FN-Z AUTO - Turns on/off automatically after some (configurable) time FN-Z - Creates "toshiba::kbd_backlight" and it's toggled by userspace 2nd gen keyborads, supporting AUTO, ON and OFF AUTO - Ditto ON - Always on OFF - Always off The second gen keyboards are completely driven by hardware, userspace must be checking sysfs for "kbd_backlight_mode" changes, however, the Toshiba interface emits the 0x92 ACPI event when we have a kbd mode change, but it's not currently being transmitted to userspace via netlink. Saludos Azael -- -- El mundo apesta y vosotros apestais tambien --
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi there Let me add my two cents on the Toshiba side. 2018-06-04 8:23 GMT-06:00 Hans de Goede : > > 1) drivers/platform/x86/toshiba_acpi.c > > I don't know how the key on Toshiba's behaves on models where > it is hardwired / under Windows With Toshiba we have two types of hardware implementations: 1st gen keyboards, supporting AUTO and FN-Z AUTO - Turns on/off automatically after some (configurable) time FN-Z - Creates "toshiba::kbd_backlight" and it's toggled by userspace 2nd gen keyborads, supporting AUTO, ON and OFF AUTO - Ditto ON - Always on OFF - Always off The second gen keyboards are completely driven by hardware, userspace must be checking sysfs for "kbd_backlight_mode" changes, however, the Toshiba interface emits the 0x92 ACPI event when we have a kbd mode change, but it's not currently being transmitted to userspace via netlink. Saludos Azael -- -- El mundo apesta y vosotros apestais tambien --
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 04-06-18 15:51, Daniel Drake wrote: On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede 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=152639169210655=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. ### With that all said lets look at a userspace solution grepping through the kernel for KEY_KBDILLUMTOGGLE there are 4 users: 1) drivers/platform/x86/toshiba_acpi.c I don't know how the key on Toshiba's behaves on models where it is hardwired / under Windows 2) drivers/platform/x86/dell-wmi.c All Dells I know of have the hotkey do a cycle, not a toggle on/off 3) Some apple specific drivers: According to: https://support.apple.com/en-us/HT202310 Apple has separate up/down keys, so no idea why the drivers sometimes send a KEY_KBDILLUMTOGGLE event 4) drivers/hid/hid-input.c Okay, so the HUT clearly defines the Consumer Page mapping 0x35 as "OOC – Toggles illumination of consumer control's buttons and controls on/off." which is a bit of a bummer, because I was hoping that we could just redefine KEY_KBDILLUMTOGGLE to mean cycle. Still despite HID HUT being clear about this being an on/off toggle. I'm thinking that cycling makes more sense everywhere and that we should simply rename gnome-settings-daemon/plugins/power/gsd-power-manager.c 's upower_kbd_toggle() function to upower_kbd_cycle() and make it behave accordingly. If we device the range up into 8 steps (if there are more then 8 to being with) then I think this will be more useful everywhere then just the on/off toggle. The alternative would be to define a new keycode, but that will be > 240 so it will only work on Wayland and not under X11. Benjamin, Bastien, opinions? Regards, Hans
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi, On 04-06-18 15:51, Daniel Drake wrote: On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede 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=152639169210655=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. ### With that all said lets look at a userspace solution grepping through the kernel for KEY_KBDILLUMTOGGLE there are 4 users: 1) drivers/platform/x86/toshiba_acpi.c I don't know how the key on Toshiba's behaves on models where it is hardwired / under Windows 2) drivers/platform/x86/dell-wmi.c All Dells I know of have the hotkey do a cycle, not a toggle on/off 3) Some apple specific drivers: According to: https://support.apple.com/en-us/HT202310 Apple has separate up/down keys, so no idea why the drivers sometimes send a KEY_KBDILLUMTOGGLE event 4) drivers/hid/hid-input.c Okay, so the HUT clearly defines the Consumer Page mapping 0x35 as "OOC – Toggles illumination of consumer control's buttons and controls on/off." which is a bit of a bummer, because I was hoping that we could just redefine KEY_KBDILLUMTOGGLE to mean cycle. Still despite HID HUT being clear about this being an on/off toggle. I'm thinking that cycling makes more sense everywhere and that we should simply rename gnome-settings-daemon/plugins/power/gsd-power-manager.c 's upower_kbd_toggle() function to upower_kbd_cycle() and make it behave accordingly. If we device the range up into 8 steps (if there are more then 8 to being with) then I think this will be more useful everywhere then just the on/off toggle. The alternative would be to define a new keycode, but that will be > 240 so it will only work on Wayland and not under X11. Benjamin, Bastien, opinions? Regards, Hans
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede 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=152639169210655=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? Daniel
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
On Mon, Jun 4, 2018 at 7:22 AM, Hans de Goede 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=152639169210655=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? Daniel
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi Chris. On 04-06-18 14:32, Chris Chiu wrote: Make asus-wmi notify on hotkey kbd brightness changes, listen for brightness events and update the brightness directly in the driver. For this purpose, bound check on brightness in kbd_led_set must be based on the same data type to prevent illegal value been set. Update the brightness by led_classdev_notify_brightness_hw_changed. This will allow userspace to monitor (poll) for brightness changes on the LED without reporting via input keymapping. 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). Regards, Hans Signed-off-by: Chris Chiu --- drivers/platform/x86/asus-nb-wmi.c | 2 -- drivers/platform/x86/asus-wmi.c| 21 +++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c index 136ff2b4cce5..7ce80e4bb5a3 100644 --- a/drivers/platform/x86/asus-nb-wmi.c +++ b/drivers/platform/x86/asus-nb-wmi.c @@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = { { KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */ { KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + HDMI */ { KE_KEY, 0xB5, { KEY_CALC } }, - { KE_KEY, 0xC4, { KEY_KBDILLUMUP } }, - { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } }, { KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */ { KE_END, 0}, }; diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 1f6e68f0b646..b4915b7718c1 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work) ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F); asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL); + led_classdev_notify_brightness_hw_changed(>kbd_led, asus->kbd_led_wk); } static int kbd_led_read(struct asus_wmi *asus, int *level, int *env) @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, asus = container_of(led_cdev, struct asus_wmi, kbd_led); - if (value > asus->kbd_led.max_brightness) + if ((int)value > (int)asus->kbd_led.max_brightness) value = asus->kbd_led.max_brightness; - else if (value < 0) + else if ((int)value < 0) value = 0; asus->kbd_led_wk = value; @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus) asus->kbd_led_wk = led_val; asus->kbd_led.name = "asus::kbd_backlight"; + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED; asus->kbd_led.brightness_set = kbd_led_set; asus->kbd_led.brightness_get = kbd_led_get; asus->kbd_led.max_brightness = 3; @@ -1700,6 +1702,13 @@ static int is_display_toggle(int code) return 0; } +static int is_kbd_led_event(int code) +{ + if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN) + return 1; + return 0; +} + static void asus_wmi_notify(u32 value, void *context) { struct asus_wmi *asus = context; @@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context) } } + if (is_kbd_led_event(code)) { + if (code == NOTIFY_KBD_BRTDWN) + kbd_led_set(>kbd_led, asus->kbd_led_wk - 1); + else + kbd_led_set(>kbd_led, asus->kbd_led_wk + 1); + goto exit; + } + if (is_display_toggle(code) && asus->driver->quirks->no_display_toggle) goto exit;
Re: [PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Hi Chris. On 04-06-18 14:32, Chris Chiu wrote: Make asus-wmi notify on hotkey kbd brightness changes, listen for brightness events and update the brightness directly in the driver. For this purpose, bound check on brightness in kbd_led_set must be based on the same data type to prevent illegal value been set. Update the brightness by led_classdev_notify_brightness_hw_changed. This will allow userspace to monitor (poll) for brightness changes on the LED without reporting via input keymapping. 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). Regards, Hans Signed-off-by: Chris Chiu --- drivers/platform/x86/asus-nb-wmi.c | 2 -- drivers/platform/x86/asus-wmi.c| 21 +++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c index 136ff2b4cce5..7ce80e4bb5a3 100644 --- a/drivers/platform/x86/asus-nb-wmi.c +++ b/drivers/platform/x86/asus-nb-wmi.c @@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = { { KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */ { KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + HDMI */ { KE_KEY, 0xB5, { KEY_CALC } }, - { KE_KEY, 0xC4, { KEY_KBDILLUMUP } }, - { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } }, { KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */ { KE_END, 0}, }; diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 1f6e68f0b646..b4915b7718c1 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work) ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F); asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL); + led_classdev_notify_brightness_hw_changed(>kbd_led, asus->kbd_led_wk); } static int kbd_led_read(struct asus_wmi *asus, int *level, int *env) @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, asus = container_of(led_cdev, struct asus_wmi, kbd_led); - if (value > asus->kbd_led.max_brightness) + if ((int)value > (int)asus->kbd_led.max_brightness) value = asus->kbd_led.max_brightness; - else if (value < 0) + else if ((int)value < 0) value = 0; asus->kbd_led_wk = value; @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus) asus->kbd_led_wk = led_val; asus->kbd_led.name = "asus::kbd_backlight"; + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED; asus->kbd_led.brightness_set = kbd_led_set; asus->kbd_led.brightness_get = kbd_led_get; asus->kbd_led.max_brightness = 3; @@ -1700,6 +1702,13 @@ static int is_display_toggle(int code) return 0; } +static int is_kbd_led_event(int code) +{ + if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN) + return 1; + return 0; +} + static void asus_wmi_notify(u32 value, void *context) { struct asus_wmi *asus = context; @@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context) } } + if (is_kbd_led_event(code)) { + if (code == NOTIFY_KBD_BRTDWN) + kbd_led_set(>kbd_led, asus->kbd_led_wk - 1); + else + kbd_led_set(>kbd_led, asus->kbd_led_wk + 1); + goto exit; + } + if (is_display_toggle(code) && asus->driver->quirks->no_display_toggle) goto exit;
[PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Make asus-wmi notify on hotkey kbd brightness changes, listen for brightness events and update the brightness directly in the driver. For this purpose, bound check on brightness in kbd_led_set must be based on the same data type to prevent illegal value been set. Update the brightness by led_classdev_notify_brightness_hw_changed. This will allow userspace to monitor (poll) for brightness changes on the LED without reporting via input keymapping. Signed-off-by: Chris Chiu --- drivers/platform/x86/asus-nb-wmi.c | 2 -- drivers/platform/x86/asus-wmi.c| 21 +++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c index 136ff2b4cce5..7ce80e4bb5a3 100644 --- a/drivers/platform/x86/asus-nb-wmi.c +++ b/drivers/platform/x86/asus-nb-wmi.c @@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = { { KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */ { KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + HDMI */ { KE_KEY, 0xB5, { KEY_CALC } }, - { KE_KEY, 0xC4, { KEY_KBDILLUMUP } }, - { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } }, { KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */ { KE_END, 0}, }; diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 1f6e68f0b646..b4915b7718c1 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work) ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F); asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL); + led_classdev_notify_brightness_hw_changed(>kbd_led, asus->kbd_led_wk); } static int kbd_led_read(struct asus_wmi *asus, int *level, int *env) @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, asus = container_of(led_cdev, struct asus_wmi, kbd_led); - if (value > asus->kbd_led.max_brightness) + if ((int)value > (int)asus->kbd_led.max_brightness) value = asus->kbd_led.max_brightness; - else if (value < 0) + else if ((int)value < 0) value = 0; asus->kbd_led_wk = value; @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus) asus->kbd_led_wk = led_val; asus->kbd_led.name = "asus::kbd_backlight"; + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED; asus->kbd_led.brightness_set = kbd_led_set; asus->kbd_led.brightness_get = kbd_led_get; asus->kbd_led.max_brightness = 3; @@ -1700,6 +1702,13 @@ static int is_display_toggle(int code) return 0; } +static int is_kbd_led_event(int code) +{ + if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN) + return 1; + return 0; +} + static void asus_wmi_notify(u32 value, void *context) { struct asus_wmi *asus = context; @@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context) } } + if (is_kbd_led_event(code)) { + if (code == NOTIFY_KBD_BRTDWN) + kbd_led_set(>kbd_led, asus->kbd_led_wk - 1); + else + kbd_led_set(>kbd_led, asus->kbd_led_wk + 1); + goto exit; + } + if (is_display_toggle(code) && asus->driver->quirks->no_display_toggle) goto exit; -- 2.11.0
[PATCH 1/2] platform/x86: asus-wmi: Call new led hw_changed API on kbd brightness change
Make asus-wmi notify on hotkey kbd brightness changes, listen for brightness events and update the brightness directly in the driver. For this purpose, bound check on brightness in kbd_led_set must be based on the same data type to prevent illegal value been set. Update the brightness by led_classdev_notify_brightness_hw_changed. This will allow userspace to monitor (poll) for brightness changes on the LED without reporting via input keymapping. Signed-off-by: Chris Chiu --- drivers/platform/x86/asus-nb-wmi.c | 2 -- drivers/platform/x86/asus-wmi.c| 21 +++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c index 136ff2b4cce5..7ce80e4bb5a3 100644 --- a/drivers/platform/x86/asus-nb-wmi.c +++ b/drivers/platform/x86/asus-nb-wmi.c @@ -493,8 +493,6 @@ static const struct key_entry asus_nb_wmi_keymap[] = { { KE_KEY, 0xA6, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + HDMI */ { KE_KEY, 0xA7, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + HDMI */ { KE_KEY, 0xB5, { KEY_CALC } }, - { KE_KEY, 0xC4, { KEY_KBDILLUMUP } }, - { KE_KEY, 0xC5, { KEY_KBDILLUMDOWN } }, { KE_IGNORE, 0xC6, }, /* Ambient Light Sensor notification */ { KE_END, 0}, }; diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 1f6e68f0b646..b4915b7718c1 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -460,6 +460,7 @@ static void kbd_led_update(struct work_struct *work) ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F); asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL); + led_classdev_notify_brightness_hw_changed(>kbd_led, asus->kbd_led_wk); } static int kbd_led_read(struct asus_wmi *asus, int *level, int *env) @@ -497,9 +498,9 @@ static void kbd_led_set(struct led_classdev *led_cdev, asus = container_of(led_cdev, struct asus_wmi, kbd_led); - if (value > asus->kbd_led.max_brightness) + if ((int)value > (int)asus->kbd_led.max_brightness) value = asus->kbd_led.max_brightness; - else if (value < 0) + else if ((int)value < 0) value = 0; asus->kbd_led_wk = value; @@ -656,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus) asus->kbd_led_wk = led_val; asus->kbd_led.name = "asus::kbd_backlight"; + asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED; asus->kbd_led.brightness_set = kbd_led_set; asus->kbd_led.brightness_get = kbd_led_get; asus->kbd_led.max_brightness = 3; @@ -1700,6 +1702,13 @@ static int is_display_toggle(int code) return 0; } +static int is_kbd_led_event(int code) +{ + if (code == NOTIFY_KBD_BRTUP || code == NOTIFY_KBD_BRTDWN) + return 1; + return 0; +} + static void asus_wmi_notify(u32 value, void *context) { struct asus_wmi *asus = context; @@ -1745,6 +1754,14 @@ static void asus_wmi_notify(u32 value, void *context) } } + if (is_kbd_led_event(code)) { + if (code == NOTIFY_KBD_BRTDWN) + kbd_led_set(>kbd_led, asus->kbd_led_wk - 1); + else + kbd_led_set(>kbd_led, asus->kbd_led_wk + 1); + goto exit; + } + if (is_display_toggle(code) && asus->driver->quirks->no_display_toggle) goto exit; -- 2.11.0