On Thu, Sep 24, 2009 at 08:59:56AM -0600, Miguel Aguilar wrote:
> Dmitry,
>
> I addressed your comments but I still have a couple of questions.
Thank you for making the adjustments.
>
> [MA] This is the current irq function:
> static irqreturn_t davinci_ks_interrupt(int irq, void *dev_id)
> {
> struct davinci_ks *davinci_ks = dev_id;
> struct device *dev = &davinci_ks->input->dev;
> unsigned short *keymap = davinci_ks->keymap;
> u32 prev_status, new_status, changed, position;
> bool release;
> int keycode = KEY_UNKNOWN;
> int ret = IRQ_NONE;
>
> /* Disable interrupt */
> davinci_ks_write(davinci_ks, 0x0, DAVINCI_KEYSCAN_INTENA);
>
> /* Reading previous and new status of the key scan */
> prev_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_PREVSTATE);
> new_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_CURRENTST);
>
> changed = prev_status ^ new_status;
> position = ffs(changed) - 1;
>
> if (changed) {
> keycode = keymap[position];
> release = (new_status >> position) & 0x1;
> dev_dbg(dev, "davinci_keyscan: key %d %s\n",
> keycode, release ? "released" : "pressed");
>
> input_report_key(davinci_ks->input, keycode, !release);
> input_sync(davinci_ks->input);
>
> /* Clearing interrupt */
> davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL,
> DAVINCI_KEYSCAN_INTCLR);
>
> ret = IRQ_HANDLED;
> }
>
> /* Enable interrupts */
> davinci_ks_write(davinci_ks, 0x1, DAVINCI_KEYSCAN_INTENA);
>
> return ret;
I'd just return IRQ_HANDLED unconditionally and I think you should go
through all bits in changed to ensure that you don't lose key presses
(unless controller never ever reports more than 1 key pressed).
> }
>
>
>
>>> +static int __init davinci_kp_init(void)
>>> +{
>>> + return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
>>> +}
>>> +module_init(davinci_kp_init);
> [MA] Should I use platform_driver_probe?
>
I don't see why not - you are still saving memory due to discarding
davinci_kp_probe. And if driver core is changed so that unbind is a NOP
for drivers registered with platform_driver_probe I will gladly accept a
follow-up patch to change __devexit to __exit for even more savings.
>>> +static void __exit davinci_kp_exit(void)
>>> +{
>>> + platform_driver_unregister(&davinci_kp_driver);
>>> +}
>>> +module_exit(davinci_kp_exit);
> [MA] Is the module exit function __exit or __devexit
>
Module ext routines are always __exit (and inits are __init).
Thanks.
--
Dmitry
--
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
