lkml.org 
[lkml]   [2007]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRE: [PATCH try #2] Blackfin BF54x Input Keypad controller driver
Date
From


Hi Dmitry,

Thanks for your feedback.
See my comment below.
Bryan is going to send you an updated patch shortly.

Best regards,
Michael


>Hi Bryan,
>
>On 10/4/07, Bryan Wu <bryan.wu@analog.com> wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>> 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.

Typical use cases for this driver module would be in a single user
embedded system. - More likely going through reboot than through a
module unload/load cycle with different user setting. But you are right
cleaner is to copy the key codes.


>
>> + 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.

Currently not - I'm going to remove them.

>
>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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-10-11 13:39    [W:0.055 / U:7.660 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site