[PATCH 1/3] arm: properly handle a missing /chosen node in the DT

[Simon Horman]

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