[PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[achew-DDmLM1+adcrQT0dZR+AlfA]

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

This is for the Asahi Kasei AK8975 3-axis magnetometer.  It resides within
the magnetometer section of the IIO subsystem, and implements the raw
magn_x,y,z attributes, as well as x,y,z factory calibration attributes.

Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/staging/iio/magnetometer/Kconfig  |   11 +
 drivers/staging/iio/magnetometer/Makefile |    1 +
 drivers/staging/iio/magnetometer/ak8975.c |  517 +++++++++++++++++++++++++++++
 3 files changed, 529 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/magnetometer/ak8975.c

diff --git a/drivers/staging/iio/magnetometer/Kconfig b/drivers/staging/iio/magnetometer/Kconfig
index d014450..00e1204 100644
--- a/drivers/staging/iio/magnetometer/Kconfig
+++ b/drivers/staging/iio/magnetometer/Kconfig
@@ -3,6 +3,17 @@
 #
 comment "Magnetometer sensors"
 
+config SENSORS_AK8975
+	tristate "Asahi Kasei AK8975 3-Axis Magnetometer"
+	default n
+	depends on I2C
+	help
+	  Say yes here to build support for Asahi Kasei AK8975 3-Axis
+	  Magnetometer.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ak8975.
+
 config SENSORS_HMC5843
 	tristate "Honeywell HMC5843 3-Axis Magnetometer"
 	depends on I2C
diff --git a/drivers/staging/iio/magnetometer/Makefile b/drivers/staging/iio/magnetometer/Makefile
index f9bfb2e..e3dbaa4 100644
--- a/drivers/staging/iio/magnetometer/Makefile
+++ b/drivers/staging/iio/magnetometer/Makefile
@@ -2,4 +2,5 @@
 # Makefile for industrial I/O Magnetometer sensors
 #
 
+obj-$(CONFIG_SENSORS_AK8975)	:= ak8975.o
 obj-$(CONFIG_SENSORS_HMC5843)	+= hmc5843.o
diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
new file mode 100644
index 0000000..421a4ff
--- /dev/null
+++ b/drivers/staging/iio/magnetometer/ak8975.c
@@ -0,0 +1,517 @@
+/*
+ * A sensor driver for the magnetometer AK8975.
+ *
+ * Magnetic compass sensor driver for monitoring magnetic flux information.
+ *
+ * Copyright (c) 2010, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 Street, Fifth Floor, Boston, MA	02110-1301, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+
+#include <linux/gpio.h>
+
+#include "../iio.h"
+#include "magnet.h"
+
+/*
+ * Register definitions, as well as various shifts and masks to get at the
+ * individual fields of the registers.
+ */
+#define AK8975_REG_WIA			0x00
+#define AK8975_DEVICE_ID		0x48
+
+#define AK8975_REG_INFO			0x01
+
+#define AK8975_REG_ST1			0x02
+#define AK8975_REG_ST1_DRDY_SHIFT	0
+#define AK8975_REG_ST1_DRDY_MASK	(1 << AK8975_REG_ST1_DRDY_SHIFT)
+
+#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_ST2_DERR_SHIFT	2
+#define AK8975_REG_ST2_DERR_MASK	(1 << AK8975_REG_ST2_DERR_SHIFT)
+
+#define AK8975_REG_ST2_HOFL_SHIFT	3
+#define AK8975_REG_ST2_HOFL_MASK	(1 << AK8975_REG_ST2_HOFL_SHIFT)
+
+#define AK8975_REG_CNTL			0x0A
+#define AK8975_REG_CNTL_MODE_SHIFT	0
+#define AK8975_REG_CNTL_MODE_MASK	(0xF << AK8975_REG_CNTL_MODE_SHIFT)
+#define AK8975_REG_CNTL_MODE_POWER_DOWN	0
+#define AK8975_REG_CNTL_MODE_ONCE	1
+#define AK8975_REG_CNTL_MODE_SELF_TEST	8
+#define AK8975_REG_CNTL_MODE_FUSE_ROM	0xF
+
+#define AK8975_REG_RSVC			0x0B
+#define AK8975_REG_ASTC			0x0C
+#define AK8975_REG_TS1			0x0D
+#define AK8975_REG_TS2			0x0E
+#define AK8975_REG_I2CDIS		0x0F
+#define AK8975_REG_ASAX			0x10
+#define AK8975_REG_ASAY			0x11
+#define AK8975_REG_ASAZ			0x12
+
+#define AK8975_MAX_REGS			AK8975_REG_ASAZ
+
+/*
+ * Miscellaneous values.
+ */
+#define AK8975_MAX_CONVERSION_TRIAL	5
+#define AK8975_MAX_CONVERSION_TIMEOUT	500
+#define AK8975_CONVERSION_DONE_POLL_TIME 10
+
+/*
+ * Per-instance context data for the device.
+ */
+struct ak8975_data {
+	struct i2c_client	*client;
+	struct iio_dev		*indio_dev;
+	struct attribute_group  attrs;
+	struct mutex            lock;
+	u8                      asa[3];
+	unsigned long           mode;
+	u8                      reg_cache[AK8975_MAX_REGS];
+	int                     eoc_gpio;
+	int                     eoc_irq;
+};
+
+/*
+ * Helper function to write to the I2C device's registers.
+ */
+static bool ak8975_write_data(struct i2c_client *client,
+			      u8 reg, u8 val, u8 mask, u8 shift)
+{
+	u8 regval;
+	struct i2c_msg msg;
+	u8 w_data[2];
+	int ret = 0;
+
+	struct ak8975_data *data = i2c_get_clientdata(client);
+
+	regval = data->reg_cache[reg];
+	regval &= ~mask;
+	regval |= val << shift;
+
+	w_data[0] = reg;
+	w_data[1] = regval;
+
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = 2;
+	msg.buf = w_data;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret < 0) {
+		dev_err(&client->dev, "Write to device fails status %x\n", ret);
+		return false;
+	}
+	data->reg_cache[reg] = regval;
+
+	return true;
+}
+
+/*
+ * Helper function to read a contiguous set of the I2C device's registers.
+ */
+static bool ak8975_read_data(struct i2c_client *client,
+			     u8 reg, u8 length, u8 *buffer)
+{
+	struct i2c_msg msg[2];
+	u8 w_data[2];
+	int ret = 0;
+
+	w_data[0] = reg;
+
+	msg[0].addr = client->addr;
+	msg[0].flags = I2C_M_NOSTART;	/* set repeated start and write */
+	msg[0].len = 1;
+	msg[0].buf = w_data;
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = length;
+	msg[1].buf = buffer;
+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret < 0) {
+		dev_err(&client->dev, "Read from device fails\n");
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * Perform some start-of-day setup, including reading the asa calibration
+ * values and caching them.
+ */
+static int ak8975_setup(struct i2c_client *client)
+{
+	struct ak8975_data *data = i2c_get_clientdata(client);
+	u8 device_id;
+	u8 buffer[3];
+	bool status;
+
+	/* Confirm that the device we're talking to is really an AK8975. */
+	status = ak8975_read_data(client, AK8975_REG_WIA, 1, &device_id);
+	if ((!status) || (device_id != AK8975_DEVICE_ID)) {
+		dev_err(&client->dev, "Device ak8975 not found\n");
+		return -ENODEV;
+	}
+
+	/* Write the fused rom access mode. */
+	status = ak8975_write_data(client,
+				   AK8975_REG_CNTL,
+				   AK8975_REG_CNTL_MODE_FUSE_ROM,
+				   AK8975_REG_CNTL_MODE_MASK,
+				   AK8975_REG_CNTL_MODE_SHIFT);
+	if (!status) {
+		dev_err(&client->dev, "Error in setting fuse access mode\n");
+		return false;
+	}
+
+	/* Get asa data and store in the device data. */
+	status = ak8975_read_data(client, AK8975_REG_ASAX, 3, buffer);
+	if (!status) {
+		dev_err(&client->dev, "Not able to read asa data\n");
+		return -ENODEV;
+	}
+
+	data->asa[0] = buffer[0] & 0xFF;
+	data->asa[1] = buffer[1] & 0xFF;
+	data->asa[2] = buffer[2] & 0xFF;
+
+	return 0;
+}
+
+/*
+ * Shows the device's mode.  0 = off, 1 = on.
+ */
+static ssize_t show_mode(struct device *dev, struct device_attribute *devattr,
+			 char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ak8975_data *data = indio_dev->dev_data;
+
+	return sprintf(buf, "%lu\n", data->mode);
+}
+
+/*
+ * Sets the device's mode.  0 = off, 1 = on.  The device's mode must be on
+ * for the magn raw attributes to be available.
+ */
+static ssize_t store_mode(struct device *dev, struct device_attribute *devattr,
+			  const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ak8975_data *data = indio_dev->dev_data;
+	struct i2c_client *client = data->client;
+	unsigned long oval;
+	bool status;
+
+	/* Convert mode string and do some basic sanity checking on it.
+	   only 0 or 1 are valid. */
+	if (strict_strtol(buf, 10, &oval))
+		return -EINVAL;
+
+	if ((oval < 0) || (oval > 1)) {
+		dev_err(dev, "mode value is not supported\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->lock);
+
+	/* Write the mode to the device. */
+	if (data->mode != oval) {
+		status = ak8975_write_data(client,
+					   AK8975_REG_CNTL,
+					   (u8)oval,
+					   AK8975_REG_CNTL_MODE_MASK,
+					   AK8975_REG_CNTL_MODE_SHIFT);
+
+		if (!status) {
+			dev_err(&client->dev, "Error in setting mode\n");
+			mutex_unlock(&data->lock);
+			return -EINVAL;
+		}
+		data->mode = oval;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+/*
+ * Emits the ASA sensitivity adjustment value for the x, y, or z axis.
+ * These ASA values are read from the sensor device at start of day, and
+ * cached in the device context struct.
+ */
+static ssize_t show_calibscale(struct device *dev,
+			       struct device_attribute *devattr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ak8975_data *data = indio_dev->dev_data;
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(devattr);
+
+	if (!data) {
+		dev_err(dev, "No device found\n");
+		return -ENODEV;
+	}
+
+	return sprintf(buf, "%d\n", data->asa[this_attr->address]);
+}
+
+/*
+ * Emits the raw flux value for the x, y, or z axis.
+ *
+ * Adjusting the flux value with the sensitivity adjustment value should be
+ * done via the following formula:
+ *
+ * Hadj = H * ( ( ( (ASA-128)*0.5 ) / 128 ) + 1 )
+ *
+ * where H is the raw value, ASA is the sensitivity adjustment, and Hadj
+ * is the resultant adjusted value.
+ */
+static ssize_t show_raw(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ak8975_data *data = indio_dev->dev_data;
+	struct i2c_client *client = data->client;
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(devattr);
+	u32 timeout_ms = AK8975_MAX_CONVERSION_TIMEOUT;
+	u16 meas_reg;
+	s16 raw;
+	u8 read_status;
+	bool status;
+	int state;
+
+	/* Set up the device for taking a sample. */
+	status = ak8975_write_data(client,
+				   AK8975_REG_CNTL,
+				   AK8975_REG_CNTL_MODE_ONCE,
+				   AK8975_REG_CNTL_MODE_MASK,
+				   AK8975_REG_CNTL_MODE_SHIFT);
+	if (!status) {
+		dev_err(&client->dev, "Error in setting operating mode\n");
+		return false;
+	}
+
+	/* Wait for the conversion to complete. */
+	while (timeout_ms) {
+		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
+		state = (gpio_get_value(data->eoc_gpio) ? 1 : 0);
+		if (state)
+			break;
+		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
+	}
+	if (!timeout_ms) {
+		dev_err(&client->dev, "Conversion timeout happend\n");
+		return false;
+	}
+
+	status = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
+	if (!status) {
+		dev_err(&client->dev, "Error in reading ST1\n");
+		return false;
+	}
+
+	if (read_status & AK8975_REG_ST1_DRDY_MASK) {
+		status = ak8975_read_data(client, AK8975_REG_ST2,
+					  1, &read_status);
+		if (!status) {
+			dev_err(&client->dev, "Error in reading ST2\n");
+			return false;
+		}
+		if (read_status & (AK8975_REG_ST2_DERR_MASK |
+				   AK8975_REG_ST2_HOFL_MASK)) {
+			dev_err(&client->dev, "ST2 status error 0x%x\n",
+				read_status);
+			return false;
+		}
+	}
+
+	/* Read the flux value from the appropriate register
+	   (the register is specified in the iio device attributes). */
+	status = ak8975_read_data(client, this_attr->address, 2,
+				  (u8 *)&meas_reg);
+	if (!status) {
+		dev_err(&client->dev, "Read axis data fails\n");
+		return false;
+	}
+
+	/* Endian conversion of the measured values */
+	raw = (s16) (le16_to_cpu(meas_reg));
+
+	return sprintf(buf, "%d\n", raw);
+}
+
+static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, store_mode, 0);
+static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, show_calibscale, NULL, 0);
+static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, show_calibscale, NULL, 1);
+static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, show_calibscale, NULL, 2);
+static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
+static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
+static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
+
+static struct attribute *ak8975_attr[] = {
+	&iio_dev_attr_mode.dev_attr.attr,
+	&iio_dev_attr_magn_x_calibscale.dev_attr.attr,
+	&iio_dev_attr_magn_y_calibscale.dev_attr.attr,
+	&iio_dev_attr_magn_z_calibscale.dev_attr.attr,
+	&iio_dev_attr_magn_x_raw.dev_attr.attr,
+	&iio_dev_attr_magn_y_raw.dev_attr.attr,
+	&iio_dev_attr_magn_z_raw.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group ak8975_attr_group = {
+	.attrs = ak8975_attr,
+};
+
+static int ak8975_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct ak8975_data *data;
+	int err;
+
+	/* Allocate our device context. */
+	data = kzalloc(sizeof(struct ak8975_data), GFP_KERNEL);
+	if (!data) {
+		dev_err(&client->dev, "Memory allocation fails\n");
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(client, data);
+	data->client = client;
+
+	mutex_init(&data->lock);
+
+	/* Grab and set up the supplied GPIO. */
+	data->eoc_irq = client->irq;
+	data->eoc_gpio = irq_to_gpio(client->irq);
+
+	err = gpio_request(data->eoc_gpio, "ak_8975");
+	if (err < 0) {
+		dev_err(&client->dev, "failed to request GPIO %d, error %d\n",
+			data->eoc_gpio, err);
+		goto exit_free;
+	}
+
+	err = gpio_direction_input(data->eoc_gpio);
+	if (err < 0) {
+		dev_err(&client->dev, "Failed to configure input direction for"
+			" GPIO %d, error %d\n", data->eoc_gpio, err);
+		gpio_free(data->eoc_gpio);
+		goto exit_gpio;
+	}
+
+	/* Perform some basic start-of-day setup of the device. */
+	err = ak8975_setup(client);
+	if (err < 0) {
+		dev_err(&client->dev, "AK8975 initialization fails\n");
+		goto exit_gpio;
+	}
+
+	/* Register with IIO */
+	data->indio_dev = iio_allocate_device();
+	if (data->indio_dev == NULL) {
+		err = -ENOMEM;
+		goto exit_gpio;
+	}
+
+	data->indio_dev->dev.parent = &client->dev;
+	data->indio_dev->attrs = &ak8975_attr_group;
+	data->indio_dev->dev_data = (void *)(data);
+	data->indio_dev->driver_module = THIS_MODULE;
+	data->indio_dev->modes = INDIO_DIRECT_MODE;
+
+	err = iio_device_register(data->indio_dev);
+	if (err < 0)
+		goto exit_free_iio;
+
+	return 0;
+
+exit_free_iio:
+	iio_free_device(data->indio_dev);
+exit_gpio:
+	gpio_free(data->eoc_gpio);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int ak8975_remove(struct i2c_client *client)
+{
+	struct ak8975_data *data = i2c_get_clientdata(client);
+
+	iio_device_unregister(data->indio_dev);
+	iio_free_device(data->indio_dev);
+
+	gpio_free(data->eoc_gpio);
+
+	kfree(data);
+
+	return 0;
+}
+
+static const struct i2c_device_id ak8975_id[] = {
+	{"ak8975", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, ak8975_id);
+
+static struct i2c_driver ak8975_driver = {
+	.driver = {
+		.name	= "ak8975",
+	},
+	.probe		= ak8975_probe,
+	.remove		= __devexit_p(ak8975_remove),
+	.id_table	= ak8975_id,
+};
+
+static int __init ak8975_init(void)
+{
+	return i2c_add_driver(&ak8975_driver);
+}
+
+static void __exit ak8975_exit(void)
+{
+	i2c_del_driver(&ak8975_driver);
+}
+
+module_init(ak8975_init);
+module_exit(ak8975_exit);
+
+MODULE_AUTHOR("Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("AK8975 magnetometer driver");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Andrew Chew]

> > +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, 
> store_mode, 0);
> > +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, 
> show_calibscale, NULL, 0);
> > +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, 
> show_calibscale, NULL, 1);
> > +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, 
> show_calibscale, NULL, 2);
> > +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
> > +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
> > +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
> 
> This seems odd as an interface as it's raw when the maths to provide
> non-raw (and thus abstract and easy for user space) data is trivial
> enough to do in kernel

IIO guys want to do normalization maths above the kernel-level magnetometer IIO layer.  This interface came before me, so I'm just following current conventions.--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Alan Cox]

> +/*
> + * Shows the device's mode.  0 = off, 1 = on.
> + */

Should this not be handled by runtime pm nowdays ?

> +	if ((oval < 0) || (oval > 1)) {
> +		dev_err(dev, "mode value is not supported\n");
> +		return -EINVAL;
> +	}

ulong cannot be < 0 and doesn't need all the brackets


> +	/* Wait for the conversion to complete. */
> +	while (timeout_ms) {
> +		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
> +		state = (gpio_get_value(data->eoc_gpio) ? 1 : 0);
> +		if (state)
> +			break;
> +		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
> +	}

This makes some fairly specific wiring assumptions about how the ak8975
is configured. I'm looking at the ak8974 driver in our tree and also
wondering if they can be combined sanely.

> +	status = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
> +	if (!status) {
> +		dev_err(&client->dev, "Error in reading ST1\n");
> +		return false;

I would have expected these to return a meaningful error code not 0 ?

> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, store_mode, 0);
> +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, show_calibscale, NULL, 0);
> +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, show_calibscale, NULL, 1);
> +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, show_calibscale, NULL, 2);
> +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
> +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
> +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);

