Re: [PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Fabien,

On Tue, Nov 23, 2010 at 05:36:14PM +0100, Fabien Marteau wrote:
> Driver for EasyPoint AS5011 2 axis joystick chip. This chip is plugged
> on an I2C bus. It as been tested on ARM processor (i.MX27).
> 

Thank you for the patch. In addiitoon to comments you got from the other
reviewers I'd recommend switching to threaded IRQ instead of using a
separate workqueue, it greatly simplifies the code.

> +
> +static irqreturn_t button_interrupt(int irq, void *dev_id)
> +{
> +	struct as5011_platform_data *plat_dat =
> +		(struct as5011_platform_data *)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;

That appears to be a hard IRQ; are you sure that gpio_get_value will not
sleep?

> +
> +	plat_dat->input_dev->name = "Austria Microsystem as5011 joystick";
> +	plat_dat->input_dev->uniq = "Austria0";
> +	plat_dat->input_dev->id.bustype = BUS_I2C;
> +	plat_dat->input_dev->phys = "/dev/input/event0";

Phys is not the path in device tree but rather physical device
connection path within the system. If there is not known connection path
it is better just leave it as NULL.

> +	plat_dat->input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	plat_dat->input_dev->keybit[BIT_WORD(BTN_JOYSTICK)] =
> +					BIT_MASK(BTN_JOYSTICK);
> +
> +	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);
> +
> +	plat_dat->button_irq_name =
> +		kmalloc(sizeof(unsigned char)*SIZE_NAME,
> +					 GFP_KERNEL);

Just why does it have to be separately allocated? I'd even stick with
"as5011" for all IRQ names.

> +
> +	queue_work(plat_dat->workqueue, &plat_dat->update_axes_work);
> +

The device is quite likely not opened here so why do you want to send
events?

Thanks.

-- 
Dmitry