[PATCH 0/9] Add dtget and dtput for access to fdt from build system

[PATCH 0/9] Add dtget and dtput for access to fdt from build system

[Simon Glass]

This patch set adds two new utilities:

   dtget for reading properties from a device tree binary file
   dtput for updating the device tree binary file

These are useful in scripts where configuration or other information must be
passed to the running system from the build system or vice versa. For example,
dtget can be used to obtain the LCD screen width/height for use with preparing
custom bitmap images for display on the screen. Going the other way, dtput can
be used to set configuration parameters known only to the build or
manufacturing test system, for example a hardware ID or the particular type
of boot EEPROM used on this PCB.

It is desirable to use these utilities rather than parsing or updating the
device tree source file, since this parsing is already implemented in dtc and
it makes no sense to reimplement it in a script. It also means that the build
system and the run-time environment see the same device tree values, so that
parsing or updating bugs do not creap in.

I believe that these utilities belong with dtc rather than libfdt since they
are more useful in the build system context rather than at run-time. At
run-time it is easier and better to use the provided library rather than
these utiltiies. But for build scripts and production automation, these
uitilites are better.

Some effort has been made to add tests and to use a sensible parameter format,
but comments are welcome on these and any other part of this patch set.


Simon Glass (9):
  Split out is_printable_string() into util.c
  Add dtget utility to read property values from device tree
  Add basic tests for dtget
  Add missing tests to .gitignore
  Add utilfdt for common functions, adjust dtget
  Add new dtput utility to write values to fdt
  Add some tests for dtput
  Add dtput to .gitignore
  dtput: Support adding strings with spaces

 .gitignore             |    2 +
 Makefile               |   14 ++++
 Makefile.dtget         |   14 ++++
 Makefile.dtput         |   14 ++++
 Makefile.ftdump        |    3 +-
 dtget.c                |  166 +++++++++++++++++++++++++++++++++++++++
 dtput.c                |  204 ++++++++++++++++++++++++++++++++++++++++++++++++
 ftdump.c               |   28 +------
 tests/.gitignore       |    2 +
 tests/dtget-runtest.sh |   30 +++++++
 tests/dtget-test.dts   |   63 +++++++++++++++
 tests/dtput-runtest.sh |   50 ++++++++++++
 tests/run_tests.sh     |   73 +++++++++++++++++-
 tests/tests.sh         |    2 +
 util.c                 |   28 +++++++
 util.h                 |   11 +++
 utilfdt.c              |  113 ++++++++++++++++++++++++++
 utilfdt.h              |   58 ++++++++++++++
 18 files changed, 848 insertions(+), 27 deletions(-)
 create mode 100644 Makefile.dtget
 create mode 100644 Makefile.dtput
 create mode 100644 dtget.c
 create mode 100644 dtput.c
 create mode 100755 tests/dtget-runtest.sh
 create mode 100644 tests/dtget-test.dts
 create mode 100644 tests/dtput-runtest.sh
 create mode 100644 utilfdt.c
 create mode 100644 utilfdt.h

-- 
1.7.3.1

[PATCH 1/9] Split out is_printable_string() into util.c

[Simon Glass]

This useful function is split out so it will be available to programs
other than ftdump.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 Makefile.ftdump |    3 ++-
 ftdump.c        |   28 +++-------------------------
 util.c          |   28 ++++++++++++++++++++++++++++
 util.h          |   11 +++++++++++
 4 files changed, 44 insertions(+), 26 deletions(-)

diff --git a/Makefile.ftdump b/Makefile.ftdump
index b70905a..2744a18 100644
--- a/Makefile.ftdump
+++ b/Makefile.ftdump
@@ -5,7 +5,8 @@
 #
 
 FTDUMP_SRCS = \
-	ftdump.c
+	ftdump.c \
+	util.c
 
 FTDUMP_GEN_SRCS =
 
diff --git a/ftdump.c b/ftdump.c
index bce6535..db932e3 100644
--- a/ftdump.c
+++ b/ftdump.c
@@ -11,36 +11,14 @@
 #include <fdt.h>
 #include <libfdt_env.h>
 
+#include "util.h"
+
 #define FTDUMP_BUF_SIZE	65536
 
 #define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
 #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
 #define GET_CELL(p)	(p += 4, *((const uint32_t *)(p-4)))
 
