Skip to contentSkip to conversation

hmc6352: Add driver for the HMC6352 compass

PATCH In review Alan Cox 5 participants 14 messages Jun 16, 2010 – Jun 17, 2010

Subsystems (2)
Char and misc drivers I2C subsystem host drivers
lore
Share & tools
lore

Alan Cox

author
#0 lore raw

PATCH 1/1 hmc6352: Add driver for the HMC6352 compass

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

Cleanups requested now all folded in and a sysfs descriptio to keep Andrew happy.

Signed-off-by Kalhan Trisal <[email protected]>
Signed-off-by Alan Cox <alan-VuQAYsv1563Yd54FQh9/[email protected]>

---

 .../ABI/testing/sysfs-bus-i2c-devices-hm6352       |   21 ++
 drivers/misc/Kconfig                               |    7 +
 drivers/misc/Makefile                              |    1 
 drivers/misc/hmc6352.c                             |  199 ++++++++++++++++++++
 4 files changed, 228 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
 create mode 100644 drivers/misc/hmc6352.c
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +21 -0
new file mode 100644
index 0000000..fbedf77
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Reports the current heading from the compass as a floating
+		point value in degrees.
+
+Where:		/sys/bus/i2c/devices/.../power_state
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the power state of the device. 0 sets the device into
+		sleep mode, 1 wakes it up.
+
+Where:		/sys/bus/i2c/devices/.../calibration
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
drivers/misc/Kconfig +7 -0
index 26386a9..9e825cb 100644
@@ -304,6 +304,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 6ed06a1..48597df 100644
@@ -28,6 +28,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
drivers/misc/hmc6352.c new file +199 -0

199 lines not on this page · raw message

#1 lore raw

Am Mittwoch, 16. Juni 2010 14:16:14 schrieb Alan Cox:

drivers/misc/hmc6352.c line 94

Quoted 8 lines
drivers/misc/hmc6352.c new file +8 -0
+       msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milli seconds */
+       ret = i2c_transfer(client->adapter, msg1, 1);
+       if (ret != 1) {
+               dev_warn(dev, "i2c read data cmd failed\n");
+               return ret;
+       }
+       ret_val = i2c_data[0];
+       ret_val = ((ret_val << 8) | i2c_data[1]);

Please use the correct macro for this conversion.

Regards
Oliver

#2 lore raw
Quoted 11 lines · On Wed, 16 Jun 2010 14:43:45 +0200 Oliver Neukum <[email protected]> wrote:

Am Mittwoch, 16. Juni 2010 14:16:14 schrieb Alan Cox:

drivers/misc/hmc6352.c line 94

Quoted 8 lines
drivers/misc/hmc6352.c new file +8 -0
+       msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milli seconds */
+       ret = i2c_transfer(client->adapter, msg1, 1);
+       if (ret != 1) {
+               dev_warn(dev, "i2c read data cmd failed\n");
+               return ret;
+       }
+       ret_val = i2c_data[0];
+       ret_val = ((ret_val << 8) | i2c_data[1]);

Please use the correct macro for this conversion.

Its an array anyway - there isn't as such a 'correct macro' nor does it need to be using one.

#3 lore raw

Am Donnerstag, 17. Juni 2010 13:20:48 schrieb Alan Cox:

Quoted 13 lines
Quoted 10 lines · On Wed, 16 Jun 2010 14:43:45 +0200 Oliver Neukum <[email protected]> wrote:

Am Mittwoch, 16. Juni 2010 14:16:14 schrieb Alan Cox:

Quoted 8 lines
drivers/misc/hmc6352.c new file +8 -0
+       msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milli seconds */
+       ret = i2c_transfer(client->adapter, msg1, 1);
+       if (ret != 1) {
+               dev_warn(dev, "i2c read data cmd failed\n");
+               return ret;
+       }
+       ret_val = i2c_data[0];
+       ret_val = ((ret_val << 8) | i2c_data[1]);
Quoted 4 lines
Quoted 1 line

Please use the correct macro for this conversion.

Its an array anyway - there isn't as such a 'correct macro' nor does it need to be using one.

So why does he use this unstructured data? Do we just hope the compiler is good enough on big endian architectures?

Regards
Oliver

author
#4 lore raw
Quoted 5 lines
Quoted 2 lines

Its an array anyway - there isn't as such a 'correct macro' nor does it need to be using one.

So why does he use this unstructured data? Do we just hope the compiler is good enough on big endian architectures?

Its an array because it comes from an i2c bus which is a bytestream. There is no structure on the i2c bus and the device is defined to return a pair of bytes in this manner.

Alan

#5 lore raw
In reply to Alan Cox

Hi Alan,

Quoted 7 lines · Alan Cox

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

Cleanups requested now all folded in and a sysfs descriptio to keep Andrew happy.

Typo: description.

Quick review:

Quoted 111 lines
Signed-off-by Kalhan Trisal <[email protected]>
Signed-off-by Alan Cox <alan-VuQAYsv1563Yd54FQh9/[email protected]>

---

 .../ABI/testing/sysfs-bus-i2c-devices-hm6352       |   21 ++
 drivers/misc/Kconfig                               |    7 +
 drivers/misc/Makefile                              |    1 
 drivers/misc/hmc6352.c                             |  199 ++++++++++++++++++++
 4 files changed, 228 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
 create mode 100644 drivers/misc/hmc6352.c
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +21 -0
new file mode 100644
index 0000000..fbedf77
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Reports the current heading from the compass as a floating
+		point value in degrees.
+
+Where:		/sys/bus/i2c/devices/.../power_state
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the power state of the device. 0 sets the device into
+		sleep mode, 1 wakes it up.
+
+Where:		/sys/bus/i2c/devices/.../calibration
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
drivers/misc/Kconfig +7 -0
index 26386a9..9e825cb 100644
@@ -304,6 +304,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 6ed06a1..48597df 100644
@@ -28,6 +28,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
drivers/misc/hmc6352.c new file +35 -0
new file mode 100644
index 0000000..a62a03b
@@ -0,0 +1,199 @@
+/*
+ * hmc6352.c - Honeywell Compass Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/sysfs.h>
+
+static ssize_t compass_calibration_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);

drivers/misc/hmc6352.c line 45

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+	int ret;
+	unsigned long val;
+	char  cmd = 'C';	/* Calibrate */
+	char  cmd1 = 'E';	/* Exit calibration mode */
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, &cmd1 },
+	};

It's quite overkill IMHO to have two messages here. In the end you send only one. It would make sense if you made these messages static const, but you didn't. If you use local variables, then the following is more efficient:

	char  cmd;
	struct i2c_msg msg[] = {
		{ client->addr, 0, 1, &cmd },
	};

	if (val == 1)
		cmd = 'C';	/* Calibrate */
	else
		cmd = 'E';	/* Exit calibration mode */
	else
		return -EINVAL;

	(etc.)
Quoted 5 lines
drivers/misc/hmc6352.c new file +5 -0
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val == 1) {
+		ret = i2c_transfer(client->adapter, msg, 1);

drivers/misc/hmc6352.c line 60

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+		if (ret != 1) {
+			dev_warn(dev, "i2c calib start cmd failed\n");
+			return ret;
+		}
+	} else if (val == 2) {
+		ret = i2c_transfer(client->adapter, msg1, 1);
+		if (ret != 1) {
+			dev_warn(dev, "i2c calib stop cmd failed\n");
+			return ret;
+		}

Note that for single-message transactions, i2c_master_send() and i2c_master_recv() are easier to use than i2c_transfer().

Quoted 22 lines
drivers/misc/hmc6352.c new file +22 -0
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t compass_heading_data_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);
+	static char cmd = 'A';		/* Get Data */
+	unsigned char i2c_data[2];
+	unsigned int ret, ret_val;
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, I2C_M_RD, 2, i2c_data },
+	};
+
+	ret = i2c_transfer(client->adapter, msg, 1);

drivers/misc/hmc6352.c line 92

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+	if (ret != 1) {
+		dev_warn(dev, "i2c cmd 0x41 failed\n");
+		return ret;
+	}
+	msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milli seconds */
+	ret = i2c_transfer(client->adapter, msg1, 1);
+	if (ret != 1) {
+		dev_warn(dev, "i2c read data cmd failed\n");
+		return ret;
+	}

Don't you need some form of locking here? Multiple concurrent users can access the sysfs files. What if 2 users access the file 5 ms apart?

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+	ret_val = i2c_data[0];
+	ret_val = ((ret_val << 8) | i2c_data[1]);
+	return sprintf(buf, "%d.%d\n", ret_val/10, ret_val%10);
+}
+
+static ssize_t compass_power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);

