[PATCH v4 0/4] rtc: Add NXP PCF8525 RTC driver with watchdog and hwmon

[PATCH v4 0/4] rtc: Add NXP PCF8525 RTC driver with watchdog and hwmon

[Shiv Prakash Gupta]

The PCF8525 is an NXP I2C real-time clock using an external
32.768-kHz crystal. It provides alarms, external timestamp capture,
recording of the last calendar write, backup battery switch-over,
a hardware watchdog timer, and an on-chip temperature sensor.

This series introduces DT bindings, the core RTC driver, watchdog
support, and optional hardware-monitoring (temperature readout) via
the hwmon subsystem.

Changes in v4:
- Rework timestamp handling to serialize sysfs clear and IRQ
  processing, read Timestamp1 before clearing TSF, and restore TSM
  after the CTS command.
- Correct Timestamp2 handling to report the last calendar write
  instead of treating it as a second external timestamp capture.
- Rework the INTA threaded IRQ handler to drain pending AF/MSF/TSF
  events and re-read status registers, avoiding missed events on an
  edge-triggered IRQ.
- Preserve all CTRL2 Write-0-to-Clear status flags, including WDTF,
  across register read-modify-write operations.
- Rework watchdog support to use nINTB as the watchdog output without
  a Linux watchdog IRQ or pretimeout notification; remove unsupported
  pretimeout handling and write-only watchdog-value read.
- Establish a known watchdog state at probe, clear stale WDTF before
  enabling the watchdog output, and report the hardware-achieved
  timeout.
- Fix RTC feature and time-setting handling, including
  update-interrupt capability reporting and writing the
  hundredths-of-a-second register when setting the calendar.
- Improve interrupt/wakeup and device-managed cleanup ordering so
  hardware interrupt sources are safely masked on probe failure and
  driver removal.
- Rework hwmon registration and power-management handling so the
  temperature readout is enabled before exposure to userspace and
  cleanly disabled on failure, suspend, and removal.

Changes in v3:
- patch 2: protect the Write-0-to-Clear (W0C) status flags in the
  CTRL2/CTRL3 read-modify-write cycles so concurrently asserted
  interrupt events are not silently cleared
- patch 2: replace the timestamp spinlock with a mutex and re-read
  CTRL4 under it, closing the TOCTOU race between the threaded IRQ
  handler and the sysfs timestamp store
- patch 3: configure the nINTB/CLKOUT pin (CLKOE=0) before requesting
  the watchdog IRQ to avoid a spurious interrupt storm
- patch 3: add suspend/resume PM ops to stop and re-ping the watchdog
  across system sleep
- patch 3: add WDIOF_PRETIMEOUT so the watchdog core attaches a
  pretimeout governor
- patch 3: re-mask INTA/INTB interrupts on driver unbind via devm
  cleanup actions
- patch 4: enable TEMP_RD_EN before registering the hwmon device and
  add a devm cleanup to disable it on unbind

Changes in v2:
- Fix include: use <linux/pm_wakeirq.h> instead of <pm_wakeup.h>
- Add spinlock_t ts_lock to protect ts[]/ts_valid[] from IRQ/sysfs race
- Fix W0C ordering: clear AF/TSF flags before acting on alarm/timestamp
  events
- Use dev_pm_set_wake_irq() for suspend wakeup wiring
- Clear stale WDTF before unmasking watchdog interrupt on INTB to prevent
  boot loop when battery-backed flag persists across resets
- Add watchdog_notify_pretimeout() call in INTB IRQ handler
- Set WDOG_HW_RUNNING if bootloader left watchdog running at probe time
- Register hwmon device first, then enable PCF8525_CTRL5_TEMP_RD_EN only
  after successful registration

Shiv Prakash Gupta (4):
  dt-bindings: rtc: Add NXP PCF8525 RTC
  rtc: pcf8525: Add NXP PCF8525 RTC driver
  rtc: pcf8525: Add watchdog support
  rtc: pcf8525: Add temperature sensor support via hwmon

 .../devicetree/bindings/rtc/nxp,pcf8525.yaml  |   86 +
 MAINTAINERS                                   |    7 +
 drivers/rtc/Kconfig                           |   25 +
 drivers/rtc/Makefile                          |    1 +
 drivers/rtc/rtc-pcf8525.c                     | 1608 +++++++++++++++++
 5 files changed, 1727 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8525.yaml
 create mode 100644 drivers/rtc/rtc-pcf8525.c

-- 
2.34.1

[PATCH v4 1/4] dt-bindings: rtc: Add NXP PCF8525 RTC

[Shiv Prakash Gupta]

Add a Devicetree binding for the NXP PCF8525 I2C RTC. Describe the
optional INTA interrupt used for RTC alarm and timestamp events, and
the INTB interrupt used by the watchdog.

Add properties to select the external crystal load capacitance and
temperature-compensation crystal model. Also allow the common
start-year, wakeup-source and reset-source properties, with
reset-source enabling watchdog registration

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Signed-off-by: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes in v2:
- No changes to DT binding
---
 .../devicetree/bindings/rtc/nxp,pcf8525.yaml  | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nxp,pcf8525.yaml

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8525.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf8525.yaml
new file mode 100644
index 000000000000..bee71ec3da80
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8525.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/nxp,pcf8525.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP PCF8525 Real Time Clock
+
+description:
+  The PCF8525 is an I2C real-time clock with alarm, timestamp, watchdog,
+  backup battery switch-over, and temperature compensation for an external
+  32.768 kHz crystal.
+
+allOf:
+  - $ref: rtc.yaml#
+
+maintainers:
+  - Lakshay Piplani <lakshay.piplani@nxp.com>
+  - Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
+
+properties:
+  compatible:
+    enum:
+      - nxp,pcf8525
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+    description:
+      INTA, followed by INTB when both interrupt outputs are connected.
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - const: inta
+      - const: intb
+
+  quartz-load-femtofarads:
+    description:
+      Effective load capacitance of the external 32.768 kHz crystal.
+    enum: [6000, 7000]
+    default: 6000
+
+  nxp,xtal-type:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Crystal model used by the temperature compensation engine.
+      Value 1 selects -0.035 ppm/degC^2 and value 2 selects
+      -0.04 ppm/degC^2.
+    enum: [1, 2]
+    default: 1
+
+  start-year: true
+
+  wakeup-source: true
+
+  reset-source: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc@51 {
+            compatible = "nxp,pcf8525";
+            reg = <0x51>;
+            interrupt-parent = <&gpio2>;
+            interrupts = <2 IRQ_TYPE_EDGE_FALLING>,
+                          <3 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-names = "inta", "intb";
+            quartz-load-femtofarads = <7000>;
+            nxp,xtal-type = <2>;
+            reset-source;
+        };
+    };
-- 
2.34.1

[PATCH v4 2/4] rtc: pcf8525: Add NXP PCF8525 RTC driver

[Shiv Prakash Gupta]

Add a driver for the NXP PCF8525 I2C real-time clock.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Signed-off-by: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
---
 MAINTAINERS               |    7 +
 drivers/rtc/Kconfig       |   12 +
 drivers/rtc/Makefile      |    1 +
 drivers/rtc/rtc-pcf8525.c | 1112 +++++++++++++++++++++++++++++++++++++
 4 files changed, 1132 insertions(+)
 create mode 100644 drivers/rtc/rtc-pcf8525.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 040df7bfb24f..0394874e5099 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19604,6 +19604,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/regulator/nxp,pf8x00-regulator.yaml
 F:	drivers/regulator/pf8x00-regulator.c
 
+NXP PCF8525 RTC DRIVER
+M:	Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
+L:	linux-rtc@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/rtc/nxp,pcf8525.yaml
+F:	drivers/rtc/rtc-pcf8525.c
+
 NXP PTN5150A CC LOGIC AND EXTCON DRIVER
 M:	Krzysztof Kozlowski <krzk@kernel.org>
 L:	linux-kernel@vger.kernel.org
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 01def8231873..b6c43b173445 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -513,6 +513,18 @@ config RTC_DRV_PCF8523
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-pcf8523.
 
+config RTC_DRV_PCF8525
+	tristate "NXP PCF8525"
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for the NXP PCF8525 RTC chip.
+	  The PCF8525 is an I2C real-time clock with alarm, external timestamp
+	  capture and recording of the last calendar write, crystal aging offset
+	  correction, and backup power switchover.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-pcf8525.
+
 config RTC_DRV_PCF85363
 	tristate "NXP PCF85363"
 	select REGMAP_I2C
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 0347645b021f..2b90d28235c6 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -132,6 +132,7 @@ obj-$(CONFIG_RTC_DRV_PCF2123)	+= rtc-pcf2123.o
 obj-$(CONFIG_RTC_DRV_PCF2127)	+= rtc-pcf2127.o
 obj-$(CONFIG_RTC_DRV_PCF85063)	+= rtc-pcf85063.o
 obj-$(CONFIG_RTC_DRV_PCF8523)	+= rtc-pcf8523.o
+obj-$(CONFIG_RTC_DRV_PCF8525)	+= rtc-pcf8525.o
 obj-$(CONFIG_RTC_DRV_PCF85363)	+= rtc-pcf85363.o
 obj-$(CONFIG_RTC_DRV_PCF8563)	+= rtc-pcf8563.o
 obj-$(CONFIG_RTC_DRV_PCF8583)	+= rtc-pcf8583.o
diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
new file mode 100644
index 000000000000..3c5f15f95a9e
--- /dev/null
+++ b/drivers/rtc/rtc-pcf8525.c
@@ -0,0 +1,1112 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * I2C driver for NXP PCF8525 RTC.
+ *
+ * Supports RTC, alarm, timestamp capture, crystal aging correction,
+ * watchdog and optional hwmon temperature readout.
+ */
+
+#include <linux/bcd.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/mutex.h>
+#include <linux/pm_wakeirq.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+#include <linux/slab.h>
+#include <linux/bitfield.h>
+#include <linux/uaccess.h>
+#include <linux/kernel.h>
+#include <linux/pm.h>
+
+/* Registers */
+#define PCF8525_REG_CTRL1		0x00
+#define PCF8525_REG_CTRL2		0x01
+#define PCF8525_REG_CTRL3		0x02
+#define PCF8525_REG_CTRL4		0x03
+#define PCF8525_REG_CTRL5		0x04
+#define PCF8525_REG_RESET		0x05
+
+#define PCF8525_REG_100THS		0x06
+#define PCF8525_REG_SECONDS		0x07
+#define PCF8525_REG_MINUTES		0x08
+#define PCF8525_REG_HOURS		0x09
+#define PCF8525_REG_DAYS		0x0A
+#define PCF8525_REG_WEEKDAYS		0x0B
+#define PCF8525_REG_MONTHS		0x0C
+#define PCF8525_REG_YEARS		0x0D
+
+#define PCF8525_REG_ALM_SEC		0x0E
+#define PCF8525_REG_ALM_MIN		0x0F
+#define PCF8525_REG_ALM_HOUR		0x10
+#define PCF8525_REG_ALM_DAY		0x11
+#define PCF8525_REG_ALM_WDAY		0x12
+#define PCF8525_REG_ALM_MONTH		0x13
+#define PCF8525_REG_ALM_YEAR		0x14
+
+#define PCF8525_REG_CLKOUT		0x15
+
+#define PCF8525_REG_TS_CTL1		0x16
+#define PCF8525_REG_TS1_SEC		0x17
+#define PCF8525_REG_TS1_MIN		0x18
+#define PCF8525_REG_TS1_HOUR		0x19
+#define PCF8525_REG_TS1_DAY		0x1A
+#define PCF8525_REG_TS1_MONTH		0x1B
+#define PCF8525_REG_TS1_YEAR		0x1C
+
+#define PCF8525_REG_TS2_SUBSEC		0x1D
+#define PCF8525_REG_TS2_SEC		0x1E
+#define PCF8525_REG_TS2_MIN		0x1F
+#define PCF8525_REG_TS2_HOUR		0x20
+#define PCF8525_REG_TS2_DAY		0x21
+#define PCF8525_REG_TS2_MONTH		0x22
+#define PCF8525_REG_TS2_YEAR		0x23
+
+#define PCF8525_REG_INTA_MASK1		0x28
+#define PCF8525_REG_INTA_MASK2		0x29
+#define PCF8525_REG_INTB_MASK1		0x2A
+#define PCF8525_REG_INTB_MASK2		0x2B
+
+#define PCF8525_REG_TEMP		0x2E
+
+/* CTRL1 */
+#define PCF8525_CTRL1_STOP		BIT(5)
+#define PCF8525_CTRL1_12_24		BIT(2)
+#define PCF8525_CTRL1_MI		BIT(1)
+#define PCF8525_CTRL1_SI		BIT(0)
+/* CTRL2 */
+#define PCF8525_CTRL2_MSF		BIT(7)
+#define PCF8525_CTRL2_TI_TP		BIT(6)
+#define PCF8525_CTRL2_WDTF		BIT(5)
+#define PCF8525_CTRL2_AF		BIT(4)
+#define PCF8525_CTRL2_OSFE_MASK		GENMASK(3, 2)
+#define PCF8525_CTRL2_AIE		BIT(1)
+#define PCF8525_CTRL2_SMBTO		BIT(0)
+/*
+ * All W0C status bits in CTRL2 (writing 1 = no effect, writing 0 = clear).
+ */
+#define PCF8525_CTRL2_W0C_FLAGS		(PCF8525_CTRL2_MSF | \
+					 PCF8525_CTRL2_WDTF | \
+					 PCF8525_CTRL2_AF)
+/* CTRL3 */
+#define PCF8525_CTRL3_PWRMNG_MASK	GENMASK(6, 5)
+#define PCF8525_CTRL3_BF		BIT(3)
+#define PCF8525_CTRL3_OSIE		BIT(2)
+#define PCF8525_CTRL3_BIE		BIT(1)
+/* CTRL4/5 */
+#define PCF8525_CTRL4_TSF		BIT(7)
+#define PCF8525_CTRL5_TSIE		BIT(7)
+#define PCF8525_CTRL5_TEMP_RD_EN	BIT(2)
+#define PCF8525_CTRL5_CL		BIT(1)
+#define PCF8525_CTRL5_XTL_TYP		BIT(0)
+
+/* CLKOUT_ctl */
+#define PCF8525_CLKOUT_CLKOE		BIT(3)
+
+/* Timestamp control */
+#define PCF8525_TS_CTL1_TSM             BIT(7)
+#define PCF8525_TS_CTL1_TSOFF           BIT(6)
+
+/* Seconds/Minutes flags */
+#define PCF8525_SC_OSF			BIT(7)
+#define PCF8525_MN_VLF			BIT(7)
+
+/* Alarm AE bits */
+#define PCF8525_ALM_AE			BIT(7)
+
+/* RESET commands */
+#define PCF8525_RESET_CPR_CMD		0xA4 /* clear prescaler */
+#define PCF8525_RESET_CTS_CMD		0x25 /* clear timestamp */
+#define PCF8525_RESET_CPR_CTS_CMD	0xA5 /* clear prescaler + timestamp */
+#define PCF8525_RESET_SR_CMD		0x2C /* software reset */
+
+/* Interrupt masks (mask bit = 1 means masked/disabled) - INTA_MASK1 bits */
+#define PCF8525_MASK1_WD_CD		BIT(0)
+#define PCF8525_MASK1_BIE		BIT(1)
+#define PCF8525_MASK1_AIE		BIT(2)
+#define PCF8525_MASK1_OSIE		BIT(3)
+#define PCF8525_MASK1_SI		BIT(4)
+#define PCF8525_MASK1_MI		BIT(5)
+/* INTA_MASK2 - timestamp mask bit3 */
+#define PCF8525_MASK2_TSIE		BIT(3)
+
+#define PCF8525_ALM_AE_MONTH		BIT(7)
+#define PCF8525_ALM_AE_YEAR		BIT(6)
+
+#define PCF8525_REG_AGING_OFFSET_HI	0x26
+#define PCF8525_REG_AGING_OFFSET_LO	0x27
+/*
+ * One aging-offset register step is 0.0298 ppm, or 29.8 ppb.
+ * The RTC core offset interface uses ppb.
+ */
+#define PCF8525_AGING_OFFSET_STEP_NUM	298L
+#define PCF8525_AGING_OFFSET_STEP_DEN	10L
+
+#define PCF8525_AGING_OFFSET_MIN	(-32768L)
+#define PCF8525_AGING_OFFSET_MAX	32767L
+
+struct pcf8525 {
+	struct rtc_device *rtc;
+	struct regmap *regmap;
+	struct mutex ts_mutex;	/* protects ts[] and ts_valid[] in IRQ mode */
+	bool irq_enabled;
+	time64_t ts[2];
+	bool ts_valid[2];
+	int irq_inta;
+};
+
+static const struct regmap_config pcf8525_regmap_cfg = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = PCF8525_REG_TEMP,
+};
+
+/*
+ * Configure only the crystal fields explicitly provided by firmware.
+ * If a property is absent, preserve the current hardware setting.
+ *
+ * quartz-load-femtofarads:
+ *   6000 -> Control_5.CL = 0
+ *   7000 -> Control_5.CL = 1
+ *
+ * nxp,xtal-type:
+ *   1 -> Control_5.XTL_TYP = 0 (-0.035 ppm/degree C^2 model)
+ *   2 -> Control_5.XTL_TYP = 1 (-0.04 ppm/degree C^2 model)
+ */
+static int pcf8525_configure_crystal(struct device *dev,
+				     struct pcf8525 *pcf8525)
+{
+	unsigned int value = 0;
+	unsigned int mask = 0;
+	u32 xtal_type;
+	u32 load;
+	int ret;
+
+	ret = device_property_read_u32(dev,
+				       "quartz-load-femtofarads",
+				       &load);
+	if (!ret) {
+		switch (load) {
+		case 6000:
+			mask |= PCF8525_CTRL5_CL;
+			break;
+
+		case 7000:
+			mask |= PCF8525_CTRL5_CL;
+			value |= PCF8525_CTRL5_CL;
+			break;
+
+		default:
+			dev_warn(dev,
+				 "unsupported quartz-load-femtofarads=%u; "
+				 "preserving CL bit\n",
+				 load);
+			break;
+		}
+	}
+
+	ret = device_property_read_u32(dev,
+				       "nxp,xtal-type",
+				       &xtal_type);
+	if (!ret) {
+		switch (xtal_type) {
+		case 1:
+			mask |= PCF8525_CTRL5_XTL_TYP;
+			break;
+
+		case 2:
+			mask |= PCF8525_CTRL5_XTL_TYP;
+			value |= PCF8525_CTRL5_XTL_TYP;
+			break;
+
+		default:
+			dev_warn(dev,
+				 "unsupported nxp,xtal-type=%u; "
+				 "preserving XTL_TYP bit\n",
+				 xtal_type);
+			break;
+		}
+	}
+
+	if (!mask)
+		return 0;
+
+	return regmap_update_bits(pcf8525->regmap,
+				  PCF8525_REG_CTRL5,
+				  mask, value);
+}
+
+static int pcf8525_read_aging_offset(struct pcf8525 *pcf8525, s16 *offset)
+{
+	u8 buf[2];
+	int ret;
+
+	ret = regmap_bulk_read(pcf8525->regmap,
+			       PCF8525_REG_AGING_OFFSET_HI,
+			       buf, sizeof(buf));
+	if (ret)
+		return ret;
+
+	*offset = (s16)((buf[0] << 8) | buf[1]);
+
+	return 0;
+}
+
+static int pcf8525_write_aging_offset(struct pcf8525 *pcf8525, s16 offset)
+{
+	u8 buf[2];
+
+	buf[0] = (offset >> 8) & 0xff;
+	buf[1] = offset & 0xff;
+
+	/*
+	 * Write high byte first and low byte last. The new aging correction
+	 * starts after AgingOffset_Low is written.
+	 */
+	return regmap_bulk_write(pcf8525->regmap,
+				 PCF8525_REG_AGING_OFFSET_HI,
+				 buf, sizeof(buf));
+}
+
+static int pcf8525_rtc_read_offset(struct device *dev, long *offset)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	s16 raw;
+	int ret;
+
+	ret = pcf8525_read_aging_offset(pcf8525, &raw);
+	if (ret)
+		return ret;
+
+	/*
+	 * Convert the signed 16-bit hardware value to ppb:
+	 *
+	 * one raw step = 0.0298 ppm = 29.8 ppb
+	 */
+	*offset = DIV_ROUND_CLOSEST((long)raw *
+				    PCF8525_AGING_OFFSET_STEP_NUM,
+				    PCF8525_AGING_OFFSET_STEP_DEN);
+
+	return 0;
+}
+
+static int pcf8525_rtc_set_offset(struct device *dev, long offset)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	long min_offset;
+	long max_offset;
+	long raw;
+
+	min_offset =
+		DIV_ROUND_CLOSEST(PCF8525_AGING_OFFSET_MIN *
+				  PCF8525_AGING_OFFSET_STEP_NUM,
+				  PCF8525_AGING_OFFSET_STEP_DEN);
+
+	max_offset =
+		DIV_ROUND_CLOSEST(PCF8525_AGING_OFFSET_MAX *
+				  PCF8525_AGING_OFFSET_STEP_NUM,
+				  PCF8525_AGING_OFFSET_STEP_DEN);
+
+	if (offset < min_offset || offset > max_offset)
+		return -ERANGE;
+
+	/*
+	 * Convert the requested ppb value to the closest signed
+	 * 16-bit hardware value.
+	 */
+	raw = DIV_ROUND_CLOSEST(offset *
+				PCF8525_AGING_OFFSET_STEP_DEN,
+				PCF8525_AGING_OFFSET_STEP_NUM);
+
+	if (raw < PCF8525_AGING_OFFSET_MIN ||
+	    raw > PCF8525_AGING_OFFSET_MAX)
+		return -ERANGE;
+
+	return pcf8525_write_aging_offset(pcf8525, (s16)raw);
+}
+
+static int pcf8525_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	u8 buf[7];
+	int ret;
+
+	/* Read seconds..years (0x07..0x0D) */
+	ret = regmap_bulk_read(pcf8525->regmap, PCF8525_REG_SECONDS, buf, sizeof(buf));
+	if (ret)
+		return ret;
+
+	/* OSF: oscillator stop => time not reliable */
+	if (buf[0] & PCF8525_SC_OSF)
+		return -EINVAL;
+
+	/*
+	 * VLF is on minutes register bit7;
+	 * Keep policy conservative: warn but still allow read.
+	 */
+	if (buf[1] & PCF8525_MN_VLF)
+		dev_warn(dev, "VLF set: clock integrity not guaranteed\n");
+
+	tm->tm_sec =  bcd2bin(buf[0] & 0x7f);
+	tm->tm_min =  bcd2bin(buf[1] & 0x7f);
+	tm->tm_hour = bcd2bin(buf[2] & 0x3f);
+	tm->tm_mday = bcd2bin(buf[3] & 0x3f);
+	tm->tm_wday = buf[4] & 0x07;
+	tm->tm_mon =  bcd2bin(buf[5] & 0x1f) - 1;
+	tm->tm_year = bcd2bin(buf[6]) + 100; /* 20xx */
+
+	return rtc_valid_tm(tm);
+}
+
+static int pcf8525_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	int ret, ret2;
+	u8 buf[8];
+
+	ret = rtc_valid_tm(tm);
+	if (ret)
+		return ret;
+
+	/*
+	 * Use STOP + clear prescaler (CPR) like RESET register describes.
+	 */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL1,
+				 PCF8525_CTRL1_STOP, PCF8525_CTRL1_STOP);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_RESET, PCF8525_RESET_CPR_CMD);
+	if (ret)
+		goto out_start;
+
+	buf[0] = 0;                          /* 100th seconds, always zero on set */
+	buf[1] = bin2bcd(tm->tm_sec);
+	buf[2] = bin2bcd(tm->tm_min);
+	buf[3] = bin2bcd(tm->tm_hour);
+	buf[4] = bin2bcd(tm->tm_mday);
+	buf[5] = tm->tm_wday & 0x07;
+	buf[6] = bin2bcd(tm->tm_mon + 1);
+	buf[7] = bin2bcd(tm->tm_year - 100);
+
+	ret = regmap_bulk_write(pcf8525->regmap, PCF8525_REG_100THS, buf, sizeof(buf));
+
+out_start:
+	/* Clear STOP regardless of bulk write outcome */
+	ret2 = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL1, PCF8525_CTRL1_STOP, 0);
+	if (ret2)
+		return ret2;
+	return ret;
+}
+
+static int pcf8525_rtc_ioctl(struct device *dev,
+			     unsigned int cmd, unsigned long arg)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	unsigned int flags = 0;
+	unsigned int val;
+	int ret;
+
+	switch (cmd) {
+	case RTC_VL_READ:
+		ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL3, &val);
+		if (ret)
+			return ret;
+
+		if (val & PCF8525_CTRL3_BF)
+			flags |= RTC_VL_BACKUP_SWITCH;
+
+		return put_user(flags, (unsigned int __user *)arg);
+
+	case RTC_VL_CLR:
+		/* Clear BF by writing 0 (AND semantics respected) */
+		return regmap_update_bits(pcf8525->regmap,
+					  PCF8525_REG_CTRL3,
+					  PCF8525_CTRL3_BF,
+					  0);
+
+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+
+static int pcf8525_param_get(struct device *dev, struct rtc_param *param)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	unsigned int val;
+	unsigned int mode;
+	int ret;
+
+	switch (param->param) {
+	case RTC_PARAM_BACKUP_SWITCH_MODE:
+		ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL3, &val);
+		if (ret)
+			return ret;
+
+		mode = FIELD_GET(PCF8525_CTRL3_PWRMNG_MASK, val);
+
+		switch (mode) {
+		case 0:
+			param->uvalue = RTC_BSM_LEVEL;
+			break;
+		case 1:
+			param->uvalue = RTC_BSM_DIRECT;
+			break;
+		default:
+			param->uvalue = RTC_BSM_DISABLED;
+			break;
+		}
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pcf8525_param_set(struct device *dev, struct rtc_param *param)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	unsigned int mode;
+
+	switch (param->param) {
+	case RTC_PARAM_BACKUP_SWITCH_MODE:
+		switch (param->uvalue) {
+		case RTC_BSM_LEVEL:
+			mode = 0;
+			break;
+		case RTC_BSM_DIRECT:
+			mode = 1;
+			break;
+		case RTC_BSM_DISABLED:
+			mode = 2;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+	/*
+	 * CTRL3 BF is a W0C bit: include it in the mask with value=1 so the
+	 * RMW cycle does not accidentally clear it if it asserted between the
+	 * regmap read and write.
+	 */
+	return regmap_update_bits(pcf8525->regmap,
+				  PCF8525_REG_CTRL3,
+				  PCF8525_CTRL3_PWRMNG_MASK | PCF8525_CTRL3_BF,
+				  FIELD_PREP(PCF8525_CTRL3_PWRMNG_MASK, mode) |
+				  PCF8525_CTRL3_BF);
+	default:
+		return -EINVAL;
+	}
+}
+
+/* Alarm uses RTC wkalrm: map to sec/min/hour/day and disable others */
+static int pcf8525_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	unsigned int ctrl2;
+	u8 buf[7];
+	int ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL2, &ctrl2);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(pcf8525->regmap, PCF8525_REG_ALM_SEC, buf, sizeof(buf));
+	if (ret)
+		return ret;
+
+	alrm->enabled = !!(ctrl2 & PCF8525_CTRL2_AIE);
+	alrm->pending = !!(ctrl2 & PCF8525_CTRL2_AF);
+
+	alrm->time.tm_sec = bcd2bin(buf[0] & 0x7f);
+	alrm->time.tm_min = bcd2bin(buf[1] & 0x7f);
+	alrm->time.tm_hour = bcd2bin(buf[2] & 0x3f);
+	alrm->time.tm_mday = bcd2bin(buf[3] & 0x3f);
+
+	return 0;
+}
+
+static int pcf8525_alarm_irq_enable(struct device *dev, unsigned int enable)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	int ret;
+
+	/*
+	 * Include all CTRL2 W0C flags in the mask and keep them set to 1 in the
+	 * value so the RMW does not accidentally clear a concurrently asserted
+	 * interrupt flag (AF or MSF).
+	 */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
+				 PCF8525_CTRL2_AIE | PCF8525_CTRL2_W0C_FLAGS,
+				 (enable ? PCF8525_CTRL2_AIE : 0) |
+				 PCF8525_CTRL2_W0C_FLAGS);
+	if (ret)
+		dev_err(dev, "alarm_irq_enable: failed ret=%d\n", ret);
+
+	return ret;
+}
+
+static int pcf8525_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	u8 buf[7];
+	int ret;
+
+	/*
+	 * Clear AF (W0C). Use W0C_FLAGS as the mask and preserve MSF and WDTF
+	 * by writing 1 to them: writing 0 to a W0C bit that has asserted
+	 * between the read and write would silently drop the event.
+	 */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
+				 PCF8525_CTRL2_W0C_FLAGS,
+				 PCF8525_CTRL2_MSF | PCF8525_CTRL2_WDTF);
+	if (ret)
+		return ret;
+
+	buf[0] = bin2bcd(alrm->time.tm_sec) & 0x7f;
+	buf[1] = bin2bcd(alrm->time.tm_min) & 0x7f;
+	buf[2] = bin2bcd(alrm->time.tm_hour) & 0x3f;
+	buf[3] = bin2bcd(alrm->time.tm_mday) & 0x3f;
+
+	/* Disable match on weekday/month/year by setting AE bits */
+	buf[4] = PCF8525_ALM_AE; /* weekday alarm ignored */
+	buf[5] = PCF8525_ALM_AE_MONTH | PCF8525_ALM_AE_YEAR;  /* disable month + year alarm */
+	buf[6] = 0x00; /* year alarm value (ignored if month/year disabled) */
+
+	ret = regmap_bulk_write(pcf8525->regmap, PCF8525_REG_ALM_SEC, buf, sizeof(buf));
+	if (ret)
+		return ret;
+
+	return pcf8525_alarm_irq_enable(dev, alrm->enabled);
+}
+
+/* ---- Timestamp handling (sysfs) ---- */
+static int pcf8525_ts_read(struct device *dev, int id, time64_t *ts_out)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	struct rtc_time tm;
+	u8 buf[6];
+	int ret;
+	u8 base;
+
+	/*
+	 * TS0 maps to TS1 (0x17..0x1C)
+	 * TS1 maps to TS2 (0x1E..0x23) (skip subsec at 0x1D)
+	 */
+	if (id == 0)
+		base = PCF8525_REG_TS1_SEC;
+	else if (id == 1)
+		base = PCF8525_REG_TS2_SEC;
+	else
+		return -EINVAL;
+
+	ret = regmap_bulk_read(pcf8525->regmap, base, buf, sizeof(buf));
+	if (ret)
+		return ret;
+
+	tm.tm_sec = bcd2bin(buf[0] & 0x7f);
+	tm.tm_min = bcd2bin(buf[1] & 0x7f);
+	tm.tm_hour = bcd2bin(buf[2] & 0x3f);
+	tm.tm_mday = bcd2bin(buf[3] & 0x3f);
+	tm.tm_mon = bcd2bin(buf[4] & 0x1f) - 1;
+	tm.tm_year = bcd2bin(buf[5]) + 100;
+
+	ret = rtc_valid_tm(&tm);
+	if (ret)
+		return ret;
+
+	*ts_out = rtc_tm_to_time64(&tm);
+	return 0;
+}
+
+static ssize_t timestamp_show_common(struct device *dev, char *buf, int id)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev->parent);
+	unsigned int ctrl4;
+	time64_t ts;
+	int ret;
+
+	if (id < 0 || id > 1)
+		return -EINVAL;
+
+	/*
+	 * Timestamp2 (id=1) records the last calendar/date-time write.
+	 * It is updated automatically by hardware on every set_time call
+	 * and is not related to the external TS capture event (TSF).
+	 * Always read it directly from the hardware registers.
+	 */
+	if (id == 1) {
+		ret = pcf8525_ts_read(dev->parent, 1, &ts);
+		if (ret)
+			return ret;
+		return sysfs_emit(buf, "%llu\n", (unsigned long long)ts);
+	}
+
+	/* id == 0: Timestamp1 = external TS pin capture */
+	if (pcf8525->irq_enabled) {
+		bool valid;
+
+		mutex_lock(&pcf8525->ts_mutex);
+		valid = pcf8525->ts_valid[0];
+		if (valid)
+			ts = pcf8525->ts[0];
+		mutex_unlock(&pcf8525->ts_mutex);
+
+		if (!valid)
+			return 0;
+		return sysfs_emit(buf, "%llu\n", (unsigned long long)ts);
+	}
+
+	/* polling mode: only report if TSF is currently set */
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL4, &ctrl4);
+	if (ret)
+		return ret;
+
+	if (!(ctrl4 & PCF8525_CTRL4_TSF))
+		return 0;
+
+	ret = pcf8525_ts_read(dev->parent, 0, &ts);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%llu\n", (unsigned long long)ts);
+}
+
+static ssize_t timestamp_store_common(struct device *dev, const char *buf,
+				      size_t count, int id)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev->parent);
+	int ret;
+
+	if (id < 0 || id > 1)
+		return -EINVAL;
+
+	if (pcf8525->irq_enabled) {
+		/*
+		 * Serialize CTS with the IRQ handler. CTS clears the timestamp
+		 * registers and Timestp_ctl1, so invalidate the cache only after
+		 * CTS succeeds, restore TSM, then clear TSF.
+		 */
+		mutex_lock(&pcf8525->ts_mutex);
+
+		ret = regmap_write(pcf8525->regmap, PCF8525_REG_RESET,
+				   PCF8525_RESET_CTS_CMD);
+		if (ret) {
+			mutex_unlock(&pcf8525->ts_mutex);
+			return ret;
+		}
+
+		/* CTS succeeded: hardware registers are now zeroed. */
+		pcf8525->ts_valid[0] = false;
+		pcf8525->ts_valid[1] = false;
+
+		/* Restore TSM=1, TSOFF=0 (CTS cleared 0x16). */
+		ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_TS_CTL1,
+					 PCF8525_TS_CTL1_TSM | PCF8525_TS_CTL1_TSOFF,
+					 PCF8525_TS_CTL1_TSM);
+		if (!ret)
+			ret = regmap_update_bits(pcf8525->regmap,
+						 PCF8525_REG_CTRL4,
+						 PCF8525_CTRL4_TSF, 0);
+		mutex_unlock(&pcf8525->ts_mutex);
+		return ret ? ret : (ssize_t)count;
+	}
+
+	/*
+	 * Polling path: CTS first, then restore TSM=1/TSOFF=0, then clear TSF.
+	 */
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_RESET, PCF8525_RESET_CTS_CMD);
+	if (ret)
+		return ret;
+
+	/* Restore TSM=1, TSOFF=0 (CTS cleared 0x16). */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_TS_CTL1,
+				 PCF8525_TS_CTL1_TSM | PCF8525_TS_CTL1_TSOFF,
+				 PCF8525_TS_CTL1_TSM);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL4,
+				 PCF8525_CTRL4_TSF, 0);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t timestamp0_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return timestamp_show_common(dev, buf, 0);
+}
+
+static ssize_t timestamp1_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return timestamp_show_common(dev, buf, 1);
+}
+
+static ssize_t timestamp0_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	return timestamp_store_common(dev, buf, count, 0);
+}
+
+static ssize_t timestamp1_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	return timestamp_store_common(dev, buf, count, 1);
+}
+
+static DEVICE_ATTR_RW(timestamp0);
+static DEVICE_ATTR_RW(timestamp1);
+
+static struct attribute *pcf8525_attrs[] = {
+	&dev_attr_timestamp0.attr,
+	&dev_attr_timestamp1.attr,
+	NULL
+};
+
+static const struct attribute_group pcf8525_attr_group = {
+	.attrs = pcf8525_attrs,
+};
+
+static irqreturn_t pcf8525_irq(int irq, void *data)
+{
+	struct device *dev = data;
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	irqreturn_t handled = IRQ_NONE;
+	unsigned int ctrl2, ctrl4;
+	int ret;
+
+	/*
+	 * Drain all pending INTA sources. Re-read the status registers
+	 * because another source may assert while INTA is already low.
+	 */
+	for (;;) {
+		ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL2, &ctrl2);
+		if (ret)
+			return handled;
+
+		ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL4, &ctrl4);
+		if (ret)
+			return handled;
+
+		if (!(ctrl2 & (PCF8525_CTRL2_AF | PCF8525_CTRL2_MSF)) &&
+		    !(ctrl4 & PCF8525_CTRL4_TSF))
+			break;
+
+		/* Timestamp */
+		if (ctrl4 & PCF8525_CTRL4_TSF) {
+			time64_t ts0;
+			unsigned int ctrl4_now;
+			bool ok0;
+
+			/*
+			 * Serialize against CTS and re-check TSF under the mutex before
+			 * reading the timestamp registers.
+			 */
+			mutex_lock(&pcf8525->ts_mutex);
+
+			ret = regmap_read(pcf8525->regmap,
+					  PCF8525_REG_CTRL4, &ctrl4_now);
+			if (ret) {
+				mutex_unlock(&pcf8525->ts_mutex);
+				return handled;
+			}
+
+			if (!(ctrl4_now & PCF8525_CTRL4_TSF)) {
+				/*
+				 * timestamp_store_common() already issued
+				 * CTS and cleared TSF; registers are zeroed.
+				 * Continue to check AF/MSF below.
+				 */
+				mutex_unlock(&pcf8525->ts_mutex);
+			} else {
+				/*
+				 * TSM=1 retains the first event while TSF is set, so read TS1
+				 * before clearing TSF.
+				 */
+				ok0 = !pcf8525_ts_read(dev, 0, &ts0);
+				if (!ok0) {
+					/*
+					 * TS1 read failed; leave TSF set
+					 * so the event is not silently
+					 * dropped and hardware can
+					 * re-trigger.
+					 */
+					mutex_unlock(&pcf8525->ts_mutex);
+					return handled;
+				}
+
+				/* Acknowledge/re-arm the timestamp event. */
+				ret = regmap_update_bits(pcf8525->regmap,
+							 PCF8525_REG_CTRL4,
+							 PCF8525_CTRL4_TSF, 0);
+				if (ret) {
+					mutex_unlock(&pcf8525->ts_mutex);
+					return handled;
+				}
+
+				if (!pcf8525->ts_valid[0]) {
+					pcf8525->ts[0] = ts0;
+					pcf8525->ts_valid[0] = true;
+				}
+				mutex_unlock(&pcf8525->ts_mutex);
+				handled = IRQ_HANDLED;
+			}
+		}
+
+		/* Alarm */
+		if (ctrl2 & PCF8525_CTRL2_AF) {
+			/* Clear AF while preserving the other CTRL2 W0C flags. */
+			ret = regmap_update_bits(pcf8525->regmap,
+						 PCF8525_REG_CTRL2,
+						 PCF8525_CTRL2_W0C_FLAGS,
+						 PCF8525_CTRL2_MSF |
+						 PCF8525_CTRL2_WDTF);
+			if (ret)
+				return handled;
+			rtc_update_irq(pcf8525->rtc, 1, RTC_IRQF | RTC_AF);
+			handled = IRQ_HANDLED;
+		}
+
+		if (ctrl2 & PCF8525_CTRL2_MSF) {
+			/* Clear MSF while preserving AF and WDTF. */
+			ret = regmap_update_bits(pcf8525->regmap,
+						 PCF8525_REG_CTRL2,
+						 PCF8525_CTRL2_W0C_FLAGS,
+						 PCF8525_CTRL2_AF |
+						 PCF8525_CTRL2_WDTF);
+			if (ret)
+				return handled;
+			handled = IRQ_HANDLED;
+		}
+	}
+
+	return handled;
+}
+
+static const struct rtc_class_ops pcf8525_rtc_ops = {
+	.ioctl = pcf8525_rtc_ioctl,
+	.read_time = pcf8525_read_time,
+	.set_time = pcf8525_set_time,
+	.read_alarm = pcf8525_read_alarm,
+	.set_alarm = pcf8525_set_alarm,
+	.alarm_irq_enable = pcf8525_alarm_irq_enable,
+	.read_offset = pcf8525_rtc_read_offset,
+	.set_offset = pcf8525_rtc_set_offset,
+	.param_get = pcf8525_param_get,
+	.param_set = pcf8525_param_set,
+};
+
+static int pcf8525_get_irqs(struct i2c_client *client, struct pcf8525 *pcf8525)
+{
+	struct device *dev = &client->dev;
+	int irq;
+
+	irq = fwnode_irq_get_byname(dev_fwnode(dev), "inta");
+	if (irq == -ENOENT || irq == -EINVAL)
+		irq = client->irq;
+	else if (irq < 0)
+		return irq;
+	pcf8525->irq_inta = irq;
+
+	return 0;
+}
+
+/*
+ * pcf8525_mask_inta - devm cleanup: re-mask INTA alarm and timestamp
+ * interrupts on driver unbind so the open-drain line is not left asserted.
+ */
+static void pcf8525_mask_inta(void *data)
+{
+	struct pcf8525 *pcf8525 = data;
+	unsigned int m1, m2;
+	int ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK1, &m1);
+	if (!ret) {
+		m1 |= PCF8525_MASK1_AIE;
+		regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK1, m1);
+	}
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK2, &m2);
+	if (!ret) {
+		m2 |= PCF8525_MASK2_TSIE;
+		regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK2, m2);
+	}
+}
+
+static int pcf8525_unmask_irqs_inta(struct pcf8525 *pcf8525)
+{
+	/*
+	 * Mask registers: bit=1 means masked/disabled.
+	 * We want alarm + timestamp unmasked on INTA by default:
+	 * - AIE => mask bit2 must be 0
+	 * - TSIE => mask2 bit3 must be 0
+	 */
+	unsigned int m1, m2;
+	int ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK1, &m1);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK2, &m2);
+	if (ret)
+		return ret;
+
+	m1 |= PCF8525_MASK1_WD_CD;	/* keep watchdog masked on INTA */
+	m1 &= ~PCF8525_MASK1_AIE;	/* unmask alarm interrupt on INTA */
+	m2 &= ~PCF8525_MASK2_TSIE;	/* unmask timestamp interrupt on INTA */
+
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK1, m1);
+	if (ret)
+		return ret;
+
+	return regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK2, m2);
+}
+
+static int pcf8525_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct pcf8525 *pcf8525;
+	int ret;
+
+	pcf8525 = devm_kzalloc(dev, sizeof(*pcf8525), GFP_KERNEL);
+	if (!pcf8525)
+		return -ENOMEM;
+
+	mutex_init(&pcf8525->ts_mutex);
+
+	pcf8525->regmap = devm_regmap_init_i2c(client, &pcf8525_regmap_cfg);
+	if (IS_ERR(pcf8525->regmap))
+		return dev_err_probe(dev, PTR_ERR(pcf8525->regmap),
+				     "failed to init regmap\n");
+
+	i2c_set_clientdata(client, pcf8525);
+
+	ret = pcf8525_get_irqs(client, pcf8525);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get IRQs\n");
+
+	pcf8525->rtc = devm_rtc_allocate_device(dev);
+	if (IS_ERR(pcf8525->rtc))
+		return dev_err_probe(dev, PTR_ERR(pcf8525->rtc),
+				     "failed to allocate RTC device\n");
+
+	pcf8525->rtc->ops = &pcf8525_rtc_ops;
+	pcf8525->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	pcf8525->rtc->range_max = RTC_TIMESTAMP_END_2099;
+	pcf8525->rtc->set_start_time = true;
+
+	clear_bit(RTC_FEATURE_ALARM, pcf8525->rtc->features);
+	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, pcf8525->rtc->features);
+	set_bit(RTC_FEATURE_BACKUP_SWITCH_MODE, pcf8525->rtc->features);
+
+	/* Use 24-hour mode */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL1,
+				 PCF8525_CTRL1_12_24, 0);
+	if (ret)
+		return ret;
+
+	ret = pcf8525_configure_crystal(dev, pcf8525);
+	if (ret)
+		return ret;
+	/*
+	 * Enable timestamp engine + timestamp interrupt (TSIE).
+	 */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_TS_CTL1,
+				 PCF8525_TS_CTL1_TSM | PCF8525_TS_CTL1_TSOFF,
+				 PCF8525_TS_CTL1_TSM);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL5,
+				 PCF8525_CTRL5_TSIE, PCF8525_CTRL5_TSIE);
+	if (ret)
+		return ret;
+
+	/* Create sysfs timestamp0/1 */
+	ret = rtc_add_group(pcf8525->rtc, &pcf8525_attr_group);
+	if (ret)
+		return ret;
+
+	/* Optional IRQ */
+	if (pcf8525->irq_inta > 0) {
+		ret = devm_request_threaded_irq(dev, pcf8525->irq_inta,
+						NULL, pcf8525_irq,
+						IRQF_ONESHOT,
+						dev_name(dev), dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to request INTA IRQ\n");
+
+		pcf8525->irq_enabled = true;
+
+		/*
+		 * Register the INTA cleanup action before unmasking hardware
+		 * interrupt sources. If the unmask step fails, the devm
+		 * unwinding will still re-mask INTA so the line is not left
+		 * asserted on probe failure.
+		 */
+		ret = devm_add_action_or_reset(dev, pcf8525_mask_inta, pcf8525);
+		if (ret)
+			return ret;
+
+		/* Route/unmask alarm + timestamp to INTA by default */
+		ret = pcf8525_unmask_irqs_inta(pcf8525);
+		if (ret)
+			return ret;
+
+		ret = devm_device_init_wakeup(dev);
+		if (ret)
+			return ret;
+
+		ret = devm_pm_set_wake_irq(dev, pcf8525->irq_inta);
+		if (ret)
+			return ret;
+
+		set_bit(RTC_FEATURE_ALARM, pcf8525->rtc->features);
+	}
+
+	/* Register device */
+	ret = devm_rtc_register_device(pcf8525->rtc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct of_device_id pcf8525_of_match[] = {
+	{ .compatible = "nxp,pcf8525" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pcf8525_of_match);
+
+static const struct i2c_device_id pcf8525_i2c_id[] = {
+	{ .name = "pcf8525" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, pcf8525_i2c_id);
+
+static struct i2c_driver pcf8525_driver = {
+	.driver = {
+		.name = "rtc-pcf8525",
+		.of_match_table = pcf8525_of_match,
+	},
+	.probe = pcf8525_probe,
+	.id_table = pcf8525_i2c_id,
+};
+module_i2c_driver(pcf8525_driver);
+
+MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@nxp.com>");
+MODULE_AUTHOR("Shiv Prakash Gupta <shivprakash.gupta@nxp.com>");
+MODULE_DESCRIPTION("NXP PCF8525 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v4 3/4] rtc: pcf8525: Add watchdog support

[Shiv Prakash Gupta]

Extend the PCF8525 driver with an optional hardware watchdog.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Signed-off-by: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
---
 drivers/rtc/Kconfig       |   4 +-
 drivers/rtc/rtc-pcf8525.c | 263 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 266 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b6c43b173445..bc597af5b4fe 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -516,11 +516,13 @@ config RTC_DRV_PCF8523
 config RTC_DRV_PCF8525
 	tristate "NXP PCF8525"
 	select REGMAP_I2C
+	select WATCHDOG_CORE if WATCHDOG
 	help
 	  If you say yes here you get support for the NXP PCF8525 RTC chip.
 	  The PCF8525 is an I2C real-time clock with alarm, external timestamp
 	  capture and recording of the last calendar write, crystal aging offset
-	  correction, and backup power switchover.
+	  correction, backup power switchover, and an integrated hardware
+	  watchdog timer.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-pcf8525.
diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
index 3c5f15f95a9e..f8fbc00bb464 100644
--- a/drivers/rtc/rtc-pcf8525.c
+++ b/drivers/rtc/rtc-pcf8525.c
@@ -22,6 +22,7 @@
 #include <linux/uaccess.h>
 #include <linux/kernel.h>
 #include <linux/pm.h>
+#include <linux/watchdog.h>
 
 /* Registers */
 #define PCF8525_REG_CTRL1		0x00
@@ -71,6 +72,8 @@
 #define PCF8525_REG_INTB_MASK1		0x2A
 #define PCF8525_REG_INTB_MASK2		0x2B
 
+#define PCF8525_REG_WD_CTL		0x2C
+#define PCF8525_REG_WD_VAL		0x2D
 #define PCF8525_REG_TEMP		0x2E
 
 /* CTRL1 */
@@ -88,6 +91,9 @@
 #define PCF8525_CTRL2_SMBTO		BIT(0)
 /*
  * All W0C status bits in CTRL2 (writing 1 = no effect, writing 0 = clear).
+ * Include WDTF here so that any regmap_update_bits touching CTRL2 control
+ * bits (AIE, TI_TP, etc.) does not accidentally clear a pending watchdog
+ * timer interrupt by writing 0 to WDTF in the RMW cycle.
  */
 #define PCF8525_CTRL2_W0C_FLAGS		(PCF8525_CTRL2_MSF | \
 					 PCF8525_CTRL2_WDTF | \
@@ -137,6 +143,16 @@
 #define PCF8525_ALM_AE_MONTH		BIT(7)
 #define PCF8525_ALM_AE_YEAR		BIT(6)
 
+/* Watchdog control register bits */
+#define PCF8525_WD_CTL_WD_CD		BIT(7)
+#define PCF8525_WD_CTL_TF0		BIT(0)
+#define PCF8525_WD_CTL_TF1		BIT(1)
+
+#define PCF8525_WD_CLOCK_HZ_X1000	250 /* 1/4 Hz */
+#define PCF8525_WD_MIN_HW_HEARTBEAT_MS	4000
+#define PCF8525_WD_VAL_STOP		0
+#define PCF8525_WD_DEFAULT_TIMEOUT_S	60
+
 #define PCF8525_REG_AGING_OFFSET_HI	0x26
 #define PCF8525_REG_AGING_OFFSET_LO	0x27
 /*
@@ -151,6 +167,7 @@
 
 struct pcf8525 {
 	struct rtc_device *rtc;
+	struct watchdog_device wdd;
 	struct regmap *regmap;
 	struct mutex ts_mutex;	/* protects ts[] and ts_valid[] in IRQ mode */
 	bool irq_enabled;
@@ -906,6 +923,211 @@ static const struct rtc_class_ops pcf8525_rtc_ops = {
 	.param_set = pcf8525_param_set,
 };
 
+static unsigned int pcf8525_wdt_timeout_to_val(unsigned int timeout)
+{
+	unsigned int val;
+
+	val = DIV_ROUND_UP(timeout * PCF8525_WD_CLOCK_HZ_X1000, 1000) + 1;
+
+	if (val < 2)
+		return 2;
+	if (val > 255)
+		return 255;
+
+	return val;
+}
+
+static int pcf8525_wdt_ping(struct watchdog_device *wdd)
+{
+	struct pcf8525 *pcf8525 = watchdog_get_drvdata(wdd);
+	unsigned int wd_val;
+
+	wd_val = pcf8525_wdt_timeout_to_val(wdd->timeout);
+
+	return regmap_write(pcf8525->regmap, PCF8525_REG_WD_VAL, wd_val);
+}
+
+static int pcf8525_wdt_active_ping(struct watchdog_device *wdd)
+{
+	if (watchdog_active(wdd))
+		return pcf8525_wdt_ping(wdd);
+
+	return 0;
+}
+
+static int pcf8525_wdt_start(struct watchdog_device *wdd)
+{
+	return pcf8525_wdt_ping(wdd);
+}
+
+static int pcf8525_wdt_stop(struct watchdog_device *wdd)
+{
+	struct pcf8525 *pcf8525 = watchdog_get_drvdata(wdd);
+
+	return regmap_write(pcf8525->regmap, PCF8525_REG_WD_VAL,
+			    PCF8525_WD_VAL_STOP);
+}
+
+static int pcf8525_watchdog_get_period(int n, int f1000)
+{
+	return (1000 * (n - 1)) / f1000;
+}
+
+static int pcf8525_wdt_set_timeout(struct watchdog_device *wdd,
+				   unsigned int timeout)
+{
+	unsigned int wd_val;
+
+	wd_val = pcf8525_wdt_timeout_to_val(timeout);
+	/* Report the achieved timeout, not the requested value. */
+	wdd->timeout = pcf8525_watchdog_get_period(wd_val,
+						   PCF8525_WD_CLOCK_HZ_X1000);
+
+	return pcf8525_wdt_active_ping(wdd);
+}
+
+static const struct watchdog_info pcf8525_wdt_info = {
+	.identity = "NXP PCF8525 Watchdog",
+	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+};
+
+static const struct watchdog_ops pcf8525_watchdog_ops = {
+	.owner = THIS_MODULE,
+	.start = pcf8525_wdt_start,
+	.stop = pcf8525_wdt_stop,
+	.ping = pcf8525_wdt_ping,
+	.set_timeout = pcf8525_wdt_set_timeout,
+};
+
+static int pcf8525_watchdog_config(struct device *dev,
+				   struct pcf8525 *pcf8525)
+{
+	unsigned int m1, m2;
+	int ret;
+
+	/*
+	 * WD_VAL is write-only, so a watchdog started by firmware cannot be
+	 * detected reliably. Stop it explicitly to establish a known state
+	 * before registering the Linux watchdog device.
+	 */
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_WD_VAL,
+			   PCF8525_WD_VAL_STOP);
+	if (ret)
+		return ret;
+
+	/* Configure nINTB/CLKOUT as nINTB open-drain interrupt output. */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CLKOUT,
+				 PCF8525_CLKOUT_CLKOE, 0);
+	if (ret)
+		return ret;
+
+	/*
+	 * Select 1/4 Hz clock source: TF[1:0] = 10.
+	 * Do NOT enable WD_CD yet; enable it only after clearing stale WDTF
+	 * to avoid a spurious INTB assertion on a stale flag.
+	 */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_WD_CTL,
+				 PCF8525_WD_CTL_WD_CD |
+				 PCF8525_WD_CTL_TF1 |
+				 PCF8525_WD_CTL_TF0,
+				 PCF8525_WD_CTL_TF1);
+	if (ret)
+		return ret;
+
+	/* Keep watchdog masked on INTA. */
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK1, &m1);
+	if (ret)
+		return ret;
+
+	m1 |= PCF8525_MASK1_WD_CD;
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK1, m1);
+	if (ret)
+		return ret;
+
+	/* Read INTB mask; mask all non-watchdog sources on INTB first. */
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK1, &m1);
+	if (ret)
+		return ret;
+
+	m1 |= PCF8525_MASK1_BIE |
+	      PCF8525_MASK1_AIE |
+	      PCF8525_MASK1_OSIE |
+	      PCF8525_MASK1_SI |
+	      PCF8525_MASK1_MI |
+	      PCF8525_MASK1_WD_CD;   /* keep WD masked until WDTF is clear */
+
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK2, &m2);
+	if (ret)
+		return ret;
+
+	m2 |= PCF8525_MASK2_TSIE;
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK2, m2);
+	if (ret)
+		return ret;
+
+	/*
+	 * Clear stale WDTF before enabling WD_CD and unmasking INTB to
+	 * avoid asserting the watchdog output immediately.
+	 */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
+				 PCF8525_CTRL2_WDTF | PCF8525_CTRL2_AF |
+				 PCF8525_CTRL2_MSF,
+				 PCF8525_CTRL2_AF | PCF8525_CTRL2_MSF);
+	if (ret)
+		return ret;
+
+	/* Enable WD_CD now that WDTF has been cleared. */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_WD_CTL,
+				 PCF8525_WD_CTL_WD_CD,
+				 PCF8525_WD_CTL_WD_CD);
+	if (ret)
+		return ret;
+
+	/* Unmask watchdog on INTB. */
+	m1 &= ~PCF8525_MASK1_WD_CD;
+	return regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);
+}
+
+static int pcf8525_watchdog_init(struct device *dev,
+				 struct pcf8525 *pcf8525)
+{
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_WATCHDOG_CORE) ||
+	    !device_property_read_bool(dev, "reset-source"))
+		return 0;
+
+	/*
+	 * Configure the hardware first: this switches the nINTB/CLKOUT pin
+	 * from clock output (CLKOE=1) to open-drain output before the
+	 * watchdog is registered.
+	 */
+	ret = pcf8525_watchdog_config(dev, pcf8525);
+	if (ret)
+		return ret;
+
+	pcf8525->wdd.parent = dev;
+	pcf8525->wdd.info = &pcf8525_wdt_info;
+	pcf8525->wdd.ops = &pcf8525_watchdog_ops;
+	pcf8525->wdd.min_timeout = pcf8525_watchdog_get_period(2,
+							       PCF8525_WD_CLOCK_HZ_X1000);
+	pcf8525->wdd.max_timeout = pcf8525_watchdog_get_period(255,
+							       PCF8525_WD_CLOCK_HZ_X1000);
+	pcf8525->wdd.timeout = PCF8525_WD_DEFAULT_TIMEOUT_S;
+	watchdog_init_timeout(&pcf8525->wdd, 0, dev);
+	pcf8525->wdd.min_hw_heartbeat_ms = PCF8525_WD_MIN_HW_HEARTBEAT_MS;
+	pcf8525->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+	watchdog_set_drvdata(&pcf8525->wdd, pcf8525);
+	watchdog_stop_on_reboot(&pcf8525->wdd);
+
+	return devm_watchdog_register_device(dev, &pcf8525->wdd);
+}
+
 static int pcf8525_get_irqs(struct i2c_client *client, struct pcf8525 *pcf8525)
 {
 	struct device *dev = &client->dev;
@@ -973,6 +1195,42 @@ static int pcf8525_unmask_irqs_inta(struct pcf8525 *pcf8525)
 	return regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK2, m2);
 }
 
