Subject: Re: [PATCH][DRIVER][RFC] CPU5 watchdog driver for 2.5
From: Rusty Lynch <rusty@linux.co.intel.com>
To: Heiko Ronsdorf <sk048ro@crimson.ihg.uni-duisburg.de>
Cc: linux-kernel@vger.kernel.org
In-Reply-To: <20030210201732.GA25722@mail.ihg.uni-duisburg.de>
References: <20030210201732.GA25722@mail.ihg.uni-duisburg.de>
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Date: 10 Feb 2003 14:33:27 -0800
Message-Id: <1044916408.4724.11.camel@vmhack>
Mime-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 11600
Lines: 396

On Mon, 2003-02-10 at 12:17, Heiko Ronsdorf wrote:
> Hello linux-kernel,
> 
> this patch is for CPU5 watchdog hardware (kernel 2.5.59)
> 
> see: http://www.sma.de/en/inco/dokuftp/
> 
> I would appreciate if you could find the time to have a
> look at the code and send a feedback.
> 
> Heiko
> 
> ----
> 

Here are some things I noticed from a casual glance:

* Documentation/CodingStyle calls for the opening braces on functions to
be on the next line, like:

int function(int x)
{
        body of function
}

* You could make you driver fit into existing user space deamons by
conforming to Documentation/watchdog-api.txt.  Some things are a little
odd (different then existing wdt drivers) like the way you start and
stop the watchdog.

* I'm pretty sure that in general adding new code to /proc (that has
nothing to do with processes) is frowned on.

    --rustyl

