Re: [PATCH 6/6] platform/x86: uniwill-laptop: Add support for battery charge modes
From: Werner Sembach
Date: Fri Apr 10 2026 - 13:06:41 EST
Hi Armin,
Am 06.04.26 um 21:36 schrieb Armin Wolf:
Many Uniwill-based devices do not supports the already existingMaybe add a "!= 0" to make the return a clear 1 or 0
charge limit functionality, but instead support an alternative
interface for controlling the battery charge algorithm.
Add support for this interface and update the documentation.
Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
---
.../admin-guide/laptops/uniwill-laptop.rst | 25 +-
drivers/platform/x86/uniwill/uniwill-acpi.c | 244 ++++++++++++++----
drivers/platform/x86/uniwill/uniwill-wmi.c | 5 +-
3 files changed, 226 insertions(+), 48 deletions(-)
diff --git a/Documentation/admin-guide/laptops/uniwill-laptop.rst b/Documentation/admin-guide/laptops/uniwill-laptop.rst
index 561334865feb..24b41dbab886 100644
--- a/Documentation/admin-guide/laptops/uniwill-laptop.rst
+++ b/Documentation/admin-guide/laptops/uniwill-laptop.rst
@@ -43,9 +43,23 @@ Support for changing the platform performance mode is currently not implemented.
Battery Charging Control
------------------------
-The ``uniwill-laptop`` driver supports controlling the battery charge limit. This happens over
-the standard ``charge_control_end_threshold`` power supply sysfs attribute. All values
-between 1 and 100 percent are supported.
+.. warning:: Some devices do not properly implement the charging threshold interface. Forcing
+ the driver to enable access to said interface on such devices might damage the
+ battery [1]_. Because of this the driver will not enable said feature even when
+ using the ``force`` module parameter. The charging profile interface will be
+ available instead.
+
+The ``uniwill-laptop`` driver supports controlling the battery charge limit. This either happens
+over the standard ``charge_control_end_threshold`` or ``charge_types`` power supply sysfs attribute,
+depending on the device. When using the ``charge_control_end_threshold`` sysfs attribute, all values
+between 1 and 100 percent are supported. When using the ``charge_types`` sysfs attribute, the driver
+supports switching between the ``Standard``, ``Trickle`` and ``Long Life`` profiles.
+
+Keep in mind that when using the ``charge_types`` sysfs attribute, the EC firmware will hide the
+true charging status of the battery from the operating system, potentially misleading users into
+thinking that the charging profile does not work. Checking the ``current_now`` sysfs attribute
+tells you the true charging status of the battery even when using the ``charge_types`` sysfs
+attribute (0 means that the battery is currently not charging).
Additionally the driver signals the presence of battery charging issues through the standard
``health`` power supply sysfs attribute.
@@ -70,3 +84,8 @@ The ``uniwill-laptop`` driver allows to set the configurable TGP for devices wit
allow it.
See Documentation/ABI/testing/sysfs-driver-uniwill-laptop for details.
+
+References
+==========
+
+.. [1] https://www.reddit.com/r/XMG_gg/comments/ld9yyf/battery_limit_hidden_function_discovered_on/
diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
index a116e2ec6453..fbf2e101a074 100644
--- a/drivers/platform/x86/uniwill/uniwill-acpi.c
+++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
@@ -254,6 +254,10 @@
#define EC_ADDR_OEM_4 0x07A6
#define OVERBOOST_DYN_TEMP_OFF BIT(1)
+#define CHARGING_PROFILE_MASK GENMASK(5, 4)
+#define CHARGING_PROFILE_HIGH_CAPACITY 0x00
+#define CHARGING_PROFILE_BALANCED 0x01
+#define CHARGING_PROFILE_STATIONARY 0x02
#define TOUCHPAD_TOGGLE_OFF BIT(6)
#define EC_ADDR_CHARGE_CTRL 0x07B9
@@ -320,13 +324,15 @@
#define UNIWILL_FEATURE_SUPER_KEY BIT(1)
#define UNIWILL_FEATURE_TOUCHPAD_TOGGLE BIT(2)
#define UNIWILL_FEATURE_LIGHTBAR BIT(3)
-#define UNIWILL_FEATURE_BATTERY BIT(4)
-#define UNIWILL_FEATURE_CPU_TEMP BIT(5)
-#define UNIWILL_FEATURE_GPU_TEMP BIT(6)
-#define UNIWILL_FEATURE_PRIMARY_FAN BIT(7)
-#define UNIWILL_FEATURE_SECONDARY_FAN BIT(8)
-#define UNIWILL_FEATURE_NVIDIA_CTGP_CONTROL BIT(9)
-#define UNIWILL_FEATURE_USB_C_POWER_PRIORITY BIT(10)
+#define UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT BIT(4)
+/* Mutually exclusive with the charge limit feature */
+#define UNIWILL_FEATURE_BATTERY_CHARGE_MODES BIT(5)
+#define UNIWILL_FEATURE_CPU_TEMP BIT(6)
+#define UNIWILL_FEATURE_GPU_TEMP BIT(7)
+#define UNIWILL_FEATURE_PRIMARY_FAN BIT(8)
+#define UNIWILL_FEATURE_SECONDARY_FAN BIT(9)
+#define UNIWILL_FEATURE_NVIDIA_CTGP_CONTROL BIT(10)
+#define UNIWILL_FEATURE_USB_C_POWER_PRIORITY BIT(11)
enum usb_c_power_priority_options {
USB_C_POWER_PRIORITY_CHARGING = 0,
@@ -339,8 +345,15 @@ struct uniwill_data {
struct regmap *regmap;
unsigned int features;
struct acpi_battery_hook hook;
- unsigned int last_charge_ctrl;
struct mutex battery_lock; /* Protects the list of currently registered batteries */
+ union {
+ struct {
+ /* Protects writes to last_charge_type */
+ struct mutex charge_type_lock;
+ enum power_supply_charge_type last_charge_type;
+ };
+ unsigned int last_charge_ctrl;
+ };
bool last_fn_lock_state;
bool last_super_key_enable_state;
bool last_touchpad_toggle_enable_state;
@@ -447,6 +460,12 @@ static inline bool uniwill_device_supports(const struct uniwill_data *data,
return (data->features & features) == features;
}
+static inline bool uniwill_device_supports_any(const struct uniwill_data *data,
+ unsigned int features)
+{
+ return data->features & features;
+}
+
static int uniwill_ec_reg_write(void *context, unsigned int reg, unsigned int val)Like I wrote here: https://github.com/Wer-Wolf/uniwill-laptop/issues/6#issuecomment-3987100594 I think it would be less confusing to use
{
union acpi_object params[2] = {
@@ -1421,6 +1440,30 @@ static int uniwill_led_init(struct uniwill_data *data)
&init_data);
}
+static int uniwill_read_charge_type(struct uniwill_data *data, enum power_supply_charge_type *type)
+{
+ unsigned int value;
+ int ret;
+
+ ret = regmap_read(data->regmap, EC_ADDR_OEM_4, &value);
+ if (ret < 0)
+ return ret;
+
+ switch (FIELD_GET(CHARGING_PROFILE_MASK, value)) {
+ case CHARGING_PROFILE_HIGH_CAPACITY:
+ *type = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
+ return 0;
+ case CHARGING_PROFILE_BALANCED:
+ *type = POWER_SUPPLY_CHARGE_TYPE_LONGLIFE;
+ return 0;
+ case CHARGING_PROFILE_STATIONARY:
+ *type = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+ return 0;
+ default:
+ return -EPROTO;
+ }
+}
POWER_SUPPLY_CHARGE_TYPE_TRICKLE -> CHARGING_PROFILE_STATIONARY
POWER_SUPPLY_CHARGE_TYPE_STANDARD -> CHARGING_PROFILE_BALANCED
POWER_SUPPLY_CHARGE_TYPE_FAST -> CHARGING_PROFILE_HIGH_CAPACITY
because then the charge speed is at least clearly communicated
POWER_SUPPLY_CHARGE_TYPE_FAST would then be the default
+Could be mistaken, but does this work without producing a compiler warning or error not having a return on the top level?
static int uniwill_get_property(struct power_supply *psy, const struct power_supply_ext *ext,
void *drvdata, enum power_supply_property psp,
union power_supply_propval *val)
@@ -1431,6 +1474,16 @@ static int uniwill_get_property(struct power_supply *psy, const struct power_sup
int ret;
switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_TYPES:
+ /*
+ * We need to use the cached value here because the charging mode
+ * reported by the EC might temporarily change when a external power
+ * source has been connected.
+ */
+ mutex_lock(&data->charge_type_lock);
+ val->intval = data->last_charge_type;
+ mutex_unlock(&data->charge_type_lock);
+ return 0;
case POWER_SUPPLY_PROP_HEALTH:
ret = power_supply_get_property_direct(psy, POWER_SUPPLY_PROP_PRESENT, &prop);
if (ret < 0)
@@ -1479,13 +1532,52 @@ static int uniwill_get_property(struct power_supply *psy, const struct power_sup
}
}
+static int uniwill_write_charge_type(struct uniwill_data *data, enum power_supply_charge_type type)
+{
+ unsigned int value;
+
+ switch (type) {
+ case POWER_SUPPLY_CHARGE_TYPE_TRICKLE:
+ value = FIELD_PREP(CHARGING_PROFILE_MASK, CHARGING_PROFILE_STATIONARY);
+ break;
+ case POWER_SUPPLY_CHARGE_TYPE_STANDARD:
+ value = FIELD_PREP(CHARGING_PROFILE_MASK, CHARGING_PROFILE_HIGH_CAPACITY);
+ break;
+ case POWER_SUPPLY_CHARGE_TYPE_LONGLIFE:
+ value = FIELD_PREP(CHARGING_PROFILE_MASK, CHARGING_PROFILE_BALANCED);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return regmap_update_bits(data->regmap, EC_ADDR_OEM_4, CHARGING_PROFILE_MASK, value);
+}
+
+static int uniwill_restore_charge_type(struct uniwill_data *data)
+{
+ guard(mutex)(&data->charge_type_lock);
+
+ return uniwill_write_charge_type(data, data->last_charge_type);
+}
+
static int uniwill_set_property(struct power_supply *psy, const struct power_supply_ext *ext,
void *drvdata, enum power_supply_property psp,
const union power_supply_propval *val)
{
struct uniwill_data *data = drvdata;
+ int ret;
switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_TYPES:
+ mutex_lock(&data->charge_type_lock);
+
+ ret = uniwill_write_charge_type(data, val->intval);
+ if (ret >= 0)
+ data->last_charge_type = val->intval;
+
+ mutex_unlock(&data->charge_type_lock);
+
+ return ret;
case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
if (val->intval < 0 || val->intval > 100)
return -EINVAL;
@@ -1501,21 +1593,41 @@ static int uniwill_property_is_writeable(struct power_supply *psy,
const struct power_supply_ext *ext, void *drvdata,
enum power_supply_property psp)
{
- if (psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD)
+ switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_TYPES:
+ case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
return true;
-
- return false;
+ default:
+ return false;
+ }
}
-static const enum power_supply_property uniwill_properties[] = {Currently there is now way to specify just the health parameter, but i have to check if that is even required
+static const enum power_supply_property uniwill_charge_limit_properties[] = {
POWER_SUPPLY_PROP_HEALTH,
POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
};
-static const struct power_supply_ext uniwill_extension = {
+static const struct power_supply_ext uniwill_charge_limit_extension = {
+ .name = DRIVER_NAME,
+ .properties = uniwill_charge_limit_properties,
+ .num_properties = ARRAY_SIZE(uniwill_charge_limit_properties),
+ .get_property = uniwill_get_property,
+ .set_property = uniwill_set_property,
+ .property_is_writeable = uniwill_property_is_writeable,
+};
+
+static const enum power_supply_property uniwill_charge_modes_properties[] = {
+ POWER_SUPPLY_PROP_CHARGE_TYPES,
+ POWER_SUPPLY_PROP_HEALTH,
+};
+
+static const struct power_supply_ext uniwill_charge_modes_extension = {
.name = DRIVER_NAME,
- .properties = uniwill_properties,
- .num_properties = ARRAY_SIZE(uniwill_properties),
+ .charge_types = BIT(POWER_SUPPLY_CHARGE_TYPE_TRICKLE) |
+ BIT(POWER_SUPPLY_CHARGE_TYPE_STANDARD) |
+ BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE),
+ .properties = uniwill_charge_modes_properties,
+ .num_properties = ARRAY_SIZE(uniwill_charge_modes_properties),
.get_property = uniwill_get_property,
.set_property = uniwill_set_property,
.property_is_writeable = uniwill_property_is_writeable,
@@ -1531,7 +1643,13 @@ static int uniwill_add_battery(struct power_supply *battery, struct acpi_battery
if (!entry)
return -ENOMEM;
- ret = power_supply_register_extension(battery, &uniwill_extension, data->dev, data);
+ if (uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT))
+ ret = power_supply_register_extension(battery, &uniwill_charge_limit_extension,
+ data->dev, data);
+ else
+ ret = power_supply_register_extension(battery, &uniwill_charge_modes_extension,
+ data->dev, data);
+
if (ret < 0) {Would use an "else if (uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_MODES))" and put the return 0 at the end of the function.
kfree(entry);
return ret;
@@ -1560,7 +1678,10 @@ static int uniwill_remove_battery(struct power_supply *battery, struct acpi_batt
}
}
- power_supply_unregister_extension(battery, &uniwill_extension);
+ if (uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT))
+ power_supply_unregister_extension(battery, &uniwill_charge_limit_extension);
+ else
+ power_supply_unregister_extension(battery, &uniwill_charge_modes_extension);
return 0;
}
@@ -1570,25 +1691,35 @@ static int uniwill_battery_init(struct uniwill_data *data)
unsigned int value, threshold;
int ret;
- if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
- return 0;
+ if (uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT)) {
+ ret = regmap_read(data->regmap, EC_ADDR_CHARGE_CTRL, &value);
+ if (ret < 0)
+ return ret;
- ret = regmap_read(data->regmap, EC_ADDR_CHARGE_CTRL, &value);
- if (ret < 0)
- return ret;
+ /*
+ * The charge control threshold might be initialized with 0 by
+ * the EC to signal that said threshold is uninitialized. We thus
+ * need to replace this value with 100 to signal that we want to
+ * take control of battery charging. For the sake of completeness
+ * we also set the charging threshold to 100 if the EC-provided
+ * value is invalid.
+ */
+ threshold = FIELD_GET(CHARGE_CTRL_MASK, value);
+ if (threshold == 0 || threshold > 100) {
+ FIELD_MODIFY(CHARGE_CTRL_MASK, &value, 100);
+ ret = regmap_write(data->regmap, EC_ADDR_CHARGE_CTRL, value);
+ if (ret < 0)
+ return ret;
+ }
+ } else {
+ if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_MODES))
+ return 0;
- /*Maybe we should put in some assertion to ensure that UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT and UNIWILL_FEATURE_BATTERY_CHARGE_MODES are not set at the same time, or it that being to wary?
- * The charge control threshold might be initialized with 0 by
- * the EC to signal that said threshold is uninitialized. We thus
- * need to replace this value with 100 to signal that we want to
- * take control of battery charging. For the sake of completeness
- * we also set the charging threshold to 100 if the EC-provided
- * value is invalid.
- */
- threshold = FIELD_GET(CHARGE_CTRL_MASK, value);
- if (threshold == 0 || threshold > 100) {
- FIELD_MODIFY(CHARGE_CTRL_MASK, &value, 100);
- ret = regmap_write(data->regmap, EC_ADDR_CHARGE_CTRL, value);
+ ret = devm_mutex_init(data->dev, &data->charge_type_lock);
+ if (ret < 0)
+ return ret;
+
+ ret = uniwill_read_charge_type(data, &data->last_charge_type);
if (ret < 0)
return ret;
}
@@ -1609,10 +1740,13 @@ static int uniwill_notifier_call(struct notifier_block *nb, unsigned long action
{
struct uniwill_data *data = container_of(nb, struct uniwill_data, nb);
struct uniwill_battery_entry *entry;
+ int ret;
switch (action) {
case UNIWILL_OSD_BATTERY_ALERT:
- if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
+ if (!uniwill_device_supports_any(data,
+ UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT |
+ UNIWILL_FEATURE_BATTERY_CHARGE_MODES))
return NOTIFY_DONE;
mutex_lock(&data->battery_lock);
@@ -1623,10 +1757,24 @@ static int uniwill_notifier_call(struct notifier_block *nb, unsigned long action
return NOTIFY_OK;
case UNIWILL_OSD_DC_ADAPTER_CHANGED:
- if (!uniwill_device_supports(data, UNIWILL_FEATURE_USB_C_POWER_PRIORITY))
+ if (!uniwill_device_supports_any(data,
+ UNIWILL_FEATURE_BATTERY_CHARGE_MODES |
+ UNIWILL_FEATURE_USB_C_POWER_PRIORITY))
return NOTIFY_DONE;
- return notifier_from_errno(usb_c_power_priority_restore(data));
+ if (uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_MODES)) {
+ ret = uniwill_restore_charge_type(data);
+ if (ret < 0)
+ return notifier_from_errno(ret);
+ }
+
+ if (uniwill_device_supports(data, UNIWILL_FEATURE_USB_C_POWER_PRIORITY)) {
+ ret = usb_c_power_priority_restore(data);
+ if (ret < 0)
+ return notifier_from_errno(ret);
+ }
+
+ return NOTIFY_OK;
case UNIWILL_OSD_FN_LOCK:
if (!uniwill_device_supports(data, UNIWILL_FEATURE_FN_LOCK))
return NOTIFY_DONE;
@@ -1810,7 +1958,7 @@ static int uniwill_suspend_touchpad_toggle(struct uniwill_data *data)
static int uniwill_suspend_battery(struct uniwill_data *data)
{
- if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
+ if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT))
return 0;
/*
@@ -1887,7 +2035,15 @@ static int uniwill_resume_touchpad_toggle(struct uniwill_data *data)
static int uniwill_resume_battery(struct uniwill_data *data)
{
- if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
+ int ret;
+
+ if (uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_MODES)) {
+ ret = uniwill_restore_charge_type(data);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT))
return 0;
return regmap_update_bits(data->regmap, EC_ADDR_CHARGE_CTRL, CHARGE_CTRL_MASK,
@@ -1970,7 +2126,7 @@ static struct platform_driver uniwill_driver = {
static struct uniwill_device_descriptor lapqc71a_lapqc71b_descriptor __initdata = {
.features = UNIWILL_FEATURE_SUPER_KEY |
- UNIWILL_FEATURE_BATTERY |
+ UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT |
UNIWILL_FEATURE_CPU_TEMP |
UNIWILL_FEATURE_GPU_TEMP |
UNIWILL_FEATURE_PRIMARY_FAN |
@@ -1981,7 +2137,7 @@ static struct uniwill_device_descriptor lapac71h_descriptor __initdata = {
.features = UNIWILL_FEATURE_FN_LOCK |
UNIWILL_FEATURE_SUPER_KEY |
UNIWILL_FEATURE_TOUCHPAD_TOGGLE |
- UNIWILL_FEATURE_BATTERY |
+ UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT |
UNIWILL_FEATURE_CPU_TEMP |
UNIWILL_FEATURE_GPU_TEMP |
UNIWILL_FEATURE_PRIMARY_FAN |
@@ -1993,7 +2149,7 @@ static struct uniwill_device_descriptor lapkc71f_descriptor __initdata = {
UNIWILL_FEATURE_SUPER_KEY |
UNIWILL_FEATURE_TOUCHPAD_TOGGLE |
UNIWILL_FEATURE_LIGHTBAR |
- UNIWILL_FEATURE_BATTERY |
+ UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT |
UNIWILL_FEATURE_CPU_TEMP |
UNIWILL_FEATURE_GPU_TEMP |
UNIWILL_FEATURE_PRIMARY_FAN |
@@ -2578,8 +2734,8 @@ static int __init uniwill_init(void)Is this something specific for this patch or just general cleanup that sneaked into here?
}
if (force) {
- /* Assume that the device supports all features */
- device_descriptor.features = UINT_MAX;
+ /* Assume that the device supports all features except the charge limit */
+ device_descriptor.features = UINT_MAX & ~UNIWILL_FEATURE_BATTERY_CHARGE_LIMIT;
pr_warn("Enabling potentially unsupported features\n");
}
diff --git a/drivers/platform/x86/uniwill/uniwill-wmi.c b/drivers/platform/x86/uniwill/uniwill-wmi.c
index 31d9c39f14ab..f1b89bc63df6 100644
--- a/drivers/platform/x86/uniwill/uniwill-wmi.c
+++ b/drivers/platform/x86/uniwill/uniwill-wmi.c
@@ -48,6 +48,7 @@ int devm_uniwill_wmi_register_notifier(struct device *dev, struct notifier_block
static void uniwill_wmi_notify(struct wmi_device *wdev, union acpi_object *obj)
{
u32 value;
+ int ret;
if (obj->type != ACPI_TYPE_INTEGER)
return;
@@ -56,7 +57,9 @@ static void uniwill_wmi_notify(struct wmi_device *wdev, union acpi_object *obj)
dev_dbg(&wdev->dev, "Received WMI event %u\n", value);
- blocking_notifier_call_chain(&uniwill_wmi_chain_head, value, NULL);
+ ret = blocking_notifier_call_chain(&uniwill_wmi_chain_head, value, NULL);
+ if (notifier_to_errno(ret) < 0)
+ dev_err(&wdev->dev, "Failed to handle event %u\n", value);
}
/*
Best regards,
Werner