Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231

[Meagan Lloyd <meaganlloyd@linux.microsoft.com>]

Hi Ronan,

On Tue, May 05, 2026 at 10:24:55PM +0000, Ronan Dalton wrote:
> Hi Meagan,
> 
> On Tue, 2026-05-05 at 12:20 -0700, Meagan Lloyd wrote:
> > Hi Ronan,
> > 
> > On Mon, May 04, 2026 at 11:54:32PM +0000, Ronan Dalton wrote:
> > > Hi Meagan,
> > > 
> > > On Mon, 2026-05-04 at 15:08 -0700, Meagan Lloyd wrote:
> > > > > +       case ds_1337:
> > > > > +       case ds_1339:
> > > > > +       case ds_1341:
> > > > > +       case ds_3231:
> > > > > +               ret = regmap_read(ds1307->regmap,
> > > > > DS1337_REG_STATUS, &tmp);
> > > > > +               if (ret)
> > > > > +                       return ret;
> > > > > +               if (tmp & DS1337_BIT_OSF)
> > > > > +                       return -EINVAL;
> > > > > +               break;
> > > > 
> > > > If you're going to re-arrange the block to be in somewhat of an
> > > > order,
> > > > perhaps put it above 1338 since 1337 < 1338.
> > > 
> > > I've ordered it this way based on the first case statement in each
> > > block. Since ds_1337 > ds_1308, I've put the block below the block
> > > starting with ds_1308. I could instead order it based on the last
> > > case
> > > statement in each block, if you think that's better.
> > 
> > I agree with your ordering strategy, but your patch inserts it after
> > the
> > ds_1338 case statement block (rather than the intended ds_1308).
> > 
> 
> Here's how it currently is in ds1307_get_time:
> 
> 	case ds_1308:
> 	case ds_1338:
> 		[statement block #1]
> 	case ds_1337:
> 	case ds_1339:
> 	case ds_1341:
> 	case ds_3231:
> 		[statement block #2]
> 
> To insert statement block #2 after the ds_1308 case label would involve
> the following:
> 
> 	case ds_1308:
> 		[statement block #1]
> 	case ds_1337:
> 	case ds_1339:
> 	case ds_1341:
> 	case ds_3231:
> 		[statement block #2]
> 	case ds_1338:
> 		[statement block #1, duplicated]
> 
> Or the following with strict order:
> 
> 	case ds_1308:
> 		[statement block #1]
> 	case ds_1337:
> 		[statement block #2]
> 	case ds_1338:
> 		[statement block #1, duplicated]
> 	case ds_1339:
> 	case ds_1341:
> 	case ds_3231:
> 		[statement block #2, duplicated]
> 
> The case statements can be put strictly in order, but that will involve
> some duplication.

You are absolutely right. I missed that ds_1308 and ds_1338 share a
statement block. No need to change anything, thanks for the clear
explanation!

Reviewed-by: Meagan Lloyd <meaganlloyd@linux.microsoft.com>