| Message ID | 00b101ca2e30$84135d90$8c3a18b0$%yang@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
> -----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
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,
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
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
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,
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
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.
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"); +