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

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

John Crispin wrote:

> This patch adds the driver for the watchdog found inside the Lantiq SoC family.

> Signed-off-by: John Crispin <blogic@openwrt.org>
> Signed-off-by: Ralph Hempel <ralph.hempel@lantiq.com>
> Cc: Wim Van Sebroeck <wim@iguana.be>
> Cc: linux-mips@linux-mips.org
> Cc: linux-watchdog@vger.kernel.org

> diff --git a/drivers/watchdog/lantiq_wdt.c b/drivers/watchdog/lantiq_wdt.c
> new file mode 100644
> index 0000000..0a78dfb
> --- /dev/null
> +++ b/drivers/watchdog/lantiq_wdt.c
> @@ -0,0 +1,217 @@
[...]
> +/* Section 3.4 of the datasheet
> + * The password sequence protects the WDT control register from unintended
> + * write actions, which might cause malfunction of the WDT.
> + *
> + * essentially the following two magic passwords need to be written to allow
> + * io access to the wdt core

    s/io/IO/, s/wdt/WDT. Be consistent. :-)

> +static void
> +ltq_wdt_enable(unsigned int timeout)

    This function is always called with 'ltw_wdt_timeout' as a parameter. Seems 
better to use it internally, and not pass it every time.

> +{
> +	timeout = ((timeout * (ltq_io_region_clk_rate / LTQ_WDT_DIVIDER))
> +		+ 0x1000);

    The parens around rvalue are not needed.

[...]
> +static ssize_t
> +ltq_wdt_write(struct file *file, const char __user *data,
> +		size_t len, loff_t *ppos)
> +{
> +	size_t i;
> +
> +	if (!len)
> +		return 0;
> +#ifndef CONFIG_WATCHDOG_NOWAYOUT

    Er, Documentation/CodingStyle asks not to use #ifdef inside the code. You 
could create a special function here...

> +	for (i = 0; i != len; i++) {
> +		char c;
> +
> +		if (get_user(c, data + i))
> +			return -EFAULT;
> +		if (c == 'V')
> +			ltq_wdt_ok_to_close = 1;
> +	}
> +#endif
> +	ltq_wdt_enable(ltq_wdt_timeout);
> +	return len;
> +}

[...]

> +static int __init
> +ltq_wdt_probe(struct platform_device *pdev)
> +{
> +	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	struct clk *clk;
> +
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot obtain I/O memory region");
> +		return -ENOENT;
> +	}
> +	res = devm_request_mem_region(&pdev->dev, res->start,
> +		resource_size(res), dev_name(&pdev->dev));
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot request I/O memory region");
> +		return -EBUSY;
> +	}
> +	ltq_wdt_membase = devm_ioremap_nocache(&pdev->dev, res->start,
> +		resource_size(res));
> +	if (!ltq_wdt_membase) {
> +		dev_err(&pdev->dev, "cannot remap I/O memory region\n");
> +		return -ENOMEM;
> +	}
> +	/* we do not need to enable the clock as it is always running */
> +	clk = clk_get(&pdev->dev, "io");
> +	BUG_ON(!clk);

    WARN_ON(). We shouldn't kill the whole machine I think.

> +static int __init
> +init_ltq_wdt(void)
> +{
> +	return platform_driver_probe(&ltq_wdt_driver, ltq_wdt_probe);
> +}
> +
> +module_init(init_ltq_wdt);

    How about module_exit()?

WBR, Sergei