Subject: mb800 watchdog driver

Hello guys !

I've wrote small driver for mb800 motherboards (x86, intel). And i want
to share with others.
Any comments/patches are welcome.

You can download source from :
http://hit-six.co.uk/~gj/mb800_watchdog.tar.bz2


--
Grzegorz Jaskiewicz <[email protected]>
K4 Labs


2003-03-13 15:02:58

by Dave Jones

[permalink] [raw]
Subject: Re: mb800 watchdog driver

On Thu, Mar 13, 2003 at 08:11:04AM +0000, Grzegorz Jaskiewicz wrote:
>
> I've wrote small driver for mb800 motherboards (x86, intel). And i want
> to share with others.
> Any comments/patches are welcome.


> #include <sys/syscall.h>

not needed.

> #include <linux/version.h>

not needed.

> #include <linux/errno.h>
> #include <linux/stddef.h>

not needed.

> #include <asm/page.h>
> #include <asm/pgtable.h>

not needed.

> #ifdef CONFIG_DEVFS_FS
> #include <linux/devfs_fs_kernel.h>
> #else
> #error "this driver requires devfs, otherwise it would not work - sorry dude"
> #endif

devfs only drivers == EVIL. Pure EVIL.

> spinlock_t tab_lock;

can be static

> static struct proc_dir_entry *proc_mtd;
> int tab_o_count;

unused

>static struct file_operations tabfs = {
> owner : THIS_MODULE,
> open : open_wdog, /* open */
> write : write_wdog, /* write */
> release : close_wdog, /* release */
> };

use C99 .owner = THIS_MODULE
Ok, this is a 2.4 driver, but it makes forward porting simpler.
Also, the comments are pointless.

> void LockChip()
> void UnLockChip()

can be static (as can most other functions in this file)

>/*
> if (check_region(0x2e, 2)){
> printk(KERN_INFO "tab: my I/O space is allready in use, can't share it .. sorry\n");
> return -1;
> }
>
> request_region(0x2e, 2, "mb800 watchdog");
>*/

1. Probably shouldn't be commented out.
2. Don't use check_region, just check return of request_region.

> printk(KERN_INFO "watchdog: DIE !!!\n");

Something more userfriendly "mb8wdog: unloading\n" would be nicer.

> printk(KERN_INFO "MB800 WATCHDOG: LOAD DEVICE ENDED\n");

KERN_DEBUG ? and STOP SHOUTING!

> printk(KERN_INFO "MB800 WATCHDOG: ENTER CREATE DISPATCH\n");

in fact, these printk's should probably be something like dprintk's
with dprintk being defined at the top of source like..

#define DEBUG
#ifndef DEBUG
# define dprintk(format, args...)
#else
# define dprintk(format, args...) printk(KERN_DEBUG "mb8wdog:", ## args)
#endif

> if (copy_from_user(&b, buf, 1)){
> return -EFAULT;
> }
>
> if (b){
> EnableAndSetWatchdog(b);
> }
> else{
> DisableWatchdog();
> }

Please choose one indentation style, and stick to it.
Preferably the one described in Documenation/CodingStyle

if (b) {
EnableAndSetWatchdog(b);
} else {
DisableWatchdog();
}

Other than these small nits, I don't see any problems with it.
The biggest issue is the devfs-only requirement, which as I mentioned,
is really evil, and afaics, totally unnecessary.

Dave

2003-03-13 15:26:22

by Jörn Engel

[permalink] [raw]
Subject: Re: mb800 watchdog driver

On Thu, 13 March 2003 15:10:33 -0100, Dave Jones wrote:
>
> in fact, these printk's should probably be something like dprintk's
> with dprintk being defined at the top of source like..
>
> #define DEBUG
> #ifndef DEBUG
> # define dprintk(format, args...)
> #else
> # define dprintk(format, args...) printk(KERN_DEBUG "mb8wdog:", ## args)
^^
> #endif

You might want to put an additional whitespace at the indicated
position, that works around a gcc 2.x bug.

J?rn

--
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

2003-03-13 16:08:43

by Christoph Hellwig

[permalink] [raw]
Subject: Re: mb800 watchdog driver

On Thu, Mar 13, 2003 at 03:10:33PM -0100, Dave Jones wrote:
> On Thu, Mar 13, 2003 at 08:11:04AM +0000, Grzegorz Jaskiewicz wrote:
> >
> > I've wrote small driver for mb800 motherboards (x86, intel). And i want
> > to share with others.
> > Any comments/patches are welcome.
>
>
> > #include <sys/syscall.h>
>
> not needed.

Nor does it even compile with any recent kernel..

Subject: Re: mb800 watchdog driver

On Thu, 2003-03-13 at 16:10, Dave Jones wrote:

> devfs only drivers == EVIL. Pure EVIL.
well, since i am using only devfs and i don't see (personaly) any reason
to continue standard /dev directory structure i didn't wrote that part,
but if somebody really want it - patches are welcomed.

I've tried to remove all unneded parts, make it more kernel style of
programming compatible.

new version is on the same place where older one was, as i assume it
does not make sense to keep old one.

http://hit-six.co.uk/~gj/mb800_watchdog.tar.bz2

Cheers.

--
Grzegorz Jaskiewicz <[email protected]>
K4 Labs