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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
