[PATCH 0/3] ak8974 / ami305 magnetometer driver

[PATCH 0/3] ak8974 / ami305 magnetometer driver

[Samu Onkalo]

Patch set provides driver for ak8974 and ami305 3 axis magnetometer driver.
Data is provided via misc character device as a structure of data.
Driver supports regulator framework and has PM support.

Short documentation is also provided. Data format and sysfs interface
is shortly described in the documentation.

Provided data is raw magnetic field density for 3 axis. For compass
purposes data requires further processing.


Samu Onkalo (3):
  drivers: misc: ak8974 / ami305 magnetometer driver
  drivers: misc: ak8974 support to Kconfig and Makefile
  Documentation: Documentation for ak8974 magnetometer chip driver

 Documentation/misc-devices/ak8974 |   66 ++++
 drivers/misc/Kconfig              |   11 +
 drivers/misc/Makefile             |    1 +
 drivers/misc/ak8974.c             |  703 +++++++++++++++++++++++++++++++++++++
 include/linux/i2c/ak8974.h        |   50 +++
 5 files changed, 831 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/misc-devices/ak8974
 create mode 100644 drivers/misc/ak8974.c
 create mode 100644 include/linux/i2c/ak8974.h

[PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Samu Onkalo]

Patch provides support for ak8974 / ami305 3-axis magnetometer chip.
Results are provided via misc character device.
Driver supports multiple instaces of the chip.
Driver has PM and regulator framework support.

Signed-off-by: Samu Onkalo <samu.p.onkalo@nokia.com>
---
 drivers/misc/ak8974.c      |  703 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/ak8974.h |   50 ++++
 2 files changed, 753 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/ak8974.c
 create mode 100644 include/linux/i2c/ak8974.h

