Received-SPF: pass (google.com: domain of linux-kernel+bounces-75621-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <042951f1-200f-4d89-b968-ea68c52ea607@roeck-us.net>
Date: Wed, 21 Feb 2024 14:55:06 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Add wmi driver support for Casper Excalibur laptops.
Content-Language: en-US
To: =?UTF-8?Q?Mustafa_Ek=C5=9Fi?= <mustafa.eskieksi@gmail.com>,
 hdegoede@redhat.com, ilpo.jarvinen@linux.intel.com,
 linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
 linux-hwmon@vger.kernel.org
Cc: jdelvare@suse.com
References: <20240221221549.640515-1-mustafa.eskieksi@gmail.com>
From: Guenter Roeck <linux@roeck-us.net>
Autocrypt: addr=linux@roeck-us.net; keydata=
 xsFNBE6H1WcBEACu6jIcw5kZ5dGeJ7E7B2uweQR/4FGxH10/H1O1+ApmcQ9i87XdZQiB9cpN
 RYHA7RCEK2dh6dDccykQk3bC90xXMPg+O3R+C/SkwcnUak1UZaeK/SwQbq/t0tkMzYDRxfJ7
 nyFiKxUehbNF3r9qlJgPqONwX5vJy4/GvDHdddSCxV41P/ejsZ8PykxyJs98UWhF54tGRWFl
 7i1xvaDB9lN5WTLRKSO7wICuLiSz5WZHXMkyF4d+/O5ll7yz/o/JxK5vO/sduYDIlFTvBZDh
 gzaEtNf5tQjsjG4io8E0Yq0ViobLkS2RTNZT8ICq/Jmvl0SpbHRvYwa2DhNsK0YjHFQBB0FX
 IdhdUEzNefcNcYvqigJpdICoP2e4yJSyflHFO4dr0OrdnGLe1Zi/8Xo/2+M1dSSEt196rXaC
 kwu2KgIgmkRBb3cp2vIBBIIowU8W3qC1+w+RdMUrZxKGWJ3juwcgveJlzMpMZNyM1jobSXZ0
 VHGMNJ3MwXlrEFPXaYJgibcg6brM6wGfX/LBvc/haWw4yO24lT5eitm4UBdIy9pKkKmHHh7s
 jfZJkB5fWKVdoCv/omy6UyH6ykLOPFugl+hVL2Prf8xrXuZe1CMS7ID9Lc8FaL1ROIN/W8Vk
 BIsJMaWOhks//7d92Uf3EArDlDShwR2+D+AMon8NULuLBHiEUQARAQABzTJHdWVudGVyIFJv
 ZWNrIChMaW51eCBhY2NvdW50KSA8bGludXhAcm9lY2stdXMubmV0PsLBgQQTAQIAKwIbAwYL
 CQgHAwIGFQgCCQoLBBYCAwECHgECF4ACGQEFAlVcphcFCRmg06EACgkQyx8mb86fmYFg0RAA
 nzXJzuPkLJaOmSIzPAqqnutACchT/meCOgMEpS5oLf6xn5ySZkl23OxuhpMZTVX+49c9pvBx
 hpvl5bCWFu5qC1jC2eWRYU+aZZE4sxMaAGeWenQJsiG9lP8wkfCJP3ockNu0ZXXAXwIbY1O1
 c+l11zQkZw89zNgWgKobKzrDMBFOYtAh0pAInZ9TSn7oA4Ctejouo5wUugmk8MrDtUVXmEA9
 7f9fgKYSwl/H7dfKKsS1bDOpyJlqhEAH94BHJdK/b1tzwJCFAXFhMlmlbYEk8kWjcxQgDWMu
 GAthQzSuAyhqyZwFcOlMCNbAcTSQawSo3B9yM9mHJne5RrAbVz4TWLnEaX8gA5xK3uCNCeyI
 sqYuzA4OzcMwnnTASvzsGZoYHTFP3DQwf2nzxD6yBGCfwNGIYfS0i8YN8XcBgEcDFMWpOQhT
 Pu3HeztMnF3HXrc0t7e5rDW9zCh3k2PA6D2NV4fews9KDFhLlTfCVzf0PS1dRVVWM+4jVl6l
 HRIAgWp+2/f8dx5vPc4Ycp4IsZN0l1h9uT7qm1KTwz+sSl1zOqKD/BpfGNZfLRRxrXthvvY8
 BltcuZ4+PGFTcRkMytUbMDFMF9Cjd2W9dXD35PEtvj8wnEyzIos8bbgtLrGTv/SYhmPpahJA
 l8hPhYvmAvpOmusUUyB30StsHIU2LLccUPPOwU0ETofVZwEQALlLbQeBDTDbwQYrj0gbx3bq
 7kpKABxN2MqeuqGr02DpS9883d/t7ontxasXoEz2GTioevvRmllJlPQERVxM8gQoNg22twF7
 pB/zsrIjxkE9heE4wYfN1AyzT+AxgYN6f8hVQ7Nrc9XgZZe+8IkuW/Nf64KzNJXnSH4u6nJM
 J2+Dt274YoFcXR1nG76Q259mKwzbCukKbd6piL+VsT/qBrLhZe9Ivbjq5WMdkQKnP7gYKCAi
 pNVJC4enWfivZsYupMd9qn7Uv/oCZDYoBTdMSBUblaLMwlcjnPpOYK5rfHvC4opxl+P/Vzyz
 6WC2TLkPtKvYvXmdsI6rnEI4Uucg0Au/Ulg7aqqKhzGPIbVaL+U0Wk82nz6hz+WP2ggTrY1w
 ZlPlRt8WM9w6WfLf2j+PuGklj37m+KvaOEfLsF1v464dSpy1tQVHhhp8LFTxh/6RWkRIR2uF
 I4v3Xu/k5D0LhaZHpQ4C+xKsQxpTGuYh2tnRaRL14YMW1dlI3HfeB2gj7Yc8XdHh9vkpPyuT
 nY/ZsFbnvBtiw7GchKKri2gDhRb2QNNDyBnQn5mRFw7CyuFclAksOdV/sdpQnYlYcRQWOUGY
 HhQ5eqTRZjm9z+qQe/T0HQpmiPTqQcIaG/edgKVTUjITfA7AJMKLQHgp04Vylb+G6jocnQQX
 JqvvP09whbqrABEBAAHCwWUEGAECAA8CGwwFAlVcpi8FCRmg08MACgkQyx8mb86fmYHNRQ/+
 J0OZsBYP4leJvQF8lx9zif+v4ZY/6C9tTcUv/KNAE5leyrD4IKbnV4PnbrVhjq861it/zRQW
 cFpWQszZyWRwNPWUUz7ejmm9lAwPbr8xWT4qMSA43VKQ7ZCeTQJ4TC8kjqtcbw41SjkjrcTG
 wF52zFO4bOWyovVAPncvV9eGA/vtnd3xEZXQiSt91kBSqK28yjxAqK/c3G6i7IX2rg6pzgqh
 hiH3/1qM2M/LSuqAv0Rwrt/k+pZXE+B4Ud42hwmMr0TfhNxG+X7YKvjKC+SjPjqp0CaztQ0H
 nsDLSLElVROxCd9m8CAUuHplgmR3seYCOrT4jriMFBtKNPtj2EE4DNV4s7k0Zy+6iRQ8G8ng
 QjsSqYJx8iAR8JRB7Gm2rQOMv8lSRdjva++GT0VLXtHULdlzg8VjDnFZ3lfz5PWEOeIMk7Rj
 trjv82EZtrhLuLjHRCaG50OOm0hwPSk1J64R8O3HjSLdertmw7eyAYOo4RuWJguYMg5DRnBk
 WkRwrSuCn7UG+qVWZeKEsFKFOkynOs3pVbcbq1pxbhk3TRWCGRU5JolI4ohy/7JV1TVbjiDI
 HP/aVnm6NC8of26P40Pg8EdAhajZnHHjA7FrJXsy3cyIGqvg9os4rNkUWmrCfLLsZDHD8FnU
 mDW4+i+XlNFUPUYMrIKi9joBhu18ssf5i5Q=
In-Reply-To: <20240221221549.640515-1-mustafa.eskieksi@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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