[PATCH] char: Nano 7240 watchdog driver

[Dave Jones]

On Sat, Oct 06, 2007 at 01:10:23PM +1000, Gilles GIGAN wrote:
 > Hi everyone,
 > I thought I d submit this patch here first, to see if I m doing the right
 > thing. I do also have a couple of questions:
 > Below is the email I d like to send to the LKML for inclusion in the kernel.
 > - Is is appropriately formatted ? (I tried following the guidelines in
 > SubmittingPatches as closely as possible)

Unfortunatly gmail mangled it. It's word-wrapped and white-space damaged
so will never apply.  Maybe there's some options in the gmail UI you can
change to make this not happen.  Test it by sending the patch to
yourself first, and see if you can get it to apply with patch(1).

 > - is the patch correctly generated ? Are the changes to Kconfig and Makefile
 > to be included in the patch (like I did) ?
 > - Is the email title OK ? (not really sure what subsystem I should use:
 > "char" , "watchdog", "device drivers", ... ?)

'watchdog' sounds sane. Remember to Cc the watchdog maintainer too
Wim Van Sebroeck <wim at iguana.be>  (He may have more specific comments).

Some minor nits..

 > +config SBC7240_WDT
 > + tristate "SBC Nano 7240 Watchdog Timer"
 > + depends on X86

Unless your SBC is 64bit, changing this to X86_32 might make sense,
to stop it showing up for everyone building 64bit kernels.

 > +static unsigned long wdt_status = 0;

You don't need to initialise static vars to 0. They'll go into .bss,
and will be auto-zeroed.

 > + // is there a magic char ?

Use /* */ rather than c++ style comments.

 > + printk(KERN_INFO PREFIX "Removing watchog\n");

Typo.

 > + //The  IO port 0x043 is already claimed by the system timer

C++ comments again.
Also, how does the wdt_disable() work if 0x43 is the system timer?
We're going to poke that rather than the watchdog won't we?

That explains why all this is disabled...

 > +//      release_region(DISABLE_SBC7240_PORT,1);

 ...

 > +/* if (!request_region(DISABLE_SBC7240_PORT, 1, "SBC7240 WDT"))
 > + {
 > + printk(KERN_ERR "I/O address 0x%04x already in use\n",
 > + DISABLE_SBC7240_PORT);
 > + rc = -EIO;
 > + goto err_out_region1;
 > + }
 > +*/

It may be better to just omit this, and leave the comment explaining
why the reservation isn't needed.   Though as I mentioned above,
it sounds odd that we have two things both wanting to be at 0x43.


On the whole, looks pretty clean.   Running it through scripts/checkpatch.pl
may highlight some other minor things.

	Dave

-- 
http://www.codemonkey.org.uk