Re: [PATCH v3 5/5] rtc: pcf85363: add watchdog support with configurable step size
From: Guenter Roeck
Date: Wed Nov 19 2025 - 10:37:26 EST
On 11/19/25 00:33, Lakshay Piplani wrote:
Add watchdog timer support to PCF85263/PCF85363 using the linux watchdog
subsystem. The driver programs the hardware watchdog timeout based on
the requested period.
Also use rtc_add_group() instead of sysfs_create_group() to register
timestamp attributes under the RTC class device (/sys/class/rtc/rtcX).
Signed-off-by: Lakshay Piplani <lakshay.piplani@xxxxxxx>
---
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
- Removed failure paths after RTC device registration as per subsystem guidelines.
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 | 168 +++++++++++++++++++++++++++++++++++--
1 file changed, 160 insertions(+), 8 deletions(-)
diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 3d733375187b..34d4c2e16774 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>
@@ -17,6 +21,8 @@
#include <linux/device.h>
#include <linux/of.h>
#include <linux/regmap.h>
+#include <linux/rtc.h>
+#include <linux/watchdog.h>
/*
* Date/Time registers
@@ -127,6 +133,18 @@
#define OFFSET_MAXIMUM 127
#define OFFSET_MASK 0xFF
+#define WD_MODE_REPEAT BIT(7)
+#define WD_TIMEOUT_MASK GENMASK(6, 2)
+#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_TIMEOUT_MIN 1
+#define WD_TIMEOUT_MAX 0x1F
+
struct pcf85363 {
struct rtc_device *rtc;
struct regmap *regmap;
@@ -138,6 +156,15 @@ 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;
+ bool repeat;
+};
+
static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
{
u32 load = 7000;
@@ -323,12 +350,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) {
@@ -360,6 +388,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;
}
@@ -503,6 +536,123 @@ static const struct pcf85x63_config pcf_85363_config = {
.num_nvram = 2
};
+/*
+ * 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->repeat ? WD_MODE_REPEAT : 0) |
+ ((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;
+
+ 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;
+ unsigned int timeout_sec;
+ 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, 10, dev);
Calling watchdog_init_timeout() with a value other than 0 means that
a parameter from devicetree won't be accepted. Calling it with a fixed
value is usually pointless unless the value is out of the valid range,
which by itself would be pointless.
watchdog_init_timeout() is normally called to pass and validate a module
parameter or to pick a timeout from devicetree. Calling it with a constant
value other than 0 is unnecessary.
+ if (ret)
+ wd->wdd.timeout = clamp(10U, WD_TIMEOUT_MIN, WD_TIMEOUT_MAX);
So if 10 seconds is invalid, 10 is clamped to [1, 31] and applied directly.
That is an odd and complicated way of setting the timeout to 10 seconds.
If you don't want a timeout value from devicetree to be accepted, just make this
wd->wdd.timeout = 10;
and do not call watchdog_init_timeout() in the first place.
+
+ timeout_sec = wd->wdd.timeout;
+
+ if (timeout_sec <= 2)
+ wd->clock_sel = WD_CLKSEL_16HZ;
+ else if (timeout_sec <= 8)
+ wd->clock_sel = WD_CLKSEL_4HZ;
+ else if (timeout_sec <= 16)
+ wd->clock_sel = WD_CLKSEL_1HZ;
+ else
+ wd->clock_sel = WD_CLKSEL_0_25HZ;
+
This seems an odd location for this code. What if the timeout changes
later on to one of the other values ?
Also, the timeout is set to a fixed value of 10. That means the above
can be simplified to
wd->clock_sel = WD_CLKSEL_1HZ;
... and that in turn means that the variable is pointless, and that
WD_CLKSEL_1HZ could be used as constant instead.
Why all that complexity ? Am I missing something ? I am quite concerned that
I may be missing trees in the forest, meaning that the real problems are hiding
behind the noise.
Guenter
+ wd->repeat = true;
What is the purpose of this variable ? It is always set to true.
You might as well drop it.
+It is not entirely obvious how those changes are related to adding watchdog support
+ 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);
+
+ dev_dbg(dev, "pcf85363 watchdog registered (timeout=%us, clk_sel=%u)\n",
+ timeout_sec, wd->clock_sel);
+
+ 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
@@ -684,20 +834,22 @@ 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);
dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
- ret = devm_rtc_register_device(pcf85363->rtc);
-
+ ret = rtc_add_group(pcf85363->rtc, &pcf85363_attr_group);
if (ret)
- return dev_err_probe(dev, ret, "RTC registration failed\n");
-
- ret = sysfs_create_group(&pcf85363->rtc->dev.kobj, &pcf85363_attr_group);
+ return ret;
+ ret = devm_rtc_register_device(pcf85363->rtc);
if (ret)
- return dev_err_probe(dev, ret, "Timestamp sysfs creation failed\n");
+ return dev_err_probe(dev, ret, "RTC registration failed\n");
to this driver.
for (i = 0; i < config->num_nvram; i++) {
nvmem_cfg[i].priv = pcf85363;