[PATCH v2 0/2] Add device tree support for Samsung's keypad controller driver

[PATCH v2 0/2] Add device tree support for Samsung's keypad controller driver

[Thomas Abraham]

Changes since v1:
- Addressed comments from Dmitry Torokhov.
  - Type of controller is cached in driver's private data and the function
    that determines the type of the controller for each keypad scan is
    removed.
  - pdata allocated during probe is explicitly freed after probe completes
    without any error. In case of exit from probe due a error, no explicit
    deallocation of pdata memory is performed and it is left to devres to
    handle that.
  - The generic code to translate KEY(x,y,code) is retained in the driver
    itself. I am unsure of a right common place for it.
  - Driver continues with the probe even if one or more gpio configuration
    fails.
  - Patch 1 that adds a new config option is still retained in this patchset.


This patchset adds device tree support for samsung's keypad controller driver.
First patch adds a new config option to be used by device tree enabled platforms
for selecting the samsung's keypad controller driver. The second patch adds device
tree support for the keypad driver.

Thomas Abraham (2):
  input: samsung-keypad: Add HAVE_SAMSUNG_KEYPAD config option
  input: samsung-keypad: Add device tree support

 .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
 drivers/input/keyboard/Kconfig                     |    9 +-
 drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
 3 files changed, 261 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt

[PATCH v2 0/2] Add device tree support for Samsung's keypad controller driver

[Thomas Abraham]

Changes since v1:
- Addressed comments from Dmitry Torokhov.
  - Type of controller is cached in driver's private data and the function
    that determines the type of the controller for each keypad scan is
    removed.
  - pdata allocated during probe is explicitly freed after probe completes
    without any error. In case of exit from probe due a error, no explicit
    deallocation of pdata memory is performed and it is left to devres to
    handle that.
  - The generic code to translate KEY(x,y,code) is retained in the driver
    itself. I am unsure of a right common place for it.
  - Driver continues with the probe even if one or more gpio configuration
    fails.
  - Patch 1 that adds a new config option is still retained in this patchset.


This patchset adds device tree support for samsung's keypad controller driver.
First patch adds a new config option to be used by device tree enabled platforms
for selecting the samsung's keypad controller driver. The second patch adds device
tree support for the keypad driver.

Thomas Abraham (2):
  input: samsung-keypad: Add HAVE_SAMSUNG_KEYPAD config option
  input: samsung-keypad: Add device tree support

 .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
 drivers/input/keyboard/Kconfig                     |    9 +-
 drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
 3 files changed, 261 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt

[PATCH 1/2] input: samsung-keypad: Add HAVE_SAMSUNG_KEYPAD config option

[Thomas Abraham]

For platforms using device tree, the static keypad device instances
are not required and SAMSUNG_DEV_KEYPAD is not selected. Since,
samsung keypad driver has dependency on SAMSUNG_DEV_KEYPAD config
option, the driver is left out of the compilation for dt enabled
platforms.

An additional config option 'HAVE_SAMSUNG_KEYPAD' is added
which the device tree based platforms can select. This config
option is added as an alternative dependency for keypad driver.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/input/keyboard/Kconfig |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index b4dee9d..7c322a3 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -423,9 +423,16 @@ config KEYBOARD_PMIC8XXX
 	  To compile this driver as a module, choose M here: the module will
 	  be called pmic8xxx-keypad.
 
+config HAVE_SAMSUNG_KEYPAD
+	bool
+	help
+	  This will include Samsung Keypad controller driver support. If you
+	  want to include Samsung Keypad support for any machine, kindly
+	  select this in the respective mach-xxxx/Kconfig file.
+
 config KEYBOARD_SAMSUNG
 	tristate "Samsung keypad support"
-	depends on SAMSUNG_DEV_KEYPAD
+	depends on SAMSUNG_DEV_KEYPAD || HAVE_SAMSUNG_KEYPAD
 	help
 	  Say Y here if you want to use the Samsung keypad.
 
-- 
1.6.6.rc2

[PATCH 1/2] input: samsung-keypad: Add HAVE_SAMSUNG_KEYPAD config option

[Thomas Abraham]

For platforms using device tree, the static keypad device instances
are not required and SAMSUNG_DEV_KEYPAD is not selected. Since,
samsung keypad driver has dependency on SAMSUNG_DEV_KEYPAD config
option, the driver is left out of the compilation for dt enabled
platforms.

An additional config option 'HAVE_SAMSUNG_KEYPAD' is added
which the device tree based platforms can select. This config
option is added as an alternative dependency for keypad driver.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/input/keyboard/Kconfig |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index b4dee9d..7c322a3 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -423,9 +423,16 @@ config KEYBOARD_PMIC8XXX
 	  To compile this driver as a module, choose M here: the module will
 	  be called pmic8xxx-keypad.
 
+config HAVE_SAMSUNG_KEYPAD
+	bool
+	help
+	  This will include Samsung Keypad controller driver support. If you
+	  want to include Samsung Keypad support for any machine, kindly
+	  select this in the respective mach-xxxx/Kconfig file.
+
 config KEYBOARD_SAMSUNG
 	tristate "Samsung keypad support"
-	depends on SAMSUNG_DEV_KEYPAD
+	depends on SAMSUNG_DEV_KEYPAD || HAVE_SAMSUNG_KEYPAD
 	help
 	  Say Y here if you want to use the Samsung keypad.
 
-- 
1.6.6.rc2

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Add device tree based discovery support for Samsung's keypad controller.

Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Donghwa Lee <dh09.lee@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
 drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
 2 files changed, 253 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt

diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
new file mode 100644
index 0000000..e1c7237
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
@@ -0,0 +1,88 @@
+* Samsung's Keypad Controller device tree bindings
+
+Samsung's Keypad controller is used to interface a SoC with a matrix-type
+keypad device. The keypad controller supports multiple row and column lines.
+A key can be placed at each intersection of a unique row and a unique column.
+The keypad controller can sense a key-press and key-release and report the
+event using a interrupt to the cpu.
+
+Required SoC Specific Properties:
+- compatible: should be one of the following
+  - "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
+    controller.
+  - "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
+    controller.
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- interrupts: The interrupt number to the cpu.
+
+Required Board Specific Properties:
+- samsung,keypad-num-rows: Number of row lines connected to the keypad
+  controller.
+
+- samsung,keypad-num-columns: Number of column lines connected to the
+  keypad controller.
+
+- row-gpios: List of gpios used as row lines. The gpio specifier for
+  this property depends on the gpio controller to which these row lines
+  are connected.
+
+- col-gpios: List of gpios used as column lines. The gpio specifier for
+  this property depends on the gpio controller to which these column
+  lines are connected.
+
+- Keys represented as child nodes: Each key connected to the keypad
+  controller is represented as a child node to the keypad controller
+  device node and should include the following properties.
+  - keypad,row: the row number to which the key is connected.
+  - keypad,column: the column number to which the key is connected.
+  - keypad,key-code: the key-code to be reported when the key is pressed
+    and released.
+
+Optional Properties specific to linux:
+- linux,keypad-no-autorepeat: do no enable autorepeat feature.
+- linux,keypad-wakeup: use any event on keypad as wakeup event.
+
+
+Example:
+	keypad@100A0000 {
+		compatible = "samsung,s5pv210-keypad";
+		reg = <0x100A0000 0x100>;
+		interrupts = <173>;
+		samsung,keypad-num-rows = <2>;
+		samsung,keypad-num-columns = <8>;
+		linux,input-no-autorepeat;
+		linux,input-wakeup;
+
+		row-gpios = <&gpx2 0 3 3 0
+			     &gpx2 1 3 3 0>;
+
+		col-gpios = <&gpx1 0 3 0 0
+			     &gpx1 1 3 0 0
+			     &gpx1 2 3 0 0
+			     &gpx1 3 3 0 0
+			     &gpx1 4 3 0 0
+			     &gpx1 5 3 0 0
+			     &gpx1 6 3 0 0
+			     &gpx1 7 3 0 0>;
+
+		key_1 {
+			keypad,row = <0>;
+			keypad,column = <3>;
+			keypad,key-code = <2>;
+		};
+
+		key_2 {
+			keypad,row = <0>;
+			keypad,column = <4>;
+			keypad,key-code = <3>;
+		};
+
+		key_3 {
+			keypad,row = <0>;
+			keypad,column = <5>;
+			keypad,key-code = <4>;
+		};
+	};
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index f689f49..cf01a56 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -21,6 +21,8 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/sched.h>
 #include <plat/keypad.h>
 
@@ -68,31 +70,26 @@ struct samsung_keypad {
 	wait_queue_head_t wait;
 	bool stopped;
 	int irq;
+	enum samsung_keypad_type type;
 	unsigned int row_shift;
 	unsigned int rows;
 	unsigned int cols;
 	unsigned int row_state[SAMSUNG_MAX_COLS];
+#ifdef CONFIG_OF
+	int row_gpios[SAMSUNG_MAX_ROWS];
+	int col_gpios[SAMSUNG_MAX_COLS];
+#endif
 	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 =
-		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)) {
+		if (keypad->type == KEYPAD_TYPE_S5PV210) {
 			val = S5PV210_KEYIFCOLEN_MASK;
 			val &= ~(1 << col) << 8;
 		} else {
@@ -235,6 +232,129 @@ static void samsung_keypad_close(struct input_dev *input_dev)
 	samsung_keypad_stop(keypad);
 }
 
+#ifdef CONFIG_OF
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+	struct samsung_keypad_platdata *pdata;
+	struct matrix_keymap_data *keymap_data;
+	uint32_t *keymap;
+	struct device_node *np = dev->of_node, *key_np;
+	unsigned int key_count = 0;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "could not allocate memory for platform data\n");
+		return NULL;
+	}
+
+	of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
+	of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
+	if (!pdata->rows || !pdata->cols) {
+		dev_err(dev, "number of keypad rows/columns not specified\n");
+		return NULL;
+	}
+
+	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
+	if (!keymap_data) {
+		dev_err(dev, "could not allocate memory for keymap data\n");
+		return NULL;
+	}
+	pdata->keymap_data = keymap_data;
+
+	for_each_child_of_node(np, key_np)
+		key_count++;
+
+	keymap_data->keymap_size = key_count;
+	keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
+	if (!keymap) {
+		dev_err(dev, "could not allocate memory for keymap\n");
+		return NULL;
+	}
+	keymap_data->keymap = keymap;
+
+	for_each_child_of_node(np, key_np) {
+		unsigned int row, col, key_code;
+		of_property_read_u32(key_np, "keypad,row", &row);
+		of_property_read_u32(key_np, "keypad,column", &col);
+		of_property_read_u32(key_np, "keypad,key-code", &key_code);
+		*keymap++ = KEY(row, col, key_code);
+	}
+
+	if (of_get_property(np, "linux,input-no-autorepeat", NULL))
+		pdata->no_autorepeat = true;
+	if (of_get_property(np, "linux,input-wakeup", NULL))
+		pdata->wakeup = true;
+
+	return pdata;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device *dev,
+				struct samsung_keypad *keypad)
+{
+	struct device_node *np = dev->of_node;
+	int gpio, ret, row, col;
+
+	for (row = 0; row < keypad->rows; row++) {
+		gpio = of_get_named_gpio(np, "row-gpios", row);
+		keypad->row_gpios[row] = gpio;
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
+					row, gpio);
+			continue;
+		}
+
+		ret = gpio_request(gpio, "keypad-row");
+		if (ret)
+			dev_err(dev, "keypad row[%d] gpio request failed\n",
+					row);
+	}
+
+	for (col = 0; col < keypad->cols; col++) {
+		gpio = of_get_named_gpio(np, "col-gpios", col);
+		keypad->col_gpios[col] = gpio;
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
+					col, gpio);
+			continue;
+		}
+
+		ret = gpio_request(col, "keypad-col");
+		if (ret)
+			dev_err(dev, "keypad column[%d] gpio request failed\n",
+					col);
+	}
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+	int cnt;
+
+	for (cnt = 0; cnt < keypad->rows; cnt++)
+		if (gpio_is_valid(keypad->row_gpios[cnt]))
+			gpio_free(keypad->row_gpios[cnt]);
+
+	for (cnt = 0; cnt < keypad->cols; cnt++)
+		if (gpio_is_valid(keypad->col_gpios[cnt]))
+			gpio_free(keypad->col_gpios[cnt]);
+}
+#else
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device *dev,
+				struct samsung_keypad *keypad)
+{
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+}
+#endif
+
 static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 {
 	const struct samsung_keypad_platdata *pdata;
@@ -246,7 +366,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 	unsigned int keymap_size;
 	int error;
 
-	pdata = pdev->dev.platform_data;
+	if (pdev->dev.of_node)
+		pdata = samsung_keypad_parse_dt(&pdev->dev);
+	else
+		pdata = pdev->dev.platform_data;
 	if (!pdata) {
 		dev_err(&pdev->dev, "no platform data defined\n");
 		return -EINVAL;
@@ -303,6 +426,16 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 	keypad->cols = pdata->cols;
 	init_waitqueue_head(&keypad->wait);
 
+	if (pdev->dev.of_node) {
+		samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
+#ifdef CONFIG_OF
+		keypad->type = of_device_is_compatible(pdev->dev.of_node,
+					"samsung,s5pv210-keypad");
+#endif
+	} else {
+		keypad->type = platform_get_device_id(pdev)->driver_data;
+	}
+
 	input_dev->name = pdev->name;
 	input_dev->id.bustype = BUS_HOST;
 	input_dev->dev.parent = &pdev->dev;
@@ -343,12 +476,19 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 	platform_set_drvdata(pdev, keypad);
+
+	if (pdev->dev.of_node) {
+		devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
+		devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
+		devm_kfree(&pdev->dev, (void *)pdata);
+	}
 	return 0;
 
 err_free_irq:
 	free_irq(keypad->irq, keypad);
 err_put_clk:
 	clk_put(keypad->clk);
+	samsung_keypad_dt_gpio_free(keypad);
 err_unmap_base:
 	iounmap(keypad->base);
 err_free_mem:
@@ -374,6 +514,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
 	free_irq(keypad->irq, keypad);
 
 	clk_put(keypad->clk);
+	samsung_keypad_dt_gpio_free(keypad);
 
 	iounmap(keypad->base);
 	kfree(keypad);
@@ -447,6 +588,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
 };
 #endif
 
+#ifdef CONFIG_OF
+static const struct of_device_id samsung_keypad_dt_match[] = {
+	{ .compatible = "samsung,s3c6410-keypad" },
+	{ .compatible = "samsung,s5pv210-keypad" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
+#else
+#define samsung_keypad_dt_match NULL
+#endif
+
 static struct platform_device_id samsung_keypad_driver_ids[] = {
 	{
 		.name		= "samsung-keypad",
@@ -465,6 +617,7 @@ static struct platform_driver samsung_keypad_driver = {
 	.driver		= {
 		.name	= "samsung-keypad",
 		.owner	= THIS_MODULE,
+		.of_match_table = samsung_keypad_dt_match,
 #ifdef CONFIG_PM
 		.pm	= &samsung_keypad_pm_ops,
 #endif
-- 
1.6.6.rc2

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Add device tree based discovery support for Samsung's keypad controller.

Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Donghwa Lee <dh09.lee@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
 drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
 2 files changed, 253 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt

diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
new file mode 100644
index 0000000..e1c7237
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
@@ -0,0 +1,88 @@
+* Samsung's Keypad Controller device tree bindings
+
+Samsung's Keypad controller is used to interface a SoC with a matrix-type
+keypad device. The keypad controller supports multiple row and column lines.
+A key can be placed at each intersection of a unique row and a unique column.
+The keypad controller can sense a key-press and key-release and report the
+event using a interrupt to the cpu.
+
+Required SoC Specific Properties:
+- compatible: should be one of the following
+  - "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
+    controller.
+  - "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
+    controller.
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- interrupts: The interrupt number to the cpu.
+
+Required Board Specific Properties:
+- samsung,keypad-num-rows: Number of row lines connected to the keypad
+  controller.
+
+- samsung,keypad-num-columns: Number of column lines connected to the
+  keypad controller.
+
+- row-gpios: List of gpios used as row lines. The gpio specifier for
+  this property depends on the gpio controller to which these row lines
+  are connected.
+
+- col-gpios: List of gpios used as column lines. The gpio specifier for
+  this property depends on the gpio controller to which these column
+  lines are connected.
+
+- Keys represented as child nodes: Each key connected to the keypad
+  controller is represented as a child node to the keypad controller
+  device node and should include the following properties.
+  - keypad,row: the row number to which the key is connected.
+  - keypad,column: the column number to which the key is connected.
+  - keypad,key-code: the key-code to be reported when the key is pressed
+    and released.
+
+Optional Properties specific to linux:
+- linux,keypad-no-autorepeat: do no enable autorepeat feature.
+- linux,keypad-wakeup: use any event on keypad as wakeup event.
+
+
+Example:
+	keypad at 100A0000 {
+		compatible = "samsung,s5pv210-keypad";
+		reg = <0x100A0000 0x100>;
+		interrupts = <173>;
+		samsung,keypad-num-rows = <2>;
+		samsung,keypad-num-columns = <8>;
+		linux,input-no-autorepeat;
+		linux,input-wakeup;
+
+		row-gpios = <&gpx2 0 3 3 0
+			     &gpx2 1 3 3 0>;
+
+		col-gpios = <&gpx1 0 3 0 0
+			     &gpx1 1 3 0 0
+			     &gpx1 2 3 0 0
+			     &gpx1 3 3 0 0
+			     &gpx1 4 3 0 0
+			     &gpx1 5 3 0 0
+			     &gpx1 6 3 0 0
+			     &gpx1 7 3 0 0>;
+
+		key_1 {
+			keypad,row = <0>;
+			keypad,column = <3>;
+			keypad,key-code = <2>;
+		};
+
+		key_2 {
+			keypad,row = <0>;
+			keypad,column = <4>;
+			keypad,key-code = <3>;
+		};
+
+		key_3 {
+			keypad,row = <0>;
+			keypad,column = <5>;
+			keypad,key-code = <4>;
+		};
+	};
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index f689f49..cf01a56 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -21,6 +21,8 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/sched.h>
 #include <plat/keypad.h>
 
@@ -68,31 +70,26 @@ struct samsung_keypad {
 	wait_queue_head_t wait;
 	bool stopped;
 	int irq;
+	enum samsung_keypad_type type;
 	unsigned int row_shift;
 	unsigned int rows;
 	unsigned int cols;
 	unsigned int row_state[SAMSUNG_MAX_COLS];
+#ifdef CONFIG_OF
+	int row_gpios[SAMSUNG_MAX_ROWS];
+	int col_gpios[SAMSUNG_MAX_COLS];
+#endif
 	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 =
-		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)) {
+		if (keypad->type == KEYPAD_TYPE_S5PV210) {
 			val = S5PV210_KEYIFCOLEN_MASK;
 			val &= ~(1 << col) << 8;
 		} else {
@@ -235,6 +232,129 @@ static void samsung_keypad_close(struct input_dev *input_dev)
 	samsung_keypad_stop(keypad);
 }
 
+#ifdef CONFIG_OF
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+	struct samsung_keypad_platdata *pdata;
+	struct matrix_keymap_data *keymap_data;
+	uint32_t *keymap;
+	struct device_node *np = dev->of_node, *key_np;
+	unsigned int key_count = 0;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "could not allocate memory for platform data\n");
+		return NULL;
+	}
+
+	of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
+	of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
+	if (!pdata->rows || !pdata->cols) {
+		dev_err(dev, "number of keypad rows/columns not specified\n");
+		return NULL;
+	}
+
+	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
+	if (!keymap_data) {
+		dev_err(dev, "could not allocate memory for keymap data\n");
+		return NULL;
+	}
+	pdata->keymap_data = keymap_data;
+
+	for_each_child_of_node(np, key_np)
+		key_count++;
+
+	keymap_data->keymap_size = key_count;
+	keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
+	if (!keymap) {
+		dev_err(dev, "could not allocate memory for keymap\n");
+		return NULL;
+	}
+	keymap_data->keymap = keymap;
+
+	for_each_child_of_node(np, key_np) {
+		unsigned int row, col, key_code;
+		of_property_read_u32(key_np, "keypad,row", &row);
+		of_property_read_u32(key_np, "keypad,column", &col);
+		of_property_read_u32(key_np, "keypad,key-code", &key_code);
+		*keymap++ = KEY(row, col, key_code);
+	}
+
+	if (of_get_property(np, "linux,input-no-autorepeat", NULL))
+		pdata->no_autorepeat = true;
+	if (of_get_property(np, "linux,input-wakeup", NULL))
+		pdata->wakeup = true;
+
+	return pdata;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device *dev,
+				struct samsung_keypad *keypad)
+{
+	struct device_node *np = dev->of_node;
+	int gpio, ret, row, col;
+
+	for (row = 0; row < keypad->rows; row++) {
+		gpio = of_get_named_gpio(np, "row-gpios", row);
+		keypad->row_gpios[row] = gpio;
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
+					row, gpio);
+			continue;
+		}
+
+		ret = gpio_request(gpio, "keypad-row");
+		if (ret)
+			dev_err(dev, "keypad row[%d] gpio request failed\n",
+					row);
+	}
+
+	for (col = 0; col < keypad->cols; col++) {
+		gpio = of_get_named_gpio(np, "col-gpios", col);
+		keypad->col_gpios[col] = gpio;
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
+					col, gpio);
+			continue;
+		}
+
+		ret = gpio_request(col, "keypad-col");
+		if (ret)
+			dev_err(dev, "keypad column[%d] gpio request failed\n",
+					col);
+	}
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+	int cnt;
+
+	for (cnt = 0; cnt < keypad->rows; cnt++)
+		if (gpio_is_valid(keypad->row_gpios[cnt]))
+			gpio_free(keypad->row_gpios[cnt]);
+
+	for (cnt = 0; cnt < keypad->cols; cnt++)
+		if (gpio_is_valid(keypad->col_gpios[cnt]))
+			gpio_free(keypad->col_gpios[cnt]);
+}
+#else
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device *dev,
+				struct samsung_keypad *keypad)
+{
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+}
+#endif
+
 static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 {
 	const struct samsung_keypad_platdata *pdata;
@@ -246,7 +366,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 	unsigned int keymap_size;
 	int error;
 
-	pdata = pdev->dev.platform_data;
+	if (pdev->dev.of_node)
+		pdata = samsung_keypad_parse_dt(&pdev->dev);
+	else
+		pdata = pdev->dev.platform_data;
 	if (!pdata) {
 		dev_err(&pdev->dev, "no platform data defined\n");
 		return -EINVAL;
@@ -303,6 +426,16 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 	keypad->cols = pdata->cols;
 	init_waitqueue_head(&keypad->wait);
 
+	if (pdev->dev.of_node) {
+		samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
+#ifdef CONFIG_OF
+		keypad->type = of_device_is_compatible(pdev->dev.of_node,
+					"samsung,s5pv210-keypad");
+#endif
+	} else {
+		keypad->type = platform_get_device_id(pdev)->driver_data;
+	}
+
 	input_dev->name = pdev->name;
 	input_dev->id.bustype = BUS_HOST;
 	input_dev->dev.parent = &pdev->dev;
@@ -343,12 +476,19 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 	platform_set_drvdata(pdev, keypad);
+
+	if (pdev->dev.of_node) {
+		devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
+		devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
+		devm_kfree(&pdev->dev, (void *)pdata);
+	}
 	return 0;
 
 err_free_irq:
 	free_irq(keypad->irq, keypad);
 err_put_clk:
 	clk_put(keypad->clk);
+	samsung_keypad_dt_gpio_free(keypad);
 err_unmap_base:
 	iounmap(keypad->base);
 err_free_mem:
@@ -374,6 +514,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
 	free_irq(keypad->irq, keypad);
 
 	clk_put(keypad->clk);
+	samsung_keypad_dt_gpio_free(keypad);
 
 	iounmap(keypad->base);
 	kfree(keypad);
@@ -447,6 +588,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
 };
 #endif
 
+#ifdef CONFIG_OF
+static const struct of_device_id samsung_keypad_dt_match[] = {
+	{ .compatible = "samsung,s3c6410-keypad" },
+	{ .compatible = "samsung,s5pv210-keypad" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
+#else
+#define samsung_keypad_dt_match NULL
+#endif
+
 static struct platform_device_id samsung_keypad_driver_ids[] = {
 	{
 		.name		= "samsung-keypad",
@@ -465,6 +617,7 @@ static struct platform_driver samsung_keypad_driver = {
 	.driver		= {
 		.name	= "samsung-keypad",
 		.owner	= THIS_MODULE,
+		.of_match_table = samsung_keypad_dt_match,
 #ifdef CONFIG_PM
 		.pm	= &samsung_keypad_pm_ops,
 #endif
-- 
1.6.6.rc2

Re: [PATCH 2/2] input: samsung-keypad: Add device tree support

[Grant Likely]

On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
> Add device tree based discovery support for Samsung's keypad controller.
> 
> Cc: Joonyoung Shim <jy0922.shim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Donghwa Lee <dh09.lee-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Thomas Abraham <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
>  drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
>  2 files changed, 253 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
> new file mode 100644
> index 0000000..e1c7237
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
> @@ -0,0 +1,88 @@
> +* Samsung's Keypad Controller device tree bindings
> +
> +Samsung's Keypad controller is used to interface a SoC with a matrix-type
> +keypad device. The keypad controller supports multiple row and column lines.
> +A key can be placed at each intersection of a unique row and a unique column.
> +The keypad controller can sense a key-press and key-release and report the
> +event using a interrupt to the cpu.
> +
> +Required SoC Specific Properties:
> +- compatible: should be one of the following
> +  - "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
> +    controller.
> +  - "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
> +    controller.
> +
> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +
> +- interrupts: The interrupt number to the cpu.
> +
> +Required Board Specific Properties:
> +- samsung,keypad-num-rows: Number of row lines connected to the keypad
> +  controller.
> +
> +- samsung,keypad-num-columns: Number of column lines connected to the
> +  keypad controller.
> +
> +- row-gpios: List of gpios used as row lines. The gpio specifier for
> +  this property depends on the gpio controller to which these row lines
> +  are connected.
> +
> +- col-gpios: List of gpios used as column lines. The gpio specifier for
> +  this property depends on the gpio controller to which these column
> +  lines are connected.

I know we've got overlapping terminology here.  When you say GPIOs
here, does it refer to the pin multiplexing feature of the samsung
parts, and thus how the keypad controller IO is routed to the pins?
Or does the driver manipulate GPIO lines manually?

If it is pin multiplexing, then using the GPIO binding seems rather
odd. It may be entirely appropriate, but it will need to play well
with the pinmux stuff that linusw is working on.

> +
> +- Keys represented as child nodes: Each key connected to the keypad
> +  controller is represented as a child node to the keypad controller
> +  device node and should include the following properties.
> +  - keypad,row: the row number to which the key is connected.
> +  - keypad,column: the column number to which the key is connected.
> +  - keypad,key-code: the key-code to be reported when the key is pressed
> +    and released.

What defines the meanings of the key codes?

> +
> +Optional Properties specific to linux:
> +- linux,keypad-no-autorepeat: do no enable autorepeat feature.
> +- linux,keypad-wakeup: use any event on keypad as wakeup event.

Is this really a linux-specific feature?

> +
> +
> +Example:
> +	keypad@100A0000 {
> +		compatible = "samsung,s5pv210-keypad";
> +		reg = <0x100A0000 0x100>;
> +		interrupts = <173>;
> +		samsung,keypad-num-rows = <2>;
> +		samsung,keypad-num-columns = <8>;
> +		linux,input-no-autorepeat;
> +		linux,input-wakeup;
> +
> +		row-gpios = <&gpx2 0 3 3 0
> +			     &gpx2 1 3 3 0>;
> +
> +		col-gpios = <&gpx1 0 3 0 0
> +			     &gpx1 1 3 0 0
> +			     &gpx1 2 3 0 0
> +			     &gpx1 3 3 0 0
> +			     &gpx1 4 3 0 0
> +			     &gpx1 5 3 0 0
> +			     &gpx1 6 3 0 0
> +			     &gpx1 7 3 0 0>;
> +
> +		key_1 {
> +			keypad,row = <0>;
> +			keypad,column = <3>;
> +			keypad,key-code = <2>;
> +		};
> +
> +		key_2 {
> +			keypad,row = <0>;
> +			keypad,column = <4>;
> +			keypad,key-code = <3>;
> +		};
> +
> +		key_3 {
> +			keypad,row = <0>;
> +			keypad,column = <5>;
> +			keypad,key-code = <4>;
> +		};
> +	};
> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index f689f49..cf01a56 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c
> @@ -21,6 +21,8 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/sched.h>
>  #include <plat/keypad.h>
>  
> @@ -68,31 +70,26 @@ struct samsung_keypad {
>  	wait_queue_head_t wait;
>  	bool stopped;
>  	int irq;
> +	enum samsung_keypad_type type;
>  	unsigned int row_shift;
>  	unsigned int rows;
>  	unsigned int cols;
>  	unsigned int row_state[SAMSUNG_MAX_COLS];
> +#ifdef CONFIG_OF
> +	int row_gpios[SAMSUNG_MAX_ROWS];
> +	int col_gpios[SAMSUNG_MAX_COLS];
> +#endif
>  	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 =
> -		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)) {
> +		if (keypad->type == KEYPAD_TYPE_S5PV210) {
>  			val = S5PV210_KEYIFCOLEN_MASK;
>  			val &= ~(1 << col) << 8;
>  		} else {
> @@ -235,6 +232,129 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>  	samsung_keypad_stop(keypad);
>  }
>  
> +#ifdef CONFIG_OF
> +static
> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)

Nit: keep everything up to the function name on the same line.  It is
more grep-friendly to make line breaks in the parameters list.

> +{
> +	struct samsung_keypad_platdata *pdata;
> +	struct matrix_keymap_data *keymap_data;
> +	uint32_t *keymap;
> +	struct device_node *np = dev->of_node, *key_np;
> +	unsigned int key_count = 0;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		dev_err(dev, "could not allocate memory for platform data\n");
> +		return NULL;
> +	}
> +
> +	of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
> +	of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);

pdata->rows and ->cols needs to be changed to a u32 for this to be
safe.

> +	if (!pdata->rows || !pdata->cols) {
> +		dev_err(dev, "number of keypad rows/columns not specified\n");
> +		return NULL;
> +	}
> +
> +	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
> +	if (!keymap_data) {
> +		dev_err(dev, "could not allocate memory for keymap data\n");
> +		return NULL;
> +	}
> +	pdata->keymap_data = keymap_data;
> +
> +	for_each_child_of_node(np, key_np)
> +		key_count++;
> +
> +	keymap_data->keymap_size = key_count;
> +	keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
> +	if (!keymap) {
> +		dev_err(dev, "could not allocate memory for keymap\n");
> +		return NULL;
> +	}
> +	keymap_data->keymap = keymap;
> +
> +	for_each_child_of_node(np, key_np) {
> +		unsigned int row, col, key_code;

These need to be u32

> +		of_property_read_u32(key_np, "keypad,row", &row);
> +		of_property_read_u32(key_np, "keypad,column", &col);
> +		of_property_read_u32(key_np, "keypad,key-code", &key_code);
> +		*keymap++ = KEY(row, col, key_code);
> +	}
> +
> +	if (of_get_property(np, "linux,input-no-autorepeat", NULL))
> +		pdata->no_autorepeat = true;
> +	if (of_get_property(np, "linux,input-wakeup", NULL))
> +		pdata->wakeup = true;
> +
> +	return pdata;
> +}
> +
> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
> +				struct samsung_keypad *keypad)
> +{
> +	struct device_node *np = dev->of_node;
> +	int gpio, ret, row, col;
> +
> +	for (row = 0; row < keypad->rows; row++) {
> +		gpio = of_get_named_gpio(np, "row-gpios", row);
> +		keypad->row_gpios[row] = gpio;
> +		if (!gpio_is_valid(gpio)) {
> +			dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
> +					row, gpio);
> +			continue;
> +		}
> +
> +		ret = gpio_request(gpio, "keypad-row");
> +		if (ret)
> +			dev_err(dev, "keypad row[%d] gpio request failed\n",
> +					row);
> +	}
> +
> +	for (col = 0; col < keypad->cols; col++) {
> +		gpio = of_get_named_gpio(np, "col-gpios", col);
> +		keypad->col_gpios[col] = gpio;
> +		if (!gpio_is_valid(gpio)) {
> +			dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
> +					col, gpio);
> +			continue;
> +		}
> +
> +		ret = gpio_request(col, "keypad-col");
> +		if (ret)
> +			dev_err(dev, "keypad column[%d] gpio request failed\n",
> +					col);
> +	}
> +}
> +
> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
> +{
> +	int cnt;
> +
> +	for (cnt = 0; cnt < keypad->rows; cnt++)
> +		if (gpio_is_valid(keypad->row_gpios[cnt]))
> +			gpio_free(keypad->row_gpios[cnt]);
> +
> +	for (cnt = 0; cnt < keypad->cols; cnt++)
> +		if (gpio_is_valid(keypad->col_gpios[cnt]))
> +			gpio_free(keypad->col_gpios[cnt]);
> +}
> +#else
> +static
> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +
> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
> +				struct samsung_keypad *keypad)
> +{
> +}
> +
> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
> +{
> +}
> +#endif
> +
>  static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  {
>  	const struct samsung_keypad_platdata *pdata;
> @@ -246,7 +366,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  	unsigned int keymap_size;
>  	int error;
>  
> -	pdata = pdev->dev.platform_data;
> +	if (pdev->dev.of_node)
> +		pdata = samsung_keypad_parse_dt(&pdev->dev);
> +	else
> +		pdata = pdev->dev.platform_data;
>  	if (!pdata) {
>  		dev_err(&pdev->dev, "no platform data defined\n");
>  		return -EINVAL;
> @@ -303,6 +426,16 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  	keypad->cols = pdata->cols;
>  	init_waitqueue_head(&keypad->wait);
>  
> +	if (pdev->dev.of_node) {
> +		samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
> +#ifdef CONFIG_OF
> +		keypad->type = of_device_is_compatible(pdev->dev.of_node,
> +					"samsung,s5pv210-keypad");
> +#endif

It is odd having the #ifdef around only part of the if() block.

> +	} else {
> +		keypad->type = platform_get_device_id(pdev)->driver_data;
> +	}
> +
>  	input_dev->name = pdev->name;
>  	input_dev->id.bustype = BUS_HOST;
>  	input_dev->dev.parent = &pdev->dev;
> @@ -343,12 +476,19 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, pdata->wakeup);
>  	platform_set_drvdata(pdev, keypad);
> +
> +	if (pdev->dev.of_node) {
> +		devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
> +		devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
> +		devm_kfree(&pdev->dev, (void *)pdata);
> +	}

Don't need to do this.  The driver core will take care of freeing the
devm for you when removed.

A few things that should be fixed up, but otherwise looks pretty good.

>  	return 0;
>  
>  err_free_irq:
>  	free_irq(keypad->irq, keypad);
>  err_put_clk:
>  	clk_put(keypad->clk);
> +	samsung_keypad_dt_gpio_free(keypad);
>  err_unmap_base:
>  	iounmap(keypad->base);
>  err_free_mem:
> @@ -374,6 +514,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
>  	free_irq(keypad->irq, keypad);
>  
>  	clk_put(keypad->clk);
> +	samsung_keypad_dt_gpio_free(keypad);
>  
>  	iounmap(keypad->base);
>  	kfree(keypad);
> @@ -447,6 +588,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
>  };
>  #endif
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id samsung_keypad_dt_match[] = {
> +	{ .compatible = "samsung,s3c6410-keypad" },
> +	{ .compatible = "samsung,s5pv210-keypad" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
> +#else
> +#define samsung_keypad_dt_match NULL
> +#endif
> +
>  static struct platform_device_id samsung_keypad_driver_ids[] = {
>  	{
>  		.name		= "samsung-keypad",
> @@ -465,6 +617,7 @@ static struct platform_driver samsung_keypad_driver = {
>  	.driver		= {
>  		.name	= "samsung-keypad",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = samsung_keypad_dt_match,
>  #ifdef CONFIG_PM
>  		.pm	= &samsung_keypad_pm_ops,
>  #endif
> -- 
> 1.6.6.rc2
> 

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Grant Likely]

On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
> Add device tree based discovery support for Samsung's keypad controller.
> 
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Donghwa Lee <dh09.lee@samsung.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
>  drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
>  2 files changed, 253 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
> new file mode 100644
> index 0000000..e1c7237
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
> @@ -0,0 +1,88 @@
> +* Samsung's Keypad Controller device tree bindings
> +
> +Samsung's Keypad controller is used to interface a SoC with a matrix-type
> +keypad device. The keypad controller supports multiple row and column lines.
> +A key can be placed at each intersection of a unique row and a unique column.
> +The keypad controller can sense a key-press and key-release and report the
> +event using a interrupt to the cpu.
> +
> +Required SoC Specific Properties:
> +- compatible: should be one of the following
> +  - "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
> +    controller.
> +  - "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
> +    controller.
> +
> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +
> +- interrupts: The interrupt number to the cpu.
> +
> +Required Board Specific Properties:
> +- samsung,keypad-num-rows: Number of row lines connected to the keypad
> +  controller.
> +
> +- samsung,keypad-num-columns: Number of column lines connected to the
> +  keypad controller.
> +
> +- row-gpios: List of gpios used as row lines. The gpio specifier for
> +  this property depends on the gpio controller to which these row lines
> +  are connected.
> +
> +- col-gpios: List of gpios used as column lines. The gpio specifier for
> +  this property depends on the gpio controller to which these column
> +  lines are connected.

I know we've got overlapping terminology here.  When you say GPIOs
here, does it refer to the pin multiplexing feature of the samsung
parts, and thus how the keypad controller IO is routed to the pins?
Or does the driver manipulate GPIO lines manually?

If it is pin multiplexing, then using the GPIO binding seems rather
odd. It may be entirely appropriate, but it will need to play well
with the pinmux stuff that linusw is working on.

> +
> +- Keys represented as child nodes: Each key connected to the keypad
> +  controller is represented as a child node to the keypad controller
> +  device node and should include the following properties.
> +  - keypad,row: the row number to which the key is connected.
> +  - keypad,column: the column number to which the key is connected.
> +  - keypad,key-code: the key-code to be reported when the key is pressed
> +    and released.

What defines the meanings of the key codes?

> +
> +Optional Properties specific to linux:
> +- linux,keypad-no-autorepeat: do no enable autorepeat feature.
> +- linux,keypad-wakeup: use any event on keypad as wakeup event.

Is this really a linux-specific feature?

> +
> +
> +Example:
> +	keypad at 100A0000 {
> +		compatible = "samsung,s5pv210-keypad";
> +		reg = <0x100A0000 0x100>;
> +		interrupts = <173>;
> +		samsung,keypad-num-rows = <2>;
> +		samsung,keypad-num-columns = <8>;
> +		linux,input-no-autorepeat;
> +		linux,input-wakeup;
> +
> +		row-gpios = <&gpx2 0 3 3 0
> +			     &gpx2 1 3 3 0>;
> +
> +		col-gpios = <&gpx1 0 3 0 0
> +			     &gpx1 1 3 0 0
> +			     &gpx1 2 3 0 0
> +			     &gpx1 3 3 0 0
> +			     &gpx1 4 3 0 0
> +			     &gpx1 5 3 0 0
> +			     &gpx1 6 3 0 0
> +			     &gpx1 7 3 0 0>;
> +
> +		key_1 {
> +			keypad,row = <0>;
> +			keypad,column = <3>;
> +			keypad,key-code = <2>;
> +		};
> +
> +		key_2 {
> +			keypad,row = <0>;
> +			keypad,column = <4>;
> +			keypad,key-code = <3>;
> +		};
> +
> +		key_3 {
> +			keypad,row = <0>;
> +			keypad,column = <5>;
> +			keypad,key-code = <4>;
> +		};
> +	};
> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index f689f49..cf01a56 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c
> @@ -21,6 +21,8 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/sched.h>
>  #include <plat/keypad.h>
>  
> @@ -68,31 +70,26 @@ struct samsung_keypad {
>  	wait_queue_head_t wait;
>  	bool stopped;
>  	int irq;
> +	enum samsung_keypad_type type;
>  	unsigned int row_shift;
>  	unsigned int rows;
>  	unsigned int cols;
>  	unsigned int row_state[SAMSUNG_MAX_COLS];
> +#ifdef CONFIG_OF
> +	int row_gpios[SAMSUNG_MAX_ROWS];
> +	int col_gpios[SAMSUNG_MAX_COLS];
> +#endif
>  	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 =
> -		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)) {
> +		if (keypad->type == KEYPAD_TYPE_S5PV210) {
>  			val = S5PV210_KEYIFCOLEN_MASK;
>  			val &= ~(1 << col) << 8;
>  		} else {
> @@ -235,6 +232,129 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>  	samsung_keypad_stop(keypad);
>  }
>  
> +#ifdef CONFIG_OF
> +static
> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)

Nit: keep everything up to the function name on the same line.  It is
more grep-friendly to make line breaks in the parameters list.

> +{
> +	struct samsung_keypad_platdata *pdata;
> +	struct matrix_keymap_data *keymap_data;
> +	uint32_t *keymap;
> +	struct device_node *np = dev->of_node, *key_np;
> +	unsigned int key_count = 0;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		dev_err(dev, "could not allocate memory for platform data\n");
> +		return NULL;
> +	}
> +
> +	of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
> +	of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);

pdata->rows and ->cols needs to be changed to a u32 for this to be
safe.

> +	if (!pdata->rows || !pdata->cols) {
> +		dev_err(dev, "number of keypad rows/columns not specified\n");
> +		return NULL;
> +	}
> +
> +	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
> +	if (!keymap_data) {
> +		dev_err(dev, "could not allocate memory for keymap data\n");
> +		return NULL;
> +	}
> +	pdata->keymap_data = keymap_data;
> +
> +	for_each_child_of_node(np, key_np)
> +		key_count++;
> +
> +	keymap_data->keymap_size = key_count;
> +	keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
> +	if (!keymap) {
> +		dev_err(dev, "could not allocate memory for keymap\n");
> +		return NULL;
> +	}
> +	keymap_data->keymap = keymap;
> +
> +	for_each_child_of_node(np, key_np) {
> +		unsigned int row, col, key_code;

These need to be u32

> +		of_property_read_u32(key_np, "keypad,row", &row);
> +		of_property_read_u32(key_np, "keypad,column", &col);
> +		of_property_read_u32(key_np, "keypad,key-code", &key_code);
> +		*keymap++ = KEY(row, col, key_code);
> +	}
> +
> +	if (of_get_property(np, "linux,input-no-autorepeat", NULL))
> +		pdata->no_autorepeat = true;
> +	if (of_get_property(np, "linux,input-wakeup", NULL))
> +		pdata->wakeup = true;
> +
> +	return pdata;
> +}
> +
> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
> +				struct samsung_keypad *keypad)
> +{
> +	struct device_node *np = dev->of_node;
> +	int gpio, ret, row, col;
> +
> +	for (row = 0; row < keypad->rows; row++) {
> +		gpio = of_get_named_gpio(np, "row-gpios", row);
> +		keypad->row_gpios[row] = gpio;
> +		if (!gpio_is_valid(gpio)) {
> +			dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
> +					row, gpio);
> +			continue;
> +		}
> +
> +		ret = gpio_request(gpio, "keypad-row");
> +		if (ret)
> +			dev_err(dev, "keypad row[%d] gpio request failed\n",
> +					row);
> +	}
> +
> +	for (col = 0; col < keypad->cols; col++) {
> +		gpio = of_get_named_gpio(np, "col-gpios", col);
> +		keypad->col_gpios[col] = gpio;
> +		if (!gpio_is_valid(gpio)) {
> +			dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
> +					col, gpio);
> +			continue;
> +		}
> +
> +		ret = gpio_request(col, "keypad-col");
> +		if (ret)
> +			dev_err(dev, "keypad column[%d] gpio request failed\n",
> +					col);
> +	}
> +}
> +
> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
> +{
> +	int cnt;
> +
> +	for (cnt = 0; cnt < keypad->rows; cnt++)
> +		if (gpio_is_valid(keypad->row_gpios[cnt]))
> +			gpio_free(keypad->row_gpios[cnt]);
> +
> +	for (cnt = 0; cnt < keypad->cols; cnt++)
> +		if (gpio_is_valid(keypad->col_gpios[cnt]))
> +			gpio_free(keypad->col_gpios[cnt]);
> +}
> +#else
> +static
> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +
> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
> +				struct samsung_keypad *keypad)
> +{
> +}
> +
> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
> +{
> +}
> +#endif
> +
>  static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  {
>  	const struct samsung_keypad_platdata *pdata;
> @@ -246,7 +366,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  	unsigned int keymap_size;
>  	int error;
>  
> -	pdata = pdev->dev.platform_data;
> +	if (pdev->dev.of_node)
> +		pdata = samsung_keypad_parse_dt(&pdev->dev);
> +	else
> +		pdata = pdev->dev.platform_data;
>  	if (!pdata) {
>  		dev_err(&pdev->dev, "no platform data defined\n");
>  		return -EINVAL;
> @@ -303,6 +426,16 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  	keypad->cols = pdata->cols;
>  	init_waitqueue_head(&keypad->wait);
>  
> +	if (pdev->dev.of_node) {
> +		samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
> +#ifdef CONFIG_OF
> +		keypad->type = of_device_is_compatible(pdev->dev.of_node,
> +					"samsung,s5pv210-keypad");
> +#endif

It is odd having the #ifdef around only part of the if() block.

> +	} else {
> +		keypad->type = platform_get_device_id(pdev)->driver_data;
> +	}
> +
>  	input_dev->name = pdev->name;
>  	input_dev->id.bustype = BUS_HOST;
>  	input_dev->dev.parent = &pdev->dev;
> @@ -343,12 +476,19 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, pdata->wakeup);
>  	platform_set_drvdata(pdev, keypad);
> +
> +	if (pdev->dev.of_node) {
> +		devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
> +		devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
> +		devm_kfree(&pdev->dev, (void *)pdata);
> +	}

Don't need to do this.  The driver core will take care of freeing the
devm for you when removed.

A few things that should be fixed up, but otherwise looks pretty good.

>  	return 0;
>  
>  err_free_irq:
>  	free_irq(keypad->irq, keypad);
>  err_put_clk:
>  	clk_put(keypad->clk);
> +	samsung_keypad_dt_gpio_free(keypad);
>  err_unmap_base:
>  	iounmap(keypad->base);
>  err_free_mem:
> @@ -374,6 +514,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
>  	free_irq(keypad->irq, keypad);
>  
>  	clk_put(keypad->clk);
> +	samsung_keypad_dt_gpio_free(keypad);
>  
>  	iounmap(keypad->base);
>  	kfree(keypad);
> @@ -447,6 +588,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
>  };
>  #endif
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id samsung_keypad_dt_match[] = {
> +	{ .compatible = "samsung,s3c6410-keypad" },
> +	{ .compatible = "samsung,s5pv210-keypad" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
> +#else
> +#define samsung_keypad_dt_match NULL
> +#endif
> +
>  static struct platform_device_id samsung_keypad_driver_ids[] = {
>  	{
>  		.name		= "samsung-keypad",
> @@ -465,6 +617,7 @@ static struct platform_driver samsung_keypad_driver = {
>  	.driver		= {
>  		.name	= "samsung-keypad",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = samsung_keypad_dt_match,
>  #ifdef CONFIG_PM
>  		.pm	= &samsung_keypad_pm_ops,
>  #endif
> -- 
> 1.6.6.rc2
> 

Re: [PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Hi Grant,

On 14 September 2011 21:41, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
>> Add device tree based discovery support for Samsung's keypad controller.
>>
>> Cc: Joonyoung Shim <jy0922.shim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Cc: Donghwa Lee <dh09.lee-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Thomas Abraham <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
>>  drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
>>  2 files changed, 253 insertions(+), 12 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
>> new file mode 100644
>> index 0000000..e1c7237
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
>> @@ -0,0 +1,88 @@
>> +* Samsung's Keypad Controller device tree bindings
>> +
>> +Samsung's Keypad controller is used to interface a SoC with a matrix-type
>> +keypad device. The keypad controller supports multiple row and column lines.
>> +A key can be placed at each intersection of a unique row and a unique column.
>> +The keypad controller can sense a key-press and key-release and report the
>> +event using a interrupt to the cpu.
>> +
>> +Required SoC Specific Properties:
>> +- compatible: should be one of the following
>> +  - "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
>> +    controller.
>> +  - "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
>> +    controller.
>> +
>> +- reg: physical base address of the controller and length of memory mapped
>> +  region.
>> +
>> +- interrupts: The interrupt number to the cpu.
>> +
>> +Required Board Specific Properties:
>> +- samsung,keypad-num-rows: Number of row lines connected to the keypad
>> +  controller.
>> +
>> +- samsung,keypad-num-columns: Number of column lines connected to the
>> +  keypad controller.
>> +
>> +- row-gpios: List of gpios used as row lines. The gpio specifier for
>> +  this property depends on the gpio controller to which these row lines
>> +  are connected.
>> +
>> +- col-gpios: List of gpios used as column lines. The gpio specifier for
>> +  this property depends on the gpio controller to which these column
>> +  lines are connected.
>
> I know we've got overlapping terminology here.  When you say GPIOs
> here, does it refer to the pin multiplexing feature of the samsung
> parts, and thus how the keypad controller IO is routed to the pins?
> Or does the driver manipulate GPIO lines manually?

It is intended to mean gpio and not the pinmux. But the way the gpio
specifier is specified in the dts file, it includes information about
both gpio number and the pin-control details. For instance, if 'gpa0'
is a gpio controller (which includes pin-control functionality as well
in the hardware), then the gpio specifier for the keypad would be as
below.

row-gpios = <&gpa0 0 3 3 0>,
                 <&gpa0 1 3 3 0>;

The syntax for the gpio specifier is
<[phandle of gpio controller] [pin number within the gpio controller]
[mux function] [pull up/down] [drive strength]>

The keypad driver does not know or use the mux function, pull up/down
and drive strength details. The driver would call of_get_gpio to get
the linux gpio number and then do a gpio_request on that gpio number.
The gpio dt support manges the pin-mux and other settings
transparently for the keypad driver. So even though the gpio specifier
format changes, the dt support code for the driver does not change.

The driver does not manipulate the gpios directly. It just requests
for the gpio number and makes a note of the number to free it when the
driver unbinds.

>
> If it is pin multiplexing, then using the GPIO binding seems rather
> odd. It may be entirely appropriate, but it will need to play well
> with the pinmux stuff that linusw is working on.

When pinmux/pin-ctrl functionality which linusw is developing is used
for this driver, it would be a extension to this patch. The driver
would request for the gpio and then use the pin-ctrl subsystem api to
setup the pin-control. In that case, the gpio-specifier would change
but that change would be break anything which this patch does.

>
>> +
>> +- Keys represented as child nodes: Each key connected to the keypad
>> +  controller is represented as a child node to the keypad controller
>> +  device node and should include the following properties.
>> +  - keypad,row: the row number to which the key is connected.
>> +  - keypad,column: the column number to which the key is connected.
>> +  - keypad,key-code: the key-code to be reported when the key is pressed
>> +    and released.
>
> What defines the meanings of the key codes?

The key-code could be any value which the system would want the keypad
driver to report when that key is pressed.

>
>> +
>> +Optional Properties specific to linux:
>> +- linux,keypad-no-autorepeat: do no enable autorepeat feature.
>> +- linux,keypad-wakeup: use any event on keypad as wakeup event.
>
> Is this really a linux-specific feature?

Yes, it is a property defined by the linux subsystems. A non-linux
system might not have such features.

>
>> +
>> +
>> +Example:
>> +     keypad@100A0000 {
>> +             compatible = "samsung,s5pv210-keypad";
>> +             reg = <0x100A0000 0x100>;
>> +             interrupts = <173>;
>> +             samsung,keypad-num-rows = <2>;
>> +             samsung,keypad-num-columns = <8>;
>> +             linux,input-no-autorepeat;
>> +             linux,input-wakeup;
>> +
>> +             row-gpios = <&gpx2 0 3 3 0
>> +                          &gpx2 1 3 3 0>;
>> +
>> +             col-gpios = <&gpx1 0 3 0 0
>> +                          &gpx1 1 3 0 0
>> +                          &gpx1 2 3 0 0
>> +                          &gpx1 3 3 0 0
>> +                          &gpx1 4 3 0 0
>> +                          &gpx1 5 3 0 0
>> +                          &gpx1 6 3 0 0
>> +                          &gpx1 7 3 0 0>;
>> +
>> +             key_1 {
>> +                     keypad,row = <0>;
>> +                     keypad,column = <3>;
>> +                     keypad,key-code = <2>;
>> +             };
>> +
>> +             key_2 {
>> +                     keypad,row = <0>;
>> +                     keypad,column = <4>;
>> +                     keypad,key-code = <3>;
>> +             };
>> +
>> +             key_3 {
>> +                     keypad,row = <0>;
>> +                     keypad,column = <5>;
>> +                     keypad,key-code = <4>;
>> +             };
>> +     };
>> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
>> index f689f49..cf01a56 100644
>> --- a/drivers/input/keyboard/samsung-keypad.c
>> +++ b/drivers/input/keyboard/samsung-keypad.c
>> @@ -21,6 +21,8 @@
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>>  #include <linux/sched.h>
>>  #include <plat/keypad.h>
>>
>> @@ -68,31 +70,26 @@ struct samsung_keypad {
>>       wait_queue_head_t wait;
>>       bool stopped;
>>       int irq;
>> +     enum samsung_keypad_type type;
>>       unsigned int row_shift;
>>       unsigned int rows;
>>       unsigned int cols;
>>       unsigned int row_state[SAMSUNG_MAX_COLS];
>> +#ifdef CONFIG_OF
>> +     int row_gpios[SAMSUNG_MAX_ROWS];
>> +     int col_gpios[SAMSUNG_MAX_COLS];
>> +#endif
>>       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 =
>> -             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)) {
>> +             if (keypad->type == KEYPAD_TYPE_S5PV210) {
>>                       val = S5PV210_KEYIFCOLEN_MASK;
>>                       val &= ~(1 << col) << 8;
>>               } else {
>> @@ -235,6 +232,129 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>>       samsung_keypad_stop(keypad);
>>  }
>>
>> +#ifdef CONFIG_OF
>> +static
>> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
>
> Nit: keep everything up to the function name on the same line.  It is
> more grep-friendly to make line breaks in the parameters list.

Ok. I will change this.

>
>> +{
>> +     struct samsung_keypad_platdata *pdata;
>> +     struct matrix_keymap_data *keymap_data;
>> +     uint32_t *keymap;
>> +     struct device_node *np = dev->of_node, *key_np;
>> +     unsigned int key_count = 0;
>> +
>> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +     if (!pdata) {
>> +             dev_err(dev, "could not allocate memory for platform data\n");
>> +             return NULL;
>> +     }
>> +
>> +     of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
>> +     of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
>
> pdata->rows and ->cols needs to be changed to a u32 for this to be
> safe.

Ok.

>
>> +     if (!pdata->rows || !pdata->cols) {
>> +             dev_err(dev, "number of keypad rows/columns not specified\n");
>> +             return NULL;
>> +     }
>> +
>> +     keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
>> +     if (!keymap_data) {
>> +             dev_err(dev, "could not allocate memory for keymap data\n");
>> +             return NULL;
>> +     }
>> +     pdata->keymap_data = keymap_data;
>> +
>> +     for_each_child_of_node(np, key_np)
>> +             key_count++;
>> +
>> +     keymap_data->keymap_size = key_count;
>> +     keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
>> +     if (!keymap) {
>> +             dev_err(dev, "could not allocate memory for keymap\n");
>> +             return NULL;
>> +     }
>> +     keymap_data->keymap = keymap;
>> +
>> +     for_each_child_of_node(np, key_np) {
>> +             unsigned int row, col, key_code;
>
> These need to be u32

Ok.

>
>> +             of_property_read_u32(key_np, "keypad,row", &row);
>> +             of_property_read_u32(key_np, "keypad,column", &col);
>> +             of_property_read_u32(key_np, "keypad,key-code", &key_code);
>> +             *keymap++ = KEY(row, col, key_code);
>> +     }
>> +
>> +     if (of_get_property(np, "linux,input-no-autorepeat", NULL))
>> +             pdata->no_autorepeat = true;
>> +     if (of_get_property(np, "linux,input-wakeup", NULL))
>> +             pdata->wakeup = true;
>> +
>> +     return pdata;
>> +}
>> +
>> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
>> +                             struct samsung_keypad *keypad)
>> +{
>> +     struct device_node *np = dev->of_node;
>> +     int gpio, ret, row, col;
>> +
>> +     for (row = 0; row < keypad->rows; row++) {
>> +             gpio = of_get_named_gpio(np, "row-gpios", row);
>> +             keypad->row_gpios[row] = gpio;
>> +             if (!gpio_is_valid(gpio)) {
>> +                     dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
>> +                                     row, gpio);
>> +                     continue;
>> +             }
>> +
>> +             ret = gpio_request(gpio, "keypad-row");
>> +             if (ret)
>> +                     dev_err(dev, "keypad row[%d] gpio request failed\n",
>> +                                     row);
>> +     }
>> +
>> +     for (col = 0; col < keypad->cols; col++) {
>> +             gpio = of_get_named_gpio(np, "col-gpios", col);
>> +             keypad->col_gpios[col] = gpio;
>> +             if (!gpio_is_valid(gpio)) {
>> +                     dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
>> +                                     col, gpio);
>> +                     continue;
>> +             }
>> +
>> +             ret = gpio_request(col, "keypad-col");
>> +             if (ret)
>> +                     dev_err(dev, "keypad column[%d] gpio request failed\n",
>> +                                     col);
>> +     }
>> +}
>> +
>> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
>> +{
>> +     int cnt;
>> +
>> +     for (cnt = 0; cnt < keypad->rows; cnt++)
>> +             if (gpio_is_valid(keypad->row_gpios[cnt]))
>> +                     gpio_free(keypad->row_gpios[cnt]);
>> +
>> +     for (cnt = 0; cnt < keypad->cols; cnt++)
>> +             if (gpio_is_valid(keypad->col_gpios[cnt]))
>> +                     gpio_free(keypad->col_gpios[cnt]);
>> +}
>> +#else
>> +static
>> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
>> +{
>> +     return NULL;
>> +}
>> +
>> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
>> +                             struct samsung_keypad *keypad)
>> +{
>> +}
>> +
>> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
>> +{
>> +}
>> +#endif
>> +
>>  static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>>  {
>>       const struct samsung_keypad_platdata *pdata;
>> @@ -246,7 +366,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>>       unsigned int keymap_size;
>>       int error;
>>
>> -     pdata = pdev->dev.platform_data;
>> +     if (pdev->dev.of_node)
>> +             pdata = samsung_keypad_parse_dt(&pdev->dev);
>> +     else
>> +             pdata = pdev->dev.platform_data;
>>       if (!pdata) {
>>               dev_err(&pdev->dev, "no platform data defined\n");
>>               return -EINVAL;
>> @@ -303,6 +426,16 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>>       keypad->cols = pdata->cols;
>>       init_waitqueue_head(&keypad->wait);
>>
>> +     if (pdev->dev.of_node) {
>> +             samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
>> +#ifdef CONFIG_OF
>> +             keypad->type = of_device_is_compatible(pdev->dev.of_node,
>> +                                     "samsung,s5pv210-keypad");
>> +#endif
>
> It is odd having the #ifdef around only part of the if() block.

I did not want to do it this way. But any other way would have
increased the number of lines added. If is against the coding rules or
style, I will change it.

>
>> +     } else {
>> +             keypad->type = platform_get_device_id(pdev)->driver_data;
>> +     }
>> +
>>       input_dev->name = pdev->name;
>>       input_dev->id.bustype = BUS_HOST;
>>       input_dev->dev.parent = &pdev->dev;
>> @@ -343,12 +476,19 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>>
>>       device_init_wakeup(&pdev->dev, pdata->wakeup);
>>       platform_set_drvdata(pdev, keypad);
>> +
>> +     if (pdev->dev.of_node) {
>> +             devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
>> +             devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
>> +             devm_kfree(&pdev->dev, (void *)pdata);
>> +     }
>
> Don't need to do this.  The driver core will take care of freeing the
> devm for you when removed.

Dmitry Torokhov suggested that since the allocated pdata is not used
after the probe, it can be deallocated to save some memory. So this
was added.

>
> A few things that should be fixed up, but otherwise looks pretty good.
>

Thanks for your review and suggestions.

Regards,
Thomas.

[...]

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Hi Grant,

On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
>> Add device tree based discovery support for Samsung's keypad controller.
>>
>> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
>> Cc: Donghwa Lee <dh09.lee@samsung.com>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>> ?.../devicetree/bindings/input/samsung-keypad.txt ? | ? 88 ++++++++++
>> ?drivers/input/keyboard/samsung-keypad.c ? ? ? ? ? ?| ?177 ++++++++++++++++++--
>> ?2 files changed, 253 insertions(+), 12 deletions(-)
>> ?create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
>> new file mode 100644
>> index 0000000..e1c7237
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
>> @@ -0,0 +1,88 @@
>> +* Samsung's Keypad Controller device tree bindings
>> +
>> +Samsung's Keypad controller is used to interface a SoC with a matrix-type
>> +keypad device. The keypad controller supports multiple row and column lines.
>> +A key can be placed at each intersection of a unique row and a unique column.
>> +The keypad controller can sense a key-press and key-release and report the
>> +event using a interrupt to the cpu.
>> +
>> +Required SoC Specific Properties:
>> +- compatible: should be one of the following
>> + ?- "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
>> + ? ?controller.
>> + ?- "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
>> + ? ?controller.
>> +
>> +- reg: physical base address of the controller and length of memory mapped
>> + ?region.
>> +
>> +- interrupts: The interrupt number to the cpu.
>> +
>> +Required Board Specific Properties:
>> +- samsung,keypad-num-rows: Number of row lines connected to the keypad
>> + ?controller.
>> +
>> +- samsung,keypad-num-columns: Number of column lines connected to the
>> + ?keypad controller.
>> +
>> +- row-gpios: List of gpios used as row lines. The gpio specifier for
>> + ?this property depends on the gpio controller to which these row lines
>> + ?are connected.
>> +
>> +- col-gpios: List of gpios used as column lines. The gpio specifier for
>> + ?this property depends on the gpio controller to which these column
>> + ?lines are connected.
>
> I know we've got overlapping terminology here. ?When you say GPIOs
> here, does it refer to the pin multiplexing feature of the samsung
> parts, and thus how the keypad controller IO is routed to the pins?
> Or does the driver manipulate GPIO lines manually?

It is intended to mean gpio and not the pinmux. But the way the gpio
specifier is specified in the dts file, it includes information about
both gpio number and the pin-control details. For instance, if 'gpa0'
is a gpio controller (which includes pin-control functionality as well
in the hardware), then the gpio specifier for the keypad would be as
below.

row-gpios = <&gpa0 0 3 3 0>,
                 <&gpa0 1 3 3 0>;

The syntax for the gpio specifier is
<[phandle of gpio controller] [pin number within the gpio controller]
[mux function] [pull up/down] [drive strength]>

The keypad driver does not know or use the mux function, pull up/down
and drive strength details. The driver would call of_get_gpio to get
the linux gpio number and then do a gpio_request on that gpio number.
The gpio dt support manges the pin-mux and other settings
transparently for the keypad driver. So even though the gpio specifier
format changes, the dt support code for the driver does not change.

The driver does not manipulate the gpios directly. It just requests
for the gpio number and makes a note of the number to free it when the
driver unbinds.

>
> If it is pin multiplexing, then using the GPIO binding seems rather
> odd. It may be entirely appropriate, but it will need to play well
> with the pinmux stuff that linusw is working on.

When pinmux/pin-ctrl functionality which linusw is developing is used
for this driver, it would be a extension to this patch. The driver
would request for the gpio and then use the pin-ctrl subsystem api to
setup the pin-control. In that case, the gpio-specifier would change
but that change would be break anything which this patch does.

>
>> +
>> +- Keys represented as child nodes: Each key connected to the keypad
>> + ?controller is represented as a child node to the keypad controller
>> + ?device node and should include the following properties.
>> + ?- keypad,row: the row number to which the key is connected.
>> + ?- keypad,column: the column number to which the key is connected.
>> + ?- keypad,key-code: the key-code to be reported when the key is pressed
>> + ? ?and released.
>
> What defines the meanings of the key codes?

The key-code could be any value which the system would want the keypad
driver to report when that key is pressed.

>
>> +
>> +Optional Properties specific to linux:
>> +- linux,keypad-no-autorepeat: do no enable autorepeat feature.
>> +- linux,keypad-wakeup: use any event on keypad as wakeup event.
>
> Is this really a linux-specific feature?

Yes, it is a property defined by the linux subsystems. A non-linux
system might not have such features.

>
>> +
>> +
>> +Example:
>> + ? ? keypad at 100A0000 {
>> + ? ? ? ? ? ? compatible = "samsung,s5pv210-keypad";
>> + ? ? ? ? ? ? reg = <0x100A0000 0x100>;
>> + ? ? ? ? ? ? interrupts = <173>;
>> + ? ? ? ? ? ? samsung,keypad-num-rows = <2>;
>> + ? ? ? ? ? ? samsung,keypad-num-columns = <8>;
>> + ? ? ? ? ? ? linux,input-no-autorepeat;
>> + ? ? ? ? ? ? linux,input-wakeup;
>> +
>> + ? ? ? ? ? ? row-gpios = <&gpx2 0 3 3 0
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?&gpx2 1 3 3 0>;
>> +
>> + ? ? ? ? ? ? col-gpios = <&gpx1 0 3 0 0
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?&gpx1 1 3 0 0
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?&gpx1 2 3 0 0
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?&gpx1 3 3 0 0
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?&gpx1 4 3 0 0
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?&gpx1 5 3 0 0
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?&gpx1 6 3 0 0
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?&gpx1 7 3 0 0>;
>> +
>> + ? ? ? ? ? ? key_1 {
>> + ? ? ? ? ? ? ? ? ? ? keypad,row = <0>;
>> + ? ? ? ? ? ? ? ? ? ? keypad,column = <3>;
>> + ? ? ? ? ? ? ? ? ? ? keypad,key-code = <2>;
>> + ? ? ? ? ? ? };
>> +
>> + ? ? ? ? ? ? key_2 {
>> + ? ? ? ? ? ? ? ? ? ? keypad,row = <0>;
>> + ? ? ? ? ? ? ? ? ? ? keypad,column = <4>;
>> + ? ? ? ? ? ? ? ? ? ? keypad,key-code = <3>;
>> + ? ? ? ? ? ? };
>> +
>> + ? ? ? ? ? ? key_3 {
>> + ? ? ? ? ? ? ? ? ? ? keypad,row = <0>;
>> + ? ? ? ? ? ? ? ? ? ? keypad,column = <5>;
>> + ? ? ? ? ? ? ? ? ? ? keypad,key-code = <4>;
>> + ? ? ? ? ? ? };
>> + ? ? };
>> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
>> index f689f49..cf01a56 100644
>> --- a/drivers/input/keyboard/samsung-keypad.c
>> +++ b/drivers/input/keyboard/samsung-keypad.c
>> @@ -21,6 +21,8 @@
>> ?#include <linux/module.h>
>> ?#include <linux/platform_device.h>
>> ?#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> ?#include <linux/sched.h>
>> ?#include <plat/keypad.h>
>>
>> @@ -68,31 +70,26 @@ struct samsung_keypad {
>> ? ? ? wait_queue_head_t wait;
>> ? ? ? bool stopped;
>> ? ? ? int irq;
>> + ? ? enum samsung_keypad_type type;
>> ? ? ? unsigned int row_shift;
>> ? ? ? unsigned int rows;
>> ? ? ? unsigned int cols;
>> ? ? ? unsigned int row_state[SAMSUNG_MAX_COLS];
>> +#ifdef CONFIG_OF
>> + ? ? int row_gpios[SAMSUNG_MAX_ROWS];
>> + ? ? int col_gpios[SAMSUNG_MAX_COLS];
>> +#endif
>> ? ? ? 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 =
>> - ? ? ? ? ? ? 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)) {
>> + ? ? ? ? ? ? if (keypad->type == KEYPAD_TYPE_S5PV210) {
>> ? ? ? ? ? ? ? ? ? ? ? val = S5PV210_KEYIFCOLEN_MASK;
>> ? ? ? ? ? ? ? ? ? ? ? val &= ~(1 << col) << 8;
>> ? ? ? ? ? ? ? } else {
>> @@ -235,6 +232,129 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>> ? ? ? samsung_keypad_stop(keypad);
>> ?}
>>
>> +#ifdef CONFIG_OF
>> +static
>> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
>
> Nit: keep everything up to the function name on the same line. ?It is
> more grep-friendly to make line breaks in the parameters list.

Ok. I will change this.

>
>> +{
>> + ? ? struct samsung_keypad_platdata *pdata;
>> + ? ? struct matrix_keymap_data *keymap_data;
>> + ? ? uint32_t *keymap;
>> + ? ? struct device_node *np = dev->of_node, *key_np;
>> + ? ? unsigned int key_count = 0;
>> +
>> + ? ? pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + ? ? if (!pdata) {
>> + ? ? ? ? ? ? dev_err(dev, "could not allocate memory for platform data\n");
>> + ? ? ? ? ? ? return NULL;
>> + ? ? }
>> +
>> + ? ? of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
>> + ? ? of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
>
> pdata->rows and ->cols needs to be changed to a u32 for this to be
> safe.

Ok.

>
>> + ? ? if (!pdata->rows || !pdata->cols) {
>> + ? ? ? ? ? ? dev_err(dev, "number of keypad rows/columns not specified\n");
>> + ? ? ? ? ? ? return NULL;
>> + ? ? }
>> +
>> + ? ? keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
>> + ? ? if (!keymap_data) {
>> + ? ? ? ? ? ? dev_err(dev, "could not allocate memory for keymap data\n");
>> + ? ? ? ? ? ? return NULL;
>> + ? ? }
>> + ? ? pdata->keymap_data = keymap_data;
>> +
>> + ? ? for_each_child_of_node(np, key_np)
>> + ? ? ? ? ? ? key_count++;
>> +
>> + ? ? keymap_data->keymap_size = key_count;
>> + ? ? keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
>> + ? ? if (!keymap) {
>> + ? ? ? ? ? ? dev_err(dev, "could not allocate memory for keymap\n");
>> + ? ? ? ? ? ? return NULL;
>> + ? ? }
>> + ? ? keymap_data->keymap = keymap;
>> +
>> + ? ? for_each_child_of_node(np, key_np) {
>> + ? ? ? ? ? ? unsigned int row, col, key_code;
>
> These need to be u32

Ok.

>
>> + ? ? ? ? ? ? of_property_read_u32(key_np, "keypad,row", &row);
>> + ? ? ? ? ? ? of_property_read_u32(key_np, "keypad,column", &col);
>> + ? ? ? ? ? ? of_property_read_u32(key_np, "keypad,key-code", &key_code);
>> + ? ? ? ? ? ? *keymap++ = KEY(row, col, key_code);
>> + ? ? }
>> +
>> + ? ? if (of_get_property(np, "linux,input-no-autorepeat", NULL))
>> + ? ? ? ? ? ? pdata->no_autorepeat = true;
>> + ? ? if (of_get_property(np, "linux,input-wakeup", NULL))
>> + ? ? ? ? ? ? pdata->wakeup = true;
>> +
>> + ? ? return pdata;
>> +}
>> +
>> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct samsung_keypad *keypad)
>> +{
>> + ? ? struct device_node *np = dev->of_node;
>> + ? ? int gpio, ret, row, col;
>> +
>> + ? ? for (row = 0; row < keypad->rows; row++) {
>> + ? ? ? ? ? ? gpio = of_get_named_gpio(np, "row-gpios", row);
>> + ? ? ? ? ? ? keypad->row_gpios[row] = gpio;
>> + ? ? ? ? ? ? if (!gpio_is_valid(gpio)) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? row, gpio);
>> + ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ret = gpio_request(gpio, "keypad-row");
>> + ? ? ? ? ? ? if (ret)
>> + ? ? ? ? ? ? ? ? ? ? dev_err(dev, "keypad row[%d] gpio request failed\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? row);
>> + ? ? }
>> +
>> + ? ? for (col = 0; col < keypad->cols; col++) {
>> + ? ? ? ? ? ? gpio = of_get_named_gpio(np, "col-gpios", col);
>> + ? ? ? ? ? ? keypad->col_gpios[col] = gpio;
>> + ? ? ? ? ? ? if (!gpio_is_valid(gpio)) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? col, gpio);
>> + ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ret = gpio_request(col, "keypad-col");
>> + ? ? ? ? ? ? if (ret)
>> + ? ? ? ? ? ? ? ? ? ? dev_err(dev, "keypad column[%d] gpio request failed\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? col);
>> + ? ? }
>> +}
>> +
>> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
>> +{
>> + ? ? int cnt;
>> +
>> + ? ? for (cnt = 0; cnt < keypad->rows; cnt++)
>> + ? ? ? ? ? ? if (gpio_is_valid(keypad->row_gpios[cnt]))
>> + ? ? ? ? ? ? ? ? ? ? gpio_free(keypad->row_gpios[cnt]);
>> +
>> + ? ? for (cnt = 0; cnt < keypad->cols; cnt++)
>> + ? ? ? ? ? ? if (gpio_is_valid(keypad->col_gpios[cnt]))
>> + ? ? ? ? ? ? ? ? ? ? gpio_free(keypad->col_gpios[cnt]);
>> +}
>> +#else
>> +static
>> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
>> +{
>> + ? ? return NULL;
>> +}
>> +
>> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct samsung_keypad *keypad)
>> +{
>> +}
>> +
>> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
>> +{
>> +}
>> +#endif
>> +
>> ?static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>> ?{
>> ? ? ? const struct samsung_keypad_platdata *pdata;
>> @@ -246,7 +366,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>> ? ? ? unsigned int keymap_size;
>> ? ? ? int error;
>>
>> - ? ? pdata = pdev->dev.platform_data;
>> + ? ? if (pdev->dev.of_node)
>> + ? ? ? ? ? ? pdata = samsung_keypad_parse_dt(&pdev->dev);
>> + ? ? else
>> + ? ? ? ? ? ? pdata = pdev->dev.platform_data;
>> ? ? ? if (!pdata) {
>> ? ? ? ? ? ? ? dev_err(&pdev->dev, "no platform data defined\n");
>> ? ? ? ? ? ? ? return -EINVAL;
>> @@ -303,6 +426,16 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>> ? ? ? keypad->cols = pdata->cols;
>> ? ? ? init_waitqueue_head(&keypad->wait);
>>
>> + ? ? if (pdev->dev.of_node) {
>> + ? ? ? ? ? ? samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
>> +#ifdef CONFIG_OF
>> + ? ? ? ? ? ? keypad->type = of_device_is_compatible(pdev->dev.of_node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "samsung,s5pv210-keypad");
>> +#endif
>
> It is odd having the #ifdef around only part of the if() block.

I did not want to do it this way. But any other way would have
increased the number of lines added. If is against the coding rules or
style, I will change it.

>
>> + ? ? } else {
>> + ? ? ? ? ? ? keypad->type = platform_get_device_id(pdev)->driver_data;
>> + ? ? }
>> +
>> ? ? ? input_dev->name = pdev->name;
>> ? ? ? input_dev->id.bustype = BUS_HOST;
>> ? ? ? input_dev->dev.parent = &pdev->dev;
>> @@ -343,12 +476,19 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>>
>> ? ? ? device_init_wakeup(&pdev->dev, pdata->wakeup);
>> ? ? ? platform_set_drvdata(pdev, keypad);
>> +
>> + ? ? if (pdev->dev.of_node) {
>> + ? ? ? ? ? ? devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
>> + ? ? ? ? ? ? devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
>> + ? ? ? ? ? ? devm_kfree(&pdev->dev, (void *)pdata);
>> + ? ? }
>
> Don't need to do this. ?The driver core will take care of freeing the
> devm for you when removed.

Dmitry Torokhov suggested that since the allocated pdata is not used
after the probe, it can be deallocated to save some memory. So this
was added.

>
> A few things that should be fixed up, but otherwise looks pretty good.
>

Thanks for your review and suggestions.

Regards,
Thomas.

[...]

Re: [PATCH 2/2] input: samsung-keypad: Add device tree support

[Grant Likely]

On Wed, Sep 14, 2011 at 10:19:22PM +0530, Thomas Abraham wrote:
> Hi Grant,
> 
> On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
> >> Add device tree based discovery support for Samsung's keypad controller.
> >>
> >> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> >> Cc: Donghwa Lee <dh09.lee@samsung.com>
> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> ---
> >>  .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
> >>  drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
> >>  2 files changed, 253 insertions(+), 12 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
> >> new file mode 100644
> >> index 0000000..e1c7237
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
> >> @@ -0,0 +1,88 @@
> >> +* Samsung's Keypad Controller device tree bindings
> >> +
> >> +Samsung's Keypad controller is used to interface a SoC with a matrix-type
> >> +keypad device. The keypad controller supports multiple row and column lines.
> >> +A key can be placed at each intersection of a unique row and a unique column.
> >> +The keypad controller can sense a key-press and key-release and report the
> >> +event using a interrupt to the cpu.
> >> +
> >> +Required SoC Specific Properties:
> >> +- compatible: should be one of the following
> >> +  - "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
> >> +    controller.
> >> +  - "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
> >> +    controller.
> >> +
> >> +- reg: physical base address of the controller and length of memory mapped
> >> +  region.
> >> +
> >> +- interrupts: The interrupt number to the cpu.
> >> +
> >> +Required Board Specific Properties:
> >> +- samsung,keypad-num-rows: Number of row lines connected to the keypad
> >> +  controller.
> >> +
> >> +- samsung,keypad-num-columns: Number of column lines connected to the
> >> +  keypad controller.
> >> +
> >> +- row-gpios: List of gpios used as row lines. The gpio specifier for
> >> +  this property depends on the gpio controller to which these row lines
> >> +  are connected.
> >> +
> >> +- col-gpios: List of gpios used as column lines. The gpio specifier for
> >> +  this property depends on the gpio controller to which these column
> >> +  lines are connected.
> >
> > I know we've got overlapping terminology here.  When you say GPIOs
> > here, does it refer to the pin multiplexing feature of the samsung
> > parts, and thus how the keypad controller IO is routed to the pins?
> > Or does the driver manipulate GPIO lines manually?
> 
> It is intended to mean gpio and not the pinmux. But the way the gpio
> specifier is specified in the dts file, it includes information about
> both gpio number and the pin-control details. For instance, if 'gpa0'
> is a gpio controller (which includes pin-control functionality as well
> in the hardware), then the gpio specifier for the keypad would be as
> below.
> 
> row-gpios = <&gpa0 0 3 3 0>,
>                  <&gpa0 1 3 3 0>;
> 
> The syntax for the gpio specifier is
> <[phandle of gpio controller] [pin number within the gpio controller]
> [mux function] [pull up/down] [drive strength]>
> 
> The keypad driver does not know or use the mux function, pull up/down
> and drive strength details. The driver would call of_get_gpio to get
> the linux gpio number and then do a gpio_request on that gpio number.
> The gpio dt support manges the pin-mux and other settings
> transparently for the keypad driver. So even though the gpio specifier
> format changes, the dt support code for the driver does not change.
> 
> The driver does not manipulate the gpios directly. It just requests
> for the gpio number and makes a note of the number to free it when the
> driver unbinds.
> 
> >
> > If it is pin multiplexing, then using the GPIO binding seems rather
> > odd. It may be entirely appropriate, but it will need to play well
> > with the pinmux stuff that linusw is working on.
> 
> When pinmux/pin-ctrl functionality which linusw is developing is used
> for this driver, it would be a extension to this patch. The driver
> would request for the gpio and then use the pin-ctrl subsystem api to
> setup the pin-control. In that case, the gpio-specifier would change
> but that change would be break anything which this patch does.
> 
> >
> >> +
> >> +- Keys represented as child nodes: Each key connected to the keypad
> >> +  controller is represented as a child node to the keypad controller
> >> +  device node and should include the following properties.
> >> +  - keypad,row: the row number to which the key is connected.
> >> +  - keypad,column: the column number to which the key is connected.
> >> +  - keypad,key-code: the key-code to be reported when the key is pressed
> >> +    and released.
> >
> > What defines the meanings of the key codes?
> 
> The key-code could be any value which the system would want the keypad
> driver to report when that key is pressed.

Are they linux keycodes?  If so, then this property name can
probably be linux,code.  There is already precedence for that
usage in Documentation/devicetree/bindings/gpio/gpio-keys.txt.  (I
would personally prefer "linux,key-code", but sometimes it is better
to go with existing precidence) You could also use linux,input-type as
specified in that binding.

> 
> >
> >> +
> >> +Optional Properties specific to linux:
> >> +- linux,keypad-no-autorepeat: do no enable autorepeat feature.
> >> +- linux,keypad-wakeup: use any event on keypad as wakeup event.
> >
> > Is this really a linux-specific feature?
> 
> Yes, it is a property defined by the linux subsystems. A non-linux
> system might not have such features.

I was specifically referring to the wakeup property, but okay, I can
see the argument that it is linux-specific because it is usage-policy
related.

g.

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Grant Likely]

On Wed, Sep 14, 2011 at 10:19:22PM +0530, Thomas Abraham wrote:
> Hi Grant,
> 
> On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
> >> Add device tree based discovery support for Samsung's keypad controller.
> >>
> >> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> >> Cc: Donghwa Lee <dh09.lee@samsung.com>
> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> >> ---
> >> ?.../devicetree/bindings/input/samsung-keypad.txt ? | ? 88 ++++++++++
> >> ?drivers/input/keyboard/samsung-keypad.c ? ? ? ? ? ?| ?177 ++++++++++++++++++--
> >> ?2 files changed, 253 insertions(+), 12 deletions(-)
> >> ?create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
> >> new file mode 100644
> >> index 0000000..e1c7237
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
> >> @@ -0,0 +1,88 @@
> >> +* Samsung's Keypad Controller device tree bindings
> >> +
> >> +Samsung's Keypad controller is used to interface a SoC with a matrix-type
> >> +keypad device. The keypad controller supports multiple row and column lines.
> >> +A key can be placed at each intersection of a unique row and a unique column.
> >> +The keypad controller can sense a key-press and key-release and report the
> >> +event using a interrupt to the cpu.
> >> +
> >> +Required SoC Specific Properties:
> >> +- compatible: should be one of the following
> >> + ?- "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
> >> + ? ?controller.
> >> + ?- "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
> >> + ? ?controller.
> >> +
> >> +- reg: physical base address of the controller and length of memory mapped
> >> + ?region.
> >> +
> >> +- interrupts: The interrupt number to the cpu.
> >> +
> >> +Required Board Specific Properties:
> >> +- samsung,keypad-num-rows: Number of row lines connected to the keypad
> >> + ?controller.
> >> +
> >> +- samsung,keypad-num-columns: Number of column lines connected to the
> >> + ?keypad controller.
> >> +
> >> +- row-gpios: List of gpios used as row lines. The gpio specifier for
> >> + ?this property depends on the gpio controller to which these row lines
> >> + ?are connected.
> >> +
> >> +- col-gpios: List of gpios used as column lines. The gpio specifier for
> >> + ?this property depends on the gpio controller to which these column
> >> + ?lines are connected.
> >
> > I know we've got overlapping terminology here. ?When you say GPIOs
> > here, does it refer to the pin multiplexing feature of the samsung
> > parts, and thus how the keypad controller IO is routed to the pins?
> > Or does the driver manipulate GPIO lines manually?
> 
> It is intended to mean gpio and not the pinmux. But the way the gpio
> specifier is specified in the dts file, it includes information about
> both gpio number and the pin-control details. For instance, if 'gpa0'
> is a gpio controller (which includes pin-control functionality as well
> in the hardware), then the gpio specifier for the keypad would be as
> below.
> 
> row-gpios = <&gpa0 0 3 3 0>,
>                  <&gpa0 1 3 3 0>;
> 
> The syntax for the gpio specifier is
> <[phandle of gpio controller] [pin number within the gpio controller]
> [mux function] [pull up/down] [drive strength]>
> 
> The keypad driver does not know or use the mux function, pull up/down
> and drive strength details. The driver would call of_get_gpio to get
> the linux gpio number and then do a gpio_request on that gpio number.
> The gpio dt support manges the pin-mux and other settings
> transparently for the keypad driver. So even though the gpio specifier
> format changes, the dt support code for the driver does not change.
> 
> The driver does not manipulate the gpios directly. It just requests
> for the gpio number and makes a note of the number to free it when the
> driver unbinds.
> 
> >
> > If it is pin multiplexing, then using the GPIO binding seems rather
> > odd. It may be entirely appropriate, but it will need to play well
> > with the pinmux stuff that linusw is working on.
> 
> When pinmux/pin-ctrl functionality which linusw is developing is used
> for this driver, it would be a extension to this patch. The driver
> would request for the gpio and then use the pin-ctrl subsystem api to
> setup the pin-control. In that case, the gpio-specifier would change
> but that change would be break anything which this patch does.
> 
> >
> >> +
> >> +- Keys represented as child nodes: Each key connected to the keypad
> >> + ?controller is represented as a child node to the keypad controller
> >> + ?device node and should include the following properties.
> >> + ?- keypad,row: the row number to which the key is connected.
> >> + ?- keypad,column: the column number to which the key is connected.
> >> + ?- keypad,key-code: the key-code to be reported when the key is pressed
> >> + ? ?and released.
> >
> > What defines the meanings of the key codes?
> 
> The key-code could be any value which the system would want the keypad
> driver to report when that key is pressed.

Are they linux keycodes?  If so, then this property name can
probably be linux,code.  There is already precedence for that
usage in Documentation/devicetree/bindings/gpio/gpio-keys.txt.  (I
would personally prefer "linux,key-code", but sometimes it is better
to go with existing precidence) You could also use linux,input-type as
specified in that binding.

> 
> >
> >> +
> >> +Optional Properties specific to linux:
> >> +- linux,keypad-no-autorepeat: do no enable autorepeat feature.
> >> +- linux,keypad-wakeup: use any event on keypad as wakeup event.
> >
> > Is this really a linux-specific feature?
> 
> Yes, it is a property defined by the linux subsystems. A non-linux
> system might not have such features.

I was specifically referring to the wakeup property, but okay, I can
see the argument that it is linux-specific because it is usage-policy
related.

g.

Re: [PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Hi Grant,

On 14 September 2011 22:43, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Sep 14, 2011 at 10:19:22PM +0530, Thomas Abraham wrote:
>> Hi Grant,
>>
>> On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
>> > On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
>> >> Add device tree based discovery support for Samsung's keypad controller.
>> >>
>> >> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
>> >> Cc: Donghwa Lee <dh09.lee@samsung.com>
>> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> >> ---
>> >>  .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
>> >>  drivers/input/keyboard/samsung-keypad.c            |  177 ++++++++++++++++++--
>> >>  2 files changed, 253 insertions(+), 12 deletions(-)
>> >>  create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
>> >>

[...]

>> >> +- Keys represented as child nodes: Each key connected to the keypad
>> >> +  controller is represented as a child node to the keypad controller
>> >> +  device node and should include the following properties.
>> >> +  - keypad,row: the row number to which the key is connected.
>> >> +  - keypad,column: the column number to which the key is connected.
>> >> +  - keypad,key-code: the key-code to be reported when the key is pressed
>> >> +    and released.
>> >
>> > What defines the meanings of the key codes?
>>
>> The key-code could be any value which the system would want the keypad
>> driver to report when that key is pressed.
>
> Are they linux keycodes?  If so, then this property name can
> probably be linux,code.  There is already precedence for that
> usage in Documentation/devicetree/bindings/gpio/gpio-keys.txt.  (I
> would personally prefer "linux,key-code", but sometimes it is better
> to go with existing precidence) You could also use linux,input-type as
> specified in that binding.

Ok. For linux, "keypad,key-code" would mean linux keycodes. The
property name 'keypad,key-code' was chosen since it can be reused on
non-linux platforms as well. I did have a look at
Documentation/devicetree/bindings/gpio/gpio-keys.txt while doing this,
but preferred using 'keypad,key-code' since it would be generic. Given
a choice, I would like to retain this.

>
>>
>> >
>> >> +
>> >> +Optional Properties specific to linux:
>> >> +- linux,keypad-no-autorepeat: do no enable autorepeat feature.
>> >> +- linux,keypad-wakeup: use any event on keypad as wakeup event.
>> >
>> > Is this really a linux-specific feature?
>>
>> Yes, it is a property defined by the linux subsystems. A non-linux
>> system might not have such features.
>
> I was specifically referring to the wakeup property, but okay, I can
> see the argument that it is linux-specific because it is usage-policy
> related.
>
> g.

Thanks for your review.

Regards,
Thomas.
--
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

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Hi Grant,

On 14 September 2011 22:43, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Sep 14, 2011 at 10:19:22PM +0530, Thomas Abraham wrote:
>> Hi Grant,
>>
>> On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
>> > On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
>> >> Add device tree based discovery support for Samsung's keypad controller.
>> >>
>> >> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
>> >> Cc: Donghwa Lee <dh09.lee@samsung.com>
>> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> >> ---
>> >> ?.../devicetree/bindings/input/samsung-keypad.txt ? | ? 88 ++++++++++
>> >> ?drivers/input/keyboard/samsung-keypad.c ? ? ? ? ? ?| ?177 ++++++++++++++++++--
>> >> ?2 files changed, 253 insertions(+), 12 deletions(-)
>> >> ?create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt
>> >>

[...]

>> >> +- Keys represented as child nodes: Each key connected to the keypad
>> >> + ?controller is represented as a child node to the keypad controller
>> >> + ?device node and should include the following properties.
>> >> + ?- keypad,row: the row number to which the key is connected.
>> >> + ?- keypad,column: the column number to which the key is connected.
>> >> + ?- keypad,key-code: the key-code to be reported when the key is pressed
>> >> + ? ?and released.
>> >
>> > What defines the meanings of the key codes?
>>
>> The key-code could be any value which the system would want the keypad
>> driver to report when that key is pressed.
>
> Are they linux keycodes? ?If so, then this property name can
> probably be linux,code. ?There is already precedence for that
> usage in Documentation/devicetree/bindings/gpio/gpio-keys.txt. ?(I
> would personally prefer "linux,key-code", but sometimes it is better
> to go with existing precidence) You could also use linux,input-type as
> specified in that binding.

Ok. For linux, "keypad,key-code" would mean linux keycodes. The
property name 'keypad,key-code' was chosen since it can be reused on
non-linux platforms as well. I did have a look at
Documentation/devicetree/bindings/gpio/gpio-keys.txt while doing this,
but preferred using 'keypad,key-code' since it would be generic. Given
a choice, I would like to retain this.

>
>>
>> >
>> >> +
>> >> +Optional Properties specific to linux:
>> >> +- linux,keypad-no-autorepeat: do no enable autorepeat feature.
>> >> +- linux,keypad-wakeup: use any event on keypad as wakeup event.
>> >
>> > Is this really a linux-specific feature?
>>
>> Yes, it is a property defined by the linux subsystems. A non-linux
>> system might not have such features.
>
> I was specifically referring to the wakeup property, but okay, I can
> see the argument that it is linux-specific because it is usage-policy
> related.
>
> g.

Thanks for your review.

Regards,
Thomas.

Re: [PATCH 2/2] input: samsung-keypad: Add device tree support

[Grant Likely]

On Wed, Sep 14, 2011 at 12:09 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> On 14 September 2011 22:43, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Sep 14, 2011 at 10:19:22PM +0530, Thomas Abraham wrote:
>>> On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> > On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
>>> >> +- Keys represented as child nodes: Each key connected to the keypad
>>> >> +  controller is represented as a child node to the keypad controller
>>> >> +  device node and should include the following properties.
>>> >> +  - keypad,row: the row number to which the key is connected.
>>> >> +  - keypad,column: the column number to which the key is connected.
>>> >> +  - keypad,key-code: the key-code to be reported when the key is pressed
>>> >> +    and released.
>>> >
>>> > What defines the meanings of the key codes?
>>>
>>> The key-code could be any value which the system would want the keypad
>>> driver to report when that key is pressed.
>>
>> Are they linux keycodes?  If so, then this property name can
>> probably be linux,code.  There is already precedence for that
>> usage in Documentation/devicetree/bindings/gpio/gpio-keys.txt.  (I
>> would personally prefer "linux,key-code", but sometimes it is better
>> to go with existing precidence) You could also use linux,input-type as
>> specified in that binding.
>
> Ok. For linux, "keypad,key-code" would mean linux keycodes. The
> property name 'keypad,key-code' was chosen since it can be reused on
> non-linux platforms as well. I did have a look at
> Documentation/devicetree/bindings/gpio/gpio-keys.txt while doing this,
> but preferred using 'keypad,key-code' since it would be generic. Given
> a choice, I would like to retain this.

This was debated a bit on the gpio-keys binding.  The binding *must*
specify where it is getting the keycodes from.  For the gpio-keys
binding, it was decided that the Linux keycodes were sufficient since
they are exported to userspace, and therefore part of the stable
kernel ABI (they will never change).  "keypad,key-code" is completely
useless as a generic binding since it doesn't specify where the
keycode meanings come from.  Besides, "linux,code" can be reused on
non-linux platforms too, it just means that the authoritative source
of the keycodes is the Linux kernel.

g
--
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

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Grant Likely]

On Wed, Sep 14, 2011 at 12:09 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> On 14 September 2011 22:43, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Sep 14, 2011 at 10:19:22PM +0530, Thomas Abraham wrote:
>>> On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> > On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
>>> >> +- Keys represented as child nodes: Each key connected to the keypad
>>> >> + ?controller is represented as a child node to the keypad controller
>>> >> + ?device node and should include the following properties.
>>> >> + ?- keypad,row: the row number to which the key is connected.
>>> >> + ?- keypad,column: the column number to which the key is connected.
>>> >> + ?- keypad,key-code: the key-code to be reported when the key is pressed
>>> >> + ? ?and released.
>>> >
>>> > What defines the meanings of the key codes?
>>>
>>> The key-code could be any value which the system would want the keypad
>>> driver to report when that key is pressed.
>>
>> Are they linux keycodes? ?If so, then this property name can
>> probably be linux,code. ?There is already precedence for that
>> usage in Documentation/devicetree/bindings/gpio/gpio-keys.txt. ?(I
>> would personally prefer "linux,key-code", but sometimes it is better
>> to go with existing precidence) You could also use linux,input-type as
>> specified in that binding.
>
> Ok. For linux, "keypad,key-code" would mean linux keycodes. The
> property name 'keypad,key-code' was chosen since it can be reused on
> non-linux platforms as well. I did have a look at
> Documentation/devicetree/bindings/gpio/gpio-keys.txt while doing this,
> but preferred using 'keypad,key-code' since it would be generic. Given
> a choice, I would like to retain this.

This was debated a bit on the gpio-keys binding.  The binding *must*
specify where it is getting the keycodes from.  For the gpio-keys
binding, it was decided that the Linux keycodes were sufficient since
they are exported to userspace, and therefore part of the stable
kernel ABI (they will never change).  "keypad,key-code" is completely
useless as a generic binding since it doesn't specify where the
keycode meanings come from.  Besides, "linux,code" can be reused on
non-linux platforms too, it just means that the authoritative source
of the keycodes is the Linux kernel.

g

Re: [PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

On 15 September 2011 00:40, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Sep 14, 2011 at 12:09 PM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>> On 14 September 2011 22:43, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Wed, Sep 14, 2011 at 10:19:22PM +0530, Thomas Abraham wrote:
>>>> On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>> > On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
>>>> >> +- Keys represented as child nodes: Each key connected to the keypad
>>>> >> +  controller is represented as a child node to the keypad controller
>>>> >> +  device node and should include the following properties.
>>>> >> +  - keypad,row: the row number to which the key is connected.
>>>> >> +  - keypad,column: the column number to which the key is connected.
>>>> >> +  - keypad,key-code: the key-code to be reported when the key is pressed
>>>> >> +    and released.
>>>> >
>>>> > What defines the meanings of the key codes?
>>>>
>>>> The key-code could be any value which the system would want the keypad
>>>> driver to report when that key is pressed.
>>>
>>> Are they linux keycodes?  If so, then this property name can
>>> probably be linux,code.  There is already precedence for that
>>> usage in Documentation/devicetree/bindings/gpio/gpio-keys.txt.  (I
>>> would personally prefer "linux,key-code", but sometimes it is better
>>> to go with existing precidence) You could also use linux,input-type as
>>> specified in that binding.
>>
>> Ok. For linux, "keypad,key-code" would mean linux keycodes. The
>> property name 'keypad,key-code' was chosen since it can be reused on
>> non-linux platforms as well. I did have a look at
>> Documentation/devicetree/bindings/gpio/gpio-keys.txt while doing this,
>> but preferred using 'keypad,key-code' since it would be generic. Given
>> a choice, I would like to retain this.
>
> This was debated a bit on the gpio-keys binding.  The binding *must*
> specify where it is getting the keycodes from.  For the gpio-keys
> binding, it was decided that the Linux keycodes were sufficient since
> they are exported to userspace, and therefore part of the stable
> kernel ABI (they will never change).  "keypad,key-code" is completely
> useless as a generic binding since it doesn't specify where the
> keycode meanings come from.  Besides, "linux,code" can be reused on
> non-linux platforms too, it just means that the authoritative source
> of the keycodes is the Linux kernel.
>
> g
>

Ok. I understand this now. I will change this to "linux,code" in the
next submission.

Thanks,
Thomas.

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

On 15 September 2011 00:40, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Sep 14, 2011 at 12:09 PM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>> On 14 September 2011 22:43, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Wed, Sep 14, 2011 at 10:19:22PM +0530, Thomas Abraham wrote:
>>>> On 14 September 2011 21:41, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>> > On Tue, Sep 13, 2011 at 05:56:19PM +0530, Thomas Abraham wrote:
>>>> >> +- Keys represented as child nodes: Each key connected to the keypad
>>>> >> + ?controller is represented as a child node to the keypad controller
>>>> >> + ?device node and should include the following properties.
>>>> >> + ?- keypad,row: the row number to which the key is connected.
>>>> >> + ?- keypad,column: the column number to which the key is connected.
>>>> >> + ?- keypad,key-code: the key-code to be reported when the key is pressed
>>>> >> + ? ?and released.
>>>> >
>>>> > What defines the meanings of the key codes?
>>>>
>>>> The key-code could be any value which the system would want the keypad
>>>> driver to report when that key is pressed.
>>>
>>> Are they linux keycodes? ?If so, then this property name can
>>> probably be linux,code. ?There is already precedence for that
>>> usage in Documentation/devicetree/bindings/gpio/gpio-keys.txt. ?(I
>>> would personally prefer "linux,key-code", but sometimes it is better
>>> to go with existing precidence) You could also use linux,input-type as
>>> specified in that binding.
>>
>> Ok. For linux, "keypad,key-code" would mean linux keycodes. The
>> property name 'keypad,key-code' was chosen since it can be reused on
>> non-linux platforms as well. I did have a look at
>> Documentation/devicetree/bindings/gpio/gpio-keys.txt while doing this,
>> but preferred using 'keypad,key-code' since it would be generic. Given
>> a choice, I would like to retain this.
>
> This was debated a bit on the gpio-keys binding. ?The binding *must*
> specify where it is getting the keycodes from. ?For the gpio-keys
> binding, it was decided that the Linux keycodes were sufficient since
> they are exported to userspace, and therefore part of the stable
> kernel ABI (they will never change). ?"keypad,key-code" is completely
> useless as a generic binding since it doesn't specify where the
> keycode meanings come from. ?Besides, "linux,code" can be reused on
> non-linux platforms too, it just means that the authoritative source
> of the keycodes is the Linux kernel.
>
> g
>

Ok. I understand this now. I will change this to "linux,code" in the
next submission.

Thanks,
Thomas.

[PATCH 0/2] Add device tree support for Samsung's keypad controller driver

[Thomas Abraham]

This patchset adds device tree support for samsung's keypad controller driver.
First patch adds a new config option to be used by device tree enabled platforms
for selecting the samsung's keypad controller driver. The second patch adds device
tree support for the keypad driver.

Thomas Abraham (2):
  input: samsung-keypad: Add HAVE_SAMSUNG_KEYPAD config option
  input: samsung-keypad: Add device tree support

 .../devicetree/bindings/input/samsung-keypad.txt   |   88 +++++++++++
 drivers/input/keyboard/Kconfig                     |    9 +-
 drivers/input/keyboard/samsung-keypad.c            |  161 +++++++++++++++++++-
 3 files changed, 254 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt

[PATCH 1/2] input: samsung-keypad: Add HAVE_SAMSUNG_KEYPAD config option

[Thomas Abraham]

Samsung keyboard driver could be used with platforms using device tree.
So the inclusion of samsung keyboard driver cannot be based on
SAMSUNG_DEV_KEYPAD. A new config option HAVE_SAMSUNG_KEYPAD is added
which device tree based platforms should use to include samsung keyboard
driver.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 drivers/input/keyboard/Kconfig |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index b4dee9d..370fb18 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -423,9 +423,16 @@ config KEYBOARD_PMIC8XXX
 	  To compile this driver as a module, choose M here: the module will
 	  be called pmic8xxx-keypad.
 
+config HAVE_SAMSUNG_KEYPAD
+	bool
+	help
+	  This will include Samsung Keypad controller driver support. If you
+	  want to include Samsung Keypad support for any machine, kindly
+	  select this in the respective mach-xxxx/Kconfig file.
+
 config KEYBOARD_SAMSUNG
 	tristate "Samsung keypad support"
-	depends on SAMSUNG_DEV_KEYPAD
+	depends on SAMSUNG_DEV_KEYPAD || HAVE_SAMSUNG_KEYPAD
 	help
 	  Say Y here if you want to use the Samsung keypad.
 
-- 
1.6.6.rc2

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Add device tree based discovery support for Samsung's keypad controller.

Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Donghwa Lee <dh09.lee@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 .../devicetree/bindings/input/samsung-keypad.txt   |   88 +++++++++++
 drivers/input/keyboard/samsung-keypad.c            |  161 +++++++++++++++++++-
 2 files changed, 246 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt

diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
new file mode 100644
index 0000000..e1c7237
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
@@ -0,0 +1,88 @@
+* Samsung's Keypad Controller device tree bindings
+
+Samsung's Keypad controller is used to interface a SoC with a matrix-type
+keypad device. The keypad controller supports multiple row and column lines.
+A key can be placed at each intersection of a unique row and a unique column.
+The keypad controller can sense a key-press and key-release and report the
+event using a interrupt to the cpu.
+
+Required SoC Specific Properties:
+- compatible: should be one of the following
+  - "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
+    controller.
+  - "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
+    controller.
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- interrupts: The interrupt number to the cpu.
+
+Required Board Specific Properties:
+- samsung,keypad-num-rows: Number of row lines connected to the keypad
+  controller.
+
+- samsung,keypad-num-columns: Number of column lines connected to the
+  keypad controller.
+
+- row-gpios: List of gpios used as row lines. The gpio specifier for
+  this property depends on the gpio controller to which these row lines
+  are connected.
+
+- col-gpios: List of gpios used as column lines. The gpio specifier for
+  this property depends on the gpio controller to which these column
+  lines are connected.
+
+- Keys represented as child nodes: Each key connected to the keypad
+  controller is represented as a child node to the keypad controller
+  device node and should include the following properties.
+  - keypad,row: the row number to which the key is connected.
+  - keypad,column: the column number to which the key is connected.
+  - keypad,key-code: the key-code to be reported when the key is pressed
+    and released.
+
+Optional Properties specific to linux:
+- linux,keypad-no-autorepeat: do no enable autorepeat feature.
+- linux,keypad-wakeup: use any event on keypad as wakeup event.
+
+
+Example:
+	keypad@100A0000 {
+		compatible = "samsung,s5pv210-keypad";
+		reg = <0x100A0000 0x100>;
+		interrupts = <173>;
+		samsung,keypad-num-rows = <2>;
+		samsung,keypad-num-columns = <8>;
+		linux,input-no-autorepeat;
+		linux,input-wakeup;
+
+		row-gpios = <&gpx2 0 3 3 0
+			     &gpx2 1 3 3 0>;
+
+		col-gpios = <&gpx1 0 3 0 0
+			     &gpx1 1 3 0 0
+			     &gpx1 2 3 0 0
+			     &gpx1 3 3 0 0
+			     &gpx1 4 3 0 0
+			     &gpx1 5 3 0 0
+			     &gpx1 6 3 0 0
+			     &gpx1 7 3 0 0>;
+
+		key_1 {
+			keypad,row = <0>;
+			keypad,column = <3>;
+			keypad,key-code = <2>;
+		};
+
+		key_2 {
+			keypad,row = <0>;
+			keypad,column = <4>;
+			keypad,key-code = <3>;
+		};
+
+		key_3 {
+			keypad,row = <0>;
+			keypad,column = <5>;
+			keypad,key-code = <4>;
+		};
+	};
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index f689f49..29f0a23 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -21,6 +21,8 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/sched.h>
 #include <plat/keypad.h>
 
@@ -72,15 +74,24 @@ struct samsung_keypad {
 	unsigned int rows;
 	unsigned int cols;
 	unsigned int row_state[SAMSUNG_MAX_COLS];
+#ifdef CONFIG_OF
+	int row_gpios[SAMSUNG_MAX_ROWS];
+	int col_gpios[SAMSUNG_MAX_COLS];
+#endif
 	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 =
-		platform_get_device_id(pdev)->driver_data;
+	enum samsung_keypad_type type;
 
+#ifdef CONFIG_OF
+	if (dev->of_node)
+		return of_device_is_compatible(dev->of_node,
+				"samsung,s5pv210-keypad");
+#endif
+	type = platform_get_device_id(pdev)->driver_data;
 	return type == KEYPAD_TYPE_S5PV210;
 }
 
@@ -235,6 +246,130 @@ static void samsung_keypad_close(struct input_dev *input_dev)
 	samsung_keypad_stop(keypad);
 }
 
+#ifdef CONFIG_OF
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+	struct samsung_keypad_platdata *pdata;
+	struct matrix_keymap_data *keymap_data;
+	uint32_t *keymap;
+	struct device_node *np = dev->of_node, *key_np;
+	unsigned int key_count = 0;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "could not allocate memory for platform data\n");
+		return NULL;
+	}
+
+	of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
+	of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
+	if (!pdata->rows || !pdata->cols) {
+		dev_err(dev, "number of keypad rows/columns not specified\n");
+		return NULL;
+	}
+
+	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
+	if (!keymap_data) {
+		dev_err(dev, "could not allocate memory for keymap data\n");
+		return NULL;
+	}
+	pdata->keymap_data = keymap_data;
+
+	for_each_child_of_node(np, key_np)
+		key_count++;
+
+	keymap_data->keymap_size = key_count;
+	keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
+	if (!keymap) {
+		dev_err(dev, "could not allocate memory for keymap\n");
+		return NULL;
+	}
+	keymap_data->keymap = keymap;
+
+	for_each_child_of_node(np, key_np) {
+		unsigned int row, col, key_code;
+		of_property_read_u32(key_np, "keypad,row", &row);
+		of_property_read_u32(key_np, "keypad,column", &col);
+		of_property_read_u32(key_np, "keypad,key-code", &key_code);
+		*keymap++ = KEY(row, col, key_code);
+	}
+
+	if (of_get_property(np, "linux,input-no-autorepeat", NULL))
+		pdata->no_autorepeat = true;
+	if (of_get_property(np, "linux,input-wakeup", NULL))
+		pdata->wakeup = true;
+
+	return pdata;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device *dev,
+				struct samsung_keypad *keypad)
+{
+	struct device_node *np = dev->of_node;
+	int gpio, ret, row, col;
+
+	for (row = 0; row < keypad->rows; row++) {
+		gpio = of_get_named_gpio(np, "row-gpios", row);
+		keypad->row_gpios[row] = gpio;
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
+					row, gpio);
+			continue;
+		}
+
+		ret = gpio_request(gpio, "keypad-row");
+		if (ret)
+			dev_err(dev, "keypad row[%d] gpio request failed\n",
+					row);
+	}
+
+	for (col = 0; col < keypad->cols; col++) {
+		gpio = of_get_named_gpio(np, "col-gpios", col);
+		keypad->col_gpios[col] = gpio;
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
+					col, gpio);
+			continue;
+		}
+
+		ret = gpio_request(col, "keypad-col");
+		if (ret)
+			dev_err(dev, "keypad column[%d] gpio request failed\n",
+					col);
+	}
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+	int cnt;
+
+	for (cnt = 0; cnt < keypad->rows; cnt++)
+		if (gpio_is_valid(keypad->row_gpios[cnt]))
+			gpio_free(keypad->row_gpios[cnt]);
+
+	for (cnt = 0; cnt < keypad->cols; cnt++)
+		if (gpio_is_valid(keypad->col_gpios[cnt]))
+			gpio_free(keypad->col_gpios[cnt]);
+}
+#else
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device_node *np,
+		struct samsung_keypad *keypad, unsigned int rows,
+		unsigned int cols)
+{
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+}
+#endif
+
 static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 {
 	const struct samsung_keypad_platdata *pdata;
@@ -246,7 +381,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 	unsigned int keymap_size;
 	int error;
 
-	pdata = pdev->dev.platform_data;
+	if (pdev->dev.of_node)
+		pdata = samsung_keypad_parse_dt(&pdev->dev);
+	else
+		pdata = pdev->dev.platform_data;
 	if (!pdata) {
 		dev_err(&pdev->dev, "no platform data defined\n");
 		return -EINVAL;
@@ -303,6 +441,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 	keypad->cols = pdata->cols;
 	init_waitqueue_head(&keypad->wait);
 
+	if (pdev->dev.of_node)
+		samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
+
 	input_dev->name = pdev->name;
 	input_dev->id.bustype = BUS_HOST;
 	input_dev->dev.parent = &pdev->dev;
@@ -349,6 +490,7 @@ err_free_irq:
 	free_irq(keypad->irq, keypad);
 err_put_clk:
 	clk_put(keypad->clk);
+	samsung_keypad_dt_gpio_free(keypad);
 err_unmap_base:
 	iounmap(keypad->base);
 err_free_mem:
@@ -374,6 +516,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
 	free_irq(keypad->irq, keypad);
 
 	clk_put(keypad->clk);
+	samsung_keypad_dt_gpio_free(keypad);
 
 	iounmap(keypad->base);
 	kfree(keypad);
@@ -447,6 +590,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
 };
 #endif
 
+#ifdef CONFIG_OF
+static const struct of_device_id samsung_keypad_dt_match[] = {
+	{ .compatible = "samsung,s3c6410-keypad" },
+	{ .compatible = "samsung,s5pv210-keypad" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
+#else
+#define samsung_keypad_dt_match NULL
+#endif
+
 static struct platform_device_id samsung_keypad_driver_ids[] = {
 	{
 		.name		= "samsung-keypad",
@@ -465,6 +619,7 @@ static struct platform_driver samsung_keypad_driver = {
 	.driver		= {
 		.name	= "samsung-keypad",
 		.owner	= THIS_MODULE,
+		.of_match_table = samsung_keypad_dt_match,
 #ifdef CONFIG_PM
 		.pm	= &samsung_keypad_pm_ops,
 #endif
-- 
1.6.6.rc2

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Add device tree based discovery support for Samsung's keypad controller.

Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Donghwa Lee <dh09.lee@samsung.com>
Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---
 .../devicetree/bindings/input/samsung-keypad.txt   |   88 +++++++++++
 drivers/input/keyboard/samsung-keypad.c            |  161 +++++++++++++++++++-
 2 files changed, 246 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/samsung-keypad.txt

diff --git a/Documentation/devicetree/bindings/input/samsung-keypad.txt b/Documentation/devicetree/bindings/input/samsung-keypad.txt
new file mode 100644
index 0000000..e1c7237
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/samsung-keypad.txt
@@ -0,0 +1,88 @@
+* Samsung's Keypad Controller device tree bindings
+
+Samsung's Keypad controller is used to interface a SoC with a matrix-type
+keypad device. The keypad controller supports multiple row and column lines.
+A key can be placed at each intersection of a unique row and a unique column.
+The keypad controller can sense a key-press and key-release and report the
+event using a interrupt to the cpu.
+
+Required SoC Specific Properties:
+- compatible: should be one of the following
+  - "samsung,s3c6410-keypad": For controllers compatible with s3c6410 keypad
+    controller.
+  - "samsung,s5pv210-keypad": For controllers compatible with s5pv210 keypad
+    controller.
+
+- reg: physical base address of the controller and length of memory mapped
+  region.
+
+- interrupts: The interrupt number to the cpu.
+
+Required Board Specific Properties:
+- samsung,keypad-num-rows: Number of row lines connected to the keypad
+  controller.
+
+- samsung,keypad-num-columns: Number of column lines connected to the
+  keypad controller.
+
+- row-gpios: List of gpios used as row lines. The gpio specifier for
+  this property depends on the gpio controller to which these row lines
+  are connected.
+
+- col-gpios: List of gpios used as column lines. The gpio specifier for
+  this property depends on the gpio controller to which these column
+  lines are connected.
+
+- Keys represented as child nodes: Each key connected to the keypad
+  controller is represented as a child node to the keypad controller
+  device node and should include the following properties.
+  - keypad,row: the row number to which the key is connected.
+  - keypad,column: the column number to which the key is connected.
+  - keypad,key-code: the key-code to be reported when the key is pressed
+    and released.
+
+Optional Properties specific to linux:
+- linux,keypad-no-autorepeat: do no enable autorepeat feature.
+- linux,keypad-wakeup: use any event on keypad as wakeup event.
+
+
+Example:
+	keypad at 100A0000 {
+		compatible = "samsung,s5pv210-keypad";
+		reg = <0x100A0000 0x100>;
+		interrupts = <173>;
+		samsung,keypad-num-rows = <2>;
+		samsung,keypad-num-columns = <8>;
+		linux,input-no-autorepeat;
+		linux,input-wakeup;
+
+		row-gpios = <&gpx2 0 3 3 0
+			     &gpx2 1 3 3 0>;
+
+		col-gpios = <&gpx1 0 3 0 0
+			     &gpx1 1 3 0 0
+			     &gpx1 2 3 0 0
+			     &gpx1 3 3 0 0
+			     &gpx1 4 3 0 0
+			     &gpx1 5 3 0 0
+			     &gpx1 6 3 0 0
+			     &gpx1 7 3 0 0>;
+
+		key_1 {
+			keypad,row = <0>;
+			keypad,column = <3>;
+			keypad,key-code = <2>;
+		};
+
+		key_2 {
+			keypad,row = <0>;
+			keypad,column = <4>;
+			keypad,key-code = <3>;
+		};
+
+		key_3 {
+			keypad,row = <0>;
+			keypad,column = <5>;
+			keypad,key-code = <4>;
+		};
+	};
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index f689f49..29f0a23 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -21,6 +21,8 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/sched.h>
 #include <plat/keypad.h>
 
@@ -72,15 +74,24 @@ struct samsung_keypad {
 	unsigned int rows;
 	unsigned int cols;
 	unsigned int row_state[SAMSUNG_MAX_COLS];
+#ifdef CONFIG_OF
+	int row_gpios[SAMSUNG_MAX_ROWS];
+	int col_gpios[SAMSUNG_MAX_COLS];
+#endif
 	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 =
-		platform_get_device_id(pdev)->driver_data;
+	enum samsung_keypad_type type;
 
+#ifdef CONFIG_OF
+	if (dev->of_node)
+		return of_device_is_compatible(dev->of_node,
+				"samsung,s5pv210-keypad");
+#endif
+	type = platform_get_device_id(pdev)->driver_data;
 	return type == KEYPAD_TYPE_S5PV210;
 }
 
@@ -235,6 +246,130 @@ static void samsung_keypad_close(struct input_dev *input_dev)
 	samsung_keypad_stop(keypad);
 }
 
+#ifdef CONFIG_OF
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+	struct samsung_keypad_platdata *pdata;
+	struct matrix_keymap_data *keymap_data;
+	uint32_t *keymap;
+	struct device_node *np = dev->of_node, *key_np;
+	unsigned int key_count = 0;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "could not allocate memory for platform data\n");
+		return NULL;
+	}
+
+	of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
+	of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
+	if (!pdata->rows || !pdata->cols) {
+		dev_err(dev, "number of keypad rows/columns not specified\n");
+		return NULL;
+	}
+
+	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
+	if (!keymap_data) {
+		dev_err(dev, "could not allocate memory for keymap data\n");
+		return NULL;
+	}
+	pdata->keymap_data = keymap_data;
+
+	for_each_child_of_node(np, key_np)
+		key_count++;
+
+	keymap_data->keymap_size = key_count;
+	keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
+	if (!keymap) {
+		dev_err(dev, "could not allocate memory for keymap\n");
+		return NULL;
+	}
+	keymap_data->keymap = keymap;
+
+	for_each_child_of_node(np, key_np) {
+		unsigned int row, col, key_code;
+		of_property_read_u32(key_np, "keypad,row", &row);
+		of_property_read_u32(key_np, "keypad,column", &col);
+		of_property_read_u32(key_np, "keypad,key-code", &key_code);
+		*keymap++ = KEY(row, col, key_code);
+	}
+
+	if (of_get_property(np, "linux,input-no-autorepeat", NULL))
+		pdata->no_autorepeat = true;
+	if (of_get_property(np, "linux,input-wakeup", NULL))
+		pdata->wakeup = true;
+
+	return pdata;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device *dev,
+				struct samsung_keypad *keypad)
+{
+	struct device_node *np = dev->of_node;
+	int gpio, ret, row, col;
+
+	for (row = 0; row < keypad->rows; row++) {
+		gpio = of_get_named_gpio(np, "row-gpios", row);
+		keypad->row_gpios[row] = gpio;
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
+					row, gpio);
+			continue;
+		}
+
+		ret = gpio_request(gpio, "keypad-row");
+		if (ret)
+			dev_err(dev, "keypad row[%d] gpio request failed\n",
+					row);
+	}
+
+	for (col = 0; col < keypad->cols; col++) {
+		gpio = of_get_named_gpio(np, "col-gpios", col);
+		keypad->col_gpios[col] = gpio;
+		if (!gpio_is_valid(gpio)) {
+			dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
+					col, gpio);
+			continue;
+		}
+
+		ret = gpio_request(col, "keypad-col");
+		if (ret)
+			dev_err(dev, "keypad column[%d] gpio request failed\n",
+					col);
+	}
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+	int cnt;
+
+	for (cnt = 0; cnt < keypad->rows; cnt++)
+		if (gpio_is_valid(keypad->row_gpios[cnt]))
+			gpio_free(keypad->row_gpios[cnt]);
+
+	for (cnt = 0; cnt < keypad->cols; cnt++)
+		if (gpio_is_valid(keypad->col_gpios[cnt]))
+			gpio_free(keypad->col_gpios[cnt]);
+}
+#else
+static
+struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+
+static void samsung_keypad_parse_dt_gpio(struct device_node *np,
+		struct samsung_keypad *keypad, unsigned int rows,
+		unsigned int cols)
+{
+}
+
+static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
+{
+}
+#endif
+
 static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 {
 	const struct samsung_keypad_platdata *pdata;
@@ -246,7 +381,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 	unsigned int keymap_size;
 	int error;
 
-	pdata = pdev->dev.platform_data;
+	if (pdev->dev.of_node)
+		pdata = samsung_keypad_parse_dt(&pdev->dev);
+	else
+		pdata = pdev->dev.platform_data;
 	if (!pdata) {
 		dev_err(&pdev->dev, "no platform data defined\n");
 		return -EINVAL;
@@ -303,6 +441,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
 	keypad->cols = pdata->cols;
 	init_waitqueue_head(&keypad->wait);
 
+	if (pdev->dev.of_node)
+		samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
+
 	input_dev->name = pdev->name;
 	input_dev->id.bustype = BUS_HOST;
 	input_dev->dev.parent = &pdev->dev;
@@ -349,6 +490,7 @@ err_free_irq:
 	free_irq(keypad->irq, keypad);
 err_put_clk:
 	clk_put(keypad->clk);
+	samsung_keypad_dt_gpio_free(keypad);
 err_unmap_base:
 	iounmap(keypad->base);
 err_free_mem:
@@ -374,6 +516,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
 	free_irq(keypad->irq, keypad);
 
 	clk_put(keypad->clk);
+	samsung_keypad_dt_gpio_free(keypad);
 
 	iounmap(keypad->base);
 	kfree(keypad);
@@ -447,6 +590,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
 };
 #endif
 
+#ifdef CONFIG_OF
+static const struct of_device_id samsung_keypad_dt_match[] = {
+	{ .compatible = "samsung,s3c6410-keypad" },
+	{ .compatible = "samsung,s5pv210-keypad" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
+#else
+#define samsung_keypad_dt_match NULL
+#endif
+
 static struct platform_device_id samsung_keypad_driver_ids[] = {
 	{
 		.name		= "samsung-keypad",
@@ -465,6 +619,7 @@ static struct platform_driver samsung_keypad_driver = {
 	.driver		= {
 		.name	= "samsung-keypad",
 		.owner	= THIS_MODULE,
+		.of_match_table = samsung_keypad_dt_match,
 #ifdef CONFIG_PM
 		.pm	= &samsung_keypad_pm_ops,
 #endif
-- 
1.6.6.rc2

Re: [PATCH 2/2] input: samsung-keypad: Add device tree support

[Dmitry Torokhov]

Hi Thomas,

On Tue, Sep 06, 2011 at 07:25:17PM +0530, Thomas Abraham wrote:
>  static int samsung_keypad_is_s5pv210(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> -	enum samsung_keypad_type type =
> -		platform_get_device_id(pdev)->driver_data;
> +	enum samsung_keypad_type type;
>  
> +#ifdef CONFIG_OF
> +	if (dev->of_node)
> +		return of_device_is_compatible(dev->of_node,
> +				"samsung,s5pv210-keypad");
> +#endif
> +	type = platform_get_device_id(pdev)->driver_data;
>  	return type == KEYPAD_TYPE_S5PV210;

This function is called every time we scan keypad matrix, I do not think
you want to scan DT bindings here. You need to cache the device type at
probe time and use it.

>  }
>  
> @@ -235,6 +246,130 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>  	samsung_keypad_stop(keypad);
>  }
>  
> +#ifdef CONFIG_OF
> +static
> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
> +{
> +	struct samsung_keypad_platdata *pdata;
> +	struct matrix_keymap_data *keymap_data;
> +	uint32_t *keymap;
> +	struct device_node *np = dev->of_node, *key_np;
> +	unsigned int key_count = 0;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		dev_err(dev, "could not allocate memory for platform data\n");
> +		return NULL;
> +	}

pdata is not used once probe() completes so it would be better to free
it and not rely on devm_* facilities to free it for you once device
unbinds from the driver.

> +
> +	of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
> +	of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
> +	if (!pdata->rows || !pdata->cols) {
> +		dev_err(dev, "number of keypad rows/columns not specified\n");
> +		return NULL;
> +	}
> +
> +	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
> +	if (!keymap_data) {
> +		dev_err(dev, "could not allocate memory for keymap data\n");
> +		return NULL;
> +	}
> +	pdata->keymap_data = keymap_data;
> +
> +	for_each_child_of_node(np, key_np)
> +		key_count++;
> +
> +	keymap_data->keymap_size = key_count;
> +	keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
> +	if (!keymap) {
> +		dev_err(dev, "could not allocate memory for keymap\n");
> +		return NULL;
> +	}
> +	keymap_data->keymap = keymap;
> +
> +	for_each_child_of_node(np, key_np) {
> +		unsigned int row, col, key_code;
> +		of_property_read_u32(key_np, "keypad,row", &row);
> +		of_property_read_u32(key_np, "keypad,column", &col);
> +		of_property_read_u32(key_np, "keypad,key-code", &key_code);
> +		*keymap++ = KEY(row, col, key_code);
> +	}

THis seems like generic mechanism that could be used by other drivers...
Maybe move into matrix-keypad.c? You would also not need to allocate
temporary buffer for intermediate keymap data.

> +
> +	if (of_get_property(np, "linux,input-no-autorepeat", NULL))
> +		pdata->no_autorepeat = true;
> +	if (of_get_property(np, "linux,input-wakeup", NULL))
> +		pdata->wakeup = true;
> +
> +	return pdata;
> +}
> +
> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
> +				struct samsung_keypad *keypad)
> +{
> +	struct device_node *np = dev->of_node;
> +	int gpio, ret, row, col;
> +
> +	for (row = 0; row < keypad->rows; row++) {
> +		gpio = of_get_named_gpio(np, "row-gpios", row);
> +		keypad->row_gpios[row] = gpio;
> +		if (!gpio_is_valid(gpio)) {
> +			dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
> +					row, gpio);
> +			continue;
> +		}
> +
> +		ret = gpio_request(gpio, "keypad-row");
> +		if (ret)
> +			dev_err(dev, "keypad row[%d] gpio request failed\n",
> +					row);
> +	}
> +
> +	for (col = 0; col < keypad->cols; col++) {
> +		gpio = of_get_named_gpio(np, "col-gpios", col);
> +		keypad->col_gpios[col] = gpio;
> +		if (!gpio_is_valid(gpio)) {
> +			dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
> +					col, gpio);
> +			continue;
> +		}
> +
> +		ret = gpio_request(col, "keypad-col");
> +		if (ret)
> +			dev_err(dev, "keypad column[%d] gpio request failed\n",
> +					col);

I think we should bail out if one of the calls fails.

> +	}
> +}
> +
> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
> +{
> +	int cnt;
> +
> +	for (cnt = 0; cnt < keypad->rows; cnt++)
> +		if (gpio_is_valid(keypad->row_gpios[cnt]))
> +			gpio_free(keypad->row_gpios[cnt]);
> +
> +	for (cnt = 0; cnt < keypad->cols; cnt++)
> +		if (gpio_is_valid(keypad->col_gpios[cnt]))
> +			gpio_free(keypad->col_gpios[cnt]);
> +}
> +#else
> +static
> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +
> +static void samsung_keypad_parse_dt_gpio(struct device_node *np,
> +		struct samsung_keypad *keypad, unsigned int rows,
> +		unsigned int cols)
> +{
> +}
> +
> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
> +{
> +}
> +#endif
> +
>  static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  {
>  	const struct samsung_keypad_platdata *pdata;
> @@ -246,7 +381,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  	unsigned int keymap_size;
>  	int error;
>  
> -	pdata = pdev->dev.platform_data;
> +	if (pdev->dev.of_node)
> +		pdata = samsung_keypad_parse_dt(&pdev->dev);
> +	else
> +		pdata = pdev->dev.platform_data;
>  	if (!pdata) {
>  		dev_err(&pdev->dev, "no platform data defined\n");
>  		return -EINVAL;
> @@ -303,6 +441,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  	keypad->cols = pdata->cols;
>  	init_waitqueue_head(&keypad->wait);
>  
> +	if (pdev->dev.of_node)
> +		samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
> +
>  	input_dev->name = pdev->name;
>  	input_dev->id.bustype = BUS_HOST;
>  	input_dev->dev.parent = &pdev->dev;
> @@ -349,6 +490,7 @@ err_free_irq:
>  	free_irq(keypad->irq, keypad);
>  err_put_clk:
>  	clk_put(keypad->clk);
> +	samsung_keypad_dt_gpio_free(keypad);
>  err_unmap_base:
>  	iounmap(keypad->base);
>  err_free_mem:
> @@ -374,6 +516,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
>  	free_irq(keypad->irq, keypad);
>  
>  	clk_put(keypad->clk);
> +	samsung_keypad_dt_gpio_free(keypad);
>  
>  	iounmap(keypad->base);
>  	kfree(keypad);
> @@ -447,6 +590,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
>  };
>  #endif
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id samsung_keypad_dt_match[] = {
> +	{ .compatible = "samsung,s3c6410-keypad" },
> +	{ .compatible = "samsung,s5pv210-keypad" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
> +#else
> +#define samsung_keypad_dt_match NULL
> +#endif
> +
>  static struct platform_device_id samsung_keypad_driver_ids[] = {
>  	{
>  		.name		= "samsung-keypad",
> @@ -465,6 +619,7 @@ static struct platform_driver samsung_keypad_driver = {
>  	.driver		= {
>  		.name	= "samsung-keypad",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = samsung_keypad_dt_match,
>  #ifdef CONFIG_PM
>  		.pm	= &samsung_keypad_pm_ops,
>  #endif
> -- 
> 1.6.6.rc2
> 

Thanks.

-- 
Dmitry

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Dmitry Torokhov]

Hi Thomas,

On Tue, Sep 06, 2011 at 07:25:17PM +0530, Thomas Abraham wrote:
>  static int samsung_keypad_is_s5pv210(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> -	enum samsung_keypad_type type =
> -		platform_get_device_id(pdev)->driver_data;
> +	enum samsung_keypad_type type;
>  
> +#ifdef CONFIG_OF
> +	if (dev->of_node)
> +		return of_device_is_compatible(dev->of_node,
> +				"samsung,s5pv210-keypad");
> +#endif
> +	type = platform_get_device_id(pdev)->driver_data;
>  	return type == KEYPAD_TYPE_S5PV210;

This function is called every time we scan keypad matrix, I do not think
you want to scan DT bindings here. You need to cache the device type at
probe time and use it.

>  }
>  
> @@ -235,6 +246,130 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>  	samsung_keypad_stop(keypad);
>  }
>  
> +#ifdef CONFIG_OF
> +static
> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
> +{
> +	struct samsung_keypad_platdata *pdata;
> +	struct matrix_keymap_data *keymap_data;
> +	uint32_t *keymap;
> +	struct device_node *np = dev->of_node, *key_np;
> +	unsigned int key_count = 0;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		dev_err(dev, "could not allocate memory for platform data\n");
> +		return NULL;
> +	}

pdata is not used once probe() completes so it would be better to free
it and not rely on devm_* facilities to free it for you once device
unbinds from the driver.

> +
> +	of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
> +	of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
> +	if (!pdata->rows || !pdata->cols) {
> +		dev_err(dev, "number of keypad rows/columns not specified\n");
> +		return NULL;
> +	}
> +
> +	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
> +	if (!keymap_data) {
> +		dev_err(dev, "could not allocate memory for keymap data\n");
> +		return NULL;
> +	}
> +	pdata->keymap_data = keymap_data;
> +
> +	for_each_child_of_node(np, key_np)
> +		key_count++;
> +
> +	keymap_data->keymap_size = key_count;
> +	keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
> +	if (!keymap) {
> +		dev_err(dev, "could not allocate memory for keymap\n");
> +		return NULL;
> +	}
> +	keymap_data->keymap = keymap;
> +
> +	for_each_child_of_node(np, key_np) {
> +		unsigned int row, col, key_code;
> +		of_property_read_u32(key_np, "keypad,row", &row);
> +		of_property_read_u32(key_np, "keypad,column", &col);
> +		of_property_read_u32(key_np, "keypad,key-code", &key_code);
> +		*keymap++ = KEY(row, col, key_code);
> +	}

THis seems like generic mechanism that could be used by other drivers...
Maybe move into matrix-keypad.c? You would also not need to allocate
temporary buffer for intermediate keymap data.

> +
> +	if (of_get_property(np, "linux,input-no-autorepeat", NULL))
> +		pdata->no_autorepeat = true;
> +	if (of_get_property(np, "linux,input-wakeup", NULL))
> +		pdata->wakeup = true;
> +
> +	return pdata;
> +}
> +
> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
> +				struct samsung_keypad *keypad)
> +{
> +	struct device_node *np = dev->of_node;
> +	int gpio, ret, row, col;
> +
> +	for (row = 0; row < keypad->rows; row++) {
> +		gpio = of_get_named_gpio(np, "row-gpios", row);
> +		keypad->row_gpios[row] = gpio;
> +		if (!gpio_is_valid(gpio)) {
> +			dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
> +					row, gpio);
> +			continue;
> +		}
> +
> +		ret = gpio_request(gpio, "keypad-row");
> +		if (ret)
> +			dev_err(dev, "keypad row[%d] gpio request failed\n",
> +					row);
> +	}
> +
> +	for (col = 0; col < keypad->cols; col++) {
> +		gpio = of_get_named_gpio(np, "col-gpios", col);
> +		keypad->col_gpios[col] = gpio;
> +		if (!gpio_is_valid(gpio)) {
> +			dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
> +					col, gpio);
> +			continue;
> +		}
> +
> +		ret = gpio_request(col, "keypad-col");
> +		if (ret)
> +			dev_err(dev, "keypad column[%d] gpio request failed\n",
> +					col);

I think we should bail out if one of the calls fails.

> +	}
> +}
> +
> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
> +{
> +	int cnt;
> +
> +	for (cnt = 0; cnt < keypad->rows; cnt++)
> +		if (gpio_is_valid(keypad->row_gpios[cnt]))
> +			gpio_free(keypad->row_gpios[cnt]);
> +
> +	for (cnt = 0; cnt < keypad->cols; cnt++)
> +		if (gpio_is_valid(keypad->col_gpios[cnt]))
> +			gpio_free(keypad->col_gpios[cnt]);
> +}
> +#else
> +static
> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +
> +static void samsung_keypad_parse_dt_gpio(struct device_node *np,
> +		struct samsung_keypad *keypad, unsigned int rows,
> +		unsigned int cols)
> +{
> +}
> +
> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
> +{
> +}
> +#endif
> +
>  static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  {
>  	const struct samsung_keypad_platdata *pdata;
> @@ -246,7 +381,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  	unsigned int keymap_size;
>  	int error;
>  
> -	pdata = pdev->dev.platform_data;
> +	if (pdev->dev.of_node)
> +		pdata = samsung_keypad_parse_dt(&pdev->dev);
> +	else
> +		pdata = pdev->dev.platform_data;
>  	if (!pdata) {
>  		dev_err(&pdev->dev, "no platform data defined\n");
>  		return -EINVAL;
> @@ -303,6 +441,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>  	keypad->cols = pdata->cols;
>  	init_waitqueue_head(&keypad->wait);
>  
> +	if (pdev->dev.of_node)
> +		samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
> +
>  	input_dev->name = pdev->name;
>  	input_dev->id.bustype = BUS_HOST;
>  	input_dev->dev.parent = &pdev->dev;
> @@ -349,6 +490,7 @@ err_free_irq:
>  	free_irq(keypad->irq, keypad);
>  err_put_clk:
>  	clk_put(keypad->clk);
> +	samsung_keypad_dt_gpio_free(keypad);
>  err_unmap_base:
>  	iounmap(keypad->base);
>  err_free_mem:
> @@ -374,6 +516,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
>  	free_irq(keypad->irq, keypad);
>  
>  	clk_put(keypad->clk);
> +	samsung_keypad_dt_gpio_free(keypad);
>  
>  	iounmap(keypad->base);
>  	kfree(keypad);
> @@ -447,6 +590,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
>  };
>  #endif
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id samsung_keypad_dt_match[] = {
> +	{ .compatible = "samsung,s3c6410-keypad" },
> +	{ .compatible = "samsung,s5pv210-keypad" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
> +#else
> +#define samsung_keypad_dt_match NULL
> +#endif
> +
>  static struct platform_device_id samsung_keypad_driver_ids[] = {
>  	{
>  		.name		= "samsung-keypad",
> @@ -465,6 +619,7 @@ static struct platform_driver samsung_keypad_driver = {
>  	.driver		= {
>  		.name	= "samsung-keypad",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = samsung_keypad_dt_match,
>  #ifdef CONFIG_PM
>  		.pm	= &samsung_keypad_pm_ops,
>  #endif
> -- 
> 1.6.6.rc2
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Hi Dmitry,

On 8 September 2011 02:20, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Thomas,
>
> On Tue, Sep 06, 2011 at 07:25:17PM +0530, Thomas Abraham wrote:
>>  static int samsung_keypad_is_s5pv210(struct device *dev)
>>  {
>>       struct platform_device *pdev = to_platform_device(dev);
>> -     enum samsung_keypad_type type =
>> -             platform_get_device_id(pdev)->driver_data;
>> +     enum samsung_keypad_type type;
>>
>> +#ifdef CONFIG_OF
>> +     if (dev->of_node)
>> +             return of_device_is_compatible(dev->of_node,
>> +                             "samsung,s5pv210-keypad");
>> +#endif
>> +     type = platform_get_device_id(pdev)->driver_data;
>>       return type == KEYPAD_TYPE_S5PV210;
>
> This function is called every time we scan keypad matrix, I do not think
> you want to scan DT bindings here. You need to cache the device type at
> probe time and use it.

Ok. As you suggested, this will be changed to cache the type in driver
private data during probe and use the cached copy when required.

>
>>  }
>>
>> @@ -235,6 +246,130 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>>       samsung_keypad_stop(keypad);
>>  }
>>
>> +#ifdef CONFIG_OF
>> +static
>> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
>> +{
>> +     struct samsung_keypad_platdata *pdata;
>> +     struct matrix_keymap_data *keymap_data;
>> +     uint32_t *keymap;
>> +     struct device_node *np = dev->of_node, *key_np;
>> +     unsigned int key_count = 0;
>> +
>> +     pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +     if (!pdata) {
>> +             dev_err(dev, "could not allocate memory for platform data\n");
>> +             return NULL;
>> +     }
>
> pdata is not used once probe() completes so it would be better to free
> it and not rely on devm_* facilities to free it for you once device
> unbinds from the driver.

Ok. That would be better. pdata will be freed after probe completes.

>
>> +
>> +     of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
>> +     of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
>> +     if (!pdata->rows || !pdata->cols) {
>> +             dev_err(dev, "number of keypad rows/columns not specified\n");
>> +             return NULL;
>> +     }
>> +
>> +     keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
>> +     if (!keymap_data) {
>> +             dev_err(dev, "could not allocate memory for keymap data\n");
>> +             return NULL;
>> +     }
>> +     pdata->keymap_data = keymap_data;
>> +
>> +     for_each_child_of_node(np, key_np)
>> +             key_count++;
>> +
>> +     keymap_data->keymap_size = key_count;
>> +     keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
>> +     if (!keymap) {
>> +             dev_err(dev, "could not allocate memory for keymap\n");
>> +             return NULL;
>> +     }
>> +     keymap_data->keymap = keymap;
>> +
>> +     for_each_child_of_node(np, key_np) {
>> +             unsigned int row, col, key_code;
>> +             of_property_read_u32(key_np, "keypad,row", &row);
>> +             of_property_read_u32(key_np, "keypad,column", &col);
>> +             of_property_read_u32(key_np, "keypad,key-code", &key_code);
>> +             *keymap++ = KEY(row, col, key_code);
>> +     }
>
> THis seems like generic mechanism that could be used by other drivers...
> Maybe move into matrix-keypad.c? You would also not need to allocate
> temporary buffer for intermediate keymap data.

Yes, this could be reused in other drivers as well. But, moving this
into matrix-keypad.c file means that KEYBOARD_MATRIX config option
would have to be selected for all platforms reusing this code. So, I
am not sure of the right place for this.

>
>> +
>> +     if (of_get_property(np, "linux,input-no-autorepeat", NULL))
>> +             pdata->no_autorepeat = true;
>> +     if (of_get_property(np, "linux,input-wakeup", NULL))
>> +             pdata->wakeup = true;
>> +
>> +     return pdata;
>> +}
>> +
>> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
>> +                             struct samsung_keypad *keypad)
>> +{
>> +     struct device_node *np = dev->of_node;
>> +     int gpio, ret, row, col;
>> +
>> +     for (row = 0; row < keypad->rows; row++) {
>> +             gpio = of_get_named_gpio(np, "row-gpios", row);
>> +             keypad->row_gpios[row] = gpio;
>> +             if (!gpio_is_valid(gpio)) {
>> +                     dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
>> +                                     row, gpio);
>> +                     continue;
>> +             }
>> +
>> +             ret = gpio_request(gpio, "keypad-row");
>> +             if (ret)
>> +                     dev_err(dev, "keypad row[%d] gpio request failed\n",
>> +                                     row);
>> +     }
>> +
>> +     for (col = 0; col < keypad->cols; col++) {
>> +             gpio = of_get_named_gpio(np, "col-gpios", col);
>> +             keypad->col_gpios[col] = gpio;
>> +             if (!gpio_is_valid(gpio)) {
>> +                     dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
>> +                                     col, gpio);
>> +                     continue;
>> +             }
>> +
>> +             ret = gpio_request(col, "keypad-col");
>> +             if (ret)
>> +                     dev_err(dev, "keypad column[%d] gpio request failed\n",
>> +                                     col);
>
> I think we should bail out if one of the calls fails.

I intended to continue even if some request fails because there could
a partially usable keyboard. If there is a failure, there is a error
message to indicate that keyboard might not be fully functional. If
you are not too strict on this one, I would like to retain it this
way.

>
>> +     }
>> +}
>> +
>> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
>> +{
>> +     int cnt;
>> +
>> +     for (cnt = 0; cnt < keypad->rows; cnt++)
>> +             if (gpio_is_valid(keypad->row_gpios[cnt]))
>> +                     gpio_free(keypad->row_gpios[cnt]);
>> +
>> +     for (cnt = 0; cnt < keypad->cols; cnt++)
>> +             if (gpio_is_valid(keypad->col_gpios[cnt]))
>> +                     gpio_free(keypad->col_gpios[cnt]);
>> +}
>> +#else
>> +static
>> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
>> +{
>> +     return NULL;
>> +}
>> +
>> +static void samsung_keypad_parse_dt_gpio(struct device_node *np,
>> +             struct samsung_keypad *keypad, unsigned int rows,
>> +             unsigned int cols)
>> +{
>> +}
>> +
>> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
>> +{
>> +}
>> +#endif
>> +
>>  static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>>  {
>>       const struct samsung_keypad_platdata *pdata;
>> @@ -246,7 +381,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>>       unsigned int keymap_size;
>>       int error;
>>
>> -     pdata = pdev->dev.platform_data;
>> +     if (pdev->dev.of_node)
>> +             pdata = samsung_keypad_parse_dt(&pdev->dev);
>> +     else
>> +             pdata = pdev->dev.platform_data;
>>       if (!pdata) {
>>               dev_err(&pdev->dev, "no platform data defined\n");
>>               return -EINVAL;
>> @@ -303,6 +441,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>>       keypad->cols = pdata->cols;
>>       init_waitqueue_head(&keypad->wait);
>>
>> +     if (pdev->dev.of_node)
>> +             samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
>> +
>>       input_dev->name = pdev->name;
>>       input_dev->id.bustype = BUS_HOST;
>>       input_dev->dev.parent = &pdev->dev;
>> @@ -349,6 +490,7 @@ err_free_irq:
>>       free_irq(keypad->irq, keypad);
>>  err_put_clk:
>>       clk_put(keypad->clk);
>> +     samsung_keypad_dt_gpio_free(keypad);
>>  err_unmap_base:
>>       iounmap(keypad->base);
>>  err_free_mem:
>> @@ -374,6 +516,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
>>       free_irq(keypad->irq, keypad);
>>
>>       clk_put(keypad->clk);
>> +     samsung_keypad_dt_gpio_free(keypad);
>>
>>       iounmap(keypad->base);
>>       kfree(keypad);
>> @@ -447,6 +590,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
>>  };
>>  #endif
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id samsung_keypad_dt_match[] = {
>> +     { .compatible = "samsung,s3c6410-keypad" },
>> +     { .compatible = "samsung,s5pv210-keypad" },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
>> +#else
>> +#define samsung_keypad_dt_match NULL
>> +#endif
>> +
>>  static struct platform_device_id samsung_keypad_driver_ids[] = {
>>       {
>>               .name           = "samsung-keypad",
>> @@ -465,6 +619,7 @@ static struct platform_driver samsung_keypad_driver = {
>>       .driver         = {
>>               .name   = "samsung-keypad",
>>               .owner  = THIS_MODULE,
>> +             .of_match_table = samsung_keypad_dt_match,
>>  #ifdef CONFIG_PM
>>               .pm     = &samsung_keypad_pm_ops,
>>  #endif
>> --
>> 1.6.6.rc2
>>
>
> Thanks.
>
> --
> Dmitry
>

Thanks for your review and comments.

Regards,
Thomas.
--
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

[PATCH 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Hi Dmitry,

On 8 September 2011 02:20, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Thomas,
>
> On Tue, Sep 06, 2011 at 07:25:17PM +0530, Thomas Abraham wrote:
>> ?static int samsung_keypad_is_s5pv210(struct device *dev)
>> ?{
>> ? ? ? struct platform_device *pdev = to_platform_device(dev);
>> - ? ? enum samsung_keypad_type type =
>> - ? ? ? ? ? ? platform_get_device_id(pdev)->driver_data;
>> + ? ? enum samsung_keypad_type type;
>>
>> +#ifdef CONFIG_OF
>> + ? ? if (dev->of_node)
>> + ? ? ? ? ? ? return of_device_is_compatible(dev->of_node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? "samsung,s5pv210-keypad");
>> +#endif
>> + ? ? type = platform_get_device_id(pdev)->driver_data;
>> ? ? ? return type == KEYPAD_TYPE_S5PV210;
>
> This function is called every time we scan keypad matrix, I do not think
> you want to scan DT bindings here. You need to cache the device type at
> probe time and use it.

Ok. As you suggested, this will be changed to cache the type in driver
private data during probe and use the cached copy when required.

>
>> ?}
>>
>> @@ -235,6 +246,130 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>> ? ? ? samsung_keypad_stop(keypad);
>> ?}
>>
>> +#ifdef CONFIG_OF
>> +static
>> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
>> +{
>> + ? ? struct samsung_keypad_platdata *pdata;
>> + ? ? struct matrix_keymap_data *keymap_data;
>> + ? ? uint32_t *keymap;
>> + ? ? struct device_node *np = dev->of_node, *key_np;
>> + ? ? unsigned int key_count = 0;
>> +
>> + ? ? pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> + ? ? if (!pdata) {
>> + ? ? ? ? ? ? dev_err(dev, "could not allocate memory for platform data\n");
>> + ? ? ? ? ? ? return NULL;
>> + ? ? }
>
> pdata is not used once probe() completes so it would be better to free
> it and not rely on devm_* facilities to free it for you once device
> unbinds from the driver.

Ok. That would be better. pdata will be freed after probe completes.

>
>> +
>> + ? ? of_property_read_u32(np, "samsung,keypad-num-rows", &pdata->rows);
>> + ? ? of_property_read_u32(np, "samsung,keypad-num-columns", &pdata->cols);
>> + ? ? if (!pdata->rows || !pdata->cols) {
>> + ? ? ? ? ? ? dev_err(dev, "number of keypad rows/columns not specified\n");
>> + ? ? ? ? ? ? return NULL;
>> + ? ? }
>> +
>> + ? ? keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
>> + ? ? if (!keymap_data) {
>> + ? ? ? ? ? ? dev_err(dev, "could not allocate memory for keymap data\n");
>> + ? ? ? ? ? ? return NULL;
>> + ? ? }
>> + ? ? pdata->keymap_data = keymap_data;
>> +
>> + ? ? for_each_child_of_node(np, key_np)
>> + ? ? ? ? ? ? key_count++;
>> +
>> + ? ? keymap_data->keymap_size = key_count;
>> + ? ? keymap = devm_kzalloc(dev, sizeof(uint32_t) * key_count, GFP_KERNEL);
>> + ? ? if (!keymap) {
>> + ? ? ? ? ? ? dev_err(dev, "could not allocate memory for keymap\n");
>> + ? ? ? ? ? ? return NULL;
>> + ? ? }
>> + ? ? keymap_data->keymap = keymap;
>> +
>> + ? ? for_each_child_of_node(np, key_np) {
>> + ? ? ? ? ? ? unsigned int row, col, key_code;
>> + ? ? ? ? ? ? of_property_read_u32(key_np, "keypad,row", &row);
>> + ? ? ? ? ? ? of_property_read_u32(key_np, "keypad,column", &col);
>> + ? ? ? ? ? ? of_property_read_u32(key_np, "keypad,key-code", &key_code);
>> + ? ? ? ? ? ? *keymap++ = KEY(row, col, key_code);
>> + ? ? }
>
> THis seems like generic mechanism that could be used by other drivers...
> Maybe move into matrix-keypad.c? You would also not need to allocate
> temporary buffer for intermediate keymap data.

Yes, this could be reused in other drivers as well. But, moving this
into matrix-keypad.c file means that KEYBOARD_MATRIX config option
would have to be selected for all platforms reusing this code. So, I
am not sure of the right place for this.

>
>> +
>> + ? ? if (of_get_property(np, "linux,input-no-autorepeat", NULL))
>> + ? ? ? ? ? ? pdata->no_autorepeat = true;
>> + ? ? if (of_get_property(np, "linux,input-wakeup", NULL))
>> + ? ? ? ? ? ? pdata->wakeup = true;
>> +
>> + ? ? return pdata;
>> +}
>> +
>> +static void samsung_keypad_parse_dt_gpio(struct device *dev,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct samsung_keypad *keypad)
>> +{
>> + ? ? struct device_node *np = dev->of_node;
>> + ? ? int gpio, ret, row, col;
>> +
>> + ? ? for (row = 0; row < keypad->rows; row++) {
>> + ? ? ? ? ? ? gpio = of_get_named_gpio(np, "row-gpios", row);
>> + ? ? ? ? ? ? keypad->row_gpios[row] = gpio;
>> + ? ? ? ? ? ? if (!gpio_is_valid(gpio)) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(dev, "keypad row[%d]: invalid gpio %d\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? row, gpio);
>> + ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ret = gpio_request(gpio, "keypad-row");
>> + ? ? ? ? ? ? if (ret)
>> + ? ? ? ? ? ? ? ? ? ? dev_err(dev, "keypad row[%d] gpio request failed\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? row);
>> + ? ? }
>> +
>> + ? ? for (col = 0; col < keypad->cols; col++) {
>> + ? ? ? ? ? ? gpio = of_get_named_gpio(np, "col-gpios", col);
>> + ? ? ? ? ? ? keypad->col_gpios[col] = gpio;
>> + ? ? ? ? ? ? if (!gpio_is_valid(gpio)) {
>> + ? ? ? ? ? ? ? ? ? ? dev_err(dev, "keypad column[%d]: invalid gpio %d\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? col, gpio);
>> + ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? ret = gpio_request(col, "keypad-col");
>> + ? ? ? ? ? ? if (ret)
>> + ? ? ? ? ? ? ? ? ? ? dev_err(dev, "keypad column[%d] gpio request failed\n",
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? col);
>
> I think we should bail out if one of the calls fails.

I intended to continue even if some request fails because there could
a partially usable keyboard. If there is a failure, there is a error
message to indicate that keyboard might not be fully functional. If
you are not too strict on this one, I would like to retain it this
way.

>
>> + ? ? }
>> +}
>> +
>> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
>> +{
>> + ? ? int cnt;
>> +
>> + ? ? for (cnt = 0; cnt < keypad->rows; cnt++)
>> + ? ? ? ? ? ? if (gpio_is_valid(keypad->row_gpios[cnt]))
>> + ? ? ? ? ? ? ? ? ? ? gpio_free(keypad->row_gpios[cnt]);
>> +
>> + ? ? for (cnt = 0; cnt < keypad->cols; cnt++)
>> + ? ? ? ? ? ? if (gpio_is_valid(keypad->col_gpios[cnt]))
>> + ? ? ? ? ? ? ? ? ? ? gpio_free(keypad->col_gpios[cnt]);
>> +}
>> +#else
>> +static
>> +struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev)
>> +{
>> + ? ? return NULL;
>> +}
>> +
>> +static void samsung_keypad_parse_dt_gpio(struct device_node *np,
>> + ? ? ? ? ? ? struct samsung_keypad *keypad, unsigned int rows,
>> + ? ? ? ? ? ? unsigned int cols)
>> +{
>> +}
>> +
>> +static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad)
>> +{
>> +}
>> +#endif
>> +
>> ?static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>> ?{
>> ? ? ? const struct samsung_keypad_platdata *pdata;
>> @@ -246,7 +381,10 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>> ? ? ? unsigned int keymap_size;
>> ? ? ? int error;
>>
>> - ? ? pdata = pdev->dev.platform_data;
>> + ? ? if (pdev->dev.of_node)
>> + ? ? ? ? ? ? pdata = samsung_keypad_parse_dt(&pdev->dev);
>> + ? ? else
>> + ? ? ? ? ? ? pdata = pdev->dev.platform_data;
>> ? ? ? if (!pdata) {
>> ? ? ? ? ? ? ? dev_err(&pdev->dev, "no platform data defined\n");
>> ? ? ? ? ? ? ? return -EINVAL;
>> @@ -303,6 +441,9 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev)
>> ? ? ? keypad->cols = pdata->cols;
>> ? ? ? init_waitqueue_head(&keypad->wait);
>>
>> + ? ? if (pdev->dev.of_node)
>> + ? ? ? ? ? ? samsung_keypad_parse_dt_gpio(&pdev->dev, keypad);
>> +
>> ? ? ? input_dev->name = pdev->name;
>> ? ? ? input_dev->id.bustype = BUS_HOST;
>> ? ? ? input_dev->dev.parent = &pdev->dev;
>> @@ -349,6 +490,7 @@ err_free_irq:
>> ? ? ? free_irq(keypad->irq, keypad);
>> ?err_put_clk:
>> ? ? ? clk_put(keypad->clk);
>> + ? ? samsung_keypad_dt_gpio_free(keypad);
>> ?err_unmap_base:
>> ? ? ? iounmap(keypad->base);
>> ?err_free_mem:
>> @@ -374,6 +516,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev)
>> ? ? ? free_irq(keypad->irq, keypad);
>>
>> ? ? ? clk_put(keypad->clk);
>> + ? ? samsung_keypad_dt_gpio_free(keypad);
>>
>> ? ? ? iounmap(keypad->base);
>> ? ? ? kfree(keypad);
>> @@ -447,6 +590,17 @@ static const struct dev_pm_ops samsung_keypad_pm_ops = {
>> ?};
>> ?#endif
>>
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id samsung_keypad_dt_match[] = {
>> + ? ? { .compatible = "samsung,s3c6410-keypad" },
>> + ? ? { .compatible = "samsung,s5pv210-keypad" },
>> + ? ? {},
>> +};
>> +MODULE_DEVICE_TABLE(of, samsung_keypad_dt_match);
>> +#else
>> +#define samsung_keypad_dt_match NULL
>> +#endif
>> +
>> ?static struct platform_device_id samsung_keypad_driver_ids[] = {
>> ? ? ? {
>> ? ? ? ? ? ? ? .name ? ? ? ? ? = "samsung-keypad",
>> @@ -465,6 +619,7 @@ static struct platform_driver samsung_keypad_driver = {
>> ? ? ? .driver ? ? ? ? = {
>> ? ? ? ? ? ? ? .name ? = "samsung-keypad",
>> ? ? ? ? ? ? ? .owner ?= THIS_MODULE,
>> + ? ? ? ? ? ? .of_match_table = samsung_keypad_dt_match,
>> ?#ifdef CONFIG_PM
>> ? ? ? ? ? ? ? .pm ? ? = &samsung_keypad_pm_ops,
>> ?#endif
>> --
>> 1.6.6.rc2
>>
>
> Thanks.
>
> --
> Dmitry
>

Thanks for your review and comments.

Regards,
Thomas.