diff mbox

drivers/rtc/rtc-m41t93.c: don't let get_time() reset error state

Message ID 201204231230.q3NCUbbP015327@gatekeeper.vosshq.de
State Superseded
Headers show

Commit Message

Voss, Nikolaus April 23, 2012, 10:51 a.m. UTC 
If the rtc reports the time might be invalid due to oscillator
failure, this flags must not be reset by get_time() as the read
operation doesn't make the time valid.

Instead, the flag is reset in set_time() when a valid time is
to be written.

Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
---
 drivers/rtc/rtc-m41t93.c |   46 +++++++++++++++++++++++++++-------------------
 1 files changed, 27 insertions(+), 19 deletions(-)

Comments

Andrew Morton April 23, 2012, 8:53 p.m. UTC | #1 
On Mon, 23 Apr 2012 12:51:23 +0200
Nikolaus Voss <n.voss@weinmann.de> wrote:

> If the rtc reports the time might be invalid due to oscillator
> failure, this flags must not be reset by get_time() as the read
> operation doesn't make the time valid.
> 
> Instead, the flag is reset in set_time() when a valid time is
> to be written.

This is rather vague - what is/are "these flags"?  From the patch I
think you're referring to (the maddeningly undocumented)
M41T93_FLAG_ST?  If so, I'd suggest something like

: If the rtc reports the time might be invalid due to oscillator failure,
: the M41T93_FLAG_ST flag must not be reset by get_time() as the read
: operation doesn't make the time valid.
: 
: Instead, the M41T93_FLAG_ST flag is reset in set_time() when a valid time
: is to be written.

Secondly, you provided no description of the user-visible effects of
the bug.  Hence I cannot work out which kernel version(s) this patch
should be merged into.
Voss, Nikolaus April 24, 2012, 4:51 a.m. UTC | #2 
Andrew Morton wrote on 2012-04-23:
> On Mon, 23 Apr 2012 12:51:23 +0200
> Nikolaus Voss <n.voss@weinmann.de> wrote:
> 
>> If the rtc reports the time might be invalid due to oscillator
>> failure, this flags must not be reset by get_time() as the read
>> operation doesn't make the time valid.
>> 
>> Instead, the flag is reset in set_time() when a valid time is
>> to be written.
> 
> This is rather vague - what is/are "these flags"?

Sorry, that's true. I speak of the M41T93_FLAG_OF
(Oscillator Fail) flag.

[...]
> Secondly, you provided no description of the user-visible effects of
> the bug.  Hence I cannot work out which kernel version(s) this patch
> should be merged into.

What the user sees is that the first get_time() read reports
an error (invalid time) and the second reports no error, so the
first read magically heals the rtc time.

The patch applies to all kernel versions with support for m41t93 rtc.

I'll repost the patch with a proper description.

Niko
diff mbox

Patch

diff --git a/drivers/rtc/rtc-m41t93.c b/drivers/rtc/rtc-m41t93.c
index ef71132..f19f45c 100644
--- a/drivers/rtc/rtc-m41t93.c
+++ b/drivers/rtc/rtc-m41t93.c
@@ -48,6 +48,7 @@  static inline int m41t93_set_reg(struct spi_device *spi, u8 addr, u8 data)
 static int m41t93_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct spi_device *spi = to_spi_device(dev);
+	int tmp;
 	u8 buf[9] = {0x80};        /* write cmd + 8 data bytes */
 	u8 * const data = &buf[1]; /* ptr to first data byte */
 
@@ -62,6 +63,30 @@  static int m41t93_set_time(struct device *dev, struct rtc_time *tm)
 		return -EINVAL;
 	}
 
+	tmp = spi_w8r8(spi, M41T93_REG_FLAGS);
+	if (tmp < 0)
+		return tmp;
+
+	if (tmp & M41T93_FLAG_OF) {
+		dev_warn(&spi->dev, "OF bit is set, resetting.\n");
+		m41t93_set_reg(spi, M41T93_REG_FLAGS, tmp & ~M41T93_FLAG_OF);
+
+		tmp = spi_w8r8(spi, M41T93_REG_FLAGS);
+		if (tmp < 0) {
+			return tmp;
+		} else if (tmp & M41T93_FLAG_OF) {
+			/* OF cannot be immediately reset: oscillator has to be
+			 * restarted. */
+			u8 reset_osc = buf[M41T93_REG_ST_SEC] | M41T93_FLAG_ST;
+
+			dev_warn(&spi->dev,
+				 "OF bit is still set, kickstarting clock.\n");
+			m41t93_set_reg(spi, M41T93_REG_ST_SEC, reset_osc);
+			reset_osc &= ~M41T93_FLAG_ST;
+			m41t93_set_reg(spi, M41T93_REG_ST_SEC, reset_osc);
+		}
+	}
+
 	data[M41T93_REG_SSEC]		= 0;
 	data[M41T93_REG_ST_SEC]		= bin2bcd(tm->tm_sec);
 	data[M41T93_REG_MIN]		= bin2bcd(tm->tm_min);
@@ -89,10 +114,7 @@  static int m41t93_get_time(struct device *dev, struct rtc_time *tm)
 	   1. halt bit (HT) is set: the clock is running but update of readout
 	      registers has been disabled due to power failure. This is normal
 	      case after poweron. Time is valid after resetting HT bit.
-	   2. oscillator fail bit (OF) is set. Oscillator has be stopped and
-	      time is invalid:
-	      a) OF can be immeditely reset.
-	      b) OF cannot be immediately reset: oscillator has to be restarted.
+	   2. oscillator fail bit (OF) is set: time is invalid.
 	*/
 	tmp = spi_w8r8(spi, M41T93_REG_ALM_HOUR_HT);
 	if (tmp < 0)
@@ -110,21 +132,7 @@  static int m41t93_get_time(struct device *dev, struct rtc_time *tm)
 
 	if (tmp & M41T93_FLAG_OF) {
 		ret = -EINVAL;
-		dev_warn(&spi->dev, "OF bit is set, resetting.\n");
-		m41t93_set_reg(spi, M41T93_REG_FLAGS, tmp & ~M41T93_FLAG_OF);
-
-		tmp = spi_w8r8(spi, M41T93_REG_FLAGS);
-		if (tmp < 0)
-			return tmp;
-		else if (tmp & M41T93_FLAG_OF) {
-			u8 reset_osc = buf[M41T93_REG_ST_SEC] | M41T93_FLAG_ST;
-
-			dev_warn(&spi->dev,
-				 "OF bit is still set, kickstarting clock.\n");
-			m41t93_set_reg(spi, M41T93_REG_ST_SEC, reset_osc);
-			reset_osc &= ~M41T93_FLAG_ST;
-			m41t93_set_reg(spi, M41T93_REG_ST_SEC, reset_osc);
-		}
+		dev_warn(&spi->dev, "OF bit is set, write time to restart.\n");
 	}
 
 	if (tmp & M41T93_FLAG_BL)