Messages in this thread | | | Date | Fri, 10 May 2019 00:34:37 +0200 | From | Pavel Machek <> | Subject | 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" > > > > 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.
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 [unhandled content-type:application/pgp-signature] | |