Re: [PATCH v6 5/5] watchdog: aaeon: Add watchdog driver for SRG-IMX8P MCU

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing PM hooks to stop the hardware watchdog during system suspend.
- [Medium] The `timeout-sec` device tree property is ignored because the driver reads it from the child MFD device instead of the parent device.
--

commit 0395f10fd7f7a423d535b7eff45bb1cf967808e5
Author: Thomas Perrot (Schneider Electric) <[email protected]>

watchdog: aaeon: Add watchdog driver for SRG-IMX8P MCU

Add watchdog driver for the Aaeon SRG-IMX8P embedded controller.
This driver provides system monitoring and recovery capabilities
through the MCU's watchdog timer.

> diff --git a/drivers/watchdog/aaeon_mcu_wdt.c b/drivers/watchdog/aaeon_mcu_wdt.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/watchdog/aaeon_mcu_wdt.c

[ ... ]

> +static int aaeon_mcu_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;

[ ... ]

> +	/*
> +	 * The MCU firmware has a fixed hardware timeout of 25 seconds that
> +	 * cannot be changed. The watchdog core handles automatic pinging to
> +	 * support software timeouts longer than the hardware limit. The default
> +	 * software timeout of 240 seconds can be overridden via the DT
> +	 * timeout-sec property or the watchdog_timeout kernel boot parameter.
> +	 */
> +	wdt->timeout = AAEON_MCU_WDT_TIMEOUT;
> +	wdt->min_timeout = AAEON_MCU_WDT_MIN_TIMEOUT;
> +	wdt->max_timeout = AAEON_MCU_WDT_MAX_TIMEOUT;
> +	wdt->max_hw_heartbeat_ms = AAEON_MCU_WDT_HEARTBEAT_MS;
> +	watchdog_init_timeout(wdt, timeout, dev);

[Severity: Medium]
Does this pass the correct device for reading the DT timeout-sec property?

Since dev is a child platform device instantiated by the MFD core, it typically
lacks an of_node. Should this be dev->parent instead, so that
device_property_read_u32() in the watchdog core can successfully find the
timeout-sec property?

> +
> +	watchdog_set_drvdata(wdt, data);
> +	watchdog_stop_on_reboot(wdt);

[ ... ]

> +	return devm_watchdog_register_device(dev, wdt);
> +}
> +
> +static struct platform_driver aaeon_mcu_wdt_driver = {
> +	.driver		= {
> +		.name	= "aaeon-mcu-wdt",
> +	},
> +	.probe		= aaeon_mcu_wdt_probe,
> +};

[Severity: High]
Will this cause unexpected system resets during suspend?

If the system enters suspend while the watchdog is active, the watchdog core's
ping worker is frozen. Since there are no PM hooks provided here to stop the
hardware timer or configure it for sleep, will the MCU's 25-second hardware
watchdog tick down and reset the board if suspend lasts longer than 25 seconds?

> +
> +module_platform_driver(aaeon_mcu_wdt_driver);
> +
> +MODULE_ALIAS("platform:aaeon-mcu-wdt");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5