Hi Juuso,
On 2026-07-10T04:51:17, Juuso Rinta <[email protected]> wrote:
> watchdog: wdt-uclass: add get_timeout operation and helper
>
> The wdt_start function may be called with a timeout greater
> than the hardware-supported maximum. This in turn can result
> to bogus print, e.g.
> Started <watchdog@> with servicing every 1000ms (60s timeout)
>
> Add a uclass get_timeout operation and helper so drivers can
> report the effective timeout. When available, the startup
> print now shows both actual and requested timeout values, e.g.
> Started <watchdog@> with servicing every 1000ms (10s timeout, requested 60s)
>
> Signed-off-by: Juuso Rinta <[email protected]>
>
> drivers/watchdog/wdt-uclass.c | 35 +++++++++++++++++++++++++++++++++--
> include/wdt.h | 22 ++++++++++++++++++++++
> 2 files changed, 55 insertions(+), 2 deletions(-)
Assuming this is a static value, it might be more efficient (code size
and simplicity) to add a max_timeout_ms (max timeout in milliseconds)
uint to the per-device uclass-plat data - 0 could mean no limit, thus
allowing existing drivers to not set it.
If you are OK with that, then a lot of my comments might become less relevant.
> The wdt_start function may be called with a timeout greater
> than the hardware-supported maximum. This in turn can result
> to bogus print, e.g.
'can result in a bogus print'. Also please use () when referring
to functions, i.e. wdt_start()
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> @@ -143,9 +161,22 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong
> flags)
> const struct wdt_ops *ops = device_get_ops(dev);
> int ret;
> + u64 effective_timeout = timeout_ms;
> + char requested_timeout[32];
These are only used in the success path, so please can you move them
into the if (!ret) block, next to the existing 'str' declaration?
Also 'requested_timeout' holds a message suffix rather than a timeout
value, so something like 'req_str' would be clearer.
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> @@ -143,9 +161,22 @@ int wdt_start(struct udevice *dev, u64 timeout_ms, ulong
> flags)
> + /* Get the effective timeout from driver if supported */
> + memset(requested_timeout, 0, 32);
> + if (wdt_get_timeout(dev, &effective_timeout) == 0) {
> + u32 effective_s = (u32)lldiv(effective_timeout, 1000);
> + u32 requested_s = (u32)lldiv(timeout_ms, 1000);
> +
> + if (effective_s != requested_s) {
> + snprintf(requested_timeout, 32, ", requested
> %ds",
> + requested_s);
> + }
> + }
There is no need to clear the whole buffer - snprintf() terminates the
string, so requested_timeout[0] = '\0' is enough. Either way, please
use sizeof(requested_timeout) rather than repeating the magic 32 here
and in the snprintf() call.
The comparison is done in whole seconds, so a request of 1500ms clamped
to 1000ms prints just '1s timeout' with no note. I suspect that is
intentional, to avoid noise from sub-second rounding, but a brief
comment saying so would help.
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> @@ -116,10 +116,28 @@ int initr_watchdog(void)
> + ret = ops->get_timeout(dev, &tout_val);
> + if (ret == 0)
> + *timeout_ms = tout_val;
> +
> + return ret;
The 'tout_val' intermediate exists only to guarantee the 'unchanged on
error' behaviour documented in the header. Since you control all the
driver implementations in this series, it would be simpler to pass
timeout_ms straight through and document that drivers must only write
it on success. What do you think?
> diff --git a/include/wdt.h b/include/wdt.h
> @@ -74,6 +74,19 @@ int wdt_expire_now(struct udevice *dev, ulong flags);
> + * @dev: WDT device
> + * @timeout_ms: A pointer where the effective timeout in milliseconds
> + * is returned to. On error, the value is left unchanged.
> + * @return 0 if OK -ve on error
Please match the format used elsewhere in this file: '@return: 0 if
OK, -ve on error'. The @timeout_ms description could be tightened too,
e.g. 'Returns the effective timeout in milliseconds; unchanged on
error'
Regards,
Simon
