linux-rtc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] rtc: abx80x: Don't warn about oscillator failure after PoR
@ 2023-10-19 16:39 Sean Anderson
  2023-12-11 16:03 ` Sean Anderson
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Anderson @ 2023-10-19 16:39 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: linux-kernel, Sean Anderson

According to the datasheet, the "oscillator failure" bit is set

> ...on a power on reset, when both the system and battery voltages have
> dropped below acceptable levels. It is also set if an Oscillator Failure
> occurs....

From testing, this bit is also set if a software reset is initiated.

This bit has a confusing name; it really tells us whether the time data
is valid. We clear it when writing the time. If it is still set, that
means there is a persistent issue (such as an oscillator failure),
instead of a transient one (such as power loss).

Because there are several other reasons which might cause this bit
to be set (including booting for the first time or a battery failure),
do not warn about oscillator failures willy-nilly. This may cause system
integrators to waste time looking into the wrong line of investigation.

We continue printing a message about invalid time data or an oscillator
failure. There is no voltimeter in this RTC, so this is the best
indication that the battery is dead (or dying) and reeds replacement.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
Note that the following drivers all warn when they detect a problem with
the oscillator:

drivers/rtc/rtc-ds1672.c
drivers/rtc/rtc-pcf*.c
drivers/rtc/rtc-rs5c*.c
drivers/rtc/rtc-sc27xx.c

So warning about such an error has good precedent.

Changes in v3:
- Use info since this is a good indication of a battery failure

Changes in v2:
- Use debug instead of info in the typical case (no battery)

 drivers/rtc/rtc-abx80x.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
index fde2b8054c2e..f463a58a240b 100644
--- a/drivers/rtc/rtc-abx80x.c
+++ b/drivers/rtc/rtc-abx80x.c
@@ -127,6 +127,7 @@ struct abx80x_priv {
 	struct rtc_device *rtc;
 	struct i2c_client *client;
 	struct watchdog_device wdog;
+	bool wrote_time;
 };
 
 static int abx80x_write_config_key(struct i2c_client *client, u8 key)
@@ -179,6 +180,7 @@ static int abx80x_enable_trickle_charger(struct i2c_client *client,
 static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	unsigned char buf[8];
 	int err, flags, rc_mode = 0;
 
@@ -193,7 +195,18 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 			return flags;
 
 		if (flags & ABX8XX_OSS_OF) {
-			dev_err(dev, "Oscillator failure, data is invalid.\n");
+			/*
+			 * The OF bit can be set either because of a reset
+			 * (PoR/Software reset) or because of an oscillator
+			 * failure. Effectively, it indicates that the stored
+			 * time is invalid. When we write the time, we clear
+			 * this bit. If it stays set, then this indicates an
+			 * oscillator failure.
+			 */
+			if (priv->wrote_time)
+				dev_err(dev, "Oscillator failure\n");
+			else
+				dev_info(dev, "Time data invalid\n");
 			return -EINVAL;
 		}
 	}
@@ -219,6 +232,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct abx80x_priv *priv = i2c_get_clientdata(client);
 	unsigned char buf[8];
 	int err, flags;
 
@@ -252,6 +266,7 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		dev_err(&client->dev, "Unable to write oscillator status register\n");
 		return err;
 	}
+	priv->wrote_time = true;
 
 	return 0;
 }
-- 
2.35.1.1320.gc452695387.dirty


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] rtc: abx80x: Don't warn about oscillator failure after PoR
  2023-10-19 16:39 [PATCH v3] rtc: abx80x: Don't warn about oscillator failure after PoR Sean Anderson
@ 2023-12-11 16:03 ` Sean Anderson
  2024-01-08 22:05   ` Sean Anderson
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Anderson @ 2023-12-11 16:03 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc; +Cc: linux-kernel

On 10/19/23 12:39, Sean Anderson wrote:
> According to the datasheet, the "oscillator failure" bit is set
> 
>> ...on a power on reset, when both the system and battery voltages have
>> dropped below acceptable levels. It is also set if an Oscillator Failure
>> occurs....
> 
> From testing, this bit is also set if a software reset is initiated.
> 
> This bit has a confusing name; it really tells us whether the time data
> is valid. We clear it when writing the time. If it is still set, that
> means there is a persistent issue (such as an oscillator failure),
> instead of a transient one (such as power loss).
> 
> Because there are several other reasons which might cause this bit
> to be set (including booting for the first time or a battery failure),
> do not warn about oscillator failures willy-nilly. This may cause system
> integrators to waste time looking into the wrong line of investigation.
> 
> We continue printing a message about invalid time data or an oscillator
> failure. There is no voltimeter in this RTC, so this is the best
> indication that the battery is dead (or dying) and reeds replacement.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> Note that the following drivers all warn when they detect a problem with
> the oscillator:
> 
> drivers/rtc/rtc-ds1672.c
> drivers/rtc/rtc-pcf*.c
> drivers/rtc/rtc-rs5c*.c
> drivers/rtc/rtc-sc27xx.c
> 
> So warning about such an error has good precedent.
> 
> Changes in v3:
> - Use info since this is a good indication of a battery failure
> 
> Changes in v2:
> - Use debug instead of info in the typical case (no battery)
> 
>  drivers/rtc/rtc-abx80x.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index fde2b8054c2e..f463a58a240b 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -127,6 +127,7 @@ struct abx80x_priv {
>  	struct rtc_device *rtc;
>  	struct i2c_client *client;
>  	struct watchdog_device wdog;
> +	bool wrote_time;
>  };
>  
>  static int abx80x_write_config_key(struct i2c_client *client, u8 key)
> @@ -179,6 +180,7 @@ static int abx80x_enable_trickle_charger(struct i2c_client *client,
>  static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	struct abx80x_priv *priv = i2c_get_clientdata(client);
>  	unsigned char buf[8];
>  	int err, flags, rc_mode = 0;
>  
> @@ -193,7 +195,18 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  			return flags;
>  
>  		if (flags & ABX8XX_OSS_OF) {
> -			dev_err(dev, "Oscillator failure, data is invalid.\n");
> +			/*
> +			 * The OF bit can be set either because of a reset
> +			 * (PoR/Software reset) or because of an oscillator
> +			 * failure. Effectively, it indicates that the stored
> +			 * time is invalid. When we write the time, we clear
> +			 * this bit. If it stays set, then this indicates an
> +			 * oscillator failure.
> +			 */
> +			if (priv->wrote_time)
> +				dev_err(dev, "Oscillator failure\n");
> +			else
> +				dev_info(dev, "Time data invalid\n");
>  			return -EINVAL;
>  		}
>  	}
> @@ -219,6 +232,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	struct abx80x_priv *priv = i2c_get_clientdata(client);
>  	unsigned char buf[8];
>  	int err, flags;
>  
> @@ -252,6 +266,7 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  		dev_err(&client->dev, "Unable to write oscillator status register\n");
>  		return err;
>  	}
> +	priv->wrote_time = true;
>  
>  	return 0;
>  }

ping?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] rtc: abx80x: Don't warn about oscillator failure after PoR
  2023-12-11 16:03 ` Sean Anderson
@ 2024-01-08 22:05   ` Sean Anderson
  2024-01-25 17:14     ` Sean Anderson
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Anderson @ 2024-01-08 22:05 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc; +Cc: linux-kernel

On 12/11/23 11:03, Sean Anderson wrote:
> On 10/19/23 12:39, Sean Anderson wrote:
>> According to the datasheet, the "oscillator failure" bit is set
>> 
>>> ...on a power on reset, when both the system and battery voltages have
>>> dropped below acceptable levels. It is also set if an Oscillator Failure
>>> occurs....
>> 
>> From testing, this bit is also set if a software reset is initiated.
>> 
>> This bit has a confusing name; it really tells us whether the time data
>> is valid. We clear it when writing the time. If it is still set, that
>> means there is a persistent issue (such as an oscillator failure),
>> instead of a transient one (such as power loss).
>> 
>> Because there are several other reasons which might cause this bit
>> to be set (including booting for the first time or a battery failure),
>> do not warn about oscillator failures willy-nilly. This may cause system
>> integrators to waste time looking into the wrong line of investigation.
>> 
>> We continue printing a message about invalid time data or an oscillator
>> failure. There is no voltimeter in this RTC, so this is the best
>> indication that the battery is dead (or dying) and reeds replacement.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> Note that the following drivers all warn when they detect a problem with
>> the oscillator:
>> 
>> drivers/rtc/rtc-ds1672.c
>> drivers/rtc/rtc-pcf*.c
>> drivers/rtc/rtc-rs5c*.c
>> drivers/rtc/rtc-sc27xx.c
>> 
>> So warning about such an error has good precedent.
>> 
>> Changes in v3:
>> - Use info since this is a good indication of a battery failure
>> 
>> Changes in v2:
>> - Use debug instead of info in the typical case (no battery)
>> 
>>  drivers/rtc/rtc-abx80x.c | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
>> index fde2b8054c2e..f463a58a240b 100644
>> --- a/drivers/rtc/rtc-abx80x.c
>> +++ b/drivers/rtc/rtc-abx80x.c
>> @@ -127,6 +127,7 @@ struct abx80x_priv {
>>  	struct rtc_device *rtc;
>>  	struct i2c_client *client;
>>  	struct watchdog_device wdog;
>> +	bool wrote_time;
>>  };
>>  
>>  static int abx80x_write_config_key(struct i2c_client *client, u8 key)
>> @@ -179,6 +180,7 @@ static int abx80x_enable_trickle_charger(struct i2c_client *client,
>>  static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>  {
>>  	struct i2c_client *client = to_i2c_client(dev);
>> +	struct abx80x_priv *priv = i2c_get_clientdata(client);
>>  	unsigned char buf[8];
>>  	int err, flags, rc_mode = 0;
>>  
>> @@ -193,7 +195,18 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>  			return flags;
>>  
>>  		if (flags & ABX8XX_OSS_OF) {
>> -			dev_err(dev, "Oscillator failure, data is invalid.\n");
>> +			/*
>> +			 * The OF bit can be set either because of a reset
>> +			 * (PoR/Software reset) or because of an oscillator
>> +			 * failure. Effectively, it indicates that the stored
>> +			 * time is invalid. When we write the time, we clear
>> +			 * this bit. If it stays set, then this indicates an
>> +			 * oscillator failure.
>> +			 */
>> +			if (priv->wrote_time)
>> +				dev_err(dev, "Oscillator failure\n");
>> +			else
>> +				dev_info(dev, "Time data invalid\n");
>>  			return -EINVAL;
>>  		}
>>  	}
>> @@ -219,6 +232,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>  static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>  {
>>  	struct i2c_client *client = to_i2c_client(dev);
>> +	struct abx80x_priv *priv = i2c_get_clientdata(client);
>>  	unsigned char buf[8];
>>  	int err, flags;
>>  
>> @@ -252,6 +266,7 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>  		dev_err(&client->dev, "Unable to write oscillator status register\n");
>>  		return err;
>>  	}
>> +	priv->wrote_time = true;
>>  
>>  	return 0;
>>  }
> 
> ping?

