diff mbox

RTC: fm3130 fixes

Message ID 1256369471-11901-1-git-send-email-geomatsi@gmail.com
State Superseded, archived
Headers show

Commit Message

Sergey Matyukevich Oct. 24, 2009, 7:31 a.m. UTC 
This patch contains the following fixes and enhancements 
for fm3130 rtc driver:

* add missing braces for multiline 'if' statements in fm3130_probe

* add sanity check for alarm data in fm3130_probe

* fix fm3130_set_alarm:
	according to datasheet, setting match bit '0' indicates
	that the corresponding alarm field will be used in the
	match process

* add ioctls for RTC_AIE_ON, RTC_AIE_OFF

* Remove clearing of AF bit after reading rtc/alarm control register:
	according to datasheet this bit is cleared 
	anyway when rtc/alarm control register is read

* add whitespaces for '#define' to improve readability


Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>

---
 drivers/rtc/rtc-fm3130.c |  213 ++++++++++++++++++++++++++++++++--------------
 1 files changed, 148 insertions(+), 65 deletions(-)

Comments

Sergey Lapin Oct. 24, 2009, 8:29 a.m. UTC | #1 
On Sat, Oct 24, 2009 at 11:31:11AM +0400, Sergey Matyukevich wrote:
> 
> This patch contains the following fixes and enhancements 
> for fm3130 rtc driver:
> 
> * add missing braces for multiline 'if' statements in fm3130_probe
> 
> * add sanity check for alarm data in fm3130_probe
> 
> * fix fm3130_set_alarm:
> 	according to datasheet, setting match bit '0' indicates
> 	that the corresponding alarm field will be used in the
> 	match process
> 
> * add ioctls for RTC_AIE_ON, RTC_AIE_OFF
> 
> * Remove clearing of AF bit after reading rtc/alarm control register:
> 	according to datasheet this bit is cleared 
> 	anyway when rtc/alarm control register is read
> 
> * add whitespaces for '#define' to improve readability
> 
> 
> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>

Hi, Sergey!
Patch is mostly ok, but please split it into parts which add new
functionality and parts which fix bugs. Also, please have excessive whitespace
changes separate of functional changes.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---
Sergey Matyukevich Oct. 31, 2009, 4:56 p.m. UTC | #2 
Hi Alessandro,

Could you please take a look at the second version of patches for
fm3130 driver. The driver's author has already made his review. 
Is there a chance for those changes to be accepted now or later ? 

P.S. 	Just in the case this letter will not be linked to a proper
	mail thread in rtc-linux mail list, here is a direct link:
	http://groups.google.com/group/rtc-linux/browse_thread/thread/740ba9cd535b6cee#	

Thanks, 
Sergey

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---
diff mbox

Patch

diff --git a/drivers/rtc/rtc-fm3130.c b/drivers/rtc/rtc-fm3130.c
index 3a7be11..27e8616 100644
--- a/drivers/rtc/rtc-fm3130.c
+++ b/drivers/rtc/rtc-fm3130.c
@@ -31,15 +31,15 @@ 
 #define FM3130_ALARM_MONTHS	(0xd)
 #define FM3130_ALARM_WP_CONTROL	(0xe)
 
-#define FM3130_CAL_CONTROL_BIT_nOSCEN (1 << 7) /* Osciallator enabled */
-#define FM3130_RTC_CONTROL_BIT_LB (1 << 7) /* Low battery */
-#define FM3130_RTC_CONTROL_BIT_AF (1 << 6) /* Alarm flag */
-#define FM3130_RTC_CONTROL_BIT_CF (1 << 5) /* Century overflow */
-#define FM3130_RTC_CONTROL_BIT_POR (1 << 4) /* Power on reset */
-#define FM3130_RTC_CONTROL_BIT_AEN (1 << 3) /* Alarm enable */
-#define FM3130_RTC_CONTROL_BIT_CAL (1 << 2) /* Calibration mode */
-#define FM3130_RTC_CONTROL_BIT_WRITE (1 << 1) /* W=1 -> write mode W=0 normal */
-#define FM3130_RTC_CONTROL_BIT_READ (1 << 0) /* R=1 -> read mode R=0 normal */
+#define FM3130_CAL_CONTROL_BIT_nOSCEN 	(1 << 7) /* Osciallator enabled */
+#define FM3130_RTC_CONTROL_BIT_LB 	(1 << 7) /* Low battery */
+#define FM3130_RTC_CONTROL_BIT_AF 	(1 << 6) /* Alarm flag */
+#define FM3130_RTC_CONTROL_BIT_CF 	(1 << 5) /* Century overflow */
+#define FM3130_RTC_CONTROL_BIT_POR 	(1 << 4) /* Power on reset */
+#define FM3130_RTC_CONTROL_BIT_AEN 	(1 << 3) /* Alarm enable */
+#define FM3130_RTC_CONTROL_BIT_CAL 	(1 << 2) /* Calibration mode */
+#define FM3130_RTC_CONTROL_BIT_WRITE 	(1 << 1) /* Write RTC */
+#define FM3130_RTC_CONTROL_BIT_READ 	(1 << 0) /* Read RTC */
 
 #define FM3130_CLOCK_REGS 7
 #define FM3130_ALARM_REGS 5
@@ -51,8 +51,8 @@  struct fm3130 {
 	struct i2c_msg		msg[4];
 	struct i2c_client	*client;
 	struct rtc_device	*rtc;
+	int			alarm_valid;
 	int			data_valid;
-	int			alarm;
 };
 static const struct i2c_device_id fm3130_id[] = {
 	{ "fm3130", 0 },
@@ -74,7 +74,7 @@  static void fm3130_rtc_mode(struct device *dev, int mode)
 	case FM3130_MODE_NORMAL:
 		fm3130->regs[FM3130_RTC_CONTROL] &=
 			~(FM3130_RTC_CONTROL_BIT_WRITE |
-			FM3130_RTC_CONTROL_BIT_READ);
+				FM3130_RTC_CONTROL_BIT_READ);
 		break;
 	case FM3130_MODE_WRITE:
 		fm3130->regs[FM3130_RTC_CONTROL] |= FM3130_RTC_CONTROL_BIT_WRITE;
@@ -86,11 +86,7 @@  static void fm3130_rtc_mode(struct device *dev, int mode)
 		dev_dbg(dev, "invalid mode %d\n", mode);
 		break;
 	}
-	/* Checking for alarm */
-	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_AF) {
-		fm3130->alarm = 1;
-		fm3130->regs[FM3130_RTC_CONTROL] &= ~FM3130_RTC_CONTROL_BIT_AF;
-	}
+
 	i2c_smbus_write_byte_data(fm3130->client,
 		 FM3130_RTC_CONTROL, fm3130->regs[FM3130_RTC_CONTROL]);
 }
@@ -103,7 +99,7 @@  static int fm3130_get_time(struct device *dev, struct rtc_time *t)
 	if (!fm3130->data_valid) {
 		/* We have invalid data in RTC, probably due
 		to battery faults or other problems. Return EIO
-		for now, it will allow us to set data later insted
+		for now, it will allow us to set data later instead
 		of error during probing which disables device */
 		return -EIO;
 	}
@@ -153,7 +149,6 @@  static int fm3130_get_time(struct device *dev, struct rtc_time *t)
 	return rtc_valid_tm(t);
 }
 
-
 static int fm3130_set_time(struct device *dev, struct rtc_time *t)
 {
 	struct fm3130 *fm3130 = dev_get_drvdata(dev);
@@ -207,6 +202,15 @@  static int fm3130_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	struct fm3130 *fm3130 = dev_get_drvdata(dev);
 	int tmp;
 	struct rtc_time *tm = &alrm->time;
+
+	if (!fm3130->alarm_valid) {
+		/* We have invalid alarm in RTC, probably due
+		to battery faults or other problems. Return EIO
+		for now, it will allow us to set alarm value later
+		instead of error during probing which disables device */
+		return -EIO;
+	}
+
 	/* read the RTC alarm registers all at once */
 	tmp = i2c_transfer(to_i2c_adapter(fm3130->client->dev.parent),
 			&fm3130->msg[2], 2);
@@ -214,6 +218,7 @@  static int fm3130_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 		dev_err(dev, "%s error %d\n", "read", tmp);
 		return -EIO;
 	}
+
 	dev_dbg(dev, "alarm read %02x %02x %02x %02x %02x\n",
 			fm3130->regs[FM3130_ALARM_SECONDS],
 			fm3130->regs[FM3130_ALARM_MINUTES],
@@ -221,20 +226,30 @@  static int fm3130_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 			fm3130->regs[FM3130_ALARM_DATE],
 			fm3130->regs[FM3130_ALARM_MONTHS]);
 
-
 	tm->tm_sec	= bcd2bin(fm3130->regs[FM3130_ALARM_SECONDS] & 0x7F);
 	tm->tm_min	= bcd2bin(fm3130->regs[FM3130_ALARM_MINUTES] & 0x7F);
 	tm->tm_hour	= bcd2bin(fm3130->regs[FM3130_ALARM_HOURS] & 0x3F);
 	tm->tm_mday	= bcd2bin(fm3130->regs[FM3130_ALARM_DATE] & 0x3F);
 	tm->tm_mon	= bcd2bin(fm3130->regs[FM3130_ALARM_MONTHS] & 0x1F);
+
 	if (tm->tm_mon > 0)
 		tm->tm_mon -= 1; /* RTC is 1-12, tm_mon is 0-11 */
+
 	dev_dbg(dev, "%s secs=%d, mins=%d, "
 		"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
 		"read alarm", tm->tm_sec, tm->tm_min,
 		tm->tm_hour, tm->tm_mday,
 		tm->tm_mon, tm->tm_year, tm->tm_wday);
 
+	/* check if alarm enabled */
+	fm3130->regs[FM3130_RTC_CONTROL] =
+		i2c_smbus_read_byte_data(fm3130->client, FM3130_RTC_CONTROL);
+
+	if ((fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_AEN) &&
+		(~fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_CAL)) {
+		alrm->enabled = 1;
+	}
+
 	return 0;
 }
 
@@ -250,25 +265,20 @@  static int fm3130_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 		tm->tm_hour, tm->tm_mday,
 		tm->tm_mon, tm->tm_year, tm->tm_wday);
 
-	if (tm->tm_sec != -1)
-		fm3130->regs[FM3130_ALARM_SECONDS] =
-			bin2bcd(tm->tm_sec) | 0x80;
+	fm3130->regs[FM3130_ALARM_SECONDS] =
+		(tm->tm_sec != -1) ? bin2bcd(tm->tm_sec) : 0x80;
 
-	if (tm->tm_min != -1)
-		fm3130->regs[FM3130_ALARM_MINUTES] =
-			bin2bcd(tm->tm_min) | 0x80;
+	fm3130->regs[FM3130_ALARM_MINUTES] =
+		(tm->tm_min != -1) ? bin2bcd(tm->tm_min) : 0x80;
 
-	if (tm->tm_hour != -1)
-		fm3130->regs[FM3130_ALARM_HOURS] =
-			bin2bcd(tm->tm_hour) | 0x80;
+	fm3130->regs[FM3130_ALARM_HOURS] =
+		(tm->tm_hour != -1) ? bin2bcd(tm->tm_hour) : 0x80;
 
-	if (tm->tm_mday != -1)
-		fm3130->regs[FM3130_ALARM_DATE] =
-			bin2bcd(tm->tm_mday) | 0x80;
+	fm3130->regs[FM3130_ALARM_DATE] =
+		(tm->tm_mday != -1) ? bin2bcd(tm->tm_mday) : 0x80;
 
-	if (tm->tm_mon != -1)
-		fm3130->regs[FM3130_ALARM_MONTHS] =
-			bin2bcd(tm->tm_mon + 1) | 0x80;
+	fm3130->regs[FM3130_ALARM_MONTHS] =
+		(tm->tm_mon != -1) ? bin2bcd(tm->tm_mon + 1) : 0x80;
 
 	dev_dbg(dev, "alarm write %02x %02x %02x %02x %02x\n",
 			fm3130->regs[FM3130_ALARM_SECONDS],
@@ -276,19 +286,18 @@  static int fm3130_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 			fm3130->regs[FM3130_ALARM_HOURS],
 			fm3130->regs[FM3130_ALARM_DATE],
 			fm3130->regs[FM3130_ALARM_MONTHS]);
+
 	/* Writing time registers, we don't support multibyte transfers */
 	for (i = 0; i < FM3130_ALARM_REGS; i++) {
 		i2c_smbus_write_byte_data(fm3130->client,
 					FM3130_ALARM_SECONDS + i,
 					fm3130->regs[FM3130_ALARM_SECONDS + i]);
 	}
+
 	fm3130->regs[FM3130_RTC_CONTROL] =
 		i2c_smbus_read_byte_data(fm3130->client, FM3130_RTC_CONTROL);
-	/* Checking for alarm */
-	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_AF) {
-		fm3130->alarm = 1;
-		fm3130->regs[FM3130_RTC_CONTROL] &= ~FM3130_RTC_CONTROL_BIT_AF;
-	}
+
+	/* enable or disable alarm */
 	if (alrm->enabled) {
 		i2c_smbus_write_byte_data(fm3130->client, FM3130_RTC_CONTROL,
 			(fm3130->regs[FM3130_RTC_CONTROL] &
@@ -297,12 +306,52 @@  static int fm3130_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	} else {
 		i2c_smbus_write_byte_data(fm3130->client, FM3130_RTC_CONTROL,
 			fm3130->regs[FM3130_RTC_CONTROL] &
-				~(FM3130_RTC_CONTROL_BIT_AEN));
+				~(FM3130_RTC_CONTROL_BIT_CAL) &
+					~(FM3130_RTC_CONTROL_BIT_AEN));
 	}
+
+	/* We assume here that data is valid once written */
+	if (!fm3130->alarm_valid)
+		fm3130->alarm_valid = 1;
+
 	return 0;
 }
 
+static int fm3130_ioctl(struct device *dev, unsigned int cmd,
+			unsigned long arg)
+{
+	struct fm3130 *fm3130 = dev_get_drvdata(dev);
+	int ret = 0;
+
+	fm3130->regs[FM3130_RTC_CONTROL] =
+		i2c_smbus_read_byte_data(fm3130->client, FM3130_RTC_CONTROL);
+
+	dev_dbg(dev, "ioctl: cmd=%08x, FM3130_RTC_CONTROL=%08x\n",
+			cmd, fm3130->regs[FM3130_RTC_CONTROL]);
+
+	switch (cmd) {
+	case RTC_AIE_OFF:		/* alarm off */
+		ret = i2c_smbus_write_byte_data(fm3130->client,
+			FM3130_RTC_CONTROL, fm3130->regs[FM3130_RTC_CONTROL] &
+				~(FM3130_RTC_CONTROL_BIT_CAL) &
+					~(FM3130_RTC_CONTROL_BIT_AEN));
+		break;
+	case RTC_AIE_ON:		/* alarm on */
+		ret = i2c_smbus_write_byte_data(fm3130->client,
+			FM3130_RTC_CONTROL, (fm3130->regs[FM3130_RTC_CONTROL] &
+				~(FM3130_RTC_CONTROL_BIT_CAL)) |
+					FM3130_RTC_CONTROL_BIT_AEN);
+		break;
+	default:
+		ret = -ENOIOCTLCMD;
+		break;
+	}
+
+	return ret;
+}
+
 static const struct rtc_class_ops fm3130_rtc_ops = {
+	.ioctl		= fm3130_ioctl,
 	.read_time	= fm3130_get_time,
 	.set_time	= fm3130_set_time,
 	.read_alarm	= fm3130_read_alarm,
@@ -355,6 +404,7 @@  static int __devinit fm3130_probe(struct i2c_client *client,
 	fm3130->msg[3].len = FM3130_ALARM_REGS;
 	fm3130->msg[3].buf = &fm3130->regs[FM3130_ALARM_SECONDS];
 
+	fm3130->alarm_valid = 0;
 	fm3130->data_valid = 0;
 
 	tmp = i2c_transfer(adapter, fm3130->msg, 4);
@@ -366,30 +416,27 @@  static int __devinit fm3130_probe(struct i2c_client *client,
 
 	fm3130->regs[FM3130_RTC_CONTROL] =
 		i2c_smbus_read_byte_data(client, FM3130_RTC_CONTROL);
+
 	fm3130->regs[FM3130_CAL_CONTROL] =
 		i2c_smbus_read_byte_data(client, FM3130_CAL_CONTROL);
 
-	/* Checking for alarm */
-	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_AF) {
-		fm3130->alarm = 1;
-		fm3130->regs[FM3130_RTC_CONTROL] &= ~FM3130_RTC_CONTROL_BIT_AF;
-	}
-
 	/* Disabling calibration mode */
-	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_CAL)
+	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_CAL) {
 		i2c_smbus_write_byte_data(client, FM3130_RTC_CONTROL,
 			fm3130->regs[FM3130_RTC_CONTROL] &
 				~(FM3130_RTC_CONTROL_BIT_CAL));
 		dev_warn(&client->dev, "Disabling calibration mode!\n");
+	}
 
 	/* Disabling read and write modes */
 	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_WRITE ||
-	    fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_READ)
+	    fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_READ) {
 		i2c_smbus_write_byte_data(client, FM3130_RTC_CONTROL,
 			fm3130->regs[FM3130_RTC_CONTROL] &
 				~(FM3130_RTC_CONTROL_BIT_READ |
 					FM3130_RTC_CONTROL_BIT_WRITE));
 		dev_warn(&client->dev, "Disabling READ or WRITE mode!\n");
+	}
 
 	/* oscillator off?  turn it on, so clock can tick. */
 	if (fm3130->regs[FM3130_CAL_CONTROL] & FM3130_CAL_CONTROL_BIT_nOSCEN)
