[PATCH v2 0/6] Add fdtget and fdtput for access to fdt from build system

[PATCH v2 0/6] Add fdtget and fdtput for access to fdt from build system

[Simon Glass]

This patch set adds two new utilities:

   fdtget for reading properties from a device tree binary file
   fdtput 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, fdtget 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, fdtput 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 the parameter format has been
revised after previous feedback.

Changes in v2:
- Adjust tests for new fdtput arguments
- Add test for multiple strings
- Add test for exhausting fdt space
- Separate arguments for node and property
- Remove limits on data size of property writting to fdt
- Remove use of getopt_long()
- Adjust tests for new fdtget arguments
- Merge two related commits into this one
- Use structure for storing display options
- Remove util_decode_key
- Add utilfdt_decode_type to be used by fdtget/put
- Remove limits on device tree binary size

Simon Glass (6):
  Add utilfdt for common functions
  ftdump: use util_read_fdt
  Add fdtget utility to read property values from device tree
  fdtget: Add basic tests
  Add new fdtput utility to write values to fdt
  fdtput: Add basic tests

 .gitignore              |    2 +
 Makefile                |   12 +++-
 Makefile.fdtget         |   12 +++
 Makefile.fdtput         |   12 +++
 Makefile.ftdump         |    3 +-
 fdtget.c                |  200 +++++++++++++++++++++++++++++++++++++++++++
 fdtput.c                |  217 +++++++++++++++++++++++++++++++++++++++++++++++
 ftdump.c                |   33 +------
 tests/fdtget-runtest.sh |   35 ++++++++
 tests/fdtput-runtest.sh |   55 ++++++++++++
 tests/run_tests.sh      |  105 ++++++++++++++++++++++-
 tests/tests.sh          |    2 +
 utilfdt.c               |  120 ++++++++++++++++++++++++++
 utilfdt.h               |   58 +++++++++++++
 14 files changed, 835 insertions(+), 31 deletions(-)
 create mode 100644 Makefile.fdtget
 create mode 100644 Makefile.fdtput
 create mode 100644 fdtget.c
 create mode 100644 fdtput.c
 create mode 100755 tests/fdtget-runtest.sh
 create mode 100644 tests/fdtput-runtest.sh
 create mode 100644 utilfdt.c
 create mode 100644 utilfdt.h

-- 
1.7.3.1

[PATCH v2 1/6] Add utilfdt for common functions

[Simon Glass]

This adds a new utility library for performing libfdt operations.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Remove util_decode_key
- Add utilfdt_decode_type to be used by fdtget/put
- Remove limits on device tree binary size

 utilfdt.c |  120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 utilfdt.h |   58 +++++++++++++++++++++++++++++
 2 files changed, 178 insertions(+), 0 deletions(-)
 create mode 100644 utilfdt.c
 create mode 100644 utilfdt.h

diff --git a/utilfdt.c b/utilfdt.c
new file mode 100644
index 0000000..5c3682e
--- /dev/null
+++ b/utilfdt.c
@@ -0,0 +1,120 @@
+/*
+ * Copyright 2011 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 <stdio.h>
+#include <stdlib.h>
+#include <sys/stat.h>
+
+#include "libfdt.h"
+#include "utilfdt.h"
+
+char *util_read_fdt(const char *filename)
+{
+	FILE *fp;
+	char *buf = NULL;
+	struct stat stat_buf;
+	off_t size, upto = 0, toread;
+	int err = 0;
+
+	/* Open the file, or stdin, and work out how many bytes to read */
+	if (strcmp(filename, "-") == 0) {
+		fp = stdin;
+		toread = 64 * 1024;	/* Suitable likely maximum */
+	} else {
+		fp = fopen(filename, "rb");
+		if (fp == NULL) {
+			fprintf(stderr, "unable to open '%s'\n", filename);
+			return NULL;
+		}
+		if (fstat(fileno(fp), &stat_buf)) {
+			fprintf(stderr, "couldn't get size of file\n");
+			return NULL;
+		}
+		toread = stat_buf.st_size;
+	}
+
+	/* Loop until we have everything */
+	size = toread;
+	while (!feof(fp)) {
+		buf = realloc(buf, size);
+		if (!buf) {
+			fprintf(stderr, "couldn't allocate %ld byte buffer\n",
+				size);
+			return NULL;
+		}
+
+		upto += fread(buf + upto, 1, toread, fp);
+		if (ferror(fp)) {
+			fprintf(stderr, "file read error\n");
+			err = -1;
+			break;
+		}
+
+		/* Expand the buffer in case we need to read more */
+		size += toread;
+	}
+
+	/* Clean up, returning NULL on error */
+	if (fp != stdin)
+		fclose(fp);
+	if (err) {
+		free(buf);
+		buf = NULL;
+	}
+	return buf;
+}
+
+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 utilfdt_decode_type(const char *arg, int *type, int *format)
+{
+	const char *s;
+
+	for (s = arg; *s; s++) {
+		if (strchr("iubs", *s))
+			*type = *s;
+		else if (*s == 'x')
+			*format = *s;
+		else
+			return -1;
+	}
+	return 0;
+}
diff --git a/utilfdt.h b/utilfdt.h
new file mode 100644
index 0000000..7c2828a
--- /dev/null
+++ b/utilfdt.h
@@ -0,0 +1,58 @@
+#ifndef _UTILFDT_H
+#define _UTILFDT_H
+
+/*
+ * Copyright 2011 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 data type string.
+ *
+ * The string consists of:
+ *	One optional character (s=string, i=int, u=unsigned b=byte)
+ *	Optional format character (x=hex)
+ *
+ * If either of type or format is not found, then that variable is not
+ * updated, and the caller's default will be used.
+ *
+ * @param arg		Argument string to process
+ * @param type		Returns type found, if any
+ * @param format	Returns format fonud, if any
+ * @return 0 if ok, -1 on error
+ */
+int utilfdt_decode_type(const char *arg, int *type, int *format);
+
+#endif /* _UTILFDT_H */
-- 
1.7.3.1

Re: [PATCH v2 1/6] Add utilfdt for common functions

[David Gibson]

On Wed, Sep 07, 2011 at 12:54:15PM -0700, Simon Glass wrote:
> This adds a new utility library for performing libfdt operations.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Changes in v2:
> - Remove util_decode_key
> - Add utilfdt_decode_type to be used by fdtget/put
> - Remove limits on device tree binary size
> 
>  utilfdt.c |  120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  utilfdt.h |   58 +++++++++++++++++++++++++++++
>  2 files changed, 178 insertions(+), 0 deletions(-)
>  create mode 100644 utilfdt.c
>  create mode 100644 utilfdt.h
> 
> diff --git a/utilfdt.c b/utilfdt.c
> new file mode 100644
> index 0000000..5c3682e
> --- /dev/null
> +++ b/utilfdt.c
> @@ -0,0 +1,120 @@
> +/*
> + * Copyright 2011 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 <stdio.h>
> +#include <stdlib.h>
> +#include <sys/stat.h>
> +
> +#include "libfdt.h"
> +#include "utilfdt.h"
> +
> +char *util_read_fdt(const char *filename)
> +{
> +	FILE *fp;
> +	char *buf = NULL;
> +	struct stat stat_buf;
> +	off_t size, upto = 0, toread;
> +	int err = 0;
> +
> +	/* Open the file, or stdin, and work out how many bytes to read */
> +	if (strcmp(filename, "-") == 0) {
> +		fp = stdin;
> +		toread = 64 * 1024;	/* Suitable likely maximum */

Arbitrary limits, yuck.

> +	} else {
> +		fp = fopen(filename, "rb");
> +		if (fp == NULL) {
> +			fprintf(stderr, "unable to open '%s'\n", filename);
> +			return NULL;
> +		}
> +		if (fstat(fileno(fp), &stat_buf)) {
> +			fprintf(stderr, "couldn't get size of file\n");
> +			return NULL;

stdin is not the only possibility for something that's not stat()able.

You shouldn't use stat() at all to get the size, but either just
read() until EOF, or read the header first and use the size in there.

Wait.. I already wrote this function.. look at load_blob() in
tests/testutils.c.  Why don't you give it a cleaner name, rather than
rewriting it badly.

> +		}
> +		toread = stat_buf.st_size;
> +	}
> +
> +	/* Loop until we have everything */
> +	size = toread;
> +	while (!feof(fp)) {
> +		buf = realloc(buf, size);
> +		if (!buf) {
> +			fprintf(stderr, "couldn't allocate %ld byte buffer\n",
> +				size);
> +			return NULL;
> +		}
> +
> +		upto += fread(buf + upto, 1, toread, fp);
> +		if (ferror(fp)) {
> +			fprintf(stderr, "file read error\n");
> +			err = -1;
> +			break;
> +		}
> +
> +		/* Expand the buffer in case we need to read more */
> +		size += toread;
> +	}
> +
> +	/* Clean up, returning NULL on error */
> +	if (fp != stdin)
> +		fclose(fp);
> +	if (err) {
> +		free(buf);
> +		buf = NULL;
> +	}
> +	return buf;
> +}
> +
> +int util_write_fdt(const char *buf, const char *filename)

save_blob() also in testutils.c

[snip]
> +/**
> + * Decode a data type string.
> + *
> + * The string consists of:
> + *	One optional character (s=string, i=int, u=unsigned b=byte)
> + *	Optional format character (x=hex)
> + *
> + * If either of type or format is not found, then that variable is not
> + * updated, and the caller's default will be used.

Yes, but what does the function actually *do*?

-- 
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 v2 1/6] Add utilfdt for common functions

[Simon Glass]

Hi David,

On Wed, Sep 7, 2011 at 10:20 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Sep 07, 2011 at 12:54:15PM -0700, Simon Glass wrote:
>> This adds a new utility library for performing libfdt operations.
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>> Changes in v2:
>> - Remove util_decode_key
>> - Add utilfdt_decode_type to be used by fdtget/put
>> - Remove limits on device tree binary size

[snip]

>> +     if (strcmp(filename, "-") == 0) {
>> +             fp = stdin;
>> +             toread = 64 * 1024;     /* Suitable likely maximum */
>
> Arbitrary limits, yuck.

There is no arbitrary limit here - it is just the number of bytes read
initially.

