Re: [PATCH] platform/x86: acer-wmi: Add support for Acer Helios 300 RGB keyboard backlight

[Hans de Goede]

Hi Pavel,

On 5/19/21 10:54 AM, Pavel Machek wrote:
> Hi!
> 
>>> From e65b0ddbf559aa3ca8a7998404e7e67e64b705e9 Mon Sep 17 00:00:00 2001
>>> From: JafarAkhondali <jafar.akhoondali@xxxxxxxxx>
>>> Date: Fri, 14 May 2021 08:26:47 +0430
>>> Subject: [PATCH] platform/x86: acer-wmi: Add support for Acer Helios
>>> 300 rgb keyboard backlight
>>>
>>> The Acer helios 300 provides gaming functions WMI that is available in
>>> Windows, however this was not implemented in Linux. The process of finding
>>> the related method was done by decompiling PredatorSense(official Acer
>>> gaming functions software for Predator series) and decompiling WQ
>>> buffers. This patch provides a gaming interface which will then expose a
>>> character device named "acer-gkbbl". This character device accepts 16
>>> bytes long config, which is specific for the backlight method. The
>>> meaning of each bytes ordered by bit position is as follows:
>>>
>>> Bit 0 -> Backlight modes:
>>> 1: Breath
>>> 2: Neon
>>> 3: Wave
>>> 4: Shifting
>>> 5: Zoom
>>> Bit 1 -> Animation Speed: from 1 to 9 ( 1 is slowest, 9 is fastest)
>>> Bit 2 -> Brightness from 0 to 100 ( 0 is no backlight, 100 is brightest)
>>> Bit 3 -> Unknown. Wave effect uses 8, other modes must use 0
>>> Bit 4 -> Animation Direction:
>>> 1: Right-to-Left
>>> 2: Left-to-Right
>>> Bit 5 -> Red Color Selection
>>> Bit 6 -> Green Color Selection
>>> Bit 7 -> Blue Color Selection
>>> Bit 8 -> Currently unknown, or not used in known model
>>> Bit 9 -> Currently unknown, or not used in known model
>>> Bit 10 -> Currently unknown, or not used in known model
>>> Bit 11 -> Currently unknown, or not used in known model
>>> Bit 12 -> Currently unknown, or not used in known model
>>> Bit 13 -> Currently unknown, or not used in known model
>>> Bit 14 -> Currently unknown, or not used in known model
>>> Bit 15 -> Currently unknown, or not used in known model
>>>
>>> Filling this config is out of scope for the kernel module, and this module
>>> only acts as an interface.
>>>
>>> Currently, I'm not sure with the method for communicating with user-space,
>>> but since leds.h subsystem wouldn't fit for complex actions such as this
>>> complex config, I couldn't find any better method than char dev.
>>
>> Thank you for your patch, given that there is no existing kernel
>> interface which is a good match for the features exported by this
>> keyboard I'm fine with just having a raw interface where userspace
>> writes GAMING_KBBL_CONFIG_LEN bytes as you suggest.
> 
> Keyboard backlight goes through LED interface (so please cc the mailing list) and
> no, passing raw bytes to hardware is not an acceptable interface.
> 
>> But lets not use a classdev + chardev for this please, you can
>> just add a binary write-only sysfs-atrribute under the wmi-dev for
>> this with a name like (for example) gaming_kbd_backlight_config
>> and then userspace can write to that without needing a class + char
>> dev just for this single write.
> 
> NAK. We have existing interfaces for this.

I don't think we have existing interfaces for things like wave / zoom /
neon effects.  I guess this could use the new CONFIG_LEDS_CLASS_MULTICOLOR
API to set the color + overall brightness, combined with a custom sysfs attribute
under the led_classdev's device's dir for selecting the things for which there
is no standard API ?

Regards,

Hans