lkml.org 
[lkml]   [2008]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] watchdog: Add support for the WM8350 watchdog
Hi Marc,

Finally a stable ADSL line again. I just finished reviewing your driver:

> +static int wm8350_wdt_open(struct inode *inode, struct file *file)
> +{
> + struct wm8350 *wm8350 = get_wm8350();
> + int ret;
> +
> + if (!wm8350) {
> + printk(KERN_CRIT "FNORD\n");

:-) as you pointed out this should be removed.

> +static int wm8350_wdt_release(struct inode *inode, struct file *file)
> +{
> + struct wm8350 *wm8350 = get_wm8350();
> +
> + if (wm8350_wdt_expect_close)
> + wm8350_wdt_stop(wm8350);
> + else
> + dev_warn(wm8350->dev, "Watchdog device closed uncleanly\n");

you should ping the watchdog if you don't stop it.

> +static int wm8350_wdt_ioctl(struct inode *inode, struct file *file,
> + unsigned int cmd, unsigned long arg)

we use the unlocked_ioctl call in watchdog drivers now. So this should be:
static long wm8350_wdt_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)

> +{
> + struct wm8350 *wm8350 = get_wm8350();
> + int ret = -ENOTTY, time, i;
> + void __user *argp = (void __user *)arg;
> + int __user *p = argp;
> + u16 reg;
> +
> + switch (cmd) {
> + case WDIOC_GETSUPPORT:
> + ret = copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
> + break;
> +
> + case WDIOC_GETSTATUS:

Bootstatus is a mandatory ioctl according to the API. So just add:
case WDIOC_GETBOOTSTATUS:

> +static const struct file_operations wm8350_wdt_fops = {
> + .owner = THIS_MODULE,
> + .llseek = no_llseek,
> + .write = wm8350_wdt_write,
> + .ioctl = wm8350_wdt_ioctl,

and for the unlocked_ioctl call this needs to be:
.unlocked_ioctl = wm8350_wdt_ioctl,

For the rest: seems OK to me.

Kind regards,
Wim.



\
 
 \ /
  Last update: 2008-10-24 14:39    [W:0.044 / U:0.756 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site