Re: [PATCH v2] checks: Remove check for graph child addresses

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

On 2025-08-17 18:19:46 +1000, David Gibson wrote:
> On Sun, Aug 17, 2025 at 08:42:59AM +0200, Niklas Söderlund wrote:
> > Hi David,
> > 
> > Thanks for your feedback.
> > 
> > On 2025-08-16 14:45:58 +1000, David Gibson wrote:
> > > On Tue, Aug 12, 2025 at 06:51:22PM +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>
> > > > 	    };
> > > > 	};
> > > >     };
> > > > 
> > > > Remove the check as it is somewhat redundant now that we can use schemas
> > > > to validate the full node.
> > > > 
> > > > Signed-off-by: Niklas Söderlund
> > > > <niklas.soderlund+renesas@ragnatech.se>
> > > 
> > > This causes test suite failures.  You'll need to also remove the
> > > explicit graph_child_address test from the testsuite.
> > 
> > Thanks for pointing this out. I'm no expert on DTC, this is the 
> > references to it in tests/run_tests.sh? Is there any more locations I 
> > should check?
> 
> No, that's it.  "make check" should run the full suite for you; good
> practice before sending patches.  If you want you can use the github
> MR mechanism which will also run the tests as part of its CI.
> Old-school patches on the mailing list are fine too, though.

Thanks, indeed 'make check' catches the issue. I did not know about 
that, will fix for next version.

> 
> > > Note that this patch does technically cause an incompatible change:
> > > when I suggested removing this I hadn't realised this was a single
> > > check, rather than a piece of a larger check.  That means that anyone
> > > configuring this test in their scripts will be broken by removing it.
> > > This is probably an obscure enough case that I'm prepared to accept
> > > it, though.
> > > 
> > > > ---
> > > > Changes since v2
> > > > - Rebase after 6.17-rc1 merge window closed.
> > > > ---
> > > >  checks.c | 27 +--------------------------
> > > >  1 file changed, 1 insertion(+), 26 deletions(-)
> > > > 
> > > > diff --git a/checks.c b/checks.c
> > > > index 7e3fed5005b3..2072e1ea82dc 100644
> > > > --- a/checks.c
> > > > +++ b/checks.c
> > > > @@ -1894,31 +1894,6 @@ static void check_graph_endpoint(struct check *c, struct dt_info *dti,
> > > >  }
> > > >  WARNING(graph_endpoint, check_graph_endpoint, NULL, &graph_nodes);
> > > >  
> > > > -static void check_graph_child_address(struct check *c, struct dt_info *dti,
> > > > -				      struct node *node)
> > > > -{
> > > > -	int cnt = 0;
> > > > -	struct node *child;
> > > > -
> > > > -	if (node->bus != &graph_ports_bus && node->bus != &graph_port_bus)
> > > > -		return;
> > > > -
> > > > -	for_each_child(node, child) {
> > > > -		struct property *prop = get_property(child, "reg");
> > > > -
> > > > -		/* No error if we have any non-zero unit address */
> > > > -                if (prop && propval_cell(prop) != 0 )
> > > > -			return;
> > > > -
> > > > -		cnt++;
> > > > -	}
> > > > -
> > > > -	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);
> > > > -}
> > > > -WARNING(graph_child_address, check_graph_child_address, NULL, &graph_nodes, &graph_port, &graph_endpoint);
> > > > -
> > > >  static struct check *check_table[] = {
> > > >  	&duplicate_node_names, &duplicate_property_names,
> > > >  	&node_name_chars, &node_name_format, &property_name_chars,
> > > > @@ -2005,7 +1980,7 @@ static struct check *check_table[] = {
> > > >  
> > > >  	&alias_paths,
> > > >  
> > > > -	&graph_nodes, &graph_child_address, &graph_port, &graph_endpoint,
> > > > +	&graph_nodes, &graph_port, &graph_endpoint,
> > > >  
> > > >  	&always_fail,
> > > >  };
> > > 
> > 
> > 
> > 
> 
> -- 
> 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