Hi Wim,
thanks for reviewing. xianglong, will you do some update according to
Wim's comments.
> >
> > +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.
> > +
> > +
> > +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.
> > +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.
>
> > +
> > + 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.
>
> > + 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
> > +
> > +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.
>
>
> > + 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.
> > +
> > +MODULE_DESCRIPTION("SiRF SoC watchdog driver");
> > +MODULE_AUTHOR("Xianglong Du <Xianglong.Du@xxxxxxx>");
> > +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).
>
> > +MODULE_ALIAS("platform:sirfsoc-wdt");
agree.
> > --
> > 1.7.4.1
>
> Kind regards,
> Wim.
>
-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
