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

[Ronan Dalton]

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.