[patch 1/3] rtc-x1205: fix rtc_time to y2k register value conversion

[patch 1/3] rtc-x1205: fix rtc_time to y2k register value conversion

[Johannes Weiner]

The possible CCR_Y2K register values are 19 or 20 and struct
rtc_time's tm_year is in years since 1900.

The function translating rtc_time to register values assumes tm_year
to be years since first christmas, though, and we end up storing 0 or
1 in the CCR_Y2K register, which the hardware does not refuse to do.

A subsequent probing of the clock fails due to the invalid value range
in the register, though.

[ And if it didn't, reading the clock would yield a bogus year because
  the function translating registers to tm_year is assuming a register
  value of 19 or 20. ]

This fixes the conversion from years since 1900 in tm_year to the
corresponding CCR_Y2K value of 19 or 20.

Signed-off-by: Johannes Weiner <jw@emlix.com>
---
 drivers/rtc/rtc-x1205.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/rtc/rtc-x1205.c b/drivers/rtc/rtc-x1205.c
index 310c107..cc9ba47 100644
--- a/drivers/rtc/rtc-x1205.c
+++ b/drivers/rtc/rtc-x1205.c
@@ -195,7 +195,7 @@ static int x1205_set_datetime(struct i2c_client *client, struct rtc_time *tm,
 		/* year, since the rtc epoch*/
 		buf[CCR_YEAR] = bin2bcd(tm->tm_year % 100);
 		buf[CCR_WDAY] = tm->tm_wday & 0x07;
-		buf[CCR_Y2K] = bin2bcd(tm->tm_year / 100);
+		buf[CCR_Y2K] = bin2bcd((tm->tm_year + 1900) / 100);
 	}
 
 	/* If writing alarm registers, set compare bits on registers 0-4 */
-- 
1.6.5

[patch 2/3] rtc-x1205: reset clock to sane state after power failure

[Johannes Weiner]

When detecting power failure, the probe function would reset the clock
time to defined state.

However, the clock's _date_ might still be bogus and a subsequent
probe fails when sanity-checking these values.

Change the power-failure fixup code to do a full setting of rtc_time,
including a valid date.

Signed-off-by: Johannes Weiner <jw@emlix.com>
---
 drivers/rtc/rtc-x1205.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-x1205.c b/drivers/rtc/rtc-x1205.c
index cc9ba47..6583c1a 100644
--- a/drivers/rtc/rtc-x1205.c
+++ b/drivers/rtc/rtc-x1205.c
@@ -280,9 +280,9 @@ static int x1205_fix_osc(struct i2c_client *client)
 	int err;
 	struct rtc_time tm;
 
-	tm.tm_hour = tm.tm_min = tm.tm_sec = 0;
+	memset(&tm, 0, sizeof(tm));
 
-	err = x1205_set_datetime(client, &tm, 0, X1205_CCR_BASE, 0);
+	err = x1205_set_datetime(client, &tm, 1, X1205_CCR_BASE, 0);
 	if (err < 0)
 		dev_err(&client->dev, "unable to restart the oscillator\n");
 
-- 
1.6.5

[patch 3/3] rtc-x1205: unconditionally set date when setting clock

[Johannes Weiner]

All callsites of x1205_set_datetime() want the date to be set as well,
so remove the flag parameter and set it unconditionally.

Signed-off-by: Johannes Weiner <jw@emlix.com>
---
 drivers/rtc/rtc-x1205.c |   53 ++++++++++++++++-------------------------------
 1 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/drivers/rtc/rtc-x1205.c b/drivers/rtc/rtc-x1205.c
