Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 25 Nov 2010 11:10:58 +0100 | From | Fabien Marteau <> | Subject | Re: [PATCH] input: joystick: Adding Austria Microsystem AS5011 joystick driver |
| |
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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| ![\](/images/icornerr.gif) |