[U-Boot] [PATCH] input: simplify key_matrix_decode_fdt()

[U-Boot] [PATCH] input: simplify key_matrix_decode_fdt()

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

We know the exact property names that the code wants to process. Look
these up directly with fdt_get_property(), rather than iterating over
all properties within the node, and checking each property's name, in
a convoluted fashion, against the expected name.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
Note: This depends on my previous patch "input: fix unaligned access
in key_matrix_decode_fdt()". While to two patches could be squashed
together, I'd prefer them to go in separately, since the former is a
bug-fix that makes the original code work again on ARMv7 at least, and
this patch is an unrelated refactoring.
---
 drivers/input/key_matrix.c |   66 +++++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 40 deletions(-)

diff --git a/drivers/input/key_matrix.c b/drivers/input/key_matrix.c
index c8b048e..ea05c77 100644
--- a/drivers/input/key_matrix.c
+++ b/drivers/input/key_matrix.c
@@ -154,54 +154,40 @@ static uchar *create_keymap(struct key_matrix *config, u32 *data, int len,
 	return map;
 }
 
-int key_matrix_decode_fdt(struct key_matrix *config, const void *blob,
-			  int node)
+int key_matrix_decode_fdt(struct key_matrix *config, const void *blob, int node)
 {
 	const struct fdt_property *prop;
-	static const char prefix[] = "linux,";
-	int plen = sizeof(prefix) - 1;
-	int offset;
-
-	/* Check each property name for ones that we understand */
-	for (offset = fdt_first_property_offset(blob, node);
-		      offset > 0;
-		      offset = fdt_next_property_offset(blob, offset)) {
-		const char *name;
-		int len;
-
-		prop = fdt_get_property_by_offset(blob, offset, NULL);
-		name = fdt_string(blob, fdt32_to_cpu(prop->nameoff));
-		len = strlen(name);
-
-		/* Name needs to match "1,<type>keymap" */
-		debug("%s: property '%s'\n", __func__, name);
-		if (strncmp(name, prefix, plen) ||
-				len < plen + 6 ||
-				strcmp(name + len - 6, "keymap"))
-			continue;
+	int proplen;
+	uchar *plain_keycode;
 
-		len -= plen + 6;
-		if (len == 0) {
-			config->plain_keycode = create_keymap(config,
-				(u32 *)prop->data, fdt32_to_cpu(prop->len),
-				KEY_FN, &config->fn_pos);
-		} else if (0 == strncmp(name + plen, "fn-", len)) {
-			config->fn_keycode = create_keymap(config,
-				(u32 *)prop->data, fdt32_to_cpu(prop->len),
-				-1, NULL);
-		} else {
-			debug("%s: unrecognised property '%s'\n", __func__,
-			      name);
-		}
-	}
-	debug("%s: Decoded key maps %p, %p from fdt\n", __func__,
-	      config->plain_keycode, config->fn_keycode);
+	prop = fdt_get_property(blob, node, "linux,keymap", &proplen);
+	/* Basic keymap is required */
+	if (!prop)
+		return -1;
 
+	plain_keycode = create_keymap(config, (u32 *)prop->data,
+		proplen, KEY_FN, &config->fn_pos);
+	config->plain_keycode = plain_keycode;
+	/* Conversion error -> fail */
+	if (!config->plain_keycode)
+		return -1;
+
+	prop = fdt_get_property(blob, node, "linux,fn-keymap", &proplen);
+	/* fn keymap is optional */
+	if (!prop)
+		goto done;
+
+	config->fn_keycode = create_keymap(config, (u32 *)prop->data,
+		proplen, -1, NULL);
+	/* Conversion error -> fail */
 	if (!config->plain_keycode) {
-		debug("%s: cannot find keycode-plain map\n", __func__);
+		free(plain_keycode);
 		return -1;
 	}
 
+done:
+	debug("%s: Decoded key maps %p, %p from fdt\n", __func__,
+	      config->plain_keycode, config->fn_keycode);
 	return 0;
 }
 
-- 
1.7.10.4

[U-Boot] [PATCH] input: simplify key_matrix_decode_fdt()

[Simon Glass]

Hi Stephen,

On Thu, May 23, 2013 at 3:09 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> From: Stephen Warren <swarren@nvidia.com>
>
> We know the exact property names that the code wants to process. Look
> these up directly with fdt_get_property(), rather than iterating over
> all properties within the node, and checking each property's name, in
> a convoluted fashion, against the expected name.
>

The original code dealt with ctrl and shift also - since removed. I think
it is good to simplify it.


>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> Note: This depends on my previous patch "input: fix unaligned access
> in key_matrix_decode_fdt()". While to two patches could be squashed
> together, I'd prefer them to go in separately, since the former is a
> bug-fix that makes the original code work again on ARMv7 at least, and
> this patch is an unrelated refactoring.
>

Yes.


