[PATCH v3 0/3] Input: omap-keypad: Wakeup capability and w/a for i689 errata.

[PATCH v3 0/3] Input: omap-keypad: Wakeup capability and w/a for i689 errata.

[Illia Smyrnov]

This patchset adds wake up capability for the keypad and
workaround for i689 errata.

Based on top of v3.11-rc3

Depends on:

Input: omap-keypad: Convert to threaded IRQ
https://patchwork.kernel.org/patch/2832920/

Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded values
https://patchwork.kernel.org/patch/2832913/

Axel Haslam (1):
  Input: omap-keypad: errata i689: Correct debounce time

Illia Smyrnov (2):
  Input: omap-keypad: Enable wakeup capability for keypad.
  Input: omap-keypad: Setup irq type from DT

 drivers/input/keyboard/omap4-keypad.c |   52 ++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH v3 0/3] Input: omap-keypad: Wakeup capability and w/a for i689 errata.

[Illia Smyrnov]

This patchset adds wake up capability for the keypad and
workaround for i689 errata.

Based on top of v3.11-rc3

Depends on:

Input: omap-keypad: Convert to threaded IRQ
https://patchwork.kernel.org/patch/2832920/

Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded values
https://patchwork.kernel.org/patch/2832913/

Axel Haslam (1):
  Input: omap-keypad: errata i689: Correct debounce time

Illia Smyrnov (2):
  Input: omap-keypad: Enable wakeup capability for keypad.
  Input: omap-keypad: Setup irq type from DT

 drivers/input/keyboard/omap4-keypad.c |   52 ++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Illia Smyrnov]

Enable/disable IRQ wake in suspend/resume handlers
to make the keypad wakeup capable.

Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 drivers/input/keyboard/omap4-keypad.c |   43 +++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index 0244262..feab00f 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -74,6 +74,7 @@ struct omap4_keypad {
 	struct input_dev *input;
 
 	void __iomem *base;
+	bool irq_wake_enabled;
 	unsigned int irq;
 
 	unsigned int rows;
@@ -380,6 +381,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 		goto err_free_input;
 	}
 
+	device_init_wakeup(&pdev->dev, true);
 	pm_runtime_put_sync(&pdev->dev);
 
 	error = input_register_device(keypad_data->input);
@@ -393,6 +395,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
+	device_init_wakeup(&pdev->dev, false);
 	free_irq(keypad_data->irq, keypad_data);
 err_free_keymap:
 	kfree(keypad_data->keymap);
@@ -418,6 +421,8 @@ static int omap4_keypad_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
+	device_init_wakeup(&pdev->dev, false);
+
 	input_unregister_device(keypad_data->input);
 
 	iounmap(keypad_data->base);
@@ -439,12 +444,50 @@ static const struct of_device_id omap_keypad_dt_match[] = {
 MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
 #endif
 
+#ifdef CONFIG_PM_SLEEP
+static int omap4_keypad_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
+	int error;
+
+	if (device_may_wakeup(&pdev->dev)) {
+		error = enable_irq_wake(keypad_data->irq);
+		if (!error)
+			keypad_data->irq_wake_enabled = true;
+	}
+
+	return 0;
+}
+
+static int omap4_keypad_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&pdev->dev) && keypad_data->irq_wake_enabled) {
+		disable_irq_wake(keypad_data->irq);
+		keypad_data->irq_wake_enabled = false;
+	}
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(omap4_keypad_pm_ops, omap4_keypad_suspend,
+				omap4_keypad_resume);
+
+#define OMAP4_KEYPAD_PM_OPS (&omap4_keypad_pm_ops)
+#else
+#define OMAP4_KEYPAD_PM_OPS NULL
+#endif
+
 static struct platform_driver omap4_keypad_driver = {
 	.probe		= omap4_keypad_probe,
 	.remove		= omap4_keypad_remove,
 	.driver		= {
 		.name	= "omap4-keypad",
 		.owner	= THIS_MODULE,
+		.pm	= OMAP4_KEYPAD_PM_OPS,
 		.of_match_table = of_match_ptr(omap_keypad_dt_match),
 	},
 };
-- 
1.7.9.5

[PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Illia Smyrnov]

Enable/disable IRQ wake in suspend/resume handlers
to make the keypad wakeup capable.

Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 drivers/input/keyboard/omap4-keypad.c |   43 +++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index 0244262..feab00f 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -74,6 +74,7 @@ struct omap4_keypad {
 	struct input_dev *input;
 
 	void __iomem *base;
+	bool irq_wake_enabled;
 	unsigned int irq;
 
 	unsigned int rows;
@@ -380,6 +381,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 		goto err_free_input;
 	}
 
+	device_init_wakeup(&pdev->dev, true);
 	pm_runtime_put_sync(&pdev->dev);
 
 	error = input_register_device(keypad_data->input);
@@ -393,6 +395,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
+	device_init_wakeup(&pdev->dev, false);
 	free_irq(keypad_data->irq, keypad_data);
 err_free_keymap:
 	kfree(keypad_data->keymap);
@@ -418,6 +421,8 @@ static int omap4_keypad_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
+	device_init_wakeup(&pdev->dev, false);
+
 	input_unregister_device(keypad_data->input);
 
 	iounmap(keypad_data->base);
@@ -439,12 +444,50 @@ static const struct of_device_id omap_keypad_dt_match[] = {
 MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
 #endif
 
+#ifdef CONFIG_PM_SLEEP
+static int omap4_keypad_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
+	int error;
+
+	if (device_may_wakeup(&pdev->dev)) {
+		error = enable_irq_wake(keypad_data->irq);
+		if (!error)
+			keypad_data->irq_wake_enabled = true;
+	}
+
+	return 0;
+}
+
+static int omap4_keypad_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&pdev->dev) && keypad_data->irq_wake_enabled) {
+		disable_irq_wake(keypad_data->irq);
+		keypad_data->irq_wake_enabled = false;
+	}
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(omap4_keypad_pm_ops, omap4_keypad_suspend,
+				omap4_keypad_resume);
+
+#define OMAP4_KEYPAD_PM_OPS (&omap4_keypad_pm_ops)
+#else
+#define OMAP4_KEYPAD_PM_OPS NULL
+#endif
+
 static struct platform_driver omap4_keypad_driver = {
 	.probe		= omap4_keypad_probe,
 	.remove		= omap4_keypad_remove,
 	.driver		= {
 		.name	= "omap4-keypad",
 		.owner	= THIS_MODULE,
+		.pm	= OMAP4_KEYPAD_PM_OPS,
 		.of_match_table = of_match_ptr(omap_keypad_dt_match),
 	},
 };
-- 
1.7.9.5

[PATCH v3 2/3] Input: omap-keypad: errata i689: Correct debounce time

[Illia Smyrnov]

From: Axel Haslam <axelhaslam@ti.com>

Set debounce time value to 12ms to workaround the errata.

ERRATA DESCRIPTION:
When a key is released for a time shorter than the debounce time,
in-between 2 key press (KP1 and KP2), the keyboard state machine will go to
idle mode and will never detect the key release (after KP1,and also after KP2),
and thus will never generate a new IRQ indicating the key release.
>From the operating system standpoint, only a key press as been detected,
and the key will keep on being printed on the screen until another or
the same key is pressed again.When the failing state has been reached,
the KBD_STATEMACHINE register will show "idle state" and the KBD_FULLCODE
register won't be empty, this is the signature of the bug.There is no impact
on the power consumption of the system as the state machine goes to IDLE state.

WORKAROUND:
First thing is to program the debounce time correctly:
If X (us) is the maximum time of bounces when a key is pressed or released,
and Y (us) is the minimum time of a key release that is expected to be detected,
then the debounce time should be set to a value inbetween X and Y.

In case it is still possible to get shorter than debounce time key-release
events, then the only solution is to implement a software workaround:

Before printing a second character on the screen, the software must check if
the keyboard has hit the failing condition (cf signature of the bug above)
or if the key is still really pressed and then take the appropriate actions.

Silicon revisions impacted: OMAP4430 ES 1.0,2.0,2.1,2.2,2.3
                            OMAP4460 ES 1.0,1.1
                            OMAP5430 ES 1.0

Signed-off-by: Axel Haslam <axelhaslam@ti.com>
Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 Based on top of v3.11-rc3

 Depends on:
 Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded values
 https://patchwork.kernel.org/patch/2832913/

 drivers/input/keyboard/omap4-keypad.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index feab00f..e8bdc76 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -62,8 +62,10 @@
 
 /* OMAP4 values */
 #define OMAP4_VAL_IRQDISABLE		0x0
-#define OMAP4_VAL_DEBOUNCINGTIME	0x7
-#define OMAP4_VAL_PVT			0x7
+
+/* Errata i689: set debounce time value to 12ms */
+#define OMAP4_VAL_DEBOUNCINGTIME	0x2
+#define OMAP4_VAL_PVT			0x6
 
 enum {
 	KBD_REVISION_OMAP4 = 0,
-- 
1.7.9.5

[PATCH v3 2/3] Input: omap-keypad: errata i689: Correct debounce time

[Illia Smyrnov]

From: Axel Haslam <axelhaslam@ti.com>

Set debounce time value to 12ms to workaround the errata.

ERRATA DESCRIPTION:
When a key is released for a time shorter than the debounce time,
in-between 2 key press (KP1 and KP2), the keyboard state machine will go to
idle mode and will never detect the key release (after KP1,and also after KP2),
and thus will never generate a new IRQ indicating the key release.
>From the operating system standpoint, only a key press as been detected,
and the key will keep on being printed on the screen until another or
the same key is pressed again.When the failing state has been reached,
the KBD_STATEMACHINE register will show "idle state" and the KBD_FULLCODE
register won't be empty, this is the signature of the bug.There is no impact
on the power consumption of the system as the state machine goes to IDLE state.

WORKAROUND:
First thing is to program the debounce time correctly:
If X (us) is the maximum time of bounces when a key is pressed or released,
and Y (us) is the minimum time of a key release that is expected to be detected,
then the debounce time should be set to a value inbetween X and Y.

In case it is still possible to get shorter than debounce time key-release
events, then the only solution is to implement a software workaround:

Before printing a second character on the screen, the software must check if
the keyboard has hit the failing condition (cf signature of the bug above)
or if the key is still really pressed and then take the appropriate actions.

Silicon revisions impacted: OMAP4430 ES 1.0,2.0,2.1,2.2,2.3
                            OMAP4460 ES 1.0,1.1
                            OMAP5430 ES 1.0

Signed-off-by: Axel Haslam <axelhaslam@ti.com>
Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 Based on top of v3.11-rc3

 Depends on:
 Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded values
 https://patchwork.kernel.org/patch/2832913/

 drivers/input/keyboard/omap4-keypad.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index feab00f..e8bdc76 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -62,8 +62,10 @@
 
 /* OMAP4 values */
 #define OMAP4_VAL_IRQDISABLE		0x0
-#define OMAP4_VAL_DEBOUNCINGTIME	0x7
-#define OMAP4_VAL_PVT			0x7
+
+/* Errata i689: set debounce time value to 12ms */
+#define OMAP4_VAL_DEBOUNCINGTIME	0x2
+#define OMAP4_VAL_PVT			0x6
 
 enum {
 	KBD_REVISION_OMAP4 = 0,
-- 
1.7.9.5

[PATCH v3 3/3] Input: omap-keypad: Setup irq type from DT

[Illia Smyrnov]

OMAP4 is DT only, so read the keypad IRQ type from DT instead hardcoding it
in driver.

Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 Based on top of v3.11-rc3

 Depends on:
 Input: omap-keypad: Convert to threaded IRQ
 https://patchwork.kernel.org/patch/2832920/

 drivers/input/keyboard/omap4-keypad.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index e8bdc76..75f8b94 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -375,8 +375,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	}
 
 	error = request_threaded_irq(keypad_data->irq, omap4_keypad_irq_handler,
-				     omap4_keypad_irq_thread_fn,
-				     IRQF_TRIGGER_RISING,
+				     omap4_keypad_irq_thread_fn, 0,
 				     "omap4-keypad", keypad_data);
 	if (error) {
 		dev_err(&pdev->dev, "failed to register interrupt\n");
-- 
1.7.9.5

[PATCH v3 3/3] Input: omap-keypad: Setup irq type from DT

[Illia Smyrnov]

OMAP4 is DT only, so read the keypad IRQ type from DT instead hardcoding it
in driver.

Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 Based on top of v3.11-rc3

 Depends on:
 Input: omap-keypad: Convert to threaded IRQ
 https://patchwork.kernel.org/patch/2832920/

 drivers/input/keyboard/omap4-keypad.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index e8bdc76..75f8b94 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -375,8 +375,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	}
 
 	error = request_threaded_irq(keypad_data->irq, omap4_keypad_irq_handler,
-				     omap4_keypad_irq_thread_fn,
-				     IRQF_TRIGGER_RISING,
+				     omap4_keypad_irq_thread_fn, 0,
 				     "omap4-keypad", keypad_data);
 	if (error) {
 		dev_err(&pdev->dev, "failed to register interrupt\n");
-- 
1.7.9.5

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]

Hi,

On Mon, Jul 29, 2013 at 07:45:09PM +0300, Illia Smyrnov wrote:
> Enable/disable IRQ wake in suspend/resume handlers
> to make the keypad wakeup capable.
> 
> Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |   43 +++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index 0244262..feab00f 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -74,6 +74,7 @@ struct omap4_keypad {
>  	struct input_dev *input;
>  
>  	void __iomem *base;
> +	bool irq_wake_enabled;

this flag is a bit weird... but I can't find a better way to handle this
situation. In one way, you shouldn't prevent system suspend, so you can
error out in case enable_irq_wake() fails, otoh if enable_irq_wake()
fails and you return 0, on resume disable_irq_wake() will throw
unbalanced calls warning. Maybe someone else has a better idea.

> @@ -439,12 +444,50 @@ static const struct of_device_id omap_keypad_dt_match[] = {
>  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
>  #endif
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int omap4_keypad_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);

you don't need to access the platform_device...

> +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);

... since this can become:

	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);

> +	int error;
> +
> +	if (device_may_wakeup(&pdev->dev)) {
> +		error = enable_irq_wake(keypad_data->irq);
> +		if (!error)
> +			keypad_data->irq_wake_enabled = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int omap4_keypad_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);

ditto, use dev_get_drvdata() instead.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]

Hi,

On Mon, Jul 29, 2013 at 07:45:09PM +0300, Illia Smyrnov wrote:
> Enable/disable IRQ wake in suspend/resume handlers
> to make the keypad wakeup capable.
> 
> Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |   43 +++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index 0244262..feab00f 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -74,6 +74,7 @@ struct omap4_keypad {
>  	struct input_dev *input;
>  
>  	void __iomem *base;
> +	bool irq_wake_enabled;

this flag is a bit weird... but I can't find a better way to handle this
situation. In one way, you shouldn't prevent system suspend, so you can
error out in case enable_irq_wake() fails, otoh if enable_irq_wake()
fails and you return 0, on resume disable_irq_wake() will throw
unbalanced calls warning. Maybe someone else has a better idea.

> @@ -439,12 +444,50 @@ static const struct of_device_id omap_keypad_dt_match[] = {
>  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
>  #endif
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int omap4_keypad_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);

you don't need to access the platform_device...

> +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);

... since this can become:

	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);

> +	int error;
> +
> +	if (device_may_wakeup(&pdev->dev)) {
> +		error = enable_irq_wake(keypad_data->irq);
> +		if (!error)
> +			keypad_data->irq_wake_enabled = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int omap4_keypad_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);

ditto, use dev_get_drvdata() instead.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/3] Input: omap-keypad: errata i689: Correct debounce time

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2776 bytes --]

Hi,

On Mon, Jul 29, 2013 at 07:45:10PM +0300, Illia Smyrnov wrote:
> From: Axel Haslam <axelhaslam@ti.com>
> 
> Set debounce time value to 12ms to workaround the errata.
> 
> ERRATA DESCRIPTION:
> When a key is released for a time shorter than the debounce time,
> in-between 2 key press (KP1 and KP2), the keyboard state machine will go to
> idle mode and will never detect the key release (after KP1,and also after KP2),
> and thus will never generate a new IRQ indicating the key release.
> From the operating system standpoint, only a key press as been detected,
> and the key will keep on being printed on the screen until another or
> the same key is pressed again.When the failing state has been reached,
> the KBD_STATEMACHINE register will show "idle state" and the KBD_FULLCODE
> register won't be empty, this is the signature of the bug.There is no impact
> on the power consumption of the system as the state machine goes to IDLE state.
> 
> WORKAROUND:
> First thing is to program the debounce time correctly:
> If X (us) is the maximum time of bounces when a key is pressed or released,
> and Y (us) is the minimum time of a key release that is expected to be detected,
> then the debounce time should be set to a value inbetween X and Y.
> 
> In case it is still possible to get shorter than debounce time key-release
> events, then the only solution is to implement a software workaround:
> 
> Before printing a second character on the screen, the software must check if
> the keyboard has hit the failing condition (cf signature of the bug above)
> or if the key is still really pressed and then take the appropriate actions.
> 
> Silicon revisions impacted: OMAP4430 ES 1.0,2.0,2.1,2.2,2.3
>                             OMAP4460 ES 1.0,1.1
>                             OMAP5430 ES 1.0

if only these are affected, should you have revision check ? Also we
don't support OMAP5430 ES 1.0. I say this because, IMO, debouncing time
is likely to be board-specific and/or application-specific.

> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index feab00f..e8bdc76 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -62,8 +62,10 @@
>  
>  /* OMAP4 values */
>  #define OMAP4_VAL_IRQDISABLE		0x0
> -#define OMAP4_VAL_DEBOUNCINGTIME	0x7
> -#define OMAP4_VAL_PVT			0x7
> +
> +/* Errata i689: set debounce time value to 12ms */
> +#define OMAP4_VAL_DEBOUNCINGTIME	0x2
> +#define OMAP4_VAL_PVT			0x6

to make this easier to understand, you might want to convert this into a
better macro that takes an argument in microseconds and you calculate
the debouncing time which should be written to HW.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/3] Input: omap-keypad: errata i689: Correct debounce time

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2776 bytes --]

Hi,

On Mon, Jul 29, 2013 at 07:45:10PM +0300, Illia Smyrnov wrote:
> From: Axel Haslam <axelhaslam@ti.com>
> 
> Set debounce time value to 12ms to workaround the errata.
> 
> ERRATA DESCRIPTION:
> When a key is released for a time shorter than the debounce time,
> in-between 2 key press (KP1 and KP2), the keyboard state machine will go to
> idle mode and will never detect the key release (after KP1,and also after KP2),
> and thus will never generate a new IRQ indicating the key release.
> From the operating system standpoint, only a key press as been detected,
> and the key will keep on being printed on the screen until another or
> the same key is pressed again.When the failing state has been reached,
> the KBD_STATEMACHINE register will show "idle state" and the KBD_FULLCODE
> register won't be empty, this is the signature of the bug.There is no impact
> on the power consumption of the system as the state machine goes to IDLE state.
> 
> WORKAROUND:
> First thing is to program the debounce time correctly:
> If X (us) is the maximum time of bounces when a key is pressed or released,
> and Y (us) is the minimum time of a key release that is expected to be detected,
> then the debounce time should be set to a value inbetween X and Y.
> 
> In case it is still possible to get shorter than debounce time key-release
> events, then the only solution is to implement a software workaround:
> 
> Before printing a second character on the screen, the software must check if
> the keyboard has hit the failing condition (cf signature of the bug above)
> or if the key is still really pressed and then take the appropriate actions.
> 
> Silicon revisions impacted: OMAP4430 ES 1.0,2.0,2.1,2.2,2.3
>                             OMAP4460 ES 1.0,1.1
>                             OMAP5430 ES 1.0

if only these are affected, should you have revision check ? Also we
don't support OMAP5430 ES 1.0. I say this because, IMO, debouncing time
is likely to be board-specific and/or application-specific.

> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index feab00f..e8bdc76 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -62,8 +62,10 @@
>  
>  /* OMAP4 values */
>  #define OMAP4_VAL_IRQDISABLE		0x0
> -#define OMAP4_VAL_DEBOUNCINGTIME	0x7
> -#define OMAP4_VAL_PVT			0x7
> +
> +/* Errata i689: set debounce time value to 12ms */
> +#define OMAP4_VAL_DEBOUNCINGTIME	0x2
> +#define OMAP4_VAL_PVT			0x6

to make this easier to understand, you might want to convert this into a
better macro that takes an argument in microseconds and you calculate
the debouncing time which should be written to HW.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 3/3] Input: omap-keypad: Setup irq type from DT

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On Mon, Jul 29, 2013 at 07:45:11PM +0300, Illia Smyrnov wrote:
> OMAP4 is DT only, so read the keypad IRQ type from DT instead hardcoding it
> in driver.
> 
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>

Looks good to my eyes:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 3/3] Input: omap-keypad: Setup irq type from DT

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

On Mon, Jul 29, 2013 at 07:45:11PM +0300, Illia Smyrnov wrote:
> OMAP4 is DT only, so read the keypad IRQ type from DT instead hardcoding it
> in driver.
> 
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>

Looks good to my eyes:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Dmitry Torokhov]

On Monday, July 29, 2013 09:04:41 PM Felipe Balbi wrote:
> Hi,
> 
> On Mon, Jul 29, 2013 at 07:45:09PM +0300, Illia Smyrnov wrote:
> > Enable/disable IRQ wake in suspend/resume handlers
> > to make the keypad wakeup capable.
> > 
> > Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
> > ---
> > 
> >  drivers/input/keyboard/omap4-keypad.c |   43
> >  +++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
> > 
> > diff --git a/drivers/input/keyboard/omap4-keypad.c
> > b/drivers/input/keyboard/omap4-keypad.c index 0244262..feab00f 100644
> > --- a/drivers/input/keyboard/omap4-keypad.c
> > +++ b/drivers/input/keyboard/omap4-keypad.c
> > @@ -74,6 +74,7 @@ struct omap4_keypad {
> > 
> >  	struct input_dev *input;
> >  	
> >  	void __iomem *base;
> > 
> > +	bool irq_wake_enabled;
> 
> this flag is a bit weird... but I can't find a better way to handle this
> situation. In one way, you shouldn't prevent system suspend, so you can
> error out in case enable_irq_wake() fails, otoh if enable_irq_wake()
> fails and you return 0, on resume disable_irq_wake() will throw
> unbalanced calls warning. Maybe someone else has a better idea.
> 
> > @@ -439,12 +444,50 @@ static const struct of_device_id
> > omap_keypad_dt_match[] = {> 
> >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> >  #endif
> > 
> > +#ifdef CONFIG_PM_SLEEP
> > +static int omap4_keypad_suspend(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> 
> you don't need to access the platform_device...
> 
> > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> 
> ... since this can become:
> 
> 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);

No, please use correct accessors for the objects. Platform drivers deal
with platform devices and I prefer using platform_get_drvdata() on them.

Thanks.

-- 
Dmitry

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

Hi,

On Mon, Jul 29, 2013 at 11:59:45AM -0700, Dmitry Torokhov wrote:
> > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > omap_keypad_dt_match[] = {> 
> > >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > >  #endif
> > > 
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int omap4_keypad_suspend(struct device *dev)
> > > +{
> > > +	struct platform_device *pdev = to_platform_device(dev);
> > 
> > you don't need to access the platform_device...
> > 
> > > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > 
> > ... since this can become:
> > 
> > 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> 
> No, please use correct accessors for the objects. Platform drivers deal
> with platform devices and I prefer using platform_get_drvdata() on them.

The argument to this function is a struct device, you prefer to do some
pointer math to find the containing pdev, then deref that back to dev,
then to struct device_private and further to driver_data ?

Sounds like a waste of time IMHO. You already have the device pointer
anyway, why would you go through the trouble of calculating the
offsets for the containing struct platform_device ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

Hi,

On Mon, Jul 29, 2013 at 11:59:45AM -0700, Dmitry Torokhov wrote:
> > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > omap_keypad_dt_match[] = {> 
> > >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > >  #endif
> > > 
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int omap4_keypad_suspend(struct device *dev)
> > > +{
> > > +	struct platform_device *pdev = to_platform_device(dev);
> > 
> > you don't need to access the platform_device...
> > 
> > > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > 
> > ... since this can become:
> > 
> > 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> 
> No, please use correct accessors for the objects. Platform drivers deal
> with platform devices and I prefer using platform_get_drvdata() on them.

The argument to this function is a struct device, you prefer to do some
pointer math to find the containing pdev, then deref that back to dev,
then to struct device_private and further to driver_data ?

Sounds like a waste of time IMHO. You already have the device pointer
anyway, why would you go through the trouble of calculating the
offsets for the containing struct platform_device ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Dmitry Torokhov]

On Monday, July 29, 2013 10:13:24 PM Felipe Balbi wrote:
> Hi,
> 
> On Mon, Jul 29, 2013 at 11:59:45AM -0700, Dmitry Torokhov wrote:
> > > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > > omap_keypad_dt_match[] = {>
> > > > 
> > > >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > > >  #endif
> > > > 
> > > > +#ifdef CONFIG_PM_SLEEP
> > > > +static int omap4_keypad_suspend(struct device *dev)
> > > > +{
> > > > +	struct platform_device *pdev = to_platform_device(dev);
> > > 
> > > you don't need to access the platform_device...
> > > 
> > > > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > > 
> > > ... since this can become:
> > > 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> > 
> > No, please use correct accessors for the objects. Platform drivers deal
> > with platform devices and I prefer using platform_get_drvdata() on them.
> 
> The argument to this function is a struct device, you prefer to do some
> pointer math to find the containing pdev, then deref that back to dev,
> then to struct device_private and further to driver_data ?
> 
> Sounds like a waste of time IMHO. You already have the device pointer
> anyway, why would you go through the trouble of calculating the
> offsets for the containing struct platform_device ?

This assumes knowledge of dev_get_drvdata() implementation and assumption
that it will stay the same. Unless I hear from device core guys that
<bus>_{get|set}_drvdata() methods are obsolete and will be eventually
removed I will require proper accessors being used.

Thanks!

-- 
Dmitry

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

Hi,

On Mon, Jul 29, 2013 at 12:59:23PM -0700, Dmitry Torokhov wrote:
> > > > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > > > omap_keypad_dt_match[] = {>
> > > > > 
> > > > >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > > > >  #endif
> > > > > 
> > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > +static int omap4_keypad_suspend(struct device *dev)
> > > > > +{
> > > > > +	struct platform_device *pdev = to_platform_device(dev);
> > > > 
> > > > you don't need to access the platform_device...
> > > > 
> > > > > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > > > 
> > > > ... since this can become:
> > > > 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> > > 
> > > No, please use correct accessors for the objects. Platform drivers deal
> > > with platform devices and I prefer using platform_get_drvdata() on them.
> > 
> > The argument to this function is a struct device, you prefer to do some
> > pointer math to find the containing pdev, then deref that back to dev,
> > then to struct device_private and further to driver_data ?
> > 
> > Sounds like a waste of time IMHO. You already have the device pointer
> > anyway, why would you go through the trouble of calculating the
> > offsets for the containing struct platform_device ?
> 
> This assumes knowledge of dev_get_drvdata() implementation and assumption
> that it will stay the same. Unless I hear from device core guys that
> <bus>_{get|set}_drvdata() methods are obsolete and will be eventually
> removed I will require proper accessors being used.

they're not obsolete and will never be removed. They're nothing but
helpers though. Instead of calling:

	dev_set_drvdata(&pdev->dev);

you call:

	platform_set_drvdata(pdev);

same is valid for every single bus, but in the end they all just wrap a
call dev_{set,get}_drvdata() internally. If you already have a struct
device pointer as argument, why waste cycles doing pointer math just to
go back to the same struct device pointer on the next call ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2087 bytes --]

Hi,

On Mon, Jul 29, 2013 at 12:59:23PM -0700, Dmitry Torokhov wrote:
> > > > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > > > omap_keypad_dt_match[] = {>
> > > > > 
> > > > >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > > > >  #endif
> > > > > 
> > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > +static int omap4_keypad_suspend(struct device *dev)
> > > > > +{
> > > > > +	struct platform_device *pdev = to_platform_device(dev);
> > > > 
> > > > you don't need to access the platform_device...
> > > > 
> > > > > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > > > 
> > > > ... since this can become:
> > > > 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> > > 
> > > No, please use correct accessors for the objects. Platform drivers deal
> > > with platform devices and I prefer using platform_get_drvdata() on them.
> > 
> > The argument to this function is a struct device, you prefer to do some
> > pointer math to find the containing pdev, then deref that back to dev,
> > then to struct device_private and further to driver_data ?
> > 
> > Sounds like a waste of time IMHO. You already have the device pointer
> > anyway, why would you go through the trouble of calculating the
> > offsets for the containing struct platform_device ?
> 
> This assumes knowledge of dev_get_drvdata() implementation and assumption
> that it will stay the same. Unless I hear from device core guys that
> <bus>_{get|set}_drvdata() methods are obsolete and will be eventually
> removed I will require proper accessors being used.

they're not obsolete and will never be removed. They're nothing but
helpers though. Instead of calling:

	dev_set_drvdata(&pdev->dev);

you call:

	platform_set_drvdata(pdev);

same is valid for every single bus, but in the end they all just wrap a
call dev_{set,get}_drvdata() internally. If you already have a struct
device pointer as argument, why waste cycles doing pointer math just to
go back to the same struct device pointer on the next call ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Dmitry Torokhov]

On Monday, July 29, 2013 11:36:05 PM Felipe Balbi wrote:
> Hi,
> 
> On Mon, Jul 29, 2013 at 12:59:23PM -0700, Dmitry Torokhov wrote:
> > > > > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > > > > omap_keypad_dt_match[] = {>
> > > > > > 
> > > > > >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > > > > >  #endif
> > > > > > 
> > > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > > +static int omap4_keypad_suspend(struct device *dev)
> > > > > > +{
> > > > > > +	struct platform_device *pdev = to_platform_device(dev);
> > > > > 
> > > > > you don't need to access the platform_device...
> > > > > 
> > > > > > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > > > > 
> > > > > ... since this can become:
> > > > > 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> > > > 
> > > > No, please use correct accessors for the objects. Platform drivers
> > > > deal
> > > > with platform devices and I prefer using platform_get_drvdata() on
> > > > them.
> > > 
> > > The argument to this function is a struct device, you prefer to do some
> > > pointer math to find the containing pdev, then deref that back to dev,
> > > then to struct device_private and further to driver_data ?
> > > 
> > > Sounds like a waste of time IMHO. You already have the device pointer
> > > anyway, why would you go through the trouble of calculating the
> > > offsets for the containing struct platform_device ?
> > 
> > This assumes knowledge of dev_get_drvdata() implementation and assumption
> > that it will stay the same. Unless I hear from device core guys that
> > <bus>_{get|set}_drvdata() methods are obsolete and will be eventually
> > removed I will require proper accessors being used.
> 
> they're not obsolete and will never be removed. They're nothing but
> helpers though. Instead of calling:
> 
> 	dev_set_drvdata(&pdev->dev);
> 
> you call:
> 
> 	platform_set_drvdata(pdev);
> 
> same is valid for every single bus, but in the end they all just wrap a
> call dev_{set,get}_drvdata() internally. If you already have a struct
> device pointer as argument, why waste cycles doing pointer math just to
> go back to the same struct device pointer on the next call ?

Because I do not want to rely on the fact that what my driver set up
with platform_set_drvdata(pdev, XXX) is the same as what dev_get_drvdata()
will return *in the current implementation*. Software layers and all
that...

-- 
Dmitry

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]

On Mon, Jul 29, 2013 at 01:40:28PM -0700, Dmitry Torokhov wrote:
> On Monday, July 29, 2013 11:36:05 PM Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Jul 29, 2013 at 12:59:23PM -0700, Dmitry Torokhov wrote:
> > > > > > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > > > > > omap_keypad_dt_match[] = {>
> > > > > > > 
> > > > > > >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > > > > > >  #endif
> > > > > > > 
> > > > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > > > +static int omap4_keypad_suspend(struct device *dev)
> > > > > > > +{
> > > > > > > +	struct platform_device *pdev = to_platform_device(dev);
> > > > > > 
> > > > > > you don't need to access the platform_device...
> > > > > > 
> > > > > > > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > > > > > 
> > > > > > ... since this can become:
> > > > > > 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> > > > > 
> > > > > No, please use correct accessors for the objects. Platform drivers
> > > > > deal
> > > > > with platform devices and I prefer using platform_get_drvdata() on
> > > > > them.
> > > > 
> > > > The argument to this function is a struct device, you prefer to do some
> > > > pointer math to find the containing pdev, then deref that back to dev,
> > > > then to struct device_private and further to driver_data ?
> > > > 
> > > > Sounds like a waste of time IMHO. You already have the device pointer
> > > > anyway, why would you go through the trouble of calculating the
> > > > offsets for the containing struct platform_device ?
> > > 
> > > This assumes knowledge of dev_get_drvdata() implementation and assumption
> > > that it will stay the same. Unless I hear from device core guys that
> > > <bus>_{get|set}_drvdata() methods are obsolete and will be eventually
> > > removed I will require proper accessors being used.
> > 
> > they're not obsolete and will never be removed. They're nothing but
> > helpers though. Instead of calling:
> > 
> > 	dev_set_drvdata(&pdev->dev);
> > 
> > you call:
> > 
> > 	platform_set_drvdata(pdev);
> > 
> > same is valid for every single bus, but in the end they all just wrap a
> > call dev_{set,get}_drvdata() internally. If you already have a struct
> > device pointer as argument, why waste cycles doing pointer math just to
> > go back to the same struct device pointer on the next call ?
> 
> Because I do not want to rely on the fact that what my driver set up
> with platform_set_drvdata(pdev, XXX) is the same as what dev_get_drvdata()
> will return *in the current implementation*. Software layers and all
> that...

fair enough, your call. It's a waste of CPU anyway.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]

On Mon, Jul 29, 2013 at 01:40:28PM -0700, Dmitry Torokhov wrote:
> On Monday, July 29, 2013 11:36:05 PM Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Jul 29, 2013 at 12:59:23PM -0700, Dmitry Torokhov wrote:
> > > > > > > @@ -439,12 +444,50 @@ static const struct of_device_id
> > > > > > > omap_keypad_dt_match[] = {>
> > > > > > > 
> > > > > > >  MODULE_DEVICE_TABLE(of, omap_keypad_dt_match);
> > > > > > >  #endif
> > > > > > > 
> > > > > > > +#ifdef CONFIG_PM_SLEEP
> > > > > > > +static int omap4_keypad_suspend(struct device *dev)
> > > > > > > +{
> > > > > > > +	struct platform_device *pdev = to_platform_device(dev);
> > > > > > 
> > > > > > you don't need to access the platform_device...
> > > > > > 
> > > > > > > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > > > > > 
> > > > > > ... since this can become:
> > > > > > 	struct omap4_keypad *keypad_data = dev_get_drvdata(dev);
> > > > > 
> > > > > No, please use correct accessors for the objects. Platform drivers
> > > > > deal
> > > > > with platform devices and I prefer using platform_get_drvdata() on
> > > > > them.
> > > > 
> > > > The argument to this function is a struct device, you prefer to do some
> > > > pointer math to find the containing pdev, then deref that back to dev,
> > > > then to struct device_private and further to driver_data ?
> > > > 
> > > > Sounds like a waste of time IMHO. You already have the device pointer
> > > > anyway, why would you go through the trouble of calculating the
> > > > offsets for the containing struct platform_device ?
> > > 
> > > This assumes knowledge of dev_get_drvdata() implementation and assumption
> > > that it will stay the same. Unless I hear from device core guys that
> > > <bus>_{get|set}_drvdata() methods are obsolete and will be eventually
> > > removed I will require proper accessors being used.
> > 
> > they're not obsolete and will never be removed. They're nothing but
> > helpers though. Instead of calling:
> > 
> > 	dev_set_drvdata(&pdev->dev);
> > 
> > you call:
> > 
> > 	platform_set_drvdata(pdev);
> > 
> > same is valid for every single bus, but in the end they all just wrap a
> > call dev_{set,get}_drvdata() internally. If you already have a struct
> > device pointer as argument, why waste cycles doing pointer math just to
> > go back to the same struct device pointer on the next call ?
> 
> Because I do not want to rely on the fact that what my driver set up
> with platform_set_drvdata(pdev, XXX) is the same as what dev_get_drvdata()
> will return *in the current implementation*. Software layers and all
> that...

fair enough, your call. It's a waste of CPU anyway.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 0/3] Input: omap-keypad: Wakeup capability and w/a for i689 errata.

[Illia Smyrnov]

Hello Dmitry,

could you take reviewed patches from this patchset?

Reviewed patches:
[PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.
[PATCH v3 3/3] Input: omap-keypad: Setup irq type from DT
are not depend on
[PATCH v3 2/3] Input: omap-keypad: errata i689: Correct debounce time

Thanks.

On 07/29/2013 07:45 PM, Illia Smyrnov wrote:
> This patchset adds wake up capability for the keypad and
> workaround for i689 errata.
>
> Based on top of v3.11-rc3
>
> Depends on:
>
> Input: omap-keypad: Convert to threaded IRQ
> https://patchwork.kernel.org/patch/2832920/
>
> Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded values
> https://patchwork.kernel.org/patch/2832913/
>
> Axel Haslam (1):
>    Input: omap-keypad: errata i689: Correct debounce time
>
> Illia Smyrnov (2):
>    Input: omap-keypad: Enable wakeup capability for keypad.
>    Input: omap-keypad: Setup irq type from DT
>
>   drivers/input/keyboard/omap4-keypad.c |   52 ++++++++++++++++++++++++++++++---
>   1 file changed, 48 insertions(+), 4 deletions(-)
>

Re: [PATCH v3 0/3] Input: omap-keypad: Wakeup capability and w/a for i689 errata.

[Dmitry Torokhov]

Hi Illia,
On Tue, Aug 27, 2013 at 01:08:05PM +0300, Illia Smyrnov wrote:
> Hello Dmitry,
> 
> could you take reviewed patches from this patchset?
> 
> Reviewed patches:
> [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.
> [PATCH v3 3/3] Input: omap-keypad: Setup irq type from DT
> are not depend on
> [PATCH v3 2/3] Input: omap-keypad: errata i689: Correct debounce time
> 
> Thanks.

Yes, they have been applied.

Thanks.

-- 
Dmitry