This seems odd as an interface as it's raw when the maths to provide
non-raw (and thus abstract and easy for user space) data is trivial
enough to do in kernel

(but then I still suspect it should jusst be an input device of course)

> +static int ak8975_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct ak8975_data *data;
> +	int err;
> +
> +	/* Allocate our device context. */
> +	data = kzalloc(sizeof(struct ak8975_data), GFP_KERNEL);
> +	if (!data) {
> +		dev_err(&client->dev, "Memory allocation fails\n");
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	i2c_set_clientdata(client, data);
> +	data->client = client;
> +
> +	mutex_init(&data->lock);
> +
> +	/* Grab and set up the supplied GPIO. */
> +	data->eoc_irq = client->irq;
> +	data->eoc_gpio = irq_to_gpio(client->irq);

It may not be via a GPIO. Better to do the GPIO handling in platform
abstraction or accept passing IRQ and no GPIO value to mean "just use the
IRQ". Ie do all the gpio foo if (data->eoc_gpio) { ... }


> +
> +	err = gpio_request(data->eoc_gpio, "ak_8975");
> +	if (err < 0) {
> +		dev_err(&client->dev, "failed to request GPIO %d, error %d\n",
> +			data->eoc_gpio, err);
> +		goto exit_free;
> +	}
> +
> +	err = gpio_direction_input(data->eoc_gpio);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Failed to configure input direction for"
> +			" GPIO %d, error %d\n", data->eoc_gpio, err);
> +		gpio_free(data->eoc_gpio);

This frees the GPIO twice ?

Looks basically sound to me.

Alan

Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Alan Cox]

On Thu, 2 Sep 2010 15:16:20 -0700
Andrew Chew <AChew@nvidia.com> wrote:

> > > +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, 
> > store_mode, 0);
> > > +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, 
> > show_calibscale, NULL, 0);
> > > +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, 
> > show_calibscale, NULL, 1);
> > > +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, 
> > show_calibscale, NULL, 2);
> > > +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
> > > +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
> > > +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
> > 
> > This seems odd as an interface as it's raw when the maths to provide
> > non-raw (and thus abstract and easy for user space) data is trivial
> > enough to do in kernel
> 
> IIO guys want to do normalization maths above the kernel-level magnetometer IIO layer.  This interface came before me, so I'm just following current conventions.

