Re: [RFC PATCH] soc: audio-graph-card2: use correct endpoint when getting link parameters

[Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>]

ping

On 20.12.24 г. 9:11 ч., Ivaylo Dimitrov wrote:
> We may have multiple links between ports, with each link
> having different parameters. Currently, no matter the topology,
> it is always port endpoint 0 that is used when setting parameters.
> 
> On a complex sound system, like the one found on Motorola droid4,
> hifi and voice DAIs require differents formats (i2s vs dsp_a)
> and curently it is impossible to use DT to set that.
>   
> Implementing the change leads to partially dropping of at least
> 0dedbde5062d (ASoC: cpcap: Implement set_tdm_slot for voice call
> support), as core does most of what is needed to configure voice DAI.
> 
> We (on Maemo Leste ) use the patch (along with few others) to have
> voice calls working properly on d4 through UCM.
> 
> The patch is for linux 6.6, I want to know whether the
> approach would be accepted before sending a proper patch for
> current master.
> 
> the original commit message follows:
> 
> When link parameters are parsed, it is always endpoint@0 that is used and
> parameters set to other endpoints are ignored.
> 
> Fix that by using endpoint that is set in DT when parsing link parameters.
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>   sound/soc/generic/audio-graph-card2.c | 59 +++++++++++++--------------
>   1 file changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c
> index b1c675c6b6db..163a20c8ffee 100644
> --- a/sound/soc/generic/audio-graph-card2.c
> +++ b/sound/soc/generic/audio-graph-card2.c
> @@ -508,17 +508,16 @@ static int __graph_parse_node(struct asoc_simple_priv *priv,
>   
>   static int graph_parse_node(struct asoc_simple_priv *priv,
>   			    enum graph_type gtype,
> -			    struct device_node *port,
> +			    struct device_node *ep,
>   			    struct link_info *li, int is_cpu)
>   {
> -	struct device_node *ep;
>   	int ret = 0;
> +	struct device_node *port = of_get_parent(ep);
> +	bool is_multi = graph_lnk_is_multi(port);
>   
> -	if (graph_lnk_is_multi(port)) {
> +	if (is_multi) {
>   		int idx;
>   
> -		of_node_get(port);
> -
>   		for (idx = 0;; idx++) {
>   			ep = graph_get_next_multi_ep(&port);
>   			if (!ep)
> @@ -532,9 +531,8 @@ static int graph_parse_node(struct asoc_simple_priv *priv,
>   		}
>   	} else {
>   		/* Single CPU / Codec */
> -		ep = port_to_endpoint(port);
> +		of_node_put(port);
>   		ret = __graph_parse_node(priv, gtype, ep, li, is_cpu, 0);
> -		of_node_put(ep);
>   	}
>   
>   	return ret;
> @@ -591,22 +589,20 @@ static void graph_parse_daifmt(struct device_node *node,
>   }
>   
>   static void graph_link_init(struct asoc_simple_priv *priv,
> -			    struct device_node *port,
> +			    struct device_node *ep,
>   			    struct link_info *li,
>   			    int is_cpu_node)
>   {
>   	struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, li->link);
> -	struct device_node *ep;
> +	struct device_node *port = of_get_parent(ep);
> +	bool is_multi = graph_lnk_is_multi(port);
>   	struct device_node *ports;
>   	unsigned int daifmt = 0, daiclk = 0;
>   	unsigned int bit_frame = 0;
>   
> -	if (graph_lnk_is_multi(port)) {
> -		of_node_get(port);
> +	if (is_multi) {
>   		ep = graph_get_next_multi_ep(&port);
>   		port = of_get_parent(ep);
> -	} else {
> -		ep = port_to_endpoint(port);
>   	}
>   
>   	ports = of_get_parent(port);
> @@ -642,6 +638,9 @@ static void graph_link_init(struct asoc_simple_priv *priv,
>   	dai_link->ops		= &graph_ops;
>   	if (priv->ops)
>   		dai_link->ops	= priv->ops;
> +
> +	of_node_put(port);
> +	of_node_put(ports);
>   }
>   
>   int audio_graph2_link_normal(struct asoc_simple_priv *priv,
> @@ -650,7 +649,7 @@ int audio_graph2_link_normal(struct asoc_simple_priv *priv,
>   {
>   	struct device_node *cpu_port = lnk;
>   	struct device_node *cpu_ep = port_to_endpoint(cpu_port);
> -	struct device_node *codec_port = of_graph_get_remote_port(cpu_ep);
> +	struct device_node *codec_ep = of_graph_get_remote_endpoint(cpu_ep);
>   	int ret;
>   
>   	/*
> @@ -658,20 +657,20 @@ int audio_graph2_link_normal(struct asoc_simple_priv *priv,
>   	 * see
>   	 *	__graph_parse_node() :: DAI Naming
>   	 */
> -	ret = graph_parse_node(priv, GRAPH_NORMAL, codec_port, li, 0);
> +	ret = graph_parse_node(priv, GRAPH_NORMAL, codec_ep, li, 0);
>   	if (ret < 0)
>   		goto err;
>   
>   	/*
>   	 * call CPU, and set DAI Name
>   	 */
> -	ret = graph_parse_node(priv, GRAPH_NORMAL, cpu_port, li, 1);
> +	ret = graph_parse_node(priv, GRAPH_NORMAL, cpu_ep, li, 1);
>   	if (ret < 0)
>   		goto err;
>   
> -	graph_link_init(priv, cpu_port, li, 1);
> +	graph_link_init(priv, cpu_ep, li, 1);
>   err:
> -	of_node_put(codec_port);
> +	of_node_put(codec_ep);
>   	of_node_put(cpu_ep);
>   
>   	return ret;
> @@ -684,7 +683,6 @@ int audio_graph2_link_dpcm(struct asoc_simple_priv *priv,
>   {
>   	struct device_node *ep = port_to_endpoint(lnk);
>   	struct device_node *rep = of_graph_get_remote_endpoint(ep);
> -	struct device_node *rport = of_graph_get_remote_port(ep);
>   	struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, li->link);
>   	struct simple_dai_props *dai_props = simple_priv_to_props(priv, li->link);
>   	int is_cpu = asoc_graph_is_ports0(lnk);
> @@ -718,7 +716,7 @@ int audio_graph2_link_dpcm(struct asoc_simple_priv *priv,
>   		dai_link->dynamic		= 1;
>   		dai_link->dpcm_merged_format	= 1;
>   
> -		ret = graph_parse_node(priv, GRAPH_DPCM, rport, li, 1);
> +		ret = graph_parse_node(priv, GRAPH_DPCM, rep, li, 1);
>   		if (ret)
>   			goto err;
>   	} else {
> @@ -751,7 +749,7 @@ int audio_graph2_link_dpcm(struct asoc_simple_priv *priv,
>   		dai_link->no_pcm		= 1;
>   		dai_link->be_hw_params_fixup	= asoc_simple_be_hw_params_fixup;
>   
> -		ret = graph_parse_node(priv, GRAPH_DPCM, rport, li, 0);
> +		ret = graph_parse_node(priv, GRAPH_DPCM, rep, li, 0);
>   		if (ret < 0)
>   			goto err;
>   	}
> @@ -761,11 +759,10 @@ int audio_graph2_link_dpcm(struct asoc_simple_priv *priv,
>   
>   	snd_soc_dai_link_set_capabilities(dai_link);
>   
> -	graph_link_init(priv, rport, li, is_cpu);
> +	graph_link_init(priv, rep, li, is_cpu);
>   err:
>   	of_node_put(ep);
>   	of_node_put(rep);
> -	of_node_put(rport);
>   
>   	return ret;
>   }
> @@ -777,7 +774,7 @@ int audio_graph2_link_c2c(struct asoc_simple_priv *priv,
>   {
>   	struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, li->link);
>   	struct device_node *port0, *port1, *ports;
> -	struct device_node *codec0_port, *codec1_port;
> +	struct device_node *codec0_ep, *codec1_ep;
>   	struct device_node *ep0, *ep1;
>   	u32 val = 0;
>   	int ret = -EINVAL;
> @@ -834,31 +831,31 @@ int audio_graph2_link_c2c(struct asoc_simple_priv *priv,
>   	ep0 = port_to_endpoint(port0);
>   	ep1 = port_to_endpoint(port1);
>   
> -	codec0_port = of_graph_get_remote_port(ep0);
> -	codec1_port = of_graph_get_remote_port(ep1);
> +	codec0_ep = of_graph_get_remote_endpoint(ep0);
> +	codec1_ep = of_graph_get_remote_endpoint(ep1);
>   
>   	/*
>   	 * call Codec first.
>   	 * see
>   	 *	__graph_parse_node() :: DAI Naming
>   	 */
> -	ret = graph_parse_node(priv, GRAPH_C2C, codec1_port, li, 0);
> +	ret = graph_parse_node(priv, GRAPH_C2C, codec1_ep, li, 0);
>   	if (ret < 0)
>   		goto err2;
>   
>   	/*
>   	 * call CPU, and set DAI Name
>   	 */
> -	ret = graph_parse_node(priv, GRAPH_C2C, codec0_port, li, 1);
> +	ret = graph_parse_node(priv, GRAPH_C2C, codec0_ep, li, 1);
>   	if (ret < 0)
>   		goto err2;
>   
> -	graph_link_init(priv, codec0_port, li, 1);
> +	graph_link_init(priv, codec0_ep, li, 1);
>   err2:
>   	of_node_put(ep0);
>   	of_node_put(ep1);
> -	of_node_put(codec0_port);
> -	of_node_put(codec1_port);
> +	of_node_put(codec0_ep);
> +	of_node_put(codec1_ep);
>   err1:
>   	of_node_put(ports);
>   	of_node_put(port0);