Message-ID: <42e2d2d7-e5d1-4f13-88f4-888e8bfb561c@gmx.de>
Date: Sun, 25 Jan 2026 01:29:03 +0100
From: Armin Wolf <W_Armin@....de>
To: Mingyou Chen <qby140326@...il.com>, hansg@...nel.org,
 ilpo.jarvinen@...ux.intel.com
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
 cryolitia.pukngae@...ux.dev
Subject: Re: [PATCH v2] platform/x86: tongfang-mifs-wmi: Add new Tongfang MIFS
 WMI driver

Am 24.01.26 um 15:39 schrieb Mingyou Chen:

> platform/x86: tongfang-mifs-wmi: Add new Tongfang MIFS WMI driver
>
> Add a WMI-based driver for Tongfang laptops that use the MIFS
> (MiInterface) ACPI WMI interface. These laptops are commonly sold under
> brands like Mechrevo, Schenker, XMG, and Eluktronics.
>
> The driver implements the following features:
> - Platform Profile support: Allows switching between Low Power,
> Balanced,
>    and Performance modes.
> - Hwmon support: Provides monitoring for CPU temperature and fan speeds
>    (CPU, GPU, and System fans).
> - LED support: Standard LED class interface for controlling keyboard
>    backlight brightness.
> - Sysfs interface:
>      - GPU mode switching (Hybrid, Discrete, UMA).
>      - Keyboard RGB mode and color control.
>      - Fan boost (Max fan speed) toggle.
>
> The driver communicates with the BIOS via WMI GUID
> "B60BFB48-3E5B-49E4-A0E9-8CFFE1B3434B" using method IDs 250 (GET)
> and 251 (SET).

Hi,

could you write a short description of the WMI classes used by this driver
(with the MOF description) and place it under "Documentation/wmi/devices/"?

This would help future developers in understanding and maintaining this driver.

> Signed-off-by: Mingyou Chen <qby140326@...il.com>
> ---
>   drivers/platform/x86/Kconfig             |  11 +
>   drivers/platform/x86/Makefile            |   1 +
>   drivers/platform/x86/tongfang-mifs-wmi.c | 495 +++++++++++++++++++++++
>   3 files changed, 507 insertions(+)
>   create mode 100644 drivers/platform/x86/tongfang-mifs-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 4cb7d97a9fcc..ff12631561e0 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -113,6 +113,17 @@ config GIGABYTE_WMI
>   	  To compile this driver as a module, choose M here: the module will
>   	  be called gigabyte-wmi.
>   
> +config TONGFANG_MIFS_WMI
> +	tristate "Tongfang MIFS WMI temperature, fan and GPU controller driver"
> +	depends on ACPI_WMI
> +	depends on HWMON
> +	depends on ACPI_PLATFORM_PROFILE

Please select ACPI_PLATFORM_PROFILE instead of depending on it. Also add a dependency
on LEDS_CLASS_MULTICOLOR for the rgb keyboard controls.

> +	help
> +	  The driver for Tongfang laptops

Please describe here what kind of services this driver provides, and replace the final lines
with:

	To compile this driver as a module, choose M here: the module will
         be called tongfang-mifs-wmi.

> +
> +	  Say Y here if you want to support WMI-based features on Tongfang
> +	  laptops, such as performance mode switching.
> +
>   config ACERHDF
>   	tristate "Acer Aspire One temperature and fan driver"
>   	depends on ACPI_EC && THERMAL
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index d25762f7114f..1160c726bda6 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_NVIDIA_WMI_EC_BACKLIGHT)	+= nvidia-wmi-ec-backlight.o
>   obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
>   obj-$(CONFIG_REDMI_WMI)			+= redmi-wmi.o
>   obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
> +obj-$(CONFIG_TONGFANG_MIFS_WMI)		+= tongfang-mifs-wmi.o
>   
>   # Acer
>   obj-$(CONFIG_ACERHDF)		+= acerhdf.o
> diff --git a/drivers/platform/x86/tongfang-mifs-wmi.c b/drivers/platform/x86/tongfang-mifs-wmi.c
> new file mode 100644
> index 000000000000..6e1b7f4e9069
> --- /dev/null
> +++ b/drivers/platform/x86/tongfang-mifs-wmi.c
> @@ -0,0 +1,495 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/acpi.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_profile.h>
> +#include <linux/sysfs.h>
> +#include <linux/wmi.h>
> +
> +#define DRV_NAME "tongfang-mifs-wmi"
> +#define TONGFANG_MIFS_GUID "B60BFB48-3E5B-49E4-A0E9-8CFFE1B3434B"
> +#define WMI_BUFFER_SIZE 32
> +
> +enum wmi_method_type {
> +	WMI_METHOD_GET = 250,
> +	WMI_METHOD_SET = 251,
> +};

