[PATCH v8 0/4] watchdog: da9063: Fix timeout handling

[PATCH v8 0/4] watchdog: da9063: Fix timeout handling

[Marco Felsch]

The current set_timeout() handling can't change the timeout value if the
watchdog is already running because the watchdog must be
disabled before a new timeout value can be set. This sounds a bit
strange but it is the only way to change the timeout value.

The watchdog also has a second quirk. The timeout value combines the
timeout and the watchdog enable signal. So a value not equal zero
indicates that the watchdog is running. This leads into problems if the
watchdog is disabled and a new timeout value should be set because
during setting the timeout value the watchdog gets enabled. The driver
must check if the watchdog is disabled. In that case the timeout value
must be stored to be available for the later wdt_start() call. If the
watchdog is already running the driver must write the new timeout value
immediately to the watchdog.

There was also a issue during the driver probe sequence. The driver must
set a new timeout reference mark if the watchdog was enabled in a earlier
stage e.g. the bootloader.

Changelog:

v8:
 - make use of watchdog_active() helper instead of watchdog_hw_running()
 - fix misspellings
 - drop set HW_RUNNING flag during wdt_start() introduced with v6
 - call _da9063_wdt_set_timeout() during probe() directly if watchdog is
   running.
 - da9063_wdt_is_running() returns now the raw timeout regval instead of
   the maped timeout in seconds.
 - fix da9063_wdt_is_running() return value, use unsigned int instead of
   int.

v7:
 - reorder patch stack "Fixes:" patches first.

v6:
 - fix timeout value buffering (don't buffer it twice, use HW_RUNNING
   flag)
 - set HW_RUNNING flag during wdt_start()
 - add watchdog enable check during probe()
 - rename _da9063_wdt_set_timeout -> da9063_wdt_update_timeout

v5:
 - add buffering the timeout if the watchdog is disabled
 - add more comments

v4:
 - Fix patch description

v3:
 - add delay between disable watchdog and write new timeout value during
   timeout update
 - fix comment to be more common

v2:
 - style changes

v1:
 - initial commit
 
Marco Felsch (4):
  watchdog: da9063: Fix setting/changing timeout
  watchdog: da9063: Fix updating timeout value
  watchdog: da9063: Fix timeout handling during probe
  watchdog: da9063: rename helper function to avoid misunderstandings

 drivers/watchdog/da9063_wdt.c | 67 +++++++++++++++++++++++++++++++----
 1 file changed, 61 insertions(+), 6 deletions(-)

-- 
2.17.0

[PATCH v8 1/4] watchdog: da9063: Fix setting/changing timeout

[Marco Felsch]

If the timeout value is set more than once the DA9063 watchdog triggers
a reset signal which reset the system.

To update the timeout value we have to disable the watchdog, clear the
watchdog counter value and write the new timeout value to the watchdog.
Clearing the counter value is a feature to be on the safe side because the
data sheet doesn't describe the behaviour of the watchdog counter value
after a watchdog disabling-enable-sequence.

The patch is based on Philipp Zabel's previous patch.

Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/watchdog/da9063_wdt.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index b17ac1bb1f28..c1216e61e64e 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -45,8 +45,31 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
 	return DA9063_TWDSCALE_MAX;
 }
 
+static int da9063_wdt_disable_timer(struct da9063 *da9063)
+{
+	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
+				  DA9063_TWDSCALE_MASK,
+				  DA9063_TWDSCALE_DISABLE);
+}
+
 static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
 {
+	int ret;
+
+	/*
+	 * The watchdog triggers a reboot if a timeout value is already
+	 * programmed because the timeout value combines two functions
+	 * in one: indicating the counter limit and starting the watchdog.
+	 * The watchdog must be disabled to be able to change the timeout
+	 * value if the watchdog is already running. Then we can set the
+	 * new timeout value which enables the watchdog again.
+	 */
+	ret = da9063_wdt_disable_timer(da9063);
+	if (ret)
+		return ret;
+
+	usleep_range(150, 300);
+
 	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
 				  DA9063_TWDSCALE_MASK, regval);
 }
@@ -71,8 +94,7 @@ static int da9063_wdt_stop(struct watchdog_device *wdd)
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
 	int ret;
 
-	ret = regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
-				 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
+	ret = da9063_wdt_disable_timer(da9063);
 	if (ret)
 		dev_alert(da9063->dev, "Watchdog failed to stop (err = %d)\n",
 			  ret);
-- 
2.17.0

[PATCH v8 2/4] watchdog: da9063: Fix updating timeout value

[Marco Felsch]

The DA9063 watchdog has only one register field to store the timeout value
and to enable the watchdog. The watchdog gets enabled if the value is
not zero. There is no issue if the watchdog is already running but it
leads into problems if the watchdog is disabled.

