[PATCH 2.6.16-mm1 1/3] rtc: m41t00 driver should use workqueue instead of tasklet

[PATCH 2.6.16-mm1 1/3] rtc: m41t00 driver should use workqueue instead of tasklet

[Mark A. Greer]

The m41t00 i2c/rtc driver currently uses a tasklet to schedule interrupt-level
writes to the rtc.  This patch causes the driver to use a workqueue instead.

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
---

 m41t00.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
---

diff -Nurp linux-2.6.16-mm1/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-wq/drivers/i2c/chips/m41t00.c
--- linux-2.6.16-mm1/drivers/i2c/chips/m41t00.c	2006-03-23 15:04:55.000000000 -0700
+++ linux-2.6.16-mm1-wq/drivers/i2c/chips/m41t00.c	2006-03-23 16:04:01.000000000 -0700
@@ -25,6 +25,7 @@
 #include <linux/rtc.h>
 #include <linux/bcd.h>
 #include <linux/mutex.h>
+#include <linux/workqueue.h>
 
 #include <asm/time.h>
 #include <asm/rtc.h>
@@ -32,6 +33,7 @@
 #define	M41T00_DRV_NAME		"m41t00"
 
 static DEFINE_MUTEX(m41t00_mutex);
+static struct work_struct set_rtc_time_task;
 
 static struct i2c_driver m41t00_driver;
 static struct i2c_client *save_client;
@@ -111,7 +113,7 @@ m41t00_get_rtc_time(void)
 }
 
 static void
-m41t00_set_tlet(ulong arg)
+m41t00_set(void *arg)
 {
 	struct rtc_time	tm;
 	ulong	nowtime = *(ulong *)arg;
@@ -147,17 +149,15 @@ m41t00_set_tlet(ulong arg)
 
 static ulong	new_time;
 
-DECLARE_TASKLET_DISABLED(m41t00_tasklet, m41t00_set_tlet, (ulong)&new_time);
-
 int
 m41t00_set_rtc_time(ulong nowtime)
 {
 	new_time = nowtime;
 
 	if (in_interrupt())
-		tasklet_schedule(&m41t00_tasklet);
+		schedule_work(&set_rtc_time_task);
 	else
-		m41t00_set_tlet((ulong)&new_time);
+		m41t00_set((void *)&new_time);
 
 	return 0;
 }
@@ -189,6 +189,7 @@ m41t00_probe(struct i2c_adapter *adap, i
 		return rc;
 	}
 
+	INIT_WORK(&set_rtc_time_task, &m41t00_set, &new_time);
 	save_client = client;
 	return 0;
 }
@@ -206,7 +207,7 @@ m41t00_detach(struct i2c_client *client)
 
 	if ((rc = i2c_detach_client(client)) == 0) {
 		kfree(client);
-		tasklet_kill(&m41t00_tasklet);
+		flush_scheduled_work();
 	}
 	return rc;
 }

Re: [PATCH 2.6.16-mm1 1/3] rtc: m41t00 driver should use workqueue instead of tasklet

[Jean Delvare]

Hi Mark,

> The m41t00 i2c/rtc driver currently uses a tasklet to schedule interrupt-level
> writes to the rtc.  This patch causes the driver to use a workqueue instead.
> (...)
>  int
>  m41t00_set_rtc_time(ulong nowtime)
>  {
>  	new_time = nowtime;
>  
>  	if (in_interrupt())
> -		tasklet_schedule(&m41t00_tasklet);
> +		schedule_work(&set_rtc_time_task);
>  	else
> -		m41t00_set_tlet((ulong)&new_time);
> +		m41t00_set((void *)&new_time);

This cast is not needed.

>  
>  	return 0;
>  }

Thanks,
-- 
Jean Delvare

Re: [PATCH 2.6.16-mm1 1/3] rtc: m41t00 driver should use workqueue instead of tasklet

[Mark A. Greer]

Resend...
---

The m41t00 i2c/rtc driver currently uses a tasklet to schedule
interrupt-level writes to the rtc.  This patch causes the driver
to use a workqueue instead.

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
---

 m41t00.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)
---

diff -Nurp linux-2.6.16-mm1/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-wq/drivers/i2c/chips/m41t00.c
--- linux-2.6.16-mm1/drivers/i2c/chips/m41t00.c	2006-03-23 15:04:55.000000000 -0700
+++ linux-2.6.16-mm1-wq/drivers/i2c/chips/m41t00.c	2006-03-27 15:29:35.000000000 -0700
@@ -25,6 +25,7 @@
 #include <linux/rtc.h>
 #include <linux/bcd.h>
 #include <linux/mutex.h>
+#include <linux/workqueue.h>
 
 #include <asm/time.h>
 #include <asm/rtc.h>
@@ -111,7 +112,7 @@ m41t00_get_rtc_time(void)
 }
 
 static void
-m41t00_set_tlet(ulong arg)
+m41t00_set(void *arg)
 {
 	struct rtc_time	tm;
 	ulong	nowtime = *(ulong *)arg;
@@ -145,9 +146,9 @@ m41t00_set_tlet(ulong arg)
 	return;
 }
 
-static ulong	new_time;
-
-DECLARE_TASKLET_DISABLED(m41t00_tasklet, m41t00_set_tlet, (ulong)&new_time);
+static ulong new_time;
+static struct workqueue_struct *m41t00_wq;
+static DECLARE_WORK(m41t00_work, m41t00_set, &new_time);
 
 int
 m41t00_set_rtc_time(ulong nowtime)
@@ -155,9 +156,9 @@ m41t00_set_rtc_time(ulong nowtime)
 	new_time = nowtime;
 
 	if (in_interrupt())
-		tasklet_schedule(&m41t00_tasklet);
+		queue_work(m41t00_wq, &m41t00_work);
 	else
-		m41t00_set_tlet((ulong)&new_time);
+		m41t00_set(&new_time);
 
 	return 0;
 }
@@ -189,6 +190,7 @@ m41t00_probe(struct i2c_adapter *adap, i
 		return rc;
 	}
 
+	m41t00_wq = create_singlethread_workqueue("m41t00");
 	save_client = client;
 	return 0;
 }
@@ -206,7 +208,7 @@ m41t00_detach(struct i2c_client *client)
 
 	if ((rc = i2c_detach_client(client)) == 0) {
 		kfree(client);
-		tasklet_kill(&m41t00_tasklet);
+		destroy_workqueue(m41t00_wq);
 	}
 	return rc;
 }

[PATCH 2.6.16-mm1 2/3] rtc: m41t00 driver cleanup

[Mark A. Greer]

This patch does some cleanup to the m41t00 i2c/rtc driver including:
- use BCD2BIN/BIN2BCD instead of BCD_TO_BIN/BIN_TO_BCD
- use strlcpy instead of strncpy
- some whitespace cleanup

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
---

 m41t00.c |   52 +++++++++++++++++++++++-----------------------------
 1 files changed, 23 insertions(+), 29 deletions(-)
---

diff -Nurp linux-2.6.16-mm1-wq/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c
--- linux-2.6.16-mm1-wq/drivers/i2c/chips/m41t00.c	2006-03-23 16:04:01.000000000 -0700
+++ linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c	2006-03-23 16:16:35.000000000 -0700
@@ -1,6 +1,4 @@
 /*
- * drivers/i2c/chips/m41t00.c
- *
  * I2C client/driver for the ST M41T00 Real-Time Clock chip.
  *
  * Author: Mark A. Greer <mgreer@mvista.com>
@@ -13,9 +11,6 @@
 /*
  * This i2c client/driver wedges between the drivers/char/genrtc.c RTC
  * interface and the SMBus interface of the i2c subsystem.
- * It would be more efficient to use i2c msgs/i2c_transfer directly but, as
- * recommened in .../Documentation/i2c/writing-clients section
- * "Sending and receiving", using SMBus level communication is preferred.
  */
 
 #include <linux/kernel.h>
@@ -42,17 +37,17 @@ static unsigned short ignore[] = { I2C_C
 static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
 
 static struct i2c_client_address_data addr_data = {
-	.normal_i2c		= normal_addr,
-	.probe			= ignore,
-	.ignore			= ignore,
+	.normal_i2c	= normal_addr,
+	.probe		= ignore,
+	.ignore		= ignore,
 };
 
 ulong
 m41t00_get_rtc_time(void)
 {
-	s32	sec, min, hour, day, mon, year;
-	s32	sec1, min1, hour1, day1, mon1, year1;
-	ulong	limit = 10;
+	s32 sec, min, hour, day, mon, year;
+	s32 sec1, min1, hour1, day1, mon1, year1;
+	ulong limit = 10;
 
 	sec = min = hour = day = mon = year = 0;
 	sec1 = min1 = hour1 = day1 = mon1 = year1 = 0;
@@ -98,12 +93,12 @@ m41t00_get_rtc_time(void)
 	mon &= 0x1f;
 	year &= 0xff;
 
-	BCD_TO_BIN(sec);
-	BCD_TO_BIN(min);
-	BCD_TO_BIN(hour);
-	BCD_TO_BIN(day);
-	BCD_TO_BIN(mon);
-	BCD_TO_BIN(year);
+	sec = BCD2BIN(sec);
+	min = BCD2BIN(min);
+	hour = BCD2BIN(hour);
+	day = BCD2BIN(day);
+	mon = BCD2BIN(mon);
+	year = BCD2BIN(year);
 
 	year += 1900;
 	if (year < 1970)
@@ -116,17 +111,17 @@ static void
 m41t00_set(void *arg)
 {
 	struct rtc_time	tm;
-	ulong	nowtime = *(ulong *)arg;
+	ulong nowtime = *(ulong *)arg;
 
 	to_tm(nowtime, &tm);
 	tm.tm_year = (tm.tm_year - 1900) % 100;
 
-	BIN_TO_BCD(tm.tm_sec);
-	BIN_TO_BCD(tm.tm_min);
-	BIN_TO_BCD(tm.tm_hour);
-	BIN_TO_BCD(tm.tm_mon);
-	BIN_TO_BCD(tm.tm_mday);
-	BIN_TO_BCD(tm.tm_year);
+	tm.tm_sec = BIN2BCD(tm.tm_sec);
+	tm.tm_min = BIN2BCD(tm.tm_min);
+	tm.tm_hour = BIN2BCD(tm.tm_hour);
+	tm.tm_mon = BIN2BCD(tm.tm_mon);
+	tm.tm_mday = BIN2BCD(tm.tm_mday);
+	tm.tm_year = BIN2BCD(tm.tm_year);
 
 	mutex_lock(&m41t00_mutex);
 	if ((i2c_smbus_write_byte_data(save_client, 0, tm.tm_sec & 0x7f) < 0)
@@ -144,10 +139,9 @@ m41t00_set(void *arg)
 		dev_warn(&save_client->dev,"m41t00: can't write to rtc chip\n");
 
 	mutex_unlock(&m41t00_mutex);
-	return;
 }
 
-static ulong	new_time;
+static ulong new_time;
 
 int
 m41t00_set_rtc_time(ulong nowtime)
@@ -179,7 +173,7 @@ m41t00_probe(struct i2c_adapter *adap, i
 	if (!client)
 		return -ENOMEM;
 
-	strncpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
+	strlcpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
 	client->addr = addr;
 	client->adapter = adap;
 	client->driver = &m41t00_driver;
@@ -203,7 +197,7 @@ m41t00_attach(struct i2c_adapter *adap)
 static int
 m41t00_detach(struct i2c_client *client)
 {
-	int	rc;
+	int rc;
 
 	if ((rc = i2c_detach_client(client)) == 0) {
 		kfree(client);
@@ -214,6 +208,7 @@ m41t00_detach(struct i2c_client *client)
 
 static struct i2c_driver m41t00_driver = {
 	.driver = {
+		.owner	= THIS_MODULE,
 		.name	= M41T00_DRV_NAME,
 	},
 	.id		= I2C_DRIVERID_STM41T00,
@@ -231,7 +226,6 @@ static void __exit
 m41t00_exit(void)
 {
 	i2c_del_driver(&m41t00_driver);
-	return;
 }
 
 module_init(m41t00_init);

Re: [PATCH 2.6.16-mm1 2/3] rtc: m41t00 driver cleanup

[Jean Delvare]

Hi Mark,

> This patch does some cleanup to the m41t00 i2c/rtc driver including:
> - use BCD2BIN/BIN2BCD instead of BCD_TO_BIN/BIN_TO_BCD
> - use strlcpy instead of strncpy
> - some whitespace cleanup

Looks overall good, except:

> @@ -214,6 +208,7 @@ m41t00_detach(struct i2c_client *client)
>  
>  static struct i2c_driver m41t00_driver = {
>  	.driver = {
> +		.owner	= THIS_MODULE,
>  		.name	= M41T00_DRV_NAME,
>  	},
>  	.id		= I2C_DRIVERID_STM41T00,

i2c_add_driver sets the owner for you, so it was omitted here on
purpose.

I'll drop that change before pushing the patch to Greg, no need to
resend.

Thanks,
-- 
Jean Delvare

Re: [PATCH 2.6.16-mm1 2/3] rtc: m41t00 driver cleanup

[Mark A. Greer]

On Mon, Mar 27, 2006 at 07:11:11PM +0200, Jean Delvare wrote:
> Hi Mark,
> 
> > This patch does some cleanup to the m41t00 i2c/rtc driver including:
> > - use BCD2BIN/BIN2BCD instead of BCD_TO_BIN/BIN_TO_BCD
> > - use strlcpy instead of strncpy
> > - some whitespace cleanup
> 
> Looks overall good, except:
> 
> > @@ -214,6 +208,7 @@ m41t00_detach(struct i2c_client *client)
> >  
> >  static struct i2c_driver m41t00_driver = {
> >  	.driver = {
> > +		.owner	= THIS_MODULE,
> >  		.name	= M41T00_DRV_NAME,
> >  	},
> >  	.id		= I2C_DRIVERID_STM41T00,
> 
> i2c_add_driver sets the owner for you, so it was omitted here on
> purpose.
> 
> I'll drop that change before pushing the patch to Greg, no need to
> resend.

Hang on for a bit, Jean.  I'm making a new set of patches with yours and
Andrew's comments addressed.  Will be out shortly.

Mark

Re: [PATCH 2.6.16-mm1 2/3] rtc: m41t00 driver cleanup

[Mark A. Greer]

Resend...
---

This patch does some cleanup to the m41t00 i2c/rtc driver including:
- use BCD2BIN/BIN2BCD instead of BCD_TO_BIN/BIN_TO_BCD
- use strlcpy instead of strncpy
- some whitespace cleanup

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
---

 m41t00.c |   49 +++++++++++++++++++++----------------------------
 1 files changed, 21 insertions(+), 28 deletions(-)
---

diff -Nurp linux-2.6.16-mm1-wq/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c
--- linux-2.6.16-mm1-wq/drivers/i2c/chips/m41t00.c	2006-03-27 15:29:35.000000000 -0700
+++ linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c	2006-03-27 16:00:35.000000000 -0700
@@ -1,6 +1,4 @@
 /*
- * drivers/i2c/chips/m41t00.c
- *
  * I2C client/driver for the ST M41T00 Real-Time Clock chip.
  *
  * Author: Mark A. Greer <mgreer@mvista.com>
@@ -13,9 +11,6 @@
 /*
  * This i2c client/driver wedges between the drivers/char/genrtc.c RTC
  * interface and the SMBus interface of the i2c subsystem.
- * It would be more efficient to use i2c msgs/i2c_transfer directly but, as
- * recommened in .../Documentation/i2c/writing-clients section
- * "Sending and receiving", using SMBus level communication is preferred.
  */
 
 #include <linux/kernel.h>
@@ -41,17 +36,17 @@ static unsigned short ignore[] = { I2C_C
 static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
 
 static struct i2c_client_address_data addr_data = {
-	.normal_i2c		= normal_addr,
-	.probe			= ignore,
-	.ignore			= ignore,
+	.normal_i2c	= normal_addr,
+	.probe		= ignore,
+	.ignore		= ignore,
 };
 
 ulong
 m41t00_get_rtc_time(void)
 {
-	s32	sec, min, hour, day, mon, year;
-	s32	sec1, min1, hour1, day1, mon1, year1;
-	ulong	limit = 10;
+	s32 sec, min, hour, day, mon, year;
+	s32 sec1, min1, hour1, day1, mon1, year1;
+	ulong limit = 10;
 
 	sec = min = hour = day = mon = year = 0;
 	sec1 = min1 = hour1 = day1 = mon1 = year1 = 0;
@@ -97,12 +92,12 @@ m41t00_get_rtc_time(void)
 	mon &= 0x1f;
 	year &= 0xff;
 
-	BCD_TO_BIN(sec);
-	BCD_TO_BIN(min);
-	BCD_TO_BIN(hour);
-	BCD_TO_BIN(day);
-	BCD_TO_BIN(mon);
-	BCD_TO_BIN(year);
+	sec = BCD2BIN(sec);
+	min = BCD2BIN(min);
+	hour = BCD2BIN(hour);
+	day = BCD2BIN(day);
+	mon = BCD2BIN(mon);
+	year = BCD2BIN(year);
 
 	year += 1900;
 	if (year < 1970)
@@ -115,17 +110,17 @@ static void
 m41t00_set(void *arg)
 {
 	struct rtc_time	tm;
-	ulong	nowtime = *(ulong *)arg;
+	ulong nowtime = *(ulong *)arg;
 
 	to_tm(nowtime, &tm);
 	tm.tm_year = (tm.tm_year - 1900) % 100;
 
-	BIN_TO_BCD(tm.tm_sec);
-	BIN_TO_BCD(tm.tm_min);
-	BIN_TO_BCD(tm.tm_hour);
-	BIN_TO_BCD(tm.tm_mon);
-	BIN_TO_BCD(tm.tm_mday);
-	BIN_TO_BCD(tm.tm_year);
+	tm.tm_sec = BIN2BCD(tm.tm_sec);
+	tm.tm_min = BIN2BCD(tm.tm_min);
+	tm.tm_hour = BIN2BCD(tm.tm_hour);
+	tm.tm_mon = BIN2BCD(tm.tm_mon);
+	tm.tm_mday = BIN2BCD(tm.tm_mday);
+	tm.tm_year = BIN2BCD(tm.tm_year);
 
 	mutex_lock(&m41t00_mutex);
 	if ((i2c_smbus_write_byte_data(save_client, 0, tm.tm_sec & 0x7f) < 0)
@@ -143,7 +138,6 @@ m41t00_set(void *arg)
 		dev_warn(&save_client->dev,"m41t00: can't write to rtc chip\n");
 
 	mutex_unlock(&m41t00_mutex);
-	return;
 }
 
 static ulong new_time;
@@ -180,7 +174,7 @@ m41t00_probe(struct i2c_adapter *adap, i
 	if (!client)
 		return -ENOMEM;
 
-	strncpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
+	strlcpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
 	client->addr = addr;
 	client->adapter = adap;
 	client->driver = &m41t00_driver;
@@ -204,7 +198,7 @@ m41t00_attach(struct i2c_adapter *adap)
 static int
 m41t00_detach(struct i2c_client *client)
 {
-	int	rc;
+	int rc;
 
 	if ((rc = i2c_detach_client(client)) == 0) {
 		kfree(client);
@@ -232,7 +226,6 @@ static void __exit
 m41t00_exit(void)
 {
 	i2c_del_driver(&m41t00_driver);
-	return;
 }
 
 module_init(m41t00_init);

[PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Mark A. Greer]

This patch adds support for the ST m41t81 and m41t85 i2c rtc chips to
the existing m41t00 driver.

Since there is no way to reliably determine what type of rtc chip is in use,
the chip type is passed in via platform_data.  The i2c address and square
wave frequency are passed in via platform_data as well.  To accommodate
the use of platform_data, a new header file include/linux/m41t00.h has been
added.

The m41t81 and m41t85 chips halt the updating of their time registers
while they are being accessed.  They resume when a stop condition exists on
the i2c bus or when non-time related regs are accessed.  To make the best use
of that facility and to make more efficient use of the i2c bus, this patch
replaces multiple i2c_smbus_xxx calls with a single i2c_transfer call.

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
---

 drivers/i2c/chips/m41t00.c |  262 +++++++++++++++++++++++++++++++++------------
 include/linux/m41t00.h     |   50 ++++++++
 2 files changed, 246 insertions(+), 66 deletions(-)
---

diff -Nurp linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c
--- linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c	2006-03-23 16:16:35.000000000 -0700
+++ linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c	2006-03-23 18:01:26.000000000 -0700
@@ -1,5 +1,5 @@
 /*
- * I2C client/driver for the ST M41T00 Real-Time Clock chip.
+ * I2C client/driver for the ST M41T00 family of i2c rtc chips.
  *
  * Author: Mark A. Greer <mgreer@mvista.com>
  *
@@ -19,22 +19,20 @@
 #include <linux/i2c.h>
 #include <linux/rtc.h>
 #include <linux/bcd.h>
-#include <linux/mutex.h>
 #include <linux/workqueue.h>
+#include <linux/platform_device.h>
+#include <linux/m41t00.h>
 
 #include <asm/time.h>
 #include <asm/rtc.h>
 
-#define	M41T00_DRV_NAME		"m41t00"
-
-static DEFINE_MUTEX(m41t00_mutex);
 static struct work_struct set_rtc_time_task;
 
 static struct i2c_driver m41t00_driver;
 static struct i2c_client *save_client;
 
 static unsigned short ignore[] = { I2C_CLIENT_END };
-static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
+static unsigned short normal_addr[] = { 0, I2C_CLIENT_END };
 
 static struct i2c_client_address_data addr_data = {
 	.normal_i2c	= normal_addr,
@@ -42,34 +40,45 @@ static struct i2c_client_address_data ad
 	.ignore		= ignore,
 };
 
+struct m41t00_chip_info {
+	u16	type;
+	u16	read_limit;
+	u8	sec;		/* Offsets for chip regs */
+	u8	min;
+	u8	hour;
+	u8	day;
+	u8	mon;
+	u8	year;
+	u8	alarm_mon;
+	u8	alarm_hour;
+	u8	sqw;
+	u32	sqw_freq;
+};
+
+static struct m41t00_chip_info m41t00_chip_info_tbl[] = {
+	{ M41T00_TYPE_M41T00, 5, 0, 1, 2, 4, 5, 6, 0, 0, 0, 0 },
+	{ M41T00_TYPE_M41T81, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
+	{ M41T00_TYPE_M41T85, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
+};
+static struct m41t00_chip_info *m41t00_chip;
+
 ulong
 m41t00_get_rtc_time(void)
 {
 	s32 sec, min, hour, day, mon, year;
 	s32 sec1, min1, hour1, day1, mon1, year1;
-	ulong limit = 10;
+	u16 reads = 0;
+	u8 buf[8], msgbuf[1] = { 0 }; /* offset into rtc's regs */
+	struct i2c_msg msgs[] = {
+		{ save_client->addr, 0, 1, msgbuf },
+		{ save_client->addr, I2C_M_RD, 8, buf }
+	};
 
 	sec = min = hour = day = mon = year = 0;
-	sec1 = min1 = hour1 = day1 = mon1 = year1 = 0;
 
-	mutex_lock(&m41t00_mutex);
 	do {
-		if (((sec = i2c_smbus_read_byte_data(save_client, 0)) >= 0)
-			&& ((min = i2c_smbus_read_byte_data(save_client, 1))
-				>= 0)
-			&& ((hour = i2c_smbus_read_byte_data(save_client, 2))
-				>= 0)
-			&& ((day = i2c_smbus_read_byte_data(save_client, 4))
-				>= 0)
-			&& ((mon = i2c_smbus_read_byte_data(save_client, 5))
-				>= 0)
-			&& ((year = i2c_smbus_read_byte_data(save_client, 6))
-				>= 0)
-			&& ((sec == sec1) && (min == min1) && (hour == hour1)
-				&& (day == day1) && (mon == mon1)
-				&& (year == year1)))
-
-				break;
+		if (i2c_transfer(save_client->adapter, msgs, 2) < 0)
+			goto read_err;
 
 		sec1 = sec;
 		min1 = min;
@@ -77,21 +86,21 @@ m41t00_get_rtc_time(void)
 		day1 = day;
 		mon1 = mon;
 		year1 = year;
-	} while (--limit > 0);
-	mutex_unlock(&m41t00_mutex);
 
-	if (limit == 0) {
-		dev_warn(&save_client->dev,
-			"m41t00: can't read rtc chip\n");
-		sec = min = hour = day = mon = year = 0;
-	}
-
-	sec &= 0x7f;
-	min &= 0x7f;
-	hour &= 0x3f;
-	day &= 0x3f;
-	mon &= 0x1f;
-	year &= 0xff;
+		sec = buf[m41t00_chip->sec] & 0x7f;
+		min = buf[m41t00_chip->min] & 0x7f;
+		hour = buf[m41t00_chip->hour] & 0x3f;
+		day = buf[m41t00_chip->day] & 0x3f;
+		mon = buf[m41t00_chip->mon] & 0x1f;
+		year = buf[m41t00_chip->year] & 0xff;
+	} while ((++reads < m41t00_chip->read_limit) && ((sec != sec1)
+			|| (min != min1) || (hour != hour1) || (day != day1)
+			|| (mon != mon1) || (year != year1)));
+
+	if ((m41t00_chip->read_limit > 1) && ((sec != sec1) || (min != min1)
+			|| (hour != hour1) || (day != day1) || (mon != mon1)
+			|| (year != year1)))
+		goto read_err;
 
 	sec = BCD2BIN(sec);
 	min = BCD2BIN(min);
@@ -105,40 +114,49 @@ m41t00_get_rtc_time(void)
 		year += 100;
 
 	return mktime(year, mon, day, hour, min, sec);
+
+read_err:
+	dev_err(&save_client->dev, "m41t00_get_rtc_time: Read error\n");
+	return 0;
 }
 
 static void
 m41t00_set(void *arg)
 {
 	struct rtc_time	tm;
-	ulong nowtime = *(ulong *)arg;
+	int nowtime = *(int *)arg;
+	s32 sec, min, hour, day, mon, year;
+	u8 wbuf[9], *buf = &wbuf[1], msgbuf[1] = { 0 };
+	struct i2c_msg msgs[] = {
+		{ save_client->addr, 0, 1, msgbuf },
+		{ save_client->addr, I2C_M_RD, 8, buf }
+	};
 
 	to_tm(nowtime, &tm);
 	tm.tm_year = (tm.tm_year - 1900) % 100;
 
-	tm.tm_sec = BIN2BCD(tm.tm_sec);
-	tm.tm_min = BIN2BCD(tm.tm_min);
-	tm.tm_hour = BIN2BCD(tm.tm_hour);
-	tm.tm_mon = BIN2BCD(tm.tm_mon);
-	tm.tm_mday = BIN2BCD(tm.tm_mday);
-	tm.tm_year = BIN2BCD(tm.tm_year);
-
-	mutex_lock(&m41t00_mutex);
-	if ((i2c_smbus_write_byte_data(save_client, 0, tm.tm_sec & 0x7f) < 0)
-		|| (i2c_smbus_write_byte_data(save_client, 1, tm.tm_min & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 2, tm.tm_hour & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 4, tm.tm_mday & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 5, tm.tm_mon & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 6, tm.tm_year & 0x7f)
-			< 0))
+	sec = BIN2BCD(tm.tm_sec);
+	min = BIN2BCD(tm.tm_min);
+	hour = BIN2BCD(tm.tm_hour);
+	day = BIN2BCD(tm.tm_mday);
+	mon = BIN2BCD(tm.tm_mon);
+	year = BIN2BCD(tm.tm_year);
+
+	/* Read reg values into buf[0..7]/wbuf[1..8] */
+	if (i2c_transfer(save_client->adapter, msgs, 2) < 0) {
+		dev_err(&save_client->dev, "m41t00_set: Read error\n");
+		return;
+	}
 
-		dev_warn(&save_client->dev,"m41t00: can't write to rtc chip\n");
+	wbuf[0] = 0; /* offset into rtc's regs */
+	buf[m41t00_chip->sec] = (buf[m41t00_chip->sec] & ~0x7f) | (sec & 0x7f);
+	buf[m41t00_chip->min] = (buf[m41t00_chip->min] & ~0x7f) | (min & 0x7f);
+	buf[m41t00_chip->hour] = (buf[m41t00_chip->hour]& ~0x3f) | (hour& 0x3f);
+	buf[m41t00_chip->day] = (buf[m41t00_chip->day] & ~0x3f) | (day & 0x3f);
+	buf[m41t00_chip->mon] = (buf[m41t00_chip->mon] & ~0x1f) | (mon & 0x1f);
 
-	mutex_unlock(&m41t00_mutex);
+	if (i2c_master_send(save_client, wbuf, 9) < 0)
+		dev_err(&save_client->dev, "m41t00_set: Write error\n");
 }
 
 static ulong new_time;
@@ -159,6 +177,47 @@ m41t00_set_rtc_time(ulong nowtime)
 /*
  *****************************************************************************
  *
+ *	platform_data Driver Interface
+ *
+ *****************************************************************************
+ */
+static int __init
+m41t00_platform_probe(struct platform_device *pdev)
+{
+	struct m41t00_platform_data *pdata;
+	int i;
+
+	if (pdev && (pdata = pdev->dev.platform_data)) {
+		normal_addr[0] = pdata->i2c_addr;
+
+		for (i=0; i<ARRAY_SIZE(m41t00_chip_info_tbl); i++)
+			if (m41t00_chip_info_tbl[i].type == pdata->type) {
+				m41t00_chip = &m41t00_chip_info_tbl[i];
+				m41t00_chip->sqw_freq = pdata->sqw_freq;
+				return 0;
+			}
+	}
+	return -ENODEV;
+}
+
+static int __exit
+m41t00_platform_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver m41t00_platform_driver = {
+	.probe  = m41t00_platform_probe,
+	.remove = m41t00_platform_remove,
+	.driver = {
+		.owner = THIS_MODULE,
+		.name  = M41T00_DRV_NAME,
+	},
+};
+
+/*
+ *****************************************************************************
+ *
  *	Driver Interface
  *
  *****************************************************************************
@@ -168,24 +227,90 @@ m41t00_probe(struct i2c_adapter *adap, i
 {
 	struct i2c_client *client;
 	int rc;
+	u8 rbuf[1], wbuf[2], msgbuf[1] = { 0 }; /* offset into rtc's regs */
+	struct i2c_msg msgs[] = {
+		{ 0, 0, 1, msgbuf },
+		{ 0, I2C_M_RD, 1, rbuf }
+	};
 
 	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
 	if (!client)
 		return -ENOMEM;
 
-	strlcpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
+	strlcpy(client->name, m41t00_driver.driver.name, I2C_NAME_SIZE);
 	client->addr = addr;
 	client->adapter = adap;
 	client->driver = &m41t00_driver;
 
-	if ((rc = i2c_attach_client(client)) != 0) {
-		kfree(client);
-		return rc;
+	if ((rc = i2c_attach_client(client)) != 0)
+		goto attach_err;
+
+	msgs[0].addr = addr;
+	msgs[1].addr = addr;
+
+	if (m41t00_chip->type != M41T00_TYPE_M41T00) {
+		/* If asked, set SQW frequency & enable */
+		if (m41t00_chip->sqw_freq) {
+			msgbuf[0] = m41t00_chip->alarm_mon;
+			if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
+				goto sqw_err;
+
+			wbuf[0] = m41t00_chip->alarm_mon;
+			wbuf[1] = rbuf[0] & ~0x40;
+			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+				goto sqw_err;
+
+			wbuf[0] = m41t00_chip->sqw;
+			wbuf[1] = m41t00_chip->sqw_freq;
+			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+				goto sqw_err;
+
+			wbuf[0] = m41t00_chip->alarm_mon;
+			wbuf[1] = rbuf[0] | 0x40;
+			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+				goto sqw_err;
+		}
+
+		/* Make sure HT (Halt Update) bit is cleared */
+		msgbuf[0] = m41t00_chip->alarm_hour;
+		if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
+			goto ht_err;
+
+		if (rbuf[0] & 0x40) {
+			wbuf[0] = m41t00_chip->alarm_hour;
+			wbuf[1] = rbuf[0] & ~0x40;
+			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+				goto ht_err;
+		}
+	}
+
+	/* Make sure ST (stop) bit is cleared */
+	msgbuf[0] = m41t00_chip->sec;
+	if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
+		goto st_err;
+
+	if (rbuf[0] & 0x80) {
+		wbuf[0] = m41t00_chip->sec;
+		wbuf[1] = rbuf[0] & ~0x80;
+		if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+			goto st_err;
 	}
 
 	INIT_WORK(&set_rtc_time_task, &m41t00_set, &new_time);
 	save_client = client;
 	return 0;
+
+st_err:
+	dev_err(&client->dev, "m41t00_probe: Can't clear ST bit\n");
+	goto attach_err;
+ht_err:
+	dev_err(&client->dev, "m41t00_probe: Can't clear HT bit\n");
+	goto attach_err;
+sqw_err:
+	dev_err(&client->dev, "m41t00_probe: Can't set SQW Frequency\n");
+attach_err:
+	kfree(client);
+	return rc;
 }
 
 static int
@@ -219,13 +344,18 @@ static struct i2c_driver m41t00_driver =
 static int __init
 m41t00_init(void)
 {
-	return i2c_add_driver(&m41t00_driver);
+	int rc;
+
+	if (!(rc = platform_driver_register(&m41t00_platform_driver)))
+		rc = i2c_add_driver(&m41t00_driver);
+	return rc;
 }
 
 static void __exit
 m41t00_exit(void)
 {
 	i2c_del_driver(&m41t00_driver);
+	platform_driver_unregister(&m41t00_platform_driver);
 }
 
 module_init(m41t00_init);
diff -Nurp linux-2.6.16-mm1-cleanup/include/linux/m41t00.h linux-2.6.16-mm1-newsupp/include/linux/m41t00.h
--- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
+++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-23 17:54:54.000000000 -0700
@@ -0,0 +1,50 @@
+/*
+ * Definitions for the ST M41T00 family of i2c rtc chips.
+ *
+ * Author: Mark A. Greer <mgreer@mvista.com>
+ *
+ * 2005 (c) MontaVista Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+
+#ifndef _M41T00_H
+#define _M41T00_H
+
+#define	M41T00_DRV_NAME		"m41t00"
+#define	M41T00_I2C_ADDR		0x68
+
+#define	M41T00_TYPE_M41T00	0
+#define	M41T00_TYPE_M41T81	81
+#define	M41T00_TYPE_M41T85	85
+
+struct m41t00_platform_data {
+	u16	type;
+	u16	i2c_addr;
+	u32	sqw_freq;
+};
+
+/* SQW output disabled, this is default value by power on*/
+#define SQW_FREQ_DISABLE	(0)
+
+#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
+#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
+#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
+#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
+#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
+#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
+#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
+#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
+#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
+#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
+#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
+#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
+#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
+#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
+#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */
+
+extern ulong m41t00_get_rtc_time(void);
+extern int m41t00_set_rtc_time(ulong nowtime);
+
+#endif /* _M41T00_H */

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Andrew Morton]

"Mark A. Greer" <mgreer@mvista.com> wrote:
>
>  +	struct i2c_msg msgs[] = {
>  +		{ save_client->addr, 0, 1, msgbuf },
>  +		{ save_client->addr, I2C_M_RD, 8, buf }

The

	.name = value,

form would be preferred.  It reduces the possibility of silent breakage if
someone changes struct i2c_msg.

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Mark A. Greer]

Hi Andrew,

On Sun, Mar 26, 2006 at 02:58:40PM -0800, Andrew Morton wrote:
> "Mark A. Greer" <mgreer@mvista.com> wrote:
> >
> >  +	struct i2c_msg msgs[] = {
> >  +		{ save_client->addr, 0, 1, msgbuf },
> >  +		{ save_client->addr, I2C_M_RD, 8, buf }
> 
> The
> 
> 	.name = value,
> 
> form would be preferred.  It reduces the possibility of silent breakage if
> someone changes struct i2c_msg.

Noted.  Will fix in upcoming set of new patches.

Mark

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Mark A. Greer]

Resend...
---

 drivers/i2c/chips/m41t00.c |  294 ++++++++++++++++++++++++++++++++++-----------
 include/linux/m41t00.h     |   50 +++++++
 2 files changed, 276 insertions(+), 68 deletions(-)
---

diff -Nurp linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c
--- linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c	2006-03-27 16:00:35.000000000 -0700
+++ linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c	2006-03-27 16:49:35.000000000 -0700
@@ -1,5 +1,5 @@
 /*
- * I2C client/driver for the ST M41T00 Real-Time Clock chip.
+ * I2C client/driver for the ST M41T00 family of i2c rtc chips.
  *
  * Author: Mark A. Greer <mgreer@mvista.com>
  *
@@ -19,21 +19,17 @@
 #include <linux/i2c.h>
 #include <linux/rtc.h>
 #include <linux/bcd.h>
-#include <linux/mutex.h>
 #include <linux/workqueue.h>
-
+#include <linux/platform_device.h>
+#include <linux/m41t00.h>
 #include <asm/time.h>
 #include <asm/rtc.h>
 
-#define	M41T00_DRV_NAME		"m41t00"
-
-static DEFINE_MUTEX(m41t00_mutex);
-
 static struct i2c_driver m41t00_driver;
 static struct i2c_client *save_client;
 
 static unsigned short ignore[] = { I2C_CLIENT_END };
-static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
+static unsigned short normal_addr[] = { 0, I2C_CLIENT_END };
 
 static struct i2c_client_address_data addr_data = {
 	.normal_i2c	= normal_addr,
@@ -41,34 +37,55 @@ static struct i2c_client_address_data ad
 	.ignore		= ignore,
 };
 
+struct m41t00_chip_info {
+	u16	type;
+	u16	read_limit;
+	u8	sec;		/* Offsets for chip regs */
+	u8	min;
+	u8	hour;
+	u8	day;
+	u8	mon;
+	u8	year;
+	u8	alarm_mon;
+	u8	alarm_hour;
+	u8	sqw;
+	u32	sqw_freq;
+};
+
+static struct m41t00_chip_info m41t00_chip_info_tbl[] = {
+	{ M41T00_TYPE_M41T00, 5, 0, 1, 2, 4, 5, 6, 0, 0, 0, 0 },
+	{ M41T00_TYPE_M41T81, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
+	{ M41T00_TYPE_M41T85, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
+};
+static struct m41t00_chip_info *m41t00_chip;
+
 ulong
 m41t00_get_rtc_time(void)
 {
 	s32 sec, min, hour, day, mon, year;
 	s32 sec1, min1, hour1, day1, mon1, year1;
-	ulong limit = 10;
+	u16 reads = 0;
+	u8 buf[8], msgbuf[1] = { 0 }; /* offset into rtc's regs */
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= save_client->addr,
+			.flags	= 0,
+			.len	= 1,
+			.buf	= msgbuf,
+		},
+		{
+			.addr	= save_client->addr,
+			.flags	= I2C_M_RD,
+			.len	= 8,
+			.buf	= buf,
+		},
+	};
 
 	sec = min = hour = day = mon = year = 0;
-	sec1 = min1 = hour1 = day1 = mon1 = year1 = 0;
 
-	mutex_lock(&m41t00_mutex);
 	do {
-		if (((sec = i2c_smbus_read_byte_data(save_client, 0)) >= 0)
-			&& ((min = i2c_smbus_read_byte_data(save_client, 1))
-				>= 0)
-			&& ((hour = i2c_smbus_read_byte_data(save_client, 2))
-				>= 0)
-			&& ((day = i2c_smbus_read_byte_data(save_client, 4))
-				>= 0)
-			&& ((mon = i2c_smbus_read_byte_data(save_client, 5))
-				>= 0)
-			&& ((year = i2c_smbus_read_byte_data(save_client, 6))
-				>= 0)
-			&& ((sec == sec1) && (min == min1) && (hour == hour1)
-				&& (day == day1) && (mon == mon1)
-				&& (year == year1)))
-
-				break;
+		if (i2c_transfer(save_client->adapter, msgs, 2) < 0)
+			goto read_err;
 
 		sec1 = sec;
 		min1 = min;
@@ -76,21 +93,21 @@ m41t00_get_rtc_time(void)
 		day1 = day;
 		mon1 = mon;
 		year1 = year;
-	} while (--limit > 0);
-	mutex_unlock(&m41t00_mutex);
-
-	if (limit == 0) {
-		dev_warn(&save_client->dev,
-			"m41t00: can't read rtc chip\n");
-		sec = min = hour = day = mon = year = 0;
-	}
 
-	sec &= 0x7f;
-	min &= 0x7f;
-	hour &= 0x3f;
-	day &= 0x3f;
-	mon &= 0x1f;
-	year &= 0xff;
+		sec = buf[m41t00_chip->sec] & 0x7f;
+		min = buf[m41t00_chip->min] & 0x7f;
+		hour = buf[m41t00_chip->hour] & 0x3f;
+		day = buf[m41t00_chip->day] & 0x3f;
+		mon = buf[m41t00_chip->mon] & 0x1f;
+		year = buf[m41t00_chip->year] & 0xff;
+	} while ((++reads < m41t00_chip->read_limit) && ((sec != sec1)
+			|| (min != min1) || (hour != hour1) || (day != day1)
+			|| (mon != mon1) || (year != year1)));
+
+	if ((m41t00_chip->read_limit > 1) && ((sec != sec1) || (min != min1)
+			|| (hour != hour1) || (day != day1) || (mon != mon1)
+			|| (year != year1)))
+		goto read_err;
 
 	sec = BCD2BIN(sec);
 	min = BCD2BIN(min);
@@ -104,40 +121,59 @@ m41t00_get_rtc_time(void)
 		year += 100;
 
 	return mktime(year, mon, day, hour, min, sec);
+
+read_err:
+	dev_err(&save_client->dev, "m41t00_get_rtc_time: Read error\n");
+	return 0;
 }
 
 static void
 m41t00_set(void *arg)
 {
 	struct rtc_time	tm;
-	ulong nowtime = *(ulong *)arg;
+	int nowtime = *(int *)arg;
+	s32 sec, min, hour, day, mon, year;
+	u8 wbuf[9], *buf = &wbuf[1], msgbuf[1] = { 0 };
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= save_client->addr,
+			.flags	= 0,
+			.len	= 1,
+			.buf	= msgbuf,
+		},
+		{
+			.addr	= save_client->addr,
+			.flags	= I2C_M_RD,
+			.len	= 8,
+			.buf	= buf,
+		},
+	};
 
 	to_tm(nowtime, &tm);
 	tm.tm_year = (tm.tm_year - 1900) % 100;
 
-	tm.tm_sec = BIN2BCD(tm.tm_sec);
-	tm.tm_min = BIN2BCD(tm.tm_min);
-	tm.tm_hour = BIN2BCD(tm.tm_hour);
-	tm.tm_mon = BIN2BCD(tm.tm_mon);
-	tm.tm_mday = BIN2BCD(tm.tm_mday);
-	tm.tm_year = BIN2BCD(tm.tm_year);
-
-	mutex_lock(&m41t00_mutex);
-	if ((i2c_smbus_write_byte_data(save_client, 0, tm.tm_sec & 0x7f) < 0)
-		|| (i2c_smbus_write_byte_data(save_client, 1, tm.tm_min & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 2, tm.tm_hour & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 4, tm.tm_mday & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 5, tm.tm_mon & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 6, tm.tm_year & 0x7f)
-			< 0))
+	sec = BIN2BCD(tm.tm_sec);
+	min = BIN2BCD(tm.tm_min);
+	hour = BIN2BCD(tm.tm_hour);
+	day = BIN2BCD(tm.tm_mday);
+	mon = BIN2BCD(tm.tm_mon);
+	year = BIN2BCD(tm.tm_year);
+
+	/* Read reg values into buf[0..7]/wbuf[1..8] */
+	if (i2c_transfer(save_client->adapter, msgs, 2) < 0) {
+		dev_err(&save_client->dev, "m41t00_set: Read error\n");
+		return;
+	}
 
-		dev_warn(&save_client->dev,"m41t00: can't write to rtc chip\n");
+	wbuf[0] = 0; /* offset into rtc's regs */
+	buf[m41t00_chip->sec] = (buf[m41t00_chip->sec] & ~0x7f) | (sec & 0x7f);
+	buf[m41t00_chip->min] = (buf[m41t00_chip->min] & ~0x7f) | (min & 0x7f);
+	buf[m41t00_chip->hour] = (buf[m41t00_chip->hour]& ~0x3f) | (hour& 0x3f);
+	buf[m41t00_chip->day] = (buf[m41t00_chip->day] & ~0x3f) | (day & 0x3f);
+	buf[m41t00_chip->mon] = (buf[m41t00_chip->mon] & ~0x1f) | (mon & 0x1f);
 
-	mutex_unlock(&m41t00_mutex);
+	if (i2c_master_send(save_client, wbuf, 9) < 0)
+		dev_err(&save_client->dev, "m41t00_set: Write error\n");
 }
 
 static ulong new_time;
@@ -160,6 +196,47 @@ m41t00_set_rtc_time(ulong nowtime)
 /*
  *****************************************************************************
  *
+ *	platform_data Driver Interface
+ *
+ *****************************************************************************
+ */
+static int __init
+m41t00_platform_probe(struct platform_device *pdev)
+{
+	struct m41t00_platform_data *pdata;
+	int i;
+
+	if (pdev && (pdata = pdev->dev.platform_data)) {
+		normal_addr[0] = pdata->i2c_addr;
+
+		for (i=0; i<ARRAY_SIZE(m41t00_chip_info_tbl); i++)
+			if (m41t00_chip_info_tbl[i].type == pdata->type) {
+				m41t00_chip = &m41t00_chip_info_tbl[i];
+				m41t00_chip->sqw_freq = pdata->sqw_freq;
+				return 0;
+			}
+	}
+	return -ENODEV;
+}
+
+static int __exit
+m41t00_platform_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver m41t00_platform_driver = {
+	.probe  = m41t00_platform_probe,
+	.remove = m41t00_platform_remove,
+	.driver = {
+		.owner = THIS_MODULE,
+		.name  = M41T00_DRV_NAME,
+	},
+};
+
+/*
+ *****************************************************************************
+ *
  *	Driver Interface
  *
  *****************************************************************************
@@ -169,24 +246,100 @@ m41t00_probe(struct i2c_adapter *adap, i
 {
 	struct i2c_client *client;
 	int rc;
+	u8 rbuf[1], wbuf[2], msgbuf[1] = { 0 }; /* offset into rtc's regs */
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= 0,
+			.flags	= 0,
+			.len	= 1,
+			.buf	= msgbuf,
+		},
+		{
+			.addr	= 0,
+			.flags	= I2C_M_RD,
+			.len	= 1,
+			.buf	= rbuf,
+		},
+	};
 
 	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
 	if (!client)
 		return -ENOMEM;
 
-	strlcpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
+	strlcpy(client->name, m41t00_driver.driver.name, I2C_NAME_SIZE);
 	client->addr = addr;
 	client->adapter = adap;
 	client->driver = &m41t00_driver;
 
-	if ((rc = i2c_attach_client(client)) != 0) {
-		kfree(client);
-		return rc;
+	if ((rc = i2c_attach_client(client)) != 0)
+		goto attach_err;
+
+	msgs[0].addr = addr;
+	msgs[1].addr = addr;
+
+	if (m41t00_chip->type != M41T00_TYPE_M41T00) {
+		/* If asked, set SQW frequency & enable */
+		if (m41t00_chip->sqw_freq) {
+			msgbuf[0] = m41t00_chip->alarm_mon;
+			if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
+				goto sqw_err;
+
+			wbuf[0] = m41t00_chip->alarm_mon;
+			wbuf[1] = rbuf[0] & ~0x40;
+			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+				goto sqw_err;
+
+			wbuf[0] = m41t00_chip->sqw;
+			wbuf[1] = m41t00_chip->sqw_freq;
+			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+				goto sqw_err;
+
+			wbuf[0] = m41t00_chip->alarm_mon;
+			wbuf[1] = rbuf[0] | 0x40;
+			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+				goto sqw_err;
+		}
+
+		/* Make sure HT (Halt Update) bit is cleared */
+		msgbuf[0] = m41t00_chip->alarm_hour;
+		if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
+			goto ht_err;
+
+		if (rbuf[0] & 0x40) {
+			wbuf[0] = m41t00_chip->alarm_hour;
+			wbuf[1] = rbuf[0] & ~0x40;
+			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+				goto ht_err;
+		}
+	}
+
+	/* Make sure ST (stop) bit is cleared */
+	msgbuf[0] = m41t00_chip->sec;
+	if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
+		goto st_err;
+
+	if (rbuf[0] & 0x80) {
+		wbuf[0] = m41t00_chip->sec;
+		wbuf[1] = rbuf[0] & ~0x80;
+		if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
+			goto st_err;
 	}
 
 	m41t00_wq = create_singlethread_workqueue("m41t00");
 	save_client = client;
 	return 0;
+
+st_err:
+	dev_err(&client->dev, "m41t00_probe: Can't clear ST bit\n");
+	goto attach_err;
+ht_err:
+	dev_err(&client->dev, "m41t00_probe: Can't clear HT bit\n");
+	goto attach_err;
+sqw_err:
+	dev_err(&client->dev, "m41t00_probe: Can't set SQW Frequency\n");
+attach_err:
+	kfree(client);
+	return rc;
 }
 
 static int
@@ -219,13 +372,18 @@ static struct i2c_driver m41t00_driver =
 static int __init
 m41t00_init(void)
 {
-	return i2c_add_driver(&m41t00_driver);
+	int rc;
+
+	if (!(rc = platform_driver_register(&m41t00_platform_driver)))
+		rc = i2c_add_driver(&m41t00_driver);
+	return rc;
 }
 
 static void __exit
 m41t00_exit(void)
 {
 	i2c_del_driver(&m41t00_driver);
+	platform_driver_unregister(&m41t00_platform_driver);
 }
 
 module_init(m41t00_init);
diff -Nurp linux-2.6.16-mm1-cleanup/include/linux/m41t00.h linux-2.6.16-mm1-newsupp/include/linux/m41t00.h
--- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
+++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-27 16:25:51.000000000 -0700
@@ -0,0 +1,50 @@
+/*
+ * Definitions for the ST M41T00 family of i2c rtc chips.
+ *
+ * Author: Mark A. Greer <mgreer@mvista.com>
+ *
+ * 2005 (c) MontaVista Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+
+#ifndef _M41T00_H
+#define _M41T00_H
+
+#define	M41T00_DRV_NAME		"m41t00"
+#define	M41T00_I2C_ADDR		0x68
+
+#define	M41T00_TYPE_M41T00	0
+#define	M41T00_TYPE_M41T81	81
+#define	M41T00_TYPE_M41T85	85
+
+struct m41t00_platform_data {
+	u16	type;
+	u16	i2c_addr;
+	u32	sqw_freq;
+};
+
+/* SQW output disabled, this is default value by power on*/
+#define SQW_FREQ_DISABLE	(0)
+
+#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
+#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
+#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
+#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
+#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
+#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
+#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
+#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
+#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
+#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
+#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
+#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
+#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
+#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
+#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */
+
+extern ulong m41t00_get_rtc_time(void);
+extern int m41t00_set_rtc_time(ulong nowtime);
+
+#endif /* _M41T00_H */

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Andrew Morton]

"Mark A. Greer" <mgreer@mvista.com> wrote:
>
> Resend...
> ---
> 
>  drivers/i2c/chips/m41t00.c |  294 ++++++++++++++++++++++++++++++++++-----------
>  include/linux/m41t00.h     |   50 +++++++

Not sure what to make of this.  It has no changelog, it doesn't apply on
top of your previous three patches:

rtc-m41t00-driver-should-use-workqueue-instead-of-tasklet.patch
rtc-m41t00-driver-cleanup.patch
rtc-add-support-for-m41t81-m41t85-chips-to-m41t00-driver.patch

and it doesn't apply when used to replace
rtc-add-support-for-m41t81-m41t85-chips-to-m41t00-driver.patch.

An incremental patch against the three above patches would be ideal...

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Jean Delvare]

Hi Mark, Andrew,

> Mark A. Greer wrote:
> >
> > Resend...
> > ---
> > 
> >  drivers/i2c/chips/m41t00.c |  294 ++++++++++++++++++++++++++++++++++-----------
> >  include/linux/m41t00.h     |   50 +++++++
> 

Andrew Morton wrote:
> Not sure what to make of this.  It has no changelog, it doesn't apply on
> top of your previous three patches:
> 
> rtc-m41t00-driver-should-use-workqueue-instead-of-tasklet.patch
> rtc-m41t00-driver-cleanup.patch
> rtc-add-support-for-m41t81-m41t85-chips-to-m41t00-driver.patch
> 
> and it doesn't apply when used to replace
> rtc-add-support-for-m41t81-m41t85-chips-to-m41t00-driver.patch.

Replacing works for me, after also replacing the two first patches of
the series with their new respective version. As for the changelog I
picked the one from the original third patch.

> An incremental patch against the three above patches would be ideal...

Well, the first two patches (workqueue and cleanup) look ok to me now,
so I'll send them to Greg now, together with one similar patch for the
ds1374 driver. The workqueue patches need to go to Linus soon, as they
fix a bug in these drivers, and I know that Greg has other i2c patches
waiting to go to Linus anyway. The cleanup patch can go in too, I
think, as it's simple enough.

As for the third patch, I have some more comments on it (sorry for
being slow) and it might need some tweaking, so it's probably better
(and easier) if Andrew just drops it for now, and it'll get back to him
when ready.

Mark, is it OK if this third patch adding the m41t81 and m41t85 support
spends some time in Andrew's tree and only goes in mainline for 2.6.18,
or do you need it in 2.6.17?

Thanks,
-- 
Jean Delvare

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Mark A. Greer]

On Tue, Mar 28, 2006 at 02:21:14PM +0200, Jean Delvare wrote:
> Hi Mark, Andrew,
> 
> > Mark A. Greer wrote:
> > >
> > > Resend...
> > > ---
> > > 
> > >  drivers/i2c/chips/m41t00.c |  294 ++++++++++++++++++++++++++++++++++-----------
> > >  include/linux/m41t00.h     |   50 +++++++
> > 
> 
> Andrew Morton wrote:
> > Not sure what to make of this.  It has no changelog,

Oops.  My bad.

> > it doesn't apply on
> > top of your previous three patches:
> > 
> > rtc-m41t00-driver-should-use-workqueue-instead-of-tasklet.patch
> > rtc-m41t00-driver-cleanup.patch
> > rtc-add-support-for-m41t81-m41t85-chips-to-m41t00-driver.patch
> > 
> > and it doesn't apply when used to replace
> > rtc-add-support-for-m41t81-m41t85-chips-to-m41t00-driver.patch.
> 
> Replacing works for me, after also replacing the two first patches of
> the series with their new respective version. As for the changelog I
> picked the one from the original third patch.

Sorry for the confusion, Andrew.  The 3 patches I sent were all
replacement patches for the previous 3 patches.

> > An incremental patch against the three above patches would be ideal...

I would have done that but there wasn't a 2.6.16-mm2 patch yesterday
(that I see is there now) so I had nothing to diff against.

<snip>

> Mark, is it OK if this third patch adding the m41t81 and m41t85 support
> spends some time in Andrew's tree and only goes in mainline for 2.6.18,
> or do you need it in 2.6.17?

No, waiting is fine, Jean.

Thanks guys.

Mark

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Jean Delvare]

Hi Mark,

Some more comments, sorry for being slow.

>  drivers/i2c/chips/m41t00.c |  294 ++++++++++++++++++++++++++++++++++-----------
>  include/linux/m41t00.h     |   50 +++++++

Who else is going to use this header file? It's rather rare to have a
hardware-specific header file under include/linux.

> diff -Nurp linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c
> --- linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c	2006-03-27 16:00:35.000000000 -0700
> +++ linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c	2006-03-27 16:49:35.000000000 -0700
> (...)
>  static unsigned short ignore[] = { I2C_CLIENT_END };
> -static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
> +static unsigned short normal_addr[] = { 0, I2C_CLIENT_END };

Bad idea. If no platform data is found, the driver will attempt to
probe I2C address 0, which is a broadcast address. I can predict
interesting results ;)

So, if you don't want the driver to do anything in the absence of
platform data, you should define normal_addr the following way:

static unsigned short normal_addr[] = { I2C_CLIENT_END, I2C_CLIENT_END };

> +struct m41t00_chip_info {
> +	u16	type;
> +	u16	read_limit;
> +	u8	sec;		/* Offsets for chip regs */
> +	u8	min;
> +	u8	hour;
> +	u8	day;
> +	u8	mon;
> +	u8	year;
> +	u8	alarm_mon;
> +	u8	alarm_hour;
> +	u8	sqw;
> +	u32	sqw_freq;
> +};

u16 is probably overkill for .type and .read_limit.

> +static struct m41t00_chip_info m41t00_chip_info_tbl[] = {
> +	{ M41T00_TYPE_M41T00, 5, 0, 1, 2, 4, 5, 6, 0, 0, 0, 0 },
> +	{ M41T00_TYPE_M41T81, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
> +	{ M41T00_TYPE_M41T85, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
> +};

C99-style initialization, please? It'll take much more lines, granted,
but is also way more robust to future changes, and will be more
readable too.

May I ask why you define separate types for the M41T81 and the M41T85,
when you handle both exactly the same way?

> +		sec = buf[m41t00_chip->sec] & 0x7f;
> +		min = buf[m41t00_chip->min] & 0x7f;
> +		hour = buf[m41t00_chip->hour] & 0x3f;
> +		day = buf[m41t00_chip->day] & 0x3f;
> +		mon = buf[m41t00_chip->mon] & 0x1f;
> +		year = buf[m41t00_chip->year] & 0xff;

The year masking is a no-op, you could omit it.

> @@ -169,24 +246,100 @@ m41t00_probe(struct i2c_adapter *adap, i
>  {
>  	struct i2c_client *client;
>  	int rc;
> +	u8 rbuf[1], wbuf[2], msgbuf[1] = { 0 }; /* offset into rtc's regs */
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= 0,
> +			.flags	= 0,
> +			.len	= 1,
> +			.buf	= msgbuf,
> +		},
> +		{
> +			.addr	= 0,
> +			.flags	= I2C_M_RD,
> +			.len	= 1,
> +			.buf	= rbuf,
> +		},
> +	};

Why don't you initialize both .addr right away?

>  
>  	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
>  	if (!client)
>  		return -ENOMEM;
>  
> -	strlcpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
> +	strlcpy(client->name, m41t00_driver.driver.name, I2C_NAME_SIZE);

The driver name may not be suitable here, the client name should
instead reflect what chip it is.

> +	if (m41t00_chip->type != M41T00_TYPE_M41T00) {
> +		/* If asked, set SQW frequency & enable */
> +		if (m41t00_chip->sqw_freq) {
> +			msgbuf[0] = m41t00_chip->alarm_mon;
> +			if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> +				goto sqw_err;

You did not check whether the adapter you are attaching to supports this
kind of transaction! You need to call i2c_check_functionality, see how
other i2c chip drivers (for example ds1337) are doing. For i2c_transfer
and i2c_master_send, you want to check for I2C_FUNC_I2C.

> +
> +			wbuf[0] = m41t00_chip->alarm_mon;
> +			wbuf[1] = rbuf[0] & ~0x40;
> +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +				goto sqw_err;
> +
> +			wbuf[0] = m41t00_chip->sqw;
> +			wbuf[1] = m41t00_chip->sqw_freq;
> +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +				goto sqw_err;
> +
> +			wbuf[0] = m41t00_chip->alarm_mon;
> +			wbuf[1] = rbuf[0] | 0x40;
> +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +				goto sqw_err;
> +		}
> +
> +		/* Make sure HT (Halt Update) bit is cleared */
> +		msgbuf[0] = m41t00_chip->alarm_hour;
> +		if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> +			goto ht_err;
> +
> +		if (rbuf[0] & 0x40) {
> +			wbuf[0] = m41t00_chip->alarm_hour;
> +			wbuf[1] = rbuf[0] & ~0x40;
> +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +				goto ht_err;
> +		}
> +	}
> +
> +	/* Make sure ST (stop) bit is cleared */
> +	msgbuf[0] = m41t00_chip->sec;
> +	if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> +		goto st_err;
> +
> +	if (rbuf[0] & 0x80) {
> +		wbuf[0] = m41t00_chip->sec;
> +		wbuf[1] = rbuf[0] & ~0x80;
> +		if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> +			goto st_err;
>  	}

What you do here are basically SMBus Read Byte and SMBus Write Byte
transactions. The code would be much more simple if you were using the
i2c_smbus_read_byte_data and i2c_smbus_write_byte_data functions, which
take care of all the technical details.

> +st_err:
> +	dev_err(&client->dev, "m41t00_probe: Can't clear ST bit\n");
> +	goto attach_err;
> +ht_err:
> +	dev_err(&client->dev, "m41t00_probe: Can't clear HT bit\n");
> +	goto attach_err;
> +sqw_err:
> +	dev_err(&client->dev, "m41t00_probe: Can't set SQW Frequency\n");
> +attach_err:
> +	kfree(client);
> +	return rc;
>  }

Mrghh, this isn't exactly elegant... You should be able to make it
better if you switch to i2c_smbus_read/write_byte_data as suggested.

> diff -Nurp linux-2.6.16-mm1-cleanup/include/linux/m41t00.h linux-2.6.16-mm1-newsupp/include/linux/m41t00.h
> --- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
> +++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-27 16:25:51.000000000 -0700
> @@ -0,0 +1,50 @@
> +/*
> + * Definitions for the ST M41T00 family of i2c rtc chips.
> + *
> + * Author: Mark A. Greer <mgreer@mvista.com>
> + *
> + * 2005 (c) MontaVista Software, Inc. This file is licensed under

We're in year 2006 now :)

> + * the terms of the GNU General Public License version 2. This program
> + * is licensed "as is" without any warranty of any kind, whether express
> + * or implied.
> + */
> +
> +#ifndef _M41T00_H
> +#define _M41T00_H
> +
> +#define	M41T00_DRV_NAME		"m41t00"

Why do you need to export this?

> +#define	M41T00_I2C_ADDR		0x68

Not used anywhere?

> +#define	M41T00_TYPE_M41T00	0
> +#define	M41T00_TYPE_M41T81	81
> +#define	M41T00_TYPE_M41T85	85

The i2c core has a facility for drivers supporting several types of
devices. Check for I2C_CLIENT_INSMOD_3() in your case. The advantage if
you go that way is that chip types can be set from userspace through
module parameters, which may be convenient for testing when adding
support for new platforms.

> +/* SQW output disabled, this is default value by power on*/
> +#define SQW_FREQ_DISABLE	(0)
> +
> +#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
> +#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
> +#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
> +#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
> +#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
> +#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
> +#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
> +#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
> +#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
> +#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
> +#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
> +#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
> +#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
> +#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
> +#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */

Not used anywhere either?

> +extern ulong m41t00_get_rtc_time(void);
> +extern int m41t00_set_rtc_time(ulong nowtime);

Hopefully you won't have to export these anymore after you move to the
new RTC subsystem. But that's a different story. In the meantime,
shouldn't you have EXPORT_SYMBOL_GPL() for these functions? Else
compiling m41t00 as a module won't work.

Thanks,
-- 
Jean Delvare

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Mark A. Greer]

Hi Jean,

On Tue, Mar 28, 2006 at 05:54:50PM +0200, Jean Delvare wrote:
> Hi Mark,
> 
> Some more comments, sorry for being slow.

You're not slow at all.  No worries.

> >  drivers/i2c/chips/m41t00.c |  294 ++++++++++++++++++++++++++++++++++-----------
> >  include/linux/m41t00.h     |   50 +++++++
> 
> Who else is going to use this header file? It's rather rare to have a
> hardware-specific header file under include/linux.

The platform-specific code for any platform that uses that rtc will need
to include that header file so it can pass the platform data to the rtc
driver.  I have a patch for arch/ppc/platforms/katana.c but I haven't
submitted it b/c of the ppc->powerpc merge that's going on right now.
I will get it in there once I've moved the katana code into the merged tree.
There is a lot of work before that happens, though.

> > diff -Nurp linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c
> > --- linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c	2006-03-27 16:00:35.000000000 -0700
> > +++ linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c	2006-03-27 16:49:35.000000000 -0700
> > (...)
> >  static unsigned short ignore[] = { I2C_CLIENT_END };
> > -static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
> > +static unsigned short normal_addr[] = { 0, I2C_CLIENT_END };
> 
> Bad idea. If no platform data is found, the driver will attempt to
> probe I2C address 0, which is a broadcast address. I can predict
> interesting results ;)
>
> So, if you don't want the driver to do anything in the absence of
> platform data, you should define normal_addr the following way:

If there is no platform data, m41t00_platform_probe() should return
-ENODEV and make m41t00_init() causing the driver to not become active.
I've tested that, actually, and it seems to work.

> static unsigned short normal_addr[] = { I2C_CLIENT_END, I2C_CLIENT_END };

Hrm, that's a good idea.  I'll do that, even though it shouldn't be
necessary.

> > +struct m41t00_chip_info {
> > +	u16	type;
> > +	u16	read_limit;
> > +	u8	sec;		/* Offsets for chip regs */
> > +	u8	min;
> > +	u8	hour;
> > +	u8	day;
> > +	u8	mon;
> > +	u8	year;
> > +	u8	alarm_mon;
> > +	u8	alarm_hour;
> > +	u8	sqw;
> > +	u32	sqw_freq;
> > +};
> 
> u16 is probably overkill for .type and .read_limit.

Yeah, probably.  I'll change them to u8.

> > +static struct m41t00_chip_info m41t00_chip_info_tbl[] = {
> > +	{ M41T00_TYPE_M41T00, 5, 0, 1, 2, 4, 5, 6, 0, 0, 0, 0 },
> > +	{ M41T00_TYPE_M41T81, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
> > +	{ M41T00_TYPE_M41T85, 1, 1, 2, 3, 5, 6, 7, 0xa, 0xc, 0x13, 0 },
> > +};
> 
> C99-style initialization, please? It'll take much more lines, granted,
> but is also way more robust to future changes, and will be more
> readable too.

Well, the struct is right there so I figured the chances of changing the
struct and not seeing that the compile-time init code needs to be
changed too was small.  I guess its better to be safe than sorry and it
does make it more readable so I'll change it.

> May I ask why you define separate types for the M41T81 and the M41T85,
> when you handle both exactly the same way?

The only reason is so that its obvious that both the t81 and t85 are
supported.  If I make it M41T81 only then I can easily see someone
grep'ing around looking for M41T85, not finding it, and writing their
own driver.  I don't see any harm in having both, do you?

> > +		sec = buf[m41t00_chip->sec] & 0x7f;
> > +		min = buf[m41t00_chip->min] & 0x7f;
> > +		hour = buf[m41t00_chip->hour] & 0x3f;
> > +		day = buf[m41t00_chip->day] & 0x3f;
> > +		mon = buf[m41t00_chip->mon] & 0x1f;
> > +		year = buf[m41t00_chip->year] & 0xff;
> 
> The year masking is a no-op, you could omit it.

Yes.

> > @@ -169,24 +246,100 @@ m41t00_probe(struct i2c_adapter *adap, i
> >  {
> >  	struct i2c_client *client;
> >  	int rc;
> > +	u8 rbuf[1], wbuf[2], msgbuf[1] = { 0 }; /* offset into rtc's regs */
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr	= 0,
> > +			.flags	= 0,
> > +			.len	= 1,
> > +			.buf	= msgbuf,
> > +		},
> > +		{
> > +			.addr	= 0,
> > +			.flags	= I2C_M_RD,
> > +			.len	= 1,
> > +			.buf	= rbuf,
> > +		},
> > +	};
> 
> Why don't you initialize both .addr right away?

Actually, I already init the .addr field right away so I'll remove them
from them from the code above.

> >  
> >  	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
> >  	if (!client)
> >  		return -ENOMEM;
> >  
> > -	strlcpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
> > +	strlcpy(client->name, m41t00_driver.driver.name, I2C_NAME_SIZE);
> 
> The driver name may not be suitable here, the client name should
> instead reflect what chip it is.

Ah, good one.

> > +	if (m41t00_chip->type != M41T00_TYPE_M41T00) {
> > +		/* If asked, set SQW frequency & enable */
> > +		if (m41t00_chip->sqw_freq) {
> > +			msgbuf[0] = m41t00_chip->alarm_mon;
> > +			if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> > +				goto sqw_err;
> 
> You did not check whether the adapter you are attaching to supports this
> kind of transaction! You need to call i2c_check_functionality, see how
> other i2c chip drivers (for example ds1337) are doing. For i2c_transfer
> and i2c_master_send, you want to check for I2C_FUNC_I2C.

Okay.

> > +
> > +			wbuf[0] = m41t00_chip->alarm_mon;
> > +			wbuf[1] = rbuf[0] & ~0x40;
> > +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> > +				goto sqw_err;
> > +
> > +			wbuf[0] = m41t00_chip->sqw;
> > +			wbuf[1] = m41t00_chip->sqw_freq;
> > +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> > +				goto sqw_err;
> > +
> > +			wbuf[0] = m41t00_chip->alarm_mon;
> > +			wbuf[1] = rbuf[0] | 0x40;
> > +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> > +				goto sqw_err;
> > +		}
> > +
> > +		/* Make sure HT (Halt Update) bit is cleared */
> > +		msgbuf[0] = m41t00_chip->alarm_hour;
> > +		if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> > +			goto ht_err;
> > +
> > +		if (rbuf[0] & 0x40) {
> > +			wbuf[0] = m41t00_chip->alarm_hour;
> > +			wbuf[1] = rbuf[0] & ~0x40;
> > +			if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> > +				goto ht_err;
> > +		}
> > +	}
> > +
> > +	/* Make sure ST (stop) bit is cleared */
> > +	msgbuf[0] = m41t00_chip->sec;
> > +	if ((rc = i2c_transfer(client->adapter, msgs, 2)) < 0)
> > +		goto st_err;
> > +
> > +	if (rbuf[0] & 0x80) {
> > +		wbuf[0] = m41t00_chip->sec;
> > +		wbuf[1] = rbuf[0] & ~0x80;
> > +		if ((rc = i2c_master_send(client, wbuf, 2)) < 0)
> > +			goto st_err;
> >  	}
> 
> What you do here are basically SMBus Read Byte and SMBus Write Byte
> transactions. The code would be much more simple if you were using the
> i2c_smbus_read_byte_data and i2c_smbus_write_byte_data functions, which
> take care of all the technical details.

That's true but I assumed that since I was using i2c_transfer
earlier, I should use it here.  Is that a bad assumption?
I do see that ds1337.c uses both types.

> > +st_err:
> > +	dev_err(&client->dev, "m41t00_probe: Can't clear ST bit\n");
> > +	goto attach_err;
> > +ht_err:
> > +	dev_err(&client->dev, "m41t00_probe: Can't clear HT bit\n");
> > +	goto attach_err;
> > +sqw_err:
> > +	dev_err(&client->dev, "m41t00_probe: Can't set SQW Frequency\n");
> > +attach_err:
> > +	kfree(client);
> > +	return rc;
> >  }
> 
> Mrghh, this isn't exactly elegant... You should be able to make it
> better if you switch to i2c_smbus_read/write_byte_data as suggested.

Yep.

> > diff -Nurp linux-2.6.16-mm1-cleanup/include/linux/m41t00.h linux-2.6.16-mm1-newsupp/include/linux/m41t00.h
> > --- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
> > +++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-27 16:25:51.000000000 -0700
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Definitions for the ST M41T00 family of i2c rtc chips.
> > + *
> > + * Author: Mark A. Greer <mgreer@mvista.com>
> > + *
> > + * 2005 (c) MontaVista Software, Inc. This file is licensed under
> 
> We're in year 2006 now :)

Yes, but my understanding is that I should leave the 2005 there b/c that
file was already copyrighted with that year.  I could do a "2005, 2006"
if you like.

> > + * the terms of the GNU General Public License version 2. This program
> > + * is licensed "as is" without any warranty of any kind, whether express
> > + * or implied.
> > + */
> > +
> > +#ifndef _M41T00_H
> > +#define _M41T00_H
> > +
> > +#define	M41T00_DRV_NAME		"m41t00"
> 
> Why do you need to export this?

Because the code that passes the platform_data needs to put that name
in the data so that the driver can find it.  That's the value that the
driver uses to search for its platform_data.  I'll attach the patch to
the platform code that I used to test it so you can see what I mean.
Its at the end of this email.

> > +#define	M41T00_I2C_ADDR		0x68
> 
> Not used anywhere?

Yes, in the platform code.

> > +#define	M41T00_TYPE_M41T00	0
> > +#define	M41T00_TYPE_M41T81	81
> > +#define	M41T00_TYPE_M41T85	85
> 
> The i2c core has a facility for drivers supporting several types of
> devices. Check for I2C_CLIENT_INSMOD_3() in your case. The advantage if
> you go that way is that chip types can be set from userspace through
> module parameters, which may be convenient for testing when adding
> support for new platforms.

Okay, I'll take a look.

> > +/* SQW output disabled, this is default value by power on*/
> > +#define SQW_FREQ_DISABLE	(0)
> > +
> > +#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
> > +#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
> > +#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
> > +#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
> > +#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
> > +#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
> > +#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
> > +#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
> > +#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
> > +#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
> > +#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
> > +#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
> > +#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
> > +#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
> > +#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */
> 
> Not used anywhere either?

Platform code can pass one of these values into the driver via
platform_data (if its a t81 or t85, not needed for the t00).

> > +extern ulong m41t00_get_rtc_time(void);
> > +extern int m41t00_set_rtc_time(ulong nowtime);
> 
> Hopefully you won't have to export these anymore after you move to the
> new RTC subsystem. But that's a different story. In the meantime,
> shouldn't you have EXPORT_SYMBOL_GPL() for these functions? Else
> compiling m41t00 as a module won't work.

Good point.  Will fix.

Below is the katana patch.  Basically, it adds the platform_data
required for the m41t00 rtc which the m41t00 driver later takes out.
Note that the m41t00 doesn't need the sqw_freq value so you won't see
it being set in this patch.

I did have a couple questions above so I'll wait for your response
and then make a new patch.

Thanks for your time, Jean.

Mark
---

diff -Nurp linux-2.6.16-mm1-cleanup/arch/ppc/platforms/katana.c linux-2.6.16-mm1-newsupp/arch/ppc/platforms/katana.c
--- linux-2.6.16-mm1-cleanup/arch/ppc/platforms/katana.c	2006-03-23 16:15:22.000000000 -0700
+++ linux-2.6.16-mm1-newsupp/arch/ppc/platforms/katana.c	2006-03-23 17:37:46.000000000 -0700
@@ -28,6 +28,7 @@
 #include <linux/mtd/physmap.h>
 #include <linux/mv643xx.h>
 #include <linux/platform_device.h>
+#include <linux/m41t00.h>
 #ifdef CONFIG_BOOTIMG
 #include <linux/bootimg.h>
 #endif
@@ -843,8 +844,31 @@ katana_find_end_of_memory(void)
 }
 
 #if defined(CONFIG_I2C_MV64XXX) && defined(CONFIG_SENSORS_M41T00)
-extern ulong	m41t00_get_rtc_time(void);
-extern int	m41t00_set_rtc_time(ulong);
+static struct m41t00_platform_data katana_m41t00_pdata = {
+	.type = M41T00_TYPE_M41T00,
+	.i2c_addr = M41T00_I2C_ADDR,
+};
+
+static struct platform_device katana_m41t00_pdev = {
+	.name		= M41T00_DRV_NAME,
+	.id		= 0,
+	.num_resources	= 0,
+	.dev = {
+		.platform_data = &katana_m41t00_pdata,
+	},
+};
+
+static int __init
+katana_add_pdata(void)
+{
+	int rc;
+
+	if ((rc = platform_device_register(&katana_m41t00_pdev)))
+		platform_device_unregister(&katana_m41t00_pdev);
+
+	return rc;
+}
+arch_initcall(katana_add_pdata);
 
 static int __init
 katana_rtc_hookup(void)

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Jean Delvare]

