From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Wed, 02 Mar 2011 12:23:53 +0100 (CET) Received: from mail-bw0-f49.google.com ([209.85.214.49]:56005 "EHLO mail-bw0-f49.google.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S1491054Ab1CBLXt (ORCPT ); Wed, 2 Mar 2011 12:23:49 +0100 Received: by bwz1 with SMTP id 1so51926bwz.36 for ; Wed, 02 Mar 2011 03:23:42 -0800 (PST) Received: by 10.204.22.16 with SMTP id l16mr7249947bkb.198.1299065021994; Wed, 02 Mar 2011 03:23:41 -0800 (PST) Received: from [192.168.2.2] ([91.79.108.232]) by mx.google.com with ESMTPS id b16sm4073689bkw.2.2011.03.02.03.23.39 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 02 Mar 2011 03:23:40 -0800 (PST) Message-ID: <4D6E286D.9050100@mvista.com> Date: Wed, 02 Mar 2011 14:22:21 +0300 From: Sergei Shtylyov User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.13) Gecko/20101207 Thunderbird/3.1.7 MIME-Version: 1.0 To: John Crispin CC: Ralf Baechle , Ralph Hempel , Wim Van Sebroeck , linux-mips@linux-mips.org, linux-watchdog@vger.kernel.org Subject: Re: [PATCH V2 05/10] MIPS: lantiq: add watchdog support References: <1298996006-15960-1-git-send-email-blogic@openwrt.org> <1298996006-15960-6-git-send-email-blogic@openwrt.org> In-Reply-To: <1298996006-15960-6-git-send-email-blogic@openwrt.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 29313 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: sshtylyov@mvista.com Precedence: bulk X-list: linux-mips Hello. On 01-03-2011 19:13, John Crispin wrote: > This patch adds the driver for the watchdog found inside the Lantiq SoC family. > Changes in V2 > * add comments to explain register access > * cleanup resource allocation > * cleanup clock handling > * whitespace fixes The patch revision history is normally put under --- tearline. > Signed-off-by: John Crispin > Signed-off-by: Ralph Hempel > Cc: Wim Van Sebroeck > 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..8515c1f > --- /dev/null > +++ b/drivers/watchdog/lantiq_wdt.c > @@ -0,0 +1,233 @@ > +/* > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published > + * by the Free Software Foundation. > + * > + * Copyright (C) 2010 John Crispin > + * Based on EP93xx wdt driver > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +/* 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 */ The preferred style for the multi-line comments is this: /* * bla * bla */ > +#define LTQ_WDT_CR 0x03F0 /* watchdog control register */ > +#define LTQ_WDT_SR 0x03F8 /* watchdog status register */ > + > +#define LTQ_WDT_SR_EN (0x1 << 31) /* enable bit */ > +#define LTQ_WDT_SR_PWD (0x3 << 26) /* turn on power */ > +#define LTQ_WDT_SR_CLKDIV (0x3 << 24) /* turn on clock and set */ > + /* divider to 0x40000 */ > +#define LTQ_WDT_DIVIDER 0x40000 > +#define LTQ_MAX_TIMEOUT ((1 << 16) - 1) /* the reload field is 16 bit */ Should align like the above... > + > +#ifndef CONFIG_WATCHDOG_NOWAYOUT > +static int ltq_wdt_ok_to_close; > +#endif > + > +static int ltq_wdt_timeout = 30; > +static __iomem void *ltq_wdt_membase; It's normally "void __iomem *". > +static unsigned long ltq_io_region_clk_rate; > + > +static int > +ltq_wdt_enable(unsigned int timeout) > +{ > + timeout = ((timeout * (ltq_io_region_clk_rate / LTQ_WDT_DIVIDER)) > + + 0x1000); > + if (timeout > LTQ_MAX_TIMEOUT) > + timeout = LTQ_MAX_TIMEOUT; > + > + /* write the first paswword magic */ s/paswword/password/. > + ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR); > + /* write the second magic plus the configuration and new timeout */ > + ltq_w32(LTQ_WDT_SR_EN | LTQ_WDT_SR_PWD | LTQ_WDT_SR_CLKDIV | > + LTQ_WDT_PW2 | timeout, ltq_wdt_membase + LTQ_WDT_CR); > + return 0; This function should be *void* -- you alway return 0 and never check the result. > +} > + > +static void > +ltq_wdt_disable(void) > +{ > +#ifndef CONFIG_WATCHDOG_NOWAYOUT > + ltq_wdt_ok_to_close = 0; > +#endif > + /* write the first paswword magic */ > + ltq_w32(LTQ_WDT_PW1, ltq_wdt_membase + LTQ_WDT_CR); > + /* write the second paswword magic with no config > + this turns the watchdog off */ Multi-line comment style... > + ltq_w32(LTQ_WDT_PW2, ltq_wdt_membase + LTQ_WDT_CR); > +} > + > +static ssize_t > +ltq_wdt_write(struct file *file, const char __user *data, > + size_t len, loff_t *ppos) > +{ > + size_t i; Empty line between the variables and code won't hurt... > + if (!len) > + return 0; > +#ifndef CONFIG_WATCHDOG_NOWAYOUT > + for (i = 0; i != len; i++) { > + char c; Here too... > + if (get_user(c, data + i)) > + return -EFAULT; > + if (c == 'V') > + ltq_wdt_ok_to_close = 1; > + } > +#endif [...] > +static long > +ltq_wdt_ioctl(struct file *file, > + unsigned int cmd, unsigned long arg) > +{ > + int ret = -ENOTTY; Empty line between the variables and code won't hurt... > + switch (cmd) { > + case WDIOC_GETSUPPORT: > + ret = copy_to_user((struct watchdog_info __user *)arg, &ident, > + sizeof(ident)) ? -EFAULT : 0; Doesn't copy_to_user() return 0 or -EFAULT? > +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 > + printk(KERN_ERR "ltq_wdt: watchdog closed without warning\n"); Use pr_err() instead. > +static int > +ltq_wdt_probe(struct platform_device *pdev) > +{ > + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + struct clk *clk; > + int ret = 0; Empty line between the variables and code won't hurt... > + /* we do not need to enable the clock as it is always running */ > + clk = clk_get(&pdev->dev, "io"); clk_get() may fail... > + ret = misc_register(<q_wdt_miscdev); > + if (ret) > + return 0; Er, didn't you mean: if (!ret) return 0; > + return ret; 'ret' is always 0 here with your code. > +} > + > +static int > +ltq_wdt_remove(struct platform_device *dev) __exit? > +static int __init > +init_ltq_wdt(void) > +{ > + return platform_driver_register(<q_wdt_driver); Why not platfrom_driver_probe()? It's hardly a hotplug device... WBR, Sergei