[PATCH v2] dtc: Add support for named integer constants

[PATCH v2] dtc: Add support for named integer constants

[Stephen Warren]

You may define constants as follows:

/define/ TWO 2;
/define/ FOUR 4;
/define/ OTHER FOUR;

And properties may use these values as follows:

foo = <1 TWO 3 FOUR 5>;

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: Now against upstream dtc, now doesn't use $ as a prefix to variable,
names, added a test for the new functionality.

I tested this using the dtc test-suite, and against the kernel's Tegra
device-tree files, after addition of the pinmux initialization table,
and conversion of that to using identifiers.

 dtc-lexer.l           |   13 +++++++++
 dtc-parser.y          |   73 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.tests  |    3 +-
 tests/identifiers.c   |   44 +++++++++++++++++++++++++++++
 tests/identifiers.dts |   12 ++++++++
 tests/run_tests.sh    |    4 +++
 6 files changed, 148 insertions(+), 1 deletions(-)
 create mode 100644 tests/identifiers.c
 create mode 100644 tests/identifiers.dts

diff --git a/dtc-lexer.l b/dtc-lexer.l
index e866ea5..77492d0 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -28,6 +28,7 @@
 PROPNODECHAR	[a-zA-Z0-9,._+*#?@-]
 PATHCHAR	({PROPNODECHAR}|[/])
 LABEL		[a-zA-Z_][a-zA-Z0-9_]*
+IDENTIFIER	[a-zA-Z_][a-zA-Z0-9_]*
 STRING		\"([^\\"]|\\.)*\"
 WS		[[:space:]]
 COMMENT		"/*"([^*]|\*+[^*/])*\*+"/"
@@ -96,6 +97,12 @@ static int pop_input_file(void);
 			return DT_MEMRESERVE;
 		}
 
+<*>"/define/"	{
+			DPRINT("Keyword: /define/\n");
+			BEGIN_DEFAULT();
+			return DT_DEFINE;
+		}
+
 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);
@@ -103,6 +110,12 @@ static int pop_input_file(void);
 			return DT_LABEL;
 		}
 
+<V1>{IDENTIFIER} {
+			DPRINT("identifier: %s\n", yytext);
+			yylval.identifier = xstrdup(yytext + 1);
+			return DT_IDENTIFIER;
+		}
+
 <V1>[0-9]+|0[xX][0-9a-fA-F]+      {
 			yylval.literal = xstrdup(yytext);
 			DPRINT("Literal: '%s'\n", yylval.literal);
diff --git a/dtc-parser.y b/dtc-parser.y
index 5e84a67..0558c38 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -33,6 +33,9 @@ extern void yyerror(char const *s);
 extern struct boot_info *the_boot_info;
 extern int treesource_error;
 
+static struct identifier *get_identifier(const char *s);
+static void set_identifier(const char *name, unsigned long long value);
+static unsigned long long eval_identifier(const char *name);
 static unsigned long long eval_literal(const char *s, int base, int bits);
 %}
 
@@ -40,6 +43,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
 	char *propnodename;
 	char *literal;
 	char *labelref;
+	char *identifier;
 	unsigned int cbase;
 	uint8_t byte;
 	struct data data;
@@ -55,7 +59,9 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
 
 %token DT_V1
 %token DT_MEMRESERVE
+%token DT_DEFINE
 %token <propnodename> DT_PROPNODENAME
+%token <identifier> DT_IDENTIFIER
 %token <literal> DT_LITERAL
 %token <cbase> DT_BASE
 %token <byte> DT_BYTE
@@ -113,6 +119,13 @@ memreserve:
 		}
 	;
 
+define:
+	  DT_DEFINE DT_IDENTIFIER cellval ';'
+		{
+			set_identifier($2, $3);
+		}
+	;
+
 addr:
 	  DT_LITERAL
 		{
@@ -125,6 +138,10 @@ devicetree:
 		{
 			$$ = name_node($2, "");
 		}
+	| define
+		{
+			$$ = name_node(build_node(NULL, NULL), "");
+		}
 	| devicetree '/' nodedef
 		{
 			$$ = merge_nodes($1, $3);
@@ -139,6 +156,10 @@ devicetree:
 				print_error("label or path, '%s', not found", $2);
 			$$ = $1;
 		}
+	| devicetree define
+		{
+			$$ = $1;
+		}
 	;
 
 nodedef:
@@ -265,6 +286,10 @@ cellval:
 		{
 			$$ = eval_literal($1, 0, 32);
 		}
+	| DT_IDENTIFIER
+		{
+			$$ = eval_identifier($1);
+		}
 	;
 
 bytestring:
@@ -327,6 +352,54 @@ void yyerror(char const *s) {
 	print_error("%s", s);
 }
 
+struct identifier {
+	const char *name;
+	unsigned long long value;
+	struct identifier *next;
+};
+static struct identifier *identifiers;
+
+static struct identifier *get_identifier(const char *name)
+{
+	struct identifier *identifier = identifiers;
+
+	while (identifier != NULL) {
+		if (streq(name, identifier->name))
+			return identifier;
+		identifier = identifier->next;
+	}
+
+	return NULL;
+}
+
+static void set_identifier(const char *name, unsigned long long value)
+{
+	struct identifier *identifier;
+
+	if (get_identifier(name) != NULL) {
+		print_error("redefining %s", name);
+		return;
+	}
+
+	identifier = xmalloc(sizeof(*identifier));
+	identifier->name = name;
+	identifier->value = value;
+	identifier->next = identifiers;
+	identifiers = identifier;
+}
+
+unsigned long long eval_identifier(const char *name)
+{
+	struct identifier *identifier = get_identifier(name);
+
+	if (identifier == NULL) {
+		print_error("identifier %s does not exist", name);
+		return 0;
+	}
+
+	return identifier->value;
+}
+
 static unsigned long long eval_literal(const char *s, int base, int bits)
 {
 	unsigned long long val;
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index c564e72..25d2e3c 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -15,7 +15,8 @@ LIB_TESTS_L = get_mem_rsv \
 	extra-terminating-null \
 	dtbs_equal_ordered \
 	dtb_reverse dtbs_equal_unordered \
-	add_subnode_with_nops path_offset_aliases
+	add_subnode_with_nops path_offset_aliases \
+	identifiers
 LIB_TESTS = $(LIB_TESTS_L:%=$(TESTS_PREFIX)%)
 
 LIBTREE_TESTS_L = truncated_property
diff --git a/tests/identifiers.c b/tests/identifiers.c
new file mode 100644
index 0000000..a013d00
--- /dev/null
+++ b/tests/identifiers.c
@@ -0,0 +1,44 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for /define/
+ * Copyright (C) 2011 NVIDIA, Inc.
+ * Derived from code:
+ * Copyright (C) 2006 David Gibson, IBM Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <fdt.h>
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+
+	test_init(argc, argv);
+	fdt = load_blob_arg(argc, argv);
+
+	check_property_cell(fdt, 0, "var1", TEST_VALUE_1);
+	check_property_cell(fdt, 0, "var2", TEST_VALUE_1);
+
+	PASS();
+}
diff --git a/tests/identifiers.dts b/tests/identifiers.dts
new file mode 100644
index 0000000..e8d8e23
--- /dev/null
+++ b/tests/identifiers.dts
@@ -0,0 +1,12 @@
+/dts-v1/;
+
+/define/ VAR1 0xdeadbeef;
+/define/ VAR2 VAR1;
+
+/ {
+	var1 = <VAR1>;
+	var2 = <VAR2>;
+};
+
+/define/ OTHER 0xdeadbeef;
+
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 72dda32..e0da087 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -343,6 +343,10 @@ dtc_tests () {
     run_dtc_test -I dtb -O dts -o stdin_odts_test_tree1.dtb.test.dts - < test_tree1.dtb
     run_wrap_test cmp stdin_odts_test_tree1.dtb.test.dts odts_test_tree1.dtb.test.dts
 
+    # Test identifiers
+    run_dtc_test -I dts -O dtb -o identifiers.dtb identifiers.dts
+    run_test identifiers identifiers.dtb
+
     # Check for graceful failure in some error conditions
     run_sh_test dtc-fatal.sh -I dts -O dtb nosuchfile.dts
     run_sh_test dtc-fatal.sh -I dtb -O dtb nosuchfile.dtb
-- 
1.7.0.4

RE: [PATCH v2] dtc: Add support for named integer constants

[Stephen Warren]

Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
> You may define constants as follows:
> 
> /define/ TWO 2;
> /define/ FOUR 4;
> /define/ OTHER FOUR;
> 
> And properties may use these values as follows:
> 
> foo = <1 TWO 3 FOUR 5>;
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

David, Jon,

Does this seem reasonable?

I think the syntax is simple enough it wouldn't interfere with any more
advanced expression/function/... support in the future, and it could be
easily extended to allow e.g.:

/define/ FOO "BAR";
/define/ BAX [0af8dacb0];
...

And even arbitrary expressions on the RHS as/when/if support is added
for those more generally:

/define/ A (B | (C << 2));

-- 
nvpublic

Re: [PATCH v2] dtc: Add support for named integer constants

[Simon Glass]

Hi Stephen,

On Fri, Sep 2, 2011 at 11:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
>> You may define constants as follows:
>>
>> /define/ TWO 2;
>> /define/ FOUR 4;
>> /define/ OTHER FOUR;
>>
>> And properties may use these values as follows:
>>
>> foo = <1 TWO 3 FOUR 5>;
>>
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> David, Jon,
>
> Does this seem reasonable?
>
> I think the syntax is simple enough it wouldn't interfere with any more
> advanced expression/function/... support in the future, and it could be
> easily extended to allow e.g.:
>
> /define/ FOO "BAR";
> /define/ BAX [0af8dacb0];
> ...

This seems very reasonable to me, and very useful. From the syntax it
looks like lower case symbols are allowed also, which is fine with me.

I hope that this can go into dtc as we would definitely use it.

Regards,
Simon

>
> And even arbitrary expressions on the RHS as/when/if support is added
> for those more generally:
>
> /define/ A (B | (C << 2));
>
> --
> nvpublic
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>

Re: [PATCH v2] dtc: Add support for named integer constants

[Simon Glass]

Hi Stephen,

On Thu, Sep 8, 2011 at 6:09 AM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Stephen,
>
> On Fri, Sep 2, 2011 at 11:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
>>> You may define constants as follows:
>>>
>>> /define/ TWO 2;
>>> /define/ FOUR 4;
>>> /define/ OTHER FOUR;
>>>
>>> And properties may use these values as follows:
>>>
>>> foo = <1 TWO 3 FOUR 5>;
>>>
>>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> David, Jon,
>>
>> Does this seem reasonable?
>>
>> I think the syntax is simple enough it wouldn't interfere with any more
>> advanced expression/function/... support in the future, and it could be
>> easily extended to allow e.g.:
>>
>> /define/ FOO "BAR";
>> /define/ BAX [0af8dacb0];
>> ...
>
> This seems very reasonable to me, and very useful. From the syntax it
> looks like lower case symbols are allowed also, which is fine with me.
>
> I hope that this can go into dtc as we would definitely use it.
>
> Regards,
> Simon
>
>>
>> And even arbitrary expressions on the RHS as/when/if support is added
>> for those more generally:
>>
>> /define/ A (B | (C << 2));

[in a separate email since I don't want to pollute my comments on
Stephen's nice patch with separate ravings]

Thinking of bitfields, we already have a problem with the way GPIOs
are specified. Typically we do something like:

something = <&gpio 46 5>

and define the third parameter to be three bits (direction, initial
value, polarity). It would be nice to be able to do something like:

something = <&gpio 46 INPUT,ACTIVE_LOW>
something-else = <&gpio 46 OUTPUT,HIGH>

instead. That would avoid having to explicitly have expressions
containing | or define all combinations like this:

/define/ INPUT 0
/define/ OUTPUT_LOW 2
/define/ OUTPUT_HIGH 3
/define/ INPUT_INVERT 4
/define/ OUTPUT_LOW_INVERT 6
/define/ OUTPUT_HIGH_INVERT 7

which seems more confusing to me. Some hardware has options for
pull-ups also, which I haven't yet addressed in code.

So how about an easy way to combine bit masks into a single integer?

Regards,
Simon

>>
>> --
>> nvpublic
>>
>> _______________________________________________
>> devicetree-discuss mailing list
>> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>> https://lists.ozlabs.org/listinfo/devicetree-discuss
>>
>

Re: [PATCH v2] dtc: Add support for named integer constants

[Grant Likely]

On Thu, Sep 08, 2011 at 06:09:27AM -0700, Simon Glass wrote:
> Hi Stephen,
> 
> On Fri, Sep 2, 2011 at 11:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
> >> You may define constants as follows:
> >>
> >> /define/ TWO 2;
> >> /define/ FOUR 4;
> >> /define/ OTHER FOUR;
> >>
> >> And properties may use these values as follows:
> >>
> >> foo = <1 TWO 3 FOUR 5>;
> >>
> >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > David, Jon,
> >
> > Does this seem reasonable?
> >
> > I think the syntax is simple enough it wouldn't interfere with any more
> > advanced expression/function/... support in the future, and it could be
> > easily extended to allow e.g.:
> >
> > /define/ FOO "BAR";
> > /define/ BAX [0af8dacb0];
> > ...
> 
> This seems very reasonable to me, and very useful. From the syntax it
> looks like lower case symbols are allowed also, which is fine with me.
> 
> I hope that this can go into dtc as we would definitely use it.

What are the risks of symbol conflict with this approach?  I'm
concerned that a poorly chosen /define/ name will break parsing in
non-obvious ways.  Would it be better to have a every define reference
to be explicit in the syntax?

g.

Re: [PATCH v2] dtc: Add support for named integer constants

[Simon Glass]

Hi Grant,

On Thu, Sep 8, 2011 at 11:32 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Thu, Sep 08, 2011 at 06:09:27AM -0700, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Fri, Sep 2, 2011 at 11:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
>> >> You may define constants as follows:
>> >>
>> >> /define/ TWO 2;
>> >> /define/ FOUR 4;
>> >> /define/ OTHER FOUR;
>> >>
>> >> And properties may use these values as follows:
>> >>
>> >> foo = <1 TWO 3 FOUR 5>;
>> >>
>> >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> >
>> > David, Jon,
>> >
>> > Does this seem reasonable?
>> >
>> > I think the syntax is simple enough it wouldn't interfere with any more
>> > advanced expression/function/... support in the future, and it could be
>> > easily extended to allow e.g.:
>> >
>> > /define/ FOO "BAR";
>> > /define/ BAX [0af8dacb0];
>> > ...
>>
>> This seems very reasonable to me, and very useful. From the syntax it
>> looks like lower case symbols are allowed also, which is fine with me.
>>
>> I hope that this can go into dtc as we would definitely use it.
>
> What are the risks of symbol conflict with this approach?  I'm
> concerned that a poorly chosen /define/ name will break parsing in
> non-obvious ways.  Would it be better to have a every define reference
> to be explicit in the syntax?

Can you give an example of symbol conflict?

I vote against explicit syntax...things like $var are pretty ugly I
think. We will end up with Perl. Let's make these defines first class
citizens.

It should be clear enough since these will appear within a value -
i.e. I assume we will do:

/define/ ONE 1
...
/ {
   testing = <ONE TWO THREE>;
}

but not:

/define/ which testing
/ {
   which = <1 2 3>;
}

which does seem a little ambiguous (although not unresolveable by the
parser with the help of the symbol table).

Regards,
Simon

>
> g.
>
>

Re: [PATCH v2] dtc: Add support for named integer constants

[David Gibson]

On Thu, Sep 08, 2011 at 11:32:11AM -0700, Grant Likely wrote:
> On Thu, Sep 08, 2011 at 06:09:27AM -0700, Simon Glass wrote:
> > Hi Stephen,
> > 
> > On Fri, Sep 2, 2011 at 11:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > > Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
> > >> You may define constants as follows:
> > >>
> > >> /define/ TWO 2;
> > >> /define/ FOUR 4;
> > >> /define/ OTHER FOUR;
> > >>
> > >> And properties may use these values as follows:
> > >>
> > >> foo = <1 TWO 3 FOUR 5>;
> > >>
> > >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >
> > > David, Jon,
> > >
> > > Does this seem reasonable?
> > >
> > > I think the syntax is simple enough it wouldn't interfere with any more
> > > advanced expression/function/... support in the future, and it could be
> > > easily extended to allow e.g.:
> > >
> > > /define/ FOO "BAR";
> > > /define/ BAX [0af8dacb0];
> > > ...
> > 
> > This seems very reasonable to me, and very useful. From the syntax it
> > looks like lower case symbols are allowed also, which is fine with me.
> > 
> > I hope that this can go into dtc as we would definitely use it.
> 
> What are the risks of symbol conflict with this approach?  I'm
> concerned that a poorly chosen /define/ name will break parsing in
> non-obvious ways.  Would it be better to have a every define reference
> to be explicit in the syntax?

I really don't want to make identifiers - which is essentially what
we're talking here - explicitly marked, on the basis of "be like C".
I believe they should be safe, as long as we don't attempt to
recognize them in property/nodename context.

-- 
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] dtc: Add support for named integer constants

[David Gibson]

On Thu, Sep 08, 2011 at 06:18:21AM -0700, Simon Glass wrote:
> Hi Stephen,
> 
> On Thu, Sep 8, 2011 at 6:09 AM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> > Hi Stephen,
> >
> > On Fri, Sep 2, 2011 at 11:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> >> Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
> >>> You may define constants as follows:
> >>>
> >>> /define/ TWO 2;
> >>> /define/ FOUR 4;
> >>> /define/ OTHER FOUR;
> >>>
> >>> And properties may use these values as follows:
> >>>
> >>> foo = <1 TWO 3 FOUR 5>;
> >>>
> >>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>
> >> David, Jon,
> >>
> >> Does this seem reasonable?
> >>
> >> I think the syntax is simple enough it wouldn't interfere with any more
> >> advanced expression/function/... support in the future, and it could be
> >> easily extended to allow e.g.:
> >>
> >> /define/ FOO "BAR";
> >> /define/ BAX [0af8dacb0];
> >> ...
> >
> > This seems very reasonable to me, and very useful. From the syntax it
> > looks like lower case symbols are allowed also, which is fine with me.
> >
> > I hope that this can go into dtc as we would definitely use it.
> >
> > Regards,
> > Simon
> >
> >>
> >> And even arbitrary expressions on the RHS as/when/if support is added
> >> for those more generally:
> >>
> >> /define/ A (B | (C << 2));
> 
> [in a separate email since I don't want to pollute my comments on
> Stephen's nice patch with separate ravings]
> 
> Thinking of bitfields, we already have a problem with the way GPIOs
> are specified. Typically we do something like:
> 
> something = <&gpio 46 5>
> 
> and define the third parameter to be three bits (direction, initial
> value, polarity). It would be nice to be able to do something like:
> 
> something = <&gpio 46 INPUT,ACTIVE_LOW>
> something-else = <&gpio 46 OUTPUT,HIGH>
> 
> instead. That would avoid having to explicitly have expressions
> containing | or define all combinations like this:

I really don't like the idea of adding an extra explicit syntax for
bitfields.  I much prefer just implementing integer expressions, which
would allow the same thing through |.

-- 
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] dtc: Add support for named integer constants

[Simon Glass]

Hi David,

On Thu, Sep 8, 2011 at 6:35 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
[snip]
>> [in a separate email since I don't want to pollute my comments on
>> Stephen's nice patch with separate ravings]
>>
>> Thinking of bitfields, we already have a problem with the way GPIOs
>> are specified. Typically we do something like:
>>
>> something = <&gpio 46 5>
>>
>> and define the third parameter to be three bits (direction, initial
>> value, polarity). It would be nice to be able to do something like:
>>
>> something = <&gpio 46 INPUT,ACTIVE_LOW>
>> something-else = <&gpio 46 OUTPUT,HIGH>
>>
>> instead. That would avoid having to explicitly have expressions
>> containing | or define all combinations like this:
>
> I really don't like the idea of adding an extra explicit syntax for
> bitfields.  I much prefer just implementing integer expressions, which
> would allow the same thing through |.

Well that's fine as well. In fact much better since that's on my wish
list too. It will be interesting to try out these new toys and see how
if affects some of the problems we have had with fdt on our platforms.

My justification for explicit bitfield support is that they are
essentially flags which could just occupy a whole cell each, but for
space/efficiency reasons we decide they should be packed together into
a single cell. So the syntax between these two options should perhaps
be similar. With integer expressions, this works, since we convert
<&gpio 147 1 2 4> to <<&gpio 147 1 | 2 | 4> if you see what I mean. It
is the <&gpio 147 7> that I don't like. With Stephen's /define/ patch
we should be able to do things like <&gpio 147 INPUT | ACTIVE_LOW>.

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] dtc: Add support for named integer constants

[Stephen Warren]

David Gibson wrote at Thursday, September 08, 2011 7:35 PM:
> On Thu, Sep 08, 2011 at 11:32:11AM -0700, Grant Likely wrote:
> > On Thu, Sep 08, 2011 at 06:09:27AM -0700, Simon Glass wrote:
> > > Hi Stephen,
> > >
> > > On Fri, Sep 2, 2011 at 11:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > > > Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
> > > >> You may define constants as follows:
> > > >>
> > > >> /define/ TWO 2;
> > > >> /define/ FOUR 4;
> > > >> /define/ OTHER FOUR;
> > > >>
> > > >> And properties may use these values as follows:
> > > >>
> > > >> foo = <1 TWO 3 FOUR 5>;
> > > >>
> > > >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
...
> >
> > What are the risks of symbol conflict with this approach?  I'm
> > concerned that a poorly chosen /define/ name will break parsing in
> > non-obvious ways.  Would it be better to have a every define reference
> > to be explicit in the syntax?
> 
> I really don't want to make identifiers - which is essentially what
> we're talking here - explicitly marked, on the basis of "be like C".
> I believe they should be safe, as long as we don't attempt to
> recognize them in property/nodename context.

Grant,

As far as I understand the parser code, the define names aren't accepted
where they'd cause conflicts with node names etc. I just tested using
node names that matched names set up with /define/, and didn't see it
cause any unexpected syntax errors.

Jon, David, 

Does the change look good to go in?

-- 
nvpublic

Re: [PATCH v2] dtc: Add support for named integer constants

[David Gibson]

On Mon, Sep 19, 2011 at 03:13:55PM -0700, Stephen Warren wrote:
> David Gibson wrote at Thursday, September 08, 2011 7:35 PM:
> > On Thu, Sep 08, 2011 at 11:32:11AM -0700, Grant Likely wrote:
> > > On Thu, Sep 08, 2011 at 06:09:27AM -0700, Simon Glass wrote:
> > > > Hi Stephen,
> > > >
> > > > On Fri, Sep 2, 2011 at 11:34 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > > > > Stephen Warren wrote at Tuesday, August 30, 2011 3:30 PM:
> > > > >> You may define constants as follows:
> > > > >>
> > > > >> /define/ TWO 2;
> > > > >> /define/ FOUR 4;
> > > > >> /define/ OTHER FOUR;
> > > > >>
> > > > >> And properties may use these values as follows:
> > > > >>
> > > > >> foo = <1 TWO 3 FOUR 5>;
> > > > >>
> > > > >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ...
> > >
> > > What are the risks of symbol conflict with this approach?  I'm
> > > concerned that a poorly chosen /define/ name will break parsing in
> > > non-obvious ways.  Would it be better to have a every define reference
> > > to be explicit in the syntax?
> > 
> > I really don't want to make identifiers - which is essentially what
> > we're talking here - explicitly marked, on the basis of "be like C".
> > I believe they should be safe, as long as we don't attempt to
> > recognize them in property/nodename context.
> 
> Grant,
> 
> As far as I understand the parser code, the define names aren't accepted
> where they'd cause conflicts with node names etc. I just tested using
> node names that matched names set up with /define/, and didn't see it
> cause any unexpected syntax errors.
> 
> Jon, David, 
> 
> Does the change look good to go in?

Um, so.  I meant to discuss this earlier, but I've had many things to
do.

The complexity here is that as you've noted, this kind of all ties in
with more extensive expression / macro / function support as was toyed
with a while back in Jon's test branch.

As you've shown, simple constants are quite straightforward to
implement, and some of my experimental patches show that implementing
constant expressions evaluated during parse is also quite
straightforward.

Things get trickier when we want to extend this to macros or
functions.  The only problem I have with your patch at present is that
I'd prefer not to implement a constant defining syntax, only to have
it obsoleted by whatever we do for macros or functions.


So, there are basically two approaches to macro or function support.

A) Functions

We allow definition of functions with parameters.  These are stored
in some kind of parse tree representation in a symbol table.  Instead
of producing a more-or-less fully realised device tree as we parse, we
produce some kind of parse tree showing the expressions and function
calls.  After parsing is complete we make another pass evaluating all
the expressions and functions.

Advantages:
 * Allows repeats / iteration to be done simply.
 * Potentially allows superior type and other error reporting
 * Jon already has a prototype in the test branch

Disadvantages:
 * Requires a substantial runtime evaluation infrastructure to be
   implemented, including representation and storage of function
   definitions.
 * I don't like Jon's proposed syntax in the test branch because
   although it's C like, it's very much against the current
   declarative feel of dts.  That is, it feels somewhat like a program
   that's executed to produce the device tree rather than a
   representation of the device tree itself

B) Macros

Before parsing proper, we preprocess the source to expand macros
(which may expand to constant expressions).  Then we parse, evaluating
expressions as we go.

Advantages:
 * Lots of flexibility with a conceptually simple model; once we get
   to the parser we need only consider the declarative structure of
   the tree, not runtime evaluation.
 * Matches existing C prepropcessor usage.

Disadvantages:
 * Iteration is awkward.  Either special iteration built in functions
   or awkward macro constructs (THINGX1, THINGX2, THINGX4...) are
   required.

There are two specific suboptions of note here:

  B1) Use cpp itself

  Advantages:
   * Saves a lot of work implementing the preprocessor
   * I already have a prototype patch
   * cpp is already being used by some people, they wouldn't have to
     change anything

  Disadvantages:
   * The # used for preprocessor directives clashes with common
     property names beginning with #.  In practice this can be
     disambiguated by assuming only # in column 0 is for cpp, but the
     options to make cpp behave this way are not necessarily portable.
   
  B2) Make our own preprocessor, isomorphic to cpp

  Advantages:
   * We can change the directive format to avoid the # problem
   * We can potentially extend with new syntax if we want it
  Disadvantages:
   * Quite a lot of work in re-implementing cpp.
   * Not clear if this can sanely be done during lexing, or if it will
     need an explicit extra pass requiring awkward temporary storage
     of the preprocessed source.

Our current impasse is roughly that Jon prefers approach (A), whereas
I prefer (B1) on balance.  (B1) would obsolete your suggested define
syntax.  (A) and (B2) could both potentially subsume it, instead.

-- 
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] dtc: Add support for named integer constants

[Jon Loeliger]

> 
> Things get trickier when we want to extend this to macros or
> functions.  The only problem I have with your patch at present is that
> I'd prefer not to implement a constant defining syntax, only to have
> it obsoleted by whatever we do for macros or functions.

Exactly.

> So, there are basically two approaches to macro or function support.
> 
> A) Functions
> B) Macros
>   B1) Use cpp itself
>   B2) Make our own preprocessor, isomorphic to cpp

To be thorough, there has been one other macro proposal: Use m4.
Suggesting that, however, has had the entertaining side effect
of causing internet-wide vomiting.


> Our current impasse is roughly that Jon prefers approach (A), whereas
> I prefer (B1) on balance.  (B1) would obsolete your suggested define
> syntax.  (A) and (B2) could both potentially subsume it, instead.

Right.

jdl

RE: [PATCH v2] dtc: Add support for named integer constants

[Stephen Warren]

Jon Loeliger wrote at Tuesday, September 20, 2011 7:48 AM:
> David Gibson wrote:
> > Things get trickier when we want to extend this to macros or
> > functions.  The only problem I have with your patch at present is that
> > I'd prefer not to implement a constant defining syntax, only to have
> > it obsoleted by whatever we do for macros or functions.
> 
> Exactly.
> 
> > So, there are basically two approaches to macro or function support.
> >
> > A) Functions
> > B) Macros
> >   B1) Use cpp itself
> >   B2) Make our own preprocessor, isomorphic to cpp
> 
> To be thorough, there has been one other macro proposal: Use m4.
> Suggesting that, however, has had the entertaining side effect
> of causing internet-wide vomiting.
> 
> > Our current impasse is roughly that Jon prefers approach (A), whereas
> > I prefer (B1) on balance.  (B1) would obsolete your suggested define
> > syntax.  (A) and (B2) could both potentially subsume it, instead.
> 
> Right.