If the watchdog is disabled and only the timeout value should be prepared
the watchdog gets enabled too. Add a check to get the current watchdog
state and update the watchdog timeout value on hw-side only if the
watchdog is already active.

Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/watchdog/da9063_wdt.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index c1216e61e64e..6b0092b7d5a6 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -121,10 +121,23 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 {
 	struct da9063 *da9063 = watchdog_get_drvdata(wdd);
 	unsigned int selector;
-	int ret;
+	int ret = 0;
 
 	selector = da9063_wdt_timeout_to_sel(timeout);
-	ret = _da9063_wdt_set_timeout(da9063, selector);
+
+	/*
+	 * There are two cases when a set_timeout() will be called:
+	 * 1. The watchdog is off and someone wants to set the timeout for the
+	 *    further use.
+	 * 2. The watchdog is already running and a new timeout value should be
+	 *    set.
+	 *
+	 * The watchdog can't store a timeout value not equal zero without
+	 * enabling the watchdog, so the timeout must be buffered by the driver.
+	 */
+	if (watchdog_active(wdd))
+		ret = _da9063_wdt_set_timeout(da9063, selector);
+
 	if (ret)
 		dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
 			ret);
-- 
2.17.0

[PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Marco Felsch]

The watchdog can be enabled in previous steps (e.g. the bootloader). Check
if the watchdog is already running, retrieve the set timeout value and
set it again to set the new timeout reference mark.

Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index 6b0092b7d5a6..c5bd5ffe8ded 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
 	return DA9063_TWDSCALE_MAX;
 }
 
+/*
+ * Return 0 if watchdog is disabled, else non zero.
+ */
+static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
+{
+	unsigned int val;
+
+	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
+
+	return val & DA9063_TWDSCALE_MASK;
+}
+
 static int da9063_wdt_disable_timer(struct da9063 *da9063)
 {
 	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
@@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 {
 	struct da9063 *da9063;
 	struct watchdog_device *wdd;
+	unsigned int cur_timeout;
 
 	if (!pdev->dev.parent)
 		return -EINVAL;
@@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 
 	watchdog_set_drvdata(wdd, da9063);
 
+	cur_timeout = da9063_wdt_is_running(da9063);
+	if (cur_timeout) {
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+		_da9063_wdt_set_timeout(da9063, cur_timeout);
+	}
+
 	return devm_watchdog_register_device(&pdev->dev, wdd);
 }
 
-- 
2.17.0

[PATCH v8 4/4] watchdog: da9063: rename helper function to avoid misunderstandings

[Marco Felsch]

_da9063_wdt_set_timeout() is called from da9063_wdg_set_timeout() and
from da9063_wdg_start() but the name expect to be called only from
da9063_wdg_set_timeout(). Rename the function to avoid
misunderstandings.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/watchdog/da9063_wdt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index c5bd5ffe8ded..d6e30a1e6535 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -64,7 +64,7 @@ static int da9063_wdt_disable_timer(struct da9063 *da9063)
 				  DA9063_TWDSCALE_DISABLE);
 }
 
-static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
+static int da9063_wdt_update_timeout(struct da9063 *da9063, unsigned int regval)
 {
 	int ret;
 
@@ -93,7 +93,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd)
 	int ret;
 
 	selector = da9063_wdt_timeout_to_sel(wdd->timeout);
-	ret = _da9063_wdt_set_timeout(da9063, selector);
+	ret = da9063_wdt_update_timeout(da9063, selector);
 	if (ret)
 		dev_err(da9063->dev, "Watchdog failed to start (err = %d)\n",
 			ret);
@@ -148,7 +148,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 	 * enabling the watchdog, so the timeout must be buffered by the driver.
 	 */
 	if (watchdog_active(wdd))
-		ret = _da9063_wdt_set_timeout(da9063, selector);
+		ret = da9063_wdt_update_timeout(da9063, selector);
 
 	if (ret)
 		dev_err(da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
@@ -222,7 +222,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 	cur_timeout = da9063_wdt_is_running(da9063);
 	if (cur_timeout) {
 		set_bit(WDOG_HW_RUNNING, &wdd->status);
-		_da9063_wdt_set_timeout(da9063, cur_timeout);
+		da9063_wdt_update_timeout(da9063, cur_timeout);
 	}
 
 	return devm_watchdog_register_device(&pdev->dev, wdd);
-- 
2.17.0

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Guenter Roeck]

On Thu, May 24, 2018 at 01:51:00PM +0200, Marco Felsch wrote:
> The watchdog can be enabled in previous steps (e.g. the bootloader). Check
> if the watchdog is already running, retrieve the set timeout value and
> set it again to set the new timeout reference mark.
> 
> Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index 6b0092b7d5a6..c5bd5ffe8ded 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
>  	return DA9063_TWDSCALE_MAX;
>  }
>  
> +/*
> + * Return 0 if watchdog is disabled, else non zero.
> + */
> +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> +{
> +	unsigned int val;
> +
> +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
> +
> +	return val & DA9063_TWDSCALE_MASK;
> +}
> +
>  static int da9063_wdt_disable_timer(struct da9063 *da9063)
>  {
>  	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> @@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>  {
>  	struct da9063 *da9063;
>  	struct watchdog_device *wdd;
> +	unsigned int cur_timeout;
>  
>  	if (!pdev->dev.parent)
>  		return -EINVAL;
> @@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>  
>  	watchdog_set_drvdata(wdd, da9063);
>  
> +	cur_timeout = da9063_wdt_is_running(da9063);
> +	if (cur_timeout) {
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +		_da9063_wdt_set_timeout(da9063, cur_timeout);

Sorry, I should have been more specific. That doesn't make sense as written
since it just sets the same timeout as before. You would have to replace
the second argument with the desired timeout in seconds, and call
da9063_wdt_timeout_to_sel() in _da9063_wdt_set_timeout(). That would
actually make sense since all callers of _da9063_wdt_set_timeout()
do that anyway before the call.

Guenter

> +	}
> +
>  	return devm_watchdog_register_device(&pdev->dev, wdd);
>  }
>  
> -- 
> 2.17.0
> 

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Marco Felsch]

On 18-05-24 11:07, Guenter Roeck wrote:
> On Thu, May 24, 2018 at 01:51:00PM +0200, Marco Felsch wrote:
> > The watchdog can be enabled in previous steps (e.g. the bootloader). Check
> > if the watchdog is already running, retrieve the set timeout value and
> > set it again to set the new timeout reference mark.
> > 
> > Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> > index 6b0092b7d5a6..c5bd5ffe8ded 100644
> > --- a/drivers/watchdog/da9063_wdt.c
> > +++ b/drivers/watchdog/da9063_wdt.c
> > @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
> >  	return DA9063_TWDSCALE_MAX;
> >  }
> >  
> > +/*
> > + * Return 0 if watchdog is disabled, else non zero.
> > + */
> > +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> > +{
> > +	unsigned int val;
> > +
> > +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
> > +
> > +	return val & DA9063_TWDSCALE_MASK;
> > +}
> > +
> >  static int da9063_wdt_disable_timer(struct da9063 *da9063)
> >  {
> >  	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> > @@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> >  {
> >  	struct da9063 *da9063;
> >  	struct watchdog_device *wdd;
> > +	unsigned int cur_timeout;
> >  
> >  	if (!pdev->dev.parent)
> >  		return -EINVAL;
> > @@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> >  
> >  	watchdog_set_drvdata(wdd, da9063);
> >  
> > +	cur_timeout = da9063_wdt_is_running(da9063);
> > +	if (cur_timeout) {
> > +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> > +		_da9063_wdt_set_timeout(da9063, cur_timeout);
> 
> Sorry, I should have been more specific. That doesn't make sense as written
> since it just sets the same timeout as before. You would have to replace
> the second argument with the desired timeout in seconds, and call
> da9063_wdt_timeout_to_sel() in _da9063_wdt_set_timeout(). That would
> actually make sense since all callers of _da9063_wdt_set_timeout()
> do that anyway before the call.

That's the reason why I used da9063_wdt_set_timeout() in v6 because in
v6 da9063_wdt_is_running() has returned the mapped timeout in seconds.

However, I'm with you to move da9063_wdt_timeout_to_sel() to
_da9063_wdt_set_timeout(). Should that happen in a seperate patch?

Yes it sets the same timeout as before but I didn't want change the timeout
value without the user knowledge. Should I rather use the ping()
function or is it okay to change the timeout value to a different value
as set before by the bootloader?

Marco
> 
> Guenter
> 
> > +	}
> > +
> >  	return devm_watchdog_register_device(&pdev->dev, wdd);
> >  }
> >  
> > -- 
> > 2.17.0
> > 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5082 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Guenter Roeck]