The method type might be easily confused with the WMI method id, i suggest you
rename this to "mifs_operation".

> +
> +enum wmi_method_name {
> +	WMI_FN_SYSTEM_PER_MODE = 8,
> +	WMI_FN_GPU_MODE = 9,
> +	WMI_FN_KBD_TYPE = 10,
> +	WMI_FN_FN_LOCK = 11,
> +	WMI_FN_TP_LOCK = 12,
> +	WMI_FN_FAN_SPEEDS = 13,
> +	WMI_FN_RGB_KB_MODE = 16,
> +	WMI_FN_RGB_KB_COLOR = 17,
> +	WMI_FN_RGB_KB_BRIGHTNESS = 18,
> +	WMI_FN_SYSTEM_AC_TYPE = 19,
> +	WMI_FN_MAX_FAN_SWITCH = 20,
> +	WMI_FN_MAX_FAN_SPEED = 21,
> +	WMI_FN_CPU_THERMOMETER = 22,
> +	WMI_FN_CPU_POWER = 23,
> +};

Same as above, i suggest "mifs_function".

> +
> +struct tongfang_mifs_wmi_data {
> +	struct wmi_device *wdev;
> +	struct mutex lock;

checkpatch complains about a struct mutex definition without a comment, please fix.

> +	struct led_classdev kbd_led;
> +	struct device *hwmon_dev;

hwmon_dev is only used during probing, please move this field into a local variable.

> +};
> +
> +static int tongfang_mifs_wmi_call(struct tongfang_mifs_wmi_data *data, u8 type,
> +				  u8 method, u8 *payload, size_t payload_len,
> +				  u8 *out_data)
> +{
> +	struct acpi_buffer input = { 0, NULL };
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	u8 *buffer;
> +	acpi_status status;
> +	int ret = 0;
> +
> +	if (!data)
> +		return -EINVAL;

Unnecessary check, please remove.

> +
> +	buffer = kzalloc(WMI_BUFFER_SIZE, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;

Maybe it would make sense to define the input buffer on the stack of the caller and
just pass a pointer to it. This way you save a memory allocation.

> +
> +	buffer[1] = type;
> +	buffer[3] = method;

Why not defining a struct for those input parameters:

struct mifs_input {
	u8 reserved1;
	u8 operation;
	u8 reserved2;
	u8 function;
	u8 payload[28];
} __packed;

This way users of this function can populate the buffer as a local variable and just pass a pointer to it.
Also, could it be that those fields are actually 16-bit little-endian fields? If yes then you can model them
like this:

struct mifs_input {
	__le16 operation;
	__le16 function;
	u8 payload[28];
} __packed;

> +
> +	if (payload && payload_len > 0) {
> +		size_t copy_len =
> +			min_t(size_t, payload_len, WMI_BUFFER_SIZE - 4);
> +		memcpy(&buffer[4], payload, copy_len);
> +	}
> +
> +	input.length = WMI_BUFFER_SIZE;
> +	input.pointer = buffer;
> +
> +	status = wmi_evaluate_method(TONGFANG_MIFS_GUID, 0, 1, &input, &output);

Please do not use the deprecated GUID-based WMI interface, use wmidev_invoke_method()
instead (see Documentation/wmi/driver-development-guide.rst for details). Said function
handles the ACPI stuff for you, so you can focus on parsing the output buffer content.

(Please note that this function is currently only available on the for-next branch inside
the pdx86 kernel tree).

> +	if (ACPI_FAILURE(status)) {
> +		ret = -EIO;
> +		goto out_free_in;
> +	}
> +
> +	obj = output.pointer;
> +	if (!obj) {
> +		ret = -ENODATA;
> +		goto out_free_in;
> +	}
> +
> +	if (obj->type == ACPI_TYPE_BUFFER &&
> +	    obj->buffer.length >= WMI_BUFFER_SIZE) {
> +		if (out_data)
> +			memcpy(out_data, obj->buffer.pointer, WMI_BUFFER_SIZE);

The MOF definition of this WMI class says that the output buffer contains a return code:

[WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x40A"), Description("WMI Get Set Method"), guid("{b60bfb48-3e5b-49e4-a0e9-8cffe1b3434b}")]
class MICommonInterface {
   [key, read] string InstanceName;
   [read] boolean Active;

   [WmiMethodId(1), Implemented, read, write, Description("WMI Get Set Method")] void MiInterface([in] uint8 InData[32], [out] uint8 OutData[30], [out] uint16 ReturnCode);
};

I suggest that you use a struct for thist that looks like that:

struct mifs_output {
	data[30];
	__le16 return_code;
}

You can then easily check the return code and copy the output data.

> +	} else {
> +		ret = -EINVAL;
> +	}
> +
> +	kfree(output.pointer);
> +out_free_in:
> +	kfree(buffer);
> +	return ret;
> +}
> +
> +static int laptop_profile_get(struct device *dev,
> +			      enum platform_profile_option *profile)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	u8 result[WMI_BUFFER_SIZE];
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;

Unnecessary check, please remove.

> +
> +	mutex_lock(&data->lock);

I think tongfang_mifs_wmi_call() should handle the locking itself, maybe you can
use the new guard() macro in linux/cleanup.h for this.

> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_GET,
> +				     WMI_FN_SYSTEM_PER_MODE, NULL, 0, result);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	switch (result[4]) {
> +	case 0:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case 1:
> +		*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
> +		break;
> +	case 2:
> +		*profile = PLATFORM_PROFILE_LOW_POWER;
> +		break;
> +	case 3:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break; /* Fullspeed */
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;

Maybe you want to use a enum for those WMI performance profiles here.

> +}
> +
> +static int laptop_profile_set(struct device *dev,
> +			      enum platform_profile_option profile)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	u8 val;
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;

Unnecessary check, please remove.

> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_LOW_POWER:
> +		val = 2;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		val = 0;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> +		val = 1;
> +		break;
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		val = 3; /* Fullspeed */
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	mutex_lock(&data->lock);
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_SET,
> +				     WMI_FN_SYSTEM_PER_MODE, &val, 1, NULL);
> +	mutex_unlock(&data->lock);

