[PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver

[PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver

[Andrew-u79uwXL29TY76Z2rM5mHXA, Chew-u79uwXL29TY76Z2rM5mHXA]

From: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

This is for the Asahi Kasei's I2C compass sensor AK8975.

Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/misc/Kconfig    |    8 +
 drivers/misc/Makefile   |    1 +
 drivers/misc/akm8975.c  |  670 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/akm8975.h |   87 ++++++
 4 files changed, 766 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/akm8975.c
 create mode 100644 include/linux/akm8975.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 26386a9..9bb3d03 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -304,6 +304,14 @@ config SENSORS_TSL2550
 	  This driver can also be built as a module.  If so, the module
 	  will be called tsl2550.
 
+config SENSORS_AK8975
+	tristate "AK8975 compass support"
+	default n
+	depends on I2C
+	help
+	  If you say yes here you get support for Asahi Kasei's
+	  orientation sensor AK8975.
+
 config EP93XX_PWM
 	tristate "EP93xx PWM support"
 	depends on ARCH_EP93XX
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 6ed06a1..366791b 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
 obj-y				+= eeprom/
 obj-y				+= cb710/
 obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
+obj-$(CONFIG_SENSORS_AK8975)	+= akm8975.o
diff --git a/drivers/misc/akm8975.c b/drivers/misc/akm8975.c
new file mode 100644
index 0000000..17a096e
--- /dev/null
+++ b/drivers/misc/akm8975.c
@@ -0,0 +1,670 @@
+/* drivers/misc/akm8975.c - akm8975 compass driver
+ *
+ * Copyright (C) 2007-2008 HTC Corporation.
+ * Author: Hou-Kun Chen <houkun.chen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ */
+
+/*
+ * Revised by AKM 2009/04/02
+ * Revised by Motorola 2010/05/27
+ *
+ */
+
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/irq.h>
+#include <linux/miscdevice.h>
+#include <linux/gpio.h>
+#include <linux/uaccess.h>
+#include <linux/delay.h>
+#include <linux/input.h>
+#include <linux/workqueue.h>
+#include <linux/freezer.h>
+#include <linux/akm8975.h>
+
+#define AK8975DRV_CALL_DBG 0
+#if AK8975DRV_CALL_DBG
+#define FUNCDBG(msg)	pr_err("%s:%s\n", __func__, msg);
+#else
+#define FUNCDBG(msg)
+#endif
+
+#define AK8975DRV_DATA_DBG 0
+#define MAX_FAILURE_COUNT 10
+
+struct akm8975_data {
+	struct i2c_client *this_client;
+	struct akm8975_platform_data *pdata;
+	struct input_dev *input_dev;
+	struct work_struct work;
+	struct mutex flags_lock;
+};
+
+/*
+* Because misc devices can not carry a pointer from driver register to
+* open, we keep this global. This limits the driver to a single instance.
+*/
+struct akm8975_data *akmd_data;
+
+static DECLARE_WAIT_QUEUE_HEAD(open_wq);
+
+static atomic_t open_flag;
+
+static short m_flag;
+static short a_flag;
+static short t_flag;
+static short mv_flag;
+
+static short akmd_delay;
+
+static ssize_t akm8975_show(struct device *dev, struct device_attribute *attr,
+				 char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	return sprintf(buf, "%u\n", i2c_smbus_read_byte_data(client,
+							     AK8975_REG_CNTL));
+}
+static ssize_t akm8975_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;
+	strict_strtoul(buf, 10, &val);
+	if (val > 0xff)
+		return -EINVAL;
+	i2c_smbus_write_byte_data(client, AK8975_REG_CNTL, val);
+	return count;
+}
+static DEVICE_ATTR(akm_ms1, S_IWUSR | S_IRUGO, akm8975_show, akm8975_store);
+
+static int akm8975_i2c_rxdata(struct akm8975_data *akm, char *buf, int length)
+{
+	struct i2c_msg msgs[] = {
+		{
+			.addr = akm->this_client->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = buf,
+		},
+		{
+			.addr = akm->this_client->addr,
+			.flags = I2C_M_RD,
+			.len = length,
+			.buf = buf,
+		},
+	};
+
+	FUNCDBG("called");
+
+	if (i2c_transfer(akm->this_client->adapter, msgs, 2) < 0) {
+		pr_err("akm8975_i2c_rxdata: transfer error\n");
+		return EIO;
+	} else
+		return 0;
+}
+
+static int akm8975_i2c_txdata(struct akm8975_data *akm, char *buf, int length)
+{
+	struct i2c_msg msgs[] = {
+		{
+			.addr = akm->this_client->addr,
+			.flags = 0,
+			.len = length,
+			.buf = buf,
+		},
+	};
+
+	FUNCDBG("called");
+
+	if (i2c_transfer(akm->this_client->adapter, msgs, 1) < 0) {
+		pr_err("akm8975_i2c_txdata: transfer error\n");
+		return -EIO;
+	} else
+		return 0;
+}
+
+static void akm8975_ecs_report_value(struct akm8975_data *akm, short *rbuf)
+{
+	struct akm8975_data *data = i2c_get_clientdata(akm->this_client);
+
+	FUNCDBG("called");
+
+#if AK8975DRV_DATA_DBG
+	pr_info("akm8975_ecs_report_value: yaw = %d, pitch = %d, roll = %d\n",
+				 rbuf[0], rbuf[1], rbuf[2]);
+	pr_info("tmp = %d, m_stat= %d, g_stat=%d\n", rbuf[3], rbuf[4], rbuf[5]);
+	pr_info("Acceleration:	 x = %d LSB, y = %d LSB, z = %d LSB\n",
+				 rbuf[6], rbuf[7], rbuf[8]);
+	pr_info("Magnetic:	 x = %d LSB, y = %d LSB, z = %d LSB\n\n",
+				 rbuf[9], rbuf[10], rbuf[11]);
+#endif
+	mutex_lock(&akm->flags_lock);
+	/* Report magnetic sensor information */
+	if (m_flag) {
+		input_report_abs(data->input_dev, ABS_RX, rbuf[0]);
+		input_report_abs(data->input_dev, ABS_RY, rbuf[1]);
+		input_report_abs(data->input_dev, ABS_RZ, rbuf[2]);
+		input_report_abs(data->input_dev, ABS_RUDDER, rbuf[4]);
+	}
+
+	/* Report acceleration sensor information */
+	if (a_flag) {
+		input_report_abs(data->input_dev, ABS_X, rbuf[6]);
+		input_report_abs(data->input_dev, ABS_Y, rbuf[7]);
+		input_report_abs(data->input_dev, ABS_Z, rbuf[8]);
+		input_report_abs(data->input_dev, ABS_WHEEL, rbuf[5]);
+	}
+
+	/* Report temperature information */
+	if (t_flag)
+		input_report_abs(data->input_dev, ABS_THROTTLE, rbuf[3]);
+
+	if (mv_flag) {
+		input_report_abs(data->input_dev, ABS_HAT0X, rbuf[9]);
+		input_report_abs(data->input_dev, ABS_HAT0Y, rbuf[10]);
+		input_report_abs(data->input_dev, ABS_BRAKE, rbuf[11]);
+	}
+	mutex_unlock(&akm->flags_lock);
+
+	input_sync(data->input_dev);
+}
+
+static void akm8975_ecs_close_done(struct akm8975_data *akm)
+{
+	FUNCDBG("called");
+	mutex_lock(&akm->flags_lock);
+	m_flag = 1;
+	a_flag = 1;
+	t_flag = 1;
+	mv_flag = 1;
+	mutex_unlock(&akm->flags_lock);
+}
+
+static int akm_aot_open(struct inode *inode, struct file *file)
+{
+	int ret = -1;
+
+	FUNCDBG("called");
+	if (atomic_cmpxchg(&open_flag, 0, 1) == 0) {
+		wake_up(&open_wq);
+		ret = 0;
+	}
+
+	ret = nonseekable_open(inode, file);
+	if (ret)
+		return ret;
+
+	file->private_data = akmd_data;
+
+	return ret;
+}
+
+static int akm_aot_release(struct inode *inode, struct file *file)
+{
+	FUNCDBG("called");
+	atomic_set(&open_flag, 0);
+	wake_up(&open_wq);
+	return 0;
+}
+
+static int akm_aot_ioctl(struct inode *inode, struct file *file,
+	      unsigned int cmd, unsigned long arg)
+{
+	void __user *argp = (void __user *) arg;
+	short flag;
+	struct akm8975_data *akm = file->private_data;
+
+	FUNCDBG("called");
+
+	switch (cmd) {
+	case ECS_IOCTL_APP_SET_MFLAG:
+	case ECS_IOCTL_APP_SET_AFLAG:
+	case ECS_IOCTL_APP_SET_MVFLAG:
+		if (copy_from_user(&flag, argp, sizeof(flag)))
+			return -EFAULT;
+		if (flag < 0 || flag > 1)
+			return -EINVAL;
+		break;
+	case ECS_IOCTL_APP_SET_DELAY:
+		if (copy_from_user(&flag, argp, sizeof(flag)))
+			return -EFAULT;
+		break;
+	default:
+		break;
+	}
+
+	mutex_lock(&akm->flags_lock);
+	switch (cmd) {
+	case ECS_IOCTL_APP_SET_MFLAG:
+		m_flag = flag;
+		break;
+	case ECS_IOCTL_APP_GET_MFLAG:
+		flag = m_flag;
+		break;
+	case ECS_IOCTL_APP_SET_AFLAG:
+		a_flag = flag;
+		break;
+	case ECS_IOCTL_APP_GET_AFLAG:
+		flag = a_flag;
+		break;
+	case ECS_IOCTL_APP_SET_MVFLAG:
+		mv_flag = flag;
+		break;
+	case ECS_IOCTL_APP_GET_MVFLAG:
+		flag = mv_flag;
+		break;
+	case ECS_IOCTL_APP_SET_DELAY:
+		akmd_delay = flag;
+		break;
+	case ECS_IOCTL_APP_GET_DELAY:
+		flag = akmd_delay;
+		break;
+	default:
+		return -ENOTTY;
+	}
+	mutex_unlock(&akm->flags_lock);
+
+	switch (cmd) {
+	case ECS_IOCTL_APP_GET_MFLAG:
+	case ECS_IOCTL_APP_GET_AFLAG:
+	case ECS_IOCTL_APP_GET_MVFLAG:
+	case ECS_IOCTL_APP_GET_DELAY:
+		if (copy_to_user(argp, &flag, sizeof(flag)))
+			return -EFAULT;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int akmd_open(struct inode *inode, struct file *file)
+{
+	int err = 0;
+
+	FUNCDBG("called");
+	err = nonseekable_open(inode, file);
+	if (err)
+		return err;
+
+	file->private_data = akmd_data;
+	return 0;
+}
+
+static int akmd_release(struct inode *inode, struct file *file)
+{
+	struct akm8975_data *akm = file->private_data;
+
+	FUNCDBG("called");
+	akm8975_ecs_close_done(akm);
+	return 0;
+}
+
+static int akmd_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
+		      unsigned long arg)
+{
+	void __user *argp = (void __user *) arg;
+
+	char rwbuf[16];
+	int ret = -1;
+	int status;
+	short value[12];
+	short delay;
+	struct akm8975_data *akm = file->private_data;
+
+	FUNCDBG("called");
+
+	switch (cmd) {
+	case ECS_IOCTL_READ:
+	case ECS_IOCTL_WRITE:
+		if (copy_from_user(&rwbuf, argp, sizeof(rwbuf)))
+			return -EFAULT;
+		break;
+
+	case ECS_IOCTL_SET_YPR:
+		if (copy_from_user(&value, argp, sizeof(value)))
+			return -EFAULT;
+		break;
+
+	default:
+		break;
+	}
+
+	switch (cmd) {
+	case ECS_IOCTL_READ:
+		if (rwbuf[0] < 1)
+			return -EINVAL;
+
+		ret = akm8975_i2c_rxdata(akm, &rwbuf[1], rwbuf[0]);
+		if (ret < 0)
+			return ret;
+		break;
+
+	case ECS_IOCTL_WRITE:
+		if (rwbuf[0] < 2)
+			return -EINVAL;
+
+		ret = akm8975_i2c_txdata(akm, &rwbuf[1], rwbuf[0]);
+		if (ret < 0)
+			return ret;
+		break;
+	case ECS_IOCTL_SET_YPR:
+		akm8975_ecs_report_value(akm, value);
+		break;
+
+	case ECS_IOCTL_GET_OPEN_STATUS:
+		wait_event_interruptible(open_wq,
+					 (atomic_read(&open_flag) != 0));
+		status = atomic_read(&open_flag);
+		break;
+	case ECS_IOCTL_GET_CLOSE_STATUS:
+		wait_event_interruptible(open_wq,
+					 (atomic_read(&open_flag) == 0));
+		status = atomic_read(&open_flag);
+		break;
+
+	case ECS_IOCTL_GET_DELAY:
+		delay = akmd_delay;
+		break;
+
+	default:
+		FUNCDBG("Unknown cmd\n");
+		return -ENOTTY;
+	}
+
+	switch (cmd) {
+	case ECS_IOCTL_READ:
+		if (copy_to_user(argp, &rwbuf, sizeof(rwbuf)))
+			return -EFAULT;
+		break;
+	case ECS_IOCTL_GET_OPEN_STATUS:
+	case ECS_IOCTL_GET_CLOSE_STATUS:
+		if (copy_to_user(argp, &status, sizeof(status)))
+			return -EFAULT;
+		break;
+	case ECS_IOCTL_GET_DELAY:
+		if (copy_to_user(argp, &delay, sizeof(delay)))
+			return -EFAULT;
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+/* needed to clear the int. pin */
+static void akm_work_func(struct work_struct *work)
+{
+	struct akm8975_data *akm =
+	    container_of(work, struct akm8975_data, work);
+
+	FUNCDBG("called");
+	enable_irq(akm->this_client->irq);
+}
+
+static irqreturn_t akm8975_interrupt(int irq, void *dev_id)
+{
+	struct akm8975_data *akm = dev_id;
+	FUNCDBG("called");
+
+	disable_irq_nosync(akm->this_client->irq);
+	schedule_work(&akm->work);
+	return IRQ_HANDLED;
+}
+
+static int akm8975_power_off(struct akm8975_data *akm)
+{
+#if AK8975DRV_CALL_DBG
+	pr_info("%s\n", __func__);
+#endif
+	if (akm->pdata->power_off)
+		akm->pdata->power_off();
+
+	return 0;
+}
+
+static int akm8975_power_on(struct akm8975_data *akm)
+{
+	int err;
+
+#if AK8975DRV_CALL_DBG
+	pr_info("%s\n", __func__);
+#endif
+	if (akm->pdata->power_on) {
+		err = akm->pdata->power_on();
+		if (err < 0)
+			return err;
+	}
+	return 0;
+}
+
+static int akm8975_init_client(struct i2c_client *client)
+{
+	struct akm8975_data *data;
+	int ret;
+
+	data = i2c_get_clientdata(client);
+
+	ret = request_irq(client->irq, akm8975_interrupt, IRQF_TRIGGER_RISING,
+				"akm8975", data);
+
+	if (ret < 0) {
+		pr_err("akm8975_init_client: request irq failed\n");
+		goto err;
+	}
+
+	init_waitqueue_head(&open_wq);
+
+	mutex_lock(&data->flags_lock);
+	m_flag = 1;
+	a_flag = 1;
+	t_flag = 1;
+	mv_flag = 1;
+	mutex_unlock(&data->flags_lock);
+
+	return 0;
+err:
+	return ret;
+}
+
+static const struct file_operations akmd_fops = {
+	.owner = THIS_MODULE,
+	.open = akmd_open,
+	.release = akmd_release,
+	.ioctl = akmd_ioctl,
+};
+
+static const struct file_operations akm_aot_fops = {
+	.owner = THIS_MODULE,
+	.open = akm_aot_open,
+	.release = akm_aot_release,
+	.ioctl = akm_aot_ioctl,
+};
+
+static struct miscdevice akm_aot_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "akm8975_aot",
+	.fops = &akm_aot_fops,
+};
+
+static struct miscdevice akmd_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "akm8975_dev",
+	.fops = &akmd_fops,
+};
+
+int akm8975_probe(struct i2c_client *client,
+		  const struct i2c_device_id *devid)
+{
+	struct akm8975_data *akm;
+	int err;
+	FUNCDBG("called");
+
+	if (client->dev.platform_data == NULL) {
+		dev_err(&client->dev, "platform data is NULL. exiting.\n");
+		err = -ENODEV;
+		goto exit_platform_data_null;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(&client->dev, "platform data is NULL. exiting.\n");
+		err = -ENODEV;
+		goto exit_check_functionality_failed;
+	}
+
+	akm = kzalloc(sizeof(struct akm8975_data), GFP_KERNEL);
+	if (!akm) {
+		dev_err(&client->dev,
+			"failed to allocate memory for module data\n");
+		err = -ENOMEM;
+		goto exit_alloc_data_failed;
+	}
+
+	akm->pdata = client->dev.platform_data;
+
+	mutex_init(&akm->flags_lock);
+	INIT_WORK(&akm->work, akm_work_func);
+	i2c_set_clientdata(client, akm);
+
+	err = akm8975_power_on(akm);
+	if (err < 0)
+		goto exit_power_on_failed;
+
+	akm8975_init_client(client);
+	akm->this_client = client;
+	akmd_data = akm;
+
+	akm->input_dev = input_allocate_device();
+	if (!akm->input_dev) {
+		err = -ENOMEM;
+		dev_err(&akm->this_client->dev,
+			"input device allocate failed\n");
+		goto exit_input_dev_alloc_failed;
+	}
+
+	set_bit(EV_ABS, akm->input_dev->evbit);
+
+	/* yaw */
+	input_set_abs_params(akm->input_dev, ABS_RX, 0, 23040, 0, 0);
+	/* pitch */
+	input_set_abs_params(akm->input_dev, ABS_RY, -11520, 11520, 0, 0);
+	/* roll */
+	input_set_abs_params(akm->input_dev, ABS_RZ, -5760, 5760, 0, 0);
+	/* x-axis acceleration */
+	input_set_abs_params(akm->input_dev, ABS_X, -5760, 5760, 0, 0);
+	/* y-axis acceleration */
+	input_set_abs_params(akm->input_dev, ABS_Y, -5760, 5760, 0, 0);
+	/* z-axis acceleration */
+	input_set_abs_params(akm->input_dev, ABS_Z, -5760, 5760, 0, 0);
+	/* temparature */
+	input_set_abs_params(akm->input_dev, ABS_THROTTLE, -30, 85, 0, 0);
+	/* status of magnetic sensor */
+	input_set_abs_params(akm->input_dev, ABS_RUDDER, 0, 3, 0, 0);
+	/* status of acceleration sensor */
+	input_set_abs_params(akm->input_dev, ABS_WHEEL, 0, 3, 0, 0);
+	/* x-axis of raw magnetic vector */
+	input_set_abs_params(akm->input_dev, ABS_HAT0X, -20480, 20479, 0, 0);
+	/* y-axis of raw magnetic vector */
+	input_set_abs_params(akm->input_dev, ABS_HAT0Y, -20480, 20479, 0, 0);
+	/* z-axis of raw magnetic vector */
+	input_set_abs_params(akm->input_dev, ABS_BRAKE, -20480, 20479, 0, 0);
+
+	akm->input_dev->name = "compass";
+
+	err = input_register_device(akm->input_dev);
+	if (err) {
+		pr_err("akm8975_probe: Unable to register input device: %s\n",
+					 akm->input_dev->name);
+		goto exit_input_register_device_failed;
+	}
+
+	err = misc_register(&akmd_device);
+	if (err) {
+		pr_err("akm8975_probe: akmd_device register failed\n");
+		goto exit_misc_device_register_failed;
+	}
+
+	err = misc_register(&akm_aot_device);
+	if (err) {
+		pr_err("akm8975_probe: akm_aot_device register failed\n");
+		goto exit_misc_device_register_failed;
+	}
+
+	err = device_create_file(&client->dev, &dev_attr_akm_ms1);
+
+	return 0;
+
+exit_misc_device_register_failed:
+exit_input_register_device_failed:
+	input_free_device(akm->input_dev);
+exit_input_dev_alloc_failed:
+	akm8975_power_off(akm);
+exit_power_on_failed:
+	kfree(akm);
+exit_alloc_data_failed:
+exit_check_functionality_failed:
+exit_platform_data_null:
+	return err;
+}
+
+static int __devexit akm8975_remove(struct i2c_client *client)
+{
+	struct akm8975_data *akm = i2c_get_clientdata(client);
+	FUNCDBG("called");
+	free_irq(client->irq, NULL);
+	input_unregister_device(akm->input_dev);
+	misc_deregister(&akmd_device);
+	misc_deregister(&akm_aot_device);
+	akm8975_power_off(akm);
+	kfree(akm);
+	return 0;
+}
+
+static const struct i2c_device_id akm8975_id[] = {
+	{ "akm8975", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, akm8975_id);
+
+static struct i2c_driver akm8975_driver = {
+	.probe = akm8975_probe,
+	.remove = akm8975_remove,
+	.id_table = akm8975_id,
+	.driver = {
+		.name = "akm8975",
+	},
+};
+
+static int __init akm8975_init(void)
+{
+	pr_info("AK8975 compass driver: init\n");
+	FUNCDBG("AK8975 compass driver: init\n");
+	return i2c_add_driver(&akm8975_driver);
+}
+
+static void __exit akm8975_exit(void)
+{
+	FUNCDBG("AK8975 compass driver: exit\n");
+	i2c_del_driver(&akm8975_driver);
+}
+
+module_init(akm8975_init);
+module_exit(akm8975_exit);
+
+MODULE_AUTHOR("Hou-Kun Chen <hk_chen-TYsT/PRPW1c@public.gmane.org>");
+MODULE_DESCRIPTION("AK8975 compass driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/akm8975.h b/include/linux/akm8975.h
new file mode 100644
index 0000000..7d02120
--- /dev/null
+++ b/include/linux/akm8975.h
@@ -0,0 +1,87 @@
+/*
+ * Definitions for akm8975 compass chip.
+ */
+#ifndef AKM8975_H
+#define AKM8975_H
+
+#include <linux/ioctl.h>
+
+/*! \name AK8975 operation mode
+ \anchor AK8975_Mode
+ Defines an operation mode of the AK8975.*/
+/*! @{*/
+#define AK8975_MODE_SNG_MEASURE	0x01
+#define	AK8975_MODE_SELF_TEST	0x08
+#define	AK8975_MODE_FUSE_ACCESS	0x0F
+#define	AK8975_MODE_POWER_DOWN	0x00
+/*! @}*/
+
+#define RBUFF_SIZE		8	/* Rx buffer size */
+
+/*! \name AK8975 register address
+\anchor AK8975_REG
+Defines a register address of the AK8975.*/
+/*! @{*/
+#define AK8975_REG_WIA		0x00
+#define AK8975_REG_INFO		0x01
+#define AK8975_REG_ST1		0x02
+#define AK8975_REG_HXL		0x03
+#define AK8975_REG_HXH		0x04
+#define AK8975_REG_HYL		0x05
+#define AK8975_REG_HYH		0x06
+#define AK8975_REG_HZL		0x07
+#define AK8975_REG_HZH		0x08
+#define AK8975_REG_ST2		0x09
+#define AK8975_REG_CNTL		0x0A
+#define AK8975_REG_RSV		0x0B
+#define AK8975_REG_ASTC		0x0C
+#define AK8975_REG_TS1		0x0D
+#define AK8975_REG_TS2		0x0E
+#define AK8975_REG_I2CDIS	0x0F
+/*! @}*/
+
+/*! \name AK8975 fuse-rom address
+\anchor AK8975_FUSE
+Defines a read-only address of the fuse ROM of the AK8975.*/
+/*! @{*/
+#define AK8975_FUSE_ASAX	0x10
+#define AK8975_FUSE_ASAY	0x11
+#define AK8975_FUSE_ASAZ	0x12
+/*! @}*/
+
+#define AKMIO			0xA1
+
+/* IOCTLs for AKM library */
+#define ECS_IOCTL_WRITE                 _IOW(AKMIO, 0x02, char[5])
+#define ECS_IOCTL_READ                  _IOWR(AKMIO, 0x03, char[5])
+#define ECS_IOCTL_GETDATA               _IOR(AKMIO, 0x08, char[RBUFF_SIZE])
+#define ECS_IOCTL_SET_YPR               _IOW(AKMIO, 0x0C, short[12])
+#define ECS_IOCTL_GET_OPEN_STATUS       _IOR(AKMIO, 0x0D, int)
+#define ECS_IOCTL_GET_CLOSE_STATUS      _IOR(AKMIO, 0x0E, int)
+#define ECS_IOCTL_GET_DELAY             _IOR(AKMIO, 0x30, short)
+
+/* IOCTLs for APPs */
+#define ECS_IOCTL_APP_SET_MFLAG		_IOW(AKMIO, 0x11, short)
+#define ECS_IOCTL_APP_GET_MFLAG		_IOW(AKMIO, 0x12, short)
+#define ECS_IOCTL_APP_SET_AFLAG		_IOW(AKMIO, 0x13, short)
+#define ECS_IOCTL_APP_GET_AFLAG		_IOR(AKMIO, 0x14, short)
+#define ECS_IOCTL_APP_SET_DELAY		_IOW(AKMIO, 0x18, short)
+#define ECS_IOCTL_APP_GET_DELAY		ECS_IOCTL_GET_DELAY
+/* Set raw magnetic vector flag */
+#define ECS_IOCTL_APP_SET_MVFLAG	_IOW(AKMIO, 0x19, short)
+/* Get raw magnetic vector flag */
+#define ECS_IOCTL_APP_GET_MVFLAG	_IOR(AKMIO, 0x1A, short)
+#define ECS_IOCTL_APP_SET_TFLAG         _IOR(AKMIO, 0x15, short)
+
+
+struct akm8975_platform_data {
+	int intr;
+
+	int (*init)(void);
+	void (*exit)(void);
+	int (*power_on)(void);
+	int (*power_off)(void);
+};
+
+#endif
+
-- 
1.7.0.4


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver

[Andrew Morton]

> From: Andrew.Chew-FOgKQjlUJ6BQetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org

Your email setup is doing strange things.

On Thu, 26 Aug 2010 18:31:57 -0700 Andrew.Chew-FOgKQjlUJ6BQetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org wrote:

> From: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> This is for the Asahi Kasei's I2C compass sensor AK8975.
> 
> Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/misc/Kconfig    |    8 +
>  drivers/misc/Makefile   |    1 +
>  drivers/misc/akm8975.c  |  670 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/akm8975.h |   87 ++++++
>  4 files changed, 766 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/akm8975.c
>  create mode 100644 include/linux/akm8975.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9bb3d03 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -304,6 +304,14 @@ config SENSORS_TSL2550
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tsl2550.
>  
> +config SENSORS_AK8975
> +	tristate "AK8975 compass support"
> +	default n
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Asahi Kasei's
> +	  orientation sensor AK8975.

Should it be in drivers/hwmon?

Perhaps not, given that drivers/misc/hmc6352.c is in drivers/misc/

Did you review the userspace interface in hmc6352.c?  Is the interface
for this driver identical to that one (I hope so).

Your changelog didn't describe this driver's interface at all, and the
patch provided no documentation for the interface.  But the interface
is the most important part of the driver, because it is the one thing
we can never change.

>  config EP93XX_P
>  	tristate "EP93xx PWM support"
>  	depends on ARCH_EP93XX
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..366791b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
>  obj-y				+= eeprom/
>  obj-y				+= cb710/
>  obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
> +obj-$(CONFIG_SENSORS_AK8975)	+= akm8975.o
> diff --git a/drivers/misc/akm8975.c b/drivers/misc/akm8975.c
> new file mode 100644
> index 0000000..17a096e
> --- /dev/null
> +++ b/drivers/misc/akm8975.c
> @@ -0,0 +1,670 @@
> +/* drivers/misc/akm8975.c - akm8975 compass driver
> + *
> + * Copyright (C) 2007-2008 HTC Corporation.
> + * Author: Hou-Kun Chen <houkun.chen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

It would be nice to get this person's signed-off-by.

> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> + */
> +
> +/*
> + * Revised by AKM 2009/04/02
> + * Revised by Motorola 2010/05/27

And even person@motorola's, if possible?

> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/irq.h>
> +#include <linux/miscdevice.h>
> +#include <linux/gpio.h>
> +#include <linux/uaccess.h>
> +#include <linux/delay.h>
> +#include <linux/input.h>
> +#include <linux/workqueue.h>
> +#include <linux/freezer.h>
> +#include <linux/akm8975.h>
> +
> +#define AK8975DRV_CALL_DBG 0
> +#if AK8975DRV_CALL_DBG
> +#define FUNCDBG(msg)	pr_err("%s:%s\n", __func__, msg);
> +#else
> +#define FUNCDBG(msg)
> +#endif
> +
> +#define AK8975DRV_DATA_DBG 0
> +#define MAX_FAILURE_COUNT 10
> +
> +struct akm8975_data {
> +	struct i2c_client *this_client;
> +	struct akm8975_platform_data *pdata;
> +	struct input_dev *input_dev;
> +	struct work_struct work;
> +	struct mutex flags_lock;
> +};
> +
> +/*
> +* Because misc devices can not carry a pointer from driver register to
> +* open, we keep this global. This limits the driver to a single instance.
> +*/
> +struct akm8975_data *akmd_data;

That has changed.  See the post-2.6.35 patch

commit fa1f68db6ca7ebb6fc4487ac215bffba06c01c28
Author: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Date:   Mon May 24 14:33:10 2010 -0700

    drivers: misc: pass miscdevice pointer via file private data


> +static DECLARE_WAIT_QUEUE_HEAD(open_wq);
> +
> +static atomic_t open_flag;
> +
> +static short m_flag;
> +static short a_flag;
> +static short t_flag;
> +static short mv_flag;
> +
> +static short akmd_delay;

It would bee great to add comments describing the above globals.

The driver assumes that I will only ever have one device in the
machine.  That's surely a reasonable assumption, unless I'm making a
compass-testing workstation.  But it's a bit sad from a general driver
design point of view.

> +static ssize_t akm8975_show(struct device *dev, struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	return sprintf(buf, "%u\n", i2c_smbus_read_byte_data(client,
> +							     AK8975_REG_CNTL));
> +}

Please include a single empty line between functions.

> +static ssize_t akm8975_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;
> +	strict_strtoul(buf, 10, &val);
> +	if (val > 0xff)
> +		return -EINVAL;
> +	i2c_smbus_write_byte_data(client, AK8975_REG_CNTL, val);
> +	return count;
> +}

And please have a blank line between end-of-locals and start-of code.

The whole point of using strict_strto*() is to check the return value! 
If I try to store "42foo" into this pseudo-file, this code will send
random junk down the i2c bus.

<I'll do a patch to add __must_check to those functions>

>
> ...
>
> +static void akm8975_ecs_report_value(struct akm8975_data *akm, short *rbuf)
> +{
> +	struct akm8975_data *data = i2c_get_clientdata(akm->this_client);
> +
> +	FUNCDBG("called");
> +
> +#if AK8975DRV_DATA_DBG
> +	pr_info("akm8975_ecs_report_value: yaw = %d, pitch = %d, roll = %d\n",
> +				 rbuf[0], rbuf[1], rbuf[2]);
> +	pr_info("tmp = %d, m_stat= %d, g_stat=%d\n", rbuf[3], rbuf[4], rbuf[5]);
> +	pr_info("Acceleration:	 x = %d LSB, y = %d LSB, z = %d LSB\n",
> +				 rbuf[6], rbuf[7], rbuf[8]);
> +	pr_info("Magnetic:	 x = %d LSB, y = %d LSB, z = %d LSB\n\n",
> +				 rbuf[9], rbuf[10], rbuf[11]);
> +#endif
> +	mutex_lock(&akm->flags_lock);
> +	/* Report magnetic sensor information */
> +	if (m_flag) {
> +		input_report_abs(data->input_dev, ABS_RX, rbuf[0]);
> +		input_report_abs(data->input_dev, ABS_RY, rbuf[1]);
> +		input_report_abs(data->input_dev, ABS_RZ, rbuf[2]);
> +		input_report_abs(data->input_dev, ABS_RUDDER, rbuf[4]);
> +	}
> +
> +	/* Report acceleration sensor information */
> +	if (a_flag) {
> +		input_report_abs(data->input_dev, ABS_X, rbuf[6]);
> +		input_report_abs(data->input_dev, ABS_Y, rbuf[7]);
> +		input_report_abs(data->input_dev, ABS_Z, rbuf[8]);
> +		input_report_abs(data->input_dev, ABS_WHEEL, rbuf[5]);
> +	}
> +
> +	/* Report temperature information */
> +	if (t_flag)
> +		input_report_abs(data->input_dev, ABS_THROTTLE, rbuf[3]);
> +
> +	if (mv_flag) {
> +		input_report_abs(data->input_dev, ABS_HAT0X, rbuf[9]);
> +		input_report_abs(data->input_dev, ABS_HAT0Y, rbuf[10]);
> +		input_report_abs(data->input_dev, ABS_BRAKE, rbuf[11]);
> +	}

oh, so that's what m, a and t mean.  <scratches head over mv>

> +	mutex_unlock(&akm->flags_lock);

I keep reading this as akpm.  Scary.

> +	input_sync(data->input_dev);
> +}
> +
> +static void akm8975_ecs_close_done(struct akm8975_data *akm)
> +{
> +	FUNCDBG("called");
> +	mutex_lock(&akm->flags_lock);
> +	m_flag = 1;
> +	a_flag = 1;
> +	t_flag = 1;
> +	mv_flag = 1;
> +	mutex_unlock(&akm->flags_lock);
> +}
> +
> +static int akm_aot_open(struct inode *inode, struct file *file)
> +{
> +	int ret = -1;
> +
> +	FUNCDBG("called");
> +	if (atomic_cmpxchg(&open_flag, 0, 1) == 0) {
> +		wake_up(&open_wq);
> +		ret = 0;
> +	}

What's all this doing?

> +	ret = nonseekable_open(inode, file);
> +	if (ret)
> +		return ret;
> +
> +	file->private_data = akmd_data;
> +
> +	return ret;
> +}
> +
> +static int akm_aot_release(struct inode *inode, struct file *file)
> +{
> +	FUNCDBG("called");
> +	atomic_set(&open_flag, 0);
> +	wake_up(&open_wq);

And what's this doing and why and what's the user-visible behaviour here?

Documentation, please!  Code comments.  And user docs if it's user-visible!

> +	return 0;
> +}
> +
> +static int akm_aot_ioctl(struct inode *inode, struct file *file,
> +	      unsigned int cmd, unsigned long arg)
> +{
> +	void __user *argp = (void __user *) arg;
> +	short flag;
> +	struct akm8975_data *akm = file->private_data;
> +
> +	FUNCDBG("called");
> +
> +	switch (cmd) {
> +	case ECS_IOCTL_APP_SET_MFLAG:
> +	case ECS_IOCTL_APP_SET_AFLAG:
> +	case ECS_IOCTL_APP_SET_MVFLAG:
> +		if (copy_from_user(&flag, argp, sizeof(flag)))
> +			return -EFAULT;
> +		if (flag < 0 || flag > 1)
> +			return -EINVAL;
> +		break;
> +	case ECS_IOCTL_APP_SET_DELAY:
> +		if (copy_from_user(&flag, argp, sizeof(flag)))
> +			return -EFAULT;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	mutex_lock(&akm->flags_lock);
> +	switch (cmd) {
> +	case ECS_IOCTL_APP_SET_MFLAG:
> +		m_flag = flag;
> +		break;
> +	case ECS_IOCTL_APP_GET_MFLAG:
> +		flag = m_flag;
> +		break;
> +	case ECS_IOCTL_APP_SET_AFLAG:
> +		a_flag = flag;
> +		break;
> +	case ECS_IOCTL_APP_GET_AFLAG:
> +		flag = a_flag;
> +		break;
> +	case ECS_IOCTL_APP_SET_MVFLAG:
> +		mv_flag = flag;
> +		break;
> +	case ECS_IOCTL_APP_GET_MVFLAG:
> +		flag = mv_flag;
> +		break;
> +	case ECS_IOCTL_APP_SET_DELAY:
> +		akmd_delay = flag;
> +		break;
> +	case ECS_IOCTL_APP_GET_DELAY:
> +		flag = akmd_delay;
> +		break;
> +	default:
> +		return -ENOTTY;
> +	}
> +	mutex_unlock(&akm->flags_lock);
> +
> +	switch (cmd) {
> +	case ECS_IOCTL_APP_GET_MFLAG:
> +	case ECS_IOCTL_APP_GET_AFLAG:
> +	case ECS_IOCTL_APP_GET_MVFLAG:
> +	case ECS_IOCTL_APP_GET_DELAY:
> +		if (copy_to_user(argp, &flag, sizeof(flag)))
> +			return -EFAULT;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}

I'm suspecting that we're not getting ourselves a standardised Linux
compass driver API :( I think this is a significant problem!

>
> ...
>
> +/* needed to clear the int. pin */
> +static void akm_work_func(struct work_struct *work)
> +{
> +	struct akm8975_data *akm =
> +	    container_of(work, struct akm8975_data, work);
> +
> +	FUNCDBG("called");
> +	enable_irq(akm->this_client->irq);
> +}
> +
> +static irqreturn_t akm8975_interrupt(int irq, void *dev_id)
> +{
> +	struct akm8975_data *akm = dev_id;
> +	FUNCDBG("called");
> +
> +	disable_irq_nosync(akm->this_client->irq);
> +	schedule_work(&akm->work);
> +	return IRQ_HANDLED;
> +}

enable_irq() and disable_irq() tend to be red flags - drivers really
shouldn't be fiddling with them.

Maybe there's a good reason, but it should have been completely
described in code comments.  This reviewer is mystified, and assumes
that people who read the code later will be as well.

> +static int akm8975_power_off(struct akm8975_data *akm)
> +{
> +#if AK8975DRV_CALL_DBG
> +	pr_info("%s\n", __func__);
> +#endif
> +	if (akm->pdata->power_off)
> +		akm->pdata->power_off();
> +
> +	return 0;
> +}
> +
> +static int akm8975_power_on(struct akm8975_data *akm)
> +{
> +	int err;
> +
> +#if AK8975DRV_CALL_DBG
> +	pr_info("%s\n", __func__);
> +#endif
> +	if (akm->pdata->power_on) {
> +		err = akm->pdata->power_on();
> +		if (err < 0)
> +			return err;
> +	}
> +	return 0;
> +}

Can ->power_off and ->power_on ever be NULL?

> +static int akm8975_init_client(struct i2c_client *client)
> +{
> +	struct akm8975_data *data;
> +	int ret;
> +
> +	data = i2c_get_clientdata(client);
> +
> +	ret = request_irq(client->irq, akm8975_interrupt, IRQF_TRIGGER_RISING,
> +				"akm8975", data);
> +
> +	if (ret < 0) {
> +		pr_err("akm8975_init_client: request irq failed\n");
> +		goto err;
> +	}
> +
> +	init_waitqueue_head(&open_wq);
> +
> +	mutex_lock(&data->flags_lock);
> +	m_flag = 1;
> +	a_flag = 1;
> +	t_flag = 1;
> +	mv_flag = 1;
> +	mutex_unlock(&data->flags_lock);

This makes no sense.  We take a per-device lock to protect global
variables.

> +	return 0;
> +err:
> +	return ret;
> +}
> +
> +static const struct file_operations akmd_fops = {
> +	.owner = THIS_MODULE,
> +	.open = akmd_open,
> +	.release = akmd_release,
> +	.ioctl = akmd_ioctl,
> +};
> +
> +static const struct file_operations akm_aot_fops = {
> +	.owner = THIS_MODULE,
> +	.open = akm_aot_open,
> +	.release = akm_aot_release,
> +	.ioctl = akm_aot_ioctl,
> +};

.ioctl is deprecated.  Please use .unlocked_ioctl.

Do these ioctls need 64-bit compat handling?

ioctls are generally a pretty unpopular way of interfacing to a driver.
Usually we get around that by using a shower of sysfs files, which
isn't much better.

> +static struct miscdevice akm_aot_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "akm8975_aot",
> +	.fops = &akm_aot_fops,
> +};
> +
> +static struct miscdevice akmd_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "akm8975_dev",
> +	.fops = &akmd_fops,
> +};
> +
> +int akm8975_probe(struct i2c_client *client,
> +		  const struct i2c_device_id *devid)

static, please.

> +{
> +	struct akm8975_data *akm;
> +	int err;
> +	FUNCDBG("called");
> +
> +	if (client->dev.platform_data == NULL) {
> +		dev_err(&client->dev, "platform data is NULL. exiting.\n");
> +		err = -ENODEV;
> +		goto exit_platform_data_null;
> +	}
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev, "platform data is NULL. exiting.\n");
> +		err = -ENODEV;
> +		goto exit_check_functionality_failed;
> +	}
> +
> +	akm = kzalloc(sizeof(struct akm8975_data), GFP_KERNEL);
> +	if (!akm) {
> +		dev_err(&client->dev,
> +			"failed to allocate memory for module data\n");
> +		err = -ENOMEM;
> +		goto exit_alloc_data_failed;
> +	}
> +
> +	akm->pdata = client->dev.platform_data;
> +
> +	mutex_init(&akm->flags_lock);
> +	INIT_WORK(&akm->work, akm_work_func);
> +	i2c_set_clientdata(client, akm);
> +
> +	err = akm8975_power_on(akm);
> +	if (err < 0)
> +		goto exit_power_on_failed;
> +
> +	akm8975_init_client(client);
> +	akm->this_client = client;
> +	akmd_data = akm;
> +
> +	akm->input_dev = input_allocate_device();
> +	if (!akm->input_dev) {
> +		err = -ENOMEM;
> +		dev_err(&akm->this_client->dev,
> +			"input device allocate failed\n");
> +		goto exit_input_dev_alloc_failed;
> +	}
> +
> +	set_bit(EV_ABS, akm->input_dev->evbit);
> +
> +	/* yaw */
> +	input_set_abs_params(akm->input_dev, ABS_RX, 0, 23040, 0, 0);
> +	/* pitch */
> +	input_set_abs_params(akm->input_dev, ABS_RY, -11520, 11520, 0, 0);
> +	/* roll */
> +	input_set_abs_params(akm->input_dev, ABS_RZ, -5760, 5760, 0, 0);
> +	/* x-axis acceleration */
> +	input_set_abs_params(akm->input_dev, ABS_X, -5760, 5760, 0, 0);
> +	/* y-axis acceleration */
> +	input_set_abs_params(akm->input_dev, ABS_Y, -5760, 5760, 0, 0);
> +	/* z-axis acceleration */
> +	input_set_abs_params(akm->input_dev, ABS_Z, -5760, 5760, 0, 0);
> +	/* temparature */
> +	input_set_abs_params(akm->input_dev, ABS_THROTTLE, -30, 85, 0, 0);
> +	/* status of magnetic sensor */
> +	input_set_abs_params(akm->input_dev, ABS_RUDDER, 0, 3, 0, 0);
> +	/* status of acceleration sensor */
> +	input_set_abs_params(akm->input_dev, ABS_WHEEL, 0, 3, 0, 0);
> +	/* x-axis of raw magnetic vector */
> +	input_set_abs_params(akm->input_dev, ABS_HAT0X, -20480, 20479, 0, 0);
> +	/* y-axis of raw magnetic vector */
> +	input_set_abs_params(akm->input_dev, ABS_HAT0Y, -20480, 20479, 0, 0);
> +	/* z-axis of raw magnetic vector */
> +	input_set_abs_params(akm->input_dev, ABS_BRAKE, -20480, 20479, 0, 0);
> +
> +	akm->input_dev->name = "compass";
> +
> +	err = input_register_device(akm->input_dev);
> +	if (err) {
> +		pr_err("akm8975_probe: Unable to register input device: %s\n",
> +					 akm->input_dev->name);
> +		goto exit_input_register_device_failed;
> +	}

Seems that the driver implements an input device!?!!?  This patch
*really* needs documenting!

> +	err = misc_register(&akmd_device);
> +	if (err) {
> +		pr_err("akm8975_probe: akmd_device register failed\n");
> +		goto exit_misc_device_register_failed;
> +	}
> +
> +	err = misc_register(&akm_aot_device);
> +	if (err) {
> +		pr_err("akm8975_probe: akm_aot_device register failed\n");
> +		goto exit_misc_device_register_failed;
> +	}
> +
> +	err = device_create_file(&client->dev, &dev_attr_akm_ms1);
> +
> +	return 0;
> +
> +exit_misc_device_register_failed:
> +exit_input_register_device_failed:
> +	input_free_device(akm->input_dev);
> +exit_input_dev_alloc_failed:
> +	akm8975_power_off(akm);
> +exit_power_on_failed:
> +	kfree(akm);
> +exit_alloc_data_failed:
> +exit_check_functionality_failed:
> +exit_platform_data_null:
> +	return err;
> +}

No suspend/resume handling?

>
> ...
>

RE: [PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver

[Andrew Chew]

Yes, really really sorry about that.  I'll have to see what's up with my git send-email configuration.

-----Original Message-----
From: Andrew Morton [mailto:akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org]
Sent: Thursday, August 26, 2010 7:11 PM
To: Andrew Chew
Cc: lkml-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org; olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org
Subject: Re: [PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver


> From: Andrew.Chew-FOgKQjlUJ6BQetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org

Your email setup is doing strange things.

On Thu, 26 Aug 2010 18:31:57 -0700 Andrew.Chew-FOgKQjlUJ6BQetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org wrote:

> From: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> This is for the Asahi Kasei's I2C compass sensor AK8975.
>
> Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/misc/Kconfig    |    8 +
>  drivers/misc/Makefile   |    1 +
>  drivers/misc/akm8975.c  |  670 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/akm8975.h |   87 ++++++
>  4 files changed, 766 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/akm8975.c
>  create mode 100644 include/linux/akm8975.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9bb3d03 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -304,6 +304,14 @@ config SENSORS_TSL2550
>         This driver can also be built as a module.  If so, the module
>         will be called tsl2550.
>
> +config SENSORS_AK8975
> +     tristate "AK8975 compass support"
> +     default n
> +     depends on I2C
> +     help
> +       If you say yes here you get support for Asahi Kasei's
> +       orientation sensor AK8975.

Should it be in drivers/hwmon?

Perhaps not, given that drivers/misc/hmc6352.c is in drivers/misc/

Did you review the userspace interface in hmc6352.c?  Is the interface
for this driver identical to that one (I hope so).

Your changelog didn't describe this driver's interface at all, and the
patch provided no documentation for the interface.  But the interface
is the most important part of the driver, because it is the one thing
we can never change.

>  config EP93XX_P
>       tristate "EP93xx PWM support"
>       depends on ARCH_EP93XX
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..366791b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
>  obj-y                                += eeprom/
>  obj-y                                += cb710/
>  obj-$(CONFIG_VMWARE_BALLOON) += vmware_balloon.o
> +obj-$(CONFIG_SENSORS_AK8975) += akm8975.o
> diff --git a/drivers/misc/akm8975.c b/drivers/misc/akm8975.c
> new file mode 100644
> index 0000000..17a096e
> --- /dev/null
> +++ b/drivers/misc/akm8975.c
> @@ -0,0 +1,670 @@
> +/* drivers/misc/akm8975.c - akm8975 compass driver
> + *
> + * Copyright (C) 2007-2008 HTC Corporation.
> + * Author: Hou-Kun Chen <houkun.chen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

It would be nice to get this person's signed-off-by.

> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> + */
> +
> +/*
> + * Revised by AKM 2009/04/02
> + * Revised by Motorola 2010/05/27

And even person@motorola's, if possible?

> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/irq.h>
> +#include <linux/miscdevice.h>
> +#include <linux/gpio.h>
> +#include <linux/uaccess.h>
> +#include <linux/delay.h>
> +#include <linux/input.h>
> +#include <linux/workqueue.h>
> +#include <linux/freezer.h>
> +#include <linux/akm8975.h>
> +
> +#define AK8975DRV_CALL_DBG 0
> +#if AK8975DRV_CALL_DBG
> +#define FUNCDBG(msg) pr_err("%s:%s\n", __func__, msg);
> +#else
> +#define FUNCDBG(msg)
> +#endif
> +
> +#define AK8975DRV_DATA_DBG 0
> +#define MAX_FAILURE_COUNT 10
> +
> +struct akm8975_data {
> +     struct i2c_client *this_client;
> +     struct akm8975_platform_data *pdata;
> +     struct input_dev *input_dev;
> +     struct work_struct work;
> +     struct mutex flags_lock;
> +};
> +
> +/*
> +* Because misc devices can not carry a pointer from driver register to
> +* open, we keep this global. This limits the driver to a single instance.
> +*/
> +struct akm8975_data *akmd_data;

That has changed.  See the post-2.6.35 patch

commit fa1f68db6ca7ebb6fc4487ac215bffba06c01c28
Author: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Date:   Mon May 24 14:33:10 2010 -0700

    drivers: misc: pass miscdevice pointer via file private data


> +static DECLARE_WAIT_QUEUE_HEAD(open_wq);
> +
> +static atomic_t open_flag;
> +
> +static short m_flag;
> +static short a_flag;
> +static short t_flag;
> +static short mv_flag;
> +
> +static short akmd_delay;

It would bee great to add comments describing the above globals.

The driver assumes that I will only ever have one device in the
machine.  That's surely a reasonable assumption, unless I'm making a
compass-testing workstation.  But it's a bit sad from a general driver
design point of view.

> +static ssize_t akm8975_show(struct device *dev, struct device_attribute *attr,
> +                              char *buf)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     return sprintf(buf, "%u\n", i2c_smbus_read_byte_data(client,
> +                                                          AK8975_REG_CNTL));
> +}

Please include a single empty line between functions.

> +static ssize_t akm8975_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;
> +     strict_strtoul(buf, 10, &val);
> +     if (val > 0xff)
> +             return -EINVAL;
> +     i2c_smbus_write_byte_data(client, AK8975_REG_CNTL, val);
> +     return count;
> +}

And please have a blank line between end-of-locals and start-of code.

The whole point of using strict_strto*() is to check the return value!
If I try to store "42foo" into this pseudo-file, this code will send
random junk down the i2c bus.

<I'll do a patch to add __must_check to those functions>

>
> ...
>
> +static void akm8975_ecs_report_value(struct akm8975_data *akm, short *rbuf)
> +{
> +     struct akm8975_data *data = i2c_get_clientdata(akm->this_client);
> +
> +     FUNCDBG("called");
> +
> +#if AK8975DRV_DATA_DBG
> +     pr_info("akm8975_ecs_report_value: yaw = %d, pitch = %d, roll = %d\n",
> +                              rbuf[0], rbuf[1], rbuf[2]);
> +     pr_info("tmp = %d, m_stat= %d, g_stat=%d\n", rbuf[3], rbuf[4], rbuf[5]);
> +     pr_info("Acceleration:   x = %d LSB, y = %d LSB, z = %d LSB\n",
> +                              rbuf[6], rbuf[7], rbuf[8]);
> +     pr_info("Magnetic:       x = %d LSB, y = %d LSB, z = %d LSB\n\n",
> +                              rbuf[9], rbuf[10], rbuf[11]);
> +#endif
> +     mutex_lock(&akm->flags_lock);
> +     /* Report magnetic sensor information */
> +     if (m_flag) {
> +             input_report_abs(data->input_dev, ABS_RX, rbuf[0]);
> +             input_report_abs(data->input_dev, ABS_RY, rbuf[1]);
> +             input_report_abs(data->input_dev, ABS_RZ, rbuf[2]);
> +             input_report_abs(data->input_dev, ABS_RUDDER, rbuf[4]);
> +     }
> +
> +     /* Report acceleration sensor information */
> +     if (a_flag) {
> +             input_report_abs(data->input_dev, ABS_X, rbuf[6]);
> +             input_report_abs(data->input_dev, ABS_Y, rbuf[7]);
> +             input_report_abs(data->input_dev, ABS_Z, rbuf[8]);
> +             input_report_abs(data->input_dev, ABS_WHEEL, rbuf[5]);
> +     }
> +
> +     /* Report temperature information */
> +     if (t_flag)
> +             input_report_abs(data->input_dev, ABS_THROTTLE, rbuf[3]);
> +
> +     if (mv_flag) {
> +             input_report_abs(data->input_dev, ABS_HAT0X, rbuf[9]);
> +             input_report_abs(data->input_dev, ABS_HAT0Y, rbuf[10]);
> +             input_report_abs(data->input_dev, ABS_BRAKE, rbuf[11]);
> +     }

oh, so that's what m, a and t mean.  <scratches head over mv>

> +     mutex_unlock(&akm->flags_lock);

I keep reading this as akpm.  Scary.

> +     input_sync(data->input_dev);
> +}
> +
> +static void akm8975_ecs_close_done(struct akm8975_data *akm)
> +{
> +     FUNCDBG("called");
> +     mutex_lock(&akm->flags_lock);
> +     m_flag = 1;
> +     a_flag = 1;
> +     t_flag = 1;
> +     mv_flag = 1;
> +     mutex_unlock(&akm->flags_lock);
> +}
> +
> +static int akm_aot_open(struct inode *inode, struct file *file)
> +{
> +     int ret = -1;
> +
> +     FUNCDBG("called");
> +     if (atomic_cmpxchg(&open_flag, 0, 1) == 0) {
> +             wake_up(&open_wq);
> +             ret = 0;
> +     }

What's all this doing?

> +     ret = nonseekable_open(inode, file);
> +     if (ret)
> +             return ret;
> +
> +     file->private_data = akmd_data;
> +
> +     return ret;
> +}
> +
> +static int akm_aot_release(struct inode *inode, struct file *file)
> +{
> +     FUNCDBG("called");
> +     atomic_set(&open_flag, 0);
> +     wake_up(&open_wq);

And what's this doing and why and what's the user-visible behaviour here?

Documentation, please!  Code comments.  And user docs if it's user-visible!

> +     return 0;
> +}
> +
> +static int akm_aot_ioctl(struct inode *inode, struct file *file,
> +           unsigned int cmd, unsigned long arg)
> +{
> +     void __user *argp = (void __user *) arg;
> +     short flag;
> +     struct akm8975_data *akm = file->private_data;
> +
> +     FUNCDBG("called");
> +
> +     switch (cmd) {
> +     case ECS_IOCTL_APP_SET_MFLAG:
> +     case ECS_IOCTL_APP_SET_AFLAG:
> +     case ECS_IOCTL_APP_SET_MVFLAG:
> +             if (copy_from_user(&flag, argp, sizeof(flag)))
> +                     return -EFAULT;
> +             if (flag < 0 || flag > 1)
> +                     return -EINVAL;
> +             break;
> +     case ECS_IOCTL_APP_SET_DELAY:
> +             if (copy_from_user(&flag, argp, sizeof(flag)))
> +                     return -EFAULT;
> +             break;
> +     default:
> +             break;
> +     }
> +
> +     mutex_lock(&akm->flags_lock);
> +     switch (cmd) {
> +     case ECS_IOCTL_APP_SET_MFLAG:
> +             m_flag = flag;
> +             break;
> +     case ECS_IOCTL_APP_GET_MFLAG:
> +             flag = m_flag;
> +             break;
> +     case ECS_IOCTL_APP_SET_AFLAG:
> +             a_flag = flag;
> +             break;
> +     case ECS_IOCTL_APP_GET_AFLAG:
> +             flag = a_flag;
> +             break;
> +     case ECS_IOCTL_APP_SET_MVFLAG:
> +             mv_flag = flag;
> +             break;
> +     case ECS_IOCTL_APP_GET_MVFLAG:
> +             flag = mv_flag;
> +             break;
> +     case ECS_IOCTL_APP_SET_DELAY:
> +             akmd_delay = flag;
> +             break;
> +     case ECS_IOCTL_APP_GET_DELAY:
> +             flag = akmd_delay;
> +             break;
> +     default:
> +             return -ENOTTY;
> +     }
> +     mutex_unlock(&akm->flags_lock);
> +
> +     switch (cmd) {
> +     case ECS_IOCTL_APP_GET_MFLAG:
> +     case ECS_IOCTL_APP_GET_AFLAG:
> +     case ECS_IOCTL_APP_GET_MVFLAG:
> +     case ECS_IOCTL_APP_GET_DELAY:
> +             if (copy_to_user(argp, &flag, sizeof(flag)))
> +                     return -EFAULT;
> +             break;
> +     default:
> +             break;
> +     }
> +
> +     return 0;
> +}

I'm suspecting that we're not getting ourselves a standardised Linux
compass driver API :( I think this is a significant problem!

>
> ...
>
> +/* needed to clear the int. pin */
> +static void akm_work_func(struct work_struct *work)
> +{
> +     struct akm8975_data *akm =
> +         container_of(work, struct akm8975_data, work);
> +
> +     FUNCDBG("called");
> +     enable_irq(akm->this_client->irq);
> +}
> +
> +static irqreturn_t akm8975_interrupt(int irq, void *dev_id)
> +{
> +     struct akm8975_data *akm = dev_id;
> +     FUNCDBG("called");
> +
> +     disable_irq_nosync(akm->this_client->irq);
> +     schedule_work(&akm->work);
> +     return IRQ_HANDLED;
> +}

enable_irq() and disable_irq() tend to be red flags - drivers really
shouldn't be fiddling with them.

Maybe there's a good reason, but it should have been completely
described in code comments.  This reviewer is mystified, and assumes
that people who read the code later will be as well.

> +static int akm8975_power_off(struct akm8975_data *akm)
> +{
> +#if AK8975DRV_CALL_DBG
> +     pr_info("%s\n", __func__);
> +#endif
> +     if (akm->pdata->power_off)
> +             akm->pdata->power_off();
> +
> +     return 0;
> +}
> +
> +static int akm8975_power_on(struct akm8975_data *akm)
> +{
> +     int err;
> +
> +#if AK8975DRV_CALL_DBG
> +     pr_info("%s\n", __func__);
> +#endif
> +     if (akm->pdata->power_on) {
> +             err = akm->pdata->power_on();
> +             if (err < 0)
> +                     return err;
> +     }
> +     return 0;
> +}

Can ->power_off and ->power_on ever be NULL?

> +static int akm8975_init_client(struct i2c_client *client)
> +{
> +     struct akm8975_data *data;
> +     int ret;
> +
> +     data = i2c_get_clientdata(client);
> +
> +     ret = request_irq(client->irq, akm8975_interrupt, IRQF_TRIGGER_RISING,
> +                             "akm8975", data);
> +
> +     if (ret < 0) {
> +             pr_err("akm8975_init_client: request irq failed\n");
> +             goto err;
> +     }
> +
> +     init_waitqueue_head(&open_wq);
> +
> +     mutex_lock(&data->flags_lock);
> +     m_flag = 1;
> +     a_flag = 1;
> +     t_flag = 1;
> +     mv_flag = 1;
> +     mutex_unlock(&data->flags_lock);

This makes no sense.  We take a per-device lock to protect global
variables.

> +     return 0;
> +err:
> +     return ret;
> +}
> +
> +static const struct file_operations akmd_fops = {
> +     .owner = THIS_MODULE,
> +     .open = akmd_open,
> +     .release = akmd_release,
> +     .ioctl = akmd_ioctl,
> +};
> +
> +static const struct file_operations akm_aot_fops = {
> +     .owner = THIS_MODULE,
> +     .open = akm_aot_open,
> +     .release = akm_aot_release,
> +     .ioctl = akm_aot_ioctl,
> +};

.ioctl is deprecated.  Please use .unlocked_ioctl.

Do these ioctls need 64-bit compat handling?

ioctls are generally a pretty unpopular way of interfacing to a driver.
Usually we get around that by using a shower of sysfs files, which
isn't much better.

> +static struct miscdevice akm_aot_device = {
> +     .minor = MISC_DYNAMIC_MINOR,
> +     .name = "akm8975_aot",
> +     .fops = &akm_aot_fops,
> +};
> +
> +static struct miscdevice akmd_device = {
> +     .minor = MISC_DYNAMIC_MINOR,
> +     .name = "akm8975_dev",
> +     .fops = &akmd_fops,
> +};
> +
> +int akm8975_probe(struct i2c_client *client,
> +               const struct i2c_device_id *devid)

static, please.

> +{
> +     struct akm8975_data *akm;
> +     int err;
> +     FUNCDBG("called");
> +
> +     if (client->dev.platform_data == NULL) {
> +             dev_err(&client->dev, "platform data is NULL. exiting.\n");
> +             err = -ENODEV;
> +             goto exit_platform_data_null;
> +     }
> +
> +     if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +             dev_err(&client->dev, "platform data is NULL. exiting.\n");
> +             err = -ENODEV;
> +             goto exit_check_functionality_failed;
> +     }
> +
> +     akm = kzalloc(sizeof(struct akm8975_data), GFP_KERNEL);
> +     if (!akm) {
> +             dev_err(&client->dev,
> +                     "failed to allocate memory for module data\n");
> +             err = -ENOMEM;
> +             goto exit_alloc_data_failed;
> +     }
> +
> +     akm->pdata = client->dev.platform_data;
> +
> +     mutex_init(&akm->flags_lock);
> +     INIT_WORK(&akm->work, akm_work_func);
> +     i2c_set_clientdata(client, akm);
> +
> +     err = akm8975_power_on(akm);
> +     if (err < 0)
> +             goto exit_power_on_failed;
> +
> +     akm8975_init_client(client);
> +     akm->this_client = client;
> +     akmd_data = akm;
> +
> +     akm->input_dev = input_allocate_device();
> +     if (!akm->input_dev) {
> +             err = -ENOMEM;
> +             dev_err(&akm->this_client->dev,
> +                     "input device allocate failed\n");
> +             goto exit_input_dev_alloc_failed;
> +     }
> +
> +     set_bit(EV_ABS, akm->input_dev->evbit);
> +
> +     /* yaw */
> +     input_set_abs_params(akm->input_dev, ABS_RX, 0, 23040, 0, 0);
> +     /* pitch */
> +     input_set_abs_params(akm->input_dev, ABS_RY, -11520, 11520, 0, 0);
> +     /* roll */
> +     input_set_abs_params(akm->input_dev, ABS_RZ, -5760, 5760, 0, 0);
> +     /* x-axis acceleration */
> +     input_set_abs_params(akm->input_dev, ABS_X, -5760, 5760, 0, 0);
> +     /* y-axis acceleration */
> +     input_set_abs_params(akm->input_dev, ABS_Y, -5760, 5760, 0, 0);
> +     /* z-axis acceleration */
> +     input_set_abs_params(akm->input_dev, ABS_Z, -5760, 5760, 0, 0);
> +     /* temparature */
> +     input_set_abs_params(akm->input_dev, ABS_THROTTLE, -30, 85, 0, 0);
> +     /* status of magnetic sensor */
> +     input_set_abs_params(akm->input_dev, ABS_RUDDER, 0, 3, 0, 0);
> +     /* status of acceleration sensor */
> +     input_set_abs_params(akm->input_dev, ABS_WHEEL, 0, 3, 0, 0);
> +     /* x-axis of raw magnetic vector */
> +     input_set_abs_params(akm->input_dev, ABS_HAT0X, -20480, 20479, 0, 0);
> +     /* y-axis of raw magnetic vector */
> +     input_set_abs_params(akm->input_dev, ABS_HAT0Y, -20480, 20479, 0, 0);
> +     /* z-axis of raw magnetic vector */
> +     input_set_abs_params(akm->input_dev, ABS_BRAKE, -20480, 20479, 0, 0);
> +
> +     akm->input_dev->name = "compass";
> +
> +     err = input_register_device(akm->input_dev);
> +     if (err) {
> +             pr_err("akm8975_probe: Unable to register input device: %s\n",
> +                                      akm->input_dev->name);
> +             goto exit_input_register_device_failed;
> +     }

Seems that the driver implements an input device!?!!?  This patch
*really* needs documenting!

> +     err = misc_register(&akmd_device);
> +     if (err) {
> +             pr_err("akm8975_probe: akmd_device register failed\n");
> +             goto exit_misc_device_register_failed;
> +     }
> +
> +     err = misc_register(&akm_aot_device);
> +     if (err) {
> +             pr_err("akm8975_probe: akm_aot_device register failed\n");
> +             goto exit_misc_device_register_failed;
> +     }
> +
> +     err = device_create_file(&client->dev, &dev_attr_akm_ms1);
> +
> +     return 0;
> +
> +exit_misc_device_register_failed:
> +exit_input_register_device_failed:
> +     input_free_device(akm->input_dev);
> +exit_input_dev_alloc_failed:
> +     akm8975_power_off(akm);
> +exit_power_on_failed:
> +     kfree(akm);
> +exit_alloc_data_failed:
> +exit_check_functionality_failed:
> +exit_platform_data_null:
> +     return err;
> +}

No suspend/resume handling?

>
> ...
>

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver

[Jean Delvare]

On Thu, 26 Aug 2010 19:10:37 -0700, Andrew Morton wrote:
> > From: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > This is for the Asahi Kasei's I2C compass sensor AK8975.
> > 
> > Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/misc/Kconfig    |    8 +
> >  drivers/misc/Makefile   |    1 +
> >  drivers/misc/akm8975.c  |  670 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/akm8975.h |   87 ++++++
> >  4 files changed, 766 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/misc/akm8975.c
> >  create mode 100644 include/linux/akm8975.h
> > 
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 26386a9..9bb3d03 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -304,6 +304,14 @@ config SENSORS_TSL2550
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called tsl2550.
> >  
> > +config SENSORS_AK8975
> > +	tristate "AK8975 compass support"
> > +	default n
> > +	depends on I2C
> > +	help
> > +	  If you say yes here you get support for Asahi Kasei's
> > +	  orientation sensor AK8975.
> 
> Should it be in drivers/hwmon?
> 
> Perhaps not, given that drivers/misc/hmc6352.c is in drivers/misc/

No, this driver doesn't implement any standard hwmon attribute, so it
doesn't belong to drivers/hwmon. drivers/misc/ seems OK, until either
the IIO subsystem emerges, or someone creates a compass subsystem.

-- 
Jean Delvare

Re: [PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver

[Jean Delvare]

Hi Andrew,

Please note: lkml-u79uwXL29TY76Z2rM5mHXA@public.gmane.org doesn't exist.

Here's my review, as a complement to Andrew's.

On Thu, 26 Aug 2010 18:31:57 -0700, Andrew-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Chew-u79uwXL29TY76Z2rM5mHXA@public.gmane.org wrote:
> From: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> This is for the Asahi Kasei's I2C compass sensor AK8975.
> 
> Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/misc/Kconfig    |    8 +
>  drivers/misc/Makefile   |    1 +
>  drivers/misc/akm8975.c  |  670 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/akm8975.h |   87 ++++++
>  4 files changed, 766 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/misc/akm8975.c
>  create mode 100644 include/linux/akm8975.h
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 26386a9..9bb3d03 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -304,6 +304,14 @@ config SENSORS_TSL2550
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tsl2550.
>  
> +config SENSORS_AK8975
> +	tristate "AK8975 compass support"
> +	default n
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Asahi Kasei's
> +	  orientation sensor AK8975.
> +
>  config EP93XX_PWM
>  	tristate "EP93xx PWM support"
>  	depends on ARCH_EP93XX
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6ed06a1..366791b 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_IWMC3200TOP)      += iwmc3200top/
>  obj-y				+= eeprom/
>  obj-y				+= cb710/
>  obj-$(CONFIG_VMWARE_BALLOON)	+= vmware_balloon.o
> +obj-$(CONFIG_SENSORS_AK8975)	+= akm8975.o

CONFIG_COMPASS_AK8975 would be a better name IMHO, in anticipation of a
compass subsystem being created someday.

> diff --git a/drivers/misc/akm8975.c b/drivers/misc/akm8975.c
> new file mode 100644
> index 0000000..17a096e
> --- /dev/null
> +++ b/drivers/misc/akm8975.c
> @@ -0,0 +1,670 @@
> +/* drivers/misc/akm8975.c - akm8975 compass driver
> + *
> + * Copyright (C) 2007-2008 HTC Corporation.
> + * Author: Hou-Kun Chen <houkun.chen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> + */
> +
> +/*
> + * Revised by AKM 2009/04/02
> + * Revised by Motorola 2010/05/27
> + *
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/irq.h>

Do you really need this one?

> +#include <linux/miscdevice.h>
> +#include <linux/gpio.h>

You don't seem to need this.

> +#include <linux/uaccess.h>
> +#include <linux/delay.h>

You don't need this?

> +#include <linux/input.h>
> +#include <linux/workqueue.h>
> +#include <linux/freezer.h>

What is this one being used for?

> +#include <linux/akm8975.h>
> +
> +#define AK8975DRV_CALL_DBG 0
> +#if AK8975DRV_CALL_DBG
> +#define FUNCDBG(msg)	pr_err("%s:%s\n", __func__, msg);
> +#else
> +#define FUNCDBG(msg)
> +#endif

This tracing can probably go away once this driver is considered good
for merging.

> +
> +#define AK8975DRV_DATA_DBG 0
> +#define MAX_FAILURE_COUNT 10

Not used anywhere?

> +
> +struct akm8975_data {
> +	struct i2c_client *this_client;

"this_" makes the name longer without adding any value..

> +	struct akm8975_platform_data *pdata;
> +	struct input_dev *input_dev;
> +	struct work_struct work;
> +	struct mutex flags_lock;
> +};
> +
> +/*
> +* Because misc devices can not carry a pointer from driver register to
> +* open, we keep this global. This limits the driver to a single instance.
> +*/
> +struct akm8975_data *akmd_data;

Should be static.

> +
> +static DECLARE_WAIT_QUEUE_HEAD(open_wq);
> +
> +static atomic_t open_flag;
> +
> +static short m_flag;
> +static short a_flag;
> +static short t_flag;
> +static short mv_flag;
> +
> +static short akmd_delay;
> +
> +static ssize_t akm8975_show(struct device *dev, struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	return sprintf(buf, "%u\n", i2c_smbus_read_byte_data(client,
> +							     AK8975_REG_CNTL));

Missing error handling. i2c_smbus_read_byte_data can return an error,
which you want to pass to user-space as a proper error code, not a
random negative value.

> +}
> +static ssize_t akm8975_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;
> +	strict_strtoul(buf, 10, &val);
> +	if (val > 0xff)
> +		return -EINVAL;
> +	i2c_smbus_write_byte_data(client, AK8975_REG_CNTL, val);

Here again, error code is ignored.

> +	return count;
> +}
> +static DEVICE_ATTR(akm_ms1, S_IWUSR | S_IRUGO, akm8975_show, akm8975_store);
> +
> +static int akm8975_i2c_rxdata(struct akm8975_data *akm, char *buf, int length)
> +{
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = akm->this_client->addr,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = buf,
> +		},
> +		{
> +			.addr = akm->this_client->addr,
> +			.flags = I2C_M_RD,
> +			.len = length,
> +			.buf = buf,

Using the same buffer for writing and reading is unsupported. I think
it should work, but there's no guarantee.

> +		},
> +	};
> +
> +	FUNCDBG("called");
> +
> +	if (i2c_transfer(akm->this_client->adapter, msgs, 2) < 0) {

What's the largest data block you need to handle? If 32 or less, you
should be using i2c_smbus_read_i2c_block_data(), which is more portable.

> +		pr_err("akm8975_i2c_rxdata: transfer error\n");
> +		return EIO;

Should be -EIO, otherwise error handling won't trigger.

> +	} else
> +		return 0;
> +}
> +
> +static int akm8975_i2c_txdata(struct akm8975_data *akm, char *buf, int length)
> +{
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = akm->this_client->addr,
> +			.flags = 0,
> +			.len = length,
> +			.buf = buf,
> +		},
> +	};
> +
> +	FUNCDBG("called");
> +
> +	if (i2c_transfer(akm->this_client->adapter, msgs, 1) < 0) {

What's the largest data block you need to handle? If 32 or less, you
should be using i2c_smbus_write_i2c_block_data(), which is more
portable.

> +		pr_err("akm8975_i2c_txdata: transfer error\n");
> +		return -EIO;
> +	} else
> +		return 0;
> +}
> +
> +static void akm8975_ecs_report_value(struct akm8975_data *akm, short *rbuf)
> +{
> +	struct akm8975_data *data = i2c_get_clientdata(akm->this_client);
> +
> +	FUNCDBG("called");
> +
> +#if AK8975DRV_DATA_DBG
> +	pr_info("akm8975_ecs_report_value: yaw = %d, pitch = %d, roll = %d\n",
> +				 rbuf[0], rbuf[1], rbuf[2]);
> +	pr_info("tmp = %d, m_stat= %d, g_stat=%d\n", rbuf[3], rbuf[4], rbuf[5]);
> +	pr_info("Acceleration:	 x = %d LSB, y = %d LSB, z = %d LSB\n",
> +				 rbuf[6], rbuf[7], rbuf[8]);
> +	pr_info("Magnetic:	 x = %d LSB, y = %d LSB, z = %d LSB\n\n",
> +				 rbuf[9], rbuf[10], rbuf[11]);
> +#endif
> +	mutex_lock(&akm->flags_lock);
> +	/* Report magnetic sensor information */
> +	if (m_flag) {
> +		input_report_abs(data->input_dev, ABS_RX, rbuf[0]);
> +		input_report_abs(data->input_dev, ABS_RY, rbuf[1]);
> +		input_report_abs(data->input_dev, ABS_RZ, rbuf[2]);
> +		input_report_abs(data->input_dev, ABS_RUDDER, rbuf[4]);
> +	}
> +
> +	/* Report acceleration sensor information */
> +	if (a_flag) {
> +		input_report_abs(data->input_dev, ABS_X, rbuf[6]);
> +		input_report_abs(data->input_dev, ABS_Y, rbuf[7]);
> +		input_report_abs(data->input_dev, ABS_Z, rbuf[8]);
> +		input_report_abs(data->input_dev, ABS_WHEEL, rbuf[5]);
> +	}
> +
> +	/* Report temperature information */
> +	if (t_flag)
> +		input_report_abs(data->input_dev, ABS_THROTTLE, rbuf[3]);
> +
> +	if (mv_flag) {
> +		input_report_abs(data->input_dev, ABS_HAT0X, rbuf[9]);
> +		input_report_abs(data->input_dev, ABS_HAT0Y, rbuf[10]);
> +		input_report_abs(data->input_dev, ABS_BRAKE, rbuf[11]);
> +	}
> +	mutex_unlock(&akm->flags_lock);
> +
> +	input_sync(data->input_dev);
> +}
> +
> +static void akm8975_ecs_close_done(struct akm8975_data *akm)
> +{
> +	FUNCDBG("called");
> +	mutex_lock(&akm->flags_lock);
> +	m_flag = 1;
> +	a_flag = 1;
> +	t_flag = 1;
> +	mv_flag = 1;
> +	mutex_unlock(&akm->flags_lock);
> +}
> +
> +static int akm_aot_open(struct inode *inode, struct file *file)
> +{
> +	int ret = -1;

Useless and dangerous initialization.

> +
> +	FUNCDBG("called");
> +	if (atomic_cmpxchg(&open_flag, 0, 1) == 0) {
> +		wake_up(&open_wq);
> +		ret = 0;
> +	}
> +
> +	ret = nonseekable_open(inode, file);
> +	if (ret)
> +		return ret;
> +
> +	file->private_data = akmd_data;
> +
> +	return ret;
> +}
> +
> +static int akm_aot_release(struct inode *inode, struct file *file)
> +{
> +	FUNCDBG("called");
> +	atomic_set(&open_flag, 0);
> +	wake_up(&open_wq);
> +	return 0;
> +}
> +
> +static int akm_aot_ioctl(struct inode *inode, struct file *file,
> +	      unsigned int cmd, unsigned long arg)
> +{
> +	void __user *argp = (void __user *) arg;
> +	short flag;
> +	struct akm8975_data *akm = file->private_data;
> +
> +	FUNCDBG("called");
> +
> +	switch (cmd) {
> +	case ECS_IOCTL_APP_SET_MFLAG:
> +	case ECS_IOCTL_APP_SET_AFLAG:
> +	case ECS_IOCTL_APP_SET_MVFLAG:
> +		if (copy_from_user(&flag, argp, sizeof(flag)))
> +			return -EFAULT;
> +		if (flag < 0 || flag > 1)
> +			return -EINVAL;
> +		break;
> +	case ECS_IOCTL_APP_SET_DELAY:
> +		if (copy_from_user(&flag, argp, sizeof(flag)))
> +			return -EFAULT;

No range check?

> +		break;
> +	default:
> +		break;

What's the point?

> +	}
> +
> +	mutex_lock(&akm->flags_lock);
> +	switch (cmd) {
> +	case ECS_IOCTL_APP_SET_MFLAG:
> +		m_flag = flag;
> +		break;
> +	case ECS_IOCTL_APP_GET_MFLAG:
> +		flag = m_flag;
> +		break;
> +	case ECS_IOCTL_APP_SET_AFLAG:
> +		a_flag = flag;
> +		break;
> +	case ECS_IOCTL_APP_GET_AFLAG:
> +		flag = a_flag;
> +		break;
> +	case ECS_IOCTL_APP_SET_MVFLAG:
> +		mv_flag = flag;
> +		break;
> +	case ECS_IOCTL_APP_GET_MVFLAG:
> +		flag = mv_flag;
> +		break;
> +	case ECS_IOCTL_APP_SET_DELAY:
> +		akmd_delay = flag;
> +		break;
> +	case ECS_IOCTL_APP_GET_DELAY:
> +		flag = akmd_delay;
> +		break;
> +	default:
> +		return -ENOTTY;

You return with a lock held...

> +	}
> +	mutex_unlock(&akm->flags_lock);
> +
> +	switch (cmd) {
> +	case ECS_IOCTL_APP_GET_MFLAG:
> +	case ECS_IOCTL_APP_GET_AFLAG:
> +	case ECS_IOCTL_APP_GET_MVFLAG:
> +	case ECS_IOCTL_APP_GET_DELAY:
> +		if (copy_to_user(argp, &flag, sizeof(flag)))
> +			return -EFAULT;
> +		break;
> +	default:
> +		break;

Needless statement.

> +	}
> +
> +	return 0;
> +}
> +
> +static int akmd_open(struct inode *inode, struct file *file)
> +{
> +	int err = 0;

Useless initialization.

> +
> +	FUNCDBG("called");
> +	err = nonseekable_open(inode, file);
> +	if (err)
> +		return err;
> +
> +	file->private_data = akmd_data;
> +	return 0;
> +}
> +
> +static int akmd_release(struct inode *inode, struct file *file)
> +{
> +	struct akm8975_data *akm = file->private_data;
> +
> +	FUNCDBG("called");
> +	akm8975_ecs_close_done(akm);
> +	return 0;
> +}
> +
> +static int akmd_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> +		      unsigned long arg)
> +{
> +	void __user *argp = (void __user *) arg;
> +
> +	char rwbuf[16];
> +	int ret = -1;

Useless and dangerous initialization.

> +	int status;
> +	short value[12];
> +	short delay;
> +	struct akm8975_data *akm = file->private_data;
> +
> +	FUNCDBG("called");
> +
> +	switch (cmd) {
> +	case ECS_IOCTL_READ:
> +	case ECS_IOCTL_WRITE:
> +		if (copy_from_user(&rwbuf, argp, sizeof(rwbuf)))
> +			return -EFAULT;
> +		break;
> +
> +	case ECS_IOCTL_SET_YPR:
> +		if (copy_from_user(&value, argp, sizeof(value)))
> +			return -EFAULT;
> +		break;
> +
> +	default:
> +		break;

What's the point?

> +	}
> +
> +	switch (cmd) {
> +	case ECS_IOCTL_READ:
> +		if (rwbuf[0] < 1)
> +			return -EINVAL;

Missing upper bound check.

> +
> +		ret = akm8975_i2c_rxdata(akm, &rwbuf[1], rwbuf[0]);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +
> +	case ECS_IOCTL_WRITE:
> +		if (rwbuf[0] < 2)
> +			return -EINVAL;
> +

Missing upper bound check.

> +		ret = akm8975_i2c_txdata(akm, &rwbuf[1], rwbuf[0]);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	case ECS_IOCTL_SET_YPR:
> +		akm8975_ecs_report_value(akm, value);
> +		break;
> +
> +	case ECS_IOCTL_GET_OPEN_STATUS:
> +		wait_event_interruptible(open_wq,
> +					 (atomic_read(&open_flag) != 0));
> +		status = atomic_read(&open_flag);
> +		break;
> +	case ECS_IOCTL_GET_CLOSE_STATUS:
> +		wait_event_interruptible(open_wq,
> +					 (atomic_read(&open_flag) == 0));
> +		status = atomic_read(&open_flag);
> +		break;
> +
> +	case ECS_IOCTL_GET_DELAY:
> +		delay = akmd_delay;
> +		break;
> +
> +	default:
> +		FUNCDBG("Unknown cmd\n");
> +		return -ENOTTY;
> +	}
> +
> +	switch (cmd) {
> +	case ECS_IOCTL_READ:
> +		if (copy_to_user(argp, &rwbuf, sizeof(rwbuf)))
> +			return -EFAULT;
> +		break;
> +	case ECS_IOCTL_GET_OPEN_STATUS:
> +	case ECS_IOCTL_GET_CLOSE_STATUS:
> +		if (copy_to_user(argp, &status, sizeof(status)))
> +			return -EFAULT;
> +		break;
> +	case ECS_IOCTL_GET_DELAY:
> +		if (copy_to_user(argp, &delay, sizeof(delay)))
> +			return -EFAULT;
> +		break;
> +	default:
> +		break;

Needless statement.

> +	}
> +
> +	return 0;
> +}
> +
> +/* needed to clear the int. pin */
> +static void akm_work_func(struct work_struct *work)
> +{
> +	struct akm8975_data *akm =
> +	    container_of(work, struct akm8975_data, work);
> +
> +	FUNCDBG("called");
> +	enable_irq(akm->this_client->irq);
> +}
> +
> +static irqreturn_t akm8975_interrupt(int irq, void *dev_id)
> +{
> +	struct akm8975_data *akm = dev_id;
> +	FUNCDBG("called");
> +
> +	disable_irq_nosync(akm->this_client->irq);
> +	schedule_work(&akm->work);
> +	return IRQ_HANDLED;
> +}
> +
> +static int akm8975_power_off(struct akm8975_data *akm)
> +{
> +#if AK8975DRV_CALL_DBG
> +	pr_info("%s\n", __func__);
> +#endif

Why not just FUNCDBG("called") as everywhere else?

> +	if (akm->pdata->power_off)
> +		akm->pdata->power_off();

This function could return an error, which you want to transmit.

> +
> +	return 0;
> +}
> +
> +static int akm8975_power_on(struct akm8975_data *akm)
> +{
> +	int err;
> +
> +#if AK8975DRV_CALL_DBG
> +	pr_info("%s\n", __func__);
> +#endif
> +	if (akm->pdata->power_on) {
> +		err = akm->pdata->power_on();
> +		if (err < 0)
> +			return err;
> +	}
> +	return 0;
> +}
> +
> +static int akm8975_init_client(struct i2c_client *client)
> +{
> +	struct akm8975_data *data;
> +	int ret;
> +
> +	data = i2c_get_clientdata(client);
> +
> +	ret = request_irq(client->irq, akm8975_interrupt, IRQF_TRIGGER_RISING,
> +				"akm8975", data);
> +
> +	if (ret < 0) {
> +		pr_err("akm8975_init_client: request irq failed\n");

Please use dev_err().

> +		goto err;
> +	}
> +
> +	init_waitqueue_head(&open_wq);
> +
> +	mutex_lock(&data->flags_lock);
> +	m_flag = 1;
> +	a_flag = 1;
> +	t_flag = 1;
> +	mv_flag = 1;
> +	mutex_unlock(&data->flags_lock);

Shouldn't you set up everything _before_ requesting the IRQ?

> +
> +	return 0;
> +err:
> +	return ret;
> +}
> +
> +static const struct file_operations akmd_fops = {
> +	.owner = THIS_MODULE,
> +	.open = akmd_open,
> +	.release = akmd_release,
> +	.ioctl = akmd_ioctl,
> +};
> +
> +static const struct file_operations akm_aot_fops = {
> +	.owner = THIS_MODULE,
> +	.open = akm_aot_open,
> +	.release = akm_aot_release,
> +	.ioctl = akm_aot_ioctl,
> +};
> +
> +static struct miscdevice akm_aot_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "akm8975_aot",
> +	.fops = &akm_aot_fops,
> +};
> +
> +static struct miscdevice akmd_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "akm8975_dev",
> +	.fops = &akmd_fops,
> +};
> +
> +int akm8975_probe(struct i2c_client *client,
> +		  const struct i2c_device_id *devid)
> +{
> +	struct akm8975_data *akm;
> +	int err;
> +	FUNCDBG("called");
> +
> +	if (client->dev.platform_data == NULL) {
> +		dev_err(&client->dev, "platform data is NULL. exiting.\n");
> +		err = -ENODEV;
> +		goto exit_platform_data_null;
> +	}
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {

This check doesn't match the actual API use. Will need revisiting if
you go for i2c_smbus_{write,read}_i2c_block_data anyway.

> +		dev_err(&client->dev, "platform data is NULL. exiting.\n");

Wrong error message.

> +		err = -ENODEV;
> +		goto exit_check_functionality_failed;
> +	}
> +
> +	akm = kzalloc(sizeof(struct akm8975_data), GFP_KERNEL);
> +	if (!akm) {
> +		dev_err(&client->dev,
> +			"failed to allocate memory for module data\n");
> +		err = -ENOMEM;
> +		goto exit_alloc_data_failed;
> +	}
> +
> +	akm->pdata = client->dev.platform_data;
> +
> +	mutex_init(&akm->flags_lock);
> +	INIT_WORK(&akm->work, akm_work_func);
> +	i2c_set_clientdata(client, akm);
> +
> +	err = akm8975_power_on(akm);
> +	if (err < 0)
> +		goto exit_power_on_failed;
> +
> +	akm8975_init_client(client);
> +	akm->this_client = client;
> +	akmd_data = akm;
> +
> +	akm->input_dev = input_allocate_device();
> +	if (!akm->input_dev) {
> +		err = -ENOMEM;
> +		dev_err(&akm->this_client->dev,
> +			"input device allocate failed\n");
> +		goto exit_input_dev_alloc_failed;
> +	}
> +
> +	set_bit(EV_ABS, akm->input_dev->evbit);
> +
> +	/* yaw */
> +	input_set_abs_params(akm->input_dev, ABS_RX, 0, 23040, 0, 0);
> +	/* pitch */
> +	input_set_abs_params(akm->input_dev, ABS_RY, -11520, 11520, 0, 0);
> +	/* roll */
> +	input_set_abs_params(akm->input_dev, ABS_RZ, -5760, 5760, 0, 0);
> +	/* x-axis acceleration */
> +	input_set_abs_params(akm->input_dev, ABS_X, -5760, 5760, 0, 0);
> +	/* y-axis acceleration */
> +	input_set_abs_params(akm->input_dev, ABS_Y, -5760, 5760, 0, 0);
> +	/* z-axis acceleration */
> +	input_set_abs_params(akm->input_dev, ABS_Z, -5760, 5760, 0, 0);
> +	/* temparature */
> +	input_set_abs_params(akm->input_dev, ABS_THROTTLE, -30, 85, 0, 0);
> +	/* status of magnetic sensor */
> +	input_set_abs_params(akm->input_dev, ABS_RUDDER, 0, 3, 0, 0);
> +	/* status of acceleration sensor */
> +	input_set_abs_params(akm->input_dev, ABS_WHEEL, 0, 3, 0, 0);
> +	/* x-axis of raw magnetic vector */
> +	input_set_abs_params(akm->input_dev, ABS_HAT0X, -20480, 20479, 0, 0);
> +	/* y-axis of raw magnetic vector */
> +	input_set_abs_params(akm->input_dev, ABS_HAT0Y, -20480, 20479, 0, 0);
> +	/* z-axis of raw magnetic vector */
> +	input_set_abs_params(akm->input_dev, ABS_BRAKE, -20480, 20479, 0, 0);
> +
> +	akm->input_dev->name = "compass";
> +
> +	err = input_register_device(akm->input_dev);
> +	if (err) {
> +		pr_err("akm8975_probe: Unable to register input device: %s\n",
> +					 akm->input_dev->name);

dev_err()

> +		goto exit_input_register_device_failed;
> +	}
> +
> +	err = misc_register(&akmd_device);
> +	if (err) {
> +		pr_err("akm8975_probe: akmd_device register failed\n");

dev_err()

> +		goto exit_misc_device_register_failed;
> +	}
> +
> +	err = misc_register(&akm_aot_device);
> +	if (err) {
> +		pr_err("akm8975_probe: akm_aot_device register failed\n");

dev_err()

> +		goto exit_misc_device_register_failed;
> +	}
> +
> +	err = device_create_file(&client->dev, &dev_attr_akm_ms1);

Missing error handling.

> +
> +	return 0;
> +
> +exit_misc_device_register_failed:
> +exit_input_register_device_failed:
> +	input_free_device(akm->input_dev);
> +exit_input_dev_alloc_failed:
> +	akm8975_power_off(akm);
> +exit_power_on_failed:
> +	kfree(akm);
> +exit_alloc_data_failed:
> +exit_check_functionality_failed:
> +exit_platform_data_null:

It is more common to name the labels based on the next undo action.
This avoids having several labels for the same point.

> +	return err;
> +}
> +
> +static int __devexit akm8975_remove(struct i2c_client *client)
> +{
> +	struct akm8975_data *akm = i2c_get_clientdata(client);
> +	FUNCDBG("called");
> +	free_irq(client->irq, NULL);
> +	input_unregister_device(akm->input_dev);
> +	misc_deregister(&akmd_device);
> +	misc_deregister(&akm_aot_device);
> +	akm8975_power_off(akm);
> +	kfree(akm);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id akm8975_id[] = {
> +	{ "akm8975", 0 },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, akm8975_id);
> +
> +static struct i2c_driver akm8975_driver = {
> +	.probe = akm8975_probe,
> +	.remove = akm8975_remove,

Missing __devexit_p().

> +	.id_table = akm8975_id,
> +	.driver = {
> +		.name = "akm8975",
> +	},
> +};
> +
> +static int __init akm8975_init(void)
> +{
> +	pr_info("AK8975 compass driver: init\n");

Debugging message shouldn't be printed with pr_info().

> +	FUNCDBG("AK8975 compass driver: init\n");
> +	return i2c_add_driver(&akm8975_driver);
> +}
> +
> +static void __exit akm8975_exit(void)
> +{
> +	FUNCDBG("AK8975 compass driver: exit\n");
> +	i2c_del_driver(&akm8975_driver);
> +}
> +
> +module_init(akm8975_init);
> +module_exit(akm8975_exit);
> +
> +MODULE_AUTHOR("Hou-Kun Chen <hk_chen-TYsT/PRPW1c@public.gmane.org>");
> +MODULE_DESCRIPTION("AK8975 compass driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/akm8975.h b/include/linux/akm8975.h
> new file mode 100644
> index 0000000..7d02120
> --- /dev/null
> +++ b/include/linux/akm8975.h

This should be include/linux/i2c/akm8975.h, as all other i2c device
drivers to.

> @@ -0,0 +1,87 @@
> +/*
> + * Definitions for akm8975 compass chip.
> + */
> +#ifndef AKM8975_H
> +#define AKM8975_H

Should be __LINUX_I2C_AKM8975_H.

> +
> +#include <linux/ioctl.h>
> +
> +/*! \name AK8975 operation mode
> + \anchor AK8975_Mode
> + Defines an operation mode of the AK8975.*/
> +/*! @{*/

What's this ugly syntax? Please follow the standard documentation
syntax.

> +#define AK8975_MODE_SNG_MEASURE	0x01
> +#define	AK8975_MODE_SELF_TEST	0x08
> +#define	AK8975_MODE_FUSE_ACCESS	0x0F
> +#define	AK8975_MODE_POWER_DOWN	0x00

Incorrect alignment.

> +/*! @}*/
> +
> +#define RBUFF_SIZE		8	/* Rx buffer size */
> +
> +/*! \name AK8975 register address
> +\anchor AK8975_REG
> +Defines a register address of the AK8975.*/
> +/*! @{*/
> +#define AK8975_REG_WIA		0x00
> +#define AK8975_REG_INFO		0x01
> +#define AK8975_REG_ST1		0x02
> +#define AK8975_REG_HXL		0x03
> +#define AK8975_REG_HXH		0x04
> +#define AK8975_REG_HYL		0x05
> +#define AK8975_REG_HYH		0x06
> +#define AK8975_REG_HZL		0x07
> +#define AK8975_REG_HZH		0x08
> +#define AK8975_REG_ST2		0x09
> +#define AK8975_REG_CNTL		0x0A
> +#define AK8975_REG_RSV		0x0B
> +#define AK8975_REG_ASTC		0x0C
> +#define AK8975_REG_TS1		0x0D
> +#define AK8975_REG_TS2		0x0E
> +#define AK8975_REG_I2CDIS	0x0F
> +/*! @}*/

Do users need to know this? I guess not. Public header files should
only contain the API users need. Everything which is only used
internally by the driver shouldn't be there.

> +
> +/*! \name AK8975 fuse-rom address
> +\anchor AK8975_FUSE
> +Defines a read-only address of the fuse ROM of the AK8975.*/
> +/*! @{*/
> +#define AK8975_FUSE_ASAX	0x10
> +#define AK8975_FUSE_ASAY	0x11
> +#define AK8975_FUSE_ASAZ	0x12
> +/*! @}*/
> +
> +#define AKMIO			0xA1
> +
> +/* IOCTLs for AKM library */
> +#define ECS_IOCTL_WRITE                 _IOW(AKMIO, 0x02, char[5])
> +#define ECS_IOCTL_READ                  _IOWR(AKMIO, 0x03, char[5])
> +#define ECS_IOCTL_GETDATA               _IOR(AKMIO, 0x08, char[RBUFF_SIZE])
> +#define ECS_IOCTL_SET_YPR               _IOW(AKMIO, 0x0C, short[12])
> +#define ECS_IOCTL_GET_OPEN_STATUS       _IOR(AKMIO, 0x0D, int)
> +#define ECS_IOCTL_GET_CLOSE_STATUS      _IOR(AKMIO, 0x0E, int)
> +#define ECS_IOCTL_GET_DELAY             _IOR(AKMIO, 0x30, short)
> +
> +/* IOCTLs for APPs */
> +#define ECS_IOCTL_APP_SET_MFLAG		_IOW(AKMIO, 0x11, short)
> +#define ECS_IOCTL_APP_GET_MFLAG		_IOW(AKMIO, 0x12, short)
> +#define ECS_IOCTL_APP_SET_AFLAG		_IOW(AKMIO, 0x13, short)
> +#define ECS_IOCTL_APP_GET_AFLAG		_IOR(AKMIO, 0x14, short)
> +#define ECS_IOCTL_APP_SET_DELAY		_IOW(AKMIO, 0x18, short)
> +#define ECS_IOCTL_APP_GET_DELAY		ECS_IOCTL_GET_DELAY
> +/* Set raw magnetic vector flag */
> +#define ECS_IOCTL_APP_SET_MVFLAG	_IOW(AKMIO, 0x19, short)
> +/* Get raw magnetic vector flag */
> +#define ECS_IOCTL_APP_GET_MVFLAG	_IOR(AKMIO, 0x1A, short)
> +#define ECS_IOCTL_APP_SET_TFLAG         _IOR(AKMIO, 0x15, short)
> +
> +
> +struct akm8975_platform_data {
> +	int intr;
> +
> +	int (*init)(void);
> +	void (*exit)(void);
> +	int (*power_on)(void);
> +	int (*power_off)(void);

These "void" parameters seem suspicious. I would expect at least the
device to be passed as a parameter. I understand that your driver is
currently limited to supporting a single device, but you should avoid
designing everything based on this assumption, because hopefully it can
be solved someday.

> +};
> +
> +#endif
> +


-- 
Jean Delvare

Re: [PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver

[Andrew Morton]

On Fri, 27 Aug 2010 09:16:07 +0200 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> > +static int akm_aot_open(struct inode *inode, struct file *file)
> > +{
> > +	int ret = -1;
> 
> Useless and dangerous initialization.
> 
> > +
> > +	FUNCDBG("called");
> > +	if (atomic_cmpxchg(&open_flag, 0, 1) == 0) {
> > +		wake_up(&open_wq);
> > +		ret = 0;

this doesn't do anything either.

> > +	}
> > +
> > +	ret = nonseekable_open(inode, file);
> > +	if (ret)
> > +		return ret;
> > +
> > +	file->private_data = akmd_data;
> > +
> > +	return ret;
> > +}

Re: [PATCH 1/1] drivers/misc/akm8975: Add compass sensor driver

[Alan Cox]

> > +       If you say yes here you get support for Asahi Kasei's
> > +       orientation sensor AK8975.
> 
> Should it be in drivers/hwmon?

Who knows - Linus needs to settle that mess at KS for all these
sensors. It's not hwmon however. Some acceleromters are in hwmon for
historical reasons. Input makes some sense to me but...


> > +#define AK8975DRV_CALL_DBG 0
> > +#if AK8975DRV_CALL_DBG
> > +#define FUNCDBG(msg) pr_err("%s:%s\n", __func__, msg);
> > +#else
> > +#define FUNCDBG(msg)
> > +#endif

Can we stop having DIY macros for debug

> The driver assumes that I will only ever have one device in the
> machine.  That's surely a reasonable assumption, unless I'm making a
> compass-testing workstation.  But it's a bit sad from a general driver
> design point of view.

I'm not so sure - its a generic component. There are lots of similar
devices and robots and the like often have multiple sensors as does
anything fault-tolerant.

> > +static int akm_aot_open(struct inode *inode, struct file *file)
> > +{
> > +     int ret = -1;
> > +
> > +     FUNCDBG("called");
> > +     if (atomic_cmpxchg(&open_flag, 0, 1) == 0) {
> > +             wake_up(&open_wq);
> > +             ret = 0;
> > +     }
> 
> What's all this doing?

It's from some kind of template various people keep using for these
drivers, unfortunately the template is a bit weird and uses
atomic_cmpxchg not test_and_set as it should. The template also uses
ioctls on a random misc device not sysfs files on the input device, and
has locking errors on the ioctl

All faithfully copied in this one...

> I'm suspecting that we're not getting ourselves a standardised Linux
> compass driver API :( I think this is a significant problem!

It needs sorting. This driver should go to the linux-input list so
Dmitry can veto it using the input layer, or accept it in which case we
can all copy the interface.

(With Intel hat on we have an akm8974 device with driver internally
that is also waiting someone to work out wtf the kernel API should be
so we can submit it). I will have to see if we can merge 8974/5 into
one driver sanely.

> Can ->power_off and ->power_on ever be NULL?

In the template it is copied from yes - because it assumes the platform
layer may want to fill stuff in. Of course it *should* be using runtime
pm

> No suspend/resume handling?

Seems odd as it has power functions yes.

The misc interface really ought to go away, its for random ioctls and
once you've got an input device and multi-device support you cannot
associate the two sanely. sysfs on the input device is surely the way
to go - or the lot on the misc device depending what Dmitry thinks.

Alan