Re: [patch 2.6.28-rc6-davinci1 5/6] dm355evm input driver

[David Brownell <david-b@pacbell.net>]

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.  :)