Re: [DRIVER][RFC] SC1200 Watchdog driver

[Jeff Garzik <jgarzik@mandrakesoft.com>]

Christer Weinigel wrote:
> static int margin = 60;         /* in seconds */
> MODULE_PARM(margin, "i");
> 
> static int nowayout = CONFIG_WATCHDOG_NOWAYOUT;
> MODULE_PARM(nowayout, "i");

MODULE_PARM_DESC would be nice

> static void scx200_watchdog_update_margin(void)
> {
>         printk(KERN_INFO "%s: timer margin %d seconds\n", name, margin);
>         wdto_restart = 32768 / 1024 * margin;
>         scx200_watchdog_ping();
> }

if you can turn multiplication and division of powers-of-2 into left and
right shifts, other simplications sometimes follow.  Certainly you want
to avoid division especially and multiplication also if possible.

> static int scx200_watchdog_open(struct inode *inode, struct file *file)
> {
>         /* only allow one at a time */
>         if (down_trylock(&open_sem))
>                 return -EBUSY;
>         scx200_watchdog_enable();
>         expect_close = 0;
> 
>         return 0;
> }

now, a policy question -- do you want to fail or simply put to sleep
multiple openers?  if you want to fail, this should be ok I think.  if
you want to sleep, you can look at sound/oss/* in 2.5.x or
drivers/sound/* in 2.4.x for some examples of semaphore use on open(2).

> static int scx200_watchdog_release(struct inode *inode, struct file *file)
> {
>         if (!expect_close) {
>                 printk(KERN_WARNING "%s: watchdog device closed unexpectedly, "
>                        "will not disable the watchdog timer\n", name);

I wonder why 'name' is not simply a macro defining a string constant? 
Oh yeah, it matters very little.  You might want to make 'name' const,
though.

> static struct notifier_block scx200_watchdog_notifier =
> {
>         scx200_watchdog_notify_sys,
>         NULL,
>         0
> };

use name:value style of struct initialization, and omit any struct
members which are 0/NULL (that's implicit).

> static int __init scx200_watchdog_init(void)
> {
>         int r;

Here's a big one, I still don't like this lack of probing in the
driver.  Sure we have "probed elsewhere", but IMO each driver like this
one needs to check -something- to ensure that SC1200 hardware is
present.  Otherwise, a random user from a distro-that-builds-all-drivers
might "modprobe sc1200_watchdog" and things go boom.

If the upper levels have set up resources correctly, for example, then
there should be a resource for the watchdog that is -not- marked busy,
which the watchdog subsequently calls request_region on.  Or, the
resource is already marked busy (this is present state, as you
indicated) and you peek at the resource strings for the one you want.

Tiny drivers for hardware embedded on some larger device tends to need
little hacks like that to check for the presence of hardware... but you
need that sanity check somewhere in each driver.

	Jeff



-- 
Jeff Garzik      | "Why is it that attractive girls like you
Building 1024    |  always seem to have a boyfriend?"
MandrakeSoft     | "Because I'm a nympho that owns a brewery?"
                 |             - BBC TV show "Coupling"