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

[Andrea Scian <rnd4@dave-tech.it>]

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