All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 2/2] Input: matrix_keymap - wire up device tree support
@ 2012-04-26  8:19 Dmitry Torokhov
  2012-04-26 15:05 ` Stephen Warren
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2012-04-26  8:19 UTC (permalink / raw)
  To: linux-input
  Cc: Grant Likely, Rob Herring, Rakesh Iyer, Stephen Warren,
	Olof Johansson, Viresh Kumar, linux-kernel, devicetree-discuss

When platform keymap is not supplied to matrix_keypad_build_keymap()
and device tree support is enabled, try locating specified property
and load keymap from it. If property name is not defined, try using
"linux,keymap".

Based on earlier patch by Viresh Kumar <viresh.kumar@st.com>

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/keyboard/tegra-kbc.c  |   56 +++++++-----
 drivers/input/matrix-keymap.c       |  175 +++++++++++++++++++---------------
 include/linux/input/matrix_keypad.h |   18 ----
 3 files changed, 130 insertions(+), 119 deletions(-)

diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 6722d37..fba4991 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 platform_device *pdev)
+static struct tegra_kbc_platform_data * __devinit tegra_kbc_dt_parse_pdata(
+	struct platform_device *pdev)
 {
 	struct tegra_kbc_platform_data *pdata;
 	struct device_node *np = pdev->dev.of_node;
@@ -660,10 +660,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
@@ -674,10 +670,36 @@ static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
 }
 #endif
 
+static int __devinit tegra_kbd_setup_keymap(struct tegra_kbc *kbc)
+{
+	const struct tegra_kbc_platform_data *pdata = kbc->pdata;
+	const struct matrix_keymap_data *keymap_data = pdata->keymap_data;
+	unsigned int keymap_rows = KBC_MAX_KEY;
+	int retval;
+
+	if (pdata->use_fn_map)
+		keymap_rows *= 2;
+
+	retval = matrix_keypad_build_keymap(keymap_data, NULL,
+					    keymap_rows, KBC_MAX_COL,
+					    kbc->keycode, kbc->idev);
+	if (retval == -ENOSYS || retval == -ENOENT) {
+		/*
+		 * If there is no OF support in kernel or keymap
+		 * property is missing, use default keymap.
+		 */
+		retval = matrix_keypad_build_keymap(
+					&tegra_kbc_default_keymap_data, NULL,
+					keymap_rows, KBC_MAX_COL,
+					kbc->keycode, kbc->idev);
+	}
+
+	return retval;
+}
+
 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;
@@ -686,7 +708,6 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	int num_rows = 0;
 	unsigned int debounce_cnt;
 	unsigned int scan_time_rows;
-	unsigned int keymap_rows;
 
 	if (!pdata)
 		pdata = tegra_kbc_dt_parse_pdata(pdev);
@@ -768,17 +789,9 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	input_dev->open = tegra_kbc_open;
 	input_dev->close = tegra_kbc_close;
 
-	keymap_rows = KBC_MAX_KEY;
-	if (pdata->use_fn_map)
-		keymap_rows *= 2;
-
-	keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
-
-	err = matrix_keypad_build_keymap(keymap_data, NULL,
-					 keymap_rows, KBC_MAX_COL,
-					 kbc->keycode, input_dev);
+	err = tegra_kbd_setup_keymap(kbc);
 	if (err) {
-		dev_err(&pdev->dev, "failed to build keymap\n");
+		dev_err(&pdev->dev, "failed to setup keymap\n");
 		goto err_put_clk;
 	}
 
@@ -805,9 +818,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:
@@ -822,10 +832,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/matrix-keymap.c b/drivers/input/matrix-keymap.c
index de7992d..b57531e 100644
--- a/drivers/input/matrix-keymap.c
+++ b/drivers/input/matrix-keymap.c
@@ -17,15 +17,91 @@
  *
  */
 
+#include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/input.h>
 #include <linux/of.h>
 #include <linux/export.h>
-#include <linux/gfp.h>
-#include <linux/slab.h>
 #include <linux/input/matrix_keypad.h>
 
+static bool matrix_keypad_map_key(struct input_dev *input_dev,
+				  unsigned int rows, unsigned int cols,
+				  unsigned int row_shift, unsigned int key)
+{
+	unsigned char *keymap = input_dev->keycode;
+	unsigned int row = KEY_ROW(key);
+	unsigned int col = KEY_COL(key);
+	unsigned short code = KEY_VAL(key);
+
+	if (row >= rows || col >= cols) {
+		dev_err(input_dev->dev.parent,
+			"%s: invalid keymap entry 0x%x (row: %d, col: %d, rows: %d, cols: %d)\n",
+			__func__, key, row, col, rows, cols);
+		return false;
+	}
+
+	keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
+	__set_bit(code, input_dev->keybit);
+
+	return true;
+}
+
+#ifdef CONFIG_OF
+static int matrix_keypad_parse_of_keymap(const char *propname,
+					 unsigned int rows, unsigned int cols,
+					 struct input_dev *input_dev)
+{
+	struct device *dev = input_dev->dev.parent;
+	struct device_node *np = dev->of_node;
+	unsigned int row_shift = get_count_order(cols);
+	unsigned int max_keys = rows << row_shift;
+	unsigned int proplen, i, size;
+	const __be32 *prop;
+
+	if (!np)
+		return -ENODEV;
+
+	if (!propname)
+		propname = "linux,keymap";
+
+	prop = of_get_property(np, propname, &proplen);
+	if (!prop) {
+		dev_err(dev, "OF: %s property not defined in %s\n",
+			propname, np->full_name);
+		return -ENODEV;
+	}
+
+	if (proplen % sizeof(u32)) {
+		dev_err(dev, "OF: Malformed keycode property %s in %s\n",
+			propname, np->full_name);
+		return -EINVAL;
+	}
+
+	size = proplen / sizeof(u32);
+	if (size > max_keys) {
+		dev_err(dev, "OF: %s size overflow\n", propname);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < size; i++) {
+		unsigned int key = be32_to_cpup(prop + i);
+
+		if (!matrix_keypad_map_key(input_dev, rows, cols,
+					   row_shift, key))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+#else
+static int matrix_keypad_parse_of_keymap(const char *propname,
+					 unsigned int rows, unsigned int cols,
+					 struct input_dev *input_dev)
+{
+	return -ENOSYS;
+}
+#endif
 
 /**
  * matrix_keypad_build_keymap - convert platform keymap into matrix keymap
@@ -41,6 +117,13 @@
  * This function converts platform keymap (encoded with KEY() macro) into
  * an array of keycodes that is suitable for using in a standard matrix
  * keyboard driver that uses row and col as indices.
+ *
+ * If @keymap_data is not supplied and device tree support is enabled
+ * it will attempt load the keymap from property specified by @keymap_name
+ * argument (or "linux,keymap" if @keymap_name is %NULL).
+ *
+ * Callers are expected to set up input_dev->dev.parent before calling this
+ * function.
  */
 int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 			       const char *keymap_name,
@@ -50,6 +133,7 @@ int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 {
 	unsigned int row_shift = get_count_order(cols);
 	int i;
+	int error;
 
 	input_dev->keycode = keymap;
 	input_dev->keycodesize = sizeof(*keymap);
@@ -57,86 +141,23 @@ int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 
 	__set_bit(EV_KEY, input_dev->evbit);
 
-	for (i = 0; i < keymap_data->keymap_size; i++) {
-		unsigned int key = keymap_data->keymap[i];
-		unsigned int row = KEY_ROW(key);
-		unsigned int col = KEY_COL(key);
-		unsigned short code = KEY_VAL(key);
+	if (keymap_data) {
+		for (i = 0; i < keymap_data->keymap_size; i++) {
+			unsigned int key = keymap_data->keymap[i];
 
-		if (row >= rows || col >= cols) {
-			dev_err(input_dev->dev.parent,
-				"%s: invalid keymap entry %d (row: %d, col: %d, rows: %d, cols: %d)\n",
-				__func__, i, row, col, rows, cols);
-			return -EINVAL;
+			if (!matrix_keypad_map_key(input_dev, rows, cols,
+						   row_shift, key))
+				return -EINVAL;
 		}
-
-		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
-		__set_bit(code, input_dev->keybit);
+	} else {
+		error = matrix_keypad_parse_of_keymap(keymap_name, rows, cols,
+						      input_dev);
+		if (error)
+			return error;
 	}
+
 	__clear_bit(KEY_RESERVED, input_dev->keybit);
 
 	return 0;
 }
 EXPORT_SYMBOL(matrix_keypad_build_keymap);
-
-#ifdef CONFIG_OF
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np,
-			       const char *propname)
-{
-	struct matrix_keymap_data *kd;
-	u32 *keymap;
-	int proplen, i;
-	const __be32 *prop;
-
-	if (!np)
-		return NULL;
-
-	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;
-	}
-
-	kd = kzalloc(sizeof(*kd), GFP_KERNEL);
-	if (!kd)
-		return NULL;
-
-	kd->keymap = keymap = kzalloc(proplen, GFP_KERNEL);
-	if (!kd->keymap) {
-		kfree(kd);
-		return NULL;
-	}
-
-	kd->keymap_size = proplen / sizeof(u32);
-
-	for (i = 0; i < kd->keymap_size; i++) {
-		u32 tmp = be32_to_cpup(prop + i);
-		int key_code, row, col;
-
-		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);
-
-void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-	if (kd) {
-		kfree(kd->keymap);
-		kfree(kd);
-	}
-}
-EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
-#endif
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index e8fe08e..5f3aa6b 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -81,22 +81,4 @@ int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 			       unsigned short *keymap,
 			       struct input_dev *input_dev);
 
-#ifdef CONFIG_OF
-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);
-#else
-static inline struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np, 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.7.6


-- 
Dmitry

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Input: matrix_keymap - wire up device tree support
  2012-04-26  8:19 [PATCH 2/2] Input: matrix_keymap - wire up device tree support Dmitry Torokhov
@ 2012-04-26 15:05 ` Stephen Warren
  2012-04-30  4:19   ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2012-04-26 15:05 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-input, Viresh Kumar, devicetree-discuss, linux-kernel,
	Rob Herring, Rakesh Iyer

