[lm-sensors] [PATCH] hwmon: (f71882fg) Add support for the F81865F

[lm-sensors] [PATCH] hwmon: (f71882fg) Add support for the F81865F

[Jean Delvare]

Add support for the Fintek F81865F. It's essentially compatible with
the F71882FG, but has fewer inputs: 7 voltage, 2 temperature and 2 fan
inputs only.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/hwmon/f71882fg |    4 ++++
 drivers/hwmon/Kconfig        |    1 +
 drivers/hwmon/f71882fg.c     |   32 +++++++++++++++++++-------------
 3 files changed, 24 insertions(+), 13 deletions(-)

--- linux-2.6.39-rc0.orig/drivers/hwmon/f71882fg.c	2011-03-24 08:15:44.000000000 +0100
+++ linux-2.6.39-rc0/drivers/hwmon/f71882fg.c	2011-03-25 13:37:32.000000000 +0100
@@ -55,6 +55,7 @@
 #define SIO_F71889_ID		0x0723	/* Chipset ID */
 #define SIO_F71889E_ID		0x0909	/* Chipset ID */
 #define SIO_F8000_ID		0x0581	/* Chipset ID */
+#define SIO_F81865_ID		0x0704	/* Chipset ID */
 
 #define REGION_LENGTH		8
 #define ADDR_REG_OFFSET		5
