[PATCH 0/6] platform/x86: uniwill-laptop: Charging-related improvements

[PATCH 0/6] platform/x86: uniwill-laptop: Charging-related improvements

[Armin Wolf]

This patch series contains various charging-related improvements
for the uniwill-laptop driver. The first two patches fix some minor
issues inside the charging threshold code, while the third patch
fixes an issue inside the module initialization code. The next two
patches contain some preparations for the last patch. Said patch
introduces support for controlling the charging modes available
on many Uniwill-based devices not sold by Intel.

All patches have been tested on a Tuxedo InfinityBook Pro 15 Gen10 AMD
and work just fine.

Armin Wolf (6):
  platform/x86: uniwill-laptop: Properly initialize charging threshold
  platform/x86: uniwill-laptop: Accept charging threshold of 0
  platform/x86: uniwill-laptop: Fix behavior of "force" module param
  platform/x86: uniwill-laptop: Rework FN lock/super key suspend
    handling
  platform/x86: uniwill-laptop: Mark EC_ADDR_OEM_4 as volatile
  platform/x86: uniwill-laptop: Add support for battery charge modes

 .../admin-guide/laptops/uniwill-laptop.rst    |  25 +-
 drivers/platform/x86/uniwill/uniwill-acpi.c   | 431 ++++++++++++++----
 drivers/platform/x86/uniwill/uniwill-wmi.c    |   5 +-
 3 files changed, 374 insertions(+), 87 deletions(-)

-- 
2.39.5

[PATCH 1/6] platform/x86: uniwill-laptop: Properly initialize charging threshold

[Armin Wolf]

The EC might initialize the charge threshold with 0 to signal that
said threshold is uninitialized. Detect this and replace said value
with 100 to signal the EC that we want to take control of battery
charging. Also set the threshold to 100 if the EC-provided value
is invalid.

Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/uniwill/uniwill-acpi.c | 28 ++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
index faade4cf08be..8f16c94221aa 100644
--- a/drivers/platform/x86/uniwill/uniwill-acpi.c
+++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
@@ -1404,7 +1404,12 @@ static int uniwill_get_property(struct power_supply *psy, const struct power_sup
 		if (ret < 0)
 			return ret;
 
-		val->intval = clamp_val(FIELD_GET(CHARGE_CTRL_MASK, regval), 0, 100);
+		regval = FIELD_GET(CHARGE_CTRL_MASK, regval);
+		if (!regval)
+			val->intval = 100;
+		else
+			val->intval = min(regval, 100);
+
 		return 0;
 	default:
 		return -EINVAL;
@@ -1499,11 +1504,32 @@ static int uniwill_remove_battery(struct power_supply *battery, struct acpi_batt
 
 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;
 
+	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;
+	}
+
 	ret = devm_mutex_init(data->dev, &data->battery_lock);
 	if (ret < 0)
 		return ret;
-- 
2.39.5

Re: [PATCH 1/6] platform/x86: uniwill-laptop: Properly initialize charging threshold

[Werner Sembach]

Am 06.04.26 um 21:36 schrieb Armin Wolf:
> The EC might initialize the charge threshold with 0 to signal that
> said threshold is uninitialized. Detect this and replace said value
> with 100 to signal the EC that we want to take control of battery
> charging. Also set the threshold to 100 if the EC-provided value
> is invalid.
>
> Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>   drivers/platform/x86/uniwill/uniwill-acpi.c | 28 ++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
> index faade4cf08be..8f16c94221aa 100644
> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> @@ -1404,7 +1404,12 @@ static int uniwill_get_property(struct power_supply *psy, const struct power_sup
>   		if (ret < 0)
>   			return ret;
>   
> -		val->intval = clamp_val(FIELD_GET(CHARGE_CTRL_MASK, regval), 0, 100);
> +		regval = FIELD_GET(CHARGE_CTRL_MASK, regval);
> +		if (!regval)
personal preference: explicitly compair to the uninitialized-value "0" with == 
for better readability, but don't know if that is the normal coding style for 
the kernel
> +			val->intval = 100;
> +		else
> +			val->intval = min(regval, 100);
> +
We silently correct invalid values here. Wouldn't it be better to detect that 
something is off?
>   		return 0;
>   	default:
>   		return -EINVAL;
> @@ -1499,11 +1504,32 @@ static int uniwill_remove_battery(struct power_supply *battery, struct acpi_batt
>   
>   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;
>   
> +	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;
> +	}
> +
>   	ret = devm_mutex_init(data->dev, &data->battery_lock);
>   	if (ret < 0)
>   		return ret;

Re: [PATCH 1/6] platform/x86: uniwill-laptop: Properly initialize charging threshold

[Armin Wolf]

Am 09.04.26 um 00:59 schrieb Werner Sembach:

>
> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>> The EC might initialize the charge threshold with 0 to signal that
>> said threshold is uninitialized. Detect this and replace said value
>> with 100 to signal the EC that we want to take control of battery
>> charging. Also set the threshold to 100 if the EC-provided value
>> is invalid.
>>
>> Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/uniwill/uniwill-acpi.c | 28 ++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
>> b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> index faade4cf08be..8f16c94221aa 100644
>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> @@ -1404,7 +1404,12 @@ static int uniwill_get_property(struct 
>> power_supply *psy, const struct power_sup
>>           if (ret < 0)
>>               return ret;
>>   -        val->intval = clamp_val(FIELD_GET(CHARGE_CTRL_MASK, 
>> regval), 0, 100);
>> +        regval = FIELD_GET(CHARGE_CTRL_MASK, regval);
>> +        if (!regval)
> personal preference: explicitly compair to the uninitialized-value "0" 
> with == for better readability, but don't know if that is the normal 
> coding style for the kernel

Hi,

AFAIK the default kernel coding style encourages people to write NULL/0 checks as !value. However i am not sure
if this only applies to NULL checks. Ilpo, what is your preference in this case?

>> +            val->intval = 100;
>> +        else
>> +            val->intval = min(regval, 100);
>> +
> We silently correct invalid values here. Wouldn't it be better to 
> detect that something is off?

The ACPI code on my device suggests that values equal to or larger than 100 will be interpreted as
"charging limit is disabled", so i think we should replicate this assumption inside the driver.

Thanks,
Armin Wolf

>>           return 0;
>>       default:
>>           return -EINVAL;
>> @@ -1499,11 +1504,32 @@ static int uniwill_remove_battery(struct 
>> power_supply *battery, struct acpi_batt
>>     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;
>>   +    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;
>> +    }
>> +
>>       ret = devm_mutex_init(data->dev, &data->battery_lock);
>>       if (ret < 0)
>>           return ret;

Re: [PATCH 1/6] platform/x86: uniwill-laptop: Properly initialize charging threshold

[Werner Sembach]

Am 10.04.26 um 19:27 schrieb Armin Wolf:
> Am 09.04.26 um 00:59 schrieb Werner Sembach:
>
>>
>> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>>> The EC might initialize the charge threshold with 0 to signal that
>>> said threshold is uninitialized. Detect this and replace said value
>>> with 100 to signal the EC that we want to take control of battery
>>> charging. Also set the threshold to 100 if the EC-provided value
>>> is invalid.
>>>
>>> Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>>   drivers/platform/x86/uniwill/uniwill-acpi.c | 28 ++++++++++++++++++++-
>>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
>>> b/drivers/platform/x86/uniwill/uniwill-acpi.c
>>> index faade4cf08be..8f16c94221aa 100644
>>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>>> @@ -1404,7 +1404,12 @@ static int uniwill_get_property(struct power_supply 
>>> *psy, const struct power_sup
>>>           if (ret < 0)
>>>               return ret;
>>>   -        val->intval = clamp_val(FIELD_GET(CHARGE_CTRL_MASK, regval), 0, 
>>> 100);
>>> +        regval = FIELD_GET(CHARGE_CTRL_MASK, regval);
>>> +        if (!regval)
>> personal preference: explicitly compair to the uninitialized-value "0" with 
>> == for better readability, but don't know if that is the normal coding style 
>> for the kernel
>
> Hi,
>
> AFAIK the default kernel coding style encourages people to write NULL/0 checks 
> as !value. However i am not sure
> if this only applies to NULL checks. Ilpo, what is your preference in this case?
ok
>
>>> +            val->intval = 100;
>>> +        else
>>> +            val->intval = min(regval, 100);
>>> +
>> We silently correct invalid values here. Wouldn't it be better to detect that 
>> something is off?
>
> The ACPI code on my device suggests that values equal to or larger than 100 
> will be interpreted as
> "charging limit is disabled", so i think we should replicate this assumption 
> inside the driver.

ok

then:

Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>

>
> Thanks,
> Armin Wolf
>
>>>           return 0;
>>>       default:
>>>           return -EINVAL;
>>> @@ -1499,11 +1504,32 @@ static int uniwill_remove_battery(struct 
>>> power_supply *battery, struct acpi_batt
>>>     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;
>>>   +    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;
>>> +    }
>>> +
>>>       ret = devm_mutex_init(data->dev, &data->battery_lock);
>>>       if (ret < 0)
>>>           return ret;

[PATCH 2/6] platform/x86: uniwill-laptop: Accept charging threshold of 0

[Armin Wolf]

The power supply sysfs ABI states that:

	Not all hardware is capable of setting this to an arbitrary
	percentage. Drivers will round written values to the nearest
	supported value. Reading back the value will show the actual
	threshold set by the driver.

The driver currently violates this ABI by rejecting a charging
threshold of 0. Fix this by clamping this value to 1.

Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/uniwill/uniwill-acpi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
index 8f16c94221aa..5551f193c2c1 100644
--- a/drivers/platform/x86/uniwill/uniwill-acpi.c
+++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
@@ -1424,11 +1424,11 @@ static int uniwill_set_property(struct power_supply *psy, const struct power_sup
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
-		if (val->intval < 1 || val->intval > 100)
+		if (val->intval < 0 || val->intval > 100)
 			return -EINVAL;
 
 		return regmap_update_bits(data->regmap, EC_ADDR_CHARGE_CTRL, CHARGE_CTRL_MASK,
-					  val->intval);
+					  max(val->intval, 1));
 	default:
 		return -EINVAL;
 	}
-- 
2.39.5

Re: [PATCH 2/6] platform/x86: uniwill-laptop: Accept charging threshold of 0

[Werner Sembach]

Am 06.04.26 um 21:36 schrieb Armin Wolf:
> The power supply sysfs ABI states that:
>
> 	Not all hardware is capable of setting this to an arbitrary
> 	percentage. Drivers will round written values to the nearest
> 	supported value. Reading back the value will show the actual
> 	threshold set by the driver.
>
> The driver currently violates this ABI by rejecting a charging
> threshold of 0. Fix this by clamping this value to 1.
>
> Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>   drivers/platform/x86/uniwill/uniwill-acpi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
> index 8f16c94221aa..5551f193c2c1 100644
> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> @@ -1424,11 +1424,11 @@ static int uniwill_set_property(struct power_supply *psy, const struct power_sup
>   
>   	switch (psp) {
>   	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
> -		if (val->intval < 1 || val->intval > 100)
> +		if (val->intval < 0 || val->intval > 100)
>   			return -EINVAL;
>   
>   		return regmap_update_bits(data->regmap, EC_ADDR_CHARGE_CTRL, CHARGE_CTRL_MASK,
> -					  val->intval);
> +					  max(val->intval, 1));

looks good

Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>

>   	default:
>   		return -EINVAL;
>   	}

[PATCH 3/6] platform/x86: uniwill-laptop: Fix behavior of "force" module param

[Armin Wolf]

Users might want to force-enable all possible features even on
machines with a valid device descriptor. Until now the "force"
module param was ignored on such machines. Fix this to make
it easier to test for support of new features.

Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/uniwill/uniwill-acpi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
index 5551f193c2c1..7c7ea0e76966 100644
--- a/drivers/platform/x86/uniwill/uniwill-acpi.c
+++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
@@ -2481,8 +2481,6 @@ static int __init uniwill_init(void)
 		if (!force)
 			return -ENODEV;
 
-		/* Assume that the device supports all features */
-		device_descriptor.features = UINT_MAX;
 		pr_warn("Loading on a potentially unsupported device\n");
 	} else {
 		/*
@@ -2500,6 +2498,12 @@ static int __init uniwill_init(void)
 		device_descriptor = *descriptor;
 	}
 
+	if (force) {
+		/* Assume that the device supports all features */
+		device_descriptor.features = UINT_MAX;
+		pr_warn("Enabling potentially unsupported features\n");
+	}
+
 	ret = platform_driver_register(&uniwill_driver);
 	if (ret < 0)
 		return ret;
-- 
2.39.5

Re: [PATCH 3/6] platform/x86: uniwill-laptop: Fix behavior of "force" module param

[Werner Sembach]

Am 06.04.26 um 21:36 schrieb Armin Wolf:
> Users might want to force-enable all possible features even on
> machines with a valid device descriptor. Until now the "force"
> module param was ignored on such machines. Fix this to make
> it easier to test for support of new features.
>
> Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>   drivers/platform/x86/uniwill/uniwill-acpi.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
> index 5551f193c2c1..7c7ea0e76966 100644
> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> @@ -2481,8 +2481,6 @@ static int __init uniwill_init(void)
>   		if (!force)
>   			return -ENODEV;
>   
> -		/* Assume that the device supports all features */
> -		device_descriptor.features = UINT_MAX;
>   		pr_warn("Loading on a potentially unsupported device\n");
>   	} else {
>   		/*
> @@ -2500,6 +2498,12 @@ static int __init uniwill_init(void)
>   		device_descriptor = *descriptor;
>   	}
>   
> +	if (force) {
> +		/* Assume that the device supports all features */
> +		device_descriptor.features = UINT_MAX;
> +		pr_warn("Enabling potentially unsupported features\n");
> +	}
> +
this should already exclude the potentially harmfull "charging threshold"
>   	ret = platform_driver_register(&uniwill_driver);
>   	if (ret < 0)
>   		return ret;

Re: [PATCH 3/6] platform/x86: uniwill-laptop: Fix behavior of "force" module param

[Armin Wolf]

Am 09.04.26 um 01:08 schrieb Werner Sembach:

>
> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>> Users might want to force-enable all possible features even on
>> machines with a valid device descriptor. Until now the "force"
>> module param was ignored on such machines. Fix this to make
>> it easier to test for support of new features.
>>
>> Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/uniwill/uniwill-acpi.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
>> b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> index 5551f193c2c1..7c7ea0e76966 100644
>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> @@ -2481,8 +2481,6 @@ static int __init uniwill_init(void)
>>           if (!force)
>>               return -ENODEV;
>>   -        /* Assume that the device supports all features */
>> -        device_descriptor.features = UINT_MAX;
>>           pr_warn("Loading on a potentially unsupported device\n");
>>       } else {
>>           /*
>> @@ -2500,6 +2498,12 @@ static int __init uniwill_init(void)
>>           device_descriptor = *descriptor;
>>       }
>>   +    if (force) {
>> +        /* Assume that the device supports all features */
>> +        device_descriptor.features = UINT_MAX;
>> +        pr_warn("Enabling potentially unsupported features\n");
>> +    }
>> +
> this should already exclude the potentially harmfull "charging threshold"

I decided to keep this patch small so that it can be picked up by the stable team
without much fuss. Personally i believe that the charging limit should be dealt with
inside the last patch where we add support for the (safe) charging modes.

Thanks,
Armin Wolf

>>       ret = platform_driver_register(&uniwill_driver);
>>       if (ret < 0)
>>           return ret;

Re: [PATCH 3/6] platform/x86: uniwill-laptop: Fix behavior of "force" module param

[Werner Sembach]

Am 10.04.26 um 19:32 schrieb Armin Wolf:
> Am 09.04.26 um 01:08 schrieb Werner Sembach:
>
>>
>> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>>> Users might want to force-enable all possible features even on
>>> machines with a valid device descriptor. Until now the "force"
>>> module param was ignored on such machines. Fix this to make
>>> it easier to test for support of new features.
>>>
>>> Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>>   drivers/platform/x86/uniwill/uniwill-acpi.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
>>> b/drivers/platform/x86/uniwill/uniwill-acpi.c
>>> index 5551f193c2c1..7c7ea0e76966 100644
>>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>>> @@ -2481,8 +2481,6 @@ static int __init uniwill_init(void)
>>>           if (!force)
>>>               return -ENODEV;
>>>   -        /* Assume that the device supports all features */
>>> -        device_descriptor.features = UINT_MAX;
>>>           pr_warn("Loading on a potentially unsupported device\n");
>>>       } else {
>>>           /*
>>> @@ -2500,6 +2498,12 @@ static int __init uniwill_init(void)
>>>           device_descriptor = *descriptor;
>>>       }
>>>   +    if (force) {
>>> +        /* Assume that the device supports all features */
>>> +        device_descriptor.features = UINT_MAX;
>>> +        pr_warn("Enabling potentially unsupported features\n");
>>> +    }
>>> +
>> this should already exclude the potentially harmfull "charging threshold"
>
> I decided to keep this patch small so that it can be picked up by the stable team
> without much fuss. Personally i believe that the charging limit should be 
> dealt with
> inside the last patch where we add support for the (safe) charging modes.

Stable should get this protection too, and it is just one more line.

Could even be an own patch with just that line

>
> Thanks,
> Armin Wolf
>
>>>       ret = platform_driver_register(&uniwill_driver);
>>>       if (ret < 0)
>>>           return ret;

Re: [PATCH 3/6] platform/x86: uniwill-laptop: Fix behavior of "force" module param

[Armin Wolf]

Am 10.04.26 um 22:30 schrieb Werner Sembach:

>
> Am 10.04.26 um 19:32 schrieb Armin Wolf:
>> Am 09.04.26 um 01:08 schrieb Werner Sembach:
>>
>>>
>>> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>>>> Users might want to force-enable all possible features even on
>>>> machines with a valid device descriptor. Until now the "force"
>>>> module param was ignored on such machines. Fix this to make
>>>> it easier to test for support of new features.
>>>>
>>>> Fixes: d050479693bb ("platform/x86: Add Uniwill laptop driver")
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> ---
>>>>   drivers/platform/x86/uniwill/uniwill-acpi.c | 8 ++++++--
>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
>>>> b/drivers/platform/x86/uniwill/uniwill-acpi.c
>>>> index 5551f193c2c1..7c7ea0e76966 100644
>>>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>>>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>>>> @@ -2481,8 +2481,6 @@ static int __init uniwill_init(void)
>>>>           if (!force)
>>>>               return -ENODEV;
>>>>   -        /* Assume that the device supports all features */
>>>> -        device_descriptor.features = UINT_MAX;
>>>>           pr_warn("Loading on a potentially unsupported device\n");
>>>>       } else {
>>>>           /*
>>>> @@ -2500,6 +2498,12 @@ static int __init uniwill_init(void)
>>>>           device_descriptor = *descriptor;
>>>>       }
>>>>   +    if (force) {
>>>> +        /* Assume that the device supports all features */
>>>> +        device_descriptor.features = UINT_MAX;
>>>> +        pr_warn("Enabling potentially unsupported features\n");
>>>> +    }
>>>> +
>>> this should already exclude the potentially harmfull "charging 
>>> threshold"
>>
>> I decided to keep this patch small so that it can be picked up by the 
>> stable team
>> without much fuss. Personally i believe that the charging limit 
>> should be dealt with
>> inside the last patch where we add support for the (safe) charging 
>> modes.
>
> Stable should get this protection too, and it is just one more line.
>
> Could even be an own patch with just that line
>
Agree, i will do this in the v2 series.

Thanks,
Armin Wolf

>>
>> Thanks,
>> Armin Wolf
>>
>>>>       ret = platform_driver_register(&uniwill_driver);
>>>>       if (ret < 0)
>>>>           return ret;
>

[PATCH 4/6] platform/x86: uniwill-laptop: Rework FN lock/super key suspend handling

[Armin Wolf]

Currently the supend handling for the FN lock and super key enable
features saves the whole values of the affected registers instead of
the individual feature state. This duplicates the register access
logic from the associated sysfs attributes.

Rework the suspend handling to reuse said register access logic and
only store the individual feature state as a boolean value.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/uniwill/uniwill-acpi.c | 117 ++++++++++++--------
 1 file changed, 73 insertions(+), 44 deletions(-)

diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
index 7c7ea0e76966..4d149b39d94e 100644
--- a/drivers/platform/x86/uniwill/uniwill-acpi.c
+++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
@@ -341,8 +341,8 @@ struct uniwill_data {
 	struct acpi_battery_hook hook;
 	unsigned int last_charge_ctrl;
 	struct mutex battery_lock;	/* Protects the list of currently registered batteries */
-	unsigned int last_status;
-	unsigned int last_switch_status;
+	bool last_fn_lock_state;
+	bool last_super_key_enable_state;
 	struct mutex super_key_lock;	/* Protects the toggling of the super key lock state */
 	struct list_head batteries;
 	struct mutex led_lock;		/* Protects writes to the lightbar registers */
@@ -619,11 +619,22 @@ static const struct regmap_config uniwill_ec_config = {
 	.use_single_write = true,
 };
 
+static int uniwill_write_fn_lock(struct uniwill_data *data, bool status)
+{
+	unsigned int value;
+
+	if (status)
+		value = FN_LOCK_STATUS;
+	else
+		value = 0;
+
+	return regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, FN_LOCK_STATUS, value);
+}
+
 static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr, const char *buf,
 			     size_t count)
 {
 	struct uniwill_data *data = dev_get_drvdata(dev);
-	unsigned int value;
 	bool enable;
 	int ret;
 
@@ -631,21 +642,15 @@ static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr,
 	if (ret < 0)
 		return ret;
 
-	if (enable)
-		value = FN_LOCK_STATUS;
-	else
-		value = 0;
-
-	ret = regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, FN_LOCK_STATUS, value);
+	ret = uniwill_write_fn_lock(data, enable);
 	if (ret < 0)
 		return ret;
 
 	return count;
 }
 
-static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf)
+static int uniwill_read_fn_lock(struct uniwill_data *data, bool *status)
 {
-	struct uniwill_data *data = dev_get_drvdata(dev);
 	unsigned int value;
 	int ret;
 
@@ -653,23 +658,31 @@ static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, c
 	if (ret < 0)
 		return ret;
 
-	return sysfs_emit(buf, "%d\n", !!(value & FN_LOCK_STATUS));
-}
+	*status = !!(value & FN_LOCK_STATUS);
 
-static DEVICE_ATTR_RW(fn_lock);
+	return 0;
+}
 
-static ssize_t super_key_enable_store(struct device *dev, struct device_attribute *attr,
-				      const char *buf, size_t count)
+static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct uniwill_data *data = dev_get_drvdata(dev);
-	unsigned int value;
-	bool enable;
+	bool status;
 	int ret;
 
-	ret = kstrtobool(buf, &enable);
+	ret = uniwill_read_fn_lock(data, &status);
 	if (ret < 0)
 		return ret;
 
+	return sysfs_emit(buf, "%d\n", status);
+}
+
+static DEVICE_ATTR_RW(fn_lock);
+
+static int uniwill_write_super_key_enable(struct uniwill_data *data, bool status)
+{
+	unsigned int value;
+	int ret;
+
 	guard(mutex)(&data->super_key_lock);
 
 	ret = regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &value);
@@ -680,20 +693,33 @@ static ssize_t super_key_enable_store(struct device *dev, struct device_attribut
 	 * We can only toggle the super key lock, so we return early if the setting
 	 * is already in the correct state.
 	 */
-	if (enable == !(value & SUPER_KEY_LOCK_STATUS))
-		return count;
+	if (status == !(value & SUPER_KEY_LOCK_STATUS))
+		return 0;
+
+	return regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, TRIGGER_SUPER_KEY_LOCK,
+				 TRIGGER_SUPER_KEY_LOCK);
+}
+
+static ssize_t super_key_enable_store(struct device *dev, struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct uniwill_data *data = dev_get_drvdata(dev);
+	bool enable;
+	int ret;
 
-	ret = regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, TRIGGER_SUPER_KEY_LOCK,
-				TRIGGER_SUPER_KEY_LOCK);
+	ret = kstrtobool(buf, &enable);
+	if (ret < 0)
+		return ret;
+
+	ret = uniwill_write_super_key_enable(data, enable);
 	if (ret < 0)
 		return ret;
 
 	return count;
 }
 
-static ssize_t super_key_enable_show(struct device *dev, struct device_attribute *attr, char *buf)
+static int uniwill_read_super_key_enable(struct uniwill_data *data, bool *status)
 {
-	struct uniwill_data *data = dev_get_drvdata(dev);
 	unsigned int value;
 	int ret;
 
@@ -701,7 +727,22 @@ static ssize_t super_key_enable_show(struct device *dev, struct device_attribute
 	if (ret < 0)
 		return ret;
 
-	return sysfs_emit(buf, "%d\n", !(value & SUPER_KEY_LOCK_STATUS));
+	*status = !(value & SUPER_KEY_LOCK_STATUS);
+
+	return 0;
+}
+
+static ssize_t super_key_enable_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct uniwill_data *data = dev_get_drvdata(dev);
+	bool status;
+	int ret;
+
+	ret = uniwill_read_super_key_enable(data, &status);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", status);
 }
 
 static DEVICE_ATTR_RW(super_key_enable);
@@ -1715,10 +1756,10 @@ static int uniwill_suspend_fn_lock(struct uniwill_data *data)
 		return 0;
 
 	/*
-	 * The EC_ADDR_BIOS_OEM is marked as volatile, so we have to restore it
+	 * EC_ADDR_BIOS_OEM is marked as volatile, so we have to restore it
 	 * ourselves.
 	 */
-	return regmap_read(data->regmap, EC_ADDR_BIOS_OEM, &data->last_status);
+	return uniwill_read_fn_lock(data, &data->last_fn_lock_state);
 }
 
 static int uniwill_suspend_super_key(struct uniwill_data *data)
@@ -1727,10 +1768,10 @@ static int uniwill_suspend_super_key(struct uniwill_data *data)
 		return 0;
 
 	/*
-	 * The EC_ADDR_SWITCH_STATUS is marked as volatile, so we have to restore it
+	 * EC_ADDR_SWITCH_STATUS is marked as volatile, so we have to restore it
 	 * ourselves.
 	 */
-	return regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &data->last_switch_status);
+	return uniwill_read_super_key_enable(data, &data->last_super_key_enable_state);
 }
 
 static int uniwill_suspend_battery(struct uniwill_data *data)
@@ -1787,27 +1828,15 @@ static int uniwill_resume_fn_lock(struct uniwill_data *data)
 	if (!uniwill_device_supports(data, UNIWILL_FEATURE_FN_LOCK))
 		return 0;
 
-	return regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, FN_LOCK_STATUS,
-				  data->last_status);
+	return uniwill_write_fn_lock(data, data->last_fn_lock_state);
 }
 
 static int uniwill_resume_super_key(struct uniwill_data *data)
 {
-	unsigned int value;
-	int ret;
-
 	if (!uniwill_device_supports(data, UNIWILL_FEATURE_SUPER_KEY))
 		return 0;
 
-	ret = regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &value);
-	if (ret < 0)
-		return ret;
-
-	if ((data->last_switch_status & SUPER_KEY_LOCK_STATUS) == (value & SUPER_KEY_LOCK_STATUS))
-		return 0;
-
-	return regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, TRIGGER_SUPER_KEY_LOCK,
-				 TRIGGER_SUPER_KEY_LOCK);
+	return uniwill_write_super_key_enable(data, data->last_super_key_enable_state);
 }
 
 static int uniwill_resume_battery(struct uniwill_data *data)
-- 
2.39.5

Re: [PATCH 4/6] platform/x86: uniwill-laptop: Rework FN lock/super key suspend handling

[Werner Sembach]

Am 06.04.26 um 21:36 schrieb Armin Wolf:
> Currently the supend handling for the FN lock and super key enable

*suspend

Other then this typo lgtm:

Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>

> features saves the whole values of the affected registers instead of
> the individual feature state. This duplicates the register access
> logic from the associated sysfs attributes.
>
> Rework the suspend handling to reuse said register access logic and
> only store the individual feature state as a boolean value.
>
> Signed-off-by: Armin Wolf<W_Armin@gmx.de>
> ---
>   drivers/platform/x86/uniwill/uniwill-acpi.c | 117 ++++++++++++--------
>   1 file changed, 73 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
> index 7c7ea0e76966..4d149b39d94e 100644
> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> @@ -341,8 +341,8 @@ struct uniwill_data {
>   	struct acpi_battery_hook hook;
>   	unsigned int last_charge_ctrl;
>   	struct mutex battery_lock;	/* Protects the list of currently registered batteries */
> -	unsigned int last_status;
> -	unsigned int last_switch_status;
> +	bool last_fn_lock_state;
> +	bool last_super_key_enable_state;
>   	struct mutex super_key_lock;	/* Protects the toggling of the super key lock state */
>   	struct list_head batteries;
>   	struct mutex led_lock;		/* Protects writes to the lightbar registers */
> @@ -619,11 +619,22 @@ static const struct regmap_config uniwill_ec_config = {
>   	.use_single_write = true,
>   };
>   
> +static int uniwill_write_fn_lock(struct uniwill_data *data, bool status)
> +{
> +	unsigned int value;
> +
> +	if (status)
> +		value = FN_LOCK_STATUS;
> +	else
> +		value = 0;
> +
> +	return regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, FN_LOCK_STATUS, value);
> +}
> +
>   static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr, const char *buf,
>   			     size_t count)
>   {
>   	struct uniwill_data *data = dev_get_drvdata(dev);
> -	unsigned int value;
>   	bool enable;
>   	int ret;
>   
> @@ -631,21 +642,15 @@ static ssize_t fn_lock_store(struct device *dev, struct device_attribute *attr,
>   	if (ret < 0)
>   		return ret;
>   
> -	if (enable)
> -		value = FN_LOCK_STATUS;
> -	else
> -		value = 0;
> -
> -	ret = regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, FN_LOCK_STATUS, value);
> +	ret = uniwill_write_fn_lock(data, enable);
>   	if (ret < 0)
>   		return ret;
>   
>   	return count;
>   }
>   
> -static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf)
> +static int uniwill_read_fn_lock(struct uniwill_data *data, bool *status)
>   {
> -	struct uniwill_data *data = dev_get_drvdata(dev);
>   	unsigned int value;
>   	int ret;
>   
> @@ -653,23 +658,31 @@ static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, c
>   	if (ret < 0)
>   		return ret;
>   
> -	return sysfs_emit(buf, "%d\n", !!(value & FN_LOCK_STATUS));
> -}
> +	*status = !!(value & FN_LOCK_STATUS);
>   
> -static DEVICE_ATTR_RW(fn_lock);
> +	return 0;
> +}
>   
> -static ssize_t super_key_enable_store(struct device *dev, struct device_attribute *attr,
> -				      const char *buf, size_t count)
> +static ssize_t fn_lock_show(struct device *dev, struct device_attribute *attr, char *buf)
>   {
>   	struct uniwill_data *data = dev_get_drvdata(dev);
> -	unsigned int value;
> -	bool enable;
> +	bool status;
>   	int ret;
>   
> -	ret = kstrtobool(buf, &enable);
> +	ret = uniwill_read_fn_lock(data, &status);
>   	if (ret < 0)
>   		return ret;
>   
> +	return sysfs_emit(buf, "%d\n", status);
> +}
> +
> +static DEVICE_ATTR_RW(fn_lock);
> +
> +static int uniwill_write_super_key_enable(struct uniwill_data *data, bool status)
> +{
> +	unsigned int value;
> +	int ret;
> +
>   	guard(mutex)(&data->super_key_lock);
>   
>   	ret = regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &value);
> @@ -680,20 +693,33 @@ static ssize_t super_key_enable_store(struct device *dev, struct device_attribut
>   	 * We can only toggle the super key lock, so we return early if the setting
>   	 * is already in the correct state.
>   	 */
> -	if (enable == !(value & SUPER_KEY_LOCK_STATUS))
> -		return count;
> +	if (status == !(value & SUPER_KEY_LOCK_STATUS))
> +		return 0;
> +
> +	return regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, TRIGGER_SUPER_KEY_LOCK,
> +				 TRIGGER_SUPER_KEY_LOCK);
> +}
> +
> +static ssize_t super_key_enable_store(struct device *dev, struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct uniwill_data *data = dev_get_drvdata(dev);
> +	bool enable;
> +	int ret;
>   
> -	ret = regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, TRIGGER_SUPER_KEY_LOCK,
> -				TRIGGER_SUPER_KEY_LOCK);
> +	ret = kstrtobool(buf, &enable);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = uniwill_write_super_key_enable(data, enable);
>   	if (ret < 0)
>   		return ret;
>   
>   	return count;
>   }
>   
> -static ssize_t super_key_enable_show(struct device *dev, struct device_attribute *attr, char *buf)
> +static int uniwill_read_super_key_enable(struct uniwill_data *data, bool *status)
>   {
> -	struct uniwill_data *data = dev_get_drvdata(dev);
>   	unsigned int value;
>   	int ret;
>   
> @@ -701,7 +727,22 @@ static ssize_t super_key_enable_show(struct device *dev, struct device_attribute
>   	if (ret < 0)
>   		return ret;
>   
> -	return sysfs_emit(buf, "%d\n", !(value & SUPER_KEY_LOCK_STATUS));
> +	*status = !(value & SUPER_KEY_LOCK_STATUS);
> +
> +	return 0;
> +}
> +
> +static ssize_t super_key_enable_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct uniwill_data *data = dev_get_drvdata(dev);
> +	bool status;
> +	int ret;
> +
> +	ret = uniwill_read_super_key_enable(data, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", status);
>   }
>   
>   static DEVICE_ATTR_RW(super_key_enable);
> @@ -1715,10 +1756,10 @@ static int uniwill_suspend_fn_lock(struct uniwill_data *data)
>   		return 0;
>   
>   	/*
> -	 * The EC_ADDR_BIOS_OEM is marked as volatile, so we have to restore it
> +	 * EC_ADDR_BIOS_OEM is marked as volatile, so we have to restore it
>   	 * ourselves.
>   	 */
> -	return regmap_read(data->regmap, EC_ADDR_BIOS_OEM, &data->last_status);
> +	return uniwill_read_fn_lock(data, &data->last_fn_lock_state);
>   }
>   
>   static int uniwill_suspend_super_key(struct uniwill_data *data)
> @@ -1727,10 +1768,10 @@ static int uniwill_suspend_super_key(struct uniwill_data *data)
>   		return 0;
>   
>   	/*
> -	 * The EC_ADDR_SWITCH_STATUS is marked as volatile, so we have to restore it
> +	 * EC_ADDR_SWITCH_STATUS is marked as volatile, so we have to restore it
>   	 * ourselves.
>   	 */
> -	return regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &data->last_switch_status);
> +	return uniwill_read_super_key_enable(data, &data->last_super_key_enable_state);
>   }
>   
>   static int uniwill_suspend_battery(struct uniwill_data *data)
> @@ -1787,27 +1828,15 @@ static int uniwill_resume_fn_lock(struct uniwill_data *data)
>   	if (!uniwill_device_supports(data, UNIWILL_FEATURE_FN_LOCK))
>   		return 0;
>   
> -	return regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, FN_LOCK_STATUS,
> -				  data->last_status);
> +	return uniwill_write_fn_lock(data, data->last_fn_lock_state);
>   }
>   
>   static int uniwill_resume_super_key(struct uniwill_data *data)
>   {
> -	unsigned int value;
> -	int ret;
> -
>   	if (!uniwill_device_supports(data, UNIWILL_FEATURE_SUPER_KEY))
>   		return 0;
>   
> -	ret = regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &value);
> -	if (ret < 0)
> -		return ret;
> -
> -	if ((data->last_switch_status & SUPER_KEY_LOCK_STATUS) == (value & SUPER_KEY_LOCK_STATUS))
> -		return 0;
> -
> -	return regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, TRIGGER_SUPER_KEY_LOCK,
> -				 TRIGGER_SUPER_KEY_LOCK);
> +	return uniwill_write_super_key_enable(data, data->last_super_key_enable_state);
>   }
>   
>   static int uniwill_resume_battery(struct uniwill_data *data)

Re: [PATCH 4/6] platform/x86: uniwill-laptop: Rework FN lock/super key suspend handling

[Armin Wolf]

Am 10.04.26 um 17:30 schrieb Werner Sembach:

>
> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>> Currently the supend handling for the FN lock and super key enable
>
> *suspend
>
> Other then this typo lgtm:
>
> Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>

Good catch, will fix.

Thanks,
Armin Wolf

>
>> features saves the whole values of the affected registers instead of
>> the individual feature state. This duplicates the register access
>> logic from the associated sysfs attributes.
>>
>> Rework the suspend handling to reuse said register access logic and
>> only store the individual feature state as a boolean value.
>>
>> Signed-off-by: Armin Wolf<W_Armin@gmx.de>
>> ---
>>   drivers/platform/x86/uniwill/uniwill-acpi.c | 117 ++++++++++++--------
>>   1 file changed, 73 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c 
>> b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> index 7c7ea0e76966..4d149b39d94e 100644
>> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
>> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
>> @@ -341,8 +341,8 @@ struct uniwill_data {
>>       struct acpi_battery_hook hook;
>>       unsigned int last_charge_ctrl;
>>       struct mutex battery_lock;    /* Protects the list of currently 
>> registered batteries */
>> -    unsigned int last_status;
>> -    unsigned int last_switch_status;
>> +    bool last_fn_lock_state;
>> +    bool last_super_key_enable_state;
>>       struct mutex super_key_lock;    /* Protects the toggling of the 
>> super key lock state */
>>       struct list_head batteries;
>>       struct mutex led_lock;        /* Protects writes to the 
>> lightbar registers */
>> @@ -619,11 +619,22 @@ static const struct regmap_config 
>> uniwill_ec_config = {
>>       .use_single_write = true,
>>   };
>>   +static int uniwill_write_fn_lock(struct uniwill_data *data, bool 
>> status)
>> +{
>> +    unsigned int value;
>> +
>> +    if (status)
>> +        value = FN_LOCK_STATUS;
>> +    else
>> +        value = 0;
>> +
>> +    return regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, 
>> FN_LOCK_STATUS, value);
>> +}
>> +
>>   static ssize_t fn_lock_store(struct device *dev, struct 
>> device_attribute *attr, const char *buf,
>>                    size_t count)
>>   {
>>       struct uniwill_data *data = dev_get_drvdata(dev);
>> -    unsigned int value;
>>       bool enable;
>>       int ret;
>>   @@ -631,21 +642,15 @@ static ssize_t fn_lock_store(struct device 
>> *dev, struct device_attribute *attr,
>>       if (ret < 0)
>>           return ret;
>>   -    if (enable)
>> -        value = FN_LOCK_STATUS;
>> -    else
>> -        value = 0;
>> -
>> -    ret = regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, 
>> FN_LOCK_STATUS, value);
>> +    ret = uniwill_write_fn_lock(data, enable);
>>       if (ret < 0)
>>           return ret;
>>         return count;
>>   }
>>   -static ssize_t fn_lock_show(struct device *dev, struct 
>> device_attribute *attr, char *buf)
>> +static int uniwill_read_fn_lock(struct uniwill_data *data, bool 
>> *status)
>>   {
>> -    struct uniwill_data *data = dev_get_drvdata(dev);
>>       unsigned int value;
>>       int ret;
>>   @@ -653,23 +658,31 @@ static ssize_t fn_lock_show(struct device 
>> *dev, struct device_attribute *attr, c
>>       if (ret < 0)
>>           return ret;
>>   -    return sysfs_emit(buf, "%d\n", !!(value & FN_LOCK_STATUS));
>> -}
>> +    *status = !!(value & FN_LOCK_STATUS);
>>   -static DEVICE_ATTR_RW(fn_lock);
>> +    return 0;
>> +}
>>   -static ssize_t super_key_enable_store(struct device *dev, struct 
>> device_attribute *attr,
>> -                      const char *buf, size_t count)
>> +static ssize_t fn_lock_show(struct device *dev, struct 
>> device_attribute *attr, char *buf)
>>   {
>>       struct uniwill_data *data = dev_get_drvdata(dev);
>> -    unsigned int value;
>> -    bool enable;
>> +    bool status;
>>       int ret;
>>   -    ret = kstrtobool(buf, &enable);
>> +    ret = uniwill_read_fn_lock(data, &status);
>>       if (ret < 0)
>>           return ret;
>>   +    return sysfs_emit(buf, "%d\n", status);
>> +}
>> +
>> +static DEVICE_ATTR_RW(fn_lock);
>> +
>> +static int uniwill_write_super_key_enable(struct uniwill_data *data, 
>> bool status)
>> +{
>> +    unsigned int value;
>> +    int ret;
>> +
>>       guard(mutex)(&data->super_key_lock);
>>         ret = regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &value);
>> @@ -680,20 +693,33 @@ static ssize_t super_key_enable_store(struct 
>> device *dev, struct device_attribut
>>        * We can only toggle the super key lock, so we return early if 
>> the setting
>>        * is already in the correct state.
>>        */
>> -    if (enable == !(value & SUPER_KEY_LOCK_STATUS))
>> -        return count;
>> +    if (status == !(value & SUPER_KEY_LOCK_STATUS))
>> +        return 0;
>> +
>> +    return regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, 
>> TRIGGER_SUPER_KEY_LOCK,
>> +                 TRIGGER_SUPER_KEY_LOCK);
>> +}
>> +
>> +static ssize_t super_key_enable_store(struct device *dev, struct 
>> device_attribute *attr,
>> +                      const char *buf, size_t count)
>> +{
>> +    struct uniwill_data *data = dev_get_drvdata(dev);
>> +    bool enable;
>> +    int ret;
>>   -    ret = regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, 
>> TRIGGER_SUPER_KEY_LOCK,
>> -                TRIGGER_SUPER_KEY_LOCK);
>> +    ret = kstrtobool(buf, &enable);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret = uniwill_write_super_key_enable(data, enable);
>>       if (ret < 0)
>>           return ret;
>>         return count;
>>   }
>>   -static ssize_t super_key_enable_show(struct device *dev, struct 
>> device_attribute *attr, char *buf)
>> +static int uniwill_read_super_key_enable(struct uniwill_data *data, 
>> bool *status)
>>   {
>> -    struct uniwill_data *data = dev_get_drvdata(dev);
>>       unsigned int value;
>>       int ret;
>>   @@ -701,7 +727,22 @@ static ssize_t super_key_enable_show(struct 
>> device *dev, struct device_attribute
>>       if (ret < 0)
>>           return ret;
>>   -    return sysfs_emit(buf, "%d\n", !(value & SUPER_KEY_LOCK_STATUS));
>> +    *status = !(value & SUPER_KEY_LOCK_STATUS);
>> +
>> +    return 0;
>> +}
>> +
>> +static ssize_t super_key_enable_show(struct device *dev, struct 
>> device_attribute *attr, char *buf)
>> +{
>> +    struct uniwill_data *data = dev_get_drvdata(dev);
>> +    bool status;
>> +    int ret;
>> +
>> +    ret = uniwill_read_super_key_enable(data, &status);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    return sysfs_emit(buf, "%d\n", status);
>>   }
>>     static DEVICE_ATTR_RW(super_key_enable);
>> @@ -1715,10 +1756,10 @@ static int uniwill_suspend_fn_lock(struct 
>> uniwill_data *data)
>>           return 0;
>>         /*
>> -     * The EC_ADDR_BIOS_OEM is marked as volatile, so we have to 
>> restore it
>> +     * EC_ADDR_BIOS_OEM is marked as volatile, so we have to restore it
>>        * ourselves.
>>        */
>> -    return regmap_read(data->regmap, EC_ADDR_BIOS_OEM, 
>> &data->last_status);
>> +    return uniwill_read_fn_lock(data, &data->last_fn_lock_state);
>>   }
>>     static int uniwill_suspend_super_key(struct uniwill_data *data)
>> @@ -1727,10 +1768,10 @@ static int uniwill_suspend_super_key(struct 
>> uniwill_data *data)
>>           return 0;
>>         /*
>> -     * The EC_ADDR_SWITCH_STATUS is marked as volatile, so we have 
>> to restore it
>> +     * EC_ADDR_SWITCH_STATUS is marked as volatile, so we have to 
>> restore it
>>        * ourselves.
>>        */
>> -    return regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, 
>> &data->last_switch_status);
>> +    return uniwill_read_super_key_enable(data, 
>> &data->last_super_key_enable_state);
>>   }
>>     static int uniwill_suspend_battery(struct uniwill_data *data)
>> @@ -1787,27 +1828,15 @@ static int uniwill_resume_fn_lock(struct 
>> uniwill_data *data)
>>       if (!uniwill_device_supports(data, UNIWILL_FEATURE_FN_LOCK))
>>           return 0;
>>   -    return regmap_update_bits(data->regmap, EC_ADDR_BIOS_OEM, 
>> FN_LOCK_STATUS,
>> -                  data->last_status);
>> +    return uniwill_write_fn_lock(data, data->last_fn_lock_state);
>>   }
>>     static int uniwill_resume_super_key(struct uniwill_data *data)
>>   {
>> -    unsigned int value;
>> -    int ret;
>> -
>>       if (!uniwill_device_supports(data, UNIWILL_FEATURE_SUPER_KEY))
>>           return 0;
>>   -    ret = regmap_read(data->regmap, EC_ADDR_SWITCH_STATUS, &value);
>> -    if (ret < 0)
>> -        return ret;
>> -
>> -    if ((data->last_switch_status & SUPER_KEY_LOCK_STATUS) == (value 
>> & SUPER_KEY_LOCK_STATUS))
>> -        return 0;
>> -
>> -    return regmap_write_bits(data->regmap, EC_ADDR_TRIGGER, 
>> TRIGGER_SUPER_KEY_LOCK,
>> -                 TRIGGER_SUPER_KEY_LOCK);
>> +    return uniwill_write_super_key_enable(data, 
>> data->last_super_key_enable_state);
>>   }
>>     static int uniwill_resume_battery(struct uniwill_data *data)

[PATCH 5/6] platform/x86: uniwill-laptop: Mark EC_ADDR_OEM_4 as volatile

[Armin Wolf]

It turned out that EC_ADDR_OEM_4 also contains bits with a volatile
nature. Mark the whole register as volatile to prepare for the usage
of said bits. This also means that we now have to save/restore the
touchpad toggle state ourself.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/uniwill/uniwill-acpi.c | 70 ++++++++++++++++++---
 1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
index 4d149b39d94e..a116e2ec6453 100644
--- a/drivers/platform/x86/uniwill/uniwill-acpi.c
+++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
@@ -343,6 +343,7 @@ struct uniwill_data {
 	struct mutex battery_lock;	/* Protects the list of currently registered batteries */
 	bool last_fn_lock_state;
 	bool last_super_key_enable_state;
+	bool last_touchpad_toggle_enable_state;
 	struct mutex super_key_lock;	/* Protects the toggling of the super key lock state */
 	struct list_head batteries;
 	struct mutex led_lock;		/* Protects writes to the lightbar registers */
@@ -598,6 +599,7 @@ static bool uniwill_volatile_reg(struct device *dev, unsigned int reg)
 	case EC_ADDR_PWM_2:
 	case EC_ADDR_TRIGGER:
 	case EC_ADDR_SWITCH_STATUS:
+	case EC_ADDR_OEM_4:
 	case EC_ADDR_CHARGE_CTRL:
 	case EC_ADDR_USB_C_POWER_PRIORITY:
 		return true;
@@ -747,11 +749,22 @@ static ssize_t super_key_enable_show(struct device *dev, struct device_attribute
 
 static DEVICE_ATTR_RW(super_key_enable);
 
+static int uniwill_write_touchpad_toggle_enable(struct uniwill_data *data, bool status)
+{
+	unsigned int value;
+
+	if (status)
+		value = 0;
+	else
+		value = TOUCHPAD_TOGGLE_OFF;
+
+	return regmap_update_bits(data->regmap, EC_ADDR_OEM_4, TOUCHPAD_TOGGLE_OFF, value);
+}
+
 static ssize_t touchpad_toggle_enable_store(struct device *dev, struct device_attribute *attr,
 					    const char *buf, size_t count)
 {
 	struct uniwill_data *data = dev_get_drvdata(dev);
-	unsigned int value;
 	bool enable;
 	int ret;
 
@@ -759,30 +772,39 @@ static ssize_t touchpad_toggle_enable_store(struct device *dev, struct device_at
 	if (ret < 0)
 		return ret;
 
-	if (enable)
-		value = 0;
-	else
-		value = TOUCHPAD_TOGGLE_OFF;
-
-	ret = regmap_update_bits(data->regmap, EC_ADDR_OEM_4, TOUCHPAD_TOGGLE_OFF, value);
+	ret = uniwill_write_touchpad_toggle_enable(data, enable);
 	if (ret < 0)
 		return ret;
 
 	return count;
 }
 
+static int uniwill_read_touchpad_toggle_enable(struct uniwill_data *data, bool *status)
+{
+	unsigned int value;
+	int ret;
+
+	ret = regmap_read(data->regmap, EC_ADDR_OEM_4, &value);
+	if (ret < 0)
+		return ret;
+
+	*status = !(value & TOUCHPAD_TOGGLE_OFF);
+
+	return 0;
+}
+
 static ssize_t touchpad_toggle_enable_show(struct device *dev, struct device_attribute *attr,
 					   char *buf)
 {
 	struct uniwill_data *data = dev_get_drvdata(dev);
-	unsigned int value;
+	bool status;
 	int ret;
 
-	ret = regmap_read(data->regmap, EC_ADDR_OEM_4, &value);
+	ret = uniwill_read_touchpad_toggle_enable(data, &status);
 	if (ret < 0)
 		return ret;
 
-	return sysfs_emit(buf, "%d\n", !(value & TOUCHPAD_TOGGLE_OFF));
+	return sysfs_emit(buf, "%d\n", status);
 }
 
 static DEVICE_ATTR_RW(touchpad_toggle_enable);
@@ -1774,6 +1796,18 @@ static int uniwill_suspend_super_key(struct uniwill_data *data)
 	return uniwill_read_super_key_enable(data, &data->last_super_key_enable_state);
 }
 
+static int uniwill_suspend_touchpad_toggle(struct uniwill_data *data)
+{
+	if (!uniwill_device_supports(data, UNIWILL_FEATURE_TOUCHPAD_TOGGLE))
+		return 0;
+
+	/*
+	 * EC_ADDR_OEM_4 is marked as volatile, so we have to restore it
+	 * ourselves.
+	 */
+	return uniwill_read_touchpad_toggle_enable(data, &data->last_touchpad_toggle_enable_state);
+}
+
 static int uniwill_suspend_battery(struct uniwill_data *data)
 {
 	if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
@@ -1809,6 +1843,10 @@ static int uniwill_suspend(struct device *dev)
 	if (ret < 0)
 		return ret;
 
+	ret = uniwill_suspend_touchpad_toggle(data);
+	if (ret < 0)
+		return ret;
+
 	ret = uniwill_suspend_battery(data);
 	if (ret < 0)
 		return ret;
@@ -1839,6 +1877,14 @@ static int uniwill_resume_super_key(struct uniwill_data *data)
 	return uniwill_write_super_key_enable(data, data->last_super_key_enable_state);
 }
 
+static int uniwill_resume_touchpad_toggle(struct uniwill_data *data)
+{
+	if (!uniwill_device_supports(data, UNIWILL_FEATURE_TOUCHPAD_TOGGLE))
+		return 0;
+
+	return uniwill_write_touchpad_toggle_enable(data, data->last_touchpad_toggle_enable_state);
+}
+
 static int uniwill_resume_battery(struct uniwill_data *data)
 {
 	if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
@@ -1884,6 +1930,10 @@ static int uniwill_resume(struct device *dev)
 	if (ret < 0)
 		return ret;
 
+	ret = uniwill_resume_touchpad_toggle(data);
+	if (ret < 0)
+		return ret;
+
 	ret = uniwill_resume_battery(data);
 	if (ret < 0)
 		return ret;
-- 
2.39.5

Re: [PATCH 5/6] platform/x86: uniwill-laptop: Mark EC_ADDR_OEM_4 as volatile

[Werner Sembach]

Am 06.04.26 um 21:36 schrieb Armin Wolf:
> It turned out that EC_ADDR_OEM_4 also contains bits with a volatile
> nature. Mark the whole register as volatile to prepare for the usage
> of said bits. This also means that we now have to save/restore the
> touchpad toggle state ourself.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
looks good

Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/uniwill/uniwill-acpi.c | 70 ++++++++++++++++++---
>   1 file changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
> index 4d149b39d94e..a116e2ec6453 100644
> --- a/drivers/platform/x86/uniwill/uniwill-acpi.c
> +++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
> @@ -343,6 +343,7 @@ struct uniwill_data {
>   	struct mutex battery_lock;	/* Protects the list of currently registered batteries */
>   	bool last_fn_lock_state;
>   	bool last_super_key_enable_state;
> +	bool last_touchpad_toggle_enable_state;
>   	struct mutex super_key_lock;	/* Protects the toggling of the super key lock state */
>   	struct list_head batteries;
>   	struct mutex led_lock;		/* Protects writes to the lightbar registers */
> @@ -598,6 +599,7 @@ static bool uniwill_volatile_reg(struct device *dev, unsigned int reg)
>   	case EC_ADDR_PWM_2:
>   	case EC_ADDR_TRIGGER:
>   	case EC_ADDR_SWITCH_STATUS:
> +	case EC_ADDR_OEM_4:
>   	case EC_ADDR_CHARGE_CTRL:
>   	case EC_ADDR_USB_C_POWER_PRIORITY:
>   		return true;
> @@ -747,11 +749,22 @@ static ssize_t super_key_enable_show(struct device *dev, struct device_attribute
>   
>   static DEVICE_ATTR_RW(super_key_enable);
>   
> +static int uniwill_write_touchpad_toggle_enable(struct uniwill_data *data, bool status)
> +{
> +	unsigned int value;
> +
> +	if (status)
> +		value = 0;
> +	else
> +		value = TOUCHPAD_TOGGLE_OFF;
> +
> +	return regmap_update_bits(data->regmap, EC_ADDR_OEM_4, TOUCHPAD_TOGGLE_OFF, value);
> +}
> +
>   static ssize_t touchpad_toggle_enable_store(struct device *dev, struct device_attribute *attr,
>   					    const char *buf, size_t count)
>   {
>   	struct uniwill_data *data = dev_get_drvdata(dev);
> -	unsigned int value;
>   	bool enable;
>   	int ret;
>   
> @@ -759,30 +772,39 @@ static ssize_t touchpad_toggle_enable_store(struct device *dev, struct device_at
>   	if (ret < 0)
>   		return ret;
>   
> -	if (enable)
> -		value = 0;
> -	else
> -		value = TOUCHPAD_TOGGLE_OFF;
> -
> -	ret = regmap_update_bits(data->regmap, EC_ADDR_OEM_4, TOUCHPAD_TOGGLE_OFF, value);
> +	ret = uniwill_write_touchpad_toggle_enable(data, enable);
>   	if (ret < 0)
>   		return ret;
>   
>   	return count;
>   }
>   
> +static int uniwill_read_touchpad_toggle_enable(struct uniwill_data *data, bool *status)
> +{
> +	unsigned int value;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, EC_ADDR_OEM_4, &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	*status = !(value & TOUCHPAD_TOGGLE_OFF);
> +
> +	return 0;
> +}
> +
>   static ssize_t touchpad_toggle_enable_show(struct device *dev, struct device_attribute *attr,
>   					   char *buf)
>   {
>   	struct uniwill_data *data = dev_get_drvdata(dev);
> -	unsigned int value;
> +	bool status;
>   	int ret;
>   
> -	ret = regmap_read(data->regmap, EC_ADDR_OEM_4, &value);
> +	ret = uniwill_read_touchpad_toggle_enable(data, &status);
>   	if (ret < 0)
>   		return ret;
>   
> -	return sysfs_emit(buf, "%d\n", !(value & TOUCHPAD_TOGGLE_OFF));
> +	return sysfs_emit(buf, "%d\n", status);
>   }
>   
>   static DEVICE_ATTR_RW(touchpad_toggle_enable);
> @@ -1774,6 +1796,18 @@ static int uniwill_suspend_super_key(struct uniwill_data *data)
>   	return uniwill_read_super_key_enable(data, &data->last_super_key_enable_state);
>   }
>   
> +static int uniwill_suspend_touchpad_toggle(struct uniwill_data *data)
> +{
> +	if (!uniwill_device_supports(data, UNIWILL_FEATURE_TOUCHPAD_TOGGLE))
> +		return 0;
> +
> +	/*
> +	 * EC_ADDR_OEM_4 is marked as volatile, so we have to restore it
> +	 * ourselves.
> +	 */
> +	return uniwill_read_touchpad_toggle_enable(data, &data->last_touchpad_toggle_enable_state);
> +}
> +
>   static int uniwill_suspend_battery(struct uniwill_data *data)
>   {
>   	if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
> @@ -1809,6 +1843,10 @@ static int uniwill_suspend(struct device *dev)
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = uniwill_suspend_touchpad_toggle(data);
> +	if (ret < 0)
> +		return ret;
> +
>   	ret = uniwill_suspend_battery(data);
>   	if (ret < 0)
>   		return ret;
> @@ -1839,6 +1877,14 @@ static int uniwill_resume_super_key(struct uniwill_data *data)
>   	return uniwill_write_super_key_enable(data, data->last_super_key_enable_state);
>   }
>   
> +static int uniwill_resume_touchpad_toggle(struct uniwill_data *data)
> +{
> +	if (!uniwill_device_supports(data, UNIWILL_FEATURE_TOUCHPAD_TOGGLE))
> +		return 0;
> +
> +	return uniwill_write_touchpad_toggle_enable(data, data->last_touchpad_toggle_enable_state);
> +}
> +
>   static int uniwill_resume_battery(struct uniwill_data *data)
>   {
>   	if (!uniwill_device_supports(data, UNIWILL_FEATURE_BATTERY))
> @@ -1884,6 +1930,10 @@ static int uniwill_resume(struct device *dev)
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = uniwill_resume_touchpad_toggle(data);
> +	if (ret < 0)
> +		return ret;
> +
>   	ret = uniwill_resume_battery(data);
>   	if (ret < 0)
>   		return ret;

[PATCH 6/6] platform/x86: uniwill-laptop: Add support for battery charge modes

[Armin Wolf]

Many Uniwill-based devices do not supports the already existing
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@gmx.de>
---
 .../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)
 {
 	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;
+	}
+}
+
 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[] = {
+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) {
 		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;
 
-	/*
-	 * 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)
 	}
 
 	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);
 }
 
 /*
-- 
2.39.5

Re: [PATCH 6/6] platform/x86: uniwill-laptop: Add support for battery charge modes

[Werner Sembach]

Hi Armin,

Am 06.04.26 um 21:36 schrieb Armin Wolf:
> Many Uniwill-based devices do not supports the already existing
> 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@gmx.de>
> ---
>   .../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;
> +}
> +
Maybe add a "!= 0" to make the return a clear 1 or 0
>   static int uniwill_ec_reg_write(void *context, unsigned int reg, unsigned int val)
>   {
>   	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;
> +	}
> +}
Like I wrote here: 
https://github.com/Wer-Wolf/uniwill-laptop/issues/6#issuecomment-3987100594 I 
think it would be less confusing to use

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

> +
>   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;
> +	}
>   }
Could be mistaken, but does this work without producing a compiler warning or 
error not having a return on the top level?
>   
> -static const enum power_supply_property uniwill_properties[] = {
> +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);
> +
Currently there is now way to specify just the health parameter, but i have to 
check if that is even required
>   	if (ret < 0) {
>   		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;
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.
>   
> -	/*
> -	 * 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 |
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?
> @@ -2578,8 +2734,8 @@ static int __init uniwill_init(void)
>   	}
>   
>   	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);
>   }
>   
>   /*
Is this something specific for this patch or just general cleanup that sneaked 
into here?

Best regards,

Werner

Re: [PATCH 6/6] platform/x86: uniwill-laptop: Add support for battery charge modes

[Armin Wolf]

Am 10.04.26 um 18:53 schrieb Werner Sembach:

> Hi Armin,
>
> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>> Many Uniwill-based devices do not supports the already existing
>> 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@gmx.de>
>> ---
>>   .../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;
>> +}
>> +
> Maybe add a "!= 0" to make the return a clear 1 or 0

This function already returns a boolean, so the compiler will already do this for us.

>>   static int uniwill_ec_reg_write(void *context, unsigned int reg, 
>> unsigned int val)
>>   {
>>       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;
>> +    }
>> +}
> Like I wrote here: 
> https://github.com/Wer-Wolf/uniwill-laptop/issues/6#issuecomment-3987100594 
> I think it would be less confusing to use
>
> 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
>
I think that this will confuse userspace applications. POWER_SUPPLY_CHARGE_TYPE_FAST is meant to stand for
above-average/non-default charging speed, while POWER_SUPPLY_CHARGE_TYPE_STANDARD is meant to stand for
the default/recommended setting.

In this case CHARGING_PROFILE_HIGH_CAPACITY is the default charging setting with the default charging speed,
and CHARGING_PROFILE_BALANCED is the "Long Life" option with reduced charging speed. CHARGING_PROFILE_STATIONARY
on the other hand could as well be called "Primary AC", so POWER_SUPPLY_CHARGE_TYPE_TRICKLE fits best for it.

On a separate note, UPower prefers to select "Standard" over "Fast" when disabling the charging threshold.
Mapping CHARGING_PROFILE_HIGH_CAPACITY to POWER_SUPPLY_CHARGE_TYPE_FAST would thus confuse users, because
disabling the battery charging threshold thru upower will limit them CHARGING_PROFILE_BALANCED. Basically
they cannot reach CHARGING_PROFILE_HIGH_CAPACITY again thru UPower when the charging threshold has been
enabled.

>> +
>>   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;
>> +    }
>>   }
> Could be mistaken, but does this work without producing a compiler 
> warning or error not having a return on the top level?

All exits from the switch statement end with a return statement, so we will always leave this function once we
enter the switch statement.

>>   -static const enum power_supply_property uniwill_properties[] = {
>> +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);
>> +
> Currently there is now way to specify just the health parameter, but i 
> have to check if that is even required

AFAIK the OEM software always bundles this feature with the charging limit or the charging modes.
We can separate this in the future should the need arise.

>>       if (ret < 0) {
>>           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;
> 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.

This would not work, because we must not register the battery extension when both features are not supported.

>>   -    /*
>> -     * 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 |
> 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?

I do not think that we really need that. Those device descriptors are mostly static and have a fairly trivial structure,
so we should easily spot such a mistake.

>> @@ -2578,8 +2734,8 @@ static int __init uniwill_init(void)
>>       }
>>         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);
>>   }
>>     /*
> Is this something specific for this patch or just general cleanup that 
> sneaked into here?
>
> Best regards,
>
> Werner

It is necessary because handling of UNIWILL_OSD_DC_ADAPTER_CHANGED can now fail with an error code.
I decided to place the "error handling" code inside uniwill_wmi_notify() so that the resulting error
codes are not silently swallowed.

Thanks,
Armin Wolf

Re: [PATCH 6/6] platform/x86: uniwill-laptop: Add support for battery charge modes

[Werner Sembach]

Am 10.04.26 um 19:53 schrieb Armin Wolf:
> Am 10.04.26 um 18:53 schrieb Werner Sembach:
>
>> Hi Armin,
>>
>> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>>> Many Uniwill-based devices do not supports the already existing
>>> 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@gmx.de>
>>> ---
>>>   .../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;
>>> +}
>>> +
>> Maybe add a "!= 0" to make the return a clear 1 or 0
>
> This function already returns a boolean, so the compiler will already do this 
> for us.
ack
>
>>>   static int uniwill_ec_reg_write(void *context, unsigned int reg, unsigned 
>>> int val)
>>>   {
>>>       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;
>>> +    }
>>> +}
>> Like I wrote here: 
>> https://github.com/Wer-Wolf/uniwill-laptop/issues/6#issuecomment-3987100594 I 
>> think it would be less confusing to use
>>
>> 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
>>
> I think that this will confuse userspace applications. 
> POWER_SUPPLY_CHARGE_TYPE_FAST is meant to stand for
> above-average/non-default charging speed, while 
> POWER_SUPPLY_CHARGE_TYPE_STANDARD is meant to stand for
> the default/recommended setting.
>
> In this case CHARGING_PROFILE_HIGH_CAPACITY is the default charging setting 
> with the default charging speed,
> and CHARGING_PROFILE_BALANCED is the "Long Life" option with reduced charging 
> speed. CHARGING_PROFILE_STATIONARY
> on the other hand could as well be called "Primary AC", so 
> POWER_SUPPLY_CHARGE_TYPE_TRICKLE fits best for it.
>
> On a separate note, UPower prefers to select "Standard" over "Fast" when 
> disabling the charging threshold.
> Mapping CHARGING_PROFILE_HIGH_CAPACITY to POWER_SUPPLY_CHARGE_TYPE_FAST would 
> thus confuse users, because
> disabling the battery charging threshold thru upower will limit them 
> CHARGING_PROFILE_BALANCED. Basically
> they cannot reach CHARGING_PROFILE_HIGH_CAPACITY again thru UPower when the 
> charging threshold has been
> enabled.
ok didn't know about the upower behaviour
>
>>> +
>>>   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;
>>> +    }
>>>   }
>> Could be mistaken, but does this work without producing a compiler warning or 
>> error not having a return on the top level?
>
> All exits from the switch statement end with a return statement, so we will 
> always leave this function once we
> enter the switch statement.
ack
>
>>>   -static const enum power_supply_property uniwill_properties[] = {
>>> +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);
>>> +
>> Currently there is now way to specify just the health parameter, but i have 
>> to check if that is even required
>
> AFAIK the OEM software always bundles this feature with the charging limit or 
> the charging modes.
> We can separate this in the future should the need arise.
ack
>
>>>       if (ret < 0) {
>>>           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;
>> 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.
>
> This would not work, because we must not register the battery extension when 
> both features are not supported.

ah sorry overlooked that, but what about:

     } else if (uniwill_device_supports(data, 
UNIWILL_FEATURE_BATTERY_CHARGE_MODES)) {
         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;
     } else {
         return 0;
     }

It's the same number of lines and same indentation level but more readable imho

>
>>>   -    /*
>>> -     * 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 |
>> 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?
>
> I do not think that we really need that. Those device descriptors are mostly 
> static and have a fairly trivial structure,
> so we should easily spot such a mistake.
ok
>
>>> @@ -2578,8 +2734,8 @@ static int __init uniwill_init(void)
>>>       }
>>>         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);
>>>   }
>>>     /*
>> Is this something specific for this patch or just general cleanup that 
>> sneaked into here?
>>
>> Best regards,
>>
>> Werner
>
> It is necessary because handling of UNIWILL_OSD_DC_ADAPTER_CHANGED can now 
> fail with an error code.
> I decided to place the "error handling" code inside uniwill_wmi_notify() so 
> that the resulting error
> codes are not silently swallowed.
ok
>
> Thanks,
> Armin Wolf
>

Re: [PATCH 6/6] platform/x86: uniwill-laptop: Add support for battery charge modes

[Armin Wolf]

Am 10.04.26 um 22:24 schrieb Werner Sembach:

>
> Am 10.04.26 um 19:53 schrieb Armin Wolf:
>> Am 10.04.26 um 18:53 schrieb Werner Sembach:
>>
>>> Hi Armin,
>>>
>>> Am 06.04.26 um 21:36 schrieb Armin Wolf:
>>>> Many Uniwill-based devices do not supports the already existing
>>>> 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@gmx.de>
>>>> ---
>>>>   .../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;
>>>> +}
>>>> +
>>> Maybe add a "!= 0" to make the return a clear 1 or 0
>>
>> This function already returns a boolean, so the compiler will already 
>> do this for us.
> ack
>>
>>>>   static int uniwill_ec_reg_write(void *context, unsigned int reg, 
>>>> unsigned int val)
>>>>   {
>>>>       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;
>>>> +    }
>>>> +}
>>> Like I wrote here: 
>>> https://github.com/Wer-Wolf/uniwill-laptop/issues/6#issuecomment-3987100594 
>>> I think it would be less confusing to use
>>>
>>> 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
>>>
>> I think that this will confuse userspace applications. 
>> POWER_SUPPLY_CHARGE_TYPE_FAST is meant to stand for
>> above-average/non-default charging speed, while 
>> POWER_SUPPLY_CHARGE_TYPE_STANDARD is meant to stand for
>> the default/recommended setting.
>>
>> In this case CHARGING_PROFILE_HIGH_CAPACITY is the default charging 
>> setting with the default charging speed,
>> and CHARGING_PROFILE_BALANCED is the "Long Life" option with reduced 
>> charging speed. CHARGING_PROFILE_STATIONARY
>> on the other hand could as well be called "Primary AC", so 
>> POWER_SUPPLY_CHARGE_TYPE_TRICKLE fits best for it.
>>
>> On a separate note, UPower prefers to select "Standard" over "Fast" 
>> when disabling the charging threshold.
>> Mapping CHARGING_PROFILE_HIGH_CAPACITY to 
>> POWER_SUPPLY_CHARGE_TYPE_FAST would thus confuse users, because
>> disabling the battery charging threshold thru upower will limit them 
>> CHARGING_PROFILE_BALANCED. Basically
>> they cannot reach CHARGING_PROFILE_HIGH_CAPACITY again thru UPower 
>> when the charging threshold has been
>> enabled.
> ok didn't know about the upower behaviour
>>
>>>> +
>>>>   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;
>>>> +    }
>>>>   }
>>> Could be mistaken, but does this work without producing a compiler 
>>> warning or error not having a return on the top level?
>>
>> All exits from the switch statement end with a return statement, so 
>> we will always leave this function once we
>> enter the switch statement.
> ack
>>
>>>>   -static const enum power_supply_property uniwill_properties[] = {
>>>> +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);
>>>> +
>>> Currently there is now way to specify just the health parameter, but 
>>> i have to check if that is even required
>>
>> AFAIK the OEM software always bundles this feature with the charging 
>> limit or the charging modes.
>> We can separate this in the future should the need arise.
> ack
>>
>>>>       if (ret < 0) {
>>>>           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;
>>> 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.
>>
>> This would not work, because we must not register the battery 
>> extension when both features are not supported.
>
> ah sorry overlooked that, but what about:
>
>     } else if (uniwill_device_supports(data, 
> UNIWILL_FEATURE_BATTERY_CHARGE_MODES)) {
>         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;
>     } else {
>         return 0;
>     }
>
> It's the same number of lines and same indentation level but more 
> readable imho
>
Alright, i will check if checkpatch agrees with using "else if". If that is fine then
i will restructure the code.

Thanks,
Armin Wolf

>>
>>>>   -    /*
>>>> -     * 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 |
>>> 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?
>>
>> I do not think that we really need that. Those device descriptors are 
>> mostly static and have a fairly trivial structure,
>> so we should easily spot such a mistake.
> ok
>>
>>>> @@ -2578,8 +2734,8 @@ static int __init uniwill_init(void)
>>>>       }
>>>>         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);
>>>>   }
>>>>     /*
>>> Is this something specific for this patch or just general cleanup 
>>> that sneaked into here?
>>>
>>> Best regards,
>>>
>>> Werner
>>
>> It is necessary because handling of UNIWILL_OSD_DC_ADAPTER_CHANGED 
>> can now fail with an error code.
>> I decided to place the "error handling" code inside 
>> uniwill_wmi_notify() so that the resulting error
>> codes are not silently swallowed.
> ok
>>
>> Thanks,
>> Armin Wolf
>>
>