[rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[rnd4]

From: Andrea Scian <andrea.scian@dave.eu>

I'm using PCF2127 in a custom ARM-based board and, by looking into PCF2127 datasheet 
I've found that, in my understanding, it's wrong to say that the date in unreliable
if BLF (battery low flag) is set but you should use OSF flag (seconds register) to
check if oscillator, for any reason, stopped.
Battery may be low (usually below 2V5 threshold) but the date may be anyway correct
(tipically date is unreliable when input voltage is below 1V2).

Feel free to comment and apply if useful.

Kind Regards,

Andrea

---

Also change the power fail message, which does NOT mean that time is not
valid but also states that user should replace the battery

Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
---
 drivers/rtc/rtc-pcf2127.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 1ee514a..2365788 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -59,7 +59,16 @@ static int pcf2127_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 	if (buf[PCF2127_REG_CTRL3] & 0x04) {
 		pcf2127->voltage_low = 1;
 		dev_info(&client->dev,
-			"low voltage detected, date/time is not reliable.\n");
+			"low voltage detected, check/replace RTC battery.\n");
+	}
+
+	if (buf[PCF2127_REG_SC] & 0x80) {
+		dev_warn(&client->dev,
+			"oscillator stop detected, date/time is not reliable.\n");
+		/*
+		 * no need clear the flag here,
+		 * it will be cleared once the new date is saved
+		 */
 	}
 
 	dev_dbg(&client->dev,
@@ -112,7 +121,7 @@ static int pcf2127_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 	buf[i++] = PCF2127_REG_SC;
 
 	/* hours, minutes and seconds */
-	buf[i++] = bin2bcd(tm->tm_sec);
+	buf[i++] = bin2bcd(tm->tm_sec);	/* this will also clear OFS flag */
 	buf[i++] = bin2bcd(tm->tm_min);
 	buf[i++] = bin2bcd(tm->tm_hour);
 	buf[i++] = bin2bcd(tm->tm_mday);
@@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
 	switch (cmd) {
 	case RTC_VL_READ:
 		if (pcf2127->voltage_low)
-			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
+			dev_info(dev, "low voltage detected, check/replace battery\n");
 
 		if (copy_to_user((void __user *)arg, &pcf2127->voltage_low,
 					sizeof(int)))
-- 
1.7.9.5

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Alexandre Belloni]

Hi,

This seems ok, a few comments below:

On 26/05/2015 at 10:17:40 +0200, rnd4@dave-tech.it wrote :
> From: Andrea Scian <andrea.scian@dave.eu>
> 
> I'm using PCF2127 in a custom ARM-based board and, by looking into PCF2127 datasheet 
> I've found that, in my understanding, it's wrong to say that the date in unreliable
> if BLF (battery low flag) is set but you should use OSF flag (seconds register) to
> check if oscillator, for any reason, stopped.
> Battery may be low (usually below 2V5 threshold) but the date may be anyway correct
> (tipically date is unreliable when input voltage is below 1V2).

typically -^

> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 1ee514a..2365788 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -59,7 +59,16 @@ static int pcf2127_get_datetime(struct i2c_client *client, struct rtc_time *tm)
>  	if (buf[PCF2127_REG_CTRL3] & 0x04) {
>  		pcf2127->voltage_low = 1;
>  		dev_info(&client->dev,
> -			"low voltage detected, date/time is not reliable.\n");
> +			"low voltage detected, check/replace RTC battery.\n");
> +	}
> +
> +	if (buf[PCF2127_REG_SC] & 0x80) {

Maybe use a define instead of 0x80, remember to use the BIT() macro.

> +		dev_warn(&client->dev,
> +			"oscillator stop detected, date/time is not reliable.\n");

I would return -EINVAL here because the result might still pass
rtc_valid_tm() but be outdated.

> +		/*
> +		 * no need clear the flag here,
> +		 * it will be cleared once the new date is saved
> +		 */
>  	}
>  
>  	dev_dbg(&client->dev,
> @@ -112,7 +121,7 @@ static int pcf2127_set_datetime(struct i2c_client *client, struct rtc_time *tm)
>  	buf[i++] = PCF2127_REG_SC;
>  
>  	/* hours, minutes and seconds */
> -	buf[i++] = bin2bcd(tm->tm_sec);
> +	buf[i++] = bin2bcd(tm->tm_sec);	/* this will also clear OFS flag */
>  	buf[i++] = bin2bcd(tm->tm_min);
>  	buf[i++] = bin2bcd(tm->tm_hour);
>  	buf[i++] = bin2bcd(tm->tm_mday);
> @@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
>  	switch (cmd) {
>  	case RTC_VL_READ:
>  		if (pcf2127->voltage_low)
> -			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
> +			dev_info(dev, "low voltage detected, check/replace battery\n");

I would also print a warning about OFS here.

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

Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Andrea Scian]

Dear Alexandre,

On 08/06/2015 17:42, Alexandre Belloni wrote:
> Hi,
>
> This seems ok, a few comments below:
>
> On 26/05/2015 at 10:17:40 +0200, rnd4@dave-tech.it wrote :
>> From: Andrea Scian <andrea.scian@dave.eu>
>>
>> I'm using PCF2127 in a custom ARM-based board and, by looking into PCF2127 datasheet
>> I've found that, in my understanding, it's wrong to say that the date in unreliable
>> if BLF (battery low flag) is set but you should use OSF flag (seconds register) to
>> check if oscillator, for any reason, stopped.
>> Battery may be low (usually below 2V5 threshold) but the date may be anyway correct
>> (tipically date is unreliable when input voltage is below 1V2).
>
> typically -^


wooops.. thanks

>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index 1ee514a..2365788 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -59,7 +59,16 @@ static int pcf2127_get_datetime(struct i2c_client *client, struct rtc_time *tm)
>>   	if (buf[PCF2127_REG_CTRL3] & 0x04) {
>>   		pcf2127->voltage_low = 1;
>>   		dev_info(&client->dev,
>> -			"low voltage detected, date/time is not reliable.\n");
>> +			"low voltage detected, check/replace RTC battery.\n");
>> +	}
>> +
>> +	if (buf[PCF2127_REG_SC] & 0x80) {
>
> Maybe use a define instead of 0x80, remember to use the BIT() macro.

I'll fix it

I'll also use BIT() for the 0x04 above (in a different commit)

>
>> +		dev_warn(&client->dev,
>> +			"oscillator stop detected, date/time is not reliable.\n");
>
> I would return -EINVAL here because the result might still pass
> rtc_valid_tm() but be outdated.

At first look I agree with you, but a bit later they say:

/* the clock can give out invalid datetime, but we cannot return
  * -EINVAL otherwise hwclock will refuse to set the time on bootup.
  */

http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91

so they don't actually return -EINVAL even if rtc_valid_tm() fails.
WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something..

If the comment above is correct, so we can't return -EINVAL, I will 
reset the time to epoch, with something like

rtc_time64_to_tm((time64_t)0, tm);

and issue the warning.
Later userspace script usually reset the time to /etc/timestamp to avoid 
"back to future" problems ;-)

>
>> +		/*
>> +		 * no need clear the flag here,
>> +		 * it will be cleared once the new date is saved
>> +		 */
>>   	}
>>
>>   	dev_dbg(&client->dev,
>> @@ -112,7 +121,7 @@ static int pcf2127_set_datetime(struct i2c_client *client, struct rtc_time *tm)
>>   	buf[i++] = PCF2127_REG_SC;
>>
>>   	/* hours, minutes and seconds */
>> -	buf[i++] = bin2bcd(tm->tm_sec);
>> +	buf[i++] = bin2bcd(tm->tm_sec);	/* this will also clear OFS flag */
>>   	buf[i++] = bin2bcd(tm->tm_min);
>>   	buf[i++] = bin2bcd(tm->tm_hour);
>>   	buf[i++] = bin2bcd(tm->tm_mday);
>> @@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
>>   	switch (cmd) {
>>   	case RTC_VL_READ:
>>   		if (pcf2127->voltage_low)
>> -			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
>> +			dev_info(dev, "low voltage detected, check/replace battery\n");
>
> I would also print a warning about OFS here.
>

I'll do.
Do you think is better to add another variable inside struct pcf2127 or 
is better to re-read the RTC registers?
(for the former I have also to clear the variable inside 
pcf2127_set_datetime(), for the latter I have to issue another read in a 
function that, at the moment, does not read anything..)

Thanks for you feedback and kind regards,

-- 

Andrea SCIAN

DAVE Embedded Systems

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Andrea Scian]

Dear Alexandre,

On 08/06/2015 17:42, Alexandre Belloni wrote:
> Hi,
>
> This seems ok, a few comments below:
>
> On 26/05/2015 at 10:17:40 +0200, rnd4@dave-tech.it wrote :
>> From: Andrea Scian <andrea.scian@dave.eu>
>>
>> I'm using PCF2127 in a custom ARM-based board and, by looking into PCF2127 datasheet
>> I've found that, in my understanding, it's wrong to say that the date in unreliable
>> if BLF (battery low flag) is set but you should use OSF flag (seconds register) to
>> check if oscillator, for any reason, stopped.
>> Battery may be low (usually below 2V5 threshold) but the date may be anyway correct
>> (tipically date is unreliable when input voltage is below 1V2).
>
> typically -^


wooops.. thanks

>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index 1ee514a..2365788 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -59,7 +59,16 @@ static int pcf2127_get_datetime(struct i2c_client *client, struct rtc_time *tm)
>>   	if (buf[PCF2127_REG_CTRL3] & 0x04) {
>>   		pcf2127->voltage_low = 1;
>>   		dev_info(&client->dev,
>> -			"low voltage detected, date/time is not reliable.\n");
>> +			"low voltage detected, check/replace RTC battery.\n");
>> +	}
>> +
>> +	if (buf[PCF2127_REG_SC] & 0x80) {
>
> Maybe use a define instead of 0x80, remember to use the BIT() macro.

I'll fix it

I'll also use BIT() for the 0x04 above (in a different commit)

>
>> +		dev_warn(&client->dev,
>> +			"oscillator stop detected, date/time is not reliable.\n");
>
> I would return -EINVAL here because the result might still pass
> rtc_valid_tm() but be outdated.

At first look I agree with you, but a bit later they say:

/* the clock can give out invalid datetime, but we cannot return
  * -EINVAL otherwise hwclock will refuse to set the time on bootup.
  */

http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91

so they don't actually return -EINVAL even if rtc_valid_tm() fails.
WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something..

If the comment above is correct, so we can't return -EINVAL, I will 
reset the time to epoch, with something like

rtc_time64_to_tm((time64_t)0, tm);

and issue the warning.
Later userspace script usually reset the time to /etc/timestamp to avoid 
"back to future" problems ;-)

>
>> +		/*
>> +		 * no need clear the flag here,
>> +		 * it will be cleared once the new date is saved
>> +		 */
>>   	}
>>
>>   	dev_dbg(&client->dev,
>> @@ -112,7 +121,7 @@ static int pcf2127_set_datetime(struct i2c_client *client, struct rtc_time *tm)
>>   	buf[i++] = PCF2127_REG_SC;
>>
>>   	/* hours, minutes and seconds */
>> -	buf[i++] = bin2bcd(tm->tm_sec);
>> +	buf[i++] = bin2bcd(tm->tm_sec);	/* this will also clear OFS flag */
>>   	buf[i++] = bin2bcd(tm->tm_min);
>>   	buf[i++] = bin2bcd(tm->tm_hour);
>>   	buf[i++] = bin2bcd(tm->tm_mday);
>> @@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
>>   	switch (cmd) {
>>   	case RTC_VL_READ:
>>   		if (pcf2127->voltage_low)
>> -			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
>> +			dev_info(dev, "low voltage detected, check/replace battery\n");
>
> I would also print a warning about OFS here.
>

I'll do.
Do you think is better to add another variable inside struct pcf2127 or 
is better to re-read the RTC registers?
(for the former I have also to clear the variable inside 
pcf2127_set_datetime(), for the latter I have to issue another read in a 
function that, at the moment, does not read anything..)

Thanks for you feedback and kind regards,

-- 

Andrea SCIAN

DAVE Embedded Systems

Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Alexandre Belloni]

On 10/06/2015 at 17:21:57 +0200, Andrea Scian wrote :
> >I would return -EINVAL here because the result might still pass
> >rtc_valid_tm() but be outdated.
> 
> At first look I agree with you, but a bit later they say:
> 
> /* the clock can give out invalid datetime, but we cannot return
>  * -EINVAL otherwise hwclock will refuse to set the time on bootup.
>  */
> 
> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91
> 
> so they don't actually return -EINVAL even if rtc_valid_tm() fails.
> WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something..
> 

This has been copy pasted from other drivers and this is simply not
true.

> If the comment above is correct, so we can't return -EINVAL, I will reset
> the time to epoch, with something like
> 
> rtc_time64_to_tm((time64_t)0, tm);
> 

Doing that is worse. You really want userspace to know that the time is
invalid instead of giving an incorrect value. This allow userspace to
actually choose its policy when the time is invalid. For example, use
epoch or any other later date that probabyl makes more sense for the
product.

> >>@@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
> >>  	switch (cmd) {
> >>  	case RTC_VL_READ:
> >>  		if (pcf2127->voltage_low)
> >>-			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
> >>+			dev_info(dev, "low voltage detected, check/replace battery\n");
> >
> >I would also print a warning about OFS here.
> >
> 
> I'll do.
> Do you think is better to add another variable inside struct pcf2127 or is
> better to re-read the RTC registers?
> (for the former I have also to clear the variable inside
> pcf2127_set_datetime(), for the latter I have to issue another read in a
> function that, at the moment, does not read anything..)
> 

I don't really care. But since one of them is already cached, it is
probably better to cache OFS. Maybe you could also use voltage_low as a
bit field which would allow userspace to make the difference between a
simple low voltage and the time loss condition.


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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Alexandre Belloni]

On 10/06/2015 at 17:21:57 +0200, Andrea Scian wrote :
> >I would return -EINVAL here because the result might still pass
> >rtc_valid_tm() but be outdated.
> 
> At first look I agree with you, but a bit later they say:
> 
> /* the clock can give out invalid datetime, but we cannot return
>  * -EINVAL otherwise hwclock will refuse to set the time on bootup.
>  */
> 
> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91
> 
> so they don't actually return -EINVAL even if rtc_valid_tm() fails.
> WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something..
> 

This has been copy pasted from other drivers and this is simply not
true.

> If the comment above is correct, so we can't return -EINVAL, I will reset
> the time to epoch, with something like
> 
> rtc_time64_to_tm((time64_t)0, tm);
> 

Doing that is worse. You really want userspace to know that the time is
invalid instead of giving an incorrect value. This allow userspace to
actually choose its policy when the time is invalid. For example, use
epoch or any other later date that probabyl makes more sense for the
product.

> >>@@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
> >>  	switch (cmd) {
> >>  	case RTC_VL_READ:
> >>  		if (pcf2127->voltage_low)
> >>-			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
> >>+			dev_info(dev, "low voltage detected, check/replace battery\n");
> >
> >I would also print a warning about OFS here.
> >
> 
> I'll do.
> Do you think is better to add another variable inside struct pcf2127 or is
> better to re-read the RTC registers?
> (for the former I have also to clear the variable inside
> pcf2127_set_datetime(), for the latter I have to issue another read in a
> function that, at the moment, does not read anything..)
> 

I don't really care. But since one of them is already cached, it is
probably better to cache OFS. Maybe you could also use voltage_low as a
bit field which would allow userspace to make the difference between a
simple low voltage and the time loss condition.


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

Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Andrea Scian]

Hi Alexandre,

On 12/06/2015 09:42, Alexandre Belloni wrote:
> On 10/06/2015 at 17:21:57 +0200, Andrea Scian wrote :
>>> I would return -EINVAL here because the result might still pass
>>> rtc_valid_tm() but be outdated.
>> At first look I agree with you, but a bit later they say:
>>
>> /* the clock can give out invalid datetime, but we cannot return
>>   * -EINVAL otherwise hwclock will refuse to set the time on bootup.
>>   */
>>
>> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91
>>
>> so they don't actually return -EINVAL even if rtc_valid_tm() fails.
>> WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something..
>>
> This has been copy pasted from other drivers and this is simply not
> true.

Thanks for point this out.
I'll split the patch to fix this for all PCF drivers (which have nearly 
all the same structure) and later add the OFS flag

>> If the comment above is correct, so we can't return -EINVAL, I will reset
>> the time to epoch, with something like
>>
>> rtc_time64_to_tm((time64_t)0, tm);
>>
> Doing that is worse. You really want userspace to know that the time is
> invalid instead of giving an incorrect value. This allow userspace to
> actually choose its policy when the time is invalid. For example, use
> epoch or any other later date that probabyl makes more sense for the
> product.

Most of minimal RFS I saw reset the date to what's inside /etc/timestamp 
(which is updated in runlevel 6). However, this is OT here.

>>>> @@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
>>>>   	switch (cmd) {
>>>>   	case RTC_VL_READ:
>>>>   		if (pcf2127->voltage_low)
>>>> -			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
>>>> +			dev_info(dev, "low voltage detected, check/replace battery\n");
>>> I would also print a warning about OFS here.
>>>
>> I'll do.
>> Do you think is better to add another variable inside struct pcf2127 or is
>> better to re-read the RTC registers?
>> (for the former I have also to clear the variable inside
>> pcf2127_set_datetime(), for the latter I have to issue another read in a
>> function that, at the moment, does not read anything..)
>>
> I don't really care. But since one of them is already cached, it is
> probably better to cache OFS. Maybe you could also use voltage_low as a
> bit field which would allow userspace to make the difference between a
> simple low voltage and the time loss condition.
>
>

I'll cache OFS too, in a different variable. Returning different values 
depending on OFS when querying about voltage low may mislead some 
application. Moreover I think that there's may be some cases when OFS is 
set and voltage low is not (e.g. when replacing battery with a brand new 
one).

I'll send the updated patch soon

Thanks for your comments/help,

-- 

Andrea SCIAN

DAVE Embedded Systems

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Andrea Scian]

Hi Alexandre,

On 12/06/2015 09:42, Alexandre Belloni wrote:
> On 10/06/2015 at 17:21:57 +0200, Andrea Scian wrote :
>>> I would return -EINVAL here because the result might still pass
>>> rtc_valid_tm() but be outdated.
>> At first look I agree with you, but a bit later they say:
>>
>> /* the clock can give out invalid datetime, but we cannot return
>>   * -EINVAL otherwise hwclock will refuse to set the time on bootup.
>>   */
>>
>> http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/rtc-pcf2127.c#n91
>>
>> so they don't actually return -EINVAL even if rtc_valid_tm() fails.
>> WDYT? I'm not an RTC subsystem expert, so maybe I'm missing something..
>>
> This has been copy pasted from other drivers and this is simply not
> true.

Thanks for point this out.
I'll split the patch to fix this for all PCF drivers (which have nearly 
all the same structure) and later add the OFS flag

>> If the comment above is correct, so we can't return -EINVAL, I will reset
>> the time to epoch, with something like
>>
>> rtc_time64_to_tm((time64_t)0, tm);
>>
> Doing that is worse. You really want userspace to know that the time is
> invalid instead of giving an incorrect value. This allow userspace to
> actually choose its policy when the time is invalid. For example, use
> epoch or any other later date that probabyl makes more sense for the
> product.

Most of minimal RFS I saw reset the date to what's inside /etc/timestamp 
(which is updated in runlevel 6). However, this is OT here.

>>>> @@ -144,7 +153,7 @@ static int pcf2127_rtc_ioctl(struct device *dev,
>>>>   	switch (cmd) {
>>>>   	case RTC_VL_READ:
>>>>   		if (pcf2127->voltage_low)
>>>> -			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
>>>> +			dev_info(dev, "low voltage detected, check/replace battery\n");
>>> I would also print a warning about OFS here.
>>>
>> I'll do.
>> Do you think is better to add another variable inside struct pcf2127 or is
>> better to re-read the RTC registers?
>> (for the former I have also to clear the variable inside
>> pcf2127_set_datetime(), for the latter I have to issue another read in a
>> function that, at the moment, does not read anything..)
>>
> I don't really care. But since one of them is already cached, it is
> probably better to cache OFS. Maybe you could also use voltage_low as a
> bit field which would allow userspace to make the difference between a
> simple low voltage and the time loss condition.
>
>

I'll cache OFS too, in a different variable. Returning different values 
depending on OFS when querying about voltage low may mislead some 
application. Moreover I think that there's may be some cases when OFS is 
set and voltage low is not (e.g. when replacing battery with a brand new 
one).

I'll send the updated patch soon

Thanks for your comments/help,

-- 

Andrea SCIAN

DAVE Embedded Systems

[rtc-linux] [PATCH] driver: rtc: use rtc_valid_tm() error code when reading date/time

[rnd4]

From: Andrea Scian <andrea.scian@dave.eu>

There's a wrong commend in some RTC driver that say it's better to ignore
rtc_valid_tm() when reading RTC timestamp. However this is wrong and is
better to return to the userspace the an error if timestamp is not valid.

Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
---
 drivers/rtc/rtc-isl12022.c |    7 +------
 drivers/rtc/rtc-pcf2123.c  |    8 +-------
 drivers/rtc/rtc-pcf2127.c  |    8 +-------
 drivers/rtc/rtc-pcf8563.c  |    8 +-------
 4 files changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index a1bbbb8..54e9eca 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -148,12 +148,7 @@ static int isl12022_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 and time is invalid.\n");
-
-	return 0;
+	return rtc_valid_tm(tm);
 }
 
 static int isl12022_set_datetime(struct i2c_client *client, struct rtc_time *tm)
diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c
index 796a6c5..e0a2469 100644
--- a/drivers/rtc/rtc-pcf2123.c
+++ b/drivers/rtc/rtc-pcf2123.c
@@ -159,13 +159,7 @@ static int pcf2123_rtc_read_time(struct device *dev, 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(dev, "retrieved date/time is not valid.\n");
-
-	return 0;
+	return rtc_valid_tm(tm);
 }
 
 static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm)
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 1ee514a..7f803b2 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -88,13 +88,7 @@ static int pcf2127_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;
+	return rtc_valid_tm(tm);
 }
 
 static int pcf2127_set_datetime(struct i2c_client *client, struct rtc_time *tm)
diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 97b354a..3a6d7e0 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -131,13 +131,7 @@ 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;
+	return rtc_valid_tm(tm);
 }
 
 static int pcf8563_set_datetime(struct i2c_client *client, struct rtc_time *tm)
-- 
1.7.9.5

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] driver: rtc: use rtc_valid_tm() error code when reading date/time

[Alexandre Belloni]

On 16/06/2015 at 11:35:19 +0200, rnd4@dave-tech.it wrote :
> From: Andrea Scian <andrea.scian@dave.eu>
> 
> There's a wrong commend in some RTC driver that say it's better to ignore
> rtc_valid_tm() when reading RTC timestamp. However this is wrong and is
> better to return to the userspace the an error if timestamp is not valid.
> 
> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
> ---
>  drivers/rtc/rtc-isl12022.c |    7 +------
>  drivers/rtc/rtc-pcf2123.c  |    8 +-------
>  drivers/rtc/rtc-pcf2127.c  |    8 +-------
>  drivers/rtc/rtc-pcf8563.c  |    8 +-------
>  4 files changed, 4 insertions(+), 27 deletions(-)
> 

Applied, after dropping the pcf8563 part as it was part of another
patch.


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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH] driver: rtc: use rtc_valid_tm() error code when reading date/time

[Alexandre Belloni]

On 16/06/2015 at 11:35:19 +0200, rnd4@dave-tech.it wrote :
> From: Andrea Scian <andrea.scian@dave.eu>
> 
> There's a wrong commend in some RTC driver that say it's better to ignore
> rtc_valid_tm() when reading RTC timestamp. However this is wrong and is
> better to return to the userspace the an error if timestamp is not valid.
> 
> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
> ---
>  drivers/rtc/rtc-isl12022.c |    7 +------
>  drivers/rtc/rtc-pcf2123.c  |    8 +-------
>  drivers/rtc/rtc-pcf2127.c  |    8 +-------
>  drivers/rtc/rtc-pcf8563.c  |    8 +-------
>  4 files changed, 4 insertions(+), 27 deletions(-)
> 

Applied, after dropping the pcf8563 part as it was part of another
patch.


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

[rtc-linux] [PATCH v2] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[rnd4]

From: Andrea Scian <andrea.scian@dave.eu>

Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
---

Changes since v1:
* use BIT() to select the right bitfield instead of hardcode value
* return -EINVAL if OFS is detected
* cache OSF into driver structure to warn the user even later

 drivers/rtc/rtc-pcf2127.c |   26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 7f803b2..aecee0b 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -33,11 +33,14 @@
 #define PCF2127_REG_MO          (0x08)
 #define PCF2127_REG_YR          (0x09)
 
+#define PCF2127_OSF             BIT(7)  /* Oscillator Fail flag */
+
 static struct i2c_driver pcf2127_driver;
 
 struct pcf2127 {
 	struct rtc_device *rtc;
 	int voltage_low; /* indicates if a low_voltage was detected */
+	int oscillator_failed; /* OSF was detected and date in unreliable */
 };
 
 /*
@@ -59,7 +62,18 @@ static int pcf2127_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 	if (buf[PCF2127_REG_CTRL3] & 0x04) {
 		pcf2127->voltage_low = 1;
 		dev_info(&client->dev,
-			"low voltage detected, date/time is not reliable.\n");
+			"low voltage detected, check/replace RTC battery.\n");
+	}
+
+	if (buf[PCF2127_REG_SC] & PCF2127_OSF) {
+		/*
+		 * no need clear the flag here,
+		 * it will be cleared once the new date is saved
+		 */
+		pcf2127->oscillator_failed = 1;
+		dev_warn(&client->dev,
+			"oscillator stop detected, date/time is not reliable\n");
+		return -EINVAL;
 	}
 
 	dev_dbg(&client->dev,
@@ -93,6 +107,7 @@ static int pcf2127_get_datetime(struct i2c_client *client, struct rtc_time *tm)
 
 static int pcf2127_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 {
+	struct pcf2127 *pcf2127 = i2c_get_clientdata(client);
 	unsigned char buf[8];
 	int i = 0, err;
 
@@ -106,7 +121,7 @@ static int pcf2127_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 	buf[i++] = PCF2127_REG_SC;
 
 	/* hours, minutes and seconds */
-	buf[i++] = bin2bcd(tm->tm_sec);
+	buf[i++] = bin2bcd(tm->tm_sec);	/* this will also clear OFS flag */
 	buf[i++] = bin2bcd(tm->tm_min);
 	buf[i++] = bin2bcd(tm->tm_hour);
 	buf[i++] = bin2bcd(tm->tm_mday);
@@ -126,6 +141,9 @@ static int pcf2127_set_datetime(struct i2c_client *client, struct rtc_time *tm)
 		return -EIO;
 	}
 
+	/* clear OSF flag in client data */
+	pcf2127->oscillator_failed = 0;
+
 	return 0;
 }
 
@@ -138,7 +156,9 @@ static int pcf2127_rtc_ioctl(struct device *dev,
 	switch (cmd) {
 	case RTC_VL_READ:
 		if (pcf2127->voltage_low)
-			dev_info(dev, "low voltage detected, date/time is not reliable.\n");
+			dev_info(dev, "low voltage detected, check/replace battery\n");
+		if (pcf2127->oscillator_failed)
+			dev_info(dev, "oscillator stop detected, date/time is not reliable\n");
 
 		if (copy_to_user((void __user *)arg, &pcf2127->voltage_low,
 					sizeof(int)))
-- 
1.7.9.5

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH v2] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Alexandre Belloni]

On 16/06/2015 at 11:39:47 +0200, rnd4@dave-tech.it wrote :
> From: Andrea Scian <andrea.scian@dave.eu>
> 
> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
> ---
> 
> Changes since v1:
> * use BIT() to select the right bitfield instead of hardcode value
> * return -EINVAL if OFS is detected
> * cache OSF into driver structure to warn the user even later
> 
>  drivers/rtc/rtc-pcf2127.c |   26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 

Applied, after adding back the commit message. Thanks!


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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] [PATCH v2] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user

[Alexandre Belloni]

On 16/06/2015 at 11:39:47 +0200, rnd4@dave-tech.it wrote :
> From: Andrea Scian <andrea.scian@dave.eu>
> 
> Signed-off-by: Andrea Scian <andrea.scian@dave.eu>
> ---
> 
> Changes since v1:
> * use BIT() to select the right bitfield instead of hardcode value
> * return -EINVAL if OFS is detected
> * cache OSF into driver structure to warn the user even later
> 
>  drivers/rtc/rtc-pcf2127.c |   26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 

Applied, after adding back the commit message. Thanks!


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