2023-10-18 08:01:10

by Ai Chao

[permalink] [raw]
Subject: [PATCH v3] platform/x86: inspur-wmi: Add platform profile support

Add support for Inspur platforms to used the platform profile feature.

This will allow users to determine and control the platform modes
between low-power, balanced and performance modes.

change for v3
- Remove input device
- Using the platform profile interface

change for v2
- Remove Event GUID, remove inspur_wmi_notify and inspur_wmi_notify.
- Add more explanation.

Signed-off-by: Ai Chao <[email protected]>
---
drivers/platform/x86/Kconfig | 10 ++
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/inspur-wmi.c | 215 ++++++++++++++++++++++++++++++
3 files changed, 228 insertions(+)
create mode 100644 drivers/platform/x86/inspur-wmi.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 2a1070543391..d7bd27c53751 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -988,6 +988,16 @@ 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 platform profile driver"
+ depends on ACPI_WMI
+ help
+ This will allow users to determine and control the platform modes
+ between low-power, balanced and performance modes.
+
+ 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..d0c5ae2e3fcb
--- /dev/null
+++ b/drivers/platform/x86/inspur-wmi.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Inspur WMI power mode
+ *
+ * Copyright (C) 2018 Ai Chao <[email protected]>
+ */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_profile.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,
+};
+
+/**
+ * Power Mode:
+ * 0x0: Balance Mode
+ * 0x1: Performance Mode
+ * 0x2: Power Saver Mode
+ */
+enum inspur_tmp_profile {
+ INSPUR_TMP_PROFILE_BALANCE = 0,
+ INSPUR_TMP_PROFILE_PERFORMANCE = 1,
+ INSPUR_TMP_PROFILE_POWERSAVE = 2,
+};
+
+struct inspur_wmi_priv {
+ struct wmi_device *wdev;
+ struct platform_profile_handler handler;
+};
+
+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)
+{
+ struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+ struct acpi_buffer input = { insize, buffer};
+ union acpi_object *obj;
+ acpi_status status;
+ int ret = 0;
+
+ 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 ||
+ 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
+ */
+static int inspur_platform_profile_set(struct platform_profile_handler *pprof,
+ enum platform_profile_option profile)
+{
+ struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
+ handler);
+ u8 ret_code[4] = {0, 0, 0, 0};
+ int ret;
+
+ switch (profile) {
+ case PLATFORM_PROFILE_BALANCED:
+ ret_code[0] = INSPUR_TMP_PROFILE_BALANCE;
+ break;
+ case PLATFORM_PROFILE_PERFORMANCE:
+ ret_code[0] = INSPUR_TMP_PROFILE_PERFORMANCE;
+ break;
+ case PLATFORM_PROFILE_LOW_POWER:
+ ret_code[0] = INSPUR_TMP_PROFILE_POWERSAVE;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_SET_POWERMODE,
+ ret_code, sizeof(ret_code),
+ sizeof(ret_code));
+
+ if (ret < 0)
+ return ret;
+ else if (ret_code[0])
+ return -EBADRQC;
+
+ return 0;
+}
+
+/**
+ * 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 int inspur_platform_profile_get(struct platform_profile_handler *pprof,
+ enum platform_profile_option *profile)
+{
+ struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
+ handler);
+ u8 ret_code[4] = {0, 0, 0, 0};
+ int ret;
+
+ ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_GET_POWERMODE,
+ &ret_code, sizeof(ret_code),
+ sizeof(ret_code));
+ if (ret < 0)
+ return ret;
+ else if (ret_code[0])
+ return -EBADRQC;
+
+ switch (ret_code[1]) {
+ case INSPUR_TMP_PROFILE_BALANCE:
+ *profile = PLATFORM_PROFILE_BALANCED;
+ break;
+ case INSPUR_TMP_PROFILE_PERFORMANCE:
+ *profile = PLATFORM_PROFILE_PERFORMANCE;
+ break;
+ case INSPUR_TMP_PROFILE_POWERSAVE:
+ *profile = PLATFORM_PROFILE_LOW_POWER;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+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);
+
+ priv->handler.profile_get = inspur_platform_profile_get;
+ priv->handler.profile_set = inspur_platform_profile_set;
+
+ set_bit(PLATFORM_PROFILE_LOW_POWER, priv->handler.choices);
+ set_bit(PLATFORM_PROFILE_BALANCED, priv->handler.choices);
+ set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->handler.choices);
+
+ platform_profile_register(&priv->handler);
+ return 0;
+}
+
+static void inspur_wmi_remove(struct wmi_device *wdev)
+{
+ platform_profile_remove();
+}
+
+static const struct wmi_device_id inspur_wmi_id_table[] = {
+ { .guid_string = WMI_INSPUR_POWERMODE_BIOS_GUID },
+ { }
+};
+
+MODULE_DEVICE_TABLE(wmi, inspur_wmi_id_table);
+
+static struct wmi_driver inspur_wmi_driver = {
+ .driver = {
+ .name = "inspur-wmi",
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .id_table = inspur_wmi_id_table,
+ .probe = inspur_wmi_probe,
+ .remove = inspur_wmi_remove,
+};
+
+module_wmi_driver(inspur_wmi_driver);
+
+MODULE_AUTHOR("Ai Chao <[email protected]>");
+MODULE_DESCRIPTION("Inspur WMI hotkeys");
+MODULE_LICENSE("GPL");
--
2.25.1


2023-10-18 12:24:32

by Armin Wolf

[permalink] [raw]
Subject: Re: [PATCH v3] platform/x86: inspur-wmi: Add platform profile support

Am 18.10.23 um 10:00 schrieb Ai Chao:

> Add support for Inspur platforms to used the platform profile feature.
>
> This will allow users to determine and control the platform modes
> between low-power, balanced and performance modes.
>
> change for v3
> - Remove input device
> - Using the platform profile interface
>
> change for v2
> - Remove Event GUID, remove inspur_wmi_notify and inspur_wmi_notify.
> - Add more explanation.

Please move the changelog out of the commit message, see
https://lore.kernel.org/lkml/[email protected]
for an example.

> Signed-off-by: Ai Chao <[email protected]>
> ---
> drivers/platform/x86/Kconfig | 10 ++
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/inspur-wmi.c | 215 ++++++++++++++++++++++++++++++
> 3 files changed, 228 insertions(+)
> create mode 100644 drivers/platform/x86/inspur-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 2a1070543391..d7bd27c53751 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -988,6 +988,16 @@ 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 platform profile driver"
> + depends on ACPI_WMI

Please select the platform profile subsystem with "select ACPI_PLATFORM_PROFILE".

> + help
> + This will allow users to determine and control the platform modes
> + between low-power, balanced and performance modes.
> +
> + 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..d0c5ae2e3fcb
> --- /dev/null
> +++ b/drivers/platform/x86/inspur-wmi.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Inspur WMI power mode
> + *
> + * Copyright (C) 2018 Ai Chao <[email protected]>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_profile.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,
> +};
> +
> +/**
> + * Power Mode:
> + * 0x0: Balance Mode
> + * 0x1: Performance Mode
> + * 0x2: Power Saver Mode
> + */
> +enum inspur_tmp_profile {
> + INSPUR_TMP_PROFILE_BALANCE = 0,
> + INSPUR_TMP_PROFILE_PERFORMANCE = 1,
> + INSPUR_TMP_PROFILE_POWERSAVE = 2,
> +};
> +
> +struct inspur_wmi_priv {
> + struct wmi_device *wdev;
> + struct platform_profile_handler handler;
> +};
> +
> +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)
> +{
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct acpi_buffer input = { insize, buffer};
> + union acpi_object *obj;
> + acpi_status status;
> + int ret = 0;
> +
> + 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 ||
> + 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
> + */
> +static int inspur_platform_profile_set(struct platform_profile_handler *pprof,
> + enum platform_profile_option profile)
> +{
> + struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
> + handler);
> + u8 ret_code[4] = {0, 0, 0, 0};
> + int ret;
> +
> + switch (profile) {
> + case PLATFORM_PROFILE_BALANCED:
> + ret_code[0] = INSPUR_TMP_PROFILE_BALANCE;
> + break;
> + case PLATFORM_PROFILE_PERFORMANCE:
> + ret_code[0] = INSPUR_TMP_PROFILE_PERFORMANCE;
> + break;
> + case PLATFORM_PROFILE_LOW_POWER:
> + ret_code[0] = INSPUR_TMP_PROFILE_POWERSAVE;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_SET_POWERMODE,
> + ret_code, sizeof(ret_code),
> + sizeof(ret_code));
> +
> + if (ret < 0)
> + return ret;
> + else if (ret_code[0])
> + return -EBADRQC;

That "else" is pointless due to the early return. Please move the ret_code[0] check
out of the else clause, like this:

if (ret < 0)
return ret;

if (ret_code[0])
return -EBADRQC;

> +
> + return 0;
> +}
> +
> +/**
> + * 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 int inspur_platform_profile_get(struct platform_profile_handler *pprof,
> + enum platform_profile_option *profile)
> +{
> + struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
> + handler);
> + u8 ret_code[4] = {0, 0, 0, 0};
> + int ret;
> +
> + ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_GET_POWERMODE,
> + &ret_code, sizeof(ret_code),
> + sizeof(ret_code));
> + if (ret < 0)
> + return ret;
> + else if (ret_code[0])
> + return -EBADRQC;

Same as above.

> +
> + switch (ret_code[1]) {
> + case INSPUR_TMP_PROFILE_BALANCE:
> + *profile = PLATFORM_PROFILE_BALANCED;
> + break;
> + case INSPUR_TMP_PROFILE_PERFORMANCE:
> + *profile = PLATFORM_PROFILE_PERFORMANCE;
> + break;
> + case INSPUR_TMP_PROFILE_POWERSAVE:
> + *profile = PLATFORM_PROFILE_LOW_POWER;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +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);
> +
> + priv->handler.profile_get = inspur_platform_profile_get;
> + priv->handler.profile_set = inspur_platform_profile_set;
> +
> + set_bit(PLATFORM_PROFILE_LOW_POWER, priv->handler.choices);
> + set_bit(PLATFORM_PROFILE_BALANCED, priv->handler.choices);
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->handler.choices);
> +
> + platform_profile_register(&priv->handler);
> + return 0;

I said that the surface platform profile driver was a good example _except_ for this
minor issue. Please return the result of platform_profile_register(), see
https://lore.kernel.org/lkml/[email protected] for an explanation.

Other than that and the remaining issues above, the driver looks good to me.

Armin Wolf

> +}
> +
> +static void inspur_wmi_remove(struct wmi_device *wdev)
> +{
> + platform_profile_remove();
> +}
> +
> +static const struct wmi_device_id inspur_wmi_id_table[] = {
> + { .guid_string = WMI_INSPUR_POWERMODE_BIOS_GUID },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(wmi, inspur_wmi_id_table);
> +
> +static struct wmi_driver inspur_wmi_driver = {
> + .driver = {
> + .name = "inspur-wmi",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = inspur_wmi_id_table,
> + .probe = inspur_wmi_probe,
> + .remove = inspur_wmi_remove,
> +};
> +
> +module_wmi_driver(inspur_wmi_driver);
> +
> +MODULE_AUTHOR("Ai Chao <[email protected]>");
> +MODULE_DESCRIPTION("Inspur WMI hotkeys");
> +MODULE_LICENSE("GPL");

2023-10-18 13:38:35

by Thomas Weißschuh

[permalink] [raw]
Subject: Re: [PATCH v3] platform/x86: inspur-wmi: Add platform profile support

On 2023-10-18 16:00:14+0800, Ai Chao wrote:
> Add support for Inspur platforms to used the platform profile feature.
>
> This will allow users to determine and control the platform modes
> between low-power, balanced and performance modes.
>
> change for v3
> - Remove input device
> - Using the platform profile interface
>
> change for v2
> - Remove Event GUID, remove inspur_wmi_notify and inspur_wmi_notify.
> - Add more explanation.
>
> Signed-off-by: Ai Chao <[email protected]>
> ---
> drivers/platform/x86/Kconfig | 10 ++
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/inspur-wmi.c | 215 ++++++++++++++++++++++++++++++
> 3 files changed, 228 insertions(+)
> create mode 100644 drivers/platform/x86/inspur-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 2a1070543391..d7bd27c53751 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -988,6 +988,16 @@ 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 platform profile driver"
> + depends on ACPI_WMI
> + help
> + This will allow users to determine and control the platform modes
> + between low-power, balanced and performance modes.
> +
> + 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..d0c5ae2e3fcb
> --- /dev/null
> +++ b/drivers/platform/x86/inspur-wmi.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Inspur WMI power mode
> + *
> + * Copyright (C) 2018 Ai Chao <[email protected]>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_profile.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,
> +};
> +
> +/**
> + * Power Mode:
> + * 0x0: Balance Mode
> + * 0x1: Performance Mode
> + * 0x2: Power Saver Mode
> + */
> +enum inspur_tmp_profile {
> + INSPUR_TMP_PROFILE_BALANCE = 0,
> + INSPUR_TMP_PROFILE_PERFORMANCE = 1,
> + INSPUR_TMP_PROFILE_POWERSAVE = 2,
> +};
> +
> +struct inspur_wmi_priv {
> + struct wmi_device *wdev;
> + struct platform_profile_handler handler;
> +};
> +
> +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)
> +{
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct acpi_buffer input = { insize, buffer};
> + union acpi_object *obj;
> + acpi_status status;
> + int ret = 0;
> +
> + 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 ||
> + 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
> + */
> +static int inspur_platform_profile_set(struct platform_profile_handler *pprof,
> + enum platform_profile_option profile)
> +{
> + struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
> + handler);
> + u8 ret_code[4] = {0, 0, 0, 0};
> + int ret;
> +
> + switch (profile) {
> + case PLATFORM_PROFILE_BALANCED:
> + ret_code[0] = INSPUR_TMP_PROFILE_BALANCE;
> + break;
> + case PLATFORM_PROFILE_PERFORMANCE:
> + ret_code[0] = INSPUR_TMP_PROFILE_PERFORMANCE;
> + break;
> + case PLATFORM_PROFILE_LOW_POWER:
> + ret_code[0] = INSPUR_TMP_PROFILE_POWERSAVE;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_SET_POWERMODE,
> + ret_code, sizeof(ret_code),
> + sizeof(ret_code));
> +
> + if (ret < 0)
> + return ret;
> + else if (ret_code[0])
> + return -EBADRQC;
> +
> + return 0;
> +}
> +
> +/**
> + * 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 int inspur_platform_profile_get(struct platform_profile_handler *pprof,
> + enum platform_profile_option *profile)
> +{
> + struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
> + handler);
> + u8 ret_code[4] = {0, 0, 0, 0};
> + int ret;
> +
> + ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_GET_POWERMODE,
> + &ret_code, sizeof(ret_code),
> + sizeof(ret_code));
> + if (ret < 0)
> + return ret;
> + else if (ret_code[0])
> + return -EBADRQC;
> +
> + switch (ret_code[1]) {
> + case INSPUR_TMP_PROFILE_BALANCE:
> + *profile = PLATFORM_PROFILE_BALANCED;
> + break;
> + case INSPUR_TMP_PROFILE_PERFORMANCE:
> + *profile = PLATFORM_PROFILE_PERFORMANCE;
> + break;
> + case INSPUR_TMP_PROFILE_POWERSAVE:
> + *profile = PLATFORM_PROFILE_LOW_POWER;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +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);
> +
> + priv->handler.profile_get = inspur_platform_profile_get;
> + priv->handler.profile_set = inspur_platform_profile_set;
> +
> + set_bit(PLATFORM_PROFILE_LOW_POWER, priv->handler.choices);
> + set_bit(PLATFORM_PROFILE_BALANCED, priv->handler.choices);
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->handler.choices);
> +
> + platform_profile_register(&priv->handler);
> + return 0;
> +}
> +
> +static void inspur_wmi_remove(struct wmi_device *wdev)
> +{
> + platform_profile_remove();
> +}
> +
> +static const struct wmi_device_id inspur_wmi_id_table[] = {
> + { .guid_string = WMI_INSPUR_POWERMODE_BIOS_GUID },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(wmi, inspur_wmi_id_table);
> +
> +static struct wmi_driver inspur_wmi_driver = {
> + .driver = {
> + .name = "inspur-wmi",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,

Seems unnecessary.
The docs say this should be used for "slow" devices.
But the probe function here doesn't really do anything.

> + },
> + .id_table = inspur_wmi_id_table,
> + .probe = inspur_wmi_probe,
> + .remove = inspur_wmi_remove,
> +};
> +
> +module_wmi_driver(inspur_wmi_driver);
> +
> +MODULE_AUTHOR("Ai Chao <[email protected]>");
> +MODULE_DESCRIPTION("Inspur WMI hotkeys");

There is no more hotkey functionality left.
Kconfig says "Inspur WMI platform profile driver",
should also be used here.

If you are also going to submit the hotkey driver it may make sense to
call this one inspur-wmi-platform-profile, or something similar unique.

> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>

2023-10-18 13:49:16

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v3] platform/x86: inspur-wmi: Add platform profile support

Hi,

On 10/18/23 10:00, Ai Chao wrote:
> Add support for Inspur platforms to used the platform profile feature.
>
> This will allow users to determine and control the platform modes
> between low-power, balanced and performance modes.
>
> change for v3
> - Remove input device
> - Using the platform profile interface

Thanks this is much better.

Only remaining question I have is can the platform_profile
setting be changed by a hotkey (directly by the hotkey,
so upon pressing the hotkey the EC changes the platform_profile
itself).

If the answer to this is yes, is there then any event which
the driver could listen to and then use to notify userspace
about the change by calling platform_profile_notify() upon
receiving the event ?

Or maybe the event will be received by the discussed
hotkeys driver, so that can call platform_profile_notify()
instead. Note platform_profile_notify() does not require a
platform_profile_handler pointer, so it could indeed be
called from a separate driver if the events are received
elsewhere.

Regards,

Hans




>
> change for v2
> - Remove Event GUID, remove inspur_wmi_notify and inspur_wmi_notify.
> - Add more explanation.
>
> Signed-off-by: Ai Chao <[email protected]>
> ---
> drivers/platform/x86/Kconfig | 10 ++
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/inspur-wmi.c | 215 ++++++++++++++++++++++++++++++
> 3 files changed, 228 insertions(+)
> create mode 100644 drivers/platform/x86/inspur-wmi.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 2a1070543391..d7bd27c53751 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -988,6 +988,16 @@ 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 platform profile driver"
> + depends on ACPI_WMI
> + help
> + This will allow users to determine and control the platform modes
> + between low-power, balanced and performance modes.
> +
> + 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..d0c5ae2e3fcb
> --- /dev/null
> +++ b/drivers/platform/x86/inspur-wmi.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Inspur WMI power mode
> + *
> + * Copyright (C) 2018 Ai Chao <[email protected]>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_profile.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,
> +};
> +
> +/**
> + * Power Mode:
> + * 0x0: Balance Mode
> + * 0x1: Performance Mode
> + * 0x2: Power Saver Mode
> + */
> +enum inspur_tmp_profile {
> + INSPUR_TMP_PROFILE_BALANCE = 0,
> + INSPUR_TMP_PROFILE_PERFORMANCE = 1,
> + INSPUR_TMP_PROFILE_POWERSAVE = 2,
> +};
> +
> +struct inspur_wmi_priv {
> + struct wmi_device *wdev;
> + struct platform_profile_handler handler;
> +};
> +
> +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)
> +{
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct acpi_buffer input = { insize, buffer};
> + union acpi_object *obj;
> + acpi_status status;
> + int ret = 0;
> +
> + 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 ||
> + 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
> + */
> +static int inspur_platform_profile_set(struct platform_profile_handler *pprof,
> + enum platform_profile_option profile)
> +{
> + struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
> + handler);
> + u8 ret_code[4] = {0, 0, 0, 0};
> + int ret;
> +
> + switch (profile) {
> + case PLATFORM_PROFILE_BALANCED:
> + ret_code[0] = INSPUR_TMP_PROFILE_BALANCE;
> + break;
> + case PLATFORM_PROFILE_PERFORMANCE:
> + ret_code[0] = INSPUR_TMP_PROFILE_PERFORMANCE;
> + break;
> + case PLATFORM_PROFILE_LOW_POWER:
> + ret_code[0] = INSPUR_TMP_PROFILE_POWERSAVE;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_SET_POWERMODE,
> + ret_code, sizeof(ret_code),
> + sizeof(ret_code));
> +
> + if (ret < 0)
> + return ret;
> + else if (ret_code[0])
> + return -EBADRQC;
> +
> + return 0;
> +}
> +
> +/**
> + * 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 int inspur_platform_profile_get(struct platform_profile_handler *pprof,
> + enum platform_profile_option *profile)
> +{
> + struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
> + handler);
> + u8 ret_code[4] = {0, 0, 0, 0};
> + int ret;
> +
> + ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_GET_POWERMODE,
> + &ret_code, sizeof(ret_code),
> + sizeof(ret_code));
> + if (ret < 0)
> + return ret;
> + else if (ret_code[0])
> + return -EBADRQC;
> +
> + switch (ret_code[1]) {
> + case INSPUR_TMP_PROFILE_BALANCE:
> + *profile = PLATFORM_PROFILE_BALANCED;
> + break;
> + case INSPUR_TMP_PROFILE_PERFORMANCE:
> + *profile = PLATFORM_PROFILE_PERFORMANCE;
> + break;
> + case INSPUR_TMP_PROFILE_POWERSAVE:
> + *profile = PLATFORM_PROFILE_LOW_POWER;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +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);
> +
> + priv->handler.profile_get = inspur_platform_profile_get;
> + priv->handler.profile_set = inspur_platform_profile_set;
> +
> + set_bit(PLATFORM_PROFILE_LOW_POWER, priv->handler.choices);
> + set_bit(PLATFORM_PROFILE_BALANCED, priv->handler.choices);
> + set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->handler.choices);
> +
> + platform_profile_register(&priv->handler);
> + return 0;
> +}
> +
> +static void inspur_wmi_remove(struct wmi_device *wdev)
> +{
> + platform_profile_remove();
> +}
> +
> +static const struct wmi_device_id inspur_wmi_id_table[] = {
> + { .guid_string = WMI_INSPUR_POWERMODE_BIOS_GUID },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(wmi, inspur_wmi_id_table);
> +
> +static struct wmi_driver inspur_wmi_driver = {
> + .driver = {
> + .name = "inspur-wmi",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = inspur_wmi_id_table,
> + .probe = inspur_wmi_probe,
> + .remove = inspur_wmi_remove,
> +};
> +
> +module_wmi_driver(inspur_wmi_driver);
> +
> +MODULE_AUTHOR("Ai Chao <[email protected]>");
> +MODULE_DESCRIPTION("Inspur WMI hotkeys");
> +MODULE_LICENSE("GPL");

2023-10-18 14:38:21

by Armin Wolf

[permalink] [raw]
Subject: Re: [PATCH v3] platform/x86: inspur-wmi: Add platform profile support

Am 18.10.23 um 15:38 schrieb Thomas Weißschuh:

> On 2023-10-18 16:00:14+0800, Ai Chao wrote:
>> Add support for Inspur platforms to used the platform profile feature.
>>
>> This will allow users to determine and control the platform modes
>> between low-power, balanced and performance modes.
>>
>> change for v3
>> - Remove input device
>> - Using the platform profile interface
>>
>> change for v2
>> - Remove Event GUID, remove inspur_wmi_notify and inspur_wmi_notify.
>> - Add more explanation.
>>
>> Signed-off-by: Ai Chao <[email protected]>
>> ---
>> drivers/platform/x86/Kconfig | 10 ++
>> drivers/platform/x86/Makefile | 3 +
>> drivers/platform/x86/inspur-wmi.c | 215 ++++++++++++++++++++++++++++++
>> 3 files changed, 228 insertions(+)
>> create mode 100644 drivers/platform/x86/inspur-wmi.c
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 2a1070543391..d7bd27c53751 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -988,6 +988,16 @@ 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 platform profile driver"
>> + depends on ACPI_WMI
>> + help
>> + This will allow users to determine and control the platform modes
>> + between low-power, balanced and performance modes.
>> +
>> + 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..d0c5ae2e3fcb
>> --- /dev/null
>> +++ b/drivers/platform/x86/inspur-wmi.c
>> @@ -0,0 +1,215 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Inspur WMI power mode
>> + *
>> + * Copyright (C) 2018 Ai Chao <[email protected]>
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_profile.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,
>> +};
>> +
>> +/**
>> + * Power Mode:
>> + * 0x0: Balance Mode
>> + * 0x1: Performance Mode
>> + * 0x2: Power Saver Mode
>> + */
>> +enum inspur_tmp_profile {
>> + INSPUR_TMP_PROFILE_BALANCE = 0,
>> + INSPUR_TMP_PROFILE_PERFORMANCE = 1,
>> + INSPUR_TMP_PROFILE_POWERSAVE = 2,
>> +};
>> +
>> +struct inspur_wmi_priv {
>> + struct wmi_device *wdev;
>> + struct platform_profile_handler handler;
>> +};
>> +
>> +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)
>> +{
>> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>> + struct acpi_buffer input = { insize, buffer};
>> + union acpi_object *obj;
>> + acpi_status status;
>> + int ret = 0;
>> +
>> + 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 ||
>> + 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
>> + */
>> +static int inspur_platform_profile_set(struct platform_profile_handler *pprof,
>> + enum platform_profile_option profile)
>> +{
>> + struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
>> + handler);
>> + u8 ret_code[4] = {0, 0, 0, 0};
>> + int ret;
>> +
>> + switch (profile) {
>> + case PLATFORM_PROFILE_BALANCED:
>> + ret_code[0] = INSPUR_TMP_PROFILE_BALANCE;
>> + break;
>> + case PLATFORM_PROFILE_PERFORMANCE:
>> + ret_code[0] = INSPUR_TMP_PROFILE_PERFORMANCE;
>> + break;
>> + case PLATFORM_PROFILE_LOW_POWER:
>> + ret_code[0] = INSPUR_TMP_PROFILE_POWERSAVE;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_SET_POWERMODE,
>> + ret_code, sizeof(ret_code),
>> + sizeof(ret_code));
>> +
>> + if (ret < 0)
>> + return ret;
>> + else if (ret_code[0])
>> + return -EBADRQC;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * 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 int inspur_platform_profile_get(struct platform_profile_handler *pprof,
>> + enum platform_profile_option *profile)
>> +{
>> + struct inspur_wmi_priv *priv = container_of(pprof, struct inspur_wmi_priv,
>> + handler);
>> + u8 ret_code[4] = {0, 0, 0, 0};
>> + int ret;
>> +
>> + ret = inspur_wmi_perform_query(priv->wdev, INSPUR_WMI_GET_POWERMODE,
>> + &ret_code, sizeof(ret_code),
>> + sizeof(ret_code));
>> + if (ret < 0)
>> + return ret;
>> + else if (ret_code[0])
>> + return -EBADRQC;
>> +
>> + switch (ret_code[1]) {
>> + case INSPUR_TMP_PROFILE_BALANCE:
>> + *profile = PLATFORM_PROFILE_BALANCED;
>> + break;
>> + case INSPUR_TMP_PROFILE_PERFORMANCE:
>> + *profile = PLATFORM_PROFILE_PERFORMANCE;
>> + break;
>> + case INSPUR_TMP_PROFILE_POWERSAVE:
>> + *profile = PLATFORM_PROFILE_LOW_POWER;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +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);
>> +
>> + priv->handler.profile_get = inspur_platform_profile_get;
>> + priv->handler.profile_set = inspur_platform_profile_set;
>> +
>> + set_bit(PLATFORM_PROFILE_LOW_POWER, priv->handler.choices);
>> + set_bit(PLATFORM_PROFILE_BALANCED, priv->handler.choices);
>> + set_bit(PLATFORM_PROFILE_PERFORMANCE, priv->handler.choices);
>> +
>> + platform_profile_register(&priv->handler);
>> + return 0;
>> +}
>> +
>> +static void inspur_wmi_remove(struct wmi_device *wdev)
>> +{
>> + platform_profile_remove();
>> +}
>> +
>> +static const struct wmi_device_id inspur_wmi_id_table[] = {
>> + { .guid_string = WMI_INSPUR_POWERMODE_BIOS_GUID },
>> + { }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(wmi, inspur_wmi_id_table);
>> +
>> +static struct wmi_driver inspur_wmi_driver = {
>> + .driver = {
>> + .name = "inspur-wmi",
>> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> Seems unnecessary.
> The docs say this should be used for "slow" devices.
> But the probe function here doesn't really do anything.

The reason why i prefer drivers to enable asynchronous probing is that
without enabling it by default, asynchronous probing wont have any chance
of becoming the default someday.

Additionally, the WMI core might call some ACPI methods when probing an
WMI device, and those can be quite slow. By enabling asynchronous probing,
we avoid any slowdowns in such a case.

Armin Wolf

>> + },
>> + .id_table = inspur_wmi_id_table,
>> + .probe = inspur_wmi_probe,
>> + .remove = inspur_wmi_remove,
>> +};
>> +
>> +module_wmi_driver(inspur_wmi_driver);
>> +
>> +MODULE_AUTHOR("Ai Chao <[email protected]>");
>> +MODULE_DESCRIPTION("Inspur WMI hotkeys");
> There is no more hotkey functionality left.
> Kconfig says "Inspur WMI platform profile driver",
> should also be used here.
>
> If you are also going to submit the hotkey driver it may make sense to
> call this one inspur-wmi-platform-profile, or something similar unique.
>
>> +MODULE_LICENSE("GPL");
>> --
>> 2.25.1
>>

2023-10-19 12:32:54

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v3] platform/x86: inspur-wmi: Add platform profile support

Hi,

On 10/19/23 04:20, 艾超 wrote:
> Hi.
>
>
>> Thanks this is much better.
>
>> Only remaining question I have is can the platform_profile
>> setting be changed by a hotkey (directly by the hotkey,
>> so upon pressing the hotkey the EC changes the platform_profile
>> itself).
>
>> If the answer to this is yes, is there then any event which
>> the driver could listen to and then use to notify userspace
>> about the change by calling platform_profile_notify() upon
>> receiving the event ?
>
>> Or maybe the event will be received by the discussed
>> hotkeys driver, so that can call platform_profile_notify()
>> instead. Note platform_profile_notify() does not require a
>> platform_profile_handler pointer, so it could indeed be
>> called from a separate driver if the events are received
>> elsewhere.
>
>> Regards,
>
>> Hans
>
> There is no any event need to listen by this driver. If user
>
> pressing the hotkey, desktop applications receive
>
> hotkeys(Fn+Q) event, then APP get the currently power mode
> and change it. The desktop applications modify brightness
>
> based on power mode. EC can't changes the power mode
>
> itself.

OK, that is good to know. Thank you for answering
my questions about this.

I believe that once the few small remaining review remarks
are addressed this should be ready for merging then.

Regards,

Hans