input: joystick: Adding Austria Microsystem AS5011 joystick driver (third version)
Subsystems (2)
PATCH 1/1 input: joystick: Adding Austria Microsystem AS5011 joystick driver (third version)
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.
--- 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
drivers/input/joystick/Kconfig +9 -0
@@ -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
drivers/input/joystick/Makefile +1 -0
@@ -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
drivers/input/joystick/as5011.c new file +377 -0
377 lines not on this page · raw message
include/linux/as5011.h new file +35 -0
@@ -0,0 +1,35 @@ +#ifndef _AS5011_H +#define _AS5011_H + +/* +* Copyright (c) 2010, 2011 Fabien Marteau <[email protected]> +* +* 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 */
Signature · 1 line
1.7.0.4
Hi Fabien,
Quoted 8 lines · 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 <[email protected]>---
Looks good. Few comments.
Quoted 4 lines
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?
drivers/input/joystick/as5011.c line 33
Quoted 10 lines
drivers/input/joystick/as5011.c new file +10
-0+ +#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.
drivers/input/joystick/as5011.c line 82
Quoted 3 lines
drivers/input/joystick/as5011.c new file +3
-0+ +#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.
Quoted 13 lines
drivers/input/joystick/as5011.c new file +13
-0+#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,
drivers/input/joystick/as5011.c line 105
Quoted 10 lines
drivers/input/joystick/as5011.c new file +10
-0+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.
drivers/input/joystick/as5011.c line 140
Quoted 7 lines
drivers/input/joystick/as5011.c new file +7
-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);
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.
Quoted 5 lines
drivers/input/joystick/as5011.c new file +5
-0+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);
drivers/input/joystick/as5011.c line 155
Quoted 10 lines
drivers/input/joystick/as5011.c new file +10
-0+ +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.
drivers/input/joystick/as5011.c line 167
Quoted 12 lines
drivers/input/joystick/as5011.c new file +12
-0+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.
drivers/input/joystick/as5011.c line 175
Quoted 8 lines
drivers/input/joystick/as5011.c new file +8
-0+ +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.
Quoted 37 lines
drivers/input/joystick/as5011.c new file +37
-0+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,
drivers/input/joystick/as5011.c line 222
Quoted 10 lines
drivers/input/joystick/as5011.c new file +10
-0+ 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?
Quoted 73 lines
drivers/input/joystick/as5011.c new file +73
-0+ 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,
drivers/input/joystick/as5011.c line 305
Quoted 10 lines
drivers/input/joystick/as5011.c new file +10
-0+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?
Quoted 62 lines
drivers/input/joystick/as5011.c new file +62
-0+ +/* 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 ) +{
drivers/input/joystick/as5011.c line 377
Quoted 10 lines
drivers/input/joystick/as5011.c new file +10
-0+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.
Quoted 11 lines
include/linux/as5011.h new file +11
-0+#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 */
include/linux/as5011.h line 32
Quoted 10 lines
include/linux/as5011.h new file +10
-0+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
Signature · 2 lines
Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 12 lines · Trilok Soni
Hi Fabien,
Quoted 8 lines · 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 <[email protected]>---
Quoted 8 lines
Looks good. Few comments.
Quoted 4 lines
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.
Quoted 14 lines
drivers/input/joystick/as5011.c line 33
Quoted 11 lines
drivers/input/joystick/as5011.c new file +10
-0+ +#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
Quoted 7 lines
drivers/input/joystick/as5011.c line 82
Quoted 4 lines
drivers/input/joystick/as5011.c new file +3
-0+ +#define SIZE_NAME 30 +#define SIZE_EVENT_PATH 40I don't find anybody using these #defines. Please remove the defines which are not used.
Old useless macro, deleted.
Quoted 18 lines
Quoted 18 lines
drivers/input/joystick/as5011.c new file +17
-0+#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 }; +
Quoted 8 lines
drivers/input/joystick/as5011.c line 105
Quoted 6 lines
drivers/input/joystick/as5011.c new file +6
-0+ 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.
Quoted 12 lines
drivers/input/joystick/as5011.c line 140
Quoted 8 lines
drivers/input/joystick/as5011.c new file +7
-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);I don't find
gpio_requestcall 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.
Quoted 12 lines
Quoted 12 lines
drivers/input/joystick/as5011.c new file +11
-0+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) +{
Quoted 8 lines
drivers/input/joystick/as5011.c line 155
Quoted 4 lines
drivers/input/joystick/as5011.c new file +4
-0+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_readwrapper 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
Quoted 15 lines
drivers/input/joystick/as5011.c line 167
Quoted 13 lines
drivers/input/joystick/as5011.c new file +12
-0+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.
Quoted 11 lines
drivers/input/joystick/as5011.c line 175
Quoted 9 lines
drivers/input/joystick/as5011.c new file +8
-0+ +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
Quoted 11 lines
drivers/input/joystick/as5011.c line 222
Quoted 9 lines
drivers/input/joystick/as5011.c new file +9
-0+ + 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.
Quoted 7 lines
drivers/input/joystick/as5011.c line 305
Quoted 3 lines
drivers/input/joystick/as5011.c new file +3
-0+ +/* 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.
Quoted 14 lines
Quoted 14 lines
include/linux/as5011.h new file +14
-0+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;
Quoted 8 lines
include/linux/as5011.h line 32
Quoted 3 lines
include/linux/as5011.h new file +3
-0+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.
Quoted 4 lines
---Trilok Soni
-- Fabien Marteau
Dmitry Torokhov
maintainerQuoted 15 lines · Fabien Marteau
Quoted 13 lines · Trilok Soni
Hi Fabien,
drivers/input/joystick/as5011.c line 167
Quoted 8 lines · Fabien Marteau
drivers/input/joystick/as5011.c new file +8
-0+ +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.
Quoted 1 line
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?
Signature · 1 line
Dmitry
Quoted 16 lines · Dmitry Torokhov
Quoted 15 lines · Fabien Marteau
Quoted 13 lines · Trilok Soni
Hi Fabien,
drivers/input/joystick/as5011.c line 167
Quoted 8 lines · Fabien Marteau
drivers/input/joystick/as5011.c new file +8
-0+ +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.
Quoted 5 lines
Quoted 1 line
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