>
>> +     } else {
>> +             fp = fopen(filename, "rb");
>> +             if (fp == NULL) {
>> +                     fprintf(stderr, "unable to open '%s'\n", filename);
>> +                     return NULL;
>> +             }
>> +             if (fstat(fileno(fp), &stat_buf)) {
>> +                     fprintf(stderr, "couldn't get size of file\n");
>> +                     return NULL;
>
> stdin is not the only possibility for something that's not stat()able.
>
> You shouldn't use stat() at all to get the size, but either just
> read() until EOF, or read the header first and use the size in there.
>
> Wait.. I already wrote this function.. look at load_blob() in
> tests/testutils.c.  Why don't you give it a cleaner name, rather than
> rewriting it badly.

Thanks for the pointer to the existing code. It looks very similar
except for my use of stat and its use of xmalloc() and CONFIG. Also I
note that it uses open() rather than fopen(). I will need to move it
to the library and change it a bit.

It uses this macro to display errors:

#define CONFIG(fmt, ...)				\
	do {						\
		cleanup();				\
		printf("Bad configuration: " fmt "\n", ##__VA_ARGS__);	\
		exit(RC_CONFIG);			\
	} while (0)

Given that I am trying to write a general utility function I would
prefer not to bomb out in a low-level function, but to return an error
result.

So are you ok with the error message bring different in this case (not
printing 'Bad configuration' before the error, and going out on
stderr)? Then I can make all the test code call this function in
libfdt.

[snip]
> [snip]
>> +/**
>> + * Decode a data type string.
>> + *
>> + * The string consists of:
>> + *   One optional character (s=string, i=int, u=unsigned b=byte)
>> + *   Optional format character (x=hex)
>> + *
>> + * If either of type or format is not found, then that variable is not
>> + * updated, and the caller's default will be used.
>
> Yes, but what does the function actually *do*?

I don't follow - are you asking that I make it more specific like
"Decode a data type string as used by fdtget and fdtput to specify the
data type format to use for property values"?

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
>

Re: [PATCH v2 1/6] Add utilfdt for common functions

[David Gibson]

On Thu, Sep 08, 2011 at 05:37:08AM -0700, Simon Glass wrote:
> Hi David,
> 
> On Wed, Sep 7, 2011 at 10:20 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Sep 07, 2011 at 12:54:15PM -0700, Simon Glass wrote:
> >> This adds a new utility library for performing libfdt operations.
> >>
> >> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> ---
> >> Changes in v2:
> >> - Remove util_decode_key
> >> - Add utilfdt_decode_type to be used by fdtget/put
> >> - Remove limits on device tree binary size
> 
> [snip]
> 
> >> +     if (strcmp(filename, "-") == 0) {
> >> +             fp = stdin;
> >> +             toread = 64 * 1024;     /* Suitable likely maximum */
> >
> > Arbitrary limits, yuck.
> 
> There is no arbitrary limit here - it is just the number of bytes read
> initially.

Hrm, ok.

> >> +     } else {
> >> +             fp = fopen(filename, "rb");
> >> +             if (fp == NULL) {
> >> +                     fprintf(stderr, "unable to open '%s'\n", filename);
> >> +                     return NULL;
> >> +             }
> >> +             if (fstat(fileno(fp), &stat_buf)) {
> >> +                     fprintf(stderr, "couldn't get size of file\n");
> >> +                     return NULL;
> >
> > stdin is not the only possibility for something that's not stat()able.
> >
> > You shouldn't use stat() at all to get the size, but either just
> > read() until EOF, or read the header first and use the size in there.
> >
> > Wait.. I already wrote this function.. look at load_blob() in
> > tests/testutils.c.  Why don't you give it a cleaner name, rather than
> > rewriting it badly.
> 
> Thanks for the pointer to the existing code. It looks very similar
> except for my use of stat and its use of xmalloc() and CONFIG. Also I
> note that it uses open() rather than fopen(). I will need to move it
> to the library and change it a bit.
> 
> It uses this macro to display errors:
> 
> #define CONFIG(fmt, ...)				\
> 	do {						\
> 		cleanup();				\
> 		printf("Bad configuration: " fmt "\n", ##__VA_ARGS__);	\
> 		exit(RC_CONFIG);			\
> 	} while (0)
> 
> Given that I am trying to write a general utility function I would
> prefer not to bomb out in a low-level function, but to return an error
> result.

Yes, you'll need to fix that for a general library version.  It
should indeed return an error rather than dying.

> So are you ok with the error message bring different in this case (not
> printing 'Bad configuration' before the error, and going out on
> stderr)? Then I can make all the test code call this function in
> libfdt.

I don't think you'll want to direct replace the tests' load_blob()
with your util.  Instead make load_blob() a little wrapper that uses
your load function and bombs out with CONFIG() if it fails.  Use of
CONFIG() in the testcases is important for correctly collating the
test results (specifically it means "we couldn't complete this test
because the environment hasn't been set up correctly" rather than a
true pass or fail).

[snip]
> [snip]
> > [snip]
> >> +/**
> >> + * Decode a data type string.
> >> + *
> >> + * The string consists of:
> >> + *   One optional character (s=string, i=int, u=unsigned b=byte)
> >> + *   Optional format character (x=hex)
> >> + *
> >> + * If either of type or format is not found, then that variable is not
> >> + * updated, and the caller's default will be used.
> >
> > Yes, but what does the function actually *do*?
> 
> I don't follow - are you asking that I make it more specific like
> "Decode a data type string as used by fdtget and fdtput to specify the
> data type format to use for property values"?

Well, I've found almost every description of your format strings to be
confusing.  But in this case I'm asking what does "decode" mean - in
particular what are the outputs.  From further reading, it seems it
means splitting the "type" part from the "format" part, but that's far
from obvious at first glance.

-- 
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 v2 2/6] ftdump: use util_read_fdt

[Simon Glass]

Now that we have this function, ftdump should use it too.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 Makefile        |    2 +-
 Makefile.ftdump |    3 ++-
 ftdump.c        |   33 +++++----------------------------
 3 files changed, 8 insertions(+), 30 deletions(-)

diff --git a/Makefile b/Makefile
index 2172d9a..0baada5 100644
--- a/Makefile
+++ b/Makefile
@@ -178,7 +178,7 @@ convert-dtsv0: $(CONVERT_OBJS)
 	@$(VECHO) LD $@
 	$(LINK.c) -o $@ $^
 
-ftdump:	$(FTDUMP_OBJS)
+ftdump:	$(FTDUMP_OBJS) $(LIBFDT_archive)
 
 
 #
diff --git a/Makefile.ftdump b/Makefile.ftdump
index 2744a18..dc3a2c1 100644
--- a/Makefile.ftdump
+++ b/Makefile.ftdump
@@ -6,7 +6,8 @@
 
 FTDUMP_SRCS = \
 	ftdump.c \
-	util.c
+	util.c \
+	utilfdt.c
 
 FTDUMP_GEN_SRCS =
 
diff --git a/ftdump.c b/ftdump.c
index db932e3..43c09c0 100644
--- a/ftdump.c
+++ b/ftdump.c
@@ -12,8 +12,7 @@
 #include <libfdt_env.h>
 
 #include "util.h"
-
-#define FTDUMP_BUF_SIZE	65536
+#include "utilfdt.h"
 
 #define ALIGN(x, a)	(((x) + ((a) - 1)) & ~((a) - 1))
 #define PALIGN(p, a)	((void *)(ALIGN((unsigned long)(p), (a))))
@@ -147,40 +146,18 @@ static void dump_blob(void *blob)
 
 int main(int argc, char *argv[])
 {
-	FILE *fp;
 	char *buf;
-	int size;
 
 	if (argc < 2) {
 		fprintf(stderr, "supply input filename\n");
 		return 5;
 	}
 
-	if (strcmp(argv[1], "-") == 0) {
-		fp = stdin;
-	} else {
-		fp = fopen(argv[1], "rb");
-		if (fp == NULL) {
-			fprintf(stderr, "unable to open %s\n", argv[1]);
-			return 10;
-		}
-	}
-
-	buf = malloc(FTDUMP_BUF_SIZE);
-	if (!buf) {
-		fprintf(stderr, "Couldn't allocate %d byte buffer\n", FTDUMP_BUF_SIZE);
+	buf = util_read_fdt(argv[1]);
+	if (buf)
+		dump_blob(buf);
+	else
 		return 10;
-	}
-
-	size = fread(buf, 1, FTDUMP_BUF_SIZE, fp);
-	if (size == FTDUMP_BUF_SIZE) {
-		fprintf(stderr, "file too large (maximum is %d bytes)\n", FTDUMP_BUF_SIZE);
-		return 10;
-	}
-
-	dump_blob(buf);
-
-	fclose(fp);
 
 	return 0;
 }
-- 
1.7.3.1

[PATCH v2 3/6] Add fdtget 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. It is written in C and uses the same libfdt as the rest of the dtc
package.

What is it for:
- Reading fdt values from scripts
- Extracting fdt information within build systems
- Looking at particular values without having to dump the entire tree

To use it, specify the fdt binary file on command line followed by a list of
node, property pairs. The utility then looks up each node, finds the property
and displays the value.

Each value is printed on a new line.

fdtget tries to guess the type of each property based on its contents. This
is not always reliable, so you can use the -t option to force fdtget to decode
the value as a string, or byte, etc.

To read from stdin, use - as the file.

Usage:
	fdtget <options> <dt file> [<node> <property>]...
Options:
	-t [<type>][<format>]	Type of data
	-h		Print this help

	<type>		One character (s=string, i=int, u=unsigned, b=byte)
	<format>	Optional format character (x=hex)

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Separate arguments for node and property
- Merge two related commits into this one
- Use structure for storing display options
- Remove use of getopt_long()

 .gitignore      |    1 +
 Makefile        |    4 +
 Makefile.fdtget |   12 +++
 fdtget.c        |  200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 217 insertions(+), 0 deletions(-)
 create mode 100644 Makefile.fdtget
 create mode 100644 fdtget.c

diff --git a/.gitignore b/.gitignore
index ae7a46a..9f27f34 100644
--- a/.gitignore
+++ b/.gitignore
@@ -10,3 +10,4 @@ lex.yy.c
 /ftdump
 /convert-dtsv0
 /version_gen.h
+/fdtget
diff --git a/Makefile b/Makefile
index 0baada5..3d460d7 100644
--- a/Makefile
+++ b/Makefile
@@ -106,10 +106,12 @@ endef
 include Makefile.convert-dtsv0
 include Makefile.dtc
 include Makefile.ftdump
+include Makefile.fdtget
 
 BIN += convert-dtsv0
 BIN += dtc
 BIN += ftdump
+BIN += fdtget
 
 SCRIPTS = dtdiff
 
@@ -120,6 +122,7 @@ ifneq ($(DEPTARGETS),)
 -include $(DTC_OBJS:%.o=%.d)
 -include $(CONVERT_OBJS:%.o=%.d)
 -include $(FTDUMP_OBJS:%.o=%.d)
+-include $(FDTGET_OBJS:%.o=%.d)
 endif
 
 
@@ -180,6 +183,7 @@ convert-dtsv0: $(CONVERT_OBJS)
 
 ftdump:	$(FTDUMP_OBJS) $(LIBFDT_archive)
 
+fdtget:	$(FDTGET_OBJS) $(LIBFDT_archive)
 
 #
 # Testsuite rules
diff --git a/Makefile.fdtget b/Makefile.fdtget
new file mode 100644
index 0000000..c1ab041
--- /dev/null
+++ b/Makefile.fdtget
@@ -0,0 +1,12 @@
+#
+# This is not a complete Makefile.
+# Instead, it is designed to be easily embeddable
+# into other systems of Makefiles.
+#
+
+FDTGET_SRCS = \
+	fdtget.c \
+	util.c \
+	utilfdt.c
+
+FDTGET_OBJS = $(FDTGET_SRCS:%.c=%.o)
diff --git a/fdtget.c b/fdtget.c
new file mode 100644
index 0000000..4a94e99
--- /dev/null
+++ b/fdtget.c
@@ -0,0 +1,200 @@
+/*
+ * 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"
+
+/* Holds information which controls our output and options */
+struct display_info {
+	int type;		/* data type (s/i/u/b or 0 for default) */
+	int format;		/* data format (x or 0 for default) */
+};
+
+static void report_error(const char *where, int err)
+{
+	fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
+}
+
+/**
+ * Displays data of a given length according to selected options
+ *
+ * If a specific data type is provided in disp, then this is used. Otherwise
+ * we try to guess the data type / size from the contents.
+ *
+ * @param disp		Display information / options
+ * @param data		Data to display
+ * @param len		Maximum length of buffer
+ */
+static void show_data(struct display_info *disp, 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 = (disp->type) == 's' ||
+		(!disp->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("%s", (const char *)s);
+		}
+		return;
+	}
+	if (disp->type == 'i' || disp->type == 'u')
+		size = 4;
+	else if (disp->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(disp->format == 'x' ? "%x" : "%d", value);
+	}
+}
+
+/**
+ * Show the data for a given node (and perhaps property) according to the
+ * display option provided.
+ *
+ * @param blob		FDT blob
+ * @param disp		Display information / options
+ * @param node		Node to display
+ * @param property	Name of property to display, or NULL if none
+ * @return 0 if ok, -ve on error
+ */
+static int show_data_for_item(const void *blob, struct display_info *disp,
+		int node, const char *property)
+{
+	const void *value = NULL;
+	int len, err = 0;
+
+	value = fdt_getprop(blob, node, property, &len);
+	if (value) {
+		show_data(disp, value, len);
+		printf("\n");
+	} else {
+		report_error(property, len);
+		err = -1;
+	}
+	return err;
+}
+
+/**
+ * Run the main fdtget operation, given a filename and valid arguments
+ *
+ * @param disp		Display information / options
+ * @param filename	Filename of blob file
+ * @param arg		List of arguments to process
+ * @param arg_count	Number of arguments
+ * @param return 0 if ok, -ve on error
+ */
+static int do_fdtget(struct display_info *disp, const char *filename,
+		     char **arg, int arg_count)
+{
+	char *blob;
+	int i, node;
+
+	blob = util_read_fdt(filename);
+	if (!blob)
+		return -1;
+
+	for (i = 0; i + 2 <= arg_count; i += 2) {
+		node = fdt_path_offset(blob, arg[0]);
+		if (node < 0) {
+			report_error(arg[0], node);
+			return -1;
+		}
+
+		if (show_data_for_item(blob, disp, node, arg[1]))
+			return -1;
+	}
+	return 0;
+}
+
+static const char *usage_msg =
+	"fdtget - read values from device tree\n"
+	"\n"
+	"Each value is printed on a new line.\n\n"
+	"Usage:\n"
+	"	fdtget <options> <dt file> [<node> <property>]...\n"
+	"Options:\n"
+	"\t-t [<type>][<format>]\tType of data\n"
+	"\t-h\t\tPrint this help\n\n"
+	"\t<type>\t\tOne character (s=string, i=int, u=unsigned, b=byte)\n"
+	"\t<format>\tOptional format character (x=hex)\n";
+
+static void usage(const char *msg)
+{
+	if (msg)
+		fprintf(stderr, "Error: %s\n\n", msg);
+
+	fprintf(stderr, "%s", usage_msg);
+	exit(2);
+}
+
+int main(int argc, char *argv[])
+{
+	char *filename = NULL;
+	struct display_info disp;
+
+	memset(&disp, '\0', sizeof(disp));
+	for (;;) {
+		int c = getopt(argc, argv, "ht:");
+		if (c == -1)
+			break;
+
+		switch (c) {
+		case 'h':
+		case '?':
+			usage("");
+
+		case 't':
+			if (utilfdt_decode_type(optarg, &disp.type,
+					&disp.format))
+				usage("Invalid type string");
+			break;
+		}
+	}
+
+	if (optind < argc)
+		filename = argv[optind++];
+	if (!filename)
+		usage("Missing filename");
+
+	argv += optind;
+	argc -= optind;
+
+	/* Allow no arguments, and silently succeed */
+	if (!argc)
+		return 0;
+
+	/* Check for node, property arguments */
+	if (argc % 2)
+		usage("Must have an even number of arguments");
+
+	if (do_fdtget(&disp, filename, argv, argc))
+		return 1;
+	return 0;
+}
-- 
1.7.3.1

Re: [PATCH v2 3/6] Add fdtget utility to read property values from device tree

[David Gibson]

On Wed, Sep 07, 2011 at 12:54:17PM -0700, Simon Glass wrote:
> This simply utility makes it easy for scripts to read values from the device
> tree. It is written in C and uses the same libfdt as the rest of the dtc
> package.
> 
> What is it for:
> - Reading fdt values from scripts
> - Extracting fdt information within build systems
> - Looking at particular values without having to dump the entire tree
> 
> To use it, specify the fdt binary file on command line followed by a list of
> node, property pairs. The utility then looks up each node, finds the property
> and displays the value.
> 
> Each value is printed on a new line.
> 
> fdtget tries to guess the type of each property based on its contents. This
> is not always reliable, so you can use the -t option to force fdtget to decode
> the value as a string, or byte, etc.
> 
> To read from stdin, use - as the file.
> 
> Usage:
> 	fdtget <options> <dt file> [<node> <property>]...
> Options:
> 	-t [<type>][<format>]	Type of data
> 	-h		Print this help
> 
> 	<type>		One character (s=string, i=int, u=unsigned, b=byte)
> 	<format>	Optional format character (x=hex)

It is not terribly clear how these are combined.

Is there any way to make these more closely resemble printf() style
format specifiers?

> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Changes in v2:
> - Separate arguments for node and property
> - Merge two related commits into this one
> - Use structure for storing display options
> - Remove use of getopt_long()
> 
>  .gitignore      |    1 +
>  Makefile        |    4 +
>  Makefile.fdtget |   12 +++
>  fdtget.c        |  200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 217 insertions(+), 0 deletions(-)
>  create mode 100644 Makefile.fdtget
>  create mode 100644 fdtget.c
> 
> diff --git a/.gitignore b/.gitignore
> index ae7a46a..9f27f34 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -10,3 +10,4 @@ lex.yy.c
>  /ftdump
>  /convert-dtsv0
>  /version_gen.h
> +/fdtget
> diff --git a/Makefile b/Makefile
> index 0baada5..3d460d7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -106,10 +106,12 @@ endef
>  include Makefile.convert-dtsv0
>  include Makefile.dtc
>  include Makefile.ftdump
> +include Makefile.fdtget
>  
>  BIN += convert-dtsv0
>  BIN += dtc
>  BIN += ftdump
> +BIN += fdtget
>  
>  SCRIPTS = dtdiff
>  
> @@ -120,6 +122,7 @@ ifneq ($(DEPTARGETS),)
>  -include $(DTC_OBJS:%.o=%.d)
>  -include $(CONVERT_OBJS:%.o=%.d)
>  -include $(FTDUMP_OBJS:%.o=%.d)
> +-include $(FDTGET_OBJS:%.o=%.d)
>  endif
>  
>  
> @@ -180,6 +183,7 @@ convert-dtsv0: $(CONVERT_OBJS)
>  
>  ftdump:	$(FTDUMP_OBJS) $(LIBFDT_archive)
>  
> +fdtget:	$(FDTGET_OBJS) $(LIBFDT_archive)
>  
>  #
>  # Testsuite rules
> diff --git a/Makefile.fdtget b/Makefile.fdtget
> new file mode 100644
> index 0000000..c1ab041
> --- /dev/null
> +++ b/Makefile.fdtget
> @@ -0,0 +1,12 @@
> +#
> +# This is not a complete Makefile.
> +# Instead, it is designed to be easily embeddable
> +# into other systems of Makefiles.
> +#
> +
> +FDTGET_SRCS = \
> +	fdtget.c \
> +	util.c \
> +	utilfdt.c
> +
> +FDTGET_OBJS = $(FDTGET_SRCS:%.c=%.o)
> diff --git a/fdtget.c b/fdtget.c
> new file mode 100644
> index 0000000..4a94e99
> --- /dev/null
> +++ b/fdtget.c
> @@ -0,0 +1,200 @@
> +/*
> + * 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"
> +
> +/* Holds information which controls our output and options */
> +struct display_info {
> +	int type;		/* data type (s/i/u/b or 0 for default) */
> +	int format;		/* data format (x or 0 for default) */
> +};
> +
> +static void report_error(const char *where, int err)
> +{
> +	fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
> +}
> +
> +/**
> + * Displays data of a given length according to selected options
> + *
> + * If a specific data type is provided in disp, then this is used. Otherwise
> + * we try to guess the data type / size from the contents.
> + *
> + * @param disp		Display information / options
> + * @param data		Data to display
> + * @param len		Maximum length of buffer
> + */
> +static void show_data(struct display_info *disp, 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 = (disp->type) == 's' ||
> +		(!disp->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("%s", (const char *)s);
> +		}
> +		return;
> +	}
> +	if (disp->type == 'i' || disp->type == 'u')
> +		size = 4;
> +	else if (disp->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(disp->format == 'x' ? "%x" : "%d", value);
> +	}
> +}
> +
> +/**
> + * Show the data for a given node (and perhaps property) according to the
> + * display option provided.

"and perhaps property"?  AFAICT this will SEGV if property is NULL.

> + * @param blob		FDT blob
> + * @param disp		Display information / options
> + * @param node		Node to display
> + * @param property	Name of property to display, or NULL if none
> + * @return 0 if ok, -ve on error
> + */
> +static int show_data_for_item(const void *blob, struct display_info *disp,
> +		int node, const char *property)
> +{
> +	const void *value = NULL;
> +	int len, err = 0;
> +
> +	value = fdt_getprop(blob, node, property, &len);
> +	if (value) {
> +		show_data(disp, value, len);
> +		printf("\n");
> +	} else {
> +		report_error(property, len);
> +		err = -1;
> +	}
> +	return err;
> +}
> +
> +/**
> + * Run the main fdtget operation, given a filename and valid arguments
> + *
> + * @param disp		Display information / options
> + * @param filename	Filename of blob file
> + * @param arg		List of arguments to process
> + * @param arg_count	Number of arguments
> + * @param return 0 if ok, -ve on error
> + */
> +static int do_fdtget(struct display_info *disp, const char *filename,
> +		     char **arg, int arg_count)
> +{
> +	char *blob;
> +	int i, node;
> +
> +	blob = util_read_fdt(filename);
> +	if (!blob)
> +		return -1;
> +
> +	for (i = 0; i + 2 <= arg_count; i += 2) {
> +		node = fdt_path_offset(blob, arg[0]);
> +		if (node < 0) {
> +			report_error(arg[0], node);
> +			return -1;
> +		}
> +
> +		if (show_data_for_item(blob, disp, node, arg[1]))
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +static const char *usage_msg =
> +	"fdtget - read values from device tree\n"
> +	"\n"
> +	"Each value is printed on a new line.\n\n"
> +	"Usage:\n"
> +	"	fdtget <options> <dt file> [<node> <property>]...\n"
> +	"Options:\n"
> +	"\t-t [<type>][<format>]\tType of data\n"
> +	"\t-h\t\tPrint this help\n\n"
> +	"\t<type>\t\tOne character (s=string, i=int, u=unsigned, b=byte)\n"
> +	"\t<format>\tOptional format character (x=hex)\n";
> +
> +static void usage(const char *msg)
> +{
> +	if (msg)
> +		fprintf(stderr, "Error: %s\n\n", msg);
> +
> +	fprintf(stderr, "%s", usage_msg);
> +	exit(2);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	char *filename = NULL;
> +	struct display_info disp;
> +
> +	memset(&disp, '\0', sizeof(disp));
> +	for (;;) {
> +		int c = getopt(argc, argv, "ht:");
> +		if (c == -1)
> +			break;
> +
> +		switch (c) {
> +		case 'h':
> +		case '?':
> +			usage("");
> +
> +		case 't':
> +			if (utilfdt_decode_type(optarg, &disp.type,
> +					&disp.format))
> +				usage("Invalid type string");
> +			break;
> +		}
> +	}
> +
> +	if (optind < argc)
> +		filename = argv[optind++];
> +	if (!filename)
> +		usage("Missing filename");
> +
> +	argv += optind;
> +	argc -= optind;
> +
> +	/* Allow no arguments, and silently succeed */
> +	if (!argc)
> +		return 0;
> +
> +	/* Check for node, property arguments */
> +	if (argc % 2)
> +		usage("Must have an even number of arguments");
> +
> +	if (do_fdtget(&disp, filename, argv, argc))
> +		return 1;
> +	return 0;
> +}

-- 
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 v2 3/6] Add fdtget utility to read property values from device tree

[Simon Glass]

Hi David,

On Wed, Sep 7, 2011 at 10:25 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Sep 07, 2011 at 12:54:17PM -0700, Simon Glass wrote:
>> This simply utility makes it easy for scripts to read values from the device
>> tree. It is written in C and uses the same libfdt as the rest of the dtc
>> package.
>>
>> What is it for:
>> - Reading fdt values from scripts
>> - Extracting fdt information within build systems
>> - Looking at particular values without having to dump the entire tree
>>
>> To use it, specify the fdt binary file on command line followed by a list of
>> node, property pairs. The utility then looks up each node, finds the property
>> and displays the value.
>>
>> Each value is printed on a new line.
>>
>> fdtget tries to guess the type of each property based on its contents. This
>> is not always reliable, so you can use the -t option to force fdtget to decode
>> the value as a string, or byte, etc.
>>
>> To read from stdin, use - as the file.
>>
>> Usage:
>>       fdtget <options> <dt file> [<node> <property>]...
>> Options:
>>       -t [<type>][<format>]   Type of data
>>       -h              Print this help
>>
>>       <type>          One character (s=string, i=int, u=unsigned, b=byte)
>>       <format>        Optional format character (x=hex)
>
> It is not terribly clear how these are combined.

A type character, then an optional format character. In fact the type
character is also optional at present since integer is assumed.

Please can you suggest a suitable help message to convey this?

>
> Is there any way to make these more closely resemble printf() style
> format specifiers?

The first patch series allowed printf specifiers but could segfault if
the user specified %s for something that was in fact not a string. I
have moved away from that as you suggested at the time. I am not keen
to re-implement printf(), or manually decode a partial subset of
printf strings, to avoid that problem. So something simple like this
is what I have come up with.

We need to encode the data size in there also - byte or integer - so
it is not quite the same as what printf is trying to do. While
printf() has %c I don't think it is a good idea to use that, since we
are printing a byte as a number, not an ASCII character.

But it is fairly close to printf, in that you can use:

-ts
-tx
-ti
-tu
-tix
-tbx

We could combine -tbx into a single letter somehow but I'm not sure I
like that idea.

I don't see any point in requiring a % since then users would expect
to be able to provide an arbitrary printf() string (see first patch
set).

So, suggestions please.

Regards,
Simon

>
>>
>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>> Changes in v2:
>> - Separate arguments for node and property
>> - Merge two related commits into this one
>> - Use structure for storing display options
>> - Remove use of getopt_long()
>>
>>  .gitignore      |    1 +
>>  Makefile        |    4 +
>>  Makefile.fdtget |   12 +++
>>  fdtget.c        |  200 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 217 insertions(+), 0 deletions(-)
>>  create mode 100644 Makefile.fdtget
>>  create mode 100644 fdtget.c
>>
[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 v2 3/6] Add fdtget utility to read property values from device tree

[David Gibson]

On Thu, Sep 08, 2011 at 05:47:34AM -0700, Simon Glass wrote:
> Hi David,

[snip]
> >> Usage:
> >>       fdtget <options> <dt file> [<node> <property>]...
> >> Options:
> >>       -t [<type>][<format>]   Type of data
> >>       -h              Print this help
> >>
> >>       <type>          One character (s=string, i=int, u=unsigned, b=byte)
> >>       <format>        Optional format character (x=hex)
> >
> > It is not terribly clear how these are combined.
> 
> A type character, then an optional format character. In fact the type
> character is also optional at present since integer is assumed.
> 
> Please can you suggest a suitable help message to convey this?

Hrm, ok, so.  I think part of the problem is that I'm finding what is
covered by "type" and what is covered by "format" is only obvious once
you've already understood what they mean.  They're not fully
independent either, since "format" makes no sense with type "s".

> > Is there any way to make these more closely resemble printf() style
> > format specifiers?
> 
> The first patch series allowed printf specifiers but could segfault if
> the user specified %s for something that was in fact not a string. I
> have moved away from that as you suggested at the time.

Yeah, I better understand your motivation for doing so now.  But it's
just not safe to do it that way.

> I am not keen
> to re-implement printf(), or manually decode a partial subset of
> printf strings, to avoid that problem. So something simple like this
> is what I have come up with.

Hrm.  The problem I have is that this sort-of-like printf() but not
really like printf() may violate least surprise.

> We need to encode the data size in there also - byte or integer - so
> it is not quite the same as what printf is trying to do. While
> printf() has %c I don't think it is a good idea to use that, since we
> are printing a byte as a number, not an ASCII character.

Printing a byte value as a number would be %hhx or %hhd in printf.

> But it is fairly close to printf, in that you can use:
> 
> -ts
> -tx
> -ti
> -tu
> -tix
> -tbx
> 
> We could combine -tbx into a single letter somehow but I'm not sure I
> like that idea.
> 
> I don't see any point in requiring a % since then users would expect
> to be able to provide an arbitrary printf() string (see first patch
> set).
> 
> So, suggestions please.

So, I think the first thing is I think you need to treat the "format"
as the primary parameter, with the size (roughly what you're calling
"type" at the moment) as a subordinate modifier to that.  The trouble
with making format subordinate to type, or supposedly independent
parameters is that what sizes or "types" are acceptable depends on the
format.  A string format always accepts variable length input, and the
integer formats always require a specified fixed length.  Future
extensions could conceivably have other constraints.

With that basic principle, I basically see two reasonable ways to go.

1) Use a subset of printf() specifiers.  Despite your balking at that,
I don't really think it would be any harder than what you have now.
Last character is always the non-optional "format" specifier, anything
preceding it is modifiers.  So for the integer formats "d", "x" and
"o", valid modifiers are size, either "hh" (1 byte), "h" (2 byte), "l"
(4 byte, default) or "ll" (8 byte).  For the string format "s", no
modifiers are valid.

2) Treat the size as a different parameter.  So the -f option gives a
single format char (or instead use -s, -x, -d, .. options).  Then have
a -s option for size (-s1 ... -s8) which is valid only with the
integer format options.


Either way does leave a couple of unanswered questions:

A) What to do if the format doesn't consume the whole parameter.  I
see two options:
	A1) Just print as much as the format says, then stop
	A2) Keep repeating the same format until the property is
