[PATCH] watchdog: add support for w83697hg chip

[PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

Winbond W83697HG watchdog timer

The Winbond SuperIO W83697HG includes a watchdog that can count from
1 to 255 seconds (or minutes). This drivers allows the seconds mode to
be used. It exposes a standard /dev/watchdog interface.

This chip is currently being used on motherboards designed by
VIA and Dedibox. It should be compatible with W83697HF chips as well
according to the datasheet but is untested on those.

By default, the module looks for a chip at I/O port 0x4e. The chip can
be configured to be at 0x2e on some motherboards, the address can be
chosen using the wdt_io module parameter. Using 0 will try to autodetect
the address.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>

diff -r b1d36669f98d drivers/char/watchdog/Kconfig
--- a/drivers/char/watchdog/Kconfig	Mon Sep 04 03:00:04 2006 +0000
+++ b/drivers/char/watchdog/Kconfig	Wed Sep 06 00:35:22 2006 +0200
@@ -371,6 +371,21 @@ config W83627HF_WDT
 
 	  Most people will say N.
 
+config W83697HG_WDT
+	tristate "W83697HG Watchdog Timer"
+	depends on WATCHDOG && X86
+	---help---
+	  This is the driver for the hardware watchdog on the W83697HG chipset
+	  as used in Dedibox/VIA motherboards (and likely others).
+	  This watchdog simply watches your kernel to make sure it doesn't
+	  freeze, and if it does, it reboots your computer after a certain
+	  amount of time.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called w83697hg_wdt.
+
+	  Most people will say N.
+
 config W83877F_WDT
 	tristate "W83877F (EMACS) Watchdog Timer"
 	depends on WATCHDOG && X86
diff -r b1d36669f98d drivers/char/watchdog/Makefile
--- a/drivers/char/watchdog/Makefile	Mon Sep 04 03:00:04 2006 +0000
+++ b/drivers/char/watchdog/Makefile	Wed Sep 06 00:51:35 2006 +0200
@@ -51,6 +51,7 @@ obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
+obj-$(CONFIG_W83697HG_WDT) += w83697hg_wdt.o
 obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
 obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
 obj-$(CONFIG_MACHZ_WDT) += machzwd.o
diff -r b1d36669f98d drivers/char/watchdog/w83697hg_wdt.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/char/watchdog/w83697hg_wdt.c	Wed Sep 06 12:02:32 2006 +0200
@@ -0,0 +1,423 @@
+/*
+ *	w83697hg WDT driver
+ *
+ *      (c) Copyright 2006 Samuel Tardieu <sam@rfc1149.net>
+ *
+ *	Based on w83627hf_wdt which is based on wadvantechwdt.c
+ *      which is based on wdt.c.
+ *	Original copyright messages:
+ *
+ *	(c) Copyright 2003 Pádraig Brady <P@draigBrady.com>
+ *
+ *	(c) Copyright 2000-2001 Marek Michalkiewicz <marekm@linux.org.pl>
+ *
+ *	(c) Copyright 1996 Alan Cox <alan@redhat.com>, All Rights Reserved.
+ *				http://www.redhat.com
+ *
+ *	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.
+ *
+ *	Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
+ *	warranty for any of this software. This material is provided
+ *	"AS-IS" and at no charge.
+ *
+ *	(c) Copyright 1995    Alan Cox <alan@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/ioport.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/init.h>
+
+#include <asm/io.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+
+#define WATCHDOG_NAME "w83697hg WDT"
+#define PFX WATCHDOG_NAME ": "
+#define WATCHDOG_TIMEOUT 60		/* 60 sec default timeout */
+
+static unsigned long wdt_is_open;
+static char expect_close;
+
+/* You must set this - there is no sane way to probe for this board. */
+static int wdt_io = 0x4e;
+module_param(wdt_io, int, 0);
+MODULE_PARM_DESC(wdt_io, "w83697hg WDT io port (default 0x4e, 0 = autodetect)");
+
+static int timeout = WATCHDOG_TIMEOUT;	/* in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=255, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=CONFIG_WATCHDOG_NOWAYOUT)");
+
+/*
+ *	Kernel methods.
+ */
+
+#define W83697HG_EFER (wdt_io+0)   /* Extended function enable register */
+#define W83697HG_EFIR (wdt_io+0)   /* Extended function index register */
+#define W83697HG_EFDR (wdt_io+1)   /* Extended function data register */
+
+static inline void
+w83697hg_unlock(void)
+{
+	outb_p(0x87, W83697HG_EFER);
+	outb_p(0x87, W83697HG_EFER);
+}
+
+static inline void
+w83697hg_lock(void)
+{
+	outb_p(0xAA, W83697HG_EFER);
+}
+
+/*
+ *	The three functions w83697hg_get_reg(), w83697_set_reg() and
+ *	wdt_ctrl() must be called with the device unlocked.
+ */
+
+static unsigned char
+w83697hg_get_reg(unsigned char reg)
+{
+	outb_p(reg, W83697HG_EFIR);
+	return inb_p(W83697HG_EFDR);
+}
+
+static void
+w83697hg_set_reg(unsigned char reg, unsigned char data)
+{
+	outb_p(reg, W83697HG_EFIR);
+	outb_p(data, W83697HG_EFDR);
+}
+
+static void
+wdt_ctrl(int timeout)
+{
+	w83697hg_set_reg(0xF4, timeout);
+}
+
+static void
+w83697hg_select_wdt(void)
+{
+	w83697hg_unlock();
+	w83697hg_set_reg(0x07, 0x08);   /* Switch to logic device 8 */
+}
+
+static inline void
+w83697hg_deselect_wdt(void)
+{
+	w83697hg_lock();
+}
+
+static void
+wdt_ping(void)
+{
+	w83697hg_select_wdt();
+	wdt_ctrl(timeout);
+	w83697hg_deselect_wdt();
+}
+
+static void
+wdt_enable(void)
+{
+	unsigned char bbuf;
+
+	w83697hg_select_wdt();
+
+	bbuf = w83697hg_get_reg(0x29);
+	bbuf &= ~0x60;
+	bbuf |= 0x20;
+	w83697hg_set_reg(0x29, bbuf);   /* Set pin 119 to WDTO# mode */
+
+	bbuf = w83697hg_get_reg(0xF3);
+	bbuf &= ~0x04;
+	w83697hg_set_reg(0xF3, bbuf);   /* Count mode is seconds */
+
+	wdt_ctrl(timeout);              /* Start timer */
+	w83697hg_set_reg(0x30, 1);      /* Enable timer */
+
+	w83697hg_deselect_wdt();
+}
+
+static void
+wdt_disable(void)
+{
+	w83697hg_select_wdt();
+	w83697hg_set_reg(0x30, 0);       /* Disable timer */
+	wdt_ctrl(0);
+	w83697hg_deselect_wdt();
+}
+
+static int
+wdt_set_heartbeat(int t)
+{
+	if ((t < 1) || (t > 255))
+		return -EINVAL;
+
+	timeout = t;
+	return 0;
+}
+
+static ssize_t
+wdt_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	if (count) {
+		if (!nowayout) {
+			size_t i;
+      
+			expect_close = 0;
+      
+			for (i = 0; i != count; i++) {
+				char c;
+				if (get_user(c, buf+i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_close = 42;
+			}
+		}
+		wdt_ping();
+	}
+	return count;
+}
+
+static int
+wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+	  unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_timeout;
+	static struct watchdog_info ident = {
+		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
+		.firmware_version = 1,
+		.identity = "W83697HG WDT",
+	};
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			return -EFAULT;
+		break;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_KEEPALIVE:
+		wdt_ping();
+		break;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_timeout, p))
+			return -EFAULT;
+		if (wdt_set_heartbeat(new_timeout))
+			return -EINVAL;
+		wdt_ping();
+		/* Fall */
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(timeout, p);
+
+	case WDIOC_SETOPTIONS:
+	{
+		int options, retval = -EINVAL;
+      
+		if (get_user(options, p))
+			return -EFAULT;
+      
+		if (options & WDIOS_DISABLECARD) {
+			wdt_disable();
+			retval = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			wdt_ping();
+			retval = 0;
+		}
+
+		return retval;
+	}
+
+	default:
+		return -ENOIOCTLCMD;
+	}
+	return 0;
+}
+
+static int
+wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &wdt_is_open))
+		return -EBUSY;
+	/*
+	 *	Activate
+	 */
+
+	wdt_enable();
+	wdt_ping();
+	return nonseekable_open(inode, file);
+}
+
+static int
+wdt_close(struct inode *inode, struct file *file)
+{
+	if (expect_close == 42) {
+		wdt_disable();
+	} else {
+		printk(KERN_CRIT PFX "Unexpected close, not stopping watchdog!\n");
+		wdt_ping();
+	}
+	expect_close = 0;
+	clear_bit(0, &wdt_is_open);
+	return 0;
+}
+
+/*
+ *	Notifier for system down
+ */
+
+static int
+wdt_notify_sys(struct notifier_block *this, unsigned long code,
+	       void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		/* Turn the WDT off */
+		wdt_disable();
+	}
+	return NOTIFY_DONE;
+}
+
+/*
+ *	Kernel Interfaces
+ */
+
+static struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= wdt_write,
+	.ioctl		= wdt_ioctl,
+	.open		= wdt_open,
+	.release	= wdt_close,
+};
+
+static struct miscdevice wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &wdt_fops,
+};
+
+/*
+ *	The WDT needs to learn about soft shutdowns in order to
+ *	turn the timebomb registers off.
+ */
+
+static struct notifier_block wdt_notifier = {
+	.notifier_call = wdt_notify_sys,
+};
+
+static int
+w83697hg_init(void)
+{
+	if (!request_region(wdt_io, 2, WATCHDOG_NAME)) {
+		printk(KERN_ERR PFX "I/O address 0x%x already in use\n", wdt_io);
+		return -EIO;
+	}
+  
+	printk(KERN_INFO PFX "Looking for W83697HG at address 0x%x\n", wdt_io);
+	w83697hg_unlock();
+	if (w83697hg_get_reg(0x20) == 0x60) {
+		printk(KERN_INFO PFX "W83697HG found at address 0x%x\n", wdt_io);
+		w83697hg_lock();
+		return 0;
+	}
+	w83697hg_lock();   /* Reprotect in case it was a compatible device */
+  
+	printk(KERN_INFO PFX "W83697HG not found at address 0x%x\n", wdt_io);
+	release_region(wdt_io, 2);
+	return -EIO;
+}
+
+static int __init
+wdt_init(void)
+{
+	int ret, autodetect;
+  
+	printk(KERN_INFO PFX "WDT driver for W83697HG initializing\n");
+  
+	autodetect = wdt_io == 0;
+	if (autodetect)
+		wdt_io = 0x2e;
+  
+	if (!w83697hg_init())
+		goto found;
+  
+	if (autodetect) {
+		wdt_io = 0x4e;
+		if (!w83697hg_init())
+			goto found;
+	}
+  
+	printk(KERN_ERR PFX "No W83697HG could be found\n");
+	ret = -EIO;
+	goto out;
+  
+ found:
+  
+	if (wdt_set_heartbeat(timeout)) {
+		wdt_set_heartbeat(WATCHDOG_TIMEOUT);
+		printk(KERN_INFO PFX "timeout value must be 1<=timeout<=255, using %d\n",
+		       WATCHDOG_TIMEOUT);
+	}
+  
+	ret = register_reboot_notifier(&wdt_notifier);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register reboot notifier (err=%d)\n",
+			ret);
+		goto unreg_regions;
+	}
+  
+	ret = misc_register(&wdt_miscdev);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register miscdev on minor=%d (err=%d)\n",
+			WATCHDOG_MINOR, ret);
+		goto unreg_reboot;
+	}
+  
+	printk (KERN_INFO PFX "initialized. timeout=%d sec (nowayout=%d)\n",
+		timeout, nowayout);
+  
+ out:
+	return ret;
+ unreg_reboot:
+	unregister_reboot_notifier(&wdt_notifier);
+ unreg_regions:
+	release_region(wdt_io, 2);
+	goto out;
+}
+
+static void __exit
+wdt_exit(void)
+{
+	misc_deregister(&wdt_miscdev);
+	unregister_reboot_notifier(&wdt_notifier);
+	release_region(wdt_io, 2);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Samuel Tardieu <sam@rfc1149.net>");
+MODULE_DESCRIPTION("w83697hg WDT driver");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Re: [PATCH] watchdog: add support for w83697hg chip

[Pádraig Brady]

Samuel Tardieu wrote:
> Winbond W83697HG watchdog timer

Looks good, thanks.

I've got a W83697H*F* here on a VIA motherboard,
which is the same from a watchdog point of view.
It is on port 0x2e though.
Is 0x4e a good default?
Is W83697HG a good name?

Note I've CC'd Wim Van Sebroeck who is the watchdog tree maintainer.

I noticed you didn't include the check that's in the
W83627HF driver to reset the timeout if already running
from the BIOS. This was because some BIOS set the timeout
to 4 minutes for example, so when the driver was loaded
and reset the mode to seconds, the machine rebooted
before the init scripts could run and start the userspace
watchdog daemon.

cheers,
Pádraig.

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On  6/09, Pádraig Brady wrote:

| Looks good, thanks.

Thanks a lot Pádraig for your review.

| I've got a W83697H*F* here on a VIA motherboard,
| which is the same from a watchdog point of view.
| It is on port 0x2e though.
| Is 0x4e a good default?

I think we may find half of each (0x2e and 0x4e). I'm reluctant to let
the autodetection routine be the default. What if another peripheral is at
the other address, could bad things happen?

| Is W83697HG a good name?

HF would be equally good. I just named it after the watchdog I had in my
hardware. If there is a preference for HF, I can change it.

| Note I've CC'd Wim Van Sebroeck who is the watchdog tree maintainer.

Thank you.

| I noticed you didn't include the check that's in the
| W83627HF driver to reset the timeout if already running
| from the BIOS. This was because some BIOS set the timeout
| to 4 minutes for example, so when the driver was loaded
| and reset the mode to seconds, the machine rebooted
| before the init scripts could run and start the userspace
| watchdog daemon.

Note that the watchdog is enabled only when the device is open and is
signalled during the wdt_enable() routine just after switching the mode
to seconds. The opportunity for the device to reboot while we are in the
middle of open() will exist anyway and would be a very bad luck.

But I buy your argument in order to reduce the risks: setting the
counter to 0 before switching the mode (in wdt_enable()) would decrease
the possibility of bad luck happening :) But I don't think this is worth
a warning; just set the counter to 0 (not counting) before doing the
configuration is benign enough not to be signalled IMO.

Also, wdt_open() has a wdt_ping() just after the wdt_enable() and this
is superfluous, I remove it.

Here is an updated patch with the changes mentionned above. Don't
hesitate to comment on it or request other changes.



Winbond W83697HG watchdog timer

The Winbond SuperIO W83697HG includes a watchdog that can count from
1 to 255 seconds (or minutes). This drivers allows the seconds mode to
be used. It exposes a standard /dev/watchdog interface.

This chip is currently being used on motherboards designed by
VIA and Dedibox. It should be compatible with W83697HF chips as well
according to the datasheet but is untested on those.

By default, the module looks for a chip at I/O port 0x4e. The chip can
be configured to be at 0x2e on some motherboards, the address can be
chosen using the wdt_io module parameter. Using 0 will try to autodetect
the address.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>

diff -r b1d36669f98d drivers/char/watchdog/Kconfig
--- a/drivers/char/watchdog/Kconfig	Mon Sep 04 03:00:04 2006 +0000
+++ b/drivers/char/watchdog/Kconfig	Wed Sep 06 12:25:06 2006 +0200
@@ -371,6 +371,21 @@ config W83627HF_WDT
 
 	  Most people will say N.
 
+config W83697HG_WDT
+	tristate "W83697HG Watchdog Timer"
+	depends on WATCHDOG && X86
+	---help---
+	  This is the driver for the hardware watchdog on the W83697HG chipset
+	  as used in Dedibox/VIA motherboards (and likely others).
+	  This watchdog simply watches your kernel to make sure it doesn't
+	  freeze, and if it does, it reboots your computer after a certain
+	  amount of time.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called w83697hg_wdt.
+
+	  Most people will say N.
+
 config W83877F_WDT
 	tristate "W83877F (EMACS) Watchdog Timer"
 	depends on WATCHDOG && X86
diff -r b1d36669f98d drivers/char/watchdog/Makefile
--- a/drivers/char/watchdog/Makefile	Mon Sep 04 03:00:04 2006 +0000
+++ b/drivers/char/watchdog/Makefile	Wed Sep 06 12:25:06 2006 +0200
@@ -51,6 +51,7 @@ obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
+obj-$(CONFIG_W83697HG_WDT) += w83697hg_wdt.o
 obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
 obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
 obj-$(CONFIG_MACHZ_WDT) += machzwd.o
diff -r b1d36669f98d drivers/char/watchdog/w83697hg_wdt.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/char/watchdog/w83697hg_wdt.c	Wed Sep 06 13:25:02 2006 +0200
@@ -0,0 +1,424 @@
+/*
+ *	w83697hg WDT driver
+ *
+ *      (c) Copyright 2006 Samuel Tardieu <sam@rfc1149.net>
+ *
+ *	Based on w83627hf_wdt which is based on wadvantechwdt.c
+ *      which is based on wdt.c.
+ *	Original copyright messages:
+ *
+ *	(c) Copyright 2003 Pádraig Brady <P@draigBrady.com>
+ *
+ *	(c) Copyright 2000-2001 Marek Michalkiewicz <marekm@linux.org.pl>
+ *
+ *	(c) Copyright 1996 Alan Cox <alan@redhat.com>, All Rights Reserved.
+ *				http://www.redhat.com
+ *
+ *	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.
+ *
+ *	Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
+ *	warranty for any of this software. This material is provided
+ *	"AS-IS" and at no charge.
+ *
+ *	(c) Copyright 1995    Alan Cox <alan@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/ioport.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/init.h>
+
+#include <asm/io.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+
+#define WATCHDOG_NAME "w83697hg WDT"
+#define PFX WATCHDOG_NAME ": "
+#define WATCHDOG_TIMEOUT 60		/* 60 sec default timeout */
+
+static unsigned long wdt_is_open;
+static char expect_close;
+
+/* You must set this - there is no sane way to probe for this board. */
+static int wdt_io = 0x4e;
+module_param(wdt_io, int, 0);
+MODULE_PARM_DESC(wdt_io, "w83697hg WDT io port (default 0x4e, 0 = autodetect)");
+
+static int timeout = WATCHDOG_TIMEOUT;	/* in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=255, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=CONFIG_WATCHDOG_NOWAYOUT)");
+
+/*
+ *	Kernel methods.
+ */
+
+#define W83697HG_EFER (wdt_io+0)   /* Extended function enable register */
+#define W83697HG_EFIR (wdt_io+0)   /* Extended function index register */
+#define W83697HG_EFDR (wdt_io+1)   /* Extended function data register */
+
+static inline void
+w83697hg_unlock(void)
+{
+	outb_p(0x87, W83697HG_EFER);
+	outb_p(0x87, W83697HG_EFER);
+}
+
+static inline void
+w83697hg_lock(void)
+{
+	outb_p(0xAA, W83697HG_EFER);
+}
+
+/*
+ *	The three functions w83697hg_get_reg(), w83697_set_reg() and
+ *	wdt_ctrl() must be called with the device unlocked.
+ */
+
+static unsigned char
+w83697hg_get_reg(unsigned char reg)
+{
+	outb_p(reg, W83697HG_EFIR);
+	return inb_p(W83697HG_EFDR);
+}
+
+static void
+w83697hg_set_reg(unsigned char reg, unsigned char data)
+{
+	outb_p(reg, W83697HG_EFIR);
+	outb_p(data, W83697HG_EFDR);
+}
+
+static void
+wdt_ctrl(int timeout)
+{
+	w83697hg_set_reg(0xF4, timeout);
+}
+
+static void
+w83697hg_select_wdt(void)
+{
+	w83697hg_unlock();
+	w83697hg_set_reg(0x07, 0x08);   /* Switch to logic device 8 */
+}
+
+static inline void
+w83697hg_deselect_wdt(void)
+{
+	w83697hg_lock();
+}
+
+static void
+wdt_ping(void)
+{
+	w83697hg_select_wdt();
+	wdt_ctrl(timeout);
+	w83697hg_deselect_wdt();
+}
+
+static void
+wdt_enable(void)
+{
+	unsigned char bbuf;
+
+	w83697hg_select_wdt();
+
+	wdt_ctrl(0);                    /* Disable while configuring */
+
+	bbuf = w83697hg_get_reg(0x29);
+	bbuf &= ~0x60;
+	bbuf |= 0x20;
+	w83697hg_set_reg(0x29, bbuf);   /* Set pin 119 to WDTO# mode */
+
+	bbuf = w83697hg_get_reg(0xF3);
+	bbuf &= ~0x04;
+	w83697hg_set_reg(0xF3, bbuf);   /* Count mode is seconds */
+
+	wdt_ctrl(timeout);              /* Start timer */
+	w83697hg_set_reg(0x30, 1);      /* Enable timer */
+
+	w83697hg_deselect_wdt();
+}
+
+static void
+wdt_disable(void)
+{
+	w83697hg_select_wdt();
+	w83697hg_set_reg(0x30, 0);       /* Disable timer */
+	wdt_ctrl(0);
+	w83697hg_deselect_wdt();
+}
+
+static int
+wdt_set_heartbeat(int t)
+{
+	if ((t < 1) || (t > 255))
+		return -EINVAL;
+
+	timeout = t;
+	return 0;
+}
+
+static ssize_t
+wdt_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	if (count) {
+		if (!nowayout) {
+			size_t i;
+      
+			expect_close = 0;
+      
+			for (i = 0; i != count; i++) {
+				char c;
+				if (get_user(c, buf+i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_close = 42;
+			}
+		}
+		wdt_ping();
+	}
+	return count;
+}
+
+static int
+wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+	  unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_timeout;
+	static struct watchdog_info ident = {
+		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
+		.firmware_version = 1,
+		.identity = "W83697HG WDT",
+	};
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			return -EFAULT;
+		break;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_KEEPALIVE:
+		wdt_ping();
+		break;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_timeout, p))
+			return -EFAULT;
+		if (wdt_set_heartbeat(new_timeout))
+			return -EINVAL;
+		wdt_ping();
+		/* Fall */
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(timeout, p);
+
+	case WDIOC_SETOPTIONS:
+	{
+		int options, retval = -EINVAL;
+      
+		if (get_user(options, p))
+			return -EFAULT;
+      
+		if (options & WDIOS_DISABLECARD) {
+			wdt_disable();
+			retval = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			wdt_ping();
+			retval = 0;
+		}
+
+		return retval;
+	}
+
+	default:
+		return -ENOIOCTLCMD;
+	}
+	return 0;
+}
+
+static int
+wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &wdt_is_open))
+		return -EBUSY;
+	/*
+	 *	Activate
+	 */
+
+	wdt_enable();
+	return nonseekable_open(inode, file);
+}
+
+static int
+wdt_close(struct inode *inode, struct file *file)
+{
+	if (expect_close == 42) {
+		wdt_disable();
+	} else {
+		printk(KERN_CRIT PFX "Unexpected close, not stopping watchdog!\n");
+		wdt_ping();
+	}
+	expect_close = 0;
+	clear_bit(0, &wdt_is_open);
+	return 0;
+}
+
+/*
+ *	Notifier for system down
+ */
+
+static int
+wdt_notify_sys(struct notifier_block *this, unsigned long code,
+	       void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		/* Turn the WDT off */
+		wdt_disable();
+	}
+	return NOTIFY_DONE;
+}
+
+/*
+ *	Kernel Interfaces
+ */
+
+static struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= wdt_write,
+	.ioctl		= wdt_ioctl,
+	.open		= wdt_open,
+	.release	= wdt_close,
+};
+
+static struct miscdevice wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &wdt_fops,
+};
+
+/*
+ *	The WDT needs to learn about soft shutdowns in order to
+ *	turn the timebomb registers off.
+ */
+
+static struct notifier_block wdt_notifier = {
+	.notifier_call = wdt_notify_sys,
+};
+
+static int
+w83697hg_init(void)
+{
+	if (!request_region(wdt_io, 2, WATCHDOG_NAME)) {
+		printk(KERN_ERR PFX "I/O address 0x%x already in use\n", wdt_io);
+		return -EIO;
+	}
+  
+	printk(KERN_INFO PFX "Looking for W83697HG at address 0x%x\n", wdt_io);
+	w83697hg_unlock();
+	if (w83697hg_get_reg(0x20) == 0x60) {
+		printk(KERN_INFO PFX "W83697HG found at address 0x%x\n", wdt_io);
+		w83697hg_lock();
+		return 0;
+	}
+	w83697hg_lock();   /* Reprotect in case it was a compatible device */
+  
+	printk(KERN_INFO PFX "W83697HG not found at address 0x%x\n", wdt_io);
+	release_region(wdt_io, 2);
+	return -EIO;
+}
+
+static int __init
+wdt_init(void)
+{
+	int ret, autodetect;
+  
+	printk(KERN_INFO PFX "WDT driver for W83697HG initializing\n");
+  
+	autodetect = wdt_io == 0;
+	if (autodetect)
+		wdt_io = 0x2e;
+  
+	if (!w83697hg_init())
+		goto found;
+  
+	if (autodetect) {
+		wdt_io = 0x4e;
+		if (!w83697hg_init())
+			goto found;
+	}
+  
+	printk(KERN_ERR PFX "No W83697HG could be found\n");
+	ret = -EIO;
+	goto out;
+  
+ found:
+  
+	if (wdt_set_heartbeat(timeout)) {
+		wdt_set_heartbeat(WATCHDOG_TIMEOUT);
+		printk(KERN_INFO PFX "timeout value must be 1<=timeout<=255, using %d\n",
+		       WATCHDOG_TIMEOUT);
+	}
+  
+	ret = register_reboot_notifier(&wdt_notifier);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register reboot notifier (err=%d)\n",
+			ret);
+		goto unreg_regions;
+	}
+  
+	ret = misc_register(&wdt_miscdev);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register miscdev on minor=%d (err=%d)\n",
+			WATCHDOG_MINOR, ret);
+		goto unreg_reboot;
+	}
+  
+	printk (KERN_INFO PFX "initialized. timeout=%d sec (nowayout=%d)\n",
+		timeout, nowayout);
+  
+ out:
+	return ret;
+ unreg_reboot:
+	unregister_reboot_notifier(&wdt_notifier);
+ unreg_regions:
+	release_region(wdt_io, 2);
+	goto out;
+}
+
+static void __exit
+wdt_exit(void)
+{
+	misc_deregister(&wdt_miscdev);
+	unregister_reboot_notifier(&wdt_notifier);
+	release_region(wdt_io, 2);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Samuel Tardieu <sam@rfc1149.net>");
+MODULE_DESCRIPTION("w83697hg WDT driver");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Re: [PATCH] watchdog: add support for w83697hg chip

[Pádraig Brady]

Samuel Tardieu wrote:
> On  6/09, Pádraig Brady wrote:
> | I noticed you didn't include the check that's in the
> | W83627HF driver to reset the timeout if already running
> | from the BIOS. This was because some BIOS set the timeout
> | to 4 minutes for example, so when the driver was loaded
> | and reset the mode to seconds, the machine rebooted
> | before the init scripts could run and start the userspace
> | watchdog daemon.
> 
> Note that the watchdog is enabled only when the device is open and is
> signalled during the wdt_enable() routine just after switching the mode
> to seconds.

Sorry I missed that. That's fine so.

So in the case the BIOS sets the watchdog to 4 mins
for example the 2 drivers are a little different.

W83627HF resets to timeout seconds on module load
W83697HG resets to timeout seconds on /dev/watchdog open

cheers,
Pádraig.

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On  6/09, Pádraig Brady wrote:

| So in the case the BIOS sets the watchdog to 4 mins
| for example the 2 drivers are a little different.
| 
| W83627HF resets to timeout seconds on module load
| W83697HG resets to timeout seconds on /dev/watchdog open

Yes, I'm reluctant at changing anything set by the BIOS before the first
*use* of the module. In particular, if the watchdog was not activated by
default in the BIOS, I'd prefer the box not to reboot just because the
module was loaded (maybe by mistake) if no daemon open /dev/watchdog
at least once.

In particular, some boxes may take a long time to boot, e.g. if fscks are
needed; if the module is loaded by an initrd before filesystems are mounted
and fscks are done, if I'm not mistaken the box could reboot in loop
every time in the middle of fscks.

  Sam

Re: [PATCH] watchdog: add support for w83697hg chip

[Pádraig Brady]

Samuel Tardieu wrote:
> On  6/09, Pádraig Brady wrote:
> 
> | So in the case the BIOS sets the watchdog to 4 mins
> | for example the 2 drivers are a little different.
> | 
> | W83627HF resets to timeout seconds on module load
> | W83697HG resets to timeout seconds on /dev/watchdog open
> 
> Yes, I'm reluctant at changing anything set by the BIOS before the first
> *use* of the module.

Sure.

> In particular, if the watchdog was not activated by
> default in the BIOS, I'd prefer the box not to reboot just because the
> module was loaded (maybe by mistake) if no daemon open /dev/watchdog
> at least once.

Of course. To clarify, the W83627HF watchdog does not enable
the watchdog on load if the BIOS had not enabled it already.

cheers,
Pádraig.

Re: [PATCH] watchdog: add support for w83697hg chip

[Wim Van Sebroeck]

Hi All,

> | So in the case the BIOS sets the watchdog to 4 mins
> | for example the 2 drivers are a little different.
> | 
> | W83627HF resets to timeout seconds on module load
> | W83697HG resets to timeout seconds on /dev/watchdog open
> 
> Yes, I'm reluctant at changing anything set by the BIOS before the first
> *use* of the module. In particular, if the watchdog was not activated by
> default in the BIOS, I'd prefer the box not to reboot just because the
> module was loaded (maybe by mistake) if no daemon open /dev/watchdog
> at least once.

My feedback: it is important that during the initialization of the module,
the watchdog is being disabled. A watchdog should only start working after
it has been started via /dev/watchdog.

> In particular, some boxes may take a long time to boot, e.g. if fscks are
> needed; if the module is loaded by an initrd before filesystems are mounted
> and fscks are done, if I'm not mistaken the box could reboot in loop
> every time in the middle of fscks.

Please note that I added 4 days ago a patch of Marcus Junker <junker@anduras.de>
in my linux-2.6-watchdog-mm tree for the w83697hf chipset.
See: http://www.kernel.org/git/?p=linux/kernel/git/wim/linux-2.6-watchdog-mm.git;a=commitdiff;h=d19ea38e6e99c4924c894cb54440e242179bf27d;hp=19cdb014d58f2c47470d86188a7e556380469008

Greetings,
Wim.

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On  6/09, Wim Van Sebroeck wrote:

| My feedback: it is important that during the initialization of the module,
| the watchdog is being disabled. A watchdog should only start working after
| it has been started via /dev/watchdog.

Agreed. See updated patch at the end.

| Please note that I added 4 days ago a patch of Marcus Junker <junker@anduras.de>
| in my linux-2.6-watchdog-mm tree for the w83697hf chipset.
| See: http://www.kernel.org/git/?p=linux/kernel/git/wim/linux-2.6-watchdog-mm.git;a=commitdiff;h=d19ea38e6e99c4924c894cb54440e242179bf27d;hp=19cdb014d58f2c47470d86188a7e556380469008

Ah, we did duplicate work then, too bad I didn't notice it first :)
However, the patch you refer to:
  - doesn't check whether the device is really present (we *can* probe
    it, my patch does it)
  - limits the watchdog timeout to 1-63 while this device accepts 1-255
  - has several functions returning an int whose result is always 0 and
    is never used
  - the line reading "set second mode & disable keyboard ..." is plain
    wrong, the register being manipulated (CRF4) is the counter itself,
    not the control byte (CRF3) -- looks like it has been copied from
    another driver
  - the note concerning tyan motherboards has been also copied from
    another driver, I'm not sure at all it applies here
  - the comments concerning CRF6 are wrong as CRF3 is manipulated and
    CRF6 is never read nor written
  - I think garbage is being written in CRF3 (the control word) as the
    timeout value is being stored in this register (such as 60 for 60
    seconds)

To make it short, I think the patch you have integrated works by chance.
In particular, if the mode is set to minutes instead of seconds by the
BIOS and you load a timeout value with the bit 2 set (such as 36
seconds), it will be 36 minutes instead. Moreover, the bogus CRF3
manipulations may change the behaviour of the leds as well and bits
marked as "reserved" are erased with random data.

I suggest that you use the following patch instead. It is my patch
renamed as w83697hf_wdt (it covers hf and hg variants) with the
following three modifications: I added Marcus Junker copyright with mine,
as he beats me by a few days, I disable the watchdog until it is
first used, and I set the default address to 0x2e to make it compatible
with Marcus' original patch.

Patch follows this line:


Winbond W83697HF/W83697HGHG watchdog timer

The Winbond SuperIO W83697HF/HG includes a watchdog that can count from
1 to 255 seconds (or minutes). This drivers allows the seconds mode to
be used. It exposes a standard /dev/watchdog interface. This chip is
currently being used on some motherboards designed by VIA.

By default, the module looks for a chip at I/O port 0x4e. The chip can
be configured to be at 0x2e on some motherboards, the address can be
chosen using the wdt_io module parameter. Using 0 will try to autodetect
the address.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>

diff -r b1d36669f98d drivers/char/watchdog/Kconfig
--- a/drivers/char/watchdog/Kconfig	Mon Sep 04 03:00:04 2006 +0000
+++ b/drivers/char/watchdog/Kconfig	Thu Sep 07 11:52:32 2006 +0200
@@ -371,6 +371,21 @@ config W83627HF_WDT
 
 	  Most people will say N.
 
+config W83697HF_WDT
+	tristate "W83697HF/W83697HG Watchdog Timer"
+	depends on WATCHDOG && X86
+	---help---
+	  This is the driver for the hardware watchdog on the W83697HF/HG
+	  chipset as used in Dedibox/VIA motherboards (and likely others).
+	  This watchdog simply watches your kernel to make sure it doesn't
+	  freeze, and if it does, it reboots your computer after a certain
+	  amount of time.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called w83697hf_wdt.
+
+	  Most people will say N.
+
 config W83877F_WDT
 	tristate "W83877F (EMACS) Watchdog Timer"
 	depends on WATCHDOG && X86
diff -r b1d36669f98d drivers/char/watchdog/Makefile
--- a/drivers/char/watchdog/Makefile	Mon Sep 04 03:00:04 2006 +0000
+++ b/drivers/char/watchdog/Makefile	Thu Sep 07 11:52:32 2006 +0200
@@ -51,6 +51,7 @@ obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
+obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
 obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
 obj-$(CONFIG_MACHZ_WDT) += machzwd.o
