[PATCH 0/2] hwmon: (adt7475) Fixes for acoustics and hysteresis

[PATCH 0/2] hwmon: (adt7475) Fixes for acoustics and hysteresis

[Tony O'Brien]

The patches contained herein fix the ADT7475 driver.  The first fixes
the reading of the Enhanced Acoustics Register 2, and the second fixes the
setting of the hysteresis registers.

Tony O'Brien (2):
  hwmon: (adt7475) Display smoothing attributes in correct order
  hwmon: (adt7475) Fix setting of hysteresis registers

 drivers/hwmon/adt7475.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.39.2

[PATCH 1/2] hwmon: (adt7475) Display smoothing attributes in correct order

[Tony O'Brien]

Throughout the ADT7475 driver, attributes relating to the temperature
sensors are displayed in the order Remote 1, Local, Remote 2.  Make
temp_st_show() conform to this expectation so that values set by
temp_st_store() can be displayed using the correct attribute.

Fixes: 8f05bcc33e74 ("hwmon: (adt7475) temperature smoothing")
Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
---
 drivers/hwmon/adt7475.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 51b3d16c3223..77222c35a38e 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -556,11 +556,11 @@ static ssize_t temp_st_show(struct device *dev, struct device_attribute *attr,
 		val = data->enh_acoustics[0] & 0xf;
 		break;
 	case 1:
-		val = (data->enh_acoustics[1] >> 4) & 0xf;
+		val = data->enh_acoustics[1] & 0xf;
 		break;
 	case 2:
 	default:
-		val = data->enh_acoustics[1] & 0xf;
+		val = (data->enh_acoustics[1] >> 4) & 0xf;
 		break;
 	}
 
-- 
2.39.2

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

[Tony O'Brien]

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
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].

Fixes: 1c301fc5394f ("hwmon: Add a driver for the ADT7475 hardware monitoring chip")
Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
---
 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

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@alliedtelesis.co.nz>
> ---
>  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
> 

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

[Guenter Roeck]

On 2/20/23 06:08, Guenter Roeck wrote:
> 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.
> 

Actually, the problem is obvious from the code. Still, either drop the first
part of the patch or separate into two patches and explain in detail what
you think is wrong in the first part. There is a comment in the code

                 /*
                  * The value will be given as an absolute value, turn it
                  * into an offset based on THERM
                  */

Maybe I am missing something, but I think that is exactly what the current
code is doing.

Thanks,
Guenter

> Thanks,
> Guenter
> 
>>
>> Fixes: 1c301fc5394f ("hwmon: Add a driver for the ADT7475 hardware monitoring chip")
>> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
>> ---
>>   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
>>

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

[Tony O'Brien]

On 22/02/23 06:15, Guenter Roeck wrote:
> On 2/20/23 06:08, Guenter Roeck wrote:
>> 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.
>>
> 
> Actually, the problem is obvious from the code. Still, either drop the 
> first
> part of the patch or separate into two patches and explain in detail what
> you think is wrong in the first part. There is a comment in the code
> 
>                  /*
>                   * The value will be given as an absolute value, turn it
>                   * into an offset based on THERM
>                   */
> 
> Maybe I am missing something, but I think that is exactly what the current
> code is doing.
Sorry for the late reply, my email wasn't working properly.

You are correct, and so is the code.  I assumed the hysteresis would be 
written to the sysfs temp#_crit_hyst file as a relative temperature, 
(being called 'hyst') not an absolute value.  I have fixed our host code 
to write this correctly and it works fine now.  This part of the patch 
has been dropped and v2 has been sent.  Thanks for your help.

Cheers,
Tony.
> 
> Thanks,
> Guenter
> 
>> Thanks,
>> Guenter
>>
>>>
>>> Fixes: 1c301fc5394f ("hwmon: Add a driver for the ADT7475 hardware 
>>> monitoring chip")
>>> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz>
>>> ---
>>>   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
>>>
>