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

[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 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 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 1/2] input: samsung-keypad: Add HAVE_SAMSUNG_KEYPAD config option

[Dmitry Torokhov]

Hi Thomas,

On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
> 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.

I am sorry, I do not follow... What is the difference between
SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.

Thanks,

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

-- 
Dmitry

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

[Dmitry Torokhov]

Hi Thomas,

On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
> 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.

I am sorry, I do not follow... What is the difference between
SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.

Thanks,

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

-- 
Dmitry

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 1/2] input: samsung-keypad: Add HAVE_SAMSUNG_KEYPAD config option

[Dmitry Torokhov]

On Wed, Sep 07, 2011 at 11:22:48AM -0700, Dmitry Torokhov wrote:
> Hi Thomas,
> 
> On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
> > 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.
> 
> I am sorry, I do not follow... What is the difference between
> SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.
> 

BTW, should we do something like below?

Thanks.

-- 
Dmitry

Input: samsung-keypad - enable compiling on other platforms

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

There is nothing in keypad platform definitions that requires
the driver be complied on Samsung platform only, so let's move them
out of the platform subdirectory and relax the dependencies.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 arch/arm/plat-samsung/include/plat/keypad.h |   27 +----------------
 drivers/input/keyboard/Kconfig              |    5 ++-
 drivers/input/keyboard/samsung-keypad.c     |    2 +
 include/linux/input/samsung-keypad.h        |   43 +++++++++++++++++++++++++++
 4 files changed, 48 insertions(+), 29 deletions(-)
 create mode 100644 include/linux/input/samsung-keypad.h


diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
index b59a648..8fddee3 100644
--- a/arch/arm/plat-samsung/include/plat/keypad.h
+++ b/arch/arm/plat-samsung/include/plat/keypad.h
@@ -13,32 +13,7 @@
 #ifndef __PLAT_SAMSUNG_KEYPAD_H
 #define __PLAT_SAMSUNG_KEYPAD_H
 
-#include <linux/input/matrix_keypad.h>
-
-#define SAMSUNG_MAX_ROWS	8
-#define SAMSUNG_MAX_COLS	8
-
-/**
- * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
- * @keymap_data: pointer to &matrix_keymap_data.
- * @rows: number of keypad row supported.
- * @cols: number of keypad col supported.
- * @no_autorepeat: disable key autorepeat.
- * @wakeup: controls whether the device should be set up as wakeup source.
- * @cfg_gpio: configure the GPIO.
- *
- * Initialisation data specific to either the machine or the platform
- * for the device driver to use or call-back when configuring gpio.
- */
-struct samsung_keypad_platdata {
-	const struct matrix_keymap_data	*keymap_data;
-	unsigned int rows;
-	unsigned int cols;
-	bool no_autorepeat;
-	bool wakeup;
-
-	void (*cfg_gpio)(unsigned int rows, unsigned int cols);
-};
+#include <linux/input/samsung_keypad.h>
 
 /**
  * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index de846f8..db1b221 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -434,9 +434,10 @@ config KEYBOARD_PMIC8XXX
 
 config KEYBOARD_SAMSUNG
 	tristate "Samsung keypad support"
-	depends on SAMSUNG_DEV_KEYPAD
+	depends on HAVE_CLK
 	help
-	  Say Y here if you want to use the Samsung keypad.
+	  Say Y here if you want to use the keypad on your Samsung mobile
+	  device.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called samsung-keypad.
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index d244fdf..1a2b755 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -22,7 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
-#include <plat/keypad.h>
+#include <linux/input/samsung-keypad.h>
 
 #define SAMSUNG_KEYIFCON			0x00
 #define SAMSUNG_KEYIFSTSCLR			0x04
diff --git a/include/linux/input/samsung-keypad.h b/include/linux/input/samsung-keypad.h
new file mode 100644
index 0000000..f25619b
--- /dev/null
+++ b/include/linux/input/samsung-keypad.h
@@ -0,0 +1,43 @@
+/*
+ * Samsung Keypad platform data definitions
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#ifndef __SAMSUNG_KEYPAD_H
+#define __SAMSUNG_KEYPAD_H
+
+#include <linux/input/matrix_keypad.h>
+
+#define SAMSUNG_MAX_ROWS	8
+#define SAMSUNG_MAX_COLS	8
+
+/**
+ * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
+ * @keymap_data: pointer to &matrix_keymap_data.
+ * @rows: number of keypad row supported.
+ * @cols: number of keypad col supported.
+ * @no_autorepeat: disable key autorepeat.
+ * @wakeup: controls whether the device should be set up as wakeup source.
+ * @cfg_gpio: configure the GPIO.
+ *
+ * Initialisation data specific to either the machine or the platform
+ * for the device driver to use or call-back when configuring gpio.
+ */
+struct samsung_keypad_platdata {
+	const struct matrix_keymap_data	*keymap_data;
+	unsigned int rows;
+	unsigned int cols;
+	bool no_autorepeat;
+	bool wakeup;
+
+	void (*cfg_gpio)(unsigned int rows, unsigned int cols);
+};
+
+#endif /* __SAMSUNG_KEYPAD_H */

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

