On Sun, Mar 24, 2013 at 03:06:59PM +0100, Lubomir Rintel wrote: > On Fri, 2013-03-22 at 06:56 -0700, Guenter Roeck wrote: > > Thank you for your response! > > On Fri Mar 22 09:56:01 EDT 2013, Guenter Roeck wrote: > > On Fri, Mar 22, 2013 at 12:55:07PM -0000, Lubomir Rintel wrote: > ... > > > + writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) | > > > + PM_RSTC_WRCFG_FULL_RESET, wdt_regs + PM_RSTC); > > > + > > Nitpick - I prefer people to use the recommended continuation line style, > > but that is really up to the maintainer to decide. > > Well, I intended to comply with Documentation/CodingStyle, are you referring to > it? I fail to understand what to do to be more compliant and could not really > identify a style that would be consistently used across the kernel source. > Should I cut then second line into two smaller parts that would be aligned with > right line end? > I was referring to line continuation aligned with '(', such as writel_relaxed(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) | PM_RSTC_WRCFG_FULL_RESET, wdt_regs + PM_RSTC); I don't recall how deep the indentation was - does that not fit ? > ... > > > +static int bcm2835_wdt_set_timeout(struct watchdog_device *wdog, unsigned int t) > > > +{ > > > + wdog->timeout = t; > > > > No need to update the actual chip timeout ? > > No need to, watchdog core applies the new timeout by pinging the device (see > below for what happens when this driver is pinged). > > See: WDIOC_SETTIMEOUT in drivers/watchdog/watchdog_dev.c > Ok, makes sense. > ... > > > +static struct watchdog_ops bcm2835_wdt_ops = { > > > + .owner = THIS_MODULE, > > > + .start = bcm2835_wdt_start, > > > + .stop = bcm2835_wdt_stop, > > > + .set_timeout = bcm2835_wdt_set_timeout, > > > + .get_timeleft = bcm2835_wdt_get_timeleft, > > > > No separate ping function ? > > The watchdog documentation core states: > > "Most hardware that does not support this as a separate function uses the > start function to restart the watchdog timer hardware. And that's also what > the watchdog timer driver core does." > > This indeed applies to this driver. > Ok. > ... > > > + if (WARN(!wdt_regs, "failed to remap watchdog regs")) > > > + return -ENODEV; > > > > WARN seems to be a bit extreme. Is this necessary ? > > Probably not. I'll replace it with dev_err() instead. > > > > + dev_info(dev, "Broadcom BCM2835 watchdog timer"); > > > + > > > + watchdog_init_timeout(&bcm2835_wdt_wdd, heartbeat, dev); > > > > Since heartbeat is by default set to -1, which is interpreted as unsigned > > int, I would expect this call to return -EINVAL, leaving the default timeout > > undefined. Is this really what you want ? > > Well, I looked into orion-wdt for an example how to initialize the default > timeout, but failed to understand it correctly. I thought that watchdog core > picks a sensible value upon getting -1, which is incorrect. They in fact use > initialize timeout with maximal value, and use a fall-through vi EINVAL to leave > it untouched if it was not overridden. I'll do the same thing now. > Some user level documentation states that the default timeout for all drivers would be 60 seconds. Unfortunately, that is not correct, as all drivers do what they want. I myself use it as a guideline, ie use a 60 seconds default unless there is a good reason to use another default value (eg if the chip only supports a lower maximum). > > > + watchdog_set_nowayout(&bcm2835_wdt_wdd, nowayout); > > > + return watchdog_register_device(&bcm2835_wdt_wdd); > > > > Leaking iomap if this fails. > > Oops. Fixing. > > > Would be nice to have something like devm_of_iomap ... > > That sounds sound to me. Sent out a separate patch implementing it, and I'll > modify this if it gets merged. > Excellent! Thanks, Guenter -- 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