Skip to contentSkip to conversation

i2c chips: ds1337 RTC driver

PATCH In review James Chapman 4 participants 13 messages Feb 28, 2005 – May 19, 2005

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 i2c chips: ds1337 RTC driver

Add DS1337 RTC chip driver.

drivers/i2c/chips/Kconfig +11 -0
diff -Nru a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
@@ -62,6 +62,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called asb100.
 
+config SENSORS_DS1337
+      	tristate "Dallas Semiconductor DS1337 Real Time Clock"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Dallas Semiconductor
+	  DS1337 real-time clock chips. 
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ds1337.
+
 config SENSORS_DS1621
       	tristate "Dallas Semiconductor DS1621 and DS1625"
 	depends on I2C && EXPERIMENTAL
drivers/i2c/chips/Makefile +1 -0
diff -Nru a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
drivers/i2c/chips/ds1337.c new file +394 -0

394 lines not on this page · raw message

#1 lore raw

drivers/i2c/chips/ds1337.c line 17

Quoted 2 lines · James Chapman
drivers/i2c/chips/ds1337.c new file +2 -0
+/* Define to compile in pr_debug() trace */
+#undef DEBUG

Not needed, we do this in the makefile now.

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (debug >= 1)
+		pr_debug("%s: client=%p, dt=%p\n", __FUNCTION__, client, dt);

Please use the dev_dbg(), dev_err() and friend functions instead of pr_debug(). It provides a sane user interface that all of the other drivers use.

thanks,

greg k-h

#2 lore raw

Revised ds1337 chip driver patch.

- change all driver log messages to use dev_dbg() or dev_err()
- remove debug module parameter

Quoted 17 lines · Greg KH

Review

Quoted 2 lines · James Chapman
(quoted diff) +2 -0
+/* Define to compile in pr_debug() trace */
+#undef DEBUG

Not needed, we do this in the makefile now.

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (debug >= 1)
+		pr_debug("%s: client=%p, dt=%p\n", __FUNCTION__, client, dt);

Please use the dev_dbg(), dev_err() and friend functions instead of pr_debug(). It provides a sane user interface that all of the other drivers use.

Quoted 3 lines

thanks,

greg k-h

drivers/i2c/chips/Kconfig +11 -0
diff -Nru a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
@@ -62,6 +62,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called asb100.
 
+config SENSORS_DS1337
+      	tristate "Dallas Semiconductor DS1337 Real Time Clock"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Dallas Semiconductor
+	  DS1337 real-time clock chips. 
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ds1337.
+
 config SENSORS_DS1621
       	tristate "Dallas Semiconductor DS1621 and DS1625"
 	depends on I2C && EXPERIMENTAL
drivers/i2c/chips/Makefile +1 -0
diff -Nru a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
drivers/i2c/chips/ds1337.c new file +383 -0

383 lines not on this page · raw message

#3 lore raw

Hi James,

Quoted 11 lines

diff -Nru a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig

drivers/i2c/chips/Kconfig +4 -0
@@ -62,6 +62,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called asb100.
 
+config SENSORS_DS1337
+      	tristate "Dallas Semiconductor DS1337 Real Time Clock"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR

drivers/i2c/chips/Kconfig line 78

Quoted 10 lines
drivers/i2c/chips/Kconfig +7 -0
+	help
+	  If you say yes here you get support for Dallas Semiconductor
+	  DS1337 real-time clock chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ds1337.
+
 config SENSORS_DS1621
       	tristate "Dallas Semiconductor DS1621 and DS1625"
 	depends on I2C && EXPERIMENTAL

I don't think it belongs there. The DS1337 is not a hardware monitoring chip, please move it down to the "Other I2C Chip support" menu. I also think I see an indentation issue on the "tristate" line, seemingly copied from the SENSORS_DS1621 section which would need to be fixed as well.

drivers/i2c/chips/Kconfig

Quoted 9 lines
drivers/i2c/chips/Kconfig +9 -0
+static struct i2c_driver ds1337_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "ds1337",
+	.id		= I2C_DRIVERID_DS1337,
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= ds1337_attach_adapter,
+	.detach_client	= ds1337_detach_client,
+	.command	= ds1337_command,
+};

I2C_DRIVERID_DS1337 isn't defined anywhere that I can see, so your driver will hardly compile. You don't seem to actually need a driver id, so you can simply omit it.

drivers/i2c/chips/Kconfig

Quoted 12 lines

+static inline int ds1337_read(struct i2c_client *client, u8 reg, u8

	*value)
drivers/i2c/chips/Kconfig +10 -0
+{
+	s32 tmp = i2c_smbus_read_byte_data(client, reg);
+
+	if (tmp < 0)
+		return -EIO;
+
+	*value = (u8) (tmp & 0xff);
+
+	return 0;
+}

The bitmasking is a no-op, and so is probably the cast as I'd expect the compiler to do the right thing on its own.

drivers/i2c/chips/Kconfig

