Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] Ziirave_wdt driver fixes
@ 2019-07-31 17:42 Andrey Smirnov
  2019-07-31 17:42 ` [PATCH 1/5] watchdog: ziirave_wdt: Add missing newline Andrey Smirnov
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-07-31 17:42 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Andrey Smirnov, Chris Healy, Guenter Roeck, Rick Ramstetter,
	linux-kernel

Everyone,

This series contains various fixes/improvements for ziirave_wdt
driver. Hopefully each commit is self-explanatory.

Feedback is welcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (5):
  watchdog: ziirave_wdt: Add missing newline
  watchdog: ziirave_wdt: Be verbose about errors in probe()
  watchdog: ziirave_wdt: Be more verbose during firmware update
  watchdog: ziirave_wdt: Don't bail out on unexpected timeout value
  watchdog: ziirave_wdt: Log bootloader/firmware info during probe

 drivers/watchdog/ziirave_wdt.c | 74 +++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 23 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] watchdog: ziirave_wdt: Add missing newline
  2019-07-31 17:42 [PATCH 0/5] Ziirave_wdt driver fixes Andrey Smirnov
@ 2019-07-31 17:42 ` Andrey Smirnov
  2019-07-31 21:42   ` Guenter Roeck
  2019-07-31 17:42 ` [PATCH 2/5] watchdog: ziirave_wdt: Be verbose about errors in probe() Andrey Smirnov
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Andrey Smirnov @ 2019-07-31 17:42 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Andrey Smirnov, Chris Healy, Guenter Roeck, Rick Ramstetter,
	linux-kernel

Add missing newline.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index dec660c509b3..6ec028fb2635 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -671,7 +671,7 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 		if (ret)
 			return ret;
 
-		dev_info(&client->dev, "Timeout set to %ds.",
+		dev_info(&client->dev, "Timeout set to %ds\n",
 			 w_priv->wdd.timeout);
 	}
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/5] watchdog: ziirave_wdt: Be verbose about errors in probe()
  2019-07-31 17:42 [PATCH 0/5] Ziirave_wdt driver fixes Andrey Smirnov
  2019-07-31 17:42 ` [PATCH 1/5] watchdog: ziirave_wdt: Add missing newline Andrey Smirnov
@ 2019-07-31 17:42 ` Andrey Smirnov
  2019-07-31 21:42   ` Guenter Roeck
  2019-07-31 17:42 ` [PATCH 3/5] watchdog: ziirave_wdt: Be more verbose during firmware update Andrey Smirnov
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Andrey Smirnov @ 2019-07-31 17:42 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Andrey Smirnov, Chris Healy, Guenter Roeck, Rick Ramstetter,
	linux-kernel

The driver is quite silent in case of probe failure, which makes it
more difficult to diagnose problem from the kernel log. Add logging to
all of the silent error paths ziirave_wdt_probe() to improve that.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 6ec028fb2635..8c71341a9c1c 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -658,8 +658,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 	 */
 	if (w_priv->wdd.timeout == 0) {
 		val = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT);
-		if (val < 0)
+		if (val < 0) {
+			dev_err(&client->dev, "Failed to read timeout\n");
 			return val;
+		}
 
 		if (val < ZIIRAVE_TIMEOUT_MIN)
 			return -ENODEV;
@@ -668,8 +670,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 	} else {
 		ret = ziirave_wdt_set_timeout(&w_priv->wdd,
 					      w_priv->wdd.timeout);
-		if (ret)
+		if (ret) {
+			dev_err(&client->dev, "Failed to set timeout\n");
 			return ret;
+		}
 
 		dev_info(&client->dev, "Timeout set to %ds\n",
 			 w_priv->wdd.timeout);
@@ -681,34 +685,46 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 
 	/* If in unconfigured state, set to stopped */
 	val = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_STATE);
