Re: [PATCH] input: add support for generic GPIO-based matrix keypad

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Eric,

On Thu, May 7, 2009 at 1:30 PM, Eric Miao <eric.y.miao@xxxxxxxxx> wrote:
> Original patch by Marek Vasut, modified by Eric in:
>
> 1. use delayed work to simplify the debouncing
> 2. build keycode array for fast lookup
> 3. combine col_polarity/row_polarity into a single active_low field
> (are there some cases where the GPIOs are externally connected
> with an inverter and thus causing two different polarity requirement??)
> 4. use a generic bit array based XOR algorithm to detect key press/release,
> which should make the column assertion time shorter and code a bit
> cleaner
> 5. remove the ALT_FN handling, which is no way generic, the ALT_FN
> key should be treated as no different from other keys, and translation
> will be done by user space by commands like 'loadkeys'.
>
> Patch tested on Littleton/PXA310 (though PXA310 has a dedicate keypad
> controller, I have to configure those pins as generic GPIO to use this
> driver, works quite well, though ;-)

Any support about removing/clearing ghost/phantom keys? Also we assume
that all gpios in the matrix should be able to generate interrupts so
no polldev support required.


> diff --git a/drivers/input/keyboard/matrix_keypad.c
> b/drivers/input/keyboard/matrix_keypad.c
> new file mode 100644
> index 0000000..da9a3fc
> --- /dev/null
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -0,0 +1,345 @@
> +/*
> + * drivers/input/keyboard/matrix_keypad.c
> + *

Better to remove full path and just write matrix_keypad.c

> + *  GPIO driven matrix keyboard driver
> + *
> + *  Copyright (c) 2008 Marek Vasut <marek.vasut@xxxxxxxxx>

You may want to add your Copyright here.

> + *
> + *  Based on corgikbd.c
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/gpio.h>
> +#include <linux/input/matrix_keypad.h>
> +


> +
> +static void build_keycodes(struct matrix_keypad *keypad)
> +{
> +       struct matrix_keypad_platform_data *pdata = keypad->pdata;
> +       struct input_dev *input_dev = keypad->input_dev;
> +       uint32_t *key;
> +       int i;
> +
> +       keypad->keycodes = kzalloc(MATRIX_MAX_KEYS * sizeof(int), GFP_KERNEL);
> +
> +       key = &pdata->key_map[0];
> +       for (i = 0; i < pdata->key_map_size; i++, key++) {
> +               keypad->keycodes[KEY_ROWCOL(*key)] = KEY_VAL(*key);
> +               set_bit(KEY_VAL(*key), input_dev->keybit);

__set_bit


> +
> +
> +#ifdef CONFIG_PM
> +static int matrix_keypad_suspend(struct platform_device *pdev,
> pm_message_t state)
> +{
> +       struct matrix_keypad *keypad = platform_get_drvdata(pdev);
> +       struct matrix_keypad_platform_data *pdata = keypad->pdata;
> +       int i;
> +
> +       if (device_may_wakeup(&pdev->dev))
> +               for (i = 0; i < pdata->num_row_gpios; i++)
> +                       enable_irq_wake(gpio_to_irq(pdata->row_gpios[i]));
> +
> +       return 0;
> +}
> +
> +static int matrix_keypad_resume(struct platform_device *pdev)
> +{
> +       struct matrix_keypad *keypad = platform_get_drvdata(pdev);
> +       struct matrix_keypad_platform_data *pdata = keypad->pdata;
> +       int i;
> +
> +       if (device_may_wakeup(&pdev->dev))

I think I have not seen device_init_wakeup anywhere in this code.

> +               for (i = 0; i < pdata->num_row_gpios; i++)
> +                       disable_irq_wake(gpio_to_irq(pdata->row_gpios[i]));
> +
> +       return 0;
> +}
> +#else
> +#define matrix_keypad_suspend  NULL
> +#define matrix_keypad_resume   NULL
> +#endif
> +
> +
> +static int __devinit matrix_keypad_probe(struct platform_device *pdev)
> +{


> +
> +err_unregister:
> +       input_unregister_device(input_dev);
> +err_free_input:
> +       input_free_device(input_dev);

No need of input_free_device after input_unregister_device(..). You
can just do input_dev = NULL after input_unregister_device instead?


> +
> +static struct platform_driver matrix_keypad_driver = {
> +       .probe          = matrix_keypad_probe,
> +       .remove         = matrix_keypad_remove,

__exit_p ?




-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux