diff mbox

[1/2] Input: DaVinci Keypad Driver

Message ID 1253654850-11983-1-git-send-email-miguel.aguilar@ridgerun.com
State New, archived
Headers show

Commit Message

miguel.aguilar@ridgerun.com Sept. 22, 2009, 9:27 p.m. UTC
From: Miguel Aguilar <miguel.aguilar@ridgerun.com>

Adds the driver for enabling keypad support for DaVinci platforms.

DM365 is the only platform that uses this driver at the moment.

This driver was tested on DM365 EVM rev C.

Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
---
 arch/arm/configs/davinci_all_defconfig      |    1 +
 arch/arm/mach-davinci/include/mach/keypad.h |   35 +++
 drivers/input/keyboard/Kconfig              |    7 +
 drivers/input/keyboard/Makefile             |    1 +
 drivers/input/keyboard/davinci_keypad.c     |  319 +++++++++++++++++++++++++++
 5 files changed, 363 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h
 create mode 100644 drivers/input/keyboard/davinci_keypad.c

Comments

Dmitry Torokhov Sept. 23, 2009, 3:46 a.m. UTC | #1
Hi Miguel,

On Tue, Sep 22, 2009 at 03:27:30PM -0600, miguel.aguilar@ridgerun.com wrote:
> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> 
> Adds the driver for enabling keypad support for DaVinci platforms.
> 
> DM365 is the only platform that uses this driver at the moment.
> 
> This driver was tested on DM365 EVM rev C.
> 
> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> ---
>  arch/arm/configs/davinci_all_defconfig      |    1 +
>  arch/arm/mach-davinci/include/mach/keypad.h |   35 +++
>  drivers/input/keyboard/Kconfig              |    7 +
>  drivers/input/keyboard/Makefile             |    1 +
>  drivers/input/keyboard/davinci_keypad.c     |  319 +++++++++++++++++++++++++++
>  5 files changed, 363 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h
>  create mode 100644 drivers/input/keyboard/davinci_keypad.c
> 
> diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
> index ec63c15..e994c83 100644
> --- a/arch/arm/configs/davinci_all_defconfig
> +++ b/arch/arm/configs/davinci_all_defconfig
> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y
>  # CONFIG_KEYBOARD_STOWAWAY is not set
>  # CONFIG_KEYBOARD_SUNKBD is not set
>  CONFIG_KEYBOARD_XTKBD=m
> +CONFIG_KEYBOARD_DAVINCI_DM365=m
>  # CONFIG_INPUT_MOUSE is not set
>  # CONFIG_INPUT_JOYSTICK is not set
>  # CONFIG_INPUT_TABLET is not set


If you want to merge the driver through input tree the defconfig chunk
has to go elsewhere.

> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h
> new file mode 100644
> index 0000000..922d20e
> --- /dev/null
> +++ b/arch/arm/mach-davinci/include/mach/keypad.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2009 Texas Instruments, Inc
> + *
> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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
> + */
> +
> +#ifndef DAVINCI_KEYPAD_H
> +#define DAVINCI_KEYPAD_H
> +
> +#include <linux/io.h>
> +
> +struct davinci_kp_platform_data {
> +	int	*keymap;
> +	u32	keymapsize;
> +	u32	rep:1;
> +	u32	strobe;
> +	u32	interval;
> +};
> +
> +#endif
> +
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index a6b989a..b6b9517 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called xtkbd.
>  
> +config KEYBOARD_DAVINCI
> +	tristate "TI DaVinci Keypad"
> +	depends on ARCH_DAVINCI_DM365
> +	help
> +	  Say Y to enable keypad module support for the TI DaVinci
> +	  platforms (DM365)
> +

"To compile this driver as a module..."

>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index b5b5eae..0b0274e 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
>  obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
>  obj-$(CONFIG_KEYBOARD_TOSA)		+= tosakbd.o
>  obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
> +obj-$(CONFIG_KEYBOARD_DAVINCI)		+= davinci_keypad.o
> diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c
> new file mode 100644
> index 0000000..6f0e793
> --- /dev/null
> +++ b/drivers/input/keyboard/davinci_keypad.c
> @@ -0,0 +1,319 @@
> +/*
> + * DaVinci Keypad Driver for TI platforms
> + *
> + * Copyright (C) 2009 Texas Instruments, Inc
> + *
> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com>
> + *
> + * Intial Code: Sandeep Paulraj <s-paulraj@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/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 DAVINCI_KEYPAD_KEYCTRL		0x0000
> +#define DAVINCI_KEYPAD_INTENA		0x0004
> +#define DAVINCI_KEYPAD_INTFLAG		0x0008
> +#define DAVINCI_KEYPAD_INTCLR		0x000c
> +#define DAVINCI_KEYPAD_STRBWIDTH	0x0010
> +#define DAVINCI_KEYPAD_INTERVAL		0x0014
> +#define DAVINCI_KEYPAD_CONTTIME		0x0018
> +#define DAVINCI_KEYPAD_CURRENTST	0x001c
> +#define DAVINCI_KEYPAD_PREVSTATE	0x0020
> +#define DAVINCI_KEYPAD_EMUCTRL		0x0024
> +#define DAVINCI_KEYPAD_IODFTCTRL	0x002c
> +
> +/* Key Control Register (KEYCTRL) */
> +#define DAVINCI_KEYPAD_KEYEN		0x00000001
> +#define DAVINCI_KEYPAD_PREVMODE		0x00000002
> +#define DAVINCI_KEYPAD_CHATOFF		0x00000004
> +#define DAVINCI_KEYPAD_AUTODET		0x00000008
> +#define DAVINCI_KEYPAD_SCANMODE		0x00000010
> +#define DAVINCI_KEYPAD_OUTTYPE		0x00000020
> +#define DAVINCI_KEYPAD_4X4		0x00000040
> +
> +/* Masks for the interrupts */
> +#define DAVINCI_KEYPAD_INT_CONT		0x00000008
> +#define DAVINCI_KEYPAD_INT_OFF		0x00000004
> +#define DAVINCI_KEYPAD_INT_ON		0x00000002
> +#define DAVINCI_KEYPAD_INT_CHANGE	0x00000001
> +#define DAVINCI_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 davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr)
> +{
> +	u32 base = (u32)davinci_kp->base;
> +
> +	__raw_writel(val,(u32 *)(base + addr));
> +}
> +
> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr)
> +{
> +	u32 base = (u32)davinci_kp->base;
> +
> +	return __raw_readl((u32 *)(base + addr));
> +}
> +
> +/* Initializing the kp Module */
> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp)
> +{
> +	u32 strobe = davinci_kp->pdata->strobe;
> +	u32 interval = davinci_kp->pdata->interval;
> +
> +	/* Enable all interrupts */
> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA);
> +
> +	/* Clear interrupts if any */
> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
> +
> +	/* Setup the scan period = strobe + interval */
> +	davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH);
> +	davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL);
> +	davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME);
> +
> +	/* Enable Keyscan module and enable */
> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN,
> +			DAVINCI_KEYPAD_KEYCTRL);
> +}
> +
> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id)
> +{
> +	struct davinci_kp *davinci_kp = dev_id;
> +	struct device *dev = &davinci_kp->input->dev;
> +	int *keymap = davinci_kp->pdata->keymap;
> +		u32 prev_status, new_status, changed, position;
> +	int keycode = KEY_UNKNOWN;
> +	int ret = IRQ_NONE;
> +
> +	/* Disable interrupt */
> +	davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA);
> +
> +	/* Reading previous and new status of the keypad */
> +	prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE);
> +	new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST);
> +
> +	changed = prev_status ^ new_status;
> +	position = ffs(changed) - 1;
> +
> +	if (changed) {

Can there be several buttons that change status at once?

> +		keycode = keymap[position];
> +		if((new_status >> position) & 0x1) {

bool release = (new_status >> position) & 0x1;
input_report_key(davinci_kp->input, keycode, !release);
dev_dbg(dev, "davinci_keypad: key %d %s\n",
	keycode, release ? "released" : "pressed");

is shorter.

> +			/* Report release */
> +			dev_dbg(dev, "davinci_keypad: key %d released\n",
> +				    keycode);
> +			input_report_key(davinci_kp->input, keycode, 0);
> +		} else {
> +			/* Report press */
> +			dev_dbg(dev, "davinci_keypad: key %d pressed\n",
> +				    keycode);
> +			input_report_key(davinci_kp->input, keycode, 1);
> +		}
> +		input_sync(davinci_kp->input);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	/* Clearing interrupt */
> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);

You return IRQ_HANDLED only if keypad state changed but clear interrupt
regardless. This is suspicious.

> +
> +	/* Enable interrupts */
> +	davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA);
> +
> +	return ret;
> +}
> +
> +static int __init davinci_kp_probe(struct platform_device *pdev)
> +{
> +	struct davinci_kp *davinci_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 Keypad Driver\n");
> +
> +	if (!pdata->keymap) {
> +		dev_dbg(dev, "no keymap from pdata\n");
> +		return -EINVAL;
> +	}
> +
> +	davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL);
> +	if(!davinci_kp) {
> +		dev_dbg(dev, "could not allocate memory for private data\n");
> +		return -ENOMEM;
> +	}
> +
> +	key_dev = input_allocate_device();
> +	if (!key_dev) {
> +		dev_dbg(dev, "could not allocate input device\n");
> +		ret = -ENOMEM;
> +		goto fail1;
> +	}
> +
> +	platform_set_drvdata(pdev, davinci_kp);
> +
> +	davinci_kp->input = key_dev;
> +
> +	davinci_kp->irq = platform_get_irq(pdev, 0);
> +	if (davinci_kp->irq < 0) {
> +		dev_err(dev, "no keypad irq\n");
> +		ret = davinci_kp->irq;
> +		goto fail2;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "no mem resource\n");
> +		ret = -ENODEV;

-EINVAL I'd say.

> +		goto fail2;
> +	}
> +
> +	davinci_kp->pbase = res->start;
> +	davinci_kp->base_size = resource_size(res);
> +
> +	mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name);
> +	if (!mem) {
> +		dev_err(dev, "KEYSCAN registers at %08x are not free\n",
> +			davinci_kp->pbase);
> +		ret = -EBUSY;
> +		goto fail2;
> +	}
> +
> +	davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size);
> +	if (!davinci_kp->base) {
> +		dev_err(dev, "can't ioremap MEM resource.\n");
> +		ret = -ENOMEM;
> +		goto fail3;
> +	}
> +
> +	/* Enable auto repeat feature of Linux input subsystem */
> +	if (pdata->rep)
> +		__set_bit(EV_REP, key_dev->evbit);
> +
> +	/* Setup input device */
> +	__set_bit(EV_KEY, key_dev->evbit);
> +
> +	/* Setup the keymap */
> +	davinci_kp->pdata = pdata;
> +
> +	for (i = 0; i < davinci_kp->pdata->keymapsize; i++)
> +		__set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit);
> +
> +	key_dev->name = "davinci_keypad";
> +	key_dev->phys = "davinci_keypad/input0";
> +	key_dev->dev.parent = &pdev->dev;
> +	key_dev->id.bustype = BUS_HOST;
> +	key_dev->id.vendor = 0x0001;
> +	key_dev->id.product = 0x0001;
> +	key_dev->id.version = 0x0001;
> +	key_dev->keycode = davinci_kp->pdata->keymap;

Please kopy keymap into the davinci_kp stucture and use it so that
platform data is never changed and can be declared const.

> +	key_dev->keycodesize = sizeof(unsigned int);

sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short.

> +	key_dev->keycodemax = davinci_kp->pdata->keymapsize;
> +
> +	ret = input_register_device(davinci_kp->input);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to register DaVinci keypad device\n");
> +		goto fail4;
> +	}
> +
> +	ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED,
> +                         "davinci_keypad", davinci_kp);
> +	if (ret < 0) {
> +		dev_err(dev, "unable to register DaVinci keypad Interrupt\n");
> +		goto fail5;
> +	}
> +
> +	davinci_kp_initialize(davinci_kp);
> +
> +	return 0;
> +fail5:
> +	input_unregister_device(davinci_kp->input);
> +	key_dev = NULL;
> +fail4:
> +	iounmap(davinci_kp->base);
> +fail3:
> +	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
> +fail2:
> +	input_free_device(key_dev);
> +fail1:
> +	kfree(davinci_kp);
> +
> +	return ret;
> +}
> +
> +static int __exit davinci_kp_remove(struct platform_device *pdev)

__devexit?

