Saturday 18 December 2010 07:01:04 Dmitry Torokhov napisaÅ(a):
> Hi Janusz,
>
> On Fri, Dec 17, 2010 at 11:32:04PM +0100, Janusz Krzysztofik wrote:
> > Most keypad drivers make use of the <linux/input/matrix_keypad.h>
> > defined macros, structures and inline functions.
> >
> > Convert omap-keypad driver to use those as well, as suggested by a
> > compile time warning, hardcoded into the OMAP <palt/keypad.h>.
>
> Looks nice, couple of comments.
>
> > struct omap_kp_platform_data {
> > int rows;
> > int cols;
> > - int *keymap;
> > - unsigned int keymapsize;
> > + const struct matrix_keymap_data *keymap_data;
> > unsigned int rep:1;
> > unsigned long delay;
> > unsigned int dbounce:1;
>
> Since you are updating all boards maybe also change rep and dbounce
> to bool?
OK.
> > ---
> > linux-2.6.37-rc5/drivers/input/keyboard/omap-keypad.c.orig 2010-12-
> >09 23:08:09.000000000 +0100 +++
> > linux-2.6.37-rc5/drivers/input/keyboard/omap-keypad.c 2010-12-16
> > 21:04:07.000000000 +0100 @@ -65,7 +65,6 @@ struct omap_kp {
> >
> > static DECLARE_TASKLET_DISABLED(kp_tasklet, omap_kp_tasklet, 0);
> >
> > -static int *keymap;
> > static unsigned int *row_gpios;
> > static unsigned int *col_gpios;
> >
> > @@ -162,20 +161,11 @@ static void omap_kp_scan_keypad(struct o
> > }
> > }
> >
> > -static inline int omap_kp_find_key(int col, int row)
> > -{
> > - int i, key;
> > -
> > - key = KEY(col, row, 0);
> > - for (i = 0; keymap[i] != 0; i++)
> > - if ((keymap[i] & 0xff000000) == key)
> > - return keymap[i] & 0x00ffffff;
> > - return -1;
> > -}
> > -
> > static void omap_kp_tasklet(unsigned long data)
> > {
> > struct omap_kp *omap_kp_data = (struct omap_kp *) data;
> > + unsigned short *keycodes = omap_kp_data->input->keycode;
> > + unsigned int row_shift = get_count_order(omap_kp_data->cols);
> > unsigned char new_state[8], changed, key_down = 0;
> > int col, row;
> > int spurious = 0;
> > @@ -199,7 +189,7 @@ static void omap_kp_tasklet(unsigned lon
> > row, (new_state[col] & (1 << row)) ?
> > "pressed" : "released");
> > #else
> > - key = omap_kp_find_key(col, row);
> > + key = keycodes[MATRIX_SCAN_CODE(row, col, row_shift)];
> > if (key < 0) {
> > printk(KERN_WARNING
> > "omap-keypad: Spurious key event %d-%d\n",
> > @@ -298,16 +288,22 @@ static int __devinit omap_kp_probe(struc
> > struct input_dev *input_dev;
> > struct omap_kp_platform_data *pdata = pdev->dev.platform_data;
> > int i, col_idx, row_idx, irq_idx, ret;
> > + unsigned short *keycodes;
> > + unsigned int row_shift;
> >
> > - if (!pdata->rows || !pdata->cols || !pdata->keymap) {
> > - printk(KERN_ERR "No rows, cols or keymap from pdata\n");
> > + if (!pdata->rows || !pdata->cols || !pdata->keymap_data) {
> > + printk(KERN_ERR "No rows, cols or keymap_data from pdata\n");
> > return -EINVAL;
> > }
> >
> > omap_kp = kzalloc(sizeof(struct omap_kp), GFP_KERNEL);
> > + row_shift = get_count_order(pdata->cols);
> > + keycodes = kzalloc((pdata->rows << row_shift) *
> > sizeof(*keycodes), + GFP_KERNEL);
> > input_dev = input_allocate_device();
> > - if (!omap_kp || !input_dev) {
> > + if (!omap_kp || !keycodes || !input_dev) {
> > kfree(omap_kp);
> > + kfree(keycodes);
>
> I do not see where you freeing keycodes in case something else fails
> nor in omap_kp_remove().
Yeah, I missed that, thanks.
> But why don't you put keycodes at the end of
> struct omap_kp and allocate it in one shot?
Perhaps because of blindly following a matrix_keypad.c pattern ;-).
I'll do as you suggest, thank you.
Janusz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
