Re: [PATCH 21/74] ST SPEAr : Added keyboard support

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

 



Hi Rajeev,

On Mon, Aug 30, 2010 at 04:13:01PM +0530, Viresh KUMAR wrote:
> From: Rajeev Kumar <rajeev-dlh.kumar@xxxxxx>
> 
> Signed-off-by: Rajeev Kumar <rajeev-dlh.kumar@xxxxxx>
> Signed-off-by: shiraz hashim <shiraz.hashim@xxxxxx>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxx>
> ---
>  arch/arm/mach-spear13xx/clock.c                |    2 +-
>  arch/arm/mach-spear13xx/include/mach/generic.h |    1 +
>  arch/arm/mach-spear13xx/spear1300_evb.c        |   14 +
>  arch/arm/mach-spear13xx/spear13xx.c            |   19 ++
>  arch/arm/mach-spear3xx/clock.c                 |   12 +
>  arch/arm/mach-spear3xx/include/mach/generic.h  |    1 +
>  arch/arm/mach-spear3xx/spear300.c              |   19 ++
>  arch/arm/mach-spear3xx/spear300_evb.c          |   14 +
>  arch/arm/plat-spear/include/plat/keyboard.h    |  154 +++++++++++
>  drivers/input/keyboard/Kconfig                 |    8 +
>  drivers/input/keyboard/Makefile                |    1 +
>  drivers/input/keyboard/spear-keyboard.c        |  335 ++++++++++++++++++++++++
>  12 files changed, 579 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/plat-spear/include/plat/keyboard.h
>  create mode 100644 drivers/input/keyboard/spear-keyboard.c
> 

First of all, please split platform modifications from the driver itself
(as I care about the latter but less about the former).