[Dmitry Torokhov]

On Wed, Sep 07, 2011 at 11:22:48AM -0700, Dmitry Torokhov wrote:
> Hi Thomas,
> 
> On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
> > 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.
> 
> I am sorry, I do not follow... What is the difference between
> SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.
> 

BTW, should we do something like below?

Thanks.

-- 
Dmitry

Input: samsung-keypad - enable compiling on other platforms

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

There is nothing in keypad platform definitions that requires
the driver be complied on Samsung platform only, so let's move them
out of the platform subdirectory and relax the dependencies.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 arch/arm/plat-samsung/include/plat/keypad.h |   27 +----------------
 drivers/input/keyboard/Kconfig              |    5 ++-
 drivers/input/keyboard/samsung-keypad.c     |    2 +
 include/linux/input/samsung-keypad.h        |   43 +++++++++++++++++++++++++++
 4 files changed, 48 insertions(+), 29 deletions(-)
 create mode 100644 include/linux/input/samsung-keypad.h


diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
index b59a648..8fddee3 100644
--- a/arch/arm/plat-samsung/include/plat/keypad.h
+++ b/arch/arm/plat-samsung/include/plat/keypad.h
@@ -13,32 +13,7 @@
 #ifndef __PLAT_SAMSUNG_KEYPAD_H
 #define __PLAT_SAMSUNG_KEYPAD_H
 
-#include <linux/input/matrix_keypad.h>
-
-#define SAMSUNG_MAX_ROWS	8
-#define SAMSUNG_MAX_COLS	8
-
-/**
- * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
- * @keymap_data: pointer to &matrix_keymap_data.
- * @rows: number of keypad row supported.
- * @cols: number of keypad col supported.
- * @no_autorepeat: disable key autorepeat.
- * @wakeup: controls whether the device should be set up as wakeup source.
- * @cfg_gpio: configure the GPIO.
- *
- * Initialisation data specific to either the machine or the platform
- * for the device driver to use or call-back when configuring gpio.
- */
-struct samsung_keypad_platdata {
-	const struct matrix_keymap_data	*keymap_data;
-	unsigned int rows;
-	unsigned int cols;
-	bool no_autorepeat;
-	bool wakeup;
-
-	void (*cfg_gpio)(unsigned int rows, unsigned int cols);
-};
+#include <linux/input/samsung_keypad.h>
 
 /**
  * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index de846f8..db1b221 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -434,9 +434,10 @@ config KEYBOARD_PMIC8XXX
 
 config KEYBOARD_SAMSUNG
 	tristate "Samsung keypad support"
-	depends on SAMSUNG_DEV_KEYPAD
+	depends on HAVE_CLK
 	help
-	  Say Y here if you want to use the Samsung keypad.
+	  Say Y here if you want to use the keypad on your Samsung mobile
+	  device.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called samsung-keypad.
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index d244fdf..1a2b755 100644
--- a/drivers/input/keyboard/samsung-keypad.c
+++ b/drivers/input/keyboard/samsung-keypad.c
@@ -22,7 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
-#include <plat/keypad.h>
+#include <linux/input/samsung-keypad.h>
 
 #define SAMSUNG_KEYIFCON			0x00
 #define SAMSUNG_KEYIFSTSCLR			0x04
diff --git a/include/linux/input/samsung-keypad.h b/include/linux/input/samsung-keypad.h
new file mode 100644
index 0000000..f25619b
--- /dev/null
+++ b/include/linux/input/samsung-keypad.h
@@ -0,0 +1,43 @@
+/*
+ * Samsung Keypad platform data definitions
+ *
+ * Copyright (C) 2010 Samsung Electronics Co.Ltd
+ * Author: Joonyoung Shim <jy0922.shim@samsung.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#ifndef __SAMSUNG_KEYPAD_H
+#define __SAMSUNG_KEYPAD_H
+
+#include <linux/input/matrix_keypad.h>
+
+#define SAMSUNG_MAX_ROWS	8
+#define SAMSUNG_MAX_COLS	8
+
+/**
+ * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
+ * @keymap_data: pointer to &matrix_keymap_data.
+ * @rows: number of keypad row supported.
+ * @cols: number of keypad col supported.
+ * @no_autorepeat: disable key autorepeat.
+ * @wakeup: controls whether the device should be set up as wakeup source.
+ * @cfg_gpio: configure the GPIO.
+ *
+ * Initialisation data specific to either the machine or the platform
+ * for the device driver to use or call-back when configuring gpio.
+ */
+struct samsung_keypad_platdata {
+	const struct matrix_keymap_data	*keymap_data;
+	unsigned int rows;
+	unsigned int cols;
+	bool no_autorepeat;
+	bool wakeup;
+
+	void (*cfg_gpio)(unsigned int rows, unsigned int cols);
+};
+
+#endif /* __SAMSUNG_KEYPAD_H */

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

[Thomas Abraham]

Hi Dmitry,

On 7 September 2011 23:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Thomas,
>
> On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
>> 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.
>
> I am sorry, I do not follow... What is the difference between
> SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.

The inclusion of platform device instance for keypad (in
arch/arm/plat-samsung/dev-keypad.c) in the build depends on
SAMSUNG_DEV_KEYPAD. The samsung-keypad driver is also dependent on
SAMSUNG_DEV_KEYPAD.

In case of device tree based instantiation of keypad, compilation of
dev-keypad.c file is not required. So SAMSUNG_DEV_KEYPAD config option
will not be selected.In that case, the compilation of the keypad
driver cannot be dependent on SAMSUNG_DEV_KEYPAD. There should be
another option to select the keypad driver and so HAVE_SAMSUNG_KEYPAD
was introduced. HAVE_SAMSUNG_KEYPAD can be selected on platforms that
need the samsung-keypad driver but do no need the keypad platform
device.

Thanks for your review and comments.

Regards,
Thomas.

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

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

[Thomas Abraham]

Hi Dmitry,

On 7 September 2011 23:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Thomas,
>
> On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
>> 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.
>
> I am sorry, I do not follow... What is the difference between
> SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.

The inclusion of platform device instance for keypad (in
arch/arm/plat-samsung/dev-keypad.c) in the build depends on
SAMSUNG_DEV_KEYPAD. The samsung-keypad driver is also dependent on
SAMSUNG_DEV_KEYPAD.

In case of device tree based instantiation of keypad, compilation of
dev-keypad.c file is not required. So SAMSUNG_DEV_KEYPAD config option
will not be selected.In that case, the compilation of the keypad
driver cannot be dependent on SAMSUNG_DEV_KEYPAD. There should be
another option to select the keypad driver and so HAVE_SAMSUNG_KEYPAD
was introduced. HAVE_SAMSUNG_KEYPAD can be selected on platforms that
need the samsung-keypad driver but do no need the keypad platform
device.

