-- Treść oryginalnej wiadomości --
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