consumed (note that this makes sense for "s" as well, and would be an
obvious way of handling the pretty common list-of-strings properties).

B) What to do when the property just doesn't fit into that format -
either it's length is not a multiple of the fixed integer size, or
it's not NUL-terminated for "s".  Obviously some kind of error is in
order, but in case A2 above, do we *just* error, or print what we can
before giving up.

-- 
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 v2 3/6] Add fdtget utility to read property values from device tree

[Simon Glass]

Hi David,

A bit rushed as late here but I wanted to respond before your weekend.

On Thu, Sep 8, 2011 at 9:49 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Sep 08, 2011 at 05:47:34AM -0700, Simon Glass wrote:
>> Hi David,
>
> [snip]
>> >> Usage:
>> >>       fdtget <options> <dt file> [<node> <property>]...
>> >> Options:
>> >>       -t [<type>][<format>]   Type of data
>> >>       -h              Print this help
>> >>
>> >>       <type>          One character (s=string, i=int, u=unsigned, b=byte)
>> >>       <format>        Optional format character (x=hex)
>> >
>> > It is not terribly clear how these are combined.
>>
>> A type character, then an optional format character. In fact the type
>> character is also optional at present since integer is assumed.
>>
>> Please can you suggest a suitable help message to convey this?
>
> Hrm, ok, so.  I think part of the problem is that I'm finding what is
> covered by "type" and what is covered by "format" is only obvious once
> you've already understood what they mean.  They're not fully
> independent either, since "format" makes no sense with type "s".

Yes I'm really just trying to explain a format similar to printf.

>
>> > Is there any way to make these more closely resemble printf() style
>> > format specifiers?
>>
>> The first patch series allowed printf specifiers but could segfault if
>> the user specified %s for something that was in fact not a string. I
>> have moved away from that as you suggested at the time.
>
> Yeah, I better understand your motivation for doing so now.  But it's
> just not safe to do it that way.
>
>> I am not keen
>> to re-implement printf(), or manually decode a partial subset of
>> printf strings, to avoid that problem. So something simple like this
>> is what I have come up with.
>
> Hrm.  The problem I have is that this sort-of-like printf() but not
> really like printf() may violate least surprise.

Well I mean that I don't want to reimplement it, but just using the
same syntax for a tiny subset would be fine.

>
>> We need to encode the data size in there also - byte or integer - so
>> it is not quite the same as what printf is trying to do. While
>> printf() has %c I don't think it is a good idea to use that, since we
>> are printing a byte as a number, not an ASCII character.
>
> Printing a byte value as a number would be %hhx or %hhd in printf.

Ick.

>
>> But it is fairly close to printf, in that you can use:
>>
>> -ts
>> -tx
>> -ti
>> -tu
>> -tix
>> -tbx
>>
>> We could combine -tbx into a single letter somehow but I'm not sure I
>> like that idea.
>>
>> I don't see any point in requiring a % since then users would expect
>> to be able to provide an arbitrary printf() string (see first patch
>> set).
>>
>> So, suggestions please.
>
> So, I think the first thing is I think you need to treat the "format"
> as the primary parameter, with the size (roughly what you're calling
> "type" at the moment) as a subordinate modifier to that.  The trouble
> with making format subordinate to type, or supposedly independent
> parameters is that what sizes or "types" are acceptable depends on the
> format.  A string format always accepts variable length input, and the
> integer formats always require a specified fixed length.  Future
> extensions could conceivably have other constraints.

OK sounds good.

>
> With that basic principle, I basically see two reasonable ways to go.
>
> 1) Use a subset of printf() specifiers.  Despite your balking at that,
> I don't really think it would be any harder than what you have now.
> Last character is always the non-optional "format" specifier, anything
> preceding it is modifiers.  So for the integer formats "d", "x" and
> "o", valid modifiers are size, either "hh" (1 byte), "h" (2 byte), "l"
> (4 byte, default) or "ll" (8 byte).  For the string format "s", no
> modifiers are valid.

OK, I like this option more. Particularly if you think I might get
away with also supporting 'b' as a shortcut for 'hh'.

>
> 2) Treat the size as a different parameter.  So the -f option gives a
> single format char (or instead use -s, -x, -d, .. options).  Then have
> a -s option for size (-s1 ... -s8) which is valid only with the
> integer format options.
>

More like what I had - have moved away from that and feel more
comfortable in my new space.

>
> Either way does leave a couple of unanswered questions:
>
> A) What to do if the format doesn't consume the whole parameter.  I
> see two options:
>        A1) Just print as much as the format says, then stop
>        A2) Keep repeating the same format until the property is
> consumed (note that this makes sense for "s" as well, and would be an
> obvious way of handling the pretty common list-of-strings properties).

Well I think you have to keep printing. This is not a true format
string, but just a data type for each element. Again we have moved
away from being able to say -f "The %d item is %d and has a flag word
of %d".

So no need to use -tddd if there are three cells - just -td is enough.

>
> B) What to do when the property just doesn't fit into that format -
> either it's length is not a multiple of the fixed integer size, or
> it's not NUL-terminated for "s".  Obviously some kind of error is in
> order, but in case A2 above, do we *just* error, or print what we can
> before giving up.

Prefer to error since any output might be confusing. But I will take a
look to make sure that is easy to do.

Thanks very much for your helpful comments. I will work on a new patch set.

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
>

Re: [PATCH v2 3/6] Add fdtget utility to read property values from device tree

[David Gibson]

On Thu, Sep 08, 2011 at 10:44:40PM -0700, Simon Glass wrote:
> Hi David,
> 
> A bit rushed as late here but I wanted to respond before your weekend.
> 
> On Thu, Sep 8, 2011 at 9:49 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Thu, Sep 08, 2011 at 05:47:34AM -0700, Simon Glass wrote:
[snip]
> > 1) Use a subset of printf() specifiers.  Despite your balking at that,
> > I don't really think it would be any harder than what you have now.
> > Last character is always the non-optional "format" specifier, anything
> > preceding it is modifiers.  So for the integer formats "d", "x" and
> > "o", valid modifiers are size, either "hh" (1 byte), "h" (2 byte), "l"
> > (4 byte, default) or "ll" (8 byte).  For the string format "s", no
> > modifiers are valid.
> 
> OK, I like this option more. Particularly if you think I might get
> away with also supporting 'b' as a shortcut for 'hh'.

Hrm, I have mixed feelings about that. "hh" is kind of obscure.  And
as far as I can tell "b" is not used for anything else at present (at
least in glibc printf()).  But there are so many letters with existing
meanings for printf() that redefining any makes me a little nervous.

> > 2) Treat the size as a different parameter.  So the -f option gives a
> > single format char (or instead use -s, -x, -d, .. options).  Then have
> > a -s option for size (-s1 ... -s8) which is valid only with the
> > integer format options.
> 
> More like what I had - have moved away from that and feel more
> comfortable in my new space.

Ok.

> > Either way does leave a couple of unanswered questions:
> >
> > A) What to do if the format doesn't consume the whole parameter.  I
> > see two options:
> >        A1) Just print as much as the format says, then stop
> >        A2) Keep repeating the same format until the property is
> > consumed (note that this makes sense for "s" as well, and would be an
> > obvious way of handling the pretty common list-of-strings properties).
> 
> Well I think you have to keep printing. This is not a true format
> string, but just a data type for each element. Again we have moved
> away from being able to say -f "The %d item is %d and has a flag word
> of %d".
> 
> So no need to use -tddd if there are three cells - just -td is enough.

Yes, I tend to agree.

> > B) What to do when the property just doesn't fit into that format -
> > either it's length is not a multiple of the fixed integer size, or
> > it's not NUL-terminated for "s".  Obviously some kind of error is in
> > order, but in case A2 above, do we *just* error, or print what we can
> > before giving up.
> 
> Prefer to error since any output might be confusing. But I will take a
> look to make sure that is easy to do.

It is trivial to check in advance for the existing types - for
integers just check if the property has length a multiple of the
integer size, for strings check if the last byte is '\0'.  Well.. then
it depends a bit on how safe you want "s" to be - do you check for
reasonably printable characters first or not.  How easy it is to
pre-check for possible future types is not neccessarily obvious of
course.

-- 
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 v2 3/6] Add fdtget utility to read property values from device tree

[Simon Glass]

Hi David,

Thanks for your comments.

On Sun, Sep 11, 2011 at 5:53 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Thu, Sep 08, 2011 at 10:44:40PM -0700, Simon Glass wrote:
>> Hi David,
>>
>> A bit rushed as late here but I wanted to respond before your weekend.
>>
>> On Thu, Sep 8, 2011 at 9:49 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Thu, Sep 08, 2011 at 05:47:34AM -0700, Simon Glass wrote:
> [snip]
>> > 1) Use a subset of printf() specifiers.  Despite your balking at that,
>> > I don't really think it would be any harder than what you have now.
>> > Last character is always the non-optional "format" specifier, anything
>> > preceding it is modifiers.  So for the integer formats "d", "x" and
>> > "o", valid modifiers are size, either "hh" (1 byte), "h" (2 byte), "l"
>> > (4 byte, default) or "ll" (8 byte).  For the string format "s", no
>> > modifiers are valid.
>>
>> OK, I like this option more. Particularly if you think I might get
>> away with also supporting 'b' as a shortcut for 'hh'.
>
> Hrm, I have mixed feelings about that. "hh" is kind of obscure.  And
> as far as I can tell "b" is not used for anything else at present (at
> least in glibc printf()).  But there are so many letters with existing
> meanings for printf() that redefining any makes me a little nervous.

Well yes - I think I will support both hh and b and see how it looks.

>
>> > 2) Treat the size as a different parameter.  So the -f option gives a
>> > single format char (or instead use -s, -x, -d, .. options).  Then have
>> > a -s option for size (-s1 ... -s8) which is valid only with the
>> > integer format options.
>>
>> More like what I had - have moved away from that and feel more
>> comfortable in my new space.
>
> Ok.
>
>> > Either way does leave a couple of unanswered questions:
>> >
>> > A) What to do if the format doesn't consume the whole parameter.  I
>> > see two options:
>> >        A1) Just print as much as the format says, then stop
>> >        A2) Keep repeating the same format until the property is
>> > consumed (note that this makes sense for "s" as well, and would be an
>> > obvious way of handling the pretty common list-of-strings properties).
>>
>> Well I think you have to keep printing. This is not a true format
>> string, but just a data type for each element. Again we have moved
>> away from being able to say -f "The %d item is %d and has a flag word
>> of %d".
>>
>> So no need to use -tddd if there are three cells - just -td is enough.
>
> Yes, I tend to agree.
>
>> > B) What to do when the property just doesn't fit into that format -
>> > either it's length is not a multiple of the fixed integer size, or
>> > it's not NUL-terminated for "s".  Obviously some kind of error is in
>> > order, but in case A2 above, do we *just* error, or print what we can
>> > before giving up.
>>
>> Prefer to error since any output might be confusing. But I will take a
>> look to make sure that is easy to do.
>
> It is trivial to check in advance for the existing types - for
> integers just check if the property has length a multiple of the
> integer size, for strings check if the last byte is '\0'.  Well.. then
> it depends a bit on how safe you want "s" to be - do you check for
> reasonably printable characters first or not.  How easy it is to
> pre-check for possible future types is not neccessarily obvious of
> course.

Yes I have added a check for (length % size) == 0. For strings I don't
think we should be picky so long as there is a NUL terminator. If you
don't specify a type, the utility already tries to guess, and will
fall back to integer or even byte if it finds the string has ctrl
characters. When the user says -ts I think we need to try hard to
obey. I can't think of any reason to store strange characters in a
string property, but others might.

I will send the V3 patch set in the next few days and we'll see how it looks.

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
>

Re: [PATCH v2 3/6] Add fdtget utility to read property values from device tree

[David Gibson]

On Sun, Sep 11, 2011 at 09:34:01PM -0700, Simon Glass wrote:
> Hi David,
> 
> Thanks for your comments.

[snip]
> >> > B) What to do when the property just doesn't fit into that format -
> >> > either it's length is not a multiple of the fixed integer size, or
> >> > it's not NUL-terminated for "s".  Obviously some kind of error is in
> >> > order, but in case A2 above, do we *just* error, or print what we can
> >> > before giving up.
> >>
> >> Prefer to error since any output might be confusing. But I will take a
> >> look to make sure that is easy to do.
> >
> > It is trivial to check in advance for the existing types - for
> > integers just check if the property has length a multiple of the
> > integer size, for strings check if the last byte is '\0'.  Well.. then
> > it depends a bit on how safe you want "s" to be - do you check for
> > reasonably printable characters first or not.  How easy it is to
> > pre-check for possible future types is not neccessarily obvious of
> > course.
> 
> Yes I have added a check for (length % size) == 0. For strings I don't
> think we should be picky so long as there is a NUL terminator. If you
> don't specify a type, the utility already tries to guess, and will
> fall back to integer or even byte if it finds the string has ctrl
> characters. When the user says -ts I think we need to try hard to
> obey. I can't think of any reason to store strange characters in a
> string property, but others might.

Yes, that makes sense.  One reason for funny bytes in a string that
springs to mind would be inclusion of UTF-8 or other high-bit
character encodings.

One useful extension of what you have now might be to add a "raw
binary" format.  Not much use for humans, but could be handy for
scripts extracting blobs from the device tree.  In fact, using the
same interpretation of existing printf() descriptors, that would
actually be equivalent to 'c'.

-- 
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 v2 3/6] Add fdtget utility to read property values from device tree

[Simon Glass]

Hi David,

On Fri, Sep 16, 2011 at 1:18 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sun, Sep 11, 2011 at 09:34:01PM -0700, Simon Glass wrote:
>> Hi David,
>>
>> Thanks for your comments.
>
> [snip]
>> >> > B) What to do when the property just doesn't fit into that format -
>> >> > either it's length is not a multiple of the fixed integer size, or
>> >> > it's not NUL-terminated for "s".  Obviously some kind of error is in
>> >> > order, but in case A2 above, do we *just* error, or print what we can
>> >> > before giving up.
>> >>
>> >> Prefer to error since any output might be confusing. But I will take a
>> >> look to make sure that is easy to do.
>> >
>> > It is trivial to check in advance for the existing types - for
>> > integers just check if the property has length a multiple of the
>> > integer size, for strings check if the last byte is '\0'.  Well.. then
>> > it depends a bit on how safe you want "s" to be - do you check for
>> > reasonably printable characters first or not.  How easy it is to
>> > pre-check for possible future types is not neccessarily obvious of
>> > course.
>>
>> Yes I have added a check for (length % size) == 0. For strings I don't
>> think we should be picky so long as there is a NUL terminator. If you
>> don't specify a type, the utility already tries to guess, and will
>> fall back to integer or even byte if it finds the string has ctrl
>> characters. When the user says -ts I think we need to try hard to
>> obey. I can't think of any reason to store strange characters in a
>> string property, but others might.
>
> Yes, that makes sense.  One reason for funny bytes in a string that
> springs to mind would be inclusion of UTF-8 or other high-bit
> character encodings.
>
> One useful extension of what you have now might be to add a "raw
> binary" format.  Not much use for humans, but could be handy for
> scripts extracting blobs from the device tree.  In fact, using the
> same interpretation of existing printf() descriptors, that would
> actually be equivalent to 'c'.

Yes I was thinking about how to do that as it might be useful. I think
'c' makes perfect sense. Will add to the todo.

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
>

Re: [PATCH v2 3/6] Add fdtget utility to read property values from device tree

[David Gibson]

On Fri, Sep 16, 2011 at 09:25:10AM -0700, Simon Glass wrote:
> Hi David,
[snip]
> > One useful extension of what you have now might be to add a "raw
> > binary" format.  Not much use for humans, but could be handy for
> > scripts extracting blobs from the device tree.  In fact, using the
> > same interpretation of existing printf() descriptors, that would
> > actually be equivalent to 'c'.
> 
> Yes I was thinking about how to do that as it might be useful. I think
> 'c' makes perfect sense. Will add to the todo.

Actually, on second thoughts using 'c' for this doesn't quite work.
For the other format specifiers, you will presumably have some sort of
separator between each repeat (newline, I'm guessing).  True binary
output, obviously, doesn't want that.

-- 
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 v2 3/6] Add fdtget utility to read property values from device tree

[Simon Glass]

Hi David,

On Sun, Sep 18, 2011 at 7:04 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Fri, Sep 16, 2011 at 09:25:10AM -0700, Simon Glass wrote:
>> Hi David,
> [snip]
>> > One useful extension of what you have now might be to add a "raw
>> > binary" format.  Not much use for humans, but could be handy for
>> > scripts extracting blobs from the device tree.  In fact, using the
>> > same interpretation of existing printf() descriptors, that would
>> > actually be equivalent to 'c'.
>>
>> Yes I was thinking about how to do that as it might be useful. I think
>> 'c' makes perfect sense. Will add to the todo.
>
> Actually, on second thoughts using 'c' for this doesn't quite work.
> For the other format specifiers, you will presumably have some sort of
> separator between each repeat (newline, I'm guessing).  True binary
> output, obviously, doesn't want that.

There is a space between each repeat and a newline after each property
printed. It makes sense to suppress the space for binary output and I
think an option to flick this switch either way might be in order, too
(even something to select a new separator char?). Something else for
the todo once this patch set is in perhaps.

I suppose we need to be a bit careful not to build out the pier too
far until we have some pedestrian traffic.

Did you see patch set 3?

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
>

Re: [PATCH v2 3/6] Add fdtget utility to read property values from device tree

[David Gibson]

On Sun, Sep 18, 2011 at 10:46:04PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Sun, Sep 18, 2011 at 7:04 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Sep 16, 2011 at 09:25:10AM -0700, Simon Glass wrote:
> >> Hi David,
> > [snip]
> >> > One useful extension of what you have now might be to add a "raw
> >> > binary" format.  Not much use for humans, but could be handy for
> >> > scripts extracting blobs from the device tree.  In fact, using the
> >> > same interpretation of existing printf() descriptors, that would
> >> > actually be equivalent to 'c'.
> >>
> >> Yes I was thinking about how to do that as it might be useful. I think
> >> 'c' makes perfect sense. Will add to the todo.
> >
> > Actually, on second thoughts using 'c' for this doesn't quite work.
> > For the other format specifiers, you will presumably have some sort of
> > separator between each repeat (newline, I'm guessing).  True binary
> > output, obviously, doesn't want that.
> 
> There is a space between each repeat and a newline after each property
> printed. It makes sense to suppress the space for binary output and I
> think an option to flick this switch either way might be in order, too
> (even something to select a new separator char?). Something else for
> the todo once this patch set is in perhaps.

Yeah, fair enough.  I don't think skipping the separators should be
explicit one way or another, not a magic side-effect of using a 'c'
type.

> I suppose we need to be a bit careful not to build out the pier too
> far until we have some pedestrian traffic.

A good point.

> Did you see patch set 3?

Uh, no I don't think so.

-- 
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 v2 3/6] Add fdtget utility to read property values from device tree

[Simon Glass]

Hi David,

On Mon, Sep 19, 2011 at 1:05 AM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sun, Sep 18, 2011 at 10:46:04PM -0700, Simon Glass wrote:
>> Hi David,
>>
>> On Sun, Sep 18, 2011 at 7:04 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Fri, Sep 16, 2011 at 09:25:10AM -0700, Simon Glass wrote:
>> >> Hi David,
>> > [snip]
>> >> > One useful extension of what you have now might be to add a "raw
>> >> > binary" format.  Not much use for humans, but could be handy for
>> >> > scripts extracting blobs from the device tree.  In fact, using the
>> >> > same interpretation of existing printf() descriptors, that would
>> >> > actually be equivalent to 'c'.
>> >>
>> >> Yes I was thinking about how to do that as it might be useful. I think
>> >> 'c' makes perfect sense. Will add to the todo.
>> >
>> > Actually, on second thoughts using 'c' for this doesn't quite work.
>> > For the other format specifiers, you will presumably have some sort of
>> > separator between each repeat (newline, I'm guessing).  True binary
>> > output, obviously, doesn't want that.
>>
>> There is a space between each repeat and a newline after each property
>> printed. It makes sense to suppress the space for binary output and I
>> think an option to flick this switch either way might be in order, too
>> (even something to select a new separator char?). Something else for
>> the todo once this patch set is in perhaps.
>
> Yeah, fair enough.  I don't think skipping the separators should be
> explicit one way or another, not a magic side-effect of using a 'c'
> type.

OK I think you are saying you want an explicit switch.

>
>> I suppose we need to be a bit careful not to build out the pier too
>> far until we have some pedestrian traffic.
>
> A good point.
>
>> Did you see patch set 3?
>
> Uh, no I don't think so.

OK I will resend to list.

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 v2 4/6] fdtget: Add basic tests

[Simon Glass]

This adds a few tests for common use cases.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Adjust tests for new fdtget arguments

 tests/fdtget-runtest.sh |   35 +++++++++++++++++++++++++++++++++++
 tests/run_tests.sh      |   36 +++++++++++++++++++++++++++++++++++-
 tests/tests.sh          |    1 +
 3 files changed, 71 insertions(+), 1 deletions(-)
 create mode 100755 tests/fdtget-runtest.sh

diff --git a/tests/fdtget-runtest.sh b/tests/fdtget-runtest.sh
new file mode 100755
index 0000000..f38184f
--- /dev/null
+++ b/tests/fdtget-runtest.sh
@@ -0,0 +1,35 @@
+#! /bin/sh
+
+. ./tests.sh
+
+LOG="tmp.log.$$"
+EXPECT="tmp.expect.$$"
+
+rm -f $TMPFILE $LOG
+
+expect="$1"
+echo "$expect" >$EXPECT
+shift
+
+verbose_run_log "$LOG" $VALGRIND "$DTGET" "$@"
+ret="$?"
+
+if [ "$ret" -ne 0 -a "$expect" = "ERR" ]; then
+	PASS
+fi
+
+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 72dda32..9acdeb5 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -83,6 +83,13 @@ asm_to_so_test () {
     run_wrap_test asm_to_so "$@"
 }
 
+run_fdtget_test () {
+    # run_fdtget_test name expected_output dtb_file args...
+    echo -n "$1:	"
+    shift
+    base_run_test sh fdtget-runtest.sh "$@"
+}
+
 tree1_tests () {
     TREE=$1
 
@@ -388,6 +395,30 @@ dtbs_equal_tests () {
     cmp_tests test_tree1.dtb $WRONG_TREE1
 }
 
+fdtget_tests () {
+    file=label01.dtb
+    $DTC -O dtb -o $file ${file%.dtb}.dts 2>/dev/null
+
+    # run_fdtget_test <test-name> <expected-result> <args>...
+    run_fdtget_test "Simple string" "MyBoardName" $file / model
+    run_fdtget_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_fdtget_test "Multiple string s" "MyBoardName MyBoardFamilyName" \
+	-t s $file / compatible
+    run_fdtget_test "Integer" "32768" $file /cpus/PowerPC,970@1 d-cache-size
+    run_fdtget_test "Integer hex" "8000" -tx $file \
+	/cpus/PowerPC,970@1 d-cache-size
+    run_fdtget_test "Integer list" "61 62 63 0" -tbx $file \
+	/randomnode tricky1
+    run_fdtget_test "Byte list short" "a b c d de ea ad be ef" -tbx \
+	$file /randomnode blob
+    run_fdtget_test "Integer list short" "a0b0c0d deeaadbe ef000000" -tx \
+	-t i $file /randomnode blob
+    run_fdtget_test "Missing property" ERR -tx \
+	$file /randomnode doctor-who
+}
+
 while getopts "vt:m" ARG ; do
     case $ARG in
 	"v")
@@ -403,7 +434,7 @@ while getopts "vt:m" ARG ; do
 done
 
 if [ -z "$TESTSETS" ]; then
-    TESTSETS="libfdt dtc dtbs_equal"
+    TESTSETS="libfdt dtc dtbs_equal fdtget"
 fi
 
 # Make sure we don't have stale blobs lying around
@@ -420,6 +451,9 @@ for set in $TESTSETS; do
 	"dtbs_equal")
 	    dtbs_equal_tests
 	    ;;
+	"fdtget")
+	    fdtget_tests
+	    ;;
     esac
 done
 
diff --git a/tests/tests.sh b/tests/tests.sh
index 30ffead..d9a0524 100644
--- a/tests/tests.sh
+++ b/tests/tests.sh
@@ -11,6 +11,7 @@ FAIL () {
 }
 
 DTC=../dtc
+DTGET=../fdtget
 
 verbose_run () {
     if [ -z "$QUIET_TEST" ]; then
-- 
1.7.3.1

Re: [PATCH v2 4/6] fdtget: Add basic tests

[David Gibson]

On Wed, Sep 07, 2011 at 12:54:18PM -0700, Simon Glass wrote:
> This adds a few tests for common use cases.

Fold this in with the patch adding fdtget itself.
-- 
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 v2 5/6] Add new fdtput utility to write values to fdt

[Simon Glass]

This simple utility allows writing of values into a device tree from the
command line. It aimes to be the opposite of fdtget.

What is it for:
- Updating fdt values when a binary blob already exists
   (even though source may be available it might be easier to use this
    utility rather than sed, etc.)
- Writing machine-specific fdt values within a build system

To use it, specify the fdt binary file on command line followed by the node
and property to set. Then, provide a list of values to put into that
property. Often there will be just one, but fdtput also supports arrays and
string lists.

fdtput does not trit to guess the type of the property based on looking at
the arguments. Instead it always assumed that an integer is provided. To
indicate that you want to write a string, use -ts. You can also provide
hex values with -tx.

The command line arguments are joined together into a single value. For
strings, a nul terminator is placed between each string. To avoid this, pass
the string as a single parameter.

Usage:
	fdtput <options> <dt file> <<node> <property> [<value>...]
Options:
	-t [<type>][<format>]	Type of data
	-v		Verbose: display each value decoded from command line
	-h		Print this help

	<type>		One character (s=string, i=int, u=unsigned, b=byte)
	<format>	Optional format character (x=hex)

To read from stdin and write to stdout, use - as the file. So you can do:

cat somefile.dtb | fdtput -ts - /node prop "My string value" > newfile.dtb

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Separate arguments for node and property
- Remove limits on data size of property writting to fdt
- Remove use of getopt_long()

 .gitignore      |    1 +
 Makefile        |    6 ++
 Makefile.fdtput |   12 +++
 fdtput.c        |  217 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 236 insertions(+), 0 deletions(-)
 create mode 100644 Makefile.fdtput
 create mode 100644 fdtput.c

diff --git a/.gitignore b/.gitignore
index 9f27f34..2c9a64e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -11,3 +11,4 @@ lex.yy.c
 /convert-dtsv0
 /version_gen.h
 /fdtget
+/fdtput
diff --git a/Makefile b/Makefile
index 3d460d7..c6b57d7 100644
--- a/Makefile
+++ b/Makefile
@@ -107,11 +107,13 @@ include Makefile.convert-dtsv0
 include Makefile.dtc
 include Makefile.ftdump
 include Makefile.fdtget
+include Makefile.fdtput
 
 BIN += convert-dtsv0
 BIN += dtc
 BIN += ftdump
 BIN += fdtget
+BIN += fdtput
 
 SCRIPTS = dtdiff
 
@@ -123,6 +125,7 @@ ifneq ($(DEPTARGETS),)
 -include $(CONVERT_OBJS:%.o=%.d)
 -include $(FTDUMP_OBJS:%.o=%.d)
 -include $(FDTGET_OBJS:%.o=%.d)
+-include $(FDTPUT_OBJS:%.o=%.d)
 endif
 
 
@@ -185,6 +188,9 @@ ftdump:	$(FTDUMP_OBJS) $(LIBFDT_archive)
 
 fdtget:	$(FDTGET_OBJS) $(LIBFDT_archive)
 
+fdtput:	$(FDTPUT_OBJS) $(LIBFDT_archive)
+
+
 #
 # Testsuite rules
 #
