[PATCH 00/16] watchdog: refactor init_timeout and update users

[PATCH 00/16] watchdog: refactor init_timeout and update users

[Wolfram Sang]

When trying to add DT support to the DA9063 WDT, I didn't want to add yet
another error message when watchdog_init_timeout fails. The core could do this
with more detail, and also much more consistent. So, I refactored this routine
and removed error strings from its callers. Note that DA9063 support is not
added here because I still need to think about another issue. But this series
has been tested using the renesas_wdt driver on a Renesas Salvator-XS board
(R-Car M3N) and build bot is happy, too.

Looking forward to comments.

Thanks,

   Wolfram

Wolfram Sang (16):
  watchdog: refactor watchdog_init_timeout
  watchdog: add error messages when initializing timeout fails
  watchdog: cadence_wdt: drop warning after calling
    watchdog_init_timeout
  watchdog: cadence_wdt: still probe if user supplied timeout is invalid
  watchdog: ebc-c384_wdt: drop warning after calling
    watchdog_init_timeout
  watchdog: hpwdt: drop warning after calling watchdog_init_timeout
  watchdog: i6300esb: drop warning after calling watchdog_init_timeout
  watchdog: imx_sc_wdt: drop warning after calling watchdog_init_timeout
  watchdog: ni903x_wdt: drop warning after calling watchdog_init_timeout
  watchdog: nic7018_wdt: drop warning after calling
    watchdog_init_timeout
  watchdog: renesas_wdt: drop warning after calling
    watchdog_init_timeout
  watchdog: sp5100_tco: drop warning after calling watchdog_init_timeout
  watchdog: st_lpc_wdt: drop warning after calling watchdog_init_timeout
  watchdog: stm32_iwdg: drop warning after calling watchdog_init_timeout
  watchdog: xen_wdt: drop warning after calling watchdog_init_timeout
  watchdog: ziirave_wdt: drop warning after calling
    watchdog_init_timeout

 drivers/watchdog/cadence_wdt.c   |  7 +------
 drivers/watchdog/ebc-c384_wdt.c  |  5 +----
 drivers/watchdog/hpwdt.c         |  3 +--
 drivers/watchdog/i6300esb.c      |  5 +----
 drivers/watchdog/imx_sc_wdt.c    |  5 +----
 drivers/watchdog/ni903x_wdt.c    |  4 +---
 drivers/watchdog/nic7018_wdt.c   |  5 +----
 drivers/watchdog/renesas_wdt.c   |  4 +---
 drivers/watchdog/sp5100_tco.c    |  4 +---
 drivers/watchdog/st_lpc_wdt.c    |  4 +---
 drivers/watchdog/stm32_iwdg.c    |  6 +-----
 drivers/watchdog/watchdog_core.c | 39 ++++++++++++++++++++++++---------------
 drivers/watchdog/xen_wdt.c       |  4 +---
 drivers/watchdog/ziirave_wdt.c   |  6 +-----
 14 files changed, 37 insertions(+), 64 deletions(-)

-- 
2.11.0

[PATCH 01/16] watchdog: refactor watchdog_init_timeout

[Wolfram Sang]

The function is not easy to read and has two problems: a) -EINVAL is
returned when the module parameter is invalid but the DT parameter is
OK, and b) for the module parameter, zero is a valid value but for DT it
is invalid.

Refactor the code to have a the same pattern of checks for the module
parameter and DT. Further ones can be easily added in the future if the
need arises. The above mentioned problems are fixed, too.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/watchdog_core.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index eb8fa25f8eb2..85c136acc0e9 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -104,10 +104,11 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
  * Initialize the timeout field of the watchdog_device struct with either the
  * timeout module parameter (if it is valid value) or the timeout-sec property
  * (only if it is a valid value and the timeout_parm is out of bounds).
- * If none of them are valid then we keep the old value (which should normally
- * be the default timeout value).
+ * If none of them are valid or all of them are zero ("don't care") then we keep
+ * the old value (which should normally be the default timeout value).
  *