OK, that all makes sense.

However, it leaves me wondering what the next steps are; I was hoping to
get a quick and simple constant syntax into dtc that I could use for the
pinmux initialization patches I was working on, so the data tables there
would be integer-based for efficiency, yet named using constant names
instead of seemingly random numbers. It sounds like the dtc issue isn't
going to be resolved particularly quickly; should I just go back to using
strings in my pinmux patches? But that'd tie us to using strings forever
in order for the bindings to remain compatible...

-- 
nvpublic

Re: [PATCH v2] dtc: Add support for named integer constants

[Scott Wood]

On 09/20/2011 03:04 AM, David Gibson wrote:
> So, there are basically two approaches to macro or function support.
> 
> A) Functions
> 
> We allow definition of functions with parameters.  These are stored
> in some kind of parse tree representation in a symbol table.  Instead
> of producing a more-or-less fully realised device tree as we parse, we
> produce some kind of parse tree showing the expressions and function
> calls.  After parsing is complete we make another pass evaluating all
> the expressions and functions.
> 
> Advantages:
>  * Allows repeats / iteration to be done simply.
>  * Potentially allows superior type and other error reporting
>  * Jon already has a prototype in the test branch
> 
> Disadvantages:
>  * Requires a substantial runtime evaluation infrastructure to be
>    implemented, including representation and storage of function
>    definitions.
>  * I don't like Jon's proposed syntax in the test branch because
>    although it's C like, it's very much against the current
>    declarative feel of dts.  That is, it feels somewhat like a program
>    that's executed to produce the device tree rather than a
>    representation of the device tree itself