drivers/misc/hmc6352.c line 112

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+	unsigned long val;
+	unsigned int ret;
+	static char cmd = 'S';		/* Sleep mode */
+	static char cmd1 = 'W';		/* Wake up */
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, &cmd1 },
+	};

Same comment as in compass_calibration_store().

Quoted 20 lines
drivers/misc/hmc6352.c new file +20 -0
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val == 0) {
+		ret = i2c_transfer(client->adapter, msg, 1);
+		if (ret != 1)
+			dev_warn(dev, "i2c cmd  sleep mode failed\n");
+	} else if (val == 1) {
+		ret = i2c_transfer(client->adapter, msg1, 1);
+		if (ret != 1)
+			dev_warn(dev, "i2c cmd active mode failed\n");
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static DEVICE_ATTR(heading, S_IRUGO, compass_heading_data_show, NULL);
+static DEVICE_ATTR(calibration, S_IWUSR, NULL, compass_calibration_store);

drivers/misc/hmc6352.c line 142

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+static DEVICE_ATTR(power_state, S_IWUSR, NULL, compass_power_mode_store);
+
+static struct attribute *mid_att_compass[] = {
+	&dev_attr_heading.attr,
+	&dev_attr_calibration.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_compass_gr = {

Could be const.

drivers/misc/hmc6352.c line 147

Quoted 5 lines
drivers/misc/hmc6352.c new file +5 -0
+	.name = "hmc6352",
+	.attrs = mid_att_compass
+};
+
+static int  hmc6352_probe(struct i2c_client *client,

Double space.

Quoted 52 lines
drivers/misc/hmc6352.c new file +52 -0
+					const struct i2c_device_id *id)
+{
+	int res;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_compass_gr);
+	if (res) {
+		dev_err(&client->dev, "device_create_file failed\n");
+		return res;
+	}
+	dev_info(&client->dev, "%s HMC6352 compass chip found\n",
+							client->name);
+	return 0;
+}
+
+static int hmc6352_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &m_compass_gr);
+	return 0;
+}
+
+static struct i2c_device_id hmc6352_id[] = {
+	{ "hmc6352", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, hmc6352_id);
+
+static struct i2c_driver hmc6352_driver = {
+	.driver = {
+		.name = "hmc6352",
+	},
+	.probe = hmc6352_probe,
+	.remove = hmc6352_remove,
+	.id_table = hmc6352_id,
+};
+
+static int __init sensor_hmc6352_init(void)
+{
+	return i2c_add_driver(&hmc6352_driver);
+}
+
+static void  __exit sensor_hmc6352_exit(void)
+{
+	i2c_del_driver(&hmc6352_driver);
+}
+
+module_init(sensor_hmc6352_init);
+module_exit(sensor_hmc6352_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");
Signature · 1 line
Jean Delvare
#6 lore raw
Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, &cmd1 },
+	};

It's quite overkill IMHO to have two messages here. In the end you send only one. It would make sense if you made these messages static const,

They can't be const but I cleaned it up based on your other suggestions and then jumped up and down on it a bit more.

hmc6352: Add driver for the HMC6352 compass

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

Cleanups requested now all folded in and a sysfs description to keep Andrew happy.

Signed-off-by Kalhan Trisal <[email protected]>
Signed-off-by Alan Cox <[email protected]>

---

 .../ABI/testing/sysfs-bus-i2c-devices-hm6352       |   21 +++
 drivers/misc/Kconfig                               |    7 +
 drivers/misc/Makefile                              |    1 
 drivers/misc/hmc6352.c                             |  165 ++++++++++++++++++++
 4 files changed, 194 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
 create mode 100644 drivers/misc/hmc6352.c
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +21 -0
new file mode 100644
index 0000000..feb2e4a
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading0_input
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Reports the current heading from the compass as a floating
+		point value in degrees.
+
+Where:		/sys/bus/i2c/devices/.../power_state
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the power state of the device. 0 sets the device into
+		sleep mode, 1 wakes it up.
+
+Where:		/sys/bus/i2c/devices/.../calibration
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
drivers/misc/Kconfig +7 -0
index 26386a9..9e825cb 100644
@@ -304,6 +304,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 6ed06a1..48597df 100644
@@ -28,6 +28,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
drivers/misc/hmc6352.c new file +165 -0

165 lines not on this page · raw message

Acked-by
#7 lore raw
Quoted 15 lines · Alan Cox
Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, &cmd1 },
+	};

