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

[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

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

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

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

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

[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

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

[...]