On 1/22/24 13:05, Jerry Hoemann wrote:
On Mon, Jan 22, 2024 at 09:39:27AM -0800, 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. 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.
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
Guenter,
I'm working on a similar patch.
Is your concern limited to the use of the "pm_ptr" macro? Or is it
wider?
patch 3/10 adds an error check of the return value from pm_runtime_put().
pm_runtime_put() calls __pm_runtime_idle() which returns -ENOSYS if
CONFIG_PM=n. That means checking the return value of pm_runtime_put()
is equivalent to making CONFIG_PM mandatory. My argument is that this
should be expressed in Kconfig to avoid the impression that the driver
works with CONFIG_PM=n. If the driver depends on CONFIG_PM, pm_ptr()
is unnecessary. If it doesn't, the variable referenced by it needs
to be defined as __maybe_unused, but then the driver should actually
work with CONFIG_PM=n.
Yes, I have noticed the recent trend of adding error checks to
pm_runtime_put(), but I only now realized that it has consequences.
Guenter
