On 09 December 2014 11:23 Geert Uytterhoeven wrote: Hi Geert,
> Subject: [PATCH 1/4] watchdog: da9063: Add restart handler support > > Register a restart handler for the da9063 watchdog. System restart is > triggered by sending the shutdown command to the PMIC. > As more-suitable restart handlers may exist, the priority of the > watchdog restart handler is set to 128. > > The actual restart method was inspired by a platform-specific patch from > the BSP by Hisashi Nakamura <[email protected]>. > > Signed-off-by: Geert Uytterhoeven <[email protected]> > --- > Tested on r8a7791/koelsch > --- [...] +static int da9063_wdt_restart_handler(struct notifier_block *this, + unsigned long mode, void *cmd) +{ + struct da9063_watchdog *wdt = container_of(this, + struct da9063_watchdog, + restart_handler); + int ret; + + ret = regmap_write(wdt->da9063->regmap, DA9063_REG_CONTROL_F, + DA9063_SHUTDOWN); I am stating the obvious here. DA9063_REG_CONTROL_F, DA9063_SHUTDOWN will cause the sequencer to power down to RESET Mode -- which means if this I2C command succeeds there will be a sudden loss of power and any unsaved information will be lost after this point. This is exactly how I have done it in the past. + if (ret) + dev_alert(wdt->da9063->dev, "Failed to shutdown (err = %d)\n", + ret); + + return NOTIFY_DONE; +} + [...] > static const struct watchdog_info da9063_watchdog_info = { > .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, > .identity = "DA9063 Watchdog", > @@ -163,14 +182,25 @@ static int da9063_wdt_probe(struct > platform_device *pdev) > dev_set_drvdata(&pdev->dev, wdt); > > ret = watchdog_register_device(&wdt->wdtdev); > + if (ret) > + return ret; > > - return ret; > + wdt->restart_handler.notifier_call = da9063_wdt_restart_handler; > + wdt->restart_handler.priority = 128; > + ret = register_restart_handler(&wdt->restart_handler); > + if (ret) > + dev_err(wdt->da9063->dev, > + "Failed to register restart handler (err = %d)\n", ret); (I guess this is intentional).. if the restart handler doesn't get registered then this problem is just ignored and not counted as a real error. > + > + return 0; > } > > static int da9063_wdt_remove(struct platform_device *pdev) > { > struct da9063_watchdog *wdt = dev_get_drvdata(&pdev->dev); > > + unregister_restart_handler(&wdt->restart_handler); > + > watchdog_unregister_device(&wdt->wdtdev); > > return 0; Despite the above, these are just my comments. It all looks fine to me. Acked-by: Steve Twiss <[email protected]> Regards, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
