[PATCH 2/2] Allow nodes at the root to be specified by path as well as by label.

[John Bonesio]

It seems we have conflicting syntax for path references and label
references.

Here's what we have now:

&label
&{/path/to/the/node}

It's not a problem until we want to combine them. If we want a path rooted
at label suddenly there is the question of the curly braces. What should we
have?

&label{/path/to/the/subnode/}
&{label/path/to/the/subnode/}
&label/path/to/the/subnode


In looking at the dtc code, it appears that '&' is only used to reference
exising nodes. So I don't think the curly braces are necessary.

How about we do the following:

   1. deprecate &{/path/to/the/node/} - if it's not used anywhere, can just
   remove the code and not worry about deprecating it
   2. use the following to reference nodes:

&label   /* just refers to a labeled node */
&/path/to/the/node /* refers to a node by it's path */
&label/path/to/the/subnode /* refers to a node rooted at the label and
follows the path down */

Thoughts?

- John


On Mon, Oct 18, 2010 at 2:51 PM, Grant Likely <grant.likely at secretlab.ca>wrote:

> On Mon, Oct 18, 2010 at 02:08:33PM -0700, John Bonesio wrote:
> > On Mon, Oct 18, 2010 at 1:50 PM, Grant Likely <grant.likely at secretlab.ca
> >wrote:
> >
> > > On Mon, Oct 18, 2010 at 01:25:50PM -0700, John Bonesio wrote:
> > > > Changes to allow us to specify a node by it's path. A path can be
> used in
> > > > place of a label.
> > > >
> > > > This is particularly useful when overriding existing nodes.
> > > > This way we don't have to label every possible node in a device tree
> we
> > > know
> > > > is a base device tree for a class of systems, and we know the tree
> will
> > > be
> > > > modified for the specific systems.
> > > >
> > > > Signed-off-by: John Bonesio <bones at secretlab.ca>
> > > > ---
> > > >
> > > >  dtc-parser.y |   16 ++++++++++++----
> > > >  1 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > Need to add test cases.
> > >
> > > >
> > > > diff --git a/dtc-parser.y b/dtc-parser.y
> > > > index 56d7789..0efd0cc 100644
> > > > --- a/dtc-parser.y
> > > > +++ b/dtc-parser.y
> > > > @@ -134,22 +134,30 @@ devicetree:
> > > >               {
> > > >                       struct node *target;
> > > >
> > > > -                     target = get_node_by_label($1, $2);
> > > > +                     if ($2[0] == '/')
> > > > +                             target = get_node_by_path($1, $2);
> > > > +                     else
> > > > +                             target = get_node_by_label($1, $2);
> > > > +
> > > >                       if (target)
> > > >                               merge_nodes(target, $3);
> > > >                       else
> > > > -                             print_error("label, '%s' not found",
> $2);
> > > > +                             print_error("label or path, '%s', not
> > > found", $2);
> > > >                       $$ = $1;
> > > >               }
> > > >       | devicetree DT_REMOVENODE DT_REF ';'
> > > >               {
> > > >                       struct node *target;
> > > >
> > > > -                     target = get_node_by_label($1, $3);
> > > > +                     if ($3[0] == '/')
> > > > +                             target = get_node_by_path($1, $3);
> > > > +                     else
> > > > +                             target = get_node_by_label($1, $3);
> > > > +
> > > >                       if (target)
> > > >                               remove_nodes(target);
> > > >                       else
> > > > -                             print_error("label, '%s' not found",
> $3);
> > > > +                             print_error("label or path, '%s', not
> > > found", $3);
> > >
> > > Hmmm, this is effectively the same code block implemented twice.  Its
> > > gotten complex enough with this patch that the common parts should be
> > > factored out inot a common function...  In fact, after looking at the
> > > code, it looks like it should be calling get_node_by_ref() here.
> > >
> > > Also, this patch doesn't have any way to use the form
> > > &<label>/path/based/on/label which is also a critical feature.
> > >
> > >
> > Ok, so now I'm glad I separated this patch out.
> >
> > You're right in that it should be call get_node_by_ref(). I missed that.
> >
> > As for the rest of it, allow me to explain ...
> >
> > The lexer already parses &{/ <node path>} as a single lexical unit. When
> it
> > does this, it returns the token DT_REF, which is the same as for a label.
> > All I had to do is call the right routine to go find the right node.
>
> Right, I understand this.
>
> But I don't currently see how the code can handle the use case of a
> subnode from a label.  ie:
>
>        /{
>                my-label: parent {
>                        my-node {
>                        };
>                };
>        };
>
>        &my-label/my-node {
>                hi-there;
>        };
>
> The current code seems to be able to handle a full path, or a single
> label, but not a child node of a labelled node.
>
>
> >
> > I've run this through a sample dts file, and it does indeed work.
>
> (BTW, I think I gave you bad information earlier about syntax.  We
> have a syntax for full path, but we're missing a syntax for
> label+subpath), which you'll have to define.
>
> g.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/devicetree-discuss/attachments/20101018/07016616/attachment-0001.html>