[PATCH V3 0/2] Input: Add matrix_keypad_of_build_keymap()

[PATCH V3 0/2] Input: Add matrix_keypad_of_build_keymap()

[Viresh Kumar]

This patchset adds matrix_keypad_of_build_keymap() routine for building keymap
directly from device tree.

V2->V3:
- Preference given to platform data over device tree in probe routine of
  drivers.
- Declaration of matrix_keypad_of_build_keymap() routine is changed.
- Range/Overflow checking is done on keys and column.

V1->V2:
- Introduced matrix_keypad_of_build_keymap() and removed fill and free keymap
  routines.
- Updated tegra-kbc.

Viresh Kumar (2):
  Input: of_keymap: Introduce matrix_keypad_of_build_keymap()
  Input: spear-keyboard: add device tree bindings

 .../devicetree/bindings/input/spear-keyboard.txt   |   21 +++++
 drivers/input/keyboard/Kconfig                     |    1 +
 drivers/input/keyboard/spear-keyboard.c            |   86 +++++++++++++++---
 drivers/input/keyboard/tegra-kbc.c                 |   48 +++++-----
 drivers/input/of_keymap.c                          |   94 +++++++++++---------
 include/linux/input/matrix_keypad.h                |   16 +---
 6 files changed, 180 insertions(+), 86 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/spear-keyboard.txt

-- 
1.7.10.rc2.10.gb47606

[PATCH V3 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

We don't need to allocate memory for keymap in matrix_keyboard_of_fill_keymap(),
as this would only be used by matrix_keyboard_of_free_keymap(). Instead create
another routine matrix_keypad_of_build_keymap() which reads directly the
property from struct device_node and builds keymap.

With this eariler routines matrix_keyboard_of_fill_keymap() and
matrix_keyboard_of_free_keymap() go away.

This patch also fixes tegra driver according to these changes.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/input/keyboard/tegra-kbc.c  |   48 ++++++++++--------
 drivers/input/of_keymap.c           |   94 ++++++++++++++++++++---------------
 include/linux/input/matrix_keypad.h |   16 ++----
 3 files changed, 85 insertions(+), 73 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 21c42f8..04967e0 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -619,11 +619,10 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
 }
 
 #ifdef CONFIG_OF
-static struct tegra_kbc_platform_data * __devinit
-tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
+static struct tegra_kbc_platform_data *
+__devinit tegra_kbc_dt_parse_pdata(struct device_node *np)
 {
 	struct tegra_kbc_platform_data *pdata;
-	struct device_node *np = pdev->dev.of_node;
 	u32 prop;
 	int i;
 
@@ -659,15 +658,11 @@ tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
 		pdata->pin_cfg[KBC_MAX_ROW + i].type = PIN_CFG_COL;
 	}
 
-	pdata->keymap_data = matrix_keyboard_of_fill_keymap(np, "linux,keymap");
-
-	/* FIXME: Add handling of linux,fn-keymap here */
-
 	return pdata;
 }
 #else
-static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
-	struct platform_device *pdev)
+static struct tegra_kbc_platform_data *
+__devinit tegra_kbc_dt_parse_pdata(struct device_node *np)
 {
 	return NULL;
 }
@@ -676,7 +671,7 @@ static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
 static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 {
 	const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
-	const struct matrix_keymap_data *keymap_data;
+	struct device_node *np = NULL;
 	struct tegra_kbc *kbc;
 	struct input_dev *input_dev;
 	struct resource *res;
@@ -686,8 +681,10 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	unsigned int debounce_cnt;
 	unsigned int scan_time_rows;
 
-	if (!pdata)
-		pdata = tegra_kbc_dt_parse_pdata(pdev);
+	if (!pdata) {
+		np = pdev->dev.of_node;
+		pdata = tegra_kbc_dt_parse_pdata(np);
+	}
 
 	if (!pdata)
 		return -EINVAL;
@@ -775,9 +772,23 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 
 	kbc->use_fn_map = pdata->use_fn_map;
 	kbc->use_ghost_filter = pdata->use_ghost_filter;
-	keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
-	matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
-				   input_dev->keycode, input_dev->keybit);
+
+	if (np) {
+		/* FIXME: Add handling of linux,fn-keymap here */
+		err = matrix_keypad_of_build_keymap(input_dev, KBC_ROW_SHIFT,
+				"linux,keymap");
+		if (err) {
+			dev_err(&pdev->dev, "OF: failed to build keymap\n");
+			goto err_put_clk;
+		}
+	} else {
+		const struct matrix_keymap_data *keymap_data =
+			pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
+
+		matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
+				input_dev->keycode, input_dev->keybit);
+	}
+
 	kbc->wakeup_key = pdata->wakeup_key;
 
 	err = request_irq(kbc->irq, tegra_kbc_isr,
@@ -798,9 +809,6 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, kbc);
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 
-	if (!pdev->dev.platform_data)
-		matrix_keyboard_of_free_keymap(pdata->keymap_data);
-
 	return 0;
 
 err_free_irq:
@@ -815,10 +823,8 @@ err_free_mem:
 	input_free_device(input_dev);
 	kfree(kbc);
 err_free_pdata:
-	if (!pdev->dev.platform_data) {
-		matrix_keyboard_of_free_keymap(pdata->keymap_data);
+	if (!pdev->dev.platform_data)
 		kfree(pdata);
-	}
 
 	return err;
 }
diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
index 061493d..ff667ae 100644
--- a/drivers/input/of_keymap.c
+++ b/drivers/input/of_keymap.c
@@ -17,6 +17,7 @@
  *
  */
 
