Message-ID: <aGVHkgDaWN83X9hA@redbud>
Date: Wed, 2 Jul 2025 09:52:02 -0500
From: Tyler Hicks <code@...icks.com>
To: Rodolfo Giometti <giometti@...eenne.com>, alexandre.belloni@...tlin.com
Cc: Meagan Lloyd <meaganlloyd@...ux.microsoft.com>,
	linux-rtc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] Expand oscillator stop flag (OSF) validity check to
 ds1341

On 2025-07-02 16:37:41, Rodolfo Giometti wrote:
> On 01/07/25 00:54, Tyler Hicks wrote:
> > [Adding Rodolfo Giometti]
> > 
> > On 2025-06-11 11:14:14, Meagan Lloyd wrote:
> > > We would like to use CONFIG_RTC_HCTOSYS to sync a supercapacitor-backed
> > > DS1342 RTC to the kernel time early in boot. An obstacle is that the
> > > sync in rtc_hctosys() is unconditional as long as rtc_read_time()
> > > succeeds and in some power loss situations, our RTC comes up with either
> > > an unpredictable future time or the default 01/01/00 from the datasheet.
> > > Syncing a future time, followed by an NTP sync would not be desired as
> > > it would result in a backwards time jump. The sync feature is useful in
> > > boot scenarios where power is maintained so syncing only when the RTC
> > > data is valid would allow us to make use of the feature.
> > > 
> > > The DS1342 has the oscillator stop flag (OSF) which is a status flag
> > > indicating that the oscillator stopped for a period of time. It can be
> > > set due to power loss. Some chip types in the ds1307 driver already use
> > > the OSF to determine whether .read_time should provide valid data or
> > > return -EINVAL. This patch series expands that handling to the ds1341
> > > chip type (DS1341 and DS1342 share a datasheet).
> > > 
> > > These changes enable us to make use of CONFIG_RTC_HCTOSYS as they
> > > prevent the invalid time from getting synced to the kernel time. It will
> > > also prevent userspace programs from getting the invalid time as the fix
> > > cuts it off at the source - the .read_time function.
> > 
> > These two patches look good to me, although I'm not an expert in RTC drivers.
> > I've reviewed the DS1341/DS1342 datasheet and the approach that Meagan has
> > taken makes sense to me given our (Meagan and I work together) desire to use
> > CONFIG_RTC_HCTOSYS and the need to avoid syncing from an invalid RTC state.
> > 
> > I've added Rodolfo because he first added the logic to clear the Oscillator
> > Stop Flag, during driver initialization, way back in 2007 with v2.6.23 commit
> > be5f59f4b67f ("rtc-ds1307: oscillator restart for ds13{37,38,39,40}") and may
> > have additional context to provide.
> > 
> > Alexandre and Rodolfo, does this approach make sense to you? If not, do you
> > have any other suggestions on how to make CONFIG_RTC_HCTOSYS work with this
> > driver? Thanks!
> 
> They look good to me. You can add my Acked-by line to all of them:
> 
>     Acked-by: Rodolfo Giometti <giometti@...eenne.com>

Thanks for taking a look!

I should have formally given my Reviewed-by tag for both patches earlier in the thread:

  Reviewed-by: Tyler Hicks <code@...icks.com>

Tyler

> 
> Rodolfo
> 
> > Tyler
> > 
> > > 
> > > Meagan Lloyd (2):
> > >    rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe
> > >    rtc: ds1307: handle oscillator stop flag (OSF) for ds1341
> > > 
> > >   drivers/rtc/rtc-ds1307.c | 15 ++++++++++++---
> > >   1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > 
> > > base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> > > -- 
> > > 2.49.0
> > > 
> 
> -- 
> GNU/Linux Solutions                  e-mail: giometti@...eenne.com
> Linux Device Driver                          giometti@...ux.it
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming
> 

Powered by blists - more mailing lists