dm355evm input driver

Quoted 2 lines

+     platform_set_drvdata(pdev, keys);
+     keys->pdev = pdev;

you could be holding only the device pointer.

        keys->dev = &pdev->dev;

then, if you really happen to need the pdev pointer you can use

to_platform_device(keys->dev);

True, that would be a bit simpler; the device handle is what's normally needed, e.g. for dev_dbg(). I'd take an update patch; but for now I'll avoid a re-test cycle. :)

Quoted 7 lines · Felipe Balbi

Quoted 2 lines

+     platform_set_drvdata(pdev, keys);
+     keys->pdev = pdev;

you could be holding only the device pointer.

        keys->dev = &pdev->dev;

Quoted 3 lines

then, if you really happen to need the pdev pointer you can use

to_platform_device(keys->dev);

True, that would be a bit simpler; the device handle is what's normally needed, e.g. for dev_dbg(). I'd take an update patch; but for now I'll avoid a re-test cycle. :)

Here's an update patch for your input driver:

From: David Brownell <[email protected]>

Simple input driver support for the events reported by the MSP430 firmware on the DM355 EVM. Verified using the remote included with the kit; docs weren't quite right. Some of the keycode selections might need improvement; I'm hoping I implemented the remapping support right, so there's at least a runtime workaround.

Keys on the remote seem to repeat undesirably. Someone might want to see what's up with that, and fix it; it might just be an issue of tracking RC5 state and using the toggle.

Signed-off-by David Brownell <[email protected]>

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

 drivers/input/keyboard/Kconfig         |    8 
 drivers/input/keyboard/Makefile        |    1 
 drivers/input/keyboard/dm355evm_keys.c |  303 +++++++++++++++++++++++++++++++
 3 files changed, 312 insertions(+)

	...

drivers/input/keyboard/dm355evm_keys.c new file +42 -0

+	/*
+	 * IR buttons ... codes assigned to match the universal remote
+	 * provided with the EVM (Philips PM4S) using DVD code 0020.
+	 *
+	 * These event codes match firmware documentation, but other
+	 * remote controls could easily send more RC5-encoded events.
+	 * The PM4S manual was used in several cases to help select
+	 * a keycode reflecting the intended usage.
+	 *
+	 * RC5 codes are 14 bits, with two start bits (0x3 prefix)
+	 * and a toggle bit (masked out below).
+	 */
+	{ 0x300c, KEY_POWER, },		/* NOTE: docs omit this */
+	{ 0x3000, KEY_NUMERIC_0, },
+	{ 0x3001, KEY_NUMERIC_1, },
+	{ 0x3002, KEY_NUMERIC_2, },
+	{ 0x3003, KEY_NUMERIC_3, },
+	{ 0x3004, KEY_NUMERIC_4, },
+	{ 0x3005, KEY_NUMERIC_5, },
+	{ 0x3006, KEY_NUMERIC_6, },
+	{ 0x3007, KEY_NUMERIC_7, },
+	{ 0x3008, KEY_NUMERIC_8, },
+	{ 0x3009, KEY_NUMERIC_9, },
+	{ 0x3022, KEY_ENTER, },
+	{ 0x30ec, KEY_MODE, },		/* "tv/vcr/..." */
+	{ 0x300f, KEY_SELECT, },	/* "info" */
+	{ 0x3020, KEY_CHANNELUP, },	/* "up" */
+	{ 0x302e, KEY_MENU, },		/* "in/out" */
+	{ 0x3011, KEY_VOLUMEDOWN, },	/* "left" */
+	{ 0x300d, KEY_MUTE, },		/* "ok" */
+	{ 0x3010, KEY_VOLUMEUP, },	/* "right" */
+	{ 0x301e, KEY_SUBTITLE, },	/* "cc" */
+	{ 0x3021, KEY_CHANNELDOWN, },	/* "down" */
+	{ 0x3022, KEY_PREVIOUS, },
+	{ 0x3026, KEY_SLEEP, },
+	{ 0x3172, KEY_REWIND, },	/* NOTE: docs wrongly say 0x30ca */
+	{ 0x3175, KEY_PLAY, },
+	{ 0x3174, KEY_FASTFORWARD, },
+	{ 0x3177, KEY_RECORD, },
+	{ 0x3176, KEY_STOP, },
+	{ 0x3169, KEY_PAUSE, },
+};

From: David Brownell <[email protected]>

Simple input driver support for the events reported by the MSP430 firmware on the DM355 EVM. Verified using the remote included with the kit; docs weren't quite right. Some of the keycode selections might need improvement; I'm hoping I implemented the remapping support right, so there's at least a runtime workaround.

Keys on the remote seem to repeat undesirably. Someone might want to see what's up with that, and fix it; it might just be an issue of tracking RC5 state and using the toggle.

Signed-off-by David Brownell <[email protected]>

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

drivers/input/keyboard/dm355evm_keys.c new file +38 -0

+	return 0;
+}
+
+static int dm355evm_getkeycode(struct input_dev *dev, int index, int *keycode)
+{
+	if (index >= ARRAY_SIZE(dm355evm_keys))
+		return -EINVAL;
+
+	return dm355evm_keys[index].keycode;
+}
+
+/*----------------------------------------------------------------------*/
+
+static int __devinit dm355evm_keys_probe(struct platform_device *pdev)
+{
+	struct dm355evm_keys	*keys;
+	int			status = -ENOMEM;
+	struct input_dev	*input;
+	int			i;
+
+	/* allocate instance struct */
+	keys = kzalloc(sizeof *keys, GFP_KERNEL);
+	if (!keys)
+		goto fail0;
+	platform_set_drvdata(pdev, keys);
+	keys->pdev = pdev;
+
+	/* ... and input dev ... */
+	input = input_allocate_device();
+	if (!input)
+		goto fail0;
+	keys->input = input;
+	input_set_drvdata(input, keys);
+
+	input->name = "DM355 EVM Controls";
+	input->phys = "dm355evm/input0";
+	input->dev.parent = &pdev->dev;
+

drivers/input/keyboard/dm355evm_keys.c new file +15 -0

+	input->keycode = dm355evm_keys;
+	input->setkeycode = dm355evm_setkeycode;
+	input->getkeycode = dm355evm_getkeycode;
+
+	/* set up "threaded IRQ handler" */
+	status = platform_get_irq(pdev, 0);
+	if (status < 0)
+		goto fail1;
+	keys->irq = status;
+	INIT_WORK(&keys->work, dm355evm_keys_work);
+
+	/* REVISIT:  flush the event queue? */
+
+	/* register */
+	status = input_register_device(input);

drivers/input/keyboard/dm355evm_keys.c new file +12 -0

+	}
+
+	return 0;
+
+fail1:
+	input_free_device(input);
+fail0:
+	kfree(keys);
+	dev_err(&pdev->dev, "can't register, err %d\n", status);
+	return status;
+}
+

...

Quoted 6 lines

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

drivers/input/keyboard/dm355evm_keys.c line 166

Quoted 9 lines

drivers/input/keyboard/dm355evm_keys.c new file +9 -0

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

drivers/input/keyboard/dm355evm_keys.c line 169

Quoted 3 lines

drivers/input/keyboard/dm355evm_keys.c new file +3 -0

+		input_report_key(keys->input, keycode, 0);
+	}
+	input_sync(keys->input);

Quoted 4 lines

drivers/input/keyboard/dm355evm_keys.c new file +4 -0

+}
+
+static int dm355evm_setkeycode(struct input_dev *dev, int index, int keycode)
+{

drivers/input/keyboard/dm355evm_keys.c line 177

Quoted 4 lines

drivers/input/keyboard/dm355evm_keys.c new file +4 -0

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

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.

drivers/input/keyboard/dm355evm_keys.c line 225

Quoted 6 lines

drivers/input/keyboard/dm355evm_keys.c new file +6 -0

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

drivers/input/keyboard/dm355evm_keys.c line 250

Quoted 7 lines

drivers/input/keyboard/dm355evm_keys.c new file +7 -0

+	/* 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;".

Quoted 10 lines · Dmitry Torokhov

...

Quoted 6 lines

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

Quoted 12 lines

drivers/input/keyboard/dm355evm_keys.c line 166

Quoted 9 lines

drivers/input/keyboard/dm355evm_keys.c new file +9 -0

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

Quoted 3 lines

drivers/input/keyboard/dm355evm_keys.c line 169

Quoted 3 lines

drivers/input/keyboard/dm355evm_keys.c new file +3 -0

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

Quoted 13 lines

drivers/input/keyboard/dm355evm_keys.c line 177

Quoted 9 lines

drivers/input/keyboard/dm355evm_keys.c new file +8 -0

+}
+
+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.)

Quoted 4 lines

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.

Quoted 10 lines

drivers/input/keyboard/dm355evm_keys.c line 225

Quoted 7 lines

drivers/input/keyboard/dm355evm_keys.c new file +6 -0

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

Quoted 11 lines

drivers/input/keyboard/dm355evm_keys.c line 250

Quoted 8 lines

drivers/input/keyboard/dm355evm_keys.c new file +7 -0

+	/* 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. :)

Quoted 8 lines

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+		/* 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.

Quoted 9 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

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

It is and they can. The idea is that userspace accumulates input events until it gets the sync event which signals that as full as possible hardware state was transmitted. The problem with keys is that if userspace really does accumulate events the 2 down/up in succession will cancel each other. There are really 2 separate states - button pressed and button released which should be accompanied with its own sync. But if you were reposting several buttons at once they could all "share" the same sync event. Does it make any sense to you?

So you will be sending an update, right?