From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-path: Date: Sat, 26 May 2018 19:39:34 +0200 From: Marco Felsch To: Guenter Roeck Cc: wim@linux-watchdog.org, support.opensource@diasemi.com, kernel@pengutronix.de, linux-watchdog@vger.kernel.org Subject: Re: [PATCH v8 3/4] watchdog: da9063: Fix timeout handling during probe Message-ID: <20180526173934.GA12799@pengutronix.de> References: <20180524115101.17097-1-m.felsch@pengutronix.de> <20180524115101.17097-4-m.felsch@pengutronix.de> <20180524180744.GA13327@roeck-us.net> <20180525064454.x3yn2eq4xuxvdtza@pengutronix.de> <03553172-0d10-013e-5809-55d490dff8e5@roeck-us.net> <20180525130934.zimcqmdtpkijlz4r@pengutronix.de> <62e8397e-bb92-d0b9-04e5-ebb1838d1af9@roeck-us.net> <20180525214157.GA12107@pengutronix.de> <20180525224230.GA4632@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180525224230.GA4632@roeck-us.net> List-ID: 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 > > > > > > > > --- > > > > > > > > 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