Does the firmware keep the platform profile setting during suspend and hibernation?
If no then please save this value during suspend and restore it during resume.

> +	return ret;
> +}
> +
> +static int platform_profile_probe(void *drvdata, unsigned long *choices)
> +{
> +	__set_bit(PLATFORM_PROFILE_LOW_POWER, choices);
> +	__set_bit(PLATFORM_PROFILE_BALANCED, choices);
> +	__set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, choices);
> +	__set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);

Please use set_bit() instead.

> +	return 0;
> +}
> +
> +static struct platform_profile_ops laptop_profile_ops = {
> +	.probe = platform_profile_probe,
> +	.profile_get = laptop_profile_get,
> +	.profile_set = laptop_profile_set,
> +};

Please make this struct const.

> +
> +static int laptop_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			     u32 attr, int channel, long *val)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	u8 res[WMI_BUFFER_SIZE];
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;

Unnecessary check, please remove.

> +
> +	mutex_lock(&data->lock);
> +	if (type == hwmon_temp) {
> +		ret = tongfang_mifs_wmi_call(data, WMI_METHOD_GET,
> +					     WMI_FN_CPU_THERMOMETER, NULL, 0,
> +					     res);
> +		if (!ret)
> +			*val = res[4] * 1000;
> +	} else if (type == hwmon_fan) {
> +		ret = tongfang_mifs_wmi_call(data, WMI_METHOD_GET,
> +					     WMI_FN_FAN_SPEEDS, NULL, 0, res);
> +		if (!ret) {
> +			if (channel == 0) /* CPU */
> +				*val = (res[5] << 8) | res[4];
> +			else if (channel == 1) /* GPU */
> +				*val = (res[7] << 8) | res[6];
> +			else if (channel == 2) /* SYS */
> +				*val = (res[11] << 8) | res[10];
> +			else
> +				ret = -EINVAL;

Maybe you want to use a switch-case statements for that? Additionally i suggest that you
report the labels of each fan channel ("CPU", "GPU" and "SYS") as HWMON_F_LABEL to userspace.

> +		}
> +	} else {
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static umode_t laptop_hwmon_is_visible(const void *drvdata,
> +				       enum hwmon_sensor_types type, u32 attr,
> +				       int channel)
> +{
> +	if (type == hwmon_temp && attr == hwmon_temp_input && channel == 0)
> +		return 0444;
> +	if (type == hwmon_fan && attr == hwmon_fan_input && channel < 3)
> +		return 0444;
> +	return 0;
> +}

You can replace this by setting .visible to 0444 inside struct hwmon_ops.

> +
> +static const struct hwmon_channel_info *laptop_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT, HWMON_F_INPUT),
> +	NULL
> +};
> +
> +static const struct hwmon_ops laptop_hwmon_ops = {
> +	.is_visible = laptop_hwmon_is_visible,
> +	.read = laptop_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info laptop_chip_info = {
> +	.ops = &laptop_hwmon_ops,
> +	.info = laptop_hwmon_info,
> +};
> +
> +static int laptop_kbd_led_set(struct led_classdev *led_cdev,
> +			      enum led_brightness value)
> +{
> +	struct tongfang_mifs_wmi_data *data =
> +		container_of(led_cdev, struct tongfang_mifs_wmi_data, kbd_led);
> +	u8 val = (u8)value;
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_SET,
> +				     WMI_FN_RGB_KB_BRIGHTNESS, &val, 1, NULL);
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +/* GPU Mode: 0:Hybrid, 1:Discrete, 2:UMA */

Please output strings instead of the raw WMI numbers to make it easier for
userspace to understand the resulting data.

> +static ssize_t gpu_mode_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	u8 result[WMI_BUFFER_SIZE];
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;

See the previous comments on this.

> +
> +	mutex_lock(&data->lock);
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_GET, WMI_FN_GPU_MODE,
> +				     NULL, 0, result);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret)
> +		return ret;
> +	return sysfs_emit(buf, "%d\n", result[4]);
> +}
> +
> +static ssize_t gpu_mode_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	u8 val;
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (kstrtou8(buf, 10, &val) || val > 2)
> +		return -EINVAL;

See comment above, you could use sysfs_match_string() here for this.

> +
> +	mutex_lock(&data->lock);
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_SET, WMI_FN_GPU_MODE,
> +				     &val, 1, NULL);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +/* RGB Mode: 0:OFF, 1:Cyclic, 2:Fixed, 3:Custom */

Same as with the GPU sysfs atrtibute, use strings instead of magic numbers.

> +static ssize_t kb_mode_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	u8 result[WMI_BUFFER_SIZE];
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_GET, WMI_FN_RGB_KB_MODE,
> +				     NULL, 0, result);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret)
> +		return ret;
> +	return sysfs_emit(buf, "%d\n", result[4]);
> +}
> +
> +static ssize_t kb_mode_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	u8 val;
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (kstrtou8(buf, 10, &val) || val > 3)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_SET, WMI_FN_RGB_KB_MODE,
> +				     &val, 1, NULL);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +/* RGB Color: R G B */

Please use the multicolor LED interface for this so that userspace applications can use
the standard multicolor LED API.

> +static ssize_t kb_color_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	unsigned int r, g, b;
> +	u8 color_buf[3];
> +	u8 fixed_mode = 2;
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (sscanf(buf, "%u %u %u", &r, &g, &b) != 3)
> +		return -EINVAL;
> +	if (r > 255 || g > 255 || b > 255)
> +		return -EINVAL;
> +
> +	color_buf[0] = (u8)r;
> +	color_buf[1] = (u8)g;
> +	color_buf[2] = (u8)b;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_SET, WMI_FN_RGB_KB_MODE,
> +				     &fixed_mode, 1, NULL);
> +	if (ret)
> +		goto unlock;
> +
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_SET, WMI_FN_RGB_KB_COLOR,
> +				     color_buf, 3, NULL);

Should you move the mutex handling into tongfang_mifs_wmi_call(), then you will
need a separate lock here. I leave it up to you how you handle the locking in this case,
nut please try to use guard() when suitable.

