* Re: [PATCH 1/1] platform/x86: add Acer battery control driver
2026-01-05 17:10 ` [PATCH 1/1] platform/x86: " Jelle van der Waa
@ 2026-01-06 9:34 ` Ilpo Järvinen
2026-01-08 13:50 ` Armin Wolf
1 sibling, 0 replies; 9+ messages in thread
From: Ilpo Järvinen @ 2026-01-06 9:34 UTC (permalink / raw)
To: Jelle van der Waa; +Cc: Hans de Goede, platform-driver-x86, Frederik Harwath
On Mon, 5 Jan 2026, Jelle van der Waa wrote:
> Some Acer laptops can configure battery related features through Acer
> Care Center on Windows. This driver uses the power supply extension to
> set a battery charge limit and exposes the battery
> temperature.
>
> This driver is based on the existing acer-wmi-battery project on GitHub
> and was tested on an Acer Aspire A315-510P.
>
> Signed-off-by: Jelle van der Waa <jelle@vdwaa.nl>
> ---
> drivers/platform/x86/Kconfig | 12 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/acer-wmi-battery.c | 345 ++++++++++++++++++++++++
> 3 files changed, 358 insertions(+)
> create mode 100644 drivers/platform/x86/acer-wmi-battery.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 4cb7d97a9fcc..b3de6a2827e8 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -170,6 +170,18 @@ config ACER_WMI
> If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M
> here.
>
> +config ACER_WMI_BATTERY
> + tristate "Acer WMI Battery"
> + depends on ACPI_WMI
> + depends on ACPI_BATTERY
> + depends on HWMON
> + help
> + This is a driver for Acer laptops with battery health control. It
> + adds charge limit control and battery temperature reporting.
> +
> + If you have an ACPI-WMI Battery compatible Acer laptop, say Y or M
> + here.
> +
> source "drivers/platform/x86/amd/Kconfig"
>
> config ADV_SWBUTTON
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index d25762f7114f..9cf28baff3ae 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
> obj-$(CONFIG_ACERHDF) += acerhdf.o
> obj-$(CONFIG_ACER_WIRELESS) += acer-wireless.o
> obj-$(CONFIG_ACER_WMI) += acer-wmi.o
> +obj-$(CONFIG_ACER_WMI_BATTERY) += acer-wmi-battery.o
>
> # AMD
> obj-y += amd/
> diff --git a/drivers/platform/x86/acer-wmi-battery.c b/drivers/platform/x86/acer-wmi-battery.c
> new file mode 100644
> index 000000000000..fffa521cde27
> --- /dev/null
> +++ b/drivers/platform/x86/acer-wmi-battery.c
> @@ -0,0 +1,345 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * acer-wmi-battery.c: Acer battery health control driver
> + *
> + * This is a driver for the WMI battery health control interface found
> + * on some Acer laptops. This interface allows to enable/disable a
> + * battery charge limit ("health mode") and exposes the battery temperature.
> + *
> + * Based on acer-wmi-battery https://github.com/frederik-h/acer-wmi-battery/
Add an empty line here.
> + * Copyright (C) 2022-2025 Frederik Harwath <frederik@harwath.name>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/power_supply.h>
> +#include <linux/version.h>
> +#include <linux/wmi.h>
> +#include <linux/unaligned.h>
Always use alphabetical order within each block of includes (such as
those under linux/).
> +#include <acpi/battery.h>
> +
> +#define DRIVER_NAME "acer-wmi-battery"
> +
> +#define ACER_BATTERY_GUID "79772EC5-04B1-4BFD-843C-61E7F77B6CC9"
> +
> +/*
> + * The Acer OEM software seems to always use this battery index,
> + * so we emulate this behaviour to not confuse the underlying firmware.
> + *
> + * However this also means that we only fully support devices with a
> + * single battery for now.
> + */
> +#define ACER_BATTERY_INDEX 0x1
> +
> +struct get_battery_health_control_status_input {
> + u8 uBatteryNo;
> + u8 uFunctionQuery;
> + u8 uReserved[2];
+ types.h
> +} __packed;
__packed needs #include.
> +struct get_battery_health_control_status_output {
> + u8 uFunctionList;
> + u8 uReturn[2];
> + u8 uFunctionStatus[5];
> +} __packed;
> +
> +struct set_battery_health_control_input {
> + u8 uBatteryNo;
> + u8 uFunctionMask;
> + u8 uFunctionStatus;
> + u8 uReservedIn[5];
> +} __packed;
> +
> +struct set_battery_health_control_output {
> + u8 uReturn;
> + u8 uReservedOut;
> +} __packed;
> +
> +enum battery_mode { HEALTH_MODE = 1, CALIBRATION_MODE = 2 };
Do we expect to ever extend this list? If yes, put them on separate lines
and add the trailing comma.
Are these BIT() or values of a field, you seem to use HEALTH_MODE with &
down below which sounds like BIT()?
> +
> +struct acer_wmi_battery_data {
> + struct acpi_battery_hook hook;
> + struct wmi_device *wdev;
> +};
> +
> +static int acer_wmi_battery_get_information(struct acer_wmi_battery_data *data,
> + u32 index, u32 battery, u32 *result)
> +{
> + u32 args[2] = { index, battery };
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct acpi_buffer input = { sizeof(args), args };
> + union acpi_object *obj;
> + int ret;
> +
> + ret = wmi_evaluate_method(ACER_BATTERY_GUID, 0, 19, &input, &output);
> + if (ACPI_FAILURE(ret))
> + return -EIO;
> +
> + obj = output.pointer;
> + if (!obj)
> + return -EIO;
> +
> + if (obj->type != ACPI_TYPE_BUFFER) {
> + ret = -EIO;
> + goto out_free_obj;
> + }
> +
> + if (obj->buffer.length != sizeof(u32)) {
> + dev_err(&data->wdev->dev, "WMI battery information call returned buffer of unexpected length %u\n",
> + obj->buffer.length);
> + ret = -EINVAL;
> + goto out_free_obj;
> + }
> +
> + *result = get_unaligned_le32(obj->buffer.pointer);
> +
> +out_free_obj:
> + kfree(obj);
Please use cleanup.h instead of goto + label. Remember to declare var at
the site, not in the variable declarations block.
> + return ret;
> +}
> +
> +static int acer_wmi_battery_get_health_control_status(struct acer_wmi_battery_data *data,
> + s8 *health_mode)
> +{
> + /*
> + * Acer Care Center seems to always call the WMI method
> + * with fixed parameters. This yields information about
> + * the availability and state of both health and
> + * calibration mode. The modes probably apply to
> + * all batteries of the system.
> + */
> + struct get_battery_health_control_status_input params = {
> + .uBatteryNo = ACER_BATTERY_INDEX,
> + .uFunctionQuery = 0x1,
> + .uReserved = { 0x0, 0x0 }
> + };
> + struct acpi_buffer input = {
> + sizeof(struct get_battery_health_control_status_input), ¶ms
> + };
> + struct get_battery_health_control_status_output status_output;
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + int ret;
> +
> + ret = wmi_evaluate_method(ACER_BATTERY_GUID, 0, 20, &input, &output);
> + if (ACPI_FAILURE(ret))
> + return -EIO;
> +
> + obj = output.pointer;
> + if (!obj)
> + return -EIO;
> + else if (obj->type != ACPI_TYPE_BUFFER) {
> + ret = -EIO;
> + goto out_free_obj;
> + }
> +
> + status_output = *((struct get_battery_health_control_status_output *)
> + obj->buffer.pointer);
> + if (obj->buffer.length != 8) {
> + dev_err(&data->wdev->dev, "WMI battery status call returned a buffer of unexpected length %d\n",
> + obj->buffer.length);
> + ret = -EINVAL;
> + goto out_free_obj;
> + }
> +
> + if (health_mode)
> + *health_mode = status_output.uFunctionList & HEALTH_MODE ?
> + status_output.uFunctionStatus[0] > 0 :
> + -1;
Please use braces for multiline blocks.
> +
> +out_free_obj:
> + kfree(obj);
cleanup.h, please change all of these.
> + return ret;
> +}
> +
> +static int set_battery_health_control(struct acer_wmi_battery_data *data,
Missing prefix.
> + u8 function, bool function_status)
> +{
> + struct set_battery_health_control_input params = {
> + .uBatteryNo = ACER_BATTERY_INDEX,
> + .uFunctionMask = function,
> + .uFunctionStatus = (u8)function_status,
Use ? : instead of relying C's bool -> u8 conversion.
> + .uReservedIn = { 0x0, 0x0, 0x0, 0x0, 0x0 }
> + };
> + struct acpi_buffer input = {
> + sizeof(struct set_battery_health_control_input),
> + ¶ms,
Previouslly you had these on the same line, it would be nice to use
consistent style.
> + };
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + int ret;
> +
> + ret = wmi_evaluate_method(ACER_BATTERY_GUID, 0, 21, &input, &output);
> + if (ACPI_FAILURE(ret))
> + return -EIO;
> +
> + obj = output.pointer;
> +
> + if (!obj)
> + return -EIO;
> +
> + if (obj->type != ACPI_TYPE_BUFFER) {
> + ret = -EIO;
> + goto out_free_obj;
> + }
> +
> + if (obj->buffer.length != 4) {
> + dev_err(&data->wdev->dev, "WMI battery status set operation returned a buffer of unexpected length %d\n",
> + obj->buffer.length);
> + ret = -EINVAL;
> + goto out_free_obj;
> + }
> +
> +out_free_obj:
> + kfree(obj);
> + return ret;
> +}
> +
> +static int acer_battery_ext_property_get(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *ext_data,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct acer_wmi_battery_data *data = ext_data;
> + s8 health_mode;
> + u32 value;
> + int ret;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_CHARGE_TYPES:
> + ret = acer_wmi_battery_get_health_control_status(data, &health_mode);
> + if (ret)
> + return ret;
> +
> + if (health_mode < 0)
> + return -EINVAL;
Why doesn't acer_wmi_battery_get_health_control_status() return -EINVAL
directlu but does this odd s8 trickery?
> +
> + val->intval = health_mode ? POWER_SUPPLY_CHARGE_TYPE_LONGLIFE :
> + POWER_SUPPLY_CHARGE_TYPE_STANDARD;
Please align this properly.
> + break;
> + case POWER_SUPPLY_PROP_TEMP:
> + ret = acer_wmi_battery_get_information(data, 0x8, ACER_BATTERY_INDEX, &value);
> + if (ret)
> + return ret;
> +
> + if (value > U16_MAX)
> + return -ERANGE;
> +
> + val->intval = value - 2731;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int acer_battery_ext_property_set(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *ext_data,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct acer_wmi_battery_data *data = ext_data;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_CHARGE_TYPES:
> + return set_battery_health_control(data, HEALTH_MODE,
> + val->intval == POWER_SUPPLY_CHARGE_TYPE_LONGLIFE);
There's quite big leap from POWER_SUPPLY_CHARGE_TYPE_LONGLIFE to something
called "function_status". Is this perhaps a (argument) naming issue?
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int acer_battery_ext_property_is_writeable(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *ext_data,
> + enum power_supply_property psp)
> +{
> + switch (psp) {
> + case POWER_SUPPLY_PROP_TEMP:
> + return false;
> + default:
> + return true;
> + }
> +}
> +
> +static const enum power_supply_property acer_battery_properties[] = {
> + POWER_SUPPLY_PROP_CHARGE_TYPES,
> + POWER_SUPPLY_PROP_TEMP,
> +};
> +
> +static const struct power_supply_ext acer_wmi_battery_extension = {
> + .name = "acer_laptop",
> + .properties = acer_battery_properties,
> + .num_properties = ARRAY_SIZE(acer_battery_properties),
> + .charge_types = (BIT(POWER_SUPPLY_CHARGE_TYPE_STANDARD) |
> + BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE)),
Aren't parenthesis unnecessary in this contruct?
> + .get_property = acer_battery_ext_property_get,
> + .set_property = acer_battery_ext_property_set,
> + .property_is_writeable = acer_battery_ext_property_is_writeable,
> +};
> +
> +static int acer_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> + struct acer_wmi_battery_data *data = container_of(hook, struct acer_wmi_battery_data, hook);
> +
> + return power_supply_register_extension(battery, &acer_wmi_battery_extension,
> + &data->wdev->dev, data);
> +}
> +
> +static int acer_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> + power_supply_unregister_extension(battery, &acer_wmi_battery_extension);
> +
> + return 0;
> +}
> +
> +static int acer_wmi_battery_battery_add(struct acer_wmi_battery_data *data)
> +{
> + data->hook.name = "Acer Battery Extension";
> + data->hook.add_battery = acer_battery_add;
> + data->hook.remove_battery = acer_battery_remove;
> +
> + return devm_battery_hook_register(&data->wdev->dev, &data->hook);
> +}
> +
> +static int acer_wmi_battery_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct acer_wmi_battery_data *data;
> +
> + data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&wdev->dev, data);
> + data->wdev = wdev;
> +
> + return acer_wmi_battery_battery_add(data);
> +}
> +
> +static const struct wmi_device_id acer_wmi_battery_id_table[] = {
> + { ACER_BATTERY_GUID, NULL },
> + { }
> +};
> +MODULE_DEVICE_TABLE(wmi, acer_wmi_battery_id_table);
> +
> +static struct wmi_driver acer_wmi_battery_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = acer_wmi_battery_id_table,
> + .probe = acer_wmi_battery_probe,
> +};
> +module_wmi_driver(acer_wmi_battery_driver);
> +
> +MODULE_AUTHOR("Frederik Harwath <frederik@harwath.name>");
> +MODULE_AUTHOR("Jelle van der Waa <jelle@vdwaa.nl>");
> +MODULE_DESCRIPTION("Acer battery health control WMI driver");
> +MODULE_LICENSE("GPL");
>
--
i.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/1] platform/x86: add Acer battery control driver
2026-01-05 17:10 ` [PATCH 1/1] platform/x86: " Jelle van der Waa
2026-01-06 9:34 ` Ilpo Järvinen
@ 2026-01-08 13:50 ` Armin Wolf
2026-01-23 19:48 ` Jelle van der Waa
1 sibling, 1 reply; 9+ messages in thread
From: Armin Wolf @ 2026-01-08 13:50 UTC (permalink / raw)
To: Jelle van der Waa, Hans de Goede, Ilpo Järvinen
Cc: platform-driver-x86, Frederik Harwath
Am 05.01.26 um 18:10 schrieb Jelle van der Waa:
> Some Acer laptops can configure battery related features through Acer
> Care Center on Windows. This driver uses the power supply extension to
> set a battery charge limit and exposes the battery
> temperature.
>
> This driver is based on the existing acer-wmi-battery project on GitHub
> and was tested on an Acer Aspire A315-510P.
>
> Signed-off-by: Jelle van der Waa <jelle@vdwaa.nl>
> ---
> drivers/platform/x86/Kconfig | 12 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/acer-wmi-battery.c | 345 ++++++++++++++++++++++++
> 3 files changed, 358 insertions(+)
> create mode 100644 drivers/platform/x86/acer-wmi-battery.c
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 4cb7d97a9fcc..b3de6a2827e8 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -170,6 +170,18 @@ config ACER_WMI
> If you have an ACPI-WMI compatible Acer/ Wistron laptop, say Y or M
> here.
>
> +config ACER_WMI_BATTERY
> + tristate "Acer WMI Battery"
> + depends on ACPI_WMI
> + depends on ACPI_BATTERY
> + depends on HWMON
Hi,
why do you need this HWMON dependency here? AFAIK you are not using any parts
of the hwmon API, so please remove this.
> + help
> + This is a driver for Acer laptops with battery health control. It
> + adds charge limit control and battery temperature reporting.
> +
> + If you have an ACPI-WMI Battery compatible Acer laptop, say Y or M
> + here.
> +
> source "drivers/platform/x86/amd/Kconfig"
>
> config ADV_SWBUTTON
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index d25762f7114f..9cf28baff3ae 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_GIGABYTE_WMI) += gigabyte-wmi.o
> obj-$(CONFIG_ACERHDF) += acerhdf.o
> obj-$(CONFIG_ACER_WIRELESS) += acer-wireless.o
> obj-$(CONFIG_ACER_WMI) += acer-wmi.o
> +obj-$(CONFIG_ACER_WMI_BATTERY) += acer-wmi-battery.o
>
> # AMD
> obj-y += amd/
> diff --git a/drivers/platform/x86/acer-wmi-battery.c b/drivers/platform/x86/acer-wmi-battery.c
> new file mode 100644
> index 000000000000..fffa521cde27
> --- /dev/null
> +++ b/drivers/platform/x86/acer-wmi-battery.c
> @@ -0,0 +1,345 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * acer-wmi-battery.c: Acer battery health control driver
> + *
> + * This is a driver for the WMI battery health control interface found
> + * on some Acer laptops. This interface allows to enable/disable a
> + * battery charge limit ("health mode") and exposes the battery temperature.
> + *
> + * Based on acer-wmi-battery https://github.com/frederik-h/acer-wmi-battery/
> + * Copyright (C) 2022-2025 Frederik Harwath <frederik@harwath.name>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/power_supply.h>
> +#include <linux/version.h>
> +#include <linux/wmi.h>
> +#include <linux/unaligned.h>
> +
> +#include <acpi/battery.h>
> +
> +#define DRIVER_NAME "acer-wmi-battery"
> +
> +#define ACER_BATTERY_GUID "79772EC5-04B1-4BFD-843C-61E7F77B6CC9"
> +
> +/*
> + * The Acer OEM software seems to always use this battery index,
> + * so we emulate this behaviour to not confuse the underlying firmware.
> + *
> + * However this also means that we only fully support devices with a
> + * single battery for now.
> + */
> +#define ACER_BATTERY_INDEX 0x1
> +
> +struct get_battery_health_control_status_input {
> + u8 uBatteryNo;
Please use underscores instead of camel case.
> + u8 uFunctionQuery;
> + u8 uReserved[2];
> +} __packed;
> +
> +struct get_battery_health_control_status_output {
> + u8 uFunctionList;
> + u8 uReturn[2];
> + u8 uFunctionStatus[5];
> +} __packed;
> +
> +struct set_battery_health_control_input {
> + u8 uBatteryNo;
> + u8 uFunctionMask;
> + u8 uFunctionStatus;
> + u8 uReservedIn[5];
> +} __packed;
> +
> +struct set_battery_health_control_output {
> + u8 uReturn;
> + u8 uReservedOut;
> +} __packed;
> +
> +enum battery_mode { HEALTH_MODE = 1, CALIBRATION_MODE = 2 };
> +
> +struct acer_wmi_battery_data {
> + struct acpi_battery_hook hook;
> + struct wmi_device *wdev;
> +};
> +
> +static int acer_wmi_battery_get_information(struct acer_wmi_battery_data *data,
> + u32 index, u32 battery, u32 *result)
> +{
> + u32 args[2] = { index, battery };
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct acpi_buffer input = { sizeof(args), args };
> + union acpi_object *obj;
> + int ret;
> +
> + ret = wmi_evaluate_method(ACER_BATTERY_GUID, 0, 19, &input, &output);
Please use wmidev_evaluate_method() instead of the decprecated GUID-based interface.
> + if (ACPI_FAILURE(ret))
> + return -EIO;
> +
> + obj = output.pointer;
> + if (!obj)
> + return -EIO;
> +
> + if (obj->type != ACPI_TYPE_BUFFER) {
> + ret = -EIO;
> + goto out_free_obj;
> + }
> +
> + if (obj->buffer.length != sizeof(u32)) {
Please also accept oversized ACPI buffers to emulate the behavior of the Windows WMI-ACPI driver.
> + dev_err(&data->wdev->dev, "WMI battery information call returned buffer of unexpected length %u\n",
> + obj->buffer.length);
> + ret = -EINVAL;
> + goto out_free_obj;
> + }
> +
> + *result = get_unaligned_le32(obj->buffer.pointer);
> +
> +out_free_obj:
> + kfree(obj);
> + return ret;
> +}
> +
> +static int acer_wmi_battery_get_health_control_status(struct acer_wmi_battery_data *data,
> + s8 *health_mode)
Please run checkpatch --strict over this source file and fix any issues.
> +{
> + /*
> + * Acer Care Center seems to always call the WMI method
> + * with fixed parameters. This yields information about
> + * the availability and state of both health and
> + * calibration mode. The modes probably apply to
> + * all batteries of the system.
> + */
> + struct get_battery_health_control_status_input params = {
> + .uBatteryNo = ACER_BATTERY_INDEX,
> + .uFunctionQuery = 0x1,
Maybe use a define for that?
> + .uReserved = { 0x0, 0x0 }
> + };
> + struct acpi_buffer input = {
> + sizeof(struct get_battery_health_control_status_input), ¶ms
Please use sizeof(params).
> + };
> + struct get_battery_health_control_status_output status_output;
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + int ret;
> +
> + ret = wmi_evaluate_method(ACER_BATTERY_GUID, 0, 20, &input, &output);
Please use wmidev_evaluate_method().
> + if (ACPI_FAILURE(ret))
> + return -EIO;
> +
> + obj = output.pointer;
> + if (!obj)
> + return -EIO;
> + else if (obj->type != ACPI_TYPE_BUFFER) {
The "else" is pointless here, please remove.
> + ret = -EIO;
> + goto out_free_obj;
> + }
> +
> + status_output = *((struct get_battery_health_control_status_output *)
> + obj->buffer.pointer);
> + if (obj->buffer.length != 8) {
Please do the size check first before assigning status_output (keep the previous comment regarding oversized buffers in mind).
Also please turn status_output into a pointer so that you can avoid a copy operation.
> + dev_err(&data->wdev->dev, "WMI battery status call returned a buffer of unexpected length %d\n",
> + obj->buffer.length);
> + ret = -EINVAL;
> + goto out_free_obj;
> + }
> +
> + if (health_mode)
> + *health_mode = status_output.uFunctionList & HEALTH_MODE ?
> + status_output.uFunctionStatus[0] > 0 :
> + -1;
> +
> +out_free_obj:
> + kfree(obj);
> + return ret;
> +}
> +
> +static int set_battery_health_control(struct acer_wmi_battery_data *data,
> + u8 function, bool function_status)
> +{
> + struct set_battery_health_control_input params = {
> + .uBatteryNo = ACER_BATTERY_INDEX,
> + .uFunctionMask = function,
> + .uFunctionStatus = (u8)function_status,
> + .uReservedIn = { 0x0, 0x0, 0x0, 0x0, 0x0 }
> + };
> + struct acpi_buffer input = {
> + sizeof(struct set_battery_health_control_input),
> + ¶ms,
> + };
> + struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + int ret;
> +
> + ret = wmi_evaluate_method(ACER_BATTERY_GUID, 0, 21, &input, &output);
Please use wmidev_evaluate_method().
> + if (ACPI_FAILURE(ret))
> + return -EIO;
> +
> + obj = output.pointer;
> +
> + if (!obj)
> + return -EIO;
> +
> + if (obj->type != ACPI_TYPE_BUFFER) {
> + ret = -EIO;
> + goto out_free_obj;
> + }
> +
> + if (obj->buffer.length != 4) {
Please also accept oversized buffers. By the way, what is the usage of the return value?
> + dev_err(&data->wdev->dev, "WMI battery status set operation returned a buffer of unexpected length %d\n",
> + obj->buffer.length);
> + ret = -EINVAL;
> + goto out_free_obj;
> + }
> +
> +out_free_obj:
> + kfree(obj);
> + return ret;
> +}
> +
> +static int acer_battery_ext_property_get(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *ext_data,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct acer_wmi_battery_data *data = ext_data;
> + s8 health_mode;
> + u32 value;
> + int ret;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_CHARGE_TYPES:
> + ret = acer_wmi_battery_get_health_control_status(data, &health_mode);
> + if (ret)
> + return ret;
> +
> + if (health_mode < 0)
> + return -EINVAL;
> +
> + val->intval = health_mode ? POWER_SUPPLY_CHARGE_TYPE_LONGLIFE :
> + POWER_SUPPLY_CHARGE_TYPE_STANDARD;
> + break;
> + case POWER_SUPPLY_PROP_TEMP:
> + ret = acer_wmi_battery_get_information(data, 0x8, ACER_BATTERY_INDEX, &value);
> + if (ret)
> + return ret;
> +
> + if (value > U16_MAX)
> + return -ERANGE;
> +
> + val->intval = value - 2731;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int acer_battery_ext_property_set(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *ext_data,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct acer_wmi_battery_data *data = ext_data;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_CHARGE_TYPES:
> + return set_battery_health_control(data, HEALTH_MODE,
> + val->intval == POWER_SUPPLY_CHARGE_TYPE_LONGLIFE);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int acer_battery_ext_property_is_writeable(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *ext_data,
> + enum power_supply_property psp)
> +{
> + switch (psp) {
> + case POWER_SUPPLY_PROP_TEMP:
> + return false;
Nitpick: Only return true for charge_types and return false for everything else.
> + default:
> + return true;
> + }
> +}
> +
> +static const enum power_supply_property acer_battery_properties[] = {
> + POWER_SUPPLY_PROP_CHARGE_TYPES,
I own a Acer notebook that only supports the temperature sensor, not the health mode feature.
You can detect this by looking at the embedded WMI binary MOF data, but we currently have no
in-kernel parser for this.
I am working on adding support for the binary MOF data, but for now i suggest that you use a
DMI whitelist before enabling access to the health mode feature.
> + POWER_SUPPLY_PROP_TEMP,
> +};
> +
> +static const struct power_supply_ext acer_wmi_battery_extension = {
> + .name = "acer_laptop",
Could you please use "acer_wmi_battery" (or DRIVER_NAME) instead? This would prevent any future name conflicts
with other drivers for Acer notebooks.
> + .properties = acer_battery_properties,
> + .num_properties = ARRAY_SIZE(acer_battery_properties),
> + .charge_types = (BIT(POWER_SUPPLY_CHARGE_TYPE_STANDARD) |
> + BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE)),
> + .get_property = acer_battery_ext_property_get,
> + .set_property = acer_battery_ext_property_set,
> + .property_is_writeable = acer_battery_ext_property_is_writeable,
> +};
> +
> +static int acer_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> + struct acer_wmi_battery_data *data = container_of(hook, struct acer_wmi_battery_data, hook);
> +
> + return power_supply_register_extension(battery, &acer_wmi_battery_extension,
> + &data->wdev->dev, data);
> +}
> +
> +static int acer_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
> +{
> + power_supply_unregister_extension(battery, &acer_wmi_battery_extension);
> +
> + return 0;
> +}
> +
> +static int acer_wmi_battery_battery_add(struct acer_wmi_battery_data *data)
> +{
> + data->hook.name = "Acer Battery Extension";
> + data->hook.add_battery = acer_battery_add;
> + data->hook.remove_battery = acer_battery_remove;
> +
> + return devm_battery_hook_register(&data->wdev->dev, &data->hook);
> +}
> +
> +static int acer_wmi_battery_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct acer_wmi_battery_data *data;
> +
> + data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&wdev->dev, data);
> + data->wdev = wdev;
> +
> + return acer_wmi_battery_battery_add(data);
Nitpick: Please fold this function into the caller.
> +}
> +
> +static const struct wmi_device_id acer_wmi_battery_id_table[] = {
> + { ACER_BATTERY_GUID, NULL },
> + { }
> +};
> +MODULE_DEVICE_TABLE(wmi, acer_wmi_battery_id_table);
> +
> +static struct wmi_driver acer_wmi_battery_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = acer_wmi_battery_id_table,
> + .probe = acer_wmi_battery_probe,
Please also set .no_singleton to signal that this driver can be instantiated multiple times.
Thanks,
Armin Wolf
> +};
> +module_wmi_driver(acer_wmi_battery_driver);
> +
> +MODULE_AUTHOR("Frederik Harwath <frederik@harwath.name>");
> +MODULE_AUTHOR("Jelle van der Waa <jelle@vdwaa.nl>");
> +MODULE_DESCRIPTION("Acer battery health control WMI driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 9+ messages in thread