@@ -106,7 +107,7 @@ module_param(force_id, ushort, 0);
 MODULE_PARM_DESC(force_id, "Override the detected device ID");
 
 enum chips { f71808e, f71858fg, f71862fg, f71869, f71882fg, f71889fg,
-	     f71889ed, f8000 };
+	     f71889ed, f8000, f81865f };
 
 static const char *f71882fg_names[] = {
 	"f71808e",
@@ -117,9 +118,10 @@ static const char *f71882fg_names[] = {
 	"f71889fg", /* f81801u too, same id */
 	"f71889ed",
 	"f8000",
+	"f81865f",
 };
 
-static const char f71882fg_has_in[8][F71882FG_MAX_INS] = {
+static const char f71882fg_has_in[9][F71882FG_MAX_INS] = {
 	[f71808e]	= { 1, 1, 1, 1, 1, 1, 0, 1, 1 },
 	[f71858fg]	= { 1, 1, 1, 0, 0, 0, 0, 0, 0 },
 	[f71862fg]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
@@ -128,9 +130,10 @@ static const char f71882fg_has_in[8][F71
 	[f71889fg]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
 	[f71889ed]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
 	[f8000]		= { 1, 1, 1, 0, 0, 0, 0, 0, 0 },
+	[f81865f]	= { 1, 1, 1, 1, 1, 1, 1, 0, 0 },
 };
 
-static const char f71882fg_has_in1_alarm[8] = {
+static const char f71882fg_has_in1_alarm[9] = {
 	[f71808e]	= 0,
 	[f71858fg]	= 0,
 	[f71862fg]	= 0,
@@ -139,9 +142,10 @@ static const char f71882fg_has_in1_alarm
 	[f71889fg]	= 1,
 	[f71889ed]	= 1,
 	[f8000]		= 0,
+	[f81865f]	= 1,
 };
 
-static const char f71882fg_has_beep[8] = {
+static const char f71882fg_has_beep[9] = {
 	[f71808e]	= 0,
 	[f71858fg]	= 0,
 	[f71862fg]	= 1,
@@ -150,9 +154,10 @@ static const char f71882fg_has_beep[8]  	[f71889fg]	= 1,
 	[f71889ed]	= 1,
 	[f8000]		= 0,
+	[f81865f]	= 1,
 };
 
-static const char f71882fg_nr_fans[8] = {
+static const char f71882fg_nr_fans[9] = {
 	[f71808e]	= 3,
 	[f71858fg]	= 3,
 	[f71862fg]	= 3,
@@ -161,9 +166,10 @@ static const char f71882fg_nr_fans[8]  	[f71889fg]	= 3,
 	[f71889ed]	= 3,
 	[f8000]		= 3,
+	[f81865f]	= 2,
 };
 
-static const char f71882fg_nr_temps[8] = {
+static const char f71882fg_nr_temps[9] = {
 	[f71808e]	= 2,
 	[f71858fg]	= 3,
 	[f71862fg]	= 3,
@@ -172,6 +178,7 @@ static const char f71882fg_nr_temps[8]  	[f71889fg]	= 3,
 	[f71889ed]	= 3,
 	[f8000]		= 3,
+	[f81865f]	= 2,
 };
 
 static struct platform_device *f71882fg_pdev;
@@ -2186,16 +2193,12 @@ static int __devinit f71882fg_probe(stru
 		case f71862fg:
 			err = (data->pwm_enable & 0x15) != 0x15;
 			break;
-		case f71808e:
-		case f71869:
-		case f71882fg:
-		case f71889fg:
-		case f71889ed:
-			err = 0;
-			break;
 		case f8000:
 			err = data->pwm_enable & 0x20;
 			break;
+		default:
+			err = 0;
+			break;
 		}
 		if (err) {
 			dev_err(&pdev->dev,
@@ -2433,6 +2436,9 @@ static int __init f71882fg_find(int sioa
 	case SIO_F8000_ID:
 		sio_data->type = f8000;
 		break;
+	case SIO_F81865_ID:
+		sio_data->type = f81865f;
+		break;
 	default:
 		pr_info("Unsupported Fintek device: %04x\n",
 			(unsigned int)devid);
--- linux-2.6.39-rc0.orig/Documentation/hwmon/f71882fg	2011-03-24 08:15:44.000000000 +0100
+++ linux-2.6.39-rc0/Documentation/hwmon/f71882fg	2011-03-25 13:38:13.000000000 +0100
@@ -41,6 +41,10 @@ Supported chips:
     Note: This is the 64-pin variant of the F71889FG, they have the
 	  same device ID and are fully compatible as far as hardware
 	  monitoring is concerned.
+  * Fintek F81865F
+    Prefix: 'f81865f'
+    Addresses scanned: none, address read from Super I/O config space
+    Datasheet: Available from the Fintek website
 
 Author: Hans de Goede <hdegoede@redhat.com>
 
--- linux-2.6.39-rc0.orig/drivers/hwmon/Kconfig	2011-03-24 08:16:44.000000000 +0100
+++ linux-2.6.39-rc0/drivers/hwmon/Kconfig	2011-03-25 13:38:41.000000000 +0100
@@ -330,6 +330,7 @@ config SENSORS_F71882FG
 	    F71889FG/ED
 	    F8000
 	    F81801U
+	    F81865F
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called f71882fg.


-- 
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: (f71882fg) Add support for the

[Guenter Roeck]

On Fri, Mar 25, 2011 at 08:50:49AM -0400, Jean Delvare wrote:
> Add support for the Fintek F81865F. It's essentially compatible with
> the F71882FG, but has fewer inputs: 7 voltage, 2 temperature and 2 fan
> inputs only.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>

Thanks,
Guenter

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

[lm-sensors] [PATCH] hwmon: (f71882fg) Add support for the F71889A

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/hwmon/f71882fg |    4 ++++
 drivers/hwmon/Kconfig        |    2 +-
 drivers/hwmon/f71882fg.c     |   24 ++++++++++++++++++------
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
index b802a22..df02245 100644
--- a/Documentation/hwmon/f71882fg
+++ b/Documentation/hwmon/f71882fg
@@ -30,6 +30,10 @@ Supported chips:
     Prefix: 'f71889ed'
     Addresses scanned: none, address read from Super I/O config space
     Datasheet: Should become available on the Fintek website soon
+  * Fintek F71889A
+    Prefix: 'f71889a'
+    Addresses scanned: none, address read from Super I/O config space
+    Datasheet: Should become available on the Fintek website soon
   * Fintek F8000
     Prefix: 'f8000'
     Addresses scanned: none, address read from Super I/O config space
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 504117c..065d01b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -327,7 +327,7 @@ config SENSORS_F71882FG
 	    F71869F/E
 	    F71882FG
 	    F71883FG
-	    F71889FG/ED
+	    F71889FG/ED/A
 	    F8000
 	    F81801U
 	    F81865F
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 41248cd..ca07a32 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -54,6 +54,7 @@
 #define SIO_F71882_ID		0x0541	/* Chipset ID */
 #define SIO_F71889_ID		0x0723	/* Chipset ID */
 #define SIO_F71889E_ID		0x0909	/* Chipset ID */
+#define SIO_F71889A_ID		0x1005	/* Chipset ID */
 #define SIO_F8000_ID		0x0581	/* Chipset ID */
 #define SIO_F81865_ID		0x0704	/* Chipset ID */
 
@@ -107,7 +108,7 @@ module_param(force_id, ushort, 0);
 MODULE_PARM_DESC(force_id, "Override the detected device ID");
 
 enum chips { f71808e, f71858fg, f71862fg, f71869, f71882fg, f71889fg,
-	     f71889ed, f8000, f81865f };
+	     f71889ed, f71889a, f8000, f81865f };
 
 static const char *f71882fg_names[] = {
 	"f71808e",
@@ -117,11 +118,12 @@ static const char *f71882fg_names[] = {
 	"f71882fg",
 	"f71889fg", /* f81801u too, same id */
 	"f71889ed",
+	"f71889a",
 	"f8000",
 	"f81865f",
 };
 
-static const char f71882fg_has_in[9][F71882FG_MAX_INS] = {
+static const char f71882fg_has_in[][F71882FG_MAX_INS] = {
 	[f71808e]	= { 1, 1, 1, 1, 1, 1, 0, 1, 1 },
 	[f71858fg]	= { 1, 1, 1, 0, 0, 0, 0, 0, 0 },
 	[f71862fg]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
@@ -129,11 +131,12 @@ static const char f71882fg_has_in[9][F71882FG_MAX_INS] = {
 	[f71882fg]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
 	[f71889fg]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
 	[f71889ed]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
+	[f71889a]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
 	[f8000]		= { 1, 1, 1, 0, 0, 0, 0, 0, 0 },
 	[f81865f]	= { 1, 1, 1, 1, 1, 1, 1, 0, 0 },
 };
 
-static const char f71882fg_has_in1_alarm[9] = {
+static const char f71882fg_has_in1_alarm[] = {
 	[f71808e]	= 0,
 	[f71858fg]	= 0,
 	[f71862fg]	= 0,
@@ -141,11 +144,12 @@ static const char f71882fg_has_in1_alarm[9] = {
 	[f71882fg]	= 1,
 	[f71889fg]	= 1,
 	[f71889ed]	= 1,
+	[f71889a]	= 1,
 	[f8000]		= 0,
 	[f81865f]	= 1,
 };
 
-static const char f71882fg_has_beep[9] = {
+static const char f71882fg_has_beep[] = {
 	[f71808e]	= 0,
 	[f71858fg]	= 0,
 	[f71862fg]	= 1,
@@ -153,11 +157,12 @@ static const char f71882fg_has_beep[9] = {
 	[f71882fg]	= 1,
 	[f71889fg]	= 1,
 	[f71889ed]	= 1,
+	[f71889a]	= 1,
 	[f8000]		= 0,
 	[f81865f]	= 1,
 };
 
-static const char f71882fg_nr_fans[9] = {
+static const char f71882fg_nr_fans[] = {
 	[f71808e]	= 3,
 	[f71858fg]	= 3,
 	[f71862fg]	= 3,
@@ -165,11 +170,12 @@ static const char f71882fg_nr_fans[9] = {
 	[f71882fg]	= 4,
 	[f71889fg]	= 3,
 	[f71889ed]	= 3,
+	[f71889a]	= 3,
 	[f8000]		= 3,
 	[f81865f]	= 2,
 };
 
-static const char f71882fg_nr_temps[9] = {
+static const char f71882fg_nr_temps[] = {
 	[f71808e]	= 2,
 	[f71858fg]	= 3,
 	[f71862fg]	= 3,
@@ -177,6 +183,7 @@ static const char f71882fg_nr_temps[9] = {
 	[f71882fg]	= 3,
 	[f71889fg]	= 3,
 	[f71889ed]	= 3,
+	[f71889a]	= 3,
 	[f8000]		= 3,
 	[f81865f]	= 2,
 };
@@ -2168,6 +2175,7 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
 			/* Fall through to select correct fan/pwm reg bank! */
 		case f71889fg:
 		case f71889ed:
+		case f71889a:
 			reg = f71882fg_read8(data, F71882FG_REG_FAN_FAULT_T);
 			if (reg & F71882FG_FAN_NEG_TEMP_EN)
 				data->auto_point_temp_signed = 1;
@@ -2225,6 +2233,7 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
 		case f71869:
 		case f71889fg:
 		case f71889ed:
+		case f71889a:
 			for (i = 0; i < nr_fans; i++) {
 				data->pwm_auto_point_mapping[i]  					f71882fg_read8(data,
@@ -2433,6 +2442,9 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
 	case SIO_F71889E_ID:
 		sio_data->type = f71889ed;
 		break;
+	case SIO_F71889A_ID:
+		sio_data->type = f71889a;
+		break;
 	case SIO_F8000_ID:
 		sio_data->type = f8000;
 		break;
-- 
1.7.4.1


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

Re: [lm-sensors] [PATCH] hwmon: (f71882fg) Add support for the

[Guenter Roeck]

On Fri, Mar 25, 2011 at 09:56:26AM -0400, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>

Thanks,
Guenter

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

Re: [lm-sensors] [PATCH] hwmon: (f71882fg) Add support for the

[Jean Delvare]

Hi Hans,

On Fri, 25 Mar 2011 14:56:26 +0100, Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  Documentation/hwmon/f71882fg |    4 ++++
>  drivers/hwmon/Kconfig        |    2 +-
>  drivers/hwmon/f71882fg.c     |   24 ++++++++++++++++++------
>  3 files changed, 23 insertions(+), 7 deletions(-)

I'm curious... what was the rationale for using 3 different types for
the 889FG, 889ED and 889A if they all behave exactly the same?

> 
> diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
> index b802a22..df02245 100644
> --- a/Documentation/hwmon/f71882fg
> +++ b/Documentation/hwmon/f71882fg
> @@ -30,6 +30,10 @@ Supported chips:
>      Prefix: 'f71889ed'
>      Addresses scanned: none, address read from Super I/O config space
>      Datasheet: Should become available on the Fintek website soon
> +  * Fintek F71889A
> +    Prefix: 'f71889a'
> +    Addresses scanned: none, address read from Super I/O config space
> +    Datasheet: Should become available on the Fintek website soon
>    * Fintek F8000
>      Prefix: 'f8000'
>      Addresses scanned: none, address read from Super I/O config space
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 504117c..065d01b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -327,7 +327,7 @@ config SENSORS_F71882FG
>  	    F71869F/E
>  	    F71882FG
>  	    F71883FG
> -	    F71889FG/ED
> +	    F71889FG/ED/A
>  	    F8000
>  	    F81801U
>  	    F81865F
> diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
> index 41248cd..ca07a32 100644
> --- a/drivers/hwmon/f71882fg.c
> +++ b/drivers/hwmon/f71882fg.c
> @@ -54,6 +54,7 @@
>  #define SIO_F71882_ID		0x0541	/* Chipset ID */
>  #define SIO_F71889_ID		0x0723	/* Chipset ID */
>  #define SIO_F71889E_ID		0x0909	/* Chipset ID */
> +#define SIO_F71889A_ID		0x1005	/* Chipset ID */
>  #define SIO_F8000_ID		0x0581	/* Chipset ID */
>  #define SIO_F81865_ID		0x0704	/* Chipset ID */
>  
> @@ -107,7 +108,7 @@ module_param(force_id, ushort, 0);
>  MODULE_PARM_DESC(force_id, "Override the detected device ID");
>  
>  enum chips { f71808e, f71858fg, f71862fg, f71869, f71882fg, f71889fg,
> -	     f71889ed, f8000, f81865f };
> +	     f71889ed, f71889a, f8000, f81865f };
>  
>  static const char *f71882fg_names[] = {
>  	"f71808e",
> @@ -117,11 +118,12 @@ static const char *f71882fg_names[] = {
>  	"f71882fg",
>  	"f71889fg", /* f81801u too, same id */
>  	"f71889ed",
> +	"f71889a",
>  	"f8000",
>  	"f81865f",
>  };
>  
> -static const char f71882fg_has_in[9][F71882FG_MAX_INS] = {
> +static const char f71882fg_has_in[][F71882FG_MAX_INS] = {

Ah, I was hesitant to drop the array size. If you are OK with this then
I'll move the change into my first two patches. This will make all
other patches smaller.

>  	[f71808e]	= { 1, 1, 1, 1, 1, 1, 0, 1, 1 },
>  	[f71858fg]	= { 1, 1, 1, 0, 0, 0, 0, 0, 0 },
>  	[f71862fg]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },

Other than this, no objection, so I'll queue this patch. I plan to send
all 5 patches to Linus later today, tomorrow at the latest.

-- 
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: (f71882fg) Add support for the

[Hans de Goede]

Hi,

On 03/25/2011 03:14 PM, Jean Delvare wrote:
> Hi Hans,
>
> On Fri, 25 Mar 2011 14:56:26 +0100, Hans de Goede wrote:
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>>   Documentation/hwmon/f71882fg |    4 ++++
>>   drivers/hwmon/Kconfig        |    2 +-
>>   drivers/hwmon/f71882fg.c     |   24 ++++++++++++++++++------
>>   3 files changed, 23 insertions(+), 7 deletions(-)
>
> I'm curious... what was the rationale for using 3 different types for
> the 889FG, 889ED and 889A if they all behave exactly the same?
>

There are differences in the unused parts, such as the digital temp
sensor interface support (PECI & friends), which I hope to add support
for one day. I have an isadump of all registers of 1 board which
actually uses this functionality. But I'll need a significant sized
slot of free time to look into this.

>>
>> diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
>> index b802a22..df02245 100644
>> --- a/Documentation/hwmon/f71882fg
>> +++ b/Documentation/hwmon/f71882fg
>> @@ -30,6 +30,10 @@ Supported chips:
>>       Prefix: 'f71889ed'
>>       Addresses scanned: none, address read from Super I/O config space
>>       Datasheet: Should become available on the Fintek website soon
>> +  * Fintek F71889A
>> +    Prefix: 'f71889a'
>> +    Addresses scanned: none, address read from Super I/O config space
>> +    Datasheet: Should become available on the Fintek website soon
>>     * Fintek F8000
>>       Prefix: 'f8000'
>>       Addresses scanned: none, address read from Super I/O config space
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 504117c..065d01b 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -327,7 +327,7 @@ config SENSORS_F71882FG
>>   	    F71869F/E
>>   	    F71882FG
>>   	    F71883FG
>> -	    F71889FG/ED
>> +	    F71889FG/ED/A
>>   	    F8000
>>   	    F81801U
>>   	    F81865F
>> diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
>> index 41248cd..ca07a32 100644
>> --- a/drivers/hwmon/f71882fg.c
>> +++ b/drivers/hwmon/f71882fg.c
>> @@ -54,6 +54,7 @@
>>   #define SIO_F71882_ID		0x0541	/* Chipset ID */
>>   #define SIO_F71889_ID		0x0723	/* Chipset ID */
>>   #define SIO_F71889E_ID		0x0909	/* Chipset ID */
>> +#define SIO_F71889A_ID		0x1005	/* Chipset ID */
>>   #define SIO_F8000_ID		0x0581	/* Chipset ID */
>>   #define SIO_F81865_ID		0x0704	/* Chipset ID */
>>
>> @@ -107,7 +108,7 @@ module_param(force_id, ushort, 0);
>>   MODULE_PARM_DESC(force_id, "Override the detected device ID");
>>
>>   enum chips { f71808e, f71858fg, f71862fg, f71869, f71882fg, f71889fg,
>> -	     f71889ed, f8000, f81865f };
>> +	     f71889ed, f71889a, f8000, f81865f };
>>
>>   static const char *f71882fg_names[] = {
>>   	"f71808e",
>> @@ -117,11 +118,12 @@ static const char *f71882fg_names[] = {
>>   	"f71882fg",
>>   	"f71889fg", /* f81801u too, same id */
>>   	"f71889ed",
>> +	"f71889a",
>>   	"f8000",
>>   	"f81865f",
>>   };
>>
>> -static const char f71882fg_has_in[9][F71882FG_MAX_INS] = {
>> +static const char f71882fg_has_in[][F71882FG_MAX_INS] = {
>
> Ah, I was hesitant to drop the array size. If you are OK with this then
> I'll move the change into my first two patches. This will make all
> other patches smaller.

I was hesitant myself too, but after thinking about it I see no reason not
to, gcc will only warn if the array is smaller then our initializers need,
AFAIK gcc won't warn if we are missing an initializer. So I see no gain in
having an explicit size, where as using auto sizing makes future patches
for adding support for more models (such as the F71808A and F71869A)
smaller / easier to review.

>
>>   	[f71808e]	= { 1, 1, 1, 1, 1, 1, 0, 1, 1 },
>>   	[f71858fg]	= { 1, 1, 1, 0, 0, 0, 0, 0, 0 },
>>   	[f71862fg]	= { 1, 1, 1, 1, 1, 1, 1, 1, 1 },
>
> Other than this, no objection, so I'll queue this patch. I plan to send
> all 5 patches to Linus later today, tomorrow at the latest.
>

Thanks!

Regards,

Hans

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

Re: [lm-sensors] [PATCH 1/4] hwmon: f71882fg: Add support for the

[Giel van Schijndel]

[-- Attachment #1.1: Type: text/plain, Size: 1031 bytes --]

On Sun, Aug 01, 2010 at 08:12:40 +0200, Hans de Goede wrote:
> On 08/01/2010 01:31 AM, Giel van Schijndel wrote:
>> On Wed, Mar 24, 2010 at 11:31:40 +0100, Hans de Goede wrote:
>>> On 03/24/2010 10:23, Giel van Schijndel wrote:
>>>> Ack. New and improved patch follows this line.
>>>> ===================================================================
>>>> hwmon: f71882fg: Add support for the Fintek F71808E
>>>
>>> This new version looks good to me:
>>> Acked-by: Hans de Goede<hdegoede@redhat.com>
>>
>> Thanks.  Anyone else I need to poke in order to set this on track to
>> mainline?
> 
> No,
> 
> Jean should have picked this up, I guess it has fallen through the cracks.
> 
> I think it is probably best if you resend this patch and the watchdog
> patches re-based against the latest upstream rc, then I'll re-review
> them and ack them and Jean can then put them into his tree.

Ack.  I'll resend them as replies to this mail.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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

[lm-sensors] [PATCH] hwmon: f71882fg: Add support for the Fintek

[Giel van Schijndel]

Allow device probing to recognise the Fintek F71808E.

Sysfs interface:
 * Fan/pwm control is the same as for F71889FG
 * Temperature and voltage sensor handling is largely the same as for
   the F71889FG
  - Has one temperature sensor less (doesn't have temp3)
  - Misses one voltage sensor (doesn't have V6, thus in6_input refers to
    what in7_input refers for F71889FG)

For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
such that it can largely be reused.

Signed-off-by: Giel van Schijndel <me@mortis.eu>
---
 Documentation/hwmon/f71882fg |    4 ++
 drivers/hwmon/Kconfig        |    6 ++--
 drivers/hwmon/f71882fg.c     |   83 ++++++++++++++++++++++++++++++++++++++----
 3 files changed, 82 insertions(+), 11 deletions(-)

diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
index a7952c2..1a07fd6 100644
--- a/Documentation/hwmon/f71882fg
+++ b/Documentation/hwmon/f71882fg
@@ -2,6 +2,10 @@ Kernel driver f71882fg
 ===========
 
 Supported chips:
+  * Fintek F71808E
+    Prefix: 'f71808fg'
+    Addresses scanned: none, address read from Super I/O config space
+    Datasheet: Not public
   * Fintek F71858FG
     Prefix: 'f71858fg'
     Addresses scanned: none, address read from Super I/O config space
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e19cf8e..7d0beac 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -332,11 +332,11 @@ config SENSORS_F71805F
 	  will be called f71805f.
 
 config SENSORS_F71882FG
-	tristate "Fintek F71858FG, F71862FG, F71882FG, F71889FG and F8000"
+	tristate "Fintek F71808E, F71858FG, F71862FG, F71882FG, F71889FG and F8000"
 	depends on EXPERIMENTAL
 	help
-	  If you say yes here you get support for hardware monitoring
-	  features of the Fintek F71858FG, F71862FG/71863FG, F71882FG/F71883FG,
+	  If you say yes here you get support for hardware monitoring features
+	  of the Fintek F71808E, F71858FG, F71862FG/71863FG, F71882FG/F71883FG,
 	  F71889FG and F8000 Super-I/O chips.
 
 	  This driver can also be built as a module.  If so, the module
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index f4d2279..7857ed3 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -45,6 +45,7 @@
 #define SIO_REG_ADDR		0x60	/* Logical device address (2 bytes) */
 
 #define SIO_FINTEK_ID		0x1934	/* Manufacturers ID */
+#define SIO_F71808_ID		0x0901  /* Chipset ID */
 #define SIO_F71858_ID		0x0507  /* Chipset ID */
 #define SIO_F71862_ID		0x0601	/* Chipset ID */
 #define SIO_F71882_ID		0x0541	/* Chipset ID */
@@ -96,9 +97,10 @@ static unsigned short force_id;
 module_param(force_id, ushort, 0);
 MODULE_PARM_DESC(force_id, "Override the detected device ID");
 
-enum chips { f71858fg, f71862fg, f71882fg, f71889fg, f8000 };
+enum chips { f71808fg, f71858fg, f71862fg, f71882fg, f71889fg, f8000 };
 
 static const char *f71882fg_names[] = {
+	"f71808fg",
 	"f71858fg",
 	"f71862fg",
 	"f71882fg",
@@ -306,8 +308,8 @@ static struct sensor_device_attribute_2 f71858fg_in_temp_attr[] = {
 	SENSOR_ATTR_2(temp3_fault, S_IRUGO, show_temp_fault, NULL, 0, 2),
 };
 
-/* Temp and in attr common to the f71862fg, f71882fg and f71889fg */
-static struct sensor_device_attribute_2 fxxxx_in_temp_attr[] = {
+/* In attr common to the f71862fg, f71882fg and f71889fg */
+static struct sensor_device_attribute_2 fxxxx_in_attr[] = {
 	SENSOR_ATTR_2(in0_input, S_IRUGO, show_in, NULL, 0, 0),
 	SENSOR_ATTR_2(in1_input, S_IRUGO, show_in, NULL, 0, 1),
 	SENSOR_ATTR_2(in2_input, S_IRUGO, show_in, NULL, 0, 2),
@@ -317,6 +319,22 @@ static struct sensor_device_attribute_2 fxxxx_in_temp_attr[] = {
 	SENSOR_ATTR_2(in6_input, S_IRUGO, show_in, NULL, 0, 6),
 	SENSOR_ATTR_2(in7_input, S_IRUGO, show_in, NULL, 0, 7),
 	SENSOR_ATTR_2(in8_input, S_IRUGO, show_in, NULL, 0, 8),
+};
+
+/* In attr for the f71808fg */
+static struct sensor_device_attribute_2 f71808_in_attr[] = {
+	SENSOR_ATTR_2(in0_input, S_IRUGO, show_in, NULL, 0, 0),
+	SENSOR_ATTR_2(in1_input, S_IRUGO, show_in, NULL, 0, 1),
+	SENSOR_ATTR_2(in2_input, S_IRUGO, show_in, NULL, 0, 2),
+	SENSOR_ATTR_2(in3_input, S_IRUGO, show_in, NULL, 0, 3),
+	SENSOR_ATTR_2(in4_input, S_IRUGO, show_in, NULL, 0, 4),
+	SENSOR_ATTR_2(in5_input, S_IRUGO, show_in, NULL, 0, 5),
+	SENSOR_ATTR_2(in6_input, S_IRUGO, show_in, NULL, 0, 7),
+	SENSOR_ATTR_2(in7_input, S_IRUGO, show_in, NULL, 0, 8),
+};
+
+/* Temp attr common to the f71808fg, f71862fg, f71882fg and f71889fg */
+static struct sensor_device_attribute_2 fxxxx_temp_attr[] = {
 	SENSOR_ATTR_2(temp1_input, S_IRUGO, show_temp, NULL, 0, 1),
 	SENSOR_ATTR_2(temp1_max, S_IRUGO|S_IWUSR, show_temp_max,
 		store_temp_max, 0, 1),
@@ -355,6 +373,10 @@ static struct sensor_device_attribute_2 fxxxx_in_temp_attr[] = {
 		store_temp_beep, 0, 6),
 	SENSOR_ATTR_2(temp2_type, S_IRUGO, show_temp_type, NULL, 0, 2),
 	SENSOR_ATTR_2(temp2_fault, S_IRUGO, show_temp_fault, NULL, 0, 2),
+};
+
+/* Temp and in attr common to the f71862fg, f71882fg and f71889fg */
+static struct sensor_device_attribute_2 f71862_temp_attr[] = {
 	SENSOR_ATTR_2(temp3_input, S_IRUGO, show_temp, NULL, 0, 3),
 	SENSOR_ATTR_2(temp3_max, S_IRUGO|S_IWUSR, show_temp_max,
 		store_temp_max, 0, 3),
@@ -989,6 +1011,11 @@ static struct f71882fg_data *f71882fg_update_device(struct device *dev)
 				data->temp_type[1] = 6;
 				break;
 			}
+		} else if (data->type = f71808fg) {
+			reg  = f71882fg_read8(data, F71882FG_REG_TEMP_TYPE);
+			data->temp_type[1] = (reg & 0x02) ? 2 : 4;
+			data->temp_type[2] = (reg & 0x04) ? 2 : 4;
+
 		} else {
 			reg2 = f71882fg_read8(data, F71882FG_REG_PECI);
 			if ((reg2 & 0x03) = 0x01)
@@ -1871,7 +1898,8 @@ static ssize_t store_pwm_auto_point_temp(struct device *dev,
 
 	val /= 1000;
 
-	if (data->type = f71889fg)
+	if (data->type = f71889fg
+	 || data->type = f71808fg)
 		val = SENSORS_LIMIT(val, -128, 127);
 	else
 		val = SENSORS_LIMIT(val, 0, 127);
@@ -1974,8 +2002,28 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
 			/* fall through! */
 		case f71862fg:
 			err = f71882fg_create_sysfs_files(pdev,
-					fxxxx_in_temp_attr,
-					ARRAY_SIZE(fxxxx_in_temp_attr));
+					f71862_temp_attr,
+					ARRAY_SIZE(f71862_temp_attr));
+			if (err)
+				goto exit_unregister_sysfs;
+			err = f71882fg_create_sysfs_files(pdev,
+					fxxxx_in_attr,
+					ARRAY_SIZE(fxxxx_in_attr));
+			if (err)
+				goto exit_unregister_sysfs;
+			err = f71882fg_create_sysfs_files(pdev,
+					fxxxx_temp_attr,
+					ARRAY_SIZE(fxxxx_temp_attr));
+			break;
+		case f71808fg:
+			err = f71882fg_create_sysfs_files(pdev,
+					f71808_in_attr,
+					ARRAY_SIZE(f71808_in_attr));
+			if (err)
+				goto exit_unregister_sysfs;
+			err = f71882fg_create_sysfs_files(pdev,
+					fxxxx_temp_attr,
+					ARRAY_SIZE(fxxxx_temp_attr));
 			break;
 		case f8000:
 			err = f71882fg_create_sysfs_files(pdev,
@@ -2002,6 +2050,7 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
 		case f71862fg:
 			err = (data->pwm_enable & 0x15) != 0x15;
 			break;
+		case f71808fg:
 		case f71882fg:
 		case f71889fg:
 			err = 0;
@@ -2047,6 +2096,7 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
 					f8000_auto_pwm_attr,
 					ARRAY_SIZE(f8000_auto_pwm_attr));
 			break;
+		case f71808fg:
 		case f71889fg:
 			for (i = 0; i < nr_fans; i++) {
 				data->pwm_auto_point_mapping[i] @@ -2126,8 +2176,22 @@ static int f71882fg_remove(struct platform_device *pdev)
 			/* fall through! */
 		case f71862fg:
 			f71882fg_remove_sysfs_files(pdev,
-					fxxxx_in_temp_attr,
-					ARRAY_SIZE(fxxxx_in_temp_attr));
+					f71862_temp_attr,
+					ARRAY_SIZE(f71862_temp_attr));
+			f71882fg_remove_sysfs_files(pdev,
+					fxxxx_in_attr,
+					ARRAY_SIZE(fxxxx_in_attr));
+			f71882fg_remove_sysfs_files(pdev,
+					fxxxx_temp_attr,
+					ARRAY_SIZE(fxxxx_temp_attr));
+			break;
+		case f71808fg:
+			f71882fg_remove_sysfs_files(pdev,
+					f71808_in_attr,
+					ARRAY_SIZE(f71808_in_attr));
+			f71882fg_remove_sysfs_files(pdev,
+					fxxxx_temp_attr,
+					ARRAY_SIZE(fxxxx_temp_attr));
 			break;
 		case f8000:
 			f71882fg_remove_sysfs_files(pdev,
@@ -2195,6 +2259,9 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
 
 	devid = force_id ? force_id : superio_inw(sioaddr, SIO_REG_DEVID);
 	switch (devid) {
+	case SIO_F71808_ID:
+		sio_data->type = f71808fg;
+		break;
 	case SIO_F71858_ID:
 		sio_data->type = f71858fg;
 		break;
-- 
1.6.4.4


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

Re: [PATCH] hwmon: f71882fg: Add support for the Fintek F71808E

[Hans de Goede]

Hi,

I know I've reviewed this patch before, but now I have a datasheet
so this time I've been a bit more thorough and I've found 2 small
issues and 1 bigger one.

Andrew can you please drop this patch from -mm until this is resolved?

On 08/01/2010 03:30 PM, Giel van Schijndel wrote:
> Allow device probing to recognise the Fintek F71808E.
>
> Sysfs interface:
>   * Fan/pwm control is the same as for F71889FG

My datasheet strongly disagrees with this the F71889FG has 5 pwm zones
each with their own speed divided by 4 boundary temps, where as
the F71808E has 3 pwm zones divided by 2 boundary temps. So it is much
more like the F71862FG, which also has 2 boundary temps, and 3 pwm zones,
*but* the F71862FG has one pwm zone hardwired to 100%.

So it looks like you need to create a new f71808e_auto_pwm_attr array
esp. for this model, as well as a special case for reading the
auto pwm attr in f71882fg_update_device.

Also the auto pwm of the F71808E allows following of digital temps
read to peci / amdsi / ibex rather then following a directly connected
temp diode like the F71889FG, which the driver does not support, so
you should check if this is enabled and if so disable the auto pwm
attr entirely. Code for this is already in place for the F71889FG,
you simply need to make it trigger when the chip is a F71808E too.

>   * Temperature and voltage sensor handling is largely the same as for
>     the F71889FG
>    - Has one temperature sensor less (doesn't have temp3)
>    - Misses one voltage sensor (doesn't have V6, thus in6_input refers to
>      what in7_input refers for F71889FG)
>
> For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
> such that it can largely be reused.

There is a problem here though, the new fxxxx_temp_attr contains
attributes for temp#_max_beep and temp#_crit_beep, but the F71808E
lacks that function. So I think that the new fxxxx_temp_attr
need to be split into fxxxx_temp_attr and fxxxx_temp_beep_attr,
like is already done with fxxxx_fan_attr.

Also while making changes, I must say I don't like the splitting
of fxxxx_temp_attr into fxxxx_temp_attr and f71862_temp_attr just because
the number of sensors differs. I think it would be better to instead
make fxxxx_temp_attr a 2 dimensional array like fxxxx_fan_attr and like
with fxxxx_fan_attr register as many sensor attr blocks as the specific
model has.

> Signed-off-by: Giel van Schijndel<me@mortis.eu>
> ---
>   Documentation/hwmon/f71882fg |    4 ++
>   drivers/hwmon/Kconfig        |    6 ++--
>   drivers/hwmon/f71882fg.c     |   83 ++++++++++++++++++++++++++++++++++++++----
>   3 files changed, 82 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
> index a7952c2..1a07fd6 100644
> --- a/Documentation/hwmon/f71882fg
> +++ b/Documentation/hwmon/f71882fg
> @@ -2,6 +2,10 @@ Kernel driver f71882fg
>   ======================
>
>   Supported chips:
> +  * Fintek F71808E
> +    Prefix: 'f71808fg'

This is wrong, as you already indicate and the datasheet as well this
chip in question is an f71808e not an f71808fg, also note that there is
an f71808a model as well which is different (and has a different super io
chip id).

One last request in the second switch case in f71882fg_remove()
there is a default label which contains a comment which models it applies
to, please add the f71808e to that comment.

Regards,

Hans

Re: [PATCH] hwmon: f71882fg: Add support for the Fintek F71808E

[Giel van Schijndel]

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

On Wed, Aug 04, 2010 at 13:36:22 +0200, Hans de Goede wrote:
> On 08/01/2010 03:30, Giel van Schijndel wrote:
>> Allow device probing to recognise the Fintek F71808E.
>> 
>> Sysfs interface:
>>   * Fan/pwm control is the same as for F71889FG
> 
> My datasheet strongly disagrees with this the F71889FG has 5 pwm zones
> each with their own speed divided by 4 boundary temps, where as
> the F71808E has 3 pwm zones divided by 2 boundary temps. So it is much
> more like the F71862FG, which also has 2 boundary temps, and 3 pwm zones,
> *but* the F71862FG has one pwm zone hardwired to 100%.

I'm assuming that by "pwm zone" you mean a separate PWM output channel?
I.e. each "pwm zone" controls a single fan?

Because in the datasheet I have for the F71889 only 3 fan controls are
mentioned, not 5, it does however have 4 boundary temps:
> 7.5. Hardware Monitor
> ... snip ...
> ... page 46-47:
> Device registers, the following is a register map order which shows a
> summary of all registers.  Please refer each one register if you want
> more detail information.
> Register CR01 ~ CR03 -> Configuration Registers
> Register CR0A ~ CR0F -> PECI/SST/TSI Control Register
> Register CR10 ~ CR4F -> Voltage Setting Register
> Register CR60 ~ CR8E -> Temperature Setting Register
> Register CR90 ~ CRDF -> Fan Control Setting Register
>                      -> Fan1 Detail Setting CRA0 ~ CRAF
>                      -> Fan2 Detail Setting CRB0 ~ CRBF
>                      -> Fan3 Detail Setting CRC0 ~ CRCF
> Register CR5A ~ CR5D -> HW Chip ID and Vender ID Register

> So it looks like you need to create a new f71808e_auto_pwm_attr array
> esp. for this model, as well as a special case for reading the
> auto pwm attr in f71882fg_update_device.

Ack.

> Also the auto pwm of the F71808E allows following of digital temps
> read to peci / amdsi / ibex rather then following a directly connected
> temp diode like the F71889FG, which the driver does not support, so
> you should check if this is enabled and if so disable the auto pwm
> attr entirely. Code for this is already in place for the F71889FG,
> you simply need to make it trigger when the chip is a F71808E too.

Do you mean the checking of the FANx_TEMP_SEL_DIG flag in the fan's
"Temperature Mapping Select" registers currently done for the F71889 in
the second switch-statement inside f71882fg_probe?  As that code is
already used for the F71808E as well.

>>   * Temperature and voltage sensor handling is largely the same as for
>>     the F71889FG
>>    - Has one temperature sensor less (doesn't have temp3)
>>    - Misses one voltage sensor (doesn't have V6, thus in6_input refers to
>>      what in7_input refers for F71889FG)
>> 
>> For the purpose of the sysfs interface fxxxx_in_temp_attr[] is split up
>> such that it can largely be reused.
> 
> There is a problem here though, the new fxxxx_temp_attr contains
> attributes for temp#_max_beep and temp#_crit_beep, but the F71808E
> lacks that function. So I think that the new fxxxx_temp_attr
> need to be split into fxxxx_temp_attr and fxxxx_temp_beep_attr,
> like is already done with fxxxx_fan_attr.

Ack.

> Also while making changes, I must say I don't like the splitting
> of fxxxx_temp_attr into fxxxx_temp_attr and f71862_temp_attr just because
> the number of sensors differs. I think it would be better to instead
> make fxxxx_temp_attr a 2 dimensional array like fxxxx_fan_attr and like
> with fxxxx_fan_attr register as many sensor attr blocks as the specific
> model has.

Right, that's probably a nicer way of going about it, I think that might
be easier done in a separate patch (most likely preceding the addition
of F71808E support), though I'll look at that.

>> Signed-off-by: Giel van Schijndel<me@mortis.eu>
>> ---
>>   Documentation/hwmon/f71882fg |    4 ++
>>   drivers/hwmon/Kconfig        |    6 ++--
>>   drivers/hwmon/f71882fg.c     |   83 ++++++++++++++++++++++++++++++++++++++----
>>   3 files changed, 82 insertions(+), 11 deletions(-)
>> 
>> diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
>> index a7952c2..1a07fd6 100644
>> --- a/Documentation/hwmon/f71882fg
>> +++ b/Documentation/hwmon/f71882fg
>> @@ -2,6 +2,10 @@ Kernel driver f71882fg
>>   ======================
>> 
>>   Supported chips:
>> +  * Fintek F71808E
>> +    Prefix: 'f71808fg'
> 
> This is wrong, as you already indicate and the datasheet as well this
> chip in question is an f71808e not an f71808fg, also note that there is
> an f71808a model as well which is different (and has a different super io
> chip id).

Ah yes, I think I, wrongly, assumed that 'fg' was just some suffix used
in this driver.  For example, I cannot find F71889FG in the datasheet I
have, only 'F71889' along with 'F71889F' in the section "Ordering
Information" (for the F71889 I've got datasheet version V0.17P released
December 2008).

At the same time the F71808E datasheet I have clearly marks the chip as
F71808E all over the entire datasheet (version V0.17P released October
2009).

Either way I changed that ^^ portion of documentation while changing the
enumeration value 'f71808fg' -> 'f71808e' in the code itself as well.

> One last request in the second switch case in f71882fg_remove()
> there is a default label which contains a comment which models it applies
> to, please add the f71808e to that comment.

Wouldn't it be better, to instead replace that 'default' label with a
serie of case labels that code applies to?  Along with providing the
same documentation effect (expressed in C instead of English) it would
cause GCC to warn whenever one of the chips was forgotten in a switch
statement.  E.g. something similar to this patch:
========================================================================
diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index b891b65..bfb2b4c 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -2113,7 +2113,8 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
                                break;
                        }
                        /* fall through */
-               default: /* f71858fg / f71882fg */
+               case f71858fg:
+               case f71882fg:
                        err = f71882fg_create_sysfs_files(pdev,
                                &fxxxx_auto_pwm_attr[0][0],
                                ARRAY_SIZE(fxxxx_auto_pwm_attr[0]) * nr_fans);
@@ -2224,7 +2225,10 @@ static int f71882fg_remove(struct platform_device *pdev)
                                        f8000_auto_pwm_attr,
                                        ARRAY_SIZE(f8000_auto_pwm_attr));
                        break;
-               default: /* f71808e / f71858fg / f71882fg / f71889fg */
+               case f71808e:
+               case f71858fg:
+               case f71882fg:
+               case f71889fg:
                        f71882fg_remove_sysfs_files(pdev,
                                &fxxxx_auto_pwm_attr[0][0],
                                ARRAY_SIZE(fxxxx_auto_pwm_attr[0]) * nr_fans);
========================================================================

PS For comparison, which datasheet versions do you have?
All Fintek datasheets I have access to:
 * F71808E - V0.17P, October 2009
 * F71858  - V0.26P, July 2007
 * F71862  - V0.28P, July 2008
 * F71882  - V0.24P, November 2006
 * F71889  - V0.17P, December 2008

Those most interesting are of course the F71808E, F71862 and F71889---as
you refer to those in your text.  This because I have already had
experience with a hardware vendor giving me the wrong datasheets and
would like to prevent any such mistakes from causing similar
communication problems here.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [lm-sensors] [PATCH] hwmon: f71882fg: Add support for the

[Hans de Goede]

Hi,

On 08/04/2010 05:44 PM, Giel van Schijndel wrote:
> On Wed, Aug 04, 2010 at 13:36:22 +0200, Hans de Goede wrote:
>> On 08/01/2010 03:30, Giel van Schijndel wrote:
>>> Allow device probing to recognise the Fintek F71808E.
>>>
>>> Sysfs interface:
>>>    * Fan/pwm control is the same as for F71889FG
>>
>> My datasheet strongly disagrees with this the F71889FG has 5 pwm zones
>> each with their own speed divided by 4 boundary temps, where as
>> the F71808E has 3 pwm zones divided by 2 boundary temps. So it is much
>> more like the F71862FG, which also has 2 boundary temps, and 3 pwm zones,
>> *but* the F71862FG has one pwm zone hardwired to 100%.
>
> I'm assuming that by "pwm zone" you mean a separate PWM output channel?
> I.e. each "pwm zone" controls a single fan?
>

With pwm zone I mean the number of different speeds which can be programmed
for one output channel, the temps divide the entire temp range into zones
(number of zones = number of temps + 1) and for each zone one can then
tell at what speed / pwm setting the fan should operate when the temperature
is in that zone.


<snip>

>> Also while making changes, I must say I don't like the splitting
>> of fxxxx_temp_attr into fxxxx_temp_attr and f71862_temp_attr just because
>> the number of sensors differs. I think it would be better to instead
>> make fxxxx_temp_attr a 2 dimensional array like fxxxx_fan_attr and like
>> with fxxxx_fan_attr register as many sensor attr blocks as the specific
>> model has.
>
> Right, that's probably a nicer way of going about it, I think that might
> be easier done in a separate patch (most likely preceding the addition
> of F71808E support), though I'll look at that.
>

Yes first splitting the attr in a separate patch would be very good.

>>> Signed-off-by: Giel van Schijndel<me@mortis.eu>
>>> ---
>>>    Documentation/hwmon/f71882fg |    4 ++
>>>    drivers/hwmon/Kconfig        |    6 ++--
>>>    drivers/hwmon/f71882fg.c     |   83 ++++++++++++++++++++++++++++++++++++++----
>>>    3 files changed, 82 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/Documentation/hwmon/f71882fg b/Documentation/hwmon/f71882fg
>>> index a7952c2..1a07fd6 100644
>>> --- a/Documentation/hwmon/f71882fg
>>> +++ b/Documentation/hwmon/f71882fg
>>> @@ -2,6 +2,10 @@ Kernel driver f71882fg
>>>    ===========
>>>
>>>    Supported chips:
>>> +  * Fintek F71808E
>>> +    Prefix: 'f71808fg'
>>
>> This is wrong, as you already indicate and the datasheet as well this
>> chip in question is an f71808e not an f71808fg, also note that there is
>> an f71808a model as well which is different (and has a different super io
>> chip id).
>
> Ah yes, I think I, wrongly, assumed that 'fg' was just some suffix used
> in this driver.  For example, I cannot find F71889FG in the datasheet I
> have, only 'F71889' along with 'F71889F' in the section "Ordering
> Information" (for the F71889 I've got datasheet version V0.17P released
> December 2008).
>

I have a V0.27P datasheet for the 71889, but yes the fg suffix does not
seem to be mentioned anywhere in the datasheet not sure where it comes from.
I do know however that there are now new chips coming out with different
a and e suffixes so I suggest that we stay with fg for the old chips and
use a and e to distuingish the new ones.

> At the same time the F71808E datasheet I have clearly marks the chip as
> F71808E all over the entire datasheet (version V0.17P released October
> 2009).
>

Right.

> Either way I changed that ^^ portion of documentation while changing the
> enumeration value 'f71808fg' ->  'f71808e' in the code itself as well.
>

Good.

>> One last request in the second switch case in f71882fg_remove()
>> there is a default label which contains a comment which models it applies
>> to, please add the f71808e to that comment.
>
> Wouldn't it be better, to instead replace that 'default' label with a
> serie of case labels that code applies to?  Along with providing the
> same documentation effect (expressed in C instead of English) it would
> cause GCC to warn whenever one of the chips was forgotten in a switch
> statement.

Ack, if you could do that that would be great! Please do that
in a preparation patch though and not in the main patch.

<snip>

> PS For comparison, which datasheet versions do you have?
> All Fintek datasheets I have access to:
>   * F71808E - V0.17P, October 2009
>   * F71858  - V0.26P, July 2007
>   * F71862  - V0.28P, July 2008
>   * F71882  - V0.24P, November 2006
>   * F71889  - V0.17P, December 2008
>
> Those most interesting are of course the F71808E, F71862 and F71889---as
> you refer to those in your text.  This because I have already had
> experience with a hardware vendor giving me the wrong datasheets and
> would like to prevent any such mistakes from causing similar
> communication problems here.

Here is my list:
F71612A_V020P.pdf
F71808A_V0.15P.pdf
F71808E_V0.20P.pdf
F71858_V026P.pdf
F71862_V028P.pdf
F71869E_V0.19P.pdf
F71869_V1.1.pdf
F71882_V0.24P.pdf
F71889E_V0.19P.pdf
F71889_V0.27P.pdf
F8000_REG.pdf

Regards,

Hans

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

Re: [lm-sensors] [PATCH] hwmon: f71882fg: Add support for the

[Giel van Schijndel]

[-- Attachment #1.1: Type: text/plain, Size: 1393 bytes --]

On Wed, Aug 04, 2010 at 01:36:22PM +0200, Hans de Goede wrote:
> On 08/01/2010 03:30 PM, Giel van Schijndel wrote:
>> Allow device probing to recognise the Fintek F71808E.
>> 
>> Sysfs interface:
>>   * Fan/pwm control is the same as for F71889FG
> 
> My datasheet strongly disagrees with this ...

I just noticed this patch was applied to mainline anyway.  Regardless, I
will (try to) address these issues you raised.

Right now however, I am prioritising personal stuff above this
driver---bachelor's thesis and graduation presentation.  When finished
with that (september) I'll allocate time to these issues again.

PS Those datasheets are written very poorly, although I have seen worse.
   And as a reader I tend to become more "lazy" when I discover the
   writer was "lazy" (not exactly by choice, more out of habit).
   Unfortunately that doesn't work very well as reading style with
   technical documentation, so this probably explains some of the errors
   in this current patch.

PPS I do have a patch ready that addresses some of the issues you
    raised.  Those are only the cosmetic changes though (e.g. change
    naming of chip, comment updates, etc.).  Would you suggest me to
    send it right now already or wait until I have time to address the
    other issues as well?

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: 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: f71882fg: Add support for the

[Hans de Goede]

Hi,

On 08/10/2010 09:11 PM, Giel van Schijndel wrote:
> On Wed, Aug 04, 2010 at 01:36:22PM +0200, Hans de Goede wrote:
>> On 08/01/2010 03:30 PM, Giel van Schijndel wrote:
>>> Allow device probing to recognise the Fintek F71808E.
>>>
>>> Sysfs interface:
>>>    * Fan/pwm control is the same as for F71889FG
>>
>> My datasheet strongly disagrees with this ...
>
> I just noticed this patch was applied to mainline anyway.  Regardless, I
> will (try to) address these issues you raised.
>
> Right now however, I am prioritising personal stuff above this
> driver---bachelor's thesis and graduation presentation.  When finished
> with that (september) I'll allocate time to these issues again.
>

I've send a mail directly to akpm asking for this to be removed, hopefully
this will help.

> PPS I do have a patch ready that addresses some of the issues you
>      raised.  Those are only the cosmetic changes though (e.g. change
>      naming of chip, comment updates, etc.).  Would you suggest me to
>      send it right now already or wait until I have time to address the
>      other issues as well?

My goal for 2.6.36 is to pull the patch, and then we can wait til all
issues are fixed properly before re-introducing it.

Regards,

Hans

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

Re: [lm-sensors] [PATCH] hwmon: f71882fg: Add support for the

[Andrew Morton]

On Fri, 13 Aug 2010 12:01:36 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 08/10/2010 09:11 PM, Giel van Schijndel wrote:
> > On Wed, Aug 04, 2010 at 01:36:22PM +0200, Hans de Goede wrote:
> >> On 08/01/2010 03:30 PM, Giel van Schijndel wrote:
> >>> Allow device probing to recognise the Fintek F71808E.
> >>>
> >>> Sysfs interface:
> >>>    * Fan/pwm control is the same as for F71889FG
> >>
> >> My datasheet strongly disagrees with this ...
> >
> > I just noticed this patch was applied to mainline anyway.  Regardless, I
> > will (try to) address these issues you raised.
> >
> > Right now however, I am prioritising personal stuff above this
> > driver---bachelor's thesis and graduation presentation.  When finished
> > with that (september) I'll allocate time to these issues again.
> >
> 
> I've send a mail directly to akpm asking for this to be removed, hopefully
> this will help.

It seems that I managed to muck up my paperwork and the patch was
merged into mainline.

We can revert it, or we can fix it up in place in time for 2.6.36. 
What do you guys suggest?


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

Re: [lm-sensors] [PATCH] hwmon: f71882fg: Add support for the

[Hans de Goede]

Hi,

On 08/18/2010 08:24 PM, Andrew Morton wrote:
> On Fri, 13 Aug 2010 12:01:36 +0200
> Hans de Goede<hdegoede@redhat.com>  wrote:
>
>> Hi,
>>
>> On 08/10/2010 09:11 PM, Giel van Schijndel wrote:
>>> On Wed, Aug 04, 2010 at 01:36:22PM +0200, Hans de Goede wrote:
>>>> On 08/01/2010 03:30 PM, Giel van Schijndel wrote:
>>>>> Allow device probing to recognise the Fintek F71808E.
>>>>>
>>>>> Sysfs interface:
>>>>>     * Fan/pwm control is the same as for F71889FG
>>>>
>>>> My datasheet strongly disagrees with this ...
>>>
>>> I just noticed this patch was applied to mainline anyway.  Regardless, I
>>> will (try to) address these issues you raised.
>>>
>>> Right now however, I am prioritising personal stuff above this
>>> driver---bachelor's thesis and graduation presentation.  When finished
>>> with that (september) I'll allocate time to these issues again.
>>>
>>
>> I've send a mail directly to akpm asking for this to be removed, hopefully
>> this will help.
>
> It seems that I managed to muck up my paperwork and the patch was
> merged into mainline.
>
> We can revert it, or we can fix it up in place in time for 2.6.36.
> What do you guys suggest?

Since both Giel and I are low on time to work on this, and since missing
hwmon support on a system is not really a big deal (for most users) I suggest
that this patch is reverted until we find the time to do it properly.

Regards,

Hans

p.s.

I've already seen mails that you've dropped this patch from -mm, does that
mean it thas been removed from mainly too ? If not please remove it from
mainline / ask Linus to remove it.

Thanks.

Hans

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

Re: [lm-sensors] [PATCH] hwmon: f71882fg: Add support for the

[Giel van Schijndel]

[-- Attachment #1.1: Type: text/plain, Size: 445 bytes --]

On Sun, Aug 22, 2010 at 08:04:38PM +0200, Hans de Goede wrote:
> p.s.
> 
> I've already seen mails that you've dropped this patch from -mm, does that
> mean it thas been removed from mainly too ? If not please remove it from
> mainline / ask Linus to remove it.

Checking .../torvalds/linux-2.6.git you can see it's been reverted in
mainline (see commit f2e41e9).

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

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