diff mbox

[03/03,INPUT,KEYBOARD] Add new keypad driver for s3c series SoCs

Message ID 4AA49C53.3030400@samsung.com
State New, archived
Headers show

Commit Message

Joonyoung Shim Sept. 7, 2009, 5:38 a.m. UTC
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

Comments

Harald Welte Sept. 7, 2009, 6:33 a.m. UTC | #1
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,
Joonyoung Shim Sept. 7, 2009, 7:31 a.m. UTC | #2
On 9/7/2009 3:33 PM, Harald Welte wrote:
> On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote:
>> We already implemented and tested the keypad driver for s5pc100 &
>> s5pc110 and maybe it will operate on s3c64xx. We use the samsung prefix
>> to support three cpu. Because there is no the arch for s5pc100 and
>> s5pc110, this driver doesn't compile yet on upstream kernel.
> 
> The schedule of System LSI (the department that makes the SoC and provides the
> BSP) is to first complete the 6410 support in the mainline kernel, and then
> incrementally submit the core architecture code for 6440, c100 and c110,
> followed by driver additions.
> 
> So the events (ordered by time) have to be in the following order
> 
> 1) the s3c-keypad driver with 6400/6410 support needs to be submitted and
>    submitted mainline.  This is what Jinsun was starting, and which he
>    will continue until it is included mainline.
> 

The keypad of s3c64xx and s5pc1xx is same almost, so we need to submit
the well-defined driver from the first.

> 2) the c100 / c110 or other SoC core architecture support needs to be
>    submitted mainline.  Nobody at System LSI is working on this yet,
>    but they already have a tree based on 2.6.31-rcX for this.  I suppose
>    the real work on submitting this will only be started after most of the
>    pending 6410 stuff has been submitted.
> 
> 3) Samsung (either system LSI or DMC) can then send an incremental patch
>    for adding 6440/c100/c110 support to the s3c-keypad driver.
> 
> Regards,

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Sept. 7, 2009, 7:48 a.m. UTC | #3
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
Joonyoung Shim Sept. 7, 2009, 8:40 a.m. UTC | #4
Hi,

On 9/7/2009 4:48 PM, Dmitry Torokhov wrote:
> Hi Joonyoung,
> 
> On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote:
>> +
>> +static irqreturn_t samsung_kp_interrupt(int irq, void *dev_id)
>> +{
>> +	struct samsung_keypad *keypad = dev_id;
>> +
>> +	if (!work_pending(&keypad->work.work)) {
>> +		disable_irq_nosync(keypad->irq);
>> +		atomic_inc(&keypad->irq_disable);
>> +		schedule_delayed_work(&keypad->work, msecs_to_jiffies(10));
> 
> Why do you need to have the delayed work? Can't we query the touchpad
> state immediately? Or are you trying to implement debounce logic?

Yes, debounce logic. Actually this is enough only the schedule_work. I'm 
not sure about the debounce yet. If use the debounce logic, i think the
delay time should be moved into platform data.

> Also, the driver seems to be using the edge-triggered interrupts;
> do you really need to disable IRQ until you scan the keypad?
> 

Yes, if the interrupt isn't disabled, when press keypad, many interrupt
occur.

> 
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int __devinit samsung_kp_probe(struct platform_device *pdev)
>> +{
>> +	const struct samsung_keypad_platdata *pdata;
>> +	struct samsung_keypad *keypad;
>> +	struct resource *res;
>> +	struct input_dev *input_dev;
>> +	unsigned short *keycodes;
>> +	unsigned int max_keymap_size;
>> +	unsigned int val;
>> +	int i;
>> +	int ret;
>> +
>> +	pdata = pdev->dev.platform_data;
>> +	if (!pdata) {
>> +		dev_err(&pdev->dev, "no platform data defined\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if ((pdata->num_rows <= 0) || (pdata->num_rows > SAMSUNG_MAX_ROWS))
>> +		return -EINVAL;
>> +
>> +	if ((pdata->num_cols <= 0) || (pdata->num_cols > SAMSUNG_MAX_COLS))
>> +		return -EINVAL;
>> +
>> +	/* initialize the gpio */
>> +	if (pdata->cfg_gpio)
>> +		pdata->cfg_gpio(pdata->num_rows, pdata->num_cols);
>> +
>> +	max_keymap_size = pdata->num_rows * pdata->num_cols;
>> +	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
>> +	keycodes = devm_kzalloc(&pdev->dev,
>> +			max_keymap_size * sizeof(*keycodes), GFP_KERNEL);
>> +	input_dev = input_allocate_device();
>> +	if (!keypad || !keycodes || !input_dev)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		ret = -ENODEV;
>> +		goto err_free_mem;
>> +	}
>> +
>> +	keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +	if (!keypad->base) {
>> +		ret = -EBUSY;
>> +		goto err_free_mem;
>> +	}
>> +
>> +	if (pdata->clock) {
>> +		keypad->clk = clk_get(&pdev->dev, pdata->clock);
>> +		if (IS_ERR(keypad->clk)) {
>> +			dev_err(&pdev->dev, "failed to get keypad clk\n");
>> +			ret = PTR_ERR(keypad->clk);
>> +			goto err_unmap_base;
>> +		}
>> +		clk_enable(keypad->clk);
>> +	}
>> +
>> +	keypad->input_dev = input_dev;
>> +	keypad->keycodes = keycodes;
>> +	keypad->num_cols = pdata->num_cols;
>> +	INIT_DELAYED_WORK(&keypad->work, samsung_kp_scan);
>> +
>> +	/* enable interrupt and debouncing filter and wakeup bit */
>> +	val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN |
>> +		SAMSUNG_WAKEUPEN;
>> +	writel(val, keypad->base + SAMSUNG_KEYIFCON);
>> +
>> +	keypad->irq = platform_get_irq(pdev, 0);
>> +	if (keypad->irq < 0) {
>> +		ret = keypad->irq;
>> +		goto err_disable_clk;
>> +	}
>> +
>> +	ret = devm_request_irq(&pdev->dev, keypad->irq,
>> +			samsung_kp_interrupt,
>> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> +			dev_name(&pdev->dev), keypad);
>> +
>> +	if (ret)
>> +		goto err_disable_clk;
>> +
>> +	input_dev->name		= pdev->name;
>> +	input_dev->id.bustype	= BUS_HOST;
>> +	input_dev->dev.parent	= &pdev->dev;
>> +	input_dev->evbit[0]	= BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
>> +
>> +	input_dev->keycode	= keycodes;
>> +	input_dev->keycodesize	= sizeof(*keycodes);
>> +	input_dev->keycodemax	= max_keymap_size;
>> +
>> +	for (i = 0; i < pdata->keymap_size; i++) {
>> +		unsigned int key = pdata->keymap[i];
>> +		unsigned int row = KEY_ROW(key);
>> +		unsigned int col = KEY_COL(key);
>> +		unsigned short code = KEY_VAL(key);
>> +		unsigned int index = row * keypad->num_cols + col;
>> +
>> +		keycodes[index] = code;
>> +		set_bit(code, input_dev->keybit);
>> +	}
>> +
>> +	ret = input_register_device(keypad->input_dev);
>> +	if (ret)
>> +		goto err_free_irq;
>> +
>> +	device_init_wakeup(&pdev->dev, 1);
>> +
>> +	platform_set_drvdata(pdev, keypad);
>> +
>> +	return 0;
>> +
>> +err_free_irq:
>> +	devm_free_irq(&pdev->dev, keypad->irq, keypad);
> 
> 
> If you are using devres why do you release resources manually?
> 

I wonder the irq is released automatically if we don't use devm_free_irq
here.

>> +err_disable_clk:
>> +	if (keypad->clk) {
>> +		clk_disable(keypad->clk);
>> +		clk_put(keypad->clk);
>> +	}
>> +err_unmap_base:
>> +	devm_iounmap(&pdev->dev, keypad->base);
>> +err_free_mem:
>> +	input_free_device(input_dev);
>> +	devm_kfree(&pdev->dev, keycodes);
>> +	devm_kfree(&pdev->dev, keypad);
>> +
>> +	return ret;
>> +}
>> +
>> +static int __devexit samsung_kp_remove(struct platform_device *pdev)
>> +{
>> +	struct samsung_keypad *keypad = platform_get_drvdata(pdev);
>> +
>> +	devm_free_irq(&pdev->dev, keypad->irq, keypad);
>> +	cancel_delayed_work_sync(&keypad->work);
> 
> Since you need tight control of the ordering between freeing IRQ and
> canceling the work you probably should not be using devres for IRQ
> allocation.
> 

Hmm, i'm not sure about this, but i can change it not to use devres for
IRQ allocation.

>> +
>> +	/*
>> +	 * If work indeed has been cancelled, disable_irq() will have been left
>> +	 * unbalanced from samsung_kp_interrupt().
>> +	 */
>> +	while (atomic_dec_return(&keypad->irq_disable) >= 0)
>> +		enable_irq(keypad->irq);
>> +
>> +	platform_set_drvdata(pdev, NULL);
>> +	input_unregister_device(keypad->input_dev);
>> +
>> +	if (keypad->clk) {
>> +		clk_disable(keypad->clk);
>> +		clk_put(keypad->clk);
>> +	}
>> +
>> +	devm_iounmap(&pdev->dev, keypad->base);
>> +	devm_kfree(&pdev->dev, keypad->keycodes);
>> +	devm_kfree(&pdev->dev, keypad);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int samsung_kp_suspend(struct platform_device *pdev, pm_message_t state)
>> +{
>> +	struct samsung_keypad *keypad = platform_get_drvdata(pdev);
>> +
>> +	disable_irq(keypad->irq);
>> +	enable_irq_wake(keypad->irq);
>> +
>> +	if (keypad->clk)
>> +		clk_disable(keypad->clk);
> 
> This should probaly gop into ->close() method.
> 

This driver doesn't use open() and close method. Why should move into
->close()?

>> +
>> +	return 0;
>> +}
>> +
>> +static int samsung_kp_resume(struct platform_device *pdev)
>> +{
>> +	struct samsung_keypad *keypad = platform_get_drvdata(pdev);
>> +	unsigned int val;
>> +
>> +	if (keypad->clk)
>> +		clk_enable(keypad->clk);
>> +
>> +	/* enable interrupt and debouncing filter and wakeup bit */
>> +	val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN |
>> +		SAMSUNG_WAKEUPEN;
>> +	writel(val, keypad->base + SAMSUNG_KEYIFCON);
>> +
>> +	disable_irq_wake(keypad->irq);
>> +	enable_irq(keypad->irq);
>> +
>> +	return 0;
>> +}
>> +#else
>> +#define samsung_kp_suspend	NULL
>> +#define samsung_kp_resume	NULL
>> +#endif
>> +
>> +static struct platform_driver samsung_kp_driver = {
>> +	.probe		= samsung_kp_probe,
>> +	.remove		= __devexit_p(samsung_kp_remove),
>> +	.suspend	= samsung_kp_suspend,
>> +	.resume		= samsung_kp_resume,
> 
> Please switch to dev_pm_ops.
> 

OK.

>> +	.driver		= {
>> +		.name	= "samsung-keypad",
>> +		.owner	= THIS_MODULE,
>> +	},
>> +};
>> +
>> +static int __init samsung_kp_init(void)
>> +{
>> +	return platform_driver_register(&samsung_kp_driver);
>> +}
>> +
>> +static void __exit samsung_kp_exit(void)
>> +{
>> +	platform_driver_unregister(&samsung_kp_driver);
>> +}
>> +
>> +module_init(samsung_kp_init);
>> +module_exit(samsung_kp_exit);
>> +
>> +MODULE_DESCRIPTION("Samsung keypad driver");
>> +MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
>> +MODULE_AUTHOR("Jaehoon Chung <jh80.chung@samsung.com>");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 1.6.0.4
> 

Thanks for review.

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
양진성 Sept. 7, 2009, 11:32 a.m. UTC | #5
Hi, Mr.Shim

> >    submitted mainline.  This is what Jinsun was starting, and which he
> >    will continue until it is included mainline.
> >
> 
> The keypad of s3c64xx and s5pc1xx is same almost, so we need to submit
> the well-defined driver from the first.

I have some questions:
1) Could you explain to us what is the 'well-defined' driver?
2) Did you test your keypad driver at s3c6410 based platform?
3) There is no architecture codes for s5pc1xx series, why did you send keypad driver first for s5pc1xx before architecture codes?

Best Regards
--
Jinsung, Yang <jsgood.yang@samsung.com>
AP Development Team
System LSI, Semiconductor Business
SAMSUNG Electronics Co., LTD


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joonyoung Shim Sept. 7, 2009, 12:15 p.m. UTC | #6
Hi,

On 9/7/2009 8:32 PM, Jinsung Yang wrote:
> Hi, Mr.Shim
> 
>>>    submitted mainline.  This is what Jinsun was starting, and which he
>>>    will continue until it is included mainline.
>>>
>> The keypad of s3c64xx and s5pc1xx is same almost, so we need to submit
>> the well-defined driver from the first.
> 
> I have some questions:
> 1) Could you explain to us what is the 'well-defined' driver?