On 04/26/2012 02:19 AM, Dmitry Torokhov wrote:
> When platform keymap is not supplied to matrix_keypad_build_keymap()
> and device tree support is enabled, try locating specified property
> and load keymap from it. If property name is not defined, try using
> "linux,keymap".
> 
> Based on earlier patch by Viresh Kumar <viresh.kumar@st.com>

I think this series looks mostly OK. A few comments below.

We don't actually have the KBC driver hooked up on any boards yet, so I
can't actually test this yet.

How will the linux,fn-keymap handling work? It looks like this code is
allocating a keymap data structure with one additional row to represent
fn-not-pressed vs. fn-pressed. I assume this will work without issue
even though the second half is not filled in. Won't this allow the
linux,keymap property entries to pass validation "if (row >= rows)" for
one more row than it should?

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

> +static int __devinit tegra_kbd_setup_keymap(struct tegra_kbc *kbc)
> +{
> +	const struct tegra_kbc_platform_data *pdata = kbc->pdata;
> +	const struct matrix_keymap_data *keymap_data = pdata->keymap_data;
> +	unsigned int keymap_rows = KBC_MAX_KEY;
> +	int retval;
> +
> +	if (pdata->use_fn_map)
> +		keymap_rows *= 2;
> +
> +	retval = matrix_keypad_build_keymap(keymap_data, NULL,
> +					    keymap_rows, KBC_MAX_COL,
> +					    kbc->keycode, kbc->idev);
> +	if (retval == -ENOSYS || retval == -ENOENT) {

This is looking for ENOSYS or ENOENT, but ...

> diff --git a/drivers/input/matrix-keymap.c b/drivers/input/matrix-keymap.c

> +static int matrix_keypad_parse_of_keymap(const char *propname,

> +	if (!np)
> +		return -ENODEV;

Here and ...

> +	prop = of_get_property(np, propname, &proplen);
> +	if (!prop) {
> +		dev_err(dev, "OF: %s property not defined in %s\n",
> +			propname, np->full_name);
> +		return -ENODEV;

Here return ENODEV instead.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Input: matrix_keymap - wire up device tree support
  2012-04-26 15:05 ` Stephen Warren
@ 2012-04-30  4:19   ` Dmitry Torokhov
  2012-04-30 16:00     ` Stephen Warren
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2012-04-30  4:19 UTC (permalink / raw)
  To: Stephen Warren
  Cc: linux-input, Viresh Kumar, devicetree-discuss, linux-kernel,
	Rob Herring, Rakesh Iyer

On Thu, Apr 26, 2012 at 09:05:18AM -0600, Stephen Warren wrote:
> On 04/26/2012 02:19 AM, Dmitry Torokhov wrote:
> > When platform keymap is not supplied to matrix_keypad_build_keymap()
> > and device tree support is enabled, try locating specified property
> > and load keymap from it. If property name is not defined, try using
> > "linux,keymap".
> > 
> > Based on earlier patch by Viresh Kumar <viresh.kumar@st.com>
> 
> I think this series looks mostly OK. A few comments below.
> 
> We don't actually have the KBC driver hooked up on any boards yet, so I
> can't actually test this yet.
> 
> How will the linux,fn-keymap handling work? It looks like this code is
> allocating a keymap data structure with one additional row to represent
> fn-not-pressed vs. fn-pressed.

No, it loads 2x rows (therefore giving you twice original keymap size).

> I assume this will work without issue
> even though the second half is not filled in. Won't this allow the
> linux,keymap property entries to pass validation "if (row >= rows)" for
> one more row than it should?

Maybe... I think we should revisit this when you actually have
linux,fn-keymap. Probably will need to export matrix_keypad_parse_

> 
> > diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
> 
> > +static int __devinit tegra_kbd_setup_keymap(struct tegra_kbc *kbc)
> > +{
> > +	const struct tegra_kbc_platform_data *pdata = kbc->pdata;
> > +	const struct matrix_keymap_data *keymap_data = pdata->keymap_data;
> > +	unsigned int keymap_rows = KBC_MAX_KEY;
> > +	int retval;
> > +
> > +	if (pdata->use_fn_map)
> > +		keymap_rows *= 2;
> > +
> > +	retval = matrix_keypad_build_keymap(keymap_data, NULL,
> > +					    keymap_rows, KBC_MAX_COL,
> > +					    kbc->keycode, kbc->idev);
> > +	if (retval == -ENOSYS || retval == -ENOENT) {
> 
> This is looking for ENOSYS or ENOENT, but ...

Maybe just change it to retval && retval != -EINVAL...

> 
> > diff --git a/drivers/input/matrix-keymap.c b/drivers/input/matrix-keymap.c
> 
> > +static int matrix_keypad_parse_of_keymap(const char *propname,
> 
> > +	if (!np)
> > +		return -ENODEV;
> 
> Here and ...

Hmm, -ENODEV is probably not good. Not sure if this should be -ENOENT,
-ENOSYS or -EINVAL.

> 
> > +	prop = of_get_property(np, propname, &proplen);
> > +	if (!prop) {
> > +		dev_err(dev, "OF: %s property not defined in %s\n",
> > +			propname, np->full_name);
> > +		return -ENODEV;
> 
> Here return ENODEV instead.

This indeed should be -ENOENT.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Input: matrix_keymap - wire up device tree support
  2012-04-30  4:19   ` Dmitry Torokhov
@ 2012-04-30 16:00     ` Stephen Warren
  2012-05-09  5:25       ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Warren @ 2012-04-30 16:00 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-input, Viresh Kumar, devicetree-discuss, linux-kernel,
	Rob Herring, Rakesh Iyer

On 04/29/2012 10:19 PM, Dmitry Torokhov wrote:
> On Thu, Apr 26, 2012 at 09:05:18AM -0600, Stephen Warren wrote:
>> On 04/26/2012 02:19 AM, Dmitry Torokhov wrote:
>>> When platform keymap is not supplied to matrix_keypad_build_keymap()
>>> and device tree support is enabled, try locating specified property
>>> and load keymap from it. If property name is not defined, try using
>>> "linux,keymap".
>>>
>>> Based on earlier patch by Viresh Kumar <viresh.kumar@st.com>
>>
>> I think this series looks mostly OK. A few comments below.
>>
>> We don't actually have the KBC driver hooked up on any boards yet, so I
>> can't actually test this yet.
>>
>> How will the linux,fn-keymap handling work? It looks like this code is
>> allocating a keymap data structure with one additional row to represent
>> fn-not-pressed vs. fn-pressed.
> 
> No, it loads 2x rows (therefore giving you twice original keymap size).

Yes, I mean an extra row signal, so therefore twice as many rows.

>> I assume this will work without issue
>> even though the second half is not filled in. Won't this allow the
>> linux,keymap property entries to pass validation "if (row >= rows)" for
>> one more row than it should?
> 
> Maybe... I think we should revisit this when you actually have
> linux,fn-keymap. Probably will need to export matrix_keypad_parse_

Would it be better to just drop the support for the linux,fn-keymap
property, until the full support is there? That wouldn't lose any
functionality, but would avoid this potential error-checking issue.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Input: matrix_keymap - wire up device tree support
  2012-04-30 16:00     ` Stephen Warren
@ 2012-05-09  5:25       ` Dmitry Torokhov
  2012-05-09 15:46         ` Stephen Warren
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2012-05-09  5:25 UTC (permalink / raw)
  To: Stephen Warren
  Cc: linux-input, Viresh Kumar, devicetree-discuss, linux-kernel,
	Rob Herring, Rakesh Iyer

On Mon, Apr 30, 2012 at 10:00:42AM -0600, Stephen Warren wrote:
> On 04/29/2012 10:19 PM, Dmitry Torokhov wrote:
> > On Thu, Apr 26, 2012 at 09:05:18AM -0600, Stephen Warren wrote:
> >> On 04/26/2012 02:19 AM, Dmitry Torokhov wrote:
> >>> When platform keymap is not supplied to matrix_keypad_build_keymap()
> >>> and device tree support is enabled, try locating specified property
> >>> and load keymap from it. If property name is not defined, try using
> >>> "linux,keymap".
> >>>
> >>> Based on earlier patch by Viresh Kumar <viresh.kumar@st.com>
> >>
> >> I think this series looks mostly OK. A few comments below.
> >>
> >> We don't actually have the KBC driver hooked up on any boards yet, so I
> >> can't actually test this yet.
> >>
> >> How will the linux,fn-keymap handling work? It looks like this code is
> >> allocating a keymap data structure with one additional row to represent
> >> fn-not-pressed vs. fn-pressed.
> > 
> > No, it loads 2x rows (therefore giving you twice original keymap size).
> 
> Yes, I mean an extra row signal, so therefore twice as many rows.
> 
> >> I assume this will work without issue
> >> even though the second half is not filled in. Won't this allow the
> >> linux,keymap property entries to pass validation "if (row >= rows)" for
> >> one more row than it should?
> > 
> > Maybe... I think we should revisit this when you actually have
> > linux,fn-keymap. Probably will need to export matrix_keypad_parse_
> 
> Would it be better to just drop the support for the linux,fn-keymap
> property, until the full support is there? That wouldn't lose any
> functionality, but would avoid this potential error-checking issue.

Fair enough, how about the version below?

-- 
Dmitry


Input: matrix_keymap - wire up device tree support

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

When platform keymap is not supplied to matrix_keypad_build_keymap()
and device tree support is enabled, try locating specified property
and load keymap from it. If property name is not defined, try using
"linux,keymap".

Based on earlier patch by Viresh Kumar <viresh.kumar@st.com>

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

 drivers/input/keyboard/tegra-kbc.c  |   56 ++++++-----
 drivers/input/matrix-keymap.c       |  175 ++++++++++++++++++++---------------
 include/linux/input/matrix_keypad.h |   18 ----
 3 files changed, 130 insertions(+), 119 deletions(-)


diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 6722d37..4ffe64d 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 platform_device *pdev)
+static struct tegra_kbc_platform_data * __devinit tegra_kbc_dt_parse_pdata(
+	struct platform_device *pdev)
 {
 	struct tegra_kbc_platform_data *pdata;
 	struct device_node *np = pdev->dev.of_node;
@@ -660,10 +660,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
@@ -674,10 +670,36 @@ static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
 }
 #endif
 
+static int __devinit tegra_kbd_setup_keymap(struct tegra_kbc *kbc)
+{
+	const struct tegra_kbc_platform_data *pdata = kbc->pdata;
+	const struct matrix_keymap_data *keymap_data = pdata->keymap_data;
+	unsigned int keymap_rows = KBC_MAX_KEY;
+	int retval;
+
+	if (keymap_data && pdata->use_fn_map)
+		keymap_rows *= 2;
+
+	retval = matrix_keypad_build_keymap(keymap_data, NULL,
+					    keymap_rows, KBC_MAX_COL,
+					    kbc->keycode, kbc->idev);
+	if (retval == -ENOSYS || retval == -ENOENT) {
+		/*
+		 * If there is no OF support in kernel or keymap
+		 * property is missing, use default keymap.
+		 */
+		retval = matrix_keypad_build_keymap(
+					&tegra_kbc_default_keymap_data, NULL,
+					keymap_rows, KBC_MAX_COL,
+					kbc->keycode, kbc->idev);
+	}
+
+	return retval;
+}
+
 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;
@@ -686,7 +708,6 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	int num_rows = 0;
 	unsigned int debounce_cnt;
 	unsigned int scan_time_rows;
-	unsigned int keymap_rows;
 
 	if (!pdata)
 		pdata = tegra_kbc_dt_parse_pdata(pdev);
@@ -768,17 +789,9 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	input_dev->open = tegra_kbc_open;
 	input_dev->close = tegra_kbc_close;
 
-	keymap_rows = KBC_MAX_KEY;
-	if (pdata->use_fn_map)
-		keymap_rows *= 2;
-
-	keymap_data = pdata->keymap_data ?: &tegra_kbc_default_keymap_data;
-
-	err = matrix_keypad_build_keymap(keymap_data, NULL,
-					 keymap_rows, KBC_MAX_COL,
-					 kbc->keycode, input_dev);
+	err = tegra_kbd_setup_keymap(kbc);
 	if (err) {
-		dev_err(&pdev->dev, "failed to build keymap\n");
+		dev_err(&pdev->dev, "failed to setup keymap\n");
 		goto err_put_clk;
 	}
 
@@ -805,9 +818,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:
@@ -822,10 +832,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/matrix-keymap.c b/drivers/input/matrix-keymap.c
index de7992d..db92c1e 100644
--- a/drivers/input/matrix-keymap.c
+++ b/drivers/input/matrix-keymap.c
@@ -17,15 +17,91 @@
  *
  */
 
+#include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
 #include <linux/input.h>
 #include <linux/of.h>
 #include <linux/export.h>
-#include <linux/gfp.h>
-#include <linux/slab.h>
 #include <linux/input/matrix_keypad.h>
 
+static bool matrix_keypad_map_key(struct input_dev *input_dev,
+				  unsigned int rows, unsigned int cols,
+				  unsigned int row_shift, unsigned int key)
+{
+	unsigned char *keymap = input_dev->keycode;
+	unsigned int row = KEY_ROW(key);
+	unsigned int col = KEY_COL(key);
+	unsigned short code = KEY_VAL(key);
+
+	if (row >= rows || col >= cols) {
+		dev_err(input_dev->dev.parent,
+			"%s: invalid keymap entry 0x%x (row: %d, col: %d, rows: %d, cols: %d)\n",
+			__func__, key, row, col, rows, cols);
+		return false;
+	}
+
+	keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
+	__set_bit(code, input_dev->keybit);
+
+	return true;
+}
+
+#ifdef CONFIG_OF
+static int matrix_keypad_parse_of_keymap(const char *propname,
+					 unsigned int rows, unsigned int cols,
+					 struct input_dev *input_dev)
+{
+	struct device *dev = input_dev->dev.parent;
+	struct device_node *np = dev->of_node;
+	unsigned int row_shift = get_count_order(cols);
+	unsigned int max_keys = rows << row_shift;
+	unsigned int proplen, i, size;
+	const __be32 *prop;
+
+	if (!np)
+		return -ENOENT;
+
+	if (!propname)
+		propname = "linux,keymap";
+
+	prop = of_get_property(np, propname, &proplen);
+	if (!prop) {
+		dev_err(dev, "OF: %s property not defined in %s\n",
+			propname, np->full_name);
+		return -ENOENT;
+	}
+
+	if (proplen % sizeof(u32)) {
+		dev_err(dev, "OF: Malformed keycode property %s in %s\n",
+			propname, np->full_name);
+		return -EINVAL;
+	}
+
+	size = proplen / sizeof(u32);
+	if (size > max_keys) {
+		dev_err(dev, "OF: %s size overflow\n", propname);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < size; i++) {
+		unsigned int key = be32_to_cpup(prop + i);
+
+		if (!matrix_keypad_map_key(input_dev, rows, cols,
+					   row_shift, key))
+			return -EINVAL;
+	}
+
+	return 0;
+}
+#else
+static int matrix_keypad_parse_of_keymap(const char *propname,
+					 unsigned int rows, unsigned int cols,
+					 struct input_dev *input_dev)
+{
+	return -ENOSYS;
+}
+#endif
 
 /**
  * matrix_keypad_build_keymap - convert platform keymap into matrix keymap
@@ -41,6 +117,13 @@
  * This function converts platform keymap (encoded with KEY() macro) into
  * an array of keycodes that is suitable for using in a standard matrix
  * keyboard driver that uses row and col as indices.
+ *
+ * If @keymap_data is not supplied and device tree support is enabled
+ * it will attempt load the keymap from property specified by @keymap_name
+ * argument (or "linux,keymap" if @keymap_name is %NULL).
+ *
+ * Callers are expected to set up input_dev->dev.parent before calling this
+ * function.
  */
 int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 			       const char *keymap_name,
@@ -50,6 +133,7 @@ int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 {
 	unsigned int row_shift = get_count_order(cols);
 	int i;
+	int error;
 
 	input_dev->keycode = keymap;
 	input_dev->keycodesize = sizeof(*keymap);
@@ -57,86 +141,23 @@ int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 
 	__set_bit(EV_KEY, input_dev->evbit);
 
-	for (i = 0; i < keymap_data->keymap_size; i++) {
-		unsigned int key = keymap_data->keymap[i];
-		unsigned int row = KEY_ROW(key);
-		unsigned int col = KEY_COL(key);
-		unsigned short code = KEY_VAL(key);
+	if (keymap_data) {
+		for (i = 0; i < keymap_data->keymap_size; i++) {
+			unsigned int key = keymap_data->keymap[i];
 
-		if (row >= rows || col >= cols) {
-			dev_err(input_dev->dev.parent,
-				"%s: invalid keymap entry %d (row: %d, col: %d, rows: %d, cols: %d)\n",
-				__func__, i, row, col, rows, cols);
-			return -EINVAL;
+			if (!matrix_keypad_map_key(input_dev, rows, cols,
+						   row_shift, key))
+				return -EINVAL;
 		}
-
-		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
-		__set_bit(code, input_dev->keybit);
+	} else {
+		error = matrix_keypad_parse_of_keymap(keymap_name, rows, cols,
+						      input_dev);
+		if (error)
+			return error;
 	}
+
 	__clear_bit(KEY_RESERVED, input_dev->keybit);
 
 	return 0;
 }
 EXPORT_SYMBOL(matrix_keypad_build_keymap);
-
-#ifdef CONFIG_OF
-struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np,
-			       const char *propname)
-{
-	struct matrix_keymap_data *kd;
-	u32 *keymap;
-	int proplen, i;
-	const __be32 *prop;
-
-	if (!np)
-		return NULL;
-
-	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;
-	}
-
-	kd = kzalloc(sizeof(*kd), GFP_KERNEL);
-	if (!kd)
-		return NULL;
-
-	kd->keymap = keymap = kzalloc(proplen, GFP_KERNEL);
-	if (!kd->keymap) {
-		kfree(kd);
-		return NULL;
-	}
-
-	kd->keymap_size = proplen / sizeof(u32);
-
-	for (i = 0; i < kd->keymap_size; i++) {
-		u32 tmp = be32_to_cpup(prop + i);
-		int key_code, row, col;
-
-		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);
-
-void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-	if (kd) {
-		kfree(kd->keymap);
-		kfree(kd);
-	}
-}
-EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
-#endif
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index e8fe08e..5f3aa6b 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -81,22 +81,4 @@ int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 			       unsigned short *keymap,
 			       struct input_dev *input_dev);
 
-#ifdef CONFIG_OF
-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);
-#else
-static inline struct matrix_keymap_data *
-matrix_keyboard_of_fill_keymap(struct device_node *np, const char *propname)
-{
-	return NULL;
-}
-
-static inline void
-matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
-{
-}
-#endif
-
 #endif /* _MATRIX_KEYPAD_H */

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] Input: matrix_keymap - wire up device tree support
  2012-05-09  5:25       ` Dmitry Torokhov
@ 2012-05-09 15:46         ` Stephen Warren
  0 siblings, 0 replies; 6+ messages in thread
