| Message ID | f801d4f26d4c4944b1a5353d5b5fde4e507b45b2.1446587705.git.stillcompiling@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 04/11/2015 at 07:36:37 -0800, Joshua Clayton wrote : > + ret = pcf2123_rtc_read_time(dev, &tm); > + if (ret < 0) > + return false; > + > + if (rtc_valid_tm(&tm) < 0) { > + dev_err(dev, "retrieved date/time is not valid.\n"); > + return false; > + } > + I would remove that test as basically, the date/time will only be valid when OSC_HAS_STOPPED is not set.
On Wed, 25 Nov 2015 00:25:12 +0100 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 04/11/2015 at 07:36:37 -0800, Joshua Clayton wrote : > > + ret = pcf2123_rtc_read_time(dev, &tm); > > + if (ret < 0) > > + return false; > > + > > + if (rtc_valid_tm(&tm) < 0) { > > + dev_err(dev, "retrieved date/time is not > > valid.\n"); > > + return false; > > + } > > + > > I would remove that test as basically, the date/time will only be > valid when OSC_HAS_STOPPED is not set. > OSC_HAS_STOPPED really only protects us in case everything else looks good, but we've lost power. There are other reasons to check for a valid time. Specifically, if there is no communication with the device, the spi operation will succeed and return all zeros or all ones. Since either of these results in an invalid time, it is a nice way to probe whether we really have a pcf2123 compatible device. I don't think I should remove this test, but I can add a comment
On 01/12/2015 at 12:23:51 -0800, Joshua Clayton wrote : > On Wed, 25 Nov 2015 00:25:12 +0100 > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > On 04/11/2015 at 07:36:37 -0800, Joshua Clayton wrote : > > > + ret = pcf2123_rtc_read_time(dev, &tm); > > > + if (ret < 0) > > > + return false; > > > + > > > + if (rtc_valid_tm(&tm) < 0) { > > > + dev_err(dev, "retrieved date/time is not > > > valid.\n"); > > > + return false; > > > + } > > > + > > > > I would remove that test as basically, the date/time will only be > > valid when OSC_HAS_STOPPED is not set. > > > OSC_HAS_STOPPED really only protects us in case everything else > looks good, but we've lost power. > There are other reasons to check for a valid time. > Specifically, if there is no communication with the device, the spi > operation will succeed and return all zeros or all ones. > Since either of these results in an invalid time, it is a nice way > to probe whether we really have a pcf2123 compatible device. > > I don't think I should remove this test, but I can add a comment OK but then pcf2123_rtc_read_time actually returns rtc_valid_tm(tm) ;) The proper course of action is probably to do the OSC_HAS_STOPPED check in pcf2123_rtc_read_time then you don't even need pcf2123_time_valid().
diff --git a/drivers/rtc/rtc-pcf2123.c b/drivers/rtc/rtc-pcf2123.c index d3c1447..4964d5c 100644 --- a/drivers/rtc/rtc-pcf2123.c +++ b/drivers/rtc/rtc-pcf2123.c @@ -260,6 +260,33 @@ static int pcf2123_rtc_set_time(struct device *dev, struct rtc_time *tm) return 0; } +static bool pcf2123_time_valid(struct device *dev) +{ + u8 seconds; + struct rtc_time tm; + int ret; + + ret = pcf2123_read(dev, PCF2123_REG_SC, &seconds, 1); + if (ret < 0) + return false; + + if (seconds & OSC_HAS_STOPPED) { + dev_info(dev, "clock was stopped. seconds: 0x%02X\n", seconds); + return false; + } + + ret = pcf2123_rtc_read_time(dev, &tm); + if (ret < 0) + return false; + + if (rtc_valid_tm(&tm) < 0) { + dev_err(dev, "retrieved date/time is not valid.\n"); + return false; + } + + return true; +} + static int pcf2123_reset(struct device *dev) { int ret; @@ -311,10 +338,12 @@ static int pcf2123_probe(struct spi_device *spi) return -ENOMEM; spi->dev.platform_data = pdata; - ret = pcf2123_reset(&spi->dev); - if (ret < 0) { - dev_err(&spi->dev, "chip not found\n"); - goto kfree_exit; + if (!pcf2123_time_valid(&spi->dev)) { + ret = pcf2123_reset(&spi->dev); + if (ret < 0) { + dev_err(&spi->dev, "chip not found\n"); + goto kfree_exit; + } } dev_info(&spi->dev, "chip found, driver version " DRV_VERSION "\n");
pcf2123 data sheet recommends a software reset when the chip is first powered on. This change avoids resetting the chip every time the driver is loaded, which has some negative effects. There are several registers including a clock rate adjustment that really should survive a reload of the driver (or reboot). In addition, stopping and restarting the clock to verify the chip is there is not a good thing once the time is set. According to the data sheet, the seconds register has a 1 in the high bit when the voltage has gotten low. We check for this condition, as well as whether the time retrieved from the chip is valid. We reset the rtc only if the time is not reliable and valid. This is sufficient for checking for the presence of the chip, as either all zeros or all 0xff will result in an invalid time/date Signed-off-by: Joshua Clayton <stillcompiling@gmail.com> --- drivers/rtc/rtc-pcf2123.c | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-)