I mean the driver to support three cpu and various target in one keypad
driver, but i think that your posted driver seems for only SMDK6410.
Also, we can make better driver via the review.

> 2) Did you test your keypad driver at s3c6410 based platform?

No, i cannot test it on s3c64xx because i don't have a target using the
keypad of s3c64xx such SMDK6410 but i tested the keypad driver on
s5pc100 and s5pc110 and as you know, the keypad of the s3c6410 and 
s5pc100 datasheet is same almost, so i think it will operate on s3c64xx.

> 3) There is no architecture codes for s5pc1xx series, why did you send keypad driver first for s5pc1xx before architecture codes?
> 

Of course, I can post the arch code for keypad, but it is the common
code for s3c64xx and s5pc1xx and we didn't post s5pc1xx arch code yet on
ML. I also think it is better to post the keypad driver after posting
arch.

> Best Regards
> --
> Jinsung, Yang <jsgood.yang@samsung.com>
> AP Development Team
> System LSI, Semiconductor Business
> SAMSUNG Electronics Co., LTD
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
양진성 Sept. 7, 2009, 12:38 p.m. UTC | #7
> > I have some questions:
> > 1) Could you explain to us what is the 'well-defined' driver?
> 
> I mean the driver to support three cpu and various target in one keypad
> driver, but i think that your posted driver seems for only SMDK6410.
> Also, we can make better driver via the review.
> 

Why do you think my driver cannot support various target?
I think there are misunderstandings, are there any architecture specific codes?
Surely, there can be something wrong driver-specific codes such as 'dev_pm_ops',
but that's not a big deal.. we can modify it with mainline feedbacks.
My driver codes also have considered for our s5pc100, s5pc110, and many Samsung SoCs.
We will support other SoCs with adding some architecture definitions (not changing driver codes), but just 'NOT NOW'.
Please understand, 'step by step'.
Of course we aware now our SoCs line-ups better than you, we are not happy to hear from you.

Finally, in your driver, regarding the if (cpu_is_s5pc100()) statement,
how do you think if you have to check another SoC at some time in the future?
Doing like this if (cpu_is_s5pc110()) else if (cpu_is_s5pc120()) else if (cpu_is_s5pc130()) ??
Will you send some patches for this? looks be not good.

> > 2) Did you test your keypad driver at s3c6410 based platform?
> 
> No, i cannot test it on s3c64xx because i don't have a target using the
> keypad of s3c64xx such SMDK6410 but i tested the keypad driver on
> s5pc100 and s5pc110 and as you know, the keypad of the s3c6410 and
> s5pc100 datasheet is same almost, so i think it will operate on s3c64xx.
> 
> > 3) There is no architecture codes for s5pc1xx series, why did you send
> keypad driver first for s5pc1xx before architecture codes?
> >
> 
> Of course, I can post the arch code for keypad, but it is the common
> code for s3c64xx and s5pc1xx and we didn't post s5pc1xx arch code yet on
> ML. I also think it is better to post the keypad driver after posting
> arch.
> 

Best Regards
--
Jinsung, Yang <jsgood.yang@samsung.com>
AP Development Team
System LSI, Semiconductor Business
SAMSUNG Electronics Co., LTD


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kyungmin Park Sept. 7, 2009, 1:14 p.m. UTC | #8
On Mon, Sep 7, 2009 at 9:38 PM, Jinsung Yang<jsgood.yang@samsung.com> wrote:
>> > I have some questions:
>> > 1) Could you explain to us what is the 'well-defined' driver?
>>
>> I mean the driver to support three cpu and various target in one keypad
>> driver, but i think that your posted driver seems for only SMDK6410.
>> Also, we can make better driver via the review.
>>
>
> Why do you think my driver cannot support various target?
> I think there are misunderstandings, are there any architecture specific codes?
> Surely, there can be something wrong driver-specific codes such as 'dev_pm_ops',
> but that's not a big deal.. we can modify it with mainline feedbacks.
> My driver codes also have considered for our s5pc100, s5pc110, and many Samsung SoCs.
> We will support other SoCs with adding some architecture definitions (not changing driver codes), but just 'NOT NOW'.
> Please understand, 'step by step'.
> Of course we aware now our SoCs line-ups better than you, we are not happy to hear from you.
>
> Finally, in your driver, regarding the if (cpu_is_s5pc100()) statement,
> how do you think if you have to check another SoC at some time in the future?
> Doing like this if (cpu_is_s5pc110()) else if (cpu_is_s5pc120()) else if (cpu_is_s5pc130()) ??
> Will you send some patches for this? looks be not good.

Then question. If next new chips has different field but has same
offset or address. In this case how you handle it?
E.g., How to use handle s5pc100 and s5pc110 simultaneously at one
driver without re-compile?

BR,
Kyungmin Park
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kyungmin Park Sept. 7, 2009, 1:40 p.m. UTC | #9
On Mon, Sep 7, 2009 at 10:14 PM, Kyungmin Park<kmpark@infradead.org> wrote:
> On Mon, Sep 7, 2009 at 9:38 PM, Jinsung Yang<jsgood.yang@samsung.com> wrote:
>>> > I have some questions:
>>> > 1) Could you explain to us what is the 'well-defined' driver?
>>>
>>> I mean the driver to support three cpu and various target in one keypad
>>> driver, but i think that your posted driver seems for only SMDK6410.
>>> Also, we can make better driver via the review.
>>>
>>
>> Why do you think my driver cannot support various target?
>> I think there are misunderstandings, are there any architecture specific codes?
>> Surely, there can be something wrong driver-specific codes such as 'dev_pm_ops',
>> but that's not a big deal.. we can modify it with mainline feedbacks.
>> My driver codes also have considered for our s5pc100, s5pc110, and many Samsung SoCs.
>> We will support other SoCs with adding some architecture definitions (not changing driver codes), but just 'NOT NOW'.
>> Please understand, 'step by step'.
>> Of course we aware now our SoCs line-ups better than you, we are not happy to hear from you.
>>
>> Finally, in your driver, regarding the if (cpu_is_s5pc100()) statement,
>> how do you think if you have to check another SoC at some time in the future?
>> Doing like this if (cpu_is_s5pc110()) else if (cpu_is_s5pc120()) else if (cpu_is_s5pc130()) ??
>> Will you send some patches for this? looks be not good.
>
> Then question. If next new chips has different field but has same
> offset or address. In this case how you handle it?
> E.g., How to use handle s5pc100 and s5pc110 simultaneously at one
> driver without re-compile?

Just FYI: please see the other arch, OMAP. and how to use it.
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/plat-omap/include/mach/cpu.h;h=7a5f9e882e54b140d87b1a4648f130366c102f69;hb=HEAD