diff --git a/drivers/misc/ak8974.c b/drivers/misc/ak8974.c
new file mode 100644
index 0000000..e480314
--- /dev/null
+++ b/drivers/misc/ak8974.c
@@ -0,0 +1,703 @@
+/*
+ * Driver for ak8974 (Asahi Kasei EMD Corporation)
+ * and ami305 (Aichi Steel) magnetometer chip.
+ *
+ * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
+ *
+ * Contact: Samu Onkalo <samu.p.onkalo@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * 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., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <asm/atomic.h>
+#include <linux/delay.h>
+#include <linux/i2c/ak8974.h>
+#include <linux/regulator/consumer.h>
+#include <linux/poll.h>
+#include <linux/device.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+
+/*
+ * 16-bit registers are little-endian. LSB is at the address defined below
+ * and MSB is at the next higher address.
+ */
+
+/* for AK8974 and AMI305 registers */
+#define AK8974_SELFTEST		0x0C
+#define AK8974_INFO		0x0D
+#define AK8974_WHOAMI		0x0F
+#define AK8974_DATA_X		0x10
+#define AK8974_DATA_Y		0x12
+#define AK8974_DATA_Z		0x14
+#define AK8974_INT_SRC		0x16
+#define AK8974_STATUS		0x18
+#define AK8974_INT_CLEAR	0x1A
+#define AK8974_CTRL1		0x1B
+#define AK8974_CTRL2		0x1C
+#define AK8974_CTRL3		0x1D
+#define AK8974_INT_CTRL		0x1E
+#define AK8974_OFFSET_X		0x20
+#define AK8974_OFFSET_Y		0x22
+#define AK8974_OFFSET_Z		0x24
+#define AK8974_INT_THRES	0x26  /* absolute any axis value threshold */
+#define AK8974_PRESET		0x30
+#define AK8974_TEMP		0x31
+
+#define AK8974_SELFTEST_IDLE	0x55
+#define AK8974_SELFTEST_OK	0xAA
+
+#define AK8974_WHOAMI_VALUE_AMI305 0x47
+#define AK8974_WHOAMI_VALUE_AK8974 0x48
+#define AK8974_INT_X_HIGH	0x80  /* Axis over +threshold  */
+#define AK8974_INT_Y_HIGH	0x40
+#define AK8974_INT_Z_HIGH	0x20
+#define AK8974_INT_X_LOW	0x10  /* Axis below -threshold	*/
+#define AK8974_INT_Y_LOW	0x08
+#define AK8974_INT_Z_LOW	0x04
+#define AK8974_INT_RANGE	0x02  /* Range overflow (any axis)   */
+
+#define AK8974_STATUS_DRDY	0x40  /* Data ready	    */
+#define AK8974_STATUS_OVERRUN	0x20  /* Data overrun	    */
+#define AK8974_STATUS_INT	0x10  /* Interrupt occurred */
+
+#define AK8974_CTRL1_POWER	0x80  /* 0 = standby; 1 = active */
+#define AK8974_CTRL1_RATE	0x10  /* 0 = 10 Hz;   1 = 20 Hz	 */
+#define AK8974_CTRL1_FORCE_EN	0x02  /* 0 = normal;  1 = force	 */
+#define AK8974_CTRL1_MODE2	0x01  /* 0 */
+
+#define AK8974_CTRL2_INT_EN	0x10  /* 1 = enable interrupts	      */
+#define AK8974_CTRL2_DRDY_EN	0x08  /* 1 = enable data ready signal */
+#define AK8974_CTRL2_DRDY_POL	0x04  /* 1 = data ready active high   */
+#define AK8974_CTRL2_RESDEF	(AK8974_CTRL2_DRDY_POL)
+
+#define AK8974_CTRL3_RESET	0x80  /* Software reset		  */
+#define AK8974_CTRL3_FORCE	0x40  /* Start forced measurement */
+#define AK8974_CTRL3_SELFTEST	0x10  /* Set selftest register	  */
+#define AK8974_CTRL3_RESDEF	0x00
+
+#define AK8974_INT_CTRL_XEN	0x80  /* Enable interrupt for this axis */
+#define AK8974_INT_CTRL_YEN	0x40
+#define AK8974_INT_CTRL_ZEN	0x20
+#define AK8974_INT_CTRL_XYZEN	0xE0
+#define AK8974_INT_CTRL_POL	0x08  /* 0 = active low; 1 = active high     */
+#define AK8974_INT_CTRL_PULSE	0x02  /* 0 = latched;	 1 = pulse (50 usec) */
+#define AK8974_INT_CTRL_RESDEF	(AK8974_INT_CTRL_XYZEN | AK8974_INT_CTRL_POL)
+
+#define AK8974_INT_SCR_MROI	0x02
+
+#define AK8974_MAX_RANGE	2048
+
+#define AK8974_POWERON_DELAY	50
+#define AK8974_ACTIVATE_DELAY	1
+#define AK8974_SELFTEST_DELAY	1
+
+#define AK8974_MEASTIME		3
+
+#define AK8974_PWR_ON		1
+#define AK8974_PWR_OFF		0
+
+struct ak8974_chip {
+	struct miscdevice	miscdev;
+	struct mutex		lock;	/* Serialize access to chip */
+	struct mutex		users_lock;
+	struct i2c_client	*client;
+	struct regulator_bulk_data regs[2];
+	struct work_struct	work;
+	wait_queue_head_t	misc_wait;
+	loff_t			offset;
+
+	int			max_range;
+	int			users;
+
+	const char		*id;
+	u8			info[2];
+
+	s16			x, y, z; /* Latest measurements */
+	s8			axis_x;
+	s8			axis_y;
+	s8			axis_z;
+	bool			valid;
+
+	char			name[20];
+};
+
+static const char reg_avdd[] = "AVdd";
+static const char reg_dvdd[] = "DVdd";
+static atomic_t device_number = ATOMIC_INIT(0);
+
+static inline int ak8974_write(struct ak8974_chip *chip, u8 reg, u8 data)
+{
+	return i2c_smbus_write_byte_data(chip->client, reg, data);
+}
+
+static inline int ak8974_read(struct ak8974_chip *chip, u8 reg)
+{
+	return i2c_smbus_read_byte_data(chip->client, reg);
+}
+
+static inline int ak8974_read_block(struct ak8974_chip *chip, u8 reg,
+				u8 *data, u8 length)
+{
+	return i2c_smbus_read_i2c_block_data(chip->client, reg, length, data);
+}
+
+static int ak8974_enable(struct ak8974_chip *chip, bool mode)
+{
+	int r, v;
+
+	v = mode ? AK8974_CTRL1_POWER : 0;
+	v |= AK8974_CTRL1_FORCE_EN;
+	r = ak8974_write(chip, AK8974_CTRL1, v);
+	if (r < 0)
+		return r;
+
+	if (mode)
+		msleep(AK8974_ACTIVATE_DELAY);
+
+	return 0;
+}
+
+static int ak8974_reset(struct ak8974_chip *chip)
+{
+	int r;
+
+	/* Power on to get register access. Sets CTRL1 reg to reset state */
+	r = ak8974_enable(chip, AK8974_PWR_ON);
+	r |= ak8974_write(chip, AK8974_CTRL2, AK8974_CTRL2_RESDEF);
+	r |= ak8974_write(chip, AK8974_CTRL3, AK8974_CTRL3_RESDEF);
+	r |= ak8974_write(chip, AK8974_INT_CTRL, AK8974_INT_CTRL_RESDEF);
+
+	/* After reset, power off is default state */
+	r |= ak8974_enable(chip, AK8974_PWR_OFF);
+
+	return r;
+}
+
+static int ak8974_configure(struct ak8974_chip *chip)
+{
+	int err;
+
+	err = ak8974_write(chip, AK8974_CTRL2, AK8974_CTRL2_DRDY_EN |
+			AK8974_CTRL2_INT_EN);
+	err |= ak8974_write(chip, AK8974_CTRL3, 0);
+	err |= ak8974_write(chip, AK8974_INT_CTRL, AK8974_INT_CTRL_POL);
+	err |= ak8974_write(chip, AK8974_PRESET, 0);
+
+	return err;
+}
+
+static int ak8974_poll_drdy(struct ak8974_chip *chip)
+{
+	int timeout = 2;
+	int ret;
+
+	do {
+		msleep(AK8974_MEASTIME);
+		ret = ak8974_read(chip, AK8974_STATUS);
+		if (ret < 0)
+			return ret;
+		if (ret & AK8974_STATUS_DRDY)
+			return 0;
+	} while (--timeout);
+	return ret;
+}
+
+static int ak8974_trigmeas(struct ak8974_chip *chip)
+{
+	int ctrl3;
+
+	/* Clear previous measurement overflow status */
+	ak8974_read(chip, AK8974_INT_CLEAR);
+
+	ctrl3 = ak8974_read(chip, AK8974_CTRL3);
+	if (ctrl3 < 0)
+		return ctrl3;
+
+	return ak8974_write(chip, AK8974_CTRL3, ctrl3 | AK8974_CTRL3_FORCE);
+}
+
+static int ak8974_getresult(struct ak8974_chip *chip, s16 *result)
+{
+	int ret;
+	int i;
+
+	ret = ak8974_poll_drdy(chip);
+	/* Measurement overflow ? */
+	ret |= ak8974_read(chip, AK8974_INT_SRC);
+	if (ret < 0)
+		return ret;
+	if (ret & AK8974_INT_SCR_MROI)
+		ret = -ERANGE;
+	else
+		ret = 0;
+
+	ak8974_read_block(chip, AK8974_DATA_X, (u8 *)result, 6);
+	for (i = 0; i < 3; i++)
+		result[i] = le16_to_cpu(result[i]);
+
+	return ret;
+}
+
+static int ak8974_selftest(struct ak8974_chip *chip)
+{
+	int r;
+	int ret = -EIO;
+
+	r = ak8974_read(chip, AK8974_SELFTEST);
+	if (r != AK8974_SELFTEST_IDLE)
+		goto out;
+
+	r = ak8974_read(chip, AK8974_CTRL3);
+	if (r < 0)
+		goto out;
+
+	r = ak8974_write(chip, AK8974_CTRL3, r | AK8974_CTRL3_SELFTEST);
+	if (r < 0)
+		goto out;
+
+	msleep(AK8974_SELFTEST_DELAY);
+
+	r = ak8974_read(chip, AK8974_SELFTEST);
+	if (r != AK8974_SELFTEST_OK)
+		goto out;
+
+	r = ak8974_read(chip, AK8974_SELFTEST);
+	if (r == AK8974_SELFTEST_IDLE)
+		ret = 0;
+
+out:
+	return ret;
+}
+
+static int ak8974_regulators_on(struct ak8974_chip *chip)
+{
+	int ret;
+	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regs), chip->regs);
+	if (ret == 0)
+		msleep(AK8974_POWERON_DELAY);
+
+	return ret;
+}
+
+static inline void ak8974_regulators_off(struct ak8974_chip *chip)
+{
+	regulator_bulk_disable(ARRAY_SIZE(chip->regs), chip->regs);
+}
+
+static int ak8974_add_users(struct ak8974_chip *chip)
+{
+	int r = 0;
+	mutex_lock(&chip->users_lock);
+
+	if (chip->users == 0) {
+		r = ak8974_regulators_on(chip);
+		if (r < 0)
+			goto release_lock;
+		r = ak8974_enable(chip, AK8974_PWR_ON);
+		if (r < 0)
+			goto fail2;
+		r = ak8974_configure(chip);
+		if (r < 0)
+			goto fail3;
+	}
+	chip->users++;
+	goto release_lock;
+fail3:
+	ak8974_enable(chip, AK8974_PWR_OFF);
+fail2:
+	ak8974_regulators_off(chip);
+release_lock:
+	mutex_unlock(&chip->users_lock);
+	return r;
+}
+
+static void ak8974_remove_users(struct ak8974_chip *chip)
+{
+	mutex_lock(&chip->users_lock);
+
+	if (chip->users != 0)
+		chip->users--;
+
+	if (chip->users == 0) {
+		ak8974_enable(chip, AK8974_PWR_OFF);
+		ak8974_regulators_off(chip);
+	}
+	mutex_unlock(&chip->users_lock);
+}
+
+static int ak8974_get_axis(s8 axis, s16 hw_values[3])
+{
+	if (axis > 0)
+		return hw_values[axis - 1];
+	else
+		return -hw_values[-axis - 1];
+}
+
+static int ak8974_read_values(struct ak8974_chip *chip)
+{
+	s16 hw_values[3];
+	int ret;
+
+	mutex_lock(&chip->lock);
+	ak8974_trigmeas(chip);
+	ret = ak8974_getresult(chip, hw_values);
+	chip->x = ak8974_get_axis(chip->axis_x, hw_values);
+	chip->y = ak8974_get_axis(chip->axis_y, hw_values);
+	chip->z = ak8974_get_axis(chip->axis_z, hw_values);
+	chip->valid = (ret == 0) ? 1 : 0;
+	chip->offset += sizeof(struct ak8974_data);
+	mutex_unlock(&chip->lock);
+	wake_up_interruptible(&chip->misc_wait);
+	return 0;
+}
+
+static void ak8974_work(struct work_struct *work)
+{
+	struct ak8974_chip *chip = container_of(work,
+						struct ak8974_chip,
+						work);
+	ak8974_read_values(chip);
+}
+
+static int ak8974_misc_open(struct inode *inode, struct file *file)
+{
+	struct ak8974_chip *chip = container_of(file->private_data,
+						struct ak8974_chip,
+						miscdev);
+	file->f_pos = chip->offset;
+	return ak8974_add_users(chip);
+}
+
+static int ak8974_misc_close(struct inode *inode, struct file *file)
+{
+	struct ak8974_chip *chip = container_of(file->private_data,
+						struct ak8974_chip,
+						miscdev);
+	ak8974_remove_users(chip);
+	return 0;
+}
+
+static ssize_t ak8974_misc_read(struct file *file, char __user *buf,
+			size_t count, loff_t *offset)
+{
+	struct ak8974_chip *chip = container_of(file->private_data,
+						struct ak8974_chip,
+						miscdev);
+	struct ak8974_data data;
+
+	if (count < sizeof(data))
+		return -EINVAL;
+
+	if (*offset >= chip->offset) {
+		schedule_work(&chip->work);
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+		if (wait_event_interruptible(chip->misc_wait,
+						(*offset < chip->offset)))
+			return -ERESTARTSYS;
+	}
+
+	mutex_lock(&chip->lock);
+	data.x = chip->x;
+	data.y = chip->y;
+	data.z = chip->z;
+	data.valid = chip->valid;
+	*offset = chip->offset;
+	mutex_unlock(&chip->lock);
+
+	return copy_to_user(buf, &data, sizeof(data)) ? -EFAULT : sizeof(data);
+}
+
+static ssize_t ak8974_show_selftest(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct ak8974_chip *chip = dev_get_drvdata(dev);
+	int ret;
+
+	ak8974_add_users(chip);
+	mutex_lock(&chip->lock);
+	ret = ak8974_selftest(chip);
+	mutex_unlock(&chip->lock);
+	ak8974_remove_users(chip);
+
+	return sprintf(buf, "%s\n", ret ? "FAIL" : "OK");
+}
+
+static DEVICE_ATTR(selftest, S_IRUGO, ak8974_show_selftest, NULL);
+
+static ssize_t ak8974_show_chip_id(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct ak8974_chip *chip = dev_get_drvdata(dev);
+	return sprintf(buf, "%s rev %d\n", chip->id,
+		chip->info[0] | (u16)chip->info[1] << 8);
+}
+
+static DEVICE_ATTR(chip_id, S_IRUGO, ak8974_show_chip_id, NULL);
+
+static ssize_t ak8974_show_range(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct ak8974_chip *chip = dev_get_drvdata(dev);
+	return sprintf(buf, "%d\n", chip->max_range);
+}
+
+static DEVICE_ATTR(range, S_IRUGO, ak8974_show_range, NULL);
+
+static struct attribute *sysfs_attrs[] = {
+	&dev_attr_selftest.attr,
+	&dev_attr_range.attr,
+	&dev_attr_chip_id.attr,
+	NULL
+};
+
+static struct attribute_group ak8974_attribute_group = {
+	.attrs = sysfs_attrs
+};
+
+static const struct file_operations ak8974_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.read		= ak8974_misc_read,
+	.open		= ak8974_misc_open,
+	.release	= ak8974_misc_close,
+};
+
+static int ak8974_detect(struct ak8974_chip *chip, const char *name)
+{
+	int whoiam;
+	whoiam = ak8974_read(chip, AK8974_WHOAMI);
+
+	switch (whoiam) {
+	case AK8974_WHOAMI_VALUE_AMI305:
+		chip->id = chip->client->driver->id_table[0].name;
+		break;
+	case AK8974_WHOAMI_VALUE_AK8974:
+		chip->id = chip->client->driver->id_table[1].name;
+		break;
+	default:
+		dev_dbg(&chip->client->dev, "unsupported device\n");
+		return -ENODEV;
+	}
+
+	ak8974_read_block(chip, AK8974_INFO, chip->info, 2);
+
+	chip->max_range = AK8974_MAX_RANGE;
+
+	return 0;
+}
+
+static int __devinit ak8974_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct ak8974_chip *chip;
+	struct ak8974_platform_data *pdata;
+	int err;
+	int x, y, z;
+
+	chip = kzalloc(sizeof *chip, GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, chip);
+	chip->client  = client;
+
+	pdata = client->dev.platform_data;
+
+	x = AK8974_DEV_X;
+	y = AK8974_DEV_Y;
+	z = AK8974_DEV_Z;
+
+	if (pdata) {
+		/* Remap axes */
+		x = pdata->axis_x ? pdata->axis_x : x;
+		y = pdata->axis_y ? pdata->axis_y : y;
+		z = pdata->axis_z ? pdata->axis_z : z;
+	}
+
+	if ((abs(x) > 3) || (abs(y) > 3) || (abs(z) > 3)) {
+		dev_err(&client->dev, "Incorrect platform data\n");
+		err = -EINVAL;
+		goto fail1;
+	}
+
+	chip->axis_x = x;
+	chip->axis_y = y;
+	chip->axis_z = z;
+
+	mutex_init(&chip->lock);
+	mutex_init(&chip->users_lock);
+	init_waitqueue_head(&chip->misc_wait);
+	INIT_WORK(&chip->work, ak8974_work);
+
+	chip->regs[0].supply = reg_avdd;
+	chip->regs[1].supply = reg_dvdd;
+
+	err = regulator_bulk_get(&client->dev,
+				 ARRAY_SIZE(chip->regs), chip->regs);
+	if (err < 0) {
+		dev_err(&client->dev, "Cannot get regulators\n");
+		goto fail1;
+	}
+
+	err = ak8974_regulators_on(chip);
+	if (err < 0) {
+		dev_err(&client->dev, "Cannot enable regulators\n");
+		goto fail2;
+	}
+
+	err = ak8974_detect(chip, id->name);
+	if (err < 0) {
+		dev_err(&client->dev, "Neither AK8974 nor ami305 found\n");
+		goto fail3;
+	}
+
+	snprintf(chip->name, sizeof(chip->name), "ak8974%d",
+		atomic_add_return(1, &device_number) - 1);
+
+	err = ak8974_reset(chip);
+	if (err) {
+		dev_err(&client->dev, "Chip reset failed");
+		goto fail3;
+	}
+
+	ak8974_regulators_off(chip);
+
+	chip->miscdev.minor  = MISC_DYNAMIC_MINOR;
+	chip->miscdev.name   = chip->name;
+	chip->miscdev.fops   = &ak8974_fops;
+	chip->miscdev.parent = &chip->client->dev;
+	err = misc_register(&chip->miscdev);
+	if (err < 0) {
+		dev_err(&chip->client->dev, "Device registration failed\n");
+		goto fail3;
+	}
+
+	err = sysfs_create_group(&chip->client->dev.kobj,
+				&ak8974_attribute_group);
+	if (err) {
+		dev_err(&client->dev, "Sysfs registration failed\n");
+		goto fail4;
+	}
+
+
+	return 0;
+fail4:
+	misc_deregister(&chip->miscdev);
+fail3:
+	ak8974_regulators_off(chip);
+fail2:
+	regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
+fail1:
+	kfree(chip);
+	return err;
+}
+
+static int __devexit ak8974_remove(struct i2c_client *client)
+{
+	struct ak8974_chip *chip = i2c_get_clientdata(client);
+	misc_deregister(&chip->miscdev);
+	cancel_work_sync(&chip->work);
+	sysfs_remove_group(&chip->client->dev.kobj,
+			&ak8974_attribute_group);
+	/* No users after sysfs removal and unregisterin of input device */
+	regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
+	kfree(chip);
+	return 0;
+}
+
+
+#ifdef CONFIG_PM
+static int ak8974_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	struct ak8974_chip *chip = i2c_get_clientdata(client);
+	mutex_lock(&chip->users_lock);
+	if (chip->users > 0)
+		ak8974_enable(chip, AK8974_PWR_OFF);
+	mutex_unlock(&chip->users_lock);
+	return 0;
+}
+
+static int ak8974_resume(struct i2c_client *client)
+{
+	struct ak8974_chip *chip = i2c_get_clientdata(client);
+	mutex_lock(&chip->users_lock);
+	if (chip->users > 0)
+		ak8974_enable(chip, AK8974_PWR_ON);
+	mutex_unlock(&chip->users_lock);
+	return 0;
+}
+
+static void ak8974_shutdown(struct i2c_client *client)
+{
+	struct ak8974_chip *chip = i2c_get_clientdata(client);
+
+	mutex_lock(&chip->users_lock);
+	if (chip->users > 0)
+		ak8974_enable(chip, AK8974_PWR_OFF);
+
+	mutex_unlock(&chip->users_lock);
+}
+
+#else
+#define ak8974_suspend	NULL
+#define ak8974_shutdown NULL
+#define ak8974_resume	NULL
+#endif
+
+static const struct i2c_device_id ak8974_id[] = {
+	{"ami305", 0 },
+	{"ak8974", 0 },
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, ak8974_id);
+
+static struct i2c_driver ak8974_driver = {
+	.driver	 = {
+		.name	= "ak8974",
+		.owner	= THIS_MODULE,
+	},
+	.suspend  = ak8974_suspend,
+	.shutdown = ak8974_shutdown,
+	.resume	  = ak8974_resume,
+	.probe	  = ak8974_probe,
+	.remove	  = __devexit_p(ak8974_remove),
+	.id_table = ak8974_id,
+};
+
+static int __init ak8974_init(void)
+{
+	return i2c_add_driver(&ak8974_driver);
+}
+
+static void __exit ak8974_exit(void)
+{
+	i2c_del_driver(&ak8974_driver);
+}
+
+
+MODULE_DESCRIPTION("AK8974/5 and AMI305 3-axis magnetometer driver");
+MODULE_AUTHOR("Samu Onkalo, Nokia Corporation");
+MODULE_LICENSE("GPL v2");
+
+module_init(ak8974_init);
+module_exit(ak8974_exit);
diff --git a/include/linux/i2c/ak8974.h b/include/linux/i2c/ak8974.h
new file mode 100644
index 0000000..b991404
--- /dev/null
+++ b/include/linux/i2c/ak8974.h
@@ -0,0 +1,50 @@
+/*
+ * Driver for ak8974 (Asahi Kasei EMD Corporation)
+ * and ami305 (Aichi Steel) magnetometer chip.
+ *
+ * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
+ *
+ * Contact: Samu Onkalo <samu.p.onkalo@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * 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., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+
+#ifndef __LINUX_I2C_AK8974_H
+#define __LINUX_I2C_AK8974_H
+
+#define AK8974_NO_MAP		  0
+#define AK8974_DEV_X		  1
+#define AK8974_DEV_Y		  2
+#define AK8974_DEV_Z		  3
+#define AK8974_INV_DEV_X	 -1
+#define AK8974_INV_DEV_Y	 -2
+#define AK8974_INV_DEV_Z	 -3
+
+struct ak8974_platform_data {
+	s8 axis_x;
+	s8 axis_y;
+	s8 axis_z;
+};
+
+/* Device name: /dev/ak8974n, where n is a running number */
+struct ak8974_data {
+	__s16 x;
+	__s16 y;
+	__s16 z;
+	__u16 valid;
+} __attribute__((packed));
+
+#endif
-- 
1.6.3.3

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Alan Cox]

