RE: [PATCH v3 1/2] Input: Add DaVinci DM365 Keypad support

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

 



On Fri, Sep 18, 2009 at 01:36:36, miguel.aguilar@xxxxxxxxxxxx wrote:
> From: Miguel Aguilar <miguel.aguilar@xxxxxxxxxxxx>
>
> Adds the driver for enabling keypad support on DM365 platform.
>
> This driver was tested on DM365 EVM rev c.
>
> Signed-off-by: Miguel Aguilar <miguel.aguilar@xxxxxxxxxxxx>
> ---

[...]

> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index a6b989a..08ed7d4 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -361,4 +361,10 @@ config KEYBOARD_XTKBD
>         To compile this driver as a module, choose M here: the
>         module will be called xtkbd.
>
> +config KEYBOARD_DAVINCI_DM365

How about calling it just KEYBOARD_DAVINCI in honor of this
being the first DaVinci keypad driver in drivers/input/keyboard?
Also, in the hope that this will get reused on a future DaVinci.

Unless, there is something that makes it really DM365 specific..

> +     tristate "TI DaVinci DM365 Keypad"
> +     depends on ARCH_DAVINCI_DM365
> +     help
> +      Supports the keypad module on the DM365
> +
>  endif

[...]

> diff --git a/drivers/input/keyboard/davinci_dm365_keypad.c b/drivers/input/keyboard/davinci_dm365_keypad.c
> new file mode 100644
> index 0000000..5db8eed
> --- /dev/null
> +++ b/drivers/input/keyboard/davinci_dm365_keypad.c
> @@ -0,0 +1,331 @@
> +/*
> + * DaVinci DM365 Keypad Driver
> + *
> + * Copyright (C) 2009 Texas Instruments, Inc
> + *
> + * Author: Miguel Aguilar <miguel.aguilar@xxxxxxxxxxxx>
> + *
> + * Intial Code: Sandeep Paulraj <s-paulraj@xxxxxx>
> + *
> + * 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/types.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/errno.h>
> +
> +#include <asm/irq.h>
> +
> +#include <mach/hardware.h>
> +#include <mach/irqs.h>
> +#include <mach/keypad.h>
> +
> +/* Keypad registers */
> +#define DM365_KEYPAD_KEYCTRL         0x0000

There are many places in the driver (functions, macros) which
probably can just be called DAVINCI_ instead of DM365_

> +#define DM365_KEYPAD_INTENA          0x0004
> +#define DM365_KEYPAD_INTFLAG         0x0008
> +#define DM365_KEYPAD_INTCLR          0x000c
> +#define DM365_KEYPAD_STRBWIDTH               0x0010
> +#define DM365_KEYPAD_INTERVAL                0x0014
> +#define DM365_KEYPAD_CONTTIME                0x0018
> +#define DM365_KEYPAD_CURRENTST               0x001c
> +#define DM365_KEYPAD_PREVSTATE               0x0020
> +#define DM365_KEYPAD_EMUCTRL         0x0024
> +#define DM365_KEYPAD_IODFTCTRL               0x002c
> +
> +/* Key Control Register (KEYCTRL) */
> +#define DM365_KEYPAD_KEYEN           0x00000001
> +#define DM365_KEYPAD_PREVMODE                0x00000002
> +#define DM365_KEYPAD_CHATOFF         0x00000004
> +#define DM365_KEYPAD_AUTODET         0x00000008
> +#define DM365_KEYPAD_SCANMODE                0x00000010
> +#define DM365_KEYPAD_OUTTYPE         0x00000020
> +#define DM365_KEYPAD_4X4             0x00000040
> +
> +/* Masks for the interrupts */
> +#define DM365_KEYPAD_INT_CONT                0x00000008
> +#define DM365_KEYPAD_INT_OFF         0x00000004
> +#define DM365_KEYPAD_INT_ON          0x00000002
> +#define DM365_KEYPAD_INT_CHANGE              0x00000001
> +#define DM365_KEYPAD_INT_ALL         0x0000000f
> +
> +struct davinci_kp {
> +     struct input_dev                *input;
> +     struct davinci_kp_platform_data *pdata;
> +     int                             irq;
> +     void __iomem                    *base;
> +     resource_size_t                 pbase;
> +     size_t                          base_size;
> +};
> +
> +static void dm365_kp_write(struct davinci_kp *dm365_kp, u32 val, u32 addr)
> +{
> +     u32 base = (u32)dm365_kp->base;
> +
> +     __raw_writel(val,(u32 *)(base + addr));
> +}
> +
> +static u32 dm365_kp_read(struct davinci_kp *dm365_kp, u32 addr)
> +{
> +     u32 base = (u32)dm365_kp->base;
> +
> +     return __raw_readl((u32 *)(base + addr));
> +}
> +
> +/* Initializing the kp Module */
> +static void dm365_kp_initialize(struct davinci_kp *dm365_kp)
> +{
> +     u32 strobe = dm365_kp->pdata->strobe;
> +     u32 interval = dm365_kp->pdata->interval;
> +
> +     /* Enable all interrupts */
> +     dm365_kp_write(dm365_kp, DM365_KEYPAD_INT_ALL, DM365_KEYPAD_INTENA);
> +
> +     /* Clear interrupts if any */
> +     dm365_kp_write(dm365_kp, DM365_KEYPAD_INT_ALL, DM365_KEYPAD_INTCLR);
> +
> +     /* Setup the scan period = strobe + interval */
> +     dm365_kp_write(dm365_kp, strobe, DM365_KEYPAD_STRBWIDTH);
> +     dm365_kp_write(dm365_kp, interval, DM365_KEYPAD_INTERVAL);
> +     dm365_kp_write(dm365_kp, 0x01, DM365_KEYPAD_CONTTIME);
> +
> +     /* Enable Keyscan module and enable */
> +     dm365_kp_write(dm365_kp, DM365_KEYPAD_AUTODET | DM365_KEYPAD_KEYEN,
> +                     DM365_KEYPAD_KEYCTRL);
> +}
> +
> +static irqreturn_t dm365_kp_interrupt(int irq, void *dev_id)
> +{
> +     int i;
> +     u32 prev_status, new_status, changed;
> +     int keycode = KEY_UNKNOWN;
> +     struct davinci_kp *dm365_kp = dev_id;
> +     int *keymap = dm365_kp->pdata->keymap;
> +     u32 keymapsize = dm365_kp->pdata->keymapsize;
> +     struct device *dev = &dm365_kp->input->dev;
> +
> +     /* Disable interrupt */
> +     dm365_kp_write(dm365_kp, 0x0, DM365_KEYPAD_INTENA);
> +
> +     /* Reading previous and new status of the keypad */
> +     prev_status = dm365_kp_read(dm365_kp, DM365_KEYPAD_PREVSTATE);
> +     new_status = dm365_kp_read(dm365_kp, DM365_KEYPAD_CURRENTST);
> +
> +     changed = prev_status ^ new_status;
> +
> +     for (i = 0; i < keymapsize; i++) {
> +             if ((changed >> i) & 0x1) {

Using ffs(changed) will probably lead to a faster ISR, especially
because only one key would have changed in most cases.

> +                     keycode = keymap[i];
> +                     if((new_status >> i) & 0x1) {
> +                             /* Report release */
> +                             input_report_key(dm365_kp->input, keycode, 0);
> +                             input_sync(dm365_kp->input);
> +                             dev_dbg(dev, "dm365_keypad: key %d released\n",
> +                                             keycode);
> +                     } else {
> +                             /* Report press */
> +                             input_report_key(dm365_kp->input, keycode, 1);
> +                             input_sync(dm365_kp->input);
> +                             dev_dbg(dev, "dm365_keypad: key %d pressed\n",
> +                                             keycode);
> +                     }
> +             }
> +     }
> +
> +     /* Clearing interrupt */
> +     dm365_kp_write(dm365_kp, DM365_KEYPAD_INT_ALL, DM365_KEYPAD_INTCLR);
> +
> +     /* Enable interrupts */
> +     dm365_kp_write(dm365_kp, 0x1, DM365_KEYPAD_INTENA);
> +
> +     return IRQ_HANDLED;
> +}
> +
> +/*
> + * Registers keypad device with input sub system and configures
> + * DM365 keypad registers
> + */
> +static int dm365_kp_probe(struct platform_device *pdev)

This should be __init function.