+#include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/input.h>
@@ -26,62 +27,75 @@
 #include <linux/gfp.h>
 #include <linux/slab.h>
 
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np,
-			       const char *propname)
+/**
+ * matrix_keypad_of_build_keymap - convert platform DT keymap into matrix keymap
+ * @idev: pointer to struct input_dev; used for getting keycode, keybit and
+ * keycodemax.
+ * @row_shift: number of bits to shift row value by to advance to the next
+ * line in the keymap
+ * @propname: Device Tree property name to be used for reading keymap. If passed
+ * as NULL, "linux,keymap" is used.
+ *
+ * This function creates an array of keycodes, by reading propname property from
+ * device tree passed, that is suitable for using in a standard matrix keyboard
+ * driver that uses row and col as indices.
+ *
+ * Expectation from user driver: idev must be initialized with following fields:
+ * dev.parent, keycode, keybit and keycodemax.
+ */
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname)
 {
-	struct matrix_keymap_data *kd;
-	u32 *keymap;
-	int proplen, i;
+	struct device *dev = idev->dev.parent;
+	struct device_node *np = dev->of_node;
 	const __be32 *prop;
+	unsigned int proplen, i, size, col_range = 1 << row_shift;
+	unsigned short *keycode;
 
-	if (!np)
-		return NULL;
+	if (!np || !idev)
+		return -ENODEV;
 
 	if (!propname)
 		propname = "linux,keymap";
 
 	prop = of_get_property(np, propname, &proplen);
-	if (!prop)
-		return NULL;
+	if (!prop) {
+		dev_err(dev, "OF: %s property not defined in %s\n", propname,
+				np->full_name);
+		return -ENODEV;
+	}
 
 	if (proplen % sizeof(u32)) {
-		pr_warn("Malformed keymap property %s in %s\n",
-			propname, np->full_name);
-		return NULL;
+		dev_warn(dev, "Malformed keycode property %s in %s\n", propname,
+				np->full_name);
+		return -EINVAL;
 	}
 
-	kd = kzalloc(sizeof(*kd), GFP_KERNEL);
-	if (!kd)
-		return NULL;
-
-	kd->keymap = keymap = kzalloc(proplen, GFP_KERNEL);
-	if (!kd->keymap) {
-		kfree(kd);
-		return NULL;
+	size = proplen / sizeof(u32);
+	if (size > idev->keycodemax) {
+		dev_err(dev, "OF: %s overflow\n", propname);
+		return -EINVAL;
 	}
 
-	kd->keymap_size = proplen / sizeof(u32);
+	keycode = idev->keycode;
 
-	for (i = 0; i < kd->keymap_size; i++) {
-		u32 tmp = be32_to_cpup(prop + i);
-		int key_code, row, col;
+	for (i = 0; i < size; i++) {
+		unsigned int key = be32_to_cpup(prop + i);
+		unsigned int row = KEY_ROW(key);
+		unsigned int col = KEY_COL(key);
+		unsigned short code = KEY_VAL(key);
 
-		row = (tmp >> 24) & 0xff;
-		col = (tmp >> 16) & 0xff;
-		key_code = tmp & 0xffff;
-		keymap[i] = KEY(row, col, key_code);
-	}
-
-	return kd;
-}
-EXPORT_SYMBOL_GPL(matrix_keyboard_of_fill_keymap);
+		if (col >= col_range) {
+			dev_err(dev, "OF: %s: column %x overflowed its range %d\n",
+					propname, col, col_range);
+			return -EINVAL;
+		}
 
-void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-	if (kd) {
-		kfree(kd->keymap);
-		kfree(kd);
+		keycode[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
+		__set_bit(code, idev->keybit);
 	}
+	__clear_bit(KEY_RESERVED, idev->keybit);
+
+	return 0;
 }
-EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
+EXPORT_SYMBOL_GPL(matrix_keypad_of_build_keymap);
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index 6c07ced..341aca1 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -108,21 +108,13 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 }
 
 #ifdef CONFIG_INPUT_OF_MATRIX_KEYMAP
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname);
-
-void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd);
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname);
 #else
-static inline struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname)
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname)
 {
 	return NULL;
 }
-
-static inline void
-matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-}
 #endif
-
 #endif /* _MATRIX_KEYPAD_H */
-- 
1.7.10.rc2.10.gb47606

[PATCH V3 2/2] Input: spear-keyboard: add device tree bindings

[Viresh Kumar]

This adds simple DT bindings for spear-keyboard controller.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 .../devicetree/bindings/input/spear-keyboard.txt   |   21 +++++
 drivers/input/keyboard/Kconfig                     |    1 +
 drivers/input/keyboard/spear-keyboard.c            |   86 +++++++++++++++++---
 3 files changed, 95 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/spear-keyboard.txt

diff --git a/Documentation/devicetree/bindings/input/spear-keyboard.txt b/Documentation/devicetree/bindings/input/spear-keyboard.txt
new file mode 100644
index 0000000..e1bcf13
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/spear-keyboard.txt
@@ -0,0 +1,21 @@
+* SPEAr keyboard controller
+
+Required properties:
+- compatible: "st,spear300-kbd"
+
+Optional properties, in addition to those specified by the shared
+matrix-keyboard bindings:
+- autorepeat: bool: enables key autorepeat
+- st,mode: keyboard mode: 0 - 9x9, 1 - 6x6, 2 - 2x2
+
+Example:
+
+kbd@fc400000 {
+	compatible = "st,spear300-kbd";
+	reg = <0xfc400000 0x100>;
+	linux,keymap = < 0x00030012
+			 0x0102003a >;
+	autorepeat;
+	st,mode = <0>;
+};
+
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index f354813..6da2b02 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -522,6 +522,7 @@ config KEYBOARD_OMAP4
 config KEYBOARD_SPEAR
 	tristate "ST SPEAR keyboard support"
 	depends on PLAT_SPEAR
+	select INPUT_OF_MATRIX_KEYMAP if USE_OF
 	help
 	  Say Y here if you want to use the SPEAR keyboard.
 
diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 3b6b528..fd1b38e 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -19,6 +19,7 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_wakeup.h>
 #include <linux/slab.h>
@@ -136,25 +137,66 @@ static void spear_kbd_close(struct input_dev *dev)
 	kbd->last_key = KEY_RESERVED;
 }
 
+#ifdef CONFIG_OF
+static int __devinit spear_kbd_probe_dt(struct platform_device *pdev)
+{
+	struct kbd_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct device_node *np = pdev->dev.of_node;
+	u32 val;
+
+	if (of_property_read_bool(np, "autorepeat"))
+		pdata->rep = true;
+
+	if (!of_property_read_u32(np, "st,mode", &val)) {
+		pdata->mode = val;
+	} else {
+		dev_err(&pdev->dev, "DT: Invalid mode\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+#else
+static int __devinit spear_kbd_probe_dt(struct platform_device *pdev)
+{
+	return -ENOSYS;
+}
+#endif
+
 static int __devinit spear_kbd_probe(struct platform_device *pdev)
 {
-	const struct kbd_platform_data *pdata = pdev->dev.platform_data;
-	const struct matrix_keymap_data *keymap;
+	struct kbd_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct device_node *np = NULL;
 	struct spear_kbd *kbd;
 	struct input_dev *input_dev;
 	struct resource *res;
 	int irq;
 	int error;
 
-	if (!pdata) {
-		dev_err(&pdev->dev, "Invalid platform data\n");
-		return -EINVAL;
-	}
-
-	keymap = pdata->keymap;
-	if (!keymap) {
-		dev_err(&pdev->dev, "no keymap defined\n");
-		return -EINVAL;
+	if (pdata) {
+		if (!pdata->keymap) {
+			dev_err(&pdev->dev, "Invalid platform data\n");
+			return -EINVAL;
+		}
+	} else {
+		np = pdev->dev.of_node;
+		if (!np) {
+			dev_err(&pdev->dev, "Failed: Neither DT nor Pdata is passed\n");
+			return -EINVAL;
+		}
+
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata) {
+			dev_err(&pdev->dev, "DT: kzalloc failed\n");
+			return -ENOMEM;
+		}
+
+		pdev->dev.platform_data = pdata;
+		error = spear_kbd_probe_dt(pdev);
+		if (error) {
+			dev_err(&pdev->dev, "DT: no platform data\n");
+			return error;
+		}
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -221,8 +263,17 @@ static int __devinit spear_kbd_probe(struct platform_device *pdev)
 	input_dev->keycodesize = sizeof(kbd->keycodes[0]);
 	input_dev->keycodemax = ARRAY_SIZE(kbd->keycodes);
 
-	matrix_keypad_build_keymap(keymap, ROW_SHIFT,
-			input_dev->keycode, input_dev->keybit);
+	if (np) {
+		error = matrix_keypad_of_build_keymap(input_dev, ROW_SHIFT,
+				"linux,keymap");
+		if (error) {
+			dev_err(&pdev->dev, "OF: failed to build keymap\n");
+			goto err_put_clk;
+		}
+	} else {
+		matrix_keypad_build_keymap(pdata->keymap, ROW_SHIFT,
+				input_dev->keycode, input_dev->keybit);
+	}
 
 	input_set_drvdata(input_dev, kbd);
 
@@ -317,6 +368,14 @@ static int spear_kbd_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
 
+#ifdef CONFIG_OF
+static const struct of_device_id spear_kbd_id_table[] = {
+	{ .compatible = "st,spear300-kbd" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, spear_kbd_id_table);
+#endif
+
 static struct platform_driver spear_kbd_driver = {
 	.probe		= spear_kbd_probe,
 	.remove		= __devexit_p(spear_kbd_remove),
@@ -324,6 +383,7 @@ static struct platform_driver spear_kbd_driver = {
 		.name	= "keyboard",
 		.owner	= THIS_MODULE,
 		.pm	= &spear_kbd_pm_ops,
+		.of_match_table = of_match_ptr(spear_kbd_id_table),
 	},
 };
 module_platform_driver(spear_kbd_driver);
-- 
1.7.10.rc2.10.gb47606

Re: [PATCH V3 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Stephen Warren]

On 03/29/2012 02:33 AM, Viresh Kumar wrote:
> We don't need to allocate memory for keymap in matrix_keyboard_of_fill_keymap(),
> as this would only be used by matrix_keyboard_of_free_keymap(). Instead create
> another routine matrix_keypad_of_build_keymap() which reads directly the
> property from struct device_node and builds keymap.
> 
> With this eariler routines matrix_keyboard_of_fill_keymap() and
> matrix_keyboard_of_free_keymap() go away.
> 
> This patch also fixes tegra driver according to these changes.

> diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c

> -static struct tegra_kbc_platform_data * __devinit
> -tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
> +static struct tegra_kbc_platform_data *
> +__devinit tegra_kbc_dt_parse_pdata(struct device_node *np)

I'd be tempted to keep __devinit on the first line, but not a big deal.

> +int matrix_keypad_of_build_keymap(struct input_dev *idev,
> +		unsigned int row_shift, const char *propname)

> +	size = proplen / sizeof(u32);
> +	if (size > idev->keycodemax) {
> +		dev_err(dev, "OF: %s overflow\n", propname);
> +		return -EINVAL;
>  	}

That is checking the number of entries in the property, not the values
of the MATRIX_SCAN_CODE values derived from those entries. I'd say just
remove this check. See below.

> +	keycode = idev->keycode;
>  
> +	for (i = 0; i < size; i++) {
> +		unsigned int key = be32_to_cpup(prop + i);
> +		unsigned int row = KEY_ROW(key);
> +		unsigned int col = KEY_COL(key);
> +		unsigned short code = KEY_VAL(key);
>  
> +		if (col >= col_range) {
> +			dev_err(dev, "OF: %s: column %x overflowed its range %d\n",
> +					propname, col, col_range);
> +			return -EINVAL;
> +		}

That check is good.

> +		keycode[MATRIX_SCAN_CODE(row, col, row_shift)] = code;

But, you also need to do something like:

scancode = MATRIX_SCAN_CODE(row, col, row_shift);
if (scancode >= idev->keycodemax) {
    error out;
}
keycode[scancode] = code;

> +		__set_bit(code, idev->keybit);
>  	}
> +	__clear_bit(KEY_RESERVED, idev->keybit);
> +
> +	return 0;
>  }

Re: [PATCH V3 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[viresh kumar]

On 3/29/12, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/29/2012 02:33 AM, Viresh Kumar wrote:
>> +static struct tegra_kbc_platform_data *
>> +__devinit tegra_kbc_dt_parse_pdata(struct device_node *np)
>
> I'd be tempted to keep __devinit on the first line, but not a big deal.

I need to send V4. So will do this too. :)

>> +int matrix_keypad_of_build_keymap(struct input_dev *idev,
>> +		unsigned int row_shift, const char *propname)
>
>> +	size = proplen / sizeof(u32);
>> +	if (size > idev->keycodemax) {
>> +		dev_err(dev, "OF: %s overflow\n", propname);
>> +		return -EINVAL;
>>  	}
>
> That is checking the number of entries in the property, not the values
> of the MATRIX_SCAN_CODE values derived from those entries. I'd say just
> remove this check. See below.

Absolutely correct.

--
Viresh

[PATCH v4 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

We don't need to allocate memory for keymap in matrix_keyboard_of_fill_keymap(),
as this would only be used by matrix_keyboard_of_free_keymap(). Instead create
another routine matrix_keypad_of_build_keymap() which reads directly the
property from struct device_node and builds keymap.

With this eariler routines matrix_keyboard_of_fill_keymap() and
matrix_keyboard_of_free_keymap() go away.

This patch also fixes tegra driver according to these changes.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
V4:
- check return value of MATRIX_SCAN_CODE() to guarantee that it doesn't overflow
  keycodemax sized buffer.

 drivers/input/keyboard/tegra-kbc.c  |   48 ++++++++++--------
 drivers/input/of_keymap.c           |   98 ++++++++++++++++++++--------------
 include/linux/input/matrix_keypad.h |   16 ++----
 3 files changed, 89 insertions(+), 73 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 21c42f8..04967e0 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -619,11 +619,10 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
 }
 
 #ifdef CONFIG_OF
-static struct tegra_kbc_platform_data * __devinit
-tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
+static struct tegra_kbc_platform_data *
+__devinit tegra_kbc_dt_parse_pdata(struct device_node *np)
 {
 	struct tegra_kbc_platform_data *pdata;
-	struct device_node *np = pdev->dev.of_node;
 	u32 prop;
 	int i;
 
@@ -659,15 +658,11 @@ tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
 		pdata->pin_cfg[KBC_MAX_ROW + i].type = PIN_CFG_COL;
 	}
 
-	pdata->keymap_data = matrix_keyboard_of_fill_keymap(np, "linux,keymap");
-
-	/* FIXME: Add handling of linux,fn-keymap here */
-
 	return pdata;
 }
 #else
-static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
-	struct platform_device *pdev)
+static struct tegra_kbc_platform_data *
+__devinit tegra_kbc_dt_parse_pdata(struct device_node *np)
 {
 	return NULL;
 }
@@ -676,7 +671,7 @@ static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
 static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 {
 	const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
-	const struct matrix_keymap_data *keymap_data;
+	struct device_node *np = NULL;
 	struct tegra_kbc *kbc;
 	struct input_dev *input_dev;
 	struct resource *res;
@@ -686,8 +681,10 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	unsigned int debounce_cnt;
 	unsigned int scan_time_rows;
 
-	if (!pdata)
-		pdata = tegra_kbc_dt_parse_pdata(pdev);
+	if (!pdata) {
+		np = pdev->dev.of_node;
+		pdata = tegra_kbc_dt_parse_pdata(np);
+	}
 
 	if (!pdata)
 		return -EINVAL;
@@ -775,9 +772,23 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 
 	kbc->use_fn_map = pdata->use_fn_map;
 	kbc->use_ghost_filter = pdata->use_ghost_filter;
-	keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
-	matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
-				   input_dev->keycode, input_dev->keybit);
+
+	if (np) {
+		/* FIXME: Add handling of linux,fn-keymap here */
+		err = matrix_keypad_of_build_keymap(input_dev, KBC_ROW_SHIFT,
+				"linux,keymap");
+		if (err) {
+			dev_err(&pdev->dev, "OF: failed to build keymap\n");
+			goto err_put_clk;
+		}
+	} else {
+		const struct matrix_keymap_data *keymap_data =
+			pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
+
+		matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
+				input_dev->keycode, input_dev->keybit);
+	}
+
 	kbc->wakeup_key = pdata->wakeup_key;
 
 	err = request_irq(kbc->irq, tegra_kbc_isr,
@@ -798,9 +809,6 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, kbc);
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 
-	if (!pdev->dev.platform_data)
-		matrix_keyboard_of_free_keymap(pdata->keymap_data);
-
 	return 0;
 
 err_free_irq:
@@ -815,10 +823,8 @@ err_free_mem:
 	input_free_device(input_dev);
 	kfree(kbc);
 err_free_pdata:
-	if (!pdev->dev.platform_data) {
-		matrix_keyboard_of_free_keymap(pdata->keymap_data);
+	if (!pdev->dev.platform_data)
 		kfree(pdata);
-	}
 
 	return err;
 }
diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
index 061493d..631054f 100644
--- a/drivers/input/of_keymap.c
+++ b/drivers/input/of_keymap.c
@@ -17,6 +17,7 @@
  *
  */
 
+#include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/input.h>
@@ -26,62 +27,79 @@
 #include <linux/gfp.h>
 #include <linux/slab.h>
 
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np,
-			       const char *propname)
+/**
+ * matrix_keypad_of_build_keymap - convert platform DT keymap into matrix keymap
+ * @idev: pointer to struct input_dev; used for getting keycode, keybit and
+ * keycodemax.
+ * @row_shift: number of bits to shift row value by to advance to the next
+ * line in the keymap
+ * @propname: Device Tree property name to be used for reading keymap. If passed
+ * as NULL, "linux,keymap" is used.
+ *
+ * This function creates an array of keycodes, by reading propname property from
+ * device tree passed, that is suitable for using in a standard matrix keyboard
+ * driver that uses row and col as indices.
+ *
+ * Expectation from user driver: idev must be initialized with following fields:
+ * dev.parent, keycode, keybit and keycodemax.
+ */
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname)
 {
-	struct matrix_keymap_data *kd;
-	u32 *keymap;
-	int proplen, i;
+	struct device *dev = idev->dev.parent;
+	struct device_node *np = dev->of_node;
+	unsigned short *keycode;
 	const __be32 *prop;
+	unsigned int proplen, i, size, col_range = 1 << row_shift, index;
 
-	if (!np)
-		return NULL;
+	if (!np || !idev)
+		return -ENODEV;
 
 	if (!propname)
 		propname = "linux,keymap";
 
 	prop = of_get_property(np, propname, &proplen);
-	if (!prop)
-		return NULL;
+	if (!prop) {
+		dev_err(dev, "OF: %s property not defined in %s\n", propname,
+				np->full_name);
+		return -ENODEV;
+	}
 
 	if (proplen % sizeof(u32)) {
-		pr_warn("Malformed keymap property %s in %s\n",
-			propname, np->full_name);
-		return NULL;
+		dev_warn(dev, "Malformed keycode property %s in %s\n", propname,
+				np->full_name);
+		return -EINVAL;
 	}
 
-	kd = kzalloc(sizeof(*kd), GFP_KERNEL);
-	if (!kd)
-		return NULL;
-
-	kd->keymap = keymap = kzalloc(proplen, GFP_KERNEL);
-	if (!kd->keymap) {
-		kfree(kd);
-		return NULL;
+	size = proplen / sizeof(u32);
+	if (size > idev->keycodemax) {
+		dev_err(dev, "OF: %s size overflow\n", propname);
+		return -EINVAL;
 	}
 
-	kd->keymap_size = proplen / sizeof(u32);
+	keycode = idev->keycode;
+	for (i = 0; i < size; i++) {
+		unsigned int key = be32_to_cpup(prop + i);
+		unsigned int row = KEY_ROW(key);
+		unsigned int col = KEY_COL(key);
+		unsigned short code = KEY_VAL(key);
 
-	for (i = 0; i < kd->keymap_size; i++) {
-		u32 tmp = be32_to_cpup(prop + i);
-		int key_code, row, col;
+		if (col >= col_range) {
+			dev_err(dev, "OF: %s: column %x overflowed its range %d\n",
+					propname, col, col_range);
+			return -EINVAL;
+		}
 
-		row = (tmp >> 24) & 0xff;
-		col = (tmp >> 16) & 0xff;
-		key_code = tmp & 0xffff;
-		keymap[i] = KEY(row, col, key_code);
+		index = MATRIX_SCAN_CODE(row, col, row_shift);
+		if (index > idev->keycodemax) {
+			dev_err(dev, "OF: %s index overflow\n", propname);
+			return -EINVAL;
+		}
+		keycode[index] = code;
+		__set_bit(code, idev->keybit);
 	}
+	__clear_bit(KEY_RESERVED, idev->keybit);
 
-	return kd;
-}
-EXPORT_SYMBOL_GPL(matrix_keyboard_of_fill_keymap);
-
-void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-	if (kd) {
-		kfree(kd->keymap);
-		kfree(kd);
-	}
+	return 0;
 }
-EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
+EXPORT_SYMBOL_GPL(matrix_keypad_of_build_keymap);
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index 6c07ced..341aca1 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -108,21 +108,13 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 }
 
 #ifdef CONFIG_INPUT_OF_MATRIX_KEYMAP
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname);
-
-void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd);
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname);
 #else
-static inline struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname)
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname)
 {
 	return NULL;
 }
-
-static inline void
-matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-}
 #endif
-
 #endif /* _MATRIX_KEYPAD_H */
-- 
1.7.9

Re: [PATCH V3 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

On 3/29/2012 9:14 PM, Stephen Warren wrote:
>> > +	size = proplen / sizeof(u32);
>> > +	if (size > idev->keycodemax) {
>> > +		dev_err(dev, "OF: %s overflow\n", propname);
>> > +		return -EINVAL;
>> >  	}
> That is checking the number of entries in the property, not the values
> of the MATRIX_SCAN_CODE values derived from those entries. I'd say just
> remove this check. See below.
> 

Stephen,

I have added a check on return value of MATRIX_SCAN_CODE(), but
would still keep above check. Number of keys passed should
also be less than keycodemax.

-- 
viresh

[PATCH V4 Resend 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

We don't need to allocate memory for keymap in matrix_keyboard_of_fill_keymap(),
as this would only be used by matrix_keyboard_of_free_keymap(). Instead create
another routine matrix_keypad_of_build_keymap() which reads directly the
property from struct device_node and builds keymap.

With this eariler routines matrix_keyboard_of_fill_keymap() and
matrix_keyboard_of_free_keymap() go away.

This patch also fixes tegra driver according to these changes.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
V4:
- check return value of MATRIX_SCAN_CODE() to guarantee that it doesn't overflow
  keycodemax sized buffer.
- moved __devinit with the first line in function prototype

 drivers/input/keyboard/tegra-kbc.c  |   46 +++++++++-------
 drivers/input/of_keymap.c           |   98 ++++++++++++++++++++--------------
 include/linux/input/matrix_keypad.h |   16 ++----
 3 files changed, 88 insertions(+), 72 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 21c42f8..fd8a291 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -620,10 +620,9 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
 
 #ifdef CONFIG_OF
 static struct tegra_kbc_platform_data * __devinit
-tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
+tegra_kbc_dt_parse_pdata(struct device_node *np)
 {
 	struct tegra_kbc_platform_data *pdata;
-	struct device_node *np = pdev->dev.of_node;
 	u32 prop;
 	int i;
 
@@ -659,15 +658,11 @@ tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
 		pdata->pin_cfg[KBC_MAX_ROW + i].type = PIN_CFG_COL;
 	}
 
-	pdata->keymap_data = matrix_keyboard_of_fill_keymap(np, "linux,keymap");
-
-	/* FIXME: Add handling of linux,fn-keymap here */
-
 	return pdata;
 }
 #else
-static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
-	struct platform_device *pdev)
+static struct tegra_kbc_platform_data * __devinit
+tegra_kbc_dt_parse_pdata(struct device_node *np)
 {
 	return NULL;
 }
