diff mbox series

watchdog: meson_gxbb_wdt: improve

Message ID 20210622095639.1280774-1-art@khadas.com (mailing list archive)
State Superseded
Headers show
Series watchdog: meson_gxbb_wdt: improve | expand

Commit Message

Artem Lapkin June 22, 2021, 9:56 a.m. UTC 
Improve meson_gxbb_wdt watchdog driver
1) added module param timeout and nowayout same as other modules
2) print watchdog driver start status
3) add watchdog_stop_on_unregister
4) remove watchdog_stop_on_reboot ( still can be activated by
watchdog.stop_on_reboot=1 ) i think this driver configuration more useful
becouse we can get reboot waranty for abnormal situations on shutdown stage

Signed-off-by: Artem Lapkin <art@khadas.com>
---
 drivers/watchdog/meson_gxbb_wdt.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Comments

Neil Armstrong June 22, 2021, 11:08 a.m. UTC | #1 
Hi Art,

On 22/06/2021 11:56, Artem Lapkin wrote:
> Improve meson_gxbb_wdt watchdog driver
> 1) added module param timeout and nowayout same as other modules
> 2) print watchdog driver start status
> 3) add watchdog_stop_on_unregister
> 4) remove watchdog_stop_on_reboot ( still can be activated by
> watchdog.stop_on_reboot=1 ) i think this driver configuration more useful
> becouse we can get reboot waranty for abnormal situations on shutdown stage

Can you split the patch in 4 distinct changes ?

Neil

> 
> Signed-off-by: Artem Lapkin <art@khadas.com>
> ---
>  drivers/watchdog/meson_gxbb_wdt.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> index 5a9ca10fbcfa..15c889932c13 100644
> --- a/drivers/watchdog/meson_gxbb_wdt.c
> +++ b/drivers/watchdog/meson_gxbb_wdt.c
> @@ -35,6 +35,17 @@ struct meson_gxbb_wdt {
>  	struct clk *clk;
>  };
>  
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +static unsigned int timeout = DEFAULT_TIMEOUT;
> +
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default="
> +			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds="
> +			__MODULE_STRING(DEFAULT_TIMEOUT) ")");
> +
>  static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
>  {
>  	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> @@ -174,7 +185,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>  	data->wdt_dev.ops = &meson_gxbb_wdt_ops;
>  	data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>  	data->wdt_dev.min_timeout = 1;
> -	data->wdt_dev.timeout = DEFAULT_TIMEOUT;
> +	data->wdt_dev.timeout = timeout;
>  	watchdog_set_drvdata(&data->wdt_dev, data);
>  
>  	/* Setup with 1ms timebase */
> @@ -186,7 +197,12 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>  
>  	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>  
> -	watchdog_stop_on_reboot(&data->wdt_dev);
> +	watchdog_set_nowayout(&data->wdt_dev, nowayout);
> +	watchdog_stop_on_unregister(&data->wdt_dev);
> +
> +	dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
> +		data->wdt_dev.timeout, nowayout);
> +
>  	return devm_watchdog_register_device(dev, &data->wdt_dev);
>  }
>  
>
Artem Lapkin June 22, 2021, 11:53 a.m. UTC | #2 
> Neil
> Can you split the patch in 4 distinct changes ?

yes  no problem i can try to do it tomorrow !
maybe somebody have other ideas, suggestion, comments ...


On Tue, Jun 22, 2021 at 7:08 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Hi Art,
>
> On 22/06/2021 11:56, Artem Lapkin wrote:
> > Improve meson_gxbb_wdt watchdog driver
> > 1) added module param timeout and nowayout same as other modules
> > 2) print watchdog driver start status
> > 3) add watchdog_stop_on_unregister
> > 4) remove watchdog_stop_on_reboot ( still can be activated by
> > watchdog.stop_on_reboot=1 ) i think this driver configuration more useful
> > becouse we can get reboot waranty for abnormal situations on shutdown stage
>
> Can you split the patch in 4 distinct changes ?
>
> Neil
>
> >
> > Signed-off-by: Artem Lapkin <art@khadas.com>
> > ---
> >  drivers/watchdog/meson_gxbb_wdt.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
> > index 5a9ca10fbcfa..15c889932c13 100644
> > --- a/drivers/watchdog/meson_gxbb_wdt.c
> > +++ b/drivers/watchdog/meson_gxbb_wdt.c
> > @@ -35,6 +35,17 @@ struct meson_gxbb_wdt {
> >       struct clk *clk;
> >  };
> >
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> > +static unsigned int timeout = DEFAULT_TIMEOUT;
> > +
> > +module_param(nowayout, bool, 0);
> > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default="
> > +                     __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +module_param(timeout, uint, 0);
> > +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds="
> > +                     __MODULE_STRING(DEFAULT_TIMEOUT) ")");
> > +
> >  static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
> >  {
> >       struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
> > @@ -174,7 +185,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
> >       data->wdt_dev.ops = &meson_gxbb_wdt_ops;
> >       data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
> >       data->wdt_dev.min_timeout = 1;
> > -     data->wdt_dev.timeout = DEFAULT_TIMEOUT;
> > +     data->wdt_dev.timeout = timeout;
> >       watchdog_set_drvdata(&data->wdt_dev, data);
> >
> >       /* Setup with 1ms timebase */
> > @@ -186,7 +197,12 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
> >
> >       meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
> >
> > -     watchdog_stop_on_reboot(&data->wdt_dev);
> > +     watchdog_set_nowayout(&data->wdt_dev, nowayout);
> > +     watchdog_stop_on_unregister(&data->wdt_dev);
> > +
> > +     dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
> > +             data->wdt_dev.timeout, nowayout);
> > +
> >       return devm_watchdog_register_device(dev, &data->wdt_dev);
> >  }
> >
> >
>
Neil Armstrong June 22, 2021, 12:43 p.m. UTC | #3 
Hi,

On 22/06/2021 13:53, Art Nikpal wrote:
>> Neil
>> Can you split the patch in 4 distinct changes ?
> 
> yes  no problem i can try to do it tomorrow !
> maybe somebody have other ideas, suggestion, comments ...

The changeset is clean, and overall I'm ok with the changes, but I'm pretty sure the wdt maintainers
will prefer separate changes in order to comment of each.
Neil

> 
> 
> On Tue, Jun 22, 2021 at 7:08 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> Hi Art,
>>
>> On 22/06/2021 11:56, Artem Lapkin wrote:
>>> Improve meson_gxbb_wdt watchdog driver
>>> 1) added module param timeout and nowayout same as other modules
>>> 2) print watchdog driver start status
>>> 3) add watchdog_stop_on_unregister
>>> 4) remove watchdog_stop_on_reboot ( still can be activated by
>>> watchdog.stop_on_reboot=1 ) i think this driver configuration more useful
>>> becouse we can get reboot waranty for abnormal situations on shutdown stage
>>
>> Can you split the patch in 4 distinct changes ?
>>
>> Neil
>>
>>>
>>> Signed-off-by: Artem Lapkin <art@khadas.com>
>>> ---
>>>  drivers/watchdog/meson_gxbb_wdt.c | 20 ++++++++++++++++++--
>>>  1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
>>> index 5a9ca10fbcfa..15c889932c13 100644
>>> --- a/drivers/watchdog/meson_gxbb_wdt.c
>>> +++ b/drivers/watchdog/meson_gxbb_wdt.c
>>> @@ -35,6 +35,17 @@ struct meson_gxbb_wdt {
>>>       struct clk *clk;
>>>  };
>>>
>>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>>> +static unsigned int timeout = DEFAULT_TIMEOUT;
>>> +
>>> +module_param(nowayout, bool, 0);
>>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default="
>>> +                     __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>> +
>>> +module_param(timeout, uint, 0);
>>> +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds="
>>> +                     __MODULE_STRING(DEFAULT_TIMEOUT) ")");
>>> +
>>>  static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
>>>  {
>>>       struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
>>> @@ -174,7 +185,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>>>       data->wdt_dev.ops = &meson_gxbb_wdt_ops;
>>>       data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>>>       data->wdt_dev.min_timeout = 1;
>>> -     data->wdt_dev.timeout = DEFAULT_TIMEOUT;
>>> +     data->wdt_dev.timeout = timeout;
>>>       watchdog_set_drvdata(&data->wdt_dev, data);
>>>
>>>       /* Setup with 1ms timebase */
>>> @@ -186,7 +197,12 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev)
>>>
>>>       meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>>>
>>> -     watchdog_stop_on_reboot(&data->wdt_dev);
>>> +     watchdog_set_nowayout(&data->wdt_dev, nowayout);
>>> +     watchdog_stop_on_unregister(&data->wdt_dev);
>>> +
>>> +     dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
>>> +             data->wdt_dev.timeout, nowayout);
>>> +
>>>       return devm_watchdog_register_device(dev, &data->wdt_dev);
>>>  }
>>>
>>>
>>
Guenter Roeck June 22, 2021, 5 p.m. UTC | #4 
On Tue, Jun 22, 2021 at 02:43:45PM +0200, Neil Armstrong wrote:
> Hi,
> 
> On 22/06/2021 13:53, Art Nikpal wrote:
> >> Neil
> >> Can you split the patch in 4 distinct changes ?
> > 
> > yes  no problem i can try to do it tomorrow !
> > maybe somebody have other ideas, suggestion, comments ...
> 
> The changeset is clean, and overall I'm ok with the changes, but I'm pretty sure the wdt maintainers
> will prefer separate changes in order to comment of each.

Correct. As per guidelines, "one logical change per patch".

Guenter
diff mbox series

Patch

diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c
index 5a9ca10fbcfa..15c889932c13 100644
--- a/drivers/watchdog/meson_gxbb_wdt.c
+++ b/drivers/watchdog/meson_gxbb_wdt.c
@@ -35,6 +35,17 @@  struct meson_gxbb_wdt {
 	struct clk *clk;
 };
 
+static bool nowayout = WATCHDOG_NOWAYOUT;
+static unsigned int timeout = DEFAULT_TIMEOUT;
+
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started default="
+			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds="
+			__MODULE_STRING(DEFAULT_TIMEOUT) ")");
+
 static int meson_gxbb_wdt_start(struct watchdog_device *wdt_dev)
 {
 	struct meson_gxbb_wdt *data = watchdog_get_drvdata(wdt_dev);
@@ -174,7 +185,7 @@  static int meson_gxbb_wdt_probe(struct platform_device *pdev)
 	data->wdt_dev.ops = &meson_gxbb_wdt_ops;
 	data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
 	data->wdt_dev.min_timeout = 1;
-	data->wdt_dev.timeout = DEFAULT_TIMEOUT;
+	data->wdt_dev.timeout = timeout;
 	watchdog_set_drvdata(&data->wdt_dev, data);
 
 	/* Setup with 1ms timebase */
@@ -186,7 +197,12 @@  static int meson_gxbb_wdt_probe(struct platform_device *pdev)
 
 	meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
 
-	watchdog_stop_on_reboot(&data->wdt_dev);
+	watchdog_set_nowayout(&data->wdt_dev, nowayout);
+	watchdog_stop_on_unregister(&data->wdt_dev);
+
+	dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)",
+		data->wdt_dev.timeout, nowayout);
+
 	return devm_watchdog_register_device(dev, &data->wdt_dev);
 }