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