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
>