> +{
> +     struct davinci_kp *dm365_kp;
> +     struct input_dev *key_dev;
> +     struct resource *res, *mem;
> +     int ret, i;
> +     struct device * dev = &pdev->dev;
> +     struct davinci_kp_platform_data *pdata = pdev->dev.platform_data;
> +
> +     dev_info(dev, "DaVinci DM365 Keypad Driver\n");
> +
> +     if (!pdata->keymap) {
> +             dev_dbg(dev, "%s: No keymap from pdata\n", pdev->name);
> +             return -EINVAL;
> +     }
> +
> +     dm365_kp = kzalloc(sizeof *dm365_kp, GFP_KERNEL);
> +     if(!dm365_kp) {
> +             dev_dbg(dev, "%s: Could not allocate memory for private data\n",
> +                     pdev->name);
> +             return -ENOMEM;
> +     }
> +
> +     key_dev = input_allocate_device();
> +     if (!key_dev) {
> +             dev_dbg(dev, "%s: Could not allocate input device\n",
> +                     pdev->name);
> +             ret = -ENOMEM;
> +             goto fail1;
> +     }
> +
> +     platform_set_drvdata(pdev, dm365_kp);
> +
> +     dm365_kp->input = key_dev;
> +
> +     dm365_kp->irq = platform_get_irq(pdev, 0);
> +     if (dm365_kp->irq <= 0) {
> +             dev_err(dev, "%s: No DM365 Keypad irq\n", pdev->name);
> +             ret = dm365_kp->irq;

Probe will return success when dm365_kp->irq is 0, but you actually
want a failure.

> +             goto fail2;
> +     }
> +

[...]

> +     key_dev->name = "dm365_keypad";
> +     key_dev->phys = "dm365_keypad/input0";
...
> +     key_dev->id.vendor = 0x0001;
> +     key_dev->id.product = 0x0365;

These initializations seem to make the driver's use on future SoCs
a little uncomfortable..

> +     key_dev->id.version = 0x0001;
> +     key_dev->keycode = dm365_kp->pdata->keymap;
> +     key_dev->keycodesize = sizeof(unsigned int);
> +     key_dev->keycodemax = dm365_kp->pdata->keymapsize;
> +
> +     ret = input_register_device(dm365_kp->input);
> +     if (ret < 0) {
> +             dev_err(dev, "%s: Unable to register DaVinci DM365 keypad device\n",

Can avoid hard coding "DM365" in error messages too...

> +                     pdev->name);
> +             goto fail4;
> +     }
> +
> +     ret = request_irq(dm365_kp->irq, dm365_kp_interrupt, IRQF_DISABLED,
> +                         "dm365_keypad", dm365_kp);
> +     if (ret < 0) {
> +             dev_err(dev, "%s: Unable to register DaVinci DM365 keypad Interrupt\n",
> +                     pdev->name);
> +             goto fail5;
> +     }
> +
> +     dm365_kp_initialize(dm365_kp);
> +
> +     return 0;
> +fail5:
> +     input_unregister_device(dm365_kp->input);
> +     key_dev = NULL;
> +fail4:
> +     iounmap(dm365_kp->base);
> +fail3:
> +     release_mem_region(dm365_kp->pbase, dm365_kp->base_size);
> +fail2:
> +     input_free_device(key_dev);
> +fail1:
> +     kfree(dm365_kp);
> +
> +     return ret;
> +}
> +
> +static int __exit dm365_kp_remove(struct platform_device *pdev)
> +{
> +     struct davinci_kp *dm365_kp = platform_get_drvdata(pdev);
> +
> +     free_irq(dm365_kp->irq, dm365_kp);
> +
> +     iounmap(dm365_kp->base);
> +     release_mem_region(dm365_kp->pbase, dm365_kp->base_size);
> +
> +     platform_set_drvdata(pdev, NULL);
> +
> +     input_unregister_device(dm365_kp->input);

It would be nice to see the remove happen in reverse order of probe,
ie. unregister device first and unmap memory later.

> +
> +     kfree(dm365_kp);
> +
> +     return 0;
> +}
> +
> +static struct platform_driver dm365_kp_driver = {
> +     .driver = {
> +                     .name = "dm365_keypad",
> +                     .owner = THIS_MODULE,
> +             },
> +     .remove = __exit_p(dm365_kp_remove),
> +};
> +
> +static int __init dm365_kp_init(void)
> +{
> +
> +

Extra lines here.

> +     return platform_driver_probe(&dm365_kp_driver, dm365_kp_probe);
> +}
> +module_init(dm365_kp_init);
> +
> +static void __exit dm365_kp_exit(void)
> +{
> +     platform_driver_unregister(&dm365_kp_driver);
> +}
> +module_exit(dm365_kp_exit);
> +
> +MODULE_AUTHOR("Miguel Aguilar");
> +MODULE_DESCRIPTION("Texas Instruments DaVinci DM365 EVM Keypad Driver");

The driver is definitely not specific to the EVM.

Thanks,
Sekhar
--
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