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

[Werner Sembach <wse@tuxedocomputers.com>]

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;