[PATCH v7 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config

[PATCH v7 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config

[Lakshay Piplani]

NXP PCF85263/PCF85363 provides three timestamp registers (TSR1-TSR3)
which latch the current time when a selected event occurs. Add a
vendor specific property, nxp,timestamp-mode, to select the event
source for each register.

Also introduce a new header 'pcf85363-tsr.h' to expose
macros for timestamp mode fields, improving readability
of device tree file.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Reviewed-by: Rob Herring <robh@kernel.org>

---
V6 -> V7:
- No change.

V5 -> V6:
- No change

V4 -> V5:
- No change

V3 -> V4:
- Added Reviewed-by tag in commit message (previously only in changelog) 

V2 -> V3:
- No changes in v3
- Added Reviewed-by: Rob Herring <robh@kernel.org>

V1 -> V2:
- Addressed review comments from Rob Herring:
  * use $ref: /schemas/types.yaml#/definitions/uint32-array
  * tuple form with exactly 3 items (TSR1/TSR2/TSR3), per items decimal enums
  * define 'nxp,timestamp-mode' clearly
  * drop watchdog related vendor properties
  * remove watchdog related vendor properties from i2c example
---
---
 .../devicetree/bindings/rtc/nxp,pcf85363.yaml | 23 ++++++++++++++-
 include/dt-bindings/rtc/pcf85363-tsr.h        | 28 +++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/rtc/pcf85363-tsr.h

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
index 52aa3e2091e9..cf9c155162d6 100644
--- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/rtc/nxp,pcf85363.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Philips PCF85263/PCF85363 Real Time Clock
+title: NXP PCF85263/PCF85363 Real Time Clock
 
 maintainers:
   - Alexandre Belloni <alexandre.belloni@bootlin.com>
@@ -39,6 +39,24 @@ properties:
   start-year: true
   wakeup-source: true
 
+  nxp,timestamp-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    items:
+      - enum: [0, 1, 2] # TSR1: NONE, FE, LE
+        description: TSR1 mode
+      - enum: [0, 1, 2, 3, 4, 5] # TSR2: NONE, FB, LB, LV, FE, LE
+        description: TSR2 mode
+      - enum: [0, 1, 2, 3] # TSR3: NONE, FB, LB, LV
+        description: TSR3 mode
+    description: |
+      Defines timestamp modes for TSR1, TSR2, and TSR3.
+      Use macros from <dt-bindings/rtc/pcf85363-tsr.h>.
+
+      Each value corresponds to a mode constant:
+        - TSR1: NONE, FE, LE
+        - TSR2: NONE, FB, LB, LV, FE, LE
+        - TSR3: NONE, FB, LB, LV
+
 required:
   - compatible
   - reg
@@ -47,6 +65,7 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/rtc/pcf85363-tsr.h>
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -56,5 +75,7 @@ examples:
             reg = <0x51>;
             #clock-cells = <0>;
             quartz-load-femtofarads = <12500>;
+            wakeup-source;
+            nxp,timestamp-mode = <PCF85363_TSR1_FE PCF85363_TSR2_LB PCF85363_TSR3_LV>;
         };
     };
diff --git a/include/dt-bindings/rtc/pcf85363-tsr.h b/include/dt-bindings/rtc/pcf85363-tsr.h
new file mode 100644
index 000000000000..1fb5b9b3601e
--- /dev/null
+++ b/include/dt-bindings/rtc/pcf85363-tsr.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Copyright 2025 NXP
+ */
+
+#ifndef _DT_BINDINGS_RTC_PCF85363_TSR_H
+#define _DT_BINDINGS_RTC_PCF85363_TSR_H
+
+/* TSR1 modes */
+#define PCF85363_TSR1_NONE 0x00
+#define PCF85363_TSR1_FE 0x01
+#define PCF85363_TSR1_LE 0x02
+
+/* TSR2 modes */
+#define PCF85363_TSR2_NONE 0x00
+#define PCF85363_TSR2_FB 0x01
+#define PCF85363_TSR2_LB 0x02
+#define PCF85363_TSR2_LV 0x03
+#define PCF85363_TSR2_FE 0x04
+#define PCF85363_TSR2_LE 0x05
+
+/* TSR3 modes */
+#define PCF85363_TSR3_NONE 0x00
+#define PCF85363_TSR3_FB 0x01
+#define PCF85363_TSR3_LB 0x02
+#define PCF85363_TSR3_LV 0x03
+
+#endif /* _DT_BINDINGS_RTC_PCF85363_TSR_H */

base-commit: 704340f1cd0dcef829eb62f5b48ae95a2ce17bdf
-- 
2.25.1

