Re: [PATCH] watchdog: meson_gxbb_wdt: improve

[Neil Armstrong <narmstrong@baylibre.com>]

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);
>>>  }
>>>
>>>
>>