[RFC][PATCH] staging:iio:kxtf9 accelerometer minimal support

[RFC][PATCH] staging:iio:kxtf9 accelerometer minimal support

[Chris Wolfe]

This provides minimal support for a Kionix KXTF9 accelerometer on I2C.

Against staging-next@dc5a189. Fairly RFC as this does not yet use iio_priv or
channels, and does not hold any locks when interacting with the state or device.
In particular for the locking, should I reuse indio->mlock or add state->lock?

Signed-off-by: Chris Wolfe <cwolfe@chromium.org>
---
 drivers/staging/iio/accel/Kconfig  |   10 +
 drivers/staging/iio/accel/Makefile |    1 +
 drivers/staging/iio/accel/kxtf9.c  |  394 ++++++++++++++++++++++++++++++++++++
 3 files changed, 405 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/accel/kxtf9.c

diff --git a/drivers/staging/iio/accel/Kconfig b/drivers/staging/iio/accel/Kconfig
index 81a33b6..b54863a 100644
--- a/drivers/staging/iio/accel/Kconfig
+++ b/drivers/staging/iio/accel/Kconfig
@@ -62,6 +62,16 @@ config KXSD9
 	  Say yes here to build support for the Kionix KXSD9 accelerometer.
 	  Currently this only supports the device via an SPI interface.
 
+config KXTF9
+	tristate "Kionix KXTF9 Accelerometer Driver"
+	depends on I2C
+	help
+	  Say Y here if you have a Kionix KXTF9 accelerometer.
+	  Currently this only supports the device on an I2C interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called kxtf9.
+
 config LIS3L02DQ
 	tristate "ST Microelectronics LIS3L02DQ Accelerometer Driver"
 	depends on SPI
diff --git a/drivers/staging/iio/accel/Makefile b/drivers/staging/iio/accel/Makefile
index 1b2a6d3..8e6d804 100644
--- a/drivers/staging/iio/accel/Makefile
+++ b/drivers/staging/iio/accel/Makefile
@@ -26,6 +26,7 @@ adis16240-$(CONFIG_IIO_RING_BUFFER) += adis16240_ring.o adis16240_trigger.o
 obj-$(CONFIG_ADIS16240) += adis16240.o
 
 obj-$(CONFIG_KXSD9)	+= kxsd9.o
+obj-$(CONFIG_KXTF9)	+= kxtf9.o
 
 lis3l02dq-y		:= lis3l02dq_core.o
 lis3l02dq-$(CONFIG_IIO_RING_BUFFER) += lis3l02dq_ring.o