diff -r b1d36669f98d drivers/char/watchdog/w83697hf_wdt.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/char/watchdog/w83697hf_wdt.c	Thu Sep 07 11:53:00 2006 +0200
@@ -0,0 +1,423 @@
+/*
+ *	w83697hf/hg WDT driver
+ *
+ *	(c) Copyright 2006 Samuel Tardieu <sam@rfc1149.net>
+ *	(c) Copyright 2006 Marcus Junker <junker@anduras.de>
+ *
+ *	Based on w83627hf_wdt which is based on wadvantechwdt.c
+ *	which is based on wdt.c.
+ *	Original copyright messages:
+ *
+ *	(c) Copyright 2003 Pádraig Brady <P@draigBrady.com>
+ *
+ *	(c) Copyright 2000-2001 Marek Michalkiewicz <marekm@linux.org.pl>
+ *
+ *	(c) Copyright 1996 Alan Cox <alan@redhat.com>, All Rights Reserved.
+ *				http://www.redhat.com
+ *
+ *	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.
+ *
+ *	Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
+ *	warranty for any of this software. This material is provided
+ *	"AS-IS" and at no charge.
+ *
+ *	(c) Copyright 1995    Alan Cox <alan@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/ioport.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/init.h>
+
+#include <asm/io.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+
+#define WATCHDOG_NAME "w83697hf/hg WDT"
+#define PFX WATCHDOG_NAME ": "
+#define WATCHDOG_TIMEOUT 60		/* 60 sec default timeout */
+
+static unsigned long wdt_is_open;
+static char expect_close;
+
+/* You must set this - there is no sane way to probe for this board. */
+static int wdt_io = 0x2e;
+module_param(wdt_io, int, 0);
+MODULE_PARM_DESC(wdt_io, "w83697hf/hg WDT io port (default 0x2e, 0 = autodetect)");
+
+static int timeout = WATCHDOG_TIMEOUT;	/* in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=255, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=CONFIG_WATCHDOG_NOWAYOUT)");
+
+/*
+ *	Kernel methods.
+ */
+
+#define W83697HF_EFER (wdt_io+0)   /* Extended function enable register */
+#define W83697HF_EFIR (wdt_io+0)   /* Extended function index register */
+#define W83697HF_EFDR (wdt_io+1)   /* Extended function data register */
+
+static inline void
+w83697hf_unlock(void)
+{
+	outb_p(0x87, W83697HF_EFER);
+	outb_p(0x87, W83697HF_EFER);
+}
+
+static inline void
+w83697hf_lock(void)
+{
+	outb_p(0xAA, W83697HF_EFER);
+}
+
+/*
+ *	The three functions w83697hf_get_reg(), w83697_set_reg() and
+ *	wdt_ctrl() must be called with the device unlocked.
+ */
+
+static unsigned char
+w83697hf_get_reg(unsigned char reg)
+{
+	outb_p(reg, W83697HF_EFIR);
+	return inb_p(W83697HF_EFDR);
+}
+
+static void
+w83697hf_set_reg(unsigned char reg, unsigned char data)
+{
+	outb_p(reg, W83697HF_EFIR);
+	outb_p(data, W83697HF_EFDR);
+}
+
+static void
+wdt_ctrl(int timeout)
+{
+	w83697hf_set_reg(0xF4, timeout);
+}
+
+static void
+w83697hf_select_wdt(void)
+{
+	w83697hf_unlock();
+	w83697hf_set_reg(0x07, 0x08);   /* Switch to logic device 8 */
+}
+
+static inline void
+w83697hf_deselect_wdt(void)
+{
+	w83697hf_lock();
+}
+
+static void
+wdt_ping(void)
+{
+	w83697hf_select_wdt();
+	wdt_ctrl(timeout);
+	w83697hf_deselect_wdt();
+}
+
+static void
+wdt_enable(void)
+{
+	unsigned char bbuf;
+
+	w83697hf_select_wdt();
+
+	wdt_ctrl(0);                    /* Disable watchdog until first use */
+
+	bbuf = w83697hf_get_reg(0x29);
+	bbuf &= ~0x60;
+	bbuf |= 0x20;
+	w83697hf_set_reg(0x29, bbuf);   /* Set pin 119 to WDTO# mode */
+
+	bbuf = w83697hf_get_reg(0xF3);
+	bbuf &= ~0x04;
+	w83697hf_set_reg(0xF3, bbuf);   /* Count mode is seconds */
+	w83697hf_set_reg(0x30, 1);      /* Enable timer */
+
+	w83697hf_deselect_wdt();
+}
+
+static void
+wdt_disable(void)
+{
+	w83697hf_select_wdt();
+	w83697hf_set_reg(0x30, 0);       /* Disable timer */
+	wdt_ctrl(0);
+	w83697hf_deselect_wdt();
+}
+
+static int
+wdt_set_heartbeat(int t)
+{
+	if ((t < 1) || (t > 255))
+		return -EINVAL;
+
+	timeout = t;
+	return 0;
+}
+
+static ssize_t
+wdt_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	if (count) {
+		if (!nowayout) {
+			size_t i;
+      
+			expect_close = 0;
+      
+			for (i = 0; i != count; i++) {
+				char c;
+				if (get_user(c, buf+i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_close = 42;
+			}
+		}
+		wdt_ping();
+	}
+	return count;
+}
+
+static int
+wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+	  unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_timeout;
+	static struct watchdog_info ident = {
+		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
+		.firmware_version = 1,
+		.identity = "W83697HF WDT",
+	};
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			return -EFAULT;
+		break;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_KEEPALIVE:
+		wdt_ping();
+		break;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_timeout, p))
+			return -EFAULT;
+		if (wdt_set_heartbeat(new_timeout))
+			return -EINVAL;
+		wdt_ping();
+		/* Fall */
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(timeout, p);
+
+	case WDIOC_SETOPTIONS:
+	{
+		int options, retval = -EINVAL;
+      
+		if (get_user(options, p))
+			return -EFAULT;
+      
+		if (options & WDIOS_DISABLECARD) {
+			wdt_disable();
+			retval = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			wdt_ping();
+			retval = 0;
+		}
+
+		return retval;
+	}
+
+	default:
+		return -ENOIOCTLCMD;
+	}
+	return 0;
+}
+
+static int
+wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &wdt_is_open))
+		return -EBUSY;
+	/*
+	 *	Activate
+	 */
+
+	wdt_enable();
+	return nonseekable_open(inode, file);
+}
+
+static int
+wdt_close(struct inode *inode, struct file *file)
+{
+	if (expect_close == 42) {
+		wdt_disable();
+	} else {
+		printk(KERN_CRIT PFX "Unexpected close, not stopping watchdog!\n");
+		wdt_ping();
+	}
+	expect_close = 0;
+	clear_bit(0, &wdt_is_open);
+	return 0;
+}
+
+/*
+ *	Notifier for system down
+ */
+
+static int
+wdt_notify_sys(struct notifier_block *this, unsigned long code,
+	       void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		/* Turn the WDT off */
+		wdt_disable();
+	}
+	return NOTIFY_DONE;
+}
+
+/*
+ *	Kernel Interfaces
+ */
+
+static struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= wdt_write,
+	.ioctl		= wdt_ioctl,
+	.open		= wdt_open,
+	.release	= wdt_close,
+};
+
+static struct miscdevice wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &wdt_fops,
+};
+
+/*
+ *	The WDT needs to learn about soft shutdowns in order to
+ *	turn the timebomb registers off.
+ */
+
+static struct notifier_block wdt_notifier = {
+	.notifier_call = wdt_notify_sys,
+};
+
+static int
+w83697hf_init(void)
+{
+	if (!request_region(wdt_io, 2, WATCHDOG_NAME)) {
+		printk(KERN_ERR PFX "I/O address 0x%x already in use\n", wdt_io);
+		return -EIO;
+	}
+  
+	printk(KERN_INFO PFX "Looking for watchdog at address 0x%x\n", wdt_io);
+	w83697hf_unlock();
+	if (w83697hf_get_reg(0x20) == 0x60) {
+		printk(KERN_INFO PFX "watchdog found at address 0x%x\n", wdt_io);
+		w83697hf_lock();
+		return 0;
+	}
+	w83697hf_lock();   /* Reprotect in case it was a compatible device */
+  
+	printk(KERN_INFO PFX "watchdog not found at address 0x%x\n", wdt_io);
+	release_region(wdt_io, 2);
+	return -EIO;
+}
+
+static int __init
+wdt_init(void)
+{
+	int ret, autodetect;
+  
+	printk(KERN_INFO PFX "WDT driver for W83697HF/HG initializing\n");
+  
+	autodetect = wdt_io == 0;
+	if (autodetect)
+		wdt_io = 0x2e;
+  
+	if (!w83697hf_init())
+		goto found;
+  
+	if (autodetect) {
+		wdt_io = 0x4e;
+		if (!w83697hf_init())
+			goto found;
+	}
+  
+	printk(KERN_ERR PFX "No W83697HF/HG could be found\n");
+	ret = -EIO;
+	goto out;
+  
+ found:
+  
+	if (wdt_set_heartbeat(timeout)) {
+		wdt_set_heartbeat(WATCHDOG_TIMEOUT);
+		printk(KERN_INFO PFX "timeout value must be 1<=timeout<=255, using %d\n",
+		       WATCHDOG_TIMEOUT);
+	}
+  
+	ret = register_reboot_notifier(&wdt_notifier);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register reboot notifier (err=%d)\n",
+			ret);
+		goto unreg_regions;
+	}
+  
+	ret = misc_register(&wdt_miscdev);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register miscdev on minor=%d (err=%d)\n",
+			WATCHDOG_MINOR, ret);
+		goto unreg_reboot;
+	}
+  
+	printk (KERN_INFO PFX "initialized. timeout=%d sec (nowayout=%d)\n",
+		timeout, nowayout);
+  
+ out:
+	return ret;
+ unreg_reboot:
+	unregister_reboot_notifier(&wdt_notifier);
+ unreg_regions:
+	release_region(wdt_io, 2);
+	goto out;
+}
+
+static void __exit
+wdt_exit(void)
+{
+	misc_deregister(&wdt_miscdev);
+	unregister_reboot_notifier(&wdt_notifier);
+	release_region(wdt_io, 2);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Samuel Tardieu <sam@rfc1149.net>");
+MODULE_DESCRIPTION("w83697hf WDT driver");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Re: [PATCH] watchdog: add support for w83697hg chip

[Pádraig Brady]

Samuel Tardieu wrote:
> Ah, we did duplicate work then, too bad I didn't notice it first :)

[snip valid points]

> I suggest that you use the following patch instead. It is my patch
> renamed as w83697hf_wdt (it covers hf and hg variants) with the
> following three modifications: I added Marcus Junker copyright with mine,
> as he beats me by a few days, I disable the watchdog until it is
> first used, and I set the default address to 0x2e to make it compatible
> with Marcus' original patch.

I concur.

Pádraig.

p.s. Wim, mail to your address is bouncing

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

>>>>> "Sam" == Samuel Tardieu <sam@rfc1149.net> writes:

Sam> I suggest that you use the following patch instead. [...] I
Sam> disable the watchdog until it is first used

Oops, disabling will work best if done at module initialization time :)
Revised patch attached.


Winbond W83697HF/W83697HGHG watchdog timer

The Winbond SuperIO W83697HF/HG includes a watchdog that can count from
1 to 255 seconds (or minutes). This drivers allows the seconds mode to
be used. It exposes a standard /dev/watchdog interface. This chip is
currently being used on some motherboards designed by VIA.

By default, the module looks for a chip at I/O port 0x4e. The chip can
be configured to be at 0x2e on some motherboards, the address can be
chosen using the wdt_io module parameter. Using 0 will try to autodetect
the address.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>

diff -r 9cfff4a421bd drivers/char/watchdog/Kconfig
--- a/drivers/char/watchdog/Kconfig	Wed Sep 06 19:00:09 2006 +0000
+++ b/drivers/char/watchdog/Kconfig	Thu Sep 07 13:27:52 2006 +0200
@@ -371,6 +371,21 @@ config W83627HF_WDT
 
 	  Most people will say N.
 
+config W83697HF_WDT
+	tristate "W83697HF/W83697HG Watchdog Timer"
+	depends on WATCHDOG && X86
+	---help---
+	  This is the driver for the hardware watchdog on the W83697HF/HG
+	  chipset as used in Dedibox/VIA motherboards (and likely others).
+	  This watchdog simply watches your kernel to make sure it doesn't
+	  freeze, and if it does, it reboots your computer after a certain
+	  amount of time.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called w83697hf_wdt.
+
+	  Most people will say N.
+
 config W83877F_WDT
 	tristate "W83877F (EMACS) Watchdog Timer"
 	depends on WATCHDOG && X86
diff -r 9cfff4a421bd drivers/char/watchdog/Makefile
--- a/drivers/char/watchdog/Makefile	Wed Sep 06 19:00:09 2006 +0000
+++ b/drivers/char/watchdog/Makefile	Thu Sep 07 13:27:52 2006 +0200
@@ -51,6 +51,7 @@ obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
+obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
 obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
 obj-$(CONFIG_MACHZ_WDT) += machzwd.o
diff -r 9cfff4a421bd drivers/char/watchdog/w83697hf_wdt.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/char/watchdog/w83697hf_wdt.c	Thu Sep 07 18:39:07 2006 +0200
@@ -0,0 +1,425 @@
+/*
+ *	w83697hf/hg WDT driver
+ *
+ *	(c) Copyright 2006 Samuel Tardieu <sam@rfc1149.net>
+ *	(c) Copyright 2006 Marcus Junker <junker@anduras.de>
+ *
+ *	Based on w83627hf_wdt which is based on wadvantechwdt.c
+ *	which is based on wdt.c.
+ *	Original copyright messages:
+ *
+ *	(c) Copyright 2003 Pádraig Brady <P@draigBrady.com>
+ *
+ *	(c) Copyright 2000-2001 Marek Michalkiewicz <marekm@linux.org.pl>
+ *
+ *	(c) Copyright 1996 Alan Cox <alan@redhat.com>, All Rights Reserved.
+ *				http://www.redhat.com
+ *
+ *	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.
+ *
+ *	Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
+ *	warranty for any of this software. This material is provided
+ *	"AS-IS" and at no charge.
+ *
+ *	(c) Copyright 1995    Alan Cox <alan@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/ioport.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/init.h>
+
+#include <asm/io.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+
+#define WATCHDOG_NAME "w83697hf/hg WDT"
+#define PFX WATCHDOG_NAME ": "
+#define WATCHDOG_TIMEOUT 60		/* 60 sec default timeout */
+
+static unsigned long wdt_is_open;
+static char expect_close;
+
+/* You must set this - there is no sane way to probe for this board. */
+static int wdt_io = 0x2e;
+module_param(wdt_io, int, 0);
+MODULE_PARM_DESC(wdt_io, "w83697hf/hg WDT io port (default 0x2e, 0 = autodetect)");
+
+static int timeout = WATCHDOG_TIMEOUT;	/* in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=255, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=CONFIG_WATCHDOG_NOWAYOUT)");
+
+/*
+ *	Kernel methods.
+ */
+
+#define W83697HF_EFER (wdt_io+0)   /* Extended function enable register */
+#define W83697HF_EFIR (wdt_io+0)   /* Extended function index register */
+#define W83697HF_EFDR (wdt_io+1)   /* Extended function data register */
+
+static inline void
+w83697hf_unlock(void)
+{
+	outb_p(0x87, W83697HF_EFER);
+	outb_p(0x87, W83697HF_EFER);
+}
+
+static inline void
+w83697hf_lock(void)
+{
+	outb_p(0xAA, W83697HF_EFER);
+}
+
+/*
+ *	The three functions w83697hf_get_reg(), w83697_set_reg() and
+ *	wdt_ctrl() must be called with the device unlocked.
+ */
+
+static unsigned char
+w83697hf_get_reg(unsigned char reg)
+{
+	outb_p(reg, W83697HF_EFIR);
+	return inb_p(W83697HF_EFDR);
+}
+
+static void
+w83697hf_set_reg(unsigned char reg, unsigned char data)
+{
+	outb_p(reg, W83697HF_EFIR);
+	outb_p(data, W83697HF_EFDR);
+}
+
+static void
+wdt_ctrl(int timeout)
+{
+	w83697hf_set_reg(0xF4, timeout);
+}
+
+static void
+w83697hf_select_wdt(void)
+{
+	w83697hf_unlock();
+	w83697hf_set_reg(0x07, 0x08);   /* Switch to logic device 8 */
+}
+
+static inline void
+w83697hf_deselect_wdt(void)
+{
+	w83697hf_lock();
+}
+
+static void
+wdt_ping(void)
+{
+	w83697hf_select_wdt();
+	wdt_ctrl(timeout);
+	w83697hf_deselect_wdt();
+}
+
+static void
+wdt_enable(void)
+{
+	unsigned char bbuf;
+
+	w83697hf_select_wdt();
+
+	bbuf = w83697hf_get_reg(0x29);
+	bbuf &= ~0x60;
+	bbuf |= 0x20;
+	w83697hf_set_reg(0x29, bbuf);   /* Set pin 119 to WDTO# mode */
+
+	bbuf = w83697hf_get_reg(0xF3);
+	bbuf &= ~0x04;
+	w83697hf_set_reg(0xF3, bbuf);   /* Count mode is seconds */
+	w83697hf_set_reg(0x30, 1);      /* Enable timer */
+
+	w83697hf_deselect_wdt();
+}
+
+static void
+wdt_disable(void)
+{
+	w83697hf_select_wdt();
+	w83697hf_set_reg(0x30, 0);       /* Disable timer */
+	wdt_ctrl(0);
+	w83697hf_deselect_wdt();
+}
+
+static int
+wdt_set_heartbeat(int t)
+{
+	if ((t < 1) || (t > 255))
+		return -EINVAL;
+
+	timeout = t;
+	return 0;
+}
+
+static ssize_t
+wdt_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	if (count) {
+		if (!nowayout) {
+			size_t i;
+      
+			expect_close = 0;
+      
+			for (i = 0; i != count; i++) {
+				char c;
+				if (get_user(c, buf+i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_close = 42;
+			}
+		}
+		wdt_ping();
+	}
+	return count;
+}
+
+static int
+wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+	  unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_timeout;
+	static struct watchdog_info ident = {
+		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
+		.firmware_version = 1,
+		.identity = "W83697HF WDT",
+	};
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			return -EFAULT;
+		break;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_KEEPALIVE:
+		wdt_ping();
+		break;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_timeout, p))
+			return -EFAULT;
+		if (wdt_set_heartbeat(new_timeout))
+			return -EINVAL;
+		wdt_ping();
+		/* Fall */
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(timeout, p);
+
+	case WDIOC_SETOPTIONS:
+	{
+		int options, retval = -EINVAL;
+      
+		if (get_user(options, p))
+			return -EFAULT;
+      
+		if (options & WDIOS_DISABLECARD) {
+			wdt_disable();
+			retval = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			wdt_ping();
+			retval = 0;
+		}
+
+		return retval;
+	}
+
+	default:
+		return -ENOIOCTLCMD;
+	}
+	return 0;
+}
+
+static int
+wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &wdt_is_open))
+		return -EBUSY;
+	/*
+	 *	Activate
+	 */
+
+	wdt_enable();
+	return nonseekable_open(inode, file);
+}
+
+static int
+wdt_close(struct inode *inode, struct file *file)
+{
+	if (expect_close == 42) {
+		wdt_disable();
+	} else {
+		printk(KERN_CRIT PFX "Unexpected close, not stopping watchdog!\n");
+		wdt_ping();
+	}
+	expect_close = 0;
+	clear_bit(0, &wdt_is_open);
+	return 0;
+}
+
+/*
+ *	Notifier for system down
+ */
+
+static int
+wdt_notify_sys(struct notifier_block *this, unsigned long code,
+	       void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		/* Turn the WDT off */
+		wdt_disable();
+	}
+	return NOTIFY_DONE;
+}
+
+/*
+ *	Kernel Interfaces
+ */
+
+static struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= wdt_write,
+	.ioctl		= wdt_ioctl,
+	.open		= wdt_open,
+	.release	= wdt_close,
+};
+
+static struct miscdevice wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &wdt_fops,
+};
+
+/*
+ *	The WDT needs to learn about soft shutdowns in order to
+ *	turn the timebomb registers off.
+ */
+
+static struct notifier_block wdt_notifier = {
+	.notifier_call = wdt_notify_sys,
+};
+
+static int
+w83697hf_init(void)
+{
+	if (!request_region(wdt_io, 2, WATCHDOG_NAME)) {
+		printk(KERN_ERR PFX "I/O address 0x%x already in use\n", wdt_io);
+		return -EIO;
+	}
+  
+	printk(KERN_INFO PFX "Looking for watchdog at address 0x%x\n", wdt_io);
+	w83697hf_unlock();
+	if (w83697hf_get_reg(0x20) == 0x60) {
+		printk(KERN_INFO PFX "watchdog found at address 0x%x\n", wdt_io);
+		w83697hf_lock();
+		return 0;
+	}
+	w83697hf_lock();   /* Reprotect in case it was a compatible device */
+  
+	printk(KERN_INFO PFX "watchdog not found at address 0x%x\n", wdt_io);
+	release_region(wdt_io, 2);
+	return -EIO;
+}
+
+static int __init
+wdt_init(void)
+{
+	int ret, autodetect;
+  
+	printk(KERN_INFO PFX "WDT driver for W83697HF/HG initializing\n");
+  
+	autodetect = wdt_io == 0;
+	if (autodetect)
+		wdt_io = 0x2e;
+  
+	if (!w83697hf_init())
+		goto found;
+  
+	if (autodetect) {
+		wdt_io = 0x4e;
+		if (!w83697hf_init())
+			goto found;
+	}
+  
+	printk(KERN_ERR PFX "No W83697HF/HG could be found\n");
+	ret = -EIO;
+	goto out;
+  
+ found:
+
+	w83697hf_select_wdt();
+	wdt_ctrl(0);                    /* Disable watchdog until first use */
+	w83697hf_deselect_wdt();
+
+	if (wdt_set_heartbeat(timeout)) {
+		wdt_set_heartbeat(WATCHDOG_TIMEOUT);
+		printk(KERN_INFO PFX "timeout value must be 1<=timeout<=255, using %d\n",
+		       WATCHDOG_TIMEOUT);
+	}
+  
+	ret = register_reboot_notifier(&wdt_notifier);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register reboot notifier (err=%d)\n",
+			ret);
+		goto unreg_regions;
+	}
+  
+	ret = misc_register(&wdt_miscdev);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register miscdev on minor=%d (err=%d)\n",
+			WATCHDOG_MINOR, ret);
+		goto unreg_reboot;
+	}
+  
+	printk (KERN_INFO PFX "initialized. timeout=%d sec (nowayout=%d)\n",
+		timeout, nowayout);
+  
+ out:
+	return ret;
+ unreg_reboot:
+	unregister_reboot_notifier(&wdt_notifier);
+ unreg_regions:
+	release_region(wdt_io, 2);
+	goto out;
+}
+
+static void __exit
+wdt_exit(void)
+{
+	misc_deregister(&wdt_miscdev);
+	unregister_reboot_notifier(&wdt_notifier);
+	release_region(wdt_io, 2);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Samuel Tardieu <sam@rfc1149.net>");
+MODULE_DESCRIPTION("w83697hf WDT driver");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Re: [PATCH] watchdog: add support for w83697hg chip

[Jim Cromie]

Pádraig Brady wrote:
> Is W83697HG a good name?
>   


could you add a suffix, say _watchdog ?

the name youve got is confusingly close to the convention used in 
drivers/hwmon

 ls hwmon/w*.c
hwmon/w83627ehf.c  hwmon/w83627hf.c  hwmon/w83781d.c  hwmon/w83791d.c  
hwmon/w83792d.c  hwmon/w83l785ts.c

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

>>>>> "Jim" == Jim Cromie <jim.cromie@gmail.com> writes:

Jim> Pádraig Brady wrote:
>> Is W83697HG a good name?
>> 


Jim> could you add a suffix, say _watchdog ?

Jim> the name youve got is confusingly close to the convention used in
Jim> drivers/hwmon

The module is named as others in drivers/char/watchdog: w83697hf_wtd.c

  Sam
-- 
Samuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/

Re: [PATCH] watchdog: add support for w83697hg chip

[Pádraig Brady]

Samuel Tardieu wrote:
>>>>>>"Sam" == Samuel Tardieu <sam@rfc1149.net> writes:
> 
> 
> Sam> I suggest that you use the following patch instead. [...] I
> Sam> disable the watchdog until it is first used
> 
> Oops, disabling will work best if done at module initialization time :)
> Revised patch attached.