+static int pcf8525_suspend(struct device *dev)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	int ret;
+
+	/* Stop watchdog first: failure here must abort suspend. */
+	if (IS_ENABLED(CONFIG_WATCHDOG_CORE) &&
+	    device_property_read_bool(dev, "reset-source") &&
+	    watchdog_active(&pcf8525->wdd)) {
+		ret = pcf8525_wdt_stop(&pcf8525->wdd);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int pcf8525_resume(struct device *dev)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	int ret;
+
+	/* Ping watchdog first to avoid an imminent reset after resume. */
+	if (IS_ENABLED(CONFIG_WATCHDOG_CORE) &&
+	    device_property_read_bool(dev, "reset-source") &&
+	    watchdog_active(&pcf8525->wdd)) {
+		ret = pcf8525_wdt_ping(&pcf8525->wdd);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pcf8525_pm_ops, pcf8525_suspend, pcf8525_resume);
+
 static int pcf8525_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -1076,6 +1334,10 @@ static int pcf8525_probe(struct i2c_client *client)
 		set_bit(RTC_FEATURE_ALARM, pcf8525->rtc->features);
 	}
 
+	ret = pcf8525_watchdog_init(dev, pcf8525);
+	if (ret)
+		return ret;
+
 	/* Register device */
 	ret = devm_rtc_register_device(pcf8525->rtc);
 	if (ret)