diff --git a/drivers/staging/iio/accel/kxtf9.c b/drivers/staging/iio/accel/kxtf9.c
new file mode 100644
index 0000000..1a6b0e7
--- /dev/null
+++ b/drivers/staging/iio/accel/kxtf9.c
@@ -0,0 +1,394 @@
+/*
+ * drivers/staging/iio/accel/kxtf9.c
+ *
+ * Copyright (C) 2011 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
+#include "accel.h"
+
+/*** REGISTERS ****************************************************************/
+
+/* These constants are based on the datasheet published by Kionix for the
+ * KXTF9-4100 (Rev 5, March 2011). For more information see:
+ * http://www.kionix.com/accelerometers/accelerometer-KXTF9.html
+ */
+
+#define KXTF9_REG_X			0x06 /* signed le16 */
+#define KXTF9_REG_Y			0x08 /* signed le16 */
+#define KXTF9_REG_Z			0x0A /* signed le16 */
+#define KXTF9_REG_WIA			0x0F
+#define KXTF9_REG_CTRL1			0x1B
+#define KXTF9_REG_CTRL2			0x1C
+#define KXTF9_REG_CTRL3			0x1D
+#define KXTF9_REG_DATA			0x21
+
+#define KXTF9_WIA_EXPECT		0x01
+
+#define KXTF9_CTRL1_ACTIVE		(1<<7)
+#define KXTF9_CTRL1_PRECISE		(1<<6)
+#define KXTF9_CTRL1_RANGE_2G		(0<<3)
+#define KXTF9_CTRL1_RANGE_4G		(1<<3)
+#define KXTF9_CTRL1_RANGE_8G		(2<<3)
+#define KXTF9_CTRL1_RANGE_MASK		(3<<3)
+
+#define KXTF9_CTRL3_RESET		(1<<7)
+
+#define KXTF9_DATA_RATE_25_HZ		(1<<0)
+#define KXTF9_DATA_RATE_50_HZ		(2<<0)
+#define KXTF9_DATA_RATE_100_HZ		(3<<0)
+#define KXTF9_DATA_RATE_200_HZ		(4<<0)
+#define KXTF9_DATA_RATE_400_HZ		(5<<0)
+#define KXTF9_DATA_RATE_800_HZ		(6<<0)
+#define KXTF9_DATA_RATE_MASK		(7<<0)
+
+/*** CONSTANTS ****************************************************************/
+
+/* Depending on the precision mode, the X/Y/Z output registers will contain
+ * either 8 or 12 bits of valid data in their high bits. The low bits are
+ * undefined, so must be zeroed by the driver.
+ *
+ * As a signed 16-bit integer, each range is mapped such that -XG is reported
+ * as -32768 and +XG would be 32768. This yields a scale of 2*X / 65536.
+ * Note that the +XG value can not actually be reported. The data sheet
+ * considers the register values as 8- and 12-bit integers, so shows a
+ * different calculation.
+ */
+
+#define KXTF9_SCALE_2G		"0.00006103515625"
+#define KXTF9_SCALE_4G		"0.0001220703125"
+#define KXTF9_SCALE_8G		"0.000244140625"
+
+/* Determines the time to wait for a reset to complete. */
+#define KXTF9_RESET_MAX_SLEEPS	10
+#define KXTF9_RESET_SLEEP	10
+
+/*** DEVICE STATE *************************************************************/
+
+struct kxtf9 {
+	struct iio_dev *iio;
+	struct i2c_client *i2c;
+
+	u8 cache_ctrl1;
+	u8 cache_ctrl2;
+	u8 cache_ctrl3;
+	u8 cache_data;
+};
+
+/*** ATTRIBUTES ***************************************************************/
+
+static int kxtf9_reset(struct kxtf9 *state);
+
+static inline struct kxtf9 *dev_get_kxtf9(struct device *dev)
+{
+	struct iio_dev *iio = dev_get_drvdata(dev);
+	return iio_dev_get_devdata(iio);
+}
+
+static ssize_t kxtf9_store_reset(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct kxtf9 *state = dev_get_kxtf9(dev);
+	long val;
+	s32 ret;
+
+	ret = strict_strtol(buf, 0, &val);
+	if (ret < 0)
+		goto error;
+
+	if (val != 1) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	ret = kxtf9_reset(state);
+	if (ret < 0)
+		goto error;
+
+	return count;
+error:
+	return ret;
+}
+
+static ssize_t kxtf9_show_accel_raw(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct kxtf9 *state = dev_get_kxtf9(dev);
+	struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
+	bool precise;
+	s32 ret;
+	s16 val;
+
+	ret = i2c_smbus_read_word_data(state->i2c, iio_attr->address);
+	if (ret < 0)
+		goto error;
+
+	precise = (state->cache_ctrl1 & KXTF9_CTRL1_PRECISE) != 0;
+	val = __le16_to_cpu((__le16)(s16)ret);
+	val &= (precise ? 0xFFF0 : 0xFF00);
+
+	return sprintf(buf, "%d\n", (int)val);
+
+error:
+	return ret;
+}
+
+static ssize_t kxtf9_show_accel_scale(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct kxtf9 *state = dev_get_kxtf9(dev);
+	const char *scale;
+
+	switch (state->cache_ctrl1 & KXTF9_CTRL1_RANGE_MASK) {
+	case KXTF9_CTRL1_RANGE_2G:
+		scale = KXTF9_SCALE_2G;
+		break;
+	case KXTF9_CTRL1_RANGE_4G:
+		scale = KXTF9_SCALE_4G;
+		break;
+	case KXTF9_CTRL1_RANGE_8G:
+		scale = KXTF9_SCALE_8G;
+		break;
+	default:
+		return -EIO;
+	}
+
+	return sprintf(buf, "%s\n", scale);
+}
+
+static IIO_CONST_ATTR_NAME("kxtf9");
+static IIO_DEV_ATTR_RESET(kxtf9_store_reset);
+static IIO_DEV_ATTR_ACCEL_SCALE(S_IRUGO, kxtf9_show_accel_scale, NULL, -1);
+static IIO_DEV_ATTR_ACCEL_X(kxtf9_show_accel_raw, KXTF9_REG_X);
+static IIO_DEV_ATTR_ACCEL_Y(kxtf9_show_accel_raw, KXTF9_REG_Y);
+static IIO_DEV_ATTR_ACCEL_Z(kxtf9_show_accel_raw, KXTF9_REG_Z);
+
+static struct attribute *kxtf9_attributes[] = {
+	&iio_const_attr_name.dev_attr.attr,
+	&iio_dev_attr_reset.dev_attr.attr,
+	&iio_dev_attr_accel_x_raw.dev_attr.attr,
+	&iio_dev_attr_accel_y_raw.dev_attr.attr,
+	&iio_dev_attr_accel_z_raw.dev_attr.attr,
+	&iio_dev_attr_accel_scale.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group kxtf9_attribute_group = {
+	.attrs = kxtf9_attributes,
+};
+
+/*** MODULE *******************************************************************/
+
+static int kxtf9_check(struct i2c_client *i2c)
+{
+	s32 ret;
+
+	ret = i2c_smbus_read_byte_data(i2c, KXTF9_REG_WIA);
+	if (ret < 0)
+		return ret;
+
+	if (ret != KXTF9_WIA_EXPECT) {
+		dev_err(&i2c->dev, "device not found; unexpected WIA value\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void kxtf9_default_config(struct kxtf9 *state)
+{
+	state->cache_ctrl1 = KXTF9_CTRL1_ACTIVE | KXTF9_CTRL1_RANGE_2G;
+	state->cache_ctrl2 = 0x00;
+	state->cache_ctrl3 = 0x00;
+
+	/* Possible errata: 25 Hz output rate seems to produce only one sample.
+	 * Use 50 Hz as a default instead. */
+	state->cache_data = KXTF9_DATA_RATE_50_HZ;
+}
+
+/* Write out the configuration when the device is already inactive. */
+static int kxtf9_write_config_inactive(struct kxtf9 *state)
+{
+	s32 ret;
+
+	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL2,
+		state->cache_ctrl2);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL3,
+		state->cache_ctrl3);
+	if (ret < 0)
+		return ret;
+
+	/* Finally write out CTRL1, likely reactivating the device. */
+	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL1,
+		state->cache_ctrl1);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int kxtf9_reset(struct kxtf9 *state)
+{
+	int i;
+	s32 ret;
+
+	dev_info(&state->i2c->dev, "reset\n");
+
+	/* Deactivate the device so it can be reset. */
+	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL1,
+		state->cache_ctrl1 & ~KXTF9_CTRL1_ACTIVE);
+	if (ret < 0)
+		return ret;
+
+	/* Start the reset. */
+	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL3,
+		state->cache_ctrl3 | KXTF9_CTRL3_RESET);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < KXTF9_RESET_MAX_SLEEPS; ++i) {
+		if (i > 0)
+			msleep(KXTF9_RESET_SLEEP);
+
+		ret = i2c_smbus_read_byte_data(state->i2c, KXTF9_REG_CTRL3);
+		if (ret == -EAGAIN)
+			continue;
+		else if (ret < 0)
+			return ret;
+
+		if ((ret & KXTF9_CTRL3_RESET) == 0)
+			break;
+	}
+	if (i == KXTF9_RESET_MAX_SLEEPS) {
+		dev_err(&state->i2c->dev, "timed out while waiting for reset\n");
+		return -ETIME;
+	}
+
+	/* Restore the normal configuration and activation. */
+	return kxtf9_write_config_inactive(state);
+}
+
+static const struct iio_info kxtf9_info = {
+	.attrs = &kxtf9_attribute_group,
+	.driver_module = THIS_MODULE,
+};
+
+static int kxtf9_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct kxtf9 *state;
+	int ret;
+
+	ret = kxtf9_check(client);
+	if (ret < 0)
+		goto error;
+
+	state = kzalloc(sizeof(struct kxtf9), GFP_KERNEL);
+	if (!state) {
+		ret = -ENOMEM;
+		goto error_free;
+	}
+
+	state->i2c = client;
+	i2c_set_clientdata(client, state);
+
+	kxtf9_default_config(state);
+
+	ret = kxtf9_reset(state);
+	if (ret < 0) {
+		dev_err(&state->i2c->dev, "initialization failed\n");
+		goto error_free;
+	}
+
+	state->iio = iio_allocate_device(0);
+	if (!state->iio) {
+		ret = -ENOMEM;
+		goto error_free;
+	}
+
+	state->iio->dev.parent = &client->dev;
+	state->iio->info = &kxtf9_info;
+	state->iio->dev_data = state;
+	state->iio->modes = INDIO_DIRECT_MODE;
+
+	ret = iio_device_register(state->iio);
+	if (ret < 0)
+		goto error_free_iio;
+
+	return 0;
+error_free_iio:
+	iio_free_device(state->iio);
+error_free:
+	i2c_set_clientdata(client, NULL);
+	kfree(state);
+error:
+	return ret;
+}
+
+static int kxtf9_remove(struct i2c_client *client)
+{
+	struct kxtf9 *state = i2c_get_clientdata(client);
+
+	iio_device_unregister(state->iio);
+	iio_free_device(state->iio);
+
+	i2c_set_clientdata(client, NULL);
+	kfree(state);
+	return 0;
+}
+
+/******************************************************************************/
+
+static const struct i2c_device_id kxtf9_id[] = {
+	{ "kxtf9", 0 },
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, kxtf9_id);
+
+static struct i2c_driver kxtf9_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "kxtf9",
+	},
+	.id_table = kxtf9_id,
+	.probe = kxtf9_probe,
+	.remove = kxtf9_remove,
+};
+
+static int __init kxtf9_init(void)
+{
+	return i2c_add_driver(&kxtf9_driver);
+}
+
+static void __exit kxtf9_exit(void)
+{
+	i2c_del_driver(&kxtf9_driver);
+}
+
+module_init(kxtf9_init);
+module_exit(kxtf9_exit);
+
+MODULE_AUTHOR("Chris Wolfe <cwolfe@chromium.org>");
+MODULE_DESCRIPTION("Kionix KXTF9 Accelerometer Driver");
+MODULE_LICENSE("GPL");
-- 
1.7.3.1

