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

    > 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 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"
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2005-03-22 13:24    [W:0.028 / U:32.756 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site