Adding wmi driver for Casper Excalibur Laptops:
This driver implements a ledclass_dev device for keyboard backlight
and hwmon driver to read fan speed and (also write) pwm mode. NEW_LEDS is
selected because this driver introduces new leds, and LEDS_CLASS is selected
because this driver implements a led class device. All of Casper Excalibur
Laptops are supported but fan speeds has a bug for older generations.
Signed-off-by: Mustafa Ekşi <[email protected]>
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 14 ++
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/casper-wmi.c | 315 ++++++++++++++++++++++++++++++
4 files changed, 336 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 <[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 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..012ebda195d
--- /dev/null
+++ b/drivers/platform/x86/casper-wmi.c
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/bitops.h>
+#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/hwmon.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
+#include <acpi/acexcep.h>
+#include <linux/bitfield.h>
+#include <linux/sysfs.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_ID 0xF00000000
+#define CASPER_LED_MODE 0x0F0000000
+#define CASPER_LED_ALPHA 0x00F000000
+
+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 int 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
+ };
+ if (ACPI_FAILURE(wmidev_block_set(wdev, 0, &input)))
+ return -EINVAL;
+ return 0;
+}
+
+static ssize_t led_control_show(struct device *dev, struct device_attribute
+ *attr, char *buf)
+{
+ return sysfs_emit("%u%08x\n", buf, casper_last_led,
+ casper_last_color);
+}
+
+/*
+ * Format wanted from user is a hexadecimal 36-bit integer: most significant
+ * 4 bits are led_id, next 4 bits are mode and next 4 bits are brightness,
+ * next 24 bits are rgb value. 64 bits
+ * IMARRGGBB
+ */
+static ssize_t led_control_store(struct device *dev, struct device_attribute
+ *attr, const char *buf, size_t count)
+{
+ if (strlen(buf) != 10)
+ return -EINVAL;
+ u64 user_input;
+ /*
+ * 16-base selected for ease of writing color codes. I chose 64 bit and
+ * kstrtou64 because format I use determined fits into 64 bit.
+ */
+ int ret = kstrtou64(buf, 16, &user_input);
+ if (ret)
+ return ret;
+ /*
+ * led_id can't exceed 255 but it can vary among newer versions and
+ * other models.
+ */
+ u8 led_id = FIELD_GET(CASPER_LED_ID, user_input);
+ ret = casper_set(to_wmi_device(dev->parent), CASPER_SET_LED,
+ led_id, (u32) user_input);
+ if (ret)
+ return ret;
+ if (led_id != CASPER_CORNER_LEDS) {
+ casper_last_color = (u32) user_input;
+ 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
+ u32 bright_prep = FIELD_PREP(CASPER_LED_ALPHA, brightness);
+ u32 color_no_alpha = casper_last_color&~CASPER_LED_ALPHA;
+
+ casper_set(to_wmi_device(led_cdev->dev->parent), CASPER_SET_LED,
+ CASPER_KEYBOARD_LED_1, color_no_alpha | bright_prep
+ );
+}
+
+static enum led_brightness get_casper_backlight_brightness(struct led_classdev
+ *led_cdev)
+{
+ return FIELD_GET(CASPER_LED_ALPHA, casper_last_color);
+}
+
+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 int 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))
+ return -EIO;
+
+ union acpi_object *obj = wmidev_block_query(wdev, 0);
+ if (obj->type != ACPI_TYPE_BUFFER) // obj will be int (0x10) on failure
+ return -EINVAL;
+ if (obj->buffer.length != 32)
+ return -EIO;
+
+ memcpy(out, obj->buffer.pointer, sizeof(struct casper_wmi_args));
+ 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 };
+ struct wmi_device *wdev = to_wmi_device(dev->parent);
+ int ret;
+
+ switch (type) {
+ case hwmon_fan:
+ ret = casper_query(wdev, CASPER_GET_HARDWAREINFO, &out);
+ /*
+ * a4 and a5 is little endian in older laptops (with 10th gen
+ * cpus or older) and big endian in newer ones. I don't think
+ * dmi has something for cpu information. Also, defining a
+ * dmi_list just for this seems like an overkill. This problem
+ * can be solved in userspace too.
+ */
+ if (channel == 0) // CPU fan
+ *val = out.a4;
+ else if (channel == 1) // GPU fan
+ *val = out.a5;
+ return 0;
+ case hwmon_pwm:
+ ret = casper_query(wdev, CASPER_POWERPLAN, &out);
+ if (ret) // power plan count varies generations.
+ return ret;
+ if (channel == 0)
+ *val = out.a2;
+ return 0;
+ default:
+ return -ENODEV;
+ }
+}
+
+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 -ENODEV;
+ }
+ break;
+ default:
+ return -ENODEV;
+ }
+ 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 (val > 5 || val < 0)
+ return -EINVAL;
+ ret = casper_set(to_wmi_device(dev->parent),
+ CASPER_POWERPLAN, val, 0);
+ if (ret)
+ return ret;
+ 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;
+
+ if (ACPI_FAILURE(led_classdev_register(&wdev->dev, &casper_kbd_led)))
+ return -ENODEV;
+ hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
+ "casper_wmi", wdev,
+ &casper_wmi_hwmon_chip_info,
+ NULL);
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+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);
+
+MODULE_AUTHOR("Mustafa Ekşi <[email protected]>");
+MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
+MODULE_LICENSE("GPL");
--
2.43.2
On 2/22/24 13:48, Mustafa Ekşi wrote:
> Adding wmi driver for Casper Excalibur Laptops:
> This driver implements a ledclass_dev device for keyboard backlight
> and hwmon driver to read fan speed and (also write) pwm mode. NEW_LEDS is
> selected because this driver introduces new leds, and LEDS_CLASS is selected
> because this driver implements a led class device. All of Casper Excalibur
> Laptops are supported but fan speeds has a bug for older generations.
>
"Impressive" subject (I dropped most of it). Really, you should not do that.
> Signed-off-by: Mustafa Ekşi <[email protected]>
>
[ ... ]
> +
> +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 wmi_device *wdev = to_wmi_device(dev->parent);
> + int ret;
> +
> + switch (type) {
> + case hwmon_fan:
> + ret = casper_query(wdev, CASPER_GET_HARDWAREINFO, &out);
Ignoring errors is unacceptable.
> + /*
> + * a4 and a5 is little endian in older laptops (with 10th gen
> + * cpus or older) and big endian in newer ones. I don't think
> + * dmi has something for cpu information. Also, defining a
> + * dmi_list just for this seems like an overkill. This problem
> + * can be solved in userspace too.
> + */
> + if (channel == 0) // CPU fan
> + *val = out.a4;
> + else if (channel == 1) // GPU fan
> + *val = out.a5;
> + return 0;
> + case hwmon_pwm:
> + ret = casper_query(wdev, CASPER_POWERPLAN, &out);
> + if (ret) // power plan count varies generations.
That comment doesn't explain anything. If it is supposed to mean that it is
not supported on all laptop variants, the attribute should not be instantiated
in the first place. I am sure I mentioned this before.
> + return ret;
> + if (channel == 0)
> + *val = out.a2;
There is only one pwm channel. Checking the channel number is pointless.
> + return 0;
> + default:
> + return -ENODEV;
-EOPNOTSUPP
> + }
> +}
> +
> +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 -ENODEV;
-EOPNOTSUPP
> + }
> + break;
> + default:
> + return -ENODEV;
-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 (val > 5 || val < 0)
> + return -EINVAL;
pwm range, per ABI, is 0..255, not 0..5. It is expected to be normalized.
Since you are not using the standard ABI, what are the values returned
as fan speed ? Is that RPM or something else ?
> + ret = casper_set(to_wmi_device(dev->parent),
> + CASPER_POWERPLAN, val, 0);
> + if (ret)
> + return ret;
> + return 0;
return ret;
> + 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;
> +
> + if (ACPI_FAILURE(led_classdev_register(&wdev->dev, &casper_kbd_led)))
I am quite sure that led_classdev_register() doesn't return ACPI error codes.
> + return -ENODEV;
> + hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
> + "casper_wmi", wdev,
> + &casper_wmi_hwmon_chip_info,
> + NULL);
> + return PTR_ERR_OR_ZERO(hwmon_dev);
Why use devm_hwmon_device_register_with_info() but not
devm_led_classdev_register() ?
> +}
> +
> +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);
> +
> +MODULE_AUTHOR("Mustafa Ekşi <[email protected]>");
> +MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
> +MODULE_LICENSE("GPL");
On 2/22/24 1:48 PM, Mustafa Ekşi wrote:
> Adding wmi driver for Casper Excalibur Laptops:
> This driver implements a ledclass_dev device for keyboard backlight
> and hwmon driver to read fan speed and (also write) pwm mode. NEW_LEDS is
> selected because this driver introduces new leds, and LEDS_CLASS is selected
> because this driver implements a led class device. All of Casper Excalibur
> Laptops are supported but fan speeds has a bug for older generations.
Please fix your subject line. It looks like you have included responses
to the previous comments in the subject line. Please go through the
https://docs.kernel.org/process/submitting-patches.html for the
guidelines
>
> Signed-off-by: Mustafa Ekşi <[email protected]>
> ---
> MAINTAINERS | 6 +
> drivers/platform/x86/Kconfig | 14 ++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/casper-wmi.c | 315 ++++++++++++++++++++++++++++++
> 4 files changed, 336 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 <[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 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..012ebda195d
> --- /dev/null
> +++ b/drivers/platform/x86/casper-wmi.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/bitops.h>
> +#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/hwmon.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <acpi/acexcep.h>
> +#include <linux/bitfield.h>
> +#include <linux/sysfs.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_ID 0xF00000000
> +#define CASPER_LED_MODE 0x0F0000000
> +#define CASPER_LED_ALPHA 0x00F000000
> +
> +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 int 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
> + };
I would add an empty line here.
> + if (ACPI_FAILURE(wmidev_block_set(wdev, 0, &input)))
> + return -EINVAL;
Same as above.
> + return 0;
> +}
> +
> +static ssize_t led_control_show(struct device *dev, struct device_attribute
> + *attr, char *buf)
> +{
> + return sysfs_emit("%u%08x\n", buf, casper_last_led,
> + casper_last_color);
> +}
> +
> +/*
> + * Format wanted from user is a hexadecimal 36-bit integer: most significant
> + * 4 bits are led_id, next 4 bits are mode and next 4 bits are brightness,
> + * next 24 bits are rgb value. 64 bits
> + * IMARRGGBB
> + */
> +static ssize_t led_control_store(struct device *dev, struct device_attribute
> + *attr, const char *buf, size_t count)
> +{
> + if (strlen(buf) != 10)
> + return -EINVAL;
Why 10? use a macro for it.
> + u64 user_input;
Please declare the variable at the top of the function.
> + /*
> + * 16-base selected for ease of writing color codes. I chose 64 bit and
> + * kstrtou64 because format I use determined fits into 64 bit.
> + */
> + int ret = kstrtou64(buf, 16, &user_input);
> + if (ret)
> + return ret;
> + /*
> + * led_id can't exceed 255 but it can vary among newer versions and
> + * other models.
> + */
> + u8 led_id = FIELD_GET(CASPER_LED_ID, user_input);
> + ret = casper_set(to_wmi_device(dev->parent), CASPER_SET_LED,
> + led_id, (u32) user_input);
> + if (ret)
> + return ret;
> + if (led_id != CASPER_CORNER_LEDS) {
> + casper_last_color = (u32) user_input;
> + 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
> + u32 bright_prep = FIELD_PREP(CASPER_LED_ALPHA, brightness);
> + u32 color_no_alpha = casper_last_color&~CASPER_LED_ALPHA;
> +
> + casper_set(to_wmi_device(led_cdev->dev->parent), CASPER_SET_LED,
> + CASPER_KEYBOARD_LED_1, color_no_alpha | bright_prep
> + );
> +}
> +
> +static enum led_brightness get_casper_backlight_brightness(struct led_classdev
> + *led_cdev)
> +{
> + return FIELD_GET(CASPER_LED_ALPHA, casper_last_color);
> +}
> +
> +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 int 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))
> + return -EIO;
> +
> + union acpi_object *obj = wmidev_block_query(wdev, 0);
> + if (obj->type != ACPI_TYPE_BUFFER) // obj will be int (0x10) on failure
> + return -EINVAL;
You don't need to free obj here?
> + if (obj->buffer.length != 32)
> + return -EIO;
Please use defines for constant values.
> +
> + memcpy(out, obj->buffer.pointer, sizeof(struct casper_wmi_args));
> + kfree(obj);
Add a empty line here.
> + 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;
How about S_IRUSR | S_IRGRP | S_IROTH ?
> + case hwmon_pwm:
> + return 0644;
Same as above.
> + 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 };
> + struct wmi_device *wdev = to_wmi_device(dev->parent);
> + int ret;
> +
> + switch (type) {
> + case hwmon_fan:
> + ret = casper_query(wdev, CASPER_GET_HARDWAREINFO, &out);
Not checking the ret?
> + /*
> + * a4 and a5 is little endian in older laptops (with 10th gen
> + * cpus or older) and big endian in newer ones. I don't think
> + * dmi has something for cpu information. Also, defining a
> + * dmi_list just for this seems like an overkill. This problem
> + * can be solved in userspace too.
> + */
> + if (channel == 0) // CPU fan
> + *val = out.a4;
> + else if (channel == 1) // GPU fan
> + *val = out.a5;
empty line here
> + return 0;
> + case hwmon_pwm:
> + ret = casper_query(wdev, CASPER_POWERPLAN, &out);
> + if (ret) // power plan count varies generations.
> + return ret;
> + if (channel == 0)
> + *val = out.a2;
> + return 0;
> + default:
> + return -ENODEV;
EINVAL or ENOTSUPP?
> + }
> +}
> +
> +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 -ENODEV;
> + }
> + break;
> + default:
> + return -ENODEV;
> + }
Same as above function
> + 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 (val > 5 || val < 0)
Use macro for constant values. It is not clear why you check for 0..5
> + return -EINVAL;
> + ret = casper_set(to_wmi_device(dev->parent),
> + CASPER_POWERPLAN, val, 0);
> + if (ret)
> + return ret;
> + 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;
> +
> + if (ACPI_FAILURE(led_classdev_register(&wdev->dev, &casper_kbd_led)))
> + return -ENODEV;
> + hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
> + "casper_wmi", wdev,
> + &casper_wmi_hwmon_chip_info,
> + NULL);
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +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);
> +
> +MODULE_AUTHOR("Mustafa Ekşi <[email protected]>");
> +MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
> +MODULE_LICENSE("GPL");
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Fri, 23 Feb 2024, Mustafa Ek?i wrote:
> Adding wmi driver for Casper Excalibur Laptops:
> This driver implements a ledclass_dev device for keyboard backlight
> and hwmon driver to read fan speed and (also write) pwm mode. NEW_LEDS is
> selected because this driver introduces new leds, and LEDS_CLASS is selected
> because this driver implements a led class device. All of Casper Excalibur
> Laptops are supported but fan speeds has a bug for older generations.
>
> Signed-off-by: Mustafa Ek?i <[email protected]>
> ---
v1 -> v2 changelog is missing from here!
> MAINTAINERS | 6 +
> drivers/platform/x86/Kconfig | 14 ++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/casper-wmi.c | 315 ++++++++++++++++++++++++++++++
> 4 files changed, 336 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 <[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 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..012ebda195d
> --- /dev/null
> +++ b/drivers/platform/x86/casper-wmi.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/bitops.h>
> +#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/hwmon.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <acpi/acexcep.h>
> +#include <linux/bitfield.h>
> +#include <linux/sysfs.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_ID 0xF00000000
> +#define CASPER_LED_MODE 0x0F0000000
> +#define CASPER_LED_ALPHA 0x00F000000
GENMASK()
> +
> +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 int 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
> + };
> + if (ACPI_FAILURE(wmidev_block_set(wdev, 0, &input)))
int status;
status = wmidev_block_set(wdev, 0, &input);
if (ACPI_FAILURE(status))
> + return -EINVAL;
> + return 0;
> +}
> +
> +static ssize_t led_control_show(struct device *dev, struct device_attribute
> + *attr, char *buf)
> +{
> + return sysfs_emit("%u%08x\n", buf, casper_last_led,
> + casper_last_color);
I think I mentioned you should put this to one line, why you didn't follow it?
> +}
> +
> +/*
> + * Format wanted from user is a hexadecimal 36-bit integer: most significant
> + * 4 bits are led_id, next 4 bits are mode and next 4 bits are brightness,
> + * next 24 bits are rgb value. 64 bits
> + * IMARRGGBB
> + */
> +static ssize_t led_control_store(struct device *dev, struct device_attribute
> + *attr, const char *buf, size_t count)
Don't split argumetns like this but only at commas.
> +{
> + if (strlen(buf) != 10)
> + return -EINVAL;
> + u64 user_input;
Wrong place for declaration.
> + /*
> + * 16-base selected for ease of writing color codes. I chose 64 bit and
> + * kstrtou64 because format I use determined fits into 64 bit.
> + */
> + int ret = kstrtou64(buf, 16, &user_input);
Don't declare varibles in the middle of nowhere even if that is nowadays
not prevented by the compiler UNLESS you're using the cleanup.h which is
the reason why that is allowed at all.
> + if (ret)
> + return ret;
> + /*
> + * led_id can't exceed 255 but it can vary among newer versions and
> + * other models.
> + */
> + u8 led_id = FIELD_GET(CASPER_LED_ID, user_input);
Add #include <linux/bitfield.h>
> + ret = casper_set(to_wmi_device(dev->parent), CASPER_SET_LED,
> + led_id, (u32) user_input);
> + if (ret)
> + return ret;
> + if (led_id != CASPER_CORNER_LEDS) {
> + casper_last_color = (u32) user_input;
> + casper_last_led = led_id;
> + }
> + return count;
> +}
So codingstylewise you should do this:
static ssize_t led_control_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
struct wdev = to_wmi_device(dev->parent);
u64 user_input;
u8 led_id;
ret = kstrtou64(buf, 16, &user_input);
if (ret)
return ret;
led_id = FIELD_GET(CASPER_LED_ID, user_input);
ret = casper_set(wdev, CASPER_SET_LED, led_id, (u32)user_input);
if (ret)
return ret;
if (led_id ...) {
...
}
return 0;
}
However, I still suspect this is wrong way to do RGB leds and the multi_*
sysfs interface is the way you should use.
> +
> +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
> + u32 bright_prep = FIELD_PREP(CASPER_LED_ALPHA, brightness);
> + u32 color_no_alpha = casper_last_color&~CASPER_LED_ALPHA;
Missing spaces.
> +
> + casper_set(to_wmi_device(led_cdev->dev->parent), CASPER_SET_LED,
Create the wdev local var to avoid need to call to_wmi_device() on this
line like I told you already!
> + CASPER_KEYBOARD_LED_1, color_no_alpha | bright_prep
> + );
> +}
> +
> +static enum led_brightness get_casper_backlight_brightness(struct led_classdev
> + *led_cdev)
> +{
> + return FIELD_GET(CASPER_LED_ALPHA, casper_last_color);
> +}
> +
> +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 int 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);
Put the declaration separately into the declarations block:
acpi_status ret;
ret = wmidev_block_set(wdev, 0, &input);
> + if (ACPI_FAILURE(ret))
> + return -EIO;
> +
> + union acpi_object *obj = wmidev_block_query(wdev, 0);
> + if (obj->type != ACPI_TYPE_BUFFER) // obj will be int (0x10) on failure
> + return -EINVAL;
> + if (obj->buffer.length != 32)
> + return -EIO;
> +
> + memcpy(out, obj->buffer.pointer, sizeof(struct casper_wmi_args));
> + 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 };
> + struct wmi_device *wdev = to_wmi_device(dev->parent);
> + int ret;
> +
> + switch (type) {
> + case hwmon_fan:
> + ret = casper_query(wdev, CASPER_GET_HARDWAREINFO, &out);
> + /*
> + * a4 and a5 is little endian in older laptops (with 10th gen
> + * cpus or older) and big endian in newer ones. I don't think
> + * dmi has something for cpu information. Also, defining a
> + * dmi_list just for this seems like an overkill. This problem
> + * can be solved in userspace too.
> + */
> + if (channel == 0) // CPU fan
> + *val = out.a4;
> + else if (channel == 1) // GPU fan
Instead of comments like this, use defines so you can say:
if (channel == CASPER_FAN_CPU)
...
if (channel == CASPER_FAN_GPU)
> + *val = out.a5;
> + return 0;
> + case hwmon_pwm:
> + ret = casper_query(wdev, CASPER_POWERPLAN, &out);
> + if (ret) // power plan count varies generations.
I fail to see how the comment relates to if (ret) at all because that
looks like error handling!
> + return ret;
> + if (channel == 0)
> + *val = out.a2;
> + return 0;
> + default:
> + return -ENODEV;
> + }
> +}
> +
> +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:
You can use those defines here too I think.
> + *str = "gpu_fan_speed";
> + break;
> + default:
> + return -ENODEV;
> + }
> + break;
> + default:
> + return -ENODEV;
> + }
Do you expect other types? If not, this would be easier to follow:
if (type != hwmon_fan)
return -ENODEV;
switch (channel) {
...
> + 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 (val > 5 || val < 0)
There's in_range() which can be used.
> + return -EINVAL;
> + ret = casper_set(to_wmi_device(dev->parent),
> + CASPER_POWERPLAN, val, 0);
> + if (ret)
> + return ret;
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
Similar structural comment here with if + early return as in the above
function.
> +}
> +
> +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;
> +
> + if (ACPI_FAILURE(led_classdev_register(&wdev->dev, &casper_kbd_led)))
> + return -ENODEV;
> + hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
> + "casper_wmi", wdev,
> + &casper_wmi_hwmon_chip_info,
> + NULL);
> + return PTR_ERR_OR_ZERO(hwmon_dev);
Don't you need to do rollback here for led_classdev_register() if
devm_hwmon_device_register_with_info() fails?
> +}
> +
> +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);
> +
> +MODULE_AUTHOR("Mustafa Ek?i <[email protected]>");
> +MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
> +MODULE_LICENSE("GPL");
>
--
i.