Re: [PATCH v5 4/6] rtc: s35390a: force 24-hour mode

[Alexandre Belloni]

On 20/08/2026 01:26:34+0200, Alexandre Belloni wrote:
> On 19/08/2026 22:57:13+0000, Markus Probst wrote:
> > On Thu, 2026-08-20 at 00:20 +0200, Alexandre Belloni wrote:
> > > On 19/08/2026 22:05:56+0000, Markus Probst wrote:
> > > > Instead of trying to remember the current 12/24-hour mode, which could
> > > > result in time corruption if out of sync, we force the use of the 24-hour
> > > > mode. Set to 24-hour mode if in 12-hour mode on startup and assume
> > > > 24-hour mode. The 24-hour mode is already forced on reset.
> > > > 
> > > > Signed-off-by: Markus Probst <markus.probst@xxxxxxxxx>
> > > > ---
> > > >  drivers/rtc/rtc-s35390a.c | 67 +++++++++++++++++++++--------------------------
> > > >  1 file changed, 30 insertions(+), 37 deletions(-)
> > > > 
> > > > diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
> > > > index 575bb256eb25..b34a3f7e8476 100644
> > > > --- a/drivers/rtc/rtc-s35390a.c
> > > > +++ b/drivers/rtc/rtc-s35390a.c
> > > > @@ -64,7 +64,6 @@ MODULE_DEVICE_TABLE(of, s35390a_of_match);
> > > >  
> > > >  struct s35390a {
> > > >  	struct i2c_client *client[8];
> > > > -	int twentyfourhour;
> > > >  };
> > > >  
> > > >  static int s35390a_set_reg(struct s35390a *s35390a, int reg, u8  *buf, int len)
> > > > @@ -181,31 +180,6 @@ static int s35390a_disable_test_mode(struct s35390a *s35390a)
> > > >  	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, buf, sizeof(buf));
> > > >  }
> > > >  
> > > > -static char s35390a_hr2reg(struct s35390a *s35390a, int hour)
> > > > -{
> > > > -	if (s35390a->twentyfourhour)
> > > > -		return bin2bcd(hour);
> > > > -
> > > > -	if (hour < 12)
> > > > -		return bin2bcd(hour);
> > > > -
> > > > -	return 0x40 | bin2bcd(hour - 12);
> > > > -}
> > > > -
> > > > -static int s35390a_reg2hr(struct s35390a *s35390a, char reg)
> > > > -{
> > > > -	unsigned hour;
> > > > -
> > > > -	if (s35390a->twentyfourhour)
> > > > -		return bcd2bin(reg & 0x3f);
> > > > -
> > > > -	hour = bcd2bin(reg & 0x3f);
> > > > -	if (reg & 0x40)
> > > > -		hour += 12;
> > > > -
> > > > -	return hour;
> > > > -}
> > > > -
> > > >  static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > >  {
> > > >  	struct i2c_client *client = to_i2c_client(dev);
> > > > @@ -225,7 +199,7 @@ static int s35390a_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > >  	buf[S35390A_BYTE_MONTH] = bin2bcd(tm->tm_mon + 1);
> > > >  	buf[S35390A_BYTE_DAY] = bin2bcd(tm->tm_mday);
> > > >  	buf[S35390A_BYTE_WDAY] = bin2bcd(tm->tm_wday);
> > > > -	buf[S35390A_BYTE_HOURS] = s35390a_hr2reg(s35390a, tm->tm_hour);
> > > > +	buf[S35390A_BYTE_HOURS] = bin2bcd(tm->tm_hour);
> > > >  	buf[S35390A_BYTE_MINS] = bin2bcd(tm->tm_min);
> > > >  	buf[S35390A_BYTE_SECS] = bin2bcd(tm->tm_sec);
> > > >  
> > > > @@ -256,7 +230,7 @@ static int s35390a_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > >  
> > > >  	tm->tm_sec = bcd2bin(buf[S35390A_BYTE_SECS]);
> > > >  	tm->tm_min = bcd2bin(buf[S35390A_BYTE_MINS]);
> > > > -	tm->tm_hour = s35390a_reg2hr(s35390a, buf[S35390A_BYTE_HOURS]);
> > > > +	tm->tm_hour = bcd2bin(buf[S35390A_BYTE_HOURS] & 0x3f);
> > > >  	tm->tm_wday = bcd2bin(buf[S35390A_BYTE_WDAY]);
> > > >  	tm->tm_mday = bcd2bin(buf[S35390A_BYTE_DAY]);
> > > >  	tm->tm_mon = bcd2bin(buf[S35390A_BYTE_MONTH]) - 1;
> > > > @@ -325,8 +299,7 @@ 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] = bin2bcd(alm->time.tm_hour) | 0x80;
> > > >  	buf[S35390A_ALRM_BYTE_MINS] = bin2bcd(alm->time.tm_min) | 0x80;
> > > >  
> > > >  	if (alm->time.tm_hour >= 12)
> > > > @@ -381,8 +354,7 @@ static int s35390a_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > > >  
> > > >  	if (buf[S35390A_ALRM_BYTE_HOURS] & 0x80)
> > > >  		alm->time.tm_hour =
> > > > -			s35390a_reg2hr(s35390a,
> > > > -				       buf[S35390A_ALRM_BYTE_HOURS] & ~0x80);
> > > > +			bcd2bin(buf[S35390A_ALRM_BYTE_HOURS] & 0x3f);
> > > >  
> > > >  	if (buf[S35390A_ALRM_BYTE_MINS] & 0x80)
> > > >  		alm->time.tm_min = bcd2bin(buf[S35390A_ALRM_BYTE_MINS] & ~0x80);
> > > > @@ -459,7 +431,7 @@ static int s35390a_probe(struct i2c_client *client)
> > > >  	unsigned int i;
> > > >  	struct s35390a *s35390a;
> > > >  	struct rtc_device *rtc;
> > > > -	u8 buf, status1;
> > > > +	u8 buf, status1, time[7];
> > > >  	struct device *dev = &client->dev;
> > > >  	struct nvmem_config nvmem_cfg = {
> > > >  		.name = "s35390a_nvram",
> > > > @@ -503,10 +475,31 @@ static int s35390a_probe(struct i2c_client *client)
> > > >  		return err_read;
> > > >  	}
> > > >  
> > > > -	if (status1 & S35390A_FLAG_24H)
> > > > -		s35390a->twentyfourhour = 1;
> > > > -	else
> > > > -		s35390a->twentyfourhour = 0;
> > > > +	if (!(status1 & S35390A_FLAG_24H) && err_read != 1) {
> > > > +		unsigned int hour;
> > > > +
> > > > +		status1 |= S35390A_FLAG_24H;
> > > > +
> > > > +		err = s35390a_get_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> > > > +		if (err < 0)
> > > > +			return dev_err_probe(dev, err, "reading 12-hour time failed\n");
> > > > +
> > > > +		/* converting to 24-hour time */
> > > > +		time[S35390A_BYTE_HOURS] = bitrev8(time[S35390A_BYTE_HOURS]);
> > > > +		hour = bcd2bin(time[S35390A_BYTE_HOURS] & 0x3f);
> > > > +		if (time[S35390A_BYTE_HOURS] & 0x40)
> > > > +			hour += 12;
> > > > +
> > > > +		time[S35390A_BYTE_HOURS] = bitrev8(bin2bcd(hour));
> > > > +
> > > > +		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &status1, sizeof(status1));
> > > > +		if (err < 0)
> > > > +			return dev_err_probe(dev, err, "setting 24-hour mode failed\n");
> > > > +
> > > > +		err = s35390a_set_reg(s35390a, S35390A_CMD_TIME1, time, sizeof(time));
> > > > +		if (err < 0)
> > > > +			return dev_err_probe(dev, err, "setting 24-hour time failed\n");
> > > 
> > > No, don't do this, this will break existing users as your conversion may
> > > happen after a minute rollover. The only function in which you are
> > > allowed to change the time registers is set_time. You definitively need
> > > to be able to read 12H mode time but you are allowed to only support
> > > writing 24h time.
> > > 
> > > Also, this is super verbose, adding strings to the kernel makes it
> > > bloated.
> > > 
> > > To avoid any desync, simply test for the S35390A_FLAG_24H bit in status
> > > in s35390a_rtc_read_time as the register is already read anyway.
> > > 
> > I think that would defeat the whole point of the patch, that the rtc is
> > always in 24-hour mode. If this driver was always used with the rtc, it
> > already should be in 24-hour mode, as thats the mode this driver does
> > initialize the rtc.
> 
> This assumption may not be true, the RTC may have been initialized by a
> bootloader or a different system in the factory.
> 
> > Your idea would work for the time, but not in combination with alarms.
> > If the rtc could be in 12-hour mode, then this would need to be
> > considered in setting and reading alarm. Also setting 24-hour mode
> > inside set_time would then also require to convert the alarm time.
> > Effectively making it less maintainable than before.
> 
> Note that while setting the time when an alarm has been set is not
> forbidden, it is never guaranteed to work anyway. I guess something we
> could do in the core is that whenever the time is set we go over the
> timerqueue and set the alarm again.

Actually this is already what we do so this is not an issue.


>This would allow to remove the UIE
> disabling/enabling dance.
> 
> > 
> > Currently s35390a_init does initialize the rtc in 24-hour mode, while
> > not updating s35390a->twentyfourhour. So some change needs to be made.
> > Just setting s35390a->twentyfourhour inside s35390a_init might now be
> > the simplest solution here.
> 
> Or maybe simply stop caching the value.
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com