On 11/12/2013 02:17 PM, Markus Mayer wrote:

quoted

quoted

+
+     if (!timeout)
+             dev_info(wdog->dev, "Watchdog timer stopped");
+

All that noise.

Would it be acceptable to turn these calls into dev_dbg() calls, here
and elsewhere?

Ok with me.

quoted

quoted

+
+     wdt->resolution = SECWDOG_DEFAULT_RESOLUTION;
+     ret = bcm_kona_wdt_set_resolution_reg(wdt);
+     if (ret) {
+             dev_err(dev, "Failed to set resolution (error: %d)", ret);

ret can be -EAGAIN or -EINVAL. -EINVAL suggests a bad internale error (hopefully
SECWDOG_DEFAULT_RESOLUTION is defined to be smaller than SECWDOG_MAX_RES),
and if it is -EAGAIN there should be no error message.

Actually, bcm_kona_wdt_set_resolution_reg is only called from here, meaning the
error check in the function is really unnecessary.

This again goes back to making resolution available to userland. Then
bcm_kona_wdt_set_resolution_reg() would be called from elsewhere. Why
is it bad to print an error message on timeout? Would this still apply

That was related to -EAGAIN. Which would be bad here anyway as it could result
in an endless loop if there is a problem with the chip.

if I switch the code to -ETIMEDOUT?

That is one option, or -EIO if the condition indicates a chip error.

quoted

quoted

+             return ret;
+     }
+
+     spin_lock_init(&wdt->lock);
+     platform_set_drvdata(pdev, wdt);
+     watchdog_set_drvdata(&bcm_kona_wdt_wdd, wdt);
+
+     ret = bcm_kona_wdt_set_timeout_reg(&bcm_kona_wdt_wdd);
+     if (ret) {
+             dev_err(dev, "Failed set watchdog timeout");

The only error case is -EAGAIN. I don't think there should be an error mesasge
in this case (though I am not sure what the reaction should be).

I am thinking that probe() needs to return an error if setting the
timeout fails, as it can't really rely on the watchdog timer or let
the system use it. Shouldn't that be accompanied by an error message
letting the user know what happened?

Oh, I agree it should return an error, and an error message is ok as well.
I am just sure it should not be -EAGAIN, but I don't know what it should be.
Maybe -ETIMEDOUT, or -EIO.

quoted

quoted

+             return ret;
+     }
+
+     ret = watchdog_register_device(&bcm_kona_wdt_wdd);
+     if (ret) {
+             dev_err(dev, "Failed to register watchdog device");
+             return ret;
+     }
+
+#ifdef CONFIG_BCM_KONA_WDT_DEBUG
+     wdt->debugfs = bcm_kona_wdt_debugfs_init(wdt, &bcm_kona_wdt_wdd);
+#endif
+     dev_info(dev, "Broadcom Kona Watchdog Timer");
+

Such messages are in general considered nuisance nowadays. I would suggest to
remove it (or ask Greg KH for advice).

Referring to your other mail.... seems those messages are falling out of favor.
I consider it a nuisance, though so far I let it go through. The messages do
increase boot time, especially on systems with slow serial console, and IMO
do not provide any real value. Users either don't care, or can check if the
driver is loaded by other means. I would suggest to at least make it dev_dbg.

Thanks,
Guenter