Re: [PATCH v7 4/6] rtc: s35390a: Read 24-hour mode on access

[sashiko-bot@kernel.org]

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

New issues:
- [High] Reading STATUS1 in s35390a_rtc_read_alarm() unintentionally clears pending interrupts.

Pre-existing issues:
- [High] Alarm configuration corrupts PM hours in 24-hour mode.
--

commit e047fd38f53476482d3e7de4c5b2930bff24fc30
Author: Markus Probst <markus.probst@posteo.de>
Date:   Sat Aug 29 14:22:58 2026 +0000

    rtc: s35390a: Read 24-hour mode on access

This patch modifies the s35390a RTC driver to read the 24-hour mode flag
dynamically upon access rather than caching it once at probe time. This ensures
the mode cannot get out of sync and resolves time corruption issues that occur
when resetting the chip while in 12-hour mode.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index 575bb256eb258..12ed504db94d8 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
[ ... ]
> @@ -325,8 +330,8 @@ static int s35390a_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  	else
>  		buf[S35390A_ALRM_BYTE_WDAY] = 0;
>  
> -	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
> -			alm->time.tm_hour) | 0x80;
> +	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(alm->time.tm_hour,
> +						      status1 & S35390A_FLAG_24H) | 0x80;
>  	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
>  
>  	if (alm->time.tm_hour >= 12)
>  		buf[S35390A_ALRM_BYTE_HOURS] |= 0x40;

[Severity: High]
This isn't a bug introduced by this patch, but does this bitwise operation
corrupt the 24-hour mode alarm hour format by wrongly applying the 12-hour PM
bit?

When configuring an alarm via s35390a_rtc_set_alarm(), the hour is encoded via
s35390a_hr2reg(). In 24-hour mode, this correctly returns the BCD value of the
hour without bit 6 (0x40) set.

However, unconditionally applying the 0x40 bit if tm_hour >= 12 forces bit 6
high. Since the actual time register in 24-hour mode does not use bit 6 (max
hour is 23, BCD 0x23), will this prevent the RTC hardware's alarm match logic
from matching the corrupted alarm register with the current time?

[ ... ]
> @@ -345,14 +350,20 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct s35390a *s35390a = i2c_get_clientdata(client);
> -	u8 buf[3], sts;
> +	u8 buf[3], status1, status2;
>  	int i, err;
>  
> -	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
> +	err = s35390a_read_status(s35390a, &status1);
> +	if (err == 1)
> +		return -EINVAL;

[Severity: High]
Does reading STATUS1 here in s35390a_rtc_read_alarm() unintentionally clear
pending interrupts?

As documented by the clear pending interrupt comment in
s35390a_rtc_set_alarm(), reading the STATUS1 register clears the hardware's
pending interrupt flag.

If a user or subsystem merely queries the alarm configuration by calling
s35390a_rtc_read_alarm(), could this I2C read cause pending wake events or
interrupts to be silently cleared and dropped in the RTC hardware without being
processed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829-rtc_s35390a_int1-v7-0-debcd31143ae@posteo.de?part=4