On 05/24/2018 11:44 PM, Marco Felsch wrote:
> On 18-05-24 11:07, Guenter Roeck wrote:
>> On Thu, May 24, 2018 at 01:51:00PM +0200, Marco Felsch wrote:
>>> The watchdog can be enabled in previous steps (e.g. the bootloader). Check
>>> if the watchdog is already running, retrieve the set timeout value and
>>> set it again to set the new timeout reference mark.
>>>
>>> Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>>> ---
>>>   drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
>>>   1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
>>> index 6b0092b7d5a6..c5bd5ffe8ded 100644
>>> --- a/drivers/watchdog/da9063_wdt.c
>>> +++ b/drivers/watchdog/da9063_wdt.c
>>> @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
>>>   	return DA9063_TWDSCALE_MAX;
>>>   }
>>>   
>>> +/*
>>> + * Return 0 if watchdog is disabled, else non zero.
>>> + */
>>> +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
>>> +{
>>> +	unsigned int val;
>>> +
>>> +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
>>> +
>>> +	return val & DA9063_TWDSCALE_MASK;
>>> +}
>>> +
>>>   static int da9063_wdt_disable_timer(struct da9063 *da9063)
>>>   {
>>>   	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
>>> @@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>>>   {
>>>   	struct da9063 *da9063;
>>>   	struct watchdog_device *wdd;
>>> +	unsigned int cur_timeout;
>>>   
>>>   	if (!pdev->dev.parent)
>>>   		return -EINVAL;
>>> @@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>>>   
>>>   	watchdog_set_drvdata(wdd, da9063);
>>>   
>>> +	cur_timeout = da9063_wdt_is_running(da9063);
>>> +	if (cur_timeout) {
>>> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
>>> +		_da9063_wdt_set_timeout(da9063, cur_timeout);
>>
>> Sorry, I should have been more specific. That doesn't make sense as written
>> since it just sets the same timeout as before. You would have to replace
>> the second argument with the desired timeout in seconds, and call
>> da9063_wdt_timeout_to_sel() in _da9063_wdt_set_timeout(). That would
>> actually make sense since all callers of _da9063_wdt_set_timeout()
>> do that anyway before the call.
> 
> That's the reason why I used da9063_wdt_set_timeout() in v6 because in
> v6 da9063_wdt_is_running() has returned the mapped timeout in seconds.
> 
> However, I'm with you to move da9063_wdt_timeout_to_sel() to
> _da9063_wdt_set_timeout(). Should that happen in a seperate patch?
> 
Yes, that would be better.

> Yes it sets the same timeout as before but I didn't want change the timeout
> value without the user knowledge. Should I rather use the ping()
> function or is it okay to change the timeout value to a different value
> as set before by the bootloader?
> 

I thought that changing the timeout to the configured or default value was
the idea.

Guenter

> Marco
>>
>> Guenter
>>
>>> +	}
>>> +
>>>   	return devm_watchdog_register_device(&pdev->dev, wdd);
>>>   }
>>>   
>>> -- 
>>> 2.17.0
>>>
>>
> 

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Marco Felsch]

