[PATCH] Implements new features for updating existing device tree nodes.

[PATCH] Implements new features for updating existing device tree nodes.

[John Bonesio]

This is interesting when the /include/ "<filename>" feature is used. This way
we can create base device tree source files for a family of systems and modify
the device tree for a specific system.

The current sytem allows an existing node to be extended with new properties
and subnodes.

The new features allow an existing node to be replaced completely by the new
properties and subnodes. The new features also allow an existing node to be
deleted.

This patch does not yet provide a way to remove properties.

- John
---

 dtc-lexer.l  |   12 ++++++++++
 dtc-parser.y |   24 ++++++++++++++++++++
 dtc.h        |    2 ++
 livetree.c   |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+), 0 deletions(-)

diff --git a/dtc-lexer.l b/dtc-lexer.l
index 081e13a..a24edf5 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -96,6 +96,18 @@ static int pop_input_file(void);
 			return DT_MEMRESERVE;
 		}
 
+<*>"/trim-node/"	{
+			DPRINT("Keyword: /trim-node/\n");
+			BEGIN_DEFAULT();
+			return DT_REPLACENODE;
+		}
+
+<*>"/remove-node/"	{
+			DPRINT("Keyword: /remove-node/\n");
+			BEGIN_DEFAULT();
+			return DT_REMOVENODE;
+		}
+
 <*>{LABEL}:	{
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);
diff --git a/dtc-parser.y b/dtc-parser.y
index aa250f1..a9cde72 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -54,6 +54,8 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
 
 %token DT_V1
 %token DT_MEMRESERVE
+%token DT_REPLACENODE
+%token DT_REMOVENODE
 %token <propnodename> DT_PROPNODENAME
 %token <literal> DT_LITERAL
 %token <cbase> DT_BASE
@@ -140,6 +142,28 @@ devicetree:
 					" node extension", $2);
 			$$ = $1;
 		}
+	| devicetree DT_REPLACENODE DT_REF nodedef
+		{
+			struct node *target;
+
+			target = get_node_by_label($1, $3);
+			if (target)
+				replace_nodes(target, $4);
+			else
+				yyerror("label, '%s' does not exist in"
+					" node redefinition", $3);
+			$$ = $1;
+		}
+	| devicetree DT_REMOVENODE DT_REF ';'
+		{
+			struct node *target;
+
+			target = get_node_by_label($1, $3);
+			if (target)
+				remove_nodes(target);
+			else
+				yyerror("label, '%s' does not exist in"
+					" node removal", $3);
 	;
 
 nodedef:
diff --git a/dtc.h b/dtc.h
index b36ac5d..1340568 100644
--- a/dtc.h
+++ b/dtc.h
@@ -175,6 +175,8 @@ struct node *build_node(struct property *proplist, struct node *children);
 struct node *name_node(struct node *node, char *name);
 struct node *chain_node(struct node *first, struct node *list);
 struct node *merge_nodes(struct node *old_node, struct node *new_node);
+struct node *replace_nodes(struct node *old_node, struct node *new_node);
+struct node *remove_nodes(struct node *node);
 
 void add_property(struct node *node, struct property *prop);
 void add_child(struct node *parent, struct node *child);
diff --git a/livetree.c b/livetree.c
index 13c5f10..5e32510 100644
--- a/livetree.c
+++ b/livetree.c
@@ -167,6 +167,75 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
 	return old_node;
 }
 
+struct node *remove_nodes(struct node *node) {
+	struct property *prop;
+	struct node *child, *parent;
+
+	while (node->proplist) {
+		/* Pop the property off the list */
+		prop = node->proplist;
+		node->proplist = prop->next;
+		prop->next = NULL;
+
+		free(prop);
+	}
+
+	/* Free up the node's children */
+	/* This assumes we have a tree and not a cylic or acylic graph */
+	while (node->children) {
+		/* Pop the child node off the list */
+		child = node->children;
+		node->children = child->next_sibling;
+		child->parent = NULL;
+		child->next_sibling = NULL;
+
+		child = remove_nodes(child);
+	}
+
+	/* Remove the node from it's parent's child list */
+	if (node->parent) {
+		int found = 0;
+
+		parent = node->parent;
+
+		/* if the node is the first child ... */
+		if (parent->children == node) {
+			parent->children = node->next_sibling;
+			found = 1;
+		} else {
+			for_each_child(parent, child) {
+				if (child->next_sibling == node) {
+					child->next_sibling = node->next_sibling;
+					found = 1;
+					break;
+				}
+			}
+		}
+		assert(found); /* Woah! We've got an inconsistent tree here */
+		node->parent = NULL;
+		node->next_sibling = NULL;
+	}
+
+	free(node);
+
+	return NULL;
+}
+
+struct node *replace_nodes(struct node *old_node, struct node *new_node)
+{
+	struct label *l;
+
+	/* Add old node labels to new node, so we can still refer to the
+	 * new node by the old labels.  */
+	for_each_label(old_node->labels, l)
+		add_label(&new_node->labels, l->label);
+
+	/* Free up the old_node */
+	remove_nodes(old_node);
+
+	return new_node;
+}
+
 struct node *chain_node(struct node *first, struct node *list)
 {
 	assert(first->next_sibling == NULL);

Re: [PATCH] Implements new features for updating existing device tree nodes.

[Grant Likely]

Hi John.

On Fri, Oct 15, 2010 at 7:07 PM, John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> This is interesting when the /include/ "<filename>" feature is used. This way
> we can create base device tree source files for a family of systems and modify
> the device tree for a specific system.
>
> The current sytem allows an existing node to be extended with new properties
> and subnodes.
>
> The new features allow an existing node to be replaced completely by the new
> properties and subnodes. The new features also allow an existing node to be
> deleted.
>
> This patch does not yet provide a way to remove properties.
>
> - John

Also need s-o-b line, and anything that doesn't belong as part of the
commit message, such as '- John', needs to be below the '---' line so
that git will ignore it.

The patch looks pretty good, but I cannot comment fully until the
keyword handling works when deeper in the tree (see below).

> ---
>
>  dtc-lexer.l  |   12 ++++++++++
>  dtc-parser.y |   24 ++++++++++++++++++++
>  dtc.h        |    2 ++
>  livetree.c   |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 107 insertions(+), 0 deletions(-)
>
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 081e13a..a24edf5 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -96,6 +96,18 @@ static int pop_input_file(void);
>                        return DT_MEMRESERVE;
>                }
>
> +<*>"/trim-node/"       {
> +                       DPRINT("Keyword: /trim-node/\n");
> +                       BEGIN_DEFAULT();
> +                       return DT_REPLACENODE;
> +               }
> +
> +<*>"/remove-node/"     {
> +                       DPRINT("Keyword: /remove-node/\n");
> +                       BEGIN_DEFAULT();
> +                       return DT_REMOVENODE;
> +               }
> +
>  <*>{LABEL}:    {
>                        DPRINT("Label: %s\n", yytext);
>                        yylval.labelref = xstrdup(yytext);
> diff --git a/dtc-parser.y b/dtc-parser.y
> index aa250f1..a9cde72 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -54,6 +54,8 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
>
>  %token DT_V1
>  %token DT_MEMRESERVE
> +%token DT_REPLACENODE
> +%token DT_REMOVENODE
>  %token <propnodename> DT_PROPNODENAME
>  %token <literal> DT_LITERAL
>  %token <cbase> DT_BASE
> @@ -140,6 +142,28 @@ devicetree:
>                                        " node extension", $2);
>                        $$ = $1;
>                }
> +       | devicetree DT_REPLACENODE DT_REF nodedef
> +               {
> +                       struct node *target;
> +
> +                       target = get_node_by_label($1, $3);
> +                       if (target)
> +                               replace_nodes(target, $4);
> +                       else
> +                               yyerror("label, '%s' does not exist in"
> +                                       " node redefinition", $3);
> +                       $$ = $1;
> +               }
> +       | devicetree DT_REMOVENODE DT_REF ';'
> +               {
> +                       struct node *target;
> +
> +                       target = get_node_by_label($1, $3);
> +                       if (target)
> +                               remove_nodes(target);
> +                       else
> +                               yyerror("label, '%s' does not exist in"
> +                                       " node removal", $3);
>        ;

This handles the syntax at the top level, but doesn't deal with the
/*-node/ keywords when they are within the overlay tree.

Also, I don't know if you'll be able to handle it in the same way when
it comes to handling the new tokens embedded within an overlay tree
because it isn't so easy to resolve the node to be removed.  In order
to handle both cases in the same way, it might be better to set a
'replace' flag in struct node that defers the actual remove of the
original node until merge_nodes() time.

>
>  nodedef:
> diff --git a/dtc.h b/dtc.h
> index b36ac5d..1340568 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -175,6 +175,8 @@ struct node *build_node(struct property *proplist, struct node *children);
>  struct node *name_node(struct node *node, char *name);
>  struct node *chain_node(struct node *first, struct node *list);
>  struct node *merge_nodes(struct node *old_node, struct node *new_node);
> +struct node *replace_nodes(struct node *old_node, struct node *new_node);
> +struct node *remove_nodes(struct node *node);
>
>  void add_property(struct node *node, struct property *prop);
>  void add_child(struct node *parent, struct node *child);
> diff --git a/livetree.c b/livetree.c
> index 13c5f10..5e32510 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -167,6 +167,75 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>        return old_node;
>  }
>
> +struct node *remove_nodes(struct node *node) {

Can be void.  The return value of remove_nodes() is never used.

> +       struct property *prop;
> +       struct node *child, *parent;
> +
> +       while (node->proplist) {
> +               /* Pop the property off the list */
> +               prop = node->proplist;
> +               node->proplist = prop->next;
> +               prop->next = NULL;
> +
> +               free(prop);
> +       }

It would be useful to have a 'pop_property()' macro so this could be
simplified to:

	while (prop = pop_child_node(&node->proplist))
		free(prop);

But then on the other hand, most of the list management is open coded
in dtc. It would be nice to use the list_head macros like used in the
kernel, but that is probably work for another day. (if it is even worth
the effort).

http://ccan.ozlabs.org/info/list.html

> +
> +       /* Free up the node's children */
> +       /* This assumes we have a tree and not a cylic or acylic graph */
> +       while (node->children) {
> +               /* Pop the child node off the list */
> +               child = node->children;
> +               node->children = child->next_sibling;
> +               child->parent = NULL;
> +               child->next_sibling = NULL;
> +
> +               child = remove_nodes(child);
> +       }

Ditto here, except as pop_child_node().

> +
> +       /* Remove the node from it's parent's child list */
> +       if (node->parent) {
> +               int found = 0;
> +
> +               parent = node->parent;
> +
> +               /* if the node is the first child ... */
> +               if (parent->children == node) {
> +                       parent->children = node->next_sibling;
> +                       found = 1;
> +               } else {
> +                       for_each_child(parent, child) {
> +                               if (child->next_sibling == node) {
> +                                       child->next_sibling = node->next_sibling;
> +                                       found = 1;
> +                                       break;
> +                               }
> +                       }
> +               }
> +               assert(found); /* Woah! We've got an inconsistent tree here */
> +               node->parent = NULL;
> +               node->next_sibling = NULL;
> +       }

This hunk is redundantly located because recursive calls have already
removed the node from the parent.  Should split into a separate
function called by dtc-parse.y so that the recursive bit is simpler.

> +
> +       free(node);
> +
> +       return NULL;
> +}

This block looks correct to me.  It seemed a little verbose, but when
I went through it line by line I saw that it pretty much needs to be
that way.  Especially with the list operations being open coded.

> +
> +struct node *replace_nodes(struct node *old_node, struct node *new_node)
> +{
> +       struct label *l;
> +
> +       /* Add old node labels to new node, so we can still refer to the
> +        * new node by the old labels.  */
> +       for_each_label(old_node->labels, l)
> +               add_label(&new_node->labels, l->label);
> +
> +       /* Free up the old_node */
> +       remove_nodes(old_node);
> +
> +       return new_node;
> +}
> +
>  struct node *chain_node(struct node *first, struct node *list)
>  {
>        assert(first->next_sibling == NULL);
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] Implements new features for updating existing device tree nodes.

[John Bonesio]

On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
> Hi John.
> 
> On Fri, Oct 15, 2010 at 7:07 PM, John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> > This is interesting when the /include/ "<filename>" feature is used. This way
> > we can create base device tree source files for a family of systems and modify
> > the device tree for a specific system.
> >
> > The current sytem allows an existing node to be extended with new properties
> > and subnodes.
> >
> > The new features allow an existing node to be replaced completely by the new
> > properties and subnodes. The new features also allow an existing node to be
> > deleted.
> >
> > This patch does not yet provide a way to remove properties.
> >
> > - John
> 
> Also need s-o-b line, and anything that doesn't belong as part of the
> commit message, such as '- John', needs to be below the '---' line so
> that git will ignore it.
> 
> The patch looks pretty good, but I cannot comment fully until the
> keyword handling works when deeper in the tree (see below).
> 
> > ---
> >
> >  dtc-lexer.l  |   12 ++++++++++
> >  dtc-parser.y |   24 ++++++++++++++++++++
> >  dtc.h        |    2 ++
> >  livetree.c   |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 107 insertions(+), 0 deletions(-)
> >
> > diff --git a/dtc-lexer.l b/dtc-lexer.l
> > index 081e13a..a24edf5 100644
> > --- a/dtc-lexer.l
> > +++ b/dtc-lexer.l
> > @@ -96,6 +96,18 @@ static int pop_input_file(void);
> >                        return DT_MEMRESERVE;
> >                }
> >
> > +<*>"/trim-node/"       {
> > +                       DPRINT("Keyword: /trim-node/\n");
> > +                       BEGIN_DEFAULT();
> > +                       return DT_REPLACENODE;
> > +               }
> > +
> > +<*>"/remove-node/"     {
> > +                       DPRINT("Keyword: /remove-node/\n");
> > +                       BEGIN_DEFAULT();
> > +                       return DT_REMOVENODE;
> > +               }
> > +
> >  <*>{LABEL}:    {
> >                        DPRINT("Label: %s\n", yytext);
> >                        yylval.labelref = xstrdup(yytext);
> > diff --git a/dtc-parser.y b/dtc-parser.y
> > index aa250f1..a9cde72 100644
> > --- a/dtc-parser.y
> > +++ b/dtc-parser.y
> > @@ -54,6 +54,8 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
> >
> >  %token DT_V1
> >  %token DT_MEMRESERVE
> > +%token DT_REPLACENODE
> > +%token DT_REMOVENODE
> >  %token <propnodename> DT_PROPNODENAME
> >  %token <literal> DT_LITERAL
> >  %token <cbase> DT_BASE
> > @@ -140,6 +142,28 @@ devicetree:
> >                                        " node extension", $2);
> >                        $$ = $1;
> >                }
> > +       | devicetree DT_REPLACENODE DT_REF nodedef
> > +               {
> > +                       struct node *target;
> > +
> > +                       target = get_node_by_label($1, $3);
> > +                       if (target)
> > +                               replace_nodes(target, $4);
> > +                       else
> > +                               yyerror("label, '%s' does not exist in"
> > +                                       " node redefinition", $3);
> > +                       $$ = $1;
> > +               }
> > +       | devicetree DT_REMOVENODE DT_REF ';'
> > +               {
> > +                       struct node *target;
> > +
> > +                       target = get_node_by_label($1, $3);
> > +                       if (target)
> > +                               remove_nodes(target);
> > +                       else
> > +                               yyerror("label, '%s' does not exist in"
> > +                                       " node removal", $3);
> >        ;
> 
> This handles the syntax at the top level, but doesn't deal with the
> /*-node/ keywords when they are within the overlay tree.
> 
> Also, I don't know if you'll be able to handle it in the same way when
> it comes to handling the new tokens embedded within an overlay tree
> because it isn't so easy to resolve the node to be removed.  In order
> to handle both cases in the same way, it might be better to set a
> 'replace' flag in struct node that defers the actual remove of the
> original node until merge_nodes() time.

Ok, so maybe I misunderstood. I thought we had decided that node
modifications would only happen at the top level, but now that I'm
looking back through your examples earlier today, and I see this isn't
true.

I'll take a look at changing the implementation.

> 
> >
> >  nodedef:
> > diff --git a/dtc.h b/dtc.h
> > index b36ac5d..1340568 100644
> > --- a/dtc.h
> > +++ b/dtc.h
> > @@ -175,6 +175,8 @@ struct node *build_node(struct property *proplist, struct node *children);
> >  struct node *name_node(struct node *node, char *name);
> >  struct node *chain_node(struct node *first, struct node *list);
> >  struct node *merge_nodes(struct node *old_node, struct node *new_node);
> > +struct node *replace_nodes(struct node *old_node, struct node *new_node);
> > +struct node *remove_nodes(struct node *node);
> >
> >  void add_property(struct node *node, struct property *prop);
> >  void add_child(struct node *parent, struct node *child);
> > diff --git a/livetree.c b/livetree.c
> > index 13c5f10..5e32510 100644
> > --- a/livetree.c
> > +++ b/livetree.c
> > @@ -167,6 +167,75 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> >        return old_node;
> >  }
> >
> > +struct node *remove_nodes(struct node *node) {
> 
> Can be void.  The return value of remove_nodes() is never used.
> 
> > +       struct property *prop;
> > +       struct node *child, *parent;
> > +
> > +       while (node->proplist) {
> > +               /* Pop the property off the list */
> > +               prop = node->proplist;
> > +               node->proplist = prop->next;
> > +               prop->next = NULL;
> > +
> > +               free(prop);
> > +       }
> 
> It would be useful to have a 'pop_property()' macro so this could be
> simplified to:
> 
> 	while (prop = pop_child_node(&node->proplist))
> 		free(prop);
> 
> But then on the other hand, most of the list management is open coded
> in dtc. It would be nice to use the list_head macros like used in the
> kernel, but that is probably work for another day. (if it is even worth
> the effort).
> 
> http://ccan.ozlabs.org/info/list.html
> 
> > +
> > +       /* Free up the node's children */
> > +       /* This assumes we have a tree and not a cylic or acylic graph */
> > +       while (node->children) {
> > +               /* Pop the child node off the list */
> > +               child = node->children;
> > +               node->children = child->next_sibling;
> > +               child->parent = NULL;
> > +               child->next_sibling = NULL;
> > +
> > +               child = remove_nodes(child);
> > +       }
> 
> Ditto here, except as pop_child_node().
> 
> > +
> > +       /* Remove the node from it's parent's child list */
> > +       if (node->parent) {
> > +               int found = 0;
> > +
> > +               parent = node->parent;
> > +
> > +               /* if the node is the first child ... */
> > +               if (parent->children == node) {
> > +                       parent->children = node->next_sibling;
> > +                       found = 1;
> > +               } else {
> > +                       for_each_child(parent, child) {
> > +                               if (child->next_sibling == node) {
> > +                                       child->next_sibling = node->next_sibling;
> > +                                       found = 1;
> > +                                       break;
> > +                               }
> > +                       }
> > +               }
> > +               assert(found); /* Woah! We've got an inconsistent tree here */
> > +               node->parent = NULL;
> > +               node->next_sibling = NULL;
> > +       }
> 
> This hunk is redundantly located because recursive calls have already
> removed the node from the parent.  Should split into a separate
> function called by dtc-parse.y so that the recursive bit is simpler.
> 
> > +
> > +       free(node);
> > +
> > +       return NULL;
> > +}
> 
> This block looks correct to me.  It seemed a little verbose, but when
> I went through it line by line I saw that it pretty much needs to be
> that way.  Especially with the list operations being open coded.
> 
> > +
> > +struct node *replace_nodes(struct node *old_node, struct node *new_node)
> > +{
> > +       struct label *l;
> > +
> > +       /* Add old node labels to new node, so we can still refer to the
> > +        * new node by the old labels.  */
> > +       for_each_label(old_node->labels, l)
> > +               add_label(&new_node->labels, l->label);
> > +
> > +       /* Free up the old_node */
> > +       remove_nodes(old_node);
> > +
> > +       return new_node;
> > +}
> > +
> >  struct node *chain_node(struct node *first, struct node *list)
> >  {
> >        assert(first->next_sibling == NULL);
> >
> > _______________________________________________
> > devicetree-discuss mailing list
> > devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> > https://lists.ozlabs.org/listinfo/devicetree-discuss
> >
> 
> 
> 

Re: [PATCH] Implements new features for updating existing device tree nodes.

[David Gibson]

On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
> On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
[snip]
> > > +       | devicetree DT_REMOVENODE DT_REF ';'
> > > +               {
> > > +                       struct node *target;
> > > +
> > > +                       target = get_node_by_label($1, $3);
> > > +                       if (target)
> > > +                               remove_nodes(target);
> > > +                       else
> > > +                               yyerror("label, '%s' does not exist in"
> > > +                                       " node removal", $3);
> > >        ;
> > 
> > This handles the syntax at the top level, but doesn't deal with the
> > /*-node/ keywords when they are within the overlay tree.
> > 
> > Also, I don't know if you'll be able to handle it in the same way when
> > it comes to handling the new tokens embedded within an overlay tree
> > because it isn't so easy to resolve the node to be removed.  In order
> > to handle both cases in the same way, it might be better to set a
> > 'replace' flag in struct node that defers the actual remove of the
> > original node until merge_nodes() time.
> 
> Ok, so maybe I misunderstood. I thought we had decided that node
> modifications would only happen at the top level, but now that I'm
> looking back through your examples earlier today, and I see this isn't
> true.

Um.. that part of the discussion got confusing.  I think only allowing
the operations (and I still don't think the set implemented here are
quite right, though they're close) at the top-level is a better idea.

Grant, is there a reason you think we need to have replacement as an
option within each tree, rather than a "per-overlay-sheet" option.

-- 
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] Implements new features for updating existing device tree nodes.

[Grant Likely]

On Sat, Oct 16, 2010 at 05:47:44PM +1100, David Gibson wrote:
> On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
> > On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
> [snip]
> > > > +       | devicetree DT_REMOVENODE DT_REF ';'
> > > > +               {
> > > > +                       struct node *target;
> > > > +
> > > > +                       target = get_node_by_label($1, $3);
> > > > +                       if (target)
> > > > +                               remove_nodes(target);
> > > > +                       else
> > > > +                               yyerror("label, '%s' does not exist in"
> > > > +                                       " node removal", $3);
> > > >        ;
> > > 
> > > This handles the syntax at the top level, but doesn't deal with the
> > > /*-node/ keywords when they are within the overlay tree.
> > > 
> > > Also, I don't know if you'll be able to handle it in the same way when
> > > it comes to handling the new tokens embedded within an overlay tree
> > > because it isn't so easy to resolve the node to be removed.  In order
> > > to handle both cases in the same way, it might be better to set a
> > > 'replace' flag in struct node that defers the actual remove of the
> > > original node until merge_nodes() time.
> > 
> > Ok, so maybe I misunderstood. I thought we had decided that node
> > modifications would only happen at the top level, but now that I'm
> > looking back through your examples earlier today, and I see this isn't
> > true.
> 
> Um.. that part of the discussion got confusing.  I think only allowing
> the operations (and I still don't think the set implemented here are
> quite right, though they're close) at the top-level is a better idea.
> 
> Grant, is there a reason you think we need to have replacement as an
> option within each tree, rather than a "per-overlay-sheet" option.

Yes, because I need the ability to remove/replace a child of a
labelled node.  Not every node that needs to be manipulated will
necessarily have a label.

For example; I could have a labelled spi bus node with child nodes for
each of the spi devices.  I want to be able to remove/replace the
child nodes without having to label them first.

g.

Re: [PATCH] Implements new features for updating existing device tree nodes.

[David Gibson]

On Sat, Oct 16, 2010 at 02:10:12AM -0600, Grant Likely wrote:
> On Sat, Oct 16, 2010 at 05:47:44PM +1100, David Gibson wrote:
> > On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
> > > On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
[snip]
> > > Ok, so maybe I misunderstood. I thought we had decided that node
> > > modifications would only happen at the top level, but now that I'm
> > > looking back through your examples earlier today, and I see this isn't
> > > true.
> > 
> > Um.. that part of the discussion got confusing.  I think only allowing
> > the operations (and I still don't think the set implemented here are
> > quite right, though they're close) at the top-level is a better idea.
> > 
> > Grant, is there a reason you think we need to have replacement as an
> > option within each tree, rather than a "per-overlay-sheet" option.
> 
> Yes, because I need the ability to remove/replace a child of a
> labelled node.  Not every node that needs to be manipulated will
> necessarily have a label.
> 
> For example; I could have a labelled spi bus node with child nodes for
> each of the spi devices.  I want to be able to remove/replace the
> child nodes without having to label them first.

Ah, I see.  This could also be accomplished if we allowed overlays to
be targeted at (label + relative path) which we were talking about
anyway.

-- 
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] Implements new features for updating existing device tree nodes.

[John Bonesio]

On Sat, 2010-10-16 at 02:10 -0600, Grant Likely wrote:
> On Sat, Oct 16, 2010 at 05:47:44PM +1100, David Gibson wrote:
> > On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
> > > On Fri, 2010-10-15 at 20:35 -0600, Grant Likely wrote:
> > [snip]
> > > > > +       | devicetree DT_REMOVENODE DT_REF ';'
> > > > > +               {
> > > > > +                       struct node *target;
> > > > > +
> > > > > +                       target = get_node_by_label($1, $3);
> > > > > +                       if (target)
> > > > > +                               remove_nodes(target);
> > > > > +                       else
> > > > > +                               yyerror("label, '%s' does not exist in"
> > > > > +                                       " node removal", $3);
> > > > >        ;
> > > > 
> > > > This handles the syntax at the top level, but doesn't deal with the
> > > > /*-node/ keywords when they are within the overlay tree.
> > > > 
> > > > Also, I don't know if you'll be able to handle it in the same way when
> > > > it comes to handling the new tokens embedded within an overlay tree
> > > > because it isn't so easy to resolve the node to be removed.  In order
> > > > to handle both cases in the same way, it might be better to set a
> > > > 'replace' flag in struct node that defers the actual remove of the
> > > > original node until merge_nodes() time.
> > > 
> > > Ok, so maybe I misunderstood. I thought we had decided that node
> > > modifications would only happen at the top level, but now that I'm
> > > looking back through your examples earlier today, and I see this isn't
> > > true.
> > 
> > Um.. that part of the discussion got confusing.  I think only allowing
> > the operations (and I still don't think the set implemented here are
> > quite right, though they're close) at the top-level is a better idea.
> > 
> > Grant, is there a reason you think we need to have replacement as an
> > option within each tree, rather than a "per-overlay-sheet" option.
> 
> Yes, because I need the ability to remove/replace a child of a
> labelled node.  Not every node that needs to be manipulated will
> necessarily have a label.
> 
> For example; I could have a labelled spi bus node with child nodes for
> each of the spi devices.  I want to be able to remove/replace the
> child nodes without having to label them first.
> 
> g.
> 

Why not just add the label to the node you need first?

- John

Re: [PATCH] Implements new features for updating existing device tree nodes.

[Grant Likely]

On Sat, Oct 16, 2010 at 06:45:47AM -0700, John Bonesio wrote:
> On Sat, 2010-10-16 at 02:10 -0600, Grant Likely wrote:
> > On Sat, Oct 16, 2010 at 05:47:44PM +1100, David Gibson wrote:
> > > On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
> > > > Ok, so maybe I misunderstood. I thought we had decided that node
> > > > modifications would only happen at the top level, but now that I'm
> > > > looking back through your examples earlier today, and I see this isn't
> > > > true.
> > > 
> > > Um.. that part of the discussion got confusing.  I think only allowing
> > > the operations (and I still don't think the set implemented here are
> > > quite right, though they're close) at the top-level is a better idea.
> > > 
> > > Grant, is there a reason you think we need to have replacement as an
> > > option within each tree, rather than a "per-overlay-sheet" option.
> > 
> > Yes, because I need the ability to remove/replace a child of a
> > labelled node.  Not every node that needs to be manipulated will
> > necessarily have a label.
> > 
> > For example; I could have a labelled spi bus node with child nodes for
> > each of the spi devices.  I want to be able to remove/replace the
> > child nodes without having to label them first.
> > 
> > g.
> > 
> 
> Why not just add the label to the node you need first?

That may not be possible and/or desirable.  For instance, when the
tree is extracted from /proc.  Or if the base tree in stored in the
kernel source and an external tool is modifying it for a specific
purpose (when unable to patch the kernel tree).  Or for generated
device trees from the Xilinx toolchain for FPGAs.  Modifying the
source data isn't always an option.

Plus I want the ability to logically group related changes.  ie.
delete a device and add a device to the same bus at the same time.  Or
delete a phy node, add a replacement phy node at a different address,
and update the ethernet phy-device phandle in the same block.

g.

Re: [PATCH] Implements new features for updating existing device tree nodes.

[David Gibson]

On Sat, Oct 16, 2010 at 12:32:17PM -0600, Grant Likely wrote:
> On Sat, Oct 16, 2010 at 06:45:47AM -0700, John Bonesio wrote:
> > On Sat, 2010-10-16 at 02:10 -0600, Grant Likely wrote:
> > > On Sat, Oct 16, 2010 at 05:47:44PM +1100, David Gibson wrote:
> > > > On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
[snip]
> > > For example; I could have a labelled spi bus node with child nodes for
> > > each of the spi devices.  I want to be able to remove/replace the
> > > child nodes without having to label them first.
> > > 
> > > g.
> > 
> > Why not just add the label to the node you need first?
> 
> That may not be possible and/or desirable.  For instance, when the
> tree is extracted from /proc.  Or if the base tree in stored in the
> kernel source and an external tool is modifying it for a specific
> purpose (when unable to patch the kernel tree).  Or for generated
> device trees from the Xilinx toolchain for FPGAs.  Modifying the
> source data isn't always an option.

You can apply your overlays by path in this case..

> Plus I want the ability to logically group related changes.  ie.
> delete a device and add a device to the same bus at the same time.
> Or delete a phy node, add a replacement phy node at a different
> address, and update the ethernet phy-device phandle in the same
> block.

Hrm, I really don't think this is a good argument.  You're conflating
the grouping of logically related changes with the tree structure
which I think is a mistake.  Putting the changes in the same file
and/or adjusting your spacing and comments to show the logical
grouping seems less likely to cause other problems.

-- 
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] Implements new features for updating existing device tree nodes.

[David Gibson]

On Fri, Oct 15, 2010 at 06:07:50PM -0700, John Bonesio wrote:
> This is interesting when the /include/ "<filename>" feature is used. This way
> we can create base device tree source files for a family of systems and modify
> the device tree for a specific system.
> 
> The current sytem allows an existing node to be extended with new properties
> and subnodes.
> 
> The new features allow an existing node to be replaced completely by the new
> properties and subnodes. The new features also allow an existing node to be
> deleted.
> 
> This patch does not yet provide a way to remove properties.

Detailed comments below.

>  dtc-lexer.l  |   12 ++++++++++
>  dtc-parser.y |   24 ++++++++++++++++++++
>  dtc.h        |    2 ++
>  livetree.c   |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 107 insertions(+), 0 deletions(-)
> 
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 081e13a..a24edf5 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -96,6 +96,18 @@ static int pop_input_file(void);
>  			return DT_MEMRESERVE;
>  		}
>  
> +<*>"/trim-node/"	{
> +			DPRINT("Keyword: /trim-node/\n");
> +			BEGIN_DEFAULT();
> +			return DT_REPLACENODE;
> +		}

Hrm.  The keyword is "/trim-node/", but the C symbol is REPLACENODE.
Potentially confusing.

Also, while "trim-node" seemed a reasonable option to me in some
contexts, with the semantics implemented in this patch, I think
"replace-node" would be far clearer.

> +<*>"/remove-node/"	{
> +			DPRINT("Keyword: /remove-node/\n");
> +			BEGIN_DEFAULT();
> +			return DT_REMOVENODE;
> +		}
> +
>  <*>{LABEL}:	{
>  			DPRINT("Label: %s\n", yytext);
>  			yylval.labelref = xstrdup(yytext);
> diff --git a/dtc-parser.y b/dtc-parser.y
> index aa250f1..a9cde72 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -54,6 +54,8 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
>  
>  %token DT_V1
>  %token DT_MEMRESERVE
> +%token DT_REPLACENODE
> +%token DT_REMOVENODE
>  %token <propnodename> DT_PROPNODENAME
>  %token <literal> DT_LITERAL
>  %token <cbase> DT_BASE
> @@ -140,6 +142,28 @@ devicetree:
>  					" node extension", $2);
>  			$$ = $1;
>  		}
> +	| devicetree DT_REPLACENODE DT_REF nodedef
> +		{
> +			struct node *target;
> +
> +			target = get_node_by_label($1, $3);
> +			if (target)
> +				replace_nodes(target, $4);
> +			else
> +				yyerror("label, '%s' does not exist in"
> +					" node redefinition", $3);
> +			$$ = $1;
> +		}
> +	| devicetree DT_REMOVENODE DT_REF ';'
> +		{
> +			struct node *target;
> +
> +			target = get_node_by_label($1, $3);
> +			if (target)
> +				remove_nodes(target);
> +			else
> +				yyerror("label, '%s' does not exist in"
> +					" node removal", $3);
>  	;
>  
>  nodedef:
> diff --git a/dtc.h b/dtc.h
> index b36ac5d..1340568 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -175,6 +175,8 @@ struct node *build_node(struct property *proplist, struct node *children);
>  struct node *name_node(struct node *node, char *name);
>  struct node *chain_node(struct node *first, struct node *list);
>  struct node *merge_nodes(struct node *old_node, struct node *new_node);
> +struct node *replace_nodes(struct node *old_node, struct node *new_node);
> +struct node *remove_nodes(struct node *node);
>  
>  void add_property(struct node *node, struct property *prop);
>  void add_child(struct node *parent, struct node *child);
> diff --git a/livetree.c b/livetree.c
> index 13c5f10..5e32510 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -167,6 +167,75 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
>  	return old_node;
>  }
>  
> +struct node *remove_nodes(struct node *node) {
> +	struct property *prop;
> +	struct node *child, *parent;
> +
> +	while (node->proplist) {
> +		/* Pop the property off the list */
> +		prop = node->proplist;
> +		node->proplist = prop->next;
> +		prop->next = NULL;
> +
> +		free(prop);
> +	}
> +
> +	/* Free up the node's children */
> +	/* This assumes we have a tree and not a cylic or acylic graph */
> +	while (node->children) {
> +		/* Pop the child node off the list */
> +		child = node->children;
> +		node->children = child->next_sibling;
> +		child->parent = NULL;
> +		child->next_sibling = NULL;
> +
> +		child = remove_nodes(child);
> +	}
> +
> +	/* Remove the node from it's parent's child list */
> +	if (node->parent) {
> +		int found = 0;

This should go first, before the recursive free()ing.  I know we don't
have any concurrency to worry about in dtc, but as a general habit it
should always be remove from the structure first, then do the
disposal.

And then, don't bother with the disposal.  I'm pretty sure we already
have some leaks and I really don't care.  dtc is a strictly transient
process, so we can basically treating malloc() as a pool allocator
with exactly one pool, freed at exit.  If we do need something more
subtle infuture, we can use an actual pool allocator (talloc, for
preference).  The fact that in dts parsing we build the tree from the
leaves, but in dtb parsing we bulid it from the root makes lifetimes
complex enough that fully correct explicit free()ing is actually quite
a pain.

(Oh, and, in the case where you're removing, not replacing, you
already leak the labels. See my point :)

> +		parent = node->parent;
> +
> +		/* if the node is the first child ... */
> +		if (parent->children == node) {
> +			parent->children = node->next_sibling;
> +			found = 1;
> +		} else {
> +			for_each_child(parent, child) {
> +				if (child->next_sibling == node) {
> +					child->next_sibling = node->next_sibling;
> +					found = 1;
> +					break;
> +				}
> +			}
> +		}

You can do better:

	ptr = &parent->children;
	for_each_child(...) {
		if (child == node) {
			*ptr = child->next_sibling;
			found = 1;
			     ptr = &child->next_sibling;
		}
	}

This pointer-to-pointer idiom is used in a number of other places.

> +		assert(found); /* Woah! We've got an inconsistent tree here */
> +		node->parent = NULL;
> +		node->next_sibling = NULL;
> +	}
> +
> +	free(node);
> +
> +	return NULL;
> +}
> +
> +struct node *replace_nodes(struct node *old_node, struct node *new_node)
> +{
> +	struct label *l;
> +
> +	/* Add old node labels to new node, so we can still refer to the
> +	 * new node by the old labels.  */
> +	for_each_label(old_node->labels, l)
> +		add_label(&new_node->labels, l->label);

Yeah.  labels to subnodes of the original still disappear without
trace, and could therefore be re-used.  I think this has the potential
to be very confusing.  We need to be more careful with the label
semantics.

> +	/* Free up the old_node */
> +	remove_nodes(old_node);
> +
> +	return new_node;
> +}
> +
>  struct node *chain_node(struct node *first, struct node *list)
>  {
>  	assert(first->next_sibling == NULL);
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

-- 
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] Implements new features for updating existing device tree nodes.