@@ -1100,6 +1362,7 @@ static struct i2c_driver pcf8525_driver = {
 	.driver = {
 		.name = "rtc-pcf8525",
 		.of_match_table = pcf8525_of_match,
+		.pm = pm_sleep_ptr(&pcf8525_pm_ops),
 	},
 	.probe = pcf8525_probe,
 	.id_table = pcf8525_i2c_id,
-- 
2.34.1

[PATCH v4 4/4] rtc: pcf8525: Add temperature sensor support via hwmon

[Shiv Prakash Gupta]

Expose the PCF8525 on-chip temperature sensor through the hwmon
subsystem as an optional feature controlled by
CONFIG_RTC_DRV_PCF8525_HWMON.

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Signed-off-by: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
---
 drivers/rtc/Kconfig       |  11 ++
 drivers/rtc/rtc-pcf8525.c | 233 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 244 insertions(+)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index bc597af5b4fe..27bca34c2fca 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -527,6 +527,17 @@ config RTC_DRV_PCF8525
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-pcf8525.
 
+
+config RTC_DRV_PCF8525_HWMON
+	bool "HWMON support for NXP PCF8525"
+	depends on RTC_DRV_PCF8525 && HWMON && !(RTC_DRV_PCF8525=y && HWMON=m)
+	default y
+	help
+	  Say Y here to expose the PCF8525 on-chip temperature sensor
+	  through the hwmon interface (temp1_input in millidegrees Celsius).
+	  The sensor resolution is 1 degree C.  The measurement interval is
+	  programmable from 1 to 32 minutes via hwmon chip_update_interval.
+
 config RTC_DRV_PCF85363
 	tristate "NXP PCF85363"
 	select REGMAP_I2C
diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
index f8fbc00bb464..5cb984958f28 100644
--- a/drivers/rtc/rtc-pcf8525.c
+++ b/drivers/rtc/rtc-pcf8525.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/bcd.h>
+#include <linux/hwmon.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/device.h>
@@ -152,6 +153,7 @@
 #define PCF8525_WD_MIN_HW_HEARTBEAT_MS	4000
 #define PCF8525_WD_VAL_STOP		0
 #define PCF8525_WD_DEFAULT_TIMEOUT_S	60
+#define PCF8525_CLKOUT_TCR_MASK		GENMASK(7, 5)
 
 #define PCF8525_REG_AGING_OFFSET_HI	0x26
 #define PCF8525_REG_AGING_OFFSET_LO	0x27
@@ -171,6 +173,7 @@ struct pcf8525 {
 	struct regmap *regmap;
 	struct mutex ts_mutex;	/* protects ts[] and ts_valid[] in IRQ mode */
 	bool irq_enabled;
+	bool hwmon_enabled;
 	time64_t ts[2];
 	bool ts_valid[2];
 	int irq_inta;
@@ -346,6 +349,217 @@ static int pcf8525_rtc_set_offset(struct device *dev, long offset)
 	return pcf8525_write_aging_offset(pcf8525, (s16)raw);
 }
 
+static int pcf8525_hwmon_read_temp(struct device *dev, long *temp)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_TEMP, &regval);
+	if (ret)
+		return ret;
+
+	/*
+	 * PCF8525: signed 8-bit, 1 degree C per LSB.
+	 * HWMON requires millidegree Celsius.
+	 */
+	*temp = (long)(s8)(u8)regval * 1000L;
+
+	return 0;
+}
+
+static int pcf8525_hwmon_read_update_interval(struct device *dev, long *val)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	unsigned int regval;
+	unsigned int tcr;
+	int ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CLKOUT, &regval);
+	if (ret)
+		return ret;
+
+	tcr = FIELD_GET(PCF8525_CLKOUT_TCR_MASK, regval);
+
+	switch (tcr) {
+	case 0:
+		*val = 32 * 60 * 1000L;
+		break;
+	case 1:
+		*val = 16 * 60 * 1000L;
+		break;
+	case 2:
+		*val = 8 * 60 * 1000L;
+		break;
+	case 3:
+		*val = 4 * 60 * 1000L;
+		break;
+	case 4:
+		*val = 2 * 60 * 1000L;
+		break;
+	default:
+		*val = 60 * 1000L;
+		break;
+	}
+
+	return 0;
+}
+
+static int pcf8525_hwmon_write_update_interval(struct device *dev, long val)
+{
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	unsigned int tcr;
+
+	switch (val) {
+	case 32 * 60 * 1000L:
+		tcr = 0;
+		break;
+	case 16 * 60 * 1000L:
+		tcr = 1;
+		break;
+	case 8 * 60 * 1000L:
+		tcr = 2;
+		break;
+	case 4 * 60 * 1000L:
+		tcr = 3;
+		break;
+	case 2 * 60 * 1000L:
+		tcr = 4;
+		break;
+	case 60 * 1000L:
+		tcr = 5;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Update only TCR[2:0]; preserve OTPR, CLKOE and COF[2:0]. */
+	return regmap_update_bits(pcf8525->regmap, PCF8525_REG_CLKOUT,
+				  PCF8525_CLKOUT_TCR_MASK,
+				  FIELD_PREP(PCF8525_CLKOUT_TCR_MASK, tcr));
+}
+
+static umode_t pcf8525_hwmon_is_visible(const void *data,
+					enum hwmon_sensor_types type,
+					u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_chip:
+		if (attr == hwmon_chip_update_interval)
+			return 0644;
+		break;
+	case hwmon_temp:
+		if (attr == hwmon_temp_input && channel == 0)
+			return 0444;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int pcf8525_hwmon_read(struct device *dev,
+			      enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_chip:
+		if (attr == hwmon_chip_update_interval)
+			return pcf8525_hwmon_read_update_interval(dev, val);
+		break;
+	case hwmon_temp:
+		if (attr == hwmon_temp_input && channel == 0)
+			return pcf8525_hwmon_read_temp(dev, val);
+		break;
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int pcf8525_hwmon_write(struct device *dev,
+			       enum hwmon_sensor_types type,
+			       u32 attr, int channel, long val)
+{
+	if (type == hwmon_chip && attr == hwmon_chip_update_interval)
+		return pcf8525_hwmon_write_update_interval(dev, val);
+
+	return -EOPNOTSUPP;
+}
+
+static const struct hwmon_channel_info * const pcf8525_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+	NULL
+};
+
+static const struct hwmon_ops pcf8525_hwmon_ops = {
+	.is_visible = pcf8525_hwmon_is_visible,
+	.read = pcf8525_hwmon_read,
+	.write = pcf8525_hwmon_write,
+};
+
+static const struct hwmon_chip_info pcf8525_hwmon_chip_info = {
+	.ops = &pcf8525_hwmon_ops,
+	.info = pcf8525_hwmon_info,
+};
+
+static void pcf8525_hwmon_disable_temp(void *data)
+{
+	struct pcf8525 *pcf8525 = data;
+
+	/* Disable digital temperature readout on driver removal. */
+	regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL5,
+			   PCF8525_CTRL5_TEMP_RD_EN, 0);
+}
+
+/*
+ * Keep HWMON optional and non-fatal so RTC and watchdog registration remain
+ * usable even if the temperature interface cannot be registered.
+ */
+static void pcf8525_hwmon_register(struct device *dev,
+				   struct pcf8525 *pcf8525)
+{
+	struct device *hwmon_dev;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_RTC_DRV_PCF8525_HWMON))
+		return;
+
+	/* Enable the temperature readout before exposing hwmon attributes. */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL5,
+				 PCF8525_CTRL5_TEMP_RD_EN,
+				 PCF8525_CTRL5_TEMP_RD_EN);
+	if (ret) {
+		dev_warn(dev, "failed to enable temperature readout: %d\n", ret);
+		return;
+	}
+
+	/*
+	 * Register a devm cleanup action to disable the digital
+	 * temperature readout when the driver is unbound.
+	 */
+	if (devm_add_action_or_reset(dev, pcf8525_hwmon_disable_temp, pcf8525)) {
+		dev_warn(dev, "failed to register HWMON cleanup action\n");
+		return;
+	}
+
+	/* Only expose sysfs attributes once the hardware is fully ready. */
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "pcf8525",
+							 pcf8525,
+							 &pcf8525_hwmon_chip_info,
+							 NULL);
+	if (IS_ERR(hwmon_dev)) {
+		dev_warn(dev, "failed to register HWMON device: %ld\n",
+			 PTR_ERR(hwmon_dev));
+		pcf8525_hwmon_disable_temp(pcf8525);
+		return;
+	}
+	pcf8525->hwmon_enabled = true;
+}
+
 static int pcf8525_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
@@ -1209,6 +1423,14 @@ static int pcf8525_suspend(struct device *dev)
 			return ret;
 	}
 
+	/* Disable digital temperature readout (warn-only on failure). */
+	if (IS_ENABLED(CONFIG_RTC_DRV_PCF8525_HWMON) && pcf8525->hwmon_enabled) {
+		ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL5,
+					 PCF8525_CTRL5_TEMP_RD_EN, 0);
+		if (ret)
+			dev_warn(dev, "suspend: failed to disable TEMP_RD_EN: %d\n", ret);
+	}
+
 	return 0;
 }
 
@@ -1226,6 +1448,15 @@ static int pcf8525_resume(struct device *dev)
 			return ret;
 	}
 
+	/* Re-enable digital temperature readout (warn-only on failure). */
+	if (IS_ENABLED(CONFIG_RTC_DRV_PCF8525_HWMON) && pcf8525->hwmon_enabled) {
+		ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL5,
+					 PCF8525_CTRL5_TEMP_RD_EN,
+					 PCF8525_CTRL5_TEMP_RD_EN);
+		if (ret)
+			dev_warn(dev, "resume: failed to restore TEMP_RD_EN: %d\n", ret);
+	}
+
 	return 0;
 }
 
@@ -1343,6 +1574,8 @@ static int pcf8525_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	pcf8525_hwmon_register(dev, pcf8525);
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v4 4/4] rtc: pcf8525: Add temperature sensor support via hwmon

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rtc: pcf8525: use devm_release_action() on hwmon error in pcf8525_hwmon_register()
--

commit 657bb5e33615f2ed31527354e958f8aeecac64a5
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>

rtc: pcf8525: Add temperature sensor support via hwmon

This commit adds optional hwmon support for the PCF8525 on-chip temperature
sensor, exposing temperature readings and a configurable update interval.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> index f8fbc00bb464d..5cb984958f286 100644
> --- a/drivers/rtc/rtc-pcf8525.c
> +++ b/drivers/rtc/rtc-pcf8525.c
[ ... ]
> @@ -346,6 +349,217 @@ static int pcf8525_rtc_set_offset(struct device *dev, long offset)
[ ... ]
> +static void pcf8525_hwmon_register(struct device *dev,
> +				   struct pcf8525 *pcf8525)
> +{
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_RTC_DRV_PCF8525_HWMON))
> +		return;
> +
> +	/* Enable the temperature readout before exposing hwmon attributes. */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL5,
> +				 PCF8525_CTRL5_TEMP_RD_EN,
> +				 PCF8525_CTRL5_TEMP_RD_EN);
> +	if (ret) {
> +		dev_warn(dev, "failed to enable temperature readout: %d\n", ret);
> +		return;
> +	}
> +
> +	/*
> +	 * Register a devm cleanup action to disable the digital
> +	 * temperature readout when the driver is unbound.
> +	 */
> +	if (devm_add_action_or_reset(dev, pcf8525_hwmon_disable_temp, pcf8525)) {
> +		dev_warn(dev, "failed to register HWMON cleanup action\n");
> +		return;
> +	}
> +
> +	/* Only expose sysfs attributes once the hardware is fully ready. */
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "pcf8525",
> +							 pcf8525,
> +							 &pcf8525_hwmon_chip_info,
> +							 NULL);
> +	if (IS_ERR(hwmon_dev)) {
> +		dev_warn(dev, "failed to register HWMON device: %ld\n",
> +			 PTR_ERR(hwmon_dev));
> +		pcf8525_hwmon_disable_temp(pcf8525);
> +		return;
> +	}

[Severity: Low]
In pcf8525_hwmon_register() in drivers/rtc/rtc-pcf8525.c, if
devm_hwmon_device_register_with_info() fails after devm_add_action_or_reset()
has already registered pcf8525_hwmon_disable_temp() on dev, calling
pcf8525_hwmon_disable_temp(pcf8525) directly leaves the devres action queued
while pcf8525_probe() succeeds. When the device is later unbound, devres will
invoke pcf8525_hwmon_disable_temp() a second time.

Should this error path call devm_release_action(dev,
pcf8525_hwmon_disable_temp, pcf8525) instead of calling
pcf8525_hwmon_disable_temp(pcf8525) directly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030110139.1929109-1-shivprakash.gupta@nxp.com?part=4

Re: [PATCH v4 2/4] rtc: pcf8525: Add NXP PCF8525 RTC driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] rtc: pcf8525: duplicate wake IRQ registration in pcf8525_probe()
- [High] rtc: pcf8525: stuck IRQ and skipped alarm in pcf8525_irq() on TS1 read error
- [Medium] rtc: pcf8525: cross-clobbering and -EINVAL in timestamp_store_common()
- [Medium] rtc: pcf8525: undrained pending flags and unmasked IRQs in pcf8525_probe()
- [Medium] rtc: pcf8525: alarm updated while AIE is enabled in pcf8525_set_alarm()
- [Medium] rtc: pcf8525: year range shift and timestamp offset mismatch in pcf8525_ts_read()
- [Medium] rtc: pcf8525: missing ts_mutex serialization in sysfs timestamp callbacks
- [Medium] rtc: pcf8525: clearing TSF after CTS in timestamp_store_common() drops events
--

commit e0078479b8adc02e333509cd27a01c1bdef9f20e
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>

rtc: pcf8525: Add NXP PCF8525 RTC driver

This commit adds an I2C RTC driver for the NXP PCF8525, supporting calendar
timekeeping, alarms, external and last-write timestamp capture, crystal aging
offset correction, and backup power switchover.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> new file mode 100644
> index 0000000000000..3c5f15f95a9ed
> --- /dev/null
> +++ b/drivers/rtc/rtc-pcf8525.c
> @@ -0,0 +1,1112 @@

[ ... ]

> +static int pcf8525_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	u8 buf[7];
> +	int ret;
> +
> +	/*
> +	 * Clear AF (W0C). Use W0C_FLAGS as the mask and preserve MSF and WDTF
> +	 * by writing 1 to them: writing 0 to a W0C bit that has asserted
> +	 * between the read and write would silently drop the event.
> +	 */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
> +				 PCF8525_CTRL2_W0C_FLAGS,
> +				 PCF8525_CTRL2_MSF | PCF8525_CTRL2_WDTF);
> +	if (ret)
> +		return ret;
> +
> +	buf[0] = bin2bcd(alrm->time.tm_sec) & 0x7f;
> +	buf[1] = bin2bcd(alrm->time.tm_min) & 0x7f;
> +	buf[2] = bin2bcd(alrm->time.tm_hour) & 0x3f;
> +	buf[3] = bin2bcd(alrm->time.tm_mday) & 0x3f;
> +
> +	/* Disable match on weekday/month/year by setting AE bits */
> +	buf[4] = PCF8525_ALM_AE; /* weekday alarm ignored */
> +	buf[5] = PCF8525_ALM_AE_MONTH | PCF8525_ALM_AE_YEAR;  /* disable month + year alarm */
> +	buf[6] = 0x00; /* year alarm value (ignored if month/year disabled) */
> +
> +	ret = regmap_bulk_write(pcf8525->regmap, PCF8525_REG_ALM_SEC, buf, sizeof(buf));
> +	if (ret)
> +		return ret;
> +
> +	return pcf8525_alarm_irq_enable(dev, alrm->enabled);
> +}

[Severity: Medium]
In pcf8525_set_alarm(), PCF8525_CTRL2_AF is cleared before writing the alarm
registers while PCF8525_CTRL2_AIE may still be enabled.

If the previous alarm register values or intermediate values during the
regmap_bulk_write() to PCF8525_REG_ALM_SEC match the current RTC time after
PCF8525_CTRL2_AF is cleared, can the hardware re-assert PCF8525_CTRL2_AF and
trigger a spurious interrupt (even when alrm->enabled is 0) or leave a stale
PCF8525_CTRL2_AF pending when pcf8525_alarm_irq_enable() enables AIE?

Should pcf8525_set_alarm() disable PCF8525_CTRL2_AIE first, write the new
alarm registers, and then clear PCF8525_CTRL2_AF when enabling AIE?

> +
> +/* ---- Timestamp handling (sysfs) ---- */
> +static int pcf8525_ts_read(struct device *dev, int id, time64_t *ts_out)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	struct rtc_time tm;
> +	u8 buf[6];
> +	int ret;
> +	u8 base;
> +
> +	/*
> +	 * TS0 maps to TS1 (0x17..0x1C)
> +	 * TS1 maps to TS2 (0x1E..0x23) (skip subsec at 0x1D)
> +	 */
> +	if (id == 0)
> +		base = PCF8525_REG_TS1_SEC;
> +	else if (id == 1)
> +		base = PCF8525_REG_TS2_SEC;
> +	else
> +		return -EINVAL;
> +
> +	ret = regmap_bulk_read(pcf8525->regmap, base, buf, sizeof(buf));
> +	if (ret)
> +		return ret;
> +
> +	tm.tm_sec = bcd2bin(buf[0] & 0x7f);
> +	tm.tm_min = bcd2bin(buf[1] & 0x7f);
> +	tm.tm_hour = bcd2bin(buf[2] & 0x3f);
> +	tm.tm_mday = bcd2bin(buf[3] & 0x3f);
> +	tm.tm_mon = bcd2bin(buf[4] & 0x1f) - 1;
> +	tm.tm_year = bcd2bin(buf[5]) + 100;
> +
> +	ret = rtc_valid_tm(&tm);
> +	if (ret)
> +		return ret;
> +
> +	*ts_out = rtc_tm_to_time64(&tm);
> +	return 0;
> +}

[Severity: Medium]
In pcf8525_probe(), the driver sets:

	pcf8525->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
	pcf8525->rtc->range_max = RTC_TIMESTAMP_END_2099;
	pcf8525->rtc->set_start_time = true;

while leaving pcf8525->rtc->start_secs at 0 (1970-01-01). When start-year is
not specified in Device Tree, rtc_device_get_offset() in drivers/rtc/class.c
sees set_start_time == true with start_secs == 0, sets rtc->offset_secs to
-(range_max - range_min + 1), and shifts the valid RTC range from 2000..2099
to 1970..2069 (causing dates in 2070..2099 to be rejected with -ERANGE). Note
that rtc_device_get_offset() already sets set_start_time = true automatically
whenever the start-year property is present in firmware.

Furthermore, when rtc->offset_secs is non-zero, rtc_read_time() and
rtc_set_time() apply rtc_add_offset() and rtc_subtract_offset(), whereas
pcf8525_ts_read() decodes tm.tm_year = bcd2bin(buf[5]) + 100 directly without
applying the RTC offset. Won't that cause a 100-year mismatch between
calendar reads/writes and timestamps read via pcf8525_ts_read()?

> +
> +static ssize_t timestamp_show_common(struct device *dev, char *buf, int id)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev->parent);
> +	unsigned int ctrl4;
> +	time64_t ts;
> +	int ret;
> +
> +	if (id < 0 || id > 1)
> +		return -EINVAL;
> +
> +	/*
> +	 * Timestamp2 (id=1) records the last calendar/date-time write.
> +	 * It is updated automatically by hardware on every set_time call
> +	 * and is not related to the external TS capture event (TSF).
> +	 * Always read it directly from the hardware registers.
> +	 */
> +	if (id == 1) {
> +		ret = pcf8525_ts_read(dev->parent, 1, &ts);
> +		if (ret)
> +			return ret;
> +		return sysfs_emit(buf, "%llu\n", (unsigned long long)ts);
> +	}

[Severity: Medium]
There appear to be a couple of issues with how timestamp0 and timestamp1 are
handled across timestamp_show_common() and timestamp_store_common():

1. When the TS2 registers (0x1E..0x23) are zeroed (at power-on before
pcf8525_set_time() has been called, or after PCF8525_RESET_CTS_CMD is written
to PCF8525_REG_RESET), pcf8525_ts_read(dev->parent, 1, &ts) reads tm_mday == 0
and tm_mon == -1, so rtc_valid_tm() returns -EINVAL. Because
timestamp_show_common() returns ret directly for id == 1, reading sysfs
timestamp1 when no timestamp is recorded fails with -EINVAL instead of
returning 0 (empty output) like timestamp0_show() does.

2. Both timestamp0_store() (id = 0) and timestamp1_store() (id = 1) call
timestamp_store_common(), which validates id but then ignores it and writes
PCF8525_RESET_CTS_CMD to PCF8525_REG_RESET, resets pcf8525->ts_valid[0] and
pcf8525->ts_valid[1], and clears PCF8525_CTRL4_TSF. Because
PCF8525_RESET_CTS_CMD zeroes both TS1 and TS2 hardware registers, writing to
timestamp0 erases the last-calendar-write timestamp in timestamp1, and
writing to timestamp1 erases any captured external event in timestamp0. Is
this cross-clobbering between the two sysfs attributes intended?

> +
> +	/* id == 0: Timestamp1 = external TS pin capture */
> +	if (pcf8525->irq_enabled) {
> +		bool valid;
> +
> +		mutex_lock(&pcf8525->ts_mutex);
> +		valid = pcf8525->ts_valid[0];
> +		if (valid)
> +			ts = pcf8525->ts[0];
> +		mutex_unlock(&pcf8525->ts_mutex);
> +
> +		if (!valid)
> +			return 0;
> +		return sysfs_emit(buf, "%llu\n", (unsigned long long)ts);
> +	}
> +
> +	/* polling mode: only report if TSF is currently set */
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL4, &ctrl4);
> +	if (ret)
> +		return ret;
> +
> +	if (!(ctrl4 & PCF8525_CTRL4_TSF))
> +		return 0;
> +
> +	ret = pcf8525_ts_read(dev->parent, 0, &ts);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%llu\n", (unsigned long long)ts);
> +}
> +
> +static ssize_t timestamp_store_common(struct device *dev, const char *buf,
> +				      size_t count, int id)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev->parent);
> +	int ret;
> +
> +	if (id < 0 || id > 1)
> +		return -EINVAL;
> +
> +	if (pcf8525->irq_enabled) {
> +		/*
> +		 * Serialize CTS with the IRQ handler. CTS clears the timestamp
> +		 * registers and Timestp_ctl1, so invalidate the cache only after
> +		 * CTS succeeds, restore TSM, then clear TSF.
> +		 */
> +		mutex_lock(&pcf8525->ts_mutex);
> +
> +		ret = regmap_write(pcf8525->regmap, PCF8525_REG_RESET,
> +				   PCF8525_RESET_CTS_CMD);
> +		if (ret) {
> +			mutex_unlock(&pcf8525->ts_mutex);
> +			return ret;
> +		}
> +
> +		/* CTS succeeded: hardware registers are now zeroed. */
> +		pcf8525->ts_valid[0] = false;
> +		pcf8525->ts_valid[1] = false;
> +
> +		/* Restore TSM=1, TSOFF=0 (CTS cleared 0x16). */
> +		ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_TS_CTL1,
> +					 PCF8525_TS_CTL1_TSM | PCF8525_TS_CTL1_TSOFF,
> +					 PCF8525_TS_CTL1_TSM);
> +		if (!ret)
> +			ret = regmap_update_bits(pcf8525->regmap,
> +						 PCF8525_REG_CTRL4,
> +						 PCF8525_CTRL4_TSF, 0);
> +		mutex_unlock(&pcf8525->ts_mutex);
> +		return ret ? ret : (ssize_t)count;
> +	}

[Severity: Medium]
In timestamp_store_common(), PCF8525_CTRL4_TSF is cleared after issuing
PCF8525_RESET_CTS_CMD and after restoring PCF8525_TS_CTL1_TSM in
PCF8525_REG_TS_CTL1.

If a new external timestamp event arrives on the TS pin after
PCF8525_RESET_CTS_CMD (or after PCF8525_TS_CTL1_TSM is restored) and before
the regmap_update_bits() on PCF8525_REG_CTRL4 completes, the hardware latches
the new timestamp into TS1 and sets PCF8525_CTRL4_TSF = 1, which
timestamp_store_common() then immediately clears to 0. When pcf8525_irq()
acquires pcf8525->ts_mutex and re-reads PCF8525_REG_CTRL4 into ctrl4_now, it
sees PCF8525_CTRL4_TSF == 0, assumes timestamp_store_common() zeroed the
registers after the event, and drops the newly captured timestamp.

Could PCF8525_CTRL4_TSF be cleared before issuing PCF8525_RESET_CTS_CMD and
re-enabling PCF8525_TS_CTL1_TSM so a new event arriving after CTS is not
wiped out?

> +
> +	/*
> +	 * Polling path: CTS first, then restore TSM=1/TSOFF=0, then clear TSF.
> +	 */
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_RESET, PCF8525_RESET_CTS_CMD);
> +	if (ret)
> +		return ret;
> +
> +	/* Restore TSM=1, TSOFF=0 (CTS cleared 0x16). */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_TS_CTL1,
> +				 PCF8525_TS_CTL1_TSM | PCF8525_TS_CTL1_TSOFF,
> +				 PCF8525_TS_CTL1_TSM);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL4,
> +				 PCF8525_CTRL4_TSF, 0);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}

[Severity: Medium]
When pcf8525->irq_enabled is false, neither timestamp_show_common() nor
timestamp_store_common() acquires pcf8525->ts_mutex, and the CTS reset
sequence in timestamp_store_common() is duplicated across the two branches.

If one process reads timestamp0 in polling mode (observing PCF8525_CTRL4_TSF
set in PCF8525_REG_CTRL4) while another process concurrently writes to
timestamp0 or timestamp1, PCF8525_RESET_CTS_CMD can zero the TS1 registers
before PCF8525_CTRL4_TSF is cleared two I2C transactions later, causing
pcf8525_ts_read() in timestamp_show_common() to read zeroed registers and
return -EINVAL instead of 0.

Could pcf8525->ts_mutex be held unconditionally in timestamp_show_common()
and timestamp_store_common(), eliminating the duplicated polling branch in
timestamp_store_common()?

[ ... ]

> +static irqreturn_t pcf8525_irq(int irq, void *data)
> +{
> +	struct device *dev = data;
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	irqreturn_t handled = IRQ_NONE;
> +	unsigned int ctrl2, ctrl4;
> +	int ret;
> +
> +	/*
> +	 * Drain all pending INTA sources. Re-read the status registers
> +	 * because another source may assert while INTA is already low.
> +	 */
> +	for (;;) {

[ ... ]

> +			} else {
> +				/*
> +				 * TSM=1 retains the first event while TSF is set, so read TS1
> +				 * before clearing TSF.
> +				 */
> +				ok0 = !pcf8525_ts_read(dev, 0, &ts0);
> +				if (!ok0) {\
> +					/*
> +					 * TS1 read failed; leave TSF set
> +					 * so the event is not silently
> +					 * dropped and hardware can
> +					 * re-trigger.
> +					 */
> +					mutex_unlock(&pcf8525->ts_mutex);
> +					return handled;
> +				}

[Severity: High]
In pcf8525_irq(), if pcf8525_ts_read(dev, 0, &ts0) fails, the handler unlocks
pcf8525->ts_mutex and returns handled immediately without clearing
PCF8525_CTRL4_TSF and before checking PCF8525_CTRL2_AF or PCF8525_CTRL2_MSF.

If an external transition occurs on the TS pin while the RTC calendar is not
yet initialized (so TS1 latches invalid BCD date fields and rtc_valid_tm() in
pcf8525_ts_read() returns -EINVAL), PCF8525_TS_CTL1_TSM = 1 freezes the TS1
registers as long as PCF8525_CTRL4_TSF remains 1. Won't leaving
PCF8525_CTRL4_TSF set cause pcf8525_ts_read() to fail on every subsequent
attempt, either triggering an unhandled IRQ_NONE storm on a level-triggered
IRQ line or holding the open-drain INTA line permanently low on an
edge-triggered IRQ line while starving any pending alarm interrupts?

[ ... ]

> +static int pcf8525_get_irqs(struct i2c_client *client, struct pcf8525 *pcf8525)
> +{
> +	struct device *dev = &client->dev;
> +	int irq;
> +
> +	irq = fwnode_irq_get_byname(dev_fwnode(dev), "inta");
> +	if (irq == -ENOENT || irq == -EINVAL)
> +		irq = client->irq;
> +	else if (irq < 0)
> +		return irq;
> +	pcf8525->irq_inta = irq;
> +
> +	return 0;
> +}

[ ... ]

> +static int pcf8525_unmask_irqs_inta(struct pcf8525 *pcf8525)
> +{
> +	/*
> +	 * Mask registers: bit=1 means masked/disabled.
> +	 * We want alarm + timestamp unmasked on INTA by default:
> +	 * - AIE => mask bit2 must be 0
> +	 * - TSIE => mask2 bit3 must be 0
> +	 */
> +	unsigned int m1, m2;
> +	int ret;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK1, &m1);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK2, &m2);
> +	if (ret)
> +		return ret;
> +
> +	m1 |= PCF8525_MASK1_WD_CD;	/* keep watchdog masked on INTA */
> +	m1 &= ~PCF8525_MASK1_AIE;	/* unmask alarm interrupt on INTA */
> +	m2 &= ~PCF8525_MASK2_TSIE;	/* unmask timestamp interrupt on INTA */
> +
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK1, m1);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK2, m2);
> +}

[Severity: Medium]
There appear to be two issues with how INTA interrupts and pre-existing status
flags are initialized during probe:

1. In pcf8525_unmask_irqs_inta(), only PCF8525_MASK1_WD_CD is masked in
PCF8525_REG_INTA_MASK1 while PCF8525_MASK1_BIE, PCF8525_MASK1_OSIE,
PCF8525_MASK1_SI, and PCF8525_MASK1_MI are left untouched. If any of those
bits were left unmasked by firmware or across a warm reset, pcf8525_irq() does
not clear PCF8525_CTRL3_BF or PCF8525_SC_OSF, which would leave INTA asserted.
Should all unused interrupt sources in PCF8525_REG_INTA_MASK1 and
PCF8525_REG_INTA_MASK2 be explicitly masked?

2. In pcf8525_probe(), PCF8525_CTRL5_TSIE is enabled before requesting the IRQ
(and even when pcf8525->irq_inta <= 0), and pcf8525_probe() never drains
pending status flags after unmasking INTA. If PCF8525_CTRL4_TSF or
PCF8525_CTRL2_AF was already latched prior to probe (for example, while
running on backup power or across a warm reboot) on an IRQ_TYPE_EDGE_FALLING
interrupt line, INTA is already held low so no falling edge will occur. That
leaves pcf8525->ts_valid[0] false (hiding the pre-boot timestamp from
timestamp0_show() in IRQ mode) and leaves INTA stuck low, blocking all future
falling-edge interrupts. Should pcf8525_probe() invoke pcf8525_irq() (or drain
pending flags) after unmasking INTA?

> +
> +static int pcf8525_probe(struct i2c_client *client)
> +{

[ ... ]

> +	/* Optional IRQ */
> +	if (pcf8525->irq_inta > 0) {
> +		ret = devm_request_threaded_irq(dev, pcf8525->irq_inta,
> +						NULL, pcf8525_irq,
> +						IRQF_ONESHOT,
> +						dev_name(dev), dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to request INTA IRQ\n");
> +
> +		pcf8525->irq_enabled = true;
> +
> +		/*
> +		 * Register the INTA cleanup action before unmasking hardware
> +		 * interrupt sources. If the unmask step fails, the devm
> +		 * unwinding will still re-mask INTA so the line is not left
> +		 * asserted on probe failure.
> +		 */
> +		ret = devm_add_action_or_reset(dev, pcf8525_mask_inta, pcf8525);
> +		if (ret)
> +			return ret;
> +
> +		/* Route/unmask alarm + timestamp to INTA by default */
> +		ret = pcf8525_unmask_irqs_inta(pcf8525);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_device_init_wakeup(dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_pm_set_wake_irq(dev, pcf8525->irq_inta);
> +		if (ret)
> +			return ret;
> +
> +		set_bit(RTC_FEATURE_ALARM, pcf8525->rtc->features);
> +	}

[Severity: High]
When a Device Tree node specifies both interrupts and wakeup-source,
of_i2c_get_board_info() sets I2C_CLIENT_WAKE on client->flags. Before calling
pcf8525_probe(), i2c_device_probe() in drivers/i2c/i2c-core-base.c already
calls device_init_wakeup(&client->dev, true) and dev_pm_set_wake_irq(dev,
client->irq):

drivers/i2c/i2c-core-base.c:i2c_device_probe() {
    ...
    device_init_wakeup(&client->dev, true);

    if (wakeirq > 0 && wakeirq != client->irq)
        status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
    else if (client->irq > 0)
        status = dev_pm_set_wake_irq(dev, client->irq);
    ...
}

When pcf8525_probe() then calls devm_pm_set_wake_irq(dev, pcf8525->irq_inta)
with pcf8525->irq_inta == client->irq, dev_pm_attach_wake_irq() hits
dev_WARN_ONCE(dev, dev->power.wakeirq, "wake irq already initialized\n") and
returns -EEXIST, causing pcf8525_probe() to fail.

Conversely, if wakeup-source is specified in Device Tree without interrupts
(for example, when INTA is wired to a PMIC or power-button wake pin),
pcf8525->irq_inta is 0, so pcf8525_probe() never unmasks AIE on INTA or sets
RTC_FEATURE_ALARM. In addition, in pcf8525_get_irqs(), fwnode_irq_get_byname()
returns -ENODATA if interrupt-names is present without an "inta" entry, which
causes pcf8525_get_irqs() to fail instead of falling back to client->irq.

Should pcf8525_probe() check device_property_read_bool(dev, "wakeup-source")
for alarm/wakeup capability and avoid calling devm_pm_set_wake_irq() when
pcf8525->irq_inta == client->irq and I2C_CLIENT_WAKE is already handled by the
I2C core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030110139.1929109-1-shivprakash.gupta@nxp.com?part=2

Re: [PATCH v4 3/4] rtc: pcf8525: Add watchdog support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] rtc: pcf8525: premature nINTB pin switch in pcf8525_watchdog_config()
- [High] rtc: pcf8525: uncanceled deferred ping worker in pcf8525_suspend()
- [Medium] rtc: pcf8525: unthrottled duplicate ping in pcf8525_wdt_set_timeout()
- [Medium] rtc: pcf8525: min_hw_heartbeat_ms equals min_timeout in pcf8525_watchdog_init()
- [Medium] rtc: pcf8525: missing WDIOF_MAGICCLOSE in pcf8525_wdt_info
- [Medium] rtc: pcf8525: missing watchdog_stop_on_unregister() in probe
- [Low] rtc: pcf8525: unquantized timeout in pcf8525_watchdog_init()
--

commit ef4793891aaa47f5f86ec5e83faae0a7013999b0
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>

rtc: pcf8525: Add watchdog support

This commit extends the PCF8525 RTC driver with optional hardware watchdog
support when the reset-source device property is present.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> index 3c5f15f95a9ed..f8fbc00bb464d 100644
> --- a/drivers/rtc/rtc-pcf8525.c
> +++ b/drivers/rtc/rtc-pcf8525.c

[ ... ]

> @@ -906,6 +923,211 @@ static const struct rtc_class_ops pcf8525_rtc_ops = {
>  	.param_set = pcf8525_param_set,
>  };
>  

[ ... ]

> +static int pcf8525_wdt_set_timeout(struct watchdog_device *wdd,
> +				   unsigned int timeout)
> +{
> +	unsigned int wd_val;
> +
> +	wd_val = pcf8525_wdt_timeout_to_val(timeout);
> +	/* Report the achieved timeout, not the requested value. */
> +	wdd->timeout = pcf8525_watchdog_get_period(wd_val,
> +						   PCF8525_WD_CLOCK_HZ_X1000);
> +
> +	return pcf8525_wdt_active_ping(wdd);
> +}

[Severity: Medium]
In pcf8525_wdt_set_timeout(), does calling pcf8525_wdt_active_ping() bypass
wdd->min_hw_heartbeat_ms and cause duplicate writes to PCF8525_REG_WD_VAL?

When userspace invokes ioctl(WDIOC_SETTIMEOUT), watchdog_ioctl() in
drivers/watchdog/watchdog_dev.c calls watchdog_set_timeout() and then
immediately calls watchdog_ping() -> __watchdog_ping():

	case WDIOC_SETTIMEOUT:
		if (get_user(val, p))
			return -EFAULT;
		err = watchdog_set_timeout(wdd, val);
		if (err < 0)
			return err;
		/* If the watchdog is active then we send a keepalive ping
		 * to make sure that the watchdog keep's running (and if
		 * possible that it takes the new timeout) */
		err = watchdog_ping(wdd);
		if (err < 0)
			return err;

Because pcf8525_wdt_set_timeout() -> pcf8525_wdt_active_ping() ->
pcf8525_wdt_ping() writes PCF8525_REG_WD_VAL directly when watchdog_active()
is true, it neither checks wdd->min_hw_heartbeat_ms (4000 ms) nor updates
wd_data->last_hw_keepalive. If less than 4000 ms has elapsed since the prior
hardware write, pcf8525_wdt_active_ping() writes to the hardware immediately
in violation of min_hw_heartbeat_ms, and if at least 4000 ms has elapsed,
pcf8525_wdt_active_ping() writes PCF8525_REG_WD_VAL and watchdog_ping()
immediately writes PCF8525_REG_WD_VAL a second time back-to-back. Also, if
regmap_write() in pcf8525_wdt_active_ping() fails, wdd->timeout has already
been overwritten.

Should pcf8525_wdt_set_timeout() only update wdd->timeout and let the
watchdog core's subsequent watchdog_ping() call handle the hardware write
and rate limiting?

> +
> +static const struct watchdog_info pcf8525_wdt_info = {
> +	.identity = "NXP PCF8525 Watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};

[Severity: Medium]
Should pcf8525_wdt_info.options include WDIOF_MAGICCLOSE?

Without WDIOF_MAGICCLOSE, when CONFIG_WATCHDOG_NOWAYOUT is disabled and the
userspace watchdog daemon crashes or closes /dev/watchdog without writing
the magic 'V' character, watchdog_release() sees !(wdd->info->options &
WDIOF_MAGICCLOSE) and silently stops the hardware watchdog via
watchdog_stop() instead of allowing it to reset the system.

> +
> +static const struct watchdog_ops pcf8525_watchdog_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pcf8525_wdt_start,
> +	.stop = pcf8525_wdt_stop,
> +	.ping = pcf8525_wdt_ping,
> +	.set_timeout = pcf8525_wdt_set_timeout,
> +};
> +
> +static int pcf8525_watchdog_config(struct device *dev,
> +				   struct pcf8525 *pcf8525)
> +{
> +	unsigned int m1, m2;
> +	int ret;
> +
> +	/*
> +	 * WD_VAL is write-only, so a watchdog started by firmware cannot be
> +	 * detected reliably. Stop it explicitly to establish a known state
> +	 * before registering the Linux watchdog device.
> +	 */
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_WD_VAL,
> +			   PCF8525_WD_VAL_STOP);
> +	if (ret)
> +		return ret;
> +
> +	/* Configure nINTB/CLKOUT as nINTB open-drain interrupt output. */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CLKOUT,
> +				 PCF8525_CLKOUT_CLKOE, 0);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Select 1/4 Hz clock source: TF[1:0] = 10.
> +	 * Do NOT enable WD_CD yet; enable it only after clearing stale WDTF
> +	 * to avoid a spurious INTB assertion on a stale flag.
> +	 */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_WD_CTL,
> +				 PCF8525_WD_CTL_WD_CD |
> +				 PCF8525_WD_CTL_TF1 |
> +				 PCF8525_WD_CTL_TF0,
> +				 PCF8525_WD_CTL_TF1);
> +	if (ret)
> +		return ret;
> +
> +	/* Keep watchdog masked on INTA. */
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK1, &m1);
> +	if (ret)
> +		return ret;
> +
> +	m1 |= PCF8525_MASK1_WD_CD;
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK1, m1);
> +	if (ret)
> +		return ret;
> +
> +	/* Read INTB mask; mask all non-watchdog sources on INTB first. */
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK1, &m1);
> +	if (ret)
> +		return ret;
> +
> +	m1 |= PCF8525_MASK1_BIE |
> +	      PCF8525_MASK1_AIE |
> +	      PCF8525_MASK1_OSIE |
> +	      PCF8525_MASK1_SI |
> +	      PCF8525_MASK1_MI |
> +	      PCF8525_MASK1_WD_CD;   /* keep WD masked until WDTF is clear */
> +
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK2, &m2);
> +	if (ret)
> +		return ret;
> +
> +	m2 |= PCF8525_MASK2_TSIE;
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK2, m2);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Clear stale WDTF before enabling WD_CD and unmasking INTB to
> +	 * avoid asserting the watchdog output immediately.
> +	 */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
> +				 PCF8525_CTRL2_WDTF | PCF8525_CTRL2_AF |
> +				 PCF8525_CTRL2_MSF,
> +				 PCF8525_CTRL2_AF | PCF8525_CTRL2_MSF);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable WD_CD now that WDTF has been cleared. */
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_WD_CTL,
> +				 PCF8525_WD_CTL_WD_CD,
> +				 PCF8525_WD_CTL_WD_CD);
> +	if (ret)
> +		return ret;
> +
> +	/* Unmask watchdog on INTB. */
> +	m1 &= ~PCF8525_MASK1_WD_CD;
> +	return regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);
> +}

[Severity: High]
Can clearing PCF8525_CLKOUT_CLKOE at the beginning of
pcf8525_watchdog_config() cause an immediate spurious board reset during
probe?

In pcf8525_probe(), before pcf8525_watchdog_init() ->
pcf8525_watchdog_config() runs, the driver already enables timestamp capture
and PCF8525_CTRL5_TSIE, and across a warm reboot or prior firmware activity,
stale interrupt flags (such as WDTF, AF, MSF, or TSF) and unmasked bits in
PCF8525_REG_INTB_MASK1 / PCF8525_REG_INTB_MASK2 or PCF8525_WD_CTL_WD_CD may
already be active.

Although the comments note that WD_CD and INTB_MASK1 must remain
disabled/masked until non-watchdog sources are masked and stale WDTF is
cleared, pcf8525_watchdog_config() switches the physical nINTB/CLKOUT pin to
active-low nINTB by clearing PCF8525_CLKOUT_CLKOE before clearing WD_CD,
before masking non-watchdog sources in PCF8525_REG_INTB_MASK1 and
PCF8525_REG_INTB_MASK2, and before clearing stale WDTF in PCF8525_REG_CTRL2.
During those intervening I2C transactions, any pending unmasked INTB source
can drive nINTB low and reset the system.

Should clearing PCF8525_CLKOUT_CLKOE in PCF8525_REG_CLKOUT be moved to the
end of pcf8525_watchdog_config(), after all INTB masks, stale flags, and
WD_CTL have been configured?

> +
> +static int pcf8525_watchdog_init(struct device *dev,
> +				 struct pcf8525 *pcf8525)
> +{
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_WATCHDOG_CORE) ||
> +	    !device_property_read_bool(dev, "reset-source"))
> +		return 0;
> +
> +	/*
> +	 * Configure the hardware first: this switches the nINTB/CLKOUT pin
> +	 * from clock output (CLKOE=1) to open-drain output before the
> +	 * watchdog is registered.
> +	 */
> +	ret = pcf8525_watchdog_config(dev, pcf8525);
> +	if (ret)
> +		return ret;
> +
> +	pcf8525->wdd.parent = dev;
> +	pcf8525->wdd.info = &pcf8525_wdt_info;
> +	pcf8525->wdd.ops = &pcf8525_watchdog_ops;
> +	pcf8525->wdd.min_timeout = pcf8525_watchdog_get_period(2,
> +							       PCF8525_WD_CLOCK_HZ_X1000);
> +	pcf8525->wdd.max_timeout = pcf8525_watchdog_get_period(255,
> +							       PCF8525_WD_CLOCK_HZ_X1000);
> +	pcf8525->wdd.timeout = PCF8525_WD_DEFAULT_TIMEOUT_S;
> +	watchdog_init_timeout(&pcf8525->wdd, 0, dev);

[Severity: Low]
When timeout-sec is provided in Device Tree with a value that is not a
multiple of the 4-second hardware period, watchdog_init_timeout() stores the
unquantized value into pcf8525->wdd.timeout, whereas pcf8525_wdt_ping()
programs a rounded-up multiple of 4 seconds into PCF8525_REG_WD_VAL. Should
pcf8525->wdd.timeout be normalized via pcf8525_wdt_timeout_to_val() and
pcf8525_watchdog_get_period() after watchdog_init_timeout() so that
WDIOC_GETTIMEOUT and sysfs report the actual hardware period, matching
pcf8525_wdt_set_timeout()?

> +	pcf8525->wdd.min_hw_heartbeat_ms = PCF8525_WD_MIN_HW_HEARTBEAT_MS;

[Severity: Medium]
Can the watchdog ever be kept alive when configured with min_timeout (4
seconds) while min_hw_heartbeat_ms is also 4000 ms?

In pcf8525_watchdog_init(), min_timeout is pcf8525_watchdog_get_period(2,
250) = 4 seconds (4000 ms), and min_hw_heartbeat_ms is set to
PCF8525_WD_MIN_HW_HEARTBEAT_MS (4000 ms). When timeout is 4 seconds,
pcf8525_wdt_ping() writes WD_VAL = 2 (1 tick of the 1/4 Hz clock, at most
4000 ms). However, in drivers/watchdog/watchdog_dev.c, __watchdog_ping()
computes earliest_keepalive = wd_data->last_hw_keepalive + 4000 ms and
defers any keepalive ping arriving before 4000 ms have elapsed until after
the 4-second hardware countdown has already expired.

Should min_timeout be increased (for example to 8 seconds, WD_VAL = 3) if
min_hw_heartbeat_ms must remain 4000 ms?

> +	pcf8525->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +	watchdog_set_drvdata(&pcf8525->wdd, pcf8525);
> +	watchdog_stop_on_reboot(&pcf8525->wdd);
> +
> +	return devm_watchdog_register_device(dev, &pcf8525->wdd);
> +}

[Severity: Medium]
Should pcf8525_watchdog_init() also call
watchdog_stop_on_unregister(&pcf8525->wdd)?

Without WDOG_STOP_ON_UNREGISTER set, if the driver is unbound via sysfs
while the watchdog is active, or if devm_rtc_register_device() fails in
pcf8525_probe() after userspace has opened /dev/watchdog,
devm_watchdog_unregister_device() removes the watchdog device without
stopping the hardware countdown in PCF8525_REG_WD_VAL, leaving the system to
reset when the timer expires.

[ ... ]

> @@ -973,6 +1195,42 @@ static int pcf8525_unmask_irqs_inta(struct pcf8525 *pcf8525)
>  	return regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK2, m2);
>  }
>  
> +static int pcf8525_suspend(struct device *dev)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* Stop watchdog first: failure here must abort suspend. */
> +	if (IS_ENABLED(CONFIG_WATCHDOG_CORE) &&
> +	    device_property_read_bool(dev, "reset-source") &&
> +	    watchdog_active(&pcf8525->wdd)) {
> +		ret = pcf8525_wdt_stop(&pcf8525->wdd);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pcf8525_resume(struct device *dev)
> +{
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	/* Ping watchdog first to avoid an imminent reset after resume. */
> +	if (IS_ENABLED(CONFIG_WATCHDOG_CORE) &&
> +	    device_property_read_bool(dev, "reset-source") &&
> +	    watchdog_active(&pcf8525->wdd)) {
> +		ret = pcf8525_wdt_ping(&pcf8525->wdd);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Can a deferred keepalive ping from the watchdog core fire after
pcf8525_suspend() has stopped the hardware watchdog?

Because pcf8525_watchdog_init() sets min_hw_heartbeat_ms = 4000 ms, a
userspace keepalive ping within 4 seconds prior to system suspend causes
__watchdog_ping() in drivers/watchdog/watchdog_dev.c to arm wd_data->timer,
which queues wd_data->work (watchdog_ping_work) on the non-freezable
watchdog_kworker kthread.

Since pcf8525_watchdog_init() does not call
watchdog_stop_ping_on_suspend(&pcf8525->wdd), wd_data->timer and
wd_data->work are not canceled on suspend, and WDOG_ACTIVE remains set when
pcf8525_suspend() calls pcf8525_wdt_stop(). If wd_data->timer expires after
pcf8525_suspend() runs (for example during s2idle or before noirq suspend),
watchdog_ping_work() sees watchdog_active() is true and calls
__watchdog_ping() -> pcf8525_wdt_ping(), either re-enabling the hardware
watchdog while the system is asleep or issuing an I2C transfer after the
parent I2C adapter has suspended. In addition, pcf8525_resume() calls
pcf8525_wdt_ping() directly without updating wd_data->last_hw_keepalive or
serializing with wd_data->lock.

Could pcf8525_watchdog_init() call watchdog_stop_ping_on_suspend() (or use
watchdog_dev_suspend() and watchdog_dev_resume() in pcf8525_suspend() and
pcf8525_resume()) so the watchdog core cancels any pending ping timer/worker
on suspend and updates last_hw_keepalive on resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030110139.1929109-1-shivprakash.gupta@nxp.com?part=3