Input: omap-keypad: dynamically handle register offsets

From: G, Manjunath Kondaiah <[email protected]>

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 <[email protected]>

Signed-off-by G, Manjunath Kondaiah <[email protected]>

Signed-off-by Sourav Poddar <[email protected]>

---
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(-)

drivers/input/keyboard/Kconfig +0 -0

index f354813..9a0e1a9 100644
@@ -511,10 +511,10 @@ config KEYBOARD_OMAP

? ? ? ? ?To compile this driver as a module, choose M here: the ? ? ? ? ?module will be called omap-keypad.

drivers/input/keyboard/Kconfig +2 -2

-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.

drivers/input/keyboard/omap4-keypad.c +0 -0

index e809ac0..742e1e3 100644
@@ -68,6 +68,11 @@
 

?#define OMAP4_MASK_IRQSTATUSDISABLE ? ?0xFFFF

drivers/input/keyboard/omap4-keypad.c +5 -0

+enum {
+ ? ? ? KBD_REVISION_OMAP4 = 0,
+ ? ? ? KBD_REVISION_OMAP5,
+};
+

?struct omap4_keypad {
? ? ? ?struct input_dev *input;

drivers/input/keyboard/omap4-keypad.c +55 -0

@@ -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) ?{

drivers/input/keyboard/omap4-keypad.c +0 -0

@@ -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];

drivers/input/keyboard/omap4-keypad.c +0 -0

@@ -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;
?}

drivers/input/keyboard/omap4-keypad.c +24 -10

@@ -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

From: G, Manjunath Kondaiah <[email protected]>

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 <[email protected]>

Signed-off-by G, Manjunath Kondaiah <[email protected]>

Signed-off-by Sourav Poddar <[email protected]>

---
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(-)

drivers/input/keyboard/Kconfig +3 -3

index f354813..9a0e1a9 100644
@@ -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.

drivers/input/keyboard/omap4-keypad.c +14 -0

index e809ac0..742e1e3 100644
@@ -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,

drivers/input/keyboard/omap4-keypad.c +12 -0

+
+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)

drivers/input/keyboard/omap4-keypad.c +14 -0

+	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;
 

drivers/input/keyboard/omap4-keypad.c +34 -18

+	*(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);
 

Hi Felipe,

On Tue, Apr 10, 2012 at 2:54 PM, Felipe Balbi <[email protected]> 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.

Quoted 21 lines · Sourav

Hi Felipe,

On Tue, Apr 10, 2012 at 2:54 PM, Felipe Balbi <[email protected]> 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.

Quoted 6 lines

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

From: G, Manjunath Kondaiah <[email protected]>

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

drivers/input/keyboard/omap4-keypad.c +6 -0

+}
+
+static int kbd_read_revision(struct omap4_keypad *keypad_data, u32 offset)
+{
+	int reg;
+	reg = __raw_readl(keypad_data->base + offset);

drivers/input/keyboard/omap4-keypad.c +12 -11

+	}
+}
+
 /* 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);

drivers/input/keyboard/omap4-keypad.c +14 -2

+	}
+
+	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,

Quoted 6 lines · Sourav Poddar

From: G, Manjunath Kondaiah <[email protected]>

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.

drivers/input/keyboard/omap4-keypad.c struct omap4_keypad { · line 86

Quoted 7 lines

drivers/input/keyboard/omap4-keypad.c +3 -0

@@ -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.

Quoted 5 lines

drivers/input/keyboard/omap4-keypad.c +0 -0

 	unsigned int row_shift;
 	unsigned char key_state[8];
 	unsigned short keymap[];
 };
 

drivers/input/keyboard/omap4-keypad.c struct omap4_keypad { · line 112

Quoted 4 lines

drivers/input/keyboard/omap4-keypad.c +4 -0

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

Quoted 27 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

From: G, Manjunath Kondaiah <[email protected]>

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.

drivers/input/keyboard/omap4-keypad.c struct omap4_keypad { · line 86

Quoted 7 lines

drivers/input/keyboard/omap4-keypad.c +3 -0

@@ -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.

Quoted 5 lines

drivers/input/keyboard/omap4-keypad.c +0 -0

 	unsigned int row_shift;
 	unsigned char key_state[8];
 	unsigned short keymap[];
 };
 

Quoted 4 lines

drivers/input/keyboard/omap4-keypad.c struct omap4_keypad { · line 112

Quoted 4 lines

drivers/input/keyboard/omap4-keypad.c +4 -0

+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?

Hi Sourav,

Quoted 6 lines · Sourav Poddar

From: G, Manjunath Kondaiah <[email protected]>

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.

Review struct omap4_keypad { · line 86

Quoted 7 lines

(quoted diff) +3 -0

@@ -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.

Review struct omap4_keypad { · line 99

Quoted 13 lines

(quoted diff) +8 -0

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

Quoted 1 line

(quoted diff) +1 -0

+}

Review struct omap4_keypad { · line 108

Quoted 8 lines

(quoted diff) +7 -0

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

Quoted 11 lines

(quoted diff) +10 -0

+}
+
+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) {

Review struct omap4_keypad { · line 125

Quoted 6 lines

(quoted diff) +6 -0

+ ? ? case 1:
+ ? ? ? ? ? ? return KBD_REVISION_OMAP5;
+ ? ? case 0:
+ ? ? ? ? ? ? return KBD_REVISION_OMAP4;
+ ? ? default:
+ ? ? ? ? ? ? return -ENODEV;

-EINVAL? -EIO?

Quoted 51 lines

drivers/input/keyboard/omap4-keypad.c +3 -0

+ ? ? }
+}
+

?/* Interrupt handler */
?static irqreturn_t omap4_keypad_interrupt(int irq, void *dev_id) ?{

(quoted diff) +0 -0

@@ -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];

(quoted diff) +0 -0

@@ -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;
?}

(quoted diff) +0 -0

@@ -139,16 +199,30 @@ static int omap4_keypad_open(struct input_dev
 *input)
 
 ? ? ? disable_irq(keypad_data->irq);
 

Review static int omap4_keypad_open(struct input_dev · line 205

Quoted 13 lines

(quoted diff) +2 -10

- ? ? __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()

Review static int omap4_keypad_open(struct input_dev · line 214

Quoted 9 lines

(quoted diff) +9 -0

+ ? ? 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().

Quoted 29 lines

(quoted diff) +13 -0

+ ? ? }
+
+ ? ? 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);

(quoted diff) +0 -0

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

Quoted 8 lines

? ? ? /* 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

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+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:

Quoted 3 lines

? ? ? ?return __raw_readl(keypad_data->base + keypad_data->reg_offset + ? ? ? ? ? ? ? ? ? ? ? ? ? offset);

Quoted 1 line

+}

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.

Quoted 13 lines · Sourav

Quoted 13 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

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

Quoted 5 lines

Quoted 1 line

Quoted 1 line

+}

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