On Wed, Nov 11, 2020 at 03:15:17PM +0000, Vaittinen, Matti wrote:
[ ... ]

quoted

quoted

quoted

+
+	priv->wdd.info			= &bd957x_wdt_ident;
+	priv->wdd.ops			= &bd957x_wdt_ops;
+	priv->wdd.min_hw_heartbeat_ms	= hw_margin_min;
+	priv->wdd.max_hw_heartbeat_ms	= hw_margin_max;
+	priv->wdd.parent		= dev;
+	priv->wdd.timeout		= (hw_margin_max / 2) * 1000;

Hmm. Just noticed this value does not make sense, right?
Maximum hw_margin is 4416 ms. If I read this correctly timeout
should
be in seconds -  so result is around 2 000 000 seconds here. I
think it
is useless value...

Perhaps
	priv->wdd.timeout		= (hw_margin_max / 2) / 1000;
	if (!priv->wdd.timeout)
		priv->wdd.timeout = 1;
would be more appropriate.

Yes. Good catch. Actually, since max_hw_heartbeat_ms is specified,
it can and should be a reasonable constant (like the usual 30
seconds).
It does not and should not be bound by max_hw_heartbeat_ms.

Thanks for confirming this Guenter. I'd better admit I didn't
understand how the max_hw_heartbeat_ms works.

If I now read the code correctly, the "watchdog worker" takes care of
feeding for shorter periods than the "timeout" - and only stops
feeding max_hw_heartbeat_ms before timeout expires if userland has not
been feedin wdg. This is really cool approach for short(ish)
max_hw_heartbeat_ms configurations as user-space does not need to meet
"RT requirements". WDG framework is much more advanced that I knew :)

Yes, exactly, that is the idea. Various drivers used to implement this
within the driver, so it just made sense to pull the functionality into
the watchdog core.

Thanks,
Guenter