[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Sourav Poddar]

From: G, Manjunath Kondaiah <manjugk@ti.com>

Keypad controller register offsets are different for omap4
and omap5. Handle these offsets through static mapping and
assign these mappings during run time.

Signed-off-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
Test Info
 - Tested on omap4 sdp with 3.4-rc1
 - Tested on omap5430evm with 3.1 custom kernel.
 drivers/input/keyboard/Kconfig        |    6 +-
 drivers/input/keyboard/omap4-keypad.c |  120 ++++++++++++++++++++++++++------
 2 files changed, 100 insertions(+), 26 deletions(-)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index f354813..9a0e1a9 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -511,10 +511,10 @@ config KEYBOARD_OMAP
 	  To compile this driver as a module, choose M here: the
 	  module will be called omap-keypad.
 
-config KEYBOARD_OMAP4
-	tristate "TI OMAP4 keypad support"
+config KEYBOARD_OMAP4+
+	tristate "TI OMAP4+ keypad support"
 	help
-	  Say Y here if you want to use the OMAP4 keypad.
+	  Say Y here if you want to use the OMAP4+ keypad.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called omap4-keypad.
diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index e809ac0..742e1e3 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -68,6 +68,11 @@
 
 #define OMAP4_MASK_IRQSTATUSDISABLE	0xFFFF
 
+enum {
+	KBD_REVISION_OMAP4 = 0,
+	KBD_REVISION_OMAP5,
+};
+
 struct omap4_keypad {
 	struct input_dev *input;
 
@@ -76,11 +81,66 @@ struct omap4_keypad {
 
 	unsigned int rows;
 	unsigned int cols;
+	unsigned int revision;
+	u32 irqstatus;
+	u32 irqenable;
 	unsigned int row_shift;
 	unsigned char key_state[8];
 	unsigned short keymap[];
 };
 
+static int kbd_read_irqstatus(struct omap4_keypad *keypad_data, u32 offset)
+{
+	return __raw_readl(keypad_data->base + offset);
+}
+
+static int kbd_write_irqstatus(struct omap4_keypad *keypad_data,
+						u32 offset, u32 value)
+{
+	return __raw_writel(value, keypad_data->base + offset);
+}
+
+static int kbd_write_irqenable(struct omap4_keypad *keypad_data,
+						u32 offset, u32 value)
+{
+	return __raw_writel(value, keypad_data->base + offset);
+}
+
+static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
+{
+	if (keypad_data->revision == KBD_REVISION_OMAP4)
+		return __raw_readl(keypad_data->base + offset);
+	else if (keypad_data->revision == KBD_REVISION_OMAP5)
+		return __raw_readl(keypad_data->base + offset + 0x10);
+
+	return -ENODEV;
+}
+
+static void kbd_writel(struct omap4_keypad *keypad_data, u32 offset, u32 value)
+{
+	if (keypad_data->revision == KBD_REVISION_OMAP4)
+		__raw_writel(value, keypad_data->base + offset);
+	else if (keypad_data->revision == KBD_REVISION_OMAP5)
+		__raw_writel(value, keypad_data->base + offset + 0x10);
+}
+
+static int kbd_read_revision(struct omap4_keypad *keypad_data, u32 offset)
+{
+	int reg;
+	reg = __raw_readl(keypad_data->base + offset);
+	reg &= 0x03 << 30;
+	reg >>= 30;
+
+	switch (reg) {
+	case 1:
+		return KBD_REVISION_OMAP5;
+	case 0:
+		return KBD_REVISION_OMAP4;
+	default:
+		return -ENODEV;
+	}
+}
+
 /* Interrupt handler */
 static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
 {
@@ -91,12 +151,11 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
 	u32 *new_state = (u32 *) key_state;
 
 	/* Disable interrupts */
-	__raw_writel(OMAP4_VAL_IRQDISABLE,
-		     keypad_data->base + OMAP4_KBD_IRQENABLE);
+	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
+			OMAP4_VAL_IRQDISABLE);
 
-	*new_state = __raw_readl(keypad_data->base + OMAP4_KBD_FULLCODE31_0);
-	*(new_state + 1) = __raw_readl(keypad_data->base
-						+ OMAP4_KBD_FULLCODE63_32);
+	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
+	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
 
 	for (row = 0; row < keypad_data->rows; row++) {
 		changed = key_state[row] ^ keypad_data->key_state[row];
@@ -121,12 +180,13 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
 		sizeof(keypad_data->key_state));
 
 	/* clear pending interrupts */
-	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
-			keypad_data->base + OMAP4_KBD_IRQSTATUS);
+	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
+		kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
 
 	/* enable interrupts */
-	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
-			keypad_data->base + OMAP4_KBD_IRQENABLE);
+	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
+		OMAP4_DEF_IRQENABLE_EVENTEN |
+			OMAP4_DEF_IRQENABLE_LONGKEY);
 
 	return IRQ_HANDLED;
 }
@@ -139,16 +199,30 @@ static int omap4_keypad_open(struct input_dev *input)
 
 	disable_irq(keypad_data->irq);
 
-	__raw_writel(OMAP4_VAL_FUNCTIONALCFG,
-			keypad_data->base + OMAP4_KBD_CTRL);
-	__raw_writel(OMAP4_VAL_DEBOUNCINGTIME,
-			keypad_data->base + OMAP4_KBD_DEBOUNCINGTIME);
-	__raw_writel(OMAP4_VAL_IRQDISABLE,
-			keypad_data->base + OMAP4_KBD_IRQSTATUS);
-	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
-			keypad_data->base + OMAP4_KBD_IRQENABLE);
-	__raw_writel(OMAP4_DEF_WUP_EVENT_ENA | OMAP4_DEF_WUP_LONG_KEY_ENA,
-			keypad_data->base + OMAP4_KBD_WAKEUPENABLE);
+	keypad_data->revision = kbd_read_revision(keypad_data,
+			OMAP4_KBD_REVISION);
+	if (keypad_data->revision == KBD_REVISION_OMAP4) {
+		keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS;
+		keypad_data->irqenable = OMAP4_KBD_IRQENABLE;
+	} else if (keypad_data->revision == KBD_REVISION_OMAP5) {
+		keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS + 0x0c;
+		keypad_data->irqenable = OMAP4_KBD_IRQENABLE + 0x0c;
+	} else {
+		pr_err("Omap keypad not found\n");
+		return -ENODEV;
+	}
+
+	kbd_writel(keypad_data, OMAP4_KBD_CTRL,
+			OMAP4_VAL_FUNCTIONALCFG);
+	kbd_writel(keypad_data, OMAP4_KBD_DEBOUNCINGTIME,
+			OMAP4_VAL_DEBOUNCINGTIME);
+	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
+			OMAP4_VAL_IRQDISABLE);
+	kbd_write_irqenable(keypad_data, keypad_data->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);
 
 	enable_irq(keypad_data->irq);
 
@@ -162,12 +236,12 @@ static void omap4_keypad_close(struct input_dev *input)
 	disable_irq(keypad_data->irq);
 
 	/* Disable interrupts */
-	__raw_writel(OMAP4_VAL_IRQDISABLE,
-		     keypad_data->base + OMAP4_KBD_IRQENABLE);
+	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
+			OMAP4_VAL_IRQDISABLE);
 
 	/* clear pending interrupts */
-	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
-			keypad_data->base + OMAP4_KBD_IRQSTATUS);
+	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
+		kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
 
 	enable_irq(keypad_data->irq);
 
-- 
1.7.1

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[S, Venkatraman]

On Tue, Apr 3, 2012 at 10:52 AM, Sourav Poddar <sourav.poddar@ti.com> wrote:
> From: G, Manjunath Kondaiah <manjugk@ti.com>
>
> Keypad controller register offsets are different for omap4
> and omap5. Handle these offsets through static mapping and
> assign these mappings during run time.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

FWIW,
Reviewed-by: Venkatraman S <svenkatr@ti.com>

> ---
> Test Info
> ?- Tested on omap4 sdp with 3.4-rc1
> ?- Tested on omap5430evm with 3.1 custom kernel.
> ?drivers/input/keyboard/Kconfig ? ? ? ?| ? ?6 +-
> ?drivers/input/keyboard/omap4-keypad.c | ?120 ++++++++++++++++++++++++++------
> ?2 files changed, 100 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index f354813..9a0e1a9 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -511,10 +511,10 @@ config KEYBOARD_OMAP
> ? ? ? ? ?To compile this driver as a module, choose M here: the
> ? ? ? ? ?module will be called omap-keypad.
>
> -config KEYBOARD_OMAP4
> - ? ? ? tristate "TI OMAP4 keypad support"
> +config KEYBOARD_OMAP4+
> + ? ? ? tristate "TI OMAP4+ keypad support"
> ? ? ? ?help
> - ? ? ? ? Say Y here if you want to use the OMAP4 keypad.
> + ? ? ? ? Say Y here if you want to use the OMAP4+ keypad.
>
> ? ? ? ? ?To compile this driver as a module, choose M here: the
> ? ? ? ? ?module will be called omap4-keypad.
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index e809ac0..742e1e3 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -68,6 +68,11 @@
>
> ?#define OMAP4_MASK_IRQSTATUSDISABLE ? ?0xFFFF
>
> +enum {
> + ? ? ? KBD_REVISION_OMAP4 = 0,
> + ? ? ? KBD_REVISION_OMAP5,
> +};
> +
> ?struct omap4_keypad {
> ? ? ? ?struct input_dev *input;
>
> @@ -76,11 +81,66 @@ struct omap4_keypad {
>
> ? ? ? ?unsigned int rows;
> ? ? ? ?unsigned int cols;
> + ? ? ? unsigned int revision;
> + ? ? ? u32 irqstatus;
> + ? ? ? u32 irqenable;
> ? ? ? ?unsigned int row_shift;
> ? ? ? ?unsigned char key_state[8];
> ? ? ? ?unsigned short keymap[];
> ?};
>
> +static int kbd_read_irqstatus(struct omap4_keypad *keypad_data, u32 offset)
> +{
> + ? ? ? return __raw_readl(keypad_data->base + offset);
> +}
> +
> +static int kbd_write_irqstatus(struct omap4_keypad *keypad_data,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 offset, u32 value)
> +{
> + ? ? ? return __raw_writel(value, keypad_data->base + offset);
> +}
> +
> +static int kbd_write_irqenable(struct omap4_keypad *keypad_data,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 offset, u32 value)
> +{
> + ? ? ? return __raw_writel(value, keypad_data->base + offset);
> +}
> +
> +static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
> +{
> + ? ? ? if (keypad_data->revision == KBD_REVISION_OMAP4)
> + ? ? ? ? ? ? ? return __raw_readl(keypad_data->base + offset);
> + ? ? ? else if (keypad_data->revision == KBD_REVISION_OMAP5)
> + ? ? ? ? ? ? ? return __raw_readl(keypad_data->base + offset + 0x10);
> +
> + ? ? ? return -ENODEV;
> +}
> +
> +static void kbd_writel(struct omap4_keypad *keypad_data, u32 offset, u32 value)
> +{
> + ? ? ? if (keypad_data->revision == KBD_REVISION_OMAP4)
> + ? ? ? ? ? ? ? __raw_writel(value, keypad_data->base + offset);
> + ? ? ? else if (keypad_data->revision == KBD_REVISION_OMAP5)
> + ? ? ? ? ? ? ? __raw_writel(value, keypad_data->base + offset + 0x10);
> +}
> +
> +static int kbd_read_revision(struct omap4_keypad *keypad_data, u32 offset)
> +{
> + ? ? ? int reg;
> + ? ? ? reg = __raw_readl(keypad_data->base + offset);
> + ? ? ? reg &= 0x03 << 30;
> + ? ? ? reg >>= 30;
> +
> + ? ? ? switch (reg) {
> + ? ? ? case 1:
> + ? ? ? ? ? ? ? return KBD_REVISION_OMAP5;
> + ? ? ? case 0:
> + ? ? ? ? ? ? ? return KBD_REVISION_OMAP4;
> + ? ? ? default:
> + ? ? ? ? ? ? ? return -ENODEV;
> + ? ? ? }
> +}
> +
> ?/* Interrupt handler */
> ?static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
> ?{
> @@ -91,12 +151,11 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
> ? ? ? ?u32 *new_state = (u32 *) key_state;
>
> ? ? ? ?/* Disable interrupts */
> - ? ? ? __raw_writel(OMAP4_VAL_IRQDISABLE,
> - ? ? ? ? ? ? ? ? ? ?keypad_data->base + OMAP4_KBD_IRQENABLE);
> + ? ? ? kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> + ? ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_IRQDISABLE);
>
> - ? ? ? *new_state = __raw_readl(keypad_data->base + OMAP4_KBD_FULLCODE31_0);
> - ? ? ? *(new_state + 1) = __raw_readl(keypad_data->base
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + OMAP4_KBD_FULLCODE63_32);
> + ? ? ? *new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
> + ? ? ? *(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
>
> ? ? ? ?for (row = 0; row < keypad_data->rows; row++) {
> ? ? ? ? ? ? ? ?changed = key_state[row] ^ keypad_data->key_state[row];
> @@ -121,12 +180,13 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
> ? ? ? ? ? ? ? ?sizeof(keypad_data->key_state));
>
> ? ? ? ?/* clear pending interrupts */
> - ? ? ? __raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> - ? ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQSTATUS);
> + ? ? ? kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> + ? ? ? ? ? ? ? kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
>
> ? ? ? ?/* enable interrupts */
> - ? ? ? __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> - ? ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQENABLE);
> + ? ? ? kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> + ? ? ? ? ? ? ? OMAP4_DEF_IRQENABLE_EVENTEN |
> + ? ? ? ? ? ? ? ? ? ? ? OMAP4_DEF_IRQENABLE_LONGKEY);
>
> ? ? ? ?return IRQ_HANDLED;
> ?}
> @@ -139,16 +199,30 @@ static int omap4_keypad_open(struct input_dev *input)
>
> ? ? ? ?disable_irq(keypad_data->irq);
>
> - ? ? ? __raw_writel(OMAP4_VAL_FUNCTIONALCFG,
> - ? ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_CTRL);
> - ? ? ? __raw_writel(OMAP4_VAL_DEBOUNCINGTIME,
> - ? ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_DEBOUNCINGTIME);
> - ? ? ? __raw_writel(OMAP4_VAL_IRQDISABLE,
> - ? ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQSTATUS);
> - ? ? ? __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> - ? ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQENABLE);
> - ? ? ? __raw_writel(OMAP4_DEF_WUP_EVENT_ENA | OMAP4_DEF_WUP_LONG_KEY_ENA,
> - ? ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_WAKEUPENABLE);
> + ? ? ? keypad_data->revision = kbd_read_revision(keypad_data,
> + ? ? ? ? ? ? ? ? ? ? ? OMAP4_KBD_REVISION);
> + ? ? ? if (keypad_data->revision == KBD_REVISION_OMAP4) {
> + ? ? ? ? ? ? ? keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS;
> + ? ? ? ? ? ? ? keypad_data->irqenable = OMAP4_KBD_IRQENABLE;
> + ? ? ? } else if (keypad_data->revision == KBD_REVISION_OMAP5) {
> + ? ? ? ? ? ? ? keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS + 0x0c;
> + ? ? ? ? ? ? ? keypad_data->irqenable = OMAP4_KBD_IRQENABLE + 0x0c;
> + ? ? ? } else {
> + ? ? ? ? ? ? ? pr_err("Omap keypad not found\n");
> + ? ? ? ? ? ? ? return -ENODEV;
> + ? ? ? }
> +
> + ? ? ? kbd_writel(keypad_data, OMAP4_KBD_CTRL,
> + ? ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_FUNCTIONALCFG);
> + ? ? ? kbd_writel(keypad_data, OMAP4_KBD_DEBOUNCINGTIME,
> + ? ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_DEBOUNCINGTIME);
> + ? ? ? kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> + ? ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_IRQDISABLE);
> + ? ? ? kbd_write_irqenable(keypad_data, keypad_data->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);
>
> ? ? ? ?enable_irq(keypad_data->irq);
>
> @@ -162,12 +236,12 @@ static void omap4_keypad_close(struct input_dev *input)
> ? ? ? ?disable_irq(keypad_data->irq);
>
> ? ? ? ?/* Disable interrupts */
> - ? ? ? __raw_writel(OMAP4_VAL_IRQDISABLE,
> - ? ? ? ? ? ? ? ? ? ?keypad_data->base + OMAP4_KBD_IRQENABLE);
> + ? ? ? kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> + ? ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_IRQDISABLE);
>
> ? ? ? ?/* clear pending interrupts */
> - ? ? ? __raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> - ? ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQSTATUS);
> + ? ? ? kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> + ? ? ? ? ? ? ? kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
>
> ? ? ? ?enable_irq(keypad_data->irq);
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Shubhrajyoti]

 Hi Sourav,
