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 <hswee...@visionengravers.com>
>> Cc: Mika Westerberg <mika.westerb...@iki.fi>
>> Cc: Wim Van Sebroeck <w...@iguana.be>
>> Cc: Guenter Roeck <li...@roeck-us.net>
>> ---
>> 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
