[PATCH v2] rtc: pcf8563 fix: return -EINVAL if we read an invalid time.

[PATCH v2] rtc: pcf8563 fix: return -EINVAL if we read an invalid time.

[Jan Kardell]

Return -EINVAL if the voltage low bit is set to avoid getting a bogus
time at boot.
There was a comment stating that util-linux hwclock refuses to set a
new time if we return an error code on read, but at least the current
version do set the time as expected. Remove the comment and the check
for valid time, and let the rtc core check it for us.

Changes in v2: Remove the test for invalid time. Change dev_err to
dev_info when voltage low is set.

Signed-off-by: Jan Kardell <jan.kardell@telliq.com>
---
 drivers/rtc/rtc-pcf8563.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 0ba7e59..6ce0a86 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -22,7 +22,7 @@
 #include <linux/of.h>
 #include <linux/err.h>
 
-#define DRV_VERSION "0.4.3"
+#define DRV_VERSION "0.4.4"
 
 #define PCF8563_REG_ST1		0x00 /* status */
 #define PCF8563_REG_ST2		0x01
@@ -204,6 +204,7 @@ static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 		pcf8563->voltage_low = 1;
 		dev_info(&client->dev,
 			"low voltage detected, date/time is not reliable.\n");
+		return -EINVAL;
 	}
 
 	dev_dbg(&client->dev,
@@ -234,12 +235,6 @@ static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 		tm->tm_sec, tm->tm_min, tm->tm_hour,
 		tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);
 
-	/* the clock can give out invalid datetime, but we cannot return
-	 * -EINVAL otherwise hwclock will refuse to set the time on bootup.
-	 */
-	if (rtc_valid_tm(tm) < 0)
-		dev_err(&client->dev, "retrieved date/time is not valid.\n");
-
 	return 0;
 }
 
-- 
1.8.4.5

Re: [PATCH v2] rtc: pcf8563 fix: return -EINVAL if we read an invalid time.

[Alexandre Belloni]

Hi,

On 27/05/2015 at 14:48:16 +0200, Jan Kardell wrote :
> @@ -204,6 +204,7 @@ static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
>  		pcf8563->voltage_low = 1;
>  		dev_info(&client->dev,

Aren't dev_err or dev_warn more appropriate? You used dev_err in your
first patch, what made you change your mind?

>  			"low voltage detected, date/time is not reliable.\n");
> +		return -EINVAL;
>  	}
>  
>  	dev_dbg(&client->dev,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2] rtc: pcf8563 fix: return -EINVAL if we read an invalid time.

[Jan Kardell]

Alexandre Belloni skrev:
> Hi,
>
> On 27/05/2015 at 14:48:16 +0200, Jan Kardell wrote :
>> @@ -204,6 +204,7 @@ static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
>>   		pcf8563->voltage_low = 1;
>>   		dev_info(&client->dev,
> Aren't dev_err or dev_warn more appropriate? You used dev_err in your
> first patch, what made you change your mind?
Your comment to the first patch. But I interpreted it to broadly.
I'll change it back to dev_err.
>
>>   			"low voltage detected, date/time is not reliable.\n");
>> +		return -EINVAL;
>>   	}
>>   
>>   	dev_dbg(&client->dev,

Re: [PATCH v2] rtc: pcf8563 fix: return -EINVAL if we read an invalid time.

[Richard Weinberger]

On Wed, May 27, 2015 at 2:48 PM, Jan Kardell <jan.kardell@telliq.com> wrote:
> Return -EINVAL if the voltage low bit is set to avoid getting a bogus
> time at boot.
> There was a comment stating that util-linux hwclock refuses to set a
> new time if we return an error code on read, but at least the current
> version do set the time as expected. Remove the comment and the check
> for valid time, and let the rtc core check it for us.
>
> Changes in v2: Remove the test for invalid time. Change dev_err to
> dev_info when voltage low is set.
>
> Signed-off-by: Jan Kardell <jan.kardell@telliq.com>
> ---
>  drivers/rtc/rtc-pcf8563.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
> index 0ba7e59..6ce0a86 100644
> --- a/drivers/rtc/rtc-pcf8563.c
> +++ b/drivers/rtc/rtc-pcf8563.c
> @@ -22,7 +22,7 @@
>  #include <linux/of.h>
>  #include <linux/err.h>
>
> -#define DRV_VERSION "0.4.3"
> +#define DRV_VERSION "0.4.4"

Why do you need that?
Isn't the kernel version enough? :)

-- 
Thanks,
//richard

Re: [rtc-linux] Re: [PATCH v2] rtc: pcf8563 fix: return -EINVAL if we read an invalid time.

[Alexandre Belloni]

On 31/05/2015 at 00:16:39 +0200, Richard Weinberger wrote :
> On Wed, May 27, 2015 at 2:48 PM, Jan Kardell <jan.kardell@telliq.com> wrote:
> > Return -EINVAL if the voltage low bit is set to avoid getting a bogus
> > time at boot.
> > There was a comment stating that util-linux hwclock refuses to set a
> > new time if we return an error code on read, but at least the current
> > version do set the time as expected. Remove the comment and the check
> > for valid time, and let the rtc core check it for us.
> >
> > Changes in v2: Remove the test for invalid time. Change dev_err to
> > dev_info when voltage low is set.
> >
> > Signed-off-by: Jan Kardell <jan.kardell@telliq.com>
> > ---
> >  drivers/rtc/rtc-pcf8563.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
> > index 0ba7e59..6ce0a86 100644
> > --- a/drivers/rtc/rtc-pcf8563.c
> > +++ b/drivers/rtc/rtc-pcf8563.c
> > @@ -22,7 +22,7 @@
> >  #include <linux/of.h>
> >  #include <linux/err.h>
> >
> > -#define DRV_VERSION "0.4.3"
> > +#define DRV_VERSION "0.4.4"
> 
> Why do you need that?
> Isn't the kernel version enough? :)
> 

That's also what I'm thinking but I don't care enough to ask people to
remove those.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com