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

[Armin Wolf <W_Armin@gmx.de>]

Am 06.05.26 um 15:42 schrieb Ilpo Järvinen:

> On Sun, 3 May 2026, Armin Wolf wrote:
>
>> Am 30.04.26 um 14:53 schrieb Ilpo Järvinen:
>>> On Fri, 17 Apr 2026, Armin Wolf wrote:
>>>
>>>> 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")
>>>> Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
>>>> 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);
>>> ...
>>>
>>>> +	/*
>>>> +	 * 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);
>>> AFAICT, this does exactly the same thing as the other code above (but
>>> looks very different on surface). Wouldn't it make sense to have them
>>> share code?
>> I do not think that this would be a good idea. The two call sides are two
>> different, creating a helper function for both would likely be very
>> difficult.
> Both seem to be sanitizing the charge control threshold (AFAICT, both map
> 0 and out-of-range values to 100) isn't that the case? Why cannot we have
> uniwill_charge_ctrl_thres_sanitize() or something along those lines?

Alright, i will implement this.

Thanks,
Armin Wolf