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

[Armin Wolf <W_Armin@gmx.de>]

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;