Re: [PATCH] input: add support for generic GPIO-based matrix keypad

On Sunday 07 June 2009 07:04:09 Eric Miao wrote:

On Wed, Jun 3, 2009 at 2:24 PM, Eric Miao[off-list ref] wrote:

quoted

On Tue, Jun 2, 2009 at 11:14 PM, Uli Luckas [off-list ref] wrote:

quoted

One more...

As all gpio handling is done in a workqueue,
gpio_set_value_cansleep/gpio_get_value_cansleep could probably be used.
This would make the driver ready for gpios on external controllers (via
i2c for example).

Sounds good, patch updated again, with attachment.

Hi Dmitry,

I'd like to know if this patch is able to get into the boat of the
next merge window, I have several patches depending on
this, and it seems that Trilok has something as well. I know
it might take some time to review all this, but let me know
if there are anything I can help.

Eric,

I tried to merge your latest patch with some work I have done earlier,
the result is below. Please give it a try and if it still works then
I will queue it for pull.

The most important change IMHO is the way IRQs are enabled and disabled.
We cannot simply do that without any locking because (on SMP) more than
one interrupt handler may be executing at the same time causing interrupts
being disabled several times. Same goes for enabling interrupts at the end
of the scan - once first is enabled we have a chance of getting it raised
right away and then we have 2 pieces racing against each other, one
enabling and another disabling interrupts. Also care is needed to flush
work properly at remove time. Please check out the logic for holes in case
I missed something.

Other changes:
- Make key table definition useable to other drivers;
- Allow changing keymap from userspace
- open/close
- various cleanups.

Thanks!

--
Dmitry

Input: add support for generic GPIO-based matrix keypad

From: Eric Miao <redacted>

Original patch by Marek Vasut, modified by Eric in:

1. use delayed work to simplify the debouncing
2. combine col_polarity/row_polarity into a single active_low field
3. use a generic bit array based XOR algorithm to detect key
   press/release, which should make the column assertion time
   shorter and code a bit cleaner
4. remove the ALT_FN handling, which is no way generic, the ALT_FN
   key should be treated as no different from other keys, and
   translation will be done by user space by commands like 'loadkeys'.

[dtor@mail.ru: fix error unwinding path, support changing keymap
 from userspace]
Signed-off-by: Marek Vasut <redacted>
Signed-off-by: Eric Miao <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

 drivers/input/keyboard/Kconfig         |   13 +
 drivers/input/keyboard/Makefile        |    1 
 drivers/input/keyboard/matrix_keypad.c |  453 ++++++++++++++++++++++++++++++++
 include/linux/input/matrix_keypad.h    |   65 +++++
 4 files changed, 530 insertions(+), 2 deletions(-)
 create mode 100644 drivers/input/keyboard/matrix_keypad.c
 create mode 100644 include/linux/input/matrix_keypad.h

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index d2df103..a6b989a 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig

@@ -158,7 +158,16 @@ config KEYBOARD_GPIO
 	  with configuration data saying which GPIOs are used.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called gpio-keys.
+	  module will be called gpio_keys.
+
+config KEYBOARD_MATRIX
+	tristate "GPIO driven matrix keypad support"
+	depends on GENERIC_GPIO
+	help
+	  Enable support for GPIO driven matrix keypad.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called matrix_keypad.
 
 config KEYBOARD_HIL_OLD
 	tristate "HP HIL keyboard support (simple driver)"

@@ -254,7 +263,7 @@ config KEYBOARD_PXA27x
 	tristate "PXA27x/PXA3xx keypad support"
 	depends on PXA27x || PXA3xx
 	help
-	  Enable support for PXA27x/PXA3xx keypad controller
+	  Enable support for PXA27x/PXA3xx keypad controller.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called pxa27x_keypad.

diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 632efbc..b5b5eae 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile

@@ -20,6 +20,7 @@ obj-$(CONFIG_KEYBOARD_LKKBD)		+= lkkbd.o
 obj-$(CONFIG_KEYBOARD_LM8323)		+= lm8323.o
 obj-$(CONFIG_KEYBOARD_LOCOMO)		+= locomokbd.o
 obj-$(CONFIG_KEYBOARD_MAPLE)		+= maple_keyb.o
+obj-$(CONFIG_KEYBOARD_MATRIX)		+= matrix_keypad.o
 obj-$(CONFIG_KEYBOARD_NEWTON)		+= newtonkbd.o
 obj-$(CONFIG_KEYBOARD_OMAP)		+= omap-keypad.o
 obj-$(CONFIG_KEYBOARD_PXA27x)		+= pxa27x_keypad.o

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
new file mode 100644
index 0000000..4040cf0
--- /dev/null
+++ b/drivers/input/keyboard/matrix_keypad.c

@@ -0,0 +1,453 @@
+/*
+ *  GPIO driven matrix keyboard driver
+ *
+ *  Copyright (c) 2008 Marek Vasut <marek.vasut@gmail.com>
+ *
+ *  Based on corgikbd.c
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/gpio.h>
+#include <linux/input/matrix_keypad.h>
+
+struct matrix_keypad {
+	const struct matrix_keypad_platform_data *pdata;
+	struct input_dev *input_dev;
+	unsigned short *keycodes;
+
+	uint32_t last_key_state[MATRIX_MAX_COLS];
+	struct delayed_work work;
+	bool scan_pending;
+	bool stopped;
+	spinlock_t lock;
+};
+
+/*
+ * NOTE: normally the GPIO has to be put into HiZ when de-activated to cause
+ * minmal side effect when scanning other columns, here it is configured to
+ * be input, and it should work on most platforms.
+ */
+static void __activate_col(const struct matrix_keypad_platform_data *pdata,
+			   int col, bool on)
+{
+	bool level_on = !pdata->active_low;
+
+	if (on) {
+		gpio_direction_output(pdata->col_gpios[col], level_on);
+	} else {
+		gpio_set_value_cansleep(pdata->col_gpios[col], !level_on);
+		gpio_direction_input(pdata->col_gpios[col]);
+	}
+}
+
+static void activate_col(const struct matrix_keypad_platform_data *pdata,
+			 int col, bool on)
+{
+	__activate_col(pdata, col, on);
+
+	if (on && pdata->col_scan_delay_us)
+		udelay(pdata->col_scan_delay_us);
+}
+
+static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
+			      bool on)
+{
+	int col;
+
+	for (col = 0; col < pdata->num_col_gpios; col++)
+		__activate_col(pdata, col, on);
+}
+
+static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
+			 int row)
+{
+	return gpio_get_value_cansleep(pdata->row_gpios[row]) ?
+			!pdata->active_low : pdata->active_low;
+}
+
+static void enable_row_irqs(struct matrix_keypad *keypad)
+{
+	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
+	int i;
+
+	for (i = 0; i < pdata->num_row_gpios; i++)
+		enable_irq(gpio_to_irq(pdata->row_gpios[i]));
+}
+
+static void disable_row_irqs(struct matrix_keypad *keypad)
+{
+	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
+	int i;
+
+	for (i = 0; i < pdata->num_row_gpios; i++)
+		disable_irq_nosync(gpio_to_irq(pdata->row_gpios[i]));
+}
+
+/*
+ * This gets the keys from keyboard and reports it to input subsystem
+ */
+static void matrix_keypad_scan(struct work_struct *work)
+{
+	struct matrix_keypad *keypad =
+		container_of(work, struct matrix_keypad, work.work);
+	struct input_dev *input_dev = keypad->input_dev;
+	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
+	uint32_t new_state[MATRIX_MAX_COLS];
+	int row, col, code;
+
+	/* de-activate all columns for scanning */
+	activate_all_cols(pdata, false);
+
+	memset(new_state, 0, sizeof(new_state));
+
+	/* assert each column and read the row status out */
+	for (col = 0; col < pdata->num_col_gpios; col++) {
+
+		activate_col(pdata, col, true);
+
+		for (row = 0; row < pdata->num_row_gpios; row++)
+			new_state[col] |=
+				row_asserted(pdata, row) ? (1 << row) : 0;
+
+		activate_col(pdata, col, false);
+	}
+
+	for (col = 0; col < pdata->num_col_gpios; col++) {
+		uint32_t bits_changed;
+
+		bits_changed = keypad->last_key_state[col] ^ new_state[col];
+		if (bits_changed == 0)
+			continue;
+
+		for (row = 0; row < pdata->num_row_gpios; row++) {
+			if ((bits_changed & (1 << row)) == 0)
+				continue;
+
+			code = (row << 4) + col;
+			input_event(input_dev, EV_MSC, MSC_SCAN, code);
+			input_report_key(input_dev,
+					 keypad->keycodes[code],
+					 new_state[col] & (1 << row));
+		}
+	}
+	input_sync(input_dev);
+
+	memcpy(keypad->last_key_state, new_state, sizeof(new_state));
+
+	activate_all_cols(pdata, true);
+
+	/* Enable IRQs again */
+	spin_lock_irq(&keypad->lock);
+	keypad->scan_pending = false;
+	enable_row_irqs(keypad);
+	spin_unlock_irq(&keypad->lock);
+}
+
+static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
+{
+	struct matrix_keypad *keypad = id;
+	unsigned long flags;
+
+	spin_lock_irqsave(&keypad->lock, flags);
+
+	/*
+	 * See if another IRQ beaten us to it and scheduled the
+	 * scan already. In that case we should not try to
+	 * disable IRQs again.
+	 */
+	if (unlikely(keypad->scan_pending || keypad->stopped))
+		goto out;
+
+	disable_row_irqs(keypad);
+	keypad->scan_pending = true;
+	schedule_delayed_work(&keypad->work,
+		msecs_to_jiffies(keypad->pdata->debounce_ms));
+
+out:
+	spin_unlock_irqrestore(&keypad->lock, flags);
+	return IRQ_HANDLED;
+}
+
+static int matrix_keypad_start(struct input_dev *dev)
+{
+	struct matrix_keypad *keypad = input_get_drvdata(dev);
+
+	keypad->stopped = false;
+	mb();
+
+	/*
+	 * Schedule an immediate key scan to capture current key state;
+	 * columns will be activated and IRQs be enabled after the scan.
+	 */
+	schedule_delayed_work(&keypad->work, 0);
+
+	return 0;
+}
+
+static void matrix_keypad_stop(struct input_dev *dev)
+{
+	struct matrix_keypad *keypad = input_get_drvdata(dev);
+
+	keypad->stopped = true;
+	mb();
+	flush_work(&keypad->work.work);
+	/*
+	 * matrix_keypad_scan() will leave IRQs enabled;
+	 * we should disable them now.
+	 */
+	disable_row_irqs(keypad);
+}
+
+#ifdef CONFIG_PM
+static int matrix_keypad_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct matrix_keypad *keypad = platform_get_drvdata(pdev);
+	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
+	int i;
+
+	matrix_keypad_stop(keypad->input_dev);
+
+	if (device_may_wakeup(&pdev->dev))
+		for (i = 0; i < pdata->num_row_gpios; i++)
+			enable_irq_wake(gpio_to_irq(pdata->row_gpios[i]));
+
+	return 0;
+}
+
+static int matrix_keypad_resume(struct platform_device *pdev)
+{
+	struct matrix_keypad *keypad = platform_get_drvdata(pdev);
+	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
+	int i;
+
+	if (device_may_wakeup(&pdev->dev))
+		for (i = 0; i < pdata->num_row_gpios; i++)
+			disable_irq_wake(gpio_to_irq(pdata->row_gpios[i]));
+
+	matrix_keypad_start(keypad->input_dev);
+
+	return 0;
+}
+#else
+#define matrix_keypad_suspend	NULL
+#define matrix_keypad_resume	NULL
+#endif
+
+static int __devinit init_matrix_gpio(struct platform_device *pdev,
+					struct matrix_keypad *keypad)
+{
+	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
+	int i, err = -EINVAL;
+
+	/* initialized strobe lines as outputs, activated */
+	for (i = 0; i < pdata->num_col_gpios; i++) {
+		err = gpio_request(pdata->col_gpios[i], "matrix_kbd_col");
+		if (err) {
+			dev_err(&pdev->dev,
+				"failed to request GPIO%d for COL%d\n",
+				pdata->col_gpios[i], i);
+			goto err_free_cols;
+		}
+
+		gpio_direction_output(pdata->col_gpios[i], !pdata->active_low);
+	}
+
+	for (i = 0; i < pdata->num_row_gpios; i++) {
+		err = gpio_request(pdata->row_gpios[i], "matrix_kbd_row");
+		if (err) {
+			dev_err(&pdev->dev,
+				"failed to request GPIO%d for ROW%d\n",
+				pdata->row_gpios[i], i);
+			goto err_free_rows;
+		}
+
+		gpio_direction_input(pdata->row_gpios[i]);
+	}
+
+	for (i = 0; i < pdata->num_row_gpios; i++) {
+		err = request_irq(gpio_to_irq(pdata->row_gpios[i]),
+				matrix_keypad_interrupt,
+				IRQF_DISABLED |
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				"matrix-keypad", keypad);
+		if (err) {
+			dev_err(&pdev->dev,
+				"Unable to acquire interrupt for GPIO line %i\n",
+				pdata->row_gpios[i]);
+			goto err_free_irqs;
+		}
+	}
+	return 0;
+
+err_free_irqs:
+	while (--i >= 0)
+		free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad);
+	i = pdata->num_row_gpios;
+err_free_rows:
+	while (--i >= 0)
+		gpio_free(pdata->row_gpios[i]);
+	i = pdata->num_col_gpios;
+err_free_cols:
+	while (--i >= 0)
+		gpio_free(pdata->col_gpios[i]);
+
+	return err;
+}
+
+static int __devinit matrix_keypad_probe(struct platform_device *pdev)
+{
+	const struct matrix_keypad_platform_data *pdata;
+	const struct matrix_keymap_data *keymap_data;
+	struct matrix_keypad *keypad;
+	struct input_dev *input_dev;
+	unsigned short *keycodes;
+	int i;
+	int err;
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "no platform data defined\n");
+		return -EINVAL;
+	}
+
+	keymap_data = pdata->keymap_data;
+	if (!keymap_data) {
+		dev_err(&pdev->dev, "no keymap data defined\n");
+		return -EINVAL;
+	}
+
+	if (!keymap_data->max_keymap_size) {
+		dev_err(&pdev->dev, "invalid keymap data supplied\n");
+		return -EINVAL;
+	}
+
+	keypad = kzalloc(sizeof(struct matrix_keypad), GFP_KERNEL);
+	keycodes = kzalloc(keymap_data->max_keymap_size *
+				sizeof(keypad->keycodes),
+			   GFP_KERNEL);
+	input_dev = input_allocate_device();
+	if (!keypad || !keycodes || !input_dev) {
+		err = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	keypad->input_dev = input_dev;
+	keypad->pdata = pdata;
+	keypad->keycodes = keycodes;
+	keypad->stopped = true;
+	INIT_DELAYED_WORK(&keypad->work, matrix_keypad_scan);
+	spin_lock_init(&keypad->lock);
+
+	input_dev->name		= pdev->name;
+	input_dev->id.bustype	= BUS_HOST;
+	input_dev->dev.parent	= &pdev->dev;
+	input_dev->evbit[0]	= BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
+	input_dev->open		= matrix_keypad_start;
+	input_dev->close	= matrix_keypad_stop;
+
+	input_dev->keycode	= keycodes;
+	input_dev->keycodesize	= sizeof(*keycodes);
+	input_dev->keycodemax	= keymap_data->max_keymap_size;
+
+	for (i = 0; i < keymap_data->keymap_size; i++) {
+		unsigned int key = keymap_data->keymap[i];
+		unsigned int row = KEY_ROW(key);
+		unsigned int col = KEY_COL(key);
+		unsigned short code = KEY_VAL(key);
+
+		keycodes[(row << 4) + col] = code;
+		__set_bit(code, input_dev->keybit);
+	}
+	__clear_bit(KEY_RESERVED, input_dev->keybit);
+
+	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+	input_set_drvdata(input_dev, keypad);
+
+	err = input_register_device(keypad->input_dev);
+	if (err)
+		goto err_free_mem;
+
+	err = init_matrix_gpio(pdev, keypad);
+	if (err)
+		goto err_unregister;
+
+	device_init_wakeup(&pdev->dev, pdata->wakeup);
+	platform_set_drvdata(pdev, keypad);
+
+	return 0;
+
+err_unregister:
+	input_unregister_device(input_dev);
+	input_dev = NULL;
+err_free_mem:
+	input_free_device(input_dev);
+	kfree(keycodes);
+	kfree(keypad);
+	return err;
+}
+
+static int __devexit matrix_keypad_remove(struct platform_device *pdev)
+{
+	struct matrix_keypad *keypad = platform_get_drvdata(pdev);
+	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
+	int i;
+
+	device_init_wakeup(&pdev->dev, 0);
+
+	for (i = 0; i < pdata->num_row_gpios; i++) {
+		free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad);
+		gpio_free(pdata->row_gpios[i]);
+	}
+
+	for (i = 0; i < pdata->num_col_gpios; i++)
+		gpio_free(pdata->col_gpios[i]);
+
+	input_unregister_device(keypad->input_dev);
+	platform_set_drvdata(pdev, NULL);
+	kfree(keypad->keycodes);
+	kfree(keypad);
+
+	return 0;
+}
+
+static struct platform_driver matrix_keypad_driver = {
+	.probe		= matrix_keypad_probe,
+	.remove		= __devexit_p(matrix_keypad_remove),
+	.suspend	= matrix_keypad_suspend,
+	.resume		= matrix_keypad_resume,
+	.driver		= {
+		.name	= "matrix-keypad",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init matrix_keypad_init(void)
+{
+	return platform_driver_register(&matrix_keypad_driver);
+}
+
+static void __exit matrix_keypad_exit(void)
+{
+	platform_driver_unregister(&matrix_keypad_driver);
+}
+
+module_init(matrix_keypad_init);
+module_exit(matrix_keypad_exit);
+
+MODULE_AUTHOR("Marek Vasut <marek.vasut@gmail.com>");
+MODULE_DESCRIPTION("GPIO Driven Matrix Keypad Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:matrix-keypad");

diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
new file mode 100644
index 0000000..f945402
--- /dev/null
+++ b/include/linux/input/matrix_keypad.h

@@ -0,0 +1,65 @@
+#ifndef _MATRIX_KEYPAD_H
+#define _MATRIX_KEYPAD_H
+
+#include <linux/types.h>
+#include <linux/input.h>
+
+#define MATRIX_MAX_ROWS		16
+#define MATRIX_MAX_COLS		16
+
+#define KEY(row, col, val)	((((row) & (MATRIX_MAX_ROWS - 1)) << 24) |\
+				 (((col) & (MATRIX_MAX_COLS - 1)) << 16) |\
+				 (val & 0xffff))
+
+#define KEY_ROW(k)		(((k) >> 24) & 0xff)
+#define KEY_COL(k)		(((k) >> 16) & 0xff)
+#define KEY_VAL(k)		((k) & 0xffff)
+
+/**
+ * struct matrix_keymap_data - keymap for matrix keyboards
+ * @keymap: pointer to array of uint32 values encoded with KEY() macro
+ * 	representing keymap
+ * @keymap_size: number of entries (initialized) in this keymap
+ * @max_keymap_size: maximum size of keymap supported by the device
+ *
+ * This structure is supposed to be used by platform code to supply
+ * keymaps to drivers that implement matrix-like keypads/keyboards.
+ */
+struct matrix_keymap_data {
+	const uint32_t *keymap;
+	unsigned int	keymap_size;
+	unsigned int	max_keymap_size;
+};
+
+/**
+ * struct matrix_keypad_platform_data - platform-dependent keypad data
+ * @keymap_data: pointer to &matrix_keymap_data
+ * @row_gpios: array of gpio numbers reporesenting rows
+ * @col_gpios: array of gpio numbers reporesenting colums
+ * @num_row_gpios: actual number of row gpios used by device
+ * @num_col_gpios: actual number of col gpios used by device
+ * @col_scan_delay_us: delay, measured in microseconds, that is
+ * 	needed before we can keypad after activating column gpio
+ * @debounce_ms: debounce interval in milliseconds
+ *
+ * This structure represents platform-specific data that use used by
+ * matrix_keypad driver to perform proper initialization.
+ */
+struct matrix_keypad_platform_data {
+	const struct matrix_keymap_data *keymap_data;
+
+	unsigned int	row_gpios[MATRIX_MAX_ROWS];
+	unsigned int	col_gpios[MATRIX_MAX_COLS];
+	unsigned int	num_row_gpios;
+	unsigned int	num_col_gpios;
+
+	unsigned int	col_scan_delay_us;
+
+	/* key debounce interval in milli-second */
+	unsigned int	debounce_ms;
+
+	bool		active_low;
+	bool		wakeup;
+};
+
+#endif /* _MATRIX_KEYPAD_H */

Hi Dmitry,

Input: add support for generic GPIO-based matrix keypad

From: Eric Miao <redacted>

Original patch by Marek Vasut, modified by Eric in:

1. use delayed work to simplify the debouncing
2. combine col_polarity/row_polarity into a single active_low field
3. use a generic bit array based XOR algorithm to detect key
  press/release, which should make the column assertion time
  shorter and code a bit cleaner
4. remove the ALT_FN handling, which is no way generic, the ALT_FN
  key should be treated as no different from other keys, and
  translation will be done by user space by commands like 'loadkeys'.

[dtor@mail.ru: fix error unwinding path, support changing keymap
 from userspace]
Signed-off-by: Marek Vasut <redacted>
Signed-off-by: Eric Miao <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

Did you took latest patch submitted from Eric? Because it had more
signed-off and acked-by lines, like this.

Signed-off-by: Marek Vasut <redacted>
Reviewed-by: Trilok Soni <redacted>
Reviewed-by: Uli Luckas <redacted>
Reviewed-by: Russell King <redacted>
Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Eric Miao <redacted>

http://markmail.org/message/2wrr2b6mr6qsd4xs#query:+page:1+mid:fkkfxlumfm4mjhk4+state:results

Eric can confirm otherwise.

Why this change in this patch?

+
+                       code = (row << 4) + col;

<< 4 logic will break once MAX_ROWS increased, right?


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

On Fri, Jun 12, 2009 at 9:01 PM, Trilok Soni[off-list ref] wrote:

Hi Dmitry,

quoted

Input: add support for generic GPIO-based matrix keypad

From: Eric Miao <redacted>

Original patch by Marek Vasut, modified by Eric in:

1. use delayed work to simplify the debouncing
2. combine col_polarity/row_polarity into a single active_low field
3. use a generic bit array based XOR algorithm to detect key
  press/release, which should make the column assertion time
  shorter and code a bit cleaner
4. remove the ALT_FN handling, which is no way generic, the ALT_FN
  key should be treated as no different from other keys, and
  translation will be done by user space by commands like 'loadkeys'.

[dtor@mail.ru: fix error unwinding path, support changing keymap
 from userspace]
Signed-off-by: Marek Vasut <redacted>
Signed-off-by: Eric Miao <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

Did you took latest patch submitted from Eric? Because it had more
signed-off and acked-by lines, like this.

Signed-off-by: Marek Vasut <redacted>
Reviewed-by: Trilok Soni <redacted>
Reviewed-by: Uli Luckas <redacted>
Reviewed-by: Russell King <redacted>
Reviewed-by: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Eric Miao <redacted>

http://markmail.org/message/2wrr2b6mr6qsd4xs#query:+page:1+mid:fkkfxlumfm4mjhk4+state:results

Eric can confirm otherwise.

quoted

 config KEYBOARD_HIL_OLD
       tristate "HP HIL keyboard support (simple driver)"

@@ -254,7 +263,7 @@ config KEYBOARD_PXA27x

       tristate "PXA27x/PXA3xx keypad support"
       depends on PXA27x || PXA3xx
       help
-         Enable support for PXA27x/PXA3xx keypad controller
+         Enable support for PXA27x/PXA3xx keypad controller.

Why this change in this patch?

quoted

+
+                       code = (row << 4) + col;

<< 4 logic will break once MAX_ROWS increased, right?

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Diff follows:

--- drivers/input/keyboard/matrix_keypad.c.orig	2009-06-12

21:18:20.000000000 +0800

+++ drivers/input/keyboard/matrix_keypad.c	2009-06-12 21:07:26.000000000 +0800

@@ -290,6 +290,9 @@ static int __devinit init_matrix_gpio(st
 			goto err_free_irqs;
 		}
 	}
+
+	/* initialized as disabled - enabled by input->open */
+	disable_row_irqs(keypad);
 	return 0;

 err_free_irqs:

@@ -376,22 +379,19 @@ static int __devinit matrix_keypad_probe
 	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
 	input_set_drvdata(input_dev, keypad);

-	err = input_register_device(keypad->input_dev);
+	err = init_matrix_gpio(pdev, keypad);
 	if (err)
 		goto err_free_mem;

-	err = init_matrix_gpio(pdev, keypad);
+	err = input_register_device(keypad->input_dev);
 	if (err)
-		goto err_unregister;
+		goto err_free_mem;

 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 	platform_set_drvdata(pdev, keypad);

 	return 0;

-err_unregister:
-	input_unregister_device(input_dev);
-	input_dev = NULL;
 err_free_mem:
 	input_free_device(input_dev);
 	kfree(keycodes);
--

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

On Fri, Jun 12, 2009 at 6:56 PM, Eric Miao[off-list ref] wrote:

Ping? We are already half-way through open merge window.

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

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Thanks!

-- 
Dmitry

Hi Dmitry,

On Mon, Jun 29, 2009 at 9:56 PM, Dmitry
Torokhov[off-list ref] wrote:

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

quoted

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Are you planning to take "MAX7359 input switch controller driver" too
for submission to Linus in -rc1?

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

On Tue, Jun 30, 2009 at 3:43 PM, Trilok Soni[off-list ref] wrote:

Hi Dmitry,

On Mon, Jun 29, 2009 at 9:56 PM, Dmitry
Torokhov[off-list ref] wrote:

quoted

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

quoted

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Are you planning to take "MAX7359 input switch controller driver" too
for submission to Linus in -rc1?

Thanks, Trilok

Dmitry, can I know when MAX7359 keypad driver will be merged to mainline?

Regards,
Kyuwon

Dmitry Torokhov wrote:

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

quoted

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Sorry Dmitry,

Reply so late and til now I have some time to test this. The patch there
still has at least one issue I guess - the keycodes[] allocation size
should be MATRIX_MAX_ROWS * MATRIX_MAX_COLS if I understand correctly.

I'm experiencing a problem of keypad->pdata being overwritten, and found
keycodes are actually accessed by [(row << 4) + col], so that should be
a full range.

And one more minor issue that may not be significant is the hardcoding of
the '4' in this (row << 4).

Some of the key hits on my Littleton just worked smoothly with the above
correction, but not all - some key hits just gave intermittent events, I
have no idea how's that - maybe related to my scan delay and debouncing
settings

Thanks!

Hi Eric,

On Thu, Jul 9, 2009 at 3:16 PM, Eric Miao[off-list ref] wrote:

Dmitry Torokhov wrote:

quoted

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

quoted

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Sorry Dmitry,

Reply so late and til now I have some time to test this. The patch there
still has at least one issue I guess - the keycodes[] allocation size
should be MATRIX_MAX_ROWS * MATRIX_MAX_COLS if I understand correctly.

I'm experiencing a problem of keypad->pdata being overwritten, and found
keycodes are actually accessed by [(row << 4) + col], so that should be
a full range.

And one more minor issue that may not be significant is the hardcoding of
the '4' in this (row << 4).

I had pointed out this. We should probably do fls(MATRIX_MAX_COLS) there.

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

Eric Miao wrote:

Dmitry Torokhov wrote:

quoted

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

quoted

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Sorry Dmitry,

Reply so late and til now I have some time to test this. The patch there
still has at least one issue I guess - the keycodes[] allocation size
should be MATRIX_MAX_ROWS * MATRIX_MAX_COLS if I understand correctly.

I'm experiencing a problem of keypad->pdata being overwritten, and found
keycodes are actually accessed by [(row << 4) + col], so that should be
a full range.

As this driver is already in the mainline and I was trying to use it...
I get various kernel crashes due to this keycodes[] allocation size bug.
Care to send a patch for this bug?

And one more minor issue that may not be significant is the hardcoding of
the '4' in this (row << 4).

Some of the key hits on my Littleton just worked smoothly with the above
correction, but not all - some key hits just gave intermittent events, I
have no idea how's that - maybe related to my scan delay and debouncing
settings

quoted

Thanks!

Hi Paulius,

On Fri, Jul 17, 2009 at 1:21 PM, Paulius
Zaleckas[off-list ref] wrote:

Eric Miao wrote:

quoted

Dmitry Torokhov wrote:

quoted

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

quoted

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Sorry Dmitry,

Reply so late and til now I have some time to test this. The patch there
still has at least one issue I guess - the keycodes[] allocation size
should be MATRIX_MAX_ROWS * MATRIX_MAX_COLS if I understand correctly.

I'm experiencing a problem of keypad->pdata being overwritten, and found
keycodes are actually accessed by [(row << 4) + col], so that should be
a full range.

As this driver is already in the mainline and I was trying to use it...
I get various kernel crashes due to this keycodes[] allocation size bug.
Care to send a patch for this bug?

quoted

And one more minor issue that may not be significant is the hardcoding of
the '4' in this (row << 4).

Some of the key hits on my Littleton just worked smoothly with the above
correction, but not all - some key hits just gave intermittent events, I
have no idea how's that - maybe related to my scan delay and debouncing
settings

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

Trilok Soni wrote:

Hi Paulius,

On Fri, Jul 17, 2009 at 1:21 PM, Paulius
Zaleckas[off-list ref] wrote:

quoted

Eric Miao wrote:

quoted

Dmitry Torokhov wrote:

quoted

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

quoted

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Sorry Dmitry,

Reply so late and til now I have some time to test this. The patch there
still has at least one issue I guess - the keycodes[] allocation size
should be MATRIX_MAX_ROWS * MATRIX_MAX_COLS if I understand correctly.

I'm experiencing a problem of keypad->pdata being overwritten, and found
keycodes are actually accessed by [(row << 4) + col], so that should be
a full range.

As this driver is already in the mainline and I was trying to use it...
I get various kernel crashes due to this keycodes[] allocation size bug.
Care to send a patch for this bug?

quoted

And one more minor issue that may not be significant is the hardcoding of
the '4' in this (row << 4).

Some of the key hits on my Littleton just worked smoothly with the above
correction, but not all - some key hits just gave intermittent events, I
have no idea how's that - maybe related to my scan delay and debouncing
settings

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

Yes, this fixes crashes. But this is just workaround for bug in driver.

Hi Paulius,

On Fri, Jul 17, 2009 at 2:50 PM, Paulius
Zaleckas[off-list ref] wrote:

Trilok Soni wrote:

quoted

Hi Paulius,

On Fri, Jul 17, 2009 at 1:21 PM, Paulius
Zaleckas[off-list ref] wrote:

quoted

Eric Miao wrote:

quoted

Dmitry Torokhov wrote:

quoted

Hi Eric,

On Fri, Jun 12, 2009 at 09:26:43PM +0800, Eric Miao wrote:

quoted

Hi Dmitry,

I've tested the driver code, and it's basically OK except for two minor
fixes:

1) GPIO and IRQs have to be initialized before input_register_device(),
otherwise input->open() will be invoked before that, which will schedule
an immediate scan work and fail.

2) disable_irq_rows() called in init_matrix_gpio() so that by default it's
initialized to disabled - and will be enabled by input->open()

Could you please take a look at the driver as it is commited to
'for-linus' branch of my tree and let me know if you see anything wrong
there? Otherwise I inted to submit it in my next update to Linus.

Sorry Dmitry,

Reply so late and til now I have some time to test this. The patch there
still has at least one issue I guess - the keycodes[] allocation size
should be MATRIX_MAX_ROWS * MATRIX_MAX_COLS if I understand correctly.

I'm experiencing a problem of keypad->pdata being overwritten, and found
keycodes are actually accessed by [(row << 4) + col], so that should be
a full range.

As this driver is already in the mainline and I was trying to use it...
I get various kernel crashes due to this keycodes[] allocation size bug.
Care to send a patch for this bug?

quoted

And one more minor issue that may not be significant is the hardcoding of
the '4' in this (row << 4).

Some of the key hits on my Littleton just worked smoothly with the above
correction, but not all - some key hits just gave intermittent events, I
have no idea how's that - maybe related to my scan delay and debouncing
settings

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

Yes, this fixes crashes. But this is just workaround for bug in driver.

As you have access to h/w, care to submit a patch which fixes this?



-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

quoted

quoted

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

Yes, this fixes crashes. But this is just workaround for bug in driver.

As you have access to h/w, care to submit a patch which fixes this?

Dmitry & Trilok,

How about this?  Due to the fact that we are not able to sort out the
proper solution for a dynamic maximum of columns/rows, let's simplify
the fix to the patch below:

From 61ea1bd16a3636f526fb12619e84a75fa16b7f38 Mon Sep 17 00:00:00 2001

From: Eric Miao <redacted>
Date: Mon, 20 Jul 2009 11:31:08 +0800
Subject: [PATCH] input: matrix keymap size fixed to maximum

Introduced KEY_IDX(), merged keymap_data into 'matrix_keypad_platform_data'.

Signed-off-by: Eric Miao <redacted>
---
 drivers/input/keyboard/matrix_keypad.c |   22 ++++++----------------
 include/linux/input/matrix_keypad.h    |   21 +++++----------------
 2 files changed, 11 insertions(+), 32 deletions(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index e9b2e7c..a0ba134 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c

@@ -136,7 +136,7 @@ static void matrix_keypad_scan(struct work_struct *work)
 			if ((bits_changed & (1 << row)) == 0)
 				continue;
 
-			code = (row << 4) + col;
+			code = KEY_IDX(row, col);
 			input_event(input_dev, EV_MSC, MSC_SCAN, code);
 			input_report_key(input_dev,
 					 keypad->keycodes[code],

@@ -313,7 +313,6 @@ err_free_cols:
 static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 {
 	const struct matrix_keypad_platform_data *pdata;
-	const struct matrix_keymap_data *keymap_data;
 	struct matrix_keypad *keypad;
 	struct input_dev *input_dev;
 	unsigned short *keycodes;

@@ -326,20 +325,13 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	keymap_data = pdata->keymap_data;
-	if (!keymap_data) {
+	if (!pdata->keymap) {
 		dev_err(&pdev->dev, "no keymap data defined\n");
 		return -EINVAL;
 	}
 
-	if (!keymap_data->max_keymap_size) {
-		dev_err(&pdev->dev, "invalid keymap data supplied\n");
-		return -EINVAL;
-	}
-
 	keypad = kzalloc(sizeof(struct matrix_keypad), GFP_KERNEL);
-	keycodes = kzalloc(keymap_data->max_keymap_size *
-				sizeof(keypad->keycodes),
+	keycodes = kzalloc(sizeof(keypad->keycodes) * MATRIX_MAX_KEYS,
 			   GFP_KERNEL);
 	input_dev = input_allocate_device();
 	if (!keypad || !keycodes || !input_dev) {

@@ -362,16 +354,14 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 	input_dev->close	= matrix_keypad_stop;
 
 	input_dev->keycode	= keycodes;
-	input_dev->keycodesize	= sizeof(*keycodes);
-	input_dev->keycodemax	= keymap_data->max_keymap_size;
 
-	for (i = 0; i < keymap_data->keymap_size; i++) {
-		unsigned int key = keymap_data->keymap[i];
+	for (i = 0; i < pdata->keymap_size; i++) {
+		unsigned int key = pdata->keymap[i];
 		unsigned int row = KEY_ROW(key);
 		unsigned int col = KEY_COL(key);
 		unsigned short code = KEY_VAL(key);
 
-		keycodes[(row << 4) + col] = code;
+		keycodes[KEY_IDX(row, col)] = code;
 		__set_bit(code, input_dev->keybit);
 	}
 	__clear_bit(KEY_RESERVED, input_dev->keybit);

diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index 7964516..59173c9 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h

@@ -6,6 +6,7 @@
 
 #define MATRIX_MAX_ROWS		16
 #define MATRIX_MAX_COLS		16
+#define MATRIX_MAX_KEYS		(MATRIX_MAX_ROWS * MATRIX_MAX_COLS)
 
 #define KEY(row, col, val)	((((row) & (MATRIX_MAX_ROWS - 1)) << 24) |\
 				 (((col) & (MATRIX_MAX_COLS - 1)) << 16) |\

@@ -14,26 +15,13 @@
 #define KEY_ROW(k)		(((k) >> 24) & 0xff)
 #define KEY_COL(k)		(((k) >> 16) & 0xff)
 #define KEY_VAL(k)		((k) & 0xffff)
+#define KEY_IDX(row, col)	(((row) << 4) + (col))
 
 /**
- * struct matrix_keymap_data - keymap for matrix keyboards
+ * struct matrix_keypad_platform_data - platform-dependent keypad data
  * @keymap: pointer to array of uint32 values encoded with KEY() macro
  *	representing keymap
  * @keymap_size: number of entries (initialized) in this keymap
- * @max_keymap_size: maximum size of keymap supported by the device
- *
- * This structure is supposed to be used by platform code to supply
- * keymaps to drivers that implement matrix-like keypads/keyboards.
- */
-struct matrix_keymap_data {
-	const uint32_t *keymap;
-	unsigned int	keymap_size;
-	unsigned int	max_keymap_size;
-};
-
-/**
- * struct matrix_keypad_platform_data - platform-dependent keypad data
- * @keymap_data: pointer to &matrix_keymap_data
  * @row_gpios: array of gpio numbers reporesenting rows
  * @col_gpios: array of gpio numbers reporesenting colums
  * @num_row_gpios: actual number of row gpios used by device

@@ -46,7 +34,8 @@ struct matrix_keymap_data {
  * matrix_keypad driver to perform proper initialization.
  */
 struct matrix_keypad_platform_data {
-	const struct matrix_keymap_data *keymap_data;
+	const uint32_t *keymap;
+	unsigned int	keymap_size;
 
 	unsigned int	row_gpios[MATRIX_MAX_ROWS];
 	unsigned int	col_gpios[MATRIX_MAX_COLS];

-- 
1.6.0.4

Hi Eric,

On Mon, Jul 20, 2009 at 4:07 PM, Eric Miao[off-list ref] wrote:

How about just doing like this

keycodes[(row << ((fls(MATRIX_MAX_COLS) - 1))) + col] ?

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

Trilok Soni wrote:

Hi Eric,

On Mon, Jul 20, 2009 at 4:07 PM, Eric Miao[off-list ref] wrote:

quoted

quoted

quoted

quoted

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

Yes, this fixes crashes. But this is just workaround for bug in driver.

As you have access to h/w, care to submit a patch which fixes this?

Dmitry & Trilok,

How about this?  Due to the fact that we are not able to sort out the
proper solution for a dynamic maximum of columns/rows, let's simplify
the fix to the patch below:


From 61ea1bd16a3636f526fb12619e84a75fa16b7f38 Mon Sep 17 00:00:00 2001
From: Eric Miao <redacted>
Date: Mon, 20 Jul 2009 11:31:08 +0800
Subject: [PATCH] input: matrix keymap size fixed to maximum

Introduced KEY_IDX(), merged keymap_data into 'matrix_keypad_platform_data'.

Signed-off-by: Eric Miao <redacted>
---
 drivers/input/keyboard/matrix_keypad.c |   22 ++++++----------------
 include/linux/input/matrix_keypad.h    |   21 +++++----------------
 2 files changed, 11 insertions(+), 32 deletions(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index e9b2e7c..a0ba134 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c

@@ -136,7 +136,7 @@ static void matrix_keypad_scan(struct work_struct *work)
                       if ((bits_changed & (1 << row)) == 0)
                               continue;

-                       code = (row << 4) + col;
+                       code = KEY_IDX(row, col);
                       input_event(input_dev, EV_MSC, MSC_SCAN, code);
                       input_report_key(input_dev,
                                        keypad->keycodes[code],

@@ -313,7 +313,6 @@ err_free_cols:
 static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 {
       const struct matrix_keypad_platform_data *pdata;
-       const struct matrix_keymap_data *keymap_data;
       struct matrix_keypad *keypad;
       struct input_dev *input_dev;
       unsigned short *keycodes;

@@ -326,20 +325,13 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
               return -EINVAL;
       }

-       keymap_data = pdata->keymap_data;
-       if (!keymap_data) {
+       if (!pdata->keymap) {
               dev_err(&pdev->dev, "no keymap data defined\n");
               return -EINVAL;
       }

-       if (!keymap_data->max_keymap_size) {
-               dev_err(&pdev->dev, "invalid keymap data supplied\n");
-               return -EINVAL;
-       }
-
       keypad = kzalloc(sizeof(struct matrix_keypad), GFP_KERNEL);
-       keycodes = kzalloc(keymap_data->max_keymap_size *
-                               sizeof(keypad->keycodes),
+       keycodes = kzalloc(sizeof(keypad->keycodes) * MATRIX_MAX_KEYS,
                          GFP_KERNEL);
       input_dev = input_allocate_device();
       if (!keypad || !keycodes || !input_dev) {

@@ -362,16 +354,14 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
       input_dev->close        = matrix_keypad_stop;

       input_dev->keycode      = keycodes;
-       input_dev->keycodesize  = sizeof(*keycodes);
-       input_dev->keycodemax   = keymap_data->max_keymap_size;

-       for (i = 0; i < keymap_data->keymap_size; i++) {
-               unsigned int key = keymap_data->keymap[i];
+       for (i = 0; i < pdata->keymap_size; i++) {
+               unsigned int key = pdata->keymap[i];
               unsigned int row = KEY_ROW(key);
               unsigned int col = KEY_COL(key);
               unsigned short code = KEY_VAL(key);

-               keycodes[(row << 4) + col] = code;
+               keycodes[KEY_IDX(row, col)] = code;

How about just doing like this

keycodes[(row << ((fls(MATRIX_MAX_COLS) - 1))) + col] ?

I doubt this will work correctly if MATRIX_MAX_COLS isn't something 2 ^ N

Hi Eric,

On Mon, Jul 20, 2009 at 06:37:04PM +0800, Eric Miao wrote:

quoted

quoted

quoted

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

Yes, this fixes crashes. But this is just workaround for bug in driver.

As you have access to h/w, care to submit a patch which fixes this?

Dmitry & Trilok,

How about this?  Due to the fact that we are not able to sort out the
proper solution for a dynamic maximum of columns/rows, let's simplify
the fix to the patch below:


From 61ea1bd16a3636f526fb12619e84a75fa16b7f38 Mon Sep 17 00:00:00 2001
From: Eric Miao <redacted>
Date: Mon, 20 Jul 2009 11:31:08 +0800
Subject: [PATCH] input: matrix keymap size fixed to maximum

Introduced KEY_IDX(), merged keymap_data into 'matrix_keypad_platform_data'.

I would like to keep definitions in matrix_keymap.h useable to other
drivers so we either make KEY_IDX() work with different number of
columns or drop it. I would also like to keep keymap data separate so
drivers that don't use matrix encoding could still use it.

Overall, I don't quite understand what the problem with the current
drive is since it works fine as long as we set up max_keymap_size
properly. We could improve diagnostic by checking row and cols values
and warning users when they supply suspicious data and maybe adjust the
documentation, right?

-- 
Dmitry

Dmitry Torokhov wrote:

Hi Eric,

On Mon, Jul 20, 2009 at 06:37:04PM +0800, Eric Miao wrote:

quoted

quoted

quoted

quoted

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

Yes, this fixes crashes. But this is just workaround for bug in driver.

As you have access to h/w, care to submit a patch which fixes this?

Dmitry & Trilok,

How about this?  Due to the fact that we are not able to sort out the
proper solution for a dynamic maximum of columns/rows, let's simplify
the fix to the patch below:


From 61ea1bd16a3636f526fb12619e84a75fa16b7f38 Mon Sep 17 00:00:00 2001
From: Eric Miao <redacted>
Date: Mon, 20 Jul 2009 11:31:08 +0800
Subject: [PATCH] input: matrix keymap size fixed to maximum

Introduced KEY_IDX(), merged keymap_data into 'matrix_keypad_platform_data'.

I would like to keep definitions in matrix_keymap.h useable to other
drivers so we either make KEY_IDX() work with different number of
columns or drop it.

What about:

#define KEY_IDX(row, col)	(((row) * keypad->num_columns) + (col))

if we want to make it dynamic.

I would also like to keep keymap data separate so
drivers that don't use matrix encoding could still use it.

Do you mean there are possibilities that some drivers are not going to
define any matrix keycodes, and depend on EV_MSC to know the position
happened? That way, we may want to omit the ->keycodes[] accesses.

Overall, I don't quite understand what the problem with the current
drive is since it works fine as long as we set up max_keymap_size
properly.

I think the root of this problem lies in the code below:

                        code = (row << 4) + col;
                        input_event(input_dev, EV_MSC, MSC_SCAN, code);
                        input_report_key(input_dev,
                                         keypad->keycodes[code],
                                         new_state[col] & (1 << row));

that 'code = (row << 4) + col;' is hardcoded to shift left by '4', and
is then used to index into keypad->keycodes[] array, the size of which
in turn is specified by 'max_keymap_size'. This is a bit inconsistent.

If written as (row << 4) + col, it means the max_keymap_size should be
setup as 'max_rows * 16', instead of expected 'max_rows * max_cols'.

And there seems to be a typo in the allocation:

        keycodes = kzalloc(keymap_data->max_keymap_size *
                                sizeof(keypad->keycodes),
                           GFP_KERNEL);

that, 'sizeof(keypad->keycodes)' should be written as
'sizeof(keypad->keycodes[0])' if I guess it correct.

We could improve diagnostic by checking row and cols values
and warning users when they supply suspicious data and maybe adjust the
documentation, right?

On Tue, Jul 21, 2009 at 04:26:47PM +0800, Eric Miao wrote:

Dmitry Torokhov wrote:

quoted

Hi Eric,

On Mon, Jul 20, 2009 at 06:37:04PM +0800, Eric Miao wrote:

quoted

quoted

quoted

quoted

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

Yes, this fixes crashes. But this is just workaround for bug in driver.

As you have access to h/w, care to submit a patch which fixes this?

Dmitry & Trilok,

How about this?  Due to the fact that we are not able to sort out the
proper solution for a dynamic maximum of columns/rows, let's simplify
the fix to the patch below:


From 61ea1bd16a3636f526fb12619e84a75fa16b7f38 Mon Sep 17 00:00:00 2001
From: Eric Miao <redacted>
Date: Mon, 20 Jul 2009 11:31:08 +0800
Subject: [PATCH] input: matrix keymap size fixed to maximum

Introduced KEY_IDX(), merged keymap_data into 'matrix_keypad_platform_data'.

I would like to keep definitions in matrix_keymap.h useable to other
drivers so we either make KEY_IDX() work with different number of
columns or drop it.

What about:

#define KEY_IDX(row, col)	(((row) * keypad->num_columns) + (col))

if we want to make it dynamic.

I'd rather not have any references to particular data structures there,
so something like KEY_IDX(row, col, shift) or KEY_IDX(row, col, maxcol).
Would that work for you?

quoted

I would also like to keep keymap data separate so
drivers that don't use matrix encoding could still use it.

Do you mean there are possibilities that some drivers are not going to
define any matrix keycodes, and depend on EV_MSC to know the position
happened? That way, we may want to omit the ->keycodes[] accesses.

Poorly spoken on my part. I should have said "not use entire structure
from the matrix_keymap.h" but only the keymap part.

quoted

Overall, I don't quite understand what the problem with the current
drive is since it works fine as long as we set up max_keymap_size
properly.

I think the root of this problem lies in the code below:

                        code = (row << 4) + col;
                        input_event(input_dev, EV_MSC, MSC_SCAN, code);
                        input_report_key(input_dev,
                                         keypad->keycodes[code],
                                         new_state[col] & (1 << row));

that 'code = (row << 4) + col;' is hardcoded to shift left by '4', and
is then used to index into keypad->keycodes[] array, the size of which
in turn is specified by 'max_keymap_size'. This is a bit inconsistent.

Right, I agree.

If written as (row << 4) + col, it means the max_keymap_size should be
setup as 'max_rows * 16', instead of expected 'max_rows * max_cols'.

Yes, at the moment... I guess we need to put the true dimensions instead
of max size into the keymap data, right? Then we'd be able to calculate
proper shift value.

And there seems to be a typo in the allocation:

        keycodes = kzalloc(keymap_data->max_keymap_size *
                                sizeof(keypad->keycodes),
                           GFP_KERNEL);

that, 'sizeof(keypad->keycodes)' should be written as
'sizeof(keypad->keycodes[0])' if I guess it correct.

It should not hurt anything (since sizeof(keypad->keycodes) is 4 bytes)
but indeed I need to fix that.

quoted

We could improve diagnostic by checking row and cols values
and warning users when they supply suspicious data and maybe adjust the
documentation, right?

-- 
Dmitry

Dmitry Torokhov wrote:

On Tue, Jul 21, 2009 at 04:26:47PM +0800, Eric Miao wrote:

quoted

Dmitry Torokhov wrote:

quoted

Hi Eric,

On Mon, Jul 20, 2009 at 06:37:04PM +0800, Eric Miao wrote:

quoted

quoted

quoted

quoted

Did you tried assigning max_keypmap_size in platform data to
MATRIX_MAX_COLS * MATRIX_MAX_ROWS ?

Yes, this fixes crashes. But this is just workaround for bug in driver.

As you have access to h/w, care to submit a patch which fixes this?

Dmitry & Trilok,

How about this?  Due to the fact that we are not able to sort out the
proper solution for a dynamic maximum of columns/rows, let's simplify
the fix to the patch below:


From 61ea1bd16a3636f526fb12619e84a75fa16b7f38 Mon Sep 17 00:00:00 2001
From: Eric Miao <redacted>
Date: Mon, 20 Jul 2009 11:31:08 +0800
Subject: [PATCH] input: matrix keymap size fixed to maximum

Introduced KEY_IDX(), merged keymap_data into 'matrix_keypad_platform_data'.

I would like to keep definitions in matrix_keymap.h useable to other
drivers so we either make KEY_IDX() work with different number of
columns or drop it.

What about:

#define KEY_IDX(row, col)	(((row) * keypad->num_columns) + (col))

if we want to make it dynamic.

I'd rather not have any references to particular data structures there,
so something like KEY_IDX(row, col, shift) or KEY_IDX(row, col, maxcol).
Would that work for you?

quoted

quoted

I would also like to keep keymap data separate so
drivers that don't use matrix encoding could still use it.

Do you mean there are possibilities that some drivers are not going to
define any matrix keycodes, and depend on EV_MSC to know the position
happened? That way, we may want to omit the ->keycodes[] accesses.

Poorly spoken on my part. I should have said "not use entire structure
from the matrix_keymap.h" but only the keymap part.

quoted

quoted

Overall, I don't quite understand what the problem with the current
drive is since it works fine as long as we set up max_keymap_size
properly.

I think the root of this problem lies in the code below:

                        code = (row << 4) + col;
                        input_event(input_dev, EV_MSC, MSC_SCAN, code);
                        input_report_key(input_dev,
                                         keypad->keycodes[code],
                                         new_state[col] & (1 << row));

that 'code = (row << 4) + col;' is hardcoded to shift left by '4', and
is then used to index into keypad->keycodes[] array, the size of which
in turn is specified by 'max_keymap_size'. This is a bit inconsistent.

Right, I agree.

quoted

If written as (row << 4) + col, it means the max_keymap_size should be
setup as 'max_rows * 16', instead of expected 'max_rows * max_cols'.

Yes, at the moment... I guess we need to put the true dimensions instead
of max size into the keymap data, right? Then we'd be able to calculate
proper shift value.

quoted

And there seems to be a typo in the allocation:

        keycodes = kzalloc(keymap_data->max_keymap_size *
                                sizeof(keypad->keycodes),
                           GFP_KERNEL);

that, 'sizeof(keypad->keycodes)' should be written as
'sizeof(keypad->keycodes[0])' if I guess it correct.

It should not hurt anything (since sizeof(keypad->keycodes) is 4 bytes)
but indeed I need to fix that.

quoted

quoted

We could improve diagnostic by checking row and cols values
and warning users when they supply suspicious data and maybe adjust the
documentation, right?

OK, then how about this one?

From be59051b471dc87a0cd846630dd9964602b310f6 Mon Sep 17 00:00:00 2001

From: Eric Miao <redacted>
Date: Mon, 20 Jul 2009 11:31:08 +0800
Subject: [PATCH] input: make matrix keymap size dynamic

The number of rows and columns should really belong to 'struct keymap_data',
and assumption on the shift and size of rows/columns removed.

Signed-off-by: Eric Miao <redacted>
---
 drivers/input/keyboard/matrix_keypad.c |   85 +++++++++++++++++---------------
 include/linux/input/matrix_keypad.h    |    9 ++--
 2 files changed, 49 insertions(+), 45 deletions(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index e9b2e7c..aa9442a 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c

@@ -26,6 +26,8 @@
 struct matrix_keypad {
 	const struct matrix_keypad_platform_data *pdata;
 	struct input_dev *input_dev;
+	int num_rows;
+	int num_cols;
 	unsigned short *keycodes;
 
 	uint32_t last_key_state[MATRIX_MAX_COLS];

@@ -35,14 +37,16 @@ struct matrix_keypad {
 	spinlock_t lock;
 };
 
+#define KEY_IDX(kp, row, col)	(((row) * (kp)->num_cols) + col)
+
 /*
  * NOTE: normally the GPIO has to be put into HiZ when de-activated to cause
  * minmal side effect when scanning other columns, here it is configured to
  * be input, and it should work on most platforms.
  */
-static void __activate_col(const struct matrix_keypad_platform_data *pdata,
-			   int col, bool on)
+static void __activate_col(struct matrix_keypad *keypad, int col, bool on)
 {
+	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
 	bool level_on = !pdata->active_low;
 
 	if (on) {

@@ -53,22 +57,20 @@ static void __activate_col(const struct matrix_keypad_platform_data *pdata,
 	}
 }
 
-static void activate_col(const struct matrix_keypad_platform_data *pdata,
-			 int col, bool on)
+static void activate_col(struct matrix_keypad *keypad, int col, bool on)
 {
-	__activate_col(pdata, col, on);
+	__activate_col(keypad, col, on);
 
-	if (on && pdata->col_scan_delay_us)
-		udelay(pdata->col_scan_delay_us);
+	if (on && keypad->pdata->col_scan_delay_us)
+		udelay(keypad->pdata->col_scan_delay_us);
 }
 
-static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
-			      bool on)
+static void activate_all_cols(struct matrix_keypad *keypad, bool on)
 {
 	int col;
 
-	for (col = 0; col < pdata->num_col_gpios; col++)
-		__activate_col(pdata, col, on);
+	for (col = 0; col < keypad->num_cols; col++)
+		__activate_col(keypad, col, on);
 }
 
 static bool row_asserted(const struct matrix_keypad_platform_data *pdata,