On Monday 12 January 2009, Dmitry Torokhov wrote:
> ...
> > Depends on the patch for the parent MFD driver, and won't work
> > without the patch making GPIO IRQs work on dm355.
> > 
> > NOTE:  not suitable for mainline until the dm355evm board support
> > (and parent MFD driver) is in the merge queue.
> > 
> 
> It looks like the MFD driver was merged so we need to start wokring
> on this one :)

Much to my surprise!  :)


> > +           dev_dbg(&keys->pdev->dev,
> > +                   "input event 0x%04x--> keycode %d\n",
> > +                   event, keycode);
> > +
> > +           /* Report press + release ... we can't tell if
> > +            * this is an autorepeat, and we need to guess
> > +            * about the release.
> > +            */
> > +           input_report_key(keys->input, keycode, 1);
> 
> input_sync() is also needed here.
> 
> > +           input_report_key(keys->input, keycode, 0);
> > +   }
> > +   input_sync(keys->input);

If so, then the existing input_sync() needs to move up
a few lines too ... I had thought that the "sync" was
like with a filesystem, where lots of events could be
batched, but evidently not.


> > +}
> > +
> > +static int dm355evm_setkeycode(struct input_dev *dev, int index, int 
> > keycode)
> > +{
> > +   if (index >= ARRAY_SIZE(dm355evm_keys))
> > +           return -EINVAL;
> > +
> > +   dm355evm_keys[index].keycode = keycode;
> 
> You also need to alter dev->keybit to indicate that device may generate
> new keycode, otherwise input core will drop event intead of passing it
> on.

Should something then be scrubbing out dev->keybit to
indicate the *old* key code is no longer reported?
(After verifying that no other button reports it.)


> Also I prefer devices that support remapping to keep their copy of 
> keymap so in unlikely case there are 2 devices in the system they can
> have separate keymaps.

That's physically impossible in this case.


> > +   input->evbit[0] = BIT(EV_KEY);
> > +   for (i = 0; i < ARRAY_SIZE(dm355evm_keys); i++)
> > +           set_bit(dm355evm_keys[i].keycode, input->keybit);
> > +
> > +   input->keycodemax = ARRAY_SIZE(dm355evm_keys);
> > +   input->keycodesize = sizeof(dm355evm_keys[0]);
> 
> You don't need to setup keycodesize and keycodemax since you provide
> your own get and set keycode helpers.

... which I'm presuming is the right thing to do.  It's
a bit surprising to see that the input core will then
have no way to tell what keycodes are valid other than
querying all possible codes!

 
> > +   /* start reporting events */
> > +   status = request_irq(keys->irq, dm355evm_keys_irq,
> > +                   IRQF_TRIGGER_FALLING,
> > +                   dev_name(&pdev->dev), keys);
> > +   if (status < 0) {
> > +           input_unregister_device(input);
> > +           goto fail1;
> 
> You should not call input_free_device() after input_unregister_device().
> Either jump to "fail0" or do "input = NULL;".

"goto fail0" seems much simpler.  :)





_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Reply via email to