[PATCH 0/2] Input: omap-keypad: Cleanup - remove hardcoded values and IRQ enabling/disabling

[PATCH 0/2] Input: omap-keypad: Cleanup - remove hardcoded values and IRQ enabling/disabling

[Illia Smyrnov]

From: Illia Smyrnov <illia.smyrnov@ti.com>

Replace unclear hardcoded values with bit field and remove 
unnecessary IRQ enabling/disabling.

Based on top of v3.10-rc1.

Tested on OMAP4 SDP.

Illia Smyrnov (2):
  Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded
    values
  Input: omap-keypad: Cleanup - remove unnecessary IRQ
    enabling/disabling

 drivers/input/keyboard/omap4-keypad.c |   40 ++++++++++++--------------------
 1 files changed, 15 insertions(+), 25 deletions(-)

[PATCH 1/2] Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded values

[Illia Smyrnov]

From: Illia Smyrnov <illia.smyrnov@ti.com>

Use bitfiled instead of hardcoded values to set KBD_CTRL, use BIT macro,
remove unused defines.

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

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index f4aa53a..c727548 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -53,21 +53,17 @@
 #define OMAP4_KBD_FULLCODE63_32		0x48
 
 /* OMAP4 bit definitions */
-#define OMAP4_DEF_IRQENABLE_EVENTEN	(1 << 0)
-#define OMAP4_DEF_IRQENABLE_LONGKEY	(1 << 1)
-#define OMAP4_DEF_IRQENABLE_TIMEOUTEN	(1 << 2)
-#define OMAP4_DEF_WUP_EVENT_ENA		(1 << 0)
-#define OMAP4_DEF_WUP_LONG_KEY_ENA	(1 << 1)
-#define OMAP4_DEF_CTRL_NOSOFTMODE	(1 << 1)
-#define OMAP4_DEF_CTRLPTVVALUE		(1 << 2)
-#define OMAP4_DEF_CTRLPTV		(1 << 1)
+#define OMAP4_DEF_IRQENABLE_EVENTEN	BIT(0)
+#define OMAP4_DEF_IRQENABLE_LONGKEY	BIT(1)
+#define OMAP4_DEF_WUP_EVENT_ENA		BIT(0)
+#define OMAP4_DEF_WUP_LONG_KEY_ENA	BIT(1)
+#define OMAP4_DEF_CTRL_NOSOFTMODE	BIT(1)
+#define OMAP4_DEF_CTRL_PTV_SHIFT	2
 
 /* OMAP4 values */
-#define OMAP4_VAL_IRQDISABLE		0x00
-#define OMAP4_VAL_DEBOUNCINGTIME	0x07
-#define OMAP4_VAL_FUNCTIONALCFG		0x1E
-
-#define OMAP4_MASK_IRQSTATUSDISABLE	0xFFFF
+#define OMAP4_VAL_IRQDISABLE		0x0
+#define OMAP4_VAL_DEBOUNCINGTIME	0x7
+#define OMAP4_VAL_PVT			0x7
 
 enum {
 	KBD_REVISION_OMAP4 = 0,
@@ -175,7 +171,8 @@ static int omap4_keypad_open(struct input_dev *input)
 	disable_irq(keypad_data->irq);
 
 	kbd_writel(keypad_data, OMAP4_KBD_CTRL,
-			OMAP4_VAL_FUNCTIONALCFG);
+			OMAP4_DEF_CTRL_NOSOFTMODE |
+			(OMAP4_VAL_PVT << OMAP4_DEF_CTRL_PTV_SHIFT));
 	kbd_writel(keypad_data, OMAP4_KBD_DEBOUNCINGTIME,
 			OMAP4_VAL_DEBOUNCINGTIME);
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
-- 
1.7.0.4

[PATCH 2/2] Input: omap-keypad: Cleanup - remove unnecessary IRQ enabling/disabling

[Illia Smyrnov]

From: Illia Smyrnov <illia.smyrnov@ti.com>

Remove unnecessary IRQ enabling/disabling for certain keyboard events.

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

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index c727548..73813b6 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -121,10 +121,6 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
 	unsigned int col, row, code, changed;
 	u32 *new_state = (u32 *) key_state;
 
-	/* Disable interrupts */
-	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
-			 OMAP4_VAL_IRQDISABLE);
-
 	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
 	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
 
@@ -154,11 +150,6 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
 
-	/* enable interrupts */
-	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
-		OMAP4_DEF_IRQENABLE_EVENTEN |
-				OMAP4_DEF_IRQENABLE_LONGKEY);
-
 	return IRQ_HANDLED;
 }
 
@@ -175,14 +166,16 @@ static int omap4_keypad_open(struct input_dev *input)
 			(OMAP4_VAL_PVT << OMAP4_DEF_CTRL_PTV_SHIFT));
 	kbd_writel(keypad_data, OMAP4_KBD_DEBOUNCINGTIME,
 			OMAP4_VAL_DEBOUNCINGTIME);
-	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
-			OMAP4_VAL_IRQDISABLE);
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
 			OMAP4_DEF_IRQENABLE_EVENTEN |
 				OMAP4_DEF_IRQENABLE_LONGKEY);
 	kbd_writel(keypad_data, OMAP4_KBD_WAKEUPENABLE,
 			OMAP4_DEF_WUP_EVENT_ENA | OMAP4_DEF_WUP_LONG_KEY_ENA);
 
+	/* clear pending interrupts */
+	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
+			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
+
 	enable_irq(keypad_data->irq);
 
 	return 0;
-- 
1.7.0.4

Re: [PATCH 2/2] Input: omap-keypad: Cleanup - remove unnecessary IRQ enabling/disabling

[Felipe Balbi]

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

Hi,

On Fri, Jul 19, 2013 at 04:03:47PM +0300, Illia Smyrnov wrote:
> From: Illia Smyrnov <illia.smyrnov@ti.com>
> 
> Remove unnecessary IRQ enabling/disabling for certain keyboard events.
> 
> Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |   15 ++++-----------
>  1 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index c727548..73813b6 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -121,10 +121,6 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  	unsigned int col, row, code, changed;
>  	u32 *new_state = (u32 *) key_state;
>  
> -	/* Disable interrupts */
> -	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
> -			 OMAP4_VAL_IRQDISABLE);
> -
>  	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
>  	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
>  
> @@ -154,11 +150,6 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
>  			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
>  
> -	/* enable interrupts */
> -	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
> -		OMAP4_DEF_IRQENABLE_EVENTEN |
> -				OMAP4_DEF_IRQENABLE_LONGKEY);
> -

please don't remove this code. It'll be good to have this around when we
move the driver to threaded IRQs without IRQF_ONESHOT. In fact, it would
be very simple to implement such a change, wanna take it up ?

It should be doable in few patches:

1) switch over to request_threaded_irq()

	just blind move to a thread, without hardirq handler, so
	IRQF_ONESHOT is mandatory.

2) add hardirq handler

	read IRQSTATUS to check if our device has generated IRQs
	returning IRQ_WAKE_THREAD if true

3) move 'IRQ masking logic' to hardirq handler, before returning
IRQ_WAKE_THREAD

	this will let you remove IRQF_ONESHOT

4) finally remove IRQF_ONESHOT

	this makes sure that IRQs aren't kept disabled until we have
	time to iterate over the entire keypad matrix. Only the keypad
	IRQ will be masked.

-- 
balbi

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

Re: [PATCH 2/2] Input: omap-keypad: Cleanup - remove unnecessary IRQ enabling/disabling

[Illia Smyrnov]

Hi,

On 07/19/2013 04:26 PM, Felipe Balbi wrote:
> Hi,
>
> [...]
> please don't remove this code. It'll be good to have this around when we
> move the driver to threaded IRQs without IRQF_ONESHOT. In fact, it would
> be very simple to implement such a change, wanna take it up ?
>
> It should be doable in few patches:
>
> 1) switch over to request_threaded_irq()
>
> 	just blind move to a thread, without hardirq handler, so
> 	IRQF_ONESHOT is mandatory.
>
> 2) add hardirq handler
>
> 	read IRQSTATUS to check if our device has generated IRQs
> 	returning IRQ_WAKE_THREAD if true
>
> 3) move 'IRQ masking logic' to hardirq handler, before returning
> IRQ_WAKE_THREAD
>
> 	this will let you remove IRQF_ONESHOT
>
> 4) finally remove IRQF_ONESHOT
>
> 	this makes sure that IRQs aren't kept disabled until we have
> 	time to iterate over the entire keypad matrix. Only the keypad
> 	IRQ will be masked.
>

