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

[Fabien Marteau <fabien.marteau@armadeus.com>]

Hi Dmitry,

Thank you for yours comments, it's my first kernel driver patch and your advices
are really helpful.

On 24/11/2010 20:55, Dmitry Torokhov wrote:
> 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.

Ok I will see it.

> 
>> +
>> +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?

For my platform, yes I'm sure … but if somebody else use this driver with
gpio that can sleep, it can be a bad idea of course.
I did it because it's under an interrupt call, then we can't sleep.
I will see how to use  a threaded IRQ to avoid it.

> 
>> +
>> +	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.

Ok, done.

> 
>> +	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.

I thought that irq name should be different for each IRQ used under device and
for each device used in system. I can use the same irq name if I've got several
as5011 joystick in the same system ?

> 
>> +
>> +	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?
It was a mistake.

> 
> Thanks.
> 

Thanks too.
Fabien