-	if (val < 0)
+	if (val < 0) {
+		dev_err(&client->dev, "Failed to read state\n");
 		return val;
+	}
 
 	if (val == ZIIRAVE_STATE_INITIAL)
 		ziirave_wdt_stop(&w_priv->wdd);
 
 	ret = ziirave_wdt_init_duration(client);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to init duration\n");
 		return ret;
+	}
 
 	ret = ziirave_wdt_revision(client, &w_priv->firmware_rev,
 				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to read firmware version\n");
 		return ret;
+	}
 
 	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
 				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to read bootloader version\n");
 		return ret;
+	}
 
 	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
 						ZIIRAVE_WDT_RESET_REASON);
-	if (w_priv->reset_reason < 0)
+	if (w_priv->reset_reason < 0) {
+		dev_err(&client->dev, "Failed to read reset reason\n");
 		return w_priv->reset_reason;
+	}
 
 	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons) ||
-	    !ziirave_reasons[w_priv->reset_reason])
+	    !ziirave_reasons[w_priv->reset_reason]) {
+		dev_err(&client->dev, "Invalid reset reason\n");
 		return -ENODEV;
+	}
 
 	ret = watchdog_register_device(&w_priv->wdd);
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 3/5] watchdog: ziirave_wdt: Be more verbose during firmware update
  2019-07-31 17:42 [PATCH 0/5] Ziirave_wdt driver fixes Andrey Smirnov
  2019-07-31 17:42 ` [PATCH 1/5] watchdog: ziirave_wdt: Add missing newline Andrey Smirnov
  2019-07-31 17:42 ` [PATCH 2/5] watchdog: ziirave_wdt: Be verbose about errors in probe() Andrey Smirnov
@ 2019-07-31 17:42 ` Andrey Smirnov
  2019-07-31 21:43   ` Guenter Roeck
  2019-07-31 17:42 ` [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value Andrey Smirnov
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Andrey Smirnov @ 2019-07-31 17:42 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Andrey Smirnov, Chris Healy, Guenter Roeck, Rick Ramstetter,
	linux-kernel

Add more error logging to ziirave_firm_upload() for diagnostics.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 8c71341a9c1c..89ce6982ba53 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -335,14 +335,18 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
 
 	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_JUMP_TO_BOOTLOADER, 1,
 				      false);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to jump to bootloader\n");
 		return ret;
+	}
 
 	msleep(500);
 
 	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_START, 1, true);
-	if (ret)
+	if (ret) {
+		dev_err(&client->dev, "Failed to start download\n");
 		return ret;
+	}
 
 	msleep(500);
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value
  2019-07-31 17:42 [PATCH 0/5] Ziirave_wdt driver fixes Andrey Smirnov
                   ` (2 preceding siblings ...)
  2019-07-31 17:42 ` [PATCH 3/5] watchdog: ziirave_wdt: Be more verbose during firmware update Andrey Smirnov
@ 2019-07-31 17:42 ` Andrey Smirnov
  2019-07-31 18:09   ` Guenter Roeck
  2019-08-02 20:49   ` Guenter Roeck
  2019-07-31 17:42 ` [PATCH 5/5] watchdog: ziirave_wdt: Log bootloader/firmware info during probe Andrey Smirnov
  2019-07-31 18:11 ` [PATCH 0/5] Ziirave_wdt driver fixes Guenter Roeck
  5 siblings, 2 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-07-31 17:42 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Andrey Smirnov, Chris Healy, Guenter Roeck, Rick Ramstetter,
	linux-kernel

Reprogramming bootloader on watchdog MCU will result in reported
default timeout value of "0". That in turn will be unnecesarily
rejected by the driver as invalid device (-ENODEV). Simplify probe to
just read stored timeout value, clamp it to an acceptable range and
program the value unconditionally to fix the above.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 89ce6982ba53..33c8d2eadada 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -667,22 +667,18 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 			return val;
 		}
 