It's quite overkill IMHO to have two messages here. In the end you send only one. It would make sense if you made these messages static const,

They can't be const but I cleaned it up based on your other suggestions and then jumped up and down on it a bit more.

hmc6352: Add driver for the HMC6352 compass

From: Kalhan Trisal <[email protected]>

Quoted 8 lines

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

Cleanups requested now all folded in and a sysfs description to keep Andrew happy.

Signed-off-by Kalhan Trisal <[email protected]>
Signed-off-by Alan Cox <alan-VuQAYsv1563Yd54FQh9/[email protected]>

Hi Alan,

I guess it is pretty unlikely anyone would ever have more than one compass and if they do they can deal with the mutex sharing if they care.

I think there is a small issue where the code and documentation don't match for the calibration attribute. The version in the docs makes more sense to me and I think it will slightly simplify the code.

Fix that and I'm happy to add:

Acked-by Jonathan Cameron <jic23-KWPb1pKIrIJaa/[email protected]>
Quoted 27 lines

---

 .../ABI/testing/sysfs-bus-i2c-devices-hm6352       |   21 +++
 drivers/misc/Kconfig                               |    7 +
 drivers/misc/Makefile                              |    1 
 drivers/misc/hmc6352.c                             |  165 ++++++++++++++++++++
 4 files changed, 194 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
 create mode 100644 drivers/misc/hmc6352.c
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +10 -0
new file mode 100644
index 0000000..feb2e4a
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading0_input
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Reports the current heading from the compass as a floating
+		point value in degrees.
+
+Where:		/sys/bus/i2c/devices/.../power_state
+Date:		April 2010
+Kernel Version: 2.6.36?

Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 line 20

Quoted 10 lines
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +10 -0
+Contact:	[email protected]
+Description:	Sets the power state of the device. 0 sets the device into
+		sleep mode, 1 wakes it up.
+
+Where:		/sys/bus/i2c/devices/.../calibration
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.

I think there is a disrepancy between what is described here and what the code does. Looks to me like values are 1 for on and 2 for off in the code (which is a little odd).

Quoted 122 lines
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +1 -0
+
drivers/misc/Kconfig +7 -0
index 26386a9..9e825cb 100644
@@ -304,6 +304,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 6ed06a1..48597df 100644
@@ -28,6 +28,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
drivers/misc/hmc6352.c new file +85 -0
new file mode 100644
index 0000000..3243c8c
@@ -0,0 +1,165 @@
+/*
+ * hmc6352.c - Honeywell Compass Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/sysfs.h>
+
+static DEFINE_MUTEX(compass_mutex);
+
+static int compass_command(struct i2c_client *c, u8 cmd)
+{
+	int ret = i2c_master_send(c, &cmd, 1);
+	if (ret < 0)
+		dev_warn(&c->dev, "command '%c' failed.\n", cmd);
+	return ret;
+}
+
+static int compass_store(struct device *dev, const char *buf, size_t count,
+			const char *map)
+{
+	struct i2c_client *c = to_i2c_client(dev);
+	int ret;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val < 0 || val >= strlen(map) || map[val] == ' ')
+		return -EINVAL;
+	mutex_lock(&compass_mutex);
+	ret = compass_command(c, map[val]);
+	mutex_unlock(&compass_mutex);
+	return ret;
+}
+
+static ssize_t compass_calibration_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	return compass_store(dev, buf, count, " CE");
+}	
+
+static ssize_t compass_power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+	return compass_store(dev, buf, count, "SW");
+}
+
+static ssize_t compass_heading_data_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned char i2c_data[2];
+	unsigned int ret;
+
+	mutex_lock(&compass_mutex);
+	ret = compass_command(client, 'A');
+	if (ret != 1) {
+		mutex_unlock(&compass_mutex);
+		return ret;
+	}
+	msleep(10); /* sending 'A' cmd we need to wait for 7-10 millisecs */

drivers/misc/hmc6352.c line 95

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+	ret = i2c_master_recv(client, i2c_data, 2);
+	mutex_unlock(&compass_mutex);
+	if (ret != 1) {
+		dev_warn(dev, "i2c read data cmd failed\n");
+		return ret;
+	}
+	ret = (i2c_data[0] << 8) | i2c_data[1];
+	return sprintf(buf, "%d.%d\n", ret/10, ret%10);
+}
+

If we are being ludicrously fussy, no point in 2 blank lines here...

Quoted 76 lines
drivers/misc/hmc6352.c new file +70 -1
+
+static DEVICE_ATTR(heading0_input, S_IRUGO, compass_heading_data_show, NULL);
+static DEVICE_ATTR(calibration, S_IWUSR, NULL, compass_calibration_store);
+static DEVICE_ATTR(power_state, S_IWUSR, NULL, compass_power_mode_store);
+
+static struct attribute *mid_att_compass[] = {
+	&dev_attr_heading0_input.attr,
+	&dev_attr_calibration.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static const struct attribute_group m_compass_gr = {
+	.name = "hmc6352",
+	.attrs = mid_att_compass
+};
+
+static int hmc6352_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_compass_gr);
+	if (res) {
+		dev_err(&client->dev, "device_create_file failed\n");
+		return res;
+	}
+	dev_info(&client->dev, "%s HMC6352 compass chip found\n",
+							client->name);
+	return 0;
+}
+
+static int hmc6352_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &m_compass_gr);
+	return 0;
+}
+
+static struct i2c_device_id hmc6352_id[] = {
+	{ "hmc6352", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, hmc6352_id);
+
+static struct i2c_driver hmc6352_driver = {
+	.driver = {
+		.name = "hmc6352",
+	},
+	.probe = hmc6352_probe,
+	.remove = hmc6352_remove,
+	.id_table = hmc6352_id,
+};
+
+static int __init sensor_hmc6352_init(void)
+{
+	return i2c_add_driver(&hmc6352_driver);
+}
+
+static void  __exit sensor_hmc6352_exit(void)
+{
+	i2c_del_driver(&hmc6352_driver);
+}
+
+module_init(sensor_hmc6352_init);
+module_exit(sensor_hmc6352_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");
 
--

To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#8 lore raw

Ok - cleaning up the sysfs API to match the docs also cleans up the code a tiny bit more so do it that way

--
hmc6352: Add driver for the HMC6352 compass

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

Alan: Cleanups requested now all folded in and a sysfs description to keep Andrew happy. The sysfs description now resembles hwmon.

Signed-off-by Kalhan Trisal <[email protected]>
Signed-off-by Alan Cox <[email protected]>

---

 .../ABI/testing/sysfs-bus-i2c-devices-hm6352       |   21 +++
 drivers/misc/Kconfig                               |    7 +
 drivers/misc/Makefile                              |    1 
 drivers/misc/hmc6352.c                             |  165 ++++++++++++++++++++
 4 files changed, 194 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
 create mode 100644 drivers/misc/hmc6352.c
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +21 -0
new file mode 100644
index 0000000..feb2e4a
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading0_input
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Reports the current heading from the compass as a floating
+		point value in degrees.
+
+Where:		/sys/bus/i2c/devices/.../power_state
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the power state of the device. 0 sets the device into
+		sleep mode, 1 wakes it up.
+
+Where:		/sys/bus/i2c/devices/.../calibration
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
drivers/misc/Kconfig +7 -0
index 26386a9..9e825cb 100644
@@ -304,6 +304,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 6ed06a1..48597df 100644
@@ -28,6 +28,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
drivers/misc/hmc6352.c new file +165 -0

165 lines not on this page · raw message

Reviewed-by
#9 lore raw

Hi Alan,

Quoted 8 lines · Alan Cox

Ok - cleaning up the sysfs API to match the docs also cleans up the code a tiny bit more so do it that way

--
hmc6352: Add driver for the HMC6352 compass

From: Kalhan Trisal <[email protected]>

Quoted 8 lines

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

Alan: Cleanups requested now all folded in and a sysfs description to keep Andrew happy. The sysfs description now resembles hwmon.

Signed-off-by Kalhan Trisal <[email protected]>
Signed-off-by Alan Cox <alan-VuQAYsv1563Yd54FQh9/[email protected]>

Code looks nicer, however I fear you introduced a bug in the process...

Quoted 133 lines

---

 .../ABI/testing/sysfs-bus-i2c-devices-hm6352       |   21 +++
 drivers/misc/Kconfig                               |    7 +
 drivers/misc/Makefile                              |    1 
 drivers/misc/hmc6352.c                             |  165 ++++++++++++++++++++
 4 files changed, 194 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
 create mode 100644 drivers/misc/hmc6352.c
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +21 -0
new file mode 100644
index 0000000..feb2e4a
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading0_input
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Reports the current heading from the compass as a floating
+		point value in degrees.
+
+Where:		/sys/bus/i2c/devices/.../power_state
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the power state of the device. 0 sets the device into
+		sleep mode, 1 wakes it up.
+
+Where:		/sys/bus/i2c/devices/.../calibration
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
drivers/misc/Kconfig +7 -0
index 26386a9..9e825cb 100644
@@ -304,6 +304,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 6ed06a1..48597df 100644
@@ -28,6 +28,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
drivers/misc/hmc6352.c new file +59 -0
new file mode 100644
index 0000000..63d9519
@@ -0,0 +1,165 @@
+/*
+ * hmc6352.c - Honeywell Compass Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/sysfs.h>
+
+static DEFINE_MUTEX(compass_mutex);
+
+static int compass_command(struct i2c_client *c, u8 cmd)
+{
+	int ret = i2c_master_send(c, &cmd, 1);
+	if (ret < 0)
+		dev_warn(&c->dev, "command '%c' failed.\n", cmd);
+	return ret;
+}
+
+static int compass_store(struct device *dev, const char *buf, size_t count,
+			const char *map)
+{
+	struct i2c_client *c = to_i2c_client(dev);
+	int ret;
+	unsigned long val;
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val >= strlen(map))
+		return -EINVAL;
+	mutex_lock(&compass_mutex);
+	ret = compass_command(c, map[val]);
+	mutex_unlock(&compass_mutex);
+	return ret;
+}
+
+static ssize_t compass_calibration_store(struct device *dev,

drivers/misc/hmc6352.c line 69

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	return compass_store(dev, buf, count, "EC");
+}	
+
+static ssize_t compass_power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+	return compass_store(dev, buf, count, "SW");
+}

These functions are supposed to return count on success. Instead, you are returning 1.

drivers/misc/hmc6352.c line 74

Quoted 5 lines
drivers/misc/hmc6352.c new file +5 -0
+
+static ssize_t compass_heading_data_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+

Extra blank line.

Quoted 81 lines
drivers/misc/hmc6352.c new file +81 -0
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned char i2c_data[2];
+	unsigned int ret;
+
+	mutex_lock(&compass_mutex);
+	ret = compass_command(client, 'A');
+	if (ret != 1) {
+		mutex_unlock(&compass_mutex);
+		return ret;
+	}
+	msleep(10); /* sending 'A' cmd we need to wait for 7-10 millisecs */
+	ret = i2c_master_recv(client, i2c_data, 2);
+	mutex_unlock(&compass_mutex);
+	if (ret != 1) {
+		dev_warn(dev, "i2c read data cmd failed\n");
+		return ret;
+	}
+	ret = (i2c_data[0] << 8) | i2c_data[1];
+	return sprintf(buf, "%d.%d\n", ret/10, ret%10);
+}
+
+
+static DEVICE_ATTR(heading0_input, S_IRUGO, compass_heading_data_show, NULL);
+static DEVICE_ATTR(calibration, S_IWUSR, NULL, compass_calibration_store);
+static DEVICE_ATTR(power_state, S_IWUSR, NULL, compass_power_mode_store);
+
+static struct attribute *mid_att_compass[] = {
+	&dev_attr_heading0_input.attr,
+	&dev_attr_calibration.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static const struct attribute_group m_compass_gr = {
+	.name = "hmc6352",
+	.attrs = mid_att_compass
+};
+
+static int hmc6352_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_compass_gr);
+	if (res) {
+		dev_err(&client->dev, "device_create_file failed\n");
+		return res;
+	}
+	dev_info(&client->dev, "%s HMC6352 compass chip found\n",
+							client->name);
+	return 0;
+}
+
+static int hmc6352_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &m_compass_gr);
+	return 0;
+}
+
+static struct i2c_device_id hmc6352_id[] = {
+	{ "hmc6352", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, hmc6352_id);
+
+static struct i2c_driver hmc6352_driver = {
+	.driver = {
+		.name = "hmc6352",
+	},
+	.probe = hmc6352_probe,
+	.remove = hmc6352_remove,
+	.id_table = hmc6352_id,
+};
+
+static int __init sensor_hmc6352_init(void)
+{
+	return i2c_add_driver(&hmc6352_driver);
+}
+
+static void  __exit sensor_hmc6352_exit(void)

drivers/misc/hmc6352.c line 165

Quoted 10 lines
drivers/misc/hmc6352.c new file +10 -0
+{
+	i2c_del_driver(&hmc6352_driver);
+}
+
+module_init(sensor_hmc6352_init);
+module_exit(sensor_hmc6352_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");

All the rest looks OK, feel free to add:

Reviewed-by Jean Delvare <[email protected]>
Signature · 1 line
Jean Delvare
#10 lore raw
Quoted 2 lines

These functions are supposed to return count on success. Instead, you are returning 1.

Ah yes - I only tested with 1 byte sized I/O.

Fixed - do you want this to go via the I2C tree ?

#11 lore raw
Quoted 4 lines · Alan Cox
Quoted 2 lines

These functions are supposed to return count on success. Instead, you are returning 1.

Ah yes - I only tested with 1 byte sized I/O.

Good.

Quoted 1 line

Fixed - do you want this to go via the I2C tree ?

No.

Signature · 1 line
Jean Delvare
#12 lore raw
In reply to Alan Cox
Quoted 7 lines · Alan Cox

From: Kalhan Trisal <[email protected]>

This driver will report the heading values in degrees to the sysfs interface. The values returned are headings . e.g. 245.6

Cleanups requested now all folded in and a sysfs descriptio to keep Andrew happy.

Hi Alan,

Others seem to have reviewed the code fairly thoroughly so the only question I want to raise is that of the attribute naming. As the only other magnetometer in tree afaik (adis16400 has one amongst numerous other things) doesn't actually overlap with any of your attributes things are pretty unconstrained. Still as people have suggested (and we have acted on wrt to IIO) I would advocate matching hwmon's naming structure where it doesn't carry a significant cost. Simple arguement being that it is the biggest sensor related subsystem and what it uses works.

So here we would have heading0_input.

As this only applies to the one attribute anyway and maintaining that if we ever sweep all these drivers up into a common system would be trivial, this doesn't really matter that much. Still if it doesn't cost anything.... (though obviously it does if you have userspace code in place!)

Thanks,

Jonathan

Quoted 281 lines
Signed-off-by Kalhan Trisal <[email protected]>
Signed-off-by Alan Cox <[email protected]>

---

 .../ABI/testing/sysfs-bus-i2c-devices-hm6352       |   21 ++
 drivers/misc/Kconfig                               |    7 +
 drivers/misc/Makefile                              |    1 
 drivers/misc/hmc6352.c                             |  199 ++++++++++++++++++++
 4 files changed, 228 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
 create mode 100644 drivers/misc/hmc6352.c
Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 new file +21 -0
new file mode 100644
index 0000000..fbedf77
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Reports the current heading from the compass as a floating
+		point value in degrees.
+
+Where:		/sys/bus/i2c/devices/.../power_state
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the power state of the device. 0 sets the device into
+		sleep mode, 1 wakes it up.
+
+Where:		/sys/bus/i2c/devices/.../calibration
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	[email protected]
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
drivers/misc/Kconfig +7 -0
index 26386a9..9e825cb 100644
@@ -304,6 +304,13 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config HMC6352
+	tristate "Honeywell HMC6352 compass"
+	depends on I2C
+	help
+	  This driver provides support for the Honeywell HMC6352 compass,
+	  providing configuration and heading data via sysfs.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
drivers/misc/Makefile +1 -0
index 6ed06a1..48597df 100644
@@ -28,6 +28,7 @@ obj-$(CONFIG_DS1682)		+= ds1682.o
 obj-$(CONFIG_TI_DAC7512)	+= ti_dac7512.o
 obj-$(CONFIG_C2PORT)		+= c2port/
 obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
+obj-$(CONFIG_HMC6352)		+= hmc6352.o
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
drivers/misc/hmc6352.c new file +199 -1
new file mode 100644
index 0000000..a62a03b
@@ -0,0 +1,199 @@
+/*
+ * hmc6352.c - Honeywell Compass Driver
+ *
+ * Copyright (C) 2009 Intel Corp
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/sysfs.h>
+
+static ssize_t compass_calibration_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+	unsigned long val;
+	char  cmd = 'C';	/* Calibrate */
+	char  cmd1 = 'E';	/* Exit calibration mode */
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, &cmd1 },
+	};
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+	if (val == 1) {
+		ret = i2c_transfer(client->adapter, msg, 1);
+		if (ret != 1) {
+			dev_warn(dev, "i2c calib start cmd failed\n");
+			return ret;
+		}
+	} else if (val == 2) {
+		ret = i2c_transfer(client->adapter, msg1, 1);
+		if (ret != 1) {
+			dev_warn(dev, "i2c calib stop cmd failed\n");
+			return ret;
+		}
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t compass_heading_data_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);
+	static char cmd = 'A';		/* Get Data */
+	unsigned char i2c_data[2];
+	unsigned int ret, ret_val;
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, I2C_M_RD, 2, i2c_data },
+	};
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_warn(dev, "i2c cmd 0x41 failed\n");
+		return ret;
+	}
+	msleep(10); /* sending 0x41 cmd we need to wait for 7-10 milli seconds */
+	ret = i2c_transfer(client->adapter, msg1, 1);
+	if (ret != 1) {
+		dev_warn(dev, "i2c read data cmd failed\n");
+		return ret;
+	}
+	ret_val = i2c_data[0];
+	ret_val = ((ret_val << 8) | i2c_data[1]);
+	return sprintf(buf, "%d.%d\n", ret_val/10, ret_val%10);
+}
+
+static ssize_t compass_power_mode_store(struct device *dev,
+		struct device_attribute *attr, const  char *buf, size_t count)
+{
+
+	struct i2c_client *client = to_i2c_client(dev);
+	unsigned long val;
+	unsigned int ret;
+	static char cmd = 'S';		/* Sleep mode */
+	static char cmd1 = 'W';		/* Wake up */
+	struct i2c_msg msg[] = {
+		{ client->addr, 0, 1, &cmd },
+	};
+	struct i2c_msg msg1[] = {
+		{ client->addr, 0, 1, &cmd1 },
+	};
+
+	if (strict_strtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val == 0) {
+		ret = i2c_transfer(client->adapter, msg, 1);
+		if (ret != 1)
+			dev_warn(dev, "i2c cmd  sleep mode failed\n");
+	} else if (val == 1) {
+		ret = i2c_transfer(client->adapter, msg1, 1);
+		if (ret != 1)
+			dev_warn(dev, "i2c cmd active mode failed\n");
+	} else
+		return -EINVAL;
+
+	return count;
+}
+
+static DEVICE_ATTR(heading, S_IRUGO, compass_heading_data_show, NULL);
+static DEVICE_ATTR(calibration, S_IWUSR, NULL, compass_calibration_store);
+static DEVICE_ATTR(power_state, S_IWUSR, NULL, compass_power_mode_store);
+
+static struct attribute *mid_att_compass[] = {
+	&dev_attr_heading.attr,
+	&dev_attr_calibration.attr,
+	&dev_attr_power_state.attr,
+	NULL
+};
+
+static struct attribute_group m_compass_gr = {
+	.name = "hmc6352",
+	.attrs = mid_att_compass
+};
+
+static int  hmc6352_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	int res;
+
+	res = sysfs_create_group(&client->dev.kobj, &m_compass_gr);
+	if (res) {
+		dev_err(&client->dev, "device_create_file failed\n");
+		return res;
+	}
+	dev_info(&client->dev, "%s HMC6352 compass chip found\n",
+							client->name);
+	return 0;
+}
+
+static int hmc6352_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &m_compass_gr);
+	return 0;
+}
+
+static struct i2c_device_id hmc6352_id[] = {
+	{ "hmc6352", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, hmc6352_id);
+
+static struct i2c_driver hmc6352_driver = {
+	.driver = {
+		.name = "hmc6352",
+	},
+	.probe = hmc6352_probe,
+	.remove = hmc6352_remove,
+	.id_table = hmc6352_id,
+};
+
+static int __init sensor_hmc6352_init(void)
+{
+	return i2c_add_driver(&hmc6352_driver);
+}
+
+static void  __exit sensor_hmc6352_exit(void)
+{
+	i2c_del_driver(&hmc6352_driver);
+}
+
+module_init(sensor_hmc6352_init);
+module_exit(sensor_hmc6352_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <[email protected]");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");
 
--

To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#13 lore raw
Quoted 1 line

So here we would have heading0_input.

Makes complete sense

Quoted 5 lines

As this only applies to the one attribute anyway and maintaining that if we ever sweep all these drivers up into a common system would be trivial, this doesn't really matter that much. Still if it doesn't cost anything.... (though obviously it does if you have userspace code in place!)

Alan