2019-01-25 07:53:07

by Jisheng Zhang

Subject: [PATCH] watchdog: dw: use devm_watchdog_register_device()

Use devm_watchdog_register_device() to simplify the code.

Signed-off-by: Jisheng Zhang <[email protected]>
---
drivers/watchdog/dw_wdt.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
index 501aebb5b81f..c053c2de5c2f 100644
--- a/drivers/watchdog/dw_wdt.c
+++ b/drivers/watchdog/dw_wdt.c
@@ -303,7 +303,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)

watchdog_set_restart_priority(wdd, 128);

- ret = watchdog_register_device(wdd);
+ ret = devm_watchdog_register_device(wdd);
if (ret)
goto out_disable_clk;

@@ -318,7 +318,6 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
{
struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);

- watchdog_unregister_device(&dw_wdt->wdd);
reset_control_assert(dw_wdt->rst);
clk_disable_unprepare(dw_wdt->clk);

--
2.20.1

2019-01-25 08:04:57

by Guenter Roeck

Subject: Re: [PATCH] watchdog: dw: use devm_watchdog_register_device()

Hi,

On 1/24/19 11:52 PM, Jisheng Zhang wrote:
> Use devm_watchdog_register_device() to simplify the code.
>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> drivers/watchdog/dw_wdt.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index 501aebb5b81f..c053c2de5c2f 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -303,7 +303,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>
> watchdog_set_restart_priority(wdd, 128);
>
> - ret = watchdog_register_device(wdd);
> + ret = devm_watchdog_register_device(wdd);
> if (ret)
> goto out_disable_clk;
>
> @@ -318,7 +318,6 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
> {
> struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
>
> - watchdog_unregister_device(&dw_wdt->wdd);
> reset_control_assert(dw_wdt->rst);
> clk_disable_unprepare(dw_wdt->clk);
>
>
Unfortunately it isn't that easy. The other two calls have to be executed after
unregistering the watchdog, meaning you would have to add devm_add_action()
in the probe function to call them.

Guenter

2019-01-25 08:18:41

by Jisheng Zhang

Subject: Re: [PATCH] watchdog: dw: use devm_watchdog_register_device()

Hi,

On Fri, 25 Jan 2019 00:04:25 -0800 Guenter Roeck wrote:

> Hi,
>
> On 1/24/19 11:52 PM, Jisheng Zhang wrote:
> > Use devm_watchdog_register_device() to simplify the code.
> >
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > drivers/watchdog/dw_wdt.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> > index 501aebb5b81f..c053c2de5c2f 100644
> > --- a/drivers/watchdog/dw_wdt.c
> > +++ b/drivers/watchdog/dw_wdt.c
> > @@ -303,7 +303,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> >
> > watchdog_set_restart_priority(wdd, 128);
> >
> > - ret = watchdog_register_device(wdd);
> > + ret = devm_watchdog_register_device(wdd);
> > if (ret)
> > goto out_disable_clk;
> >
> > @@ -318,7 +318,6 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
> > {
> > struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
> >
> > - watchdog_unregister_device(&dw_wdt->wdd);
> > reset_control_assert(dw_wdt->rst);
> > clk_disable_unprepare(dw_wdt->clk);
> >
> >
> Unfortunately it isn't that easy. The other two calls have to be executed after
> unregistering the watchdog, meaning you would have to add devm_add_action()
> in the probe function to call them.

do you mean reset_control_asser() and the clk_disable_unprepare()? If yes,
does unregister the watchdog could trigger any register access? Per my
understanding, there's no register access path in the unregister. Am I
missing somthing?

Thanks

2019-01-25 09:01:57

by Guenter Roeck

Subject: Re: [PATCH] watchdog: dw: use devm_watchdog_register_device()

On 1/25/19 12:17 AM, Jisheng Zhang wrote:
> Hi,
>
> On Fri, 25 Jan 2019 00:04:25 -0800 Guenter Roeck wrote:
>
>> Hi,
>>
>> On 1/24/19 11:52 PM, Jisheng Zhang wrote:
>>> Use devm_watchdog_register_device() to simplify the code.
>>>
>>> Signed-off-by: Jisheng Zhang <[email protected]>
>>> ---
>>> drivers/watchdog/dw_wdt.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
>>> index 501aebb5b81f..c053c2de5c2f 100644
>>> --- a/drivers/watchdog/dw_wdt.c
>>> +++ b/drivers/watchdog/dw_wdt.c
>>> @@ -303,7 +303,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>>>
>>> watchdog_set_restart_priority(wdd, 128);
>>>
>>> - ret = watchdog_register_device(wdd);
>>> + ret = devm_watchdog_register_device(wdd);
>>> if (ret)
>>> goto out_disable_clk;
>>>
>>> @@ -318,7 +318,6 @@ static int dw_wdt_drv_remove(struct platform_device *pdev)
>>> {
>>> struct dw_wdt *dw_wdt = platform_get_drvdata(pdev);
>>>
>>> - watchdog_unregister_device(&dw_wdt->wdd);
>>> reset_control_assert(dw_wdt->rst);
>>> clk_disable_unprepare(dw_wdt->clk);
>>>
>>>
>> Unfortunately it isn't that easy. The other two calls have to be executed after
>> unregistering the watchdog, meaning you would have to add devm_add_action()
>> in the probe function to call them.
>
> do you mean reset_control_asser() and the clk_disable_unprepare()? If yes,

Yes.

> does unregister the watchdog could trigger any register access? Per my
> understanding, there's no register access path in the unregister. Am I
> missing somthing?
>

If the watchdog is unregistered last, it would be still accessible via its device
nodes, even though the clock has been reset, and even though the HW has been
put in reset. This would be racy.

Guenter