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

[RESEND v4 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>
---
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 */
-- 
2.25.1

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

[Lakshay Piplani]

Add battery switch-over reporting for PCF85263/PCF85363 using the standard
RTC_VL_* ioctl interface. When the backup supply takes over, the BSF flag
is exposed to userspace through RTC_VL_READ and can be cleared using
RTC_VL_CLR.

This allows applications to detect loss of main power without relying on
non-standard interfaces.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
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 | 49 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 540042b9eec8..c03d5a65c5f7 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/bcd.h>
+#include <linux/device.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
 
@@ -295,23 +296,67 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 {
 	struct pcf85363 *pcf85363 = i2c_get_clientdata(dev_id);
+	bool handled = false;
 	unsigned int flags;
 	int err;
 
 	err = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
+
 	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;
+		handled = true;
 	}
 
-	return IRQ_NONE;
+	if (flags & FLAGS_BSF) {
+		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
+		handled = true;
+	}
+
+	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);
+	unsigned int val;
+	int ret;
+
+	switch (cmd) {
+	case RTC_VL_READ: {
+		u32 status = 0;
+
+		ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &val);
+
+		if (ret)
+			return ret;
+
+		if (val & FLAGS_BSF)
+			status |= RTC_VL_BACKUP_SWITCH;
+
+		return put_user(status, (u32 __user *)arg);
+	}
+
+	case RTC_VL_CLR:
+		return regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 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,
-- 
2.25.1

[RESEND v4 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode

[Lakshay Piplani]

Add support for the timestamp capture registers available on PCF85263 and
PCF85363. The registers latch the current time when selected events occur,
such as TS pin activation or battery switch-over.

The capture source can be configured via the nxp,timestamp-mode device
tree property, and latched values are exported through read-only sysfs
attributes.

Additionally:
- Use rtc_add_group() instead of sysfs_create_group() to register the
  timestamp attributes under the RTC class device (/sys/class/rtc/rtcX).
- Perform minor cleanups in the probe function for better readability.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
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 | 209 +++++++++++++++++++++++++++++++------
 1 file changed, 175 insertions(+), 34 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index c03d5a65c5f7..e10e58f69012 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -16,6 +16,7 @@
 #include <linux/bcd.h>
 #include <linux/device.h>
 #include <linux/of.h>
+#include <linux/rtc.h>
 #include <linux/regmap.h>
 
 /*
@@ -101,19 +102,31 @@
 #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
+
 struct pcf85363 {
 	struct rtc_device	*rtc;
 	struct regmap		*regmap;
+	u8 ts_valid_flags;
 };
 
 struct pcf85x63_config {
@@ -306,8 +319,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]" : "");
 	}
 
@@ -317,6 +333,24 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 		handled = true;
 	}
 
+	if (flags & FLAGS_TSR1F) {
+		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR1F, 0);
+		pcf85363->ts_valid_flags |= FLAGS_TSR1F;
+		handled = true;
+	}
+
+	if (flags & FLAGS_TSR2F) {
+		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR2F, 0);
+		pcf85363->ts_valid_flags |= FLAGS_TSR2F;
+		handled = true;
+	}
+
+	if (flags & FLAGS_TSR3F) {
+		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR3F, 0);
+		pcf85363->ts_valid_flags |= FLAGS_TSR3F;
+		handled = true;
+	}
+
 	if (flags & FLAGS_BSF) {
 		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
 		handled = true;
@@ -424,11 +458,94 @@ static const struct pcf85x63_config pcf_85363_config = {
 	.num_nvram = 2
 };
 
+/*
+ * Reads 6 bytes of timestamp data starting at the given base register,
+ * converts them from BCD to binary, and formats the result into a
+ * human-readable string in "YYYY-MM-DD HH:MM:SS" format.
+ */
+static int pcf85363_read_timestamp(struct pcf85363 *pcf85363, u8 base_reg, char *buf)
+{
+	struct rtc_time tm;
+	u8 regs[6];
+	int ret;
+
+	ret = regmap_bulk_read(pcf85363->regmap, base_reg, regs, sizeof(regs));
+
+	if (ret)
+		return ret;
+
+	tm.tm_sec = bcd2bin(regs[0]);
+	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);
+}
+
+/*
+ * Checks whether a specific timestamp flag is set. If so, reads and
+ * returns the formatted timestamp. Otherwise, returns "00-00-00 00:00:00".
+ */
+
+static ssize_t pcf85363_timestamp_show(struct device *dev, char *buf,
+				       u8 timestamp_flag, u8 base_reg)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+
+	if (!(pcf85363->ts_valid_flags & timestamp_flag))
+		return sysfs_emit(buf, "00-00-00 00:00:00\n");
+
+	return pcf85363_read_timestamp(pcf85363, base_reg, buf);
+}
+
+static ssize_t timestamp1_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return pcf85363_timestamp_show(dev, buf, FLAGS_TSR1F, DT_TIMESTAMP1);
+}
+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, FLAGS_TSR2F, DT_TIMESTAMP2);
+}
+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, FLAGS_TSR3F, DT_TIMESTAMP3);
+}
+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);
+	struct device *dev = &client->dev;
+	struct pcf85363 *pcf85363;
+	int irq_a = client->irq;
+	bool wakeup_source;
+	int ret, i, err;
+	u32 tsr_mode[3];
+	u8 val;
+
 	static struct nvmem_config nvmem_cfg[] = {
 		{
 			.name = "pcf85x63-",
@@ -446,25 +563,43 @@ 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;
 
+	pcf85363->ts_valid_flags = 0;
+
 	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)) {
+		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)
+			dev_warn(dev, "Failed to write timestamp mode register\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);
@@ -478,38 +613,44 @@ 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");
-	if (client->irq > 0 || wakeup_source) {
-		regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
-		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
-				   PIN_IO_INTAPM, PIN_IO_INTA_OUT);
-	}
+	wakeup_source = device_property_read_bool(dev, "wakeup-source");
 
