Add wmi driver support for Casper Excalibur laptops.

  Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@gmail.com>
---
 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
  

On 2/21/24 14:15, Mustafa Ekşi wrote:
> Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@gmail.com>
> ---
>   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@gmail.com>
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/casper-wmi.c
> +
>   CAVIUM I2C DRIVER
>   M:	Robert Richter <rric@kernel.org>
>   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@gmail.com>");
> +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);
  

On 2/21/24 2:15 PM, Mustafa Ekşi wrote:
> Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@gmail.com>
> ---

Please add some commit log about the driver. Like what it supports,
who uses it , etc.

>  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@gmail.com>
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/casper-wmi.c
> +
>  CAVIUM I2C DRIVER
>  M:	Robert Richter <rric@kernel.org>
>  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

In commit log, add some info about why you are selecting the NEW_LEDS
and 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@gmail.com>");
> +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);
> +
above empty line is not needed
> +	if (ret)
> +		return ret;
> +
> +	u8 led_id = (tmp >> (8 * 4))&0xFF;
Why not 32 ?
> +
> +	ret =
> +	    casper_set(to_wmi_device(dev->parent), CASPER_SET_LED, led_id,
> +		       (u32) tmp
> +	    );
Since it returns acpi_status, I would define a new variable to test it.
> +	if (ACPI_FAILURE(ret)) {
> +		dev_err(dev, "casper-wmi ACPI status: %d\n", ret);
> +		return ret;
> +	}
> +	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;
> +}

I recommend adding defines for all magic numbers you have used.

> +
> +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);
> +		return ret;
> +	}
> +
> +	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;
> +
> +		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;
> +			*val = (long) gpu_fanspeed;
> +		}
> +		return 0;
> +	case hwmon_pwm:
> +		casper_query(to_wmi_device(dev->parent), CASPER_POWERPLAN,
> +			     &out);
> +		if (channel == 0)
> +			*val = (long)out.a2;
> +		else
> +			return -EOPNOTSUPP;
> +		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;
> +		ret =
> +		    casper_set(to_wmi_device(dev->parent), CASPER_POWERPLAN,
> +			       val, 0);
> +
> +		if (ACPI_FAILURE(ret)) {
> +			dev_err(dev, "Couldn't set power plan, acpi_status: %d",
> +				ret);
> +			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;
> +
> +	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);
> +	}
> +
> +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);
  

Hi,

Added LED subsys people, please include them in future versions 
automatically.

On Thu, 22 Feb 2024, Mustafa Ekşi wrote:

> Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@gmail.com>
> ---
>  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@gmail.com>
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/casper-wmi.c
> +
>  CAVIUM I2C DRIVER
>  M:	Robert Richter <rric@kernel.org>
>  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@gmail.com>");
> +MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
> +MODULE_LICENSE("GPL");

Put these to the end of file.

> +#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);

Fits one line. Use sysfs_emit().

> +}
> +
> +
> +// input is formatted as "IMARRGGBB", I: led_id, M: mode, A: brightness, ...

If you do things like this, please add defines for such "fields" and use 
FIELD_GET/PREP().

Could LED subsystem folks plese check this the correct way to do RGB 
control? (I suspect it's not).

> +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);
> +
> +	if (ret)
> +		return ret;

Don't place empty line between function and its error handling. Please go 
through the entire patch and fix all of them (I won't mark them all).

> +
> +	u8 led_id = (tmp >> (8 * 4))&0xFF;

FIELD_GET() + add #include for it.

> +
> +	ret =
> +	    casper_set(to_wmi_device(dev->parent), CASPER_SET_LED, led_id,
> +		       (u32) tmp

Don't call variable "tmp"!

Please create a local variable for these to_wmi_device(dev->parent) to 
make this fit one line.

> +	    );
> +	if (ACPI_FAILURE(ret)) {
> +		dev_err(dev, "casper-wmi ACPI status: %d\n", ret);
> +		return ret;
> +	}
> +	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)

Use FIELD_PREP().

As you need to split lines, please do the calculations in a local variable 
beforehand and only then call.

> +	    );
> +
> +	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;

FIELD_GET().

> +}
> +
> +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);

One line.

> +		return ret;
> +	}
> +
> +	union acpi_object *obj = wmidev_block_query(wdev, 0);
> +
> +	if (obj == NULL) {
> +		dev_err(&wdev->dev,
> +			"Could not query hardware information");

Ditto.

> +		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);

32 appears at least twice here, add define for it.

> +	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;

Don't put empty line between the call and its error handling. Move 
the declaration of the ret variable to function level.


> +		if (channel == 0) { // CPU fan
> +			u32 cpu_fanspeed = out.a4;
> +
> +			cpu_fanspeed <<= 8;
> +			cpu_fanspeed += out.a4 >> 8;

Is this byteswapping? Use proper endianness helpers/types when dealing 
with endianness.

> +			*val = (long) cpu_fanspeed;
> +		} else if (channel == 1) { // GPU fan
> +			u32 gpu_fanspeed = out.a5;
> +
> +			gpu_fanspeed <<= 8;
> +			gpu_fanspeed += out.a5 >> 8;
> +			*val = (long) gpu_fanspeed;
> +		}

Should the other channel values return -ENODEV or -EINVAL?

> +		return 0;
> +	case hwmon_pwm:
> +		casper_query(to_wmi_device(dev->parent), CASPER_POWERPLAN,
> +			     &out);
> +		if (channel == 0)
> +			*val = (long)out.a2;
> +		else
> +			return -EOPNOTSUPP;
> +		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;
> +		ret =
> +		    casper_set(to_wmi_device(dev->parent), CASPER_POWERPLAN,
> +			       val, 0);
> +
> +		if (ACPI_FAILURE(ret)) {
> +			dev_err(dev, "Couldn't set power plan, acpi_status: %d",
> +				ret);
> +			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;
> +
> +	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);
> +	}

Misindented brace.

> +
> +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

Put comma to the end of this line.

> +};
> +
> +module_wmi_driver(casper_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, casper_wmi_id_table);
>
  

Am 21.02.24 um 23:55 schrieb Guenter Roeck:

> On 2/21/24 14:15, Mustafa Ekşi wrote:
>> Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@gmail.com>
>> ---
>>   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@gmail.com>
>> +L:    platform-driver-x86@vger.kernel.org
>> +S:    Maintained
>> +F:    drivers/platform/x86/casper-wmi.c
>> +
>>   CAVIUM I2C DRIVER
>>   M:    Robert Richter <rric@kernel.org>
>>   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@gmail.com>");
>> +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 ?
>
Hi,

this is indeed unnecessary, the WMI driver core already takes care that the WMI driver is
only bound to WMI devices with a matching GUID.

I think this was copied from the dell-wmi-privacy driver, i will send a patch to remove this.

Armin Wolf

>> +    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);
>
>
  

On 2/22/24 01:48, Ilpo Järvinen wrote:
> Hi,
> 
> Added LED subsys people, please include them in future versions
> automatically.
> 

A MAINTAINERS entry such as

K:      (devm_)?led_(classdev|trigger)_(un)?register(|_ext|_simple)

might be useful if subsystem maintainers want to be informed
if the subsystem is used outside its normal locations.

Guenter