Re: [PATCH v5 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Ignored `dev_err_probe()` return value defeats probe deferral for the INTA interrupt line.
- [Medium] Unconditionally clearing `RTC_FEATURE_ALARM` when no IRQ is present breaks wake-on-alarm functionality for boards that only configure a wakeup-source.
--

commit c4131e782120105d3473c8eca13fcfb2ec8f0bf1
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 on the
PCF85263/PCF85363 to latch the time on selected events. The capture source
is configured via device tree and exported through read-only sysfs attributes,
along with other initialization cleanups.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -573,57 +819,75 @@ static int pcf85363_probe(struct i2c_client *client)
[ ... ]
> +		ret = devm_request_threaded_irq(dev, irq_a, NULL,
> +						pcf85363_rtc_handle_irq,
> +						irqflags | IRQF_ONESHOT,
> +						"pcf85363-inta", client);
> +
> +		if (ret) {
> +			dev_err_probe(dev, ret, "INTA IRQ request failed\n");
> +			irq_a = 0;
> +			clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
> +		} else {

[Severity: Medium]
Will ignoring the return value of dev_err_probe() in this block defeat
probe deferral?

If devm_request_threaded_irq() fails because the interrupt provider is not
yet ready, it returns -EPROBE_DEFER. By not propagating this error and
falling back to setting irq_a to 0, the driver permanently drops interrupt
support on affected boots instead of allowing the core to defer probing.

> +			u8 inta_en = INT_BSIE;
> +
> +			/*
> +			 * Enable the timestamp interrupt only when a capture
> +			 * mode is configured; always enable battery-switch.
> +			 */
> +			if (ts_mode_configured)
> +				inta_en |= INT_TSRIE;
> +
> +			ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
> +						 INT_BSIE | INT_TSRIE, inta_en);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "Failed to enable INTA sources\n");
>  		}
> -	}
> -
> -	if (client->irq > 0 || wakeup_source) {
> -		device_init_wakeup(&client->dev, true);
> -		set_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
>  	} else {
> +		/* Without an interrupt line the alarm cannot be delivered. */
>  		clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
>  	}

[Severity: Medium]
Does clearing RTC_FEATURE_ALARM unconditionally when there is no direct IRQ
break wake-on-alarm functionality?

If a system routes the RTC interrupt pin to an external PMIC to wake the
system and defines the device tree with a wakeup-source property but no
direct interrupts property, this logic clears the alarm feature bit. The
RTC core will reject alarm setup requests when this feature bit is cleared,
which seems like it could cause a functional regression for userspace tools
like rtcwake that rely on the wakeup-source capability.

> +	if (irq_a > 0 || wakeup_source)
> +		device_init_wakeup(dev, true);
> +
> +	dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
[ ... ]

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