That will make a generic IIO <-> input bridge very hard to do right. I
can see why IIO wants to do that but you need both if so and this also
needs discussion.

Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Jonathan Cameron]

Before I dive blindly in please note I have only taken a quick look as away from
a computer with a decent size screen until tomorrow evening. I'll take a closer
look at the driver then.
On 09/02/10 23:41, Alan Cox wrote:
> On Thu, 2 Sep 2010 15:16:20 -0700
> Andrew Chew <AChew@nvidia.com> wrote:
> 
>>>> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, 
>>> store_mode, 0);
>>>> +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, 
>>> show_calibscale, NULL, 0);
>>>> +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, 
>>> show_calibscale, NULL, 1);
>>>> +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, 
>>> show_calibscale, NULL, 2);
>>>> +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
>>>> +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
>>>> +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
>>>
>>> This seems odd as an interface as it's raw when the maths to provide
>>> non-raw (and thus abstract and easy for user space) data is trivial
>>> enough to do in kernel
>>
>> IIO guys want to do normalization maths above the kernel-level magnetometer IIO layer. 
>>This interface came before me, so I'm just following current conventions.
> 
> That will make a generic IIO <-> input bridge very hard to do right. I
> can see why IIO wants to do that but you need both if so and this also
> needs discussion.

Note that, though there are few drivers using it (just the tsl2563 iirc)
we do allow the hwmon style _input syntax for values in the standard units
for the device type.  If you are going to use the raw option then,
assuming linear conversion, at the very least magn_scale should be provided
to tell userspace how to do the conversion.  If the bridge to input
is done in userspace then it is trivial to apply this before passing
into uinput.  For background info, the bridge approach currently
relies on implementation of the 'buffered' iio interface which uses
chrdevs rather than the hwmon like 'simple' interface provided here.
It would be a bit of a nightmare to bridge from sysfs (if it could
even be done?)  Typically all drivers start with the sysfs interface
and the buffered one is only added if needed by someone.

