diff mbox series

[RESEND,v4,5/5] rtc: pcf85363: add watchdog support with configurable step size

Message ID 20260812074533.3545058-5-lakshay.piplani@nxp.com
State New
Headers show
Series [RESEND,v4,1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config | expand

Commit Message

Lakshay Piplani Aug. 12, 2026, 7:45 a.m. UTC 
Add watchdog timer support to PCF85263/PCF85363 using the linux watchdog
subsystem. The driver programs the hardware watchdog timeout based on
the requested period.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
V3 -> V4:
- Use watchdog_init_timeout(&wd->wdd, 0, dev) to allow devicetree or module parameter overrides; 
  fallback to WD_DEFAULT_TIMEOUT if not provided.
- Centralized clock selection logic in pcf85363_wdt_select_clock() and applied dynamically 
  whenever timeout changes.
- Removed unused repeat variable and simplified timeout handling for clarity.
V2 -> V3:
- Split into separate patches as suggested:
  - Battery switch-over detection.
  - Timestamp recording for TS pin and battery switch-over events.
  - Offset calibration.
  - Watchdog timer (to be reviewed by watchdog maintainers).
- Dropped Alarm2 support
- Switched to rtc_add_group() for sysfs attributes
V1 -> V2:
- Watchdog related changes due to removal of vendor specific properties
  from device tree
  * remove vendor DT knobs (enable/timeout/stepsize/repeat)
  * use watchdog_init_timeout (with 10s default)
  * derive clock_sel from final timeout
  * default, repeat=true (repeat mode)
- Fixed uninitalised warning on 'ret' (reported by kernel test robot)
- Use dev_dbg instead of dev_info for debug related print messages
- Minor cleanup and comments.

 drivers/rtc/rtc-pcf85363.c | 156 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 154 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org Aug. 12, 2026, 7:57 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Hardware timer value is not scaled by the selected clock frequency, resulting in radically incorrect watchdog timeouts.
- [High] The watchdog initializes with a 0-second hardware timeout if the device tree lacks a timeout property.
- [High] Missing `return` statement ignores watchdog initialization failures and breaks probe deferral.

Pre-existing issues:
- [High] Read-modify-write on the write-0-to-clear `CTRL_FLAGS` register silently drops concurrent hardware interrupts.
--

--- Patch [5]: [RESEND v4 5/5] rtc: pcf85363: add watchdog support with configurable step size ---
commit b334f1fe3594f36842ec186290e4071c7236a04d
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
Subject: rtc: pcf85363: add watchdog support with configurable step size

Add watchdog timer support to PCF85263/PCF85363 using the linux watchdog
subsystem. The driver programs the hardware watchdog timeout based on
the requested period.

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 665bbbb169b05..3eb87306c83c8 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c

[ ... ]

> +static int pcf85363_wdt_reload(struct pcf85363_watchdog *wd)
> +{
> +	u8 val;
> +
> +	val = ((wd->timeout_val & WD_TIMEOUT_MAX) << WD_TIMEOUT_SHIFT) |
> +	       (wd->clock_sel & WD_CLKSEL_MASK);
> +
> +	return regmap_write(wd->regmap, CTRL_WDOG, val);
> +}

[Severity: High]
Should the timeout value be scaled by the selected clock frequency (e.g., in
ticks) before it is written to the hardware?

It appears the raw seconds value assigned from pcf85363_wdt_set_timeout() is
written directly into the hardware register here. At frequencies higher than
1Hz, this would cause the timeout to expire significantly faster than
configured.

[ ... ]

> +static int pcf85363_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> +	regmap_update_bits(wd->regmap, CTRL_FLAGS, FLAGS_WDF, 0);

[Severity: High]
This is a pre-existing issue, but since CTRL_FLAGS is a write-0-to-clear
register, can this read-modify-write cycle silently clear concurrent
hardware interrupts?

If an interrupt asserts immediately after the read step in
regmap_update_bits(), its flag will read as 0. The cycle will then write 0
back to that flag, potentially clearing the interrupt before it can be
handled. Pinging the watchdog in a background loop might exacerbate this.

[ ... ]

> +static int pcf85363_wdt_set_timeout(struct watchdog_device *wdd,
> +				    unsigned int timeout)
> +{
> +	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
> +
> +	wd->timeout_val = clamp(timeout, WD_TIMEOUT_MIN, WD_TIMEOUT_MAX);

[Severity: High]
Is the driver storing the unscaled timeout in seconds directly into the
hardware timer value here?

If this value is later used in pcf85363_wdt_reload() without being adjusted
for the selected clock frequency, the hardware timeout may be radically
incorrect.

[ ... ]

> +static int pcf85363_watchdog_init(struct device *dev, struct regmap *regmap)
> +{
[ ... ]
> +	wd->wdd.parent = dev;
> +	wd->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +	ret = watchdog_init_timeout(&wd->wdd, 0, dev);
> +	if (ret)
> +		wd->wdd.timeout = WD_DEFAULT_TIMEOUT;

[Severity: High]
Does watchdog_init_timeout() return 0 when the device tree lacks a timeout
property?

If it returns 0 without modifying wdd.timeout, this check would be bypassed,
leaving the watchdog initialized with a 0-second timeout. Setting
wd->wdd.timeout = WD_DEFAULT_TIMEOUT before calling watchdog_init_timeout()
might be safer.

[ ... ]

> @@ -685,6 +833,10 @@ static int pcf85363_probe(struct i2c_client *client)
>  			   PIN_IO_TSPM | PIN_IO_TSIM,
>  			   PIN_IO_TSPM | PIN_IO_TSIM);
>  
> +	ret = pcf85363_watchdog_init(dev, pcf85363->regmap);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Watchdog init failed\n");
> +

[Severity: High]
Is a return statement missing here?

Without it, probe failures from pcf85363_watchdog_init() such as -ENOMEM
or -EPROBE_DEFER will be discarded, allowing the device probe to falsely
succeed with a broken or uninitialized watchdog.
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 665bbbb169b0..3eb87306c83c 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -5,6 +5,10 @@ 
  * Driver for NXP PCF85363 real-time clock.
  *
  * Copyright (C) 2017 Eric Nelson
+ *
+ * Copyright 2025 NXP
+ * Added support for timestamps, battery switch-over,
+ * watchdog, offset calibration.
  */
 #include <linux/module.h>
 #include <linux/i2c.h>
@@ -18,6 +22,7 @@ 
 #include <linux/of.h>
 #include <linux/rtc.h>
 #include <linux/regmap.h>
+#include <linux/watchdog.h>
 
 /*
  * Date/Time registers
@@ -128,6 +133,17 @@ 
 #define OFFSET_MAXIMUM  127
 #define OFFSET_MASK     0xFF
 
+#define WD_TIMEOUT_SHIFT        2
+#define WD_CLKSEL_MASK  GENMASK(1, 0)
+#define WD_CLKSEL_0_25HZ        0x00
+#define WD_CLKSEL_1HZ   0x01
+#define WD_CLKSEL_4HZ   0x02
+#define WD_CLKSEL_16HZ  0x03
+
+#define WD_DEFAULT_TIMEOUT  10
+#define WD_TIMEOUT_MIN	1
+#define WD_TIMEOUT_MAX	0x1F
+
 struct pcf85363 {
 	struct rtc_device	*rtc;
 	struct regmap		*regmap;
@@ -139,6 +155,14 @@  struct pcf85x63_config {
 	unsigned int num_nvram;
 };
 
+struct pcf85363_watchdog {
+	struct watchdog_device wdd;
+	struct regmap *regmap;
+	struct device *dev;
+	u8 timeout_val;
+	u8 clock_sel;
+};
+
 static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
 {
 	u32 load = 7000;
@@ -324,12 +348,13 @@  static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	if (flags) {
-		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s\n",
+		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s%s\n",
 			flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
 			(flags & FLAGS_TSR1F) ? " [TSR1F]" : "",
 			(flags & FLAGS_TSR2F) ? " [TSR2F]" : "",
 			(flags & FLAGS_TSR3F) ? " [TSR3F]" : "",
-			(flags & FLAGS_BSF) ? " [BSF]" : "");
+			(flags & FLAGS_BSF) ? " [BSF]" : "",
+			(flags & FLAGS_WDF) ? " [WDF]" : "");
 	}
 
 	if (flags & FLAGS_A1F) {
@@ -361,6 +386,11 @@  static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 		handled = true;
 	}
 
+	if (flags & FLAGS_WDF) {
+		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_WDF, 0);
+		handled = true;
+	}
+
 	return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
@@ -504,6 +534,124 @@  static const struct pcf85x63_config pcf_85363_config = {
 	.num_nvram = 2
 };
 
+static void pcf85363_wdt_select_clock(struct pcf85363_watchdog *wd)
+{
+	unsigned int t = wd->wdd.timeout;
+
+	if (t <= 2)
+		wd->clock_sel = WD_CLKSEL_16HZ;
+	else if (t <= 8)
+		wd->clock_sel = WD_CLKSEL_4HZ;
+	else if (t <= 16)
+		wd->clock_sel = WD_CLKSEL_1HZ;
+	else
+		wd->clock_sel = WD_CLKSEL_0_25HZ;
+}
+
+/*
+ * This function sets the watchdog control register based on the timeout,
+ * clock selection and repeat mode settings. It prepares the value to
+ * write into the watchdog control register (CTRL_WDOG).
+ */
+static int pcf85363_wdt_reload(struct pcf85363_watchdog *wd)
+{
+	u8 val;
+
+	val = ((wd->timeout_val & WD_TIMEOUT_MAX) << WD_TIMEOUT_SHIFT) |
+	       (wd->clock_sel & WD_CLKSEL_MASK);
+
+	return regmap_write(wd->regmap, CTRL_WDOG, val);
+}
+
+static int pcf85363_wdt_start(struct watchdog_device *wdd)
+{
+	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+
+	return pcf85363_wdt_reload(wd);
+}
+
+static int pcf85363_wdt_stop(struct watchdog_device *wdd)
+{
+	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+
+	return regmap_write(wd->regmap, CTRL_WDOG, 0);
+}
+
+static int pcf85363_wdt_ping(struct watchdog_device *wdd)
+{
+	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+
+	regmap_update_bits(wd->regmap, CTRL_FLAGS, FLAGS_WDF, 0);
+
+	return pcf85363_wdt_reload(wd);
+}
+
+static int pcf85363_wdt_set_timeout(struct watchdog_device *wdd,
+				    unsigned int timeout)
+{
+	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+
+	wd->timeout_val = clamp(timeout, WD_TIMEOUT_MIN, WD_TIMEOUT_MAX);
+	wdd->timeout = wd->timeout_val;
+
+	pcf85363_wdt_select_clock(wd);
+
+	return pcf85363_wdt_reload(wd);
+}
+
+static const struct watchdog_info pcf85363_wdt_info = {
+	.identity = "PCF85363 Watchdog",
+	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+};
+
+static const struct watchdog_ops pcf85363_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = pcf85363_wdt_start,
+	.stop = pcf85363_wdt_stop,
+	.ping = pcf85363_wdt_ping,
+	.set_timeout = pcf85363_wdt_set_timeout,
+};
+
+static int pcf85363_watchdog_init(struct device *dev, struct regmap *regmap)
+{
+	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->regmap = regmap;
+	wd->dev = dev;
+
+	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.parent = dev;
+	wd->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+	ret = watchdog_init_timeout(&wd->wdd, 0, dev);
+	if (ret)
+		wd->wdd.timeout = WD_DEFAULT_TIMEOUT;
+
+	wd->timeout_val = wd->wdd.timeout;
+	pcf85363_wdt_select_clock(wd);
+
+	ret = regmap_update_bits(regmap, CTRL_FLAGS, FLAGS_WDF, 0);
+	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);
+}
+
 /*
  * Reads 6 bytes of timestamp data starting at the given base register,
  * converts them from BCD to binary, and formats the result into a
@@ -685,6 +833,10 @@  static int pcf85363_probe(struct i2c_client *client)
 			   PIN_IO_TSPM | PIN_IO_TSIM,
 			   PIN_IO_TSPM | PIN_IO_TSIM);
 
+	ret = pcf85363_watchdog_init(dev, pcf85363->regmap);
+	if (ret)
+		dev_err_probe(dev, ret, "Watchdog init failed\n");
+
 	if (irq_a > 0 || wakeup_source)
 		device_init_wakeup(dev, true);