index 6583c1a..9aae491 100644
--- a/drivers/rtc/rtc-x1205.c
+++ b/drivers/rtc/rtc-x1205.c
@@ -155,11 +155,11 @@ static int x1205_get_status(struct i2c_client *client, unsigned char *sr)
 }
 
 static int x1205_set_datetime(struct i2c_client *client, struct rtc_time *tm,
-			int datetoo, u8 reg_base, unsigned char alm_enable)
+			u8 reg_base, unsigned char alm_enable)
 {
-	int i, xfer, nbytes;
-	unsigned char buf[8];
+	int i, xfer;
 	unsigned char rdata[10] = { 0, reg_base };
+	unsigned char *buf = rdata + 2;
 
 	static const unsigned char wel[3] = { 0, X1205_REG_SR,
 						X1205_SR_WEL };
@@ -170,9 +170,9 @@ static int x1205_set_datetime(struct i2c_client *client, struct rtc_time *tm,
 	static const unsigned char diswe[3] = { 0, X1205_REG_SR, 0 };
 
 	dev_dbg(&client->dev,
-		"%s: secs=%d, mins=%d, hours=%d\n",
-		__func__,
-		tm->tm_sec, tm->tm_min, tm->tm_hour);
+		"%s: sec=%d min=%d hour=%d mday=%d mon=%d year=%d wday=%d\n",
+		__func__, tm->tm_sec, tm->tm_min, tm->tm_hour, tm->tm_mday,
+		tm->tm_mon, tm->tm_year, tm->tm_wday);
 
 	buf[CCR_SEC] = bin2bcd(tm->tm_sec);
 	buf[CCR_MIN] = bin2bcd(tm->tm_min);
@@ -180,23 +180,15 @@ static int x1205_set_datetime(struct i2c_client *client, struct rtc_time *tm,
 	/* set hour and 24hr bit */
 	buf[CCR_HOUR] = bin2bcd(tm->tm_hour) | X1205_HR_MIL;
 
-	/* should we also set the date? */
-	if (datetoo) {
-		dev_dbg(&client->dev,
-			"%s: mday=%d, mon=%d, year=%d, wday=%d\n",
-			__func__,
-			tm->tm_mday, tm->tm_mon, tm->tm_year, tm->tm_wday);
+	buf[CCR_MDAY] = bin2bcd(tm->tm_mday);
 
-		buf[CCR_MDAY] = bin2bcd(tm->tm_mday);
+	/* month, 1 - 12 */
+	buf[CCR_MONTH] = bin2bcd(tm->tm_mon + 1);
 
-		/* month, 1 - 12 */
-		buf[CCR_MONTH] = bin2bcd(tm->tm_mon + 1);
-
-		/* year, since the rtc epoch*/
-		buf[CCR_YEAR] = bin2bcd(tm->tm_year % 100);
-		buf[CCR_WDAY] = tm->tm_wday & 0x07;
-		buf[CCR_Y2K] = bin2bcd((tm->tm_year + 1900) / 100);
-	}
+	/* year, since the rtc epoch*/
+	buf[CCR_YEAR] = bin2bcd(tm->tm_year % 100);
+	buf[CCR_WDAY] = tm->tm_wday & 0x07;
+	buf[CCR_Y2K] = bin2bcd((tm->tm_year + 1900) / 100);
 
 	/* If writing alarm registers, set compare bits on registers 0-4 */
 	if (reg_base < X1205_CCR_BASE)
@@ -214,17 +206,8 @@ static int x1205_set_datetime(struct i2c_client *client, struct rtc_time *tm,
 		return -EIO;
 	}
 
-
-	/* write register's data */
-	if (datetoo)
-		nbytes = 8;
-	else
-		nbytes = 3;
-	for (i = 0; i < nbytes; i++)
-		rdata[2+i] = buf[i];
-
-	xfer = i2c_master_send(client, rdata, nbytes+2);
-	if (xfer != nbytes+2) {
+	xfer = i2c_master_send(client, rdata, sizeof(rdata));
+	if (xfer != sizeof(rdata)) {
 		dev_err(&client->dev,
 			"%s: result=%d addr=%02x, data=%02x\n",
 			__func__,
@@ -282,7 +265,7 @@ static int x1205_fix_osc(struct i2c_client *client)
 
 	memset(&tm, 0, sizeof(tm));
 
-	err = x1205_set_datetime(client, &tm, 1, X1205_CCR_BASE, 0);
+	err = x1205_set_datetime(client, &tm, X1205_CCR_BASE, 0);
 	if (err < 0)
 		dev_err(&client->dev, "unable to restart the oscillator\n");
 
@@ -481,7 +464,7 @@ static int x1205_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 static int x1205_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
 	return x1205_set_datetime(to_i2c_client(dev),
-		&alrm->time, 1, X1205_ALM0_BASE, alrm->enabled);
+		&alrm->time, X1205_ALM0_BASE, alrm->enabled);
 }
 
 static int x1205_rtc_read_time(struct device *dev, struct rtc_time *tm)
@@ -493,7 +476,7 @@ static int x1205_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int x1205_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	return x1205_set_datetime(to_i2c_client(dev),
-		tm, 1, X1205_CCR_BASE, 0);
+		tm, X1205_CCR_BASE, 0);
 }
 
 static int x1205_rtc_proc(struct device *dev, struct seq_file *seq)
-- 
1.6.5

Re: [patch 1/3] rtc-x1205: fix rtc_time to y2k register value conversion

[Andrew Morton]

I'm thinking that patches 1 and 2 are 2.6.32 material?

Re: [patch 1/3] rtc-x1205: fix rtc_time to y2k register value conversion

[Johannes Weiner]

On Wed, Nov 25, 2009 at 02:11:00PM -0800, Andrew Morton wrote:
> I'm thinking that patches 1 and 2 are 2.6.32 material?

Yeah, I think so too, although I hope we can get feedback from the
authors beforehand.

Because #1 looks obvious to me and makes the driver at all usable for
me but the code has been like that since the initial merge in 2006.
So either

	a) nobody ever set the date with this thing,
	b) every embedded linux company has its own private patch or
	c) I am missing something,

where likelyhood is in reverse order.  That makes me feel slightly
uncomfortable.

Re: [patch 1/3] rtc-x1205: fix rtc_time to y2k register value conversion

[Alessandro Zummo]

On Thu, 26 Nov 2009 02:02:34 +0100
Johannes Weiner <jw@emlix.com> wrote:

> eah, I think so too, although I hope we can get feedback from the
> authors beforehand.
> 
> Because #1 looks obvious to me and makes the driver at all usable for
> me but the code has been like that since the initial merge in 2006.
> So either
> 
> 	a) nobody ever set the date with this thing,
> 	b) every embedded linux company has its own private patch or
> 	c) I am missing something,
> 
> where likelyhood is in reverse order.  That makes me feel slightly
> uncomfortable.

 The last time I used an x1205 it worked nicely. It's been two or three
 years since. However the datasheet says 0x19 o 0x20 so your patch
 looks good.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it