2010-07-31 23:31:26

by Giel van Schijndel

[permalink] [raw]
Subject: Re: [PATCH 1/4] hwmon: f71882fg: Add support for the Fintek F71808E

On Wed, Mar 24, 2010 at 11:31:40 +0100, Hans de Goede wrote:
> On 03/24/2010 10:23, Giel van Schijndel wrote:
>> On Wed, Mar 24, 2010 at 09:25:08 +0100, Hans de Goede wrote:
>>> See comments inline.
>>>
>>> On 03/24/2010 12:12 AM, Giel van Schijndel wrote:
>>>> 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.
>>>>---
>>>> Documentation/hwmon/f71882fg | 4 ++
>>>> drivers/hwmon/Kconfig | 6 ++--
>>>> drivers/hwmon/f71882fg.c | 80 +++++++++++++++++++++++++++++++++++++----
>>>> 3 files changed, 79 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
>>>> index 25e1cad..b290b87 100644
>>>> --- a/drivers/hwmon/f71882fg.c
>>>> +++ b/drivers/hwmon/f71882fg.c
>>>> @@ -1974,8 +2002,27 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
>>>> ... snip ...
>>>> + /* fall through! */
>>>
>>> Ugh, please don't fall through, and then have an if below to only do
>>> some parts of the case falling through. This is quite confusing at
>>> first I thought your code was buggy I had to read it twice to notice
>>> the if. Instead just duplicate the following lines:
>>>> + err = f71882fg_create_sysfs_files(pdev,
>>>> + fxxxx_temp_attr,
>>>> + ARRAY_SIZE(fxxxx_temp_attr));
>>> In the f71862fg case, end the f71862fg case with a break and remove
>>> the if test from the f71808fg case.
>>>
>>>>+ case f71808fg:
>>>>+ if (data->type == 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));
>>>> ... snip ...
>>
>> 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 <[email protected]>

Thanks. Anyone else I need to poke in order to set this on track to
mainline?

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


Attachments:
(No filename) (2.62 kB)
signature.asc (198.00 B)
Digital signature
Download all attachments

2010-08-01 06:13:56

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH 1/4] hwmon: f71882fg: Add support for the Fintek F71808E

Hi,

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:
>>> On Wed, Mar 24, 2010 at 09:25:08 +0100, Hans de Goede wrote:
>>>> See comments inline.
>>>>
>>>> On 03/24/2010 12:12 AM, Giel van Schijndel wrote:
>>>>> 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.
>>>>> ---
>>>>> Documentation/hwmon/f71882fg | 4 ++
>>>>> drivers/hwmon/Kconfig | 6 ++--
>>>>> drivers/hwmon/f71882fg.c | 80 +++++++++++++++++++++++++++++++++++++----
>>>>> 3 files changed, 79 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
>>>>> index 25e1cad..b290b87 100644
>>>>> --- a/drivers/hwmon/f71882fg.c
>>>>> +++ b/drivers/hwmon/f71882fg.c
>>>>> @@ -1974,8 +2002,27 @@ static int __devinit f71882fg_probe(struct platform_device *pdev)
>>>>> ... snip ...
>>>>> + /* fall through! */
>>>>
>>>> Ugh, please don't fall through, and then have an if below to only do
>>>> some parts of the case falling through. This is quite confusing at
>>>> first I thought your code was buggy I had to read it twice to notice
>>>> the if. Instead just duplicate the following lines:
>>>>> + err = f71882fg_create_sysfs_files(pdev,
>>>>> + fxxxx_temp_attr,
>>>>> + ARRAY_SIZE(fxxxx_temp_attr));
>>>> In the f71862fg case, end the f71862fg case with a break and remove
>>>> the if test from the f71808fg case.
>>>>
>>>>> + case f71808fg:
>>>>> + if (data->type == 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));
>>>>> ... snip ...
>>>
>>> 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<[email protected]>
>
> 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.

Thanks & Regards,

Hans

2010-08-01 13:22:32

by Giel van Schijndel

[permalink] [raw]
Subject: Re: [PATCH 1/4] hwmon: f71882fg: Add support for the Fintek F71808E

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<[email protected]>
>>
>> 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


Attachments:
(No filename) (0.98 kB)
signature.asc (198.00 B)
Digital signature
Download all attachments

2010-08-01 13:30:10

by Giel van Schijndel

[permalink] [raw]
Subject: [PATCH] hwmon: f71882fg: Add support for the Fintek F71808E

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 <[email protected]>
---
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

2010-08-01 13:31:29

by Giel van Schijndel

[permalink] [raw]
Subject: [PATCH 1/2] hwmon: f71882fg: use a muxed resource lock for the Super I/O port

Sleep while acquiring a resource lock on the Super I/O port. This should
prevent collisions from causing the hardware probe to fail with -EBUSY.

Signed-off-by: Giel van Schijndel <[email protected]>
---
drivers/hwmon/f71882fg.c | 32 +++++++++++++++++++-------------
1 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
index 7857ed3..267cb92 100644
--- a/drivers/hwmon/f71882fg.c
+++ b/drivers/hwmon/f71882fg.c
@@ -113,7 +113,7 @@ static struct platform_device *f71882fg_pdev;
/* Super-I/O Function prototypes */
static inline int superio_inb(int base, int reg);
static inline int superio_inw(int base, int reg);
-static inline void superio_enter(int base);
+static inline int superio_enter(int base);
static inline void superio_select(int base, int ld);
static inline void superio_exit(int base);

@@ -883,11 +883,20 @@ static int superio_inw(int base, int reg)
return val;
}

-static inline void superio_enter(int base)
+static inline int superio_enter(int base)
{
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_muxed_region(base, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ base);
+ return -EBUSY;
+ }
+
/* according to the datasheet the key must be send twice! */
outb(SIO_UNLOCK_KEY, base);
outb(SIO_UNLOCK_KEY, base);
+
+ return 0;
}

static inline void superio_select(int base, int ld)
@@ -899,6 +908,7 @@ static inline void superio_select(int base, int ld)
static inline void superio_exit(int base)
{
outb(SIO_LOCK_KEY, base);
+ release_region(base, 2);
}

static inline int fan_from_reg(u16 reg)
@@ -2239,21 +2249,15 @@ static int f71882fg_remove(struct platform_device *pdev)
static int __init f71882fg_find(int sioaddr, unsigned short *address,
struct f71882fg_sio_data *sio_data)
{
- int err = -ENODEV;
u16 devid;
-
- /* Don't step on other drivers' I/O space by accident */
- if (!request_region(sioaddr, 2, DRVNAME)) {
- printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
- (int)sioaddr);
- return -EBUSY;
- }
-
- superio_enter(sioaddr);
+ int err = superio_enter(sioaddr);
+ if (err)
+ return err;

devid = superio_inw(sioaddr, SIO_REG_MANID);
if (devid != SIO_FINTEK_ID) {
pr_debug(DRVNAME ": Not a Fintek device\n");
+ err = -ENODEV;
goto exit;
}

@@ -2280,6 +2284,7 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
default:
printk(KERN_INFO DRVNAME ": Unsupported Fintek device: %04x\n",
(unsigned int)devid);
+ err = -ENODEV;
goto exit;
}

@@ -2290,12 +2295,14 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,

if (!(superio_inb(sioaddr, SIO_REG_ENABLE) & 0x01)) {
printk(KERN_WARNING DRVNAME ": Device not activated\n");
+ err = -ENODEV;
goto exit;
}

*address = superio_inw(sioaddr, SIO_REG_ADDR);
if (*address == 0) {
printk(KERN_WARNING DRVNAME ": Base address not set\n");
+ err = -ENODEV;
goto exit;
}
*address &= ~(REGION_LENGTH - 1); /* Ignore 3 LSB */
@@ -2306,7 +2313,6 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
(int)superio_inb(sioaddr, SIO_REG_DEVREV));
exit:
superio_exit(sioaddr);
- release_region(sioaddr, 2);
return err;
}

--
1.6.4.4

2010-08-01 13:31:42

by Giel van Schijndel

[permalink] [raw]
Subject: [PATCH 2/2] watchdog: f71808e_wdt: new watchdog driver for Fintek F71808E and F71882FG

Add a new watchdog driver for the Fintek F71808E and F71882FG Super I/O
controllers.

Signed-off-by: Giel van Schijndel <[email protected]>
---
drivers/watchdog/Kconfig | 11 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/f71808e_wdt.c | 768 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 780 insertions(+), 0 deletions(-)
create mode 100644 drivers/watchdog/f71808e_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index afcfacc..edfd57d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -401,6 +401,17 @@ config ALIM7101_WDT

Most people will say N.