> +
> +unlock:
> +	mutex_unlock(&data->lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +/* Fan Boost: 0:Normal, 1:Max Speed */

Here using binary numbers is fine, but please use kstrtobool() instead
of kstrtou8().

> +static ssize_t fan_boost_store(struct device *dev,
> +			       struct device_attribute *attr, const char *buf,
> +			       size_t count)
> +{
> +	struct tongfang_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	u8 val, payload[2];
> +	int ret;
> +
> +	if (!data)
> +		return -EINVAL;
> +
> +	if (kstrtou8(buf, 10, &val) || val > 1)
> +		return -EINVAL;
> +
> +	payload[0] = 0; /* CPU/GPU Fan */
> +	payload[1] = val;
> +
> +	mutex_lock(&data->lock);
> +	ret = tongfang_mifs_wmi_call(data, WMI_METHOD_SET,
> +				     WMI_FN_MAX_FAN_SWITCH, payload, 2, NULL);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(gpu_mode);
> +static DEVICE_ATTR_RW(kb_mode);
> +static DEVICE_ATTR_WO(kb_color);
> +static DEVICE_ATTR_WO(fan_boost);

Please mark those attributes as const. This also requires you to mark laptop_attrs as const as well.

> +
> +static struct attribute *laptop_attrs[] = { &dev_attr_gpu_mode.attr,
> +					    &dev_attr_kb_mode.attr,
> +					    &dev_attr_kb_color.attr,
> +					    &dev_attr_fan_boost.attr, NULL };

Please use the standard kernel array style:

type example[] {
	entry1,
	entry2,
	...
}:

> +ATTRIBUTE_GROUPS(laptop);
> +
> +static int tongfang_mifs_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct tongfang_mifs_wmi_data *drv_data;
> +	struct device *pp_dev;
> +	int ret;
> +
> +	drv_data = devm_kzalloc(&wdev->dev, sizeof(*drv_data), GFP_KERNEL);
> +	if (!drv_data)
> +		return -ENOMEM;
> +
> +	drv_data->wdev = wdev;
> +	mutex_init(&drv_data->lock);

Please use devm_mutex_init() here.

> +	dev_set_drvdata(&wdev->dev, drv_data);
> +
> +	/* Register platform profile */
> +	pp_dev = devm_platform_profile_register(&wdev->dev, DRV_NAME, drv_data,
> +						&laptop_profile_ops);
> +	if (IS_ERR(pp_dev))
> +		dev_err(&wdev->dev, "Failed to register platform profile\n");

Please abort probing if you cannot register this feature. The same applies to the hwmon
and LED features.

> +
> +	/* Register hwmon */
> +	drv_data->hwmon_dev = devm_hwmon_device_register_with_info(

Lines should not end with a '('.

> +		&wdev->dev, "tongfang_mifs", drv_data, &laptop_chip_info, NULL);
> +	if (IS_ERR(drv_data->hwmon_dev))
> +		dev_err(&wdev->dev, "Failed to register hwmon\n");
> +
> +	/* Register keyboard LED */
> +	drv_data->kbd_led.name = "laptop::kbd_backlight";
> +	drv_data->kbd_led.max_brightness = 3;
> +	drv_data->kbd_led.brightness_set_blocking = laptop_kbd_led_set;
> +	ret = devm_led_classdev_register(&wdev->dev, &drv_data->kbd_led);
> +	if (ret)
> +		dev_err(&wdev->dev, "Failed to register keyboard LED\n");
> +
> +	return 0;
> +}
> +
> +static const struct wmi_device_id tongfang_mifs_wmi_id_table[] = {
> +	{ TONGFANG_MIFS_GUID, NULL },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(wmi, tongfang_mifs_wmi_id_table);
> +
> +static struct wmi_driver tongfang_mifs_wmi_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.dev_groups = laptop_groups,
> +	},
> +	.id_table = tongfang_mifs_wmi_id_table,
> +	.probe = tongfang_mifs_wmi_probe,

Please set .no_singleton = true. Also please check if the various settings (RGB LED, platform profile and sysfs)
need to be restored when resuming from hibernation and/or suspend.

All in all the basic structure of the driver seems good, except for the WMI method call handling and
the RGB LED integration.

Thanks,
Armin Wolf

> +};
> +
> +module_wmi_driver(tongfang_mifs_wmi_driver);
> +
> +MODULE_AUTHOR("Mingyou Chen <qby104326@...il.com>");
> +MODULE_DESCRIPTION("Tongfang MIFS (MiInterface) WMI driver");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists