On Sat, Sep 05, 2009 at 10:55:24PM +0900, 양진성 wrote:
> +static void s3c_keypad_timer_handler(unsigned long data)
> +{
> + if (keymask_low | keymask_high) {
> + mod_timer(&keypad->timer, jiffies + HZ / 10);
> + } else {
> + cfg = readl(keypad->regs + S3C_KEYIFCON);
> + cfg &= ~S3C_KEYIF_CON_MASK_ALL;
> + cfg |= (S3C_KEYIF_INT_F_EN | S3C_KEYIF_INT_R_EN |
> + S3C_KEYIF_DF_EN | S3C_KEYIF_FC_EN);
> + writel(cfg, keypad->regs + S3C_KEYIFCON);
> +
> + keypad->timer_enabled = 0;
This looks racy - we first enable the interrupts and then change the
variable that flags the timer as enabled. Potentially the interrupt
could fire between these two operations, meaning that it would see the
timer as disabled.
> +static irqreturn_t s3c_keypad_irq(int irq, void *dev_id)
> +{
> + keypad->timer.expires = jiffies + (HZ / 100);
> + if (keypad->timer_enabled) {
> + mod_timer(&keypad->timer, keypad->timer.expires);
> + } else {
> + add_timer(&keypad->timer);
> + keypad->timer_enabled = 1;
> + }
The driver should be able to avoid the above issue by just calling
mod_timer() unconditionally here - if mod_timer() is called on an
inactive timer then it will activate it for you. This would also avoid
modifying the timer expiry while the timer is active, which I'm not
convinced is safe. This would mean that the driver wouldn't need to
remember if the timer was enabled.
> +#define res_size(res) ((res)->end - (res)->start + 1)
There's resource_size() for this.
> + /* Register the input device */
> + error = input_register_device(input);
> + if (error) {
> + dev_err(&pdev->dev, "failed to register input device\n");
> + goto err_reg_input;
> + }
This is called after your interrupt handler is registerd. Are you sure
that the interrupt handler can safely run before this happens - it looks
> + device_init_wakeup(&pdev->dev, 1);
> + dev_info(&pdev->dev, "Samsung Keypad Interface driver\n");
I'd drop this dev_info().
> +static int __devexit s3c_keypad_remove(struct platform_device *pdev)
> +{
I can't see anything here or elsewhere which stops the timer you're
using to poll the keypad. This means that the timer may still be
running if someone is pressing a key when the driver is removed. I
believe the same issue applies over suspend and resume.
> +static struct platform_driver s3c_keypad_driver = {
> + .probe = s3c_keypad_probe,
> + .remove = s3c_keypad_remove,
> + .suspend = s3c_keypad_suspend,
> + .resume = s3c_keypad_resume,
The driver should be using dev_pm_ops rather than the suspend and resume
methods of the struct platform_driver.
--
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