Personally I don't agree with disabling the watchdog on load.
If it is already started from the BIOS or GRUB for example,
there will be a large window of time/logic that is not protected.

Pádraig.

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On  8/09, Pádraig Brady wrote:

| Personally I don't agree with disabling the watchdog on load.
| If it is already started from the BIOS or GRUB for example,
| there will be a large window of time/logic that is not protected.

Not necessarily: you are free to load the module only when you are ready
to run the controlling program.

Re: [PATCH] watchdog: add support for w83697hg chip

[Jean Delvare]

> Pádraig Brady wrote:
> > Is W83697HG a good name?
> >   
> 
> 
> could you add a suffix, say _watchdog ?
> 
> the name youve got is confusingly close to the convention used in 
> drivers/hwmon
> 
>  ls hwmon/w*.c
> hwmon/w83627ehf.c  hwmon/w83627hf.c  hwmon/w83781d.c  hwmon/w83791d.c  
> hwmon/w83792d.c  hwmon/w83l785ts.c

I second Jim's suggestion. Given the name of the other Winbond watchdog
drivers:

char/watchdog/w83627hf_wdt.c 
char/watchdog/w83977f_wdt.c
char/watchdog/w83877f_wdt.c

I'd suggest w83697hf_wdt.c. The W83697HG if the lead-free version of
the older W83697HF and they are otherwise the same chip. BTW I see that
there's already a driver for the W83627HF watchdog. Given the
similarities between the W83627HF and the W83697HF, it might make sense
to add support for the latter directly in the w83627hf_wdt driver,
rather than writing a brand new one.

-- 
Jean Delvare

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On  9/09, Alan Cox wrote:

| No kernel level locking anywhere in the driver. Yet you could have two
| people accessing it at once.

The device can be open only by one client at a time, this is checked in
open(), as was done in most other watchdog drivers.

| > +wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
| > +	  unsigned long arg)
| > +{
| > +	default:
| > +		return -ENOIOCTLCMD;
| 
| Should be -ENOTTY

We have 44 instances of ENOIOCTLCMD in other watchdog drivers
and zero instances of ENOTTY. Should we change all the instances, adopt
what has been done or just change the new ones?

| > +	printk(KERN_INFO PFX "Looking for W83697HG at address 0x%x\n", wdt_io);
| 
| KERN_DEBUG

Fixed in my copy.

  Sam

Re: [PATCH] watchdog: add support for w83697hg chip

[Alan Cox]

Ar Mer, 2006-09-06 am 12:29 +0200, ysgrifennodd Samuel Tardieu:
> +static unsigned char
> +w83697hg_get_reg(unsigned char reg)
> +{
> +	outb_p(reg, W83697HG_EFIR);
> +	return inb_p(W83697HG_EFDR);
> +}

No kernel level locking anywhere in the driver. Yet you could have two
people accessing it at once.


> +wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> +	  unsigned long arg)
> +{
> +	default:
> +		return -ENOIOCTLCMD;

Should be -ENOTTY

> +	printk(KERN_INFO PFX "Looking for W83697HG at address 0x%x\n", wdt_io);

KERN_DEBUG

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On  9/09, Samuel Tardieu wrote:

| We have 44 instances of ENOIOCTLCMD in other watchdog drivers
| and zero instances of ENOTTY. Should we change all the instances, adopt
| what has been done or just change the new ones?

Ok, I've found http://tinyurl.com/pmsqm where you explain that
ENOIOCTLCMD should never be seen by the user. Patch follows to change
the other watchdogs.

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On  9/09, Alan Cox wrote:

| This is insufficient. Many watchdog drivers are broken here but that's
| no excuse to continue the problem because people will copy the errror
| (as I suspect you did)
| 
| 	fd = open("/dev/watchdog", O_RDWR);
| 	switch(fork())
| 	{
| 
| .. one open, two users, two processes, two CPUs

Right. Thanks for the review, will fix.

Re: [PATCH] watchdog: add support for w83697hg chip

[Alan Cox]

Ar Sad, 2006-09-09 am 17:18 +0200, ysgrifennodd Samuel Tardieu:
> On  9/09, Alan Cox wrote:
> 
> | No kernel level locking anywhere in the driver. Yet you could have two
> | people accessing it at once.
> 
> The device can be open only by one client at a time, this is checked in
> open(), as was done in most other watchdog drivers.

This is insufficient. Many watchdog drivers are broken here but that's
no excuse to continue the problem because people will copy the errror
(as I suspect you did)

	fd = open("/dev/watchdog", O_RDWR);
	switch(fork())
	{

.. one open, two users, two processes, two CPUs


> | > +	default:
> | > +		return -ENOIOCTLCMD;
> | 
> | Should be -ENOTTY
> 
> We have 44 instances of ENOIOCTLCMD in other watchdog drivers
> and zero instances of ENOTTY. Should we change all the instances, adopt
> what has been done or just change the new ones?

-ENOIOCTLCMD should never be returned to userspace. An unknown ioctl
returns -ENOTTY. -ENOIOCTLCMD is an internal magic value used with
helper layers to tell the helper layer "I don't handle this, use your
own handler"

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On  9/09, Samuel Tardieu wrote:

| | 	fd = open("/dev/watchdog", O_RDWR);
| | 	switch(fork())
| | 	{
| | 
| | .. one open, two users, two processes, two CPUs
| 
| Right. Thanks for the review, will fix.

Updated patch follows.

  Sam



Winbond W83697HF/W83697HGHG watchdog timer

The Winbond SuperIO W83697HF/HG includes a watchdog that can count from
1 to 255 seconds (or minutes). This drivers allows the seconds mode to
be used. It exposes a standard /dev/watchdog interface. This chip is
currently being used on some motherboards designed by VIA.

By default, the module looks for a chip at I/O port 0x4e. The chip can
be configured to be at 0x2e on some motherboards, the address can be
chosen using the wdt_io module parameter. Using 0 will try to autodetect
the address.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>

diff -r 92038c30d67b drivers/char/watchdog/Kconfig
--- a/drivers/char/watchdog/Kconfig	Sat Sep 09 17:30:15 2006 +0200
+++ b/drivers/char/watchdog/Kconfig	Sat Sep 09 17:31:47 2006 +0200
@@ -371,6 +371,21 @@ config W83627HF_WDT
 
 	  Most people will say N.
 
+config W83697HF_WDT
+	tristate "W83697HF/W83697HG Watchdog Timer"
+	depends on WATCHDOG && X86
+	---help---
+	  This is the driver for the hardware watchdog on the W83697HF/HG
+	  chipset as used in Dedibox/VIA motherboards (and likely others).
+	  This watchdog simply watches your kernel to make sure it doesn't
+	  freeze, and if it does, it reboots your computer after a certain
+	  amount of time.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called w83697hf_wdt.
+
+	  Most people will say N.
+
 config W83877F_WDT
 	tristate "W83877F (EMACS) Watchdog Timer"
 	depends on WATCHDOG && X86
diff -r 92038c30d67b drivers/char/watchdog/Makefile
--- a/drivers/char/watchdog/Makefile	Sat Sep 09 17:30:15 2006 +0200
+++ b/drivers/char/watchdog/Makefile	Sat Sep 09 17:31:47 2006 +0200
@@ -51,6 +51,7 @@ obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
+obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
 obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
 obj-$(CONFIG_MACHZ_WDT) += machzwd.o
diff -r 92038c30d67b drivers/char/watchdog/w83697hf_wdt.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/char/watchdog/w83697hf_wdt.c	Sat Sep 09 18:23:56 2006 +0200
@@ -0,0 +1,429 @@
+/*
+ *	w83697hf/hg WDT driver
+ *
+ *	(c) Copyright 2006 Samuel Tardieu <sam@rfc1149.net>
+ *	(c) Copyright 2006 Marcus Junker <junker@anduras.de>
+ *
+ *	Based on w83627hf_wdt which is based on wadvantechwdt.c
+ *	which is based on wdt.c.
+ *	Original copyright messages:
+ *
+ *	(c) Copyright 2003 Pádraig Brady <P@draigBrady.com>
+ *
+ *	(c) Copyright 2000-2001 Marek Michalkiewicz <marekm@linux.org.pl>
+ *
+ *	(c) Copyright 1996 Alan Cox <alan@redhat.com>, All Rights Reserved.
+ *				http://www.redhat.com
+ *
+ *	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.
+ *
+ *	Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
+ *	warranty for any of this software. This material is provided
+ *	"AS-IS" and at no charge.
+ *
+ *	(c) Copyright 1995    Alan Cox <alan@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/ioport.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/init.h>
+
+#include <asm/io.h>
+#include <asm/semaphore.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+
+#define WATCHDOG_NAME "w83697hf/hg WDT"
+#define PFX WATCHDOG_NAME ": "
+#define WATCHDOG_TIMEOUT 60		/* 60 sec default timeout */
+
+static unsigned long wdt_is_open;
+static char expect_close;
+static DECLARE_MUTEX(dev_lock);         /* Protect sequential operations */
+
+/* You must set this - there is no sane way to probe for this board. */
+static int wdt_io = 0x2e;
+module_param(wdt_io, int, 0);
+MODULE_PARM_DESC(wdt_io, "w83697hf/hg WDT io port (default 0x2e, 0 = autodetect)");
+
+static int timeout = WATCHDOG_TIMEOUT;	/* in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=255, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=CONFIG_WATCHDOG_NOWAYOUT)");
+
+/*
+ *	Kernel methods.
+ */
+
+#define W83697HF_EFER (wdt_io+0)   /* Extended function enable register */
+#define W83697HF_EFIR (wdt_io+0)   /* Extended function index register */
+#define W83697HF_EFDR (wdt_io+1)   /* Extended function data register */
+
+static inline void
+w83697hf_unlock(void)
+{
+	outb_p(0x87, W83697HF_EFER);
+	outb_p(0x87, W83697HF_EFER);
+}
+
+static inline void
+w83697hf_lock(void)
+{
+	outb_p(0xAA, W83697HF_EFER);
+}
+
+/*
+ *	The three functions w83697hf_get_reg(), w83697_set_reg() and
+ *	wdt_ctrl() must be called with the device unlocked.
+ */
+
+static unsigned char
+w83697hf_get_reg(unsigned char reg)
+{
+	outb_p(reg, W83697HF_EFIR);
+	return inb_p(W83697HF_EFDR);
+}
+
+static void
+w83697hf_set_reg(unsigned char reg, unsigned char data)
+{
+	outb_p(reg, W83697HF_EFIR);
+	outb_p(data, W83697HF_EFDR);
+}
+
+static void
+wdt_ctrl(int timeout)
+{
+	w83697hf_set_reg(0xF4, timeout);
+}
+
+static void
+w83697hf_select_wdt(void)
+{
+	down(&dev_lock);
+	w83697hf_unlock();
+	w83697hf_set_reg(0x07, 0x08);   /* Switch to logic device 8 */
+}
+
+static inline void
+w83697hf_deselect_wdt(void)
+{
+	w83697hf_lock();
+	up(&dev_lock);
+}
+
+static void
+wdt_ping(void)
+{
+	w83697hf_select_wdt();
+	wdt_ctrl(timeout);
+	w83697hf_deselect_wdt();
+}
+
+static void
+wdt_enable(void)
+{
+	unsigned char bbuf;
+
+	w83697hf_select_wdt();
+
+	bbuf = w83697hf_get_reg(0x29);
+	bbuf &= ~0x60;
+	bbuf |= 0x20;
+	w83697hf_set_reg(0x29, bbuf);   /* Set pin 119 to WDTO# mode */
+
+	bbuf = w83697hf_get_reg(0xF3);
+	bbuf &= ~0x04;
+	w83697hf_set_reg(0xF3, bbuf);   /* Count mode is seconds */
+	w83697hf_set_reg(0x30, 1);      /* Enable timer */
+
+	w83697hf_deselect_wdt();
+}
+
+static void
+wdt_disable(void)
+{
+	w83697hf_select_wdt();
+	w83697hf_set_reg(0x30, 0);       /* Disable timer */
+	wdt_ctrl(0);
+	w83697hf_deselect_wdt();
+}
+
+static int
+wdt_set_heartbeat(int t)
+{
+	if ((t < 1) || (t > 255))
+		return -EINVAL;
+
+	timeout = t;
+	return 0;
+}
+
+static ssize_t
+wdt_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	if (count) {
+		if (!nowayout) {
+			size_t i;
+      
+			expect_close = 0;
+      
+			for (i = 0; i != count; i++) {
+				char c;
+				if (get_user(c, buf+i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_close = 42;
+			}
+		}
+		wdt_ping();
+	}
+	return count;
+}
+
+static int
+wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+	  unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_timeout;
+	static struct watchdog_info ident = {
+		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
+		.firmware_version = 1,
+		.identity = "W83697HF WDT",
+	};
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			return -EFAULT;
+		break;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_KEEPALIVE:
+		wdt_ping();
+		break;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_timeout, p))
+			return -EFAULT;
+		if (wdt_set_heartbeat(new_timeout))
+			return -EINVAL;
+		wdt_ping();
+		/* Fall */
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(timeout, p);
+
+	case WDIOC_SETOPTIONS:
+	{
+		int options, retval = -EINVAL;
+      
+		if (get_user(options, p))
+			return -EFAULT;
+      
+		if (options & WDIOS_DISABLECARD) {
+			wdt_disable();
+			retval = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			wdt_ping();
+			retval = 0;
+		}
+
+		return retval;
+	}
+
+	default:
+		return -ENOTTY;
+	}
+	return 0;
+}
+
+static int
+wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &wdt_is_open))
+		return -EBUSY;
+	/*
+	 *	Activate
+	 */
+
+	wdt_enable();
+	return nonseekable_open(inode, file);
+}
+
+static int
+wdt_close(struct inode *inode, struct file *file)
+{
+	if (expect_close == 42) {
+		wdt_disable();
+	} else {
+		printk(KERN_CRIT PFX "Unexpected close, not stopping watchdog!\n");
+		wdt_ping();
+	}
+	expect_close = 0;
+	clear_bit(0, &wdt_is_open);
+	return 0;
+}
+
+/*
+ *	Notifier for system down
+ */
+
+static int
+wdt_notify_sys(struct notifier_block *this, unsigned long code,
+	       void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		/* Turn the WDT off */
+		wdt_disable();
+	}
+	return NOTIFY_DONE;
+}
+
+/*
+ *	Kernel Interfaces
+ */
+
+static struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= wdt_write,
+	.ioctl		= wdt_ioctl,
+	.open		= wdt_open,
+	.release	= wdt_close,
+};
+
+static struct miscdevice wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &wdt_fops,
+};
+
+/*
+ *	The WDT needs to learn about soft shutdowns in order to
+ *	turn the timebomb registers off.
+ */
+
+static struct notifier_block wdt_notifier = {
+	.notifier_call = wdt_notify_sys,
+};
+
+static int
+w83697hf_init(void)
+{
+	if (!request_region(wdt_io, 2, WATCHDOG_NAME)) {
+		printk(KERN_ERR PFX "I/O address 0x%x already in use\n", wdt_io);
+		return -EIO;
+	}
+  
+	printk(KERN_DEBUG PFX "Looking for watchdog at address 0x%x\n", wdt_io);
+	w83697hf_unlock();
+	if (w83697hf_get_reg(0x20) == 0x60) {
+		printk(KERN_INFO PFX "watchdog found at address 0x%x\n", wdt_io);
+		w83697hf_lock();
+		return 0;
+	}
+	w83697hf_lock();   /* Reprotect in case it was a compatible device */
+  
+	printk(KERN_INFO PFX "watchdog not found at address 0x%x\n", wdt_io);
+	release_region(wdt_io, 2);
+	return -EIO;
+}
+
+static int __init
+wdt_init(void)
+{
+	int ret, autodetect;
+  
+	printk(KERN_INFO PFX "WDT driver for W83697HF/HG initializing\n");
+  
+	autodetect = wdt_io == 0;
+	if (autodetect)
+		wdt_io = 0x2e;
+  
+	if (!w83697hf_init())
+		goto found;
+  
+	if (autodetect) {
+		wdt_io = 0x4e;
+		if (!w83697hf_init())
+			goto found;
+	}
+  
+	printk(KERN_ERR PFX "No W83697HF/HG could be found\n");
+	ret = -EIO;
+	goto out;
+  
+ found:
+
+	w83697hf_select_wdt();
+	wdt_ctrl(0);                    /* Disable watchdog until first use */
+	w83697hf_deselect_wdt();
+
+	if (wdt_set_heartbeat(timeout)) {
+		wdt_set_heartbeat(WATCHDOG_TIMEOUT);
+		printk(KERN_INFO PFX "timeout value must be 1<=timeout<=255, using %d\n",
+		       WATCHDOG_TIMEOUT);
+	}
+  
+	ret = register_reboot_notifier(&wdt_notifier);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register reboot notifier (err=%d)\n",
+			ret);
+		goto unreg_regions;
+	}
+  
+	ret = misc_register(&wdt_miscdev);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register miscdev on minor=%d (err=%d)\n",
+			WATCHDOG_MINOR, ret);
+		goto unreg_reboot;
+	}
+  
+	printk (KERN_INFO PFX "initialized. timeout=%d sec (nowayout=%d)\n",
+		timeout, nowayout);
+  
+ out:
+	return ret;
+ unreg_reboot:
+	unregister_reboot_notifier(&wdt_notifier);
+ unreg_regions:
+	release_region(wdt_io, 2);
+	goto out;
+}
+
+static void __exit
+wdt_exit(void)
+{
+	misc_deregister(&wdt_miscdev);
+	unregister_reboot_notifier(&wdt_notifier);
+	release_region(wdt_io, 2);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Samuel Tardieu <sam@rfc1149.net>");
+MODULE_DESCRIPTION("w83697hf WDT driver");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Re: [PATCH] watchdog: add support for w83697hg chip

[Sergey Vlasov]

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

On Sat, 09 Sep 2006 16:25:25 +0100 Alan Cox wrote:

> Ar Mer, 2006-09-06 am 12:29 +0200, ysgrifennodd Samuel Tardieu:
> > +static unsigned char
> > +w83697hg_get_reg(unsigned char reg)
> > +{
> > +	outb_p(reg, W83697HG_EFIR);
> > +	return inb_p(W83697HG_EFDR);
> > +}
> 
> No kernel level locking anywhere in the driver. Yet you could have two
> people accessing it at once.

Actually the situation is worse.  This driver pokes at SuperIO
configuration registers, which are shared by all logical devices of the
SuperIO chip.  There are other drivers which touch these registers -
e.g., drivers/hwmon/w83627hf.c handles the hardware monitor part of this
chip; many other hwmon drivers handle other SuperIO devices.  Hardware
monitor drivers access the SuperIO config during initialization and do
not request that port region, therefore loading hwmon drivers when
w83697hf_wdt is loaded can lead to conflicts.

More places which seem to touch SuperIO config:

  - drivers/parport/parport_pc.c
  - drivers/net/irda/nsc-ircc.c
  - drivers/net/irda/smsc-ircc2.c
  - drivers/net/irda/via-ircc.h

I can find at least two attempts to fix the SuperIO problem:

  - a SuperIO subsystem proposed by Evgeniy Polyakov (cc'd);

  - a simple SuperIO locks coordinator proposed by Jim Cromie (also
    cc'd; http://thread.gmane.org/gmane.linux.drivers.sensors/10052 -
    can't find actual patches).

However, the mainline kernel still does not have anything for proper
SuperIO access locking.

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

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

>>>>> "Sergey" == Sergey Vlasov <vsu@altlinux.ru> writes:

Sergey> Actually the situation is worse.  This driver pokes at SuperIO
Sergey> configuration registers, which are shared by all logical
Sergey> devices of the SuperIO chip.

Exactly (this is something we discussed on a French
mailing-list). However, in our case (dedibox.fr hardware), that was
not an issue because we only use single-core boards, no other parts
of the SuperIO are used at the same time and every existing code sets
the logical device number to use each time it needs to access it.

Also note that this situation exists with any Winbond SuperIO already
in the kernel, this is not specific to this one.

A SuperIO subsystem would be the cleanest solution IMO, even if at the
beginning it only provides lock/unlock functionality. Then it could be
extended as to factor common operation (selecting a logical device and
reading/writing registers) and to allow access to the device from
userland.

On an unrelated subject, the whole watchdog subsystem would benefit
from a refactoring; the code looking for a 'V' in a write() operation
is present at least 35 times.

  Sam
-- 
Samuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/

Re: [PATCH] watchdog: add support for w83697hg chip

[Alexey Dobriyan]

On Sat, Sep 09, 2006 at 06:28:44PM +0200, Samuel Tardieu wrote:
> --- /dev/null
> +++ b/drivers/char/watchdog/w83697hf_wdt.c

> +#define W83697HF_EFER (wdt_io+0)   /* Extended function enable register */
> +#define W83697HF_EFIR (wdt_io+0)   /* Extended function index register */
> +#define W83697HF_EFDR (wdt_io+1)   /* Extended function data register */

Perhaps REG_ENABLE, REG_IDX, REG_DATA (with common short prefix) so
you won't refer to comments every time to understant meaning?

> +static inline void
> +w83697hf_unlock(void)

Make it on one line:

	static inline void w83697hf_unlock(void)

Ditto for all such short lines.

> +static int
> +w83697hf_init(void)

Can be __init?

> +{
> +	if (!request_region(wdt_io, 2, WATCHDOG_NAME)) {
> +		printk(KERN_ERR PFX "I/O address 0x%x already in use\n", wdt_io);
> +		return -EIO;
> +	}
> +
> +	printk(KERN_DEBUG PFX "Looking for watchdog at address 0x%x\n", wdt_io);
> +	w83697hf_unlock();
> +	if (w83697hf_get_reg(0x20) == 0x60) {
> +		printk(KERN_INFO PFX "watchdog found at address 0x%x\n", wdt_io);
> +		w83697hf_lock();
> +		return 0;
> +	}
> +	w83697hf_lock();   /* Reprotect in case it was a compatible device */
> +
> +	printk(KERN_INFO PFX "watchdog not found at address 0x%x\n", wdt_io);

KERN_ERR probably.

> +	release_region(wdt_io, 2);
> +	return -EIO;
> +}

Generally errorless codepath is kept straight and error codepaths branch
from it. I don't remember if this in CodingStyle, so feel free to
ignore. ;-) Something like that:

	if (w83697hf_get_reg(0x20) != 0x60) {
		w83697hf_lock();   /* Reprotect in case it was a compatible device */
			...
		return
	}
	printk(KERN_INFO PFX "watchdog found at address 0x%x\n", wdt_io);
		...

Re: [PATCH] watchdog: add support for w83697hg chip

[Samuel Tardieu]

On 10/09, Alexey Dobriyan wrote:

| > +#define W83697HF_EFER (wdt_io+0)   /* Extended function enable register */
| > +#define W83697HF_EFIR (wdt_io+0)   /* Extended function index register */
| > +#define W83697HF_EFDR (wdt_io+1)   /* Extended function data register */
| 
| Perhaps REG_ENABLE, REG_IDX, REG_DATA (with common short prefix) so
| you won't refer to comments every time to understant meaning?

When interfacing with the hardware, I prefer to keep the names given in
the datasheet to avoid ambiguities.

| > +static inline void
| > +w83697hf_unlock(void)
| 
| Make it on one line:

Done.

| > +static int w83697hf_init(void)
| 
| Can be __init?

Nope, wdt_init is __init.

| > +	printk(KERN_INFO PFX "watchdog not found at address 0x%x\n", wdt_io);
| 
| KERN_ERR probably.

No, this is used in autodetection routine, a final KERN_ERR will be
used if none of the two autoprobed address has a device attached.

| > +	release_region(wdt_io, 2);
| > +	return -EIO;
| > +}
| 
| Generally errorless codepath is kept straight and error codepaths branch
| from it. I don't remember if this in CodingStyle, so feel free to
| ignore. ;-)