> +{
> +	struct davinci_kp *davinci_kp = platform_get_drvdata(pdev);
> +
> +	free_irq(davinci_kp->irq, davinci_kp);
> +
> +	input_unregister_device(davinci_kp->input);
> +
> +	iounmap(davinci_kp->base);
> +	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	kfree(davinci_kp);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver davinci_kp_driver = {
> +	.driver = {
> +			.name = "davinci_keypad",
> +			.owner = THIS_MODULE,
> +		},
> +	.remove = __exit_p(davinci_kp_remove),

__devexit_p(). I think you can still unbind the device even if you use
platform_driver_probe.

> +};
> +
> +static int __init davinci_kp_init(void)
> +{
> +	return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
> +}
> +module_init(davinci_kp_init);
> +
> +static void __exit davinci_kp_exit(void)
> +{
> +	platform_driver_unregister(&davinci_kp_driver);
> +}
> +module_exit(davinci_kp_exit);
> +
> +MODULE_AUTHOR("Miguel Aguilar");
> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.6.0.4
> 
> --
> 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
miguel.aguilar@ridgerun.com Sept. 23, 2009, 2:52 p.m. UTC | #2
Dmitry,

Dmitry Torokhov wrote:
> Hi Miguel,
> 
> On Tue, Sep 22, 2009 at 03:27:30PM -0600, miguel.aguilar@ridgerun.com wrote:
>> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>
>> Adds the driver for enabling keypad support for DaVinci platforms.
>>
>> DM365 is the only platform that uses this driver at the moment.
>>
>> This driver was tested on DM365 EVM rev C.
>>
>> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>> ---
>>  arch/arm/configs/davinci_all_defconfig      |    1 +
>>  arch/arm/mach-davinci/include/mach/keypad.h |   35 +++
>>  drivers/input/keyboard/Kconfig              |    7 +
>>  drivers/input/keyboard/Makefile             |    1 +
>>  drivers/input/keyboard/davinci_keypad.c     |  319 +++++++++++++++++++++++++++
>>  5 files changed, 363 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h
>>  create mode 100644 drivers/input/keyboard/davinci_keypad.c
>>
>> diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
>> index ec63c15..e994c83 100644
>> --- a/arch/arm/configs/davinci_all_defconfig
>> +++ b/arch/arm/configs/davinci_all_defconfig
>> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y
>>  # CONFIG_KEYBOARD_STOWAWAY is not set
>>  # CONFIG_KEYBOARD_SUNKBD is not set
>>  CONFIG_KEYBOARD_XTKBD=m
>> +CONFIG_KEYBOARD_DAVINCI_DM365=m
>>  # CONFIG_INPUT_MOUSE is not set
>>  # CONFIG_INPUT_JOYSTICK is not set
>>  # CONFIG_INPUT_TABLET is not set
> 
> 
> If you want to merge the driver through input tree the defconfig chunk
> has to go elsewhere.
[MA] So, Where it should go?
> 
>> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h
>> new file mode 100644
>> index 0000000..922d20e
>> --- /dev/null
>> +++ b/arch/arm/mach-davinci/include/mach/keypad.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * Copyright (C) 2009 Texas Instruments, Inc
>> + *
>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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
>> + */
>> +
>> +#ifndef DAVINCI_KEYPAD_H
>> +#define DAVINCI_KEYPAD_H
>> +
>> +#include <linux/io.h>
>> +
>> +struct davinci_kp_platform_data {
>> +	int	*keymap;
>> +	u32	keymapsize;
>> +	u32	rep:1;
>> +	u32	strobe;
>> +	u32	interval;
>> +};
>> +
>> +#endif
>> +
>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>> index a6b989a..b6b9517 100644
>> --- a/drivers/input/keyboard/Kconfig
>> +++ b/drivers/input/keyboard/Kconfig
>> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called xtkbd.
>>  
>> +config KEYBOARD_DAVINCI
>> +	tristate "TI DaVinci Keypad"
>> +	depends on ARCH_DAVINCI_DM365
>> +	help
>> +	  Say Y to enable keypad module support for the TI DaVinci
>> +	  platforms (DM365)
>> +
> 
> "To compile this driver as a module..."
[MA] Ok.
> 
>>  endif
>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
>> index b5b5eae..0b0274e 100644
>> --- a/drivers/input/keyboard/Makefile
>> +++ b/drivers/input/keyboard/Makefile
>> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
>>  obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
>>  obj-$(CONFIG_KEYBOARD_TOSA)		+= tosakbd.o
>>  obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
>> +obj-$(CONFIG_KEYBOARD_DAVINCI)		+= davinci_keypad.o
>> diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c
>> new file mode 100644
>> index 0000000..6f0e793
>> --- /dev/null
>> +++ b/drivers/input/keyboard/davinci_keypad.c
>> @@ -0,0 +1,319 @@
>> +/*
>> + * DaVinci Keypad Driver for TI platforms
>> + *
>> + * Copyright (C) 2009 Texas Instruments, Inc
>> + *
>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>> + *
>> + * Intial Code: Sandeep Paulraj <s-paulraj@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/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 DAVINCI_KEYPAD_KEYCTRL		0x0000
>> +#define DAVINCI_KEYPAD_INTENA		0x0004
>> +#define DAVINCI_KEYPAD_INTFLAG		0x0008
>> +#define DAVINCI_KEYPAD_INTCLR		0x000c
>> +#define DAVINCI_KEYPAD_STRBWIDTH	0x0010
>> +#define DAVINCI_KEYPAD_INTERVAL		0x0014
>> +#define DAVINCI_KEYPAD_CONTTIME		0x0018
>> +#define DAVINCI_KEYPAD_CURRENTST	0x001c
>> +#define DAVINCI_KEYPAD_PREVSTATE	0x0020
>> +#define DAVINCI_KEYPAD_EMUCTRL		0x0024
>> +#define DAVINCI_KEYPAD_IODFTCTRL	0x002c
>> +
>> +/* Key Control Register (KEYCTRL) */
>> +#define DAVINCI_KEYPAD_KEYEN		0x00000001
>> +#define DAVINCI_KEYPAD_PREVMODE		0x00000002
>> +#define DAVINCI_KEYPAD_CHATOFF		0x00000004
>> +#define DAVINCI_KEYPAD_AUTODET		0x00000008
>> +#define DAVINCI_KEYPAD_SCANMODE		0x00000010
>> +#define DAVINCI_KEYPAD_OUTTYPE		0x00000020
>> +#define DAVINCI_KEYPAD_4X4		0x00000040
>> +
>> +/* Masks for the interrupts */
>> +#define DAVINCI_KEYPAD_INT_CONT		0x00000008
>> +#define DAVINCI_KEYPAD_INT_OFF		0x00000004
>> +#define DAVINCI_KEYPAD_INT_ON		0x00000002
>> +#define DAVINCI_KEYPAD_INT_CHANGE	0x00000001
>> +#define DAVINCI_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 davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr)
>> +{
>> +	u32 base = (u32)davinci_kp->base;
>> +
>> +	__raw_writel(val,(u32 *)(base + addr));
>> +}
>> +
>> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr)
>> +{
>> +	u32 base = (u32)davinci_kp->base;
>> +
>> +	return __raw_readl((u32 *)(base + addr));
>> +}
>> +
>> +/* Initializing the kp Module */
>> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp)
>> +{
>> +	u32 strobe = davinci_kp->pdata->strobe;
>> +	u32 interval = davinci_kp->pdata->interval;
>> +
>> +	/* Enable all interrupts */
>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA);
>> +
>> +	/* Clear interrupts if any */
>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
>> +
>> +	/* Setup the scan period = strobe + interval */
>> +	davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH);
>> +	davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL);
>> +	davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME);
>> +
>> +	/* Enable Keyscan module and enable */
>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN,
>> +			DAVINCI_KEYPAD_KEYCTRL);
>> +}
>> +
>> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id)
>> +{
>> +	struct davinci_kp *davinci_kp = dev_id;
>> +	struct device *dev = &davinci_kp->input->dev;
>> +	int *keymap = davinci_kp->pdata->keymap;
>> +		u32 prev_status, new_status, changed, position;
>> +	int keycode = KEY_UNKNOWN;
>> +	int ret = IRQ_NONE;
>> +
>> +	/* Disable interrupt */
>> +	davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA);
>> +
>> +	/* Reading previous and new status of the keypad */
>> +	prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE);
>> +	new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST);
>> +
>> +	changed = prev_status ^ new_status;
>> +	position = ffs(changed) - 1;
>> +
>> +	if (changed) {
> 
> Can there be several buttons that change status at once?
[MA] It is not suppose to change several buttons at once.
> 
>> +		keycode = keymap[position];
>> +		if((new_status >> position) & 0x1) {
> 
> bool release = (new_status >> position) & 0x1;
> input_report_key(davinci_kp->input, keycode, !release);
> dev_dbg(dev, "davinci_keypad: key %d %s\n",
> 	keycode, release ? "released" : "pressed");
> 
[MA] Ok I'll try that.
> is shorter.
> 
>> +			/* Report release */
>> +			dev_dbg(dev, "davinci_keypad: key %d released\n",
>> +				    keycode);
>> +			input_report_key(davinci_kp->input, keycode, 0);
>> +		} else {
>> +			/* Report press */
>> +			dev_dbg(dev, "davinci_keypad: key %d pressed\n",
>> +				    keycode);
>> +			input_report_key(davinci_kp->input, keycode, 1);
>> +		}
>> +		input_sync(davinci_kp->input);
>> +		ret = IRQ_HANDLED;
>> +	}
>> +
>> +	/* Clearing interrupt */
>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
> 
> You return IRQ_HANDLED only if keypad state changed but clear interrupt
> regardless. This is suspicious.
[MA] Ok. I'll clear the irq status only if IRQ_HANDLED.
> 
>> +
>> +	/* Enable interrupts */
>> +	davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA);
>> +
>> +	return ret;
>> +}
>> +
>> +static int __init davinci_kp_probe(struct platform_device *pdev)
>> +{
>> +	struct davinci_kp *davinci_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 Keypad Driver\n");
>> +
>> +	if (!pdata->keymap) {
>> +		dev_dbg(dev, "no keymap from pdata\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL);
>> +	if(!davinci_kp) {
>> +		dev_dbg(dev, "could not allocate memory for private data\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	key_dev = input_allocate_device();
>> +	if (!key_dev) {
>> +		dev_dbg(dev, "could not allocate input device\n");
>> +		ret = -ENOMEM;
>> +		goto fail1;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, davinci_kp);
>> +
>> +	davinci_kp->input = key_dev;
>> +
>> +	davinci_kp->irq = platform_get_irq(pdev, 0);
>> +	if (davinci_kp->irq < 0) {
>> +		dev_err(dev, "no keypad irq\n");
>> +		ret = davinci_kp->irq;
>> +		goto fail2;
>> +	}
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(dev, "no mem resource\n");
>> +		ret = -ENODEV;
> 
> -EINVAL I'd say.
[MA] platform_get_resource should fail with -ENODEV.
> 
>> +		goto fail2;
>> +	}
>> +
>> +	davinci_kp->pbase = res->start;
>> +	davinci_kp->base_size = resource_size(res);
>> +
>> +	mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name);
>> +	if (!mem) {
>> +		dev_err(dev, "KEYSCAN registers at %08x are not free\n",
>> +			davinci_kp->pbase);
>> +		ret = -EBUSY;
>> +		goto fail2;
>> +	}
>> +
>> +	davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size);
>> +	if (!davinci_kp->base) {
>> +		dev_err(dev, "can't ioremap MEM resource.\n");
>> +		ret = -ENOMEM;
>> +		goto fail3;
>> +	}
>> +
>> +	/* Enable auto repeat feature of Linux input subsystem */
>> +	if (pdata->rep)
>> +		__set_bit(EV_REP, key_dev->evbit);
>> +
>> +	/* Setup input device */
>> +	__set_bit(EV_KEY, key_dev->evbit);
>> +
>> +	/* Setup the keymap */
>> +	davinci_kp->pdata = pdata;
>> +
>> +	for (i = 0; i < davinci_kp->pdata->keymapsize; i++)
>> +		__set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit);
>> +
>> +	key_dev->name = "davinci_keypad";
>> +	key_dev->phys = "davinci_keypad/input0";
>> +	key_dev->dev.parent = &pdev->dev;
>> +	key_dev->id.bustype = BUS_HOST;
>> +	key_dev->id.vendor = 0x0001;
>> +	key_dev->id.product = 0x0001;
>> +	key_dev->id.version = 0x0001;
>> +	key_dev->keycode = davinci_kp->pdata->keymap;
> 
> Please kopy keymap into the davinci_kp stucture and use it so that
> platform data is never changed and can be declared const.
Do you mean something like this?

struct davinci_kp {
	...
	const int	*keymap;
	...
};

> 
>> +	key_dev->keycodesize = sizeof(unsigned int);
> 
> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short.
[MA] Ok.
> 
>> +	key_dev->keycodemax = davinci_kp->pdata->keymapsize;
>> +
>> +	ret = input_register_device(davinci_kp->input);
>> +	if (ret < 0) {
>> +		dev_err(dev, "unable to register DaVinci keypad device\n");
>> +		goto fail4;
>> +	}
>> +
>> +	ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED,
>> +                         "davinci_keypad", davinci_kp);
>> +	if (ret < 0) {
>> +		dev_err(dev, "unable to register DaVinci keypad Interrupt\n");
>> +		goto fail5;
>> +	}
>> +
>> +	davinci_kp_initialize(davinci_kp);
>> +
>> +	return 0;
>> +fail5:
>> +	input_unregister_device(davinci_kp->input);
>> +	key_dev = NULL;
>> +fail4:
>> +	iounmap(davinci_kp->base);
>> +fail3:
>> +	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
>> +fail2:
>> +	input_free_device(key_dev);
>> +fail1:
>> +	kfree(davinci_kp);
>> +
>> +	return ret;
>> +}
>> +
>> +static int __exit davinci_kp_remove(struct platform_device *pdev)
> 
> __devexit?
[MA] According to comments from David Brownell to the first version of this 
patch the __exit should be used.

" - Use platform_driver_probe() and __exit/__exit_p();
    there's no point in keeping that code around in
    typical configs, it'd just waste memory. "

> 
>> +{
>> +	struct davinci_kp *davinci_kp = platform_get_drvdata(pdev);
>> +
>> +	free_irq(davinci_kp->irq, davinci_kp);
>> +
>> +	input_unregister_device(davinci_kp->input);
>> +
>> +	iounmap(davinci_kp->base);
>> +	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
>> +
>> +	platform_set_drvdata(pdev, NULL);
>> +
>> +	kfree(davinci_kp);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver davinci_kp_driver = {
>> +	.driver = {
>> +			.name = "davinci_keypad",
>> +			.owner = THIS_MODULE,
>> +		},
>> +	.remove = __exit_p(davinci_kp_remove),
> 
> __devexit_p(). I think you can still unbind the device even if you use
> platform_driver_probe.
[MA] Same.
> 
>> +};
>> +
>> +static int __init davinci_kp_init(void)
>> +{
>> +	return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
>> +}
>> +module_init(davinci_kp_init);
>> +
>> +static void __exit davinci_kp_exit(void)
>> +{
>> +	platform_driver_unregister(&davinci_kp_driver);
>> +}
>> +module_exit(davinci_kp_exit);
>> +
>> +MODULE_AUTHOR("Miguel Aguilar");
>> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 1.6.0.4
>>
>> --
>> 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
> 


--
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
Dmitry Torokhov Sept. 23, 2009, 4:35 p.m. UTC | #3
Hi Miguel,

[Adding David to CC for the __devexit/__exit discussion]

On Wed, Sep 23, 2009 at 08:52:40AM -0600, Miguel Aguilar wrote:
> Dmitry,
>
> Dmitry Torokhov wrote:
>> Hi Miguel,
>>
>> On Tue, Sep 22, 2009 at 03:27:30PM -0600, miguel.aguilar@ridgerun.com wrote:
>>> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>>
>>> Adds the driver for enabling keypad support for DaVinci platforms.
>>>
>>> DM365 is the only platform that uses this driver at the moment.
>>>
>>> This driver was tested on DM365 EVM rev C.
>>>
>>> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>> ---
>>>  arch/arm/configs/davinci_all_defconfig      |    1 +
>>>  arch/arm/mach-davinci/include/mach/keypad.h |   35 +++
>>>  drivers/input/keyboard/Kconfig              |    7 +
>>>  drivers/input/keyboard/Makefile             |    1 +
>>>  drivers/input/keyboard/davinci_keypad.c     |  319 +++++++++++++++++++++++++++
>>>  5 files changed, 363 insertions(+), 0 deletions(-)
>>>  create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h
>>>  create mode 100644 drivers/input/keyboard/davinci_keypad.c
>>>
>>> diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
>>> index ec63c15..e994c83 100644
>>> --- a/arch/arm/configs/davinci_all_defconfig
>>> +++ b/arch/arm/configs/davinci_all_defconfig
>>> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y
>>>  # CONFIG_KEYBOARD_STOWAWAY is not set
>>>  # CONFIG_KEYBOARD_SUNKBD is not set
>>>  CONFIG_KEYBOARD_XTKBD=m
>>> +CONFIG_KEYBOARD_DAVINCI_DM365=m
>>>  # CONFIG_INPUT_MOUSE is not set
>>>  # CONFIG_INPUT_JOYSTICK is not set
>>>  # CONFIG_INPUT_TABLET is not set
>>
>>
>> If you want to merge the driver through input tree the defconfig chunk
>> has to go elsewhere.
> [MA] So, Where it should go?

In a separate patch submitted to the person maintaining defconfig for
the arch in question once driver is in mainline.

>>
>>> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h
>>> new file mode 100644
>>> index 0000000..922d20e
>>> --- /dev/null
>>> +++ b/arch/arm/mach-davinci/include/mach/keypad.h
>>> @@ -0,0 +1,35 @@
>>> +/*
>>> + * Copyright (C) 2009 Texas Instruments, Inc
>>> + *
>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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
>>> + */
>>> +
>>> +#ifndef DAVINCI_KEYPAD_H
>>> +#define DAVINCI_KEYPAD_H
>>> +
>>> +#include <linux/io.h>
>>> +
>>> +struct davinci_kp_platform_data {
>>> +	int	*keymap;
>>> +	u32	keymapsize;
>>> +	u32	rep:1;
>>> +	u32	strobe;
>>> +	u32	interval;
>>> +};
>>> +
>>> +#endif
>>> +
>>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>>> index a6b989a..b6b9517 100644
>>> --- a/drivers/input/keyboard/Kconfig
>>> +++ b/drivers/input/keyboard/Kconfig
>>> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD
>>>  	  To compile this driver as a module, choose M here: the
>>>  	  module will be called xtkbd.
>>>  +config KEYBOARD_DAVINCI
>>> +	tristate "TI DaVinci Keypad"
>>> +	depends on ARCH_DAVINCI_DM365
>>> +	help
>>> +	  Say Y to enable keypad module support for the TI DaVinci
>>> +	  platforms (DM365)
>>> +
>>
>> "To compile this driver as a module..."
> [MA] Ok.
>>
>>>  endif
>>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
>>> index b5b5eae..0b0274e 100644
>>> --- a/drivers/input/keyboard/Makefile
>>> +++ b/drivers/input/keyboard/Makefile
>>> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
>>>  obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
>>>  obj-$(CONFIG_KEYBOARD_TOSA)		+= tosakbd.o
>>>  obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
>>> +obj-$(CONFIG_KEYBOARD_DAVINCI)		+= davinci_keypad.o
>>> diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c
>>> new file mode 100644
>>> index 0000000..6f0e793
>>> --- /dev/null
>>> +++ b/drivers/input/keyboard/davinci_keypad.c
>>> @@ -0,0 +1,319 @@
>>> +/*
>>> + * DaVinci Keypad Driver for TI platforms
>>> + *
>>> + * Copyright (C) 2009 Texas Instruments, Inc
>>> + *
>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>> + *
>>> + * Intial Code: Sandeep Paulraj <s-paulraj@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/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 DAVINCI_KEYPAD_KEYCTRL		0x0000
>>> +#define DAVINCI_KEYPAD_INTENA		0x0004
>>> +#define DAVINCI_KEYPAD_INTFLAG		0x0008
>>> +#define DAVINCI_KEYPAD_INTCLR		0x000c
>>> +#define DAVINCI_KEYPAD_STRBWIDTH	0x0010
>>> +#define DAVINCI_KEYPAD_INTERVAL		0x0014
>>> +#define DAVINCI_KEYPAD_CONTTIME		0x0018
>>> +#define DAVINCI_KEYPAD_CURRENTST	0x001c
>>> +#define DAVINCI_KEYPAD_PREVSTATE	0x0020
>>> +#define DAVINCI_KEYPAD_EMUCTRL		0x0024
>>> +#define DAVINCI_KEYPAD_IODFTCTRL	0x002c
>>> +
>>> +/* Key Control Register (KEYCTRL) */
>>> +#define DAVINCI_KEYPAD_KEYEN		0x00000001
>>> +#define DAVINCI_KEYPAD_PREVMODE		0x00000002
>>> +#define DAVINCI_KEYPAD_CHATOFF		0x00000004
>>> +#define DAVINCI_KEYPAD_AUTODET		0x00000008
>>> +#define DAVINCI_KEYPAD_SCANMODE		0x00000010
>>> +#define DAVINCI_KEYPAD_OUTTYPE		0x00000020
>>> +#define DAVINCI_KEYPAD_4X4		0x00000040
>>> +
>>> +/* Masks for the interrupts */
>>> +#define DAVINCI_KEYPAD_INT_CONT		0x00000008
>>> +#define DAVINCI_KEYPAD_INT_OFF		0x00000004
>>> +#define DAVINCI_KEYPAD_INT_ON		0x00000002
>>> +#define DAVINCI_KEYPAD_INT_CHANGE	0x00000001
>>> +#define DAVINCI_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 davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr)
>>> +{
>>> +	u32 base = (u32)davinci_kp->base;
>>> +
>>> +	__raw_writel(val,(u32 *)(base + addr));
>>> +}
>>> +
>>> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr)
>>> +{
>>> +	u32 base = (u32)davinci_kp->base;
>>> +
>>> +	return __raw_readl((u32 *)(base + addr));
>>> +}
>>> +
>>> +/* Initializing the kp Module */
>>> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp)
>>> +{
>>> +	u32 strobe = davinci_kp->pdata->strobe;
>>> +	u32 interval = davinci_kp->pdata->interval;
>>> +
>>> +	/* Enable all interrupts */
>>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA);
>>> +
>>> +	/* Clear interrupts if any */
>>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
>>> +
>>> +	/* Setup the scan period = strobe + interval */
>>> +	davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH);
>>> +	davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL);
>>> +	davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME);
>>> +
>>> +	/* Enable Keyscan module and enable */
>>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN,
>>> +			DAVINCI_KEYPAD_KEYCTRL);
>>> +}
>>> +
>>> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id)
>>> +{
>>> +	struct davinci_kp *davinci_kp = dev_id;
>>> +	struct device *dev = &davinci_kp->input->dev;
>>> +	int *keymap = davinci_kp->pdata->keymap;
>>> +		u32 prev_status, new_status, changed, position;
>>> +	int keycode = KEY_UNKNOWN;
>>> +	int ret = IRQ_NONE;
>>> +
>>> +	/* Disable interrupt */
>>> +	davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA);
>>> +
>>> +	/* Reading previous and new status of the keypad */
>>> +	prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE);
>>> +	new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST);
>>> +
>>> +	changed = prev_status ^ new_status;
>>> +	position = ffs(changed) - 1;
>>> +
>>> +	if (changed) {
>>
>> Can there be several buttons that change status at once?
> [MA] It is not suppose to change several buttons at once.

"Not supposed to happen" vs. "it can not happen, even in theory"?

>>
>>> +		keycode = keymap[position];
>>> +		if((new_status >> position) & 0x1) {
>>
>> bool release = (new_status >> position) & 0x1;
>> input_report_key(davinci_kp->input, keycode, !release);
>> dev_dbg(dev, "davinci_keypad: key %d %s\n",
>> 	keycode, release ? "released" : "pressed");
>>
> [MA] Ok I'll try that.
>> is shorter.
>>
>>> +			/* Report release */
>>> +			dev_dbg(dev, "davinci_keypad: key %d released\n",
>>> +				    keycode);
>>> +			input_report_key(davinci_kp->input, keycode, 0);
>>> +		} else {
>>> +			/* Report press */
>>> +			dev_dbg(dev, "davinci_keypad: key %d pressed\n",
>>> +				    keycode);
>>> +			input_report_key(davinci_kp->input, keycode, 1);
>>> +		}
>>> +		input_sync(davinci_kp->input);
>>> +		ret = IRQ_HANDLED;
>>> +	}
>>> +
>>> +	/* Clearing interrupt */
>>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
>>
>> You return IRQ_HANDLED only if keypad state changed but clear interrupt
>> regardless. This is suspicious.
> [MA] Ok. I'll clear the irq status only if IRQ_HANDLED.

