Linux-Watchdog Archive on lore.kernel.org
 help / Atom feed
* [PATCH] watchdog: dw: use devm_watchdog_register_device()
@ 2019-01-25  7:52 Jisheng Zhang
  2019-01-25  8:04 ` Guenter Roeck
  0 siblings, 1 reply; 4+ messages in thread
From: Jisheng Zhang @ 2019-01-25  7:52 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck; +Cc: linux-watchdog, linux-kernel

Use devm_watchdog_register_device() to simplify the code.

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] watchdog: dw: use devm_watchdog_register_device()
  2019-01-25  7:52 [PATCH] watchdog: dw: use devm_watchdog_register_device() Jisheng Zhang
@ 2019-01-25  8:04 ` Guenter Roeck
  2019-01-25  8:17   ` Jisheng Zhang
  0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2019-01-25  8:04 UTC (permalink / raw)
  To: Jisheng Zhang, Wim Van Sebroeck; +Cc: linux-watchdog, linux-kernel

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 <Jisheng.Zhang@synaptics.com>
> ---
>   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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] watchdog: dw: use devm_watchdog_register_device()
  2019-01-25  8:04 ` Guenter Roeck
@ 2019-01-25  8:17   ` Jisheng Zhang
  2019-01-25  9:01     ` Guenter Roeck
  0 siblings, 1 reply; 4+ messages in thread
From: Jisheng Zhang @ 2019-01-25  8:17 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel

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 <Jisheng.Zhang@synaptics.com>
> > ---
> >   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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] watchdog: dw: use devm_watchdog_register_device()
  2019-01-25  8:17   ` Jisheng Zhang
@ 2019-01-25  9:01     ` Guenter Roeck
  0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2019-01-25  9:01 UTC (permalink / raw)
  To: Jisheng Zhang; +Cc: Wim Van Sebroeck, linux-watchdog, linux-kernel

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 <Jisheng.Zhang@synaptics.com>
>>> ---
>>>    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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-25  7:52 [PATCH] watchdog: dw: use devm_watchdog_register_device() Jisheng Zhang
2019-01-25  8:04 ` Guenter Roeck
2019-01-25  8:17   ` Jisheng Zhang
2019-01-25  9:01     ` Guenter Roeck

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/ public-inbox