Note calibscale is meant for internally applied values.  That is ones applied
actually on the device (looking at what you have here, my choice of naming
wasn't ideal, anyone have a better naming suggestion).  The value used to do raw to
standard unit conversion in userspace is magn_x_scale or equivalent.
I think based on the comment in your code that you also need to provide
magn_x_offset.  Looks like our documentation could do to be a little clearer.
Rather tediously these are both computed floating point values here.  Perhaps
we will need to think further on that interface (this is the first time
we have hit this particular situation)

The raw option is principally there because we share the attributes with the
buffer access (there performance dictates doing as little as possible to the
data on the way through.) Admittedly our overheads are way too high at the
moment for other reasons but the intent is there.  If you are intending to
add this interface later then keeping to the _raw attributes makes for a
more consistent choice (though as long as the scale and bias are there for
the buffer you could use the processed value...)

Thanks,

Jonathan

[PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[achew-DDmLM1+adcrQT0dZR+AlfA]

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

This is for the Asahi Kasei AK8975 3-axis magnetometer.  It resides within
the magnetometer section of the IIO subsystem, and implements the raw
magn_x,y,z attributes, as well as x,y,z factory calibration attributes.

Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/staging/iio/magnetometer/Kconfig  |   11 +
 drivers/staging/iio/magnetometer/Makefile |    1 +
 drivers/staging/iio/magnetometer/ak8975.c |  521 +++++++++++++++++++++++++++++
 3 files changed, 533 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/magnetometer/ak8975.c

diff --git a/drivers/staging/iio/magnetometer/Kconfig b/drivers/staging/iio/magnetometer/Kconfig
index d014450..00e1204 100644
--- a/drivers/staging/iio/magnetometer/Kconfig
+++ b/drivers/staging/iio/magnetometer/Kconfig
@@ -3,6 +3,17 @@
 #
 comment "Magnetometer sensors"
 
+config SENSORS_AK8975
+	tristate "Asahi Kasei AK8975 3-Axis Magnetometer"
+	default n
+	depends on I2C
+	help
+	  Say yes here to build support for Asahi Kasei AK8975 3-Axis
+	  Magnetometer.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called ak8975.
+
 config SENSORS_HMC5843
 	tristate "Honeywell HMC5843 3-Axis Magnetometer"
 	depends on I2C
diff --git a/drivers/staging/iio/magnetometer/Makefile b/drivers/staging/iio/magnetometer/Makefile
index f9bfb2e..e3dbaa4 100644
--- a/drivers/staging/iio/magnetometer/Makefile
+++ b/drivers/staging/iio/magnetometer/Makefile
@@ -2,4 +2,5 @@
 # Makefile for industrial I/O Magnetometer sensors
 #
 
+obj-$(CONFIG_SENSORS_AK8975)	:= ak8975.o
 obj-$(CONFIG_SENSORS_HMC5843)	+= hmc5843.o
diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
new file mode 100644
index 0000000..ca39840
--- /dev/null
+++ b/drivers/staging/iio/magnetometer/ak8975.c
@@ -0,0 +1,521 @@
+/*
+ * A sensor driver for the magnetometer AK8975.
+ *
+ * Magnetic compass sensor driver for monitoring magnetic flux information.
+ *
+ * Copyright (c) 2010, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 Street, Fifth Floor, Boston, MA	02110-1301, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+
+#include <linux/gpio.h>
+
+#include "../iio.h"
+#include "magnet.h"
+
+/*
+ * Register definitions, as well as various shifts and masks to get at the
+ * individual fields of the registers.
+ */
+#define AK8975_REG_WIA			0x00
+#define AK8975_DEVICE_ID		0x48
+
+#define AK8975_REG_INFO			0x01
+
+#define AK8975_REG_ST1			0x02
+#define AK8975_REG_ST1_DRDY_SHIFT	0
+#define AK8975_REG_ST1_DRDY_MASK	(1 << AK8975_REG_ST1_DRDY_SHIFT)
+
+#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_ST2_DERR_SHIFT	2
+#define AK8975_REG_ST2_DERR_MASK	(1 << AK8975_REG_ST2_DERR_SHIFT)
+
+#define AK8975_REG_ST2_HOFL_SHIFT	3
+#define AK8975_REG_ST2_HOFL_MASK	(1 << AK8975_REG_ST2_HOFL_SHIFT)
+
+#define AK8975_REG_CNTL			0x0A
+#define AK8975_REG_CNTL_MODE_SHIFT	0
+#define AK8975_REG_CNTL_MODE_MASK	(0xF << AK8975_REG_CNTL_MODE_SHIFT)
+#define AK8975_REG_CNTL_MODE_POWER_DOWN	0
+#define AK8975_REG_CNTL_MODE_ONCE	1
+#define AK8975_REG_CNTL_MODE_SELF_TEST	8
+#define AK8975_REG_CNTL_MODE_FUSE_ROM	0xF
+
+#define AK8975_REG_RSVC			0x0B
+#define AK8975_REG_ASTC			0x0C
+#define AK8975_REG_TS1			0x0D
+#define AK8975_REG_TS2			0x0E
+#define AK8975_REG_I2CDIS		0x0F
+#define AK8975_REG_ASAX			0x10
+#define AK8975_REG_ASAY			0x11
+#define AK8975_REG_ASAZ			0x12
+
+#define AK8975_MAX_REGS			AK8975_REG_ASAZ
+
+/*
+ * Miscellaneous values.
+ */
+#define AK8975_MAX_CONVERSION_TRIAL	5
+#define AK8975_MAX_CONVERSION_TIMEOUT	500
+#define AK8975_CONVERSION_DONE_POLL_TIME 10
+
+/*
+ * Per-instance context data for the device.
+ */
+struct ak8975_data {
+	struct i2c_client	*client;
+	struct iio_dev		*indio_dev;
+	struct attribute_group  attrs;
+	struct mutex            lock;
+	u8                      asa[3];
+	unsigned long           mode;
+	u8                      reg_cache[AK8975_MAX_REGS];
+	int                     eoc_gpio;
+	int                     eoc_irq;
+};
+
+/*
+ * Helper function to write to the I2C device's registers.
+ */
+static bool ak8975_write_data(struct i2c_client *client,
+			      u8 reg, u8 val, u8 mask, u8 shift)
+{
+	u8 regval;
+	struct i2c_msg msg;
+	u8 w_data[2];
+	int ret = 0;
+
+	struct ak8975_data *data = i2c_get_clientdata(client);
+
+	regval = data->reg_cache[reg];
+	regval &= ~mask;
+	regval |= val << shift;
+
+	w_data[0] = reg;
+	w_data[1] = regval;
+
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = 2;
+	msg.buf = w_data;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret < 0) {
+		dev_err(&client->dev, "Write to device fails status %x\n", ret);
+		return false;
+	}
+	data->reg_cache[reg] = regval;
+
+	return true;
+}
+
+/*
+ * Helper function to read a contiguous set of the I2C device's registers.
+ */
+static bool ak8975_read_data(struct i2c_client *client,
+			     u8 reg, u8 length, u8 *buffer)
+{
+	struct i2c_msg msg[2];
+	u8 w_data[2];
+	int ret = 0;
+
+	w_data[0] = reg;
+
+	msg[0].addr = client->addr;
+	msg[0].flags = I2C_M_NOSTART;	/* set repeated start and write */
+	msg[0].len = 1;
+	msg[0].buf = w_data;
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = length;
+	msg[1].buf = buffer;
+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret < 0) {
+		dev_err(&client->dev, "Read from device fails\n");
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * Perform some start-of-day setup, including reading the asa calibration
+ * values and caching them.
+ */
+static int ak8975_setup(struct i2c_client *client)
+{
+	struct ak8975_data *data = i2c_get_clientdata(client);
+	u8 device_id;
+	u8 buffer[3];
+	bool status;
+
+	/* Confirm that the device we're talking to is really an AK8975. */
+	status = ak8975_read_data(client, AK8975_REG_WIA, 1, &device_id);
+	if ((!status) || (device_id != AK8975_DEVICE_ID)) {
+		dev_err(&client->dev, "Device ak8975 not found\n");
+		return -ENODEV;
+	}
+
+	/* Write the fused rom access mode. */
+	status = ak8975_write_data(client,
+				   AK8975_REG_CNTL,
+				   AK8975_REG_CNTL_MODE_FUSE_ROM,
+				   AK8975_REG_CNTL_MODE_MASK,
+				   AK8975_REG_CNTL_MODE_SHIFT);
+	if (!status) {
+		dev_err(&client->dev, "Error in setting fuse access mode\n");
+		return -EINVAL;
+	}
+
+	/* Get asa data and store in the device data. */
+	status = ak8975_read_data(client, AK8975_REG_ASAX, 3, buffer);
+	if (!status) {
+		dev_err(&client->dev, "Not able to read asa data\n");
+		return -EINVAL;
+	}
+
+	data->asa[0] = buffer[0] & 0xFF;
+	data->asa[1] = buffer[1] & 0xFF;
+	data->asa[2] = buffer[2] & 0xFF;
+
+	return 0;
+}
+
+/*
+ * Shows the device's mode.  0 = off, 1 = on.
+ */
+static ssize_t show_mode(struct device *dev, struct device_attribute *devattr,
+			 char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ak8975_data *data = indio_dev->dev_data;
+
+	return sprintf(buf, "%lu\n", data->mode);
+}
+
+/*
+ * Sets the device's mode.  0 = off, 1 = on.  The device's mode must be on
+ * for the magn raw attributes to be available.
+ */
+static ssize_t store_mode(struct device *dev, struct device_attribute *devattr,
+			  const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ak8975_data *data = indio_dev->dev_data;
+	struct i2c_client *client = data->client;
+	unsigned long oval;
+	bool status;
+
+	/* Convert mode string and do some basic sanity checking on it.
+	   only 0 or 1 are valid. */
+	if (strict_strtoul(buf, 10, &oval))
+		return -EINVAL;
+
+	if (oval > 1) {
+		dev_err(dev, "mode value is not supported\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->lock);
+
+	/* Write the mode to the device. */
+	if (data->mode != oval) {
+		status = ak8975_write_data(client,
+					   AK8975_REG_CNTL,
+					   (u8)oval,
+					   AK8975_REG_CNTL_MODE_MASK,
+					   AK8975_REG_CNTL_MODE_SHIFT);
+
+		if (!status) {
+			dev_err(&client->dev, "Error in setting mode\n");
+			mutex_unlock(&data->lock);
+			return -EINVAL;
+		}
+		data->mode = oval;
+	}
+
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+/*
+ * Emits the ASA sensitivity adjustment value for the x, y, or z axis.
+ * These ASA values are read from the sensor device at start of day, and
+ * cached in the device context struct.
+ */
+static ssize_t show_calibscale(struct device *dev,
+			       struct device_attribute *devattr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ak8975_data *data = indio_dev->dev_data;
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(devattr);
+
+	if (!data) {
+		dev_err(dev, "No device found\n");
+		return -ENODEV;
+	}
+
+	return sprintf(buf, "%d\n", data->asa[this_attr->address]);
+}
+
+/*
+ * Emits the raw flux value for the x, y, or z axis.
+ *
+ * Adjusting the flux value with the sensitivity adjustment value should be
+ * done via the following formula:
+ *
+ * Hadj = H * ( ( ( (ASA-128)*0.5 ) / 128 ) + 1 )
+ *
+ * where H is the raw value, ASA is the sensitivity adjustment, and Hadj
+ * is the resultant adjusted value.
+ */
+static ssize_t show_raw(struct device *dev, struct device_attribute *devattr,
+			char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ak8975_data *data = indio_dev->dev_data;
+	struct i2c_client *client = data->client;
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(devattr);
+	u32 timeout_ms = AK8975_MAX_CONVERSION_TIMEOUT;
+	u16 meas_reg;
+	s16 raw;
+	u8 read_status;
+	bool status;
+	int state;
+
+	/* Set up the device for taking a sample. */
+	status = ak8975_write_data(client,
+				   AK8975_REG_CNTL,
+				   AK8975_REG_CNTL_MODE_ONCE,
+				   AK8975_REG_CNTL_MODE_MASK,
+				   AK8975_REG_CNTL_MODE_SHIFT);
+	if (!status) {
+		dev_err(&client->dev, "Error in setting operating mode\n");
+		return -EINVAL;
+	}
+
+	/* Wait for the conversion to complete. */
+	while (timeout_ms) {
+		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
+		state = (gpio_get_value(data->eoc_gpio) ? 1 : 0);
+		if (state)
+			break;
+		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
+	}
+	if (!timeout_ms) {
+		dev_err(&client->dev, "Conversion timeout happend\n");
+		return -EINVAL;
+	}
+
+	status = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
+	if (!status) {
+		dev_err(&client->dev, "Error in reading ST1\n");
+		return -EINVAL;
+	}
+
+	if (read_status & AK8975_REG_ST1_DRDY_MASK) {
+		status = ak8975_read_data(client, AK8975_REG_ST2,
+					  1, &read_status);
+		if (!status) {
+			dev_err(&client->dev, "Error in reading ST2\n");
+			return -EINVAL;
+		}
+		if (read_status & (AK8975_REG_ST2_DERR_MASK |
+				   AK8975_REG_ST2_HOFL_MASK)) {
+			dev_err(&client->dev, "ST2 status error 0x%x\n",
+				read_status);
+			return -EINVAL;
+		}
+	}
+
+	/* Read the flux value from the appropriate register
+	   (the register is specified in the iio device attributes). */
+	status = ak8975_read_data(client, this_attr->address, 2,
+				  (u8 *)&meas_reg);
+	if (!status) {
+		dev_err(&client->dev, "Read axis data fails\n");
+		return -EINVAL;
+	}
+
+	/* Endian conversion of the measured values */
+	raw = (s16) (le16_to_cpu(meas_reg));
+
+	return sprintf(buf, "%d\n", raw);
+}
+
+static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, store_mode, 0);
+static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, show_calibscale, NULL, 0);
+static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, show_calibscale, NULL, 1);
+static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, show_calibscale, NULL, 2);
+static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
+static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
+static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
+
+static struct attribute *ak8975_attr[] = {
+	&iio_dev_attr_mode.dev_attr.attr,
+	&iio_dev_attr_magn_x_calibscale.dev_attr.attr,
+	&iio_dev_attr_magn_y_calibscale.dev_attr.attr,
+	&iio_dev_attr_magn_z_calibscale.dev_attr.attr,
+	&iio_dev_attr_magn_x_raw.dev_attr.attr,
+	&iio_dev_attr_magn_y_raw.dev_attr.attr,
+	&iio_dev_attr_magn_z_raw.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group ak8975_attr_group = {
+	.attrs = ak8975_attr,
+};
+
+static int ak8975_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct ak8975_data *data;
+	int err;
+
+	/* Allocate our device context. */
+	data = kzalloc(sizeof(struct ak8975_data), GFP_KERNEL);
+	if (!data) {
+		dev_err(&client->dev, "Memory allocation fails\n");
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(client, data);
+	data->client = client;
+
+	mutex_init(&data->lock);
+
+	/* Grab and set up the supplied GPIO. */
+	data->eoc_irq = client->irq;
+	data->eoc_gpio = irq_to_gpio(client->irq);
+
+	if (!data->eoc_gpio) {
+		dev_err(&client->dev, "failed, no valid GPIO\n");
+		goto exit_free;
+	}
+
+	err = gpio_request(data->eoc_gpio, "ak_8975");
+	if (err < 0) {
+		dev_err(&client->dev, "failed to request GPIO %d, error %d\n",
+			data->eoc_gpio, err);
+		goto exit_free;
+	}
+
+	err = gpio_direction_input(data->eoc_gpio);
+	if (err < 0) {
+		dev_err(&client->dev, "Failed to configure input direction for"
+			" GPIO %d, error %d\n", data->eoc_gpio, err);
+		goto exit_gpio;
+	}
+
+	/* Perform some basic start-of-day setup of the device. */
+	err = ak8975_setup(client);
+	if (err < 0) {
+		dev_err(&client->dev, "AK8975 initialization fails\n");
+		goto exit_gpio;
+	}
+
+	/* Register with IIO */
+	data->indio_dev = iio_allocate_device();
+	if (data->indio_dev == NULL) {
+		err = -ENOMEM;
+		goto exit_gpio;
+	}
+
+	data->indio_dev->dev.parent = &client->dev;
+	data->indio_dev->attrs = &ak8975_attr_group;
+	data->indio_dev->dev_data = (void *)(data);
+	data->indio_dev->driver_module = THIS_MODULE;
+	data->indio_dev->modes = INDIO_DIRECT_MODE;
+
+	err = iio_device_register(data->indio_dev);
+	if (err < 0)
+		goto exit_free_iio;
+
+	return 0;
+
+exit_free_iio:
+	iio_free_device(data->indio_dev);
+exit_gpio:
+	gpio_free(data->eoc_gpio);
+exit_free:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int ak8975_remove(struct i2c_client *client)
+{
+	struct ak8975_data *data = i2c_get_clientdata(client);
+
+	iio_device_unregister(data->indio_dev);
+	iio_free_device(data->indio_dev);
+
+	gpio_free(data->eoc_gpio);
+
+	kfree(data);
+
+	return 0;
+}
+
+static const struct i2c_device_id ak8975_id[] = {
+	{"ak8975", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, ak8975_id);
+
+static struct i2c_driver ak8975_driver = {
+	.driver = {
+		.name	= "ak8975",
+	},
+	.probe		= ak8975_probe,
+	.remove		= __devexit_p(ak8975_remove),
+	.id_table	= ak8975_id,
+};
+
+static int __init ak8975_init(void)
+{
+	return i2c_add_driver(&ak8975_driver);
+}
+
+static void __exit ak8975_exit(void)
+{
+	i2c_del_driver(&ak8975_driver);
+}
+
+module_init(ak8975_init);
+module_exit(ak8975_exit);
+
+MODULE_AUTHOR("Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("AK8975 magnetometer driver");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w]

>-----Original Message-----
>From: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-iio-
>owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of ext Alan Cox
>Sent: 03 September, 2010 01:20
>To: achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
>Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org;
>ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
>Subject: Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
>
>> +/*
>> + * Shows the device's mode.  0 = off, 1 = on.
>> + */
>
>Should this not be handled by runtime pm nowdays ?
>
>> +	if ((oval < 0) || (oval > 1)) {
>> +		dev_err(dev, "mode value is not supported\n");
>> +		return -EINVAL;
>> +	}
>
>ulong cannot be < 0 and doesn't need all the brackets
>
>
>> +	/* Wait for the conversion to complete. */
>> +	while (timeout_ms) {
>> +		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
>> +		state = (gpio_get_value(data->eoc_gpio) ? 1 : 0);
>> +		if (state)
>> +			break;
>> +		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
>> +	}
>
>This makes some fairly specific wiring assumptions about how the ak8975
>is configured. I'm looking at the ak8974 driver in our tree and also
>wondering if they can be combined sanely.

With ak8974 chip, it is possible to have similar functionality without interrupt
pin. This is most probably true also for ak8975 chip. It is not good to assume
that everyone uses interrupt pin if the same functionally can be achieved
another way. I mean polling via I2C instead of checking GPIO state after the
sleep. 

Based on the this driver it seems that ak8974 and ak8975 are quite similar, but
also there are many differences like different register map. Maybe combining
these two makes implementation just messy.


>
>> +	status = ak8975_read_data(client, AK8975_REG_ST1, 1,
>&read_status);
>> +	if (!status) {
>> +		dev_err(&client->dev, "Error in reading ST1\n");
>> +		return false;
>
>I would have expected these to return a meaningful error code not 0 ?
>
>> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode,
>store_mode, 0);
>> +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, show_calibscale,
>NULL, 0);
>> +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, show_calibscale,
>NULL, 1);
>> +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, show_calibscale,
>NULL, 2);
>> +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
>> +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
>> +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
>
>This seems odd as an interface as it's raw when the maths to provide
>non-raw (and thus abstract and easy for user space) data is trivial
>enough to do in kernel
>
>(but then I still suspect it should jusst be an input device of course)
>
>> +static int ak8975_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *id)
>> +{
>> +	struct ak8975_data *data;
>> +	int err;
>> +
>> +	/* Allocate our device context. */
>> +	data = kzalloc(sizeof(struct ak8975_data), GFP_KERNEL);
>> +	if (!data) {
>> +		dev_err(&client->dev, "Memory allocation fails\n");
>> +		err = -ENOMEM;
>> +		goto exit;
>> +	}
>> +
>> +	i2c_set_clientdata(client, data);
>> +	data->client = client;
>> +
>> +	mutex_init(&data->lock);
>> +
>> +	/* Grab and set up the supplied GPIO. */
>> +	data->eoc_irq = client->irq;
>> +	data->eoc_gpio = irq_to_gpio(client->irq);
>
>It may not be via a GPIO. Better to do the GPIO handling in platform
>abstraction or accept passing IRQ and no GPIO value to mean "just use
>the
>IRQ". Ie do all the gpio foo if (data->eoc_gpio) { ... }
>
>
>> +
>> +	err = gpio_request(data->eoc_gpio, "ak_8975");
>> +	if (err < 0) {
>> +		dev_err(&client->dev, "failed to request GPIO %d, error
>%d\n",
>> +			data->eoc_gpio, err);
>> +		goto exit_free;
>> +	}
>> +
>> +	err = gpio_direction_input(data->eoc_gpio);
>> +	if (err < 0) {
>> +		dev_err(&client->dev, "Failed to configure input direction
>for"
>> +			" GPIO %d, error %d\n", data->eoc_gpio, err);
>> +		gpio_free(data->eoc_gpio);
>
>This frees the GPIO twice ?
>
>Looks basically sound to me.
>
>Alan
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w]

>-----Original Message-----
>From: ext Jonathan Cameron [mailto:jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org]
>Sent: 03 September, 2010 10:23
>To: Onkalo Samu.P (Nokia-MS/Tampere)
>Cc: alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org; achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org; linux-
>kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org;
>ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
>Subject: Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
>
>On 09/03/10 06:18, samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org wrote:
>>
>>
>>> -----Original Message-----
>>> From: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-iio-
>>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of ext Alan Cox
>>> Sent: 03 September, 2010 01:20
>>> To: achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
>>> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>>> i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org;
>>> ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
>>> Subject: Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
>>>
>>>> +/*
>>>> + * Shows the device's mode.  0 = off, 1 = on.
>>>> + */
>>>
>>> Should this not be handled by runtime pm nowdays ?
>>>
>>>> +	if ((oval < 0) || (oval > 1)) {
>>>> +		dev_err(dev, "mode value is not supported\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> ulong cannot be < 0 and doesn't need all the brackets
>>>
>>>
>>>> +	/* Wait for the conversion to complete. */
>>>> +	while (timeout_ms) {
>>>> +		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
>>>> +		state = (gpio_get_value(data->eoc_gpio) ? 1 : 0);
>>>> +		if (state)
>>>> +			break;
>>>> +		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
>>>> +	}
>>>
>>> This makes some fairly specific wiring assumptions about how the
>ak8975
>>> is configured. I'm looking at the ak8974 driver in our tree and also
>>> wondering if they can be combined sanely.
>>
>> With ak8974 chip, it is possible to have similar functionality without
>interrupt
>> pin. This is most probably true also for ak8975 chip. It is not good
>to assume
>> that everyone uses interrupt pin if the same functionally can be
>achieved
>> another way. I mean polling via I2C instead of checking GPIO state
>after the
>> sleep.
>Of course this can be done, but it's up to Andrew to decide whether he
>wants to.
>I think the usual principal of writing only what people currently need
>applies
>here.  Perhaps a comment in the code to point out this could be done is
>a sensible
>compromise?

Sure. And the one who needs it may freely send patches.

>>
>> Based on the this driver it seems that ak8974 and ak8975 are quite
>similar, but
>> also there are many differences like different register map. Maybe
>combining
>> these two makes implementation just messy.
>>
>>
>>>
>>>> +	status = ak8975_read_data(client, AK8975_REG_ST1, 1,
>>> &read_status);
>>>> +	if (!status) {
>>>> +		dev_err(&client->dev, "Error in reading ST1\n");
>>>> +		return false;
>>>
>>> I would have expected these to return a meaningful error code not 0 ?
>>>
>>>> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode,
>>> store_mode, 0);
>>>> +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, show_calibscale,
>>> NULL, 0);
>>>> +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, show_calibscale,
>>> NULL, 1);
>>>> +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, show_calibscale,
>>> NULL, 2);
>>>> +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
>>>> +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
>>>> +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
>>>
>>> This seems odd as an interface as it's raw when the maths to provide
>>> non-raw (and thus abstract and easy for user space) data is trivial
>>> enough to do in kernel
>>>
>>> (but then I still suspect it should jusst be an input device of
>course)
>>>
>>>> +static int ak8975_probe(struct i2c_client *client,
>>>> +			const struct i2c_device_id *id)
>>>> +{
>>>> +	struct ak8975_data *data;
>>>> +	int err;
>>>> +
>>>> +	/* Allocate our device context. */
>>>> +	data = kzalloc(sizeof(struct ak8975_data), GFP_KERNEL);
>>>> +	if (!data) {
>>>> +		dev_err(&client->dev, "Memory allocation fails\n");
>>>> +		err = -ENOMEM;
>>>> +		goto exit;
>>>> +	}
>>>> +
>>>> +	i2c_set_clientdata(client, data);
>>>> +	data->client = client;
>>>> +
>>>> +	mutex_init(&data->lock);
>>>> +
>>>> +	/* Grab and set up the supplied GPIO. */
>>>> +	data->eoc_irq = client->irq;
>>>> +	data->eoc_gpio = irq_to_gpio(client->irq);
>>>
>>> It may not be via a GPIO. Better to do the GPIO handling in platform
>>> abstraction or accept passing IRQ and no GPIO value to mean "just use
>>> the
>>> IRQ". Ie do all the gpio foo if (data->eoc_gpio) { ... }
>>>
>>>
>>>> +
>>>> +	err = gpio_request(data->eoc_gpio, "ak_8975");
>>>> +	if (err < 0) {
>>>> +		dev_err(&client->dev, "failed to request GPIO %d, error
>>> %d\n",
>>>> +			data->eoc_gpio, err);
>>>> +		goto exit_free;
>>>> +	}
>>>> +
>>>> +	err = gpio_direction_input(data->eoc_gpio);
>>>> +	if (err < 0) {
>>>> +		dev_err(&client->dev, "Failed to configure input direction
>>> for"
>>>> +			" GPIO %d, error %d\n", data->eoc_gpio, err);
>>>> +		gpio_free(data->eoc_gpio);
>>>
>>> This frees the GPIO twice ?
>>>
>>> Looks basically sound to me.
>>>
>>> Alan

Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Jonathan Cameron]

On 09/03/10 06:18, samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org wrote:
> 
> 
>> -----Original Message-----
>> From: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-iio-
>> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of ext Alan Cox
>> Sent: 03 September, 2010 01:20
>> To: achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
>> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>> i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org;
>> ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
>> Subject: Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
>>
>>> +/*
>>> + * Shows the device's mode.  0 = off, 1 = on.
>>> + */
>>
>> Should this not be handled by runtime pm nowdays ?
>>
>>> +	if ((oval < 0) || (oval > 1)) {
>>> +		dev_err(dev, "mode value is not supported\n");
>>> +		return -EINVAL;
>>> +	}
>>
>> ulong cannot be < 0 and doesn't need all the brackets
>>
>>
>>> +	/* Wait for the conversion to complete. */
>>> +	while (timeout_ms) {
>>> +		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
>>> +		state = (gpio_get_value(data->eoc_gpio) ? 1 : 0);
>>> +		if (state)
>>> +			break;
>>> +		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
>>> +	}
>>
>> This makes some fairly specific wiring assumptions about how the ak8975
>> is configured. I'm looking at the ak8974 driver in our tree and also
>> wondering if they can be combined sanely.
> 
> With ak8974 chip, it is possible to have similar functionality without interrupt
> pin. This is most probably true also for ak8975 chip. It is not good to assume
> that everyone uses interrupt pin if the same functionally can be achieved
> another way. I mean polling via I2C instead of checking GPIO state after the
> sleep. 
Of course this can be done, but it's up to Andrew to decide whether he wants to.
I think the usual principal of writing only what people currently need applies
here.  Perhaps a comment in the code to point out this could be done is a sensible
compromise?  
> 
> Based on the this driver it seems that ak8974 and ak8975 are quite similar, but
> also there are many differences like different register map. Maybe combining
> these two makes implementation just messy.
> 
> 
>>
>>> +	status = ak8975_read_data(client, AK8975_REG_ST1, 1,
>> &read_status);
>>> +	if (!status) {
>>> +		dev_err(&client->dev, "Error in reading ST1\n");
>>> +		return false;
>>
>> I would have expected these to return a meaningful error code not 0 ?
>>
>>> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode,
>> store_mode, 0);
>>> +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, show_calibscale,
>> NULL, 0);
>>> +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, show_calibscale,
>> NULL, 1);
>>> +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, show_calibscale,
>> NULL, 2);
>>> +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
>>> +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
>>> +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
>>
>> This seems odd as an interface as it's raw when the maths to provide
>> non-raw (and thus abstract and easy for user space) data is trivial
>> enough to do in kernel
>>
>> (but then I still suspect it should jusst be an input device of course)
>>
>>> +static int ak8975_probe(struct i2c_client *client,
>>> +			const struct i2c_device_id *id)
>>> +{
>>> +	struct ak8975_data *data;
>>> +	int err;
>>> +
>>> +	/* Allocate our device context. */
>>> +	data = kzalloc(sizeof(struct ak8975_data), GFP_KERNEL);
>>> +	if (!data) {
>>> +		dev_err(&client->dev, "Memory allocation fails\n");
>>> +		err = -ENOMEM;
>>> +		goto exit;
>>> +	}
>>> +
>>> +	i2c_set_clientdata(client, data);
>>> +	data->client = client;
>>> +
>>> +	mutex_init(&data->lock);
>>> +
>>> +	/* Grab and set up the supplied GPIO. */
>>> +	data->eoc_irq = client->irq;
>>> +	data->eoc_gpio = irq_to_gpio(client->irq);
>>
>> It may not be via a GPIO. Better to do the GPIO handling in platform
>> abstraction or accept passing IRQ and no GPIO value to mean "just use
>> the
>> IRQ". Ie do all the gpio foo if (data->eoc_gpio) { ... }
>>
>>
>>> +
>>> +	err = gpio_request(data->eoc_gpio, "ak_8975");
>>> +	if (err < 0) {
>>> +		dev_err(&client->dev, "failed to request GPIO %d, error
>> %d\n",
>>> +			data->eoc_gpio, err);
>>> +		goto exit_free;
>>> +	}
>>> +
>>> +	err = gpio_direction_input(data->eoc_gpio);
>>> +	if (err < 0) {
>>> +		dev_err(&client->dev, "Failed to configure input direction
>> for"
>>> +			" GPIO %d, error %d\n", data->eoc_gpio, err);
>>> +		gpio_free(data->eoc_gpio);
>>
>> This frees the GPIO twice ?
>>
>> Looks basically sound to me.
>>
>> Alan

Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Andrew Morton]

On Thu,  2 Sep 2010 16:40:37 -0700
achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org wrote:

> This is for the Asahi Kasei AK8975 3-axis magnetometer.  It resides within
> the magnetometer section of the IIO subsystem, and implements the raw
> magn_x,y,z attributes, as well as x,y,z factory calibration attributes.
> 
> Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/staging/iio/magnetometer/Kconfig  |   11 +
>  drivers/staging/iio/magnetometer/Makefile |    1 +
>  drivers/staging/iio/magnetometer/ak8975.c |  521 +++++++++++++++++++++++++++++
>  3 files changed, 533 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/magnetometer/ak8975.c

This is version 2, yes?

It would be nice to provide some accounting of how it differs from
version 1, for those who already reviewed version 1.  The v1->v2 diff
is below.

--- a/drivers/staging/iio/magnetometer/ak8975.c~a
+++ a/drivers/staging/iio/magnetometer/ak8975.c
@@ -192,14 +192,14 @@ static int ak8975_setup(struct i2c_clien
 				   AK8975_REG_CNTL_MODE_SHIFT);
 	if (!status) {
 		dev_err(&client->dev, "Error in setting fuse access mode\n");
-		return false;
+		return -EINVAL;
 	}
 
 	/* Get asa data and store in the device data. */
 	status = ak8975_read_data(client, AK8975_REG_ASAX, 3, buffer);
 	if (!status) {
 		dev_err(&client->dev, "Not able to read asa data\n");
-		return -ENODEV;
+		return -EINVAL;
 	}
 
 	data->asa[0] = buffer[0] & 0xFF;
@@ -236,10 +236,10 @@ static ssize_t store_mode(struct device 
 
 	/* Convert mode string and do some basic sanity checking on it.
 	   only 0 or 1 are valid. */
-	if (strict_strtol(buf, 10, &oval))
+	if (strict_strtoul(buf, 10, &oval))
 		return -EINVAL;
 
-	if ((oval < 0) || (oval > 1)) {
+	if (oval > 1) {
 		dev_err(dev, "mode value is not supported\n");
 		return -EINVAL;
 	}
@@ -320,7 +320,7 @@ static ssize_t show_raw(struct device *d
 				   AK8975_REG_CNTL_MODE_SHIFT);
 	if (!status) {
 		dev_err(&client->dev, "Error in setting operating mode\n");
-		return false;
+		return -EINVAL;
 	}
 
 	/* Wait for the conversion to complete. */
@@ -333,13 +333,13 @@ static ssize_t show_raw(struct device *d
 	}
 	if (!timeout_ms) {
 		dev_err(&client->dev, "Conversion timeout happend\n");
-		return false;
+		return -EINVAL;
 	}
 
 	status = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
 	if (!status) {
 		dev_err(&client->dev, "Error in reading ST1\n");
-		return false;
+		return -EINVAL;
 	}
 
 	if (read_status & AK8975_REG_ST1_DRDY_MASK) {
@@ -347,13 +347,13 @@ static ssize_t show_raw(struct device *d
 					  1, &read_status);
 		if (!status) {
 			dev_err(&client->dev, "Error in reading ST2\n");
-			return false;
+			return -EINVAL;
 		}
 		if (read_status & (AK8975_REG_ST2_DERR_MASK |
 				   AK8975_REG_ST2_HOFL_MASK)) {
 			dev_err(&client->dev, "ST2 status error 0x%x\n",
 				read_status);
-			return false;
+			return -EINVAL;
 		}
 	}
 
@@ -363,7 +363,7 @@ static ssize_t show_raw(struct device *d
 				  (u8 *)&meas_reg);
 	if (!status) {
 		dev_err(&client->dev, "Read axis data fails\n");
-		return false;
+		return -EINVAL;
 	}
 
 	/* Endian conversion of the measured values */
@@ -418,6 +418,11 @@ static int ak8975_probe(struct i2c_clien
 	data->eoc_irq = client->irq;
 	data->eoc_gpio = irq_to_gpio(client->irq);
 
+	if (!data->eoc_gpio) {
+		dev_err(&client->dev, "failed, no valid GPIO\n");
+		goto exit_free;
+	}
+
 	err = gpio_request(data->eoc_gpio, "ak_8975");
 	if (err < 0) {
 		dev_err(&client->dev, "failed to request GPIO %d, error %d\n",
@@ -429,7 +434,6 @@ static int ak8975_probe(struct i2c_clien
 	if (err < 0) {
 		dev_err(&client->dev, "Failed to configure input direction for"
 			" GPIO %d, error %d\n", data->eoc_gpio, err);
-		gpio_free(data->eoc_gpio);
 		goto exit_gpio;
 	}
 
_

Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Jonathan Cameron]

On 09/03/10 00:40, achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org wrote:
> From: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> This is for the Asahi Kasei AK8975 3-axis magnetometer.  It resides within
> the magnetometer section of the IIO subsystem, and implements the raw
> magn_x,y,z attributes, as well as x,y,z factory calibration attributes.
> 
> Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Hi Andrew,

Looks pretty clean.  A nice little driver.

Anyhow inline you will find:

* A few nitpicking points about eating errors. Lots of drivers in
  kernel do it, but lets not encourage even more.
* Couple of other cleanup suggestions (none of which are vital, but
  might be worth doing if you are respinning anyway)  I'm particularly
  intrigued to know if the sannity check for existence of the device
  can ever fail...

I'll take a final look after you have done the abi related changes.

Thanks,

Jonathan

p.s. As Andrew said, please make it clear which version of a patch
this is, typically make the subject [Patch 1/1 V2] iio: ak8975 ...
And some brief comments to say what has changed from the previous
version are a great time saver for reviewers.


> ---
>  drivers/staging/iio/magnetometer/Kconfig  |   11 +
>  drivers/staging/iio/magnetometer/Makefile |    1 +
>  drivers/staging/iio/magnetometer/ak8975.c |  521 +++++++++++++++++++++++++++++
>  3 files changed, 533 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/magnetometer/ak8975.c
> 
> diff --git a/drivers/staging/iio/magnetometer/Kconfig b/drivers/staging/iio/magnetometer/Kconfig
> index d014450..00e1204 100644
> --- a/drivers/staging/iio/magnetometer/Kconfig
> +++ b/drivers/staging/iio/magnetometer/Kconfig
> @@ -3,6 +3,17 @@
>  #
>  comment "Magnetometer sensors"
>  
> +config SENSORS_AK8975
> +	tristate "Asahi Kasei AK8975 3-Axis Magnetometer"
> +	default n
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Asahi Kasei AK8975 3-Axis
> +	  Magnetometer.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ak8975.
> +
>  config SENSORS_HMC5843
>  	tristate "Honeywell HMC5843 3-Axis Magnetometer"
>  	depends on I2C
> diff --git a/drivers/staging/iio/magnetometer/Makefile b/drivers/staging/iio/magnetometer/Makefile
> index f9bfb2e..e3dbaa4 100644
> --- a/drivers/staging/iio/magnetometer/Makefile
> +++ b/drivers/staging/iio/magnetometer/Makefile
> @@ -2,4 +2,5 @@
>  # Makefile for industrial I/O Magnetometer sensors
>  #
>  
> +obj-$(CONFIG_SENSORS_AK8975)	:= ak8975.o
>  obj-$(CONFIG_SENSORS_HMC5843)	+= hmc5843.o
> diff --git a/drivers/staging/iio/magnetometer/ak8975.c b/drivers/staging/iio/magnetometer/ak8975.c
> new file mode 100644
> index 0000000..ca39840
> --- /dev/null
> +++ b/drivers/staging/iio/magnetometer/ak8975.c
> @@ -0,0 +1,521 @@
> +/*
> + * A sensor driver for the magnetometer AK8975.
> + *
> + * Magnetic compass sensor driver for monitoring magnetic flux information.
> + *
> + * Copyright (c) 2010, NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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 Street, Fifth Floor, Boston, MA	02110-1301, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +
> +#include <linux/gpio.h>
> +
> +#include "../iio.h"
> +#include "magnet.h"
> +
> +/*
> + * Register definitions, as well as various shifts and masks to get at the
> + * individual fields of the registers.
> + */
> +#define AK8975_REG_WIA			0x00
> +#define AK8975_DEVICE_ID		0x48
> +
> +#define AK8975_REG_INFO			0x01
> +
> +#define AK8975_REG_ST1			0x02
> +#define AK8975_REG_ST1_DRDY_SHIFT	0
> +#define AK8975_REG_ST1_DRDY_MASK	(1 << AK8975_REG_ST1_DRDY_SHIFT)
> +
> +#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_ST2_DERR_SHIFT	2
> +#define AK8975_REG_ST2_DERR_MASK	(1 << AK8975_REG_ST2_DERR_SHIFT)
> +
> +#define AK8975_REG_ST2_HOFL_SHIFT	3
> +#define AK8975_REG_ST2_HOFL_MASK	(1 << AK8975_REG_ST2_HOFL_SHIFT)
> +
> +#define AK8975_REG_CNTL			0x0A
> +#define AK8975_REG_CNTL_MODE_SHIFT	0
> +#define AK8975_REG_CNTL_MODE_MASK	(0xF << AK8975_REG_CNTL_MODE_SHIFT)
> +#define AK8975_REG_CNTL_MODE_POWER_DOWN	0
> +#define AK8975_REG_CNTL_MODE_ONCE	1
> +#define AK8975_REG_CNTL_MODE_SELF_TEST	8
> +#define AK8975_REG_CNTL_MODE_FUSE_ROM	0xF
> +
> +#define AK8975_REG_RSVC			0x0B
> +#define AK8975_REG_ASTC			0x0C
> +#define AK8975_REG_TS1			0x0D
> +#define AK8975_REG_TS2			0x0E
> +#define AK8975_REG_I2CDIS		0x0F
> +#define AK8975_REG_ASAX			0x10
> +#define AK8975_REG_ASAY			0x11
> +#define AK8975_REG_ASAZ			0x12
> +
> +#define AK8975_MAX_REGS			AK8975_REG_ASAZ
> +
> +/*
> + * Miscellaneous values.
> + */
> +#define AK8975_MAX_CONVERSION_TRIAL	5
> +#define AK8975_MAX_CONVERSION_TIMEOUT	500
> +#define AK8975_CONVERSION_DONE_POLL_TIME 10
> +
> +/*
> + * Per-instance context data for the device.
> + */
> +struct ak8975_data {
> +	struct i2c_client	*client;
> +	struct iio_dev		*indio_dev;
> +	struct attribute_group  attrs;
> +	struct mutex            lock;
> +	u8                      asa[3];
> +	unsigned long           mode;
> +	u8                      reg_cache[AK8975_MAX_REGS];
> +	int                     eoc_gpio;
> +	int                     eoc_irq;
> +};
> +
> +/*
> + * Helper function to write to the I2C device's registers.
> + */
> +static bool ak8975_write_data(struct i2c_client *client,
> +			      u8 reg, u8 val, u8 mask, u8 shift)
> +{
> +	u8 regval;
> +	struct i2c_msg msg;
> +	u8 w_data[2];
> +	int ret = 0;
> +
> +	struct ak8975_data *data = i2c_get_clientdata(client);
> +
> +	regval = data->reg_cache[reg];
> +	regval &= ~mask;
> +	regval |= val << shift;
> +
> +	w_data[0] = reg;
> +	w_data[1] = regval;
> +
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = 2;
> +	msg.buf = w_data;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Write to device fails status %x\n", ret);
> +		return false;
> +	}
> +	data->reg_cache[reg] = regval;
> +
> +	return true;
> +}
> +
> +/*
> + * Helper function to read a contiguous set of the I2C device's registers.
> + */
> +static bool ak8975_read_data(struct i2c_client *client,
> +			     u8 reg, u8 length, u8 *buffer)
> +{
> +	struct i2c_msg msg[2];
> +	u8 w_data[2];
> +	int ret = 0;
> +
> +	w_data[0] = reg;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = I2C_M_NOSTART;	/* set repeated start and write */
> +	msg[0].len = 1;
> +	msg[0].buf = w_data;
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].len = length;
> +	msg[1].buf = buffer;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Read from device fails\n");
> +		return false;
Ideally you would pass on the actual error code here. Sometimes i2c_transfer
will give more information than simply that it failed.
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * Perform some start-of-day setup, including reading the asa calibration
> + * values and caching them.
> + */
> +static int ak8975_setup(struct i2c_client *client)
> +{
> +	struct ak8975_data *data = i2c_get_clientdata(client);
> +	u8 device_id;
> +	u8 buffer[3];
> +	bool status;
> +
> +	/* Confirm that the device we're talking to is really an AK8975. */
> +	status = ak8975_read_data(client, AK8975_REG_WIA, 1, &device_id);
> +	if ((!status) || (device_id != AK8975_DEVICE_ID)) {
> +		dev_err(&client->dev, "Device ak8975 not found\n");
That's not necessarily the right error. If you had passed on the result
of the i2c_transfter in ak8975_read_data we might have something more specific.
> +		return -ENODEV;
> +	}
> +
> +	/* Write the fused rom access mode. */
> +	status = ak8975_write_data(client,
> +				   AK8975_REG_CNTL,
> +				   AK8975_REG_CNTL_MODE_FUSE_ROM,
> +				   AK8975_REG_CNTL_MODE_MASK,
> +				   AK8975_REG_CNTL_MODE_SHIFT);
> +	if (!status) {
> +		dev_err(&client->dev, "Error in setting fuse access mode\n");
Same point as above.  The underlying code can in theory produce a number of
errors. It's always best to pass up as much information as possible.
> +		return -EINVAL;
> +	}
> +
> +	/* Get asa data and store in the device data. */
> +	status = ak8975_read_data(client, AK8975_REG_ASAX, 3, buffer);
> +	if (!status) {
> +		dev_err(&client->dev, "Not able to read asa data\n");
> +		return -EINVAL;
> +	}
> +
Why do you need & 0xFF, buffer is a u8 so I don't see how any other bits
could be set? I think you could just read directly into data->asa and
skip these copies entirely.
> +	data->asa[0] = buffer[0] & 0xFF;
> +	data->asa[1] = buffer[1] & 0xFF;
> +	data->asa[2] = buffer[2] & 0xFF;
> +
> +	return 0;
> +}
> +
> +/*
> + * Shows the device's mode.  0 = off, 1 = on.
> + */
> +static ssize_t show_mode(struct device *dev, struct device_attribute *devattr,
> +			 char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ak8975_data *data = indio_dev->dev_data;
> +
Using an unsigned long for something that is a boolean value seems a little
unusual... I guess this was to simplify the next function, but simply copying
the unsigned long there into a boolean for the tests would be more conventional.

> +	return sprintf(buf, "%lu\n", data->mode);
> +}
> +
> +/*
> + * Sets the device's mode.  0 = off, 1 = on.  The device's mode must be on
> + * for the magn raw attributes to be available.
> + */
> +static ssize_t store_mode(struct device *dev, struct device_attribute *devattr,
> +			  const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ak8975_data *data = indio_dev->dev_data;
> +	struct i2c_client *client = data->client;
> +	unsigned long oval;
> +	bool status;
> +
> +	/* Convert mode string and do some basic sanity checking on it.
> +	   only 0 or 1 are valid. */
> +	if (strict_strtoul(buf, 10, &oval))
> +		return -EINVAL;
> +
> +	if (oval > 1) {
> +		dev_err(dev, "mode value is not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&data->lock);
> +
> +	/* Write the mode to the device. */
> +	if (data->mode != oval) {
> +		status = ak8975_write_data(client,
> +					   AK8975_REG_CNTL,
> +					   (u8)oval,
> +					   AK8975_REG_CNTL_MODE_MASK,
> +					   AK8975_REG_CNTL_MODE_SHIFT);
> +
> +		if (!status) {
> +			dev_err(&client->dev, "Error in setting mode\n");
> +			mutex_unlock(&data->lock);
> +			return -EINVAL;
> +		}
> +		data->mode = oval;
> +	}
> +
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +/*
> + * Emits the ASA sensitivity adjustment value for the x, y, or z axis.
> + * These ASA values are read from the sensor device at start of day, and
> + * cached in the device context struct.
> + */
> +static ssize_t show_calibscale(struct device *dev,
> +			       struct device_attribute *devattr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ak8975_data *data = indio_dev->dev_data;
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(devattr);
> +
I think this sanity check is overkill.  If there were no device, then the attribute
should never have been pushed.  Have you managed to cause this to ever happen?
> +	if (!data) {
> +		dev_err(dev, "No device found\n");
> +		return -ENODEV;
> +	}
> +
> +	return sprintf(buf, "%d\n", data->asa[this_attr->address]);
> +}
> +
> +/*
> + * Emits the raw flux value for the x, y, or z axis.
> + *
> + * Adjusting the flux value with the sensitivity adjustment value should be
> + * done via the following formula:
> + *
> + * Hadj = H * ( ( ( (ASA-128)*0.5 ) / 128 ) + 1 )
> + *
> + * where H is the raw value, ASA is the sensitivity adjustment, and Hadj
> + * is the resultant adjusted value.
> + */
> +static ssize_t show_raw(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ak8975_data *data = indio_dev->dev_data;
> +	struct i2c_client *client = data->client;
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(devattr);
> +	u32 timeout_ms = AK8975_MAX_CONVERSION_TIMEOUT;
> +	u16 meas_reg;
> +	s16 raw;
> +	u8 read_status;
> +	bool status;
> +	int state;
> +
> +	/* Set up the device for taking a sample. */
> +	status = ak8975_write_data(client,
> +				   AK8975_REG_CNTL,
> +				   AK8975_REG_CNTL_MODE_ONCE,
> +				   AK8975_REG_CNTL_MODE_MASK,
> +				   AK8975_REG_CNTL_MODE_SHIFT);
> +	if (!status) {
> +		dev_err(&client->dev, "Error in setting operating mode\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Wait for the conversion to complete. */
> +	while (timeout_ms) {
> +		msleep(AK8975_CONVERSION_DONE_POLL_TIME);
> +		state = (gpio_get_value(data->eoc_gpio) ? 1 : 0);
This is fine, but personally I'm in favour of,
state = !!gpio_get_value(data->eoc_gpio) which should do the same thing.

Why not just replace the if statement with
if (gpio_get_value(data->eoc_gpio))  - we don't need to enforce the 0 or 1
if we are going to use it like this.

> +		if (state)
> +			break;
> +		timeout_ms -= AK8975_CONVERSION_DONE_POLL_TIME;
> +	}
> +	if (!timeout_ms) {
> +		dev_err(&client->dev, "Conversion timeout happend\n");
> +		return -EINVAL;
> +	}
> +
> +	status = ak8975_read_data(client, AK8975_REG_ST1, 1, &read_status);
> +	if (!status) {
> +		dev_err(&client->dev, "Error in reading ST1\n");
> +		return -EINVAL;
> +	}
> +
> +	if (read_status & AK8975_REG_ST1_DRDY_MASK) {
> +		status = ak8975_read_data(client, AK8975_REG_ST2,
> +					  1, &read_status);
> +		if (!status) {
> +			dev_err(&client->dev, "Error in reading ST2\n");
> +			return -EINVAL;
> +		}
> +		if (read_status & (AK8975_REG_ST2_DERR_MASK |
> +				   AK8975_REG_ST2_HOFL_MASK)) {
> +			dev_err(&client->dev, "ST2 status error 0x%x\n",
> +				read_status);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Read the flux value from the appropriate register
> +	   (the register is specified in the iio device attributes). */
> +	status = ak8975_read_data(client, this_attr->address, 2,
> +				  (u8 *)&meas_reg);
> +	if (!status) {
> +		dev_err(&client->dev, "Read axis data fails\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Endian conversion of the measured values */
> +	raw = (s16) (le16_to_cpu(meas_reg));
> +
> +	return sprintf(buf, "%d\n", raw);
> +}
> +
> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, store_mode, 0);
> +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, show_calibscale, NULL, 0);
> +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, show_calibscale, NULL, 1);
> +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, show_calibscale, NULL, 2);
> +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
> +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
> +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
> +
> +static struct attribute *ak8975_attr[] = {
> +	&iio_dev_attr_mode.dev_attr.attr,
> +	&iio_dev_attr_magn_x_calibscale.dev_attr.attr,
> +	&iio_dev_attr_magn_y_calibscale.dev_attr.attr,
> +	&iio_dev_attr_magn_z_calibscale.dev_attr.attr,
> +	&iio_dev_attr_magn_x_raw.dev_attr.attr,
> +	&iio_dev_attr_magn_y_raw.dev_attr.attr,
> +	&iio_dev_attr_magn_z_raw.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group ak8975_attr_group = {
> +	.attrs = ak8975_attr,
> +};
> +
> +static int ak8975_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct ak8975_data *data;
> +	int err;
> +
> +	/* Allocate our device context. */
> +	data = kzalloc(sizeof(struct ak8975_data), GFP_KERNEL);
> +	if (!data) {
> +		dev_err(&client->dev, "Memory allocation fails\n");
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	i2c_set_clientdata(client, data);
> +	data->client = client;
> +
> +	mutex_init(&data->lock);
> +
> +	/* Grab and set up the supplied GPIO. */
> +	data->eoc_irq = client->irq;
> +	data->eoc_gpio = irq_to_gpio(client->irq);
> +
> +	if (!data->eoc_gpio) {
> +		dev_err(&client->dev, "failed, no valid GPIO\n");
> +		goto exit_free;
> +	}
> +
> +	err = gpio_request(data->eoc_gpio, "ak_8975");
> +	if (err < 0) {
> +		dev_err(&client->dev, "failed to request GPIO %d, error %d\n",
> +			data->eoc_gpio, err);
> +		goto exit_free;
> +	}
> +
> +	err = gpio_direction_input(data->eoc_gpio);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Failed to configure input direction for"
> +			" GPIO %d, error %d\n", data->eoc_gpio, err);
> +		goto exit_gpio;
> +	}
> +
> +	/* Perform some basic start-of-day setup of the device. */
> +	err = ak8975_setup(client);
> +	if (err < 0) {
> +		dev_err(&client->dev, "AK8975 initialization fails\n");
> +		goto exit_gpio;
> +	}
> +
> +	/* Register with IIO */
> +	data->indio_dev = iio_allocate_device();
> +	if (data->indio_dev == NULL) {
> +		err = -ENOMEM;
> +		goto exit_gpio;
> +	}
> +
> +	data->indio_dev->dev.parent = &client->dev;
> +	data->indio_dev->attrs = &ak8975_attr_group;
> +	data->indio_dev->dev_data = (void *)(data);
> +	data->indio_dev->driver_module = THIS_MODULE;
> +	data->indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	err = iio_device_register(data->indio_dev);
> +	if (err < 0)
> +		goto exit_free_iio;
> +
> +	return 0;
> +
> +exit_free_iio:
> +	iio_free_device(data->indio_dev);
> +exit_gpio:
> +	gpio_free(data->eoc_gpio);
> +exit_free:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int ak8975_remove(struct i2c_client *client)
> +{
> +	struct ak8975_data *data = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(data->indio_dev);
> +	iio_free_device(data->indio_dev);
> +
> +	gpio_free(data->eoc_gpio);
> +
> +	kfree(data);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ak8975_id[] = {
> +	{"ak8975", 0},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ak8975_id);
> +
> +static struct i2c_driver ak8975_driver = {
> +	.driver = {
> +		.name	= "ak8975",
> +	},
> +	.probe		= ak8975_probe,
> +	.remove		= __devexit_p(ak8975_remove),
> +	.id_table	= ak8975_id,
> +};
> +
> +static int __init ak8975_init(void)
> +{
> +	return i2c_add_driver(&ak8975_driver);
> +}
> +
> +static void __exit ak8975_exit(void)
> +{
> +	i2c_del_driver(&ak8975_driver);
> +}
> +
> +module_init(ak8975_init);
> +module_exit(ak8975_exit);
> +
> +MODULE_AUTHOR("Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("AK8975 magnetometer driver");
> +MODULE_LICENSE("GPL");

RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Datta, Shubhrajyoti]

> -----Original Message-----
> From: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-iio-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org
> Sent: Friday, September 03, 2010 5:11 AM
> To: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org; ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org;
> Andrew Chew
> Subject: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
> 
> From: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> This is for the Asahi Kasei AK8975 3-axis magnetometer.  It resides within
> the magnetometer section of the IIO subsystem, and implements the raw
> magn_x,y,z attributes, as well as x,y,z factory calibration attributes.
> 
Could you help me with the datasheet details.
> Signed-off-by: Andrew Chew <achew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/staging/iio/magnetometer/Kconfig  |   11 +
>  drivers/staging/iio/magnetometer/Makefile |    1 +
>  drivers/staging/iio/magnetometer/ak8975.c |  521
> +++++++++++++++++++++++++++++
>  3 files changed, 533 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/magnetometer/ak8975.c
> 
> diff --git a/drivers/staging/iio/magnetometer/Kconfig
> b/drivers/staging/iio/magnetometer/Kconfig
> index d014450..00e1204 100644
> --- a/drivers/staging/iio/magnetometer/Kconfig
> +++ b/drivers/staging/iio/magnetometer/Kconfig
> @@ -3,6 +3,17 @@
>  #
>  comment "Magnetometer sensors"
> 
> +config SENSORS_AK8975
> +	tristate "Asahi Kasei AK8975 3-Axis Magnetometer"
> +	default n
Default is anyways n however does no harm.
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Asahi Kasei AK8975 3-Axis
> +	  Magnetometer.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ak8975.
> +
>  config SENSORS_HMC5843
>  	tristate "Honeywell HMC5843 3-Axis Magnetometer"
>  	depends on I2C
> diff --git a/drivers/staging/iio/magnetometer/Makefile
> b/drivers/staging/iio/magnetometer/Makefile
> index f9bfb2e..e3dbaa4 100644
> --- a/drivers/staging/iio/magnetometer/Makefile
> +++ b/drivers/staging/iio/magnetometer/Makefile
> @@ -2,4 +2,5 @@
>  # Makefile for industrial I/O Magnetometer sensors
>  #
> 
> +obj-$(CONFIG_SENSORS_AK8975)	:= ak8975.o

Was the : intentional

>  obj-$(CONFIG_SENSORS_HMC5843)	+= hmc5843.o
> diff --git a/drivers/staging/iio/magnetometer/ak8975.c
> b/drivers/staging/iio/magnetometer/ak8975.c
> new file mode 100644
> index 0000000..ca39840
> --- /dev/null
> +++ b/drivers/staging/iio/magnetometer/ak8975.c
> @@ -0,0 +1,521 @@
> +/*
> + * A sensor driver for the magnetometer AK8975.

RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Andrew Chew]

> > This is for the Asahi Kasei AK8975 3-axis magnetometer.  It 
> resides within
> > the magnetometer section of the IIO subsystem, and 
> implements the raw
> > magn_x,y,z attributes, as well as x,y,z factory calibration 
> attributes.
> > 
> Could you help me with the datasheet details.

What do you mean by this?--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Datta, Shubhrajyoti]

> -----Original Message-----
> From: Andrew Chew [mailto:AChew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org]
> Sent: Wednesday, September 08, 2010 4:37 AM
> To: Datta, Shubhrajyoti; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org; Laxman Dewangan
> Subject: RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
> 
> > > This is for the Asahi Kasei AK8975 3-axis magnetometer.  It
> > resides within
> > > the magnetometer section of the IIO subsystem, and
> > implements the raw
> > > magn_x,y,z attributes, as well as x,y,z factory calibration
> > attributes.
> > >
> > Could you help me with the datasheet details.
> 
> What do you mean by this?
Could you share a link where the datasheet could be downloaded.

RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Murphy, Dan]

Andrew
Do you know if AKM is updating their user space eCompass library to interface with IIO?

Dan

> -----Original Message-----
> From: linux-iio-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-iio-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Datta, Shubhrajyoti
> Sent: Wednesday, September 08, 2010 5:05 AM
> To: Andrew Chew; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org; Laxman Dewangan
> Subject: RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Chew [mailto:AChew-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org]
> > Sent: Wednesday, September 08, 2010 4:37 AM
> > To: Datta, Shubhrajyoti; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> > iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org; khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org; Laxman Dewangan
> > Subject: RE: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor
> >
> > > > This is for the Asahi Kasei AK8975 3-axis magnetometer.  It
> > > resides within
> > > > the magnetometer section of the IIO subsystem, and
> > > implements the raw
> > > > magn_x,y,z attributes, as well as x,y,z factory calibration
> > > attributes.
> > > >
> > > Could you help me with the datasheet details.
> >
> > What do you mean by this?
> Could you share a link where the datasheet could be downloaded.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html