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

[sashiko-bot@kernel.org]

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