> +static int ak8974_regulators_on(struct ak8974_chip *chip)
> +{
> +	int ret;
> +	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regs), chip->regs);
> +	if (ret == 0)
> +		msleep(AK8974_POWERON_DELAY);
> +
> +	return ret;
> +}
> +
> +static inline void ak8974_regulators_off(struct ak8974_chip *chip)
> +{
> +	regulator_bulk_disable(ARRAY_SIZE(chip->regs), chip->regs);
> +}

That bit seems platform specific but in generic code ?


> +static ssize_t ak8974_misc_read(struct file *file, char __user *buf,
> +			size_t count, loff_t *offset)
> +{
> +	struct ak8974_chip *chip = container_of(file->private_data,
> +						struct ak8974_chip,
> +						miscdev);
> +	struct ak8974_data data;

So we have a different API to the ak8975 just posted and to the other
existing devices. This needs sorting out across the devices before there
is a complete disaster. Right now we have a mix of submissions pending
which variously use

	misc + sysfs
	sysfs
	input (reporting X Y Z etc axes)

Someone needs to decide on a single API before it's too late.

> +
> +	if (count < sizeof(data))
> +		return -EINVAL;
> +
> +	if (*offset >= chip->offset) {
> +		schedule_work(&chip->work);
> +		if (file->f_flags & O_NONBLOCK)
> +			return -EAGAIN;
> +		if (wait_event_interruptible(chip->misc_wait,
> +						(*offset < chip->offset)))
> +			return -ERESTARTSYS;
> +	}
> +
> +	mutex_lock(&chip->lock);
> +	data.x = chip->x;
> +	data.y = chip->y;
> +	data.z = chip->z;
> +	data.valid = chip->valid;
> +	*offset = chip->offset;
> +	mutex_unlock(&chip->lock);

What happens if you have two readers - is it fine they get copies of the
same event when it races ?


> +
> +	return copy_to_user(buf, &data, sizeof(data)) ? -EFAULT : sizeof(data);

Pedantically if data is consumed and a partial copy occurs you should
return the bytes successfully copied.

> +static DEVICE_ATTR(chip_id, S_IRUGO, ak8974_show_chip_id, NULL);
> +
> +static ssize_t ak8974_show_range(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct ak8974_chip *chip = dev_get_drvdata(dev);
> +	return sprintf(buf, "%d\n", chip->max_range);
> +}

Other devices make all of this sysfs or use input. We need to work out
what the right interface actually is.

> +	snprintf(chip->name, sizeof(chip->name), "ak8974%d",
> +		atomic_add_return(1, &device_number) - 1);

Surely this is serialized anyway ?


> +#ifdef CONFIG_PM
> +static int ak8974_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	struct ak8974_chip *chip = i2c_get_clientdata(client);
> +	mutex_lock(&chip->users_lock);
> +	if (chip->users > 0)
> +		ak8974_enable(chip, AK8974_PWR_OFF);
> +	mutex_unlock(&chip->users_lock);
> +	return 0;
> +}
> +
> +static int ak8974_resume(struct i2c_client *client)
> +{
> +	struct ak8974_chip *chip = i2c_get_clientdata(client);
> +	mutex_lock(&chip->users_lock);
> +	if (chip->users > 0)
> +		ak8974_enable(chip, AK8974_PWR_ON);
> +	mutex_unlock(&chip->users_lock);
> +	return 0;
> +}

The whole chip->users thing you are implementing is basically a
reimplementation of the kernel runtime pm stuff - so can that be used
instead ?


> +#ifndef __LINUX_I2C_AK8974_H
> +#define __LINUX_I2C_AK8974_H
> +
> +#define AK8974_NO_MAP		  0
> +#define AK8974_DEV_X		  1
> +#define AK8974_DEV_Y		  2
> +#define AK8974_DEV_Z		  3
> +#define AK8974_INV_DEV_X	 -1
> +#define AK8974_INV_DEV_Y	 -2
> +#define AK8974_INV_DEV_Z	 -3
> +
> +struct ak8974_platform_data {
> +	s8 axis_x;
> +	s8 axis_y;
> +	s8 axis_z;
> +};
> +
> +/* Device name: /dev/ak8974n, where n is a running number */
> +struct ak8974_data {
> +	__s16 x;
> +	__s16 y;
> +	__s16 z;
> +	__u16 valid;
> +} __attribute__((packed));

This could go in the C file.

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Onkalo Samu]

Alan, Thank you for the comments.
Dmitry, there is one question for you.

-Samu

On Fri, 2010-08-27 at 14:31 +0200, ext Alan Cox wrote:
> > +static int ak8974_regulators_on(struct ak8974_chip *chip)
> > +{
> > +	int ret;
> > +	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regs), chip->regs);
> > +	if (ret == 0)
> > +		msleep(AK8974_POWERON_DELAY);
> > +
> > +	return ret;
> > +}
> > +
> > +static inline void ak8974_regulators_off(struct ak8974_chip *chip)
> > +{
> > +	regulator_bulk_disable(ARRAY_SIZE(chip->regs), chip->regs);
> > +}
> 
> That bit seems platform specific but in generic code ?
> 