diff --git a/Makefile.fdtput b/Makefile.fdtput
new file mode 100644
index 0000000..de1cfdf
--- /dev/null
+++ b/Makefile.fdtput
@@ -0,0 +1,12 @@
+#
+# This is not a complete Makefile.
+# Instead, it is designed to be easily embeddable
+# into other systems of Makefiles.
+#
+
+FDTPUT_SRCS = \
+	fdtput.c \
+	util.c \
+	utilfdt.c
+
+FDTPUT_OBJS = $(FDTPUT_SRCS:%.c=%.o)
diff --git a/fdtput.c b/fdtput.c
new file mode 100644
index 0000000..fd03d76
--- /dev/null
+++ b/fdtput.c
@@ -0,0 +1,217 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+ * Distributed under the terms of the GNU General Public License v2
+ */
+
+#include <assert.h>
+#include <ctype.h>
+#include <getopt.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <libfdt.h>
+
+#include "util.h"
+#include "utilfdt.h"
+
+struct display_info {
+	int type;		/* data type (s/i/u/b or 0 for default) */
+	int format;		/* data format (x or 0 for default) */
+	int verbose;		/* verbose output */
+};
+
+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 disp		Display information / options
+ * @param arg		List of arguments from command line
+ * @param arg_count	Number of arguments (may be 0)
+ * @param valuep	Returns buffer containing value
+ * @param *value_len	Returns length of value encoded
+ */
+static int encode_value(struct display_info *disp, char **arg, int arg_count,
+			char **valuep, int *value_len)
+{
+	char *value = NULL;	/* holding area for value */
+	int value_size = 0;	/* size of holding area */
+	char *ptr;		/* pointer to current value position */
+	int len;		/* length of this cell/string/byte */
+	int ival;
+	int upto;	/* the number of bytes we have written to buf */
+
+	upto = 0;
+
+	if (disp->verbose)
+		fprintf(stderr, "Decoding value:\n");
+	for (; arg_count > 0; arg++, arg_count--, upto += len) {
+		/* assume integer unless told otherwise */
+		if (disp->type == 's')
+			len = strlen(*arg) + 1;
+		else
+			len = disp->type == 'b' ? 1 : 4;
+
+		/* enlarge our value buffer by a suitable margin if needed */
+		if (upto + len > value_size) {
+			value_size = (upto + len) + 500;
+			value = realloc(value, value_size);
+			if (!value) {
+				fprintf(stderr, "Out of mmory: cannot alloc "
+					"%d bytes\n", value_size);
+				return -1;
+			}
+		}
+
+		ptr = value + upto;
+		if (disp->type == 's') {
+			memcpy(ptr, *arg, len);
+			if (disp->verbose)
+				fprintf(stderr, "\tstring: '%s'\n", ptr);
+		} else {
+			int *iptr = (int *)ptr;
+
+			sscanf(*arg, disp->format == 'x' ? "%x" : "%d", &ival);
+			if (len == 4)
+				*iptr = cpu_to_fdt32(ival);
+			else
+				*ptr = (uint8_t)ival;
+			if (disp->verbose) {
+				fprintf(stderr, "\t%s: %d\n",
+					disp->type == 'b' ? "byte" : "int",
+					ival);
+			}
+		}
+	}
+	*value_len = upto;
+	*valuep = value;
+	if (disp->verbose)
+		fprintf(stderr, "Value size %d\n", upto);
+	return 0;
+}
+
+static int store_key_value(void *blob, const char *node_name,
+		const char *property, const char *buf, int len)
+{
+	int node;
+	int err;
+
+	node = fdt_path_offset(blob, node_name);
+	if (node < 0) {
+		report_error(node_name, node);
+		return -1;
+	}
+
+	err = fdt_setprop(blob, node, property, buf, len);
+	if (err) {
+		report_error(property, err);
+		return -1;
+	}
+	return 0;
+}
+
+static int do_fdtput(struct display_info *disp, const char *filename,
+		    char **arg, int arg_count)
+{
+	char *value;
+	char *blob;
+	int len, ret = 0;
+
+	blob = util_read_fdt(filename);
+	if (!blob)
+		return -1;
+
+	/* convert the arguments into a single binary value, then store */
+	assert(arg_count >= 2);
+	if (encode_value(disp, arg + 2, arg_count - 2, &value, &len) ||
+		store_key_value(blob, *arg, arg[1], value, len))
+		ret = -1;
+
+	if (!ret)
+		ret = util_write_fdt(blob, filename);
+
+	free(blob);
+	return ret;
+}
+
+static const char *usage_msg =
+	"fdtput - write a property value to a device tree\n"
+	"\n"
+	"The command line arguments are joined together into a single value.\n"
+	"\n"
+	"Usage:\n"
+	"	fdtput <options> <dt file> <<node> <property> [<value>...]\n"
+	"Options:\n"
+	"\t-t [<type>][<format>]\tType of data\n"
+	"\t-v\t\tVerbose: display each value decoded from command line\n"
+	"\t-h\t\tPrint this help\n\n"
+	"\t<type>\t\tOne character (s=string, i=int, u=unsigned, b=byte)\n"
+	"\t<format>\tOptional format character (x=hex)\n";
+
+static void usage(const char *msg)
+{
+	if (msg)
+		fprintf(stderr, "Error: %s\n\n", msg);
+
+	fprintf(stderr, "%s", usage_msg);
+	exit(2);
+}
+
+int main(int argc, char *argv[])
+{
+	struct display_info disp;
+	char *filename = NULL;
+
+	memset(&disp, '\0', sizeof(disp));
+	for (;;) {
+		int c = getopt(argc, argv, "ht:v");
+		if (c == -1)
+			break;
+
+		/*
+		 * TODO: add options to:
+		 * - delete property
+		 * - delete node (optionally recursively)
+		 * - rename node
+		 * - pack fdt before writing
+		 * - set amount of free space when writing
+		 * - expand fdt if value doesn't fit
+		 */
+		switch (c) {
+		case 'h':
+		case '?':
+			usage("");
+
+		case 't':
+			if (utilfdt_decode_type(optarg, &disp.type,
+					&disp.format))
+				usage("Invalid type string");
+			break;
+
+		case 'v':
+			disp.verbose = 1;
+			break;
+		}
+	}
+
+	if (optind < argc)
+		filename = argv[optind++];
+	if (!filename)
+		usage("Missing filename");
+
+	argv += optind;
+	argc -= optind;
+
+	if (argc < 1)
+		usage("Missing node");
+	if (argc < 2)
+		usage("Missing property");
+
+	if (do_fdtput(&disp, filename, argv, argc))
+		return 1;
+	return 0;
+}
-- 
1.7.3.1

Re: [PATCH v2 5/6] Add new fdtput utility to write values to fdt

[David Gibson]

On Wed, Sep 07, 2011 at 12:54:19PM -0700, Simon Glass wrote:
> This simple utility allows writing of values into a device tree from the
> command line. It aimes to be the opposite of fdtget.
> 
> What is it for:
> - Updating fdt values when a binary blob already exists
>    (even though source may be available it might be easier to use this
>     utility rather than sed, etc.)
> - Writing machine-specific fdt values within a build system
> 
> To use it, specify the fdt binary file on command line followed by the node
> and property to set. Then, provide a list of values to put into that
> property. Often there will be just one, but fdtput also supports arrays and
> string lists.
> 
> fdtput does not trit to guess the type of the property based on looking at
> the arguments. Instead it always assumed that an integer is provided. To
> indicate that you want to write a string, use -ts. You can also provide
> hex values with -tx.
> 
> The command line arguments are joined together into a single value. For
> strings, a nul terminator is placed between each string. To avoid this, pass
> the string as a single parameter.
> 
> Usage:
> 	fdtput <options> <dt file> <<node> <property> [<value>...]
> Options:
> 	-t [<type>][<format>]	Type of data
> 	-v		Verbose: display each value decoded from command line
> 	-h		Print this help
> 
> 	<type>		One character (s=string, i=int, u=unsigned, b=byte)
> 	<format>	Optional format character (x=hex)
> 
> To read from stdin and write to stdout, use - as the file. So you can do:
> 
> cat somefile.dtb | fdtput -ts - /node prop "My string value" > newfile.dtb
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> Changes in v2:
> - Separate arguments for node and property
> - Remove limits on data size of property writting to fdt
> - Remove use of getopt_long()
> 
>  .gitignore      |    1 +
>  Makefile        |    6 ++
>  Makefile.fdtput |   12 +++
>  fdtput.c        |  217 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 236 insertions(+), 0 deletions(-)
>  create mode 100644 Makefile.fdtput
>  create mode 100644 fdtput.c
> 
> diff --git a/.gitignore b/.gitignore
> index 9f27f34..2c9a64e 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -11,3 +11,4 @@ lex.yy.c
>  /convert-dtsv0
>  /version_gen.h
>  /fdtget
> +/fdtput
> diff --git a/Makefile b/Makefile
> index 3d460d7..c6b57d7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -107,11 +107,13 @@ include Makefile.convert-dtsv0
>  include Makefile.dtc
>  include Makefile.ftdump
>  include Makefile.fdtget
> +include Makefile.fdtput

Separate makefile fragments for the two utilities seems overkill.
Can't you just have one Makefile.utils or something.

>  
>  BIN += convert-dtsv0
>  BIN += dtc
>  BIN += ftdump
>  BIN += fdtget
> +BIN += fdtput
>  
>  SCRIPTS = dtdiff
>  
> @@ -123,6 +125,7 @@ ifneq ($(DEPTARGETS),)
>  -include $(CONVERT_OBJS:%.o=%.d)
>  -include $(FTDUMP_OBJS:%.o=%.d)
>  -include $(FDTGET_OBJS:%.o=%.d)
> +-include $(FDTPUT_OBJS:%.o=%.d)
>  endif
>  
>  
> @@ -185,6 +188,9 @@ ftdump:	$(FTDUMP_OBJS) $(LIBFDT_archive)
>  
>  fdtget:	$(FDTGET_OBJS) $(LIBFDT_archive)
>  
> +fdtput:	$(FDTPUT_OBJS) $(LIBFDT_archive)
> +
> +
>  #
>  # Testsuite rules
>  #
> diff --git a/Makefile.fdtput b/Makefile.fdtput
> new file mode 100644
> index 0000000..de1cfdf
> --- /dev/null
> +++ b/Makefile.fdtput
> @@ -0,0 +1,12 @@
> +#
> +# This is not a complete Makefile.
> +# Instead, it is designed to be easily embeddable
> +# into other systems of Makefiles.
> +#
> +
> +FDTPUT_SRCS = \
> +	fdtput.c \
> +	util.c \
> +	utilfdt.c
> +
> +FDTPUT_OBJS = $(FDTPUT_SRCS:%.c=%.o)
> diff --git a/fdtput.c b/fdtput.c
> new file mode 100644
> index 0000000..fd03d76
> --- /dev/null
> +++ b/fdtput.c
> @@ -0,0 +1,217 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
> + * Distributed under the terms of the GNU General Public License v2
> + */
> +
> +#include <assert.h>
> +#include <ctype.h>
> +#include <getopt.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <libfdt.h>
> +
> +#include "util.h"
> +#include "utilfdt.h"
> +
> +struct display_info {
> +	int type;		/* data type (s/i/u/b or 0 for default) */
> +	int format;		/* data format (x or 0 for default) */
> +	int verbose;		/* verbose output */
> +};
> +
> +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 disp		Display information / options
> + * @param arg		List of arguments from command line
> + * @param arg_count	Number of arguments (may be 0)
> + * @param valuep	Returns buffer containing value
> + * @param *value_len	Returns length of value encoded
> + */
> +static int encode_value(struct display_info *disp, char **arg, int arg_count,
> +			char **valuep, int *value_len)
> +{
> +	char *value = NULL;	/* holding area for value */
> +	int value_size = 0;	/* size of holding area */
> +	char *ptr;		/* pointer to current value position */
> +	int len;		/* length of this cell/string/byte */
> +	int ival;
> +	int upto;	/* the number of bytes we have written to buf */
> +
> +	upto = 0;
> +
> +	if (disp->verbose)
> +		fprintf(stderr, "Decoding value:\n");
> +	for (; arg_count > 0; arg++, arg_count--, upto += len) {
> +		/* assume integer unless told otherwise */
> +		if (disp->type == 's')
> +			len = strlen(*arg) + 1;
> +		else
> +			len = disp->type == 'b' ? 1 : 4;
> +
> +		/* enlarge our value buffer by a suitable margin if needed */
> +		if (upto + len > value_size) {
> +			value_size = (upto + len) + 500;
> +			value = realloc(value, value_size);
> +			if (!value) {
> +				fprintf(stderr, "Out of mmory: cannot alloc "
> +					"%d bytes\n", value_size);
> +				return -1;
> +			}
> +		}
> +
> +		ptr = value + upto;
> +		if (disp->type == 's') {
> +			memcpy(ptr, *arg, len);
> +			if (disp->verbose)
> +				fprintf(stderr, "\tstring: '%s'\n", ptr);
> +		} else {
> +			int *iptr = (int *)ptr;
> +
> +			sscanf(*arg, disp->format == 'x' ? "%x" : "%d", &ival);
> +			if (len == 4)
> +				*iptr = cpu_to_fdt32(ival);
> +			else
> +				*ptr = (uint8_t)ival;
> +			if (disp->verbose) {
> +				fprintf(stderr, "\t%s: %d\n",
> +					disp->type == 'b' ? "byte" : "int",
> +					ival);
> +			}
> +		}
> +	}
> +	*value_len = upto;
> +	*valuep = value;
> +	if (disp->verbose)
> +		fprintf(stderr, "Value size %d\n", upto);
> +	return 0;
> +}
> +
> +static int store_key_value(void *blob, const char *node_name,
> +		const char *property, const char *buf, int len)
> +{
> +	int node;
> +	int err;
> +
> +	node = fdt_path_offset(blob, node_name);
> +	if (node < 0) {
> +		report_error(node_name, node);
> +		return -1;
> +	}
> +
> +	err = fdt_setprop(blob, node, property, buf, len);
> +	if (err) {
> +		report_error(property, err);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static int do_fdtput(struct display_info *disp, const char *filename,
> +		    char **arg, int arg_count)
> +{
> +	char *value;
> +	char *blob;
> +	int len, ret = 0;
> +
> +	blob = util_read_fdt(filename);
> +	if (!blob)
> +		return -1;
> +
> +	/* convert the arguments into a single binary value, then store */
> +	assert(arg_count >= 2);
> +	if (encode_value(disp, arg + 2, arg_count - 2, &value, &len) ||
> +		store_key_value(blob, *arg, arg[1], value, len))
> +		ret = -1;
> +
> +	if (!ret)
> +		ret = util_write_fdt(blob, filename);
> +
> +	free(blob);
> +	return ret;
> +}
> +
> +static const char *usage_msg =
> +	"fdtput - write a property value to a device tree\n"
> +	"\n"
> +	"The command line arguments are joined together into a single value.\n"
> +	"\n"
> +	"Usage:\n"
> +	"	fdtput <options> <dt file> <<node> <property> [<value>...]\n"
> +	"Options:\n"
> +	"\t-t [<type>][<format>]\tType of data\n"
> +	"\t-v\t\tVerbose: display each value decoded from command line\n"
> +	"\t-h\t\tPrint this help\n\n"
> +	"\t<type>\t\tOne character (s=string, i=int, u=unsigned, b=byte)\n"
> +	"\t<format>\tOptional format character (x=hex)\n";
> +
> +static void usage(const char *msg)
> +{
> +	if (msg)
> +		fprintf(stderr, "Error: %s\n\n", msg);
> +
> +	fprintf(stderr, "%s", usage_msg);
> +	exit(2);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	struct display_info disp;
> +	char *filename = NULL;
> +
> +	memset(&disp, '\0', sizeof(disp));
> +	for (;;) {
> +		int c = getopt(argc, argv, "ht:v");
> +		if (c == -1)
> +			break;
> +
> +		/*
> +		 * TODO: add options to:
> +		 * - delete property
> +		 * - delete node (optionally recursively)
> +		 * - rename node
> +		 * - pack fdt before writing
> +		 * - set amount of free space when writing
> +		 * - expand fdt if value doesn't fit
> +		 */
> +		switch (c) {
> +		case 'h':
> +		case '?':
> +			usage("");
> +
> +		case 't':
> +			if (utilfdt_decode_type(optarg, &disp.type,
> +					&disp.format))
> +				usage("Invalid type string");
> +			break;
> +
> +		case 'v':
> +			disp.verbose = 1;
> +			break;
> +		}
> +	}
> +
> +	if (optind < argc)
> +		filename = argv[optind++];
> +	if (!filename)
> +		usage("Missing filename");
> +
> +	argv += optind;
> +	argc -= optind;
> +
> +	if (argc < 1)
> +		usage("Missing node");
> +	if (argc < 2)
> +		usage("Missing property");
> +
> +	if (do_fdtput(&disp, filename, argv, argc))
> +		return 1;
> +	return 0;
> +}

-- 
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 v2 5/6] Add new fdtput utility to write values to fdt

[Simon Glass]

Hi David,

On Wed, Sep 7, 2011 at 10:32 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Sep 07, 2011 at 12:54:19PM -0700, Simon Glass wrote:
>> This simple utility allows writing of values into a device tree from the
>> command line. It aimes to be the opposite of fdtget.
>>
[snip]
>>  include Makefile.dtc
>>  include Makefile.ftdump
>>  include Makefile.fdtget
>> +include Makefile.fdtput
>
> Separate makefile fragments for the two utilities seems overkill.
> Can't you just have one Makefile.utils or something.

Yes of course - I was just following the existing code patterns.
Should I roll ftdump into there as well?

[snip]

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
>

Re: [PATCH v2 5/6] Add new fdtput utility to write values to fdt

[David Gibson]

On Thu, Sep 08, 2011 at 05:51:28AM -0700, Simon Glass wrote:
> Hi David,
> 
> On Wed, Sep 7, 2011 at 10:32 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Wed, Sep 07, 2011 at 12:54:19PM -0700, Simon Glass wrote:
> >> This simple utility allows writing of values into a device tree from the
> >> command line. It aimes to be the opposite of fdtget.
> >>
> [snip]
> >>  include Makefile.dtc
> >>  include Makefile.ftdump
> >>  include Makefile.fdtget
> >> +include Makefile.fdtput
> >
> > Separate makefile fragments for the two utilities seems overkill.
> > Can't you just have one Makefile.utils or something.
> 
> Yes of course - I was just following the existing code patterns.
> Should I roll ftdump into there as well?

Yes, might as well.

-- 
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 v2 6/6] fdtput: Add basic tests

