[PATCH v2 0/3] Input: omap-keypad: Convert to threaded IRQ and cleanup

[PATCH v2 0/3] Input: omap-keypad: Convert to threaded IRQ and cleanup

[Illia Smyrnov]

Replace unclear hardcoded values with bit field, convert to threaded IRQ and
clear pending interrupts when open the keypad.

Based on top of v3.11-rc2.

Tested on OMAP4 SDP.

Illia Smyrnov (3):
  Input: omap-keypad: Cleanup - use bitfiled instead of hardcoded
    values
  Input: omap-keypad: Convert to threaded IRQ
  Input: omap-keypad: Clear pending interrupts on open

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

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

[Illia Smyrnov]

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 v2 2/3] Input: omap-keypad: Convert to threaded IRQ

[Illia Smyrnov]

Convert to use threaded IRQ.

Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 drivers/input/keyboard/omap4-keypad.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index c727548..b876a0d 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -112,8 +112,22 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
 }
 
 
-/* Interrupt handler */
-static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
+/* Interrupt handlers */
+static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
+{
+	struct omap4_keypad *keypad_data = dev_id;
+
+	if (kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS)) {
+		/* Disable interrupts */
+		kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
+				 OMAP4_VAL_IRQDISABLE);
+		return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 {
 	struct omap4_keypad *keypad_data = dev_id;
 	struct input_dev *input_dev = keypad_data->input;
@@ -121,10 +135,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);
 
@@ -360,9 +370,10 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 		goto err_free_keymap;
 	}
 
-	error = request_irq(keypad_data->irq, omap4_keypad_interrupt,
-			     IRQF_TRIGGER_RISING,
-			     "omap4-keypad", keypad_data);
+	error = request_threaded_irq(keypad_data->irq, omap4_keypad_irq_handler,
+				     omap4_keypad_irq_thread_fn,
+				     IRQF_TRIGGER_RISING,
+				     "omap4-keypad", keypad_data);
 	if (error) {
 		dev_err(&pdev->dev, "failed to register interrupt\n");
 		goto err_free_input;
-- 
1.7.0.4

[PATCH v2 3/3] Input: omap-keypad: Clear pending interrupts on open

[Illia Smyrnov]

Clear pending interrupts when open keypad.

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

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index b876a0d..62abc3e 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -185,13 +185,14 @@ 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);
 
-- 
1.7.0.4

Re: [PATCH v2 3/3] Input: omap-keypad: Clear pending interrupts on open

[Felipe Balbi]

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

Hi,

On Tue, Jul 23, 2013 at 07:09:58PM +0300, Illia Smyrnov wrote:
> Clear pending interrupts when open keypad.

where are these interrupts coming from ?

> Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index b876a0d..62abc3e 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -185,13 +185,14 @@ 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));

you might want to clear these interrupts before unmasking them :-)

-- 
balbi

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

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

[Felipe Balbi]

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

Hi,

On Tue, Jul 23, 2013 at 07:09:56PM +0300, Illia Smyrnov wrote:
> 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>

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

-- 
balbi

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

Re: [PATCH v2 2/3] Input: omap-keypad: Convert to threaded IRQ

[Felipe Balbi]

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

Hi,

On Tue, Jul 23, 2013 at 07:09:57PM +0300, Illia Smyrnov wrote:
> Convert to use threaded IRQ.
> 
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |   29 ++++++++++++++++++++---------
>  1 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index c727548..b876a0d 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -112,8 +112,22 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
>  }
>  
>  
> -/* Interrupt handler */
> -static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
> +/* Interrupt handlers */
> +static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
> +{
> +	struct omap4_keypad *keypad_data = dev_id;
> +
> +	if (kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS)) {
> +		/* Disable interrupts */
> +		kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
> +				 OMAP4_VAL_IRQDISABLE);
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
>  {
>  	struct omap4_keypad *keypad_data = dev_id;
>  	struct input_dev *input_dev = keypad_data->input;
> @@ -121,10 +135,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);

looking a lot better, but I wonder if you should add a mutex to this
threaded handler, but I guess there's no way this will never race since
IRQs are masked anyway and registers are only accessed from within IRQ
handlers.

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

-- 
balbi

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

Re: [PATCH v2 2/3] Input: omap-keypad: Convert to threaded IRQ

[Felipe Balbi]

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

Hi,

On Tue, Jul 23, 2013 at 08:25:01PM +0300, Felipe Balbi wrote:
> > Convert to use threaded IRQ.
> > 
> > Cc: Felipe Balbi <balbi@ti.com>
> > Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
> > ---
> >  drivers/input/keyboard/omap4-keypad.c |   29 ++++++++++++++++++++---------
> >  1 files changed, 20 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> > index c727548..b876a0d 100644
> > --- a/drivers/input/keyboard/omap4-keypad.c
> > +++ b/drivers/input/keyboard/omap4-keypad.c
> > @@ -112,8 +112,22 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
> >  }
> >  
> >  
> > -/* Interrupt handler */
> > -static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
> > +/* Interrupt handlers */
> > +static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
> > +{
> > +	struct omap4_keypad *keypad_data = dev_id;
> > +
> > +	if (kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS)) {
> > +		/* Disable interrupts */
> > +		kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
> > +				 OMAP4_VAL_IRQDISABLE);
> > +		return IRQ_WAKE_THREAD;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> > +static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
> >  {
> >  	struct omap4_keypad *keypad_data = dev_id;
> >  	struct input_dev *input_dev = keypad_data->input;
> > @@ -121,10 +135,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);
> 
> looking a lot better, but I wonder if you should add a mutex to this
> threaded handler, but I guess there's no way this will never race since

s/never/ever

-- 
balbi

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