-	if (client->irq > 0) {
-		unsigned long irqflags = IRQF_TRIGGER_LOW;
+	ret = regmap_write(pcf85363->regmap, CTRL_FLAGS, 0x00);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to clear CTRL_FLAGS\n");
+
+	if (irq_a > 0) {
+		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO, PIN_IO_INTAPM, PIN_IO_INTA_OUT);
+		ret = devm_request_threaded_irq(dev, irq_a, NULL,
+						pcf85363_rtc_handle_irq,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"pcf85363-inta", client);
 
-		if (dev_fwnode(&client->dev))
-			irqflags = 0;
-		ret = devm_request_threaded_irq(&client->dev, client->irq,
-						NULL, pcf85363_rtc_handle_irq,
-						irqflags | IRQF_ONESHOT,
-						"pcf85363", client);
 		if (ret) {
-			dev_warn(&client->dev,
-				 "unable to request IRQ, alarms disabled\n");
-			client->irq = 0;
+			dev_err_probe(dev, ret, "INTA IRQ request failed\n");
+			irq_a = 0;
+		} else {
+			regmap_write(pcf85363->regmap, CTRL_INTA_EN, INT_BSIE
+				     | INT_TSRIE);
 		}
 	}
 
-	if (client->irq > 0 || wakeup_source) {
-		device_init_wakeup(&client->dev, true);
-		set_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
-	} else {
-		clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
-	}
+	regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+			   PIN_IO_TSPM | PIN_IO_TSIM,
+			   PIN_IO_TSPM | PIN_IO_TSIM);
+
+	if (irq_a > 0 || wakeup_source)
+		device_init_wakeup(dev, true);
+
+	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

[RESEND v4 4/5] rtc: pcf85363: add oscillator offset calibration support

[Lakshay Piplani]

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

The correction mode defaults to normal mode (OFFM = 0), where each step
introduces an offset of approximately 2.170 ppm and corrections occur
every 4 hours.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
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 | 46 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index e10e58f69012..665bbbb169b0 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -123,6 +123,11 @@
 #define TSR2_SHIFT      2
 #define TSR3_SHIFT      6
 
+#define OFFSET_SIGN_BIT 7
+#define OFFSET_MINIMUM  -128
+#define OFFSET_MAXIMUM  127
+#define OFFSET_MASK     0xFF
+
 struct pcf85363 {
 	struct rtc_device	*rtc;
 	struct regmap		*regmap;
@@ -359,6 +364,45 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 	return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
+/*
+ * Read the current RTC offset from the CTRL_OFFSET
+ * register. This value is an 8-bit signed 2's complement
+ * value that corrects osciallator drift.
+ */
+static int pcf85363_read_offset(struct device *dev, long *offset)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(pcf85363->regmap, CTRL_OFFSET, &val);
+
+	if (ret)
+		return ret;
+
+	*offset = sign_extend32(val & OFFSET_MASK, OFFSET_SIGN_BIT);
+
+	return 0;
+}
+
+/*
+ * Write an oscillator offset correction value to
+ * the CTRL_OFFSET register. The valid range is
+ * -128 to 127 (8-bit signed), typically used to fine
+ * tune accuracy.
+ */
+static int pcf85363_set_offset(struct device *dev, long offset)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+
+	if (offset < OFFSET_MINIMUM || offset > OFFSET_MAXIMUM) {
+		dev_warn(dev, "Offset out of range: %ld\n", offset);
+		return -ERANGE;
+	}
+
+	return regmap_write(pcf85363->regmap, CTRL_OFFSET, offset & OFFSET_MASK);
+}
+
 static int pcf85363_rtc_ioctl(struct device *dev,
 			      unsigned int cmd, unsigned long arg)
 {
@@ -396,6 +440,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

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

[Lakshay Piplani]

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(-)

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);
 
-- 
2.25.1

Re: [RESEND v4 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/20260812074533.3545058-1-lakshay.piplani@nxp.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The IRQ handler silently clears the Battery Switch-over Flag (BSF) from the hardware register without saving its state, causing battery switch-over events to be lost if an interrupt triggers before userspace calls RTC_VL_READ.

Pre-existing issues:
- [High] `pcf85363_probe()` unconditionally wipes the `CTRL_FLAGS` register, destroying boot-time battery switch-over events before they can be read via `RTC_VL_READ`.
--

--- Patch [2]: [RESEND v4 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL ---
commit 641bd03440f1f0094b5ca119b02a17fb2dd848b5
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: pcf85363: support reporting battery switch-over via RTC_VL

Add battery switch-over reporting for PCF85263/PCF85363 using the standard
RTC_VL_* ioctl interface. When the backup supply takes over, the BSF flag
is exposed to userspace through RTC_VL_READ and can be cleared using
RTC_VL_CLR.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 540042b9eec8f..c03d5a65c5f79 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -295,23 +296,67 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  {
>  	struct pcf85363 *pcf85363 = i2c_get_clientdata(dev_id);
> +	bool handled = false;
>  	unsigned int flags;
>  	int err;
>  
>  	err = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
> +
>  	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;
> +		handled = true;
>  	}
>  
> -	return IRQ_NONE;
> +	if (flags & FLAGS_BSF) {
> +		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
> +		handled = true;
> +	}

[Severity: Medium]
Does this unconditionally clear the hardware Battery Switch-over Flag (BSF)
without saving its state?

If an interrupt triggers before userspace calls RTC_VL_READ, won't the
battery switch-over event be lost since the hardware flag is cleared but not
cached in software?

> +
> +	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);
> +	unsigned int val;
> +	int ret;
> +
> +	switch (cmd) {
> +	case RTC_VL_READ: {
> +		u32 status = 0;
> +
> +		ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &val);

[Severity: Medium]
Because RTC_VL_READ reads directly from the hardware register, it will return
0 if the IRQ handler has already cleared the flag.

> +
> +		if (ret)
> +			return ret;
> +
> +		if (val & FLAGS_BSF)
> +			status |= RTC_VL_BACKUP_SWITCH;
> +
> +		return put_user(status, (u32 __user *)arg);
> +	}
> +
> +	case RTC_VL_CLR:
> +		return regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
> +
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the pre-existing code
in pcf85363_probe() appears to break the boot-time power loss detection
intended by this new feature.

During system boot, pcf85363_probe() unconditionally clears all hardware flags
if an IRQ or wakeup source is configured:

drivers/rtc/rtc-pcf85363.c:pcf85363_probe() {
    ...
	if (client->irq > 0 || wakeup_source) {
		regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
				   PIN_IO_INTAPM, PIN_IO_INTA_OUT);
	}
    ...
}

Does this destroy the battery switch-over event before userspace can query it
with RTC_VL_READ?

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

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

[sashiko-bot]

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.

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

Re: [RESEND v4 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [High] The driver unconditionally hardcodes IRQF_TRIGGER_LOW, overriding firmware-specified IRQ trigger polarity.
- [High] Using regmap_write on CTRL_INTA_EN clobbers existing interrupt enable bits for alarms and watchdog.
- [High] Timestamp data tearing and loss due to immediate interrupt clearing without caching.
- [Medium] Timestamp and battery switch-over events captured while powered off are permanently lost during probe.
- [Medium] RTC_FEATURE_ALARM is falsely advertised on devices lacking an interrupt and wakeup source due to a removed cleanup path.
- [Medium] Timestamps are inaccessible on systems without an IRQ line (polling mode).
- [Medium] The OS (Oscillator Stop) flag is not masked when reading the timestamp seconds, resulting in corrupted time values.

Pre-existing issues:
- [High] Lost interrupts due to Read-Modify-Write (RMW) on write-0-to-clear hardware status register.
--

--- Patch [3]: [RESEND v4 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode ---
commit 8204c1848e74a3f22f0093e4140b50cfeec66fb5
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: pcf85363: add timestamp support with configurable timestamp mode

This commit adds support for the timestamp capture registers available on
PCF85263 and PCF85363 devices. The capture source is configured via the
device tree, 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 c03d5a65c5f79..e10e58f690127 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -317,6 +333,24 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  		handled = true;
>  	}
>  
> +	if (flags & FLAGS_TSR1F) {
> +		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR1F, 0);

[Severity: High]
This is a pre-existing issue, but does using regmap_update_bits() here cause
lost interrupts?

The CTRL_FLAGS register uses a write-0-to-clear mechanism. Since
regmap_update_bits() performs a read-modify-write, any new hardware interrupt
asserted between the read and the write could be silently cleared when the
read value of 0 is written back.

[Severity: High]
Does clearing FLAGS_TSR1F immediately upon receiving the interrupt cause
timestamp data tearing?

By clearing the flag before caching the data, the hardware is re-armed. If
a new hardware event fires while sysfs is reading the 6 bytes via
regmap_bulk_read() in pcf85363_read_timestamp(), the data read could be
corrupted or lost.

> +		pcf85363->ts_valid_flags |= FLAGS_TSR1F;
> +		handled = true;
> +	}
[ ... ]
> @@ -424,11 +458,94 @@ static const struct pcf85x63_config pcf_85363_config = {
> +static int pcf85363_read_timestamp(struct pcf85363 *pcf85363, u8 base_reg, char *buf)
> +{
[ ... ]
> +	ret = regmap_bulk_read(pcf85363->regmap, base_reg, regs, sizeof(regs));
> +
> +	if (ret)
> +		return ret;
> +
> +	tm.tm_sec = bcd2bin(regs[0]);

[Severity: Medium]
Does this bcd2bin conversion fail to mask the OS (Oscillator Stop) flag?

The PCF85363 copies the OS flag into bit 7 of the timestamp seconds register.
If the OS flag is set, passing the unmasked regs[0] directly to bcd2bin()
might result in corrupted time values. Should this be masked with 0x7F first?

> +	tm.tm_min = bcd2bin(regs[1]);
[ ... ]
> +static ssize_t pcf85363_timestamp_show(struct device *dev, char *buf,
> +				       u8 timestamp_flag, u8 base_reg)
> +{
> +	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> +
> +	if (!(pcf85363->ts_valid_flags & timestamp_flag))
> +		return sysfs_emit(buf, "00-00-00 00:00:00\n");

[Severity: Medium]
Does this prevent timestamps from working on polling systems?

The ts_valid_flags variable is purely memory-backed and is only updated
inside the pcf85363_rtc_handle_irq() interrupt handler. On systems without an
IRQ line, this variable is never updated, which means timestamps will always
silently return "00-00-00 00:00:00".

> +
> +	return pcf85363_read_timestamp(pcf85363, base_reg, buf);
> +}
[ ... ]
> @@ -478,38 +613,44 @@ static int pcf85363_probe(struct i2c_client *client)
[ ... ]
> -	wakeup_source = device_property_read_bool(&client->dev,
> -						  "wakeup-source");
> -	if (client->irq > 0 || wakeup_source) {
> -		regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
> -		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> -				   PIN_IO_INTAPM, PIN_IO_INTA_OUT);
> -	}
> +	wakeup_source = device_property_read_bool(dev, "wakeup-source");
>  
> -	if (client->irq > 0) {
> -		unsigned long irqflags = IRQF_TRIGGER_LOW;
> +	ret = regmap_write(pcf85363->regmap, CTRL_FLAGS, 0x00);

[Severity: Medium]
Does this unconditional write destroy timestamp and battery switch-over
events captured while the device was powered off?

Since CTRL_FLAGS is cleared during probe before reading the existing
timestamp flags, pre-boot timestamp indicators are permanently lost.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to clear CTRL_FLAGS\n");
> +
> +	if (irq_a > 0) {
> +		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO, PIN_IO_INTAPM, PIN_IO_INTA_OUT);
> +		ret = devm_request_threaded_irq(dev, irq_a, NULL,
> +						pcf85363_rtc_handle_irq,
> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +						"pcf85363-inta", client);

[Severity: High]
Does hardcoding IRQF_TRIGGER_LOW here break compatibility with systems that
specify a different IRQ polarity in firmware?

The baseline code gracefully degraded the flags to 0 if dev_fwnode() returned
true, allowing device tree or ACPI to supply the correct trigger flags.
Overriding this with a hardcoded value could break interrupt delivery on
affected platforms.

> -		if (dev_fwnode(&client->dev))
> -			irqflags = 0;
> -		ret = devm_request_threaded_irq(&client->dev, client->irq,
> -						NULL, pcf85363_rtc_handle_irq,
> -						irqflags | IRQF_ONESHOT,
> -						"pcf85363", client);
>  		if (ret) {
> -			dev_warn(&client->dev,
> -				 "unable to request IRQ, alarms disabled\n");
> -			client->irq = 0;
> +			dev_err_probe(dev, ret, "INTA IRQ request failed\n");
> +			irq_a = 0;
> +		} else {
> +			regmap_write(pcf85363->regmap, CTRL_INTA_EN, INT_BSIE
> +				     | INT_TSRIE);

[Severity: High]
Does using regmap_write() here unintentionally clear other interrupt
enable bits?

This direct write overwrites the entire CTRL_INTA_EN register, which might
clobber previously configured interrupt enables like INT_A1IE for alarms,
breaking alarm capabilities.

>  		}
>  	}
>  
> -	if (client->irq > 0 || wakeup_source) {
> -		device_init_wakeup(&client->dev, true);
> -		set_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
> -	} else {
> -		clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
> -	}

[Severity: Medium]
Does removing this else block falsely advertise RTC_FEATURE_ALARM?

By dropping clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features), the RTC
core might automatically advertise alarm capabilities to userspace on devices
that lack an interrupt and wakeup source.

> +	regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> +			   PIN_IO_TSPM | PIN_IO_TSIM,
> +			   PIN_IO_TSPM | PIN_IO_TSIM);

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

Re: [RESEND v4 4/5] rtc: pcf85363: add oscillator offset calibration support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] The offset callbacks operate in raw register steps instead of parts per billion (ppb), violating the RTC subsystem API.
- [High] The offset value is wrongly treated as an 8-bit signed integer, which stomps on the OFFM (mode) bit and silently triggers Fast Mode.
- [High] The IRQ handler clears the timestamp interrupt flags before the timestamp data is read, creating a hardware race condition.
- [Low] User space can trigger unratelimited dev_warn() by writing out-of-bounds offset values.
- [Low] The `ts_valid_flags` variable is modified and read concurrently without appropriate lockless access annotations.

Pre-existing issues:
- [High] The static `nvmem_cfg` array is modified concurrently during device probe, risking corruption of private data pointers.
--

--- Patch [4]: [RESEND v4 4/5] rtc: pcf85363: add oscillator offset calibration support ---
commit cf1f1a013f4f67e8c4a20b8646213cc7ca971733
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: pcf85363: add oscillator offset calibration support

This commit adds oscillator offset calibration support to the PCF85363 RTC
driver. It exposes the read_offset and set_offset callbacks, allowing
userspace to apply frequency correction for drift compensation.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index e10e58f690127..665bbbb169b05 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
> @@ -123,6 +123,11 @@
>  #define TSR2_SHIFT      2
>  #define TSR3_SHIFT      6
>  
> +#define OFFSET_SIGN_BIT 7
> +#define OFFSET_MINIMUM  -128
> +#define OFFSET_MAXIMUM  127
> +#define OFFSET_MASK     0xFF

[Severity: High]
Does treating this as an 8-bit signed integer stomp on the OFFM (mode) bit?

