Re: [PATCH V2 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar <viresh.kumar@st.com>]

On 3/27/2012 9:15 PM, Stephen Warren wrote:
>> >  static int __devinit tegra_kbc_probe(struct platform_device *pdev)
> ...
>> > +	if (pdev->dev.of_node) {
>> > +		/* FIXME: Add handling of linux,fn-keymap here */
>> > +		err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
>> > +				input_dev->keycode, input_dev->keybit,
>> > +				"linux,keymap");
> Where do input_dev->keycode/keybit get allocated? As far as I can tell,
> matrix_keypad_of_build_keymap() just writes to those and assumes they're
> already allocated.

If i am not reading the code incorrectly, keycode is allocated memory with
kbc. And then we do this:

	input_dev->keycode = kbc->keycode;

keybit is again present as part of struct input_dev.
Am i missing something.

>> > diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
> ...
>> > +int matrix_keypad_of_build_keymap(struct device *dev, unsigned int row_shift,
> ...
>> > +		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
>> > +		__set_bit(code, keybit);
> How bit are keymap and keybit?

Couldn't get this one. :(
Can you please elaborate the question a bit?

> I think we need range-checking here to
> make sure that row/col/row_shift/code are valid and in-range.

I picked this directly from matrix_keypad_build_keymap() as is.
Anyway there is no loss in improving it. :)

What kind of range-check you are looking for?
Currently we do following

		unsigned int row = KEY_ROW(key);
		unsigned int col = KEY_COL(key);
		unsigned short code = KEY_VAL(key);

All these macros '&' 'key' with 0xFF, 0xFF and 0xFFFF.
Which is also kind of range checking.

-- 
viresh