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. Thanks, Yurii
