Re: [PATCH] 2.6.0-test4 - Watchdog patches

[Justin Cormack]

On Sun, 2003-08-31 at 21:52, Wim Van Sebroeck wrote: 
 = 0;
>  
>  /*
>   *	You must set these - there is no sane way to probe for this board.
> @@ -56,10 +56,17 @@
>   *      to restart it again.
>   */
>  
> -#define WDT_START 0x443
> -#define WDT_STOP 0x843
> -
> -static int wd_margin = WD_TIMO;
> +static int wdt_stop = 0x843;
> +module_param(wdt_stop, int, 0);
> +MODULE_PARM_DESC(wdt_stop, "Wafer 5823 WDT 'stop' io port (default 0x843)");
> +
> +static int wdt_start = 0x443;
> +module_param(wdt_start, int, 0);
> +MODULE_PARM_DESC(wdt_start, "Wafer 5823 WDT 'start' io port (default 0x443)");

There is *no point* making these module parameters. There is no other
watchdog that works in exactly the same way but using different io
ports. If there was this still wouldnt be the sensible way to do it. 

> +	if (timeout < 1 || timeout > 63) {
> +		timeout = WD_TIMO;
> +		printk (KERN_INFO PFX "timeout value must be 1<=x<=255, using %d\n",
> +			timeout);

where did that 63 come from? should be 255.

Also, generally, please cc authors when you send patches. And making the comments the
same in the watchdog drivers is a complete  waste of time. If you want to reduce
the amount of duplicated code in the drivers you could make a watchdog_ops interface
for almost all of them.

Justin


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/