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

[PATCH V2 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>
---
V2:
- Introduced matrix_keypad_of_build_keymap() and removed fill and free keymap
  routines.
- Updated tegra-kbc.

 drivers/input/keyboard/tegra-kbc.c  |   33 ++++++++------
 drivers/input/of_keymap.c           |   82 +++++++++++++++++------------------
 include/linux/input/matrix_keypad.h |   18 +++-----
 3 files changed, 66 insertions(+), 67 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 21c42f8..96ee31e 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -659,10 +659,6 @@ 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
@@ -676,7 +672,6 @@ 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 tegra_kbc *kbc;
 	struct input_dev *input_dev;
 	struct resource *res;
@@ -775,9 +770,24 @@ 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 (pdev->dev.of_node) {
+		/* FIXME: Add handling of linux,fn-keymap here */
+		err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
+				input_dev->keycode, input_dev->keybit,
+				"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 +808,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 +822,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..2837566 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,61 @@
 #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
+ * @dev: Pointer to struct device, will be used for getting struct device_node
+ * @row_shift: number of bits to shift row value by to advance to the next
+ * line in the keymap
+ * @keymap: expanded version of keymap that is suitable for use by
+ * matrix keyboad driver
+ * @keybit: pointer to bitmap of keys supported by input device
+ * @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.
+ */
+int matrix_keypad_of_build_keymap(struct device *dev, unsigned int row_shift,
+		unsigned short *keymap, unsigned long *keybit,
+		const char *propname)
 {
-	struct matrix_keymap_data *kd;
-	u32 *keymap;
-	int proplen, i;
+	struct device_node *np = dev->of_node;
 	const __be32 *prop;
+	int proplen, i, size;
 
 	if (!np)
-		return NULL;
+		return -ENODEV;
 
 	if (!propname)
 		propname = "linux,keymap";
 
 	prop = of_get_property(np, propname, &proplen);
-	if (!prop)
-		return NULL;
-
-	if (proplen % sizeof(u32)) {
-		pr_warn("Malformed keymap property %s in %s\n",
-			propname, np->full_name);
-		return NULL;
+	if (!prop) {
+		dev_err(dev, "OF: %s property not defined in %s\n", propname,
+				np->full_name);
+		return -ENODEV;
 	}
 
-	kd = kzalloc(sizeof(*kd), GFP_KERNEL);
-	if (!kd)
-		return NULL;
-
-	kd->keymap = keymap = kzalloc(proplen, GFP_KERNEL);
-	if (!kd->keymap) {
-		kfree(kd);
-		return NULL;
+	if (proplen % sizeof(u32)) {
+		dev_warn(dev, "Malformed keymap property %s in %s\n", propname,
+				np->full_name);
+		return -EINVAL;
 	}
 
-	kd->keymap_size = proplen / sizeof(u32);
+	size = proplen / sizeof(u32);
 
-	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);
+		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
+		__set_bit(code, keybit);
 	}
+	__clear_bit(KEY_RESERVED, 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..aef352f 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -108,21 +108,15 @@ 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 device *dev, unsigned int row_shift,
+		unsigned short *keymap, unsigned long *keybit,
+		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 device *dev, unsigned int row_shift,
+		unsigned short *keymap, unsigned long *keybit,
+		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 V2 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            |   82 ++++++++++++++++----
 3 files changed, 91 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..e423a44 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,61 @@ 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;
+	struct device_node *np = pdev->dev.of_node;
 	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 (np) {
+		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;
+		}
+	} else {
+		pdata = dev_get_platdata(&pdev->dev);
+		if (!pdata || !pdata->keymap) {
+			dev_err(&pdev->dev, "Invalid platform data\n");
+			return -EINVAL;
+		}
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -221,8 +258,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 +364,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 +379,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 V2 2/2] Input: spear-keyboard: add device tree bindings

[Dmitry Torokhov]

Hi Viresh,

On Tue, Mar 27, 2012 at 11:08:12AM +0530, Viresh Kumar wrote:
> +	if (np) {
> +		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;
> +		}
> +	} else {
> +		pdata = dev_get_platdata(&pdev->dev);
> +		if (!pdata || !pdata->keymap) {
> +			dev_err(&pdev->dev, "Invalid platform data\n");
> +			return -EINVAL;
> +		}
>  	}

I think the opposite order woudl make more sense - if pdata is supplied
by the platform code then we should use it, otherwise try to see if
there is OF data available instead. This way one can easily override OF
data, if needed.

Same goes for keymap data.

Thanks.

-- 
Dmitry

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

[Viresh Kumar]

On 3/27/2012 12:43 PM, Dmitry Torokhov wrote:
> I think the opposite order woudl make more sense - if pdata is supplied
> by the platform code then we should use it, otherwise try to see if
> there is OF data available instead. This way one can easily override OF
> data, if needed.

Okay.

Just for better understanding, why should we give more preference to pdata?

I thought, a single image with pdata can be booted with or without DT
support for keyboard. So, if we are booting without of DT for keyboard,
then use pdata, otherwise use DT data.

-- 
viresh

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

[Dmitry Torokhov]

On Tue, Mar 27, 2012 at 01:07:51PM +0530, Viresh Kumar wrote:
> On 3/27/2012 12:43 PM, Dmitry Torokhov wrote:
> > I think the opposite order woudl make more sense - if pdata is supplied
> > by the platform code then we should use it, otherwise try to see if
> > there is OF data available instead. This way one can easily override OF
> > data, if needed.
> 
> Okay.
> 
> Just for better understanding, why should we give more preference to pdata?
> 
> I thought, a single image with pdata can be booted with or without DT
> support for keyboard. So, if we are booting without of DT for keyboard,
> then use pdata, otherwise use DT data.

My reasoning is that device tree is in firmware and it might be
desirable to override firmware in certain cases.

Thanks.

-- 
Dmitry

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

[Viresh Kumar]

On 3/27/2012 1:21 PM, Dmitry Torokhov wrote:
> My reasoning is that device tree is in firmware and it might be
> desirable to override firmware in certain cases.

Ok. Will do that.
Otherwise, you are okay with both the patches? So that
i can send V3.

-- 
viresh

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

[Stephen Warren]

On 03/26/2012 11:38 PM, Viresh Kumar wrote:
> This adds simple DT bindings for spear-keyboard controller.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

The binding documentation only,
Reviewed-by: Stephen Warren <swarren@wwwdotorg.org>

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

[Stephen Warren]

On 03/26/2012 11:38 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.

> diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
...
>  static int __devinit tegra_kbc_probe(struct platform_device *pdev)
...
> +	if (pdev->dev.of_node) {
> +		/* FIXME: Add handling of linux,fn-keymap here */
> +		err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
> +				input_dev->keycode, input_dev->keybit,
> +				"linux,keymap");

Where do input_dev->keycode/keybit get allocated? As far as I can tell,
matrix_keypad_of_build_keymap() just writes to those and assumes they're
already allocated.

> diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
...
> +int matrix_keypad_of_build_keymap(struct device *dev, unsigned int row_shift,
...
> +		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
> +		__set_bit(code, keybit);

How bit are keymap and keybit? I think we need range-checking here to
make sure that row/col/row_shift/code are valid and in-range.

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

[Viresh Kumar]

On 3/27/2012 9:15 PM, Stephen Warren wrote:
>> >  static int __devinit tegra_kbc_probe(struct platform_device *pdev)
> ...
>> > +	if (pdev->dev.of_node) {
>> > +		/* FIXME: Add handling of linux,fn-keymap here */
>> > +		err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
>> > +				input_dev->keycode, input_dev->keybit,
>> > +				"linux,keymap");
> Where do input_dev->keycode/keybit get allocated? As far as I can tell,
> matrix_keypad_of_build_keymap() just writes to those and assumes they're
> already allocated.

If i am not reading the code incorrectly, keycode is allocated memory with
kbc. And then we do this:

	input_dev->keycode = kbc->keycode;

keybit is again present as part of struct input_dev.
Am i missing something.

>> > diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
> ...
>> > +int matrix_keypad_of_build_keymap(struct device *dev, unsigned int row_shift,
> ...
>> > +		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
>> > +		__set_bit(code, keybit);
> How bit are keymap and keybit?

Couldn't get this one. :(
Can you please elaborate the question a bit?

> I think we need range-checking here to
> make sure that row/col/row_shift/code are valid and in-range.

I picked this directly from matrix_keypad_build_keymap() as is.
Anyway there is no loss in improving it. :)

What kind of range-check you are looking for?
Currently we do following

		unsigned int row = KEY_ROW(key);
		unsigned int col = KEY_COL(key);
		unsigned short code = KEY_VAL(key);

All these macros '&' 'key' with 0xFF, 0xFF and 0xFFFF.
Which is also kind of range checking.

-- 
viresh

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

[Stephen Warren]

On 03/27/2012 10:55 PM, Viresh Kumar wrote:
> On 3/27/2012 9:15 PM, Stephen Warren wrote:
>>>>  static int __devinit tegra_kbc_probe(struct platform_device *pdev)
>> ...
>>>> +	if (pdev->dev.of_node) {
>>>> +		/* FIXME: Add handling of linux,fn-keymap here */
>>>> +		err = matrix_keypad_of_build_keymap(&pdev->dev, KBC_ROW_SHIFT,
>>>> +				input_dev->keycode, input_dev->keybit,
>>>> +				"linux,keymap");
>>
>> Where do input_dev->keycode/keybit get allocated? As far as I can tell,
>> matrix_keypad_of_build_keymap() just writes to those and assumes they're
>> already allocated.
> 
> If i am not reading the code incorrectly, keycode is allocated memory with
> kbc. And then we do this:
> 
> 	input_dev->keycode = kbc->keycode;
> 
> keybit is again present as part of struct input_dev.
> Am i missing something.

Ah right, I'd been looking inside input_allocate_device() rather than
the specific driver's probe(). So, no issue here.

>>>> diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
>> ...
>>>> +int matrix_keypad_of_build_keymap(struct device *dev, unsigned int row_shift,
>> ...
>>>> +		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
>>>> +		__set_bit(code, keybit);
>> How bit are keymap and keybit?
> 
> Couldn't get this one. :(
> Can you please elaborate the question a bit?

s/bit/big/ might make the question clearer:-)

>> I think we need range-checking here to
>> make sure that row/col/row_shift/code are valid and in-range.
> 
> I picked this directly from matrix_keypad_build_keymap() as is.
> Anyway there is no loss in improving it. :)
> 
> What kind of range-check you are looking for?
> Currently we do following
> 
> 		unsigned int row = KEY_ROW(key);
> 		unsigned int col = KEY_COL(key);
> 		unsigned short code = KEY_VAL(key);
> 
> All these macros '&' 'key' with 0xFF, 0xFF and 0xFFFF.
> Which is also kind of range checking.

The size of the keycode array is much smaller though:

mach-tegra's kbc.h:

#define KBC_MAX_ROW	16
#define KBC_MAX_COL	8
#define KBC_MAX_KEY	(KBC_MAX_ROW * KBC_MAX_COL)

tegra-kbc.c:

struct tegra_kbc {
...
	unsigned short keycode[KBC_MAX_KEY * 2];

so the DT parsing can easily overflow this, and is driven by
user-supplied data. I think you'll need to pass input_dev->keycodesize
into matrix_keypad_of_build_keymap() to achieve the overall
range-checking, but also checking col against (1<<row_shift) would be
needed to make sure the individual "fields" of the array index don't
overflow too.

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

[Viresh Kumar]

On 3/29/2012 2:58 AM, Stephen Warren wrote:
> so the DT parsing can easily overflow this, and is driven by
> user-supplied data.

Got it. This check is must.

> I think you'll need to pass input_dev->keycodesize

you mean keycodemax here?

> into matrix_keypad_of_build_keymap() to achieve the overall
> range-checking.

Will do that.

> but also checking col against (1<<row_shift) would be
> needed to make sure the individual "fields" of the array index don't
> overflow too.

Ok. Please cross check V3 to verify, i got you comments correctly.

-- 
viresh

[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