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

[Armin Wolf]

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@xxxxxxxxxxxxxxxxxxx>
> > 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)
> > +			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.

Thanks,
Armin Wolf