- * A zero is returned on success and -EINVAL for failure.
+ * A zero is returned on success or -EINVAL if all provided values are out of
+ * bounds.
  */
 int watchdog_init_timeout(struct watchdog_device *wdd,
 				unsigned int timeout_parm, struct device *dev)
@@ -117,22 +118,24 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 
 	watchdog_check_min_max_timeout(wdd);
 
-	/* try to get the timeout module parameter first */
-	if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
-		wdd->timeout = timeout_parm;
-		return ret;
-	}
-	if (timeout_parm)
+	/* check the driver supplied value (likely a module parameter) first */
+	if (timeout_parm) {
+		if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
+			wdd->timeout = timeout_parm;
+			return 0;
+		}
 		ret = -EINVAL;
+	}
 
 	/* try to get the timeout_sec property */
-	if (dev == NULL || dev->of_node == NULL)
-		return ret;
-	of_property_read_u32(dev->of_node, "timeout-sec", &t);
-	if (!watchdog_timeout_invalid(wdd, t) && t)
-		wdd->timeout = t;
-	else
+	if (dev && dev->of_node &&
+	    of_property_read_u32(dev->of_node, "timeout-sec", &t) == 0 && t) {
+		if (!watchdog_timeout_invalid(wdd, t)) {
+			wdd->timeout = t;
+			return 0;
+		}
 		ret = -EINVAL;
+	}
 
 	return ret;
 }
-- 
2.11.0

[PATCH 02/16] watchdog: add error messages when initializing timeout fails

[Wolfram Sang]

This not only removes boilerplate code from watchdog drivers, it can
also be more specific which of the supplied value actually fails. Also,
the loglevel becomes now consistent across drivers.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/watchdog_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 85c136acc0e9..407a5ca121d7 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -124,6 +124,8 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 			wdd->timeout = timeout_parm;
 			return 0;
 		}
+		dev_err(dev, "driver supplied timeout (%u) out of range\n",
+			timeout_parm);
 		ret = -EINVAL;
 	}
 
@@ -134,9 +136,13 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
 			wdd->timeout = t;
 			return 0;
 		}
+		dev_err(dev, "DT supplied timeout (%u) out of range\n", t);
 		ret = -EINVAL;
 	}
 
+	if (ret < 0 && wdd->timeout)
+		dev_warn(dev, "falling back to default timeout (%u)\n", wdd->timeout);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(watchdog_init_timeout);
-- 
2.11.0

[PATCH 03/16] watchdog: cadence_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/cadence_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index c5051907df00..e6eaeaf3dfb1 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -329,10 +329,8 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	cdns_wdt_device->parent = dev;
 
 	ret = watchdog_init_timeout(cdns_wdt_device, wdt_timeout, dev);
-	if (ret) {
-		dev_err(dev, "unable to set timeout value\n");
+	if (ret)
 		return ret;
-	}
 
 	watchdog_set_nowayout(cdns_wdt_device, nowayout);
 	watchdog_stop_on_reboot(cdns_wdt_device);
-- 
2.11.0

[PATCH 04/16] watchdog: cadence_wdt: still probe if user supplied timeout is invalid

[Wolfram Sang]

We have a default timeout value in the driver which we will fall back to
if the user supplied values are out of bounce.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/cadence_wdt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/cadence_wdt.c b/drivers/watchdog/cadence_wdt.c
index e6eaeaf3dfb1..a22f2d431a35 100644
--- a/drivers/watchdog/cadence_wdt.c
+++ b/drivers/watchdog/cadence_wdt.c
@@ -328,10 +328,7 @@ static int cdns_wdt_probe(struct platform_device *pdev)
 	/* Initialize the members of cdns_wdt structure */
 	cdns_wdt_device->parent = dev;
 
-	ret = watchdog_init_timeout(cdns_wdt_device, wdt_timeout, dev);
-	if (ret)
-		return ret;
-
+	watchdog_init_timeout(cdns_wdt_device, wdt_timeout, dev);
 	watchdog_set_nowayout(cdns_wdt_device, nowayout);
 	watchdog_stop_on_reboot(cdns_wdt_device);
 	watchdog_set_drvdata(cdns_wdt_device, wdt);
-- 
2.11.0

[PATCH 05/16] watchdog: ebc-c384_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/ebc-c384_wdt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/ebc-c384_wdt.c b/drivers/watchdog/ebc-c384_wdt.c
index 4c4c8ce78021..c176f59fea28 100644
--- a/drivers/watchdog/ebc-c384_wdt.c
+++ b/drivers/watchdog/ebc-c384_wdt.c
@@ -117,10 +117,7 @@ static int ebc_c384_wdt_probe(struct device *dev, unsigned int id)
 	wdd->max_timeout = WATCHDOG_MAX_TIMEOUT;
 
 	watchdog_set_nowayout(wdd, nowayout);
-
-	if (watchdog_init_timeout(wdd, timeout, dev))
-		dev_warn(dev, "Invalid timeout (%u seconds), using default (%u seconds)\n",
-			timeout, WATCHDOG_TIMEOUT);
+	watchdog_init_timeout(wdd, timeout, dev);
 
 	return devm_watchdog_register_device(dev, wdd);
 }
-- 
2.11.0

[PATCH 06/16] watchdog: hpwdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/hpwdt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index ef30c7e9728d..db1bf6f546ae 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -311,8 +311,7 @@ static int hpwdt_init_one(struct pci_dev *dev,
 		goto error_init_nmi_decoding;
 
 	watchdog_set_nowayout(&hpwdt_dev, nowayout);
-	if (watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL))
-		dev_warn(&dev->dev, "Invalid soft_margin: %d.\n", soft_margin);
+	watchdog_init_timeout(&hpwdt_dev, soft_margin, NULL);
 
 	if (pretimeout && hpwdt_dev.timeout <= PRETIMEOUT_SEC) {
 		dev_warn(&dev->dev, "timeout <= pretimeout. Setting pretimeout to zero\n");
-- 
2.11.0

[PATCH 07/16] watchdog: i6300esb: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/i6300esb.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/i6300esb.c b/drivers/watchdog/i6300esb.c
index e312a2aeecad..17941c03996b 100644
--- a/drivers/watchdog/i6300esb.c
+++ b/drivers/watchdog/i6300esb.c
@@ -311,10 +311,7 @@ static int esb_probe(struct pci_dev *pdev,
 	edev->wdd.min_timeout = ESB_HEARTBEAT_MIN;
 	edev->wdd.max_timeout = ESB_HEARTBEAT_MAX;
 	edev->wdd.timeout = ESB_HEARTBEAT_DEFAULT;
-	if (watchdog_init_timeout(&edev->wdd, heartbeat, NULL))
-		dev_info(&pdev->dev,
-			"heartbeat value must be " ESB_HEARTBEAT_RANGE
-			", using %u\n", edev->wdd.timeout);
+	watchdog_init_timeout(&edev->wdd, heartbeat, NULL);
 	watchdog_set_nowayout(&edev->wdd, nowayout);
 	watchdog_stop_on_reboot(&edev->wdd);
 	watchdog_stop_on_unregister(&edev->wdd);
-- 
2.11.0

[PATCH 08/16] watchdog: imx_sc_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/imx_sc_wdt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
index 86c2722f2a09..6dc24ceb1b2c 100644
--- a/drivers/watchdog/imx_sc_wdt.c
+++ b/drivers/watchdog/imx_sc_wdt.c
@@ -117,10 +117,7 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
 	imx_sc_wdd->parent = &pdev->dev;
 	imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
 
-	ret = watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev);
-	if (ret)
-		dev_warn(&pdev->dev, "Failed to set timeout value, using default\n");
-
+	watchdog_init_timeout(imx_sc_wdd, 0, &pdev->dev);
 	watchdog_stop_on_reboot(imx_sc_wdd);
 	watchdog_stop_on_unregister(imx_sc_wdd);
 
-- 
2.11.0

[PATCH 09/16] watchdog: ni903x_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/ni903x_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/ni903x_wdt.c b/drivers/watchdog/ni903x_wdt.c
index dc67742e9018..fbc1df86c6cc 100644
--- a/drivers/watchdog/ni903x_wdt.c
+++ b/drivers/watchdog/ni903x_wdt.c
@@ -217,9 +217,7 @@ static int ni903x_acpi_add(struct acpi_device *device)
 	wdd->parent = dev;
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, nowayout);
-	ret = watchdog_init_timeout(wdd, timeout, dev);
-	if (ret)
-		dev_err(dev, "unable to set timeout value, using default\n");
+	watchdog_init_timeout(wdd, timeout, dev);
 
 	ret = watchdog_register_device(wdd);
 	if (ret) {
-- 
2.11.0

[PATCH 10/16] watchdog: nic7018_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/nic7018_wdt.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/watchdog/nic7018_wdt.c b/drivers/watchdog/nic7018_wdt.c
index dcd265685837..82843abe38f8 100644
--- a/drivers/watchdog/nic7018_wdt.c
+++ b/drivers/watchdog/nic7018_wdt.c
@@ -211,10 +211,7 @@ static int nic7018_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, nowayout);
-
-	ret = watchdog_init_timeout(wdd, timeout, dev);
-	if (ret)
-		dev_warn(dev, "unable to set timeout value, using default\n");
+	watchdog_init_timeout(wdd, timeout, dev);
 
 	/* Unlock WDT register */
 	outb(UNLOCK, wdt->io_base + WDT_REG_LOCK);