I checked it and this is ok :) In fact, the code would be more obscure
by using goto the other way around.

Thanks for your review, updated patch follows.



Winbond W83697HF/W83697HGHG watchdog timer

The Winbond SuperIO W83697HF/HG includes a watchdog that can count from
1 to 255 seconds (or minutes). This drivers allows the seconds mode to
be used. It exposes a standard /dev/watchdog interface. This chip is
currently being used on some motherboards designed by VIA.

By default, the module looks for a chip at I/O port 0x4e. The chip can
be configured to be at 0x2e on some motherboards, the address can be
chosen using the wdt_io module parameter. Using 0 will try to autodetect
the address.

Signed-off-by: Samuel Tardieu <sam@rfc1149.net>

diff -r 92038c30d67b drivers/char/watchdog/Kconfig
--- a/drivers/char/watchdog/Kconfig	Sat Sep 09 17:30:15 2006 +0200
+++ b/drivers/char/watchdog/Kconfig	Sat Sep 09 17:31:47 2006 +0200
@@ -371,6 +371,21 @@ config W83627HF_WDT
 
 	  Most people will say N.
 
+config W83697HF_WDT
+	tristate "W83697HF/W83697HG Watchdog Timer"
+	depends on WATCHDOG && X86
+	---help---
+	  This is the driver for the hardware watchdog on the W83697HF/HG
+	  chipset as used in Dedibox/VIA motherboards (and likely others).
+	  This watchdog simply watches your kernel to make sure it doesn't
+	  freeze, and if it does, it reboots your computer after a certain
+	  amount of time.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called w83697hf_wdt.
+
+	  Most people will say N.
+
 config W83877F_WDT
 	tristate "W83877F (EMACS) Watchdog Timer"
 	depends on WATCHDOG && X86
diff -r 92038c30d67b drivers/char/watchdog/Makefile
--- a/drivers/char/watchdog/Makefile	Sat Sep 09 17:30:15 2006 +0200
+++ b/drivers/char/watchdog/Makefile	Sat Sep 09 17:31:47 2006 +0200
@@ -51,6 +51,7 @@ obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_SBC8360_WDT) += sbc8360.o
 obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
 obj-$(CONFIG_W83627HF_WDT) += w83627hf_wdt.o
+obj-$(CONFIG_W83697HF_WDT) += w83697hf_wdt.o
 obj-$(CONFIG_W83877F_WDT) += w83877f_wdt.o
 obj-$(CONFIG_W83977F_WDT) += w83977f_wdt.o
 obj-$(CONFIG_MACHZ_WDT) += machzwd.o
diff -r 92038c30d67b drivers/char/watchdog/w83697hf_wdt.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/char/watchdog/w83697hf_wdt.c	Sun Sep 10 00:03:40 2006 +0200
@@ -0,0 +1,412 @@
+/*
+ *	w83697hf/hg WDT driver
+ *
+ *	(c) Copyright 2006 Samuel Tardieu <sam@rfc1149.net>
+ *	(c) Copyright 2006 Marcus Junker <junker@anduras.de>
+ *
+ *	Based on w83627hf_wdt which is based on wadvantechwdt.c
+ *	which is based on wdt.c.
+ *	Original copyright messages:
+ *
+ *	(c) Copyright 2003 Pádraig Brady <P@draigBrady.com>
+ *
+ *	(c) Copyright 2000-2001 Marek Michalkiewicz <marekm@linux.org.pl>
+ *
+ *	(c) Copyright 1996 Alan Cox <alan@redhat.com>, All Rights Reserved.
+ *				http://www.redhat.com
+ *
+ *	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.
+ *
+ *	Neither Alan Cox nor CymruNet Ltd. admit liability nor provide
+ *	warranty for any of this software. This material is provided
+ *	"AS-IS" and at no charge.
+ *
+ *	(c) Copyright 1995    Alan Cox <alan@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/types.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+#include <linux/fs.h>
+#include <linux/ioport.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/init.h>
+
+#include <asm/io.h>
+#include <asm/semaphore.h>
+#include <asm/uaccess.h>
+#include <asm/system.h>
+
+#define WATCHDOG_NAME "w83697hf/hg WDT"
+#define PFX WATCHDOG_NAME ": "
+#define WATCHDOG_TIMEOUT 60		/* 60 sec default timeout */
+
+static unsigned long wdt_is_open;
+static char expect_close;
+static DECLARE_MUTEX(dev_lock);         /* Protect sequential operations */
+
+/* You must set this - there is no sane way to probe for this board. */
+static int wdt_io = 0x2e;
+module_param(wdt_io, int, 0);
+MODULE_PARM_DESC(wdt_io, "w83697hf/hg WDT io port (default 0x2e, 0 = autodetect)");
+
+static int timeout = WATCHDOG_TIMEOUT;	/* in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. 1<= timeout <=255, default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=CONFIG_WATCHDOG_NOWAYOUT)");
+
+/*
+ *	Kernel methods.
+ */
+
+#define W83697HF_EFER (wdt_io+0)   /* Extended function enable register */
+#define W83697HF_EFIR (wdt_io+0)   /* Extended function index register */
+#define W83697HF_EFDR (wdt_io+1)   /* Extended function data register */
+
+static inline void w83697hf_unlock(void)
+{
+	outb_p(0x87, W83697HF_EFER);
+	outb_p(0x87, W83697HF_EFER);
+}
+
+static inline void w83697hf_lock(void)
+{
+	outb_p(0xAA, W83697HF_EFER);
+}
+
+/*
+ *	The three functions w83697hf_get_reg(), w83697_set_reg() and
+ *	wdt_ctrl() must be called with the device unlocked.
+ */
+
+static unsigned char w83697hf_get_reg(unsigned char reg)
+{
+	outb_p(reg, W83697HF_EFIR);
+	return inb_p(W83697HF_EFDR);
+}
+
+static void w83697hf_set_reg(unsigned char reg, unsigned char data)
+{
+	outb_p(reg, W83697HF_EFIR);
+	outb_p(data, W83697HF_EFDR);
+}
+
+static void wdt_ctrl(int timeout)
+{
+	w83697hf_set_reg(0xF4, timeout);
+}
+
+static void w83697hf_select_wdt(void)
+{
+	down(&dev_lock);
+	w83697hf_unlock();
+	w83697hf_set_reg(0x07, 0x08);   /* Switch to logic device 8 */
+}
+
+static inline void w83697hf_deselect_wdt(void)
+{
+	w83697hf_lock();
+	up(&dev_lock);
+}
+
+static void wdt_ping(void)
+{
+	w83697hf_select_wdt();
+	wdt_ctrl(timeout);
+	w83697hf_deselect_wdt();
+}
+
+static void wdt_enable(void)
+{
+	unsigned char bbuf;
+
+	w83697hf_select_wdt();
+
+	bbuf = w83697hf_get_reg(0x29);
+	bbuf &= ~0x60;
+	bbuf |= 0x20;
+	w83697hf_set_reg(0x29, bbuf);   /* Set pin 119 to WDTO# mode */
+
+	bbuf = w83697hf_get_reg(0xF3);
+	bbuf &= ~0x04;
+	w83697hf_set_reg(0xF3, bbuf);   /* Count mode is seconds */
+	w83697hf_set_reg(0x30, 1);      /* Enable timer */
+
+	w83697hf_deselect_wdt();
+}
+
+static void wdt_disable(void)
+{
+	w83697hf_select_wdt();
+	w83697hf_set_reg(0x30, 0);       /* Disable timer */
+	wdt_ctrl(0);
+	w83697hf_deselect_wdt();
+}
+
+static int wdt_set_heartbeat(int t)
+{
+	if ((t < 1) || (t > 255))
+		return -EINVAL;
+
+	timeout = t;
+	return 0;
+}
+
+static ssize_t
+wdt_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	if (count) {
+		if (!nowayout) {
+			size_t i;
+      
+			expect_close = 0;
+      
+			for (i = 0; i != count; i++) {
+				char c;
+				if (get_user(c, buf+i))
+					return -EFAULT;
+				if (c == 'V')
+					expect_close = 42;
+			}
+		}
+		wdt_ping();
+	}
+	return count;
+}
+
+static int
+wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+	  unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	int new_timeout;
+	static struct watchdog_info ident = {
+		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE,
+		.firmware_version = 1,
+		.identity = "W83697HF WDT",
+	};
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		if (copy_to_user(argp, &ident, sizeof(ident)))
+			return -EFAULT;
+		break;
+
+	case WDIOC_GETSTATUS:
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(0, p);
+
+	case WDIOC_KEEPALIVE:
+		wdt_ping();
+		break;
+
+	case WDIOC_SETTIMEOUT:
+		if (get_user(new_timeout, p))
+			return -EFAULT;
+		if (wdt_set_heartbeat(new_timeout))
+			return -EINVAL;
+		wdt_ping();
+		/* Fall */
+
+	case WDIOC_GETTIMEOUT:
+		return put_user(timeout, p);
+
+	case WDIOC_SETOPTIONS:
+	{
+		int options, retval = -EINVAL;
+      
+		if (get_user(options, p))
+			return -EFAULT;
+      
+		if (options & WDIOS_DISABLECARD) {
+			wdt_disable();
+			retval = 0;
+		}
+
+		if (options & WDIOS_ENABLECARD) {
+			wdt_ping();
+			retval = 0;
+		}
+
+		return retval;
+	}
+
+	default:
+		return -ENOTTY;
+	}
+	return 0;
+}
+
+static int wdt_open(struct inode *inode, struct file *file)
+{
+	if (test_and_set_bit(0, &wdt_is_open))
+		return -EBUSY;
+	/*
+	 *	Activate
+	 */
+
+	wdt_enable();
+	return nonseekable_open(inode, file);
+}
+
+static int wdt_close(struct inode *inode, struct file *file)
+{
+	if (expect_close == 42) {
+		wdt_disable();
+	} else {
+		printk(KERN_CRIT PFX "Unexpected close, not stopping watchdog!\n");
+		wdt_ping();
+	}
+	expect_close = 0;
+	clear_bit(0, &wdt_is_open);
+	return 0;
+}
+
+/*
+ *	Notifier for system down
+ */
+
+static int wdt_notify_sys(struct notifier_block *this, unsigned long code,
+	       void *unused)
+{
+	if (code == SYS_DOWN || code == SYS_HALT) {
+		/* Turn the WDT off */
+		wdt_disable();
+	}
+	return NOTIFY_DONE;
+}
+
+/*
+ *	Kernel Interfaces
+ */
+
+static struct file_operations wdt_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= wdt_write,
+	.ioctl		= wdt_ioctl,
+	.open		= wdt_open,
+	.release	= wdt_close,
+};
+
+static struct miscdevice wdt_miscdev = {
+	.minor = WATCHDOG_MINOR,
+	.name = "watchdog",
+	.fops = &wdt_fops,
+};
+
+/*
+ *	The WDT needs to learn about soft shutdowns in order to
+ *	turn the timebomb registers off.
+ */
+
+static struct notifier_block wdt_notifier = {
+	.notifier_call = wdt_notify_sys,
+};
+
+static int w83697hf_init(void)
+{
+	if (!request_region(wdt_io, 2, WATCHDOG_NAME)) {
+		printk(KERN_ERR PFX "I/O address 0x%x already in use\n", wdt_io);
+		return -EIO;
+	}
+  
+	printk(KERN_DEBUG PFX "Looking for watchdog at address 0x%x\n", wdt_io);
+	w83697hf_unlock();
+	if (w83697hf_get_reg(0x20) == 0x60) {
+		printk(KERN_INFO PFX "watchdog found at address 0x%x\n", wdt_io);
+		w83697hf_lock();
+		return 0;
+	}
+	w83697hf_lock();   /* Reprotect in case it was a compatible device */
+  
+	printk(KERN_INFO PFX "watchdog not found at address 0x%x\n", wdt_io);
+	release_region(wdt_io, 2);
+	return -EIO;
+}
+
+static int __init wdt_init(void)
+{
+	int ret, autodetect;
+  
+	printk(KERN_INFO PFX "WDT driver for W83697HF/HG initializing\n");
+  
+	autodetect = wdt_io == 0;
+	if (autodetect)
+		wdt_io = 0x2e;
+  
+	if (!w83697hf_init())
+		goto found;
+  
+	if (autodetect) {
+		wdt_io = 0x4e;
+		if (!w83697hf_init())
+			goto found;
+	}
+  
+	printk(KERN_ERR PFX "No W83697HF/HG could be found\n");
+	ret = -EIO;
+	goto out;
+  
+ found:
+
+	w83697hf_select_wdt();
+	wdt_ctrl(0);                    /* Disable watchdog until first use */
+	w83697hf_deselect_wdt();
+
+	if (wdt_set_heartbeat(timeout)) {
+		wdt_set_heartbeat(WATCHDOG_TIMEOUT);
+		printk(KERN_INFO PFX "timeout value must be 1<=timeout<=255, using %d\n",
+		       WATCHDOG_TIMEOUT);
+	}
+  
+	ret = register_reboot_notifier(&wdt_notifier);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register reboot notifier (err=%d)\n",
+			ret);
+		goto unreg_regions;
+	}
+  
+	ret = misc_register(&wdt_miscdev);
+	if (ret != 0) {
+		printk (KERN_ERR PFX "cannot register miscdev on minor=%d (err=%d)\n",
+			WATCHDOG_MINOR, ret);
+		goto unreg_reboot;
+	}
+  
+	printk (KERN_INFO PFX "initialized. timeout=%d sec (nowayout=%d)\n",
+		timeout, nowayout);
+  
+ out:
+	return ret;
+ unreg_reboot:
+	unregister_reboot_notifier(&wdt_notifier);
+ unreg_regions:
+	release_region(wdt_io, 2);
+	goto out;
+}
+
+static void __exit wdt_exit(void)
+{
+	misc_deregister(&wdt_miscdev);
+	unregister_reboot_notifier(&wdt_notifier);
+	release_region(wdt_io, 2);
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Samuel Tardieu <sam@rfc1149.net>");
+MODULE_DESCRIPTION("w83697hf WDT driver");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Re: [PATCH] watchdog: add support for w83697hg chip

[Alexey Dobriyan]

On Sun, Sep 10, 2006 at 12:11:56AM +0200, Samuel Tardieu wrote:
> | > +static int w83697hf_init(void)
> |
> | Can be __init?
>
> Nope, wdt_init is __init.

Functions called only from __init functions can be marked as __init. ;-)

Re: [PATCH] watchdog: add support for w83697hg chip

[Evgeniy Polyakov]

Hello.

On Sat, Sep 09, 2006 at 10:02:56PM +0400, Sergey Vlasov (vsu@altlinux.ru) wrote:
> I can find at least two attempts to fix the SuperIO problem:
> 
>   - a SuperIO subsystem proposed by Evgeniy Polyakov (cc'd);
> 
>   - a simple SuperIO locks coordinator proposed by Jim Cromie (also
>     cc'd; http://thread.gmane.org/gmane.linux.drivers.sensors/10052 -
>     can't find actual patches).
> 
> However, the mainline kernel still does not have anything for proper
> SuperIO access locking.

I created SuperIO subsystem for soekris board initially.
Later it was extended to support scx200/scx100 gpio (for w1 subsytem).
There is support for acb(i2c) bus, GPIO, and some stub for other
elements in pc8736x superIO chip.
But I was told those days (about at least 1.5 years ago) in lm_sensors 
mail list that splitting all that functionality into separated modules 
is the way to go.

As far as I recall pc8736x GPIO was added recently.

Here is one of the implementations posted to lm_sensors@:
http://archives.andrew.net.au/lm-sensors/msg27895.html

And here is my drawing board with image of how it looks (just for pure
interest) in my mind :) :
http://tservice.net.ru/~s0mbre/old/?section=gallery&item=superio_design

-- 
	Evgeniy Polyakov

Re: [PATCH] watchdog: add support for w83697hg chip

[Wim Van Sebroeck]

Hi Sergey,

> Actually the situation is worse.  This driver pokes at SuperIO
> configuration registers, which are shared by all logical devices of the
> SuperIO chip.  There are other drivers which touch these registers -
> e.g., drivers/hwmon/w83627hf.c handles the hardware monitor part of this
> chip; many other hwmon drivers handle other SuperIO devices.  Hardware
> monitor drivers access the SuperIO config during initialization and do
> not request that port region, therefore loading hwmon drivers when
> w83697hf_wdt is loaded can lead to conflicts.

My opinion: a watchdog device is a special "harware monitoring" device and
thus we should combine the existing hwmon and watchdog code in one driver
where possible.
This will definitely not solve all SuperIO alike problems but it is a first
step towards having a single device driver for every autonomous piece of
electronics.

We can start with the Winbond SuperIO chipsets (Rudolf allready started
this (see his initial mail about Watchdog device classes)).
We should also change/convert the /dev/temperature setup in the watchdog
drivers to a hwmon-alike setup. I will definitely do this for the pcwd 
drivers. And secondly we should define how we handle "temperature panic's"
because there we still have differences between the watchdog drivers.

Greetings,
Wim.

[RFC-patch 0/3] SuperIO locks coordinator (was: watchdog: add support for w83697hg chip)

[Jim Cromie]

Sergey Vlasov wrote:
> On Sat, 09 Sep 2006 16:25:25 +0100 Alan Cox wrote:
>
>   
>> No kernel level locking anywhere in the driver. Yet you could have two
>> people accessing it at once.
>>     
>
> Actually the situation is worse.  This driver pokes at SuperIO
> configuration registers, which are shared by all logical devices of the
> SuperIO chip.  There are other drivers which touch these registers -
> e.g., drivers/hwmon/w83627hf.c handles the hardware monitor part of this
> chip; many other hwmon drivers handle other SuperIO devices.  Hardware
> monitor drivers access the SuperIO config during initialization and do
> not request that port region, therefore loading hwmon drivers when
> w83697hf_wdt is loaded can lead to conflicts.
>
> More places which seem to touch SuperIO config:
>
>   - drivers/parport/parport_pc.c
>   - drivers/net/irda/nsc-ircc.c
>   - drivers/net/irda/smsc-ircc2.c
>   - drivers/net/irda/via-ircc.h
>
> I can find at least two attempts to fix the SuperIO problem:
>
>   - a SuperIO subsystem proposed by Evgeniy Polyakov (cc'd);
>
>   - a simple SuperIO locks coordinator proposed by Jim Cromie (also
>     cc'd; http://thread.gmane.org/gmane.linux.drivers.sensors/10052 -
>     can't find actual patches).
>
>   

So, with that as motivation, Ive dusted off the old patches.

The superio_locks odule creates an array (default=3) of superio-locks
structures, and doles them out to requesting modules.

Drivers which want to coordinate their use of a SuperIO device
reserve one of those structures by specifying where they expect
to find the superio port, and the device-ids theyre looking for,
the reservation routine checks for superio-port presence,
reserves one of the structures, and returns its address.
This approach avoids arbitrary scanning for superio ports,
and put the onus on the client-driver, which must know it already.

When a 2nd module which wants to use the device makes its request,
it gets the same pointer, and thus the same lock.

Thereafter, the 2+ client modules use the lock in the structure to 
coordinate
access with other client modules using the same device.
Drivers use superio_enter/exit() to do their own locking/unlocking,
letting them trade off lock-fiddling vs locked-duration.

The module does *not* guarantee anything, it offers no protection
against rogue (existing) modules which dont use the superio-locks 
coordinator.
( is anti-rogue protection even possible ?
    Perhaps, IFF modules reserve the 2 superio addresses - semi-rogue ? )


Thanks Sergey, for cc'g, and for your off-list review that saved me some 
embarrassment,
and saved the list from a sloppy patch.


1/3   adds superio_locks, into newly created drivers/isa
    Its a bit chatty, which I presume is ok for now..
    the number of reservations is settable via modparam: max_locks

soekris:~# modprobe superio_locks
[ 8715.042408] superio: initializing with 3 reservation slots
soekris:~# rmmod superio_locks
[ 8701.149869] superio: releasing 3 superio reservation slots


2/3   adapts drivers/hwmon/pc87360 to use superio_find()
    this module needs superio port only during initialization,
    so releases it quickly.

soekris:~# modprobe pc87360
[ 8794.528967] superio: no devid e1 at 2e
[ 8794.532929] superio: no devid e8 at 2e
[ 8794.536845] superio: no devid e4 at 2e
[ 8794.540768] superio: no devid e5 at 2e
[ 8794.544688] superio: allocating slot 0, addr 2e for device e9
[ 8794.550606] superio: devid e9 found at 2e
[ 8794.554802] pc87360: Device 0x09 not activated
[ 8794.559423] superio: releasing last user of superio-port 2e


3/3   adapts drivers/char/pc8736x_gpio
    this module needs the superio-port at runtime to alter pin-configs,
    so it doesnt release its superio-port reservation until module-exit

soekris:~# modprobe pc8736x_gpio
[ 8996.498524] platform pc8736x_gpio.0: NatSemi pc8736x GPIO Driver 
Initializing
[ 8996.505892] superio: no devid e1 at 2e
[ 8996.509826] superio: no devid e8 at 2e
[ 8996.513739] superio: no devid e4 at 2e
[ 8996.517669] superio: no devid e5 at 2e
[ 8996.521594] superio: allocating slot 0, addr 2e for device e9
[ 8996.527582] superio: devid e9 found at 2e
[ 8996.531819] platform pc8736x_gpio.0: GPIO ioport 6600 reserved
soekris:~#
soekris:~# rmmod pc8736x_gpio
[ 9008.702637] superio: releasing last user of superio-port 2e

soekris:~# modprobe pc8736x_gpio
[ 4595.154139] superio: initializing with 3 reservation slots
[ 4596.742521] platform pc8736x_gpio.0: NatSemi pc8736x GPIO Driver 
Initializing
[ 4596.750192] superio: no devid:e1 at port:2e
[ 4596.755212] superio: no devid:e8 at port:2e
[ 4596.759702] superio: no devid:e4 at port:2e
[ 4596.764184] superio: no devid:e5 at port:2e
[ 4596.768663] superio: allocating slot 0, addr 2e for device e9
[ 4596.774698] superio: found devid:e9 port:2e
[ 4596.779419] platform pc8736x_gpio.0: GPIO ioport 6600 reserved
soekris:~#
soekris:~# modprobe pc87360
[ 4603.693727] superio: no devid:e1 at port:2e
[ 4603.698245] superio: no devid:e8 at port:2e
[ 4603.703429] superio: no devid:e4 at port:2e
[ 4603.707934] superio: no devid:e5 at port:2e
[ 4603.712950] superio: sharing port:2e dev:e9 users:2
[ 4603.718125] superio: found devid:e9 port:2e
[ 4603.722609] pc87360: Device 0x09 not activated
[ 4604.965883] pc87360 9191-6620: VLM conversion set to 1s period, 160us 
delay
soekris:~#


Ive done limited testing, using the 2 drivers for my PC-87366 chip,
to insure that I havent badly botched something, (I still may have ;)
and looked at several of the hwmon drivers that operate superio ports.
comments in code speak to those observations.

Re: [RFC-patch 1/3] SuperIO locks coordinator

[Jim Cromie]

> 1/3   adds superio_locks, into newly created drivers/isa
>    Its a bit chatty, which I presume is ok for now..
>    the number of reservations is settable via modparam: max_locks
>
signoff later..


diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/Kconfig 6locks-2/drivers/isa/Kconfig
--- 6locks-1/drivers/isa/Kconfig	1969-12-31 17:00:00.000000000 -0700
+++ 6locks-2/drivers/isa/Kconfig	2006-09-13 09:54:18.000000000 -0600
@@ -0,0 +1,7 @@
+
+config SUPERIO_LOCKS
+	tristate "Super-IO port sharing"
+	help
+	  this module provides locks for use by drivers which need to
+	  share access to a multi-function device via its superio port, 
+	  and which register that port.
diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/Makefile 6locks-2/drivers/isa/Makefile
--- 6locks-1/drivers/isa/Makefile	1969-12-31 17:00:00.000000000 -0700
+++ 6locks-2/drivers/isa/Makefile	2006-09-13 09:54:18.000000000 -0600
@@ -0,0 +1 @@
+obj-$(CONFIG_SUPERIO_LOCKS)	+= superio_locks.o
diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/superio_locks.c 6locks-2/drivers/isa/superio_locks.c
--- 6locks-1/drivers/isa/superio_locks.c	1969-12-31 17:00:00.000000000 -0700
+++ 6locks-2/drivers/isa/superio_locks.c	2006-09-13 14:56:32.000000000 -0600
@@ -0,0 +1,169 @@
+#include <linux/config.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/mutex.h>
+#include <asm/io.h>
+#include <linux/superio-locks.h>
+
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/i2c-isa.h>
+#include <linux/err.h>
+
+MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com");
+MODULE_LICENSE("GPL");
+
+/**
+   module provides a means for modules to register their use of a
+   Super-IO port, and provides an access-lock for the registering
+   modules to use to coordinate with each other.  Consider it a
+   parking-attendant's key-board.  Design is perhaps ISA centric,
+   maybe formalize this, with (platform|isa)_driver.
+*/
+
+static int max_locks = 3;	/* 1 is enough for 90% uses */
+module_param(max_locks, int, 0);
+MODULE_PARM_DESC(max_locks,
+		 " Number of sio-lock clients to serve (default=3)");
+
+static struct superio *sio_locks;
+static int num_locks;
+static struct mutex reservation_lock;
+
+
+/* TB-Replaced by something better: platform_driver ? */
+#define dprintk(...)	printk(KERN_NOTICE "superio: " __VA_ARGS__)
+
+/* superio_get() checks whether the expected SuperIO device is
+   present at a specific cmd-addr.  Use in loop to scan.
+*/
+
+struct superio* superio_get(u16 cmd_addr, u8 dev_id_addr,
+			    u8 want_devid)
+{
+	int slot, rc, mydevid;
+
+	mutex_lock(&reservation_lock);
+
+	/* share any already allocated lock for this cmd_addr, device-id */
+	for (slot = 0; slot < max_locks; slot++) {
+		if (sio_locks[slot].users 
+		    && cmd_addr == sio_locks[slot].sioaddr
+		    && want_devid == sio_locks[slot].devid) {
+
+			if (sio_locks[slot].users == 255) {
+				dprintk("too many drivers sharing port %x\n", cmd_addr);
+				mutex_unlock(&reservation_lock);
+				return 0;
+			}
+			sio_locks[slot].users++;
+			dprintk("sharing port:%x dev:%x users:%d\n",
+				cmd_addr, want_devid, sio_locks[slot].users);
+			mutex_unlock(&reservation_lock);
+			return &sio_locks[slot];
+		}
+	}
+	/* read the device-id-address */
+	outb(dev_id_addr, cmd_addr);
+	mydevid = inb(cmd_addr+1);
+
+	/* but 1st, check that the cmd register remembers the val just written */
+	rc = inb(cmd_addr);
+	if (rc != dev_id_addr) {
+		dprintk("superio_cmdaddr %x absent %d\n", cmd_addr, rc);
+		mutex_unlock(&reservation_lock);
+		return NULL;
+	}
+	/* test for the desired device id value */
+	if (mydevid != want_devid) {
+		mutex_unlock(&reservation_lock);
+		return NULL;
+	}
+	/* find 1st unused slot */
+	for (slot = 0; slot < max_locks; slot++)
+		if (!sio_locks[slot].users)
+			break;
+
+	if (slot >= max_locks) {
+		printk(KERN_ERR "No superio-locks left. increase max_locks\n");
+		mutex_unlock(&reservation_lock);
+		return NULL;
+	}
+	dprintk("allocating slot %d, addr %x for device %x\n",
+		slot, cmd_addr, want_devid);
+
+	sio_locks[slot].sioaddr = cmd_addr;
+	sio_locks[slot].devid = want_devid;
+	sio_locks[slot].users = 1;
+	num_locks++;
+
+	mutex_unlock(&reservation_lock);
+	return &sio_locks[slot];
+}
+EXPORT_SYMBOL_GPL(superio_get);
+
+/* array args must be null terminated */
+struct superio* superio_find(u16 cmd_addrs[], u8 devid_addr,
+			     u8 want_devids[])
+{
+	int i, j;
+	struct superio* gate;
+
+	for (i = 0; cmd_addrs[i]; i++) {
+		for (j = 0; want_devids[j]; j++) {
+			gate = superio_get(cmd_addrs[i], devid_addr,
+					   want_devids[j]);
+			if (gate) {
+				dprintk("found devid:%x port:%x\n",
+					want_devids[j], cmd_addrs[i]);
+				return gate;
+			} else
+				dprintk("no devid:%x at port:%x\n",
+					want_devids[j], cmd_addrs[i]);
+		}
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(superio_find);
+
+void superio_release(struct superio* const gate)
+{
+	if (gate < &sio_locks[0] || gate >= &sio_locks[max_locks]) {
+		printk(KERN_ERR
+		       " superio: attempt to release corrupted superio-lock"
+		       " %p vs %p\n", gate, &sio_locks);
+		return;
+	}
+	if (!(--gate->users))
+		dprintk("releasing last user of superio-port %x\n", gate->sioaddr);
+	return;
+}
+EXPORT_SYMBOL_GPL(superio_release);
+
+static int superio_locks_init_module(void)
+{
+	int i;
+
+	dprintk("initializing with %d reservation slots\n", max_locks);
+	sio_locks = kzalloc(max_locks*sizeof(struct superio), GFP_KERNEL);
+	if (!sio_locks) {
+		printk(KERN_ERR "superio: no memory\n");
+		return -ENOMEM;
+	}
+	for (i = 0; i < max_locks; i++)
+		mutex_init(&sio_locks[i].lock);
+
+	mutex_init(&reservation_lock);
+	return 0;
+}
+
+static void superio_locks_cleanup_module(void)
+{
+	dprintk("releasing %d superio reservation slots\n", max_locks);
+	kfree(sio_locks);
+}
+
+module_init(superio_locks_init_module);
+module_exit(superio_locks_cleanup_module);
diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/Kconfig 6locks-2/drivers/Kconfig
--- 6locks-1/drivers/Kconfig	2006-09-07 16:11:30.000000000 -0600
+++ 6locks-2/drivers/Kconfig	2006-09-13 09:54:18.000000000 -0600
@@ -74,4 +74,6 @@ source "drivers/rtc/Kconfig"
 
 source "drivers/dma/Kconfig"
 
+source "drivers/isa/Kconfig"
+
 endmenu
diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/Makefile 6locks-2/drivers/Makefile
--- 6locks-1/drivers/Makefile	2006-09-07 16:11:30.000000000 -0600
+++ 6locks-2/drivers/Makefile	2006-09-13 09:54:18.000000000 -0600
@@ -76,3 +76,4 @@ obj-$(CONFIG_CRYPTO)		+= crypto/
 obj-$(CONFIG_SUPERH)		+= sh/
 obj-$(CONFIG_GENERIC_TIME)	+= clocksource/
 obj-$(CONFIG_DMA_ENGINE)	+= dma/
+obj-$(CONFIG_ISA)		+= isa/
diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/include/linux/superio-locks.h 6locks-2/include/linux/superio-locks.h
--- 6locks-1/include/linux/superio-locks.h	1969-12-31 17:00:00.000000000 -0700
+++ 6locks-2/include/linux/superio-locks.h	2006-09-13 14:21:08.000000000 -0600
@@ -0,0 +1,55 @@
+#include <linux/mutex.h>
+#include <asm/io.h>
+
+/* Super-IO ports are found in low-pin-count hardware (typically ISA,
+   any others ?).  They usually provide access to many functional
+   units, so many drivers must share the superio port.  This struct
+   provides a lock that allows the drivers to coordinate access to that
+   port.
+*/
+struct superio {
+	struct mutex lock;	/* lock shared amongst user drivers */
+	u16 sioaddr;		/* port's tested cmd-address */
+	u8 devid;		/* devid found by the registering driver */
+	u8 users;		/* I cant imagine >256 user drivers */
+};
+
+/* array args must be null terminated */
+struct superio* superio_find(u16 sioaddrs[], u8 devid_addr, u8 devid_vals[]);
+struct superio* superio_get(u16 sioaddr, u8 devid_addr, u8 devid_val);
+void superio_release(struct superio* const gate);
+
+/* these locking ops do not address the idling & activation of some
+   superio devices, which will, once 'locked', ignore accesses until
+   the 'unlock' sequence is done 1st.  Unfortunately these sequences
+   vary by device, and in any case don't protect 2 drivers from
+   stepping on each other's operations.
+
+   Callbacks are a possible approach, but every driver using a device
+   would have to provide them, and only the 1st loaded module would
+   actually succeed in registering them.  Furthermore, if any driver
+   accessing a port uses the idle/activate sequences, they all must.
+   On the whole, this is complexity w/o benefit.
+*/
+static inline void superio_enter(struct superio * const sio_port)
+{
+	mutex_lock(&sio_port->lock);
+}
+
+static inline void superio_exit(struct superio * const sio_port)
+{
+	mutex_unlock(&sio_port->lock);
+}
+
+static inline void superio_outb(struct superio * const sio_port, u8 reg, u8 val)
+{
+	outb(reg, sio_port->sioaddr);
+	outb(val, sio_port->sioaddr+1);
+}
+
+static inline int superio_inb(struct superio * const sio_port, u8 reg)
+{
+	outb(reg, sio_port->sioaddr);
+	return inb(sio_port->sioaddr+1);
+}
+

Re: [PATCH] watchdog: add support for w83697hg chip

[Wim Van Sebroeck]

Hi All,

> My opinion: a watchdog device is a special "harware monitoring" device and
> thus we should combine the existing hwmon and watchdog code in one driver
> where possible.
> This will definitely not solve all SuperIO alike problems but it is a first
> step towards having a single device driver for every autonomous piece of
> electronics.
> 
> We can start with the Winbond SuperIO chipsets (Rudolf allready started
> this (see his initial mail about Watchdog device classes)).
> We should also change/convert the /dev/temperature setup in the watchdog
> drivers to a hwmon-alike setup. I will definitely do this for the pcwd 
> drivers. And secondly we should define how we handle "temperature panic's"
> because there we still have differences between the watchdog drivers.

Just an extra thought: the current drivers directory is a mix of bus related
(pci, usb), /dev/* (char, block, ...) related and functionality related 
(bluetooth, cdrom, hwmon, watchdog, ...) items. Since our device drivers are
gradually moving to the new driver structure (with sysfs support) where
everything is bus related we will probably need to think how we best manage
this towards the user that will compile his kernel. (For instance: the 
watchdog usb driver depend on USB but USB is only later selectable if you do
a make config...)
Perhaps we will have in the future a drivers directory that is even oriented
towards the different busses (pci, usb, platform, isa, eisa, sbus, ...)

Greetings,
Wim.

Re: [RFC-patch 2/3] SuperIO locks coordinator

[Jim Cromie]

>
> 2/3   adapts drivers/hwmon/pc87360 to use superio_find()
>    this module needs superio port only during initialization,
>    so releases it quickly.
>

diff -ruNp -X dontdiff -X exclude-diffs 6locks-2/drivers/hwmon/pc87360.c 6locks-3/drivers/hwmon/pc87360.c
--- 6locks-2/drivers/hwmon/pc87360.c	2006-09-13 09:50:35.000000000 -0600
+++ 6locks-3/drivers/hwmon/pc87360.c	2006-09-13 16:26:51.000000000 -0600
@@ -42,6 +42,7 @@
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/hwmon-vid.h>
+#include <linux/superio-locks.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <asm/io.h>
@@ -53,6 +54,9 @@ static u8 confreg[4];
 
 enum chips { any_chip, pc87360, pc87363, pc87364, pc87365, pc87366 };
 
+static u16 cmd_addrs[] = { 0x2E, 0x4E, 0 };
+static u8 devid_vals[] = { 0xE1, 0xE8, 0xE4, 0xE5, 0xE9, 0 };
+
 static int init = 1;
 module_param(init, int, 0);
 MODULE_PARM_DESC(init,
@@ -80,24 +84,6 @@ static const u8 logdev[3] = { FSCM, VLM,
 #define LD_IN		1
 #define LD_TEMP		2
 
-static inline void superio_outb(int sioaddr, int reg, int val)
-{
-	outb(reg, sioaddr);
-	outb(val, sioaddr+1);
-}
-
-static inline int superio_inb(int sioaddr, int reg)
-{
-	outb(reg, sioaddr);
-	return inb(sioaddr+1);
-}
-
-static inline void superio_exit(int sioaddr)
-{
-	outb(0x02, sioaddr);
-	outb(0x02, sioaddr+1);
-}
-
 /*
  * Logical devices
  */
@@ -821,17 +807,22 @@ static const struct attribute_group pc87
  * Device detection, registration and update
  */
 
-static int __init pc87360_find(int sioaddr, u8 *devid, unsigned short *addresses)
+static int __init pc87360_find(unsigned short *addresses)
 {
 	u16 val;
-	int i;
-	int nrdev; /* logical device count */
+	int i, nrdev; /* logical device count */
 
-	/* No superio_enter */
+	struct superio* const gate = superio_find(cmd_addrs, DEVID, devid_vals);
+	if (!gate) {
+		printk(KERN_WARNING "pc87360: superio port not detected, "
+		       "module not intalled.\n");
+		return -ENODEV;
+	}
+	superio_enter(gate);
+	devid = gate->devid;		/* Remember the device id */
 
 	/* Identify device */
-	val = superio_inb(sioaddr, DEVID);
-	switch (val) {
+	switch (devid) {
 	case 0xE1: /* PC87360 */
 	case 0xE8: /* PC87363 */
 	case 0xE4: /* PC87364 */
@@ -842,25 +833,23 @@ static int __init pc87360_find(int sioad
 		nrdev = 3;
 		break;
 	default:
-		superio_exit(sioaddr);
+		superio_exit(gate);
 		return -ENODEV;
 	}
-	/* Remember the device id */
-	*devid = val;
 
 	for (i = 0; i < nrdev; i++) {
 		/* select logical device */
-		superio_outb(sioaddr, DEV, logdev[i]);
+		superio_outb(gate, DEV, logdev[i]);
 
-		val = superio_inb(sioaddr, ACT);
+		val = superio_inb(gate, ACT);
 		if (!(val & 0x01)) {
 			printk(KERN_INFO "pc87360: Device 0x%02x not "
 			       "activated\n", logdev[i]);
 			continue;
 		}
 
-		val = (superio_inb(sioaddr, BASE) << 8)
-		    | superio_inb(sioaddr, BASE + 1);
+		val = (superio_inb(gate, BASE) << 8)
+		    | superio_inb(gate, BASE + 1);
 		if (!val) {
 			printk(KERN_INFO "pc87360: Base address not set for "
 			       "device 0x%02x\n", logdev[i]);
@@ -870,8 +859,8 @@ static int __init pc87360_find(int sioad
 		addresses[i] = val;
 
 		if (i==0) { /* Fans */
-			confreg[0] = superio_inb(sioaddr, 0xF0);
-			confreg[1] = superio_inb(sioaddr, 0xF1);
+			confreg[0] = superio_inb(gate, 0xF0);
+			confreg[1] = superio_inb(gate, 0xF1);
 
 #ifdef DEBUG
 			printk(KERN_DEBUG "pc87360: Fan 1: mon=%d "
@@ -886,12 +875,12 @@ static int __init pc87360_find(int sioad
 #endif
 		} else if (i==1) { /* Voltages */
 			/* Are we using thermistors? */
-			if (*devid == 0xE9) { /* PC87366 */
+			if (devid == 0xE9) { /* PC87366 */
 				/* These registers are not logical-device
 				   specific, just that we won't need them if
 				   we don't use the VLM device */
-				confreg[2] = superio_inb(sioaddr, 0x2B);
-				confreg[3] = superio_inb(sioaddr, 0x25);
+				confreg[2] = superio_inb(gate, 0x2B);
+				confreg[3] = superio_inb(gate, 0x25);
 
 				if (confreg[2] & 0x40) {
 					printk(KERN_INFO "pc87360: Using "
@@ -907,7 +896,8 @@ static int __init pc87360_find(int sioad
 		}
 	}
 
-	superio_exit(sioaddr);
+	superio_exit(gate);
+	superio_release(gate);	/* not needed for any post-load operations */
 	return 0;
 }
 
@@ -1411,14 +1401,10 @@ static struct pc87360_data *pc87360_upda
 
 static int __init pc87360_init(void)
 {
-	int i;
+	int i, status = -ENODEV;
 
-	if (pc87360_find(0x2e, &devid, extra_isa)
-	 && pc87360_find(0x4e, &devid, extra_isa)) {
-		printk(KERN_WARNING "pc87360: PC8736x not detected, "
-		       "module not inserted.\n");
-		return -ENODEV;
-	}
+	if (pc87360_find(extra_isa))
+		return status;
 
 	/* Arbitrarily pick one of the addresses */
 	for (i = 0; i < 3; i++) {
@@ -1431,10 +1417,10 @@ static int __init pc87360_init(void)
 	if (address == 0x0000) {
 		printk(KERN_WARNING "pc87360: No active logical device, "
 		       "module not inserted.\n");
-		return -ENODEV;
+		return status;
 	}
-
-	return i2c_isa_add_driver(&pc87360_driver);
+	status = i2c_isa_add_driver(&pc87360_driver);
+	return status;
 }
 
 static void __exit pc87360_exit(void)

Re: [RFC-patch 3/3] SuperIO locks coordinator - use in pc8736x_gpio

[Jim Cromie]

>
> 3/3   adapts drivers/char/pc8736x_gpio
>    this module needs the superio-port at runtime to alter pin-configs,
>    so it doesnt release its superio-port reservation until module-exit
>

diff -ruNp -X dontdiff -X exclude-diffs 6locks-3/drivers/char/pc8736x_gpio.c 6locks-4/drivers/char/pc8736x_gpio.c
--- 6locks-3/drivers/char/pc8736x_gpio.c	2006-09-07 16:11:44.000000000 -0600
+++ 6locks-4/drivers/char/pc8736x_gpio.c	2006-09-13 23:03:38.000000000 -0600
@@ -20,6 +20,7 @@
 #include <linux/mutex.h>
 #include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
+#include <linux/superio-locks.h>
 #include <asm/uaccess.h>
 
 #define DEVNAME "pc8736x_gpio"
@@ -36,12 +37,11 @@ static DEFINE_MUTEX(pc8736x_gpio_config_
 static unsigned pc8736x_gpio_base;
 static u8 pc8736x_gpio_shadow[4];
 
-#define SIO_BASE1       0x2E	/* 1st command-reg to check */
-#define SIO_BASE2       0x4E	/* alt command-reg to check */
-
-#define SIO_SID		0x20	/* SuperI/O ID Register */
-#define SIO_SID_VALUE	0xe9	/* Expected value in SuperI/O ID Register */
+static u16 cmd_addrs[] = { 0x2E, 0x4E, 0 };
+static u8 devid_vals[] = { 0xE1, 0xE8, 0xE4, 0xE5, 0xE9, 0 };
+static struct superio* gate;
 
+#define SIO_DEVID	0x20	/* SuperI/O Device ID Register */
 #define SIO_CF1		0x21	/* chip config, bit0 is chip enable */
 
 #define PC8736X_GPIO_RANGE	16 /* ioaddr range */
@@ -62,7 +62,6 @@ static u8 pc8736x_gpio_shadow[4];
 #define SIO_GPIO_PIN_CONFIG     0xF1
 #define SIO_GPIO_PIN_EVENT      0xF2
 
-static unsigned char superio_cmd = 0;
 static unsigned char selected_device = 0xFF;	/* bogus start val */
 
 /* GPIO port runtime access, functionality */
@@ -76,35 +75,9 @@ static int port_offset[] = { 0, 4, 8, 10
 
 static struct platform_device *pdev;  /* use in dev_*() */
 
-static inline void superio_outb(int addr, int val)
-{
-	outb_p(addr, superio_cmd);
-	outb_p(val, superio_cmd + 1);
-}
-
-static inline int superio_inb(int addr)
-{
-	outb_p(addr, superio_cmd);
-	return inb_p(superio_cmd + 1);
-}
-
-static int pc8736x_superio_present(void)
-{
-	/* try the 2 possible values, read a hardware reg to verify */
-	superio_cmd = SIO_BASE1;
-	if (superio_inb(SIO_SID) == SIO_SID_VALUE)
-		return superio_cmd;
-
-	superio_cmd = SIO_BASE2;
-	if (superio_inb(SIO_SID) == SIO_SID_VALUE)
-		return superio_cmd;
-
-	return 0;
-}
-
 static void device_select(unsigned devldn)
 {
-	superio_outb(SIO_UNIT_SEL, devldn);
+	superio_outb(gate, SIO_UNIT_SEL, devldn);
 	selected_device = devldn;
 }
 
@@ -112,7 +85,7 @@ static void select_pin(unsigned iminor)
 {
 	/* select GPIO port/pin from device minor number */
 	device_select(SIO_GPIO_UNIT);
-	superio_outb(SIO_GPIO_PIN_SELECT,
+	superio_outb(gate, SIO_GPIO_PIN_SELECT,
 		     ((iminor << 1) & 0xF0) | (iminor & 0x7));
 }
 
@@ -121,19 +94,19 @@ static inline u32 pc8736x_gpio_configure
 {
 	u32 config, new_config;
 
+	superio_enter(gate);
 	mutex_lock(&pc8736x_gpio_config_lock);
 
-	device_select(SIO_GPIO_UNIT);
+	/* read pin's current config value */
 	select_pin(index);
-
-	/* read current config value */
-	config = superio_inb(func_slct);
+	config = superio_inb(gate, func_slct);
 
 	/* set new config */
 	new_config = (config & mask) | bits;
-	superio_outb(func_slct, new_config);
+	superio_outb(gate, func_slct, new_config);
 
 	mutex_unlock(&pc8736x_gpio_config_lock);
+	superio_exit(gate);
 
 	return config;
 }
@@ -188,6 +161,8 @@ static void pc8736x_gpio_set(unsigned mi
 	pc8736x_gpio_shadow[port] = val;
 }
 
+#if 0
+/* may re-enable for sysfs-gpio */
 static void pc8736x_gpio_set_high(unsigned index)
 {
 	pc8736x_gpio_set(index, 1);
@@ -197,6 +172,7 @@ static void pc8736x_gpio_set_low(unsigne
 {
 	pc8736x_gpio_set(index, 0);
 }
+#endif
 
 static int pc8736x_gpio_current(unsigned minor)
 {
@@ -269,40 +245,44 @@ static int __init pc8736x_gpio_init(void
 		rc = -ENODEV;
 		goto undo_platform_dev_alloc;
 	}
+	pc8736x_gpio_ops.dev = &pdev->dev;
+
 	dev_info(&pdev->dev, "NatSemi pc8736x GPIO Driver Initializing\n");
 
-	if (!pc8736x_superio_present()) {
+	gate = superio_find(cmd_addrs, SIO_DEVID, devid_vals);
+	if (!gate) {
 		rc = -ENODEV;
-		dev_err(&pdev->dev, "no device found\n");
+		dev_err(&pdev->dev, "no superio port found\n");
+		// goto err2;
 		goto undo_platform_dev_add;
 	}
-	pc8736x_gpio_ops.dev = &pdev->dev;
 
 	/* Verify that chip and it's GPIO unit are both enabled.
 	   My BIOS does this, so I take minimum action here
 	 */
-	rc = superio_inb(SIO_CF1);
+	superio_enter(gate);
+	rc = superio_inb(gate, SIO_CF1);
 	if (!(rc & 0x01)) {
 		rc = -ENODEV;
 		dev_err(&pdev->dev, "device not enabled\n");
-		goto undo_platform_dev_add;
+		goto undo_superio_enter;
 	}
 	device_select(SIO_GPIO_UNIT);
-	if (!superio_inb(SIO_UNIT_ACT)) {
+	if (!superio_inb(gate, SIO_UNIT_ACT)) {
 		rc = -ENODEV;
 		dev_err(&pdev->dev, "GPIO unit not enabled\n");
-		goto undo_platform_dev_add;
+		goto undo_superio_enter;
 	}
 
 	/* read the GPIO unit base addr that chip responds to */
-	pc8736x_gpio_base = (superio_inb(SIO_BASE_HADDR) << 8
-			     | superio_inb(SIO_BASE_LADDR));
+	pc8736x_gpio_base = (superio_inb(gate, SIO_BASE_HADDR) << 8
+			     | superio_inb(gate, SIO_BASE_LADDR));
 
 	if (!request_region(pc8736x_gpio_base, PC8736X_GPIO_RANGE, DEVNAME)) {
 		rc = -ENODEV;
 		dev_err(&pdev->dev, "GPIO ioport %x busy\n",
 			pc8736x_gpio_base);
-		goto undo_platform_dev_add;
+		goto undo_superio_enter;
 	}
 	dev_info(&pdev->dev, "GPIO ioport %x reserved\n", pc8736x_gpio_base);
 
@@ -329,10 +309,14 @@ static int __init pc8736x_gpio_init(void
 	cdev_init(&pc8736x_gpio_cdev, &pc8736x_gpio_fileops);
 	cdev_add(&pc8736x_gpio_cdev, devid, PC8736X_GPIO_CT);
 
+	superio_exit(gate);	/* no release, we need to stay registered */
 	return 0;
 
 undo_request_region:
 	release_region(pc8736x_gpio_base, PC8736X_GPIO_RANGE);
+undo_superio_enter:
+	superio_exit(gate);
+	superio_release(gate);
 undo_platform_dev_add:
 	platform_device_del(pdev);
 undo_platform_dev_alloc:
@@ -351,6 +335,7 @@ static void __exit pc8736x_gpio_cleanup(
 
 	platform_device_del(pdev);
 	platform_device_put(pdev);
+	superio_release(gate);
 }
 
 module_init(pc8736x_gpio_init);

Re: [RFC-patch 0/3] SuperIO locks coordinator

[Jim Cromie]

Jim Cromie wrote:

[ adding Rudolf Marek to cc, since he was kind enough to review the 
original patch ]
>
>
> Ive done limited testing, using the 2 drivers for my PC-87366 chip,
> to insure that I havent badly botched something, (I still may have ;)
> and looked at several of the hwmon drivers that operate superio ports.
> comments in code speak to those observations.
>
one of those observations, stated so they can be more easily reviewed 
(and refuted)

idle/activate IO-sequences:

Some SuperIO devices implement a pseudo-locking scheme which
turns off the port unless an activation-sequence is used 1st.
Once a port is 'idled', it will ignore access until it is re-activated
by doing a specific sequence of operations. 

Those sequences vary per-device, and generally would require a callback
to accomodate the variations.  This implies that all drivers for a 
superio device
would have to supply the (same) callbacks, with superio-locks remembering
only the 1st (they better all be the same anyway, so this by itself isnt 
a limitation).

w83627ehf.c:
static inline void superio_enter(void)
{
    outb(0x87, REG);
    outb(0x87, REG);
}
static inline void superio_exit(void)
{
    outb(0x02, REG);
    outb(0x02, VAL);
}

w83627hf.c differs from above !!!

static inline void superio_exit(void)
{
    outb(0xAA, REG);
}


The problem with these pseudo-locks is they dont really protect anyway;
if the sequences are used at all, every driver would have to unlock the 
chip b4 doing
anything else, and (I assume) activating an already active port will work,
allowing one driver to interfere with another.

I therefore redefined those functions: superio_enter/exit() now do 
mutex_lock/unlock()
on the reserved mutex.  Perhaps the API should have 
superio_lock/unlock() instead,
and leave superio_enter/exit() for drivers to implement themselves if 
they need/want it.


And let me be the 1st to throw a few rocks at my patches.

1- several devices in drivers/hwmon have 2-byte DEVICE_IDs,
and effectively have superio_inw(), (in both open-coded, and explicit 
forms).
My superio_get() cannot handle those devices.  I can fix it 'easily' by 
doing an inw()
if wanted devid>255, but that may be -ETOOHACKY

comments ?

Re: [lm-sensors] [RFC-patch 0/3] SuperIO locks coordinator

[Jim Cromie]

David Hubbard wrote:
> Hi Jim,
>
>> BTW, are the idle/activate sequences doc'd in your datasheet ?
>> I ask this cuz pc87360 has a superio-exit defined (and used), but no
>> superio-enter(),
>> and I couldnt find the idle/activate sequences docd in my datasheet.
>> With the long history of copy & modify in these drivers, its possible
>> that some cargo-cult features were inadvertently carried forward,
>> esp when drivers are written w/o actually having the hardare.
>>
>> Could you disable your superio-enter(), and see if that breaks the
>> functionality ?
>
> I'll do that and let you know how it goes. I suspect that the BIOS
> initializes the w83627ehf correctly, 
yes. very likely
> and so the superio-enter and
> -exit that are used may not be required...except during detection. 

Also likely/possible.  Some chips can be told to map their logical 
devices (LDs)
to specific ISA address ranges, then those devices can be largely or 
completely
operated using vanilla IO operations, w/o the superio-port overheads,
and the BIOS often takes care of this, and enables devices that the mobo
is designed to use.

Forex:
6600-660f : pc8736x_gpio
6620-662f : pc87360
6640-664f : pc87360

To complicate things, some LDs have features that are only controllable
via superio, pc8736x GPIO has runtime regs and configuration regs, the 
latter
are only available via superio.  These LDs are much more dependent upon
proper superio locking, vs hwmon/pc87360, which uses vanilla IO after 
detection,
and thus releases the superio-reservation once detection/initialization 
is complete.

FYI - GregKH said this on LKML, re 2.6.19
http://marc.theaimsgroup.com/?l=linux-kernel&m=115778993800623&w=2

	- The driver core was changed to allow multi-threaded device
	  probing.  This means that every device added to the system
	  gets a new kernel thread in which to do the probe sequence.
	  The PCI subsystem was modified to allow PCI drivers to do this
	  (this is made a configuration option, as it breaks numerous
	  boxes if enabled).  It does have the potential to speed up the
	  boot sequence a lot for some machines, and is even measurable
	  on single processor laptops.


This appears to increase the potential of problems related to current 
lack of superio locking,


> The
> w83627ehf is mapped into ISA I/O space (probably by the BIOS). So I'll
> test my theory and get back to you soon.
>
thanks.

> David
>
jimc

Re: [RFC-patch 1/3] SuperIO locks coordinator

[Randy.Dunlap]

On Thu, 14 Sep 2006 01:13:03 -0600 Jim Cromie wrote:

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/Kconfig 6locks-2/drivers/isa/Kconfig
> --- 6locks-1/drivers/isa/Kconfig	1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/drivers/isa/Kconfig	2006-09-13 09:54:18.000000000 -0600
> @@ -0,0 +1,7 @@
> +
> +config SUPERIO_LOCKS
> +	tristate "Super-IO port sharing"
> +	help
> +	  this module provides locks for use by drivers which need to

          This

> +	  share access to a multi-function device via its superio port, 
> +	  and which register that port.

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/drivers/isa/superio_locks.c 6locks-2/drivers/isa/superio_locks.c
> --- 6locks-1/drivers/isa/superio_locks.c	1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/drivers/isa/superio_locks.c	2006-09-13 14:56:32.000000000 -0600
> @@ -0,0 +1,169 @@
> +
> +/**
> +   module provides a means for modules to register their use of a
> +   Super-IO port, and provides an access-lock for the registering
> +   modules to use to coordinate with each other.  Consider it a
> +   parking-attendant's key-board.  Design is perhaps ISA centric,
> +   maybe formalize this, with (platform|isa)_driver.
> +*/

Two comments about that comment:

a.  Kernel long-comment style is:

/*
 * This module provides a means for modules to register
 * their use of a Super-IO port, ...
 */

b.  Don't use "/**" to begin a comment block unless the comment block
contains kernel-doc formatted comments.  (This one does not.)


> +
> +/* superio_get() checks whether the expected SuperIO device is
> +   present at a specific cmd-addr.  Use in loop to scan.
> +*/

For functions that are not static (and when you want this merged,
not just RFC), please include kernel-doc comments describing the
function and its parameters.  See Documentation/kernel-doc-nano-HOWTO.txt
for more info.

> +struct superio* superio_get(u16 cmd_addr, u8 dev_id_addr,
> +			    u8 want_devid)
> +{
> +	int slot, rc, mydevid;
> +
> +	mutex_lock(&reservation_lock);
> +
> +	/* share any already allocated lock for this cmd_addr, device-id */
> +	for (slot = 0; slot < max_locks; slot++) {
> +		if (sio_locks[slot].users 
> +		    && cmd_addr == sio_locks[slot].sioaddr
> +		    && want_devid == sio_locks[slot].devid) {
> +
> +			if (sio_locks[slot].users == 255) {
> +				dprintk("too many drivers sharing port %x\n", cmd_addr);
> +				mutex_unlock(&reservation_lock);
> +				return 0;
> +			}
> +			sio_locks[slot].users++;
> +			dprintk("sharing port:%x dev:%x users:%d\n",
> +				cmd_addr, want_devid, sio_locks[slot].users);
> +			mutex_unlock(&reservation_lock);
> +			return &sio_locks[slot];
> +		}
> +	}
> +	/* read the device-id-address */
> +	outb(dev_id_addr, cmd_addr);
> +	mydevid = inb(cmd_addr+1);
> +
> +	/* but 1st, check that the cmd register remembers the val just written */
> +	rc = inb(cmd_addr);
> +	if (rc != dev_id_addr) {
> +		dprintk("superio_cmdaddr %x absent %d\n", cmd_addr, rc);
> +		mutex_unlock(&reservation_lock);
> +		return NULL;
> +	}
> +	/* test for the desired device id value */
> +	if (mydevid != want_devid) {
> +		mutex_unlock(&reservation_lock);
> +		return NULL;
> +	}
> +	/* find 1st unused slot */
> +	for (slot = 0; slot < max_locks; slot++)
> +		if (!sio_locks[slot].users)
> +			break;
> +
> +	if (slot >= max_locks) {
> +		printk(KERN_ERR "No superio-locks left. increase max_locks\n");
> +		mutex_unlock(&reservation_lock);
> +		return NULL;
> +	}
> +	dprintk("allocating slot %d, addr %x for device %x\n",
> +		slot, cmd_addr, want_devid);
> +
> +	sio_locks[slot].sioaddr = cmd_addr;
> +	sio_locks[slot].devid = want_devid;
> +	sio_locks[slot].users = 1;
> +	num_locks++;
> +
> +	mutex_unlock(&reservation_lock);
> +	return &sio_locks[slot];
> +}
> +EXPORT_SYMBOL_GPL(superio_get);
> +
> +/* array args must be null terminated */

Also needs kernel-doc function comment header.

> +struct superio* superio_find(u16 cmd_addrs[], u8 devid_addr,
> +			     u8 want_devids[])
> +{
> +	int i, j;
> +	struct superio* gate;
> +
> +	for (i = 0; cmd_addrs[i]; i++) {
> +		for (j = 0; want_devids[j]; j++) {
> +			gate = superio_get(cmd_addrs[i], devid_addr,
> +					   want_devids[j]);
> +			if (gate) {
> +				dprintk("found devid:%x port:%x\n",
> +					want_devids[j], cmd_addrs[i]);
> +				return gate;
> +			} else
> +				dprintk("no devid:%x at port:%x\n",
> +					want_devids[j], cmd_addrs[i]);
> +		}
> +	}
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(superio_find);
> +
> +void superio_release(struct superio* const gate)

Ditto.

> +{
> +	if (gate < &sio_locks[0] || gate >= &sio_locks[max_locks]) {
> +		printk(KERN_ERR
> +		       " superio: attempt to release corrupted superio-lock"
> +		       " %p vs %p\n", gate, &sio_locks);
> +		return;
> +	}
> +	if (!(--gate->users))
> +		dprintk("releasing last user of superio-port %x\n", gate->sioaddr);
> +	return;
> +}
> +EXPORT_SYMBOL_GPL(superio_release);
> +

> diff -ruNp -X dontdiff -X exclude-diffs 6locks-1/include/linux/superio-locks.h 6locks-2/include/linux/superio-locks.h
> --- 6locks-1/include/linux/superio-locks.h	1969-12-31 17:00:00.000000000 -0700
> +++ 6locks-2/include/linux/superio-locks.h	2006-09-13 14:21:08.000000000 -0600
> @@ -0,0 +1,55 @@
> +#include <linux/mutex.h>
> +#include <asm/io.h>
> +
> +/* Super-IO ports are found in low-pin-count hardware (typically ISA,
> +   any others ?).  They usually provide access to many functional
> +   units, so many drivers must share the superio port.  This struct
> +   provides a lock that allows the drivers to coordinate access to that
> +   port.
> +*/

Use long-comment style, please.

> +struct superio {
> +	struct mutex lock;	/* lock shared amongst user drivers */
> +	u16 sioaddr;		/* port's tested cmd-address */
> +	u8 devid;		/* devid found by the registering driver */
> +	u8 users;		/* I cant imagine >256 user drivers */
> +};
> +
> +/* array args must be null terminated */
> +struct superio* superio_find(u16 sioaddrs[], u8 devid_addr, u8 devid_vals[]);
> +struct superio* superio_get(u16 sioaddr, u8 devid_addr, u8 devid_val);
> +void superio_release(struct superio* const gate);
> +
> +/* these locking ops do not address the idling & activation of some
> +   superio devices, which will, once 'locked', ignore accesses until
> +   the 'unlock' sequence is done 1st.  Unfortunately these sequences
> +   vary by device, and in any case don't protect 2 drivers from
> +   stepping on each other's operations.
> +
> +   Callbacks are a possible approach, but every driver using a device
> +   would have to provide them, and only the 1st loaded module would
> +   actually succeed in registering them.  Furthermore, if any driver
> +   accessing a port uses the idle/activate sequences, they all must.
> +   On the whole, this is complexity w/o benefit.
> +*/

long-comment style, please.

---
~Randy

Re: [RFC-patch 0/3] SuperIO locks coordinator

[Samuel Tardieu]

>>>>> "Jim" == Jim Cromie <jim.cromie@gmail.com> writes:

Jim> The module does *not* guarantee anything, it offers no protection
Jim> against rogue (existing) modules which dont use the superio-locks
Jim> coordinator.  (is anti-rogue protection even possible?  Perhaps,
Jim> IFF modules reserve the 2 superio addresses - semi-rogue?)

I don't think we can prevent bad things from happening if someone
doesn't play the game and uses the device using IO ports directly on a
SMP.

  Sam
-- 
Samuel Tardieu -- sam@rfc1149.net -- http://www.rfc1149.net/