-		if (val < ZIIRAVE_TIMEOUT_MIN)
-			return -ENODEV;
-
-		w_priv->wdd.timeout = val;
-	} else {
-		ret = ziirave_wdt_set_timeout(&w_priv->wdd,
-					      w_priv->wdd.timeout);
-		if (ret) {
-			dev_err(&client->dev, "Failed to set timeout\n");
-			return ret;
-		}
+		w_priv->wdd.timeout = clamp(val, ZIIRAVE_TIMEOUT_MIN,
+					    ZIIRAVE_TIMEOUT_MAX);
+	}
 
-		dev_info(&client->dev, "Timeout set to %ds\n",
-			 w_priv->wdd.timeout);
+	ret = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout);
+	if (ret) {
+		dev_err(&client->dev, "Failed to set timeout\n");
+		return ret;
 	}
 
+	dev_info(&client->dev, "Timeout set to %ds\n", w_priv->wdd.timeout);
+
 	watchdog_set_nowayout(&w_priv->wdd, nowayout);
 
 	i2c_set_clientdata(client, w_priv);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 5/5] watchdog: ziirave_wdt: Log bootloader/firmware info during probe
  2019-07-31 17:42 [PATCH 0/5] Ziirave_wdt driver fixes Andrey Smirnov
                   ` (3 preceding siblings ...)
  2019-07-31 17:42 ` [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value Andrey Smirnov
@ 2019-07-31 17:42 ` Andrey Smirnov
  2019-07-31 21:44   ` Guenter Roeck
  2019-07-31 18:11 ` [PATCH 0/5] Ziirave_wdt driver fixes Guenter Roeck
  5 siblings, 1 reply; 14+ messages in thread
From: Andrey Smirnov @ 2019-07-31 17:42 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Andrey Smirnov, Chris Healy, Guenter Roeck, Rick Ramstetter,
	linux-kernel

Log bootloader/firmware info during probe. This information is
available via sysfs already, but it's really helpful to have this in
kernel log during startup as well.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Rick Ramstetter <rick@anteaterllc.com>
Cc: linux-watchdog@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/watchdog/ziirave_wdt.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index 33c8d2eadada..84ba8820ac86 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -68,6 +68,9 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL,
 #define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER		0x0c
 #define ZIIRAVE_CMD_DOWNLOAD_PACKET		0x0e
 
+#define ZIIRAVE_FW_VERSION_FMT	"02.%02u.%02u"
+#define ZIIRAVE_BL_VERSION_FMT	"01.%02u.%02u"
+
 struct ziirave_wdt_rev {
 	unsigned char major;
 	unsigned char minor;
@@ -482,7 +485,7 @@ static ssize_t ziirave_wdt_sysfs_show_firm(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = sprintf(buf, "02.%02u.%02u", w_priv->firmware_rev.major,
+	ret = sprintf(buf, ZIIRAVE_FW_VERSION_FMT, w_priv->firmware_rev.major,
 		      w_priv->firmware_rev.minor);
 
 	mutex_unlock(&w_priv->sysfs_mutex);
@@ -505,7 +508,7 @@ static ssize_t ziirave_wdt_sysfs_show_boot(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = sprintf(buf, "01.%02u.%02u", w_priv->bootloader_rev.major,
+	ret = sprintf(buf, ZIIRAVE_BL_VERSION_FMT, w_priv->bootloader_rev.major,
 		      w_priv->bootloader_rev.minor);
 
 	mutex_unlock(&w_priv->sysfs_mutex);
@@ -572,7 +575,8 @@ static ssize_t ziirave_wdt_sysfs_store_firm(struct device *dev,
 		goto unlock_mutex;
 	}
 
-	dev_info(&client->dev, "Firmware updated to version 02.%02u.%02u\n",
+	dev_info(&client->dev,
+		 "Firmware updated to version " ZIIRAVE_FW_VERSION_FMT "\n",
 		 w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
 
 	/* Restore the watchdog timeout */
@@ -706,6 +710,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	dev_info(&client->dev,
+		 "Firmware version: " ZIIRAVE_FW_VERSION_FMT "\n",
+		 w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
+
 	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
 				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
 	if (ret) {
@@ -713,6 +721,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	dev_info(&client->dev,
+		 "Bootloader version: " ZIIRAVE_BL_VERSION_FMT "\n",
+		 w_priv->bootloader_rev.major, w_priv->bootloader_rev.minor);
+
 	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
 						ZIIRAVE_WDT_RESET_REASON);
 	if (w_priv->reset_reason < 0) {
-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value
  2019-07-31 17:42 ` [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value Andrey Smirnov
@ 2019-07-31 18:09   ` Guenter Roeck
  2019-08-10 21:17     ` Andrey Smirnov
  2019-08-02 20:49   ` Guenter Roeck
  1 sibling, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2019-07-31 18:09 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: linux-watchdog, Chris Healy, Rick Ramstetter, linux-kernel

On Wed, Jul 31, 2019 at 10:42:51AM -0700, Andrey Smirnov wrote:
> Reprogramming bootloader on watchdog MCU will result in reported
> default timeout value of "0". That in turn will be unnecesarily

unnecessarily

> rejected by the driver as invalid device (-ENODEV). Simplify probe to
> just read stored timeout value, clamp it to an acceptable range and
> program the value unconditionally to fix the above.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/watchdog/ziirave_wdt.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> index 89ce6982ba53..33c8d2eadada 100644
> --- a/drivers/watchdog/ziirave_wdt.c
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -667,22 +667,18 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>  			return val;
>  		}
>  
> -		if (val < ZIIRAVE_TIMEOUT_MIN)
> -			return -ENODEV;
> -
> -		w_priv->wdd.timeout = val;
> -	} else {
> -		ret = ziirave_wdt_set_timeout(&w_priv->wdd,
> -					      w_priv->wdd.timeout);
> -		if (ret) {
> -			dev_err(&client->dev, "Failed to set timeout\n");
> -			return ret;
> -		}
> +		w_priv->wdd.timeout = clamp(val, ZIIRAVE_TIMEOUT_MIN,
> +					    ZIIRAVE_TIMEOUT_MAX);

Are you sure ? Effectively that will set the timeout to the minimum,
ie three seconds. It might be better to define and set some default.
Your call, of course.

Thanks,
Guenter

> +	}
>  
> -		dev_info(&client->dev, "Timeout set to %ds\n",
> -			 w_priv->wdd.timeout);
> +	ret = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to set timeout\n");
> +		return ret;
>  	}
>  
> +	dev_info(&client->dev, "Timeout set to %ds\n", w_priv->wdd.timeout);
> +
>  	watchdog_set_nowayout(&w_priv->wdd, nowayout);
>  
>  	i2c_set_clientdata(client, w_priv);
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] Ziirave_wdt driver fixes
  2019-07-31 17:42 [PATCH 0/5] Ziirave_wdt driver fixes Andrey Smirnov
                   ` (4 preceding siblings ...)
  2019-07-31 17:42 ` [PATCH 5/5] watchdog: ziirave_wdt: Log bootloader/firmware info during probe Andrey Smirnov
@ 2019-07-31 18:11 ` Guenter Roeck
  5 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-07-31 18:11 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: linux-watchdog, Chris Healy, Rick Ramstetter, linux-kernel

On Wed, Jul 31, 2019 at 10:42:47AM -0700, Andrey Smirnov wrote:
> Everyone,
> 
> This series contains various fixes/improvements for ziirave_wdt
> driver. Hopefully each commit is self-explanatory.
> 
> Feedback is welcome!
> 
> Thanks,
> Andrey Smirnov
> 
> Andrey Smirnov (5):
>   watchdog: ziirave_wdt: Add missing newline
>   watchdog: ziirave_wdt: Be verbose about errors in probe()
>   watchdog: ziirave_wdt: Be more verbose during firmware update
>   watchdog: ziirave_wdt: Don't bail out on unexpected timeout value
>   watchdog: ziirave_wdt: Log bootloader/firmware info during probe

Since you are touching the driver, I would suggest another change:
Use devm_watchdog_register_device() and drop the remove function.

Guenter

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/5] watchdog: ziirave_wdt: Add missing newline
  2019-07-31 17:42 ` [PATCH 1/5] watchdog: ziirave_wdt: Add missing newline Andrey Smirnov
@ 2019-07-31 21:42   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-07-31 21:42 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: linux-watchdog, Chris Healy, Rick Ramstetter, linux-kernel

On Wed, Jul 31, 2019 at 10:42:48AM -0700, Andrey Smirnov wrote:
> Add missing newline.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

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

> ---
>  drivers/watchdog/ziirave_wdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> index dec660c509b3..6ec028fb2635 100644
> --- a/drivers/watchdog/ziirave_wdt.c
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -671,7 +671,7 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>  		if (ret)
>  			return ret;
>  
> -		dev_info(&client->dev, "Timeout set to %ds.",
> +		dev_info(&client->dev, "Timeout set to %ds\n",
>  			 w_priv->wdd.timeout);
>  	}
>  
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/5] watchdog: ziirave_wdt: Be verbose about errors in probe()
  2019-07-31 17:42 ` [PATCH 2/5] watchdog: ziirave_wdt: Be verbose about errors in probe() Andrey Smirnov
@ 2019-07-31 21:42   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-07-31 21:42 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: linux-watchdog, Chris Healy, Rick Ramstetter, linux-kernel

On Wed, Jul 31, 2019 at 10:42:49AM -0700, Andrey Smirnov wrote:
> The driver is quite silent in case of probe failure, which makes it
> more difficult to diagnose problem from the kernel log. Add logging to
> all of the silent error paths ziirave_wdt_probe() to improve that.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

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

> ---
>  drivers/watchdog/ziirave_wdt.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> index 6ec028fb2635..8c71341a9c1c 100644
> --- a/drivers/watchdog/ziirave_wdt.c
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -658,8 +658,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>  	 */
>  	if (w_priv->wdd.timeout == 0) {
>  		val = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_TIMEOUT);
> -		if (val < 0)
> +		if (val < 0) {
> +			dev_err(&client->dev, "Failed to read timeout\n");
>  			return val;
> +		}
>  
>  		if (val < ZIIRAVE_TIMEOUT_MIN)
>  			return -ENODEV;
> @@ -668,8 +670,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>  	} else {
>  		ret = ziirave_wdt_set_timeout(&w_priv->wdd,
>  					      w_priv->wdd.timeout);
> -		if (ret)
> +		if (ret) {
> +			dev_err(&client->dev, "Failed to set timeout\n");
>  			return ret;
> +		}
>  
>  		dev_info(&client->dev, "Timeout set to %ds\n",
>  			 w_priv->wdd.timeout);
> @@ -681,34 +685,46 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>  
>  	/* If in unconfigured state, set to stopped */
>  	val = i2c_smbus_read_byte_data(client, ZIIRAVE_WDT_STATE);
> -	if (val < 0)
> +	if (val < 0) {
> +		dev_err(&client->dev, "Failed to read state\n");
>  		return val;
> +	}
>  
>  	if (val == ZIIRAVE_STATE_INITIAL)
>  		ziirave_wdt_stop(&w_priv->wdd);
>  
>  	ret = ziirave_wdt_init_duration(client);
> -	if (ret)
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to init duration\n");
>  		return ret;
> +	}
>  
>  	ret = ziirave_wdt_revision(client, &w_priv->firmware_rev,
>  				   ZIIRAVE_WDT_FIRM_VER_MAJOR);
> -	if (ret)
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to read firmware version\n");
>  		return ret;
> +	}
>  
>  	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
>  				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
> -	if (ret)
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to read bootloader version\n");
>  		return ret;
> +	}
>  
>  	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
>  						ZIIRAVE_WDT_RESET_REASON);
> -	if (w_priv->reset_reason < 0)
> +	if (w_priv->reset_reason < 0) {
> +		dev_err(&client->dev, "Failed to read reset reason\n");
>  		return w_priv->reset_reason;
> +	}
>  
>  	if (w_priv->reset_reason >= ARRAY_SIZE(ziirave_reasons) ||
> -	    !ziirave_reasons[w_priv->reset_reason])
> +	    !ziirave_reasons[w_priv->reset_reason]) {
> +		dev_err(&client->dev, "Invalid reset reason\n");
>  		return -ENODEV;
> +	}
>  
>  	ret = watchdog_register_device(&w_priv->wdd);
>  
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/5] watchdog: ziirave_wdt: Be more verbose during firmware update
  2019-07-31 17:42 ` [PATCH 3/5] watchdog: ziirave_wdt: Be more verbose during firmware update Andrey Smirnov
@ 2019-07-31 21:43   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-07-31 21:43 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: linux-watchdog, Chris Healy, Rick Ramstetter, linux-kernel

On Wed, Jul 31, 2019 at 10:42:50AM -0700, Andrey Smirnov wrote:
> Add more error logging to ziirave_firm_upload() for diagnostics.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

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

> ---
>  drivers/watchdog/ziirave_wdt.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> index 8c71341a9c1c..89ce6982ba53 100644
> --- a/drivers/watchdog/ziirave_wdt.c
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -335,14 +335,18 @@ static int ziirave_firm_upload(struct watchdog_device *wdd,
>  
>  	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_JUMP_TO_BOOTLOADER, 1,
>  				      false);
> -	if (ret)
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to jump to bootloader\n");
>  		return ret;
> +	}
>  
>  	msleep(500);
>  
>  	ret = ziirave_firm_write_byte(wdd, ZIIRAVE_CMD_DOWNLOAD_START, 1, true);
> -	if (ret)
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to start download\n");
>  		return ret;
> +	}
>  
>  	msleep(500);
>  
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 5/5] watchdog: ziirave_wdt: Log bootloader/firmware info during probe
  2019-07-31 17:42 ` [PATCH 5/5] watchdog: ziirave_wdt: Log bootloader/firmware info during probe Andrey Smirnov
@ 2019-07-31 21:44   ` Guenter Roeck
  0 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-07-31 21:44 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: linux-watchdog, Chris Healy, Rick Ramstetter, linux-kernel

On Wed, Jul 31, 2019 at 10:42:52AM -0700, Andrey Smirnov wrote:
> Log bootloader/firmware info during probe. This information is
> available via sysfs already, but it's really helpful to have this in
> kernel log during startup as well.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

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

> ---
>  drivers/watchdog/ziirave_wdt.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> index 33c8d2eadada..84ba8820ac86 100644
> --- a/drivers/watchdog/ziirave_wdt.c
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -68,6 +68,9 @@ static char *ziirave_reasons[] = {"power cycle", "hw watchdog", NULL, NULL,
>  #define ZIIRAVE_CMD_JUMP_TO_BOOTLOADER		0x0c
>  #define ZIIRAVE_CMD_DOWNLOAD_PACKET		0x0e
>  
> +#define ZIIRAVE_FW_VERSION_FMT	"02.%02u.%02u"
> +#define ZIIRAVE_BL_VERSION_FMT	"01.%02u.%02u"
> +
>  struct ziirave_wdt_rev {
>  	unsigned char major;
>  	unsigned char minor;
> @@ -482,7 +485,7 @@ static ssize_t ziirave_wdt_sysfs_show_firm(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	ret = sprintf(buf, "02.%02u.%02u", w_priv->firmware_rev.major,
> +	ret = sprintf(buf, ZIIRAVE_FW_VERSION_FMT, w_priv->firmware_rev.major,
>  		      w_priv->firmware_rev.minor);
>  
>  	mutex_unlock(&w_priv->sysfs_mutex);
> @@ -505,7 +508,7 @@ static ssize_t ziirave_wdt_sysfs_show_boot(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	ret = sprintf(buf, "01.%02u.%02u", w_priv->bootloader_rev.major,
> +	ret = sprintf(buf, ZIIRAVE_BL_VERSION_FMT, w_priv->bootloader_rev.major,
>  		      w_priv->bootloader_rev.minor);
>  
>  	mutex_unlock(&w_priv->sysfs_mutex);
> @@ -572,7 +575,8 @@ static ssize_t ziirave_wdt_sysfs_store_firm(struct device *dev,
>  		goto unlock_mutex;
>  	}
>  
> -	dev_info(&client->dev, "Firmware updated to version 02.%02u.%02u\n",
> +	dev_info(&client->dev,
> +		 "Firmware updated to version " ZIIRAVE_FW_VERSION_FMT "\n",
>  		 w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
>  
>  	/* Restore the watchdog timeout */
> @@ -706,6 +710,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	dev_info(&client->dev,
> +		 "Firmware version: " ZIIRAVE_FW_VERSION_FMT "\n",
> +		 w_priv->firmware_rev.major, w_priv->firmware_rev.minor);
> +
>  	ret = ziirave_wdt_revision(client, &w_priv->bootloader_rev,
>  				   ZIIRAVE_WDT_BOOT_VER_MAJOR);
>  	if (ret) {
> @@ -713,6 +721,10 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	dev_info(&client->dev,
> +		 "Bootloader version: " ZIIRAVE_BL_VERSION_FMT "\n",
> +		 w_priv->bootloader_rev.major, w_priv->bootloader_rev.minor);
> +
>  	w_priv->reset_reason = i2c_smbus_read_byte_data(client,
>  						ZIIRAVE_WDT_RESET_REASON);
>  	if (w_priv->reset_reason < 0) {
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value
  2019-07-31 17:42 ` [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value Andrey Smirnov
  2019-07-31 18:09   ` Guenter Roeck
@ 2019-08-02 20:49   ` Guenter Roeck
  1 sibling, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2019-08-02 20:49 UTC (permalink / raw)
  To: Andrey Smirnov; +Cc: linux-watchdog, Chris Healy, Rick Ramstetter, linux-kernel

On Wed, Jul 31, 2019 at 10:42:51AM -0700, Andrey Smirnov wrote:
> Reprogramming bootloader on watchdog MCU will result in reported
> default timeout value of "0". That in turn will be unnecesarily
> rejected by the driver as invalid device (-ENODEV). Simplify probe to
> just read stored timeout value, clamp it to an acceptable range and
> program the value unconditionally to fix the above.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Rick Ramstetter <rick@anteaterllc.com>
> Cc: linux-watchdog@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

I have not heard back on the question I had about selecting the minimum
timeout and not a more reasonable default. Since the code itself
is technically correct, marking the patch as

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

Guenter

> ---
>  drivers/watchdog/ziirave_wdt.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> index 89ce6982ba53..33c8d2eadada 100644
> --- a/drivers/watchdog/ziirave_wdt.c
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -667,22 +667,18 @@ static int ziirave_wdt_probe(struct i2c_client *client,
>  			return val;
>  		}
>  
> -		if (val < ZIIRAVE_TIMEOUT_MIN)
> -			return -ENODEV;
> -
> -		w_priv->wdd.timeout = val;
> -	} else {
> -		ret = ziirave_wdt_set_timeout(&w_priv->wdd,
> -					      w_priv->wdd.timeout);
> -		if (ret) {
> -			dev_err(&client->dev, "Failed to set timeout\n");
> -			return ret;
> -		}
> +		w_priv->wdd.timeout = clamp(val, ZIIRAVE_TIMEOUT_MIN,
> +					    ZIIRAVE_TIMEOUT_MAX);
> +	}
>  
> -		dev_info(&client->dev, "Timeout set to %ds\n",
> -			 w_priv->wdd.timeout);
> +	ret = ziirave_wdt_set_timeout(&w_priv->wdd, w_priv->wdd.timeout);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to set timeout\n");
> +		return ret;
>  	}
>  
> +	dev_info(&client->dev, "Timeout set to %ds\n", w_priv->wdd.timeout);
> +
>  	watchdog_set_nowayout(&w_priv->wdd, nowayout);
>  
>  	i2c_set_clientdata(client, w_priv);
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value
  2019-07-31 18:09   ` Guenter Roeck
@ 2019-08-10 21:17     ` Andrey Smirnov
  0 siblings, 0 replies; 14+ messages in thread
From: Andrey Smirnov @ 2019-08-10 21:17 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: linux-watchdog, Chris Healy, Rick Ramstetter, linux-kernel

On Wed, Jul 31, 2019 at 11:09 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Jul 31, 2019 at 10:42:51AM -0700, Andrey Smirnov wrote:
> > Reprogramming bootloader on watchdog MCU will result in reported
> > default timeout value of "0". That in turn will be unnecesarily
>
> unnecessarily
>
> > rejected by the driver as invalid device (-ENODEV). Simplify probe to
> > just read stored timeout value, clamp it to an acceptable range and
> > program the value unconditionally to fix the above.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Rick Ramstetter <rick@anteaterllc.com>
> > Cc: linux-watchdog@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/watchdog/ziirave_wdt.c | 22 +++++++++-------------
> >  1 file changed, 9 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> > index 89ce6982ba53..33c8d2eadada 100644
> > --- a/drivers/watchdog/ziirave_wdt.c
> > +++ b/drivers/watchdog/ziirave_wdt.c
> > @@ -667,22 +667,18 @@ static int ziirave_wdt_probe(struct i2c_client *client,
> >                       return val;
> >               }
> >
> > -             if (val < ZIIRAVE_TIMEOUT_MIN)
> > -                     return -ENODEV;
> > -
> > -             w_priv->wdd.timeout = val;
> > -     } else {
> > -             ret = ziirave_wdt_set_timeout(&w_priv->wdd,
> > -                                           w_priv->wdd.timeout);
> > -             if (ret) {
> > -                     dev_err(&client->dev, "Failed to set timeout\n");
> > -                     return ret;
> > -             }
> > +             w_priv->wdd.timeout = clamp(val, ZIIRAVE_TIMEOUT_MIN,
> > +                                         ZIIRAVE_TIMEOUT_MAX);
>
> Are you sure ? Effectively that will set the timeout to the minimum,
> ie three seconds. It might be better to define and set some default.
> Your call, of course.
>

It doesn't really matter in my use-case (set timeout is a no-op),  but
it sounds like a better approach, so I'll change it in v2.

Thanks,
Andrey Smirnov

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31 17:42 [PATCH 0/5] Ziirave_wdt driver fixes Andrey Smirnov
2019-07-31 17:42 ` [PATCH 1/5] watchdog: ziirave_wdt: Add missing newline Andrey Smirnov
2019-07-31 21:42   ` Guenter Roeck
2019-07-31 17:42 ` [PATCH 2/5] watchdog: ziirave_wdt: Be verbose about errors in probe() Andrey Smirnov
2019-07-31 21:42   ` Guenter Roeck
2019-07-31 17:42 ` [PATCH 3/5] watchdog: ziirave_wdt: Be more verbose during firmware update Andrey Smirnov
2019-07-31 21:43   ` Guenter Roeck
2019-07-31 17:42 ` [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value Andrey Smirnov
2019-07-31 18:09   ` Guenter Roeck
2019-08-10 21:17     ` Andrey Smirnov
2019-08-02 20:49   ` Guenter Roeck
2019-07-31 17:42 ` [PATCH 5/5] watchdog: ziirave_wdt: Log bootloader/firmware info during probe Andrey Smirnov
2019-07-31 21:44   ` Guenter Roeck
2019-07-31 18:11 ` [PATCH 0/5] Ziirave_wdt driver fixes Guenter Roeck

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org linux-watchdog@archiver.kernel.org
	public-inbox-index linux-watchdog


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/ public-inbox