ping again?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] rtc: abx80x: Don't warn about oscillator failure after PoR
  2024-01-08 22:05   ` Sean Anderson
@ 2024-01-25 17:14     ` Sean Anderson
  0 siblings, 0 replies; 4+ messages in thread
From: Sean Anderson @ 2024-01-25 17:14 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc; +Cc: linux-kernel

On 1/8/24 17:05, Sean Anderson wrote:
> On 12/11/23 11:03, Sean Anderson wrote:
>> On 10/19/23 12:39, Sean Anderson wrote:
>>> According to the datasheet, the "oscillator failure" bit is set
>>> 
>>>> ...on a power on reset, when both the system and battery voltages have
>>>> dropped below acceptable levels. It is also set if an Oscillator Failure
>>>> occurs....
>>> 
>>> From testing, this bit is also set if a software reset is initiated.
>>> 
>>> This bit has a confusing name; it really tells us whether the time data
>>> is valid. We clear it when writing the time. If it is still set, that
>>> means there is a persistent issue (such as an oscillator failure),
>>> instead of a transient one (such as power loss).
>>> 
>>> Because there are several other reasons which might cause this bit
>>> to be set (including booting for the first time or a battery failure),
>>> do not warn about oscillator failures willy-nilly. This may cause system
>>> integrators to waste time looking into the wrong line of investigation.
>>> 
>>> We continue printing a message about invalid time data or an oscillator
>>> failure. There is no voltimeter in this RTC, so this is the best
>>> indication that the battery is dead (or dying) and reeds replacement.
>>> 
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>> Note that the following drivers all warn when they detect a problem with
>>> the oscillator:
>>> 
>>> drivers/rtc/rtc-ds1672.c
>>> drivers/rtc/rtc-pcf*.c
>>> drivers/rtc/rtc-rs5c*.c
>>> drivers/rtc/rtc-sc27xx.c
>>> 
>>> So warning about such an error has good precedent.
>>> 
>>> Changes in v3:
>>> - Use info since this is a good indication of a battery failure
>>> 
>>> Changes in v2:
>>> - Use debug instead of info in the typical case (no battery)
>>> 
>>>  drivers/rtc/rtc-abx80x.c | 17 ++++++++++++++++-
>>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
>>> index fde2b8054c2e..f463a58a240b 100644
>>> --- a/drivers/rtc/rtc-abx80x.c
>>> +++ b/drivers/rtc/rtc-abx80x.c
>>> @@ -127,6 +127,7 @@ struct abx80x_priv {
>>>  	struct rtc_device *rtc;
>>>  	struct i2c_client *client;
>>>  	struct watchdog_device wdog;
>>> +	bool wrote_time;
>>>  };
>>>  
>>>  static int abx80x_write_config_key(struct i2c_client *client, u8 key)
>>> @@ -179,6 +180,7 @@ static int abx80x_enable_trickle_charger(struct i2c_client *client,
>>>  static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>>  {
>>>  	struct i2c_client *client = to_i2c_client(dev);
>>> +	struct abx80x_priv *priv = i2c_get_clientdata(client);
>>>  	unsigned char buf[8];
>>>  	int err, flags, rc_mode = 0;
>>>  
>>> @@ -193,7 +195,18 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>>  			return flags;
>>>  
>>>  		if (flags & ABX8XX_OSS_OF) {
>>> -			dev_err(dev, "Oscillator failure, data is invalid.\n");
>>> +			/*
>>> +			 * The OF bit can be set either because of a reset
>>> +			 * (PoR/Software reset) or because of an oscillator
>>> +			 * failure. Effectively, it indicates that the stored
>>> +			 * time is invalid. When we write the time, we clear
>>> +			 * this bit. If it stays set, then this indicates an
>>> +			 * oscillator failure.
>>> +			 */
>>> +			if (priv->wrote_time)
>>> +				dev_err(dev, "Oscillator failure\n");
>>> +			else
>>> +				dev_info(dev, "Time data invalid\n");
>>>  			return -EINVAL;
>>>  		}
>>>  	}
>>> @@ -219,6 +232,7 @@ static int abx80x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>>  static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>>  {
>>>  	struct i2c_client *client = to_i2c_client(dev);
>>> +	struct abx80x_priv *priv = i2c_get_clientdata(client);
>>>  	unsigned char buf[8];
>>>  	int err, flags;
>>>  
>>> @@ -252,6 +266,7 @@ static int abx80x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>>  		dev_err(&client->dev, "Unable to write oscillator status register\n");
>>>  		return err;
>>>  	}
>>> +	priv->wrote_time = true;
>>>  
>>>  	return 0;
>>>  }
>> 
>> ping?
> 
> ping again?

Does anyone read this list? This patch has gone unreviewed since December of 2022!

--Sean

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-01-25 17:15 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-19 16:39 [PATCH v3] rtc: abx80x: Don't warn about oscillator failure after PoR Sean Anderson
2023-12-11 16:03 ` Sean Anderson
2024-01-08 22:05   ` Sean Anderson
2024-01-25 17:14     ` Sean Anderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).