Re: [PATCH v4 2/6] rtc: s35390a: Add missing newline to dev_err

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The probe function silently consumes and clears the hardware's power-loss status flags (POC and BLD) without acting upon them or preserving their state, causing the driver to later return invalid time to userspace.
- [High] Incorrect AM/PM bit logic in 24-hour mode causes alarms for PM hours to silently fail to match.
- [High] The `s35390a_init` function places the hardware into 24-hour mode but fails to update the driver's cached `twentyfourhour` state.
- [High] Missing `alarm_irq_enable` callback breaks RTC alarm interrupt toggle ioctls and core disable logic.
--

--- Patch [2]: [PATCH v4 2/6] rtc: s35390a: Add missing newline to dev_err ---
commit ea0b1d2724771f9f9acaf29d23d929d878073a1e
Author: Markus Probst <markus.probst@posteo.de>
rtc: s35390a: Add missing newline to dev_err

This commit adds a missing newline character to the dev_err message
when an error occurs disabling the alarm.

> diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> index b72eef4fb0997..4cfe7034c5165 100644
> --- a/drivers/rtc/rtc-s35390a.c
> +++ b/drivers/rtc/rtc-s35390a.c
> @@ -494,7 +494,7 @@ static int s35390a_probe(struct i2c_client *client)
>  		buf = 0;
>  		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
>  		if (err < 0) {
> -			dev_err(dev, "error disabling alarm");
> +			dev_err(dev, "error disabling alarm\n");
>  			return err;
>  		}
>  	} else {

[Severity: High]
This is a pre-existing issue, but while looking at s35390a_probe(),
it seems err_read is not checked for a value of 1 when reading status:

drivers/rtc/rtc-s35390a.c:s35390a_probe() {
	err_read = s35390a_read_status(s35390a, &status1);
	if (err_read < 0) {
		dev_err(dev, "error resetting chip\n");
		return err_read;
	}

	if (status1 & S35390A_FLAG_24H)
		s35390a->twentyfourhour = 1;
	...

Does this mean the hardware power-loss flags are consumed and cleared without
preserving their state? Since s35390a_read_status() returns 1 if POC or BLD
flags are set, ignoring this means the driver may later return invalid time
to userspace without noticing the battery failed.

[Severity: High]
This isn't a bug introduced by this patch, but does s35390a_rtc_set_alarm()
incorrectly set the AM/PM bit when in 24-hour mode?

drivers/rtc/rtc-s35390a.c:s35390a_rtc_set_alarm() {
	...
	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
			alm->time.tm_hour) | 0x80;
	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;

	if (alm->time.tm_hour >= 12)
		buf[S35390A_ALRM_BYTE_HOURS] |= 0x40;
	...
}

If the RTC is in 24-hour mode, it uses 0-23 BCD format for the time register
and does not set the 0x40 bit for PM hours. Wouldn't setting the 0x40 bit
here unconditionally cause alarms for PM hours to silently fail to match?

[Severity: High]
This is a pre-existing issue, but does s35390a_init() fail to update the
internal state after changing the hardware to 24-hour mode?

drivers/rtc/rtc-s35390a.c:s35390a_init() {
	...
initialize:
	buf = S35390A_FLAG_RESET | S35390A_FLAG_24H;
	ret = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &buf, 1);
	...
}

If the hardware is forcibly put into 24-hour mode here, but
s35390a->twentyfourhour is not updated to 1, will subsequent time setting
operations mistakenly write 12-hour formatted time to the hardware?

[Severity: High]
This isn't a bug introduced by this patch, but is the alarm_irq_enable
callback missing from s35390a_rtc_ops?

drivers/rtc/rtc-s35390a.c:
static const struct rtc_class_ops s35390a_rtc_ops = {
	.read_time	= s35390a_rtc_read_time,
	.set_time	= s35390a_rtc_set_time,
	.set_alarm	= s35390a_rtc_set_alarm,
	.read_alarm	= s35390a_rtc_read_alarm,
	.ioctl          = s35390a_rtc_ioctl,
};

Without an alarm_irq_enable function pointer, won't rtc_alarm_disable()
become a no-op, preventing the kernel from disabling the hardware alarm
interrupt?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815-rtc_s35390a_int1-v4-0-3133309c365f@posteo.de?part=2