* [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
@ 2012-04-03 5:22 Sourav Poddar
2012-04-06 7:41 ` S, Venkatraman
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Sourav Poddar @ 2012-04-03 5:22 UTC (permalink / raw)
To: dmitry.torokhov, linux-input, linux-arm-kernel, linux-omap
Cc: sourav.poddar, balbi
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-03 5:22 [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets Sourav Poddar
@ 2012-04-06 7:41 ` S, Venkatraman
2012-04-06 13:26 ` Shubhrajyoti
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: S, Venkatraman @ 2012-04-06 7:41 UTC (permalink / raw)
To: Sourav Poddar
Cc: dmitry.torokhov, linux-input, linux-arm-kernel, linux-omap, balbi
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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-03 5:22 [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets Sourav Poddar
2012-04-06 7:41 ` S, Venkatraman
@ 2012-04-06 13:26 ` Shubhrajyoti
2012-04-10 9:24 ` Felipe Balbi
2012-04-10 16:23 ` Dmitry Torokhov
3 siblings, 0 replies; 12+ messages in thread
From: Shubhrajyoti @ 2012-04-06 13:26 UTC (permalink / raw)
To: Sourav Poddar
Cc: dmitry.torokhov, linux-input, linux-arm-kernel, linux-omap, balbi
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);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-03 5:22 [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets Sourav Poddar
2012-04-06 7:41 ` S, Venkatraman
2012-04-06 13:26 ` Shubhrajyoti
@ 2012-04-10 9:24 ` Felipe Balbi
[not found] ` <CAKdam56-Pu+WJKLUpodTzOzGuGbAHuqZ3F-zVmrAtFM7EDhxpg@mail.gmail.com>
2012-04-10 16:23 ` Dmitry Torokhov
3 siblings, 1 reply; 12+ messages in thread
From: Felipe Balbi @ 2012-04-10 9:24 UTC (permalink / raw)
To: Sourav Poddar
Cc: dmitry.torokhov, linux-input, linux-arm-kernel, linux-omap, balbi
[-- Attachment #1: Type: text/plain, Size: 534 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
[not found] ` <CAKdam56-Pu+WJKLUpodTzOzGuGbAHuqZ3F-zVmrAtFM7EDhxpg@mail.gmail.com>
@ 2012-04-10 10:16 ` Felipe Balbi
2012-04-10 10:30 ` Poddar, Sourav
0 siblings, 1 reply; 12+ messages in thread
From: Felipe Balbi @ 2012-04-10 10:16 UTC (permalink / raw)
To: Poddar, Sourav
Cc: balbi, dmitry.torokhov, linux-input, linux-arm-kernel, linux-omap
[-- Attachment #1: Type: text/plain, Size: 1508 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-10 10:16 ` Felipe Balbi
@ 2012-04-10 10:30 ` Poddar, Sourav
0 siblings, 0 replies; 12+ messages in thread
From: Poddar, Sourav @ 2012-04-10 10:30 UTC (permalink / raw)
To: balbi; +Cc: dmitry.torokhov, linux-input, linux-arm-kernel, linux-omap
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-03 5:22 [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets Sourav Poddar
` (2 preceding siblings ...)
2012-04-10 9:24 ` Felipe Balbi
@ 2012-04-10 16:23 ` Dmitry Torokhov
2012-04-10 16:39 ` H Hartley Sweeten
2012-04-11 8:49 ` Poddar, Sourav
3 siblings, 2 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2012-04-10 16:23 UTC (permalink / raw)
To: Sourav Poddar; +Cc: linux-input, linux-arm-kernel, linux-omap, balbi
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-10 16:23 ` Dmitry Torokhov
@ 2012-04-10 16:39 ` H Hartley Sweeten
2012-04-10 17:47 ` Dmitry Torokhov
2012-04-11 8:49 ` Poddar, Sourav
1 sibling, 1 reply; 12+ messages in thread
From: H Hartley Sweeten @ 2012-04-10 16:39 UTC (permalink / raw)
To: Dmitry Torokhov, Sourav Poddar
Cc: linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-omap@vger.kernel.org, balbi@ti.com
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-10 16:39 ` H Hartley Sweeten
@ 2012-04-10 17:47 ` Dmitry Torokhov
0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2012-04-10 17:47 UTC (permalink / raw)
To: H Hartley Sweeten
Cc: Sourav Poddar, linux-input@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org,
balbi@ti.com
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-10 16:23 ` Dmitry Torokhov
2012-04-10 16:39 ` H Hartley Sweeten
@ 2012-04-11 8:49 ` Poddar, Sourav
2012-04-11 8:56 ` Felipe Balbi
1 sibling, 1 reply; 12+ messages in thread
From: Poddar, Sourav @ 2012-04-11 8:49 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-arm-kernel, linux-omap, balbi
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-11 8:49 ` Poddar, Sourav
@ 2012-04-11 8:56 ` Felipe Balbi
2012-04-11 9:04 ` Poddar, Sourav
0 siblings, 1 reply; 12+ messages in thread
From: Felipe Balbi @ 2012-04-11 8:56 UTC (permalink / raw)
To: Poddar, Sourav
Cc: Dmitry Torokhov, linux-input, linux-arm-kernel, linux-omap, balbi
[-- Attachment #1: Type: text/plain, Size: 928 bytes --]
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets
2012-04-11 8:56 ` Felipe Balbi
@ 2012-04-11 9:04 ` Poddar, Sourav
0 siblings, 0 replies; 12+ messages in thread
From: Poddar, Sourav @ 2012-04-11 9:04 UTC (permalink / raw)
To: balbi; +Cc: Dmitry Torokhov, linux-input, linux-arm-kernel, linux-omap
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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-04-11 9:04 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-03 5:22 [PATCH RESEND] Input: omap-keypad: dynamically handle register offsets Sourav Poddar
2012-04-06 7:41 ` S, Venkatraman
2012-04-06 13:26 ` Shubhrajyoti
2012-04-10 9:24 ` Felipe Balbi
[not found] ` <CAKdam56-Pu+WJKLUpodTzOzGuGbAHuqZ3F-zVmrAtFM7EDhxpg@mail.gmail.com>
2012-04-10 10:16 ` Felipe Balbi
2012-04-10 10:30 ` Poddar, Sourav
2012-04-10 16:23 ` Dmitry Torokhov
2012-04-10 16:39 ` H Hartley Sweeten
2012-04-10 17:47 ` Dmitry Torokhov
2012-04-11 8:49 ` Poddar, Sourav
2012-04-11 8:56 ` Felipe Balbi
2012-04-11 9:04 ` Poddar, Sourav
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).