| Message ID | 4AA49C53.3030400@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote: > We already implemented and tested the keypad driver for s5pc100 & > s5pc110 and maybe it will operate on s3c64xx. We use the samsung prefix > to support three cpu. Because there is no the arch for s5pc100 and > s5pc110, this driver doesn't compile yet on upstream kernel. The schedule of System LSI (the department that makes the SoC and provides the BSP) is to first complete the 6410 support in the mainline kernel, and then incrementally submit the core architecture code for 6440, c100 and c110, followed by driver additions. So the events (ordered by time) have to be in the following order 1) the s3c-keypad driver with 6400/6410 support needs to be submitted and submitted mainline. This is what Jinsun was starting, and which he will continue until it is included mainline. 2) the c100 / c110 or other SoC core architecture support needs to be submitted mainline. Nobody at System LSI is working on this yet, but they already have a tree based on 2.6.31-rcX for this. I suppose the real work on submitting this will only be started after most of the pending 6410 stuff has been submitted. 3) Samsung (either system LSI or DMC) can then send an incremental patch for adding 6440/c100/c110 support to the s3c-keypad driver. Regards,
On 9/7/2009 3:33 PM, Harald Welte wrote: > On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote: >> We already implemented and tested the keypad driver for s5pc100 & >> s5pc110 and maybe it will operate on s3c64xx. We use the samsung prefix >> to support three cpu. Because there is no the arch for s5pc100 and >> s5pc110, this driver doesn't compile yet on upstream kernel. > > The schedule of System LSI (the department that makes the SoC and provides the > BSP) is to first complete the 6410 support in the mainline kernel, and then > incrementally submit the core architecture code for 6440, c100 and c110, > followed by driver additions. > > So the events (ordered by time) have to be in the following order > > 1) the s3c-keypad driver with 6400/6410 support needs to be submitted and > submitted mainline. This is what Jinsun was starting, and which he > will continue until it is included mainline. > The keypad of s3c64xx and s5pc1xx is same almost, so we need to submit the well-defined driver from the first. > 2) the c100 / c110 or other SoC core architecture support needs to be > submitted mainline. Nobody at System LSI is working on this yet, > but they already have a tree based on 2.6.31-rcX for this. I suppose > the real work on submitting this will only be started after most of the > pending 6410 stuff has been submitted. > > 3) Samsung (either system LSI or DMC) can then send an incremental patch > for adding 6440/c100/c110 support to the s3c-keypad driver. > > Regards, -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joonyoung, On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote: > + > +static irqreturn_t samsung_kp_interrupt(int irq, void *dev_id) > +{ > + struct samsung_keypad *keypad = dev_id; > + > + if (!work_pending(&keypad->work.work)) { > + disable_irq_nosync(keypad->irq); > + atomic_inc(&keypad->irq_disable); > + schedule_delayed_work(&keypad->work, msecs_to_jiffies(10)); Why do you need to have the delayed work? Can't we query the touchpad state immediately? Or are you trying to implement debounce logic? Also, the driver seems to be using the edge-triggered interrupts; do you really need to disable IRQ until you scan the keypad? > + } > + > + return IRQ_HANDLED; > +} > + > +static int __devinit samsung_kp_probe(struct platform_device *pdev) > +{ > + const struct samsung_keypad_platdata *pdata; > + struct samsung_keypad *keypad; > + struct resource *res; > + struct input_dev *input_dev; > + unsigned short *keycodes; > + unsigned int max_keymap_size; > + unsigned int val; > + int i; > + int ret; > + > + pdata = pdev->dev.platform_data; > + if (!pdata) { > + dev_err(&pdev->dev, "no platform data defined\n"); > + return -EINVAL; > + } > + > + if ((pdata->num_rows <= 0) || (pdata->num_rows > SAMSUNG_MAX_ROWS)) > + return -EINVAL; > + > + if ((pdata->num_cols <= 0) || (pdata->num_cols > SAMSUNG_MAX_COLS)) > + return -EINVAL; > + > + /* initialize the gpio */ > + if (pdata->cfg_gpio) > + pdata->cfg_gpio(pdata->num_rows, pdata->num_cols); > + > + max_keymap_size = pdata->num_rows * pdata->num_cols; > + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); > + keycodes = devm_kzalloc(&pdev->dev, > + max_keymap_size * sizeof(*keycodes), GFP_KERNEL); > + input_dev = input_allocate_device(); > + if (!keypad || !keycodes || !input_dev) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + ret = -ENODEV; > + goto err_free_mem; > + } > + > + keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); > + if (!keypad->base) { > + ret = -EBUSY; > + goto err_free_mem; > + } > + > + if (pdata->clock) { > + keypad->clk = clk_get(&pdev->dev, pdata->clock); > + if (IS_ERR(keypad->clk)) { > + dev_err(&pdev->dev, "failed to get keypad clk\n"); > + ret = PTR_ERR(keypad->clk); > + goto err_unmap_base; > + } > + clk_enable(keypad->clk); > + } > + > + keypad->input_dev = input_dev; > + keypad->keycodes = keycodes; > + keypad->num_cols = pdata->num_cols; > + INIT_DELAYED_WORK(&keypad->work, samsung_kp_scan); > + > + /* enable interrupt and debouncing filter and wakeup bit */ > + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | > + SAMSUNG_WAKEUPEN; > + writel(val, keypad->base + SAMSUNG_KEYIFCON); > + > + keypad->irq = platform_get_irq(pdev, 0); > + if (keypad->irq < 0) { > + ret = keypad->irq; > + goto err_disable_clk; > + } > + > + ret = devm_request_irq(&pdev->dev, keypad->irq, > + samsung_kp_interrupt, > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, > + dev_name(&pdev->dev), keypad); > + > + if (ret) > + goto err_disable_clk; > + > + input_dev->name = pdev->name; > + input_dev->id.bustype = BUS_HOST; > + input_dev->dev.parent = &pdev->dev; > + input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP); > + > + input_dev->keycode = keycodes; > + input_dev->keycodesize = sizeof(*keycodes); > + input_dev->keycodemax = max_keymap_size; > + > + for (i = 0; i < pdata->keymap_size; i++) { > + unsigned int key = pdata->keymap[i]; > + unsigned int row = KEY_ROW(key); > + unsigned int col = KEY_COL(key); > + unsigned short code = KEY_VAL(key); > + unsigned int index = row * keypad->num_cols + col; > + > + keycodes[index] = code; > + set_bit(code, input_dev->keybit); > + } > + > + ret = input_register_device(keypad->input_dev); > + if (ret) > + goto err_free_irq; > + > + device_init_wakeup(&pdev->dev, 1); > + > + platform_set_drvdata(pdev, keypad); > + > + return 0; > + > +err_free_irq: > + devm_free_irq(&pdev->dev, keypad->irq, keypad); If you are using devres why do you release resources manually? > +err_disable_clk: > + if (keypad->clk) { > + clk_disable(keypad->clk); > + clk_put(keypad->clk); > + } > +err_unmap_base: > + devm_iounmap(&pdev->dev, keypad->base); > +err_free_mem: > + input_free_device(input_dev); > + devm_kfree(&pdev->dev, keycodes); > + devm_kfree(&pdev->dev, keypad); > + > + return ret; > +} > + > +static int __devexit samsung_kp_remove(struct platform_device *pdev) > +{ > + struct samsung_keypad *keypad = platform_get_drvdata(pdev); > + > + devm_free_irq(&pdev->dev, keypad->irq, keypad); > + cancel_delayed_work_sync(&keypad->work); Since you need tight control of the ordering between freeing IRQ and canceling the work you probably should not be using devres for IRQ allocation. > + > + /* > + * If work indeed has been cancelled, disable_irq() will have been left > + * unbalanced from samsung_kp_interrupt(). > + */ > + while (atomic_dec_return(&keypad->irq_disable) >= 0) > + enable_irq(keypad->irq); > + > + platform_set_drvdata(pdev, NULL); > + input_unregister_device(keypad->input_dev); > + > + if (keypad->clk) { > + clk_disable(keypad->clk); > + clk_put(keypad->clk); > + } > + > + devm_iounmap(&pdev->dev, keypad->base); > + devm_kfree(&pdev->dev, keypad->keycodes); > + devm_kfree(&pdev->dev, keypad); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int samsung_kp_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct samsung_keypad *keypad = platform_get_drvdata(pdev); > + > + disable_irq(keypad->irq); > + enable_irq_wake(keypad->irq); > + > + if (keypad->clk) > + clk_disable(keypad->clk); This should probaly gop into ->close() method. > + > + return 0; > +} > + > +static int samsung_kp_resume(struct platform_device *pdev) > +{ > + struct samsung_keypad *keypad = platform_get_drvdata(pdev); > + unsigned int val; > + > + if (keypad->clk) > + clk_enable(keypad->clk); > + > + /* enable interrupt and debouncing filter and wakeup bit */ > + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | > + SAMSUNG_WAKEUPEN; > + writel(val, keypad->base + SAMSUNG_KEYIFCON); > + > + disable_irq_wake(keypad->irq); > + enable_irq(keypad->irq); > + > + return 0; > +} > +#else > +#define samsung_kp_suspend NULL > +#define samsung_kp_resume NULL > +#endif > + > +static struct platform_driver samsung_kp_driver = { > + .probe = samsung_kp_probe, > + .remove = __devexit_p(samsung_kp_remove), > + .suspend = samsung_kp_suspend, > + .resume = samsung_kp_resume, Please switch to dev_pm_ops. > + .driver = { > + .name = "samsung-keypad", > + .owner = THIS_MODULE, > + }, > +}; > + > +static int __init samsung_kp_init(void) > +{ > + return platform_driver_register(&samsung_kp_driver); > +} > + > +static void __exit samsung_kp_exit(void) > +{ > + platform_driver_unregister(&samsung_kp_driver); > +} > + > +module_init(samsung_kp_init); > +module_exit(samsung_kp_exit); > + > +MODULE_DESCRIPTION("Samsung keypad driver"); > +MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>"); > +MODULE_AUTHOR("Jaehoon Chung <jh80.chung@samsung.com>"); > +MODULE_LICENSE("GPL"); > -- > 1.6.0.4
Hi, On 9/7/2009 4:48 PM, Dmitry Torokhov wrote: > Hi Joonyoung, > > On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote: >> + >> +static irqreturn_t samsung_kp_interrupt(int irq, void *dev_id) >> +{ >> + struct samsung_keypad *keypad = dev_id; >> + >> + if (!work_pending(&keypad->work.work)) { >> + disable_irq_nosync(keypad->irq); >> + atomic_inc(&keypad->irq_disable); >> + schedule_delayed_work(&keypad->work, msecs_to_jiffies(10)); > > Why do you need to have the delayed work? Can't we query the touchpad > state immediately? Or are you trying to implement debounce logic? Yes, debounce logic. Actually this is enough only the schedule_work. I'm not sure about the debounce yet. If use the debounce logic, i think the delay time should be moved into platform data. > Also, the driver seems to be using the edge-triggered interrupts; > do you really need to disable IRQ until you scan the keypad? > Yes, if the interrupt isn't disabled, when press keypad, many interrupt occur. > >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int __devinit samsung_kp_probe(struct platform_device *pdev) >> +{ >> + const struct samsung_keypad_platdata *pdata; >> + struct samsung_keypad *keypad; >> + struct resource *res; >> + struct input_dev *input_dev; >> + unsigned short *keycodes; >> + unsigned int max_keymap_size; >> + unsigned int val; >> + int i; >> + int ret; >> + >> + pdata = pdev->dev.platform_data; >> + if (!pdata) { >> + dev_err(&pdev->dev, "no platform data defined\n"); >> + return -EINVAL; >> + } >> + >> + if ((pdata->num_rows <= 0) || (pdata->num_rows > SAMSUNG_MAX_ROWS)) >> + return -EINVAL; >> + >> + if ((pdata->num_cols <= 0) || (pdata->num_cols > SAMSUNG_MAX_COLS)) >> + return -EINVAL; >> + >> + /* initialize the gpio */ >> + if (pdata->cfg_gpio) >> + pdata->cfg_gpio(pdata->num_rows, pdata->num_cols); >> + >> + max_keymap_size = pdata->num_rows * pdata->num_cols; >> + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); >> + keycodes = devm_kzalloc(&pdev->dev, >> + max_keymap_size * sizeof(*keycodes), GFP_KERNEL); >> + input_dev = input_allocate_device(); >> + if (!keypad || !keycodes || !input_dev) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + ret = -ENODEV; >> + goto err_free_mem; >> + } >> + >> + keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); >> + if (!keypad->base) { >> + ret = -EBUSY; >> + goto err_free_mem; >> + } >> + >> + if (pdata->clock) { >> + keypad->clk = clk_get(&pdev->dev, pdata->clock); >> + if (IS_ERR(keypad->clk)) { >> + dev_err(&pdev->dev, "failed to get keypad clk\n"); >> + ret = PTR_ERR(keypad->clk); >> + goto err_unmap_base; >> + } >> + clk_enable(keypad->clk); >> + } >> + >> + keypad->input_dev = input_dev; >> + keypad->keycodes = keycodes; >> + keypad->num_cols = pdata->num_cols; >> + INIT_DELAYED_WORK(&keypad->work, samsung_kp_scan); >> + >> + /* enable interrupt and debouncing filter and wakeup bit */ >> + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | >> + SAMSUNG_WAKEUPEN; >> + writel(val, keypad->base + SAMSUNG_KEYIFCON); >> + >> + keypad->irq = platform_get_irq(pdev, 0); >> + if (keypad->irq < 0) { >> + ret = keypad->irq; >> + goto err_disable_clk; >> + } >> + >> + ret = devm_request_irq(&pdev->dev, keypad->irq, >> + samsung_kp_interrupt, >> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, >> + dev_name(&pdev->dev), keypad); >> + >> + if (ret) >> + goto err_disable_clk; >> + >> + input_dev->name = pdev->name; >> + input_dev->id.bustype = BUS_HOST; >> + input_dev->dev.parent = &pdev->dev; >> + input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP); >> + >> + input_dev->keycode = keycodes; >> + input_dev->keycodesize = sizeof(*keycodes); >> + input_dev->keycodemax = max_keymap_size; >> + >> + for (i = 0; i < pdata->keymap_size; i++) { >> + unsigned int key = pdata->keymap[i]; >> + unsigned int row = KEY_ROW(key); >> + unsigned int col = KEY_COL(key); >> + unsigned short code = KEY_VAL(key); >> + unsigned int index = row * keypad->num_cols + col; >> + >> + keycodes[index] = code; >> + set_bit(code, input_dev->keybit); >> + } >> + >> + ret = input_register_device(keypad->input_dev); >> + if (ret) >> + goto err_free_irq; >> + >> + device_init_wakeup(&pdev->dev, 1); >> + >> + platform_set_drvdata(pdev, keypad); >> + >> + return 0; >> + >> +err_free_irq: >> + devm_free_irq(&pdev->dev, keypad->irq, keypad); > > > If you are using devres why do you release resources manually? > I wonder the irq is released automatically if we don't use devm_free_irq here. >> +err_disable_clk: >> + if (keypad->clk) { >> + clk_disable(keypad->clk); >> + clk_put(keypad->clk); >> + } >> +err_unmap_base: >> + devm_iounmap(&pdev->dev, keypad->base); >> +err_free_mem: >> + input_free_device(input_dev); >> + devm_kfree(&pdev->dev, keycodes); >> + devm_kfree(&pdev->dev, keypad); >> + >> + return ret; >> +} >> + >> +static int __devexit samsung_kp_remove(struct platform_device *pdev) >> +{ >> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); >> + >> + devm_free_irq(&pdev->dev, keypad->irq, keypad); >> + cancel_delayed_work_sync(&keypad->work); > > Since you need tight control of the ordering between freeing IRQ and > canceling the work you probably should not be using devres for IRQ > allocation. > Hmm, i'm not sure about this, but i can change it not to use devres for IRQ allocation. >> + >> + /* >> + * If work indeed has been cancelled, disable_irq() will have been left >> + * unbalanced from samsung_kp_interrupt(). >> + */ >> + while (atomic_dec_return(&keypad->irq_disable) >= 0) >> + enable_irq(keypad->irq); >> + >> + platform_set_drvdata(pdev, NULL); >> + input_unregister_device(keypad->input_dev); >> + >> + if (keypad->clk) { >> + clk_disable(keypad->clk); >> + clk_put(keypad->clk); >> + } >> + >> + devm_iounmap(&pdev->dev, keypad->base); >> + devm_kfree(&pdev->dev, keypad->keycodes); >> + devm_kfree(&pdev->dev, keypad); >> + >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM >> +static int samsung_kp_suspend(struct platform_device *pdev, pm_message_t state) >> +{ >> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); >> + >> + disable_irq(keypad->irq); >> + enable_irq_wake(keypad->irq); >> + >> + if (keypad->clk) >> + clk_disable(keypad->clk); > > This should probaly gop into ->close() method. > This driver doesn't use open() and close method. Why should move into ->close()? >> + >> + return 0; >> +} >> + >> +static int samsung_kp_resume(struct platform_device *pdev) >> +{ >> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); >> + unsigned int val; >> + >> + if (keypad->clk) >> + clk_enable(keypad->clk); >> + >> + /* enable interrupt and debouncing filter and wakeup bit */ >> + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | >> + SAMSUNG_WAKEUPEN; >> + writel(val, keypad->base + SAMSUNG_KEYIFCON); >> + >> + disable_irq_wake(keypad->irq); >> + enable_irq(keypad->irq); >> + >> + return 0; >> +} >> +#else >> +#define samsung_kp_suspend NULL >> +#define samsung_kp_resume NULL >> +#endif >> + >> +static struct platform_driver samsung_kp_driver = { >> + .probe = samsung_kp_probe, >> + .remove = __devexit_p(samsung_kp_remove), >> + .suspend = samsung_kp_suspend, >> + .resume = samsung_kp_resume, > > Please switch to dev_pm_ops. > OK. >> + .driver = { >> + .name = "samsung-keypad", >> + .owner = THIS_MODULE, >> + }, >> +}; >> + >> +static int __init samsung_kp_init(void) >> +{ >> + return platform_driver_register(&samsung_kp_driver); >> +} >> + >> +static void __exit samsung_kp_exit(void) >> +{ >> + platform_driver_unregister(&samsung_kp_driver); >> +} >> + >> +module_init(samsung_kp_init); >> +module_exit(samsung_kp_exit); >> + >> +MODULE_DESCRIPTION("Samsung keypad driver"); >> +MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>"); >> +MODULE_AUTHOR("Jaehoon Chung <jh80.chung@samsung.com>"); >> +MODULE_LICENSE("GPL"); >> -- >> 1.6.0.4 > Thanks for review. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Mr.Shim > > submitted mainline. This is what Jinsun was starting, and which he > > will continue until it is included mainline. > > > > The keypad of s3c64xx and s5pc1xx is same almost, so we need to submit > the well-defined driver from the first. I have some questions: 1) Could you explain to us what is the 'well-defined' driver? 2) Did you test your keypad driver at s3c6410 based platform? 3) There is no architecture codes for s5pc1xx series, why did you send keypad driver first for s5pc1xx before architecture codes? Best Regards -- Jinsung, Yang <jsgood.yang@samsung.com> AP Development Team System LSI, Semiconductor Business SAMSUNG Electronics Co., LTD -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 9/7/2009 8:32 PM, Jinsung Yang wrote: > Hi, Mr.Shim > >>> submitted mainline. This is what Jinsun was starting, and which he >>> will continue until it is included mainline. >>> >> The keypad of s3c64xx and s5pc1xx is same almost, so we need to submit >> the well-defined driver from the first. > > I have some questions: > 1) Could you explain to us what is the 'well-defined' driver? I mean the driver to support three cpu and various target in one keypad driver, but i think that your posted driver seems for only SMDK6410. Also, we can make better driver via the review. > 2) Did you test your keypad driver at s3c6410 based platform? No, i cannot test it on s3c64xx because i don't have a target using the keypad of s3c64xx such SMDK6410 but i tested the keypad driver on s5pc100 and s5pc110 and as you know, the keypad of the s3c6410 and s5pc100 datasheet is same almost, so i think it will operate on s3c64xx. > 3) There is no architecture codes for s5pc1xx series, why did you send keypad driver first for s5pc1xx before architecture codes? > Of course, I can post the arch code for keypad, but it is the common code for s3c64xx and s5pc1xx and we didn't post s5pc1xx arch code yet on ML. I also think it is better to post the keypad driver after posting arch. > Best Regards > -- > Jinsung, Yang <jsgood.yang@samsung.com> > AP Development Team > System LSI, Semiconductor Business > SAMSUNG Electronics Co., LTD > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > I have some questions: > > 1) Could you explain to us what is the 'well-defined' driver? > > I mean the driver to support three cpu and various target in one keypad > driver, but i think that your posted driver seems for only SMDK6410. > Also, we can make better driver via the review. > Why do you think my driver cannot support various target? I think there are misunderstandings, are there any architecture specific codes? Surely, there can be something wrong driver-specific codes such as 'dev_pm_ops', but that's not a big deal.. we can modify it with mainline feedbacks. My driver codes also have considered for our s5pc100, s5pc110, and many Samsung SoCs. We will support other SoCs with adding some architecture definitions (not changing driver codes), but just 'NOT NOW'. Please understand, 'step by step'. Of course we aware now our SoCs line-ups better than you, we are not happy to hear from you. Finally, in your driver, regarding the if (cpu_is_s5pc100()) statement, how do you think if you have to check another SoC at some time in the future? Doing like this if (cpu_is_s5pc110()) else if (cpu_is_s5pc120()) else if (cpu_is_s5pc130()) ?? Will you send some patches for this? looks be not good. > > 2) Did you test your keypad driver at s3c6410 based platform? > > No, i cannot test it on s3c64xx because i don't have a target using the > keypad of s3c64xx such SMDK6410 but i tested the keypad driver on > s5pc100 and s5pc110 and as you know, the keypad of the s3c6410 and > s5pc100 datasheet is same almost, so i think it will operate on s3c64xx. > > > 3) There is no architecture codes for s5pc1xx series, why did you send > keypad driver first for s5pc1xx before architecture codes? > > > > Of course, I can post the arch code for keypad, but it is the common > code for s3c64xx and s5pc1xx and we didn't post s5pc1xx arch code yet on > ML. I also think it is better to post the keypad driver after posting > arch. > Best Regards -- Jinsung, Yang <jsgood.yang@samsung.com> AP Development Team System LSI, Semiconductor Business SAMSUNG Electronics Co., LTD -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 7, 2009 at 9:38 PM, Jinsung Yang<jsgood.yang@samsung.com> wrote: >> > I have some questions: >> > 1) Could you explain to us what is the 'well-defined' driver? >> >> I mean the driver to support three cpu and various target in one keypad >> driver, but i think that your posted driver seems for only SMDK6410. >> Also, we can make better driver via the review. >> > > Why do you think my driver cannot support various target? > I think there are misunderstandings, are there any architecture specific codes? > Surely, there can be something wrong driver-specific codes such as 'dev_pm_ops', > but that's not a big deal.. we can modify it with mainline feedbacks. > My driver codes also have considered for our s5pc100, s5pc110, and many Samsung SoCs. > We will support other SoCs with adding some architecture definitions (not changing driver codes), but just 'NOT NOW'. > Please understand, 'step by step'. > Of course we aware now our SoCs line-ups better than you, we are not happy to hear from you. > > Finally, in your driver, regarding the if (cpu_is_s5pc100()) statement, > how do you think if you have to check another SoC at some time in the future? > Doing like this if (cpu_is_s5pc110()) else if (cpu_is_s5pc120()) else if (cpu_is_s5pc130()) ?? > Will you send some patches for this? looks be not good. Then question. If next new chips has different field but has same offset or address. In this case how you handle it? E.g., How to use handle s5pc100 and s5pc110 simultaneously at one driver without re-compile? BR, Kyungmin Park -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 7, 2009 at 10:14 PM, Kyungmin Park<kmpark@infradead.org> wrote: > On Mon, Sep 7, 2009 at 9:38 PM, Jinsung Yang<jsgood.yang@samsung.com> wrote: >>> > I have some questions: >>> > 1) Could you explain to us what is the 'well-defined' driver? >>> >>> I mean the driver to support three cpu and various target in one keypad >>> driver, but i think that your posted driver seems for only SMDK6410. >>> Also, we can make better driver via the review. >>> >> >> Why do you think my driver cannot support various target? >> I think there are misunderstandings, are there any architecture specific codes? >> Surely, there can be something wrong driver-specific codes such as 'dev_pm_ops', >> but that's not a big deal.. we can modify it with mainline feedbacks. >> My driver codes also have considered for our s5pc100, s5pc110, and many Samsung SoCs. >> We will support other SoCs with adding some architecture definitions (not changing driver codes), but just 'NOT NOW'. >> Please understand, 'step by step'. >> Of course we aware now our SoCs line-ups better than you, we are not happy to hear from you. >> >> Finally, in your driver, regarding the if (cpu_is_s5pc100()) statement, >> how do you think if you have to check another SoC at some time in the future? >> Doing like this if (cpu_is_s5pc110()) else if (cpu_is_s5pc120()) else if (cpu_is_s5pc130()) ?? >> Will you send some patches for this? looks be not good. > > Then question. If next new chips has different field but has same > offset or address. In this case how you handle it? > E.g., How to use handle s5pc100 and s5pc110 simultaneously at one > driver without re-compile? Just FYI: please see the other arch, OMAP. and how to use it. http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/plat-omap/include/mach/cpu.h;h=7a5f9e882e54b140d87b1a4648f130366c102f69;hb=HEAD Thank you, Kyungmin Park -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/9/7 Jinsung Yang <jsgood.yang@samsung.com>: >> > I have some questions: >> > 1) Could you explain to us what is the 'well-defined' driver? >> >> I mean the driver to support three cpu and various target in one keypad >> driver, but i think that your posted driver seems for only SMDK6410. >> Also, we can make better driver via the review. >> > > Why do you think my driver cannot support various target? > I think there are misunderstandings, are there any architecture specific codes? For example, there is the keycodes. My target uses other keycodes. > Surely, there can be something wrong driver-specific codes such as 'dev_pm_ops', > but that's not a big deal.. we can modify it with mainline feedbacks. > My driver codes also have considered for our s5pc100, s5pc110, and many Samsung SoCs. > We will support other SoCs with adding some architecture definitions (not changing driver codes), but just 'NOT NOW'. OK, but It needs to implement considering not only s3c64xx but also s5pc1xx from first, and i think it's better to use "samsung" prefix. > Please understand, 'step by step'. > Of course we aware now our SoCs line-ups better than you, we are not happy to hear from you. I know but we need many discussion to merge the better code in kernel. I welcome it whenever. > > Finally, in your driver, regarding the if (cpu_is_s5pc100()) statement, > how do you think if you have to check another SoC at some time in the future? > Doing like this if (cpu_is_s5pc110()) else if (cpu_is_s5pc120()) else if (cpu_is_s5pc130()) ?? > Will you send some patches for this? looks be not good. I used only cpu_is_s5pc110(), also you can refer the omap keypad driver. It is a good example to support many omap arch > >> > 2) Did you test your keypad driver at s3c6410 based platform? >> >> No, i cannot test it on s3c64xx because i don't have a target using the >> keypad of s3c64xx such SMDK6410 but i tested the keypad driver on >> s5pc100 and s5pc110 and as you know, the keypad of the s3c6410 and >> s5pc100 datasheet is same almost, so i think it will operate on s3c64xx. >> >> > 3) There is no architecture codes for s5pc1xx series, why did you send >> keypad driver first for s5pc1xx before architecture codes? >> > >> >> Of course, I can post the arch code for keypad, but it is the common >> code for s3c64xx and s5pc1xx and we didn't post s5pc1xx arch code yet on >> ML. I also think it is better to post the keypad driver after posting >> arch. >> > > Best Regards > -- > Jinsung, Yang <jsgood.yang@samsung.com> > AP Development Team > System LSI, Semiconductor Business > SAMSUNG Electronics Co., LTD > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at  http://vger.kernel.org/majordomo-info.html >
On Mon, Sep 07, 2009 at 05:40:44PM +0900, Joonyoung Shim wrote: > Hi, > > On 9/7/2009 4:48 PM, Dmitry Torokhov wrote: > > Hi Joonyoung, > > > > On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote: > >> + > >> +static irqreturn_t samsung_kp_interrupt(int irq, void *dev_id) > >> +{ > >> + struct samsung_keypad *keypad = dev_id; > >> + > >> + if (!work_pending(&keypad->work.work)) { > >> + disable_irq_nosync(keypad->irq); > >> + atomic_inc(&keypad->irq_disable); > >> + schedule_delayed_work(&keypad->work, msecs_to_jiffies(10)); > > > > Why do you need to have the delayed work? Can't we query the touchpad > > state immediately? Or are you trying to implement debounce logic? > > Yes, debounce logic. Actually this is enough only the schedule_work. I'm > not sure about the debounce yet. If use the debounce logic, i think the > delay time should be moved into platform data. > I don't see how debounce would work if you disable interrupts. You don't want to just delay the read, you want to perform the read after you stopped receiving interrupts for certain period of time. > > Also, the driver seems to be using the edge-triggered interrupts; > > do you really need to disable IRQ until you scan the keypad? > > > > Yes, if the interrupt isn't disabled, when press keypad, many interrupt > occur. > Hmm, that is the common case with level-triggered interrupts, still here you have edge-triggered... > > > >> + } > >> + > >> + return IRQ_HANDLED; > >> +} > >> + > >> +static int __devinit samsung_kp_probe(struct platform_device *pdev) > >> +{ > >> + const struct samsung_keypad_platdata *pdata; > >> + struct samsung_keypad *keypad; > >> + struct resource *res; > >> + struct input_dev *input_dev; > >> + unsigned short *keycodes; > >> + unsigned int max_keymap_size; > >> + unsigned int val; > >> + int i; > >> + int ret; > >> + > >> + pdata = pdev->dev.platform_data; > >> + if (!pdata) { > >> + dev_err(&pdev->dev, "no platform data defined\n"); > >> + return -EINVAL; > >> + } > >> + > >> + if ((pdata->num_rows <= 0) || (pdata->num_rows > SAMSUNG_MAX_ROWS)) > >> + return -EINVAL; > >> + > >> + if ((pdata->num_cols <= 0) || (pdata->num_cols > SAMSUNG_MAX_COLS)) > >> + return -EINVAL; > >> + > >> + /* initialize the gpio */ > >> + if (pdata->cfg_gpio) > >> + pdata->cfg_gpio(pdata->num_rows, pdata->num_cols); > >> + > >> + max_keymap_size = pdata->num_rows * pdata->num_cols; > >> + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); > >> + keycodes = devm_kzalloc(&pdev->dev, > >> + max_keymap_size * sizeof(*keycodes), GFP_KERNEL); > >> + input_dev = input_allocate_device(); > >> + if (!keypad || !keycodes || !input_dev) > >> + return -ENOMEM; > >> + > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> + if (!res) { > >> + ret = -ENODEV; > >> + goto err_free_mem; > >> + } > >> + > >> + keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); > >> + if (!keypad->base) { > >> + ret = -EBUSY; > >> + goto err_free_mem; > >> + } > >> + > >> + if (pdata->clock) { > >> + keypad->clk = clk_get(&pdev->dev, pdata->clock); > >> + if (IS_ERR(keypad->clk)) { > >> + dev_err(&pdev->dev, "failed to get keypad clk\n"); > >> + ret = PTR_ERR(keypad->clk); > >> + goto err_unmap_base; > >> + } > >> + clk_enable(keypad->clk); > >> + } > >> + > >> + keypad->input_dev = input_dev; > >> + keypad->keycodes = keycodes; > >> + keypad->num_cols = pdata->num_cols; > >> + INIT_DELAYED_WORK(&keypad->work, samsung_kp_scan); > >> + > >> + /* enable interrupt and debouncing filter and wakeup bit */ > >> + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | > >> + SAMSUNG_WAKEUPEN; > >> + writel(val, keypad->base + SAMSUNG_KEYIFCON); > >> + > >> + keypad->irq = platform_get_irq(pdev, 0); > >> + if (keypad->irq < 0) { > >> + ret = keypad->irq; > >> + goto err_disable_clk; > >> + } > >> + > >> + ret = devm_request_irq(&pdev->dev, keypad->irq, > >> + samsung_kp_interrupt, > >> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, > >> + dev_name(&pdev->dev), keypad); > >> + > >> + if (ret) > >> + goto err_disable_clk; > >> + > >> + input_dev->name = pdev->name; > >> + input_dev->id.bustype = BUS_HOST; > >> + input_dev->dev.parent = &pdev->dev; > >> + input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP); > >> + > >> + input_dev->keycode = keycodes; > >> + input_dev->keycodesize = sizeof(*keycodes); > >> + input_dev->keycodemax = max_keymap_size; > >> + > >> + for (i = 0; i < pdata->keymap_size; i++) { > >> + unsigned int key = pdata->keymap[i]; > >> + unsigned int row = KEY_ROW(key); > >> + unsigned int col = KEY_COL(key); > >> + unsigned short code = KEY_VAL(key); > >> + unsigned int index = row * keypad->num_cols + col; > >> + > >> + keycodes[index] = code; > >> + set_bit(code, input_dev->keybit); > >> + } > >> + > >> + ret = input_register_device(keypad->input_dev); > >> + if (ret) > >> + goto err_free_irq; > >> + > >> + device_init_wakeup(&pdev->dev, 1); > >> + > >> + platform_set_drvdata(pdev, keypad); > >> + > >> + return 0; > >> + > >> +err_free_irq: > >> + devm_free_irq(&pdev->dev, keypad->irq, keypad); > > > > > > If you are using devres why do you release resources manually? > > > > I wonder the irq is released automatically if we don't use devm_free_irq > here. So far I fail to see why you bother with using devres if you are managing all resources on your own... > > >> +err_disable_clk: > >> + if (keypad->clk) { > >> + clk_disable(keypad->clk); > >> + clk_put(keypad->clk); > >> + } > >> +err_unmap_base: > >> + devm_iounmap(&pdev->dev, keypad->base); > >> +err_free_mem: > >> + input_free_device(input_dev); > >> + devm_kfree(&pdev->dev, keycodes); > >> + devm_kfree(&pdev->dev, keypad); > >> + > >> + return ret; > >> +} > >> + > >> +static int __devexit samsung_kp_remove(struct platform_device *pdev) > >> +{ > >> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); > >> + > >> + devm_free_irq(&pdev->dev, keypad->irq, keypad); > >> + cancel_delayed_work_sync(&keypad->work); > > > > Since you need tight control of the ordering between freeing IRQ and > > canceling the work you probably should not be using devres for IRQ > > allocation. > > > > Hmm, i'm not sure about this, but i can change it not to use devres for > IRQ allocation. > > >> + > >> + /* > >> + * If work indeed has been cancelled, disable_irq() will have been left > >> + * unbalanced from samsung_kp_interrupt(). > >> + */ > >> + while (atomic_dec_return(&keypad->irq_disable) >= 0) > >> + enable_irq(keypad->irq); > >> + > >> + platform_set_drvdata(pdev, NULL); > >> + input_unregister_device(keypad->input_dev); > >> + > >> + if (keypad->clk) { > >> + clk_disable(keypad->clk); > >> + clk_put(keypad->clk); > >> + } > >> + > >> + devm_iounmap(&pdev->dev, keypad->base); > >> + devm_kfree(&pdev->dev, keypad->keycodes); > >> + devm_kfree(&pdev->dev, keypad); > >> + > >> + return 0; > >> +} > >> + > >> +#ifdef CONFIG_PM > >> +static int samsung_kp_suspend(struct platform_device *pdev, pm_message_t state) > >> +{ > >> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); > >> + > >> + disable_irq(keypad->irq); > >> + enable_irq_wake(keypad->irq); > >> + > >> + if (keypad->clk) > >> + clk_disable(keypad->clk); > > > > This should probaly gop into ->close() method. > > > > This driver doesn't use open() and close method. Why should move into > ->close()? > It was a suggestion to implement them ;) Although the comment should have been a few lines above (in remove method). > >> + > >> + return 0; > >> +} > >> + > >> +static int samsung_kp_resume(struct platform_device *pdev) > >> +{ > >> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); > >> + unsigned int val; > >> + > >> + if (keypad->clk) > >> + clk_enable(keypad->clk); > >> + > >> + /* enable interrupt and debouncing filter and wakeup bit */ > >> + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | > >> + SAMSUNG_WAKEUPEN; > >> + writel(val, keypad->base + SAMSUNG_KEYIFCON); > >> + > >> + disable_irq_wake(keypad->irq); > >> + enable_irq(keypad->irq); > >> + > >> + return 0; > >> +} > >> +#else > >> +#define samsung_kp_suspend NULL > >> +#define samsung_kp_resume NULL > >> +#endif > >> + > >> +static struct platform_driver samsung_kp_driver = { > >> + .probe = samsung_kp_probe, > >> + .remove = __devexit_p(samsung_kp_remove), > >> + .suspend = samsung_kp_suspend, > >> + .resume = samsung_kp_resume, > > > > Please switch to dev_pm_ops. > > > > OK. > > >> + .driver = { > >> + .name = "samsung-keypad", > >> + .owner = THIS_MODULE, > >> + }, > >> +}; > >> + > >> +static int __init samsung_kp_init(void) > >> +{ > >> + return platform_driver_register(&samsung_kp_driver); > >> +} > >> + > >> +static void __exit samsung_kp_exit(void) > >> +{ > >> + platform_driver_unregister(&samsung_kp_driver); > >> +} > >> + > >> +module_init(samsung_kp_init); > >> +module_exit(samsung_kp_exit); > >> + > >> +MODULE_DESCRIPTION("Samsung keypad driver"); > >> +MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>"); > >> +MODULE_AUTHOR("Jaehoon Chung <jh80.chung@samsung.com>"); > >> +MODULE_LICENSE("GPL"); > >> -- > >> 1.6.0.4 > > > > Thanks for review. >
On 9/8/2009 1:44 PM, Dmitry Torokhov wrote: > On Mon, Sep 07, 2009 at 05:40:44PM +0900, Joonyoung Shim wrote: >> Hi, >> >> On 9/7/2009 4:48 PM, Dmitry Torokhov wrote: >>> Hi Joonyoung, >>> >>> On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote: >>>> + >>>> +static irqreturn_t samsung_kp_interrupt(int irq, void *dev_id) >>>> +{ >>>> + struct samsung_keypad *keypad = dev_id; >>>> + >>>> + if (!work_pending(&keypad->work.work)) { >>>> + disable_irq_nosync(keypad->irq); >>>> + atomic_inc(&keypad->irq_disable); >>>> + schedule_delayed_work(&keypad->work, msecs_to_jiffies(10)); >>> Why do you need to have the delayed work? Can't we query the touchpad >>> state immediately? Or are you trying to implement debounce logic? >> Yes, debounce logic. Actually this is enough only the schedule_work. I'm >> not sure about the debounce yet. If use the debounce logic, i think the >> delay time should be moved into platform data. >> > > I don't see how debounce would work if you disable interrupts. You don't > want to just delay the read, you want to perform the read after you > stopped receiving interrupts for certain period of time. > I didn't understand completely about debounce of the samsung keypad. First, i will remove about debounce on initial driver, and will add it later. >>> Also, the driver seems to be using the edge-triggered interrupts; >>> do you really need to disable IRQ until you scan the keypad? >>> >> Yes, if the interrupt isn't disabled, when press keypad, many interrupt >> occur. >> > > Hmm, that is the common case with level-triggered interrupts, still here > you have edge-triggered... > You are right. The low level-triggered interrupt occurs and i missed it on datasheet. >>>> + } >>>> + >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static int __devinit samsung_kp_probe(struct platform_device *pdev) >>>> +{ >>>> + const struct samsung_keypad_platdata *pdata; >>>> + struct samsung_keypad *keypad; >>>> + struct resource *res; >>>> + struct input_dev *input_dev; >>>> + unsigned short *keycodes; >>>> + unsigned int max_keymap_size; >>>> + unsigned int val; >>>> + int i; >>>> + int ret; >>>> + >>>> + pdata = pdev->dev.platform_data; >>>> + if (!pdata) { >>>> + dev_err(&pdev->dev, "no platform data defined\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if ((pdata->num_rows <= 0) || (pdata->num_rows > SAMSUNG_MAX_ROWS)) >>>> + return -EINVAL; >>>> + >>>> + if ((pdata->num_cols <= 0) || (pdata->num_cols > SAMSUNG_MAX_COLS)) >>>> + return -EINVAL; >>>> + >>>> + /* initialize the gpio */ >>>> + if (pdata->cfg_gpio) >>>> + pdata->cfg_gpio(pdata->num_rows, pdata->num_cols); >>>> + >>>> + max_keymap_size = pdata->num_rows * pdata->num_cols; >>>> + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); >>>> + keycodes = devm_kzalloc(&pdev->dev, >>>> + max_keymap_size * sizeof(*keycodes), GFP_KERNEL); >>>> + input_dev = input_allocate_device(); >>>> + if (!keypad || !keycodes || !input_dev) >>>> + return -ENOMEM; >>>> + >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + if (!res) { >>>> + ret = -ENODEV; >>>> + goto err_free_mem; >>>> + } >>>> + >>>> + keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); >>>> + if (!keypad->base) { >>>> + ret = -EBUSY; >>>> + goto err_free_mem; >>>> + } >>>> + >>>> + if (pdata->clock) { >>>> + keypad->clk = clk_get(&pdev->dev, pdata->clock); >>>> + if (IS_ERR(keypad->clk)) { >>>> + dev_err(&pdev->dev, "failed to get keypad clk\n"); >>>> + ret = PTR_ERR(keypad->clk); >>>> + goto err_unmap_base; >>>> + } >>>> + clk_enable(keypad->clk); >>>> + } >>>> + >>>> + keypad->input_dev = input_dev; >>>> + keypad->keycodes = keycodes; >>>> + keypad->num_cols = pdata->num_cols; >>>> + INIT_DELAYED_WORK(&keypad->work, samsung_kp_scan); >>>> + >>>> + /* enable interrupt and debouncing filter and wakeup bit */ >>>> + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | >>>> + SAMSUNG_WAKEUPEN; >>>> + writel(val, keypad->base + SAMSUNG_KEYIFCON); >>>> + >>>> + keypad->irq = platform_get_irq(pdev, 0); >>>> + if (keypad->irq < 0) { >>>> + ret = keypad->irq; >>>> + goto err_disable_clk; >>>> + } >>>> + >>>> + ret = devm_request_irq(&pdev->dev, keypad->irq, >>>> + samsung_kp_interrupt, >>>> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, >>>> + dev_name(&pdev->dev), keypad); >>>> + >>>> + if (ret) >>>> + goto err_disable_clk; >>>> + >>>> + input_dev->name = pdev->name; >>>> + input_dev->id.bustype = BUS_HOST; >>>> + input_dev->dev.parent = &pdev->dev; >>>> + input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP); >>>> + >>>> + input_dev->keycode = keycodes; >>>> + input_dev->keycodesize = sizeof(*keycodes); >>>> + input_dev->keycodemax = max_keymap_size; >>>> + >>>> + for (i = 0; i < pdata->keymap_size; i++) { >>>> + unsigned int key = pdata->keymap[i]; >>>> + unsigned int row = KEY_ROW(key); >>>> + unsigned int col = KEY_COL(key); >>>> + unsigned short code = KEY_VAL(key); >>>> + unsigned int index = row * keypad->num_cols + col; >>>> + >>>> + keycodes[index] = code; >>>> + set_bit(code, input_dev->keybit); >>>> + } >>>> + >>>> + ret = input_register_device(keypad->input_dev); >>>> + if (ret) >>>> + goto err_free_irq; >>>> + >>>> + device_init_wakeup(&pdev->dev, 1); >>>> + >>>> + platform_set_drvdata(pdev, keypad); >>>> + >>>> + return 0; >>>> + >>>> +err_free_irq: >>>> + devm_free_irq(&pdev->dev, keypad->irq, keypad); >>> >>> If you are using devres why do you release resources manually? >>> >> I wonder the irq is released automatically if we don't use devm_free_irq >> here. > > So far I fail to see why you bother with using devres if you are managing > all resources on your own... > OK, this is my fault. I understanded again about devres. I will change to common functions instead of using devres to manage resources on driver. >>>> +err_disable_clk: >>>> + if (keypad->clk) { >>>> + clk_disable(keypad->clk); >>>> + clk_put(keypad->clk); >>>> + } >>>> +err_unmap_base: >>>> + devm_iounmap(&pdev->dev, keypad->base); >>>> +err_free_mem: >>>> + input_free_device(input_dev); >>>> + devm_kfree(&pdev->dev, keycodes); >>>> + devm_kfree(&pdev->dev, keypad); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int __devexit samsung_kp_remove(struct platform_device *pdev) >>>> +{ >>>> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); >>>> + >>>> + devm_free_irq(&pdev->dev, keypad->irq, keypad); >>>> + cancel_delayed_work_sync(&keypad->work); >>> Since you need tight control of the ordering between freeing IRQ and >>> canceling the work you probably should not be using devres for IRQ >>> allocation. >>> >> Hmm, i'm not sure about this, but i can change it not to use devres for >> IRQ allocation. >> >>>> + >>>> + /* >>>> + * If work indeed has been cancelled, disable_irq() will have been left >>>> + * unbalanced from samsung_kp_interrupt(). >>>> + */ >>>> + while (atomic_dec_return(&keypad->irq_disable) >= 0) >>>> + enable_irq(keypad->irq); >>>> + >>>> + platform_set_drvdata(pdev, NULL); >>>> + input_unregister_device(keypad->input_dev); >>>> + >>>> + if (keypad->clk) { >>>> + clk_disable(keypad->clk); >>>> + clk_put(keypad->clk); >>>> + } >>>> + >>>> + devm_iounmap(&pdev->dev, keypad->base); >>>> + devm_kfree(&pdev->dev, keypad->keycodes); >>>> + devm_kfree(&pdev->dev, keypad); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +#ifdef CONFIG_PM >>>> +static int samsung_kp_suspend(struct platform_device *pdev, pm_message_t state) >>>> +{ >>>> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); >>>> + >>>> + disable_irq(keypad->irq); >>>> + enable_irq_wake(keypad->irq); >>>> + >>>> + if (keypad->clk) >>>> + clk_disable(keypad->clk); >>> This should probaly gop into ->close() method. >>> >> This driver doesn't use open() and close method. Why should move into >> ->close()? >> > > It was a suggestion to implement them ;) Although the comment should > have been a few lines above (in remove method). > I will remove clk_enable and clk_disable in suspend and resume function. >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int samsung_kp_resume(struct platform_device *pdev) >>>> +{ >>>> + struct samsung_keypad *keypad = platform_get_drvdata(pdev); >>>> + unsigned int val; >>>> + >>>> + if (keypad->clk) >>>> + clk_enable(keypad->clk); >>>> + >>>> + /* enable interrupt and debouncing filter and wakeup bit */ >>>> + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | >>>> + SAMSUNG_WAKEUPEN; >>>> + writel(val, keypad->base + SAMSUNG_KEYIFCON); >>>> + >>>> + disable_irq_wake(keypad->irq); >>>> + enable_irq(keypad->irq); >>>> + >>>> + return 0; >>>> +} >>>> +#else >>>> +#define samsung_kp_suspend NULL >>>> +#define samsung_kp_resume NULL >>>> +#endif >>>> + >>>> +static struct platform_driver samsung_kp_driver = { >>>> + .probe = samsung_kp_probe, >>>> + .remove = __devexit_p(samsung_kp_remove), >>>> + .suspend = samsung_kp_suspend, >>>> + .resume = samsung_kp_resume, >>> Please switch to dev_pm_ops. >>> >> OK. >> >>>> + .driver = { >>>> + .name = "samsung-keypad", >>>> + .owner = THIS_MODULE, >>>> + }, >>>> +}; >>>> + >>>> +static int __init samsung_kp_init(void) >>>> +{ >>>> + return platform_driver_register(&samsung_kp_driver); >>>> +} >>>> + >>>> +static void __exit samsung_kp_exit(void) >>>> +{ >>>> + platform_driver_unregister(&samsung_kp_driver); >>>> +} >>>> + >>>> +module_init(samsung_kp_init); >>>> +module_exit(samsung_kp_exit); >>>> + >>>> +MODULE_DESCRIPTION("Samsung keypad driver"); >>>> +MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>"); >>>> +MODULE_AUTHOR("Jaehoon Chung <jh80.chung@samsung.com>"); >>>> +MODULE_LICENSE("GPL"); >>>> -- >>>> 1.6.0.4 >> Thanks for review. >> > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig index 3525c19..b2cec96 100644 --- a/drivers/input/keyboard/Kconfig +++ b/drivers/input/keyboard/Kconfig @@ -278,6 +278,15 @@ config KEYBOARD_PXA930_ROTARY To compile this driver as a module, choose M here: the module will be called pxa930_rotary. +config KEYBOARD_SAMSUNG + tristate "Samsung keypad support" + depends on ARCH_S3C64XX || ARCH_S5PC1XX + help + Say Y here if you want to use the Samsung keypad. + + To compile this driver as a module, choose M here: the + module will be called samsung-keypad. + config KEYBOARD_SPITZ tristate "Spitz keyboard" depends on PXA_SHARPSL diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile index 8a7a22b..2fa54a7 100644 --- a/drivers/input/keyboard/Makefile +++ b/drivers/input/keyboard/Makefile @@ -25,6 +25,7 @@ obj-$(CONFIG_KEYBOARD_NEWTON) += newtonkbd.o obj-$(CONFIG_KEYBOARD_OMAP) += omap-keypad.o obj-$(CONFIG_KEYBOARD_PXA27x) += pxa27x_keypad.o obj-$(CONFIG_KEYBOARD_PXA930_ROTARY) += pxa930_rotary.o +obj-$(CONFIG_KEYBOARD_SAMSUNG) += samsung-keypad.o obj-$(CONFIG_KEYBOARD_SH_KEYSC) += sh_keysc.o obj-$(CONFIG_KEYBOARD_SPITZ) += spitzkbd.o obj-$(CONFIG_KEYBOARD_STOWAWAY) += stowaway.o diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c new file mode 100644 index 0000000..ad025f7 --- /dev/null +++ b/drivers/input/keyboard/samsung-keypad.c @@ -0,0 +1,366 @@ +/* + * samsung-keypad.c -- Samsung keypad driver + * + * Copyright (C) 2009 Samsung Electronics Co.Ltd + * Author: Joonyoung Shim <jy0922.shim@samsung.com> + * Author: Jaehoon Chung <jh80.chung@samsung.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/input.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <mach/cpu.h> +#include <plat/keypad.h> +#include <plat/regs-keypad.h> + +struct samsung_keypad { + struct input_dev *input_dev; + struct clk *clk; + struct delayed_work work; + void __iomem *base; + unsigned short *keycodes; + unsigned int num_cols; + unsigned int last_rows_state; + unsigned int last_cols_val[SAMSUNG_MAX_ROWS]; + int irq; + atomic_t irq_disable; +}; + +static void samsung_kp_scan(struct work_struct *work) +{ + struct samsung_keypad *keypad = container_of(work, + struct samsung_keypad, work.work); + unsigned int row, col, val; + unsigned int mask; + unsigned int released = 0; + + val = readl(keypad->base + SAMSUNG_KEYIFSTSCLR); + + if (cpu_is_s5pc110()) + mask = S5PC110_R_INT_MASK; + else + mask = SAMSUNG_R_INT_MASK; + + if (val & mask) + released = 1; + + if (released) { + if (cpu_is_s5pc110()) { + val &= ~S5PC110_P_INT_MASK; + val = val >> S5PC110_R_INT_OFFSET; + } else { + val &= ~SAMSUNG_P_INT_MASK; + val = val >> SAMSUNG_R_INT_OFFSET; + } + + if (!(keypad->last_rows_state & val)) + goto end; + + row = 0; + while (!(val & 1)) { + val = val >> 1; + row++; + } + + col = keypad->last_cols_val[row]; + keypad->last_rows_state &= ~(1 << row); + dev_dbg(&keypad->input_dev->dev, + "key released, row: %d, col: %d\n", row, col); + } else { + row = 0; + while (!(val & 1)) { + val = val >> 1; + row++; + } + + for (col = 0; col < keypad->num_cols; col++) { + val = readl(keypad->base + SAMSUNG_KEYIFCOL); + val |= SAMSUNG_KEYIFCOL_MASK; + val &= ~(1 << col); + writel(val, keypad->base + SAMSUNG_KEYIFCOL); + + val = readl(keypad->base + SAMSUNG_KEYIFROW); + if (!(val & (1 << row))) { + keypad->last_rows_state |= (1 << row); + keypad->last_cols_val[row] = col; + dev_dbg(&keypad->input_dev->dev, + "key pressed, row: %d, col: %d\n", + row, col); + goto found; + } + } + goto end; + } + +found: + val = (row << keypad->num_cols) + col; + input_report_key(keypad->input_dev, keypad->keycodes[val], !released); + input_sync(keypad->input_dev); + +end: + /* KEYIFCOL reg clear */ + if (cpu_is_s5pc110()) { + val = ~(SAMSUNG_KEYIFCOL_MASK | S5PC110_KEYIFCOLEN_MASK); + writel(val, keypad->base + SAMSUNG_KEYIFCOL); + } else { + val = ~SAMSUNG_KEYIFCOL_MASK; + writel(val, keypad->base + SAMSUNG_KEYIFCOL); + } + + /* interrupt clear */ + if (cpu_is_s5pc110()) + val = S5PC110_P_INT_MASK | S5PC110_R_INT_MASK; + else + val = SAMSUNG_P_INT_MASK | SAMSUNG_R_INT_MASK; + writel(val, keypad->base + SAMSUNG_KEYIFSTSCLR); + + atomic_dec(&keypad->irq_disable); + enable_irq(keypad->irq); +} + +static irqreturn_t samsung_kp_interrupt(int irq, void *dev_id) +{ + struct samsung_keypad *keypad = dev_id; + + if (!work_pending(&keypad->work.work)) { + disable_irq_nosync(keypad->irq); + atomic_inc(&keypad->irq_disable); + schedule_delayed_work(&keypad->work, msecs_to_jiffies(10)); + } + + return IRQ_HANDLED; +} + +static int __devinit samsung_kp_probe(struct platform_device *pdev) +{ + const struct samsung_keypad_platdata *pdata; + struct samsung_keypad *keypad; + struct resource *res; + struct input_dev *input_dev; + unsigned short *keycodes; + unsigned int max_keymap_size; + unsigned int val; + int i; + int ret; + + pdata = pdev->dev.platform_data; + if (!pdata) { + dev_err(&pdev->dev, "no platform data defined\n"); + return -EINVAL; + } + + if ((pdata->num_rows <= 0) || (pdata->num_rows > SAMSUNG_MAX_ROWS)) + return -EINVAL; + + if ((pdata->num_cols <= 0) || (pdata->num_cols > SAMSUNG_MAX_COLS)) + return -EINVAL; + + /* initialize the gpio */ + if (pdata->cfg_gpio) + pdata->cfg_gpio(pdata->num_rows, pdata->num_cols); + + max_keymap_size = pdata->num_rows * pdata->num_cols; + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); + keycodes = devm_kzalloc(&pdev->dev, + max_keymap_size * sizeof(*keycodes), GFP_KERNEL); + input_dev = input_allocate_device(); + if (!keypad || !keycodes || !input_dev) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + ret = -ENODEV; + goto err_free_mem; + } + + keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); + if (!keypad->base) { + ret = -EBUSY; + goto err_free_mem; + } + + if (pdata->clock) { + keypad->clk = clk_get(&pdev->dev, pdata->clock); + if (IS_ERR(keypad->clk)) { + dev_err(&pdev->dev, "failed to get keypad clk\n"); + ret = PTR_ERR(keypad->clk); + goto err_unmap_base; + } + clk_enable(keypad->clk); + } + + keypad->input_dev = input_dev; + keypad->keycodes = keycodes; + keypad->num_cols = pdata->num_cols; + INIT_DELAYED_WORK(&keypad->work, samsung_kp_scan); + + /* enable interrupt and debouncing filter and wakeup bit */ + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | + SAMSUNG_WAKEUPEN; + writel(val, keypad->base + SAMSUNG_KEYIFCON); + + keypad->irq = platform_get_irq(pdev, 0); + if (keypad->irq < 0) { + ret = keypad->irq; + goto err_disable_clk; + } + + ret = devm_request_irq(&pdev->dev, keypad->irq, + samsung_kp_interrupt, + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, + dev_name(&pdev->dev), keypad); + + if (ret) + goto err_disable_clk; + + input_dev->name = pdev->name; + input_dev->id.bustype = BUS_HOST; + input_dev->dev.parent = &pdev->dev; + input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP); + + input_dev->keycode = keycodes; + input_dev->keycodesize = sizeof(*keycodes); + input_dev->keycodemax = max_keymap_size; + + for (i = 0; i < pdata->keymap_size; i++) { + unsigned int key = pdata->keymap[i]; + unsigned int row = KEY_ROW(key); + unsigned int col = KEY_COL(key); + unsigned short code = KEY_VAL(key); + unsigned int index = row * keypad->num_cols + col; + + keycodes[index] = code; + set_bit(code, input_dev->keybit); + } + + ret = input_register_device(keypad->input_dev); + if (ret) + goto err_free_irq; + + device_init_wakeup(&pdev->dev, 1); + + platform_set_drvdata(pdev, keypad); + + return 0; + +err_free_irq: + devm_free_irq(&pdev->dev, keypad->irq, keypad); +err_disable_clk: + if (keypad->clk) { + clk_disable(keypad->clk); + clk_put(keypad->clk); + } +err_unmap_base: + devm_iounmap(&pdev->dev, keypad->base); +err_free_mem: + input_free_device(input_dev); + devm_kfree(&pdev->dev, keycodes); + devm_kfree(&pdev->dev, keypad); + + return ret; +} + +static int __devexit samsung_kp_remove(struct platform_device *pdev) +{ + struct samsung_keypad *keypad = platform_get_drvdata(pdev); + + devm_free_irq(&pdev->dev, keypad->irq, keypad); + cancel_delayed_work_sync(&keypad->work); + + /* + * If work indeed has been cancelled, disable_irq() will have been left + * unbalanced from samsung_kp_interrupt(). + */ + while (atomic_dec_return(&keypad->irq_disable) >= 0) + enable_irq(keypad->irq); + + platform_set_drvdata(pdev, NULL); + input_unregister_device(keypad->input_dev); + + if (keypad->clk) { + clk_disable(keypad->clk); + clk_put(keypad->clk); + } + + devm_iounmap(&pdev->dev, keypad->base); + devm_kfree(&pdev->dev, keypad->keycodes); + devm_kfree(&pdev->dev, keypad); + + return 0; +} + +#ifdef CONFIG_PM +static int samsung_kp_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct samsung_keypad *keypad = platform_get_drvdata(pdev); + + disable_irq(keypad->irq); + enable_irq_wake(keypad->irq); + + if (keypad->clk) + clk_disable(keypad->clk); + + return 0; +} + +static int samsung_kp_resume(struct platform_device *pdev) +{ + struct samsung_keypad *keypad = platform_get_drvdata(pdev); + unsigned int val; + + if (keypad->clk) + clk_enable(keypad->clk); + + /* enable interrupt and debouncing filter and wakeup bit */ + val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN | + SAMSUNG_WAKEUPEN; + writel(val, keypad->base + SAMSUNG_KEYIFCON); + + disable_irq_wake(keypad->irq); + enable_irq(keypad->irq); + + return 0; +} +#else +#define samsung_kp_suspend NULL +#define samsung_kp_resume NULL +#endif + +static struct platform_driver samsung_kp_driver = { + .probe = samsung_kp_probe, + .remove = __devexit_p(samsung_kp_remove), + .suspend = samsung_kp_suspend, + .resume = samsung_kp_resume, + .driver = { + .name = "samsung-keypad", + .owner = THIS_MODULE, + }, +}; + +static int __init samsung_kp_init(void) +{ + return platform_driver_register(&samsung_kp_driver); +} + +static void __exit samsung_kp_exit(void) +{ + platform_driver_unregister(&samsung_kp_driver); +} + +module_init(samsung_kp_init); +module_exit(samsung_kp_exit); + +MODULE_DESCRIPTION("Samsung keypad driver"); +MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>"); +MODULE_AUTHOR("Jaehoon Chung <jh80.chung@samsung.com>"); +MODULE_LICENSE("GPL");