* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 6:26 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-21 6:26 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-samsung-soc, linux-input, ben-linux, dmitry.torokhov,
kyungmin.park, kgene.kim
This patch adds samsung keypad device definition for samsung cpus.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/plat-samsung/Kconfig | 5 ++
arch/arm/plat-samsung/Makefile | 1 +
arch/arm/plat-samsung/dev-keypad.c | 58 +++++++++++++++++++++
arch/arm/plat-samsung/include/plat/devs.h | 2 +
arch/arm/plat-samsung/include/plat/keypad.h | 59 ++++++++++++++++++++++
arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ++++++++++++++++++
6 files changed, 174 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/plat-samsung/dev-keypad.c
create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
index 2753fb3..bd007e3 100644
--- a/arch/arm/plat-samsung/Kconfig
+++ b/arch/arm/plat-samsung/Kconfig
@@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
help
Common in platform device definitions for touchscreen device
+config SAMSUNG_DEV_KEYPAD
+ bool
+ help
+ Compile in platform device definitions for keypad
+
# DMA
config S3C_DMA
diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
index b1d82cc..8269d80 100644
--- a/arch/arm/plat-samsung/Makefile
+++ b/arch/arm/plat-samsung/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) += dev-rtc.o
obj-$(CONFIG_SAMSUNG_DEV_ADC) += dev-adc.o
obj-$(CONFIG_SAMSUNG_DEV_TS) += dev-ts.o
+obj-$(CONFIG_SAMSUNG_DEV_KEYPAD) += dev-keypad.o
# DMA support
diff --git a/arch/arm/plat-samsung/dev-keypad.c b/arch/arm/plat-samsung/dev-keypad.c
new file mode 100644
index 0000000..679b444
--- /dev/null
+++ b/arch/arm/plat-samsung/dev-keypad.c
@@ -0,0 +1,58 @@
+/*
+ * linux/arch/arm/plat-samsung/dev-keypad.c
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.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.
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <mach/irqs.h>
+#include <mach/map.h>
+#include <plat/cpu.h>
+#include <plat/devs.h>
+#include <plat/keypad.h>
+
+static struct resource samsung_keypad_resources[] = {
+ [0] = {
+ .start = SAMSUNG_PA_KEYPAD,
+ .end = SAMSUNG_PA_KEYPAD + 0x20 - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = IRQ_KEYPAD,
+ .end = IRQ_KEYPAD,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+struct platform_device samsung_device_keypad = {
+ .name = "samsung-keypad",
+ .id = -1,
+ .num_resources = ARRAY_SIZE(samsung_keypad_resources),
+ .resource = samsung_keypad_resources,
+};
+
+void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
+{
+ struct samsung_keypad_platdata *npd;
+
+ if (!pd) {
+ printk(KERN_ERR "%s: no platform data\n", __func__);
+ return;
+ }
+
+ npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
+ if (!npd)
+ printk(KERN_ERR "%s: no memory for platform data\n", __func__);
+
+ if (!npd->cfg_gpio)
+ npd->cfg_gpio = samsung_keypad_cfg_gpio;
+
+ samsung_device_keypad.dev.platform_data = npd;
+}
diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
index e6144e4..6d9f01b 100644
--- a/arch/arm/plat-samsung/include/plat/devs.h
+++ b/arch/arm/plat-samsung/include/plat/devs.h
@@ -100,6 +100,8 @@ extern struct platform_device s5pc100_device_iis0;
extern struct platform_device s5pc100_device_iis1;
extern struct platform_device s5pc100_device_iis2;
+extern struct platform_device samsung_device_keypad;
+
/* s3c2440 specific devices */
#ifdef CONFIG_CPU_S3C2440
diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
new file mode 100644
index 0000000..6d139d6
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/keypad.h
@@ -0,0 +1,59 @@
+/*
+ * linux/arch/arm/plat-samsung/include/plat/keypad.h
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ *
+ * Samsung Platform - Keypad platform data definitions
+ *
+ * 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.
+ *
+ */
+
+#ifndef __PLAT_SAMSUNG_KEYPAD_H
+#define __PLAT_SAMSUNG_KEYPAD_H
+
+#include <linux/input/matrix_keypad.h>
+
+#define SAMSUNG_MAX_ROWS 8
+#define SAMSUNG_MAX_COLS 8
+
+/**
+ * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
+ * @keymap_data: pointer to &matrix_keymap_data.
+ * @rows: number of keypad row supported.
+ * @cols: number of keypad col supported.
+ * @no_autorepeat: disable key autorepeat.
+ * @wakeup: controls whether the device should be set up as wakeup source.
+ * @cfg_gpio: configure the GPIO.
+ *
+ * Initialisation data specific to either the machine or the platform
+ * for the device driver to use or call-back when configuring gpio.
+ */
+struct samsung_keypad_platdata {
+ const struct matrix_keymap_data *keymap_data;
+ unsigned int rows;
+ unsigned int cols;
+ bool no_autorepeat;
+ bool wakeup;
+
+ void (*cfg_gpio)(unsigned int rows, unsigned int cols);
+};
+
+/**
+ * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
+ * @pd: Platform data to register to device.
+ *
+ * Register the given platform data for use with Samsung Keypad device.
+ * The call will copy the platform data, so the board definitions can
+ * make the structure itself __initdata.
+ */
+extern void samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd);
+
+/* defined by architecture to configure gpio. */
+extern void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols);
+
+#endif /* __PLAT_SAMSUNG_KEYPAD_H */
diff --git a/arch/arm/plat-samsung/include/plat/regs-keypad.h b/arch/arm/plat-samsung/include/plat/regs-keypad.h
new file mode 100644
index 0000000..e4688f0
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/regs-keypad.h
@@ -0,0 +1,49 @@
+/*
+ * linux/arch/arm/plat-samsung/include/plat/regs-keypad.h
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.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.
+ *
+ */
+
+#ifndef __SAMSUNG_KEYPAD_H__
+#define __SAMSUNG_KEYPAD_H__
+
+#define SAMSUNG_KEYIFCON 0x00
+#define SAMSUNG_KEYIFSTSCLR 0x04
+#define SAMSUNG_KEYIFCOL 0x08
+#define SAMSUNG_KEYIFROW 0x0c
+#define SAMSUNG_KEYIFFC 0x10
+
+/* SAMSUNG_KEYIFCON */
+#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
+#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
+#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
+#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
+#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
+
+/* SAMSUNG_KEYIFSTSCLR */
+#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
+#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
+#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
+#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
+#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
+#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
+
+/* SAMSUNG_KEYIFCOL */
+#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
+#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
+
+/* SAMSUNG_KEYIFROW */
+#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
+#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
+
+/* SAMSUNG_KEYIFFC */
+#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
+
+#endif /* __SAMSUNG_KEYPAD_H__ */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 6:26 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-21 6:26 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds samsung keypad device definition for samsung cpus.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/plat-samsung/Kconfig | 5 ++
arch/arm/plat-samsung/Makefile | 1 +
arch/arm/plat-samsung/dev-keypad.c | 58 +++++++++++++++++++++
arch/arm/plat-samsung/include/plat/devs.h | 2 +
arch/arm/plat-samsung/include/plat/keypad.h | 59 ++++++++++++++++++++++
arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ++++++++++++++++++
6 files changed, 174 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/plat-samsung/dev-keypad.c
create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
index 2753fb3..bd007e3 100644
--- a/arch/arm/plat-samsung/Kconfig
+++ b/arch/arm/plat-samsung/Kconfig
@@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
help
Common in platform device definitions for touchscreen device
+config SAMSUNG_DEV_KEYPAD
+ bool
+ help
+ Compile in platform device definitions for keypad
+
# DMA
config S3C_DMA
diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
index b1d82cc..8269d80 100644
--- a/arch/arm/plat-samsung/Makefile
+++ b/arch/arm/plat-samsung/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) += dev-rtc.o
obj-$(CONFIG_SAMSUNG_DEV_ADC) += dev-adc.o
obj-$(CONFIG_SAMSUNG_DEV_TS) += dev-ts.o
+obj-$(CONFIG_SAMSUNG_DEV_KEYPAD) += dev-keypad.o
# DMA support
diff --git a/arch/arm/plat-samsung/dev-keypad.c b/arch/arm/plat-samsung/dev-keypad.c
new file mode 100644
index 0000000..679b444
--- /dev/null
+++ b/arch/arm/plat-samsung/dev-keypad.c
@@ -0,0 +1,58 @@
+/*
+ * linux/arch/arm/plat-samsung/dev-keypad.c
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.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.
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <mach/irqs.h>
+#include <mach/map.h>
+#include <plat/cpu.h>
+#include <plat/devs.h>
+#include <plat/keypad.h>
+
+static struct resource samsung_keypad_resources[] = {
+ [0] = {
+ .start = SAMSUNG_PA_KEYPAD,
+ .end = SAMSUNG_PA_KEYPAD + 0x20 - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = IRQ_KEYPAD,
+ .end = IRQ_KEYPAD,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+struct platform_device samsung_device_keypad = {
+ .name = "samsung-keypad",
+ .id = -1,
+ .num_resources = ARRAY_SIZE(samsung_keypad_resources),
+ .resource = samsung_keypad_resources,
+};
+
+void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
+{
+ struct samsung_keypad_platdata *npd;
+
+ if (!pd) {
+ printk(KERN_ERR "%s: no platform data\n", __func__);
+ return;
+ }
+
+ npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
+ if (!npd)
+ printk(KERN_ERR "%s: no memory for platform data\n", __func__);
+
+ if (!npd->cfg_gpio)
+ npd->cfg_gpio = samsung_keypad_cfg_gpio;
+
+ samsung_device_keypad.dev.platform_data = npd;
+}
diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
index e6144e4..6d9f01b 100644
--- a/arch/arm/plat-samsung/include/plat/devs.h
+++ b/arch/arm/plat-samsung/include/plat/devs.h
@@ -100,6 +100,8 @@ extern struct platform_device s5pc100_device_iis0;
extern struct platform_device s5pc100_device_iis1;
extern struct platform_device s5pc100_device_iis2;
+extern struct platform_device samsung_device_keypad;
+
/* s3c2440 specific devices */
#ifdef CONFIG_CPU_S3C2440
diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
new file mode 100644
index 0000000..6d139d6
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/keypad.h
@@ -0,0 +1,59 @@
+/*
+ * linux/arch/arm/plat-samsung/include/plat/keypad.h
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ *
+ * Samsung Platform - Keypad platform data definitions
+ *
+ * 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.
+ *
+ */
+
+#ifndef __PLAT_SAMSUNG_KEYPAD_H
+#define __PLAT_SAMSUNG_KEYPAD_H
+
+#include <linux/input/matrix_keypad.h>
+
+#define SAMSUNG_MAX_ROWS 8
+#define SAMSUNG_MAX_COLS 8
+
+/**
+ * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
+ * @keymap_data: pointer to &matrix_keymap_data.
+ * @rows: number of keypad row supported.
+ * @cols: number of keypad col supported.
+ * @no_autorepeat: disable key autorepeat.
+ * @wakeup: controls whether the device should be set up as wakeup source.
+ * @cfg_gpio: configure the GPIO.
+ *
+ * Initialisation data specific to either the machine or the platform
+ * for the device driver to use or call-back when configuring gpio.
+ */
+struct samsung_keypad_platdata {
+ const struct matrix_keymap_data *keymap_data;
+ unsigned int rows;
+ unsigned int cols;
+ bool no_autorepeat;
+ bool wakeup;
+
+ void (*cfg_gpio)(unsigned int rows, unsigned int cols);
+};
+
+/**
+ * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
+ * @pd: Platform data to register to device.
+ *
+ * Register the given platform data for use with Samsung Keypad device.
+ * The call will copy the platform data, so the board definitions can
+ * make the structure itself __initdata.
+ */
+extern void samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd);
+
+/* defined by architecture to configure gpio. */
+extern void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols);
+
+#endif /* __PLAT_SAMSUNG_KEYPAD_H */
diff --git a/arch/arm/plat-samsung/include/plat/regs-keypad.h b/arch/arm/plat-samsung/include/plat/regs-keypad.h
new file mode 100644
index 0000000..e4688f0
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/regs-keypad.h
@@ -0,0 +1,49 @@
+/*
+ * linux/arch/arm/plat-samsung/include/plat/regs-keypad.h
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.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.
+ *
+ */
+
+#ifndef __SAMSUNG_KEYPAD_H__
+#define __SAMSUNG_KEYPAD_H__
+
+#define SAMSUNG_KEYIFCON 0x00
+#define SAMSUNG_KEYIFSTSCLR 0x04
+#define SAMSUNG_KEYIFCOL 0x08
+#define SAMSUNG_KEYIFROW 0x0c
+#define SAMSUNG_KEYIFFC 0x10
+
+/* SAMSUNG_KEYIFCON */
+#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
+#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
+#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
+#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
+#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
+
+/* SAMSUNG_KEYIFSTSCLR */
+#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
+#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
+#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
+#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
+#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
+#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
+
+/* SAMSUNG_KEYIFCOL */
+#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
+#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
+
+/* SAMSUNG_KEYIFROW */
+#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
+#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
+
+/* SAMSUNG_KEYIFFC */
+#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
+
+#endif /* __SAMSUNG_KEYPAD_H__ */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 2/3] ARM: S5PV210: Add keypad device helpers
2010-06-21 6:26 ` Joonyoung Shim
@ 2010-06-21 6:26 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-21 6:26 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-samsung-soc, linux-input, ben-linux, dmitry.torokhov,
kyungmin.park, kgene.kim
This patch adds the keypad device platform helpers for S5PV210 cpu.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/mach-s5pv210/Kconfig | 5 +++
arch/arm/mach-s5pv210/Makefile | 1 +
arch/arm/mach-s5pv210/cpu.c | 4 ++
arch/arm/mach-s5pv210/include/mach/map.h | 3 ++
arch/arm/mach-s5pv210/setup-keypad.c | 34 ++++++++++++++++++++++
arch/arm/plat-samsung/include/plat/keypad-core.h | 31 ++++++++++++++++++++
6 files changed, 78 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-s5pv210/setup-keypad.c
create mode 100644 arch/arm/plat-samsung/include/plat/keypad-core.h
diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index 0761eac..692d01c 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig
@@ -32,6 +32,11 @@ config S5PV210_SETUP_FB_24BPP
help
Common setup code for S5PV210 with an 24bpp RGB display helper.
+config S5PV210_SETUP_KEYPAD
+ bool
+ help
+ Common setup code for keypad.
+
config S5PV210_SETUP_SDHCI
bool
select S5PV210_SETUP_SDHCI_GPIO
diff --git a/arch/arm/mach-s5pv210/Makefile b/arch/arm/mach-s5pv210/Makefile
index 30be9a6..aae592a 100644
--- a/arch/arm/mach-s5pv210/Makefile
+++ b/arch/arm/mach-s5pv210/Makefile
@@ -31,5 +31,6 @@ obj-$(CONFIG_S5PC110_DEV_ONENAND) += dev-onenand.o
obj-$(CONFIG_S5PV210_SETUP_FB_24BPP) += setup-fb-24bpp.o
obj-$(CONFIG_S5PV210_SETUP_I2C1) += setup-i2c1.o
obj-$(CONFIG_S5PV210_SETUP_I2C2) += setup-i2c2.o
+obj-$(CONFIG_S5PV210_SETUP_KEYPAD) += setup-keypad.o
obj-$(CONFIG_S5PV210_SETUP_SDHCI) += setup-sdhci.o
obj-$(CONFIG_S5PV210_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
diff --git a/arch/arm/mach-s5pv210/cpu.c b/arch/arm/mach-s5pv210/cpu.c
index 411a4a9..94c632b 100644
--- a/arch/arm/mach-s5pv210/cpu.c
+++ b/arch/arm/mach-s5pv210/cpu.c
@@ -33,6 +33,7 @@
#include <plat/clock.h>
#include <plat/s5pv210.h>
#include <plat/iic-core.h>
+#include <plat/keypad-core.h>
#include <plat/sdhci.h>
/* Initial IO mappings */
@@ -91,6 +92,9 @@ void __init s5pv210_map_io(void)
s3c_i2c0_setname("s3c2440-i2c");
s3c_i2c1_setname("s3c2440-i2c");
s3c_i2c2_setname("s3c2440-i2c");
+
+ /* Use s5pv210-keypad instead of samsung-keypad */
+ samsung_keypad_setname("s5pv210-keypad");
}
void __init s5pv210_init_clocks(int xtal)
diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-s5pv210/include/mach/map.h
index 34eb168..e2f6e2a 100644
--- a/arch/arm/mach-s5pv210/include/mach/map.h
+++ b/arch/arm/mach-s5pv210/include/mach/map.h
@@ -32,6 +32,8 @@
#define S5PV210_PA_SPI0 0xE1300000
#define S5PV210_PA_SPI1 0xE1400000
+#define S5PV210_PA_KEYPAD (0xE1600000)
+
#define S5PV210_PA_IIC0 (0xE1800000)
#define S5PV210_PA_IIC1 (0xFAB00000)
#define S5PV210_PA_IIC2 (0xE1A00000)
@@ -104,5 +106,6 @@
#define S3C_PA_WDT S5PV210_PA_WATCHDOG
#define SAMSUNG_PA_ADC S5PV210_PA_ADC
+#define SAMSUNG_PA_KEYPAD S5PV210_PA_KEYPAD
#endif /* __ASM_ARCH_MAP_H */
diff --git a/arch/arm/mach-s5pv210/setup-keypad.c b/arch/arm/mach-s5pv210/setup-keypad.c
new file mode 100644
index 0000000..37b2790
--- /dev/null
+++ b/arch/arm/mach-s5pv210/setup-keypad.c
@@ -0,0 +1,34 @@
+/*
+ * linux/arch/arm/mach-s5pv210/setup-keypad.c
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.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.
+ *
+ */
+
+#include <linux/gpio.h>
+#include <plat/gpio-cfg.h>
+
+void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols)
+{
+ unsigned int gpio, end;
+
+ /* Set all the necessary GPH3 pins to special-function 3: KP_ROW[x] */
+ end = S5PV210_GPH3(rows);
+ for (gpio = S5PV210_GPH3(0); gpio < end; gpio++) {
+ s3c_gpio_cfgpin(gpio, S3C_GPIO_SFN(3));
+ s3c_gpio_setpull(gpio, S3C_GPIO_PULL_NONE);
+ }
+
+ /* Set all the necessary GPH2 pins to special-function 3: KP_COL[x] */
+ end = S5PV210_GPH2(cols);
+ for (gpio = S5PV210_GPH2(0); gpio < end; gpio++) {
+ s3c_gpio_cfgpin(gpio, S3C_GPIO_SFN(3));
+ s3c_gpio_setpull(gpio, S3C_GPIO_PULL_NONE);
+ }
+}
diff --git a/arch/arm/plat-samsung/include/plat/keypad-core.h b/arch/arm/plat-samsung/include/plat/keypad-core.h
new file mode 100644
index 0000000..d513e1b
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/keypad-core.h
@@ -0,0 +1,31 @@
+/*
+ * linux/arch/arm/plat-samsung/include/plat/keypad-core.h
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ *
+ * Samsung keypad controller core function
+ *
+ * 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.
+ *
+ */
+
+#ifndef __ASM_ARCH_KEYPAD_CORE_H
+#define __ASM_ARCH_KEYPAD_CORE_H
+
+/* These function are only for use with the core support code, such as
+ * the cpu specific initialisation code
+ */
+
+/* re-define device name depending on support. */
+static inline void samsung_keypad_setname(char *name)
+{
+#ifdef CONFIG_SAMSUNG_DEV_KEYPAD
+ samsung_device_keypad.name = name;
+#endif
+}
+
+#endif /* __ASM_ARCH_KEYPAD_CORE_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 2/3] ARM: S5PV210: Add keypad device helpers
@ 2010-06-21 6:26 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-21 6:26 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds the keypad device platform helpers for S5PV210 cpu.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
arch/arm/mach-s5pv210/Kconfig | 5 +++
arch/arm/mach-s5pv210/Makefile | 1 +
arch/arm/mach-s5pv210/cpu.c | 4 ++
arch/arm/mach-s5pv210/include/mach/map.h | 3 ++
arch/arm/mach-s5pv210/setup-keypad.c | 34 ++++++++++++++++++++++
arch/arm/plat-samsung/include/plat/keypad-core.h | 31 ++++++++++++++++++++
6 files changed, 78 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-s5pv210/setup-keypad.c
create mode 100644 arch/arm/plat-samsung/include/plat/keypad-core.h
diff --git a/arch/arm/mach-s5pv210/Kconfig b/arch/arm/mach-s5pv210/Kconfig
index 0761eac..692d01c 100644
--- a/arch/arm/mach-s5pv210/Kconfig
+++ b/arch/arm/mach-s5pv210/Kconfig
@@ -32,6 +32,11 @@ config S5PV210_SETUP_FB_24BPP
help
Common setup code for S5PV210 with an 24bpp RGB display helper.
+config S5PV210_SETUP_KEYPAD
+ bool
+ help
+ Common setup code for keypad.
+
config S5PV210_SETUP_SDHCI
bool
select S5PV210_SETUP_SDHCI_GPIO
diff --git a/arch/arm/mach-s5pv210/Makefile b/arch/arm/mach-s5pv210/Makefile
index 30be9a6..aae592a 100644
--- a/arch/arm/mach-s5pv210/Makefile
+++ b/arch/arm/mach-s5pv210/Makefile
@@ -31,5 +31,6 @@ obj-$(CONFIG_S5PC110_DEV_ONENAND) += dev-onenand.o
obj-$(CONFIG_S5PV210_SETUP_FB_24BPP) += setup-fb-24bpp.o
obj-$(CONFIG_S5PV210_SETUP_I2C1) += setup-i2c1.o
obj-$(CONFIG_S5PV210_SETUP_I2C2) += setup-i2c2.o
+obj-$(CONFIG_S5PV210_SETUP_KEYPAD) += setup-keypad.o
obj-$(CONFIG_S5PV210_SETUP_SDHCI) += setup-sdhci.o
obj-$(CONFIG_S5PV210_SETUP_SDHCI_GPIO) += setup-sdhci-gpio.o
diff --git a/arch/arm/mach-s5pv210/cpu.c b/arch/arm/mach-s5pv210/cpu.c
index 411a4a9..94c632b 100644
--- a/arch/arm/mach-s5pv210/cpu.c
+++ b/arch/arm/mach-s5pv210/cpu.c
@@ -33,6 +33,7 @@
#include <plat/clock.h>
#include <plat/s5pv210.h>
#include <plat/iic-core.h>
+#include <plat/keypad-core.h>
#include <plat/sdhci.h>
/* Initial IO mappings */
@@ -91,6 +92,9 @@ void __init s5pv210_map_io(void)
s3c_i2c0_setname("s3c2440-i2c");
s3c_i2c1_setname("s3c2440-i2c");
s3c_i2c2_setname("s3c2440-i2c");
+
+ /* Use s5pv210-keypad instead of samsung-keypad */
+ samsung_keypad_setname("s5pv210-keypad");
}
void __init s5pv210_init_clocks(int xtal)
diff --git a/arch/arm/mach-s5pv210/include/mach/map.h b/arch/arm/mach-s5pv210/include/mach/map.h
index 34eb168..e2f6e2a 100644
--- a/arch/arm/mach-s5pv210/include/mach/map.h
+++ b/arch/arm/mach-s5pv210/include/mach/map.h
@@ -32,6 +32,8 @@
#define S5PV210_PA_SPI0 0xE1300000
#define S5PV210_PA_SPI1 0xE1400000
+#define S5PV210_PA_KEYPAD (0xE1600000)
+
#define S5PV210_PA_IIC0 (0xE1800000)
#define S5PV210_PA_IIC1 (0xFAB00000)
#define S5PV210_PA_IIC2 (0xE1A00000)
@@ -104,5 +106,6 @@
#define S3C_PA_WDT S5PV210_PA_WATCHDOG
#define SAMSUNG_PA_ADC S5PV210_PA_ADC
+#define SAMSUNG_PA_KEYPAD S5PV210_PA_KEYPAD
#endif /* __ASM_ARCH_MAP_H */
diff --git a/arch/arm/mach-s5pv210/setup-keypad.c b/arch/arm/mach-s5pv210/setup-keypad.c
new file mode 100644
index 0000000..37b2790
--- /dev/null
+++ b/arch/arm/mach-s5pv210/setup-keypad.c
@@ -0,0 +1,34 @@
+/*
+ * linux/arch/arm/mach-s5pv210/setup-keypad.c
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.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.
+ *
+ */
+
+#include <linux/gpio.h>
+#include <plat/gpio-cfg.h>
+
+void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols)
+{
+ unsigned int gpio, end;
+
+ /* Set all the necessary GPH3 pins to special-function 3: KP_ROW[x] */
+ end = S5PV210_GPH3(rows);
+ for (gpio = S5PV210_GPH3(0); gpio < end; gpio++) {
+ s3c_gpio_cfgpin(gpio, S3C_GPIO_SFN(3));
+ s3c_gpio_setpull(gpio, S3C_GPIO_PULL_NONE);
+ }
+
+ /* Set all the necessary GPH2 pins to special-function 3: KP_COL[x] */
+ end = S5PV210_GPH2(cols);
+ for (gpio = S5PV210_GPH2(0); gpio < end; gpio++) {
+ s3c_gpio_cfgpin(gpio, S3C_GPIO_SFN(3));
+ s3c_gpio_setpull(gpio, S3C_GPIO_PULL_NONE);
+ }
+}
diff --git a/arch/arm/plat-samsung/include/plat/keypad-core.h b/arch/arm/plat-samsung/include/plat/keypad-core.h
new file mode 100644
index 0000000..d513e1b
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/keypad-core.h
@@ -0,0 +1,31 @@
+/*
+ * linux/arch/arm/plat-samsung/include/plat/keypad-core.h
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ *
+ * Samsung keypad controller core function
+ *
+ * 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.
+ *
+ */
+
+#ifndef __ASM_ARCH_KEYPAD_CORE_H
+#define __ASM_ARCH_KEYPAD_CORE_H
+
+/* These function are only for use with the core support code, such as
+ * the cpu specific initialisation code
+ */
+
+/* re-define device name depending on support. */
+static inline void samsung_keypad_setname(char *name)
+{
+#ifdef CONFIG_SAMSUNG_DEV_KEYPAD
+ samsung_device_keypad.name = name;
+#endif
+}
+
+#endif /* __ASM_ARCH_KEYPAD_CORE_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
2010-06-21 6:26 ` Joonyoung Shim
@ 2010-06-21 6:26 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-21 6:26 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux-samsung-soc, linux-input, ben-linux, dmitry.torokhov,
kyungmin.park, kgene.kim
This patch adds support for keypad driver running on Samsung cpus. This
driver is tested on GONI and Aquila board using S5PC110 cpu.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/input/keyboard/Kconfig | 9 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/samsung-keypad.c | 400 +++++++++++++++++++++++++++++++
3 files changed, 410 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/keyboard/samsung-keypad.c
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index d8fa5d7..bf6a50f 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -342,6 +342,15 @@ config KEYBOARD_PXA930_ROTARY
To compile this driver as a module, choose M here: the
module will be called pxa930_rotary.
+config KEYBOARD_SAMSUNG
+ tristate "Samsung keypad support"
+ depends on SAMSUNG_DEV_KEYPAD
+ help
+ Say Y here if you want to use the Samsung keypad.
+
+ To compile this driver as a module, choose M here: the
+ module will be called samsung-keypad.
+
config KEYBOARD_STOWAWAY
tristate "Stowaway keyboard"
select SERIO
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 4596d0c..8f973ed 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_KEYBOARD_OPENCORES) += opencores-kbd.o
obj-$(CONFIG_KEYBOARD_PXA27x) += pxa27x_keypad.o
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_STOWAWAY) += stowaway.o
obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
new file mode 100644
index 0000000..4b56e6f
--- /dev/null
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -0,0 +1,400 @@
+/*
+ * samsung-keypad.c -- Samsung keypad driver
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ * Author: Donghwa Lee <dh09.lee@samsung.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.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <plat/keypad.h>
+#include <plat/regs-keypad.h>
+
+enum samsung_keypad_type {
+ KEYPAD_TYPE_SAMSUNG,
+ KEYPAD_TYPE_S5PV210,
+};
+
+struct samsung_keypad {
+ struct input_dev *input_dev;
+ struct clk *clk;
+ struct delayed_work work;
+ void __iomem *base;
+ unsigned short *keycodes;
+ unsigned int row_shift;
+ unsigned int rows;
+ unsigned int cols;
+ unsigned int row_state[SAMSUNG_MAX_COLS];
+ int irq;
+};
+
+static int samsung_keypad_is_s5pv210(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ enum samsung_keypad_type type;
+
+ type = platform_get_device_id(pdev)->driver_data;
+ return type == KEYPAD_TYPE_S5PV210;
+}
+
+static void samsung_keypad_scan(struct samsung_keypad *keypad,
+ unsigned int *row_state)
+{
+ struct device *dev = keypad->input_dev->dev.parent;
+ unsigned int col;
+ unsigned int val;
+
+ for (col = 0; col < keypad->cols; col++) {
+ if (samsung_keypad_is_s5pv210(dev)) {
+ val = S5PV210_KEYIFCOLEN_MASK;
+ val &= ~(1 << col) << 8;
+ } else {
+ val = SAMSUNG_KEYIFCOL_MASK;
+ val &= ~(1 << col);
+ }
+
+ writel(val, keypad->base + SAMSUNG_KEYIFCOL);
+ mdelay(1);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFROW);
+ row_state[col] = ~val & ((1 << keypad->rows) - 1);
+ }
+
+ /* KEYIFCOL reg clear */
+ writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+}
+
+static void samsung_keypad_worker(struct work_struct *work)
+{
+ struct samsung_keypad *keypad = container_of(work,
+ struct samsung_keypad, work.work);
+ unsigned int row_state[SAMSUNG_MAX_COLS];
+ unsigned int val;
+ unsigned int changed;
+ unsigned int pressed;
+ unsigned int key_down = 0;
+ int col, row;
+
+ clk_enable(keypad->clk);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+ /* interrupt clear */
+ writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ val &= ~(SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ samsung_keypad_scan(keypad, row_state);
+
+ for (col = 0; col < keypad->cols; col++) {
+ changed = row_state[col] ^ keypad->row_state[col];
+ key_down |= row_state[col];
+ if (!changed)
+ continue;
+
+ for (row = 0; row < keypad->rows; row++) {
+ if (!(changed & (1 << row)))
+ continue;
+
+ pressed = row_state[col] & (1 << row);
+
+ dev_dbg(&keypad->input_dev->dev,
+ "key %s, row: %d, col: %d\n",
+ pressed ? "pressed" : "released", row, col);
+
+ val = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
+
+ input_event(keypad->input_dev, EV_MSC, MSC_SCAN, val);
+ input_report_key(keypad->input_dev,
+ keypad->keycodes[val], pressed);
+ input_sync(keypad->input_dev);
+ }
+ }
+ memcpy(keypad->row_state, row_state, sizeof(row_state));
+
+ if (key_down)
+ schedule_delayed_work(&keypad->work, HZ / 20);
+ else {
+ /* enable interrupt bit */
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ val |= (SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+ enable_irq(keypad->irq);
+ }
+ clk_disable(keypad->clk);
+}
+
+static irqreturn_t samsung_keypad_interrupt(int irq, void *dev_id)
+{
+ struct samsung_keypad *keypad = dev_id;
+
+ if (!work_pending(&keypad->work.work)) {
+ disable_irq_nosync(keypad->irq);
+ schedule_delayed_work(&keypad->work, 0);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int __devinit samsung_keypad_probe(struct platform_device *pdev)
+{
+ const struct samsung_keypad_platdata *pdata;
+ const struct matrix_keymap_data *keymap_data;
+ struct samsung_keypad *keypad;
+ struct resource *res;
+ struct input_dev *input_dev;
+ unsigned short *keycodes;
+ unsigned int row_shift;
+ unsigned int val;
+ int ret;
+
+ pdata = pdev->dev.platform_data;
+ if (!pdata) {
+ dev_err(&pdev->dev, "no platform data defined\n");
+ return -EINVAL;
+ }
+
+ keymap_data = pdata->keymap_data;
+ if (!keymap_data) {
+ dev_err(&pdev->dev, "no keymap data defined\n");
+ return -EINVAL;
+ }
+
+ if (!pdata->rows || (pdata->rows > SAMSUNG_MAX_ROWS))
+ return -EINVAL;
+
+ if (!pdata->cols || (pdata->cols > SAMSUNG_MAX_COLS))
+ return -EINVAL;
+
+ /* initialize the gpio */
+ if (pdata->cfg_gpio)
+ pdata->cfg_gpio(pdata->rows, pdata->cols);
+
+ row_shift = get_count_order(pdata->cols);
+ /* alloc with keycodes memory */
+ keypad = kzalloc(sizeof(*keypad) + sizeof(*keycodes) *
+ (pdata->rows << row_shift), GFP_KERNEL);
+ input_dev = input_allocate_device();
+ if (!keypad || !input_dev) {
+ ret = -ENOMEM;
+ goto err_free_mem;
+ }
+ keycodes = (unsigned short *)(keypad + 1);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ ret = -ENODEV;
+ goto err_free_mem;
+ }
+
+ keypad->base = ioremap(res->start, resource_size(res));
+ if (!keypad->base) {
+ ret = -EBUSY;
+ goto err_free_mem;
+ }
+
+ keypad->clk = clk_get(&pdev->dev, "keypad");
+ if (IS_ERR(keypad->clk)) {
+ dev_err(&pdev->dev, "failed to get keypad clk\n");
+ ret = PTR_ERR(keypad->clk);
+ goto err_unmap_base;
+ }
+ clk_enable(keypad->clk);
+
+ keypad->input_dev = input_dev;
+ keypad->keycodes = keycodes;
+ keypad->row_shift = row_shift;
+ keypad->rows = pdata->rows;
+ keypad->cols = pdata->cols;
+
+ INIT_DELAYED_WORK(&keypad->work, samsung_keypad_worker);
+
+ /* enable interrupt and wakeup bit */
+ val = SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN |
+ SAMSUNG_KEYIFCON_WAKEUPEN;
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ /* KEYIFCOL reg clear */
+ writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+
+ keypad->irq = platform_get_irq(pdev, 0);
+ if (keypad->irq < 0) {
+ ret = keypad->irq;
+ goto err_disable_clk;
+ }
+
+ ret = request_irq(keypad->irq, samsung_keypad_interrupt, 0,
+ dev_name(&pdev->dev), keypad);
+
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register keypad interrupt\n");
+ goto err_disable_clk;
+ }
+
+ input_dev->name = pdev->name;
+ input_dev->id.bustype = BUS_HOST;
+ input_dev->dev.parent = &pdev->dev;
+
+ input_dev->evbit[0] = BIT_MASK(EV_KEY);
+ if (!pdata->no_autorepeat)
+ input_dev->evbit[0] |= BIT_MASK(EV_REP);
+
+ input_dev->keycode = keycodes;
+ input_dev->keycodesize = sizeof(*keycodes);
+ input_dev->keycodemax = pdata->rows << row_shift;
+
+ matrix_keypad_build_keymap(keymap_data, row_shift,
+ input_dev->keycode, input_dev->keybit);
+
+ input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+
+ ret = input_register_device(keypad->input_dev);
+ if (ret)
+ goto err_free_irq;
+
+ device_init_wakeup(&pdev->dev, pdata->wakeup);
+ platform_set_drvdata(pdev, keypad);
+ clk_disable(keypad->clk);
+
+ return 0;
+
+err_free_irq:
+ free_irq(keypad->irq, keypad);
+err_disable_clk:
+ clk_disable(keypad->clk);
+ clk_put(keypad->clk);
+err_unmap_base:
+ iounmap(keypad->base);
+err_free_mem:
+ input_free_device(input_dev);
+ kfree(keypad);
+
+ return ret;
+}
+
+static int __devexit samsung_keypad_remove(struct platform_device *pdev)
+{
+ struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+
+ device_init_wakeup(&pdev->dev, 0);
+
+ free_irq(keypad->irq, keypad);
+ cancel_delayed_work_sync(&keypad->work);
+
+ platform_set_drvdata(pdev, NULL);
+ input_unregister_device(keypad->input_dev);
+
+ clk_disable(keypad->clk);
+ clk_put(keypad->clk);
+
+ iounmap(keypad->base);
+ kfree(keypad);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int samsung_keypad_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+
+ disable_irq(keypad->irq);
+
+ if (device_may_wakeup(&pdev->dev))
+ enable_irq_wake(keypad->irq);
+
+ return 0;
+}
+
+static int samsung_keypad_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+ unsigned int val;
+
+ if (device_may_wakeup(&pdev->dev))
+ disable_irq_wake(keypad->irq);
+
+ clk_enable(keypad->clk);
+
+ /* enable interrupt and wakeup bit */
+ val = SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN |
+ SAMSUNG_KEYIFCON_WAKEUPEN;
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ /* KEYIFCOL reg clear */
+ writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+
+ clk_disable(keypad->clk);
+
+ enable_irq(keypad->irq);
+
+ return 0;
+}
+
+static const struct dev_pm_ops samsung_keypad_pm_ops = {
+ .suspend = samsung_keypad_suspend,
+ .resume = samsung_keypad_resume,
+};
+#endif
+
+static struct platform_device_id samsung_keypad_driver_ids[] = {
+ {
+ .name = "samsung-keypad",
+ .driver_data = KEYPAD_TYPE_SAMSUNG,
+ }, {
+ .name = "s5pv210-keypad",
+ .driver_data = KEYPAD_TYPE_S5PV210,
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(platform, samsung_keypad_driver_ids);
+
+static struct platform_driver samsung_keypad_driver = {
+ .probe = samsung_keypad_probe,
+ .remove = __devexit_p(samsung_keypad_remove),
+ .driver = {
+ .name = "samsung-keypad",
+ .owner = THIS_MODULE,
+#ifdef CONFIG_PM
+ .pm = &samsung_keypad_pm_ops,
+#endif
+ },
+ .id_table = samsung_keypad_driver_ids,
+};
+
+static int __init samsung_keypad_init(void)
+{
+ return platform_driver_register(&samsung_keypad_driver);
+}
+
+static void __exit samsung_keypad_exit(void)
+{
+ platform_driver_unregister(&samsung_keypad_driver);
+}
+
+module_init(samsung_keypad_init);
+module_exit(samsung_keypad_exit);
+
+MODULE_DESCRIPTION("Samsung keypad driver");
+MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
+MODULE_AUTHOR("Donghwa Lee <dh09.lee@samsung.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:samsung-keypad");
--
1.7.0.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
@ 2010-06-21 6:26 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-21 6:26 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds support for keypad driver running on Samsung cpus. This
driver is tested on GONI and Aquila board using S5PC110 cpu.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/input/keyboard/Kconfig | 9 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/samsung-keypad.c | 400 +++++++++++++++++++++++++++++++
3 files changed, 410 insertions(+), 0 deletions(-)
create mode 100644 drivers/input/keyboard/samsung-keypad.c
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index d8fa5d7..bf6a50f 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -342,6 +342,15 @@ config KEYBOARD_PXA930_ROTARY
To compile this driver as a module, choose M here: the
module will be called pxa930_rotary.
+config KEYBOARD_SAMSUNG
+ tristate "Samsung keypad support"
+ depends on SAMSUNG_DEV_KEYPAD
+ help
+ Say Y here if you want to use the Samsung keypad.
+
+ To compile this driver as a module, choose M here: the
+ module will be called samsung-keypad.
+
config KEYBOARD_STOWAWAY
tristate "Stowaway keyboard"
select SERIO
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 4596d0c..8f973ed 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_KEYBOARD_OPENCORES) += opencores-kbd.o
obj-$(CONFIG_KEYBOARD_PXA27x) += pxa27x_keypad.o
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_STOWAWAY) += stowaway.o
obj-$(CONFIG_KEYBOARD_SUNKBD) += sunkbd.o
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
new file mode 100644
index 0000000..4b56e6f
--- /dev/null
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -0,0 +1,400 @@
+/*
+ * samsung-keypad.c -- Samsung keypad driver
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ * Author: Donghwa Lee <dh09.lee@samsung.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.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <plat/keypad.h>
+#include <plat/regs-keypad.h>
+
+enum samsung_keypad_type {
+ KEYPAD_TYPE_SAMSUNG,
+ KEYPAD_TYPE_S5PV210,
+};
+
+struct samsung_keypad {
+ struct input_dev *input_dev;
+ struct clk *clk;
+ struct delayed_work work;
+ void __iomem *base;
+ unsigned short *keycodes;
+ unsigned int row_shift;
+ unsigned int rows;
+ unsigned int cols;
+ unsigned int row_state[SAMSUNG_MAX_COLS];
+ int irq;
+};
+
+static int samsung_keypad_is_s5pv210(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ enum samsung_keypad_type type;
+
+ type = platform_get_device_id(pdev)->driver_data;
+ return type == KEYPAD_TYPE_S5PV210;
+}
+
+static void samsung_keypad_scan(struct samsung_keypad *keypad,
+ unsigned int *row_state)
+{
+ struct device *dev = keypad->input_dev->dev.parent;
+ unsigned int col;
+ unsigned int val;
+
+ for (col = 0; col < keypad->cols; col++) {
+ if (samsung_keypad_is_s5pv210(dev)) {
+ val = S5PV210_KEYIFCOLEN_MASK;
+ val &= ~(1 << col) << 8;
+ } else {
+ val = SAMSUNG_KEYIFCOL_MASK;
+ val &= ~(1 << col);
+ }
+
+ writel(val, keypad->base + SAMSUNG_KEYIFCOL);
+ mdelay(1);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFROW);
+ row_state[col] = ~val & ((1 << keypad->rows) - 1);
+ }
+
+ /* KEYIFCOL reg clear */
+ writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+}
+
+static void samsung_keypad_worker(struct work_struct *work)
+{
+ struct samsung_keypad *keypad = container_of(work,
+ struct samsung_keypad, work.work);
+ unsigned int row_state[SAMSUNG_MAX_COLS];
+ unsigned int val;
+ unsigned int changed;
+ unsigned int pressed;
+ unsigned int key_down = 0;
+ int col, row;
+
+ clk_enable(keypad->clk);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+ /* interrupt clear */
+ writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ val &= ~(SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ samsung_keypad_scan(keypad, row_state);
+
+ for (col = 0; col < keypad->cols; col++) {
+ changed = row_state[col] ^ keypad->row_state[col];
+ key_down |= row_state[col];
+ if (!changed)
+ continue;
+
+ for (row = 0; row < keypad->rows; row++) {
+ if (!(changed & (1 << row)))
+ continue;
+
+ pressed = row_state[col] & (1 << row);
+
+ dev_dbg(&keypad->input_dev->dev,
+ "key %s, row: %d, col: %d\n",
+ pressed ? "pressed" : "released", row, col);
+
+ val = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
+
+ input_event(keypad->input_dev, EV_MSC, MSC_SCAN, val);
+ input_report_key(keypad->input_dev,
+ keypad->keycodes[val], pressed);
+ input_sync(keypad->input_dev);
+ }
+ }
+ memcpy(keypad->row_state, row_state, sizeof(row_state));
+
+ if (key_down)
+ schedule_delayed_work(&keypad->work, HZ / 20);
+ else {
+ /* enable interrupt bit */
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ val |= (SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+ enable_irq(keypad->irq);
+ }
+ clk_disable(keypad->clk);
+}
+
+static irqreturn_t samsung_keypad_interrupt(int irq, void *dev_id)
+{
+ struct samsung_keypad *keypad = dev_id;
+
+ if (!work_pending(&keypad->work.work)) {
+ disable_irq_nosync(keypad->irq);
+ schedule_delayed_work(&keypad->work, 0);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int __devinit samsung_keypad_probe(struct platform_device *pdev)
+{
+ const struct samsung_keypad_platdata *pdata;
+ const struct matrix_keymap_data *keymap_data;
+ struct samsung_keypad *keypad;
+ struct resource *res;
+ struct input_dev *input_dev;
+ unsigned short *keycodes;
+ unsigned int row_shift;
+ unsigned int val;
+ int ret;
+
+ pdata = pdev->dev.platform_data;
+ if (!pdata) {
+ dev_err(&pdev->dev, "no platform data defined\n");
+ return -EINVAL;
+ }
+
+ keymap_data = pdata->keymap_data;
+ if (!keymap_data) {
+ dev_err(&pdev->dev, "no keymap data defined\n");
+ return -EINVAL;
+ }
+
+ if (!pdata->rows || (pdata->rows > SAMSUNG_MAX_ROWS))
+ return -EINVAL;
+
+ if (!pdata->cols || (pdata->cols > SAMSUNG_MAX_COLS))
+ return -EINVAL;
+
+ /* initialize the gpio */
+ if (pdata->cfg_gpio)
+ pdata->cfg_gpio(pdata->rows, pdata->cols);
+
+ row_shift = get_count_order(pdata->cols);
+ /* alloc with keycodes memory */
+ keypad = kzalloc(sizeof(*keypad) + sizeof(*keycodes) *
+ (pdata->rows << row_shift), GFP_KERNEL);
+ input_dev = input_allocate_device();
+ if (!keypad || !input_dev) {
+ ret = -ENOMEM;
+ goto err_free_mem;
+ }
+ keycodes = (unsigned short *)(keypad + 1);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ ret = -ENODEV;
+ goto err_free_mem;
+ }
+
+ keypad->base = ioremap(res->start, resource_size(res));
+ if (!keypad->base) {
+ ret = -EBUSY;
+ goto err_free_mem;
+ }
+
+ keypad->clk = clk_get(&pdev->dev, "keypad");
+ if (IS_ERR(keypad->clk)) {
+ dev_err(&pdev->dev, "failed to get keypad clk\n");
+ ret = PTR_ERR(keypad->clk);
+ goto err_unmap_base;
+ }
+ clk_enable(keypad->clk);
+
+ keypad->input_dev = input_dev;
+ keypad->keycodes = keycodes;
+ keypad->row_shift = row_shift;
+ keypad->rows = pdata->rows;
+ keypad->cols = pdata->cols;
+
+ INIT_DELAYED_WORK(&keypad->work, samsung_keypad_worker);
+
+ /* enable interrupt and wakeup bit */
+ val = SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN |
+ SAMSUNG_KEYIFCON_WAKEUPEN;
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ /* KEYIFCOL reg clear */
+ writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+
+ keypad->irq = platform_get_irq(pdev, 0);
+ if (keypad->irq < 0) {
+ ret = keypad->irq;
+ goto err_disable_clk;
+ }
+
+ ret = request_irq(keypad->irq, samsung_keypad_interrupt, 0,
+ dev_name(&pdev->dev), keypad);
+
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register keypad interrupt\n");
+ goto err_disable_clk;
+ }
+
+ input_dev->name = pdev->name;
+ input_dev->id.bustype = BUS_HOST;
+ input_dev->dev.parent = &pdev->dev;
+
+ input_dev->evbit[0] = BIT_MASK(EV_KEY);
+ if (!pdata->no_autorepeat)
+ input_dev->evbit[0] |= BIT_MASK(EV_REP);
+
+ input_dev->keycode = keycodes;
+ input_dev->keycodesize = sizeof(*keycodes);
+ input_dev->keycodemax = pdata->rows << row_shift;
+
+ matrix_keypad_build_keymap(keymap_data, row_shift,
+ input_dev->keycode, input_dev->keybit);
+
+ input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+
+ ret = input_register_device(keypad->input_dev);
+ if (ret)
+ goto err_free_irq;
+
+ device_init_wakeup(&pdev->dev, pdata->wakeup);
+ platform_set_drvdata(pdev, keypad);
+ clk_disable(keypad->clk);
+
+ return 0;
+
+err_free_irq:
+ free_irq(keypad->irq, keypad);
+err_disable_clk:
+ clk_disable(keypad->clk);
+ clk_put(keypad->clk);
+err_unmap_base:
+ iounmap(keypad->base);
+err_free_mem:
+ input_free_device(input_dev);
+ kfree(keypad);
+
+ return ret;
+}
+
+static int __devexit samsung_keypad_remove(struct platform_device *pdev)
+{
+ struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+
+ device_init_wakeup(&pdev->dev, 0);
+
+ free_irq(keypad->irq, keypad);
+ cancel_delayed_work_sync(&keypad->work);
+
+ platform_set_drvdata(pdev, NULL);
+ input_unregister_device(keypad->input_dev);
+
+ clk_disable(keypad->clk);
+ clk_put(keypad->clk);
+
+ iounmap(keypad->base);
+ kfree(keypad);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int samsung_keypad_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+
+ disable_irq(keypad->irq);
+
+ if (device_may_wakeup(&pdev->dev))
+ enable_irq_wake(keypad->irq);
+
+ return 0;
+}
+
+static int samsung_keypad_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+ unsigned int val;
+
+ if (device_may_wakeup(&pdev->dev))
+ disable_irq_wake(keypad->irq);
+
+ clk_enable(keypad->clk);
+
+ /* enable interrupt and wakeup bit */
+ val = SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN |
+ SAMSUNG_KEYIFCON_WAKEUPEN;
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ /* KEYIFCOL reg clear */
+ writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+
+ clk_disable(keypad->clk);
+
+ enable_irq(keypad->irq);
+
+ return 0;
+}
+
+static const struct dev_pm_ops samsung_keypad_pm_ops = {
+ .suspend = samsung_keypad_suspend,
+ .resume = samsung_keypad_resume,
+};
+#endif
+
+static struct platform_device_id samsung_keypad_driver_ids[] = {
+ {
+ .name = "samsung-keypad",
+ .driver_data = KEYPAD_TYPE_SAMSUNG,
+ }, {
+ .name = "s5pv210-keypad",
+ .driver_data = KEYPAD_TYPE_S5PV210,
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(platform, samsung_keypad_driver_ids);
+
+static struct platform_driver samsung_keypad_driver = {
+ .probe = samsung_keypad_probe,
+ .remove = __devexit_p(samsung_keypad_remove),
+ .driver = {
+ .name = "samsung-keypad",
+ .owner = THIS_MODULE,
+#ifdef CONFIG_PM
+ .pm = &samsung_keypad_pm_ops,
+#endif
+ },
+ .id_table = samsung_keypad_driver_ids,
+};
+
+static int __init samsung_keypad_init(void)
+{
+ return platform_driver_register(&samsung_keypad_driver);
+}
+
+static void __exit samsung_keypad_exit(void)
+{
+ platform_driver_unregister(&samsung_keypad_driver);
+}
+
+module_init(samsung_keypad_init);
+module_exit(samsung_keypad_exit);
+
+MODULE_DESCRIPTION("Samsung keypad driver");
+MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
+MODULE_AUTHOR("Donghwa Lee <dh09.lee@samsung.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:samsung-keypad");
--
1.7.0.4
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 6:26 ` Joonyoung Shim
@ 2010-06-21 9:05 ` Eric Miao
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 9:05 UTC (permalink / raw)
To: Joonyoung Shim
Cc: linux-arm-kernel, kgene.kim, dmitry.torokhov, kyungmin.park,
linux-samsung-soc, ben-linux, linux-input
On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> This patch adds samsung keypad device definition for samsung cpus.
>
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> arch/arm/plat-samsung/Kconfig | 5 ++
> arch/arm/plat-samsung/Makefile | 1 +
> arch/arm/plat-samsung/dev-keypad.c | 58 +++++++++++++++++++++
Why need an individual file for a simple device? In the end, these files
will flood over the plat-samsung/ directory. And now think this - in your
new SoC design ,the keypad IP is replaced with a completely new one, does
that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
I personally prefer a single devices.c for all the devices, if you
orgnize well, shouldn't take up much code size in that single file.
> arch/arm/plat-samsung/include/plat/devs.h | 2 +
> arch/arm/plat-samsung/include/plat/keypad.h | 59 ++++++++++++++++++++++
> arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ++++++++++++++++++
Will these registers be used elsewhere except within the keypad driver?
If no, they might be better moved to the keypad driver itself, it makes
the driver more self-contained and avoid the registers being accessed
anywhere.
> 6 files changed, 174 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/plat-samsung/dev-keypad.c
> create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
> create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
>
> diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
> index 2753fb3..bd007e3 100644
> --- a/arch/arm/plat-samsung/Kconfig
> +++ b/arch/arm/plat-samsung/Kconfig
> @@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
> help
> Common in platform device definitions for touchscreen device
>
> +config SAMSUNG_DEV_KEYPAD
> + bool
> + help
> + Compile in platform device definitions for keypad
> +
> # DMA
>
> config S3C_DMA
> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
> index b1d82cc..8269d80 100644
> --- a/arch/arm/plat-samsung/Makefile
> +++ b/arch/arm/plat-samsung/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) += dev-rtc.o
>
> obj-$(CONFIG_SAMSUNG_DEV_ADC) += dev-adc.o
> obj-$(CONFIG_SAMSUNG_DEV_TS) += dev-ts.o
> +obj-$(CONFIG_SAMSUNG_DEV_KEYPAD) += dev-keypad.o
>
> # DMA support
>
> diff --git a/arch/arm/plat-samsung/dev-keypad.c b/arch/arm/plat-samsung/dev-keypad.c
> new file mode 100644
> index 0000000..679b444
> --- /dev/null
> +++ b/arch/arm/plat-samsung/dev-keypad.c
> @@ -0,0 +1,58 @@
> +/*
> + * linux/arch/arm/plat-samsung/dev-keypad.c
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim <jy0922.shim@samsung.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.
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <mach/irqs.h>
> +#include <mach/map.h>
> +#include <plat/cpu.h>
> +#include <plat/devs.h>
> +#include <plat/keypad.h>
> +
> +static struct resource samsung_keypad_resources[] = {
> + [0] = {
> + .start = SAMSUNG_PA_KEYPAD,
> + .end = SAMSUNG_PA_KEYPAD + 0x20 - 1,
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + .start = IRQ_KEYPAD,
> + .end = IRQ_KEYPAD,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +struct platform_device samsung_device_keypad = {
> + .name = "samsung-keypad",
> + .id = -1,
> + .num_resources = ARRAY_SIZE(samsung_keypad_resources),
> + .resource = samsung_keypad_resources,
> +};
> +
> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> +{
> + struct samsung_keypad_platdata *npd;
> +
> + if (!pd) {
> + printk(KERN_ERR "%s: no platform data\n", __func__);
> + return;
> + }
> +
> + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
> + if (!npd)
> + printk(KERN_ERR "%s: no memory for platform data\n", __func__);
This part of the code is actually duplicated again and again and again
for each device, PXA and other legacy platforms are bad references for
this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
major points:
1. A minimum 'struct pxa_device_desc' for a simple description of a
device (more than 90% of the devices can be described that way),
and avoid using a comparatively heavier weight platform_device,
which can be generated at run-time
2. pxa_register_device() to allocate and register the platform_device
at run-time, along with the platform data
3. pxa168_add_*() as a consistent/clearly named API, and with type
checking for the platform_data type (it's a bit difficult to check
type in pxa_register_device())
Just for your reference.
> +
> + if (!npd->cfg_gpio)
> + npd->cfg_gpio = samsung_keypad_cfg_gpio;
More and more platforms are moving their IO-mux to a static descriptive
array and have some API to configure. E.g. PXA, Orion/Kirkwood, iMX....
The idea basically is:
1) IO-Mux and the internal peripheral controller are conceptually
separate entities (the extreme case is the internal peripheral
controller can function normally even without IO-mux being setup
correct, only that signals not proplery routed in/out)
2) IO-mux should be configured before the peripheral drivers are
ready to go (there are cases that IO-mux will have to be changed
at run-time, so IO-mux API should be able to handle that, though
such cases are rare)
3) Peripheral drivers should not be concerned with IO-mux config
Not sure though how the IO-mux is designed in S5P, so treat the above
as reference only.
> +
> + samsung_device_keypad.dev.platform_data = npd;
> +}
> diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
> index e6144e4..6d9f01b 100644
> --- a/arch/arm/plat-samsung/include/plat/devs.h
> +++ b/arch/arm/plat-samsung/include/plat/devs.h
> @@ -100,6 +100,8 @@ extern struct platform_device s5pc100_device_iis0;
> extern struct platform_device s5pc100_device_iis1;
> extern struct platform_device s5pc100_device_iis2;
>
> +extern struct platform_device samsung_device_keypad;
> +
> /* s3c2440 specific devices */
>
> #ifdef CONFIG_CPU_S3C2440
> diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
> new file mode 100644
> index 0000000..6d139d6
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/keypad.h
> @@ -0,0 +1,59 @@
> +/*
> + * linux/arch/arm/plat-samsung/include/plat/keypad.h
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim <jy0922.shim@samsung.com>
> + *
> + * Samsung Platform - Keypad platform data definitions
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __PLAT_SAMSUNG_KEYPAD_H
> +#define __PLAT_SAMSUNG_KEYPAD_H
> +
> +#include <linux/input/matrix_keypad.h>
> +
> +#define SAMSUNG_MAX_ROWS 8
> +#define SAMSUNG_MAX_COLS 8
> +
> +/**
> + * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
> + * @keymap_data: pointer to &matrix_keymap_data.
> + * @rows: number of keypad row supported.
> + * @cols: number of keypad col supported.
> + * @no_autorepeat: disable key autorepeat.
> + * @wakeup: controls whether the device should be set up as wakeup source.
> + * @cfg_gpio: configure the GPIO.
> + *
> + * Initialisation data specific to either the machine or the platform
> + * for the device driver to use or call-back when configuring gpio.
> + */
> +struct samsung_keypad_platdata {
> + const struct matrix_keymap_data *keymap_data;
> + unsigned int rows;
> + unsigned int cols;
> + bool no_autorepeat;
> + bool wakeup;
> +
> + void (*cfg_gpio)(unsigned int rows, unsigned int cols);
> +};
Why are you calling everything samsung_this, samsung_that? Now think about:
1. Is this keypad applicable to all Samsung silicon?
2. Samsung has a new IP for the keypad, now what? samsung_new_keypad_platdata?
s5p_keypad sounds more reasable to me if it's applicable to all the s5p
series. Now I also doubt that's the case. So normally, we will name it
after the first silicon where this IP appears, e.g.
s5pc100_keypad
and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
here.
> +
> +/**
> + * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
> + * @pd: Platform data to register to device.
> + *
> + * Register the given platform data for use with Samsung Keypad device.
> + * The call will copy the platform data, so the board definitions can
> + * make the structure itself __initdata.
> + */
> +extern void samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd);
> +
> +/* defined by architecture to configure gpio. */
> +extern void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols);
> +
> +#endif /* __PLAT_SAMSUNG_KEYPAD_H */
> diff --git a/arch/arm/plat-samsung/include/plat/regs-keypad.h b/arch/arm/plat-samsung/include/plat/regs-keypad.h
> new file mode 100644
> index 0000000..e4688f0
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/regs-keypad.h
> @@ -0,0 +1,49 @@
> +/*
> + * linux/arch/arm/plat-samsung/include/plat/regs-keypad.h
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim <jy0922.shim@samsung.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.
> + *
> + */
> +
> +#ifndef __SAMSUNG_KEYPAD_H__
> +#define __SAMSUNG_KEYPAD_H__
> +
> +#define SAMSUNG_KEYIFCON 0x00
> +#define SAMSUNG_KEYIFSTSCLR 0x04
> +#define SAMSUNG_KEYIFCOL 0x08
> +#define SAMSUNG_KEYIFROW 0x0c
> +#define SAMSUNG_KEYIFFC 0x10
> +
> +/* SAMSUNG_KEYIFCON */
> +#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
> +#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
> +#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
> +#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
> +#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
> +
> +/* SAMSUNG_KEYIFSTSCLR */
> +#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
> +#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
> +#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
> +#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
> +#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
> +#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
> +
> +/* SAMSUNG_KEYIFCOL */
> +#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
> +#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
> +
> +/* SAMSUNG_KEYIFROW */
> +#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
> +#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
> +
> +/* SAMSUNG_KEYIFFC */
> +#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
> +
> +#endif /* __SAMSUNG_KEYPAD_H__ */
> --
> 1.7.0.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 9:05 ` Eric Miao
0 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 9:05 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> This patch adds samsung keypad device definition for samsung cpus.
>
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> ?arch/arm/plat-samsung/Kconfig ? ? ? ? ? ? ? ? ? ?| ? ?5 ++
> ?arch/arm/plat-samsung/Makefile ? ? ? ? ? ? ? ? ? | ? ?1 +
> ?arch/arm/plat-samsung/dev-keypad.c ? ? ? ? ? ? ? | ? 58 +++++++++++++++++++++
Why need an individual file for a simple device? In the end, these files
will flood over the plat-samsung/ directory. And now think this - in your
new SoC design ,the keypad IP is replaced with a completely new one, does
that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
I personally prefer a single devices.c for all the devices, if you
orgnize well, shouldn't take up much code size in that single file.
> ?arch/arm/plat-samsung/include/plat/devs.h ? ? ? ?| ? ?2 +
> ?arch/arm/plat-samsung/include/plat/keypad.h ? ? ?| ? 59 ++++++++++++++++++++++
> ?arch/arm/plat-samsung/include/plat/regs-keypad.h | ? 49 ++++++++++++++++++
Will these registers be used elsewhere except within the keypad driver?
If no, they might be better moved to the keypad driver itself, it makes
the driver more self-contained and avoid the registers being accessed
anywhere.
> ?6 files changed, 174 insertions(+), 0 deletions(-)
> ?create mode 100644 arch/arm/plat-samsung/dev-keypad.c
> ?create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
> ?create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
>
> diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
> index 2753fb3..bd007e3 100644
> --- a/arch/arm/plat-samsung/Kconfig
> +++ b/arch/arm/plat-samsung/Kconfig
> @@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
> ? ? ? ?help
> ? ? ? ? ? ?Common in platform device definitions for touchscreen device
>
> +config SAMSUNG_DEV_KEYPAD
> + ? ? ? bool
> + ? ? ? help
> + ? ? ? ? Compile in platform device definitions for keypad
> +
> ?# DMA
>
> ?config S3C_DMA
> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
> index b1d82cc..8269d80 100644
> --- a/arch/arm/plat-samsung/Makefile
> +++ b/arch/arm/plat-samsung/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) ? ? += dev-rtc.o
>
> ?obj-$(CONFIG_SAMSUNG_DEV_ADC) ?+= dev-adc.o
> ?obj-$(CONFIG_SAMSUNG_DEV_TS) ? += dev-ts.o
> +obj-$(CONFIG_SAMSUNG_DEV_KEYPAD) ? ? ? += dev-keypad.o
>
> ?# DMA support
>
> diff --git a/arch/arm/plat-samsung/dev-keypad.c b/arch/arm/plat-samsung/dev-keypad.c
> new file mode 100644
> index 0000000..679b444
> --- /dev/null
> +++ b/arch/arm/plat-samsung/dev-keypad.c
> @@ -0,0 +1,58 @@
> +/*
> + * linux/arch/arm/plat-samsung/dev-keypad.c
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim <jy0922.shim@samsung.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.
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <mach/irqs.h>
> +#include <mach/map.h>
> +#include <plat/cpu.h>
> +#include <plat/devs.h>
> +#include <plat/keypad.h>
> +
> +static struct resource samsung_keypad_resources[] = {
> + ? ? ? [0] = {
> + ? ? ? ? ? ? ? .start ?= SAMSUNG_PA_KEYPAD,
> + ? ? ? ? ? ? ? .end ? ?= SAMSUNG_PA_KEYPAD + 0x20 - 1,
> + ? ? ? ? ? ? ? .flags ?= IORESOURCE_MEM,
> + ? ? ? },
> + ? ? ? [1] = {
> + ? ? ? ? ? ? ? .start ?= IRQ_KEYPAD,
> + ? ? ? ? ? ? ? .end ? ?= IRQ_KEYPAD,
> + ? ? ? ? ? ? ? .flags ?= IORESOURCE_IRQ,
> + ? ? ? },
> +};
> +
> +struct platform_device samsung_device_keypad = {
> + ? ? ? .name ? ? ? ? ? = "samsung-keypad",
> + ? ? ? .id ? ? ? ? ? ? = -1,
> + ? ? ? .num_resources ?= ARRAY_SIZE(samsung_keypad_resources),
> + ? ? ? .resource ? ? ? = samsung_keypad_resources,
> +};
> +
> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> +{
> + ? ? ? struct samsung_keypad_platdata *npd;
> +
> + ? ? ? if (!pd) {
> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
> + ? ? ? ? ? ? ? return;
> + ? ? ? }
> +
> + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
> + ? ? ? if (!npd)
> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
This part of the code is actually duplicated again and again and again
for each device, PXA and other legacy platforms are bad references for
this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
major points:
1. A minimum 'struct pxa_device_desc' for a simple description of a
device (more than 90% of the devices can be described that way),
and avoid using a comparatively heavier weight platform_device,
which can be generated at run-time
2. pxa_register_device() to allocate and register the platform_device
at run-time, along with the platform data
3. pxa168_add_*() as a consistent/clearly named API, and with type
checking for the platform_data type (it's a bit difficult to check
type in pxa_register_device())
Just for your reference.
> +
> + ? ? ? if (!npd->cfg_gpio)
> + ? ? ? ? ? ? ? npd->cfg_gpio = samsung_keypad_cfg_gpio;
More and more platforms are moving their IO-mux to a static descriptive
array and have some API to configure. E.g. PXA, Orion/Kirkwood, iMX....
The idea basically is:
1) IO-Mux and the internal peripheral controller are conceptually
separate entities (the extreme case is the internal peripheral
controller can function normally even without IO-mux being setup
correct, only that signals not proplery routed in/out)
2) IO-mux should be configured before the peripheral drivers are
ready to go (there are cases that IO-mux will have to be changed
at run-time, so IO-mux API should be able to handle that, though
such cases are rare)
3) Peripheral drivers should not be concerned with IO-mux config
Not sure though how the IO-mux is designed in S5P, so treat the above
as reference only.
> +
> + ? ? ? samsung_device_keypad.dev.platform_data = npd;
> +}
> diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
> index e6144e4..6d9f01b 100644
> --- a/arch/arm/plat-samsung/include/plat/devs.h
> +++ b/arch/arm/plat-samsung/include/plat/devs.h
> @@ -100,6 +100,8 @@ extern struct platform_device s5pc100_device_iis0;
> ?extern struct platform_device s5pc100_device_iis1;
> ?extern struct platform_device s5pc100_device_iis2;
>
> +extern struct platform_device samsung_device_keypad;
> +
> ?/* s3c2440 specific devices */
>
> ?#ifdef CONFIG_CPU_S3C2440
> diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
> new file mode 100644
> index 0000000..6d139d6
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/keypad.h
> @@ -0,0 +1,59 @@
> +/*
> + * linux/arch/arm/plat-samsung/include/plat/keypad.h
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim <jy0922.shim@samsung.com>
> + *
> + * Samsung Platform - Keypad platform data definitions
> + *
> + * ?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.
> + *
> + */
> +
> +#ifndef __PLAT_SAMSUNG_KEYPAD_H
> +#define __PLAT_SAMSUNG_KEYPAD_H
> +
> +#include <linux/input/matrix_keypad.h>
> +
> +#define SAMSUNG_MAX_ROWS ? ? ? 8
> +#define SAMSUNG_MAX_COLS ? ? ? 8
> +
> +/**
> + * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
> + * @keymap_data: pointer to &matrix_keymap_data.
> + * @rows: number of keypad row supported.
> + * @cols: number of keypad col supported.
> + * @no_autorepeat: disable key autorepeat.
> + * @wakeup: controls whether the device should be set up as wakeup source.
> + * @cfg_gpio: configure the GPIO.
> + *
> + * Initialisation data specific to either the machine or the platform
> + * for the device driver to use or call-back when configuring gpio.
> + */
> +struct samsung_keypad_platdata {
> + ? ? ? const struct matrix_keymap_data *keymap_data;
> + ? ? ? unsigned int ? ? ? ? ? ?rows;
> + ? ? ? unsigned int ? ? ? ? ? ?cols;
> + ? ? ? bool ? ? ? ? ? ? ? ? ? ?no_autorepeat;
> + ? ? ? bool ? ? ? ? ? ? ? ? ? ?wakeup;
> +
> + ? ? ? void ? ?(*cfg_gpio)(unsigned int rows, unsigned int cols);
> +};
Why are you calling everything samsung_this, samsung_that? Now think about:
1. Is this keypad applicable to all Samsung silicon?
2. Samsung has a new IP for the keypad, now what? samsung_new_keypad_platdata?
s5p_keypad sounds more reasable to me if it's applicable to all the s5p
series. Now I also doubt that's the case. So normally, we will name it
after the first silicon where this IP appears, e.g.
s5pc100_keypad
and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
here.
> +
> +/**
> + * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
> + * @pd: Platform data to register to device.
> + *
> + * Register the given platform data for use with Samsung Keypad device.
> + * The call will copy the platform data, so the board definitions can
> + * make the structure itself __initdata.
> + */
> +extern void samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd);
> +
> +/* defined by architecture to configure gpio. */
> +extern void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols);
> +
> +#endif /* __PLAT_SAMSUNG_KEYPAD_H */
> diff --git a/arch/arm/plat-samsung/include/plat/regs-keypad.h b/arch/arm/plat-samsung/include/plat/regs-keypad.h
> new file mode 100644
> index 0000000..e4688f0
> --- /dev/null
> +++ b/arch/arm/plat-samsung/include/plat/regs-keypad.h
> @@ -0,0 +1,49 @@
> +/*
> + * linux/arch/arm/plat-samsung/include/plat/regs-keypad.h
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim <jy0922.shim@samsung.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.
> + *
> + */
> +
> +#ifndef __SAMSUNG_KEYPAD_H__
> +#define __SAMSUNG_KEYPAD_H__
> +
> +#define SAMSUNG_KEYIFCON ? ? ? ? ? ? ? ? ? ? ? 0x00
> +#define SAMSUNG_KEYIFSTSCLR ? ? ? ? ? ? ? ? ? ?0x04
> +#define SAMSUNG_KEYIFCOL ? ? ? ? ? ? ? ? ? ? ? 0x08
> +#define SAMSUNG_KEYIFROW ? ? ? ? ? ? ? ? ? ? ? 0x0c
> +#define SAMSUNG_KEYIFFC ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?0x10
> +
> +/* SAMSUNG_KEYIFCON */
> +#define SAMSUNG_KEYIFCON_INT_F_EN ? ? ? ? ? ? ?(1 << 0)
> +#define SAMSUNG_KEYIFCON_INT_R_EN ? ? ? ? ? ? ?(1 << 1)
> +#define SAMSUNG_KEYIFCON_DF_EN ? ? ? ? ? ? ? ? (1 << 2)
> +#define SAMSUNG_KEYIFCON_FC_EN ? ? ? ? ? ? ? ? (1 << 3)
> +#define SAMSUNG_KEYIFCON_WAKEUPEN ? ? ? ? ? ? ?(1 << 4)
> +
> +/* SAMSUNG_KEYIFSTSCLR */
> +#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK ? ? ? ? (0xff << 0)
> +#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK ? ? ? ? (0xff << 8)
> +#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET ? ? ? 8
> +#define S5PV210_KEYIFSTSCLR_P_INT_MASK ? ? ? ? (0x3fff << 0)
> +#define S5PV210_KEYIFSTSCLR_R_INT_MASK ? ? ? ? (0x3fff << 16)
> +#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET ? ? ? 16
> +
> +/* SAMSUNG_KEYIFCOL */
> +#define SAMSUNG_KEYIFCOL_MASK ? ? ? ? ? ? ? ? ?(0xff << 0)
> +#define S5PV210_KEYIFCOLEN_MASK ? ? ? ? ? ? ? ? ? ? ? ?(0xff << 8)
> +
> +/* SAMSUNG_KEYIFROW */
> +#define SAMSUNG_KEYIFROW_MASK ? ? ? ? ? ? ? ? ?(0xff << 0)
> +#define S5PV210_KEYIFROW_MASK ? ? ? ? ? ? ? ? ?(0x3fff << 0)
> +
> +/* SAMSUNG_KEYIFFC */
> +#define SAMSUNG_KEYIFFC_MASK ? ? ? ? ? ? ? ? ? (0x3ff << 0)
> +
> +#endif /* __SAMSUNG_KEYPAD_H__ */
> --
> 1.7.0.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 9:05 ` Eric Miao
@ 2010-06-21 9:19 ` Russell King - ARM Linux
-1 siblings, 0 replies; 50+ messages in thread
From: Russell King - ARM Linux @ 2010-06-21 9:19 UTC (permalink / raw)
To: Eric Miao
Cc: Joonyoung Shim, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> > +{
> > + struct samsung_keypad_platdata *npd;
> > +
> > + if (!pd) {
> > + printk(KERN_ERR "%s: no platform data\n", __func__);
> > + return;
> > + }
> > +
> > + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
> > + if (!npd)
> > + printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
>
> 1. A minimum 'struct pxa_device_desc' for a simple description of a
> device (more than 90% of the devices can be described that way),
> and avoid using a comparatively heavier weight platform_device,
> which can be generated at run-time
>
> 2. pxa_register_device() to allocate and register the platform_device
> at run-time, along with the platform data
It's a bad idea to make platform data be run-time discardable like this:
> > +struct samsung_keypad_platdata {
> > + const struct matrix_keymap_data *keymap_data;
What you end up with is some platform data structures which must be kept
(those which have pointers to them from the platform data), and others
(the platform data itself) which can be discarded at runtime.
We know that the __initdata attributations cause lots of problems -
they're frequently wrong. Just see the constant hastle with __devinit
et.al. The same issue happens with __initdata as well.
So why make things more complicated by allowing some platform data
structures to be discardable and others not to be? Is their small
size (maybe 6 words for this one) really worth the hastle of getting
__initdata attributations wrong (eg, on the keymap data?)
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 9:19 ` Russell King - ARM Linux
0 siblings, 0 replies; 50+ messages in thread
From: Russell King - ARM Linux @ 2010-06-21 9:19 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> > +{
> > + ? ? ? struct samsung_keypad_platdata *npd;
> > +
> > + ? ? ? if (!pd) {
> > + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
> > + ? ? ? ? ? ? ? return;
> > + ? ? ? }
> > +
> > + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
> > + ? ? ? if (!npd)
> > + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
>
> 1. A minimum 'struct pxa_device_desc' for a simple description of a
> device (more than 90% of the devices can be described that way),
> and avoid using a comparatively heavier weight platform_device,
> which can be generated at run-time
>
> 2. pxa_register_device() to allocate and register the platform_device
> at run-time, along with the platform data
It's a bad idea to make platform data be run-time discardable like this:
> > +struct samsung_keypad_platdata {
> > + ? ? ? const struct matrix_keymap_data *keymap_data;
What you end up with is some platform data structures which must be kept
(those which have pointers to them from the platform data), and others
(the platform data itself) which can be discarded at runtime.
We know that the __initdata attributations cause lots of problems -
they're frequently wrong. Just see the constant hastle with __devinit
et.al. The same issue happens with __initdata as well.
So why make things more complicated by allowing some platform data
structures to be discardable and others not to be? Is their small
size (maybe 6 words for this one) really worth the hastle of getting
__initdata attributations wrong (eg, on the keymap data?)
^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 9:05 ` Eric Miao
@ 2010-06-21 9:29 ` Marek Szyprowski
-1 siblings, 0 replies; 50+ messages in thread
From: Marek Szyprowski @ 2010-06-21 9:29 UTC (permalink / raw)
To: 'Eric Miao', 'Joonyoung Shim'
Cc: linux-arm-kernel, kgene.kim, dmitry.torokhov, kyungmin.park,
linux-samsung-soc, ben-linux, linux-input
Hello,
On Monday, June 21, 2010 11:06 AM Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com>
> wrote:
> > This patch adds samsung keypad device definition for samsung cpus.
> >
> > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > arch/arm/plat-samsung/Kconfig | 5 ++
> > arch/arm/plat-samsung/Makefile | 1 +
> > arch/arm/plat-samsung/dev-keypad.c | 58
> +++++++++++++++++++++
>
> Why need an individual file for a simple device? In the end, these files
> will flood over the plat-samsung/ directory. And now think this - in your
> new SoC design ,the keypad IP is replaced with a completely new one, does
> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>
> I personally prefer a single devices.c for all the devices, if you
> orgnize well, shouldn't take up much code size in that single file.
A separate dev-abc.c files has been chosen by a platform maintainer. This
has some advantages as some unused data can be easily not compiled into
the kernel which is configured specially for a particular board.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 9:29 ` Marek Szyprowski
0 siblings, 0 replies; 50+ messages in thread
From: Marek Szyprowski @ 2010-06-21 9:29 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Monday, June 21, 2010 11:06 AM Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com>
> wrote:
> > This patch adds samsung keypad device definition for samsung cpus.
> >
> > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > arch/arm/plat-samsung/Kconfig | 5 ++
> > arch/arm/plat-samsung/Makefile | 1 +
> > arch/arm/plat-samsung/dev-keypad.c | 58
> +++++++++++++++++++++
>
> Why need an individual file for a simple device? In the end, these files
> will flood over the plat-samsung/ directory. And now think this - in your
> new SoC design ,the keypad IP is replaced with a completely new one, does
> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>
> I personally prefer a single devices.c for all the devices, if you
> orgnize well, shouldn't take up much code size in that single file.
A separate dev-abc.c files has been chosen by a platform maintainer. This
has some advantages as some unused data can be easily not compiled into
the kernel which is configured specially for a particular board.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 9:19 ` Russell King - ARM Linux
@ 2010-06-21 10:39 ` Eric Miao
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 10:39 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Joonyoung Shim, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>> > +{
>> > + struct samsung_keypad_platdata *npd;
>> > +
>> > + if (!pd) {
>> > + printk(KERN_ERR "%s: no platform data\n", __func__);
>> > + return;
>> > + }
>> > +
>> > + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>> > + if (!npd)
>> > + printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>
>> This part of the code is actually duplicated again and again and again
>> for each device, PXA and other legacy platforms are bad references for
>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> major points:
>>
>> 1. A minimum 'struct pxa_device_desc' for a simple description of a
>> device (more than 90% of the devices can be described that way),
>> and avoid using a comparatively heavier weight platform_device,
>> which can be generated at run-time
>>
>> 2. pxa_register_device() to allocate and register the platform_device
>> at run-time, along with the platform data
>
> It's a bad idea to make platform data be run-time discardable like this:
>
>> > +struct samsung_keypad_platdata {
>> > + const struct matrix_keymap_data *keymap_data;
>
> What you end up with is some platform data structures which must be kept
> (those which have pointers to them from the platform data), and others
> (the platform data itself) which can be discarded at runtime.
>
> We know that the __initdata attributations cause lots of problems -
> they're frequently wrong. Just see the constant hastle with __devinit
> et.al. The same issue happens with __initdata as well.
>
> So why make things more complicated by allowing some platform data
> structures to be discardable and others not to be? Is their small
> size (maybe 6 words for this one) really worth the hastle of getting
> __initdata attributations wrong (eg, on the keymap data?)
>
Russell,
The benefit I see is when multiple boards are compiled in, those
data not used can be automatically discarded.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 10:39 ` Eric Miao
0 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 10:39 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>> > +{
>> > + ? ? ? struct samsung_keypad_platdata *npd;
>> > +
>> > + ? ? ? if (!pd) {
>> > + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>> > + ? ? ? ? ? ? ? return;
>> > + ? ? ? }
>> > +
>> > + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>> > + ? ? ? if (!npd)
>> > + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>
>> This part of the code is actually duplicated again and again and again
>> for each device, PXA and other legacy platforms are bad references for
>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> major points:
>>
>> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>> ? ? device (more than 90% of the devices can be described that way),
>> ? ? and avoid using a comparatively heavier weight platform_device,
>> ? ? which can be generated at run-time
>>
>> ?2. pxa_register_device() to allocate and register the platform_device
>> ? ? at run-time, along with the platform data
>
> It's a bad idea to make platform data be run-time discardable like this:
>
>> > +struct samsung_keypad_platdata {
>> > + ? ? ? const struct matrix_keymap_data *keymap_data;
>
> What you end up with is some platform data structures which must be kept
> (those which have pointers to them from the platform data), and others
> (the platform data itself) which can be discarded at runtime.
>
> We know that the __initdata attributations cause lots of problems -
> they're frequently wrong. ?Just see the constant hastle with __devinit
> et.al. ?The same issue happens with __initdata as well.
>
> So why make things more complicated by allowing some platform data
> structures to be discardable and others not to be? ?Is their small
> size (maybe 6 words for this one) really worth the hastle of getting
> __initdata attributations wrong (eg, on the keymap data?)
>
Russell,
The benefit I see is when multiple boards are compiled in, those
data not used can be automatically discarded.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 9:29 ` Marek Szyprowski
@ 2010-06-21 10:43 ` Eric Miao
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 10:43 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Joonyoung Shim, linux-arm-kernel, kgene.kim, dmitry.torokhov,
kyungmin.park, linux-samsung-soc, ben-linux, linux-input
On Mon, Jun 21, 2010 at 5:29 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hello,
>
> On Monday, June 21, 2010 11:06 AM Eric Miao wrote:
>
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com>
>> wrote:
>> > This patch adds samsung keypad device definition for samsung cpus.
>> >
>> > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> > ---
>> > arch/arm/plat-samsung/Kconfig | 5 ++
>> > arch/arm/plat-samsung/Makefile | 1 +
>> > arch/arm/plat-samsung/dev-keypad.c | 58
>> +++++++++++++++++++++
>>
>> Why need an individual file for a simple device? In the end, these files
>> will flood over the plat-samsung/ directory. And now think this - in your
>> new SoC design ,the keypad IP is replaced with a completely new one, does
>> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>>
>> I personally prefer a single devices.c for all the devices, if you
>> orgnize well, shouldn't take up much code size in that single file.
>
> A separate dev-abc.c files has been chosen by a platform maintainer. This
> has some advantages as some unused data can be easily not compiled into
> the kernel which is configured specially for a particular board.
This also can be done by something like below:
#if defined(CONFIG_S5P_KEYPAD) || defined(CONFIG_S5P_KEYPAD_MODULE)
struct platform_device s5p_device_keypad {
........
};
#endif
And if you want run-time discardable platform_device, you have to mark
this structure as __initdata, and duplicate that device when registering.
That's why in arch/arm/mach-mmp/, a light weight and descriptive
'struct pxa_device_desc' is introduced, so only those platform_devices
registered will be generated and registered.
--
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 10:43 ` Eric Miao
0 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 10:43 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 21, 2010 at 5:29 PM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hello,
>
> On Monday, June 21, 2010 11:06 AM Eric Miao wrote:
>
>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com>
>> wrote:
>> > This patch adds samsung keypad device definition for samsung cpus.
>> >
>> > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> > ---
>> > ?arch/arm/plat-samsung/Kconfig ? ? ? ? ? ? ? ? ? ?| ? ?5 ++
>> > ?arch/arm/plat-samsung/Makefile ? ? ? ? ? ? ? ? ? | ? ?1 +
>> > ?arch/arm/plat-samsung/dev-keypad.c ? ? ? ? ? ? ? | ? 58
>> +++++++++++++++++++++
>>
>> Why need an individual file for a simple device? ?In the end, these files
>> will flood over the plat-samsung/ directory. And now think this - in your
>> new SoC design ,the keypad IP is replaced with a completely new one, does
>> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>>
>> I personally prefer a single devices.c for all the devices, if you
>> orgnize well, shouldn't take up much code size in that single file.
>
> A separate dev-abc.c files has been chosen by a platform maintainer. This
> has some advantages as some unused data can be easily not compiled into
> the kernel which is configured specially for a particular board.
This also can be done by something like below:
#if defined(CONFIG_S5P_KEYPAD) || defined(CONFIG_S5P_KEYPAD_MODULE)
struct platform_device s5p_device_keypad {
........
};
#endif
And if you want run-time discardable platform_device, you have to mark
this structure as __initdata, and duplicate that device when registering.
That's why in arch/arm/mach-mmp/, a light weight and descriptive
'struct pxa_device_desc' is introduced, so only those platform_devices
registered will be generated and registered.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 10:39 ` Eric Miao
@ 2010-06-21 11:16 ` Russell King - ARM Linux
-1 siblings, 0 replies; 50+ messages in thread
From: Russell King - ARM Linux @ 2010-06-21 11:16 UTC (permalink / raw)
To: Eric Miao
Cc: Joonyoung Shim, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> >> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> >> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> >> > +{
> >> > + struct samsung_keypad_platdata *npd;
> >> > +
> >> > + if (!pd) {
> >> > + printk(KERN_ERR "%s: no platform data\n", __func__);
> >> > + return;
> >> > + }
> >> > +
> >> > + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
> >> > + if (!npd)
> >> > + printk(KERN_ERR "%s: no memory for platform data\n", __func__);
> >>
> >> This part of the code is actually duplicated again and again and again
> >> for each device, PXA and other legacy platforms are bad references for
> >> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> >> major points:
> >>
> >> 1. A minimum 'struct pxa_device_desc' for a simple description of a
> >> device (more than 90% of the devices can be described that way),
> >> and avoid using a comparatively heavier weight platform_device,
> >> which can be generated at run-time
> >>
> >> 2. pxa_register_device() to allocate and register the platform_device
> >> at run-time, along with the platform data
> >
> > It's a bad idea to make platform data be run-time discardable like this:
> >
> >> > +struct samsung_keypad_platdata {
> >> > + const struct matrix_keymap_data *keymap_data;
> >
> > What you end up with is some platform data structures which must be kept
> > (those which have pointers to them from the platform data), and others
> > (the platform data itself) which can be discarded at runtime.
> >
> > We know that the __initdata attributations cause lots of problems -
> > they're frequently wrong. Just see the constant hastle with __devinit
> > et.al. The same issue happens with __initdata as well.
> >
> > So why make things more complicated by allowing some platform data
> > structures to be discardable and others not to be? Is their small
> > size (maybe 6 words for this one) really worth the hastle of getting
> > __initdata attributations wrong (eg, on the keymap data?)
> >
>
> Russell,
>
> The benefit I see is when multiple boards are compiled in, those
> data not used can be automatically discarded.
Yes, but only some of the data can be discarded. Continuing with the
example in hand, while you can discard the six words which represent
samsung_keypad_platdata, but the keymap_data can't be because that won't
be re-allocated, which is probably a much larger data structure.
So, samsung_keypad_platdata can be marked with __initdata, but the keymap
data must not be - and this is where the confusion about what can be
marked with __initdata starts - and eventually leads to the keymap data
also being mistakenly marked __initdata.
Is saving six words (or so) worth the effort to track down stuff
inappropriately marked with __initdata ? I'm sure there's bigger savings
to be had in other parts of the kernel (such as shrinking the size of
tcp hash tables, reducing the kernel message ring buffer, etc) if size is
really a concern.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 11:16 ` Russell King - ARM Linux
0 siblings, 0 replies; 50+ messages in thread
From: Russell King - ARM Linux @ 2010-06-21 11:16 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
> >> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> >> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
> >> > +{
> >> > + ? ? ? struct samsung_keypad_platdata *npd;
> >> > +
> >> > + ? ? ? if (!pd) {
> >> > + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
> >> > + ? ? ? ? ? ? ? return;
> >> > + ? ? ? }
> >> > +
> >> > + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
> >> > + ? ? ? if (!npd)
> >> > + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
> >>
> >> This part of the code is actually duplicated again and again and again
> >> for each device, PXA and other legacy platforms are bad references for
> >> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> >> major points:
> >>
> >> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
> >> ? ? device (more than 90% of the devices can be described that way),
> >> ? ? and avoid using a comparatively heavier weight platform_device,
> >> ? ? which can be generated at run-time
> >>
> >> ?2. pxa_register_device() to allocate and register the platform_device
> >> ? ? at run-time, along with the platform data
> >
> > It's a bad idea to make platform data be run-time discardable like this:
> >
> >> > +struct samsung_keypad_platdata {
> >> > + ? ? ? const struct matrix_keymap_data *keymap_data;
> >
> > What you end up with is some platform data structures which must be kept
> > (those which have pointers to them from the platform data), and others
> > (the platform data itself) which can be discarded at runtime.
> >
> > We know that the __initdata attributations cause lots of problems -
> > they're frequently wrong. ?Just see the constant hastle with __devinit
> > et.al. ?The same issue happens with __initdata as well.
> >
> > So why make things more complicated by allowing some platform data
> > structures to be discardable and others not to be? ?Is their small
> > size (maybe 6 words for this one) really worth the hastle of getting
> > __initdata attributations wrong (eg, on the keymap data?)
> >
>
> Russell,
>
> The benefit I see is when multiple boards are compiled in, those
> data not used can be automatically discarded.
Yes, but only some of the data can be discarded. Continuing with the
example in hand, while you can discard the six words which represent
samsung_keypad_platdata, but the keymap_data can't be because that won't
be re-allocated, which is probably a much larger data structure.
So, samsung_keypad_platdata can be marked with __initdata, but the keymap
data must not be - and this is where the confusion about what can be
marked with __initdata starts - and eventually leads to the keymap data
also being mistakenly marked __initdata.
Is saving six words (or so) worth the effort to track down stuff
inappropriately marked with __initdata ? I'm sure there's bigger savings
to be had in other parts of the kernel (such as shrinking the size of
tcp hash tables, reducing the kernel message ring buffer, etc) if size is
really a concern.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 9:05 ` Eric Miao
@ 2010-06-21 11:21 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-21 11:21 UTC (permalink / raw)
To: Eric Miao
Cc: linux-samsung-soc, dmitry.torokhov, kyungmin.park, kgene.kim,
ben-linux, linux-input, linux-arm-kernel
On 6/21/2010 6:05 PM, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> This patch adds samsung keypad device definition for samsung cpus.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> arch/arm/plat-samsung/Kconfig | 5 ++
>> arch/arm/plat-samsung/Makefile | 1 +
>> arch/arm/plat-samsung/dev-keypad.c | 58 +++++++++++++++++++++
>
> Why need an individual file for a simple device? In the end, these files
> will flood over the plat-samsung/ directory. And now think this - in your
> new SoC design ,the keypad IP is replaced with a completely new one, does
> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>
> I personally prefer a single devices.c for all the devices, if you
> orgnize well, shouldn't take up much code size in that single file.
>
I know, but will need more effort of other device as well as keypad to
do it.
>> arch/arm/plat-samsung/include/plat/devs.h | 2 +
>> arch/arm/plat-samsung/include/plat/keypad.h | 59 ++++++++++++++++++++++
>> arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ++++++++++++++++++
>
> Will these registers be used elsewhere except within the keypad driver?
> If no, they might be better moved to the keypad driver itself, it makes
> the driver more self-contained and avoid the registers being accessed
> anywhere.
>
Right, these registers are used only in the keypad driver. I think it's
possible.
>> 6 files changed, 174 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/plat-samsung/dev-keypad.c
>> create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
>> create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
>>
>> diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
>> index 2753fb3..bd007e3 100644
>> --- a/arch/arm/plat-samsung/Kconfig
>> +++ b/arch/arm/plat-samsung/Kconfig
>> @@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
>> help
>> Common in platform device definitions for touchscreen device
>>
>> +config SAMSUNG_DEV_KEYPAD
>> + bool
>> + help
>> + Compile in platform device definitions for keypad
>> +
>> # DMA
>>
>> config S3C_DMA
>> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
>> index b1d82cc..8269d80 100644
>> --- a/arch/arm/plat-samsung/Makefile
>> +++ b/arch/arm/plat-samsung/Makefile
>> @@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) += dev-rtc.o
>>
>> obj-$(CONFIG_SAMSUNG_DEV_ADC) += dev-adc.o
>> obj-$(CONFIG_SAMSUNG_DEV_TS) += dev-ts.o
>> +obj-$(CONFIG_SAMSUNG_DEV_KEYPAD) += dev-keypad.o
>>
>> # DMA support
>>
>> diff --git a/arch/arm/plat-samsung/dev-keypad.c b/arch/arm/plat-samsung/dev-keypad.c
>> new file mode 100644
>> index 0000000..679b444
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/dev-keypad.c
>> @@ -0,0 +1,58 @@
>> +/*
>> + * linux/arch/arm/plat-samsung/dev-keypad.c
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
>> + * Author: Joonyoung Shim <jy0922.shim@samsung.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.
>> + *
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <mach/irqs.h>
>> +#include <mach/map.h>
>> +#include <plat/cpu.h>
>> +#include <plat/devs.h>
>> +#include <plat/keypad.h>
>> +
>> +static struct resource samsung_keypad_resources[] = {
>> + [0] = {
>> + .start = SAMSUNG_PA_KEYPAD,
>> + .end = SAMSUNG_PA_KEYPAD + 0x20 - 1,
>> + .flags = IORESOURCE_MEM,
>> + },
>> + [1] = {
>> + .start = IRQ_KEYPAD,
>> + .end = IRQ_KEYPAD,
>> + .flags = IORESOURCE_IRQ,
>> + },
>> +};
>> +
>> +struct platform_device samsung_device_keypad = {
>> + .name = "samsung-keypad",
>> + .id = -1,
>> + .num_resources = ARRAY_SIZE(samsung_keypad_resources),
>> + .resource = samsung_keypad_resources,
>> +};
>> +
>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>> +{
>> + struct samsung_keypad_platdata *npd;
>> +
>> + if (!pd) {
>> + printk(KERN_ERR "%s: no platform data\n", __func__);
>> + return;
>> + }
>> +
>> + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>> + if (!npd)
>> + printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
>
I know ben posted patches to remove duplicated codes such this.
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg01474.html
> 1. A minimum 'struct pxa_device_desc' for a simple description of a
> device (more than 90% of the devices can be described that way),
> and avoid using a comparatively heavier weight platform_device,
> which can be generated at run-time
>
> 2. pxa_register_device() to allocate and register the platform_device
> at run-time, along with the platform data
>
> 3. pxa168_add_*() as a consistent/clearly named API, and with type
> checking for the platform_data type (it's a bit difficult to check
> type in pxa_register_device())
>
> Just for your reference.
>
>> +
>> + if (!npd->cfg_gpio)
>> + npd->cfg_gpio = samsung_keypad_cfg_gpio;
>
> More and more platforms are moving their IO-mux to a static descriptive
> array and have some API to configure. E.g. PXA, Orion/Kirkwood, iMX....
> The idea basically is:
>
> 1) IO-Mux and the internal peripheral controller are conceptually
> separate entities (the extreme case is the internal peripheral
> controller can function normally even without IO-mux being setup
> correct, only that signals not proplery routed in/out)
>
> 2) IO-mux should be configured before the peripheral drivers are
> ready to go (there are cases that IO-mux will have to be changed
> at run-time, so IO-mux API should be able to handle that, though
> such cases are rare)
>
> 3) Peripheral drivers should not be concerned with IO-mux config
>
> Not sure though how the IO-mux is designed in S5P, so treat the above
> as reference only.
>
Hmm... i think it don't care to go to above reference.
Ben, what do you think about this?
>> +
>> + samsung_device_keypad.dev.platform_data = npd;
>> +}
>> diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
>> index e6144e4..6d9f01b 100644
>> --- a/arch/arm/plat-samsung/include/plat/devs.h
>> +++ b/arch/arm/plat-samsung/include/plat/devs.h
>> @@ -100,6 +100,8 @@ extern struct platform_device s5pc100_device_iis0;
>> extern struct platform_device s5pc100_device_iis1;
>> extern struct platform_device s5pc100_device_iis2;
>>
>> +extern struct platform_device samsung_device_keypad;
>> +
>> /* s3c2440 specific devices */
>>
>> #ifdef CONFIG_CPU_S3C2440
>> diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
>> new file mode 100644
>> index 0000000..6d139d6
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/include/plat/keypad.h
>> @@ -0,0 +1,59 @@
>> +/*
>> + * linux/arch/arm/plat-samsung/include/plat/keypad.h
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
>> + * Author: Joonyoung Shim <jy0922.shim@samsung.com>
>> + *
>> + * Samsung Platform - Keypad platform data definitions
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#ifndef __PLAT_SAMSUNG_KEYPAD_H
>> +#define __PLAT_SAMSUNG_KEYPAD_H
>> +
>> +#include <linux/input/matrix_keypad.h>
>> +
>> +#define SAMSUNG_MAX_ROWS 8
>> +#define SAMSUNG_MAX_COLS 8
>> +
>> +/**
>> + * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
>> + * @keymap_data: pointer to &matrix_keymap_data.
>> + * @rows: number of keypad row supported.
>> + * @cols: number of keypad col supported.
>> + * @no_autorepeat: disable key autorepeat.
>> + * @wakeup: controls whether the device should be set up as wakeup source.
>> + * @cfg_gpio: configure the GPIO.
>> + *
>> + * Initialisation data specific to either the machine or the platform
>> + * for the device driver to use or call-back when configuring gpio.
>> + */
>> +struct samsung_keypad_platdata {
>> + const struct matrix_keymap_data *keymap_data;
>> + unsigned int rows;
>> + unsigned int cols;
>> + bool no_autorepeat;
>> + bool wakeup;
>> +
>> + void (*cfg_gpio)(unsigned int rows, unsigned int cols);
>> +};
>
> Why are you calling everything samsung_this, samsung_that? Now think about:
>
> 1. Is this keypad applicable to all Samsung silicon?
> 2. Samsung has a new IP for the keypad, now what? samsung_new_keypad_platdata?
>
> s5p_keypad sounds more reasable to me if it's applicable to all the s5p
> series. Now I also doubt that's the case. So normally, we will name it
> after the first silicon where this IP appears, e.g.
>
> s5pc100_keypad
>
> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
> here.
>
This is naming issue and i know it was not desided yet.
This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210.
>> +
>> +/**
>> + * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
>> + * @pd: Platform data to register to device.
>> + *
>> + * Register the given platform data for use with Samsung Keypad device.
>> + * The call will copy the platform data, so the board definitions can
>> + * make the structure itself __initdata.
>> + */
>> +extern void samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd);
>> +
>> +/* defined by architecture to configure gpio. */
>> +extern void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols);
>> +
>> +#endif /* __PLAT_SAMSUNG_KEYPAD_H */
>> diff --git a/arch/arm/plat-samsung/include/plat/regs-keypad.h b/arch/arm/plat-samsung/include/plat/regs-keypad.h
>> new file mode 100644
>> index 0000000..e4688f0
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/include/plat/regs-keypad.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * linux/arch/arm/plat-samsung/include/plat/regs-keypad.h
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
>> + * Author: Joonyoung Shim <jy0922.shim@samsung.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.
>> + *
>> + */
>> +
>> +#ifndef __SAMSUNG_KEYPAD_H__
>> +#define __SAMSUNG_KEYPAD_H__
>> +
>> +#define SAMSUNG_KEYIFCON 0x00
>> +#define SAMSUNG_KEYIFSTSCLR 0x04
>> +#define SAMSUNG_KEYIFCOL 0x08
>> +#define SAMSUNG_KEYIFROW 0x0c
>> +#define SAMSUNG_KEYIFFC 0x10
>> +
>> +/* SAMSUNG_KEYIFCON */
>> +#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
>> +#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
>> +#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
>> +#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
>> +#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
>> +
>> +/* SAMSUNG_KEYIFSTSCLR */
>> +#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
>> +#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
>> +#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
>> +#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
>> +#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
>> +#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
>> +
>> +/* SAMSUNG_KEYIFCOL */
>> +#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
>> +#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
>> +
>> +/* SAMSUNG_KEYIFROW */
>> +#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
>> +#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
>> +
>> +/* SAMSUNG_KEYIFFC */
>> +#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
>> +
>> +#endif /* __SAMSUNG_KEYPAD_H__ */
>> --
>> 1.7.0.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 11:21 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-21 11:21 UTC (permalink / raw)
To: linux-arm-kernel
On 6/21/2010 6:05 PM, Eric Miao wrote:
> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> This patch adds samsung keypad device definition for samsung cpus.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> arch/arm/plat-samsung/Kconfig | 5 ++
>> arch/arm/plat-samsung/Makefile | 1 +
>> arch/arm/plat-samsung/dev-keypad.c | 58 +++++++++++++++++++++
>
> Why need an individual file for a simple device? In the end, these files
> will flood over the plat-samsung/ directory. And now think this - in your
> new SoC design ,the keypad IP is replaced with a completely new one, does
> that mean a new dev-keypad-new1.c, dev-keypad-new2.c?
>
> I personally prefer a single devices.c for all the devices, if you
> orgnize well, shouldn't take up much code size in that single file.
>
I know, but will need more effort of other device as well as keypad to
do it.
>> arch/arm/plat-samsung/include/plat/devs.h | 2 +
>> arch/arm/plat-samsung/include/plat/keypad.h | 59 ++++++++++++++++++++++
>> arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ++++++++++++++++++
>
> Will these registers be used elsewhere except within the keypad driver?
> If no, they might be better moved to the keypad driver itself, it makes
> the driver more self-contained and avoid the registers being accessed
> anywhere.
>
Right, these registers are used only in the keypad driver. I think it's
possible.
>> 6 files changed, 174 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/plat-samsung/dev-keypad.c
>> create mode 100644 arch/arm/plat-samsung/include/plat/keypad.h
>> create mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
>>
>> diff --git a/arch/arm/plat-samsung/Kconfig b/arch/arm/plat-samsung/Kconfig
>> index 2753fb3..bd007e3 100644
>> --- a/arch/arm/plat-samsung/Kconfig
>> +++ b/arch/arm/plat-samsung/Kconfig
>> @@ -227,6 +227,11 @@ config SAMSUNG_DEV_TS
>> help
>> Common in platform device definitions for touchscreen device
>>
>> +config SAMSUNG_DEV_KEYPAD
>> + bool
>> + help
>> + Compile in platform device definitions for keypad
>> +
>> # DMA
>>
>> config S3C_DMA
>> diff --git a/arch/arm/plat-samsung/Makefile b/arch/arm/plat-samsung/Makefile
>> index b1d82cc..8269d80 100644
>> --- a/arch/arm/plat-samsung/Makefile
>> +++ b/arch/arm/plat-samsung/Makefile
>> @@ -48,6 +48,7 @@ obj-$(CONFIG_S3C_DEV_RTC) += dev-rtc.o
>>
>> obj-$(CONFIG_SAMSUNG_DEV_ADC) += dev-adc.o
>> obj-$(CONFIG_SAMSUNG_DEV_TS) += dev-ts.o
>> +obj-$(CONFIG_SAMSUNG_DEV_KEYPAD) += dev-keypad.o
>>
>> # DMA support
>>
>> diff --git a/arch/arm/plat-samsung/dev-keypad.c b/arch/arm/plat-samsung/dev-keypad.c
>> new file mode 100644
>> index 0000000..679b444
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/dev-keypad.c
>> @@ -0,0 +1,58 @@
>> +/*
>> + * linux/arch/arm/plat-samsung/dev-keypad.c
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
>> + * Author: Joonyoung Shim <jy0922.shim@samsung.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.
>> + *
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <mach/irqs.h>
>> +#include <mach/map.h>
>> +#include <plat/cpu.h>
>> +#include <plat/devs.h>
>> +#include <plat/keypad.h>
>> +
>> +static struct resource samsung_keypad_resources[] = {
>> + [0] = {
>> + .start = SAMSUNG_PA_KEYPAD,
>> + .end = SAMSUNG_PA_KEYPAD + 0x20 - 1,
>> + .flags = IORESOURCE_MEM,
>> + },
>> + [1] = {
>> + .start = IRQ_KEYPAD,
>> + .end = IRQ_KEYPAD,
>> + .flags = IORESOURCE_IRQ,
>> + },
>> +};
>> +
>> +struct platform_device samsung_device_keypad = {
>> + .name = "samsung-keypad",
>> + .id = -1,
>> + .num_resources = ARRAY_SIZE(samsung_keypad_resources),
>> + .resource = samsung_keypad_resources,
>> +};
>> +
>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>> +{
>> + struct samsung_keypad_platdata *npd;
>> +
>> + if (!pd) {
>> + printk(KERN_ERR "%s: no platform data\n", __func__);
>> + return;
>> + }
>> +
>> + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>> + if (!npd)
>> + printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>
> This part of the code is actually duplicated again and again and again
> for each device, PXA and other legacy platforms are bad references for
> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
> major points:
>
I know ben posted patches to remove duplicated codes such this.
http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg01474.html
> 1. A minimum 'struct pxa_device_desc' for a simple description of a
> device (more than 90% of the devices can be described that way),
> and avoid using a comparatively heavier weight platform_device,
> which can be generated at run-time
>
> 2. pxa_register_device() to allocate and register the platform_device
> at run-time, along with the platform data
>
> 3. pxa168_add_*() as a consistent/clearly named API, and with type
> checking for the platform_data type (it's a bit difficult to check
> type in pxa_register_device())
>
> Just for your reference.
>
>> +
>> + if (!npd->cfg_gpio)
>> + npd->cfg_gpio = samsung_keypad_cfg_gpio;
>
> More and more platforms are moving their IO-mux to a static descriptive
> array and have some API to configure. E.g. PXA, Orion/Kirkwood, iMX....
> The idea basically is:
>
> 1) IO-Mux and the internal peripheral controller are conceptually
> separate entities (the extreme case is the internal peripheral
> controller can function normally even without IO-mux being setup
> correct, only that signals not proplery routed in/out)
>
> 2) IO-mux should be configured before the peripheral drivers are
> ready to go (there are cases that IO-mux will have to be changed
> at run-time, so IO-mux API should be able to handle that, though
> such cases are rare)
>
> 3) Peripheral drivers should not be concerned with IO-mux config
>
> Not sure though how the IO-mux is designed in S5P, so treat the above
> as reference only.
>
Hmm... i think it don't care to go to above reference.
Ben, what do you think about this?
>> +
>> + samsung_device_keypad.dev.platform_data = npd;
>> +}
>> diff --git a/arch/arm/plat-samsung/include/plat/devs.h b/arch/arm/plat-samsung/include/plat/devs.h
>> index e6144e4..6d9f01b 100644
>> --- a/arch/arm/plat-samsung/include/plat/devs.h
>> +++ b/arch/arm/plat-samsung/include/plat/devs.h
>> @@ -100,6 +100,8 @@ extern struct platform_device s5pc100_device_iis0;
>> extern struct platform_device s5pc100_device_iis1;
>> extern struct platform_device s5pc100_device_iis2;
>>
>> +extern struct platform_device samsung_device_keypad;
>> +
>> /* s3c2440 specific devices */
>>
>> #ifdef CONFIG_CPU_S3C2440
>> diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
>> new file mode 100644
>> index 0000000..6d139d6
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/include/plat/keypad.h
>> @@ -0,0 +1,59 @@
>> +/*
>> + * linux/arch/arm/plat-samsung/include/plat/keypad.h
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
>> + * Author: Joonyoung Shim <jy0922.shim@samsung.com>
>> + *
>> + * Samsung Platform - Keypad platform data definitions
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#ifndef __PLAT_SAMSUNG_KEYPAD_H
>> +#define __PLAT_SAMSUNG_KEYPAD_H
>> +
>> +#include <linux/input/matrix_keypad.h>
>> +
>> +#define SAMSUNG_MAX_ROWS 8
>> +#define SAMSUNG_MAX_COLS 8
>> +
>> +/**
>> + * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
>> + * @keymap_data: pointer to &matrix_keymap_data.
>> + * @rows: number of keypad row supported.
>> + * @cols: number of keypad col supported.
>> + * @no_autorepeat: disable key autorepeat.
>> + * @wakeup: controls whether the device should be set up as wakeup source.
>> + * @cfg_gpio: configure the GPIO.
>> + *
>> + * Initialisation data specific to either the machine or the platform
>> + * for the device driver to use or call-back when configuring gpio.
>> + */
>> +struct samsung_keypad_platdata {
>> + const struct matrix_keymap_data *keymap_data;
>> + unsigned int rows;
>> + unsigned int cols;
>> + bool no_autorepeat;
>> + bool wakeup;
>> +
>> + void (*cfg_gpio)(unsigned int rows, unsigned int cols);
>> +};
>
> Why are you calling everything samsung_this, samsung_that? Now think about:
>
> 1. Is this keypad applicable to all Samsung silicon?
> 2. Samsung has a new IP for the keypad, now what? samsung_new_keypad_platdata?
>
> s5p_keypad sounds more reasable to me if it's applicable to all the s5p
> series. Now I also doubt that's the case. So normally, we will name it
> after the first silicon where this IP appears, e.g.
>
> s5pc100_keypad
>
> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
> here.
>
This is naming issue and i know it was not desided yet.
This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210.
>> +
>> +/**
>> + * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
>> + * @pd: Platform data to register to device.
>> + *
>> + * Register the given platform data for use with Samsung Keypad device.
>> + * The call will copy the platform data, so the board definitions can
>> + * make the structure itself __initdata.
>> + */
>> +extern void samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd);
>> +
>> +/* defined by architecture to configure gpio. */
>> +extern void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols);
>> +
>> +#endif /* __PLAT_SAMSUNG_KEYPAD_H */
>> diff --git a/arch/arm/plat-samsung/include/plat/regs-keypad.h b/arch/arm/plat-samsung/include/plat/regs-keypad.h
>> new file mode 100644
>> index 0000000..e4688f0
>> --- /dev/null
>> +++ b/arch/arm/plat-samsung/include/plat/regs-keypad.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * linux/arch/arm/plat-samsung/include/plat/regs-keypad.h
>> + *
>> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
>> + * Author: Joonyoung Shim <jy0922.shim@samsung.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.
>> + *
>> + */
>> +
>> +#ifndef __SAMSUNG_KEYPAD_H__
>> +#define __SAMSUNG_KEYPAD_H__
>> +
>> +#define SAMSUNG_KEYIFCON 0x00
>> +#define SAMSUNG_KEYIFSTSCLR 0x04
>> +#define SAMSUNG_KEYIFCOL 0x08
>> +#define SAMSUNG_KEYIFROW 0x0c
>> +#define SAMSUNG_KEYIFFC 0x10
>> +
>> +/* SAMSUNG_KEYIFCON */
>> +#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
>> +#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
>> +#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
>> +#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
>> +#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
>> +
>> +/* SAMSUNG_KEYIFSTSCLR */
>> +#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
>> +#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
>> +#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
>> +#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
>> +#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
>> +#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
>> +
>> +/* SAMSUNG_KEYIFCOL */
>> +#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
>> +#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
>> +
>> +/* SAMSUNG_KEYIFROW */
>> +#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
>> +#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
>> +
>> +/* SAMSUNG_KEYIFFC */
>> +#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
>> +
>> +#endif /* __SAMSUNG_KEYPAD_H__ */
>> --
>> 1.7.0.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 11:16 ` Russell King - ARM Linux
@ 2010-06-21 14:15 ` Eric Miao
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 14:15 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Joonyoung Shim, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On Mon, Jun 21, 2010 at 7:16 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> >> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> >> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>> >> > +{
>> >> > + struct samsung_keypad_platdata *npd;
>> >> > +
>> >> > + if (!pd) {
>> >> > + printk(KERN_ERR "%s: no platform data\n", __func__);
>> >> > + return;
>> >> > + }
>> >> > +
>> >> > + npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>> >> > + if (!npd)
>> >> > + printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>> >>
>> >> This part of the code is actually duplicated again and again and again
>> >> for each device, PXA and other legacy platforms are bad references for
>> >> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> >> major points:
>> >>
>> >> 1. A minimum 'struct pxa_device_desc' for a simple description of a
>> >> device (more than 90% of the devices can be described that way),
>> >> and avoid using a comparatively heavier weight platform_device,
>> >> which can be generated at run-time
>> >>
>> >> 2. pxa_register_device() to allocate and register the platform_device
>> >> at run-time, along with the platform data
>> >
>> > It's a bad idea to make platform data be run-time discardable like this:
>> >
>> >> > +struct samsung_keypad_platdata {
>> >> > + const struct matrix_keymap_data *keymap_data;
>> >
>> > What you end up with is some platform data structures which must be kept
>> > (those which have pointers to them from the platform data), and others
>> > (the platform data itself) which can be discarded at runtime.
>> >
>> > We know that the __initdata attributations cause lots of problems -
>> > they're frequently wrong. Just see the constant hastle with __devinit
>> > et.al. The same issue happens with __initdata as well.
>> >
>> > So why make things more complicated by allowing some platform data
>> > structures to be discardable and others not to be? Is their small
>> > size (maybe 6 words for this one) really worth the hastle of getting
>> > __initdata attributations wrong (eg, on the keymap data?)
>> >
>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
>
> Yes, but only some of the data can be discarded. Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
> So, samsung_keypad_platdata can be marked with __initdata, but the keymap
> data must not be - and this is where the confusion about what can be
> marked with __initdata starts - and eventually leads to the keymap data
> also being mistakenly marked __initdata.
Indeed. One ideal solution would be to make all the board code into
a module, yet since some of the code should be executed really
early and need to stay as built-in, looks like we need a way to
separate these two parts.
>
> Is saving six words (or so) worth the effort to track down stuff
> inappropriately marked with __initdata ? I'm sure there's bigger savings
> to be had in other parts of the kernel (such as shrinking the size of
> tcp hash tables, reducing the kernel message ring buffer, etc) if size is
> really a concern.
>
That's true, however, I think the size will become a bit more significant
if more and more board code is compiled into a single kernel.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 14:15 ` Eric Miao
0 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 14:15 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 21, 2010 at 7:16 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>> >> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> >> > +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>> >> > +{
>> >> > + ? ? ? struct samsung_keypad_platdata *npd;
>> >> > +
>> >> > + ? ? ? if (!pd) {
>> >> > + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>> >> > + ? ? ? ? ? ? ? return;
>> >> > + ? ? ? }
>> >> > +
>> >> > + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>> >> > + ? ? ? if (!npd)
>> >> > + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>> >>
>> >> This part of the code is actually duplicated again and again and again
>> >> for each device, PXA and other legacy platforms are bad references for
>> >> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>> >> major points:
>> >>
>> >> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>> >> ? ? device (more than 90% of the devices can be described that way),
>> >> ? ? and avoid using a comparatively heavier weight platform_device,
>> >> ? ? which can be generated at run-time
>> >>
>> >> ?2. pxa_register_device() to allocate and register the platform_device
>> >> ? ? at run-time, along with the platform data
>> >
>> > It's a bad idea to make platform data be run-time discardable like this:
>> >
>> >> > +struct samsung_keypad_platdata {
>> >> > + ? ? ? const struct matrix_keymap_data *keymap_data;
>> >
>> > What you end up with is some platform data structures which must be kept
>> > (those which have pointers to them from the platform data), and others
>> > (the platform data itself) which can be discarded at runtime.
>> >
>> > We know that the __initdata attributations cause lots of problems -
>> > they're frequently wrong. ?Just see the constant hastle with __devinit
>> > et.al. ?The same issue happens with __initdata as well.
>> >
>> > So why make things more complicated by allowing some platform data
>> > structures to be discardable and others not to be? ?Is their small
>> > size (maybe 6 words for this one) really worth the hastle of getting
>> > __initdata attributations wrong (eg, on the keymap data?)
>> >
>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
>
> Yes, but only some of the data can be discarded. ?Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
> So, samsung_keypad_platdata can be marked with __initdata, but the keymap
> data must not be - and this is where the confusion about what can be
> marked with __initdata starts - and eventually leads to the keymap data
> also being mistakenly marked __initdata.
Indeed. One ideal solution would be to make all the board code into
a module, yet since some of the code should be executed really
early and need to stay as built-in, looks like we need a way to
separate these two parts.
>
> Is saving six words (or so) worth the effort to track down stuff
> inappropriately marked with __initdata ? ?I'm sure there's bigger savings
> to be had in other parts of the kernel (such as shrinking the size of
> tcp hash tables, reducing the kernel message ring buffer, etc) if size is
> really a concern.
>
That's true, however, I think the size will become a bit more significant
if more and more board code is compiled into a single kernel.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 11:21 ` Joonyoung Shim
@ 2010-06-21 14:19 ` Eric Miao
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 14:19 UTC (permalink / raw)
To: Joonyoung Shim
Cc: linux-samsung-soc, dmitry.torokhov, kyungmin.park, kgene.kim,
ben-linux, linux-input, linux-arm-kernel
>> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
>> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
>> here.
>>
>
> This is naming issue and i know it was not desided yet.
> This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210.
>
I bet the first silicon this IP is deployed is s3c64xx, so my understanding
's3c64xx-keypad.c' should be an acceptable name. And it's also reasonable,
a s3c64xx-keypad driver, and is later expanded to support subsequent silicons
without the file name being changed.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-21 14:19 ` Eric Miao
0 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-21 14:19 UTC (permalink / raw)
To: linux-arm-kernel
>> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
>> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
>> here.
>>
>
> This is naming issue and i know it was not desided yet.
> This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210.
>
I bet the first silicon this IP is deployed is s3c64xx, so my understanding
's3c64xx-keypad.c' should be an acceptable name. And it's also reasonable,
a s3c64xx-keypad driver, and is later expanded to support subsequent silicons
without the file name being changed.
^ permalink raw reply [flat|nested] 50+ messages in thread
* RE: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 14:19 ` Eric Miao
@ 2010-06-22 0:18 ` Kukjin Kim
-1 siblings, 0 replies; 50+ messages in thread
From: Kukjin Kim @ 2010-06-22 0:18 UTC (permalink / raw)
To: 'Eric Miao', 'Joonyoung Shim'
Cc: linux-samsung-soc, dmitry.torokhov, kyungmin.park, ben-linux,
linux-input, linux-arm-kernel
Eric Miao wrote:
>
> >> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
> >> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
> >> here.
> >>
> >
> > This is naming issue and i know it was not desided yet.
> > This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210.
> >
>
> I bet the first silicon this IP is deployed is s3c64xx, so my understanding
> 's3c64xx-keypad.c' should be an acceptable name. And it's also reasonable,
> a s3c64xx-keypad driver, and is later expanded to support subsequent silicons
> without the file name being changed.
Hi,
Seems to be more suitable 'samsung-keypad', because can support S5PC100, S5PC110, and S5PV210 as well as S3C64XX.
Already tested on SMDK6410(S3C6410), SMDKC100(S5PC100), Aquila(S5PC110), GONI(S5PC110) and SMDKV210(S5PV210) boards.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-22 0:18 ` Kukjin Kim
0 siblings, 0 replies; 50+ messages in thread
From: Kukjin Kim @ 2010-06-22 0:18 UTC (permalink / raw)
To: linux-arm-kernel
Eric Miao wrote:
>
> >> and now in s5pc200 code, you can also register/re-use the s5pc100_keypad
> >> device/driver, and everyone knows - OK, the IP on S5PC100 is now re-used
> >> here.
> >>
> >
> > This is naming issue and i know it was not desided yet.
> > This keypad driver can support s3c64xx, s5pc100, s5pc110, s5pv210.
> >
>
> I bet the first silicon this IP is deployed is s3c64xx, so my understanding
> 's3c64xx-keypad.c' should be an acceptable name. And it's also reasonable,
> a s3c64xx-keypad driver, and is later expanded to support subsequent silicons
> without the file name being changed.
Hi,
Seems to be more suitable 'samsung-keypad', because can support S5PC100, S5PC110, and S5PV210 as well as S3C64XX.
Already tested on SMDK6410(S3C6410), SMDKC100(S5PC100), Aquila(S5PC110), GONI(S5PC110) and SMDKV210(S5PV210) boards.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-21 11:16 ` Russell King - ARM Linux
@ 2010-06-22 0:48 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-22 0:48 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Eric Miao, linux-samsung-soc, dmitry.torokhov, kyungmin.park,
kgene.kim, ben-linux, linux-input, linux-arm-kernel
On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>> +{
>>>>> + � � � struct samsung_keypad_platdata *npd;
>>>>> +
>>>>> + � � � if (!pd) {
>>>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>> + � � � � � � � return;
>>>>> + � � � }
>>>>> +
>>>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>> + � � � if (!npd)
>>>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>> This part of the code is actually duplicated again and again and again
>>>> for each device, PXA and other legacy platforms are bad references for
>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>> major points:
>>>>
>>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>> � � device (more than 90% of the devices can be described that way),
>>>> � � and avoid using a comparatively heavier weight platform_device,
>>>> � � which can be generated at run-time
>>>>
>>>> �2. pxa_register_device() to allocate and register the platform_device
>>>> � � at run-time, along with the platform data
>>> It's a bad idea to make platform data be run-time discardable like this:
>>>
>>>>> +struct samsung_keypad_platdata {
>>>>> + � � � const struct matrix_keymap_data *keymap_data;
>>> What you end up with is some platform data structures which must be kept
>>> (those which have pointers to them from the platform data), and others
>>> (the platform data itself) which can be discarded at runtime.
>>>
>>> We know that the __initdata attributations cause lots of problems -
>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>> et.al. �The same issue happens with __initdata as well.
>>>
>>> So why make things more complicated by allowing some platform data
>>> structures to be discardable and others not to be? �Is their small
>>> size (maybe 6 words for this one) really worth the hastle of getting
>>> __initdata attributations wrong (eg, on the keymap data?)
>>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
>
> Yes, but only some of the data can be discarded. Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
No. the keymap_data is possible too. The keypad driver allocates other
keymap area of input device and it is assigned from datas based on this
keymap_data.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-22 0:48 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-22 0:48 UTC (permalink / raw)
To: linux-arm-kernel
On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>> +{
>>>>> + ? ? ? struct samsung_keypad_platdata *npd;
>>>>> +
>>>>> + ? ? ? if (!pd) {
>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>> + ? ? ? ? ? ? ? return;
>>>>> + ? ? ? }
>>>>> +
>>>>> + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>> + ? ? ? if (!npd)
>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>> This part of the code is actually duplicated again and again and again
>>>> for each device, PXA and other legacy platforms are bad references for
>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>> major points:
>>>>
>>>> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>> ? ? device (more than 90% of the devices can be described that way),
>>>> ? ? and avoid using a comparatively heavier weight platform_device,
>>>> ? ? which can be generated at run-time
>>>>
>>>> ?2. pxa_register_device() to allocate and register the platform_device
>>>> ? ? at run-time, along with the platform data
>>> It's a bad idea to make platform data be run-time discardable like this:
>>>
>>>>> +struct samsung_keypad_platdata {
>>>>> + ? ? ? const struct matrix_keymap_data *keymap_data;
>>> What you end up with is some platform data structures which must be kept
>>> (those which have pointers to them from the platform data), and others
>>> (the platform data itself) which can be discarded at runtime.
>>>
>>> We know that the __initdata attributations cause lots of problems -
>>> they're frequently wrong. ?Just see the constant hastle with __devinit
>>> et.al. ?The same issue happens with __initdata as well.
>>>
>>> So why make things more complicated by allowing some platform data
>>> structures to be discardable and others not to be? ?Is their small
>>> size (maybe 6 words for this one) really worth the hastle of getting
>>> __initdata attributations wrong (eg, on the keymap data?)
>>>
>> Russell,
>>
>> The benefit I see is when multiple boards are compiled in, those
>> data not used can be automatically discarded.
>
> Yes, but only some of the data can be discarded. Continuing with the
> example in hand, while you can discard the six words which represent
> samsung_keypad_platdata, but the keymap_data can't be because that won't
> be re-allocated, which is probably a much larger data structure.
>
No. the keymap_data is possible too. The keypad driver allocates other
keymap area of input device and it is assigned from datas based on this
keymap_data.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-22 0:48 ` Joonyoung Shim
@ 2010-06-22 3:02 ` Eric Miao
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-22 3:02 UTC (permalink / raw)
To: Joonyoung Shim
Cc: Russell King - ARM Linux, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk> wrote:
>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>> +{
>>>>>> + � � � struct samsung_keypad_platdata *npd;
>>>>>> +
>>>>>> + � � � if (!pd) {
>>>>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>> + � � � � � � � return;
>>>>>> + � � � }
>>>>>> +
>>>>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>> + � � � if (!npd)
>>>>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>> This part of the code is actually duplicated again and again and again
>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>> major points:
>>>>>
>>>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>> � � device (more than 90% of the devices can be described that way),
>>>>> � � and avoid using a comparatively heavier weight platform_device,
>>>>> � � which can be generated at run-time
>>>>>
>>>>> �2. pxa_register_device() to allocate and register the platform_device
>>>>> � � at run-time, along with the platform data
>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>
>>>>>> +struct samsung_keypad_platdata {
>>>>>> + � � � const struct matrix_keymap_data *keymap_data;
>>>> What you end up with is some platform data structures which must be kept
>>>> (those which have pointers to them from the platform data), and others
>>>> (the platform data itself) which can be discarded at runtime.
>>>>
>>>> We know that the __initdata attributations cause lots of problems -
>>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>>> et.al. �The same issue happens with __initdata as well.
>>>>
>>>> So why make things more complicated by allowing some platform data
>>>> structures to be discardable and others not to be? �Is their small
>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>
>>> Russell,
>>>
>>> The benefit I see is when multiple boards are compiled in, those
>>> data not used can be automatically discarded.
>>
>> Yes, but only some of the data can be discarded. Continuing with the
>> example in hand, while you can discard the six words which represent
>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>> be re-allocated, which is probably a much larger data structure.
>>
>
> No. the keymap_data is possible too. The keypad driver allocates other
> keymap area of input device and it is assigned from datas based on this
> keymap_data.
>
This is a generic issue. Even if in your example, you can avoid this by
re-allocation and re-assignment (ignore the performance issue for such
behavior), the real question is the difficult to track all these down. Since
matrix_keypad_data is something out of your control (it was actually
drafted by me and Dmitry if you are interested), and think about one day
I changed it's definition, now you have to sync your driver and code every
time to make sure the discarded data is not referenced.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-22 3:02 ` Eric Miao
0 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-22 3:02 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk> wrote:
>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>> +{
>>>>>> + ? ? ? struct samsung_keypad_platdata *npd;
>>>>>> +
>>>>>> + ? ? ? if (!pd) {
>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>> + ? ? ? ? ? ? ? return;
>>>>>> + ? ? ? }
>>>>>> +
>>>>>> + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>> + ? ? ? if (!npd)
>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>> This part of the code is actually duplicated again and again and again
>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>> major points:
>>>>>
>>>>> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>> ? ? device (more than 90% of the devices can be described that way),
>>>>> ? ? and avoid using a comparatively heavier weight platform_device,
>>>>> ? ? which can be generated at run-time
>>>>>
>>>>> ?2. pxa_register_device() to allocate and register the platform_device
>>>>> ? ? at run-time, along with the platform data
>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>
>>>>>> +struct samsung_keypad_platdata {
>>>>>> + ? ? ? const struct matrix_keymap_data *keymap_data;
>>>> What you end up with is some platform data structures which must be kept
>>>> (those which have pointers to them from the platform data), and others
>>>> (the platform data itself) which can be discarded at runtime.
>>>>
>>>> We know that the __initdata attributations cause lots of problems -
>>>> they're frequently wrong. ?Just see the constant hastle with __devinit
>>>> et.al. ?The same issue happens with __initdata as well.
>>>>
>>>> So why make things more complicated by allowing some platform data
>>>> structures to be discardable and others not to be? ?Is their small
>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>
>>> Russell,
>>>
>>> The benefit I see is when multiple boards are compiled in, those
>>> data not used can be automatically discarded.
>>
>> Yes, but only some of the data can be discarded. ?Continuing with the
>> example in hand, while you can discard the six words which represent
>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>> be re-allocated, which is probably a much larger data structure.
>>
>
> No. the keymap_data is possible too. The keypad driver allocates other
> keymap area of input device and it is assigned from datas based on this
> keymap_data.
>
This is a generic issue. Even if in your example, you can avoid this by
re-allocation and re-assignment (ignore the performance issue for such
behavior), the real question is the difficult to track all these down. Since
matrix_keypad_data is something out of your control (it was actually
drafted by me and Dmitry if you are interested), and think about one day
I changed it's definition, now you have to sync your driver and code every
time to make sure the discarded data is not referenced.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-22 3:02 ` Eric Miao
@ 2010-06-22 3:27 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-22 3:27 UTC (permalink / raw)
To: Eric Miao
Cc: Russell King - ARM Linux, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On 6/22/2010 12:02 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> wrote:
>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>> +{
>>>>>>> + � � � struct samsung_keypad_platdata *npd;
>>>>>>> +
>>>>>>> + � � � if (!pd) {
>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>> + � � � � � � � return;
>>>>>>> + � � � }
>>>>>>> +
>>>>>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>> + � � � if (!npd)
>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>> This part of the code is actually duplicated again and again and again
>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>> major points:
>>>>>>
>>>>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>> � � device (more than 90% of the devices can be described that way),
>>>>>> � � and avoid using a comparatively heavier weight platform_device,
>>>>>> � � which can be generated at run-time
>>>>>>
>>>>>> �2. pxa_register_device() to allocate and register the platform_device
>>>>>> � � at run-time, along with the platform data
>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>
>>>>>>> +struct samsung_keypad_platdata {
>>>>>>> + � � � const struct matrix_keymap_data *keymap_data;
>>>>> What you end up with is some platform data structures which must be kept
>>>>> (those which have pointers to them from the platform data), and others
>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>
>>>>> We know that the __initdata attributations cause lots of problems -
>>>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>>>> et.al. �The same issue happens with __initdata as well.
>>>>>
>>>>> So why make things more complicated by allowing some platform data
>>>>> structures to be discardable and others not to be? �Is their small
>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>
>>>> Russell,
>>>>
>>>> The benefit I see is when multiple boards are compiled in, those
>>>> data not used can be automatically discarded.
>>> Yes, but only some of the data can be discarded. Continuing with the
>>> example in hand, while you can discard the six words which represent
>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>> be re-allocated, which is probably a much larger data structure.
>>>
>> No. the keymap_data is possible too. The keypad driver allocates other
>> keymap area of input device and it is assigned from datas based on this
>> keymap_data.
>>
>
> This is a generic issue. Even if in your example, you can avoid this by
> re-allocation and re-assignment (ignore the performance issue for such
> behavior), the real question is the difficult to track all these down. Since
Right, it can occur difficulty of maintain. I wanted just to inform the
current fact.
> matrix_keypad_data is something out of your control (it was actually
> drafted by me and Dmitry if you are interested), and think about one day
> I changed it's definition, now you have to sync your driver and code every
> time to make sure the discarded data is not referenced.
>
if matrix_keypad_data is changed, i think the patchset should included
change of related other parts using it.
--
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-22 3:27 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-22 3:27 UTC (permalink / raw)
To: linux-arm-kernel
On 6/22/2010 12:02 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> wrote:
>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>> +{
>>>>>>> + ? ? ? struct samsung_keypad_platdata *npd;
>>>>>>> +
>>>>>>> + ? ? ? if (!pd) {
>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>> + ? ? ? ? ? ? ? return;
>>>>>>> + ? ? ? }
>>>>>>> +
>>>>>>> + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>> + ? ? ? if (!npd)
>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>> This part of the code is actually duplicated again and again and again
>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>> major points:
>>>>>>
>>>>>> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>> ? ? device (more than 90% of the devices can be described that way),
>>>>>> ? ? and avoid using a comparatively heavier weight platform_device,
>>>>>> ? ? which can be generated at run-time
>>>>>>
>>>>>> ?2. pxa_register_device() to allocate and register the platform_device
>>>>>> ? ? at run-time, along with the platform data
>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>
>>>>>>> +struct samsung_keypad_platdata {
>>>>>>> + ? ? ? const struct matrix_keymap_data *keymap_data;
>>>>> What you end up with is some platform data structures which must be kept
>>>>> (those which have pointers to them from the platform data), and others
>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>
>>>>> We know that the __initdata attributations cause lots of problems -
>>>>> they're frequently wrong. ?Just see the constant hastle with __devinit
>>>>> et.al. ?The same issue happens with __initdata as well.
>>>>>
>>>>> So why make things more complicated by allowing some platform data
>>>>> structures to be discardable and others not to be? ?Is their small
>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>
>>>> Russell,
>>>>
>>>> The benefit I see is when multiple boards are compiled in, those
>>>> data not used can be automatically discarded.
>>> Yes, but only some of the data can be discarded. Continuing with the
>>> example in hand, while you can discard the six words which represent
>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>> be re-allocated, which is probably a much larger data structure.
>>>
>> No. the keymap_data is possible too. The keypad driver allocates other
>> keymap area of input device and it is assigned from datas based on this
>> keymap_data.
>>
>
> This is a generic issue. Even if in your example, you can avoid this by
> re-allocation and re-assignment (ignore the performance issue for such
> behavior), the real question is the difficult to track all these down. Since
Right, it can occur difficulty of maintain. I wanted just to inform the
current fact.
> matrix_keypad_data is something out of your control (it was actually
> drafted by me and Dmitry if you are interested), and think about one day
> I changed it's definition, now you have to sync your driver and code every
> time to make sure the discarded data is not referenced.
>
if matrix_keypad_data is changed, i think the patchset should included
change of related other parts using it.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-22 3:27 ` Joonyoung Shim
@ 2010-06-22 3:38 ` Eric Miao
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-22 3:38 UTC (permalink / raw)
To: Joonyoung Shim
Cc: Russell King - ARM Linux, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
<jy0922.shim@samsung.com> wrote:
> On 6/22/2010 12:02 PM, Eric Miao wrote:
>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>>> +{
>>>>>>>> + � � � struct samsung_keypad_platdata *npd;
>>>>>>>> +
>>>>>>>> + � � � if (!pd) {
>>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>>> + � � � � � � � return;
>>>>>>>> + � � � }
>>>>>>>> +
>>>>>>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>>> + � � � if (!npd)
>>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>>> This part of the code is actually duplicated again and again and again
>>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>>> major points:
>>>>>>>
>>>>>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>>> � � device (more than 90% of the devices can be described that way),
>>>>>>> � � and avoid using a comparatively heavier weight platform_device,
>>>>>>> � � which can be generated at run-time
>>>>>>>
>>>>>>> �2. pxa_register_device() to allocate and register the platform_device
>>>>>>> � � at run-time, along with the platform data
>>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>>
>>>>>>>> +struct samsung_keypad_platdata {
>>>>>>>> + � � � const struct matrix_keymap_data *keymap_data;
>>>>>> What you end up with is some platform data structures which must be kept
>>>>>> (those which have pointers to them from the platform data), and others
>>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>>
>>>>>> We know that the __initdata attributations cause lots of problems -
>>>>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>>>>> et.al. �The same issue happens with __initdata as well.
>>>>>>
>>>>>> So why make things more complicated by allowing some platform data
>>>>>> structures to be discardable and others not to be? �Is their small
>>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>>
>>>>> Russell,
>>>>>
>>>>> The benefit I see is when multiple boards are compiled in, those
>>>>> data not used can be automatically discarded.
>>>> Yes, but only some of the data can be discarded. Continuing with the
>>>> example in hand, while you can discard the six words which represent
>>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>>> be re-allocated, which is probably a much larger data structure.
>>>>
>>> No. the keymap_data is possible too. The keypad driver allocates other
>>> keymap area of input device and it is assigned from datas based on this
>>> keymap_data.
>>>
>>
>> This is a generic issue. Even if in your example, you can avoid this by
>> re-allocation and re-assignment (ignore the performance issue for such
>> behavior), the real question is the difficult to track all these down. Since
>
> Right, it can occur difficulty of maintain. I wanted just to inform the
> current fact.
>
>> matrix_keypad_data is something out of your control (it was actually
>> drafted by me and Dmitry if you are interested), and think about one day
>> I changed it's definition, now you have to sync your driver and code every
>> time to make sure the discarded data is not referenced.
>>
>
> if matrix_keypad_data is changed, i think the patchset should included
> change of related other parts using it.
>
That's reasonable but difficult in practice, every keypad driver using
matrix_keypad_data could be doing things differently. That's what I'm
concerned about. Things will be much easier for driver writers if he
knows the data passed in will always be reference-able.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-22 3:38 ` Eric Miao
0 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-22 3:38 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
<jy0922.shim@samsung.com> wrote:
> On 6/22/2010 12:02 PM, Eric Miao wrote:
>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>>> +{
>>>>>>>> + ? ? ? struct samsung_keypad_platdata *npd;
>>>>>>>> +
>>>>>>>> + ? ? ? if (!pd) {
>>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>>> + ? ? ? ? ? ? ? return;
>>>>>>>> + ? ? ? }
>>>>>>>> +
>>>>>>>> + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>>> + ? ? ? if (!npd)
>>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>>> This part of the code is actually duplicated again and again and again
>>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>>> major points:
>>>>>>>
>>>>>>> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>>> ? ? device (more than 90% of the devices can be described that way),
>>>>>>> ? ? and avoid using a comparatively heavier weight platform_device,
>>>>>>> ? ? which can be generated at run-time
>>>>>>>
>>>>>>> ?2. pxa_register_device() to allocate and register the platform_device
>>>>>>> ? ? at run-time, along with the platform data
>>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>>
>>>>>>>> +struct samsung_keypad_platdata {
>>>>>>>> + ? ? ? const struct matrix_keymap_data *keymap_data;
>>>>>> What you end up with is some platform data structures which must be kept
>>>>>> (those which have pointers to them from the platform data), and others
>>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>>
>>>>>> We know that the __initdata attributations cause lots of problems -
>>>>>> they're frequently wrong. ?Just see the constant hastle with __devinit
>>>>>> et.al. ?The same issue happens with __initdata as well.
>>>>>>
>>>>>> So why make things more complicated by allowing some platform data
>>>>>> structures to be discardable and others not to be? ?Is their small
>>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>>
>>>>> Russell,
>>>>>
>>>>> The benefit I see is when multiple boards are compiled in, those
>>>>> data not used can be automatically discarded.
>>>> Yes, but only some of the data can be discarded. ?Continuing with the
>>>> example in hand, while you can discard the six words which represent
>>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>>> be re-allocated, which is probably a much larger data structure.
>>>>
>>> No. the keymap_data is possible too. The keypad driver allocates other
>>> keymap area of input device and it is assigned from datas based on this
>>> keymap_data.
>>>
>>
>> This is a generic issue. Even if in your example, you can avoid this by
>> re-allocation and re-assignment (ignore the performance issue for such
>> behavior), the real question is the difficult to track all these down. Since
>
> Right, it can occur difficulty of maintain. I wanted just to inform the
> current fact.
>
>> matrix_keypad_data is something out of your control (it was actually
>> drafted by me and Dmitry if you are interested), and think about one day
>> I changed it's definition, now you have to sync your driver and code every
>> time to make sure the discarded data is not referenced.
>>
>
> if matrix_keypad_data is changed, i think the patchset should included
> change of related other parts using it.
>
That's reasonable but difficult in practice, every keypad driver using
matrix_keypad_data could be doing things differently. That's what I'm
concerned about. Things will be much easier for driver writers if he
knows the data passed in will always be reference-able.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-22 3:38 ` Eric Miao
@ 2010-06-22 4:00 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-22 4:00 UTC (permalink / raw)
To: Eric Miao
Cc: Russell King - ARM Linux, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On 6/22/2010 12:38 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
> <jy0922.shim@samsung.com> wrote:
>> On 6/22/2010 12:02 PM, Eric Miao wrote:
>>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>>>> +{
>>>>>>>>> + � � � struct samsung_keypad_platdata *npd;
>>>>>>>>> +
>>>>>>>>> + � � � if (!pd) {
>>>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>>>> + � � � � � � � return;
>>>>>>>>> + � � � }
>>>>>>>>> +
>>>>>>>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>>>> + � � � if (!npd)
>>>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>>>> This part of the code is actually duplicated again and again and again
>>>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>>>> major points:
>>>>>>>>
>>>>>>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>>>> � � device (more than 90% of the devices can be described that way),
>>>>>>>> � � and avoid using a comparatively heavier weight platform_device,
>>>>>>>> � � which can be generated at run-time
>>>>>>>>
>>>>>>>> �2. pxa_register_device() to allocate and register the platform_device
>>>>>>>> � � at run-time, along with the platform data
>>>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>>>
>>>>>>>>> +struct samsung_keypad_platdata {
>>>>>>>>> + � � � const struct matrix_keymap_data *keymap_data;
>>>>>>> What you end up with is some platform data structures which must be kept
>>>>>>> (those which have pointers to them from the platform data), and others
>>>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>>>
>>>>>>> We know that the __initdata attributations cause lots of problems -
>>>>>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>>>>>> et.al. �The same issue happens with __initdata as well.
>>>>>>>
>>>>>>> So why make things more complicated by allowing some platform data
>>>>>>> structures to be discardable and others not to be? �Is their small
>>>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>>>
>>>>>> Russell,
>>>>>>
>>>>>> The benefit I see is when multiple boards are compiled in, those
>>>>>> data not used can be automatically discarded.
>>>>> Yes, but only some of the data can be discarded. Continuing with the
>>>>> example in hand, while you can discard the six words which represent
>>>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>>>> be re-allocated, which is probably a much larger data structure.
>>>>>
>>>> No. the keymap_data is possible too. The keypad driver allocates other
>>>> keymap area of input device and it is assigned from datas based on this
>>>> keymap_data.
>>>>
>>> This is a generic issue. Even if in your example, you can avoid this by
>>> re-allocation and re-assignment (ignore the performance issue for such
>>> behavior), the real question is the difficult to track all these down. Since
>> Right, it can occur difficulty of maintain. I wanted just to inform the
>> current fact.
>>
>>> matrix_keypad_data is something out of your control (it was actually
>>> drafted by me and Dmitry if you are interested), and think about one day
>>> I changed it's definition, now you have to sync your driver and code every
>>> time to make sure the discarded data is not referenced.
>>>
>> if matrix_keypad_data is changed, i think the patchset should included
>> change of related other parts using it.
>>
>
> That's reasonable but difficult in practice, every keypad driver using
> matrix_keypad_data could be doing things differently. That's what I'm
Just FYI, correct name is matrix_keymap_data and current all keypad
drivers using matrix_keymap_data use it to same way.
> concerned about. Things will be much easier for driver writers if he
> knows the data passed in will always be reference-able.
>
--
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-22 4:00 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-22 4:00 UTC (permalink / raw)
To: linux-arm-kernel
On 6/22/2010 12:38 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
> <jy0922.shim@samsung.com> wrote:
>> On 6/22/2010 12:02 PM, Eric Miao wrote:
>>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>>>> +{
>>>>>>>>> + ? ? ? struct samsung_keypad_platdata *npd;
>>>>>>>>> +
>>>>>>>>> + ? ? ? if (!pd) {
>>>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>>>> + ? ? ? ? ? ? ? return;
>>>>>>>>> + ? ? ? }
>>>>>>>>> +
>>>>>>>>> + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>>>> + ? ? ? if (!npd)
>>>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>>>> This part of the code is actually duplicated again and again and again
>>>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>>>> major points:
>>>>>>>>
>>>>>>>> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>>>> ? ? device (more than 90% of the devices can be described that way),
>>>>>>>> ? ? and avoid using a comparatively heavier weight platform_device,
>>>>>>>> ? ? which can be generated at run-time
>>>>>>>>
>>>>>>>> ?2. pxa_register_device() to allocate and register the platform_device
>>>>>>>> ? ? at run-time, along with the platform data
>>>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>>>
>>>>>>>>> +struct samsung_keypad_platdata {
>>>>>>>>> + ? ? ? const struct matrix_keymap_data *keymap_data;
>>>>>>> What you end up with is some platform data structures which must be kept
>>>>>>> (those which have pointers to them from the platform data), and others
>>>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>>>
>>>>>>> We know that the __initdata attributations cause lots of problems -
>>>>>>> they're frequently wrong. ?Just see the constant hastle with __devinit
>>>>>>> et.al. ?The same issue happens with __initdata as well.
>>>>>>>
>>>>>>> So why make things more complicated by allowing some platform data
>>>>>>> structures to be discardable and others not to be? ?Is their small
>>>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>>>
>>>>>> Russell,
>>>>>>
>>>>>> The benefit I see is when multiple boards are compiled in, those
>>>>>> data not used can be automatically discarded.
>>>>> Yes, but only some of the data can be discarded. Continuing with the
>>>>> example in hand, while you can discard the six words which represent
>>>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>>>> be re-allocated, which is probably a much larger data structure.
>>>>>
>>>> No. the keymap_data is possible too. The keypad driver allocates other
>>>> keymap area of input device and it is assigned from datas based on this
>>>> keymap_data.
>>>>
>>> This is a generic issue. Even if in your example, you can avoid this by
>>> re-allocation and re-assignment (ignore the performance issue for such
>>> behavior), the real question is the difficult to track all these down. Since
>> Right, it can occur difficulty of maintain. I wanted just to inform the
>> current fact.
>>
>>> matrix_keypad_data is something out of your control (it was actually
>>> drafted by me and Dmitry if you are interested), and think about one day
>>> I changed it's definition, now you have to sync your driver and code every
>>> time to make sure the discarded data is not referenced.
>>>
>> if matrix_keypad_data is changed, i think the patchset should included
>> change of related other parts using it.
>>
>
> That's reasonable but difficult in practice, every keypad driver using
> matrix_keypad_data could be doing things differently. That's what I'm
Just FYI, correct name is matrix_keymap_data and current all keypad
drivers using matrix_keymap_data use it to same way.
> concerned about. Things will be much easier for driver writers if he
> knows the data passed in will always be reference-able.
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-22 4:00 ` Joonyoung Shim
@ 2010-06-22 7:15 ` Eric Miao
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-22 7:15 UTC (permalink / raw)
To: Joonyoung Shim
Cc: Russell King - ARM Linux, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On Tue, Jun 22, 2010 at 12:00 PM, Joonyoung Shim
<jy0922.shim@samsung.com> wrote:
> On 6/22/2010 12:38 PM, Eric Miao wrote:
>> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
>> <jy0922.shim@samsung.com> wrote:
>>> On 6/22/2010 12:02 PM, Eric Miao wrote:
>>>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>>>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>>>>> +{
>>>>>>>>>> + � � � struct samsung_keypad_platdata *npd;
>>>>>>>>>> +
>>>>>>>>>> + � � � if (!pd) {
>>>>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>>>>> + � � � � � � � return;
>>>>>>>>>> + � � � }
>>>>>>>>>> +
>>>>>>>>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>>>>> + � � � if (!npd)
>>>>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>>>>> This part of the code is actually duplicated again and again and again
>>>>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>>>>> major points:
>>>>>>>>>
>>>>>>>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>>>>> � � device (more than 90% of the devices can be described that way),
>>>>>>>>> � � and avoid using a comparatively heavier weight platform_device,
>>>>>>>>> � � which can be generated at run-time
>>>>>>>>>
>>>>>>>>> �2. pxa_register_device() to allocate and register the platform_device
>>>>>>>>> � � at run-time, along with the platform data
>>>>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>>>>
>>>>>>>>>> +struct samsung_keypad_platdata {
>>>>>>>>>> + � � � const struct matrix_keymap_data *keymap_data;
>>>>>>>> What you end up with is some platform data structures which must be kept
>>>>>>>> (those which have pointers to them from the platform data), and others
>>>>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>>>>
>>>>>>>> We know that the __initdata attributations cause lots of problems -
>>>>>>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>>>>>>> et.al. �The same issue happens with __initdata as well.
>>>>>>>>
>>>>>>>> So why make things more complicated by allowing some platform data
>>>>>>>> structures to be discardable and others not to be? �Is their small
>>>>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>>>>
>>>>>>> Russell,
>>>>>>>
>>>>>>> The benefit I see is when multiple boards are compiled in, those
>>>>>>> data not used can be automatically discarded.
>>>>>> Yes, but only some of the data can be discarded. Continuing with the
>>>>>> example in hand, while you can discard the six words which represent
>>>>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>>>>> be re-allocated, which is probably a much larger data structure.
>>>>>>
>>>>> No. the keymap_data is possible too. The keypad driver allocates other
>>>>> keymap area of input device and it is assigned from datas based on this
>>>>> keymap_data.
>>>>>
>>>> This is a generic issue. Even if in your example, you can avoid this by
>>>> re-allocation and re-assignment (ignore the performance issue for such
>>>> behavior), the real question is the difficult to track all these down. Since
>>> Right, it can occur difficulty of maintain. I wanted just to inform the
>>> current fact.
>>>
>>>> matrix_keypad_data is something out of your control (it was actually
>>>> drafted by me and Dmitry if you are interested), and think about one day
>>>> I changed it's definition, now you have to sync your driver and code every
>>>> time to make sure the discarded data is not referenced.
>>>>
>>> if matrix_keypad_data is changed, i think the patchset should included
>>> change of related other parts using it.
>>>
>>
>> That's reasonable but difficult in practice, every keypad driver using
>> matrix_keypad_data could be doing things differently. That's what I'm
>
> Just FYI, correct name is matrix_keymap_data and current all keypad
> drivers using matrix_keymap_data use it to same way.
>
Note I was just thinking there is a potential issue as been pointed out that
we can improve. And I'm not NACKing your perfect patch. Sorry if I made
you think so.
--
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
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-22 7:15 ` Eric Miao
0 siblings, 0 replies; 50+ messages in thread
From: Eric Miao @ 2010-06-22 7:15 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 22, 2010 at 12:00 PM, Joonyoung Shim
<jy0922.shim@samsung.com> wrote:
> On 6/22/2010 12:38 PM, Eric Miao wrote:
>> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
>> <jy0922.shim@samsung.com> wrote:
>>> On 6/22/2010 12:02 PM, Eric Miao wrote:
>>>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>>>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>>>>> +{
>>>>>>>>>> + ? ? ? struct samsung_keypad_platdata *npd;
>>>>>>>>>> +
>>>>>>>>>> + ? ? ? if (!pd) {
>>>>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>>>>> + ? ? ? ? ? ? ? return;
>>>>>>>>>> + ? ? ? }
>>>>>>>>>> +
>>>>>>>>>> + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>>>>> + ? ? ? if (!npd)
>>>>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>>>>> This part of the code is actually duplicated again and again and again
>>>>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>>>>> major points:
>>>>>>>>>
>>>>>>>>> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>>>>> ? ? device (more than 90% of the devices can be described that way),
>>>>>>>>> ? ? and avoid using a comparatively heavier weight platform_device,
>>>>>>>>> ? ? which can be generated at run-time
>>>>>>>>>
>>>>>>>>> ?2. pxa_register_device() to allocate and register the platform_device
>>>>>>>>> ? ? at run-time, along with the platform data
>>>>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>>>>
>>>>>>>>>> +struct samsung_keypad_platdata {
>>>>>>>>>> + ? ? ? const struct matrix_keymap_data *keymap_data;
>>>>>>>> What you end up with is some platform data structures which must be kept
>>>>>>>> (those which have pointers to them from the platform data), and others
>>>>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>>>>
>>>>>>>> We know that the __initdata attributations cause lots of problems -
>>>>>>>> they're frequently wrong. ?Just see the constant hastle with __devinit
>>>>>>>> et.al. ?The same issue happens with __initdata as well.
>>>>>>>>
>>>>>>>> So why make things more complicated by allowing some platform data
>>>>>>>> structures to be discardable and others not to be? ?Is their small
>>>>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>>>>
>>>>>>> Russell,
>>>>>>>
>>>>>>> The benefit I see is when multiple boards are compiled in, those
>>>>>>> data not used can be automatically discarded.
>>>>>> Yes, but only some of the data can be discarded. ?Continuing with the
>>>>>> example in hand, while you can discard the six words which represent
>>>>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>>>>> be re-allocated, which is probably a much larger data structure.
>>>>>>
>>>>> No. the keymap_data is possible too. The keypad driver allocates other
>>>>> keymap area of input device and it is assigned from datas based on this
>>>>> keymap_data.
>>>>>
>>>> This is a generic issue. Even if in your example, you can avoid this by
>>>> re-allocation and re-assignment (ignore the performance issue for such
>>>> behavior), the real question is the difficult to track all these down. Since
>>> Right, it can occur difficulty of maintain. I wanted just to inform the
>>> current fact.
>>>
>>>> matrix_keypad_data is something out of your control (it was actually
>>>> drafted by me and Dmitry if you are interested), and think about one day
>>>> I changed it's definition, now you have to sync your driver and code every
>>>> time to make sure the discarded data is not referenced.
>>>>
>>> if matrix_keypad_data is changed, i think the patchset should included
>>> change of related other parts using it.
>>>
>>
>> That's reasonable but difficult in practice, every keypad driver using
>> matrix_keypad_data could be doing things differently. That's what I'm
>
> Just FYI, correct name is matrix_keymap_data and current all keypad
> drivers using matrix_keymap_data use it to same way.
>
Note I was just thinking there is a potential issue as been pointed out that
we can improve. And I'm not NACKing your perfect patch. Sorry if I made
you think so.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
2010-06-22 7:15 ` Eric Miao
@ 2010-06-22 7:33 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-22 7:33 UTC (permalink / raw)
To: Eric Miao
Cc: Russell King - ARM Linux, linux-samsung-soc, dmitry.torokhov,
kyungmin.park, kgene.kim, ben-linux, linux-input,
linux-arm-kernel
On 6/22/2010 4:15 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 12:00 PM, Joonyoung Shim
> <jy0922.shim@samsung.com> wrote:
>> On 6/22/2010 12:38 PM, Eric Miao wrote:
>>> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
>>> <jy0922.shim@samsung.com> wrote:
>>>> On 6/22/2010 12:02 PM, Eric Miao wrote:
>>>>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>>>>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>>>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>>>>>> +{
>>>>>>>>>>> + � � � struct samsung_keypad_platdata *npd;
>>>>>>>>>>> +
>>>>>>>>>>> + � � � if (!pd) {
>>>>>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>>>>>> + � � � � � � � return;
>>>>>>>>>>> + � � � }
>>>>>>>>>>> +
>>>>>>>>>>> + � � � npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>>>>>> + � � � if (!npd)
>>>>>>>>>>> + � � � � � � � printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>>>>>> This part of the code is actually duplicated again and again and again
>>>>>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>>>>>> major points:
>>>>>>>>>>
>>>>>>>>>> �1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>>>>>> � � device (more than 90% of the devices can be described that way),
>>>>>>>>>> � � and avoid using a comparatively heavier weight platform_device,
>>>>>>>>>> � � which can be generated at run-time
>>>>>>>>>>
>>>>>>>>>> �2. pxa_register_device() to allocate and register the platform_device
>>>>>>>>>> � � at run-time, along with the platform data
>>>>>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>>>>>
>>>>>>>>>>> +struct samsung_keypad_platdata {
>>>>>>>>>>> + � � � const struct matrix_keymap_data *keymap_data;
>>>>>>>>> What you end up with is some platform data structures which must be kept
>>>>>>>>> (those which have pointers to them from the platform data), and others
>>>>>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>>>>>
>>>>>>>>> We know that the __initdata attributations cause lots of problems -
>>>>>>>>> they're frequently wrong. �Just see the constant hastle with __devinit
>>>>>>>>> et.al. �The same issue happens with __initdata as well.
>>>>>>>>>
>>>>>>>>> So why make things more complicated by allowing some platform data
>>>>>>>>> structures to be discardable and others not to be? �Is their small
>>>>>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>>>>>
>>>>>>>> Russell,
>>>>>>>>
>>>>>>>> The benefit I see is when multiple boards are compiled in, those
>>>>>>>> data not used can be automatically discarded.
>>>>>>> Yes, but only some of the data can be discarded. Continuing with the
>>>>>>> example in hand, while you can discard the six words which represent
>>>>>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>>>>>> be re-allocated, which is probably a much larger data structure.
>>>>>>>
>>>>>> No. the keymap_data is possible too. The keypad driver allocates other
>>>>>> keymap area of input device and it is assigned from datas based on this
>>>>>> keymap_data.
>>>>>>
>>>>> This is a generic issue. Even if in your example, you can avoid this by
>>>>> re-allocation and re-assignment (ignore the performance issue for such
>>>>> behavior), the real question is the difficult to track all these down. Since
>>>> Right, it can occur difficulty of maintain. I wanted just to inform the
>>>> current fact.
>>>>
>>>>> matrix_keypad_data is something out of your control (it was actually
>>>>> drafted by me and Dmitry if you are interested), and think about one day
>>>>> I changed it's definition, now you have to sync your driver and code every
>>>>> time to make sure the discarded data is not referenced.
>>>>>
>>>> if matrix_keypad_data is changed, i think the patchset should included
>>>> change of related other parts using it.
>>>>
>>> That's reasonable but difficult in practice, every keypad driver using
>>> matrix_keypad_data could be doing things differently. That's what I'm
>> Just FYI, correct name is matrix_keymap_data and current all keypad
>> drivers using matrix_keymap_data use it to same way.
>>
>
> Note I was just thinking there is a potential issue as been pointed out that
> we can improve. And I'm not NACKing your perfect patch. Sorry if I made
> you think so.
>
Don't mind me. I appreciate your interest.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support
@ 2010-06-22 7:33 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-22 7:33 UTC (permalink / raw)
To: linux-arm-kernel
On 6/22/2010 4:15 PM, Eric Miao wrote:
> On Tue, Jun 22, 2010 at 12:00 PM, Joonyoung Shim
> <jy0922.shim@samsung.com> wrote:
>> On 6/22/2010 12:38 PM, Eric Miao wrote:
>>> On Tue, Jun 22, 2010 at 11:27 AM, Joonyoung Shim
>>> <jy0922.shim@samsung.com> wrote:
>>>> On 6/22/2010 12:02 PM, Eric Miao wrote:
>>>>> On Tue, Jun 22, 2010 at 8:48 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>> On 6/21/2010 8:16 PM, Russell King - ARM Linux wrote:
>>>>>>> On Mon, Jun 21, 2010 at 06:39:10PM +0800, Eric Miao wrote:
>>>>>>>> On Mon, Jun 21, 2010 at 5:19 PM, Russell King - ARM Linux
>>>>>>>> <linux@arm.linux.org.uk> wrote:
>>>>>>>>> On Mon, Jun 21, 2010 at 05:05:34PM +0800, Eric Miao wrote:
>>>>>>>>>> On Mon, Jun 21, 2010 at 2:26 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>>>>>>>>>>> +void __init samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd)
>>>>>>>>>>> +{
>>>>>>>>>>> + ? ? ? struct samsung_keypad_platdata *npd;
>>>>>>>>>>> +
>>>>>>>>>>> + ? ? ? if (!pd) {
>>>>>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no platform data\n", __func__);
>>>>>>>>>>> + ? ? ? ? ? ? ? return;
>>>>>>>>>>> + ? ? ? }
>>>>>>>>>>> +
>>>>>>>>>>> + ? ? ? npd = kmemdup(pd, sizeof(struct samsung_keypad_platdata), GFP_KERNEL);
>>>>>>>>>>> + ? ? ? if (!npd)
>>>>>>>>>>> + ? ? ? ? ? ? ? printk(KERN_ERR "%s: no memory for platform data\n", __func__);
>>>>>>>>>> This part of the code is actually duplicated again and again and again
>>>>>>>>>> for each device, PXA and other legacy platforms are bad references for
>>>>>>>>>> this. In arch/arm/mach-mmp/, it might be a bit cleaner, there are three
>>>>>>>>>> major points:
>>>>>>>>>>
>>>>>>>>>> ?1. A minimum 'struct pxa_device_desc' for a simple description of a
>>>>>>>>>> ? ? device (more than 90% of the devices can be described that way),
>>>>>>>>>> ? ? and avoid using a comparatively heavier weight platform_device,
>>>>>>>>>> ? ? which can be generated at run-time
>>>>>>>>>>
>>>>>>>>>> ?2. pxa_register_device() to allocate and register the platform_device
>>>>>>>>>> ? ? at run-time, along with the platform data
>>>>>>>>> It's a bad idea to make platform data be run-time discardable like this:
>>>>>>>>>
>>>>>>>>>>> +struct samsung_keypad_platdata {
>>>>>>>>>>> + ? ? ? const struct matrix_keymap_data *keymap_data;
>>>>>>>>> What you end up with is some platform data structures which must be kept
>>>>>>>>> (those which have pointers to them from the platform data), and others
>>>>>>>>> (the platform data itself) which can be discarded at runtime.
>>>>>>>>>
>>>>>>>>> We know that the __initdata attributations cause lots of problems -
>>>>>>>>> they're frequently wrong. ?Just see the constant hastle with __devinit
>>>>>>>>> et.al. ?The same issue happens with __initdata as well.
>>>>>>>>>
>>>>>>>>> So why make things more complicated by allowing some platform data
>>>>>>>>> structures to be discardable and others not to be? ?Is their small
>>>>>>>>> size (maybe 6 words for this one) really worth the hastle of getting
>>>>>>>>> __initdata attributations wrong (eg, on the keymap data?)
>>>>>>>>>
>>>>>>>> Russell,
>>>>>>>>
>>>>>>>> The benefit I see is when multiple boards are compiled in, those
>>>>>>>> data not used can be automatically discarded.
>>>>>>> Yes, but only some of the data can be discarded. Continuing with the
>>>>>>> example in hand, while you can discard the six words which represent
>>>>>>> samsung_keypad_platdata, but the keymap_data can't be because that won't
>>>>>>> be re-allocated, which is probably a much larger data structure.
>>>>>>>
>>>>>> No. the keymap_data is possible too. The keypad driver allocates other
>>>>>> keymap area of input device and it is assigned from datas based on this
>>>>>> keymap_data.
>>>>>>
>>>>> This is a generic issue. Even if in your example, you can avoid this by
>>>>> re-allocation and re-assignment (ignore the performance issue for such
>>>>> behavior), the real question is the difficult to track all these down. Since
>>>> Right, it can occur difficulty of maintain. I wanted just to inform the
>>>> current fact.
>>>>
>>>>> matrix_keypad_data is something out of your control (it was actually
>>>>> drafted by me and Dmitry if you are interested), and think about one day
>>>>> I changed it's definition, now you have to sync your driver and code every
>>>>> time to make sure the discarded data is not referenced.
>>>>>
>>>> if matrix_keypad_data is changed, i think the patchset should included
>>>> change of related other parts using it.
>>>>
>>> That's reasonable but difficult in practice, every keypad driver using
>>> matrix_keypad_data could be doing things differently. That's what I'm
>> Just FYI, correct name is matrix_keymap_data and current all keypad
>> drivers using matrix_keymap_data use it to same way.
>>
>
> Note I was just thinking there is a potential issue as been pointed out that
> we can improve. And I'm not NACKing your perfect patch. Sorry if I made
> you think so.
>
Don't mind me. I appreciate your interest.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
2010-06-21 6:26 ` Joonyoung Shim
@ 2010-06-25 8:30 ` Dmitry Torokhov
-1 siblings, 0 replies; 50+ messages in thread
From: Dmitry Torokhov @ 2010-06-25 8:30 UTC (permalink / raw)
To: Joonyoung Shim
Cc: linux-arm-kernel, linux-samsung-soc, linux-input, ben-linux,
kyungmin.park, kgene.kim
Hi Joonyoung,
On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
> This patch adds support for keypad driver running on Samsung cpus. This
> driver is tested on GONI and Aquila board using S5PC110 cpu.
>
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Following my conversation with Thomas Gleixner reagrding "long playing"
threaded interrupt handlers I tried to convert your driver to use this
concept. The idea is to keep polling within IRQ thread context instead
of using additional work/timer structures to simplify code and ensure
race-free shutdown/unbind.
I think it was based on v4 of your driver and I'd appreciate if you could
give it a try.
Thank you.
--
Dmitry
Input: samsung-keypad - updates
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
arch/arm/plat-samsung/include/plat/keypad.h | 21 -
arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ---
drivers/input/keyboard/samsung-keypad.c | 334 ++++++++++++++--------
3 files changed, 215 insertions(+), 189 deletions(-)
delete mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
index 04ba600..0a35784 100644
--- a/arch/arm/plat-samsung/include/plat/keypad.h
+++ b/arch/arm/plat-samsung/include/plat/keypad.h
@@ -34,24 +34,11 @@
*/
struct samsung_keypad_platdata {
const struct matrix_keymap_data *keymap_data;
- unsigned int rows;
- unsigned int cols;
- unsigned int rep:1;
+ unsigned int rows;
+ unsigned int cols;
+ bool rep;
- void (*cfg_gpio)(unsigned int rows, unsigned int cols);
+ void (*cfg_gpio)(unsigned int rows, unsigned int cols);
};
-/**
- * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
- * @pd: Platform data to register to device.
- *
- * Register the given platform data for use with Samsung Keypad device.
- * The call will copy the platform data, so the board definitions can
- * make the structure itself __initdata.
- */
-extern void samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd);
-
-/* defined by architecture to configure gpio. */
-extern void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols);
-
#endif /* __PLAT_SAMSUNG_KEYPAD_H */
diff --git a/arch/arm/plat-samsung/include/plat/regs-keypad.h b/arch/arm/plat-samsung/include/plat/regs-keypad.h
deleted file mode 100644
index e4688f0..0000000
--- a/arch/arm/plat-samsung/include/plat/regs-keypad.h
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
- * linux/arch/arm/plat-samsung/include/plat/regs-keypad.h
- *
- * Copyright (C) 2010 Samsung Electronics Co.Ltd
- * Author: Joonyoung Shim <jy0922.shim@samsung.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.
- *
- */
-
-#ifndef __SAMSUNG_KEYPAD_H__
-#define __SAMSUNG_KEYPAD_H__
-
-#define SAMSUNG_KEYIFCON 0x00
-#define SAMSUNG_KEYIFSTSCLR 0x04
-#define SAMSUNG_KEYIFCOL 0x08
-#define SAMSUNG_KEYIFROW 0x0c
-#define SAMSUNG_KEYIFFC 0x10
-
-/* SAMSUNG_KEYIFCON */
-#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
-#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
-#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
-#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
-#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
-
-/* SAMSUNG_KEYIFSTSCLR */
-#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
-#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
-#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
-#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
-#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
-#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
-
-/* SAMSUNG_KEYIFCOL */
-#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
-#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
-
-/* SAMSUNG_KEYIFROW */
-#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
-#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
-
-/* SAMSUNG_KEYIFFC */
-#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
-
-#endif /* __SAMSUNG_KEYPAD_H__ */
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index 244a3b6..c243fc5 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -1,5 +1,5 @@
/*
- * samsung-keypad.c -- Samsung keypad driver
+ * Samsung keypad driver
*
* Copyright (C) 2010 Samsung Electronics Co.Ltd
* Author: Joonyoung Shim <jy0922.shim@samsung.com>
@@ -21,8 +21,45 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
-#include <plat/keypad.h>
-#include <plat/regs-keypad.h>
+//#include <plat/keypad.h>
+#include "/home/dtor/kernel/work/arch/arm/plat-samsung/include/plat/keypad.h"
+
+MODULE_DESCRIPTION("Samsung keypad driver");
+MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
+MODULE_AUTHOR("Donghwa Lee <dh09.lee@samsung.com>");
+MODULE_LICENSE("GPL");
+
+#define SAMSUNG_KEYIFCON 0x00
+#define SAMSUNG_KEYIFSTSCLR 0x04
+#define SAMSUNG_KEYIFCOL 0x08
+#define SAMSUNG_KEYIFROW 0x0c
+#define SAMSUNG_KEYIFFC 0x10
+
+/* SAMSUNG_KEYIFCON */
+#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
+#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
+#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
+#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
+#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
+
+/* SAMSUNG_KEYIFSTSCLR */
+#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
+#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
+#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
+#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
+#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
+#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
+
+/* SAMSUNG_KEYIFCOL */
+#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
+#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
+
+/* SAMSUNG_KEYIFROW */
+#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
+#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
+
+/* SAMSUNG_KEYIFFC */
+#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
enum samsung_keypad_type {
KEYPAD_TYPE_SAMSUNG,
@@ -31,29 +68,28 @@ enum samsung_keypad_type {
struct samsung_keypad {
struct input_dev *input_dev;
- struct timer_list timer;
struct clk *clk;
- struct work_struct work;
void __iomem *base;
- unsigned short *keycodes;
+ wait_queue_head_t wait;
+ bool stopped;
+ int irq;
unsigned int row_shift;
unsigned int rows;
unsigned int cols;
unsigned int row_state[SAMSUNG_MAX_COLS];
- int irq;
+ unsigned short keycodes[];
};
static int samsung_keypad_is_s5pv210(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- enum samsung_keypad_type type;
+ enum samsung_keypad_type type = platform_get_device_id(pdev)->driver_data;
- type = platform_get_device_id(pdev)->driver_data;
return type == KEYPAD_TYPE_S5PV210;
}
static void samsung_keypad_scan(struct samsung_keypad *keypad,
- unsigned int *row_state)
+ unsigned int *row_state)
{
struct device *dev = keypad->input_dev->dev.parent;
unsigned int col;
@@ -79,29 +115,15 @@ static void samsung_keypad_scan(struct samsung_keypad *keypad,
writel(0, keypad->base + SAMSUNG_KEYIFCOL);
}
-static void samsung_keypad_worker(struct work_struct *work)
+static bool samsung_keypad_report(struct samsung_keypad *keypad,
+ unsigned int *row_state)
{
- struct samsung_keypad *keypad = container_of(work,
- struct samsung_keypad, work);
- unsigned int row_state[SAMSUNG_MAX_COLS];
- unsigned int val;
+ struct input_dev *input_dev = keypad->input_dev;
unsigned int changed;
unsigned int pressed;
unsigned int key_down = 0;
- int col, row;
-
- clk_enable(keypad->clk);
-
- val = readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
-
- /* interrupt clear */
- writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
-
- val = readl(keypad->base + SAMSUNG_KEYIFCON);
- val &= ~(SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
- writel(val, keypad->base + SAMSUNG_KEYIFCON);
-
- samsung_keypad_scan(keypad, row_state);
+ unsigned int val;
+ unsigned int col, row;
for (col = 0; col < keypad->cols; col++) {
changed = row_state[col] ^ keypad->row_state[col];
@@ -121,42 +143,108 @@ static void samsung_keypad_worker(struct work_struct *work)
val = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
- input_report_key(keypad->input_dev,
+ input_event(input_dev, EV_MSC, MSC_SCAN, val);
+ input_report_key(input_dev,
keypad->keycodes[val], pressed);
- input_sync(keypad->input_dev);
}
+ input_sync(keypad->input_dev);
}
+
memcpy(keypad->row_state, row_state, sizeof(row_state));
- if (key_down)
- mod_timer(&keypad->timer, jiffies + HZ / 20);
- else {
- /* enable interrupt bit */
- val = readl(keypad->base + SAMSUNG_KEYIFCON);
- val |= (SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
- writel(val, keypad->base + SAMSUNG_KEYIFCON);
- enable_irq(keypad->irq);
- }
- clk_disable(keypad->clk);
+ return key_down;
}
-static irqreturn_t samsung_keypad_interrupt(int irq, void *dev_id)
+static irqreturn_t samsung_keypad_irq(int irq, void *dev_id)
{
struct samsung_keypad *keypad = dev_id;
+ unsigned int row_state[SAMSUNG_MAX_COLS];
+ unsigned int val;
+ bool key_down;
- if (!work_pending(&keypad->work)) {
- disable_irq_nosync(keypad->irq);
- schedule_work(&keypad->work);
- }
+ do {
+ clk_enable(keypad->clk);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
+ /* Clear interrupt. */
+ writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+ samsung_keypad_scan(keypad, row_state);
+
+ clk_disable(keypad->clk);
+
+ key_down = samsung_keypad_report(keypad, row_state);
+ if (key_down)
+ wait_event_timeout(keypad->wait, keypad->stopped,
+ msecs_to_jiffies(50));
+
+ } while (key_down && !keypad->stopped);
return IRQ_HANDLED;
}
-static void samsung_keypad_timer(unsigned long data)
+static void samsung_keypad_start(struct samsung_keypad *keypad)
+{
+ unsigned int val;
+
+ /* Tell IRQ thread that it may poll the device. */
+ keypad->stopped = false;
+
+ clk_enable(keypad->clk);
+
+ /* Enable interrupt bits. */
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ val |= SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN;
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ /* KEYIFCOL reg clear. */
+ writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+
+ clk_disable(keypad->clk);
+}
+
+static void samsung_keypad_stop(struct samsung_keypad *keypad)
{
- struct samsung_keypad *keypad = (struct samsung_keypad *)data;
+ unsigned int val;
+
+ /* Signal IRQ thread to stop polling and disable the handler. */
+ keypad->stopped = true;
+ wake_up(&keypad->wait);
+ disable_irq(keypad->irq);
- schedule_work(&keypad->work);
+ clk_enable(keypad->clk);
+
+ /* Clear interrupt. */
+ writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+ /* Disable interrupt bits. */
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ val &= ~(SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ clk_disable(keypad->clk);
+
+ /*
+ * Now that chip should not generate interrupts we can safely
+ * re-enable the handler.
+ */
+ enable_irq(keypad->irq);
+}
+
+static int samsung_keypad_open(struct input_dev *input_dev)
+{
+ struct samsung_keypad *keypad = input_get_drvdata(input_dev);
+
+ samsung_keypad_start(keypad);
+
+ return 0;
+}
+
+static void samsung_keypad_close(struct input_dev *input_dev)
+{
+ struct samsung_keypad *keypad = input_get_drvdata(input_dev);
+
+ samsung_keypad_stop(keypad);
}
static int __devinit samsung_keypad_probe(struct platform_device *pdev)
@@ -166,10 +254,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
struct samsung_keypad *keypad;
struct resource *res;
struct input_dev *input_dev;
- unsigned short *keycodes;
unsigned int row_shift;
- unsigned int val;
- int ret;
+ unsigned int keymap_size;
+ int error;
pdata = pdev->dev.platform_data;
if (!pdata) {
@@ -183,10 +270,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
return -EINVAL;
}
- if (!pdata->rows || (pdata->rows > SAMSUNG_MAX_ROWS))
+ if (!pdata->rows || pdata->rows > SAMSUNG_MAX_ROWS)
return -EINVAL;
- if (!pdata->cols || (pdata->cols > SAMSUNG_MAX_COLS))
+ if (!pdata->cols || pdata->cols > SAMSUNG_MAX_COLS)
return -EINVAL;
/* initialize the gpio */
@@ -194,133 +281,144 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
pdata->cfg_gpio(pdata->rows, pdata->cols);
row_shift = get_count_order(pdata->cols);
- keypad = kzalloc(sizeof(*keypad), GFP_KERNEL);
- keycodes = kzalloc((pdata->rows << row_shift) * sizeof(*keycodes),
- GFP_KERNEL);
+ keymap_size = (pdata->rows << row_shift) * sizeof(keypad->keycodes[0]);
+
+ keypad = kzalloc(sizeof(*keypad) + keymap_size, GFP_KERNEL);
input_dev = input_allocate_device();
- if (!keypad || !keycodes || !input_dev) {
- ret = -ENOMEM;
+ if (!keypad || !input_dev) {
+ error = -ENOMEM;
goto err_free_mem;
}
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
- ret = -ENODEV;
+ error = -ENODEV;
goto err_free_mem;
}
keypad->base = ioremap(res->start, resource_size(res));
if (!keypad->base) {
- ret = -EBUSY;
+ error = -EBUSY;
goto err_free_mem;
}
keypad->clk = clk_get(&pdev->dev, "keypad");
if (IS_ERR(keypad->clk)) {
dev_err(&pdev->dev, "failed to get keypad clk\n");
- ret = PTR_ERR(keypad->clk);
+ error = PTR_ERR(keypad->clk);
goto err_unmap_base;
}
- clk_enable(keypad->clk);
keypad->input_dev = input_dev;
- keypad->keycodes = keycodes;
keypad->row_shift = row_shift;
keypad->rows = pdata->rows;
keypad->cols = pdata->cols;
-
- INIT_WORK(&keypad->work, samsung_keypad_worker);
-
- setup_timer(&keypad->timer, samsung_keypad_timer,
- (unsigned long)keypad);
-
- /* enable interrupt and wakeup bit */
- val = SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN |
- SAMSUNG_KEYIFCON_WAKEUPEN;
- writel(val, keypad->base + SAMSUNG_KEYIFCON);
-
- /* KEYIFCOL reg clear */
- writel(0, keypad->base + SAMSUNG_KEYIFCOL);
-
- keypad->irq = platform_get_irq(pdev, 0);
- if (keypad->irq < 0) {
- ret = keypad->irq;
- goto err_disable_clk;
- }
-
- ret = request_irq(keypad->irq, samsung_keypad_interrupt, 0,
- dev_name(&pdev->dev), keypad);
-
- if (ret)
- goto err_disable_clk;
+ init_waitqueue_head(&keypad->wait);
input_dev->name = pdev->name;
input_dev->id.bustype = BUS_HOST;
input_dev->dev.parent = &pdev->dev;
+ input_set_drvdata(input_dev, keypad);
+
+ input_dev->open = samsung_keypad_open;
+ input_dev->close = samsung_keypad_close;
input_dev->evbit[0] = BIT_MASK(EV_KEY);
if (pdata->rep)
input_dev->evbit[0] |= BIT_MASK(EV_REP);
- input_dev->keycode = keycodes;
- input_dev->keycodesize = sizeof(*keycodes);
+ input_dev->keycode = keypad->keycodes;
+ input_dev->keycodesize = sizeof(keypad->keycodes[0]);
input_dev->keycodemax = pdata->rows << row_shift;
matrix_keypad_build_keymap(keymap_data, row_shift,
input_dev->keycode, input_dev->keybit);
- ret = input_register_device(keypad->input_dev);
- if (ret)
+ keypad->irq = platform_get_irq(pdev, 0);
+ if (keypad->irq < 0) {
+ error = keypad->irq;
+ goto err_put_clk;
+ }
+
+ error = request_threaded_irq(keypad->irq, NULL, samsung_keypad_irq,
+ IRQF_ONESHOT, dev_name(&pdev->dev), keypad);
+ if (error)
+ goto err_put_clk;
+
+ error = input_register_device(keypad->input_dev);
+ if (error)
goto err_free_irq;
platform_set_drvdata(pdev, keypad);
- clk_disable(keypad->clk);
-
return 0;
err_free_irq:
free_irq(keypad->irq, keypad);
-err_disable_clk:
- clk_disable(keypad->clk);
+err_put_clk:
clk_put(keypad->clk);
err_unmap_base:
iounmap(keypad->base);
err_free_mem:
input_free_device(input_dev);
- kfree(keycodes);
kfree(keypad);
- return ret;
+ return error;
}
static int __devexit samsung_keypad_remove(struct platform_device *pdev)
{
struct samsung_keypad *keypad = platform_get_drvdata(pdev);
- free_irq(keypad->irq, keypad);
- cancel_work_sync(&keypad->work);
- del_timer_sync(&keypad->timer);
-
platform_set_drvdata(pdev, NULL);
+
input_unregister_device(keypad->input_dev);
- clk_disable(keypad->clk);
+ /*
+ * It is safe to free IRQ after unregistering device because
+ * samsung_keypad_close will shut off interrupts.
+ */
+ free_irq(keypad->irq, keypad);
+
clk_put(keypad->clk);
iounmap(keypad->base);
- kfree(keypad->keycodes);
kfree(keypad);
return 0;
}
#ifdef CONFIG_PM
+static void samsung_keypad_toggle_wakeup(struct samsung_keypad *keypad,
+ bool enable)
+{
+ unsigned int val;
+
+ clk_enable(keypad->clk);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ if (enable)
+ val |= SAMSUNG_KEYIFCON_WAKEUPEN;
+ else
+ val &= ~SAMSUNG_KEYIFCON_WAKEUPEN;
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ clk_disable(keypad->clk);
+}
+
static int samsung_keypad_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+ struct input_dev *input_dev = keypad->input_dev;
- disable_irq(keypad->irq);
+ mutex_lock(&input_dev->mutex);
+
+ if (input_dev->users)
+ samsung_keypad_stop(keypad);
+
+ samsung_keypad_toggle_wakeup(keypad, true);
+
+ mutex_unlock(&input_dev->mutex);
return 0;
}
@@ -329,20 +427,16 @@ static int samsung_keypad_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct samsung_keypad *keypad = platform_get_drvdata(pdev);
- unsigned int val;
+ struct input_dev *input_dev = keypad->input_dev;
- clk_enable(keypad->clk);
+ mutex_lock(&input_dev->mutex);
- /* enable interrupt and wakeup bit */
- val = SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN |
- SAMSUNG_KEYIFCON_WAKEUPEN;
- writel(val, keypad->base + SAMSUNG_KEYIFCON);
+ samsung_keypad_toggle_wakeup(keypad, false);
- /* KEYIFCOL reg clear */
- writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+ if (input_dev->users)
+ samsung_keypad_start(keypad);
- enable_irq(keypad->irq);
- clk_disable(keypad->clk);
+ mutex_unlock(&input_dev->mutex);
return 0;
}
@@ -382,16 +476,10 @@ static int __init samsung_keypad_init(void)
{
return platform_driver_register(&samsung_keypad_driver);
}
+module_init(samsung_keypad_init);
static void __exit samsung_keypad_exit(void)
{
platform_driver_unregister(&samsung_keypad_driver);
}
-
-module_init(samsung_keypad_init);
module_exit(samsung_keypad_exit);
-
-MODULE_DESCRIPTION("Samsung keypad driver");
-MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
-MODULE_AUTHOR("Donghwa Lee <dh09.lee@samsung.com>");
-MODULE_LICENSE("GPL");
^ permalink raw reply related [flat|nested] 50+ messages in thread
* [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
@ 2010-06-25 8:30 ` Dmitry Torokhov
0 siblings, 0 replies; 50+ messages in thread
From: Dmitry Torokhov @ 2010-06-25 8:30 UTC (permalink / raw)
To: linux-arm-kernel
Hi Joonyoung,
On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
> This patch adds support for keypad driver running on Samsung cpus. This
> driver is tested on GONI and Aquila board using S5PC110 cpu.
>
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Following my conversation with Thomas Gleixner reagrding "long playing"
threaded interrupt handlers I tried to convert your driver to use this
concept. The idea is to keep polling within IRQ thread context instead
of using additional work/timer structures to simplify code and ensure
race-free shutdown/unbind.
I think it was based on v4 of your driver and I'd appreciate if you could
give it a try.
Thank you.
--
Dmitry
Input: samsung-keypad - updates
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
arch/arm/plat-samsung/include/plat/keypad.h | 21 -
arch/arm/plat-samsung/include/plat/regs-keypad.h | 49 ---
drivers/input/keyboard/samsung-keypad.c | 334 ++++++++++++++--------
3 files changed, 215 insertions(+), 189 deletions(-)
delete mode 100644 arch/arm/plat-samsung/include/plat/regs-keypad.h
diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
index 04ba600..0a35784 100644
--- a/arch/arm/plat-samsung/include/plat/keypad.h
+++ b/arch/arm/plat-samsung/include/plat/keypad.h
@@ -34,24 +34,11 @@
*/
struct samsung_keypad_platdata {
const struct matrix_keymap_data *keymap_data;
- unsigned int rows;
- unsigned int cols;
- unsigned int rep:1;
+ unsigned int rows;
+ unsigned int cols;
+ bool rep;
- void (*cfg_gpio)(unsigned int rows, unsigned int cols);
+ void (*cfg_gpio)(unsigned int rows, unsigned int cols);
};
-/**
- * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
- * @pd: Platform data to register to device.
- *
- * Register the given platform data for use with Samsung Keypad device.
- * The call will copy the platform data, so the board definitions can
- * make the structure itself __initdata.
- */
-extern void samsung_keypad_set_platdata(struct samsung_keypad_platdata *pd);
-
-/* defined by architecture to configure gpio. */
-extern void samsung_keypad_cfg_gpio(unsigned int rows, unsigned int cols);
-
#endif /* __PLAT_SAMSUNG_KEYPAD_H */
diff --git a/arch/arm/plat-samsung/include/plat/regs-keypad.h b/arch/arm/plat-samsung/include/plat/regs-keypad.h
deleted file mode 100644
index e4688f0..0000000
--- a/arch/arm/plat-samsung/include/plat/regs-keypad.h
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
- * linux/arch/arm/plat-samsung/include/plat/regs-keypad.h
- *
- * Copyright (C) 2010 Samsung Electronics Co.Ltd
- * Author: Joonyoung Shim <jy0922.shim@samsung.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.
- *
- */
-
-#ifndef __SAMSUNG_KEYPAD_H__
-#define __SAMSUNG_KEYPAD_H__
-
-#define SAMSUNG_KEYIFCON 0x00
-#define SAMSUNG_KEYIFSTSCLR 0x04
-#define SAMSUNG_KEYIFCOL 0x08
-#define SAMSUNG_KEYIFROW 0x0c
-#define SAMSUNG_KEYIFFC 0x10
-
-/* SAMSUNG_KEYIFCON */
-#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
-#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
-#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
-#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
-#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
-
-/* SAMSUNG_KEYIFSTSCLR */
-#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
-#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
-#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
-#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
-#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
-#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
-
-/* SAMSUNG_KEYIFCOL */
-#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
-#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
-
-/* SAMSUNG_KEYIFROW */
-#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
-#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
-
-/* SAMSUNG_KEYIFFC */
-#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
-
-#endif /* __SAMSUNG_KEYPAD_H__ */
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index 244a3b6..c243fc5 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -1,5 +1,5 @@
/*
- * samsung-keypad.c -- Samsung keypad driver
+ * Samsung keypad driver
*
* Copyright (C) 2010 Samsung Electronics Co.Ltd
* Author: Joonyoung Shim <jy0922.shim@samsung.com>
@@ -21,8 +21,45 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
-#include <plat/keypad.h>
-#include <plat/regs-keypad.h>
+//#include <plat/keypad.h>
+#include "/home/dtor/kernel/work/arch/arm/plat-samsung/include/plat/keypad.h"
+
+MODULE_DESCRIPTION("Samsung keypad driver");
+MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
+MODULE_AUTHOR("Donghwa Lee <dh09.lee@samsung.com>");
+MODULE_LICENSE("GPL");
+
+#define SAMSUNG_KEYIFCON 0x00
+#define SAMSUNG_KEYIFSTSCLR 0x04
+#define SAMSUNG_KEYIFCOL 0x08
+#define SAMSUNG_KEYIFROW 0x0c
+#define SAMSUNG_KEYIFFC 0x10
+
+/* SAMSUNG_KEYIFCON */
+#define SAMSUNG_KEYIFCON_INT_F_EN (1 << 0)
+#define SAMSUNG_KEYIFCON_INT_R_EN (1 << 1)
+#define SAMSUNG_KEYIFCON_DF_EN (1 << 2)
+#define SAMSUNG_KEYIFCON_FC_EN (1 << 3)
+#define SAMSUNG_KEYIFCON_WAKEUPEN (1 << 4)
+
+/* SAMSUNG_KEYIFSTSCLR */
+#define SAMSUNG_KEYIFSTSCLR_P_INT_MASK (0xff << 0)
+#define SAMSUNG_KEYIFSTSCLR_R_INT_MASK (0xff << 8)
+#define SAMSUNG_KEYIFSTSCLR_R_INT_OFFSET 8
+#define S5PV210_KEYIFSTSCLR_P_INT_MASK (0x3fff << 0)
+#define S5PV210_KEYIFSTSCLR_R_INT_MASK (0x3fff << 16)
+#define S5PV210_KEYIFSTSCLR_R_INT_OFFSET 16
+
+/* SAMSUNG_KEYIFCOL */
+#define SAMSUNG_KEYIFCOL_MASK (0xff << 0)
+#define S5PV210_KEYIFCOLEN_MASK (0xff << 8)
+
+/* SAMSUNG_KEYIFROW */
+#define SAMSUNG_KEYIFROW_MASK (0xff << 0)
+#define S5PV210_KEYIFROW_MASK (0x3fff << 0)
+
+/* SAMSUNG_KEYIFFC */
+#define SAMSUNG_KEYIFFC_MASK (0x3ff << 0)
enum samsung_keypad_type {
KEYPAD_TYPE_SAMSUNG,
@@ -31,29 +68,28 @@ enum samsung_keypad_type {
struct samsung_keypad {
struct input_dev *input_dev;
- struct timer_list timer;
struct clk *clk;
- struct work_struct work;
void __iomem *base;
- unsigned short *keycodes;
+ wait_queue_head_t wait;
+ bool stopped;
+ int irq;
unsigned int row_shift;
unsigned int rows;
unsigned int cols;
unsigned int row_state[SAMSUNG_MAX_COLS];
- int irq;
+ unsigned short keycodes[];
};
static int samsung_keypad_is_s5pv210(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
- enum samsung_keypad_type type;
+ enum samsung_keypad_type type = platform_get_device_id(pdev)->driver_data;
- type = platform_get_device_id(pdev)->driver_data;
return type == KEYPAD_TYPE_S5PV210;
}
static void samsung_keypad_scan(struct samsung_keypad *keypad,
- unsigned int *row_state)
+ unsigned int *row_state)
{
struct device *dev = keypad->input_dev->dev.parent;
unsigned int col;
@@ -79,29 +115,15 @@ static void samsung_keypad_scan(struct samsung_keypad *keypad,
writel(0, keypad->base + SAMSUNG_KEYIFCOL);
}
-static void samsung_keypad_worker(struct work_struct *work)
+static bool samsung_keypad_report(struct samsung_keypad *keypad,
+ unsigned int *row_state)
{
- struct samsung_keypad *keypad = container_of(work,
- struct samsung_keypad, work);
- unsigned int row_state[SAMSUNG_MAX_COLS];
- unsigned int val;
+ struct input_dev *input_dev = keypad->input_dev;
unsigned int changed;
unsigned int pressed;
unsigned int key_down = 0;
- int col, row;
-
- clk_enable(keypad->clk);
-
- val = readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
-
- /* interrupt clear */
- writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
-
- val = readl(keypad->base + SAMSUNG_KEYIFCON);
- val &= ~(SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
- writel(val, keypad->base + SAMSUNG_KEYIFCON);
-
- samsung_keypad_scan(keypad, row_state);
+ unsigned int val;
+ unsigned int col, row;
for (col = 0; col < keypad->cols; col++) {
changed = row_state[col] ^ keypad->row_state[col];
@@ -121,42 +143,108 @@ static void samsung_keypad_worker(struct work_struct *work)
val = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
- input_report_key(keypad->input_dev,
+ input_event(input_dev, EV_MSC, MSC_SCAN, val);
+ input_report_key(input_dev,
keypad->keycodes[val], pressed);
- input_sync(keypad->input_dev);
}
+ input_sync(keypad->input_dev);
}
+
memcpy(keypad->row_state, row_state, sizeof(row_state));
- if (key_down)
- mod_timer(&keypad->timer, jiffies + HZ / 20);
- else {
- /* enable interrupt bit */
- val = readl(keypad->base + SAMSUNG_KEYIFCON);
- val |= (SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
- writel(val, keypad->base + SAMSUNG_KEYIFCON);
- enable_irq(keypad->irq);
- }
- clk_disable(keypad->clk);
+ return key_down;
}
-static irqreturn_t samsung_keypad_interrupt(int irq, void *dev_id)
+static irqreturn_t samsung_keypad_irq(int irq, void *dev_id)
{
struct samsung_keypad *keypad = dev_id;
+ unsigned int row_state[SAMSUNG_MAX_COLS];
+ unsigned int val;
+ bool key_down;
- if (!work_pending(&keypad->work)) {
- disable_irq_nosync(keypad->irq);
- schedule_work(&keypad->work);
- }
+ do {
+ clk_enable(keypad->clk);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
+ /* Clear interrupt. */
+ writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+ samsung_keypad_scan(keypad, row_state);
+
+ clk_disable(keypad->clk);
+
+ key_down = samsung_keypad_report(keypad, row_state);
+ if (key_down)
+ wait_event_timeout(keypad->wait, keypad->stopped,
+ msecs_to_jiffies(50));
+
+ } while (key_down && !keypad->stopped);
return IRQ_HANDLED;
}
-static void samsung_keypad_timer(unsigned long data)
+static void samsung_keypad_start(struct samsung_keypad *keypad)
+{
+ unsigned int val;
+
+ /* Tell IRQ thread that it may poll the device. */
+ keypad->stopped = false;
+
+ clk_enable(keypad->clk);
+
+ /* Enable interrupt bits. */
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ val |= SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN;
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ /* KEYIFCOL reg clear. */
+ writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+
+ clk_disable(keypad->clk);
+}
+
+static void samsung_keypad_stop(struct samsung_keypad *keypad)
{
- struct samsung_keypad *keypad = (struct samsung_keypad *)data;
+ unsigned int val;
+
+ /* Signal IRQ thread to stop polling and disable the handler. */
+ keypad->stopped = true;
+ wake_up(&keypad->wait);
+ disable_irq(keypad->irq);
- schedule_work(&keypad->work);
+ clk_enable(keypad->clk);
+
+ /* Clear interrupt. */
+ writel(~0x0, keypad->base + SAMSUNG_KEYIFSTSCLR);
+
+ /* Disable interrupt bits. */
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ val &= ~(SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN);
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ clk_disable(keypad->clk);
+
+ /*
+ * Now that chip should not generate interrupts we can safely
+ * re-enable the handler.
+ */
+ enable_irq(keypad->irq);
+}
+
+static int samsung_keypad_open(struct input_dev *input_dev)
+{
+ struct samsung_keypad *keypad = input_get_drvdata(input_dev);
+
+ samsung_keypad_start(keypad);
+
+ return 0;
+}
+
+static void samsung_keypad_close(struct input_dev *input_dev)
+{
+ struct samsung_keypad *keypad = input_get_drvdata(input_dev);
+
+ samsung_keypad_stop(keypad);
}
static int __devinit samsung_keypad_probe(struct platform_device *pdev)
@@ -166,10 +254,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
struct samsung_keypad *keypad;
struct resource *res;
struct input_dev *input_dev;
- unsigned short *keycodes;
unsigned int row_shift;
- unsigned int val;
- int ret;
+ unsigned int keymap_size;
+ int error;
pdata = pdev->dev.platform_data;
if (!pdata) {
@@ -183,10 +270,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
return -EINVAL;
}
- if (!pdata->rows || (pdata->rows > SAMSUNG_MAX_ROWS))
+ if (!pdata->rows || pdata->rows > SAMSUNG_MAX_ROWS)
return -EINVAL;
- if (!pdata->cols || (pdata->cols > SAMSUNG_MAX_COLS))
+ if (!pdata->cols || pdata->cols > SAMSUNG_MAX_COLS)
return -EINVAL;
/* initialize the gpio */
@@ -194,133 +281,144 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
pdata->cfg_gpio(pdata->rows, pdata->cols);
row_shift = get_count_order(pdata->cols);
- keypad = kzalloc(sizeof(*keypad), GFP_KERNEL);
- keycodes = kzalloc((pdata->rows << row_shift) * sizeof(*keycodes),
- GFP_KERNEL);
+ keymap_size = (pdata->rows << row_shift) * sizeof(keypad->keycodes[0]);
+
+ keypad = kzalloc(sizeof(*keypad) + keymap_size, GFP_KERNEL);
input_dev = input_allocate_device();
- if (!keypad || !keycodes || !input_dev) {
- ret = -ENOMEM;
+ if (!keypad || !input_dev) {
+ error = -ENOMEM;
goto err_free_mem;
}
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
- ret = -ENODEV;
+ error = -ENODEV;
goto err_free_mem;
}
keypad->base = ioremap(res->start, resource_size(res));
if (!keypad->base) {
- ret = -EBUSY;
+ error = -EBUSY;
goto err_free_mem;
}
keypad->clk = clk_get(&pdev->dev, "keypad");
if (IS_ERR(keypad->clk)) {
dev_err(&pdev->dev, "failed to get keypad clk\n");
- ret = PTR_ERR(keypad->clk);
+ error = PTR_ERR(keypad->clk);
goto err_unmap_base;
}
- clk_enable(keypad->clk);
keypad->input_dev = input_dev;
- keypad->keycodes = keycodes;
keypad->row_shift = row_shift;
keypad->rows = pdata->rows;
keypad->cols = pdata->cols;
-
- INIT_WORK(&keypad->work, samsung_keypad_worker);
-
- setup_timer(&keypad->timer, samsung_keypad_timer,
- (unsigned long)keypad);
-
- /* enable interrupt and wakeup bit */
- val = SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN |
- SAMSUNG_KEYIFCON_WAKEUPEN;
- writel(val, keypad->base + SAMSUNG_KEYIFCON);
-
- /* KEYIFCOL reg clear */
- writel(0, keypad->base + SAMSUNG_KEYIFCOL);
-
- keypad->irq = platform_get_irq(pdev, 0);
- if (keypad->irq < 0) {
- ret = keypad->irq;
- goto err_disable_clk;
- }
-
- ret = request_irq(keypad->irq, samsung_keypad_interrupt, 0,
- dev_name(&pdev->dev), keypad);
-
- if (ret)
- goto err_disable_clk;
+ init_waitqueue_head(&keypad->wait);
input_dev->name = pdev->name;
input_dev->id.bustype = BUS_HOST;
input_dev->dev.parent = &pdev->dev;
+ input_set_drvdata(input_dev, keypad);
+
+ input_dev->open = samsung_keypad_open;
+ input_dev->close = samsung_keypad_close;
input_dev->evbit[0] = BIT_MASK(EV_KEY);
if (pdata->rep)
input_dev->evbit[0] |= BIT_MASK(EV_REP);
- input_dev->keycode = keycodes;
- input_dev->keycodesize = sizeof(*keycodes);
+ input_dev->keycode = keypad->keycodes;
+ input_dev->keycodesize = sizeof(keypad->keycodes[0]);
input_dev->keycodemax = pdata->rows << row_shift;
matrix_keypad_build_keymap(keymap_data, row_shift,
input_dev->keycode, input_dev->keybit);
- ret = input_register_device(keypad->input_dev);
- if (ret)
+ keypad->irq = platform_get_irq(pdev, 0);
+ if (keypad->irq < 0) {
+ error = keypad->irq;
+ goto err_put_clk;
+ }
+
+ error = request_threaded_irq(keypad->irq, NULL, samsung_keypad_irq,
+ IRQF_ONESHOT, dev_name(&pdev->dev), keypad);
+ if (error)
+ goto err_put_clk;
+
+ error = input_register_device(keypad->input_dev);
+ if (error)
goto err_free_irq;
platform_set_drvdata(pdev, keypad);
- clk_disable(keypad->clk);
-
return 0;
err_free_irq:
free_irq(keypad->irq, keypad);
-err_disable_clk:
- clk_disable(keypad->clk);
+err_put_clk:
clk_put(keypad->clk);
err_unmap_base:
iounmap(keypad->base);
err_free_mem:
input_free_device(input_dev);
- kfree(keycodes);
kfree(keypad);
- return ret;
+ return error;
}
static int __devexit samsung_keypad_remove(struct platform_device *pdev)
{
struct samsung_keypad *keypad = platform_get_drvdata(pdev);
- free_irq(keypad->irq, keypad);
- cancel_work_sync(&keypad->work);
- del_timer_sync(&keypad->timer);
-
platform_set_drvdata(pdev, NULL);
+
input_unregister_device(keypad->input_dev);
- clk_disable(keypad->clk);
+ /*
+ * It is safe to free IRQ after unregistering device because
+ * samsung_keypad_close will shut off interrupts.
+ */
+ free_irq(keypad->irq, keypad);
+
clk_put(keypad->clk);
iounmap(keypad->base);
- kfree(keypad->keycodes);
kfree(keypad);
return 0;
}
#ifdef CONFIG_PM
+static void samsung_keypad_toggle_wakeup(struct samsung_keypad *keypad,
+ bool enable)
+{
+ unsigned int val;
+
+ clk_enable(keypad->clk);
+
+ val = readl(keypad->base + SAMSUNG_KEYIFCON);
+ if (enable)
+ val |= SAMSUNG_KEYIFCON_WAKEUPEN;
+ else
+ val &= ~SAMSUNG_KEYIFCON_WAKEUPEN;
+ writel(val, keypad->base + SAMSUNG_KEYIFCON);
+
+ clk_disable(keypad->clk);
+}
+
static int samsung_keypad_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct samsung_keypad *keypad = platform_get_drvdata(pdev);
+ struct input_dev *input_dev = keypad->input_dev;
- disable_irq(keypad->irq);
+ mutex_lock(&input_dev->mutex);
+
+ if (input_dev->users)
+ samsung_keypad_stop(keypad);
+
+ samsung_keypad_toggle_wakeup(keypad, true);
+
+ mutex_unlock(&input_dev->mutex);
return 0;
}
@@ -329,20 +427,16 @@ static int samsung_keypad_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct samsung_keypad *keypad = platform_get_drvdata(pdev);
- unsigned int val;
+ struct input_dev *input_dev = keypad->input_dev;
- clk_enable(keypad->clk);
+ mutex_lock(&input_dev->mutex);
- /* enable interrupt and wakeup bit */
- val = SAMSUNG_KEYIFCON_INT_F_EN | SAMSUNG_KEYIFCON_INT_R_EN |
- SAMSUNG_KEYIFCON_WAKEUPEN;
- writel(val, keypad->base + SAMSUNG_KEYIFCON);
+ samsung_keypad_toggle_wakeup(keypad, false);
- /* KEYIFCOL reg clear */
- writel(0, keypad->base + SAMSUNG_KEYIFCOL);
+ if (input_dev->users)
+ samsung_keypad_start(keypad);
- enable_irq(keypad->irq);
- clk_disable(keypad->clk);
+ mutex_unlock(&input_dev->mutex);
return 0;
}
@@ -382,16 +476,10 @@ static int __init samsung_keypad_init(void)
{
return platform_driver_register(&samsung_keypad_driver);
}
+module_init(samsung_keypad_init);
static void __exit samsung_keypad_exit(void)
{
platform_driver_unregister(&samsung_keypad_driver);
}
-
-module_init(samsung_keypad_init);
module_exit(samsung_keypad_exit);
-
-MODULE_DESCRIPTION("Samsung keypad driver");
-MODULE_AUTHOR("Joonyoung Shim <jy0922.shim@samsung.com>");
-MODULE_AUTHOR("Donghwa Lee <dh09.lee@samsung.com>");
-MODULE_LICENSE("GPL");
^ permalink raw reply related [flat|nested] 50+ messages in thread
* Re: [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
2010-06-25 8:30 ` Dmitry Torokhov
@ 2010-06-25 10:25 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-25 10:25 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: kgene.kim, kyungmin.park, linux-samsung-soc, ben-linux,
linux-input, linux-arm-kernel
On 6/25/2010 5:30 PM, Dmitry Torokhov wrote:
> Hi Joonyoung,
>
> On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
>> This patch adds support for keypad driver running on Samsung cpus. This
>> driver is tested on GONI and Aquila board using S5PC110 cpu.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> Following my conversation with Thomas Gleixner reagrding "long playing"
> threaded interrupt handlers I tried to convert your driver to use this
> concept. The idea is to keep polling within IRQ thread context instead
> of using additional work/timer structures to simplify code and ensure
> race-free shutdown/unbind.
>
> I think it was based on v4 of your driver and I'd appreciate if you could
> give it a try.
>
Your patch gives me below patch errors.
$ patch -p1 --dry-run < 2.patch
(Stripping trailing CRs from patch.)
patching file drivers/input/keyboard/samsung-keypad.c
Hunk #3 FAILED at 68.
Hunk #4 FAILED at 116.
Hunk #5 FAILED at 158.
Hunk #6 succeeded at 196 (offset -7 lines).
Hunk #7 succeeded at 212 (offset -7 lines).
Hunk #8 FAILED at 230.
Hunk #9 FAILED at 365.
Hunk #10 FAILED at 418.
6 out of 10 hunks FAILED -- saving rejects to file
drivers/input/keyboard/samsung-keypad.c.rej
Can you give me your patch based on my v5 patches? If not, i should try
manual merge.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
@ 2010-06-25 10:25 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-25 10:25 UTC (permalink / raw)
To: linux-arm-kernel
On 6/25/2010 5:30 PM, Dmitry Torokhov wrote:
> Hi Joonyoung,
>
> On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
>> This patch adds support for keypad driver running on Samsung cpus. This
>> driver is tested on GONI and Aquila board using S5PC110 cpu.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> Following my conversation with Thomas Gleixner reagrding "long playing"
> threaded interrupt handlers I tried to convert your driver to use this
> concept. The idea is to keep polling within IRQ thread context instead
> of using additional work/timer structures to simplify code and ensure
> race-free shutdown/unbind.
>
> I think it was based on v4 of your driver and I'd appreciate if you could
> give it a try.
>
Your patch gives me below patch errors.
$ patch -p1 --dry-run < 2.patch
(Stripping trailing CRs from patch.)
patching file drivers/input/keyboard/samsung-keypad.c
Hunk #3 FAILED at 68.
Hunk #4 FAILED at 116.
Hunk #5 FAILED at 158.
Hunk #6 succeeded at 196 (offset -7 lines).
Hunk #7 succeeded at 212 (offset -7 lines).
Hunk #8 FAILED at 230.
Hunk #9 FAILED at 365.
Hunk #10 FAILED@418.
6 out of 10 hunks FAILED -- saving rejects to file
drivers/input/keyboard/samsung-keypad.c.rej
Can you give me your patch based on my v5 patches? If not, i should try
manual merge.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
2010-06-25 10:25 ` Joonyoung Shim
@ 2010-06-25 10:39 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-25 10:39 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-samsung-soc, kyungmin.park, kgene.kim, ben-linux,
linux-input, linux-arm-kernel
On 6/25/2010 7:25 PM, Joonyoung Shim wrote:
> On 6/25/2010 5:30 PM, Dmitry Torokhov wrote:
>> Hi Joonyoung,
>>
>> On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
>>> This patch adds support for keypad driver running on Samsung cpus. This
>>> driver is tested on GONI and Aquila board using S5PC110 cpu.
>>>
>>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Following my conversation with Thomas Gleixner reagrding "long playing"
>> threaded interrupt handlers I tried to convert your driver to use this
>> concept. The idea is to keep polling within IRQ thread context instead
>> of using additional work/timer structures to simplify code and ensure
>> race-free shutdown/unbind.
>>
>> I think it was based on v4 of your driver and I'd appreciate if you could
>> give it a try.
>>
>
> Your patch gives me below patch errors.
>
Your patch maybe seems be based on my v3 patch :)
> $ patch -p1 --dry-run < 2.patch
> (Stripping trailing CRs from patch.)
> patching file drivers/input/keyboard/samsung-keypad.c
> Hunk #3 FAILED at 68.
> Hunk #4 FAILED at 116.
> Hunk #5 FAILED at 158.
> Hunk #6 succeeded at 196 (offset -7 lines).
> Hunk #7 succeeded at 212 (offset -7 lines).
> Hunk #8 FAILED at 230.
> Hunk #9 FAILED at 365.
> Hunk #10 FAILED at 418.
> 6 out of 10 hunks FAILED -- saving rejects to file
> drivers/input/keyboard/samsung-keypad.c.rej
>
> Can you give me your patch based on my v5 patches? If not, i should try
> manual merge.
>
> Thanks.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
@ 2010-06-25 10:39 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-25 10:39 UTC (permalink / raw)
To: linux-arm-kernel
On 6/25/2010 7:25 PM, Joonyoung Shim wrote:
> On 6/25/2010 5:30 PM, Dmitry Torokhov wrote:
>> Hi Joonyoung,
>>
>> On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
>>> This patch adds support for keypad driver running on Samsung cpus. This
>>> driver is tested on GONI and Aquila board using S5PC110 cpu.
>>>
>>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Following my conversation with Thomas Gleixner reagrding "long playing"
>> threaded interrupt handlers I tried to convert your driver to use this
>> concept. The idea is to keep polling within IRQ thread context instead
>> of using additional work/timer structures to simplify code and ensure
>> race-free shutdown/unbind.
>>
>> I think it was based on v4 of your driver and I'd appreciate if you could
>> give it a try.
>>
>
> Your patch gives me below patch errors.
>
Your patch maybe seems be based on my v3 patch :)
> $ patch -p1 --dry-run < 2.patch
> (Stripping trailing CRs from patch.)
> patching file drivers/input/keyboard/samsung-keypad.c
> Hunk #3 FAILED at 68.
> Hunk #4 FAILED at 116.
> Hunk #5 FAILED at 158.
> Hunk #6 succeeded at 196 (offset -7 lines).
> Hunk #7 succeeded at 212 (offset -7 lines).
> Hunk #8 FAILED at 230.
> Hunk #9 FAILED at 365.
> Hunk #10 FAILED at 418.
> 6 out of 10 hunks FAILED -- saving rejects to file
> drivers/input/keyboard/samsung-keypad.c.rej
>
> Can you give me your patch based on my v5 patches? If not, i should try
> manual merge.
>
> Thanks.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
2010-06-25 8:30 ` Dmitry Torokhov
@ 2010-06-28 8:39 ` Joonyoung Shim
-1 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-28 8:39 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: kgene.kim, kyungmin.park, linux-samsung-soc, ben-linux,
linux-input, linux-arm-kernel
Hi, Dmitry.
On 6/25/2010 5:30 PM, Dmitry Torokhov wrote:
> Hi Joonyoung,
>
> On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
>> This patch adds support for keypad driver running on Samsung cpus. This
>> driver is tested on GONI and Aquila board using S5PC110 cpu.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> Following my conversation with Thomas Gleixner reagrding "long playing"
> threaded interrupt handlers I tried to convert your driver to use this
> concept. The idea is to keep polling within IRQ thread context instead
> of using additional work/timer structures to simplify code and ensure
> race-free shutdown/unbind.
>
> I think it was based on v4 of your driver and I'd appreciate if you could
> give it a try.
>
> Thank you.
>
I have tested your patch on my v5 patchset and it is working after some
fixing. I will post v6 patchset appling your patch.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
@ 2010-06-28 8:39 ` Joonyoung Shim
0 siblings, 0 replies; 50+ messages in thread
From: Joonyoung Shim @ 2010-06-28 8:39 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Dmitry.
On 6/25/2010 5:30 PM, Dmitry Torokhov wrote:
> Hi Joonyoung,
>
> On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
>> This patch adds support for keypad driver running on Samsung cpus. This
>> driver is tested on GONI and Aquila board using S5PC110 cpu.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> Following my conversation with Thomas Gleixner reagrding "long playing"
> threaded interrupt handlers I tried to convert your driver to use this
> concept. The idea is to keep polling within IRQ thread context instead
> of using additional work/timer structures to simplify code and ensure
> race-free shutdown/unbind.
>
> I think it was based on v4 of your driver and I'd appreciate if you could
> give it a try.
>
> Thank you.
>
I have tested your patch on my v5 patchset and it is working after some
fixing. I will post v6 patchset appling your patch.
Thanks.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
2010-06-28 8:39 ` Joonyoung Shim
@ 2010-06-28 9:01 ` Dmitry Torokhov
-1 siblings, 0 replies; 50+ messages in thread
From: Dmitry Torokhov @ 2010-06-28 9:01 UTC (permalink / raw)
To: Joonyoung Shim
Cc: linux-arm-kernel, linux-samsung-soc, linux-input, ben-linux,
kyungmin.park, kgene.kim
On Mon, Jun 28, 2010 at 05:39:39PM +0900, Joonyoung Shim wrote:
> Hi, Dmitry.
>
> On 6/25/2010 5:30 PM, Dmitry Torokhov wrote:
> > Hi Joonyoung,
> >
> > On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
> >> This patch adds support for keypad driver running on Samsung cpus. This
> >> driver is tested on GONI and Aquila board using S5PC110 cpu.
> >>
> >> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >
> > Following my conversation with Thomas Gleixner reagrding "long playing"
> > threaded interrupt handlers I tried to convert your driver to use this
> > concept. The idea is to keep polling within IRQ thread context instead
> > of using additional work/timer structures to simplify code and ensure
> > race-free shutdown/unbind.
> >
> > I think it was based on v4 of your driver and I'd appreciate if you could
> > give it a try.
> >
> > Thank you.
> >
>
> I have tested your patch on my v5 patchset and it is working after some
> fixing. I will post v6 patchset appling your patch.
Cool, thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver
@ 2010-06-28 9:01 ` Dmitry Torokhov
0 siblings, 0 replies; 50+ messages in thread
From: Dmitry Torokhov @ 2010-06-28 9:01 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 28, 2010 at 05:39:39PM +0900, Joonyoung Shim wrote:
> Hi, Dmitry.
>
> On 6/25/2010 5:30 PM, Dmitry Torokhov wrote:
> > Hi Joonyoung,
> >
> > On Mon, Jun 21, 2010 at 03:26:45PM +0900, Joonyoung Shim wrote:
> >> This patch adds support for keypad driver running on Samsung cpus. This
> >> driver is tested on GONI and Aquila board using S5PC110 cpu.
> >>
> >> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >
> > Following my conversation with Thomas Gleixner reagrding "long playing"
> > threaded interrupt handlers I tried to convert your driver to use this
> > concept. The idea is to keep polling within IRQ thread context instead
> > of using additional work/timer structures to simplify code and ensure
> > race-free shutdown/unbind.
> >
> > I think it was based on v4 of your driver and I'd appreciate if you could
> > give it a try.
> >
> > Thank you.
> >
>
> I have tested your patch on my v5 patchset and it is working after some
> fixing. I will post v6 patchset appling your patch.
Cool, thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 50+ messages in thread
end of thread, other threads:[~2010-06-28 9:01 UTC | newest]
Thread overview: 50+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-06-21 6:26 [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support Joonyoung Shim
2010-06-21 6:26 ` Joonyoung Shim
2010-06-21 6:26 ` [PATCH v5 2/3] ARM: S5PV210: Add keypad device helpers Joonyoung Shim
2010-06-21 6:26 ` Joonyoung Shim
2010-06-21 6:26 ` [PATCH v5 3/3] input: samsung-keypad - Add samsung keypad driver Joonyoung Shim
2010-06-21 6:26 ` Joonyoung Shim
2010-06-25 8:30 ` Dmitry Torokhov
2010-06-25 8:30 ` Dmitry Torokhov
2010-06-25 10:25 ` Joonyoung Shim
2010-06-25 10:25 ` Joonyoung Shim
2010-06-25 10:39 ` Joonyoung Shim
2010-06-25 10:39 ` Joonyoung Shim
2010-06-28 8:39 ` Joonyoung Shim
2010-06-28 8:39 ` Joonyoung Shim
2010-06-28 9:01 ` Dmitry Torokhov
2010-06-28 9:01 ` Dmitry Torokhov
2010-06-21 9:05 ` [PATCH v5 1/3] ARM: SAMSUNG: Add keypad device support Eric Miao
2010-06-21 9:05 ` Eric Miao
2010-06-21 9:19 ` Russell King - ARM Linux
2010-06-21 9:19 ` Russell King - ARM Linux
2010-06-21 10:39 ` Eric Miao
2010-06-21 10:39 ` Eric Miao
2010-06-21 11:16 ` Russell King - ARM Linux
2010-06-21 11:16 ` Russell King - ARM Linux
2010-06-21 14:15 ` Eric Miao
2010-06-21 14:15 ` Eric Miao
2010-06-22 0:48 ` Joonyoung Shim
2010-06-22 0:48 ` Joonyoung Shim
2010-06-22 3:02 ` Eric Miao
2010-06-22 3:02 ` Eric Miao
2010-06-22 3:27 ` Joonyoung Shim
2010-06-22 3:27 ` Joonyoung Shim
2010-06-22 3:38 ` Eric Miao
2010-06-22 3:38 ` Eric Miao
2010-06-22 4:00 ` Joonyoung Shim
2010-06-22 4:00 ` Joonyoung Shim
2010-06-22 7:15 ` Eric Miao
2010-06-22 7:15 ` Eric Miao
2010-06-22 7:33 ` Joonyoung Shim
2010-06-22 7:33 ` Joonyoung Shim
2010-06-21 9:29 ` Marek Szyprowski
2010-06-21 9:29 ` Marek Szyprowski
2010-06-21 10:43 ` Eric Miao
2010-06-21 10:43 ` Eric Miao
2010-06-21 11:21 ` Joonyoung Shim
2010-06-21 11:21 ` Joonyoung Shim
2010-06-21 14:19 ` Eric Miao
2010-06-21 14:19 ` Eric Miao
2010-06-22 0:18 ` Kukjin Kim
2010-06-22 0:18 ` Kukjin Kim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.