The PCF85363 CTRL_OFFSET register uses bit 7 as the OFFM (mode) bit and bits
6-0 as a 7-bit two's complement offset (range -64 to +63). Writing a negative
8-bit integer sets bit 7 to 1, silently reconfiguring the chip into Fast Mode,
while a positive integer switches it to Normal Mode.

Should the driver explicitly preserve the OFFM bit and restrict the offset to
a 7-bit value?

[ ... ]
> @@ -359,6 +364,45 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  	return handled ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
> +/*
> + * Read the current RTC offset from the CTRL_OFFSET
> + * register. This value is an 8-bit signed 2's complement
> + * value that corrects osciallator drift.
> + */
> +static int pcf85363_read_offset(struct device *dev, long *offset)
> +{
> +	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(pcf85363->regmap, CTRL_OFFSET, &val);
> +
> +	if (ret)
> +		return ret;
> +
> +	*offset = sign_extend32(val & OFFSET_MASK, OFFSET_SIGN_BIT);
> +
> +	return 0;
> +}
> +
> +/*
> + * Write an oscillator offset correction value to
> + * the CTRL_OFFSET register. The valid range is
> + * -128 to 127 (8-bit signed), typically used to fine
> + * tune accuracy.
> + */
> +static int pcf85363_set_offset(struct device *dev, long offset)
> +{
> +	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
> +
> +	if (offset < OFFSET_MINIMUM || offset > OFFSET_MAXIMUM) {

[Severity: High]
Does this code reject valid ppb offsets from userspace?

The rtc_read_offset and rtc_set_offset kernel API expects offset values in
parts per billion (ppb), but this implementation appears to operate directly
in raw register values.

Consequently, if userspace passes a valid offset like 4340 ppb, wouldn't it be
bounds-checked against [-128, 127] and rejected with -ERANGE, rendering the
API unusable?

> +		dev_warn(dev, "Offset out of range: %ld\n", offset);

[Severity: Low]
Can this unratelimited warning log be triggered directly by userspace?

It appears an out-of-bounds write to the sysfs offset node will spam the
kernel log, which could be avoided by simply returning -ERANGE.

> +		return -ERANGE;
> +	}
> +
> +	return regmap_write(pcf85363->regmap, CTRL_OFFSET, offset & OFFSET_MASK);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but does clearing the timestamp
interrupt flags before reading the timestamp data create a hardware race
condition?

In pcf85363_rtc_handle_irq():

    if (flags & FLAGS_TSR1F) {
        regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR1F, 0);
        pcf85363->ts_valid_flags |= FLAGS_TSR1F;
        handled = true;
    }

The driver exposes timestamp registers via sysfs which performs a lockless
regmap_bulk_read over I2C in pcf85363_read_timestamp(). According to the
PCF85363 behavior, clearing FLAGS_TSR1F rearms the hardware latch.

If a new timestamp event occurs while userspace is in the middle of reading
the timestamp registers, couldn't the hardware update the registers
concurrently and result in a torn read?

[Severity: Low]
This is a pre-existing issue, but is the lockless access to ts_valid_flags
missing necessary concurrency annotations?

The flag is modified in the threaded IRQ handler:

drivers/rtc/rtc-pcf85363.c:pcf85363_rtc_handle_irq() {
...
    pcf85363->ts_valid_flags |= FLAGS_TSR1F;
...
}

And read locklessly in sysfs:

drivers/rtc/rtc-pcf85363.c:pcf85363_timestamp_show() {
...
    if (!(pcf85363->ts_valid_flags & timestamp_flag))
...
}

While it's a 1-byte read/write that won't practically tear on supported
architectures, missing READ_ONCE() and WRITE_ONCE() annotations violates
kernel concurrency standards and could trigger KCSAN warnings.

[Severity: High]
This is a pre-existing issue, but does modifying the static nvmem_cfg array
during device probe risk corrupting private data pointers?

drivers/rtc/rtc-pcf85363.c:pcf85363_probe() {
...
    static struct nvmem_config nvmem_cfg[] = {
...
    for (i = 0; i < config->num_nvram; i++) {
        nvmem_cfg[i].priv = pcf85363;
        devm_rtc_nvmem_register(pcf85363->rtc, &nvmem_cfg[i]);
    }
...
}

If multiple PCF85363 instances are probed on the system, wouldn't the later
probe overwrite the priv pointer of the earlier instances in the shared
static array? This would cause subsequent NVRAM reads/writes for the first
RTC device to incorrectly target the last probed device's regmap.

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