On Mon, Sep 10, 2018 at 05:36:39PM +0000, Chris Brandt wrote:
> On Monday, September 10, 2018 1, Guenter Roeck wrote:
> > > #2. If the CKS is only 4-bits, the max HW timeout is 32 seconds. (so
> > > 'timeout' can never be more that a u8).
> > >
> > That is not the point. The point is that there is no need to keep two
> > 'timeout' variables.
>
> But there was a reason.
>
> The reason was that the upper layer would call the .ping() function
> without calling .start() again.
>
> Meaning it would change 'timeout' in the structure, but not explicitly
> tell the driver.
>
It does that because there is no set_timeout function.
> That why I had to keep track of my own timeout (what the HW was actually
> set to).
>
> Every time the upper layer calls .ping(), I have to see if the timeout
> field still matches.
>
>
> > > The number "4194304" is exactly how it is documented in the hardware
> > > manual, that is why I'm using it that way. Yes, 0x400000 makes more
> > > sense, but I like matching the device documenting as much as possible to
> > > help the next person that comes along and has to debug this code.
> > >
> > Use at least a define.
>
> OK.
>
>
> > > Because when I was doing all my testing, I found cases where '.ping' was
> > > called from the upper layer without '.start' being called first. So, I
> > > changed the code as you see it now. This guaranteed I would also get the
> > > timeout the user was requesting.
> > >
> > That would only happen if the watchdog is considered to be running.
> > Also, we are talking about the set_timeout function which is the one which
> > should set the timeout and update the HW if needed, ie if the watchdog is
> > already running.
>
> This driver doesn't have .set_timeout
>
> static const struct watchdog_ops rza_wdt_ops = {
> .owner = THIS_MODULE,
> .start = rza_wdt_start,
> .stop = rza_wdt_stop,
> .ping = rza_wdt_ping,
> .restart = rza_wdt_restart,
> };
>
It wasn't needed before, but that doesn't mean it is not needed now.
>
> If you really don't like checking in .ping, I can add a set_timeout
> function and remove the local priv->timeout.
>
No, I do not like checking and setting the timeout in the ping function
at all. That is what the set_timeout function is for, and I don't see
a point in bypassing the infrastructure.
Guenter
