Re: [PATCH v5 13/14] ASoC: rsnd: Support unprefixed DT node names for RZ/G3E

[John Madieu]

On Fri, Apr 17, 2026 at 03:44:41AM +0000, Kuninori Morimoto wrote:
> 
> Hi John

Hi Kuninori,

Thank you for the review.

> 
> Thank you for your patch
> 
> > ---
> (snip)
> > +struct device_node *rsnd_parse_of_node(struct rsnd_priv *priv, const char *name)
> > +{
> > +	struct device_node *np = rsnd_priv_to_dev(priv)->of_node;
> > +	struct device_node *node;
> > +	const char *unprefixed;
> > +
> > +	node = of_get_child_by_name(np, name);
> > +	if (node)
> > +		return node;
> > +
> > +	/*
> > +	 * RZ/G3E binding uses unprefixed node names (e.g. "ssi" instead
> > +	 * of "rcar_sound,ssi"). Try stripping the "rcar_sound," prefix.
> > +	 */
> > +	unprefixed = strchr(name, ',');
> > +	if (unprefixed)
> > +		node = of_get_child_by_name(np, unprefixed + 1);
> > +
> > +	return node;
> > +}
> 
> I think it is better to have name get function, and use it on parse func ?
> 
> 	char *rsnd_xx_name(node, name)
> 	{
> 		char *sub_name;
> 
> 		/* name = "rcar_sound,ssi" */
> 		ret = of_node_name_eq(node, name);
> 		if (ret == 0)
> 			return name;
> 
> 		/* sub_name = "ssi" */
> 		sub_name = strchr(name, ",");
> 		ret = of_node_name_eq(node, sub_name);
> 		if (ret == 0)
> 			return sub_name;
> 
> 		return NULL;
> 	}
>

I agree that having the "try prefixed, fall back to unprefixed" rule
spelled out at multiple call sites is a consistency problem, and I'll
fix that in v6.

What I think keeps consistency, and it is to factor out
just the string operation, and have both sites build on it:

    /* "rcar_sound,ssi" -> "ssi"; "ssi" -> NULL */
    static const char *rsnd_node_name_strip_prefix(const char *name)
    {
        const char *comma = strchr(name, ',');

        return comma ? comma + 1 : NULL;
    }

Then rsnd_parse_of_node() uses it in its fallback path:

    struct device_node *rsnd_parse_of_node(struct rsnd_priv *priv,
                                           const char *name)
    {
        struct device_node *np = rsnd_priv_to_dev(priv)->of_node;
        struct device_node *node;
        const char *unprefixed;

        node = of_get_child_by_name(np, name);
        if (node)
            return node;

        unprefixed = rsnd_node_name_strip_prefix(name);
        if (unprefixed)
            node = of_get_child_by_name(np, unprefixed);

        return node;
    }
 
> 
> > @@ -1273,7 +1294,8 @@ static int rsnd_dai_of_node(struct rsnd_priv *priv, int *is_graph)
> >  	of_node_put(node);
> >  
> >  	for_each_child_of_node_scoped(np, node) {
> > -		if (!of_node_name_eq(node, RSND_NODE_DAI))
> > +		if (!of_node_name_eq(node, RSND_NODE_DAI) &&
> > +		    !of_node_name_eq(node, "dai"))
> >  			continue;
> 
> If driver is handling almost same things individually and/or randomly in per
> each places, it will eventually lose consistency.
> 
> rsnd_xx_name() can keep consistency ?
> 

and rsnd_dai_of_node() uses the same helper instead of an open-coded
"dai" literal:

    const char *alt = rsnd_node_name_strip_prefix(RSND_NODE_DAI);

    for_each_child_of_node_scoped(np, node) {
        if (!of_node_name_eq(node, RSND_NODE_DAI) &&
            (!alt || !of_node_name_eq(node, alt)))
            continue;
        ...
    }

This way the "rcar_sound," prefix convention lives in exactly one
place, and each call site keeps its natural operation (fetch vs.
compare) without redundant lookups.

Does this work for you, or would you still prefer the node-based
getter?

Regards,
--
John Madieu