> diff -urN linux-vanilla/drivers/char/watchdog/Kconfig linux-patched/drivers/char/watchdog/Kconfig
> --- linux-vanilla/drivers/char/watchdog/Kconfig	Fri Feb  7 20:45:30 2003
> +++ linux-patched/drivers/char/watchdog/Kconfig	Fri Feb  7 20:52:40 2003
> @@ -316,4 +316,14 @@
>  	tristate "ICP Wafer 5823 Single Board Computer Watchdog"
>  	depends on WATCHDOG
>  
> +config CPU5_WDT
> +	tristate "SMA CPU5 Watchdog"
> +	depends on WATCHDOG
> +	---help---
> +	  TBD.
> +	  This driver is also available as a module ( = code which can be
> +	  inserted in and removed from the running kernel whenever you want).
> +	  The module is called cpu5wdt.o.  If you want to compile it as a
> +	  module, say M here and read <file:Documentation/modules.txt>.
> +
>  endmenu
> diff -urN linux-vanilla/drivers/char/watchdog/Makefile linux-patched/drivers/char/watchdog/Makefile
> --- linux-vanilla/drivers/char/watchdog/Makefile	Fri Feb  7 20:45:30 2003
> +++ linux-patched/drivers/char/watchdog/Makefile	Fri Feb  7 20:52:40 2003
> @@ -29,3 +29,4 @@
>  obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
>  obj-$(CONFIG_SC1200_WDT) += sc1200wdt.o
>  obj-$(CONFIG_WAFER_WDT) += wafer5823wdt.o
> +obj-$(CONFIG_CPU5_WDT) += cpu5wdt.o
> diff -urN linux-vanilla/drivers/char/watchdog/cpu5wdt.c linux-patched/drivers/char/watchdog/cpu5wdt.c
> --- linux-vanilla/drivers/char/watchdog/cpu5wdt.c	Thu Jan  1 01:00:00 1970
> +++ linux-patched/drivers/char/watchdog/cpu5wdt.c	Mon Feb 10 20:38:14 2003
> @@ -0,0 +1,312 @@
> +/*
> + * sma cpu5 watchdog driver
> + *
> + * Copyright (C) 2003 Heiko Ronsdorf <hero@ihg.uni-duisburg.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + *
> + */
> +
> +#include <linux/config.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/miscdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/proc_fs.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/timer.h>
> +#include <asm/io.h>
> +#include <asm/uaccess.h>
> +
> +#include <linux/watchdog.h>
> +
> +/* adjustable parameters */
> +
> +static int verbose = 0;
> +static int port = 0x91;
> +static volatile int ticks = 10000;
> +
> +#define PFX			"cpu5wdt: "
> +
> +#define CPU5WDT_EXTENT          0x0A
> +
> +#define CPU5WDT_STATUS_REG      0x00
> +#define CPU5WDT_TIME_A_REG      0x02
> +#define CPU5WDT_TIME_B_REG      0x03
> +#define CPU5WDT_MODE_REG        0x04
> +#define CPU5WDT_TRIGGER_REG     0x07
> +#define CPU5WDT_ENABLE_REG      0x08
> +#define CPU5WDT_RESET_REG       0x09
> +
> +#define CPU5WDT_INTERVAL	(HZ/10+1)
> +
> +/* some device data */
> +
> +static struct {
> +	struct semaphore stop;
> +	volatile int running;
> +	struct timer_list timer;
> +	volatile int queue;
> +	int default_ticks;
> +	int min_ticks;
> +	unsigned long inuse;
> +} cpu5wdt_device;
> +
> +/* generic helper functions */
> +
> +static void cpu5wdt_trigger(unsigned long unused) {
> +
> +	if ( verbose > 2 )
> +		printk(KERN_DEBUG PFX "trigger at %i ticks\n", ticks);
> +
> +	if( cpu5wdt_device.running )
> +		ticks--;
> +
> +	/* keep watchdog alive */
> +	outb(1, port + CPU5WDT_TRIGGER_REG);
> +
> +	/* requeue?? */
> +	if( cpu5wdt_device.queue && ticks ) {
> +		cpu5wdt_device.timer.expires = jiffies + CPU5WDT_INTERVAL;
> +		add_timer(&cpu5wdt_device.timer);
> +	}
> +	else {
> +		/* ticks doesn't matter anyway */
> +		up(&cpu5wdt_device.stop);
> +	}
> +
> +}
> +
> +static void cpu5wdt_reset(void) {
> +
> +	if ( ticks < cpu5wdt_device.min_ticks )
> +		cpu5wdt_device.min_ticks = ticks;
> +
> +	ticks = cpu5wdt_device.default_ticks;
> +
> +	if ( verbose )
> +		printk(KERN_DEBUG PFX "reset (%i ticks)\n", (int) ticks);
> +
> +}
> +
> +#ifdef CONFIG_PROC_FS
> +static int cpu5wdt_read_proc(char *buf, char **start, off_t offset, int len) {
> +	len = sprintf(buf,      "activation:       %i\n", cpu5wdt_device.queue);
> +	len += sprintf(buf+len, "status:           %i\n", cpu5wdt_device.running);
> +	len += sprintf(buf+len, "current ticks: %i\n", ticks);
> +	len += sprintf(buf+len, "min ticks:     %i\n", cpu5wdt_device.min_ticks);
> +	return len;
> +}
> +
> +static inline void cpu5wdt_register_proc(void) {
> +	create_proc_info_entry("driver/cpu5wdt", 0, NULL, cpu5wdt_read_proc);
> +}
> +
> +static inline void cpu5wdt_unregister_proc(void) {
> +	remove_proc_entry("driver/cpu5wdt", NULL);
> +}
> +#else
> +static inline void cpu5wdt_register_proc(void) {}
> +static inline void cpu5wdt_unregister_proc(void) {}
> +#endif
> +
> +/* filesystem operations */
> +
> +static int cpu5wdt_open(struct inode *inode, struct file *file) {
> +
> +	switch(minor(inode->i_rdev)) {
> +		case WATCHDOG_MINOR:
> +			if ( test_and_set_bit(0, &cpu5wdt_device.inuse) )
> +				return -EBUSY;
> +			break;
> +		default:
> +			return -ENODEV;
> +	}
> +	return 0;
> +
> +}
> +
> +static int cpu5wdt_release(struct inode *inode, struct file *file) {
> +
> +	if(minor(inode->i_rdev)==WATCHDOG_MINOR) {
> +		clear_bit(0, &cpu5wdt_device.inuse);
> +	}
> +	return 0;
> +}
> +
> +static int cpu5wdt_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg) {
> +	unsigned int value;
> +  
> +	switch(cmd) {
> +		case WDIOC_KEEPALIVE:
> +			cpu5wdt_reset();
> +			break;
> +		case WDIOC_START:
> +			if ( !cpu5wdt_device.queue ) {
> +				cpu5wdt_device.queue = 1;
> +				outb(0, port + CPU5WDT_TIME_A_REG);  
> +				outb(0, port + CPU5WDT_TIME_B_REG);  
> +				outb(1, port + CPU5WDT_MODE_REG);
> +				outb(0, port + CPU5WDT_RESET_REG);
> +				outb(0, port + CPU5WDT_ENABLE_REG);
> +				cpu5wdt_device.timer.expires = jiffies + CPU5WDT_INTERVAL;
> +				add_timer(&cpu5wdt_device.timer);
> +			}
> +			/* if process dies, counter is not decremented */
> +			cpu5wdt_device.running++;
> +			break;
> +		case WDIOC_GETSTATUS:    
> +			value = inb(port + CPU5WDT_STATUS_REG); 
> +			value = (value >> 2) & 1;
> +			if ( copy_to_user((int *)arg, (int *)&value, sizeof(int)) )
> +				return -EFAULT;
> +			break;
> +		case WDIOC_STOP:
> +			if ( cpu5wdt_device.running )
> +				cpu5wdt_device.running = 0;
> +
> +			ticks = cpu5wdt_device.default_ticks;
> +
> +			if ( verbose )
> +				printk(KERN_CRIT PFX "stop not possible\n");
> +			return -EIO;
> +		default:
> +    			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t cpu5wdt_write(struct file *file, const char *buf, size_t count, loff_t *ppos) {
> +
> +	if ( !count )
> +		return -EIO;
> +	
> +	cpu5wdt_reset();
> +	return count;
> +
> +}
> +
> +static struct file_operations cpu5wdt_fops = {
> +	.owner		= THIS_MODULE,
> +	.ioctl		= cpu5wdt_ioctl,
> +	.open		= cpu5wdt_open,
> +	.write		= cpu5wdt_write,
> +	.release	= cpu5wdt_release,
> +};
> +
> +static struct miscdevice cpu5wdt_misc = {
> +	.minor	= WATCHDOG_MINOR,
> +	.name	= "watchdog",
> +	.fops	= &cpu5wdt_fops
> +};
> +
> +/* init/exit function */
> +
> +static int __devinit cpu5wdt_init(void) {
> +	unsigned int val;
> +	int err;
> +
> +	if ( verbose )
> +		printk(KERN_DEBUG PFX "port=0x%x, verbose=%i\n", port, verbose);
> +
> +	if ( (err = misc_register(&cpu5wdt_misc)) < 0 ) {
> +		printk(KERN_ERR PFX "misc_register failed\n");
> +		goto no_misc;
> +	}
> +
> +	if ( !request_region(port, CPU5WDT_EXTENT, PFX) ) {
> +		printk(KERN_ERR PFX "request_region failed\n");
> +		err = -EBUSY;
> +		goto no_port;
> +	}
> +
> +	/* watchdog reboot? */
> +	val = inb(port + CPU5WDT_STATUS_REG); 
> +	val = (val >> 2) & 1;
> +	if ( !val )
> +		printk(KERN_INFO PFX "sorry, was my fault\n");
> +
> +	init_MUTEX_LOCKED(&cpu5wdt_device.stop);
> +	cpu5wdt_device.queue = 0;
> +	cpu5wdt_device.min_ticks = ticks;
> +
> +	clear_bit(0, &cpu5wdt_device.inuse);
> +
> +	cpu5wdt_register_proc();
> +
> +	init_timer(&cpu5wdt_device.timer);
> +	cpu5wdt_device.timer.function = cpu5wdt_trigger;
> +	cpu5wdt_device.timer.data = 0;
> +
> +	cpu5wdt_device.default_ticks = ticks;
> +
> +	printk(KERN_INFO PFX "init success\n");
> +
> +	return 0;
> +
> +no_port:
> +	misc_deregister(&cpu5wdt_misc);
> +no_misc:
> +	return err;
> +}
> +
> +static int __devinit cpu5wdt_init_module(void) {
> +
> +	return cpu5wdt_init();
> +}
> +
> +static void __devexit cpu5wdt_exit(void) {
> +
> +	if ( cpu5wdt_device.queue ) {
> +		cpu5wdt_device.queue = 0;
> +		down(&cpu5wdt_device.stop);
> +	}
> +
> +	cpu5wdt_unregister_proc();
> +
> +	misc_deregister(&cpu5wdt_misc);
> +
> +	release_region(port, CPU5WDT_EXTENT);
> +
> +}
> +
> +static void __devexit cpu5wdt_exit_module(void) {
> +
> +	cpu5wdt_exit();
> +}
> +
> +/* module entry points */
> +
> +module_init(cpu5wdt_init_module);
> +module_exit(cpu5wdt_exit_module);
> +
> +MODULE_AUTHOR("Heiko Ronsdorf <hero@ihg.uni-duisburg.de>");
> +MODULE_DESCRIPTION("sma cpu5 watchdog driver");
> +MODULE_SUPPORTED_DEVICE("sma cpu5 watchdog");
> +MODULE_LICENSE("GPL");
> +
> +MODULE_PARM(port, "i");
> +MODULE_PARM_DESC(port, "base address of watchdog card, default is 0x91");
> +
> +MODULE_PARM(verbose, "i");
> +MODULE_PARM_DESC(verbose, "be verbose, default is 0 (no)");
> +
> +MODULE_PARM(ticks, "i");
> +MODULE_PARM_DESC(ticks, "count down ticks, default is 10000");
> +
> +EXPORT_NO_SYMBOLS;
> diff -urN linux-vanilla/include/linux/watchdog.h linux-patched/include/linux/watchdog.h
> --- linux-vanilla/include/linux/watchdog.h	Fri Jan 10 22:08:18 2003
> +++ linux-patched/include/linux/watchdog.h	Fri Feb  7 20:52:40 2003
> @@ -27,6 +27,8 @@
>  #define	WDIOC_KEEPALIVE		_IOR(WATCHDOG_IOCTL_BASE, 5, int)
>  #define	WDIOC_SETTIMEOUT        _IOWR(WATCHDOG_IOCTL_BASE, 6, int)
>  #define	WDIOC_GETTIMEOUT        _IOR(WATCHDOG_IOCTL_BASE, 7, int)
> +#define	WDIOC_START             _IO(WATCHDOG_IOCTL_BASE, 8)
> +#define	WDIOC_STOP              _IO(WATCHDOG_IOCTL_BASE, 9)
>  
>  #define	WDIOF_UNKNOWN		-1	/* Unknown flag error */
>  #define	WDIOS_UNKNOWN		-1	/* Unknown status error */


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/