[David Gibson]

On Fri, Oct 15, 2010 at 08:35:56PM -0600, Grant Likely wrote:
> Hi John.
> 
> On Fri, Oct 15, 2010 at 7:07 PM, John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> > This is interesting when the /include/ "<filename>" feature is used. This way
> > we can create base device tree source files for a family of systems and modify
> > the device tree for a specific system.
> >
> > The current sytem allows an existing node to be extended with new properties
> > and subnodes.
> >
> > The new features allow an existing node to be replaced completely by the new
> > properties and subnodes. The new features also allow an existing node to be
> > deleted.
> >
> > This patch does not yet provide a way to remove properties.
> >
> > - John
> 
> Also need s-o-b line, and anything that doesn't belong as part of the
> commit message, such as '- John', needs to be below the '---' line so
> that git will ignore it.
> 
> The patch looks pretty good, but I cannot comment fully until the
> keyword handling works when deeper in the tree (see below).

[snip]
> > +       struct property *prop;
> > +       struct node *child, *parent;
> > +
> > +       while (node->proplist) {
> > +               /* Pop the property off the list */
> > +               prop = node->proplist;
> > +               node->proplist = prop->next;
> > +               prop->next = NULL;
> > +
> > +               free(prop);
> > +       }
> 
> It would be useful to have a 'pop_property()' macro so this could be
> simplified to:
> 
> 	while (prop = pop_child_node(&node->proplist))
> 		free(prop);
> 
> But then on the other hand, most of the list management is open coded
> in dtc. It would be nice to use the list_head macros like used in the
> kernel, but that is probably work for another day. (if it is even worth
> the effort).

I had a look into this at one point.  The way we build up the tree in
a kinda-sorta functional programming like way in dts parsing actually
makes using the list.h form kind of hard.  At least without a fancier
allocator (maybe I should bite the bullet and do a talloc conversion).

-- 
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] Implements new features for updating existing device tree nodes.

[Grant Likely]

On Mon, Oct 18, 2010 at 11:11:35AM +1100, David Gibson wrote:
> On Sat, Oct 16, 2010 at 12:32:17PM -0600, Grant Likely wrote:
> > On Sat, Oct 16, 2010 at 06:45:47AM -0700, John Bonesio wrote:
> > > On Sat, 2010-10-16 at 02:10 -0600, Grant Likely wrote:
> > > > On Sat, Oct 16, 2010 at 05:47:44PM +1100, David Gibson wrote:
> > > > > On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
> [snip]
> > > > For example; I could have a labelled spi bus node with child nodes for
> > > > each of the spi devices.  I want to be able to remove/replace the
> > > > child nodes without having to label them first.
> > > > 
> > > > g.
> > > 
> > > Why not just add the label to the node you need first?
> > 
> > That may not be possible and/or desirable.  For instance, when the
> > tree is extracted from /proc.  Or if the base tree in stored in the
> > kernel source and an external tool is modifying it for a specific
> > purpose (when unable to patch the kernel tree).  Or for generated
> > device trees from the Xilinx toolchain for FPGAs.  Modifying the
> > source data isn't always an option.
> 
> You can apply your overlays by path in this case..

Right, but my argument is that in this case it is a useful syntax to
be able to delve deep into the tree, or deeper off of a label and
delete nodes within it.  And it is actually a really simple thing to
implement cleanly (as shown in the patch I posted).

> 
> > Plus I want the ability to logically group related changes.  ie.
> > delete a device and add a device to the same bus at the same time.
> > Or delete a phy node, add a replacement phy node at a different
> > address, and update the ethernet phy-device phandle in the same
> > block.
> 
> Hrm, I really don't think this is a good argument.  You're conflating
> the grouping of logically related changes with the tree structure
> which I think is a mistake.  Putting the changes in the same file
> and/or adjusting your spacing and comments to show the logical
> grouping seems less likely to cause other problems.

Though I'm not happy with it, I can live with only allowing removal of
nodes at the top level.  In fact, that constraint also completely
removes the need for a /replace-node/ keyword since the remove
operation is performed at the top level where ordering is explicitly
allowed.  So, instead of:

	/{
		node { oldprop; };
	};
	/{
		/replace-node/ the-node { newprop; };
	};

do this instead:

	/{
		node { oldprop; };
	};
	/remove-node/ &{/the-node};
	/{
		the-node { newprop; };
	};

It means only the /remove-node/ keyword needs to be defined.  However,
it does mean that the syntax for full path references must be defined
immediately before it is useful.

g.

Re: [PATCH] Implements new features for updating existing device tree nodes.

[Scott Wood]

On Mon, 18 Oct 2010 10:01:15 -0600
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Mon, Oct 18, 2010 at 11:11:35AM +1100, David Gibson wrote:
> > On Sat, Oct 16, 2010 at 12:32:17PM -0600, Grant Likely wrote:
> > > Plus I want the ability to logically group related changes.  ie.
> > > delete a device and add a device to the same bus at the same time.
> > > Or delete a phy node, add a replacement phy node at a different
> > > address, and update the ethernet phy-device phandle in the same
> > > block.
> > 
> > Hrm, I really don't think this is a good argument.  You're conflating
> > the grouping of logically related changes with the tree structure
> > which I think is a mistake.  Putting the changes in the same file
> > and/or adjusting your spacing and comments to show the logical
> > grouping seems less likely to cause other problems.

Shouldn't the tree structure, if practical, represent the logical
structure?  The actual operations (as the user intends them) are on
particular properties or subnodes, not on top-level trees.

Likewise with the extension of nodes being unordered in the output
tree (or allowing forward references) to disallowing any
situation where ordering of dts statements matters.  It seems to be
sticking with a certain design idea just for the sake of it (or
possibly for the convenience of the existing dtc implementation?), at
the cost of readability/intuitiveness.

It's not an unbearable cost, but I wonder what we really gain from it.

> Though I'm not happy with it, I can live with only allowing removal of
> nodes at the top level.  In fact, that constraint also completely
> removes the need for a /replace-node/ keyword since the remove
> operation is performed at the top level where ordering is explicitly
> allowed.  So, instead of:
> 
> 	/{
> 		node { oldprop; };
> 	};
> 	/{
> 		/replace-node/ the-node { newprop; };
> 	};
> 
> do this instead:
> 
> 	/{
> 		node { oldprop; };
> 	};
> 	/remove-node/ &{/the-node};
> 	/{
> 		the-node { newprop; };
> 	};
> 
> It means only the /remove-node/ keyword needs to be defined.

What about property removal?  Or do you mean the only one related to
nodes?

-Scott

Re: [PATCH] Implements new features for updating existing device tree nodes.

[Grant Likely]

On Mon, Oct 18, 2010 at 12:42:20PM -0500, Scott Wood wrote:
> On Mon, 18 Oct 2010 10:01:15 -0600
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> > On Mon, Oct 18, 2010 at 11:11:35AM +1100, David Gibson wrote:
> > > On Sat, Oct 16, 2010 at 12:32:17PM -0600, Grant Likely wrote:
> > > > Plus I want the ability to logically group related changes.  ie.
> > > > delete a device and add a device to the same bus at the same time.
> > > > Or delete a phy node, add a replacement phy node at a different
> > > > address, and update the ethernet phy-device phandle in the same
> > > > block.
> > > 
> > > Hrm, I really don't think this is a good argument.  You're conflating
> > > the grouping of logically related changes with the tree structure
> > > which I think is a mistake.  Putting the changes in the same file
> > > and/or adjusting your spacing and comments to show the logical
> > > grouping seems less likely to cause other problems.
> 
> Shouldn't the tree structure, if practical, represent the logical
> structure?  The actual operations (as the user intends them) are on
> particular properties or subnodes, not on top-level trees.
> 
> Likewise with the extension of nodes being unordered in the output
> tree (or allowing forward references) to disallowing any
> situation where ordering of dts statements matters.  It seems to be
> sticking with a certain design idea just for the sake of it (or
> possibly for the convenience of the existing dtc implementation?), at
> the cost of readability/intuitiveness.

The current structure of dtc is allowed to be quite simple because it
builds up each top level tree in memory and then merge them wholesale.
It doesn't have to merge at two levels (ie. merge each node as it is
added, and then merge the trees again at the top level), and the
parser doesn't need to have any context about the rest of the tree
until the trees get merged.

It really does keep things simple without being too onerous on the
user.  At the very least, I don't have any use cases that require the
ability to reference the same node twice from within a single top
level tree.

> It's not an unbearable cost, but I wonder what we really gain from it.
> 
> > Though I'm not happy with it, I can live with only allowing removal of
> > nodes at the top level.  In fact, that constraint also completely
> > removes the need for a /replace-node/ keyword since the remove
> > operation is performed at the top level where ordering is explicitly
> > allowed.  So, instead of:
> > 
> > 	/{
> > 		node { oldprop; };
> > 	};
> > 	/{
> > 		/replace-node/ the-node { newprop; };
> > 	};
> > 
> > do this instead:
> > 
> > 	/{
> > 		node { oldprop; };
> > 	};
> > 	/remove-node/ &{/the-node};
> > 	/{
> > 		the-node { newprop; };
> > 	};
> > 
> > It means only the /remove-node/ keyword needs to be defined.
> 
> What about property removal?  Or do you mean the only one related to
> nodes?

Property removal will have its own syntax because it isn't the same
thing, and properties live in a different namespace from nodes.  Also,
the use case is simpler because 'extend' isn't assumed for properties
like it is for nodes. Properties can either be replaced, or removed
outright.  If we ever do have an extend syntax it will mostly likely
just be another form of replace which a syntax to pull in parts of the
old value (or parts of another properties value).

