[PATCH 0/5] ASoC: minor cleanup for soc_get_playback_capture()
Hi Mark
This is minor cleanup patches for soc_get_playback_capture().
Link: https://lore.kernel.org/r/87ttw1gqgn.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87353uqjiu.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.c...
Kuninori Morimoto (5): ASoC: soc-pcm.c: indicate error if stream has no playback no capture ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture() ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture() ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture()
sound/soc/soc-pcm.c | 58 ++++++++++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 25 deletions(-)
soc_get_playback_capture() (A) returns number of substreams for playback/capture (B). ASoC will probe the Sound Card and mapps CPU<->Codec pair.
(A) static int soc_get_playback_capture(..., (B) int *playback, int *capture) { ... if (rtd->dai_link->playback_only) { *playback = 1; *capture = 0; }
if (rtd->dai_link->capture_only) { *playback = 0; *capture = 1; } (C) return 0; }
But it might be no playback no capture if it returns playback=0, capture=0. It is very difficult to notice about it. This patch indicates error at (C) then.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/soc-pcm.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 7247f44faa1c..fe65994485f8 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2815,6 +2815,13 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, *capture = 1; }
+ if (!*playback && !*capture) { + dev_err(rtd->dev, "substream %s has no playback, no capture\n", + rtd->dai_link->stream_name); + + return -EINVAL; + } + return 0; }
soc_get_playback_capture() (A) is using rtd->dai_link->xxx everywhere. Because of that, 1 line is unnecessarily long and not readable.
(A) static int soc_get_playback_capture(...) { if (rtd->dai_link->dynamic ...) { ^^^^^^^^^^^^^ ... } else { int cpu_capture = rtd->dai_link->c2c_params ? ^^^^^^^^^^^^^ ... }
if (rtd->dai_link->playback_only) { ^^^^^^^^^^^^^ ... } ... }
This patch uses variable "dai_link" to be clear code. Nothing changes the meanings.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/soc-pcm.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index fe65994485f8..db3fbe1af2ce 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2731,19 +2731,20 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream) static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, int *playback, int *capture) { + struct snd_soc_dai_link *dai_link = rtd->dai_link; struct snd_soc_dai *cpu_dai; int i;
- if (rtd->dai_link->dynamic && rtd->dai_link->num_cpus > 1) { + if (dai_link->dynamic && dai_link->num_cpus > 1) { dev_err(rtd->dev, "DPCM doesn't support Multi CPU for Front-Ends yet\n"); return -EINVAL; }
- if (rtd->dai_link->dynamic || rtd->dai_link->no_pcm) { + if (dai_link->dynamic || dai_link->no_pcm) { int stream;
- if (rtd->dai_link->dpcm_playback) { + if (dai_link->dpcm_playback) { stream = SNDRV_PCM_STREAM_PLAYBACK;
for_each_rtd_cpu_dais(rtd, i, cpu_dai) { @@ -2755,11 +2756,11 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, if (!*playback) { dev_err(rtd->card->dev, "No CPU DAIs support playback for stream %s\n", - rtd->dai_link->stream_name); + dai_link->stream_name); return -EINVAL; } } - if (rtd->dai_link->dpcm_capture) { + if (dai_link->dpcm_capture) { stream = SNDRV_PCM_STREAM_CAPTURE;
for_each_rtd_cpu_dais(rtd, i, cpu_dai) { @@ -2772,7 +2773,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, if (!*capture) { dev_err(rtd->card->dev, "No CPU DAIs support capture for stream %s\n", - rtd->dai_link->stream_name); + dai_link->stream_name); return -EINVAL; } } @@ -2780,15 +2781,15 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, struct snd_soc_dai *codec_dai;
/* Adapt stream for codec2codec links */ - int cpu_capture = rtd->dai_link->c2c_params ? + int cpu_capture = dai_link->c2c_params ? SNDRV_PCM_STREAM_PLAYBACK : SNDRV_PCM_STREAM_CAPTURE; - int cpu_playback = rtd->dai_link->c2c_params ? + int cpu_playback = dai_link->c2c_params ? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
for_each_rtd_codec_dais(rtd, i, codec_dai) { - if (rtd->dai_link->num_cpus == 1) { + if (dai_link->num_cpus == 1) { cpu_dai = asoc_rtd_to_cpu(rtd, 0); - } else if (rtd->dai_link->num_cpus == rtd->dai_link->num_codecs) { + } else if (dai_link->num_cpus == dai_link->num_codecs) { cpu_dai = asoc_rtd_to_cpu(rtd, i); } else { dev_err(rtd->card->dev, @@ -2805,19 +2806,19 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, } }
- if (rtd->dai_link->playback_only) { + if (dai_link->playback_only) { *playback = 1; *capture = 0; }
- if (rtd->dai_link->capture_only) { + if (dai_link->capture_only) { *playback = 0; *capture = 1; }
if (!*playback && !*capture) { dev_err(rtd->dev, "substream %s has no playback, no capture\n", - rtd->dai_link->stream_name); + dai_link->stream_name);
return -EINVAL; }
soc_get_playback_capture() (A) checks dai_link status, and indicate error if it was not matching (B).
(A) static int soc_get_playback_capture(...) { ... ^ if (dai_link->dynamic && dai_link->num_cpus > 1) { | dev_err(rtd->dev, (B) "DPCM doesn't support Multi CPU for Front-Ends yet\n"); | return -EINVAL; v } ... }
We can use 100 char for 1 line today. This patch cleanup error code line.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/soc-pcm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index db3fbe1af2ce..47da3be0ff46 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2736,8 +2736,7 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, int i;
if (dai_link->dynamic && dai_link->num_cpus > 1) { - dev_err(rtd->dev, - "DPCM doesn't support Multi CPU for Front-Ends yet\n"); + dev_err(rtd->dev, "DPCM doesn't support Multi CPU for Front-Ends yet\n"); return -EINVAL; }
soc_get_playback_capture() (A) returns number of substreams for playback/capture (B).
(A) static int soc_get_playback_capture(..., (B) int *playback, int *capture) { ... for_each_xxx(...) { if (xxx) return -EINVAL; => *playback = 1; ... => *capture = 1; ... } ... }
But, it is directly updating playback/capture which is the result of this function even though it might be error. It should be updated in case of succeed only. This patch updates it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 47da3be0ff46..b3d569e7ba61 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2733,6 +2733,8 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, { struct snd_soc_dai_link *dai_link = rtd->dai_link; struct snd_soc_dai *cpu_dai; + int has_playback = 0; + int has_capture = 0; int i;
if (dai_link->dynamic && dai_link->num_cpus > 1) { @@ -2748,11 +2750,11 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
for_each_rtd_cpu_dais(rtd, i, cpu_dai) { if (snd_soc_dai_stream_valid(cpu_dai, stream)) { - *playback = 1; + has_playback = 1; break; } } - if (!*playback) { + if (!has_playback) { dev_err(rtd->card->dev, "No CPU DAIs support playback for stream %s\n", dai_link->stream_name); @@ -2764,12 +2766,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
for_each_rtd_cpu_dais(rtd, i, cpu_dai) { if (snd_soc_dai_stream_valid(cpu_dai, stream)) { - *capture = 1; + has_capture = 1; break; } }
- if (!*capture) { + if (!has_capture) { dev_err(rtd->card->dev, "No CPU DAIs support capture for stream %s\n", dai_link->stream_name); @@ -2798,30 +2800,33 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && snd_soc_dai_stream_valid(cpu_dai, cpu_playback)) - *playback = 1; + has_playback = 1; if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && snd_soc_dai_stream_valid(cpu_dai, cpu_capture)) - *capture = 1; + has_capture = 1; } }
if (dai_link->playback_only) { - *playback = 1; - *capture = 0; + has_playback = 1; + has_capture = 0; }
if (dai_link->capture_only) { - *playback = 0; - *capture = 1; + has_playback = 0; + has_capture = 1; }
- if (!*playback && !*capture) { + if (!has_playback && !has_capture) { dev_err(rtd->dev, "substream %s has no playback, no capture\n", dai_link->stream_name);
return -EINVAL; }
+ *playback = has_playback; + *capture = has_capture; + return 0; }
On 5/30/2023 2:50 AM, Kuninori Morimoto wrote:
soc_get_playback_capture() (A) returns number of substreams for playback/capture (B).
(A) static int soc_get_playback_capture(..., (B) int *playback, int *capture) { ... for_each_xxx(...) { if (xxx) return -EINVAL; => *playback = 1; ... => *capture = 1; ... } ... }
But, it is directly updating playback/capture which is the result of this function even though it might be error. It should be updated in case of succeed only. This patch updates it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-pcm.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 47da3be0ff46..b3d569e7ba61 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2733,6 +2733,8 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, { struct snd_soc_dai_link *dai_link = rtd->dai_link; struct snd_soc_dai *cpu_dai;
int has_playback = 0;
int has_capture = 0; int i;
if (dai_link->dynamic && dai_link->num_cpus > 1) {
@@ -2748,11 +2750,11 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
for_each_rtd_cpu_dais(rtd, i, cpu_dai) { if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
*playback = 1;
has_playback = 1; break; } }
if (!*playback) {
if (!has_playback) { dev_err(rtd->card->dev, "No CPU DAIs support playback for stream %s\n", dai_link->stream_name);@@ -2764,12 +2766,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
for_each_rtd_cpu_dais(rtd, i, cpu_dai) { if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
*capture = 1;
has_capture = 1; break; } }
if (!*capture) {
if (!has_capture) { dev_err(rtd->card->dev, "No CPU DAIs support capture for stream %s\n", dai_link->stream_name);@@ -2798,30 +2800,33 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) && snd_soc_dai_stream_valid(cpu_dai, cpu_playback))
*playback = 1;
has_playback = 1; if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_CAPTURE) && snd_soc_dai_stream_valid(cpu_dai, cpu_capture))
*capture = 1;
has_capture = 1;} }
if (dai_link->playback_only) {
*playback = 1;*capture = 0;
has_playback = 1;has_capture = 0;}
if (dai_link->capture_only) {
*playback = 0;*capture = 1;
has_playback = 0;has_capture = 1;
- if (!*playback && !*capture) {
if (!has_playback && !has_capture) { dev_err(rtd->dev, "substream %s has no playback, no capture\n", dai_link->stream_name);
return -EINVAL; }
*playback = has_playback;
*capture = has_capture;
return 0; }
Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
soc_get_playback_capture() (A) returns number of substreams for playback/capture, and then, we can use playback/capture_only flag (X)(Y).
(A) static int soc_get_playback_capture(...) { ... (X) if (dai_link->playback_only) { (*) *playback = 1; *capture = 0; }
(Y) if (dai_link->capture_only) { *playback = 0; (*) *capture = 1; } ... }
But this flag should not have effect to opposite side stream (*). This patch tidyup it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/soc-pcm.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index b3d569e7ba61..159670612de3 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2807,15 +2807,11 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd, } }
- if (dai_link->playback_only) { - has_playback = 1; + if (dai_link->playback_only) has_capture = 0; - }
- if (dai_link->capture_only) { + if (dai_link->capture_only) has_playback = 0; - has_capture = 1; - }
if (!has_playback && !has_capture) { dev_err(rtd->dev, "substream %s has no playback, no capture\n",
This is minor cleanup patches for soc_get_playback_capture().
Link: https://lore.kernel.org/r/87ttw1gqgn.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87353uqjiu.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.c...
Kuninori Morimoto (5): ASoC: soc-pcm.c: indicate error if stream has no playback no capture ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture() ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture() ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture()
Very nice cleanup, thank you Morimoto-san for splitting the steps in different patches to make the changes simple to identify.
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
FWIW test results can be found at the following link (no new issues found) https://github.com/thesofproject/linux/pull/4392
On Tue, 30 May 2023 00:49:27 +0000, Kuninori Morimoto wrote:
This is minor cleanup patches for soc_get_playback_capture().
Link: https://lore.kernel.org/r/87ttw1gqgn.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/87353uqjiu.wl-kuninori.morimoto.gx@renesas.com Link: https://lore.kernel.org/r/ab3f0c0a-62fd-a468-b3cf-0e4b59bac6ae@linux.intel.c...
Kuninori Morimoto (5): ASoC: soc-pcm.c: indicate error if stream has no playback no capture ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture() ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture() ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture()
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/5] ASoC: soc-pcm.c: indicate error if stream has no playback no capture commit: 092830cf550667d5fa6286605167d232f2c1f61e [2/5] ASoC: soc-pcm.c: use dai_link on soc_get_playback_capture() commit: cfcb31c456b15e298f88fb5ebedf7b32b009d32d [3/5] ASoC: soc-pcm.c: cleanup soc_get_playback_capture() error commit: a1c0221fa5baeae6c9dc30294c2c6d01f1f4379b [4/5] ASoC: soc-pcm.c: use temporary variable at soc_get_playback_capture() commit: c3e9b6d6ef5a0a3e841c3aa29e7afc48a0b73806 [5/5] ASoC: soc-pcm.c: tidyup playback/capture_only at soc_get_playback_capture() commit: e1f653ce847bab7285dd135cabe3ce544e574c75
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (4)
-
Amadeusz Sławiński -
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart