Re: [PATCH v2 1/1] platform/x86: add Acer battery control driver

[Armin Wolf <W_Armin@gmx.de>]

Am 10.05.26 um 20:48 schrieb Jelle van der Waa:
> On 1/31/26 00:24, Armin Wolf wrote:
>> Am 25.01.26 um 19:23 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>
>>>
>>> ---
>>> v2:
>>> - Alphabetically sort linux headers
>>> - Include headers for types / _packed
>>> - Use cleanup.h instead of goto + label
>>> - Add missing prefix for set_battery_health_control
>>> - General code formatting fixes
>>> - Remove HWMON dependency in Kconfig
>>> - Use wmidev_evaluate_method()
>>> - Accept oversized ACPI buffers
>>> - Use DRIVER_NAME for battery extension name
>>> - Set no_singleton = true
>>> - Implement DMI matching to support laptops with only battery
>>>    temperature support.
>>> ---
>>>   drivers/platform/x86/Kconfig            |  11 +
>>>   drivers/platform/x86/Makefile           |   1 +
>>>   drivers/platform/x86/acer-wmi-battery.c | 355 ++++++++++++++++++++++++
>>>   3 files changed, 367 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..88c11a698fb9 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -170,6 +170,17 @@ 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
>>
>> Please also depend on DMI so that dmi_check_system() will always be 
>> available
>> when building this driver.
>>
>>> +    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..abb45cfcc6fe
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/acer-wmi-battery.c
>>> @@ -0,0 +1,355 @@
>>> +// 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/acpi.h>
>>> +#include <linux/cleanup.h>
>>> +#include <linux/compiler_attributes.h>
>>> +#include <linux/dmi.h>
>>> +#include <linux/init.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/limits.h>
>>> +#include <linux/module.h>
>>> +#include <linux/power_supply.h>
>>> +#include <linux/types.h>
>>> +#include <linux/unaligned.h>
>>> +#include <linux/version.h>
>>> +#include <linux/wmi.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;
>>> +    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;
>>> +    const struct power_supply_ext *battery_ext;
>>> +    struct {
>>> +        bool health_mode : 1;
>>> +    } features;
>>> +};
>>> +
>>> +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 };
>>> +    int ret;
>>> +
>>> +    ret = wmidev_evaluate_method(data->wdev, 0, 19, &input, &output);
>>> +    if (ACPI_FAILURE(ret))
>>> +        return -EIO;
>>> +
>>> +    union acpi_object *obj __free(kfree) = output.pointer;
>>> +    if (!obj)
>>> +        return -EIO;
>>> +
>>> +    if (obj->type != ACPI_TYPE_BUFFER)
>>> +        ret = -EIO;
>>> +
>>> +    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;
>>> +    }
>>> +
>>> +    *result = get_unaligned_le32(obj->buffer.pointer);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int acer_wmi_battery_get_health_control_status(struct 
>>> acer_wmi_battery_data *data,
>>> +                              bool *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 args = {
>>> +        .uBatteryNo = ACER_BATTERY_INDEX,
>>> +        .uFunctionQuery = 0x1,
>>> +        .uReserved = { 0x0, 0x0 }
>>> +    };
>>> +    struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
>>> +    struct get_battery_health_control_status_output *status_output;
>>> +    struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>>> +    int ret;
>>> +
>>> +    ret = wmidev_evaluate_method(data->wdev, 0, 20, &input, &output);
>>> +    if (ACPI_FAILURE(ret))
>>> +        return -EIO;
>>> +
>>> +    union acpi_object *obj __free(kfree) = output.pointer;
>>> +    if (!obj)
>>> +        return -EIO;
>>> +
>>> +    if (obj->type != ACPI_TYPE_BUFFER)
>>> +        ret = -EIO;
>>> +
>>> +    if (obj->buffer.length < 8) {
>>
>> Better use sizeof(*status_output) here.
>>
>>> +        dev_err(&data->wdev->dev, "WMI battery status call returned 
>>> a buffer of unexpected length %d\n",
>>> +            obj->buffer.length);
>>> +        ret = -EINVAL;
>>> +    }
>>> +
>>> +    status_output = (struct get_battery_health_control_status_output 
>>> *)obj->buffer.pointer;
>>> +
>>> +    if (health_mode) {
>>> +        if (status_output->uFunctionList & HEALTH_MODE)
>>> +            *health_mode = status_output->uFunctionStatus[0] > 0;
>>> +        else
>>> +            ret = -EINVAL;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int acer_wmi_battery_set_battery_health_control(struct 
>>> acer_wmi_battery_data *data,
>>> +                               u8 function, bool function_status)
>>> +{
>>> +    struct set_battery_health_control_input args = {
>>> +        .uBatteryNo = ACER_BATTERY_INDEX,
>>> +        .uFunctionMask = function,
>>> +        .uFunctionStatus = function_status ? 1 : 0,
>>> +        .uReservedIn = { 0x0, 0x0, 0x0, 0x0, 0x0 }
>>> +    };
>>> +    struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
>>> +    struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>>> +    union acpi_object *obj;
>>> +    int ret;
>>> +
>>> +    ret = wmidev_evaluate_method(data->wdev, 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;
>>> +
>>> +    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;
>>> +    }
>>> +
>>> +    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;
>>> +    bool 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;
>>> +
>>> +        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 acer_wmi_battery_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_CHARGE_TYPES:
>>> +        return true;
>>> +    default:
>>> +        return false;
>>> +    }
>>> +}
>>> +
>>> +static const struct dmi_system_id 
>>> acer_wmi_battery_health_mode_table[] = {
>>> +    {
>>> +        .matches = {
>>> +            DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
>>> +            DMI_MATCH(DMI_PRODUCT_NAME, "Aspire A315-510P")
>>> +        }
>>> +    },
>>> +    {}
>>> +};
>>
>> I suggest that you perform the DMI check during module initialization. 
>> This way
>> the DMI table can be marked as __initconst and does not consume any 
>> memory after
>> anymore after module initialization is complete.
>>
>> The result of the DMI check could then be stored inside a global 
>> variable.
>>
>> Other than that, the driver seems fine to me.
> 
> Thanks for the review comments, due to some personal circumstances it 
> has taken me while to spin up v3.
> 
> I've been digging into the WMI dump data looking if I could figure out 
> if there was a way to check if health checks are available but only 
> stumbled on another battery related WMI method.
> 
> Method 0x16: GetBatteryFunctionData
> - Input: uFunctionMask (uint8), uReservedIn (uint8 array)
> - Output: uReturnCode (uint8 array), uBACStartTime (uint8 array), 
> uBACStopTime (uint8 array), uBACStatus (uint8), uReservedOut (uint8 array)
> Method 0x17: SetBatteryFunctionData
> - Input: uFunctionMask (uint8), uBACSwitch (uint8), uReservedIn (uint8 
> array)
> 
> But I was wondering why in probe or init, the driver couldn't call 
> "acer_wmi_battery_get_health_control_status". Then we wouldn't need to 
> add a WMI whitelist for every device that supports this.
> 
> If I recall correctly you own an Acer laptop which supports the battery 
> temperature but lacks health control? Seeing how many WMI methods are 
> exposed on my device I suppose it is also exposed for you, but it should 
> hopefully error out?

Calling an non-existing WMI method is undefined behavior and could in 
the worst cast cause the ACPI firmware to crash. Because of this we have 
to use a DMI whitelist for now :(

I am working on a in-kernel BMOF parser, but i am currently occupied by 
other work.

Thanks,
Armin Wolf

> 
> Thanks,
> 
> Jelle van der Waa