diff mbox series

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

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

Commit Message

Lakshay Piplani Aug. 12, 2026, 7:45 a.m. UTC 
Add support for the timestamp capture registers available on PCF85263 and
PCF85363. The registers latch the current time when selected events occur,
such as TS pin activation or battery switch-over.

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

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

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
V3 -> V4:
- No changes in v4.
V2 -> V3:
- Split into separate patches as suggested:
  - Battery switch-over detection.
  - Timestamp recording for TS pin and battery switch-over events.
  - Offset calibration.
  - Watchdog timer (to be reviewed by watchdog maintainers).
- Dropped Alarm2 support
- Switched to rtc_add_group() for sysfs attributes
V1 -> V2:
- Watchdog related changes due to removal of vendor specific properties
  from device tree
  * remove vendor DT knobs (enable/timeout/stepsize/repeat)
  * use watchdog_init_timeout (with 10s default)
  * derive clock_sel from final timeout
  * default, repeat=true (repeat mode)
- Fixed uninitalised warning on 'ret' (reported by kernel test robot)
- Use dev_dbg instead of dev_info for debug related print messages
- Minor cleanup and comments.

 drivers/rtc/rtc-pcf85363.c | 209 +++++++++++++++++++++++++++++++------
 1 file changed, 175 insertions(+), 34 deletions(-)

Comments

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

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

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

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

rtc: pcf85363: add timestamp support with configurable timestamp mode

This commit adds support for the timestamp capture registers available on
PCF85263 and PCF85363 devices. The capture source is configured via the
device tree, and latched values are exported through read-only sysfs
attributes.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index c03d5a65c5f79..e10e58f690127 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -317,6 +333,24 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
>  		handled = true;
>  	}
>  
> +	if (flags & FLAGS_TSR1F) {
> +		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR1F, 0);

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Patch

diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index c03d5a65c5f7..e10e58f69012 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -16,6 +16,7 @@ 
 #include <linux/bcd.h>
 #include <linux/device.h>
 #include <linux/of.h>
+#include <linux/rtc.h>
 #include <linux/regmap.h>
 
 /*
@@ -101,19 +102,31 @@ 
 #define PIN_IO_INTA_OUT	2
 #define PIN_IO_INTA_HIZ	3
 
+#define PIN_IO_TSPM     GENMASK(3, 2)
+#define PIN_IO_TSIM     BIT(4)
+
 #define OSC_CAP_SEL	GENMASK(1, 0)
 #define OSC_CAP_6000	0x01
 #define OSC_CAP_12500	0x02
 
 #define STOP_EN_STOP	BIT(0)
+#define RTCM_BIT        BIT(4)
 
 #define RESET_CPR	0xa4
 
 #define NVRAM_SIZE	0x40
 
+#define TSR1_MASK       0x03
+#define TSR2_MASK       0x07
+#define TSR3_MASK       0x03
+#define TSR1_SHIFT      0
+#define TSR2_SHIFT      2
+#define TSR3_SHIFT      6
+
 struct pcf85363 {
 	struct rtc_device	*rtc;
 	struct regmap		*regmap;
+	u8 ts_valid_flags;
 };
 
 struct pcf85x63_config {
@@ -306,8 +319,11 @@  static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	if (flags) {
-		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s\n",
+		dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s\n",
 			flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
+			(flags & FLAGS_TSR1F) ? " [TSR1F]" : "",
+			(flags & FLAGS_TSR2F) ? " [TSR2F]" : "",
+			(flags & FLAGS_TSR3F) ? " [TSR3F]" : "",
 			(flags & FLAGS_BSF) ? " [BSF]" : "");
 	}
 
@@ -317,6 +333,24 @@  static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
 		handled = true;
 	}
 
+	if (flags & FLAGS_TSR1F) {
+		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR1F, 0);
+		pcf85363->ts_valid_flags |= FLAGS_TSR1F;
+		handled = true;
+	}
+
+	if (flags & FLAGS_TSR2F) {
+		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR2F, 0);
+		pcf85363->ts_valid_flags |= FLAGS_TSR2F;
+		handled = true;
+	}
+
+	if (flags & FLAGS_TSR3F) {
+		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_TSR3F, 0);
+		pcf85363->ts_valid_flags |= FLAGS_TSR3F;
+		handled = true;
+	}
+
 	if (flags & FLAGS_BSF) {
 		regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0);
 		handled = true;
@@ -424,11 +458,94 @@  static const struct pcf85x63_config pcf_85363_config = {
 	.num_nvram = 2
 };
 
+/*
+ * Reads 6 bytes of timestamp data starting at the given base register,
+ * converts them from BCD to binary, and formats the result into a
+ * human-readable string in "YYYY-MM-DD HH:MM:SS" format.
+ */
+static int pcf85363_read_timestamp(struct pcf85363 *pcf85363, u8 base_reg, char *buf)
+{
+	struct rtc_time tm;
+	u8 regs[6];
+	int ret;
+
+	ret = regmap_bulk_read(pcf85363->regmap, base_reg, regs, sizeof(regs));
+
+	if (ret)
+		return ret;
+
+	tm.tm_sec = bcd2bin(regs[0]);
+	tm.tm_min = bcd2bin(regs[1]);
+	tm.tm_hour = bcd2bin(regs[2]);
+	tm.tm_mday = bcd2bin(regs[3]);
+	tm.tm_mon = bcd2bin(regs[4]) - 1;
+	tm.tm_year = bcd2bin(regs[5]) + 100;
+
+	return sysfs_emit(buf, "%04d-%02d-%02d %02d:%02d:%02d\n",
+			  tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+			  tm.tm_hour, tm.tm_min, tm.tm_sec);
+}
+
+/*
+ * Checks whether a specific timestamp flag is set. If so, reads and
+ * returns the formatted timestamp. Otherwise, returns "00-00-00 00:00:00".
+ */
+
+static ssize_t pcf85363_timestamp_show(struct device *dev, char *buf,
+				       u8 timestamp_flag, u8 base_reg)
+{
+	struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+
+	if (!(pcf85363->ts_valid_flags & timestamp_flag))
+		return sysfs_emit(buf, "00-00-00 00:00:00\n");
+
+	return pcf85363_read_timestamp(pcf85363, base_reg, buf);
+}
+
+static ssize_t timestamp1_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return pcf85363_timestamp_show(dev, buf, FLAGS_TSR1F, DT_TIMESTAMP1);
+}
+static DEVICE_ATTR_RO(timestamp1);
+
+static ssize_t timestamp2_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return pcf85363_timestamp_show(dev, buf, FLAGS_TSR2F, DT_TIMESTAMP2);
+}
+static DEVICE_ATTR_RO(timestamp2);
+
+static ssize_t timestamp3_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	return pcf85363_timestamp_show(dev, buf, FLAGS_TSR3F, DT_TIMESTAMP3);
+}
+static DEVICE_ATTR_RO(timestamp3);
+
+static struct attribute *pcf85363_attrs[] = {
+	&dev_attr_timestamp1.attr,
+	&dev_attr_timestamp2.attr,
+	&dev_attr_timestamp3.attr,
+	NULL,
+};
+
+static const struct attribute_group pcf85363_attr_group = {
+	.attrs = pcf85363_attrs,
+};
+
 static int pcf85363_probe(struct i2c_client *client)
 {
-	struct pcf85363 *pcf85363;
 	const struct pcf85x63_config *config = &pcf_85363_config;
 	const void *data = of_device_get_match_data(&client->dev);
+	struct device *dev = &client->dev;
+	struct pcf85363 *pcf85363;
+	int irq_a = client->irq;
+	bool wakeup_source;
+	int ret, i, err;
+	u32 tsr_mode[3];
+	u8 val;
+
 	static struct nvmem_config nvmem_cfg[] = {
 		{
 			.name = "pcf85x63-",
@@ -446,25 +563,43 @@  static int pcf85363_probe(struct i2c_client *client)
 			.reg_write = pcf85363_nvram_write,
 		},
 	};
-	int ret, i, err;
-	bool wakeup_source;
 
 	if (data)
 		config = data;
 
-	pcf85363 = devm_kzalloc(&client->dev, sizeof(struct pcf85363),
-				GFP_KERNEL);
+	pcf85363 = devm_kzalloc(&client->dev, sizeof(*pcf85363), GFP_KERNEL);
 	if (!pcf85363)
 		return -ENOMEM;
 
+	pcf85363->ts_valid_flags = 0;
+
 	pcf85363->regmap = devm_regmap_init_i2c(client, &config->regmap);
-	if (IS_ERR(pcf85363->regmap)) {
-		dev_err(&client->dev, "regmap allocation failed\n");
-		return PTR_ERR(pcf85363->regmap);
-	}
+	if (IS_ERR(pcf85363->regmap))
+		return dev_err_probe(dev, PTR_ERR(pcf85363->regmap), "regmap init failed\n");
 
 	i2c_set_clientdata(client, pcf85363);
 
+	ret = regmap_update_bits(pcf85363->regmap, CTRL_FUNCTION, RTCM_BIT, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable RTC mode\n");
+
+	if (!device_property_read_u32_array(dev, "nxp,timestamp-mode", tsr_mode, 3)) {
+		tsr_mode[0] &= TSR1_MASK;
+		tsr_mode[1] &= TSR2_MASK;
+		tsr_mode[2] &= TSR3_MASK;
+
+		val = (tsr_mode[2] << TSR3_SHIFT) |
+		      (tsr_mode[1] << TSR2_SHIFT) |
+		      (tsr_mode[0] << TSR1_SHIFT);
+
+		ret = regmap_write(pcf85363->regmap, DT_TS_MODE, val);
+		if (ret)
+			dev_warn(dev, "Failed to write timestamp mode register\n");
+
+		dev_dbg(dev, "Timestamp mode set: TSR1=0x%x TSR2=0x%x TSR3=0x%x\n",
+			tsr_mode[0], tsr_mode[1], tsr_mode[2]);
+	}
+
 	pcf85363->rtc = devm_rtc_allocate_device(&client->dev);
 	if (IS_ERR(pcf85363->rtc))
 		return PTR_ERR(pcf85363->rtc);
@@ -478,38 +613,44 @@  static int pcf85363_probe(struct i2c_client *client)
 	pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
 	pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
 
-	wakeup_source = device_property_read_bool(&client->dev,
-						  "wakeup-source");
-	if (client->irq > 0 || wakeup_source) {
-		regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
-		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
-				   PIN_IO_INTAPM, PIN_IO_INTA_OUT);
-	}
+	wakeup_source = device_property_read_bool(dev, "wakeup-source");
 
-	if (client->irq > 0) {
-		unsigned long irqflags = IRQF_TRIGGER_LOW;
+	ret = regmap_write(pcf85363->regmap, CTRL_FLAGS, 0x00);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to clear CTRL_FLAGS\n");
+
+	if (irq_a > 0) {
+		regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO, PIN_IO_INTAPM, PIN_IO_INTA_OUT);
+		ret = devm_request_threaded_irq(dev, irq_a, NULL,
+						pcf85363_rtc_handle_irq,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						"pcf85363-inta", client);
 
-		if (dev_fwnode(&client->dev))
-			irqflags = 0;
-		ret = devm_request_threaded_irq(&client->dev, client->irq,
-						NULL, pcf85363_rtc_handle_irq,
-						irqflags | IRQF_ONESHOT,
-						"pcf85363", client);
 		if (ret) {
-			dev_warn(&client->dev,
-				 "unable to request IRQ, alarms disabled\n");
-			client->irq = 0;
+			dev_err_probe(dev, ret, "INTA IRQ request failed\n");
+			irq_a = 0;
+		} else {
+			regmap_write(pcf85363->regmap, CTRL_INTA_EN, INT_BSIE
+				     | INT_TSRIE);
 		}
 	}
 
-	if (client->irq > 0 || wakeup_source) {
-		device_init_wakeup(&client->dev, true);
-		set_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
-	} else {
-		clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
-	}
+	regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+			   PIN_IO_TSPM | PIN_IO_TSIM,
+			   PIN_IO_TSPM | PIN_IO_TSIM);
+
+	if (irq_a > 0 || wakeup_source)
+		device_init_wakeup(dev, true);
+
+	dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
+
+	ret = rtc_add_group(pcf85363->rtc, &pcf85363_attr_group);
+	if (ret)
+		return ret;
 
 	ret = devm_rtc_register_device(pcf85363->rtc);
+	if (ret)
+		return dev_err_probe(dev, ret, "RTC registration failed\n");
 
 	for (i = 0; i < config->num_nvram; i++) {
 		nvmem_cfg[i].priv = pcf85363;