Hi Trilok, Thanks for your comments...
[snip] > > + > > +/* Interrupt thread handler thread */ > > + > > +static irqreturn_t omap_keypad_threaded(int irq, void *dev_id) > > +{ > > Why is iti threaded? I fo not see anything that will sleep. It was implemented based on previous comments... > > > + struct omap_keypad *keypad_data = dev_id; > > + struct input_dev *input = keypad_data->input; > > + unsigned char key_state[8]; > > + int col, row, code; > > + u32 *new_state = (u32 *) key_state; > > + > > + *new_state = __raw_readl(keypad_data->base + > > + OMAP4_KBD_FULLCODE31_0); > > + *(new_state + 1) = __raw_readl(keypad_data->base + > > + OMAP4_KBD_FULLCODE63_32); > > + > > + for (col = 0; col < keypad_data->cols; col++) { > > + for (row = 0; row < keypad_data->rows; row++) { > > + code = MATRIX_SCAN_CODE(row, col, > > + keypad_data->row_shift); > > + if (code < 0) { > > + printk(KERN_INFO "Spurious key %d-%d\n", > > + col, row); > > + continue; > > + } > > MATRIX_SCAN_CODE() is static data. ISR is wrong place to report > incorrect values in keymap... Removed > > > + input_report_key(input, keypad_data->keycodes[code], > > + key_state[col] & (1 << row)); > > + } > > + } > > + > > + memcpy(keypad_data->key_state, &key_state, > > + sizeof(keypad_data->key_state)); > > + > > + /* enable interrupts */ > > + __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY, > > + keypad_data->base + OMAP4_KBD_IRQENABLE); > > + > > + /* clear pending interrupts */ > > + __raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS), > > + keypad_data->base + OMAP4_KBD_IRQSTATUS); > > + > > + /* clear pending interrupts */ > > + return IRQ_HANDLED; > > + > > +} > > + > > + > > +static int __devinit omap_keypad_probe(struct platform_device *pdev) > > +{ > > + struct omap_device *od; > > + struct omap_hwmod *oh; > > + struct matrix_keypad_platform_data *pdata; > > + struct input_dev *input_dev; > > + const struct matrix_keymap_data *keymap_data; > > + struct omap_keypad *keypad_data; > > + > > + unsigned int status = 0, row_shift = 0, error = 0, length = 0, id = 0; > > + unsigned short *keypad_codes; > > + > > + int hw_mod_name_len = 16; > > + char oh_name[hw_mod_name_len]; > > + char *name = "omap4-keypad"; > > + > > + length = snprintf(oh_name, hw_mod_name_len, "keyboard"); > > + WARN(length >= hw_mod_name_len, > > + "String buffer overflow in keyboard device setup\n"); > > + > > + oh = omap_hwmod_lookup(oh_name); > > + if (!oh) > > + pr_err("Could not look up %s\n", oh_name); > > + > > + pdata = kzalloc(sizeof(struct matrix_keypad_platform_data), > > + GFP_KERNEL); > > Why is it allocated? Moved to board file... > > > + > > + od = omap_device_build(name, id, oh, pdata, > > + sizeof(struct matrix_keypad_platform_data), > > + omap_keyboard_latency, > > + ARRAY_SIZE(omap_keyboard_latency), 1); > > + WARN(IS_ERR(od), "Could not build omap_device for %s %s\n", > > + name, oh_name); > > + > > + /* platform data */ > > + pdata = pdev->dev.platform_data; > > Umm, here you are overriding pdata pointer... don't you leak memory > doing this? hwmod has been moved to board configuration so we need to get platform data > > > > + if (!pdata) { > > + dev_err(&pdev->dev, "no platform data defined\n"); > > + kfree(pdata); > > + return -EINVAL; > > + } > > + > > + /* keymap data */ > > + keymap_data = pdata->keymap_data; > > + if (!keymap_data) { > > + dev_err(&pdev->dev, "no keymap data defined\n"); > > + kfree(keymap_data); > > + return -EINVAL; > > + } > > + > > + /* omap keypad data */ > > + row_shift = get_count_order(pdata->num_col_gpios); > > + keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL); > > + if (!keypad_data) { > > + error = -ENOMEM; > > + goto failed_free_platform; > > + } > > + > > + /* omap keypad codes */ > > + keypad_codes = kzalloc((pdata->num_row_gpios << row_shift) * > > + sizeof(*keypad_codes), GFP_KERNEL); > > Why is it al;loctaed separately? Can it be pickky-backed onto > keypad_data? Not getting the idea here... could you explain? > > > + if (!keypad_codes) { > > + error = -ENOMEM; > > + goto failed_free_data; > > + } > > + > > + /* input device allocation */ > > + input_dev = input_allocate_device(); > > + if (!input_dev) { > > + error = -ENOMEM; > > + goto failed_free_codes; > > + } > > + > > + /* base resource */ > > + keypad_data->base = oh->_rt_va; > > + if (!keypad_data->base) { > > > You could probably check this earlier, before allocating stuff. Early means before input_allocate_device? > > > + error = -ENOMEM; > > + goto failed_free_input; > > + } > > + > > + /* irq */ > > + keypad_data->irq = oh->mpu_irqs[0].irq; > > + if (keypad_data->irq < 0) { > > You could probably check this earlier, before allocating stuff. Early means before input_allocate_device? > > > + dev_err(&pdev->dev, "no keyboard irq assigned\n"); > > + error = keypad_data->irq; > > + goto failed_free_base; > > + } > > + > > + /* threaded irq init */ > > + status = request_threaded_irq(keypad_data->irq, omap_keypad_interrupt, > > + omap_keypad_threaded, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > + "omap4-keypad", keypad_data); > > + if (status) { > > + dev_err(&pdev->dev, "failed to register interrupt\n"); > > + error = -ENOMEM; > > + goto failed_free_irq; > > + } > > + > > + keypad_data->input = input_dev; > > + keypad_data->row_shift = row_shift; > > + keypad_data->rows = pdata->num_row_gpios; > > + keypad_data->cols = pdata->num_col_gpios; > > + keypad_data->keycodes = keypad_codes; > > + > > + input_dev->name = pdev->name; > > + input_dev->id.bustype = BUS_HOST; > > + input_dev->dev.parent = &pdev->dev; > > + input_dev->id.vendor = 0x0001; > > + input_dev->id.product = 0x0001; > > + input_dev->id.version = 0x0100; > > + > > + input_dev->keycode = keypad_codes; > > + input_dev->keycodesize = sizeof(*keypad_codes); > > You also need keycodemax so that keymap can be changed from userpsace. Added! > > > + > > + matrix_keypad_build_keymap(keymap_data, row_shift, > > + input_dev->keycode, input_dev->keybit); > > + > > + __set_bit(EV_REP, input_dev->evbit); > > + __set_bit(KEY_OK, input_dev->keybit); > > Why KEY_OK is set up separately? In previous implementation manual set of bit was needed, now removed... > > > + __set_bit(EV_KEY, input_dev->evbit); > > + > > + input_set_capability(input_dev, EV_MSC, MSC_SCAN); > > You forgot to actualy report MSC_SCAN though. Should I remove it? I think I do need it only when KEY_UNKNOWN keys will be remapped, right? > > > + input_set_drvdata(input_dev, keypad_data); > > + > > + status = input_register_device(keypad_data->input); > > + if (status < 0) { > > + dev_err(&pdev->dev, "failed to register input device\n"); > > + goto failed_free_device; > > + } > > + > > + omap_keypad_config(keypad_data); > > + > > + platform_set_drvdata(pdev, keypad_data); > > + > > + return 0; > > + > > +failed_free_device: > > + input_unregister_device(keypad_data->input); > > input_dev = NULL; > > or you will try to free it later. Not getting the idea here... could you explain? > > > +failed_free_irq: > > + free_irq(keypad_data->irq, pdev); > > +failed_free_base: > > + keypad_data->base = NULL; > > +failed_free_input: > > + input_free_device(input_dev); > > +failed_free_codes: > > + kfree(keypad_codes); > > +failed_free_data: > > + kfree(keypad_data); > > +failed_free_platform: > > + kfree(keymap_data); > > + kfree(pdata); > > + return error; > > +} > > + > > +static int __devexit omap_keypad_remove(struct platform_device *pdev) > > +{ > > + struct omap_keypad *keypad_data = platform_get_drvdata(pdev); > > + > > + input_unregister_device(keypad_data->input); > > + free_irq(keypad_data->irq, keypad_data); > > Thsi should happen before unregistering input device, othersie there is > a change IRQ will arrive when input device is freed. Done! Best Regards Abraham -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html