2024-03-24 18:13:28

by Mustafa Ekşi

[permalink] [raw]
Subject: [PATCH v5 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops

From: Mustafa Ekşi <[email protected]>

Hi,
I want to note that moving mutex_init to the bottom of the function
crashes the driver when mutex_lock is called. I didn't investigate it
further but I wanted to say that since Ai Chao also did it like that.

Driver sets all leds to white on start. Before that, when a led's
brightness is changed, that led's color gets set to white but others
keep their old colors which creates a bad user experience (at least for
me). Please inform me if this is a bad approach.
Also, this driver still lacks support for changing modes and I seek
advise for that.

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 | 641 ++++++++++++++++++++++++++++++
4 files changed, 662 insertions(+)
create mode 100644 drivers/platform/x86/casper-wmi.c

--
2.44.0



2024-03-24 18:13:34

by Mustafa Ekşi

[permalink] [raw]
Subject: [PATCH v5 1/1] platform/x86: Add wmi driver for Casper Excalibur laptops

From: Mustafa Ekşi <[email protected]>

This wmi driver supports Casper Excalibur laptops' changing keyboard
backlight, reading fan speeds and changing power profiles. Multicolor
led device is used for backlight, platform_profile for power management
and hwmon for fan speeds. It supports both old (10th gen or older) and
new (11th gen or newer) laptops. It uses x86_match_cpu to check if the
laptop is old or new.
This driver's Multicolor keyboard backlight API is very similar to Rishit
Bansal's proposed API.

Signed-off-by: Mustafa Ekşi <[email protected]>
---
Changes in v5:
- Added mutex_destroy to casper_wmi_probe error handling
- casper_multicolor_register now sets all leds to CASPER_DEFAULT_COLOR
- Some minor changes
Changes in v4:
- Renamed casper_driver to casper_drv
- Moved all global variables to casper_drv struct. Devices access
casper_drv via wdev's driver_data.
- Removed struct led_cache, because only its u32 array was used. It is
replaced with color_cache.
- Added mutex_locks in casper_set and casper_query, so they now accept
casper_drv instead of wmi_device as argument.
- Changed endianess conversion in hwmon_read to something sparse doesn't
complain about.
- Moved registrations of multicolor leds and platform profile to their
own functions. This makes casper_wmi_probe more readable.
- Added .no_singleton to wmi_device.
- Some minor changes.
Changes in v3:
- Replaced led_control attribute with multicolor led interface.
- Added struct led_cache, instead of storing only last color change.
- Added dmi list to prevent registering platform_profile driver in models
that doesn't have this feature.
- Added a x86_cpu_id to differentiate older laptops that are reporting
fan speeds in big-endian. Also newer laptops have a different power
profile scheme. I'm using x86_cpu_id because they don't have a
difference in model names, only in cpu generations (the official driver
download page makes you select your cpu's generation too).
- Removed hwmon_pwm device in favor of platform_profile driver. It
indirectly affects fans' speed but they also affect frequency and
power consumption as well.
- Replaced handwritten masks with GENMASK equivalents.
- Replaced led_classdev_register with
devm_led_classdev_multicolor_register. This should solve the bug
where led_classdev remains registered even if casper_wmi_probe
returns -ENODEV.
- Removed select NEW_LEDS and LEDS_CLASS, because it creates recursive
dependencies.
- And some minor changes.
Changes in v2:
- Added masks for
- Changed casper_set and casper_query returns Linux error code rather
than acpi_status.
- replaced complicated bit operations with FIELD_GET.
- Fixed some indentation and spacing.
- Broke fan speeds further.
- Moved module metadata to the end of the file.
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 14 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/casper-wmi.c | 641 ++++++++++++++++++++++++++++++
4 files changed, 662 insertions(+)
create mode 100644 drivers/platform/x86/casper-wmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb08..78f908449ea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4761,6 +4761,12 @@ S: Maintained
W: https://wireless.wiki.kernel.org/en/users/Drivers/carl9170
F: drivers/net/wireless/ath/carl9170/

+CASPER EXCALIBUR WMI DRIVER
+M: Mustafa Ekşi <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/platform/x86/casper-wmi.c
+
CAVIUM I2C DRIVER
M: Robert Richter <[email protected]>
S: Odd Fixes
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7e9251fc334..e671ccbf23a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1122,6 +1122,20 @@ config SEL3350_PLATFORM
To compile this driver as a module, choose M here: the module
will be called sel3350-platform.

+config CASPER_WMI
+ tristate "Casper Excalibur Laptop WMI driver"
+ depends on ACPI_WMI
+ depends on HWMON
+ depends on LEDS_CLASS_MULTICOLOR
+ select ACPI_PLATFORM_PROFILE
+ help
+ Say Y here if you want to support WMI-based fan speed reporting,
+ power management and keyboard backlight support on Casper Excalibur
+ Laptops.
+
+ To compile this driver as a module, choose M here: the module will
+ be called casper-wmi.
+
endif # X86_PLATFORM_DEVICES

config P2SB
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 1de432e8861..4b527dd44ad 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
obj-$(CONFIG_NVIDIA_WMI_EC_BACKLIGHT) += nvidia-wmi-ec-backlight.o
obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
+obj-$(CONFIG_CASPER_WMI) += casper-wmi.o

# Acer
obj-$(CONFIG_ACERHDF) += acerhdf.o
diff --git a/drivers/platform/x86/casper-wmi.c b/drivers/platform/x86/casper-wmi.c
new file mode 100644
index 00000000000..e86def01753
--- /dev/null
+++ b/drivers/platform/x86/casper-wmi.c
@@ -0,0 +1,641 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/module.h>
+#include <linux/bits.h>
+#include <linux/bitops.h>
+#include <linux/acpi.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
+#include <linux/wmi.h>
+#include <linux/device.h>
+#include <linux/hwmon.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <acpi/acexcep.h>
+#include <linux/bitfield.h>
+#include <linux/platform_profile.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/types.h>
+#include <linux/mutex_types.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/container_of.h>
+
+#include <linux/dmi.h>
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
+#define CASPER_WMI_GUID "644C5791-B7B0-4123-A90B-E93876E0DAAD"
+
+#define CASPER_READ 0xfa00
+#define CASPER_WRITE 0xfb00
+#define CASPER_GET_HARDWAREINFO 0x0200
+#define CASPER_SET_LED 0x0100
+#define CASPER_POWERPLAN 0x0300
+
+#define CASPER_KEYBOARD_LED_1 0x03
+#define CASPER_KEYBOARD_LED_2 0x04
+#define CASPER_KEYBOARD_LED_3 0x05
+#define CASPER_ALL_KEYBOARD_LEDS 0x06
+#define CASPER_CORNER_LEDS 0x07
+#define CASPER_LED_COUNT 4
+
+static const char * const zone_names[CASPER_LED_COUNT] = {
+ "casper::kbd_zoned_backlight-right",
+ "casper::kbd_zoned_backlight-middle",
+ "casper::kbd_zoned_backlight-left",
+ "casper::kbd_zoned_backlight-corners",
+};
+
+#define CASPER_LED_ALPHA GENMASK(31, 24)
+#define CASPER_LED_RED GENMASK(23, 16)
+#define CASPER_LED_GREEN GENMASK(15, 8)
+#define CASPER_LED_BLUE GENMASK(7, 0)
+#define CASPER_DEFAULT_COLOR (CASPER_LED_RED | CASPER_LED_GREEN | \
+ CASPER_LED_BLUE)
+#define CASPER_FAN_CPU 0
+#define CASPER_FAN_GPU 1
+
+enum casper_power_profile_old {
+ CASPER_HIGH_PERFORMANCE = 1,
+ CASPER_GAMING = 2,
+ CASPER_TEXT_MODE = 3,
+ CASPER_POWERSAVE = 4
+};
+
+enum casper_power_profile_new {
+ CASPER_NEW_HIGH_PERFORMANCE = 0,
+ CASPER_NEW_GAMING = 1,
+ CASPER_NEW_AUDIO = 2
+};
+
+struct casper_quirk_entry {
+ bool big_endian_fans;
+ bool no_power_profiles;
+ bool new_power_scheme;
+};
+
+struct casper_drv {
+ struct platform_profile_handler handler;
+ struct mutex casper_mutex;
+ u32 color_cache[CASPER_LED_COUNT];
+ struct led_classdev_mc *casper_kbd_mc;
+ struct mc_subled *subleds;
+ struct wmi_device *wdev;
+ struct casper_quirk_entry *quirk_applied;
+};
+
+struct casper_wmi_args {
+ u16 a0, a1;
+ u32 a2, a3, a4, a5, a6, a7, a8;
+};
+
+enum casper_led_mode {
+ LED_NORMAL = 0x10,
+ LED_BLINK = 0x20,
+ LED_FADE = 0x30,
+ LED_HEARTBEAT = 0x40,
+ LED_REPEAT = 0x50,
+ LED_RANDOM = 0x60
+};
+
+static int casper_set(struct casper_drv *drv, u16 a1, u8 led_id, u32 data)
+{
+ acpi_status ret = 0;
+ struct casper_wmi_args wmi_args;
+ struct acpi_buffer input;
+
+ wmi_args = (struct casper_wmi_args) {
+ .a0 = CASPER_WRITE,
+ .a1 = a1,
+ .a2 = led_id,
+ .a3 = data
+ };
+
+ input = (struct acpi_buffer) {
+ (acpi_size) sizeof(struct casper_wmi_args),
+ &wmi_args
+ };
+
+ mutex_lock(&drv->casper_mutex);
+
+ ret = wmidev_block_set(drv->wdev, 0, &input);
+ if (ACPI_FAILURE(ret))
+ ret = -EIO;
+
+ mutex_unlock(&drv->casper_mutex);
+ return ret;
+}
+
+static int casper_query(struct casper_drv *drv, u16 a1,
+ struct casper_wmi_args *out)
+{
+ union acpi_object *obj;
+ struct casper_wmi_args wmi_args;
+ struct acpi_buffer input;
+ int ret = 0;
+
+ wmi_args = (struct casper_wmi_args) {
+ .a0 = CASPER_READ,
+ .a1 = a1
+ };
+ input = (struct acpi_buffer) {
+ (acpi_size) sizeof(struct casper_wmi_args),
+ &wmi_args
+ };
+
+ mutex_lock(&drv->casper_mutex);
+
+ ret = wmidev_block_set(drv->wdev, 0, &input);
+ if (ACPI_FAILURE(ret)) {
+ ret = -EIO;
+ goto unlock;
+ }
+
+ obj = wmidev_block_query(drv->wdev, 0);
+ if (!obj) {
+ ret = -EIO;
+ goto unlock;
+ }
+
+ if (obj->type != ACPI_TYPE_BUFFER) { // obj will be 0x10 on failure
+ ret = -EINVAL;
+ goto freeobj;
+ }
+ if (obj->buffer.length != sizeof(struct casper_wmi_args)) {
+ ret = -EIO;
+ goto freeobj;
+ }
+
+ memcpy(out, obj->buffer.pointer, sizeof(struct casper_wmi_args));
+
+freeobj:
+ kfree(obj);
+unlock:
+ mutex_unlock(&drv->casper_mutex);
+ return ret;
+}
+
+static enum led_brightness get_casper_brightness(struct led_classdev *led_cdev)
+{
+ struct casper_drv *drv = dev_get_drvdata(led_cdev->dev->parent);
+ struct casper_wmi_args hardware_alpha = {0};
+
+ if (strcmp(led_cdev->name, zone_names[3]) == 0)
+ return FIELD_GET(CASPER_LED_ALPHA, drv->color_cache[3]);
+
+ casper_query(drv, CASPER_GET_HARDWAREINFO, &hardware_alpha);
+
+ return hardware_alpha.a6;
+}
+
+static void set_casper_brightness(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ u32 bright_with_mode, bright_prep, led_data, led_data_no_alpha;
+ struct casper_drv *drv;
+ int ret;
+ size_t zone;
+ u8 zone_to_change;
+
+ drv = dev_get_drvdata(led_cdev->dev->parent);
+
+ for (size_t i = 0; i < CASPER_LED_COUNT; i++)
+ if (strcmp(led_cdev->name, zone_names[i]) == 0)
+ zone = i;
+ if (zone == 3)
+ zone_to_change = CASPER_CORNER_LEDS;
+ else
+ zone_to_change = zone + CASPER_KEYBOARD_LED_1;
+
+ led_data_no_alpha = drv->subleds[zone].intensity & ~CASPER_LED_ALPHA;
+
+ if ((drv->color_cache[zone] & ~CASPER_LED_ALPHA) == led_data_no_alpha)
+ bright_with_mode = brightness | LED_NORMAL;
+ else
+ bright_with_mode = get_casper_brightness(led_cdev) | LED_NORMAL;
+
+ bright_prep = FIELD_PREP(CASPER_LED_ALPHA, bright_with_mode);
+ led_data = bright_prep | led_data_no_alpha;
+ ret = casper_set(drv, CASPER_SET_LED, zone_to_change, led_data);
+ if (ret)
+ return;
+
+ drv->color_cache[zone] = led_data;
+}
+
+static int casper_platform_profile_get(struct platform_profile_handler *pprof,
+ enum platform_profile_option *profile)
+{
+ struct casper_drv *drv = container_of(pprof, struct casper_drv,
+ handler);
+ struct casper_wmi_args ret_buff = {0};
+ int ret;
+
+ ret = casper_query(drv, CASPER_POWERPLAN, &ret_buff);
+ if (ret)
+ return ret;
+
+ if (drv->quirk_applied->new_power_scheme) {
+ switch (ret_buff.a2) {
+ case CASPER_NEW_HIGH_PERFORMANCE:
+ *profile = PLATFORM_PROFILE_PERFORMANCE;
+ break;
+ case CASPER_NEW_GAMING:
+ *profile = PLATFORM_PROFILE_BALANCED;
+ break;
+ case CASPER_NEW_AUDIO:
+ *profile = PLATFORM_PROFILE_LOW_POWER;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return 0;
+ }
+
+ switch (ret_buff.a2) {
+ case CASPER_HIGH_PERFORMANCE:
+ *profile = PLATFORM_PROFILE_PERFORMANCE;
+ break;
+ case CASPER_GAMING:
+ *profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
+ break;
+ case CASPER_TEXT_MODE:
+ *profile = PLATFORM_PROFILE_BALANCED;
+ break;
+ case CASPER_POWERSAVE:
+ *profile = PLATFORM_PROFILE_LOW_POWER;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int casper_platform_profile_set(struct platform_profile_handler *pprof,
+ enum platform_profile_option profile)
+{
+ struct casper_drv *drv = container_of(pprof, struct casper_drv,
+ handler);
+ enum casper_power_profile_old prf_old;
+ enum casper_power_profile_new prf_new;
+
+ if (drv->quirk_applied->new_power_scheme) {
+
+ switch (profile) {
+ case PLATFORM_PROFILE_PERFORMANCE:
+ prf_new = CASPER_NEW_HIGH_PERFORMANCE;
+ break;
+ case PLATFORM_PROFILE_BALANCED:
+ prf_new = CASPER_NEW_GAMING;
+ break;
+ case PLATFORM_PROFILE_LOW_POWER:
+ prf_new = CASPER_NEW_AUDIO;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return casper_set(drv, CASPER_POWERPLAN, prf_new, 0);
+ }
+
+ switch (profile) {
+ case PLATFORM_PROFILE_PERFORMANCE:
+ prf_old = CASPER_HIGH_PERFORMANCE;
+ break;
+ case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
+ prf_old = CASPER_GAMING;
+ break;
+ case PLATFORM_PROFILE_BALANCED:
+ prf_old = CASPER_TEXT_MODE;
+ break;
+ case PLATFORM_PROFILE_LOW_POWER:
+ prf_old = CASPER_POWERSAVE;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return casper_set(drv, CASPER_POWERPLAN, prf_old, 0);
+}
+
+static umode_t casper_wmi_hwmon_is_visible(const void *drvdata,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ return 0444;
+}
+
+static int casper_wmi_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct casper_wmi_args out = { 0 };
+ struct casper_drv *drv = dev_get_drvdata(dev->parent);
+ int ret;
+
+ ret = casper_query(drv, CASPER_GET_HARDWAREINFO, &out);
+ if (ret)
+ return ret;
+
+ switch (channel) {
+ case CASPER_FAN_CPU:
+ if (drv->quirk_applied->big_endian_fans)
+ *val = be16_to_cpu(*(__be16 *)&out.a4);
+ else
+ *val = out.a5;
+ break;
+ case CASPER_FAN_GPU:
+ if (drv->quirk_applied->big_endian_fans)
+ *val = be16_to_cpu(*(__be16 *)&out.a5);
+ else
+ *val = out.a5;
+ break;
+ }
+
+ return 0;
+}
+
+static int casper_wmi_hwmon_read_string(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, const char **str)
+{
+ if (channel == CASPER_FAN_CPU)
+ *str = "cpu_fan_speed";
+ else if (channel == CASPER_FAN_GPU)
+ *str = "gpu_fan_speed";
+ return 0;
+}
+
+static const struct hwmon_ops casper_wmi_hwmon_ops = {
+ .is_visible = &casper_wmi_hwmon_is_visible,
+ .read = &casper_wmi_hwmon_read,
+ .read_string = &casper_wmi_hwmon_read_string,
+};
+
+static const struct hwmon_channel_info *const casper_wmi_hwmon_info[] = {
+ HWMON_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_LABEL,
+ HWMON_F_INPUT | HWMON_F_LABEL),
+ NULL
+};
+
+static const struct hwmon_chip_info casper_wmi_hwmon_chip_info = {
+ .ops = &casper_wmi_hwmon_ops,
+ .info = casper_wmi_hwmon_info,
+};
+
+static struct casper_quirk_entry gen_older_than_11 = {
+ .big_endian_fans = true,
+ .new_power_scheme = false
+};
+
+static struct casper_quirk_entry gen_newer_than_11 = {
+ .big_endian_fans = false,
+ .new_power_scheme = true
+};
+
+static const struct x86_cpu_id casper_gen[] = {
+ X86_MATCH_INTEL_FAM6_MODEL(KABYLAKE, &gen_older_than_11),
+ X86_MATCH_INTEL_FAM6_MODEL(COMETLAKE, &gen_older_than_11),
+ X86_MATCH_INTEL_FAM6_MODEL(TIGERLAKE, &gen_newer_than_11),
+ X86_MATCH_INTEL_FAM6_MODEL(ALDERLAKE, &gen_newer_than_11),
+ X86_MATCH_INTEL_FAM6_MODEL(RAPTORLAKE, &gen_newer_than_11),
+ X86_MATCH_INTEL_FAM6_MODEL(METEORLAKE, &gen_newer_than_11),
+ {}
+};
+
+static struct casper_quirk_entry quirk_no_power_profile = {
+ .no_power_profiles = true
+};
+
+static struct casper_quirk_entry quirk_has_power_profile = {
+ .no_power_profiles = false
+};
+
+static const struct dmi_system_id casper_quirks[] = {
+ {
+ .ident = "CASPER EXCALIBUR G650",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR,
+ "CASPER BILGISAYAR SISTEMLERI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G650")
+ },
+ .driver_data = &quirk_no_power_profile
+ },
+ {
+ .ident = "CASPER EXCALIBUR G670",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR,
+ "CASPER BILGISAYAR SISTEMLERI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G670")
+ },
+ .driver_data = &quirk_no_power_profile
+ },
+ {
+ .ident = "CASPER EXCALIBUR G750",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR,
+ "CASPER BILGISAYAR SISTEMLERI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G750")
+ },
+ .driver_data = &quirk_no_power_profile
+ },
+ {
+ .ident = "CASPER EXCALIBUR G770",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR,
+ "CASPER BILGISAYAR SISTEMLERI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G770")
+ },
+ .driver_data = &quirk_has_power_profile
+ },
+ {
+ .ident = "CASPER EXCALIBUR G780",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR,
+ "CASPER BILGISAYAR SISTEMLERI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G780")
+ },
+ .driver_data = &quirk_has_power_profile
+ },
+ {
+ .ident = "CASPER EXCALIBUR G870",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR,
+ "CASPER BILGISAYAR SISTEMLERI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G870")
+ },
+ .driver_data = &quirk_has_power_profile
+ },
+ {
+ .ident = "CASPER EXCALIBUR G900",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR,
+ "CASPER BILGISAYAR SISTEMLERI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G900")
+ },
+ .driver_data = &quirk_has_power_profile
+ },
+ {
+ .ident = "CASPER EXCALIBUR G911",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR,
+ "CASPER BILGISAYAR SISTEMLERI"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G911")
+ },
+ .driver_data = &quirk_has_power_profile
+ },
+ { }
+};
+
+static int casper_platform_profile_register(struct casper_drv *drv)
+{
+ drv->handler.profile_get = casper_platform_profile_get;
+ drv->handler.profile_set = casper_platform_profile_set;
+
+ set_bit(PLATFORM_PROFILE_LOW_POWER, drv->handler.choices);
+ set_bit(PLATFORM_PROFILE_BALANCED, drv->handler.choices);
+ if (!drv->quirk_applied->new_power_scheme)
+ set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
+ drv->handler.choices);
+ set_bit(PLATFORM_PROFILE_PERFORMANCE, drv->handler.choices);
+
+ return platform_profile_register(&drv->handler);
+}
+
+static int casper_multicolor_register(struct casper_drv *drv)
+{
+ int ret = 0;
+
+ drv->casper_kbd_mc = devm_kcalloc(&drv->wdev->dev,
+ CASPER_LED_COUNT, sizeof(*drv->casper_kbd_mc), GFP_KERNEL);
+
+ drv->subleds = devm_kcalloc(&drv->wdev->dev,
+ CASPER_LED_COUNT, sizeof(struct mc_subled *), GFP_KERNEL);
+ if (!drv->casper_kbd_mc || !drv->subleds)
+ return -ENOMEM;
+
+ for (size_t i = 0; i < CASPER_LED_COUNT; i++) {
+ drv->subleds[i] = (struct mc_subled) {
+ .color_index = LED_COLOR_ID_RGB,
+ .brightness = 2,
+ .intensity = CASPER_DEFAULT_COLOR
+ };
+ drv->casper_kbd_mc[i] = (struct led_classdev_mc) {
+ .led_cdev = {
+ .name = zone_names[i],
+ .brightness = 0,
+ .max_brightness = 2,
+ .brightness_set = &set_casper_brightness,
+ .brightness_get = &get_casper_brightness,
+ .color = LED_COLOR_ID_RGB,
+ },
+ .num_colors = 1,
+ .subled_info = &drv->subleds[i]
+ };
+
+ ret = devm_led_classdev_multicolor_register(&drv->wdev->dev,
+ &drv->casper_kbd_mc[i]);
+ if (ret)
+ return -ENODEV;
+ drv->color_cache[i] = CASPER_DEFAULT_COLOR;
+ }
+
+ // Setting leds to the default color
+ ret = casper_set(drv, CASPER_SET_LED, CASPER_ALL_KEYBOARD_LEDS,
+ CASPER_DEFAULT_COLOR);
+ if (ret)
+ return ret;
+
+ ret = casper_set(drv, CASPER_SET_LED, CASPER_CORNER_LEDS,
+ CASPER_DEFAULT_COLOR);
+ return ret;
+}
+
+static int casper_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct device *hwmon_dev;
+ struct casper_drv *drv;
+ const struct x86_cpu_id *gen_id;
+ const struct dmi_system_id *dmi_id;
+ struct casper_quirk_entry *pp_quirk;
+ int ret;
+
+ drv = devm_kzalloc(&wdev->dev, sizeof(*drv), GFP_KERNEL);
+ if (!drv)
+ return -ENOMEM;
+
+ drv->wdev = wdev;
+ dev_set_drvdata(&wdev->dev, drv);
+
+ gen_id = x86_match_cpu(casper_gen);
+ if (!gen_id)
+ return -ENODEV;
+
+ drv->quirk_applied = (struct casper_quirk_entry *)gen_id->driver_data;
+
+ dmi_id = dmi_first_match(casper_quirks);
+ if (!dmi_id)
+ return -ENODEV;
+
+ pp_quirk = (struct casper_quirk_entry *)dmi_id->driver_data;
+ drv->quirk_applied->no_power_profiles = pp_quirk->no_power_profiles;
+
+ mutex_init(&drv->casper_mutex);
+
+ ret = casper_multicolor_register(drv);
+ if (ret)
+ goto destroy;
+
+ hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
+ "casper_wmi", wdev,
+ &casper_wmi_hwmon_chip_info,
+ NULL);
+ if (IS_ERR(hwmon_dev)) {
+ ret = PTR_ERR(hwmon_dev);
+ goto destroy;
+ }
+
+ if (!drv->quirk_applied->no_power_profiles) {
+ ret = casper_platform_profile_register(drv);
+ if (ret)
+ goto destroy;
+ }
+
+ return 0;
+destroy:
+ mutex_destroy(&drv->casper_mutex);
+ return ret;
+}
+
+static void casper_wmi_remove(struct wmi_device *wdev)
+{
+ struct casper_drv *drv = dev_get_drvdata(&wdev->dev);
+
+ mutex_destroy(&drv->casper_mutex);
+ if (!drv->quirk_applied->no_power_profiles)
+ platform_profile_remove();
+}
+
+static const struct wmi_device_id casper_wmi_id_table[] = {
+ { CASPER_WMI_GUID, NULL },
+ { }
+};
+
+static struct wmi_driver casper_drv = {
+ .driver = {
+ .name = "casper-wmi",
+ },
+ .id_table = casper_wmi_id_table,
+ .probe = casper_wmi_probe,
+ .remove = &casper_wmi_remove,
+ .no_singleton = true,
+};
+
+module_wmi_driver(casper_drv);
+MODULE_DEVICE_TABLE(wmi, casper_wmi_id_table);
+
+MODULE_AUTHOR("Mustafa Ekşi <[email protected]>");
+MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
+MODULE_LICENSE("GPL");
--
2.44.0


