[PATCH v2 2/5] watchdog: pnx4008: add restart handler
From: Joachim Eastwood <hidden>
Date: 2016-02-22 18:03:02
Also in:
linux-watchdog
Hi Sylvain, On 22 February 2016 at 17:55, [off-list ref] wrote:
From: Sylvain Lemieux <redacted>
Add restart handler capability to the driver;
the restart handler implementation was taken from
"mach-lpc32xx" ("lpc23xx_restart" function).I think it would be better if you were to expand on "watchdog: core: add restart handler support" to give you the parameters you need to make the restart hook work for you. But that is up to watchdog maintainers to decide.
quoted hunk ↗ jump to hunk
Signed-off-by: Sylvain Lemieux <redacted> --- Changes from v1 to v2: - new patch in v2. drivers/watchdog/pnx4008_wdt.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)diff --git a/drivers/watchdog/pnx4008_wdt.c b/drivers/watchdog/pnx4008_wdt.c index 88264a2..4b01b2b 100644 --- a/drivers/watchdog/pnx4008_wdt.c +++ b/drivers/watchdog/pnx4008_wdt.c@@ -31,7 +31,10 @@ #include <linux/slab.h> #include <linux/err.h> #include <linux/of.h> +#include <linux/delay.h> +#include <linux/reboot.h> #include <mach/hardware.h> +#include <mach/platform.h>
What are you using from mach/platform.h?
quoted hunk ↗ jump to hunk
/* WatchDog Timer - Chapter 23 Page 207 */@@ -77,6 +80,7 @@ static bool nowayout = WATCHDOG_NOWAYOUT; static unsigned int heartbeat = DEFAULT_HEARTBEAT; +static struct notifier_block restart_handler; static DEFINE_SPINLOCK(io_lock); static void __iomem *wdt_base;@@ -124,6 +128,27 @@ static int pnx4008_wdt_set_timeout(struct watchdog_device *wdd, return 0; } +static void pnx4008_wdt_sys_reset(enum reboot_mode mode, const char *cmd) +{ + /* Make sure WDT clocks are enabled */ + writel(LPC32XX_CLKPWR_PWMCLK_WDOG_EN, LPC32XX_CLKPWR_TIMER_CLK_CTRL);
Doesn't the watchdog driver already enable this clock in probe? Either way doing a direct write to LPC32XX_CLKPWR_TIMER_CLK_CTRL from here isn't very nice.
+ /* Instant assert of RESETOUT_N with pulse length 1mS */
+ writel(13000, WDTIM_PULSE(wdt_base));
+ writel(M_RES2 | RESFRC1 | RESFRC2, WDTIM_MCTRL(wdt_base));
+
+ /* Wait for watchdog to reset system */
+ mdelay(1000);
+}
+
+static int pnx4008_restart_handler(struct notifier_block *this,
+ unsigned long mode, void *cmd)
+{
+ pnx4008_wdt_sys_reset((enum reboot_mode)mode, cmd);Why can you just the stuff from pnx4008_wdt_sys_reset() in this function?
quoted hunk ↗ jump to hunk
+ + return NOTIFY_DONE; +} + static const struct watchdog_info pnx4008_wdt_ident = { .options = WDIOF_CARDRESET | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,@@ -157,6 +182,11 @@ static int pnx4008_wdt_probe(struct platform_device *pdev) if (IS_ERR(wdt_base)) return PTR_ERR(wdt_base); + /* Register restart handler. */ + restart_handler.notifier_call = pnx4008_restart_handler; + restart_handler.priority = 128; + ret = register_restart_handler(&restart_handler);
Checking return value would be a nice thing to do. You should also do a unregister_restart_handler() in driver remove hook. regards, Joachim Eastwood