Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Guenter,

On 20 January 2018 at 21:20, Guenter Roeck  wrote:
> On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil  wrote:
>>>
>>> When the watchdog was configured for nowayout, and after the
>>> userspace watchdog daemon closed the dev node without sending the
>>> magic character, unloading this module stopped the watchdog
>>> hardware, which was clearly a problem.
>>>
>>> Besides, unloading the module is not possible when the userspace
>>> watchdog daemon is running, so it's safe to assume that we don't
>>> need to stop the watchdog hardware in the jz4740_wdt_remove()
>>> function.
>>>
>>> For this reason, the jz4740_wdt_remove() function can then be
>>> dropped alltogether.
>>>
>>> Signed-off-by: Paul Cercueil 
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 8 
>>>   1 file changed, 8 deletions(-)
>>>
>>>   v2: New patch in this series
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index fa7f49a3212c..02b9b8e946a2 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device
>>> *pdev)
>>>  return 0;
>>>   }
>>>
>>> -static int jz4740_wdt_remove(struct platform_device *pdev)
>>> -{
>>> -   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>>> -
>>> -   return jz4740_wdt_stop(>wdt);
>>> -}
>>> -
>>>   static struct platform_driver jz4740_wdt_driver = {
>>>  .probe = jz4740_wdt_probe,
>>> -   .remove = jz4740_wdt_remove,
>>>  .driver = {
>>>  .name = "jz4740-wdt",
>>>  .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
>>> --
>>> 2.11.0
>>>
>>>
>>
>> As ".remove" is removed and wdt is required for restarting the system
>> I am thinking that stop callback is also not required. If so does it
>> makes sense to remove the stop callback? I can submit a patch for the
>> same once this patch series goes in.
>>
> The remove function was removed because it would otherwise be an empty
> function. Since it is optional, it can and should be removed if it does not
> do anything. If the stop function is removed, it is no longer possible
> to stop the watchdog. Why would this make sense, and why would it make sense
> to drop the stop function if there is no remove function ?
>
> Guenter
>

I missed the fact that stopping is watchdog is possible. Sorry for the noise.

Thanks,
PrasannaKumar

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Guenter,

On 20 January 2018 at 21:20, Guenter Roeck  wrote:
> On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil  wrote:
>>>
>>> When the watchdog was configured for nowayout, and after the
>>> userspace watchdog daemon closed the dev node without sending the
>>> magic character, unloading this module stopped the watchdog
>>> hardware, which was clearly a problem.
>>>
>>> Besides, unloading the module is not possible when the userspace
>>> watchdog daemon is running, so it's safe to assume that we don't
>>> need to stop the watchdog hardware in the jz4740_wdt_remove()
>>> function.
>>>
>>> For this reason, the jz4740_wdt_remove() function can then be
>>> dropped alltogether.
>>>
>>> Signed-off-by: Paul Cercueil 
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 8 
>>>   1 file changed, 8 deletions(-)
>>>
>>>   v2: New patch in this series
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index fa7f49a3212c..02b9b8e946a2 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device
>>> *pdev)
>>>  return 0;
>>>   }
>>>
>>> -static int jz4740_wdt_remove(struct platform_device *pdev)
>>> -{
>>> -   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>>> -
>>> -   return jz4740_wdt_stop(>wdt);
>>> -}
>>> -
>>>   static struct platform_driver jz4740_wdt_driver = {
>>>  .probe = jz4740_wdt_probe,
>>> -   .remove = jz4740_wdt_remove,
>>>  .driver = {
>>>  .name = "jz4740-wdt",
>>>  .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
>>> --
>>> 2.11.0
>>>
>>>
>>
>> As ".remove" is removed and wdt is required for restarting the system
>> I am thinking that stop callback is also not required. If so does it
>> makes sense to remove the stop callback? I can submit a patch for the
>> same once this patch series goes in.
>>
> The remove function was removed because it would otherwise be an empty
> function. Since it is optional, it can and should be removed if it does not
> do anything. If the stop function is removed, it is no longer possible
> to stop the watchdog. Why would this make sense, and why would it make sense
> to drop the stop function if there is no remove function ?
>
> Guenter
>

I missed the fact that stopping is watchdog is possible. Sorry for the noise.

Thanks,
PrasannaKumar

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Guenter Roeck]

On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:

When the watchdog was configured for nowayout, and after the
userspace watchdog daemon closed the dev node without sending the
magic character, unloading this module stopped the watchdog
hardware, which was clearly a problem.

Besides, unloading the module is not possible when the userspace
watchdog daemon is running, so it's safe to assume that we don't
need to stop the watchdog hardware in the jz4740_wdt_remove()
function.

For this reason, the jz4740_wdt_remove() function can then be
dropped alltogether.

Signed-off-by: Paul Cercueil 
---
  drivers/watchdog/jz4740_wdt.c | 8 
  1 file changed, 8 deletions(-)

  v2: New patch in this series

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index fa7f49a3212c..02b9b8e946a2 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
 return 0;
  }

-static int jz4740_wdt_remove(struct platform_device *pdev)
-{
-   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
-   return jz4740_wdt_stop(>wdt);
-}
-
  static struct platform_driver jz4740_wdt_driver = {
 .probe = jz4740_wdt_probe,
-   .remove = jz4740_wdt_remove,
 .driver = {
 .name = "jz4740-wdt",
 .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
--
2.11.0




As ".remove" is removed and wdt is required for restarting the system
I am thinking that stop callback is also not required. If so does it
makes sense to remove the stop callback? I can submit a patch for the
same once this patch series goes in.


The remove function was removed because it would otherwise be an empty
function. Since it is optional, it can and should be removed if it does not
do anything. If the stop function is removed, it is no longer possible
to stop the watchdog. Why would this make sense, and why would it make sense
to drop the stop function if there is no remove function ?

Guenter

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Guenter Roeck]

On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:

When the watchdog was configured for nowayout, and after the
userspace watchdog daemon closed the dev node without sending the
magic character, unloading this module stopped the watchdog
hardware, which was clearly a problem.

Besides, unloading the module is not possible when the userspace
watchdog daemon is running, so it's safe to assume that we don't
need to stop the watchdog hardware in the jz4740_wdt_remove()
function.

For this reason, the jz4740_wdt_remove() function can then be
dropped alltogether.

Signed-off-by: Paul Cercueil 
---
  drivers/watchdog/jz4740_wdt.c | 8 
  1 file changed, 8 deletions(-)

  v2: New patch in this series

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index fa7f49a3212c..02b9b8e946a2 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
 return 0;
  }

-static int jz4740_wdt_remove(struct platform_device *pdev)
-{
-   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
-   return jz4740_wdt_stop(>wdt);
-}
-
  static struct platform_driver jz4740_wdt_driver = {
 .probe = jz4740_wdt_probe,
-   .remove = jz4740_wdt_remove,
 .driver = {
 .name = "jz4740-wdt",
 .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
--
2.11.0




As ".remove" is removed and wdt is required for restarting the system
I am thinking that stop callback is also not required. If so does it
makes sense to remove the stop callback? I can submit a patch for the
same once this patch series goes in.


The remove function was removed because it would otherwise be an empty
function. Since it is optional, it can and should be removed if it does not
do anything. If the stop function is removed, it is no longer possible
to stop the watchdog. Why would this make sense, and why would it make sense
to drop the stop function if there is no remove function ?

Guenter

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:
> When the watchdog was configured for nowayout, and after the
> userspace watchdog daemon closed the dev node without sending the
> magic character, unloading this module stopped the watchdog
> hardware, which was clearly a problem.
>
> Besides, unloading the module is not possible when the userspace
> watchdog daemon is running, so it's safe to assume that we don't
> need to stop the watchdog hardware in the jz4740_wdt_remove()
> function.
>
> For this reason, the jz4740_wdt_remove() function can then be
> dropped alltogether.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/watchdog/jz4740_wdt.c | 8 
>  1 file changed, 8 deletions(-)
>
>  v2: New patch in this series
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index fa7f49a3212c..02b9b8e946a2 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
> return 0;
>  }
>
> -static int jz4740_wdt_remove(struct platform_device *pdev)
> -{
> -   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
> -
> -   return jz4740_wdt_stop(>wdt);
> -}
> -
>  static struct platform_driver jz4740_wdt_driver = {
> .probe = jz4740_wdt_probe,
> -   .remove = jz4740_wdt_remove,
> .driver = {
> .name = "jz4740-wdt",
> .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
> --
> 2.11.0
>
>

As ".remove" is removed and wdt is required for restarting the system
I am thinking that stop callback is also not required. If so does it
makes sense to remove the stop callback? I can submit a patch for the
same once this patch series goes in.

Regards,
PrasannaKumar

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:
> When the watchdog was configured for nowayout, and after the
> userspace watchdog daemon closed the dev node without sending the
> magic character, unloading this module stopped the watchdog
> hardware, which was clearly a problem.
>
> Besides, unloading the module is not possible when the userspace
> watchdog daemon is running, so it's safe to assume that we don't
> need to stop the watchdog hardware in the jz4740_wdt_remove()
> function.
>
> For this reason, the jz4740_wdt_remove() function can then be
> dropped alltogether.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/watchdog/jz4740_wdt.c | 8 
>  1 file changed, 8 deletions(-)
>
>  v2: New patch in this series
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index fa7f49a3212c..02b9b8e946a2 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
> return 0;
>  }
>
> -static int jz4740_wdt_remove(struct platform_device *pdev)
> -{
> -   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
> -
> -   return jz4740_wdt_stop(>wdt);
> -}
> -
>  static struct platform_driver jz4740_wdt_driver = {
> .probe = jz4740_wdt_probe,
> -   .remove = jz4740_wdt_remove,
> .driver = {
> .name = "jz4740-wdt",
> .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
> --
> 2.11.0
>
>

As ".remove" is removed and wdt is required for restarting the system
I am thinking that stop callback is also not required. If so does it
makes sense to remove the stop callback? I can submit a patch for the
same once this patch series goes in.

Regards,
PrasannaKumar

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Guenter Roeck]

On 12/30/2017 05:51 AM, Paul Cercueil wrote:

When the watchdog was configured for nowayout, and after the
userspace watchdog daemon closed the dev node without sending the
magic character, unloading this module stopped the watchdog
hardware, which was clearly a problem.

Besides, unloading the module is not possible when the userspace
watchdog daemon is running, so it's safe to assume that we don't
need to stop the watchdog hardware in the jz4740_wdt_remove()
function.

For this reason, the jz4740_wdt_remove() function can then be
dropped alltogether.

Signed-off-by: Paul Cercueil 


Reviewed-by: Guenter Roeck 


---
  drivers/watchdog/jz4740_wdt.c | 8 
  1 file changed, 8 deletions(-)

  v2: New patch in this series

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index fa7f49a3212c..02b9b8e946a2 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
return 0;
  }
  
-static int jz4740_wdt_remove(struct platform_device *pdev)

-{
-   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
-   return jz4740_wdt_stop(>wdt);
-}
-
  static struct platform_driver jz4740_wdt_driver = {
.probe = jz4740_wdt_probe,
-   .remove = jz4740_wdt_remove,
.driver = {
.name = "jz4740-wdt",
.of_match_table = of_match_ptr(jz4740_wdt_of_matches),

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Guenter Roeck]

On 12/30/2017 05:51 AM, Paul Cercueil wrote:

When the watchdog was configured for nowayout, and after the
userspace watchdog daemon closed the dev node without sending the
magic character, unloading this module stopped the watchdog
hardware, which was clearly a problem.

Besides, unloading the module is not possible when the userspace
watchdog daemon is running, so it's safe to assume that we don't
need to stop the watchdog hardware in the jz4740_wdt_remove()
function.

For this reason, the jz4740_wdt_remove() function can then be
dropped alltogether.

Signed-off-by: Paul Cercueil 


Reviewed-by: Guenter Roeck 


---
  drivers/watchdog/jz4740_wdt.c | 8 
  1 file changed, 8 deletions(-)

  v2: New patch in this series

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index fa7f49a3212c..02b9b8e946a2 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
return 0;
  }
  
-static int jz4740_wdt_remove(struct platform_device *pdev)

-{
-   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
-   return jz4740_wdt_stop(>wdt);
-}
-
  static struct platform_driver jz4740_wdt_driver = {
.probe = jz4740_wdt_probe,
-   .remove = jz4740_wdt_remove,
.driver = {
.name = "jz4740-wdt",
.of_match_table = of_match_ptr(jz4740_wdt_of_matches),

[PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Paul Cercueil]

When the watchdog was configured for nowayout, and after the
userspace watchdog daemon closed the dev node without sending the
magic character, unloading this module stopped the watchdog
hardware, which was clearly a problem.

Besides, unloading the module is not possible when the userspace
watchdog daemon is running, so it's safe to assume that we don't
need to stop the watchdog hardware in the jz4740_wdt_remove()
function.

For this reason, the jz4740_wdt_remove() function can then be
dropped alltogether.

Signed-off-by: Paul Cercueil 
---
 drivers/watchdog/jz4740_wdt.c | 8 
 1 file changed, 8 deletions(-)

 v2: New patch in this series

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index fa7f49a3212c..02b9b8e946a2 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
return 0;
 }
 
-static int jz4740_wdt_remove(struct platform_device *pdev)
-{
-   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
-   return jz4740_wdt_stop(>wdt);
-}
-
 static struct platform_driver jz4740_wdt_driver = {
.probe = jz4740_wdt_probe,
-   .remove = jz4740_wdt_remove,
.driver = {
.name = "jz4740-wdt",
.of_match_table = of_match_ptr(jz4740_wdt_of_matches),
-- 
2.11.0

[PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Paul Cercueil]

When the watchdog was configured for nowayout, and after the
userspace watchdog daemon closed the dev node without sending the
magic character, unloading this module stopped the watchdog
hardware, which was clearly a problem.

Besides, unloading the module is not possible when the userspace
watchdog daemon is running, so it's safe to assume that we don't
need to stop the watchdog hardware in the jz4740_wdt_remove()
function.

For this reason, the jz4740_wdt_remove() function can then be
dropped alltogether.

Signed-off-by: Paul Cercueil 
---
 drivers/watchdog/jz4740_wdt.c | 8 
 1 file changed, 8 deletions(-)

 v2: New patch in this series

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index fa7f49a3212c..02b9b8e946a2 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
return 0;
 }
 
-static int jz4740_wdt_remove(struct platform_device *pdev)
-{
-   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
-   return jz4740_wdt_stop(>wdt);
-}
-
 static struct platform_driver jz4740_wdt_driver = {
.probe = jz4740_wdt_probe,
-   .remove = jz4740_wdt_remove,
.driver = {
.name = "jz4740-wdt",
.of_match_table = of_match_ptr(jz4740_wdt_of_matches),
-- 
2.11.0