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@xxxxxx>
> > > ---
> > >   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@xxxxxxxxxxxxxxxxxxx>

> 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;