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@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'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 -- 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
