Re: [PATCH 2/2] hwmon: (adt7475) Fix setting of hysteresis registers

[Guenter Roeck]

On Mon, Feb 20, 2023 at 12:29:56PM +1300, Tony O'Brien wrote:
> In temp_store(), for the hysteresis setting calculation there were two
> errors.  The first tries to clamp the hysteresis value by comparing the
> required hysteresis value to THERM - 15C.  This is incorrect since the
> hysteresis value is a relative value whereas THERM - 15C is an absolute

No, it isn't. The hysteresis attribute is an absolute temperature.
The value written into the attribute is expected to be in the range
[THERM - 15, THERM] (in degrees C). The value written into the register
is then in the range [15, 0]. I see nothing wrong with the current code.

> value. This causes it to always select 15C for hysteresis.  Change the
> first parameter to THERM - val to compare two absolute temperatures.
> The second error masks the wrong bits in the hysteresis register; indices
> 0 and 2 should zero bits [7:4] and preserve bits [3:0], and index 1 should
> zero bits [3:0] and preserve bits [7:4].

I'll have to verify that with the datasheet. Either case, one logical change
per patch, please.

Thanks,
Guenter

> 
> Fixes: 1c301fc5394f ("hwmon: Add a driver for the ADT7475 hardware monitoring chip")
> Signed-off-by: Tony O'Brien <tony.obrien@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/hwmon/adt7475.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index 77222c35a38e..68233191798e 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -484,14 +484,14 @@ static ssize_t temp_store(struct device *dev, struct device_attribute *attr,
>  		adt7475_read_hystersis(client);
>  
>  		temp = reg2temp(data, data->temp[THERM][sattr->index]);
> -		val = clamp_val(val, temp - 15000, temp);
> +		val = clamp_val(temp - val, temp - 15000, temp);
>  		val = (temp - val) / 1000;
>  
>  		if (sattr->index != 1) {
> -			data->temp[HYSTERSIS][sattr->index] &= 0xF0;
> +			data->temp[HYSTERSIS][sattr->index] &= 0x0F;
>  			data->temp[HYSTERSIS][sattr->index] |= (val & 0xF) << 4;
>  		} else {
> -			data->temp[HYSTERSIS][sattr->index] &= 0x0F;
> +			data->temp[HYSTERSIS][sattr->index] &= 0xF0;
>  			data->temp[HYSTERSIS][sattr->index] |= (val & 0xF);
>  		}
>  
> -- 
> 2.39.2
>