Date: Tue, 12 Mar 2024 23:31:27 +0300
From: mustafa <mustafa.eskieksi@...il.com>
To: hdegoede@...hat.com,
	ilpo.jarvinen@...ux.intel.com,
	mustafa.eskieksi@...il.com,
	jdelvare@...e.com,
	linux@...ck-us.net,
	linux-kernel@...r.kernel.org,
	platform-driver-x86@...r.kernel.org,
	linux-hwmon@...r.kernel.org,
	pavel@....cz,
	lee@...nel.org,
	linux-leds@...r.kernel.org
Cc: rishitbansal0@...il.com
Subject: [PATCH v4 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops

From: Mustafa Ekşi <mustafa.eskieksi@...il.com>

Hi,

On 10.03.2024 22:17, Armin Wolf wrote:
>> +static struct casper_quirk_entry *quirk_applied;
>> +static struct led_classdev_mc *casper_kbd_mcled_info;
> 
> Hi,
> 
> those global variables are initialized inside the probe() callback of 
> the WMI driver,
> so there are going to be issues when this driver is going to be 
> instantiated multiple times.
> 
> Please move those global variables into a private driver struct using 
> the state container pattern:
> https://www.kernel.org/doc/html/latest/driver-api/driver-model/design-patterns.html
> 
> Maybe you can keep the variables associated with quirk handling global 
> and instead do the DMI matching
> inside the modules init function before registering the WMI driver (this 
> would replace module_wmi_driver()).
I tried to use container_of, but I couldn't make it work with multicolor leds.
So I used driver_data instead. I don't think there is much difference.

I copied below from earlier patch.

On 23.02.2024 13:14, Ilpo Järvinen wrote:
> However, I still suspect this is wrong way to do RGB leds and the multi_*
> sysfs interface is the way you should use.
I was skeptical about using multicolor because it said it wasn't a good
fit for rgb drivers in drivers/leds/TODO but after I read the thread about
hp omen backlight driver support, I changed my mind. Hp omen's keyboard
backlight is very similar to Casper's keyboard backlight. And probably
TODO file meant "per key rgb"s and not zoned rgbs.
I think Rishit's' driver didn't get into mainline but I still liked the
API he used so I decided to use the same API.
Driver creates 4 different led_classdev_mc devices:
> casper::kbd_zoned_backlight-right/
> casper::kbd_zoned_backlight-middle/
> casper::kbd_zoned_backlight-left/
> casper::kbd_zoned_backlight-corners/
right, middle, and left leds share the same brightness but the corners
led doesn't.
I found a way to get the brightness level from the hardware, so it shows
the correct brightness when it is changed via keyboard shortcut. But
still, there's most likely no way to get color data from firmware,
Windows driver uses a Windows registry (as a cache) for same reason.

I seek your advice on how to support "led modes". It is very different
from led_pattern. It is not possible to change the interval, or anything.
This version has an enum called casper_led_mode:
> enum casper_led_mode {
>        LED_NORMAL = 0x10,
>        LED_BLINK = 0x20,
>        LED_FADE = 0x30,
>        LED_HEARTBEAT = 0x40,
>        LED_REPEAT = 0x50,
>        LED_RANDOM = 0x60
> };
For example, random mode assigns random colors to leds every second. Fade
mode slowly fades out brightness and then fades in. I thought of creating
an attribute but working with multiple leds seemed uneasy.

And also this multicolor approach doesn't include a way to set all
keyboard leds all at once (like Rishit's driver). This can create long
delays when a userspace program (like OpenRGB) sets all keyboard leds
to user configuration.

Link to Rishit Bansal's thread:
https://lore.kernel.org/lkml/20230131235027.36304-1-rishitbansal0@gmail.com/

Regards,
Mustafa Ekşi

Mustafa Ekşi (1):
  platform/x86: Add wmi driver for Casper Excalibur laptops

 MAINTAINERS                       |   6 +
 drivers/platform/x86/Kconfig      |  14 +
 drivers/platform/x86/Makefile     |   1 +
 drivers/platform/x86/casper-wmi.c | 629 ++++++++++++++++++++++++++++++
 4 files changed, 650 insertions(+)
 create mode 100644 drivers/platform/x86/casper-wmi.c

-- 
2.44.0

Powered by blists - more mailing lists