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

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

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

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 <dbrownell@users.sourceforge.net>
---
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(+)

--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -356,4 +356,12 @@ config KEYBOARD_SH_KEYSC
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called sh_keysc.
+
+config KEYBOARD_DM355EVM
+	tristate "TI DaVinci DM355 EVM Keypad and IR Remote"
+	depends on MFD_DM355EVM_MSP
+	help
+	  Supports the pushbuttons and IR remote used with
+	  the DM355 EVM board.
+
 endif
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_HP7XX)		+= jornada
 obj-$(CONFIG_KEYBOARD_MAPLE)		+= maple_keyb.o
 obj-$(CONFIG_KEYBOARD_BFIN)		+= bf54x-keys.o
 obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
+obj-$(CONFIG_KEYBOARD_DM355EVM)		+= dm355evm_keys.o
--- /dev/null
+++ b/drivers/input/keyboard/dm355evm_keys.c
@@ -0,0 +1,303 @@
+/*
+ * dm355evm_keys.c - support buttons and IR remote on DM355 EVM board
+ *
+ * Copyright (c) 2008 by David Brownell
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+
+#include <linux/i2c/dm355evm_msp.h>
+
+
+/*
+ * The MSP430 firmware on the DM355 EVM monitors on-board pushbuttons
+ * and an IR receptor used for the remote control.  When any key is
+ * pressed, or its autorepeat kicks in, an event is sent.  This driver
+ * read those events from the small (32 event) queue and reports them.
+ *
+ * Because we communicate with the MSP430 using I2C, and all I2C calls
+ * in Linux sleep, we need to cons up a kind of threaded IRQ handler
+ * using a work_struct.  The IRQ is active low, but we use it through
+ * the GPIO controller so we can trigger on falling edges.
+ *
+ * This driver was tested with firmware revision A4.
+ */
+struct dm355evm_keys {
+	struct work_struct	work;
+	struct input_dev	*input;
+	struct platform_device	*pdev;
+	int			irq;
+};
+
+#include <linux/gpio.h>
+
+static irqreturn_t dm355evm_keys_irq(int irq, void *_keys)
+{
+	struct dm355evm_keys	*keys = _keys;
+
+	schedule_work(&keys->work);
+	return IRQ_HANDLED;
+}
+
+/* These initial keycodes can be remapped by dm355evm_setkeycode(). */
+static struct {
+	u16	event;
+	u16	keycode;
+} dm355evm_keys[] = {
+
+	/*
+	 * Pushbuttons on the EVM board ... note that the labels for these
+	 * are SW10/SW11/etc on the PC board.  The left/right orientation
+	 * comes only from the firmware's documentation, and presumes the
+	 * power connector is immediately in front of you and the IR sensor
+	 * is to the right.  (That is, rotate the board counter-clockwise
+	 * by 90 degrees from the SW10/etc and "DM355 EVM" labels.)
+	 */
+	{ 0x00d8, KEY_OK, },		/* SW12 */
+	{ 0x00b8, KEY_UP, },		/* SW13 */
+	{ 0x00e8, KEY_DOWN, },		/* SW11 */
+	{ 0x0078, KEY_LEFT, },		/* SW14 */
+	{ 0x00f0, KEY_RIGHT, },		/* SW10 */
+
+	/*
+	 * 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, },
+};
+
+static void dm355evm_keys_work(struct work_struct *work)
+{
+	struct dm355evm_keys	*keys;
+	int			status;
+
+	keys = container_of(work, struct dm355evm_keys, work);
+
+	/* For simplicity we ignore INPUT_COUNT and just read
+	 * events until we get the "queue empty" indicator.
+	 * Reading INPUT_LOW decrements the count.
+	 */
+	for (;;) {
+		u16		event;
+		int		keycode;
+		int		i;
+
+		status = dm355evm_msp_read(DM355EVM_MSP_INPUT_HIGH);
+		if (status < 0) {
+			dev_dbg(&keys->pdev->dev, "input high err %d\n",
+					status);
+			break;
+		}
+		event = status << 8;
+
+		status = dm355evm_msp_read(DM355EVM_MSP_INPUT_LOW);
+		if (status < 0) {
+			dev_dbg(&keys->pdev->dev, "input low err %d\n",
+					status);
+			break;
+		}
+		event |= status;
+		if (event == 0xdead)
+			break;
+
+		/* ignore the RC5 toggle bit */
+		event &= ~0x0800;
+
+		/* find the key, or leave it as unknown */
+		keycode = KEY_UNKNOWN;
+		for (i = 0; i < ARRAY_SIZE(dm355evm_keys); i++) {
+			if (dm355evm_keys[i].event != event)
+				continue;
+			keycode = dm355evm_keys[i].keycode;
+			break;
+		}
+		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_report_key(keys->input, keycode, 0);
+	}
+	input_sync(keys->input);
+}
+
+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;
+	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;
+
+	input->id.bustype = BUS_I2C;
+	input->id.product = 0x0355;
+	input->id.version = dm355evm_msp_read(DM355EVM_MSP_FIRMREV);
+
+	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]);
+	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);
+	if (status < 0)
+		goto fail1;
+
+	/* 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;
+	}
+
+	return 0;
+
+fail1:
+	input_free_device(input);
+fail0:
+	kfree(keys);
+	dev_err(&pdev->dev, "can't register, err %d\n", status);
+	return status;
+}
+
+static int __devexit dm355evm_keys_remove(struct platform_device *pdev)
+{
+	struct dm355evm_keys	*keys = platform_get_drvdata(pdev);
+
+	free_irq(keys->irq, keys);
+	input_unregister_device(keys->input);
+	kfree(keys);
+	return 0;
+}
+
+/* REVISIT:  add suspend/resume when DaVinci supports it.  The IRQ should
+ * be able to wake up the system.  When device_may_wakeup(&pdev->dev), call
+ * enable_irq_wake() on suspend, and disable_irq_wake() on resume.
+ */
+
+/*
+ * I2C is used to talk to the MSP430, but this platform device is
+ * exposed by an MFD driver that manages I2C communications.
+ */
+static struct platform_driver dm355evm_keys_driver = {
+	.probe		= dm355evm_keys_probe,
+	.remove		= __devexit_p(dm355evm_keys_remove),
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "dm355evm_keys",
+	},
+};
+
+static int __init dm355evm_keys_init(void)
+{
+	return platform_driver_register(&dm355evm_keys_driver);
+}
+module_init(dm355evm_keys_init);
+
+static void __exit dm355evm_keys_exit(void)
+{
+	platform_driver_unregister(&dm355evm_keys_driver);
+}
+module_exit(dm355evm_keys_exit);
+
+MODULE_LICENSE("GPL");

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

[Felipe Balbi]

Hi Dave,

only one comment below.

On Sun, Dec 07, 2008 at 11:59:50AM -0800, David Brownell wrote:
> +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;

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

-- 
balbi

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

[David Brownell]

On Sunday 07 December 2008, Felipe Balbi wrote:
> > +     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.  :)

- Dave

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

[Felipe Balbi]

On Sun, Dec 07, 2008 at 12:28:38PM -0800, David Brownell wrote:
> 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.  :)

Sure, I'm in the testing your patches right now. Will send you an
update patch soonish ;-)

Great work btw :-D everything looks great.

-- 
balbi

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

[Felipe Balbi]

On Sun, Dec 07, 2008 at 12:28:38PM -0800, David Brownell wrote:
> On Sunday 07 December 2008, Felipe Balbi wrote:
> > > +     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.  :)

Here's an update patch for your input driver:

================ cut here ===================

diff --git a/drivers/input/keyboard/dm355evm_keys.c b/drivers/input/keyboard/dm355evm_keys.c
index ed36127..0871d9c 100644
--- a/drivers/input/keyboard/dm355evm_keys.c
+++ b/drivers/input/keyboard/dm355evm_keys.c
@@ -33,7 +33,7 @@
 struct dm355evm_keys {
 	struct work_struct	work;
 	struct input_dev	*input;
-	struct platform_device	*pdev;
+	struct device		*dev;
 	int			irq;
 };
 
@@ -128,7 +128,7 @@ static void dm355evm_keys_work(struct work_struct *work)
 
 		status = dm355evm_msp_read(DM355EVM_MSP_INPUT_HIGH);
 		if (status < 0) {
-			dev_dbg(&keys->pdev->dev, "input high err %d\n",
+			dev_dbg(keys->dev, "input high err %d\n",
 					status);
 			break;
 		}
@@ -136,7 +136,7 @@ static void dm355evm_keys_work(struct work_struct *work)
 
 		status = dm355evm_msp_read(DM355EVM_MSP_INPUT_LOW);
 		if (status < 0) {
-			dev_dbg(&keys->pdev->dev, "input low err %d\n",
+			dev_dbg(keys->dev, "input low err %d\n",
 					status);
 			break;
 		}
@@ -155,7 +155,7 @@ static void dm355evm_keys_work(struct work_struct *work)
 			keycode = dm355evm_keys[i].keycode;
 			break;
 		}
-		dev_dbg(&keys->pdev->dev,
+		dev_dbg(keys->dev,
 			"input event 0x%04x--> keycode %d\n",
 			event, keycode);
 
@@ -200,7 +200,7 @@ static int __devinit dm355evm_keys_probe(struct platform_device *pdev)
 	if (!keys)
 		goto fail0;
 	platform_set_drvdata(pdev, keys);
-	keys->pdev = pdev;
+	keys->dev = &pdev->dev;
 
 	/* ... and input dev ... */
 	input = input_allocate_device();
@@ -257,6 +257,7 @@ fail1:
 fail0:
 	kfree(keys);
 	dev_err(&pdev->dev, "can't register, err %d\n", status);
+
 	return status;
 }
 
@@ -267,6 +268,7 @@ static int __devexit dm355evm_keys_remove(struct platform_device *pdev)
 	free_irq(keys->irq, keys);
 	input_unregister_device(keys->input);
 	kfree(keys);
+
 	return 0;
 }
 

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Brownell]

On Monday 08 December 2008, Felipe Balbi wrote:
> Here's an update patch for your input driver:

Looks good, thanks.  If someone commits the original
patch, this one has my ack.  Else I'll merge this
into the next version I send.

- Dave

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

[David Brownell]

I have a question for the Input folk, see below ...

On Sunday 07 December 2008, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> 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 <dbrownell@users.sourceforge.net>
> ---
> 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(+)
> 
> 	...

It seems this RC5 coding is used by a fair number of devices.
Is this the first one to get Linux support?

I kind of get the impression that some more generic support for
this might be useful at some point.  Along the lines of a module
to map (and remap?) RC5 codes to what Linux uses.

Maybe the *next* remote control driver can worry about that.  :)

- Dave


