Re: [RFC] Allow device tree to be modified by additonal device tree sections

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

On Tue, Feb 23, 2010 at 07:05:53PM -0700, Grant Likely wrote:
> On Tue, Feb 23, 2010 at 6:36 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Tue, Feb 23, 2010 at 12:28:13PM -0700, Grant Likely wrote:
[snip]
> >> @@ -83,7 +84,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
> >>  %%
> >>
> >>  sourcefile:
> >> -       DT_V1 ';' memreserves devicetree
> >> +       DT_V1 ';' memreserves devicetrees
> >>               {
> >>                       the_boot_info = build_boot_info($3, $4,
> >>                                                       guess_boot_cpuid($4));
> >> @@ -115,6 +116,17 @@ addr:
> >>               }
> >>         ;
> >>
> >> +devicetrees:
> >> +       /* empty */
> >
> > We always want at least one device tree block, so the base case here
> > should be 'devicetree', rather than empty.
> 
> Okay, I'll change this.  I'm unclear however, if a single 'devicetree'
> can get promoted up to a 'devicetrees', then will that cause problems
> with the 'devicetree devicetrees' rule because the stack will contain
> a 'devicetrees' when the rule wants a 'devicetree' first?

As we discussed on IRC, no.  bison is clever enough to figure out the
right reduction to make based on context and lookahead.

> >> +     printf("Merge node, old_node:%s new_node:%s\n", old_node->name,
> >> +             new_node ? new_node->name : "<NULL>" );
> >
> > There already exist some debug() macros in dtc.h for this sort of
> > message.
> 
> I hadn't meant to leave this in.  It's already gone.
> 
> >
> >> +     if (!new_node)
> >> +             return old_node;
> >
> > With the grammar change suggested above, you don't need this.
> 
> Cool.
> 
> >
> >> +     /* Move the override properties into the old node.  If there
> >> +      * is a collision, replace the old definition with the new */
> >> +     while (new_node->proplist) {
> >> +             /* Pop the property off the list */
> >> +             new_prop = new_node->proplist;
> >> +             new_node->proplist = new_prop->next;
> >> +             new_prop->next = NULL;
> >> +
> >> +             /* Look for a collision, set new value if there is */
> >> +             for_each_property(old_node, old_prop) {
> >> +                     if (strcmp(old_prop->name, new_prop->name) == 0) {
> >> +                             old_prop->val = new_prop->val;
> >> +                             free(new_prop);
> >> +                             new_prop = NULL;
> >> +                             break;
> >
> > I guess there should be a data_free() here of the old value.  Mind you
> > memory management in dtc is already a bit of a mess.  I've considered
> > moving it to talloc() (Tridge's nifty hierarchical pool allocator) or,
> > more controversially, just never bothering to free() anything on the
> > grounds that dtc processes are always shortlived anyway.
> 
> Okay, think about it and let me know what you want me to do here.

Sure.  I guess my point is that I'm not really worried about leaks,
since dtc is always short-lived.

[snip]
> >  However, there's also a bigger question here.  How precise do
> > we want the nodename matching to be.  Should we be using the OF-style
> > matchin where you can omit the unit address when it's unambiguous.
> >
> > i.e. Should we allow:
> >
> > / {
> >        ...
> >        somebus@1234 {
> >                widget@17 {
> >                };
> >        };
> > };
> >
> > / {
> >        ...
> >        somebus {
> >                widget {
> >                        new-property;
> >                };
> >        };
> > };
> >
> 
> My opinion: no way.  We're not working with real OF, and none of the
> use cases I see have any need of such a feature.  While we could
> forbid it now and permit it later, we cannot permit it now and forbid
> it later.  I say no until someone comes up with a reasonable use case
> on why it should be implemented.

A sound argument.  Done.

-- 
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