@@ -676,7 +671,7 @@ static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
 static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 {
 	const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
-	const struct matrix_keymap_data *keymap_data;
+	struct device_node *np = NULL;
 	struct tegra_kbc *kbc;
 	struct input_dev *input_dev;
 	struct resource *res;
@@ -686,8 +681,10 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	unsigned int debounce_cnt;
 	unsigned int scan_time_rows;
 
-	if (!pdata)
-		pdata = tegra_kbc_dt_parse_pdata(pdev);
+	if (!pdata) {
+		np = pdev->dev.of_node;
+		pdata = tegra_kbc_dt_parse_pdata(np);
+	}
 
 	if (!pdata)
 		return -EINVAL;
@@ -775,9 +772,23 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 
 	kbc->use_fn_map = pdata->use_fn_map;
 	kbc->use_ghost_filter = pdata->use_ghost_filter;
-	keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
-	matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
-				   input_dev->keycode, input_dev->keybit);
+
+	if (np) {
+		/* FIXME: Add handling of linux,fn-keymap here */
+		err = matrix_keypad_of_build_keymap(input_dev, KBC_ROW_SHIFT,
+				"linux,keymap");
+		if (err) {
+			dev_err(&pdev->dev, "OF: failed to build keymap\n");
+			goto err_put_clk;
+		}
+	} else {
+		const struct matrix_keymap_data *keymap_data =
+			pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
+
+		matrix_keypad_build_keymap(keymap_data, KBC_ROW_SHIFT,
+				input_dev->keycode, input_dev->keybit);
+	}
+
 	kbc->wakeup_key = pdata->wakeup_key;
 
 	err = request_irq(kbc->irq, tegra_kbc_isr,
@@ -798,9 +809,6 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, kbc);
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 
-	if (!pdev->dev.platform_data)
-		matrix_keyboard_of_free_keymap(pdata->keymap_data);
-
 	return 0;
 
 err_free_irq:
@@ -815,10 +823,8 @@ err_free_mem:
 	input_free_device(input_dev);
 	kfree(kbc);
 err_free_pdata:
-	if (!pdev->dev.platform_data) {
-		matrix_keyboard_of_free_keymap(pdata->keymap_data);
+	if (!pdev->dev.platform_data)
 		kfree(pdata);
-	}
 
 	return err;
 }
diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
index 061493d..631054f 100644
--- a/drivers/input/of_keymap.c
+++ b/drivers/input/of_keymap.c
@@ -17,6 +17,7 @@
  *
  */
 
+#include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/input.h>
@@ -26,62 +27,79 @@
 #include <linux/gfp.h>
 #include <linux/slab.h>
 
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np,
-			       const char *propname)
+/**
+ * matrix_keypad_of_build_keymap - convert platform DT keymap into matrix keymap
+ * @idev: pointer to struct input_dev; used for getting keycode, keybit and
+ * keycodemax.
+ * @row_shift: number of bits to shift row value by to advance to the next
+ * line in the keymap
+ * @propname: Device Tree property name to be used for reading keymap. If passed
+ * as NULL, "linux,keymap" is used.
+ *
+ * This function creates an array of keycodes, by reading propname property from
+ * device tree passed, that is suitable for using in a standard matrix keyboard
+ * driver that uses row and col as indices.
+ *
+ * Expectation from user driver: idev must be initialized with following fields:
+ * dev.parent, keycode, keybit and keycodemax.
+ */
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname)
 {
-	struct matrix_keymap_data *kd;
-	u32 *keymap;
-	int proplen, i;
+	struct device *dev = idev->dev.parent;
+	struct device_node *np = dev->of_node;
+	unsigned short *keycode;
 	const __be32 *prop;
+	unsigned int proplen, i, size, col_range = 1 << row_shift, index;
 
-	if (!np)
-		return NULL;
+	if (!np || !idev)
+		return -ENODEV;
 
 	if (!propname)
 		propname = "linux,keymap";
 
 	prop = of_get_property(np, propname, &proplen);
-	if (!prop)
-		return NULL;
+	if (!prop) {
+		dev_err(dev, "OF: %s property not defined in %s\n", propname,
+				np->full_name);
+		return -ENODEV;
+	}
 
 	if (proplen % sizeof(u32)) {
-		pr_warn("Malformed keymap property %s in %s\n",
-			propname, np->full_name);
-		return NULL;
+		dev_warn(dev, "Malformed keycode property %s in %s\n", propname,
+				np->full_name);
+		return -EINVAL;
 	}
 
-	kd = kzalloc(sizeof(*kd), GFP_KERNEL);
-	if (!kd)
-		return NULL;
-
-	kd->keymap = keymap = kzalloc(proplen, GFP_KERNEL);
-	if (!kd->keymap) {
-		kfree(kd);
-		return NULL;
+	size = proplen / sizeof(u32);
+	if (size > idev->keycodemax) {
+		dev_err(dev, "OF: %s size overflow\n", propname);
+		return -EINVAL;
 	}
 
-	kd->keymap_size = proplen / sizeof(u32);
+	keycode = idev->keycode;
+	for (i = 0; i < size; i++) {
+		unsigned int key = be32_to_cpup(prop + i);
+		unsigned int row = KEY_ROW(key);
+		unsigned int col = KEY_COL(key);
+		unsigned short code = KEY_VAL(key);
 
-	for (i = 0; i < kd->keymap_size; i++) {
-		u32 tmp = be32_to_cpup(prop + i);
-		int key_code, row, col;
+		if (col >= col_range) {
+			dev_err(dev, "OF: %s: column %x overflowed its range %d\n",
+					propname, col, col_range);
+			return -EINVAL;
+		}
 
-		row = (tmp >> 24) & 0xff;
-		col = (tmp >> 16) & 0xff;
-		key_code = tmp & 0xffff;
-		keymap[i] = KEY(row, col, key_code);
+		index = MATRIX_SCAN_CODE(row, col, row_shift);
+		if (index > idev->keycodemax) {
+			dev_err(dev, "OF: %s index overflow\n", propname);
+			return -EINVAL;
+		}
+		keycode[index] = code;
+		__set_bit(code, idev->keybit);
 	}
+	__clear_bit(KEY_RESERVED, idev->keybit);
 
-	return kd;
-}
-EXPORT_SYMBOL_GPL(matrix_keyboard_of_fill_keymap);
-
-void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-	if (kd) {
-		kfree(kd->keymap);
-		kfree(kd);
-	}
+	return 0;
 }
-EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
+EXPORT_SYMBOL_GPL(matrix_keypad_of_build_keymap);
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index 6c07ced..341aca1 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -108,21 +108,13 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 }
 
 #ifdef CONFIG_INPUT_OF_MATRIX_KEYMAP
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname);
-
-void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd);
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname);
 #else
