Messages in this thread Patch in this message | | | Date | Wed, 10 Jun 2015 17:21:57 +0200 | From | Andrea Scian <> | Subject | Re: [rtc-linux] [PATCH] driver: rtc: pcf2127: use OFS flag to detect unreliable date and warn the user |
| |
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
| |