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.
This archive was generated by hypermail 2b29 : Sat Feb 23 2002 - 21:00:32 EST