Message-ID: <20110503105735.GB2497@pengutronix.de>
Date:	Tue, 3 May 2011 12:57:35 +0200
From:	Wolfram Sang <w.sang@...gutronix.de>
To:	Jimmy Chen (陳永達) <jimmy.chen@...a.com>
Cc:	Alan Cox <alan@...rguk.ukuu.org.uk>, linux-kernel@...r.kernel.org,
	linux-watchdog@...r.kernel.org, wim@...ana.be
Subject: Re: [PATCH 2/2] watchdog: add support for MOXA V2100 watchdog
 driver

Hi,

On Tue, May 03, 2011 at 06:25:50PM +0800, Jimmy Chen (陳永達) wrote:
> From: Jimmy Chen <jimmy.chen@...a.com>
> 
> -Add real function for watchdog driver
> -Follow advices from Alan Cox
> 
> Signed-off-by: Jimmy Chen <jimmy.chen@...a.com>
> ---
> diff --git a/drivers/watchdog/moxa_wdt.c b/drivers/watchdog/moxa_wdt.c
> new file mode 100644
> index 0000000..bcd8164
> --- /dev/null
> +++ b/drivers/watchdog/moxa_wdt.c
> @@ -0,0 +1,385 @@
> +/*
> + * serial driver for the MOXA V2100 platform.
> + *
> + * Copyright (c) MOXA Inc.  All rights reserved.
> + *	Jimmy Chen <jimmy.chen@...a.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * History:
> + * Date		Author			Comment
> + * 04-29-2011	Jimmy Chen.		copy wdt.c code

Not needed, we have the git-repo-history.

> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME":"fmt
> +
> +#include <linux/interrupt.h>
> +#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 <linux/io.h>
> +#include <linux/uaccess.h>
> +
> +#include <asm/system.h>
> +
> +#include "moxa_wdt.h"
> +
> +#define MOXA_WDT_VERSION		"v0.1.0"

Not needed, we have the git-repo-history.

> +#define HW_VENDOR_ID_H			0x87
> +#define HW_VENDOR_ID_L			0x83
> +
> +
> +static unsigned long wdt_is_open;
> +static char expect_close;
> +
> +static int timeout = DEFAULT_WATCHDOG_TIMEOUT;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout,
> +	"Watchdog timeout in seconds. 1<= timeout <=63, 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)");
> +
> +static int debug;
> +module_param(debug, int, 0);
> +MODULE_PARM_DESC(debug, "print the debug message in this driver");

Can go, there is a dynamic_debug Kconfig option.

> +
> +static spinlock_t wdt_lock = SPIN_LOCK_UNLOCKED;
> +
> +/**
> + *	wdt_start:
> + *
> + *	Start the watchdog driver.
> + */
> +
> +static int moxawdt_start(void)
> +{
> +	unsigned long flags;
> +	unsigned char val;
> +
> +	if (debug)
> +		pr_debug("wdt_start: timeout=%d\n", timeout);
> +
> +	spin_lock_irqsave(&wdt_lock, flags);
> +	superio_init();
> +	superio_select_dev(7);
> +	val = superio_get_reg(0x72) | 0x10;

How about register names instead of magic values?

> +	superio_set_reg(val, 0x72);
> +	superio_set_reg((timeout/1000), 0x73);

Spaces around operators. Please use checkpatch.pl for further hints.

> +	spin_unlock_irqrestore(&wdt_lock, flags);
> +	return 0;
> +}
> +
> +/**
> + *	wdt_stop:
> + *
> + *	Stop the watchdog driver.
> + */
> +
> +static int wdt_stop(void)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt_lock, flags);
> +
> +	if (debug)
> +		pr_debug("wdt_disable\n");
> +	superio_init();
> +	superio_select_dev(7);	/* logic device 8 */
> +	superio_set_reg(0, 0x73);	/* Reg:F6 counter register */
> +	spin_unlock_irqrestore(&wdt_lock, flags);
> +	return 0;
> +}
> +
> +/**
> + *	wdt_ping:
> + *
> + *	Reload counter one with the watchdog heartbeat. We don't bother
> + *	reloading the cascade counter.
> + */
> +
> +static void wdt_ping(void)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&wdt_lock, flags);
> +
> +	if (debug)
> +		pr_debug("wdt_ping: timeout=%d\n", timeout);
> +	superio_init();
> +	superio_select_dev(7);    /* logic device 7 */
> +	superio_set_reg((timeout/1000), 0x73);    /* Reg:F6,30 sec */
> +	spin_unlock_irqrestore(&wdt_lock, flags);
> +}
> +
> +/**
> + *	wdt_verify_vendor:
> + *	return true if vendor ID match
> + */
> +
> +static int wdt_verify_vendor(void)
> +{
> +	unsigned char chip_id_h; /* chip id high byte */
> +	unsigned char chip_id_l; /* chip id low byte */

Comment can go IMHO (obvious).

> +
> +	superio_init();
> +	superio_select_dev(7);
> +	chip_id_h = superio_get_reg(0x20);
> +	chip_id_l = superio_get_reg(0x21);
> +	if ((chip_id_h == HW_VENDOR_ID_H) && (chip_id_l == HW_VENDOR_ID_L))
> +		return 0;
> +
> +	return -1;

-EINVAL?

> +}
> +
> +/**
> + *	wdt_set_timeout:
> + *	@t:		the new heartbeat value that needs to be set.
> + *
> + *	Set a new heartbeat value for the watchdog device. If the heartbeat
> + *	value is incorrect we keep the old value and return -EINVAL. If
> + *	successful we return 0.
> + */
> +
> +static int wdt_set_timeout(int *t)
> +{
> +	if (*t < WATCHDOG_MIN_TIMEOUT || *t > WATCHDOG_MAX_TIMEOUT) {
> +		*t = DEFAULT_WATCHDOG_TIMEOUT;
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int moxawdt_open(struct inode *inode, struct file *file)
> +{
> +
> +	if (test_and_set_bit(0, &wdt_is_open))
> +		return -EBUSY;
> +	/*
> +	 *	Activate
> +	 */

Comment can go (obvious).

> +	if (debug)
> +		pr_debug("moxawdt_open entry\n");
> +	moxawdt_start();
> +	return nonseekable_open(inode, file);
> +
> +	return 0;
> +}
> +
> +static int moxawdt_release(struct inode *inode, struct file *file)
> +{
> +	if (debug)
> +		pr_debug("moxawdt_release entry\n");
> +
> +	if (expect_close == 42) {
> +		wdt_stop();
> +		clear_bit(0, &wdt_is_open);
> +	} else {
> +		pr_crit("wdt: WDT device closed unexpectedly.  WDT will not stop!\n");
> +		wdt_ping();
> +	}
> +	expect_close = 0;
> +
> +	return 0;
> +}
> +
> +static long moxawdt_ioctl(struct file *file,
> +		unsigned int cmd, unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	int __user *p = argp;
> +	int new_timeout;
> +	int status;
> +
> +	static struct watchdog_info ident = {
> +		.options =		WDIOF_SETTIMEOUT|
> +					WDIOF_MAGICCLOSE|
> +					WDIOF_KEEPALIVEPING,
> +		.firmware_version =	1,
> +		.identity =		"MOXA2100WDT ",
> +	};
> +
> +	switch (cmd) {
> +	case WDIOC_GETSUPPORT:
> +		return copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
> +	case WDIOC_GETSTATUS:
> +		status = 1;
> +		return put_user(status, p);
> +	case WDIOC_GETBOOTSTATUS:
> +		return put_user(0, p);
> +	case WDIOC_KEEPALIVE:
> +		wdt_ping();
> +		return 0;
> +	case WDIOC_SETTIMEOUT:
> +		if (get_user(new_timeout, p))
> +			return -EFAULT;
> +		if (wdt_set_timeout(&new_timeout))
> +			return -EINVAL;
> +		wdt_ping();
> +		/* Fall */
> +	case WDIOC_GETTIMEOUT:
> +		return put_user(timeout, p);
> +	default:
> +		return -ENOTTY;
> +	}
> +	return 0;
> +}
> +
> +/*
> + *      moxawdt_write:
> + *      @file: file handle to the watchdog
> + *      @buf: buffer to write (unused as data does not matter here
> + *      @count: count of bytes
> + *      @ppos: pointer to the position to write. No seeks allowed
> + *
> + *      A write to a watchdog device is defined as a keepalive signal. Any
> + *      write of data will do, as we we don't define content meaning.
> + */
> +
> +static ssize_t moxawdt_write(struct file *file, const char *buf, \
> +			    size_t count, loff_t *ppos)
> +{
> +	if (count) {
> +		if (!nowayout) {
> +			size_t i;
> +
> +			/* In case it was set long ago */
> +			for (i = 0; i != count; i++) {
> +				char c;
> +				if (get_user(c, buf + i))
> +					return -EFAULT;
> +				if (c == 'V')
> +					expect_close = 42;
> +			}
> +		}
> +
> +	}
> +	return count;
> +}
> +
> +/**
> + *	notify_sys:
> + *	@this: our notifier block
> + *	@code: the event being reported
> + *	@unused: unused
> + *
> + *	Our notifier is called on system shutdowns. We want to turn the card
> + *	off at reboot otherwise the machine will reboot again during memory
> + *	test or worse yet during the following fsck. This would suck, in fact
> + *	trust me - if it happens it does suck.
> + */
> +
> +static int wdt_notify_sys(struct notifier_block *this, unsigned long code,
> +	void *unused)
> +{
> +	if (code == SYS_DOWN || code == SYS_HALT)
> +		wdt_stop();
> +	return NOTIFY_DONE;
> +}
> +
> +/*
> + *	The WDT card 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 const struct file_operations moxawdt_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= no_llseek,
> +	.open		= moxawdt_open,
> +	.write		= moxawdt_write,
> +	.unlocked_ioctl	= moxawdt_ioctl,
> +	.release	= moxawdt_release,
> +};
> +
> +static struct miscdevice wdt_miscdev = {
> +	.minor = WATCHDOG_MINOR,
> +	.name = "watchdog",
> +	.fops = &moxawdt_fops,
> +};
> +
> +static int __init moxawdt_init(void)
> +{
> +	int ret;
> +
> +	/* check timeout valid */

Comment can go (obvious).

> +	if (wdt_set_timeout(&timeout)) {
> +		pr_err("timeout value must be "
> +				"%lu < timeout < %lu, using %d\n",

String should fit into one line.

> +				WATCHDOG_MIN_TIMEOUT, WATCHDOG_MAX_TIMEOUT,
> +				timeout);
> +	}
> +
> +	if (!request_region(SUPERIO_PORT, 2, "moxawdt")) {
> +		pr_err("moxawdt_init: can't get I/O "
> +				"address 0x%x\n", SUPERIO_PORT);
> +		ret = -EBUSY;
> +		goto reqreg_err;
> +	}
> +
> +	/* we suppose to check magic id in case wrong devices */

Comment can go (obvious).

> +	if (wdt_verify_vendor()) {
> +		pr_err("hw device id not match!!\n");
> +		ret = -ENODEV;
> +		goto reqreg_err;
> +	}
> +
> +	ret = register_reboot_notifier(&wdt_notifier);
> +	if (ret) {
> +		pr_err("can't register reboot notifier\n");
> +		goto regreb_err;
> +	}
> +
> +	ret = misc_register(&wdt_miscdev);
> +	if (ret) {
> +		pr_err("Moxa V2100-LX WatchDog: Register misc fail !\n");
> +		goto regmisc_err;
> +	}
> +
> +	pr_info("Moxa V2100 Watchdog Driver, version %s\n", MOXA_WDT_VERSION);
> +	pr_info("initialized. (nowayout=%d)\n", nowayout);
> +	pr_info("initialized. (timeout=%d)\n", timeout);
> +	pr_info("initialized. (debug=%d)\n", debug);

Too excessive IMHO. Imagine every driver in your system printing that
much.

> +
> +	return 0;
> +
> +regmisc_err:
> +	unregister_reboot_notifier(&wdt_notifier);
> +regreb_err:
> +	release_region(SUPERIO_PORT, 2);
> +reqreg_err:
> +	return ret;
> +}
> +
> +static void __exit moxawdt_exit(void)
> +{
> +	misc_deregister(&wdt_miscdev);
> +	unregister_reboot_notifier(&wdt_notifier);
> +	release_region(SUPERIO_PORT, 2);
> +	superio_release();
> +}
> +
> +module_init(moxawdt_init);

Most people put this right below the referenced function.

> +module_exit(moxawdt_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Jimmy.Chen@...a.com");
> +MODULE_DESCRIPTION("Moxa V2100-LX WDT driver");
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> +
> diff --git a/drivers/watchdog/moxa_wdt.h b/drivers/watchdog/moxa_wdt.h
> new file mode 100644
> index 0000000..3f3ff68
> --- /dev/null
> +++ b/drivers/watchdog/moxa_wdt.h
> @@ -0,0 +1,53 @@
> +/*
> + * serial driver for the MOXA V2100 platform.
> + *
> + * Copyright (c) MOXA Inc.  All rights reserved.
> + *	Jimmy Chen <jimmy.chen@...a.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __X86__MOXAWDT__
> +#define __X86__MOXAWDT__
> +
> +#define	SUPERIO_PORT		((u8)0x2e)
> +
> +#define DEFAULT_WATCHDOG_TIMEOUT	(30UL*1000UL)	/* 30 seconds */
> +#define WATCHDOG_MIN_TIMEOUT	(1UL*1000UL)	/*  2 seconds */
> +#define WATCHDOG_MAX_TIMEOUT	(255UL*1000UL)	/* 255 seconds */
> +
> +unsigned char superio_get_reg(u8 val)
> +{
> +	outb_p(val, SUPERIO_PORT);
> +	val = inb_p(SUPERIO_PORT+1);
> +	return val;
> +}

Hmm, code in a header? Can't this go into the main source? Hmm, at least
it87_wdt.c has something very similar. Maybe it can even be shared? What
about locking BTW?

> +
> +void superio_set_reg(u8 val, u8 index)
> +{
> +	outb_p(index, SUPERIO_PORT);
> +	outb_p(val, (SUPERIO_PORT+1));
> +}
> +
> +void superio_select_dev(u8 val)
> +{
> +	superio_set_reg(val, 0x07);
> +}
> +
> +void superio_init(void)
> +{
> +	outb(0x87, SUPERIO_PORT);
> +	outb(0x01, SUPERIO_PORT);
> +	outb(0x55, SUPERIO_PORT);
> +	outb(0x55, SUPERIO_PORT);
> +}
> +
> +void superio_release(void)
> +{
> +	outb_p(0x02, SUPERIO_PORT);
> +	outb_p(0x02, SUPERIO_PORT+1);
> +}
> +
> +#endif	/* __X86__MOXAWDT__ */

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists