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@xxxxxxxxxxxx>
>> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