-- 
2.11.0

[PATCH 11/16] watchdog: renesas_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/renesas_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/renesas_wdt.c b/drivers/watchdog/renesas_wdt.c
index 5b3186492087..94cd8830ba9a 100644
--- a/drivers/watchdog/renesas_wdt.c
+++ b/drivers/watchdog/renesas_wdt.c
@@ -236,9 +236,7 @@ static int rwdt_probe(struct platform_device *pdev)
 	watchdog_stop_on_unregister(&priv->wdev);
 
 	/* This overrides the default timeout only if DT configuration was found */
-	ret = watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
-	if (ret)
-		dev_warn(&pdev->dev, "Specified timeout value invalid, using default\n");
+	watchdog_init_timeout(&priv->wdev, 0, &pdev->dev);
 
 	ret = watchdog_register_device(&priv->wdev);
 	if (ret < 0)
-- 
2.11.0

[PATCH 12/16] watchdog: sp5100_tco: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/sp5100_tco.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 41aaae2d5287..553735b256e2 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -395,9 +395,7 @@ static int sp5100_tco_probe(struct platform_device *pdev)
 	wdd->min_timeout = 1;
 	wdd->max_timeout = 0xffff;
 
-	if (watchdog_init_timeout(wdd, heartbeat, NULL))
-		dev_info(dev, "timeout value invalid, using %d\n",
-			 wdd->timeout);
+	watchdog_init_timeout(wdd, heartbeat, NULL);
 	watchdog_set_nowayout(wdd, nowayout);
 	watchdog_stop_on_reboot(wdd);
 	watchdog_stop_on_unregister(wdd);
-- 
2.11.0

[PATCH 13/16] watchdog: st_lpc_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/st_lpc_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/st_lpc_wdt.c b/drivers/watchdog/st_lpc_wdt.c
index 9a5ed95c3403..7a90184eb950 100644
--- a/drivers/watchdog/st_lpc_wdt.c
+++ b/drivers/watchdog/st_lpc_wdt.c
@@ -224,10 +224,8 @@ static int st_wdog_probe(struct platform_device *pdev)
 
 	/* Init Watchdog timeout with value in DT */
 	ret = watchdog_init_timeout(&st_wdog_dev, 0, dev);
