Hi Sylvain,
On 22 February 2016 at 17:55, <slemieux.tyco@xxxxxxxxx> wrote:
> From: Sylvain Lemieux <slemieux@xxxxxxxxxxx>
>
> 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.
> Signed-off-by: Sylvain Lemieux <slemieux@xxxxxxxxxxx>
> ---
> 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?
>
> /* 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?
> +
> + 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
--
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