Hi Mark,

Answering your questions before I forget about them and let a week pass
again...

Anything I don't comment on, means that you were right and I agree with
you.

> > May I ask why you define separate types for the M41T81 and the M41T85,
> > when you handle both exactly the same way?
> 
> The only reason is so that its obvious that both the t81 and t85 are
> supported.  If I make it M41T81 only then I can easily see someone
> grep'ing around looking for M41T85, not finding it, and writing their
> own driver.  I don't see any harm in having both, do you?

It wastes some memory, and you may later fix something for the M41T81
and forget to fix it for the M41T85.

If your only concern is to help grepers, you can add a clear list of
supported chips either as a comment at the top of the source file, or
as Documentation/i2c/chips/m41t00. That's what we do for hardware
monitoring chips.

No big deal anyway, so the decision is up to you.

> > What you do here are basically SMBus Read Byte and SMBus Write Byte
> > transactions. The code would be much more simple if you were using the
> > i2c_smbus_read_byte_data and i2c_smbus_write_byte_data functions, which
> > take care of all the technical details.
> 
> That's true but I assumed that since I was using i2c_transfer
> earlier, I should use it here.  Is that a bad assumption?
> I do see that ds1337.c uses both types.

Bad assumption, indeed. Nothing prevents you from using the smbus
functions and the i2c functions in the same driver, as long as your
call to i2c_check_functionality covers every function you use. So,
just use whatever makes your driver easier to write.

Thanks,
-- 
Jean Delvare

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Mark A. Greer]

Jean,

This is a complete replacement for the patch(es) with the same subject
submitted previously.  It is still against 2.6.16-mm1 but if you want
it against 2.6.16-mm2, let me know.

I kept separate entries for t81 and t85 b/c I also added a string with
the chip name.  That string is copied into the client struct and is
also used as the name of the workqueue thread.

I still have several error goto's in m41t00_probe().  If I don't,
I either need to have a bunch of returns in that routine (frowned
upon) or I have to get rid of the dev_err msgs which I think are
useful.  If you have a better way, let me know.

Other than that, I think I have addressed all of your concerns.
Sorry for all of the iterations.

Mark
---

This patch adds support for the ST m41t81 and m41t85 i2c rtc chips
to the existing m41t00 driver.

Since there is no way to reliably determine what type of rtc chip
is in use, the chip type is passed in via platform_data.  The i2c
address and square wave frequency are passed in via platform_data
as well.  To accommodate the use of platform_data, a new header
file include/linux/m41t00.h has been added.

The m41t81 and m41t85 chips halt the updating of their time registers
while they are being accessed.  They resume when a stop condition
exists on the i2c bus or when non-time related regs are accessed.
To make the best use of that facility and to make more efficient
use of the i2c bus, this patch replaces multiple i2c_smbus_xxx calls
with a single i2c_transfer call.

Signed-off-by: Mark A. Greer <mgreer@mvista.com>
---

 drivers/i2c/chips/m41t00.c |  320 ++++++++++++++++++++++++++++++++++-----------
 include/linux/m41t00.h     |   50 +++++++
 2 files changed, 296 insertions(+), 74 deletions(-)
---

diff -Nurp linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c
--- linux-2.6.16-mm1-cleanup/drivers/i2c/chips/m41t00.c	2006-03-28 12:58:43.000000000 -0700
+++ linux-2.6.16-mm1-newsupp/drivers/i2c/chips/m41t00.c	2006-03-28 14:39:28.000000000 -0700
@@ -1,9 +1,9 @@
 /*
- * I2C client/driver for the ST M41T00 Real-Time Clock chip.
+ * I2C client/driver for the ST M41T00 family of i2c rtc chips.
  *
  * Author: Mark A. Greer <mgreer@mvista.com>
  *
- * 2005 (c) MontaVista Software, Inc. This file is licensed under
+ * 2005, 2006 (c) MontaVista Software, Inc. This file is licensed under
  * the terms of the GNU General Public License version 2. This program
  * is licensed "as is" without any warranty of any kind, whether express
  * or implied.
@@ -19,21 +19,17 @@
 #include <linux/i2c.h>
 #include <linux/rtc.h>
 #include <linux/bcd.h>
-#include <linux/mutex.h>
 #include <linux/workqueue.h>
-
+#include <linux/platform_device.h>
+#include <linux/m41t00.h>
 #include <asm/time.h>
 #include <asm/rtc.h>
 
-#define	M41T00_DRV_NAME		"m41t00"
-
-static DEFINE_MUTEX(m41t00_mutex);
-
 static struct i2c_driver m41t00_driver;
 static struct i2c_client *save_client;
 
 static unsigned short ignore[] = { I2C_CLIENT_END };
-static unsigned short normal_addr[] = { 0x68, I2C_CLIENT_END };
+static unsigned short normal_addr[] = { I2C_CLIENT_END, I2C_CLIENT_END };
 
 static struct i2c_client_address_data addr_data = {
 	.normal_i2c	= normal_addr,
@@ -41,34 +37,92 @@ static struct i2c_client_address_data ad
 	.ignore		= ignore,
 };
 
+struct m41t00_chip_info {
+	u8	type;
+	char	*name;
+	u8	read_limit;
+	u8	sec;		/* Offsets for chip regs */
+	u8	min;
+	u8	hour;
+	u8	day;
+	u8	mon;
+	u8	year;
+	u8	alarm_mon;
+	u8	alarm_hour;
+	u8	sqw;
+	u8	sqw_freq;
+};
+
+static struct m41t00_chip_info m41t00_chip_info_tbl[] = {
+	{
+		.type		= M41T00_TYPE_M41T00,
+		.name		= "m41t00",
+		.read_limit	= 5,
+		.sec		= 0,
+		.min		= 1,
+		.hour		= 2,
+		.day		= 4,
+		.mon		= 5,
+		.year		= 6,
+	},
+	{
+		.type		= M41T00_TYPE_M41T81,
+		.name		= "m41t81",
+		.read_limit	= 1,
+		.sec		= 1,
+		.min		= 2,
+		.hour		= 3,
+		.day		= 5,
+		.mon		= 6,
+		.year		= 7,
+		.alarm_mon	= 0xa,
+		.alarm_hour	= 0xc,
+		.sqw		= 0x13,
+	},
+	{
+		.type		= M41T00_TYPE_M41T85,
+		.name		= "m41t85",
+		.read_limit	= 1,
+		.sec		= 1,
+		.min		= 2,
+		.hour		= 3,
+		.day		= 5,
+		.mon		= 6,
+		.year		= 7,
+		.alarm_mon	= 0xa,
+		.alarm_hour	= 0xc,
+		.sqw		= 0x13,
+	},
+};
+static struct m41t00_chip_info *m41t00_chip;
+
 ulong
 m41t00_get_rtc_time(void)
 {
 	s32 sec, min, hour, day, mon, year;
 	s32 sec1, min1, hour1, day1, mon1, year1;
-	ulong limit = 10;
+	u8 reads = 0;
+	u8 buf[8], msgbuf[1] = { 0 }; /* offset into rtc's regs */
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= save_client->addr,
+			.flags	= 0,
+			.len	= 1,
+			.buf	= msgbuf,
+		},
+		{
+			.addr	= save_client->addr,
+			.flags	= I2C_M_RD,
+			.len	= 8,
+			.buf	= buf,
+		},
+	};
 
 	sec = min = hour = day = mon = year = 0;
-	sec1 = min1 = hour1 = day1 = mon1 = year1 = 0;
 
-	mutex_lock(&m41t00_mutex);
 	do {
-		if (((sec = i2c_smbus_read_byte_data(save_client, 0)) >= 0)
-			&& ((min = i2c_smbus_read_byte_data(save_client, 1))
-				>= 0)
-			&& ((hour = i2c_smbus_read_byte_data(save_client, 2))
-				>= 0)
-			&& ((day = i2c_smbus_read_byte_data(save_client, 4))
-				>= 0)
-			&& ((mon = i2c_smbus_read_byte_data(save_client, 5))
-				>= 0)
-			&& ((year = i2c_smbus_read_byte_data(save_client, 6))
-				>= 0)
-			&& ((sec == sec1) && (min == min1) && (hour == hour1)
-				&& (day == day1) && (mon == mon1)
-				&& (year == year1)))
-
-				break;
+		if (i2c_transfer(save_client->adapter, msgs, 2) < 0)
+			goto read_err;
 
 		sec1 = sec;
 		min1 = min;
@@ -76,21 +130,21 @@ m41t00_get_rtc_time(void)
 		day1 = day;
 		mon1 = mon;
 		year1 = year;
-	} while (--limit > 0);
-	mutex_unlock(&m41t00_mutex);
-
-	if (limit == 0) {
-		dev_warn(&save_client->dev,
-			"m41t00: can't read rtc chip\n");
-		sec = min = hour = day = mon = year = 0;
-	}
 
-	sec &= 0x7f;
-	min &= 0x7f;
-	hour &= 0x3f;
-	day &= 0x3f;
-	mon &= 0x1f;
-	year &= 0xff;
+		sec = buf[m41t00_chip->sec] & 0x7f;
+		min = buf[m41t00_chip->min] & 0x7f;
+		hour = buf[m41t00_chip->hour] & 0x3f;
+		day = buf[m41t00_chip->day] & 0x3f;
+		mon = buf[m41t00_chip->mon] & 0x1f;
+		year = buf[m41t00_chip->year];
+	} while ((++reads < m41t00_chip->read_limit) && ((sec != sec1)
+			|| (min != min1) || (hour != hour1) || (day != day1)
+			|| (mon != mon1) || (year != year1)));
+
+	if ((m41t00_chip->read_limit > 1) && ((sec != sec1) || (min != min1)
+			|| (hour != hour1) || (day != day1) || (mon != mon1)
+			|| (year != year1)))
+		goto read_err;
 
 	sec = BCD2BIN(sec);
 	min = BCD2BIN(min);
@@ -104,40 +158,60 @@ m41t00_get_rtc_time(void)
 		year += 100;
 
 	return mktime(year, mon, day, hour, min, sec);
+
+read_err:
+	dev_err(&save_client->dev, "m41t00_get_rtc_time: Read error\n");
+	return 0;
 }
+EXPORT_SYMBOL_GPL(m41t00_get_rtc_time);
 
 static void
 m41t00_set(void *arg)
 {
 	struct rtc_time	tm;
-	ulong nowtime = *(ulong *)arg;
+	int nowtime = *(int *)arg;
+	s32 sec, min, hour, day, mon, year;
+	u8 wbuf[9], *buf = &wbuf[1], msgbuf[1] = { 0 };
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= save_client->addr,
+			.flags	= 0,
+			.len	= 1,
+			.buf	= msgbuf,
+		},
+		{
+			.addr	= save_client->addr,
+			.flags	= I2C_M_RD,
+			.len	= 8,
+			.buf	= buf,
+		},
+	};
 
 	to_tm(nowtime, &tm);
 	tm.tm_year = (tm.tm_year - 1900) % 100;
 
-	tm.tm_sec = BIN2BCD(tm.tm_sec);
-	tm.tm_min = BIN2BCD(tm.tm_min);
-	tm.tm_hour = BIN2BCD(tm.tm_hour);
-	tm.tm_mon = BIN2BCD(tm.tm_mon);
-	tm.tm_mday = BIN2BCD(tm.tm_mday);
-	tm.tm_year = BIN2BCD(tm.tm_year);
-
-	mutex_lock(&m41t00_mutex);
-	if ((i2c_smbus_write_byte_data(save_client, 0, tm.tm_sec & 0x7f) < 0)
-		|| (i2c_smbus_write_byte_data(save_client, 1, tm.tm_min & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 2, tm.tm_hour & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 4, tm.tm_mday & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 5, tm.tm_mon & 0x7f)
-			< 0)
-		|| (i2c_smbus_write_byte_data(save_client, 6, tm.tm_year & 0x7f)
-			< 0))
+	sec = BIN2BCD(tm.tm_sec);
+	min = BIN2BCD(tm.tm_min);
+	hour = BIN2BCD(tm.tm_hour);
+	day = BIN2BCD(tm.tm_mday);
+	mon = BIN2BCD(tm.tm_mon);
+	year = BIN2BCD(tm.tm_year);
+
+	/* Read reg values into buf[0..7]/wbuf[1..8] */
+	if (i2c_transfer(save_client->adapter, msgs, 2) < 0) {
+		dev_err(&save_client->dev, "m41t00_set: Read error\n");
+		return;
+	}
 
-		dev_warn(&save_client->dev,"m41t00: can't write to rtc chip\n");
+	wbuf[0] = 0; /* offset into rtc's regs */
+	buf[m41t00_chip->sec] = (buf[m41t00_chip->sec] & ~0x7f) | (sec & 0x7f);
+	buf[m41t00_chip->min] = (buf[m41t00_chip->min] & ~0x7f) | (min & 0x7f);
+	buf[m41t00_chip->hour] = (buf[m41t00_chip->hour]& ~0x3f) | (hour& 0x3f);
+	buf[m41t00_chip->day] = (buf[m41t00_chip->day] & ~0x3f) | (day & 0x3f);
+	buf[m41t00_chip->mon] = (buf[m41t00_chip->mon] & ~0x1f) | (mon & 0x1f);
 
-	mutex_unlock(&m41t00_mutex);
+	if (i2c_master_send(save_client, wbuf, 9) < 0)
+		dev_err(&save_client->dev, "m41t00_set: Write error\n");
 }
 
 static ulong new_time;
@@ -156,6 +230,48 @@ m41t00_set_rtc_time(ulong nowtime)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(m41t00_set_rtc_time);
+
+/*
+ *****************************************************************************
+ *
+ *	platform_data Driver Interface
+ *
+ *****************************************************************************
+ */
+static int __init
+m41t00_platform_probe(struct platform_device *pdev)
+{
+	struct m41t00_platform_data *pdata;
+	int i;
+
+	if (pdev && (pdata = pdev->dev.platform_data)) {
+		normal_addr[0] = pdata->i2c_addr;
+
+		for (i=0; i<ARRAY_SIZE(m41t00_chip_info_tbl); i++)
+			if (m41t00_chip_info_tbl[i].type == pdata->type) {
+				m41t00_chip = &m41t00_chip_info_tbl[i];
+				m41t00_chip->sqw_freq = pdata->sqw_freq;
+				return 0;
+			}
+	}
+	return -ENODEV;
+}
+
+static int __exit
+m41t00_platform_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver m41t00_platform_driver = {
+	.probe  = m41t00_platform_probe,
+	.remove = m41t00_platform_remove,
+	.driver = {
+		.owner = THIS_MODULE,
+		.name  = M41T00_DRV_NAME,
+	},
+};
 
 /*
  *****************************************************************************
@@ -168,25 +284,76 @@ static int
 m41t00_probe(struct i2c_adapter *adap, int addr, int kind)
 {
 	struct i2c_client *client;
-	int rc;
+	int rc = -EINVAL;
 
-	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
-	if (!client)
-		return -ENOMEM;
+	if (!i2c_check_functionality(adap, I2C_FUNC_I2C
+			| I2C_FUNC_SMBUS_BYTE_DATA))
+		goto early_err;
+
+	if (!(client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
+		rc = -ENOMEM;
+		goto early_err;
+	}
 
-	strlcpy(client->name, M41T00_DRV_NAME, I2C_NAME_SIZE);
+	strlcpy(client->name, m41t00_chip->name, I2C_NAME_SIZE);
 	client->addr = addr;
 	client->adapter = adap;
 	client->driver = &m41t00_driver;
 
-	if ((rc = i2c_attach_client(client)) != 0) {
-		kfree(client);
-		return rc;
+	if ((rc = i2c_attach_client(client)))
+		goto attach_err;
+
+	if (m41t00_chip->type != M41T00_TYPE_M41T00) {
+		/* If asked, disable SQW, set SQW frequency & re-enable */
+		if (m41t00_chip->sqw_freq)
+			if (((rc = i2c_smbus_read_byte_data(client,
+					m41t00_chip->alarm_mon)) < 0)
+				|| ((rc = i2c_smbus_write_byte_data(client,
+					m41t00_chip->alarm_mon, rc & ~0x40)) <0)
+				|| ((rc = i2c_smbus_write_byte_data(client,
+					m41t00_chip->sqw,
+					m41t00_chip->sqw_freq)) < 0)
+				|| ((rc = i2c_smbus_write_byte_data(client,
+					m41t00_chip->alarm_mon, rc | 0x40)) <0))
+
+				goto sqw_err;
+
+		/* Make sure HT (Halt Update) bit is cleared */
+		if ((rc = i2c_smbus_read_byte_data(client,
+				m41t00_chip->alarm_hour)) < 0)
+			goto ht_err;
+
+		if (rc & 0x40) 
+			if ((rc = i2c_smbus_write_byte_data(client,
+					m41t00_chip->alarm_hour, rc & ~0x40))<0)
+				goto ht_err;
 	}
 
-	m41t00_wq = create_singlethread_workqueue("m41t00");
+	/* Make sure ST (stop) bit is cleared */
+	if ((rc = i2c_smbus_read_byte_data(client, m41t00_chip->sec)) < 0)
+		goto st_err;
+
+	if (rc & 0x80) 
+		if ((rc = i2c_smbus_write_byte_data(client, m41t00_chip->sec,
+				rc & ~0x80)) < 0)
+			goto st_err;
+
+	m41t00_wq = create_singlethread_workqueue(m41t00_chip->name);
 	save_client = client;
 	return 0;
