[PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver (third version)

[PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver (third version)

[Fabien Marteau]

 Driver for EasyPoint AS5011 2 axis joystick chip. This chip is plugged
 on an I2C bus. It has been tested on ARM processor (i.MX27).

Third version, according to Dmitry Torokhov and Trilok Soni comments.


Signed-off-by: Fabien Marteau <fabien.marteau@armadeus.com>
---
 drivers/input/joystick/Kconfig  |    9 +
 drivers/input/joystick/Makefile |    1 +
 drivers/input/joystick/as5011.c |  377 +++++++++++++++++++++++++++++++++++++++
 include/linux/as5011.h          |   35 ++++
 4 files changed, 422 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/joystick/as5011.c
 create mode 100644 include/linux/as5011.h

diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index 5b59616..5fad03e 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -255,6 +255,15 @@ config JOYSTICK_AMIGA
 	  To compile this driver as a module, choose M here: the
 	  module will be called amijoy.

+config JOYSTICK_AS5011
+	tristate "Austria Microsystem AS5011 joystick"
+	depends on I2C
+	help
+	  Say Y here if you have an AS5011 digital joystick.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called as5011.
+
 config JOYSTICK_JOYDUMP
 	tristate "Gameport data dumper"
 	select GAMEPORT
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index f3a8cbe..92dc0de 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -7,6 +7,7 @@
 obj-$(CONFIG_JOYSTICK_A3D)		+= a3d.o
 obj-$(CONFIG_JOYSTICK_ADI)		+= adi.o
 obj-$(CONFIG_JOYSTICK_AMIGA)		+= amijoy.o
+obj-$(CONFIG_JOYSTICK_AS5011)		+= as5011.o
 obj-$(CONFIG_JOYSTICK_ANALOG)		+= analog.o
 obj-$(CONFIG_JOYSTICK_COBRA)		+= cobra.o
 obj-$(CONFIG_JOYSTICK_DB9)		+= db9.o
diff --git a/drivers/input/joystick/as5011.c b/drivers/input/joystick/as5011.c
new file mode 100644
index 0000000..066a125
--- /dev/null
+++ b/drivers/input/joystick/as5011.c
@@ -0,0 +1,377 @@
+/*
+ * Copyright (c) 2010, 2011 Fabien Marteau <fabien.marteau@armadeus.com>
+ * Sponsored by ARMadeus Systems
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Driver for Austria Microsystems joysticks AS5011
+ *
+ * TODO:
+ *	- Manage power mode
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/string.h>
+#include <linux/list.h>
+#include <linux/sysfs.h>
+#include <linux/ctype.h>
+#include <linux/hwmon-sysfs.h>
+
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+
+#include <linux/input.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+
+#include <linux/as5011.h>
+#define DRIVER_DESC "Driver for Austria Microsystems AS5011 joystick"
+#define MODULE_DEVICE_ALIAS "as5011"
+
+MODULE_AUTHOR("Fabien Marteau <fabien.marteau@armadeus.com>");
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
+
+/* registers */
+#define AS5011_CTRL1		0x76
+#define AS5011_CTRL2		0x75
+#define AS5011_XP		0x43
+#define AS5011_XN		0x44
+#define AS5011_YP		0x53
+#define AS5011_YN		0x54
+#define AS5011_X_REG		0x41
+#define AS5011_Y_REG		0x42
+#define AS5011_X_RES_INT	0x51
+#define AS5011_Y_RES_INT	0x52
+
+/* CTRL1 bits */
+#define AS5011_CTRL1_LP_PULSED		0x80
+#define AS5011_CTRL1_LP_ACTIVE		0x40
+#define AS5011_CTRL1_LP_CONTINUE	0x20
+#define AS5011_CTRL1_INT_WUP_EN		0x10
+#define AS5011_CTRL1_INT_ACT_EN		0x08
+#define AS5011_CTRL1_EXT_CLK_EN		0x04
+#define AS5011_CTRL1_SOFT_RST		0x02
+#define AS5011_CTRL1_DATA_VALID		0x01
+
+/* CTRL2 bits */
+#define AS5011_CTRL2_EXT_SAMPLE_EN	0x08
+#define AS5011_CTRL2_RC_BIAS_ON		0x04
+#define AS5011_CTRL2_INV_SPINNING	0x02
+
+#define SIZE_NAME 30
+#define SIZE_EVENT_PATH 40
+#define AS5011_MAX_AXIS	80
+#define AS5011_MIN_AXIS	(-80)
+#define AS5011_FUZZ	8
+#define AS5011_FLAT	40
+
+static int as5011_i2c_write(struct i2c_client *client,
+			    uint8_t aregaddr,
+			    uint8_t avalue)
+{
+	int ret;
+	uint8_t data[2] = { aregaddr, avalue };
+
+	struct i2c_msg msg = {	client->addr,
+				I2C_M_IGNORE_NAK,
+				2,
+				(uint8_t *)data };
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+
+	if (ret < 0)
+		return ret;
+	return 1;
+}
+
+static signed char as5011_i2c_read(struct i2c_client *client,
+			   uint8_t aregaddr)
+{
+	int ret;
+	uint8_t data[2];
+	struct i2c_msg msg_set[2];
+	struct i2c_msg msg1 = {	client->addr,
+				I2C_M_REV_DIR_ADDR,
+				1,
+				(uint8_t *)data};
+	struct i2c_msg msg2 = {	client->addr,
+				I2C_M_RD|I2C_M_NOSTART,
+				1,
+				(uint8_t *)data};
+
+	data[0] = aregaddr;
+	msg_set[0] = msg1;
+	msg_set[1] = msg2;
+	ret = i2c_transfer(client->adapter, msg_set, 2);
+	if (ret < 0)
+		return ret;
+
+	if (data[0] & 0x80)
+		return -1*(1+(~data[0]));
+	else
+		return data[0];
+}
+
+static irqreturn_t button_interrupt(int irq, void *dev_id)
+{
+	struct as5011_platform_data *plat_dat = dev_id;
+	int ret;
+
+	ret = gpio_get_value(plat_dat->button_gpio);
+	if (ret)
+		input_report_key(plat_dat->input_dev, BTN_JOYSTICK, 0);
+	else
+		input_report_key(plat_dat->input_dev, BTN_JOYSTICK, 1);
+	input_sync(plat_dat->input_dev);
+
+	return IRQ_HANDLED;
+}
+
+static void as5011_update_axes(struct as5011_platform_data *plat_dat)
+{
+	signed char x, y;
+
+	x = as5011_i2c_read(plat_dat->i2c_client, AS5011_X_RES_INT);
+	y = as5011_i2c_read(plat_dat->i2c_client, AS5011_Y_RES_INT);
+	input_report_abs(plat_dat->input_dev, ABS_X, x);
+	input_report_abs(plat_dat->input_dev, ABS_Y, y);
+	input_sync(plat_dat->input_dev);
+}
+
+static irqreturn_t as5011_int_interrupt(int irq, void *dev_id)
+{
+	struct as5011_platform_data *plat_dat = dev_id;
+
+	mutex_lock(&plat_dat->update_lock);
+	as5011_update_axes(plat_dat);
+	mutex_unlock(&plat_dat->update_lock);
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit as5011_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct as5011_platform_data *plat_dat = client->dev.platform_data;
+	int retval = 0;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_PROTOCOL_MANGLING)) {
+		dev_err(&client->dev,
+		"i2c bus does not support protocol mangling, as5011 can't work\n");
+		retval = -ENODEV;
+		goto err_out;
+	}
+	plat_dat->i2c_client = client;
+
+
+	plat_dat->input_dev = input_allocate_device();
+	if (plat_dat->input_dev == NULL) {
+		dev_err(&client->dev,
+		"not enough memory for input devices structure\n");
+		retval = -ENOMEM;
+		goto err_out;
+	}
+
+	plat_dat->input_dev->name = "Austria Microsystem as5011 joystick";
+	plat_dat->input_dev->id.bustype = BUS_I2C;
+	__set_bit(EV_KEY, plat_dat->input_dev->evbit);
+	__set_bit(EV_ABS, plat_dat->input_dev->evbit);
+	__set_bit(BTN_JOYSTICK, plat_dat->input_dev->keybit);
+
+	input_set_abs_params(plat_dat->input_dev,
+			     ABS_X,
+			     AS5011_MIN_AXIS,
+			     AS5011_MAX_AXIS,
+			     AS5011_FUZZ,
+			     AS5011_FLAT);
+	input_set_abs_params(plat_dat->input_dev,
+			     ABS_Y,
+			     AS5011_MIN_AXIS,
+			     AS5011_MAX_AXIS,
+			     AS5011_FUZZ,
+			     AS5011_FLAT);
+
+	retval = request_threaded_irq(plat_dat->button_irq,
+					NULL, button_interrupt,
+					0, "as5011_button",
+					plat_dat);
+	if (retval < 0) {
+		dev_err(&client->dev, "Can't allocate irq %d\n",
+			plat_dat->button_irq);
+		retval = -EBUSY;
+		goto err_input_free_device;
+	}
+
+	if (plat_dat->init_gpio != NULL) {
+		retval = plat_dat->init_gpio();
+		if (retval < 0) {
+			dev_err(&client->dev, "Failed to init gpios\n");
+			goto err_free_irq_button_interrupt;
+		}
+	}
+
+	/* chip soft reset */
+	retval = as5011_i2c_write(plat_dat->i2c_client,
+				  AS5011_CTRL1,
+				  AS5011_CTRL1_SOFT_RST);
+	if (retval < 0) {
+		dev_err(&plat_dat->i2c_client->dev,
+			"Soft reset failed\n");
+		goto err_exit_gpio;
+	}
+
+	mdelay(10);
+
+	retval = as5011_i2c_write(plat_dat->i2c_client,
+				  AS5011_CTRL1,
+				  AS5011_CTRL1_LP_PULSED |
+				  AS5011_CTRL1_LP_ACTIVE |
+				  AS5011_CTRL1_INT_ACT_EN
+				  );
+	if (retval < 0) {
+		dev_err(&plat_dat->i2c_client->dev,
+			"Power config failed\n");
+		goto err_exit_gpio;
+	}
+
+	retval = as5011_i2c_write(plat_dat->i2c_client,
+				  AS5011_CTRL2,
+				  AS5011_CTRL2_INV_SPINNING);
+	if (retval < 0) {
+		dev_err(&plat_dat->i2c_client->dev,
+			"Can't invert spinning\n");
+		goto err_exit_gpio;
+	}
+
+	/* write threshold */
+	retval = as5011_i2c_write(plat_dat->i2c_client,
+				  AS5011_XP,
+				  plat_dat->xp);
+	if (retval < 0) {
+		dev_err(&plat_dat->i2c_client->dev,
+			"Can't write threshold\n");
+		goto err_exit_gpio;
+	}
+
+	retval = as5011_i2c_write(plat_dat->i2c_client,
+				  AS5011_XN,
+				  plat_dat->xn);
+	if (retval < 0) {
+		dev_err(&plat_dat->i2c_client->dev,
+			"Can't write threshold\n");
+		goto err_exit_gpio;
+	}
+
+	retval = as5011_i2c_write(plat_dat->i2c_client,
+				  AS5011_YP,
+				  plat_dat->yp);
+	if (retval < 0) {
+		dev_err(&plat_dat->i2c_client->dev,
+			"Can't write threshold\n");
+		goto err_exit_gpio;
+	}
+
+	retval = as5011_i2c_write(plat_dat->i2c_client,
+				  AS5011_YN,
+				  plat_dat->yn);
+	if (retval < 0) {
+		dev_err(&plat_dat->i2c_client->dev,
+			"Can't write threshold\n");
+		goto err_exit_gpio;
+	}
+
+	/* to free irq gpio in chip*/
+	as5011_i2c_read(plat_dat->i2c_client, AS5011_X_RES_INT);
+
+	/* initialize mutex */
+	mutex_init(&plat_dat->update_lock);
+
+	if (request_threaded_irq(plat_dat->int_irq, NULL,
+				as5011_int_interrupt,
+				0, "as5011_joystick", plat_dat)) {
+		dev_err(&client->dev, "Can't allocate int irq %d\n",
+			plat_dat->int_irq);
+		retval = -EBUSY;
+		goto err_exit_gpio;
+	}
+
+	retval = input_register_device(plat_dat->input_dev);
+	if (retval) {
+		dev_err(&client->dev, "Failed to register device\n");
+		goto err_free_irq_int;
+	}
+
+	return 0;
+
+	/* Error management */
+err_free_irq_int:
+	free_irq(plat_dat->int_irq, as5011_int_interrupt);
+err_exit_gpio:
+	if (plat_dat->exit_gpio != NULL)
+		plat_dat->exit_gpio();
+err_free_irq_button_interrupt:
+	free_irq(plat_dat->button_irq, button_interrupt);
+err_input_free_device:
+	input_free_device(plat_dat->input_dev);
+err_out:
+	return retval;
+}
+static int as5011_remove(struct i2c_client *client)
+{
+	struct as5011_platform_data *plat_dat = client->dev.platform_data;
+
+	input_unregister_device(plat_dat->input_dev);
+	free_irq(plat_dat->button_irq, plat_dat);
+	free_irq(plat_dat->int_irq, plat_dat);
+	if (plat_dat->exit_gpio != NULL)
+		plat_dat->exit_gpio();
+
+	return 0;
+}
+static const struct i2c_device_id as5011_id[] = {
+	{ MODULE_DEVICE_ALIAS, 0 },
+	{ }
+};
+
+static struct i2c_driver as5011_driver = {
+	.driver = {
+		.name = "as5011",
+	},
+	.probe		= as5011_probe,
+	.remove		= __devexit_p(as5011_remove),
+	.id_table	= as5011_id,
+};
+
+static int __init as5011_init(void)
+{
+	return i2c_add_driver(&as5011_driver);
+}
+
+static void __exit as5011_exit(void)
+{
+	i2c_del_driver(&as5011_driver);
+}
+
+module_init(as5011_init);
+module_exit(as5011_exit);
diff --git a/include/linux/as5011.h b/include/linux/as5011.h
new file mode 100644
index 0000000..48dfcb6
--- /dev/null
+++ b/include/linux/as5011.h
@@ -0,0 +1,35 @@
+#ifndef _AS5011_H
+#define _AS5011_H
+
+/*
+ * Copyright (c) 2010, 2011 Fabien Marteau <fabien.marteau@armadeus.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.
+ */
+
+#include <linux/mutex.h>
+
+#define AS5011_MAX_NAME_LENGTH	64
+
+struct as5011_platform_data {
+	/* public */
+	int button_gpio;
+	int button_irq;
+	int int_gpio;
+	int int_irq;
+	char xp, xn; /* threshold for x axis */
+	char yp, yn; /* threshold for y axis */
+
+	int (*init_gpio)(void); /* init interrupts gpios */
+	void (*exit_gpio)(void);/* exit gpios */
+
+	/* private */
+	struct input_dev *input_dev;
+	struct i2c_client *i2c_client;
+	char name[AS5011_MAX_NAME_LENGTH];
+	struct mutex update_lock;
+};
+
+#endif /* _AS5011_H */
-- 
1.7.0.4

Re: [PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver (third version)

[Trilok Soni]

Hi Fabien,

On 1/4/2011 3:17 PM, Fabien Marteau wrote:
>  Driver for EasyPoint AS5011 2 axis joystick chip. This chip is plugged
>  on an I2C bus. It has been tested on ARM processor (i.MX27).
> 
> Third version, according to Dmitry Torokhov and Trilok Soni comments.
> 
> 
> Signed-off-by: Fabien Marteau <fabien.marteau@armadeus.com>
> ---

Looks good. Few comments.

>  drivers/input/joystick/Kconfig  |    9 +
>  drivers/input/joystick/Makefile |    1 +
>  drivers/input/joystick/as5011.c |  377 +++++++++++++++++++++++++++++++++++++++
>  include/linux/as5011.h          |   35 ++++

How about moving it to include/linux/input?

> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/string.h>
> +#include <linux/list.h>
> +#include <linux/sysfs.h>
> +#include <linux/ctype.h>
> +#include <linux/hwmon-sysfs.h>

Are you using anything from this file? Please audit this list and remove
the files which are not used. Thanks.

> +
> +#define SIZE_NAME 30
> +#define SIZE_EVENT_PATH 40

I don't find anybody using these #defines. Please remove the defines which
are not used.

> +#define AS5011_MAX_AXIS	80
> +#define AS5011_MIN_AXIS	(-80)
> +#define AS5011_FUZZ	8
> +#define AS5011_FLAT	40
> +
> +static int as5011_i2c_write(struct i2c_client *client,
> +			    uint8_t aregaddr,
> +			    uint8_t avalue)
> +{
> +	int ret;
> +	uint8_t data[2] = { aregaddr, avalue };
> +
> +	struct i2c_msg msg = {	client->addr,
> +				I2C_M_IGNORE_NAK,
> +				2,
> +				(uint8_t *)data };
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +
> +	if (ret < 0)
> +		return ret;
> +	return 1;
> +}

We return '0' on success and negative error code for error.

> +
> +static irqreturn_t button_interrupt(int irq, void *dev_id)
> +{
> +	struct as5011_platform_data *plat_dat = dev_id;
> +	int ret;
> +
> +	ret = gpio_get_value(plat_dat->button_gpio);

I don't find gpio_request call for this gpio anywhere in this driver. I prefer
that we do that for each gpio we use in the driver itself. Only muxing stuff
can be left out in your init/exit hooks.

> +	if (ret)
> +		input_report_key(plat_dat->input_dev, BTN_JOYSTICK, 0);
> +	else
> +		input_report_key(plat_dat->input_dev, BTN_JOYSTICK, 1);
> +	input_sync(plat_dat->input_dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void as5011_update_axes(struct as5011_platform_data *plat_dat)
> +{
> +	signed char x, y;
> +
> +	x = as5011_i2c_read(plat_dat->i2c_client, AS5011_X_RES_INT);
> +	y = as5011_i2c_read(plat_dat->i2c_client, AS5011_Y_RES_INT);

I prefer that i2c_read wrapper here would return the actual error code instead,
and have one function argument to return the "x, y" co-ordinates, that way
we don't loose the error codes, and return from here itself if i2c read fails.

> +	input_report_abs(plat_dat->input_dev, ABS_X, x);
> +	input_report_abs(plat_dat->input_dev, ABS_Y, y);
> +	input_sync(plat_dat->input_dev);
> +}
> +
> +static irqreturn_t as5011_int_interrupt(int irq, void *dev_id)
> +{
> +	struct as5011_platform_data *plat_dat = dev_id;
> +
> +	mutex_lock(&plat_dat->update_lock);
> +	as5011_update_axes(plat_dat);
> +	mutex_unlock(&plat_dat->update_lock);

Please explain this lock. Sorry if I have missed out your explanation in the earlier threads.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __devinit as5011_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct as5011_platform_data *plat_dat = client->dev.platform_data;

const please. 

> +	int retval = 0;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_PROTOCOL_MANGLING)) {
> +		dev_err(&client->dev,
> +		"i2c bus does not support protocol mangling, as5011 can't work\n");
> +		retval = -ENODEV;
> +		goto err_out;
> +	}
> +	plat_dat->i2c_client = client;
> +
> +
> +	plat_dat->input_dev = input_allocate_device();
> +	if (plat_dat->input_dev == NULL) {
> +		dev_err(&client->dev,
> +		"not enough memory for input devices structure\n");
> +		retval = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	plat_dat->input_dev->name = "Austria Microsystem as5011 joystick";
> +	plat_dat->input_dev->id.bustype = BUS_I2C;
> +	__set_bit(EV_KEY, plat_dat->input_dev->evbit);
> +	__set_bit(EV_ABS, plat_dat->input_dev->evbit);
> +	__set_bit(BTN_JOYSTICK, plat_dat->input_dev->keybit);
> +
> +	input_set_abs_params(plat_dat->input_dev,
> +			     ABS_X,
> +			     AS5011_MIN_AXIS,
> +			     AS5011_MAX_AXIS,
> +			     AS5011_FUZZ,
> +			     AS5011_FLAT);
> +	input_set_abs_params(plat_dat->input_dev,
> +			     ABS_Y,
> +			     AS5011_MIN_AXIS,
> +			     AS5011_MAX_AXIS,
> +			     AS5011_FUZZ,
> +			     AS5011_FLAT);
> +
> +	retval = request_threaded_irq(plat_dat->button_irq,
> +					NULL, button_interrupt,
> +					0, "as5011_button",
> +					plat_dat);
> +	if (retval < 0) {
> +		dev_err(&client->dev, "Can't allocate irq %d\n",
> +			plat_dat->button_irq);
> +		retval = -EBUSY;

Why don't we return same error code instead of overwriting it here?

> +		goto err_input_free_device;
> +	}
> +
> +	if (plat_dat->init_gpio != NULL) {
> +		retval = plat_dat->init_gpio();
> +		if (retval < 0) {
> +			dev_err(&client->dev, "Failed to init gpios\n");
> +			goto err_free_irq_button_interrupt;
> +		}
> +	}
> +
> +	/* chip soft reset */
> +	retval = as5011_i2c_write(plat_dat->i2c_client,
> +				  AS5011_CTRL1,
> +				  AS5011_CTRL1_SOFT_RST);
> +	if (retval < 0) {
> +		dev_err(&plat_dat->i2c_client->dev,
> +			"Soft reset failed\n");
> +		goto err_exit_gpio;
> +	}
> +
> +	mdelay(10);
> +
> +	retval = as5011_i2c_write(plat_dat->i2c_client,
> +				  AS5011_CTRL1,
> +				  AS5011_CTRL1_LP_PULSED |
> +				  AS5011_CTRL1_LP_ACTIVE |
> +				  AS5011_CTRL1_INT_ACT_EN
> +				  );
> +	if (retval < 0) {
> +		dev_err(&plat_dat->i2c_client->dev,
> +			"Power config failed\n");
> +		goto err_exit_gpio;
> +	}
> +
> +	retval = as5011_i2c_write(plat_dat->i2c_client,
> +				  AS5011_CTRL2,
> +				  AS5011_CTRL2_INV_SPINNING);
> +	if (retval < 0) {
> +		dev_err(&plat_dat->i2c_client->dev,
> +			"Can't invert spinning\n");
> +		goto err_exit_gpio;
> +	}
> +
> +	/* write threshold */
> +	retval = as5011_i2c_write(plat_dat->i2c_client,
> +				  AS5011_XP,
> +				  plat_dat->xp);
> +	if (retval < 0) {
> +		dev_err(&plat_dat->i2c_client->dev,
> +			"Can't write threshold\n");
> +		goto err_exit_gpio;
> +	}
> +
> +	retval = as5011_i2c_write(plat_dat->i2c_client,
> +				  AS5011_XN,
> +				  plat_dat->xn);
> +	if (retval < 0) {
> +		dev_err(&plat_dat->i2c_client->dev,
> +			"Can't write threshold\n");
> +		goto err_exit_gpio;
> +	}
> +
> +	retval = as5011_i2c_write(plat_dat->i2c_client,
> +				  AS5011_YP,
> +				  plat_dat->yp);
> +	if (retval < 0) {
> +		dev_err(&plat_dat->i2c_client->dev,
> +			"Can't write threshold\n");
> +		goto err_exit_gpio;
> +	}
> +
> +	retval = as5011_i2c_write(plat_dat->i2c_client,
> +				  AS5011_YN,
> +				  plat_dat->yn);
> +	if (retval < 0) {
> +		dev_err(&plat_dat->i2c_client->dev,
> +			"Can't write threshold\n");
> +		goto err_exit_gpio;
> +	}
> +
> +	/* to free irq gpio in chip*/
> +	as5011_i2c_read(plat_dat->i2c_client, AS5011_X_RES_INT);

Would you prefer to move some of the initialization of the chip except
reset of controller to open() call, so that if there is no user of this
device than we don't need to leave this device left configured?

> +
> +	/* initialize mutex */
> +	mutex_init(&plat_dat->update_lock);
> +
> +	if (request_threaded_irq(plat_dat->int_irq, NULL,
> +				as5011_int_interrupt,
> +				0, "as5011_joystick", plat_dat)) {
> +		dev_err(&client->dev, "Can't allocate int irq %d\n",
> +			plat_dat->int_irq);
> +		retval = -EBUSY;
> +		goto err_exit_gpio;
> +	}
> +
> +	retval = input_register_device(plat_dat->input_dev);
> +	if (retval) {
> +		dev_err(&client->dev, "Failed to register device\n");
> +		goto err_free_irq_int;
> +	}
> +
> +	return 0;
> +
> +	/* Error management */
> +err_free_irq_int:
> +	free_irq(plat_dat->int_irq, as5011_int_interrupt);
> +err_exit_gpio:
> +	if (plat_dat->exit_gpio != NULL)
> +		plat_dat->exit_gpio();
> +err_free_irq_button_interrupt:
> +	free_irq(plat_dat->button_irq, button_interrupt);
> +err_input_free_device:
> +	input_free_device(plat_dat->input_dev);
> +err_out:
> +	return retval;
> +}
> +static int as5011_remove(struct i2c_client *client)
> +{
> +	struct as5011_platform_data *plat_dat = client->dev.platform_data;
> +
> +	input_unregister_device(plat_dat->input_dev);
> +	free_irq(plat_dat->button_irq, plat_dat);
> +	free_irq(plat_dat->int_irq, plat_dat);
> +	if (plat_dat->exit_gpio != NULL)
> +		plat_dat->exit_gpio();
> +
> +	return 0;
> +}
> +static const struct i2c_device_id as5011_id[] = {
> +	{ MODULE_DEVICE_ALIAS, 0 },
> +	{ }
> +};
> +
> +static struct i2c_driver as5011_driver = {
> +	.driver = {
> +		.name = "as5011",
> +	},
> +	.probe		= as5011_probe,
> +	.remove		= __devexit_p(as5011_remove),
> +	.id_table	= as5011_id,
> +};
> +
> +static int __init as5011_init(void)
> +{
> +	return i2c_add_driver(&as5011_driver);
> +}
> +
> +static void __exit as5011_exit(void)
> +{
> +	i2c_del_driver(&as5011_driver);
> +}
> +
> +module_init(as5011_init);
> +module_exit(as5011_exit);

nit-pick: I prefer that we put module_init/exit macros with their relevant functions.

> +#include <linux/mutex.h>
> +
> +#define AS5011_MAX_NAME_LENGTH	64
> +
> +struct as5011_platform_data {
> +	/* public */
> +	int button_gpio;
> +	int button_irq;
> +	int int_gpio;
> +	int int_irq;
> +	char xp, xn; /* threshold for x axis */
> +	char yp, yn; /* threshold for y axis */
> +
> +	int (*init_gpio)(void); /* init interrupts gpios */
> +	void (*exit_gpio)(void);/* exit gpios */
> +
> +	/* private */
> +	struct input_dev *input_dev;
> +	struct i2c_client *i2c_client;
> +	char name[AS5011_MAX_NAME_LENGTH];
> +	struct mutex update_lock;

These private stuff should not be part of your platform data structure. When we say
platform data meaning that it will be all public with "const". Please allocate
local data structure in the probe itself with these private members. You can refer
other drivers.


---Trilok Soni

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver (third version)

[Fabien Marteau]

On 04/01/2011 12:01, Trilok Soni wrote:
> Hi Fabien,
> 
> On 1/4/2011 3:17 PM, Fabien Marteau wrote:
>>  Driver for EasyPoint AS5011 2 axis joystick chip. This chip is plugged
>>  on an I2C bus. It has been tested on ARM processor (i.MX27).
>>
>> Third version, according to Dmitry Torokhov and Trilok Soni comments.
>>
>>
>> Signed-off-by: Fabien Marteau <fabien.marteau@armadeus.com>
>> ---
> 
> Looks good. Few comments.
> 
>>  drivers/input/joystick/Kconfig  |    9 +
>>  drivers/input/joystick/Makefile |    1 +
>>  drivers/input/joystick/as5011.c |  377 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/as5011.h          |   35 ++++
> 
> How about moving it to include/linux/input?

It's better yes.

> 
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/slab.h>
>> +#include <linux/i2c.h>
>> +#include <linux/string.h>
>> +#include <linux/list.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/ctype.h>
>> +#include <linux/hwmon-sysfs.h>
> 
> Are you using anything from this file? Please audit this list and remove
> the files which are not used. Thanks.

Done

> 
>> +
>> +#define SIZE_NAME 30
>> +#define SIZE_EVENT_PATH 40
> 
> I don't find anybody using these #defines. Please remove the defines which
> are not used.

Old useless macro, deleted.

> 
>> +#define AS5011_MAX_AXIS	80
>> +#define AS5011_MIN_AXIS	(-80)
>> +#define AS5011_FUZZ	8
>> +#define AS5011_FLAT	40
>> +
>> +static int as5011_i2c_write(struct i2c_client *client,
>> +			    uint8_t aregaddr,
>> +			    uint8_t avalue)
>> +{
>> +	int ret;
>> +	uint8_t data[2] = { aregaddr, avalue };
>> +
>> +	struct i2c_msg msg = {	client->addr,
>> +				I2C_M_IGNORE_NAK,
>> +				2,
>> +				(uint8_t *)data };
>> +
>> +	ret = i2c_transfer(client->adapter, &msg, 1);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +	return 1;
>> +}
> 
> We return '0' on success and negative error code for error.

Ok.

> 
>> +
>> +static irqreturn_t button_interrupt(int irq, void *dev_id)
>> +{
>> +	struct as5011_platform_data *plat_dat = dev_id;
>> +	int ret;
>> +
>> +	ret = gpio_get_value(plat_dat->button_gpio);
> 
> I don't find gpio_request call for this gpio anywhere in this driver. I prefer
> that we do that for each gpio we use in the driver itself. Only muxing stuff
> can be left out in your init/exit hooks.

Ok I added it and deleted init/exit hooks.

> 
>> +	if (ret)
>> +		input_report_key(plat_dat->input_dev, BTN_JOYSTICK, 0);
>> +	else
>> +		input_report_key(plat_dat->input_dev, BTN_JOYSTICK, 1);
>> +	input_sync(plat_dat->input_dev);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void as5011_update_axes(struct as5011_platform_data *plat_dat)
>> +{
>> +	signed char x, y;
>> +
>> +	x = as5011_i2c_read(plat_dat->i2c_client, AS5011_X_RES_INT);
>> +	y = as5011_i2c_read(plat_dat->i2c_client, AS5011_Y_RES_INT);
> 
> I prefer that i2c_read wrapper here would return the actual error code instead,
> and have one function argument to return the "x, y" co-ordinates, that way
> we don't loose the error codes, and return from here itself if i2c read fails.

Ok

> 
>> +	input_report_abs(plat_dat->input_dev, ABS_X, x);
>> +	input_report_abs(plat_dat->input_dev, ABS_Y, y);
>> +	input_sync(plat_dat->input_dev);
>> +}
>> +
>> +static irqreturn_t as5011_int_interrupt(int irq, void *dev_id)
>> +{
>> +	struct as5011_platform_data *plat_dat = dev_id;
>> +
>> +	mutex_lock(&plat_dat->update_lock);
>> +	as5011_update_axes(plat_dat);
>> +	mutex_unlock(&plat_dat->update_lock);
> 
> Please explain this lock. Sorry if I have missed out your explanation in the earlier threads.

If a second interruption occurs the first update must be finished before update again.

> 
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int __devinit as5011_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *id)
>> +{
>> +	struct as5011_platform_data *plat_dat = client->dev.platform_data;
> 
> const please. 

ok

>> +
>> +	retval = request_threaded_irq(plat_dat->button_irq,
>> +					NULL, button_interrupt,
>> +					0, "as5011_button",
>> +					plat_dat);
>> +	if (retval < 0) {
>> +		dev_err(&client->dev, "Can't allocate irq %d\n",
>> +			plat_dat->button_irq);
>> +		retval = -EBUSY;
> 
> Why don't we return same error code instead of overwriting it here?

Copy/paste error.


>> +
>> +	/* to free irq gpio in chip*/
>> +	as5011_i2c_read(plat_dat->i2c_client, AS5011_X_RES_INT);
> 
> Would you prefer to move some of the initialization of the chip except
> reset of controller to open() call, so that if there is no user of this
> device than we don't need to leave this device left configured?

I it could be a good idea. I didn't know that I can catch the open() call under the driver.

>> +struct as5011_platform_data {
>> +	/* public */
>> +	int button_gpio;
>> +	int button_irq;
>> +	int int_gpio;
>> +	int int_irq;
>> +	char xp, xn; /* threshold for x axis */
>> +	char yp, yn; /* threshold for y axis */
>> +
>> +	int (*init_gpio)(void); /* init interrupts gpios */
>> +	void (*exit_gpio)(void);/* exit gpios */
>> +
>> +	/* private */
>> +	struct input_dev *input_dev;
>> +	struct i2c_client *i2c_client;
>> +	char name[AS5011_MAX_NAME_LENGTH];
>> +	struct mutex update_lock;
> 
> These private stuff should not be part of your platform data structure. When we say
> platform data meaning that it will be all public with "const". Please allocate
> local data structure in the probe itself with these private members. You can refer
> other drivers.

Ok done.

> 
> 
> ---Trilok Soni
> 

-- Fabien Marteau

Re: [PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver (third version)

[Dmitry Torokhov]

On Fri, Jan 07, 2011 at 08:42:37PM +0100, Fabien Marteau wrote:
> On 04/01/2011 12:01, Trilok Soni wrote:
> > Hi Fabien,
> > 
> > On 1/4/2011 3:17 PM, Fabien Marteau wrote:
> >> +
> >> +static irqreturn_t as5011_int_interrupt(int irq, void *dev_id)
> >> +{
> >> +	struct as5011_platform_data *plat_dat = dev_id;
> >> +
> >> +	mutex_lock(&plat_dat->update_lock);
> >> +	as5011_update_axes(plat_dat);
> >> +	mutex_unlock(&plat_dat->update_lock);
> > 
> > Please explain this lock. Sorry if I have missed out your explanation in the earlier threads.
> 
> If a second interruption occurs the first update must be finished before update again.

The ISR handler will not be called simultaneously on several CPUs. What
else can it race with?

-- 
Dmitry

Re: [PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver (third version)

[Fabien Marteau]

On 07/01/2011 20:50, Dmitry Torokhov wrote:
> On Fri, Jan 07, 2011 at 08:42:37PM +0100, Fabien Marteau wrote:
>> On 04/01/2011 12:01, Trilok Soni wrote:
>>> Hi Fabien,
>>>
>>> On 1/4/2011 3:17 PM, Fabien Marteau wrote:
>>>> +
>>>> +static irqreturn_t as5011_int_interrupt(int irq, void *dev_id)
>>>> +{
>>>> +	struct as5011_platform_data *plat_dat = dev_id;
>>>> +
>>>> +	mutex_lock(&plat_dat->update_lock);
>>>> +	as5011_update_axes(plat_dat);
>>>> +	mutex_unlock(&plat_dat->update_lock);
>>>
>>> Please explain this lock. Sorry if I have missed out your explanation in the earlier threads.
>>
>> If a second interruption occurs the first update must be finished before update again.
> 
> The ISR handler will not be called simultaneously on several CPUs. What
> else can it race with?
> 
Hum in fact nothing I thinks. I will delete it.

Thanks
Fabien