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

[Marco Felsch <m.felsch@pengutronix.de>]

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