| Message ID | 20240122111115.2861835-8-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: rzg2l_wdt: Add support for RZ/G3S | expand |
On 1/22/24 03:11, Claudiu wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > 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@bp.renesas.com> > --- > 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 > }, > .probe = rzg2l_wdt_probe, > };
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@bp.renesas.com> > > > > 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@bp.renesas.com> > > --- > > 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? Thanks Jerry
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@bp.renesas.com> >>> >>> 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@bp.renesas.com> >>> --- >>> 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
On 22.01.2024 19:39, Guenter Roeck wrote: > On 1/22/24 03:11, Claudiu wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> 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@bp.renesas.com> >> --- >> 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, >> }; >
On 1/22/24 23:13, claudiu beznea wrote: > > > On 22.01.2024 19:39, Guenter Roeck wrote: >> On 1/22/24 03:11, Claudiu wrote: >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>> >>> 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@bp.renesas.com> >>> --- >>> 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. > Ok, but are you sure you did ? You just mentioned earlier that CONFIG_PM is set automatically through ARCH_RZG2L. >> 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. > FWIW, COMPILE_TEST dependencies on watchdog drivers fails for most of them. Regarding pm_ptr(), it is there for practical reasons and not associated with COMPILE_TEST. Again, if the driver depends on CONFIG_PM to work, using constructs such as pm_ptr() just hides that and creates the impression that it would work without it. I do not think that is a good idea. You can use something like depends on (ARCH_RENESAS && PM) || COMPILE_TEST to make that explicit. Even if not, I _really_ don't see the point in using pm_ptr() even without above dependency. What do you see as its benefit ? Thanks, Guenter > 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, >>> }; >>
On 23.01.2024 12:09, Guenter Roeck wrote: > On 1/22/24 23:13, claudiu beznea wrote: >> >> >> On 22.01.2024 19:39, Guenter Roeck wrote: >>> On 1/22/24 03:11, Claudiu wrote: >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >>>> >>>> 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@bp.renesas.com> >>>> --- >>>> 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. >> > > Ok, but are you sure you did ? You just mentioned earlier that CONFIG_PM > is set automatically through ARCH_RZG2L. Yes, I disabled everything that selected the CONFIG_PM, checked that CONFIG_PM is disabled in my .config, enabled COMPILE_TEST and RENESAS_RZG2LWDT (sorry, I missed to mention all these). > >>> 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. >> > > FWIW, COMPILE_TEST dependencies on watchdog drivers fails for most of them. > Regarding pm_ptr(), it is there for practical reasons and not associated with > COMPILE_TEST. Again, if the driver depends on CONFIG_PM to work, using > constructs > such as pm_ptr() just hides that and creates the impression that it would work > without it. > I do not think that is a good idea. You can use something like > > depends on (ARCH_RENESAS && PM) || COMPILE_TEST > Ok, I don't have anything against. I'm not sure if this will trigger any circular dependency for Kconfig. I'll check it. > to make that explicit. Even if not, I _really_ don't see the point in using > pm_ptr() even without above dependency. What do you see as its benefit ? I remember it comes on the same package with the LATE_SYSTEM_SLEEP_PM_OPS() kind of macros. Looking at it's definition I see it useful because it sets properly the struct platform_driver::driver::pm. AFAIK, at the moment there are no checks on this member in the driver core code so we should be safe w/o using it. I checked the compilation w/ COMPILE_TEST=y and CONFIG_PM=n and compilation is good, too. Thank you, Claudiu Beznea > > Thanks, > Guenter > >> 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, >>>> }; >>> >
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), }, .probe = rzg2l_wdt_probe, };