+config F71808E_WDT
+ tristate "Fintek F71808E and F71882FG Watchdog"
+ depends on X86 && EXPERIMENTAL
+ help
+ This is the driver for the hardware watchdog on the Fintek
+ F71808E and F71882FG Super I/O controllers.
+
+ You can compile this driver directly into the kernel, or use
+ it as a module. The module will be called f71808e_wdt.
+
+
config GEODE_WDT
tristate "AMD Geode CS5535/CS5536 Watchdog"
depends on CS5535_MFGPT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 72f3e20..9e3d616 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
+obj-$(CONFIG_F71808E_WDT) += f71808e_wdt.o
obj-$(CONFIG_GEODE_WDT) += geodewdt.o
obj-$(CONFIG_SC520_WDT) += sc520_wdt.o
obj-$(CONFIG_SBC_FITPC2_WATCHDOG) += sbc_fitpc2_wdt.o
diff --git a/drivers/watchdog/f71808e_wdt.c b/drivers/watchdog/f71808e_wdt.c
new file mode 100644
index 0000000..7e5c266
--- /dev/null
+++ b/drivers/watchdog/f71808e_wdt.c
@@ -0,0 +1,768 @@
+/***************************************************************************
+ * Copyright (C) 2006 by Hans Edgington <[email protected]> *
+ * Copyright (C) 2007-2009 Hans de Goede <[email protected]> *
+ * Copyright (C) 2010 Giel van Schijndel <[email protected]> *
+ * *
+ * This program is free software; you can redistribute it and/or modify *
+ * it under the terms of the GNU General Public License as published by *
+ * the Free Software Foundation; either version 2 of the License, or *
+ * (at your option) any later version. *
+ * *
+ * This program is distributed in the hope that it will be useful, *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
+ * GNU General Public License for more details. *
+ * *
+ * You should have received a copy of the GNU General Public License *
+ * along with this program; if not, write to the *
+ * Free Software Foundation, Inc., *
+ * 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. *
+ ***************************************************************************/
+
+#include <linux/err.h>
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+
+#define DRVNAME "f71808e_wdt"
+
+#define SIO_F71808FG_LD_WDT 0x07 /* Watchdog timer logical device */
+#define SIO_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */
+#define SIO_LOCK_KEY 0xAA /* Key to diasble Super-I/O */
+
+#define SIO_REG_LDSEL 0x07 /* Logical device select */
+#define SIO_REG_DEVID 0x20 /* Device ID (2 bytes) */
+#define SIO_REG_DEVREV 0x22 /* Device revision */
+#define SIO_REG_MANID 0x23 /* Fintek ID (2 bytes) */
+#define SIO_REG_ENABLE 0x30 /* Logical device enable */
+#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 */
+#define SIO_F71889_ID 0x0723 /* Chipset ID */
+
+#define F71882FG_REG_START 0x01
+
+#define F71808FG_REG_WDO_CONF 0xf0
+#define F71808FG_REG_WDT_CONF 0xf5
+#define F71808FG_REG_WD_TIME 0xf6
+
+#define F71808FG_FLAG_WDOUT_EN 7
+
+#define F71808FG_FLAG_WDTMOUT_STS 5
+#define F71808FG_FLAG_WD_EN 5
+#define F71808FG_FLAG_WD_PULSE 4
+#define F71808FG_FLAG_WD_UNIT 3
+
+/* Default values */
+#define WATCHDOG_TIMEOUT 60 /* 1 minute default timeout */
+#define WATCHDOG_MAX_TIMEOUT (60 * 255)
+#define WATCHDOG_PULSE_WIDTH 125 /* 125 ms, default pulse width for
+ watchdog signal */
+
+static unsigned short force_id;
+module_param(force_id, ushort, 0);
+MODULE_PARM_DESC(force_id, "Override the detected device ID");
+
+static const int max_timeout = WATCHDOG_MAX_TIMEOUT;
+static int timeout = 60; /* default timeout in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout,
+ "Watchdog timeout in seconds. 1<= timeout <="
+ __MODULE_STRING(WATCHDOG_MAX_TIMEOUT) " (default="
+ __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
+
+static unsigned int pulse_width = WATCHDOG_PULSE_WIDTH;
+module_param(pulse_width, uint, 0);
+MODULE_PARM_DESC(pulse_width,
+ "Watchdog signal pulse width. 0(=level), 1 ms, 25 ms, 125 ms or 5000 ms"
+ " (default=" __MODULE_STRING(WATCHDOG_PULSE_WIDTH) ")");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0444);
+MODULE_PARM_DESC(nowayout, "Disable watchdog shutdown on close");
+
+static unsigned int start_withtimeout;
+module_param(start_withtimeout, uint, 0);
+MODULE_PARM_DESC(start_withtimeout, "Start watchdog timer on module load with"
+ " given initial timeout. Zero (default) disables this feature.");
+
+enum chips { f71808fg, f71858fg, f71862fg, f71882fg, f71889fg };
+
+static const char *f71808e_names[] = {
+ "f71808fg",
+ "f71858fg",
+ "f71862fg",
+ "f71882fg",
+ "f71889fg",
+};
+
+/* Super-I/O Function prototypes */
+static inline int superio_inb(int base, int reg);
+static inline int superio_inw(int base, int reg);
+static inline void superio_outb(int base, int reg, u8 val);
+static inline void superio_set_bit(int base, int reg, int bit);
+static inline void superio_clear_bit(int base, int reg, int bit);
+static inline int superio_enter(int base);
+static inline void superio_select(int base, int ld);
+static inline void superio_exit(int base);
+
+struct watchdog_data {
+ unsigned short sioaddr;
+ enum chips type;
+ unsigned long opened;
+ struct mutex lock;
+ char expect_close;
+ struct watchdog_info ident;
+
+ unsigned short timeout;
+ u8 timer_val; /* content for the wd_time register */
+ char minutes_mode;
+ u8 pulse_val; /* pulse width flag */
+ char pulse_mode; /* enable pulse output mode? */
+ char caused_reboot; /* last reboot was by the watchdog */
+};
+
+static struct watchdog_data watchdog = {
+ .lock = __MUTEX_INITIALIZER(watchdog.lock),
+};
+
+/* Super I/O functions */
+static inline int superio_inb(int base, int reg)
+{
+ outb(reg, base);
+ return inb(base + 1);
+}
+
+static int superio_inw(int base, int reg)
+{
+ int val;
+ val = superio_inb(base, reg) << 8;
+ val |= superio_inb(base, reg + 1);
+ return val;
+}
+
+static inline void superio_outb(int base, int reg, u8 val)
+{
+ outb(reg, base);
+ outb(val, base + 1);
+}
+
+static inline void superio_set_bit(int base, int reg, int bit)
+{
+ unsigned long val = superio_inb(base, reg);
+ __set_bit(bit, &val);
+ superio_outb(base, reg, val);
+}
+
+static inline void superio_clear_bit(int base, int reg, int bit)
+{
+ unsigned long val = superio_inb(base, reg);
+ __clear_bit(bit, &val);
+ superio_outb(base, reg, val);
+}
+
+static inline int superio_enter(int base)
+{
+ /* Don't step on other drivers' I/O space by accident */
+ if (!request_muxed_region(base, 2, DRVNAME)) {
+ printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
+ (int)base);
+ return -EBUSY;
+ }
+
+ /* according to the datasheet the key must be send twice! */
+ outb(SIO_UNLOCK_KEY, base);
+ outb(SIO_UNLOCK_KEY, base);
+
+ return 0;
+}
+
+static inline void superio_select(int base, int ld)
+{
+ outb(SIO_REG_LDSEL, base);
+ outb(ld, base + 1);
+}
+
+static inline void superio_exit(int base)
+{
+ outb(SIO_LOCK_KEY, base);
+ release_region(base, 2);
+}
+
+static int watchdog_set_timeout(int timeout)
+{
+ if (timeout <= 0
+ || timeout > max_timeout) {
+ printk(KERN_ERR DRVNAME ": watchdog timeout out of range\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&watchdog.lock);
+
+ watchdog.timeout = timeout;
+ if (timeout > 0xff) {
+ watchdog.timer_val = DIV_ROUND_UP(timeout, 60);
+ watchdog.minutes_mode = true;
+ } else {
+ watchdog.timer_val = timeout;
+ watchdog.minutes_mode = false;
+ }
+
+ mutex_unlock(&watchdog.lock);
+
+ return 0;
+}
+
+static int watchdog_set_pulse_width(unsigned int pw)
+{
+ int err = 0;
+
+ mutex_lock(&watchdog.lock);
+
+ if (pw <= 1) {
+ watchdog.pulse_val = 0;
+ } else if (pw <= 25) {
+ watchdog.pulse_val = 1;
+ } else if (pw <= 125) {
+ watchdog.pulse_val = 2;
+ } else if (pw <= 5000) {
+ watchdog.pulse_val = 3;
+ } else {
+ printk(KERN_ERR DRVNAME ": pulse width out of range\n");
+ err = -EINVAL;
+ goto exit_unlock;
+ }
+
+ watchdog.pulse_mode = pw;
+
+exit_unlock:
+ mutex_unlock(&watchdog.lock);
+ return err;
+}
+
+static int watchdog_keepalive(void)
+{
+ int err = 0;
+
+ mutex_lock(&watchdog.lock);
+ err = superio_enter(watchdog.sioaddr);
+ if (err)
+ goto exit_unlock;
+ superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+
+ if (watchdog.minutes_mode)
+ /* select minutes for timer units */
+ superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
+ F71808FG_FLAG_WD_UNIT);
+ else
+ /* select seconds for timer units */
+ superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
+ F71808FG_FLAG_WD_UNIT);
+
+ /* Set timer value */
+ superio_outb(watchdog.sioaddr, F71808FG_REG_WD_TIME,
+ watchdog.timer_val);
+
+ superio_exit(watchdog.sioaddr);
+
+exit_unlock:
+ mutex_unlock(&watchdog.lock);
+ return err;
+}
+
+static int watchdog_start(void)
+{
+ /* Make sure we don't die as soon as the watchdog is enabled below */
+ int err = watchdog_keepalive();
+ if (err)
+ return err;
+
+ mutex_lock(&watchdog.lock);
+ err = superio_enter(watchdog.sioaddr);
+ if (err)
+ goto exit_unlock;
+ superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+
+ /* Watchdog pin configuration */
+ switch (watchdog.type) {
+ case f71808fg:
+ /* Set pin 21 to GPIO23/WDTRST#, then to WDTRST# */
+ superio_clear_bit(watchdog.sioaddr, 0x2a, 3);
+ superio_clear_bit(watchdog.sioaddr, 0x2b, 3);
+ break;
+
+ case f71882fg:
+ /* Set pin 56 to WDTRST# */
+ superio_set_bit(watchdog.sioaddr, 0x29, 1);
+ break;
+
+ default:
+ /*
+ * 'default' label to shut up the compiler and catch
+ * programmer errors
+ */
+ err = -ENODEV;
+ goto exit_superio;
+ }
+
+ superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+ superio_set_bit(watchdog.sioaddr, SIO_REG_ENABLE, 0);
+ superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDO_CONF,
+ F71808FG_FLAG_WDOUT_EN);
+
+ superio_set_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
+ F71808FG_FLAG_WD_EN);
+
+ if (watchdog.pulse_mode) {
+ /* Select "pulse" output mode with given duration */
+ u8 wdt_conf = superio_inb(watchdog.sioaddr,
+ F71808FG_REG_WDT_CONF);
+
+ /* Set WD_PSWIDTH bits (1:0) */
+ wdt_conf = (wdt_conf & 0xfc) | (watchdog.pulse_val & 0x03);
+ /* Set WD_PULSE to "pulse" mode */
+ wdt_conf |= BIT(F71808FG_FLAG_WD_PULSE);
+
+ superio_outb(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
+ wdt_conf);
+ } else {
+ /* Select "level" output mode */
+ superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
+ F71808FG_FLAG_WD_PULSE);
+ }
+
+exit_superio:
+ superio_exit(watchdog.sioaddr);
+exit_unlock:
+ mutex_unlock(&watchdog.lock);
+
+ return err;
+}
+
+static int watchdog_stop(void)
+{
+ int err = 0;
+
+ mutex_lock(&watchdog.lock);
+ err = superio_enter(watchdog.sioaddr);
+ if (err)
+ goto exit_unlock;
+ superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+
+ superio_clear_bit(watchdog.sioaddr, F71808FG_REG_WDT_CONF,
+ F71808FG_FLAG_WD_EN);
+
+ superio_exit(watchdog.sioaddr);
+
+exit_unlock:
+ mutex_unlock(&watchdog.lock);
+
+ return err;
+}
+
+static int watchdog_get_status(void)
+{
+ int status = 0;
+
+ mutex_lock(&watchdog.lock);
+ status = (watchdog.caused_reboot) ? WDIOF_CARDRESET : 0;
+ mutex_unlock(&watchdog.lock);
+
+ return status;
+}
+
+static bool watchdog_is_running(void)
+{
+ /*
+ * if we fail to determine the watchdog's status assume it to be
+ * running to be on the safe side
+ */
+ bool is_running = true;
+
+ mutex_lock(&watchdog.lock);
+ if (superio_enter(watchdog.sioaddr))
+ goto exit_unlock;
+ superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+
+ is_running = (superio_inb(watchdog.sioaddr, SIO_REG_ENABLE) & BIT(0))
+ && (superio_inb(watchdog.sioaddr, F71808FG_REG_WDT_CONF)
+ & F71808FG_FLAG_WD_EN);
+
+ superio_exit(watchdog.sioaddr);
+
+exit_unlock:
+ mutex_unlock(&watchdog.lock);
+ return is_running;
+}
+
+/* /dev/watchdog api */
+
+static int watchdog_open(struct inode *inode, struct file *file)
+{
+ int err;
+
+ /* If the watchdog is alive we don't need to start it again */
+ if (test_and_set_bit(0, &watchdog.opened))
+ return -EBUSY;
+
+ err = watchdog_start();
+ if (err) {
+ clear_bit(0, &watchdog.opened);
+ return err;
+ }
+
+ if (nowayout)
+ __module_get(THIS_MODULE);
+
+ watchdog.expect_close = 0;
+ return nonseekable_open(inode, file);
+}
+
+static int watchdog_release(struct inode *inode, struct file *file)
+{
+ clear_bit(0, &watchdog.opened);
+
+ if (!watchdog.expect_close) {
+ watchdog_keepalive();
+ printk(KERN_CRIT DRVNAME
+ ": Unexpected close, not stopping watchdog!\n");
+ } else if (!nowayout) {
+ watchdog_stop();
+ }
+ return 0;
+}
+
+/*
+ * watchdog_write:
+ * @file: file handle to the watchdog
+ * @buf: buffer to write
+ * @count: count of bytes
+ * @ppos: pointer to the position to write. No seeks allowed
+ *
+ * A write to a watchdog device is defined as a keepalive signal. Any
+ * write of data will do, as we we don't define content meaning.
+ */
+
+static ssize_t watchdog_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ if (count) {
+ if (!nowayout) {
+ size_t i;
+
+ /* In case it was set long ago */
+ bool expect_close = false;
+
+ for (i = 0; i != count; i++) {
+ char c;
+ if (get_user(c, buf + i))
+ return -EFAULT;
+ expect_close = (c == 'V');
+ }
+
+ /* Properly order writes across fork()ed processes */
+ mutex_lock(&watchdog.lock);
+ watchdog.expect_close = expect_close;
+ mutex_unlock(&watchdog.lock);
+ }
+
+ /* someone wrote to us, we should restart timer */
+ watchdog_keepalive();
+ }
+ return count;
+}
+
+/*
+ * watchdog_ioctl:
+ * @inode: inode of the device
+ * @file: file handle to the device
+ * @cmd: watchdog command
+ * @arg: argument pointer
+ *
+ * The watchdog API defines a common set of functions for all watchdogs
+ * according to their available features.
+ */
+static long watchdog_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ int status;
+ int new_options;
+ int new_timeout;
+ union {
+ struct watchdog_info __user *ident;
+ int __user *i;
+ } uarg;
+
+ uarg.i = (int __user *)arg;
+
+ switch (cmd) {
+ case WDIOC_GETSUPPORT:
+ return copy_to_user(uarg.ident, &watchdog.ident,
+ sizeof(watchdog.ident)) ? -EFAULT : 0;
+
+ case WDIOC_GETSTATUS:
+ status = watchdog_get_status();
+ if (status < 0)
+ return status;
+ return put_user(status, uarg.i);
+
+ case WDIOC_GETBOOTSTATUS:
+ return put_user(0, uarg.i);
+
+ case WDIOC_SETOPTIONS:
+ if (get_user(new_options, uarg.i))
+ return -EFAULT;
+
+ if (new_options & WDIOS_DISABLECARD)
+ watchdog_stop();
+
+ if (new_options & WDIOS_ENABLECARD)
+ return watchdog_start();
+
+
+ case WDIOC_KEEPALIVE:
+ watchdog_keepalive();
+ return 0;
+
+ case WDIOC_SETTIMEOUT:
+ if (get_user(new_timeout, uarg.i))
+ return -EFAULT;
+
+ if (watchdog_set_timeout(new_timeout))
+ return -EINVAL;
+
+ watchdog_keepalive();
+ /* Fall */
+
+ case WDIOC_GETTIMEOUT:
+ return put_user(watchdog.timeout, uarg.i);
+
+ default:
+ return -ENOTTY;
+
+ }
+}
+
+static int watchdog_notify_sys(struct notifier_block *this, unsigned long code,
+ void *unused)
+{
+ if (code == SYS_DOWN || code == SYS_HALT)
+ watchdog_stop();
+ return NOTIFY_DONE;
+}
+
+static const struct file_operations watchdog_fops = {
+ .owner = THIS_MODULE,
+ .llseek = no_llseek,
+ .open = watchdog_open,
+ .release = watchdog_release,
+ .write = watchdog_write,
+ .unlocked_ioctl = watchdog_ioctl,
+};
+
+static struct miscdevice watchdog_miscdev = {
+ .minor = WATCHDOG_MINOR,
+ .name = "watchdog",
+ .fops = &watchdog_fops,
+};
+
+static struct notifier_block watchdog_notifier = {
+ .notifier_call = watchdog_notify_sys,
+};
+
+static int __init watchdog_init(int sioaddr)
+{
+ int wdt_conf, err = 0;
+
+ /* No need to lock watchdog.lock here because no entry points
+ * into the module have been registered yet.
+ */
+ watchdog.sioaddr = sioaddr;
+ watchdog.ident.options = WDIOC_SETTIMEOUT
+ | WDIOF_MAGICCLOSE
+ | WDIOF_KEEPALIVEPING;
+
+ snprintf(watchdog.ident.identity,
+ sizeof(watchdog.ident.identity), "%s watchdog",
+ f71808e_names[watchdog.type]);
+
+ err = superio_enter(sioaddr);
+ if (err)
+ return err;
+ superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+
+ wdt_conf = superio_inb(sioaddr, F71808FG_REG_WDT_CONF);
+ watchdog.caused_reboot = wdt_conf & F71808FG_FLAG_WDTMOUT_STS;
+
+ superio_exit(sioaddr);
+
+ err = watchdog_set_timeout(timeout);
+ if (err)
+ return err;
+ err = watchdog_set_pulse_width(pulse_width);
+ if (err)
+ return err;
+
+ err = register_reboot_notifier(&watchdog_notifier);
+ if (err)
+ return err;
+
+ err = misc_register(&watchdog_miscdev);
+ if (err) {
+ printk(KERN_ERR DRVNAME
+ ": cannot register miscdev on minor=%d\n",
+ watchdog_miscdev.minor);
+ goto exit_reboot;
+ }
+
+ if (start_withtimeout) {
+ if (start_withtimeout <= 0
+ || start_withtimeout > max_timeout) {
+ printk(KERN_ERR DRVNAME
+ ": starting timeout out of range\n");
+ err = -EINVAL;
+ goto exit_miscdev;
+ }
+
+ err = watchdog_start();
+ if (err) {
+ printk(KERN_ERR DRVNAME
+ ": cannot start watchdog timer\n");
+ goto exit_miscdev;
+ }
+
+ mutex_lock(&watchdog.lock);
+ err = superio_enter(sioaddr);
+ if (err)
+ goto exit_unlock;
+ superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+
+ if (start_withtimeout > 0xff) {
+ /* select minutes for timer units */
+ superio_set_bit(sioaddr, F71808FG_REG_WDT_CONF,
+ F71808FG_FLAG_WD_UNIT);
+ superio_outb(sioaddr, F71808FG_REG_WD_TIME,
+ DIV_ROUND_UP(start_withtimeout, 60));
+ } else {
+ /* select seconds for timer units */
+ superio_clear_bit(sioaddr, F71808FG_REG_WDT_CONF,
+ F71808FG_FLAG_WD_UNIT);
+ superio_outb(sioaddr, F71808FG_REG_WD_TIME,
+ start_withtimeout);
+ }
+
+ superio_exit(sioaddr);
+ mutex_unlock(&watchdog.lock);
+
+ if (nowayout)
+ __module_get(THIS_MODULE);
+
+ printk(KERN_INFO DRVNAME
+ ": watchdog started with initial timeout of %u sec\n",
+ start_withtimeout);
+ }
+
+ return 0;
+
+exit_unlock:
+ mutex_unlock(&watchdog.lock);
+exit_miscdev:
+ misc_deregister(&watchdog_miscdev);
+exit_reboot:
+ unregister_reboot_notifier(&watchdog_notifier);
+
+ return err;
+}
+
+static int __init f71808e_find(int sioaddr)
+{
+ u16 devid;
+ int err = superio_enter(sioaddr);
+ if (err)
+ return err;
+
+ devid = superio_inw(sioaddr, SIO_REG_MANID);
+ if (devid != SIO_FINTEK_ID) {
+ pr_debug(DRVNAME ": Not a Fintek device\n");
+ err = -ENODEV;
+ goto exit;
+ }
+
+ devid = force_id ? force_id : superio_inw(sioaddr, SIO_REG_DEVID);
+ switch (devid) {
+ case SIO_F71808_ID:
+ watchdog.type = f71808fg;
+ break;
+ case SIO_F71882_ID:
+ watchdog.type = f71882fg;
+ break;
+ case SIO_F71862_ID:
+ case SIO_F71889_ID:
+ /* These have a watchdog, though it isn't implemented (yet). */
+ err = -ENOSYS;
+ goto exit;
+ case SIO_F71858_ID:
+ /* Confirmed (by datasheet) not to have a watchdog. */
+ err = -ENODEV;
+ goto exit;
+ default:
+ printk(KERN_INFO DRVNAME ": Unrecognized Fintek device: %04x\n",
+ (unsigned int)devid);
+ err = -ENODEV;
+ goto exit;
+ }
+
+ printk(KERN_INFO DRVNAME ": Found %s watchdog chip, revision %d\n",
+ f71808e_names[watchdog.type],
+ (int)superio_inb(sioaddr, SIO_REG_DEVREV));
+exit:
+ superio_exit(sioaddr);
+ return err;
+}
+
+static int __init f71808e_init(void)
+{
+ static const unsigned short addrs[] = { 0x2e, 0x4e };
+ int err = -ENODEV;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(addrs); i++) {
+ err = f71808e_find(addrs[i]);
+ if (err == 0)
+ break;
+ }
+ if (i == ARRAY_SIZE(addrs))
+ return err;
+
+ return watchdog_init(addrs[i]);
+}
+
+static void __exit f71808e_exit(void)
+{
+ if (watchdog_is_running()) {
+ printk(KERN_WARNING DRVNAME
+ ": Watchdog timer still running, stopping it\n");
+ watchdog_stop();
+ }
+ misc_deregister(&watchdog_miscdev);
+ unregister_reboot_notifier(&watchdog_notifier);
+}
+
+MODULE_DESCRIPTION("F71808E Watchdog Driver");
+MODULE_AUTHOR("Giel van Schijndel <[email protected]>");
+MODULE_LICENSE("GPL");
+
+module_init(f71808e_init);
+module_exit(f71808e_exit);
--
1.6.4.4

2010-08-04 11:33:04

by Hans de Goede

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

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<[email protected]>
> ---
> 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

2010-08-04 11:35:05

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH 1/2] hwmon: f71882fg: use a muxed resource lock for the Super I/O port

Ack!

Acked-by: Hans de Goede <[email protected]>

On 08/01/2010 03:30 PM, Giel van Schijndel wrote:
> Sleep while acquiring a resource lock on the Super I/O port. This should
> prevent collisions from causing the hardware probe to fail with -EBUSY.
>
> Signed-off-by: Giel van Schijndel<[email protected]>
> ---
> drivers/hwmon/f71882fg.c | 32 +++++++++++++++++++-------------
> 1 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwmon/f71882fg.c b/drivers/hwmon/f71882fg.c
> index 7857ed3..267cb92 100644
> --- a/drivers/hwmon/f71882fg.c
> +++ b/drivers/hwmon/f71882fg.c
> @@ -113,7 +113,7 @@ static struct platform_device *f71882fg_pdev;
> /* Super-I/O Function prototypes */
> static inline int superio_inb(int base, int reg);
> static inline int superio_inw(int base, int reg);
> -static inline void superio_enter(int base);
> +static inline int superio_enter(int base);
> static inline void superio_select(int base, int ld);
> static inline void superio_exit(int base);
>
> @@ -883,11 +883,20 @@ static int superio_inw(int base, int reg)
> return val;
> }
>
> -static inline void superio_enter(int base)
> +static inline int superio_enter(int base)
> {
> + /* Don't step on other drivers' I/O space by accident */
> + if (!request_muxed_region(base, 2, DRVNAME)) {
> + printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
> + base);
> + return -EBUSY;
> + }
> +
> /* according to the datasheet the key must be send twice! */
> outb(SIO_UNLOCK_KEY, base);
> outb(SIO_UNLOCK_KEY, base);
> +
> + return 0;
> }
>
> static inline void superio_select(int base, int ld)
> @@ -899,6 +908,7 @@ static inline void superio_select(int base, int ld)
> static inline void superio_exit(int base)
> {
> outb(SIO_LOCK_KEY, base);
> + release_region(base, 2);
> }
>
> static inline int fan_from_reg(u16 reg)
> @@ -2239,21 +2249,15 @@ static int f71882fg_remove(struct platform_device *pdev)
> static int __init f71882fg_find(int sioaddr, unsigned short *address,
> struct f71882fg_sio_data *sio_data)
> {
> - int err = -ENODEV;
> u16 devid;
> -
> - /* Don't step on other drivers' I/O space by accident */
> - if (!request_region(sioaddr, 2, DRVNAME)) {
> - printk(KERN_ERR DRVNAME ": I/O address 0x%04x already in use\n",
> - (int)sioaddr);
> - return -EBUSY;
> - }
> -
> - superio_enter(sioaddr);
> + int err = superio_enter(sioaddr);
> + if (err)
> + return err;
>
> devid = superio_inw(sioaddr, SIO_REG_MANID);
> if (devid != SIO_FINTEK_ID) {
> pr_debug(DRVNAME ": Not a Fintek device\n");
> + err = -ENODEV;
> goto exit;
> }
>
> @@ -2280,6 +2284,7 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
> default:
> printk(KERN_INFO DRVNAME ": Unsupported Fintek device: %04x\n",
> (unsigned int)devid);
> + err = -ENODEV;
> goto exit;
> }
>
> @@ -2290,12 +2295,14 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
>
> if (!(superio_inb(sioaddr, SIO_REG_ENABLE)& 0x01)) {
> printk(KERN_WARNING DRVNAME ": Device not activated\n");
> + err = -ENODEV;
> goto exit;
> }
>
> *address = superio_inw(sioaddr, SIO_REG_ADDR);
> if (*address == 0) {
> printk(KERN_WARNING DRVNAME ": Base address not set\n");
> + err = -ENODEV;
> goto exit;
> }
> *address&= ~(REGION_LENGTH - 1); /* Ignore 3 LSB */
> @@ -2306,7 +2313,6 @@ static int __init f71882fg_find(int sioaddr, unsigned short *address,
> (int)superio_inb(sioaddr, SIO_REG_DEVREV));
> exit:
> superio_exit(sioaddr);
> - release_region(sioaddr, 2);
> return err;
> }
>

2010-08-04 15:44:39

by Giel van Schijndel

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

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<[email protected]>
>> ---
>> 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


Attachments:
(No filename) (7.58 kB)
signature.asc (198.00 B)
Digital signature
Download all attachments

2010-08-10 19:11:29

by Giel van Schijndel

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

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


Attachments:
(No filename) (1.33 kB)
signature.asc (198.00 B)
Digital signature
Download all attachments

2010-08-13 09:57:18

by Hans de Goede

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

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

2010-08-13 10:53:25

by Hans de Goede

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

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<[email protected]>
>>> ---
>>> 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

2010-08-18 18:25:58

by Andrew Morton

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

On Fri, 13 Aug 2010 12:01:36 +0200
Hans de Goede <[email protected]> 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?

2010-08-22 18:00:15

by Hans de Goede

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

Hi,

On 08/18/2010 08:24 PM, Andrew Morton wrote:
> On Fri, 13 Aug 2010 12:01:36 +0200
> Hans de Goede<[email protected]> 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

2010-08-22 18:28:17

by Giel van Schijndel

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

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


Attachments:
(No filename) (431.00 B)
signature.asc (198.00 B)
Digital signature
Download all attachments