[PATCH] watchdog: add f71862fg support

[PATCH] watchdog: add f71862fg support

[Lutz Ballaschke]

From: Lutz Ballaschke <vegan.grindcore@gmail.com>

Fintek Super-I/O f71862fg watchdog support added to f71808e_wdt driver.
Furthermore the WDIOC_GETTIMELEFT ioctl is added to the driver.

Signed-off-by: Lutz Ballaschke <vegan.grindcore@gmail.com>
---

diff -Nurp linux-2.6.36-rc1-orig/drivers/watchdog/f71808e_wdt.c linux-2.6.36-rc1/drivers/watchdog/f71808e_wdt.c
--- linux-2.6.36-rc1-orig/drivers/watchdog/f71808e_wdt.c	2010-08-16 02:41:37.000000000 +0200
+++ linux-2.6.36-rc1/drivers/watchdog/f71808e_wdt.c	2010-09-04 18:52:14.000000000 +0200
@@ -42,6 +42,11 @@
 #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_ROM_ADDR_SEL	0x27	/* ROM address select */
+#define SIO_REG_MFUNCT1		0x29	/* Multi function select 1 */
+#define SIO_REG_MFUNCT2		0x2a	/* Multi function select 2 */
+#define SIO_REG_MFUNCT3		0x2b	/* Multi function select 3 */
+#define SIO_REG_MFUNCT4		0x2c	/* Multi function select 4 */
 #define SIO_REG_ENABLE		0x30	/* Logical device enable */
 #define SIO_REG_ADDR		0x60	/* Logical device address (2 bytes) */
 
@@ -52,8 +57,6 @@
 #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
@@ -252,6 +255,24 @@ exit_unlock:
 	return err;
 }
 
+static int watchdog_time_left(void)
+{
+	int ret = 0;
+
+	mutex_lock(&watchdog.lock);
+	ret = superio_enter(watchdog.sioaddr);
+	if (ret)
+		goto exit_unlock;
+
+	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
+	ret = (int) superio_inb(watchdog.sioaddr, F71808FG_REG_WD_TIME);
+	superio_exit(watchdog.sioaddr);
+
+exit_unlock:
+	mutex_unlock(&watchdog.lock);
+	return ret;
+}
+
 static int watchdog_keepalive(void)
 {
 	int err = 0;
@@ -299,13 +320,19 @@ static int watchdog_start(void)
 	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);
+		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT2, 3);
+		superio_clear_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 3);
 		break;
 
 	case f71882fg:
 		/* Set pin 56 to WDTRST# */
-		superio_set_bit(watchdog.sioaddr, 0x29, 1);
+		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
+		break;
+
+	case f71862fg:
+		/* Set pin 63 to WDTRST# (SPI must be disabled first!) */
+		superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
+		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
 		break;
 
 	default:
@@ -549,6 +576,12 @@ static long watchdog_ioctl(struct file *
 	case WDIOC_GETTIMEOUT:
 		return put_user(watchdog.timeout, uarg.i);
 
+	case WDIOC_GETTIMELEFT:
+		status = watchdog_time_left();
+		if (status < 0)
+			return status;
+		return put_user(status, uarg.i);
+
 	default:
 		return -ENOTTY;
 
@@ -709,6 +742,8 @@ static int __init f71808e_find(int sioad
 		watchdog.type = f71882fg;
 		break;
 	case SIO_F71862_ID:
+		watchdog.type = f71862fg;
+		break;
 	case SIO_F71889_ID:
 		/* These have a watchdog, though it isn't implemented (yet). */
 		err = -ENOSYS;

Re: [PATCH] watchdog: add f71862fg support

[Giel van Schijndel]

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

On Mon, Sep 06, 2010 at 03:24:34PM +0200, Lutz Ballaschke wrote:
> Fintek Super-I/O f71862fg watchdog support added to f71808e_wdt driver.
> Furthermore the WDIOC_GETTIMELEFT ioctl is added to the driver.

You're patch makes three changes:
 * Add f71862 support
 * Add the WDIOC_GETTIMELEFT ioctl
 * Change some config-register (magic) numbers into constants

Please split these things into separate patches.

> diff -Nurp linux-2.6.36-rc1-orig/drivers/watchdog/f71808e_wdt.c linux-2.6.36-rc1/drivers/watchdog/f71808e_wdt.c
> --- linux-2.6.36-rc1-orig/drivers/watchdog/f71808e_wdt.c	2010-08-16 02:41:37.000000000 +0200
> +++ linux-2.6.36-rc1/drivers/watchdog/f71808e_wdt.c	2010-09-04 18:52:14.000000000 +0200
> @@ -252,6 +255,24 @@ exit_unlock:
>  	return err;
>  }
>  
> +static int watchdog_time_left(void)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&watchdog.lock);
> +	ret = superio_enter(watchdog.sioaddr);
> +	if (ret)
> +		goto exit_unlock;
> +
> +	superio_select(watchdog.sioaddr, SIO_F71808FG_LD_WDT);
> +	ret = (int) superio_inb(watchdog.sioaddr, F71808FG_REG_WD_TIME);
> +	superio_exit(watchdog.sioaddr);
> +
> +exit_unlock:
> +	mutex_unlock(&watchdog.lock);
> +	return ret;
> +}
> +
>  static int watchdog_keepalive(void)
>  {
>  	int err = 0;

This function doesn't perform any unit-conversion.  The ioctl expects to
receive seconds as the result, while this function can either return
minutes or seconds depending on the value of WD_UNIT (see datasheet
and/or uses of minutes_mode in the driver).

Further, why are you casting superio_inb()'s return value to (int) while
that's already the return type of that function?

> @@ -308,4 +329,10 @@ static int watchdog_start(void)
>  		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT1, 1);
>  		break;
>  
> +	case f71862fg:
> +		/* Set pin 63 to WDTRST# (SPI must be disabled first!) */
> +		superio_clear_bit(watchdog.sioaddr, SIO_REG_ROM_ADDR_SEL, 6);
> +		superio_set_bit(watchdog.sioaddr, SIO_REG_MFUNCT3, 4);
> +		break;
> +
>  	default:

