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@xxxxxxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
