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

[Chris Packham]

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@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-rtc@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>
> Cc: Tyler Hicks <code@xxxxxxxxxxx>
> Cc: Sasha Levin <sashal@xxxxxxxxxx>
> Cc: Meagan Lloyd <meaganlloyd@xxxxxxxxxxxxxxxxxxx>
> Cc: Rodolfo Giometti <giometti@xxxxxxxxxxxx>
> Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> Fixes: 6cb0d8587b96 ("rtc: ds1307: remove clear of oscillator stop flag (OSF) in probe")

Reviewed-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>

> ---
>   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);