LKML Archive on
help / color / mirror / Atom feed
From: Yurii Pavlovskyi <>
To: Pavel Machek <>, Andy Shevchenko <>
Cc: Jacek Anaszewski <>,
	Linux LED Subsystem <>,
	Corentin Chary <>,
	Darren Hart <>,
	Andy Shevchenko <>,
	Daniel Drake <>,
	acpi4asus-user <>,
	Platform Driver <>,
	Linux Kernel Mailing List <>,
Subject: Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight
Date: Thu, 9 May 2019 21:04:53 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <20190508171229.GA22024@amd>

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

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.


  reply	other threads:[~2019-05-09 19:05 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-19  9:57 [PATCH v3 00/11] asus-wmi: Support of ASUS TUF Gaming series laptops Yurii Pavlovskyi
2019-04-19 10:00 ` [PATCH v3 01/11] platform/x86: asus-wmi: Fix hwmon device cleanup Yurii Pavlovskyi
2019-05-08 13:25   ` Andy Shevchenko
2019-04-19 10:03 ` [PATCH v3 02/11] platform/x86: asus-wmi: Fix preserving keyboard backlight intensity on load Yurii Pavlovskyi
2019-04-19 10:05 ` [PATCH v3 03/11] platform/x86: asus-wmi: Increase the input buffer size of WMI methods Yurii Pavlovskyi
2019-05-08 13:30   ` Andy Shevchenko
2019-04-19 10:08 ` [PATCH v3 04/11] platform/x86: asus-wmi: Improve DSTS WMI method ID detection Yurii Pavlovskyi
2019-05-08 13:36   ` Andy Shevchenko
2019-05-09  6:08     ` Daniel Drake
2019-05-09 17:29       ` Yurii Pavlovskyi
2019-05-09 17:57         ` Andy Shevchenko
2019-04-19 10:10 ` [PATCH v3 05/11] platform/x86: asus-wmi: Support WMI event queue Yurii Pavlovskyi
2019-05-08 13:47   ` Andy Shevchenko
2019-05-09 17:36     ` Yurii Pavlovskyi
2019-04-19 10:11 ` [PATCH v3 06/11] platform/x86: asus-nb-wmi: Add microphone mute key code Yurii Pavlovskyi
2019-04-19 10:12 ` [PATCH v3 07/11] platform/x86: asus-wmi: Organize code into sections Yurii Pavlovskyi
2019-05-08 13:46   ` Andy Shevchenko
2019-04-19 10:12 ` [PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of thermal data Yurii Pavlovskyi
2019-04-24 18:25   ` Pawnikar, Sumeet R
2019-04-25 18:51     ` Yurii Pavlovskyi
2019-05-08 13:58   ` Andy Shevchenko
2019-05-09 17:49     ` Yurii Pavlovskyi
2019-05-09 17:54       ` Andy Shevchenko
2019-04-19 10:14 ` [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight Yurii Pavlovskyi
2019-05-08 14:02   ` Andy Shevchenko
2019-05-08 17:12     ` Pavel Machek
2019-05-09 19:04       ` Yurii Pavlovskyi [this message]
2019-05-09 20:45         ` Dan Murphy
2019-05-09 21:06           ` Andy Shevchenko
2019-05-09 21:44             ` Dan Murphy
2019-05-09 22:15             ` Pavel Machek
2019-05-09 22:34           ` Pavel Machek
2019-04-19 10:15 ` [PATCH v3 10/11] platform/x86: asus-wmi: Switch fan boost mode Yurii Pavlovskyi
2019-04-19 10:16 ` [PATCH v3 11/11] platform/x86: asus-wmi: Do not disable keyboard backlight on unloading Yurii Pavlovskyi
2019-05-08 13:26 ` [PATCH v3 00/11] asus-wmi: Support of ASUS TUF Gaming series laptops Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).