Re: [RFC] [PATCH 1/3] OMAP4: Keyboard Controller Support

Felipe Balbi
2010-04-16T05:06:48Z

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