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

Ronan Dalton posted 1 patch 1 month, 2 weeks ago
There is a newer version of this series
drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
1 file changed, 17 insertions(+), 11 deletions(-)
[PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Ronan Dalton 1 month, 2 weeks ago
Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator
stop flag (OSF) in probe"), the oscillator stop flag (OSF) bit was
checked during device probe for the ds1337, ds1339, ds1341, and ds3231
chips; if it was set, it would be cleared and a warning would be logged
saying "SET TIME!". Since that commit, the OSF bit is no longer cleared,
but the warning is still printed.

Directly following that commit, there was no way to get rid of this
warning because nothing cleared the OSF bit on these chips.

The commit associated with the previous commit, ae03a28e12a7 ("rtc:
ds1307: handle oscillator stop flag (OSF) for ds1341"), made proper use
of the OSF when getting and setting the time in the RTC. However, the
other RTC variants ds1337, ds1339 and ds3231 didn't have a corresponding
change made.

Given that the OSF bit is no longer cleared at probe time when it is
set, the remaining three chips should have the same handling as the
ds1341 chip has for the OSF bit.

Fix the issue on the ds1337, ds1339 and ds3231 chips by applying the
same logic as the ds1341 has to these chips.

Note that any devices brought up between the first referenced commit and
this one may begin mistrusting the time reported by the RTC until it is
set again, if the bit was never explicitly cleared.

Note that only the ds1339 was tested with this change, but the
datasheets for the other chips contain essentially identical
descriptions of the OSF bit so the same change should work.

An alternative to this change could be just to revert the referenced two
commits and not use the OSF bit at all, apart from logging a warning and
clearing it on probe.

Signed-off-by: Ronan Dalton <ronan.dalton@alliedtelesis.co.nz>
Cc: linux-rtc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Tyler Hicks <code@tyhicks.com>
Cc: Sasha Levin <sashal@kernel.org>
Cc: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
Cc: Rodolfo Giometti <giometti@enneenne.com>
Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")
---
 drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 7205c59ff729..edf81b975dec 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -269,6 +269,16 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 		if (tmp & DS1338_BIT_OSF)
 			return -EINVAL;
 		break;
+	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;
 	case ds_1340:
 		if (tmp & DS1340_BIT_nEOSC)
 			return -EINVAL;
@@ -279,13 +289,6 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 		if (tmp & DS1340_BIT_OSF)
 			return -EINVAL;
 		break;
-	case ds_1341:
-		ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
-		if (ret)
-			return ret;
-		if (tmp & DS1337_BIT_OSF)
-			return -EINVAL;
-		break;
 	case ds_1388:
 		ret = regmap_read(ds1307->regmap, DS1388_REG_FLAG, &tmp);
 		if (ret)
@@ -380,14 +383,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 		regmap_update_bits(ds1307->regmap, DS1307_REG_CONTROL,
 				   DS1338_BIT_OSF, 0);
 		break;
+	case ds_1337:
+	case ds_1339:
+	case ds_1341:
+	case ds_3231:
+		regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
+				   DS1337_BIT_OSF, 0);
+		break;
 	case ds_1340:
 		regmap_update_bits(ds1307->regmap, DS1340_REG_FLAG,
 				   DS1340_BIT_OSF, 0);
 		break;
-	case ds_1341:
-		regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
-				   DS1337_BIT_OSF, 0);
-		break;
 	case ds_1388:
 		regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
 				   DS1388_BIT_OSF, 0);
-- 
2.53.0
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Meagan Lloyd 1 month, 1 week ago
On 4/30/2026 9:46 PM, Ronan Dalton wrote:
> An alternative to this change could be just to revert the referenced two
> commits and not use the OSF bit at all, apart from logging a warning and
> clearing it on probe.
Can you remove this from the commit message?
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Ronan Dalton 1 month, 1 week ago
On Thu, 2026-05-07 at 19:09 -0700, Meagan Lloyd wrote:
> 
> On 4/30/2026 9:46 PM, Ronan Dalton wrote:
> > An alternative to this change could be just to revert the
> > referenced two
> > commits and not use the OSF bit at all, apart from logging a
> > warning and
> > clearing it on probe.
> Can you remove this from the commit message?

Done.
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Tyler Hicks 1 month, 1 week ago
On 2026-05-01 16:46:10, Ronan Dalton wrote:
> Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator

This commit hash is from the linux-6.12.y stable branch but we should
use hashes from Linus' tree in this commit message:

 48458654659c ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")

> stop flag (OSF) in probe"), the oscillator stop flag (OSF) bit was
> checked during device probe for the ds1337, ds1339, ds1341, and ds3231
> chips; if it was set, it would be cleared and a warning would be logged
> saying "SET TIME!". Since that commit, the OSF bit is no longer cleared,
> but the warning is still printed.
> 
> Directly following that commit, there was no way to get rid of this
> warning because nothing cleared the OSF bit on these chips.
> 
> The commit associated with the previous commit, ae03a28e12a7 ("rtc:

The commit hash referenced here should be 523923cfd5d6.

> ds1307: handle oscillator stop flag (OSF) for ds1341"), made proper use
> of the OSF when getting and setting the time in the RTC. However, the
> other RTC variants ds1337, ds1339 and ds3231 didn't have a corresponding
> change made.
> 
> Given that the OSF bit is no longer cleared at probe time when it is
> set, the remaining three chips should have the same handling as the
> ds1341 chip has for the OSF bit.
> 
> Fix the issue on the ds1337, ds1339 and ds3231 chips by applying the
> same logic as the ds1341 has to these chips.
> 
> Note that any devices brought up between the first referenced commit and
> this one may begin mistrusting the time reported by the RTC until it is
> set again, if the bit was never explicitly cleared.
> 
> Note that only the ds1339 was tested with this change, but the
> datasheets for the other chips contain essentially identical
> descriptions of the OSF bit so the same change should work.
> 
> An alternative to this change could be just to revert the referenced two
> commits and not use the OSF bit at all, apart from logging a warning and
> clearing it on probe.
> 
> Signed-off-by: Ronan Dalton <ronan.dalton@alliedtelesis.co.nz>
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Tyler Hicks <code@tyhicks.com>
> Cc: Sasha Levin <sashal@kernel.org>
> Cc: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
> Cc: Rodolfo Giometti <giometti@enneenne.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")

Please adjust the commit hash here, as well. Everything else looks good.
Thanks!

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

Tyler

> ---
>  drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 7205c59ff729..edf81b975dec 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -269,6 +269,16 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>  		if (tmp & DS1338_BIT_OSF)
>  			return -EINVAL;
>  		break;
> +	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;
>  	case ds_1340:
>  		if (tmp & DS1340_BIT_nEOSC)
>  			return -EINVAL;
> @@ -279,13 +289,6 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>  		if (tmp & DS1340_BIT_OSF)
>  			return -EINVAL;
>  		break;
> -	case ds_1341:
> -		ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> -		if (ret)
> -			return ret;
> -		if (tmp & DS1337_BIT_OSF)
> -			return -EINVAL;
> -		break;
>  	case ds_1388:
>  		ret = regmap_read(ds1307->regmap, DS1388_REG_FLAG, &tmp);
>  		if (ret)
> @@ -380,14 +383,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>  		regmap_update_bits(ds1307->regmap, DS1307_REG_CONTROL,
>  				   DS1338_BIT_OSF, 0);
>  		break;
> +	case ds_1337:
> +	case ds_1339:
> +	case ds_1341:
> +	case ds_3231:
> +		regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> +				   DS1337_BIT_OSF, 0);
> +		break;
>  	case ds_1340:
>  		regmap_update_bits(ds1307->regmap, DS1340_REG_FLAG,
>  				   DS1340_BIT_OSF, 0);
>  		break;
> -	case ds_1341:
> -		regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> -				   DS1337_BIT_OSF, 0);
> -		break;
>  	case ds_1388:
>  		regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
>  				   DS1388_BIT_OSF, 0);
> -- 
> 2.53.0
>
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Ronan Dalton 1 month, 1 week ago
On Thu, 2026-05-07 at 10:19 -0500, Tyler Hicks wrote:
> On 2026-05-01 16:46:10, Ronan Dalton wrote:
> > Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of
> > oscillator
> 
> This commit hash is from the linux-6.12.y stable branch but we should
> use hashes from Linus' tree in this commit message:
> 
>  48458654659c ("rtc: ds1307: remove clear of oscillator stop flag
> (OSF) in probe")
> > 
> > The commit associated with the previous commit, ae03a28e12a7 ("rtc:
> 
> The commit hash referenced here should be 523923cfd5d6.
> > 
> > Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop
> > flag (OSF) in probe")
> 
> Please adjust the commit hash here, as well. Everything else looks
> good.

Good catch. I'll send a follow-up patch with these commit hashes fixed.

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

Cheers
> 
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Meagan Lloyd 1 month, 1 week ago
Hi Ronan -

On Fri, May 01, 2026 at 04:46:10PM +1200, Ronan Dalton wrote:
> Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator
> stop flag (OSF) in probe"), the oscillator stop flag (OSF) bit was
> checked during device probe for the ds1337, ds1339, ds1341, and ds3231
> chips; if it was set, it would be cleared and a warning would be logged
> saying "SET TIME!". Since that commit, the OSF bit is no longer cleared,
> but the warning is still printed.
> 
> Directly following that commit, there was no way to get rid of this
> warning because nothing cleared the OSF bit on these chips.
> 
> The commit associated with the previous commit, ae03a28e12a7 ("rtc:
> ds1307: handle oscillator stop flag (OSF) for ds1341"), made proper use
> of the OSF when getting and setting the time in the RTC. However, the
> other RTC variants ds1337, ds1339 and ds3231 didn't have a corresponding
> change made.
> 
> Given that the OSF bit is no longer cleared at probe time when it is
> set, the remaining three chips should have the same handling as the
> ds1341 chip has for the OSF bit.
> 
> Fix the issue on the ds1337, ds1339 and ds3231 chips by applying the
> same logic as the ds1341 has to these chips.
> 
> Note that any devices brought up between the first referenced commit and
> this one may begin mistrusting the time reported by the RTC until it is
> set again, if the bit was never explicitly cleared.
> 
> Note that only the ds1339 was tested with this change, but the
> datasheets for the other chips contain essentially identical
> descriptions of the OSF bit so the same change should work.
> 

Thanks for finding and fixing this issue. I agree that the kernel logging
"SET TIME!" on every boot after the OSF gets set is unwanted behavior.

The original series was backported, so we will probably want this fix
backported too.

> An alternative to this change could be just to revert the referenced two
> commits and not use the OSF bit at all, apart from logging a warning and
> clearing it on probe.

The OSF handling fixed a real problem and is in use by a number of other
chip types in the driver, so I think we should keep the commits.

> 
> Signed-off-by: Ronan Dalton <ronan.dalton@alliedtelesis.co.nz>
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Tyler Hicks <code@tyhicks.com>
> Cc: Sasha Levin <sashal@kernel.org>
> Cc: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
> Cc: Rodolfo Giometti <giometti@enneenne.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")
> ---
>  drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 7205c59ff729..edf81b975dec 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -269,6 +269,16 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>  		if (tmp & DS1338_BIT_OSF)
>  			return -EINVAL;
>  		break;
> +	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.

>  	case ds_1340:
>  		if (tmp & DS1340_BIT_nEOSC)
>  			return -EINVAL;
> @@ -279,13 +289,6 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>  		if (tmp & DS1340_BIT_OSF)
>  			return -EINVAL;
>  		break;
> -	case ds_1341:
> -		ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> -		if (ret)
> -			return ret;
> -		if (tmp & DS1337_BIT_OSF)
> -			return -EINVAL;
> -		break;
>  	case ds_1388:
>  		ret = regmap_read(ds1307->regmap, DS1388_REG_FLAG, &tmp);
>  		if (ret)
> @@ -380,14 +383,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>  		regmap_update_bits(ds1307->regmap, DS1307_REG_CONTROL,
>  				   DS1338_BIT_OSF, 0);
>  		break;
> +	case ds_1337:
> +	case ds_1339:
> +	case ds_1341:
> +	case ds_3231:
> +		regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> +				   DS1337_BIT_OSF, 0);
> +		break;

Same here

>  	case ds_1340:
>  		regmap_update_bits(ds1307->regmap, DS1340_REG_FLAG,
>  				   DS1340_BIT_OSF, 0);
>  		break;
> -	case ds_1341:
> -		regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> -				   DS1337_BIT_OSF, 0);
> -		break;
>  	case ds_1388:
>  		regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
>  				   DS1388_BIT_OSF, 0);
> -- 
> 2.53.0
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Ronan Dalton 1 month, 1 week ago
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.
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Meagan Lloyd 1 month, 1 week ago
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).
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Ronan Dalton 1 month, 1 week ago
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.
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Meagan Lloyd 1 month, 1 week ago
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>
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Ronan Dalton 1 month, 1 week ago
On Tue, 2026-05-05 at 18:41 -0700, Meagan Lloyd wrote:
> Reviewed-by: Meagan Lloyd <meaganlloyd@linux.microsoft.com>

Thanks!
Re: [PATCH] rtc: ds1307: handle oscillator stop flag for ds1337/ds1339/ds3231
Posted by Chris Packham 1 month, 2 weeks ago
Hi Ronan

On 01/05/2026 16:46, Ronan Dalton wrote:
> Prior to commit 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator
> stop flag (OSF) in probe"), the oscillator stop flag (OSF) bit was
> checked during device probe for the ds1337, ds1339, ds1341, and ds3231
> chips; if it was set, it would be cleared and a warning would be logged
> saying "SET TIME!". Since that commit, the OSF bit is no longer cleared,
> but the warning is still printed.
>
> Directly following that commit, there was no way to get rid of this
> warning because nothing cleared the OSF bit on these chips.
>
> The commit associated with the previous commit, ae03a28e12a7 ("rtc:
> ds1307: handle oscillator stop flag (OSF) for ds1341"), made proper use
> of the OSF when getting and setting the time in the RTC. However, the
> other RTC variants ds1337, ds1339 and ds3231 didn't have a corresponding
> change made.
>
> Given that the OSF bit is no longer cleared at probe time when it is
> set, the remaining three chips should have the same handling as the
> ds1341 chip has for the OSF bit.
>
> Fix the issue on the ds1337, ds1339 and ds3231 chips by applying the
> same logic as the ds1341 has to these chips.
>
> Note that any devices brought up between the first referenced commit and
> this one may begin mistrusting the time reported by the RTC until it is
> set again, if the bit was never explicitly cleared.
>
> Note that only the ds1339 was tested with this change, but the
> datasheets for the other chips contain essentially identical
> descriptions of the OSF bit so the same change should work.
>
> An alternative to this change could be just to revert the referenced two
> commits and not use the OSF bit at all, apart from logging a warning and
> clearing it on probe.
>
> Signed-off-by: Ronan Dalton <ronan.dalton@alliedtelesis.co.nz>
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Tyler Hicks <code@tyhicks.com>
> Cc: Sasha Levin <sashal@kernel.org>
> Cc: Meagan Lloyd <meaganlloyd@linux.microsoft.com>
> Cc: Rodolfo Giometti <giometti@enneenne.com>
> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")

Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

> ---
>   drivers/rtc/rtc-ds1307.c | 28 +++++++++++++++++-----------
>   1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 7205c59ff729..edf81b975dec 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -269,6 +269,16 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>   		if (tmp & DS1338_BIT_OSF)
>   			return -EINVAL;
>   		break;
> +	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;
>   	case ds_1340:
>   		if (tmp & DS1340_BIT_nEOSC)
>   			return -EINVAL;
> @@ -279,13 +289,6 @@ static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>   		if (tmp & DS1340_BIT_OSF)
>   			return -EINVAL;
>   		break;
> -	case ds_1341:
> -		ret = regmap_read(ds1307->regmap, DS1337_REG_STATUS, &tmp);
> -		if (ret)
> -			return ret;
> -		if (tmp & DS1337_BIT_OSF)
> -			return -EINVAL;
> -		break;
>   	case ds_1388:
>   		ret = regmap_read(ds1307->regmap, DS1388_REG_FLAG, &tmp);
>   		if (ret)
> @@ -380,14 +383,17 @@ static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>   		regmap_update_bits(ds1307->regmap, DS1307_REG_CONTROL,
>   				   DS1338_BIT_OSF, 0);
>   		break;
> +	case ds_1337:
> +	case ds_1339:
> +	case ds_1341:
> +	case ds_3231:
> +		regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> +				   DS1337_BIT_OSF, 0);
> +		break;
>   	case ds_1340:
>   		regmap_update_bits(ds1307->regmap, DS1340_REG_FLAG,
>   				   DS1340_BIT_OSF, 0);
>   		break;
> -	case ds_1341:
> -		regmap_update_bits(ds1307->regmap, DS1337_REG_STATUS,
> -				   DS1337_BIT_OSF, 0);
> -		break;
>   	case ds_1388:
>   		regmap_update_bits(ds1307->regmap, DS1388_REG_FLAG,
>   				   DS1388_BIT_OSF, 0);