Thanks for your review and comments.

Regards,
Thomas.

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

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

[Thomas Abraham]

Hi Dmitry,

On 8 September 2011 03:19, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

[...]

> BTW, should we do something like below?
>
> Thanks.
>
> --
> Dmitry
>
> Input: samsung-keypad - enable compiling on other platforms
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> There is nothing in keypad platform definitions that requires
> the driver be complied on Samsung platform only, so let's move them
> out of the platform subdirectory and relax the dependencies.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
>  arch/arm/plat-samsung/include/plat/keypad.h |   27 +----------------
>  drivers/input/keyboard/Kconfig              |    5 ++-
>  drivers/input/keyboard/samsung-keypad.c     |    2 +
>  include/linux/input/samsung-keypad.h        |   43 +++++++++++++++++++++++++++
>  4 files changed, 48 insertions(+), 29 deletions(-)
>  create mode 100644 include/linux/input/samsung-keypad.h
>
>
> diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
> index b59a648..8fddee3 100644
> --- a/arch/arm/plat-samsung/include/plat/keypad.h
> +++ b/arch/arm/plat-samsung/include/plat/keypad.h
> @@ -13,32 +13,7 @@
>  #ifndef __PLAT_SAMSUNG_KEYPAD_H
>  #define __PLAT_SAMSUNG_KEYPAD_H
>
> -#include <linux/input/matrix_keypad.h>
> -
> -#define SAMSUNG_MAX_ROWS       8
> -#define SAMSUNG_MAX_COLS       8
> -
> -/**
> - * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
> - * @keymap_data: pointer to &matrix_keymap_data.
> - * @rows: number of keypad row supported.
> - * @cols: number of keypad col supported.
> - * @no_autorepeat: disable key autorepeat.
> - * @wakeup: controls whether the device should be set up as wakeup source.
> - * @cfg_gpio: configure the GPIO.
> - *
> - * Initialisation data specific to either the machine or the platform
> - * for the device driver to use or call-back when configuring gpio.
> - */
> -struct samsung_keypad_platdata {
> -       const struct matrix_keymap_data *keymap_data;
> -       unsigned int rows;
> -       unsigned int cols;
> -       bool no_autorepeat;
> -       bool wakeup;
> -
> -       void (*cfg_gpio)(unsigned int rows, unsigned int cols);
> -};
> +#include <linux/input/samsung_keypad.h>
>
>  /**
>  * samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index de846f8..db1b221 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -434,9 +434,10 @@ config KEYBOARD_PMIC8XXX
>
>  config KEYBOARD_SAMSUNG
>        tristate "Samsung keypad support"
> -       depends on SAMSUNG_DEV_KEYPAD
> +       depends on HAVE_CLK
>        help
> -         Say Y here if you want to use the Samsung keypad.
> +         Say Y here if you want to use the keypad on your Samsung mobile
> +         device.

In this case, Samsung Keyboad option would be listed in the available
keyboard list in menuconfig for non-samsung platforms as well. Knowing
that Samsung keypad controller has been used only in Samsung
Application Processor SoC's only (and would be this way for quite
sometime), should we not retain its selection to Samsung AP SoC's
only?

>
>          To compile this driver as a module, choose M here: the
>          module will be called samsung-keypad.
> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index d244fdf..1a2b755 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c
> @@ -22,7 +22,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
> -#include <plat/keypad.h>
> +#include <linux/input/samsung-keypad.h>
>
>  #define SAMSUNG_KEYIFCON                       0x00
>  #define SAMSUNG_KEYIFSTSCLR                    0x04
> diff --git a/include/linux/input/samsung-keypad.h b/include/linux/input/samsung-keypad.h
> new file mode 100644
> index 0000000..f25619b
> --- /dev/null
> +++ b/include/linux/input/samsung-keypad.h
> @@ -0,0 +1,43 @@
> +/*
> + * Samsung Keypad platform data definitions
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim <jy0922.shim@samsung.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#ifndef __SAMSUNG_KEYPAD_H
> +#define __SAMSUNG_KEYPAD_H
> +
> +#include <linux/input/matrix_keypad.h>
> +
> +#define SAMSUNG_MAX_ROWS       8
> +#define SAMSUNG_MAX_COLS       8
> +
> +/**
> + * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
> + * @keymap_data: pointer to &matrix_keymap_data.
> + * @rows: number of keypad row supported.
> + * @cols: number of keypad col supported.
> + * @no_autorepeat: disable key autorepeat.
> + * @wakeup: controls whether the device should be set up as wakeup source.
> + * @cfg_gpio: configure the GPIO.
> + *
> + * Initialisation data specific to either the machine or the platform
> + * for the device driver to use or call-back when configuring gpio.
> + */
> +struct samsung_keypad_platdata {
> +       const struct matrix_keymap_data *keymap_data;
> +       unsigned int rows;
> +       unsigned int cols;
> +       bool no_autorepeat;
> +       bool wakeup;
> +
> +       void (*cfg_gpio)(unsigned int rows, unsigned int cols);
> +};
> +
> +#endif /* __SAMSUNG_KEYPAD_H */
>

