Re: [PATCH v8 4/6] rtc: s35390a: Read 24-hour mode on access
From: sashiko-bot@kernel.org
Date: 2026-08-31 20:24:02
Also in:
linux-rtc
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Pre-existing issues: - [High] Alarm PM bit is unconditionally set for PM hours, breaking alarms in 24-hour mode. - [High] Multiple functions read the STATUS1 register but fail to handle the POC (Power-On Clear) and BLD (Battery Low Detect) flags correctly, permanently masking power-loss events. -- commit b899282922e9e75f3f9690275fbd3d093dfa12e5 Author: Markus Probst [off-list ref] Subject: rtc: s35390a: Read 24-hour mode on access This commit modifies the driver to dynamically read the 24-hour mode status from the chip on access, rather than caching it during probe. This prevents the mode from going out of sync if the chip is reset while in 12-hour mode.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -292,7 +297,7 @@ static int s35390a_rtc_set_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 = 0; + u8 buf[3], status1, status2 = 0; int err, i;
[ ... ]
/* clear pending interrupt (in STATUS1 only), if any */ - err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &sts, sizeof(sts)); + err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &status1, sizeof(status1)); if (err < 0) return err;
[Severity: High] This is a pre-existing issue, but does reading S35390A_CMD_STATUS1 here silently clear the POC (Power-On Clear) and BLD (Battery Low Detect) flags in the hardware without handling them? Since these flags are automatically cleared by the hardware upon reading STATUS1, it appears that s35390a_probe(), s35390a_rtc_set_alarm(), and s35390a_rtc_ioctl() all inadvertently erase the power-loss state. By bypassing the proper power-loss handling, will subsequent reads of the time unknowingly trust and present an invalid or garbage RTC time to the system after a battery failure?
quoted hunk ↗ jump to hunk
if (alm->enabled) - sts = S35390A_INT2_MODE_ALARM; + status2 = S35390A_INT2_MODE_ALARM; else - sts = S35390A_INT2_MODE_NOINTR; + status2 = S35390A_INT2_MODE_NOINTR; /* set interrupt mode*/ - err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts)); + err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &status2, sizeof(status2)); if (err < 0) return err;@@ -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 unconditionally setting the 0x40 bit for PM hours here break alarms in 24-hour mode? In 24-hour mode, bit 6 is not used for PM, so setting it writes an invalid BCD value that the time register will never match. Will this prevent the alarm from ever firing for times >= 12:00 when the RTC is configured in 24-hour mode? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260831-rtc_s35390a_int1-v8-0-dac1dc86ce1f@posteo.de?part=4