On Monday, January 30, 2017 12:02 PM, Guenter Roeck wrote:
> On Mon, Jan 30, 2017 at 09:55:48AM -0700, H Hartley Sweeten wrote:
>> Cleanup this driver and convert it to use the watchdog framework API.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> Cc: Mika Westerberg <mika.westerberg@xxxxxx>
>> Cc: Wim Van Sebroeck <wim@xxxxxxxxx>
>> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
>> ---
>> drivers/watchdog/ts72xx_wdt.c | 447 +++++++++---------------------------------
>> 1 file changed, 93 insertions(+), 354 deletions(-)
<snip>
>> -#define TS72XX_WDT_DEFAULT_TIMEOUT 8
>> -
>> -static int timeout = TS72XX_WDT_DEFAULT_TIMEOUT;
>> -module_param(timeout, int, 0);
>> -MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
>> - "(1 <= timeout <= 8, default="
>> - __MODULE_STRING(TS72XX_WDT_DEFAULT_TIMEOUT)
>> - ")");
>
> Same question as with patch #1 - are you sure you want to take this away ?
Again, not a problem leaving it in.
> You might just drop the limits instead (also see below).
<snip>
>> + wdd->min_timeout = 1;
>> + wdd->max_timeout = 8;
>
> With such a low maximum timeout, it might make sense to use the core
> to be able to support larger timeouts.
Agree. I'll update and test this for the next version.
>> + wdd->timeout = 8;
>> + wdd->parent = &pdev->dev;
>>
>> - /* make sure that the watchdog is disabled */
>> - ts72xx_wdt_stop(wdt);
>
> Are you sure this is no longer needed ? If there is a means to detect if the
> watchdog is running, it might make sense to set the WDOG_HW_RUNNING flag instead
> if it is running and let the core handle the ping until the watchdog device
> is opened.
A patch to make sure this watchdog is disabled early during the kernel uncompress
will soon be applied. Arnd Bergmann has it in his todo folder:
[PATCH] ARM: ep93xx: Disable TS-72xx watchdog before uncompressing
The bootloader currently enables the watchdog for 8 seconds and it needs to be
disabled just in case the uncompress takes longer.
I don't have a problem leaving it in here it's just not necessary.
>> + watchdog_set_nowayout(wdd, nowayout);
>>
>> - error = misc_register(&ts72xx_wdt_miscdev);
>> - if (error) {
>> - dev_err(&pdev->dev, "failed to register miscdev\n");
>> - return error;
>> - }
>> + watchdog_set_drvdata(wdd, priv);
>> +
>> + ret = watchdog_register_device(wdd);
>
> devm_watchdog_register_device() ?
I'll update this.
Thanks,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