It really depends... What if key was pressed but is released before
interrupt is handled? Do you want to see "spurious IRQ" warnings?

>>
>>> +
>>> +	/* Enable interrupts */
>>> +	davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int __init davinci_kp_probe(struct platform_device *pdev)
>>> +{
>>> +	struct davinci_kp *davinci_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 Keypad Driver\n");
>>> +
>>> +	if (!pdata->keymap) {
>>> +		dev_dbg(dev, "no keymap from pdata\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL);
>>> +	if(!davinci_kp) {
>>> +		dev_dbg(dev, "could not allocate memory for private data\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	key_dev = input_allocate_device();
>>> +	if (!key_dev) {
>>> +		dev_dbg(dev, "could not allocate input device\n");
>>> +		ret = -ENOMEM;
>>> +		goto fail1;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, davinci_kp);
>>> +
>>> +	davinci_kp->input = key_dev;
>>> +
>>> +	davinci_kp->irq = platform_get_irq(pdev, 0);
>>> +	if (davinci_kp->irq < 0) {
>>> +		dev_err(dev, "no keypad irq\n");
>>> +		ret = davinci_kp->irq;
>>> +		goto fail2;
>>> +	}
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	if (!res) {
>>> +		dev_err(dev, "no mem resource\n");
>>> +		ret = -ENODEV;
>>
>> -EINVAL I'd say.
> [MA] platform_get_resource should fail with -ENODEV.

If you fail to get platform resource then th eplatform code is set up
incorrectly, therefore I'd still argue for -EINVAL. It is not as if
device may not be there because then the platform device would not be
present at all.

>>
>>> +		goto fail2;
>>> +	}
>>> +
>>> +	davinci_kp->pbase = res->start;
>>> +	davinci_kp->base_size = resource_size(res);
>>> +
>>> +	mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name);
>>> +	if (!mem) {
>>> +		dev_err(dev, "KEYSCAN registers at %08x are not free\n",
>>> +			davinci_kp->pbase);
>>> +		ret = -EBUSY;
>>> +		goto fail2;
>>> +	}
>>> +
>>> +	davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size);
>>> +	if (!davinci_kp->base) {
>>> +		dev_err(dev, "can't ioremap MEM resource.\n");
>>> +		ret = -ENOMEM;
>>> +		goto fail3;
>>> +	}
>>> +
>>> +	/* Enable auto repeat feature of Linux input subsystem */
>>> +	if (pdata->rep)
>>> +		__set_bit(EV_REP, key_dev->evbit);
>>> +
>>> +	/* Setup input device */
>>> +	__set_bit(EV_KEY, key_dev->evbit);
>>> +
>>> +	/* Setup the keymap */
>>> +	davinci_kp->pdata = pdata;
>>> +
>>> +	for (i = 0; i < davinci_kp->pdata->keymapsize; i++)
>>> +		__set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit);
>>> +
>>> +	key_dev->name = "davinci_keypad";
>>> +	key_dev->phys = "davinci_keypad/input0";
>>> +	key_dev->dev.parent = &pdev->dev;
>>> +	key_dev->id.bustype = BUS_HOST;
>>> +	key_dev->id.vendor = 0x0001;
>>> +	key_dev->id.product = 0x0001;
>>> +	key_dev->id.version = 0x0001;
>>> +	key_dev->keycode = davinci_kp->pdata->keymap;
>>
>> Please kopy keymap into the davinci_kp stucture and use it so that
>> platform data is never changed and can be declared const.
> Do you mean something like this?
>
> struct davinci_kp {
> 	...
> 	const int	*keymap;
> 	...
> };
>

More like:

struct davinci_kp {
	...
	unsgned char keymap[];
	...
};

and then you copy keymap from platform data into device-private
structure so it can be safely changed via EVIOCSKEYCODE without
affecting platform data. Also, if you reload the module, the keymap with
be restored to its original state. The platform data may also be marked
as const.

If max size of keymap is not known then you may need to allocate it
dynamically.

>>
>>> +	key_dev->keycodesize = sizeof(unsigned int);
>>
>> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short.
> [MA] Ok.
>>
>>> +	key_dev->keycodemax = davinci_kp->pdata->keymapsize;
>>> +
>>> +	ret = input_register_device(davinci_kp->input);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "unable to register DaVinci keypad device\n");
>>> +		goto fail4;
>>> +	}
>>> +
>>> +	ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED,
>>> +                         "davinci_keypad", davinci_kp);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "unable to register DaVinci keypad Interrupt\n");
>>> +		goto fail5;
>>> +	}
>>> +
>>> +	davinci_kp_initialize(davinci_kp);
>>> +
>>> +	return 0;
>>> +fail5:
>>> +	input_unregister_device(davinci_kp->input);
>>> +	key_dev = NULL;
>>> +fail4:
>>> +	iounmap(davinci_kp->base);
>>> +fail3:
>>> +	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
>>> +fail2:
>>> +	input_free_device(key_dev);
>>> +fail1:
>>> +	kfree(davinci_kp);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int __exit davinci_kp_remove(struct platform_device *pdev)
>>
>> __devexit?
> [MA] According to comments from David Brownell to the first version of 
> this patch the __exit should be used.
>
> " - Use platform_driver_probe() and __exit/__exit_p();
>    there's no point in keeping that code around in
>    typical configs, it'd just waste memory. "

I am afraid David is wrong here. Even when we register driver with
platform_driver_probe() we still have "unbind" attribute in sysfs which
may be used to unbind the device from driver. If code is __exit then
such attempts will cause oops.

