On 22.01.2024 19:39, Guenter Roeck wrote: > On 1/22/24 03:11, Claudiu wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> >> >> The RZ/G3S supports deep sleep states where power to most of the IP blocks >> is cut off. To ensure proper working of the watchdog when resuming from >> such states, the suspend function is stopping the watchdog and the resume >> function is starting it. There is no need to configure the watchdog >> in case the watchdog was stopped prior to starting suspend. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> >> --- >> drivers/watchdog/rzg2l_wdt.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c >> index 9333dc1a75ab..186796b739f7 100644 >> --- a/drivers/watchdog/rzg2l_wdt.c >> +++ b/drivers/watchdog/rzg2l_wdt.c >> @@ -279,6 +279,7 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) >> priv->wdev.timeout = WDT_DEFAULT_TIMEOUT; >> watchdog_set_drvdata(&priv->wdev, priv); >> + dev_set_drvdata(dev, priv); >> ret = devm_add_action_or_reset(&pdev->dev, rzg2l_wdt_pm_disable, >> &priv->wdev); >> if (ret) >> return ret; >> @@ -300,10 +301,35 @@ static const struct of_device_id rzg2l_wdt_ids[] = { >> }; >> MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids); >> +static int rzg2l_wdt_suspend_late(struct device *dev) >> +{ >> + struct rzg2l_wdt_priv *priv = dev_get_drvdata(dev); >> + >> + if (!watchdog_active(&priv->wdev)) >> + return 0; >> + >> + return rzg2l_wdt_stop(&priv->wdev); >> +} >> + >> +static int rzg2l_wdt_resume_early(struct device *dev) >> +{ >> + struct rzg2l_wdt_priv *priv = dev_get_drvdata(dev); >> + >> + if (!watchdog_active(&priv->wdev)) >> + return 0; >> + >> + return rzg2l_wdt_start(&priv->wdev); >> +} >> + >> +static const struct dev_pm_ops rzg2l_wdt_pm_ops = { >> + LATE_SYSTEM_SLEEP_PM_OPS(rzg2l_wdt_suspend_late, >> rzg2l_wdt_resume_early) >> +}; >> + >> static struct platform_driver rzg2l_wdt_driver = { >> .driver = { >> .name = "rzg2l_wdt", >> .of_match_table = rzg2l_wdt_ids, >> + .pm = pm_ptr(&rzg2l_wdt_pm_ops), > > I think this will create a build error if CONFIG_PM=n because rzg2l_wdt_pm_ops > will be unused but is not marked with __maybe_unused. The necessity of __maybe_unused has been removed along with the introduction of LATE_SYSTEM_SLEEP_PM_OPS() and friends (and *SET_*LATE_SYSTEM_SLEEP_PM_OPS along with the other helpers were marked deprecated for that) and we can use pm_ptr() along with LATE_SYSTEM_SLEEP_PM_OPS() to avoid build errors you mentioned. FYI, I just build the driver with CONFIG_PM=n and all good. > But then the driver > won't be > operational with CONFIG_PM=n, so I really wonder if it makes sense to > include any > such conditional code instead of making the driver depend on CONFIG_PM. That's true. The driver wouldn't work if the CONFIG_PM=n but then it depends on COMPILE_TEST which is exactly for this (just to compile test it for platforms that don't support it). I see many watchdog drivers depends on COMPILE_TEST. Give this, please let me know would you like me to proceed with it. Thank you, Claudiu Beznea > > I really don't think it is desirable to suggest that the driver would work > with > CONFIG_PM=n if that isn't really true. > > Guenter > >> }, >> .probe = rzg2l_wdt_probe, >> }; >