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

[Werner Sembach <wse@tuxedocomputers.com>]

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