Re: [RFC PATCH] watchdog: ep93xx: Use the WatchDog Timer Driver Core.

[Wim Van Sebroeck <wim@iguana.be>]

Hi All,

> >> @@ -24,11 +24,9 @@
> >>   */
> >>  
> >>  #include <linux/module.h>
> >> -#include <linux/fs.h>
> >>  #include <linux/miscdevice.h>
> >
> > Is the above header still needed?
> 
> I think so due to the MODULE_ALIAS_MISCDEV() at the end of the file.  But,
> I'm not sure if that is really needed... Wim?

Yes it's till needed. Both MODULE_ALIAS_MISCDEV() and WATCHDOG_MINOR are defined in miscdevice.h .

> >> @@ -210,43 +126,31 @@ static int __init ep93xx_wdt_init(void)
> >>  {
> >>  	int err;
> >>  
> >> -	err = misc_register(&ep93xx_wdt_miscdev);
> >> +	ep93xx_wdd.bootstatus = readl(EP93XX_WDT_WATCHDOG);
> >> +	ep93xx_wdd.timeout = timeout;
> >
> > Should you check that the given timeout is in valid range here, like it is
> > done in the original driver?
> 
> I thought the core would handle that.  If not maybe it should validate the
> timeout based on the min/max values.  Wim?

The core doesn't handle that.
What happens is: We set the parameter normally the same as the default value.
The user can change this when loading the module by another value.
We then normally need to check if this value makes any sense for the hardware.
If not we reset it back to the default value.

The core is however not aware of the default value.
Several solutions are possible:
1) we add a timeout_default value into the struct (waste of space imho)
2) we set it to timeout_max if out of range
3) we set it to timeout_min if less then timeout_min or timeout_max if bigger then timeout_max
4) we can add an operation to handle this so that the driver can decide

For now I would leave the check in. In parallel we could indeed adopt a common behaviour and add it to the core.

Kind regards,
Wim.

PS: Congratulations with your first child!