Re: [PATCH 1/2] input/joystick: new AD7142 driver

[Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Tue, Jul 14, 2009 at 14:03, Dmitry Torokhov wrote:
> On Tue, Jul 14, 2009 at 01:33:46PM -0400, Mike Frysinger wrote:
>> +static int ad7142_probe(struct i2c_client *client,
>> +                                     const struct i2c_device_id *id)
>
> __devinit

fixed

>> +             rc = request_irq(client->irq, ad7142_interrupt,
>> +                             IRQF_TRIGGER_LOW, "ad7142_joystick", data);
>
> Let's use threaded IRQs, they are so nice for devices like this one.

i'll open a tracker item for someone to look at this ... Bryan has left our team

>> +             disable_irq_nosync(client->irq);
>
> Why nosync here?

no idea -- changed to normal version

>> +     /* Allocate and register AD7142 input device */
>> +     data->input = input_allocate_device();
>> +     if (!data->input) {
>> +             dev_err(&client->dev, "Can't allocate input device\n");
>> +             rc = -ENOMEM;
>> +             goto fail_allocate;
>> +     }
>
> Allocate the device before you request IRQ.. Even if you disabled it
> there is still a window where it can be raised without input device
> allocated.

fixed ... sure would be nice to have a IRQF_xxx flag to request an IRQ
in the disabled state

>> +     input->evbit[0] = BIT_MASK(EV_KEY);
>> +     input->keybit[BIT_WORD(BTN_BASE)] = BIT_MASK(BTN_BASE) |
>> +                                             BIT_MASK(BTN_BASE2) |
>> +                                             BIT_MASK(BTN_BASE3) |
>> +                                             BIT_MASK(BTN_BASE4);
>> +     input->keybit[BIT_WORD(KEY_UP)] |=  BIT_MASK(KEY_UP) |
>> +                                             BIT_MASK(KEY_DOWN) |
>> +                                             BIT_MASK(KEY_LEFT) |
>> +                                             BIT_MASK(KEY_RIGHT);
>> +
>
> I am really not sure why you call it a joystick since it does not report
> relative axes... Let's put it in misc and just another button device.

if the hardware doesnt actually support relative events, sure ...
otherwise we'll add support for it

>> +static int __exit ad7142_remove(struct i2c_client *client)
>
> __devexit()

fixed

>> +     .remove = __exit_p(ad7142_remove),
>
> __devexit_p()

fixed

> Any PM support?

guess we'll add it
-mike

_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@blackfin.uclinux.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel