Re: [PATCH V5 05/10] MIPS: lantiq: add watchdog support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi John,

> Changes in V2
> * add comments to explain register access
> * cleanup resource allocation
> * cleanup clock handling
> * whitespace fixes
> 
> Changes in V3
> * whitespace
> * change __iomem void to void __iomem
> * typo fixes
> * comment style
> * fix exit path in init function
> 
> Changes in V4
> * fixes register offsets (we use a smaller memory window)
> * typo in the comments
> * add __init to probe function

What were the changes for V5?

> +#ifndef CONFIG_WATCHDOG_NOWAYOUT
> +static int ltq_wdt_ok_to_close;
> +#endif
...
> +static void
> +ltq_wdt_disable(void)
> +{
> +#ifndef CONFIG_WATCHDOG_NOWAYOUT
> +	ltq_wdt_ok_to_close = 0;
> +#endif
> +	/* write the first password magic */
> +	ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR);
> +	/* write the second password magic with no config
> +	 * this turns the watchdog off
> +	 */
> +	ltq_w32(LTQ_WDT_PW2, ltq_wdt_membase + LTQ_WDT_CR);
> +}

Don't like this ifdef/ifndef stuff. The nowayout things can be done in the /dev/watchdog handling.

> +static long
> +ltq_wdt_ioctl(struct file *file,
> +		unsigned int cmd, unsigned long arg)
> +{
> +	int ret = -ENOTTY;
> +
> +	switch (cmd) {
> +	case WDIOC_GETSUPPORT:
> +		ret = copy_to_user((struct watchdog_info __user *)arg, &ident,
> +				sizeof(ident)) ? -EFAULT : 0;
> +		break;
> +
> +	case WDIOC_GETTIMEOUT:
> +		ret = put_user(ltq_wdt_timeout, (int __user *)arg);
> +		break;
> +
> +	case WDIOC_SETTIMEOUT:
> +		ret = get_user(ltq_wdt_timeout, (int __user *)arg);
> +		if (!ret)
> +			ltq_wdt_enable(ltq_wdt_timeout);
> +		break;
> +
> +	case WDIOC_KEEPALIVE:
> +		ltq_wdt_enable(ltq_wdt_timeout);
> +		ret = 0;
> +		break;
> +	}
> +	return ret;
> +}

Please add WDIOC_GETSTATUS and WDIOC_GETBOOTSTATUS iotcl calls.

> +static int
> +ltq_wdt_open(struct inode *inode, struct file *file)
> +{
> +	ltq_wdt_enable(ltq_wdt_timeout);
> +	return nonseekable_open(inode, file);
> +}
> +
> +static int
> +ltq_wdt_release(struct inode *inode, struct file *file)
> +{
> +#ifndef CONFIG_WATCHDOG_NOWAYOUT
> +	if (ltq_wdt_ok_to_close)
> +		ltq_wdt_disable();
> +	else
> +#endif
> +		pr_err("ltq_wdt: watchdog closed without warning\n");
> +	return 0;
> +}

Please add the code to make sure that /dev/watchdog can be opened once.
The rest I will look into at a later moment.

Kind regards,
Wim.




[Index of Archives]     [Linux MIPS Home]     [LKML Archive]     [Linux ARM Kernel]     [Linux ARM]     [Linux]     [Git]     [Yosemite News]     [Linux SCSI]     [Linux Hams]

  Powered by Linux