[PATCH 0/5] Ziirave_wdt driver fixes

[PATCH 0/5] Ziirave_wdt driver fixes

[Andrey Smirnov]

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

[PATCH 1/5] watchdog: ziirave_wdt: Add missing newline

[Andrey Smirnov]

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

[PATCH 2/5] watchdog: ziirave_wdt: Be verbose about errors in probe()

[Andrey Smirnov]

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

[PATCH 3/5] watchdog: ziirave_wdt: Be more verbose during firmware update

[Andrey Smirnov]

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

[PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value

[Andrey Smirnov]

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

[PATCH 5/5] watchdog: ziirave_wdt: Log bootloader/firmware info during probe

[Andrey Smirnov]

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

Re: [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value

[Guenter Roeck]

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
> 

Re: [PATCH 0/5] Ziirave_wdt driver fixes

[Guenter Roeck]

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

Re: [PATCH 1/5] watchdog: ziirave_wdt: Add missing newline

[Guenter Roeck]

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
> 

Re: [PATCH 2/5] watchdog: ziirave_wdt: Be verbose about errors in probe()

[Guenter Roeck]

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
> 

Re: [PATCH 3/5] watchdog: ziirave_wdt: Be more verbose during firmware update

[Guenter Roeck]

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
> 

Re: [PATCH 5/5] watchdog: ziirave_wdt: Log bootloader/firmware info during probe

[Guenter Roeck]

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
> 

Re: [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value

[Guenter Roeck]

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
> 

Re: [PATCH 4/5] watchdog: ziirave_wdt: Don't bail out on unexpected timeout value

[Andrey Smirnov]

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