[lm-sensors] [PATCH] hwmon: (adt7475) Fix temperature fault flags

[lm-sensors] [PATCH] hwmon: (adt7475) Fix temperature fault flags

[Jean Delvare]

The logic of temperature fault flags is wrong, it shows faults when
there are none and vice versa. Fix it.

I can't believe this has been broken since the driver was added, 8
months ago, basically breaking temp1 and temp3, and nobody ever
complained.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Jordan Crouse <jordan@cosmicpenguin.net>
---
 drivers/hwmon/adt7475.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- linux-2.6.32-rc6.orig/drivers/hwmon/adt7475.c	2009-11-08 19:21:34.000000000 +0100
+++ linux-2.6.32-rc6/drivers/hwmon/adt7475.c	2009-11-08 19:22:06.000000000 +0100
@@ -350,8 +350,7 @@ static ssize_t show_temp(struct device *
 
 	case FAULT:
 		/* Note - only for remote1 and remote2 */
-		out = data->alarms & (sattr->index ? 0x8000 : 0x4000);
-		out = out ? 0 : 1;
+		out = !!(data->alarms & (sattr->index ? 0x8000 : 0x4000));
 		break;
 
 	default:


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (adt7475) Fix temperature fault

[Hans de Goede]

Hmm,

I've a adt7475 on my home development machine in the Netherlands
(I'm currently in Brno).

I can (and would like to) verify this fix when I'm back home (friday).
The adt7475 on my machine is used to control an additional fan, and has
no external temperature sensors connected (AFAIK).

I've tested the original adt7475 driver on this machine, but I may
have simply out "temp1 ignore" (and temp3) in my sensors.conf missing
this.

Regards,

Hans


On 11/09/2009 11:37 AM, Jean Delvare wrote:
> The logic of temperature fault flags is wrong, it shows faults when
> there are none and vice versa. Fix it.
>
> I can't believe this has been broken since the driver was added, 8
> months ago, basically breaking temp1 and temp3, and nobody ever
> complained.
>
> Signed-off-by: Jean Delvare<khali@linux-fr.org>
> Cc: Hans de Goede<hdegoede@redhat.com>
> Cc: Jordan Crouse<jordan@cosmicpenguin.net>
> ---
>   drivers/hwmon/adt7475.c |    3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> --- linux-2.6.32-rc6.orig/drivers/hwmon/adt7475.c	2009-11-08 19:21:34.000000000 +0100
> +++ linux-2.6.32-rc6/drivers/hwmon/adt7475.c	2009-11-08 19:22:06.000000000 +0100
> @@ -350,8 +350,7 @@ static ssize_t show_temp(struct device *
>
>   	case FAULT:
>   		/* Note - only for remote1 and remote2 */
> -		out = data->alarms&  (sattr->index ? 0x8000 : 0x4000);
> -		out = out ? 0 : 1;
> +		out = !!(data->alarms&  (sattr->index ? 0x8000 : 0x4000));
>   		break;
>
>   	default:
>
>

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (adt7475) Fix temperature fault

[Jean Delvare]

Hi Hans,

On Mon, 09 Nov 2009 11:51:54 +0100, Hans de Goede wrote:
> I've a adt7475 on my home development machine in the Netherlands
> (I'm currently in Brno).
> 
> I can (and would like to) verify this fix when I'm back home (friday).
> The adt7475 on my machine is used to control an additional fan, and has
> no external temperature sensors connected (AFAIK).
> 
> I've tested the original adt7475 driver on this machine, but I may
> have simply out "temp1 ignore" (and temp3) in my sensors.conf missing
> this.

Thanks for the feedback. I have tested on 3 dumps I have here, one
ADT7475 and two ADT7473. The ADT7475 reports -64°C for temp1 and temp3
and has both fault bits set to 1. The two ADT7473 reports reasonable
values for both temp1 and temp3 and have the fault bits set to 0.
Assuming that the ADT7473 and ADT7475 are almost the same chip and
should thus behave the same, my change would be correct.

I have also noticed that Jordan's original code did not have the
inversion:

http://lists.lm-sensors.org/pipermail/lm-sensors/2008-January/022338.html

	case FAULT:
		/* Note - only for remote1 and remote2 */

		ret = sprintf(buf, "%d\n",
			(data->alarms & (sattr->index ? 0x4000 : 0x8000)));
		break;

but it lacked normalization (output value 0x4000 or 0x8000 instead of
1.) Maybe when normalizing the logic was inverted by accident?

The few ADT7475 data points I have found on the web suggest that on
many boards temp1 and temp3 are not connected. This could explain why
the bug went unnoticed so far. Why vendors use these chips, which are
specifically designed for automatic temperature-based fan speed
control, in designs where they don't bother wiring the thermal sensors,
is beyond me.

Anyway, I will wait for you to return home and test on your system,
before sending the patch to Linus.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (adt7475) Fix temperature fault

[Jean Delvare]

On Mon, 9 Nov 2009 13:26:21 +0100, Jean Delvare wrote:
> Hi Hans,
> 
> On Mon, 09 Nov 2009 11:51:54 +0100, Hans de Goede wrote:
> > I've a adt7475 on my home development machine in the Netherlands
> > (I'm currently in Brno).
> > 
> > I can (and would like to) verify this fix when I'm back home (friday).
> > The adt7475 on my machine is used to control an additional fan, and has
> > no external temperature sensors connected (AFAIK).
> > 
> > I've tested the original adt7475 driver on this machine, but I may
> > have simply out "temp1 ignore" (and temp3) in my sensors.conf missing
> > this.
> 
> Thanks for the feedback. I have tested on 3 dumps I have here, one
> ADT7475 and two ADT7473. The ADT7475 reports -64°C for temp1 and temp3
> and has both fault bits set to 1. The two ADT7473 reports reasonable
> values for both temp1 and temp3 and have the fault bits set to 0.
> Assuming that the ADT7473 and ADT7475 are almost the same chip and
> should thus behave the same, my change would be correct.
> 
> I have also noticed that Jordan's original code did not have the
> inversion:
> 
> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-January/022338.html
> 
> 	case FAULT:
> 		/* Note - only for remote1 and remote2 */
> 
> 		ret = sprintf(buf, "%d\n",
> 			(data->alarms & (sattr->index ? 0x4000 : 0x8000)));
> 		break;
> 
> but it lacked normalization (output value 0x4000 or 0x8000 instead of
> 1.) Maybe when normalizing the logic was inverted by accident?
> 
> The few ADT7475 data points I have found on the web suggest that on
> many boards temp1 and temp3 are not connected. This could explain why
> the bug went unnoticed so far. Why vendors use these chips, which are
> specifically designed for automatic temperature-based fan speed
> control, in designs where they don't bother wiring the thermal sensors,
> is beyond me.
> 
> Anyway, I will wait for you to return home and test on your system,
> before sending the patch to Linus.

Hans, did you have the time to test this patch?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (adt7475) Fix temperature fault

[Hans de Goede]

Hi,

On 11/15/2009 05:54 PM, Jean Delvare wrote:
> On Mon, 9 Nov 2009 13:26:21 +0100, Jean Delvare wrote:
>> Hi Hans,
>>
>> On Mon, 09 Nov 2009 11:51:54 +0100, Hans de Goede wrote:
>>> I've a adt7475 on my home development machine in the Netherlands
>>> (I'm currently in Brno).
>>>
>>> I can (and would like to) verify this fix when I'm back home (friday).
>>> The adt7475 on my machine is used to control an additional fan, and has
>>> no external temperature sensors connected (AFAIK).
>>>
>>> I've tested the original adt7475 driver on this machine, but I may
>>> have simply out "temp1 ignore" (and temp3) in my sensors.conf missing
>>> this.
>>
>> Thanks for the feedback. I have tested on 3 dumps I have here, one
>> ADT7475 and two ADT7473. The ADT7475 reports -64°C for temp1 and temp3
>> and has both fault bits set to 1. The two ADT7473 reports reasonable
>> values for both temp1 and temp3 and have the fault bits set to 0.
>> Assuming that the ADT7473 and ADT7475 are almost the same chip and
>> should thus behave the same, my change would be correct.
>>
>> I have also noticed that Jordan's original code did not have the
>> inversion:
>>
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-January/022338.html
>>
>> 	case FAULT:
>> 		/* Note - only for remote1 and remote2 */
>>
>> 		ret = sprintf(buf, "%d\n",
>> 			(data->alarms&  (sattr->index ? 0x4000 : 0x8000)));
>> 		break;
>>
>> but it lacked normalization (output value 0x4000 or 0x8000 instead of
>> 1.) Maybe when normalizing the logic was inverted by accident?
>>
>> The few ADT7475 data points I have found on the web suggest that on
>> many boards temp1 and temp3 are not connected. This could explain why
>> the bug went unnoticed so far. Why vendors use these chips, which are
>> specifically designed for automatic temperature-based fan speed
>> control, in designs where they don't bother wiring the thermal sensors,
>> is beyond me.
>>
>> Anyway, I will wait for you to return home and test on your system,
>> before sending the patch to Linus.
>
> Hans, did you have the time to test this patch?
>

I did not get around to it sofar, but I had already planned to do it this
evening, so I just have. Your patch seems to be correct when run on real
hardware (and the current code in the kernel is thus wrong).

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] hwmon: (adt7475) Fix temperature fault

[Jean Delvare]

On Sun, 15 Nov 2009 19:57:26 +0100, Hans de Goede wrote:
> On 11/15/2009 05:54 PM, Jean Delvare wrote:
> > On Mon, 9 Nov 2009 13:26:21 +0100, Jean Delvare wrote:
> >> Thanks for the feedback. I have tested on 3 dumps I have here, one
> >> ADT7475 and two ADT7473. The ADT7475 reports -64°C for temp1 and temp3
> >> and has both fault bits set to 1. The two ADT7473 reports reasonable
> >> values for both temp1 and temp3 and have the fault bits set to 0.
> >> Assuming that the ADT7473 and ADT7475 are almost the same chip and
> >> should thus behave the same, my change would be correct.
> >>
> >> I have also noticed that Jordan's original code did not have the
> >> inversion:
> >>
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2008-January/022338.html
> >>
> >> 	case FAULT:
> >> 		/* Note - only for remote1 and remote2 */
> >>
> >> 		ret = sprintf(buf, "%d\n",
> >> 			(data->alarms&  (sattr->index ? 0x4000 : 0x8000)));
> >> 		break;
> >>
> >> but it lacked normalization (output value 0x4000 or 0x8000 instead of
> >> 1.) Maybe when normalizing the logic was inverted by accident?
> >>
> >> The few ADT7475 data points I have found on the web suggest that on
> >> many boards temp1 and temp3 are not connected. This could explain why
> >> the bug went unnoticed so far. Why vendors use these chips, which are
> >> specifically designed for automatic temperature-based fan speed
> >> control, in designs where they don't bother wiring the thermal sensors,
> >> is beyond me.
> >>
> >> Anyway, I will wait for you to return home and test on your system,
> >> before sending the patch to Linus.
> >
> > Hans, did you have the time to test this patch?
> 
> I did not get around to it sofar, but I had already planned to do it this
> evening, so I just have. Your patch seems to be correct when run on real
> hardware (and the current code in the kernel is thus wrong).

OK, thanks. I'll send the fix to Linus shortly.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors