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

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

[Thomas Abraham]

Changes since v2:
- Addressed comments from Grant Likely.
  - Renamed 'keypad,key-code' property name to 'linux,code'.
  - Fixed incorrect data types for all instances of of_property+read_u32.
  - linux,input-type binding was not added as suggested since the driver
    supports only EV_KEY event type.

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


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

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

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

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

[Thomas Abraham]

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

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

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

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

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

[Grant Likely]

On Mon, Sep 19, 2011 at 03:49:12PM +0530, Thomas Abraham wrote:
> For platforms using device tree, the static keypad device instances
> are not required and SAMSUNG_DEV_KEYPAD is not selected. Since,
> samsung keypad driver has dependency on SAMSUNG_DEV_KEYPAD config
> option, the driver is left out of the compilation for dt enabled
> platforms.
> 
> An additional config option 'HAVE_SAMSUNG_KEYPAD' is added
> which the device tree based platforms can select. This config
> option is added as an alternative dependency for keypad driver.
> 
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>  drivers/input/keyboard/Kconfig |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index b4dee9d..7c322a3 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -423,9 +423,16 @@ config KEYBOARD_PMIC8XXX
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called pmic8xxx-keypad.
>  
> +config HAVE_SAMSUNG_KEYPAD
> +	bool
> +	help
> +	  This will include Samsung Keypad controller driver support. If you
> +	  want to include Samsung Keypad support for any machine, kindly
> +	  select this in the respective mach-xxxx/Kconfig file.
> +
>  config KEYBOARD_SAMSUNG
>  	tristate "Samsung keypad support"
> -	depends on SAMSUNG_DEV_KEYPAD
> +	depends on SAMSUNG_DEV_KEYPAD || HAVE_SAMSUNG_KEYPAD

It would be better to modify SAMSUNG_DEV_KEYPAD to select
HAVE_SAMSUNG_KEYPAD and then make KEYBOARD_SAMSUNG only depend on
HAVE_SAMSUNG_KEYPAD instead of both.

g.

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

[Thomas Abraham]

Hi Grant,

On 22 September 2011 23:05, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Sep 19, 2011 at 03:49:12PM +0530, Thomas Abraham wrote:
>> For platforms using device tree, the static keypad device instances
>> are not required and SAMSUNG_DEV_KEYPAD is not selected. Since,
>> samsung keypad driver has dependency on SAMSUNG_DEV_KEYPAD config
>> option, the driver is left out of the compilation for dt enabled
>> platforms.
>>
>> An additional config option 'HAVE_SAMSUNG_KEYPAD' is added
>> which the device tree based platforms can select. This config
>> option is added as an alternative dependency for keypad driver.
>>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>  drivers/input/keyboard/Kconfig |    9 ++++++++-
>>  1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>> index b4dee9d..7c322a3 100644
>> --- a/drivers/input/keyboard/Kconfig
>> +++ b/drivers/input/keyboard/Kconfig
>> @@ -423,9 +423,16 @@ config KEYBOARD_PMIC8XXX
>>         To compile this driver as a module, choose M here: the module will
>>         be called pmic8xxx-keypad.
>>
>> +config HAVE_SAMSUNG_KEYPAD
>> +     bool
>> +     help
>> +       This will include Samsung Keypad controller driver support. If you
>> +       want to include Samsung Keypad support for any machine, kindly
>> +       select this in the respective mach-xxxx/Kconfig file.
>> +
>>  config KEYBOARD_SAMSUNG
>>       tristate "Samsung keypad support"
>> -     depends on SAMSUNG_DEV_KEYPAD
>> +     depends on SAMSUNG_DEV_KEYPAD || HAVE_SAMSUNG_KEYPAD
>
> It would be better to modify SAMSUNG_DEV_KEYPAD to select
> HAVE_SAMSUNG_KEYPAD and then make KEYBOARD_SAMSUNG only depend on
> HAVE_SAMSUNG_KEYPAD instead of both.

The SAMSUNG_DEV_KEYPAD is not selected for device tree enabled
machine. For instance, the Kconfig section for Exynos4 dt-enabled
machine file is as below.

config MACH_EXYNOS4_DT
        bool "Samsung Exynos4 Machine using device tree"
        select CPU_EXYNOS4210
        select USE_OF
        select HAVE_SAMSUNG_KEYPAD if INPUT_KEYBOARD
        help
          Machine support for Samsung Exynos4 machine with device tree enabled.

SAMSUNG_DEV_KEYPAD (that selects the static platform device for
keypad) should not be selected for MACH_EXYNOS4_DT config option. So
selecting HAVE_SAMSUNG_KEYPAD in SAMSUNG_DEV_KEYPAD config option
would not help in this case. So for now, this is kept unchanged.
Please let me know if you think this needs to be changed.

Thanks for your review. I am sorry for delayed reply to your email.

Regards,
Thomas.

>
> g.
>
>

[PATCH v3 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            |  179 ++++++++++++++++++--
 2 files changed, 255 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..ce3e394
--- /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.
+  - linux,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>;
+			linux,code = <2>;
+		};
+
+		key_2 {
+			keypad,row = <0>;
+			keypad,column = <4>;
+			linux,code = <3>;
+		};
+
+		key_3 {
+			keypad,row = <0>;
+			keypad,column = <5>;
+			linux,code = <4>;
+		};
+	};
diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
index f689f49..2a477bc 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,131 @@ 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, num_rows, num_cols;
+	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", &num_rows);
+	of_property_read_u32(np, "samsung,keypad-num-columns", &num_cols);
+	if (!num_rows || !num_cols) {
+		dev_err(dev, "number of keypad rows/columns not specified\n");
+		return NULL;
+	}
+	pdata->rows = num_rows;
+	pdata->cols = num_cols;
+
+	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) {
+		u32 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, "linux,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 +368,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 +428,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 +478,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 +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 v3 2/2] input: samsung-keypad: Add device tree support

[Grant Likely]

On Mon, Sep 19, 2011 at 03:49:13PM +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>

A few things to fix below, but you can add my acked-by when you've
addressed them.

> ---
>  .../devicetree/bindings/input/samsung-keypad.txt   |   88 ++++++++++
>  drivers/input/keyboard/samsung-keypad.c            |  179 ++++++++++++++++++--
>  2 files changed, 255 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..ce3e394
> --- /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.
> +  - linux,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>;
> +			linux,code = <2>;
> +		};
> +
> +		key_2 {
> +			keypad,row = <0>;
> +			keypad,column = <4>;
> +			linux,code = <3>;
> +		};
> +
> +		key_3 {
> +			keypad,row = <0>;
> +			keypad,column = <5>;
> +			linux,code = <4>;
> +		};
> +	};

Binding looks okay to me.

> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index f689f49..2a477bc 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,131 @@ 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, num_rows, num_cols;
> +	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", &num_rows);
> +	of_property_read_u32(np, "samsung,keypad-num-columns", &num_cols);

property_read doesn't touch the values of num_rows or num_cols on
failure.  The values need to be initialized if you're going to test
the result value.

> +	if (!num_rows || !num_cols) {
> +		dev_err(dev, "number of keypad rows/columns not specified\n");
> +		return NULL;
> +	}
> +	pdata->rows = num_rows;
> +	pdata->cols = num_cols;
> +
> +	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) {
> +		u32 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, "linux,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 +368,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 +428,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

This still looks odd.  If you move the #ifdef up one line, then you
can remove the empty version of samsung_keypad_parse_dt_gpio(), and
this block won't look so weird.

> +	} 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 +478,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 +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 v3 2/2] input: samsung-keypad: Add device tree support

[Thomas Abraham]

Hi Grant,

On 22 September 2011 23:10, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Sep 19, 2011 at 03:49:13PM +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>
>
> A few things to fix below, but you can add my acked-by when you've
> addressed them.

Ok. Thanks for your review. I will fix as per your comments.

[...]

>
>> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
>> index f689f49..2a477bc 100644
>> --- a/drivers/input/keyboard/samsung-keypad.c
>> +++ b/drivers/input/keyboard/samsung-keypad.c

[...]

>> +
>> +     of_property_read_u32(np, "samsung,keypad-num-rows", &num_rows);
>> +     of_property_read_u32(np, "samsung,keypad-num-columns", &num_cols);
>
> property_read doesn't touch the values of num_rows or num_cols on
> failure.  The values need to be initialized if you're going to test
> the result value.

Ok. num_rows and num_cols variables will be set to zero before the
property read call.

>
>> +     if (!num_rows || !num_cols) {
>> +             dev_err(dev, "number of keypad rows/columns not specified\n");
>> +             return NULL;
>> +     }

[...]

>>
>> +     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
>
> This still looks odd.  If you move the #ifdef up one line, then you
> can remove the empty version of samsung_keypad_parse_dt_gpio(), and
> this block won't look so weird.
>

That would be a better approach. Thanks for the suggestion.

Regards,
Thomas.

[...]