Re: [RFC][PATCH] staging:iio:kxtf9 accelerometer minimal support

[Jonathan Cameron]

Hi Chris, looks like you've made a good start.

Couple of general questions first. I've cc'd Chris as his kxtj9 driver
is working it's way through review at the moment (over on linux-input@vger.kernel.org).
I've no idea if these parts are even vaguely similar! I do note
at least some registers are in the same place...

See http://www.spinics.net/lists/linux-input/msg15413.html

There may be some differences, but not clear if there are enough for a completely
separate driver...

First question here is why IIO?

What's the primary use case for this sensor?  The KXSD9 is kind of here
for historical reasons (Dmitry wasn't really taking accelerometers into
input at the time and I happened to have one). I'd have moved that one
over to input by now if there were any actual users other than me ;)

As a general principal, if it's primarily for human input, then it wants
to go into input.  If you are using it for something more 'interesting'
and need for example software buffering then IIO may be the correct location.

> This provides minimal support for a Kionix KXTF9 accelerometer on I2C.
> 
> Against staging-next@dc5a189. Fairly RFC as this does not yet use iio_priv or
> channels, and does not hold any locks when interacting with the state or device.
> In particular for the locking, should I reuse indio->mlock or add state->lock?

In theory at least, mlock was intended for the major state changes for a device
(typically firing up triggered buffer filling).  For protecting registers, a more
fine grained local lock is probably a good idea.

The priv bit will be important as I have a set patch set under review that kills
off the alternative (dev_data pointer).
> 
> Signed-off-by: Chris Wolfe <cwolfe@chromium.org>
> ---
>  drivers/staging/iio/accel/Kconfig  |   10 +
>  drivers/staging/iio/accel/Makefile |    1 +
>  drivers/staging/iio/accel/kxtf9.c  |  394 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 405 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/accel/kxtf9.c
> 
> diff --git a/drivers/staging/iio/accel/Kconfig b/drivers/staging/iio/accel/Kconfig
> index 81a33b6..b54863a 100644
> --- a/drivers/staging/iio/accel/Kconfig
> +++ b/drivers/staging/iio/accel/Kconfig
> @@ -62,6 +62,16 @@ config KXSD9
>  	  Say yes here to build support for the Kionix KXSD9 accelerometer.
>  	  Currently this only supports the device via an SPI interface.
>  
> +config KXTF9
> +	tristate "Kionix KXTF9 Accelerometer Driver"
> +	depends on I2C
> +	help
> +	  Say Y here if you have a Kionix KXTF9 accelerometer.
> +	  Currently this only supports the device on an I2C interface.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called kxtf9.
> +
>  config LIS3L02DQ
>  	tristate "ST Microelectronics LIS3L02DQ Accelerometer Driver"
>  	depends on SPI
> diff --git a/drivers/staging/iio/accel/Makefile b/drivers/staging/iio/accel/Makefile
> index 1b2a6d3..8e6d804 100644
> --- a/drivers/staging/iio/accel/Makefile
> +++ b/drivers/staging/iio/accel/Makefile
> @@ -26,6 +26,7 @@ adis16240-$(CONFIG_IIO_RING_BUFFER) += adis16240_ring.o adis16240_trigger.o
>  obj-$(CONFIG_ADIS16240) += adis16240.o
>  
>  obj-$(CONFIG_KXSD9)	+= kxsd9.o
> +obj-$(CONFIG_KXTF9)	+= kxtf9.o
>  
>  lis3l02dq-y		:= lis3l02dq_core.o
>  lis3l02dq-$(CONFIG_IIO_RING_BUFFER) += lis3l02dq_ring.o
> diff --git a/drivers/staging/iio/accel/kxtf9.c b/drivers/staging/iio/accel/kxtf9.c
> new file mode 100644
> index 0000000..1a6b0e7
> --- /dev/null
> +++ b/drivers/staging/iio/accel/kxtf9.c
> @@ -0,0 +1,394 @@
> +/*
> + * drivers/staging/iio/accel/kxtf9.c
> + *
> + * Copyright (C) 2011 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include "../iio.h"
> +#include "../sysfs.h"
> +#include "accel.h"
> +
> +/*** REGISTERS ****************************************************************/
> +
> +/* These constants are based on the datasheet published by Kionix for the
> + * KXTF9-4100 (Rev 5, March 2011). For more information see:
> + * http://www.kionix.com/accelerometers/accelerometer-KXTF9.html
> + */
> +
> +#define KXTF9_REG_X			0x06 /* signed le16 */
> +#define KXTF9_REG_Y			0x08 /* signed le16 */
> +#define KXTF9_REG_Z			0x0A /* signed le16 */
> +#define KXTF9_REG_WIA			0x0F
> +#define KXTF9_REG_CTRL1			0x1B
> +#define KXTF9_REG_CTRL2			0x1C
> +#define KXTF9_REG_CTRL3			0x1D
> +#define KXTF9_REG_DATA			0x21
> +
> +#define KXTF9_WIA_EXPECT		0x01
> +
> +#define KXTF9_CTRL1_ACTIVE		(1<<7)
> +#define KXTF9_CTRL1_PRECISE		(1<<6)
> +#define KXTF9_CTRL1_RANGE_2G		(0<<3)
> +#define KXTF9_CTRL1_RANGE_4G		(1<<3)
> +#define KXTF9_CTRL1_RANGE_8G		(2<<3)
> +#define KXTF9_CTRL1_RANGE_MASK		(3<<3)
> +
> +#define KXTF9_CTRL3_RESET		(1<<7)
> +
> +#define KXTF9_DATA_RATE_25_HZ		(1<<0)
> +#define KXTF9_DATA_RATE_50_HZ		(2<<0)
> +#define KXTF9_DATA_RATE_100_HZ		(3<<0)
> +#define KXTF9_DATA_RATE_200_HZ		(4<<0)
> +#define KXTF9_DATA_RATE_400_HZ		(5<<0)
> +#define KXTF9_DATA_RATE_800_HZ		(6<<0)
> +#define KXTF9_DATA_RATE_MASK		(7<<0)
> +
> +/*** CONSTANTS ****************************************************************/
> +
> +/* Depending on the precision mode, the X/Y/Z output registers will contain
> + * either 8 or 12 bits of valid data in their high bits. The low bits are
> + * undefined, so must be zeroed by the driver.
> + *
> + * As a signed 16-bit integer, each range is mapped such that -XG is reported
> + * as -32768 and +XG would be 32768. This yields a scale of 2*X / 65536.
> + * Note that the +XG value can not actually be reported. The data sheet
> + * considers the register values as 8- and 12-bit integers, so shows a
> + * different calculation.
> + */
> +
> +#define KXTF9_SCALE_2G		"0.00006103515625"
> +#define KXTF9_SCALE_4G		"0.0001220703125"
> +#define KXTF9_SCALE_8G		"0.000244140625"
> +
> +/* Determines the time to wait for a reset to complete. */
> +#define KXTF9_RESET_MAX_SLEEPS	10
> +#define KXTF9_RESET_SLEEP	10
> +
> +/*** DEVICE STATE *************************************************************/
Clean these general comments out and maybe add kernel-doc comments for
this structure.
> +
> +struct kxtf9 {
> +	struct iio_dev *iio;
> +	struct i2c_client *i2c;
> +
> +	u8 cache_ctrl1;
> +	u8 cache_ctrl2;
> +	u8 cache_ctrl3;
> +	u8 cache_data;
> +};
> +
> +/*** ATTRIBUTES ***************************************************************/
> +
> +static int kxtf9_reset(struct kxtf9 *state);
Hmm.. I'd reorganise the code if possible so this forward definition isn't needed.
> +
> +static inline struct kxtf9 *dev_get_kxtf9(struct device *dev)
> +{
> +	struct iio_dev *iio = dev_get_drvdata(dev);
> +	return iio_dev_get_devdata(iio);
Can combine these two lines.  iio_dev_get_devdata knows it's a struct iio_dev pointer.
> +}
> +
> +static ssize_t kxtf9_store_reset(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct kxtf9 *state = dev_get_kxtf9(dev);
> +	long val;
> +	s32 ret;
> +
> +	ret = strict_strtol(buf, 0, &val);
> +	if (ret < 0)
> +		goto error;
> +
probably use strtobool.  Bit more general for something that
is clearly boolean like here.
> +	if (val != 1) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +
> +	ret = kxtf9_reset(state);
> +	if (ret < 0)
> +		goto error;
> +
> +	return count;
> +error:
> +	return ret;
> +}
> +
> +static ssize_t kxtf9_show_accel_raw(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct kxtf9 *state = dev_get_kxtf9(dev);
> +	struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
> +	bool precise;
> +	s32 ret;
> +	s16 val;
> +
> +	ret = i2c_smbus_read_word_data(state->i2c, iio_attr->address);
> +	if (ret < 0)
> +		goto error;
> +
> +	precise = (state->cache_ctrl1 & KXTF9_CTRL1_PRECISE) != 0;
> +	val = __le16_to_cpu((__le16)(s16)ret);
> +	val &= (precise ? 0xFFF0 : 0xFF00);
I haven't actually read the datasheet properly, but can you do only an 8 bit read
for the 8 bit case?  Always nice not to hammer an i2c bus if possible.
> +
> +	return sprintf(buf, "%d\n", (int)val);
> +
> +error:
> +	return ret;
> +}
> +
> +static ssize_t kxtf9_show_accel_scale(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct kxtf9 *state = dev_get_kxtf9(dev);
> +	const char *scale;
> +
> +	switch (state->cache_ctrl1 & KXTF9_CTRL1_RANGE_MASK) {
> +	case KXTF9_CTRL1_RANGE_2G:
> +		scale = KXTF9_SCALE_2G;
> +		break;
> +	case KXTF9_CTRL1_RANGE_4G:
> +		scale = KXTF9_SCALE_4G;
> +		break;
> +	case KXTF9_CTRL1_RANGE_8G:
> +		scale = KXTF9_SCALE_8G;
> +		break;
> +	default:
> +		return -EIO;
> +	}
> +
> +	return sprintf(buf, "%s\n", scale);
> +}
> +
> +static IIO_CONST_ATTR_NAME("kxtf9");
> +static IIO_DEV_ATTR_RESET(kxtf9_store_reset);
> +static IIO_DEV_ATTR_ACCEL_SCALE(S_IRUGO, kxtf9_show_accel_scale, NULL, -1);
> +static IIO_DEV_ATTR_ACCEL_X(kxtf9_show_accel_raw, KXTF9_REG_X);
> +static IIO_DEV_ATTR_ACCEL_Y(kxtf9_show_accel_raw, KXTF9_REG_Y);
> +static IIO_DEV_ATTR_ACCEL_Z(kxtf9_show_accel_raw, KXTF9_REG_Z);
> +
> +static struct attribute *kxtf9_attributes[] = {
> +	&iio_const_attr_name.dev_attr.attr,
> +	&iio_dev_attr_reset.dev_attr.attr,
> +	&iio_dev_attr_accel_x_raw.dev_attr.attr,
> +	&iio_dev_attr_accel_y_raw.dev_attr.attr,
> +	&iio_dev_attr_accel_z_raw.dev_attr.attr,
> +	&iio_dev_attr_accel_scale.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group kxtf9_attribute_group = {
> +	.attrs = kxtf9_attributes,
> +};
> +
> +/*** MODULE *******************************************************************/
> +
> +static int kxtf9_check(struct i2c_client *i2c)
> +{
> +	s32 ret;
> +
> +	ret = i2c_smbus_read_byte_data(i2c, KXTF9_REG_WIA);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret != KXTF9_WIA_EXPECT) {
> +		dev_err(&i2c->dev, "device not found; unexpected WIA value\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static void kxtf9_default_config(struct kxtf9 *state)
> +{
> +	state->cache_ctrl1 = KXTF9_CTRL1_ACTIVE | KXTF9_CTRL1_RANGE_2G;
> +	state->cache_ctrl2 = 0x00;
> +	state->cache_ctrl3 = 0x00;
> +
> +	/* Possible errata: 25 Hz output rate seems to produce only one sample.
> +	 * Use 50 Hz as a default instead. */
> +	state->cache_data = KXTF9_DATA_RATE_50_HZ;
> +}
> +
> +/* Write out the configuration when the device is already inactive. */
> +static int kxtf9_write_config_inactive(struct kxtf9 *state)
> +{
> +	s32 ret;
> +
> +	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL2,
> +		state->cache_ctrl2);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL3,
> +		state->cache_ctrl3);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Finally write out CTRL1, likely reactivating the device. */
> +	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL1,
> +		state->cache_ctrl1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int kxtf9_reset(struct kxtf9 *state)
> +{
> +	int i;
> +	s32 ret;
> +
> +	dev_info(&state->i2c->dev, "reset\n");
> +
> +	/* Deactivate the device so it can be reset. */
> +	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL1,
> +		state->cache_ctrl1 & ~KXTF9_CTRL1_ACTIVE);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Start the reset. */
> +	ret = i2c_smbus_write_byte_data(state->i2c, KXTF9_REG_CTRL3,
> +		state->cache_ctrl3 | KXTF9_CTRL3_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < KXTF9_RESET_MAX_SLEEPS; ++i) {
> +		if (i > 0)
> +			msleep(KXTF9_RESET_SLEEP);
> +
> +		ret = i2c_smbus_read_byte_data(state->i2c, KXTF9_REG_CTRL3);
> +		if (ret == -EAGAIN)
> +			continue;
> +		else if (ret < 0)
> +			return ret;
> +
> +		if ((ret & KXTF9_CTRL3_RESET) == 0)
> +			break;
> +	}
> +	if (i == KXTF9_RESET_MAX_SLEEPS) {
> +		dev_err(&state->i2c->dev, "timed out while waiting for reset\n");
> +		return -ETIME;
> +	}
> +
> +	/* Restore the normal configuration and activation. */
> +	return kxtf9_write_config_inactive(state);
> +}
> +
> +static const struct iio_info kxtf9_info = {
> +	.attrs = &kxtf9_attribute_group,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int kxtf9_probe(struct i2c_client *client,
> +		const struct i2c_device_id *id)
> +{
> +	struct kxtf9 *state;
> +	int ret;
> +
> +	ret = kxtf9_check(client);
> +	if (ret < 0)
> +		goto error;
> +
> +	state = kzalloc(sizeof(struct kxtf9), GFP_KERNEL);
> +	if (!state) {
> +		ret = -ENOMEM;
> +		goto error_free;
> +	}
> +
> +	state->i2c = client;
> +	i2c_set_clientdata(client, state);
> +
> +	kxtf9_default_config(state);
> +
> +	ret = kxtf9_reset(state);
> +	if (ret < 0) {
> +		dev_err(&state->i2c->dev, "initialization failed\n");
> +		goto error_free;
> +	}
> +
> +	state->iio = iio_allocate_device(0);
> +	if (!state->iio) {
> +		ret = -ENOMEM;
> +		goto error_free;
> +	}
> +
> +	state->iio->dev.parent = &client->dev;
> +	state->iio->info = &kxtf9_info;
> +	state->iio->dev_data = state;
> +	state->iio->modes = INDIO_DIRECT_MODE;
> +
> +	ret = iio_device_register(state->iio);
> +	if (ret < 0)
> +		goto error_free_iio;
> +
> +	return 0;
> +error_free_iio:
> +	iio_free_device(state->iio);
> +error_free:
> +	i2c_set_clientdata(client, NULL);
> +	kfree(state);
> +error:
> +	return ret;
> +}
> +
> +static int kxtf9_remove(struct i2c_client *client)
> +{
> +	struct kxtf9 *state = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(state->iio);
> +	iio_free_device(state->iio);
> +
> +	i2c_set_clientdata(client, NULL);
> +	kfree(state);
> +	return 0;
> +}
> +
> +/******************************************************************************/
> +
> +static const struct i2c_device_id kxtf9_id[] = {
> +	{ "kxtf9", 0 },
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, kxtf9_id);
> +
> +static struct i2c_driver kxtf9_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "kxtf9",
> +	},
> +	.id_table = kxtf9_id,
> +	.probe = kxtf9_probe,
> +	.remove = kxtf9_remove,
> +};
> +
> +static int __init kxtf9_init(void)
> +{
> +	return i2c_add_driver(&kxtf9_driver);
> +}
> +
> +static void __exit kxtf9_exit(void)
> +{
> +	i2c_del_driver(&kxtf9_driver);
> +}
> +
> +module_init(kxtf9_init);
> +module_exit(kxtf9_exit);
> +
> +MODULE_AUTHOR("Chris Wolfe <cwolfe@chromium.org>");
> +MODULE_DESCRIPTION("Kionix KXTF9 Accelerometer Driver");
> +MODULE_LICENSE("GPL");

