Message-ID: <d3245372-0cd4-49ff-acbc-87ff156a00fc@gmx.de>
Date:   Sun, 15 Oct 2023 01:39:15 +0200
From:   Armin Wolf <W_Armin@....de>
To:     Ai Chao <aichao@...inos.cn>, hdegoede@...hat.com,
        ilpo.jarvinen@...ux.intel.com, markgross@...nel.org
Cc:     linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2] platform/x86: support to store/show powermode value
 for Inspur

Am 14.10.23 um 05:28 schrieb Ai Chao:

> Support to store/show powermode value for Inspur by WMI interface.
> This driver provides support for Inspur WMI hotkeys. User used Fn+Q to
> change the power mode. If desktop applications receive hotkey(Fn+Q)
> event, then it get the currently power mode and change the power mode.
> The desktop applications modify brightness and cpufreq based on
> power mode.
>
> change for v2
> - Remove Event GUID, remove inspur_wmi_notify and inspur_wmi_notify.
> - Add more explanation.
>
> Signed-off-by: Ai Chao <aichao@...inos.cn>
> ---
>   drivers/platform/x86/Kconfig      |  14 ++
>   drivers/platform/x86/Makefile     |   3 +
>   drivers/platform/x86/inspur-wmi.c | 210 ++++++++++++++++++++++++++++++
>   3 files changed, 227 insertions(+)
>   create mode 100644 drivers/platform/x86/inspur-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 2a1070543391..fa2a4335c83d 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -988,6 +988,20 @@ config TOUCHSCREEN_DMI
>   	  the OS-image for the device. This option supplies the missing info.
>   	  Enable this for x86 tablets with Silead or Chipone touchscreens.
>
> +config INSPUR_WMI
> +	tristate "Inspur WMI hotkeys driver"
> +	depends on ACPI_WMI
> +	depends on INPUT

The driver does not provide a input device anymore, please remove the
dependency on CONFIG_INPUT.

> +	help
> +	This driver provides support for Inspur WMI hotkeys.
> +	User used Fn+Q to change the power mode. If desktop applications
> +	receive hotkeys(Fn+Q) event, then it get the currently power mode
> +	and change the power mode. The desktop applications modify brightness
> +	and cpufreq based on power mode.
> +

I assume the whole hotkey stuff uses the other WMI GUID, right? In this case the
current driver should not be called "Inspur WMI hotkeys driver", since it does not
handle those hotkey notifications. Better call it "Inspur WMI power mode driver",
and remove all references to the hotkeys.

> +	To compile this driver as a module, choose M here: the module
> +	will be called inspur-wmi.
> +
>   source "drivers/platform/x86/x86-android-tablets/Kconfig"
>
>   config FW_ATTR_CLASS
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index b457de5abf7d..9285c252757e 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -98,6 +98,9 @@ obj-$(CONFIG_TOSHIBA_WMI)	+= toshiba-wmi.o
>   # before toshiba_acpi initializes
>   obj-$(CONFIG_ACPI_TOSHIBA)	+= toshiba_acpi.o
>
> +# Inspur
> +obj-$(CONFIG_INSPUR_WMI)	+= inspur-wmi.o
> +
>   # Laptop drivers
>   obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
>   obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
> diff --git a/drivers/platform/x86/inspur-wmi.c b/drivers/platform/x86/inspur-wmi.c
> new file mode 100644
> index 000000000000..ef6cfd87f074
> --- /dev/null
> +++ b/drivers/platform/x86/inspur-wmi.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Inspur WMI hotkeys
> + *
> + *  Copyright (C) 2018	      Ai Chao <aichao@...inos.cn>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +
> +#define WMI_INSPUR_POWERMODE_BIOS_GUID "596C31E3-332D-43C9-AEE9-585493284F5D"
> +
> +enum inspur_wmi_method_ids {
> +	INSPUR_WMI_GET_POWERMODE = 0x02,
> +	INSPUR_WMI_SET_POWERMODE = 0x03,
> +};
> +
> +struct inspur_wmi_priv {
> +	struct input_dev *idev;
> +	struct wmi_device *wdev;
> +};
> +
> +static int inspur_wmi_perform_query(struct wmi_device *wdev,
> +				    enum inspur_wmi_method_ids query_id,
> +				    void *buffer, size_t insize,
> +				    size_t outsize)
> +{
> +	union acpi_object *obj;
> +	acpi_status status;
> +	int ret = 0;
> +	struct acpi_buffer input = { insize, buffer};
> +	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };

Please order the variable declarations in a reverse xmas tree order,
seeDocumentation/process/maintainer-netdev.rst#Local variable ordering for
details.

> +
> +	status = wmidev_evaluate_method(wdev, 0, query_id, &input, &output);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&wdev->dev, "EC Powermode control failed: %s\n",
> +			acpi_format_exception(status));
> +		return -EIO;
> +	}
> +
> +	obj = output.pointer;
> +	if (!obj)
> +		return -EINVAL;
> +
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		ret = -EINVAL;
> +		goto out_free;
> +	}
> +
> +	/* Ignore output data of zero size */
> +	if (!outsize)
> +		goto out_free;

