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

[Markus Probst]

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.

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.

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.

Thanks
- Markus Probst

Attachment: signature.asc
Description: This is a digitally signed message part