[Simon Glass]

These tests verify the major features.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Adjust tests for new fdtput arguments
- Add test for multiple strings
- Add test for exhausting fdt space

 tests/fdtput-runtest.sh |   55 +++++++++++++++++++++++++++++++++++
 tests/run_tests.sh      |   73 +++++++++++++++++++++++++++++++++++++++++++++-
 tests/tests.sh          |    1 +
 3 files changed, 127 insertions(+), 2 deletions(-)
 create mode 100644 tests/fdtput-runtest.sh

diff --git a/tests/fdtput-runtest.sh b/tests/fdtput-runtest.sh
new file mode 100644
index 0000000..ea51569
--- /dev/null
+++ b/tests/fdtput-runtest.sh
@@ -0,0 +1,55 @@
+#! /bin/sh
+
+# Run script for fdtput tests
+# We run fdtput to update the device tree, thn fdtget to check it
+
+# Usage
+#    fdtput-runtest.sh name expected_output dtb_file node property flags value
+
+. ./tests.sh
+
+LOG="tmp.log.$$"
+EXPECT="tmp.expect.$$"
+
+rm -f $TMPFILE $LOG
+
+expect="$1"
+echo "$expect" >$EXPECT
+dtb="$2"
+node="$3"
+property="$4"
+flags="$5"
+shift 5
+value="$@"
+
+# First run fdtput
+verbose_run $VALGRIND "$DTPUT" "$dtb" "$node" "$property" $value $flags
+ret="$?"
+
+if [ "$ret" -ne 0 -a "$expect" = "ERR" ]; then
+	PASS
+fi
+if [ "$ret" -gt 127 ]; then
+    signame=$(kill -l $[ret - 128])
+    FAIL "Killed by SIG$signame"
+fi
+
+# Now fdtget to read the value
+verbose_run_log "$LOG" $VALGRIND "$DTGET" "$dtb" "$node" "$property" $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 9acdeb5..cd03d61 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -90,6 +90,21 @@ run_fdtget_test () {
     base_run_test sh fdtget-runtest.sh "$@"
 }
 
+run_fdtput_test () {
+    # run_fdtput_test name expected_output dtb_file node property flags value...
+    echo -n "$1:	"
+    shift
+    output="$1"
+    dtb="$2"
+    node="$3"
+    property="$4"
+    flags="$5"
+    shift 5
+    base_run_test sh fdtput-runtest.sh "$output" "$dtb" "$node" "$property" \
+		"$flags" $@
+#     base_run_test sh fdtput-runtest.sh "$@"
+}
+
 tree1_tests () {
     TREE=$1
 
@@ -415,10 +430,61 @@ fdtget_tests () {
 	$file /randomnode blob
     run_fdtget_test "Integer list short" "a0b0c0d deeaadbe ef000000" -tx \
 	-t i $file /randomnode blob
-    run_fdtget_test "Missing property" ERR -tx \
+    run_fdtget_test "Missing property" ERR -ts \
 	$file /randomnode doctor-who
 }
 
+fdtput_tests () {
+    file=label01.dtb
+    src=label01.dts
+
+    # Create some test files containing useful strings
+    base=tmp.test0
+    file1=tmp.test1
+    file2=tmp.test2
+    bigfile1=tmp.test3
+    bigfile2=tmp.test4
+
+    # Filter out anything the shell might not like
+    cat $src | tr -d "'\"\n\;/\.\*{}\-" | tr -s "[:blank:]" " " >$base
+
+    # Make two small files
+    head -5 $base >$file1
+    cat $file1 | tr a-z A-Z | cut -c10-30 | sort -r >$file2
+
+    # and two larger ones
+    cat $base > $bigfile1
+    tac $base | tr a-z A-Z | sort -r >$bigfile2
+
+    # Allow just enough space for both file1 and file2
+    (( space = $(stat -c %s $file1) + $(stat -c %s $file2) ))
+    $DTC -O dtb -p $space -o $file ${file%.dtb}.dts 2>/dev/null
+
+    # run_fdtput_test <test-name> <expected-result> <file> <key> <flags>
+    #		<args>...
+    run_fdtput_test "Simple string" "a_model" $file / model -ts "a_model"
+    run_fdtput_test "Multiple string s" "board1 board2" \
+	$file / compatible -ts board1 board2
+    run_fdtput_test "Single string with spaces" "board1 board2" \
+	$file / compatible -ts "board1 board2"
+    run_fdtput_test "Integer" "32768" \
+	$file /cpus/PowerPC,970@1 d-cache-size "" "32768"
+    run_fdtput_test "Integer hex" "8001" \
+	$file /cpus/PowerPC,970@1 d-cache-size -tx 0x8001
+    run_fdtput_test "Integer list" "2 3 4" \
+	$file /randomnode tricky1 -tb "02 03 04"
+    run_fdtput_test "Byte list short" "a b c ea ad be ef" \
+	$file /randomnode blob -tbx "a b c ea ad be ef"
+    run_fdtput_test "Integer list short" "a0b0c0d deeaae ef000000" \
+	$file /randomnode blob -tx "a0b0c0d deeaae ef000000"
+    run_fdtput_test "Large string list" "`cat $file1 $file2`" \
+	$file /randomnode blob -ts "`cat $file1`" "`cat $file2`"
+
+    # This should be larger than available space in the fdt ($space)
+    run_fdtput_test "Enormous string list" ERR \
+	$file /randomnode blob -ts "`cat $bigfile1`" "`cat $bigfile2`"
+}
+
 while getopts "vt:m" ARG ; do
     case $ARG in
 	"v")
@@ -434,7 +500,7 @@ while getopts "vt:m" ARG ; do
 done
 
 if [ -z "$TESTSETS" ]; then
-    TESTSETS="libfdt dtc dtbs_equal fdtget"
+    TESTSETS="libfdt dtc dtbs_equal fdtget fdtput"
 fi
 
 # Make sure we don't have stale blobs lying around
@@ -454,6 +520,9 @@ for set in $TESTSETS; do
 	"fdtget")
 	    fdtget_tests
 	    ;;
+	"fdtput")
+	    fdtput_tests
+	    ;;
     esac
 done
 
diff --git a/tests/tests.sh b/tests/tests.sh
index d9a0524..6e5e76a 100644
--- a/tests/tests.sh
+++ b/tests/tests.sh
@@ -12,6 +12,7 @@ FAIL () {
 
 DTC=../dtc
 DTGET=../fdtget
+DTPUT=../fdtput
 
 verbose_run () {
     if [ -z "$QUIET_TEST" ]; then
-- 
1.7.3.1

Re: [PATCH v2 6/6] fdtput: Add basic tests

[David Gibson]

On Wed, Sep 07, 2011 at 12:54:20PM -0700, Simon Glass wrote:
> These tests verify the major features.

Again, fold it into the previous patch.  No point having code and
tests for the code separate.

-- 
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 v2 6/6] fdtput: Add basic tests

[Simon Glass]

Hi David,

On Wed, Sep 7, 2011 at 10:32 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Wed, Sep 07, 2011 at 12:54:20PM -0700, Simon Glass wrote:
>> These tests verify the major features.
>
> Again, fold it into the previous patch.  No point having code and
> tests for the code separate.

OK will do. I generally split things up as much as I reasonably to
make it easy to deal with reviews, which can otherwise get out of
hand.

I will submit a new patch set once I have confirmation from you on
what you want done with:

- the -t format string
- the blob read function
- bringing ftdump into Makefile.utils

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
>