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@xxxxxxxxxxxx wrote :
> > From: Andrea Scian <andrea.scian@xxxxxxx>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/