On Monday, January 30, 2017 11:55 AM, Guenter Roeck wrote: > On Mon, Jan 30, 2017 at 09:55:47AM -0700, H Hartley Sweeten wrote: >> Cleanup this driver and remove the 200ms heartbeat timer. The core now >> has the ability to handle the heartbeat. >> >> Signed-off-by: H Hartley Sweeten <hswee...@visionengravers.com> >> Cc: Wim Van Sebroeck <w...@iguana.be> >> Cc: Guenter Roeck <li...@roeck-us.net>
Hi Guenter, I wasn't sure this patch was going to get delivered correctly. I got an "Undeliverable" bounce due to possible spoofing. I am trying to figure out why right now. Anyway... <snip> >> -#define WDT_VERSION "0.4" >> - >> -/* default timeout (secs) */ >> -#define WDT_TIMEOUT 30 >> - > > Personally I like those constants, even if used only once (I know, it is a > good > candidate for bikeshedding). Two reasons: 1) It is already there, and 2) It > helps seeing the default without having to dig into the code. I assume the WDT_VERSION can go away... As far as the WDT_TIMEOUT, I have no problem leaving it. I was just trying to remove some cruft. >> static bool nowayout = WATCHDOG_NOWAYOUT; >> module_param(nowayout, bool, 0); >> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started"); >> >> -static unsigned int timeout = WDT_TIMEOUT; >> -module_param(timeout, uint, 0); >> -MODULE_PARM_DESC(timeout, >> - "Watchdog timeout in seconds. (1<=timeout<=3600, default=" >> - __MODULE_STRING(WDT_TIMEOUT) ")"); >> - > > Are you sure you want to take away the means to set the timeout with > a module parameter ? You could easily retain the module parameter > and call watchdog_init_timeout(wdd, timeout, dev). The parameter should > then be initialized with 0, though, to have the watchdog core take the > timeout from devicetree if provided. Again, I have no problem leaving this. I personally don't use it but someone else might. I'm not sure if the ep93xx will ever get converted to devicetree but I might figure it one eventually. <snip> >> + watchdog_set_drvdata(wdd, priv); >> >> - setup_timer(&timer, ep93xx_wdt_timer_ping, 1); >> + ret = watchdog_register_device(wdd); > > Looks like a perfect candidate for devm_watchdog_register_device(). I just saw your patches that do this. I will update this patch to use it. Thanks, Hartley