If the regulator frame work is not configured, this code does nothing.
I have understood (hopefully correctly) that if the frame work is in use
drivers could support that directly assuming that regulators are
configured for that platform. If that is not the case, this should be
platform specific.

> 
> > +static ssize_t ak8974_misc_read(struct file *file, char __user *buf,
> > +			size_t count, loff_t *offset)
> > +{
> > +	struct ak8974_chip *chip = container_of(file->private_data,
> > +						struct ak8974_chip,
> > +						miscdev);
> > +	struct ak8974_data data;
> 
> So we have a different API to the ak8975 just posted and to the other
> existing devices. This needs sorting out across the devices before there
> is a complete disaster. Right now we have a mix of submissions pending
> which variously use
> 
> 	misc + sysfs
> 	sysfs
> 	input (reporting X Y Z etc axes)
> 

About year ago I send driver for the same chip with input-device
interface. During that time I asked from Dmitry Torokhov that is that a
correct interface for this kind of driver. I understood that input
should not be used for this kind of sensors.

sysfs is quite handy interface for small sensors. However, one problem
is that the driver doesn't know when the interface is in use.
I ended up to misc device to get information about the usercount for PM
purposes.

Dmitry, what is your opinion about using input device interface for this
kind of sensors?

> Someone needs to decide on a single API before it's too late.
> 

That is definitely true. Could it be IIO?

> > +
> > +	if (count < sizeof(data))
> > +		return -EINVAL;
> > +
> > +	if (*offset >= chip->offset) {
> > +		schedule_work(&chip->work);
> > +		if (file->f_flags & O_NONBLOCK)
> > +			return -EAGAIN;
> > +		if (wait_event_interruptible(chip->misc_wait,
> > +						(*offset < chip->offset)))
> > +			return -ERESTARTSYS;
> > +	}
> > +
> > +	mutex_lock(&chip->lock);
> > +	data.x = chip->x;
> > +	data.y = chip->y;
> > +	data.z = chip->z;
> > +	data.valid = chip->valid;
> > +	*offset = chip->offset;
> > +	mutex_unlock(&chip->lock);
> 
> What happens if you have two readers - is it fine they get copies of the
> same event when it races ?

Yes, it makes sense to report latest available information for all
users.

> 
> 
> > +
> > +	return copy_to_user(buf, &data, sizeof(data)) ? -EFAULT : sizeof(data);
> 
> Pedantically if data is consumed and a partial copy occurs you should
> return the bytes successfully copied.

I need to check that.

> 
> > +static DEVICE_ATTR(chip_id, S_IRUGO, ak8974_show_chip_id, NULL);
> > +
> > +static ssize_t ak8974_show_range(struct device *dev,
> > +				    struct device_attribute *attr, char *buf)
> > +{
> > +	struct ak8974_chip *chip = dev_get_drvdata(dev);
> > +	return sprintf(buf, "%d\n", chip->max_range);
> > +}
> 
> Other devices make all of this sysfs or use input. We need to work out
> what the right interface actually is.
> 

As mentioned above, sysfs is not used to get information about the user
count for PM purposes.

> > +	snprintf(chip->name, sizeof(chip->name), "ak8974%d",
> > +		atomic_add_return(1, &device_number) - 1);
> 
> Surely this is serialized anyway ?

Is it possible that when there are several chips on the system (in
different bussed for example), probe functions are running in parallel?

> 
> 
> > +#ifdef CONFIG_PM
> > +static int ak8974_suspend(struct i2c_client *client, pm_message_t mesg)
> > +{
> > +	struct ak8974_chip *chip = i2c_get_clientdata(client);
> > +	mutex_lock(&chip->users_lock);
> > +	if (chip->users > 0)
> > +		ak8974_enable(chip, AK8974_PWR_OFF);
> > +	mutex_unlock(&chip->users_lock);
> > +	return 0;
> > +}
> > +
> > +static int ak8974_resume(struct i2c_client *client)
> > +{
> > +	struct ak8974_chip *chip = i2c_get_clientdata(client);
> > +	mutex_lock(&chip->users_lock);
> > +	if (chip->users > 0)
> > +		ak8974_enable(chip, AK8974_PWR_ON);
> > +	mutex_unlock(&chip->users_lock);
> > +	return 0;
> > +}
> 
> The whole chip->users thing you are implementing is basically a
> reimplementation of the kernel runtime pm stuff - so can that be used
> instead ?

True. Most probably it can be used instead of own implementation.

> 
> 
> > +#ifndef __LINUX_I2C_AK8974_H
> > +#define __LINUX_I2C_AK8974_H
> > +
> > +#define AK8974_NO_MAP		  0
> > +#define AK8974_DEV_X		  1
> > +#define AK8974_DEV_Y		  2
> > +#define AK8974_DEV_Z		  3
> > +#define AK8974_INV_DEV_X	 -1
> > +#define AK8974_INV_DEV_Y	 -2
> > +#define AK8974_INV_DEV_Z	 -3
> > +
> > +struct ak8974_platform_data {
> > +	s8 axis_x;
> > +	s8 axis_y;
> > +	s8 axis_z;
> > +};
> > +
> > +/* Device name: /dev/ak8974n, where n is a running number */
> > +struct ak8974_data {
> > +	__s16 x;
> > +	__s16 y;
> > +	__s16 z;
> > +	__u16 valid;
> > +} __attribute__((packed));
> 
> This could go in the C file.

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Dmitry Torokhov]