So anyway, the property removal syntax will be implemented in a
separate patch.

g.

Re: [PATCH] Implements new features for updating existing device tree nodes.

[David Gibson]

On Mon, Oct 18, 2010 at 10:01:15AM -0600, Grant Likely wrote:
> On Mon, Oct 18, 2010 at 11:11:35AM +1100, David Gibson wrote:
> > On Sat, Oct 16, 2010 at 12:32:17PM -0600, Grant Likely wrote:
> > > On Sat, Oct 16, 2010 at 06:45:47AM -0700, John Bonesio wrote:
> > > > On Sat, 2010-10-16 at 02:10 -0600, Grant Likely wrote:
> > > > > On Sat, Oct 16, 2010 at 05:47:44PM +1100, David Gibson wrote:
> > > > > > On Fri, Oct 15, 2010 at 08:21:17PM -0700, John Bonesio wrote:
> > [snip]
> > > > > For example; I could have a labelled spi bus node with child nodes for
> > > > > each of the spi devices.  I want to be able to remove/replace the
> > > > > child nodes without having to label them first.
> > > > > 
> > > > > g.
> > > > 
> > > > Why not just add the label to the node you need first?
> > > 
> > > That may not be possible and/or desirable.  For instance, when the
> > > tree is extracted from /proc.  Or if the base tree in stored in the
> > > kernel source and an external tool is modifying it for a specific
> > > purpose (when unable to patch the kernel tree).  Or for generated
> > > device trees from the Xilinx toolchain for FPGAs.  Modifying the
> > > source data isn't always an option.
> > 
> > You can apply your overlays by path in this case..
> 
> Right, but my argument is that in this case it is a useful syntax to
> be able to delve deep into the tree, or deeper off of a label and
> delete nodes within it.  And it is actually a really simple thing to
> implement cleanly (as shown in the patch I posted).

Oh, it's perfectly easy to implement, yes.  It's the step up in
complexity of the mental model required that concerns me about it (now
each overlay in the stack can have some "transparent" and some
"opaque" parts).

-- 
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] Implements new features for updating existing device tree nodes.

[David Gibson]

On Mon, Oct 18, 2010 at 01:56:29PM -0600, Grant Likely wrote:
> On Mon, Oct 18, 2010 at 12:42:20PM -0500, Scott Wood wrote:
> > On Mon, 18 Oct 2010 10:01:15 -0600
> > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
[snip]
> > Likewise with the extension of nodes being unordered in the output
> > tree (or allowing forward references) to disallowing any
> > situation where ordering of dts statements matters.  It seems to be
> > sticking with a certain design idea just for the sake of it (or
> > possibly for the convenience of the existing dtc implementation?), at
> > the cost of readability/intuitiveness.
> 
> The current structure of dtc is allowed to be quite simple because it
> builds up each top level tree in memory and then merge them wholesale.
> It doesn't have to merge at two levels (ie. merge each node as it is
> added, and then merge the trees again at the top level), and the
> parser doesn't need to have any context about the rest of the tree
> until the trees get merged.
> 
> It really does keep things simple without being too onerous on the
> user.  At the very least, I don't have any use cases that require the
> ability to reference the same node twice from within a single top
> level tree.

It does simplify the implementation a little, but that's not the real
reason for it.  The main reason is that:
	somebus {
		ethernet@100100 {
			reg = <0x100100>;
			...
		};
		ethernet@100100 {
			reg = <0x100200>;
			...
		};
	};

Is probably just a mistake, forgetting to adjust unit address when
they copy/pasted for the second ethernet.  But if we allowed
merging/overriding everywhere, not just at the top level it would be
silently accepted, clobbering most of the first ethernet node's info
with stuff intended for the second.

Allowing the extension only at the top level makes the user do
something explicit to signal that they intend to update/override
existing parts of the tree.

> > It's not an unbearable cost, but I wonder what we really gain from it.
> > 
> > > Though I'm not happy with it, I can live with only allowing removal of
> > > nodes at the top level.  In fact, that constraint also completely
> > > removes the need for a /replace-node/ keyword since the remove
> > > operation is performed at the top level where ordering is explicitly
> > > allowed.  So, instead of:
> > > 
> > > 	/{
> > > 		node { oldprop; };
> > > 	};
> > > 	/{
> > > 		/replace-node/ the-node { newprop; };
> > > 	};
> > > 
> > > do this instead:
> > > 
> > > 	/{
> > > 		node { oldprop; };
> > > 	};
> > > 	/remove-node/ &{/the-node};
> > > 	/{
> > > 		the-node { newprop; };
> > > 	};
> > > 
> > > It means only the /remove-node/ keyword needs to be defined.
> > 
> > What about property removal?  Or do you mean the only one related to
> > nodes?
> 
> Property removal will have its own syntax because it isn't the same
> thing, and properties live in a different namespace from nodes.  Also,
> the use case is simpler because 'extend' isn't assumed for properties
> like it is for nodes. Properties can either be replaced, or removed
> outright.  If we ever do have an extend syntax it will mostly likely
> just be another form of replace which a syntax to pull in parts of the
> old value (or parts of another properties value).

Right.  I think property value expressions are pretty much a
prerequisite for a sane implementation of this.

> So anyway, the property removal syntax will be implemented in a
> separate patch.

I'd actually prefer to see that patch come first, I have far less
doubts about how best to implement that than I do with the node
removing/replacing stuff.

Specifically, I'd happily ack a patch which implemented
	someprop = /undef-prop/;
immediately.

-- 
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] Implements new features for updating existing device tree nodes.

[David Gibson]

On Mon, Oct 18, 2010 at 10:01:15AM -0600, Grant Likely wrote:
[snip]
> It means only the /remove-node/ keyword needs to be defined.  However,
> it does mean that the syntax for full path references must be defined
> immediately before it is useful.

Actually, because of the way DT_REF is lexed, I believe

&{/some/path} {
	prop = "new-value";
};

Will work already.

Or do you mean implementing references to a label + relative path?

-- 
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] Implements new features for updating existing device tree nodes.

[Grant Likely]

On Mon, Oct 18, 2010 at 7:06 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Oct 18, 2010 at 10:01:15AM -0600, Grant Likely wrote:
> [snip]
>> It means only the /remove-node/ keyword needs to be defined.  However,
>> it does mean that the syntax for full path references must be defined
>> immediately before it is useful.
>
> Actually, because of the way DT_REF is lexed, I believe
>
> &{/some/path} {
>        prop = "new-value";
> };
>
> Will work already.
>
> Or do you mean implementing references to a label + relative path?

yes.

Re: [PATCH] Implements new features for updating existing device tree nodes.

[Grant Likely]

On Mon, Oct 18, 2010 at 7:02 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> Specifically, I'd happily ack a patch which implemented
>        someprop = /undef-prop/;
> immediately.

I would to.

g.