[PATCH 0/6] ASoC: soc-pcm: cleanup code
Hi Mark
These are not so important, but for soc-pcm cleanup patches.
Kuninori Morimoto (6): ASoC: soc-pcm: move dpcm_set_fe_update_state() ASoC: soc-pcm: add dpcm_set_be_update_state() ASoC: soc-pcm: add soc_pcm_set_dai_params() ASoC: soc-pcm: cleanup soc_pcm_apply_symmetry() ASoC: soc-pcm: cleanup soc_pcm_params_symmetry() ASoC: soc-pcm: setup pcm at one place in soc_new_pcm()
sound/soc/soc-pcm.c | 231 +++++++++++++++++--------------------------- 1 file changed, 90 insertions(+), 141 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch moves dpcm_set_fe_update_state() to top side. This is prepare for cleanup soc-pcm.c
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 45 ++++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 23 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ee51dc7fd893..46b4c77eab5f 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -203,6 +203,28 @@ static inline void dpcm_remove_debugfs_state(struct snd_soc_dpcm *dpcm) } #endif
+/* Set FE's runtime_update state; the state is protected via PCM stream lock + * for avoiding the race with trigger callback. + * If the state is unset and a trigger is pending while the previous operation, + * process the pending trigger action here. + */ +static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream *substream, int cmd); +static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe, + int stream, enum snd_soc_dpcm_update state) +{ + struct snd_pcm_substream *substream = + snd_soc_dpcm_get_substream(fe, stream); + + snd_pcm_stream_lock_irq(substream); + if (state == SND_SOC_DPCM_UPDATE_NO && fe->dpcm[stream].trigger_pending) { + dpcm_fe_dai_do_trigger(substream, + fe->dpcm[stream].trigger_pending - 1); + fe->dpcm[stream].trigger_pending = 0; + } + fe->dpcm[stream].runtime_update = state; + snd_pcm_stream_unlock_irq(substream); +} + /** * snd_soc_runtime_action() - Increment/Decrement active count for * PCM runtime components @@ -1710,29 +1732,6 @@ static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream) &runtime->hw.rate_min, &runtime->hw.rate_max); }
-static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream *substream, int cmd); - -/* Set FE's runtime_update state; the state is protected via PCM stream lock - * for avoiding the race with trigger callback. - * If the state is unset and a trigger is pending while the previous operation, - * process the pending trigger action here. - */ -static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe, - int stream, enum snd_soc_dpcm_update state) -{ - struct snd_pcm_substream *substream = - snd_soc_dpcm_get_substream(fe, stream); - - snd_pcm_stream_lock_irq(substream); - if (state == SND_SOC_DPCM_UPDATE_NO && fe->dpcm[stream].trigger_pending) { - dpcm_fe_dai_do_trigger(substream, - fe->dpcm[stream].trigger_pending - 1); - fe->dpcm[stream].trigger_pending = 0; - } - fe->dpcm[stream].runtime_update = state; - snd_pcm_stream_unlock_irq(substream); -} - static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream, int stream) {
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc-pcm has dpcm_set_fe_update_state() to update FE's runtime_update (except dpcm_fe_dai_do_trigger() which needs to update it without it). OTOH, it doesn't have BE's update function.
O: dpcm_set_fe_update_state() X: dpcm_set_be_update_state()
This patch add BE's dpcm_set_fe_update_state()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 46b4c77eab5f..713fa202c67b 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -225,6 +225,12 @@ static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe, snd_pcm_stream_unlock_irq(substream); }
+static void dpcm_set_be_update_state(struct snd_soc_pcm_runtime *be, + int stream, enum snd_soc_dpcm_update state) +{ + be->dpcm[stream].runtime_update = state; +} + /** * snd_soc_runtime_action() - Increment/Decrement active count for * PCM runtime components @@ -1357,7 +1363,7 @@ static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream, stream ? "capture" : "playback", dpcm->be->dai_link->name, fe->dai_link->name); dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE; - dpcm->be->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE; + dpcm_set_be_update_state(dpcm->be, stream, SND_SOC_DPCM_UPDATE_BE); prune++; }
@@ -1412,7 +1418,7 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, continue;
/* new */ - be->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE; + dpcm_set_be_update_state(be, stream, SND_SOC_DPCM_UPDATE_BE); new++; }
@@ -1440,8 +1446,7 @@ void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream)
spin_lock_irqsave(&fe->card->dpcm_lock, flags); for_each_dpcm_be(fe, stream, dpcm) - dpcm->be->dpcm[stream].runtime_update = - SND_SOC_DPCM_UPDATE_NO; + dpcm_set_be_update_state(dpcm->be, stream, SND_SOC_DPCM_UPDATE_NO); spin_unlock_irqrestore(&fe->card->dpcm_lock, flags); }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Getting rate/channels/sample_bits from param needs fixed method. This patch adds new soc_pcm_set_dai_params() and replace existing code.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 52 +++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 25 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 713fa202c67b..98a47fc43923 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -329,6 +329,20 @@ int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir, return 0; }
+static void soc_pcm_set_dai_params(struct snd_soc_dai *dai, + struct snd_pcm_hw_params *params) +{ + if (params) { + dai->rate = params_rate(params); + dai->channels = params_channels(params); + dai->sample_bits = snd_pcm_format_physical_width(params_format(params)); + } else { + dai->rate = 0; + dai->channels = 0; + dai->sample_bits = 0; + } +} + static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream, struct snd_soc_dai *soc_dai) { @@ -390,13 +404,12 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_soc_dai d; struct snd_soc_dai *dai; struct snd_soc_dai *cpu_dai; - unsigned int rate, channels, sample_bits, symmetry, i; + unsigned int symmetry, i;
- rate = params_rate(params); - channels = params_channels(params); - sample_bits = snd_pcm_format_physical_width(params_format(params)); + soc_pcm_set_dai_params(&d, params);
/* reject unmatched parameters when applying symmetry */ symmetry = rtd->dai_link->symmetric_rates; @@ -406,9 +419,9 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
if (symmetry) { for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (cpu_dai->rate && cpu_dai->rate != rate) { + if (cpu_dai->rate && cpu_dai->rate != d.rate) { dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n", - cpu_dai->rate, rate); + cpu_dai->rate, d.rate); return -EINVAL; } } @@ -422,9 +435,9 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream, if (symmetry) { for_each_rtd_cpu_dais(rtd, i, cpu_dai) { if (cpu_dai->channels && - cpu_dai->channels != channels) { + cpu_dai->channels != d.channels) { dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n", - cpu_dai->channels, channels); + cpu_dai->channels, d.channels); return -EINVAL; } } @@ -438,9 +451,9 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream, if (symmetry) { for_each_rtd_cpu_dais(rtd, i, cpu_dai) { if (cpu_dai->sample_bits && - cpu_dai->sample_bits != sample_bits) { + cpu_dai->sample_bits != d.sample_bits) { dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n", - cpu_dai->sample_bits, sample_bits); + cpu_dai->sample_bits, d.sample_bits); return -EINVAL; } } @@ -898,11 +911,8 @@ static int soc_pcm_hw_clean(struct snd_pcm_substream *substream, int rollback) for_each_rtd_dais(rtd, i, dai) { int active = snd_soc_dai_stream_active(dai, substream->stream);
- if (snd_soc_dai_active(dai) == 1) { - dai->rate = 0; - dai->channels = 0; - dai->sample_bits = 0; - } + if (snd_soc_dai_active(dai) == 1) + soc_pcm_set_dai_params(dai, NULL);
if (active == 1) snd_soc_dai_digital_mute(dai, 1, substream->stream); @@ -999,11 +1009,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, if(ret < 0) goto out;
- codec_dai->rate = params_rate(&codec_params); - codec_dai->channels = params_channels(&codec_params); - codec_dai->sample_bits = snd_pcm_format_physical_width( - params_format(&codec_params)); - + soc_pcm_set_dai_params(codec_dai, &codec_params); snd_soc_dapm_update_dai(substream, &codec_params, codec_dai); }
@@ -1020,11 +1026,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream, goto out;
/* store the parameters for each DAI */ - cpu_dai->rate = params_rate(params); - cpu_dai->channels = params_channels(params); - cpu_dai->sample_bits = - snd_pcm_format_physical_width(params_format(params)); - + soc_pcm_set_dai_params(cpu_dai, params); snd_soc_dapm_update_dai(substream, params, cpu_dai); }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_apply_symmetry() want to call snd_pcm_hw_constraint_single() for rate/channel/sample_bits, but, it needs many condition check. These are very similar but different, thus, it needs to have very verbose code. This patch use macro for it and make code more simple.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 67 ++++++++++++++------------------------------- 1 file changed, 20 insertions(+), 47 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 98a47fc43923..bf2b49dd691d 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -349,53 +349,26 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); int ret;
- if (soc_dai->rate && (soc_dai->driver->symmetric_rates || - rtd->dai_link->symmetric_rates)) { - dev_dbg(soc_dai->dev, "ASoC: Symmetry forces %dHz rate\n", - soc_dai->rate); - - ret = snd_pcm_hw_constraint_single(substream->runtime, - SNDRV_PCM_HW_PARAM_RATE, - soc_dai->rate); - if (ret < 0) { - dev_err(soc_dai->dev, - "ASoC: Unable to apply rate constraint: %d\n", - ret); - return ret; - } - } - - if (soc_dai->channels && (soc_dai->driver->symmetric_channels || - rtd->dai_link->symmetric_channels)) { - dev_dbg(soc_dai->dev, "ASoC: Symmetry forces %d channel(s)\n", - soc_dai->channels); - - ret = snd_pcm_hw_constraint_single(substream->runtime, - SNDRV_PCM_HW_PARAM_CHANNELS, - soc_dai->channels); - if (ret < 0) { - dev_err(soc_dai->dev, - "ASoC: Unable to apply channel symmetry constraint: %d\n", - ret); - return ret; - } - } - - if (soc_dai->sample_bits && (soc_dai->driver->symmetric_samplebits || - rtd->dai_link->symmetric_samplebits)) { - dev_dbg(soc_dai->dev, "ASoC: Symmetry forces %d sample bits\n", - soc_dai->sample_bits); - - ret = snd_pcm_hw_constraint_single(substream->runtime, - SNDRV_PCM_HW_PARAM_SAMPLE_BITS, - soc_dai->sample_bits); - if (ret < 0) { - dev_err(soc_dai->dev, - "ASoC: Unable to apply sample bits symmetry constraint: %d\n", - ret); - return ret; - } - } +#define __soc_pcm_apply_symmetry(name, sname, NAME) \ + if (soc_dai->name && (soc_dai->driver->symmetric_##sname || \ + rtd->dai_link->symmetric_##sname)) { \ + dev_dbg(soc_dai->dev, "ASoC: Symmetry forces %s to %d\n",\ + #name, soc_dai->name); \ + \ + ret = snd_pcm_hw_constraint_single(substream->runtime, \ + SNDRV_PCM_HW_PARAM_##NAME,\ + soc_dai->name); \ + if (ret < 0) { \ + dev_err(soc_dai->dev, \ + "ASoC: Unable to apply %s constraint: %d\n",\ + #name, ret); \ + return ret; \ + } \ + } + + __soc_pcm_apply_symmetry(rate, rates, RATE); + __soc_pcm_apply_symmetry(channels, channels, CHANNELS); + __soc_pcm_apply_symmetry(sample_bits, samplebits, SAMPLE_BITS);
return 0; }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_params_symmetry() checks rate/channel/sample_bits state. These are very similar but different, thus, it needs to have very verbose code. This patch use macro for it and make code more simple.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 60 ++++++++++++--------------------------------- 1 file changed, 15 insertions(+), 45 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index bf2b49dd691d..2f2c4c19f1b8 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -384,53 +384,23 @@ static int soc_pcm_params_symmetry(struct snd_pcm_substream *substream,
soc_pcm_set_dai_params(&d, params);
- /* reject unmatched parameters when applying symmetry */ - symmetry = rtd->dai_link->symmetric_rates; - - for_each_rtd_cpu_dais(rtd, i, dai) - symmetry |= dai->driver->symmetric_rates; - - if (symmetry) { - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (cpu_dai->rate && cpu_dai->rate != d.rate) { - dev_err(rtd->dev, "ASoC: unmatched rate symmetry: %d - %d\n", - cpu_dai->rate, d.rate); - return -EINVAL; - } - } - } - - symmetry = rtd->dai_link->symmetric_channels; - - for_each_rtd_dais(rtd, i, dai) - symmetry |= dai->driver->symmetric_channels; - - if (symmetry) { - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (cpu_dai->channels && - cpu_dai->channels != d.channels) { - dev_err(rtd->dev, "ASoC: unmatched channel symmetry: %d - %d\n", - cpu_dai->channels, d.channels); - return -EINVAL; +#define __soc_pcm_params_symmetry(name, sname) \ + symmetry = rtd->dai_link->symmetric_##sname; \ + for_each_rtd_dais(rtd, i, dai) \ + symmetry |= dai->driver->symmetric_##sname; \ + \ + if (symmetry) \ + for_each_rtd_cpu_dais(rtd, i, cpu_dai) \ + if (cpu_dai->name && cpu_dai->name != d.name) { \ + dev_err(rtd->dev, "ASoC: unmatched %s symmetry: %d - %d\n", \ + #name, cpu_dai->name, d.name); \ + return -EINVAL; \ } - } - }
- symmetry = rtd->dai_link->symmetric_samplebits; - - for_each_rtd_dais(rtd, i, dai) - symmetry |= dai->driver->symmetric_samplebits; - - if (symmetry) { - for_each_rtd_cpu_dais(rtd, i, cpu_dai) { - if (cpu_dai->sample_bits && - cpu_dai->sample_bits != d.sample_bits) { - dev_err(rtd->dev, "ASoC: unmatched sample bits symmetry: %d - %d\n", - cpu_dai->sample_bits, d.sample_bits); - return -EINVAL; - } - } - } + /* reject unmatched parameters when applying symmetry */ + __soc_pcm_params_symmetry(rate, rates); + __soc_pcm_params_symmetry(channels, channels); + __soc_pcm_params_symmetry(sample_bits, samplebits);
return 0; }
Hi Mark
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_params_symmetry() checks rate/channel/sample_bits state. These are very similar but different, thus, it needs to have very verbose code. This patch use macro for it and make code more simple.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
(snip)
+#define __soc_pcm_params_symmetry(name, sname) \
- symmetry = rtd->dai_link->symmetric_##sname; \
- for_each_rtd_dais(rtd, i, dai) \
symmetry |= dai->driver->symmetric_##sname; \\- if (symmetry) \
for_each_rtd_cpu_dais(rtd, i, cpu_dai) \if (cpu_dai->name && cpu_dai->name != d.name) { \dev_err(rtd->dev, "ASoC: unmatched %s symmetry: %d - %d\n", \#name, cpu_dai->name, d.name); \return -EINVAL; \ }
}- }
(snip)
- /* reject unmatched parameters when applying symmetry */
- __soc_pcm_params_symmetry(rate, rates);
- __soc_pcm_params_symmetry(channels, channels);
- __soc_pcm_params_symmetry(sample_bits, samplebits);
Not a big deal, but I'm happy if we can use same naming rule for same things.
rtd->dai_link->symmetric_samplebits; ^^^^^^^^^^ dai->driver->symmetric_samplebits; ^^^^^^^^^^ cpu_dai->sample_bits ^^^^^^^^^^^
__soc_pcm_params_symmetry(rate, rates); ^^^^^ ^^^^^ __soc_pcm_params_symmetry(sample_bits, samplebits); ^^^^^^^^^^^ ^^^^^^^^^^
Thank you for your help !!
Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
pcm is setuped at randam place. This patch setup it in one place.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 2f2c4c19f1b8..1bc9fd0fec07 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2749,9 +2749,10 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) else rtd->close_delayed_work_func = snd_soc_close_delayed_work;
- pcm->nonatomic = rtd->dai_link->nonatomic; rtd->pcm = pcm; pcm->private_data = rtd; + pcm->nonatomic = rtd->dai_link->nonatomic; + pcm->no_device_suspend = true;
if (rtd->dai_link->no_pcm || rtd->dai_link->params) { if (playback) @@ -2808,7 +2809,6 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) return ret; }
- pcm->no_device_suspend = true; out: dev_dbg(rtd->card->dev, "%s <-> %s mapping ok\n", (rtd->num_codecs > 1) ? "multicodec" : asoc_rtd_to_codec(rtd, 0)->name,
On 11 Dec 2020 14:54:53 +0900, Kuninori Morimoto wrote:
These are not so important, but for soc-pcm cleanup patches.
Kuninori Morimoto (6): ASoC: soc-pcm: move dpcm_set_fe_update_state() ASoC: soc-pcm: add dpcm_set_be_update_state() ASoC: soc-pcm: add soc_pcm_set_dai_params() ASoC: soc-pcm: cleanup soc_pcm_apply_symmetry() ASoC: soc-pcm: cleanup soc_pcm_params_symmetry() ASoC: soc-pcm: setup pcm at one place in soc_new_pcm()
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/6] ASoC: soc-pcm: move dpcm_set_fe_update_state() commit: 9c6d7f9346b1526136dc142eb82085737112a3fe [2/6] ASoC: soc-pcm: add dpcm_set_be_update_state() commit: a7e20444ef5e7ab74aec34f34eb0e53024c349e1 [3/6] ASoC: soc-pcm: add soc_pcm_set_dai_params() commit: 2805b8bd3e0bdda15b3458ab9818d80f5d5b157f [4/6] ASoC: soc-pcm: cleanup soc_pcm_apply_symmetry() commit: a39748d03cbc7c0a55d217731c7e16a22a2d2bed
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 (2)
-
Kuninori Morimoto -
Mark Brown