The movement of samsung-keypad.h to include/linux/input would be
helpful if it is decided to make samsung-keypad driver available to
non-samsung platforms as well.

Thanks,
Thomas.

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

[Thomas Abraham]

Hi Dmitry,

On 8 September 2011 03:19, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

[...]

> BTW, should we do something like below?
>
> Thanks.
>
> --
> Dmitry
>
> Input: samsung-keypad - enable compiling on other platforms
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> There is nothing in keypad platform definitions that requires
> the driver be complied on Samsung platform only, so let's move them
> out of the platform subdirectory and relax the dependencies.
>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
> ?arch/arm/plat-samsung/include/plat/keypad.h | ? 27 +----------------
> ?drivers/input/keyboard/Kconfig ? ? ? ? ? ? ?| ? ?5 ++-
> ?drivers/input/keyboard/samsung-keypad.c ? ? | ? ?2 +
> ?include/linux/input/samsung-keypad.h ? ? ? ?| ? 43 +++++++++++++++++++++++++++
> ?4 files changed, 48 insertions(+), 29 deletions(-)
> ?create mode 100644 include/linux/input/samsung-keypad.h
>
>
> diff --git a/arch/arm/plat-samsung/include/plat/keypad.h b/arch/arm/plat-samsung/include/plat/keypad.h
> index b59a648..8fddee3 100644
> --- a/arch/arm/plat-samsung/include/plat/keypad.h
> +++ b/arch/arm/plat-samsung/include/plat/keypad.h
> @@ -13,32 +13,7 @@
> ?#ifndef __PLAT_SAMSUNG_KEYPAD_H
> ?#define __PLAT_SAMSUNG_KEYPAD_H
>
> -#include <linux/input/matrix_keypad.h>
> -
> -#define SAMSUNG_MAX_ROWS ? ? ? 8
> -#define SAMSUNG_MAX_COLS ? ? ? 8
> -
> -/**
> - * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
> - * @keymap_data: pointer to &matrix_keymap_data.
> - * @rows: number of keypad row supported.
> - * @cols: number of keypad col supported.
> - * @no_autorepeat: disable key autorepeat.
> - * @wakeup: controls whether the device should be set up as wakeup source.
> - * @cfg_gpio: configure the GPIO.
> - *
> - * Initialisation data specific to either the machine or the platform
> - * for the device driver to use or call-back when configuring gpio.
> - */
> -struct samsung_keypad_platdata {
> - ? ? ? const struct matrix_keymap_data *keymap_data;
> - ? ? ? unsigned int rows;
> - ? ? ? unsigned int cols;
> - ? ? ? bool no_autorepeat;
> - ? ? ? bool wakeup;
> -
> - ? ? ? void (*cfg_gpio)(unsigned int rows, unsigned int cols);
> -};
> +#include <linux/input/samsung_keypad.h>
>
> ?/**
> ?* samsung_keypad_set_platdata - Set platform data for Samsung Keypad device.
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index de846f8..db1b221 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -434,9 +434,10 @@ config KEYBOARD_PMIC8XXX
>
> ?config KEYBOARD_SAMSUNG
> ? ? ? ?tristate "Samsung keypad support"
> - ? ? ? depends on SAMSUNG_DEV_KEYPAD
> + ? ? ? depends on HAVE_CLK
> ? ? ? ?help
> - ? ? ? ? Say Y here if you want to use the Samsung keypad.
> + ? ? ? ? Say Y here if you want to use the keypad on your Samsung mobile
> + ? ? ? ? device.

In this case, Samsung Keyboad option would be listed in the available
keyboard list in menuconfig for non-samsung platforms as well. Knowing
that Samsung keypad controller has been used only in Samsung
Application Processor SoC's only (and would be this way for quite
sometime), should we not retain its selection to Samsung AP SoC's
only?

>
> ? ? ? ? ?To compile this driver as a module, choose M here: the
> ? ? ? ? ?module will be called samsung-keypad.
> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index d244fdf..1a2b755 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c
> @@ -22,7 +22,7 @@
> ?#include <linux/platform_device.h>
> ?#include <linux/slab.h>
> ?#include <linux/sched.h>
> -#include <plat/keypad.h>
> +#include <linux/input/samsung-keypad.h>
>
> ?#define SAMSUNG_KEYIFCON ? ? ? ? ? ? ? ? ? ? ? 0x00
> ?#define SAMSUNG_KEYIFSTSCLR ? ? ? ? ? ? ? ? ? ?0x04
> diff --git a/include/linux/input/samsung-keypad.h b/include/linux/input/samsung-keypad.h
> new file mode 100644
> index 0000000..f25619b
> --- /dev/null
> +++ b/include/linux/input/samsung-keypad.h
> @@ -0,0 +1,43 @@
> +/*
> + * Samsung Keypad platform data definitions
> + *
> + * Copyright (C) 2010 Samsung Electronics Co.Ltd
> + * Author: Joonyoung Shim <jy0922.shim@samsung.com>
> + *
> + * This program is free software; you can redistribute ?it and/or modify it
> + * under ?the terms of ?the GNU General ?Public License as published by the
> + * Free Software Foundation; ?either version 2 of the ?License, or (at your
> + * option) any later version.
> + */
> +
> +#ifndef __SAMSUNG_KEYPAD_H
> +#define __SAMSUNG_KEYPAD_H
> +
> +#include <linux/input/matrix_keypad.h>
> +
> +#define SAMSUNG_MAX_ROWS ? ? ? 8
> +#define SAMSUNG_MAX_COLS ? ? ? 8
> +
> +/**
> + * struct samsung_keypad_platdata - Platform device data for Samsung Keypad.
> + * @keymap_data: pointer to &matrix_keymap_data.
> + * @rows: number of keypad row supported.
> + * @cols: number of keypad col supported.
> + * @no_autorepeat: disable key autorepeat.
> + * @wakeup: controls whether the device should be set up as wakeup source.
> + * @cfg_gpio: configure the GPIO.
> + *
> + * Initialisation data specific to either the machine or the platform
> + * for the device driver to use or call-back when configuring gpio.
> + */
> +struct samsung_keypad_platdata {
> + ? ? ? const struct matrix_keymap_data *keymap_data;
> + ? ? ? unsigned int rows;
> + ? ? ? unsigned int cols;
> + ? ? ? bool no_autorepeat;
> + ? ? ? bool wakeup;
> +
> + ? ? ? void (*cfg_gpio)(unsigned int rows, unsigned int cols);
> +};
> +
> +#endif /* __SAMSUNG_KEYPAD_H */
>

