Re: [PATCH] checks: Document possible false warning for graph child addresses

Hi David,

Thanks for your comments.

On 2025-07-08 13:07:12 +1000, David Gibson wrote:
> On Sun, Jul 06, 2025 at 02:26:38PM +0200, Niklas Söderlund wrote:
> > The dtc graph_child_address check can't distinguish between bindings
> > where there can only be a single endpoint, and cases where there can be
> > multiple endpoints.
> > 
> > In cases where the bindings allow for multiple endpoints but only one is
> > described false warnings about unnecessary #address-cells/#size-cells
> > can be generated, but only if the endpoint described have an address of
> > 0 (A), for single endpoints with a non-zero address (B) no warnings are
> > generated.
> > 
> > A)
> >     ports {
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 
> > 	port@0 {
> > 	    #address-cells = <1>;
> > 	    #size-cells = <0>;
> > 
> > 	    sourceA: endpoint@0 {
> > 		reg = <0>
> > 	    };
> > 	};
> >     };
> > 
> > B)
> >     ports {
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 
> > 	port@0 {
> > 	    #address-cells = <1>;
> > 	    #size-cells = <0>;
> > 
> > 	    sourceB: endpoint@1 {
> > 		reg = <1>
> > 	    };
> > 	};
> >     };
> > 
> > Add a comment in the check to document this.
> 
> Hm.  I don't know the graph bindings at all well, so I'll take your
> word for it on what's happening here.  But simply documenting this
> within the code doesn't seem particularly useful.  Someone running dtc
> will still see the bogus error, and they'd have a pretty long way to
> go to find this explanation.

It would have been useful for me, I spent a lot of time questioning 
myself on why my dts files produced warnings and where incorrect. I even 
submitted patches to try and work around this issue before learning 
these where false positives. A comment here would have saved me that 
work :-)

I think if the check stays the comment bring some value.

> 
> Probably better to simply remove the check (and maybe comment that it
> would be nice to check further, but we can't adequately it from a
> valid case).

I'm OK with removing the check too. This comment was first posted 
together with a change to demote this check to W=2 (instead of W=1) that 
have now been posted separately [1]. I will wait for feedback on that 
and let smarter people then me pick the best way forward.

1.  https://lore.kernel.org/all/20250706123243.1050718-1-niklas.soderlund%[email protected]/

> 
> > 
> > Signed-off-by: Niklas Söderlund <[email protected]>
> > ---
> > Hello,
> > 
> > This was previously part of a patch posted to [email protected]
> > [1], but as Rob's points out in that thread it should have been posted
> > separately to [email protected]. Sorry for not
> > realising that dtc changes go to thru a separate tree.
> > 
> > 1.  https://lore.kernel.org/all/20250702085008.689727-1-niklas.soderlund%[email protected]/
> > ---
> >  checks.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/checks.c b/checks.c
> > index 123f2eb425f4..52d09fcf8d3a 100644
> > --- a/checks.c
> > +++ b/checks.c
> > @@ -1913,6 +1913,11 @@ static void check_graph_child_address(struct check *c, struct dt_info *dti,
> >  		cnt++;
> >  	}
> >  
> > +	/*
> > +	 * This check can produce false warnings if the bindings allow for more
> > +	 * then one endpoint in the node but only one is present and it has a
> > +	 * unit address of zero.
> > +	 */
> >  	if (cnt == 1 && node->addr_cells != -1)
> >  		FAIL(c, dti, node, "graph node has single child node '%s', #address-cells/#size-cells are not necessary",
> >  		     node->children->name);
> 
> -- 
> David Gibson (he or they)	| 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



-- 
Kind Regards,
Niklas Söderlund