RE: [PATCH try #2] Blackfin BF54x Input Keypad controller driver

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 [off-list ref] wrote:

quoted

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:

quoted

+
+static u16 per_rows[] = {

Change to "const" perhaps?

quoted

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

quoted

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

quoted

+               add_timer(&bf54x_kpad->timer);

mod_timer()?

quoted

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

quoted

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

quoted

+       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);

quoted

+               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()?

quoted

+
+       bfin_write_KPAD_PRESCALE(bfin_kpad_get_prescale(TIME_SCALE));
+
+       bfin_write_KPAD_CTL((((pdata->cols - 1) << 13) & KPAD_COLEN)

|

quoted

+                               (((pdata->rows - 1) << 10) &

KPAD_ROWEN)

|

quoted

+                               (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);

quoted

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

quoted

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

quoted

+{

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