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@xxxxxxxxx>
>> 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?
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
