[PATCH] watchdog: sirf: add watchdog driver of CSR SiRFprimaII and SiRFatlasVI

Hi Wim,

thanks for reviewing. xianglong, will you do some update according to
Wim's comments.

quoted

+config SIRFSOC_WATCHDOG
+     tristate "SiRFSOC watchdog"
+     depends on ARCH_SIRF

You are missing a:
        select WATCHDOG_CORE
here.

ok. it was done by changing defconfig:
+CONFIG_WATCHDOG=y
+CONFIG_WATCHDOG_CORE=y
but we can move to your solution.

quoted

+
+
+static unsigned int default_timeout = SIRFSOC_WDT_DEFAULT_TIMEOUT;
+module_param(default_timeout, uint, 0);
+MODULE_PARM_DESC(default_timeout, "Default watchdog timeout (in seconds)");

Preferred module parameter name is timeout .

ok.

quoted

+static unsigned int sirfsoc_wdt_gettimeleft(struct watchdog_device *wdd)
+{
+     u32 counter, match;
+     int time_left;
+
+     counter = readl(watchdog_get_drvdata(wdd) + SIRFSOC_TIMER_COUNTER_LO);
+     match = readl(watchdog_get_drvdata(wdd) +
+             SIRFSOC_TIMER_MATCH_0 + (SIRFSOC_TIMER_WDT_INDEX << 2));
+
+     if (match >= counter) {
+             time_left = match-counter;
+     } else {
+             /* rollover */
+             time_left = (0xffffffffUL - counter) + match;
+     }

should be:
        if (match >= counter)
                time_left = match-counter;
        else
                /* rollover */
                time_left = (0xffffffffUL - counter) + match;

according to coding style.

yes. need fix.

quoted

+
+     return time_left / CLOCK_TICK_RATE;
+}
+
+static int sirfsoc_wdt_updatetimeout(struct watchdog_device *wdd)
+{
+     u32 counter, timeout_ticks;
+
+     timeout_ticks = wdd->timeout * CLOCK_TICK_RATE;
+
+     /* Enable the latch before reading the LATCH_LO register */
+     writel(1, watchdog_get_drvdata(wdd) + SIRFSOC_TIMER_LATCH);
+
+     /* Set the TO value */
+     counter = readl(watchdog_get_drvdata(wdd) + SIRFSOC_TIMER_LATCHED_LO);
+
+     if ((0xffffffffUL - counter) >= timeout_ticks) {
+             counter += timeout_ticks;
+     } else {
+             /* Rollover */
+             counter = timeout_ticks - (0xffffffffUL - counter);
+     }

Same here (coding style).

agree.

quoted

+     writel(counter, watchdog_get_drvdata(wdd) +
+             SIRFSOC_TIMER_MATCH_0 + (SIRFSOC_TIMER_WDT_INDEX << 2));
+
+     return 0;
+}
+
+static int sirfsoc_wdt_enable(struct watchdog_device *wdd)
+{
+     sirfsoc_wdt_updatetimeout(wdd);
+
+     /*
+      * NOTE: If interrupt is not enabled
+      * then WD-Reset doesn't get generated at all.
+      */
+     writel(readl(watchdog_get_drvdata(wdd) + SIRFSOC_TIMER_INT_EN)
+             | (1 << SIRFSOC_TIMER_WDT_INDEX),
+             watchdog_get_drvdata(wdd) + SIRFSOC_TIMER_INT_EN);
+     writel(1, watchdog_get_drvdata(wdd) + SIRFSOC_TIMER_WATCHDOG_EN);

Wouldn't it be better to define a local variable first that get's the value
of watchdog_get_drvdata(wdd)? This would definitely improve readability.
Goes also for other functions (like sirfsoc_wdt_gettimeleft and
sirfsoc_wdt_disable).

ok

quoted

+
+static int sirfsoc_wdt_settimeout(struct watchdog_device *wdd, unsigned int to)
+{
+     if (to < SIRFSOC_WDT_MIN_TIMEOUT)
+             to = SIRFSOC_WDT_MIN_TIMEOUT;
+     if (to > SIRFSOC_WDT_MAX_TIMEOUT)
+             to = SIRFSOC_WDT_MAX_TIMEOUT;

Hmm, since SIRFSOC_WDT_MIN_TIMEOUT and SIRFSOC_WDT_MAX_TIMEOUT are defined:
setting the timeout will call watchdog_set_timeout and that function will do a:
if (watchdog_timeout_invalid(wddev, timeout))
                return -EINVAL;

with watchdog_timeout_invalid returning:
        ((wdd->max_timeout != 0) && (t < wdd->min_timeout || t > wdd->max_timeout));

So this will never happen...

so we will drop this.

quoted

+     wdd->timeout = to;
+     sirfsoc_wdt_updatetimeout(wdd);
+
+     return 0;
+}
+
+#define OPTIONS (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
+
+static const struct watchdog_info sirfsoc_wdt_ident = {
+     .options          =     OPTIONS,
+     .firmware_version =     0,
+     .identity         =     "SiRFSOC Watchdog",
+};
+
+static struct watchdog_ops sirfsoc_wdt_ops = {
+     .owner = THIS_MODULE,
+     .start = sirfsoc_wdt_enable,
+     .stop = sirfsoc_wdt_disable,
+     .get_timeleft = sirfsoc_wdt_gettimeleft,
+     .ping = sirfsoc_wdt_updatetimeout,
+     .set_timeout = sirfsoc_wdt_settimeout,
+};
+
+static struct watchdog_device sirfsoc_wdd = {
+     .info = &sirfsoc_wdt_ident,
+     .ops = &sirfsoc_wdt_ops,
+     .timeout = SIRFSOC_WDT_DEFAULT_TIMEOUT,
+     .min_timeout = SIRFSOC_WDT_MIN_TIMEOUT,
+     .max_timeout = SIRFSOC_WDT_MAX_TIMEOUT,
+};
+
+static int sirfsoc_wdt_probe(struct platform_device *pdev)
+{
+     struct resource *res;
+     int ret;
+     void __iomem *base;
+
+     /* reserve static register mappings */
+     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+     if (!res) {
+             dev_err(&pdev->dev, "sirfsoc wdt: could not get mem resources\n");
+             ret = -ENOMEM;
+             goto out;
+     }
+
+     base = devm_ioremap_resource(&pdev->dev, res);
+     if (!base) {
+             dev_err(&pdev->dev, "sirfsoc wdt: could not remap the mem\n");
+             ret = -EADDRNOTAVAIL;
+             goto out;
+     }
+     watchdog_set_drvdata(&sirfsoc_wdd, base);
+
+     sirfsoc_wdd.timeout = default_timeout;

You can consider using watchdog_init_timeout here.
I would also advice to add watchdog_set_nowayout and use the nowayout functionality.

agree.

quoted

+
+MODULE_DESCRIPTION("SiRF SoC watchdog driver");
+MODULE_AUTHOR("Xianglong Du [off-list ref]");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

This alias can be removed if you have the platform alias (like listed in the following line).

quoted

+MODULE_ALIAS("platform:sirfsoc-wdt");

agree.

quoted

--
1.7.4.1

Kind regards,
Wim.

-barry