Thank you,
Kyungmin Park
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joonyoung Shim Sept. 7, 2009, 1:42 p.m. UTC | #10
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
>
Dmitry Torokhov Sept. 8, 2009, 4:44 a.m. UTC | #11
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.
>
Joonyoung Shim Sept. 17, 2009, 2:17 a.m. UTC | #12
On 9/8/2009 1:44 PM, Dmitry Torokhov wrote:
> On Mon, Sep 07, 2009 at 05:40:44PM +0900, Joonyoung Shim wrote:
>> Hi,
>>
>> On 9/7/2009 4:48 PM, Dmitry Torokhov wrote:
>>> Hi Joonyoung,
>>>
>>> On Mon, Sep 07, 2009 at 02:38:27PM +0900, Joonyoung Shim wrote:
>>>> +
>>>> +static irqreturn_t samsung_kp_interrupt(int irq, void *dev_id)
>>>> +{
>>>> +	struct samsung_keypad *keypad = dev_id;
>>>> +
>>>> +	if (!work_pending(&keypad->work.work)) {
>>>> +		disable_irq_nosync(keypad->irq);
>>>> +		atomic_inc(&keypad->irq_disable);
>>>> +		schedule_delayed_work(&keypad->work, msecs_to_jiffies(10));
>>> Why do you need to have the delayed work? Can't we query the touchpad
>>> state immediately? Or are you trying to implement debounce logic?
>> Yes, debounce logic. Actually this is enough only the schedule_work. I'm 
>> not sure about the debounce yet. If use the debounce logic, i think the
>> delay time should be moved into platform data.
>>
> 
> I don't see how debounce would work if you disable interrupts. You don't
> want to just delay the read, you want to perform the read after you
> stopped receiving interrupts for certain period of time.
> 

I didn't understand completely about debounce of the samsung keypad. 
First, i will remove about debounce on initial driver, and will add it
later.

>>> Also, the driver seems to be using the edge-triggered interrupts;
>>> do you really need to disable IRQ until you scan the keypad?
>>>
>> Yes, if the interrupt isn't disabled, when press keypad, many interrupt
>> occur.
>>
> 
> Hmm, that is the common case with level-triggered interrupts, still here
> you have edge-triggered...
> 

You are right. The low level-triggered interrupt occurs and i missed it
on datasheet.

>>>> +	}
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int __devinit samsung_kp_probe(struct platform_device *pdev)
>>>> +{
>>>> +	const struct samsung_keypad_platdata *pdata;
>>>> +	struct samsung_keypad *keypad;
>>>> +	struct resource *res;
>>>> +	struct input_dev *input_dev;
>>>> +	unsigned short *keycodes;
>>>> +	unsigned int max_keymap_size;
>>>> +	unsigned int val;
>>>> +	int i;
>>>> +	int ret;
>>>> +
>>>> +	pdata = pdev->dev.platform_data;
>>>> +	if (!pdata) {
>>>> +		dev_err(&pdev->dev, "no platform data defined\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if ((pdata->num_rows <= 0) || (pdata->num_rows > SAMSUNG_MAX_ROWS))
>>>> +		return -EINVAL;
>>>> +
>>>> +	if ((pdata->num_cols <= 0) || (pdata->num_cols > SAMSUNG_MAX_COLS))
>>>> +		return -EINVAL;
>>>> +
>>>> +	/* initialize the gpio */
>>>> +	if (pdata->cfg_gpio)
>>>> +		pdata->cfg_gpio(pdata->num_rows, pdata->num_cols);
>>>> +
>>>> +	max_keymap_size = pdata->num_rows * pdata->num_cols;
>>>> +	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
>>>> +	keycodes = devm_kzalloc(&pdev->dev,
>>>> +			max_keymap_size * sizeof(*keycodes), GFP_KERNEL);
>>>> +	input_dev = input_allocate_device();
>>>> +	if (!keypad || !keycodes || !input_dev)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!res) {
>>>> +		ret = -ENODEV;
>>>> +		goto err_free_mem;
>>>> +	}
>>>> +
>>>> +	keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>> +	if (!keypad->base) {
>>>> +		ret = -EBUSY;
>>>> +		goto err_free_mem;
>>>> +	}
>>>> +
>>>> +	if (pdata->clock) {
>>>> +		keypad->clk = clk_get(&pdev->dev, pdata->clock);
>>>> +		if (IS_ERR(keypad->clk)) {
>>>> +			dev_err(&pdev->dev, "failed to get keypad clk\n");
>>>> +			ret = PTR_ERR(keypad->clk);
>>>> +			goto err_unmap_base;
>>>> +		}
>>>> +		clk_enable(keypad->clk);
>>>> +	}
>>>> +
>>>> +	keypad->input_dev = input_dev;
>>>> +	keypad->keycodes = keycodes;
>>>> +	keypad->num_cols = pdata->num_cols;
>>>> +	INIT_DELAYED_WORK(&keypad->work, samsung_kp_scan);
>>>> +
>>>> +	/* enable interrupt and debouncing filter and wakeup bit */
>>>> +	val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN |
>>>> +		SAMSUNG_WAKEUPEN;
>>>> +	writel(val, keypad->base + SAMSUNG_KEYIFCON);
>>>> +
>>>> +	keypad->irq = platform_get_irq(pdev, 0);
>>>> +	if (keypad->irq < 0) {
>>>> +		ret = keypad->irq;
>>>> +		goto err_disable_clk;
>>>> +	}
>>>> +
>>>> +	ret = devm_request_irq(&pdev->dev, keypad->irq,
>>>> +			samsung_kp_interrupt,
>>>> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>>> +			dev_name(&pdev->dev), keypad);
>>>> +
>>>> +	if (ret)
>>>> +		goto err_disable_clk;
>>>> +
>>>> +	input_dev->name		= pdev->name;
>>>> +	input_dev->id.bustype	= BUS_HOST;
>>>> +	input_dev->dev.parent	= &pdev->dev;
>>>> +	input_dev->evbit[0]	= BIT_MASK(EV_KEY) | BIT_MASK(EV_REP);
>>>> +
>>>> +	input_dev->keycode	= keycodes;
>>>> +	input_dev->keycodesize	= sizeof(*keycodes);
>>>> +	input_dev->keycodemax	= max_keymap_size;
>>>> +
>>>> +	for (i = 0; i < pdata->keymap_size; i++) {
>>>> +		unsigned int key = pdata->keymap[i];
>>>> +		unsigned int row = KEY_ROW(key);
>>>> +		unsigned int col = KEY_COL(key);
>>>> +		unsigned short code = KEY_VAL(key);
>>>> +		unsigned int index = row * keypad->num_cols + col;
>>>> +
>>>> +		keycodes[index] = code;
>>>> +		set_bit(code, input_dev->keybit);
>>>> +	}
>>>> +
>>>> +	ret = input_register_device(keypad->input_dev);
>>>> +	if (ret)
>>>> +		goto err_free_irq;
>>>> +
>>>> +	device_init_wakeup(&pdev->dev, 1);
>>>> +
>>>> +	platform_set_drvdata(pdev, keypad);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err_free_irq:
>>>> +	devm_free_irq(&pdev->dev, keypad->irq, keypad);
>>>
>>> If you are using devres why do you release resources manually?
>>>
>> I wonder the irq is released automatically if we don't use devm_free_irq
>> here.
> 
> So far I fail to see why you bother with using devres if you are managing
> all resources on your own...
> 

OK, this is my fault. I understanded again about devres. I will change
to common functions instead of using devres to manage resources on 
driver.

>>>> +err_disable_clk:
>>>> +	if (keypad->clk) {
>>>> +		clk_disable(keypad->clk);
>>>> +		clk_put(keypad->clk);
>>>> +	}
>>>> +err_unmap_base:
>>>> +	devm_iounmap(&pdev->dev, keypad->base);
>>>> +err_free_mem:
>>>> +	input_free_device(input_dev);
>>>> +	devm_kfree(&pdev->dev, keycodes);
>>>> +	devm_kfree(&pdev->dev, keypad);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int __devexit samsung_kp_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct samsung_keypad *keypad = platform_get_drvdata(pdev);
>>>> +
>>>> +	devm_free_irq(&pdev->dev, keypad->irq, keypad);
>>>> +	cancel_delayed_work_sync(&keypad->work);
>>> Since you need tight control of the ordering between freeing IRQ and
>>> canceling the work you probably should not be using devres for IRQ
>>> allocation.
>>>
>> Hmm, i'm not sure about this, but i can change it not to use devres for
>> IRQ allocation.
>>
>>>> +
>>>> +	/*
>>>> +	 * If work indeed has been cancelled, disable_irq() will have been left
>>>> +	 * unbalanced from samsung_kp_interrupt().
>>>> +	 */
>>>> +	while (atomic_dec_return(&keypad->irq_disable) >= 0)
>>>> +		enable_irq(keypad->irq);
>>>> +
>>>> +	platform_set_drvdata(pdev, NULL);
>>>> +	input_unregister_device(keypad->input_dev);
>>>> +
>>>> +	if (keypad->clk) {
>>>> +		clk_disable(keypad->clk);
>>>> +		clk_put(keypad->clk);
>>>> +	}
>>>> +
>>>> +	devm_iounmap(&pdev->dev, keypad->base);
>>>> +	devm_kfree(&pdev->dev, keypad->keycodes);
>>>> +	devm_kfree(&pdev->dev, keypad);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +#ifdef CONFIG_PM
>>>> +static int samsung_kp_suspend(struct platform_device *pdev, pm_message_t state)
>>>> +{
>>>> +	struct samsung_keypad *keypad = platform_get_drvdata(pdev);
>>>> +
>>>> +	disable_irq(keypad->irq);
>>>> +	enable_irq_wake(keypad->irq);
>>>> +
>>>> +	if (keypad->clk)
>>>> +		clk_disable(keypad->clk);
>>> This should probaly gop into ->close() method.
>>>
>> This driver doesn't use open() and close method. Why should move into
>> ->close()?
>>
> 
> It was a suggestion to implement them ;) Although the comment should
> have been a few lines above (in remove method).
> 

I will remove clk_enable and clk_disable in suspend and resume function.

>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int samsung_kp_resume(struct platform_device *pdev)
>>>> +{
>>>> +	struct samsung_keypad *keypad = platform_get_drvdata(pdev);
>>>> +	unsigned int val;
>>>> +
>>>> +	if (keypad->clk)
>>>> +		clk_enable(keypad->clk);
>>>> +
>>>> +	/* enable interrupt and debouncing filter and wakeup bit */
>>>> +	val = SAMSUNG_INT_F_EN | SAMSUNG_INT_R_EN | SAMSUNG_DF_EN |
>>>> +		SAMSUNG_WAKEUPEN;
>>>> +	writel(val, keypad->base + SAMSUNG_KEYIFCON);
>>>> +
>>>> +	disable_irq_wake(keypad->irq);
>>>> +	enable_irq(keypad->irq);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +#else
>>>> +#define samsung_kp_suspend	NULL
>>>> +#define samsung_kp_resume	NULL
>>>> +#endif
>>>> +
>>>> +static struct platform_driver samsung_kp_driver = {
>>>> +	.probe		= samsung_kp_probe,
>>>> +	.remove		= __devexit_p(samsung_kp_remove),
>>>> +	.suspend	= samsung_kp_suspend,
>>>> +	.resume		= samsung_kp_resume,
>>> Please switch to dev_pm_ops.
>>>
>> OK.
>>
>>>> +	.driver		= {
>>>> +		.name	= "samsung-keypad",
>>>> +		.owner	= THIS_MODULE,
>>>> +	},
>>>> +};
>>>> +
>>>> +static int __init samsung_kp_init(void)
>>>> +{
>>>> +	return platform_driver_register(&samsung_kp_driver);
>>>> +}
>>>> +
>>>> +static void __exit samsung_kp_exit(void)
>>>> +{
>>>> +	platform_driver_unregister(&samsung_kp_driver);
>>>> +}
>>>> +
>>>> +module_init(samsung_kp_init);
>>>> +module_exit(samsung_kp_exit);
>>>> +
>>>> +MODULE_DESCRIPTION("Samsung keypad driver");
>>>> +MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
>>>> +MODULE_AUTHOR("Jaehoon Chung <jh80.chung@samsung.com>");
>>>> +MODULE_LICENSE("GPL");
>>>> -- 
>>>> 1.6.0.4
>> Thanks for review.
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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");