[RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

Keyboard controller for OMAP4 with built-in scanning algorithm.
The following implementations are used:

  - matrix_keypac.c logic
  - hwmod framework
  - threaded irq

Signed-off-by: Syed Rafiuddin <rafiuddin.syed@ti.com>
Signed-off-by: Abraham Arce <x0066660@ti.com>
---
 drivers/input/keyboard/Kconfig        |    9 +
 drivers/input/keyboard/Makefile       |    1 +
 drivers/input/keyboard/omap4-keypad.c |  367 +++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/keyboard/omap4-keypad.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 5049c3c..8a4103e 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -390,6 +390,15 @@ config KEYBOARD_OMAP
 	  To compile this driver as a module, choose M here: the
 	  module will be called omap-keypad.
 
+config KEYBOARD_OMAP4
+        tristate "TI OMAP4 keypad support"
+        depends on (ARCH_OMAP4)
+        help
+          Say Y here if you want to use the OMAP4 keypad.
+
+          To compile this driver as a module, choose M here: the
+          module will be called omap4-keypad.
+
 config KEYBOARD_TWL4030
 	tristate "TI TWL4030/TWL5030/TPS659x0 keypad support"
 	depends on TWL4030_CORE
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 78654ef..1b3dfbe 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
 obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
 obj-$(CONFIG_KEYBOARD_NEWTON)		+= newtonkbd.o
 obj-$(CONFIG_KEYBOARD_OMAP)		+= omap-keypad.o
+obj-$(CONFIG_KEYBOARD_OMAP4)		+= omap4-keypad.o
 obj-$(CONFIG_KEYBOARD_OPENCORES)	+= opencores-kbd.o
 obj-$(CONFIG_KEYBOARD_PXA27x)		+= pxa27x_keypad.o
 obj-$(CONFIG_KEYBOARD_PXA930_ROTARY)	+= pxa930_rotary.o
diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
new file mode 100644
index 0000000..b656b4f
--- /dev/null
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -0,0 +1,367 @@
+/*
+ * linux/drivers/input/keyboard/omap4-keypad.c
+ *
+ * OMAP4 Keypad Driver
+ *
+ * Copyright (C) 2010 Texas Instruments
+ *
+ * Author: Abraham Arce <x0066660@ti.com>
+ * Initial Code: Syed Rafiuddin <rafiuddin.syed@ti.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <plat/omap_hwmod.h>
+#include <plat/omap_device.h>
+
+/* OMAP4 registers */
+#define OMAP4_KBD_REVISION		0x00
+#define OMAP4_KBD_SYSCONFIG		0x10
+#define OMAP4_KBD_SYSSTATUS		0x14
+#define OMAP4_KBD_IRQSTATUS		0x18
+#define OMAP4_KBD_IRQENABLE		0x1C
+#define OMAP4_KBD_WAKEUPENABLE		0x20
+#define OMAP4_KBD_PENDING		0x24
+#define OMAP4_KBD_CTRL			0x28
+#define OMAP4_KBD_DEBOUNCINGTIME	0x2C
+#define OMAP4_KBD_LONGKEYTIME		0x30
+#define OMAP4_KBD_TIMEOUT		0x34
+#define OMAP4_KBD_STATEMACHINE		0x38
+#define OMAP4_KBD_ROWINPUTS		0x3C
+#define OMAP4_KBD_COLUMNOUTPUTS		0x40
+#define OMAP4_KBD_FULLCODE31_0		0x44
+#define OMAP4_KBD_FULLCODE63_32		0x48
+
+/* OMAP4 bit definitions */
+#define OMAP4_DEF_SYSCONFIG_SOFTRST	(1 << 1)
+#define OMAP4_DEF_SYSCONFIG_ENAWKUP	(1 << 2)
+#define OMAP4_DEF_IRQENABLE_EVENTEN	(1 << 0)
+#define OMAP4_DEF_IRQENABLE_LONGKEY	(1 << 1)
+#define OMAP4_DEF_IRQENABLE_TIMEOUTEN	(1 << 2)
+#define OMAP4_DEF_CTRL_NOSOFTMODE	(1 << 1)
+#define OMAP4_DEF_CTRLPTVVALUE		(1 << 2)
+#define OMAP4_DEF_CTRLPTV		(1 << 1)
+#define OMAP4_DEF_IRQDISABLE		0x00
+
+/* OMAP4 values */
+#define OMAP4_VAL_DEBOUNCINGTIME	0x07
+#define OMAP4_VAL_FUNCTIONALCFG		0x1E
+
+#define OMAP4_MASK_IRQSTATUSDISABLE	0xFFFF
+
+struct omap_keypad {
+
+	struct platform_device *pdev;
+	struct omap_hwmod *oh;
+	const struct matrix_keypad_platform_data *pdata;
+
+	void __iomem    *base;
+
+	struct input_dev *input;
+
+	int irq;
+
+	unsigned short *keycodes;
+	unsigned int rows;
+	unsigned int cols;
+	unsigned int debounce;
+	unsigned int row_shift;
+	unsigned char key_state[8];
+};
+
+struct omap_device_pm_latency omap_keyboard_latency[] = {
+	{
+		.deactivate_func = omap_device_idle_hwmods,
+		.activate_func   = omap_device_enable_hwmods,
+		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+	},
+};
+
+static void __init omap_keypad_config(struct omap_keypad *keypad_data)
+{
+	__raw_writel(OMAP4_DEF_SYSCONFIG_SOFTRST | OMAP4_DEF_SYSCONFIG_ENAWKUP,
+			keypad_data->base + OMAP4_KBD_SYSCONFIG);
+	__raw_writel(OMAP4_VAL_FUNCTIONALCFG,
+			keypad_data->base + OMAP4_KBD_CTRL);
+	__raw_writel(OMAP4_VAL_DEBOUNCINGTIME,
+			keypad_data->base + OMAP4_KBD_DEBOUNCINGTIME);
+	__raw_writel(OMAP4_DEF_IRQDISABLE,
+			keypad_data->base + OMAP4_KBD_IRQSTATUS);
+	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
+			keypad_data->base + OMAP4_KBD_IRQENABLE);
+}
+
+/* Interrupt thread primary handler, called in hard interrupt context */
+
+static irqreturn_t omap_keypad_interrupt(int irq, void *dev_id)
+{
+	struct omap_keypad *keypad_data = dev_id;
+
+	/* disable interrupts */
+	__raw_writel(OMAP4_DEF_IRQDISABLE, keypad_data->base +
+				OMAP4_KBD_IRQENABLE);
+
+	/* wake up handler thread */
+	return IRQ_WAKE_THREAD;
+
+}
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
+	struct omap_keypad *keypad_data = dev_id;
+	struct input_dev *input = keypad_data->input;
+	unsigned char key_state[8];
+	int col, row, code;
+	u32 *new_state = (u32 *) key_state;
+
+	*new_state	 = __raw_readl(keypad_data->base +
+			OMAP4_KBD_FULLCODE31_0);
+	*(new_state + 1) = __raw_readl(keypad_data->base +
+			OMAP4_KBD_FULLCODE63_32);
+
+	for (col = 0; col < keypad_data->cols; col++) {
+		for (row = 0; row < keypad_data->rows; row++) {
+			code = MATRIX_SCAN_CODE(row, col,
+					keypad_data->row_shift);
+			if (code < 0) {
+				printk(KERN_INFO "Spurious key %d-%d\n",
+						col, row);
+				continue;
+			}
+			input_report_key(input, keypad_data->keycodes[code],
+					key_state[col] & (1 << row));
+		}
+	}
+
+	memcpy(keypad_data->key_state, &key_state,
+			sizeof(keypad_data->key_state));
+
+	/* enable interrupts */
+	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
+			keypad_data->base + OMAP4_KBD_IRQENABLE);
+
+	/* clear pending interrupts */
+	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
+			keypad_data->base + OMAP4_KBD_IRQSTATUS);
+
+	/* clear pending interrupts */
+	return IRQ_HANDLED;
+
+}
+
+
+static int __devinit omap_keypad_probe(struct platform_device *pdev)
+{
+	struct omap_device *od;
+	struct omap_hwmod *oh;
+	struct matrix_keypad_platform_data *pdata;
+	struct input_dev *input_dev;
+	const struct matrix_keymap_data *keymap_data;
+	struct omap_keypad *keypad_data;
+
+	unsigned int status = 0, row_shift = 0, error = 0, length = 0, id = 0;
+	unsigned short *keypad_codes;
+
+	int hw_mod_name_len = 16;
+	char oh_name[hw_mod_name_len];
+	char *name = "omap4-keypad";
+
+	length = snprintf(oh_name, hw_mod_name_len, "keyboard");
+	WARN(length >= hw_mod_name_len,
+		"String buffer overflow in keyboard device setup\n");
+
+	oh = omap_hwmod_lookup(oh_name);
+	if (!oh)
+		pr_err("Could not look up %s\n", oh_name);
+
+	pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+				GFP_KERNEL);
+
+	od = omap_device_build(name, id, oh, pdata,
+				sizeof(struct matrix_keypad_platform_data),
+				omap_keyboard_latency,
+				ARRAY_SIZE(omap_keyboard_latency), 1);
+	WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
+		name, oh_name);
+
+	/* platform data */
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "no platform data defined\n");
+		kfree(pdata);
+		return -EINVAL;
+	}
+
+	/* keymap data */
+	keymap_data = pdata->keymap_data;
+	if (!keymap_data) {
+		dev_err(&pdev->dev, "no keymap data defined\n");
+		kfree(keymap_data);
+		return -EINVAL;
+	}
+
+	/* omap keypad data */
+	row_shift = get_count_order(pdata->num_col_gpios);
+	keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
+	if (!keypad_data) {
+		error = -ENOMEM;
+		goto failed_free_platform;
+	}
+
+	/* omap keypad codes */
+	keypad_codes = kzalloc((pdata->num_row_gpios << row_shift) *
+			sizeof(*keypad_codes), GFP_KERNEL);
+	if (!keypad_codes) {
+		error = -ENOMEM;
+		goto failed_free_data;
+	}
+
+	/* input device allocation */
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		error = -ENOMEM;
+		goto failed_free_codes;
+	}
+
+	/* base resource */
+	keypad_data->base = oh->_rt_va;
+	if (!keypad_data->base) {
+		error = -ENOMEM;
+		goto failed_free_input;
+	}
+
+	/* irq */
+	keypad_data->irq = oh->mpu_irqs[0].irq;
+	if (keypad_data->irq < 0) {
+		dev_err(&pdev->dev, "no keyboard irq assigned\n");
+		error = keypad_data->irq;
+		goto failed_free_base;
+	}
+
+	/* threaded irq init */
+	status = request_threaded_irq(keypad_data->irq, omap_keypad_interrupt,
+			omap_keypad_threaded,
+				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+			"omap4-keypad", keypad_data);
+	if (status) {
+		dev_err(&pdev->dev, "failed to register interrupt\n");
+		error = -ENOMEM;
+		goto failed_free_irq;
+	}
+
+	keypad_data->input = input_dev;
+	keypad_data->row_shift = row_shift;
+	keypad_data->rows = pdata->num_row_gpios;
+	keypad_data->cols = pdata->num_col_gpios;
+	keypad_data->keycodes = keypad_codes;
+
+	input_dev->name = pdev->name;
+	input_dev->id.bustype = BUS_HOST;
+	input_dev->dev.parent = &pdev->dev;
+	input_dev->id.vendor = 0x0001;
+	input_dev->id.product = 0x0001;
+	input_dev->id.version = 0x0100;
+
+	input_dev->keycode	= keypad_codes;
+	input_dev->keycodesize	= sizeof(*keypad_codes);
+
+	matrix_keypad_build_keymap(keymap_data, row_shift,
+				   input_dev->keycode, input_dev->keybit);
+
+	__set_bit(EV_REP, input_dev->evbit);
+	__set_bit(KEY_OK, input_dev->keybit);
+	__set_bit(EV_KEY, input_dev->evbit);
+
+	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+	input_set_drvdata(input_dev, keypad_data);
+
+	status = input_register_device(keypad_data->input);
+	if (status < 0) {
+		dev_err(&pdev->dev, "failed to register input device\n");
+		goto failed_free_device;
+	}
+
+	omap_keypad_config(keypad_data);
+
+	platform_set_drvdata(pdev, keypad_data);
+
+	return 0;
+
+failed_free_device:
+	input_unregister_device(keypad_data->input);
+failed_free_irq:
+	free_irq(keypad_data->irq, pdev);
+failed_free_base:
+	keypad_data->base = NULL;
+failed_free_input:
+	input_free_device(input_dev);
+failed_free_codes:
+	kfree(keypad_codes);
+failed_free_data:
+	kfree(keypad_data);
+failed_free_platform:
+	kfree(keymap_data);
+	kfree(pdata);
+	return error;
+}
+
+static int __devexit omap_keypad_remove(struct platform_device *pdev)
+{
+	struct omap_keypad *keypad_data = platform_get_drvdata(pdev);
+
+	input_unregister_device(keypad_data->input);
+	free_irq(keypad_data->irq, keypad_data);
+	platform_set_drvdata(pdev, NULL);
+	kfree(keypad_data);
+
+	return 0;
+}
+
+static struct platform_driver omap_keypad_driver = {
+	.probe		= omap_keypad_probe,
+	.remove		= __devexit_p(omap_keypad_remove),
+	.driver		= {
+		.name	= "omap4-keypad",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init omap_keypad_init(void)
+{
+	return platform_driver_register(&omap_keypad_driver);
+}
+
+static void __exit omap_keypad_exit(void)
+{
+	platform_driver_unregister(&omap_keypad_driver);
+}
+
+module_init(omap_keypad_init);
+module_exit(omap_keypad_exit);
+
+MODULE_AUTHOR("Texas Instruments");
+MODULE_DESCRIPTION("OMAP4 Keypad Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:omap4-keypad");
-- 
1.5.4.3

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Trilok Soni]

Hi Abraham,

On Wed, Apr 14, 2010 at 6:40 AM, Arce, Abraham <x0066660@ti.com> wrote:
> Keyboard controller for OMAP4 with built-in scanning algorithm.
> The following implementations are used:
>
>  - matrix_keypac.c logic
>  - hwmod framework

Do we have hwmod framework mainlined in the kernel?

>
> +config KEYBOARD_OMAP4
> +        tristate "TI OMAP4 keypad support"
> +        depends on (ARCH_OMAP4)

No need of brackets.

Could you please provide "default y/n" option too?

> + * linux/drivers/input/keyboard/omap4-keypad.c

Better not to include file paths.

> +
> +struct omap_device_pm_latency omap_keyboard_latency[] = {

static?

> +       {
> +               .deactivate_func = omap_device_idle_hwmods,
> +               .activate_func   = omap_device_enable_hwmods,
> +               .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> +       },
> +};
> +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> +{
> +       struct omap_keypad *keypad_data = dev_id;
> +       struct input_dev *input = keypad_data->input;
> +       unsigned char key_state[8];
> +       int col, row, code;
> +       u32 *new_state = (u32 *) key_state;
> +
> +       *new_state       = __raw_readl(keypad_data->base +
> +                       OMAP4_KBD_FULLCODE31_0);
> +       *(new_state + 1) = __raw_readl(keypad_data->base +
> +                       OMAP4_KBD_FULLCODE63_32);
> +
> +       for (col = 0; col < keypad_data->cols; col++) {
> +               for (row = 0; row < keypad_data->rows; row++) {
> +                       code = MATRIX_SCAN_CODE(row, col,
> +                                       keypad_data->row_shift);
> +                       if (code < 0) {
> +                               printk(KERN_INFO "Spurious key %d-%d\n",
> +                                               col, row);
> +                               continue;
> +                       }
> +                       input_report_key(input, keypad_data->keycodes[code],
> +                                       key_state[col] & (1 << row));
> +               }
> +       }

where is input_sync(..)?

> +
> +       pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
> +                               GFP_KERNEL);

Please check error return here, because kzalloc can fail.

> +
> +       od = omap_device_build(name, id, oh, pdata,
> +                               sizeof(struct matrix_keypad_platform_data),
> +                               omap_keyboard_latency,
> +                               ARRAY_SIZE(omap_keyboard_latency), 1);
> +       WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
> +               name, oh_name);
> +
> +       /* platform data */
> +       pdata = pdev->dev.platform_data;
> +       if (!pdata) {
> +               dev_err(&pdev->dev, "no platform data defined\n");
> +               kfree(pdata);
> +               return -EINVAL;
> +       }
> +
> +       /* keymap data */
> +       keymap_data = pdata->keymap_data;
> +       if (!keymap_data) {
> +               dev_err(&pdev->dev, "no keymap data defined\n");
> +               kfree(keymap_data);

not freeing kfree(pdata)? Please check error return path again in this driver.

> +
> +       __set_bit(EV_REP, input_dev->evbit);
> +       __set_bit(KEY_OK, input_dev->keybit);
> +       __set_bit(EV_KEY, input_dev->evbit);

> +
> +       input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> +       input_set_drvdata(input_dev, keypad_data);
> +
> +       status = input_register_device(keypad_data->input);
> +       if (status < 0) {
> +               dev_err(&pdev->dev, "failed to register input device\n");
> +               goto failed_free_device;
> +       }
> +
> +       omap_keypad_config(keypad_data);
> +
> +       platform_set_drvdata(pdev, keypad_data);
> +
> +       return 0;
> +
> +failed_free_device:
> +       input_unregister_device(keypad_data->input);

add keypad_data->input = NULL.


-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
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: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

Hi Trilok!

> >  - matrix_keypac.c logic
> >  - hwmod framework
> 
> Do we have hwmod framework mainlined in the kernel?

Not yet but wanted to gather initial comments to be ready once framework is pushed

> 
> >
> > +config KEYBOARD_OMAP4
> > +        tristate "TI OMAP4 keypad support"
> > +        depends on (ARCH_OMAP4)
> 
> No need of brackets.

Removed

> 
> Could you please provide "default y/n" option too?

Added

> 
> > + * linux/drivers/input/keyboard/omap4-keypad.c
> 
> Better not to include file paths.

Removed path... only filename

> 
> > +
> > +struct omap_device_pm_latency omap_keyboard_latency[] = {
> 
> static?

Not sure... I'll check

<snip>

> > +                       input_report_key(input, keypad_data->keycodes[code],
> > +                                       key_state[col] & (1 << row));
> > +               }
> > +       }
> 
> where is input_sync(..)?

Added

> 
> > +
> > +       pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
> > +                               GFP_KERNEL);
> 
> Please check error return here, because kzalloc can fail.

Added

<snip>

> > +
> > +       /* keymap data */
> > +       keymap_data = pdata->keymap_data;
> > +       if (!keymap_data) {
> > +               dev_err(&pdev->dev, "no keymap data defined\n");
> > +               kfree(keymap_data);
> 
> not freeing kfree(pdata)? Please check error return path again in this driver.

I'll check error return in all code

<snip>

> > +
> > +failed_free_device:
> > +       input_unregister_device(keypad_data->input);
> 
> add keypad_data->input = NULL.

Added

Best Regards
Abraham
--
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: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Felipe Balbi]

Hi,

On Wed, Apr 14, 2010 at 03:10:48AM +0200, ext Arce, Abraham wrote:
>+#include <linux/module.h>
>+#include <linux/init.h>
>+#include <linux/interrupt.h>
>+#include <linux/platform_device.h>
>+#include <linux/errno.h>
>+#include <linux/io.h>
>+#include <linux/input.h>
>+#include <linux/input/matrix_keypad.h>
>+#include <plat/omap_hwmod.h>
>+#include <plat/omap_device.h>

should the platform_driver know about hwmod and omap_device ? Paul ? 
Kevin ?

>+struct omap_keypad {
>+

unnecessary blank line.

>+       struct platform_device *pdev;

generaly we save the struct device *.

>+       struct omap_hwmod *oh;
>+       const struct matrix_keypad_platform_data *pdata;

you should not save the platform_data here. Generaly you should only use 
that to initialize fields on your structure. Remember that a 
platform_data will be declared as __initdata.

>+/* Interrupt thread primary handler, called in hard interrupt context */
>+
>+static irqreturn_t omap_keypad_interrupt(int irq, void *dev_id)
>+{
>+       struct omap_keypad *keypad_data = dev_id;
>+
>+       /* disable interrupts */
>+       __raw_writel(OMAP4_DEF_IRQDISABLE, keypad_data->base +
>+                               OMAP4_KBD_IRQENABLE);
>+
>+       /* wake up handler thread */
>+       return IRQ_WAKE_THREAD;
>+
>+}
>+
>+/* Interrupt thread handler thread */
>+
>+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
>+{
>+       struct omap_keypad *keypad_data = dev_id;
>+       struct input_dev *input = keypad_data->input;
>+       unsigned char key_state[8];
>+       int col, row, code;
>+       u32 *new_state = (u32 *) key_state;
>+
>+       *new_state       = __raw_readl(keypad_data->base +
>+                       OMAP4_KBD_FULLCODE31_0);
>+       *(new_state + 1) = __raw_readl(keypad_data->base +
>+                       OMAP4_KBD_FULLCODE63_32);
>+
>+       for (col = 0; col < keypad_data->cols; col++) {
>+               for (row = 0; row < keypad_data->rows; row++) {
>+                       code = MATRIX_SCAN_CODE(row, col,
>+                                       keypad_data->row_shift);
>+                       if (code < 0) {
>+                               printk(KERN_INFO "Spurious key %d-%d\n",
>+                                               col, row);

use dev_dbg()

>+                               continue;
>+                       }
>+                       input_report_key(input, keypad_data->keycodes[code],
>+                                       key_state[col] & (1 << row));

missing input_sync()

>+static int __devinit omap_keypad_probe(struct platform_device *pdev)
>+{

[..]

>+       length = snprintf(oh_name, hw_mod_name_len, "keyboard");
>+       WARN(length >= hw_mod_name_len,
>+               "String buffer overflow in keyboard device setup\n");

you're using snprintf() this WARN() shouldn't happen even, remove it.

>+
>+       oh = omap_hwmod_lookup(oh_name);
>+       if (!oh)
>+               pr_err("Could not look up %s\n", oh_name);

I think omap_hwmod and omap_device shouldn't be here, but at least use 
dev_err();

>+
>+       pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
>+                               GFP_KERNEL);

this should come from platform code.

>+
>+       od = omap_device_build(name, id, oh, pdata,
>+                               sizeof(struct matrix_keypad_platform_data),
>+                               omap_keyboard_latency,
>+                               ARRAY_SIZE(omap_keyboard_latency), 1);
>+       WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
>+               name, oh_name);

this too.

>+       status = input_register_device(keypad_data->input);
>+       if (status < 0) {
>+               dev_err(&pdev->dev, "failed to register input device\n");
>+               goto failed_free_device;
>+       }
>+
>+       omap_keypad_config(keypad_data);

registering and configuring should be done before requesting the irq.

-- 
balbi

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Kevin Hilman]

"Arce, Abraham" <x0066660@ti.com> writes:

> Hi Trilok!
>
>> >  - matrix_keypac.c logic
>> >  - hwmod framework
>> 
>> Do we have hwmod framework mainlined in the kernel?
>
> Not yet but wanted to gather initial comments to be ready once framework is pushed

Just to clarify, hwmod framework is already in mainline.  

Only thing missing to test this driver is adding the hwmod data for
this device.

Kevin
--
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: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Kevin Hilman]

"Arce, Abraham" <x0066660@ti.com> writes:

> Keyboard controller for OMAP4 with built-in scanning algorithm.
> The following implementations are used:
>
>   - matrix_keypac.c logic
>   - hwmod framework
>   - threaded irq
>
> Signed-off-by: Syed Rafiuddin <rafiuddin.syed@ti.com>
> Signed-off-by: Abraham Arce <x0066660@ti.com>

Some general comments...

What's missing here is a separation of the driver and the device.
What you need is for arch code to register a platfrom_device (using
hwmod + omap_device).  Then this driver will use the standard
platform_device resource calls to get its base address, IRQs, etc. and
any platform_data.

IOW, as Felipe mentioned, the driver should not be doing the hwmod +
omap_device init and registration.

Kevin

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Dmitry Torokhov]

Hi Abraham,

On Tue, Apr 13, 2010 at 08:10:48PM -0500, Arce, Abraham wrote:
> Keyboard controller for OMAP4 with built-in scanning algorithm.
> The following implementations are used:
> 
>   - matrix_keypac.c logic
>   - hwmod framework
>   - threaded irq
> 
> Signed-off-by: Syed Rafiuddin <rafiuddin.syed@ti.com>
> Signed-off-by: Abraham Arce <x0066660@ti.com>
> ---
>  drivers/input/keyboard/Kconfig        |    9 +
>  drivers/input/keyboard/Makefile       |    1 +
>  drivers/input/keyboard/omap4-keypad.c |  367 +++++++++++++++++++++++++++++++++
>  3 files changed, 377 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/keyboard/omap4-keypad.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 5049c3c..8a4103e 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -390,6 +390,15 @@ config KEYBOARD_OMAP
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called omap-keypad.
>  
> +config KEYBOARD_OMAP4
> +        tristate "TI OMAP4 keypad support"
> +        depends on (ARCH_OMAP4)
> +        help
> +          Say Y here if you want to use the OMAP4 keypad.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called omap4-keypad.
> +
>  config KEYBOARD_TWL4030
>  	tristate "TI TWL4030/TWL5030/TPS659x0 keypad support"
>  	depends on TWL4030_CORE
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 78654ef..1b3dfbe 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
>  obj-$(CONFIG_KEYBOARD_MAX7359)		+= max7359_keypad.o
>  obj-$(CONFIG_KEYBOARD_NEWTON)		+= newtonkbd.o
>  obj-$(CONFIG_KEYBOARD_OMAP)		+= omap-keypad.o
> +obj-$(CONFIG_KEYBOARD_OMAP4)		+= omap4-keypad.o
>  obj-$(CONFIG_KEYBOARD_OPENCORES)	+= opencores-kbd.o
>  obj-$(CONFIG_KEYBOARD_PXA27x)		+= pxa27x_keypad.o
>  obj-$(CONFIG_KEYBOARD_PXA930_ROTARY)	+= pxa930_rotary.o
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> new file mode 100644
> index 0000000..b656b4f
> --- /dev/null
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -0,0 +1,367 @@
> +/*
> + * linux/drivers/input/keyboard/omap4-keypad.c
> + *
> + * OMAP4 Keypad Driver
> + *
> + * Copyright (C) 2010 Texas Instruments
> + *
> + * Author: Abraham Arce <x0066660@ti.com>
> + * Initial Code: Syed Rafiuddin <rafiuddin.syed@ti.com>
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/input.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <plat/omap_hwmod.h>
> +#include <plat/omap_device.h>
> +
> +/* OMAP4 registers */
> +#define OMAP4_KBD_REVISION		0x00
> +#define OMAP4_KBD_SYSCONFIG		0x10
> +#define OMAP4_KBD_SYSSTATUS		0x14
> +#define OMAP4_KBD_IRQSTATUS		0x18
> +#define OMAP4_KBD_IRQENABLE		0x1C
> +#define OMAP4_KBD_WAKEUPENABLE		0x20
> +#define OMAP4_KBD_PENDING		0x24
> +#define OMAP4_KBD_CTRL			0x28
> +#define OMAP4_KBD_DEBOUNCINGTIME	0x2C
> +#define OMAP4_KBD_LONGKEYTIME		0x30
> +#define OMAP4_KBD_TIMEOUT		0x34
> +#define OMAP4_KBD_STATEMACHINE		0x38
> +#define OMAP4_KBD_ROWINPUTS		0x3C
> +#define OMAP4_KBD_COLUMNOUTPUTS		0x40
> +#define OMAP4_KBD_FULLCODE31_0		0x44
> +#define OMAP4_KBD_FULLCODE63_32		0x48
> +
> +/* OMAP4 bit definitions */
> +#define OMAP4_DEF_SYSCONFIG_SOFTRST	(1 << 1)
> +#define OMAP4_DEF_SYSCONFIG_ENAWKUP	(1 << 2)
> +#define OMAP4_DEF_IRQENABLE_EVENTEN	(1 << 0)
> +#define OMAP4_DEF_IRQENABLE_LONGKEY	(1 << 1)
> +#define OMAP4_DEF_IRQENABLE_TIMEOUTEN	(1 << 2)
> +#define OMAP4_DEF_CTRL_NOSOFTMODE	(1 << 1)
> +#define OMAP4_DEF_CTRLPTVVALUE		(1 << 2)
> +#define OMAP4_DEF_CTRLPTV		(1 << 1)
> +#define OMAP4_DEF_IRQDISABLE		0x00
> +
> +/* OMAP4 values */
> +#define OMAP4_VAL_DEBOUNCINGTIME	0x07
> +#define OMAP4_VAL_FUNCTIONALCFG		0x1E
> +
> +#define OMAP4_MASK_IRQSTATUSDISABLE	0xFFFF
> +
> +struct omap_keypad {
> +
> +	struct platform_device *pdev;
> +	struct omap_hwmod *oh;
> +	const struct matrix_keypad_platform_data *pdata;
> +
> +	void __iomem    *base;
> +
> +	struct input_dev *input;
> +
> +	int irq;
> +
> +	unsigned short *keycodes;
> +	unsigned int rows;
> +	unsigned int cols;
> +	unsigned int debounce;
> +	unsigned int row_shift;
> +	unsigned char key_state[8];
> +};
> +
> +struct omap_device_pm_latency omap_keyboard_latency[] = {
> +	{
> +		.deactivate_func = omap_device_idle_hwmods,
> +		.activate_func   = omap_device_enable_hwmods,
> +		.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> +	},
> +};
> +
> +static void __init omap_keypad_config(struct omap_keypad *keypad_data)
> +{
> +	__raw_writel(OMAP4_DEF_SYSCONFIG_SOFTRST | OMAP4_DEF_SYSCONFIG_ENAWKUP,
> +			keypad_data->base + OMAP4_KBD_SYSCONFIG);
> +	__raw_writel(OMAP4_VAL_FUNCTIONALCFG,
> +			keypad_data->base + OMAP4_KBD_CTRL);
> +	__raw_writel(OMAP4_VAL_DEBOUNCINGTIME,
> +			keypad_data->base + OMAP4_KBD_DEBOUNCINGTIME);
> +	__raw_writel(OMAP4_DEF_IRQDISABLE,
> +			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> +	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> +			keypad_data->base + OMAP4_KBD_IRQENABLE);
> +}
> +
> +/* Interrupt thread primary handler, called in hard interrupt context */
> +
> +static irqreturn_t omap_keypad_interrupt(int irq, void *dev_id)
> +{
> +	struct omap_keypad *keypad_data = dev_id;
> +
> +	/* disable interrupts */
> +	__raw_writel(OMAP4_DEF_IRQDISABLE, keypad_data->base +
> +				OMAP4_KBD_IRQENABLE);
> +
> +	/* wake up handler thread */
> +	return IRQ_WAKE_THREAD;
> +
> +}
> +
> +/* Interrupt thread handler thread */
> +
> +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> +{

Why is iti threaded? I fo not see anything that will sleep.

> +	struct omap_keypad *keypad_data = dev_id;
> +	struct input_dev *input = keypad_data->input;
> +	unsigned char key_state[8];
> +	int col, row, code;
> +	u32 *new_state = (u32 *) key_state;
> +
> +	*new_state	 = __raw_readl(keypad_data->base +
> +			OMAP4_KBD_FULLCODE31_0);
> +	*(new_state + 1) = __raw_readl(keypad_data->base +
> +			OMAP4_KBD_FULLCODE63_32);
> +
> +	for (col = 0; col < keypad_data->cols; col++) {
> +		for (row = 0; row < keypad_data->rows; row++) {
> +			code = MATRIX_SCAN_CODE(row, col,
> +					keypad_data->row_shift);
> +			if (code < 0) {
> +				printk(KERN_INFO "Spurious key %d-%d\n",
> +						col, row);
> +				continue;
> +			}

MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
incorrect values in keymap...

> +			input_report_key(input, keypad_data->keycodes[code],
> +					key_state[col] & (1 << row));
> +		}
> +	}
> +
> +	memcpy(keypad_data->key_state, &key_state,
> +			sizeof(keypad_data->key_state));
> +
> +	/* enable interrupts */
> +	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> +			keypad_data->base + OMAP4_KBD_IRQENABLE);
> +
> +	/* clear pending interrupts */
> +	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> +			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> +
> +	/* clear pending interrupts */
> +	return IRQ_HANDLED;
> +
> +}
> +
> +
> +static int __devinit omap_keypad_probe(struct platform_device *pdev)
> +{
> +	struct omap_device *od;
> +	struct omap_hwmod *oh;
> +	struct matrix_keypad_platform_data *pdata;
> +	struct input_dev *input_dev;
> +	const struct matrix_keymap_data *keymap_data;
> +	struct omap_keypad *keypad_data;
> +
> +	unsigned int status = 0, row_shift = 0, error = 0, length = 0, id = 0;
> +	unsigned short *keypad_codes;
> +
> +	int hw_mod_name_len = 16;
> +	char oh_name[hw_mod_name_len];
> +	char *name = "omap4-keypad";
> +
> +	length = snprintf(oh_name, hw_mod_name_len, "keyboard");
> +	WARN(length >= hw_mod_name_len,
> +		"String buffer overflow in keyboard device setup\n");
> +
> +	oh = omap_hwmod_lookup(oh_name);
> +	if (!oh)
> +		pr_err("Could not look up %s\n", oh_name);
> +
> +	pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
> +				GFP_KERNEL);

Why is it allocated?

> +
> +	od = omap_device_build(name, id, oh, pdata,
> +				sizeof(struct matrix_keypad_platform_data),
> +				omap_keyboard_latency,
> +				ARRAY_SIZE(omap_keyboard_latency), 1);
> +	WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
> +		name, oh_name);
> +
> +	/* platform data */
> +	pdata = pdev->dev.platform_data;

Umm, here you are overriding pdata pointer... don't you leak memory
doing this?


> +	if (!pdata) {
> +		dev_err(&pdev->dev, "no platform data defined\n");
> +		kfree(pdata);
> +		return -EINVAL;
> +	}
> +
> +	/* keymap data */
> +	keymap_data = pdata->keymap_data;
> +	if (!keymap_data) {
> +		dev_err(&pdev->dev, "no keymap data defined\n");
> +		kfree(keymap_data);
> +		return -EINVAL;
> +	}
> +
> +	/* omap keypad data */
> +	row_shift = get_count_order(pdata->num_col_gpios);
> +	keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
> +	if (!keypad_data) {
> +		error = -ENOMEM;
> +		goto failed_free_platform;
> +	}
> +
> +	/* omap keypad codes */
> +	keypad_codes = kzalloc((pdata->num_row_gpios << row_shift) *
> +			sizeof(*keypad_codes), GFP_KERNEL);

Why is it al;loctaed separately? Can it be pickky-backed onto
keypad_data?

> +	if (!keypad_codes) {
> +		error = -ENOMEM;
> +		goto failed_free_data;
> +	}
> +
> +	/* input device allocation */
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		error = -ENOMEM;
> +		goto failed_free_codes;
> +	}
> +
> +	/* base resource */
> +	keypad_data->base = oh->_rt_va;
> +	if (!keypad_data->base) {


You could probably check this earlier, before allocating stuff.

> +		error = -ENOMEM;
> +		goto failed_free_input;
> +	}
> +
> +	/* irq */
> +	keypad_data->irq = oh->mpu_irqs[0].irq;
> +	if (keypad_data->irq < 0) {

You could probably check this earlier, before allocating stuff.

> +		dev_err(&pdev->dev, "no keyboard irq assigned\n");
> +		error = keypad_data->irq;
> +		goto failed_free_base;
> +	}
> +
> +	/* threaded irq init */
> +	status = request_threaded_irq(keypad_data->irq, omap_keypad_interrupt,
> +			omap_keypad_threaded,
> +				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +			"omap4-keypad", keypad_data);
> +	if (status) {
> +		dev_err(&pdev->dev, "failed to register interrupt\n");
> +		error = -ENOMEM;
> +		goto failed_free_irq;
> +	}
> +
> +	keypad_data->input = input_dev;
> +	keypad_data->row_shift = row_shift;
> +	keypad_data->rows = pdata->num_row_gpios;
> +	keypad_data->cols = pdata->num_col_gpios;
> +	keypad_data->keycodes = keypad_codes;
> +
> +	input_dev->name = pdev->name;
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->dev.parent = &pdev->dev;
> +	input_dev->id.vendor = 0x0001;
> +	input_dev->id.product = 0x0001;
> +	input_dev->id.version = 0x0100;
> +
> +	input_dev->keycode	= keypad_codes;
> +	input_dev->keycodesize	= sizeof(*keypad_codes);

You also need keycodemax so that keymap can be changed from userpsace.

> +
> +	matrix_keypad_build_keymap(keymap_data, row_shift,
> +				   input_dev->keycode, input_dev->keybit);
> +
> +	__set_bit(EV_REP, input_dev->evbit);
> +	__set_bit(KEY_OK, input_dev->keybit);

Why KEY_OK is set up separately?

> +	__set_bit(EV_KEY, input_dev->evbit);
> +
> +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);

You forgot to actualy report MSC_SCAN though.

> +	input_set_drvdata(input_dev, keypad_data);
> +
> +	status = input_register_device(keypad_data->input);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "failed to register input device\n");
> +		goto failed_free_device;
> +	}
> +
> +	omap_keypad_config(keypad_data);
> +
> +	platform_set_drvdata(pdev, keypad_data);
> +
> +	return 0;
> +
> +failed_free_device:
> +	input_unregister_device(keypad_data->input);

	input_dev = NULL;

or you will try to free it later.

> +failed_free_irq:
> +	free_irq(keypad_data->irq, pdev);
> +failed_free_base:
> +	keypad_data->base = NULL;
> +failed_free_input:
> +	input_free_device(input_dev);
> +failed_free_codes:
> +	kfree(keypad_codes);
> +failed_free_data:
> +	kfree(keypad_data);
> +failed_free_platform:
> +	kfree(keymap_data);
> +	kfree(pdata);
> +	return error;
> +}
> +
> +static int __devexit omap_keypad_remove(struct platform_device *pdev)
> +{
> +	struct omap_keypad *keypad_data = platform_get_drvdata(pdev);
> +
> +	input_unregister_device(keypad_data->input);
> +	free_irq(keypad_data->irq, keypad_data);

Thsi should happen before unregistering input device, othersie there is
a change IRQ will arrive when input device is freed.

> +	platform_set_drvdata(pdev, NULL);
> +	kfree(keypad_data);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver omap_keypad_driver = {
> +	.probe		= omap_keypad_probe,
> +	.remove		= __devexit_p(omap_keypad_remove),
> +	.driver		= {
> +		.name	= "omap4-keypad",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init omap_keypad_init(void)
> +{
> +	return platform_driver_register(&omap_keypad_driver);
> +}
> +
> +static void __exit omap_keypad_exit(void)
> +{
> +	platform_driver_unregister(&omap_keypad_driver);
> +}
> +
> +module_init(omap_keypad_init);
> +module_exit(omap_keypad_exit);
> +
> +MODULE_AUTHOR("Texas Instruments");
> +MODULE_DESCRIPTION("OMAP4 Keypad Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:omap4-keypad");

-- 
Dmitry

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

Felipe,

Thanks for your comments...

[..]

> >+#include <plat/omap_hwmod.h>
> >+#include <plat/omap_device.h>
> 
> should the platform_driver know about hwmod and omap_device ? Paul ?
> Kevin ?

Working on these changes...

> 
> >+struct omap_keypad {
> >+
> 
> unnecessary blank line.

Removed

> 
> >+       struct platform_device *pdev;
> 
> generaly we save the struct device *.
> 
> >+       struct omap_hwmod *oh;
> >+       const struct matrix_keypad_platform_data *pdata;
> 
> you should not save the platform_data here. Generaly you should only use
> that to initialize fields on your structure. Remember that a
> platform_data will be declared as __initdata.

Removed

> 
> >+/* Interrupt thread primary handler, called in hard interrupt context */
> >+
> >+static irqreturn_t omap_keypad_interrupt(int irq, void *dev_id)
> >+{
> >+       struct omap_keypad *keypad_data = dev_id;
> >+
> >+       /* disable interrupts */
> >+       __raw_writel(OMAP4_DEF_IRQDISABLE, keypad_data->base +
> >+                               OMAP4_KBD_IRQENABLE);
> >+
> >+       /* wake up handler thread */
> >+       return IRQ_WAKE_THREAD;
> >+
> >+}
> >+
> >+/* Interrupt thread handler thread */
> >+
> >+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> >+{
> >+       struct omap_keypad *keypad_data = dev_id;
> >+       struct input_dev *input = keypad_data->input;
> >+       unsigned char key_state[8];
> >+       int col, row, code;
> >+       u32 *new_state = (u32 *) key_state;
> >+
> >+       *new_state       = __raw_readl(keypad_data->base +
> >+                       OMAP4_KBD_FULLCODE31_0);
> >+       *(new_state + 1) = __raw_readl(keypad_data->base +
> >+                       OMAP4_KBD_FULLCODE63_32);
> >+
> >+       for (col = 0; col < keypad_data->cols; col++) {
> >+               for (row = 0; row < keypad_data->rows; row++) {
> >+                       code = MATRIX_SCAN_CODE(row, col,
> >+                                       keypad_data->row_shift);
> >+                       if (code < 0) {
> >+                               printk(KERN_INFO "Spurious key %d-%d\n",
> >+                                               col, row);
> 
> use dev_dbg()

Added

> 
> >+                               continue;
> >+                       }
> >+                       input_report_key(input, keypad_data->keycodes[code],
> >+                                       key_state[col] & (1 << row));
> 
> missing input_sync()

Added

> 
> >+static int __devinit omap_keypad_probe(struct platform_device *pdev)
> >+{
> 
> [..]
> 
> >+       length = snprintf(oh_name, hw_mod_name_len, "keyboard");
> >+       WARN(length >= hw_mod_name_len,
> >+               "String buffer overflow in keyboard device setup\n");
> 
> you're using snprintf() this WARN() shouldn't happen even, remove it.

Code now in board file, removed

> 
> >+
> >+       oh = omap_hwmod_lookup(oh_name);
> >+       if (!oh)
> >+               pr_err("Could not look up %s\n", oh_name);
> 
> I think omap_hwmod and omap_device shouldn't be here, but at least use
> dev_err();

Moving to board file...

> 
> >+
> >+       pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
> >+                               GFP_KERNEL);
> 
> this should come from platform code.

Working on these changes...


I'll have a function in 4430sdp board file that will setup hwmod and omap device called

  sdp4430_keypad_init() {
     ...
     omap_hwmod_lookup(oh_name);
     kzalloc(sizeof(struct matrix_keypad_platform_data), GFP_KERNEL);
     omap_device_build(name, id, oh, pdata,
     ...
  }

and then called within

  omap_4430sdp_init()

> 
> >+
> >+       od = omap_device_build(name, id, oh, pdata,
> >+                               sizeof(struct matrix_keypad_platform_data),
> >+                               omap_keyboard_latency,
> >+                               ARRAY_SIZE(omap_keyboard_latency), 1);
> >+       WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
> >+               name, oh_name);
> 
> this too.

Moving to board file...

> 
> >+       status = input_register_device(keypad_data->input);
> >+       if (status < 0) {
> >+               dev_err(&pdev->dev, "failed to register input device\n");
> >+               goto failed_free_device;
> >+       }
> >+
> >+       omap_keypad_config(keypad_data);
> 
> registering and configuring should be done before requesting the irq.

Changed


Best Regards
Abraham

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

Thanks Kevin,

> > Keyboard controller for OMAP4 with built-in scanning algorithm.
> > The following implementations are used:
> >
> >   - matrix_keypac.c logic
> >   - hwmod framework
> >   - threaded irq
> >
> > Signed-off-by: Syed Rafiuddin <rafiuddin.syed@ti.com>
> > Signed-off-by: Abraham Arce <x0066660@ti.com>
> 
> Some general comments...
> 
> What's missing here is a separation of the driver and the device.
> What you need is for arch code to register a platfrom_device (using
> hwmod + omap_device).  Then this driver will use the standard
> platform_device resource calls to get its base address, IRQs, etc. and
> any platform_data.
> 
> IOW, as Felipe mentioned, the driver should not be doing the hwmod +
> omap_device init and registration.

As metioned to Felipe, my approach is 

  sdp4430_keypad_init() {
     ...
     omap_hwmod_lookup(oh_name);
     ...
     kzalloc(sizeof(struct matrix_keypad_platform_data), GFP_KERNEL);
     ...
     omap_device_build(name, id, oh, pdata,
     ...
  }

and then called within

  omap_4430sdp_init()


Best Regards
Abraham

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

Hi Trilok, 

Thanks for your comments...

[snip]

> > +
> > +/* Interrupt thread handler thread */
> > +
> > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > +{
> 
> Why is iti threaded? I fo not see anything that will sleep.

It was implemented based on previous comments...

> 
> > +	struct omap_keypad *keypad_data = dev_id;
> > +	struct input_dev *input = keypad_data->input;
> > +	unsigned char key_state[8];
> > +	int col, row, code;
> > +	u32 *new_state = (u32 *) key_state;
> > +
> > +	*new_state	 = __raw_readl(keypad_data->base +
> > +			OMAP4_KBD_FULLCODE31_0);
> > +	*(new_state + 1) = __raw_readl(keypad_data->base +
> > +			OMAP4_KBD_FULLCODE63_32);
> > +
> > +	for (col = 0; col < keypad_data->cols; col++) {
> > +		for (row = 0; row < keypad_data->rows; row++) {
> > +			code = MATRIX_SCAN_CODE(row, col,
> > +					keypad_data->row_shift);
> > +			if (code < 0) {
> > +				printk(KERN_INFO "Spurious key %d-%d\n",
> > +						col, row);
> > +				continue;
> > +			}
> 
> MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
> incorrect values in keymap...

Removed

> 
> > +			input_report_key(input, keypad_data->keycodes[code],
> > +					key_state[col] & (1 << row));
> > +		}
> > +	}
> > +
> > +	memcpy(keypad_data->key_state, &key_state,
> > +			sizeof(keypad_data->key_state));
> > +
> > +	/* enable interrupts */
> > +	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> > +			keypad_data->base + OMAP4_KBD_IRQENABLE);
> > +
> > +	/* clear pending interrupts */
> > +	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> > +			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> > +
> > +	/* clear pending interrupts */
> > +	return IRQ_HANDLED;
> > +
> > +}
> > +
> > +
> > +static int __devinit omap_keypad_probe(struct platform_device *pdev)
> > +{
> > +	struct omap_device *od;
> > +	struct omap_hwmod *oh;
> > +	struct matrix_keypad_platform_data *pdata;
> > +	struct input_dev *input_dev;
> > +	const struct matrix_keymap_data *keymap_data;
> > +	struct omap_keypad *keypad_data;
> > +
> > +	unsigned int status = 0, row_shift = 0, error = 0, length = 0, id = 0;
> > +	unsigned short *keypad_codes;
> > +
> > +	int hw_mod_name_len = 16;
> > +	char oh_name[hw_mod_name_len];
> > +	char *name = "omap4-keypad";
> > +
> > +	length = snprintf(oh_name, hw_mod_name_len, "keyboard");
> > +	WARN(length >= hw_mod_name_len,
> > +		"String buffer overflow in keyboard device setup\n");
> > +
> > +	oh = omap_hwmod_lookup(oh_name);
> > +	if (!oh)
> > +		pr_err("Could not look up %s\n", oh_name);
> > +
> > +	pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
> > +				GFP_KERNEL);
> 
> Why is it allocated?

Moved to board file...

> 
> > +
> > +	od = omap_device_build(name, id, oh, pdata,
> > +				sizeof(struct matrix_keypad_platform_data),
> > +				omap_keyboard_latency,
> > +				ARRAY_SIZE(omap_keyboard_latency), 1);
> > +	WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
> > +		name, oh_name);
> > +
> > +	/* platform data */
> > +	pdata = pdev->dev.platform_data;
> 
> Umm, here you are overriding pdata pointer... don't you leak memory
> doing this?

hwmod has been moved to board configuration so we need to get platform data

> 
> 
> > +	if (!pdata) {
> > +		dev_err(&pdev->dev, "no platform data defined\n");
> > +		kfree(pdata);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* keymap data */
> > +	keymap_data = pdata->keymap_data;
> > +	if (!keymap_data) {
> > +		dev_err(&pdev->dev, "no keymap data defined\n");
> > +		kfree(keymap_data);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* omap keypad data */
> > +	row_shift = get_count_order(pdata->num_col_gpios);
> > +	keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
> > +	if (!keypad_data) {
> > +		error = -ENOMEM;
> > +		goto failed_free_platform;
> > +	}
> > +
> > +	/* omap keypad codes */
> > +	keypad_codes = kzalloc((pdata->num_row_gpios << row_shift) *
> > +			sizeof(*keypad_codes), GFP_KERNEL);
> 
> Why is it al;loctaed separately? Can it be pickky-backed onto
> keypad_data?

Not getting the idea here... could you explain?

> 
> > +	if (!keypad_codes) {
> > +		error = -ENOMEM;
> > +		goto failed_free_data;
> > +	}
> > +
> > +	/* input device allocation */
> > +	input_dev = input_allocate_device();
> > +	if (!input_dev) {
> > +		error = -ENOMEM;
> > +		goto failed_free_codes;
> > +	}
> > +
> > +	/* base resource */
> > +	keypad_data->base = oh->_rt_va;
> > +	if (!keypad_data->base) {
> 
> 
> You could probably check this earlier, before allocating stuff.

Early means before input_allocate_device?

> 
> > +		error = -ENOMEM;
> > +		goto failed_free_input;
> > +	}
> > +
> > +	/* irq */
> > +	keypad_data->irq = oh->mpu_irqs[0].irq;
> > +	if (keypad_data->irq < 0) {
> 
> You could probably check this earlier, before allocating stuff.

Early means before input_allocate_device?

> 
> > +		dev_err(&pdev->dev, "no keyboard irq assigned\n");
> > +		error = keypad_data->irq;
> > +		goto failed_free_base;
> > +	}
> > +
> > +	/* threaded irq init */
> > +	status = request_threaded_irq(keypad_data->irq, omap_keypad_interrupt,
> > +			omap_keypad_threaded,
> > +				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > +			"omap4-keypad", keypad_data);
> > +	if (status) {
> > +		dev_err(&pdev->dev, "failed to register interrupt\n");
> > +		error = -ENOMEM;
> > +		goto failed_free_irq;
> > +	}
> > +
> > +	keypad_data->input = input_dev;
> > +	keypad_data->row_shift = row_shift;
> > +	keypad_data->rows = pdata->num_row_gpios;
> > +	keypad_data->cols = pdata->num_col_gpios;
> > +	keypad_data->keycodes = keypad_codes;
> > +
> > +	input_dev->name = pdev->name;
> > +	input_dev->id.bustype = BUS_HOST;
> > +	input_dev->dev.parent = &pdev->dev;
> > +	input_dev->id.vendor = 0x0001;
> > +	input_dev->id.product = 0x0001;
> > +	input_dev->id.version = 0x0100;
> > +
> > +	input_dev->keycode	= keypad_codes;
> > +	input_dev->keycodesize	= sizeof(*keypad_codes);
> 
> You also need keycodemax so that keymap can be changed from userpsace.

Added!

> 
> > +
> > +	matrix_keypad_build_keymap(keymap_data, row_shift,
> > +				   input_dev->keycode, input_dev->keybit);
> > +
> > +	__set_bit(EV_REP, input_dev->evbit);
> > +	__set_bit(KEY_OK, input_dev->keybit);
> 
> Why KEY_OK is set up separately?

In previous implementation manual set of bit was needed, now removed...

> 
> > +	__set_bit(EV_KEY, input_dev->evbit);
> > +
> > +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> 
> You forgot to actualy report MSC_SCAN though.

Should I remove it? I think I do need it only when KEY_UNKNOWN keys will be remapped, right?

> 
> > +	input_set_drvdata(input_dev, keypad_data);
> > +
> > +	status = input_register_device(keypad_data->input);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "failed to register input device\n");
> > +		goto failed_free_device;
> > +	}
> > +
> > +	omap_keypad_config(keypad_data);
> > +
> > +	platform_set_drvdata(pdev, keypad_data);
> > +
> > +	return 0;
> > +
> > +failed_free_device:
> > +	input_unregister_device(keypad_data->input);
> 
> 	input_dev = NULL;
> 
> or you will try to free it later.

Not getting the idea here... could you explain?

> 
> > +failed_free_irq:
> > +	free_irq(keypad_data->irq, pdev);
> > +failed_free_base:
> > +	keypad_data->base = NULL;
> > +failed_free_input:
> > +	input_free_device(input_dev);
> > +failed_free_codes:
> > +	kfree(keypad_codes);
> > +failed_free_data:
> > +	kfree(keypad_data);
> > +failed_free_platform:
> > +	kfree(keymap_data);
> > +	kfree(pdata);
> > +	return error;
> > +}
> > +
> > +static int __devexit omap_keypad_remove(struct platform_device *pdev)
> > +{
> > +	struct omap_keypad *keypad_data = platform_get_drvdata(pdev);
> > +
> > +	input_unregister_device(keypad_data->input);
> > +	free_irq(keypad_data->irq, keypad_data);
> 
> Thsi should happen before unregistering input device, othersie there is
> a change IRQ will arrive when input device is freed.

Done!


Best Regards
Abraham

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

Sorry for the confusion in your name Dmitry...

Thanks for your comments...

[snip]

> > > +
> > > +/* Interrupt thread handler thread */
> > > +
> > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > +{
> >
> > Why is iti threaded? I fo not see anything that will sleep.


It was implemented based on previous comments...


> > > +	struct omap_keypad *keypad_data = dev_id;
> > > +	struct input_dev *input = keypad_data->input;
> > > +	unsigned char key_state[8];
> > > +	int col, row, code;
> > > +	u32 *new_state = (u32 *) key_state;
> > > +
> > > +	*new_state	 = __raw_readl(keypad_data->base +
> > > +			OMAP4_KBD_FULLCODE31_0);
> > > +	*(new_state + 1) = __raw_readl(keypad_data->base +
> > > +			OMAP4_KBD_FULLCODE63_32);
> > > +
> > > +	for (col = 0; col < keypad_data->cols; col++) {
> > > +		for (row = 0; row < keypad_data->rows; row++) {
> > > +			code = MATRIX_SCAN_CODE(row, col,
> > > +					keypad_data->row_shift);
> > > +			if (code < 0) {
> > > +				printk(KERN_INFO "Spurious key %d-%d\n",
> > > +						col, row);
> > > +				continue;
> > > +			}
> >
> > MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
> > incorrect values in keymap...

Removed

> > > +			input_report_key(input, keypad_data->keycodes[code],
> > > +					key_state[col] & (1 << row));
> > > +		}
> > > +	}
> > > +
> > > +	memcpy(keypad_data->key_state, &key_state,
> > > +			sizeof(keypad_data->key_state));
> > > +
> > > +	/* enable interrupts */
> > > +	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> > > +			keypad_data->base + OMAP4_KBD_IRQENABLE);
> > > +
> > > +	/* clear pending interrupts */
> > > +	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> > > +			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> > > +
> > > +	/* clear pending interrupts */
> > > +	return IRQ_HANDLED;
> > > +
> > > +}
> > > +
> > > +
> > > +static int __devinit omap_keypad_probe(struct platform_device *pdev)
> > > +{
> > > +	struct omap_device *od;
> > > +	struct omap_hwmod *oh;
> > > +	struct matrix_keypad_platform_data *pdata;
> > > +	struct input_dev *input_dev;
> > > +	const struct matrix_keymap_data *keymap_data;
> > > +	struct omap_keypad *keypad_data;
> > > +
> > > +	unsigned int status = 0, row_shift = 0, error = 0, length = 0, id = 0;
> > > +	unsigned short *keypad_codes;
> > > +
> > > +	int hw_mod_name_len = 16;
> > > +	char oh_name[hw_mod_name_len];
> > > +	char *name = "omap4-keypad";
> > > +
> > > +	length = snprintf(oh_name, hw_mod_name_len, "keyboard");
> > > +	WARN(length >= hw_mod_name_len,
> > > +		"String buffer overflow in keyboard device setup\n");
> > > +
> > > +	oh = omap_hwmod_lookup(oh_name);
> > > +	if (!oh)
> > > +		pr_err("Could not look up %s\n", oh_name);
> > > +
> > > +	pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
> > > +				GFP_KERNEL);
> >
> > Why is it allocated?

Moved to board file...


> > > +
> > > +	od = omap_device_build(name, id, oh, pdata,
> > > +				sizeof(struct matrix_keypad_platform_data),
> > > +				omap_keyboard_latency,
> > > +				ARRAY_SIZE(omap_keyboard_latency), 1);
> > > +	WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
> > > +		name, oh_name);
> > > +
> > > +	/* platform data */
> > > +	pdata = pdev->dev.platform_data;
> >
> > Umm, here you are overriding pdata pointer... don't you leak memory
> > doing this?


hwmod has been moved to board configuration so we need to get platform data

> > > +	if (!pdata) {
> > > +		dev_err(&pdev->dev, "no platform data defined\n");
> > > +		kfree(pdata);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* keymap data */
> > > +	keymap_data = pdata->keymap_data;
> > > +	if (!keymap_data) {
> > > +		dev_err(&pdev->dev, "no keymap data defined\n");
> > > +		kfree(keymap_data);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* omap keypad data */
> > > +	row_shift = get_count_order(pdata->num_col_gpios);
> > > +	keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
> > > +	if (!keypad_data) {
> > > +		error = -ENOMEM;
> > > +		goto failed_free_platform;
> > > +	}
> > > +
> > > +	/* omap keypad codes */
> > > +	keypad_codes = kzalloc((pdata->num_row_gpios << row_shift) *
> > > +			sizeof(*keypad_codes), GFP_KERNEL);
> >
> > Why is it al;loctaed separately? Can it be pickky-backed onto
> > keypad_data?
 
Not getting the idea here... could you explain?
 
> > > +	if (!keypad_codes) {
> > > +		error = -ENOMEM;
> > > +		goto failed_free_data;
> > > +	}
> > > +
> > > +	/* input device allocation */
> > > +	input_dev = input_allocate_device();
> > > +	if (!input_dev) {
> > > +		error = -ENOMEM;
> > > +		goto failed_free_codes;
> > > +	}
> > > +
> > > +	/* base resource */
> > > +	keypad_data->base = oh->_rt_va;
> > > +	if (!keypad_data->base) {
> >
> >
> > You could probably check this earlier, before allocating stuff.

Early means before input_allocate_device?


> > > +		error = -ENOMEM;
> > > +		goto failed_free_input;
> > > +	}
> > > +
> > > +	/* irq */
> > > +	keypad_data->irq = oh->mpu_irqs[0].irq;
> > > +	if (keypad_data->irq < 0) {
> >
> > You could probably check this earlier, before allocating stuff.

Early means before input_allocate_device?
 
> > > +		dev_err(&pdev->dev, "no keyboard irq assigned\n");
> > > +		error = keypad_data->irq;
> > > +		goto failed_free_base;
> > > +	}
> > > +
> > > +	/* threaded irq init */
> > > +	status = request_threaded_irq(keypad_data->irq, omap_keypad_interrupt,
> > > +			omap_keypad_threaded,
> > > +				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > > +			"omap4-keypad", keypad_data);
> > > +	if (status) {
> > > +		dev_err(&pdev->dev, "failed to register interrupt\n");
> > > +		error = -ENOMEM;
> > > +		goto failed_free_irq;
> > > +	}
> > > +
> > > +	keypad_data->input = input_dev;
> > > +	keypad_data->row_shift = row_shift;
> > > +	keypad_data->rows = pdata->num_row_gpios;
> > > +	keypad_data->cols = pdata->num_col_gpios;
> > > +	keypad_data->keycodes = keypad_codes;
> > > +
> > > +	input_dev->name = pdev->name;
> > > +	input_dev->id.bustype = BUS_HOST;
> > > +	input_dev->dev.parent = &pdev->dev;
> > > +	input_dev->id.vendor = 0x0001;
> > > +	input_dev->id.product = 0x0001;
> > > +	input_dev->id.version = 0x0100;
> > > +
> > > +	input_dev->keycode	= keypad_codes;
> > > +	input_dev->keycodesize	= sizeof(*keypad_codes);
> >
> > You also need keycodemax so that keymap can be changed from userpsace.
 
Added!

> >
> > > +
> > > +	matrix_keypad_build_keymap(keymap_data, row_shift,
> > > +				   input_dev->keycode, input_dev->keybit);
> > > +
> > > +	__set_bit(EV_REP, input_dev->evbit);
> > > +	__set_bit(KEY_OK, input_dev->keybit);
> >
> > Why KEY_OK is set up separately?
 
In previous implementation manual set of bit was needed, now removed...
 
> >
> > > +	__set_bit(EV_KEY, input_dev->evbit);
> > > +
> > > +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> >
> > You forgot to actualy report MSC_SCAN though.
 
Should I remove it? I think I do need it only when KEY_UNKNOWN keys will be
remapped, right?
 
> >
> > > +	input_set_drvdata(input_dev, keypad_data);
> > > +
> > > +	status = input_register_device(keypad_data->input);
> > > +	if (status < 0) {
> > > +		dev_err(&pdev->dev, "failed to register input device\n");
> > > +		goto failed_free_device;
> > > +	}
> > > +
> > > +	omap_keypad_config(keypad_data);
> > > +
> > > +	platform_set_drvdata(pdev, keypad_data);
> > > +
> > > +	return 0;
> > > +
> > > +failed_free_device:
> > > +	input_unregister_device(keypad_data->input);
> >
> > 	input_dev = NULL;
> >
> > or you will try to free it later.
 
Not getting the idea here... could you explain?
 
> > > +failed_free_irq:
> > > +	free_irq(keypad_data->irq, pdev);
> > > +failed_free_base:
> > > +	keypad_data->base = NULL;
> > > +failed_free_input:
> > > +	input_free_device(input_dev);
> > > +failed_free_codes:
> > > +	kfree(keypad_codes);
> > > +failed_free_data:
> > > +	kfree(keypad_data);
> > > +failed_free_platform:
> > > +	kfree(keymap_data);
> > > +	kfree(pdata);
> > > +	return error;
> > > +}
> > > +
> > > +static int __devexit omap_keypad_remove(struct platform_device *pdev)
> > > +{
> > > +	struct omap_keypad *keypad_data = platform_get_drvdata(pdev);
> > > +
> > > +	input_unregister_device(keypad_data->input);
> > > +	free_irq(keypad_data->irq, keypad_data);
> >
> > Thsi should happen before unregistering input device, othersie there is
> > a change IRQ will arrive when input device is freed.
 
Done!
  
Best Regards
Abraham

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Dmitry Torokhov]

On Mon, May 10, 2010 at 11:17:50PM -0500, Arce, Abraham wrote:
> Sorry for the confusion in your name Dmitry...
> 

No worries, although at first I was surprised that Trilok spoke exactly
the same words I did ;)

> Thanks for your comments...
> 
> [snip]
> 
> > > > +
> > > > +/* Interrupt thread handler thread */
> > > > +
> > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > +{
> > >
> > > Why is iti threaded? I fo not see anything that will sleep.
> 
> 
> It was implemented based on previous comments...
> 

Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.

> 
> > > > +	struct omap_keypad *keypad_data = dev_id;
> > > > +	struct input_dev *input = keypad_data->input;
> > > > +	unsigned char key_state[8];
> > > > +	int col, row, code;
> > > > +	u32 *new_state = (u32 *) key_state;
> > > > +
> > > > +	*new_state	 = __raw_readl(keypad_data->base +
> > > > +			OMAP4_KBD_FULLCODE31_0);
> > > > +	*(new_state + 1) = __raw_readl(keypad_data->base +
> > > > +			OMAP4_KBD_FULLCODE63_32);
> > > > +
> > > > +	for (col = 0; col < keypad_data->cols; col++) {
> > > > +		for (row = 0; row < keypad_data->rows; row++) {
> > > > +			code = MATRIX_SCAN_CODE(row, col,
> > > > +					keypad_data->row_shift);
> > > > +			if (code < 0) {
> > > > +				printk(KERN_INFO "Spurious key %d-%d\n",
> > > > +						col, row);
> > > > +				continue;
> > > > +			}
> > >
> > > MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
> > > incorrect values in keymap...
> 
> Removed
> 
> > > > +			input_report_key(input, keypad_data->keycodes[code],
> > > > +					key_state[col] & (1 << row));
> > > > +		}
> > > > +	}
> > > > +
> > > > +	memcpy(keypad_data->key_state, &key_state,
> > > > +			sizeof(keypad_data->key_state));
> > > > +
> > > > +	/* enable interrupts */
> > > > +	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> > > > +			keypad_data->base + OMAP4_KBD_IRQENABLE);
> > > > +
> > > > +	/* clear pending interrupts */
> > > > +	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> > > > +			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> > > > +
> > > > +	/* clear pending interrupts */
> > > > +	return IRQ_HANDLED;
> > > > +
> > > > +}
> > > > +
> > > > +
> > > > +static int __devinit omap_keypad_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct omap_device *od;
> > > > +	struct omap_hwmod *oh;
> > > > +	struct matrix_keypad_platform_data *pdata;
> > > > +	struct input_dev *input_dev;
> > > > +	const struct matrix_keymap_data *keymap_data;
> > > > +	struct omap_keypad *keypad_data;
> > > > +
> > > > +	unsigned int status = 0, row_shift = 0, error = 0, length = 0, id = 0;
> > > > +	unsigned short *keypad_codes;
> > > > +
> > > > +	int hw_mod_name_len = 16;
> > > > +	char oh_name[hw_mod_name_len];
> > > > +	char *name = "omap4-keypad";
> > > > +
> > > > +	length = snprintf(oh_name, hw_mod_name_len, "keyboard");
> > > > +	WARN(length >= hw_mod_name_len,
> > > > +		"String buffer overflow in keyboard device setup\n");
> > > > +
> > > > +	oh = omap_hwmod_lookup(oh_name);
> > > > +	if (!oh)
> > > > +		pr_err("Could not look up %s\n", oh_name);
> > > > +
> > > > +	pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
> > > > +				GFP_KERNEL);
> > >
> > > Why is it allocated?
> 
> Moved to board file...
> 
> 
> > > > +
> > > > +	od = omap_device_build(name, id, oh, pdata,
> > > > +				sizeof(struct matrix_keypad_platform_data),
> > > > +				omap_keyboard_latency,
> > > > +				ARRAY_SIZE(omap_keyboard_latency), 1);
> > > > +	WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
> > > > +		name, oh_name);
> > > > +
> > > > +	/* platform data */
> > > > +	pdata = pdev->dev.platform_data;
> > >
> > > Umm, here you are overriding pdata pointer... don't you leak memory
> > > doing this?
> 
> 
> hwmod has been moved to board configuration so we need to get platform data
> 
> > > > +	if (!pdata) {
> > > > +		dev_err(&pdev->dev, "no platform data defined\n");
> > > > +		kfree(pdata);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	/* keymap data */
> > > > +	keymap_data = pdata->keymap_data;
> > > > +	if (!keymap_data) {
> > > > +		dev_err(&pdev->dev, "no keymap data defined\n");
> > > > +		kfree(keymap_data);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	/* omap keypad data */
> > > > +	row_shift = get_count_order(pdata->num_col_gpios);
> > > > +	keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
> > > > +	if (!keypad_data) {
> > > > +		error = -ENOMEM;
> > > > +		goto failed_free_platform;
> > > > +	}
> > > > +
> > > > +	/* omap keypad codes */
> > > > +	keypad_codes = kzalloc((pdata->num_row_gpios << row_shift) *
> > > > +			sizeof(*keypad_codes), GFP_KERNEL);
> > >
> > > Why is it al;loctaed separately? Can it be pickky-backed onto
> > > keypad_data?
>  
> Not getting the idea here... could you explain?

Do something like

struct omap_keypad {
	...
	unsigned short keymap[];
};

and then

	keypad_data = kzalloc(sizeof(struct omap_keypad) +
				(pdata->num_row_gpios << row_shift) *
				sizeof(unsigned short),
				GFP_KERNEL);

Or, depending on the max allowed keymap size, just embed array of
maximum possible size into struct omap_keypad..

>  
> > > > +	if (!keypad_codes) {
> > > > +		error = -ENOMEM;
> > > > +		goto failed_free_data;
> > > > +	}
> > > > +
> > > > +	/* input device allocation */
> > > > +	input_dev = input_allocate_device();
> > > > +	if (!input_dev) {
> > > > +		error = -ENOMEM;
> > > > +		goto failed_free_codes;
> > > > +	}
> > > > +
> > > > +	/* base resource */
> > > > +	keypad_data->base = oh->_rt_va;
> > > > +	if (!keypad_data->base) {
> > >
> > >
> > > You could probably check this earlier, before allocating stuff.
> 
> Early means before input_allocate_device?
> 

Yes. Check data before allocating resources.

> 
> > > > +		error = -ENOMEM;
> > > > +		goto failed_free_input;
> > > > +	}
> > > > +
> > > > +	/* irq */
> > > > +	keypad_data->irq = oh->mpu_irqs[0].irq;
> > > > +	if (keypad_data->irq < 0) {
> > >
> > > You could probably check this earlier, before allocating stuff.
> 
> Early means before input_allocate_device?
> 

Yes.

> > > > +		dev_err(&pdev->dev, "no keyboard irq assigned\n");
> > > > +		error = keypad_data->irq;
> > > > +		goto failed_free_base;
> > > > +	}
> > > > +
> > > > +	/* threaded irq init */
> > > > +	status = request_threaded_irq(keypad_data->irq, omap_keypad_interrupt,
> > > > +			omap_keypad_threaded,
> > > > +				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > > > +			"omap4-keypad", keypad_data);
> > > > +	if (status) {
> > > > +		dev_err(&pdev->dev, "failed to register interrupt\n");
> > > > +		error = -ENOMEM;
> > > > +		goto failed_free_irq;
> > > > +	}
> > > > +
> > > > +	keypad_data->input = input_dev;
> > > > +	keypad_data->row_shift = row_shift;
> > > > +	keypad_data->rows = pdata->num_row_gpios;
> > > > +	keypad_data->cols = pdata->num_col_gpios;
> > > > +	keypad_data->keycodes = keypad_codes;
> > > > +
> > > > +	input_dev->name = pdev->name;
> > > > +	input_dev->id.bustype = BUS_HOST;
> > > > +	input_dev->dev.parent = &pdev->dev;
> > > > +	input_dev->id.vendor = 0x0001;
> > > > +	input_dev->id.product = 0x0001;
> > > > +	input_dev->id.version = 0x0100;
> > > > +
> > > > +	input_dev->keycode	= keypad_codes;
> > > > +	input_dev->keycodesize	= sizeof(*keypad_codes);
> > >
> > > You also need keycodemax so that keymap can be changed from userpsace.
>  
> Added!
> 
> > >
> > > > +
> > > > +	matrix_keypad_build_keymap(keymap_data, row_shift,
> > > > +				   input_dev->keycode, input_dev->keybit);
> > > > +
> > > > +	__set_bit(EV_REP, input_dev->evbit);
> > > > +	__set_bit(KEY_OK, input_dev->keybit);
> > >
> > > Why KEY_OK is set up separately?
>  
> In previous implementation manual set of bit was needed, now removed...
>  
> > >
> > > > +	__set_bit(EV_KEY, input_dev->evbit);
> > > > +
> > > > +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> > >
> > > You forgot to actualy report MSC_SCAN though.
>  
> Should I remove it? I think I do need it only when KEY_UNKNOWN keys will be
> remapped, right?

I'd rather you actually report EV_MSC/MSC_SCAN.

>  
> > >
> > > > +	input_set_drvdata(input_dev, keypad_data);
> > > > +
> > > > +	status = input_register_device(keypad_data->input);
> > > > +	if (status < 0) {
> > > > +		dev_err(&pdev->dev, "failed to register input device\n");
> > > > +		goto failed_free_device;
> > > > +	}
> > > > +
> > > > +	omap_keypad_config(keypad_data);
> > > > +
> > > > +	platform_set_drvdata(pdev, keypad_data);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +failed_free_device:
> > > > +	input_unregister_device(keypad_data->input);
> > >
> > > 	input_dev = NULL;
> > >
> > > or you will try to free it later.
>  
> Not getting the idea here... could you explain?

It is not allowed to call input_free_device() after
input_unregister_device() due to the fact that input devices are
fercounted and you'd cause refcount underrun and potentially
double-free. However input_free_device() is happy to accept NULL as an
argument to it is a common way to handle the problem whike having
streamlined error handling path.

>  
> > > > +failed_free_irq:
> > > > +	free_irq(keypad_data->irq, pdev);
> > > > +failed_free_base:
> > > > +	keypad_data->base = NULL;
> > > > +failed_free_input:
> > > > +	input_free_device(input_dev);
> > > > +failed_free_codes:
> > > > +	kfree(keypad_codes);
> > > > +failed_free_data:
> > > > +	kfree(keypad_data);
> > > > +failed_free_platform:
> > > > +	kfree(keymap_data);
> > > > +	kfree(pdata);
> > > > +	return error;
> > > > +}
> > > > +
> > > > +static int __devexit omap_keypad_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct omap_keypad *keypad_data = platform_get_drvdata(pdev);
> > > > +
> > > > +	input_unregister_device(keypad_data->input);
> > > > +	free_irq(keypad_data->irq, keypad_data);
> > >
> > > Thsi should happen before unregistering input device, othersie there is
> > > a change IRQ will arrive when input device is freed.
>  
> Done!
>   

Thanks for making the changes.

-- 
Dmitry

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

Hi again Dmitry,

> No worries, although at first I was surprised that Trilok spoke exactly
> the same words I did ;)
> 

:)

> > > > > +
> > > > > +/* Interrupt thread handler thread */
> > > > > +
> > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > > +{
> > > >
> > > > Why is iti threaded? I fo not see anything that will sleep.
> >
> >
> > It was implemented based on previous comments...
> >
> 
> Would you point me to that comment? Like I said, I do not see anything
> that would possibly sleep in this routine so you don't need to use
> threaded interrupt.

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html

> 
> >
> > > > > +	struct omap_keypad *keypad_data = dev_id;
> > > > > +	struct input_dev *input = keypad_data->input;
> > > > > +	unsigned char key_state[8];
> > > > > +	int col, row, code;
> > > > > +	u32 *new_state = (u32 *) key_state;
> > > > > +
> > > > > +	*new_state	 = __raw_readl(keypad_data->base +
> > > > > +			OMAP4_KBD_FULLCODE31_0);
> > > > > +	*(new_state + 1) = __raw_readl(keypad_data->base +
> > > > > +			OMAP4_KBD_FULLCODE63_32);
> > > > > +
> > > > > +	for (col = 0; col < keypad_data->cols; col++) {
> > > > > +		for (row = 0; row < keypad_data->rows; row++) {
> > > > > +			code = MATRIX_SCAN_CODE(row, col,
> > > > > +					keypad_data->row_shift);
> > > > > +			if (code < 0) {
> > > > > +				printk(KERN_INFO "Spurious key %d-%d\n",
> > > > > +						col, row);
> > > > > +				continue;
> > > > > +			}
> > > >
> > > > MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
> > > > incorrect values in keymap...
> >
> > Removed
> >
> > > > > +			input_report_key(input, keypad_data->keycodes[code],
> > > > > +					key_state[col] & (1 << row));
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	memcpy(keypad_data->key_state, &key_state,
> > > > > +			sizeof(keypad_data->key_state));
> > > > > +
> > > > > +	/* enable interrupts */
> > > > > +	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN |
> OMAP4_DEF_IRQENABLE_LONGKEY,
> > > > > +			keypad_data->base + OMAP4_KBD_IRQENABLE);
> > > > > +
> > > > > +	/* clear pending interrupts */
> > > > > +	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> > > > > +			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> > > > > +
> > > > > +	/* clear pending interrupts */
> > > > > +	return IRQ_HANDLED;
> > > > > +
> > > > > +}
> > > > > +
> > > > > +
> > > > > +static int __devinit omap_keypad_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct omap_device *od;
> > > > > +	struct omap_hwmod *oh;
> > > > > +	struct matrix_keypad_platform_data *pdata;
> > > > > +	struct input_dev *input_dev;
> > > > > +	const struct matrix_keymap_data *keymap_data;
> > > > > +	struct omap_keypad *keypad_data;
> > > > > +
> > > > > +	unsigned int status = 0, row_shift = 0, error = 0, length = 0, id
> = 0;
> > > > > +	unsigned short *keypad_codes;
> > > > > +
> > > > > +	int hw_mod_name_len = 16;
> > > > > +	char oh_name[hw_mod_name_len];
> > > > > +	char *name = "omap4-keypad";
> > > > > +
> > > > > +	length = snprintf(oh_name, hw_mod_name_len, "keyboard");
> > > > > +	WARN(length >= hw_mod_name_len,
> > > > > +		"String buffer overflow in keyboard device setup\n");
> > > > > +
> > > > > +	oh = omap_hwmod_lookup(oh_name);
> > > > > +	if (!oh)
> > > > > +		pr_err("Could not look up %s\n", oh_name);
> > > > > +
> > > > > +	pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
> > > > > +				GFP_KERNEL);
> > > >
> > > > Why is it allocated?
> >
> > Moved to board file...
> >
> >
> > > > > +
> > > > > +	od = omap_device_build(name, id, oh, pdata,
> > > > > +				sizeof(struct matrix_keypad_platform_data),
> > > > > +				omap_keyboard_latency,
> > > > > +				ARRAY_SIZE(omap_keyboard_latency), 1);
> > > > > +	WARN(IS_ERR(od), "Could not build omap_device for %s %s\n",
> > > > > +		name, oh_name);
> > > > > +
> > > > > +	/* platform data */
> > > > > +	pdata = pdev->dev.platform_data;
> > > >
> > > > Umm, here you are overriding pdata pointer... don't you leak memory
> > > > doing this?
> >
> >
> > hwmod has been moved to board configuration so we need to get platform data
> >
> > > > > +	if (!pdata) {
> > > > > +		dev_err(&pdev->dev, "no platform data defined\n");
> > > > > +		kfree(pdata);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	/* keymap data */
> > > > > +	keymap_data = pdata->keymap_data;
> > > > > +	if (!keymap_data) {
> > > > > +		dev_err(&pdev->dev, "no keymap data defined\n");
> > > > > +		kfree(keymap_data);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	/* omap keypad data */
> > > > > +	row_shift = get_count_order(pdata->num_col_gpios);
> > > > > +	keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
> > > > > +	if (!keypad_data) {
> > > > > +		error = -ENOMEM;
> > > > > +		goto failed_free_platform;
> > > > > +	}
> > > > > +
> > > > > +	/* omap keypad codes */
> > > > > +	keypad_codes = kzalloc((pdata->num_row_gpios << row_shift) *
> > > > > +			sizeof(*keypad_codes), GFP_KERNEL);
> > > >
> > > > Why is it al;loctaed separately? Can it be pickky-backed onto
> > > > keypad_data?
> >
> > Not getting the idea here... could you explain?
> 
> Do something like
> 
> struct omap_keypad {
> 	...
> 	unsigned short keymap[];
> };
> 
> and then
> 
> 	keypad_data = kzalloc(sizeof(struct omap_keypad) +
> 				(pdata->num_row_gpios << row_shift) *
> 				sizeof(unsigned short),
> 				GFP_KERNEL);
> 
> Or, depending on the max allowed keymap size, just embed array of
> maximum possible size into struct omap_keypad..

I'll work on it... will send version 2 of driver

> >
> > > > > +	if (!keypad_codes) {
> > > > > +		error = -ENOMEM;
> > > > > +		goto failed_free_data;
> > > > > +	}
> > > > > +
> > > > > +	/* input device allocation */
> > > > > +	input_dev = input_allocate_device();
> > > > > +	if (!input_dev) {
> > > > > +		error = -ENOMEM;
> > > > > +		goto failed_free_codes;
> > > > > +	}
> > > > > +
> > > > > +	/* base resource */
> > > > > +	keypad_data->base = oh->_rt_va;
> > > > > +	if (!keypad_data->base) {
> > > >
> > > >
> > > > You could probably check this earlier, before allocating stuff.
> >
> > Early means before input_allocate_device?
> >
> 
> Yes. Check data before allocating resources.

I'll work on it... will send version 2 of driver

> 
> >
> > > > > +		error = -ENOMEM;
> > > > > +		goto failed_free_input;
> > > > > +	}
> > > > > +
> > > > > +	/* irq */
> > > > > +	keypad_data->irq = oh->mpu_irqs[0].irq;
> > > > > +	if (keypad_data->irq < 0) {
> > > >
> > > > You could probably check this earlier, before allocating stuff.
> >
> > Early means before input_allocate_device?
> >
> 
> Yes.

Thanks!

> 
> > > > > +		dev_err(&pdev->dev, "no keyboard irq assigned\n");
> > > > > +		error = keypad_data->irq;
> > > > > +		goto failed_free_base;
> > > > > +	}
> > > > > +
> > > > > +	/* threaded irq init */
> > > > > +	status = request_threaded_irq(keypad_data->irq,
> omap_keypad_interrupt,
> > > > > +			omap_keypad_threaded,
> > > > > +				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > > > > +			"omap4-keypad", keypad_data);
> > > > > +	if (status) {
> > > > > +		dev_err(&pdev->dev, "failed to register interrupt\n");
> > > > > +		error = -ENOMEM;
> > > > > +		goto failed_free_irq;
> > > > > +	}
> > > > > +
> > > > > +	keypad_data->input = input_dev;
> > > > > +	keypad_data->row_shift = row_shift;
> > > > > +	keypad_data->rows = pdata->num_row_gpios;
> > > > > +	keypad_data->cols = pdata->num_col_gpios;
> > > > > +	keypad_data->keycodes = keypad_codes;
> > > > > +
> > > > > +	input_dev->name = pdev->name;
> > > > > +	input_dev->id.bustype = BUS_HOST;
> > > > > +	input_dev->dev.parent = &pdev->dev;
> > > > > +	input_dev->id.vendor = 0x0001;
> > > > > +	input_dev->id.product = 0x0001;
> > > > > +	input_dev->id.version = 0x0100;
> > > > > +
> > > > > +	input_dev->keycode	= keypad_codes;
> > > > > +	input_dev->keycodesize	= sizeof(*keypad_codes);
> > > >
> > > > You also need keycodemax so that keymap can be changed from userpsace.
> >
> > Added!
> >
> > > >
> > > > > +
> > > > > +	matrix_keypad_build_keymap(keymap_data, row_shift,
> > > > > +				   input_dev->keycode, input_dev->keybit);
> > > > > +
> > > > > +	__set_bit(EV_REP, input_dev->evbit);
> > > > > +	__set_bit(KEY_OK, input_dev->keybit);
> > > >
> > > > Why KEY_OK is set up separately?
> >
> > In previous implementation manual set of bit was needed, now removed...
> >
> > > >
> > > > > +	__set_bit(EV_KEY, input_dev->evbit);
> > > > > +
> > > > > +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> > > >
> > > > You forgot to actualy report MSC_SCAN though.
> >
> > Should I remove it? I think I do need it only when KEY_UNKNOWN keys will be
> > remapped, right?
> 
> I'd rather you actually report EV_MSC/MSC_SCAN.

Will add...

> 
> >
> > > >
> > > > > +	input_set_drvdata(input_dev, keypad_data);
> > > > > +
> > > > > +	status = input_register_device(keypad_data->input);
> > > > > +	if (status < 0) {
> > > > > +		dev_err(&pdev->dev, "failed to register input device\n");
> > > > > +		goto failed_free_device;
> > > > > +	}
> > > > > +
> > > > > +	omap_keypad_config(keypad_data);
> > > > > +
> > > > > +	platform_set_drvdata(pdev, keypad_data);
> > > > > +
> > > > > +	return 0;
> > > > > +
> > > > > +failed_free_device:
> > > > > +	input_unregister_device(keypad_data->input);
> > > >
> > > > 	input_dev = NULL;
> > > >
> > > > or you will try to free it later.
> >
> > Not getting the idea here... could you explain?
> 
> It is not allowed to call input_free_device() after
> input_unregister_device() due to the fact that input devices are
> fercounted and you'd cause refcount underrun and potentially
> double-free. However input_free_device() is happy to accept NULL as an
> argument to it is a common way to handle the problem whike having
> streamlined error handling path.

Ok...

> 
> >
> > > > > +failed_free_irq:
> > > > > +	free_irq(keypad_data->irq, pdev);
> > > > > +failed_free_base:
> > > > > +	keypad_data->base = NULL;
> > > > > +failed_free_input:
> > > > > +	input_free_device(input_dev);
> > > > > +failed_free_codes:
> > > > > +	kfree(keypad_codes);
> > > > > +failed_free_data:
> > > > > +	kfree(keypad_data);
> > > > > +failed_free_platform:
> > > > > +	kfree(keymap_data);
> > > > > +	kfree(pdata);
> > > > > +	return error;
> > > > > +}
> > > > > +
> > > > > +static int __devexit omap_keypad_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct omap_keypad *keypad_data = platform_get_drvdata(pdev);
> > > > > +
> > > > > +	input_unregister_device(keypad_data->input);
> > > > > +	free_irq(keypad_data->irq, keypad_data);
> > > >
> > > > Thsi should happen before unregistering input device, othersie there is
> > > > a change IRQ will arrive when input device is freed.
> >
> > Done!
> >
> 
> Thanks for making the changes.

Will send version 2, thanks to you for the comments!

Best Regards
Abraham

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Dmitry Torokhov]

On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
> Hi again Dmitry,
> 
> > No worries, although at first I was surprised that Trilok spoke exactly
> > the same words I did ;)
> > 
> 
> :)
> 
> > > > > > +
> > > > > > +/* Interrupt thread handler thread */
> > > > > > +
> > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > > > +{
> > > > >
> > > > > Why is iti threaded? I fo not see anything that will sleep.
> > >
> > >
> > > It was implemented based on previous comments...
> > >
> > 
> > Would you point me to that comment? Like I said, I do not see anything
> > that would possibly sleep in this routine so you don't need to use
> > threaded interrupt.
> 
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
> 

Thanks.

I think Kevin meant "use theaded IRQ, wherever possible" [if we need
to sleep in interrupt handler].

Adding him to CC.

-- 
Dmitry

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Kevin Hilman]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
>> Hi again Dmitry,
>> 
>> > No worries, although at first I was surprised that Trilok spoke exactly
>> > the same words I did ;)
>> > 
>> 
>> :)
>> 
>> > > > > > +
>> > > > > > +/* Interrupt thread handler thread */
>> > > > > > +
>> > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
>> > > > > > +{
>> > > > >
>> > > > > Why is iti threaded? I fo not see anything that will sleep.
>> > >
>> > >
>> > > It was implemented based on previous comments...
>> > >
>> > 
>> > Would you point me to that comment? Like I said, I do not see anything
>> > that would possibly sleep in this routine so you don't need to use
>> > threaded interrupt.
>> 
>> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
>> 
>
> Thanks.
>
> I think Kevin meant "use theaded IRQ, wherever possible" [if we need
> to sleep in interrupt handler].

Actually, even interrupts that don't sleep can use threaded IRQs.  I
prefer to see threaded IRQs wherever possible.  Especially since we're
moving towards a world where all interrupts are run with interrupts
disabled, using threaded IRQs minimizes interrupts-off critical
sections.

Kevin

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Dmitry Torokhov]

On Tue, May 11, 2010 at 07:53:23AM -0700, Kevin Hilman wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> 
> > On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
> >> Hi again Dmitry,
> >> 
> >> > No worries, although at first I was surprised that Trilok spoke exactly
> >> > the same words I did ;)
> >> > 
> >> 
> >> :)
> >> 
> >> > > > > > +
> >> > > > > > +/* Interrupt thread handler thread */
> >> > > > > > +
> >> > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> >> > > > > > +{
> >> > > > >
> >> > > > > Why is iti threaded? I fo not see anything that will sleep.
> >> > >
> >> > >
> >> > > It was implemented based on previous comments...
> >> > >
> >> > 
> >> > Would you point me to that comment? Like I said, I do not see anything
> >> > that would possibly sleep in this routine so you don't need to use
> >> > threaded interrupt.
> >> 
> >> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
> >> 
> >
> > Thanks.
> >
> > I think Kevin meant "use theaded IRQ, wherever possible" [if we need
> > to sleep in interrupt handler].
> 
> Actually, even interrupts that don't sleep can use threaded IRQs.  I
> prefer to see threaded IRQs wherever possible.  Especially since we're
> moving towards a world where all interrupts are run with interrupts
> disabled, using threaded IRQs minimizes interrupts-off critical
> sections.
> 

I think in this case threaded IRQ would just add unnecessary overhead.
There are no scanning delays, just a few register reads and writes.
Input core will take some cycles propagating the events but it disables
interrupts anyway. Setting up a separate thread and scheduling does not
make much sense here.

Also I am not sure if arches with large number of interrupts would want
to move to all threaded interrupts model.

-- 
Dmitry

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Kevin Hilman]

Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:

> On Tue, May 11, 2010 at 07:53:23AM -0700, Kevin Hilman wrote:
>> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>> 
>> > On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
>> >> Hi again Dmitry,
>> >> 
>> >> > No worries, although at first I was surprised that Trilok spoke exactly
>> >> > the same words I did ;)
>> >> > 
>> >> 
>> >> :)
>> >> 
>> >> > > > > > +
>> >> > > > > > +/* Interrupt thread handler thread */
>> >> > > > > > +
>> >> > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
>> >> > > > > > +{
>> >> > > > >
>> >> > > > > Why is iti threaded? I fo not see anything that will sleep.
>> >> > >
>> >> > >
>> >> > > It was implemented based on previous comments...
>> >> > >
>> >> > 
>> >> > Would you point me to that comment? Like I said, I do not see anything
>> >> > that would possibly sleep in this routine so you don't need to use
>> >> > threaded interrupt.
>> >> 
>> >> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
>> >> 
>> >
>> > Thanks.
>> >
>> > I think Kevin meant "use theaded IRQ, wherever possible" [if we need
>> > to sleep in interrupt handler].
>> 
>> Actually, even interrupts that don't sleep can use threaded IRQs.  I
>> prefer to see threaded IRQs wherever possible.  Especially since we're
>> moving towards a world where all interrupts are run with interrupts
>> disabled, using threaded IRQs minimizes interrupts-off critical
>> sections.
>> 
>
> I think in this case threaded IRQ would just add unnecessary overhead.
> There are no scanning delays, just a few register reads and writes.
> Input core will take some cycles propagating the events but it disables
> interrupts anyway. Setting up a separate thread and scheduling does not
> make much sense here.
>
> Also I am not sure if arches with large number of interrupts would want
> to move to all threaded interrupts model.

OK, it's your call for this subsystem.

Kevin

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Shilimkar, Santosh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin
> Hilman
> Sent: Wednesday, May 12, 2010 3:10 AM
> To: Dmitry Torokhov
> Cc: Arce, Abraham; linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> 
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> 
> > On Tue, May 11, 2010 at 07:53:23AM -0700, Kevin Hilman wrote:
> >> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> >>
> >> > On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
> >> >> Hi again Dmitry,
> >> >>
> >> >> > No worries, although at first I was surprised that Trilok spoke exactly
> >> >> > the same words I did ;)
> >> >> >
> >> >>
> >> >> :)
> >> >>
> >> >> > > > > > +
> >> >> > > > > > +/* Interrupt thread handler thread */
> >> >> > > > > > +
> >> >> > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> >> >> > > > > > +{
> >> >> > > > >
> >> >> > > > > Why is iti threaded? I fo not see anything that will sleep.
> >> >> > >
> >> >> > >
> >> >> > > It was implemented based on previous comments...
> >> >> > >
> >> >> >
> >> >> > Would you point me to that comment? Like I said, I do not see anything
> >> >> > that would possibly sleep in this routine so you don't need to use
> >> >> > threaded interrupt.
> >> >>
> >> >> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
> >> >>
> >> >
> >> > Thanks.
> >> >
> >> > I think Kevin meant "use theaded IRQ, wherever possible" [if we need
> >> > to sleep in interrupt handler].
> >>
> >> Actually, even interrupts that don't sleep can use threaded IRQs.  I
> >> prefer to see threaded IRQs wherever possible.  Especially since we're
> >> moving towards a world where all interrupts are run with interrupts
> >> disabled, using threaded IRQs minimizes interrupts-off critical
> >> sections.
> >>
> >
> > I think in this case threaded IRQ would just add unnecessary overhead.
> > There are no scanning delays, just a few register reads and writes.
> > Input core will take some cycles propagating the events but it disables
> > interrupts anyway. Setting up a separate thread and scheduling does not
> > make much sense here.
> >
> > Also I am not sure if arches with large number of interrupts would want
> > to move to all threaded interrupts model.
> 
> OK, it's your call for this subsystem.
> 
Abraham initial driver has top half (ISR) and bottom half (tasklet). With Threaded 
IRQ, we can get rid-of tasklet and handle the bottom half in thread context.
IIRC this was on of the intention of this new API.

Isn't this not good enough to use the threaded IRQ in this case.

Regards,
Santosh

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

Dmitry,

2 comments + one question before sending next version...

[...]

> > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > > +{
> > > >
> > > > Why is iti threaded? I fo not see anything that will sleep.
> >
> >
> > It was implemented based on previous comments...
> >
> 
> Would you point me to that comment? Like I said, I do not see anything
> that would possibly sleep in this routine so you don't need to use
> threaded interrupt.

Using now request_irq based on your comments. In same omap_keypad_interrupt disable/clear/enable interrupts will be executed

[...]

> > > > > +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> > > >
> > > > You forgot to actualy report MSC_SCAN though.
> >
> > Should I remove it? I think I do need it only when KEY_UNKNOWN keys will be
> > remapped, right?
> 
> I'd rather you actually report EV_MSC/MSC_SCAN.

I am reporting now EV_MSC/MSC_SCAN. I am using evtest to read the events, if I press and release a key no code will be reported

  Event: time 946684836.854248, -------------- Report Sync ------------
  Event: time 946684836.854248, -------------- Report Sync ------------

Unless I keep it depressed

  Event: time 946684817.945892, type 1 (Key), code 18 (E), value 2

And after releasing the key some other events will get generated

  Event: time 946684817.967559, type 4 (Misc), code 4 (ScanCode), value 24
  Event: time 946684817.967559, -------------- Report Sync ------------
  Event: time 946684817.967559, type 4 (Misc), code 4 (ScanCode), value 32
  Event: time 946684817.967559, -------------- Report Sync ------------
  Event: time 946684817.967590, type 4 (Misc), code 4 (ScanCode), value 40
  Event: time 946684817.967590, -------------- Report Sync ------------

Am I missing something?

[...]

> 
> >
> > > > > +failed_free_irq:
> > > > > +	free_irq(keypad_data->irq, pdev);
> > > > > +failed_free_base:
> > > > > +	keypad_data->base = NULL;
> > > > > +failed_free_input:
> > > > > +	input_free_device(input_dev);
> > > > > +failed_free_codes:
> > > > > +	kfree(keypad_codes);
> > > > > +failed_free_data:
> > > > > +	kfree(keypad_data);
> > > > > +failed_free_platform:
> > > > > +	kfree(keymap_data);
> > > > > +	kfree(pdata);
> > > > > +	return error;

Using now the following sequence...

	pdata = pdev->dev.platform_data;
		return -EINVAL;

	keymap_data = pdata->keymap_data;
		goto failed_free_pdata;

	keypad_data = kzalloc(sizeof(struct omap_keypad) +
		goto failed_free_keymap;
	
	keypad_data->base = pdata->base;
		goto failed_free_keypad;

	keypad_data->irq = pdata->irq;
		goto failed_free_base;

	input_dev = input_allocate_device();
		goto failed_free_base;

	status = input_register_device(keypad_data->input);
		goto failed_free_input;

	status = request_irq(keypad_data->irq, omap_keypad_interrupt,
		goto failed_unregister_device;

failed_unregister__device:
	input_unregister_device(keypad_data->input);
	input_dev = NULL;
failed_free_input:
	input_free_device(input_dev);
failed_free_base:
	keypad_data->base = NULL;
failed_free_keypad:
	kfree(keypad_data);
failed_free_keymap:
	kfree(keymap_data);
failed_free_pdata:
	kfree(pdata);
	return error;

Best Regards
Abraham

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Shilimkar, Santosh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Arce,
> Abraham
> Sent: Wednesday, May 12, 2010 11:10 AM
> To: Dmitry Torokhov
> Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> 
> Dmitry,
> 
> 2 comments + one question before sending next version...
> 
> [...]
> 
> > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > > > +{
> > > > >
> > > > > Why is iti threaded? I fo not see anything that will sleep.
> > >
> > >
> > > It was implemented based on previous comments...
> > >
> >
> > Would you point me to that comment? Like I said, I do not see anything
> > that would possibly sleep in this routine so you don't need to use
> > threaded interrupt.
> 
> Using now request_irq based on your comments. In same omap_keypad_interrupt disable/clear/enable
> interrupts will be executed
> 
> [...]
>
Sorry for jumping into the comments late. Thought this was sorted out. Key scanning
and debounce timeouts etc still there. Having all these things in ISR itself isn't good
idea.

Dmitry,
Don't you think its optimal to push the key-scanning and debounce timeout code
part of bottom half ??

Regards,
Santosh

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Dmitry Torokhov]

On Wed, May 12, 2010 at 11:15:11AM +0530, Shilimkar, Santosh wrote:
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Arce,
> > Abraham
> > Sent: Wednesday, May 12, 2010 11:10 AM
> > To: Dmitry Torokhov
> > Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> > Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> > 
> > Dmitry,
> > 
> > 2 comments + one question before sending next version...
> > 
> > [...]
> > 
> > > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > > > > +{
> > > > > >
> > > > > > Why is iti threaded? I fo not see anything that will sleep.
> > > >
> > > >
> > > > It was implemented based on previous comments...
> > > >
> > >
> > > Would you point me to that comment? Like I said, I do not see anything
> > > that would possibly sleep in this routine so you don't need to use
> > > threaded interrupt.
> > 
> > Using now request_irq based on your comments. In same omap_keypad_interrupt disable/clear/enable
> > interrupts will be executed
> > 
> > [...]
> >
> Sorry for jumping into the comments late. Thought this was sorted out. Key scanning
> and debounce timeouts etc still there. Having all these things in ISR itself isn't good
> idea.
> 
> Dmitry,
> Don't you think its optimal to push the key-scanning and debounce timeout code
> part of bottom half ??
>

If you need debounce then you need to fire a timer and keep doing this
until interrupt (or key state) settles. It really depends on the device.

-- 
Dmitry

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Shilimkar, Santosh]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Wednesday, May 12, 2010 11:33 AM
> To: Shilimkar, Santosh
> Cc: Arce, Abraham; linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> 
> On Wed, May 12, 2010 at 11:15:11AM +0530, Shilimkar, Santosh wrote:
> > > -----Original Message-----
> > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
> Arce,
> > > Abraham
> > > Sent: Wednesday, May 12, 2010 11:10 AM
> > > To: Dmitry Torokhov
> > > Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> > > Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> > >
> > > Dmitry,
> > >
> > > 2 comments + one question before sending next version...
> > >
> > > [...]
> > >
> > > > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > > > > > +{
> > > > > > >
> > > > > > > Why is iti threaded? I fo not see anything that will sleep.
> > > > >
> > > > >
> > > > > It was implemented based on previous comments...
> > > > >
> > > >
> > > > Would you point me to that comment? Like I said, I do not see anything
> > > > that would possibly sleep in this routine so you don't need to use
> > > > threaded interrupt.
> > >
> > > Using now request_irq based on your comments. In same omap_keypad_interrupt disable/clear/enable
> > > interrupts will be executed
> > >
> > > [...]
> > >
> > Sorry for jumping into the comments late. Thought this was sorted out. Key scanning
> > and debounce timeouts etc still there. Having all these things in ISR itself isn't good
> > idea.
> >
> > Dmitry,
> > Don't you think its optimal to push the key-scanning and debounce timeout code
> > part of bottom half ??
> >
> 
> If you need debounce then you need to fire a timer and keep doing this
> until interrupt (or key state) settles. It really depends on the device.
> 
The OMAP4 keypad controller has internal timeout mechanism and doesn't need any
external timer for this.

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Arce, Abraham]

> > Sorry for jumping into the comments late. Thought this was sorted out. Key
> scanning
> > and debounce timeouts etc still there. Having all these things in ISR itself
> isn't good
> > idea.
> >
> > Dmitry,
> > Don't you think its optimal to push the key-scanning and debounce timeout
> code
> > part of bottom half ??
> >
> 
> If you need debounce then you need to fire a timer and keep doing this
> until interrupt (or key state) settles. It really depends on the device.

Controller includes a debouncing feature... 

Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Dmitry Torokhov]

On Wed, May 12, 2010 at 11:49:45AM +0530, Shilimkar, Santosh wrote:
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Wednesday, May 12, 2010 11:33 AM
> > To: Shilimkar, Santosh
> > Cc: Arce, Abraham; linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> > Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> > 
> > On Wed, May 12, 2010 at 11:15:11AM +0530, Shilimkar, Santosh wrote:
> > > > -----Original Message-----
> > > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
> > Arce,
> > > > Abraham
> > > > Sent: Wednesday, May 12, 2010 11:10 AM
> > > > To: Dmitry Torokhov
> > > > Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> > > > Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> > > >
> > > > Dmitry,
> > > >
> > > > 2 comments + one question before sending next version...
> > > >
> > > > [...]
> > > >
> > > > > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > > > > > > +{
> > > > > > > >
> > > > > > > > Why is iti threaded? I fo not see anything that will sleep.
> > > > > >
> > > > > >
> > > > > > It was implemented based on previous comments...
> > > > > >
> > > > >
> > > > > Would you point me to that comment? Like I said, I do not see anything
> > > > > that would possibly sleep in this routine so you don't need to use
> > > > > threaded interrupt.
> > > >
> > > > Using now request_irq based on your comments. In same omap_keypad_interrupt disable/clear/enable
> > > > interrupts will be executed
> > > >
> > > > [...]
> > > >
> > > Sorry for jumping into the comments late. Thought this was sorted out. Key scanning
> > > and debounce timeouts etc still there. Having all these things in ISR itself isn't good
> > > idea.
> > >
> > > Dmitry,
> > > Don't you think its optimal to push the key-scanning and debounce timeout code
> > > part of bottom half ??
> > >
> > 
> > If you need debounce then you need to fire a timer and keep doing this
> > until interrupt (or key state) settles. It really depends on the device.
> > 
> The OMAP4 keypad controller has internal timeout mechanism and doesn't need any
> external timer for this.
>

Then I do not understand the question... If hardware takes care of
debouncing then just read the state and report the data.

-- 
Dmitry

RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

[Shilimkar, Santosh]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Wednesday, May 12, 2010 12:05 PM
> To: Shilimkar, Santosh
> Cc: Arce, Abraham; linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> 
> On Wed, May 12, 2010 at 11:49:45AM +0530, Shilimkar, Santosh wrote:
> > > -----Original Message-----
> > > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > > Sent: Wednesday, May 12, 2010 11:33 AM
> > > To: Shilimkar, Santosh
> > > Cc: Arce, Abraham; linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> > > Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> > >
> > > On Wed, May 12, 2010 at 11:15:11AM +0530, Shilimkar, Santosh wrote:
> > > > > -----Original Message-----
> > > > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
> > > Arce,
> > > > > Abraham
> > > > > Sent: Wednesday, May 12, 2010 11:10 AM
> > > > > To: Dmitry Torokhov
> > > > > Cc: linux-input@vger.kernel.org; linux-omap@vger.kernel.org
> > > > > Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
> > > > >
> > > > > Dmitry,
> > > > >
> > > > > 2 comments + one question before sending next version...
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
> > > > > > > > > > +{
> > > > > > > > >
> > > > > > > > > Why is iti threaded? I fo not see anything that will sleep.
> > > > > > >
> > > > > > >
> > > > > > > It was implemented based on previous comments...
> > > > > > >
> > > > > >
> > > > > > Would you point me to that comment? Like I said, I do not see anything
> > > > > > that would possibly sleep in this routine so you don't need to use
> > > > > > threaded interrupt.
> > > > >
> > > > > Using now request_irq based on your comments. In same omap_keypad_interrupt
> disable/clear/enable
> > > > > interrupts will be executed
> > > > >
> > > > > [...]
> > > > >
> > > > Sorry for jumping into the comments late. Thought this was sorted out. Key scanning
> > > > and debounce timeouts etc still there. Having all these things in ISR itself isn't good
> > > > idea.
> > > >
> > > > Dmitry,
> > > > Don't you think its optimal to push the key-scanning and debounce timeout code
> > > > part of bottom half ??
> > > >
> > >
> > > If you need debounce then you need to fire a timer and keep doing this
> > > until interrupt (or key state) settles. It really depends on the device.
> > >
> > The OMAP4 keypad controller has internal timeout mechanism and doesn't need any
> > external timer for this.
> >
> 
> Then I do not understand the question... If hardware takes care of
> debouncing then just read the state and report the data.
> 
You have point. Debounce shouldn't be an issue. If the key scan isn't taking
Much time then I agree with your suggestion.

Abraham, Can you please how much time you are spending in the key scan loops ?

Regards,
Santosh