-static int is_printable_string(const void *data, int len)
-{
-	const char *s = data;
-	const char *ss;
-
-	/* zero length is not */
-	if (len == 0)
-		return 0;
-
-	/* must terminate with zero */
-	if (s[len - 1] != '\0')
-		return 0;
-
-	ss = s;
-	while (*s && isprint(*s))
-		s++;
-
-	/* not zero, or not done yet */
-	if (*s != '\0' || (s + 1 - ss) < len)
-		return 0;
-
-	return 1;
-}
-
 static void print_data(const char *data, int len)
 {
 	int i;
@@ -50,7 +28,7 @@ static void print_data(const char *data, int len)
 	if (len == 0)
 		return;
 
-	if (is_printable_string(data, len)) {
+	if (util_is_printable_string(data, len)) {
 		printf(" = \"%s\"", (const char *)data);
 	} else if ((len % 4) == 0) {
 		printf(" = <");
diff --git a/util.c b/util.c
index d7ac27d..994436f 100644
--- a/util.c
+++ b/util.c
@@ -1,6 +1,9 @@
 /*
  * Copyright 2008 Jon Loeliger, Freescale Semiconductor, Inc.
  *
+ * util_is_printable_string contributed by
+ *	Pantelis Antoniou <pantelis.antoniou AT gmail.com>
+ *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
  * published by the Free Software Foundation; either version 2 of the
@@ -17,6 +20,7 @@
  *                                                                   USA
  */
 
+#include <ctype.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
@@ -57,3 +61,27 @@ char *join_path(const char *path, const char *name)
 	memcpy(str+lenp, name, lenn+1);
 	return str;
 }
+
+int util_is_printable_string(const void *data, int len)
+{
+	const char *s = data;
+	const char *ss;
+
+	/* zero length is not */
+	if (len == 0)
+		return 0;
+
+	/* must terminate with zero */
+	if (s[len - 1] != '\0')
+		return 0;
+
+	ss = s;
+	while (*s && isprint(*s))
+		s++;
+
+	/* not zero, or not done yet */
+	if (*s != '\0' || (s + 1 - ss) < len)
+		return 0;
+
+	return 1;
+}
diff --git a/util.h b/util.h
index 9cead84..cc68933 100644
--- a/util.h
+++ b/util.h
@@ -1,6 +1,8 @@
 #ifndef _UTIL_H
 #define _UTIL_H
 
+#include <stdarg.h>
+
 /*
  * Copyright 2008 Jon Loeliger, Freescale Semiconductor, Inc.
  *
@@ -53,4 +55,13 @@ static inline void *xrealloc(void *p, size_t len)
 extern char *xstrdup(const char *s);
 extern char *join_path(const char *path, const char *name);
 
+/**
+ * Check a string of a given length to see if it is all printable and
+ * has a valid terminator.
+ *
+ * @param data	The string to check
+ * @param len	The string length including terminator
+ * @return 1 if a valid printable string, 0 if not */
+int util_is_printable_string(const void *data, int len);
+
 #endif /* _UTIL_H */
-- 
1.7.3.1

Re: [PATCH 1/9] Split out is_printable_string() into util.c

[Grant Likely]

On Tue, Jul 05, 2011 at 12:02:49PM -0700, Simon Glass wrote:
> This useful function is split out so it will be available to programs
> other than ftdump.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

> ---
>  Makefile.ftdump |    3 ++-
>  ftdump.c        |   28 +++-------------------------
>  util.c          |   28 ++++++++++++++++++++++++++++
>  util.h          |   11 +++++++++++
>  4 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/Makefile.ftdump b/Makefile.ftdump
> index b70905a..2744a18 100644
> --- a/Makefile.ftdump
> +++ b/Makefile.ftdump
> @@ -5,7 +5,8 @@
>  #
>  
>  FTDUMP_SRCS = \
> -	ftdump.c
> +	ftdump.c \
> +	util.c
>  
>  FTDUMP_GEN_SRCS =
>  
> diff --git a/ftdump.c b/ftdump.c
> index bce6535..db932e3 100644
> --- a/ftdump.c
> +++ b/ftdump.c
> @@ -11,36 +11,14 @@
>  #include <fdt.h>
>  #include <libfdt_env.h>
>  
> +#include "util.h"
> +
>  #define FTDUMP_BUF_SIZE	65536
>  
>  #define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
>  #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
>  #define GET_CELL(p)	(p += 4, *((const uint32_t *)(p-4)))
>  
> -static int is_printable_string(const void *data, int len)
> -{
> -	const char *s = data;
> -	const char *ss;
> -
> -	/* zero length is not */
> -	if (len == 0)
> -		return 0;
> -
> -	/* must terminate with zero */
> -	if (s[len - 1] != '\0')
> -		return 0;
> -
> -	ss = s;
> -	while (*s && isprint(*s))
> -		s++;
> -
> -	/* not zero, or not done yet */
> -	if (*s != '\0' || (s + 1 - ss) < len)
> -		return 0;
> -
> -	return 1;
> -}
> -
>  static void print_data(const char *data, int len)
>  {
>  	int i;
> @@ -50,7 +28,7 @@ static void print_data(const char *data, int len)
>  	if (len == 0)
>  		return;
>  
> -	if (is_printable_string(data, len)) {
> +	if (util_is_printable_string(data, len)) {
>  		printf(" = \"%s\"", (const char *)data);
>  	} else if ((len % 4) == 0) {
>  		printf(" = <");
> diff --git a/util.c b/util.c
> index d7ac27d..994436f 100644
> --- a/util.c
> +++ b/util.c
> @@ -1,6 +1,9 @@
>  /*
>   * Copyright 2008 Jon Loeliger, Freescale Semiconductor, Inc.
>   *
> + * util_is_printable_string contributed by
> + *	Pantelis Antoniou <pantelis.antoniou AT gmail.com>
> + *
>   * This program is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU General Public License as
>   * published by the Free Software Foundation; either version 2 of the
> @@ -17,6 +20,7 @@
>   *                                                                   USA
>   */
>  
> +#include <ctype.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stdarg.h>
> @@ -57,3 +61,27 @@ char *join_path(const char *path, const char *name)
>  	memcpy(str+lenp, name, lenn+1);
>  	return str;
>  }
> +
> +int util_is_printable_string(const void *data, int len)
> +{
> +	const char *s = data;
> +	const char *ss;
> +
> +	/* zero length is not */
> +	if (len == 0)
> +		return 0;
> +
> +	/* must terminate with zero */
> +	if (s[len - 1] != '\0')
> +		return 0;
> +
> +	ss = s;
> +	while (*s && isprint(*s))
> +		s++;
> +
> +	/* not zero, or not done yet */
> +	if (*s != '\0' || (s + 1 - ss) < len)
> +		return 0;
> +
> +	return 1;
> +}
> diff --git a/util.h b/util.h
> index 9cead84..cc68933 100644
> --- a/util.h
> +++ b/util.h
> @@ -1,6 +1,8 @@
>  #ifndef _UTIL_H
>  #define _UTIL_H
>  
> +#include <stdarg.h>
> +
>  /*
>   * Copyright 2008 Jon Loeliger, Freescale Semiconductor, Inc.
>   *
> @@ -53,4 +55,13 @@ static inline void *xrealloc(void *p, size_t len)
>  extern char *xstrdup(const char *s);
>  extern char *join_path(const char *path, const char *name);
>  
> +/**
> + * Check a string of a given length to see if it is all printable and
> + * has a valid terminator.
> + *
> + * @param data	The string to check
> + * @param len	The string length including terminator
> + * @return 1 if a valid printable string, 0 if not */
> +int util_is_printable_string(const void *data, int len);
> +
>  #endif /* _UTIL_H */
> -- 
> 1.7.3.1
> 

Re: [PATCH 1/9] Split out is_printable_string() into util.c

[David Gibson]

On Tue, Jul 05, 2011 at 12:02:49PM -0700, Simon Glass wrote:
> This useful function is split out so it will be available to programs
> other than ftdump.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 1/9] Split out is_printable_string() into util.c

[Jon Loeliger]

> On Tue, Jul 05, 2011 at 12:02:49PM -0700, Simon Glass wrote:
> > This useful function is split out so it will be available to programs
> > other than ftdump.
> > 
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Applied.

jdl

[PATCH 2/9] Add dtget utility to read property values from device tree

[Simon Glass]

This simply utility makes it easy for scripts to read values from the device
tree.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 .gitignore     |    1 +
 Makefile       |    7 ++
 Makefile.dtget |   13 ++++
 dtget.c        |  207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 228 insertions(+), 0 deletions(-)
 create mode 100644 Makefile.dtget
 create mode 100644 dtget.c

diff --git a/.gitignore b/.gitignore
index ae7a46a..cde4fa5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -10,3 +10,4 @@ lex.yy.c
 /ftdump
 /convert-dtsv0
 /version_gen.h
+/dtget
diff --git a/Makefile b/Makefile
index 22f79a0..3401a65 100644
--- a/Makefile
+++ b/Makefile
@@ -106,10 +106,12 @@ endef
 include Makefile.convert-dtsv0
 include Makefile.dtc
 include Makefile.ftdump
+include Makefile.dtget
 
 BIN += convert-dtsv0
 BIN += dtc
 BIN += ftdump
+BIN += dtget
 
 SCRIPTS = dtdiff
 
@@ -120,6 +122,7 @@ ifneq ($(DEPTARGETS),)
 -include $(DTC_OBJS:%.o=%.d)
 -include $(CONVERT_OBJS:%.o=%.d)
 -include $(FTDUMP_OBJS:%.o=%.d)
+-include $(DTGET_OBJS:%.o=%.d)
 endif
 
 
@@ -180,6 +183,10 @@ convert-dtsv0: $(CONVERT_OBJS)
 
 ftdump:	$(FTDUMP_OBJS)
 
+dtget:	$(DTGET_OBJS) $(LIBFDT_archive)
+	$(VECHO) LD $@
+	$(LINK.c) -o $@ $^
+
 
 #
 # Testsuite rules
diff --git a/Makefile.dtget b/Makefile.dtget
new file mode 100644
index 0000000..edda499
--- /dev/null
+++ b/Makefile.dtget
@@ -0,0 +1,13 @@
+#
+# This is not a complete Makefile.
+# Instead, it is designed to be easily embeddable
+# into other systems of Makefiles.
+#
+
+DTGET_SRCS = \
+	dtget.c \
+	util.c
+
+DTGET_GEN_SRCS =
+
+DTGET_OBJS = $(DTGET_SRCS:%.c=%.o) $(DTGET_GEN_SRCS:%.c=%.o)
diff --git a/dtget.c b/dtget.c
new file mode 100644
index 0000000..0ee577c
--- /dev/null
+++ b/dtget.c
@@ -0,0 +1,207 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+ * Distributed under the terms of the GNU General Public License v2
+ *
+ * is_printable_string from ftdump.c
+ * Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <getopt.h>
+#include <ctype.h>
+
+#include <libfdt.h>
+
+#include "util.h"
+
+#define FTDUMP_BUF_SIZE	65536
+
+static void report_error(const char *where, int err)
+{
+	fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
+}
+
+static void show_data(const char *format, int type, const char *data, int len)
+{
+	int i, size;
+	const char *p = data, *s;
+	int value;
+	int is_string;
+
+	/* no data, don't print */
+	if (len == 0)
+		return;
+
+	is_string = type == 's' ||
+		(!type && util_is_printable_string(data, len));
+	if (is_string) {
+		for (s = data; s - data < len; s += strlen(s) + 1) {
+			if (s != data)
+				printf(" ");
+			printf(format ? format : "%s", (const char *)s);
+		}
+		return;
+	}
+	if (type == 'i')
+		size = 4;
+	else if (type == 'b')
+		size = 1;
+	else
+		size = len % 4 == 0 ? 4 : 1;
+	for (i = 0; i < len; i += size, p += size) {
+		if (i)
+			printf(" ");
+		value = size == 4 ? fdt32_to_cpu(*(const uint32_t *)p) :
+			*(const unsigned char *)p;
+		printf(format ? format : "%d", value);
+	}
+}
+
+static int show_data_for_key(const void *blob, const char *format, int type,
+			     char *key)
+{
+	char *ptr, *prop;
+	int node, guess_node = 0;
+	const void *value;
+	int len;
+
+	/* First extract the property from the end */
+	ptr = strrchr(key, '/');
+	if (!ptr) {
+		fprintf(stderr, "No node found in key '%s'\n", key);
+		return 5;
+	}
+
+	if (ptr == key) {
+		guess_node = fdt_path_offset(blob, key);
+		node = fdt_path_offset(blob, "/");
+	} else {
+		*ptr = '\0';
+		node = fdt_path_offset(blob, key);
+		*ptr = '/';
+	}
+	if (node < 0) {
+		report_error(key, node);
+		return 7;
+	}
+	prop = ptr + 1;
+	value = fdt_getprop(blob, node, prop, &len);
+	if (!value) {
+		report_error(prop, len);
+		if (guess_node)
+			fprintf(stderr, "(Node %s exists but you didn't "
+				"specify a property to print)\n", key);
+		return 8;
+	}
+	show_data(format, type, value, len);
+	printf("\n");
+	return 0;
+}
+
+static int do_dtget(const char *filename, const char *format, int type,
+		    char **key, int key_count)
+{
+	FILE *fp;
+	char *buf;
+	size_t size;
+	int i, ret;
+
+	if (strcmp(filename, "-") == 0) {
+		fp = stdin;
+	} else {
+		fp = fopen(filename, "rb");
+		if (fp == NULL) {
+			fprintf(stderr, "unable to open %s\n", filename);
+			return 10;
+		}
+	}
+
+	buf = malloc(FTDUMP_BUF_SIZE);
+	if (!buf) {
+		fprintf(stderr, "Couldn't allocate %d byte buffer\n",
+			FTDUMP_BUF_SIZE);
+		return 10;
+	}
+
+	size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
+	if (!feof(fp)) {
+		fprintf(stderr, "file too large (maximum is %d bytes)\n",
+			FTDUMP_BUF_SIZE);
+		return 10;
+	}
+	if (ferror(fp)) {
+		fprintf(stderr, "file read error\n");
+		return 10;
+	}
+	for (i = 0; i < key_count; i++) {
+		ret = show_data_for_key(buf, format, type, key[i]);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static void usage(const char *msg)
+{
+	if (msg)
+		fprintf(stderr, "Error: %s\n\n", msg);
+	fprintf(stderr, "dtget - read values from device tree\n\n");
+	fprintf(stderr, "Usage: dtget <options> <dt file> <key>...\n");
+	fprintf(stderr, "Options:\n");
+	fprintf(stderr, "\t-f, --format <fmt>\tPrintf format string to use "
+		"for value\n");
+	fprintf(stderr, "\t-t, --type <typechar>\tForce type to be string "
+		"(s), int(i) or byte(b)\n");
+	fprintf(stderr, "\t-h, --help\t\tPrint this help\n\n");
+	fprintf(stderr, "\t<key> is <node>/<property>, for example "
+		"/lcd/width for the width\n");
+	fprintf(stderr, "\tproperty in the LCD node\n");
+	exit(2);
+}
+
+int main(int argc, char *argv[])
+{
+	int c;
+	int ret;
+	char *format = NULL;
+	int type = 0;
+	char *filename = NULL;
+	static struct option long_options[] = {
+		{"format", 1, 0, 'f'},
+		{"help", 1, 0, 'h'},
+		{"type", 1, 0, 't'},
+		{0, 0, 0, 0}
+	};
+
+	for (;;) {
+		c = getopt_long(argc, argv, "f:ht:", long_options, NULL);
+		if (c == -1)
+			break;
+
+		switch (c) {
+		case 'h':
+		case '?':
+			usage("");
+
+		case 'f':
+			format = optarg;
+			break;
+
+		case 't':
+			type = *optarg;
+			break;
+		}
+	}
+
+	if (optind < argc)
+		filename = argv[optind++];
+	if (!filename)
+		usage("Missing filename");
+	if (optind == argc)
+		usage("Missing key(s)");
+
+	ret = do_dtget(filename, format, type, argv + optind, argc - optind);
+	return ret;
+}
-- 
1.7.3.1

Re: [PATCH 2/9] Add dtget utility to read property values from device tree

[Grant Likely]

On Tue, Jul 05, 2011 at 12:02:50PM -0700, Simon Glass wrote:
> This simply utility makes it easy for scripts to read values from the device
> tree.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Looks okay to me.

g.

> ---
>  .gitignore     |    1 +
>  Makefile       |    7 ++
>  Makefile.dtget |   13 ++++
>  dtget.c        |  207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 228 insertions(+), 0 deletions(-)
>  create mode 100644 Makefile.dtget
>  create mode 100644 dtget.c
> 
> diff --git a/.gitignore b/.gitignore
> index ae7a46a..cde4fa5 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -10,3 +10,4 @@ lex.yy.c
>  /ftdump
>  /convert-dtsv0
>  /version_gen.h
> +/dtget
> diff --git a/Makefile b/Makefile
> index 22f79a0..3401a65 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -106,10 +106,12 @@ endef
>  include Makefile.convert-dtsv0
>  include Makefile.dtc
>  include Makefile.ftdump
> +include Makefile.dtget
>  
>  BIN += convert-dtsv0
>  BIN += dtc
>  BIN += ftdump
> +BIN += dtget
>  
>  SCRIPTS = dtdiff
>  
> @@ -120,6 +122,7 @@ ifneq ($(DEPTARGETS),)
>  -include $(DTC_OBJS:%.o=%.d)
>  -include $(CONVERT_OBJS:%.o=%.d)
>  -include $(FTDUMP_OBJS:%.o=%.d)
> +-include $(DTGET_OBJS:%.o=%.d)
>  endif
>  
>  
> @@ -180,6 +183,10 @@ convert-dtsv0: $(CONVERT_OBJS)
>  
>  ftdump:	$(FTDUMP_OBJS)
>  
> +dtget:	$(DTGET_OBJS) $(LIBFDT_archive)
> +	$(VECHO) LD $@
> +	$(LINK.c) -o $@ $^
> +
>  
>  #
>  # Testsuite rules
> diff --git a/Makefile.dtget b/Makefile.dtget
> new file mode 100644
> index 0000000..edda499
> --- /dev/null
> +++ b/Makefile.dtget
> @@ -0,0 +1,13 @@
> +#
> +# This is not a complete Makefile.
> +# Instead, it is designed to be easily embeddable
> +# into other systems of Makefiles.
> +#
> +
> +DTGET_SRCS = \
> +	dtget.c \
> +	util.c
> +
> +DTGET_GEN_SRCS =
> +
> +DTGET_OBJS = $(DTGET_SRCS:%.c=%.o) $(DTGET_GEN_SRCS:%.c=%.o)
> diff --git a/dtget.c b/dtget.c
> new file mode 100644
> index 0000000..0ee577c
> --- /dev/null
> +++ b/dtget.c
> @@ -0,0 +1,207 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> + * Distributed under the terms of the GNU General Public License v2
> + *
> + * is_printable_string from ftdump.c
> + * Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <getopt.h>
> +#include <ctype.h>
> +
> +#include <libfdt.h>
> +
> +#include "util.h"
> +
> +#define FTDUMP_BUF_SIZE	65536
> +
> +static void report_error(const char *where, int err)
> +{
> +	fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
> +}
> +
> +static void show_data(const char *format, int type, const char *data, int len)
> +{
> +	int i, size;
> +	const char *p = data, *s;
> +	int value;
> +	int is_string;
> +
> +	/* no data, don't print */
> +	if (len == 0)
> +		return;
> +
> +	is_string = type == 's' ||
> +		(!type && util_is_printable_string(data, len));
> +	if (is_string) {
> +		for (s = data; s - data < len; s += strlen(s) + 1) {
> +			if (s != data)
> +				printf(" ");
> +			printf(format ? format : "%s", (const char *)s);
> +		}
> +		return;
> +	}
> +	if (type == 'i')
> +		size = 4;
> +	else if (type == 'b')
> +		size = 1;
> +	else
> +		size = len % 4 == 0 ? 4 : 1;
> +	for (i = 0; i < len; i += size, p += size) {
> +		if (i)
> +			printf(" ");
> +		value = size == 4 ? fdt32_to_cpu(*(const uint32_t *)p) :
> +			*(const unsigned char *)p;
> +		printf(format ? format : "%d", value);
> +	}
> +}
> +
> +static int show_data_for_key(const void *blob, const char *format, int type,
> +			     char *key)
> +{
> +	char *ptr, *prop;
> +	int node, guess_node = 0;
> +	const void *value;
> +	int len;
> +
> +	/* First extract the property from the end */
> +	ptr = strrchr(key, '/');
> +	if (!ptr) {
> +		fprintf(stderr, "No node found in key '%s'\n", key);
> +		return 5;
> +	}
> +
> +	if (ptr == key) {
> +		guess_node = fdt_path_offset(blob, key);
> +		node = fdt_path_offset(blob, "/");
> +	} else {
> +		*ptr = '\0';
> +		node = fdt_path_offset(blob, key);
> +		*ptr = '/';
> +	}
> +	if (node < 0) {
> +		report_error(key, node);
> +		return 7;
> +	}
> +	prop = ptr + 1;
> +	value = fdt_getprop(blob, node, prop, &len);
> +	if (!value) {
> +		report_error(prop, len);
> +		if (guess_node)
> +			fprintf(stderr, "(Node %s exists but you didn't "
> +				"specify a property to print)\n", key);
> +		return 8;
> +	}
> +	show_data(format, type, value, len);
> +	printf("\n");
> +	return 0;
> +}
> +
> +static int do_dtget(const char *filename, const char *format, int type,
> +		    char **key, int key_count)
> +{
> +	FILE *fp;
> +	char *buf;
> +	size_t size;
> +	int i, ret;
> +
> +	if (strcmp(filename, "-") == 0) {
> +		fp = stdin;
> +	} else {
> +		fp = fopen(filename, "rb");
> +		if (fp == NULL) {
> +			fprintf(stderr, "unable to open %s\n", filename);
> +			return 10;
> +		}
> +	}
> +
> +	buf = malloc(FTDUMP_BUF_SIZE);
> +	if (!buf) {
> +		fprintf(stderr, "Couldn't allocate %d byte buffer\n",
> +			FTDUMP_BUF_SIZE);
> +		return 10;
> +	}
> +
> +	size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
> +	if (!feof(fp)) {
> +		fprintf(stderr, "file too large (maximum is %d bytes)\n",
> +			FTDUMP_BUF_SIZE);
> +		return 10;
> +	}
> +	if (ferror(fp)) {
> +		fprintf(stderr, "file read error\n");
> +		return 10;
> +	}
> +	for (i = 0; i < key_count; i++) {
> +		ret = show_data_for_key(buf, format, type, key[i]);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +static void usage(const char *msg)
> +{
> +	if (msg)
> +		fprintf(stderr, "Error: %s\n\n", msg);
> +	fprintf(stderr, "dtget - read values from device tree\n\n");
> +	fprintf(stderr, "Usage: dtget <options> <dt file> <key>...\n");
> +	fprintf(stderr, "Options:\n");
> +	fprintf(stderr, "\t-f, --format <fmt>\tPrintf format string to use "
> +		"for value\n");
> +	fprintf(stderr, "\t-t, --type <typechar>\tForce type to be string "
> +		"(s), int(i) or byte(b)\n");
> +	fprintf(stderr, "\t-h, --help\t\tPrint this help\n\n");
> +	fprintf(stderr, "\t<key> is <node>/<property>, for example "
> +		"/lcd/width for the width\n");
> +	fprintf(stderr, "\tproperty in the LCD node\n");
> +	exit(2);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int c;
> +	int ret;
> +	char *format = NULL;
> +	int type = 0;
> +	char *filename = NULL;
> +	static struct option long_options[] = {
> +		{"format", 1, 0, 'f'},
> +		{"help", 1, 0, 'h'},
> +		{"type", 1, 0, 't'},
> +		{0, 0, 0, 0}
> +	};
> +
> +	for (;;) {
> +		c = getopt_long(argc, argv, "f:ht:", long_options, NULL);
> +		if (c == -1)
> +			break;
> +
> +		switch (c) {
> +		case 'h':
> +		case '?':
> +			usage("");
> +
> +		case 'f':
> +			format = optarg;
> +			break;
> +
> +		case 't':
> +			type = *optarg;
> +			break;
> +		}
> +	}
> +
> +	if (optind < argc)
> +		filename = argv[optind++];
> +	if (!filename)
> +		usage("Missing filename");
> +	if (optind == argc)
> +		usage("Missing key(s)");
> +
> +	ret = do_dtget(filename, format, type, argv + optind, argc - optind);
> +	return ret;
> +}
> -- 
> 1.7.3.1
> 

Re: [PATCH 2/9] Add dtget utility to read property values from device tree

[David Gibson]

On Tue, Jul 05, 2011 at 12:02:50PM -0700, Simon Glass wrote:
> This simply utility makes it easy for scripts to read values from the device
> tree.

This commit message needs more information.  Command line syntax,
expected use cases, ..

> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  .gitignore     |    1 +
>  Makefile       |    7 ++
>  Makefile.dtget |   13 ++++
>  dtget.c        |  207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++

I'd prefer to see it called fdtget, to match the naming of the library
and its functions.  The utility is, after all, specific to the
flattened tree format.

>  4 files changed, 228 insertions(+), 0 deletions(-)
>  create mode 100644 Makefile.dtget
>  create mode 100644 dtget.c
> 
> diff --git a/.gitignore b/.gitignore
> index ae7a46a..cde4fa5 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -10,3 +10,4 @@ lex.yy.c
>  /ftdump
>  /convert-dtsv0
>  /version_gen.h
> +/dtget
> diff --git a/Makefile b/Makefile
> index 22f79a0..3401a65 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -106,10 +106,12 @@ endef
>  include Makefile.convert-dtsv0
>  include Makefile.dtc
>  include Makefile.ftdump
> +include Makefile.dtget
>  
>  BIN += convert-dtsv0
>  BIN += dtc
>  BIN += ftdump
> +BIN += dtget
>  
>  SCRIPTS = dtdiff
>  
> @@ -120,6 +122,7 @@ ifneq ($(DEPTARGETS),)
>  -include $(DTC_OBJS:%.o=%.d)
>  -include $(CONVERT_OBJS:%.o=%.d)
>  -include $(FTDUMP_OBJS:%.o=%.d)
> +-include $(DTGET_OBJS:%.o=%.d)
>  endif
>  
>  
> @@ -180,6 +183,10 @@ convert-dtsv0: $(CONVERT_OBJS)
>  
>  ftdump:	$(FTDUMP_OBJS)
>  
> +dtget:	$(DTGET_OBJS) $(LIBFDT_archive)
> +	$(VECHO) LD $@
> +	$(LINK.c) -o $@ $^
> +
>  
>  #
>  # Testsuite rules
> diff --git a/Makefile.dtget b/Makefile.dtget
> new file mode 100644
> index 0000000..edda499
> --- /dev/null
> +++ b/Makefile.dtget
> @@ -0,0 +1,13 @@
> +#
> +# This is not a complete Makefile.
> +# Instead, it is designed to be easily embeddable
> +# into other systems of Makefiles.
> +#
> +
> +DTGET_SRCS = \
> +	dtget.c \
> +	util.c
> +
> +DTGET_GEN_SRCS =

You have no generated sources, nor are you likely to in future, so you
don't need this.

> +DTGET_OBJS = $(DTGET_SRCS:%.c=%.o) $(DTGET_GEN_SRCS:%.c=%.o)
> diff --git a/dtget.c b/dtget.c
> new file mode 100644
> index 0000000..0ee577c
> --- /dev/null
> +++ b/dtget.c
> @@ -0,0 +1,207 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> + * Distributed under the terms of the GNU General Public License v2
> + *
> + * is_printable_string from ftdump.c
> + * Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <getopt.h>
> +#include <ctype.h>
> +
> +#include <libfdt.h>
> +
> +#include "util.h"
> +
> +#define FTDUMP_BUF_SIZE	65536

So, first of all the name of this constant is clearly wrong.  But more
importantly, you shouldn't use a fixed size buffer like this.  ftdump
makes no pretense to be anything more than a debugging hack, so it can
get away with it.  dtget is supposed to be a real utility and should
handle arbitrary sized fdt input.

> +static void report_error(const char *where, int err)
> +{
> +	fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
> +}
> +
> +static void show_data(const char *format, int type, const char *data, int len)
> +{
> +	int i, size;
> +	const char *p = data, *s;
> +	int value;
> +	int is_string;
> +
> +	/* no data, don't print */
> +	if (len == 0)
> +		return;
> +
> +	is_string = type == 's' ||
                    ^^^^^^^^^^^
parens here please to make it easier to visually parse out from the
assignment.

> +		(!type && util_is_printable_string(data, len));
> +	if (is_string) {
> +		for (s = data; s - data < len; s += strlen(s) + 1) {
> +			if (s != data)
> +				printf(" ");
> +			printf(format ? format : "%s", (const char *)s);
> +		}
> +		return;
> +	}
> +	if (type == 'i')
> +		size = 4;
> +	else if (type == 'b')
> +		size = 1;
> +	else
> +		size = len % 4 == 0 ? 4 : 1;
                       ^^^^^^^^^^^^
parens for clarity here please.

> +	for (i = 0; i < len; i += size, p += size) {
> +		if (i)
> +			printf(" ");
> +		value = size == 4 ? fdt32_to_cpu(*(const uint32_t *)p) :
> +			*(const unsigned char *)p;
> +		printf(format ? format : "%d", value);
> +	}
> +}
> +
> +static int show_data_for_key(const void *blob, const char *format, int type,
> +			     char *key)
> +{
> +	char *ptr, *prop;
> +	int node, guess_node = 0;
> +	const void *value;
> +	int len;
> +
> +	/* First extract the property from the end */
> +	ptr = strrchr(key, '/');
> +	if (!ptr) {
> +		fprintf(stderr, "No node found in key '%s'\n", key);
> +		return 5;
> +	}

This is wrong.  properties do not have paths - they exist in a
separate namespace to nodes.  The node path and property name should
be supplied as separate parameters.  As well as being actually right,
it will greatly simplify this function's convoluted logic.


> +	if (ptr == key) {
> +		guess_node = fdt_path_offset(blob, key);
> +		node = fdt_path_offset(blob, "/");
> +	} else {
> +		*ptr = '\0';
> +		node = fdt_path_offset(blob, key);
> +		*ptr = '/';
> +	}
> +	if (node < 0) {
> +		report_error(key, node);
> +		return 7;

Open-coded arbitrary error codes?  Wtf?

> +	}
> +	prop = ptr + 1;
> +	value = fdt_getprop(blob, node, prop, &len);
> +	if (!value) {
> +		report_error(prop, len);
> +		if (guess_node)
> +			fprintf(stderr, "(Node %s exists but you didn't "
> +				"specify a property to print)\n", key);

Uh.. no.  If the fdt_path_offset() which assigned guess_node failed,
guess_node would be negative and therefore true.  guess_node will only
be zero if fdt_path_offset() returned zero, which should only happen
if key == "/".  Remember zero is a valid node offset referring to the
root node.

> +		return 8;
> +	}
> +	show_data(format, type, value, len);
> +	printf("\n");
> +	return 0;
> +}
> +
> +static int do_dtget(const char *filename, const char *format, int type,
> +		    char **key, int key_count)
> +{
> +	FILE *fp;
> +	char *buf;
> +	size_t size;
> +	int i, ret;
> +
> +	if (strcmp(filename, "-") == 0) {
> +		fp = stdin;
> +	} else {
> +		fp = fopen(filename, "rb");
> +		if (fp == NULL) {
> +			fprintf(stderr, "unable to open %s\n", filename);
> +			return 10;
> +		}
> +	}
> +
> +	buf = malloc(FTDUMP_BUF_SIZE);
> +	if (!buf) {
> +		fprintf(stderr, "Couldn't allocate %d byte buffer\n",
> +			FTDUMP_BUF_SIZE);
> +		return 10;
> +	}
> +
> +	size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
> +	if (!feof(fp)) {
> +		fprintf(stderr, "file too large (maximum is %d bytes)\n",
> +			FTDUMP_BUF_SIZE);
> +		return 10;
> +	}
> +	if (ferror(fp)) {
> +		fprintf(stderr, "file read error\n");
> +		return 10;
> +	}
> +	for (i = 0; i < key_count; i++) {
> +		ret = show_data_for_key(buf, format, type, key[i]);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +static void usage(const char *msg)
> +{
> +	if (msg)
> +		fprintf(stderr, "Error: %s\n\n", msg);
> +	fprintf(stderr, "dtget - read values from device tree\n\n");
> +	fprintf(stderr, "Usage: dtget <options> <dt file> <key>...\n");
> +	fprintf(stderr, "Options:\n");
> +	fprintf(stderr, "\t-f, --format <fmt>\tPrintf format string to use "
> +		"for value\n");
> +	fprintf(stderr, "\t-t, --type <typechar>\tForce type to be string "
> +		"(s), int(i) or byte(b)\n");
> +	fprintf(stderr, "\t-h, --help\t\tPrint this help\n\n");
> +	fprintf(stderr, "\t<key> is <node>/<property>, for example "
> +		"/lcd/width for the width\n");

DO NOT use this node/property format.

Also bare /lcd is a fairly improbably path for a node.

> +	fprintf(stderr, "\tproperty in the LCD node\n");
> +	exit(2);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int c;
> +	int ret;
> +	char *format = NULL;
> +	int type = 0;
> +	char *filename = NULL;
> +	static struct option long_options[] = {
> +		{"format", 1, 0, 'f'},
> +		{"help", 1, 0, 'h'},
> +		{"type", 1, 0, 't'},
> +		{0, 0, 0, 0}
> +	};
> +
> +	for (;;) {
> +		c = getopt_long(argc, argv, "f:ht:", long_options, NULL);
> +		if (c == -1)
> +			break;

Hrm.  using getopt_long() introduces a new dependency on GNU libc
which I'm not thrilled about.

> +
> +		switch (c) {
> +		case 'h':
> +		case '?':
> +			usage("");
> +
> +		case 'f':
> +			format = optarg;
> +			break;
> +
> +		case 't':
> +			type = *optarg;
> +			break;
> +		}
> +	}

Hrm.  I really dislike your mechanism for choosing the output format.
It's ad-hoc, doesn't really match any existing conventions and without
forcing the format, potentially ambiguous.  Translating \0s into
spaces in stringlist propertys is particularly nasty.  Decimal by
default is also rarely the right choice.  Oh, and the fact that you
could SEGV the utility by giving it a bad format string, since that's
never validated, is really sloppy.

I'd need to know more about likely use cases to suggest a better way
of handling the formatting though.

> +	if (optind < argc)
> +		filename = argv[optind++];
> +	if (!filename)
> +		usage("Missing filename");
> +	if (optind == argc)
> +		usage("Missing key(s)");
> +
> +	ret = do_dtget(filename, format, type, argv + optind, argc - optind);
> +	return ret;
> +}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 2/9] Add dtget utility to read property values from device tree

[Simon Glass]

Hi David,

On Tue, Jul 19, 2011 at 3:34 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Tue, Jul 05, 2011 at 12:02:50PM -0700, Simon Glass wrote:
>> This simply utility makes it easy for scripts to read values from the device
>> tree.
>
> This commit message needs more information.  Command line syntax,
> expected use cases, ..

OK I will come up with something.

>
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  .gitignore     |    1 +
>>  Makefile       |    7 ++
>>  Makefile.dtget |   13 ++++
>>  dtget.c        |  207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>
> I'd prefer to see it called fdtget, to match the naming of the library
> and its functions.  The utility is, after all, specific to the
> flattened tree format.

A bit of a mouthful, but ok.

fdtget
fdtput

Also is put better than set? I didn't like set since it implied
setting an existing value whereas we are potentially putting a new
value into the tree.

>
>>  4 files changed, 228 insertions(+), 0 deletions(-)
>>  create mode 100644 Makefile.dtget
>>  create mode 100644 dtget.c
>>
>> diff --git a/.gitignore b/.gitignore
>> index ae7a46a..cde4fa5 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -10,3 +10,4 @@ lex.yy.c
>>  /ftdump
>>  /convert-dtsv0
>>  /version_gen.h
>> +/dtget
>> diff --git a/Makefile b/Makefile
>> index 22f79a0..3401a65 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -106,10 +106,12 @@ endef
>>  include Makefile.convert-dtsv0
>>  include Makefile.dtc
>>  include Makefile.ftdump
>> +include Makefile.dtget
>>
>>  BIN += convert-dtsv0
>>  BIN += dtc
>>  BIN += ftdump
>> +BIN += dtget
>>
>>  SCRIPTS = dtdiff
>>
>> @@ -120,6 +122,7 @@ ifneq ($(DEPTARGETS),)
>>  -include $(DTC_OBJS:%.o=%.d)
>>  -include $(CONVERT_OBJS:%.o=%.d)
>>  -include $(FTDUMP_OBJS:%.o=%.d)
>> +-include $(DTGET_OBJS:%.o=%.d)
>>  endif
>>
>>
>> @@ -180,6 +183,10 @@ convert-dtsv0: $(CONVERT_OBJS)
>>
>>  ftdump:      $(FTDUMP_OBJS)
>>
>> +dtget:       $(DTGET_OBJS) $(LIBFDT_archive)
>> +     $(VECHO) LD $@
>> +     $(LINK.c) -o $@ $^
>> +
>>
>>  #
>>  # Testsuite rules
>> diff --git a/Makefile.dtget b/Makefile.dtget
>> new file mode 100644
>> index 0000000..edda499
>> --- /dev/null
>> +++ b/Makefile.dtget
>> @@ -0,0 +1,13 @@
>> +#
>> +# This is not a complete Makefile.
>> +# Instead, it is designed to be easily embeddable
>> +# into other systems of Makefiles.
>> +#
>> +
>> +DTGET_SRCS = \
>> +     dtget.c \
>> +     util.c
>> +
>> +DTGET_GEN_SRCS =
>
> You have no generated sources, nor are you likely to in future, so you
> don't need this.

ok

>
>> +DTGET_OBJS = $(DTGET_SRCS:%.c=%.o) $(DTGET_GEN_SRCS:%.c=%.o)
>> diff --git a/dtget.c b/dtget.c
>> new file mode 100644
>> index 0000000..0ee577c
>> --- /dev/null
>> +++ b/dtget.c
>> @@ -0,0 +1,207 @@
>> +/*
>> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
>> + * Distributed under the terms of the GNU General Public License v2
>> + *
>> + * is_printable_string from ftdump.c
>> + * Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
>> + */
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <getopt.h>
>> +#include <ctype.h>
>> +
>> +#include <libfdt.h>
>> +
>> +#include "util.h"
>> +
>> +#define FTDUMP_BUF_SIZE      65536
>
> So, first of all the name of this constant is clearly wrong.  But more
> importantly, you shouldn't use a fixed size buffer like this.  ftdump
> makes no pretense to be anything more than a debugging hack, so it can
> get away with it.  dtget is supposed to be a real utility and should
> handle arbitrary sized fdt input.

Thanks for the promotion :-) Yes that's easy enough to do and I
suppose in theory an fdt could be larger than 64KB.

>
>> +static void report_error(const char *where, int err)
>> +{
>> +     fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
>> +}
>> +
>> +static void show_data(const char *format, int type, const char *data, int len)
>> +{
>> +     int i, size;
>> +     const char *p = data, *s;
>> +     int value;
>> +     int is_string;
>> +
>> +     /* no data, don't print */
>> +     if (len == 0)
>> +             return;
>> +
>> +     is_string = type == 's' ||
>                    ^^^^^^^^^^^
> parens here please to make it easier to visually parse out from the
> assignment.
>
>> +             (!type && util_is_printable_string(data, len));
>> +     if (is_string) {
>> +             for (s = data; s - data < len; s += strlen(s) + 1) {
>> +                     if (s != data)
>> +                             printf(" ");
>> +                     printf(format ? format : "%s", (const char *)s);
>> +             }
>> +             return;
>> +     }
>> +     if (type == 'i')
>> +             size = 4;
>> +     else if (type == 'b')
>> +             size = 1;
>> +     else
>> +             size = len % 4 == 0 ? 4 : 1;
>                       ^^^^^^^^^^^^
> parens for clarity here please.
>
>> +     for (i = 0; i < len; i += size, p += size) {
>> +             if (i)
>> +                     printf(" ");
>> +             value = size == 4 ? fdt32_to_cpu(*(const uint32_t *)p) :
>> +                     *(const unsigned char *)p;
>> +             printf(format ? format : "%d", value);
>> +     }
>> +}
>> +
>> +static int show_data_for_key(const void *blob, const char *format, int type,
>> +                          char *key)
>> +{
>> +     char *ptr, *prop;
>> +     int node, guess_node = 0;
>> +     const void *value;
>> +     int len;
>> +
>> +     /* First extract the property from the end */
>> +     ptr = strrchr(key, '/');
>> +     if (!ptr) {
>> +             fprintf(stderr, "No node found in key '%s'\n", key);
>> +             return 5;
>> +     }
>
> This is wrong.  properties do not have paths - they exist in a
> separate namespace to nodes.  The node path and property name should
> be supplied as separate parameters.  As well as being actually right,
> it will greatly simplify this function's convoluted logic.

Well yes, but I feel that it makes it harder to use also. My thinking
with this was to try to make it easy to extract information. In my
view:

/lcd/width

is better than

/lcd width

But this is the kind of discussion / feedback I was hoping to get as I
am new to device trees. My thoughts:

1. / is not generally used in property names so there is no conflict
2. Neither is there any ambiguity
3. The 'complex' logic is there to make life easy for the poor
hard-pressed user.
4. Creating the concept of a sort-of unified name space for a device
tree property is probably not a bad idea.

>
>
>> +     if (ptr == key) {
>> +             guess_node = fdt_path_offset(blob, key);
>> +             node = fdt_path_offset(blob, "/");
>> +     } else {
>> +             *ptr = '\0';
>> +             node = fdt_path_offset(blob, key);
>> +             *ptr = '/';
>> +     }
>> +     if (node < 0) {
>> +             report_error(key, node);
>> +             return 7;
>
> Open-coded arbitrary error codes?  Wtf?

Just following along with the code I found - it felt like less that 10
and more than 5 :-)

>
>> +     }
>> +     prop = ptr + 1;
>> +     value = fdt_getprop(blob, node, prop, &len);
>> +     if (!value) {
>> +             report_error(prop, len);
>> +             if (guess_node)
>> +                     fprintf(stderr, "(Node %s exists but you didn't "
>> +                             "specify a property to print)\n", key);
>
> Uh.. no.  If the fdt_path_offset() which assigned guess_node failed,
> guess_node would be negative and therefore true.  guess_node will only
> be zero if fdt_path_offset() returned zero, which should only happen
> if key == "/".  Remember zero is a valid node offset referring to the
> root node.

OK - it seemed to work fine in my testing but I will take a closer look.

>
>> +             return 8;
>> +     }
>> +     show_data(format, type, value, len);
>> +     printf("\n");
>> +     return 0;
>> +}
>> +
>> +static int do_dtget(const char *filename, const char *format, int type,
>> +                 char **key, int key_count)
>> +{
>> +     FILE *fp;
>> +     char *buf;
>> +     size_t size;
>> +     int i, ret;
>> +
>> +     if (strcmp(filename, "-") == 0) {
>> +             fp = stdin;
>> +     } else {
>> +             fp = fopen(filename, "rb");
>> +             if (fp == NULL) {
>> +                     fprintf(stderr, "unable to open %s\n", filename);
>> +                     return 10;
>> +             }
>> +     }
>> +
>> +     buf = malloc(FTDUMP_BUF_SIZE);
>> +     if (!buf) {
>> +             fprintf(stderr, "Couldn't allocate %d byte buffer\n",
>> +                     FTDUMP_BUF_SIZE);
>> +             return 10;
>> +     }
>> +
>> +     size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
>> +     if (!feof(fp)) {
>> +             fprintf(stderr, "file too large (maximum is %d bytes)\n",
>> +                     FTDUMP_BUF_SIZE);
>> +             return 10;
>> +     }
>> +     if (ferror(fp)) {
>> +             fprintf(stderr, "file read error\n");
>> +             return 10;
>> +     }
>> +     for (i = 0; i < key_count; i++) {
>> +             ret = show_data_for_key(buf, format, type, key[i]);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +     return 0;
>> +}
>> +
>> +static void usage(const char *msg)
>> +{
>> +     if (msg)
>> +             fprintf(stderr, "Error: %s\n\n", msg);
>> +     fprintf(stderr, "dtget - read values from device tree\n\n");
>> +     fprintf(stderr, "Usage: dtget <options> <dt file> <key>...\n");
>> +     fprintf(stderr, "Options:\n");
>> +     fprintf(stderr, "\t-f, --format <fmt>\tPrintf format string to use "
>> +             "for value\n");
>> +     fprintf(stderr, "\t-t, --type <typechar>\tForce type to be string "
>> +             "(s), int(i) or byte(b)\n");
>> +     fprintf(stderr, "\t-h, --help\t\tPrint this help\n\n");
>> +     fprintf(stderr, "\t<key> is <node>/<property>, for example "
>> +             "/lcd/width for the width\n");
>
> DO NOT use this node/property format.
>
> Also bare /lcd is a fairly improbably path for a node.

We have it.

>
>> +     fprintf(stderr, "\tproperty in the LCD node\n");
>> +     exit(2);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +     int c;
>> +     int ret;
>> +     char *format = NULL;
>> +     int type = 0;
>> +     char *filename = NULL;
>> +     static struct option long_options[] = {
>> +             {"format", 1, 0, 'f'},
>> +             {"help", 1, 0, 'h'},
>> +             {"type", 1, 0, 't'},
>> +             {0, 0, 0, 0}
>> +     };
>> +
>> +     for (;;) {
>> +             c = getopt_long(argc, argv, "f:ht:", long_options, NULL);
>> +             if (c == -1)
>> +                     break;
>
> Hrm.  using getopt_long() introduces a new dependency on GNU libc
> which I'm not thrilled about.

How should I implement long options? Or would you rather I just didn't?

>
>> +
>> +             switch (c) {
>> +             case 'h':
>> +             case '?':
>> +                     usage("");
>> +
>> +             case 'f':
>> +                     format = optarg;
>> +                     break;
>> +
>> +             case 't':
>> +                     type = *optarg;
>> +                     break;
>> +             }
>> +     }
>
> Hrm.  I really dislike your mechanism for choosing the output format.
> It's ad-hoc, doesn't really match any existing conventions and without
> forcing the format, potentially ambiguous.  Translating \0s into
> spaces in stringlist propertys is particularly nasty.  Decimal by
> default is also rarely the right choice.  Oh, and the fact that you
> could SEGV the utility by giving it a bad format string, since that's
> never validated, is really sloppy.

Yes this is the comment I was expecting. It works for our use case but
I agree it is probably too flexible/open for a general utility.

I don't follow you wrt the transaction of \0 into space.

Perhaps just a -f option with:

x - hex
s - string
i - int
u - unsigned int

?

I suppose people can then format the value into a string themselves.

>
> I'd need to know more about likely use cases to suggest a better way
> of handling the formatting though.
>

Well most of the time we just want a string, hex value or integer
(base 10) value. I think it is reasonable to simplify things.

Regards,
Simon

>> +     if (optind < argc)
>> +             filename = argv[optind++];
>> +     if (!filename)
>> +             usage("Missing filename");
>> +     if (optind == argc)
>> +             usage("Missing key(s)");
>> +
>> +     ret = do_dtget(filename, format, type, argv + optind, argc - optind);
>> +     return ret;
>> +}
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>

Re: [PATCH 2/9] Add dtget utility to read property values from device tree

[Simon Glass]

Hi David,

On Thu, Jul 28, 2011 at 5:22 AM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Hi David,
>
> On Tue, Jul 19, 2011 at 3:34 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> On Tue, Jul 05, 2011 at 12:02:50PM -0700, Simon Glass wrote:
>>> This simply utility makes it easy for scripts to read values from the device
>>> tree.
[snip]

>>> +static int show_data_for_key(const void *blob, const char *format, int type,
>>> +                          char *key)
>>> +{
>>> +     char *ptr, *prop;
>>> +     int node, guess_node = 0;
>>> +     const void *value;
>>> +     int len;
>>> +
>>> +     /* First extract the property from the end */
>>> +     ptr = strrchr(key, '/');
>>> +     if (!ptr) {
>>> +             fprintf(stderr, "No node found in key '%s'\n", key);
>>> +             return 5;
>>> +     }
>>
>> This is wrong.  properties do not have paths - they exist in a
>> separate namespace to nodes.  The node path and property name should
>> be supplied as separate parameters.  As well as being actually right,
>> it will greatly simplify this function's convoluted logic.
>
> Well yes, but I feel that it makes it harder to use also. My thinking
> with this was to try to make it easy to extract information. In my
> view:
>
> /lcd/width
>
> is better than
>
> /lcd width
>
> But this is the kind of discussion / feedback I was hoping to get as I
> am new to device trees. My thoughts:
>
> 1. / is not generally used in property names so there is no conflict
> 2. Neither is there any ambiguity
> 3. The 'complex' logic is there to make life easy for the poor
> hard-pressed user.
> 4. Creating the concept of a sort-of unified name space for a device
> tree property is probably not a bad idea.

I didn't hear back on this so I going to assume this is reasonable,
and submit another patch set with your other comments addressed. The
intent of the fdt seems clearly hierarchical to me. Hope this is ok.

Regards,
Simon

>
[more snip]

>> --
>> David Gibson                    | I'll have my music baroque, and my code
>> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>>                                | _way_ _around_!
>> http://www.ozlabs.org/~dgibson
>>
>

Re: [PATCH 2/9] Add dtget utility to read property values from device tree

[David Gibson]

On Fri, Sep 02, 2011 at 10:54:28AM -0700, Simon Glass wrote:
> Hi David,
> 
> On Thu, Jul 28, 2011 at 5:22 AM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> > Hi David,
> >
> > On Tue, Jul 19, 2011 at 3:34 PM, David Gibson
> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> On Tue, Jul 05, 2011 at 12:02:50PM -0700, Simon Glass wrote:
> >>> This simply utility makes it easy for scripts to read values from the device
> >>> tree.
> [snip]
> 
> >>> +static int show_data_for_key(const void *blob, const char *format, int type,
> >>> +                          char *key)
> >>> +{
> >>> +     char *ptr, *prop;
> >>> +     int node, guess_node = 0;
> >>> +     const void *value;
> >>> +     int len;
> >>> +
> >>> +     /* First extract the property from the end */
> >>> +     ptr = strrchr(key, '/');
> >>> +     if (!ptr) {
> >>> +             fprintf(stderr, "No node found in key '%s'\n", key);
> >>> +             return 5;
> >>> +     }
> >>
> >> This is wrong.  properties do not have paths - they exist in a
> >> separate namespace to nodes.  The node path and property name should
> >> be supplied as separate parameters.  As well as being actually right,
> >> it will greatly simplify this function's convoluted logic.
> >
> > Well yes, but I feel that it makes it harder to use also. My thinking
> > with this was to try to make it easy to extract information. In my
> > view:
> >
> > /lcd/width
> >
> > is better than
> >
> > /lcd width
> >
> > But this is the kind of discussion / feedback I was hoping to get as I
> > am new to device trees. My thoughts:
> >
> > 1. / is not generally used in property names so there is no conflict
> > 2. Neither is there any ambiguity

Yes there is.  Real device trees exist where a node has both a
property and a subnode with the same name.

> > 3. The 'complex' logic is there to make life easy for the poor
> > hard-pressed user.
> > 4. Creating the concept of a sort-of unified name space for a device
> > tree property is probably not a bad idea.
> 
> I didn't hear back on this so I going to assume this is reasonable,

No, I'm just busy.

> and submit another patch set with your other comments addressed. The
> intent of the fdt seems clearly hierarchical to me. Hope this is ok.

No, treating node+property as a single path remains cause for an
instant nack.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 2/9] Add dtget utility to read property values from device tree

[Simon Glass]

Hi David,

On Sat, Sep 3, 2011 at 7:33 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Sep 02, 2011 at 10:54:28AM -0700, Simon Glass wrote:
>> Hi David,
>>
>> On Thu, Jul 28, 2011 at 5:22 AM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> > Hi David,
>> >
>> > On Tue, Jul 19, 2011 at 3:34 PM, David Gibson
>> > <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> >> On Tue, Jul 05, 2011 at 12:02:50PM -0700, Simon Glass wrote:
>> >>> This simply utility makes it easy for scripts to read values from the device
>> >>> tree.
>> [snip]
>>
>> >>> +static int show_data_for_key(const void *blob, const char *format, int type,
>> >>> +                          char *key)
>> >>> +{
>> >>> +     char *ptr, *prop;
>> >>> +     int node, guess_node = 0;
>> >>> +     const void *value;
>> >>> +     int len;
>> >>> +
>> >>> +     /* First extract the property from the end */
>> >>> +     ptr = strrchr(key, '/');
>> >>> +     if (!ptr) {
>> >>> +             fprintf(stderr, "No node found in key '%s'\n", key);
>> >>> +             return 5;
>> >>> +     }
>> >>
>> >> This is wrong.  properties do not have paths - they exist in a
>> >> separate namespace to nodes.  The node path and property name should
>> >> be supplied as separate parameters.  As well as being actually right,
>> >> it will greatly simplify this function's convoluted logic.
>> >
>> > Well yes, but I feel that it makes it harder to use also. My thinking
>> > with this was to try to make it easy to extract information. In my
>> > view:
>> >
>> > /lcd/width
>> >
>> > is better than
>> >
>> > /lcd width
>> >
>> > But this is the kind of discussion / feedback I was hoping to get as I
>> > am new to device trees. My thoughts:
>> >
>> > 1. / is not generally used in property names so there is no conflict
>> > 2. Neither is there any ambiguity
>
> Yes there is.  Real device trees exist where a node has both a
> property and a subnode with the same name.

Er, ok. How about something like:

/path/to/node/.property

as a compromise? It removes the ambiguity I think, and still lets me
avoid having 3 parameters in fdtput per assignment.

Otherwise if you have any other suggestions I would appreciate it.

Regards,
Simon

>
>> > 3. The 'complex' logic is there to make life easy for the poor
>> > hard-pressed user.
>> > 4. Creating the concept of a sort-of unified name space for a device
>> > tree property is probably not a bad idea.
>>
>> I didn't hear back on this so I going to assume this is reasonable,
>
> No, I'm just busy.
>
>> and submit another patch set with your other comments addressed. The
>> intent of the fdt seems clearly hierarchical to me. Hope this is ok.
>
> No, treating node+property as a single path remains cause for an
> instant nack.
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>

Re: [PATCH 2/9] Add dtget utility to read property values from device tree

[David Gibson]

On Sat, Sep 03, 2011 at 08:18:08PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Sat, Sep 3, 2011 at 7:33 AM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Sep 02, 2011 at 10:54:28AM -0700, Simon Glass wrote:
[snip]
> >> >> This is wrong.  properties do not have paths - they exist in a
> >> >> separate namespace to nodes.  The node path and property name should
> >> >> be supplied as separate parameters.  As well as being actually right,
> >> >> it will greatly simplify this function's convoluted logic.
> >> >
> >> > Well yes, but I feel that it makes it harder to use also. My thinking
> >> > with this was to try to make it easy to extract information. In my
> >> > view:
> >> >
> >> > /lcd/width
> >> >
> >> > is better than
> >> >
> >> > /lcd width
> >> >
> >> > But this is the kind of discussion / feedback I was hoping to get as I
> >> > am new to device trees. My thoughts:
> >> >
> >> > 1. / is not generally used in property names so there is no conflict
> >> > 2. Neither is there any ambiguity
> >
> > Yes there is.  Real device trees exist where a node has both a
> > property and a subnode with the same name.
> 
> Er, ok. How about something like:
> 
> /path/to/node/.property
> 
> as a compromise? It removes the ambiguity I think, and still lets me

That's not a compromise, that's worse!

> avoid having 3 parameters in fdtput per assignment.

For pete's sake, just add the extra parameter.  It's the right way to
do it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 2/9] Add dtget utility to read property values from device tree

[Simon Glass]

Hi David,

On Sun, Sep 4, 2011 at 4:14 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sat, Sep 03, 2011 at 08:18:08PM -0700, Simon Glass wrote:
>> Hi David,
>>
>> On Sat, Sep 3, 2011 at 7:33 AM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Fri, Sep 02, 2011 at 10:54:28AM -0700, Simon Glass wrote:
> [snip]
>> >> >> This is wrong.  properties do not have paths - they exist in a
>> >> >> separate namespace to nodes.  The node path and property name should
>> >> >> be supplied as separate parameters.  As well as being actually right,
>> >> >> it will greatly simplify this function's convoluted logic.
>> >> >
>> >> > Well yes, but I feel that it makes it harder to use also. My thinking
>> >> > with this was to try to make it easy to extract information. In my
>> >> > view:
>> >> >
>> >> > /lcd/width
>> >> >
>> >> > is better than
>> >> >
>> >> > /lcd width
>> >> >
>> >> > But this is the kind of discussion / feedback I was hoping to get as I
>> >> > am new to device trees. My thoughts:
>> >> >
>> >> > 1. / is not generally used in property names so there is no conflict
>> >> > 2. Neither is there any ambiguity
>> >
>> > Yes there is.  Real device trees exist where a node has both a
>> > property and a subnode with the same name.
>>
>> Er, ok. How about something like:
>>
>> /path/to/node/.property
>>
>> as a compromise? It removes the ambiguity I think, and still lets me
>
> That's not a compromise, that's worse!

Oh dear.

>
>> avoid having 3 parameters in fdtput per assignment.
>
> For pete's sake, just add the extra parameter.  It's the right way to
> do it.

Right oh. Thanks for the quick response.

Regards,
Simon

>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>

[PATCH 3/9] Add basic tests for dtget

[Simon Glass]

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 tests/dtget-runtest.sh |   30 ++++++++++++++++++++++
 tests/dtget-test.dts   |   63 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/run_tests.sh     |   33 ++++++++++++++++++++++++-
 tests/tests.sh         |    1 +
 4 files changed, 126 insertions(+), 1 deletions(-)
 create mode 100755 tests/dtget-runtest.sh
 create mode 100644 tests/dtget-test.dts

diff --git a/tests/dtget-runtest.sh b/tests/dtget-runtest.sh
new file mode 100755
index 0000000..aa2b744
--- /dev/null
+++ b/tests/dtget-runtest.sh
@@ -0,0 +1,30 @@
+#! /bin/sh
+
+. ./tests.sh
+
+LOG="tmp.log.$$"
+EXPECT="tmp.expect.$$"
+
+rm -f $TMPFILE $LOG
+
+echo "$1" >$EXPECT
+shift
+
+verbose_run_log "$LOG" $VALGRIND "$DTGET" "$@"
+ret="$?"
+
+if [ "$ret" -gt 127 ]; then
+    signame=$(kill -l $[ret - 128])
+    FAIL "Killed by SIG$signame"
+fi
+
+diff $EXPECT $LOG
+ret="$?"
+
+rm -f $LOG $EXPECT
+
+if [ "$ret" -eq 0 ]; then
+	PASS
+else
+	FAIL
+fi
diff --git a/tests/dtget-test.dts b/tests/dtget-test.dts
new file mode 100644
index 0000000..79f9633
--- /dev/null
+++ b/tests/dtget-test.dts
@@ -0,0 +1,63 @@
+/dts-v1/;
+
+/memreserve/ 0x1000000000000000 0x0000000002000000;
+memrsv2: /memreserve/ 0x2000000000000000 0x0100000000000000;
+/memreserve/ 0x0000000000000000 0x0000000000000014;
+
+/ {
+	model = "MyBoardName";
+	compatible = "MyBoardName", "MyBoardFamilyName";
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	cpus {
+		linux,phandle = <0x1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		PowerPC,970@0 {
+			name = "PowerPC,970";
+			device_type = "cpu";
+			reg = <0x00000000>;
+			clock-frequency = <1600000000>;
+			timebase-frequency = <33333333>;
+			linux,boot-cpu;
+			i-cache-size = <65536>;
+			d-cache-size = <32768>;
+		};
+
+		PowerPC,970@1 {
+			name = "PowerPC,970";
+			device_type = "cpu";
+			reg = <0x00000001>;
+			clock-frequency = <1600000000>;
+			timebase-frequency = <33333333>;
+			i-cache-size = <65536>;
+			d-cache-size = <32768>;
+		};
+
+	};
+
+	node: randomnode {
+		prop: string = str: "foo", "stuffstuff\t\t\t\n\n\n" str_end: ;
+		blob = [byte: 0a 0b 0c 0d byte_mid: de ea ad be ef byte_end: ];
+		ref = < cell: &{/memory@0} 0x0 cell_mid: 0xffffffff cell_end: >;
+		mixed = "abc", pre: [1234] post: , gap: < aligned: 0xa 0xb 0xc>;
+		tricky1 = [61 lt1: 62 63 00];
+		subnode: child {
+		};
+		/* subnode_end: is auto-generated by node emit */
+	};
+	/* node_end: is auto-generated by node emit */
+
+	memory@0 {
+		device_type = "memory";
+		memreg: reg = <0x00000000 0x00000000 0x00000000 0x20000000>;
+	};
+
+	chosen {
+		bootargs = "root=/dev/sda2";
+		linux,platform = <0x600>;
+	};
+
+};
+
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 72dda32..01a2b60 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -83,6 +83,12 @@ asm_to_so_test () {
     run_wrap_test asm_to_so "$@"
 }
 
+run_dtget_test () {
+    echo -n "$1:	"
+    shift
+    base_run_test sh dtget-runtest.sh "$@"
+}
+
 tree1_tests () {
     TREE=$1
 
@@ -388,6 +394,28 @@ dtbs_equal_tests () {
     cmp_tests test_tree1.dtb $WRONG_TREE1
 }
 
+dtget_tests () {
+    file=dtget-test.dtb
+    $DTC -O dtb -o $file ${file%.dtb}.dts 2>/dev/null
+
+    # run_dtget_test <test-name> <expected-result> <args>...
+    run_dtget_test "Simple string" "MyBoardName" $file "/model"
+    run_dtget_test "Multiple string i" "77 121 66 111 \
+97 114 100 78 97 109 101 0 77 121 66 111 97 114 100 70 97 109 105 \
+108 121 78 97 109 101 0" $file "/compatible"
+    run_dtget_test "Multiple string s" "MyBoardName MyBoardFamilyName" \
+	-t s $file "/compatible"
+    run_dtget_test "Integer" "32768" $file "/cpus/PowerPC,970@1/d-cache-size"
+    run_dtget_test "Integer hex" "8000" -f %x $file \
+	"/cpus/PowerPC,970@1/d-cache-size"
+    run_dtget_test "Integer list" "61 62 63 00" -f %02x -t b $file \
+	"/randomnode/tricky1"
+    run_dtget_test "Byte list short" "a b c d de ea ad be ef" -f %x -t b \
+	$file "/randomnode/blob"
+    run_dtget_test "Integer list short" "a0b0c0d deeaadbe ef000000" -f %x \
+	-t i $file "/randomnode/blob"
+}
+
 while getopts "vt:m" ARG ; do
     case $ARG in
 	"v")
@@ -403,7 +431,7 @@ while getopts "vt:m" ARG ; do
 done
 
 if [ -z "$TESTSETS" ]; then
-    TESTSETS="libfdt dtc dtbs_equal"
+    TESTSETS="libfdt dtc dtbs_equal dtget"
 fi
 
 # Make sure we don't have stale blobs lying around
@@ -420,6 +448,9 @@ for set in $TESTSETS; do
 	"dtbs_equal")
 	    dtbs_equal_tests
 	    ;;
+	"dtget")
+	    dtget_tests
+	    ;;
     esac
 done
 
diff --git a/tests/tests.sh b/tests/tests.sh
index 30ffead..cf7f19e 100644
--- a/tests/tests.sh
+++ b/tests/tests.sh
@@ -11,6 +11,7 @@ FAIL () {
 }
 
 DTC=../dtc
+DTGET=../dtget
 
 verbose_run () {
     if [ -z "$QUIET_TEST" ]; then
-- 
1.7.3.1

Re: [PATCH 3/9] Add basic tests for dtget

[Grant Likely]

On Tue, Jul 05, 2011 at 12:02:51PM -0700, Simon Glass wrote:
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Yay, testcases.  :-)

g.

> ---
>  tests/dtget-runtest.sh |   30 ++++++++++++++++++++++
>  tests/dtget-test.dts   |   63 ++++++++++++++++++++++++++++++++++++++++++++++++

Rather than creating a new dts file, these test should probably
leverage the existing ones in the tests directory.

>  tests/run_tests.sh     |   33 ++++++++++++++++++++++++-
>  tests/tests.sh         |    1 +
>  4 files changed, 126 insertions(+), 1 deletions(-)
>  create mode 100755 tests/dtget-runtest.sh
>  create mode 100644 tests/dtget-test.dts
> 
> diff --git a/tests/dtget-runtest.sh b/tests/dtget-runtest.sh
> new file mode 100755
> index 0000000..aa2b744
> --- /dev/null
> +++ b/tests/dtget-runtest.sh
> @@ -0,0 +1,30 @@
> +#! /bin/sh
> +
> +. ./tests.sh
> +
> +LOG="tmp.log.$$"
> +EXPECT="tmp.expect.$$"
> +
> +rm -f $TMPFILE $LOG
> +
> +echo "$1" >$EXPECT
> +shift
> +
> +verbose_run_log "$LOG" $VALGRIND "$DTGET" "$@"
> +ret="$?"
> +
> +if [ "$ret" -gt 127 ]; then
> +    signame=$(kill -l $[ret - 128])
> +    FAIL "Killed by SIG$signame"
> +fi
> +
> +diff $EXPECT $LOG
> +ret="$?"
> +
> +rm -f $LOG $EXPECT
> +
> +if [ "$ret" -eq 0 ]; then
> +	PASS
> +else
> +	FAIL
> +fi
> diff --git a/tests/dtget-test.dts b/tests/dtget-test.dts
> new file mode 100644
> index 0000000..79f9633
> --- /dev/null
> +++ b/tests/dtget-test.dts
> @@ -0,0 +1,63 @@
> +/dts-v1/;
> +
> +/memreserve/ 0x1000000000000000 0x0000000002000000;
> +memrsv2: /memreserve/ 0x2000000000000000 0x0100000000000000;
> +/memreserve/ 0x0000000000000000 0x0000000000000014;
> +
> +/ {
> +	model = "MyBoardName";
> +	compatible = "MyBoardName", "MyBoardFamilyName";
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	cpus {
> +		linux,phandle = <0x1>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		PowerPC,970@0 {
> +			name = "PowerPC,970";
> +			device_type = "cpu";
> +			reg = <0x00000000>;
> +			clock-frequency = <1600000000>;
> +			timebase-frequency = <33333333>;
> +			linux,boot-cpu;
> +			i-cache-size = <65536>;
> +			d-cache-size = <32768>;
> +		};
> +
> +		PowerPC,970@1 {
> +			name = "PowerPC,970";
> +			device_type = "cpu";
> +			reg = <0x00000001>;
> +			clock-frequency = <1600000000>;
> +			timebase-frequency = <33333333>;
> +			i-cache-size = <65536>;
> +			d-cache-size = <32768>;
> +		};
> +
> +	};
> +
> +	node: randomnode {
> +		prop: string = str: "foo", "stuffstuff\t\t\t\n\n\n" str_end: ;
> +		blob = [byte: 0a 0b 0c 0d byte_mid: de ea ad be ef byte_end: ];
> +		ref = < cell: &{/memory@0} 0x0 cell_mid: 0xffffffff cell_end: >;
> +		mixed = "abc", pre: [1234] post: , gap: < aligned: 0xa 0xb 0xc>;
> +		tricky1 = [61 lt1: 62 63 00];
> +		subnode: child {
> +		};
> +		/* subnode_end: is auto-generated by node emit */
> +	};
> +	/* node_end: is auto-generated by node emit */
> +
> +	memory@0 {
> +		device_type = "memory";
> +		memreg: reg = <0x00000000 0x00000000 0x00000000 0x20000000>;
> +	};
> +
> +	chosen {
> +		bootargs = "root=/dev/sda2";
> +		linux,platform = <0x600>;
> +	};
> +
> +};
> +
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 72dda32..01a2b60 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -83,6 +83,12 @@ asm_to_so_test () {
>      run_wrap_test asm_to_so "$@"
>  }
>  
> +run_dtget_test () {
> +    echo -n "$1:	"
> +    shift
> +    base_run_test sh dtget-runtest.sh "$@"
> +}
> +
>  tree1_tests () {
>      TREE=$1
>  
> @@ -388,6 +394,28 @@ dtbs_equal_tests () {
>      cmp_tests test_tree1.dtb $WRONG_TREE1
>  }
>  
> +dtget_tests () {
> +    file=dtget-test.dtb
> +    $DTC -O dtb -o $file ${file%.dtb}.dts 2>/dev/null
> +
> +    # run_dtget_test <test-name> <expected-result> <args>...
> +    run_dtget_test "Simple string" "MyBoardName" $file "/model"
> +    run_dtget_test "Multiple string i" "77 121 66 111 \
> +97 114 100 78 97 109 101 0 77 121 66 111 97 114 100 70 97 109 105 \
> +108 121 78 97 109 101 0" $file "/compatible"
> +    run_dtget_test "Multiple string s" "MyBoardName MyBoardFamilyName" \
> +	-t s $file "/compatible"
> +    run_dtget_test "Integer" "32768" $file "/cpus/PowerPC,970@1/d-cache-size"
> +    run_dtget_test "Integer hex" "8000" -f %x $file \
> +	"/cpus/PowerPC,970@1/d-cache-size"
> +    run_dtget_test "Integer list" "61 62 63 00" -f %02x -t b $file \
> +	"/randomnode/tricky1"
> +    run_dtget_test "Byte list short" "a b c d de ea ad be ef" -f %x -t b \
> +	$file "/randomnode/blob"
> +    run_dtget_test "Integer list short" "a0b0c0d deeaadbe ef000000" -f %x \
> +	-t i $file "/randomnode/blob"
> +}
> +
>  while getopts "vt:m" ARG ; do
>      case $ARG in
>  	"v")
> @@ -403,7 +431,7 @@ while getopts "vt:m" ARG ; do
>  done
>  
>  if [ -z "$TESTSETS" ]; then
> -    TESTSETS="libfdt dtc dtbs_equal"
> +    TESTSETS="libfdt dtc dtbs_equal dtget"
>  fi
>  
>  # Make sure we don't have stale blobs lying around
> @@ -420,6 +448,9 @@ for set in $TESTSETS; do
>  	"dtbs_equal")
>  	    dtbs_equal_tests
>  	    ;;
> +	"dtget")
> +	    dtget_tests
> +	    ;;
>      esac
>  done
>  
> diff --git a/tests/tests.sh b/tests/tests.sh
> index 30ffead..cf7f19e 100644
> --- a/tests/tests.sh
> +++ b/tests/tests.sh
> @@ -11,6 +11,7 @@ FAIL () {
>  }
>  
>  DTC=../dtc
> +DTGET=../dtget
>  
>  verbose_run () {
>      if [ -z "$QUIET_TEST" ]; then
> -- 
> 1.7.3.1
> 

Re: [PATCH 3/9] Add basic tests for dtget

[David Gibson]

On Tue, Jul 05, 2011 at 12:02:51PM -0700, Simon Glass wrote:

Tests should not be an add-on.  Fold this into the patch which added
dtget itself.

> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  tests/dtget-runtest.sh |   30 ++++++++++++++++++++++
>  tests/dtget-test.dts   |   63 ++++++++++++++++++++++++++++++++++++++++++++++++

As Grant says, I think this should use existing dts examples in
preference to creating new ones.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 4/9] Add missing tests to .gitignore

[Simon Glass]

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 tests/.gitignore |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tests/.gitignore b/tests/.gitignore
index c4e1205..f4e58b2 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -45,3 +45,5 @@
 /sw_tree1
 /truncated_property
 /value-labels
+/dtb_reverse
+/dtbs_equal_unordered
-- 
1.7.3.1

Re: [PATCH 4/9] Add missing tests to .gitignore

[Grant Likely]

On Tue, Jul 05, 2011 at 12:02:52PM -0700, Simon Glass wrote:
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

> ---
>  tests/.gitignore |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index c4e1205..f4e58b2 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -45,3 +45,5 @@
>  /sw_tree1
>  /truncated_property
>  /value-labels
> +/dtb_reverse
> +/dtbs_equal_unordered
> -- 
> 1.7.3.1
> 

Re: [PATCH 4/9] Add missing tests to .gitignore

[David Gibson]

On Tue, Jul 05, 2011 at 12:02:52PM -0700, Simon Glass wrote:

Trivial though it is, this needs a commit comment.

> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Jon, this can be applied now, regardless of the rest of the series.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 4/9] Add missing tests to .gitignore

[Jon Loeliger]

> On Tue, Jul 05, 2011 at 12:02:52PM -0700, Simon Glass wrote:
> 
> Trivial though it is, this needs a commit comment.
> 
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> 
> Jon, this can be applied now, regardless of the rest of the series.

Applied.

jdl

[PATCH 5/9] Add utilfdt for common functions, adjust dtget

[Simon Glass]

This adds a new utility library for performing libfdt operations, and
modifies dtget to use it.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 Makefile.dtget |    3 +-
 dtget.c        |   55 +++------------------------
 utilfdt.c      |  113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 utilfdt.h      |   58 ++++++++++++++++++++++++++++
 4 files changed, 180 insertions(+), 49 deletions(-)
 create mode 100644 utilfdt.c
 create mode 100644 utilfdt.h

diff --git a/Makefile.dtget b/Makefile.dtget
index edda499..fd01055 100644
--- a/Makefile.dtget
+++ b/Makefile.dtget
@@ -6,7 +6,8 @@
 
 DTGET_SRCS = \
 	dtget.c \
-	util.c
+	util.c \
+	utilfdt.c
 
 DTGET_GEN_SRCS =
 
diff --git a/dtget.c b/dtget.c
index 0ee577c..cbcd4d2 100644
--- a/dtget.c
+++ b/dtget.c
@@ -15,8 +15,7 @@
 #include <libfdt.h>
 
 #include "util.h"
-
-#define FTDUMP_BUF_SIZE	65536
+#include "utilfdt.h"
 
 static void report_error(const char *where, int err)
 {
@@ -62,31 +61,16 @@ static void show_data(const char *format, int type, const char *data, int len)
 static int show_data_for_key(const void *blob, const char *format, int type,
 			     char *key)
 {
-	char *ptr, *prop;
+	char *prop;
 	int node, guess_node = 0;
 	const void *value;
 	int len;
 
-	/* First extract the property from the end */
-	ptr = strrchr(key, '/');
-	if (!ptr) {
-		fprintf(stderr, "No node found in key '%s'\n", key);
-		return 5;
-	}
-
-	if (ptr == key) {
-		guess_node = fdt_path_offset(blob, key);
-		node = fdt_path_offset(blob, "/");
-	} else {
-		*ptr = '\0';
-		node = fdt_path_offset(blob, key);
-		*ptr = '/';
-	}
+	node = util_decode_key(blob, key, &prop, &guess_node);
 	if (node < 0) {
 		report_error(key, node);
 		return 7;
 	}
-	prop = ptr + 1;
 	value = fdt_getprop(blob, node, prop, &len);
 	if (!value) {
 		report_error(prop, len);
@@ -103,40 +87,15 @@ static int show_data_for_key(const void *blob, const char *format, int type,
 static int do_dtget(const char *filename, const char *format, int type,
 		    char **key, int key_count)
 {
-	FILE *fp;
-	char *buf;
-	size_t size;
+	char *blob;
 	int i, ret;
 
-	if (strcmp(filename, "-") == 0) {
-		fp = stdin;
-	} else {
-		fp = fopen(filename, "rb");
-		if (fp == NULL) {
-			fprintf(stderr, "unable to open %s\n", filename);
-			return 10;
-		}
-	}
-
-	buf = malloc(FTDUMP_BUF_SIZE);
-	if (!buf) {
-		fprintf(stderr, "Couldn't allocate %d byte buffer\n",
-			FTDUMP_BUF_SIZE);
+	blob = util_read_fdt(filename);
+	if (!blob)
 		return 10;
-	}
 
-	size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
-	if (!feof(fp)) {
-		fprintf(stderr, "file too large (maximum is %d bytes)\n",
-			FTDUMP_BUF_SIZE);
-		return 10;
-	}
-	if (ferror(fp)) {
-		fprintf(stderr, "file read error\n");
-		return 10;
-	}
 	for (i = 0; i < key_count; i++) {
-		ret = show_data_for_key(buf, format, type, key[i]);
+		ret = show_data_for_key(blob, format, type, key[i]);
 		if (ret)
 			return ret;
 	}
diff --git a/utilfdt.c b/utilfdt.c
new file mode 100644
index 0000000..3cbd566
--- /dev/null
+++ b/utilfdt.c
@@ -0,0 +1,113 @@
+/*
+ * Copyright 2001 The Chromium Authors, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
+ *                                                                   USA
+ */
+
+#include <ctype.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <string.h>
+
+#include "libfdt.h"
+#include "utilfdt.h"
+
+#define FTDUMP_BUF_SIZE	65536
+
+char *util_read_fdt(const char *filename)
+{
+	FILE *fp;
+	char *buf;
+	int size;
+
+	if (strcmp(filename, "-") == 0) {
+		fp = stdin;
+	} else {
+		fp = fopen(filename, "rb");
+		if (fp == NULL) {
+			fprintf(stderr, "unable to open '%s'\n", filename);
+			return NULL;
+		}
+	}
+
+	buf = malloc(FTDUMP_BUF_SIZE);
+	if (!buf) {
+		fprintf(stderr, "Couldn't allocate %d byte buffer\n",
+			FTDUMP_BUF_SIZE);
+		return NULL;
+	}
+
+	size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
+	if (!feof(fp)) {
+		fprintf(stderr, "file too large (maximum is %d bytes)\n",
+			FTDUMP_BUF_SIZE);
+	} else if (ferror(fp))
+		fprintf(stderr, "file read error\n");
+	else
+		return buf;
+	free(buf);
+	return NULL;
+}
+
+int util_write_fdt(const char *buf, const char *filename)
+{
+	FILE *fp;
+	size_t size, written;
+
+	if (strcmp(filename, "-") == 0) {
+		fp = stdout;
+	} else {
+		fp = fopen(filename, "wb");
+		if (fp == NULL) {
+			fprintf(stderr, "unable to open %s\n", filename);
+			return -1;
+		}
+	}
+
+	size = fdt_totalsize(buf);
+	written = fwrite(buf, 1, size, fp);
+	if (size != written) {
+		fprintf(stderr, "file write failed\n");
+		return -1;
+	}
+	return 0;
+}
+
+int util_decode_key(const char *blob, char *key, char **prop, int *guess_node)
+{
+	int node;
+	char *ptr;
+
+	/* First extract the property from the end */
+	*guess_node = 0;
+	ptr = strrchr(key, '/');
+	if (!ptr) {
+		fprintf(stderr, "No node found in key '%s'\n", key);
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	if (ptr == key) {
+		*guess_node = fdt_path_offset(blob, key);
+		node = fdt_path_offset(blob, "/");
+	} else {
+		*ptr = '\0';
+		node = fdt_path_offset(blob, key);
+		*ptr = '/';
+	}
+	*prop = ptr + 1;
+	return node;
+}
diff --git a/utilfdt.h b/utilfdt.h
new file mode 100644
index 0000000..79c218f
--- /dev/null
+++ b/utilfdt.h
@@ -0,0 +1,58 @@
+#ifndef _UTILFDT_H
+#define _UTILFDT_H
+
+/*
+ * Copyright 2001 The Chromium Authors, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
+ *                                                                   USA
+ */
+
+/**
+ * Read a device tree file into a buffer.
+ *
+ * @param filename	The filename to read, or - for stdin
+ * @return		Poiner to allocated buffer containing fdt, or NULL
+ *			on error
+ */
+char *util_read_fdt(const char *filename);
+
+/**
+ * Write a device tree buffer to a file.
+ *
+ * @param filename	The filename to write, or - for stdout
+ * @param buf		Poiner to buffer containing fdt
+ * @return 0 if ok, -1 on error
+ */
+int util_write_fdt(const char *buf, const char *filename);
+
+/**
+ * Decode a node and property from a key string.
+ *
+ * This converts a key of the form /lcd/width into a node offset (/lcd) and
+ * a property (width).
+ *
+ * @param blob		Device tree blob
+ * @param key		Key string to decode
+ * @param prop		Returns pointer to property string within key
+ * @param guess_node	Normally <= 0, but if not then this is the offset of
+ *			a node that the caller MAY be referring to. For example
+ *			if key="/width" and there is a node /width, then
+ *			guess_node will return the offset of /width. It is used
+ *			to print a helpful error message.
+ */
+int util_decode_key(const char *blob, char *key, char **prop, int *guess_node);
+
+#endif /* _UTILFDT_H */
-- 
1.7.3.1

Re: [PATCH 5/9] Add utilfdt for common functions, adjust dtget

[Grant Likely]

On Tue, Jul 05, 2011 at 12:02:53PM -0700, Simon Glass wrote:
> This adds a new utility library for performing libfdt operations, and
> modifies dtget to use it.

Perhaps this patch should come before the dtget patch?

Otherwise looks okay.

> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  Makefile.dtget |    3 +-
>  dtget.c        |   55 +++------------------------
>  utilfdt.c      |  113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  utilfdt.h      |   58 ++++++++++++++++++++++++++++
>  4 files changed, 180 insertions(+), 49 deletions(-)
>  create mode 100644 utilfdt.c
>  create mode 100644 utilfdt.h
> 
> diff --git a/Makefile.dtget b/Makefile.dtget
> index edda499..fd01055 100644
> --- a/Makefile.dtget
> +++ b/Makefile.dtget
> @@ -6,7 +6,8 @@
>  
>  DTGET_SRCS = \
>  	dtget.c \
> -	util.c
> +	util.c \
> +	utilfdt.c
>  
>  DTGET_GEN_SRCS =
>  
> diff --git a/dtget.c b/dtget.c
> index 0ee577c..cbcd4d2 100644
> --- a/dtget.c
> +++ b/dtget.c
> @@ -15,8 +15,7 @@
>  #include <libfdt.h>
>  
>  #include "util.h"
> -
> -#define FTDUMP_BUF_SIZE	65536
> +#include "utilfdt.h"
>  
>  static void report_error(const char *where, int err)
>  {
> @@ -62,31 +61,16 @@ static void show_data(const char *format, int type, const char *data, int len)
>  static int show_data_for_key(const void *blob, const char *format, int type,
>  			     char *key)
>  {
> -	char *ptr, *prop;
> +	char *prop;
>  	int node, guess_node = 0;
>  	const void *value;
>  	int len;
>  
> -	/* First extract the property from the end */
> -	ptr = strrchr(key, '/');
> -	if (!ptr) {
> -		fprintf(stderr, "No node found in key '%s'\n", key);
> -		return 5;
> -	}
> -
> -	if (ptr == key) {
> -		guess_node = fdt_path_offset(blob, key);
> -		node = fdt_path_offset(blob, "/");
> -	} else {
> -		*ptr = '\0';
> -		node = fdt_path_offset(blob, key);
> -		*ptr = '/';
> -	}
> +	node = util_decode_key(blob, key, &prop, &guess_node);
>  	if (node < 0) {
>  		report_error(key, node);
>  		return 7;
>  	}
> -	prop = ptr + 1;
>  	value = fdt_getprop(blob, node, prop, &len);
>  	if (!value) {
>  		report_error(prop, len);
> @@ -103,40 +87,15 @@ static int show_data_for_key(const void *blob, const char *format, int type,
>  static int do_dtget(const char *filename, const char *format, int type,
>  		    char **key, int key_count)
>  {
> -	FILE *fp;
> -	char *buf;
> -	size_t size;
> +	char *blob;
>  	int i, ret;
>  
> -	if (strcmp(filename, "-") == 0) {
> -		fp = stdin;
> -	} else {
> -		fp = fopen(filename, "rb");
> -		if (fp == NULL) {
> -			fprintf(stderr, "unable to open %s\n", filename);
> -			return 10;
> -		}
> -	}
> -
> -	buf = malloc(FTDUMP_BUF_SIZE);
> -	if (!buf) {
> -		fprintf(stderr, "Couldn't allocate %d byte buffer\n",
> -			FTDUMP_BUF_SIZE);
> +	blob = util_read_fdt(filename);
> +	if (!blob)
>  		return 10;
> -	}
>  
> -	size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
> -	if (!feof(fp)) {
> -		fprintf(stderr, "file too large (maximum is %d bytes)\n",
> -			FTDUMP_BUF_SIZE);
> -		return 10;
> -	}
> -	if (ferror(fp)) {
> -		fprintf(stderr, "file read error\n");
> -		return 10;
> -	}
>  	for (i = 0; i < key_count; i++) {
> -		ret = show_data_for_key(buf, format, type, key[i]);
> +		ret = show_data_for_key(blob, format, type, key[i]);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/utilfdt.c b/utilfdt.c
> new file mode 100644
> index 0000000..3cbd566
> --- /dev/null
> +++ b/utilfdt.c
> @@ -0,0 +1,113 @@
> +/*
> + * Copyright 2001 The Chromium Authors, All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> + *                                                                   USA
> + */
> +
> +#include <ctype.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <string.h>
> +
> +#include "libfdt.h"
> +#include "utilfdt.h"
> +
> +#define FTDUMP_BUF_SIZE	65536
> +
> +char *util_read_fdt(const char *filename)
> +{
> +	FILE *fp;
> +	char *buf;
> +	int size;
> +
> +	if (strcmp(filename, "-") == 0) {
> +		fp = stdin;
> +	} else {
> +		fp = fopen(filename, "rb");
> +		if (fp == NULL) {
> +			fprintf(stderr, "unable to open '%s'\n", filename);
> +			return NULL;
> +		}
> +	}
> +
> +	buf = malloc(FTDUMP_BUF_SIZE);
> +	if (!buf) {
> +		fprintf(stderr, "Couldn't allocate %d byte buffer\n",
> +			FTDUMP_BUF_SIZE);
> +		return NULL;
> +	}
> +
> +	size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
> +	if (!feof(fp)) {
> +		fprintf(stderr, "file too large (maximum is %d bytes)\n",
> +			FTDUMP_BUF_SIZE);
> +	} else if (ferror(fp))
> +		fprintf(stderr, "file read error\n");
> +	else
> +		return buf;
> +	free(buf);
> +	return NULL;
> +}
> +
> +int util_write_fdt(const char *buf, const char *filename)
> +{
> +	FILE *fp;
> +	size_t size, written;
> +
> +	if (strcmp(filename, "-") == 0) {
> +		fp = stdout;
> +	} else {
> +		fp = fopen(filename, "wb");
> +		if (fp == NULL) {
> +			fprintf(stderr, "unable to open %s\n", filename);
> +			return -1;
> +		}
> +	}
> +
> +	size = fdt_totalsize(buf);
> +	written = fwrite(buf, 1, size, fp);
> +	if (size != written) {
> +		fprintf(stderr, "file write failed\n");
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +int util_decode_key(const char *blob, char *key, char **prop, int *guess_node)
> +{
> +	int node;
> +	char *ptr;
> +
> +	/* First extract the property from the end */
> +	*guess_node = 0;
> +	ptr = strrchr(key, '/');
> +	if (!ptr) {
> +		fprintf(stderr, "No node found in key '%s'\n", key);
> +		return -FDT_ERR_NOTFOUND;
> +	}
> +
> +	if (ptr == key) {
> +		*guess_node = fdt_path_offset(blob, key);
> +		node = fdt_path_offset(blob, "/");
> +	} else {
> +		*ptr = '\0';
> +		node = fdt_path_offset(blob, key);
> +		*ptr = '/';
> +	}
> +	*prop = ptr + 1;
> +	return node;
> +}
> diff --git a/utilfdt.h b/utilfdt.h
> new file mode 100644
> index 0000000..79c218f
> --- /dev/null
> +++ b/utilfdt.h
> @@ -0,0 +1,58 @@
> +#ifndef _UTILFDT_H
> +#define _UTILFDT_H
> +
> +/*
> + * Copyright 2001 The Chromium Authors, All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> + *                                                                   USA
> + */
> +
> +/**
> + * Read a device tree file into a buffer.
> + *
> + * @param filename	The filename to read, or - for stdin
> + * @return		Poiner to allocated buffer containing fdt, or NULL
> + *			on error
> + */
> +char *util_read_fdt(const char *filename);
> +
> +/**
> + * Write a device tree buffer to a file.
> + *
> + * @param filename	The filename to write, or - for stdout
> + * @param buf		Poiner to buffer containing fdt
> + * @return 0 if ok, -1 on error
> + */
> +int util_write_fdt(const char *buf, const char *filename);
> +
> +/**
> + * Decode a node and property from a key string.
> + *
> + * This converts a key of the form /lcd/width into a node offset (/lcd) and
> + * a property (width).
> + *
> + * @param blob		Device tree blob
> + * @param key		Key string to decode
> + * @param prop		Returns pointer to property string within key
> + * @param guess_node	Normally <= 0, but if not then this is the offset of
> + *			a node that the caller MAY be referring to. For example
> + *			if key="/width" and there is a node /width, then
> + *			guess_node will return the offset of /width. It is used
> + *			to print a helpful error message.
> + */
> +int util_decode_key(const char *blob, char *key, char **prop, int *guess_node);
> +
> +#endif /* _UTILFDT_H */
> -- 
> 1.7.3.1
> 

[PATCH 6/9] Add new dtput utility to write values to fdt

[Simon Glass]

This simple utility allows writing of values into a device tree from the
command line.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 Makefile       |    7 ++
 Makefile.dtput |   14 ++++
 dtput.c        |  199 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 220 insertions(+), 0 deletions(-)
 create mode 100644 Makefile.dtput
 create mode 100644 dtput.c

diff --git a/Makefile b/Makefile
index 3401a65..e216006 100644
--- a/Makefile
+++ b/Makefile
@@ -107,11 +107,13 @@ include Makefile.convert-dtsv0
 include Makefile.dtc
 include Makefile.ftdump
 include Makefile.dtget
+include Makefile.dtput
 
 BIN += convert-dtsv0
 BIN += dtc
 BIN += ftdump
 BIN += dtget
+BIN += dtput
 
 SCRIPTS = dtdiff
 
@@ -123,6 +125,7 @@ ifneq ($(DEPTARGETS),)
 -include $(CONVERT_OBJS:%.o=%.d)
 -include $(FTDUMP_OBJS:%.o=%.d)
 -include $(DTGET_OBJS:%.o=%.d)
+-include $(DTPUT_OBJS:%.o=%.d)
 endif
 
 
@@ -187,6 +190,10 @@ dtget:	$(DTGET_OBJS) $(LIBFDT_archive)
 	$(VECHO) LD $@
 	$(LINK.c) -o $@ $^
 
+dtput:	$(DTPUT_OBJS) $(LIBFDT_archive)
+	$(VECHO) LD $@
+	$(LINK.c) -o $@ $^
+
 
 #
 # Testsuite rules
diff --git a/Makefile.dtput b/Makefile.dtput
new file mode 100644
index 0000000..92eb054
--- /dev/null
+++ b/Makefile.dtput
@@ -0,0 +1,14 @@
+#
+# This is not a complete Makefile.
+# Instead, it is designed to be easily embeddable
+# into other systems of Makefiles.
+#
+
+DTPUT_SRCS = \
+	dtput.c \
+	util.c \
+	utilfdt.c
+
+DTPUT_GEN_SRCS =
+
+DTPUT_OBJS = $(DTPUT_SRCS:%.c=%.o) $(DTPUT_GEN_SRCS:%.c=%.o)
diff --git a/dtput.c b/dtput.c
new file mode 100644
index 0000000..48e3c75
--- /dev/null
+++ b/dtput.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+ * Distributed under the terms of the GNU General Public License v2
+ *
+ * is_printable_string from ftdump.c
+ * Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
+ */
+
+#include <ctype.h>
+#include <getopt.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <libfdt.h>
+
+#include "util.h"
+#include "utilfdt.h"
+
+int verbose;		/* verbose output */
+
+enum {
+	MAX_VALUE_SIZE = 1024
+};
+
+static void report_error(const char *where, int err)
+{
+	fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
+}
+
+/**
+ * Encode a series of arguments in a property value.
+ *
+ * @param arg	List of arguments from command line
+ * @param arg_count	Number of arguments (may be 0)
+ * @param format	sscanf() format string to use, NULL for default
+ * @param type		Value type (s=string, i=int, b=byte)
+ * @param buf		Buffer to place value into
+ * @param malen		Maximum length of buffer
+ * @param *value_len	Returns length of value encoded
+ */
+static int encode_value(char **arg, int arg_count, const char *format,
+		int type, char *buf, int maxlen, int *value_len)
+{
+	char value[MAX_VALUE_SIZE];	/* holding area for values */
+	int len;	/* length of this cell/string/byte */
+	int *iptr = (int *)value;
+	int ival;
+	int upto;	/* the number of bytes we have written to buf */
+
+	upto = 0;
+
+	if (verbose)
+		fprintf(stderr, "Decoding value:\n");
+	for (; arg_count > 0; arg++, arg_count--, upto += len) {
+		/* assume integer unless told otherwise */
+		if (type == 's') {
+			sscanf(*arg, format ? format : "%s", value);
+			len = strlen(value) + 1;
+			if (verbose)
+				fprintf(stderr, "\tstring: '%s'\n", value);
+		} else {
+			len = type == 'b' ? 1 : 4;
+			sscanf(*arg, format ? format : "%d", &ival);
+			if (len == 4)
+				*iptr = cpu_to_fdt32(ival);
+			else
+				*value = (uint8_t)ival;
+			if (verbose) {
+				fprintf(stderr, "\t%s: %d\n",
+					type == 'b' ? "byte" : "int", ival);
+			}
+		}
+		if (upto + len > maxlen)
+			return 8;
+		memcpy(buf + upto, value, len);
+	}
+	*value_len = upto;
+	if (verbose)
+		fprintf(stderr, "Value size %d\n", upto);
+	return 0;
+}
+
+static int store_key_value(void *blob, char *key, const char *buf, int len)
+{
+	char *prop;
+	int node, guess_node;
+	int err;
+
+	node = util_decode_key(blob, key, &prop, &guess_node);
+	if (node < 0) {
+		report_error(key, node);
+		return 7;
+	}
+
+	err = fdt_setprop(blob, node, prop, buf, len);
+	if (err) {
+		report_error(prop, len);
+		if (guess_node)
+			fprintf(stderr, "(Node %s exists but you didn't "
+				"specify a property to set)\n", key);
+		return 8;
+	}
+	return 0;
+}
+
+static int do_dtput(const char *filename, const char *format, int type,
+		    char **arg, int arg_count)
+{
+	char *blob;
+	int ret, len;
+	char buf[MAX_VALUE_SIZE];
+
+	blob = util_read_fdt(filename);
+	if (!blob)
+		return 10;
+
+	if (arg_count > 0) {
+		/* convert the value into binary */
+		ret = encode_value(arg + 1, arg_count - 1, format, type, buf,
+				MAX_VALUE_SIZE, &len);
+		if (ret)
+			return 8;
+
+		ret = store_key_value(blob, *arg, buf, len);
+		if (ret)
+			return ret;
+	}
+	return util_write_fdt(blob, filename);
+}
+
+static void usage(const char *msg)
+{
+	if (msg)
+		fprintf(stderr, "Error: %s\n\n", msg);
+	fprintf(stderr, "dtput - write a property value to a device tree\n\n");
+	fprintf(stderr, "Usage: dtput <options> <dt file> <key> "
+		"[<value>...]\n");
+	fprintf(stderr, "Options:\n");
+	fprintf(stderr, "\t-f, --format <fmt>\tScanf format string to use "
+		"for value\n");
+	fprintf(stderr, "\t-t, --type <typechar>\tForce type to be string "
+		"(s), byte(b) or int(i)\n");
+	fprintf(stderr, "\t-h, --help\t\tPrint this help\n\n");
+	fprintf(stderr, "\t<key> is <node>/<property>, for example "
+		"/lcd/width for the width\n");
+	fprintf(stderr, "\tproperty in the LCD node\n");
+	exit(2);
+}
+
+int main(int argc, char *argv[])
+{
+	int c;
+	int ret;
+	char *format = NULL;
+	int type = 0;
+	char *filename = NULL;
+	static struct option long_options[] = {
+		{"format", 1, 0, 'f'},
+		{"help", 1, 0, 'h'},
+		{"type", 1, 0, 't'},
+		{"verbose", 1, 0, 'v'},
+		{0, 0, 0, 0}
+	};
+
+	for (;;) {
+		c = getopt_long(argc, argv, "f:ht:v", long_options, NULL);
+		if (c == -1)
+			break;
+
+		switch (c) {
+		case 'h':
+		case '?':
+			usage(NULL);
+
+		case 'f':
+			format = optarg;
+			break;
+
+		case 't':
+			type = *optarg;
+			break;
+
+		case 'v':
+			verbose = 1;
+			break;
+		}
+	}
+
+	if (optind < argc)
+		filename = argv[optind++];
+	if (!filename)
+		usage("Missing filename");
+	if (optind == argc)
+		usage("Missing key(s)");
+
+	ret = do_dtput(filename, format, type, argv + optind, argc - optind);
+	return ret;
+}
-- 
1.7.3.1

Re: [PATCH 6/9] Add new dtput utility to write values to fdt

[Grant Likely]

On Tue, Jul 05, 2011 at 12:02:54PM -0700, Simon Glass wrote:
> This simple utility allows writing of values into a device tree from the
> command line.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  Makefile       |    7 ++
>  Makefile.dtput |   14 ++++
>  dtput.c        |  199 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 220 insertions(+), 0 deletions(-)
>  create mode 100644 Makefile.dtput
>  create mode 100644 dtput.c
> 
> diff --git a/Makefile b/Makefile
> index 3401a65..e216006 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -107,11 +107,13 @@ include Makefile.convert-dtsv0
>  include Makefile.dtc
>  include Makefile.ftdump
>  include Makefile.dtget
> +include Makefile.dtput
>  
>  BIN += convert-dtsv0
>  BIN += dtc
>  BIN += ftdump
>  BIN += dtget
> +BIN += dtput
>  
>  SCRIPTS = dtdiff
>  
> @@ -123,6 +125,7 @@ ifneq ($(DEPTARGETS),)
>  -include $(CONVERT_OBJS:%.o=%.d)
>  -include $(FTDUMP_OBJS:%.o=%.d)
>  -include $(DTGET_OBJS:%.o=%.d)
> +-include $(DTPUT_OBJS:%.o=%.d)
>  endif
>  
>  
> @@ -187,6 +190,10 @@ dtget:	$(DTGET_OBJS) $(LIBFDT_archive)
>  	$(VECHO) LD $@
>  	$(LINK.c) -o $@ $^
>  
> +dtput:	$(DTPUT_OBJS) $(LIBFDT_archive)
> +	$(VECHO) LD $@
> +	$(LINK.c) -o $@ $^
> +
>  
>  #
>  # Testsuite rules
> diff --git a/Makefile.dtput b/Makefile.dtput
> new file mode 100644
> index 0000000..92eb054
> --- /dev/null
> +++ b/Makefile.dtput
> @@ -0,0 +1,14 @@
> +#
> +# This is not a complete Makefile.
> +# Instead, it is designed to be easily embeddable
> +# into other systems of Makefiles.
> +#
> +
> +DTPUT_SRCS = \
> +	dtput.c \
> +	util.c \
> +	utilfdt.c
> +
> +DTPUT_GEN_SRCS =
> +
> +DTPUT_OBJS = $(DTPUT_SRCS:%.c=%.o) $(DTPUT_GEN_SRCS:%.c=%.o)
> diff --git a/dtput.c b/dtput.c
> new file mode 100644
> index 0000000..48e3c75
> --- /dev/null
> +++ b/dtput.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> + * Distributed under the terms of the GNU General Public License v2
> + *
> + * is_printable_string from ftdump.c
> + * Contributed by Pantelis Antoniou <pantelis.antoniou AT gmail.com>
> + */
> +
> +#include <ctype.h>
> +#include <getopt.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <libfdt.h>
> +
> +#include "util.h"
> +#include "utilfdt.h"
> +
> +int verbose;		/* verbose output */
> +
> +enum {
> +	MAX_VALUE_SIZE = 1024
> +};
> +
> +static void report_error(const char *where, int err)
> +{
> +	fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
> +}
> +
> +/**
> + * Encode a series of arguments in a property value.
> + *
> + * @param arg	List of arguments from command line
> + * @param arg_count	Number of arguments (may be 0)
> + * @param format	sscanf() format string to use, NULL for default
> + * @param type		Value type (s=string, i=int, b=byte)
> + * @param buf		Buffer to place value into
> + * @param malen		Maximum length of buffer
> + * @param *value_len	Returns length of value encoded
> + */
> +static int encode_value(char **arg, int arg_count, const char *format,
> +		int type, char *buf, int maxlen, int *value_len)
> +{
> +	char value[MAX_VALUE_SIZE];	/* holding area for values */
> +	int len;	/* length of this cell/string/byte */
> +	int *iptr = (int *)value;
> +	int ival;
> +	int upto;	/* the number of bytes we have written to buf */
> +
> +	upto = 0;
> +
> +	if (verbose)
> +		fprintf(stderr, "Decoding value:\n");
> +	for (; arg_count > 0; arg++, arg_count--, upto += len) {
> +		/* assume integer unless told otherwise */
> +		if (type == 's') {
> +			sscanf(*arg, format ? format : "%s", value);
> +			len = strlen(value) + 1;
> +			if (verbose)
> +				fprintf(stderr, "\tstring: '%s'\n", value);
> +		} else {
> +			len = type == 'b' ? 1 : 4;
> +			sscanf(*arg, format ? format : "%d", &ival);
> +			if (len == 4)
> +				*iptr = cpu_to_fdt32(ival);
> +			else
> +				*value = (uint8_t)ival;
> +			if (verbose) {
> +				fprintf(stderr, "\t%s: %d\n",
> +					type == 'b' ? "byte" : "int", ival);
> +			}
> +		}
> +		if (upto + len > maxlen)
> +			return 8;
> +		memcpy(buf + upto, value, len);
> +	}
> +	*value_len = upto;
> +	if (verbose)
> +		fprintf(stderr, "Value size %d\n", upto);
> +	return 0;
> +}
> +
> +static int store_key_value(void *blob, char *key, const char *buf, int len)
> +{
> +	char *prop;
> +	int node, guess_node;
> +	int err;
> +
> +	node = util_decode_key(blob, key, &prop, &guess_node);
> +	if (node < 0) {
> +		report_error(key, node);
> +		return 7;
> +	}
> +
> +	err = fdt_setprop(blob, node, prop, buf, len);
> +	if (err) {
> +		report_error(prop, len);
> +		if (guess_node)
> +			fprintf(stderr, "(Node %s exists but you didn't "
> +				"specify a property to set)\n", key);
> +		return 8;
> +	}
> +	return 0;
> +}
> +
> +static int do_dtput(const char *filename, const char *format, int type,
> +		    char **arg, int arg_count)
> +{
> +	char *blob;
> +	int ret, len;
> +	char buf[MAX_VALUE_SIZE];
> +
> +	blob = util_read_fdt(filename);
> +	if (!blob)
> +		return 10;
> +
> +	if (arg_count > 0) {
> +		/* convert the value into binary */
> +		ret = encode_value(arg + 1, arg_count - 1, format, type, buf,
> +				MAX_VALUE_SIZE, &len);
> +		if (ret)
> +			return 8;
> +
> +		ret = store_key_value(blob, *arg, buf, len);
> +		if (ret)
> +			return ret;
> +	}
> +	return util_write_fdt(blob, filename);
> +}
> +
> +static void usage(const char *msg)
> +{
> +	if (msg)
> +		fprintf(stderr, "Error: %s\n\n", msg);
> +	fprintf(stderr, "dtput - write a property value to a device tree\n\n");
> +	fprintf(stderr, "Usage: dtput <options> <dt file> <key> "
> +		"[<value>...]\n");
> +	fprintf(stderr, "Options:\n");
> +	fprintf(stderr, "\t-f, --format <fmt>\tScanf format string to use "
> +		"for value\n");
> +	fprintf(stderr, "\t-t, --type <typechar>\tForce type to be string "
> +		"(s), byte(b) or int(i)\n");
> +	fprintf(stderr, "\t-h, --help\t\tPrint this help\n\n");
> +	fprintf(stderr, "\t<key> is <node>/<property>, for example "
> +		"/lcd/width for the width\n");
> +	fprintf(stderr, "\tproperty in the LCD node\n");

Can you make this text a single string and only call fprintf once?  It
will be easier to reqd that way.

It would be useful to have a mode for removing or renaming properties/nodes.

> +	exit(2);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int c;
> +	int ret;
> +	char *format = NULL;
> +	int type = 0;
> +	char *filename = NULL;
> +	static struct option long_options[] = {
> +		{"format", 1, 0, 'f'},
> +		{"help", 1, 0, 'h'},
> +		{"type", 1, 0, 't'},
> +		{"verbose", 1, 0, 'v'},
> +		{0, 0, 0, 0}
> +	};
> +
> +	for (;;) {
> +		c = getopt_long(argc, argv, "f:ht:v", long_options, NULL);
> +		if (c == -1)
> +			break;
> +
> +		switch (c) {
> +		case 'h':
> +		case '?':
> +			usage(NULL);
> +
> +		case 'f':
> +			format = optarg;
> +			break;
> +
> +		case 't':
> +			type = *optarg;
> +			break;
> +
> +		case 'v':
> +			verbose = 1;
> +			break;
> +		}
> +	}
> +
> +	if (optind < argc)
> +		filename = argv[optind++];
> +	if (!filename)
> +		usage("Missing filename");
> +	if (optind == argc)
> +		usage("Missing key(s)");
> +
> +	ret = do_dtput(filename, format, type, argv + optind, argc - optind);
> +	return ret;
> +}
> -- 
> 1.7.3.1
> 

Re: [PATCH 6/9] Add new dtput utility to write values to fdt

[Simon Glass]

Hi Grant,

On Wed, Jul 6, 2011 at 11:46 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Tue, Jul 05, 2011 at 12:02:54PM -0700, Simon Glass wrote:
>> This simple utility allows writing of values into a device tree from the
>> command line.
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
[snip]
>> +static void usage(const char *msg)
>> +{
>> +     if (msg)
>> +             fprintf(stderr, "Error: %s\n\n", msg);
>> +     fprintf(stderr, "dtput - write a property value to a device tree\n\n");
>> +     fprintf(stderr, "Usage: dtput <options> <dt file> <key> "
>> +             "[<value>...]\n");
>> +     fprintf(stderr, "Options:\n");
>> +     fprintf(stderr, "\t-f, --format <fmt>\tScanf format string to use "
>> +             "for value\n");
>> +     fprintf(stderr, "\t-t, --type <typechar>\tForce type to be string "
>> +             "(s), byte(b) or int(i)\n");
>> +     fprintf(stderr, "\t-h, --help\t\tPrint this help\n\n");
>> +     fprintf(stderr, "\t<key> is <node>/<property>, for example "
>> +             "/lcd/width for the width\n");
>> +     fprintf(stderr, "\tproperty in the LCD node\n");
>
> Can you make this text a single string and only call fprintf once?  It
> will be easier to reqd that way.

OK will do, thanks.

>
> It would be useful to have a mode for removing or renaming properties/nodes.

Yes, perhaps a -d flag to delete something (either a property or
node). I will look at this if my v2 patches pass muster.

Regards,
Simon

>
>> +     exit(2);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +     int c;
>> +     int ret;
>> +     char *format = NULL;
>> +     int type = 0;
>> +     char *filename = NULL;
>> +     static struct option long_options[] = {
>> +             {"format", 1, 0, 'f'},
>> +             {"help", 1, 0, 'h'},
>> +             {"type", 1, 0, 't'},
>> +             {"verbose", 1, 0, 'v'},
>> +             {0, 0, 0, 0}
>> +     };
>> +
>> +     for (;;) {
>> +             c = getopt_long(argc, argv, "f:ht:v", long_options, NULL);
>> +             if (c == -1)
>> +                     break;
>> +
>> +             switch (c) {
>> +             case 'h':
>> +             case '?':
>> +                     usage(NULL);
>> +
>> +             case 'f':
>> +                     format = optarg;
>> +                     break;
>> +
>> +             case 't':
>> +                     type = *optarg;
>> +                     break;
>> +
>> +             case 'v':
>> +                     verbose = 1;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (optind < argc)
>> +             filename = argv[optind++];
>> +     if (!filename)
>> +             usage("Missing filename");
>> +     if (optind == argc)
>> +             usage("Missing key(s)");
>> +
>> +     ret = do_dtput(filename, format, type, argv + optind, argc - optind);
>> +     return ret;
>> +}
>> --
>> 1.7.3.1
>>
>

[PATCH 7/9] Add some tests for dtput

[Simon Glass]

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 tests/dtput-runtest.sh |   50 ++++++++++++++++++++++++++++++++++++++++++++++++
 tests/run_tests.sh     |   40 +++++++++++++++++++++++++++++++++++++-
 tests/tests.sh         |    1 +
 3 files changed, 90 insertions(+), 1 deletions(-)
 create mode 100644 tests/dtput-runtest.sh

diff --git a/tests/dtput-runtest.sh b/tests/dtput-runtest.sh
new file mode 100644
index 0000000..a8c9655
--- /dev/null
+++ b/tests/dtput-runtest.sh
@@ -0,0 +1,50 @@
+#! /bin/sh
+
+# Run script for dtput tests
+# We run dtput to update the device tree, thn dtget to check it
+
+# Usage
+#	dtput-runtest.sh name expected_output dtb_file key value flags
+
+. ./tests.sh
+
+LOG="tmp.log.$$"
+EXPECT="tmp.expect.$$"
+
+rm -f $TMPFILE $LOG
+
+echo "$1" >$EXPECT
+dtb="$2"
+key="$3"
+value="$4"
+flags="$5"
+
+# First run dtput
+verbose_run $VALGRIND "$DTPUT" "$dtb" "$key" $value $flags
+ret="$?"
+
+if [ "$ret" -gt 127 ]; then
+    signame=$(kill -l $[ret - 128])
+    FAIL "Killed by SIG$signame"
+fi
+
+
+# Now dtget to read the value
+verbose_run_log "$LOG" $VALGRIND "$DTGET" "$dtb" "$key" $flags
+ret="$?"
+
+if [ "$ret" -gt 127 ]; then
+    signame=$(kill -l $[ret - 128])
+    FAIL "Killed by SIG$signame"
+fi
+
+diff $EXPECT $LOG
+ret="$?"
+
+rm -f $LOG $EXPECT
+
+if [ "$ret" -eq 0 ]; then
+	PASS
+else
+	FAIL
+fi
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 01a2b60..d95d1fe 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -84,11 +84,25 @@ asm_to_so_test () {
 }
 
 run_dtget_test () {
+    # run_dtget_test name expected_output dtb_file args...
     echo -n "$1:	"
     shift
     base_run_test sh dtget-runtest.sh "$@"
 }
 
+run_dtput_test () {
+    # run_dtput_test name expected_output dtb_file key value flags
+    echo -n "$1:	"
+    shift
+    output="$1"
+    dtb="$2"
+    key="$3"
+    value="$4"
+    flags="$5"
+    echo output: $output
+    base_run_test sh dtput-runtest.sh "$output" "$dtb" "$key" "$value" "$flags"
+}
+
 tree1_tests () {
     TREE=$1
 
@@ -416,6 +430,27 @@ dtget_tests () {
 	-t i $file "/randomnode/blob"
 }
 
+dtput_tests () {
+    file=dtget-test.dtb
+    $DTC -O dtb -o $file ${file%.dtb}.dts 2>/dev/null
+
+    # run_dtput_test <test-name> <expected-result> <file> <key> <value>
+    run_dtput_test "Simple string" "a_model" $file "/model" "a_model" -ts
+
+    run_dtput_test "Multiple string s" "board1 board2" \
+	$file "/compatible" "board1 board2" -ts
+    run_dtput_test "Integer" "32768" $file "/cpus/PowerPC,970@1/d-cache-size" \
+	"32768"
+    run_dtput_test "Integer hex" "8001" $file \
+	"/cpus/PowerPC,970@1/d-cache-size" 0x8001 "-f %x"
+    run_dtput_test "Integer list" "02 03 04" $file \
+	"/randomnode/tricky1" "02 03 04" "-f %02x -t b"
+    run_dtput_test "Byte list short" "a b c ea ad be ef" \
+	$file "/randomnode/blob" "a b c ea ad be ef" "-f %x -t b"
+    run_dtput_test "Integer list short" "a0b0c0d deeaae ef000000" \
+	$file "/randomnode/blob" "a0b0c0d deeaae ef000000" "-t i -f %x"
+}
+
 while getopts "vt:m" ARG ; do
     case $ARG in
 	"v")
@@ -431,7 +466,7 @@ while getopts "vt:m" ARG ; do
 done
 
 if [ -z "$TESTSETS" ]; then
-    TESTSETS="libfdt dtc dtbs_equal dtget"
+    TESTSETS="libfdt dtc dtbs_equal dtget dtput"
 fi
 
 # Make sure we don't have stale blobs lying around
@@ -451,6 +486,9 @@ for set in $TESTSETS; do
 	"dtget")
 	    dtget_tests
 	    ;;
+	"dtput")
+	    dtput_tests
+	    ;;
     esac
 done
 
diff --git a/tests/tests.sh b/tests/tests.sh
index cf7f19e..bdd3c79 100644
--- a/tests/tests.sh
+++ b/tests/tests.sh
@@ -12,6 +12,7 @@ FAIL () {
 
 DTC=../dtc
 DTGET=../dtget
+DTPUT=../dtput
 
 verbose_run () {
     if [ -z "$QUIET_TEST" ]; then
-- 
1.7.3.1

[PATCH 8/9] Add dtput to .gitignore

[Simon Glass]

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 .gitignore |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index cde4fa5..a026069 100644
--- a/.gitignore
+++ b/.gitignore
@@ -11,3 +11,4 @@ lex.yy.c
 /convert-dtsv0
 /version_gen.h
 /dtget
+/dtput
-- 
1.7.3.1

Re: [PATCH 8/9] Add dtput to .gitignore

[Grant Likely]

On Tue, Jul 05, 2011 at 12:02:56PM -0700, Simon Glass wrote:
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Make this part of the dtput patch.

> ---
>  .gitignore |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index cde4fa5..a026069 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -11,3 +11,4 @@ lex.yy.c
>  /convert-dtsv0
>  /version_gen.h
>  /dtget
> +/dtput
> -- 
> 1.7.3.1
> 

[PATCH 9/9] dtput: Support adding strings with spaces

[Simon Glass]

Previously dtput would use sscanf() to read the string from the command line,
even if no format string was provided. Thus it would split up words at
whitespace boundaries, and put a \0 between each.

This changes that behavior. Previously:

$ ../dtput dtget-test.dtb -ts /compatible this is a test

$ ../dtget dtget-test.dtb /compatible
116 104 105 115 0 105 115 0 97 110 111 116 104 101 114 0 116 101 115 116 0

and you have to use the -ts flag to see this as a list of strings

$ ../dtget dtget-test.dtb -ts /compatible
this is a test

Now:

$ ../dtput dtget-test.dtb -ts  /compatible "this is another test"

$ ../dtget dtget-test.dtb /compatible this is another test
this is another test

$ ../dtget dtget-test.dtb /compatible -tb
116 104 105 115 32 105 115 32 97 110 111 116 104 101 114 32 116 101 115 116 0

Note that the \0 terminators are gone.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 dtput.c            |    7 ++++++-
 tests/run_tests.sh |    4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/dtput.c b/dtput.c
index 48e3c75..d02e6ab 100644
--- a/dtput.c
+++ b/dtput.c
@@ -55,7 +55,12 @@ static int encode_value(char **arg, int arg_count, const char *format,
 	for (; arg_count > 0; arg++, arg_count--, upto += len) {
 		/* assume integer unless told otherwise */
 		if (type == 's') {
-			sscanf(*arg, format ? format : "%s", value);
+			if (format)
+				sscanf(*arg, format, value);
+			else {
+				strncpy(value, *arg, MAX_VALUE_SIZE - 1);
+				value[MAX_VALUE_SIZE - 1] = '\0';
+			}
 			len = strlen(value) + 1;
 			if (verbose)
 				fprintf(stderr, "\tstring: '%s'\n", value);
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index d95d1fe..71cfab3 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -434,10 +434,12 @@ dtput_tests () {
     file=dtget-test.dtb
     $DTC -O dtb -o $file ${file%.dtb}.dts 2>/dev/null
 
-    # run_dtput_test <test-name> <expected-result> <file> <key> <value>
+    # run_dtput_test <test-name> <expected-result> <file> <key> <value> <flags>
     run_dtput_test "Simple string" "a_model" $file "/model" "a_model" -ts
 
     run_dtput_test "Multiple string s" "board1 board2" \
+	$file "/compatible" "board1 board2" -ts -f%s
+    run_dtput_test "Single string with spaces" "board1 board2" \
 	$file "/compatible" "board1 board2" -ts
     run_dtput_test "Integer" "32768" $file "/cpus/PowerPC,970@1/d-cache-size" \
 	"32768"
-- 
1.7.3.1

Re: [PATCH 9/9] dtput: Support adding strings with spaces

[Grant Likely]

On Tue, Jul 05, 2011 at 12:02:57PM -0700, Simon Glass wrote:
> Previously dtput would use sscanf() to read the string from the command line,
> even if no format string was provided. Thus it would split up words at
> whitespace boundaries, and put a \0 between each.
> 
> This changes that behavior. Previously:
> 
> $ ../dtput dtget-test.dtb -ts /compatible this is a test
> 
> $ ../dtget dtget-test.dtb /compatible
> 116 104 105 115 0 105 115 0 97 110 111 116 104 101 114 0 116 101 115 116 0
> 
> and you have to use the -ts flag to see this as a list of strings
> 
> $ ../dtget dtget-test.dtb -ts /compatible
> this is a test
> 
> Now:
> 
> $ ../dtput dtget-test.dtb -ts  /compatible "this is another test"
> 
> $ ../dtget dtget-test.dtb /compatible this is another test
> this is another test
> 
> $ ../dtget dtget-test.dtb /compatible -tb
> 116 104 105 115 32 105 115 32 97 110 111 116 104 101 114 32 116 101 115 116 0
> 
> Note that the \0 terminators are gone.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

This would be squashed in with the dtput patch too.

g.

> ---
>  dtput.c            |    7 ++++++-
>  tests/run_tests.sh |    4 +++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/dtput.c b/dtput.c
> index 48e3c75..d02e6ab 100644
> --- a/dtput.c
> +++ b/dtput.c
> @@ -55,7 +55,12 @@ static int encode_value(char **arg, int arg_count, const char *format,
>  	for (; arg_count > 0; arg++, arg_count--, upto += len) {
>  		/* assume integer unless told otherwise */
>  		if (type == 's') {
> -			sscanf(*arg, format ? format : "%s", value);
> +			if (format)
> +				sscanf(*arg, format, value);
> +			else {
> +				strncpy(value, *arg, MAX_VALUE_SIZE - 1);
> +				value[MAX_VALUE_SIZE - 1] = '\0';
> +			}
>  			len = strlen(value) + 1;
>  			if (verbose)
>  				fprintf(stderr, "\tstring: '%s'\n", value);
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index d95d1fe..71cfab3 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -434,10 +434,12 @@ dtput_tests () {
>      file=dtget-test.dtb
>      $DTC -O dtb -o $file ${file%.dtb}.dts 2>/dev/null
>  
> -    # run_dtput_test <test-name> <expected-result> <file> <key> <value>
> +    # run_dtput_test <test-name> <expected-result> <file> <key> <value> <flags>
>      run_dtput_test "Simple string" "a_model" $file "/model" "a_model" -ts
>  
>      run_dtput_test "Multiple string s" "board1 board2" \
> +	$file "/compatible" "board1 board2" -ts -f%s
> +    run_dtput_test "Single string with spaces" "board1 board2" \
>  	$file "/compatible" "board1 board2" -ts
>      run_dtput_test "Integer" "32768" $file "/cpus/PowerPC,970@1/d-cache-size" \
>  	"32768"
> -- 
> 1.7.3.1
>