[PATCH v7 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL

[Lakshay Piplani]

Report battery switch-over on PCF85263/PCF85363 through the standard
RTC_VL_* ioctls: RTC_VL_READ exposes the BSF flag and RTC_VL_CLR clears
it. The flag is cached before it is cleared so it survives re-arming, and
it is collected from both the IRQ handler and RTC_VL_READ so poll-only
setups still see it.

CTRL_FLAGS is write-0-to-clear, so a read-modify-write can drop an
unrelated flag. Add pcf85363_clear_flags(), which writes the complement
of the mask to clear only the requested bits.

Probe no longer blanket-clears CTRL_FLAGS, so pre-existing events are
preserved. As the battery-backed registers can hold stale flags across a
power cycle and storm the level-triggered INTA line, probe masks off only
the interrupt sources and Alarm2 enables this driver never services and
clears their flags, and the IRQ handler clears any unserviced A2F/PIF
flags.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>

---
V6 -> V7:
- Track IRQ acknowledgement failures in the threaded handler and return
  IRQ_NONE when an asserted source cannot be serviced or cleared, so the
  genirq spurious-interrupt detector can shut down a permanently stuck
  INTA line instead of looping forever. Apply the policy consistently to
  the A1F, battery-switch and unserviced A2F/PIF paths, and only mark the
  interrupt handled when a source was actually cleared.
- Use dev_err_ratelimited() on the flag-clear failure paths so an I/O
  error on the level-triggered line cannot flood the log.

V5 -> V6:
- Mask INT_WDIE alongside the other unused INTA sources at probe so a
  battery-backed watchdog-enable left by the bootloader cannot storm the
  level-triggered INTA line before watchdog support is added later in
  the series

V4 -> V5:
- Add pcf85363_clear_flags() for the write-0-to-clear CTRL_FLAGS register
  and use it everywhere, so a read-modify-write no longer drops flags.
- Cache BSF before clearing it and collect it from both the IRQ handler
  and RTC_VL_READ so poll-only setups still observe it.
- Latch pending events in probe instead of blanket-clearing CTRL_FLAGS,
  and mask/clear only the unused INTA sources to avoid an interrupt storm.
- Propagate register-access errors out of pcf85363_collect_events(); the
  IRQ handler logs them but still acknowledges the interrupt.
- Serialise the cached event state with a mutex.

V3 -> V4:
- No changes in v4.

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 | 205 ++++++++++++++++++++++++++++++++++---
 1 file changed, 191 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index ccc7834e5759..00f37ffcca26 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -14,6 +14,8 @@
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/bcd.h>
+#include <linux/device.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
 
@@ -113,6 +115,9 @@
 struct pcf85363 {
 	struct rtc_device	*rtc;
 	struct regmap		*regmap;
+	/* Serialises access to the cached event state below. */
+	struct mutex		lock;
+	bool			bsf;
 };
 
 struct pcf85x63_config {
@@ -120,6 +125,15 @@ struct pcf85x63_config {
 	unsigned int num_nvram;
 };
 
+/*
+ * CTRL_FLAGS is write-0-to-clear, so write the complement of the mask to
+ * clear only the requested bits without disturbing the others.
+ */
+static int pcf85363_clear_flags(struct pcf85363 *pcf85363, u8 mask)
+{
+	return regmap_write(pcf85363->regmap, CTRL_FLAGS, (u8)~mask);
+}
+
 static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
 {
 	u32 load = 7000;
@@ -253,7 +267,7 @@ static int _pcf85363_rtc_alarm_irq_enable(struct pcf85363 *pcf85363, unsigned
 		return ret;
 
 	/* clear current flags */
-	return regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A1F, 0);
+	return pcf85363_clear_flags(pcf85363, FLAGS_A1F);
 }
 
 static int pcf85363_rtc_alarm_irq_enable(struct device *dev,
@@ -292,9 +306,41 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	return _pcf85363_rtc_alarm_irq_enable(pcf85363, alrm->enabled);
 }
 
+/*
+ * Latch a pending battery-switch event into the software cache. Runs from
+ * the IRQ handler and the RTC_VL_READ path, so it is seen with or without
+ * an interrupt line. BSF is cached before it is cleared so it survives
+ * re-arming. Caller must hold pcf85363->lock. Returns a negative errno on
+ * a register-access failure, otherwise the handled flag mask.
+ */
+static int pcf85363_collect_events(struct pcf85363 *pcf85363)
+{
+	unsigned int flags;
+	int handled = 0;
+	int ret;
+
+	ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
+	if (ret)
+		return ret;
+
+	if (flags & FLAGS_BSF) {
+		pcf85363->bsf = true;
+
+		ret = pcf85363_clear_flags(pcf85363, FLAGS_BSF);
+		if (ret)
+			return ret;
+
+		handled |= FLAGS_BSF;
+	}
+
+	return handled;
+}
+
 static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 {
 	struct pcf85363 *pcf85363 = i2c_get_clientdata(dev_id);
+	bool ack_failed = false;
+	bool handled = false;
 	unsigned int flags;
 	int err;
 
@@ -302,16 +348,108 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 	if (err)
 		return IRQ_NONE;
 
+	if (flags) {
+		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s\n",
+			flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
+			(flags & FLAGS_BSF) ? " [BSF]" : "");
+	}
+
 	if (flags & FLAGS_A1F) {
-		rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF);
-		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A1F, 0);
-		return IRQ_HANDLED;
+		err = pcf85363_clear_flags(pcf85363, FLAGS_A1F);
+		if (err) {
+			dev_err_ratelimited(&pcf85363->rtc->dev,
+					    "failed to clear alarm flag: %d\n",
+					    err);
+			ack_failed = true;
+		} else {
+			rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF);
+			handled = true;
+		}
+	}
+
+	if (flags & FLAGS_BSF) {
+		guard(mutex)(&pcf85363->lock);
+
+		err = pcf85363_collect_events(pcf85363);
+		if (err < 0) {
+			dev_err_ratelimited(&pcf85363->rtc->dev,
+					    "failed to collect events: %d\n",
+					    err);
+			ack_failed = true;
+		} else if (err) {
+			handled = true;
+		}
+	}
+
+	/*
+	 * Clear flags this handler does not service (e.g. A2F/PIF); otherwise
+	 * they hold the level-triggered INTA line asserted and storm the IRQ.
+	 */
+	if (flags & (FLAGS_A2F | FLAGS_PIF)) {
+		err = pcf85363_clear_flags(pcf85363, FLAGS_A2F | FLAGS_PIF);
+		if (err) {
+			dev_err_ratelimited(&pcf85363->rtc->dev,
+					    "failed to clear unhandled flags: %d\n",
+					    err);
+			ack_failed = true;
+		} else {
+			handled = true;
+		}
+	}
+
+	/*
+	 * If any asserted source could not be serviced or cleared, the INTA
+	 * line stays asserted; return IRQ_NONE so the spurious-IRQ detector can
+	 * disable a permanently stuck line instead of looping forever.
+	 */
+	if (ack_failed)
+		return IRQ_NONE;
+
+	return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int pcf85363_rtc_ioctl(struct device *dev,
+			      unsigned int cmd, unsigned long arg)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+	int ret;
+
+	switch (cmd) {
+	case RTC_VL_READ: {
+		u32 status = 0;
+
+		guard(mutex)(&pcf85363->lock);
+
+		/* Refresh so a poll-only setup still latches the BSF flag. */
+		ret = pcf85363_collect_events(pcf85363);
+		if (ret < 0)
+			return ret;
+
+		if (pcf85363->bsf)
+			status |= RTC_VL_BACKUP_SWITCH;
+
+		return put_user(status, (u32 __user *)arg);
 	}
 
-	return IRQ_NONE;
+	case RTC_VL_CLR: {
+		guard(mutex)(&pcf85363->lock);
+
+		ret = pcf85363_clear_flags(pcf85363, FLAGS_BSF);
+		if (ret)
+			return ret;
+
+		pcf85363->bsf = false;
+
+		return 0;
+	}
+
+	default:
+		return -ENOIOCTLCMD;
+	}
 }
 
 static const struct rtc_class_ops rtc_ops = {
+	.ioctl  = pcf85363_rtc_ioctl,
 	.read_time	= pcf85363_rtc_read_time,
 	.set_time	= pcf85363_rtc_set_time,
 	.read_alarm	= pcf85363_rtc_read_alarm,
@@ -412,6 +550,10 @@ static int pcf85363_probe(struct i2c_client *client)
 	if (!pcf85363)
 		return -ENOMEM;
 
+	ret = devm_mutex_init(&client->dev, &pcf85363->lock);
+	if (ret)
+		return ret;
+
 	pcf85363->regmap = devm_regmap_init_i2c(client, &config->regmap);
 	if (IS_ERR(pcf85363->regmap)) {
 		dev_err(&client->dev, "regmap allocation failed\n");
@@ -435,17 +577,52 @@ static int pcf85363_probe(struct i2c_client *client)
 
 	wakeup_source = device_property_read_bool(&client->dev,
 						  "wakeup-source");
-	if (client->irq > 0 || wakeup_source) {
-		err = regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
-		if (err)
-			return dev_err_probe(&client->dev, err,
-					     "failed to clear flags\n");
 
-		err = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+	/*
+	 * Latch and clear any battery-switch event that occurred before
+	 * probe (for example while the main supply was off) instead of
+	 * blanket clearing CTRL_FLAGS, so the cached state is preserved.
+	 */
+	scoped_guard(mutex, &pcf85363->lock) {
+		ret = pcf85363_collect_events(pcf85363);
+		if (ret < 0)
+			return dev_err_probe(&client->dev, ret,
+					     "Failed to latch boot-time events\n");
+	}
+
+	/*
+	 * Battery-backed registers can retain stale state across a power cycle.
+	 * A stale asserted flag would storm the level-triggered INTA line, so
+	 * disable only the interrupt sources and Alarm2 enables this driver
+	 * never services and clear their flags; leave the managed sources
+	 * (A1IE, BSIE) and Alarm1 for their own paths to arm. WDIE is masked
+	 * too so a bootloader-armed watchdog cannot storm INTA before the
+	 * driver is ready to service it.
+	 */
+	ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
+				 INT_WDIE | INT_A2IE | INT_OIE | INT_PIE | INT_ILP,
+				 0);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to mask unused INTA sources\n");
+
+	ret = regmap_update_bits(pcf85363->regmap, DT_ALARM_EN,
+				 ALRM_MIN_A2E | ALRM_HR_A2E | ALRM_DAY_A2E, 0);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to mask Alarm2 enables\n");
+
+	ret = pcf85363_clear_flags(pcf85363, FLAGS_A2F | FLAGS_PIF);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to clear stale flags\n");
+
+	if (client->irq > 0 || wakeup_source) {
+		ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
 					 PIN_IO_INTAPM, PIN_IO_INTA_OUT);
-		if (err)
-			return dev_err_probe(&client->dev, err,
-					     "failed to set interrupt pin mode\n");
+		if (ret)
+			return dev_err_probe(&client->dev, ret,
+					     "Failed to configure INTA pin\n");
 	}
 
 	if (client->irq > 0) {
-- 
2.25.1

Re: [PATCH v7 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL

[sashiko-bot]

> Report battery switch-over on PCF85263/PCF85363 through the standard
> RTC_VL_* ioctls: RTC_VL_READ exposes the BSF flag and RTC_VL_CLR clears
> it. The flag is cached before it is cleared so it survives re-arming, and
> it is collected from both the IRQ handler and RTC_VL_READ so poll-only
> setups still see it.
> 
> CTRL_FLAGS is write-0-to-clear, so a read-modify-write can drop an
> unrelated flag. Add pcf85363_clear_flags(), which writes the complement
> of the mask to clear only the requested bits.
> 
> Probe no longer blanket-clears CTRL_FLAGS, so pre-existing events are
> preserved. As the battery-backed registers can hold stale flags across a
> power cycle and storm the level-triggered INTA line, probe masks off only
> the interrupt sources and Alarm2 enables this driver never services and
> clears their flags, and the IRQ handler clears any unserviced A2F/PIF
> flags.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v7 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode

[Lakshay Piplani]

Add support for the timestamp capture registers on PCF85263/PCF85363,
which latch the time on selected events (TS pin or battery switch-over).
The capture source is selected via the nxp,timestamp-mode device tree
property and latched values are exported through read-only sysfs
attributes, refreshed on read so poll-only boards still latch events.

The two capture families need different handling: first-event modes hold
the value until the flag is cleared, so it is cached before clearing;
last-event modes overwrite on every event, so the flag is cleared first
and the block re-read until two reads agree.

While at it, register the attributes with rtc_add_group(), preserve
CTRL_INTA_EN with regmap_update_bits() instead of overwriting it, only
clear RTC_FEATURE_ALARM when no interrupt is available, and make the
nvmem_config array probe-local since it is patched per-device.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>

---
V6 -> V7:
- Separate data errors from acknowledgement failures in the shared
  IRQ policy from patch 2: a last-event flag clear is the
  acknowledgement, so the source is marked handled once the clear
  succeeds and a later read failure is logged and skipped instead of
  reported as an ack failure.
- Leave a first-event flag asserted on a read failure so a later IRQ
  or sysfs read can retry instead of dropping the event.

V5 -> V6:
- Return -EPROBE_DEFER from probe when the IRQ request is deferred
  instead of silently disabling alarms; on any other failure warn and
  keep RTC timekeeping alive with interrupt-driven alarms disabled
- Configure the INTA pin and keep RTC_FEATURE_ALARM for
  wakeup-source-only boards (INTA routed to a PMIC) so rtcwake still
  works even without our own IRQ line

V4 -> V5:
- Handle first-event (cache before clearing) and last-event (clear, then
  re-read until stable, -EAGAIN on a torn read) capture modes separately,
  and propagate register-access errors, surfaced on the sysfs read path.
- Refresh the cache on sysfs read so poll-only boards still latch events.
- Treat a failed DT_TS_MODE write as fatal, and drive the TS pin only for
  the TS-pin modes: set TSPM and leave TSIM at its reset default.
- Preserve CTRL_INTA_EN via regmap_update_bits(); enable INT_TSRIE only
  when a timestamp mode is configured, and check the INTA writes.
- Clear RTC_FEATURE_ALARM only when no interrupt is available.
- Make the nvmem_config array probe-local, mask the reserved seconds bit,
  and don't hardcode IRQF_TRIGGER_LOW for a firmware-node interrupt.

V3 -> V4:
- No changes in v4.

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 | 409 ++++++++++++++++++++++++++++++++-----
 1 file changed, 356 insertions(+), 53 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 00f37ffcca26..ef846dd231a0 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-2026 NXP
+ * Added support for timestamps, battery switch-over,
+ * watchdog, offset calibration.
  */
 #include <linux/module.h>
 #include <linux/i2c.h>
@@ -18,6 +22,9 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
+#include <linux/string.h>
+
+#include <dt-bindings/rtc/pcf85363-tsr.h>
 
 /*
  * Date/Time registers
@@ -102,22 +109,50 @@
 #define PIN_IO_INTA_OUT	2
 #define PIN_IO_INTA_HIZ	3
 
+#define PIN_IO_TSPM     GENMASK(3, 2)
+#define PIN_IO_TSIM     BIT(4)
+
 #define OSC_CAP_SEL	GENMASK(1, 0)
 #define OSC_CAP_6000	0x01
 #define OSC_CAP_12500	0x02
 
 #define STOP_EN_STOP	BIT(0)
 
+#define RTCM_BIT        BIT(4)
+
 #define RESET_CPR	0xa4
 
 #define NVRAM_SIZE	0x40
 
+#define TSR1_MASK       0x03
+#define TSR2_MASK       0x07
+#define TSR3_MASK       0x03
+#define TSR1_SHIFT      0
+#define TSR2_SHIFT      2
+#define TSR3_SHIFT      6
+
+#define PCF85363_NUM_TS		3
+/* Bytes latched per timestamp register (sec, min, hour, day, mon, year). */
+#define PCF85363_TS_LEN		6
+/* Bit 7 of the seconds byte is reserved, not time data; mask it off. */
+#define PCF85363_SEC_MASK	0x7F
+#define PCF85363_TS_READ_RETRIES	3
+
+/* Cached timestamp; the flag is cleared once the value is copied here. */
+struct pcf85363_ts {
+	bool valid;
+	u8 regs[PCF85363_TS_LEN];
+};
+
 struct pcf85363 {
 	struct rtc_device	*rtc;
 	struct regmap		*regmap;
 	/* Serialises access to the cached event state below. */
 	struct mutex		lock;
 	bool			bsf;
+	struct pcf85363_ts	ts[PCF85363_NUM_TS];
+	/* Per-TSR: true when the register uses a last-event capture mode. */
+	bool			ts_last_event[PCF85363_NUM_TS];
 };
 
 struct pcf85x63_config {
@@ -306,23 +341,131 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	return _pcf85363_rtc_alarm_irq_enable(pcf85363, alrm->enabled);
 }
 
+static const u8 pcf85363_ts_base[PCF85363_NUM_TS] = {
+	DT_TIMESTAMP1, DT_TIMESTAMP2, DT_TIMESTAMP3,
+};
+
+static const u8 pcf85363_ts_flag[PCF85363_NUM_TS] = {
+	FLAGS_TSR1F, FLAGS_TSR2F, FLAGS_TSR3F,
+};
+
+/* Mark which TSRs use a last-event mode; those need torn-read handling. */
+static void pcf85363_classify_ts_modes(struct pcf85363 *pcf85363, u8 tsmode)
+{
+	u8 m1 = (tsmode >> TSR1_SHIFT) & TSR1_MASK;
+	u8 m2 = (tsmode >> TSR2_SHIFT) & TSR2_MASK;
+	u8 m3 = (tsmode >> TSR3_SHIFT) & TSR3_MASK;
+
+	pcf85363->ts_last_event[0] = (m1 == PCF85363_TSR1_LE);
+	pcf85363->ts_last_event[1] = (m2 == PCF85363_TSR2_LB ||
+				      m2 == PCF85363_TSR2_LV ||
+				      m2 == PCF85363_TSR2_LE);
+	pcf85363->ts_last_event[2] = (m3 == PCF85363_TSR3_LB ||
+				      m3 == PCF85363_TSR3_LV);
+}
+
+/*
+ * Last-event registers can change under us; re-read the block until two
+ * consecutive reads agree, then publish the stable value. Reject a torn
+ * read with -EAGAIN once the retry budget is exhausted.
+ */
+static int pcf85363_read_ts_stable(struct pcf85363 *pcf85363, int i)
+{
+	u8 prev[PCF85363_TS_LEN], cur[PCF85363_TS_LEN];
+	int retries, ret;
+
+	ret = regmap_bulk_read(pcf85363->regmap, pcf85363_ts_base[i],
+			       prev, PCF85363_TS_LEN);
+	if (ret)
+		return ret;
+
+	for (retries = 0; retries < PCF85363_TS_READ_RETRIES; retries++) {
+		ret = regmap_bulk_read(pcf85363->regmap, pcf85363_ts_base[i],
+				       cur, PCF85363_TS_LEN);
+		if (ret)
+			return ret;
+
+		if (!memcmp(prev, cur, PCF85363_TS_LEN)) {
+			memcpy(pcf85363->ts[i].regs, cur, PCF85363_TS_LEN);
+			return 0;
+		}
+
+		memcpy(prev, cur, PCF85363_TS_LEN);
+	}
+
+	return -EAGAIN;
+}
+
 /*
- * Latch a pending battery-switch event into the software cache. Runs from
- * the IRQ handler and the RTC_VL_READ path, so it is seen with or without
- * an interrupt line. BSF is cached before it is cleared so it survives
- * re-arming. Caller must hold pcf85363->lock. Returns a negative errno on
- * a register-access failure, otherwise the handled flag mask.
+ * Latch pending events into the cache. First-event modes hold the value
+ * until the flag is cleared, so cache before clearing; last-event modes
+ * overwrite every event, so clear first then read until stable. Runs from
+ * the IRQ and sysfs paths; caller holds the lock. Returns a negative errno
+ * on a register-access failure (including -EAGAIN for a torn last-event
+ * read), otherwise the handled flag mask.
  */
 static int pcf85363_collect_events(struct pcf85363 *pcf85363)
 {
 	unsigned int flags;
 	int handled = 0;
-	int ret;
+	int i, ret;
 
 	ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
 	if (ret)
 		return ret;
 
+	for (i = 0; i < PCF85363_NUM_TS; i++) {
+		if (!(flags & pcf85363_ts_flag[i]))
+			continue;
+
+		if (pcf85363->ts_last_event[i]) {
+			/*
+			 * Clearing the flag acknowledges the interrupt, so mark
+			 * the source handled as soon as the clear succeeds. A
+			 * later data-read failure is a data error, not an
+			 * acknowledgment failure: log it and move on to the
+			 * remaining flags instead of returning an error.
+			 */
+			ret = pcf85363_clear_flags(pcf85363,
+						   pcf85363_ts_flag[i]);
+			if (ret)
+				return ret;
+
+			handled |= pcf85363_ts_flag[i];
+
+			ret = pcf85363_read_ts_stable(pcf85363, i);
+			if (ret) {
+				dev_warn_ratelimited(&pcf85363->rtc->dev,
+						     "timestamp %d read failed: %d\n",
+						     i + 1, ret);
+				continue;
+			}
+
+			pcf85363->ts[i].valid = true;
+		} else {
+			/*
+			 * First-event data stays latched while its flag is
+			 * set. Leave the flag asserted on a read failure so a
+			 * later IRQ or sysfs read can retry.
+			 */
+			ret = regmap_bulk_read(pcf85363->regmap,
+					       pcf85363_ts_base[i],
+					       pcf85363->ts[i].regs,
+					       PCF85363_TS_LEN);
+			if (ret)
+				return ret;
+
+			pcf85363->ts[i].valid = true;
+
+			ret = pcf85363_clear_flags(pcf85363,
+						   pcf85363_ts_flag[i]);
+			if (ret)
+				return ret;
+
+			handled |= pcf85363_ts_flag[i];
+		}
+	}
+
 	if (flags & FLAGS_BSF) {
 		pcf85363->bsf = true;
 
@@ -349,8 +492,11 @@ 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\n",
+		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%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]" : "");
 	}
 
@@ -367,7 +513,7 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 		}
 	}
 
-	if (flags & FLAGS_BSF) {
+	if (flags & (FLAGS_TSR1F | FLAGS_TSR2F | FLAGS_TSR3F | FLAGS_BSF)) {
 		guard(mutex)(&pcf85363->lock);
 
 		err = pcf85363_collect_events(pcf85363);
@@ -517,12 +663,88 @@ static const struct pcf85x63_config pcf_85363_config = {
 	.num_nvram = 2
 };
 
+/* Six BCD bytes; bit 7 of the seconds byte is reserved, not time data. */
+static ssize_t pcf85363_format_timestamp(const u8 *regs, char *buf)
+{
+	struct rtc_time tm;
+
+	tm.tm_sec = bcd2bin(regs[0] & PCF85363_SEC_MASK);
+	tm.tm_min = bcd2bin(regs[1]);
+	tm.tm_hour = bcd2bin(regs[2]);
+	tm.tm_mday = bcd2bin(regs[3]);
+	tm.tm_mon = bcd2bin(regs[4]) - 1;
+	tm.tm_year = bcd2bin(regs[5]) + 100;
+
+	return sysfs_emit(buf, "%04d-%02d-%02d %02d:%02d:%02d\n",
+			  tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+			  tm.tm_hour, tm.tm_min, tm.tm_sec);
+}
+
+/* Refresh from hardware first so poll-only setups still latch events. */
+static ssize_t pcf85363_timestamp_show(struct device *dev, char *buf,
+				       unsigned int index)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+	int ret;
+
+	guard(mutex)(&pcf85363->lock);
+
+	ret = pcf85363_collect_events(pcf85363);
+	if (ret < 0)
+		return ret;
+
+	if (!pcf85363->ts[index].valid)
+		return sysfs_emit(buf, "00-00-00 00:00:00\n");
+
+	return pcf85363_format_timestamp(pcf85363->ts[index].regs, buf);
+}
+
+static ssize_t timestamp1_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return pcf85363_timestamp_show(dev, buf, 0);
+}
+static DEVICE_ATTR_RO(timestamp1);
+
+static ssize_t timestamp2_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return pcf85363_timestamp_show(dev, buf, 1);
+}
+static DEVICE_ATTR_RO(timestamp2);
+
+static ssize_t timestamp3_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return pcf85363_timestamp_show(dev, buf, 2);
+}
+static DEVICE_ATTR_RO(timestamp3);
+
+static struct attribute *pcf85363_attrs[] = {
+	&dev_attr_timestamp1.attr,
+	&dev_attr_timestamp2.attr,
+	&dev_attr_timestamp3.attr,
+	NULL,
+};
+
+static const struct attribute_group pcf85363_attr_group = {
+	.attrs = pcf85363_attrs,
+};
+
 static int pcf85363_probe(struct i2c_client *client)
 {
-	struct pcf85363 *pcf85363;
 	const struct pcf85x63_config *config = &pcf_85363_config;
 	const void *data = of_device_get_match_data(&client->dev);
-	static struct nvmem_config nvmem_cfg[] = {
+	struct device *dev = &client->dev;
+	struct pcf85363 *pcf85363;
+	int irq_a = client->irq;
+	bool ts_mode_configured = false;
+	bool wakeup_source;
+	int ret, i, err;
+	u32 tsr_mode[3];
+	u8 val;
+
+	struct nvmem_config nvmem_cfg[] = {
 		{
 			.name = "pcf85x63-",
 			.word_size = 1,
@@ -539,29 +761,72 @@ static int pcf85363_probe(struct i2c_client *client)
 			.reg_write = pcf85363_nvram_write,
 		},
 	};
-	int ret, i, err;
-	bool wakeup_source;
 
 	if (data)
 		config = data;
 
-	pcf85363 = devm_kzalloc(&client->dev, sizeof(struct pcf85363),
-				GFP_KERNEL);
+	pcf85363 = devm_kzalloc(&client->dev, sizeof(*pcf85363), GFP_KERNEL);
 	if (!pcf85363)
 		return -ENOMEM;
 
-	ret = devm_mutex_init(&client->dev, &pcf85363->lock);
+	ret = devm_mutex_init(dev, &pcf85363->lock);
 	if (ret)
 		return ret;
 
 	pcf85363->regmap = devm_regmap_init_i2c(client, &config->regmap);
-	if (IS_ERR(pcf85363->regmap)) {
-		dev_err(&client->dev, "regmap allocation failed\n");
-		return PTR_ERR(pcf85363->regmap);
-	}
+
+	if (IS_ERR(pcf85363->regmap))
+		return dev_err_probe(dev, PTR_ERR(pcf85363->regmap), "regmap init failed\n");
 
 	i2c_set_clientdata(client, pcf85363);
 
+	ret = regmap_update_bits(pcf85363->regmap, CTRL_FUNCTION, RTCM_BIT, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable RTC mode\n");
+
+	if (!device_property_read_u32_array(dev, "nxp,timestamp-mode", tsr_mode, 3)) {
+		bool ts_pin_used;
+
+		tsr_mode[0] &= TSR1_MASK;
+		tsr_mode[1] &= TSR2_MASK;
+		tsr_mode[2] &= TSR3_MASK;
+
+		val = (tsr_mode[2] << TSR3_SHIFT) |
+		      (tsr_mode[1] << TSR2_SHIFT) |
+		      (tsr_mode[0] << TSR1_SHIFT);
+
+		ret = regmap_write(pcf85363->regmap, DT_TS_MODE, val);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to write timestamp mode register\n");
+
+		pcf85363_classify_ts_modes(pcf85363, val);
+
+		ts_mode_configured = tsr_mode[0] || tsr_mode[1] || tsr_mode[2];
+
+		/*
+		 * Only the TS-pin capture modes drive the TS pin. Select the
+		 * timestamp function (TSPM) for those and leave the input mode
+		 * (TSIM) at its reset default rather than forcing the
+		 * mechanical-switch detector.
+		 */
+		ts_pin_used = tsr_mode[0] == PCF85363_TSR1_FE ||
+			      tsr_mode[0] == PCF85363_TSR1_LE ||
+			      tsr_mode[1] == PCF85363_TSR2_FE ||
+			      tsr_mode[1] == PCF85363_TSR2_LE;
+
+		if (ts_pin_used) {
+			ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+						 PIN_IO_TSPM, PIN_IO_TSPM);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "Failed to configure TS pin\n");
+		}
+
+		dev_dbg(dev, "Timestamp mode set: TSR1=0x%x TSR2=0x%x TSR3=0x%x\n",
+			tsr_mode[0], tsr_mode[1], tsr_mode[2]);
+	}
+
 	pcf85363->rtc = devm_rtc_allocate_device(&client->dev);
 	if (IS_ERR(pcf85363->rtc))
 		return PTR_ERR(pcf85363->rtc);
@@ -575,80 +840,118 @@ static int pcf85363_probe(struct i2c_client *client)
 	pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
 	pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
 
-	wakeup_source = device_property_read_bool(&client->dev,
-						  "wakeup-source");
+	wakeup_source = device_property_read_bool(dev, "wakeup-source");
 
 	/*
-	 * Latch and clear any battery-switch event that occurred before
-	 * probe (for example while the main supply was off) instead of
-	 * blanket clearing CTRL_FLAGS, so the cached state is preserved.
+	 * Latch pre-probe events instead of blanket-clearing CTRL_FLAGS, so
+	 * pre-existing timestamps and the battery-switch flag are not lost.
 	 */
 	scoped_guard(mutex, &pcf85363->lock) {
 		ret = pcf85363_collect_events(pcf85363);
 		if (ret < 0)
-			return dev_err_probe(&client->dev, ret,
+			return dev_err_probe(dev, ret,
 					     "Failed to latch boot-time events\n");
 	}
 
 	/*
 	 * Battery-backed registers can retain stale state across a power cycle.
 	 * A stale asserted flag would storm the level-triggered INTA line, so
-	 * disable only the interrupt sources and Alarm2 enables this driver
-	 * never services and clear their flags; leave the managed sources
-	 * (A1IE, BSIE) and Alarm1 for their own paths to arm. WDIE is masked
-	 * too so a bootloader-armed watchdog cannot storm INTA before the
-	 * driver is ready to service it.
+	 * disable the interrupt sources and Alarm2 enables this driver does not
+	 * arm here and clear their flags; leave the managed sources (A1IE, BSIE,
+	 * TSRIE) and Alarm1 for their own paths to arm. WDIE is masked too so a
+	 * bootloader-armed watchdog cannot storm INTA before the driver is ready
+	 * to service it.
 	 */
 	ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
 				 INT_WDIE | INT_A2IE | INT_OIE | INT_PIE | INT_ILP,
 				 0);
 	if (ret)
-		return dev_err_probe(&client->dev, ret,
-				     "Failed to mask unused INTA sources\n");
+		return dev_err_probe(dev, ret, "Failed to mask unused INTA sources\n");
 
 	ret = regmap_update_bits(pcf85363->regmap, DT_ALARM_EN,
 				 ALRM_MIN_A2E | ALRM_HR_A2E | ALRM_DAY_A2E, 0);
 	if (ret)
-		return dev_err_probe(&client->dev, ret,
-				     "Failed to mask Alarm2 enables\n");
+		return dev_err_probe(dev, ret, "Failed to mask Alarm2 enables\n");
 
 	ret = pcf85363_clear_flags(pcf85363, FLAGS_A2F | FLAGS_PIF);
 	if (ret)
-		return dev_err_probe(&client->dev, ret,
-				     "Failed to clear stale flags\n");
+		return dev_err_probe(dev, ret, "Failed to clear stale flags\n");
 
-	if (client->irq > 0 || wakeup_source) {
-		ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
-					 PIN_IO_INTAPM, PIN_IO_INTA_OUT);
-		if (ret)
-			return dev_err_probe(&client->dev, ret,
-					     "Failed to configure INTA pin\n");
-	}
-
-	if (client->irq > 0) {
+	if (irq_a > 0) {
 		unsigned long irqflags = IRQF_TRIGGER_LOW;
 
 		if (dev_fwnode(&client->dev))
 			irqflags = 0;
-		ret = devm_request_threaded_irq(&client->dev, client->irq,
-						NULL, pcf85363_rtc_handle_irq,
+
+		ret = devm_request_threaded_irq(dev, irq_a, NULL,
+						pcf85363_rtc_handle_irq,
 						irqflags | IRQF_ONESHOT,
-						"pcf85363", client);
+						"pcf85363-inta", client);
+		/*
+		 * Let the driver core retry when the interrupt provider is not
+		 * ready yet; only a genuine failure disables interrupt-driven
+		 * alarms while leaving RTC timekeeping (and any wakeup-source
+		 * path) intact.
+		 */
+		if (ret == -EPROBE_DEFER)
+			return ret;
 		if (ret) {
-			dev_warn(&client->dev,
-				 "unable to request IRQ, alarms disabled\n");
-			client->irq = 0;
+			dev_warn(dev, "unable to request IRQ, alarms disabled: %d\n",
+				 ret);
+			irq_a = 0;
 		}
 	}
 
-	if (client->irq > 0 || wakeup_source) {
-		device_init_wakeup(&client->dev, true);
+	if (irq_a > 0 || wakeup_source) {
+		/*
+		 * The alarm can be delivered either through our own IRQ line or
+		 * via an external wakeup path (INTA routed to a PMIC), so route
+		 * INTA to its interrupt output and keep the alarm feature in
+		 * both cases; rtcwake relies on it for wakeup-source-only boards.
+		 */
+		ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+					 PIN_IO_INTAPM, PIN_IO_INTA_OUT);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to configure INTA pin\n");
+
+		if (irq_a > 0) {
+			u8 inta_en = INT_BSIE;
+
+			/*
+			 * Enable the timestamp interrupt only when a capture
+			 * mode is configured; always enable battery-switch.
+			 * Only relevant when we service the IRQ ourselves.
+			 */
+			if (ts_mode_configured)
+				inta_en |= INT_TSRIE;
+
+			ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
+						 INT_BSIE | INT_TSRIE, inta_en);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "Failed to enable INTA sources\n");
+		}
+
+		device_init_wakeup(dev, true);
 		set_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
 	} else {
+		/*
+		 * Neither an interrupt line nor a wakeup source: the alarm
+		 * cannot be delivered, so drop the alarm feature.
+		 */
 		clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
 	}
 
