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

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

[Jan Kardell]

Return -EINVAL if the voltage low bit is set or the time looks invalid
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. Anyway most rtc drivers do return
an error if the read time is invalid.

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

diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 0ba7e59..1af0c48 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
@@ -202,8 +202,9 @@ static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 
 	if (buf[PCF8563_REG_SC] & PCF8563_SC_LV) {
 		pcf8563->voltage_low = 1;
-		dev_info(&client->dev,
+		dev_err(&client->dev,
 			"low voltage detected, date/time is not reliable.\n");
+		return -EINVAL;
 	}
 
 	dev_dbg(&client->dev,
@@ -234,11 +235,11 @@ 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)
+	err = rtc_valid_tm(tm);
+	if (err < 0) {
 		dev_err(&client->dev, "retrieved date/time is not valid.\n");
+		return err;
+	}
 
 	return 0;
 }
-- 
1.8.4.5

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

[Alexandre Belloni]

Hi,

On 25/05/2015 at 11:51:14 +0200, Jan Kardell wrote :
> @@ -202,8 +202,9 @@ static int pcf8563_get_datetime(struct i2c_client *client, struct rtc_time *tm)
>  
>  	if (buf[PCF8563_REG_SC] & PCF8563_SC_LV) {
>  		pcf8563->voltage_low = 1;
> -		dev_info(&client->dev,
> +		dev_err(&client->dev,
>  			"low voltage detected, date/time is not reliable.\n");
> +		return -EINVAL;
>  	}
>  
>  	dev_dbg(&client->dev,
> @@ -234,11 +235,11 @@ 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)
> +	err = rtc_valid_tm(tm);
> +	if (err < 0) {
>  		dev_err(&client->dev, "retrieved date/time is not valid.\n");
> +		return err;
> +	}

I would avoid dev_err and directly return rtc_valid_tm(tm) or you could
also return 0 and let the core do the rtc_valid_tm() check.

>  
>  	return 0;
>  }
> -- 
> 1.8.4.5
> 

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