[lkml]   [2002]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [DRIVER][RFC] SC1200 Watchdog driver
    Jeff Garzik wrote:
    > 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.

    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

    /Christer (off to lunch)

    Blatant plug: I'm a freelance consultant looking for interesting work.[unhandled content-type:application/octet-stream]
     \ /
      Last update: 2005-03-22 13:24    [W:0.026 / U:16.676 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site