> ---
>  drivers/input/key_matrix.c |   66
> +++++++++++++++++---------------------------
>  1 file changed, 26 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/input/key_matrix.c b/drivers/input/key_matrix.c
> index c8b048e..ea05c77 100644
> --- a/drivers/input/key_matrix.c
> +++ b/drivers/input/key_matrix.c
> @@ -154,54 +154,40 @@ static uchar *create_keymap(struct key_matrix
> *config, u32 *data, int len,
>         return map;
>  }
>
> -int key_matrix_decode_fdt(struct key_matrix *config, const void *blob,
> -                         int node)
> +int key_matrix_decode_fdt(struct key_matrix *config, const void *blob,
> int node)
>  {
>         const struct fdt_property *prop;
> -       static const char prefix[] = "linux,";
> -       int plen = sizeof(prefix) - 1;
> -       int offset;
> -
> -       /* Check each property name for ones that we understand */
> -       for (offset = fdt_first_property_offset(blob, node);
> -                     offset > 0;
> -                     offset = fdt_next_property_offset(blob, offset)) {
> -               const char *name;
> -               int len;
> -
> -               prop = fdt_get_property_by_offset(blob, offset, NULL);
> -               name = fdt_string(blob, fdt32_to_cpu(prop->nameoff));
> -               len = strlen(name);
> -
> -               /* Name needs to match "1,<type>keymap" */
> -               debug("%s: property '%s'\n", __func__, name);
> -               if (strncmp(name, prefix, plen) ||
> -                               len < plen + 6 ||
> -                               strcmp(name + len - 6, "keymap"))
> -                       continue;
> +       int proplen;
> +       uchar *plain_keycode;
>
> -               len -= plen + 6;
> -               if (len == 0) {
> -                       config->plain_keycode = create_keymap(config,
> -                               (u32 *)prop->data, fdt32_to_cpu(prop->len),
> -                               KEY_FN, &config->fn_pos);
> -               } else if (0 == strncmp(name + plen, "fn-", len)) {
> -                       config->fn_keycode = create_keymap(config,
> -                               (u32 *)prop->data, fdt32_to_cpu(prop->len),
> -                               -1, NULL);
> -               } else {
> -                       debug("%s: unrecognised property '%s'\n", __func__,
> -                             name);
> -               }
> -       }
> -       debug("%s: Decoded key maps %p, %p from fdt\n", __func__,
> -             config->plain_keycode, config->fn_keycode);
> +       prop = fdt_get_property(blob, node, "linux,keymap", &proplen);
> +       /* Basic keymap is required */
> +       if (!prop)
> +               return -1;
>
> +       plain_keycode = create_keymap(config, (u32 *)prop->data,
> +               proplen, KEY_FN, &config->fn_pos);
>

Probably don't need plain_keycode variable at all.


> +       config->plain_keycode = plain_keycode;
> +       /* Conversion error -> fail */
> +       if (!config->plain_keycode)
> +               return -1;
>

Missing debug() here from old code


> +
> +       prop = fdt_get_property(blob, node, "linux,fn-keymap", &proplen);
> +       /* fn keymap is optional */
> +       if (!prop)
> +               goto done;
> +
> +       config->fn_keycode = create_keymap(config, (u32 *)prop->data,
> +               proplen, -1, NULL);
> +       /* Conversion error -> fail */
>         if (!config->plain_keycode) {
>

Should check config->fn_keycode here.


> -               debug("%s: cannot find keycode-plain map\n", __func__);
> +               free(plain_keycode);
>                 return -1;
>         }
>
> +done:
> +       debug("%s: Decoded key maps %p, %p from fdt\n", __func__,
> +             config->plain_keycode, config->fn_keycode);
>         return 0;
>  }
>
> --
> 1.7.10.4
>
>

Regards,
Simon

[U-Boot] [PATCH] input: simplify key_matrix_decode_fdt()

[Stephen Warren]

On 05/26/2013 01:31 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Thu, May 23, 2013 at 3:09 PM, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
> 
>     From: Stephen Warren <swarren at nvidia.com <mailto:swarren@nvidia.com>>
> 
>     We know the exact property names that the code wants to process. Look
>     these up directly with fdt_get_property(), rather than iterating over
>     all properties within the node, and checking each property's name, in
>     a convoluted fashion, against the expected name.
>     +       plain_keycode = create_keymap(config, (u32 *)prop->data,
>     +               proplen, KEY_FN, &config->fn_pos);
> 
> Probably don't need plain_keycode variable at all.

This is required because the variable is passed to free() later, and
config->plain_keycode is marked const, whereas free isn't prototyped to
take a const. I figured that it was simplest to use a separate variable
here rather than cast away the const when calling free(). Now, if C had
const_cast<>, then I would have made a different decision:-)

[U-Boot] [PATCH] input: simplify key_matrix_decode_fdt()

[Stephen Warren]

On 05/23/2013 04:09 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> We know the exact property names that the code wants to process. Look
> these up directly with fdt_get_property(), rather than iterating over
> all properties within the node, and checking each property's name, in
> a convoluted fashion, against the expected name.

Tom, is this likely to be applied for the upcoming release, or would it
be deferred until the next? Thanks.

[U-Boot] input: simplify key_matrix_decode_fdt()

[Tom Rini]

On Thu, May 23, 2013 at 12:09:57PM -0000, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> We know the exact property names that the code wants to process. Look
> these up directly with fdt_get_property(), rather than iterating over
> all properties within the node, and checking each property's name, in
> a convoluted fashion, against the expected name.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130605/df83088f/attachment.pgp>