On Fri, Aug 27, 2010 at 07:59:13PM +0300, Onkalo Samu wrote:
> > 
> > > +static ssize_t ak8974_misc_read(struct file *file, char __user *buf,
> > > +			size_t count, loff_t *offset)
> > > +{
> > > +	struct ak8974_chip *chip = container_of(file->private_data,
> > > +						struct ak8974_chip,
> > > +						miscdev);
> > > +	struct ak8974_data data;
> > 
> > So we have a different API to the ak8975 just posted and to the other
> > existing devices. This needs sorting out across the devices before there
> > is a complete disaster. Right now we have a mix of submissions pending
> > which variously use
> > 
> > 	misc + sysfs
> > 	sysfs
> > 	input (reporting X Y Z etc axes)
> > 
> 
> About year ago I send driver for the same chip with input-device
> interface. During that time I asked from Dmitry Torokhov that is that a
> correct interface for this kind of driver. I understood that input
> should not be used for this kind of sensors.
> 
> sysfs is quite handy interface for small sensors. However, one problem
> is that the driver doesn't know when the interface is in use.
> I ended up to misc device to get information about the usercount for PM
> purposes.
> 
> Dmitry, what is your opinion about using input device interface for this
> kind of sensors?
> 

This is really hard question and I am going back and forth myself.

When considering using input subsystem try answering the following
question - is the device's main purpose is indeed to be a human
interface device or do you want to use input subystem because evdev
interface is convenient? If the answer is former- then it should be in
input (or available through input - let's say IIO-to-input bridge
module). If the answer is latter then input is not the right place for
the device.

Lately I was persuaded that 3-axis accelerometers are mainly used as
input devices so I took adxl driver in and I need to get back and review
cma3000 patch...

> > Someone needs to decide on a single API before it's too late.
> > 
> 
> That is definitely true. Could it be IIO?
> 

I was hpoing that IIO would take care of "unnamed" sensors. Here I mean
sensors that measure something and only user/application know exactly
what it is; the same device might measure different things depending on
setup. Take a temperature sensor - ambient temperature, temperature of
some technological process, patient temperature - it is hard for the
kernel to know which one it would be.

This is in contrast with input system that tries to classify
events so that the event has the same meaning regarless of which device
emitted it - KEY_A means the same regardless of keybord; we may route
them differently (multiseat for example), but the meaning is the same.

-- 
Dmitry

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Jonathan Cameron]

On 08/27/10 19:08, Dmitry Torokhov wrote:
> On Fri, Aug 27, 2010 at 07:59:13PM +0300, Onkalo Samu wrote:
>>>
>>>> +static ssize_t ak8974_misc_read(struct file *file, char __user *buf,
>>>> +			size_t count, loff_t *offset)
>>>> +{
>>>> +	struct ak8974_chip *chip = container_of(file->private_data,
>>>> +						struct ak8974_chip,
>>>> +						miscdev);
>>>> +	struct ak8974_data data;
>>>
>>> So we have a different API to the ak8975 just posted and to the other
>>> existing devices. This needs sorting out across the devices before there
>>> is a complete disaster. Right now we have a mix of submissions pending
>>> which variously use
>>>
>>> 	misc + sysfs
>>> 	sysfs
>>> 	input (reporting X Y Z etc axes)
>>>
>>
>> About year ago I send driver for the same chip with input-device
>> interface. During that time I asked from Dmitry Torokhov that is that a
>> correct interface for this kind of driver. I understood that input
>> should not be used for this kind of sensors.
>>
>> sysfs is quite handy interface for small sensors. However, one problem
>> is that the driver doesn't know when the interface is in use.
>> I ended up to misc device to get information about the usercount for PM
>> purposes.
>>
>> Dmitry, what is your opinion about using input device interface for this
>> kind of sensors?
>>
> 
> This is really hard question and I am going back and forth myself.
> 
> When considering using input subsystem try answering the following
> question - is the device's main purpose is indeed to be a human
> interface device or do you want to use input subystem because evdev
> interface is convenient? If the answer is former- then it should be in
> input (or available through input - let's say IIO-to-input bridge
> module). If the answer is latter then input is not the right place for
> the device.
The iio to input bridge is still on the todo list.  Unfortunately none
of the core developers are particularly interested in that so it isn't
a high priority.  Of course we would welcome someone working on it!
If not it we will get to it.
> 
> Lately I was persuaded that 3-axis accelerometers are mainly used as
> input devices so I took adxl driver in and I need to get back and review
> cma3000 patch...
> 
>>> Someone needs to decide on a single API before it's too late.
>>>
>>
>> That is definitely true. Could it be IIO?
>>
I'm in favour ;)

We already have one straight magnetometer and one imu which includes
a magnetometer.  I'd love to see more and would certainly welcome this
driver.
> 
> I was hpoing that IIO would take care of "unnamed" sensors. Here I mean
> sensors that measure something and only user/application know exactly
> what it is; the same device might measure different things depending on
> setup. Take a temperature sensor - ambient temperature, temperature of
> some technological process, patient temperature - it is hard for the
> kernel to know which one it would be.
> 
> This is in contrast with input system that tries to classify
> events so that the event has the same meaning regarless of which device
> emitted it - KEY_A means the same regardless of keybord; we may route
> them differently (multiseat for example), but the meaning is the same.
> 
That's certainly our intent.  

The down side of going with IIO is that it is taking a while to cleanup
the userspace abi (and the core code for that matter!).
Manuel Stahl has been recently pinning down a few issues made apparent via
the generic userspace code he has been working on, so there will be patches
relating to that over the next week.  Ultimately the lack of interface stability
is on reason IIO is still in staging.  All help on this and more general code
review of IIO is welcomed!

For the sysfs devices I'd request that people either match our naming convention
or that of hwmon (which the IIO one extends).  By this I mean the individual
attributes, not the directory naming etc.  That way whatever the resulting
subsystems of the future, we will at least have one naming convention!

The chrdev end of things are more complex. (I'm happy to go into why we have two
types etc but that's a much larger discussion)  As a quick note though, the
structure you have used is obviously very much device (or at least narrow class of)
device specific.  Our approach to this is a description of the format via a set of
sysfs params. Much as you have done we need to maintain the linkage between a 'scan'
of the channels and this approach allows us to do this.

Jonathan

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Mark Brown]

On Fri, Aug 27, 2010 at 07:59:13PM +0300, Onkalo Samu wrote:
> On Fri, 2010-08-27 at 14:31 +0200, ext Alan Cox wrote:

> > > +static int ak8974_regulators_on(struct ak8974_chip *chip)
> > > +{
> > > +	int ret;
> > > +	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regs), chip->regs);

