Hi Bryan, On 10/4/07, Bryan Wu <[EMAIL PROTECTED]> wrote: > From: Michael Hennerich <[EMAIL PROTECTED]> > Blackfin BF54x Input Keypad controller driver: > > [try #2] Changelog: > - Coding style issue fixes > - using a temp variable for bf54x_kpad->input > - Other updates according to Dmitry's review >
I would like to ask you for a couple of additional changes: > + > +static u16 per_rows[] = { Change to "const" perhaps? > + P_KEY_ROW7, > + P_KEY_ROW6, > + P_KEY_ROW5, > + P_KEY_ROW4, > + P_KEY_ROW3, > + P_KEY_ROW2, > + P_KEY_ROW1, > + P_KEY_ROW0, > + 0 > +}; > + > +static u16 per_cols[] = { Same here. > + P_KEY_COL7, > + P_KEY_COL6, > + P_KEY_COL5, > + P_KEY_COL4, > + P_KEY_COL3, > + P_KEY_COL2, > + P_KEY_COL1, > + P_KEY_COL0, > + 0 > +}; > + > + > + if (bfin_kpad_get_keypressed(bf54x_kpad)) { > + disable_irq(bf54x_kpad->irq); > + bf54x_kpad->lastkey = key; > + bf54x_kpad->timer.expires = jiffies > + + bf54x_kpad->keyup_test_jiffies; > + add_timer(&bf54x_kpad->timer); mod_timer()? > + > + input->keycode = pdata->keymap; Please consider allocating memory for keycode so that platfrom data can be kept constant. > + input->keycodesize = sizeof(unsigned short); > + input->keycodemax = pdata->keymapsize; > + > + /* setup input device */ > + set_bit(EV_KEY, input->evbit); > + > + if (pdata->repeat) > + set_bit(EV_REP, input->evbit); > + __set_bit() should suffice here and above. > + for (i = 0; i < pdata->keymapsize; i++) > + __set_bit(pdata->keymap[i] & KEY_MAX, input->keybit); > + > + __clear_bit(KEY_RESERVED, input->keybit); > + > + error = input_register_device(input); > + > + if (error) { > + printk(KERN_ERR DRV_NAME > + ": Unable to register input device (%d)\n", error); > + goto out4; > + } > + > + /* Init Keypad Key Up/Release test timer */ > + > + init_timer(&bf54x_kpad->timer); > + bf54x_kpad->timer.function = bfin_kpad_timer; > + bf54x_kpad->timer.data = (unsigned long) pdev; > + setup_timer()? > + > + bfin_write_KPAD_PRESCALE(bfin_kpad_get_prescale(TIME_SCALE)); > + > + bfin_write_KPAD_CTL((((pdata->cols - 1) << 13) & KPAD_COLEN) | > + (((pdata->rows - 1) << 10) & KPAD_ROWEN) | > + (2 & KPAD_IRQMODE)); > + > + > + bfin_write_KPAD_CTL(bfin_read_KPAD_CTL() | KPAD_EN); > + > + printk(KERN_ERR DRV_NAME > + ": Blackfin BF54x Keypad registered IRQ %d\n", > bf54x_kpad->irq); > + > + return 0; > + > + > +out4: > + input_free_device(input); > +out3: > + free_irq(bf54x_kpad->irq, pdev); > +out2: > + peripheral_free_list(&per_cols[MAX_RC - pdata->cols]); > +out1: > + peripheral_free_list(&per_rows[MAX_RC - pdata->rows]); > +out: > + kfree(bf54x_kpad); > + platform_set_drvdata(pdev, NULL); > + > + return error; > +} > + > +static int __devexit bfin_kpad_remove(struct platform_device *pdev) > +{ > + struct bfin_kpad_platform_data *pdata = pdev->dev.platform_data; > + struct bf54x_kpad *bf54x_kpad = platform_get_drvdata(pdev); > + > + > + del_timer_sync(&bf54x_kpad->timer); > + free_irq(bf54x_kpad->irq, pdev); > + > + peripheral_free_list(&per_rows[MAX_RC - pdata->rows]); > + peripheral_free_list(&per_cols[MAX_RC - pdata->cols]); > + > + input_unregister_device(bf54x_kpad->input); > + > + kfree(bf54x_kpad); > + platform_set_drvdata(pdev, NULL); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int bfin_kpad_suspend(struct platform_device *pdev, pm_message_t > state) > +{ Do you need these empty functions? At least stop timer here. Thanks! Oh, one more thing - do not access input->private directly - it is going away - use input_set_drvdata() and input_get_drvdata(). -- Dmitry