> diff --git a/arch/arm/mach-spear13xx/clock.c b/arch/arm/mach-spear13xx/clock.c
> index 9252940..c48b779 100644
> --- a/arch/arm/mach-spear13xx/clock.c
> +++ b/arch/arm/mach-spear13xx/clock.c
> @@ -801,7 +801,7 @@ static struct clk_lookup spear_clk_lookups[] = {
>  	{.dev_id = "ssp",		.clk = &ssp_clk},
>  	{.dev_id = "gpio0",		.clk = &gpio0_clk},
>  	{.dev_id = "gpio1",		.clk = &gpio1_clk},
> -	{.dev_id = "kbd",		.clk = &kbd_clk},
> +	{.dev_id = "keyboard",		.clk = &kbd_clk},
>  	{.dev_id = "wdt",		.clk = &wdt_clk},
>  };
>  
> diff --git a/arch/arm/mach-spear13xx/include/mach/generic.h b/arch/arm/mach-spear13xx/include/mach/generic.h
> index 7e5d7e1..19c5de0 100644
> --- a/arch/arm/mach-spear13xx/include/mach/generic.h
> +++ b/arch/arm/mach-spear13xx/include/mach/generic.h
> @@ -33,6 +33,7 @@ extern struct amba_device uart_device;
>  extern struct platform_device ehci0_device;
>  extern struct platform_device ehci1_device;
>  extern struct platform_device i2c_device;
> +extern struct platform_device kbd_device;
>  extern struct platform_device ohci0_device;
>  extern struct platform_device ohci1_device;
>  extern struct platform_device rtc_device;
> diff --git a/arch/arm/mach-spear13xx/spear1300_evb.c b/arch/arm/mach-spear13xx/spear1300_evb.c
> index 0a6d26f..0cdad7a 100644
> --- a/arch/arm/mach-spear13xx/spear1300_evb.c
> +++ b/arch/arm/mach-spear13xx/spear1300_evb.c
> @@ -16,6 +16,7 @@
>  #include <asm/mach-types.h>
>  #include <mach/generic.h>
>  #include <mach/spear.h>
> +#include <plat/keyboard.h>
>  
>  static struct amba_device *amba_devs[] __initdata = {
>  	&uart_device,
> @@ -25,15 +26,28 @@ static struct platform_device *plat_devs[] __initdata = {
>  	&ehci0_device,
>  	&ehci1_device,
>  	&i2c_device,
> +	&kbd_device,
>  	&ohci0_device,
>  	&ohci1_device,
>  	&rtc_device,
>  };
>  
> +/* keyboard specific platform data */
> +static DECLARE_KEYMAP(spear_keymap);
> +
> +static struct kbd_platform_data kbd_data = {
> +	.keymap = spear_keymap,
> +	.keymapsize = ARRAY_SIZE(spear_keymap),
> +	.rep = 1,
> +};
> +
>  static void __init spear1300_evb_init(void)
>  {
>  	unsigned int i;
>  
> +	/* set keyboard plat data */
> +	kbd_set_plat_data(&kbd_device, &kbd_data);
> +
>  	/* call spear1300 machine init function */
>  	spear1300_init();
>  
> diff --git a/arch/arm/mach-spear13xx/spear13xx.c b/arch/arm/mach-spear13xx/spear13xx.c
> index c8f1aff..342fcd9 100644
> --- a/arch/arm/mach-spear13xx/spear13xx.c
> +++ b/arch/arm/mach-spear13xx/spear13xx.c
> @@ -166,6 +166,25 @@ struct platform_device ohci1_device = {
>  	.resource = ohci1_resources,
>  };
>  
> +/* keyboard device registration */
> +static struct resource kbd_resources[] = {
> +	{
> +		.start = SPEAR13XX_KBD_BASE,
> +		.end = SPEAR13XX_KBD_BASE + SZ_1K - 1,
> +		.flags = IORESOURCE_MEM,
> +	}, {
> +		.start = IRQ_KBD,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +struct platform_device kbd_device = {
> +	.name = "keyboard",
> +	.id = -1,
> +	.num_resources = ARRAY_SIZE(kbd_resources),
> +	.resource = kbd_resources,
> +};
> +
>  /* rtc device registration */
>  static struct resource rtc_resources[] = {
>  	{
> diff --git a/arch/arm/mach-spear3xx/clock.c b/arch/arm/mach-spear3xx/clock.c
> index 79f0159..3a26622 100644
> --- a/arch/arm/mach-spear3xx/clock.c
> +++ b/arch/arm/mach-spear3xx/clock.c
> @@ -470,6 +470,15 @@ static struct clk i2c1_clk = {
>  };
>  #endif
>  
> +#ifdef CONFIG_MACH_SPEAR300
> +/* keyboard clock */
> +static struct clk kbd_clk = {
> +	.flags = ALWAYS_ENABLED,
> +	.pclk = &apb_clk,
> +	.recalc = &follow_parent,
> +};
> +#endif
> +
>  /* array of all spear 3xx clock lookups */
>  static struct clk_lookup spear_clk_lookups[] = {
>  	{ .con_id = "apb_pclk",	.clk = &dummy_apb_pclk},
> @@ -511,6 +520,9 @@ static struct clk_lookup spear_clk_lookups[] = {
>  	{ .dev_id = "adc",		.clk = &adc_clk},
>  	{ .dev_id = "ssp",		.clk = &ssp_clk},
>  	{ .dev_id = "gpio",		.clk = &gpio_clk},
> +#ifdef CONFIG_MACH_SPEAR300
> +	{ .dev_id = "keyboard",		.clk = &kbd_clk},
> +#endif
>  #ifdef CONFIG_MACH_SPEAR320
>  	{ .dev_id = "i2c_designware.1",	.clk = &i2c1_clk},
>  #endif
> diff --git a/arch/arm/mach-spear3xx/include/mach/generic.h b/arch/arm/mach-spear3xx/include/mach/generic.h
> index 3eb2737..70f7ee2 100644
> --- a/arch/arm/mach-spear3xx/include/mach/generic.h
> +++ b/arch/arm/mach-spear3xx/include/mach/generic.h
> @@ -108,6 +108,7 @@ extern struct pmx_driver pmx_driver;
>  /* Add spear300 machine device structure declarations here */
>  extern struct amba_device clcd_device;
>  extern struct amba_device gpio1_device;
> +extern struct platform_device kbd_device;
>  
>  /* pad mux modes */
>  extern struct pmx_mode nand_mode;
> diff --git a/arch/arm/mach-spear3xx/spear300.c b/arch/arm/mach-spear3xx/spear300.c
> index 5d8df00..fa314e9 100644
> --- a/arch/arm/mach-spear3xx/spear300.c
> +++ b/arch/arm/mach-spear3xx/spear300.c
> @@ -407,6 +407,25 @@ struct amba_device gpio1_device = {
>  	.irq = {VIRQ_GPIO1, NO_IRQ},
>  };
>  
> +/* keyboard device registration */
> +static struct resource kbd_resources[] = {
> +	{
> +		.start = SPEAR300_KEYBOARD_BASE,
> +		.end = SPEAR300_KEYBOARD_BASE + SZ_1K - 1,
> +		.flags = IORESOURCE_MEM,
> +	}, {
> +		.start = VIRQ_KEYBOARD,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
> +struct platform_device kbd_device = {
> +	.name = "keyboard",
> +	.id = -1,
> +	.num_resources = ARRAY_SIZE(kbd_resources),
> +	.resource = kbd_resources,
> +};
> +
>  /* spear3xx shared irq */
>  struct shirq_dev_config shirq_ras1_config[] = {
>  	{
> diff --git a/arch/arm/mach-spear3xx/spear300_evb.c b/arch/arm/mach-spear3xx/spear300_evb.c
> index 3b3c6ce..afb773e 100644
> --- a/arch/arm/mach-spear3xx/spear300_evb.c
> +++ b/arch/arm/mach-spear3xx/spear300_evb.c
> @@ -15,6 +15,7 @@
>  #include <asm/mach-types.h>
>  #include <mach/generic.h>
>  #include <mach/spear.h>
> +#include <plat/keyboard.h>
>  
>  /* padmux devices to enable */
>  static struct pmx_dev *pmx_devs[] = {
> @@ -51,6 +52,16 @@ static struct platform_device *plat_devs[] __initdata = {
>  	&rtc_device,
>  
>  	/* spear300 specific devices */
> +	&kbd_device,
> +};
> +
> +/* keyboard specific platform data */
> +static DECLARE_KEYMAP(spear_keymap);
> +
> +static struct kbd_platform_data kbd_data = {
> +	.keymap = spear_keymap,
> +	.keymapsize = ARRAY_SIZE(spear_keymap),
> +	.rep = 1,
>  };
>  
>  static void __init spear300_evb_init(void)
> @@ -62,6 +73,9 @@ static void __init spear300_evb_init(void)
>  	pmx_driver.devs = pmx_devs;
>  	pmx_driver.devs_count = ARRAY_SIZE(pmx_devs);
>  
> +	/* set keyboard plat data */
> +	kbd_set_plat_data(&kbd_device, &kbd_data);
> +
>  	/* call spear300 machine init function */
>  	spear300_init();
>  
> diff --git a/arch/arm/plat-spear/include/plat/keyboard.h b/arch/arm/plat-spear/include/plat/keyboard.h
> new file mode 100644
> index 0000000..8345770
> --- /dev/null
> +++ b/arch/arm/plat-spear/include/plat/keyboard.h
> @@ -0,0 +1,154 @@
> +/*
> + * arch/arm/plat-spear/include/plat/keyboard.h
> + *
> + * Copyright (C) 2010 ST Microelectronics
> + * Rajeev Kumar<rajeev-dlh.kumar@xxxxxx>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __PLAT_KEYBOARD_H
> +#define __PLAT_KEYBOARD_H
> +
> +#include <linux/bitops.h>
> +#include <linux/input.h>
> +#include <mach/misc_regs.h>
> +
> +#define KEY(row, col, val) (((row) << 28 | ((col) << 24) | (val)))
> +

Please use definitions from include/linux/input/matrix_keypad.h

> +#define DECLARE_KEYMAP(name) \
> +int name[] = {\
> +	KEY(0, 0, KEY_ESC), \
> +	KEY(0, 1, KEY_1), \
> +	KEY(0, 2, KEY_2), \
> +	KEY(0, 3, KEY_3), \
> +	KEY(0, 4, KEY_4), \
> +	KEY(0, 5, KEY_5), \
> +	KEY(0, 6, KEY_6), \
> +	KEY(0, 7, KEY_7), \
> +	KEY(0, 8, KEY_8), \
> +	KEY(1, 0, KEY_9), \
> +	KEY(1, 1, KEY_MINUS), \
> +	KEY(1, 2, KEY_EQUAL), \
> +	KEY(1, 3, KEY_BACKSPACE), \
> +	KEY(1, 4, KEY_TAB), \
> +	KEY(1, 5, KEY_Q), \
> +	KEY(1, 6, KEY_W), \
> +	KEY(1, 7, KEY_E), \
> +	KEY(1, 8, KEY_R), \
> +	KEY(2, 0, KEY_T), \
> +	KEY(2, 1, KEY_Y), \
> +	KEY(2, 2, KEY_U), \
> +	KEY(2, 3, KEY_I), \
> +	KEY(2, 4, KEY_O), \
> +	KEY(2, 5, KEY_P), \
> +	KEY(2, 6, KEY_LEFTBRACE), \
> +	KEY(2, 7, KEY_RIGHTBRACE), \
> +	KEY(2, 8, KEY_ENTER), \
> +	KEY(3, 0, KEY_LEFTCTRL), \
> +	KEY(3, 1, KEY_A), \
> +	KEY(3, 2, KEY_S), \
> +	KEY(3, 3, KEY_D), \
> +	KEY(3, 4, KEY_F), \
> +	KEY(3, 5, KEY_G), \
> +	KEY(3, 6, KEY_H), \
> +	KEY(3, 7, KEY_J), \
> +	KEY(3, 8, KEY_K), \
> +	KEY(4, 0, KEY_L), \
> +	KEY(4, 1, KEY_SEMICOLON), \
> +	KEY(4, 2, KEY_APOSTROPHE), \
> +	KEY(4, 3, KEY_GRAVE), \
> +	KEY(4, 4, KEY_LEFTSHIFT), \
> +	KEY(4, 5, KEY_BACKSLASH), \
> +	KEY(4, 6, KEY_Z), \
> +	KEY(4, 7, KEY_X), \
> +	KEY(4, 8, KEY_C), \
> +	KEY(4, 0, KEY_L), \
> +	KEY(4, 1, KEY_SEMICOLON), \
> +	KEY(4, 2, KEY_APOSTROPHE), \
> +	KEY(4, 3, KEY_GRAVE), \
> +	KEY(4, 4, KEY_LEFTSHIFT), \
> +	KEY(4, 5, KEY_BACKSLASH), \
> +	KEY(4, 6, KEY_Z), \
> +	KEY(4, 7, KEY_X), \
> +	KEY(4, 8, KEY_C), \
> +	KEY(4, 0, KEY_L), \
> +	KEY(4, 1, KEY_SEMICOLON), \
> +	KEY(4, 2, KEY_APOSTROPHE), \
> +	KEY(4, 3, KEY_GRAVE), \
> +	KEY(4, 4, KEY_LEFTSHIFT), \
> +	KEY(4, 5, KEY_BACKSLASH), \
> +	KEY(4, 6, KEY_Z), \
> +	KEY(4, 7, KEY_X), \
> +	KEY(4, 8, KEY_C), \
> +	KEY(5, 0, KEY_V), \
> +	KEY(5, 1, KEY_B), \
> +	KEY(5, 2, KEY_N), \
> +	KEY(5, 3, KEY_M), \
> +	KEY(5, 4, KEY_COMMA), \
> +	KEY(5, 5, KEY_DOT), \
> +	KEY(5, 6, KEY_SLASH), \
> +	KEY(5, 7, KEY_RIGHTSHIFT), \
> +	KEY(5, 8, KEY_KPASTERISK), \
> +	KEY(6, 0, KEY_LEFTALT), \
> +	KEY(6, 1, KEY_SPACE), \
> +	KEY(6, 2, KEY_CAPSLOCK), \
> +	KEY(6, 3, KEY_F1), \
> +	KEY(6, 4, KEY_F2), \
> +	KEY(6, 5, KEY_F3), \
> +	KEY(6, 6, KEY_F4), \
> +	KEY(6, 7, KEY_F5), \
> +	KEY(6, 8, KEY_F6), \
> +	KEY(7, 0, KEY_F7), \
> +	KEY(7, 1, KEY_F8), \
> +	KEY(7, 2, KEY_F9), \
> +	KEY(7, 3, KEY_F10), \
> +	KEY(7, 4, KEY_NUMLOCK), \
> +	KEY(7, 5, KEY_SCROLLLOCK), \
> +	KEY(7, 6, KEY_KP7), \
> +	KEY(7, 7, KEY_KP8), \
> +	KEY(7, 8, KEY_KP9), \
> +	KEY(8, 0, KEY_KPMINUS), \
> +	KEY(8, 1, KEY_KP4), \
> +	KEY(8, 2, KEY_KP5), \
> +	KEY(8, 3, KEY_KP6), \
> +	KEY(8, 4, KEY_KPPLUS), \
> +	KEY(8, 5, KEY_KP1), \
> +	KEY(8, 6, KEY_KP2), \
> +	KEY(8, 7, KEY_KP3), \
> +	KEY(8, 8, KEY_KP0), \
> +};

Hm, I'd expect this to be in particular board code, not in the header
file.

> +/**
> + * struct kbd_platform_data - keymap for spear keyboards
> + * keymap: pointer to array of values encoded with KEY() macro representing
> + * keymap
> + * keymapsize: number of entries (initialized) in this keymap
> + * rep: current values for autorepeat parameters
> + *
> + * This structure is supposed to be used by platform code to supply
> + * keymaps to drivers that implement keyboards.
> + */
> +struct kbd_platform_data {
> +	int *keymap;
> +	unsigned int keymapsize;
> +	unsigned int rep:1;

Bool please.

> +};
> +
> +/* This function is used to set platform data field of pdev->dev */
> +static inline void
> +kbd_set_plat_data(struct platform_device *pdev, struct kbd_platform_data *data)
> +{
> +#ifdef CONFIG_ARCH_SPEAR13XX
> +#define KBD_PAD_SEL	(BIT(18) | BIT(20) | BIT(22) | BIT(24) | BIT(26))
> +
> +	u32 val;
> +	/* Workaround:Setting bit for routing it to the IP */
> +	val = readl(PAD_FUNCTION_EN_2);
> +	val &= ~KBD_PAD_SEL;
> +	writel(val, PAD_FUNCTION_EN_2);

Why does it belong here?

> +#endif
> +	pdev->dev.platform_data = data;
> +}
> +#endif /* __PLAT_KEYBOARD_H */
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 9cc488d..2d7e3a8 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -424,6 +424,14 @@ config KEYBOARD_OMAP
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called omap-keypad.
>  
> +config KEYBOARD_SPEAR
> +	tristate "ST SPEAR keyboard support"

No arch/platform dependencies?

> +	help
> +	  Say Y here if you want to use the SPEAR keyboard.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called spear-keboard.
> +
>  config KEYBOARD_TWL4030
>  	tristate "TI TWL4030/TWL5030/TPS659x0 keypad support"
>  	depends on TWL4030_CORE
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 504b591..b21c54d 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_KEYBOARD_PXA930_ROTARY)	+= pxa930_rotary.o
>  obj-$(CONFIG_KEYBOARD_QT2160)		+= qt2160.o
>  obj-$(CONFIG_KEYBOARD_SAMSUNG)		+= samsung-keypad.o
>  obj-$(CONFIG_KEYBOARD_SH_KEYSC)		+= sh_keysc.o
> +obj-$(CONFIG_KEYBOARD_SPEAR)		+= spear-keyboard.o
>  obj-$(CONFIG_KEYBOARD_STMPE)		+= stmpe-keypad.o
>  obj-$(CONFIG_KEYBOARD_STOWAWAY)		+= stowaway.o
>  obj-$(CONFIG_KEYBOARD_SUNKBD)		+= sunkbd.o
> diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
> new file mode 100644
> index 0000000..4f5c734
> --- /dev/null
> +++ b/drivers/input/keyboard/spear-keyboard.c
> @@ -0,0 +1,335 @@
> +/*
> + * drivers/input/keyboard/keyboard-spear.c
> + *
> + * SPEAr Keyboard Driver
> + * Based on omap-keypad driver
> + *
> + * Copyright (C) 2010 ST Microelectronics
> + * Rajeev Kumar<rajeev-dlh.kumar@xxxxxx>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <plat/keyboard.h>
> +
> +/* Keyboard Regsiters */
> +#define MODE_REG	0x00	/* 16 bit reg */
> +#define STATUS_REG	0x0C	/* 2 bit reg */
> +#define DATA_REG	0x10	/* 8 bit reg */
> +#define INTR_MASK	0x54
> +
> +/* Register Values */
> +/*
> + * pclk freq mask = (APB FEQ -1)= 82 MHZ.Programme bit 15-9 in mode
> + * control register as 1010010(82MHZ)
> + */
> +#define PCLK_FREQ_MSK	0xA400	/* 82 MHz */
> +#define START_SCAN	0x0100
> +#define SCAN_RATE_10	0x0000
> +#define SCAN_RATE_20	0x0004
> +#define SCAN_RATE_40	0x0008
> +#define SCAN_RATE_80	0x000C
> +#define MODE_KEYBOARD	0x0002
> +#define DATA_AVAIL	0x2
> +
> +#define KEY_MASK	0xFF000000
> +#define KEY_VALUE	0x00FFFFFF
> +#define ROW_MASK	0xF0
> +#define COLUMN_MASK	0x0F
> +#define ROW_SHIFT	4
> +
> +struct spear_kbd {
> +	struct input_dev *input;
> +	void __iomem *io_base;		/* Keyboard Base Address */
> +	struct clk *clk;
> +	int *keymap;

You need a copy of keymap here so that userspace can modify it safely
via EVIOCSKEYCODE.

> +};
> +/* TODO: Need to optimize this function */
> +static inline int get_key_value(struct spear_kbd *dev, int row, int col)
> +{
> +	int i, key;
> +	int *keymap = dev->keymap;
> +
> +	key = KEY(row, col, 0);
> +	for (i = 0; keymap[i] != 0; i++)
> +		if ((keymap[i] & KEY_MASK) == key)
> +			return keymap[i] & KEY_VALUE;
> +	return -ENOKEY;
> +}
> +
> +static irqreturn_t spear_kbd_interrupt(int irq, void *dev_id)
> +{
> +	struct spear_kbd *dev = dev_id;
> +	static u8 last_key ;
> +	static u8 last_event;

No statics please, pull it into spear_kbd structure.

> +	int key;
> +	u8 sts, val = 0;
> +
> +	if (dev == NULL) {

How can it be?

> +		pr_err("Keyboard: Invalid dev_id in irq handler\n");
> +		return IRQ_NONE;
> +	}
> +
> +	sts = readb(dev->io_base + STATUS_REG);
> +	if (sts & DATA_AVAIL) {
> +		/* following reads active (row, col) pair */
> +		val = readb(dev->io_base + DATA_REG);
> +		key = get_key_value(dev, (val & ROW_MASK)>>ROW_SHIFT, (val
> +					& COLUMN_MASK));
> +
> +		/* valid key press event */
> +		if (key >= 0) {
> +			if (last_event == 1) {
> +				/* check if we missed a release event */
> +				input_report_key(dev->input, last_key,
> +						!last_event);
> +			}
> +			/* notify key press */
> +			last_event = 1;
> +			last_key = key;
> +			input_report_key(dev->input, key, last_event);
> +		} else {
> +			/* notify key release */
> +			last_event = 0;
> +			input_report_key(dev->input, last_key, last_event);
> +		}
> +	} else

Don't you need to clear interrupt here? You got it somehow...

> +		return IRQ_NONE;
> +
> +	/* clear interrupt */
> +	writeb(0, dev->io_base + STATUS_REG);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __init spear_kbd_probe(struct platform_device *pdev)
> +{
> +	struct spear_kbd *kbd;
> +	struct kbd_platform_data *pdata = pdev->dev.platform_data;
> +	struct resource *res;
> +	int i, ret, irq;
> +	u16 val = 0;
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "Invalid platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "no keyboard resource defined\n");
> +		return -EBUSY;
> +	}
> +
> +	if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
> +		dev_err(&pdev->dev, "keyboard region already claimed\n");
> +		return -EBUSY;
> +	}
> +
> +	kbd = kzalloc(sizeof(*kbd), GFP_KERNEL);
> +	if (!kbd) {
> +		dev_err(&pdev->dev, "out of memory\n");
> +		ret = -ENOMEM;
> +		goto err_release_mem_region;
> +	}
> +
> +	kbd->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(kbd->clk)) {
> +		ret = PTR_ERR(kbd->clk);
> +		goto err_kfree;
> +	}
> +
> +	ret = clk_enable(kbd->clk);
> +	if (ret < 0)
> +		goto err_clk_put;
> +
> +	platform_set_drvdata(pdev, kbd);
> +	kbd->keymap = pdata->keymap; /* key mappings */
> +
> +	kbd->io_base = ioremap(res->start, resource_size(res));
> +	if (!kbd->io_base) {
> +		dev_err(&pdev->dev, "ioremap fail for kbd_region\n");
> +		ret = -ENOMEM;
> +		goto err_clear_plat_data;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "not able to get irq for the device\n");
> +		ret = irq;
> +		goto err_iounmap;
> +	}
> +
> +	ret = request_irq(irq, spear_kbd_interrupt, 0, "keyboard",
> +			kbd);
> +	if (ret) {
> +		dev_err(&pdev->dev, "request_irq fail\n");
> +		goto err_iounmap;
> +	}
> +

Are you 100% sure the device is shut off at this point? Because if it
isn't you risk to get interrupt before you allocated your input device.

> +	kbd->input = input_allocate_device();
> +	if (!kbd->input) {
> +		ret = -ENOMEM;
> +		dev_err(&pdev->dev, "input device allocation fail\n");
> +		goto err_free_irq;
> +	}
> +
> +	if (pdata->rep)
> +		__set_bit(EV_REP, kbd->input->evbit);
> +
> +	/* setup input device */
> +	__set_bit(EV_KEY, kbd->input->evbit);
> +
> +	for (i = 0; kbd->keymap[i] != 0; i++)
> +		__set_bit(kbd->keymap[i] & KEY_MAX, kbd->input->keybit);
> +
> +	kbd->input->name = "keyboard";
> +	kbd->input->phys = "keyboard/input0";
> +	kbd->input->dev.parent = &pdev->dev;
> +	kbd->input->id.bustype = BUS_HOST;
> +	kbd->input->id.vendor = 0x0001;
> +	kbd->input->id.product = 0x0001;
> +	kbd->input->id.version = 0x0100;
> +
> +	ret = input_register_device(kbd->input);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Unable to register keyboard device\n");
> +		goto err_free_dev;
> +	}
> +
> +	/* program keyboard */
> +	val |= SCAN_RATE_80 | MODE_KEYBOARD | PCLK_FREQ_MSK;
> +	writew(val, kbd->io_base + MODE_REG);
> +
> +	writeb(1, kbd->io_base + STATUS_REG);
> +
> +	/* start key scan */
> +	val |= START_SCAN;
> +	writew(val, kbd->io_base + MODE_REG);

This should go into open() method.

> +
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	return 0;
> +
> +err_free_dev:
> +	input_free_device(kbd->input);
> +err_free_irq:
> +	free_irq(irq, pdev);
> +err_iounmap:
> +	iounmap(kbd->io_base);
> +err_clear_plat_data:
> +	platform_set_drvdata(pdev, NULL);
> +	clk_disable(kbd->clk);
> +err_clk_put:
> +	clk_put(kbd->clk);
> +err_kfree:
> +	kfree(kbd);
> +err_release_mem_region:
> +	release_mem_region(res->start, resource_size(res));
> +
> +	return ret;
> +}
> +
> +static int spear_kbd_remove(struct platform_device *pdev)
> +{
> +	struct spear_kbd *kbd = platform_get_drvdata(pdev);
> +	struct resource *res;
> +	u16 val;
> +	int irq;
> +
> +	val = readw(kbd->io_base + MODE_REG);
> +	val &= ~START_SCAN;
> +	writew(val, kbd->io_base + MODE_REG);

Need to go into close() method.

> +
> +	/* unregister input device */
> +	input_unregister_device(kbd->input);
> +	input_free_device(kbd->input);

No free after unregister.

> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq)
> +		free_irq(irq, pdev);

Can it ever be 0 here?

> +
> +	iounmap(kbd->io_base);
> +	platform_set_drvdata(pdev, NULL);
> +	clk_disable(kbd->clk);
> +	clk_put(kbd->clk);
> +	kfree(kbd);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res)
> +		release_mem_region(res->start, resource_size(res));
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int spear_kbd_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct spear_kbd *kbd = platform_get_drvdata(pdev);
> +	int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	clk_disable(kbd->clk);
> +	if (device_may_wakeup(&pdev->dev))
> +		enable_irq_wake(irq);
> +
> +	return 0;
> +}
> +
> +static int spear_kbd_resume(struct platform_device *pdev)
> +{
> +	struct spear_kbd *kbd = platform_get_drvdata(pdev);
> +	int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (device_may_wakeup(&pdev->dev))
> +		disable_irq_wake(irq);
> +	clk_enable(kbd->clk);
> +
> +	return 0;
> +}
> +#else
> +#define spear_kbd_suspend	NULL
> +#define spear_kbd_resume	NULL
> +#endif
> +
> +static struct platform_driver spear_kbd_driver = {
> +	.probe		= spear_kbd_probe,
> +	.remove		= spear_kbd_remove,
> +	.suspend	= spear_kbd_suspend,
> +	.resume		= spear_kbd_resume,

Switch to dev_pm_ops please.

> +	.driver		= {
> +		.name	= "keyboard",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __devinit spear_kbd_init(void)
> +{
> +	return platform_driver_register(&spear_kbd_driver);
> +}
> +module_init(spear_kbd_init);
> +
> +static void __exit spear_kbd_exit(void)
> +{
> +	platform_driver_unregister(&spear_kbd_driver);
> +}
> +module_exit(spear_kbd_exit);
> +
> +MODULE_AUTHOR("Rajeev Kumar");
> +MODULE_DESCRIPTION("SPEAr Keyboard Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.2.2
> 

Thanks.

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


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

  Powered by Linux