* [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
@ 2009-09-05 13:55 양진성
2009-09-05 16:17 ` Mark Brown
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: 양진성 @ 2009-09-05 13:55 UTC (permalink / raw)
To: linux-input
Cc: laforge, ben-linux,
김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자,
'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'
This keypad driver supports Samsung s3c based SoCs such as s3c6410.
This driver is written with input device compatibles.
Signed-off-by: Jinsung Yang <jsgood.yang@samsung.com>
Signed-off-by: Kyeongil Kim <ki0351.kim@samsung.com>
---
drivers/input/keyboard/s3c-keypad.c | 468 +++++++++++++++++++++++++++++++++++
1 files changed, 468 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/keyboard/s3c-keypad.c
diff --git a/drivers/input/keyboard/s3c-keypad.c b/drivers/input/keyboard/s3c-keypad.c
new file mode 100644
index 0000000..73b9a90
--- /dev/null
+++ b/drivers/input/keyboard/s3c-keypad.c
@@ -0,0 +1,468 @@
+/*
+ * linux/drivers/input/keyboard/s3c_keypad.c
+ *
+ * Driver for Samsung SoC matrix keypad controller.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/platform_device.h>
+#include <linux/miscdevice.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+
+#include <mach/hardware.h>
+#include <asm/irq.h>
+
+#include <plat/keypad.h>
+#include <plat/regs-keypad.h>
+
+#undef DEBUG
+
+static const unsigned char s3c_keycode[] = {
+ 1, 2, KEY_1, KEY_Q, KEY_A, 6, 7, KEY_LEFT,
+ 9, 10, KEY_2, KEY_W, KEY_S, KEY_Z, KEY_RIGHT, 16,
+ 17, 18, KEY_3, KEY_E, KEY_D, KEY_X, 23, KEY_UP,
+ 25, 26, KEY_4, KEY_R, KEY_F, KEY_C, 31, 32,
+ 33, KEY_O, KEY_5, KEY_T, KEY_G, KEY_V, KEY_DOWN, KEY_BACKSPACE,
+ KEY_P, KEY_0, KEY_6, KEY_Y, KEY_H, KEY_SPACE, 47, 48,
+ KEY_M, KEY_L, KEY_7, KEY_U, KEY_J, KEY_N, 55, KEY_ENTER,
+ KEY_LEFTSHIFT, KEY_9, KEY_8, KEY_I, KEY_K, KEY_B, 63, KEY_COMMA,
+};
+
+struct s3c_keypad {
+ struct s3c_platform_keypad *pdata;
+ unsigned char keycodes[ARRAY_SIZE(s3c_keycode)];
+ struct input_dev *dev;
+ struct timer_list timer;
+ void __iomem *regs;
+ struct clk *clk;
+ int irq;
+ int timer_enabled;
+ unsigned int prevmask_low;
+ unsigned int prevmask_high;
+ unsigned int keyifcon;
+ unsigned int keyiffc;
+};
+
+static int s3c_keypad_scan(struct s3c_keypad *keypad, u32 *keymask_low,
+ u32 *keymask_high)
+{
+ struct s3c_platform_keypad *pdata = keypad->pdata;
+ int i, j = 0;
+ u32 cval, rval, cfg;
+
+ for (i = 0; i < pdata->nr_cols; i++) {
+ cval = readl(keypad->regs + S3C_KEYIFCOL);
+ cval |= S3C_KEYIF_COL_DMASK;
+ cval &= ~(1 << i);
+ writel(cval, keypad->regs + S3C_KEYIFCOL);
+ udelay(pdata->delay);
+
+ rval = ~(readl(keypad->regs + S3C_KEYIFROW)) &
+ S3C_KEYIF_ROW_DMASK;
+
+ if ((i * pdata->nr_rows) < pdata->max_masks)
+ *keymask_low |= (rval << (i * pdata->nr_rows));
+ else {
+ *keymask_high |= (rval << (j * pdata->nr_rows));
+ j++;
+ }
+ }
+
+ cfg = readl(keypad->regs + S3C_KEYIFCOL);
+ cfg &= ~S3C_KEYIF_COL_MASK_ALL;
+ writel(cfg, keypad->regs + S3C_KEYIFCOL);
+
+ return 0;
+}
+
+static void s3c_keypad_timer_handler(unsigned long data)
+{
+ struct s3c_keypad *keypad = (struct s3c_keypad *)data;
+ struct s3c_platform_keypad *pdata = keypad->pdata;
+ struct input_dev *input = keypad->dev;
+ u32 keymask_low = 0, keymask_high = 0;
+ u32 press_mask_low, press_mask_high;
+ u32 release_mask_low, release_mask_high, code, cfg;
+ int i;
+
+ s3c_keypad_scan(keypad, &keymask_low, &keymask_high);
+
+ if (keymask_low != keypad->prevmask_low) {
+ press_mask_low = ((keymask_low ^ keypad->prevmask_low) &
+ keymask_low);
+ release_mask_low = ((keymask_low ^ keypad->prevmask_low) &
+ keypad->prevmask_low);
+
+ i = 0;
+ while (press_mask_low) {
+ if (press_mask_low & 1) {
+ code = keypad->keycodes[i];
+ input_report_key(input, code, 1);
+ dev_dbg(&input->dev, "low pressed: %d\n", i);
+ }
+ press_mask_low >>= 1;
+ i++;
+ }
+
+ i = 0;
+ while (release_mask_low) {
+ if (release_mask_low & 1) {
+ code = keypad->keycodes[i];
+ input_report_key(input, code, 0);
+ dev_dbg(&input->dev, "low released : %d\n", i);
+ }
+ release_mask_low >>= 1;
+ i++;
+ }
+ keypad->prevmask_low = keymask_low;
+ }
+
+ if (keymask_high != keypad->prevmask_high) {
+ press_mask_high = ((keymask_high ^ keypad->prevmask_high) &
+ keymask_high);
+ release_mask_high = ((keymask_high ^ keypad->prevmask_high) &
+ keypad->prevmask_high);
+
+ i = 0;
+ while (press_mask_high) {
+ if (press_mask_high & 1) {
+ code = keypad->keycodes[i + pdata->max_masks];
+ input_report_key(input, code, 1);
+ dev_dbg(&input->dev, "high pressed: %d %d\n",
+ keypad->keycodes[i + pdata->max_masks],
+ i);
+ }
+ press_mask_high >>= 1;
+ i++;
+ }
+
+ i = 0;
+ while (release_mask_high) {
+ if (release_mask_high & 1) {
+ code = keypad->keycodes[i + pdata->max_masks];
+ input_report_key(input, code, 0);
+ dev_dbg(&input->dev, "high released: %d\n",
+ keypad->keycodes[i + pdata->max_masks]);
+ }
+ release_mask_high >>= 1;
+ i++;
+ }
+ keypad->prevmask_high = keymask_high;
+ }
+
+ 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;
+ }
+}
+
+static irqreturn_t s3c_keypad_irq(int irq, void *dev_id)
+{
+ struct s3c_keypad *keypad = dev_id;
+ u32 cfg;
+
+ /* disable keypad interrupt and schedule for keypad timer handler */
+ cfg = readl(keypad->regs + S3C_KEYIFCON);
+ cfg &= ~(S3C_KEYIF_INT_F_EN | S3C_KEYIF_INT_R_EN);
+ writel(cfg, keypad->regs + S3C_KEYIFCON);
+
+ 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;
+ }
+
+ /* clear the keypad interrupt status */
+ writel(S3C_KEYIF_STSCLR_CLEAR, keypad->regs + S3C_KEYIFSTSCLR);
+
+ return IRQ_HANDLED;
+}
+
+static int s3c_keypad_open(struct input_dev *dev)
+{
+ struct s3c_keypad *keypad = input_get_drvdata(dev);
+ u32 cfg;
+
+ clk_enable(keypad->clk);
+
+ /* init keypad h/w block */
+ 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);
+
+ cfg = readl(keypad->regs + S3C_KEYIFFC);
+ cfg |= 0x1;
+ writel(cfg, keypad->regs + S3C_KEYIFFC);
+
+ cfg = readl(keypad->regs + S3C_KEYIFCOL);
+ cfg &= ~S3C_KEYIF_COL_MASK_ALL;
+ writel(cfg, keypad->regs + S3C_KEYIFCOL);
+
+ /* Scan timer init */
+ init_timer(&keypad->timer);
+ keypad->timer.function = s3c_keypad_timer_handler;
+ keypad->timer.data = (unsigned long)keypad;
+ keypad->timer.expires = jiffies + (HZ / 10);
+
+ if (keypad->timer_enabled) {
+ mod_timer(&keypad->timer, keypad->timer.expires);
+ } else {
+ add_timer(&keypad->timer);
+ keypad->timer_enabled = 1;
+ }
+
+ return 0;
+}
+
+static void s3c_keypad_close(struct input_dev *dev)
+{
+ struct s3c_keypad *keypad = input_get_drvdata(dev);
+
+ clk_disable(keypad->clk);
+}
+
+#define res_size(res) ((res)->end - (res)->start + 1)
+
+static int __devinit s3c_keypad_probe(struct platform_device *pdev)
+{
+ struct s3c_platform_keypad *pdata;
+ struct s3c_keypad *keypad;
+ struct input_dev *input;
+ struct resource *res;
+ int irq, error, i;
+
+ pdata = pdev->dev.platform_data;
+ if (pdata == NULL) {
+ dev_err(&pdev->dev, "no platform data\n");
+ return -EINVAL;
+ }
+
+ keypad = kzalloc(sizeof(struct s3c_keypad), GFP_KERNEL);
+ if (keypad == NULL) {
+ dev_err(&pdev->dev, "failed to allocate driver data\n");
+ return -ENOMEM;
+ }
+
+ keypad->pdata = pdata;
+ memcpy(keypad->keycodes, s3c_keycode, sizeof(keypad->keycodes));
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res == NULL) {
+ dev_err(&pdev->dev, "failed to get I/O memory\n");
+ error = -ENXIO;
+ goto err_get_io;
+ }
+
+ res = request_mem_region(res->start, res_size(res), pdev->name);
+ if (res == NULL) {
+ dev_err(&pdev->dev, "failed to request I/O memory\n");
+ error = -EBUSY;
+ goto err_get_io;
+ }
+
+ keypad->regs = ioremap(res->start, res_size(res));
+ if (keypad->regs == NULL) {
+ dev_err(&pdev->dev, "failed to remap I/O memory\n");
+ error = -ENXIO;
+ goto err_map_io;
+ }
+
+ keypad->clk = clk_get(&pdev->dev, "keypad");
+ if (IS_ERR(keypad->clk)) {
+ dev_err(&pdev->dev, "failed to get keypad clock\n");
+ error = PTR_ERR(keypad->clk);
+ goto err_clk;
+ }
+
+ /* Create and register the input driver. */
+ input = input_allocate_device();
+ if (!input) {
+ dev_err(&pdev->dev, "failed to allocate input device\n");
+ error = -ENOMEM;
+ goto err_alloc_input;
+ }
+
+ input->name = pdev->name;
+ input->id.bustype = BUS_HOST;
+ input->open = s3c_keypad_open;
+ input->close = s3c_keypad_close;
+ input->dev.parent = &pdev->dev;
+ input->keycode = (void *)keypad->keycodes;
+ input->keycodesize = sizeof(keypad->keycodes[0]);
+ input->keycodemax = ARRAY_SIZE(keypad->keycodes);
+
+ keypad->dev = input;
+
+ input_set_drvdata(input, keypad);
+
+ __set_bit(EV_KEY, input->evbit);
+ __set_bit(EV_REP, input->evbit);
+
+ for (i = 0; i < pdata->max_keys; i++) {
+ keypad->keycodes[i] = s3c_keycode[i];
+ if (keypad->keycodes[i] <= 0)
+ continue;
+
+ __set_bit(keypad->keycodes[i] & KEY_MAX, input->keybit);
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "failed to get keypad irq\n");
+ error = -ENXIO;
+ goto err_get_irq;
+ }
+
+ platform_set_drvdata(pdev, keypad);
+
+ error = request_irq(irq, s3c_keypad_irq, IRQF_DISABLED,
+ pdev->name, keypad);
+ if (error) {
+ dev_err(&pdev->dev, "failed to request IRQ\n");
+ goto err_req_irq;
+ }
+
+ keypad->irq = irq;
+
+ /* 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;
+ }
+
+ device_init_wakeup(&pdev->dev, 1);
+ dev_info(&pdev->dev, "Samsung Keypad Interface driver\n");
+
+ return 0;
+
+err_reg_input:
+ free_irq(irq, pdev);
+
+err_req_irq:
+ platform_set_drvdata(pdev, NULL);
+
+err_get_irq:
+ input_free_device(input);
+
+err_alloc_input:
+ clk_put(keypad->clk);
+
+err_clk:
+ iounmap(keypad->regs);
+
+err_map_io:
+ release_mem_region(res->start, res_size(res));
+
+err_get_io:
+ kfree(keypad);
+
+ return error;
+}
+
+static int __devexit s3c_keypad_remove(struct platform_device *pdev)
+{
+ struct s3c_keypad *keypad = platform_get_drvdata(pdev);
+ struct resource *res;
+
+ free_irq(keypad->irq, pdev);
+
+ clk_disable(keypad->clk);
+ clk_put(keypad->clk);
+
+ input_unregister_device(keypad->dev);
+ input_free_device(keypad->dev);
+
+ iounmap(keypad->regs);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(res->start, res_size(res));
+
+ platform_set_drvdata(pdev, NULL);
+ kfree(keypad);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int s3c_keypad_suspend(struct platform_device *dev, pm_message_t state)
+{
+ struct s3c_keypad *keypad = platform_get_drvdata(pdev);
+
+ keypad->keyifcon = readl(keypad->regs + S3C_KEYIFCON);
+ keypad->keyiffc = readl(keypad->regs + S3C_KEYIFFC);
+
+ disable_irq(IRQ_KEYPAD);
+ clk_disable(keypad->clk);
+
+ return 0;
+}
+
+static int s3c_keypad_resume(struct platform_device *dev)
+{
+ struct s3c_keypad *keypad = platform_get_drvdata(pdev);
+
+ clk_enable(keypad->clock);
+
+ writel(keypad->keyifcon, keypad->regs + S3C_KEYIFCON);
+ writel(keypad->keyiffc, keypad->regs + S3C_KEYIFFC);
+
+ enable_irq(IRQ_KEYPAD);
+
+ return 0;
+}
+#else
+#define s3c_keypad_suspend NULL
+#define s3c_keypad_resume NULL
+#endif /* CONFIG_PM */
+
+static struct platform_driver s3c_keypad_driver = {
+ .probe = s3c_keypad_probe,
+ .remove = s3c_keypad_remove,
+ .suspend = s3c_keypad_suspend,
+ .resume = s3c_keypad_resume,
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = "s3c-keypad",
+ },
+};
+
+static int __init s3c_keypad_init(void)
+{
+ return platform_driver_register(&s3c_keypad_driver);
+}
+
+static void __exit s3c_keypad_exit(void)
+{
+ platform_driver_unregister(&s3c_keypad_driver);
+}
+
+module_init(s3c_keypad_init);
+module_exit(s3c_keypad_exit);
+
+MODULE_AUTHOR("Kyeongil, Kim <ki0351.kim@samsung.com>");
+MODULE_AUTHOR("Jinsung, Yang <jsgood.yang@samsung.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Keypad interface for Samsung SoC");
+
--
1.6.2.5
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-05 13:55 [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs 양진성
@ 2009-09-05 16:17 ` Mark Brown
2009-09-07 3:25 ` Kyungmin Park
` (2 subsequent siblings)
3 siblings, 0 replies; 26+ messages in thread
From: Mark Brown @ 2009-09-05 16:17 UTC (permalink / raw)
To: 양진성
Cc: linux-input, laforge, ben-linux,
김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자,
'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'
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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-05 13:55 [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs 양진성
2009-09-05 16:17 ` Mark Brown
@ 2009-09-07 3:25 ` Kyungmin Park
2009-09-07 3:50 ` Jinsung Yang
2009-09-07 6:26 ` Harald Welte
2009-09-07 5:38 ` [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs Joonyoung Shim
2009-09-07 8:00 ` Dmitry Torokhov
3 siblings, 2 replies; 26+ messages in thread
From: Kyungmin Park @ 2009-09-07 3:25 UTC (permalink / raw)
To: 양진성
Cc: linux-input, laforge, ben-linux,
김경일/AP개발팀/E3/삼성전자,
김국진/AP개발팀/E5/삼성전자
Hi,
2009/9/5 양진성 <jsgood.yang@samsung.com>:
> This keypad driver supports Samsung s3c based SoCs such as s3c6410.
> This driver is written with input device compatibles.
>
> Signed-off-by: Jinsung Yang <jsgood.yang@samsung.com>
> Signed-off-by: Kyeongil Kim <ki0351.kim@samsung.com>
> ---
> drivers/input/keyboard/s3c-keypad.c | 468 +++++++++++++++++++++++++++++++++++
> 1 files changed, 468 insertions(+), 0 deletions(-)
> create mode 100644 drivers/input/keyboard/s3c-keypad.c
>
> diff --git a/drivers/input/keyboard/s3c-keypad.c b/drivers/input/keyboard/s3c-keypad.c
> new file mode 100644
> index 0000000..73b9a90
> --- /dev/null
> +++ b/drivers/input/keyboard/s3c-keypad.c
> @@ -0,0 +1,468 @@
> +/*
> + * linux/drivers/input/keyboard/s3c_keypad.c
> + *
> + * Driver for Samsung SoC matrix keypad controller.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +
> +#include <mach/hardware.h>
> +#include <asm/irq.h>
> +
> +#include <plat/keypad.h>
> +#include <plat/regs-keypad.h>
> +
> +#undef DEBUG
> +
> +static const unsigned char s3c_keycode[] = {
> + 1, 2, KEY_1, KEY_Q, KEY_A, 6, 7, KEY_LEFT,
> + 9, 10, KEY_2, KEY_W, KEY_S, KEY_Z, KEY_RIGHT, 16,
> + 17, 18, KEY_3, KEY_E, KEY_D, KEY_X, 23, KEY_UP,
> + 25, 26, KEY_4, KEY_R, KEY_F, KEY_C, 31, 32,
> + 33, KEY_O, KEY_5, KEY_T, KEY_G, KEY_V, KEY_DOWN, KEY_BACKSPACE,
> + KEY_P, KEY_0, KEY_6, KEY_Y, KEY_H, KEY_SPACE, 47, 48,
> + KEY_M, KEY_L, KEY_7, KEY_U, KEY_J, KEY_N, 55, KEY_ENTER,
> + KEY_LEFTSHIFT, KEY_9, KEY_8, KEY_I, KEY_K, KEY_B, 63, KEY_COMMA,
> +};
Why do you use fixed keycode? Dose it provided from board specific
platform? and If we want to use only 3 * 3 keypad then how do you
handle this one?
> +
> +struct s3c_keypad {
> + struct s3c_platform_keypad *pdata;
> + unsigned char keycodes[ARRAY_SIZE(s3c_keycode)];
We don't need full keycode array here. It should be allocated with
required size.
> + struct input_dev *dev;
> + struct timer_list timer;
> + void __iomem *regs;
> + struct clk *clk;
> + int irq;
> + int timer_enabled;
> + unsigned int prevmask_low;
> + unsigned int prevmask_high;
> + unsigned int keyifcon;
> + unsigned int keyiffc;
> +};
> +
> +static int s3c_keypad_scan(struct s3c_keypad *keypad, u32 *keymask_low,
> + u32 *keymask_high)
> +{
> + struct s3c_platform_keypad *pdata = keypad->pdata;
> + int i, j = 0;
> + u32 cval, rval, cfg;
> +
> + for (i = 0; i < pdata->nr_cols; i++) {
> + cval = readl(keypad->regs + S3C_KEYIFCOL);
> + cval |= S3C_KEYIF_COL_DMASK;
> + cval &= ~(1 << i);
> + writel(cval, keypad->regs + S3C_KEYIFCOL);
> + udelay(pdata->delay);
> +
> + rval = ~(readl(keypad->regs + S3C_KEYIFROW)) &
> + S3C_KEYIF_ROW_DMASK;
> +
> + if ((i * pdata->nr_rows) < pdata->max_masks)
> + *keymask_low |= (rval << (i * pdata->nr_rows));
> + else {
> + *keymask_high |= (rval << (j * pdata->nr_rows));
> + j++;
> + }
> + }
> +
> + cfg = readl(keypad->regs + S3C_KEYIFCOL);
> + cfg &= ~S3C_KEYIF_COL_MASK_ALL;
> + writel(cfg, keypad->regs + S3C_KEYIFCOL);
> +
> + return 0;
> +}
> +
> +static void s3c_keypad_timer_handler(unsigned long data)
> +{
> + struct s3c_keypad *keypad = (struct s3c_keypad *)data;
> + struct s3c_platform_keypad *pdata = keypad->pdata;
> + struct input_dev *input = keypad->dev;
> + u32 keymask_low = 0, keymask_high = 0;
> + u32 press_mask_low, press_mask_high;
> + u32 release_mask_low, release_mask_high, code, cfg;
> + int i;
> +
> + s3c_keypad_scan(keypad, &keymask_low, &keymask_high);
> +
> + if (keymask_low != keypad->prevmask_low) {
> + press_mask_low = ((keymask_low ^ keypad->prevmask_low) &
> + keymask_low);
> + release_mask_low = ((keymask_low ^ keypad->prevmask_low) &
> + keypad->prevmask_low);
> +
> + i = 0;
> + while (press_mask_low) {
> + if (press_mask_low & 1) {
> + code = keypad->keycodes[i];
> + input_report_key(input, code, 1);
> + dev_dbg(&input->dev, "low pressed: %d\n", i);
> + }
> + press_mask_low >>= 1;
> + i++;
> + }
> +
> + i = 0;
> + while (release_mask_low) {
> + if (release_mask_low & 1) {
> + code = keypad->keycodes[i];
> + input_report_key(input, code, 0);
> + dev_dbg(&input->dev, "low released : %d\n", i);
> + }
> + release_mask_low >>= 1;
> + i++;
> + }
> + keypad->prevmask_low = keymask_low;
> + }
> +
> + if (keymask_high != keypad->prevmask_high) {
> + press_mask_high = ((keymask_high ^ keypad->prevmask_high) &
> + keymask_high);
> + release_mask_high = ((keymask_high ^ keypad->prevmask_high) &
> + keypad->prevmask_high);
> +
> + i = 0;
> + while (press_mask_high) {
> + if (press_mask_high & 1) {
> + code = keypad->keycodes[i + pdata->max_masks];
> + input_report_key(input, code, 1);
> + dev_dbg(&input->dev, "high pressed: %d %d\n",
> + keypad->keycodes[i + pdata->max_masks],
> + i);
> + }
> + press_mask_high >>= 1;
> + i++;
> + }
> +
> + i = 0;
> + while (release_mask_high) {
> + if (release_mask_high & 1) {
> + code = keypad->keycodes[i + pdata->max_masks];
> + input_report_key(input, code, 0);
> + dev_dbg(&input->dev, "high released: %d\n",
> + keypad->keycodes[i + pdata->max_masks]);
> + }
> + release_mask_high >>= 1;
> + i++;
> + }
> + keypad->prevmask_high = keymask_high;
> + }
> +
> + 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;
> + }
> +}
> +
> +static irqreturn_t s3c_keypad_irq(int irq, void *dev_id)
> +{
> + struct s3c_keypad *keypad = dev_id;
> + u32 cfg;
> +
> + /* disable keypad interrupt and schedule for keypad timer handler */
> + cfg = readl(keypad->regs + S3C_KEYIFCON);
> + cfg &= ~(S3C_KEYIF_INT_F_EN | S3C_KEYIF_INT_R_EN);
> + writel(cfg, keypad->regs + S3C_KEYIFCON);
> +
> + 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;
> + }
> +
> + /* clear the keypad interrupt status */
> + writel(S3C_KEYIF_STSCLR_CLEAR, keypad->regs + S3C_KEYIFSTSCLR);
> +
> + return IRQ_HANDLED;
> +}
Why do you use timer. As other input drivers how about to use workqueue?
> +
> +static int s3c_keypad_open(struct input_dev *dev)
> +{
> + struct s3c_keypad *keypad = input_get_drvdata(dev);
> + u32 cfg;
> +
> + clk_enable(keypad->clk);
> +
> + /* init keypad h/w block */
> + 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);
> +
> + cfg = readl(keypad->regs + S3C_KEYIFFC);
> + cfg |= 0x1;
What's the meaning of '0x1'?
> + writel(cfg, keypad->regs + S3C_KEYIFFC);
> +
> + cfg = readl(keypad->regs + S3C_KEYIFCOL);
> + cfg &= ~S3C_KEYIF_COL_MASK_ALL;
> + writel(cfg, keypad->regs + S3C_KEYIFCOL);
> +
> + /* Scan timer init */
> + init_timer(&keypad->timer);
> + keypad->timer.function = s3c_keypad_timer_handler;
> + keypad->timer.data = (unsigned long)keypad;
> + keypad->timer.expires = jiffies + (HZ / 10);
> +
> + if (keypad->timer_enabled) {
> + mod_timer(&keypad->timer, keypad->timer.expires);
> + } else {
useless curly braces
> + add_timer(&keypad->timer);
> + keypad->timer_enabled = 1;
> + }
> +
> + return 0;
> +}
> +
> +static void s3c_keypad_close(struct input_dev *dev)
> +{
> + struct s3c_keypad *keypad = input_get_drvdata(dev);
> +
> + clk_disable(keypad->clk);
> +}
> +
> +#define res_size(res) ((res)->end - (res)->start + 1)
> +
> +static int __devinit s3c_keypad_probe(struct platform_device *pdev)
> +{
> + struct s3c_platform_keypad *pdata;
> + struct s3c_keypad *keypad;
> + struct input_dev *input;
> + struct resource *res;
> + int irq, error, i;
> +
> + pdata = pdev->dev.platform_data;
> + if (pdata == NULL) {
> + dev_err(&pdev->dev, "no platform data\n");
> + return -EINVAL;
> + }
> +
> + keypad = kzalloc(sizeof(struct s3c_keypad), GFP_KERNEL);
> + if (keypad == NULL) {
> + dev_err(&pdev->dev, "failed to allocate driver data\n");
> + return -ENOMEM;
> + }
> +
> + keypad->pdata = pdata;
> + memcpy(keypad->keycodes, s3c_keycode, sizeof(keypad->keycodes));
memcpy??? if you don't modify s3c_keycode. just assign it.
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "failed to get I/O memory\n");
> + error = -ENXIO;
> + goto err_get_io;
> + }
> +
> + res = request_mem_region(res->start, res_size(res), pdev->name);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "failed to request I/O memory\n");
> + error = -EBUSY;
> + goto err_get_io;
> + }
> +
> + keypad->regs = ioremap(res->start, res_size(res));
> + if (keypad->regs == NULL) {
> + dev_err(&pdev->dev, "failed to remap I/O memory\n");
> + error = -ENXIO;
> + goto err_map_io;
> + }
> +
> + keypad->clk = clk_get(&pdev->dev, "keypad");
> + if (IS_ERR(keypad->clk)) {
> + dev_err(&pdev->dev, "failed to get keypad clock\n");
> + error = PTR_ERR(keypad->clk);
> + goto err_clk;
> + }
> +
> + /* Create and register the input driver. */
> + input = input_allocate_device();
> + if (!input) {
> + dev_err(&pdev->dev, "failed to allocate input device\n");
> + error = -ENOMEM;
> + goto err_alloc_input;
> + }
> +
> + input->name = pdev->name;
> + input->id.bustype = BUS_HOST;
> + input->open = s3c_keypad_open;
> + input->close = s3c_keypad_close;
> + input->dev.parent = &pdev->dev;
> + input->keycode = (void *)keypad->keycodes;
> + input->keycodesize = sizeof(keypad->keycodes[0]);
> + input->keycodemax = ARRAY_SIZE(keypad->keycodes);
> +
> + keypad->dev = input;
> +
> + input_set_drvdata(input, keypad);
> +
> + __set_bit(EV_KEY, input->evbit);
> + __set_bit(EV_REP, input->evbit);
> +
> + for (i = 0; i < pdata->max_keys; i++) {
> + keypad->keycodes[i] = s3c_keycode[i];
> + if (keypad->keycodes[i] <= 0)
> + continue;
> +
> + __set_bit(keypad->keycodes[i] & KEY_MAX, input->keybit);
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "failed to get keypad irq\n");
> + error = -ENXIO;
> + goto err_get_irq;
> + }
> +
> + platform_set_drvdata(pdev, keypad);
> +
> + error = request_irq(irq, s3c_keypad_irq, IRQF_DISABLED,
> + pdev->name, keypad);
> + if (error) {
> + dev_err(&pdev->dev, "failed to request IRQ\n");
> + goto err_req_irq;
> + }
> +
> + keypad->irq = irq;
> +
> + /* 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;
> + }
> +
> + device_init_wakeup(&pdev->dev, 1);
> + dev_info(&pdev->dev, "Samsung Keypad Interface driver\n");
> +
> + return 0;
> +
> +err_reg_input:
> + free_irq(irq, pdev);
> +
> +err_req_irq:
> + platform_set_drvdata(pdev, NULL);
> +
> +err_get_irq:
> + input_free_device(input);
> +
> +err_alloc_input:
> + clk_put(keypad->clk);
> +
> +err_clk:
> + iounmap(keypad->regs);
> +
> +err_map_io:
> + release_mem_region(res->start, res_size(res));
> +
> +err_get_io:
> + kfree(keypad);
> +
> + return error;
> +}
> +
> +static int __devexit s3c_keypad_remove(struct platform_device *pdev)
> +{
> + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> + struct resource *res;
> +
> + free_irq(keypad->irq, pdev);
> +
> + clk_disable(keypad->clk);
> + clk_put(keypad->clk);
> +
> + input_unregister_device(keypad->dev);
> + input_free_device(keypad->dev);
> +
> + iounmap(keypad->regs);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(res->start, res_size(res));
> +
> + platform_set_drvdata(pdev, NULL);
> + kfree(keypad);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int s3c_keypad_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> +
> + keypad->keyifcon = readl(keypad->regs + S3C_KEYIFCON);
> + keypad->keyiffc = readl(keypad->regs + S3C_KEYIFFC);
> +
> + disable_irq(IRQ_KEYPAD);
> + clk_disable(keypad->clk);
> +
> + return 0;
> +}
> +
> +static int s3c_keypad_resume(struct platform_device *dev)
> +{
> + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> +
> + clk_enable(keypad->clock);
> +
> + writel(keypad->keyifcon, keypad->regs + S3C_KEYIFCON);
> + writel(keypad->keyiffc, keypad->regs + S3C_KEYIFFC);
> +
> + enable_irq(IRQ_KEYPAD);
> +
> + return 0;
> +}
> +#else
> +#define s3c_keypad_suspend NULL
> +#define s3c_keypad_resume NULL
> +#endif /* CONFIG_PM */
> +
> +static struct platform_driver s3c_keypad_driver = {
> + .probe = s3c_keypad_probe,
> + .remove = s3c_keypad_remove,
> + .suspend = s3c_keypad_suspend,
> + .resume = s3c_keypad_resume,
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "s3c-keypad",
> + },
> +};
> +
> +static int __init s3c_keypad_init(void)
> +{
> + return platform_driver_register(&s3c_keypad_driver);
> +}
> +
> +static void __exit s3c_keypad_exit(void)
> +{
> + platform_driver_unregister(&s3c_keypad_driver);
> +}
> +
> +module_init(s3c_keypad_init);
> +module_exit(s3c_keypad_exit);
> +
> +MODULE_AUTHOR("Kyeongil, Kim <ki0351.kim@samsung.com>");
> +MODULE_AUTHOR("Jinsung, Yang <jsgood.yang@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Keypad interface for Samsung SoC");
> +
Finally,
We did duplicated works. We already implement the keypad driver for
s5pc100 & s5pc110. but we use workqueue structure instead of timer.
I will post the our works.
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 3:25 ` Kyungmin Park
@ 2009-09-07 3:50 ` Jinsung Yang
2009-09-07 6:26 ` Harald Welte
1 sibling, 0 replies; 26+ messages in thread
From: Jinsung Yang @ 2009-09-07 3:50 UTC (permalink / raw)
To: 'Kyungmin Park'
Cc: linux-input, laforge, ben-linux,
'김경일/AP개발팀/E3/삼성전자',
'김국진/AP개발팀/E5/삼성전자'
> -----Original Message-----
> From: kyungmin78@gmail.com [mailto:kyungmin78@gmail.com] On Behalf Of
> Kyungmin Park
> Sent: Monday, September 07, 2009 12:25 PM
> To: 양진성
> Cc: linux-input@vger.kernel.org; laforge@gnumonks.org; ben-linux@fluff.org;
> 김경일/AP개발팀/E3/삼성전자; 김국진/AP개발팀/E5/삼성전자
> Subject: Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c
> series SoCs
>
> Hi,
>
> 2009/9/5 양진성 <jsgood.yang@samsung.com>:
> > This keypad driver supports Samsung s3c based SoCs such as s3c6410.
> > This driver is written with input device compatibles.
> >
> > Signed-off-by: Jinsung Yang <jsgood.yang@samsung.com>
> > Signed-off-by: Kyeongil Kim <ki0351.kim@samsung.com>
> > ---
> > drivers/input/keyboard/s3c-keypad.c | 468
> +++++++++++++++++++++++++++++++++++
> > 1 files changed, 468 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/input/keyboard/s3c-keypad.c
> >
> > diff --git a/drivers/input/keyboard/s3c-keypad.c
> b/drivers/input/keyboard/s3c-keypad.c
> > new file mode 100644
> > index 0000000..73b9a90
> > --- /dev/null
> > +++ b/drivers/input/keyboard/s3c-keypad.c
> > @@ -0,0 +1,468 @@
> > +/*
> > + * linux/drivers/input/keyboard/s3c_keypad.c
> > + *
> > + * Driver for Samsung SoC matrix keypad controller.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/input.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +
> > +#include <mach/hardware.h>
> > +#include <asm/irq.h>
> > +
> > +#include <plat/keypad.h>
> > +#include <plat/regs-keypad.h>
> > +
> > +#undef DEBUG
> > +
> > +static const unsigned char s3c_keycode[] = {
> > + 1, 2, KEY_1, KEY_Q, KEY_A, 6, 7, KEY_LEFT,
> > + 9, 10, KEY_2, KEY_W, KEY_S, KEY_Z, KEY_RIGHT, 16,
> > + 17, 18, KEY_3, KEY_E, KEY_D, KEY_X, 23, KEY_UP,
> > + 25, 26, KEY_4, KEY_R, KEY_F, KEY_C, 31, 32,
> > + 33, KEY_O, KEY_5, KEY_T, KEY_G, KEY_V, KEY_DOWN, KEY_BACKSPACE,
> > + KEY_P, KEY_0, KEY_6, KEY_Y, KEY_H, KEY_SPACE, 47, 48,
> > + KEY_M, KEY_L, KEY_7, KEY_U, KEY_J, KEY_N, 55, KEY_ENTER,
> > + KEY_LEFTSHIFT, KEY_9, KEY_8, KEY_I, KEY_K, KEY_B, 63, KEY_COMMA,
> > +};
>
> Why do you use fixed keycode? Dose it provided from board specific
> platform? and If we want to use only 3 * 3 keypad then how do you
> handle this one?
>
That keycode array is just default value.
The input device layer supports EVIOCGKEYCODE and EVIOCSKEYCODE ioctls to change keymap by user application.
> > +
> > +struct s3c_keypad {
> > + struct s3c_platform_keypad *pdata;
> > + unsigned char keycodes[ARRAY_SIZE(s3c_keycode)];
>
> We don't need full keycode array here. It should be allocated with
> required size.
>
> > + struct input_dev *dev;
> > + struct timer_list timer;
> > + void __iomem *regs;
> > + struct clk *clk;
> > + int irq;
> > + int timer_enabled;
> > + unsigned int prevmask_low;
> > + unsigned int prevmask_high;
> > + unsigned int keyifcon;
> > + unsigned int keyiffc;
> > +};
> > +
> > +static int s3c_keypad_scan(struct s3c_keypad *keypad, u32 *keymask_low,
> > + u32 *keymask_high)
> > +{
> > + struct s3c_platform_keypad *pdata = keypad->pdata;
> > + int i, j = 0;
> > + u32 cval, rval, cfg;
> > +
> > + for (i = 0; i < pdata->nr_cols; i++) {
> > + cval = readl(keypad->regs + S3C_KEYIFCOL);
> > + cval |= S3C_KEYIF_COL_DMASK;
> > + cval &= ~(1 << i);
> > + writel(cval, keypad->regs + S3C_KEYIFCOL);
> > + udelay(pdata->delay);
> > +
> > + rval = ~(readl(keypad->regs + S3C_KEYIFROW)) &
> > + S3C_KEYIF_ROW_DMASK;
> > +
> > + if ((i * pdata->nr_rows) < pdata->max_masks)
> > + *keymask_low |= (rval << (i * pdata->nr_rows));
> > + else {
> > + *keymask_high |= (rval << (j * pdata->nr_rows));
> > + j++;
> > + }
> > + }
> > +
> > + cfg = readl(keypad->regs + S3C_KEYIFCOL);
> > + cfg &= ~S3C_KEYIF_COL_MASK_ALL;
> > + writel(cfg, keypad->regs + S3C_KEYIFCOL);
> > +
> > + return 0;
> > +}
> > +
> > +static void s3c_keypad_timer_handler(unsigned long data)
> > +{
> > + struct s3c_keypad *keypad = (struct s3c_keypad *)data;
> > + struct s3c_platform_keypad *pdata = keypad->pdata;
> > + struct input_dev *input = keypad->dev;
> > + u32 keymask_low = 0, keymask_high = 0;
> > + u32 press_mask_low, press_mask_high;
> > + u32 release_mask_low, release_mask_high, code, cfg;
> > + int i;
> > +
> > + s3c_keypad_scan(keypad, &keymask_low, &keymask_high);
> > +
> > + if (keymask_low != keypad->prevmask_low) {
> > + press_mask_low = ((keymask_low ^ keypad->prevmask_low) &
> > + keymask_low);
> > + release_mask_low = ((keymask_low ^ keypad->prevmask_low) &
> > + keypad->prevmask_low);
> > +
> > + i = 0;
> > + while (press_mask_low) {
> > + if (press_mask_low & 1) {
> > + code = keypad->keycodes[i];
> > + input_report_key(input, code, 1);
> > + dev_dbg(&input->dev, "low pressed: %d\n", i);
> > + }
> > + press_mask_low >>= 1;
> > + i++;
> > + }
> > +
> > + i = 0;
> > + while (release_mask_low) {
> > + if (release_mask_low & 1) {
> > + code = keypad->keycodes[i];
> > + input_report_key(input, code, 0);
> > + dev_dbg(&input->dev, "low released : %d\n",
> i);
> > + }
> > + release_mask_low >>= 1;
> > + i++;
> > + }
> > + keypad->prevmask_low = keymask_low;
> > + }
> > +
> > + if (keymask_high != keypad->prevmask_high) {
> > + press_mask_high = ((keymask_high ^ keypad->prevmask_high)
> &
> > + keymask_high);
> > + release_mask_high = ((keymask_high ^ keypad->prevmask_high)
> &
> > + keypad->prevmask_high);
> > +
> > + i = 0;
> > + while (press_mask_high) {
> > + if (press_mask_high & 1) {
> > + code = keypad->keycodes[i + pdata-
> >max_masks];
> > + input_report_key(input, code, 1);
> > + dev_dbg(&input->dev, "high pressed: %d %d\n",
> > + keypad->keycodes[i + pdata-
> >max_masks],
> > + i);
> > + }
> > + press_mask_high >>= 1;
> > + i++;
> > + }
> > +
> > + i = 0;
> > + while (release_mask_high) {
> > + if (release_mask_high & 1) {
> > + code = keypad->keycodes[i + pdata-
> >max_masks];
> > + input_report_key(input, code, 0);
> > + dev_dbg(&input->dev, "high released: %d\n",
> > + keypad->keycodes[i + pdata-
> >max_masks]);
> > + }
> > + release_mask_high >>= 1;
> > + i++;
> > + }
> > + keypad->prevmask_high = keymask_high;
> > + }
> > +
> > + 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;
> > + }
> > +}
> > +
> > +static irqreturn_t s3c_keypad_irq(int irq, void *dev_id)
> > +{
> > + struct s3c_keypad *keypad = dev_id;
> > + u32 cfg;
> > +
> > + /* disable keypad interrupt and schedule for keypad timer handler
> */
> > + cfg = readl(keypad->regs + S3C_KEYIFCON);
> > + cfg &= ~(S3C_KEYIF_INT_F_EN | S3C_KEYIF_INT_R_EN);
> > + writel(cfg, keypad->regs + S3C_KEYIFCON);
> > +
> > + 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;
> > + }
> > +
> > + /* clear the keypad interrupt status */
> > + writel(S3C_KEYIF_STSCLR_CLEAR, keypad->regs + S3C_KEYIFSTSCLR);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> Why do you use timer. As other input drivers how about to use workqueue?
>
> > +
> > +static int s3c_keypad_open(struct input_dev *dev)
> > +{
> > + struct s3c_keypad *keypad = input_get_drvdata(dev);
> > + u32 cfg;
> > +
> > + clk_enable(keypad->clk);
> > +
> > + /* init keypad h/w block */
> > + 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);
> > +
> > + cfg = readl(keypad->regs + S3C_KEYIFFC);
> > + cfg |= 0x1;
>
> What's the meaning of '0x1'?
>
> > + writel(cfg, keypad->regs + S3C_KEYIFFC);
> > +
> > + cfg = readl(keypad->regs + S3C_KEYIFCOL);
> > + cfg &= ~S3C_KEYIF_COL_MASK_ALL;
> > + writel(cfg, keypad->regs + S3C_KEYIFCOL);
> > +
> > + /* Scan timer init */
> > + init_timer(&keypad->timer);
> > + keypad->timer.function = s3c_keypad_timer_handler;
> > + keypad->timer.data = (unsigned long)keypad;
> > + keypad->timer.expires = jiffies + (HZ / 10);
> > +
> > + if (keypad->timer_enabled) {
> > + mod_timer(&keypad->timer, keypad->timer.expires);
> > + } else {
>
> useless curly braces
>
> > + add_timer(&keypad->timer);
> > + keypad->timer_enabled = 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void s3c_keypad_close(struct input_dev *dev)
> > +{
> > + struct s3c_keypad *keypad = input_get_drvdata(dev);
> > +
> > + clk_disable(keypad->clk);
> > +}
> > +
> > +#define res_size(res) ((res)->end - (res)->start + 1)
> > +
> > +static int __devinit s3c_keypad_probe(struct platform_device *pdev)
> > +{
> > + struct s3c_platform_keypad *pdata;
> > + struct s3c_keypad *keypad;
> > + struct input_dev *input;
> > + struct resource *res;
> > + int irq, error, i;
> > +
> > + pdata = pdev->dev.platform_data;
> > + if (pdata == NULL) {
> > + dev_err(&pdev->dev, "no platform data\n");
> > + return -EINVAL;
> > + }
> > +
> > + keypad = kzalloc(sizeof(struct s3c_keypad), GFP_KERNEL);
> > + if (keypad == NULL) {
> > + dev_err(&pdev->dev, "failed to allocate driver data\n");
> > + return -ENOMEM;
> > + }
> > +
> > + keypad->pdata = pdata;
> > + memcpy(keypad->keycodes, s3c_keycode, sizeof(keypad->keycodes));
>
> memcpy??? if you don't modify s3c_keycode. just assign it.
>
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res == NULL) {
> > + dev_err(&pdev->dev, "failed to get I/O memory\n");
> > + error = -ENXIO;
> > + goto err_get_io;
> > + }
> > +
> > + res = request_mem_region(res->start, res_size(res), pdev->name);
> > + if (res == NULL) {
> > + dev_err(&pdev->dev, "failed to request I/O memory\n");
> > + error = -EBUSY;
> > + goto err_get_io;
> > + }
> > +
> > + keypad->regs = ioremap(res->start, res_size(res));
> > + if (keypad->regs == NULL) {
> > + dev_err(&pdev->dev, "failed to remap I/O memory\n");
> > + error = -ENXIO;
> > + goto err_map_io;
> > + }
> > +
> > + keypad->clk = clk_get(&pdev->dev, "keypad");
> > + if (IS_ERR(keypad->clk)) {
> > + dev_err(&pdev->dev, "failed to get keypad clock\n");
> > + error = PTR_ERR(keypad->clk);
> > + goto err_clk;
> > + }
> > +
> > + /* Create and register the input driver. */
> > + input = input_allocate_device();
> > + if (!input) {
> > + dev_err(&pdev->dev, "failed to allocate input device\n");
> > + error = -ENOMEM;
> > + goto err_alloc_input;
> > + }
> > +
> > + input->name = pdev->name;
> > + input->id.bustype = BUS_HOST;
> > + input->open = s3c_keypad_open;
> > + input->close = s3c_keypad_close;
> > + input->dev.parent = &pdev->dev;
> > + input->keycode = (void *)keypad->keycodes;
> > + input->keycodesize = sizeof(keypad->keycodes[0]);
> > + input->keycodemax = ARRAY_SIZE(keypad->keycodes);
> > +
> > + keypad->dev = input;
> > +
> > + input_set_drvdata(input, keypad);
> > +
> > + __set_bit(EV_KEY, input->evbit);
> > + __set_bit(EV_REP, input->evbit);
> > +
> > + for (i = 0; i < pdata->max_keys; i++) {
> > + keypad->keycodes[i] = s3c_keycode[i];
> > + if (keypad->keycodes[i] <= 0)
> > + continue;
> > +
> > + __set_bit(keypad->keycodes[i] & KEY_MAX, input->keybit);
> > + }
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(&pdev->dev, "failed to get keypad irq\n");
> > + error = -ENXIO;
> > + goto err_get_irq;
> > + }
> > +
> > + platform_set_drvdata(pdev, keypad);
> > +
> > + error = request_irq(irq, s3c_keypad_irq, IRQF_DISABLED,
> > + pdev->name, keypad);
> > + if (error) {
> > + dev_err(&pdev->dev, "failed to request IRQ\n");
> > + goto err_req_irq;
> > + }
> > +
> > + keypad->irq = irq;
> > +
> > + /* 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;
> > + }
> > +
> > + device_init_wakeup(&pdev->dev, 1);
> > + dev_info(&pdev->dev, "Samsung Keypad Interface driver\n");
> > +
> > + return 0;
> > +
> > +err_reg_input:
> > + free_irq(irq, pdev);
> > +
> > +err_req_irq:
> > + platform_set_drvdata(pdev, NULL);
> > +
> > +err_get_irq:
> > + input_free_device(input);
> > +
> > +err_alloc_input:
> > + clk_put(keypad->clk);
> > +
> > +err_clk:
> > + iounmap(keypad->regs);
> > +
> > +err_map_io:
> > + release_mem_region(res->start, res_size(res));
> > +
> > +err_get_io:
> > + kfree(keypad);
> > +
> > + return error;
> > +}
> > +
> > +static int __devexit s3c_keypad_remove(struct platform_device *pdev)
> > +{
> > + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> > + struct resource *res;
> > +
> > + free_irq(keypad->irq, pdev);
> > +
> > + clk_disable(keypad->clk);
> > + clk_put(keypad->clk);
> > +
> > + input_unregister_device(keypad->dev);
> > + input_free_device(keypad->dev);
> > +
> > + iounmap(keypad->regs);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + release_mem_region(res->start, res_size(res));
> > +
> > + platform_set_drvdata(pdev, NULL);
> > + kfree(keypad);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int s3c_keypad_suspend(struct platform_device *dev, pm_message_t
> state)
> > +{
> > + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> > +
> > + keypad->keyifcon = readl(keypad->regs + S3C_KEYIFCON);
> > + keypad->keyiffc = readl(keypad->regs + S3C_KEYIFFC);
> > +
> > + disable_irq(IRQ_KEYPAD);
> > + clk_disable(keypad->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int s3c_keypad_resume(struct platform_device *dev)
> > +{
> > + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> > +
> > + clk_enable(keypad->clock);
> > +
> > + writel(keypad->keyifcon, keypad->regs + S3C_KEYIFCON);
> > + writel(keypad->keyiffc, keypad->regs + S3C_KEYIFFC);
> > +
> > + enable_irq(IRQ_KEYPAD);
> > +
> > + return 0;
> > +}
> > +#else
> > +#define s3c_keypad_suspend NULL
> > +#define s3c_keypad_resume NULL
> > +#endif /* CONFIG_PM */
> > +
> > +static struct platform_driver s3c_keypad_driver = {
> > + .probe = s3c_keypad_probe,
> > + .remove = s3c_keypad_remove,
> > + .suspend = s3c_keypad_suspend,
> > + .resume = s3c_keypad_resume,
> > + .driver = {
> > + .owner = THIS_MODULE,
> > + .name = "s3c-keypad",
> > + },
> > +};
> > +
> > +static int __init s3c_keypad_init(void)
> > +{
> > + return platform_driver_register(&s3c_keypad_driver);
> > +}
> > +
> > +static void __exit s3c_keypad_exit(void)
> > +{
> > + platform_driver_unregister(&s3c_keypad_driver);
> > +}
> > +
> > +module_init(s3c_keypad_init);
> > +module_exit(s3c_keypad_exit);
> > +
> > +MODULE_AUTHOR("Kyeongil, Kim <ki0351.kim@samsung.com>");
> > +MODULE_AUTHOR("Jinsung, Yang <jsgood.yang@samsung.com>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Keypad interface for Samsung SoC");
> > +
>
> Finally,
> We did duplicated works. We already implement the keypad driver for
> s5pc100 & s5pc110. but we use workqueue structure instead of timer.
>
> I will post the our works.
>
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-05 13:55 [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs 양진성
2009-09-05 16:17 ` Mark Brown
2009-09-07 3:25 ` Kyungmin Park
@ 2009-09-07 5:38 ` Joonyoung Shim
2009-09-07 6:33 ` Harald Welte
2009-09-07 7:48 ` Dmitry Torokhov
2009-09-07 8:00 ` Dmitry Torokhov
3 siblings, 2 replies; 26+ messages in thread
From: Joonyoung Shim @ 2009-09-07 5:38 UTC (permalink / raw)
To: 양진성
Cc: linux-input, laforge, ben-linux,
"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자",
"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'",
dmitry.torokhov, kyungmin.park, jh80.chung
On 9/5/2009 10:55 PM, 양진성 wrote:
> This keypad driver supports Samsung s3c based SoCs such as s3c6410.
> This driver is written with input device compatibles.
>
> Signed-off-by: Jinsung Yang <jsgood.yang@samsung.com>
> Signed-off-by: Kyeongil Kim <ki0351.kim@samsung.com>
> ---
> drivers/input/keyboard/s3c-keypad.c | 468 +++++++++++++++++++++++++++++++++++
> 1 files changed, 468 insertions(+), 0 deletions(-)
> create mode 100644 drivers/input/keyboard/s3c-keypad.c
Hi,
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.
From 0582cd271637aaebed29f2982f941bcf80d84179 Mon Sep 17 00:00:00 2001
From: Joonyoung Shim <jy0922.shim@samsung.com>
Date: Mon, 7 Sep 2009 14:35:12 +0900
Subject: [PATCH] Input: add samsung keypad driver for S3C64XX and S5PC1XX cpu
This patch adds support for keypad driver running on Samsung S3C64XX and
S5PC1XX cpu.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
drivers/input/keyboard/Kconfig | 9 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/samsung-keypad.c | 366 +++++++++++++++++++++++++++++++
3 files changed, 376 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/keyboard/samsung-keypad.c
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");
--
1.6.0.4
--
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 3:25 ` Kyungmin Park
2009-09-07 3:50 ` Jinsung Yang
@ 2009-09-07 6:26 ` Harald Welte
2009-09-07 6:59 ` Kyungmin Park
1 sibling, 1 reply; 26+ messages in thread
From: Harald Welte @ 2009-09-07 6:26 UTC (permalink / raw)
To: Kyungmin Park
Cc: 양진성, linux-input, ben-linux,
김경일/AP개발팀/E3/삼성전자,
김국진/AP개발팀/E5/삼성전자
Dear all,
On Mon, Sep 07, 2009 at 12:25:14PM +0900, Kyungmin Park wrote:
> > +static const unsigned char s3c_keycode[] = {
> > + 1, 2, KEY_1, KEY_Q, KEY_A, 6, 7, KEY_LEFT,
> > + 9, 10, KEY_2, KEY_W, KEY_S, KEY_Z, KEY_RIGHT, 16,
> > + 17, 18, KEY_3, KEY_E, KEY_D, KEY_X, 23, KEY_UP,
> > + 25, 26, KEY_4, KEY_R, KEY_F, KEY_C, 31, 32,
> > + 33, KEY_O, KEY_5, KEY_T, KEY_G, KEY_V, KEY_DOWN, KEY_BACKSPACE,
> > + KEY_P, KEY_0, KEY_6, KEY_Y, KEY_H, KEY_SPACE, 47, 48,
> > + KEY_M, KEY_L, KEY_7, KEY_U, KEY_J, KEY_N, 55, KEY_ENTER,
> > + KEY_LEFTSHIFT, KEY_9, KEY_8, KEY_I, KEY_K, KEY_B, 63, KEY_COMMA,
> > +};
>
> Why do you use fixed keycode? Dose it provided from board specific
> platform? and If we want to use only 3 * 3 keypad then how do you
> handle this one?
As Jinsung mailed, the keymap can always be reconfigured from userspace.
Whether or not the board specific code wants to set a different default keymap
is probably a matter of taste. omap-kbd has it, pxa27x_kbd also has it. So
yes, there is some reason to align with what they are doing.
> > +struct s3c_keypad {
> > + struct s3c_platform_keypad *pdata;
> > + unsigned char keycodes[ARRAY_SIZE(s3c_keycode)];
>
> We don't need full keycode array here. It should be allocated with
> required size.
I think this is really an implementation detail up to the author. Having
a static array with the maximum size (8*8 == 64) is not a big waste of
memory, if you can on the other hand save some code complexity for dynamic
kmalloc()/kfree() and the associated error paths.
> Why do you use timer. As other input drivers how about to use workqueue?
Sorry, what exactly do you mean? I see many (if not all) other keypad drivers
also using a timer.
> > +static int s3c_keypad_open(struct input_dev *dev)
> > +{
> > + struct s3c_keypad *keypad = input_get_drvdata(dev);
> > + u32 cfg;
> > +
> > + clk_enable(keypad->clk);
> > +
> > + /* init keypad h/w block */
> > + 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);
> > +
> > + cfg = readl(keypad->regs + S3C_KEYIFFC);
> > + cfg |= 0x1;
>
> What's the meaning of '0x1'?
I agree. Jinsung: The 0x1 should not be a number but a #define from
the register definition.
> > + if (keypad->timer_enabled) {
> > + mod_timer(&keypad->timer, keypad->timer.expires);
> > + } else {
>
> useless curly braces
That's what I said, too. However, the kernel CodingStyle document explicitly
states that if there is an 'else' block with multiple lines, the first block
should also have curly braces. Despite this, I have not seen it much in use.
> > + keypad->pdata = pdata;
> > + memcpy(keypad->keycodes, s3c_keycode, sizeof(keypad->keycodes));
>
> memcpy??? if you don't modify s3c_keycode. just assign it.
keypad->keycodes is a pointer to the keycode array in the s3c private data
structure. This memory can be changed from userspace by using the keypad
set/get ioctl's (e.g. 'loadkeys' command).
s3c_keycode is a const array of the default keymap that is loaded during boot.
right now there is only one 6410 specific default keymap, but as you suggested
there should probably be a platform_data (board specific) default keymap.
In any case, I personally believe the memcpy to memory that is owned by
the driver is a good idea, since userspace can modify it.
> Finally, We did duplicated works. We already implement the keypad driver for
> s5pc100 & s5pc110. but we use workqueue structure instead of timer.
This is why System LSI and DMC Lab should coordinate more on what they work.
System LSI's kernel tree is on git.kernel.org and updated every night, so you
(or other interested parties) can stay up-to-date with what is happening. If
you base your work on top of System LSI's tree, you would always follow their
work. System LSI's tree is what is scheduled to be submitted to mainline.
> I will post the our works.
I'm not sure if two different Samsung departments posting competing drivers to
the mainline mailing lists will really help. You need to define which group
will be the 'master' inside Samsung (the logical choice would be System LSI).
You should then submit your code / modifications as patches to that master
tree, and System LSI is submitting it mainline.
The same should be the case for other departments who work on Samsung SoC
related kernel code..
Regards,
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
--
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 5:38 ` [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs Joonyoung Shim
@ 2009-09-07 6:33 ` Harald Welte
2009-09-07 7:31 ` Joonyoung Shim
2009-09-07 7:48 ` Dmitry Torokhov
1 sibling, 1 reply; 26+ messages in thread
From: Harald Welte @ 2009-09-07 6:33 UTC (permalink / raw)
To: Joonyoung Shim
Cc: 양진성, linux-input, ben-linux,
김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자,
'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자',
dmitry.torokhov, kyungmin.park, jh80.chung
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,
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 6:26 ` Harald Welte
@ 2009-09-07 6:59 ` Kyungmin Park
2009-09-07 8:02 ` Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs) Harald Welte
0 siblings, 1 reply; 26+ messages in thread
From: Kyungmin Park @ 2009-09-07 6:59 UTC (permalink / raw)
To: Harald Welte
Cc: 양진성, linux-input, ben-linux,
김경일/AP개발팀/E3/삼성전자,
김국진/AP개발팀/E5/삼성전자
2009/9/7 Harald Welte <laforge@gnumonks.org>:
> Dear all,
>
> On Mon, Sep 07, 2009 at 12:25:14PM +0900, Kyungmin Park wrote:
>> > +static const unsigned char s3c_keycode[] = {
>> > + 1, 2, KEY_1, KEY_Q, KEY_A, 6, 7, KEY_LEFT,
>> > + 9, 10, KEY_2, KEY_W, KEY_S, KEY_Z, KEY_RIGHT, 16,
>> > + 17, 18, KEY_3, KEY_E, KEY_D, KEY_X, 23, KEY_UP,
>> > + 25, 26, KEY_4, KEY_R, KEY_F, KEY_C, 31, 32,
>> > + 33, KEY_O, KEY_5, KEY_T, KEY_G, KEY_V, KEY_DOWN, KEY_BACKSPACE,
>> > + KEY_P, KEY_0, KEY_6, KEY_Y, KEY_H, KEY_SPACE, 47, 48,
>> > + KEY_M, KEY_L, KEY_7, KEY_U, KEY_J, KEY_N, 55, KEY_ENTER,
>> > + KEY_LEFTSHIFT, KEY_9, KEY_8, KEY_I, KEY_K, KEY_B, 63, KEY_COMMA,
>> > +};
>>
>> Why do you use fixed keycode? Dose it provided from board specific
>> platform? and If we want to use only 3 * 3 keypad then how do you
>> handle this one?
>
> As Jinsung mailed, the keymap can always be reconfigured from userspace.
>
> Whether or not the board specific code wants to set a different default keymap
> is probably a matter of taste. omap-kbd has it, pxa27x_kbd also has it. So
> yes, there is some reason to align with what they are doing.
Right it's a matter of taste. but I can't see the provided default key
maps. these got the keymap from platform instead of driver itself.
Even though userspace can re-configure it. why give a burden to the
application programmer. In most case board got fixed keymap and
changed based on some scenarios.
>
>> > +struct s3c_keypad {
>> > + struct s3c_platform_keypad *pdata;
>> > + unsigned char keycodes[ARRAY_SIZE(s3c_keycode)];
>>
>> We don't need full keycode array here. It should be allocated with
>> required size.
>
> I think this is really an implementation detail up to the author. Having
> a static array with the maximum size (8*8 == 64) is not a big waste of
> memory, if you can on the other hand save some code complexity for dynamic
> kmalloc()/kfree() and the associated error paths.
With this approaches, it will be increased at s5pc110 since it
provides 14 * 8 matrix. whatever I only used 3 * 3.
>
>> Why do you use timer. As other input drivers how about to use workqueue?
>
> Sorry, what exactly do you mean? I see many (if not all) other keypad drivers
> also using a timer.
Right it's my taste. In my experience timer method is hide some bug at
the omap keypad drivers development.
>
>> > +static int s3c_keypad_open(struct input_dev *dev)
>> > +{
>> > + struct s3c_keypad *keypad = input_get_drvdata(dev);
>> > + u32 cfg;
>> > +
>> > + clk_enable(keypad->clk);
>> > +
>> > + /* init keypad h/w block */
>> > + 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);
>> > +
>> > + cfg = readl(keypad->regs + S3C_KEYIFFC);
>> > + cfg |= 0x1;
>>
>> What's the meaning of '0x1'?
>
> I agree. Jinsung: The 0x1 should not be a number but a #define from
> the register definition.
>
>> > + if (keypad->timer_enabled) {
>> > + mod_timer(&keypad->timer, keypad->timer.expires);
>> > + } else {
>>
>> useless curly braces
>
> That's what I said, too. However, the kernel CodingStyle document explicitly
> states that if there is an 'else' block with multiple lines, the first block
> should also have curly braces. Despite this, I have not seen it much in use.
>
>> > + keypad->pdata = pdata;
>> > + memcpy(keypad->keycodes, s3c_keycode, sizeof(keypad->keycodes));
>>
>> memcpy??? if you don't modify s3c_keycode. just assign it.
>
> keypad->keycodes is a pointer to the keycode array in the s3c private data
> structure. This memory can be changed from userspace by using the keypad
> set/get ioctl's (e.g. 'loadkeys' command).
>
> s3c_keycode is a const array of the default keymap that is loaded during boot.
> right now there is only one 6410 specific default keymap, but as you suggested
> there should probably be a platform_data (board specific) default keymap.
>
> In any case, I personally believe the memcpy to memory that is owned by
> the driver is a good idea, since userspace can modify it.
>
>> Finally, We did duplicated works. We already implement the keypad driver for
>> s5pc100 & s5pc110. but we use workqueue structure instead of timer.
>
> This is why System LSI and DMC Lab should coordinate more on what they work.
>
> System LSI's kernel tree is on git.kernel.org and updated every night, so you
> (or other interested parties) can stay up-to-date with what is happening. If
> you base your work on top of System LSI's tree, you would always follow their
> work. System LSI's tree is what is scheduled to be submitted to mainline.
Even though it's working at Samsung SoCs. but it's not mean it fits
the mainline kernel.
It's based on SMDK board and focused on SMDK.
I mean it's not generic. Some codes are hard-coded and don't call
proper frameworks APIs. especially clocks
>
>> I will post the our works.
>
> I'm not sure if two different Samsung departments posting competing drivers to
> the mainline mailing lists will really help. You need to define which group
> will be the 'master' inside Samsung (the logical choice would be System LSI).
It's simple. give a choice to customers if there are two versions. you
think it's some duplicated works
it's better to others.
I don't hope just push their codes to mainline with these discussion.
Thank you,
Kyungmin Park
>
> You should then submit your code / modifications as patches to that master
> tree, and System LSI is submitting it mainline.
>
> The same should be the case for other departments who work on Samsung SoC
> related kernel code..
>
> Regards,
> --
> - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
> ============================================================================
> "Privacy in residential applications is a desirable marketing option."
> (ETSI EN 300 175-7 Ch. A6)
>
--
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 6:33 ` Harald Welte
@ 2009-09-07 7:31 ` Joonyoung Shim
2009-09-07 11:32 ` Jinsung Yang
0 siblings, 1 reply; 26+ messages in thread
From: Joonyoung Shim @ 2009-09-07 7:31 UTC (permalink / raw)
To: Harald Welte
Cc: 양진성, linux-input, ben-linux,
"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자",
"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'",
dmitry.torokhov, kyungmin.park, jh80.chung
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,
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 5:38 ` [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs Joonyoung Shim
2009-09-07 6:33 ` Harald Welte
@ 2009-09-07 7:48 ` Dmitry Torokhov
2009-09-07 8:40 ` Joonyoung Shim
1 sibling, 1 reply; 26+ messages in thread
From: Dmitry Torokhov @ 2009-09-07 7:48 UTC (permalink / raw)
To: Joonyoung Shim
Cc: 양진성, linux-input, laforge, ben-linux,
"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자",
"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'",
kyungmin.park, jh80.chung
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
--
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-05 13:55 [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs 양진성
` (2 preceding siblings ...)
2009-09-07 5:38 ` [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs Joonyoung Shim
@ 2009-09-07 8:00 ` Dmitry Torokhov
3 siblings, 0 replies; 26+ messages in thread
From: Dmitry Torokhov @ 2009-09-07 8:00 UTC (permalink / raw)
To: 양진성
Cc: linux-input, laforge, ben-linux,
김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자,
'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'
Hi,
On Sat, Sep 05, 2009 at 10:55:24PM +0900, 양진성 wrote:
> This keypad driver supports Samsung s3c based SoCs such as s3c6410.
> This driver is written with input device compatibles.
>
> Signed-off-by: Jinsung Yang <jsgood.yang@samsung.com>
> Signed-off-by: Kyeongil Kim <ki0351.kim@samsung.com>
> ---
> drivers/input/keyboard/s3c-keypad.c | 468 +++++++++++++++++++++++++++++++++++
> 1 files changed, 468 insertions(+), 0 deletions(-)
> create mode 100644 drivers/input/keyboard/s3c-keypad.c
>
> diff --git a/drivers/input/keyboard/s3c-keypad.c b/drivers/input/keyboard/s3c-keypad.c
> new file mode 100644
> index 0000000..73b9a90
> --- /dev/null
> +++ b/drivers/input/keyboard/s3c-keypad.c
> @@ -0,0 +1,468 @@
> +/*
> + * linux/drivers/input/keyboard/s3c_keypad.c
> + *
> + * Driver for Samsung SoC matrix keypad controller.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +
> +#include <mach/hardware.h>
> +#include <asm/irq.h>
> +
> +#include <plat/keypad.h>
> +#include <plat/regs-keypad.h>
> +
> +#undef DEBUG
> +
> +static const unsigned char s3c_keycode[] = {
> + 1, 2, KEY_1, KEY_Q, KEY_A, 6, 7, KEY_LEFT,
> + 9, 10, KEY_2, KEY_W, KEY_S, KEY_Z, KEY_RIGHT, 16,
> + 17, 18, KEY_3, KEY_E, KEY_D, KEY_X, 23, KEY_UP,
> + 25, 26, KEY_4, KEY_R, KEY_F, KEY_C, 31, 32,
> + 33, KEY_O, KEY_5, KEY_T, KEY_G, KEY_V, KEY_DOWN, KEY_BACKSPACE,
> + KEY_P, KEY_0, KEY_6, KEY_Y, KEY_H, KEY_SPACE, 47, 48,
> + KEY_M, KEY_L, KEY_7, KEY_U, KEY_J, KEY_N, 55, KEY_ENTER,
> + KEY_LEFTSHIFT, KEY_9, KEY_8, KEY_I, KEY_K, KEY_B, 63, KEY_COMMA,
> +};
Please use KEY_* consistently. Also make it unsigned short, some KEY_*
defines afre greater than 255.
> +
> +struct s3c_keypad {
> + struct s3c_platform_keypad *pdata;
> + unsigned char keycodes[ARRAY_SIZE(s3c_keycode)];
> + struct input_dev *dev;
> + struct timer_list timer;
> + void __iomem *regs;
> + struct clk *clk;
> + int irq;
> + int timer_enabled;
> + unsigned int prevmask_low;
> + unsigned int prevmask_high;
> + unsigned int keyifcon;
> + unsigned int keyiffc;
> +};
> +
> +static int s3c_keypad_scan(struct s3c_keypad *keypad, u32 *keymask_low,
> + u32 *keymask_high)
> +{
> + struct s3c_platform_keypad *pdata = keypad->pdata;
> + int i, j = 0;
> + u32 cval, rval, cfg;
> +
> + for (i = 0; i < pdata->nr_cols; i++) {
> + cval = readl(keypad->regs + S3C_KEYIFCOL);
> + cval |= S3C_KEYIF_COL_DMASK;
> + cval &= ~(1 << i);
> + writel(cval, keypad->regs + S3C_KEYIFCOL);
> + udelay(pdata->delay);
> +
> + rval = ~(readl(keypad->regs + S3C_KEYIFROW)) &
> + S3C_KEYIF_ROW_DMASK;
> +
> + if ((i * pdata->nr_rows) < pdata->max_masks)
> + *keymask_low |= (rval << (i * pdata->nr_rows));
> + else {
> + *keymask_high |= (rval << (j * pdata->nr_rows));
> + j++;
> + }
> + }
> +
> + cfg = readl(keypad->regs + S3C_KEYIFCOL);
> + cfg &= ~S3C_KEYIF_COL_MASK_ALL;
> + writel(cfg, keypad->regs + S3C_KEYIFCOL);
> +
> + return 0;
> +}
> +
> +static void s3c_keypad_timer_handler(unsigned long data)
> +{
> + struct s3c_keypad *keypad = (struct s3c_keypad *)data;
> + struct s3c_platform_keypad *pdata = keypad->pdata;
> + struct input_dev *input = keypad->dev;
> + u32 keymask_low = 0, keymask_high = 0;
> + u32 press_mask_low, press_mask_high;
> + u32 release_mask_low, release_mask_high, code, cfg;
> + int i;
> +
> + s3c_keypad_scan(keypad, &keymask_low, &keymask_high);
> +
> + if (keymask_low != keypad->prevmask_low) {
> + press_mask_low = ((keymask_low ^ keypad->prevmask_low) &
> + keymask_low);
> + release_mask_low = ((keymask_low ^ keypad->prevmask_low) &
> + keypad->prevmask_low);
> +
> + i = 0;
> + while (press_mask_low) {
> + if (press_mask_low & 1) {
> + code = keypad->keycodes[i];
> + input_report_key(input, code, 1);
> + dev_dbg(&input->dev, "low pressed: %d\n", i);
> + }
> + press_mask_low >>= 1;
> + i++;
> + }
> +
> + i = 0;
> + while (release_mask_low) {
> + if (release_mask_low & 1) {
> + code = keypad->keycodes[i];
> + input_report_key(input, code, 0);
> + dev_dbg(&input->dev, "low released : %d\n", i);
> + }
> + release_mask_low >>= 1;
> + i++;
> + }
> + keypad->prevmask_low = keymask_low;
> + }
> +
> + if (keymask_high != keypad->prevmask_high) {
> + press_mask_high = ((keymask_high ^ keypad->prevmask_high) &
> + keymask_high);
> + release_mask_high = ((keymask_high ^ keypad->prevmask_high) &
> + keypad->prevmask_high);
> +
> + i = 0;
> + while (press_mask_high) {
> + if (press_mask_high & 1) {
> + code = keypad->keycodes[i + pdata->max_masks];
> + input_report_key(input, code, 1);
> + dev_dbg(&input->dev, "high pressed: %d %d\n",
> + keypad->keycodes[i + pdata->max_masks],
> + i);
> + }
> + press_mask_high >>= 1;
> + i++;
> + }
> +
> + i = 0;
> + while (release_mask_high) {
> + if (release_mask_high & 1) {
> + code = keypad->keycodes[i + pdata->max_masks];
> + input_report_key(input, code, 0);
> + dev_dbg(&input->dev, "high released: %d\n",
> + keypad->keycodes[i + pdata->max_masks]);
> + }
> + release_mask_high >>= 1;
> + i++;
> + }
> + keypad->prevmask_high = keymask_high;
> + }
> +
> + 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;
> + }
> +}
> +
> +static irqreturn_t s3c_keypad_irq(int irq, void *dev_id)
> +{
> + struct s3c_keypad *keypad = dev_id;
> + u32 cfg;
> +
> + /* disable keypad interrupt and schedule for keypad timer handler */
> + cfg = readl(keypad->regs + S3C_KEYIFCON);
> + cfg &= ~(S3C_KEYIF_INT_F_EN | S3C_KEYIF_INT_R_EN);
> + writel(cfg, keypad->regs + S3C_KEYIFCON);
> +
> + 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;
Just use mod_timer unconditionally. Also, do you always need debounce?
> + }
> +
> + /* clear the keypad interrupt status */
> + writel(S3C_KEYIF_STSCLR_CLEAR, keypad->regs + S3C_KEYIFSTSCLR);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int s3c_keypad_open(struct input_dev *dev)
> +{
> + struct s3c_keypad *keypad = input_get_drvdata(dev);
> + u32 cfg;
> +
> + clk_enable(keypad->clk);
> +
> + /* init keypad h/w block */
> + 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);
> +
> + cfg = readl(keypad->regs + S3C_KEYIFFC);
> + cfg |= 0x1;
> + writel(cfg, keypad->regs + S3C_KEYIFFC);
> +
> + cfg = readl(keypad->regs + S3C_KEYIFCOL);
> + cfg &= ~S3C_KEYIF_COL_MASK_ALL;
> + writel(cfg, keypad->regs + S3C_KEYIFCOL);
> +
--->
> + /* Scan timer init */
> + init_timer(&keypad->timer);
> + keypad->timer.function = s3c_keypad_timer_handler;
> + keypad->timer.data = (unsigned long)keypad;
> + keypad->timer.expires = jiffies + (HZ / 10);
<---
This should go into probe. I think we have setup_timer() as well.
> +
> + if (keypad->timer_enabled) {
> + mod_timer(&keypad->timer, keypad->timer.expires);
> + } else {
> + add_timer(&keypad->timer);
> + keypad->timer_enabled = 1;
> + }
Why do we do this here?
> +
> + return 0;
> +}
> +
> +static void s3c_keypad_close(struct input_dev *dev)
> +{
> + struct s3c_keypad *keypad = input_get_drvdata(dev);
> +
> + clk_disable(keypad->clk);
> +}
> +
> +#define res_size(res) ((res)->end - (res)->start + 1)
> +
> +static int __devinit s3c_keypad_probe(struct platform_device *pdev)
> +{
> + struct s3c_platform_keypad *pdata;
> + struct s3c_keypad *keypad;
> + struct input_dev *input;
> + struct resource *res;
> + int irq, error, i;
> +
> + pdata = pdev->dev.platform_data;
> + if (pdata == NULL) {
> + dev_err(&pdev->dev, "no platform data\n");
> + return -EINVAL;
> + }
> +
> + keypad = kzalloc(sizeof(struct s3c_keypad), GFP_KERNEL);
> + if (keypad == NULL) {
> + dev_err(&pdev->dev, "failed to allocate driver data\n");
> + return -ENOMEM;
> + }
> +
> + keypad->pdata = pdata;
> + memcpy(keypad->keycodes, s3c_keycode, sizeof(keypad->keycodes));
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "failed to get I/O memory\n");
> + error = -ENXIO;
> + goto err_get_io;
> + }
> +
> + res = request_mem_region(res->start, res_size(res), pdev->name);
> + if (res == NULL) {
> + dev_err(&pdev->dev, "failed to request I/O memory\n");
> + error = -EBUSY;
> + goto err_get_io;
> + }
> +
> + keypad->regs = ioremap(res->start, res_size(res));
> + if (keypad->regs == NULL) {
> + dev_err(&pdev->dev, "failed to remap I/O memory\n");
> + error = -ENXIO;
> + goto err_map_io;
> + }
> +
> + keypad->clk = clk_get(&pdev->dev, "keypad");
> + if (IS_ERR(keypad->clk)) {
> + dev_err(&pdev->dev, "failed to get keypad clock\n");
> + error = PTR_ERR(keypad->clk);
> + goto err_clk;
> + }
> +
> + /* Create and register the input driver. */
> + input = input_allocate_device();
> + if (!input) {
> + dev_err(&pdev->dev, "failed to allocate input device\n");
> + error = -ENOMEM;
> + goto err_alloc_input;
> + }
> +
> + input->name = pdev->name;
> + input->id.bustype = BUS_HOST;
> + input->open = s3c_keypad_open;
> + input->close = s3c_keypad_close;
> + input->dev.parent = &pdev->dev;
> + input->keycode = (void *)keypad->keycodes;
Why cast?
> + input->keycodesize = sizeof(keypad->keycodes[0]);
> + input->keycodemax = ARRAY_SIZE(keypad->keycodes);
> +
> + keypad->dev = input;
> +
> + input_set_drvdata(input, keypad);
> +
> + __set_bit(EV_KEY, input->evbit);
> + __set_bit(EV_REP, input->evbit);
> +
> + for (i = 0; i < pdata->max_keys; i++) {
> + keypad->keycodes[i] = s3c_keycode[i];
> + if (keypad->keycodes[i] <= 0)
> + continue;
Don't check, just do "__clear_bit(KEY_RESERVED. input_keybit);" after
the loop.
> +
> + __set_bit(keypad->keycodes[i] & KEY_MAX, input->keybit);
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "failed to get keypad irq\n");
> + error = -ENXIO;
> + goto err_get_irq;
> + }
> +
> + platform_set_drvdata(pdev, keypad);
> +
> + error = request_irq(irq, s3c_keypad_irq, IRQF_DISABLED,
> + pdev->name, keypad);
> + if (error) {
> + dev_err(&pdev->dev, "failed to request IRQ\n");
> + goto err_req_irq;
> + }
> +
> + keypad->irq = irq;
> +
> + /* 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;
> + }
> +
> + device_init_wakeup(&pdev->dev, 1);
> + dev_info(&pdev->dev, "Samsung Keypad Interface driver\n");
> +
> + return 0;
> +
> +err_reg_input:
> + free_irq(irq, pdev);
> +
> +err_req_irq:
> + platform_set_drvdata(pdev, NULL);
> +
> +err_get_irq:
> + input_free_device(input);
> +
> +err_alloc_input:
> + clk_put(keypad->clk);
> +
> +err_clk:
> + iounmap(keypad->regs);
> +
> +err_map_io:
> + release_mem_region(res->start, res_size(res));
> +
> +err_get_io:
> + kfree(keypad);
> +
> + return error;
> +}
> +
> +static int __devexit s3c_keypad_remove(struct platform_device *pdev)
> +{
> + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> + struct resource *res;
> +
> + free_irq(keypad->irq, pdev);
> +
> + clk_disable(keypad->clk);
CLock should already be disabled by ->close().
> + clk_put(keypad->clk);
> +
> + input_unregister_device(keypad->dev);
> + input_free_device(keypad->dev);
Don't call input_free_device after input_unregister_device().
> +
> + iounmap(keypad->regs);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(res->start, res_size(res));
> +
> + platform_set_drvdata(pdev, NULL);
> + kfree(keypad);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int s3c_keypad_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> +
> + keypad->keyifcon = readl(keypad->regs + S3C_KEYIFCON);
> + keypad->keyiffc = readl(keypad->regs + S3C_KEYIFFC);
> +
> + disable_irq(IRQ_KEYPAD);
> + clk_disable(keypad->clk);
> +
> + return 0;
> +}
> +
> +static int s3c_keypad_resume(struct platform_device *dev)
> +{
> + struct s3c_keypad *keypad = platform_get_drvdata(pdev);
> +
> + clk_enable(keypad->clock);
> +
> + writel(keypad->keyifcon, keypad->regs + S3C_KEYIFCON);
> + writel(keypad->keyiffc, keypad->regs + S3C_KEYIFFC);
> +
> + enable_irq(IRQ_KEYPAD);
> +
> + return 0;
> +}
> +#else
> +#define s3c_keypad_suspend NULL
> +#define s3c_keypad_resume NULL
> +#endif /* CONFIG_PM */
> +
> +static struct platform_driver s3c_keypad_driver = {
> + .probe = s3c_keypad_probe,
> + .remove = s3c_keypad_remove,
> + .suspend = s3c_keypad_suspend,
> + .resume = s3c_keypad_resume,
dev_pm_ops please.
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "s3c-keypad",
> + },
> +};
> +
> +static int __init s3c_keypad_init(void)
> +{
> + return platform_driver_register(&s3c_keypad_driver);
> +}
> +
> +static void __exit s3c_keypad_exit(void)
> +{
> + platform_driver_unregister(&s3c_keypad_driver);
> +}
> +
> +module_init(s3c_keypad_init);
> +module_exit(s3c_keypad_exit);
> +
> +MODULE_AUTHOR("Kyeongil, Kim <ki0351.kim@samsung.com>");
> +MODULE_AUTHOR("Jinsung, Yang <jsgood.yang@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Keypad interface for Samsung SoC");
> +
> --
> 1.6.2.5
>
>
> --
> 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
--
Dmitry
--
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs)
2009-09-07 6:59 ` Kyungmin Park
@ 2009-09-07 8:02 ` Harald Welte
2009-09-07 8:32 ` Kyungmin Park
0 siblings, 1 reply; 26+ messages in thread
From: Harald Welte @ 2009-09-07 8:02 UTC (permalink / raw)
To: Kyungmin Park
Cc: 양진성, linux-input, ben-linux,
김경일/AP개발팀/E3/삼성전자,
김국진/AP개발팀/E5/삼성전자
Hi Kyungmin,
On Mon, Sep 07, 2009 at 03:59:41PM +0900, Kyungmin Park wrote:
> > System LSI's kernel tree is on git.kernel.org and updated every night, so you
> > (or other interested parties) can stay up-to-date with what is happening. If
> > you base your work on top of System LSI's tree, you would always follow their
> > work. System LSI's tree is what is scheduled to be submitted to mainline.
>
> Even though it's working at Samsung SoCs. but it's not mean it fits
> the mainline kernel. It's based on SMDK board and focused on SMDK.
>
> I mean it's not generic. Some codes are hard-coded and don't call
> proper frameworks APIs. especially clocks
Yes, I agree wity you. Some customers of those SoCs who use Linux also agree :)
This is why the System LSI team needs your input based on your experience!
They will merge your patches if you send them! They will learn as much from
you, as they are from me when I provide feedback.
> >> I will post the our works.
> >
> > I'm not sure if two different Samsung departments posting competing drivers to
> > the mainline mailing lists will really help. You need to define which group
> > will be the 'master' inside Samsung (the logical choice would be System LSI).
>
> It's simple. give a choice to customers if there are two versions. you
> think it's some duplicated works it's better to others.
Don't you think it is weird to see two different Samsung groups duplicating
each others work, creating fragmentation and confusion? Each driver will have
its own bugs and or problems, and right now neither your driver nor System
LSI's driver is in mainline.
So rather than continue to try to compete with each other, you need to work
together. If you think your code is superior, why not submit your code as
patches to System LSI, or even ask System LSI if you can either
1) get commit access to system lsi's git tree
or
2) send pull requests to system LSI and ask them to pull from your tree.
Everyone has limited resources. System LSI, your team, anyone in the
community.
The point is that everyone needs to work together. There needs to be a clear
definition of the process, i.e. who will work on what, based on which tree.
Hwo will submit what mainline, and who will maintain which driver after it has
been merged in mainline..
The fragmentation between many different trees with each their own set of
drivers (let's say so far: Ben dooks / mainline, your code, System LSI's code)
is creating a complexity and maintenance nightmare that nobody can deal with
in the long term.
I understand that System LSI was probably the root cause of this problem
due to their lack of understanding of this process. But this has changed
now.
It is not "they" vs. "you" vs. "us", it is all of us together. There is too
much work to be done. There is 6400,6410,6440,6430,6442,c100,c110 and more
products lining up in the future. Many drivers spanning various subsystems.
Only if the resources are put together this task can be finished in any
reasonable amount of time.
The Linux-aware customer wants to grab one tree (preferrably mainline)
that has all the drivers/features for all SoCs. He doesn't want to look
at 5 different trees, test them, select what he wants and then manually go
merging bits and pieces from each of them.
> I don't hope just push their codes to mainline with these discussion.
Sorry, I don't understand what you're saying here.
System LSI is pushing their code for mainline, one patch at a time, like
we are seeing with this keypad driver right now. This will generate feedback
by the community (including you). System LSI then incorporates that feedback
and re-submit an updated version of the driver - just like any other person
who submits code to Linux mainline.
Regards,
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
--
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs)
2009-09-07 8:02 ` Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs) Harald Welte
@ 2009-09-07 8:32 ` Kyungmin Park
2009-09-07 9:38 ` Harald Welte
0 siblings, 1 reply; 26+ messages in thread
From: Kyungmin Park @ 2009-09-07 8:32 UTC (permalink / raw)
To: Harald Welte
Cc: 양진성, linux-input, ben-linux,
김경일/AP개발팀/E3/삼성전자,
김국진/AP개발팀/E5/삼성전자
2009/9/7 Harald Welte <laforge@gnumonks.org>:
> Hi Kyungmin,
>
> On Mon, Sep 07, 2009 at 03:59:41PM +0900, Kyungmin Park wrote:
>> > System LSI's kernel tree is on git.kernel.org and updated every night, so you
>> > (or other interested parties) can stay up-to-date with what is happening. If
>> > you base your work on top of System LSI's tree, you would always follow their
>> > work. System LSI's tree is what is scheduled to be submitted to mainline.
>>
>> Even though it's working at Samsung SoCs. but it's not mean it fits
>> the mainline kernel. It's based on SMDK board and focused on SMDK.
>>
>> I mean it's not generic. Some codes are hard-coded and don't call
>> proper frameworks APIs. especially clocks
>
> Yes, I agree wity you. Some customers of those SoCs who use Linux also agree :)
>
> This is why the System LSI team needs your input based on your experience!
>
> They will merge your patches if you send them! They will learn as much from
> you, as they are from me when I provide feedback.
It's already sent by Joonyoung,
>
>> >> I will post the our works.
>> >
>> > I'm not sure if two different Samsung departments posting competing drivers to
>> > the mainline mailing lists will really help. You need to define which group
>> > will be the 'master' inside Samsung (the logical choice would be System LSI).
>>
>> It's simple. give a choice to customers if there are two versions. you
>> think it's some duplicated works it's better to others.
>
> Don't you think it is weird to see two different Samsung groups duplicating
> each others work, creating fragmentation and confusion? Each driver will have
> its own bugs and or problems, and right now neither your driver nor System
> LSI's driver is in mainline.
>
> So rather than continue to try to compete with each other, you need to work
> together. If you think your code is superior, why not submit your code as
> patches to System LSI, or even ask System LSI if you can either
It's because we don't get same base code. LSI works on s3c6410, but we
s3c6410, s5pc100 and s5pc110.
we trying to post the patches related with s3c6410 but it takes long
time to merge to Ben's git.
at that we start the s5pc1xx series works.
Anyway. please let me know which the next patch. touchscreen? or others?
Than we can compare the devices sources and discuss internally and then post it.
>
> 1) get commit access to system lsi's git tree
> or
> 2) send pull requests to system LSI and ask them to pull from your tree.
>
> Everyone has limited resources. System LSI, your team, anyone in the
> community.
>
> The point is that everyone needs to work together. There needs to be a clear
> definition of the process, i.e. who will work on what, based on which tree.
> Hwo will submit what mainline, and who will maintain which driver after it has
> been merged in mainline..
>
> The fragmentation between many different trees with each their own set of
> drivers (let's say so far: Ben dooks / mainline, your code, System LSI's code)
> is creating a complexity and maintenance nightmare that nobody can deal with
> in the long term.
>
> I understand that System LSI was probably the root cause of this problem
> due to their lack of understanding of this process. But this has changed
> now.
>
> It is not "they" vs. "you" vs. "us", it is all of us together. There is too
> much work to be done. There is 6400,6410,6440,6430,6442,c100,c110 and more
> products lining up in the future. Many drivers spanning various subsystems.
> Only if the resources are put together this task can be finished in any
> reasonable amount of time.
That's reason to send our codes. Now you focus on the s3c6410 but we
already know the s5pc1xx series.
Please consider it at this work. why does it work twice.
>
> The Linux-aware customer wants to grab one tree (preferrably mainline)
> that has all the drivers/features for all SoCs. He doesn't want to look
> at 5 different trees, test them, select what he wants and then manually go
> merging bits and pieces from each of them.
>
>> I don't hope just push their codes to mainline with these discussion.
>
with -> without
> Sorry, I don't understand what you're saying here.
>
> System LSI is pushing their code for mainline, one patch at a time, like
> we are seeing with this keypad driver right now. This will generate feedback
> by the community (including you). System LSI then incorporates that feedback
> and re-submit an updated version of the driver - just like any other person
> who submits code to Linux mainline.
>
I think no problem to send several patch or drivers parallel. If so we
can get more feedbacks.
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 7:48 ` Dmitry Torokhov
@ 2009-09-07 8:40 ` Joonyoung Shim
2009-09-08 4:44 ` Dmitry Torokhov
0 siblings, 1 reply; 26+ messages in thread
From: Joonyoung Shim @ 2009-09-07 8:40 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: 양진성, linux-input, laforge, ben-linux,
"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자",
"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'",
kyungmin.park, jh80.chung
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs)
2009-09-07 8:32 ` Kyungmin Park
@ 2009-09-07 9:38 ` Harald Welte
2009-10-20 1:39 ` Dmitry Torokhov
0 siblings, 1 reply; 26+ messages in thread
From: Harald Welte @ 2009-09-07 9:38 UTC (permalink / raw)
To: Kyungmin Park
Cc: 양진성, linux-input, ben-linux,
김경일/AP개발팀/E3/삼성전자,
김국진/AP개발팀/E5/삼성전자
Hi Kyungmin,
On Mon, Sep 07, 2009 at 05:32:08PM +0900, Kyungmin Park wrote:
> > They will merge your patches if you send them! They will learn as much from
> > you, as they are from me when I provide feedback.
>
> It's already sent by Joonyoung,
Do you mean the patch for your full driver code, or actually patches that
modify the System LSI driver?
System LSI will merge patches you send which they can apply to their code.
If you just simply send your driver (which conflicts with their driver), this
will not work.
Your patches will have to come incremental to their work, so System LSI can apply
them to their tree.
> > So rather than continue to try to compete with each other, you need to work
> > together. If you think your code is superior, why not submit your code as
> > patches to System LSI, or even ask System LSI if you can either
>
> It's because we don't get same base code. LSI works on s3c6410, but we
> s3c6410, s5pc100 and s5pc110.
System LSI also works on s5pc100 and s5pc110, as I assume you know. They
work on code for all of their SoCs ;)
It is simply that the way their workflow is now structured with regard to
mainline that they will first send patches to complete the 6410 support,
and then move to 6440, c100 and last c110
> we trying to post the patches related with s3c6410 but it takes long
> time to merge to Ben's git.
Well, I think this is something you have to discuss with Ben.
But anyone who works with mainline has to send incremental changes. So if you
submit a keypad driver that contains code for a SoC that is not in mainline
yet, it will not get accepted. You need to get the order right. This is
independent who does the work, LSI or DMC, or anyone else.
> Anyway. please let me know which the next patch. touchscreen? or others?
> Than we can compare the devices sources and discuss internally and then post it.
I will follow-up in a different mail, as this is becoming way too off-topic for
linux-input.
> > It is not "they" vs. "you" vs. "us", it is all of us together. There is too
> > much work to be done. There is 6400,6410,6440,6430,6442,c100,c110 and more
> > products lining up in the future. Many drivers spanning various subsystems.
> > Only if the resources are put together this task can be finished in any
> > reasonable amount of time.
>
> That's reason to send our codes. Now you focus on the s3c6410 but we
> already know the s5pc1xx series.
My focus is on aligning System LSI division's development for _all_ SoC's.
As indicated before, they now no longer work on fixed kernel versions, but they
always pull/merge from torvalds/linux-2.6 to make sure their code is much
closer to mainline.
> > System LSI is pushing their code for mainline, one patch at a time, like
> > we are seeing with this keypad driver right now. This will generate feedback
> > by the community (including you). System LSI then incorporates that feedback
> > and re-submit an updated version of the driver - just like any other person
> > who submits code to Linux mainline.
>
> I think no problem to send several patch or drivers parallel. If so we
> can get more feedbacks.
I disagree. Two departments of Samsung writing two drivers and then submitting
them to the mailinglists
1) makes everyone, esp. the maintainer have to review two drivers
2) confuses the maintainer, as he does not know which one to apply
Anyway, I think we should continue this discussion off-list and see how
we can structure the workflow in a better way.
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
--
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 7:31 ` Joonyoung Shim
@ 2009-09-07 11:32 ` Jinsung Yang
2009-09-07 12:15 ` Joonyoung Shim
0 siblings, 1 reply; 26+ messages in thread
From: Jinsung Yang @ 2009-09-07 11:32 UTC (permalink / raw)
To: 'Joonyoung Shim', 'Harald Welte'
Cc: linux-input, ben-linux,
'"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자"',
'"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'"',
dmitry.torokhov, kyungmin.park, jh80.chung
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 11:32 ` Jinsung Yang
@ 2009-09-07 12:15 ` Joonyoung Shim
2009-09-07 12:38 ` Jinsung Yang
0 siblings, 1 reply; 26+ messages in thread
From: Joonyoung Shim @ 2009-09-07 12:15 UTC (permalink / raw)
To: Jinsung Yang
Cc: 'Harald Welte', linux-input, ben-linux,
"'\"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자\"'",
"'\"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'\"'",
dmitry.torokhov, kyungmin.park, jh80.chung
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 12:15 ` Joonyoung Shim
@ 2009-09-07 12:38 ` Jinsung Yang
2009-09-07 13:14 ` Kyungmin Park
2009-09-07 13:42 ` Joonyoung Shim
0 siblings, 2 replies; 26+ messages in thread
From: Jinsung Yang @ 2009-09-07 12:38 UTC (permalink / raw)
To: 'Joonyoung Shim'
Cc: 'Harald Welte', linux-input, ben-linux,
'"'\"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자\"'"',
'"'\"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'\"'"',
dmitry.torokhov, kyungmin.park, jh80.chung
> > 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 12:38 ` Jinsung Yang
@ 2009-09-07 13:14 ` Kyungmin Park
2009-09-07 13:40 ` Kyungmin Park
2009-09-07 13:42 ` Joonyoung Shim
1 sibling, 1 reply; 26+ messages in thread
From: Kyungmin Park @ 2009-09-07 13:14 UTC (permalink / raw)
To: Jinsung Yang
Cc: Joonyoung Shim, Harald Welte, linux-input, ben-linux,
"'\"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자\"'",
"'\"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'\"'",
dmitry.torokhov, jh80.chung
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 13:14 ` Kyungmin Park
@ 2009-09-07 13:40 ` Kyungmin Park
0 siblings, 0 replies; 26+ messages in thread
From: Kyungmin Park @ 2009-09-07 13:40 UTC (permalink / raw)
To: Jinsung Yang
Cc: Joonyoung Shim, Harald Welte, linux-input, ben-linux,
"'\"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자\"'",
"'\"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'\"'",
dmitry.torokhov, jh80.chung
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 12:38 ` Jinsung Yang
2009-09-07 13:14 ` Kyungmin Park
@ 2009-09-07 13:42 ` Joonyoung Shim
1 sibling, 0 replies; 26+ messages in thread
From: Joonyoung Shim @ 2009-09-07 13:42 UTC (permalink / raw)
To: Jinsung Yang
Cc: Joonyoung Shim, Harald Welte, linux-input, ben-linux,
"'\"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자\"'",
"'\"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'\"'",
dmitry.torokhov, kyungmin.park, jh80.chung
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
>
--
- Joonyoung Shim
--
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-07 8:40 ` Joonyoung Shim
@ 2009-09-08 4:44 ` Dmitry Torokhov
2009-09-17 2:17 ` Joonyoung Shim
0 siblings, 1 reply; 26+ messages in thread
From: Dmitry Torokhov @ 2009-09-08 4:44 UTC (permalink / raw)
To: Joonyoung Shim
Cc: 양진성, linux-input, laforge, ben-linux,
"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자",
"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'",
kyungmin.park, jh80.chung
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.
>
--
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs
2009-09-08 4:44 ` Dmitry Torokhov
@ 2009-09-17 2:17 ` Joonyoung Shim
0 siblings, 0 replies; 26+ messages in thread
From: Joonyoung Shim @ 2009-09-17 2:17 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: 양진성, linux-input, laforge, ben-linux,
"김경일/AP개발팀(SYS.LSI)/E3(사원)/삼성전자",
"'김국진/AP개발팀(SYS.LSI)/E5(책임)/삼성전자'",
kyungmin.park, jh80.chung
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.
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs)
2009-09-07 9:38 ` Harald Welte
@ 2009-10-20 1:39 ` Dmitry Torokhov
2009-12-08 4:24 ` Dmitry Torokhov
0 siblings, 1 reply; 26+ messages in thread
From: Dmitry Torokhov @ 2009-10-20 1:39 UTC (permalink / raw)
To: Harald Welte
Cc: Kyungmin Park, 양진성, linux-input, ben-linux,
김경일/AP개발팀/E3/삼성전자,
김국진/AP개발팀/E5/삼성전자
Ressurecting oldish topic...
On Mon, Sep 07, 2009 at 06:38:40PM +0900, Harald Welte wrote:
> Hi Kyungmin,
>
> On Mon, Sep 07, 2009 at 05:32:08PM +0900, Kyungmin Park wrote:
> >
> > I think no problem to send several patch or drivers parallel. If so we
> > can get more feedbacks.
>
> I disagree. Two departments of Samsung writing two drivers and then submitting
> them to the mailinglists
>
> 1) makes everyone, esp. the maintainer have to review two drivers
> 2) confuses the maintainer, as he does not know which one to apply
>
So me being that confused maintainer having to chose between the 2
drivers is wondering if you guys worked out which driver (if any) should
go first and if there is an updated version that I need to review and
apply...
Thanks!
--
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs)
2009-10-20 1:39 ` Dmitry Torokhov
@ 2009-12-08 4:24 ` Dmitry Torokhov
2009-12-08 4:52 ` Jinsung Yang
0 siblings, 1 reply; 26+ messages in thread
From: Dmitry Torokhov @ 2009-12-08 4:24 UTC (permalink / raw)
To: Harald Welte
Cc: Kyungmin Park, 양진성, linux-input, ben-linux,
김경일/AP개발팀/E3/삼성전자,
김국진/AP개발팀/E5/삼성전자
Hi,
On Mon, Oct 19, 2009 at 06:39:14PM -0700, Dmitry Torokhov wrote:
> Ressurecting oldish topic...
>
> On Mon, Sep 07, 2009 at 06:38:40PM +0900, Harald Welte wrote:
> > Hi Kyungmin,
> >
> > On Mon, Sep 07, 2009 at 05:32:08PM +0900, Kyungmin Park wrote:
> > >
> > > I think no problem to send several patch or drivers parallel. If so we
> > > can get more feedbacks.
> >
> > I disagree. Two departments of Samsung writing two drivers and then submitting
> > them to the mailinglists
> >
> > 1) makes everyone, esp. the maintainer have to review two drivers
> > 2) confuses the maintainer, as he does not know which one to apply
> >
>
> So me being that confused maintainer having to chose between the 2
> drivers is wondering if you guys worked out which driver (if any) should
> go first and if there is an updated version that I need to review and
> apply...
>
Now that the S3 touchscreen is on the way to Linus (will be in the next
pull request) did you guys get a chance to settle on keybiard driver by
any chance?
--
Dmitry
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs)
2009-12-08 4:24 ` Dmitry Torokhov
@ 2009-12-08 4:52 ` Jinsung Yang
0 siblings, 0 replies; 26+ messages in thread
From: Jinsung Yang @ 2009-12-08 4:52 UTC (permalink / raw)
To: 'Dmitry Torokhov', 'Harald Welte'
Cc: 'Kyungmin Park', linux-input, ben-linux,
'???/AP???/E3/????', '???/AP???/E5/????'
Hi,
> Now that the S3 touchscreen is on the way to Linus (will be in the next
> pull request) did you guys get a chance to settle on keybiard driver by
> any chance?
Because we are on the restructuring of samsung soc architecture
(plat-samsung) as new,
I think it is better to submit later again with new driver code.
Thank you for your considerations.
Best Regards
--
Jinsung, Yang <jsgood.yang@samsung.com>
AP Development Team
System LSI, Semiconductor Business
SAMSUNG Electronics Co., LTD
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2009-12-08 4:45 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-05 13:55 [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs 양진성
2009-09-05 16:17 ` Mark Brown
2009-09-07 3:25 ` Kyungmin Park
2009-09-07 3:50 ` Jinsung Yang
2009-09-07 6:26 ` Harald Welte
2009-09-07 6:59 ` Kyungmin Park
2009-09-07 8:02 ` Fragmentation of Samsung SoC code (was INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs) Harald Welte
2009-09-07 8:32 ` Kyungmin Park
2009-09-07 9:38 ` Harald Welte
2009-10-20 1:39 ` Dmitry Torokhov
2009-12-08 4:24 ` Dmitry Torokhov
2009-12-08 4:52 ` Jinsung Yang
2009-09-07 5:38 ` [PATCH 03/03] [INPUT][KEYBOARD] Add new keypad driver for s3c series SoCs Joonyoung Shim
2009-09-07 6:33 ` Harald Welte
2009-09-07 7:31 ` Joonyoung Shim
2009-09-07 11:32 ` Jinsung Yang
2009-09-07 12:15 ` Joonyoung Shim
2009-09-07 12:38 ` Jinsung Yang
2009-09-07 13:14 ` Kyungmin Park
2009-09-07 13:40 ` Kyungmin Park
2009-09-07 13:42 ` Joonyoung Shim
2009-09-07 7:48 ` Dmitry Torokhov
2009-09-07 8:40 ` Joonyoung Shim
2009-09-08 4:44 ` Dmitry Torokhov
2009-09-17 2:17 ` Joonyoung Shim
2009-09-07 8:00 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).