2024-03-25 20:17:27

by Armin Wolf

[permalink] [raw]
Subject: Re: [PATCH v5 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops

Am 24.03.24 um 19:12 schrieb mustafa:

> From: Mustafa Ekşi <[email protected]>
>
> Hi,
> I want to note that moving mutex_init to the bottom of the function
> crashes the driver when mutex_lock is called. I didn't investigate it
> further but I wanted to say that since Ai Chao also did it like that.

You mean like the lenovo-wmi-camera driver? In this case, the driver was
only using the mutex inside the WMI notify callback, which can only run
once the driver has finished probing.

In your case however, the mutex can already be used while the driver is still
probing, because it registers callbacks with other subsystems.
The subsystem (maybe led?) assumes that the device is already fully operational
and will begin calling the callbacks immediately, causing a crash.

Looking at the code, it seems that you are not calling mutex_destroy() in
casper_wmi_remove(). I suggest to use devm_add_action_or_reset() for handling this.

Thanks,
Armin Wolf

> Driver sets all leds to white on start. Before that, when a led's
> brightness is changed, that led's color gets set to white but others
> keep their old colors which creates a bad user experience (at least for
> me). Please inform me if this is a bad approach.
> Also, this driver still lacks support for changing modes and I seek
> advise for that.
>
> 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 | 641 ++++++++++++++++++++++++++++++
> 4 files changed, 662 insertions(+)
> create mode 100644 drivers/platform/x86/casper-wmi.c
>

2024-04-07 01:08:52

by Stella Bloom

[permalink] [raw]
Subject: [PATCH v5 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops

> From: Mustafa Ekşi <[email protected]>
>
> Hi,
> I want to note that moving mutex_init to the bottom of the function
> crashes the driver when mutex_lock is called. I didn't investigate it
> further but I wanted to say that since Ai Chao also did it like that.
>
> Driver sets all leds to white on start. Before that, when a led's
> brightness is changed, that led's color gets set to white but others
> keep their old colors which creates a bad user experience (at least for
> me). Please inform me if this is a bad approach.
> Also, this driver still lacks support for changing modes and I seek
> advise for that.
>
> 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 | 641 ++++++++++++++++++++++++++++++
> 4 files changed, 662 insertions(+)
> create mode 100644 drivers/platform/x86/casper-wmi.c
>

Hi there,

I just wanted to pitch in by testing the driver on the kernel I use
on my Arch install on an Excalibur G770.1245, namely xdevs23's
linux-nitrous (https://gitlab.com/xdevs23/linux-nitrous), but trying to
compile the driver using LLVM, which is the default compilation behavior
in this kernel's AUR package, spits out the following error;
```
drivers/platform/x86/casper-wmi.c:633:3: error: field designator 'no_singleton' does not refer to any field in type 'struct wmi_driver'
633 | .no_singleton = true,
| ~^~~~~~~~~~~~~~~~~~~
1 error generated.
make[5]: *** [scripts/Makefile.build:243: drivers/platform/x86/casper-wmi.o] Error 1
make[4]: *** [scripts/Makefile.build:481: drivers/platform/x86] Error 2
make[3]: *** [scripts/Makefile.build:481: drivers/platform] Error 2
make[2]: *** [scripts/Makefile.build:481: drivers] Error 2
make[1]: *** [/home/stella/.cache/yay/linux-nitrous/src/linux-nitrous/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
```

I want to help debug this somehow, but I'm more of an Android custom
ROM developer than a Linux kernel maintainer, so my knowledge on the
programming and build system languages other than Java, Makefile, Bash,
etc is pretty much limited if not outright non-existent.

I would *love* to see this driver actually hit mainline repos, and
eventually the upcoming kernel releases, given how much I need to use
this laptop of mine as a computer engineering student.

Asking just for the case I manage to get this driver up and going on
my end somehow: Is there a tool made for controlling the LED colors yet?
I can still use CLI tools much like on ASUS ROG series laptops, but it
would be much easier and more appreciated to have a GUI provided
Excalibur series laptops' LED lights can virtually take any color in
the RGB space - At least that's how I interpreted with the
configurations I used to do on mine using Excalibur Control Center
on Windows 10/11.

And as for the profiles, let me make sure we're talking about the same
thing in this term: You're talking about the "Office", "Gaming" and
"High Performance" modes as seen in Excalibur Control Center, right?
If so, can this be somehow integrated into `power-profiles-daemon`
SystemD service for easier controlling with GNOME and other DEs that
use it? It's fine if it can't be, this was just a thought struck on my
mind for whatever reason.

Please do CC me and the people I've added to the CC list with this email
of mine on the upcoming revisions, if any. We would love to keep track
of this driver and I personally would love to contribute into testing
as a power user.

Cc: Alviro Iskandar Setiawan <[email protected]>
Cc: Ammar Faizi <[email protected]>
Cc: GNU/Weeb Mailing List <[email protected]>

Also adding my organizational and school email addresses to the CC list
so I can still be notified while I stay offline on this email address.
GNOME Evolution doesn't run in the background and periodically check
for emails sadly, and I switch ROMs every now and then on my phone as a
source maintainer of 3 different custom ROMs. :/

Cc: Stella Bloom <[email protected]>
Cc: Bedirhan KURT <[email protected]>

--
Stella Bloom

2024-04-08 10:01:31

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v5 1/1] platform/x86: Add wmi driver for Casper Excalibur laptops

Hi Mustafa,

On 3/24/24 7:12 PM, mustafa wrote:
> From: Mustafa Ekşi <[email protected]>
>
> This wmi driver supports Casper Excalibur laptops' changing keyboard
> backlight, reading fan speeds and changing power profiles. Multicolor
> led device is used for backlight, platform_profile for power management
> and hwmon for fan speeds. It supports both old (10th gen or older) and
> new (11th gen or newer) laptops. It uses x86_match_cpu to check if the
> laptop is old or new.
> This driver's Multicolor keyboard backlight API is very similar to Rishit
> Bansal's proposed API.
>
> Signed-off-by: Mustafa Ekşi <[email protected]>

Thank you for your patch.

Overall this looks pretty good I have one important remark
about the LED names and some small remarks inline.

After those are addressed I believe this is ready for merging.

<snip>

> +#define CASPER_LED_COUNT 4
> +
> +static const char * const zone_names[CASPER_LED_COUNT] = {
> + "casper::kbd_zoned_backlight-right",
> + "casper::kbd_zoned_backlight-middle",
> + "casper::kbd_zoned_backlight-left",
> + "casper::kbd_zoned_backlight-corners",
> +};

So these names should be:

static const char * const zone_names[CASPER_LED_COUNT] = {
"casper:rgb:kbd_zoned_backlight-right",
"casper:rgb:kbd_zoned_backlight-middle",
"casper:rgb:kbd_zoned_backlight-left",
"casper:rgb:kbd_zoned_backlight-corners",
};

with that change I think this is fine, but we really need
to get an ack for this from the LED subsys maintainers.

Please add a second patch to this patch-serieas adding some
documentation about the use of these names for zoned RGB backlit
kbds in a new paragraph / subsection of the "LED Device Naming"
section of:

Documentation/leds/leds-class.rst

It seems there are 2 possible sets which we should
probably document in one go:

The set of 4 zones from this patch:

:rgb:kbd_zoned_backlight-right
:rgb:kbd_zoned_backlight-middle
:rgb:kbd_zoned_backlight-left
:rgb:kbd_zoned_backlight-corners

As well as:

:rgb:kbd_zoned_backlight-main
:rgb:kbd_zoned_backlight-wasd
:rgb:kbd_zoned_backlight-cursor
:rgb:kbd_zoned_backlight-numpad

Maybe with a comment that in the future different
zone names are possible if keyboards with
a different zoning scheme show up.

> +static int casper_set(struct casper_drv *drv, u16 a1, u8 led_id, u32 data)
> +{
> + acpi_status ret = 0;
> + struct casper_wmi_args wmi_args;
> + struct acpi_buffer input;

Please use a separate acpi_status and ret variables here
with the correct types:

struct casper_wmi_args wmi_args;
struct acpi_buffer input;
acpi_status status;
int ret = 0;


> +
> + wmi_args = (struct casper_wmi_args) {
> + .a0 = CASPER_WRITE,
> + .a1 = a1,
> + .a2 = led_id,
> + .a3 = data
> + };
> +
> + input = (struct acpi_buffer) {
> + (acpi_size) sizeof(struct casper_wmi_args),
> + &wmi_args
> + };
> +
> + mutex_lock(&drv->casper_mutex);
> +

And then here:

status = wmidev_block_set(drv->wdev, 0, &input);
if (ACPI_FAILURE(status))
ret = -EIO;

> +
> + mutex_unlock(&drv->casper_mutex);
> + return ret;
> +}
> +
> +static int casper_query(struct casper_drv *drv, u16 a1,
> + struct casper_wmi_args *out)
> +{

Same here, also please sort variable declarations
in reverse christmas tree order, so longest lines first:


struct casper_wmi_args wmi_args;
struct acpi_buffer input;
union acpi_object *obj;
acpi_status status;
int ret = 0;


> + wmi_args = (struct casper_wmi_args) {
> + .a0 = CASPER_READ,
> + .a1 = a1
> + };
> + input = (struct acpi_buffer) {
> + (acpi_size) sizeof(struct casper_wmi_args),
> + &wmi_args
> + };
> +
> + mutex_lock(&drv->casper_mutex);
> +

And use status here to store the acpi_status type
returned by wmidev_block_set().

> + ret = wmidev_block_set(drv->wdev, 0, &input);
> + if (ACPI_FAILURE(ret)) {
> + ret = -EIO;
> + goto unlock;
> + }
> +
> + obj = wmidev_block_query(drv->wdev, 0);
> + if (!obj) {
> + ret = -EIO;
> + goto unlock;
> + }
> +
> + if (obj->type != ACPI_TYPE_BUFFER) { // obj will be 0x10 on failure
> + ret = -EINVAL;
> + goto freeobj;
> + }
> + if (obj->buffer.length != sizeof(struct casper_wmi_args)) {
> + ret = -EIO;
> + goto freeobj;
> + }
> +
> + memcpy(out, obj->buffer.pointer, sizeof(struct casper_wmi_args));
> +
> +freeobj:
> + kfree(obj);
> +unlock:
> + mutex_unlock(&drv->casper_mutex);
> + return ret;
> +}

<snip>

The MODULE_DEVICE_TABLE() line should be directly below the declaration of
the table like this:


static const struct wmi_device_id casper_wmi_id_table[] = {
{ CASPER_WMI_GUID, NULL },
{ }
};
MODULE_DEVICE_TABLE(wmi, casper_wmi_id_table);

Regards,

Hans




2024-04-08 15:23:52

by Mustafa Ekşi

[permalink] [raw]
Subject: Re: [PATCH v5 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops

On 7.04.2024 03:57, Stella Bloom wrote:
>> From: Mustafa Ekşi <[email protected]>
>>
>> Hi,
>> I want to note that moving mutex_init to the bottom of the function
>> crashes the driver when mutex_lock is called. I didn't investigate it
>> further but I wanted to say that since Ai Chao also did it like that.
>>
>> Driver sets all leds to white on start. Before that, when a led's
>> brightness is changed, that led's color gets set to white but others
>> keep their old colors which creates a bad user experience (at least for
>> me). Please inform me if this is a bad approach.
>> Also, this driver still lacks support for changing modes and I seek
>> advise for that.
>>
>> 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 | 641 ++++++++++++++++++++++++++++++
>> 4 files changed, 662 insertions(+)
>> create mode 100644 drivers/platform/x86/casper-wmi.c
>>
> Hi there,
>
> I just wanted to pitch in by testing the driver on the kernel I use
> on my Arch install on an Excalibur G770.1245, namely xdevs23's
> linux-nitrous (https://gitlab.com/xdevs23/linux-nitrous), but trying to
> compile the driver using LLVM, which is the default compilation behavior
> in this kernel's AUR package, spits out the following error;
> ```
> drivers/platform/x86/casper-wmi.c:633:3: error: field designator 'no_singleton' does not refer to any field in type 'struct wmi_driver'
> 633 | .no_singleton = true,
> | ~^~~~~~~~~~~~~~~~~~~
> 1 error generated.
> make[5]: *** [scripts/Makefile.build:243: drivers/platform/x86/casper-wmi.o] Error 1
> make[4]: *** [scripts/Makefile.build:481: drivers/platform/x86] Error 2
> make[3]: *** [scripts/Makefile.build:481: drivers/platform] Error 2
> make[2]: *** [scripts/Makefile.build:481: drivers] Error 2
> make[1]: *** [/home/stella/.cache/yay/linux-nitrous/src/linux-nitrous/Makefile:1919: .] Error 2
> make: *** [Makefile:240: __sub-make] Error 2
> ```
>
> I want to help debug this somehow, but I'm more of an Android custom
> ROM developer than a Linux kernel maintainer, so my knowledge on the
> programming and build system languages other than Java, Makefile, Bash,
> etc is pretty much limited if not outright non-existent.
Hi,
This is because of a newly merged patch from Armin Wolf:
https://lore.kernel.org/platform-driver-x86/[email protected]/
You can comment that line or apply that patch to your tree to make it
compile. Also, you'll probablyneed to change the call to wmidev_block_set in
casper_query function with wmi_set_block (which is now deprecated).
> I would *love* to see this driver actually hit mainline repos, and
> eventually the upcoming kernel releases, given how much I need to use
> this laptop of mine as a computer engineering student.
>
> Asking just for the case I manage to get this driver up and going on
> my end somehow: Is there a tool made for controlling the LED colors yet?
> I can still use CLI tools much like on ASUS ROG series laptops, but it
> would be much easier and more appreciated to have a GUI provided
> Excalibur series laptops' LED lights can virtually take any color in
> the RGB space - At least that's how I interpreted with the
> configurations I used to do on mine using Excalibur Control Center
> on Windows 10/11.
No, there isn't a tool yet but controlling leds via sysfs ispretty easy.
For example, if you wanted to change the left led zone's color to red:
```
# echo 0xff0000 > /sys/class/leds/casper\:\:kbd_zoned_backlight-left/multi_intensity
```
And don't forget that all leds' initial brightnesses are 0.
Also, I'm planning to add support for this API in OpenRGB.
> And as for the profiles, let me make sure we're talking about the same
> thing in this term: You're talking about the "Office", "Gaming" and
> "High Performance" modes as seen in Excalibur Control Center, right?
For laptops with 11th gen processors or newer: yes.
For laptops with 10th gen processors or older: no, there are 4 power
profiles for these laptops (High Performance, Gaming, Text Mode andPower
save).
> If so, can this be somehow integrated into `power-profiles-daemon`
> SystemD service for easier controlling with GNOME and other DEs that
> use it? It's fine if it can't be, this was just a thought struck on my
> mind for whatever reason.
Yes, power-profiles-daemon is already integrated with platform_profile.
> Please do CC me and the people I've added to the CC list with this email
> of mine on the upcoming revisions, if any. We would love to keep track
> of this driver and I personally would love to contribute into testing
> as a power user.
>
> Cc: Alviro Iskandar Setiawan <[email protected]>
> Cc: Ammar Faizi <[email protected]>
> Cc: GNU/Weeb Mailing List <[email protected]>
>
> Also adding my organizational and school email addresses to the CC list
> so I can still be notified while I stay offline on this email address.
> GNOME Evolution doesn't run in the background and periodically check
> for emails sadly, and I switch ROMs every now and then on my phone as a
> source maintainer of 3 different custom ROMs. :/
>
> Cc: Stella Bloom <[email protected]>
> Cc: Bedirhan KURT <[email protected]>
>
> --
> Stella Bloom
Thanks for your interest,
Mustafa Ekşi

2024-04-08 16:36:45

by Stella Bloom

[permalink] [raw]
Subject: Re: [PATCH v5 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops

On Mon, 2024-04-08 at 18:23 +0300, Mustafa Ekşi wrote:
> On 7.04.2024 03:57, Stella Bloom wrote:
>>> From: Mustafa Ekşi <[email protected]>
>>>
>>> Hi,
>>> I want to note that moving mutex_init to the bottom of the function
>>> crashes the driver when mutex_lock is called. I didn't investigate it
>>> further but I wanted to say that since Ai Chao also did it like that.
>>>
>>> Driver sets all leds to white on start. Before that, when a led's
>>> brightness is changed, that led's color gets set to white but others
>>> keep their old colors which creates a bad user experience (at least for
>>> me). Please inform me if this is a bad approach.
>>> Also, this driver still lacks support for changing modes and I seek
>>> advise for that.
>>>
>>> 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 | 641 ++++++++++++++++++++++++++++++
>>> 4 files changed, 662 insertions(+)
>>> create mode 100644 drivers/platform/x86/casper-wmi.c
>>>
>> Hi there,
>>
>> I just wanted to pitch in by testing the driver on the kernel I use
>> on my Arch install on an Excalibur G770.1245, namely xdevs23's
>> linux-nitrous (https://gitlab.com/xdevs23/linux-nitrous), but trying to
>> compile the driver using LLVM, which is the default compilation behavior
>> in this kernel's AUR package, spits out the following error;
>> ```
>> drivers/platform/x86/casper-wmi.c:633:3: error: field designator 'no_singleton' does not refer to any field in type 'struct wmi_driver'
>> 633 | .no_singleton = true,
>> | ~^~~~~~~~~~~~~~~~~~~
>> 1 error generated.
>> make[5]: *** [scripts/Makefile.build:243: drivers/platform/x86/casper-wmi.o] Error 1
>> make[4]: *** [scripts/Makefile.build:481: drivers/platform/x86] Error 2
>> make[3]: *** [scripts/Makefile.build:481: drivers/platform] Error 2
>> make[2]: *** [scripts/Makefile.build:481: drivers] Error 2
>> make[1]: *** [/home/stella/.cache/yay/linux-nitrous/src/linux-nitrous/Makefile:1919: .] Error 2
>> make: *** [Makefile:240: __sub-make] Error 2
>> ```
>>
>> I want to help debug this somehow, but I'm more of an Android custom
>> ROM developer than a Linux kernel maintainer, so my knowledge on the
>> programming and build system languages other than Java, Makefile, Bash,
>> etc is pretty much limited if not outright non-existent.
> Hi,
> This is because of a newly merged patch from Armin Wolf:
> https://lore.kernel.org/platform-driver-x86/[email protected]/
> You can comment that line or apply that patch to your tree to make it
> compile. Also, you'll probablyneed to change the call to wmidev_block_set in
> casper_query function with wmi_set_block (which is now deprecated).
Well, I prefer not to touch the driver itself, so I already resorted to
picking the patch over the latest RC, which is v6.9-rc2 as of now, and
got onto compiling `linux-mainline` AUR package with it. It will be
kind of a hassle considering how I have to write systemd-boot entries
after the installation to get the kernel to appear (one for normal
initramfs and the other for fallback one) and sign the kernel image
using `sbctl` so I don't fail secure boot, but I'm willing to go
through it just for the sake of seeing this driver in action without
bugs related to the "backport" modifications I would do to it.
>> I would *love* to see this driver actually hit mainline repos, and
>> eventually the upcoming kernel releases, given how much I need to use
>> this laptop of mine as a computer engineering student.
>>
>> Asking just for the case I manage to get this driver up and going on
>> my end somehow: Is there a tool made for controlling the LED colors yet?
>> I can still use CLI tools much like on ASUS ROG series laptops, but it
>> would be much easier and more appreciated to have a GUI provided
>> Excalibur series laptops' LED lights can virtually take any color in
>> the RGB space - At least that's how I interpreted with the
>> configurations I used to do on mine using Excalibur Control Center
>> on Windows 10/11.
> No, there isn't a tool yet but controlling leds via sysfs ispretty easy.
> For example, if you wanted to change the left led zone's color to red:
> ```
> # echo 0xff0000 > /sys/class/leds/casper\:\:kbd_zoned_backlight-left/multi_intensity
> ```
Oh so the LED zones are in different sysfs directories, that's pretty
good. I might code a simple Bash script to make things easier later
down the road.
> And don't forget that all leds' initial brightnesses are 0.
Yeah I think I read that somewhere in the initial message. Can't I
change the brightness of the LEDs using Fn+Space anyway if I can't find
the sysfs entries for that? At least it works just fine on the latest
stable release - v6.8.4.
> Also, I'm planning to add support for this API in OpenRGB.
That's pretty nice to hear! If you need someone to test it out on a
12th gen G770, I'm more than willing to do so!
>> And as for the profiles, let me make sure we're talking about the same
>> thing in this term: You're talking about the "Office", "Gaming" and
>> "High Performance" modes as seen in Excalibur Control Center, right?
> For laptops with 11th gen processors or newer: yes.
> For laptops with 10th gen processors or older: no, there are 4 power
> profiles for these laptops (High Performance, Gaming, Text Mode andPower
> save).
Oh so that's a yes in my case as my laptop has a 12th gen processor.
Glad to know.
>> If so, can this be somehow integrated into `power-profiles-daemon`
>> SystemD service for easier controlling with GNOME and other DEs that
>> use it? It's fine if it can't be, this was just a thought struck on my
>> mind for whatever reason.
> Yes, power-profiles-daemon is already integrated with platform_profile.
Now that's exciting to hear. I haven't seen a laptop that has its power
profiles integrated into the system with a driver in terms of Linux...
At least on the Monster and ASUS laptops I've tried Ubuntu on IIRC.
>> Please do CC me and the people I've added to the CC list with this email
>> of mine on the upcoming revisions, if any. We would love to keep track
>> of this driver and I personally would love to contribute into testing
>> as a power user.
>>
>> Cc: Alviro Iskandar Setiawan <[email protected]>
>> Cc: Ammar Faizi <[email protected]>
>> Cc: GNU/Weeb Mailing List <[email protected]>
>>
>> Also adding my organizational and school email addresses to the CC list
>> so I can still be notified while I stay offline on this email address.
>> GNOME Evolution doesn't run in the background and periodically check
>> for emails sadly, and I switch ROMs every now and then on my phone as a
>> source maintainer of 3 different custom ROMs. :/
>>
>> Cc: Stella Bloom <[email protected]>
>> Cc: Bedirhan KURT <[email protected]>
>>
>> --
>> Stella Bloom
> Thanks for your interest,
> Mustafa Ekşi

Also I apologize for the previous (empty) email. I forgot to put one
newline after the "Subject" line, which caused git-send-email to not
pick up the email content.

--
Stella Bloom

2024-04-08 18:58:36

by Armin Wolf

[permalink] [raw]
Subject: Re: [PATCH v5 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops

Am 08.04.24 um 18:13 schrieb Stella Bloom:

> On Mon, 2024-04-08 at 18:23 +0300, Mustafa Ekşi wrote:
>> On 7.04.2024 03:57, Stella Bloom wrote:
>>>> From: Mustafa Ekşi <[email protected]>
>>>>
>>>> Hi,
>>>> I want to note that moving mutex_init to the bottom of the function
>>>> crashes the driver when mutex_lock is called. I didn't investigate it
>>>> further but I wanted to say that since Ai Chao also did it like that.
>>>>
>>>> Driver sets all leds to white on start. Before that, when a led's
>>>> brightness is changed, that led's color gets set to white but others
>>>> keep their old colors which creates a bad user experience (at least for
>>>> me). Please inform me if this is a bad approach.
>>>> Also, this driver still lacks support for changing modes and I seek
>>>> advise for that.
>>>>
>>>> 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 | 641 ++++++++++++++++++++++++++++++
>>>> 4 files changed, 662 insertions(+)
>>>> create mode 100644 drivers/platform/x86/casper-wmi.c
>>>>
>>> Hi there,
>>>
>>> I just wanted to pitch in by testing the driver on the kernel I use
>>> on my Arch install on an Excalibur G770.1245, namely xdevs23's
>>> linux-nitrous (https://gitlab.com/xdevs23/linux-nitrous), but trying to
>>> compile the driver using LLVM, which is the default compilation behavior
>>> in this kernel's AUR package, spits out the following error;
>>> ```
>>> drivers/platform/x86/casper-wmi.c:633:3: error: field designator 'no_singleton' does not refer to any field in type 'struct wmi_driver'
>>> 633 | .no_singleton = true,
>>> | ~^~~~~~~~~~~~~~~~~~~
>>> 1 error generated.
>>> make[5]: *** [scripts/Makefile.build:243: drivers/platform/x86/casper-wmi.o] Error 1
>>> make[4]: *** [scripts/Makefile.build:481: drivers/platform/x86] Error 2
>>> make[3]: *** [scripts/Makefile.build:481: drivers/platform] Error 2
>>> make[2]: *** [scripts/Makefile.build:481: drivers] Error 2
>>> make[1]: *** [/home/stella/.cache/yay/linux-nitrous/src/linux-nitrous/Makefile:1919: .] Error 2
>>> make: *** [Makefile:240: __sub-make] Error 2
>>> ```
>>>
>>> I want to help debug this somehow, but I'm more of an Android custom
>>> ROM developer than a Linux kernel maintainer, so my knowledge on the
>>> programming and build system languages other than Java, Makefile, Bash,
>>> etc is pretty much limited if not outright non-existent.
>> Hi,
>> This is because of a newly merged patch from Armin Wolf:
>> https://lore.kernel.org/platform-driver-x86/[email protected]/
>> You can comment that line or apply that patch to your tree to make it
>> compile. Also, you'll probablyneed to change the call to wmidev_block_set in
>> casper_query function with wmi_set_block (which is now deprecated).
> Well, I prefer not to touch the driver itself, so I already resorted to
> picking the patch over the latest RC, which is v6.9-rc2 as of now, and
> got onto compiling `linux-mainline` AUR package with it. It will be
> kind of a hassle considering how I have to write systemd-boot entries
> after the installation to get the kernel to appear (one for normal
> initramfs and the other for fallback one) and sign the kernel image
> using `sbctl` so I don't fail secure boot, but I'm willing to go
> through it just for the sake of seeing this driver in action without
> bugs related to the "backport" modifications I would do to it.

Hi,

if you use kernel 6.9-rc2, then wmidev_block_set() is already available, so you do not
have to change that.

You just have to comment out the line with the no_singleton flag, then the driver should
work.

Thanks,
Armin Wolf

>>> I would *love* to see this driver actually hit mainline repos, and
>>> eventually the upcoming kernel releases, given how much I need to use
>>> this laptop of mine as a computer engineering student.
>>>
>>> Asking just for the case I manage to get this driver up and going on
>>> my end somehow: Is there a tool made for controlling the LED colors yet?
>>> I can still use CLI tools much like on ASUS ROG series laptops, but it
>>> would be much easier and more appreciated to have a GUI provided
>>> Excalibur series laptops' LED lights can virtually take any color in
>>> the RGB space - At least that's how I interpreted with the
>>> configurations I used to do on mine using Excalibur Control Center
>>> on Windows 10/11.
>> No, there isn't a tool yet but controlling leds via sysfs ispretty easy.
>> For example, if you wanted to change the left led zone's color to red:
>> ```
>> # echo 0xff0000 > /sys/class/leds/casper\:\:kbd_zoned_backlight-left/multi_intensity
>> ```
> Oh so the LED zones are in different sysfs directories, that's pretty
> good. I might code a simple Bash script to make things easier later
> down the road.
>> And don't forget that all leds' initial brightnesses are 0.
> Yeah I think I read that somewhere in the initial message. Can't I
> change the brightness of the LEDs using Fn+Space anyway if I can't find
> the sysfs entries for that? At least it works just fine on the latest
> stable release - v6.8.4.
>> Also, I'm planning to add support for this API in OpenRGB.
> That's pretty nice to hear! If you need someone to test it out on a
> 12th gen G770, I'm more than willing to do so!
>>> And as for the profiles, let me make sure we're talking about the same
>>> thing in this term: You're talking about the "Office", "Gaming" and
>>> "High Performance" modes as seen in Excalibur Control Center, right?
>> For laptops with 11th gen processors or newer: yes.
>> For laptops with 10th gen processors or older: no, there are 4 power
>> profiles for these laptops (High Performance, Gaming, Text Mode andPower
>> save).
> Oh so that's a yes in my case as my laptop has a 12th gen processor.
> Glad to know.
>>> If so, can this be somehow integrated into `power-profiles-daemon`
>>> SystemD service for easier controlling with GNOME and other DEs that
>>> use it? It's fine if it can't be, this was just a thought struck on my
>>> mind for whatever reason.
>> Yes, power-profiles-daemon is already integrated with platform_profile.
> Now that's exciting to hear. I haven't seen a laptop that has its power
> profiles integrated into the system with a driver in terms of Linux...
> At least on the Monster and ASUS laptops I've tried Ubuntu on IIRC.
>>> Please do CC me and the people I've added to the CC list with this email
>>> of mine on the upcoming revisions, if any. We would love to keep track
>>> of this driver and I personally would love to contribute into testing
>>> as a power user.
>>>
>>> Cc: Alviro Iskandar Setiawan <[email protected]>
>>> Cc: Ammar Faizi <[email protected]>
>>> Cc: GNU/Weeb Mailing List <[email protected]>
>>>
>>> Also adding my organizational and school email addresses to the CC list
>>> so I can still be notified while I stay offline on this email address.
>>> GNOME Evolution doesn't run in the background and periodically check
>>> for emails sadly, and I switch ROMs every now and then on my phone as a
>>> source maintainer of 3 different custom ROMs. :/
>>>
>>> Cc: Stella Bloom <[email protected]>
>>> Cc: Bedirhan KURT <[email protected]>
>>>
>>> --
>>> Stella Bloom
>> Thanks for your interest,
>> Mustafa Ekşi
> Also I apologize for the previous (empty) email. I forgot to put one
> newline after the "Subject" line, which caused git-send-email to not
> pick up the email content.
>
> --
> Stella Bloom
>

2024-04-08 19:33:10

by Stella Bloom

[permalink] [raw]
Subject: Re: [PATCH v5 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops

> Am 08.04.24 um 18:13 schrieb Stella Bloom:
>
>> On Mon, 2024-04-08 at 18:23 +0300, Mustafa Ekşi wrote:
>>> On 7.04.2024 03:57, Stella Bloom wrote:
>>>>> From: Mustafa Ekşi <[email protected]>
>>>>>
>>>>> Hi,
>>>>> I want to note that moving mutex_init to the bottom of the function
>>>>> crashes the driver when mutex_lock is called. I didn't investigate it
>>>>> further but I wanted to say that since Ai Chao also did it like that.
>>>>>
>>>>> Driver sets all leds to white on start. Before that, when a led's
>>>>> brightness is changed, that led's color gets set to white but others
>>>>> keep their old colors which creates a bad user experience (at least for
>>>>> me). Please inform me if this is a bad approach.
>>>>> Also, this driver still lacks support for changing modes and I seek
>>>>> advise for that.
>>>>>
>>>>> 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 | 641 ++++++++++++++++++++++++++++++
>>>>> 4 files changed, 662 insertions(+)
>>>>> create mode 100644 drivers/platform/x86/casper-wmi.c
>>>>>
>>>> Hi there,
>>>>
>>>> I just wanted to pitch in by testing the driver on the kernel I use
>>>> on my Arch install on an Excalibur G770.1245, namely xdevs23's
>>>> linux-nitrous (https://gitlab.com/xdevs23/linux-nitrous), but trying to
>>>> compile the driver using LLVM, which is the default compilation behavior
>>>> in this kernel's AUR package, spits out the following error;
>>>> ```
>>>> drivers/platform/x86/casper-wmi.c:633:3: error: field designator 'no_singleton' does not refer to any field in type 'struct wmi_driver'
>>>> 633 | .no_singleton = true,
>>>> | ~^~~~~~~~~~~~~~~~~~~
>>>> 1 error generated.
>>>> make[5]: *** [scripts/Makefile.build:243: drivers/platform/x86/casper-wmi.o] Error 1
>>>> make[4]: *** [scripts/Makefile.build:481: drivers/platform/x86] Error 2
>>>> make[3]: *** [scripts/Makefile.build:481: drivers/platform] Error 2
>>>> make[2]: *** [scripts/Makefile.build:481: drivers] Error 2
>>>> make[1]: *** [/home/stella/.cache/yay/linux-nitrous/src/linux-nitrous/Makefile:1919: .] Error 2
>>>> make: *** [Makefile:240: __sub-make] Error 2
>>>> ```
>>>>
>>>> I want to help debug this somehow, but I'm more of an Android custom
>>>> ROM developer than a Linux kernel maintainer, so my knowledge on the
>>>> programming and build system languages other than Java, Makefile, Bash,
>>>> etc is pretty much limited if not outright non-existent.
>>> Hi,
>>> This is because of a newly merged patch from Armin Wolf:
>>> https://lore.kernel.org/platform-driver-x86/[email protected]/
>>> You can comment that line or apply that patch to your tree to make it
>>> compile. Also, you'll probablyneed to change the call to wmidev_block_set in
>>> casper_query function with wmi_set_block (which is now deprecated).
>> Well, I prefer not to touch the driver itself, so I already resorted to
>> picking the patch over the latest RC, which is v6.9-rc2 as of now, and
>> got onto compiling `linux-mainline` AUR package with it. It will be
>> kind of a hassle considering how I have to write systemd-boot entries
>> after the installation to get the kernel to appear (one for normal
>> initramfs and the other for fallback one) and sign the kernel image
>> using `sbctl` so I don't fail secure boot, but I'm willing to go
>> through it just for the sake of seeing this driver in action without
>> bugs related to the "backport" modifications I would do to it.
>
> Hi,
>
> if you use kernel 6.9-rc2, then wmidev_block_set() is already available, so you do not
> have to change that.
>
> You just have to comment out the line with the no_singleton flag, then the driver should
> work.

Hi,

Thanks for letting me know of that. I'm doing the change locally right away.

> Thanks,
> Armin Wolf
>
>>>> I would *love* to see this driver actually hit mainline repos, and
>>>> eventually the upcoming kernel releases, given how much I need to use
>>>> this laptop of mine as a computer engineering student.
>>>>
>>>> Asking just for the case I manage to get this driver up and going on
>>>> my end somehow: Is there a tool made for controlling the LED colors yet?
>>>> I can still use CLI tools much like on ASUS ROG series laptops, but it
>>>> would be much easier and more appreciated to have a GUI provided
>>>> Excalibur series laptops' LED lights can virtually take any color in
>>>> the RGB space - At least that's how I interpreted with the
>>>> configurations I used to do on mine using Excalibur Control Center
>>>> on Windows 10/11.
>>> No, there isn't a tool yet but controlling leds via sysfs ispretty easy.
>>> For example, if you wanted to change the left led zone's color to red:
>>> ```
>>> # echo 0xff0000 > /sys/class/leds/casper\:\:kbd_zoned_backlight-left/multi_intensity
>>> ```
>> Oh so the LED zones are in different sysfs directories, that's pretty
>> good. I might code a simple Bash script to make things easier later
>> down the road.
>>> And don't forget that all leds' initial brightnesses are 0.
>> Yeah I think I read that somewhere in the initial message. Can't I
>> change the brightness of the LEDs using Fn+Space anyway if I can't find
>> the sysfs entries for that? At least it works just fine on the latest
>> stable release - v6.8.4.
>>> Also, I'm planning to add support for this API in OpenRGB.
>> That's pretty nice to hear! If you need someone to test it out on a
>> 12th gen G770, I'm more than willing to do so!
>>>> And as for the profiles, let me make sure we're talking about the same
>>>> thing in this term: You're talking about the "Office", "Gaming" and
>>>> "High Performance" modes as seen in Excalibur Control Center, right?
>>> For laptops with 11th gen processors or newer: yes.
>>> For laptops with 10th gen processors or older: no, there are 4 power
>>> profiles for these laptops (High Performance, Gaming, Text Mode andPower
>>> save).
>> Oh so that's a yes in my case as my laptop has a 12th gen processor.
>> Glad to know.
>>>> If so, can this be somehow integrated into `power-profiles-daemon`
>>>> SystemD service for easier controlling with GNOME and other DEs that
>>>> use it? It's fine if it can't be, this was just a thought struck on my
>>>> mind for whatever reason.
>>> Yes, power-profiles-daemon is already integrated with platform_profile.
>> Now that's exciting to hear. I haven't seen a laptop that has its power
>> profiles integrated into the system with a driver in terms of Linux...
>> At least on the Monster and ASUS laptops I've tried Ubuntu on IIRC.
>>>> Please do CC me and the people I've added to the CC list with this email
>>>> of mine on the upcoming revisions, if any. We would love to keep track
>>>> of this driver and I personally would love to contribute into testing
>>>> as a power user.
>>>>
>>>> Cc: Alviro Iskandar Setiawan <[email protected]>
>>>> Cc: Ammar Faizi <[email protected]>
>>>> Cc: GNU/Weeb Mailing List <[email protected]>
>>>>
>>>> Also adding my organizational and school email addresses to the CC list
>>>> so I can still be notified while I stay offline on this email address.
>>>> GNOME Evolution doesn't run in the background and periodically check
>>>> for emails sadly, and I switch ROMs every now and then on my phone as a
>>>> source maintainer of 3 different custom ROMs. :/
>>>>
>>>> Cc: Stella Bloom <[email protected]>
>>>> Cc: Bedirhan KURT <[email protected]>
>>>>
>>>> --
>>>> Stella Bloom
>>> Thanks for your interest,
>>> Mustafa Ekşi
>> Also I apologize for the previous (empty) email. I forgot to put one
>> newline after the "Subject" line, which caused git-send-email to not
>> pick up the email content.
>>
>> --
>> Stella Bloom

--
Stella Bloom