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

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

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