You could say the same thing about macros.  This is just doing it at a
higher semantic level.

>   Disadvantages:
>    * The # used for preprocessor directives clashes with common
>      property names beginning with #.  In practice this can be
>      disambiguated by assuming only # in column 0 is for cpp, but the
>      options to make cpp behave this way are not necessarily portable.

There are other disadvantages, namely that cpp, while familiar, is not a
very good macro language:
 - No recursive macro expansion ("self-referential" in cpp's terms) --
   useful for iteration in the absence of explicit support
 - No conditionals inside the macro body -- needed to make recursive
   macros work, but also useful on their own
 - Awkward handling of multi-line macro bodies -- backslashes
   everywhere, and makes it difficult/awkward to fix the previous
   issues with incremental extensions of the macro language
 - Does not integrate well with the surrounding language's
   indentation/formatting, especially if the directives need to be in
   column 0

If we're going to use an existing macro language, something more like
the GNU assembler's macros would be nicer.

Another disadvantage of any approach that tries to separate macros from
the underlying language is that you can't have anything be conditional
on an expression that the macro layer doesn't understand.

-Scott

Re: [PATCH v2] dtc: Add support for named integer constants

[David Gibson]

On Tue, Sep 20, 2011 at 12:35:29PM -0500, Scott Wood wrote:
> On 09/20/2011 03:04 AM, David Gibson wrote:
> > So, there are basically two approaches to macro or function support.
> > 
> > A) Functions
> > 
> > We allow definition of functions with parameters.  These are stored
> > in some kind of parse tree representation in a symbol table.  Instead
> > of producing a more-or-less fully realised device tree as we parse, we
> > produce some kind of parse tree showing the expressions and function
> > calls.  After parsing is complete we make another pass evaluating all
> > the expressions and functions.
> > 
> > Advantages:
> >  * Allows repeats / iteration to be done simply.
> >  * Potentially allows superior type and other error reporting
> >  * Jon already has a prototype in the test branch
> > 
> > Disadvantages:
> >  * Requires a substantial runtime evaluation infrastructure to be
> >    implemented, including representation and storage of function
> >    definitions.
> >  * I don't like Jon's proposed syntax in the test branch because
> >    although it's C like, it's very much against the current
> >    declarative feel of dts.  That is, it feels somewhat like a program
> >    that's executed to produce the device tree rather than a
> >    representation of the device tree itself
> 
> You could say the same thing about macros.  This is just doing it at a
> higher semantic level.

I don't see it that way, but I recognize that that's a matter of
judgement on which reasonable people may disagree :).

> >   Disadvantages:
> >    * The # used for preprocessor directives clashes with common
> >      property names beginning with #.  In practice this can be
> >      disambiguated by assuming only # in column 0 is for cpp, but the
> >      options to make cpp behave this way are not necessarily portable.
> 
> There are other disadvantages, namely that cpp, while familiar, is not a
> very good macro language:
>  - No recursive macro expansion ("self-referential" in cpp's terms) --
>    useful for iteration in the absence of explicit support

Right, this something I mean when I say no iteration support.

>  - No conditionals inside the macro body -- needed to make recursive
>    macros work, but also useful on their own
>  - Awkward handling of multi-line macro bodies -- backslashes
>    everywhere, and makes it difficult/awkward to fix the previous
>    issues with incremental extensions of the macro language

Hm, true.

>  - Does not integrate well with the surrounding language's
>    indentation/formatting, especially if the directives need to be in
>    column 0
> 
> If we're going to use an existing macro language, something more like
> the GNU assembler's macros would be nicer.
> 
> Another disadvantage of any approach that tries to separate macros from
> the underlying language is that you can't have anything be conditional
> on an expression that the macro layer doesn't understand.

That one doesn't follow, actually.  The macro can't implement a
conditional itself, but it could expand to a (constant) conditional
expression which is evaluated later.

-- 
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] dtc: Add support for named integer constants

[Scott Wood]

On 09/27/2011 07:29 PM, David Gibson wrote:
> On Tue, Sep 20, 2011 at 12:35:29PM -0500, Scott Wood wrote:
>> Another disadvantage of any approach that tries to separate macros from
>> the underlying language is that you can't have anything be conditional
>> on an expression that the macro layer doesn't understand.
> 
> That one doesn't follow, actually.  The macro can't implement a
> conditional itself, but it could expand to a (constant) conditional
> expression which is evaluated later.

Right, you can always implement conditionals in the underlying language,
but the macro processor can't directly use the results.  This can be an
issue if the intended use is to be heavily reliant on macros as a
substitute for expressiveness in the underlying language.

-Scott