Since all users of this functions set outsize to something greater than zero,
this check is pointless. Please remove it.

> +
> +	if (obj->buffer.length != outsize) {
> +		ret = -EINVAL;
> +		goto out_free;
> +	}
> +
> +	memcpy(buffer, obj->buffer.pointer, obj->buffer.length);
> +
> +out_free:
> +	kfree(obj);
> +	return ret;
> +}
> +
> +/**
> + * Set Power Mode to EC RAM. If Power Mode value greater than 0x3,
> + * return error
> + * Method ID: 0x3
> + * Arg: 4 Bytes
> + * Byte [0]: Power Mode:
> + *         0x0: Balance Mode
> + *         0x1: Performance Mode
> + *         0x2: Power Saver Mode
> + * Return Value: 4 Bytes
> + * Byte [0]: Return Code
> + *         0x0: No Error
> + *         0x1: Error
> + */

Now i understand what this power mode means. Why not use the platform profile interface
for this? All tree modes (Balance, Performance, Power Saver) would perfectly map to standard
platform profiles (BALANCED, PERFORMANCE, LOW_POWER), and using the platform profile interface
would allow userspace software to control the power mode over a standard interface.

I believe drivers/platform/surface/surface_platform_profile.c is a good example for such a driver,
_except_ that it ignores the return value of platform_profile_register().

> +static ssize_t powermode_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct inspur_wmi_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +	u32 mode;

Since the buffer passed to the WMI method is structured in bytes, maybe you could use a
byte array in those cases? This would remove the need to cast "mode" to a "u8 *".

> +	u8 *ret_code;
> +
> +	ret = kstrtoint(buf, 0, &mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = inspur_wmi_perform_query(priv->wdev,
> +				       INSPUR_WMI_SET_POWERMODE,
> +				       &mode, sizeof(mode), sizeof(mode));
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ret_code = (u8 *)(&mode);
> +	if (ret_code[0])
> +		return -EBADRQC;
> +
> +	return count;
> +}
> +
> +/**
> + * Get Power Mode from EC RAM, If Power Mode value greater than 0x3,
> + * return error
> + * Method ID: 0x2
> + * Return Value: 4 Bytes
> + * Byte [0]: Return Code
> + *         0x0: No Error
> + *         0x1: Error
> + * Byte [1]: Power Mode
> + *         0x0: Balance Mode
> + *         0x1: Performance Mode
> + *         0x2: Power Saver Mode
> + */
> +static ssize_t powermode_show(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct inspur_wmi_priv *priv = dev_get_drvdata(dev);
> +	u32 mode = 0;

Same as above.

> +	int ret;
> +	u8 *ret_code;
> +
> +	ret = inspur_wmi_perform_query(priv->wdev,
> +				       INSPUR_WMI_GET_POWERMODE,
> +				       &mode, sizeof(mode), sizeof(mode));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret_code = (u8 *)(&mode);
> +	if (ret_code[0])
> +		return -EBADRQC;
> +
> +	return sprintf(buf, "%u\n", ret_code[1]);
> +}
> +
> +static DEVICE_ATTR_RW(powermode);
> +
> +static struct attribute *inspur_wmi_attrs[] = {
> +	&dev_attr_powermode.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group inspur_wmi_group = {
> +	.attrs = inspur_wmi_attrs,
> +};
> +
> +static const struct attribute_group *inspur_wmi_groups[] = {
> +	&inspur_wmi_group,
> +	NULL,
> +};
> +
> +static int inspur_wmi_input_setup(struct wmi_device *wdev)
> +{
> +	struct inspur_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> +
> +	priv->idev = devm_input_allocate_device(&wdev->dev);
> +	if (!priv->idev)
> +		return -ENOMEM;
> +
> +	priv->idev->name = "Inspur WMI hotkeys";
> +	priv->idev->phys = "wmi/input0";
> +	priv->idev->id.bustype = BUS_HOST;
> +	priv->idev->dev.parent = &wdev->dev;
> +
> +	return input_register_device(priv->idev);
> +}

Please remove all remains of the hotkey handling.

> +
> +static int inspur_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct inspur_wmi_priv *priv;
> +
> +	priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->wdev = wdev;
> +	dev_set_drvdata(&wdev->dev, priv);
> +
> +	return inspur_wmi_input_setup(wdev);
> +}
> +
> +static const struct wmi_device_id inspur_wmi_id_table[] = {
> +	{ .guid_string = WMI_INSPUR_POWERMODE_BIOS_GUID },
> +	{  }
> +};
> +
> +static struct wmi_driver inspur_wmi_driver = {
> +	.driver = {
> +		.name = "inspur-wmi",
> +		.dev_groups = inspur_wmi_groups,
> +	},
> +	.id_table = inspur_wmi_id_table,
> +	.probe = inspur_wmi_probe,
> +};
> +
> +module_wmi_driver(inspur_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, inspur_wmi_id_table);

Can you move this right below "inspur_wmi_id_table" please?

> +MODULE_AUTHOR("Ai Chao <aichao@...inos.cn>");
> +MODULE_DESCRIPTION("Inspur WMI hotkeys");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists