From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerry Van Baren Date: Thu, 17 May 2007 08:04:38 -0400 Subject: [U-Boot-Users] fdt_find_compatible_node() and friends In-Reply-To: <464C3D3A.5020409@grandegger.com> References: <4639F61F.2050506@grandegger.com> <4639FC57.8080403@smiths-aerospace.com> <463C80BB.5080305@grandegger.com> <464C30A8.3000002@gmail.com> <464C3D3A.5020409@grandegger.com> Message-ID: <464C44D6.7060002@smiths-aerospace.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Wolfgang Grandegger wrote: > Hi Jerry, > > Jerry Van Baren wrote: > [...] >> Hi Wolfgang G, >> >> I've applied your patches to my local (working) repository and will push >> the changes tonight (my tonight, your tomorrow ;-). I created a >> subroutine out of three snippets of code in cmd_fdt.c which your >> fdt_find_node_by_path() change fixed up so I had to fix one line in the >> new subroutine by hand. Not bad at all considering the changes I made >> in that file. > > Thanks. In the meantime I observed, that > > fdt_find_node_by_path(fdt, 0, "/"); > > returns an error. Is that by purpose? [snip] > Wolfgang. Hmm, interesting observation. The character '/' is a figment of our imagination, offset 0 in the tree is the root node. The character '/' is the path separator and doesn't actually exist anywhere in the fdt - when parsing paths, the stuff between the slashes is searched for and the slashes themselves are skipped over. What you asked in the above call is a node with no name under the root node, i.e. "//" in human-speak. That wasn't found, of course. On the other hand, it is a pretty obvious "mistake" (I was guilty of making the same mistake when I first tried to use fdt_path_offset()). It seems like I was forever doing a conditional: 59 if (strcmp(pathp, "/") == 0) { 60 nodeoffset = 0; 61 } else { 62 nodeoffset = fdt_path_offset (fdt, pathp); 63 if (nodeoffset < 0) { Trivia: Your patch we are discussing changed exactly this fdt_path_offset() into fdt_find_node_by_path() - this is the one place your patch didn't apply, because I refactored the three conditionals into a single wrapper subroutine. At the risk of being accused of codling our users, I would propose we add the equivalent "/" detection code (above) to fdt_find_node_by_path(), (I will do that tonight unless you beat me to it). It seems silly to have the caller replicate or wrap the conditional since it is going to be such a common idiom/mistake. Thanks, gvb