Re: [PATCH v8 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: pcf85363: unacknowledged FLAGS_A1F holds INTA low in wakeup-only mode
--

commit 655ad6de5d4c27a6bb5b02bf6ca7a7a0b68d6b38
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

rtc: pcf85363: support reporting battery switch-over via RTC_VL

This commit adds support for reporting and clearing the battery switch-over
flag (FLAGS_BSF) via the RTC_VL_READ and RTC_VL_CLR ioctls, introduces
pcf85363_clear_flags() for write-0-to-clear flag handling, and updates
pcf85363_probe() to selectively mask unused interrupt sources and clear
stale flags instead of zeroing CTRL_FLAGS.

> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index ccc7834e57599..00f37ffcca260 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -435,17 +577,52 @@ static int pcf85363_probe(struct i2c_client *client)
>  
>  	wakeup_source = device_property_read_bool(&client->dev,
>  						  "wakeup-source");
> -	if (client->irq > 0 || wakeup_source) {
> -		err = regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
> -		if (err)
> -			return dev_err_probe(&client->dev, err,
> -					     "failed to clear flags\n");
>  
> -		err = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> +	/*
> +	 * Latch and clear any battery-switch event that occurred before
> +	 * probe (for example while the main supply was off) instead of
> +	 * blanket clearing CTRL_FLAGS, so the cached state is preserved.
> +	 */
> +	scoped_guard(mutex, &pcf85363->lock) {
> +		ret = pcf85363_collect_events(pcf85363);
> +		if (ret < 0)
> +			return dev_err_probe(&client->dev, ret,
> +					     "Failed to latch boot-time events\n");
> +	}
> +
> +	/*
> +	 * Battery-backed registers can retain stale state across a power cycle.
> +	 * A stale asserted flag would storm the level-triggered INTA line, so
> +	 * disable only the interrupt sources and Alarm2 enables this driver
> +	 * never services and clear their flags; leave the managed sources
> +	 * (A1IE, BSIE) and Alarm1 for their own paths to arm. WDIE is masked
> +	 * too so a bootloader-armed watchdog cannot storm INTA before the
> +	 * driver is ready to service it.
> +	 */
> +	ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
> +				 INT_WDIE | INT_A2IE | INT_OIE | INT_PIE | INT_ILP,
> +				 0);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to mask unused INTA sources\n");
> +
> +	ret = regmap_update_bits(pcf85363->regmap, DT_ALARM_EN,
> +				 ALRM_MIN_A2E | ALRM_HR_A2E | ALRM_DAY_A2E, 0);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to mask Alarm2 enables\n");
> +
> +	ret = pcf85363_clear_flags(pcf85363, FLAGS_A2F | FLAGS_PIF);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to clear stale flags\n");
> +
> +	if (client->irq > 0 || wakeup_source) {
> +		ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
>  					 PIN_IO_INTAPM, PIN_IO_INTA_OUT);
> -		if (err)
> -			return dev_err_probe(&client->dev, err,
> -					     "failed to set interrupt pin mode\n");
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					     "Failed to configure INTA pin\n");
>  	}

[Severity: Medium]
On boards configured with wakeup-source where client->irq <= 0 (for example,
when INTA is wired to a PMIC or power-on circuit rather than a dedicated SoC
IRQ, or when devm_request_threaded_irq() fails and clears client->irq),
can leaving FLAGS_A1F uncleared in pcf85363_probe() keep the level-low INTA
pin permanently asserted after an alarm wakes or boots the system?

Previously, pcf85363_probe() cleared all flags in CTRL_FLAGS when
client->irq > 0 || wakeup_source, which acknowledged a boot-time FLAGS_A1F
event and released INTA.

With this change, pcf85363_probe() only clears FLAGS_BSF (via
pcf85363_collect_events()) and FLAGS_A2F | FLAGS_PIF, leaving INT_A1IE and
FLAGS_A1F asserted. When client->irq <= 0, pcf85363_rtc_handle_irq() is not
registered to clear FLAGS_A1F. In addition, devm_rtc_register_device() ->
rtc_initialize_alarm() -> __rtc_read_alarm() -> pcf85363_rtc_read_alarm()
reads alrm->enabled = 1 with tm_year = -1, which rolls the expired alarm
forward to the next year and enqueues rtc->aie_timer without touching the
hardware registers, leaving INTA driven low and causing wakealarm_store() to
return -EBUSY unless the alarm is explicitly disabled or overwritten.

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