-static inline struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname)
+int matrix_keypad_of_build_keymap(struct input_dev *idev,
+		unsigned int row_shift, const char *propname)
 {
 	return NULL;
 }
-
-static inline void
-matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-}
 #endif
-
 #endif /* _MATRIX_KEYPAD_H */
-- 
1.7.9

Re: [PATCH v4 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

On 3/30/2012 9:03 AM, Viresh KUMAR wrote:
> diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
> 
> _kbc_platform_data *
> +__devinit tegra_kbc_dt_parse_pdata(struct device_node *np)

As soon as i pressed enter, i realized i missed this one. :(
Will resend V4 with the change requested.


-- 
viresh

Re: [PATCH V4 Resend 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Stephen Warren]

On 03/29/2012 09:40 PM, Viresh Kumar wrote:
> We don't need to allocate memory for keymap in matrix_keyboard_of_fill_keymap(),
> as this would only be used by matrix_keyboard_of_free_keymap(). Instead create
> another routine matrix_keypad_of_build_keymap() which reads directly the
> property from struct device_node and builds keymap.
> 
> With this eariler routines matrix_keyboard_of_fill_keymap() and
> matrix_keyboard_of_free_keymap() go away.
> 
> This patch also fixes tegra driver according to these changes.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

The error checking looks good now, so once the issues mentioned below
are fixed:

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

> diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
...
>  #else
> -static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
> -	struct platform_device *pdev)
> +static struct tegra_kbc_platform_data * __devinit
> +tegra_kbc_dt_parse_pdata(struct device_node *np)

This one should be "inline" and not "__devinit", i.e. like it was before.

> diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
...
>  #else
> -static inline struct matrix_keymap_data *
> -matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname)
> +int matrix_keypad_of_build_keymap(struct input_dev *idev,
> +		unsigned int row_shift, const char *propname)
>  {
>  	return NULL;
>  }

This one should also be "static inline".

Re: [PATCH V4 Resend 1/2] Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

On 3/31/2012 12:15 AM, Stephen Warren wrote:
>> > diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
> ...
>> >  #else
>> > -static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
>> > -	struct platform_device *pdev)
>> > +static struct tegra_kbc_platform_data * __devinit
>> > +tegra_kbc_dt_parse_pdata(struct device_node *np)
> This one should be "inline" and not "__devinit", i.e. like it was before.
> 
>> > diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
> ...
>> >  #else
>> > -static inline struct matrix_keymap_data *
>> > -matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname)
>> > +int matrix_keypad_of_build_keymap(struct input_dev *idev,
>> > +		unsigned int row_shift, const char *propname)
>> >  {
>> >  	return NULL;
>> >  }
> This one should also be "static inline".

Would send fixup/incremental patch for this.

-- 
viresh

[PATCH] fixup! Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

Hi Dmitry,

This is an incremental patch over patch
"Input: of_keymap: Introduce matrix_keypad_of_build_keymap()"

sent earlier by me. Please squash it to that patch while applying.

This contains last few changes suggested by Stephen on V4 of this patch.
---
 drivers/input/keyboard/tegra-kbc.c  |    4 ++--
 include/linux/input/matrix_keypad.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index d1cc006..6300cc7 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -619,8 +619,8 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
 }
 
 #ifdef CONFIG_OF
-static struct tegra_kbc_platform_data * __devinit
-tegra_kbc_dt_parse_pdata(struct device_node *np)
+static inline struct tegra_kbc_platform_data * tegra_kbc_dt_parse_pdata(
+		struct device_node *np)
 {
 	struct tegra_kbc_platform_data *pdata;
 	u32 prop;
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index 341aca1..02b2118 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -111,7 +111,7 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 int matrix_keypad_of_build_keymap(struct input_dev *idev,
 		unsigned int row_shift, const char *propname);
 #else
-int matrix_keypad_of_build_keymap(struct input_dev *idev,
+static inline int matrix_keypad_of_build_keymap(struct input_dev *idev,
 		unsigned int row_shift, const char *propname)
 {
 	return NULL;
-- 
1.7.9

Re: [PATCH] fixup! Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Dmitry Torokhov]

Hi Viresh,

On Mon, Apr 02, 2012 at 09:31:51AM +0530, Viresh Kumar wrote:
> Hi Dmitry,
> 
> This is an incremental patch over patch
> "Input: of_keymap: Introduce matrix_keypad_of_build_keymap()"
> 
> sent earlier by me. Please squash it to that patch while applying.
> 
> This contains last few changes suggested by Stephen on V4 of this patch.
> ---
>  drivers/input/keyboard/tegra-kbc.c  |    4 ++--
>  include/linux/input/matrix_keypad.h |    2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
> index d1cc006..6300cc7 100644
> --- a/drivers/input/keyboard/tegra-kbc.c
> +++ b/drivers/input/keyboard/tegra-kbc.c
> @@ -619,8 +619,8 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
>  }
>  
>  #ifdef CONFIG_OF
> -static struct tegra_kbc_platform_data * __devinit
> -tegra_kbc_dt_parse_pdata(struct device_node *np)
> +static inline struct tegra_kbc_platform_data * tegra_kbc_dt_parse_pdata(
> +		struct device_node *np)
>  {

This is the full implementation and should stay __devinit. It's the stub
for !CONFIG_OF case that you want to mark "static inline". I'll fix it
up.

Thanks.

-- 
Dmitry

Re: [PATCH] fixup! Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

On 4/5/2012 6:15 AM, Dmitry Torokhov wrote:
>> >  #ifdef CONFIG_OF
>> > -static struct tegra_kbc_platform_data * __devinit
>> > -tegra_kbc_dt_parse_pdata(struct device_node *np)
>> > +static inline struct tegra_kbc_platform_data * tegra_kbc_dt_parse_pdata(
>> > +		struct device_node *np)
>> >  {
> This is the full implementation and should stay __devinit. It's the stub
> for !CONFIG_OF case that you want to mark "static inline". I'll fix it
> up.

Ahh!! I can't believe i made this blunder. I exactly knew what i have to change,
and still changed the wrong line. Sorry. :(

-- 
viresh

Re: [PATCH] fixup! Input: of_keymap: Introduce matrix_keypad_of_build_keymap()

[Viresh Kumar]

On 4/5/2012 6:15 AM, Dmitry Torokhov wrote:
> This is the full implementation and should stay __devinit. It's the stub
> for !CONFIG_OF case that you want to mark "static inline". I'll fix it
> up.

Have you pushed these for v3.5?

-- 
viresh

Re: [PATCH V3 2/2] Input: spear-keyboard: add device tree bindings

[Dmitry Torokhov]

Hi Viresh,

On Thu, Mar 29, 2012 at 02:03:27PM +0530, Viresh Kumar wrote:
>  
> -	matrix_keypad_build_keymap(keymap, ROW_SHIFT,
> -			input_dev->keycode, input_dev->keybit);
> +	if (np) {
> +		error = matrix_keypad_of_build_keymap(input_dev, ROW_SHIFT,
> +				"linux,keymap");
> +		if (error) {
> +			dev_err(&pdev->dev, "OF: failed to build keymap\n");
> +			goto err_put_clk;
> +		}
> +	} else {
> +		matrix_keypad_build_keymap(pdata->keymap, ROW_SHIFT,
> +				input_dev->keycode, input_dev->keybit);
> +	}

Now that I got matrix_keypad_build_keymap() also handle DT case, how
about the patch below?

Thanks.

-- 
Dmitry


Input: spear-keyboard - add device tree bindings

From: Viresh Kumar <viresh.kumar@st.com>

This adds simple DT bindings for spear-keyboard controller.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/keyboard/spear-keyboard.c |   71 ++++++++++++++++++++++++-------
 1 files changed, 56 insertions(+), 15 deletions(-)


diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index e83cab2..6f287f7 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -19,6 +19,7 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_wakeup.h>
 #include <linux/slab.h>
@@ -63,6 +64,7 @@ struct spear_kbd {
 	unsigned int mode;
 	unsigned short last_key;
 	unsigned short keycodes[NUM_ROWS * NUM_COLS];
+	bool rep;
 };
 
 static irqreturn_t spear_kbd_interrupt(int irq, void *dev_id)
@@ -138,27 +140,49 @@ static void spear_kbd_close(struct input_dev *dev)
 	kbd->last_key = KEY_RESERVED;
 }
 
-static int __devinit spear_kbd_probe(struct platform_device *pdev)
+#ifdef CONFIG_OF
+static int __devinit spear_kbd_parse_dt(struct platform_device *pdev,
+                                        struct spear_kbd *kbd)
 {
-	const struct kbd_platform_data *pdata = pdev->dev.platform_data;
-	const struct matrix_keymap_data *keymap;
-	struct spear_kbd *kbd;
-	struct input_dev *input_dev;
-	struct resource *res;
-	int irq;
+	struct device_node *np = pdev->dev.of_node;
 	int error;
+	u32 val;
 
-	if (!pdata) {
-		dev_err(&pdev->dev, "Invalid platform data\n");
+	if (!np) {
+		dev_err(&pdev->dev, "Missing DT data\n");
 		return -EINVAL;
 	}
 
-	keymap = pdata->keymap;
-	if (!keymap) {
-		dev_err(&pdev->dev, "no keymap defined\n");
-		return -EINVAL;
+	if (of_property_read_bool(np, "autorepeat"))
+		kbd->rep = true;
+
+	error = of_property_read_u32(np, "st,mode", &val);
+	if (error) {
+		dev_err(&pdev->dev, "DT: Invalid or missing mode\n");
+		return error;
 	}
 
+	kbd->mode = val;
+	return 0;
+}
+#else
+static inline int spear_kbd_parse_dt(struct platform_device *pdev,
+				     struct spear_kbd *kbd)
+{
+	return -ENOSYS;
+}
+#endif
+
+static int __devinit spear_kbd_probe(struct platform_device *pdev)
+{
+	struct kbd_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	const struct matrix_keymap_data *keymap = pdata ? pdata->keymap : NULL;
+	struct spear_kbd *kbd;
+	struct input_dev *input_dev;
+	struct resource *res;
+	int irq;
+	int error;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "no keyboard resource defined\n");
@@ -181,7 +205,15 @@ static int __devinit spear_kbd_probe(struct platform_device *pdev)
 
 	kbd->input = input_dev;
 	kbd->irq = irq;
-	kbd->mode = pdata->mode;
+
+	if (!pdata) {
+		error = spear_kbd_parse_dt(pdev, kbd);
+		if (error)
+			goto err_free_mem;
+	} else {
+		kbd->mode = pdata->mode;
+		kbd->rep = pdata->rep;
+	}
 
 	kbd->res = request_mem_region(res->start, resource_size(res),
 				      pdev->name);
@@ -221,7 +253,7 @@ static int __devinit spear_kbd_probe(struct platform_device *pdev)
 		goto err_put_clk;
 	}
 
-	if (pdata->rep)
+	if (kbd->rep)
 		__set_bit(EV_REP, input_dev->evbit);
 	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
 
@@ -318,6 +350,14 @@ static int spear_kbd_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
 
+#ifdef CONFIG_OF
+static const struct of_device_id spear_kbd_id_table[] = {
+	{ .compatible = "st,spear300-kbd" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, spear_kbd_id_table);
+#endif
+
 static struct platform_driver spear_kbd_driver = {
 	.probe		= spear_kbd_probe,
 	.remove		= __devexit_p(spear_kbd_remove),
@@ -325,6 +365,7 @@ static struct platform_driver spear_kbd_driver = {
 		.name	= "keyboard",
 		.owner	= THIS_MODULE,
 		.pm	= &spear_kbd_pm_ops,
+		.of_match_table = of_match_ptr(spear_kbd_id_table),
 	},
 };
 module_platform_driver(spear_kbd_driver);

Re: [PATCH V3 2/2] Input: spear-keyboard: add device tree bindings

[Viresh Kumar]

On 5/9/2012 10:58 AM, Dmitry Torokhov wrote:
> Now that I got matrix_keypad_build_keymap() also handle DT case, how
> about the patch below?

Yes it looks fine.

-- 
viresh

Re: [PATCH V3 2/2] Input: spear-keyboard: add device tree bindings

[viresh kumar]

Hi Dmitry,

On Wed, May 9, 2012 at 10:58 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Now that I got matrix_keypad_build_keymap() also handle DT case, how
> about the patch below?
>
> Input: spear-keyboard - add device tree bindings
>
> From: Viresh Kumar <viresh.kumar@st.com>
>
> This adds simple DT bindings for spear-keyboard controller.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
>  drivers/input/keyboard/spear-keyboard.c |   71 ++++++++++++++++++++++++-------
>  1 files changed, 56 insertions(+), 15 deletions(-)

You missed following file in this patch, which was there in original patch.

Documentation/devicetree/bindings/input/spear-keyboard.txt

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

Re: [PATCH V3 2/2] Input: spear-keyboard: add device tree bindings

[Dmitry Torokhov]

On Tue, May 15, 2012 at 12:24:11PM +0530, viresh kumar wrote:
> Hi Dmitry,
> 
> On Wed, May 9, 2012 at 10:58 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Now that I got matrix_keypad_build_keymap() also handle DT case, how
> > about the patch below?
> >
> > Input: spear-keyboard - add device tree bindings
> >
> > From: Viresh Kumar <viresh.kumar@st.com>
> >
> > This adds simple DT bindings for spear-keyboard controller.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> > Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> > ---
> >
> >  drivers/input/keyboard/spear-keyboard.c |   71 ++++++++++++++++++++++++-------
> >  1 files changed, 56 insertions(+), 15 deletions(-)
> 
> You missed following file in this patch, which was there in original patch.
> 
> Documentation/devicetree/bindings/input/spear-keyboard.txt

Oh, yes, sorry about that - that is danges of stgit - if patch does not
import cleanly for some reason (local changes in the tree for example)
and I have to massage and apply the patch then new files are not added
to git automatically and I sometimes forget about them. I'll ressurect
the doc change.

Thanks.

-- 
Dmitry
--
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 V3 2/2] Input: spear-keyboard: add device tree bindings

[Viresh Kumar]

This adds simple DT bindings for spear-keyboard controller.

Signed-off-by: Viresh Kumar <viresh.kumar-qxv4g6HH51o@public.gmane.org>
---
 .../devicetree/bindings/input/spear-keyboard.txt   |   21 +++++
 drivers/input/keyboard/Kconfig                     |    1 +
 drivers/input/keyboard/spear-keyboard.c            |   87 +++++++++++++++++---
 3 files changed, 96 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/spear-keyboard.txt

diff --git a/Documentation/devicetree/bindings/input/spear-keyboard.txt b/Documentation/devicetree/bindings/input/spear-keyboard.txt
new file mode 100644
index 0000000..e1bcf13
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/spear-keyboard.txt
@@ -0,0 +1,21 @@
+* SPEAr keyboard controller
+
+Required properties:
+- compatible: "st,spear300-kbd"
+
+Optional properties, in addition to those specified by the shared
+matrix-keyboard bindings:
+- autorepeat: bool: enables key autorepeat
+- st,mode: keyboard mode: 0 - 9x9, 1 - 6x6, 2 - 2x2
+
+Example:
+
+kbd@fc400000 {
+	compatible = "st,spear300-kbd";
+	reg = <0xfc400000 0x100>;
+	linux,keymap = < 0x00030012
+			 0x0102003a >;
+	autorepeat;
+	st,mode = <0>;
+};
+
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index f354813..6da2b02 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -522,6 +522,7 @@ config KEYBOARD_OMAP4
 config KEYBOARD_SPEAR
 	tristate "ST SPEAR keyboard support"
 	depends on PLAT_SPEAR
+	select INPUT_OF_MATRIX_KEYMAP if USE_OF
 	help
 	  Say Y here if you want to use the SPEAR keyboard.
 
diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 3b6b528..d72e4cb 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -19,6 +19,7 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_wakeup.h>
 #include <linux/slab.h>
@@ -136,25 +137,66 @@ static void spear_kbd_close(struct input_dev *dev)
 	kbd->last_key = KEY_RESERVED;
 }
 
+#ifdef CONFIG_OF
+static int __devinit spear_kbd_probe_dt(struct platform_device *pdev)
+{
+	struct kbd_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct device_node *np = pdev->dev.of_node;
+	u32 val;
+
+	if (of_property_read_bool(np, "autorepeat"))
+		pdata->rep = true;
+
+	if (!of_property_read_u32(np, "st,mode", &val)) {
+		pdata->mode = val;
+	} else {
+		dev_err(&pdev->dev, "DT: Invalid mode\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+#else
+static int __devinit spear_kbd_probe_dt(struct platform_device *pdev)
+{
+	return -ENOSYS;
+}
+#endif
+
 static int __devinit spear_kbd_probe(struct platform_device *pdev)
 {
-	const struct kbd_platform_data *pdata = pdev->dev.platform_data;
-	const struct matrix_keymap_data *keymap;
+	struct kbd_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct device_node *np = NULL;
 	struct spear_kbd *kbd;
 	struct input_dev *input_dev;
 	struct resource *res;
 	int irq;
 	int error;
 
-	if (!pdata) {
-		dev_err(&pdev->dev, "Invalid platform data\n");
-		return -EINVAL;
-	}
-
-	keymap = pdata->keymap;
-	if (!keymap) {
-		dev_err(&pdev->dev, "no keymap defined\n");
-		return -EINVAL;
+	if (pdata) {
+		if (!pdata->keymap) {
+			dev_err(&pdev->dev, "Invalid platform data\n");
+			return -EINVAL;
+		}
+	} else {
+		np = pdev->dev.of_node;
+		if (!np) {
+			dev_err(&pdev->dev, "Failed: Neither DT nor Pdata is passed\n");
+			return -EINVAL;
+		}
+
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata) {
+			dev_err(&pdev->dev, "DT: kzalloc failed\n");
+			return -ENOMEM;
+		}
+
+		pdev->dev.platform_data = pdata;
+		error = spear_kbd_probe_dt(pdev);
+		if (error) {
+			dev_err(&pdev->dev, "DT: no platform data\n");
+			return error;
+		}
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -221,8 +263,18 @@ static int __devinit spear_kbd_probe(struct platform_device *pdev)
 	input_dev->keycodesize = sizeof(kbd->keycodes[0]);
 	input_dev->keycodemax = ARRAY_SIZE(kbd->keycodes);
 
-	matrix_keypad_build_keymap(keymap, ROW_SHIFT,
-			input_dev->keycode, input_dev->keybit);
+	if (np) {
+		error = matrix_keypad_of_build_keymap(&pdev->dev, ROW_SHIFT,
+				input_dev->keycode, input_dev->keybit,
+				"linux,keymap");
+		if (error) {
+			dev_err(&pdev->dev, "OF: failed to build keymap\n");
+			goto err_put_clk;
+		}
+	} else {
+		matrix_keypad_build_keymap(pdata->keymap, ROW_SHIFT,
+				input_dev->keycode, input_dev->keybit);
+	}
 
 	input_set_drvdata(input_dev, kbd);
 
@@ -317,6 +369,14 @@ static int spear_kbd_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(spear_kbd_pm_ops, spear_kbd_suspend, spear_kbd_resume);
 
+#ifdef CONFIG_OF
+static const struct of_device_id spear_kbd_id_table[] = {
+	{ .compatible = "st,spear300-kbd" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, spear_kbd_id_table);
+#endif
+
 static struct platform_driver spear_kbd_driver = {
 	.probe		= spear_kbd_probe,
 	.remove		= __devexit_p(spear_kbd_remove),
@@ -324,6 +384,7 @@ static struct platform_driver spear_kbd_driver = {
 		.name	= "keyboard",
 		.owner	= THIS_MODULE,
 		.pm	= &spear_kbd_pm_ops,
+		.of_match_table = of_match_ptr(spear_kbd_id_table),
 	},
 };
 module_platform_driver(spear_kbd_driver);
-- 
1.7.10.rc2.10.gb47606