[PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[chris.hudson.comp.eng]

From: Chris Hudson <chudson@kionix.com>

- Added Dmitry's changes
- Now using input_polled_dev interface when in polling mode
- IRQ mode provides a separate sysfs node to change the hardware data rate

Signed-off-by: Chris Hudson <chudson@kionix.com>
---
 drivers/input/misc/Kconfig  |   17 ++
 drivers/input/misc/Makefile |    1 +
 drivers/input/misc/kxtj9.c  |  646 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/input/kxtj9.h |   76 +++++
 4 files changed, 740 insertions(+), 0 deletions(-)
 create mode 100755 drivers/input/misc/kxtj9.c
 create mode 100755 include/linux/input/kxtj9.h

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 45dc6aa..cd6a806 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -478,4 +478,21 @@ config INPUT_XEN_KBDDEV_FRONTEND
 	  To compile this driver as a module, choose M here: the
 	  module will be called xen-kbdfront.
 
+config INPUT_KXTJ9
+       tristate "Kionix KXTJ9 tri-axis digital accelerometer"
+       depends on I2C
+       help
+         If you say yes here you get support for the Kionix KXTJ9 digital
+         tri-axis accelerometer.
+
+         This driver can also be built as a module.  If so, the module
+         will be called kxtj9.
+
+config KXTJ9_POLLED_MODE
+       bool "Enable polling mode support"
+       depends on INPUT_KXTJ9
+       select INPUT_POLLDEV
+       help
+         Say Y here if you need accelerometer to work in polling mode.
+
 endif
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 38efb2c..bfe1057 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_INPUT_DM355EVM)		+= dm355evm_keys.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
 obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)	+= keyspan_remote.o
+obj-$(CONFIG_INPUT_KXTJ9)		+= kxtj9.o
 obj-$(CONFIG_INPUT_M68K_BEEP)		+= m68kspkr.o
 obj-$(CONFIG_INPUT_MAX8925_ONKEY)	+= max8925_onkey.o
 obj-$(CONFIG_INPUT_PCAP)		+= pcap_keys.o
diff --git a/drivers/input/misc/kxtj9.c b/drivers/input/misc/kxtj9.c
index 0000000..7faf155
--- /dev/null
+++ b/drivers/input/misc/kxtj9.c
@@ -0,0 +1,646 @@
+/*
+ * Copyright (C) 2011 Kionix, Inc.
+ * Written by Chris Hudson <chudson@kionix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+ * 02111-1307, USA
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+#include <linux/input/kxtj9.h>
+#include <linux/input-polldev.h>
+
+#define NAME			"kxtj9"
+#define G_MAX			8000
+/* OUTPUT REGISTERS */
+#define XOUT_L			0x06
+#define WHO_AM_I		0x0F
+/* CONTROL REGISTERS */
+#define INT_REL			0x1A
+#define CTRL_REG1		0x1B
+#define INT_CTRL1		0x1E
+#define DATA_CTRL		0x21
+/* CONTROL REGISTER 1 BITS */
+#define PC1_OFF			0x7F
+#define PC1_ON			(1 << 7)
+/* Data ready funtion enable bit: set during probe if using irq mode */
+#define DRDYE			(1 << 5)
+/* INTERRUPT CONTROL REGISTER 1 BITS */
+/* Set these during probe if using irq mode */
+#define KXTJ9_IEL		(1 << 3)
+#define KXTJ9_IEA		(1 << 4)
+#define KXTJ9_IEN		(1 << 5)
+/* INPUT_ABS CONSTANTS */
+#define FUZZ			3
+#define FLAT			3
+/* RESUME STATE INDICES */
+#define RES_DATA_CTRL		0
+#define RES_CTRL_REG1		1
+#define RES_INT_CTRL1		2
+#define RESUME_ENTRIES		3
+
+/*
+ * The following table lists the maximum appropriate poll interval for each
+ * available output data rate.
+ */
+static const struct {
+	unsigned int cutoff;
+	u8 mask;
+} kxtj9_odr_table[] = {
+	{ 3,	ODR800F },
+	{ 5,	ODR400F },
+	{ 10,	ODR200F },
+	{ 20,	ODR100F },
+	{ 40,	ODR50F  },
+	{ 80,	ODR25F  },
+	{ 0,	ODR12_5F},
+};
+
+struct kxtj9_data {
+	struct i2c_client *client;
+	struct kxtj9_platform_data pdata;
+	struct input_dev *input_dev;
+#ifdef CONFIG_KXTJ9_POLLED_MODE
+	struct input_polled_dev *poll_dev;
+#else
+	struct mutex lock;
+#endif
+	unsigned int last_poll_interval;
+	u8 shift;
+	u8 ctrl_reg1;
+	u8 data_ctrl;
+	u8 int_ctrl;
+};
+
+static int kxtj9_i2c_read(struct kxtj9_data *tj9, u8 addr, u8 *data, int len)
+{
+	struct i2c_msg msgs[] = {
+		{
+			.addr = tj9->client->addr,
+			.flags = tj9->client->flags,
+			.len = 1,
+			.buf = &addr,
+		},
+		{
+			.addr = tj9->client->addr,
+			.flags = tj9->client->flags | I2C_M_RD,
+			.len = len,
+			.buf = data,
+		},
+	};
+
+	return i2c_transfer(tj9->client->adapter, msgs, 2);
+}
+
+static void kxtj9_report_acceleration_data(struct kxtj9_data *tj9)
+{
+	s16 acc_data[3]; /* Data bytes from hardware xL, xH, yL, yH, zL, zH */
+	s16 x, y, z;
+	int err;
+
+	err = kxtj9_i2c_read(tj9, XOUT_L, (u8 *)acc_data, 6);
+	if (err < 0)
+		dev_err(&tj9->client->dev, "accelerometer data read failed\n");
+
+	x = le16_to_cpu(acc_data[0]) >> tj9->shift;
+	y = le16_to_cpu(acc_data[1]) >> tj9->shift;
+	z = le16_to_cpu(acc_data[2]) >> tj9->shift;
+
+	input_report_abs(tj9->input_dev, ABS_X, tj9->pdata.negate_x ? -x : x);
+	input_report_abs(tj9->input_dev, ABS_Y, tj9->pdata.negate_y ? -y : y);
+	input_report_abs(tj9->input_dev, ABS_Z, tj9->pdata.negate_x ? -y : y);
+	input_sync(tj9->input_dev);
+}
+
+static int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range)
+{
+	switch (new_g_range) {
+	case KXTJ9_G_2G:
+		tj9->shift = SHIFT_ADJ_2G;
+		break;
+
+	case KXTJ9_G_4G:
+		tj9->shift = SHIFT_ADJ_4G;
+		break;
+
+	case KXTJ9_G_8G:
+		tj9->shift = SHIFT_ADJ_8G;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	tj9->ctrl_reg1 &= 0xe7;
+	tj9->ctrl_reg1 |= new_g_range;
+
+	return 0;
+}
+
+static int kxtj9_update_odr(struct kxtj9_data *tj9, int poll_interval)
+{
+	int err;
+	int i;
+
+	/* Use the lowest ODR that can support the requested poll interval */
+	for (i = 0; i < ARRAY_SIZE(kxtj9_odr_table); i++) {
+		tj9->data_ctrl = kxtj9_odr_table[i].mask;
+		if (poll_interval < kxtj9_odr_table[i].cutoff)
+			break;
+	}
+
+	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, 0);
+	if (err < 0)
+		return err;
+
+	err = i2c_smbus_write_byte_data(tj9->client, DATA_CTRL, tj9->data_ctrl);
+	if (err < 0)
+		return err;
+
+	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, tj9->ctrl_reg1);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int kxtj9_device_power_on(struct kxtj9_data *tj9)
+{
+	int err;
+
+	if (tj9->pdata.power_on) {
+		err = tj9->pdata.power_on();
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
+static void kxtj9_device_power_off(struct kxtj9_data *tj9)
+{
+	int err;
+
+	tj9->ctrl_reg1 &= PC1_OFF;
+	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, tj9->ctrl_reg1);
+	if (err < 0)
+		dev_err(&tj9->client->dev, "soft power off failed\n");
+
+	if (tj9->pdata.power_off)
+		tj9->pdata.power_off();
+}
+
+static int kxtj9_enable(struct kxtj9_data *tj9)
+{
+	int err;
+
+	err = kxtj9_device_power_on(tj9);
+	if (err < 0)
+		return err;
+
+	/* ensure that PC1 is cleared before updating control registers */
+	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, 0);
+	if (err < 0)
+		return err;
+
+	/* only write INT_CTRL_REG1 if in irq mode */
+	if (tj9->client->irq) {
+		err = i2c_smbus_write_byte_data(tj9->client,
+						INT_CTRL1, tj9->int_ctrl);
+		if (err < 0)
+			return err;
+	}
+
+	err = kxtj9_update_g_range(tj9, tj9->pdata.g_range);
+	if (err < 0)
+		return err;
+
+	/* turn on outputs */
+	tj9->ctrl_reg1 |= PC1_ON;
+	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, tj9->ctrl_reg1);
+	if (err < 0)
+		return err;
+
+	err = kxtj9_update_odr(tj9, tj9->last_poll_interval);
+	if (err < 0)
+		return err;
+
+	/* clear initial interrupt if in irq mode */
+	if (tj9->client->irq) {
+		err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
+		if (err < 0) {
+			dev_err(&tj9->client->dev,
+					"error clearing interrupt: %d\n", err);
+			goto fail;
+		}
+	}
+
+	return 0;
+
+fail:
+	kxtj9_device_power_off(tj9);
+	return err;
+}
+
+static void __devinit kxtj9_init_input_device(struct kxtj9_data *tj9,
+						struct input_dev *input_dev)
+{
+	__set_bit(EV_ABS, input_dev->evbit);
+	input_set_abs_params(input_dev, ABS_X, -G_MAX, G_MAX, FUZZ, FLAT);
+	input_set_abs_params(input_dev, ABS_Y, -G_MAX, G_MAX, FUZZ, FLAT);
+	input_set_abs_params(input_dev, ABS_Z, -G_MAX, G_MAX, FUZZ, FLAT);
+
+	input_dev->name = "kxtj9_accel";
+	input_dev->id.bustype = BUS_I2C;
+	input_dev->dev.parent = &tj9->client->dev;
+}
+
+#ifdef CONFIG_KXTJ9_POLLED_MODE
+static void kxtj9_poll(struct input_polled_dev *dev)
+{
+	struct kxtj9_data *tj9 = dev->private;
+	unsigned int poll_interval = dev->poll_interval;
+
+	kxtj9_report_acceleration_data(tj9);
+
+	if (poll_interval != tj9->last_poll_interval) {
+		kxtj9_update_odr(tj9, poll_interval);
+		tj9->last_poll_interval = poll_interval;
+	}
+}
+
+static void kxtj9_polled_input_open(struct input_polled_dev *dev)
+{
+	struct kxtj9_data *tj9 = dev->private;
+
+	kxtj9_enable(tj9);
+}
+
+static void kxtj9_polled_input_close(struct input_polled_dev *dev)
+{
+	struct kxtj9_data *tj9 = dev->private;
+
+	kxtj9_device_power_off(tj9);
+}
+
+static int __devinit kxtj9_setup_polled_device(struct kxtj9_data *tj9)
+{
+	int err;
+	struct input_polled_dev *poll_dev;
+	poll_dev = input_allocate_polled_device();
+
+	if (!poll_dev) {
+		dev_err(&tj9->client->dev,
+			"Failed to allocate polled device\n");
+		return -ENOMEM;
+	}
+
+	tj9->poll_dev = poll_dev;
+	tj9->input_dev = poll_dev->input;
+
+	poll_dev->private = tj9;
+	poll_dev->poll = kxtj9_poll;
+	poll_dev->open = kxtj9_polled_input_open;
+	poll_dev->close = kxtj9_polled_input_close;
+
+	kxtj9_init_input_device(tj9, poll_dev->input);
+
+	err = input_register_polled_device(poll_dev);
+	if (err) {
+		dev_err(&tj9->client->dev,
+			"Unable to register polled device, err=%d\n", err);
+		input_free_polled_device(poll_dev);
+		return 0;
+	}
+
+	return 0;
+}
+
+static void kxtj9_teardown_polled_device(struct kxtj9_data *tj9)
+{
+	input_unregister_polled_device(tj9->poll_dev);
+	input_free_polled_device(tj9->poll_dev);
+}
+
+#else	/* IRQ Mode is enabled */
+static int kxtj9_input_open(struct input_dev *input)
+{
+	struct kxtj9_data *tj9 = input_get_drvdata(input);
+
+	return kxtj9_enable(tj9);
+}
+
+static void kxtj9_input_close(struct input_dev *dev)
+{
+	struct kxtj9_data *tj9 = input_get_drvdata(dev);
+
+	kxtj9_device_power_off(tj9);
+}
+
+static int __devinit kxtj9_setup_input_device(struct kxtj9_data *tj9)
+{
+	struct input_dev *input_dev;
+	int err;
+
+	input_dev = input_allocate_device();
+	if (!tj9->input_dev) {
+		dev_err(&tj9->client->dev, "input device allocate failed\n");
+		return -ENOMEM;
+	}
+
+	tj9->input_dev = input_dev;
+
+	input_dev->open = kxtj9_input_open;
+	input_dev->close = kxtj9_input_close;
+	input_set_drvdata(input_dev, tj9);
+
+	kxtj9_init_input_device(tj9, input_dev);
+
+	err = input_register_device(tj9->input_dev);
+	if (err) {
+		dev_err(&tj9->client->dev,
+			"unable to register input polled device %s: %d\n",
+			tj9->input_dev->name, err);
+		input_free_device(tj9->input_dev);
+	}
+
+	return 0;
+}
+
+static irqreturn_t kxtj9_isr(int irq, void *dev)
+{
+	struct kxtj9_data *tj9 = dev;
+	int err;
+
+	/* data ready is the only possible interrupt type */
+	kxtj9_report_acceleration_data(tj9);
+
+	err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
+	if (err < 0)
+		dev_err(&tj9->client->dev,
+				"error clearing interrupt status: %d\n", err);
+
+	return IRQ_HANDLED;
+}
+
+static inline int kxtj9_setup_polled_device(struct kxtj9_data *tj9)
+{
+	return -ENOSYS;
+}
+
+static inline void kxtj9_teardown_polled_device(struct kxtj9_data *tj9)
+{
+}
+
+/* kxtj9_get_poll: returns the currently selected poll interval (in ms) */
+static ssize_t kxtj9_get_poll(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
+	return sprintf(buf, "%d\n", tj9->last_poll_interval);
+}
+
+/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
+static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
+						const char *buf, size_t count)
+{
+	struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
+
+	unsigned long interval;
+	int ret = kstrtoul(buf, 10, &interval);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&tj9->lock);
+	/* set current interval to the greater of the minimum interval or
+	the requested interval */
+	tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
+	mutex_unlock(&tj9->lock);
+
+	kxtj9_update_odr(tj9, tj9->last_poll_interval);
+
+	return count;
+}
+/* When IRQ mode is selected, we need to provide an interface to allow the user
+ * to change the output data rate of the part.  For consistency, we are using
+ * the set_poll method, which accepts a poll interval in milliseconds, and then
+ * calls update_odr() while passing this value as an argument.  In IRQ mode, the
+ * data outputs will not be read AT the requested poll interval, rather, the
+ * lowest ODR that can support the requested interval.  The client application
+ * will be responsible for retrieving data from the input node at the desired
+ * interval.
+ */
+static DEVICE_ATTR(poll, S_IRUGO|S_IWUSR, kxtj9_get_poll, kxtj9_set_poll);
+
+static struct attribute *kxtj9_attributes[] = {
+	&dev_attr_poll.attr,
+	NULL
+};
+
+static struct attribute_group kxtj9_attribute_group = {
+	.attrs = kxtj9_attributes
+};
+#endif
+
+static int __devinit kxtj9_verify(struct kxtj9_data *tj9)
+{
+	int retval;
+	retval = kxtj9_device_power_on(tj9);
+	if (retval < 0)
+		return retval;
+	retval = i2c_smbus_read_byte_data(tj9->client, WHO_AM_I);
+	if (retval < 0) {
+		dev_err(&tj9->client->dev, "read err int source\n");
+		goto out;
+	}
+	retval = retval != 0x06 ? -EIO : 0;
+
+out:
+	kxtj9_device_power_off(tj9);
+	return retval;
+}
+
+static int __devinit kxtj9_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	const struct kxtj9_platform_data *pdata = client->dev.platform_data;
+	struct kxtj9_data *tj9;
+	int err;
+
+	if (!i2c_check_functionality(client->adapter,
+				I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE_DATA)) {
+		dev_err(&client->dev, "client is not i2c capable\n");
+		return -ENXIO;
+	}
+	if (!pdata) {
+		dev_err(&client->dev, "platform data is NULL; exiting\n");
+		return -EINVAL;
+	}
+	tj9 = kzalloc(sizeof(*tj9), GFP_KERNEL);
+	if (!tj9) {
+		dev_err(&client->dev,
+			"failed to allocate memory for module data\n");
+		return -ENOMEM;
+	}
+
+	tj9->client = client;
+	tj9->pdata = *pdata;
+
+	if (pdata->init) {
+		err = pdata->init();
+		if (err < 0)
+			goto err_free_mem;
+	}
+
+	err = kxtj9_verify(tj9);
+	if (err < 0) {
+		dev_err(&client->dev, "device not recognized\n");
+		goto err_pdata_exit;
+	}
+
+	tj9->ctrl_reg1 = tj9->pdata.res_12bit | tj9->pdata.g_range;
+	tj9->data_ctrl = tj9->pdata.data_odr_init;
+
+#ifdef CONFIG_KXTJ9_POLLED_MODE
+	err = kxtj9_setup_polled_device(tj9);
+	if (err)
+		goto err_pdata_exit;
+#else
+	/* If in irq mode, populate INT_CTRL_REG1 and enable DRDY. */
+	tj9->int_ctrl |= KXTJ9_IEN | KXTJ9_IEA | KXTJ9_IEL;
+	tj9->ctrl_reg1 |= DRDYE;
+
+	err = kxtj9_setup_input_device(tj9);
+	if (err)
+		goto err_pdata_exit;
+
+	err = request_threaded_irq(client->irq, NULL, kxtj9_isr,
+				IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+				"kxtj9-irq", tj9);
+	if (err) {
+		dev_err(&client->dev, "request irq failed: %d\n", err);
+		goto err_destroy_input;
+	}
+	err = sysfs_create_group(&client->dev.kobj, &kxtj9_attribute_group);
+	if (err) {
+		dev_err(&client->dev, "sysfs create failed: %d\n", err);
+		goto err_destroy_input;
+	}
+#endif
+	i2c_set_clientdata(client, tj9);
+	return 0;
+
+#ifndef CONFIG_KXTJ9_POLLED_MODE
+err_destroy_input:
+	input_unregister_device(tj9->input_dev);
+#endif
+err_pdata_exit:
+	if (tj9->pdata.exit)
+		tj9->pdata.exit();
+err_free_mem:
+	kfree(tj9);
+	return err;
+}
+
+static int __devexit kxtj9_remove(struct i2c_client *client)
+{
+	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
+
+#ifdef CONFIG_KXTJ9_POLLED_MODE
+	kxtj9_teardown_polled_device(tj9);
+#else
+	sysfs_remove_group(&client->dev.kobj, &kxtj9_attribute_group);
+	free_irq(client->irq, tj9);
+	input_unregister_device(tj9->input_dev);
+#endif
+	if (tj9->pdata.exit)
+		tj9->pdata.exit();
+
+	kfree(tj9);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int kxtj9_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
+	struct input_dev *input_dev = tj9->input_dev;
+
+	mutex_lock(&input_dev->mutex);
+
+	if (input_dev->users)
+		kxtj9_device_power_off(tj9);
+
+	mutex_unlock(&input_dev->mutex);
+	return 0;
+}
+
+static int kxtj9_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
+	struct input_dev *input_dev = tj9->input_dev;
+	int retval = 0;
+
+	mutex_lock(&input_dev->mutex);
+
+	if (input_dev->users)
+		kxtj9_enable(tj9);
+
+	mutex_unlock(&input_dev->mutex);
+	return retval;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(kxtj9_pm_ops, kxtj9_suspend, kxtj9_resume);
+
+static const struct i2c_device_id kxtj9_id[] = {
+	{ NAME, 0 },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(i2c, kxtj9_id);
+
+static struct i2c_driver kxtj9_driver = {
+	.driver = {
+		  .name = NAME,
+		  .owner = THIS_MODULE,
+		  .pm = &kxtj9_pm_ops,
+	},
+	.probe = kxtj9_probe,
+	.remove = __devexit_p(kxtj9_remove),
+	.id_table = kxtj9_id,
+};
+
+static int __init kxtj9_init(void)
+{
+	return i2c_add_driver(&kxtj9_driver);
+}
+module_init(kxtj9_init);
+
+static void __exit kxtj9_exit(void)
+{
+	i2c_del_driver(&kxtj9_driver);
+}
+module_exit(kxtj9_exit);
+
+MODULE_DESCRIPTION("KXTJ9 accelerometer driver");
+MODULE_AUTHOR("Chris Hudson <chudson@kionix.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/input/kxtj9.h b/include/linux/input/kxtj9.h
index 0000000..cc5928b
--- /dev/null
+++ b/include/linux/input/kxtj9.h
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) 2011 Kionix, Inc.
+ * Written by Chris Hudson <chudson@kionix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+ * 02111-1307, USA
+ */
+
+#ifndef __KXTJ9_H__
+#define __KXTJ9_H__
+
+#define KXTJ9_I2C_ADDR		0x0F
+
+/* These shift values are used to provide consistent output data */
+#define SHIFT_ADJ_2G		4
+#define SHIFT_ADJ_4G		3
+#define SHIFT_ADJ_8G		2
+
+#ifdef __KERNEL__
+struct kxtj9_platform_data {
+	int poll_interval;	/* current poll interval (in milli-seconds) */
+	int min_interval;	/* minimum poll interval (in milli-seconds) */
+
+	/* by default, x is axis 0, y is axis 1, z is axis 2; these can be
+	changed to account for sensor orientation within the host device */
+	u8 axis_map_x;
+	u8 axis_map_y;
+	u8 axis_map_z;
+
+	/* each axis can be negated to account for sensor orientation within
+	the host device; 1 = negate this axis; 0 = do not negate this axis */
+	bool negate_x;
+	bool negate_y;
+	bool negate_z;
+
+	/* CTRL_REG1: set resolution, g-range, data ready enable */
+	/* Output resolution: 8-bit valid or 12-bit valid */
+	#define RES_8BIT		0
+	#define RES_12BIT		(1 << 6)
+	u8 res_12bit;
+	/* Output g-range: +/-2g, 4g, or 8g */
+	#define KXTJ9_G_2G		0
+	#define KXTJ9_G_4G		(1 << 3)
+	#define KXTJ9_G_8G		(1 << 4)
+	u8 g_range;
+
+	/* DATA_CTRL_REG: controls the output data rate of the part */
+	#define ODR12_5F		0
+	#define ODR25F			1
+	#define ODR50F			2
+	#define ODR100F			3
+	#define ODR200F			4
+	#define ODR400F			5
+	#define ODR800F			6
+	u8 data_odr_init;
+
+	int (*init)(void);
+	void (*exit)(void);
+	int (*power_on)(void);
+	int (*power_off)(void);
+};
+#endif /* __KERNEL__ */
+
+#endif  /* __KXTJ9_H__ */
+
-- 
1.5.4.3

Re: [PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[Jonathan Cameron]

On 06/28/11 20:12, chris.hudson.comp.eng@gmail.com wrote:
> From: Chris Hudson <chudson@kionix.com>
> 
> - Added Dmitry's changes
> - Now using input_polled_dev interface when in polling mode
> - IRQ mode provides a separate sysfs node to change the hardware data rate
Hi Chris,

Few minor comments inline.
> 
> Signed-off-by: Chris Hudson <chudson@kionix.com>
> ---
>  drivers/input/misc/Kconfig  |   17 ++
>  drivers/input/misc/Makefile |    1 +
>  drivers/input/misc/kxtj9.c  |  646 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/input/kxtj9.h |   76 +++++
>  4 files changed, 740 insertions(+), 0 deletions(-)
>  create mode 100755 drivers/input/misc/kxtj9.c
>  create mode 100755 include/linux/input/kxtj9.h
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 45dc6aa..cd6a806 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -478,4 +478,21 @@ config INPUT_XEN_KBDDEV_FRONTEND
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called xen-kbdfront.
>  
> +config INPUT_KXTJ9
> +       tristate "Kionix KXTJ9 tri-axis digital accelerometer"
> +       depends on I2C
> +       help
> +         If you say yes here you get support for the Kionix KXTJ9 digital
> +         tri-axis accelerometer.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called kxtj9.
> +
> +config KXTJ9_POLLED_MODE
> +       bool "Enable polling mode support"
> +       depends on INPUT_KXTJ9
> +       select INPUT_POLLDEV
> +       help
> +         Say Y here if you need accelerometer to work in polling mode.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 38efb2c..bfe1057 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_INPUT_DM355EVM)		+= dm355evm_keys.o
>  obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
>  obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
>  obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)	+= keyspan_remote.o
> +obj-$(CONFIG_INPUT_KXTJ9)		+= kxtj9.o
>  obj-$(CONFIG_INPUT_M68K_BEEP)		+= m68kspkr.o
>  obj-$(CONFIG_INPUT_MAX8925_ONKEY)	+= max8925_onkey.o
>  obj-$(CONFIG_INPUT_PCAP)		+= pcap_keys.o
> diff --git a/drivers/input/misc/kxtj9.c b/drivers/input/misc/kxtj9.c
> index 0000000..7faf155
> --- /dev/null
> +++ b/drivers/input/misc/kxtj9.c
> @@ -0,0 +1,646 @@
> +/*
> + * Copyright (C) 2011 Kionix, Inc.
> + * Written by Chris Hudson <chudson@kionix.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> + * 02111-1307, USA
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/slab.h>
> +#include <linux/input/kxtj9.h>
> +#include <linux/input-polldev.h>
> +
> +#define NAME			"kxtj9"
> +#define G_MAX			8000
> +/* OUTPUT REGISTERS */
> +#define XOUT_L			0x06
> +#define WHO_AM_I		0x0F
> +/* CONTROL REGISTERS */
> +#define INT_REL			0x1A
> +#define CTRL_REG1		0x1B
> +#define INT_CTRL1		0x1E
> +#define DATA_CTRL		0x21
> +/* CONTROL REGISTER 1 BITS */
> +#define PC1_OFF			0x7F
> +#define PC1_ON			(1 << 7)
> +/* Data ready funtion enable bit: set during probe if using irq mode */
> +#define DRDYE			(1 << 5)
> +/* INTERRUPT CONTROL REGISTER 1 BITS */
> +/* Set these during probe if using irq mode */
> +#define KXTJ9_IEL		(1 << 3)
> +#define KXTJ9_IEA		(1 << 4)
> +#define KXTJ9_IEN		(1 << 5)
> +/* INPUT_ABS CONSTANTS */
> +#define FUZZ			3
> +#define FLAT			3
> +/* RESUME STATE INDICES */
> +#define RES_DATA_CTRL		0
> +#define RES_CTRL_REG1		1
> +#define RES_INT_CTRL1		2
> +#define RESUME_ENTRIES		3
> +
> +/*
> + * The following table lists the maximum appropriate poll interval for each
> + * available output data rate.
> + */
> +static const struct {
> +	unsigned int cutoff;
> +	u8 mask;
> +} kxtj9_odr_table[] = {
> +	{ 3,	ODR800F },
> +	{ 5,	ODR400F },
> +	{ 10,	ODR200F },
> +	{ 20,	ODR100F },
> +	{ 40,	ODR50F  },
> +	{ 80,	ODR25F  },
> +	{ 0,	ODR12_5F},
> +};
> +
> +struct kxtj9_data {
> +	struct i2c_client *client;
> +	struct kxtj9_platform_data pdata;
> +	struct input_dev *input_dev;
> +#ifdef CONFIG_KXTJ9_POLLED_MODE
> +	struct input_polled_dev *poll_dev;
> +#else
> +	struct mutex lock;
> +#endif
> +	unsigned int last_poll_interval;
> +	u8 shift;
> +	u8 ctrl_reg1;
> +	u8 data_ctrl;
> +	u8 int_ctrl;
> +};
> +
> +static int kxtj9_i2c_read(struct kxtj9_data *tj9, u8 addr, u8 *data, int len)
> +{
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr = tj9->client->addr,
> +			.flags = tj9->client->flags,
> +			.len = 1,
> +			.buf = &addr,
> +		},
> +		{
> +			.addr = tj9->client->addr,
> +			.flags = tj9->client->flags | I2C_M_RD,
> +			.len = len,
> +			.buf = data,
> +		},
> +	};
> +
> +	return i2c_transfer(tj9->client->adapter, msgs, 2);