Some suggestions/ doubts.

On Tuesday 03 April 2012 10:52 AM, Sourav Poddar wrote:
> From: G, Manjunath Kondaiah <manjugk@ti.com>
>
> Keypad controller register offsets are different for omap4
> and omap5. Handle these offsets through static mapping and
> assign these mappings during run time.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> Test Info
>  - Tested on omap4 sdp with 3.4-rc1
>  - Tested on omap5430evm with 3.1 custom kernel.
>  drivers/input/keyboard/Kconfig        |    6 +-
>  drivers/input/keyboard/omap4-keypad.c |  120 ++++++++++++++++++++++++++------
>  2 files changed, 100 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index f354813..9a0e1a9 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -511,10 +511,10 @@ config KEYBOARD_OMAP
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called omap-keypad.
>  
> -config KEYBOARD_OMAP4
> -	tristate "TI OMAP4 keypad support"
> +config KEYBOARD_OMAP4+
> +	tristate "TI OMAP4+ keypad support"
>  	help
> -	  Say Y here if you want to use the OMAP4 keypad.
> +	  Say Y here if you want to use the OMAP4+ keypad.
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called omap4-keypad.
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index e809ac0..742e1e3 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -68,6 +68,11 @@
>  
>  #define OMAP4_MASK_IRQSTATUSDISABLE	0xFFFF
>  
> +enum {
> +	KBD_REVISION_OMAP4 = 0,
> +	KBD_REVISION_OMAP5,
> +};
> +
>  struct omap4_keypad {
>  	struct input_dev *input;
>  
> @@ -76,11 +81,66 @@ struct omap4_keypad {
>  
>  	unsigned int rows;
>  	unsigned int cols;
> +	unsigned int revision;
> +	u32 irqstatus;
> +	u32 irqenable;
>  	unsigned int row_shift;
>  	unsigned char key_state[8];
>  	unsigned short keymap[];
>  };
>  
> +static int kbd_read_irqstatus(struct omap4_keypad *keypad_data, u32 offset)
> +{
> +	return __raw_readl(keypad_data->base + offset);
> +}
> +
> +static int kbd_write_irqstatus(struct omap4_keypad *keypad_data,
> +						u32 offset, u32 value)
> +{
> +	return __raw_writel(value, keypad_data->base + offset);
> +}
> +
> +static int kbd_write_irqenable(struct omap4_keypad *keypad_data,
> +						u32 offset, u32 value)
> +{
> +	return __raw_writel(value, keypad_data->base + offset);
> +}
The functions

kbd_write_irqenable and kbd_write_irqstatus look identical.
Could we optimise it? 
 
Feel free to ignore such trivial comments.

> +
> +static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
> +{
> +	if (keypad_data->revision == KBD_REVISION_OMAP4)
> +		return __raw_readl(keypad_data->base + offset);
> +	else if (keypad_data->revision == KBD_REVISION_OMAP5)
> +		return __raw_readl(keypad_data->base + offset + 0x10);
> +
> +	return -ENODEV;
> +}
> +
> +static void kbd_writel(struct omap4_keypad *keypad_data, u32 offset, u32 value)
> +{
> +	if (keypad_data->revision == KBD_REVISION_OMAP4)
> +		__raw_writel(value, keypad_data->base + offset);
> +	else if (keypad_data->revision == KBD_REVISION_OMAP5)
> +		__raw_writel(value, keypad_data->base + offset + 0x10);
> +}
> +
> +static int kbd_read_revision(struct omap4_keypad *keypad_data, u32 offset)
> +{
> +	int reg;
Can this be unsigned?

> +	reg = __raw_readl(keypad_data->base + offset);
> +	reg &= 0x03 << 30;
> +	reg >>= 30;
> +
> +	switch (reg) {
> +	case 1:
> +		return KBD_REVISION_OMAP5;
> +	case 0:
> +		return KBD_REVISION_OMAP4;
> +	default:
> +		return -ENODEV;
> +	}
> +}
> +
>  /* Interrupt handler */
>  static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  {
> @@ -91,12 +151,11 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  	u32 *new_state = (u32 *) key_state;
>  
>  	/* Disable interrupts */
> -	__raw_writel(OMAP4_VAL_IRQDISABLE,
> -		     keypad_data->base + OMAP4_KBD_IRQENABLE);
> +	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> +			OMAP4_VAL_IRQDISABLE);
>  
> -	*new_state = __raw_readl(keypad_data->base + OMAP4_KBD_FULLCODE31_0);
> -	*(new_state + 1) = __raw_readl(keypad_data->base
> -						+ OMAP4_KBD_FULLCODE63_32);
> +	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
In the error case a negative value is returned.
Could we prevent writing in that cases.
> +	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
>  
>  	for (row = 0; row < keypad_data->rows; row++) {
>  		changed = key_state[row] ^ keypad_data->key_state[row];
> @@ -121,12 +180,13 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  		sizeof(keypad_data->key_state));
>  
>  	/* clear pending interrupts */
> -	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> -			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> +	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> +		kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
>  
>  	/* enable interrupts */
> -	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> -			keypad_data->base + OMAP4_KBD_IRQENABLE);
> +	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> +		OMAP4_DEF_IRQENABLE_EVENTEN |
> +			OMAP4_DEF_IRQENABLE_LONGKEY);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -139,16 +199,30 @@ static int omap4_keypad_open(struct input_dev *input)
>  
>  	disable_irq(keypad_data->irq);
>  
> -	__raw_writel(OMAP4_VAL_FUNCTIONALCFG,
> -			keypad_data->base + OMAP4_KBD_CTRL);
> -	__raw_writel(OMAP4_VAL_DEBOUNCINGTIME,
> -			keypad_data->base + OMAP4_KBD_DEBOUNCINGTIME);
> -	__raw_writel(OMAP4_VAL_IRQDISABLE,
> -			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> -	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> -			keypad_data->base + OMAP4_KBD_IRQENABLE);
> -	__raw_writel(OMAP4_DEF_WUP_EVENT_ENA | OMAP4_DEF_WUP_LONG_KEY_ENA,
> -			keypad_data->base + OMAP4_KBD_WAKEUPENABLE);
> +	keypad_data->revision = kbd_read_revision(keypad_data,
> +			OMAP4_KBD_REVISION);
> +	if (keypad_data->revision == KBD_REVISION_OMAP4) {
> +		keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS;
> +		keypad_data->irqenable = OMAP4_KBD_IRQENABLE;
> +	} else if (keypad_data->revision == KBD_REVISION_OMAP5) {
> +		keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS + 0x0c;
> +		keypad_data->irqenable = OMAP4_KBD_IRQENABLE + 0x0c;
> +	} else {
> +		pr_err("Omap keypad not found\n");
> +		return -ENODEV;
> +	}
> +
> +	kbd_writel(keypad_data, OMAP4_KBD_CTRL,
> +			OMAP4_VAL_FUNCTIONALCFG);
> +	kbd_writel(keypad_data, OMAP4_KBD_DEBOUNCINGTIME,
> +			OMAP4_VAL_DEBOUNCINGTIME);
> +	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> +			OMAP4_VAL_IRQDISABLE);
> +	kbd_write_irqenable(keypad_data, keypad_data->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);
>  
>  	enable_irq(keypad_data->irq);
>  
> @@ -162,12 +236,12 @@ static void omap4_keypad_close(struct input_dev *input)
>  	disable_irq(keypad_data->irq);
>  
>  	/* Disable interrupts */
> -	__raw_writel(OMAP4_VAL_IRQDISABLE,
> -		     keypad_data->base + OMAP4_KBD_IRQENABLE);
> +	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> +			OMAP4_VAL_IRQDISABLE);
>  
>  	/* clear pending interrupts */
> -	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> -			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> +	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> +		kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
>  
>  	enable_irq(keypad_data->irq);
>  

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Felipe Balbi]