Re: [RFC][PATCH] staging:iio:kxtf9 accelerometer minimal support

[Chris Wolfe]

On 06/17/11 11:38, Jonathan Cameron wrote:
> Hi Chris, looks like you've made a good start.
>
> Couple of general questions first. I've cc'd Chris as his kxtj9 driver
> is working it's way through review at the moment (over on linux-input@vger.kernel.org).
> I've no idea if these parts are even vaguely similar! I do note
> at least some registers are in the same place...
>
> See http://www.spinics.net/lists/linux-input/msg15413.html
>
> There may be some differences, but not clear if there are enough for a completely
> separate driver...

Thanks, had not noticed the KXTJ9 driver appearing. Will definitely 
coordinate with chudson, as I don't yet know how the two models relate.

> First question here is why IIO?

Well, partially because I found a directory full of accelerometers :)

The big reason this became an IIO driver is because the KXTF9 also 
reports a fair variety of events, including taps, double-taps and 
changes in orientation ("which side is up"). My impression (which may 
well be wrong) is that enabling, configuring and reporting those through 
IIO is easier than stuffing them into input.

My experimental version of this driver uses a trigger on the interrupt, 
with separate poll functions to drive the software ring buffer and 
events. That works well, but needs cleanup and updating (like the 
IIO_EVENT_SH got a no-op callback).

> As a general principal, if it's primarily for human input, then it wants
> to go into input.  If you are using it for something more 'interesting'
> and need for example software buffering then IIO may be the correct location.

I think the software ring buffer and event functionality is useful 
enough to merit staying in IIO. Am well aware this version of the driver 
doesn't use them yet, and there may be good alternatives in input that I 
just missed.

> In theory at least, mlock was intended for the major state changes for a device
> (typically firing up triggered buffer filling).  For protecting registers, a more
> fine grained local lock is probably a good idea.
 >
> The priv bit will be important as I have a set patch set under review that kills
> off the alternative (dev_data pointer).

Got my testing kernel working with an updated IIO, so am switching over 
to the new functions and applying your comments from below.

Incidentally, one of the things I liked about the IIO show was that 
scale functions could report infinite-precision constant strings. That 
seems to have gotten lost in the read_raw change, though I have yet to 
go digging for related discussion in the archives.

Thanks,
Chris