Could just use i2c_smbus_read_i2c_block_data(tj9->client, addr, len, data);

> +}
> +
> +static void kxtj9_report_acceleration_data(struct kxtj9_data *tj9)
> +{
> +	s16 acc_data[3]; /* Data bytes from hardware xL, xH, yL, yH, zL, zH */
> +	s16 x, y, z;
> +	int err;
> +
> +	err = kxtj9_i2c_read(tj9, XOUT_L, (u8 *)acc_data, 6);
> +	if (err < 0)
> +		dev_err(&tj9->client->dev, "accelerometer data read failed\n");
> +
> +	x = le16_to_cpu(acc_data[0]) >> tj9->shift;
> +	y = le16_to_cpu(acc_data[1]) >> tj9->shift;
> +	z = le16_to_cpu(acc_data[2]) >> tj9->shift;
> +
> +	input_report_abs(tj9->input_dev, ABS_X, tj9->pdata.negate_x ? -x : x);
> +	input_report_abs(tj9->input_dev, ABS_Y, tj9->pdata.negate_y ? -y : y);
> +	input_report_abs(tj9->input_dev, ABS_Z, tj9->pdata.negate_x ? -y : y);
> +	input_sync(tj9->input_dev);
> +}
> +
This next function does look like it should take an enum instead
of new_g_range then use a single table to look up both the shift and the
value for new_g_range.  Something like.
enum {
     range_2g, range_4g, range_8g
} kxtj9_range;
/* could use a struct for this to make what is what more explicit */
static const u8 rangevals[3][2] = { {KXTJ9_G_2G, SHIFT_ADJ_2G},
   		     		  {KXTJ9_G_4G, SHIFT_ADJ_4G},
		       		  {KXTJ9_G_8G, SHIFT_ADJ_8G}};
static int kxtj9_update_g_range(struct kxtj9_data *tj9, enum_kxtj9_range r)
{
	tj9->shift = rangevals[r][1];
	tj9->ctrl_reg1 &= 0xe7;	
	tj9->ctrl_reg1 |= rangevals[r][0];
}

Actually you could just encode values in the array rather than using defines
at all.  Also, could make the pdata element the enum value.

> +static int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range)
> +{
> +	switch (new_g_range) {
> +	case KXTJ9_G_2G:
> +		tj9->shift = SHIFT_ADJ_2G;
> +		break;
> +
> +	case KXTJ9_G_4G:
> +		tj9->shift = SHIFT_ADJ_4G;
> +		break;
> +
> +	case KXTJ9_G_8G:
> +		tj9->shift = SHIFT_ADJ_8G;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	tj9->ctrl_reg1 &= 0xe7;
> +	tj9->ctrl_reg1 |= new_g_range;
> +
> +	return 0;
> +}
> +
> +static int kxtj9_update_odr(struct kxtj9_data *tj9, int poll_interval)
> +{
> +	int err;
> +	int i;
> +
> +	/* Use the lowest ODR that can support the requested poll interval */
> +	for (i = 0; i < ARRAY_SIZE(kxtj9_odr_table); i++) {
> +		tj9->data_ctrl = kxtj9_odr_table[i].mask;
> +		if (poll_interval < kxtj9_odr_table[i].cutoff)
> +			break;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, 0);
> +	if (err < 0)
> +		return err;
> +
> +	err = i2c_smbus_write_byte_data(tj9->client, DATA_CTRL, tj9->data_ctrl);
> +	if (err < 0)
> +		return err;
> +
> +	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, tj9->ctrl_reg1);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int kxtj9_device_power_on(struct kxtj9_data *tj9)
> +{
> +	int err;
> +
> +	if (tj9->pdata.power_on) {
> +		err = tj9->pdata.power_on();
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	return 0;
> +}
Slight semantics change to pdata.power_on() returning 0 for success and you'll
have cleaner:

static int kxtj9_device_power_on(struct kxtj9_data *tj9)
{
	if (tj9->pdata.power_on)
		return tj9->pdata.power_on();

	return 0;
}

> +
> +static void kxtj9_device_power_off(struct kxtj9_data *tj9)
> +{
> +	int err;
> +
> +	tj9->ctrl_reg1 &= PC1_OFF;
> +	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, tj9->ctrl_reg1);
> +	if (err < 0)
> +		dev_err(&tj9->client->dev, "soft power off failed\n");
> +
> +	if (tj9->pdata.power_off)
> +		tj9->pdata.power_off();
> +}
> +
> +static int kxtj9_enable(struct kxtj9_data *tj9)
> +{
> +	int err;
> +
> +	err = kxtj9_device_power_on(tj9);
> +	if (err < 0)
> +		return err;
> +
> +	/* ensure that PC1 is cleared before updating control registers */
> +	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, 0);
> +	if (err < 0)
> +		return err;
> +
> +	/* only write INT_CTRL_REG1 if in irq mode */
> +	if (tj9->client->irq) {
> +		err = i2c_smbus_write_byte_data(tj9->client,
> +						INT_CTRL1, tj9->int_ctrl);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	err = kxtj9_update_g_range(tj9, tj9->pdata.g_range);
> +	if (err < 0)
> +		return err;
> +
> +	/* turn on outputs */
> +	tj9->ctrl_reg1 |= PC1_ON;
> +	err = i2c_smbus_write_byte_data(tj9->client, CTRL_REG1, tj9->ctrl_reg1);
> +	if (err < 0)
> +		return err;
> +
> +	err = kxtj9_update_odr(tj9, tj9->last_poll_interval);
> +	if (err < 0)
> +		return err;
> +
> +	/* clear initial interrupt if in irq mode */
> +	if (tj9->client->irq) {
> +		err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
> +		if (err < 0) {
> +			dev_err(&tj9->client->dev,
> +					"error clearing interrupt: %d\n", err);
> +			goto fail;
> +		}
> +	}
> +
> +	return 0;
> +
> +fail:
> +	kxtj9_device_power_off(tj9);
> +	return err;
> +}
> +
> +static void __devinit kxtj9_init_input_device(struct kxtj9_data *tj9,
> +						struct input_dev *input_dev)
> +{
> +	__set_bit(EV_ABS, input_dev->evbit);
> +	input_set_abs_params(input_dev, ABS_X, -G_MAX, G_MAX, FUZZ, FLAT);
> +	input_set_abs_params(input_dev, ABS_Y, -G_MAX, G_MAX, FUZZ, FLAT);
> +	input_set_abs_params(input_dev, ABS_Z, -G_MAX, G_MAX, FUZZ, FLAT);
> +
> +	input_dev->name = "kxtj9_accel";
Is the _accel bit a convention? 
> +	input_dev->id.bustype = BUS_I2C;
> +	input_dev->dev.parent = &tj9->client->dev;
> +}
> +
> +#ifdef CONFIG_KXTJ9_POLLED_MODE
> +static void kxtj9_poll(struct input_polled_dev *dev)
> +{
> +	struct kxtj9_data *tj9 = dev->private;
> +	unsigned int poll_interval = dev->poll_interval;
> +
> +	kxtj9_report_acceleration_data(tj9);
> +
> +	if (poll_interval != tj9->last_poll_interval) {
> +		kxtj9_update_odr(tj9, poll_interval);
> +		tj9->last_poll_interval = poll_interval;
> +	}
> +}
> +
> +static void kxtj9_polled_input_open(struct input_polled_dev *dev)
> +{
> +	struct kxtj9_data *tj9 = dev->private;
> +
> +	kxtj9_enable(tj9);
Given ktxj9_enable knows what it's getting, squash into

kxtj9_enable(dev->private);
> +}
> +
> +static void kxtj9_polled_input_close(struct input_polled_dev *dev)
> +{
> +	struct kxtj9_data *tj9 = dev->private;
> +
> +	kxtj9_device_power_off(tj9);
same as previous.
> +}
> +
> +static int __devinit kxtj9_setup_polled_device(struct kxtj9_data *tj9)
> +{
> +	int err;
> +	struct input_polled_dev *poll_dev;
> +	poll_dev = input_allocate_polled_device();
> +
> +	if (!poll_dev) {
> +		dev_err(&tj9->client->dev,
> +			"Failed to allocate polled device\n");
> +		return -ENOMEM;
> +	}
> +
> +	tj9->poll_dev = poll_dev;
> +	tj9->input_dev = poll_dev->input;
> +
> +	poll_dev->private = tj9;
> +	poll_dev->poll = kxtj9_poll;
> +	poll_dev->open = kxtj9_polled_input_open;
> +	poll_dev->close = kxtj9_polled_input_close;
> +
> +	kxtj9_init_input_device(tj9, poll_dev->input);
> +
> +	err = input_register_polled_device(poll_dev);
> +	if (err) {
> +		dev_err(&tj9->client->dev,
> +			"Unable to register polled device, err=%d\n", err);
> +		input_free_polled_device(poll_dev);
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static void kxtj9_teardown_polled_device(struct kxtj9_data *tj9)
> +{
> +	input_unregister_polled_device(tj9->poll_dev);
> +	input_free_polled_device(tj9->poll_dev);
> +}
> +
> +#else	/* IRQ Mode is enabled */
> +static int kxtj9_input_open(struct input_dev *input)
> +{
> +	struct kxtj9_data *tj9 = input_get_drvdata(input);
> +
> +	return kxtj9_enable(tj9);
return kxtj9_enable(input_get_drvdata(input));
> +}
> +
> +static void kxtj9_input_close(struct input_dev *dev)
> +{
> +	struct kxtj9_data *tj9 = input_get_drvdata(dev);
> +
> +	kxtj9_device_power_off(tj9);
snap
> +}
> +
> +static int __devinit kxtj9_setup_input_device(struct kxtj9_data *tj9)
> +{
> +	struct input_dev *input_dev;
> +	int err;
> +
> +	input_dev = input_allocate_device();
> +	if (!tj9->input_dev) {
> +		dev_err(&tj9->client->dev, "input device allocate failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	tj9->input_dev = input_dev;
> +
> +	input_dev->open = kxtj9_input_open;
> +	input_dev->close = kxtj9_input_close;
> +	input_set_drvdata(input_dev, tj9);
> +
> +	kxtj9_init_input_device(tj9, input_dev);
> +
> +	err = input_register_device(tj9->input_dev);
> +	if (err) {
> +		dev_err(&tj9->client->dev,
> +			"unable to register input polled device %s: %d\n",
> +			tj9->input_dev->name, err);
> +		input_free_device(tj9->input_dev);
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t kxtj9_isr(int irq, void *dev)
> +{
> +	struct kxtj9_data *tj9 = dev;
> +	int err;
> +
> +	/* data ready is the only possible interrupt type */
> +	kxtj9_report_acceleration_data(tj9);
> +
> +	err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
> +	if (err < 0)
> +		dev_err(&tj9->client->dev,
> +				"error clearing interrupt status: %d\n", err);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static inline int kxtj9_setup_polled_device(struct kxtj9_data *tj9)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline void kxtj9_teardown_polled_device(struct kxtj9_data *tj9)
> +{
> +}
> +
> +/* kxtj9_get_poll: returns the currently selected poll interval (in ms) */
> +static ssize_t kxtj9_get_poll(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
> +	return sprintf(buf, "%d\n", tj9->last_poll_interval);
> +}
> +
> +/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
> +static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
> +						const char *buf, size_t count)
> +{
> +	struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	unsigned long interval;
> +	int ret = kstrtoul(buf, 10, &interval);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&tj9->lock);
> +	/* set current interval to the greater of the minimum interval or
> +	the requested interval */
> +	tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
> +	mutex_unlock(&tj9->lock);
> +
> +	kxtj9_update_odr(tj9, tj9->last_poll_interval);
> +
> +	return count;
> +}
New line here please.
> +/* When IRQ mode is selected, we need to provide an interface to allow the user
> + * to change the output data rate of the part.  For consistency, we are using
> + * the set_poll method, which accepts a poll interval in milliseconds, and then
> + * calls update_odr() while passing this value as an argument.  In IRQ mode, the
> + * data outputs will not be read AT the requested poll interval, rather, the
> + * lowest ODR that can support the requested interval.  The client application
> + * will be responsible for retrieving data from the input node at the desired
> + * interval.
> + */
> +static DEVICE_ATTR(poll, S_IRUGO|S_IWUSR, kxtj9_get_poll, kxtj9_set_poll);
> +
> +static struct attribute *kxtj9_attributes[] = {
> +	&dev_attr_poll.attr,
> +	NULL
> +};
> +
> +static struct attribute_group kxtj9_attribute_group = {
> +	.attrs = kxtj9_attributes
> +};
> +#endif
> +
> +static int __devinit kxtj9_verify(struct kxtj9_data *tj9)
> +{
> +	int retval;
> +	retval = kxtj9_device_power_on(tj9);
> +	if (retval < 0)
> +		return retval;
> +	retval = i2c_smbus_read_byte_data(tj9->client, WHO_AM_I);
> +	if (retval < 0) {
> +		dev_err(&tj9->client->dev, "read err int source\n");
> +		goto out;
> +	}
> +	retval = retval != 0x06 ? -EIO : 0;
> +
> +out:
> +	kxtj9_device_power_off(tj9);
> +	return retval;
> +}
> +
> +static int __devinit kxtj9_probe(struct i2c_client *client,
> +				const struct i2c_device_id *id)
> +{
> +	const struct kxtj9_platform_data *pdata = client->dev.platform_data;
> +	struct kxtj9_data *tj9;
> +	int err;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		dev_err(&client->dev, "client is not i2c capable\n");
> +		return -ENXIO;
> +	}
> +	if (!pdata) {
> +		dev_err(&client->dev, "platform data is NULL; exiting\n");
> +		return -EINVAL;
> +	}
> +	tj9 = kzalloc(sizeof(*tj9), GFP_KERNEL);
> +	if (!tj9) {
> +		dev_err(&client->dev,
> +			"failed to allocate memory for module data\n");
> +		return -ENOMEM;
> +	}
> +
> +	tj9->client = client;
> +	tj9->pdata = *pdata;
> +
> +	if (pdata->init) {
> +		err = pdata->init();
> +		if (err < 0)
> +			goto err_free_mem;
> +	}
> +
> +	err = kxtj9_verify(tj9);
> +	if (err < 0) {
> +		dev_err(&client->dev, "device not recognized\n");
> +		goto err_pdata_exit;
> +	}
> +
> +	tj9->ctrl_reg1 = tj9->pdata.res_12bit | tj9->pdata.g_range;
> +	tj9->data_ctrl = tj9->pdata.data_odr_init;
> +
> +#ifdef CONFIG_KXTJ9_POLLED_MODE
> +	err = kxtj9_setup_polled_device(tj9);
> +	if (err)
> +		goto err_pdata_exit;
> +#else
> +	/* If in irq mode, populate INT_CTRL_REG1 and enable DRDY. */
> +	tj9->int_ctrl |= KXTJ9_IEN | KXTJ9_IEA | KXTJ9_IEL;
> +	tj9->ctrl_reg1 |= DRDYE;
> +
> +	err = kxtj9_setup_input_device(tj9);
> +	if (err)
> +		goto err_pdata_exit;
> +
> +	err = request_threaded_irq(client->irq, NULL, kxtj9_isr,
> +				IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +				"kxtj9-irq", tj9);
> +	if (err) {
> +		dev_err(&client->dev, "request irq failed: %d\n", err);
> +		goto err_destroy_input;
> +	}
> +	err = sysfs_create_group(&client->dev.kobj, &kxtj9_attribute_group);
> +	if (err) {
> +		dev_err(&client->dev, "sysfs create failed: %d\n", err);
> +		goto err_destroy_input;
> +	}
> +#endif
> +	i2c_set_clientdata(client, tj9);
> +	return 0;
> +
> +#ifndef CONFIG_KXTJ9_POLLED_MODE
> +err_destroy_input:
> +	input_unregister_device(tj9->input_dev);
> +#endif
> +err_pdata_exit:
> +	if (tj9->pdata.exit)
> +		tj9->pdata.exit();
> +err_free_mem:
> +	kfree(tj9);
> +	return err;
> +}
> +
> +static int __devexit kxtj9_remove(struct i2c_client *client)
> +{
> +	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
> +
> +#ifdef CONFIG_KXTJ9_POLLED_MODE
> +	kxtj9_teardown_polled_device(tj9);
> +#else
> +	sysfs_remove_group(&client->dev.kobj, &kxtj9_attribute_group);
> +	free_irq(client->irq, tj9);
> +	input_unregister_device(tj9->input_dev);
> +#endif
> +	if (tj9->pdata.exit)
> +		tj9->pdata.exit();
> +
> +	kfree(tj9);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int kxtj9_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
> +	struct input_dev *input_dev = tj9->input_dev;
> +
> +	mutex_lock(&input_dev->mutex);
> +
> +	if (input_dev->users)
> +		kxtj9_device_power_off(tj9);
> +
> +	mutex_unlock(&input_dev->mutex);
> +	return 0;
> +}
> +
> +static int kxtj9_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
> +	struct input_dev *input_dev = tj9->input_dev;
> +	int retval = 0;
> +
> +	mutex_lock(&input_dev->mutex);
> +
> +	if (input_dev->users)
> +		kxtj9_enable(tj9);
> +
> +	mutex_unlock(&input_dev->mutex);
> +	return retval;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(kxtj9_pm_ops, kxtj9_suspend, kxtj9_resume);
> +
> +static const struct i2c_device_id kxtj9_id[] = {
> +	{ NAME, 0 },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, kxtj9_id);
> +
> +static struct i2c_driver kxtj9_driver = {
> +	.driver = {
> +		  .name = NAME,
> +		  .owner = THIS_MODULE,
> +		  .pm = &kxtj9_pm_ops,
> +	},
> +	.probe = kxtj9_probe,
> +	.remove = __devexit_p(kxtj9_remove),
> +	.id_table = kxtj9_id,
> +};
> +
> +static int __init kxtj9_init(void)
> +{
> +	return i2c_add_driver(&kxtj9_driver);
> +}
> +module_init(kxtj9_init);
> +
> +static void __exit kxtj9_exit(void)
> +{
> +	i2c_del_driver(&kxtj9_driver);
> +}
> +module_exit(kxtj9_exit);
> +
> +MODULE_DESCRIPTION("KXTJ9 accelerometer driver");
> +MODULE_AUTHOR("Chris Hudson <chudson@kionix.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/input/kxtj9.h b/include/linux/input/kxtj9.h
> index 0000000..cc5928b
> --- /dev/null
> +++ b/include/linux/input/kxtj9.h
> @@ -0,0 +1,76 @@
> +/*
> + * Copyright (C) 2011 Kionix, Inc.
> + * Written by Chris Hudson <chudson@kionix.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> + * 02111-1307, USA
> + */
> +
> +#ifndef __KXTJ9_H__
> +#define __KXTJ9_H__
> +
> +#define KXTJ9_I2C_ADDR		0x0F
> +
> +/* These shift values are used to provide consistent output data */
> +#define SHIFT_ADJ_2G		4
> +#define SHIFT_ADJ_4G		3
> +#define SHIFT_ADJ_8G		2
> +
> +#ifdef __KERNEL__
> +struct kxtj9_platform_data {
> +	int poll_interval;	/* current poll interval (in milli-seconds) */
> +	int min_interval;	/* minimum poll interval (in milli-seconds) */
> +
> +	/* by default, x is axis 0, y is axis 1, z is axis 2; these can be
> +	changed to account for sensor orientation within the host device */
> +	u8 axis_map_x;
> +	u8 axis_map_y;
> +	u8 axis_map_z;
> +
> +	/* each axis can be negated to account for sensor orientation within
> +	the host device; 1 = negate this axis; 0 = do not negate this axis */
> +	bool negate_x;
> +	bool negate_y;
> +	bool negate_z;
> +
> +	/* CTRL_REG1: set resolution, g-range, data ready enable */
> +	/* Output resolution: 8-bit valid or 12-bit valid */
> +	#define RES_8BIT		0
> +	#define RES_12BIT		(1 << 6)
> +	u8 res_12bit;
> +	/* Output g-range: +/-2g, 4g, or 8g */
> +	#define KXTJ9_G_2G		0
> +	#define KXTJ9_G_4G		(1 << 3)
> +	#define KXTJ9_G_8G		(1 << 4)
> +	u8 g_range;
> +
> +	/* DATA_CTRL_REG: controls the output data rate of the part */
> +	#define ODR12_5F		0
> +	#define ODR25F			1
> +	#define ODR50F			2
> +	#define ODR100F			3
> +	#define ODR200F			4
> +	#define ODR400F			5
> +	#define ODR800F			6
> +	u8 data_odr_init;
> +
> +	int (*init)(void);
> +	void (*exit)(void);
> +	int (*power_on)(void);
> +	int (*power_off)(void);
> +};
> +#endif /* __KERNEL__ */
> +
> +#endif  /* __KXTJ9_H__ */
> +

Re: [PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[Dmitry Torokhov]

On Wed, Jun 29, 2011 at 01:39:41PM +0100, Jonathan Cameron wrote:
> On 06/28/11 20:12, chris.hudson.comp.eng@gmail.com wrote:
> > +static int kxtj9_input_open(struct input_dev *input)
> > +{
> > +	struct kxtj9_data *tj9 = input_get_drvdata(input);
> > +
> > +	return kxtj9_enable(tj9);
> return kxtj9_enable(input_get_drvdata(input));

Actually I prefer limiting use of void pointers. If kxtj9_enable()
changes to take something other than "struct kxtj9_data *" then original
will issue a warning while the proposed form will happily accept it.

Thanks.

-- 
Dmitry

Re: [PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[Dmitry Torokhov]

Hi Chris,

On Tue, Jun 28, 2011 at 03:12:28PM -0400, chris.hudson.comp.eng@gmail.com wrote:
> From: Chris Hudson <chudson@kionix.com>
> 
> - Added Dmitry's changes
> - Now using input_polled_dev interface when in polling mode
> - IRQ mode provides a separate sysfs node to change the hardware data rate
> 

I am not happy with the fact that this implementation forces users to
select polled or interrupt-driven mode at compile time. The patch I
proposed had polled mode support optional and would automatically select
IRQ mode for clients that have interrupt assigned and try to activate
polled mode if interrupt is not available.

> +
> +/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
> +static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
> +						const char *buf, size_t count)
> +{
> +	struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	unsigned long interval;
> +	int ret = kstrtoul(buf, 10, &interval);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&tj9->lock);
> +	/* set current interval to the greater of the minimum interval or
> +	the requested interval */
> +	tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
> +	mutex_unlock(&tj9->lock);

This lock does not make sense. You are protecting update of last_poll_interval
field and this update can not be partial (i.e. only LSB or MSB is
written) on all our arches, but you do not protect kxtj9_update_odr()
which alters chip state and does need protection.

I tried bringing forward my changes from the older patch into yours,
please let me know if the patch below works on top of yours.

Thanks.

-- 
Dmitry


Input: ktxj9 - misc fixes

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/misc/Kconfig  |   34 ++--
 drivers/input/misc/kxtj9.c  |  355 +++++++++++++++++++++++--------------------
 include/linux/input/kxtj9.h |   18 +-
 3 files changed, 220 insertions(+), 187 deletions(-)


diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 41628d2..0134428 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -230,6 +230,23 @@ config INPUT_KEYSPAN_REMOTE
 	  To compile this driver as a module, choose M here: the module will
 	  be called keyspan_remote.
 
+config INPUT_KXTJ9
+	tristate "Kionix KXTJ9 tri-axis digital accelerometer"
+	depends on I2C
+	help
+	  Say Y here to enable support for the Kionix KXTJ9 digital tri-axis
+	  accelerometer.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called kxtj9.
+
+config INPUT_KXTJ9_POLLED_MODE
+	bool "Enable polling mode support"
+	depends on INPUT_KXTJ9
+	select INPUT_POLLDEV
+	help
+	  Say Y here if you need accelerometer to work in polling mode.
+
 config INPUT_POWERMATE
 	tristate "Griffin PowerMate and Contour Jog support"
 	depends on USB_ARCH_HAS_HCD
@@ -499,21 +516,4 @@ config INPUT_XEN_KBDDEV_FRONTEND
 	  To compile this driver as a module, choose M here: the
 	  module will be called xen-kbdfront.
 
-config INPUT_KXTJ9
-       tristate "Kionix KXTJ9 tri-axis digital accelerometer"
-       depends on I2C
-       help
-         If you say yes here you get support for the Kionix KXTJ9 digital
-         tri-axis accelerometer.
-
-         This driver can also be built as a module.  If so, the module
-         will be called kxtj9.
-
-config KXTJ9_POLLED_MODE
-       bool "Enable polling mode support"
-       depends on INPUT_KXTJ9
-       select INPUT_POLLDEV
-       help
-         Say Y here if you need accelerometer to work in polling mode.
-
 endif
diff --git a/drivers/input/misc/kxtj9.c b/drivers/input/misc/kxtj9.c
index 7faf155..31eae6a 100644
--- a/drivers/input/misc/kxtj9.c
+++ b/drivers/input/misc/kxtj9.c
@@ -75,10 +75,8 @@ struct kxtj9_data {
 	struct i2c_client *client;
 	struct kxtj9_platform_data pdata;
 	struct input_dev *input_dev;
-#ifdef CONFIG_KXTJ9_POLLED_MODE
+#ifdef CONFIG_INPUT_KXTJ9_POLLED_MODE
 	struct input_polled_dev *poll_dev;
-#else
-	struct mutex lock;
 #endif
 	unsigned int last_poll_interval;
 	u8 shift;
@@ -117,16 +115,32 @@ static void kxtj9_report_acceleration_data(struct kxtj9_data *tj9)
 	if (err < 0)
 		dev_err(&tj9->client->dev, "accelerometer data read failed\n");
 
-	x = le16_to_cpu(acc_data[0]) >> tj9->shift;
-	y = le16_to_cpu(acc_data[1]) >> tj9->shift;
-	z = le16_to_cpu(acc_data[2]) >> tj9->shift;
+	x = le16_to_cpu(acc_data[tj9->pdata.axis_map_x]) >> tj9->shift;
+	y = le16_to_cpu(acc_data[tj9->pdata.axis_map_y]) >> tj9->shift;
+	z = le16_to_cpu(acc_data[tj9->pdata.axis_map_z]) >> tj9->shift;
 
 	input_report_abs(tj9->input_dev, ABS_X, tj9->pdata.negate_x ? -x : x);
 	input_report_abs(tj9->input_dev, ABS_Y, tj9->pdata.negate_y ? -y : y);
-	input_report_abs(tj9->input_dev, ABS_Z, tj9->pdata.negate_x ? -y : y);
+	input_report_abs(tj9->input_dev, ABS_Z, tj9->pdata.negate_z ? -z : z);
 	input_sync(tj9->input_dev);
 }
 
+static irqreturn_t kxtj9_isr(int irq, void *dev)
+{
+	struct kxtj9_data *tj9 = dev;
+	int err;
+
+	/* data ready is the only possible interrupt type */
+	kxtj9_report_acceleration_data(tj9);
+
+	err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
+	if (err < 0)
+		dev_err(&tj9->client->dev,
+			"error clearing interrupt status: %d\n", err);
+
+	return IRQ_HANDLED;
+}
+
 static int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range)
 {
 	switch (new_g_range) {
@@ -152,7 +166,7 @@ static int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range)
 	return 0;
 }
 
-static int kxtj9_update_odr(struct kxtj9_data *tj9, int poll_interval)
+static int kxtj9_update_odr(struct kxtj9_data *tj9, unsigned int poll_interval)
 {
 	int err;
 	int i;
@@ -245,7 +259,7 @@ static int kxtj9_enable(struct kxtj9_data *tj9)
 		err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
 		if (err < 0) {
 			dev_err(&tj9->client->dev,
-					"error clearing interrupt: %d\n", err);
+				"error clearing interrupt: %d\n", err);
 			goto fail;
 		}
 	}
@@ -257,8 +271,27 @@ fail:
 	return err;
 }
 
+static void kxtj9_disable(struct kxtj9_data *tj9)
+{
+	kxtj9_device_power_off(tj9);
+}
+
+static int kxtj9_input_open(struct input_dev *input)
+{
+	struct kxtj9_data *tj9 = input_get_drvdata(input);
+
+	return kxtj9_enable(tj9);
+}
+
+static void kxtj9_input_close(struct input_dev *dev)
+{
+	struct kxtj9_data *tj9 = input_get_drvdata(dev);
+
+	kxtj9_disable(tj9);
+}
+
 static void __devinit kxtj9_init_input_device(struct kxtj9_data *tj9,
-						struct input_dev *input_dev)
+					      struct input_dev *input_dev)
 {
 	__set_bit(EV_ABS, input_dev->evbit);
 	input_set_abs_params(input_dev, ABS_X, -G_MAX, G_MAX, FUZZ, FLAT);
@@ -270,7 +303,104 @@ static void __devinit kxtj9_init_input_device(struct kxtj9_data *tj9,
 	input_dev->dev.parent = &tj9->client->dev;
 }
 
-#ifdef CONFIG_KXTJ9_POLLED_MODE
+static int __devinit kxtj9_setup_input_device(struct kxtj9_data *tj9)
+{
+	struct input_dev *input_dev;
+	int err;
+
+	input_dev = input_allocate_device();
+	if (!tj9->input_dev) {
+		dev_err(&tj9->client->dev, "input device allocate failed\n");
+		return -ENOMEM;
+	}
+
+	tj9->input_dev = input_dev;
+
+	input_dev->open = kxtj9_input_open;
+	input_dev->close = kxtj9_input_close;
+	input_set_drvdata(input_dev, tj9);
+
+	kxtj9_init_input_device(tj9, input_dev);
+
+	err = input_register_device(tj9->input_dev);
+	if (err) {
+		dev_err(&tj9->client->dev,
+			"unable to register input polled device %s: %d\n",
+			tj9->input_dev->name, err);
+		input_free_device(tj9->input_dev);
+		return err;
+	}
+
+	return 0;
+}
+
+/*
+ * When IRQ mode is selected, we need to provide an interface to allow the user
+ * to change the output data rate of the part.  For consistency, we are using
+ * the set_poll method, which accepts a poll interval in milliseconds, and then
+ * calls update_odr() while passing this value as an argument.  In IRQ mode, the
+ * data outputs will not be read AT the requested poll interval, rather, the
+ * lowest ODR that can support the requested interval.  The client application
+ * will be responsible for retrieving data from the input node at the desired
+ * interval.
+ */
+
+/* Returns currently selected poll interval (in ms) */
+static ssize_t kxtj9_get_poll(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
+
+	return sprintf(buf, "%d\n", tj9->last_poll_interval);
+}
+
+/* Allow users to select a new poll interval (in ms) */
+static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
+						const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
+	struct input_dev *input_dev = tj9->input_dev;
+	unsigned int interval;
+	int error;
+
+	error = kstrtouint(buf, 10, &interval);
+	if (error < 0)
+		return error;
+
+	/* Lock the device to prevent races with open/close (and itself) */
+	mutex_unlock(&input_dev->mutex);
+
+	disable_irq(client->irq);
+
+	/*
+	 * Set current interval to the greater of the minimum interval or
+	 * the requested interval
+	 */
+	tj9->last_poll_interval = max(interval, tj9->pdata.min_interval);
+
+	kxtj9_update_odr(tj9, tj9->last_poll_interval);
+
+	enable_irq(client->irq);
+	mutex_unlock(&input_dev->mutex);
+
+	return count;
+}
+
+static DEVICE_ATTR(poll, S_IRUGO|S_IWUSR, kxtj9_get_poll, kxtj9_set_poll);
+
+static struct attribute *kxtj9_attributes[] = {
+	&dev_attr_poll.attr,
+	NULL
+};
+
+static struct attribute_group kxtj9_attribute_group = {
+	.attrs = kxtj9_attributes
+};
+
+
+#ifdef CONFIG_INPUT_KXTJ9_POLLED_MODE
 static void kxtj9_poll(struct input_polled_dev *dev)
 {
 	struct kxtj9_data *tj9 = dev->private;
@@ -295,7 +425,7 @@ static void kxtj9_polled_input_close(struct input_polled_dev *dev)
 {
 	struct kxtj9_data *tj9 = dev->private;
 
-	kxtj9_device_power_off(tj9);
+	kxtj9_disable(tj9);
 }
 
 static int __devinit kxtj9_setup_polled_device(struct kxtj9_data *tj9)
@@ -325,7 +455,7 @@ static int __devinit kxtj9_setup_polled_device(struct kxtj9_data *tj9)
 		dev_err(&tj9->client->dev,
 			"Unable to register polled device, err=%d\n", err);
 		input_free_polled_device(poll_dev);
-		return 0;
+		return err;
 	}
 
 	return 0;
@@ -337,66 +467,7 @@ static void kxtj9_teardown_polled_device(struct kxtj9_data *tj9)
 	input_free_polled_device(tj9->poll_dev);
 }
 
-#else	/* IRQ Mode is enabled */
-static int kxtj9_input_open(struct input_dev *input)
-{
-	struct kxtj9_data *tj9 = input_get_drvdata(input);
-
-	return kxtj9_enable(tj9);
-}
-
-static void kxtj9_input_close(struct input_dev *dev)
-{
-	struct kxtj9_data *tj9 = input_get_drvdata(dev);
-
-	kxtj9_device_power_off(tj9);
-}
-
-static int __devinit kxtj9_setup_input_device(struct kxtj9_data *tj9)
-{
-	struct input_dev *input_dev;
-	int err;
-
-	input_dev = input_allocate_device();
-	if (!tj9->input_dev) {
-		dev_err(&tj9->client->dev, "input device allocate failed\n");
-		return -ENOMEM;
-	}
-
-	tj9->input_dev = input_dev;
-
-	input_dev->open = kxtj9_input_open;
-	input_dev->close = kxtj9_input_close;
-	input_set_drvdata(input_dev, tj9);
-
-	kxtj9_init_input_device(tj9, input_dev);
-
-	err = input_register_device(tj9->input_dev);
-	if (err) {
-		dev_err(&tj9->client->dev,
-			"unable to register input polled device %s: %d\n",
-			tj9->input_dev->name, err);
-		input_free_device(tj9->input_dev);
-	}
-
-	return 0;
-}
-
-static irqreturn_t kxtj9_isr(int irq, void *dev)
-{
-	struct kxtj9_data *tj9 = dev;
-	int err;
-
-	/* data ready is the only possible interrupt type */
-	kxtj9_report_acceleration_data(tj9);
-
-	err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
-	if (err < 0)
-		dev_err(&tj9->client->dev,
-				"error clearing interrupt status: %d\n", err);
-
-	return IRQ_HANDLED;
-}
+#else
 
 static inline int kxtj9_setup_polled_device(struct kxtj9_data *tj9)
 {
@@ -407,67 +478,22 @@ static inline void kxtj9_teardown_polled_device(struct kxtj9_data *tj9)
 {
 }
 
-/* kxtj9_get_poll: returns the currently selected poll interval (in ms) */
-static ssize_t kxtj9_get_poll(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
-	return sprintf(buf, "%d\n", tj9->last_poll_interval);
-}
-
-/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
-static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
-						const char *buf, size_t count)
-{
-	struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
-
-	unsigned long interval;
-	int ret = kstrtoul(buf, 10, &interval);
-	if (ret < 0)
-		return ret;
-
-	mutex_lock(&tj9->lock);
-	/* set current interval to the greater of the minimum interval or
-	the requested interval */
-	tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
-	mutex_unlock(&tj9->lock);
-
-	kxtj9_update_odr(tj9, tj9->last_poll_interval);
-
-	return count;
-}
-/* When IRQ mode is selected, we need to provide an interface to allow the user
- * to change the output data rate of the part.  For consistency, we are using
- * the set_poll method, which accepts a poll interval in milliseconds, and then
- * calls update_odr() while passing this value as an argument.  In IRQ mode, the
- * data outputs will not be read AT the requested poll interval, rather, the
- * lowest ODR that can support the requested interval.  The client application
- * will be responsible for retrieving data from the input node at the desired
- * interval.
- */
-static DEVICE_ATTR(poll, S_IRUGO|S_IWUSR, kxtj9_get_poll, kxtj9_set_poll);
-
-static struct attribute *kxtj9_attributes[] = {
-	&dev_attr_poll.attr,
-	NULL
-};
-
-static struct attribute_group kxtj9_attribute_group = {
-	.attrs = kxtj9_attributes
-};
 #endif
 
 static int __devinit kxtj9_verify(struct kxtj9_data *tj9)
 {
 	int retval;
+
 	retval = kxtj9_device_power_on(tj9);
 	if (retval < 0)
 		return retval;
+
 	retval = i2c_smbus_read_byte_data(tj9->client, WHO_AM_I);
 	if (retval < 0) {
 		dev_err(&tj9->client->dev, "read err int source\n");
 		goto out;
 	}
+
 	retval = retval != 0x06 ? -EIO : 0;
 
 out:
@@ -476,7 +502,7 @@ out:
 }
 
 static int __devinit kxtj9_probe(struct i2c_client *client,
-				const struct i2c_device_id *id)
+				 const struct i2c_device_id *id)
 {
 	const struct kxtj9_platform_data *pdata = client->dev.platform_data;
 	struct kxtj9_data *tj9;
@@ -487,10 +513,12 @@ static int __devinit kxtj9_probe(struct i2c_client *client,
 		dev_err(&client->dev, "client is not i2c capable\n");
 		return -ENXIO;
 	}
+
 	if (!pdata) {
 		dev_err(&client->dev, "platform data is NULL; exiting\n");
 		return -EINVAL;
 	}
+
 	tj9 = kzalloc(sizeof(*tj9), GFP_KERNEL);
 	if (!tj9) {
 		dev_err(&client->dev,
@@ -513,42 +541,46 @@ static int __devinit kxtj9_probe(struct i2c_client *client,
 		goto err_pdata_exit;
 	}
 
+	i2c_set_clientdata(client, tj9);
+
 	tj9->ctrl_reg1 = tj9->pdata.res_12bit | tj9->pdata.g_range;
 	tj9->data_ctrl = tj9->pdata.data_odr_init;
 
-#ifdef CONFIG_KXTJ9_POLLED_MODE
-	err = kxtj9_setup_polled_device(tj9);
-	if (err)
-		goto err_pdata_exit;
-#else
-	/* If in irq mode, populate INT_CTRL_REG1 and enable DRDY. */
-	tj9->int_ctrl |= KXTJ9_IEN | KXTJ9_IEA | KXTJ9_IEL;
-	tj9->ctrl_reg1 |= DRDYE;
+	if (client->irq) {
+		/* If in irq mode, populate INT_CTRL_REG1 and enable DRDY. */
+		tj9->int_ctrl |= KXTJ9_IEN | KXTJ9_IEA | KXTJ9_IEL;
+		tj9->ctrl_reg1 |= DRDYE;
+
+		err = kxtj9_setup_input_device(tj9);
+		if (err)
+			goto err_pdata_exit;
+
+		err = request_threaded_irq(client->irq, NULL, kxtj9_isr,
+					   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					   "kxtj9-irq", tj9);
+		if (err) {
+			dev_err(&client->dev, "request irq failed: %d\n", err);
+			goto err_destroy_input;
+		}
 
-	err = kxtj9_setup_input_device(tj9);
-	if (err)
-		goto err_pdata_exit;
+		err = sysfs_create_group(&client->dev.kobj, &kxtj9_attribute_group);
+		if (err) {
+			dev_err(&client->dev, "sysfs create failed: %d\n", err);
+			goto err_free_irq;
+		}
 
-	err = request_threaded_irq(client->irq, NULL, kxtj9_isr,
-				IRQF_TRIGGER_RISING | IRQF_ONESHOT,
-				"kxtj9-irq", tj9);
-	if (err) {
-		dev_err(&client->dev, "request irq failed: %d\n", err);
-		goto err_destroy_input;
-	}
-	err = sysfs_create_group(&client->dev.kobj, &kxtj9_attribute_group);
-	if (err) {
-		dev_err(&client->dev, "sysfs create failed: %d\n", err);
-		goto err_destroy_input;
+	} else {
+		err = kxtj9_setup_polled_device(tj9);
+		if (err)
+			goto err_pdata_exit;
 	}
-#endif
-	i2c_set_clientdata(client, tj9);
+
 	return 0;
 
-#ifndef CONFIG_KXTJ9_POLLED_MODE
+err_free_irq:
+	free_irq(client->irq, tj9);
 err_destroy_input:
 	input_unregister_device(tj9->input_dev);
-#endif
 err_pdata_exit:
 	if (tj9->pdata.exit)
 		tj9->pdata.exit();
@@ -561,13 +593,14 @@ static int __devexit kxtj9_remove(struct i2c_client *client)
 {
 	struct kxtj9_data *tj9 = i2c_get_clientdata(client);
 
-#ifdef CONFIG_KXTJ9_POLLED_MODE
-	kxtj9_teardown_polled_device(tj9);
-#else
-	sysfs_remove_group(&client->dev.kobj, &kxtj9_attribute_group);
-	free_irq(client->irq, tj9);
-	input_unregister_device(tj9->input_dev);
-#endif
+	if (client->irq) {
+		sysfs_remove_group(&client->dev.kobj, &kxtj9_attribute_group);
+		free_irq(client->irq, tj9);
+		input_unregister_device(tj9->input_dev);
+	} else {
+		kxtj9_teardown_polled_device(tj9);
+	}
+
 	if (tj9->pdata.exit)
 		tj9->pdata.exit();
 
@@ -620,13 +653,13 @@ MODULE_DEVICE_TABLE(i2c, kxtj9_id);
 
 static struct i2c_driver kxtj9_driver = {
 	.driver = {
-		  .name = NAME,
-		  .owner = THIS_MODULE,
-		  .pm = &kxtj9_pm_ops,
+		.name	= NAME,
+		.owner	= THIS_MODULE,
+		.pm	= &kxtj9_pm_ops,
 	},
-	.probe = kxtj9_probe,
-	.remove = __devexit_p(kxtj9_remove),
-	.id_table = kxtj9_id,
+	.probe		= kxtj9_probe,
+	.remove		= __devexit_p(kxtj9_remove),
+	.id_table	= kxtj9_id,
 };
 
 static int __init kxtj9_init(void)
diff --git a/include/linux/input/kxtj9.h b/include/linux/input/kxtj9.h
index cc5928b..0b546bd 100644
--- a/include/linux/input/kxtj9.h
+++ b/include/linux/input/kxtj9.h
@@ -27,19 +27,21 @@
 #define SHIFT_ADJ_4G		3
 #define SHIFT_ADJ_8G		2
 
-#ifdef __KERNEL__
 struct kxtj9_platform_data {
-	int poll_interval;	/* current poll interval (in milli-seconds) */
-	int min_interval;	/* minimum poll interval (in milli-seconds) */
+	unsigned int min_interval;	/* minimum poll interval (in milli-seconds) */
 
-	/* by default, x is axis 0, y is axis 1, z is axis 2; these can be
-	changed to account for sensor orientation within the host device */
+	/*
+	 * By default, x is axis 0, y is axis 1, z is axis 2; these can be
+	 * changed to account for sensor orientation within the host device.
+	 */
 	u8 axis_map_x;
 	u8 axis_map_y;
 	u8 axis_map_z;
 
-	/* each axis can be negated to account for sensor orientation within
-	the host device; 1 = negate this axis; 0 = do not negate this axis */
+	/*
+	 * Each axis can be negated to account for sensor orientation within
+	 * the host device.
+	 */
 	bool negate_x;
 	bool negate_y;
 	bool negate_z;
@@ -70,7 +72,5 @@ struct kxtj9_platform_data {
 	int (*power_on)(void);
 	int (*power_off)(void);
 };
-#endif /* __KERNEL__ */
-
 #endif  /* __KXTJ9_H__ */
 

Re: [PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[Christopher Hudson]

On Mon, Jul 4, 2011 at 12:26 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Chris,
>
> On Tue, Jun 28, 2011 at 03:12:28PM -0400, chris.hudson.comp.eng@gmail.com wrote:
>> From: Chris Hudson <chudson@kionix.com>
>>
>> - Added Dmitry's changes
>> - Now using input_polled_dev interface when in polling mode
>> - IRQ mode provides a separate sysfs node to change the hardware data rate
>>
>
> I am not happy with the fact that this implementation forces users to
> select polled or interrupt-driven mode at compile time. The patch I
> proposed had polled mode support optional and would automatically select
> IRQ mode for clients that have interrupt assigned and try to activate
> polled mode if interrupt is not available.
>
>> +
>> +/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
>> +static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
>> +                                             const char *buf, size_t count)
>> +{
>> +     struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> +     unsigned long interval;
>> +     int ret = kstrtoul(buf, 10, &interval);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     mutex_lock(&tj9->lock);
>> +     /* set current interval to the greater of the minimum interval or
>> +     the requested interval */
>> +     tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
>> +     mutex_unlock(&tj9->lock);
>
> This lock does not make sense. You are protecting update of last_poll_interval
> field and this update can not be partial (i.e. only LSB or MSB is
> written) on all our arches, but you do not protect kxtj9_update_odr()
> which alters chip state and does need protection.
>
> I tried bringing forward my changes from the older patch into yours,
> please let me know if the patch below works on top of yours.

Hi Dmitry,
Thanks for all your hard work, and yes it works fine.  There were a
couple of changes proposed by Jonathan that I had already merged into
my version though; should I submit these in a separate patch or resend
the merged version?
Thank you,
Chris

>
> Thanks.
>
> --
> Dmitry
>
>
> Input: ktxj9 - misc fixes
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
>  drivers/input/misc/Kconfig  |   34 ++--
>  drivers/input/misc/kxtj9.c  |  355 +++++++++++++++++++++++--------------------
>  include/linux/input/kxtj9.h |   18 +-
>  3 files changed, 220 insertions(+), 187 deletions(-)
>
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 41628d2..0134428 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -230,6 +230,23 @@ config INPUT_KEYSPAN_REMOTE
>          To compile this driver as a module, choose M here: the module will
>          be called keyspan_remote.
>
> +config INPUT_KXTJ9
> +       tristate "Kionix KXTJ9 tri-axis digital accelerometer"
> +       depends on I2C
> +       help
> +         Say Y here to enable support for the Kionix KXTJ9 digital tri-axis
> +         accelerometer.
> +
> +         To compile this driver as a module, choose M here: the module will
> +         be called kxtj9.
> +
> +config INPUT_KXTJ9_POLLED_MODE
> +       bool "Enable polling mode support"
> +       depends on INPUT_KXTJ9
> +       select INPUT_POLLDEV
> +       help
> +         Say Y here if you need accelerometer to work in polling mode.
> +
>  config INPUT_POWERMATE
>        tristate "Griffin PowerMate and Contour Jog support"
>        depends on USB_ARCH_HAS_HCD
> @@ -499,21 +516,4 @@ config INPUT_XEN_KBDDEV_FRONTEND
>          To compile this driver as a module, choose M here: the
>          module will be called xen-kbdfront.
>
> -config INPUT_KXTJ9
> -       tristate "Kionix KXTJ9 tri-axis digital accelerometer"
> -       depends on I2C
> -       help
> -         If you say yes here you get support for the Kionix KXTJ9 digital
> -         tri-axis accelerometer.
> -
> -         This driver can also be built as a module.  If so, the module
> -         will be called kxtj9.
> -
> -config KXTJ9_POLLED_MODE
> -       bool "Enable polling mode support"
> -       depends on INPUT_KXTJ9
> -       select INPUT_POLLDEV
> -       help
> -         Say Y here if you need accelerometer to work in polling mode.
> -
>  endif
> diff --git a/drivers/input/misc/kxtj9.c b/drivers/input/misc/kxtj9.c
> index 7faf155..31eae6a 100644
> --- a/drivers/input/misc/kxtj9.c
> +++ b/drivers/input/misc/kxtj9.c
> @@ -75,10 +75,8 @@ struct kxtj9_data {
>        struct i2c_client *client;
>        struct kxtj9_platform_data pdata;
>        struct input_dev *input_dev;
> -#ifdef CONFIG_KXTJ9_POLLED_MODE
> +#ifdef CONFIG_INPUT_KXTJ9_POLLED_MODE
>        struct input_polled_dev *poll_dev;
> -#else
> -       struct mutex lock;
>  #endif
>        unsigned int last_poll_interval;
>        u8 shift;
> @@ -117,16 +115,32 @@ static void kxtj9_report_acceleration_data(struct kxtj9_data *tj9)
>        if (err < 0)
>                dev_err(&tj9->client->dev, "accelerometer data read failed\n");
>
> -       x = le16_to_cpu(acc_data[0]) >> tj9->shift;
> -       y = le16_to_cpu(acc_data[1]) >> tj9->shift;
> -       z = le16_to_cpu(acc_data[2]) >> tj9->shift;
> +       x = le16_to_cpu(acc_data[tj9->pdata.axis_map_x]) >> tj9->shift;
> +       y = le16_to_cpu(acc_data[tj9->pdata.axis_map_y]) >> tj9->shift;
> +       z = le16_to_cpu(acc_data[tj9->pdata.axis_map_z]) >> tj9->shift;
>
>        input_report_abs(tj9->input_dev, ABS_X, tj9->pdata.negate_x ? -x : x);
>        input_report_abs(tj9->input_dev, ABS_Y, tj9->pdata.negate_y ? -y : y);
> -       input_report_abs(tj9->input_dev, ABS_Z, tj9->pdata.negate_x ? -y : y);
> +       input_report_abs(tj9->input_dev, ABS_Z, tj9->pdata.negate_z ? -z : z);
>        input_sync(tj9->input_dev);
>  }
>
> +static irqreturn_t kxtj9_isr(int irq, void *dev)
> +{
> +       struct kxtj9_data *tj9 = dev;
> +       int err;
> +
> +       /* data ready is the only possible interrupt type */
> +       kxtj9_report_acceleration_data(tj9);
> +
> +       err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
> +       if (err < 0)
> +               dev_err(&tj9->client->dev,
> +                       "error clearing interrupt status: %d\n", err);
> +
> +       return IRQ_HANDLED;
> +}
> +
>  static int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range)
>  {
>        switch (new_g_range) {
> @@ -152,7 +166,7 @@ static int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range)
>        return 0;
>  }
>
> -static int kxtj9_update_odr(struct kxtj9_data *tj9, int poll_interval)
> +static int kxtj9_update_odr(struct kxtj9_data *tj9, unsigned int poll_interval)
>  {
>        int err;
>        int i;
> @@ -245,7 +259,7 @@ static int kxtj9_enable(struct kxtj9_data *tj9)
>                err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
>                if (err < 0) {
>                        dev_err(&tj9->client->dev,
> -                                       "error clearing interrupt: %d\n", err);
> +                               "error clearing interrupt: %d\n", err);
>                        goto fail;
>                }
>        }
> @@ -257,8 +271,27 @@ fail:
>        return err;
>  }
>
> +static void kxtj9_disable(struct kxtj9_data *tj9)
> +{
> +       kxtj9_device_power_off(tj9);
> +}
> +
> +static int kxtj9_input_open(struct input_dev *input)
> +{
> +       struct kxtj9_data *tj9 = input_get_drvdata(input);
> +
> +       return kxtj9_enable(tj9);
> +}
> +
> +static void kxtj9_input_close(struct input_dev *dev)
> +{
> +       struct kxtj9_data *tj9 = input_get_drvdata(dev);
> +
> +       kxtj9_disable(tj9);
> +}
> +
>  static void __devinit kxtj9_init_input_device(struct kxtj9_data *tj9,
> -                                               struct input_dev *input_dev)
> +                                             struct input_dev *input_dev)
>  {
>        __set_bit(EV_ABS, input_dev->evbit);
>        input_set_abs_params(input_dev, ABS_X, -G_MAX, G_MAX, FUZZ, FLAT);
> @@ -270,7 +303,104 @@ static void __devinit kxtj9_init_input_device(struct kxtj9_data *tj9,
>        input_dev->dev.parent = &tj9->client->dev;
>  }
>
> -#ifdef CONFIG_KXTJ9_POLLED_MODE
> +static int __devinit kxtj9_setup_input_device(struct kxtj9_data *tj9)
> +{
> +       struct input_dev *input_dev;
> +       int err;
> +
> +       input_dev = input_allocate_device();
> +       if (!tj9->input_dev) {
> +               dev_err(&tj9->client->dev, "input device allocate failed\n");
> +               return -ENOMEM;
> +       }
> +
> +       tj9->input_dev = input_dev;
> +
> +       input_dev->open = kxtj9_input_open;
> +       input_dev->close = kxtj9_input_close;
> +       input_set_drvdata(input_dev, tj9);
> +
> +       kxtj9_init_input_device(tj9, input_dev);
> +
> +       err = input_register_device(tj9->input_dev);
> +       if (err) {
> +               dev_err(&tj9->client->dev,
> +                       "unable to register input polled device %s: %d\n",
> +                       tj9->input_dev->name, err);
> +               input_free_device(tj9->input_dev);
> +               return err;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * When IRQ mode is selected, we need to provide an interface to allow the user
> + * to change the output data rate of the part.  For consistency, we are using
> + * the set_poll method, which accepts a poll interval in milliseconds, and then
> + * calls update_odr() while passing this value as an argument.  In IRQ mode, the
> + * data outputs will not be read AT the requested poll interval, rather, the
> + * lowest ODR that can support the requested interval.  The client application
> + * will be responsible for retrieving data from the input node at the desired
> + * interval.
> + */
> +
> +/* Returns currently selected poll interval (in ms) */
> +static ssize_t kxtj9_get_poll(struct device *dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct kxtj9_data *tj9 = i2c_get_clientdata(client);
> +
> +       return sprintf(buf, "%d\n", tj9->last_poll_interval);
> +}
> +
> +/* Allow users to select a new poll interval (in ms) */
> +static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
> +                                               const char *buf, size_t count)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct kxtj9_data *tj9 = i2c_get_clientdata(client);
> +       struct input_dev *input_dev = tj9->input_dev;
> +       unsigned int interval;
> +       int error;
> +
> +       error = kstrtouint(buf, 10, &interval);
> +       if (error < 0)
> +               return error;
> +
> +       /* Lock the device to prevent races with open/close (and itself) */
> +       mutex_unlock(&input_dev->mutex);
> +
> +       disable_irq(client->irq);
> +
> +       /*
> +        * Set current interval to the greater of the minimum interval or
> +        * the requested interval
> +        */
> +       tj9->last_poll_interval = max(interval, tj9->pdata.min_interval);
> +
> +       kxtj9_update_odr(tj9, tj9->last_poll_interval);
> +
> +       enable_irq(client->irq);
> +       mutex_unlock(&input_dev->mutex);
> +
> +       return count;
> +}
> +
> +static DEVICE_ATTR(poll, S_IRUGO|S_IWUSR, kxtj9_get_poll, kxtj9_set_poll);
> +
> +static struct attribute *kxtj9_attributes[] = {
> +       &dev_attr_poll.attr,
> +       NULL
> +};
> +
> +static struct attribute_group kxtj9_attribute_group = {
> +       .attrs = kxtj9_attributes
> +};
> +
> +
> +#ifdef CONFIG_INPUT_KXTJ9_POLLED_MODE
>  static void kxtj9_poll(struct input_polled_dev *dev)
>  {
>        struct kxtj9_data *tj9 = dev->private;
> @@ -295,7 +425,7 @@ static void kxtj9_polled_input_close(struct input_polled_dev *dev)
>  {
>        struct kxtj9_data *tj9 = dev->private;
>
> -       kxtj9_device_power_off(tj9);
> +       kxtj9_disable(tj9);
>  }
>
>  static int __devinit kxtj9_setup_polled_device(struct kxtj9_data *tj9)
> @@ -325,7 +455,7 @@ static int __devinit kxtj9_setup_polled_device(struct kxtj9_data *tj9)
>                dev_err(&tj9->client->dev,
>                        "Unable to register polled device, err=%d\n", err);
>                input_free_polled_device(poll_dev);
> -               return 0;
> +               return err;
>        }
>
>        return 0;
> @@ -337,66 +467,7 @@ static void kxtj9_teardown_polled_device(struct kxtj9_data *tj9)
>        input_free_polled_device(tj9->poll_dev);
>  }
>
> -#else  /* IRQ Mode is enabled */
> -static int kxtj9_input_open(struct input_dev *input)
> -{
> -       struct kxtj9_data *tj9 = input_get_drvdata(input);
> -
> -       return kxtj9_enable(tj9);
> -}
> -
> -static void kxtj9_input_close(struct input_dev *dev)
> -{
> -       struct kxtj9_data *tj9 = input_get_drvdata(dev);
> -
> -       kxtj9_device_power_off(tj9);
> -}
> -
> -static int __devinit kxtj9_setup_input_device(struct kxtj9_data *tj9)
> -{
> -       struct input_dev *input_dev;
> -       int err;
> -
> -       input_dev = input_allocate_device();
> -       if (!tj9->input_dev) {
> -               dev_err(&tj9->client->dev, "input device allocate failed\n");
> -               return -ENOMEM;
> -       }
> -
> -       tj9->input_dev = input_dev;
> -
> -       input_dev->open = kxtj9_input_open;
> -       input_dev->close = kxtj9_input_close;
> -       input_set_drvdata(input_dev, tj9);
> -
> -       kxtj9_init_input_device(tj9, input_dev);
> -
> -       err = input_register_device(tj9->input_dev);
> -       if (err) {
> -               dev_err(&tj9->client->dev,
> -                       "unable to register input polled device %s: %d\n",
> -                       tj9->input_dev->name, err);
> -               input_free_device(tj9->input_dev);
> -       }
> -
> -       return 0;
> -}
> -
> -static irqreturn_t kxtj9_isr(int irq, void *dev)
> -{
> -       struct kxtj9_data *tj9 = dev;
> -       int err;
> -
> -       /* data ready is the only possible interrupt type */
> -       kxtj9_report_acceleration_data(tj9);
> -
> -       err = i2c_smbus_read_byte_data(tj9->client, INT_REL);
> -       if (err < 0)
> -               dev_err(&tj9->client->dev,
> -                               "error clearing interrupt status: %d\n", err);
> -
> -       return IRQ_HANDLED;
> -}
> +#else
>
>  static inline int kxtj9_setup_polled_device(struct kxtj9_data *tj9)
>  {
> @@ -407,67 +478,22 @@ static inline void kxtj9_teardown_polled_device(struct kxtj9_data *tj9)
>  {
>  }
>
> -/* kxtj9_get_poll: returns the currently selected poll interval (in ms) */
> -static ssize_t kxtj9_get_poll(struct device *dev,
> -                               struct device_attribute *attr, char *buf)
> -{
> -       struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
> -       return sprintf(buf, "%d\n", tj9->last_poll_interval);
> -}
> -
> -/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
> -static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
> -                                               const char *buf, size_t count)
> -{
> -       struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
> -
> -       unsigned long interval;
> -       int ret = kstrtoul(buf, 10, &interval);
> -       if (ret < 0)
> -               return ret;
> -
> -       mutex_lock(&tj9->lock);
> -       /* set current interval to the greater of the minimum interval or
> -       the requested interval */
> -       tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
> -       mutex_unlock(&tj9->lock);
> -
> -       kxtj9_update_odr(tj9, tj9->last_poll_interval);
> -
> -       return count;
> -}
> -/* When IRQ mode is selected, we need to provide an interface to allow the user
> - * to change the output data rate of the part.  For consistency, we are using
> - * the set_poll method, which accepts a poll interval in milliseconds, and then
> - * calls update_odr() while passing this value as an argument.  In IRQ mode, the
> - * data outputs will not be read AT the requested poll interval, rather, the
> - * lowest ODR that can support the requested interval.  The client application
> - * will be responsible for retrieving data from the input node at the desired
> - * interval.
> - */
> -static DEVICE_ATTR(poll, S_IRUGO|S_IWUSR, kxtj9_get_poll, kxtj9_set_poll);
> -
> -static struct attribute *kxtj9_attributes[] = {
> -       &dev_attr_poll.attr,
> -       NULL
> -};
> -
> -static struct attribute_group kxtj9_attribute_group = {
> -       .attrs = kxtj9_attributes
> -};
>  #endif
>
>  static int __devinit kxtj9_verify(struct kxtj9_data *tj9)
>  {
>        int retval;
> +
>        retval = kxtj9_device_power_on(tj9);
>        if (retval < 0)
>                return retval;
> +
>        retval = i2c_smbus_read_byte_data(tj9->client, WHO_AM_I);
>        if (retval < 0) {
>                dev_err(&tj9->client->dev, "read err int source\n");
>                goto out;
>        }
> +
>        retval = retval != 0x06 ? -EIO : 0;
>
>  out:
> @@ -476,7 +502,7 @@ out:
>  }
>
>  static int __devinit kxtj9_probe(struct i2c_client *client,
> -                               const struct i2c_device_id *id)
> +                                const struct i2c_device_id *id)
>  {
>        const struct kxtj9_platform_data *pdata = client->dev.platform_data;
>        struct kxtj9_data *tj9;
> @@ -487,10 +513,12 @@ static int __devinit kxtj9_probe(struct i2c_client *client,
>                dev_err(&client->dev, "client is not i2c capable\n");
>                return -ENXIO;
>        }
> +
>        if (!pdata) {
>                dev_err(&client->dev, "platform data is NULL; exiting\n");
>                return -EINVAL;
>        }
> +
>        tj9 = kzalloc(sizeof(*tj9), GFP_KERNEL);
>        if (!tj9) {
>                dev_err(&client->dev,
> @@ -513,42 +541,46 @@ static int __devinit kxtj9_probe(struct i2c_client *client,
>                goto err_pdata_exit;
>        }
>
> +       i2c_set_clientdata(client, tj9);
> +
>        tj9->ctrl_reg1 = tj9->pdata.res_12bit | tj9->pdata.g_range;
>        tj9->data_ctrl = tj9->pdata.data_odr_init;
>
> -#ifdef CONFIG_KXTJ9_POLLED_MODE
> -       err = kxtj9_setup_polled_device(tj9);
> -       if (err)
> -               goto err_pdata_exit;
> -#else
> -       /* If in irq mode, populate INT_CTRL_REG1 and enable DRDY. */
> -       tj9->int_ctrl |= KXTJ9_IEN | KXTJ9_IEA | KXTJ9_IEL;
> -       tj9->ctrl_reg1 |= DRDYE;
> +       if (client->irq) {
> +               /* If in irq mode, populate INT_CTRL_REG1 and enable DRDY. */
> +               tj9->int_ctrl |= KXTJ9_IEN | KXTJ9_IEA | KXTJ9_IEL;
> +               tj9->ctrl_reg1 |= DRDYE;
> +
> +               err = kxtj9_setup_input_device(tj9);
> +               if (err)
> +                       goto err_pdata_exit;
> +
> +               err = request_threaded_irq(client->irq, NULL, kxtj9_isr,
> +                                          IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +                                          "kxtj9-irq", tj9);
> +               if (err) {
> +                       dev_err(&client->dev, "request irq failed: %d\n", err);
> +                       goto err_destroy_input;
> +               }
>
> -       err = kxtj9_setup_input_device(tj9);
> -       if (err)
> -               goto err_pdata_exit;
> +               err = sysfs_create_group(&client->dev.kobj, &kxtj9_attribute_group);
> +               if (err) {
> +                       dev_err(&client->dev, "sysfs create failed: %d\n", err);
> +                       goto err_free_irq;
> +               }
>
> -       err = request_threaded_irq(client->irq, NULL, kxtj9_isr,
> -                               IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> -                               "kxtj9-irq", tj9);
> -       if (err) {
> -               dev_err(&client->dev, "request irq failed: %d\n", err);
> -               goto err_destroy_input;
> -       }
> -       err = sysfs_create_group(&client->dev.kobj, &kxtj9_attribute_group);
> -       if (err) {
> -               dev_err(&client->dev, "sysfs create failed: %d\n", err);
> -               goto err_destroy_input;
> +       } else {
> +               err = kxtj9_setup_polled_device(tj9);
> +               if (err)
> +                       goto err_pdata_exit;
>        }
> -#endif
> -       i2c_set_clientdata(client, tj9);
> +
>        return 0;
>
> -#ifndef CONFIG_KXTJ9_POLLED_MODE
> +err_free_irq:
> +       free_irq(client->irq, tj9);
>  err_destroy_input:
>        input_unregister_device(tj9->input_dev);
> -#endif
>  err_pdata_exit:
>        if (tj9->pdata.exit)
>                tj9->pdata.exit();
> @@ -561,13 +593,14 @@ static int __devexit kxtj9_remove(struct i2c_client *client)
>  {
>        struct kxtj9_data *tj9 = i2c_get_clientdata(client);
>
> -#ifdef CONFIG_KXTJ9_POLLED_MODE
> -       kxtj9_teardown_polled_device(tj9);
> -#else
> -       sysfs_remove_group(&client->dev.kobj, &kxtj9_attribute_group);
> -       free_irq(client->irq, tj9);
> -       input_unregister_device(tj9->input_dev);
> -#endif
> +       if (client->irq) {
> +               sysfs_remove_group(&client->dev.kobj, &kxtj9_attribute_group);
> +               free_irq(client->irq, tj9);
> +               input_unregister_device(tj9->input_dev);
> +       } else {
> +               kxtj9_teardown_polled_device(tj9);
> +       }
> +
>        if (tj9->pdata.exit)
>                tj9->pdata.exit();
>
> @@ -620,13 +653,13 @@ MODULE_DEVICE_TABLE(i2c, kxtj9_id);
>
>  static struct i2c_driver kxtj9_driver = {
>        .driver = {
> -                 .name = NAME,
> -                 .owner = THIS_MODULE,
> -                 .pm = &kxtj9_pm_ops,
> +               .name   = NAME,
> +               .owner  = THIS_MODULE,
> +               .pm     = &kxtj9_pm_ops,
>        },
> -       .probe = kxtj9_probe,
> -       .remove = __devexit_p(kxtj9_remove),
> -       .id_table = kxtj9_id,
> +       .probe          = kxtj9_probe,
> +       .remove         = __devexit_p(kxtj9_remove),
> +       .id_table       = kxtj9_id,
>  };
>
>  static int __init kxtj9_init(void)
> diff --git a/include/linux/input/kxtj9.h b/include/linux/input/kxtj9.h
> index cc5928b..0b546bd 100644
> --- a/include/linux/input/kxtj9.h
> +++ b/include/linux/input/kxtj9.h
> @@ -27,19 +27,21 @@
>  #define SHIFT_ADJ_4G           3
>  #define SHIFT_ADJ_8G           2
>
> -#ifdef __KERNEL__
>  struct kxtj9_platform_data {
> -       int poll_interval;      /* current poll interval (in milli-seconds) */
> -       int min_interval;       /* minimum poll interval (in milli-seconds) */
> +       unsigned int min_interval;      /* minimum poll interval (in milli-seconds) */
>
> -       /* by default, x is axis 0, y is axis 1, z is axis 2; these can be
> -       changed to account for sensor orientation within the host device */
> +       /*
> +        * By default, x is axis 0, y is axis 1, z is axis 2; these can be
> +        * changed to account for sensor orientation within the host device.
> +        */
>        u8 axis_map_x;
>        u8 axis_map_y;
>        u8 axis_map_z;
>
> -       /* each axis can be negated to account for sensor orientation within
> -       the host device; 1 = negate this axis; 0 = do not negate this axis */
> +       /*
> +        * Each axis can be negated to account for sensor orientation within
> +        * the host device.
> +        */
>        bool negate_x;
>        bool negate_y;
>        bool negate_z;
> @@ -70,7 +72,5 @@ struct kxtj9_platform_data {
>        int (*power_on)(void);
>        int (*power_off)(void);
>  };
> -#endif /* __KERNEL__ */
> -
>  #endif  /* __KXTJ9_H__ */
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[Dmitry Torokhov]

On Tue, Jul 05, 2011 at 02:08:01PM -0400, Christopher Hudson wrote:
> On Mon, Jul 4, 2011 at 12:26 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Chris,
> >
> > On Tue, Jun 28, 2011 at 03:12:28PM -0400, chris.hudson.comp.eng@gmail.com wrote:
> >> From: Chris Hudson <chudson@kionix.com>
> >>
> >> - Added Dmitry's changes
> >> - Now using input_polled_dev interface when in polling mode
> >> - IRQ mode provides a separate sysfs node to change the hardware data rate
> >>
> >
> > I am not happy with the fact that this implementation forces users to
> > select polled or interrupt-driven mode at compile time. The patch I
> > proposed had polled mode support optional and would automatically select
> > IRQ mode for clients that have interrupt assigned and try to activate
> > polled mode if interrupt is not available.
> >
> >> +
> >> +/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
> >> +static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
> >> +                                             const char *buf, size_t count)
> >> +{
> >> +     struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
> >> +
> >> +     unsigned long interval;
> >> +     int ret = kstrtoul(buf, 10, &interval);
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     mutex_lock(&tj9->lock);
> >> +     /* set current interval to the greater of the minimum interval or
> >> +     the requested interval */
> >> +     tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
> >> +     mutex_unlock(&tj9->lock);
> >
> > This lock does not make sense. You are protecting update of last_poll_interval
> > field and this update can not be partial (i.e. only LSB or MSB is
> > written) on all our arches, but you do not protect kxtj9_update_odr()
> > which alters chip state and does need protection.
> >
> > I tried bringing forward my changes from the older patch into yours,
> > please let me know if the patch below works on top of yours.
> 
> Hi Dmitry,
> Thanks for all your hard work, and yes it works fine.  There were a
> couple of changes proposed by Jonathan that I had already merged into
> my version though; should I submit these in a separate patch or resend
> the merged version?

Cris,

Could you please send the changes proposed by Jonathan as a separate
patch relative to the v5 you posted earlier? Then I can fold v5, mine
and your new patch together and queue the driver for 3.1.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[Christopher Hudson]

Hi Dmitry,

I found a small bug in this driver when using the code as the basis
for testing another piece of hardware...I'm not sure how this got
through before:

+static int __devinit kxtj9_setup_input_device(struct kxtj9_data *tj9)
+{
+       struct input_dev *input_dev;
+       int err;
+
+       input_dev = input_allocate_device();
+       if (!tj9->input_dev) {
Should be:

if (!input_dev) {

if (!tj9->input_dev) always returns true in this context, thereby
always following the error path; you can see that nothing is assigned
to tj9->input_dev until after this check.

+               dev_err(&tj9->client->dev, "input device allocate failed\n");
+               return -ENOMEM;
+       }
+
+       tj9->input_dev = input_dev;

Should I submit a small patch against your queued version to get this fixed?

On Tue, Jul 5, 2011 at 2:39 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Jul 05, 2011 at 02:08:01PM -0400, Christopher Hudson wrote:
>> On Mon, Jul 4, 2011 at 12:26 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > Hi Chris,
>> >
>> > On Tue, Jun 28, 2011 at 03:12:28PM -0400, chris.hudson.comp.eng@gmail.com wrote:
>> >> From: Chris Hudson <chudson@kionix.com>
>> >>
>> >> - Added Dmitry's changes
>> >> - Now using input_polled_dev interface when in polling mode
>> >> - IRQ mode provides a separate sysfs node to change the hardware data rate
>> >>
>> >
>> > I am not happy with the fact that this implementation forces users to
>> > select polled or interrupt-driven mode at compile time. The patch I
>> > proposed had polled mode support optional and would automatically select
>> > IRQ mode for clients that have interrupt assigned and try to activate
>> > polled mode if interrupt is not available.
>> >
>> >> +
>> >> +/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
>> >> +static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
>> >> +                                             const char *buf, size_t count)
>> >> +{
>> >> +     struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
>> >> +
>> >> +     unsigned long interval;
>> >> +     int ret = kstrtoul(buf, 10, &interval);
>> >> +     if (ret < 0)
>> >> +             return ret;
>> >> +
>> >> +     mutex_lock(&tj9->lock);
>> >> +     /* set current interval to the greater of the minimum interval or
>> >> +     the requested interval */
>> >> +     tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
>> >> +     mutex_unlock(&tj9->lock);
>> >
>> > This lock does not make sense. You are protecting update of last_poll_interval
>> > field and this update can not be partial (i.e. only LSB or MSB is
>> > written) on all our arches, but you do not protect kxtj9_update_odr()
>> > which alters chip state and does need protection.
>> >
>> > I tried bringing forward my changes from the older patch into yours,
>> > please let me know if the patch below works on top of yours.
>>
>> Hi Dmitry,
>> Thanks for all your hard work, and yes it works fine.  There were a
>> couple of changes proposed by Jonathan that I had already merged into
>> my version though; should I submit these in a separate patch or resend
>> the merged version?
>
> Cris,
>
> Could you please send the changes proposed by Jonathan as a separate
> patch relative to the v5 you posted earlier? Then I can fold v5, mine
> and your new patch together and queue the driver for 3.1.
>
> Thanks.
>
> --
> Dmitry
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

[Dmitry Torokhov]

On Tuesday, July 12, 2011 02:52:20 PM Christopher Hudson wrote:
> Should I submit a small patch against your queued version to get this
> fixed?

That'd be great since you could verify that it actually runs
on the hardware.

Thanks.

-- 
Dmitry