On Tue, Apr 03, 2012 at 10:52:26AM +0530, Sourav Poddar wrote:
> +static int kbd_write_irqstatus(struct omap4_keypad *keypad_data,
> +						u32 offset, u32 value)
> +{
> +	return __raw_writel(value, keypad_data->base + offset);
> +}
> +
> +static int kbd_write_irqenable(struct omap4_keypad *keypad_data,
> +						u32 offset, u32 value)
> +{
> +	return __raw_writel(value, keypad_data->base + offset);
> +}

if this only writes to irqenable, why do you pass offset as an argument?
likewise for irqstatus.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120410/8f295da2/attachment-0001.sig>

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Felipe Balbi]

On Tue, Apr 10, 2012 at 03:37:28PM +0530, Poddar, Sourav wrote:
> Hi Felipe,
> 
> On Tue, Apr 10, 2012 at 2:54 PM, Felipe Balbi <balbi@ti.com> wrote:
> 
>     On Tue, Apr 03, 2012 at 10:52:26AM +0530, Sourav Poddar wrote:
>     > +static int kbd_write_irqstatus(struct omap4_keypad *keypad_data,
>     > +                                             u32 offset, u32 value)
>     > +{
>     > +     return __raw_writel(value, keypad_data->base + offset);
>     > +}
>     > +
>     > +static int kbd_write_irqenable(struct omap4_keypad *keypad_data,
>     > +                                             u32 offset, u32 value)
>     > +{
>     > +     return __raw_writel(value, keypad_data->base + offset);
>     > +}
> 
>     if this only writes to irqenable, why do you pass offset as an argument?
>     likewise for irqstatus.
>    
> 
> Actually, here the offset is the "irqenable" register addresss offset and
> "irqstatus" register
> address offset from the base address.
> Since, the above two address are different in case of Omap4 and omap5, we are
> passing an offset
> depending on the particular revision of the keyboard selected during probe. 

but you already have those in keypad_data->irqenable/irqstatus right ?
Meaning you can access them directly here and save on the arguments:

static int kbd_write_irqenable(struct omap4_keypad *keypad_data, u32 value)
{
     return __raw_writel(value, keypad_data->base + keypad_data->irqenable);
}


-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120410/02f5a088/attachment.sig>

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Poddar, Sourav]

Hi Felipe,

On Tue, Apr 10, 2012 at 3:46 PM, Felipe Balbi <balbi@ti.com> wrote:
>
> On Tue, Apr 10, 2012 at 03:37:28PM +0530, Poddar, Sourav wrote:
> > Hi Felipe,
> >
> > On Tue, Apr 10, 2012 at 2:54 PM, Felipe Balbi <balbi@ti.com> wrote:
> >
> > ? ? On Tue, Apr 03, 2012 at 10:52:26AM +0530, Sourav Poddar wrote:
> > ? ? > +static int kbd_write_irqstatus(struct omap4_keypad *keypad_data,
> > ? ? > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 offset, u32 value)
> > ? ? > +{
> > ? ? > + ? ? return __raw_writel(value, keypad_data->base + offset);
> > ? ? > +}
> > ? ? > +
> > ? ? > +static int kbd_write_irqenable(struct omap4_keypad *keypad_data,
> > ? ? > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 offset, u32 value)
> > ? ? > +{
> > ? ? > + ? ? return __raw_writel(value, keypad_data->base + offset);
> > ? ? > +}
> >
> > ? ? if this only writes to irqenable, why do you pass offset as an argument?
> > ? ? likewise for irqstatus.
> >
> >
> > Actually, here the offset is the "irqenable" register addresss offset and
> > "irqstatus" register
> > address offset from the base address.
> > Since, the above two address are different in case of Omap4 and omap5, we are
> > passing an offset
> > depending on the particular revision of the keyboard selected during probe.
>
> but you already have those in keypad_data->irqenable/irqstatus right ?

Yes.
>
> Meaning you can access them directly here and save on the arguments:
>
> static int kbd_write_irqenable(struct omap4_keypad *keypad_data, u32 value)
> {
> ? ? return __raw_writel(value, keypad_data->base + keypad_data->irqenable);
> }
>
Thanks a lot for pointing this out. Yes, we should be able to directly use
the driver data for the above api.
Will make the necessary change in the next version.
~Sourav
>
>
> --
> balbi

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Dmitry Torokhov]

Hi Sourav,

On Tue, Apr 03, 2012 at 10:52:26AM +0530, Sourav Poddar wrote:
> From: G, Manjunath Kondaiah <manjugk@ti.com>
> 
> Keypad controller register offsets are different for omap4
> and omap5. Handle these offsets through static mapping and
> assign these mappings during run time.
> 

In addition to Felipe's comments.

