[PATCH] hmc6352: Add driver for the HMC6352 compass

[PATCH] hmc6352: Add driver for the HMC6352 compass

[Alan Cox]

From: Kalhan Trisal <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Alan Cox <alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---

 .../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


diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
new file mode 100644
index 0000000..fbedf77
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 26386a9..9e825cb 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -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
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 6ed06a1..48597df 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -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
diff --git a/drivers/misc/hmc6352.c b/drivers/misc/hmc6352.c
new file mode 100644
index 0000000..a62a03b
--- /dev/null
+++ b/drivers/misc/hmc6352.c
@@ -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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Oliver Neukum]

Am Mittwoch, 16. Juni 2010 14:16:14 schrieb Alan Cox:
> +       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

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Jean Delvare]

Hi Alan,

On Wed, 16 Jun 2010 13:16:14 +0100, Alan Cox wrote:
> From: Kalhan Trisal <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 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:

> Signed-off-by: Kalhan Trisal <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Alan Cox <alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
> 
>  .../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
> 
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
> new file mode 100644
> index 0000000..fbedf77
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
> @@ -0,0 +1,21 @@
> +Where:		/sys/bus/i2c/devices/.../heading
> +Date:		April 2010
> +Kernel Version: 2.6.36?
> +Contact:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +Description:	Sets the calibration on or off (1 = on, 0 = off). See the
> +		chip data sheet.
> +
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9e825cb 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -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
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..48597df 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -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
> diff --git a/drivers/misc/hmc6352.c b/drivers/misc/hmc6352.c
> new file mode 100644
> index 0000000..a62a03b
> --- /dev/null
> +++ b/drivers/misc/hmc6352.c
> @@ -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 },
> +	};

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.)

> +
> +	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;
> +		}

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

> +	} 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;
> +	}

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?

> +	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 },
> +	};

Same comment as in compass_calibration_store().

> +
> +	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 = {

Could be const.

> +	.name = "hmc6352",
> +	.attrs = mid_att_compass
> +};
> +
> +static int  hmc6352_probe(struct i2c_client *client,

Double space.

> +					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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org");
> +MODULE_DESCRIPTION("hmc6352 Compass Driver");
> +MODULE_LICENSE("GPL v2");

-- 
Jean Delvare

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Jonathan Cameron]

On 06/16/10 13:16, Alan Cox wrote:
> From: Kalhan Trisal <kalhan.trisal@intel.com>
> 
> 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

> Signed-off-by: Kalhan Trisal <kalhan.trisal@intel.com>
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
> 
>  .../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
> 
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
> new file mode 100644
> index 0000000..fbedf77
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
> @@ -0,0 +1,21 @@
> +Where:		/sys/bus/i2c/devices/.../heading
> +Date:		April 2010
> +Kernel Version: 2.6.36?
> +Contact:	alan.cox@intel.com
> +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:	alan.cox@intel.com
> +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:	alan.cox@intel.com
> +Description:	Sets the calibration on or off (1 = on, 0 = off). See the
> +		chip data sheet.
> +
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9e825cb 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -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
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..48597df 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -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
> diff --git a/drivers/misc/hmc6352.c b/drivers/misc/hmc6352.c
> new file mode 100644
> index 0000000..a62a03b
> --- /dev/null
> +++ b/drivers/misc/hmc6352.c
> @@ -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 <kalhan.trisal@intel.com");
> +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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Alan Cox]

On Wed, 16 Jun 2010 14:43:45 +0200
Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:

> Am Mittwoch, 16. Juni 2010 14:16:14 schrieb Alan Cox:
> > +       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.

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Alan Cox]

> So here we would have heading0_input.

Makes complete sense

> 
> 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

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Alan Cox]

> > +	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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Alan Cox <alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---

 .../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


diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
new file mode 100644
index 0000000..feb2e4a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading0_input
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 26386a9..9e825cb 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -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
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 6ed06a1..48597df 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -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
diff --git a/drivers/misc/hmc6352.c b/drivers/misc/hmc6352.c
new file mode 100644
index 0000000..3243c8c
--- /dev/null
+++ b/drivers/misc/hmc6352.c
@@ -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 */
+	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)
+{
+	i2c_del_driver(&hmc6352_driver);
+}
+
+module_init(sensor_hmc6352_init);
+module_exit(sensor_hmc6352_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Jonathan Cameron]

On 06/17/10 12:50, Alan Cox wrote:
>>> +	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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Alan Cox <alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
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/9Udqfwiw@public.gmane.org>

> ---
> 
>  .../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
> 
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
> new file mode 100644
> index 0000000..feb2e4a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
> @@ -0,0 +1,21 @@
> +Where:		/sys/bus/i2c/devices/.../heading0_input
> +Date:		April 2010
> +Kernel Version: 2.6.36?
> +Contact:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +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).


> +
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9e825cb 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -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
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..48597df 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -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
> diff --git a/drivers/misc/hmc6352.c b/drivers/misc/hmc6352.c
> new file mode 100644
> index 0000000..3243c8c
> --- /dev/null
> +++ b/drivers/misc/hmc6352.c
> @@ -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 */
> +	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...
> +
> +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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org");
> +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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Oliver Neukum]

Am Donnerstag, 17. Juni 2010 13:20:48 schrieb Alan Cox:
> On Wed, 16 Jun 2010 14:43:45 +0200
> Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
> 
> > Am Mittwoch, 16. Juni 2010 14:16:14 schrieb Alan Cox:
> > > +       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.

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

	Regards
		Oliver

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Alan Cox]

> > 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

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Alan Cox <alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---

 .../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


diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
new file mode 100644
index 0000000..feb2e4a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
@@ -0,0 +1,21 @@
+Where:		/sys/bus/i2c/devices/.../heading0_input
+Date:		April 2010
+Kernel Version: 2.6.36?
+Contact:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
+Description:	Sets the calibration on or off (1 = on, 0 = off). See the
+		chip data sheet.
+
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 26386a9..9e825cb 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -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
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 6ed06a1..48597df 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -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
diff --git a/drivers/misc/hmc6352.c b/drivers/misc/hmc6352.c
new file mode 100644
index 0000000..63d9519
--- /dev/null
+++ b/drivers/misc/hmc6352.c
@@ -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,
+		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");
+}
+
+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 */
+	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)
+{
+	i2c_del_driver(&hmc6352_driver);
+}
+
+module_init(sensor_hmc6352_init);
+module_exit(sensor_hmc6352_exit);
+
+MODULE_AUTHOR("Kalhan Trisal <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org");
+MODULE_DESCRIPTION("hmc6352 Compass Driver");
+MODULE_LICENSE("GPL v2");

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Jean Delvare]

Hi Alan,

On Thu, 17 Jun 2010 14:14:40 +0100, Alan Cox wrote:
> 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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 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 <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Alan Cox <alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

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

> ---
> 
>  .../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
> 
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352 b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
> new file mode 100644
> index 0000000..feb2e4a
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-i2c-devices-hm6352
> @@ -0,0 +1,21 @@
> +Where:		/sys/bus/i2c/devices/.../heading0_input
> +Date:		April 2010
> +Kernel Version: 2.6.36?
> +Contact:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +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:	alan.cox-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> +Description:	Sets the calibration on or off (1 = on, 0 = off). See the
> +		chip data sheet.
> +
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9e825cb 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -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
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..48597df 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -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
> diff --git a/drivers/misc/hmc6352.c b/drivers/misc/hmc6352.c
> new file mode 100644
> index 0000000..63d9519
> --- /dev/null
> +++ b/drivers/misc/hmc6352.c
> @@ -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,
> +		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.

> +
> +static ssize_t compass_heading_data_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +

Extra blank line.

> +	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)
> +{
> +	i2c_del_driver(&hmc6352_driver);
> +}
> +
> +module_init(sensor_hmc6352_init);
> +module_exit(sensor_hmc6352_exit);
> +
> +MODULE_AUTHOR("Kalhan Trisal <kalhan.trisal-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org");
> +MODULE_DESCRIPTION("hmc6352 Compass Driver");
> +MODULE_LICENSE("GPL v2");

All the rest looks OK, feel free to add:

Reviewed-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Alan Cox]

> 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 ?

Re: [PATCH] hmc6352: Add driver for the HMC6352 compass

[Jean Delvare]

On Thu, 17 Jun 2010 16:17:54 +0100, Alan Cox wrote:
> > 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.

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

No.

-- 
Jean Delvare