The movement of samsung-keypad.h to include/linux/input would be
helpful if it is decided to make samsung-keypad driver available to
non-samsung platforms as well.

Thanks,
Thomas.

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.

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

[Dmitry Torokhov]

On Thu, Sep 08, 2011 at 09:16:46AM +0530, Thomas Abraham wrote:
> Hi Dmitry,
> 
> On 7 September 2011 23:52, Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Hi Thomas,
> >
> > On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
> >> 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.
> >
> > I am sorry, I do not follow... What is the difference between
> > SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.
> 
> The inclusion of platform device instance for keypad (in
> arch/arm/plat-samsung/dev-keypad.c) in the build depends on
> SAMSUNG_DEV_KEYPAD. The samsung-keypad driver is also dependent on
> SAMSUNG_DEV_KEYPAD.
> 
> In case of device tree based instantiation of keypad, compilation of
> dev-keypad.c file is not required. So SAMSUNG_DEV_KEYPAD config option
> will not be selected.In that case, the compilation of the keypad
> driver cannot be dependent on SAMSUNG_DEV_KEYPAD. There should be
> another option to select the keypad driver and so HAVE_SAMSUNG_KEYPAD
> was introduced. HAVE_SAMSUNG_KEYPAD can be selected on platforms that
> need the samsung-keypad driver but do no need the keypad platform
> device.

I still think that it is an extra option... What about the following
dependencies:

	depends on SAMSUNG_DEV_KEYPAD || OF
	depends on HAVE_CLK

although I think we should relax dependencies like I did in my other
patch. This will extend compile coverage of the driver and lessen the
chances that API changes will cause unintended breakage because noone
but Samsung platform users compile it.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Thu, Sep 08, 2011 at 09:16:46AM +0530, Thomas Abraham wrote:
> Hi Dmitry,
> 
> On 7 September 2011 23:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > Hi Thomas,
> >
> > On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
> >> 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.
> >
> > I am sorry, I do not follow... What is the difference between
> > SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.
> 
> The inclusion of platform device instance for keypad (in
> arch/arm/plat-samsung/dev-keypad.c) in the build depends on
> SAMSUNG_DEV_KEYPAD. The samsung-keypad driver is also dependent on
> SAMSUNG_DEV_KEYPAD.
> 
> In case of device tree based instantiation of keypad, compilation of
> dev-keypad.c file is not required. So SAMSUNG_DEV_KEYPAD config option
> will not be selected.In that case, the compilation of the keypad
> driver cannot be dependent on SAMSUNG_DEV_KEYPAD. There should be
> another option to select the keypad driver and so HAVE_SAMSUNG_KEYPAD
> was introduced. HAVE_SAMSUNG_KEYPAD can be selected on platforms that
> need the samsung-keypad driver but do no need the keypad platform
> device.

I still think that it is an extra option... What about the following
dependencies:

	depends on SAMSUNG_DEV_KEYPAD || OF
	depends on HAVE_CLK

although I think we should relax dependencies like I did in my other
patch. This will extend compile coverage of the driver and lessen the
chances that API changes will cause unintended breakage because noone
but Samsung platform users compile it.

Thanks.

-- 
Dmitry

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

[Thomas Abraham]

Hi Dmitry,

On 8 September 2011 22:03, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Thu, Sep 08, 2011 at 09:16:46AM +0530, Thomas Abraham wrote:
>> Hi Dmitry,
>>
>> On 7 September 2011 23:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> > Hi Thomas,
>> >
>> > On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
>> >> 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.
>> >
>> > I am sorry, I do not follow... What is the difference between
>> > SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.
>>
>> The inclusion of platform device instance for keypad (in
>> arch/arm/plat-samsung/dev-keypad.c) in the build depends on
>> SAMSUNG_DEV_KEYPAD. The samsung-keypad driver is also dependent on
>> SAMSUNG_DEV_KEYPAD.
>>
>> In case of device tree based instantiation of keypad, compilation of
>> dev-keypad.c file is not required. So SAMSUNG_DEV_KEYPAD config option
>> will not be selected.In that case, the compilation of the keypad
>> driver cannot be dependent on SAMSUNG_DEV_KEYPAD. There should be
>> another option to select the keypad driver and so HAVE_SAMSUNG_KEYPAD
>> was introduced. HAVE_SAMSUNG_KEYPAD can be selected on platforms that
>> need the samsung-keypad driver but do no need the keypad platform
>> device.
>
> I still think that it is an extra option... What about the following
> dependencies:
>
>        depends on SAMSUNG_DEV_KEYPAD || OF
>        depends on HAVE_CLK

There could be OF based Samsung platforms that might not need the
keypad controller driver to be compiled. And it could get selected for
non-samsung platforms using OF. So, this change would not provide the
intended dependencies.

>
> although I think we should relax dependencies like I did in my other
> patch. This will extend compile coverage of the driver and lessen the
> chances that API changes will cause unintended breakage because noone
> but Samsung platform users compile it.

The only concern with your patch was that Samsung keypad driver gets
listed even for non-samsung platforms. There is no particular
preference on this. But if you prefer to extend Samsung keypad
controller driver to other platforms as well, and if others agree,
that should be fine.

(Sorry for the delayed reply. I was out of office for three days.)

Thanks,
Thomas.

[...]

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

[Thomas Abraham]

Hi Dmitry,

On 8 September 2011 22:03, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Thu, Sep 08, 2011 at 09:16:46AM +0530, Thomas Abraham wrote:
>> Hi Dmitry,
>>
>> On 7 September 2011 23:52, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> > Hi Thomas,
>> >
>> > On Tue, Sep 06, 2011 at 07:25:16PM +0530, Thomas Abraham wrote:
>> >> 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.
>> >
>> > I am sorry, I do not follow... What is the difference between
>> > SAMSUNG_DEV_KEYPAD and HAVE_SAMSUNG_KEYPAD? They look exactly the same.
>>
>> The inclusion of platform device instance for keypad (in
>> arch/arm/plat-samsung/dev-keypad.c) in the build depends on
>> SAMSUNG_DEV_KEYPAD. The samsung-keypad driver is also dependent on
>> SAMSUNG_DEV_KEYPAD.
>>
>> In case of device tree based instantiation of keypad, compilation of
>> dev-keypad.c file is not required. So SAMSUNG_DEV_KEYPAD config option
>> will not be selected.In that case, the compilation of the keypad
>> driver cannot be dependent on SAMSUNG_DEV_KEYPAD. There should be
>> another option to select the keypad driver and so HAVE_SAMSUNG_KEYPAD
>> was introduced. HAVE_SAMSUNG_KEYPAD can be selected on platforms that
>> need the samsung-keypad driver but do no need the keypad platform
>> device.
>
> I still think that it is an extra option... What about the following
> dependencies:
>
> ? ? ? ?depends on SAMSUNG_DEV_KEYPAD || OF
> ? ? ? ?depends on HAVE_CLK

There could be OF based Samsung platforms that might not need the
keypad controller driver to be compiled. And it could get selected for
non-samsung platforms using OF. So, this change would not provide the
intended dependencies.

>
> although I think we should relax dependencies like I did in my other
> patch. This will extend compile coverage of the driver and lessen the
> chances that API changes will cause unintended breakage because noone
> but Samsung platform users compile it.

The only concern with your patch was that Samsung keypad driver gets
listed even for non-samsung platforms. There is no particular
preference on this. But if you prefer to extend Samsung keypad
controller driver to other platforms as well, and if others agree,
that should be fine.

(Sorry for the delayed reply. I was out of office for three days.)

Thanks,
Thomas.

[...]

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.

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]

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

[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