Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Wed, May 12, 2010 at 11:15:11AM +0530, Shilimkar, Santosh wrote:
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Arce,
Abraham
Sent: Wednesday, May 12, 2010 11:10 AM
To: Dmitry Torokhov
Cc: linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry,
2 comments + one question before sending next version...
[...]
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
Using now request_irq based on your comments. In same omap_keypad_interrupt
disable/clear/enable
interrupts will be executed
[...]
Sorry for jumping into the comments late. Thought this was sorted out. Key
scanning
and debounce timeouts etc still there. Having all these things in ISR itself
isn't good
idea.
Dmitry,
Don't you think its optimal to push the key-scanning and debounce timeout code
part of bottom half ??
If you need debounce then you need to fire a timer and keep doing this
until interrupt (or key state) settles. It really depends on the device.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
-Original Message-
From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
Sent: Wednesday, May 12, 2010 11:33 AM
To: Shilimkar, Santosh
Cc: Arce, Abraham; linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Wed, May 12, 2010 at 11:15:11AM +0530, Shilimkar, Santosh wrote:
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of
Arce,
Abraham
Sent: Wednesday, May 12, 2010 11:10 AM
To: Dmitry Torokhov
Cc: linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry,
2 comments + one question before sending next version...
[...]
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
Using now request_irq based on your comments. In same
omap_keypad_interrupt disable/clear/enable
interrupts will be executed
[...]
Sorry for jumping into the comments late. Thought this was sorted out. Key
scanning
and debounce timeouts etc still there. Having all these things in ISR
itself isn't good
idea.
Dmitry,
Don't you think its optimal to push the key-scanning and debounce timeout
code
part of bottom half ??
If you need debounce then you need to fire a timer and keep doing this
until interrupt (or key state) settles. It really depends on the device.
The OMAP4 keypad controller has internal timeout mechanism and doesn't need any
external timer for this.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Sorry for jumping into the comments late. Thought this was sorted out. Key scanning and debounce timeouts etc still there. Having all these things in ISR itself isn't good idea. Dmitry, Don't you think its optimal to push the key-scanning and debounce timeout code part of bottom half ?? If you need debounce then you need to fire a timer and keep doing this until interrupt (or key state) settles. It really depends on the device. Controller includes a debouncing feature... -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Wed, May 12, 2010 at 11:49:45AM +0530, Shilimkar, Santosh wrote:
-Original Message-
From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
Sent: Wednesday, May 12, 2010 11:33 AM
To: Shilimkar, Santosh
Cc: Arce, Abraham; linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Wed, May 12, 2010 at 11:15:11AM +0530, Shilimkar, Santosh wrote:
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of
Arce,
Abraham
Sent: Wednesday, May 12, 2010 11:10 AM
To: Dmitry Torokhov
Cc: linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry,
2 comments + one question before sending next version...
[...]
+static irqreturn_t omap_keypad_threaded(int irq, void
*dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
Using now request_irq based on your comments. In same
omap_keypad_interrupt disable/clear/enable
interrupts will be executed
[...]
Sorry for jumping into the comments late. Thought this was sorted out.
Key scanning
and debounce timeouts etc still there. Having all these things in ISR
itself isn't good
idea.
Dmitry,
Don't you think its optimal to push the key-scanning and debounce timeout
code
part of bottom half ??
If you need debounce then you need to fire a timer and keep doing this
until interrupt (or key state) settles. It really depends on the device.
The OMAP4 keypad controller has internal timeout mechanism and doesn't need
any
external timer for this.
Then I do not understand the question... If hardware takes care of
debouncing then just read the state and report the data.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
-Original Message-
From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
Sent: Wednesday, May 12, 2010 12:05 PM
To: Shilimkar, Santosh
Cc: Arce, Abraham; linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Wed, May 12, 2010 at 11:49:45AM +0530, Shilimkar, Santosh wrote:
-Original Message-
From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
Sent: Wednesday, May 12, 2010 11:33 AM
To: Shilimkar, Santosh
Cc: Arce, Abraham; linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Wed, May 12, 2010 at 11:15:11AM +0530, Shilimkar, Santosh wrote:
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of
Arce,
Abraham
Sent: Wednesday, May 12, 2010 11:10 AM
To: Dmitry Torokhov
Cc: linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry,
2 comments + one question before sending next version...
[...]
+static irqreturn_t omap_keypad_threaded(int irq, void
*dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see
anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
Using now request_irq based on your comments. In same
omap_keypad_interrupt
disable/clear/enable
interrupts will be executed
[...]
Sorry for jumping into the comments late. Thought this was sorted out.
Key scanning
and debounce timeouts etc still there. Having all these things in ISR
itself isn't good
idea.
Dmitry,
Don't you think its optimal to push the key-scanning and debounce
timeout code
part of bottom half ??
If you need debounce then you need to fire a timer and keep doing this
until interrupt (or key state) settles. It really depends on the device.
The OMAP4 keypad controller has internal timeout mechanism and doesn't need
any
external timer for this.
Then I do not understand the question... If hardware takes care of
debouncing then just read the state and report the data.
You have point. Debounce shouldn't be an issue. If the key scan isn't taking
Much time then I agree with your suggestion.
Abraham, Can you please how much time you are spending in the key scan loops ?
Regards,
Santosh
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry Torokhov dmitry.torok...@gmail.com writes:
On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
Hi again Dmitry,
No worries, although at first I was surprised that Trilok spoke exactly
the same words I did ;)
:)
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
Thanks.
I think Kevin meant use theaded IRQ, wherever possible [if we need
to sleep in interrupt handler].
Actually, even interrupts that don't sleep can use threaded IRQs. I
prefer to see threaded IRQs wherever possible. Especially since we're
moving towards a world where all interrupts are run with interrupts
disabled, using threaded IRQs minimizes interrupts-off critical
sections.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Tue, May 11, 2010 at 07:53:23AM -0700, Kevin Hilman wrote:
Dmitry Torokhov dmitry.torok...@gmail.com writes:
On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
Hi again Dmitry,
No worries, although at first I was surprised that Trilok spoke exactly
the same words I did ;)
:)
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
Thanks.
I think Kevin meant use theaded IRQ, wherever possible [if we need
to sleep in interrupt handler].
Actually, even interrupts that don't sleep can use threaded IRQs. I
prefer to see threaded IRQs wherever possible. Especially since we're
moving towards a world where all interrupts are run with interrupts
disabled, using threaded IRQs minimizes interrupts-off critical
sections.
I think in this case threaded IRQ would just add unnecessary overhead.
There are no scanning delays, just a few register reads and writes.
Input core will take some cycles propagating the events but it disables
interrupts anyway. Setting up a separate thread and scheduling does not
make much sense here.
Also I am not sure if arches with large number of interrupts would want
to move to all threaded interrupts model.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry Torokhov dmitry.torok...@gmail.com writes:
On Tue, May 11, 2010 at 07:53:23AM -0700, Kevin Hilman wrote:
Dmitry Torokhov dmitry.torok...@gmail.com writes:
On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
Hi again Dmitry,
No worries, although at first I was surprised that Trilok spoke exactly
the same words I did ;)
:)
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
Thanks.
I think Kevin meant use theaded IRQ, wherever possible [if we need
to sleep in interrupt handler].
Actually, even interrupts that don't sleep can use threaded IRQs. I
prefer to see threaded IRQs wherever possible. Especially since we're
moving towards a world where all interrupts are run with interrupts
disabled, using threaded IRQs minimizes interrupts-off critical
sections.
I think in this case threaded IRQ would just add unnecessary overhead.
There are no scanning delays, just a few register reads and writes.
Input core will take some cycles propagating the events but it disables
interrupts anyway. Setting up a separate thread and scheduling does not
make much sense here.
Also I am not sure if arches with large number of interrupts would want
to move to all threaded interrupts model.
OK, it's your call for this subsystem.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Kevin
Hilman
Sent: Wednesday, May 12, 2010 3:10 AM
To: Dmitry Torokhov
Cc: Arce, Abraham; linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry Torokhov dmitry.torok...@gmail.com writes:
On Tue, May 11, 2010 at 07:53:23AM -0700, Kevin Hilman wrote:
Dmitry Torokhov dmitry.torok...@gmail.com writes:
On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
Hi again Dmitry,
No worries, although at first I was surprised that Trilok spoke
exactly
the same words I did ;)
:)
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void
*dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
Thanks.
I think Kevin meant use theaded IRQ, wherever possible [if we need
to sleep in interrupt handler].
Actually, even interrupts that don't sleep can use threaded IRQs. I
prefer to see threaded IRQs wherever possible. Especially since we're
moving towards a world where all interrupts are run with interrupts
disabled, using threaded IRQs minimizes interrupts-off critical
sections.
I think in this case threaded IRQ would just add unnecessary overhead.
There are no scanning delays, just a few register reads and writes.
Input core will take some cycles propagating the events but it disables
interrupts anyway. Setting up a separate thread and scheduling does not
make much sense here.
Also I am not sure if arches with large number of interrupts would want
to move to all threaded interrupts model.
OK, it's your call for this subsystem.
Abraham initial driver has top half (ISR) and bottom half (tasklet). With
Threaded
IRQ, we can get rid-of tasklet and handle the bottom half in thread context.
IIRC this was on of the intention of this new API.
Isn't this not good enough to use the threaded IRQ in this case.
Regards,
Santosh
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry,
2 comments + one question before sending next version...
[...]
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
Using now request_irq based on your comments. In same omap_keypad_interrupt
disable/clear/enable interrupts will be executed
[...]
+ input_set_capability(input_dev, EV_MSC, MSC_SCAN);
You forgot to actualy report MSC_SCAN though.
Should I remove it? I think I do need it only when KEY_UNKNOWN keys will be
remapped, right?
I'd rather you actually report EV_MSC/MSC_SCAN.
I am reporting now EV_MSC/MSC_SCAN. I am using evtest to read the events, if I
press and release a key no code will be reported
Event: time 946684836.854248, -- Report Sync
Event: time 946684836.854248, -- Report Sync
Unless I keep it depressed
Event: time 946684817.945892, type 1 (Key), code 18 (E), value 2
And after releasing the key some other events will get generated
Event: time 946684817.967559, type 4 (Misc), code 4 (ScanCode), value 24
Event: time 946684817.967559, -- Report Sync
Event: time 946684817.967559, type 4 (Misc), code 4 (ScanCode), value 32
Event: time 946684817.967559, -- Report Sync
Event: time 946684817.967590, type 4 (Misc), code 4 (ScanCode), value 40
Event: time 946684817.967590, -- Report Sync
Am I missing something?
[...]
+failed_free_irq:
+ free_irq(keypad_data-irq, pdev);
+failed_free_base:
+ keypad_data-base = NULL;
+failed_free_input:
+ input_free_device(input_dev);
+failed_free_codes:
+ kfree(keypad_codes);
+failed_free_data:
+ kfree(keypad_data);
+failed_free_platform:
+ kfree(keymap_data);
+ kfree(pdata);
+ return error;
Using now the following sequence...
pdata = pdev-dev.platform_data;
return -EINVAL;
keymap_data = pdata-keymap_data;
goto failed_free_pdata;
keypad_data = kzalloc(sizeof(struct omap_keypad) +
goto failed_free_keymap;
keypad_data-base = pdata-base;
goto failed_free_keypad;
keypad_data-irq = pdata-irq;
goto failed_free_base;
input_dev = input_allocate_device();
goto failed_free_base;
status = input_register_device(keypad_data-input);
goto failed_free_input;
status = request_irq(keypad_data-irq, omap_keypad_interrupt,
goto failed_unregister_device;
failed_unregister__device:
input_unregister_device(keypad_data-input);
input_dev = NULL;
failed_free_input:
input_free_device(input_dev);
failed_free_base:
keypad_data-base = NULL;
failed_free_keypad:
kfree(keypad_data);
failed_free_keymap:
kfree(keymap_data);
failed_free_pdata:
kfree(pdata);
return error;
Best Regards
Abraham
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Arce,
Abraham
Sent: Wednesday, May 12, 2010 11:10 AM
To: Dmitry Torokhov
Cc: linux-in...@vger.kernel.org; linux-omap@vger.kernel.org
Subject: RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Dmitry,
2 comments + one question before sending next version...
[...]
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
Using now request_irq based on your comments. In same omap_keypad_interrupt
disable/clear/enable
interrupts will be executed
[...]
Sorry for jumping into the comments late. Thought this was sorted out. Key
scanning
and debounce timeouts etc still there. Having all these things in ISR itself
isn't good
idea.
Dmitry,
Don't you think its optimal to push the key-scanning and debounce timeout code
part of bottom half ??
Regards,
Santosh
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Hi Trilok,
Thanks for your comments...
[snip]
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
+ struct omap_keypad *keypad_data = dev_id;
+ struct input_dev *input = keypad_data-input;
+ unsigned char key_state[8];
+ int col, row, code;
+ u32 *new_state = (u32 *) key_state;
+
+ *new_state = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE31_0);
+ *(new_state + 1) = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE63_32);
+
+ for (col = 0; col keypad_data-cols; col++) {
+ for (row = 0; row keypad_data-rows; row++) {
+ code = MATRIX_SCAN_CODE(row, col,
+ keypad_data-row_shift);
+ if (code 0) {
+ printk(KERN_INFO Spurious key %d-%d\n,
+ col, row);
+ continue;
+ }
MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
incorrect values in keymap...
Removed
+ input_report_key(input, keypad_data-keycodes[code],
+ key_state[col] (1 row));
+ }
+ }
+
+ memcpy(keypad_data-key_state, key_state,
+ sizeof(keypad_data-key_state));
+
+ /* enable interrupts */
+ __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
+ keypad_data-base + OMAP4_KBD_IRQENABLE);
+
+ /* clear pending interrupts */
+ __raw_writel(__raw_readl(keypad_data-base + OMAP4_KBD_IRQSTATUS),
+ keypad_data-base + OMAP4_KBD_IRQSTATUS);
+
+ /* clear pending interrupts */
+ return IRQ_HANDLED;
+
+}
+
+
+static int __devinit omap_keypad_probe(struct platform_device *pdev)
+{
+ struct omap_device *od;
+ struct omap_hwmod *oh;
+ struct matrix_keypad_platform_data *pdata;
+ struct input_dev *input_dev;
+ const struct matrix_keymap_data *keymap_data;
+ struct omap_keypad *keypad_data;
+
+ unsigned int status = 0, row_shift = 0, error = 0, length = 0, id = 0;
+ unsigned short *keypad_codes;
+
+ int hw_mod_name_len = 16;
+ char oh_name[hw_mod_name_len];
+ char *name = omap4-keypad;
+
+ length = snprintf(oh_name, hw_mod_name_len, keyboard);
+ WARN(length = hw_mod_name_len,
+ String buffer overflow in keyboard device setup\n);
+
+ oh = omap_hwmod_lookup(oh_name);
+ if (!oh)
+ pr_err(Could not look up %s\n, oh_name);
+
+ pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+ GFP_KERNEL);
Why is it allocated?
Moved to board file...
+
+ od = omap_device_build(name, id, oh, pdata,
+ sizeof(struct matrix_keypad_platform_data),
+ omap_keyboard_latency,
+ ARRAY_SIZE(omap_keyboard_latency), 1);
+ WARN(IS_ERR(od), Could not build omap_device for %s %s\n,
+ name, oh_name);
+
+ /* platform data */
+ pdata = pdev-dev.platform_data;
Umm, here you are overriding pdata pointer... don't you leak memory
doing this?
hwmod has been moved to board configuration so we need to get platform data
+ if (!pdata) {
+ dev_err(pdev-dev, no platform data defined\n);
+ kfree(pdata);
+ return -EINVAL;
+ }
+
+ /* keymap data */
+ keymap_data = pdata-keymap_data;
+ if (!keymap_data) {
+ dev_err(pdev-dev, no keymap data defined\n);
+ kfree(keymap_data);
+ return -EINVAL;
+ }
+
+ /* omap keypad data */
+ row_shift = get_count_order(pdata-num_col_gpios);
+ keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
+ if (!keypad_data) {
+ error = -ENOMEM;
+ goto failed_free_platform;
+ }
+
+ /* omap keypad codes */
+ keypad_codes = kzalloc((pdata-num_row_gpios row_shift) *
+ sizeof(*keypad_codes), GFP_KERNEL);
Why is it al;loctaed separately? Can it be pickky-backed onto
keypad_data?
Not getting the idea here... could you explain?
+ if (!keypad_codes) {
+ error = -ENOMEM;
+ goto failed_free_data;
+ }
+
+ /* input device allocation */
+ input_dev = input_allocate_device();
+ if (!input_dev) {
+ error = -ENOMEM;
+ goto failed_free_codes;
+ }
+
+ /* base resource */
+ keypad_data-base = oh-_rt_va;
+ if (!keypad_data-base) {
You could probably check this earlier, before allocating stuff.
Early means before input_allocate_device?
+ error = -ENOMEM;
+
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Sorry for the confusion in your name Dmitry...
Thanks for your comments...
[snip]
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
+ struct omap_keypad *keypad_data = dev_id;
+ struct input_dev *input = keypad_data-input;
+ unsigned char key_state[8];
+ int col, row, code;
+ u32 *new_state = (u32 *) key_state;
+
+ *new_state = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE31_0);
+ *(new_state + 1) = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE63_32);
+
+ for (col = 0; col keypad_data-cols; col++) {
+ for (row = 0; row keypad_data-rows; row++) {
+ code = MATRIX_SCAN_CODE(row, col,
+ keypad_data-row_shift);
+ if (code 0) {
+ printk(KERN_INFO Spurious key %d-%d\n,
+ col, row);
+ continue;
+ }
MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
incorrect values in keymap...
Removed
+ input_report_key(input, keypad_data-keycodes[code],
+ key_state[col] (1 row));
+ }
+ }
+
+ memcpy(keypad_data-key_state, key_state,
+ sizeof(keypad_data-key_state));
+
+ /* enable interrupts */
+ __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN | OMAP4_DEF_IRQENABLE_LONGKEY,
+ keypad_data-base + OMAP4_KBD_IRQENABLE);
+
+ /* clear pending interrupts */
+ __raw_writel(__raw_readl(keypad_data-base + OMAP4_KBD_IRQSTATUS),
+ keypad_data-base + OMAP4_KBD_IRQSTATUS);
+
+ /* clear pending interrupts */
+ return IRQ_HANDLED;
+
+}
+
+
+static int __devinit omap_keypad_probe(struct platform_device *pdev)
+{
+ struct omap_device *od;
+ struct omap_hwmod *oh;
+ struct matrix_keypad_platform_data *pdata;
+ struct input_dev *input_dev;
+ const struct matrix_keymap_data *keymap_data;
+ struct omap_keypad *keypad_data;
+
+ unsigned int status = 0, row_shift = 0, error = 0, length = 0, id = 0;
+ unsigned short *keypad_codes;
+
+ int hw_mod_name_len = 16;
+ char oh_name[hw_mod_name_len];
+ char *name = omap4-keypad;
+
+ length = snprintf(oh_name, hw_mod_name_len, keyboard);
+ WARN(length = hw_mod_name_len,
+ String buffer overflow in keyboard device setup\n);
+
+ oh = omap_hwmod_lookup(oh_name);
+ if (!oh)
+ pr_err(Could not look up %s\n, oh_name);
+
+ pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+ GFP_KERNEL);
Why is it allocated?
Moved to board file...
+
+ od = omap_device_build(name, id, oh, pdata,
+ sizeof(struct matrix_keypad_platform_data),
+ omap_keyboard_latency,
+ ARRAY_SIZE(omap_keyboard_latency), 1);
+ WARN(IS_ERR(od), Could not build omap_device for %s %s\n,
+ name, oh_name);
+
+ /* platform data */
+ pdata = pdev-dev.platform_data;
Umm, here you are overriding pdata pointer... don't you leak memory
doing this?
hwmod has been moved to board configuration so we need to get platform data
+ if (!pdata) {
+ dev_err(pdev-dev, no platform data defined\n);
+ kfree(pdata);
+ return -EINVAL;
+ }
+
+ /* keymap data */
+ keymap_data = pdata-keymap_data;
+ if (!keymap_data) {
+ dev_err(pdev-dev, no keymap data defined\n);
+ kfree(keymap_data);
+ return -EINVAL;
+ }
+
+ /* omap keypad data */
+ row_shift = get_count_order(pdata-num_col_gpios);
+ keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
+ if (!keypad_data) {
+ error = -ENOMEM;
+ goto failed_free_platform;
+ }
+
+ /* omap keypad codes */
+ keypad_codes = kzalloc((pdata-num_row_gpios row_shift) *
+ sizeof(*keypad_codes), GFP_KERNEL);
Why is it al;loctaed separately? Can it be pickky-backed onto
keypad_data?
Not getting the idea here... could you explain?
+ if (!keypad_codes) {
+ error = -ENOMEM;
+ goto failed_free_data;
+ }
+
+ /* input device allocation */
+ input_dev = input_allocate_device();
+ if (!input_dev) {
+ error = -ENOMEM;
+ goto failed_free_codes;
+ }
+
+ /* base resource */
+ keypad_data-base = oh-_rt_va;
+ if (!keypad_data-base) {
You could probably check this earlier, before allocating stuff.
Early means before input_allocate_device?
+ error = -ENOMEM;
+ goto failed_free_input;
+ }
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Mon, May 10, 2010 at 11:17:50PM -0500, Arce, Abraham wrote:
Sorry for the confusion in your name Dmitry...
No worries, although at first I was surprised that Trilok spoke exactly
the same words I did ;)
Thanks for your comments...
[snip]
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
+ struct omap_keypad *keypad_data = dev_id;
+ struct input_dev *input = keypad_data-input;
+ unsigned char key_state[8];
+ int col, row, code;
+ u32 *new_state = (u32 *) key_state;
+
+ *new_state = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE31_0);
+ *(new_state + 1) = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE63_32);
+
+ for (col = 0; col keypad_data-cols; col++) {
+ for (row = 0; row keypad_data-rows; row++) {
+ code = MATRIX_SCAN_CODE(row, col,
+ keypad_data-row_shift);
+ if (code 0) {
+ printk(KERN_INFO Spurious key %d-%d\n,
+ col, row);
+ continue;
+ }
MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
incorrect values in keymap...
Removed
+ input_report_key(input,
keypad_data-keycodes[code],
+ key_state[col] (1 row));
+ }
+ }
+
+ memcpy(keypad_data-key_state, key_state,
+ sizeof(keypad_data-key_state));
+
+ /* enable interrupts */
+ __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN |
OMAP4_DEF_IRQENABLE_LONGKEY,
+ keypad_data-base + OMAP4_KBD_IRQENABLE);
+
+ /* clear pending interrupts */
+ __raw_writel(__raw_readl(keypad_data-base +
OMAP4_KBD_IRQSTATUS),
+ keypad_data-base + OMAP4_KBD_IRQSTATUS);
+
+ /* clear pending interrupts */
+ return IRQ_HANDLED;
+
+}
+
+
+static int __devinit omap_keypad_probe(struct platform_device *pdev)
+{
+ struct omap_device *od;
+ struct omap_hwmod *oh;
+ struct matrix_keypad_platform_data *pdata;
+ struct input_dev *input_dev;
+ const struct matrix_keymap_data *keymap_data;
+ struct omap_keypad *keypad_data;
+
+ unsigned int status = 0, row_shift = 0, error = 0, length = 0,
id = 0;
+ unsigned short *keypad_codes;
+
+ int hw_mod_name_len = 16;
+ char oh_name[hw_mod_name_len];
+ char *name = omap4-keypad;
+
+ length = snprintf(oh_name, hw_mod_name_len, keyboard);
+ WARN(length = hw_mod_name_len,
+ String buffer overflow in keyboard device setup\n);
+
+ oh = omap_hwmod_lookup(oh_name);
+ if (!oh)
+ pr_err(Could not look up %s\n, oh_name);
+
+ pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+ GFP_KERNEL);
Why is it allocated?
Moved to board file...
+
+ od = omap_device_build(name, id, oh, pdata,
+ sizeof(struct
matrix_keypad_platform_data),
+ omap_keyboard_latency,
+ ARRAY_SIZE(omap_keyboard_latency), 1);
+ WARN(IS_ERR(od), Could not build omap_device for %s %s\n,
+ name, oh_name);
+
+ /* platform data */
+ pdata = pdev-dev.platform_data;
Umm, here you are overriding pdata pointer... don't you leak memory
doing this?
hwmod has been moved to board configuration so we need to get platform data
+ if (!pdata) {
+ dev_err(pdev-dev, no platform data defined\n);
+ kfree(pdata);
+ return -EINVAL;
+ }
+
+ /* keymap data */
+ keymap_data = pdata-keymap_data;
+ if (!keymap_data) {
+ dev_err(pdev-dev, no keymap data defined\n);
+ kfree(keymap_data);
+ return -EINVAL;
+ }
+
+ /* omap keypad data */
+ row_shift = get_count_order(pdata-num_col_gpios);
+ keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
+ if (!keypad_data) {
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Hi again Dmitry,
No worries, although at first I was surprised that Trilok spoke exactly
the same words I did ;)
:)
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
+ struct omap_keypad *keypad_data = dev_id;
+ struct input_dev *input = keypad_data-input;
+ unsigned char key_state[8];
+ int col, row, code;
+ u32 *new_state = (u32 *) key_state;
+
+ *new_state = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE31_0);
+ *(new_state + 1) = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE63_32);
+
+ for (col = 0; col keypad_data-cols; col++) {
+ for (row = 0; row keypad_data-rows; row++) {
+ code = MATRIX_SCAN_CODE(row, col,
+ keypad_data-row_shift);
+ if (code 0) {
+ printk(KERN_INFO Spurious key %d-%d\n,
+ col, row);
+ continue;
+ }
MATRIX_SCAN_CODE() is static data. ISR is wrong place to report
incorrect values in keymap...
Removed
+ input_report_key(input,
keypad_data-keycodes[code],
+ key_state[col] (1 row));
+ }
+ }
+
+ memcpy(keypad_data-key_state, key_state,
+ sizeof(keypad_data-key_state));
+
+ /* enable interrupts */
+ __raw_writel(OMAP4_DEF_IRQENABLE_EVENTEN |
OMAP4_DEF_IRQENABLE_LONGKEY,
+ keypad_data-base + OMAP4_KBD_IRQENABLE);
+
+ /* clear pending interrupts */
+ __raw_writel(__raw_readl(keypad_data-base +
OMAP4_KBD_IRQSTATUS),
+ keypad_data-base + OMAP4_KBD_IRQSTATUS);
+
+ /* clear pending interrupts */
+ return IRQ_HANDLED;
+
+}
+
+
+static int __devinit omap_keypad_probe(struct platform_device *pdev)
+{
+ struct omap_device *od;
+ struct omap_hwmod *oh;
+ struct matrix_keypad_platform_data *pdata;
+ struct input_dev *input_dev;
+ const struct matrix_keymap_data *keymap_data;
+ struct omap_keypad *keypad_data;
+
+ unsigned int status = 0, row_shift = 0, error = 0, length = 0,
id
= 0;
+ unsigned short *keypad_codes;
+
+ int hw_mod_name_len = 16;
+ char oh_name[hw_mod_name_len];
+ char *name = omap4-keypad;
+
+ length = snprintf(oh_name, hw_mod_name_len, keyboard);
+ WARN(length = hw_mod_name_len,
+ String buffer overflow in keyboard device setup\n);
+
+ oh = omap_hwmod_lookup(oh_name);
+ if (!oh)
+ pr_err(Could not look up %s\n, oh_name);
+
+ pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+ GFP_KERNEL);
Why is it allocated?
Moved to board file...
+
+ od = omap_device_build(name, id, oh, pdata,
+ sizeof(struct
matrix_keypad_platform_data),
+ omap_keyboard_latency,
+ ARRAY_SIZE(omap_keyboard_latency), 1);
+ WARN(IS_ERR(od), Could not build omap_device for %s %s\n,
+ name, oh_name);
+
+ /* platform data */
+ pdata = pdev-dev.platform_data;
Umm, here you are overriding pdata pointer... don't you leak memory
doing this?
hwmod has been moved to board configuration so we need to get platform data
+ if (!pdata) {
+ dev_err(pdev-dev, no platform data defined\n);
+ kfree(pdata);
+ return -EINVAL;
+ }
+
+ /* keymap data */
+ keymap_data = pdata-keymap_data;
+ if (!keymap_data) {
+ dev_err(pdev-dev, no keymap data defined\n);
+ kfree(keymap_data);
+ return -EINVAL;
+ }
+
+ /* omap keypad data */
+ row_shift = get_count_order(pdata-num_col_gpios);
+ keypad_data = kzalloc(sizeof(struct omap_keypad), GFP_KERNEL);
+ if (!keypad_data) {
+ error = -ENOMEM;
+ goto failed_free_platform;
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
On Tue, May 11, 2010 at 12:03:44AM -0500, Arce, Abraham wrote:
Hi again Dmitry,
No worries, although at first I was surprised that Trilok spoke exactly
the same words I did ;)
:)
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
Why is iti threaded? I fo not see anything that will sleep.
It was implemented based on previous comments...
Would you point me to that comment? Like I said, I do not see anything
that would possibly sleep in this routine so you don't need to use
threaded interrupt.
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg26352.html
Thanks.
I think Kevin meant use theaded IRQ, wherever possible [if we need
to sleep in interrupt handler].
Adding him to CC.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Felipe,
Thanks for your comments...
[..]
+#include plat/omap_hwmod.h
+#include plat/omap_device.h
should the platform_driver know about hwmod and omap_device ? Paul ?
Kevin ?
Working on these changes...
+struct omap_keypad {
+
unnecessary blank line.
Removed
+ struct platform_device *pdev;
generaly we save the struct device *.
+ struct omap_hwmod *oh;
+ const struct matrix_keypad_platform_data *pdata;
you should not save the platform_data here. Generaly you should only use
that to initialize fields on your structure. Remember that a
platform_data will be declared as __initdata.
Removed
+/* Interrupt thread primary handler, called in hard interrupt context */
+
+static irqreturn_t omap_keypad_interrupt(int irq, void *dev_id)
+{
+ struct omap_keypad *keypad_data = dev_id;
+
+ /* disable interrupts */
+ __raw_writel(OMAP4_DEF_IRQDISABLE, keypad_data-base +
+ OMAP4_KBD_IRQENABLE);
+
+ /* wake up handler thread */
+ return IRQ_WAKE_THREAD;
+
+}
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
+ struct omap_keypad *keypad_data = dev_id;
+ struct input_dev *input = keypad_data-input;
+ unsigned char key_state[8];
+ int col, row, code;
+ u32 *new_state = (u32 *) key_state;
+
+ *new_state = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE31_0);
+ *(new_state + 1) = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE63_32);
+
+ for (col = 0; col keypad_data-cols; col++) {
+ for (row = 0; row keypad_data-rows; row++) {
+ code = MATRIX_SCAN_CODE(row, col,
+ keypad_data-row_shift);
+ if (code 0) {
+ printk(KERN_INFO Spurious key %d-%d\n,
+ col, row);
use dev_dbg()
Added
+ continue;
+ }
+ input_report_key(input, keypad_data-keycodes[code],
+ key_state[col] (1 row));
missing input_sync()
Added
+static int __devinit omap_keypad_probe(struct platform_device *pdev)
+{
[..]
+ length = snprintf(oh_name, hw_mod_name_len, keyboard);
+ WARN(length = hw_mod_name_len,
+ String buffer overflow in keyboard device setup\n);
you're using snprintf() this WARN() shouldn't happen even, remove it.
Code now in board file, removed
+
+ oh = omap_hwmod_lookup(oh_name);
+ if (!oh)
+ pr_err(Could not look up %s\n, oh_name);
I think omap_hwmod and omap_device shouldn't be here, but at least use
dev_err();
Moving to board file...
+
+ pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+ GFP_KERNEL);
this should come from platform code.
Working on these changes...
I'll have a function in 4430sdp board file that will setup hwmod and omap
device called
sdp4430_keypad_init() {
...
omap_hwmod_lookup(oh_name);
kzalloc(sizeof(struct matrix_keypad_platform_data), GFP_KERNEL);
omap_device_build(name, id, oh, pdata,
...
}
and then called within
omap_4430sdp_init()
+
+ od = omap_device_build(name, id, oh, pdata,
+ sizeof(struct matrix_keypad_platform_data),
+ omap_keyboard_latency,
+ ARRAY_SIZE(omap_keyboard_latency), 1);
+ WARN(IS_ERR(od), Could not build omap_device for %s %s\n,
+ name, oh_name);
this too.
Moving to board file...
+ status = input_register_device(keypad_data-input);
+ if (status 0) {
+ dev_err(pdev-dev, failed to register input device\n);
+ goto failed_free_device;
+ }
+
+ omap_keypad_config(keypad_data);
registering and configuring should be done before requesting the irq.
Changed
Best Regards
Abraham
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Thanks Kevin,
Keyboard controller for OMAP4 with built-in scanning algorithm.
The following implementations are used:
- matrix_keypac.c logic
- hwmod framework
- threaded irq
Signed-off-by: Syed Rafiuddin rafiuddin.s...@ti.com
Signed-off-by: Abraham Arce x0066...@ti.com
Some general comments...
What's missing here is a separation of the driver and the device.
What you need is for arch code to register a platfrom_device (using
hwmod + omap_device). Then this driver will use the standard
platform_device resource calls to get its base address, IRQs, etc. and
any platform_data.
IOW, as Felipe mentioned, the driver should not be doing the hwmod +
omap_device init and registration.
As metioned to Felipe, my approach is
sdp4430_keypad_init() {
...
omap_hwmod_lookup(oh_name);
...
kzalloc(sizeof(struct matrix_keypad_platform_data), GFP_KERNEL);
...
omap_device_build(name, id, oh, pdata,
...
}
and then called within
omap_4430sdp_init()
Best Regards
Abraham
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Hi Abraham, On Tue, Apr 13, 2010 at 08:10:48PM -0500, Arce, Abraham wrote: Keyboard controller for OMAP4 with built-in scanning algorithm. The following implementations are used: - matrix_keypac.c logic - hwmod framework - threaded irq Signed-off-by: Syed Rafiuddin rafiuddin.s...@ti.com Signed-off-by: Abraham Arce x0066...@ti.com --- drivers/input/keyboard/Kconfig|9 + drivers/input/keyboard/Makefile |1 + drivers/input/keyboard/omap4-keypad.c | 367 + 3 files changed, 377 insertions(+), 0 deletions(-) create mode 100644 drivers/input/keyboard/omap4-keypad.c diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig index 5049c3c..8a4103e 100644 --- a/drivers/input/keyboard/Kconfig +++ b/drivers/input/keyboard/Kconfig @@ -390,6 +390,15 @@ 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 +depends on (ARCH_OMAP4) +help + 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. + config KEYBOARD_TWL4030 tristate TI TWL4030/TWL5030/TPS659x0 keypad support depends on TWL4030_CORE diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile index 78654ef..1b3dfbe 100644 --- a/drivers/input/keyboard/Makefile +++ b/drivers/input/keyboard/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_KEYBOARD_MATRIX) += matrix_keypad.o obj-$(CONFIG_KEYBOARD_MAX7359) += max7359_keypad.o obj-$(CONFIG_KEYBOARD_NEWTON)+= newtonkbd.o obj-$(CONFIG_KEYBOARD_OMAP) += omap-keypad.o +obj-$(CONFIG_KEYBOARD_OMAP4) += omap4-keypad.o obj-$(CONFIG_KEYBOARD_OPENCORES) += opencores-kbd.o obj-$(CONFIG_KEYBOARD_PXA27x)+= pxa27x_keypad.o obj-$(CONFIG_KEYBOARD_PXA930_ROTARY) += pxa930_rotary.o diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c new file mode 100644 index 000..b656b4f --- /dev/null +++ b/drivers/input/keyboard/omap4-keypad.c @@ -0,0 +1,367 @@ +/* + * linux/drivers/input/keyboard/omap4-keypad.c + * + * OMAP4 Keypad Driver + * + * Copyright (C) 2010 Texas Instruments + * + * Author: Abraham Arce x0066...@ti.com + * Initial Code: Syed Rafiuddin rafiuddin.s...@ti.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#include linux/module.h +#include linux/init.h +#include linux/interrupt.h +#include linux/platform_device.h +#include linux/errno.h +#include linux/io.h +#include linux/input.h +#include linux/input/matrix_keypad.h +#include plat/omap_hwmod.h +#include plat/omap_device.h + +/* OMAP4 registers */ +#define OMAP4_KBD_REVISION 0x00 +#define OMAP4_KBD_SYSCONFIG 0x10 +#define OMAP4_KBD_SYSSTATUS 0x14 +#define OMAP4_KBD_IRQSTATUS 0x18 +#define OMAP4_KBD_IRQENABLE 0x1C +#define OMAP4_KBD_WAKEUPENABLE 0x20 +#define OMAP4_KBD_PENDING0x24 +#define OMAP4_KBD_CTRL 0x28 +#define OMAP4_KBD_DEBOUNCINGTIME 0x2C +#define OMAP4_KBD_LONGKEYTIME0x30 +#define OMAP4_KBD_TIMEOUT0x34 +#define OMAP4_KBD_STATEMACHINE 0x38 +#define OMAP4_KBD_ROWINPUTS 0x3C +#define OMAP4_KBD_COLUMNOUTPUTS 0x40 +#define OMAP4_KBD_FULLCODE31_0 0x44 +#define OMAP4_KBD_FULLCODE63_32 0x48 + +/* OMAP4 bit definitions */ +#define OMAP4_DEF_SYSCONFIG_SOFTRST (1 1) +#define OMAP4_DEF_SYSCONFIG_ENAWKUP (1 2) +#define OMAP4_DEF_IRQENABLE_EVENTEN (1 0) +#define OMAP4_DEF_IRQENABLE_LONGKEY (1 1) +#define OMAP4_DEF_IRQENABLE_TIMEOUTEN(1 2) +#define OMAP4_DEF_CTRL_NOSOFTMODE(1 1) +#define OMAP4_DEF_CTRLPTVVALUE (1 2) +#define OMAP4_DEF_CTRLPTV(1 1) +#define OMAP4_DEF_IRQDISABLE 0x00 + +/* OMAP4 values */ +#define OMAP4_VAL_DEBOUNCINGTIME 0x07 +#define OMAP4_VAL_FUNCTIONALCFG 0x1E + +#define OMAP4_MASK_IRQSTATUSDISABLE
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Arce, Abraham x0066...@ti.com writes: Keyboard controller for OMAP4 with built-in scanning algorithm. The following implementations are used: - matrix_keypac.c logic - hwmod framework - threaded irq Signed-off-by: Syed Rafiuddin rafiuddin.s...@ti.com Signed-off-by: Abraham Arce x0066...@ti.com Some general comments... What's missing here is a separation of the driver and the device. What you need is for arch code to register a platfrom_device (using hwmod + omap_device). Then this driver will use the standard platform_device resource calls to get its base address, IRQs, etc. and any platform_data. IOW, as Felipe mentioned, the driver should not be doing the hwmod + omap_device init and registration. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Hi Trilok!
- matrix_keypac.c logic
- hwmod framework
Do we have hwmod framework mainlined in the kernel?
Not yet but wanted to gather initial comments to be ready once framework is
pushed
+config KEYBOARD_OMAP4
+ tristate TI OMAP4 keypad support
+ depends on (ARCH_OMAP4)
No need of brackets.
Removed
Could you please provide default y/n option too?
Added
+ * linux/drivers/input/keyboard/omap4-keypad.c
Better not to include file paths.
Removed path... only filename
+
+struct omap_device_pm_latency omap_keyboard_latency[] = {
static?
Not sure... I'll check
snip
+ input_report_key(input, keypad_data-keycodes[code],
+ key_state[col] (1 row));
+ }
+ }
where is input_sync(..)?
Added
+
+ pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+ GFP_KERNEL);
Please check error return here, because kzalloc can fail.
Added
snip
+
+ /* keymap data */
+ keymap_data = pdata-keymap_data;
+ if (!keymap_data) {
+ dev_err(pdev-dev, no keymap data defined\n);
+ kfree(keymap_data);
not freeing kfree(pdata)? Please check error return path again in this driver.
I'll check error return in all code
snip
+
+failed_free_device:
+ input_unregister_device(keypad_data-input);
add keypad_data-input = NULL.
Added
Best Regards
Abraham
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Hi,
On Wed, Apr 14, 2010 at 03:10:48AM +0200, ext Arce, Abraham wrote:
+#include linux/module.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/platform_device.h
+#include linux/errno.h
+#include linux/io.h
+#include linux/input.h
+#include linux/input/matrix_keypad.h
+#include plat/omap_hwmod.h
+#include plat/omap_device.h
should the platform_driver know about hwmod and omap_device ? Paul ?
Kevin ?
+struct omap_keypad {
+
unnecessary blank line.
+ struct platform_device *pdev;
generaly we save the struct device *.
+ struct omap_hwmod *oh;
+ const struct matrix_keypad_platform_data *pdata;
you should not save the platform_data here. Generaly you should only use
that to initialize fields on your structure. Remember that a
platform_data will be declared as __initdata.
+/* Interrupt thread primary handler, called in hard interrupt context */
+
+static irqreturn_t omap_keypad_interrupt(int irq, void *dev_id)
+{
+ struct omap_keypad *keypad_data = dev_id;
+
+ /* disable interrupts */
+ __raw_writel(OMAP4_DEF_IRQDISABLE, keypad_data-base +
+ OMAP4_KBD_IRQENABLE);
+
+ /* wake up handler thread */
+ return IRQ_WAKE_THREAD;
+
+}
+
+/* Interrupt thread handler thread */
+
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
+ struct omap_keypad *keypad_data = dev_id;
+ struct input_dev *input = keypad_data-input;
+ unsigned char key_state[8];
+ int col, row, code;
+ u32 *new_state = (u32 *) key_state;
+
+ *new_state = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE31_0);
+ *(new_state + 1) = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE63_32);
+
+ for (col = 0; col keypad_data-cols; col++) {
+ for (row = 0; row keypad_data-rows; row++) {
+ code = MATRIX_SCAN_CODE(row, col,
+ keypad_data-row_shift);
+ if (code 0) {
+ printk(KERN_INFO Spurious key %d-%d\n,
+ col, row);
use dev_dbg()
+ continue;
+ }
+ input_report_key(input, keypad_data-keycodes[code],
+ key_state[col] (1 row));
missing input_sync()
+static int __devinit omap_keypad_probe(struct platform_device *pdev)
+{
[..]
+ length = snprintf(oh_name, hw_mod_name_len, keyboard);
+ WARN(length = hw_mod_name_len,
+ String buffer overflow in keyboard device setup\n);
you're using snprintf() this WARN() shouldn't happen even, remove it.
+
+ oh = omap_hwmod_lookup(oh_name);
+ if (!oh)
+ pr_err(Could not look up %s\n, oh_name);
I think omap_hwmod and omap_device shouldn't be here, but at least use
dev_err();
+
+ pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+ GFP_KERNEL);
this should come from platform code.
+
+ od = omap_device_build(name, id, oh, pdata,
+ sizeof(struct matrix_keypad_platform_data),
+ omap_keyboard_latency,
+ ARRAY_SIZE(omap_keyboard_latency), 1);
+ WARN(IS_ERR(od), Could not build omap_device for %s %s\n,
+ name, oh_name);
this too.
+ status = input_register_device(keypad_data-input);
+ if (status 0) {
+ dev_err(pdev-dev, failed to register input device\n);
+ goto failed_free_device;
+ }
+
+ omap_keypad_config(keypad_data);
registering and configuring should be done before requesting the irq.
--
balbi
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support
Hi Abraham,
On Wed, Apr 14, 2010 at 6:40 AM, Arce, Abraham x0066...@ti.com wrote:
Keyboard controller for OMAP4 with built-in scanning algorithm.
The following implementations are used:
- matrix_keypac.c logic
- hwmod framework
Do we have hwmod framework mainlined in the kernel?
+config KEYBOARD_OMAP4
+ tristate TI OMAP4 keypad support
+ depends on (ARCH_OMAP4)
No need of brackets.
Could you please provide default y/n option too?
+ * linux/drivers/input/keyboard/omap4-keypad.c
Better not to include file paths.
+
+struct omap_device_pm_latency omap_keyboard_latency[] = {
static?
+ {
+ .deactivate_func = omap_device_idle_hwmods,
+ .activate_func = omap_device_enable_hwmods,
+ .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
+ },
+};
+static irqreturn_t omap_keypad_threaded(int irq, void *dev_id)
+{
+ struct omap_keypad *keypad_data = dev_id;
+ struct input_dev *input = keypad_data-input;
+ unsigned char key_state[8];
+ int col, row, code;
+ u32 *new_state = (u32 *) key_state;
+
+ *new_state = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE31_0);
+ *(new_state + 1) = __raw_readl(keypad_data-base +
+ OMAP4_KBD_FULLCODE63_32);
+
+ for (col = 0; col keypad_data-cols; col++) {
+ for (row = 0; row keypad_data-rows; row++) {
+ code = MATRIX_SCAN_CODE(row, col,
+ keypad_data-row_shift);
+ if (code 0) {
+ printk(KERN_INFO Spurious key %d-%d\n,
+ col, row);
+ continue;
+ }
+ input_report_key(input, keypad_data-keycodes[code],
+ key_state[col] (1 row));
+ }
+ }
where is input_sync(..)?
+
+ pdata = kzalloc(sizeof(struct matrix_keypad_platform_data),
+ GFP_KERNEL);
Please check error return here, because kzalloc can fail.
+
+ od = omap_device_build(name, id, oh, pdata,
+ sizeof(struct matrix_keypad_platform_data),
+ omap_keyboard_latency,
+ ARRAY_SIZE(omap_keyboard_latency), 1);
+ WARN(IS_ERR(od), Could not build omap_device for %s %s\n,
+ name, oh_name);
+
+ /* platform data */
+ pdata = pdev-dev.platform_data;
+ if (!pdata) {
+ dev_err(pdev-dev, no platform data defined\n);
+ kfree(pdata);
+ return -EINVAL;
+ }
+
+ /* keymap data */
+ keymap_data = pdata-keymap_data;
+ if (!keymap_data) {
+ dev_err(pdev-dev, no keymap data defined\n);
+ kfree(keymap_data);
not freeing kfree(pdata)? Please check error return path again in this driver.
+
+ __set_bit(EV_REP, input_dev-evbit);
+ __set_bit(KEY_OK, input_dev-keybit);
+ __set_bit(EV_KEY, input_dev-evbit);
+
+ input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+ input_set_drvdata(input_dev, keypad_data);
+
+ status = input_register_device(keypad_data-input);
+ if (status 0) {
+ dev_err(pdev-dev, failed to register input device\n);
+ goto failed_free_device;
+ }
+
+ omap_keypad_config(keypad_data);
+
+ platform_set_drvdata(pdev, keypad_data);
+
+ return 0;
+
+failed_free_device:
+ input_unregister_device(keypad_data-input);
add keypad_data-input = NULL.
--
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
