Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
Hi! > >> Yes, please. We have common interface for LED drivers; this needs to > >> use it. > > > > That is indeed a better option and I did in fact considered this first and > > even did a test implementation. The discoveries were: > > 1. The WMI methods are write-only and only written all at once in a > > transaction manner (also invoking solely first RGB-interface method has no > > effect until some other keyboard backlight method is called). Write-only is not a problem, right? Nor should be transaction. Just cache the values in kernel. > > 2. In addition to RGB there are several control values, which switch > > effects, speed and enable or disable the backlight under specific > > conditions or switch whether it is set temporarily or permanently (not that > > these are critical functionalities, but for the sake of > > completeness). Yep, lets ignore that for now. > > 3. The EC is really slow > > # time bash -c "echo 1 > /sys/devices/platform/faustus/kbbl_set" > > > > real0m0,691s > > user0m0,000s > > sys 0m0,691s > > > > (please ignore the sysfs-path there, it's essentially the same code running > > as in this patch). It is consistently same for both temporary and permanent > > configuration. Writing after every change would take about (6+)x of that. > > Not that it's that unbearable though as it is not likely to be > > done often. Yup, this is quite ugly. What about simply ignoring changes as they happen, and then setting RGB channels when nothing changes for 10msec? > > I was not quite happy with that implementation so I opted for writing sort > > of sysfs wrapper instead that would allow same sort of transactions as > > provided by BIOS. I agree that it's non-standard solution. > > > > If I understood correctly, the typical current RGB led_class devices from > > the Linux tree currently provide channels as separate LEDs. There are also > > blink / pattern options present, I guess one could misuse them for setting > > effects and speed. So one could make 3 devices for RGB + 3 for awake, > > sleep, boot modes + 1 for setting effect / speed. Take a look at "pattern" trigger. That should give you effect/speed options. .. for single channel. > > I'd guess the end solution might be also either something like combination > > of both approaches (RGB leds + separate sysfs interface) or some extension > > of the led_class device interface. Dropping support of the non-essential > > features for the sake of uniformity of ABI would also be an option to > > consider (exposing just three RGB LEDs with brightness only), not happy one > > though. > > > > In any case this looks like it might need some additional research, > > discussion, development, and a pair of iterations so I tend to separate > > this patch from the series and post it extra after the others are through > > to avoid dragging 10+ patches around. Separate patch certainly makes sense. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
On Fri 2019-05-10 00:06:11, Andy Shevchenko wrote: > On Thu, May 9, 2019 at 11:45 PM Dan Murphy wrote: > > On 5/9/19 2:04 PM, Yurii Pavlovskyi wrote: > > We are working on a framework for this. > > > > Please see this series > > https://lore.kernel.org/patchwork/project/lkml/list/?series=390141 > > > > It is still a work in progress > > Side question: > Have you considered to convert existing color LED controllers? (It > seems to me that your proposal lacks of the idea to keep back > compatibility with the existing controllers whre user may create a > sysfs node based on the arbitrary label, while it's good to have > multicolor infrastructure like in your proposal. Did I miss > something?) That's undecided at the moment. We have enough fun trying to figure out reasonable interface... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
Andy On 5/9/19 4:06 PM, Andy Shevchenko wrote: > On Thu, May 9, 2019 at 11:45 PM Dan Murphy wrote: >> On 5/9/19 2:04 PM, Yurii Pavlovskyi wrote: >> We are working on a framework for this. >> >> Please see this series >> https://lore.kernel.org/patchwork/project/lkml/list/?series=390141 >> >> It is still a work in progress > > Side question: > Have you considered to convert existing color LED controllers? (It > seems to me that your proposal lacks of the idea to keep back > compatibility with the existing controllers whre user may create a > sysfs node based on the arbitrary label, while it's good to have > multicolor infrastructure like in your proposal. Did I miss > something?) > > Yes that is part of the work that is in progress. The LED driver should be able to register either a single color LED or a group of colored LEDs. This can be based on a firmware entry and which LED framework the driver chooses to register to. Either the multicolor framework or the base LED framework. Of course we can put this in code and keep it out of the firmware nodes again thats why it is wip. I have convert a couple of drivers over in my testing that support RGB modules or have a RGB cluter used to mix colors. If the product wants to expose a single red LED via the label then they use legacy registration. If the product wants to expose RGBW as a single group then the multicolor framework should be registered too. Dan
Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
On Thu, May 9, 2019 at 11:45 PM Dan Murphy wrote: > On 5/9/19 2:04 PM, Yurii Pavlovskyi wrote: > We are working on a framework for this. > > Please see this series > https://lore.kernel.org/patchwork/project/lkml/list/?series=390141 > > It is still a work in progress Side question: Have you considered to convert existing color LED controllers? (It seems to me that your proposal lacks of the idea to keep back compatibility with the existing controllers whre user may create a sysfs node based on the arbitrary label, while it's good to have multicolor infrastructure like in your proposal. Did I miss something?) -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
Yurii On 5/9/19 2:04 PM, Yurii Pavlovskyi wrote: > First of all, thanks to Andy for all the review comments! > > I will implement all the ones that I didn't directly answer on as well and > update this series shortly. > > Regarding this patch, > > On 08.05.19 19:12, Pavel Machek wrote: >>> Shouldn't be the LED subsystem driver for this? >> >> Yes, please. We have common interface for LED drivers; this needs to >> use it. > > That is indeed a better option and I did in fact considered this first and > even did a test implementation. The discoveries were: > 1. The WMI methods are write-only and only written all at once in a > transaction manner (also invoking solely first RGB-interface method has no > effect until some other keyboard backlight method is called). > 2. In addition to RGB there are several control values, which switch > effects, speed and enable or disable the backlight under specific > conditions or switch whether it is set temporarily or permanently (not that > these are critical functionalities, but for the sake of completeness). > 3. The EC is really slow > # time bash -c "echo 1 > /sys/devices/platform/faustus/kbbl_set" > > real 0m0,691s > user 0m0,000s > sys 0m0,691s > > (please ignore the sysfs-path there, it's essentially the same code running > as in this patch). It is consistently same for both temporary and permanent > configuration. Writing after every change would take about (6+)x of that. > Not that it's that unbearable though as it is not likely to be done often. > > I was not quite happy with that implementation so I opted for writing sort > of sysfs wrapper instead that would allow same sort of transactions as > provided by BIOS. I agree that it's non-standard solution. > > If I understood correctly, the typical current RGB led_class devices from > the Linux tree currently provide channels as separate LEDs. There are also > blink / pattern options present, I guess one could misuse them for setting > effects and speed. So one could make 3 devices for RGB + 3 for awake, > sleep, boot modes + 1 for setting effect / speed. > > I'd guess the end solution might be also either something like combination > of both approaches (RGB leds + separate sysfs interface) or some extension > of the led_class device interface. Dropping support of the non-essential > features for the sake of uniformity of ABI would also be an option to > consider (exposing just three RGB LEDs with brightness only), not happy one > though. > > In any case this looks like it might need some additional research, > discussion, development, and a pair of iterations so I tend to separate > this patch from the series and post it extra after the others are through > to avoid dragging 10+ patches around. > > Any suggestions on how to do this properly would be appreciated. That's the > best I could come up with at the moment. > We are working on a framework for this. Please see this series https://lore.kernel.org/patchwork/project/lkml/list/?series=390141 It is still a work in progress > Thanks, > Yurii >
Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
First of all, thanks to Andy for all the review comments! I will implement all the ones that I didn't directly answer on as well and update this series shortly. Regarding this patch, On 08.05.19 19:12, Pavel Machek wrote: >> Shouldn't be the LED subsystem driver for this? > > Yes, please. We have common interface for LED drivers; this needs to > use it. That is indeed a better option and I did in fact considered this first and even did a test implementation. The discoveries were: 1. The WMI methods are write-only and only written all at once in a transaction manner (also invoking solely first RGB-interface method has no effect until some other keyboard backlight method is called). 2. In addition to RGB there are several control values, which switch effects, speed and enable or disable the backlight under specific conditions or switch whether it is set temporarily or permanently (not that these are critical functionalities, but for the sake of completeness). 3. The EC is really slow # time bash -c "echo 1 > /sys/devices/platform/faustus/kbbl_set" real0m0,691s user0m0,000s sys 0m0,691s (please ignore the sysfs-path there, it's essentially the same code running as in this patch). It is consistently same for both temporary and permanent configuration. Writing after every change would take about (6+)x of that. Not that it's that unbearable though as it is not likely to be done often. I was not quite happy with that implementation so I opted for writing sort of sysfs wrapper instead that would allow same sort of transactions as provided by BIOS. I agree that it's non-standard solution. If I understood correctly, the typical current RGB led_class devices from the Linux tree currently provide channels as separate LEDs. There are also blink / pattern options present, I guess one could misuse them for setting effects and speed. So one could make 3 devices for RGB + 3 for awake, sleep, boot modes + 1 for setting effect / speed. I'd guess the end solution might be also either something like combination of both approaches (RGB leds + separate sysfs interface) or some extension of the led_class device interface. Dropping support of the non-essential features for the sake of uniformity of ABI would also be an option to consider (exposing just three RGB LEDs with brightness only), not happy one though. In any case this looks like it might need some additional research, discussion, development, and a pair of iterations so I tend to separate this patch from the series and post it extra after the others are through to avoid dragging 10+ patches around. Any suggestions on how to do this properly would be appreciated. That's the best I could come up with at the moment. Thanks, Yurii
Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
Hi! > > The WMI exposes two methods for controlling RGB keyboard backlight, which > > allows controlling: > > * RGB components in range 00 - ff, > > * Switch between 4 effects, > > * Switch between 3 effect speed modes, > > * Separately enable the backlight on boot, in the awake state (after driver > > load), in sleep mode, and probably in something called shutdown mode (no > > observable effects of enabling it are known so far). > > > > The configuration should be written to several sysfs parameter buffers > > which are then written via WMI by writing either 1 or 2 to the "kbbl_set" > > parameter. When reading the buffers the last written value is returned. > > > > If the 2 is written to "kbbl_set", the parameters will be reset on reboot > > (temporary mode), 1 is permanent mode, parameters are retained. > > > > The calls use new 3-dword input buffer method call. > > > > The functionality is only enabled if corresponding DSTS methods return > > exact valid values. > > > > The following script demonstrates usage: > > > > echo Red [00 - ff] > > echo 33 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_red > > echo Green [00 - ff] > > echo ff > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_green > > echo Blue [00 - ff] > > echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_blue > > echo Mode: 0 - static color, 1 - breathing, 2 - color cycle, 3 - strobing > > echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_mode > > echo Speed for modes 1 and 2: 0 - slow, 1 - medium, 2 - fast > > echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_speed > > echo Enable: 02 - on boot, before module load, 08 - awake, 20 - sleep, > > echo 2a or ff to set all > > echo 2a > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_flags > > echo Save: 1 - permanently, 2 - temporarily, reset after reboot > > echo 1 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_set > > > > Shouldn't be the LED subsystem driver for this? Yes, please. We have common interface for LED drivers; this needs to use it. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
On Fri, Apr 19, 2019 at 1:14 PM Yurii Pavlovskyi
wrote:
>
> The WMI exposes two methods for controlling RGB keyboard backlight, which
> allows controlling:
> * RGB components in range 00 - ff,
> * Switch between 4 effects,
> * Switch between 3 effect speed modes,
> * Separately enable the backlight on boot, in the awake state (after driver
> load), in sleep mode, and probably in something called shutdown mode (no
> observable effects of enabling it are known so far).
>
> The configuration should be written to several sysfs parameter buffers
> which are then written via WMI by writing either 1 or 2 to the "kbbl_set"
> parameter. When reading the buffers the last written value is returned.
>
> If the 2 is written to "kbbl_set", the parameters will be reset on reboot
> (temporary mode), 1 is permanent mode, parameters are retained.
>
> The calls use new 3-dword input buffer method call.
>
> The functionality is only enabled if corresponding DSTS methods return
> exact valid values.
>
> The following script demonstrates usage:
>
> echo Red [00 - ff]
> echo 33 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_red
> echo Green [00 - ff]
> echo ff > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_green
> echo Blue [00 - ff]
> echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_blue
> echo Mode: 0 - static color, 1 - breathing, 2 - color cycle, 3 - strobing
> echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_mode
> echo Speed for modes 1 and 2: 0 - slow, 1 - medium, 2 - fast
> echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_speed
> echo Enable: 02 - on boot, before module load, 08 - awake, 20 - sleep,
> echo 2a or ff to set all
> echo 2a > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_flags
> echo Save: 1 - permanently, 2 - temporarily, reset after reboot
> echo 1 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_set
>
Shouldn't be the LED subsystem driver for this?
> Signed-off-by: Yurii Pavlovskyi
> ---
> .../ABI/testing/sysfs-platform-asus-wmi | 61
> drivers/platform/x86/asus-wmi.c | 331 ++
> include/linux/platform_data/x86/asus-wmi.h| 2 +
> 3 files changed, 394 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 019e1e29370e..1cc54d5e3e10 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -36,3 +36,64 @@ KernelVersion: 3.5
> Contact: "AceLan Kao"
> Description:
> Resume on lid open. 1 means on, 0 means off.
> +
> +What: /sys/devices/platform//kbbl/kbbl_red
> +Date: Apr 2019
> +KernelVersion: 5.1
> +Contact: "Yurii Pavlovskyi"
> +Description:
> + RGB keyboard backlight red component: 00 .. ff.
> +
> +What: /sys/devices/platform//kbbl/kbbl_green
> +Date: Apr 2019
> +KernelVersion: 5.1
> +Contact: "Yurii Pavlovskyi"
> +Description:
> + RGB keyboard backlight green component: 00 .. ff.
> +
> +What: /sys/devices/platform//kbbl/kbbl_blue
> +Date: Apr 2019
> +KernelVersion: 5.1
> +Contact: "Yurii Pavlovskyi"
> +Description:
> + RGB keyboard backlight blue component: 00 .. ff.
> +
> +What: /sys/devices/platform//kbbl/kbbl_mode
> +Date: Apr 2019
> +KernelVersion: 5.1
> +Contact: "Yurii Pavlovskyi"
> +Description:
> + RGB keyboard backlight mode:
> + * 0 - static color,
> + * 1 - breathing,
> + * 2 - color cycle,
> + * 3 - strobing.
> +
> +What: /sys/devices/platform//kbbl/kbbl_speed
> +Date: Apr 2019
> +KernelVersion: 5.1
> +Contact: "Yurii Pavlovskyi"
> +Description:
> + RGB keyboard backlight speed for modes 1 and 2:
> + * 0 - slow,
> + * 1 - medium,
> + * 2 - fast.
> +
> +What: /sys/devices/platform//kbbl/kbbl_flags
> +Date: Apr 2019
> +KernelVersion: 5.1
> +Contact: "Yurii Pavlovskyi"
> +Description:
> + RGB keyboard backlight enable flags (2a to enable
> everything), OR of:
> + * 02 - on boot (until module load),
> + * 08 - awake,
> + * 20 - sleep.
> +
> +What: /sys/devices/platform//kbbl/kbbl_set
> +Date: Apr 2019
> +KernelVersion: 5.1
> +Contact: "Yurii Pavlovskyi"
> +Description:
> + Write changed RGB keyboard backlight parameters:
> + * 1 - permanently,
> + * 2 - temporarily.
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1b8272374660..0a32079336d8 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -148,6 +148,21 @@ struct asus_rfkill {
> u32
[PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
The WMI exposes two methods for controlling RGB keyboard backlight, which
allows controlling:
* RGB components in range 00 - ff,
* Switch between 4 effects,
* Switch between 3 effect speed modes,
* Separately enable the backlight on boot, in the awake state (after driver
load), in sleep mode, and probably in something called shutdown mode (no
observable effects of enabling it are known so far).
The configuration should be written to several sysfs parameter buffers
which are then written via WMI by writing either 1 or 2 to the "kbbl_set"
parameter. When reading the buffers the last written value is returned.
If the 2 is written to "kbbl_set", the parameters will be reset on reboot
(temporary mode), 1 is permanent mode, parameters are retained.
The calls use new 3-dword input buffer method call.
The functionality is only enabled if corresponding DSTS methods return
exact valid values.
The following script demonstrates usage:
echo Red [00 - ff]
echo 33 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_red
echo Green [00 - ff]
echo ff > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_green
echo Blue [00 - ff]
echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_blue
echo Mode: 0 - static color, 1 - breathing, 2 - color cycle, 3 - strobing
echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_mode
echo Speed for modes 1 and 2: 0 - slow, 1 - medium, 2 - fast
echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_speed
echo Enable: 02 - on boot, before module load, 08 - awake, 20 - sleep,
echo 2a or ff to set all
echo 2a > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_flags
echo Save: 1 - permanently, 2 - temporarily, reset after reboot
echo 1 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_set
Signed-off-by: Yurii Pavlovskyi
---
.../ABI/testing/sysfs-platform-asus-wmi | 61
drivers/platform/x86/asus-wmi.c | 331 ++
include/linux/platform_data/x86/asus-wmi.h| 2 +
3 files changed, 394 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 019e1e29370e..1cc54d5e3e10 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -36,3 +36,64 @@ KernelVersion: 3.5
Contact: "AceLan Kao"
Description:
Resume on lid open. 1 means on, 0 means off.
+
+What: /sys/devices/platform//kbbl/kbbl_red
+Date: Apr 2019
+KernelVersion: 5.1
+Contact: "Yurii Pavlovskyi"
+Description:
+ RGB keyboard backlight red component: 00 .. ff.
+
+What: /sys/devices/platform//kbbl/kbbl_green
+Date: Apr 2019
+KernelVersion: 5.1
+Contact: "Yurii Pavlovskyi"
+Description:
+ RGB keyboard backlight green component: 00 .. ff.
+
+What: /sys/devices/platform//kbbl/kbbl_blue
+Date: Apr 2019
+KernelVersion: 5.1
+Contact: "Yurii Pavlovskyi"
+Description:
+ RGB keyboard backlight blue component: 00 .. ff.
+
+What: /sys/devices/platform//kbbl/kbbl_mode
+Date: Apr 2019
+KernelVersion: 5.1
+Contact: "Yurii Pavlovskyi"
+Description:
+ RGB keyboard backlight mode:
+ * 0 - static color,
+ * 1 - breathing,
+ * 2 - color cycle,
+ * 3 - strobing.
+
+What: /sys/devices/platform//kbbl/kbbl_speed
+Date: Apr 2019
+KernelVersion: 5.1
+Contact: "Yurii Pavlovskyi"
+Description:
+ RGB keyboard backlight speed for modes 1 and 2:
+ * 0 - slow,
+ * 1 - medium,
+ * 2 - fast.
+
+What: /sys/devices/platform//kbbl/kbbl_flags
+Date: Apr 2019
+KernelVersion: 5.1
+Contact: "Yurii Pavlovskyi"
+Description:
+ RGB keyboard backlight enable flags (2a to enable everything),
OR of:
+ * 02 - on boot (until module load),
+ * 08 - awake,
+ * 20 - sleep.
+
+What: /sys/devices/platform//kbbl/kbbl_set
+Date: Apr 2019
+KernelVersion: 5.1
+Contact: "Yurii Pavlovskyi"
+Description:
+ Write changed RGB keyboard backlight parameters:
+ * 1 - permanently,
+ * 2 - temporarily.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 1b8272374660..0a32079336d8 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -148,6 +148,21 @@ struct asus_rfkill {
u32 dev_id;
};
+struct asus_kbbl_rgb {
+ u8 kbbl_red;
+ u8 kbbl_green;
+ u8 kbbl_blue;
+ u8 kbbl_mode;
+ u8 kbbl_speed;
+
+ u8 kbbl_set_red;
+ u8 kbbl_set_green;
+ u8 kbbl_set_blue;
+ u8 kbbl_set_mode;
+ u8 kbbl_set_speed;
+ u8 kbbl_set_flags;
+};
+
struct asus_wmi {
int