From: Stephen Warren @ 2012-05-09 15:46 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: linux-input, Viresh Kumar, devicetree-discuss, linux-kernel,
	Rob Herring, Rakesh Iyer

On 05/08/2012 11:25 PM, Dmitry Torokhov wrote:
> On Mon, Apr 30, 2012 at 10:00:42AM -0600, Stephen Warren wrote:
>> On 04/29/2012 10:19 PM, Dmitry Torokhov wrote:
>>> On Thu, Apr 26, 2012 at 09:05:18AM -0600, Stephen Warren wrote:
>>>> On 04/26/2012 02:19 AM, Dmitry Torokhov wrote:
>>>>> When platform keymap is not supplied to matrix_keypad_build_keymap()
>>>>> and device tree support is enabled, try locating specified property
>>>>> and load keymap from it. If property name is not defined, try using
>>>>> "linux,keymap".
>>>>>
>>>>> Based on earlier patch by Viresh Kumar <viresh.kumar@st.com>
>>>>
>>>> I think this series looks mostly OK. A few comments below.
>>>>
>>>> We don't actually have the KBC driver hooked up on any boards yet, so I
>>>> can't actually test this yet.
>>>>
>>>> How will the linux,fn-keymap handling work? It looks like this code is
>>>> allocating a keymap data structure with one additional row to represent
>>>> fn-not-pressed vs. fn-pressed.
>>>
>>> No, it loads 2x rows (therefore giving you twice original keymap size).
>>
>> Yes, I mean an extra row signal, so therefore twice as many rows.
>>
>>>> I assume this will work without issue
>>>> even though the second half is not filled in. Won't this allow the
>>>> linux,keymap property entries to pass validation "if (row >= rows)" for
>>>> one more row than it should?
>>>
>>> Maybe... I think we should revisit this when you actually have
>>> linux,fn-keymap. Probably will need to export matrix_keypad_parse_
>>
>> Would it be better to just drop the support for the linux,fn-keymap
>> property, until the full support is there? That wouldn't lose any
>> functionality, but would avoid this potential error-checking issue.
> 
> Fair enough, how about the version below?

Yes, I think that will work.

I would expect that when linux,fn-keymap support gets added, perhaps the
"keymap_rows *= 2" would move into the DT keymap parsing code, since
only it will know whether its going to parse the second property. But I
don't think that influences the current patch.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-09 15:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-26  8:19 [PATCH 2/2] Input: matrix_keymap - wire up device tree support Dmitry Torokhov
2012-04-26 15:05 ` Stephen Warren
2012-04-30  4:19   ` Dmitry Torokhov
2012-04-30 16:00     ` Stephen Warren
2012-05-09  5:25       ` Dmitry Torokhov
2012-05-09 15:46         ` Stephen Warren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.