Messages in this thread | | | Date | Thu, 21 Feb 2002 08:20:16 -0500 | From | Jeff Garzik <> | Subject | Re: [DRIVER][RFC] SC1200 Watchdog driver |
| |
Christer Weinigel wrote: > Jeff Garzik wrote: > > > 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.
I mention it because we ran into a case with the ppc md where gcc did not... I doubt this code would be used on PPC :) but I mention it mainly as a matter of principle
> > 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.
;-) hey, if you took 30 minutes to jot down into a text file your observations on the implementation of the API, I'm sure we could get that into 2.4 and 2.5 ...
> > 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.
Note that every place where you aren't sure, you are using string catenation anyway with the KERN_xxx symbols:
printk (KERN_ERR "...
> 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.
Thanks!
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" - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |