* Re: [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F
2012-01-27 13:48 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F Guenter Roeck
@ 2012-01-28 9:07 ` Jean Delvare
2012-01-28 17:04 ` Guenter Roeck
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2012-01-28 9:07 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
On Fri, 27 Jan 2012 05:48:58 -0800, Guenter Roeck wrote:
> NCT6776F can select fan input pins for fans 3 to 5 with a secondary set of
> chip register bits. Check that second set of bits in addition to the first set
> to detect if fans 3..5 are monitored.
>
> Reported-by: C. Comren <ccomren@@gmail.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/hwmon/w83627ehf.c | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> index 0e0af04..c0ef1a3 100644
> --- a/drivers/hwmon/w83627ehf.c
> +++ b/drivers/hwmon/w83627ehf.c
> @@ -1914,9 +1914,20 @@ w83627ehf_check_fan_inputs(const struct w83627ehf_sio_data *sio_data,
> fan4min = 0;
> fan5pin = 0;
> } else if (sio_data->kind = nct6776) {
> + u8 val;
Please just use regval which is already declared.
> +
> fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
> fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x01);
> fan5pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x02);
A blank line here would improve readability.
> + /* Test secondary set of fan input select registers */
> + superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
> + val = superio_inb(sio_data->sioreg, SIO_REG_ENABLE);
> + if (!fan3pin && (val & 0x80))
> + fan3pin = true;
> + if (!fan4pin && (val & 0x40))
> + fan4pin = true;
> + if (!fan5pin && (val & 0x20))
> + fan5pin = true;
I don't quite get the point of testing for !fan3pin, !fan4pin
and !fan5pin. This adds code without changing the result.
Also note that according to the NCT6776F datasheet, pins 90, 91 and 92
are GP73, GP72 and GP71 only if Super-I/O configuration register 0x27
bit 7 is set, which is not the default. I would assume that a BIOS
which takes care of setting the upper bits in configuration register
0x30 also takes care of setting bit 7 in configuration register 0x27,
but maybe the driver should still check for this.
And a blank line here would improve readability too.
> fan4min = fan4pin;
> } else if (sio_data->kind = w83667hg || sio_data->kind = w83667hg_b) {
> fan3pin = 1;
--
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] 7+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F
2012-01-27 13:48 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F Guenter Roeck
2012-01-28 9:07 ` Jean Delvare
@ 2012-01-28 17:04 ` Guenter Roeck
2012-01-28 20:34 ` Jean Delvare
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-01-28 17:04 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
On Sat, Jan 28, 2012 at 04:07:59AM -0500, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 27 Jan 2012 05:48:58 -0800, Guenter Roeck wrote:
> > NCT6776F can select fan input pins for fans 3 to 5 with a secondary set of
> > chip register bits. Check that second set of bits in addition to the first set
> > to detect if fans 3..5 are monitored.
> >
> > Reported-by: C. Comren <ccomren@@gmail.com>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > drivers/hwmon/w83627ehf.c | 11 +++++++++++
> > 1 files changed, 11 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
> > index 0e0af04..c0ef1a3 100644
> > --- a/drivers/hwmon/w83627ehf.c
> > +++ b/drivers/hwmon/w83627ehf.c
> > @@ -1914,9 +1914,20 @@ w83627ehf_check_fan_inputs(const struct w83627ehf_sio_data *sio_data,
> > fan4min = 0;
> > fan5pin = 0;
> > } else if (sio_data->kind = nct6776) {
> > + u8 val;
>
> Please just use regval which is already declared.
>
Sure.
> > +
> > fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
> > fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x01);
> > fan5pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x02);
>
> A blank line here would improve readability.
>
Ok.
> > + /* Test secondary set of fan input select registers */
> > + superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
> > + val = superio_inb(sio_data->sioreg, SIO_REG_ENABLE);
> > + if (!fan3pin && (val & 0x80))
> > + fan3pin = true;
> > + if (!fan4pin && (val & 0x40))
> > + fan4pin = true;
> > + if (!fan5pin && (val & 0x20))
> > + fan5pin = true;
>
> I don't quite get the point of testing for !fan3pin, !fan4pin
> and !fan5pin. This adds code without changing the result.
>
Logic was that I only need to check if the fans are connected to 90..92 if they are
not connected to 3..5. However, it looks like 90..92 take precedence, so
I think I have to check if the fans are connected to 90..92 first, and only
check if they are connected to 3..5 if they are not connected to 90..92.
Something like:
fan3pin = 0;
fan4pin = 0;
fan5pin = 0;
if (superio_inb(sio_data->sioreg, 0x27) & 0x80) {
superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
regval = superio_inb(sio_data->sioreg, SIO_REG_ENABLE);
fan3pin = !!(regval & 0x80);
fan4pin = !!(regval & 0x40);
fan5pin = !!(regval & 0x20);
}
if (!fan3pin)
fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
if (!fan4pin)
fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x01);
if (!fan5pin)
fan5pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x02);
Does that make sense ?
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] 7+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F
2012-01-27 13:48 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F Guenter Roeck
2012-01-28 9:07 ` Jean Delvare
2012-01-28 17:04 ` Guenter Roeck
@ 2012-01-28 20:34 ` Jean Delvare
2012-01-28 23:12 ` Guenter Roeck
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2012-01-28 20:34 UTC (permalink / raw)
To: lm-sensors
On Sat, 28 Jan 2012 09:04:28 -0800, Guenter Roeck wrote:
> Logic was that I only need to check if the fans are connected to 90..92 if they are
> not connected to 3..5. However, it looks like 90..92 take precedence, so
> I think I have to check if the fans are connected to 90..92 first, and only
> check if they are connected to 3..5 if they are not connected to 90..92.
>
> Something like:
>
> fan3pin = 0;
> fan4pin = 0;
> fan5pin = 0;
> if (superio_inb(sio_data->sioreg, 0x27) & 0x80) {
> superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
> regval = superio_inb(sio_data->sioreg, SIO_REG_ENABLE);
> fan3pin = !!(regval & 0x80);
> fan4pin = !!(regval & 0x40);
> fan5pin = !!(regval & 0x20);
> }
> if (!fan3pin)
> fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
> if (!fan4pin)
> fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x01);
> if (!fan5pin)
> fan5pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x02);
>
> Does that make sense ?
Yes, this should work in practice. However some improper chip
configurations would not be dealt with properly by the code above. For
example bits 5, 6 and 7 of configuration register SIO_REG_ENABLE set but
bit 7 of configuration register 0x27 not set makes no sense, but if the
chip is so configured then I believe the fans cannot be monitored,
regardless of the bit values in configuration registers 0x24 and 0x1C.
If you want to handle this case, you have to check every fan
separately, like:
gpok = superio_inb(sio_data->sioreg, 0x27) & 0x80;
regval = superio_inb(sio_data->sioreg, SIO_REG_ENABLE);
if (regval & 0x80)
fan3pin = gpok;
else
fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
if (regval & 0x40)
fan4pin = gpok;
else
fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x01);
[etc]
It's up to you if you want to do that for absolute correctness
(assuming I read the datasheet properly...) or prefer to stick to your
approach which should be equivalent for properly configured chips.
--
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] 7+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F
2012-01-27 13:48 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F Guenter Roeck
` (2 preceding siblings ...)
2012-01-28 20:34 ` Jean Delvare
@ 2012-01-28 23:12 ` Guenter Roeck
2012-02-07 23:52 ` Guenter Roeck
2012-02-08 16:37 ` Greg KH
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-01-28 23:12 UTC (permalink / raw)
To: lm-sensors
On Sat, Jan 28, 2012 at 03:34:00PM -0500, Jean Delvare wrote:
> On Sat, 28 Jan 2012 09:04:28 -0800, Guenter Roeck wrote:
> > Logic was that I only need to check if the fans are connected to 90..92 if they are
> > not connected to 3..5. However, it looks like 90..92 take precedence, so
> > I think I have to check if the fans are connected to 90..92 first, and only
> > check if they are connected to 3..5 if they are not connected to 90..92.
> >
> > Something like:
> >
> > fan3pin = 0;
> > fan4pin = 0;
> > fan5pin = 0;
> > if (superio_inb(sio_data->sioreg, 0x27) & 0x80) {
> > superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
> > regval = superio_inb(sio_data->sioreg, SIO_REG_ENABLE);
> > fan3pin = !!(regval & 0x80);
> > fan4pin = !!(regval & 0x40);
> > fan5pin = !!(regval & 0x20);
> > }
> > if (!fan3pin)
> > fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
> > if (!fan4pin)
> > fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x01);
> > if (!fan5pin)
> > fan5pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x02);
> >
> > Does that make sense ?
>
> Yes, this should work in practice. However some improper chip
> configurations would not be dealt with properly by the code above. For
> example bits 5, 6 and 7 of configuration register SIO_REG_ENABLE set but
> bit 7 of configuration register 0x27 not set makes no sense, but if the
> chip is so configured then I believe the fans cannot be monitored,
> regardless of the bit values in configuration registers 0x24 and 0x1C.
> If you want to handle this case, you have to check every fan
> separately, like:
>
> gpok = superio_inb(sio_data->sioreg, 0x27) & 0x80;
> regval = superio_inb(sio_data->sioreg, SIO_REG_ENABLE);
>
> if (regval & 0x80)
> fan3pin = gpok;
> else
> fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
>
> if (regval & 0x40)
> fan4pin = gpok;
> else
> fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x01);
> [etc]
>
> It's up to you if you want to do that for absolute correctness
> (assuming I read the datasheet properly...) or prefer to stick to your
> approach which should be equivalent for properly configured chips.
>
I might as well (try to) do it right, so I'll use your approach.
I'll send an updated patch in a minute.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F
2012-01-27 13:48 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F Guenter Roeck
` (3 preceding siblings ...)
2012-01-28 23:12 ` Guenter Roeck
@ 2012-02-07 23:52 ` Guenter Roeck
2012-02-08 16:37 ` Greg KH
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2012-02-07 23:52 UTC (permalink / raw)
To: lm-sensors
NCT6776F can select fan input pins for fans 3 to 5 with a secondary set of
chip register bits. Check that second set of bits in addition to the first set
to detect if fans 3..5 are monitored.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Cc: stable@vger.kernel.org # 3.0+
Acked-by: Jean Delvare <khali@linux-fr.org>
---
Backport to 3.0
Upstream commit: 585c0fd8216e0c9f98e2434092af7ec0f999522d
drivers/hwmon/w83627ehf.c | 26 +++++++++++++++++++++++---
1 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c
index 6284515..359bb1e 100644
--- a/drivers/hwmon/w83627ehf.c
+++ b/drivers/hwmon/w83627ehf.c
@@ -2104,9 +2104,29 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev)
fan4min = 0;
fan5pin = 0;
} else if (sio_data->kind = nct6776) {
- fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
- fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x01);
- fan5pin = !!(superio_inb(sio_data->sioreg, 0x1C) & 0x02);
+ bool gpok = superio_inb(sio_data->sioreg, 0x27) & 0x80;
+ u8 regval;
+
+ superio_select(sio_data->sioreg, W83627EHF_LD_HWM);
+ regval = superio_inb(sio_data->sioreg, SIO_REG_ENABLE);
+
+ if (regval & 0x80)
+ fan3pin = gpok;
+ else
+ fan3pin = !(superio_inb(sio_data->sioreg, 0x24) & 0x40);
+
+ if (regval & 0x40)
+ fan4pin = gpok;
+ else
+ fan4pin = !!(superio_inb(sio_data->sioreg, 0x1C)
+ & 0x01);
+
+ if (regval & 0x20)
+ fan5pin = gpok;
+ else
+ fan5pin = !!(superio_inb(sio_data->sioreg, 0x1C)
+ & 0x02);
+
fan4min = fan4pin;
} else if (sio_data->kind = w83667hg || sio_data->kind = w83667hg_b) {
fan3pin = 1;
--
1.7.5.4
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F
2012-01-27 13:48 [lm-sensors] [PATCH] hwmon: (w83627ehf) Fix number of fans for NCT6776F Guenter Roeck
` (4 preceding siblings ...)
2012-02-07 23:52 ` Guenter Roeck
@ 2012-02-08 16:37 ` Greg KH
5 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2012-02-08 16:37 UTC (permalink / raw)
To: lm-sensors
On Tue, Feb 07, 2012 at 03:52:24PM -0800, Guenter Roeck wrote:
> NCT6776F can select fan input pins for fans 3 to 5 with a secondary set of
> chip register bits. Check that second set of bits in addition to the first set
> to detect if fans 3..5 are monitored.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> Cc: stable@vger.kernel.org # 3.0+
> Acked-by: Jean Delvare <khali@linux-fr.org>
> ---
> Backport to 3.0
> Upstream commit: 585c0fd8216e0c9f98e2434092af7ec0f999522d
Applied, thanks.
greg k-h
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread