[lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading in

[lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading in

[Andrew Paprocki]

This splits the it8712 chip type into two chip types to distinguish the
changes made in rev 8 of the chip. A new type it8712old represents all
revs prior to rev 8. The it8712 chip type now represents rev 8 and
greater. For clarity, the it87 chip type is now named it8705.

The it8712 chip type now enables 16-bit fan tachometer reading to get
proper readings. The register used in the it8712old chip type no longer
has the same meaning in rev 8 of the chip.

Also, a new module param array enable_fans is added. This was needed to
get fan2 to show up on an it8712 rev 8 chip which was not enabled at
boot time by the BIOS. The module param is checked and if a fan index
value is 1, the fan is forced on even if it is not configured that way
from the BIOS.

Signed-off-by: Andrew Paprocki <andrew@ishiboo.com>
---
  drivers/hwmon/it87.c |   84
+++++++++++++++++++++++++++++++++++++-------------
  1 files changed, 62 insertions(+), 22 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index ad6c8a3..3c23726 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -50,7 +50,7 @@

  #define DRVNAME "it87"

-enum chips { it87, it8712, it8716, it8718 };
+enum chips { it8705, it8712old, it8712, it8716, it8718 };

  static struct platform_device *pdev;

@@ -121,6 +121,9 @@ static int update_vbat;
  /* Not all BIOSes properly configure the PWM registers */
  static int fix_pwm_polarity;

+/* Not all BIOSes properly enable all available fans */
+static int enable_fans[5];
+
  /* Many IT87 constants specified below */

  /* Length of ISA address segment */
@@ -145,11 +148,11 @@ static int fix_pwm_polarity;
  #define IT87_REG_ALARM3        0x03

  /* The IT8718F has the VID value in a different register, in Super-I/O
-   configuration space. */
+ * configuration space. */
  #define IT87_REG_VID           0x0a
-/* Warning: register 0x0b is used for something completely different in
-   new chips/revisions. I suspect only 16-bit tachometer mode will work
-   for these. */
+/* The IT8705F and IT8712F earlier than revision 0x08 use register 0x0b
+ * for fan divisors. Later IT8712F revisions must use 16-bit tachometer
+ * mode. */
  #define IT87_REG_FAN_DIV       0x0b
  #define IT87_REG_FAN_16BIT     0x0c

@@ -904,16 +907,21 @@ static int __init it87_find(unsigned short *address,
  {
  	int err = -ENODEV;
  	u16 chip_type;
+        u8 revision;

  	superio_enter();
  	chip_type = superio_inw(DEVID);
+        revision = superio_inb(DEVREV);

  	switch (chip_type) {
  	case IT8705F_DEVID:
-		sio_data->type = it87;
+		sio_data->type = it8705;
  		break;
  	case IT8712F_DEVID:
-		sio_data->type = it8712;
+                if (revision < 0x08)
+                        sio_data->type = it8712old;
+                else
+                        sio_data->type = it8712;
  		break;
  	case IT8716F_DEVID:
  	case IT8726F_DEVID:
@@ -975,7 +983,8 @@ static int __devinit it87_probe(struct
platform_device *pdev)
  	int err = 0;
  	int enable_pwm_interface;
  	static const char *names[] = {
-		"it87",
+		"it8705",
+		"it8712",
  		"it8712",
  		"it8716",
  		"it8718",
@@ -1021,7 +1030,7 @@ static int __devinit it87_probe(struct
platform_device *pdev)
  		goto ERROR2;

  	/* Do not create fan files for disabled fans */
-	if (data->type = it8716 || data->type = it8718) {
+        if (data->type != it8705 && data->type != it8712old) {
  		/* 16-bit tachometers */
  		if (data->has_fan & (1 << 0)) {
  			if ((err = device_create_file(dev,
@@ -1111,8 +1120,7 @@ static int __devinit it87_probe(struct
platform_device *pdev)
  			goto ERROR4;
  	}

-	if (data->type = it8712 || data->type = it8716
-	 || data->type = it8718) {
+	if (data->type != it8705) {
  		data->vrm = vid_which_vrm();
  		/* VID reading from Super-I/O config space if available */
  		data->vid = sio_data->vid_value;
@@ -1232,6 +1240,7 @@ static void __devinit it87_init_device(struct
platform_device *pdev)
  {
  	struct it87_data *data = platform_get_drvdata(pdev);
  	int tmp, i;
+        u8 force_fan_main_ctrl;

  	/* initialize to sane defaults:
  	 * - if the chip is in manual pwm mode, this will be overwritten with
@@ -1283,21 +1292,43 @@ static void __devinit it87_init_device(struct
platform_device *pdev)
  		data->fan_main_ctrl |= 0x70;
  		it87_write_value(data, IT87_REG_FAN_MAIN_CTRL, data->fan_main_ctrl);
  	}
+
+        /* Force on any of the first three fans */
+        force_fan_main_ctrl = data->fan_main_ctrl;
+        if (enable_fans[0])
+                force_fan_main_ctrl |= (1 << 4);
+        if (enable_fans[1])
+                force_fan_main_ctrl |= (1 << 5);
+        if (enable_fans[2])
+                force_fan_main_ctrl |= (1 << 6);
+        if (force_fan_main_ctrl != data->fan_main_ctrl) {
+                data->fan_main_ctrl = force_fan_main_ctrl;
+                it87_write_value(data, IT87_REG_FAN_MAIN_CTRL,
+                                 data->fan_main_ctrl);
+        }
+
  	data->has_fan = (data->fan_main_ctrl >> 4) & 0x07;

  	/* Set tachometers to 16-bit mode if needed */
-	if (data->type = it8716 || data->type = it8718) {
-		tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
+	if (data->type = it8712 || data->type = it8716
+         || data->type = it8718) {
+                int new_tmp = tmp = it87_read_value(data,
IT87_REG_FAN_16BIT);
  		if (~tmp & 0x07 & data->has_fan) {
  			dev_dbg(&pdev->dev,
-				"Setting fan1-3 to 16-bit mode\n");
-			it87_write_value(data, IT87_REG_FAN_16BIT,
-					 tmp | 0x07);
+				"Setting all fans to 16-bit mode\n");
+			it87_write_value(data, IT87_REG_FAN_16BIT, tmp | 0x07);
  		}
  		if (tmp & (1 << 4))
  			data->has_fan |= (1 << 3);	/* fan4 enabled */
+                else if (enable_fans[3])
+                        new_tmp |= (1 << 3);            /* fan4 force
enabled */
  		if (tmp & (1 << 5))
  			data->has_fan |= (1 << 4);	/* fan5 enabled */
+                else if (enable_fans[4])
+                        new_tmp |= (1 << 4);            /* fan5 force
enabled */
+
+                if (new_tmp != tmp)
+                        it87_write_value(data, IT87_REG_FAN_16BIT,
new_tmp);
  	}

  	/* Set current fan mode registers and the default settings for the
@@ -1316,7 +1347,9 @@ static void __devinit it87_init_device(struct
platform_device *pdev)
  				data->manual_pwm_ctl[i] = PWM_FROM_REG(tmp);
  			}
  		}
- 	}
+	}
+
+        /* FIXME: perform the above logic for fan4 and fan5 if necessary */

  	/* Start monitoring */
  	it87_write_value(data, IT87_REG_CONFIG,
@@ -1362,7 +1395,8 @@ static struct it87_data *it87_update_device(struct
device *dev)
  			data->fan[i] = it87_read_value(data,
  				       IT87_REG_FAN[i]);
  			/* Add high byte if in 16-bit mode */
-			if (data->type = it8716 || data->type = it8718) {
+			if (data->type = it8712 || data->type = it8716
+                         || data->type = it8718) {
  				data->fan[i] |= it87_read_value(data,
  						IT87_REG_FANX[i]) << 8;
  				data->fan_min[i] |= it87_read_value(data,
@@ -1378,9 +1412,11 @@ static struct it87_data
*it87_update_device(struct device *dev)
  			    it87_read_value(data, IT87_REG_TEMP_LOW(i));
  		}

-		/* Newer chips don't have clock dividers */
-		if ((data->has_fan & 0x07) && data->type != it8716
-		 && data->type != it8718) {
+                /* The 8705 uses fan divisors. The 8712 had fan divisors
+                 * prior to revision 0x08.
+                 */
+		if ((data->has_fan & 0x07) &&
+                    (data->type = it8705 || data->type = it8712old)) {
  			i = it87_read_value(data, IT87_REG_FAN_DIV);
  			data->fan_div[0] = i & 0x07;
  			data->fan_div[1] = (i >> 3) & 0x07;
@@ -1396,7 +1432,9 @@ static struct it87_data *it87_update_device(struct
device *dev)
  		data->fan_ctl = it87_read_value(data, IT87_REG_FAN_CTL);

  		data->sensor = it87_read_value(data, IT87_REG_TEMP_ENABLE);
-		/* The 8705 does not have VID capability */
+                /* The 8705 does not have VID capability.
+                 * The 8718 does not use IT87_REG_VID for the same purpose.
+                 */
  		if (data->type = it8712 || data->type = it8716) {
  			data->vid = it87_read_value(data, IT87_REG_VID);
  			/* The older IT8712F revisions had only 5 VID pins,
@@ -1495,6 +1533,8 @@ module_param(update_vbat, bool, 0);
  MODULE_PARM_DESC(update_vbat, "Update vbat if set else return powerup
value");
  module_param(fix_pwm_polarity, bool, 0);
  MODULE_PARM_DESC(fix_pwm_polarity, "Force PWM polarity to active high
(DANGEROUS)");
+module_param_array(enable_fans, bool, 0, 0444);
+MODULE_PARM_DESC(enable_fans, "Force enable fans (up to 5 fans when
supported)");
  MODULE_LICENSE("GPL");

  module_init(sm_it87_init);

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

Re: [lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading

[Jean Delvare]

Hi Andrew,

On Mon, 11 Feb 2008 01:32:59 -0500, Andrew Paprocki wrote:
> This splits the it8712 chip type into two chip types to distinguish the
> changes made in rev 8 of the chip. A new type it8712old represents all
> revs prior to rev 8. The it8712 chip type now represents rev 8 and
> greater.

I am not fond of the "it8712old" name, because "old" doesn't tell the
reader in which way it is different from the (new) it8712 type. It also
doesn't "scale" well... What will you do if the next IT8712F revision
differs from rev. K in a new way?

I would prefer a separate field in struct it87_sio_data and struct
it87_data, recording the revision value. This is reusable for future
differences between the various revisions of all supported chips. This
approach is also less likely to break the current code.

As a side note, I think it's rather weird that the IT8712F rev. J (and
later) do default to 8-bit fan speed values while they no longer
support fan clock dividers. As these revisions of the chip are not
backward compatible with the older revisions, ITE should really have
switched to a saner default.

> For clarity, the it87 chip type is now named it8705.

Bad idea. Renaming it87 chips to it8705 will break compatibility with
libsensors 2.x. Even for libsensors 3.x, the configuration file
mentions specific chip names, and this change would cause the "it87-*"
section to be ignored.

So, please revert this part of your patch.

> The it8712 chip type now enables 16-bit fan tachometer reading to get
> proper readings. The register used in the it8712old chip type no longer
> has the same meaning in rev 8 of the chip.

Which register are you talking about please?

> Also, a new module param array enable_fans is added. This was needed to
> get fan2 to show up on an it8712 rev 8 chip which was not enabled at
> boot time by the BIOS. The module param is checked and if a fan index
> value is 1, the fan is forced on even if it is not configured that way
> from the BIOS.

This is a BIOS bug, which should be fixed in the BIOS. Please report to
your motherboard manufacturer and get them to fix the bug. What
motherboard is this, BTW? Usually motherboard BIOS enable all fans, or
none; I don't remember seeing a BIOS (improperly) enabling a subset
only.

It would have had to be a separate patch anyway.

Review:

> Signed-off-by: Andrew Paprocki <andrew@ishiboo.com>
> ---
>   drivers/hwmon/it87.c |   84
> +++++++++++++++++++++++++++++++++++++-------------

Your e-mail client wraps long lines, so I can't apply your patch.
Please fix your e-mail client configuration. If you can't get it to
work, include the patch as a text attachment.

>   1 files changed, 62 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index ad6c8a3..3c23726 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -50,7 +50,7 @@
> 
>   #define DRVNAME "it87"
> 
> -enum chips { it87, it8712, it8716, it8718 };
> +enum chips { it8705, it8712old, it8712, it8716, it8718 };
> 
>   static struct platform_device *pdev;
> 
> @@ -121,6 +121,9 @@ static int update_vbat;
>   /* Not all BIOSes properly configure the PWM registers */
>   static int fix_pwm_polarity;
> 
> +/* Not all BIOSes properly enable all available fans */
> +static int enable_fans[5];
> +
>   /* Many IT87 constants specified below */
> 
>   /* Length of ISA address segment */
> @@ -145,11 +148,11 @@ static int fix_pwm_polarity;
>   #define IT87_REG_ALARM3        0x03
> 
>   /* The IT8718F has the VID value in a different register, in Super-I/O
> -   configuration space. */
> + * configuration space. */

Please revert. If anything, your comments should match the style of the
file, not the other way around!

>   #define IT87_REG_VID           0x0a
> -/* Warning: register 0x0b is used for something completely different in
> -   new chips/revisions. I suspect only 16-bit tachometer mode will work
> -   for these. */
> +/* The IT8705F and IT8712F earlier than revision 0x08 use register 0x0b
> + * for fan divisors. Later IT8712F revisions must use 16-bit tachometer
> + * mode. */
>   #define IT87_REG_FAN_DIV       0x0b
>   #define IT87_REG_FAN_16BIT     0x0c
> 
> @@ -904,16 +907,21 @@ static int __init it87_find(unsigned short *address,
>   {
>   	int err = -ENODEV;
>   	u16 chip_type;
> +        u8 revision;

Bad indentation. Please use tabs, not spaces, for indentation, as
documented in Documentation/CodingStyle.

> 
>   	superio_enter();
>   	chip_type = superio_inw(DEVID);
> +        revision = superio_inb(DEVREV);

The revision is only the low nibble of the register, so you should mask
the value with & 0x0f.

> 
>   	switch (chip_type) {
>   	case IT8705F_DEVID:
> -		sio_data->type = it87;
> +		sio_data->type = it8705;
>   		break;
>   	case IT8712F_DEVID:
> -		sio_data->type = it8712;
> +                if (revision < 0x08)
> +                        sio_data->type = it8712old;
> +                else
> +                        sio_data->type = it8712;
>   		break;
>   	case IT8716F_DEVID:
>   	case IT8726F_DEVID:

Later in this function, the revision is read from the chip. Now that
you have it in a variable you should use this variable instead.

> @@ -975,7 +983,8 @@ static int __devinit it87_probe(struct
> platform_device *pdev)
>   	int err = 0;
>   	int enable_pwm_interface;
>   	static const char *names[] = {
> -		"it87",
> +		"it8705",
> +		"it8712",
>   		"it8712",
>   		"it8716",
>   		"it8718",
> @@ -1021,7 +1030,7 @@ static int __devinit it87_probe(struct
> platform_device *pdev)
>   		goto ERROR2;
> 
>   	/* Do not create fan files for disabled fans */
> -	if (data->type = it8716 || data->type = it8718) {
> +        if (data->type != it8705 && data->type != it8712old) {
>   		/* 16-bit tachometers */
>   		if (data->has_fan & (1 << 0)) {
>   			if ((err = device_create_file(dev,
> @@ -1111,8 +1120,7 @@ static int __devinit it87_probe(struct
> platform_device *pdev)
>   			goto ERROR4;
>   	}
> 
> -	if (data->type = it8712 || data->type = it8716
> -	 || data->type = it8718) {
> +	if (data->type != it8705) {
>   		data->vrm = vid_which_vrm();
>   		/* VID reading from Super-I/O config space if available */
>   		data->vid = sio_data->vid_value;

> @@ -1362,7 +1395,8 @@ static struct it87_data *it87_update_device(struct
> device *dev)
>   			data->fan[i] = it87_read_value(data,
>   				       IT87_REG_FAN[i]);
>   			/* Add high byte if in 16-bit mode */
> -			if (data->type = it8716 || data->type = it8718) {
> +			if (data->type = it8712 || data->type = it8716
> +                         || data->type = it8718) {
>   				data->fan[i] |= it87_read_value(data,
>   						IT87_REG_FANX[i]) << 8;
>   				data->fan_min[i] |= it87_read_value(data,
> @@ -1378,9 +1412,11 @@ static struct it87_data
> *it87_update_device(struct device *dev)
>   			    it87_read_value(data, IT87_REG_TEMP_LOW(i));
>   		}
> 
> -		/* Newer chips don't have clock dividers */
> -		if ((data->has_fan & 0x07) && data->type != it8716
> -		 && data->type != it8718) {
> +                /* The 8705 uses fan divisors. The 8712 had fan divisors
> +                 * prior to revision 0x08.
> +                 */
> +		if ((data->has_fan & 0x07) &&
> +                    (data->type = it8705 || data->type = it8712old)) {
>   			i = it87_read_value(data, IT87_REG_FAN_DIV);
>   			data->fan_div[0] = i & 0x07;
>   			data->fan_div[1] = (i >> 3) & 0x07;
> @@ -1396,7 +1432,9 @@ static struct it87_data *it87_update_device(struct
> device *dev)
>   		data->fan_ctl = it87_read_value(data, IT87_REG_FAN_CTL);
> 
>   		data->sensor = it87_read_value(data, IT87_REG_TEMP_ENABLE);
> -		/* The 8705 does not have VID capability */
> +                /* The 8705 does not have VID capability.
> +                 * The 8718 does not use IT87_REG_VID for the same purpose.
> +                 */
>   		if (data->type = it8712 || data->type = it8716) {
>   			data->vid = it87_read_value(data, IT87_REG_VID);
>   			/* The older IT8712F revisions had only 5 VID pins,
> @@ -1495,6 +1533,8 @@ module_param(update_vbat, bool, 0);
>   MODULE_PARM_DESC(update_vbat, "Update vbat if set else return powerup
> value");
>   module_param(fix_pwm_polarity, bool, 0);
>   MODULE_PARM_DESC(fix_pwm_polarity, "Force PWM polarity to active high
> (DANGEROUS)");
> +module_param_array(enable_fans, bool, 0, 0444);
> +MODULE_PARM_DESC(enable_fans, "Force enable fans (up to 5 fans when
> supported)");
>   MODULE_LICENSE("GPL");
> 
>   module_init(sm_it87_init);

-- 
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: it87 support for 16-bit fan reading

[Jean Delvare]

Hi Andrew,

On Sun, 24 Feb 2008 16:43:35 +0100, Jean Delvare wrote:
> On Mon, 11 Feb 2008 01:32:59 -0500, Andrew Paprocki wrote:
> > This splits the it8712 chip type into two chip types to distinguish the
> > changes made in rev 8 of the chip. A new type it8712old represents all
> > revs prior to rev 8. The it8712 chip type now represents rev 8 and
> > greater.
> 
> I am not fond of the "it8712old" name, because "old" doesn't tell the
> reader in which way it is different from the (new) it8712 type. It also
> doesn't "scale" well... What will you do if the next IT8712F revision
> differs from rev. K in a new way?
> 
> I would prefer a separate field in struct it87_sio_data and struct
> it87_data, recording the revision value. This is reusable for future
> differences between the various revisions of all supported chips. This
> approach is also less likely to break the current code.
> 
> As a side note, I think it's rather weird that the IT8712F rev. J (and
> later) do default to 8-bit fan speed values while they no longer
> support fan clock dividers. As these revisions of the chip are not
> backward compatible with the older revisions, ITE should really have
> switched to a saner default.
> (...)

Any update for this patch, addressing the points I raised in my review?
Apparently we have another user who needs this (Cc'd.)

-- 
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: it87 support for 16-bit fan reading

[Andrew Paprocki]

On Thu, Jun 12, 2008 at 3:53 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Andrew,
>
> On Sun, 24 Feb 2008 16:43:35 +0100, Jean Delvare wrote:
>> On Mon, 11 Feb 2008 01:32:59 -0500, Andrew Paprocki wrote:
>> > This splits the it8712 chip type into two chip types to distinguish the
>> > changes made in rev 8 of the chip. A new type it8712old represents all
>> > revs prior to rev 8. The it8712 chip type now represents rev 8 and
>> > greater.
>>
>> I am not fond of the "it8712old" name, because "old" doesn't tell the
>> reader in which way it is different from the (new) it8712 type. It also
>> doesn't "scale" well... What will you do if the next IT8712F revision
>> differs from rev. K in a new way?
>>
>> I would prefer a separate field in struct it87_sio_data and struct
>> it87_data, recording the revision value. This is reusable for future
>> differences between the various revisions of all supported chips. This
>> approach is also less likely to break the current code.
>>
>> As a side note, I think it's rather weird that the IT8712F rev. J (and
>> later) do default to 8-bit fan speed values while they no longer
>> support fan clock dividers. As these revisions of the chip are not
>> backward compatible with the older revisions, ITE should really have
>> switched to a saner default.
>> (...)
>
> Any update for this patch, addressing the points I raised in my review?
> Apparently we have another user who needs this (Cc'd.)

Sorry for the delay. I got sidetracked and haven't booted up this
hardware in a little while. I'll try to revisit this week and re-post.

-Andrew

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

Re: [lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading

[Andrew Paprocki]

[-- Attachment #1: Type: text/plain, Size: 2996 bytes --]

On Sun, Feb 24, 2008 at 11:43 AM, Jean Delvare <khali@linux-fr.org> wrote:

> As a side note, I think it's rather weird that the IT8712F rev. J (and
> later) do default to 8-bit fan speed values while they no longer
> support fan clock dividers. As these revisions of the chip are not
> backward compatible with the older revisions, ITE should really have
> switched to a saner default.
>

Yes, strange.. see below.

>> The it8712 chip type now enables 16-bit fan tachometer reading to get
>> proper readings. The register used in the it8712old chip type no longer
>> has the same meaning in rev 8 of the chip.
>
> Which register are you talking about please?

If you compare the PDF documents here:
http://www.ite.com.tw/product_info/PC/Brief-IT8712_2.asp

v0.8.2 shows 9.6.2.2.12 Fan Tachometer Divisor Register (Index=0Bh,
Default=09h) using bits 0-6 for the divisors. This version corresponds
to revisions of the chip < 0x8.

v0.9.1 & v0.9.3 show the same register having bits 0-5 reserved and
6-7 are the PWM Smoothing Step Frequency Selection. This version
corresponds to revisions of the chip >= 0x8.

Since those bit ranges overlap, I don't see how they could be
preserving the "old" behavior. They could have done that if bits 0-6
were reserved, but the new meaning of bits 6-7 overlaps the old FAN3
divisor. Strangely, both revisions say they have a default value of
09h. (Documentation error?)

>> Also, a new module param array enable_fans is added. This was needed to
>> get fan2 to show up on an it8712 rev 8 chip which was not enabled at
>> boot time by the BIOS. The module param is checked and if a fan index
>> value is 1, the fan is forced on even if it is not configured that way
>> from the BIOS.
>
> This is a BIOS bug, which should be fixed in the BIOS. Please report to
> your motherboard manufacturer and get them to fix the bug. What
> motherboard is this, BTW? Usually motherboard BIOS enable all fans, or
> none; I don't remember seeing a BIOS (improperly) enabling a subset
> only.
>

I've reported this to the motherboard manufacturer and I'm waiting to
hear back if they confirm that it is a BIOS setup issue on their side.
The board in question is the Albatron KI 690-AM2 motherboard. This
board has a 4-pin & a 3-pin fan header. The main fan control register
has a value of 0x11 which indicates that only FAN1 is active (in
SmartGuardian mode). If I manually hack the driver to always set bit 5
of the register (9.6.2.2.16 Fan Controller Main Control Register
(Index=13h, Default=07h) in the docs), then I get a valid fan reading
from my CPU fan on "fan2" plugged into the 3-pin header. Short of a
BIOS update to fix this, I'd have to hack it to allow me to force
enable the second fan to get a valid reading.

I've reworked the code to just add a revision field as you noted and
to only contain the 16-bit tach change. Please review and let me know
what you think re: force enabling the fans via a module parameter
bitmask or something similar.

Thanks,
-Andrew

[-- Attachment #2: 0001-hwmon-it87-support-for-16-bit-fan-reading-in-it8712-rev-8.txt --]
[-- Type: text/plain, Size: 4694 bytes --]

From fb4786e32ac13a425ff4a423aaf8fb5a258102e2 Mon Sep 17 00:00:00 2001
From: Andrew Paprocki <andrew@ishiboo.com>
Date: Tue, 24 Jun 2008 08:30:59 -0400
Subject: [PATCH] hwmon: it87 support for 16-bit fan reading in it8712 >= rev 8

The it8712 chip does not use fan divisors in revisions >= 8.
The newer version of the chip uses 16-bit fan tachometers just
like the it8716 and it8718 chips.

Signed-off-by: Andrew Paprocki <andrew@ishiboo.com>
---
 drivers/hwmon/it87.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index e12c132..4017fee 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -151,9 +151,9 @@ static int fix_pwm_polarity;
 /* The IT8718F has the VID value in a different register, in Super-I/O
    configuration space. */
 #define IT87_REG_VID           0x0a
-/* Warning: register 0x0b is used for something completely different in
-   new chips/revisions. I suspect only 16-bit tachometer mode will work
-   for these. */
+/* The IT8705F and IT8712F earlier than revision 0x08 use register 0x0b
+   for fan divisors. Later IT8712F revisions ust use 16-bit tachometer
+   mode. */
 #define IT87_REG_FAN_DIV       0x0b
 #define IT87_REG_FAN_16BIT     0x0c
 
@@ -234,6 +234,7 @@ static const unsigned int pwm_freq[8] = {
 struct it87_sio_data {
 	enum chips type;
 	/* Values read from Super-I/O config space */
+	u8 revision;
 	u8 vid_value;
 };
 
@@ -242,6 +243,7 @@ struct it87_sio_data {
 struct it87_data {
 	struct device *hwmon_dev;
 	enum chips type;
+	u8 revision;
 
 	unsigned short addr;
 	const char *name;
@@ -991,8 +993,9 @@ static int __init it87_find(unsigned short *address,
 	}
 
 	err = 0;
+	sio_data->revision = superio_inb(DEVREV) & 0x0f;
 	pr_info("it87: Found IT%04xF chip at 0x%x, revision %d\n",
-		chip_type, *address, superio_inb(DEVREV) & 0x0f);
+		chip_type, *address, sio_data->revision);
 
 	/* Read GPIO config and VID value from LDN 7 (GPIO) */
 	if (chip_type != IT8705F_DEVID) {
@@ -1045,6 +1048,7 @@ static int __devinit it87_probe(struct platform_device *pdev)
 
 	data->addr = res->start;
 	data->type = sio_data->type;
+	data->revision = sio_data->revision;
 	data->name = names[sio_data->type];
 
 	/* Now, we do the remaining detection. */
@@ -1069,7 +1073,8 @@ static int __devinit it87_probe(struct platform_device *pdev)
 		goto ERROR2;
 
 	/* Do not create fan files for disabled fans */
-	if (data->type == it8716 || data->type == it8718) {
+	if ((data->type == it8712 && data->revision >= 0x08)
+	 || data->type == it8716 || data->type == it8718) {
 		/* 16-bit tachometers */
 		if (data->has_fan & (1 << 0)) {
 			if ((err = device_create_file(dev,
@@ -1350,7 +1355,8 @@ static void __devinit it87_init_device(struct platform_device *pdev)
 	data->has_fan = (data->fan_main_ctrl >> 4) & 0x07;
 
 	/* Set tachometers to 16-bit mode if needed */
-	if (data->type == it8716 || data->type == it8718) {
+	if ((data->type == it8712 && data->revision >= 0x08)
+	 || data->type == it8716 || data->type == it8718) {
 		tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
 		if (~tmp & 0x07 & data->has_fan) {
 			dev_dbg(&pdev->dev,
@@ -1426,7 +1432,8 @@ static struct it87_data *it87_update_device(struct device *dev)
 			data->fan[i] = it87_read_value(data,
 				       IT87_REG_FAN[i]);
 			/* Add high byte if in 16-bit mode */
-			if (data->type == it8716 || data->type == it8718) {
+			if ((data->type == it8712 && data->revision >= 0x08)
+			 || data->type == it8716 || data->type == it8718) {
 				data->fan[i] |= it87_read_value(data,
 						IT87_REG_FANX[i]) << 8;
 				data->fan_min[i] |= it87_read_value(data,
@@ -1443,8 +1450,9 @@ static struct it87_data *it87_update_device(struct device *dev)
 		}
 
 		/* Newer chips don't have clock dividers */
-		if ((data->has_fan & 0x07) && data->type != it8716
-		 && data->type != it8718) {
+		if ((data->has_fan & 0x07)
+		 && (data->type == it87
+		  || (data->type == it8712 && data->revision < 0x08))) {
 			i = it87_read_value(data, IT87_REG_FAN_DIV);
 			data->fan_div[0] = i & 0x07;
 			data->fan_div[1] = (i >> 3) & 0x07;
@@ -1460,7 +1468,8 @@ static struct it87_data *it87_update_device(struct device *dev)
 		data->fan_ctl = it87_read_value(data, IT87_REG_FAN_CTL);
 
 		data->sensor = it87_read_value(data, IT87_REG_TEMP_ENABLE);
-		/* The 8705 does not have VID capability */
+		/* The 8705 does not have VID capability.
+		   The 8718 does not use IT87_REG_VID for the same purpose. */
 		if (data->type == it8712 || data->type == it8716) {
 			data->vid = it87_read_value(data, IT87_REG_VID);
 			/* The older IT8712F revisions had only 5 VID pins,
-- 
1.4.4.4


[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading

[Bruno Prémont]

On Tue, 24 June 2008 "Andrew Paprocki" <andrew@ishiboo.com> wrote:
> I've reworked the code to just add a revision field as you noted and
> to only contain the 16-bit tach change. Please review and let me know
> what you think re: force enabling the fans via a module parameter
> bitmask or something similar.
> 
> Thanks,
> -Andrew  

I've run if for about 20 minutes on my system (Kino 690S1, IT8712F-S)
and FAN speed readings look fine (around 3200 RPM (with PWM control) for
CPU fan and 1500 for Artic Cooling self-regulated FAN)
The divisor files don't show up in sysfs anymore (as expected).

Andrew, do you have the links to docs for pre-8 revisions at
hand? The link on ITE homepage for datasheet revision 0.8.2 returns a
404 error...
I would like to check if there is any change regarding voltage
inputs as for change to FAN tacho register width. The voltage readings
reported by the driver look pretty improbable!

it8712-isa-0e80
Adapter: ISA adapter
VCore 1:     +1.17 V  (min =  +0.00 V, max =  +4.08 V)
VCore 2:     +0.82 V  (min =  +0.00 V, max =  +4.08 V)
+3.3V:       +3.01 V  (min =  +0.00 V, max =  +4.08 V)
+5V:         +4.92 V  (min =  +0.00 V, max =  +6.85 V)
+12V:        +4.99 V  (min =  +0.00 V, max = +16.32 V)
-12V:       -13.86 V  (min = -27.36 V, max =  +3.93 V)
-5V:         -3.11 V  (min = -13.64 V, max =  +4.03 V)
Stdby:       +4.95 V  (min =  +0.00 V, max =  +6.85 V)
VBat:        +3.26 V
fan1:       3183 RPM  (min =    0 RPM, div = 8)
fan2:       1520 RPM  (min =    0 RPM, div = 8)
CPU Temp:    +20.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = thermal diode
M/B Temp:    +48.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = thermal diode
cpu0_vid:   +1.550 V

Bruno

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

Re: [lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading

[Jean Delvare]

Hi Bruno,

On Sat, 5 Jul 2008 14:40:03 +0200, Bruno Prémont wrote:
> On Tue, 24 June 2008 "Andrew Paprocki" <andrew@ishiboo.com> wrote:
> > I've reworked the code to just add a revision field as you noted and
> > to only contain the 16-bit tach change. Please review and let me know
> > what you think re: force enabling the fans via a module parameter
> > bitmask or something similar.
> 
> I've run if for about 20 minutes on my system (Kino 690S1, IT8712F-S)
> and FAN speed readings look fine (around 3200 RPM (with PWM control) for
> CPU fan and 1500 for Artic Cooling self-regulated FAN)
> The divisor files don't show up in sysfs anymore (as expected).
> 
> Andrew, do you have the links to docs for pre-8 revisions at
> hand? The link on ITE homepage for datasheet revision 0.8.2 returns a
> 404 error...

I think I have all the old IT8712 datasheets, I'll send revision 0.8.2
to you (in private) if that's what you need.

> I would like to check if there is any change regarding voltage
> inputs as for change to FAN tacho register width. The voltage readings
> reported by the driver look pretty improbable!
> 
> it8712-isa-0e80
> Adapter: ISA adapter
> VCore 1:     +1.17 V  (min =  +0.00 V, max =  +4.08 V)
> VCore 2:     +0.82 V  (min =  +0.00 V, max =  +4.08 V)
> +3.3V:       +3.01 V  (min =  +0.00 V, max =  +4.08 V)
> +5V:         +4.92 V  (min =  +0.00 V, max =  +6.85 V)
> +12V:        +4.99 V  (min =  +0.00 V, max = +16.32 V)
> -12V:       -13.86 V  (min = -27.36 V, max =  +3.93 V)
> -5V:         -3.11 V  (min = -13.64 V, max =  +4.03 V)
> Stdby:       +4.95 V  (min =  +0.00 V, max =  +6.85 V)
> VBat:        +3.26 V
> fan1:       3183 RPM  (min =    0 RPM, div = 8)
> fan2:       1520 RPM  (min =    0 RPM, div = 8)
> CPU Temp:    +20.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = thermal diode
> M/B Temp:    +48.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = thermal diode
> cpu0_vid:   +1.550 V

I don't remember any change in the voltage computations. More likely
you simply need to tweak your configuration file to match your
motherboard wiring, as is the case with all motherboards. This includes
changing the labels and adjusting the compute statements as well. We
can help you a bit with this if you tell us all the voltage labels and
values displayed by your BIOS, and the voltages values reported by
"sensors -c /dev/null".

-- 
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: it87 support for 16-bit fan reading

[Bruno Prémont]

On Sat, 05 July 2008 Jean Delvare <khali@linux-fr.org> wrote:
> I don't remember any change in the voltage computations. More likely
> you simply need to tweak your configuration file to match your
> motherboard wiring, as is the case with all motherboards. This
> includes changing the labels and adjusting the compute statements as
> well. We can help you a bit with this if you tell us all the voltage
> labels and values displayed by your BIOS, and the voltages values
> reported by "sensors -c /dev/null".

Raw sensors readings:

  it8712-isa-0e80
  Adapter: ISA adapter
  in0:         +1.17 V  (min =  +0.00 V, max =  +4.08 V)
  in1:         +0.82 V  (min =  +0.00 V, max =  +4.08 V)
  in2:         +3.01 V  (min =  +0.00 V, max =  +4.08 V)
  in3:         +2.93 V  (min =  +0.00 V, max =  +4.08 V)
  in4:         +1.25 V  (min =  +0.00 V, max =  +4.08 V)
  in5:         +1.76 V  (min =  +0.00 V, max =  +4.08 V)
  in6:         +1.60 V  (min =  +0.00 V, max =  +4.08 V)
  in7:         +2.94 V  (min =  +0.00 V, max =  +4.08 V)
  in8:         +3.26 V
  fan1:       3245 RPM  (min =    0 RPM, div = 8)
  fan2:       1520 RPM  (min =    0 RPM, div = 8)
  temp1:       +20.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = thermal diode
  temp2:       +48.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = thermal diode
  temp3:       +25.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = transistor
  cpu0_vid:   +1.550 V

Bios readings:

  CPU Temp: 33°C
  System temp: 45°C
  CPU FAN: 5769 RPM
  Sys FAN: 1496 RPM

  CPU Core: 1.1V
  +1.2V:   1.168V
  +3.3V:   3.008V
  +5V:     4.992V
  +1.8V:   1.76V

Looking at those values there could be:
  in0 = +1.2V
  in2 = +3.3V
  in5 = +1.8V

For the other voltages which do seem to be wired somehow even tough far less
are mentionned in BIOS monitoring section it's hard to tell what they could
match...


For temperatures:
- temp1 is related to CPU temp (no idea what's the right formula, looks
  like combination of offset and multiplier when compared to value
  reported by k8temp-pci-00c3)
- temp2 is system temp
- temp3 is constant at 25°C and probably not wired at all

Bruno

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

Re: [lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading

[Jean Delvare]

On Sat, 5 Jul 2008 16:22:45 +0200, Bruno Prémont wrote:
> On Sat, 05 July 2008 Jean Delvare <khali@linux-fr.org> wrote:
> > I don't remember any change in the voltage computations. More likely
> > you simply need to tweak your configuration file to match your
> > motherboard wiring, as is the case with all motherboards. This
> > includes changing the labels and adjusting the compute statements as
> > well. We can help you a bit with this if you tell us all the voltage
> > labels and values displayed by your BIOS, and the voltages values
> > reported by "sensors -c /dev/null".
> 
> Raw sensors readings:
> 
>   it8712-isa-0e80
>   Adapter: ISA adapter
>   in0:         +1.17 V  (min =  +0.00 V, max =  +4.08 V)
>   in1:         +0.82 V  (min =  +0.00 V, max =  +4.08 V)
>   in2:         +3.01 V  (min =  +0.00 V, max =  +4.08 V)
>   in3:         +2.93 V  (min =  +0.00 V, max =  +4.08 V)
>   in4:         +1.25 V  (min =  +0.00 V, max =  +4.08 V)
>   in5:         +1.76 V  (min =  +0.00 V, max =  +4.08 V)
>   in6:         +1.60 V  (min =  +0.00 V, max =  +4.08 V)
>   in7:         +2.94 V  (min =  +0.00 V, max =  +4.08 V)
>   in8:         +3.26 V
>   fan1:       3245 RPM  (min =    0 RPM, div = 8)
>   fan2:       1520 RPM  (min =    0 RPM, div = 8)
>   temp1:       +20.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = thermal diode
>   temp2:       +48.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = thermal diode
>   temp3:       +25.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor = transistor
>   cpu0_vid:   +1.550 V
> 
> Bios readings:
> 
>   CPU Temp: 33°C
>   System temp: 45°C
>   CPU FAN: 5769 RPM
>   Sys FAN: 1496 RPM
> 
>   CPU Core: 1.1V
>   +1.2V:   1.168V
>   +3.3V:   3.008V
>   +5V:     4.992V
>   +1.8V:   1.76V
> 
> Looking at those values there could be:
>   in0 = +1.2V

Unlikely. Vcore is almost always in0, and it seems to be the case
here. +1.2V would rather be in4.

>   in2 = +3.3V
>   in5 = +1.8V
> 
> For the other voltages which do seem to be wired somehow even tough far less
> are mentionned in BIOS monitoring section it's hard to tell what they could
> match...

The IT8712F can only measure voltages up to 4.08V directly. Other
voltages need scaling resistors. The nominal voltage after scaling is
typically 3V (3/4 of the full scale). in3 and in7 are almost 3V so they
are most probably voltages > 4V scaled down. I would guess in3 is +5V
and in7 maybe 5VSB. Now you have to figure out the scaling factor.
4.992/2.93 = 1.703. Typical factors for +5V are 1.666 and 1.68, the
later is the standard for the IT8712F so I'd say that's what you have.

in8 is normally the battery voltage, unscaled.

So my guess for the configuration of your board would be:

   label in0 "Vcore"
   label in2 "+3.3V"
   label in3 "+5V"
   label in4 "+1.2V"
   label in5 "+1.8V"
   label in7 "5VSB"
   label in8 "Vbat"

   compute in3 ((6.8/10)+1)*@ ,  @/((6.8/10)+1)
   compute in8 ((6.8/10)+1)*@ ,  @/((6.8/10)+1)

It's a bit strange that +12V isn't monitored. And this leaves in1 and
in6 without labels. Maybe they aren't wired at all. Ideally you would
ask your motherboard manufacturer and they would tell you. The above is
just guesswork based on my personal experience, I might as well be
plain wrong ;)

> For temperatures:
> - temp1 is related to CPU temp (no idea what's the right formula, looks
>   like combination of offset and multiplier when compared to value
>   reported by k8temp-pci-00c3)
> - temp2 is system temp
> - temp3 is constant at 25°C and probably not wired at all

Indeed, 25°C constant for a thermistor usually means that there is no
thermistor but a dumb resistor instead. I've seen this many times...

-- 
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: it87 support for 16-bit fan reading

[Bruno Prémont]

On Sat, 05 July 2008 Jean Delvare <khali@linux-fr.org> wrote:
> On Sat, 5 Jul 2008 16:22:45 +0200, Bruno Prémont wrote:
> > On Sat, 05 July 2008 Jean Delvare <khali@linux-fr.org> wrote:
> > > I don't remember any change in the voltage computations. More
> > > likely you simply need to tweak your configuration file to match
> > > your motherboard wiring, as is the case with all motherboards.
> > > This includes changing the labels and adjusting the compute
> > > statements as well. We can help you a bit with this if you tell
> > > us all the voltage labels and values displayed by your BIOS, and
> > > the voltages values reported by "sensors -c /dev/null".
> > 
> > Raw sensors readings:
> > 
> >   it8712-isa-0e80
> >   Adapter: ISA adapter
> >   in0:         +1.17 V  (min =  +0.00 V, max =  +4.08 V)
> >   in1:         +0.82 V  (min =  +0.00 V, max =  +4.08 V)
> >   in2:         +3.01 V  (min =  +0.00 V, max =  +4.08 V)
> >   in3:         +2.93 V  (min =  +0.00 V, max =  +4.08 V)
> >   in4:         +1.25 V  (min =  +0.00 V, max =  +4.08 V)
> >   in5:         +1.76 V  (min =  +0.00 V, max =  +4.08 V)
> >   in6:         +1.60 V  (min =  +0.00 V, max =  +4.08 V)
> >   in7:         +2.94 V  (min =  +0.00 V, max =  +4.08 V)
> >   in8:         +3.26 V
> >   fan1:       3245 RPM  (min =    0 RPM, div = 8)
> >   fan2:       1520 RPM  (min =    0 RPM, div = 8)
> >   temp1:       +20.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor > > thermal diode temp2:       +48.0°C  (low  =  -1.0°C, high > > +127.0°C)  sensor = thermal diode temp3:       +25.0°C  (low  > > -1.0°C, high = +127.0°C)  sensor = transistor cpu0_vid:   +1.550 V
> > 
> > Bios readings:
> > 
> >   CPU Temp: 33°C
> >   System temp: 45°C
> >   CPU FAN: 5769 RPM
> >   Sys FAN: 1496 RPM
> > 
> >   CPU Core: 1.1V
> >   +1.2V:   1.168V
> >   +3.3V:   3.008V
> >   +5V:     4.992V
> >   +1.8V:   1.76V
> > 
> > Looking at those values there could be:
> >   in0 = +1.2V
> 
> Unlikely. Vcore is almost always in0, and it seems to be the case
> here. +1.2V would rather be in4.
> 
> >   in2 = +3.3V
> >   in5 = +1.8V
> > 
> > For the other voltages which do seem to be wired somehow even tough
> > far less are mentionned in BIOS monitoring section it's hard to
> > tell what they could match...
> 
> The IT8712F can only measure voltages up to 4.08V directly. Other
> voltages need scaling resistors. The nominal voltage after scaling is
> typically 3V (3/4 of the full scale). in3 and in7 are almost 3V so
> they are most probably voltages > 4V scaled down. I would guess in3
> is +5V and in7 maybe 5VSB. Now you have to figure out the scaling
> factor. 4.992/2.93 = 1.703. Typical factors for +5V are 1.666 and
> 1.68, the later is the standard for the IT8712F so I'd say that's
> what you have.
> 
> in8 is normally the battery voltage, unscaled.
> 
> So my guess for the configuration of your board would be:
> 
>    label in0 "Vcore"
>    label in2 "+3.3V"
>    label in3 "+5V"
>    label in4 "+1.2V"
>    label in5 "+1.8V"
>    label in7 "5VSB"
>    label in8 "Vbat"
> 
>    compute in3 ((6.8/10)+1)*@ ,  @/((6.8/10)+1)
>    compute in8 ((6.8/10)+1)*@ ,  @/((6.8/10)+1)
for second compute you were thinking in7

Why are the compute lines that complex for just multiplying/dividing
with 1.68? Is this a workaround for floating-point precision issues?

> It's a bit strange that +12V isn't monitored. And this leaves in1 and
> in6 without labels. Maybe they aren't wired at all. Ideally you would
> ask your motherboard manufacturer and they would tell you. The above
> is just guesswork based on my personal experience, I might as well be
> plain wrong ;)
I was also surprised that there was no 12V listed in the BIOS! This
especially as the board has standard ATX connector and also has at
least FANs to feed with 12V (and probably also PCI and PCI-Express)

I will try to get that info from IEI though their account system does
not seem to work very well (or I'm not using the right browser for it)
Alternatively I can try to get the info through the reseller through
whom I got the mainboard.

> > For temperatures:
> > - temp1 is related to CPU temp (no idea what's the right formula,
> > looks like combination of offset and multiplier when compared to
> > value reported by k8temp-pci-00c3)
> > - temp2 is system temp
> > - temp3 is constant at 25°C and probably not wired at all
> 
> Indeed, 25°C constant for a thermistor usually means that there is no
> thermistor but a dumb resistor instead. I've seen this many times...

Bruno

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

Re: [lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading

[Jean Delvare]

On Sat, 5 Jul 2008 17:49:23 +0200, Bruno Prémont wrote:
> On Sat, 05 July 2008 Jean Delvare <khali@linux-fr.org> wrote:
> > On Sat, 5 Jul 2008 16:22:45 +0200, Bruno Prémont wrote:
> > > Raw sensors readings:
> > > 
> > >   it8712-isa-0e80
> > >   Adapter: ISA adapter
> > >   in0:         +1.17 V  (min =  +0.00 V, max =  +4.08 V)
> > >   in1:         +0.82 V  (min =  +0.00 V, max =  +4.08 V)
> > >   in2:         +3.01 V  (min =  +0.00 V, max =  +4.08 V)
> > >   in3:         +2.93 V  (min =  +0.00 V, max =  +4.08 V)
> > >   in4:         +1.25 V  (min =  +0.00 V, max =  +4.08 V)
> > >   in5:         +1.76 V  (min =  +0.00 V, max =  +4.08 V)
> > >   in6:         +1.60 V  (min =  +0.00 V, max =  +4.08 V)
> > >   in7:         +2.94 V  (min =  +0.00 V, max =  +4.08 V)
> > >   in8:         +3.26 V
> > >   fan1:       3245 RPM  (min =    0 RPM, div = 8)
> > >   fan2:       1520 RPM  (min =    0 RPM, div = 8)
> > >   temp1:       +20.0°C  (low  =  -1.0°C, high = +127.0°C)  sensor > > > thermal diode temp2:       +48.0°C  (low  =  -1.0°C, high > > > +127.0°C)  sensor = thermal diode temp3:       +25.0°C  (low  > > > -1.0°C, high = +127.0°C)  sensor = transistor cpu0_vid:   +1.550 V
> > > 
> > > Bios readings:
> > > 
> > >   CPU Temp: 33°C
> > >   System temp: 45°C
> > >   CPU FAN: 5769 RPM
> > >   Sys FAN: 1496 RPM
> > > 
> > >   CPU Core: 1.1V
> > >   +1.2V:   1.168V
> > >   +3.3V:   3.008V
> > >   +5V:     4.992V
> > >   +1.8V:   1.76V
> > > 
> > > Looking at those values there could be:
> > >   in0 = +1.2V
> > 
> > Unlikely. Vcore is almost always in0, and it seems to be the case
> > here. +1.2V would rather be in4.
> > 
> > >   in2 = +3.3V
> > >   in5 = +1.8V
> > > 
> > > For the other voltages which do seem to be wired somehow even tough
> > > far less are mentionned in BIOS monitoring section it's hard to
> > > tell what they could match...
> > 
> > The IT8712F can only measure voltages up to 4.08V directly. Other
> > voltages need scaling resistors. The nominal voltage after scaling is
> > typically 3V (3/4 of the full scale). in3 and in7 are almost 3V so
> > they are most probably voltages > 4V scaled down. I would guess in3
> > is +5V and in7 maybe 5VSB. Now you have to figure out the scaling
> > factor. 4.992/2.93 = 1.703. Typical factors for +5V are 1.666 and
> > 1.68, the later is the standard for the IT8712F so I'd say that's
> > what you have.
> > 
> > in8 is normally the battery voltage, unscaled.
> > 
> > So my guess for the configuration of your board would be:
> > 
> >    label in0 "Vcore"
> >    label in2 "+3.3V"
> >    label in3 "+5V"
> >    label in4 "+1.2V"
> >    label in5 "+1.8V"
> >    label in7 "5VSB"
> >    label in8 "Vbat"
> > 
> >    compute in3 ((6.8/10)+1)*@ ,  @/((6.8/10)+1)
> >    compute in8 ((6.8/10)+1)*@ ,  @/((6.8/10)+1)
> for second compute you were thinking in7

Oops, yes, of course.

> Why are the compute lines that complex for just multiplying/dividing
> with 1.68? Is this a workaround for floating-point precision issues?

No, libsensors deals with decimals just fine. The above is simply
matching the physical reality: the voltage is scaled down using
resistors of 10 kOhm and 6.8 kOhm. You can write it 1.68 if you prefer,
that doesn't make any difference.

-- 
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: it87 support for 16-bit fan reading

[Jean Delvare]

Hi Andrew,

On Tue, 24 Jun 2008 09:16:30 -0400, Andrew Paprocki wrote:
> On Sun, Feb 24, 2008 at 11:43 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > Which register are you talking about please?
> 
> If you compare the PDF documents here:
> http://www.ite.com.tw/product_info/PC/Brief-IT8712_2.asp
> 
> v0.8.2 shows 9.6.2.2.12 Fan Tachometer Divisor Register (Index\vh,
> Default	h) using bits 0-6 for the divisors. This version corresponds
> to revisions of the chip < 0x8.
> 
> v0.9.1 & v0.9.3 show the same register having bits 0-5 reserved and
> 6-7 are the PWM Smoothing Step Frequency Selection. This version
> corresponds to revisions of the chip >= 0x8.

Correct.

> Since those bit ranges overlap, I don't see how they could be
> preserving the "old" behavior. They could have done that if bits 0-6
> were reserved, but the new meaning of bits 6-7 overlaps the old FAN3
> divisor. Strangely, both revisions say they have a default value of
> 09h. (Documentation error?)

I agree it's all strange. I don't have a revision J or later chip so I
can't test. Someone with such a chip (you?) could attempt to reset it
(write 1 to bit 7 of register 00h) and check what the default value
of register 0Bh actually is.

If the default value is really 09h, one possibility is that bits 0-5 of
register 0Bh still control fan clock dividers for fan1 and fan2, even
though they are no longer needed and this is not documented. Again,
only someone with such a chip could test that. Or maybe value 09h is
read only and just meant to express the fact that the fan clock divider
is always 2 in 16-bit mode. But OTOH we don't really care, what really
matters is to add 16-bit fan speed support for the chips that need
this, just as your patch does.

> > This is a BIOS bug, which should be fixed in the BIOS. Please report to
> > your motherboard manufacturer and get them to fix the bug. What
> > motherboard is this, BTW? Usually motherboard BIOS enable all fans, or
> > none; I don't remember seeing a BIOS (improperly) enabling a subset
> > only.
> 
> I've reported this to the motherboard manufacturer and I'm waiting to
> hear back if they confirm that it is a BIOS setup issue on their side.
> The board in question is the Albatron KI 690-AM2 motherboard. This
> board has a 4-pin & a 3-pin fan header. The main fan control register
> has a value of 0x11 which indicates that only FAN1 is active (in
> SmartGuardian mode). If I manually hack the driver to always set bit 5
> of the register (9.6.2.2.16 Fan Controller Main Control Register
> (Index\x13h, Default\ah) in the docs), then I get a valid fan reading
> from my CPU fan on "fan2" plugged into the 3-pin header. Short of a
> BIOS update to fix this, I'd have to hack it to allow me to force
> enable the second fan to get a valid reading.
> 
> I've reworked the code to just add a revision field as you noted and
> to only contain the 16-bit tach change. Please review and let me know
> what you think re: force enabling the fans via a module parameter
> bitmask or something similar.

Maybe I have been too optimistic when assuming that we could always
trust the BIOS on either leaving the fans alone (in which case the
driver enables all inputs) or enabling all the ones the board uses.
After all, they have no reason to enable fan inputs for which they don't
display a value themselves. I noticed that this happens for my own
motherboard, which has an IT8705F chip. The board has 3 fans headers,
but only fan1 and fan2 are enabled (and displayed) by the BIOS. Maybe a
BIOS upgrade would help, but my attempt at this, failed.

So maybe we should simply always enable all of fan1, fan2 and fan3,
regardless of what the BIOS did. That's a one-line change as far as I
can see, fairly simple.

Review of your patch:

> Subject: [PATCH] hwmon: it87 support for 16-bit fan reading in it8712 >= rev 8
> 
> The it8712 chip does not use fan divisors in revisions >= 8.
> The newer version of the chip uses 16-bit fan tachometers just
> like the it8716 and it8718 chips.

Overall it looks fine, with minor comments:

> Signed-off-by: Andrew Paprocki <andrew@ishiboo.com>
> ---
>  drivers/hwmon/it87.c |   29 +++++++++++++++++++----------
>  1 files changed, 19 insertions(+), 10 deletions(-)

You also need to update Documentation/hwmon/it87, which currently
claims that "the driver only uses the 16-bit mode on the IT8716F and
IT8718F". Likewise, the documentation says that fan4 and fan5 are not
supported for the IT8712F, while your patch enables this for
revisions >= J as far as I can see. This makes me wonder why your patch
doesn't also cover revision I, as it already had 16-bit fan speed
registers and the 2 extra fans as well.

> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index e12c132..4017fee 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -151,9 +151,9 @@ static int fix_pwm_polarity;
>  /* The IT8718F has the VID value in a different register, in Super-I/O
>     configuration space. */
>  #define IT87_REG_VID           0x0a
> -/* Warning: register 0x0b is used for something completely different in
> -   new chips/revisions. I suspect only 16-bit tachometer mode will work
> -   for these. */
> +/* The IT8705F and IT8712F earlier than revision 0x08 use register 0x0b
> +   for fan divisors. Later IT8712F revisions ust use 16-bit tachometer

Typo: must.

> +   mode. */
>  #define IT87_REG_FAN_DIV       0x0b
>  #define IT87_REG_FAN_16BIT     0x0c
>  
> @@ -234,6 +234,7 @@ static const unsigned int pwm_freq[8] = {
>  struct it87_sio_data {
>  	enum chips type;
>  	/* Values read from Super-I/O config space */
> +	u8 revision;
>  	u8 vid_value;
>  };
>  
> @@ -242,6 +243,7 @@ struct it87_sio_data {
>  struct it87_data {
>  	struct device *hwmon_dev;
>  	enum chips type;
> +	u8 revision;
>  
>  	unsigned short addr;
>  	const char *name;
> @@ -991,8 +993,9 @@ static int __init it87_find(unsigned short *address,
>  	}
>  
>  	err = 0;
> +	sio_data->revision = superio_inb(DEVREV) & 0x0f;
>  	pr_info("it87: Found IT%04xF chip at 0x%x, revision %d\n",
> -		chip_type, *address, superio_inb(DEVREV) & 0x0f);
> +		chip_type, *address, sio_data->revision);
>  
>  	/* Read GPIO config and VID value from LDN 7 (GPIO) */
>  	if (chip_type != IT8705F_DEVID) {
> @@ -1045,6 +1048,7 @@ static int __devinit it87_probe(struct platform_device *pdev)
>  
>  	data->addr = res->start;
>  	data->type = sio_data->type;
> +	data->revision = sio_data->revision;
>  	data->name = names[sio_data->type];
>  
>  	/* Now, we do the remaining detection. */
> @@ -1069,7 +1073,8 @@ static int __devinit it87_probe(struct platform_device *pdev)
>  		goto ERROR2;
>  
>  	/* Do not create fan files for disabled fans */
> -	if (data->type = it8716 || data->type = it8718) {
> +	if ((data->type = it8712 && data->revision >= 0x08)
> +	 || data->type = it8716 || data->type = it8718) {

It might be convenient to introduce the following inline helper
function:

static inline int has_16bit_fans(const struct *it87_data)
{
	return (data->type = it8712 && data->revision >= 0x08)
	     || data->type = it8716
	     || data->type = it8718;
}

So you can call it each time you need to differenciate between chips
that have 16-bit fans and chips that do not.

>  		/* 16-bit tachometers */
>  		if (data->has_fan & (1 << 0)) {
>  			if ((err = device_create_file(dev,
> @@ -1350,7 +1355,8 @@ static void __devinit it87_init_device(struct platform_device *pdev)
>  	data->has_fan = (data->fan_main_ctrl >> 4) & 0x07;
>  
>  	/* Set tachometers to 16-bit mode if needed */
> -	if (data->type = it8716 || data->type = it8718) {
> +	if ((data->type = it8712 && data->revision >= 0x08)
> +	 || data->type = it8716 || data->type = it8718) {
>  		tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
>  		if (~tmp & 0x07 & data->has_fan) {
>  			dev_dbg(&pdev->dev,
> @@ -1426,7 +1432,8 @@ static struct it87_data *it87_update_device(struct device *dev)
>  			data->fan[i] = it87_read_value(data,
>  				       IT87_REG_FAN[i]);
>  			/* Add high byte if in 16-bit mode */
> -			if (data->type = it8716 || data->type = it8718) {
> +			if ((data->type = it8712 && data->revision >= 0x08)
> +			 || data->type = it8716 || data->type = it8718) {
>  				data->fan[i] |= it87_read_value(data,
>  						IT87_REG_FANX[i]) << 8;
>  				data->fan_min[i] |= it87_read_value(data,
> @@ -1443,8 +1450,9 @@ static struct it87_data *it87_update_device(struct device *dev)
>  		}
>  
>  		/* Newer chips don't have clock dividers */
> -		if ((data->has_fan & 0x07) && data->type != it8716
> -		 && data->type != it8718) {
> +		if ((data->has_fan & 0x07)
> +		 && (data->type = it87
> +		  || (data->type = it8712 && data->revision < 0x08))) {
>  			i = it87_read_value(data, IT87_REG_FAN_DIV);
>  			data->fan_div[0] = i & 0x07;
>  			data->fan_div[1] = (i >> 3) & 0x07;
> @@ -1460,7 +1468,8 @@ static struct it87_data *it87_update_device(struct device *dev)
>  		data->fan_ctl = it87_read_value(data, IT87_REG_FAN_CTL);
>  
>  		data->sensor = it87_read_value(data, IT87_REG_TEMP_ENABLE);
> -		/* The 8705 does not have VID capability */
> +		/* The 8705 does not have VID capability.
> +		   The 8718 does not use IT87_REG_VID for the same purpose. */
>  		if (data->type = it8712 || data->type = it8716) {
>  			data->vid = it87_read_value(data, IT87_REG_VID);
>  			/* The older IT8712F revisions had only 5 VID pins,

Other than that it looks simple and nice. Please update according to my
comments and resubmit as a new post, so that it gets a better summary
line and doesn't get lost in the list.

I've tested your patch on my IT8705F revision F, and didn't experience
any regression. I have an IT8712F somewhere, I'll test on it later
today.

-- 
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: it87 support for 16-bit fan reading

[Andrew Paprocki]

On Sun, Jul 6, 2008 at 7:16 AM, Jean Delvare <khali@linux-fr.org> wrote:

> can't test. Someone with such a chip (you?) could attempt to reset it
> (write 1 to bit 7 of register 00h) and check what the default value
> of register 0Bh actually is.

I tested this, and the value is in fact 09h. I did this as the first
thing in it87_init_device():

it87_write_value(data, 0x00, 1<<7);
tmp = it87_read_value(data, IT87_REG_FAN_DIV);

As you said, it doesn't really matter, because the chips that support
16-bit will use it now.

> So maybe we should simply always enable all of fan1, fan2 and fan3,
> regardless of what the BIOS did. That's a one-line change as far as I
> can see, fairly simple.

I'm in favor of that. It can't do any harm, really, as everyone
controls what inputs/temps/fans are ignored via sensors.conf anyway.
Should I submit a separate patch for that?

> revisions >= J as far as I can see. This makes me wonder why your patch
> doesn't also cover revision I, as it already had 16-bit fan speed
> registers and the 2 extra fans as well.

I updated the documentation in the patch I'll post next. I went back
and looked at the I datasheet, and you are correct. It (rev 07h)
supports both 8-bit and 16-bit modes, and the fan divisors were simply
dropped in rev 08h. I modified the patch to check for rev >= 07h.

I also noticed the documentation mentioned that later IT8705F
revisions also support 16-bit fans. I looked at the only datasheet
currently online (Version G) and it does indeed support the same
registers. I will submit a patch to enable 16-bit on this chip as
well. Do you happen to have any older datasheets for this chip? I'll
enable 16-bit for >= rev 05h (G), but if you have data showing it
exists in an earlier revision than that, please let me know. Also, is
Version G actually rev 05h? The G datasheet says the default value for
the 22h register is 03h and does not enumerate the version values like
the IT8712F datasheet. What revision does your chip report?

-Andrew

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

[lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading in

[Andrew Paprocki]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

The it8712 chip supports 16-bit fan tachometers in revisions >= 0x07.
Revisions >= 0x08 dropped support for 8-bit fan divisor registers. The
patch enables 16-bit fan readings on all revisions >= 0x07 just like
the it8716 and it8718 chips.

Signed-off-by: Andrew Paprocki <andrew@ishiboo.com>
---
 Documentation/hwmon/it87 |    9 +++++----
 drivers/hwmon/it87.c     |   32 ++++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 14 deletions(-)

[-- Attachment #2: 0001-hwmon-it87-support-for-16-bit-fan-reading-in-it8712-rev-0x07.txt --]
[-- Type: text/plain, Size: 6672 bytes --]

From 98ce8da0414dabab7fe7657cc1890bf12db80c24 Mon Sep 17 00:00:00 2001
From: Andrew Paprocki <andrew@ishiboo.com>
Date: Sun, 6 Jul 2008 19:41:17 -0400
Subject: [PATCH] hwmon: it87 support for 16-bit fan reading in it8712 >= rev 0x07

The it8712 chip supports 16-bit fan tachometers in revisions >= 0x07.
Revisions >= 0x08 dropped support for 8-bit fan divisor registers. The
patch enables 16-bit fan readings on all revisions >= 0x07 just like
the it8716 and it8718 chips.

Signed-off-by: Andrew Paprocki <andrew@ishiboo.com>
---
 Documentation/hwmon/it87 |    9 +++++----
 drivers/hwmon/it87.c     |   32 ++++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/Documentation/hwmon/it87 b/Documentation/hwmon/it87
index f4ce1fd..d931525 100644
--- a/Documentation/hwmon/it87
+++ b/Documentation/hwmon/it87
@@ -11,7 +11,9 @@ Supported chips:
     Prefix: 'it8712'
     Addresses scanned: from Super I/O config space (8 I/O ports)
     Datasheet: Publicly available at the ITE website
-               http://www.ite.com.tw/
+               http://www.ite.com.tw/product_info/file/pc/IT8712F_V0.9.1.pdf
+               http://www.ite.com.tw/product_info/file/pc/Errata%20V0.1%20for%20IT8712F%20V0.9.1.pdf
+               http://www.ite.com.tw/product_info/file/pc/IT8712F_V0.9.3.pdf
   * IT8716F/IT8726F
     Prefix: 'it8716'
     Addresses scanned: from Super I/O config space (8 I/O ports)
@@ -90,14 +92,13 @@ upper VID bits share their pins with voltage inputs (in5 and in6) so you
 can't have both on a given board.
 
 The IT8716F, IT8718F and later IT8712F revisions have support for
-2 additional fans. They are supported by the driver for the IT8716F and
-IT8718F but not for the IT8712F
+2 additional fans. The additional fans are supported by the driver.
 
 The IT8716F and IT8718F, and late IT8712F and IT8705F also have optional
 16-bit tachometer counters for fans 1 to 3. This is better (no more fan
 clock divider mess) but not compatible with the older chips and
 revisions. For now, the driver only uses the 16-bit mode on the
-IT8716F and IT8718F.
+late IT8712F, IT8716F and IT8718F.
 
 The IT8726F is just bit enhanced IT8716F with additional hardware
 for AMD power sequencing. Therefore the chip will appear as IT8716F
diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index e12c132..2a36568 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -151,9 +151,9 @@ static int fix_pwm_polarity;
 /* The IT8718F has the VID value in a different register, in Super-I/O
    configuration space. */
 #define IT87_REG_VID           0x0a
-/* Warning: register 0x0b is used for something completely different in
-   new chips/revisions. I suspect only 16-bit tachometer mode will work
-   for these. */
+/* The IT8705F and IT8712F earlier than revision 0x08 use register 0x0b
+   for fan divisors. Later IT8712F revisions must use 16-bit tachometer
+   mode. */
 #define IT87_REG_FAN_DIV       0x0b
 #define IT87_REG_FAN_16BIT     0x0c
 
@@ -234,6 +234,7 @@ static const unsigned int pwm_freq[8] = {
 struct it87_sio_data {
 	enum chips type;
 	/* Values read from Super-I/O config space */
+	u8 revision;
 	u8 vid_value;
 };
 
@@ -242,6 +243,7 @@ struct it87_sio_data {
 struct it87_data {
 	struct device *hwmon_dev;
 	enum chips type;
+	u8 revision;
 
 	unsigned short addr;
 	const char *name;
@@ -268,6 +270,14 @@ struct it87_data {
 	u8 manual_pwm_ctl[3];   /* manual PWM value set by user */
 };
 
+static inline int has_16bit_fans(const struct it87_data *data)
+{
+	/* IT8712F Datasheet 0.9.1, section 8.3.5 indicates 7h == Version I.
+	   This is the first revision with 16bit tachometer support. */
+	return (data->type == it8712 && data->revision >= 0x07)
+	    || data->type == it8716
+	    || data->type == it8718;
+}
 
 static int it87_probe(struct platform_device *pdev);
 static int __devexit it87_remove(struct platform_device *pdev);
@@ -991,8 +1001,9 @@ static int __init it87_find(unsigned short *address,
 	}
 
 	err = 0;
+	sio_data->revision = superio_inb(DEVREV) & 0x0f;
 	pr_info("it87: Found IT%04xF chip at 0x%x, revision %d\n",
-		chip_type, *address, superio_inb(DEVREV) & 0x0f);
+		chip_type, *address, sio_data->revision);
 
 	/* Read GPIO config and VID value from LDN 7 (GPIO) */
 	if (chip_type != IT8705F_DEVID) {
@@ -1045,6 +1056,7 @@ static int __devinit it87_probe(struct platform_device *pdev)
 
 	data->addr = res->start;
 	data->type = sio_data->type;
+	data->revision = sio_data->revision;
 	data->name = names[sio_data->type];
 
 	/* Now, we do the remaining detection. */
@@ -1069,7 +1081,7 @@ static int __devinit it87_probe(struct platform_device *pdev)
 		goto ERROR2;
 
 	/* Do not create fan files for disabled fans */
-	if (data->type == it8716 || data->type == it8718) {
+	if (has_16bit_fans(data)) {
 		/* 16-bit tachometers */
 		if (data->has_fan & (1 << 0)) {
 			if ((err = device_create_file(dev,
@@ -1350,7 +1362,7 @@ static void __devinit it87_init_device(struct platform_device *pdev)
 	data->has_fan = (data->fan_main_ctrl >> 4) & 0x07;
 
 	/* Set tachometers to 16-bit mode if needed */
-	if (data->type == it8716 || data->type == it8718) {
+	if (has_16bit_fans(data)) {
 		tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
 		if (~tmp & 0x07 & data->has_fan) {
 			dev_dbg(&pdev->dev,
@@ -1426,7 +1438,7 @@ static struct it87_data *it87_update_device(struct device *dev)
 			data->fan[i] = it87_read_value(data,
 				       IT87_REG_FAN[i]);
 			/* Add high byte if in 16-bit mode */
-			if (data->type == it8716 || data->type == it8718) {
+			if (has_16bit_fans(data)) {
 				data->fan[i] |= it87_read_value(data,
 						IT87_REG_FANX[i]) << 8;
 				data->fan_min[i] |= it87_read_value(data,
@@ -1443,8 +1455,7 @@ static struct it87_data *it87_update_device(struct device *dev)
 		}
 
 		/* Newer chips don't have clock dividers */
-		if ((data->has_fan & 0x07) && data->type != it8716
-		 && data->type != it8718) {
+		if ((data->has_fan & 0x07) && !has_16bit_fans(data)) {
 			i = it87_read_value(data, IT87_REG_FAN_DIV);
 			data->fan_div[0] = i & 0x07;
 			data->fan_div[1] = (i >> 3) & 0x07;
@@ -1460,7 +1471,8 @@ static struct it87_data *it87_update_device(struct device *dev)
 		data->fan_ctl = it87_read_value(data, IT87_REG_FAN_CTL);
 
 		data->sensor = it87_read_value(data, IT87_REG_TEMP_ENABLE);
-		/* The 8705 does not have VID capability */
+		/* The 8705 does not have VID capability.
+		   The 8718 does not use IT87_REG_VID for the same purpose. */
 		if (data->type == it8712 || data->type == it8716) {
 			data->vid = it87_read_value(data, IT87_REG_VID);
 			/* The older IT8712F revisions had only 5 VID pins,
-- 
1.4.4.4


[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

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

Re: [lm-sensors] [PATCH] hwmon: it87 support for 16-bit fan reading

[Jean Delvare]

Hi Andrew,

On Sun, 6 Jul 2008 20:52:40 -0400, Andrew Paprocki wrote:
> On Sun, Jul 6, 2008 at 7:16 AM, Jean Delvare <khali@linux-fr.org> wrote:
> 
> > can't test. Someone with such a chip (you?) could attempt to reset it
> > (write 1 to bit 7 of register 00h) and check what the default value
> > of register 0Bh actually is.
> 
> I tested this, and the value is in fact 09h. I did this as the first
> thing in it87_init_device():
> 
> it87_write_value(data, 0x00, 1<<7);
> tmp = it87_read_value(data, IT87_REG_FAN_DIV);
> 
> As you said, it doesn't really matter, because the chips that support
> 16-bit will use it now.
> 
> > So maybe we should simply always enable all of fan1, fan2 and fan3,
> > regardless of what the BIOS did. That's a one-line change as far as I
> > can see, fairly simple.
> 
> I'm in favor of that. It can't do any harm, really, as everyone
> controls what inputs/temps/fans are ignored via sensors.conf anyway.

My hope was to make the configuration a bit easier by not presenting to
the users fan inputs which were not used on their systems. But this
only works if the BIOS plays the game, and experience has shown that it
does not always do.

> Should I submit a separate patch for that?

Yes please.

> > revisions >= J as far as I can see. This makes me wonder why your patch
> > doesn't also cover revision I, as it already had 16-bit fan speed
> > registers and the 2 extra fans as well.
> 
> I updated the documentation in the patch I'll post next. I went back
> and looked at the I datasheet, and you are correct. It (rev 07h)
> supports both 8-bit and 16-bit modes, and the fan divisors were simply
> dropped in rev 08h. I modified the patch to check for rev >= 07h.

Good.

> I also noticed the documentation mentioned that later IT8705F
> revisions also support 16-bit fans. I looked at the only datasheet
> currently online (Version G) and it does indeed support the same
> registers. I will submit a patch to enable 16-bit on this chip as
> well. Do you happen to have any older datasheets for this chip? I'll
> enable 16-bit for >= rev 05h (G), but if you have data showing it
> exists in an earlier revision than that, please let me know. Also, is
> Version G actually rev 05h? The G datasheet says the default value for
> the 22h register is 03h and does not enumerate the version values like
> the IT8712F datasheet. What revision does your chip report?

My IT8705F chip reports:
it87: Found IT8705F chip at 0x290, revision 2

That would correspond to "rev. F" in the datasheets. My personal notes
on the IT8705F revisions are:

IT8705F rev. F: 0x02
IT8705F rev. G: 0x03 (datasheet v0.4.1)

My rev. F chip has no register 0Ch, so it definitely doesn't support
16-bit mode. So you would test for revision code >= 0x03 for the
IT8705F. Should be fairly easy after the patch you've just written.

Thanks,
-- 
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: it87 support for 16-bit fan reading

[Jean Delvare]

On Sun, 6 Jul 2008 20:57:12 -0400, Andrew Paprocki wrote:
> The it8712 chip supports 16-bit fan tachometers in revisions >= 0x07.
> Revisions >= 0x08 dropped support for 8-bit fan divisor registers. The
> patch enables 16-bit fan readings on all revisions >= 0x07 just like
> the it8716 and it8718 chips.
> 
> Signed-off-by: Andrew Paprocki <andrew@ishiboo.com>
> ---
>  Documentation/hwmon/it87 |    9 +++++----
>  drivers/hwmon/it87.c     |   32 ++++++++++++++++++++++----------
>  2 files changed, 27 insertions(+), 14 deletions(-)

Acked-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

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