On 11/16/2013 10:29 PM, Guenter Roeck wrote: > On 11/06/2013 03:31 AM, ivan.khoronzhuk wrote: >> To reduce code duplicate and increase code readability use WDT core >> code to handle WDT interface. >> >> Remove io_lock as the WDT core uses mutex to lock each wdt device. >> Remove wdt_state as the WDT core track state with its own variable. >> >> The watchdog_init_timeout() can read timeout value from timeout-sec >> property if the passed value is out of bounds. So set initial >> heartbeat value more than max value in order to initialize heartbeat >> in next way. If heartbeat is not set thought module parameter, try >> to read it's value from WDT node timeout-sec property. If node has >> no one, use default value. >> >> The heartbeat is hold in wdd->timeout by WDT core, so use it in >> order to set timeout period. >> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@xxxxxx> >> --- >> drivers/watchdog/Kconfig | 1 + >> drivers/watchdog/davinci_wdt.c | 150 >> ++++++++++------------------------------ >> 2 files changed, 38 insertions(+), 113 deletions(-) >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index d1d53f3..2c954b5 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -271,6 +271,7 @@ config IOP_WATCHDOG >> config DAVINCI_WATCHDOG >> tristate "DaVinci watchdog" >> depends on ARCH_DAVINCI >> + select WATCHDOG_CORE >> help >> Say Y here if to include support for the watchdog timer >> in the DaVinci DM644x/DM646x processors. >> diff --git a/drivers/watchdog/davinci_wdt.c >> b/drivers/watchdog/davinci_wdt.c >> index bead774..a6eef71 100644 >> --- a/drivers/watchdog/davinci_wdt.c >> +++ b/drivers/watchdog/davinci_wdt.c >> @@ -3,7 +3,7 @@ >> * >> * Watchdog driver for DaVinci DM644x/DM646x processors >> * >> - * Copyright (C) 2006 Texas Instruments. >> + * Copyright (C) 2013 Texas Instruments. > > 2006-2013 > Ok >> * >> * 2007 (c) MontaVista Software, Inc. This file is licensed under >> * the terms of the GNU General Public License version 2. This program >> @@ -15,18 +15,12 @@ >> #include <linux/moduleparam.h> >> #include <linux/types.h> >> #include <linux/kernel.h> >> -#include <linux/fs.h> >> -#include <linux/miscdevice.h> >> #include <linux/watchdog.h> >> #include <linux/init.h> >> -#include <linux/bitops.h> >> #include <linux/platform_device.h> >> -#include <linux/spinlock.h> >> -#include <linux/uaccess.h> >> #include <linux/io.h> >> #include <linux/device.h> >> #include <linux/clk.h> >> -#include <linux/slab.h> >> #include <linux/err.h> >> >> #define MODULE_NAME "DAVINCI-WDT: " >> @@ -61,31 +55,13 @@ >> #define WDKEY_SEQ0 (0xa5c6 << 16) >> #define WDKEY_SEQ1 (0xda7e << 16) >> >> -static int heartbeat = DEFAULT_HEARTBEAT; >> - >> -static DEFINE_SPINLOCK(io_lock); >> -static unsigned long wdt_status; >> -#define WDT_IN_USE 0 >> -#define WDT_OK_TO_CLOSE 1 >> -#define WDT_REGION_INITED 2 >> -#define WDT_DEVICE_INITED 3 >> - >> +static int heartbeat = MAX_HEARTBEAT + 1; > > Initializing it with 0 (ie not at all) would be just as good. Also see > below. > Ok >> static void __iomem *wdt_base; >> struct clk *wdt_clk; >> +static struct watchdog_device wdt_wdd; >> >> -static void wdt_service(void) >> -{ >> - spin_lock(&io_lock); >> - >> - /* put watchdog in service state */ >> - iowrite32(WDKEY_SEQ0, wdt_base + WDTCR); >> - /* put watchdog in active state */ >> - iowrite32(WDKEY_SEQ1, wdt_base + WDTCR); >> - >> - spin_unlock(&io_lock); >> -} >> - >> -static void wdt_enable(void) >> +/* davinci_wdt_start - enable watchdog */ > > That comment doesn't really provide much value. > >> +static int davinci_wdt_start(struct watchdog_device *wdd) >> { >> u32 tgcr; >> u32 timer_margin; >> @@ -93,8 +69,6 @@ static void wdt_enable(void) >> >> wdt_freq = clk_get_rate(wdt_clk); >> >> - spin_lock(&io_lock); >> - >> /* disable, internal clock source */ >> iowrite32(0, wdt_base + TCR); >> /* reset timer, set mode to 64-bit watchdog, and unreset */ >> @@ -105,9 +79,9 @@ static void wdt_enable(void) >> iowrite32(0, wdt_base + TIM12); >> iowrite32(0, wdt_base + TIM34); >> /* set timeout period */ >> - timer_margin = (((u64)heartbeat * wdt_freq) & 0xffffffff); >> + timer_margin = (((u64)wdd->timeout * wdt_freq) & 0xffffffff); >> iowrite32(timer_margin, wdt_base + PRD12); >> - timer_margin = (((u64)heartbeat * wdt_freq) >> 32); >> + timer_margin = (((u64)wdd->timeout * wdt_freq) >> 32); >> iowrite32(timer_margin, wdt_base + PRD34); >> /* enable run continuously */ >> iowrite32(ENAMODE12_PERIODIC, wdt_base + TCR); >> @@ -119,84 +93,28 @@ static void wdt_enable(void) >> iowrite32(WDKEY_SEQ0 | WDEN, wdt_base + WDTCR); >> /* put watchdog in active state */ >> iowrite32(WDKEY_SEQ1 | WDEN, wdt_base + WDTCR); >> - >> - spin_unlock(&io_lock); >> -} >> - >> -static int davinci_wdt_open(struct inode *inode, struct file *file) >> -{ >> - if (test_and_set_bit(WDT_IN_USE, &wdt_status)) >> - return -EBUSY; >> - >> - wdt_enable(); >> - >> - return nonseekable_open(inode, file); >> + return 0; >> } >> >> -static ssize_t >> -davinci_wdt_write(struct file *file, const char *data, size_t len, >> - loff_t *ppos) >> +static int davinci_wdt_ping(struct watchdog_device *wdd) >> { >> - if (len) >> - wdt_service(); >> - >> - return len; >> + /* put watchdog in service state */ >> + iowrite32(WDKEY_SEQ0, wdt_base + WDTCR); >> + /* put watchdog in active state */ >> + iowrite32(WDKEY_SEQ1, wdt_base + WDTCR); >> + return 0; >> } >> >> -static const struct watchdog_info ident = { >> +static const struct watchdog_info davinci_wdt_info = { >> .options = WDIOF_KEEPALIVEPING, >> .identity = "DaVinci Watchdog", >> }; >> >> -static long davinci_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 *)arg, &ident, >> - sizeof(ident)) ? -EFAULT : 0; >> - break; >> - >> - case WDIOC_GETSTATUS: >> - case WDIOC_GETBOOTSTATUS: >> - ret = put_user(0, (int *)arg); >> - break; >> - >> - case WDIOC_KEEPALIVE: >> - wdt_service(); >> - ret = 0; >> - break; >> - >> - case WDIOC_GETTIMEOUT: >> - ret = put_user(heartbeat, (int *)arg); >> - break; >> - } >> - return ret; >> -} >> - >> -static int davinci_wdt_release(struct inode *inode, struct file *file) >> -{ >> - wdt_service(); >> - clear_bit(WDT_IN_USE, &wdt_status); >> - >> - return 0; >> -} >> - >> -static const struct file_operations davinci_wdt_fops = { >> - .owner = THIS_MODULE, >> - .llseek = no_llseek, >> - .write = davinci_wdt_write, >> - .unlocked_ioctl = davinci_wdt_ioctl, >> - .open = davinci_wdt_open, >> - .release = davinci_wdt_release, >> -}; >> - >> -static struct miscdevice davinci_wdt_miscdev = { >> - .minor = WATCHDOG_MINOR, >> - .name = "watchdog", >> - .fops = &davinci_wdt_fops, >> +static const struct watchdog_ops davinci_wdt_ops = { >> + .owner = THIS_MODULE, >> + .start = davinci_wdt_start, >> + .stop = davinci_wdt_ping, >> + .ping = davinci_wdt_ping, >> }; >> >> static int davinci_wdt_probe(struct platform_device *pdev) >> @@ -204,6 +122,7 @@ static int davinci_wdt_probe(struct >> platform_device *pdev) >> int ret = 0; >> struct device *dev = &pdev->dev; >> struct resource *wdt_mem; >> + struct watchdog_device *wdd; >> >> wdt_clk = devm_clk_get(dev, NULL); >> if (WARN_ON(IS_ERR(wdt_clk))) >> @@ -211,29 +130,35 @@ static int davinci_wdt_probe(struct >> platform_device *pdev) >> >> clk_prepare_enable(wdt_clk); >> >> - if (heartbeat < 1 || heartbeat > MAX_HEARTBEAT) >> - heartbeat = DEFAULT_HEARTBEAT; >> + wdd = &wdt_wdd; >> + wdd->info = &davinci_wdt_info; >> + wdd->ops = &davinci_wdt_ops; >> + wdd->min_timeout = 1; >> + wdd->max_timeout = MAX_HEARTBEAT; >> + wdd->timeout = DEFAULT_HEARTBEAT; >> + >> + if (heartbeat) >> + watchdog_init_timeout(wdd, heartbeat, dev); >> + > The if statement here is unnecessary. Actually, you always want to call > watchdog_init_timeout() > to ensure that it gets the configuration from fdt if available. > Ok >> + dev_info(dev, "heartbeat %d sec\n", wdd->timeout); >> >> - dev_info(dev, "heartbeat %d sec\n", heartbeat); >> + watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT); >> >> wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> wdt_base = devm_ioremap_resource(dev, wdt_mem); >> if (IS_ERR(wdt_base)) >> return PTR_ERR(wdt_base); >> >> - ret = misc_register(&davinci_wdt_miscdev); >> - if (ret < 0) { >> - dev_err(dev, "cannot register misc device\n"); >> - } else { >> - set_bit(WDT_DEVICE_INITED, &wdt_status); >> - } >> + ret = watchdog_register_device(wdd); >> + if (ret < 0) >> + dev_err(dev, "cannot register watchdog device\n"); >> >> return ret; >> } >> >> static int davinci_wdt_remove(struct platform_device *pdev) >> { >> - misc_deregister(&davinci_wdt_miscdev); >> + watchdog_unregister_device(&wdt_wdd); >> clk_disable_unprepare(wdt_clk); >> >> return 0; >> @@ -267,5 +192,4 @@ MODULE_PARM_DESC(heartbeat, >> __MODULE_STRING(DEFAULT_HEARTBEAT)); >> >> MODULE_LICENSE("GPL"); >> -MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR); >> MODULE_ALIAS("platform:watchdog"); >> > You might want to rename the platform driver to something like > davinci-wdt and change the MODULE_ALIAS > accordingly. > Yes, it could be changed but not in this patch. > Guenter > > -- Regards, Ivan Khoronzhuk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html