-	if (ret) {
-		dev_err(dev, "Unable to initialise watchdog timeout\n");
+	if (ret)
 		return ret;
-	}
 
 	ret = devm_watchdog_register_device(dev, &st_wdog_dev);
 	if (ret) {
-- 
2.11.0

[PATCH 14/16] watchdog: stm32_iwdg: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/stm32_iwdg.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c
index 309563e002b8..50ce762bddb2 100644
--- a/drivers/watchdog/stm32_iwdg.c
+++ b/drivers/watchdog/stm32_iwdg.c
@@ -235,11 +235,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT);
-
-	ret = watchdog_init_timeout(wdd, 0, &pdev->dev);
-	if (ret)
-		dev_warn(&pdev->dev,
-			 "unable to set timeout value, using default\n");
+	watchdog_init_timeout(wdd, 0, &pdev->dev);
 
 	ret = watchdog_register_device(wdd);
 	if (ret) {
-- 
2.11.0

[PATCH 15/16] watchdog: xen_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/xen_wdt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/watchdog/xen_wdt.c b/drivers/watchdog/xen_wdt.c
index f1c016d015b3..d54da2e0f2e1 100644
--- a/drivers/watchdog/xen_wdt.c
+++ b/drivers/watchdog/xen_wdt.c
@@ -135,9 +135,7 @@ static int xen_wdt_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	if (watchdog_init_timeout(&xen_wdt_dev, timeout, NULL))
-		dev_info(&pdev->dev, "timeout value invalid, using %d\n",
-			xen_wdt_dev.timeout);
+	watchdog_init_timeout(&xen_wdt_dev, timeout, NULL);
 	watchdog_set_nowayout(&xen_wdt_dev, nowayout);
 	watchdog_stop_on_reboot(&xen_wdt_dev);
 	watchdog_stop_on_unregister(&xen_wdt_dev);
-- 
2.11.0

[PATCH 16/16] watchdog: ziirave_wdt: drop warning after calling watchdog_init_timeout

[Wolfram Sang]

The core will print out details now.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/watchdog/ziirave_wdt.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index d3594aa3a374..43e6b575c32c 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -658,11 +658,7 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 	w_priv->wdd.parent = &client->dev;
 	w_priv->wdd.groups = ziirave_wdt_groups;
 
-	ret = watchdog_init_timeout(&w_priv->wdd, wdt_timeout, &client->dev);
-	if (ret) {
-		dev_info(&client->dev,
-			 "Unable to select timeout value, using default\n");
-	}
+	watchdog_init_timeout(&w_priv->wdd, wdt_timeout, &client->dev);
 
 	/*
 	 * The default value set in the watchdog should be perfectly valid, so
-- 
2.11.0

Re: [PATCH 00/16] watchdog: refactor init_timeout and update users

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

On Sun, Apr 14, 2019 at 12:26:11PM +0200, Wolfram Sang wrote:
> When trying to add DT support to the DA9063 WDT, I didn't want to add yet
> another error message when watchdog_init_timeout fails. The core could do this
> with more detail, and also much more consistent. So, I refactored this routine
> and removed error strings from its callers. Note that DA9063 support is not
> added here because I still need to think about another issue. But this series
> has been tested using the renesas_wdt driver on a Renesas Salvator-XS board
> (R-Car M3N) and build bot is happy, too.

Sorry, I forgot to CC the (although few) driver maintainers. I suggest
to wait for review comments about the general apprach and I will make
sure to CC them for V2 then.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/16] watchdog: refactor watchdog_init_timeout

[Guenter Roeck]

On 4/14/19 3:26 AM, Wolfram Sang wrote:
> The function is not easy to read and has two problems: a) -EINVAL is
> returned when the module parameter is invalid but the DT parameter is
> OK, and b) for the module parameter, zero is a valid value but for DT it
> is invalid.
> 

That was on purpose: A module parameter of 0 reflects that no module parameter
was provided, which is not an error. An explicit DT property with value 0 _is_
an error and does not make sense. "use the default", in the DT case, can and
should be expressed by providing no property, not by providing a property with
value 0.

Guenter

> Refactor the code to have a the same pattern of checks for the module
> parameter and DT. Further ones can be easily added in the future if the
> need arises. The above mentioned problems are fixed, too.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>   drivers/watchdog/watchdog_core.c | 33 ++++++++++++++++++---------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index eb8fa25f8eb2..85c136acc0e9 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -104,10 +104,11 @@ static void watchdog_check_min_max_timeout(struct watchdog_device *wdd)
>    * Initialize the timeout field of the watchdog_device struct with either the
>    * timeout module parameter (if it is valid value) or the timeout-sec property
>    * (only if it is a valid value and the timeout_parm is out of bounds).
> - * If none of them are valid then we keep the old value (which should normally
> - * be the default timeout value).
> + * If none of them are valid or all of them are zero ("don't care") then we keep
> + * the old value (which should normally be the default timeout value).
>    *
> - * A zero is returned on success and -EINVAL for failure.
> + * A zero is returned on success or -EINVAL if all provided values are out of
> + * bounds.
>    */
>   int watchdog_init_timeout(struct watchdog_device *wdd,
>   				unsigned int timeout_parm, struct device *dev)
> @@ -117,22 +118,24 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>   
>   	watchdog_check_min_max_timeout(wdd);
>   
> -	/* try to get the timeout module parameter first */
> -	if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
> -		wdd->timeout = timeout_parm;
> -		return ret;
> -	}
> -	if (timeout_parm)
> +	/* check the driver supplied value (likely a module parameter) first */
> +	if (timeout_parm) {
> +		if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
> +			wdd->timeout = timeout_parm;
> +			return 0;
> +		}
>   		ret = -EINVAL;
> +	}
>   
>   	/* try to get the timeout_sec property */
> -	if (dev == NULL || dev->of_node == NULL)
> -		return ret;
> -	of_property_read_u32(dev->of_node, "timeout-sec", &t);
> -	if (!watchdog_timeout_invalid(wdd, t) && t)
> -		wdd->timeout = t;
> -	else
> +	if (dev && dev->of_node &&
> +	    of_property_read_u32(dev->of_node, "timeout-sec", &t) == 0 && t) {
> +		if (!watchdog_timeout_invalid(wdd, t)) {
> +			wdd->timeout = t;
> +			return 0;
> +		}
>   		ret = -EINVAL;
> +	}
>   
>   	return ret;
>   }
> 

Re: [PATCH 01/16] watchdog: refactor watchdog_init_timeout

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]


> > The function is not easy to read and has two problems: a) -EINVAL is
> > returned when the module parameter is invalid but the DT parameter is
> > OK, and b) for the module parameter, zero is a valid value but for DT it
> > is invalid.
> > 
> 
> That was on purpose: A module parameter of 0 reflects that no module parameter
> was provided, which is not an error. An explicit DT property with value 0 _is_
> an error and does not make sense. "use the default", in the DT case, can and
> should be expressed by providing no property, not by providing a property with
> value 0.

OK, I can fix the code to do that as before. And add some documentation
to describe that. Please let me know if it is documented already and I
just missed it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 01/16] watchdog: refactor watchdog_init_timeout

[Guenter Roeck]

On 4/14/19 6:42 AM, Wolfram Sang wrote:
> 
>>> The function is not easy to read and has two problems: a) -EINVAL is
>>> returned when the module parameter is invalid but the DT parameter is
>>> OK, and b) for the module parameter, zero is a valid value but for DT it
>>> is invalid.
>>>
>>
>> That was on purpose: A module parameter of 0 reflects that no module parameter
>> was provided, which is not an error. An explicit DT property with value 0 _is_
>> an error and does not make sense. "use the default", in the DT case, can and
>> should be expressed by providing no property, not by providing a property with
>> value 0.
> 
> OK, I can fix the code to do that as before. And add some documentation
> to describe that. Please let me know if it is documented already and I
> just missed it.
> 
The existing documentation is indeed a bit vague. Explicitly documenting the
expected behavior would be a good idea.  I keep having to explain how this is
supposed to work for almost every new driver. The most difficult part is to
get people to understand that the 'timeout' module parameter value should _not_
be pre-initialized with the default timeout but with 0.

