Re: [PATCH] Add wmi driver support for Casper Excalibur laptops.
From: Guenter Roeck
Date: Wed Feb 21 2024 - 17:55:25 EST
On 2/21/24 14:15, Mustafa Ekşi wrote:
Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@xxxxxxxxx>
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 14 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/casper-wmi.c | 344 ++++++++++++++++++++++++++++++
4 files changed, 365 insertions(+)
create mode 100644 drivers/platform/x86/casper-wmi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ed4d386853..d0142a75d2c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4723,6 +4723,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 <mustafa.eskieksi@xxxxxxxxx>
+L: platform-driver-x86@xxxxxxxxxxxxxxx
+S: Maintained
+F: drivers/platform/x86/casper-wmi.c
+
CAVIUM I2C DRIVER
M: Robert Richter <rric@xxxxxxxxxx>
S: Odd Fixes
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index bdd302274b9..ebef9c9dfb6 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1127,6 +1127,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
+ select NEW_LEDS
+ select LEDS_CLASS
+ 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..aae08202b19
--- /dev/null
+++ b/drivers/platform/x86/casper-wmi.c
@@ -0,0 +1,344 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/acpi.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/hwmon.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <linux/dmi.h>
+#include <acpi/acexcep.h>
+
+MODULE_AUTHOR("Mustafa Ekşi <mustafa.eskieksi@xxxxxxxxx>");
+MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
+MODULE_LICENSE("GPL");
+
+#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
+
+struct casper_wmi_args {
+ u16 a0, a1;
+ u32 a2, a3, a4, a5, a6, a7, a8;
+};
+
+static u32 casper_last_color;
+static u8 casper_last_led;
+
+static acpi_status casper_set(struct wmi_device *wdev, u16 a1, u8 led_id,
+ u32 data)
+{
+ struct casper_wmi_args wmi_args = {
+ .a0 = CASPER_WRITE,
+ .a1 = a1,
+ .a2 = led_id,
+ .a3 = data
+ };
+ struct acpi_buffer input = {
+ (acpi_size) sizeof(struct casper_wmi_args),
+ &wmi_args
+ };
+ return wmidev_block_set(wdev, 0, &input);
+}
+
+static ssize_t led_control_show(struct device *dev, struct device_attribute
+ *attr, char *buf)
+{
+ return sprintf("%u%08x\n", buf, casper_last_led,
+ casper_last_color);
+}
+
+
+// input is formatted as "IMARRGGBB", I: led_id, M: mode, A: brightness, ...
+static ssize_t led_control_store(struct device *dev, struct device_attribute
+ *attr, const char *buf, size_t count)
+{
+ u64 tmp;
+ int ret;
+
+ ret = kstrtou64(buf, 16, &tmp);
What exatly is the point of u64 and kstrtou64() ?
+
+ if (ret)
+ return ret;
+
+ u8 led_id = (tmp >> (8 * 4))&0xFF;
This will result in interesting LED IDs based on u64 input. To me it looks
very much like a poor random number generator. Does this follow wome kind
of LED subsystem API ?
+
+ ret =
+ casper_set(to_wmi_device(dev->parent), CASPER_SET_LED, led_id,
+ (u32) tmp
+ );
Odd line breaks. Does this pass checkpatch ?
+ if (ACPI_FAILURE(ret)) {
+ dev_err(dev, "casper-wmi ACPI status: %d\n", ret);
+ return ret;
The function return code is supposed to be a Linux error code.
As for the functions below, I would reject this patch due to its
logging noise.
+ }
+ if (led_id != CASPER_CORNER_LEDS) {
+ casper_last_color = (u32) tmp;
+ casper_last_led = led_id;
+ }
+ return count;
+}
+
+static DEVICE_ATTR_RW(led_control);
+
+static struct attribute *casper_kbd_led_attrs[] = {
+ &dev_attr_led_control.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(casper_kbd_led);
+
+static void set_casper_backlight_brightness(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ // Setting any of the keyboard leds' brightness sets brightness of all
+ acpi_status ret =
+ casper_set(to_wmi_device(led_cdev->dev->parent), CASPER_SET_LED,
+ CASPER_KEYBOARD_LED_1,
+ (casper_last_color & 0xF0FFFFFF) |
+ (((u32) brightness) << 24)
+ );
+
+ if (ret != 0)
+ dev_err(led_cdev->dev,
+ "Couldn't set brightness acpi status: %d\n", ret);
+}
+
+static enum led_brightness get_casper_backlight_brightness(struct led_classdev
+ *led_cdev)
+{
+ return (casper_last_color&0x0F000000)>>24;
+}
+
+static struct led_classdev casper_kbd_led = {
+ .name = "casper::kbd_backlight",
+ .brightness = 0,
+ .brightness_set = set_casper_backlight_brightness,
+ .brightness_get = get_casper_backlight_brightness,
+ .max_brightness = 2,
+ .groups = casper_kbd_led_groups,
+};
+
+static acpi_status casper_query(struct wmi_device *wdev, u16 a1,
+ struct casper_wmi_args *out)
+{
+ struct casper_wmi_args wmi_args = {
+ .a0 = CASPER_READ,
+ .a1 = a1
+ };
+ struct acpi_buffer input = {
+ (acpi_size) sizeof(struct casper_wmi_args),
+ &wmi_args
+ };
+
+ acpi_status ret = wmidev_block_set(wdev, 0, &input);
+
+ if (ACPI_FAILURE(ret)) {
+ dev_err(&wdev->dev,
+ "Could not query acpi status: %u", ret);
This code generates _way_ too much logging noise for my liking.
+ return ret;
Is there any value in having this function return acpi error
codes instead of Linux error codes ?
+ }
+
+ union acpi_object *obj = wmidev_block_query(wdev, 0);
+
+ if (obj == NULL) {
+ dev_err(&wdev->dev,
+ "Could not query hardware information");
+ return AE_ERROR;
+ }
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ dev_err(&wdev->dev, "Return type is not a buffer");
+ return AE_TYPE;
+ }
+
+ if (obj->buffer.length != 32) {
+ dev_err(&wdev->dev, "Return buffer is not long enough");
+ return AE_ERROR;
+ }
+ memcpy(out, obj->buffer.pointer, 32);
+ kfree(obj);
+ return ret;
+}
+
+static umode_t casper_wmi_hwmon_is_visible(const void *drvdata,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (type) {
+ case hwmon_fan:
+ return 0444;
+ case hwmon_pwm:
+ return 0644;
+ default:
+ return 0;
+ }
+ return 0;
+}
+
+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 };
+
+ switch (type) {
+ case hwmon_fan:
+ acpi_status ret = casper_query(to_wmi_device(dev->parent),
+ CASPER_GET_HARDWAREINFO, &out);
+
+ if (ACPI_FAILURE(ret))
+ return ret;
This function is expected to return a Linux error code, not an acpi error code.
Also, if CASPER_GET_HARDWAREINFO is not always available, the attributes
needing it should not be created in the first place.
+
+ if (channel == 0) { // CPU fan
+ u32 cpu_fanspeed = out.a4;
+
+ cpu_fanspeed <<= 8;
+ cpu_fanspeed += out.a4 >> 8;
+ *val = (long) cpu_fanspeed;
+ } else if (channel == 1) { // GPU fan
+ u32 gpu_fanspeed = out.a5;
+
+ gpu_fanspeed <<= 8;
+ gpu_fanspeed += out.a5 >> 8;
I don't know what this is supposed to be doing, but it will return
odd values. For example, if out.a5 is 0xabcd, the returned value
will be 0xabcdab. That seems to be unlikely I suspect this is supposed
to be
*val = ((out.a5 & 0xff) << 8) | ((out.a5 >> 8) & 0xff);
but I am not even sure about that because a5 is u32 and the above would suggest
a 16-bit unsigned short in big endian format. Please check return values
and implement any necessary endiannes conversion correctly.
+ *val = (long) gpu_fanspeed;
FWIW, those type casts are unnecessary.
+ }
+ return 0;
+ case hwmon_pwm:
+ casper_query(to_wmi_device(dev->parent), CASPER_POWERPLAN,
+ &out);
Why no error check here ?
+ if (channel == 0)
+ *val = (long)out.a2;
+ else
+ return -EOPNOTSUPP;
The coonditional and else case is unnecessary since only
a single pwm channel is declared.
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int casper_wmi_hwmon_read_string(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, const char **str)
+{
+ switch (type) {
+ case hwmon_fan:
+ switch (channel) {
+ case 0:
+ *str = "cpu_fan_speed";
+ break;
+ case 1:
+ *str = "gpu_fan_speed";
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int casper_wmi_hwmon_write(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, long val)
+{
+ acpi_status ret;
+
+ switch (type) {
+ case hwmon_pwm:
+ if (channel != 0)
+ return -EOPNOTSUPP;
This is unnecessary. Only a single pwm channel is declared,
so channel will never be != 0.
+ ret =
+ casper_set(to_wmi_device(dev->parent), CASPER_POWERPLAN,
+ val, 0);
+
The first line split is unnecessary.
+ if (ACPI_FAILURE(ret)) {
+ dev_err(dev, "Couldn't set power plan, acpi_status: %d",
+ ret);
Drivers should not generate such logging noise, even more so after user input.
Also, the valid range (0..255) should be checked before trying to set it.
+ return -EINVAL;
+ }
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+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,
+ .write = &casper_wmi_hwmon_write
+};
+
+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),
+ HWMON_CHANNEL_INFO(pwm, HWMON_PWM_MODE),
+ NULL
+};
+
+static const struct hwmon_chip_info casper_wmi_hwmon_chip_info = {
+ .ops = &casper_wmi_hwmon_ops,
+ .info = casper_wmi_hwmon_info,
+};
+
+static int casper_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct device *hwmon_dev;
+
+ // All Casper Excalibur Laptops use this GUID
+ if (!wmi_has_guid(CASPER_WMI_GUID))
+ return -ENODEV;
+
How would the device ever be instantiated with a different GUID,
making this check necessary ?
+ hwmon_dev =
+ devm_hwmon_device_register_with_info(&wdev->dev, "casper_wmi", wdev,
+ &casper_wmi_hwmon_chip_info,
+ NULL);
+
+ acpi_status result = led_classdev_register(&wdev->dev, &casper_kbd_led);
+
+ if (result != 0)
+ return -ENODEV;
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
This would leave the LED device registered if instantiating the hwmon device
failed. However, the probe function would return an error, meaning the driver
core will believe that instantiation failed. Is that intentional ? I am quite
sure that this would result in interesting crashes.
+ }
+
+static void casper_wmi_remove(struct wmi_device *wdev)
+{
+ led_classdev_unregister(&casper_kbd_led);
+}
+
+static const struct wmi_device_id casper_wmi_id_table[] = {
+ { CASPER_WMI_GUID, NULL },
+ { }
+};
+
+static struct wmi_driver casper_wmi_driver = {
+ .driver = {
+ .name = "casper-wmi",
+ },
+ .id_table = casper_wmi_id_table,
+ .probe = casper_wmi_probe,
+ .remove = &casper_wmi_remove
+};
+
+module_wmi_driver(casper_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, casper_wmi_id_table);