Ok, but why we need to remove IRQF_ONESHOT flag for omap keypad driver?

The keypad IRQ isn't shared IRQ and in our case hardirq handler will 
always return IRQ_WAKE_THREAD like default irq_default_primary_handler 
do. With IRQF_ONESHOT flag IRQ line will be masked until the threaded
handler finished, but there is only keypad on this line.

I tested two versions:
the first one - just threaded IRQs with IRQF_ONESHOT and without 
specific hardirq handler.
the second version - threaded IRQs without IRQF_ONESHOT as you described.
Both versions was successfully tested on Blaze's keypad.

Re: [PATCH 2/2] Input: omap-keypad: Cleanup - remove unnecessary IRQ enabling/disabling

[Felipe Balbi]

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

Hi,

On Mon, Jul 22, 2013 at 08:25:05PM +0300, Illia Smyrnov wrote:
> >please don't remove this code. It'll be good to have this around when we
> >move the driver to threaded IRQs without IRQF_ONESHOT. In fact, it would
> >be very simple to implement such a change, wanna take it up ?
> >
> >It should be doable in few patches:
> >
> >1) switch over to request_threaded_irq()
> >
> >	just blind move to a thread, without hardirq handler, so
> >	IRQF_ONESHOT is mandatory.
> >
> >2) add hardirq handler
> >
> >	read IRQSTATUS to check if our device has generated IRQs
> >	returning IRQ_WAKE_THREAD if true
> >
> >3) move 'IRQ masking logic' to hardirq handler, before returning
> >IRQ_WAKE_THREAD
> >
> >	this will let you remove IRQF_ONESHOT
> >
> >4) finally remove IRQF_ONESHOT
> >
> >	this makes sure that IRQs aren't kept disabled until we have
> >	time to iterate over the entire keypad matrix. Only the keypad
> >	IRQ will be masked.
> >
> 
> Ok, but why we need to remove IRQF_ONESHOT flag for omap keypad driver?

well, we might want to use this HW with the RT patchset. In that case,
we want to run with IRQs disabled for as short time as possible.

> The keypad IRQ isn't shared IRQ and in our case hardirq handler will
> always return IRQ_WAKE_THREAD like default

not entirely true, you still want to check if *this* HW generated to
interrupt in case you get spurious IRQs and whatnot.

> irq_default_primary_handler do. With IRQF_ONESHOT flag IRQ line will
> be masked until the threaded
> handler finished, but there is only keypad on this line.
> 
> I tested two versions:
> the first one - just threaded IRQs with IRQF_ONESHOT and without
> specific hardirq handler.
> the second version - threaded IRQs without IRQF_ONESHOT as you described.
> Both versions was successfully tested on Blaze's keypad.

you can't simply remove IRQF_ONESHOT, you need to mask this interrupt at
the keypad level, basically you clear IRQ_ENABLE_SET (or write to
IRQ_CLEAR or whatever it's called).

Remember that the hardirq handler runs with IRQs disabled and what you
need to guarantee is that once you go over your hardirq, you mask
keypad's IRQs only.

Currently, the RT patchset forces all IRQs to run as threads, but that's
less than optimal.

-- 
balbi

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

Re: [PATCH 0/2] Input: omap-keypad: Cleanup - remove hardcoded values and IRQ enabling/disabling

[Illia Smyrnov]

On 07/19/2013 04:03 PM, Illia Smyrnov wrote:
> From: Illia Smyrnov <illia.smyrnov@ti.com>
>
> Replace unclear hardcoded values with bit field and remove
> unnecessary IRQ enabling/disabling.
>
> Based on top of v3.10-rc1.

Sorry, it is typo here. Based on v3.11-rc1.

>
> Tested on OMAP4 SDP.
>
> Illia Smyrnov (2):
>    Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded
>      values
>    Input: omap-keypad: Cleanup - remove unnecessary IRQ
>      enabling/disabling
>
>   drivers/input/keyboard/omap4-keypad.c |   40 ++++++++++++--------------------
>   1 files changed, 15 insertions(+), 25 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>