> +	/*
> +	 * 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, },
> +};

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

[Dmitry Torokhov]

Hi David,

On Sun, Dec 07, 2008 at 11:59:50AM -0800, David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> 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 <dbrownell@users.sourceforge.net>
> ---
> 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 :)

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

> +	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;
> +
> +	input->id.bustype = BUS_I2C;
> +	input->id.product = 0x0355;
> +	input->id.version = dm355evm_msp_read(DM355EVM_MSP_FIRMREV);
> +
> +	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.

> +	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);
> +	if (status < 0)
> +		goto fail1;
> +
> +	/* 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;".

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

-- 
Dmitry

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

[David Brownell]

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

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

[Dmitry Torokhov]

On Tue, Jan 13, 2009 at 01:42:56AM -0800, David Brownell wrote:
> 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.
> 

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?

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

Yes, that is responsibility of the setkeycode method as well.

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

Ah, OK.

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

It knows keycodes but not scancodes (or their equivalent). Given that
many (most?) keympas are sparse input core does not really know whether
a value is a valid scancode or not even if it is within the range of
input->keycodemax. COnsider HID - its 'scancodes' are 64 bits ;) 

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

So you will be sending an update, right?

Thanks!

-- 
Dmitry

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

[David Brownell]

On Tuesday 13 January 2009, Dmitry Torokhov wrote:
> > > > +		/* 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.
> > 
> 
> 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?

Yes.  But now I seem to observe a LOT more events.  I suppose that's
partly due to the fuzzy semantics of these events:  they don't encode
"down" or "up".


> So you will be sending an update, right?

Appended is a patchlet addressing your feedback.  What I'll do is
submit this to the DaVinci tree, and send you an all-in-one patch.

- Dave


======= SNIP!
From: David Brownell <dbrownell@users.sourceforge.net>

Address feedback from Dmitry:
 - input_sync() per event
 - maintain dev->keybit when remapping keys
 - since we handle remapping, keycodemax and keycodesize aren't used
 - on probe error, don't input_free_device() unless it registered

Also:
 - avoid reporting excess events
 - more bad-parameter paranoia in the remapping code

The excess event issue is basically that we don't have a way to
distinguish "button press" events from "button release" ones or
autorepeat events, so we should try being a bit smarter about
synthesizing them.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/input/keyboard/dm355evm_keys.c |   49 ++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 10 deletions(-)

--- a/drivers/input/keyboard/dm355evm_keys.c
+++ b/drivers/input/keyboard/dm355evm_keys.c
@@ -28,6 +28,8 @@
  * using a work_struct.  The IRQ is active low, but we use it through
  * the GPIO controller so we can trigger on falling edges.
  *
+ * Note that physically there can only be one of these devices.
+ *
  * This driver was tested with firmware revision A4.
  */
 struct dm355evm_keys {
@@ -120,6 +122,8 @@ static void dm355evm_keys_work(struct wo
 	 * Reading INPUT_LOW decrements the count.
 	 */
 	for (;;) {
+		static u16	last_event;
+
 		u16		event;
 		int		keycode;
 		int		i;
@@ -142,6 +146,23 @@ static void dm355evm_keys_work(struct wo
 		if (event == 0xdead)
 			break;
 
+		/* Press and release a button:  two events, same code.
+		 * Press and hold (autorepeat), then release: N events
+		 * (N > 2), same code.  For RC5 buttons the toggle bits
+		 * distinguish (for example) "1-autorepeat" from "1 1";
+		 * but PCB buttons don't support that bit.
+		 *
+		 * So we must synthesize release events.  We do that by
+		 * mapping events to a press/release event pair; then
+		 * to avoid adding extra events, skip the second event
+		 * of each pair.
+		 */
+		if (event == last_event) {
+			last_event = 0;
+			continue;
+		}
+		last_event = event;
+
 		/* ignore the RC5 toggle bit */
 		event &= ~0x0800;
 
@@ -157,28 +178,38 @@ static void dm355evm_keys_work(struct wo
 			"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.
-		 */
+		/* report press + release */
 		input_report_key(keys->input, keycode, 1);
+		input_sync(keys->input);
 		input_report_key(keys->input, keycode, 0);
+		input_sync(keys->input);
 	}
-	input_sync(keys->input);
 }
 
 static int dm355evm_setkeycode(struct input_dev *dev, int index, int keycode)
 {
-	if (index >= ARRAY_SIZE(dm355evm_keys))
+	u16		old_keycode;
+	unsigned	i;
+
+	if (((unsigned)index) >= ARRAY_SIZE(dm355evm_keys))
 		return -EINVAL;
 
+	old_keycode = dm355evm_keys[index].keycode;
 	dm355evm_keys[index].keycode = keycode;
+	set_bit(keycode, dev->keybit);
+
+	for (i = 0; i < ARRAY_SIZE(dm355evm_keys); i++) {
+		if (dm355evm_keys[index].keycode == old_keycode)
+			goto done;
+	}
+	clear_bit(old_keycode, dev->keybit);
+done:
 	return 0;
 }
 
 static int dm355evm_getkeycode(struct input_dev *dev, int index, int *keycode)
 {
-	if (index >= ARRAY_SIZE(dm355evm_keys))
+	if (((unsigned)index) >= ARRAY_SIZE(dm355evm_keys))
 		return -EINVAL;
 
 	return dm355evm_keys[index].keycode;
@@ -219,8 +250,6 @@ static int __devinit dm355evm_keys_probe
 	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]);
 	input->keycode = dm355evm_keys;
 	input->setkeycode = dm355evm_setkeycode;
 	input->getkeycode = dm355evm_getkeycode;
@@ -237,7 +266,7 @@ static int __devinit dm355evm_keys_probe
 	/* register */
 	status = input_register_device(input);
 	if (status < 0)
-		goto fail1;
+		goto fail0;
 
 	/* start reporting events */
 	status = request_irq(keys->irq, dm355evm_keys_irq,