Message-ID: <bf9318bf-72b9-4e71-b37a-767e3e51432e@gmx.de>
Date: Sun, 10 Mar 2024 20:17:13 +0100
From: Armin Wolf <W_Armin@....de>
To: mustafa <mustafa.eskieksi@...il.com>, hdegoede@...hat.com,
 ilpo.jarvinen@...ux.intel.com, jdelvare@...e.com, linux@...ck-us.net,
 linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 linux-hwmon@...r.kernel.org, pavel@....cz, lee@...nel.org,
 linux-leds@...r.kernel.org
Cc: rishitbansal0@...il.com
Subject: Re: [PATCH v3 1/1] platform/x86: Add wmi driver for Casper Excalibur
 laptops

Am 10.03.24 um 19:14 schrieb mustafa:

> From: Mustafa Ekşi <mustafa.eskieksi@...il.com>
>
> This wmi driver supports Casper Excalibur laptops' changing keyboard
> backlight, reading fan speeds and changing power profiles. Multicolor
> led device is used for backlight, platform_profile for power management
> and hwmon for fan speeds. It supports both old (10th gen or older) and
> new (11th gen or newer) laptops. It uses x86_match_cpu to check if the
> laptop is old or new.
> This driver's Multicolor keyboard backlight API is very similar to Rishit
> Bansal's proposed API.
>
> Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@...il.com>
> ---
> Changes in v3:
>   - Replaced led_control attribute with multicolor led interface.
>   - Added struct led_cache, instead of storing only last color change.
>   - Added dmi list to prevent registering platform_profile driver in models
>     that doesn't have this feature.
>   - Added a x86_cpu_id to differentiate older laptops that are reporting
>     fan speeds in big-endian. Also newer laptops have a different power
>     profile scheme. I'm using x86_cpu_id because they don't have a
>     difference in model names, only in cpu generations (the official driver
>     download page makes you select your cpu's generation too).
>   - Removed hwmon_pwm device in favor of platform_profile driver. It
>     indirectly affects fans' speed but they also affect frequency and
>     power consumption as well.
>   - Replaced handwritten masks with GENMASK equivalents.
>   - Replaced led_classdev_register with
>     devm_led_classdev_multicolor_register. This should solve the bug
>     where led_classdev remains registered even if casper_wmi_probe
>     returns -ENODEV.
>   - Removed select NEW_LEDS and LEDS_CLASS, because it creates recursive
>     dependencies.
>   - And some minor changes.
> Changes in v2:
>   - Added masks for
>   - Changed casper_set and casper_query returns Linux error code rather than
>     acpi_status.
>   - replaced complicated bit operations with FIELD_GET.
>   - Fixed some indentation and spacing.
>   - Broke fan speeds further.
>   - Moved module metadata to the end of the file.
> ---
>   MAINTAINERS                       |   6 +
>   drivers/platform/x86/Kconfig      |  14 +
>   drivers/platform/x86/Makefile     |   1 +
>   drivers/platform/x86/casper-wmi.c | 639 ++++++++++++++++++++++++++++++
>   4 files changed, 660 insertions(+)
>   create mode 100644 drivers/platform/x86/casper-wmi.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1aabf1c15bb..e4cb770c990 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4724,6 +4724,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@...il.com>
> +L:	platform-driver-x86@...r.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/casper-wmi.c
> +
>   CAVIUM I2C DRIVER
>   M:	Robert Richter <rric@...nel.org>
>   S:	Odd Fixes
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index bdd302274b9..4f951bcac1a 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
> +	depends on LEDS_CLASS_MULTICOLOR
> +	select ACPI_PLATFORM_PROFILE
> +	help
> +	  Say Y here if you want to support WMI-based fan speed reporting,
> +	  power management and keyboard backlight support on Casper Excalibur
> +	  Laptops.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called casper-wmi.
> +
>   endif # X86_PLATFORM_DEVICES
>
>   config P2SB
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 1de432e8861..4b527dd44ad 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_MXM_WMI)			+= mxm-wmi.o
>   obj-$(CONFIG_NVIDIA_WMI_EC_BACKLIGHT)	+= nvidia-wmi-ec-backlight.o
>   obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
>   obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
> +obj-$(CONFIG_CASPER_WMI)		+= casper-wmi.o
>
>   # Acer
>   obj-$(CONFIG_ACERHDF)		+= acerhdf.o
> diff --git a/drivers/platform/x86/casper-wmi.c b/drivers/platform/x86/casper-wmi.c
> new file mode 100644
> index 00000000000..80e1e2b16fb
> --- /dev/null
> +++ b/drivers/platform/x86/casper-wmi.c
> @@ -0,0 +1,639 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/module.h>
> +#include <linux/bits.h>
> +#include <linux/bitops.h>
> +#include <linux/acpi.h>
> +#include <linux/leds.h>
> +#include <linux/slab.h>
> +#include <linux/wmi.h>
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <acpi/acexcep.h>
> +#include <linux/bitfield.h>
> +#include <linux/sysfs.h>
> +#include <linux/platform_profile.h>
> +#include <linux/led-class-multicolor.h>
> +
> +#include <linux/dmi.h>
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +#define CASPER_WMI_GUID "644C5791-B7B0-4123-A90B-E93876E0DAAD"
> +
> +#define CASPER_READ 0xfa00
> +#define CASPER_WRITE 0xfb00
> +#define CASPER_GET_HARDWAREINFO 0x0200
> +#define CASPER_SET_LED 0x0100
> +#define CASPER_POWERPLAN 0x0300
> +
> +#define CASPER_KEYBOARD_LED_1 0x03
> +#define CASPER_KEYBOARD_LED_2 0x04
> +#define CASPER_KEYBOARD_LED_3 0x05
> +#define CASPER_ALL_KEYBOARD_LEDS 0x06
> +#define CASPER_CORNER_LEDS 0x07
> +#define CASPER_LED_COUNT 4
> +
> +const char * const zone_names[CASPER_LED_COUNT] = {
> +	"casper::kbd_zoned_backlight-right",
> +	"casper::kbd_zoned_backlight-middle",
> +	"casper::kbd_zoned_backlight-left",
> +	"casper::kbd_zoned_backlight-corners",
> +};
> +
> +#define CASPER_LED_ALPHA GENMASK(31, 24)
> +#define CASPER_LED_RED	 GENMASK(23, 16)
> +#define CASPER_LED_GREEN GENMASK(15, 8)
> +#define CASPER_LED_BLUE  GENMASK(7, 0)
> +#define CASPER_DEFAULT_COLOR (CASPER_LED_RED | CASPER_LED_GREEN | \
> +			      CASPER_LED_BLUE)
> +#define CASPER_FAN_CPU 0
> +#define CASPER_FAN_GPU 1
> +
> +enum casper_power_profile_old {
> +	CASPER_HIGH_PERFORMANCE = 1,
> +	CASPER_GAMING		= 2,
> +	CASPER_TEXT_MODE	= 3,
> +	CASPER_POWERSAVE	= 4
> +};
> +
> +enum casper_power_profile_new {
> +	CASPER_NEW_HIGH_PEROFRMANCE	= 0,
> +	CASPER_NEW_GAMING		= 1,
> +	CASPER_NEW_AUDIO		= 2
> +};
> +
> +struct casper_quirk_entry {
> +	bool big_endian_fans;
> +	bool no_power_profiles;
> +	bool new_power_scheme;
> +};
> +
> +struct casper_wmi_driver {
> +	struct wmi_device *wdev;
> +	struct platform_profile_handler handler;
> +};
> +
> +struct casper_wmi_args {
> +	u16 a0, a1;
> +	u32 a2, a3, a4, a5, a6, a7, a8;
> +};
> +
> +enum casper_led_mode {
> +	LED_NORMAL = 0x10,
> +	LED_BLINK = 0x20,
> +	LED_FADE = 0x30,
> +	LED_HEARTBEAT = 0x40,
> +	LED_REPEAT = 0x50,
> +	LED_RANDOM = 0x60
> +};
> +
> +struct casper_led_cache {
> +	u32 colors[CASPER_LED_COUNT];
> +	u8 last_keyboard_led;
> +};
> +
> +static struct casper_led_cache led_cache = {
> +	.colors = {
> +		CASPER_DEFAULT_COLOR, CASPER_DEFAULT_COLOR,
> +		CASPER_DEFAULT_COLOR, CASPER_DEFAULT_COLOR,
> +	},
> +	.last_keyboard_led = CASPER_ALL_KEYBOARD_LEDS,
> +};
> +
> +static struct casper_quirk_entry *quirk_applied;
> +static struct led_classdev_mc *casper_kbd_mcled_info;

Hi,

those global variables are initialized inside the probe() callback of the WMI driver,
so there are going to be issues when this driver is going to be instantiated multiple times.

Please move those global variables into a private driver struct using the state container pattern:
https://www.kernel.org/doc/html/latest/driver-api/driver-model/design-patterns.html

Maybe you can keep the variables associated with quirk handling global and instead do the DMI matching
inside the modules init function before registering the WMI driver (this would replace module_wmi_driver()).

> +
> +static struct mc_subled casper_kbd_mcled_sub[CASPER_LED_COUNT][1] = {
> +	{
> +		{
> +			.color_index = LED_COLOR_ID_RGB,
> +			.brightness = 2,
> +			.intensity = CASPER_DEFAULT_COLOR
> +		}
> +	},
> +	{
> +		{
> +			.color_index = LED_COLOR_ID_RGB,
> +			.brightness = 2,
> +			.intensity = CASPER_DEFAULT_COLOR
> +		}
> +	},
> +	{
> +		{
> +			.color_index = LED_COLOR_ID_RGB,
> +			.brightness = 2,
> +			.intensity = CASPER_DEFAULT_COLOR
> +		}
> +	},
> +	{
> +		{
> +			.color_index = LED_COLOR_ID_RGB,
> +			.brightness = 2,
> +			.intensity = CASPER_DEFAULT_COLOR
> +		}
> +	},
> +};
> +
> +static int casper_set(struct wmi_device *wdev, u16 a1, u8 led_id, u32 data)
> +{
> +	acpi_status ret;
> +	struct casper_wmi_args wmi_args;
> +	struct acpi_buffer input;
> +
> +	wmi_args = (struct casper_wmi_args) {
> +		.a0 = CASPER_WRITE,
> +		.a1 = a1,
> +		.a2 = led_id,
> +		.a3 = data
> +	};
> +
> +	input = (struct acpi_buffer) {
> +		(acpi_size) sizeof(struct casper_wmi_args),
> +		&wmi_args
> +	};
> +
> +	ret = wmidev_block_set(wdev, 0, &input);
> +	if (ACPI_FAILURE(ret))
> +		return -EINVAL;

I think -EIO would be more suitable.

> +
> +	return 0;
> +}
> +
> +static int casper_query(struct wmi_device *wdev, u16 a1,
> +			struct casper_wmi_args *out)
> +{
> +	union acpi_object *obj;
> +	struct casper_wmi_args wmi_args;
> +	struct acpi_buffer input;
> +	int ret;
> +
> +	wmi_args = (struct casper_wmi_args) {
> +		.a0 = CASPER_READ,
> +		.a1 = a1
> +	};
> +	input = (struct acpi_buffer) {
> +		(acpi_size) sizeof(struct casper_wmi_args),
> +		&wmi_args
> +	};
> +
> +	ret = wmidev_block_set(wdev, 0, &input);
> +	if (ACPI_FAILURE(ret))
> +		return -EIO;
> +
> +	obj = wmidev_block_query(wdev, 0);
> +	if (IS_ERR_OR_NULL(obj))
> +		return -EIO;

You only need to check for NULL. Also you maybe should protect accesses like these with a
mutex, as otherwise two queries running simultaneously could overwrite themselves.

> +
> +	if (obj->type != ACPI_TYPE_BUFFER) { // obj will be 0x10 on failure
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	if (obj->buffer.length != sizeof(struct casper_wmi_args)) {
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	memcpy(out, obj->buffer.pointer, sizeof(struct casper_wmi_args));
> +
> +out:
> +	kfree(obj);
> +	return ret;
> +}
> +
> +static enum led_brightness get_casper_brightness(struct led_classdev *led_cdev)
> +{
> +	struct wmi_device *wdev = to_wmi_device(led_cdev->dev->parent);
> +	struct casper_wmi_args hardware_alpha = {0};
> +
> +	if (strcmp(led_cdev->name, zone_names[3]) == 0)
> +		return FIELD_GET(CASPER_LED_ALPHA, led_cache.colors[3]);
> +
> +	casper_query(wdev, CASPER_GET_HARDWAREINFO, &hardware_alpha);
> +
> +	return hardware_alpha.a6;
> +}
> +
> +static void set_casper_brightness(struct led_classdev *led_cdev,
> +				  enum led_brightness brightness)
> +{
> +	u32 bright_with_mode, bright_prep, led_data, led_data_no_alpha;
> +	struct wmi_device *wdev = to_wmi_device(led_cdev->dev->parent);
> +	int ret;
> +	size_t zone;
> +	u8 zone_to_change;
> +
> +	for (size_t i = 0; i < CASPER_LED_COUNT; i++)
> +		if (strcmp(led_cdev->name, zone_names[i]) == 0)
> +			zone = i;
> +
> +	if (zone == 3)
> +		zone_to_change = CASPER_CORNER_LEDS;
> +	else
> +		zone_to_change = zone + CASPER_KEYBOARD_LED_1;
> +
> +	led_data_no_alpha = casper_kbd_mcled_info[zone].subled_info[0].intensity
> +			    & ~CASPER_LED_ALPHA;
> +	if ((led_cache.colors[zone] & ~CASPER_LED_ALPHA) == led_data_no_alpha)
> +		bright_with_mode = brightness | LED_NORMAL;
> +	else
> +		bright_with_mode = get_casper_brightness(led_cdev) | LED_NORMAL;
> +
> +	bright_prep = FIELD_PREP(CASPER_LED_ALPHA, bright_with_mode);
> +	led_data = bright_prep | led_data_no_alpha;
> +	ret = casper_set(wdev, CASPER_SET_LED, zone_to_change, led_data);
> +	if (ret)
> +		return;
> +
> +	led_cache.colors[zone] = led_data;
> +}
> +
> +static int casper_platform_profile_get(struct platform_profile_handler *pprof,
> +				       enum platform_profile_option *profile)
> +{
> +	struct casper_wmi_driver *drv = container_of(pprof,
> +						     struct casper_wmi_driver,
> +						     handler);
> +	struct casper_wmi_args ret_buff = {0};
> +	int ret;
> +
> +	ret = casper_query(drv->wdev, CASPER_POWERPLAN, &ret_buff);
> +	if (ret)
> +		return ret;
> +
> +	if (quirk_applied->new_power_scheme) {
> +		switch (ret_buff.a2) {
> +		case CASPER_NEW_HIGH_PEROFRMANCE:
> +			*profile = PLATFORM_PROFILE_PERFORMANCE;
> +			break;
> +		case CASPER_NEW_GAMING:
> +			*profile = PLATFORM_PROFILE_BALANCED;
> +			break;
> +		case CASPER_NEW_AUDIO:
> +			*profile = PLATFORM_PROFILE_LOW_POWER;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		return 0;
> +	}
> +
> +	switch (ret_buff.a2) {
> +	case CASPER_HIGH_PERFORMANCE:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	case CASPER_GAMING:
> +		*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
> +		break;
> +	case CASPER_TEXT_MODE:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case CASPER_POWERSAVE:
> +		*profile = PLATFORM_PROFILE_LOW_POWER;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int casper_platform_profile_set(struct platform_profile_handler *pprof,
> +				       enum platform_profile_option profile)
> +{
> +	struct casper_wmi_driver *drv = container_of(pprof,
> +						     struct casper_wmi_driver,
> +						     handler);
> +	enum casper_power_profile_old prf_old;
> +	enum casper_power_profile_new prf_new;
> +
> +	if (quirk_applied->new_power_scheme) {
> +
> +		switch (profile) {
> +		case PLATFORM_PROFILE_PERFORMANCE:
> +			prf_new = CASPER_NEW_HIGH_PEROFRMANCE;
> +			break;
> +		case PLATFORM_PROFILE_BALANCED:
> +			prf_new = CASPER_NEW_GAMING;
> +			break;
> +		case PLATFORM_PROFILE_LOW_POWER:
> +			prf_new = CASPER_NEW_AUDIO;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		return casper_set(drv->wdev, CASPER_POWERPLAN, prf_new, 0);
> +	}
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		prf_old = CASPER_HIGH_PERFORMANCE;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> +		prf_old = CASPER_GAMING;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		prf_old = CASPER_TEXT_MODE;
> +		break;
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		prf_old = CASPER_POWERSAVE;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return casper_set(drv->wdev, CASPER_POWERPLAN, prf_old, 0);
> +}
> +
> +static umode_t casper_wmi_hwmon_is_visible(const void *drvdata,
> +					   enum hwmon_sensor_types type,
> +					   u32 attr, int channel)
> +{
> +	if (type == hwmon_fan)
> +		return 0444;
> +
> +	return 0;

Since you only support fan sensors, you can just return 0444;

> +}
> +
> +static int casper_wmi_hwmon_read(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int channel, long *val)
> +{
> +	struct casper_wmi_args out = { 0 };
> +	struct wmi_device *wdev = to_wmi_device(dev->parent);
> +	int ret;
> +
> +	if (type != hwmon_fan)
> +		return -EOPNOTSUPP;

See above.

> +
> +	ret = casper_query(wdev, CASPER_GET_HARDWAREINFO, &out);
> +	if (ret)
> +		return ret;
> +
> +	switch (channel) {
> +	case CASPER_FAN_CPU:
> +		if (quirk_applied->big_endian_fans)
> +			*val = be16_to_cpu((u16) out.a4);
> +		else
> +			*val = out.a5;
> +		break;
> +	case CASPER_FAN_GPU:
> +		if (quirk_applied->big_endian_fans)
> +			*val = be16_to_cpu((u16) out.a5);
> +		else
> +			*val = out.a5;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int casper_wmi_hwmon_read_string(struct device *dev,
> +					enum hwmon_sensor_types type, u32 attr,
> +					int channel, const char **str)
> +{
> +	if (type != hwmon_fan)
> +		return -EOPNOTSUPP;

See above.

> +
> +	switch (channel) {
> +	case CASPER_FAN_CPU:
> +		*str = "cpu_fan_speed";
> +		break;
> +	case CASPER_FAN_GPU:
> +		*str = "gpu_fan_speed";
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_ops casper_wmi_hwmon_ops = {
> +	.is_visible = &casper_wmi_hwmon_is_visible,
> +	.read = &casper_wmi_hwmon_read,
> +	.read_string = &casper_wmi_hwmon_read_string,
> +};
> +
> +static const struct hwmon_channel_info *const casper_wmi_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_LABEL,
> +			   HWMON_F_INPUT | HWMON_F_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info casper_wmi_hwmon_chip_info = {
> +	.ops = &casper_wmi_hwmon_ops,
> +	.info = casper_wmi_hwmon_info,
> +};
> +
> +static struct casper_quirk_entry gen_older_than_11 = {
> +	.big_endian_fans = true,
> +	.new_power_scheme = false
> +};
> +
> +static struct casper_quirk_entry gen_newer_than_11 = {
> +	.big_endian_fans = false,
> +	.new_power_scheme = true
> +};
> +
> +static const struct x86_cpu_id casper_gen[] = {
> +	X86_MATCH_INTEL_FAM6_MODEL(KABYLAKE, &gen_older_than_11),
> +	X86_MATCH_INTEL_FAM6_MODEL(COMETLAKE, &gen_older_than_11),
> +	X86_MATCH_INTEL_FAM6_MODEL(TIGERLAKE, &gen_newer_than_11),
> +	X86_MATCH_INTEL_FAM6_MODEL(ALDERLAKE, &gen_newer_than_11),
> +	X86_MATCH_INTEL_FAM6_MODEL(RAPTORLAKE, &gen_newer_than_11),
> +	X86_MATCH_INTEL_FAM6_MODEL(METEORLAKE, &gen_newer_than_11),
> +	{}
> +};
> +
> +/*
> + * These quirks don't get stored in quirk_applied.
> + */
> +static struct casper_quirk_entry quirk_no_power_profile = {
> +	.no_power_profiles = true
> +};
> +
> +static struct casper_quirk_entry quirk_has_power_profile = {
> +	.no_power_profiles = false
> +};
> +
> +static const struct dmi_system_id casper_quirks[] = {
> +	{
> +		.ident = "CASPER EXCALIBUR G650",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR,
> +				  "CASPER BILGISAYAR SISTEMLERI"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G650")
> +		},
> +		.driver_data = &quirk_no_power_profile
> +	},
> +	{
> +		.ident = "CASPER EXCALIBUR G670",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR,
> +				  "CASPER BILGISAYAR SISTEMLERI"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G670")
> +		},
> +		.driver_data = &quirk_no_power_profile
> +	},
> +	{
> +		.ident = "CASPER EXCALIBUR G750",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR,
> +				  "CASPER BILGISAYAR SISTEMLERI"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G750")
> +		},
> +		.driver_data = &quirk_no_power_profile
> +	},
> +	{
> +		.ident = "CASPER EXCALIBUR G770",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR,
> +				  "CASPER BILGISAYAR SISTEMLERI"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G770")
> +		},
> +		.driver_data = &quirk_has_power_profile
> +	},
> +	{
> +		.ident = "CASPER EXCALIBUR G780",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR,
> +				  "CASPER BILGISAYAR SISTEMLERI"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G780")
> +		},
> +		.driver_data = &quirk_has_power_profile
> +	},
> +	{
> +		.ident = "CASPER EXCALIBUR G870",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR,
> +				  "CASPER BILGISAYAR SISTEMLERI"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G870")
> +		},
> +		.driver_data = &quirk_has_power_profile
> +	},
> +	{
> +		.ident = "CASPER EXCALIBUR G900",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR,
> +				  "CASPER BILGISAYAR SISTEMLERI"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G900")
> +		},
> +		.driver_data = &quirk_has_power_profile
> +	},
> +	{
> +		.ident = "CASPER EXCALIBUR G911",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR,
> +				  "CASPER BILGISAYAR SISTEMLERI"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "EXCALIBUR G911")
> +		},
> +		.driver_data = &quirk_has_power_profile
> +	},
> +	{ }
> +};
> +
> +static int casper_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct device *hwmon_dev;
> +	struct casper_wmi_driver *drv;
> +	const struct x86_cpu_id *gen_id;
> +	const struct dmi_system_id *dmi_id;
> +	int ret;
> +
> +	gen_id = x86_match_cpu(casper_gen);
> +	if (!gen_id)
> +		return -ENODEV;
> +
> +	quirk_applied = (struct casper_quirk_entry *) gen_id->driver_data;
> +
> +	dmi_id = dmi_first_match(casper_quirks);
> +	if (!dmi_id)
> +		return -ENODEV;
> +
> +	quirk_applied->no_power_profiles = ((struct casper_quirk_entry *)
> +		dmi_id->driver_data)->no_power_profiles;
> +
> +	casper_kbd_mcled_info = devm_kzalloc(&wdev->dev,
> +		sizeof(*casper_kbd_mcled_info)*CASPER_LED_COUNT, GFP_KERNEL);
> +	if (casper_kbd_mcled_info == NULL)
> +		return -ENOMEM;
> +
> +	for (size_t i = 0; i < CASPER_LED_COUNT; i++) {
> +		casper_kbd_mcled_info[i] = (struct led_classdev_mc) {
> +			.led_cdev = {
> +				.name = zone_names[i],
> +				.brightness = 0,
> +				.max_brightness = 2,
> +				.brightness_set = &set_casper_brightness,
> +				.brightness_get = &get_casper_brightness,
> +				.color = LED_COLOR_ID_RGB,
> +			},
> +			.num_colors = ARRAY_SIZE(casper_kbd_mcled_sub[i]),
> +			.subled_info = casper_kbd_mcled_sub[i]
> +		};
> +
> +		ret = devm_led_classdev_multicolor_register(&wdev->dev,
> +						&casper_kbd_mcled_info[i]);
> +		if (ret)
> +			return -ENODEV;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
> +						"casper_wmi", wdev,
> +						&casper_wmi_hwmon_chip_info,
> +						NULL);
> +	if (IS_ERR_OR_NULL(hwmon_dev))
> +		return -ENODEV;

Please return the error code contained inside the pointer. Also you only need
to check for IS_ERR().

> +
> +	if (!quirk_applied->no_power_profiles) {
> +		drv = devm_kzalloc(&wdev->dev, sizeof(*drv), GFP_KERNEL);
> +		if (drv == NULL)

if (!drv)

> +			return -ENOMEM;
> +		drv->wdev = wdev;
> +		dev_set_drvdata(&wdev->dev, drv);
> +
> +		drv->handler.profile_get = casper_platform_profile_get;
> +		drv->handler.profile_set = casper_platform_profile_set;
> +
> +		set_bit(PLATFORM_PROFILE_LOW_POWER, drv->handler.choices);
> +		set_bit(PLATFORM_PROFILE_BALANCED, drv->handler.choices);
> +		if (!quirk_applied->new_power_scheme)
> +			set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
> +				drv->handler.choices);
> +		set_bit(PLATFORM_PROFILE_PERFORMANCE, drv->handler.choices);
> +
> +		ret = platform_profile_register(&drv->handler);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void casper_wmi_remove(struct wmi_device *wdev)
> +{
> +	if (!quirk_applied->no_power_profiles)
> +		platform_profile_remove();
> +}
> +
> +static const struct wmi_device_id casper_wmi_id_table[] = {
> +	{ CASPER_WMI_GUID, NULL },
> +	{ }
> +};
> +
> +static struct wmi_driver casper_wmi_driver = {
> +	.driver = {
> +		   .name = "casper-wmi",
> +		    },
> +	.id_table = casper_wmi_id_table,
> +	.probe = casper_wmi_probe,
> +	.remove = &casper_wmi_remove,

Please set the .no_singleton flag to true to indicate that your WMI driver can be
instantiated multiple times. Otherwise i cannot accept this driver.

Thanks,
Armin Wolf

> +};
> +
> +module_wmi_driver(casper_wmi_driver);
> +MODULE_DEVICE_TABLE(wmi, casper_wmi_id_table);
> +
> +MODULE_AUTHOR("Mustafa Ekşi <mustafa.eskieksi@...il.com>");
> +MODULE_DESCRIPTION("Casper Excalibur Laptop WMI driver");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists