[PATCH v2 2/5] watchdog: pnx4008: add restart handler

[Joachim Eastwood]

Hi Sylvain,

On 22 February 2016 at 17:55,  <slemieux.tyco at gmail.com> wrote:
> From: Sylvain Lemieux <slemieux at tycoint.com>
>
> 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 at tycoint.com>
> ---
> 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