> @@ -76,11 +81,66 @@ struct omap4_keypad {
>  
>  	unsigned int rows;
>  	unsigned int cols;
> +	unsigned int revision;
> +	u32 irqstatus;
> +	u32 irqenable;

	u32 reg_offset;

and you probably won't need revision field.

>  	unsigned int row_shift;
>  	unsigned char key_state[8];
>  	unsigned short keymap[];
>  };
>  
> +static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
> +{
> +	if (keypad_data->revision == KBD_REVISION_OMAP4)
> +		return __raw_readl(keypad_data->base + offset);
> +	else if (keypad_data->revision == KBD_REVISION_OMAP5)
> +		return __raw_readl(keypad_data->base + offset + 0x10);
> +
> +	return -ENODEV;

Instead do:

	return __raw_readl(keypad_data->base + keypad_data->reg_offset +
			   offset);
> +}
> +
> +static void kbd_writel(struct omap4_keypad *keypad_data, u32 offset, u32 value)
> +{
> +	if (keypad_data->revision == KBD_REVISION_OMAP4)
> +		__raw_writel(value, keypad_data->base + offset);
> +	else if (keypad_data->revision == KBD_REVISION_OMAP5)
> +		__raw_writel(value, keypad_data->base + offset + 0x10);

	__raw_writel(value,
		     keypad_data->base + keypad_data->reg_offset + offset);

> +}
> +
> +static int kbd_read_revision(struct omap4_keypad *keypad_data, u32 offset)
> +{
> +	int reg;
> +	reg = __raw_readl(keypad_data->base + offset);
> +	reg &= 0x03 << 30;
> +	reg >>= 30;
> +
> +	switch (reg) {
> +	case 1:
> +		return KBD_REVISION_OMAP5;
> +	case 0:
> +		return KBD_REVISION_OMAP4;
> +	default:
> +		return -ENODEV;

-EINVAL? -EIO?

> +	}
> +}
> +
>  /* Interrupt handler */
>  static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  {
> @@ -91,12 +151,11 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  	u32 *new_state = (u32 *) key_state;
>  
>  	/* Disable interrupts */
> -	__raw_writel(OMAP4_VAL_IRQDISABLE,
> -		     keypad_data->base + OMAP4_KBD_IRQENABLE);
> +	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> +			OMAP4_VAL_IRQDISABLE);
>  
> -	*new_state = __raw_readl(keypad_data->base + OMAP4_KBD_FULLCODE31_0);
> -	*(new_state + 1) = __raw_readl(keypad_data->base
> -						+ OMAP4_KBD_FULLCODE63_32);
> +	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
> +	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
>  
>  	for (row = 0; row < keypad_data->rows; row++) {
>  		changed = key_state[row] ^ keypad_data->key_state[row];
> @@ -121,12 +180,13 @@ static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>  		sizeof(keypad_data->key_state));
>  
>  	/* clear pending interrupts */
> -	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> -			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> +	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> +		kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
>  
>  	/* enable interrupts */
> -	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> -			keypad_data->base + OMAP4_KBD_IRQENABLE);
> +	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> +		OMAP4_DEF_IRQENABLE_EVENTEN |
> +			OMAP4_DEF_IRQENABLE_LONGKEY);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -139,16 +199,30 @@ static int omap4_keypad_open(struct input_dev *input)
>  
>  	disable_irq(keypad_data->irq);
>  
> -	__raw_writel(OMAP4_VAL_FUNCTIONALCFG,
> -			keypad_data->base + OMAP4_KBD_CTRL);
> -	__raw_writel(OMAP4_VAL_DEBOUNCINGTIME,
> -			keypad_data->base + OMAP4_KBD_DEBOUNCINGTIME);
> -	__raw_writel(OMAP4_VAL_IRQDISABLE,
> -			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> -	__raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
> -			keypad_data->base + OMAP4_KBD_IRQENABLE);
> -	__raw_writel(OMAP4_DEF_WUP_EVENT_ENA | OMAP4_DEF_WUP_LONG_KEY_ENA,
> -			keypad_data->base + OMAP4_KBD_WAKEUPENABLE);
> +	keypad_data->revision = kbd_read_revision(keypad_data,
> +			OMAP4_KBD_REVISION);

	switch()
> +	if (keypad_data->revision == KBD_REVISION_OMAP4) {
> +		keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS;
> +		keypad_data->irqenable = OMAP4_KBD_IRQENABLE;
> +	} else if (keypad_data->revision == KBD_REVISION_OMAP5) {
> +		keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS + 0x0c;
> +		keypad_data->irqenable = OMAP4_KBD_IRQENABLE + 0x0c;
> +	} else {
> +		pr_err("Omap keypad not found\n");
> +		return -ENODEV;

Hmm, this is in open() but should probably be in probe().

> +	}
> +
> +	kbd_writel(keypad_data, OMAP4_KBD_CTRL,
> +			OMAP4_VAL_FUNCTIONALCFG);
> +	kbd_writel(keypad_data, OMAP4_KBD_DEBOUNCINGTIME,
> +			OMAP4_VAL_DEBOUNCINGTIME);
> +	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> +			OMAP4_VAL_IRQDISABLE);
> +	kbd_write_irqenable(keypad_data, keypad_data->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);
>  
>  	enable_irq(keypad_data->irq);
>  
> @@ -162,12 +236,12 @@ static void omap4_keypad_close(struct input_dev *input)
>  	disable_irq(keypad_data->irq);
>  
>  	/* Disable interrupts */
> -	__raw_writel(OMAP4_VAL_IRQDISABLE,
> -		     keypad_data->base + OMAP4_KBD_IRQENABLE);
> +	kbd_write_irqenable(keypad_data, keypad_data->irqenable,
> +			OMAP4_VAL_IRQDISABLE);
>  
>  	/* clear pending interrupts */
> -	__raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
> -			keypad_data->base + OMAP4_KBD_IRQSTATUS);
> +	kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
> +		kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
>  
>  	enable_irq(keypad_data->irq);
>  

Thanks.

-- 
Dmitry

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[H Hartley Sweeten]

On Tuesday, April 10, 2012 9:24 AM, Dmitry Torokhov wrote:
> On Tue, Apr 03, 2012 at 10:52:26AM +0530, Sourav Poddar wrote:
>> From: G, Manjunath Kondaiah <manjugk@ti.com>
>> 
>> Keypad controller register offsets are different for omap4
>> and omap5. Handle these offsets through static mapping and
>> assign these mappings during run time.
>> 
>
> In addition to Felipe's comments.
>
>> @@ -76,11 +81,66 @@ struct omap4_keypad {
>>  
>>  	unsigned int rows;
>>  	unsigned int cols;
>> +	unsigned int revision;
>> +	u32 irqstatus;
>> +	u32 irqenable;
>
>	u32 reg_offset;
>
> and you probably won't need revision field.
>
>>  	unsigned int row_shift;
>>  	unsigned char key_state[8];
>>  	unsigned short keymap[];
>>  };
>>  
>> +static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
>> +{
>> +	if (keypad_data->revision == KBD_REVISION_OMAP4)
>> +		return __raw_readl(keypad_data->base + offset);

keypad_data->base is an ioremap'ed address. Shouldn't all the __raw_{read,write}l be
{read,write}l instead?

Regards,
Hartley

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Dmitry Torokhov]

On Tue, Apr 10, 2012 at 11:39:43AM -0500, H Hartley Sweeten wrote:
> On Tuesday, April 10, 2012 9:24 AM, Dmitry Torokhov wrote:
> > On Tue, Apr 03, 2012 at 10:52:26AM +0530, Sourav Poddar wrote:
> >> From: G, Manjunath Kondaiah <manjugk@ti.com>
> >> 
> >> Keypad controller register offsets are different for omap4
> >> and omap5. Handle these offsets through static mapping and
> >> assign these mappings during run time.
> >> 
> >
> > In addition to Felipe's comments.
> >
> >> @@ -76,11 +81,66 @@ struct omap4_keypad {
> >>  
> >>  	unsigned int rows;
> >>  	unsigned int cols;
> >> +	unsigned int revision;
> >> +	u32 irqstatus;
> >> +	u32 irqenable;
> >
> >	u32 reg_offset;
> >
> > and you probably won't need revision field.
> >
> >>  	unsigned int row_shift;
> >>  	unsigned char key_state[8];
> >>  	unsigned short keymap[];
> >>  };
> >>  
> >> +static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
> >> +{
> >> +	if (keypad_data->revision == KBD_REVISION_OMAP4)
> >> +		return __raw_readl(keypad_data->base + offset);
> 
> keypad_data->base is an ioremap'ed address. Shouldn't all the __raw_{read,write}l be
> {read,write}l instead?

Hmm, does it need endian conversion?

-- 
Dmitry

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Poddar, Sourav]

Hi Dmitry,

On Tue, Apr 10, 2012 at 9:53 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Sourav,
>
> On Tue, Apr 03, 2012 at 10:52:26AM +0530, Sourav Poddar wrote:
>> From: G, Manjunath Kondaiah <manjugk@ti.com>
>>
>> Keypad controller register offsets are different for omap4
>> and omap5. Handle these offsets through static mapping and
>> assign these mappings during run time.
>>
>
> In addition to Felipe's comments.
>
>> @@ -76,11 +81,66 @@ struct omap4_keypad {
>>
>> ? ? ? unsigned int rows;
>> ? ? ? unsigned int cols;
>> + ? ? unsigned int revision;
>> + ? ? u32 irqstatus;
>> + ? ? u32 irqenable;
>
> ? ? ? ?u32 reg_offset;
>
> and you probably won't need revision field.
>
>> ? ? ? unsigned int row_shift;
>> ? ? ? unsigned char key_state[8];
>> ? ? ? unsigned short keymap[];
>> ?};
>>
>> +static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
>> +{
>> + ? ? if (keypad_data->revision == KBD_REVISION_OMAP4)
>> + ? ? ? ? ? ? return __raw_readl(keypad_data->base + offset);
>> + ? ? else if (keypad_data->revision == KBD_REVISION_OMAP5)
>> + ? ? ? ? ? ? return __raw_readl(keypad_data->base + offset + 0x10);
>> +
>> + ? ? return -ENODEV;
>
> Instead do:
>
> ? ? ? ?return __raw_readl(keypad_data->base + keypad_data->reg_offset +
> ? ? ? ? ? ? ? ? ? ? ? ? ? offset);
>> +}
I have a couple of doubts on this:
1. Before using kbd_readl/kbd_write anywhere we
need to populate the "keypad_data->reg_offset"  with the register address
which we need to read or write.
>> +
>> +static void kbd_writel(struct omap4_keypad *keypad_data, u32 offset, u32
>> value)
>> +{
>> + ? ? if (keypad_data->revision == KBD_REVISION_OMAP4)
>> + ? ? ? ? ? ? __raw_writel(value, keypad_data->base + offset);
>> + ? ? else if (keypad_data->revision == KBD_REVISION_OMAP5)
>> + ? ? ? ? ? ? __raw_writel(value, keypad_data->base + offset + 0x10);
>
> ? ? ? ?__raw_writel(value,
> ? ? ? ? ? ? ? ? ? ? keypad_data->base + keypad_data->reg_offset + offset);
>
>> +}
>> +
>> +static int kbd_read_revision(struct omap4_keypad *keypad_data, u32
>> offset)
>> +{
>> + ? ? int reg;
>> + ? ? reg = __raw_readl(keypad_data->base + offset);
>> + ? ? reg &= 0x03 << 30;
>> + ? ? reg >>= 30;
>> +
>> + ? ? switch (reg) {
>> + ? ? case 1:
>> + ? ? ? ? ? ? return KBD_REVISION_OMAP5;
>> + ? ? case 0:
>> + ? ? ? ? ? ? return KBD_REVISION_OMAP4;
>> + ? ? default:
>> + ? ? ? ? ? ? return -ENODEV;
>
> -EINVAL? -EIO?
Hmm, -EINVAL looks more apt here.Will change.
>
>> + ? ? }
>> +}
>> +
>> ?/* Interrupt handler */
>> ?static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id)
>> ?{
>> @@ -91,12 +151,11 @@ static irqreturn_t omap4_keypad_interrupt(int irq,
>> void *dev_id)
>> ? ? ? u32 *new_state = (u32 *) key_state;
>>
>> ? ? ? /* Disable interrupts */
>> - ? ? __raw_writel(OMAP4_VAL_IRQDISABLE,
>> - ? ? ? ? ? ? ? ? ?keypad_data->base + OMAP4_KBD_IRQENABLE);
>> + ? ? kbd_write_irqenable(keypad_data, keypad_data->irqenable,
>> + ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_IRQDISABLE);
>>
>> - ? ? *new_state = __raw_readl(keypad_data->base +
>> OMAP4_KBD_FULLCODE31_0);
>> - ? ? *(new_state + 1) = __raw_readl(keypad_data->base
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + OMAP4_KBD_FULLCODE63_32);
>> + ? ? *new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
>> + ? ? *(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
>>
>> ? ? ? for (row = 0; row < keypad_data->rows; row++) {
>> ? ? ? ? ? ? ? changed = key_state[row] ^ keypad_data->key_state[row];
>> @@ -121,12 +180,13 @@ static irqreturn_t omap4_keypad_interrupt(int irq,
>> void *dev_id)
>> ? ? ? ? ? ? ? sizeof(keypad_data->key_state));
>>
>> ? ? ? /* clear pending interrupts */
>> - ? ? __raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
>> - ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQSTATUS);
>> + ? ? kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
>> + ? ? ? ? ? ? kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
>>
>> ? ? ? /* enable interrupts */
>> - ? ? __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN |
>> OMAP4_DEF_IRQENABLE_LONGKEY,
>> - ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQENABLE);
>> + ? ? kbd_write_irqenable(keypad_data, keypad_data->irqenable,
>> + ? ? ? ? ? ? OMAP4_DEF_IRQENABLE_EVENTEN |
>> + ? ? ? ? ? ? ? ? ? ? OMAP4_DEF_IRQENABLE_LONGKEY);
>>
>> ? ? ? return IRQ_HANDLED;
>> ?}
>> @@ -139,16 +199,30 @@ static int omap4_keypad_open(struct input_dev
>> *input)
>>
>> ? ? ? disable_irq(keypad_data->irq);
>>
>> - ? ? __raw_writel(OMAP4_VAL_FUNCTIONALCFG,
>> - ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_CTRL);
>> - ? ? __raw_writel(OMAP4_VAL_DEBOUNCINGTIME,
>> - ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_DEBOUNCINGTIME);
>> - ? ? __raw_writel(OMAP4_VAL_IRQDISABLE,
>> - ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQSTATUS);
>> - ? ? __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN |
>> OMAP4_DEF_IRQENABLE_LONGKEY,
>> - ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQENABLE);
>> - ? ? __raw_writel(OMAP4_DEF_WUP_EVENT_ENA | OMAP4_DEF_WUP_LONG_KEY_ENA,
>> - ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_WAKEUPENABLE);
>> + ? ? keypad_data->revision = kbd_read_revision(keypad_data,
>> + ? ? ? ? ? ? ? ? ? ? OMAP4_KBD_REVISION);
>
> ? ? ? ?switch()
Ok. Will covert that to switch statement.
>> + ? ? if (keypad_data->revision == KBD_REVISION_OMAP4) {
>> + ? ? ? ? ? ? keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS;
>> + ? ? ? ? ? ? keypad_data->irqenable = OMAP4_KBD_IRQENABLE;
>> + ? ? } else if (keypad_data->revision == KBD_REVISION_OMAP5) {
>> + ? ? ? ? ? ? keypad_data->irqstatus = OMAP4_KBD_IRQSTATUS + 0x0c;
>> + ? ? ? ? ? ? keypad_data->irqenable = OMAP4_KBD_IRQENABLE + 0x0c;
>> + ? ? } else {
>> + ? ? ? ? ? ? pr_err("Omap keypad not found\n");
>> + ? ? ? ? ? ? return -ENODEV;
>
> Hmm, this is in open() but should probably be in probe().
This requires reading of KBD_REVISION register, which can
be done once the clocks are enabled. As the current implementation
goes, clocks get enabled only when some input device is opened.
>

>> + ? ? }
>> +
>> + ? ? kbd_writel(keypad_data, OMAP4_KBD_CTRL,
>> + ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_FUNCTIONALCFG);
>> + ? ? kbd_writel(keypad_data, OMAP4_KBD_DEBOUNCINGTIME,
>> + ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_DEBOUNCINGTIME);
>> + ? ? kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
>> + ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_IRQDISABLE);
>> + ? ? kbd_write_irqenable(keypad_data, keypad_data->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);
>>
>> ? ? ? enable_irq(keypad_data->irq);
>>
>> @@ -162,12 +236,12 @@ static void omap4_keypad_close(struct input_dev
>> *input)
>> ? ? ? disable_irq(keypad_data->irq);
>>
>> ? ? ? /* Disable interrupts */
>> - ? ? __raw_writel(OMAP4_VAL_IRQDISABLE,
>> - ? ? ? ? ? ? ? ? ?keypad_data->base + OMAP4_KBD_IRQENABLE);
>> + ? ? kbd_write_irqenable(keypad_data, keypad_data->irqenable,
>> + ? ? ? ? ? ? ? ? ? ? OMAP4_VAL_IRQDISABLE);
>>
>> ? ? ? /* clear pending interrupts */
>> - ? ? __raw_writel(__raw_readl(keypad_data->base + OMAP4_KBD_IRQSTATUS),
>> - ? ? ? ? ? ? ? ? ? ? keypad_data->base + OMAP4_KBD_IRQSTATUS);
>> + ? ? kbd_write_irqstatus(keypad_data, keypad_data->irqstatus,
>> + ? ? ? ? ? ? kbd_read_irqstatus(keypad_data, keypad_data->irqstatus));
>>
>> ? ? ? enable_irq(keypad_data->irq);
>>
>
> Thanks.
>
> --
> Dmitry

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Felipe Balbi]

