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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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

-- 
Dmitry