On Sat, Apr 05, 2014 at 08:29:40PM +0200, Anatolij Gustschin wrote: > Convert the imx2_wdt driver to the new watchdog core api. > > Signed-off-by: Anatolij Gustschin <agust@xxxxxxx> > Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx> > Cc: Wim Van Sebroeck <wim@xxxxxxxxx> Looks pretty good, just a couple of nitpicks. > --- > NOTE: This patch applies on top of the imx2_wdt regmap-mmio support > patch series http://comments.gmane.org/gmane.linux.kernel/1677735 > > drivers/watchdog/Kconfig | 1 + > drivers/watchdog/imx2_wdt.c | 292 ++++++++++++++++++------------------------- > 2 files changed, 124 insertions(+), 169 deletions(-) > [ ... ] > -static void imx2_wdt_timer_ping(unsigned long arg) > -{ > - /* ping it every imx2_wdt.timeout / 2 seconds to prevent reboot */ > - imx2_wdt_ping(); > - mod_timer(&imx2_wdt.timer, jiffies + imx2_wdt.timeout * HZ / 2); > + regmap_read(wdev->regmap, IMX2_WDT_WCR, &val); > + > + if (val & IMX2_WDT_WCR_WDE) > + return true; > + return false; > } You can simplyfy this to return val & IMX2_WDT_WCR_WDE; since C auto-converts from int to bool. If you feel fancy and don't trust the C compiler, another option would be return !!(val & IMX2_WDT_WCR_WDE); which would at least drop the if statement. [ ... ] > + > + wdog->timeout = clamp_t(unsigned, timeout, 1, IMX2_WDT_MAX_TIME); > + if (wdog->timeout != timeout) > dev_warn(&pdev->dev, "Initial timeout out of range! " > - "Clamped from %u to %u\n", timeout, imx2_wdt.timeout); > + "Clamped from %u to %u\n", timeout, wdog->timeout); Somewhat unrelated, but this results in a checkpatch warning. I would suggest to put the string in a single line. dev_warn(&pdev->dev, "Initial timeout out of range! Clamped from %u to %u\n", timeout, wdog->timeout); Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
