On Fri, 26 Aug 2022 at 20:30, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > > > +static int ftwdt010_wdt_restart(struct watchdog_device *wdd,
> > > > + unsigned long action, void *data)
> > > > +{
> > > > + wdd->timeout = 0;
> > > > + return ftwdt010_wdt_start(wdd);
> > >
> > > While technically correct, this crates the impression that
> > > ftwdt010_wdt_start() might return an error - which it never does.
> > > Given that, I would prefer
> > > ftwdt010_wdt_start(wdd);
> > > return 0;
> >
> > Why loose the return value? My code lets it propagate. It is
> > potentially non-zero.
> >
> No, it never is. Look at the code. That is the whole point.
Of course I looked at ftwdt010_wdt_start() and saw it always returning
0. By "potentially" I meant it returning errors too in the future.
> > > Also, did you make sure that interrupt support does not interfer with
> > > restart ? We don't want to get an interrupt when the watchdog fires due
> > > to a call to the restart handler.
> >
> > No, I did not test watchdog interrupt on my hardware (don't' know yet
> > how to set it up correctly). I only tested that restart restarts the
> > hardware as expected.
> > Now that I think of it, it may be more precise to do
> > gwdt->has_irq = false;
> > before calling ftwdt010_wdt_start()
> > So that WDCR_WDINTR flag will not be set and the chip will not trigger
> > an interrupt.
>
> In that case I'd rather see a separate function which is called from both
> ftwdt010_wdt_start() and the restart function and has timeout and the interrupt
> flag as parameters. That would also address the never-happening error return.
OK. Submitting a new patch.
