From: Mustafa Ekşi <mustafa.eskieksi@xxxxxxxxx> 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@xxxxxxxxx/ 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