@@ -397,44 +444,80 @@  static int __devinit fm3130_probe(struct i2c_client *client,
 			fm3130->regs[FM3130_CAL_CONTROL] &
 				~(FM3130_CAL_CONTROL_BIT_nOSCEN));
 
-	/* oscillator fault?  clear flag, and warn */
-	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_LB)
+	/* low battery?  clear flag, and warn */
+	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_LB) {
+		i2c_smbus_write_byte_data(client, FM3130_RTC_CONTROL,
+			fm3130->regs[FM3130_RTC_CONTROL] &
+				~(FM3130_RTC_CONTROL_BIT_LB));
 		dev_warn(&client->dev, "Low battery!\n");
+	}
 
-	/* oscillator fault?  clear flag, and warn */
+	/* check if Power On Reset bit is set */
 	if (fm3130->regs[FM3130_RTC_CONTROL] & FM3130_RTC_CONTROL_BIT_POR) {
 		i2c_smbus_write_byte_data(client, FM3130_RTC_CONTROL,
 			fm3130->regs[FM3130_RTC_CONTROL] &
 				~FM3130_RTC_CONTROL_BIT_POR);
-		dev_warn(&client->dev, "SET TIME!\n");
+		dev_dbg(&client->dev, "POR bit is set\n");
 	}
+
 	/* ACS is controlled by alarm */
 	i2c_smbus_write_byte_data(client, FM3130_ALARM_WP_CONTROL, 0x80);
 
-	/* TODO */
-	/* TODO need to sanity check alarm */
-	tmp = fm3130->regs[FM3130_RTC_SECONDS];
-	tmp = bcd2bin(tmp & 0x7f);
-	if (tmp > 60)
-		goto exit_bad;
+	/* alarm registers sanity check */
+	tmp = bcd2bin(fm3130->regs[FM3130_RTC_SECONDS] & 0x7f);
+	if (tmp > 59)
+		goto bad_alarm;
+
 	tmp = bcd2bin(fm3130->regs[FM3130_RTC_MINUTES] & 0x7f);
-	if (tmp > 60)
-		goto exit_bad;
+	if (tmp > 59)
+		goto bad_alarm;
+
+	tmp = bcd2bin(fm3130->regs[FM3130_RTC_HOURS] & 0x3f);
+	if (tmp > 23)
+		goto bad_alarm;
 
 	tmp = bcd2bin(fm3130->regs[FM3130_RTC_DATE] & 0x3f);
 	if (tmp == 0 || tmp > 31)
-		goto exit_bad;
+		goto bad_alarm;
 
 	tmp = bcd2bin(fm3130->regs[FM3130_RTC_MONTHS] & 0x1f);
 	if (tmp == 0 || tmp > 12)
-		goto exit_bad;
+		goto bad_alarm;
+
+	fm3130->alarm_valid = 1;
+
+bad_alarm:
+
+	/* clock registers sanity chek */
+	tmp = bcd2bin(fm3130->regs[FM3130_RTC_SECONDS] & 0x7f);
+	if (tmp > 59)
+		goto bad_clock;
+
+	tmp = bcd2bin(fm3130->regs[FM3130_RTC_MINUTES] & 0x7f);
+	if (tmp > 59)
+		goto bad_clock;
+
+	tmp = bcd2bin(fm3130->regs[FM3130_RTC_HOURS] & 0x3f);
+	if (tmp > 23)
+		goto bad_clock;
 
-	tmp = fm3130->regs[FM3130_RTC_HOURS];
+	tmp = bcd2bin(fm3130->regs[FM3130_RTC_DAY] & 0x7);
+	if (tmp == 0 || tmp > 7)
+		goto bad_clock;
+
+	tmp = bcd2bin(fm3130->regs[FM3130_RTC_DATE] & 0x3f);
+	if (tmp == 0 || tmp > 31)
+		goto bad_clock;
+
+	tmp = bcd2bin(fm3130->regs[FM3130_RTC_MONTHS] & 0x1f);
+	if (tmp == 0 || tmp > 12)
+		goto bad_clock;
 
 	fm3130->data_valid = 1;
 
-exit_bad:
-	if (!fm3130->data_valid)
+bad_clock:
+
+	if (!fm3130->data_valid || !fm3130->alarm_valid)
 		dev_dbg(&client->dev,
 				"%s: %02x %02x %02x %02x %02x %02x %02x %02x"
 				"%02x %02x %02x %02x %02x %02x %02x\n",