Re: watchdog's pm support preffered implementation

["Janusz Użycki" <j.uzycki@elproma.com.pl>]

W dniu 2014-09-19 05:11, Guenter Roeck pisze:
> On 09/18/2014 03:02 PM, Janusz Użycki wrote:
>> Small fix below in the second implementation.
>>
>> best regards
>> Janusz
>>
>> W dniu 2014-09-18 23:40, Janusz Użycki pisze:
>>> Hi again,
>>>
>>> This is the second implementation. Which do you prefer?
>>>
> This one

ok

>
>>> Subject: [PATCH] stmp3xxx_rtc_wdt: Add suspend/resume PM support
>>>
>>>
>>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
>>> ---
>>>  drivers/watchdog/stmp3xxx_rtc_wdt.c | 39 +++++++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c 
>>> b/drivers/watchdog/stmp3xxx_rtc_wdt.c
>>> index 3546f03..1946277 100644
>>> --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c
>>> +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c
>>> @@ -95,9 +95,48 @@ static int stmp3xxx_wdt_remove(struct 
>>> platform_device *pdev)
>>>         return 0;
>>>  }
>>>
>>> +#ifdef CONFIG_PM_SLEEP
>>> +/* There is no conflict with rtc/rtc-stmp3xxx.c parent
>>> + * because modified registers in PM functions are different */
>
> Coding style, and what does this comment mean ? To me it is just 
> confusing.

I will move to comment of commit

>
>>> +static int __maybe_unused stmp3xxx_wdt_suspend(struct device *dev)
>>> +{
>>> +       struct watchdog_device *wdd = dev_get_drvdata(dev);
>>
>> drvdata is NULL, too fast,
>> +       struct watchdog_device *wdd = &stmp3xxx_wdd;
>>
>>> +
>>> +       /* Is keep-on/ping timer suspended before?
>>> +        * or additional driver-specific flag must be added
>>> +        *  to block watchdog ping in the timer?
>>> +        * or disable WATCHDOG_KEEP_ON before wdt_stop
>>> +        *  and restore it in resume? */
>
> You'll have to answer those questions.

I guess you don't know if timers are stopped before susnder of other 
drivers?

>
>>> +       if (watchdog_active(wdd)) {
>>> +               dev_info(wdd->dev, "%s: wdt was active\n", __func__);
>
> Does this message add any value ?
>
>>> +               return wdt_stop(wdd);
>>> +       }
>>> +       /* should we use pm_runtime like omap_wdt.c does? */
>
> Isn't that what you do here ?

I meant pm_runtime_put/get_sync() etc.
Maybe it is connected to my question above about timers.

>
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int __maybe_unused stmp3xxx_wdt_resume(struct device *dev)
>
> Does the __maybe_unused really apply ?

What do you preffer: __maybe_unused or ifdef CONFIG_PM_SLEEP?
I guess the first one because SIMPLE_DEV_PM_OPS/SET_SYSTEM_SLEEP_PM_OPS
just uses CONFIG_PM_SLEEP.

>
>>> +{
>>> +       struct watchdog_device *wdd = dev_get_drvdata(dev);
>> +       struct watchdog_device *wdd = &stmp3xxx_wdd;
>
> One of those lines needs to go.
Sure, I wanted to fix it on the list in fast way.
I'll use static stmp3xxx_wdd.

>
> Do you have indentation problems ? Do you use space for indentations, 
> maybe ?

No, it's probably Thunderbird copy-paste despite text email format.
When I finish fixes I will send patches using git:
1. watchdog: watchdog_dev: WATCHDOG_KEEP_ON feature
2. watchdog: boottime protection feature (requires 'keep on')
3. stmp3xxx_rtc_wdt: WATCHDOG_KEEP_ON enabled
4. stmp3xxx_rtc_wdt: Add suspend/resume PM support

>
>>
>>> +
>>> +       if (watchdog_active(wdd)) {
>>> +               dev_info(wdd->dev, "%s: wdt was active\n", __func__);
>
> Does this message add any value ?

It was only for debug. Removed.

>
>>> +               return wdt_start(wdd);
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +#endif /* CONFIG_PM_SLEEP */
>>> +
>>> +static SIMPLE_DEV_PM_OPS(stmp3xxx_wdt_pm_ops,
>>> +               stmp3xxx_wdt_suspend, stmp3xxx_wdt_resume);
>
> Please align second line with (
ok

>
>>> +
>>>  static struct platform_driver stmp3xxx_wdt_driver = {
>>>         .driver = {
>>>                 .name = "stmp3xxx_rtc_wdt",
>>> +               .owner = THIS_MODULE,
>
> Is this needed ? I have seen the .owner assignment being removed
> in other drivers recently.

I just noticed that some drivers have the assignment and others not.

https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/include/linux/platform_device.h?id=9447057eaff871dd7c63c808de761b8732407169
http://lxr.free-electrons.com/source/include/linux/platform_device.h?v=3.14
"use a macro to avoid include chaining to get THIS_MODULE"

You are right. The direction is to remove this. Removed.

"owner" history from must have to remove (for others):
http://stackoverflow.com/questions/19467150/significance-of-this-module-in-linux-driver
http://lists.linaro.org/pipermail/linaro-kernel/2013-July/005457.html
https://lkml.org/lkml/2014/9/8/1

best regards
Janusz