[RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

On 05/27/2016 03:48 PM, Guenter Roeck wrote:

On 05/27/2016 01:25 AM, Neil Armstrong wrote:
[ ... ]

quoted

quoted

quoted

+    data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
+    data->wdt_dev.min_hw_heartbeat_ms = 1;

Does the device require a minimum time between heartbeats ?
Just asking, because you violate it yourself below.

If you want to set the minimum timeout, that would be min_timeout.

Ok, these values are not very clear actually.

Hmmm .. yes, reading the description again, it doesn't really describe well
what it is doing. Essentially, min_hw_heartbeat_ms is enforced by the watchdog
core, and should be used if a watchdog hardware can not tolerate short intervals
between heartbeats. min_timeout is the minimum timeout value configurable from
user space.

OK, I'll switch to min_timeout = 1.

[ ... ]

quoted

quoted

This is unusual. If the watchdog can be already running, it might make sense
to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
can send heartbeats until user space opens the device.

Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.

Not only that - if the watchdog _is_ already running at boot time, you should
really set WDOG_HW_RUNNING to let the watchdog core know. You status function
would come handy there.

    if (meson_gxbb_wdt_status(data))    // note the changed parameter
        set_bit(WDOG_HW_RUNNING, &data->wdt_dev.status);

Yes, it can be handy.

I will push this feature in a next version, I'll stick to a simpler behavior and check
if it would be running before the kernel starts.

Thanks,
Neil

Thanks,
Guenter