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

[Eric Miao <eric.y.miao@gmail.com>]

Dmitry Torokhov wrote:
> On Tue, Jul 21, 2009 at 04:26:47PM +0800, Eric Miao wrote:
>> Dmitry Torokhov wrote:
>>> Hi Eric,
>>>
>>> On Mon, Jul 20, 2009 at 06:37:04PM +0800, Eric Miao wrote:
>>>>>>> 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 <eric.y.miao@gmail.com>
>>>> 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?
> 
> 
>>> 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.
> 
>>> 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.
> 
>>> 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 <eric.y.miao@gmail.com>
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 <eric.y.miao@gmail.com>
---
 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,
@@ -83,7 +85,7 @@ 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++)
+	for (i = 0; i < keypad->num_rows; i++)
 		enable_irq(gpio_to_irq(pdata->row_gpios[i]));
 }
 
@@ -92,7 +94,7 @@ 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++)
+	for (i = 0; i < keypad->num_rows; i++)
 		disable_irq_nosync(gpio_to_irq(pdata->row_gpios[i]));
 }
 
@@ -109,34 +111,34 @@ static void matrix_keypad_scan(struct work_struct *work)
 	int row, col, code;
 
 	/* de-activate all columns for scanning */
-	activate_all_cols(pdata, false);
+	activate_all_cols(keypad, 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++) {
+	for (col = 0; col < keypad->num_cols; col++) {
 
-		activate_col(pdata, col, true);
+		activate_col(keypad, col, true);
 
-		for (row = 0; row < pdata->num_row_gpios; row++)
+		for (row = 0; row < keypad->num_rows; row++)
 			new_state[col] |=
 				row_asserted(pdata, row) ? (1 << row) : 0;
 
-		activate_col(pdata, col, false);
+		activate_col(keypad, col, false);
 	}
 
-	for (col = 0; col < pdata->num_col_gpios; col++) {
+	for (col = 0; col < keypad->num_cols; 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++) {
+		for (row = 0; row < keypad->num_rows; row++) {
 			if ((bits_changed & (1 << row)) == 0)
 				continue;
 
-			code = (row << 4) + col;
+			code = KEY_IDX(keypad, row, col);
 			input_event(input_dev, EV_MSC, MSC_SCAN, code);
 			input_report_key(input_dev,
 					 keypad->keycodes[code],
@@ -147,7 +149,7 @@ static void matrix_keypad_scan(struct work_struct *work)
 
 	memcpy(keypad->last_key_state, new_state, sizeof(new_state));
 
-	activate_all_cols(pdata, true);
+	activate_all_cols(keypad, true);
 
 	/* Enable IRQs again */
 	spin_lock_irq(&keypad->lock);
@@ -221,7 +223,7 @@ static int matrix_keypad_suspend(struct platform_device *pdev, pm_message_t stat
 	matrix_keypad_stop(keypad->input_dev);
 
 	if (device_may_wakeup(&pdev->dev))
-		for (i = 0; i < pdata->num_row_gpios; i++)
+		for (i = 0; i < keypad->num_rows; i++)
 			enable_irq_wake(gpio_to_irq(pdata->row_gpios[i]));
 
 	return 0;
@@ -234,7 +236,7 @@ static int matrix_keypad_resume(struct platform_device *pdev)
 	int i;
 
 	if (device_may_wakeup(&pdev->dev))
-		for (i = 0; i < pdata->num_row_gpios; i++)
+		for (i = 0; i < keypad->num_rows; i++)
 			disable_irq_wake(gpio_to_irq(pdata->row_gpios[i]));
 
 	matrix_keypad_start(keypad->input_dev);
@@ -253,7 +255,7 @@ static int __devinit init_matrix_gpio(struct platform_device *pdev,
 	int i, err = -EINVAL;
 
 	/* initialized strobe lines as outputs, activated */
-	for (i = 0; i < pdata->num_col_gpios; i++) {
+	for (i = 0; i < keypad->num_cols; i++) {
 		err = gpio_request(pdata->col_gpios[i], "matrix_kbd_col");
 		if (err) {
 			dev_err(&pdev->dev,
@@ -265,7 +267,7 @@ static int __devinit init_matrix_gpio(struct platform_device *pdev,
 		gpio_direction_output(pdata->col_gpios[i], !pdata->active_low);
 	}
 
-	for (i = 0; i < pdata->num_row_gpios; i++) {
+	for (i = 0; i < keypad->num_rows; i++) {
 		err = gpio_request(pdata->row_gpios[i], "matrix_kbd_row");
 		if (err) {
 			dev_err(&pdev->dev,
@@ -277,7 +279,7 @@ static int __devinit init_matrix_gpio(struct platform_device *pdev,
 		gpio_direction_input(pdata->row_gpios[i]);
 	}
 
-	for (i = 0; i < pdata->num_row_gpios; i++) {
+	for (i = 0; i < keypad->num_rows; i++) {
 		err = request_irq(gpio_to_irq(pdata->row_gpios[i]),
 				matrix_keypad_interrupt,
 				IRQF_DISABLED |
@@ -298,11 +300,11 @@ static int __devinit init_matrix_gpio(struct platform_device *pdev,
 err_free_irqs:
 	while (--i >= 0)
 		free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad);
-	i = pdata->num_row_gpios;
+	i = keypad->num_rows;
 err_free_rows:
 	while (--i >= 0)
 		gpio_free(pdata->row_gpios[i]);
-	i = pdata->num_col_gpios;
+	i = keypad->num_cols;
 err_free_cols:
 	while (--i >= 0)
 		gpio_free(pdata->col_gpios[i]);
@@ -317,7 +319,7 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 	struct matrix_keypad *keypad;
 	struct input_dev *input_dev;
 	unsigned short *keycodes;
-	int i;
+	int i, num_rows, num_cols;
 	int err;
 
 	pdata = pdev->dev.platform_data;
@@ -332,14 +334,17 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	if (!keymap_data->max_keymap_size) {
+	if (!keymap_data->keymap_rows || !keymap_data->keymap_cols) {
 		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),
+
+	num_rows = pdata->keymap_data->keymap_rows;
+	num_cols = pdata->keymap_data->keymap_cols;
+	keycodes = kzalloc(num_rows * num_cols *
+				sizeof(keypad->keycodes[0]),
 			   GFP_KERNEL);
 	input_dev = input_allocate_device();
 	if (!keypad || !keycodes || !input_dev) {
@@ -349,6 +354,8 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 
 	keypad->input_dev = input_dev;
 	keypad->pdata = pdata;
+	keypad->num_rows = num_rows;
+	keypad->num_cols = num_cols;
 	keypad->keycodes = keycodes;
 	keypad->stopped = true;
 	INIT_DELAYED_WORK(&keypad->work, matrix_keypad_scan);
@@ -362,16 +369,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_data->keymap_size; i++) {
+		unsigned int key = pdata->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;
+		keypad->keycodes[KEY_IDX(keypad, row, col)] = code;
 		__set_bit(code, input_dev->keybit);
 	}
 	__clear_bit(KEY_RESERVED, input_dev->keybit);
@@ -407,12 +412,12 @@ static int __devexit matrix_keypad_remove(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 0);
 
-	for (i = 0; i < pdata->num_row_gpios; i++) {
+	for (i = 0; i < keypad->num_rows; 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++)
+	for (i = 0; i < keypad->num_cols; i++)
 		gpio_free(pdata->col_gpios[i]);
 
 	input_unregister_device(keypad->input_dev);
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index 7964516..46cb3db 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -27,8 +27,9 @@
  */
 struct matrix_keymap_data {
 	const uint32_t *keymap;
+	unsigned int	keymap_rows;
+	unsigned int	keymap_cols;
 	unsigned int	keymap_size;
-	unsigned int	max_keymap_size;
 };
 
 /**
@@ -48,10 +49,8 @@ struct matrix_keymap_data {
 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;
+	const int	*row_gpios;
+	const int	*col_gpios;
 
 	unsigned int	col_scan_delay_us;
 
-- 
1.6.0.4