Given that the F71862 has *two* pins assignable to the WDTRST# signal,
please document that fact, and the reason for choosing this one in the
code.

Perhaps it might even be desirable to insert a printk there.  This to
make the user aware of the fact that this driver might not work if the
pin configuration of their hardware doesn't match the assumption you
make about it in this code.

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

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

Re: [PATCH] watchdog: add f71862fg support

[Wim Van Sebroeck]

Hi All,

> Given that the F71862 has *two* pins assignable to the WDTRST# signal,
> please document that fact, and the reason for choosing this one in the
> code.
> 
> Perhaps it might even be desirable to insert a printk there.  This to
> make the user aware of the fact that this driver might not work if the
> pin configuration of their hardware doesn't match the assumption you
> make about it in this code.

I propose to Document it in the code and add a module-parameter so that it is selectable.

Kind regards,
Wim.

Re: [PATCH] watchdog: add f71862fg support

[Lutz Ballaschke]

On Fri, Sep 10, 2010 at 11:03:01AM +0200, Wim Van Sebroeck wrote:
> 
> > Given that the F71862 has *two* pins assignable to the WDTRST# signal,
> > please document that fact, and the reason for choosing this one in the
> > code.
> > 
> > Perhaps it might even be desirable to insert a printk there.  This to
> > make the user aware of the fact that this driver might not work if the
> > pin configuration of their hardware doesn't match the assumption you
> > make about it in this code.
> 
> I propose to Document it in the code and add a module-parameter so that it is selectable.
>
I agree. I will post the patch again (missing parts will follow soon). 
I placed printk in watchdog_start(). Since this means printk output 
everytime /dev/watchdog is opened I'm not sure whether i like it there. 
Maybe it fits better in f71808e_find(). But this would require
additional if / else and stuff. 

Regards,
Lutz Ballaschke

Re: [PATCH] watchdog: add f71862fg support

[Giel van Schijndel]

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

On Sat, Sep 11, 2010 at 01:24:07 +0200, Lutz Ballaschke wrote:
> On Fri, Sep 10, 2010 at 11:03:01 +0200, Wim Van Sebroeck wrote:
>>> Given that the F71862 has *two* pins assignable to the WDTRST#
>>> signal, please document that fact, and the reason for choosing this
>>> one in the code.
>>> 
>>> Perhaps it might even be desirable to insert a printk there.  This
>>> to make the user aware of the fact that this driver might not work
>>> if the pin configuration of their hardware doesn't match the
>>> assumption you make about it in this code.
>> 
>> I propose to Document it in the code and add a module-parameter so
>> that it is selectable.
> 
> I agree. I will post the patch again (missing parts will follow soon). 
> I placed printk in watchdog_start(). Since this means printk output 
> everytime /dev/watchdog is opened I'm not sure whether i like it there. 
> Maybe it fits better in f71808e_find(). But this would require
> additional if / else and stuff. 

Can't you fit that printk in the switch(devid) block?  That wouldn't
require additional logic.  In fact, if you make the selected pin
configurable I wouldn't consider a printk explaining the current
policy/config assumptions necessary because that way there aren't any
policy-related assumptions made in the driver anymore.

-- 
Met vriendelijke groet,
With kind regards,
Giel van Schijndel
--
"It would seem that perfection is attained not when no more can be
 added, but when no more can be removed."
  -- Antoine de Saint Exupéry

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