Messages in this thread | | | From | Christer Weinigel <> | Subject | Re: [DRIVER][RFC] SC1200 Watchdog driver | Date | Thu, 21 Feb 2002 13:57:43 +0100 (CET) |
| |
Jeff Garzik wrote: > MODULE_PARM_DESC would be nice
Done.
> > 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.
Since this is only called on initialization I'm not overly concerned with performance here, I prefer code clarity. This ought to be optimized by gcc anyways.
> 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).
I'm not even sure if single-open sematics are neccesary at all, but I copied most of the interface from wdt285.c so I copied this too. The watchdog API seems to be a rather ad hoc thing. For example I just noticed that the WDIOC_SETTIMEOUT call probably takes a parameter which seems to be minutes, not seconds. "Someone (tm)" ought to write a more formal API specification.
> 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.
Because "%s: " is less text than "scx200_watchdog" and I'm not sure if gcc is able to merge duplicate strings. Not much of a difference.
> > 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).
Done. I also changed the notifier codes that cause the watchdog to shut down to something that seems more useful.
> > 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.
You're right, I just assumed that nobody would load this driver unless they are on a SCx200 system. Done. I'll update all the other drivers too.
/Christer (off to lunch)
-- Blatant plug: I'm a freelance consultant looking for interesting work.[unhandled content-type:application/octet-stream] | |