> > That bit seems platform specific but in generic code ?

> If the regulator frame work is not configured, this code does nothing.
> I have understood (hopefully correctly) that if the frame work is in use
> drivers could support that directly assuming that regulators are
> configured for that platform. If that is not the case, this should be
> platform specific.

Your understanding is correct - the regulator API provides separation
between the driver and the platform so that the driver code can work
with any platform.  The driver requests supplies in terms of the
physical supplies the chip has (using the struct device and the supply
names from the datasheet).  The regulator API them matches this with
actual regulators on a given system using data provied separately by the
code specific to that machine so that the regulator and the consumer
using it don't need to know about each other.

If the regulator API is disabled then the basic regulator API calls
compile to inline stubs that report success, and there's a facility for
optionally automatically stubbing out missing supplies when the API is
enabled.

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Onkalo Samu]

On Fri, 2010-08-27 at 20:53 +0200, ext Mark Brown wrote:
> On Fri, Aug 27, 2010 at 07:59:13PM +0300, Onkalo Samu wrote:
> > On Fri, 2010-08-27 at 14:31 +0200, ext Alan Cox wrote:
> 
> > > > +static int ak8974_regulators_on(struct ak8974_chip *chip)
> > > > +{
> > > > +	int ret;
> > > > +	ret = regulator_bulk_enable(ARRAY_SIZE(chip->regs), chip->regs);
> 
> > > That bit seems platform specific but in generic code ?
> 
> > If the regulator frame work is not configured, this code does nothing.
> > I have understood (hopefully correctly) that if the frame work is in use
> > drivers could support that directly assuming that regulators are
> > configured for that platform. If that is not the case, this should be
> > platform specific.
> 
> Your understanding is correct - the regulator API provides separation
> between the driver and the platform so that the driver code can work
> with any platform.  The driver requests supplies in terms of the
> physical supplies the chip has (using the struct device and the supply
> names from the datasheet).  The regulator API them matches this with
> actual regulators on a given system using data provied separately by the
> code specific to that machine so that the regulator and the consumer
> using it don't need to know about each other.
> 
> If the regulator API is disabled then the basic regulator API calls
> compile to inline stubs that report success, and there's a facility for
> optionally automatically stubbing out missing supplies when the API is
> enabled.

Mark, thanks for clarification. So is it ok to keep this in the driver
code.

I would like to get some advice about the following related to PM and
regulator control.

Small sensor with SYSFS only interface:
---------------------------------------
Driver doesn't know when someone opens (or keeps open) sysfs entry.
It only knows when someone reads or writes to it. Sensors may have quite
long wakeup time from total power off (regulators off) so it takes quite
long time to get first result. On the other hand, driver doesn't know
when it is ok to turn off the device.
At some level, this can be handled with a timer which is is kicked every
time when there is read / write operation. However, this is sounds
little bit a hack.

Would it make sense at all to enhange sysfs to have open / close
indication for PM related control? Of course this adds overhead to
every sysfs operation and most of the sysfs entries doesn't need
this kind of feature.

One solution is to have separate disable / enable control entry
for the sensor. This needs addional operations from the applications
and is little bit tricky in case of several users - should it be
on / off type or counting type control.

Misc-char-device + sysfs interface:
-----------------------------------
PM related issues can be nicely handled with char device.
It doesn't matter if there are one or several users. However
char device with binary data format is not that handy in scripts.
Same is true for input device. 


Do you have any suggestions what could be the best way to handle this?

-Samu

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Mark Brown]

On Mon, Aug 30, 2010 at 09:55:00AM +0300, Onkalo Samu wrote:

> Misc-char-device + sysfs interface:
> -----------------------------------
> PM related issues can be nicely handled with char device.
> It doesn't matter if there are one or several users. However
> char device with binary data format is not that handy in scripts.
> Same is true for input device. 

You could always have a char device with a text mode interface, I guess.

> Do you have any suggestions what could be the best way to handle this?

Not really without knowing more about how the device will be used and
the costs associated with powering it up and down - it's all a question
of tradeoffs depending on things like how fast userspace needs a
response and the expected usage patterns (many accesses in quick
succession, occasional widely spaced accesses...).

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Sundar]

Hi Samu,

few minor comments,

On Fri, Aug 27, 2010 at 5:24 PM, Samu Onkalo <samu.p.onkalo@nokia.com> wrote:
> +
> +struct ak8974_chip {
> +       struct miscdevice       miscdev;
> +       struct mutex            lock;   /* Serialize access to chip */
> +       struct mutex            users_lock;
> +       struct i2c_client       *client;
> +       struct regulator_bulk_data regs[2];
> +       struct work_struct      work;
> +       wait_queue_head_t       misc_wait;
> +       loff_t                  offset;
> +
> +       int                     max_range;
> +       int                     users;
> +
> +       const char              *id;
> +       u8                      info[2];
> +
> +       s16                     x, y, z; /* Latest measurements */
> +       s8                      axis_x;
> +       s8                      axis_y;
> +       s8                      axis_z;
> +       bool                    valid;
> +
> +       char                    name[20];
> +};

This can be static ?

> +
> +       ak8974_regulators_off(chip);
> +

[..]

> +       if (err < 0) {
> +               dev_err(&chip->client->dev, "Device registration failed\n");
> +               goto fail3;
> +       }

[..]

> +       return 0;
> +fail4:
> +       misc_deregister(&chip->miscdev);
> +fail3:
> +       ak8974_regulators_off(chip);

in case of fail3, regulators get disabled twice. i think we will have
unbalanced calls to the supplies then.

> +
> +
> +#ifdef CONFIG_PM
> +static int ak8974_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +       struct ak8974_chip *chip = i2c_get_clientdata(client);
> +       mutex_lock(&chip->users_lock);
> +       if (chip->users > 0)
> +               ak8974_enable(chip, AK8974_PWR_OFF);
> +       mutex_unlock(&chip->users_lock);
> +       return 0;
> +}

Can we disable the regulators here too?

Regards,
Sundar

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Onkalo Samu]

Hi Sundar,

On Sat, 2010-08-28 at 18:10 +0200, ext Sundar wrote:
> Hi Samu,
> 
> few minor comments,
> 
> On Fri, Aug 27, 2010 at 5:24 PM, Samu Onkalo <samu.p.onkalo@nokia.com> wrote:
> > +
> > +struct ak8974_chip {
> > +       struct miscdevice       miscdev;
> > +       struct mutex            lock;   /* Serialize access to chip */
> > +       struct mutex            users_lock;
> > +       struct i2c_client       *client;
> > +       struct regulator_bulk_data regs[2];
> > +       struct work_struct      work;
> > +       wait_queue_head_t       misc_wait;
> > +       loff_t                  offset;
> > +
> > +       int                     max_range;
> > +       int                     users;
> > +
> > +       const char              *id;
> > +       u8                      info[2];
> > +
> > +       s16                     x, y, z; /* Latest measurements */
> > +       s8                      axis_x;
> > +       s8                      axis_y;
> > +       s8                      axis_z;
> > +       bool                    valid;
> > +
> > +       char                    name[20];
> > +};
> 
> This can be static ?

It is filled based on the detected chip type.

> 
> > +
> > +       ak8974_regulators_off(chip);
> > +
> 
> [..]
> 
> > +       if (err < 0) {
> > +               dev_err(&chip->client->dev, "Device registration failed\n");
> > +               goto fail3;
> > +       }
> 
> [..]
> 
> > +       return 0;
> > +fail4:
> > +       misc_deregister(&chip->miscdev);
> > +fail3:
> > +       ak8974_regulators_off(chip);
> 
> in case of fail3, regulators get disabled twice. i think we will have
> unbalanced calls to the supplies then.
> 

Yes, you are correct.


> > +
> > +
> > +#ifdef CONFIG_PM
> > +static int ak8974_suspend(struct i2c_client *client, pm_message_t mesg)
> > +{
> > +       struct ak8974_chip *chip = i2c_get_clientdata(client);
> > +       mutex_lock(&chip->users_lock);
> > +       if (chip->users > 0)
> > +               ak8974_enable(chip, AK8974_PWR_OFF);
> > +       mutex_unlock(&chip->users_lock);
> > +       return 0;
> > +}
> 
> Can we disable the regulators here too?

It would require some more operations since the chip would loose its
state totally. But yes, it could be done.

-Samu

Re: [PATCH 1/3] drivers: misc: ak8974 / ami305 magnetometer driver

[Sundar]

Hi again,

On Mon, Aug 30, 2010 at 12:42 PM, Onkalo Samu <samu.p.onkalo@nokia.com> wrote:
>
> It would require some more operations since the chip would loose its
> state totally. But yes, it could be done.
>
Yes. Something like re-programming the context. but it would make sure that
the module draws no power at all.

Also, would it not be correct to include a cancel_work_sync in case of
return here?

+fail4:
+       misc_deregister(&chip->miscdev);
+fail3:
+       ak8974_regulators_off(chip);
+fail2:
+       regulator_bulk_free(ARRAY_SIZE(chip->regs), chip->regs);
+fail1:
+       kfree(chip);
+       return err;
+}

Cheers!

[PATCH 2/3] drivers: misc: ak8974 support to Kconfig and Makefile

[Samu Onkalo]

Signed-off-by: Samu Onkalo <samu.p.onkalo@nokia.com>
---
 drivers/misc/Kconfig  |   11 +++++++++++
 drivers/misc/Makefile |    1 +
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 0b591b6..65615a8 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -314,6 +314,17 @@ config SENSORS_BH1780
 	  This driver can also be built as a module.  If so, the module
 	  will be called bh1780gli.
 
+config AK8974
+         tristate "AK8974 / AMI305 magnetometer chip";
+         depends on I2C
+         default n
+         ---help---
+           Say Y here if you want to build a driver for AK8974 / AMI305
+           magnetometer chip.
+
+           To compile this driver as a module, choose M here: the
+           module will be called ak8974. If unsure, say N here.
+
 config HMC6352
 	tristate "Honeywell HMC6352 compass"
 	depends on I2C
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 255a80d..4abc4b1 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_TIFM_CORE)       	+= tifm_core.o
 obj-$(CONFIG_TIFM_7XX1)       	+= tifm_7xx1.o
 obj-$(CONFIG_PHANTOM)		+= phantom.o
 obj-$(CONFIG_SENSORS_BH1780)	+= bh1780gli.o
+obj-$(CONFIG_AK8974)            += ak8974.o
 obj-$(CONFIG_SGI_IOC4)		+= ioc4.o
 obj-$(CONFIG_ENCLOSURE_SERVICES) += enclosure.o
 obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
-- 
1.6.3.3

[PATCH 3/3] Documentation: Documentation for ak8974 magnetometer chip driver

[Samu Onkalo]

Signed-off-by: Samu Onkalo <samu.p.onkalo@nokia.com>
---
 Documentation/misc-devices/ak8974 |   66 +++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/misc-devices/ak8974

diff --git a/Documentation/misc-devices/ak8974 b/Documentation/misc-devices/ak8974
new file mode 100644
index 0000000..c771f1e
--- /dev/null
+++ b/Documentation/misc-devices/ak8974
@@ -0,0 +1,66 @@
+Kernel driver ak8974
+====================
+
+Supported chips:
+Asahi Kasei ak8974
+Aichi Steel ami305
+
+
+Author: Samu P. Onkalo <samu.p.onkalo@nokia.com>
+
+
+Description
+-----------
+
+Chip is a 3 axis magnetometer sensor. It measures and reports
+magnetic field density in x, y, z axis.
+
+Driver provides interface as a misc-character device. Data is returned one
+measurement result at time as a structure of data. Measurement is triggered
+by reading the device handle. Thus measurement rate is controlled by
+the reading application. Read can be blocking or non-blocking.
+Blocking read trigs an measurement and wait until the result is ready.
+Non-blocking read trigs measurement and returns immediatelly. As soon as the
+data is available non-blocking read returns it.
+
+Driver supports regulator framework and power management.
+
+sysfs interface:
+selftest - RO - performs internal selftest procedure - output: FAIL / OK
+range    - RO - data range
+chip_id  - RO - information of the detected chip type
+
+misc-character device:
+----------------------
+device handle name: /dev/ak8974x
+x in the name starts counting from 0 and it is increased by one for each of the
+detected chip.
+
+Data format:
+struct ak8974_data {
+        __s16 x;
+        __s16 y;
+        __s16 z;
+        __u16 valid;
+} __attribute__((packed));
+Each read from the device returns one measurement result "struct ak8974_data"
+format. Data for each axis is in the same format as it is in the chip register.
+
+Platform data:
+
+define AK8974_NO_MAP             0
+#define AK8974_DEV_X              1
+#define AK8974_DEV_Y              2
+#define AK8974_DEV_Z              3
+#define AK8974_INV_DEV_X         -1
+#define AK8974_INV_DEV_Y         -2
+#define AK8974_INV_DEV_Z         -3
+
+struct ak8974_platform_data {
+        s8 axis_x;
+        s8 axis_y;
+        s8 axis_z;
+};
+
+This is used to remap device orientation to so that the returned
+data is in line with the device mechanics.
-- 
1.6.3.3