Thanks,
Guenter

Re: [PATCH 00/16] watchdog: refactor init_timeout and update users

[Guenter Roeck]

On 4/14/19 3:26 AM, Wolfram Sang wrote:
> When trying to add DT support to the DA9063 WDT, I didn't want to add yet
> another error message when watchdog_init_timeout fails. The core could do this
> with more detail, and also much more consistent. So, I refactored this routine
> and removed error strings from its callers. Note that DA9063 support is not
> added here because I still need to think about another issue. But this series
> has been tested using the renesas_wdt driver on a Renesas Salvator-XS board
> (R-Car M3N) and build bot is happy, too.
> 
> Looking forward to comments.
> 

I like the idea. We should probably do the same for [devm_]watchdog_register_device().

For the series, except patch 1:

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

> Thanks,
> 
>     Wolfram
> 
> Wolfram Sang (16):
>    watchdog: refactor watchdog_init_timeout
>    watchdog: add error messages when initializing timeout fails
>    watchdog: cadence_wdt: drop warning after calling
>      watchdog_init_timeout
>    watchdog: cadence_wdt: still probe if user supplied timeout is invalid
>    watchdog: ebc-c384_wdt: drop warning after calling
>      watchdog_init_timeout
>    watchdog: hpwdt: drop warning after calling watchdog_init_timeout
>    watchdog: i6300esb: drop warning after calling watchdog_init_timeout
>    watchdog: imx_sc_wdt: drop warning after calling watchdog_init_timeout
>    watchdog: ni903x_wdt: drop warning after calling watchdog_init_timeout
>    watchdog: nic7018_wdt: drop warning after calling
>      watchdog_init_timeout
>    watchdog: renesas_wdt: drop warning after calling
>      watchdog_init_timeout
>    watchdog: sp5100_tco: drop warning after calling watchdog_init_timeout
>    watchdog: st_lpc_wdt: drop warning after calling watchdog_init_timeout
>    watchdog: stm32_iwdg: drop warning after calling watchdog_init_timeout
>    watchdog: xen_wdt: drop warning after calling watchdog_init_timeout
>    watchdog: ziirave_wdt: drop warning after calling
>      watchdog_init_timeout
> 
>   drivers/watchdog/cadence_wdt.c   |  7 +------
>   drivers/watchdog/ebc-c384_wdt.c  |  5 +----
>   drivers/watchdog/hpwdt.c         |  3 +--
>   drivers/watchdog/i6300esb.c      |  5 +----
>   drivers/watchdog/imx_sc_wdt.c    |  5 +----
>   drivers/watchdog/ni903x_wdt.c    |  4 +---
>   drivers/watchdog/nic7018_wdt.c   |  5 +----
>   drivers/watchdog/renesas_wdt.c   |  4 +---
>   drivers/watchdog/sp5100_tco.c    |  4 +---
>   drivers/watchdog/st_lpc_wdt.c    |  4 +---
>   drivers/watchdog/stm32_iwdg.c    |  6 +-----
>   drivers/watchdog/watchdog_core.c | 39 ++++++++++++++++++++++++---------------
>   drivers/watchdog/xen_wdt.c       |  4 +---
>   drivers/watchdog/ziirave_wdt.c   |  6 +-----
>   14 files changed, 37 insertions(+), 64 deletions(-)
> 

Re: [PATCH 01/16] watchdog: refactor watchdog_init_timeout

[Sergei Shtylyov]

Hello!

On 14.04.2019 13:26, Wolfram Sang wrote:

> The function is not easy to read and has two problems: a) -EINVAL is
> returned when the module parameter is invalid but the DT parameter is
> OK, and b) for the module parameter, zero is a valid value but for DT it
> is invalid.
> 
> Refactor the code to have a the same pattern of checks for the module
                             ^^^^^
    One article would be enough. :-)

> parameter and DT. Further ones can be easily added in the future if the
> need arises. The above mentioned problems are fixed, too.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[...]

MBR, Sergei