On Wed, Apr 11, 2012 at 02:19:48PM +0530, Poddar, Sourav wrote:
> >> +static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
> >> +{
> >> + ? ? if (keypad_data->revision == KBD_REVISION_OMAP4)
> >> + ? ? ? ? ? ? return __raw_readl(keypad_data->base + offset);
> >> + ? ? else if (keypad_data->revision == KBD_REVISION_OMAP5)
> >> + ? ? ? ? ? ? return __raw_readl(keypad_data->base + offset + 0x10);
> >> +
> >> + ? ? return -ENODEV;
> >
> > Instead do:
> >
> > ? ? ? ?return __raw_readl(keypad_data->base + keypad_data->reg_offset +
> > ? ? ? ? ? ? ? ? ? ? ? ? ? offset);
> >> +}
> I have a couple of doubts on this:
> 1. Before using kbd_readl/kbd_write anywhere we
> need to populate the "keypad_data->reg_offset"  with the register address
> which we need to read or write.

no no... keypad_data->reg_offset would be 0 or 0x10 depending on the
scheme you decode from revision.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120411/c9cb20d9/attachment.sig>

[PATCH RESEND] Input: omap-keypad: dynamically handle register offsets

[Poddar, Sourav]

Hi Felipe,

On Wed, Apr 11, 2012 at 2:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Apr 11, 2012 at 02:19:48PM +0530, Poddar, Sourav wrote:
>> >> +static int kbd_readl(struct omap4_keypad *keypad_data, u32 offset)
>> >> +{
>> >> + ? ? if (keypad_data->revision == KBD_REVISION_OMAP4)
>> >> + ? ? ? ? ? ? return __raw_readl(keypad_data->base + offset);
>> >> + ? ? else if (keypad_data->revision == KBD_REVISION_OMAP5)
>> >> + ? ? ? ? ? ? return __raw_readl(keypad_data->base + offset + 0x10);
>> >> +
>> >> + ? ? return -ENODEV;
>> >
>> > Instead do:
>> >
>> > ? ? ? ?return __raw_readl(keypad_data->base + keypad_data->reg_offset +
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? offset);
>> >> +}
>> I have a couple of doubts on this:
>> 1. Before using kbd_readl/kbd_write anywhere we
>> need to populate the "keypad_data->reg_offset" ?with the register address
>> which we need to read or write.
>
> no no... keypad_data->reg_offset would be 0 or 0x10 depending on the
> scheme you decode from revision.
Ohk. Got it. Will make the necessary changes.
Thanks.

~Sourav
>
> --
> balbi