* Re: [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix documentation of
2011-11-01 10:19 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix documentation of Jean Delvare
@ 2011-11-01 13:57 ` Guenter Roeck
2011-11-05 13:57 ` Jean Delvare
2011-11-06 16:01 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2011-11-01 13:57 UTC (permalink / raw)
To: lm-sensors
On Tue, Nov 01, 2011 at 06:19:36AM -0400, Jean Delvare wrote:
> The NCT6775F and NCT6776F have specific temperature sensor resolutions
> for sensors above 3.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
> Guenter, did I get it right?
>
Did I add the 0.0 degrees note ? Brr.
Anyway, it is kind of correct. temp7 to temp9 resolution is 0.5 degrees C, but the code
only reads full degrees because it was too complicated to merge the lowest bit.
Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
Thanks,
Guenter
> Documentation/hwmon/w83627ehf | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> --- linux-3.2-rc0.orig/Documentation/hwmon/w83627ehf 2011-10-31 17:00:50.000000000 +0100
> +++ linux-3.2-rc0/Documentation/hwmon/w83627ehf 2011-11-01 10:55:03.000000000 +0100
> @@ -62,8 +62,9 @@ sensors. The configured source for each
> in tempX_label.
>
> Temperatures are measured in degrees Celsius and measurement resolution is 1
> -degC for temp1 and and 0.5 degC for temp2 and temp3. For temp4 and higher,
> -resolution is 1 degC for W83667HG-B and 0.0 degC for NCT6775F and NCT6776F.
> +degC for temp1 and and 0.5 degC for temp2 and temp3. For W83667HG-B, temp4
> +has resolution 1 degC. For NCT6775F and NCT6776F, temp4 to temp6 have
> +resolution 0.5 degC, while temp7 to temp9 have resolution 1 degC.
> An alarm is triggered when the temperature gets higher than high limit;
> it stays on until the temperature falls below the hysteresis value.
> Alarms are only supported for temp1, temp2, and temp3.
>
>
> --
> Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix documentation of
2011-11-01 10:19 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix documentation of Jean Delvare
2011-11-01 13:57 ` Guenter Roeck
@ 2011-11-05 13:57 ` Jean Delvare
2011-11-06 16:01 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2011-11-05 13:57 UTC (permalink / raw)
To: lm-sensors
On Tue, 1 Nov 2011 06:57:36 -0700, Guenter Roeck wrote:
> On Tue, Nov 01, 2011 at 06:19:36AM -0400, Jean Delvare wrote:
> > The NCT6775F and NCT6776F have specific temperature sensor resolutions
> > for sensors above 3.
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Cc: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> > Guenter, did I get it right?
> >
> Did I add the 0.0 degrees note ? Brr.
I've done worse...
> Anyway, it is kind of correct. temp7 to temp9 resolution is 0.5 degrees C, but the code
> only reads full degrees because it was too complicated to merge the lowest bit.
It is indeed different from the other values but I wouldn't call it
terribly complicated. My only worry is related to the integrity of the
reading, i.e. how can we guarantee that the MSB and the LSB belong to
the same measurement. I would hope that the chip latches the LSB when
the MSB is read, but I don't see any mention of this in the datasheet.
The problem isn't limited to temp7 to temp9, BTW, other 9-bit
temperature inputs are affected too.
Would the following (untested) be OK with you?
---
drivers/hwmon/w83627ehf.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
--- linux-3.2-rc0.orig/drivers/hwmon/w83627ehf.c 2011-11-05 12:42:03.000000000 +0100
+++ linux-3.2-rc0/drivers/hwmon/w83627ehf.c 2011-11-05 14:48:33.000000000 +0100
@@ -260,6 +260,7 @@ static const u16 NCT6775_REG_TEMP_OVER[]
= { 0x39, 0x155, 0x255, 0, 0, 0, 0x672, 0x677, 0x67C };
static const u16 NCT6775_REG_TEMP_SOURCE[]
= { 0x621, 0x622, 0x623, 0x100, 0x200, 0x300, 0x624, 0x625, 0x626 };
+#define NCT6775_REG_TEMP_LSB 0x62E
static const char *const w83667hg_b_temp_label[] = {
"SYSTIN",
@@ -888,6 +889,17 @@ static struct w83627ehf_data *w83627ehf_
= w83627ehf_read_temp(data,
data->reg_temp_hyst[i]);
}
+ /* LSB of temp7-9 are stored in the same register */
+ if (data->have_temp & (0x7 << 6)) {
+ u8 reg = w83627ehf_read_value(data,
+ NCT6775_REG_TEMP_LSB);
+
+ for (i = 0; i < 3; i++) {
+ if (data->have_temp & (1 << (6 + i)))
+ data->temp[6 + i]
+ |= (reg << (7 - i)) & 0x80;
+ }
+ }
data->alarms = w83627ehf_read_value(data,
W83627EHF_REG_ALARM1) |
> Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
Thanks.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix documentation of
2011-11-01 10:19 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix documentation of Jean Delvare
2011-11-01 13:57 ` Guenter Roeck
2011-11-05 13:57 ` Jean Delvare
@ 2011-11-06 16:01 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2011-11-06 16:01 UTC (permalink / raw)
To: lm-sensors
On Sat, Nov 05, 2011 at 09:57:14AM -0400, Jean Delvare wrote:
> On Tue, 1 Nov 2011 06:57:36 -0700, Guenter Roeck wrote:
> > On Tue, Nov 01, 2011 at 06:19:36AM -0400, Jean Delvare wrote:
> > > The NCT6775F and NCT6776F have specific temperature sensor resolutions
> > > for sensors above 3.
> > >
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > > Cc: Guenter Roeck <guenter.roeck@ericsson.com>
> > > ---
> > > Guenter, did I get it right?
> > >
> > Did I add the 0.0 degrees note ? Brr.
>
> I've done worse...
>
> > Anyway, it is kind of correct. temp7 to temp9 resolution is 0.5 degrees C, but the code
> > only reads full degrees because it was too complicated to merge the lowest bit.
>
> It is indeed different from the other values but I wouldn't call it
> terribly complicated. My only worry is related to the integrity of the
> reading, i.e. how can we guarantee that the MSB and the LSB belong to
> the same measurement. I would hope that the chip latches the LSB when
> the MSB is read, but I don't see any mention of this in the datasheet.
> The problem isn't limited to temp7 to temp9, BTW, other 9-bit
> temperature inputs are affected too.
>
> Would the following (untested) be OK with you?
>
> ---
> drivers/hwmon/w83627ehf.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> --- linux-3.2-rc0.orig/drivers/hwmon/w83627ehf.c 2011-11-05 12:42:03.000000000 +0100
> +++ linux-3.2-rc0/drivers/hwmon/w83627ehf.c 2011-11-05 14:48:33.000000000 +0100
> @@ -260,6 +260,7 @@ static const u16 NCT6775_REG_TEMP_OVER[]
> = { 0x39, 0x155, 0x255, 0, 0, 0, 0x672, 0x677, 0x67C };
> static const u16 NCT6775_REG_TEMP_SOURCE[]
> = { 0x621, 0x622, 0x623, 0x100, 0x200, 0x300, 0x624, 0x625, 0x626 };
> +#define NCT6775_REG_TEMP_LSB 0x62E
>
> static const char *const w83667hg_b_temp_label[] = {
> "SYSTIN",
> @@ -888,6 +889,17 @@ static struct w83627ehf_data *w83627ehf_
> = w83627ehf_read_temp(data,
> data->reg_temp_hyst[i]);
> }
> + /* LSB of temp7-9 are stored in the same register */
> + if (data->have_temp & (0x7 << 6)) {
> + u8 reg = w83627ehf_read_value(data,
> + NCT6775_REG_TEMP_LSB);
> +
> + for (i = 0; i < 3; i++) {
> + if (data->have_temp & (1 << (6 + i)))
> + data->temp[6 + i]
> + |= (reg << (7 - i)) & 0x80;
> + }
> + }
>
> data->alarms = w83627ehf_read_value(data,
> W83627EHF_REG_ALARM1) |
>
Looks good to me. I'll test it on my system to be sure, and let you know.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread