RE: [PATCH 2/2] watchdog: ts72xx_wdt: convert driver to watchdog core

[Hartley Sweeten]

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