+	dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
+
+	ret = rtc_add_group(pcf85363->rtc, &pcf85363_attr_group);
+	if (ret)
+		return ret;
+
 	ret = devm_rtc_register_device(pcf85363->rtc);
+	if (ret)
+		return dev_err_probe(dev, ret, "RTC registration failed\n");
 
 	for (i = 0; i < config->num_nvram; i++) {
 		nvmem_cfg[i].priv = pcf85363;
-- 
2.25.1

Re: [PATCH v7 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rtc: pcf85363: uninitialized ts_last_event state when DT property is missing
- [High] rtc: pcf85363: incorrect TSR3_SHIFT misconfigures hardware
- [High] rtc: pcf85363: GENMASK used as value misconfigures TS pin mode
--

commit f0ef4cbc1465704e262d43ce6c2e731b6a23a590
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
Subject: rtc: pcf85363: add timestamp support with configurable timestamp mode

Add support for the timestamp capture registers on PCF85263/PCF85363,
which latch the time on selected events (TS pin or battery switch-over).
The capture source is selected via the nxp,timestamp-mode device tree
property and latched values are exported through read-only sysfs
attributes.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 00f37ffcca260..ef846dd231a04 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -102,22 +109,50 @@
>  #define PIN_IO_INTA_OUT	2
>  #define PIN_IO_INTA_HIZ	3
>  
> +#define PIN_IO_TSPM     GENMASK(3, 2)
> +#define PIN_IO_TSIM     BIT(4)
>  
>  #define OSC_CAP_SEL	GENMASK(1, 0)
>  #define OSC_CAP_6000	0x01
>  #define OSC_CAP_12500	0x02
>  
>  #define STOP_EN_STOP	BIT(0)
>  
> +#define RTCM_BIT        BIT(4)
>  
>  #define RESET_CPR	0xa4
>  
>  #define NVRAM_SIZE	0x40
>  
> +#define TSR1_MASK       0x03
> +#define TSR2_MASK       0x07
> +#define TSR3_MASK       0x03
> +#define TSR1_SHIFT      0
> +#define TSR2_SHIFT      2
> +#define TSR3_SHIFT      6

[Severity: High]
Is TSR3_SHIFT correct here? The DT_TS_MODE register uses bits 7:5 for TSR3,
so the shift should be 5. Shifting by 6 maps the value to bits 7:6, corrupting
the TSR3 configuration when written to the hardware.

[ ... ]
> @@ -539,29 +761,72 @@ static int pcf85363_probe(struct i2c_client *client)
>  			.reg_write = pcf85363_nvram_write,
>  		},
>  	};
>  
>  	if (data)
>  		config = data;
>  
> -	pcf85363 = devm_kzalloc(&client->dev, sizeof(struct pcf85363),
> -				GFP_KERNEL);
> +	pcf85363 = devm_kzalloc(&client->dev, sizeof(*pcf85363), GFP_KERNEL);
>  	if (!pcf85363)
>  		return -ENOMEM;
>  
[ ... ]
> +	if (!device_property_read_u32_array(dev, "nxp,timestamp-mode", tsr_mode, 3)) {

[Severity: High]
Can this result in an uninitialized ts_last_event state?

If the "nxp,timestamp-mode" property is missing from the device tree, this
block is skipped and pcf85363_classify_ts_modes() is never called. This leaves
ts_last_event as false (from kzalloc).

If the hardware was pre-configured to a last-event mode by a bootloader, the
driver will treat it as a first-event mode, bypassing the stable-read retry
loop and exposing userspace to torn reads.

> +		bool ts_pin_used;
> +
> +		tsr_mode[0] &= TSR1_MASK;
> +		tsr_mode[1] &= TSR2_MASK;
> +		tsr_mode[2] &= TSR3_MASK;
> +
> +		val = (tsr_mode[2] << TSR3_SHIFT) |
> +		      (tsr_mode[1] << TSR2_SHIFT) |
> +		      (tsr_mode[0] << TSR1_SHIFT);
> +
> +		ret = regmap_write(pcf85363->regmap, DT_TS_MODE, val);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to write timestamp mode register\n");
> +
> +		pcf85363_classify_ts_modes(pcf85363, val);
> +
> +		ts_mode_configured = tsr_mode[0] || tsr_mode[1] || tsr_mode[2];
> +
> +		/*
> +		 * Only the TS-pin capture modes drive the TS pin. Select the
> +		 * timestamp function (TSPM) for those and leave the input mode
> +		 * (TSIM) at its reset default rather than forcing the
> +		 * mechanical-switch detector.
> +		 */
> +		ts_pin_used = tsr_mode[0] == PCF85363_TSR1_FE ||
> +			      tsr_mode[0] == PCF85363_TSR1_LE ||
> +			      tsr_mode[1] == PCF85363_TSR2_FE ||
> +			      tsr_mode[1] == PCF85363_TSR2_LE;
> +
> +		if (ts_pin_used) {
> +			ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> +						 PIN_IO_TSPM, PIN_IO_TSPM);

[Severity: High]
Does this misconfigure the TS pin mode?

PIN_IO_TSPM evaluates to 0x0C (binary 11 in bits 3:2). Using it as both the
mask and the value to regmap_update_bits() sets bits 3:2 to 11, which
configures the pin as an output according to the datasheet.

To capture timestamps from external events, shouldn't the TS pin be configured
as an input (mode 10)?

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

RE: [EXT] Re: [PATCH v7 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode

[Lakshay Piplani]

Hi Alexandre,

I have added responses inline, with references to the PCF85263A data sheet


NXP Confidential
> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Tuesday, September 15, 2026 9:25 AM
> To: Lakshay Piplani <lakshay.piplani@nxp.com>
> Cc: robh@kernel.org; Alexandre Belloni <alexandre.belloni@bootlin.com>;
> linux-watchdog@vger.kernel.org; conor+dt@kernel.org; linux-
> rtc@vger.kernel.org; devicetree@vger.kernel.org
> Subject: [EXT] Re: [PATCH v7 3/5] rtc: pcf85363: add timestamp support with
> configurable timestamp mode
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to
> consider:
> - [High] rtc: pcf85363: uninitialized ts_last_event state when DT property is
> missing
> - [High] rtc: pcf85363: incorrect TSR3_SHIFT misconfigures hardware
> - [High] rtc: pcf85363: GENMASK used as value misconfigures TS pin mode
> --
>
> commit f0ef4cbc1465704e262d43ce6c2e731b6a23a590
> Author: Lakshay Piplani <lakshay.piplani@nxp.com>
> Subject: rtc: pcf85363: add timestamp support with configurable timestamp
> mode
>
> Add support for the timestamp capture registers on PCF85263/PCF85363,
> which latch the time on selected events (TS pin or battery switch-over).
> The capture source is selected via the nxp,timestamp-mode device tree
> property and latched values are exported through read-only sysfs attributes.
>
> > diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> > index 00f37ffcca260..ef846dd231a04 100644
> > --- a/drivers/rtc/rtc-pcf85363.c
> > +++ b/drivers/rtc/rtc-pcf85363.c
> [ ... ]
> > @@ -102,22 +109,50 @@
> >  #define PIN_IO_INTA_OUT      2
> >  #define PIN_IO_INTA_HIZ      3
> >
> > +#define PIN_IO_TSPM     GENMASK(3, 2)
> > +#define PIN_IO_TSIM     BIT(4)
> >
> >  #define OSC_CAP_SEL  GENMASK(1, 0)
> >  #define OSC_CAP_6000 0x01
> >  #define OSC_CAP_12500        0x02
> >
> >  #define STOP_EN_STOP BIT(0)
> >
> > +#define RTCM_BIT        BIT(4)
> >
> >  #define RESET_CPR    0xa4
> >
> >  #define NVRAM_SIZE   0x40
> >
> > +#define TSR1_MASK       0x03
> > +#define TSR2_MASK       0x07
> > +#define TSR3_MASK       0x03
> > +#define TSR1_SHIFT      0
> > +#define TSR2_SHIFT      2
> > +#define TSR3_SHIFT      6
>
> [Severity: High]
> Is TSR3_SHIFT correct here? The DT_TS_MODE register uses bits 7:5 for TSR3,
> so the shift should be 5. Shifting by 6 maps the value to bits 7:6, corrupting the
> TSR3 configuration when written to the hardware.
>
> [ ... ]

This is not the case. Per Table 19 (TSR_mode, register 23h), the field layout is:

bits 1:0        TSR1M[1:0]
bits 4:2        TSR2M[2:0]
bit 5   not implemented
bits 7:6        TSR3M[1:0]

TSR3M is a 2-bit field at bits 7:6, so TSR3_SHIFT = 6 is correct and bit 5 is unused. A shift of 5 would place TSR3M at bits 6:5, which would be incorrect.

> > @@ -539,29 +761,72 @@ static int pcf85363_probe(struct i2c_client *client)
> >                       .reg_write = pcf85363_nvram_write,
> >               },
> >       };
> >
> >       if (data)
> >               config = data;
> >
> > -     pcf85363 = devm_kzalloc(&client->dev, sizeof(struct pcf85363),
> > -                             GFP_KERNEL);
> > +     pcf85363 = devm_kzalloc(&client->dev, sizeof(*pcf85363),
> > + GFP_KERNEL);
> >       if (!pcf85363)
> >               return -ENOMEM;
> >
> [ ... ]
> > +     if (!device_property_read_u32_array(dev, "nxp,timestamp-mode",
> > + tsr_mode, 3)) {
>
> [Severity: High]
> Can this result in an uninitialized ts_last_event state?
>
> If the "nxp,timestamp-mode" property is missing from the device tree, this
> block is skipped and pcf85363_classify_ts_modes() is never called. This leaves
> ts_last_event as false (from kzalloc).
>
> If the hardware was pre-configured to a last-event mode by a bootloader, the
> driver will treat it as a first-event mode, bypassing the stable-read retry loop
> and exposing userspace to torn reads.
>

Agreed, this is real. If "nxp,timestamp-mode" is absent but the hardware was left in a last-event mode (e.g. by a bootloader), ts_last_event[] stays false and the stable-read retry loop is skipped, so a torn read can be exposed. Reading DT_TS_MODE back unconditionally and classifying from the effective value fixes it.

Would you prefer this folded into the next revision, or as a separate follow-up fix?

> > +             bool ts_pin_used;
> > +
> > +             tsr_mode[0] &= TSR1_MASK;
> > +             tsr_mode[1] &= TSR2_MASK;
> > +             tsr_mode[2] &= TSR3_MASK;
> > +
> > +             val = (tsr_mode[2] << TSR3_SHIFT) |
> > +                   (tsr_mode[1] << TSR2_SHIFT) |
> > +                   (tsr_mode[0] << TSR1_SHIFT);
> > +
> > +             ret = regmap_write(pcf85363->regmap, DT_TS_MODE, val);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "Failed to write timestamp
> > + mode register\n");
> > +
> > +             pcf85363_classify_ts_modes(pcf85363, val);
> > +
> > +             ts_mode_configured = tsr_mode[0] || tsr_mode[1] ||
> > + tsr_mode[2];
> > +
> > +             /*
> > +              * Only the TS-pin capture modes drive the TS pin. Select the
> > +              * timestamp function (TSPM) for those and leave the input mode
> > +              * (TSIM) at its reset default rather than forcing the
> > +              * mechanical-switch detector.
> > +              */
> > +             ts_pin_used = tsr_mode[0] == PCF85363_TSR1_FE ||
> > +                           tsr_mode[0] == PCF85363_TSR1_LE ||
> > +                           tsr_mode[1] == PCF85363_TSR2_FE ||
> > +                           tsr_mode[1] == PCF85363_TSR2_LE;
> > +
> > +             if (ts_pin_used) {
> > +                     ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> > +                                              PIN_IO_TSPM,
> > + PIN_IO_TSPM);
>
> [Severity: High]
> Does this misconfigure the TS pin mode?
>
> PIN_IO_TSPM evaluates to 0x0C (binary 11 in bits 3:2). Using it as both the
> mask and the value to regmap_update_bits() sets bits 3:2 to 11, which
> configures the pin as an output according to the datasheet.
>
> To capture timestamps from external events, shouldn't the TS pin be configured
> as an input (mode 10)?
>

The encoding here is inverted. Per Table 46 (TSPM[1:0], Pin_IO register 27h, bits 3:2):

00      disabled (input, may float)
01      INTB output, push-pull
10      CLK output, push-pull
11      input mode

PIN_IO_TSPM = GENMASK(3,2) = 0x0C selects 11b = input mode, which is what is needed to capture external TS-pin events. 10b is CLK output, so that suggestion would break capture.
Using PIN_IO_TSPM as both mask and value is intentional.

Thanks
Lakshay

[PATCH v7 4/5] rtc: pcf85363: add oscillator offset calibration support

[Lakshay Piplani]

Expose the oscillator offset register of PCF85263/PCF85363 through the
read_offset and set_offset rtc_class_ops callbacks, so userspace can
apply frequency correction for drift compensation.

The offset is in parts per billion (ppb). CTRL_OFFSET is a signed 8-bit
step count whose size depends on the OFFM bit: 2170 ppb/step normal and
2034.5 ppb/step fast, the latter computed with an x10-scaled constant to
keep the arithmetic integer. Values are range-checked before that scaling
so the multiply cannot overflow, and out-of-range values are rejected
with -ERANGE rather than clamped.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>

---
V6 -> V7:
- No change.

V5 -> V6:
- No change

V4 -> V5:
- Report and accept the offset in ppb (per the RTC offset ABI) using the
  step selected by OFFM (2170 ppb, or 2034.5 ppb fast via x10 scaling).
- Range-check before the x10 scaling to avoid overflow, and reject
  out-of-range values with -ERANGE instead of clamping.

V3 -> V4:
- No changes in v4.

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 | 67 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index ef846dd231a0..891120c5955f 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -115,6 +115,7 @@
 #define OSC_CAP_SEL	GENMASK(1, 0)
 #define OSC_CAP_6000	0x01
 #define OSC_CAP_12500	0x02
+#define OSC_OFFM	BIT(6)
 
 #define STOP_EN_STOP	BIT(0)
 
@@ -131,6 +132,14 @@
 #define TSR2_SHIFT      2
 #define TSR3_SHIFT      6
 
+#define OFFSET_SIGN_BIT 7
+#define OFFSET_MASK     0xFF
+/* Offset step in ppb; fast mode (OFFM=1) is scaled x10 for integer maths. */
+#define OFFSET_STEP_PPB		2170
+#define OFFSET_STEP_PPB_FAST_X10	20345
+#define OFFSET_PPB_MAX		(127 * OFFSET_STEP_PPB)
+#define OFFSET_PPB_MIN		(-128 * OFFSET_STEP_PPB)
+
 #define PCF85363_NUM_TS		3
 /* Bytes latched per timestamp register (sec, min, hour, day, mon, year). */
 #define PCF85363_TS_LEN		6
@@ -554,6 +563,62 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 	return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
+/*
+ * CTRL_OFFSET is a signed step count; the step is 2170 ppb (normal) or
+ * 2034.5 ppb (fast/OFFM, scaled x10 to keep the arithmetic integer).
+ */
+static int pcf85363_read_offset(struct device *dev, long *offset)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+	unsigned int val, osc;
+	long steps;
+	int ret;
+
+	ret = regmap_read(pcf85363->regmap, CTRL_OFFSET, &val);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(pcf85363->regmap, CTRL_OSCILLATOR, &osc);
+	if (ret)
+		return ret;
+
+	steps = sign_extend32(val, OFFSET_SIGN_BIT);
+
+	if (osc & OSC_OFFM)
+		*offset = steps * OFFSET_STEP_PPB_FAST_X10 / 10;
+	else
+		*offset = steps * OFFSET_STEP_PPB;
+
+	return 0;
+}
+
+static int pcf85363_set_offset(struct device *dev, long offset)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+	unsigned int osc;
+	long steps;
+	int ret;
+
+	ret = regmap_read(pcf85363->regmap, CTRL_OSCILLATOR, &osc);
+	if (ret)
+		return ret;
+
+	/* Range-check before the x10 scaling so the multiply cannot overflow. */
+	if (offset > OFFSET_PPB_MAX || offset < OFFSET_PPB_MIN)
+		return -ERANGE;
+
+	if (osc & OSC_OFFM)
+		steps = DIV_ROUND_CLOSEST(offset * 10, OFFSET_STEP_PPB_FAST_X10);
+	else
+		steps = DIV_ROUND_CLOSEST(offset, OFFSET_STEP_PPB);
+
+	if (steps < -128 || steps > 127)
+		return -ERANGE;
+
+	return regmap_write(pcf85363->regmap, CTRL_OFFSET,
+			    steps & OFFSET_MASK);
+}
+
 static int pcf85363_rtc_ioctl(struct device *dev,
 			      unsigned int cmd, unsigned long arg)
 {
@@ -601,6 +666,8 @@ static const struct rtc_class_ops rtc_ops = {
 	.read_alarm	= pcf85363_rtc_read_alarm,
 	.set_alarm	= pcf85363_rtc_set_alarm,
 	.alarm_irq_enable = pcf85363_rtc_alarm_irq_enable,
+	.read_offset = pcf85363_read_offset,
+	.set_offset = pcf85363_set_offset,
 };
 
 static int pcf85363_nvram_read(void *priv, unsigned int offset, void *val,
-- 
2.25.1

Re: [PATCH v7 4/5] rtc: pcf85363: add oscillator offset calibration support

[sashiko-bot]

> Expose the oscillator offset register of PCF85263/PCF85363 through the
> read_offset and set_offset rtc_class_ops callbacks, so userspace can
> apply frequency correction for drift compensation.
> 
> The offset is in parts per billion (ppb). CTRL_OFFSET is a signed 8-bit
> step count whose size depends on the OFFM bit: 2170 ppb/step normal and
> 2034.5 ppb/step fast, the latter computed with an x10-scaled constant to
> keep the arithmetic integer. Values are range-checked before that scaling
> so the multiply cannot overflow, and out-of-range values are rejected
> with -ERANGE rather than clamped.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

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

[Lakshay Piplani]

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.

The chip has no reset output; expiry is only signalled via the WDF flag
routed to INTA, so this is an alarm-only watchdog (WDIOF_ALARMONLY). WDIE
is toggled on start/stop with rollback-safe register ordering, expiry is
rate-limited from the IRQ handler, and the watchdog is only registered
when an interrupt line is present. WDF is cleared through the
write-0-to-clear helper.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>

---
V6 -> V7:
- Return IRQ_NONE and rate-limit the message when the WDF flag cannot
  be cleared, following the shared IRQ policy from patch 2.
- Factor arming into pcf85363_wdt_arm(): mask WDIE, clear stale WDF,
  reload, then enable WDIE so a latched flag cannot assert INTA early.
  On .start() a failed WDIE-enable halts the counter (the core only
  tracks the device on success); on resume the counter is never halted
  as the watchdog may already be active with nowayout set.
- Only mask WDIE in the devres cleanup instead of stopping the counter,
  so unbinding a nowayout watchdog does not bypass its stop policy while
  still detaching WDF from INTA before the managed IRQ is released.
- Mask WDIE on suspend without halting the counter (honouring nowayout)
  and re-arm on resume via pcf85363_wdt_arm(), still paired with
  watchdog_stop_ping_on_suspend().

V5 -> V6:
- Adopt an already-running watchdog (bootloader- or battery-backed):
  read CTRL_WDOG until two reads agree, set WDOG_HW_RUNNING and re-arm
  WDIE when it is running, otherwise leave WDIE masked. Do not decode
  the live countdown as the configured timeout; keep the fixed default
  and let the core's first keep-alive reload it
- Report watchdog probe failures via dev_err_probe()

V4 -> V5:
- Select WATCHDOG_CORE if WATCHDOG so the device can register.
- Scale the timeout by the clock select with one count of margin (1 Hz
  step up to 30 s, else 0.25 Hz, max 120 s) and report it back.
- Alarm-only watchdog (WDIOF_ALARMONLY): expiry is only signalled via WDF
  routed to INTA; toggle WDIE on start/stop with rollback-safe ordering.
- Don't program WDR from .set_timeout when inactive (it would start the
  watchdog); only reload when watchdog_active(), and drop the clamp.
- Register the watchdog only when an interrupt line is present.
- Use a fixed default timeout (no timeout-sec); userspace can change it.
- Give the watchdog struct a back-pointer to struct pcf85363 and clear
  WDF via the shared write-0-to-clear helper.

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/Kconfig        |   1 +
 drivers/rtc/rtc-pcf85363.c | 392 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 391 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 05b9233b9418..4871cb3fb24d 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -516,6 +516,7 @@ config RTC_DRV_PCF8523
 config RTC_DRV_PCF85363
 	tristate "NXP PCF85363"
 	select REGMAP_I2C
+	select WATCHDOG_CORE if WATCHDOG
 	help
 	  If you say yes here you get support for the PCF85363 RTC chip.
 
diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 891120c5955f..a22d37c58318 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -21,8 +21,10 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/string.h>
+#include <linux/watchdog.h>
 
 #include <dt-bindings/rtc/pcf85363-tsr.h>
 
@@ -147,12 +149,32 @@
 #define PCF85363_SEC_MASK	0x7F
 #define PCF85363_TS_READ_RETRIES	3
 
+#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_MODE_REPEAT	BIT(7)
+
+#define WD_DEFAULT_TIMEOUT  10
+#define WD_TIMEOUT_MIN	1
+#define WD_COUNT_MAX	0x1F
+/* Longest guaranteed timeout at the 0.25 Hz step (WDR=31, one count margin). */
+#define WD_TIMEOUT_MAX	120
+/* Longest timeout served by the 1 Hz step (WDR=31, one count margin). */
+#define WD_TIMEOUT_1HZ_MAX	30
+/* CTRL_WDOG reads back the live countdown; retry until two reads agree. */
+#define WD_READ_RETRIES		3
+
 /* Cached timestamp; the flag is cleared once the value is copied here. */
 struct pcf85363_ts {
 	bool valid;
 	u8 regs[PCF85363_TS_LEN];
 };
 
+struct pcf85363_watchdog;
+
 struct pcf85363 {
 	struct rtc_device	*rtc;
 	struct regmap		*regmap;
@@ -162,6 +184,7 @@ struct pcf85363 {
 	struct pcf85363_ts	ts[PCF85363_NUM_TS];
 	/* Per-TSR: true when the register uses a last-event capture mode. */
 	bool			ts_last_event[PCF85363_NUM_TS];
+	struct pcf85363_watchdog *watchdog;
 };
 
 struct pcf85x63_config {
@@ -169,6 +192,14 @@ struct pcf85x63_config {
 	unsigned int num_nvram;
 };
 
+struct pcf85363_watchdog {
+	struct watchdog_device wdd;
+	struct pcf85363 *pcf85363;
+	u8 timeout_val;
+	u8 clock_sel;
+	bool suspended;
+};
+
 /*
  * CTRL_FLAGS is write-0-to-clear, so write the complement of the mask to
  * clear only the requested bits without disturbing the others.
@@ -501,12 +532,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) {
@@ -536,6 +568,21 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 		}
 	}
 
+	if (flags & FLAGS_WDF) {
+		dev_warn_ratelimited(&pcf85363->rtc->dev,
+				     "watchdog timer expired\n");
+
+		err = pcf85363_clear_flags(pcf85363, FLAGS_WDF);
+		if (err) {
+			dev_err_ratelimited(&pcf85363->rtc->dev,
+					    "failed to clear watchdog flag: %d\n",
+					    err);
+			ack_failed = true;
+		} else {
+			handled = true;
+		}
+	}
+
 	/*
 	 * Clear flags this handler does not service (e.g. A2F/PIF); otherwise
 	 * they hold the level-triggered INTA line asserted and storm the IRQ.
@@ -730,6 +777,287 @@ static const struct pcf85x63_config pcf_85363_config = {
 	.num_nvram = 2
 };
 
+/*
+ * Program the watchdog counter (WDR) so the reported timeout is never
+ * shorter than requested: the first period after a reload lasts between
+ * WDR and WDR-1 counts, so add one count of margin. Timeouts up to 30 s
+ * use the 1 Hz step (1 s/count); longer ones the 0.25 Hz step (4 s/count).
+ */
+static void pcf85363_wdt_select_clock(struct pcf85363_watchdog *wd)
+{
+	unsigned int timeout = wd->wdd.timeout;
+
+	if (timeout <= WD_TIMEOUT_1HZ_MAX) {
+		wd->clock_sel = WD_CLKSEL_1HZ;
+		wd->timeout_val = timeout + 1;
+		wd->wdd.timeout = timeout;
+	} else {
+		wd->clock_sel = WD_CLKSEL_0_25HZ;
+		wd->timeout_val = DIV_ROUND_UP(timeout, 4) + 1;
+		wd->wdd.timeout = DIV_ROUND_UP(timeout, 4) * 4;
+	}
+}
+
+/* Repeat mode restarts the watchdog automatically after each period. */
+static int pcf85363_wdt_reload(struct pcf85363_watchdog *wd)
+{
+	u8 val;
+
+	val = WD_MODE_REPEAT |
+	      ((wd->timeout_val & WD_COUNT_MAX) << WD_TIMEOUT_SHIFT) |
+	      (wd->clock_sel & WD_CLKSEL_MASK);
+
+	return regmap_write(wd->pcf85363->regmap, CTRL_WDOG, val);
+}
+
+/*
+ * Arm the watchdog: mask WDIE, clear any stale WDF, reload the counter, then
+ * enable WDIE last so a latched flag cannot assert INTA before the counter is
+ * reloaded. pcf85363_wdt_reload() starts the counter, so if the final WDIE
+ * enable fails the watchdog is already running.
+ *
+ * On the .start() path (rollback) the watchdog core only marks the device
+ * active/hw-running after .start() returns success, so a failure here must
+ * leave the hardware stopped or the running watchdog would be left untracked:
+ * halt the counter and keep WDIE masked before returning the error. On the
+ * resume path (!rollback) the watchdog may already be active - possibly with
+ * nowayout set - so never halt the counter on failure; just report the error.
+ */
+static int pcf85363_wdt_arm(struct pcf85363_watchdog *wd, bool rollback)
+{
+	struct regmap *regmap = wd->pcf85363->regmap;
+	int ret;
+
+	/* Mask WDF from INTA while the watchdog is being re-armed. */
+	ret = regmap_update_bits(regmap, CTRL_INTA_EN, INT_WDIE, 0);
+	if (ret)
+		return ret;
+
+	ret = pcf85363_clear_flags(wd->pcf85363, FLAGS_WDF);
+	if (ret)
+		return ret;
+
+	ret = pcf85363_wdt_reload(wd);
+	if (ret)
+		return ret;
+
+	/* Route WDF to INTA only after the flag is clear and counter reloaded. */
+	ret = regmap_update_bits(regmap, CTRL_INTA_EN, INT_WDIE, INT_WDIE);
+	if (ret && rollback) {
+		/* Counter is running but WDIE enable failed; undo the start. */
+		regmap_write(regmap, CTRL_WDOG, 0);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int pcf85363_wdt_start(struct watchdog_device *wdd)
+{
+	return pcf85363_wdt_arm(watchdog_get_drvdata(wdd), true);
+}
+
+static int pcf85363_wdt_stop(struct watchdog_device *wdd)
+{
+	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+	int ret, irq_ret;
+
+	/* Halt the counter first so a failed disable cannot leave it armed. */
+	ret = regmap_write(wd->pcf85363->regmap, CTRL_WDOG, 0);
+
+	/*
+	 * Always attempt to mask WDIE. Even if halting the counter failed the
+	 * interrupt must not stay routed to INTA after the driver is removed.
+	 */
+	irq_ret = regmap_update_bits(wd->pcf85363->regmap, CTRL_INTA_EN,
+				     INT_WDIE, 0);
+
+	return ret ? ret : irq_ret;
+}
+
+static int pcf85363_wdt_ping(struct watchdog_device *wdd)
+{
+	struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+	int ret;
+
+	ret = pcf85363_clear_flags(wd->pcf85363, FLAGS_WDF);
+	if (ret)
+		return ret;
+
+	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);
+
+	wdd->timeout = timeout;
+
+	pcf85363_wdt_select_clock(wd);
+
+	/*
+	 * Programming the counter with a non-zero value starts it, so only
+	 * reprogram when the watchdog is already running; the core keeps the
+	 * new timeout for the next start otherwise.
+	 */
+	if (!watchdog_active(wdd))
+		return 0;
+
+	return pcf85363_wdt_reload(wd);
+}
+
+static const struct watchdog_info pcf85363_wdt_info = {
+	.identity = "PCF85363 Watchdog",
+	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_ALARMONLY,
+};
+
+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,
+};
+
+/*
+ * CTRL_WDOG reads back the live countdown, which decrements while the
+ * watchdog runs, so a single read can catch a transient value. Read until
+ * two consecutive reads agree before deciding whether the watchdog is armed.
+ */
+static int pcf85363_read_wdog_stable(struct pcf85363 *pcf85363, unsigned int *out)
+{
+	unsigned int prev, cur;
+	int retries, ret;
+
+	ret = regmap_read(pcf85363->regmap, CTRL_WDOG, &prev);
+	if (ret)
+		return ret;
+
+	for (retries = 0; retries < WD_READ_RETRIES; retries++) {
+		ret = regmap_read(pcf85363->regmap, CTRL_WDOG, &cur);
+		if (ret)
+			return ret;
+
+		if (cur == prev) {
+			*out = cur;
+			return 0;
+		}
+
+		prev = cur;
+	}
+
+	*out = cur;
+	return 0;
+}
+
+/*
+ * Registered as a devres action before the watchdog device, so on unbind it
+ * runs after watchdog unregister but before the managed IRQ is released.
+ *
+ * Mask WDIE only; do not halt the counter. Halting the counter is the
+ * nowayout-governed stop that the watchdog core performs on unregister (and
+ * deliberately refuses when nowayout is set). The core only stops a WDOG_ACTIVE
+ * device, so an adopted WDOG_HW_RUNNING watchdog userspace never opened, or an
+ * active nowayout watchdog, is still counting here. Masking WDIE disconnects
+ * WDF from the level-triggered INTA line so it cannot storm the interrupt once
+ * the handler is freed, in every case, without bypassing nowayout.
+ */
+static void pcf85363_wdt_cleanup(void *data)
+{
+	struct pcf85363_watchdog *wd = data;
+	int ret;
+
+	ret = regmap_update_bits(wd->pcf85363->regmap, CTRL_INTA_EN,
+				 INT_WDIE, 0);
+	if (ret)
+		dev_warn(wd->wdd.parent,
+			 "failed to mask watchdog interrupt during cleanup: %d\n",
+			 ret);
+}
+
+static int pcf85363_watchdog_init(struct device *dev, struct pcf85363 *pcf85363)
+{
+	struct pcf85363_watchdog *wd;
+	unsigned int regval;
+	bool running;
+	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.
+	 * CTRL_WDOG reads back the live countdown, not the programmed reload
+	 * value, so it must not be decoded into wdd.timeout: adopt the known
+	 * default and let the watchdog core's first keep-alive reload it.
+	 */
+	pcf85363_wdt_select_clock(wd);
+
+	/*
+	 * The watchdog state is battery-backed and may already be running at
+	 * probe. Adopt it so the watchdog core keeps it serviced; only a
+	 * stopped watchdog gets WDIE masked so a stale enable cannot storm the
+	 * level-triggered INTA line.
+	 */
+	ret = pcf85363_read_wdog_stable(pcf85363, &regval);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to read watchdog register\n");
+
+	running = ((regval >> WD_TIMEOUT_SHIFT) & WD_COUNT_MAX) != 0;
+
+	ret = pcf85363_clear_flags(pcf85363, FLAGS_WDF);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to clear WDF\n");
+
+	/*
+	 * Once WDOG_HW_RUNNING is set the core keeps the watchdog alive via
+	 * .ping(), which does not touch WDIE, so arm WDIE here for an adopted
+	 * watchdog; otherwise keep it masked.
+	 */
+	ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN, INT_WDIE,
+				 running ? INT_WDIE : 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to configure WDIE\n");
+
+	if (running)
+		set_bit(WDOG_HW_RUNNING, &wd->wdd.status);
+
+	watchdog_set_drvdata(&wd->wdd, wd);
+	watchdog_stop_on_unregister(&wd->wdd);
+	watchdog_stop_ping_on_suspend(&wd->wdd);
+
+	/*
+	 * Register the cleanup before the watchdog device so devres unwinds it
+	 * after watchdog unregister but before the managed IRQ is freed.
+	 */
+	ret = devm_add_action_or_reset(dev, pcf85363_wdt_cleanup, wd);
+	if (ret)
+		return ret;
+
+	ret = devm_watchdog_register_device(dev, &wd->wdd);
+	if (ret)
+		return ret;
+
+	pcf85363->watchdog = wd;
+
+	return 0;
+}
+
 /* Six BCD bytes; bit 7 of the seconds byte is reserved, not time data. */
 static ssize_t pcf85363_format_timestamp(const u8 *regs, char *buf)
 {
@@ -1010,6 +1338,16 @@ static int pcf85363_probe(struct i2c_client *client)
 		clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
 	}
 
+	/*
+	 * Watchdog expiry is only signalled via INTA (no reset output), so it
+	 * needs an interrupt line to be usable.
+	 */
+	if (irq_a > 0) {
+		ret = pcf85363_watchdog_init(dev, pcf85363);
+		if (ret)
+			return dev_err_probe(dev, ret, "Watchdog init failed\n");
+	}
+
 	dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
 
 	ret = rtc_add_group(pcf85363->rtc, &pcf85363_attr_group);
@@ -1028,6 +1366,55 @@ static int pcf85363_probe(struct i2c_client *client)
 	return ret;
 }
 
+/*
+ * The watchdog only signals expiry through INTA, which is a system wakeup
+ * source, so a running watchdog would fire a spurious wakeup during sleep.
+ * Mask WDIE on suspend to suppress that wakeup without halting the counter,
+ * so nowayout is honoured, and re-arm it on resume; the RTC alarm can still
+ * wake the system.
+ */
+static int pcf85363_suspend(struct device *dev)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+	struct pcf85363_watchdog *wd = pcf85363->watchdog;
+	int ret;
+
+	if (!wd || !(watchdog_active(&wd->wdd) || watchdog_hw_running(&wd->wdd)))
+		return 0;
+
+	/* Disconnect WDF from the wakeup-capable INTA line; keep counting. */
+	ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN, INT_WDIE, 0);
+	if (ret)
+		return ret;
+
+	wd->suspended = true;
+
+	return 0;
+}
+
+static int pcf85363_resume(struct device *dev)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+	struct pcf85363_watchdog *wd = pcf85363->watchdog;
+	int ret;
+
+	if (!wd || !wd->suspended)
+		return 0;
+
+	/*
+	 * The watchdog may already be active - possibly with nowayout set - so
+	 * re-arm without ever halting the counter on failure.
+	 */
+	ret = pcf85363_wdt_arm(wd, false);
+	if (!ret)
+		wd->suspended = false;
+
+	return ret;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pcf85363_pm_ops, pcf85363_suspend,
+				pcf85363_resume);
+
 static const __maybe_unused struct of_device_id dev_ids[] = {
 	{ .compatible = "nxp,pcf85263", .data = &pcf_85263_config },
 	{ .compatible = "nxp,pcf85363", .data = &pcf_85363_config },
@@ -1039,6 +1426,7 @@ static struct i2c_driver pcf85363_driver = {
 	.driver	= {
 		.name	= "pcf85363",
 		.of_match_table = of_match_ptr(dev_ids),
+		.pm = pm_sleep_ptr(&pcf85363_pm_ops),
 	},
 	.probe = pcf85363_probe,
 };
-- 
2.25.1

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

[sashiko-bot]

> 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.
> 
> The chip has no reset output; expiry is only signalled via the WDF flag
> routed to INTA, so this is an alarm-only watchdog (WDIOF_ALARMONLY). WDIE
> is toggled on start/stop with rollback-safe register ordering, expiry is
> rate-limited from the IRQ handler, and the watchdog is only registered
> when an interrupt line is present. WDF is cleared through the
> write-0-to-clear helper.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH v7 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config

[sashiko-bot]

> NXP PCF85263/PCF85363 provides three timestamp registers (TSR1-TSR3)
> which latch the current time when a selected event occurs. Add a
> vendor specific property, nxp,timestamp-mode, to select the event
> source for each register.
> 
> Also introduce a new header 'pcf85363-tsr.h' to expose
> macros for timestamp mode fields, improving readability
> of device tree file.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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