Re: [PATCH v2 02/11] hwmon: (amc6821) Make reading and writing fan speed limits consistent

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Guenter,

On 7/1/24 11:23 PM, Guenter Roeck wrote:
> The default value of the maximum fan speed limit register is 0,
> essentially translating to an unlimited fan speed. When reading
> the limit, a value of 0 is reported in this case. However, writing
> a value of 0 results in writing a value of 0xffff into the register,
> which is inconsistent.
> 
> To solve the problem, permit writing a limit of 0 for the maximim fan
> speed, effectively translating to "no limit". Write 0 into the register
> if a limit value of 0 is written. Otherwise limit the range to
> <1..6000000> and write 1..0xffff into the register. This ensures that
> reading and writing from and to a limit register return the same value
> while at the same time not changing reported values when reading the
> speed or limits.
> 
> While at it, restrict fan limit writes to non-negative numbers; writing
> a negative limit does not make sense and should be reported instead of
> being corrected.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Do not accept negative fan speed values
>      Display fan speed and speed limit as 0 if register value is 0
>      (instead of 6000000), as in original code.
>      Only permit writing 0 (unlimited) for the maximum fan speed.
> 
>   drivers/hwmon/amc6821.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
> index eb2d5592a41a..9c19d4d278ec 100644
> --- a/drivers/hwmon/amc6821.c
> +++ b/drivers/hwmon/amc6821.c
> @@ -617,15 +617,20 @@ static ssize_t fan_store(struct device *dev, struct device_attribute *attr,
>   {
>   	struct amc6821_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
> -	long val;
> +	unsigned long val;
>   	int ix = to_sensor_dev_attr(attr)->index;
> -	int ret = kstrtol(buf, 10, &val);
> +	int ret = kstrtoul(buf, 10, &val);
>   	if (ret)
>   		return ret;
> -	val = 1 > val ? 0xFFFF : 6000000/val;
> +
> +	/* The minimum fan speed must not be unlimited (0) */
> +	if (ix == IDX_FAN1_MIN && !val)
> +		return -EINVAL;
> +
> +	val = val > 0 ? 6000000 / clamp_val(val, 1, 6000000) : 0;
>   

I'm wondering if we shouldn't check !val for min after this line 
instead? Otherwise we allow 6000001+RPM speeds... which is technically 
unlimited.

Nitpicking though, therefore:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin