| Message ID | 87h6gludmj.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: Replace dpcm_playback/capture to playback/capture_only | expand |
On 3/31/24 19:32, Kuninori Morimoto wrote: > snd_soc_dai_link_set_capabilities() checks all CPU/Codec DAI (Y)(Z) > for Playback/Capture (X) and checks its validation (A), and setup > dpcm_playback/capture flags (a). > > void snd_soc_dai_link_set_capabilities(...) > { > ... > (X) for_each_pcm_streams(direction) { > ... > (Y) for_each_link_cpus(dai_link, i, cpu) { > ... > (A) if (... snd_soc_dai_stream_valid(...)) { > ... > } > } > (Z) for_each_link_codecs(dai_link, i, codec) { > ... > (A) if (... snd_soc_dai_stream_valid(...)) { > ... > } > } > ... > } > > (a) dai_link->dpcm_playback = supported[...]; > (a) dai_link->dpcm_capture = supported[...]; > } > > This validation check will be automatically done on new > soc_get_playback_capture(). snd_soc_dai_link_set_capabilities() is no > longer needed. Let's remove it. Humm, this is really hard to review. soc_get_playback_capture() used to do a verification of the match between dailink and dais, and now it doesn't have it any longer and this patch removes the checks?
Hi Pierre-Louis > > snd_soc_dai_link_set_capabilities() checks all CPU/Codec DAI (Y)(Z) > > for Playback/Capture (X) and checks its validation (A), and setup > > dpcm_playback/capture flags (a). > > > > void snd_soc_dai_link_set_capabilities(...) > > { > > ... > > (X) for_each_pcm_streams(direction) { > > ... > > (Y) for_each_link_cpus(dai_link, i, cpu) { > > ... > > (A) if (... snd_soc_dai_stream_valid(...)) { > > ... > > } > > } > > (Z) for_each_link_codecs(dai_link, i, codec) { > > ... > > (A) if (... snd_soc_dai_stream_valid(...)) { > > ... > > } > > } > > ... > > } > > > > (a) dai_link->dpcm_playback = supported[...]; > > (a) dai_link->dpcm_capture = supported[...]; > > } > > > > This validation check will be automatically done on new > > soc_get_playback_capture(). snd_soc_dai_link_set_capabilities() is no > > longer needed. Let's remove it. > > Humm, this is really hard to review. > > soc_get_playback_capture() used to do a verification of the match > between dailink and dais, and now it doesn't have it any longer and this > patch removes the checks? Hmm..., Maybe I'm misunderstanding ? I think this patch is very clear to remove, because it is 100% duplicate code. Maybe this mutual misunderstanding is based [01/15] review ? I think we need to dig it first. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
On 4/1/24 19:29, Kuninori Morimoto wrote: > > Hi Pierre-Louis > >>> snd_soc_dai_link_set_capabilities() checks all CPU/Codec DAI (Y)(Z) >>> for Playback/Capture (X) and checks its validation (A), and setup >>> dpcm_playback/capture flags (a). >>> >>> void snd_soc_dai_link_set_capabilities(...) >>> { >>> ... >>> (X) for_each_pcm_streams(direction) { >>> ... >>> (Y) for_each_link_cpus(dai_link, i, cpu) { >>> ... >>> (A) if (... snd_soc_dai_stream_valid(...)) { >>> ... >>> } >>> } >>> (Z) for_each_link_codecs(dai_link, i, codec) { >>> ... >>> (A) if (... snd_soc_dai_stream_valid(...)) { >>> ... >>> } >>> } >>> ... >>> } >>> >>> (a) dai_link->dpcm_playback = supported[...]; >>> (a) dai_link->dpcm_capture = supported[...]; >>> } >>> >>> This validation check will be automatically done on new >>> soc_get_playback_capture(). snd_soc_dai_link_set_capabilities() is no >>> longer needed. Let's remove it. >> >> Humm, this is really hard to review. >> >> soc_get_playback_capture() used to do a verification of the match >> between dailink and dais, and now it doesn't have it any longer and this >> patch removes the checks? > > Hmm..., Maybe I'm misunderstanding ? > I think this patch is very clear to remove, because it is 100% duplicate > code. Maybe this mutual misunderstanding is based [01/15] review ? > I think we need to dig it first. I agree this looks like duplicate code, but why can't we remove it first *before* any code modification? It's very hard to review because it comes as the 13th patch of a series and you've already removed similar code earlier which precisely checked the consistency between dailink and dais. In this function, it's a similar case btw where the settings provided by the machine drivers are overridden by the framework, so that's another case of collision between machine driver and framework. Which of the two should be trusted?
Hi Pierre-Louis Thank you for your feedback. > >>> void snd_soc_dai_link_set_capabilities(...) > >>> { > >>> ... > >>> (X) for_each_pcm_streams(direction) { > >>> ... > >>> (Y) for_each_link_cpus(dai_link, i, cpu) { > >>> ... > >>> (A) if (... snd_soc_dai_stream_valid(...)) { > >>> ... > >>> } > >>> } > >>> (Z) for_each_link_codecs(dai_link, i, codec) { > >>> ... > >>> (A) if (... snd_soc_dai_stream_valid(...)) { > >>> ... > >>> } > >>> } > >>> ... > >>> } > >>> > >>> (a) dai_link->dpcm_playback = supported[...]; > >>> (a) dai_link->dpcm_capture = supported[...]; > >>> } (snip) > It's very hard to review because it comes as the 13th patch of a series > and you've already removed similar code earlier which precisely checked > the consistency between dailink and dais. Ah, OK, I see. Indeed this patch can be merged into [01/16] patch, or can be [02/16]. > In this function, it's a similar case btw where the settings provided by > the machine drivers are overridden by the framework, so that's another > case of collision between machine driver and framework. Which of the two > should be trusted? I couldn't understand this comment, either. In this function, it's a similar case btw where the settings provided by the machine drivers are overridden by the framework, Do you mean dai_link->dpcm_xxx which was set by machine drivers is overridden/overwritten by this function (= snd_soc_dai_link_set_capabilities()) ?? I think CPU/Codec driver can't set dai_link. And this function is basically called from Card driver, not from framework. And dpcm_xxx is no longer needed anyway, no collision happen any more by this patch. But am I misunderstanding ? Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index adcd8719d343..69ba1a628eab 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -219,7 +219,6 @@ void snd_soc_dai_resume(struct snd_soc_dai *dai); int snd_soc_dai_compress_new(struct snd_soc_dai *dai, struct snd_soc_pcm_runtime *rtd, int num); bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream); -void snd_soc_dai_link_set_capabilities(struct snd_soc_dai_link *dai_link); void snd_soc_dai_action(struct snd_soc_dai *dai, int stream, int action); static inline void snd_soc_dai_activate(struct snd_soc_dai *dai, diff --git a/sound/soc/fsl/imx-card.c b/sound/soc/fsl/imx-card.c index 5b7bdc5d6784..72e90e56d59a 100644 --- a/sound/soc/fsl/imx-card.c +++ b/sound/soc/fsl/imx-card.c @@ -650,9 +650,6 @@ static int imx_card_parse_of(struct imx_card_data *data) link->ops = &imx_aif_ops; } - if (link->no_pcm || link->dynamic) - snd_soc_dai_link_set_capabilities(link); - /* Get dai fmt */ ret = simple_util_parse_daifmt(dev, np, codec, NULL, &link->dai_fmt); diff --git a/sound/soc/generic/audio-graph-card.c b/sound/soc/generic/audio-graph-card.c index 83e3ba773fbd..714ce1f4a061 100644 --- a/sound/soc/generic/audio-graph-card.c +++ b/sound/soc/generic/audio-graph-card.c @@ -246,8 +246,6 @@ static int graph_dai_link_of_dpcm(struct simple_util_priv *priv, graph_parse_convert(dev, ep, &dai_props->adata); - snd_soc_dai_link_set_capabilities(dai_link); - ret = graph_link_init(priv, cpu_ep, codec_ep, li, dai_name); li->link++; diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c index 62606e20be9a..0d2ac4c9ba3d 100644 --- a/sound/soc/generic/audio-graph-card2.c +++ b/sound/soc/generic/audio-graph-card2.c @@ -925,8 +925,6 @@ int audio_graph2_link_dpcm(struct simple_util_priv *priv, graph_parse_convert(ep, dai_props); /* at node of <dpcm> */ graph_parse_convert(rep, dai_props); /* at node of <CPU/Codec> */ - snd_soc_dai_link_set_capabilities(dai_link); - graph_link_init(priv, rport, li, is_cpu); err: of_node_put(ep); diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 9c79ff6a568f..5e66812ffadf 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -276,8 +276,6 @@ static int simple_dai_link_of_dpcm(struct simple_util_priv *priv, simple_parse_convert(dev, np, &dai_props->adata); - snd_soc_dai_link_set_capabilities(dai_link); - ret = simple_link_init(priv, node, codec, li, prefix, dai_name); out_put_node: diff --git a/sound/soc/meson/axg-card.c b/sound/soc/meson/axg-card.c index 21bf1453af43..0ff7dcabd314 100644 --- a/sound/soc/meson/axg-card.c +++ b/sound/soc/meson/axg-card.c @@ -338,7 +338,6 @@ static int axg_card_add_link(struct snd_soc_card *card, struct device_node *np, dai_link->num_c2c_params = 1; } else { dai_link->no_pcm = 1; - snd_soc_dai_link_set_capabilities(dai_link); if (axg_card_cpu_is_tdm_iface(dai_link->cpus->of_node)) ret = axg_card_parse_tdm(card, np, index); } diff --git a/sound/soc/meson/gx-card.c b/sound/soc/meson/gx-card.c index f1539e542638..7edca3e49c8f 100644 --- a/sound/soc/meson/gx-card.c +++ b/sound/soc/meson/gx-card.c @@ -107,7 +107,6 @@ static int gx_card_add_link(struct snd_soc_card *card, struct device_node *np, dai_link->num_c2c_params = 1; } else { dai_link->no_pcm = 1; - snd_soc_dai_link_set_capabilities(dai_link); /* Check if the cpu is the i2s encoder and parse i2s data */ if (gx_card_cpu_identify(dai_link->cpus, "I2S Encoder")) ret = gx_card_parse_i2s(card, np, index); diff --git a/sound/soc/qcom/common.c b/sound/soc/qcom/common.c index 747041fa7866..24862002e82b 100644 --- a/sound/soc/qcom/common.c +++ b/sound/soc/qcom/common.c @@ -145,7 +145,6 @@ int qcom_snd_parse_of(struct snd_soc_card *card) if (platform || !codec) { /* DPCM */ - snd_soc_dai_link_set_capabilities(link); link->ignore_suspend = 1; link->nonatomic = 1; } diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c index fefe394dce72..f8e46bec6f80 100644 --- a/sound/soc/soc-dai.c +++ b/sound/soc/soc-dai.c @@ -479,44 +479,6 @@ bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int dir) return stream->channels_min; } -/* - * snd_soc_dai_link_set_capabilities() - set dai_link properties based on its DAIs - */ -void snd_soc_dai_link_set_capabilities(struct snd_soc_dai_link *dai_link) -{ - bool supported[SNDRV_PCM_STREAM_LAST + 1]; - int direction; - - for_each_pcm_streams(direction) { - struct snd_soc_dai_link_component *cpu; - struct snd_soc_dai_link_component *codec; - struct snd_soc_dai *dai; - bool supported_cpu = false; - bool supported_codec = false; - int i; - - for_each_link_cpus(dai_link, i, cpu) { - dai = snd_soc_find_dai_with_mutex(cpu); - if (dai && snd_soc_dai_stream_valid(dai, direction)) { - supported_cpu = true; - break; - } - } - for_each_link_codecs(dai_link, i, codec) { - dai = snd_soc_find_dai_with_mutex(codec); - if (dai && snd_soc_dai_stream_valid(dai, direction)) { - supported_codec = true; - break; - } - } - supported[direction] = supported_cpu && supported_codec; - } - - dai_link->dpcm_playback = supported[SNDRV_PCM_STREAM_PLAYBACK]; - dai_link->dpcm_capture = supported[SNDRV_PCM_STREAM_CAPTURE]; -} -EXPORT_SYMBOL_GPL(snd_soc_dai_link_set_capabilities); - void snd_soc_dai_action(struct snd_soc_dai *dai, int stream, int action) {
snd_soc_dai_link_set_capabilities() checks all CPU/Codec DAI (Y)(Z) for Playback/Capture (X) and checks its validation (A), and setup dpcm_playback/capture flags (a). void snd_soc_dai_link_set_capabilities(...) { ... (X) for_each_pcm_streams(direction) { ... (Y) for_each_link_cpus(dai_link, i, cpu) { ... (A) if (... snd_soc_dai_stream_valid(...)) { ... } } (Z) for_each_link_codecs(dai_link, i, codec) { ... (A) if (... snd_soc_dai_stream_valid(...)) { ... } } ... } (a) dai_link->dpcm_playback = supported[...]; (a) dai_link->dpcm_capture = supported[...]; } This validation check will be automatically done on new soc_get_playback_capture(). snd_soc_dai_link_set_capabilities() is no longer needed. Let's remove it. Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- include/sound/soc-dai.h | 1 - sound/soc/fsl/imx-card.c | 3 --- sound/soc/generic/audio-graph-card.c | 2 -- sound/soc/generic/audio-graph-card2.c | 2 -- sound/soc/generic/simple-card.c | 2 -- sound/soc/meson/axg-card.c | 1 - sound/soc/meson/gx-card.c | 1 - sound/soc/qcom/common.c | 1 - sound/soc/soc-dai.c | 38 --------------------------- 9 files changed, 51 deletions(-)