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

[sashiko-bot@kernel.org]

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

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

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

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

rtc: pcf85363: add timestamp support with configurable timestamp mode

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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