Re: [PATCH v5 5/5] rtc: pcf85363: add watchdog support with configurable step size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Watchdog is neither stopped nor adopted on probe, leading to spurious periodic interrupts across reboots.
--

commit a09fcbd2e909232c9247652551e0a3249e36a82e
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: pcf85363: add watchdog support with configurable step size

Add watchdog support to PCF85263/PCF85363 using the watchdog subsystem.
The 5-bit count selects the timeout with a clock step: timeouts up to
30 seconds use the 1 Hz step, longer ones the 0.25 Hz step (max 120 s),
reported back through the watchdog device. One count of margin is added
because the first period after a reload lasts between WDR and WDR-1
counts, so the timeout is never shorter than requested. Select
WATCHDOG_CORE if WATCHDOG so the device can register.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 9da4b1617f82a..2046643788fdc 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -691,6 +725,153 @@ static const struct pcf85x63_config pcf_85363_config = {
>  	.num_nvram = 2
>  };
[ ... ]
> +static int pcf85363_watchdog_init(struct device *dev, struct pcf85363 *pcf85363)
> +{
> +	struct pcf85363_watchdog *wd;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_WATCHDOG))
> +		return 0;
> +
> +	wd = devm_kzalloc(dev, sizeof(*wd), GFP_KERNEL);
> +	if (!wd)
> +		return -ENOMEM;
> +
> +	wd->pcf85363 = pcf85363;
> +
> +	wd->wdd.info = &pcf85363_wdt_info;
> +	wd->wdd.ops = &pcf85363_wdt_ops;
> +	wd->wdd.min_timeout = WD_TIMEOUT_MIN;
> +	wd->wdd.max_timeout = WD_TIMEOUT_MAX;
> +	wd->wdd.timeout = WD_DEFAULT_TIMEOUT;
> +	wd->wdd.parent = dev;
> +	wd->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +	/* Fixed default timeout; userspace can change it via WDIOC_SETTIMEOUT. */
> +	pcf85363_wdt_select_clock(wd);
> +
> +	ret = pcf85363_clear_flags(pcf85363, FLAGS_WDF);
> +	if (ret) {
> +		dev_err(dev, "failed to clear WDF:%d\n", ret);
> +		return ret;
> +	}
> +
> +	watchdog_set_drvdata(&wd->wdd, wd);
> +
> +	return devm_watchdog_register_device(dev, &wd->wdd);

[Severity: Medium]
Since the watchdog configuration registers (CTRL_WDOG and CTRL_INTA_EN) are
battery-backed and persist across system reboots, could the watchdog already
be running when the driver probes?

The driver does not clear the watchdog interrupt enable bit (WDIE) during
probe, and pcf85363_watchdog_init() registers the watchdog but neither stops
the hardware nor sets WDOG_HW_RUNNING in wdd.status.

Without WDOG_HW_RUNNING, the watchdog core will be unaware the hardware is
active and will not automatically ping it.

If the watchdog was started before a reboot or by a bootloader, will this
cause it to continuously expire and perpetually trigger the interrupt
handler, resulting in repeated "watchdog timer expired" warnings in the
logs?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910050747.1901440-1-lakshay.piplani@nxp.com?part=5