+
+st_err:
+	dev_err(&client->dev, "m41t00_probe: Can't clear ST bit\n");
+	goto attach_err;
+ht_err:
+	dev_err(&client->dev, "m41t00_probe: Can't clear HT bit\n");
+	goto attach_err;
+sqw_err:
+	dev_err(&client->dev, "m41t00_probe: Can't set SQW Frequency\n");
+attach_err:
+	kfree(client);
+early_err:
+	return rc;
 }
 
 static int
@@ -219,13 +386,18 @@ static struct i2c_driver m41t00_driver =
 static int __init
 m41t00_init(void)
 {
-	return i2c_add_driver(&m41t00_driver);
+	int rc;
+
+	if (!(rc = platform_driver_register(&m41t00_platform_driver)))
+		rc = i2c_add_driver(&m41t00_driver);
+	return rc;
 }
 
 static void __exit
 m41t00_exit(void)
 {
 	i2c_del_driver(&m41t00_driver);
+	platform_driver_unregister(&m41t00_platform_driver);
 }
 
 module_init(m41t00_init);
diff -Nurp linux-2.6.16-mm1-cleanup/include/linux/m41t00.h linux-2.6.16-mm1-newsupp/include/linux/m41t00.h
--- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
+++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-28 14:36:56.000000000 -0700
@@ -0,0 +1,50 @@
+/*
+ * Definitions for the ST M41T00 family of i2c rtc chips.
+ *
+ * Author: Mark A. Greer <mgreer@mvista.com>
+ *
+ * 2005 (c) MontaVista Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+
+#ifndef _M41T00_H
+#define _M41T00_H
+
+#define	M41T00_DRV_NAME		"m41t00"
+#define	M41T00_I2C_ADDR		0x68
+
+#define	M41T00_TYPE_M41T00	0
+#define	M41T00_TYPE_M41T81	81
+#define	M41T00_TYPE_M41T85	85
+
+struct m41t00_platform_data {
+	u8	type;
+	u8	i2c_addr;
+	u8	sqw_freq;
+};
+
+/* SQW output disabled, this is default value by power on*/
+#define SQW_FREQ_DISABLE	(0)
+
+#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
+#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
+#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
+#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
+#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
+#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
+#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
+#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
+#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
+#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
+#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
+#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
+#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
+#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
+#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */
+
+extern ulong m41t00_get_rtc_time(void);
+extern int m41t00_set_rtc_time(ulong nowtime);
+
+#endif /* _M41T00_H */

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Jean Delvare]

Hi Mark,

> This is a complete replacement for the patch(es) with the same subject
> submitted previously.  It is still against 2.6.16-mm1 but if you want
> it against 2.6.16-mm2, let me know.
> 
> I kept separate entries for t81 and t85 b/c I also added a string with
> the chip name.  That string is copied into the client struct and is
> also used as the name of the workqueue thread.

Fine with me.

> I still have several error goto's in m41t00_probe().  If I don't,
> I either need to have a bunch of returns in that routine (frowned
> upon) or I have to get rid of the dev_err msgs which I think are
> useful.  If you have a better way, let me know.

No immediate idea and I don't quite have the time to work on it. If
anyone is unhappy with it, that person gets to suggest a better way ;)

> Other than that, I think I have addressed all of your concerns.
> Sorry for all of the iterations.

No worry about the iterations, good patches go that path, methinks. And
I take my part of responsability for the slow processing.

I'm fine with almost everything now, except for a couple things below,
which I'll change myself if you're OK with my suggestions, before
pushing the patch upwards:

>  m41t00_probe(struct i2c_adapter *adap, int addr, int kind)
>  {
>  	struct i2c_client *client;
> -	int rc;
> +	int rc = -EINVAL;
>  
> -	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
> -	if (!client)
> -		return -ENOMEM;
> +	if (!i2c_check_functionality(adap, I2C_FUNC_I2C
> +			| I2C_FUNC_SMBUS_BYTE_DATA))
> +		goto early_err;

Good check, but bad return value, given the way the i2c subsystem works
for now. There is no error at this point, your i2c driver is simply not
compatible with one of the i2c busses which exist on the system. You
must return 0 (no error.)

> +
> +	if (!(client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
> +		rc = -ENOMEM;
> +		goto early_err;
> +	}

And this change doesn't really win anything compared to the original
code. So I'd suggest:

@@ -170,23 +286,72 @@
 	struct i2c_client *client;
 	int rc;
 
+	if (!i2c_check_functionality(adap, I2C_FUNC_I2C
+			| I2C_FUNC_SMBUS_BYTE_DATA))
+		return 0;
+
 	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
 	if (!client)
 		return -ENOMEM;

This minimizes the changes while doing the right thing.

> --- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
> +++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-28 14:36:56.000000000 -0700
> @@ -0,0 +1,50 @@
> +/*
> + * Definitions for the ST M41T00 family of i2c rtc chips.
> + *
> + * Author: Mark A. Greer <mgreer@mvista.com>
> + *
> + * 2005 (c) MontaVista Software, Inc. This file is licensed under

Still no 2006? You added it to the driver file so I guess you want to
do the same here.

> +/* SQW output disabled, this is default value by power on*/
> +#define SQW_FREQ_DISABLE	(0)
> +
> +#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
> +#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
> +#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
> +#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
> +#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
> +#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
> +#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
> +#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
> +#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
> +#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
> +#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
> +#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
> +#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
> +#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
> +#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */

It just hit me that, given that these defines are in a header file,
they should have a M41T00_ prefix. Don't you think? If so, in order not
to make the symbol names too long, we can probably remove the FREQ
part, which doesn't really add anything. So I'd go with:

+/* SQW output disabled, this is default value by power on */
+#define M41T00_SQW_DISABLE     (0)
+
+#define M41T00_SQW_32KHZ       (1<<4)          /* 32.768 KHz */
+#define M41T00_SQW_8KHZ                (2<<4)          /* 8.192 KHz */
+#define M41T00_SQW_4KHZ                (3<<4)          /* 4.096 KHz */
+#define M41T00_SQW_2KHZ                (4<<4)          /* 2.048 KHz */
+#define M41T00_SQW_1KHZ                (5<<4)          /* 1.024 KHz */
+#define M41T00_SQW_512HZ       (6<<4)          /* 512 Hz */
+#define M41T00_SQW_256HZ       (7<<4)          /* 256 Hz */
+#define M41T00_SQW_128HZ       (8<<4)          /* 128 Hz */
+#define M41T00_SQW_64HZ                (9<<4)          /* 64 Hz */
+#define M41T00_SQW_32HZ                (10<<4)         /* 32 Hz */
+#define M41T00_SQW_16HZ                (11<<4)         /* 16 Hz */
+#define M41T00_SQW_8HZ         (12<<4)         /* 8 Hz */
+#define M41T00_SQW_4HZ         (13<<4)         /* 4 Hz */
+#define M41T00_SQW_2HZ         (14<<4)         /* 2 Hz */
+#define M41T00_SQW_1HZ         (15<<4)         /* 1 Hz */

That's it. If you're OK with my changes, just tell so and we go with my
version. If anything isn't OK, no problem, but then you get to resend a
full patch with everything the way you want.

Thanks,
-- 
Jean Delvare

Re: [PATCH 2.6.16-mm1 3/3] rtc: add support for m41t81 & m41t85 chips to m41t00 driver

[Mark A. Greer]

Hi Jean,

On Sat, Apr 01, 2006 at 07:20:40PM +0200, Jean Delvare wrote:

<snip>

> > I either need to have a bunch of returns in that routine (frowned
> > upon) or I have to get rid of the dev_err msgs which I think are
> > useful.  If you have a better way, let me know.
> 
> No immediate idea and I don't quite have the time to work on it. If
> anyone is unhappy with it, that person gets to suggest a better way ;)

Works for me!

> > Other than that, I think I have addressed all of your concerns.
> > Sorry for all of the iterations.
> 
> No worry about the iterations, good patches go that path, methinks. And
> I take my part of responsability for the slow processing.
> 
> I'm fine with almost everything now, except for a couple things below,
> which I'll change myself if you're OK with my suggestions, before
> pushing the patch upwards:
> 
> >  m41t00_probe(struct i2c_adapter *adap, int addr, int kind)
> >  {
> >  	struct i2c_client *client;
> > -	int rc;
> > +	int rc = -EINVAL;
> >  
> > -	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
> > -	if (!client)
> > -		return -ENOMEM;
> > +	if (!i2c_check_functionality(adap, I2C_FUNC_I2C
> > +			| I2C_FUNC_SMBUS_BYTE_DATA))
> > +		goto early_err;
> 
> Good check, but bad return value, given the way the i2c subsystem works
> for now. There is no error at this point, your i2c driver is simply not
> compatible with one of the i2c busses which exist on the system. You
> must return 0 (no error.)
> 
> > +
> > +	if (!(client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
> > +		rc = -ENOMEM;
> > +		goto early_err;
> > +	}
> 
> And this change doesn't really win anything compared to the original
> code. So I'd suggest:
> 
> @@ -170,23 +286,72 @@
>  	struct i2c_client *client;
>  	int rc;
>  
> +	if (!i2c_check_functionality(adap, I2C_FUNC_I2C
> +			| I2C_FUNC_SMBUS_BYTE_DATA))
> +		return 0;
> +
>  	client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL);
>  	if (!client)
>  		return -ENOMEM;
> 
> This minimizes the changes while doing the right thing.

I'm happy with that.

> > --- linux-2.6.16-mm1-cleanup/include/linux/m41t00.h	1969-12-31 17:00:00.000000000 -0700
> > +++ linux-2.6.16-mm1-newsupp/include/linux/m41t00.h	2006-03-28 14:36:56.000000000 -0700
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Definitions for the ST M41T00 family of i2c rtc chips.
> > + *
> > + * Author: Mark A. Greer <mgreer@mvista.com>
> > + *
> > + * 2005 (c) MontaVista Software, Inc. This file is licensed under
> 
> Still no 2006? You added it to the driver file so I guess you want to
> do the same here.

Oops, missed that one.  My bad.

> > +/* SQW output disabled, this is default value by power on*/
> > +#define SQW_FREQ_DISABLE	(0)
> > +
> > +#define SQW_FREQ_32KHZ	(1<<4)		/* 32.768 KHz */
> > +#define SQW_FREQ_8KHZ	(2<<4)		/* 8.192 KHz */
> > +#define SQW_FREQ_4KHZ	(3<<4)		/* 4.096 KHz */
> > +#define SQW_FREQ_2KHZ	(4<<4)		/* 2.048 KHz */
> > +#define SQW_FREQ_1KHZ	(5<<4)		/* 1.024 KHz */
> > +#define SQW_FREQ_512HZ	(6<<4)		/* 512 Hz */
> > +#define SQW_FREQ_256HZ	(7<<4)		/* 256 Hz */
> > +#define SQW_FREQ_128HZ	(8<<4)		/* 128 Hz */
> > +#define SQW_FREQ_64HZ	(9<<4)		/* 64 Hz */
> > +#define SQW_FREQ_32HZ	(10<<4)		/* 32 Hz */
> > +#define SQW_FREQ_16HZ	(11<<4)		/* 16 Hz */
> > +#define SQW_FREQ_8HZ	(12<<4)		/* 8 Hz */
> > +#define SQW_FREQ_4HZ	(13<<4)		/* 4 Hz */
> > +#define SQW_FREQ_2HZ	(14<<4)		/* 2 Hz */
> > +#define SQW_FREQ_1HZ	(15<<4)		/* 1 Hz */
> 
> It just hit me that, given that these defines are in a header file,
> they should have a M41T00_ prefix. Don't you think?

I agree. 

> If so, in order not
> to make the symbol names too long, we can probably remove the FREQ
> part, which doesn't really add anything. So I'd go with:
> 
> +/* SQW output disabled, this is default value by power on */
> +#define M41T00_SQW_DISABLE     (0)
> +
> +#define M41T00_SQW_32KHZ       (1<<4)          /* 32.768 KHz */
> +#define M41T00_SQW_8KHZ                (2<<4)          /* 8.192 KHz */
> +#define M41T00_SQW_4KHZ                (3<<4)          /* 4.096 KHz */
> +#define M41T00_SQW_2KHZ                (4<<4)          /* 2.048 KHz */
> +#define M41T00_SQW_1KHZ                (5<<4)          /* 1.024 KHz */
> +#define M41T00_SQW_512HZ       (6<<4)          /* 512 Hz */
> +#define M41T00_SQW_256HZ       (7<<4)          /* 256 Hz */
> +#define M41T00_SQW_128HZ       (8<<4)          /* 128 Hz */
> +#define M41T00_SQW_64HZ                (9<<4)          /* 64 Hz */
> +#define M41T00_SQW_32HZ                (10<<4)         /* 32 Hz */
> +#define M41T00_SQW_16HZ                (11<<4)         /* 16 Hz */
> +#define M41T00_SQW_8HZ         (12<<4)         /* 8 Hz */
> +#define M41T00_SQW_4HZ         (13<<4)         /* 4 Hz */
> +#define M41T00_SQW_2HZ         (14<<4)         /* 2 Hz */
> +#define M41T00_SQW_1HZ         (15<<4)         /* 1 Hz */
> 
> That's it. If you're OK with my changes, just tell so and we go with my
> version. If anything isn't OK, no problem, but then you get to resend a
> full patch with everything the way you want.

I'm happy with all your changes.  Thanks again for you time, Jean.

Mark