Hi again Dmitry,
> No worries, although at first I was surprised that Trilok spoke exactly
> the same words I did ;)
>
:)
> > > > > +
> > > > > +/* 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...
> >
>
> Would you point me to that comment? Like I said, I do not see anything
> that would possibly sleep in this routine so you don't need to use
> threaded interrupt.
http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg26352.html
>
> >
> > > > > + 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?
>
> Do something like
>
> struct omap_keypad {
> ...
> unsigned short keymap[];
> };
>
> and then
>
> keypad_data = kzalloc(sizeof(struct omap_keypad) +
> (pdata->num_row_gpios << row_shift) *
> sizeof(unsigned short),
> GFP_KERNEL);
>
> Or, depending on the max allowed keymap size, just embed array of
> maximum possible size into struct omap_keypad..
I'll work on it... will send version 2 of driver
> >
> > > > > + 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?
> >
>
> Yes. Check data before allocating resources.
I'll work on it... will send version 2 of driver
>
> >
> > > > > + 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?
> >
>
> Yes.
Thanks!
>
> > > > > + 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?
>
> I'd rather you actually report EV_MSC/MSC_SCAN.
Will add...
>
> >
> > > >
> > > > > + 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?
>
> It is not allowed to call input_free_device() after
> input_unregister_device() due to the fact that input devices are
> fercounted and you'd cause refcount underrun and potentially
> double-free. However input_free_device() is happy to accept NULL as an
> argument to it is a common way to handle the problem whike having
> streamlined error handling path.
Ok...
>
> >
> > > > > +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!
> >
>
> Thanks for making the changes.
Will send version 2, thanks to you for the comments!
Best Regards
Abraham
--
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