On 18-05-25 01:50, Guenter Roeck wrote:
> On 05/24/2018 11:44 PM, Marco Felsch wrote:
> > On 18-05-24 11:07, Guenter Roeck wrote:
> > > On Thu, May 24, 2018 at 01:51:00PM +0200, Marco Felsch wrote:
> > > > The watchdog can be enabled in previous steps (e.g. the bootloader). Check
> > > > if the watchdog is already running, retrieve the set timeout value and
> > > > set it again to set the new timeout reference mark.
> > > > 
> > > > Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >   drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
> > > >   1 file changed, 19 insertions(+)
> > > > 
> > > > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> > > > index 6b0092b7d5a6..c5bd5ffe8ded 100644
> > > > --- a/drivers/watchdog/da9063_wdt.c
> > > > +++ b/drivers/watchdog/da9063_wdt.c
> > > > @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
> > > >   	return DA9063_TWDSCALE_MAX;
> > > >   }
> > > > +/*
> > > > + * Return 0 if watchdog is disabled, else non zero.
> > > > + */
> > > > +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> > > > +{
> > > > +	unsigned int val;
> > > > +
> > > > +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
> > > > +
> > > > +	return val & DA9063_TWDSCALE_MASK;
> > > > +}
> > > > +
> > > >   static int da9063_wdt_disable_timer(struct da9063 *da9063)
> > > >   {
> > > >   	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> > > > @@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> > > >   {
> > > >   	struct da9063 *da9063;
> > > >   	struct watchdog_device *wdd;
> > > > +	unsigned int cur_timeout;
> > > >   	if (!pdev->dev.parent)
> > > >   		return -EINVAL;
> > > > @@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> > > >   	watchdog_set_drvdata(wdd, da9063);
> > > > +	cur_timeout = da9063_wdt_is_running(da9063);
> > > > +	if (cur_timeout) {
> > > > +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> > > > +		_da9063_wdt_set_timeout(da9063, cur_timeout);
> > > 
> > > Sorry, I should have been more specific. That doesn't make sense as written
> > > since it just sets the same timeout as before. You would have to replace
> > > the second argument with the desired timeout in seconds, and call
> > > da9063_wdt_timeout_to_sel() in _da9063_wdt_set_timeout(). That would
> > > actually make sense since all callers of _da9063_wdt_set_timeout()
> > > do that anyway before the call.
> > 
> > That's the reason why I used da9063_wdt_set_timeout() in v6 because in
> > v6 da9063_wdt_is_running() has returned the mapped timeout in seconds.
> > 
> > However, I'm with you to move da9063_wdt_timeout_to_sel() to
> > _da9063_wdt_set_timeout(). Should that happen in a seperate patch?
> > 
> Yes, that would be better.

We can't move it to the _da9063_wdt_set_timeout() because the
da9063_wdt_set_timeout() uses the mapped timeouts later:

static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
                                  unsigned int timeout)
{
	[ ... ]
        else
		wdd->timeout = wdt_timeout[selector];

	return ret;

}

> > Yes it sets the same timeout as before but I didn't want change the timeout
> > value without the user knowledge. Should I rather use the ping()
> > function or is it okay to change the timeout value to a different value
> > as set before by the bootloader?
> > 
> 
> I thought that changing the timeout to the configured or default value was
> the idea.

Okay, now I set the default timeout DA9063_WDG_TIMEOUT.

Marco

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Guenter Roeck]

On 05/25/2018 06:09 AM, Marco Felsch wrote:
> On 18-05-25 01:50, Guenter Roeck wrote:
>> On 05/24/2018 11:44 PM, Marco Felsch wrote:
>>> On 18-05-24 11:07, Guenter Roeck wrote:
>>>> On Thu, May 24, 2018 at 01:51:00PM +0200, Marco Felsch wrote:
>>>>> The watchdog can be enabled in previous steps (e.g. the bootloader). Check
>>>>> if the watchdog is already running, retrieve the set timeout value and
>>>>> set it again to set the new timeout reference mark.
>>>>>
>>>>> Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
>>>>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
>>>>> ---
>>>>>    drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
>>>>>    1 file changed, 19 insertions(+)
>>>>>
>>>>> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
>>>>> index 6b0092b7d5a6..c5bd5ffe8ded 100644
>>>>> --- a/drivers/watchdog/da9063_wdt.c
>>>>> +++ b/drivers/watchdog/da9063_wdt.c
>>>>> @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
>>>>>    	return DA9063_TWDSCALE_MAX;
>>>>>    }
>>>>> +/*
>>>>> + * Return 0 if watchdog is disabled, else non zero.
>>>>> + */
>>>>> +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
>>>>> +{
>>>>> +	unsigned int val;
>>>>> +
>>>>> +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
>>>>> +
>>>>> +	return val & DA9063_TWDSCALE_MASK;
>>>>> +}
>>>>> +
>>>>>    static int da9063_wdt_disable_timer(struct da9063 *da9063)
>>>>>    {
>>>>>    	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
>>>>> @@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>>>>>    {
>>>>>    	struct da9063 *da9063;
>>>>>    	struct watchdog_device *wdd;
>>>>> +	unsigned int cur_timeout;
>>>>>    	if (!pdev->dev.parent)
>>>>>    		return -EINVAL;
>>>>> @@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>>>>>    	watchdog_set_drvdata(wdd, da9063);
>>>>> +	cur_timeout = da9063_wdt_is_running(da9063);
>>>>> +	if (cur_timeout) {
>>>>> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
>>>>> +		_da9063_wdt_set_timeout(da9063, cur_timeout);
>>>>
>>>> Sorry, I should have been more specific. That doesn't make sense as written
>>>> since it just sets the same timeout as before. You would have to replace
>>>> the second argument with the desired timeout in seconds, and call
>>>> da9063_wdt_timeout_to_sel() in _da9063_wdt_set_timeout(). That would
>>>> actually make sense since all callers of _da9063_wdt_set_timeout()
>>>> do that anyway before the call.
>>>
>>> That's the reason why I used da9063_wdt_set_timeout() in v6 because in
>>> v6 da9063_wdt_is_running() has returned the mapped timeout in seconds.
>>>
>>> However, I'm with you to move da9063_wdt_timeout_to_sel() to
>>> _da9063_wdt_set_timeout(). Should that happen in a seperate patch?
>>>
>> Yes, that would be better.
> 
> We can't move it to the _da9063_wdt_set_timeout() because the
> da9063_wdt_set_timeout() uses the mapped timeouts later:
> 
> static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>                                    unsigned int timeout)
> {
> 	[ ... ]
>          else
> 		wdd->timeout = wdt_timeout[selector];
> 
> 	return ret;
> 
> }
> 
	...
	if (ret)
		return ret;

         wdd->timeout = wdt_timeout[da9063_wdt_timeout_to_sel(timeout)];
	return 0;

Guenter

>>> Yes it sets the same timeout as before but I didn't want change the timeout
>>> value without the user knowledge. Should I rather use the ping()
>>> function or is it okay to change the timeout value to a different value
>>> as set before by the bootloader?
>>>
>>
>> I thought that changing the timeout to the configured or default value was
>> the idea.
> 
> Okay, now I set the default timeout DA9063_WDG_TIMEOUT.
> 
> Marco
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Marco Felsch]

Dear Guenter,

On 18-05-25 06:46, Guenter Roeck wrote:
> On 05/25/2018 06:09 AM, Marco Felsch wrote:
> > On 18-05-25 01:50, Guenter Roeck wrote:
> > > On 05/24/2018 11:44 PM, Marco Felsch wrote:
> > > > On 18-05-24 11:07, Guenter Roeck wrote:
> > > > > On Thu, May 24, 2018 at 01:51:00PM +0200, Marco Felsch wrote:
> > > > > > The watchdog can be enabled in previous steps (e.g. the bootloader). Check
> > > > > > if the watchdog is already running, retrieve the set timeout value and
> > > > > > set it again to set the new timeout reference mark.
> > > > > > 
> > > > > > Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > ---
> > > > > >    drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
> > > > > >    1 file changed, 19 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> > > > > > index 6b0092b7d5a6..c5bd5ffe8ded 100644
> > > > > > --- a/drivers/watchdog/da9063_wdt.c
> > > > > > +++ b/drivers/watchdog/da9063_wdt.c
> > > > > > @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
> > > > > >    	return DA9063_TWDSCALE_MAX;
> > > > > >    }
> > > > > > +/*
> > > > > > + * Return 0 if watchdog is disabled, else non zero.
> > > > > > + */
> > > > > > +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> > > > > > +{
> > > > > > +	unsigned int val;
> > > > > > +
> > > > > > +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
> > > > > > +
> > > > > > +	return val & DA9063_TWDSCALE_MASK;
> > > > > > +}
> > > > > > +
> > > > > >    static int da9063_wdt_disable_timer(struct da9063 *da9063)
> > > > > >    {
> > > > > >    	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> > > > > > @@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> > > > > >    {
> > > > > >    	struct da9063 *da9063;
> > > > > >    	struct watchdog_device *wdd;
> > > > > > +	unsigned int cur_timeout;
> > > > > >    	if (!pdev->dev.parent)
> > > > > >    		return -EINVAL;
> > > > > > @@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> > > > > >    	watchdog_set_drvdata(wdd, da9063);
> > > > > > +	cur_timeout = da9063_wdt_is_running(da9063);
> > > > > > +	if (cur_timeout) {
> > > > > > +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> > > > > > +		_da9063_wdt_set_timeout(da9063, cur_timeout);
> > > > > 
> > > > > Sorry, I should have been more specific. That doesn't make sense as written
> > > > > since it just sets the same timeout as before. You would have to replace
> > > > > the second argument with the desired timeout in seconds, and call
> > > > > da9063_wdt_timeout_to_sel() in _da9063_wdt_set_timeout(). That would
> > > > > actually make sense since all callers of _da9063_wdt_set_timeout()
> > > > > do that anyway before the call.
> > > > 
> > > > That's the reason why I used da9063_wdt_set_timeout() in v6 because in
> > > > v6 da9063_wdt_is_running() has returned the mapped timeout in seconds.
> > > > 
> > > > However, I'm with you to move da9063_wdt_timeout_to_sel() to
> > > > _da9063_wdt_set_timeout(). Should that happen in a seperate patch?
> > > > 
> > > Yes, that would be better.
> > 
> > We can't move it to the _da9063_wdt_set_timeout() because the
> > da9063_wdt_set_timeout() uses the mapped timeouts later:
> > 
> > static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
> >                                    unsigned int timeout)
> > {
> > 	[ ... ]
> >          else
> > 		wdd->timeout = wdt_timeout[selector];
> > 
> > 	return ret;
> > 
> > }
> > 
> 	...
> 	if (ret)
> 		return ret;
> 
>         wdd->timeout = wdt_timeout[da9063_wdt_timeout_to_sel(timeout)];
> 	return 0;
> 
> Guenter

Sorry, I tought da9063_wdt_timeout_to_sel() should get called only within
_da9063_wdt_set_timeout(). 

Marco

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Guenter Roeck]

On Fri, May 25, 2018 at 11:41:57PM +0200, Marco Felsch wrote:
> Dear Guenter,
> 
> On 18-05-25 06:46, Guenter Roeck wrote:
> > On 05/25/2018 06:09 AM, Marco Felsch wrote:
> > > On 18-05-25 01:50, Guenter Roeck wrote:
> > > > On 05/24/2018 11:44 PM, Marco Felsch wrote:
> > > > > On 18-05-24 11:07, Guenter Roeck wrote:
> > > > > > On Thu, May 24, 2018 at 01:51:00PM +0200, Marco Felsch wrote:
> > > > > > > The watchdog can be enabled in previous steps (e.g. the bootloader). Check
> > > > > > > if the watchdog is already running, retrieve the set timeout value and
> > > > > > > set it again to set the new timeout reference mark.
> > > > > > > 
> > > > > > > Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > > ---
> > > > > > >    drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
> > > > > > >    1 file changed, 19 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> > > > > > > index 6b0092b7d5a6..c5bd5ffe8ded 100644
> > > > > > > --- a/drivers/watchdog/da9063_wdt.c
> > > > > > > +++ b/drivers/watchdog/da9063_wdt.c
> > > > > > > @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
> > > > > > >    	return DA9063_TWDSCALE_MAX;
> > > > > > >    }
> > > > > > > +/*
> > > > > > > + * Return 0 if watchdog is disabled, else non zero.
> > > > > > > + */
> > > > > > > +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> > > > > > > +{
> > > > > > > +	unsigned int val;
> > > > > > > +
> > > > > > > +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
> > > > > > > +
> > > > > > > +	return val & DA9063_TWDSCALE_MASK;
> > > > > > > +}
> > > > > > > +
> > > > > > >    static int da9063_wdt_disable_timer(struct da9063 *da9063)
> > > > > > >    {
> > > > > > >    	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> > > > > > > @@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> > > > > > >    {
> > > > > > >    	struct da9063 *da9063;
> > > > > > >    	struct watchdog_device *wdd;
> > > > > > > +	unsigned int cur_timeout;
> > > > > > >    	if (!pdev->dev.parent)
> > > > > > >    		return -EINVAL;
> > > > > > > @@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> > > > > > >    	watchdog_set_drvdata(wdd, da9063);
> > > > > > > +	cur_timeout = da9063_wdt_is_running(da9063);
> > > > > > > +	if (cur_timeout) {
> > > > > > > +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> > > > > > > +		_da9063_wdt_set_timeout(da9063, cur_timeout);
> > > > > > 
> > > > > > Sorry, I should have been more specific. That doesn't make sense as written
> > > > > > since it just sets the same timeout as before. You would have to replace
> > > > > > the second argument with the desired timeout in seconds, and call
> > > > > > da9063_wdt_timeout_to_sel() in _da9063_wdt_set_timeout(). That would
> > > > > > actually make sense since all callers of _da9063_wdt_set_timeout()
> > > > > > do that anyway before the call.
> > > > > 
> > > > > That's the reason why I used da9063_wdt_set_timeout() in v6 because in
> > > > > v6 da9063_wdt_is_running() has returned the mapped timeout in seconds.
> > > > > 
> > > > > However, I'm with you to move da9063_wdt_timeout_to_sel() to
> > > > > _da9063_wdt_set_timeout(). Should that happen in a seperate patch?
> > > > > 
> > > > Yes, that would be better.
> > > 
> > > We can't move it to the _da9063_wdt_set_timeout() because the
> > > da9063_wdt_set_timeout() uses the mapped timeouts later:
> > > 
> > > static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
> > >                                    unsigned int timeout)
> > > {
> > > 	[ ... ]
> > >          else
> > > 		wdd->timeout = wdt_timeout[selector];
> > > 
> > > 	return ret;
> > > 
> > > }
> > > 
> > 	...
> > 	if (ret)
> > 		return ret;
> > 
> >         wdd->timeout = wdt_timeout[da9063_wdt_timeout_to_sel(timeout)];
> > 	return 0;
> > 
> > Guenter
> 
> Sorry, I tought da9063_wdt_timeout_to_sel() should get called only within
> _da9063_wdt_set_timeout(). 
> 
Well, you _could_ have _da9063_wdt_set_timeout() return either a negative
error code or the actually selected timeout. Just an idea.

Guenter

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Marco Felsch]

On 18-05-25 15:42, Guenter Roeck wrote:
> On Fri, May 25, 2018 at 11:41:57PM +0200, Marco Felsch wrote:
> > Dear Guenter,
> > 
> > On 18-05-25 06:46, Guenter Roeck wrote:
> > > On 05/25/2018 06:09 AM, Marco Felsch wrote:
> > > > On 18-05-25 01:50, Guenter Roeck wrote:
> > > > > On 05/24/2018 11:44 PM, Marco Felsch wrote:
> > > > > > On 18-05-24 11:07, Guenter Roeck wrote:
> > > > > > > On Thu, May 24, 2018 at 01:51:00PM +0200, Marco Felsch wrote:
> > > > > > > > The watchdog can be enabled in previous steps (e.g. the bootloader). Check
> > > > > > > > if the watchdog is already running, retrieve the set timeout value and
> > > > > > > > set it again to set the new timeout reference mark.
> > > > > > > > 
> > > > > > > > Fixes: 5e9c16e37608 ("watchdog: Add DA9063 PMIC watchdog driver.")
> > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > > > ---
> > > > > > > >    drivers/watchdog/da9063_wdt.c | 19 +++++++++++++++++++
> > > > > > > >    1 file changed, 19 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> > > > > > > > index 6b0092b7d5a6..c5bd5ffe8ded 100644
> > > > > > > > --- a/drivers/watchdog/da9063_wdt.c
> > > > > > > > +++ b/drivers/watchdog/da9063_wdt.c
> > > > > > > > @@ -45,6 +45,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
> > > > > > > >    	return DA9063_TWDSCALE_MAX;
> > > > > > > >    }
> > > > > > > > +/*
> > > > > > > > + * Return 0 if watchdog is disabled, else non zero.
> > > > > > > > + */
> > > > > > > > +static unsigned int da9063_wdt_is_running(struct da9063 *da9063)
> > > > > > > > +{
> > > > > > > > +	unsigned int val;
> > > > > > > > +
> > > > > > > > +	regmap_read(da9063->regmap, DA9063_REG_CONTROL_D, &val);
> > > > > > > > +
> > > > > > > > +	return val & DA9063_TWDSCALE_MASK;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >    static int da9063_wdt_disable_timer(struct da9063 *da9063)
> > > > > > > >    {
> > > > > > > >    	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
> > > > > > > > @@ -180,6 +192,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> > > > > > > >    {
> > > > > > > >    	struct da9063 *da9063;
> > > > > > > >    	struct watchdog_device *wdd;
> > > > > > > > +	unsigned int cur_timeout;
> > > > > > > >    	if (!pdev->dev.parent)
> > > > > > > >    		return -EINVAL;
> > > > > > > > @@ -206,6 +219,12 @@ static int da9063_wdt_probe(struct platform_device *pdev)
> > > > > > > >    	watchdog_set_drvdata(wdd, da9063);
> > > > > > > > +	cur_timeout = da9063_wdt_is_running(da9063);
> > > > > > > > +	if (cur_timeout) {
> > > > > > > > +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> > > > > > > > +		_da9063_wdt_set_timeout(da9063, cur_timeout);
> > > > > > > 
> > > > > > > Sorry, I should have been more specific. That doesn't make sense as written
> > > > > > > since it just sets the same timeout as before. You would have to replace
> > > > > > > the second argument with the desired timeout in seconds, and call
> > > > > > > da9063_wdt_timeout_to_sel() in _da9063_wdt_set_timeout(). That would
> > > > > > > actually make sense since all callers of _da9063_wdt_set_timeout()
> > > > > > > do that anyway before the call.
> > > > > > 
> > > > > > That's the reason why I used da9063_wdt_set_timeout() in v6 because in
> > > > > > v6 da9063_wdt_is_running() has returned the mapped timeout in seconds.
> > > > > > 
> > > > > > However, I'm with you to move da9063_wdt_timeout_to_sel() to
> > > > > > _da9063_wdt_set_timeout(). Should that happen in a seperate patch?
> > > > > > 
> > > > > Yes, that would be better.
> > > > 
> > > > We can't move it to the _da9063_wdt_set_timeout() because the
> > > > da9063_wdt_set_timeout() uses the mapped timeouts later:
> > > > 
> > > > static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
> > > >                                    unsigned int timeout)
> > > > {
> > > > 	[ ... ]
> > > >          else
> > > > 		wdd->timeout = wdt_timeout[selector];
> > > > 
> > > > 	return ret;
> > > > 
> > > > }
> > > > 
> > > 	...
> > > 	if (ret)
> > > 		return ret;
> > > 
> > >         wdd->timeout = wdt_timeout[da9063_wdt_timeout_to_sel(timeout)];
> > > 	return 0;
> > > 
> > > Guenter
> > 
> > Sorry, I tought da9063_wdt_timeout_to_sel() should get called only within
> > _da9063_wdt_set_timeout(). 
> > 
> Well, you _could_ have _da9063_wdt_set_timeout() return either a negative
> error code or the actually selected timeout. Just an idea.

Yes, I've tought about this approach too. Anyway, is it okay to split
the series into a "fixes" series and a "improvment" series? I think we
losing sight of the initial problem "Change the timeout value more than
once." A next improvment could be to squash the da9062/1 and da9063 into
one if this is possible.

Marco

Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Guenter Roeck]

On 05/26/2018 10:39 AM, Marco Felsch wrote:

>>>>> We can't move it to the _da9063_wdt_set_timeout() because the
>>>>> da9063_wdt_set_timeout() uses the mapped timeouts later:
>>>>>
>>>>> static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>>>>>                                     unsigned int timeout)
>>>>> {
>>>>> 	[ ... ]
>>>>>           else
>>>>> 		wdd->timeout = wdt_timeout[selector];
>>>>>
>>>>> 	return ret;
>>>>>
>>>>> }
>>>>>
>>>> 	...
>>>> 	if (ret)
>>>> 		return ret;
>>>>
>>>>          wdd->timeout = wdt_timeout[da9063_wdt_timeout_to_sel(timeout)];
>>>> 	return 0;
>>>>
>>>> Guenter
>>>
>>> Sorry, I tought da9063_wdt_timeout_to_sel() should get called only within
>>> _da9063_wdt_set_timeout().
>>>
>> Well, you _could_ have _da9063_wdt_set_timeout() return either a negative
>> error code or the actually selected timeout. Just an idea.
> 
> Yes, I've tought about this approach too. Anyway, is it okay to split
> the series into a "fixes" series and a "improvment" series? I think we

Yes

> losing sight of the initial problem "Change the timeout value more than
> once." A next improvment could be to squash the da9062/1 and da9063 into
> one if this is possible.
> 
Sure. Next step, though.

Thanks,
Guenter

RE: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe

[Steve Twiss]

On 27 May 2018 00:12 Guenter Roeck wrote:
> Subject: Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe
> 
> On 05/26/2018 10:39 AM, Marco Felsch wrote:
> 
> >
> > Yes, I've tought about this approach too. Anyway, is it okay to split
> > the series into a "fixes" series and a "improvment" series? I think we
> 
> Yes
> 
> > losing sight of the initial problem "Change the timeout value more than
> > once." A next improvment could be to squash the da9062/1 and da9063 into
> > one if this is possible.
> >
> Sure. Next step, though.
> 

There is a difference between the watchdog IP on the da9062/61 and that on
the da9063. Typically however, although there is a difference in the IP there is
not much difference between the driver implementation -- at the moment.

I did suggest there would be an addition to the da9062/61 device driver to
include the new IP and differentiate it, but I have not had time to reach that
point yet.

Regards,
Steve