>
>>
>>> +{
>>> +	struct davinci_kp *davinci_kp = platform_get_drvdata(pdev);
>>> +
>>> +	free_irq(davinci_kp->irq, davinci_kp);
>>> +
>>> +	input_unregister_device(davinci_kp->input);
>>> +
>>> +	iounmap(davinci_kp->base);
>>> +	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
>>> +
>>> +	platform_set_drvdata(pdev, NULL);
>>> +
>>> +	kfree(davinci_kp);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver davinci_kp_driver = {
>>> +	.driver = {
>>> +			.name = "davinci_keypad",
>>> +			.owner = THIS_MODULE,
>>> +		},
>>> +	.remove = __exit_p(davinci_kp_remove),
>>
>> __devexit_p(). I think you can still unbind the device even if you use
>> platform_driver_probe.
> [MA] Same.

Right, see above.

>>
>>> +};
>>> +
>>> +static int __init davinci_kp_init(void)
>>> +{
>>> +	return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
>>> +}
>>> +module_init(davinci_kp_init);
>>> +
>>> +static void __exit davinci_kp_exit(void)
>>> +{
>>> +	platform_driver_unregister(&davinci_kp_driver);
>>> +}
>>> +module_exit(davinci_kp_exit);
>>> +
>>> +MODULE_AUTHOR("Miguel Aguilar");
>>> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver");
>>> +MODULE_LICENSE("GPL");
>>> -- 
>>> 1.6.0.4
>>>
>>> --
>>> 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
>>
>
>
miguel.aguilar@ridgerun.com Sept. 23, 2009, 5:07 p.m. UTC | #4
Dmitry,

Dmitry Torokhov wrote:
> Hi Miguel,
> 
> [Adding David to CC for the __devexit/__exit discussion]
> 
> On Wed, Sep 23, 2009 at 08:52:40AM -0600, Miguel Aguilar wrote:
>> Dmitry,
>>
>> Dmitry Torokhov wrote:
>>> Hi Miguel,
>>>
>>> On Tue, Sep 22, 2009 at 03:27:30PM -0600, miguel.aguilar@ridgerun.com wrote:
>>>> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>>>
>>>> Adds the driver for enabling keypad support for DaVinci platforms.
>>>>
>>>> DM365 is the only platform that uses this driver at the moment.
>>>>
>>>> This driver was tested on DM365 EVM rev C.
>>>>
>>>> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>>> ---
>>>>  arch/arm/configs/davinci_all_defconfig      |    1 +
>>>>  arch/arm/mach-davinci/include/mach/keypad.h |   35 +++
>>>>  drivers/input/keyboard/Kconfig              |    7 +
>>>>  drivers/input/keyboard/Makefile             |    1 +
>>>>  drivers/input/keyboard/davinci_keypad.c     |  319 +++++++++++++++++++++++++++
>>>>  5 files changed, 363 insertions(+), 0 deletions(-)
>>>>  create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h
>>>>  create mode 100644 drivers/input/keyboard/davinci_keypad.c
>>>>
>>>> diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
>>>> index ec63c15..e994c83 100644
>>>> --- a/arch/arm/configs/davinci_all_defconfig
>>>> +++ b/arch/arm/configs/davinci_all_defconfig
>>>> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y
>>>>  # CONFIG_KEYBOARD_STOWAWAY is not set
>>>>  # CONFIG_KEYBOARD_SUNKBD is not set
>>>>  CONFIG_KEYBOARD_XTKBD=m
>>>> +CONFIG_KEYBOARD_DAVINCI_DM365=m
>>>>  # CONFIG_INPUT_MOUSE is not set
>>>>  # CONFIG_INPUT_JOYSTICK is not set
>>>>  # CONFIG_INPUT_TABLET is not set
>>>
>>> If you want to merge the driver through input tree the defconfig chunk
>>> has to go elsewhere.
>> [MA] So, Where it should go?
> 
> In a separate patch submitted to the person maintaining defconfig for
> the arch in question once driver is in mainline.

[MA] Ok I see.
> 
>>>> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h
>>>> new file mode 100644
>>>> index 0000000..922d20e
>>>> --- /dev/null
>>>> +++ b/arch/arm/mach-davinci/include/mach/keypad.h
>>>> @@ -0,0 +1,35 @@
>>>> +/*
>>>> + * Copyright (C) 2009 Texas Instruments, Inc
>>>> + *
>>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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
>>>> + */
>>>> +
>>>> +#ifndef DAVINCI_KEYPAD_H
>>>> +#define DAVINCI_KEYPAD_H
>>>> +
>>>> +#include <linux/io.h>
>>>> +
>>>> +struct davinci_kp_platform_data {
>>>> +	int	*keymap;
>>>> +	u32	keymapsize;
>>>> +	u32	rep:1;
>>>> +	u32	strobe;
>>>> +	u32	interval;
>>>> +};
>>>> +
>>>> +#endif
>>>> +
>>>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>>>> index a6b989a..b6b9517 100644
>>>> --- a/drivers/input/keyboard/Kconfig
>>>> +++ b/drivers/input/keyboard/Kconfig
>>>> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD
>>>>  	  To compile this driver as a module, choose M here: the
>>>>  	  module will be called xtkbd.
>>>>  +config KEYBOARD_DAVINCI
>>>> +	tristate "TI DaVinci Keypad"
>>>> +	depends on ARCH_DAVINCI_DM365
>>>> +	help
>>>> +	  Say Y to enable keypad module support for the TI DaVinci
>>>> +	  platforms (DM365)
>>>> +
>>> "To compile this driver as a module..."
>> [MA] Ok.
>>>>  endif
>>>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
>>>> index b5b5eae..0b0274e 100644
>>>> --- a/drivers/input/keyboard/Makefile
>>>> +++ b/drivers/input/keyboard/Makefile
>>>> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
>>>>  obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
>>>>  obj-$(CONFIG_KEYBOARD_TOSA)		+= tosakbd.o
>>>>  obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
>>>> +obj-$(CONFIG_KEYBOARD_DAVINCI)		+= davinci_keypad.o
>>>> diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c
>>>> new file mode 100644
>>>> index 0000000..6f0e793
>>>> --- /dev/null
>>>> +++ b/drivers/input/keyboard/davinci_keypad.c
>>>> @@ -0,0 +1,319 @@
>>>> +/*
>>>> + * DaVinci Keypad Driver for TI platforms
>>>> + *
>>>> + * Copyright (C) 2009 Texas Instruments, Inc
>>>> + *
>>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>>> + *
>>>> + * Intial Code: Sandeep Paulraj <s-paulraj@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/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 DAVINCI_KEYPAD_KEYCTRL		0x0000
>>>> +#define DAVINCI_KEYPAD_INTENA		0x0004
>>>> +#define DAVINCI_KEYPAD_INTFLAG		0x0008
>>>> +#define DAVINCI_KEYPAD_INTCLR		0x000c
>>>> +#define DAVINCI_KEYPAD_STRBWIDTH	0x0010
>>>> +#define DAVINCI_KEYPAD_INTERVAL		0x0014
>>>> +#define DAVINCI_KEYPAD_CONTTIME		0x0018
>>>> +#define DAVINCI_KEYPAD_CURRENTST	0x001c
>>>> +#define DAVINCI_KEYPAD_PREVSTATE	0x0020
>>>> +#define DAVINCI_KEYPAD_EMUCTRL		0x0024
>>>> +#define DAVINCI_KEYPAD_IODFTCTRL	0x002c
>>>> +
>>>> +/* Key Control Register (KEYCTRL) */
>>>> +#define DAVINCI_KEYPAD_KEYEN		0x00000001
>>>> +#define DAVINCI_KEYPAD_PREVMODE		0x00000002
>>>> +#define DAVINCI_KEYPAD_CHATOFF		0x00000004
>>>> +#define DAVINCI_KEYPAD_AUTODET		0x00000008
>>>> +#define DAVINCI_KEYPAD_SCANMODE		0x00000010
>>>> +#define DAVINCI_KEYPAD_OUTTYPE		0x00000020
>>>> +#define DAVINCI_KEYPAD_4X4		0x00000040
>>>> +
>>>> +/* Masks for the interrupts */
>>>> +#define DAVINCI_KEYPAD_INT_CONT		0x00000008
>>>> +#define DAVINCI_KEYPAD_INT_OFF		0x00000004
>>>> +#define DAVINCI_KEYPAD_INT_ON		0x00000002
>>>> +#define DAVINCI_KEYPAD_INT_CHANGE	0x00000001
>>>> +#define DAVINCI_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 davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr)
>>>> +{
>>>> +	u32 base = (u32)davinci_kp->base;
>>>> +
>>>> +	__raw_writel(val,(u32 *)(base + addr));
>>>> +}
>>>> +
>>>> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr)
>>>> +{
>>>> +	u32 base = (u32)davinci_kp->base;
>>>> +
>>>> +	return __raw_readl((u32 *)(base + addr));
>>>> +}
>>>> +
>>>> +/* Initializing the kp Module */
>>>> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp)
>>>> +{
>>>> +	u32 strobe = davinci_kp->pdata->strobe;
>>>> +	u32 interval = davinci_kp->pdata->interval;
>>>> +
>>>> +	/* Enable all interrupts */
>>>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA);
>>>> +
>>>> +	/* Clear interrupts if any */
>>>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
>>>> +
>>>> +	/* Setup the scan period = strobe + interval */
>>>> +	davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH);
>>>> +	davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL);
>>>> +	davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME);
>>>> +
>>>> +	/* Enable Keyscan module and enable */
>>>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN,
>>>> +			DAVINCI_KEYPAD_KEYCTRL);
>>>> +}
>>>> +
>>>> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id)
>>>> +{
>>>> +	struct davinci_kp *davinci_kp = dev_id;
>>>> +	struct device *dev = &davinci_kp->input->dev;
>>>> +	int *keymap = davinci_kp->pdata->keymap;
>>>> +		u32 prev_status, new_status, changed, position;
>>>> +	int keycode = KEY_UNKNOWN;
>>>> +	int ret = IRQ_NONE;
>>>> +
>>>> +	/* Disable interrupt */
>>>> +	davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA);
>>>> +
>>>> +	/* Reading previous and new status of the keypad */
>>>> +	prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE);
>>>> +	new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST);
>>>> +
>>>> +	changed = prev_status ^ new_status;
>>>> +	position = ffs(changed) - 1;
>>>> +
>>>> +	if (changed) {
>>> Can there be several buttons that change status at once?
>> [MA] It is not suppose to change several buttons at once.
> 
> "Not supposed to happen" vs. "it can not happen, even in theory"?
[MA]According with the TI documentation the scan process and the way of how the 
IRQ is handle, expects one key at time.
> 
>>>> +		keycode = keymap[position];
>>>> +		if((new_status >> position) & 0x1) {
>>> bool release = (new_status >> position) & 0x1;
>>> input_report_key(davinci_kp->input, keycode, !release);
>>> dev_dbg(dev, "davinci_keypad: key %d %s\n",
>>> 	keycode, release ? "released" : "pressed");
>>>
>> [MA] Ok I'll try that.
>>> is shorter.
>>>
>>>> +			/* Report release */
>>>> +			dev_dbg(dev, "davinci_keypad: key %d released\n",
>>>> +				    keycode);
>>>> +			input_report_key(davinci_kp->input, keycode, 0);
>>>> +		} else {
>>>> +			/* Report press */
>>>> +			dev_dbg(dev, "davinci_keypad: key %d pressed\n",
>>>> +				    keycode);
>>>> +			input_report_key(davinci_kp->input, keycode, 1);
>>>> +		}
>>>> +		input_sync(davinci_kp->input);
>>>> +		ret = IRQ_HANDLED;
>>>> +	}
>>>> +
>>>> +	/* Clearing interrupt */
>>>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
>>> You return IRQ_HANDLED only if keypad state changed but clear interrupt
>>> regardless. This is suspicious.
>> [MA] Ok. I'll clear the irq status only if IRQ_HANDLED.
> 
> It really depends... What if key was pressed but is released before
> interrupt is handled? Do you want to see "spurious IRQ" warnings?
> 
So, what is the proper way to clear the interrupt in this particular case?
>>>> +
>>>> +	/* Enable interrupts */
>>>> +	davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int __init davinci_kp_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct davinci_kp *davinci_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 Keypad Driver\n");
>>>> +
>>>> +	if (!pdata->keymap) {
>>>> +		dev_dbg(dev, "no keymap from pdata\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL);
>>>> +	if(!davinci_kp) {
>>>> +		dev_dbg(dev, "could not allocate memory for private data\n");
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	key_dev = input_allocate_device();
>>>> +	if (!key_dev) {
>>>> +		dev_dbg(dev, "could not allocate input device\n");
>>>> +		ret = -ENOMEM;
>>>> +		goto fail1;
>>>> +	}
>>>> +
>>>> +	platform_set_drvdata(pdev, davinci_kp);
>>>> +
>>>> +	davinci_kp->input = key_dev;
>>>> +
>>>> +	davinci_kp->irq = platform_get_irq(pdev, 0);
>>>> +	if (davinci_kp->irq < 0) {
>>>> +		dev_err(dev, "no keypad irq\n");
>>>> +		ret = davinci_kp->irq;
>>>> +		goto fail2;
>>>> +	}
>>>> +
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!res) {
>>>> +		dev_err(dev, "no mem resource\n");
>>>> +		ret = -ENODEV;
>>> -EINVAL I'd say.
>> [MA] platform_get_resource should fail with -ENODEV.
> 
> If you fail to get platform resource then th eplatform code is set up
> incorrectly, therefore I'd still argue for -EINVAL. It is not as if
> device may not be there because then the platform device would not be
> present at all.
[MA] It is weird since most of the drivers use -ENODEV. I addressed the Sergei 
comments in this sence:

"
 > What are the proper error codes when platform_get_resource,

    -ENODEV.

 > request_mem_region

    -EBUSY.

 > and ioremap functions fail?.

    -ENOMEM.
"

> 
>>>> +		goto fail2;
>>>> +	}
>>>> +
>>>> +	davinci_kp->pbase = res->start;
>>>> +	davinci_kp->base_size = resource_size(res);
>>>> +
>>>> +	mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name);
>>>> +	if (!mem) {
>>>> +		dev_err(dev, "KEYSCAN registers at %08x are not free\n",
>>>> +			davinci_kp->pbase);
>>>> +		ret = -EBUSY;
>>>> +		goto fail2;
>>>> +	}
>>>> +
>>>> +	davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size);
>>>> +	if (!davinci_kp->base) {
>>>> +		dev_err(dev, "can't ioremap MEM resource.\n");
>>>> +		ret = -ENOMEM;
>>>> +		goto fail3;
>>>> +	}
>>>> +
>>>> +	/* Enable auto repeat feature of Linux input subsystem */
>>>> +	if (pdata->rep)
>>>> +		__set_bit(EV_REP, key_dev->evbit);
>>>> +
>>>> +	/* Setup input device */
>>>> +	__set_bit(EV_KEY, key_dev->evbit);
>>>> +
>>>> +	/* Setup the keymap */
>>>> +	davinci_kp->pdata = pdata;
>>>> +
>>>> +	for (i = 0; i < davinci_kp->pdata->keymapsize; i++)
>>>> +		__set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit);
>>>> +
>>>> +	key_dev->name = "davinci_keypad";
>>>> +	key_dev->phys = "davinci_keypad/input0";
>>>> +	key_dev->dev.parent = &pdev->dev;
>>>> +	key_dev->id.bustype = BUS_HOST;
>>>> +	key_dev->id.vendor = 0x0001;
>>>> +	key_dev->id.product = 0x0001;
>>>> +	key_dev->id.version = 0x0001;
>>>> +	key_dev->keycode = davinci_kp->pdata->keymap;
>>> Please kopy keymap into the davinci_kp stucture and use it so that
>>> platform data is never changed and can be declared const.
>> Do you mean something like this?
>>
>> struct davinci_kp {
>> 	...
>> 	const int	*keymap;
>> 	...
>> };
>>
> 
> More like:
> 
> struct davinci_kp {
> 	...
> 	unsgned char keymap[];
> 	...
> };
> 
[MA] Ok.
> and then you copy keymap from platform data into device-private
> structure so it can be safely changed via EVIOCSKEYCODE without
> affecting platform data. Also, if you reload the module, the keymap with
> be restored to its original state. The platform data may also be marked
> as const.
[MA] you mean add const in the plaform data of the davinci_kp structure.
> 
> If max size of keymap is not known then you may need to allocate it
> dynamically.
> 
>>>> +	key_dev->keycodesize = sizeof(unsigned int);
>>> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short.
>> [MA] Ok.
>>>> +	key_dev->keycodemax = davinci_kp->pdata->keymapsize;
>>>> +
>>>> +	ret = input_register_device(davinci_kp->input);
>>>> +	if (ret < 0) {
>>>> +		dev_err(dev, "unable to register DaVinci keypad device\n");
>>>> +		goto fail4;
>>>> +	}
>>>> +
>>>> +	ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED,
>>>> +                         "davinci_keypad", davinci_kp);
>>>> +	if (ret < 0) {
>>>> +		dev_err(dev, "unable to register DaVinci keypad Interrupt\n");
>>>> +		goto fail5;
>>>> +	}
>>>> +
>>>> +	davinci_kp_initialize(davinci_kp);
>>>> +
>>>> +	return 0;
>>>> +fail5:
>>>> +	input_unregister_device(davinci_kp->input);
>>>> +	key_dev = NULL;
>>>> +fail4:
>>>> +	iounmap(davinci_kp->base);
>>>> +fail3:
>>>> +	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
>>>> +fail2:
>>>> +	input_free_device(key_dev);
>>>> +fail1:
>>>> +	kfree(davinci_kp);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int __exit davinci_kp_remove(struct platform_device *pdev)
>>> __devexit?
>> [MA] According to comments from David Brownell to the first version of 
>> this patch the __exit should be used.
>>
>> " - Use platform_driver_probe() and __exit/__exit_p();
>>    there's no point in keeping that code around in
>>    typical configs, it'd just waste memory. "
> 
> I am afraid David is wrong here. Even when we register driver with
> platform_driver_probe() we still have "unbind" attribute in sysfs which
> may be used to unbind the device from driver. If code is __exit then
> such attempts will cause oops.
[MA] Let's wait here for David response.
> 
>>>> +{
>>>> +	struct davinci_kp *davinci_kp = platform_get_drvdata(pdev);
>>>> +
>>>> +	free_irq(davinci_kp->irq, davinci_kp);
>>>> +
>>>> +	input_unregister_device(davinci_kp->input);
>>>> +
>>>> +	iounmap(davinci_kp->base);
>>>> +	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
>>>> +
>>>> +	platform_set_drvdata(pdev, NULL);
>>>> +
>>>> +	kfree(davinci_kp);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver davinci_kp_driver = {
>>>> +	.driver = {
>>>> +			.name = "davinci_keypad",
>>>> +			.owner = THIS_MODULE,
>>>> +		},
>>>> +	.remove = __exit_p(davinci_kp_remove),
>>> __devexit_p(). I think you can still unbind the device even if you use
>>> platform_driver_probe.
>> [MA] Same.
> 
> Right, see above.
> 
>>>> +};
>>>> +
>>>> +static int __init davinci_kp_init(void)
>>>> +{
>>>> +	return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
>>>> +}
>>>> +module_init(davinci_kp_init);
>>>> +
>>>> +static void __exit davinci_kp_exit(void)
>>>> +{
>>>> +	platform_driver_unregister(&davinci_kp_driver);
>>>> +}
>>>> +module_exit(davinci_kp_exit);
>>>> +
>>>> +MODULE_AUTHOR("Miguel Aguilar");
>>>> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver");
>>>> +MODULE_LICENSE("GPL");
>>>> -- 
>>>> 1.6.0.4
>>>>
>>>> --
>>>> 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
>>
> 

Thanks,

Miguel Aguilar
--
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
miguel.aguilar@ridgerun.com Sept. 23, 2009, 5:25 p.m. UTC | #5
Miguel Aguilar wrote:
> Dmitry,
> 
> Dmitry Torokhov wrote:
>> Hi Miguel,
>>
>> [Adding David to CC for the __devexit/__exit discussion]
>>
>> On Wed, Sep 23, 2009 at 08:52:40AM -0600, Miguel Aguilar wrote:
>>> Dmitry,
>>>
>>> Dmitry Torokhov wrote:
>>>> Hi Miguel,
>>>>
>>>> On Tue, Sep 22, 2009 at 03:27:30PM -0600, 
>>>> miguel.aguilar@ridgerun.com wrote:
>>>>> From: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>>>>
>>>>> Adds the driver for enabling keypad support for DaVinci platforms.
>>>>>
>>>>> DM365 is the only platform that uses this driver at the moment.
>>>>>
>>>>> This driver was tested on DM365 EVM rev C.
>>>>>
>>>>> Signed-off-by: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>>>> ---
>>>>>  arch/arm/configs/davinci_all_defconfig      |    1 +
>>>>>  arch/arm/mach-davinci/include/mach/keypad.h |   35 +++
>>>>>  drivers/input/keyboard/Kconfig              |    7 +
>>>>>  drivers/input/keyboard/Makefile             |    1 +
>>>>>  drivers/input/keyboard/davinci_keypad.c     |  319 
>>>>> +++++++++++++++++++++++++++
>>>>>  5 files changed, 363 insertions(+), 0 deletions(-)
>>>>>  create mode 100644 arch/arm/mach-davinci/include/mach/keypad.h
>>>>>  create mode 100644 drivers/input/keyboard/davinci_keypad.c
>>>>>
>>>>> diff --git a/arch/arm/configs/davinci_all_defconfig 
>>>>> b/arch/arm/configs/davinci_all_defconfig
>>>>> index ec63c15..e994c83 100644
>>>>> --- a/arch/arm/configs/davinci_all_defconfig
>>>>> +++ b/arch/arm/configs/davinci_all_defconfig
>>>>> @@ -763,6 +763,7 @@ CONFIG_KEYBOARD_GPIO=y
>>>>>  # CONFIG_KEYBOARD_STOWAWAY is not set
>>>>>  # CONFIG_KEYBOARD_SUNKBD is not set
>>>>>  CONFIG_KEYBOARD_XTKBD=m
>>>>> +CONFIG_KEYBOARD_DAVINCI_DM365=m
>>>>>  # CONFIG_INPUT_MOUSE is not set
>>>>>  # CONFIG_INPUT_JOYSTICK is not set
>>>>>  # CONFIG_INPUT_TABLET is not set
>>>>
>>>> If you want to merge the driver through input tree the defconfig chunk
>>>> has to go elsewhere.
>>> [MA] So, Where it should go?
>>
>> In a separate patch submitted to the person maintaining defconfig for
>> the arch in question once driver is in mainline.
> 
> [MA] Ok I see.
>>
>>>>> diff --git a/arch/arm/mach-davinci/include/mach/keypad.h 
>>>>> b/arch/arm/mach-davinci/include/mach/keypad.h
>>>>> new file mode 100644
>>>>> index 0000000..922d20e
>>>>> --- /dev/null
>>>>> +++ b/arch/arm/mach-davinci/include/mach/keypad.h
>>>>> @@ -0,0 +1,35 @@
>>>>> +/*
>>>>> + * Copyright (C) 2009 Texas Instruments, Inc
>>>>> + *
>>>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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
>>>>> + */
>>>>> +
>>>>> +#ifndef DAVINCI_KEYPAD_H
>>>>> +#define DAVINCI_KEYPAD_H
>>>>> +
>>>>> +#include <linux/io.h>
>>>>> +
>>>>> +struct davinci_kp_platform_data {
>>>>> +    int    *keymap;
>>>>> +    u32    keymapsize;
>>>>> +    u32    rep:1;
>>>>> +    u32    strobe;
>>>>> +    u32    interval;
>>>>> +};
>>>>> +
>>>>> +#endif
>>>>> +
>>>>> diff --git a/drivers/input/keyboard/Kconfig 
>>>>> b/drivers/input/keyboard/Kconfig
>>>>> index a6b989a..b6b9517 100644
>>>>> --- a/drivers/input/keyboard/Kconfig
>>>>> +++ b/drivers/input/keyboard/Kconfig
>>>>> @@ -361,4 +361,11 @@ config KEYBOARD_XTKBD
>>>>>        To compile this driver as a module, choose M here: the
>>>>>        module will be called xtkbd.
>>>>>  +config KEYBOARD_DAVINCI
>>>>> +    tristate "TI DaVinci Keypad"
>>>>> +    depends on ARCH_DAVINCI_DM365
>>>>> +    help
>>>>> +      Say Y to enable keypad module support for the TI DaVinci
>>>>> +      platforms (DM365)
>>>>> +
>>>> "To compile this driver as a module..."
>>> [MA] Ok.
>>>>>  endif
>>>>> diff --git a/drivers/input/keyboard/Makefile 
>>>>> b/drivers/input/keyboard/Makefile
>>>>> index b5b5eae..0b0274e 100644
>>>>> --- a/drivers/input/keyboard/Makefile
>>>>> +++ b/drivers/input/keyboard/Makefile
>>>>> @@ -31,3 +31,4 @@ obj-$(CONFIG_KEYBOARD_STOWAWAY)        += stowaway.o
>>>>>  obj-$(CONFIG_KEYBOARD_SUNKBD)        += sunkbd.o
>>>>>  obj-$(CONFIG_KEYBOARD_TOSA)        += tosakbd.o
>>>>>  obj-$(CONFIG_KEYBOARD_XTKBD)        += xtkbd.o
>>>>> +obj-$(CONFIG_KEYBOARD_DAVINCI)        += davinci_keypad.o
>>>>> diff --git a/drivers/input/keyboard/davinci_keypad.c 
>>>>> b/drivers/input/keyboard/davinci_keypad.c
>>>>> new file mode 100644
>>>>> index 0000000..6f0e793
>>>>> --- /dev/null
>>>>> +++ b/drivers/input/keyboard/davinci_keypad.c
>>>>> @@ -0,0 +1,319 @@
>>>>> +/*
>>>>> + * DaVinci Keypad Driver for TI platforms
>>>>> + *
>>>>> + * Copyright (C) 2009 Texas Instruments, Inc
>>>>> + *
>>>>> + * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com>
>>>>> + *
>>>>> + * Intial Code: Sandeep Paulraj <s-paulraj@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/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 DAVINCI_KEYPAD_KEYCTRL        0x0000
>>>>> +#define DAVINCI_KEYPAD_INTENA        0x0004
>>>>> +#define DAVINCI_KEYPAD_INTFLAG        0x0008
>>>>> +#define DAVINCI_KEYPAD_INTCLR        0x000c
>>>>> +#define DAVINCI_KEYPAD_STRBWIDTH    0x0010
>>>>> +#define DAVINCI_KEYPAD_INTERVAL        0x0014
>>>>> +#define DAVINCI_KEYPAD_CONTTIME        0x0018
>>>>> +#define DAVINCI_KEYPAD_CURRENTST    0x001c
>>>>> +#define DAVINCI_KEYPAD_PREVSTATE    0x0020
>>>>> +#define DAVINCI_KEYPAD_EMUCTRL        0x0024
>>>>> +#define DAVINCI_KEYPAD_IODFTCTRL    0x002c
>>>>> +
>>>>> +/* Key Control Register (KEYCTRL) */
>>>>> +#define DAVINCI_KEYPAD_KEYEN        0x00000001
>>>>> +#define DAVINCI_KEYPAD_PREVMODE        0x00000002
>>>>> +#define DAVINCI_KEYPAD_CHATOFF        0x00000004
>>>>> +#define DAVINCI_KEYPAD_AUTODET        0x00000008
>>>>> +#define DAVINCI_KEYPAD_SCANMODE        0x00000010
>>>>> +#define DAVINCI_KEYPAD_OUTTYPE        0x00000020
>>>>> +#define DAVINCI_KEYPAD_4X4        0x00000040
>>>>> +
>>>>> +/* Masks for the interrupts */
>>>>> +#define DAVINCI_KEYPAD_INT_CONT        0x00000008
>>>>> +#define DAVINCI_KEYPAD_INT_OFF        0x00000004
>>>>> +#define DAVINCI_KEYPAD_INT_ON        0x00000002
>>>>> +#define DAVINCI_KEYPAD_INT_CHANGE    0x00000001
>>>>> +#define DAVINCI_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 davinci_kp_write(struct davinci_kp *davinci_kp, u32 
>>>>> val, u32 addr)
>>>>> +{
>>>>> +    u32 base = (u32)davinci_kp->base;
>>>>> +
>>>>> +    __raw_writel(val,(u32 *)(base + addr));
>>>>> +}
>>>>> +
>>>>> +static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr)
>>>>> +{
>>>>> +    u32 base = (u32)davinci_kp->base;
>>>>> +
>>>>> +    return __raw_readl((u32 *)(base + addr));
>>>>> +}
>>>>> +
>>>>> +/* Initializing the kp Module */
>>>>> +static void davinci_kp_initialize(struct davinci_kp *davinci_kp)
>>>>> +{
>>>>> +    u32 strobe = davinci_kp->pdata->strobe;
>>>>> +    u32 interval = davinci_kp->pdata->interval;
>>>>> +
>>>>> +    /* Enable all interrupts */
>>>>> +    davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, 
>>>>> DAVINCI_KEYPAD_INTENA);
>>>>> +
>>>>> +    /* Clear interrupts if any */
>>>>> +    davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, 
>>>>> DAVINCI_KEYPAD_INTCLR);
>>>>> +
>>>>> +    /* Setup the scan period = strobe + interval */
>>>>> +    davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH);
>>>>> +    davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL);
>>>>> +    davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME);
>>>>> +
>>>>> +    /* Enable Keyscan module and enable */
>>>>> +    davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | 
>>>>> DAVINCI_KEYPAD_KEYEN,
>>>>> +            DAVINCI_KEYPAD_KEYCTRL);
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id)
>>>>> +{
>>>>> +    struct davinci_kp *davinci_kp = dev_id;
>>>>> +    struct device *dev = &davinci_kp->input->dev;
>>>>> +    int *keymap = davinci_kp->pdata->keymap;
>>>>> +        u32 prev_status, new_status, changed, position;
>>>>> +    int keycode = KEY_UNKNOWN;
>>>>> +    int ret = IRQ_NONE;
>>>>> +
>>>>> +    /* Disable interrupt */
>>>>> +    davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA);
>>>>> +
>>>>> +    /* Reading previous and new status of the keypad */
>>>>> +    prev_status = davinci_kp_read(davinci_kp, 
>>>>> DAVINCI_KEYPAD_PREVSTATE);
>>>>> +    new_status = davinci_kp_read(davinci_kp, 
>>>>> DAVINCI_KEYPAD_CURRENTST);
>>>>> +
>>>>> +    changed = prev_status ^ new_status;
>>>>> +    position = ffs(changed) - 1;
>>>>> +
>>>>> +    if (changed) {
>>>> Can there be several buttons that change status at once?
>>> [MA] It is not suppose to change several buttons at once.
>>
>> "Not supposed to happen" vs. "it can not happen, even in theory"?
> [MA]According with the TI documentation the scan process and the way of 
> how the IRQ is handle, expects one key at time.
>>
>>>>> +        keycode = keymap[position];
>>>>> +        if((new_status >> position) & 0x1) {
>>>> bool release = (new_status >> position) & 0x1;
>>>> input_report_key(davinci_kp->input, keycode, !release);
>>>> dev_dbg(dev, "davinci_keypad: key %d %s\n",
>>>>     keycode, release ? "released" : "pressed");
>>>>
>>> [MA] Ok I'll try that.
>>>> is shorter.
>>>>
>>>>> +            /* Report release */
>>>>> +            dev_dbg(dev, "davinci_keypad: key %d released\n",
>>>>> +                    keycode);
>>>>> +            input_report_key(davinci_kp->input, keycode, 0);
>>>>> +        } else {
>>>>> +            /* Report press */
>>>>> +            dev_dbg(dev, "davinci_keypad: key %d pressed\n",
>>>>> +                    keycode);
>>>>> +            input_report_key(davinci_kp->input, keycode, 1);
>>>>> +        }
>>>>> +        input_sync(davinci_kp->input);
>>>>> +        ret = IRQ_HANDLED;
>>>>> +    }
>>>>> +
>>>>> +    /* Clearing interrupt */
>>>>> +    davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, 
>>>>> DAVINCI_KEYPAD_INTCLR);
>>>> You return IRQ_HANDLED only if keypad state changed but clear interrupt
>>>> regardless. This is suspicious.
>>> [MA] Ok. I'll clear the irq status only if IRQ_HANDLED.
>>
>> It really depends... What if key was pressed but is released before
>> interrupt is handled? Do you want to see "spurious IRQ" warnings?
>>
> So, what is the proper way to clear the interrupt in this particular case?
>>>>> +
>>>>> +    /* Enable interrupts */
>>>>> +    davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int __init davinci_kp_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct davinci_kp *davinci_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 Keypad Driver\n");
>>>>> +
>>>>> +    if (!pdata->keymap) {
>>>>> +        dev_dbg(dev, "no keymap from pdata\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL);
>>>>> +    if(!davinci_kp) {
>>>>> +        dev_dbg(dev, "could not allocate memory for private data\n");
>>>>> +        return -ENOMEM;
>>>>> +    }
>>>>> +
>>>>> +    key_dev = input_allocate_device();
>>>>> +    if (!key_dev) {
>>>>> +        dev_dbg(dev, "could not allocate input device\n");
>>>>> +        ret = -ENOMEM;
>>>>> +        goto fail1;
>>>>> +    }
>>>>> +
>>>>> +    platform_set_drvdata(pdev, davinci_kp);
>>>>> +
>>>>> +    davinci_kp->input = key_dev;
>>>>> +
>>>>> +    davinci_kp->irq = platform_get_irq(pdev, 0);
>>>>> +    if (davinci_kp->irq < 0) {
>>>>> +        dev_err(dev, "no keypad irq\n");
>>>>> +        ret = davinci_kp->irq;
>>>>> +        goto fail2;
>>>>> +    }
>>>>> +
>>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> +    if (!res) {
>>>>> +        dev_err(dev, "no mem resource\n");
>>>>> +        ret = -ENODEV;
>>>> -EINVAL I'd say.
>>> [MA] platform_get_resource should fail with -ENODEV.
>>
>> If you fail to get platform resource then th eplatform code is set up
>> incorrectly, therefore I'd still argue for -EINVAL. It is not as if
>> device may not be there because then the platform device would not be
>> present at all.
> [MA] It is weird since most of the drivers use -ENODEV. I addressed the 
> Sergei comments in this sence:
> 
> "
>  > What are the proper error codes when platform_get_resource,
> 
>    -ENODEV.
> 
>  > request_mem_region
> 
>    -EBUSY.
> 
>  > and ioremap functions fail?.
> 
>    -ENOMEM.
> "
> 
>>
>>>>> +        goto fail2;
>>>>> +    }
>>>>> +
>>>>> +    davinci_kp->pbase = res->start;
>>>>> +    davinci_kp->base_size = resource_size(res);
>>>>> +
>>>>> +    mem = request_mem_region(davinci_kp->pbase, 
>>>>> davinci_kp->base_size, pdev->name);
>>>>> +    if (!mem) {
>>>>> +        dev_err(dev, "KEYSCAN registers at %08x are not free\n",
>>>>> +            davinci_kp->pbase);
>>>>> +        ret = -EBUSY;
>>>>> +        goto fail2;
>>>>> +    }
>>>>> +
>>>>> +    davinci_kp->base = ioremap(davinci_kp->pbase, 
>>>>> davinci_kp->base_size);
>>>>> +    if (!davinci_kp->base) {
>>>>> +        dev_err(dev, "can't ioremap MEM resource.\n");
>>>>> +        ret = -ENOMEM;
>>>>> +        goto fail3;
>>>>> +    }
>>>>> +
>>>>> +    /* Enable auto repeat feature of Linux input subsystem */
>>>>> +    if (pdata->rep)
>>>>> +        __set_bit(EV_REP, key_dev->evbit);
>>>>> +
>>>>> +    /* Setup input device */
>>>>> +    __set_bit(EV_KEY, key_dev->evbit);
>>>>> +
>>>>> +    /* Setup the keymap */
>>>>> +    davinci_kp->pdata = pdata;
>>>>> +
>>>>> +    for (i = 0; i < davinci_kp->pdata->keymapsize; i++)
>>>>> +        __set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit);
>>>>> +
>>>>> +    key_dev->name = "davinci_keypad";
>>>>> +    key_dev->phys = "davinci_keypad/input0";
>>>>> +    key_dev->dev.parent = &pdev->dev;
>>>>> +    key_dev->id.bustype = BUS_HOST;
>>>>> +    key_dev->id.vendor = 0x0001;
>>>>> +    key_dev->id.product = 0x0001;
>>>>> +    key_dev->id.version = 0x0001;
>>>>> +    key_dev->keycode = davinci_kp->pdata->keymap;
>>>> Please kopy keymap into the davinci_kp stucture and use it so that
>>>> platform data is never changed and can be declared const.
>>> Do you mean something like this?
>>>
>>> struct davinci_kp {
>>>     ...
>>>     const int    *keymap;
>>>     ...
>>> };
>>>
>>
>> More like:
>>
>> struct davinci_kp {
>>     ...
>>     unsgned char keymap[];
>>     ...
>> };
>>
> [MA] Ok.
[MA] Why usigned char with no pointer and not u32 as most of the keypad driver 
as defined?
>> and then you copy keymap from platform data into device-private
>> structure so it can be safely changed via EVIOCSKEYCODE without
>> affecting platform data. Also, if you reload the module, the keymap with
>> be restored to its original state. The platform data may also be marked
>> as const.
> [MA] you mean add const in the plaform data of the davinci_kp structure.
>>
>> If max size of keymap is not known then you may need to allocate it
>> dynamically.
>>
>>>>> +    key_dev->keycodesize = sizeof(unsigned int);
>>>> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short.
>>> [MA] Ok.
>>>>> +    key_dev->keycodemax = davinci_kp->pdata->keymapsize;
>>>>> +
>>>>> +    ret = input_register_device(davinci_kp->input);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "unable to register DaVinci keypad device\n");
>>>>> +        goto fail4;
>>>>> +    }
>>>>> +
>>>>> +    ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, 
>>>>> IRQF_DISABLED,
>>>>> +                         "davinci_keypad", davinci_kp);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "unable to register DaVinci keypad 
>>>>> Interrupt\n");
>>>>> +        goto fail5;
>>>>> +    }
>>>>> +
>>>>> +    davinci_kp_initialize(davinci_kp);
>>>>> +
>>>>> +    return 0;
>>>>> +fail5:
>>>>> +    input_unregister_device(davinci_kp->input);
>>>>> +    key_dev = NULL;
>>>>> +fail4:
>>>>> +    iounmap(davinci_kp->base);
>>>>> +fail3:
>>>>> +    release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
>>>>> +fail2:
>>>>> +    input_free_device(key_dev);
>>>>> +fail1:
>>>>> +    kfree(davinci_kp);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int __exit davinci_kp_remove(struct platform_device *pdev)
>>>> __devexit?
>>> [MA] According to comments from David Brownell to the first version 
>>> of this patch the __exit should be used.
>>>
>>> " - Use platform_driver_probe() and __exit/__exit_p();
>>>    there's no point in keeping that code around in
>>>    typical configs, it'd just waste memory. "
>>
>> I am afraid David is wrong here. Even when we register driver with
>> platform_driver_probe() we still have "unbind" attribute in sysfs which
>> may be used to unbind the device from driver. If code is __exit then
>> such attempts will cause oops.
> [MA] Let's wait here for David response.
>>
>>>>> +{
>>>>> +    struct davinci_kp *davinci_kp = platform_get_drvdata(pdev);
>>>>> +
>>>>> +    free_irq(davinci_kp->irq, davinci_kp);
>>>>> +
>>>>> +    input_unregister_device(davinci_kp->input);
>>>>> +
>>>>> +    iounmap(davinci_kp->base);
>>>>> +    release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
>>>>> +
>>>>> +    platform_set_drvdata(pdev, NULL);
>>>>> +
>>>>> +    kfree(davinci_kp);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static struct platform_driver davinci_kp_driver = {
>>>>> +    .driver = {
>>>>> +            .name = "davinci_keypad",
>>>>> +            .owner = THIS_MODULE,
>>>>> +        },
>>>>> +    .remove = __exit_p(davinci_kp_remove),
>>>> __devexit_p(). I think you can still unbind the device even if you use
>>>> platform_driver_probe.
>>> [MA] Same.
>>
>> Right, see above.
>>
>>>>> +};
>>>>> +
>>>>> +static int __init davinci_kp_init(void)
>>>>> +{
>>>>> +    return platform_driver_probe(&davinci_kp_driver, 
>>>>> davinci_kp_probe);
>>>>> +}
>>>>> +module_init(davinci_kp_init);
>>>>> +
>>>>> +static void __exit davinci_kp_exit(void)
>>>>> +{
>>>>> +    platform_driver_unregister(&davinci_kp_driver);
>>>>> +}
>>>>> +module_exit(davinci_kp_exit);
>>>>> +
>>>>> +MODULE_AUTHOR("Miguel Aguilar");
>>>>> +MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver");
>>>>> +MODULE_LICENSE("GPL");
>>>>> -- 
>>>>> 1.6.0.4
>>>>>
>>>>> -- 
>>>>> 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
>>>
>>
> 
> Thanks,
> 
> Miguel Aguilar
> -- 
> 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



--
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
Dmitry Torokhov Sept. 23, 2009, 5:41 p.m. UTC | #6
On Wed, Sep 23, 2009 at 11:25:59AM -0600, Miguel Aguilar wrote:
>>>>> Please kopy keymap into the davinci_kp stucture and use it so that
>>>>> platform data is never changed and can be declared const.
>>>> Do you mean something like this?
>>>>
>>>> struct davinci_kp {
>>>>     ...
>>>>     const int    *keymap;
>>>>     ...
>>>> };
>>>>
>>>
>>> More like:
>>>
>>> struct davinci_kp {
>>>     ...
>>>     unsgned char keymap[];
>>>     ...
>>> };
>>>
>> [MA] Ok.
> [MA] Why usigned char with no pointer and not u32 as most of the keypad 
> driver as defined?

Sorry, meant to say "unsigned short keymap[...]", we not going to have
more than 64K keycodes. You need to fill the array dimension, if the
size is not known before hand (and if upper bound is too jigh to always
allocate max) then you'll have to allocate it separately. Hm, well, if
you make it the last element of the davinci_kp structure you can alwqays
allocate the needed amount of memory, like this:

struct davinci_kp {
	...
	...
	unsigned short keymap[];
};

...

kp = kzalloc(sizeof(struct davinci_kp) +
			sizeof(unsigned short) * pdata->keymap_size,
	     GFP_KERNEL);
David Brownell Sept. 23, 2009, 5:51 p.m. UTC | #7
On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> >> __devexit?
> > [MA] According to comments from David Brownell to the first version of 
> > this patch the __exit should be used.
> >
> > " - Use platform_driver_probe() and __exit/__exit_p();
> >    there's no point in keeping that code around in
> >    typical configs, it'd just waste memory. "
> 
> I am afraid David is wrong here.

No, you're just pointing out a bug introduced in some
unrelated code.  Such bugs happen when folk ignore the
code bloat issues.  (And didn't Linus recently point
out how such code bloat is becoming an issue?)


> Even when we register driver with 
> platform_driver_probe() we still have "unbind" attribute in sysfs which
> may be used to unbind the device from driver. If code is __exit then
> such attempts will cause oops.

That would be a bug in the unbind() code, which doesn't
currently recognize that not every driver or bus supports
hotplugging.  It should probably check for a null release()
pointer in the driver, and politely fail in that case.

That's just about the only place that a third party
(neither the driver nor its hotplug-aware bus framework)
will try to decouple device and driver ... and it's doing
it wrong.  So it's unlikely such bugs will be elsewhere.

It's *ALWAYS* been legit to have a NULL pointer in the
remove() methods.  That's why the __exit_p() -- or for
hotpluggable drivers, __devexit_p() -- macros exist:
for that particular case.

- Dave
 
--
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
Dmitry Torokhov Sept. 23, 2009, 6:07 p.m. UTC | #8
On Wed, Sep 23, 2009 at 10:51:05AM -0700, David Brownell wrote:
> On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> > >> __devexit?
> > > [MA] According to comments from David Brownell to the first version of 
> > > this patch the __exit should be used.
> > >
> > > " - Use platform_driver_probe() and __exit/__exit_p();
> > >    there's no point in keeping that code around in
> > >    typical configs, it'd just waste memory. "
> > 
> > I am afraid David is wrong here.
> 
> No, you're just pointing out a bug introduced in some
> unrelated code.

From the very version of the patch platform_driver_probe() only ensured
that probe() would not be called after module inialization is done, it
never made any guarantees about removing devices. Hmm, let's add Greg as
well here.

>  Such bugs happen when folk ignore the
> code bloat issues.  (And didn't Linus recently point
> out how such code bloat is becoming an issue?)
> 
> 
> > Even when we register driver with 
> > platform_driver_probe() we still have "unbind" attribute in sysfs which
> > may be used to unbind the device from driver. If code is __exit then
> > such attempts will cause oops.
> 
> That would be a bug in the unbind() code, which doesn't
> currently recognize that not every driver or bus supports
> hotplugging.  It should probably check for a null release()
> pointer in the driver, and politely fail in that case.
> 

NULL release (as well as NULL probe) don't mean what you think they do.

> That's just about the only place that a third party
> (neither the driver nor its hotplug-aware bus framework)
> will try to decouple device and driver ... and it's doing
> it wrong.  So it's unlikely such bugs will be elsewhere.
> 

No, it does exactly what it is supposed to do. You may argue that such
facility is not needed but this is different. I'd argue that sometimes
you do need to shut off a device even if it is not normally
hot-pluggrable. Plus most of the blat is coming from probe() functions,
remove()s are fairly small.

> It's *ALWAYS* been legit to have a NULL pointer in the
> remove() methods.

Which means "device does not need any special actions for unbinding",
not that device can not be unbound. The same with probe(): NULL does not
mean that device can not be bound but rather that it does not need any
specal processing during binding.

>  That's why the __exit_p() -- or for
> hotpluggable drivers, __devexit_p() -- macros exist:
> for that particular case.

Huh? They are here so that the linker/module loader can discard them if
they only referenced via driver pointers and we know they not going to
be called.
miguel.aguilar@ridgerun.com Sept. 23, 2009, 6:15 p.m. UTC | #9
Dmitry Torokhov wrote:
> On Wed, Sep 23, 2009 at 11:25:59AM -0600, Miguel Aguilar wrote:
>>>>>> Please kopy keymap into the davinci_kp stucture and use it so that
>>>>>> platform data is never changed and can be declared const.
>>>>> Do you mean something like this?
>>>>>
>>>>> struct davinci_kp {
>>>>>     ...
>>>>>     const int    *keymap;
>>>>>     ...
>>>>> };
>>>>>
>>>> More like:
>>>>
>>>> struct davinci_kp {
>>>>     ...
>>>>     unsgned char keymap[];
>>>>     ...
>>>> };
>>>>
>>> [MA] Ok.
>> [MA] Why usigned char with no pointer and not u32 as most of the keypad 
>> driver as defined?
> 
> Sorry, meant to say "unsigned short keymap[...]", we not going to have
> more than 64K keycodes. You need to fill the array dimension, if the
> size is not known before hand (and if upper bound is too jigh to always
> allocate max) then you'll have to allocate it separately. Hm, well, if
> you make it the last element of the davinci_kp structure you can alwqays
> allocate the needed amount of memory, like this:
> 
> struct davinci_kp {
> 	...
> 	...
> 	unsigned short keymap[];
> };
> 
> ...
> 
> kp = kzalloc(sizeof(struct davinci_kp) +
> 			sizeof(unsigned short) * pdata->keymap_size,
> 	     GFP_KERNEL);
> 

Why do I need to use keymap[]; instead of *keymap? If I use keypad[] how is the 
proper way to assign the platform keymap to the private keymap of the driver?
--
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
Dmitry Torokhov Sept. 23, 2009, 6:19 p.m. UTC | #10
On Wed, Sep 23, 2009 at 12:15:24PM -0600, Miguel Aguilar wrote:
> Dmitry Torokhov wrote:
>> On Wed, Sep 23, 2009 at 11:25:59AM -0600, Miguel Aguilar wrote:
>>>>>>> Please kopy keymap into the davinci_kp stucture and use it so that
>>>>>>> platform data is never changed and can be declared const.
>>>>>> Do you mean something like this?
>>>>>>
>>>>>> struct davinci_kp {
>>>>>>     ...
>>>>>>     const int    *keymap;
>>>>>>     ...
>>>>>> };
>>>>>>
>>>>> More like:
>>>>>
>>>>> struct davinci_kp {
>>>>>     ...
>>>>>     unsgned char keymap[];
>>>>>     ...
>>>>> };
>>>>>
>>>> [MA] Ok.
>>> [MA] Why usigned char with no pointer and not u32 as most of the 
>>> keypad driver as defined?
>>
>> Sorry, meant to say "unsigned short keymap[...]", we not going to have
>> more than 64K keycodes. You need to fill the array dimension, if the
>> size is not known before hand (and if upper bound is too jigh to always
>> allocate max) then you'll have to allocate it separately. Hm, well, if
>> you make it the last element of the davinci_kp structure you can alwqays
>> allocate the needed amount of memory, like this:
>>
>> struct davinci_kp {
>> 	...
>> 	...
>> 	unsigned short keymap[];
>> };
>>
>> ...
>>
>> kp = kzalloc(sizeof(struct davinci_kp) +
>> 			sizeof(unsigned short) * pdata->keymap_size,
>> 	     GFP_KERNEL);
>>
>
> Why do I need to use keymap[]; instead of *keymap? If I use keypad[] how 
> is the proper way to assign the platform keymap to the private keymap of 
> the driver?

memcpy()
David Brownell Sept. 23, 2009, 7:29 p.m. UTC | #11
On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> On Wed, Sep 23, 2009 at 10:51:05AM -0700, David Brownell wrote:
> > On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> > > >> __devexit?
> > > > [MA] According to comments from David Brownell to the first version of 
> > > > this patch the __exit should be used.
> > > >
> > > > " - Use platform_driver_probe() and __exit/__exit_p();
> > > >    there's no point in keeping that code around in
> > > >    typical configs, it'd just waste memory. "
> > > 
> > > I am afraid David is wrong here.
> > 
> > No, you're just pointing out a bug introduced in some
> > unrelated code.
> 
> From the very version of the patch platform_driver_probe() only ensured
> that probe() would not be called after module inialization is done, it
> never made any guarantees about removing devices. Hmm, let's add Greg as
> well here.

I don't see your point.  If the driver doesn't support
any unbinding mechanism -- a remove() method, converse
of probe() -- then it can't safely be unbound.

And thus, if any code is presuming that *every* driver
can be unbound, it's wrong.

As I said:  bug in other code.

Looking at this a bit more, it seems like there will need
to be some "can this bus remove this driver" check, since
the struct device.remove method is now managed at the bus
level.  Easy enough to do instead of the null check that
I mentioned below.  Provide it for platform bus, and the
main potential trouble spots will be resolved.


> >  Such bugs happen when folk ignore the
> > code bloat issues.  (And didn't Linus recently point
> > out how such code bloat is becoming an issue?)

Though to be fair, it's *always* been an issue for anyone
working with embedded Linux systems.  And it's long been
a frustration that too few "mainline" developers didn't
accept such issues.

 
> > > Even when we register driver with 
> > > platform_driver_probe() we still have "unbind" attribute in sysfs which
> > > may be used to unbind the device from driver. If code is __exit then
> > > such attempts will cause oops.
> > 
> > That would be a bug in the unbind() code, which doesn't
> > currently recognize that not every driver or bus supports
> > hotplugging.  It should probably check for a null release()
> > pointer in the driver, and politely fail in that case.
> > 
> 
> NULL release (as well as NULL probe) don't mean what you think they do.

I could say the same things about how you think, of course,
and probably with just as little accuracy.  Let's not be
needlessly confrontational.


> > That's just about the only place that a third party
> > (neither the driver nor its hotplug-aware bus framework)
> > will try to decouple device and driver ... and it's doing
> > it wrong.  So it's unlikely such bugs will be elsewhere.
> > 
> 
> No, it does exactly what it is supposed to do. You may argue that such
> facility is not needed but this is different. I'd argue that sometimes
> you do need to shut off a device even if it is not normally
> hot-pluggrable. 

Erm, once a driver is freely poking around in the hardware,
and is managing IRQs and other asynch events ... exactly
how do you expect to decouple it from the hardware without
a remove()?  Drivers that *don't* respond asynchronously to
hardware events are an extreme minority; it's not worth
even considering them as the design center.

As a policy choice, there's only one sane one:  unbinding
the driver always requires an active ack from the driver.

Even if that's a trivial/NOP remove() function ... instead
of one which cleanly tears down the hardware event stream;
the driver's handling thereof; and whatever resources the
driver claimed, including links to upper level code in the
operating system (like char/block device nodes and many
other things).


> Plus most of the blat is coming from probe() functions, 
> remove()s are fairly small.

Not always.  Of course, there's still that nasty issue
with code that probe() and remove() need to share ... for
now there's no clean way to have such code be removable.
(Like an "__init_or_exit" section annotation.)  Drivers
have been stuck putting such code in the .text segement,
and thus causing additional driver bloat.


> > It's *ALWAYS* been legit to have a NULL pointer in the
> > remove() methods.
> 
> Which means "device does not need any special actions for unbinding",
> not that device can not be unbound. The same with probe(): NULL does not
> mean that device can not be bound but rather that it does not need any
> specal processing during binding.

I don't know where such a model comes from.  I'll just
say that in quite a few years of driver development I
have *never* come across hardware where "doesn't need
any attention during driver unbind" makes sense.

Recall that any driver has both lower (to hardware) and
upper (to rest of OS) links, and maybe more (to other
drivers) ... and it's a design goal in Linux to have such
links be explicit, so the drivers are more reusable.  So
requiring some hook -- remove() -- before attempting
to break the links internal to the driver model seems
like a fairly obvious requirement.


> >  That's why the __exit_p() -- or for
> > hotpluggable drivers, __devexit_p() -- macros exist:
> > for that particular case.
> 
> Huh? They are here so that the linker/module loader can discard them if
> they only referenced via driver pointers and we know they not going to
> be called.

Finish that thought.  What "exit" section code fits those
categories?  Driver remove() methods ... and not much else.
And for that matter, what else goes into exit sections,
other than such code and module hooks to call it?

That discarding of exit sections has been around in Linux
for an extremely long time, at least on architectures which
lean more towards embedded systems than bloat-is-OK servers.
It's applied to driver remove methods, and to the code which
invokes them via rmmod.

Note that your comments all seem to presume that such
discarding is either useless or should be phased out for
some reason ... but you're not quite making *that*
argument.

- Dave

--
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
Dmitry Torokhov Sept. 23, 2009, 7:51 p.m. UTC | #12
On Wed, Sep 23, 2009 at 12:29:07PM -0700, David Brownell wrote:
> On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> > On Wed, Sep 23, 2009 at 10:51:05AM -0700, David Brownell wrote:
> > > On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> > > > >> __devexit?
> > > > > [MA] According to comments from David Brownell to the first version of 
> > > > > this patch the __exit should be used.
> > > > >
> > > > > " - Use platform_driver_probe() and __exit/__exit_p();
> > > > >    there's no point in keeping that code around in
> > > > >    typical configs, it'd just waste memory. "
> > > > 
> > > > I am afraid David is wrong here.
> > > 
> > > No, you're just pointing out a bug introduced in some
> > > unrelated code.
> > 
> > From the very version of the patch platform_driver_probe() only ensured
> > that probe() would not be called after module inialization is done, it
> > never made any guarantees about removing devices. Hmm, let's add Greg as
> > well here.
> 
> I don't see your point.  If the driver doesn't support
> any unbinding mechanism -- a remove() method, converse
> of probe() -- then it can't safely be unbound.
>

This is one possible design... however you are not talking about the
current Linux kernel but some other OS.
 
> And thus, if any code is presuming that *every* driver
> can be unbound, it's wrong.
> 
> As I said:  bug in other code.

Not an implementation bug, the system behaves as designed. If you don't
agree with the current design - get the patches in that alter it and
_then_ we can change annotation from __devexit to __exit.

> 
> Looking at this a bit more, it seems like there will need
> to be some "can this bus remove this driver" check, since
> the struct device.remove method is now managed at the bus
> level.  Easy enough to do instead of the null check that
> I mentioned below.  Provide it for platform bus, and the
> main potential trouble spots will be resolved.
> 
> 
> > >  Such bugs happen when folk ignore the
> > > code bloat issues.  (And didn't Linus recently point
> > > out how such code bloat is becoming an issue?)
> 
> Though to be fair, it's *always* been an issue for anyone
> working with embedded Linux systems.  And it's long been
> a frustration that too few "mainline" developers didn't
> accept such issues.
> 

You are rambling ;(

>  
> > > > Even when we register driver with 
> > > > platform_driver_probe() we still have "unbind" attribute in sysfs which
> > > > may be used to unbind the device from driver. If code is __exit then
> > > > such attempts will cause oops.
> > > 
> > > That would be a bug in the unbind() code, which doesn't
> > > currently recognize that not every driver or bus supports
> > > hotplugging.  It should probably check for a null release()
> > > pointer in the driver, and politely fail in that case.
> > > 
> > 
> > NULL release (as well as NULL probe) don't mean what you think they do.
> 
> I could say the same things about how you think, of course,
> and probably with just as little accuracy.  Let's not be
> needlessly confrontational.
> 

I am talking about current design of the Linux driver code, as it is
present in mainline and in this particular instance probe() and remove()
do not do what you think they do. You are of course free to submit
patches that would alter the way the system behaves and then you can
make the statement above and I will agree with you ;)

> 
> > > That's just about the only place that a third party
> > > (neither the driver nor its hotplug-aware bus framework)
> > > will try to decouple device and driver ... and it's doing
> > > it wrong.  So it's unlikely such bugs will be elsewhere.
> > > 
> > 
> > No, it does exactly what it is supposed to do. You may argue that such
> > facility is not needed but this is different. I'd argue that sometimes
> > you do need to shut off a device even if it is not normally
> > hot-pluggrable. 
> 
> Erm, once a driver is freely poking around in the hardware,
> and is managing IRQs and other asynch events ... exactly
> how do you expect to decouple it from the hardware without
> a remove()?  Drivers that *don't* respond asynchronously to
> hardware events are an extreme minority; it's not worth
> even considering them as the design center.
> 
> As a policy choice, there's only one sane one:  unbinding
> the driver always requires an active ack from the driver.
> 
> Even if that's a trivial/NOP remove() function ... instead
> of one which cleanly tears down the hardware event stream;
> the driver's handling thereof; and whatever resources the
> driver claimed, including links to upper level code in the
> operating system (like char/block device nodes and many
> other things).


Look, this is all great, you are coming with wonderful new design that
will surely fix all issues embedded developers have here. But until it
is in mainline here is my perspective:

I don't want to merge code that can trivially oops the box. I also don't
want to encourage people to submit such code in the hopes that once
design of the driver core changes their code stops oopsing.

> 
> 
> > Plus most of the blat is coming from probe() functions, 
> > remove()s are fairly small.
> 
> Not always.  Of course, there's still that nasty issue
> with code that probe() and remove() need to share ... for
> now there's no clean way to have such code be removable.
> (Like an "__init_or_exit" section annotation.)  Drivers
> have been stuck putting such code in the .text segement,
> and thus causing additional driver bloat.
> 
> 
> > > It's *ALWAYS* been legit to have a NULL pointer in the
> > > remove() methods.
> > 
> > Which means "device does not need any special actions for unbinding",
> > not that device can not be unbound. The same with probe(): NULL does not
> > mean that device can not be bound but rather that it does not need any
> > specal processing during binding.
> 
> I don't know where such a model comes from.  I'll just
> say that in quite a few years of driver development I
> have *never* come across hardware where "doesn't need
> any attention during driver unbind" makes sense.
> 
> Recall that any driver has both lower (to hardware) and
> upper (to rest of OS) links, and maybe more (to other
> drivers) ... and it's a design goal in Linux to have such
> links be explicit, so the drivers are more reusable.  So
> requiring some hook -- remove() -- before attempting
> to break the links internal to the driver model seems
> like a fairly obvious requirement.
> 
> 
> > >  That's why the __exit_p() -- or for
> > > hotpluggable drivers, __devexit_p() -- macros exist:
> > > for that particular case.
> > 
> > Huh? They are here so that the linker/module loader can discard them if
> > they only referenced via driver pointers and we know they not going to
> > be called.
> 
> Finish that thought.  What "exit" section code fits those
> categories?  Driver remove() methods ... and not much else.
> And for that matter, what else goes into exit sections,
> other than such code and module hooks to call it?
> 
> That discarding of exit sections has been around in Linux
> for an extremely long time, at least on architectures which
> lean more towards embedded systems than bloat-is-OK servers.
> It's applied to driver remove methods, and to the code which
> invokes them via rmmod.
> 
> Note that your comments all seem to presume that such
> discarding is either useless or should be phased out for
> some reason ... but you're not quite making *that*
> argument.
>

Umm, no, I did not say that. You implied that __exit_p and __devexit_p
have something to do with setting remove to NULL but that is not their
main purpose. They are needed so that references are gone, but we could
just as easily be using (void *)1 instead of NULL as substitute value.
David Brownell Sept. 23, 2009, 11:05 p.m. UTC | #13
On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> This is one possible design... however you are not talking about the
> current Linux kernel but some other OS.

What other OS might it be then, which has carried around
that exit section scrubbing mechanism for quite a few years
now, and is distributed through www.kernel.org with labels
such as "Linux 2.6.31" ???


> > And thus, if any code is presuming that *every* driver
> > can be unbound, it's wrong.
> > 
> > As I said:  bug in other code.
> 
> Not an implementation bug, the system behaves as designed.

Yes, absolutely an implementation bug.

At best you can say that there are two "designs" that
are in conflict with each other.  And argue, for some
reason, that the relatively-recently-introduced oops
(OK, mid-2005, so it's been lurking for quite a while)
is "more intended" than the previous safe-no-oops one
(predating mid-2005 by many years).


> > Looking at this a bit more, it seems like there will need
> > to be some "can this bus remove this driver" check, since
> > the struct device.remove method is now managed at the bus
> > level.  Easy enough to do instead of the null check that
> > I mentioned below.  Provide it for platform bus, and the
> > main potential trouble spots will be resolved.
> 
> 		... deletia ...
> 

> I am talking about current design of the Linux driver code, as it is
> present in mainline and in this particular instance probe() and remove()
> do not do what you think they do. 

You're arguing about what it "should" do, and ignoring
all the evidence I've provided.  So I guess "talking"
is right, not "listening" or better yet "discussing".


> Look, this is all great, you are coming with wonderful new design that
> will surely fix all issues embedded developers have here. But until it
> is in mainline here is my perspective:

No, I'm just pointing out how the system has been intended
to behave *FOR MANY YEARS NOW* and you are objecting because
of a bug in some unrelated and newer code.


> > > > 		That's why the __exit_p() -- or for
> > > > hotpluggable drivers, __devexit_p() -- macros exist:
> > > > for that particular case.
> > > 
> > > Huh? They are here so that the linker/module loader can discard them if
> > > they only referenced via driver pointers and we know they not going to
> > > be called.
> > 
> > Finish that thought.  What "exit" section code fits those
> > categories?  Driver remove() methods ... and not much else.
> > And for that matter, what else goes into exit sections,
> > other than such code and module hooks to call it?
> > 
> > That discarding of exit sections has been around in Linux
> > for an extremely long time, at least on architectures which
> > lean more towards embedded systems than bloat-is-OK servers.
> > It's applied to driver remove methods, and to the code which
> > invokes them via rmmod.
> > 
> > Note that your comments all seem to presume that such
> > discarding is either useless or should be phased out for
> > some reason ... but you're not quite making *that*
> > argument.
> >
> 
> Umm, no, I did not say that.

As I said, your comments *presumed* that.  As in,
they don't seem to make a lot of sense otherwise.

If that "discard exit sections" is a longstanding
part of Linux (as it is), then using it in normal
ways wouldn't be the problem ... instead of saying
that it *is* the problem, you'd be digging deeper.


> You implied that __exit_p and __devexit_p 
> have something to do with setting remove to NULL but that is not their
> main purpose. 

It's what they have *done* since quite a few years
before the driver model existed.  Saying that what
they *do* is somehow not their main purpose ... isn't
very useful, as arguments go.


> They are needed so that references are gone, but we could 
> just as easily be using (void *)1 instead of NULL as substitute value.

Except that NULL function pointers are valid for
purposes other than calling through them, and at
least one universal idiom relies on them:  test
pointer against NULL before using.

Where in Linux is there code checking against
indirection through "(void *)1)" instead of NULL??

- Dave

--
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
Dmitry Torokhov Sept. 24, 2009, 5:40 a.m. UTC | #14
On Wed, Sep 23, 2009 at 04:05:43PM -0700, David Brownell wrote:
> On Wednesday 23 September 2009, Dmitry Torokhov wrote:
> > This is one possible design... however you are not talking about the
> > current Linux kernel but some other OS.
> 
> What other OS might it be then, which has carried around
> that exit section scrubbing mechanism for quite a few years
> now, and is distributed through www.kernel.org with labels
> such as "Linux 2.6.31" ???
> 

It can't be it since it does not exibit the behavior you are describing.

> 
> > > And thus, if any code is presuming that *every* driver
> > > can be unbound, it's wrong.
> > > 
> > > As I said:  bug in other code.
> > 
> > Not an implementation bug, the system behaves as designed.
> 
> Yes, absolutely an implementation bug.
> 
> At best you can say that there are two "designs" that
> are in conflict with each other.  And argue, for some
> reason, that the relatively-recently-introduced oops
> (OK, mid-2005, so it's been lurking for quite a while)
> is "more intended" than the previous safe-no-oops one
> (predating mid-2005 by many years).

Driver model was introduced what, 7 years ago? And since then at no
point remove methods() could be __devexit.

> 
> 
> > > Looking at this a bit more, it seems like there will need
> > > to be some "can this bus remove this driver" check, since
> > > the struct device.remove method is now managed at the bus
> > > level.  Easy enough to do instead of the null check that
> > > I mentioned below.  Provide it for platform bus, and the
> > > main potential trouble spots will be resolved.
> > 
> > 		... deletia ...
> > 
> 
> > I am talking about current design of the Linux driver code, as it is
> > present in mainline and in this particular instance probe() and remove()
> > do not do what you think they do. 
> 
> You're arguing about what it "should" do, and ignoring
> all the evidence I've provided.  So I guess "talking"
> is right, not "listening" or better yet "discussing".
>

Ok, then let me tell this once again since you snipped it off:

Until driver model is fixed so that using unbind sysfs attribute does not
cause trouble if devices discard their remove methods I will not accept
or ack drivers that mark their remove() methods as __exit.
miguel.aguilar@ridgerun.com Sept. 24, 2009, 2:59 p.m. UTC | #15
Dmitry,

I addressed your comments but I still have a couple of questions.


>> +CONFIG_KEYBOARD_DAVINCI_DM365=m
>>  # CONFIG_INPUT_MOUSE is not set
>>  # CONFIG_INPUT_JOYSTICK is not set
>>  # CONFIG_INPUT_TABLET is not set
> 
> 
> If you want to merge the driver through input tree the defconfig chunk
> has to go elsewhere.
[MA] Ok, independent patch.


>>  
>> +config KEYBOARD_DAVINCI
>> +	tristate "TI DaVinci Keypad"
>> +	depends on ARCH_DAVINCI_DM365
>> +	help
>> +	  Say Y to enable keypad module support for the TI DaVinci
>> +	  platforms (DM365)
>> +
> 
> "To compile this driver as a module..."
[MA] Comment added.


>> +		keycode = keymap[position];
>> +		if((new_status >> position) & 0x1) {
> 
> bool release = (new_status >> position) & 0x1;
> input_report_key(davinci_kp->input, keycode, !release);
> dev_dbg(dev, "davinci_keypad: key %d %s\n",
> 	keycode, release ? "released" : "pressed");
> 
> is shorter.
> 
>> +			/* Report release */
>> +			dev_dbg(dev, "davinci_keypad: key %d released\n",
>> +				    keycode);
>> +			input_report_key(davinci_kp->input, keycode, 0);
>> +		} else {
>> +			/* Report press */
>> +			dev_dbg(dev, "davinci_keypad: key %d pressed\n",
>> +				    keycode);
>> +			input_report_key(davinci_kp->input, keycode, 1);
>> +		}
>> +		input_sync(davinci_kp->input);
>> +		ret = IRQ_HANDLED;
>> +	}
>> +
>> +	/* Clearing interrupt */
>> +	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
> 
> You return IRQ_HANDLED only if keypad state changed but clear interrupt
> regardless. This is suspicious.
> 
>> +
>> +	/* Enable interrupts */
>> +	davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA);
>> +
>> +	return ret;
>> +}

[MA] This is the current irq function:
static irqreturn_t davinci_ks_interrupt(int irq, void *dev_id)
{
	struct davinci_ks *davinci_ks = dev_id;
	struct device *dev = &davinci_ks->input->dev;
	unsigned short *keymap = davinci_ks->keymap;
	u32 prev_status, new_status, changed, position;
	bool release;
	int keycode = KEY_UNKNOWN;
	int ret = IRQ_NONE;

	/* Disable interrupt */
	davinci_ks_write(davinci_ks, 0x0, DAVINCI_KEYSCAN_INTENA);

	/* Reading previous and new status of the key scan */
	prev_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_PREVSTATE);
	new_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_CURRENTST);

	changed = prev_status ^ new_status;
	position = ffs(changed) - 1;

	if (changed) {
		keycode = keymap[position];
		release = (new_status >> position) & 0x1;
		dev_dbg(dev, "davinci_keyscan: key %d %s\n",
		    keycode, release ? "released" : "pressed");

		input_report_key(davinci_ks->input, keycode, !release);
		input_sync(davinci_ks->input);

		/* Clearing interrupt */
		davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL,
				    DAVINCI_KEYSCAN_INTCLR);

		ret = IRQ_HANDLED;
	}

	/* Enable interrupts */
	davinci_ks_write(davinci_ks, 0x1, DAVINCI_KEYSCAN_INTENA);

	return ret;
}


>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(dev, "no mem resource\n");
>> +		ret = -ENODEV;
> 
> -EINVAL I'd say.
[MA] Ok. -EINVAL


>> +	key_dev->id.vendor = 0x0001;
>> +	key_dev->id.product = 0x0001;
>> +	key_dev->id.version = 0x0001;
>> +	key_dev->keycode = davinci_kp->pdata->keymap;
> 
> Please kopy keymap into the davinci_kp stucture and use it so that
> platform data is never changed and can be declared const.
> 
[MA] keymap copied to private data.

>> +	key_dev->keycodesize = sizeof(unsigned int);
> 
> sizeof(davinci_kp->keymap[0]) is safer. Plus make it unsigned short.
> 
[MA] Now it uses sizeof(davinci_kp->keymap[0])

>> +}
>> +
>> +static int __exit davinci_kp_remove(struct platform_device *pdev)
> 
> __devexit?
>

[MA] So, this will be __devexit


>> +static struct platform_driver davinci_kp_driver = {
>> +	.driver = {
>> +			.name = "davinci_keypad",
>> +			.owner = THIS_MODULE,
>> +		},
>> +	.remove = __exit_p(davinci_kp_remove),
> 
> __devexit_p(). I think you can still unbind the device even if you use
> platform_driver_probe.
> 
[MA] ... and __devexit.


>> +static int __init davinci_kp_init(void)
>> +{
>> +	return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
>> +}
>> +module_init(davinci_kp_init);
[MA] Should I use platform_driver_probe?


>> +static void __exit davinci_kp_exit(void)
>> +{
>> +	platform_driver_unregister(&davinci_kp_driver);
>> +}
>> +module_exit(davinci_kp_exit);
[MA] Is the module exit function __exit or __devexit

Thanks,
Miguel Aguilar
--
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
Dmitry Torokhov Sept. 24, 2009, 4:21 p.m. UTC | #16
On Thu, Sep 24, 2009 at 08:59:56AM -0600, Miguel Aguilar wrote:
> Dmitry,
>
> I addressed your comments but I still have a couple of questions.

Thank you for making the adjustments.

>
> [MA] This is the current irq function:
> static irqreturn_t davinci_ks_interrupt(int irq, void *dev_id)
> {
> 	struct davinci_ks *davinci_ks = dev_id;
> 	struct device *dev = &davinci_ks->input->dev;
> 	unsigned short *keymap = davinci_ks->keymap;
> 	u32 prev_status, new_status, changed, position;
> 	bool release;
> 	int keycode = KEY_UNKNOWN;
> 	int ret = IRQ_NONE;
>
> 	/* Disable interrupt */
> 	davinci_ks_write(davinci_ks, 0x0, DAVINCI_KEYSCAN_INTENA);
>
> 	/* Reading previous and new status of the key scan */
> 	prev_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_PREVSTATE);
> 	new_status = davinci_ks_read(davinci_ks, DAVINCI_KEYSCAN_CURRENTST);
>
> 	changed = prev_status ^ new_status;
> 	position = ffs(changed) - 1;
>
> 	if (changed) {
> 		keycode = keymap[position];
> 		release = (new_status >> position) & 0x1;
> 		dev_dbg(dev, "davinci_keyscan: key %d %s\n",
> 		    keycode, release ? "released" : "pressed");
>
> 		input_report_key(davinci_ks->input, keycode, !release);
> 		input_sync(davinci_ks->input);
>
> 		/* Clearing interrupt */
> 		davinci_ks_write(davinci_ks, DAVINCI_KEYSCAN_INT_ALL,
> 				    DAVINCI_KEYSCAN_INTCLR);
>
> 		ret = IRQ_HANDLED;
> 	}
>
> 	/* Enable interrupts */
> 	davinci_ks_write(davinci_ks, 0x1, DAVINCI_KEYSCAN_INTENA);
>
> 	return ret;

I'd just return IRQ_HANDLED unconditionally and I think you should go
through all bits in changed to ensure that you don't lose key presses
(unless controller never ever reports more than 1 key pressed).

> }
>
>

>
>>> +static int __init davinci_kp_init(void)
>>> +{
>>> +	return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
>>> +}
>>> +module_init(davinci_kp_init);
> [MA] Should I use platform_driver_probe?
>

I don't see why not - you are still saving memory due to discarding
davinci_kp_probe. And if driver core is changed so that unbind is a NOP
for drivers registered with platform_driver_probe I will gladly accept a
follow-up patch to change __devexit to __exit for even more savings.


>>> +static void __exit davinci_kp_exit(void)
>>> +{
>>> +	platform_driver_unregister(&davinci_kp_driver);
>>> +}
>>> +module_exit(davinci_kp_exit);
> [MA] Is the module exit function __exit or __devexit
>

Module ext routines are always __exit (and inits are __init).

Thanks.
diff mbox

Patch

diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
index ec63c15..e994c83 100644
--- a/arch/arm/configs/davinci_all_defconfig
+++ b/arch/arm/configs/davinci_all_defconfig
@@ -763,6 +763,7 @@  CONFIG_KEYBOARD_GPIO=y
 # CONFIG_KEYBOARD_STOWAWAY is not set
 # CONFIG_KEYBOARD_SUNKBD is not set
 CONFIG_KEYBOARD_XTKBD=m
+CONFIG_KEYBOARD_DAVINCI_DM365=m
 # CONFIG_INPUT_MOUSE is not set
 # CONFIG_INPUT_JOYSTICK is not set
 # CONFIG_INPUT_TABLET is not set
diff --git a/arch/arm/mach-davinci/include/mach/keypad.h b/arch/arm/mach-davinci/include/mach/keypad.h
new file mode 100644
index 0000000..922d20e
--- /dev/null
+++ b/arch/arm/mach-davinci/include/mach/keypad.h
@@ -0,0 +1,35 @@ 
+/*
+ * Copyright (C) 2009 Texas Instruments, Inc
+ *
+ * Author: Miguel Aguilar <miguel.aguilar@ridgerun.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
+ */
+
+#ifndef DAVINCI_KEYPAD_H
+#define DAVINCI_KEYPAD_H
+
+#include <linux/io.h>
+
+struct davinci_kp_platform_data {
+	int	*keymap;
+	u32	keymapsize;
+	u32	rep:1;
+	u32	strobe;
+	u32	interval;
+};
+
+#endif
+
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index a6b989a..b6b9517 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -361,4 +361,11 @@  config KEYBOARD_XTKBD
 	  To compile this driver as a module, choose M here: the
 	  module will be called xtkbd.
 
+config KEYBOARD_DAVINCI
+	tristate "TI DaVinci Keypad"
+	depends on ARCH_DAVINCI_DM365
+	help
+	  Say Y to enable keypad module support for the TI DaVinci
+	  platforms (DM365)
+
 endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index b5b5eae..0b0274e 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -31,3 +31,4 @@  obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
 obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
 obj-$(CONFIG_KEYBOARD_TOSA)		+= tosakbd.o
 obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
+obj-$(CONFIG_KEYBOARD_DAVINCI)		+= davinci_keypad.o
diff --git a/drivers/input/keyboard/davinci_keypad.c b/drivers/input/keyboard/davinci_keypad.c
new file mode 100644
index 0000000..6f0e793
--- /dev/null
+++ b/drivers/input/keyboard/davinci_keypad.c
@@ -0,0 +1,319 @@ 
+/*
+ * DaVinci Keypad Driver for TI platforms
+ *
+ * Copyright (C) 2009 Texas Instruments, Inc
+ *
+ * Author: Miguel Aguilar <miguel.aguilar@ridgerun.com>
+ *
+ * Intial Code: Sandeep Paulraj <s-paulraj@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/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 DAVINCI_KEYPAD_KEYCTRL		0x0000
+#define DAVINCI_KEYPAD_INTENA		0x0004
+#define DAVINCI_KEYPAD_INTFLAG		0x0008
+#define DAVINCI_KEYPAD_INTCLR		0x000c
+#define DAVINCI_KEYPAD_STRBWIDTH	0x0010
+#define DAVINCI_KEYPAD_INTERVAL		0x0014
+#define DAVINCI_KEYPAD_CONTTIME		0x0018
+#define DAVINCI_KEYPAD_CURRENTST	0x001c
+#define DAVINCI_KEYPAD_PREVSTATE	0x0020
+#define DAVINCI_KEYPAD_EMUCTRL		0x0024
+#define DAVINCI_KEYPAD_IODFTCTRL	0x002c
+
+/* Key Control Register (KEYCTRL) */
+#define DAVINCI_KEYPAD_KEYEN		0x00000001
+#define DAVINCI_KEYPAD_PREVMODE		0x00000002
+#define DAVINCI_KEYPAD_CHATOFF		0x00000004
+#define DAVINCI_KEYPAD_AUTODET		0x00000008
+#define DAVINCI_KEYPAD_SCANMODE		0x00000010
+#define DAVINCI_KEYPAD_OUTTYPE		0x00000020
+#define DAVINCI_KEYPAD_4X4		0x00000040
+
+/* Masks for the interrupts */
+#define DAVINCI_KEYPAD_INT_CONT		0x00000008
+#define DAVINCI_KEYPAD_INT_OFF		0x00000004
+#define DAVINCI_KEYPAD_INT_ON		0x00000002
+#define DAVINCI_KEYPAD_INT_CHANGE	0x00000001
+#define DAVINCI_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 davinci_kp_write(struct davinci_kp *davinci_kp, u32 val, u32 addr)
+{
+	u32 base = (u32)davinci_kp->base;
+
+	__raw_writel(val,(u32 *)(base + addr));
+}
+
+static u32 davinci_kp_read(struct davinci_kp *davinci_kp, u32 addr)
+{
+	u32 base = (u32)davinci_kp->base;
+
+	return __raw_readl((u32 *)(base + addr));
+}
+
+/* Initializing the kp Module */
+static void davinci_kp_initialize(struct davinci_kp *davinci_kp)
+{
+	u32 strobe = davinci_kp->pdata->strobe;
+	u32 interval = davinci_kp->pdata->interval;
+
+	/* Enable all interrupts */
+	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTENA);
+
+	/* Clear interrupts if any */
+	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
+
+	/* Setup the scan period = strobe + interval */
+	davinci_kp_write(davinci_kp, strobe, DAVINCI_KEYPAD_STRBWIDTH);
+	davinci_kp_write(davinci_kp, interval, DAVINCI_KEYPAD_INTERVAL);
+	davinci_kp_write(davinci_kp, 0x01, DAVINCI_KEYPAD_CONTTIME);
+
+	/* Enable Keyscan module and enable */
+	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_AUTODET | DAVINCI_KEYPAD_KEYEN,
+			DAVINCI_KEYPAD_KEYCTRL);
+}
+
+static irqreturn_t davinci_kp_interrupt(int irq, void *dev_id)
+{
+	struct davinci_kp *davinci_kp = dev_id;
+	struct device *dev = &davinci_kp->input->dev;
+	int *keymap = davinci_kp->pdata->keymap;
+		u32 prev_status, new_status, changed, position;
+	int keycode = KEY_UNKNOWN;
+	int ret = IRQ_NONE;
+
+	/* Disable interrupt */
+	davinci_kp_write(davinci_kp, 0x0, DAVINCI_KEYPAD_INTENA);
+
+	/* Reading previous and new status of the keypad */
+	prev_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_PREVSTATE);
+	new_status = davinci_kp_read(davinci_kp, DAVINCI_KEYPAD_CURRENTST);
+
+	changed = prev_status ^ new_status;
+	position = ffs(changed) - 1;
+
+	if (changed) {
+		keycode = keymap[position];
+		if((new_status >> position) & 0x1) {
+			/* Report release */
+			dev_dbg(dev, "davinci_keypad: key %d released\n",
+				    keycode);
+			input_report_key(davinci_kp->input, keycode, 0);
+		} else {
+			/* Report press */
+			dev_dbg(dev, "davinci_keypad: key %d pressed\n",
+				    keycode);
+			input_report_key(davinci_kp->input, keycode, 1);
+		}
+		input_sync(davinci_kp->input);
+		ret = IRQ_HANDLED;
+	}
+
+	/* Clearing interrupt */
+	davinci_kp_write(davinci_kp, DAVINCI_KEYPAD_INT_ALL, DAVINCI_KEYPAD_INTCLR);
+
+	/* Enable interrupts */
+	davinci_kp_write(davinci_kp, 0x1, DAVINCI_KEYPAD_INTENA);
+
+	return ret;
+}
+
+static int __init davinci_kp_probe(struct platform_device *pdev)
+{
+	struct davinci_kp *davinci_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 Keypad Driver\n");
+
+	if (!pdata->keymap) {
+		dev_dbg(dev, "no keymap from pdata\n");
+		return -EINVAL;
+	}
+
+	davinci_kp = kzalloc(sizeof *davinci_kp, GFP_KERNEL);
+	if(!davinci_kp) {
+		dev_dbg(dev, "could not allocate memory for private data\n");
+		return -ENOMEM;
+	}
+
+	key_dev = input_allocate_device();
+	if (!key_dev) {
+		dev_dbg(dev, "could not allocate input device\n");
+		ret = -ENOMEM;
+		goto fail1;
+	}
+
+	platform_set_drvdata(pdev, davinci_kp);
+
+	davinci_kp->input = key_dev;
+
+	davinci_kp->irq = platform_get_irq(pdev, 0);
+	if (davinci_kp->irq < 0) {
+		dev_err(dev, "no keypad irq\n");
+		ret = davinci_kp->irq;
+		goto fail2;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "no mem resource\n");
+		ret = -ENODEV;
+		goto fail2;
+	}
+
+	davinci_kp->pbase = res->start;
+	davinci_kp->base_size = resource_size(res);
+
+	mem = request_mem_region(davinci_kp->pbase, davinci_kp->base_size, pdev->name);
+	if (!mem) {
+		dev_err(dev, "KEYSCAN registers at %08x are not free\n",
+			davinci_kp->pbase);
+		ret = -EBUSY;
+		goto fail2;
+	}
+
+	davinci_kp->base = ioremap(davinci_kp->pbase, davinci_kp->base_size);
+	if (!davinci_kp->base) {
+		dev_err(dev, "can't ioremap MEM resource.\n");
+		ret = -ENOMEM;
+		goto fail3;
+	}
+
+	/* Enable auto repeat feature of Linux input subsystem */
+	if (pdata->rep)
+		__set_bit(EV_REP, key_dev->evbit);
+
+	/* Setup input device */
+	__set_bit(EV_KEY, key_dev->evbit);
+
+	/* Setup the keymap */
+	davinci_kp->pdata = pdata;
+
+	for (i = 0; i < davinci_kp->pdata->keymapsize; i++)
+		__set_bit(davinci_kp->pdata->keymap[i], key_dev->keybit);
+
+	key_dev->name = "davinci_keypad";
+	key_dev->phys = "davinci_keypad/input0";
+	key_dev->dev.parent = &pdev->dev;
+	key_dev->id.bustype = BUS_HOST;
+	key_dev->id.vendor = 0x0001;
+	key_dev->id.product = 0x0001;
+	key_dev->id.version = 0x0001;
+	key_dev->keycode = davinci_kp->pdata->keymap;
+	key_dev->keycodesize = sizeof(unsigned int);
+	key_dev->keycodemax = davinci_kp->pdata->keymapsize;
+
+	ret = input_register_device(davinci_kp->input);
+	if (ret < 0) {
+		dev_err(dev, "unable to register DaVinci keypad device\n");
+		goto fail4;
+	}
+
+	ret = request_irq(davinci_kp->irq, davinci_kp_interrupt, IRQF_DISABLED,
+                         "davinci_keypad", davinci_kp);
+	if (ret < 0) {
+		dev_err(dev, "unable to register DaVinci keypad Interrupt\n");
+		goto fail5;
+	}
+
+	davinci_kp_initialize(davinci_kp);
+
+	return 0;
+fail5:
+	input_unregister_device(davinci_kp->input);
+	key_dev = NULL;
+fail4:
+	iounmap(davinci_kp->base);
+fail3:
+	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
+fail2:
+	input_free_device(key_dev);
+fail1:
+	kfree(davinci_kp);
+
+	return ret;
+}
+
+static int __exit davinci_kp_remove(struct platform_device *pdev)
+{
+	struct davinci_kp *davinci_kp = platform_get_drvdata(pdev);
+
+	free_irq(davinci_kp->irq, davinci_kp);
+
+	input_unregister_device(davinci_kp->input);
+
+	iounmap(davinci_kp->base);
+	release_mem_region(davinci_kp->pbase, davinci_kp->base_size);
+
+	platform_set_drvdata(pdev, NULL);
+
+	kfree(davinci_kp);
+
+	return 0;
+}
+
+static struct platform_driver davinci_kp_driver = {
+	.driver = {
+			.name = "davinci_keypad",
+			.owner = THIS_MODULE,
+		},
+	.remove = __exit_p(davinci_kp_remove),
+};
+
+static int __init davinci_kp_init(void)
+{
+	return platform_driver_probe(&davinci_kp_driver, davinci_kp_probe);
+}
+module_init(davinci_kp_init);
+
+static void __exit davinci_kp_exit(void)
+{
+	platform_driver_unregister(&davinci_kp_driver);
+}
+module_exit(davinci_kp_exit);
+
+MODULE_AUTHOR("Miguel Aguilar");
+MODULE_DESCRIPTION("Texas Instruments DaVinci Keypad Driver");
+MODULE_LICENSE("GPL");