On Tue, Feb 23, 2010 at 6:36 PM, David Gibson <[email protected]> wrote: > On Tue, Feb 23, 2010 at 12:28:13PM -0700, Grant Likely wrote: >> This patch allows the following construct: >> >> / { >> property-a = "old"; >> property-b = "does not change"; >> }; >> >> / { >> property-a = "changed"; >> property-c = "new"; >> node-a { >> }; >> }; > > Heh. I'm glad I didn't far last night with my own implementation of > the concept.
:-) >> Where the later device tree overrides the properties found in the >> earlier tree. This is useful for laying down a template device tree >> in an include file and modifying it for a specific board without having >> to clone the entire tree. >> >> Signed-off-by: Grant Likely <[email protected]> >> --- >> >> I haven't extensively tested this patch yet, and I haven't figured out yet >> how to properly write the test cases, but I want to get this out there to >> make sure I'm taking the right approach. > > Ok, I have a test case from my start to this, which you can use. Your > dts files give a wider range of cases to check, but they'll also need > more test code to verify. > > To add testcases, you basically just list them in run_tests.sh. For > simple things, like just invoking dtc, or checking things for which > there is already a test program, it may be sufficient just to add the > right things into there. For more complex and specific testing you'll > need to write a testcase binary, which should use the macros in > tests.h to indicate success or failure. > > In this case, I think the simplest way to go is to write dts files > which use the merge functionality to create a tree identical to one of > the existing samples (like test_tree1.dts). Then the testcase > consists of two parts, one invoking dtc on the merge tree (to check > that it even processes it without error), then a second comparing the > output tree to the thing it's supposed to match. > > We won't be able to use the existing dtbs_equal_ordered, > unfortunately, because the merge behaviour could do odd things to the > order. Still, I've been meaning to implement a dtbs_equal_unordered > for ages. > >> diff --git a/dtc-parser.y b/dtc-parser.y >> index bd9e097..8f5c4a3 100644 >> --- a/dtc-parser.y >> +++ b/dtc-parser.y >> @@ -75,6 +75,7 @@ static unsigned long long eval_literal(const char *s, int >> base, int bits); >> %type <proplist> proplist >> >> %type <node> devicetree >> +%type <node> devicetrees >> %type <node> nodedef >> %type <node> subnode >> %type <nodelist> subnodes >> @@ -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? >> + 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. >> + } >> + } >> + >> + /* Assuming no collision, add the property to the old node. */ >> + if (new_prop) >> + add_property(old_node, new_prop); >> + } >> + >> + /* Move the override child nodes into the primary node. If >> + * there is a collision, then merge the nodes. */ >> + while (new_node->children) { >> + /* Pop the child node off the list */ >> + new_child = new_node->children; >> + new_node->children = new_child->next_sibling; >> + new_child->parent = NULL; >> + new_child->next_sibling = NULL; >> + >> + /* Search for a collision. Merge if there is */ >> + for_each_child(old_node, old_child) { >> + if (strcmp(old_child->name, new_child->name) >> == 0) { > > Ok, you should use the streq() macro instead of an explicit strcmp() > == 0. okay. > 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: > > / { > ... > some...@1234 { > wid...@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. g. _______________________________________________ devicetree-discuss mailing list [email protected] https://lists.ozlabs.org/listinfo/devicetree-discuss