Quoted 4 lines
drivers/i2c/chips/Kconfig +3 -0
+static int ds1337_get_datetime(struct i2c_client *client, struct rtc_time > 	*dt)
+{
(...)
+	if (!dt || !client) {

How could client be NULL? Same question for ds1337_set_datetime.

drivers/i2c/chips/Kconfig

Quoted 1 line
drivers/i2c/chips/Kconfig +1 -0
+	memset(buf, 0, sizeof(buf));

Why is this memset needed at all?

drivers/i2c/chips/Kconfig

Quoted 3 lines
drivers/i2c/chips/Kconfig +3 -0
+	result = client->adapter->algo->master_xfer(client->adapter,
+						    &msg[0],
+						    2);

You did not check that the adapter was capable of raw I2C transfers.

drivers/i2c/chips/Kconfig

Quoted 4 lines
drivers/i2c/chips/Kconfig +4 -0
+static int ds1337_attach_adapter(struct i2c_adapter *adapter)
+{
+	if (!(adapter->class & I2C_CLASS_HWMON))
+		return 0;

You do not want to check that. The DS1337 is not a hardware monitoring device!

Quoted 2 lines

+static int ds1337_detect(struct i2c_adapter *adapter, int address, int
kind)

+{
+ struct i2c_client *new_client;
+ struct ds1337_data *data;
+ int err = 0;
+ const char *name = "";
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+ goto exit;

You must verify the ability to do raw I2C transfers here, as said above.

+ err = -ENODEV;

Not correct. The detection function should NOT return a non-zero value unless it meets a critical error (such as memory shortage). Just finding a different device from what you were looking for isn't an error.

drivers/i2c/chips/Kconfig

Quoted 8 lines
drivers/i2c/chips/Kconfig +8 -0
+		/* Check that control register bits 5-6 are zero */
+		if (buf[14] & ((1 << 5) | (1 << 6)))
+			goto exit_free;
+
+		/* Check that status register bits 2-6 are zero */
+		if (buf[15] & ((1 << 2) | (1 << 3) | (1 << 4) |
+			       (1 << 5) | (1 << 6)))
+			goto exit_free;

These would be much more readable as "& 0x60" and "& 0x7c" if you want my opinion (but maybe it's just me). Also, I usually write "bits 6-2", not "2-6" (but again that might be just me).

drivers/i2c/chips/Kconfig

Quoted 1 line
drivers/i2c/chips/Kconfig +1 -0
+	new_client->id = ds1337_id++;

Client ids are not used by the core and were dropped recently. Don't use that struct member.

drivers/i2c/chips/Kconfig

Quoted 8 lines
drivers/i2c/chips/Kconfig +8 -0
+static void ds1337_init_client(struct i2c_client *client)
+{
+	u8 val;
+
+	/* Ensure that device is set in 24-hour mode */
+	val = i2c_smbus_read_byte_data(client, 2);
+	i2c_smbus_write_byte_data(client, 2, val | (1 << 6));
+}

You can probably spare a write if the device was already in 24-hour mode. It might also be nice to give register 2 a name (DS1337_REG_CONFIG?) and use that.

Also note that you are not handling errors properly here. You could get -1 returned from the read, forcing all bits of the register to 1 on subsequent write.

--
Jean Delvare

#4 lore raw

PATCH 1/1 Trivial indentation fix in i2c/chips/Kconfig

Hi Greg,

Quoting myself:

Quoted 3 lines

(...) I also think I see an indentation issue on the "tristate" line, seemingly copied from the SENSORS_DS1621 section which would need to be fixed as well.

Here is the trivial patch fixing that, if you want to apply it.

Thanks,

drivers/i2c/chips/Kconfig renamed from drivers/i2c/chips/Kconfig.orig +1 -1
@@ -63,7 +63,7 @@
 	  will be called asb100.
 
 config SENSORS_DS1621
-      	tristate "Dallas Semiconductor DS1621 and DS1625"
+	tristate "Dallas Semiconductor DS1621 and DS1625"
 	depends on I2C && EXPERIMENTAL
 	select I2C_SENSOR
 	help
Signature · 1 line
Jean Delvare
Applied
#5 lore raw

Re: [PATCH 2.6] Trivial indentation fix in i2c/chips/Kconfig

Quoted 8 lines · Jean Delvare

Hi Greg,

Quoting myself:

Quoted 3 lines

(...) I also think I see an indentation issue on the "tristate" line, seemingly copied from the SENSORS_DS1621 section which would need to be fixed as well.

Quoted 5 lines

Here is the trivial patch fixing that, if you want to apply it.

Thanks,

Applied, thanks.

greg k-h

#6 lore raw
In reply to Jean Delvare

A revised ds1337 patch addressing all of Jean's comments is attached.

i2c: add ds1337 RTC chip support

drivers/i2c/chips/ds1337.c new file +380 -0
Index: linux-2.6.i2c/drivers/i2c/chips/ds1337.c
===================================================================
@@ -0,0 +1,380 @@
+/*
+ *  linux/drivers/i2c/chips/ds1337.c
+ *
+ *  Copyright (C) 2005 James Chapman <[email protected]>
+ *
+ *	based on linux/drivers/acron/char/pcf8583.c
+ *  Copyright (C) 2000 Russell King
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Driver for Dallas Semiconductor DS1337 real time clock chip
+ */
+
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+#include <linux/string.h>
+#include <linux/rtc.h>		/* get the user-level API */
+#include <linux/bcd.h>
+#include <linux/list.h>
+
+#define DS1337_NUM_REGS		16
+#define DS1337_REG_CONFIG	2
+
+/* FIXME - how do we export these interface constants? */
+#define DS1337_GET_DATE		0
+#define DS1337_SET_DATE		1
+
+/*
+ * Functions declaration
+ */
+static unsigned short normal_i2c[] = { 0x68, I2C_CLIENT_END };
+static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
+
+SENSORS_INSMOD_1(ds1337);
+
+static int ds1337_attach_adapter(struct i2c_adapter *adapter);
+static int ds1337_detect(struct i2c_adapter *adapter, int address, int kind);
+static void ds1337_init_client(struct i2c_client *client);
+static int ds1337_detach_client(struct i2c_client *client);
+static int ds1337_command(struct i2c_client *client, unsigned int cmd,
+			  void *arg);
+
+/*
+ * Driver data (common to all clients)
+ */
+static struct i2c_driver ds1337_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "ds1337",
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= ds1337_attach_adapter,
+	.detach_client	= ds1337_detach_client,
+	.command	= ds1337_command,
+};
+
+/*
+ * Client data (each client gets its own)
+ */
+struct ds1337_data {
+	struct i2c_client client;
+	struct list_head list;
+	int id;
+};
+
+/*
+ * Internal variables
+ */
+static int ds1337_id;
+static LIST_HEAD(ds1337_clients);
+
+static inline int ds1337_read(struct i2c_client *client, u8 reg, u8 *value)
+{
+	s32 tmp = i2c_smbus_read_byte_data(client, reg);
+
+	if (tmp < 0)
+		return -EIO;
+
+	*value = tmp;
+
+	return 0;
+}
+
+/*
+ * Chip access functions
+ */
+static int ds1337_get_datetime(struct i2c_client *client, struct rtc_time *dt)
+{
+	struct ds1337_data *data = i2c_get_clientdata(client);
+	int result;
+	u8 buf[7];
+	u8 val;
+	struct i2c_msg msg[2];
+	u8 offs = 0;
+
+	if (!dt) {
+		dev_dbg(&client->adapter->dev, "%s: EINVAL: dt=NULL\n",
+			__FUNCTION__);
+
+		return -EINVAL;
+	}
+
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = &offs;
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = sizeof(buf);
+	msg[1].buf = &buf[0];
+
+	result = client->adapter->algo->master_xfer(client->adapter,
+						    &msg[0], 2);
+
+	dev_dbg(&client->adapter->dev,
+		"%s: [%d] %02x %02x %02x %02x %02x %02x %02x\n",
+		__FUNCTION__, result, buf[0], buf[1], buf[2], buf[3],
+		buf[4], buf[5], buf[6]);
+
+	if (result >= 0) {
+		dt->tm_sec = BCD_TO_BIN(buf[0]);
+		dt->tm_min = BCD_TO_BIN(buf[1]);
+		val = buf[2] & 0x3f;
+		dt->tm_hour = BCD_TO_BIN(val);
+		dt->tm_wday = BCD_TO_BIN(buf[3]) - 1;
+		dt->tm_mday = BCD_TO_BIN(buf[4]);
+		val = buf[5] & 0x7f;
+		dt->tm_mon = BCD_TO_BIN(val);
+		dt->tm_year = 1900 + BCD_TO_BIN(buf[6]);
+		if (buf[5] & 0x80)
+			dt->tm_year += 100;
+
+		dev_dbg(&client->adapter->dev, "%s: secs=%d, mins=%d, "
+			"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
+			__FUNCTION__, dt->tm_sec, dt->tm_min,
+			dt->tm_hour, dt->tm_mday,
+			dt->tm_mon, dt->tm_year, dt->tm_wday);
+	} else {
+		dev_err(&client->adapter->dev, "ds1337[%d]: error reading "
+			"data! %d\n", data->id, result);
+		result = -EIO;
+	}
+
+	return result;
+}
+
+static int ds1337_set_datetime(struct i2c_client *client, struct rtc_time *dt)
+{
+	struct ds1337_data *data = i2c_get_clientdata(client);
+	int result;
+	u8 buf[8];
+	u8 val;
+	struct i2c_msg msg[1];
+
+	if (!dt) {
+		dev_dbg(&client->adapter->dev, "%s: EINVAL: dt=NULL\n",
+			__FUNCTION__);
+
+		return -EINVAL;
+	}
+
+	dev_dbg(&client->adapter->dev, "%s: secs=%d, mins=%d, hours=%d, "
+		"mday=%d, mon=%d, year=%d, wday=%d\n", __FUNCTION__,
+		dt->tm_sec, dt->tm_min, dt->tm_hour,
+		dt->tm_mday, dt->tm_mon, dt->tm_year, dt->tm_wday);
+
+	buf[0] = 0;		/* reg offset */
+	buf[1] = BIN_TO_BCD(dt->tm_sec);
+	buf[2] = BIN_TO_BCD(dt->tm_min);
+	buf[3] = BIN_TO_BCD(dt->tm_hour) | (1 << 6);
+	buf[4] = BIN_TO_BCD(dt->tm_wday) + 1;
+	buf[5] = BIN_TO_BCD(dt->tm_mday);
+	buf[6] = BIN_TO_BCD(dt->tm_mon);
+	if (dt->tm_year >= 2000) {
+		val = dt->tm_year - 2000;
+		buf[6] |= (1 << 7);
+	} else {
+		val = dt->tm_year - 1900;
+	}
+	buf[7] = BIN_TO_BCD(val);
+
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = sizeof(buf);
+	msg[0].buf = &buf[0];
+
+	result = client->adapter->algo->master_xfer(client->adapter,
+						    &msg[0], 1);
+	if (result < 0) {
+		dev_err(&client->adapter->dev, "ds1337[%d]: error "
+			"writing data! %d\n", data->id, result);
+		result = -EIO;
+	} else {
+		result = 0;
+	}
+
+	return result;
+}
+
+static int ds1337_command(struct i2c_client *client, unsigned int cmd,
+			  void *arg)
+{
+	dev_dbg(&client->adapter->dev, "%s: cmd=%d\n", __FUNCTION__, cmd);
+
+	switch (cmd) {
+	case DS1337_GET_DATE:
+		return ds1337_get_datetime(client, arg);
+
+	case DS1337_SET_DATE:
+		return ds1337_set_datetime(client, arg);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+/* Public API for access to specific device. Useful for low-level
+ * RTC access from kernel code.
+ */
+int ds1337_do_command(int id, int cmd, void *arg)
+{
+	struct list_head *walk;
+	struct list_head *tmp;
+	struct ds1337_data *data;
+
+	list_for_each_safe(walk, tmp, &ds1337_clients) {
+		data = list_entry(walk, struct ds1337_data, list);
+		if (data->id == id)
+			return ds1337_command(&data->client, cmd, arg);
+	}
+
+	return -ENODEV;
+}
+
+static int ds1337_attach_adapter(struct i2c_adapter *adapter)
+{
+	return i2c_detect(adapter, &addr_data, ds1337_detect);
+}
+
+/*
+ * The following function does more than just detection. If detection
+ * succeeds, it also registers the new chip.
+ */
+static int ds1337_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct i2c_client *new_client;
+	struct ds1337_data *data;
+	int err = 0;
+	const char *name = "";
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_I2C_BLOCK))
+		goto exit;
+
+	if (!(data = kmalloc(sizeof(struct ds1337_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit;
+	}
+	memset(data, 0, sizeof(struct ds1337_data));
+	INIT_LIST_HEAD(&data->list);
+
+	/* The common I2C client data is placed right before the
+	 * DS1337-specific data. 
+	 */
+	new_client = &data->client;
+	i2c_set_clientdata(new_client, data);
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &ds1337_driver;
+	new_client->flags = 0;
+
+	/*
+	 * Now we do the remaining detection. A negative kind means that
+	 * the driver was loaded with no force parameter (default), so we
+	 * must both detect and identify the chip. A zero kind means that
+	 * the driver was loaded with the force parameter, the detection
+	 * step shall be skipped. A positive kind means that the driver
+	 * was loaded with the force parameter and a given kind of chip is
+	 * requested, so both the detection and the identification steps
+	 * are skipped.
+	 */
+
+	/* Default to an DS1337 if forced */
+	if (kind == 0)
+		kind = ds1337;
+
+	if (kind < 0) {		/* detection and identification */
+
+		u8 buf[DS1337_NUM_REGS];
+		int reg;
+
+		/* Check that all DS1337 registers are present */
+		for (reg = 0; reg < DS1337_NUM_REGS; reg++)
+			if (ds1337_read(new_client, reg, &buf[reg]) < 0)
+				goto exit_free;
+
+		/* Check that control register bits 6-5 are zero */
+		if (buf[14] & 0x60)
+			goto exit_free;
+
+		/* Check that status register bits 6-2 are zero */
+		if (buf[15] & 0x7c)
+			goto exit_free;
+
+		kind = ds1337;
+	}
+
+	if (kind == ds1337)
+		name = "ds1337";
+
+	/* We can fill in the remaining client fields */
+	strlcpy(new_client->name, name, I2C_NAME_SIZE);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(new_client)))
+		goto exit_free;
+
+	/* Initialize the DS1337 chip */
+	ds1337_init_client(new_client);
+
+	/* Add client to local list */
+	data->id = ds1337_id++;
+	list_add(&data->list, &ds1337_clients);
+
+	return 0;
+
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static void ds1337_init_client(struct i2c_client *client)
+{
+	s32 val;
+
+	/* Ensure that device is set in 24-hour mode */
+	val = i2c_smbus_read_byte_data(client, DS1337_REG_CONFIG);
+	if ((val >= 0) && (val & (1 << 6)) == 0)
+		i2c_smbus_write_byte_data(client, DS1337_REG_CONFIG,
+					  val | (1 << 6));
+}
+
+static int ds1337_detach_client(struct i2c_client *client)
+{
+	int err;
+	struct ds1337_data *data = i2c_get_clientdata(client);
+
+	if ((err = i2c_detach_client(client))) {
+		dev_err(&client->dev, "Client deregistration failed, "
+			"client not detached.\n");
+		return err;
+	}
+
+	list_del(&data->list);
+	kfree(data);
+	return 0;
+}
+
+static int __init ds1337_init(void)
+{
+	return i2c_add_driver(&ds1337_driver);
+}
+
+static void __exit ds1337_exit(void)
+{
+	i2c_del_driver(&ds1337_driver);
+}
+
+MODULE_AUTHOR("James Chapman <[email protected]>");
+MODULE_DESCRIPTION("DS1337 RTC driver");
+MODULE_LICENSE("GPL");
+
+module_init(ds1337_init);
+module_exit(ds1337_exit);
drivers/i2c/chips/Kconfig +11 -0
Index: linux-2.6.i2c/drivers/i2c/chips/Kconfig
===================================================================
@@ -320,6 +320,17 @@
 menu "Other I2C Chip support"
 	depends on I2C
 
+config SENSORS_DS1337
+	tristate "Dallas Semiconductor DS1337 Real Time Clock"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Dallas Semiconductor
+	  DS1337 real-time clock chips. 
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ds1337.
+
 config SENSORS_EEPROM
 	tristate "EEPROM reader"
 	depends on I2C && EXPERIMENTAL
drivers/i2c/chips/Makefile +1 -0
Index: linux-2.6.i2c/drivers/i2c/chips/Makefile
===================================================================
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
#7 lore raw

Hi James,

Quoted 1 line

A revised ds1337 patch addressing all of Jean's comments is attached.

Fine with me except for:

drivers/i2c/chips/ds1337.c line 257

Quoted 2 lines
drivers/i2c/chips/ds1337.c new file +2 -0
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_I2C_BLOCK))

I don't this it is correct. You are using master_xfer, not i2c_smbus_{read,write}_i2c_block_data. Adapter which declare themselves I2C_FUNC_SMBUS_I2C_BLOCK-capable may not implement master_xfer. You really need to check for I2C_FUNC_I2C.

Now I agree that the transfers you do ARE i2c block transfers, and I find it highly questionable that our implementation of i2c_smbus_read_i2c_block_data will always read 32 bytes of data from the chip. It would be much more convenient to allow I2C block reads of arbitrary length, (just like we do with I2C block writes) so that clients can use this function instead of master_xfer.

It should be a quite simple fix if I correctly remember the i2c-core code, with the only drawback that it alters the API. That being said, the only kernel user (in kernel) of this function that I could find is the eeprom driver (this can be easily explained by the fact that this function, as it is now, is essentially useless), so I wouldn't mind the risk. The net benefit would be that i2c chip drivers could start using this function instead of master_xfer, so they would possibly work with more than just the fully I2C-capable adapters (not that many of them, see list right below).

i2c-dev might be a problem if we go that way, because we will silently change the way I2C block reads requested from userspace are handled. Not sure it is a big issue though, because as underlined before, the function as it is now is rather useless. I doubt that anything but i2cdump uses it in userspace.

Then we would need to fix bus drivers that implement the call by themselves (as opposed to emulation), but in fact only a few of them do (i2c-amd8111 and i2c-nforce2) so that should be quickly done. [Reading the two bus drivers code...]

And it turns out that both bus drivers ALREADY honor the length requested by the caller, which is not consistent with the emulated variant of the call. Something definitely must be done.

Any thoughts anyone?

Thanks,

Signature · 1 line
Jean Delvare
#8 lore raw

Revised patch is attached.

Quoted 9 lines · Jean Delvare

Hi James,

Quoted 1 line

A revised ds1337 patch addressing all of Jean's comments is attached.

Fine with me except for:

Quoted 9 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_I2C_BLOCK))

I don't this it is correct. You are using master_xfer, not i2c_smbus_{read,write}_i2c_block_data. Adapter which declare themselves I2C_FUNC_SMBUS_I2C_BLOCK-capable may not implement master_xfer. You really need to check for I2C_FUNC_I2C.

i2c: add ds1337 RTC chip support

drivers/i2c/chips/ds1337.c new file +380 -0
Index: linux-2.6.i2c/drivers/i2c/chips/ds1337.c
===================================================================
@@ -0,0 +1,380 @@
+/*
+ *  linux/drivers/i2c/chips/ds1337.c
+ *
+ *  Copyright (C) 2005 James Chapman <[email protected]>
+ *
+ *	based on linux/drivers/acron/char/pcf8583.c
+ *  Copyright (C) 2000 Russell King
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Driver for Dallas Semiconductor DS1337 real time clock chip
+ */
+
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+#include <linux/string.h>
+#include <linux/rtc.h>		/* get the user-level API */
+#include <linux/bcd.h>
+#include <linux/list.h>
+
+#define DS1337_NUM_REGS		16
+#define DS1337_REG_CONFIG	2
+
+/* FIXME - how do we export these interface constants? */
+#define DS1337_GET_DATE		0
+#define DS1337_SET_DATE		1
+
+/*
+ * Functions declaration
+ */
+static unsigned short normal_i2c[] = { 0x68, I2C_CLIENT_END };
+static unsigned int normal_isa[] = { I2C_CLIENT_ISA_END };
+
+SENSORS_INSMOD_1(ds1337);
+
+static int ds1337_attach_adapter(struct i2c_adapter *adapter);
+static int ds1337_detect(struct i2c_adapter *adapter, int address, int kind);
+static void ds1337_init_client(struct i2c_client *client);
+static int ds1337_detach_client(struct i2c_client *client);
+static int ds1337_command(struct i2c_client *client, unsigned int cmd,
+			  void *arg);
+
+/*
+ * Driver data (common to all clients)
+ */
+static struct i2c_driver ds1337_driver = {
+	.owner		= THIS_MODULE,
+	.name		= "ds1337",
+	.flags		= I2C_DF_NOTIFY,
+	.attach_adapter	= ds1337_attach_adapter,
+	.detach_client	= ds1337_detach_client,
+	.command	= ds1337_command,
+};
+
+/*
+ * Client data (each client gets its own)
+ */
+struct ds1337_data {
+	struct i2c_client client;
+	struct list_head list;
+	int id;
+};
+
+/*
+ * Internal variables
+ */
+static int ds1337_id;
+static LIST_HEAD(ds1337_clients);
+
+static inline int ds1337_read(struct i2c_client *client, u8 reg, u8 *value)
+{
+	s32 tmp = i2c_smbus_read_byte_data(client, reg);
+
+	if (tmp < 0)
+		return -EIO;
+
+	*value = tmp;
+
+	return 0;
+}
+
+/*
+ * Chip access functions
+ */
+static int ds1337_get_datetime(struct i2c_client *client, struct rtc_time *dt)
+{
+	struct ds1337_data *data = i2c_get_clientdata(client);
+	int result;
+	u8 buf[7];
+	u8 val;
+	struct i2c_msg msg[2];
+	u8 offs = 0;
+
+	if (!dt) {
+		dev_dbg(&client->adapter->dev, "%s: EINVAL: dt=NULL\n",
+			__FUNCTION__);
+
+		return -EINVAL;
+	}
+
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = &offs;
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = sizeof(buf);
+	msg[1].buf = &buf[0];
+
+	result = client->adapter->algo->master_xfer(client->adapter,
+						    &msg[0], 2);
+
+	dev_dbg(&client->adapter->dev,
+		"%s: [%d] %02x %02x %02x %02x %02x %02x %02x\n",
+		__FUNCTION__, result, buf[0], buf[1], buf[2], buf[3],
+		buf[4], buf[5], buf[6]);
+
+	if (result >= 0) {
+		dt->tm_sec = BCD_TO_BIN(buf[0]);
+		dt->tm_min = BCD_TO_BIN(buf[1]);
+		val = buf[2] & 0x3f;
+		dt->tm_hour = BCD_TO_BIN(val);
+		dt->tm_wday = BCD_TO_BIN(buf[3]) - 1;
+		dt->tm_mday = BCD_TO_BIN(buf[4]);
+		val = buf[5] & 0x7f;
+		dt->tm_mon = BCD_TO_BIN(val);
+		dt->tm_year = 1900 + BCD_TO_BIN(buf[6]);
+		if (buf[5] & 0x80)
+			dt->tm_year += 100;
+
+		dev_dbg(&client->adapter->dev, "%s: secs=%d, mins=%d, "
+			"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
+			__FUNCTION__, dt->tm_sec, dt->tm_min,
+			dt->tm_hour, dt->tm_mday,
+			dt->tm_mon, dt->tm_year, dt->tm_wday);
+	} else {
+		dev_err(&client->adapter->dev, "ds1337[%d]: error reading "
+			"data! %d\n", data->id, result);
+		result = -EIO;
+	}
+
+	return result;
+}
+
+static int ds1337_set_datetime(struct i2c_client *client, struct rtc_time *dt)
+{
+	struct ds1337_data *data = i2c_get_clientdata(client);
+	int result;
+	u8 buf[8];
+	u8 val;
+	struct i2c_msg msg[1];
+
+	if (!dt) {
+		dev_dbg(&client->adapter->dev, "%s: EINVAL: dt=NULL\n",
+			__FUNCTION__);
+
+		return -EINVAL;
+	}
+
+	dev_dbg(&client->adapter->dev, "%s: secs=%d, mins=%d, hours=%d, "
+		"mday=%d, mon=%d, year=%d, wday=%d\n", __FUNCTION__,
+		dt->tm_sec, dt->tm_min, dt->tm_hour,
+		dt->tm_mday, dt->tm_mon, dt->tm_year, dt->tm_wday);
+
+	buf[0] = 0;		/* reg offset */
+	buf[1] = BIN_TO_BCD(dt->tm_sec);
+	buf[2] = BIN_TO_BCD(dt->tm_min);
+	buf[3] = BIN_TO_BCD(dt->tm_hour) | (1 << 6);
+	buf[4] = BIN_TO_BCD(dt->tm_wday) + 1;
+	buf[5] = BIN_TO_BCD(dt->tm_mday);
+	buf[6] = BIN_TO_BCD(dt->tm_mon);
+	if (dt->tm_year >= 2000) {
+		val = dt->tm_year - 2000;
+		buf[6] |= (1 << 7);
+	} else {
+		val = dt->tm_year - 1900;
+	}
+	buf[7] = BIN_TO_BCD(val);
+
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = sizeof(buf);
+	msg[0].buf = &buf[0];
+
+	result = client->adapter->algo->master_xfer(client->adapter,
+						    &msg[0], 1);
+	if (result < 0) {
+		dev_err(&client->adapter->dev, "ds1337[%d]: error "
+			"writing data! %d\n", data->id, result);
+		result = -EIO;
+	} else {
+		result = 0;
+	}
+
+	return result;
+}
+
+static int ds1337_command(struct i2c_client *client, unsigned int cmd,
+			  void *arg)
+{
+	dev_dbg(&client->adapter->dev, "%s: cmd=%d\n", __FUNCTION__, cmd);
+
+	switch (cmd) {
+	case DS1337_GET_DATE:
+		return ds1337_get_datetime(client, arg);
+
+	case DS1337_SET_DATE:
+		return ds1337_set_datetime(client, arg);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+/* Public API for access to specific device. Useful for low-level
+ * RTC access from kernel code.
+ */
+int ds1337_do_command(int id, int cmd, void *arg)
+{
+	struct list_head *walk;
+	struct list_head *tmp;
+	struct ds1337_data *data;
+
+	list_for_each_safe(walk, tmp, &ds1337_clients) {
+		data = list_entry(walk, struct ds1337_data, list);
+		if (data->id == id)
+			return ds1337_command(&data->client, cmd, arg);
+	}
+
+	return -ENODEV;
+}
+
+static int ds1337_attach_adapter(struct i2c_adapter *adapter)
+{
+	return i2c_detect(adapter, &addr_data, ds1337_detect);
+}
+
+/*
+ * The following function does more than just detection. If detection
+ * succeeds, it also registers the new chip.
+ */
+static int ds1337_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct i2c_client *new_client;
+	struct ds1337_data *data;
+	int err = 0;
+	const char *name = "";
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_I2C))
+		goto exit;
+
+	if (!(data = kmalloc(sizeof(struct ds1337_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit;
+	}
+	memset(data, 0, sizeof(struct ds1337_data));
+	INIT_LIST_HEAD(&data->list);
+
+	/* The common I2C client data is placed right before the
+	 * DS1337-specific data. 
+	 */
+	new_client = &data->client;
+	i2c_set_clientdata(new_client, data);
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &ds1337_driver;
+	new_client->flags = 0;
+
+	/*
+	 * Now we do the remaining detection. A negative kind means that
+	 * the driver was loaded with no force parameter (default), so we
+	 * must both detect and identify the chip. A zero kind means that
+	 * the driver was loaded with the force parameter, the detection
+	 * step shall be skipped. A positive kind means that the driver
+	 * was loaded with the force parameter and a given kind of chip is
+	 * requested, so both the detection and the identification steps
+	 * are skipped.
+	 */
+
+	/* Default to an DS1337 if forced */
+	if (kind == 0)
+		kind = ds1337;
+
+	if (kind < 0) {		/* detection and identification */
+
+		u8 buf[DS1337_NUM_REGS];
+		int reg;
+
+		/* Check that all DS1337 registers are present */
+		for (reg = 0; reg < DS1337_NUM_REGS; reg++)
+			if (ds1337_read(new_client, reg, &buf[reg]) < 0)
+				goto exit_free;
+
+		/* Check that control register bits 6-5 are zero */
+		if (buf[14] & 0x60)
+			goto exit_free;
+
+		/* Check that status register bits 6-2 are zero */
+		if (buf[15] & 0x7c)
+			goto exit_free;
+
+		kind = ds1337;
+	}
+
+	if (kind == ds1337)
+		name = "ds1337";
+
+	/* We can fill in the remaining client fields */
+	strlcpy(new_client->name, name, I2C_NAME_SIZE);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(new_client)))
+		goto exit_free;
+
+	/* Initialize the DS1337 chip */
+	ds1337_init_client(new_client);
+
+	/* Add client to local list */
+	data->id = ds1337_id++;
+	list_add(&data->list, &ds1337_clients);
+
+	return 0;
+
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static void ds1337_init_client(struct i2c_client *client)
+{
+	s32 val;
+
+	/* Ensure that device is set in 24-hour mode */
+	val = i2c_smbus_read_byte_data(client, DS1337_REG_CONFIG);
+	if ((val >= 0) && (val & (1 << 6)) == 0)
+		i2c_smbus_write_byte_data(client, DS1337_REG_CONFIG,
+					  val | (1 << 6));
+}
+
+static int ds1337_detach_client(struct i2c_client *client)
+{
+	int err;
+	struct ds1337_data *data = i2c_get_clientdata(client);
+
+	if ((err = i2c_detach_client(client))) {
+		dev_err(&client->dev, "Client deregistration failed, "
+			"client not detached.\n");
+		return err;
+	}
+
+	list_del(&data->list);
+	kfree(data);
+	return 0;
+}
+
+static int __init ds1337_init(void)
+{
+	return i2c_add_driver(&ds1337_driver);
+}
+
+static void __exit ds1337_exit(void)
+{
+	i2c_del_driver(&ds1337_driver);
+}
+
+MODULE_AUTHOR("James Chapman <[email protected]>");
+MODULE_DESCRIPTION("DS1337 RTC driver");
+MODULE_LICENSE("GPL");
+
+module_init(ds1337_init);
+module_exit(ds1337_exit);
drivers/i2c/chips/Kconfig +11 -0
Index: linux-2.6.i2c/drivers/i2c/chips/Kconfig
===================================================================
@@ -320,6 +320,17 @@
 menu "Other I2C Chip support"
 	depends on I2C
 
+config SENSORS_DS1337
+	tristate "Dallas Semiconductor DS1337 Real Time Clock"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Dallas Semiconductor
+	  DS1337 real-time clock chips. 
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ds1337.
+
 config SENSORS_EEPROM
 	tristate "EEPROM reader"
 	depends on I2C && EXPERIMENTAL
drivers/i2c/chips/Makefile +1 -0
Index: linux-2.6.i2c/drivers/i2c/chips/Makefile
===================================================================
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_FSCHER)	+= fscher.o
#9 lore raw

Hi James, all,

Quoted 1 line

Revised patch is attached.

Looks all OK to me.

Thanks,
--
Jean Delvare

#10 lore raw
In reply to James Chapman
Quoted 6 lines · James Chapman

Revised ds1337 chip driver patch.

- change all driver log messages to use dev_dbg() or dev_err()
- remove debug module parameter

Hm, doesn't seem to apply at all:

drivers/i2c/chips/Kconfig 1.78: 418 lines
patching file drivers/i2c/chips/Kconfig
drivers/i2c/chips/Makefile 1.61: 46 lines
patching file drivers/i2c/chips/Makefile
patching file drivers/i2c/chips/ds1337.c
patch: **** malformed patch at line 420:

Care to respin it again?

thanks,

greg k-h

#11 lore raw
In reply to James Chapman
Quoted 3 lines · James Chapman

Add DS1337 RTC chip driver.

drivers/i2c/chips/ds1337.c:60: `I2C_DRIVERID_DS1337' undeclared here (not in a function)

Also, there are changes in Greg's i2c tree which break your new driver:

drivers/i2c/chips/ds1337.c:60: initializer element is not constant drivers/i2c/chips/ds1337.c:60: (near initialization for ds1337_driver.id') drivers/i2c/chips/ds1337.c: In function ds1337_get_datetime':
drivers/i2c/chips/ds1337.c:155: structure has no member named id' drivers/i2c/chips/ds1337.c: In function ds1337_set_datetime':
drivers/i2c/chips/ds1337.c:206: structure has no member named id' drivers/i2c/chips/ds1337.c: In function ds1337_detect':
drivers/i2c/chips/ds1337.c:333: structure has no member named id' drivers/i2c/chips/ds1337.c:343: structure has no member named id'

#12 lore raw

PATCH 1/1 i2c chips: ds1337 RTC driver

In reply to James Chapman

I just posted a revised version. I forgot to include i2c-ids.h in my original patch. The new patch should also fix the reference to client->id.

/james

Quoted 24 lines · Andrew Morton
Quoted 2 lines · James Chapman

Add DS1337 RTC chip driver.

drivers/i2c/chips/ds1337.c:60: `I2C_DRIVERID_DS1337' undeclared here (not in a function)

Also, there are changes in Greg's i2c tree which break your new driver:

drivers/i2c/chips/ds1337.c:60: initializer element is not constant drivers/i2c/chips/ds1337.c:60: (near initialization for ds1337_driver.id') drivers/i2c/chips/ds1337.c: In function ds1337_get_datetime':
drivers/i2c/chips/ds1337.c:155: structure has no member named id' drivers/i2c/chips/ds1337.c: In function ds1337_set_datetime':
drivers/i2c/chips/ds1337.c:206: structure has no member named id' drivers/i2c/chips/ds1337.c: In function ds1337_detect':
drivers/i2c/chips/ds1337.c:333: structure has no member named id' drivers/i2c/chips/ds1337.c:343: structure has no member named id'

Signature · 3 lines
James Chapman
E-mail  : [email protected]
PGP key : http://www.katalix.com/~jchapman/pgpkey.txt