Re: [PATCH 1/2] Input: DaVinci Keypad Driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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