On Tue, Apr 05, 2016 at 11:34:33AM +0200, Nikolaus Schulz wrote: > On Tue, Apr 05, 2016 at 03:33:06PM +0900, Simon Horman wrote: > > On Thu, Mar 31, 2016 at 05:30:58PM +0200, Alban Bedel wrote: > > > From: Nikolaus Schulz <nikolaus.schulz at avionic-design.de> > > > > > > When adding a new node to the DT in the setup_dtb_prop function, the > > > parent offset need be passed to fdt_add_subnode. Currently a bogus > > > error code is used. Fix that by adding the parent offset as an extra > > > function argument to setup_dtb_prop, and change the handling of the > > > /chosen node to operate on a relative path plus (zero) offset instead of > > > an absolute path. > > > > > > Signed-off-by: Nikolaus Schulz <nikolaus.schulz at avionic-design.de> > > > > Hi, > > > > I am struggling to see the problem that this patch tries to solve. > > > > * Where and when is a bogus error code used? > > Hi Simon! > > The bogus error code is passed to fdt_add_subnode, see comment inline in > the patch below. Thanks, I see that clearly now. For some reason I inverted the "if (off == -FDT_ERR_NOTFOUND)" in my mind when I was reviewing your patch yesterday. > > * Why is a new parameter added to fdt_add_subnode and always passed as zero? > > Yeah, this might look irritating... the point is, setup_dtb_prop looks > like a generic function that could be used for finding/adding any nodes, > which need not be top-level in the DT. In practice though, the function > is only used for the top-level /chosen node, and currently it can't add > nodes that aren't top-level. > > So, one could either a) make the function non-generic, handling > top-level nodes only, or b) pass it the offset of the parent node, or c) > let it operate on aribtrary DT paths with possibly multiple components. > a) and b) are trivial to do. libfdt typically does b) for similar > functions, so I added that extra parameter to fdt_add_subnode. Thanks, I understand now. With the above explanation in mind I prefer b). Could you re-work the changelog to explain things as you have done so above and then resubmit the entire series? > > * If it is inappropriate to pass off to fdt_add_subnode() can > > that be resolved by simply passing 0 instead? > > If we know that we're adding a top-level node, then yes. Basically, > one can do that either in fdt_add_subnode, or when calling it in > zImage_arm_load. My patch does the latter. Understood. > Best regards, > Nikolaus
