input: make matrix keymap size dynamic

PATCH In review Eric Miao 2 participants 3 messages Aug 4, 2009 – Aug 5, 2009

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Eric Miao

author

2009-08-04 09:24 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---

Dmitry,

How about this patch to fix the .max_keymap_size issue? The numbers of rows and columns are really a part of keymap data, and by moving the num_{rows, cols} into 'struct matrix_keypad', some of the functions are changed a bit accordingly.

 drivers/input/keyboard/matrix_keypad.c |   85 +++++++++++++++++---------------
 include/linux/input/matrix_keypad.h    |    9 ++--
 2 files changed, 49 insertions(+), 45 deletions(-)

drivers/input/keyboard/matrix_keypad.c +45 ~~-40~~

index e9b2e7c..aa9442a 100644
@@ -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);

include/linux/input/matrix_keypad.h +4 -5

index 7964516..46cb3db 100644
@@ -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;

Signature · 1 line

1.6.0.4

Dmitry Torokhov

maintainer

2009-08-05 08:26 UTC #1 lore raw

Hi Eric,

Quoted 4 lines · Eric Miao

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

I aplayed with your patch for a while and moving rows and columns into struct keymap_data did not feel right; the changes to activate_cols and others where we have to access 2 different structures to get gpios and upper limit made the code awkward. In the end I decided the sizes should be kept where they were, together with the gpio arrays.

What do you think about the modified version of your patch below? The definitions seem to be working for other matrix drivers I have i my queue as well...

Signature · 17 lines

Dmitry


Input: matrix_keypad - make matrix keymap size dynamic

From: Eric Miao <[email protected]>

Remove assumption on the shift and size of rows/columns form
matrix_keypad driver.

Signed-off-by: Eric Miao <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---

 drivers/input/keyboard/matrix_keypad.c |   18 +++++++++---------
 include/linux/input/matrix_keypad.h    |   13 +++++++------
 2 files changed, 16 insertions(+), 15 deletions(-)

drivers/input/keyboard/matrix_keypad.c +9 -9

index e9b2e7c..541b981 100644
@@ -27,6 +27,7 @@ struct matrix_keypad {
 	const struct matrix_keypad_platform_data *pdata;
 	struct input_dev *input_dev;
 	unsigned short *keycodes;
+	unsigned int row_shift;
 
 	uint32_t last_key_state[MATRIX_MAX_COLS];
 	struct delayed_work work;

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

@@ -317,6 +318,7 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 	struct matrix_keypad *keypad;
 	struct input_dev *input_dev;
 	unsigned short *keycodes;
+	unsigned int row_shift;
 	int i;
 	int err;

@@ -332,14 +334,11 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	if (!keymap_data->max_keymap_size) {
-		dev_err(&pdev->dev, "invalid keymap data supplied\n");
-		return -EINVAL;
-	}
+	row_shift = get_count_order(pdata->num_col_gpios);
 
 	keypad = kzalloc(sizeof(struct matrix_keypad), GFP_KERNEL);
-	keycodes = kzalloc(keymap_data->max_keymap_size *
-				sizeof(keypad->keycodes),
+	keycodes = kzalloc((pdata->num_row_gpios << row_shift) *
+				sizeof(*keycodes),
 			   GFP_KERNEL);
 	input_dev = input_allocate_device();
 	if (!keypad || !keycodes || !input_dev) {

@@ -350,6 +349,7 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 	keypad->input_dev = input_dev;
 	keypad->pdata = pdata;
 	keypad->keycodes = keycodes;
+	keypad->row_shift = row_shift;
 	keypad->stopped = true;
 	INIT_DELAYED_WORK(&keypad->work, matrix_keypad_scan);
 	spin_lock_init(&keypad->lock);

@@ -363,7 +363,7 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 
 	input_dev->keycode	= keycodes;
 	input_dev->keycodesize	= sizeof(*keycodes);
-	input_dev->keycodemax	= keymap_data->max_keymap_size;
+	input_dev->keycodemax	= pdata->num_row_gpios << keypad->row_shift;
 
 	for (i = 0; i < keymap_data->keymap_size; i++) {
 		unsigned int key = keymap_data->keymap[i];

@@ -371,7 +371,7 @@ static int __devinit matrix_keypad_probe(struct platform_device *pdev)
 		unsigned int col = KEY_COL(key);
 		unsigned short code = KEY_VAL(key);
 
-		keycodes[(row << 4) + col] = code;
+		keycodes[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
 		__set_bit(code, input_dev->keybit);
 	}
 	__clear_bit(KEY_RESERVED, input_dev->keybit);

include/linux/input/matrix_keypad.h +7 -6

index 7964516..15d5903 100644
@@ -15,12 +15,13 @@
 #define KEY_COL(k)		(((k) >> 16) & 0xff)
 #define KEY_VAL(k)		((k) & 0xffff)
 
+#define MATRIX_SCAN_CODE(row, col, row_shift)	(((row) << (row_shift)) + (col))
+
 /**
  * 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.

@@ -28,14 +29,13 @@
 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
+ * @row_gpios: pointer to array of gpio numbers representing rows
+ * @col_gpios: pointer to 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

@@ -48,8 +48,9 @@ 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];
+	const unsigned int *row_gpios;
+	const unsigned int *col_gpios;
+
 	unsigned int	num_row_gpios;
 	unsigned int	num_col_gpios;

Eric Miao

author

2009-08-05 11:01 UTC #2 lore raw

Quoted 13 lines · Dmitry Torokhov

Hi Eric,
Quoted 3 lines · Eric Miao
The number of rows and columns should really belong to 'struct keymap_data', and assumption on the shift and size of rows/columns removed.
I aplayed with your patch for a while and moving rows and columns into struct keymap_data did not feel right; the changes to activate_cols and others where we have to access 2 different structures to get gpios and upper limit made the code awkward. In the end I decided the sizes should be kept where they were, together with the gpio arrays.

Quoted 4 lines

What do you think about the modified version of your patch below? The definitions seem to be working for other matrix drivers I have i my queue as well...

Sure go. I'm quite happy with your approach.