| Message ID | 8736879fqa.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: cleanup DAI/Component activity | expand |
On Mon, 2020-05-11 at 14:56 +0900, Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-compress.c | 4 ++-- > sound/soc/soc-core.c | 4 ++-- > sound/soc/soc-dapm.c | 8 ++++---- > sound/soc/soc-pcm.c | 14 +++++++------- > 4 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c > index def3ae78b4a7..92d70e75a5a4 100644 > --- a/sound/soc/soc-compress.c > +++ b/sound/soc/soc-compress.c > @@ -231,10 +231,10 @@ static int soc_compr_free(struct > snd_compr_stream *cstream) > > snd_soc_dai_digital_mute(codec_dai, 1, cstream->direction); > > - if (!cpu_dai->active) > + if (!snd_soc_dai_activity(cpu_dai)) I have a feeling this is probably incorrect. snd_soc_dai_activity() checks for stream_active count which is different from snd_soc_dai's active member, isnt it? Thanks, Ranjani
Hi Ranjani Thank you for reviewing > > - if (!cpu_dai->active) > > + if (!snd_soc_dai_activity(cpu_dai)) > I have a feeling this is probably incorrect. snd_soc_dai_activity() > checks for stream_active count which is different from snd_soc_dai's > active member, isnt it? OK, let me check. The original code is like this static void snd_soc_runtime_action(struct snd_soc_pcm_runtime *rtd, int stream, int action) { ... for_each_rtd_dais(rtd, i, dai) { dai->stream_active[stream] += action; dai->active += action; ... } } void snd_soc_runtime_activate(...) { snd_soc_runtime_action(rtd, stream, 1); } void snd_soc_runtime_deactivate(...) { snd_soc_runtime_action(rtd, stream, -1); } Basically, ASoC calls snd_soc_runtime_activate() for activate count up, and, snd_soc_runtime_deactivate() for activate count down. I think snd_soc_dai_activity() is correct, *so far*. Exceptions are soc-dapm.c :: snd_soc_dai_link_event_pre_pmu() soc-dapm.c :: snd_soc_dai_link_event() snd_soc_dai_link_event_pre_pmu(...) { ... snd_soc_dapm_widget_for_each_source_path(w, path) { ... source->active++; } ... snd_soc_dapm_widget_for_each_sink_path(w, path) { ... sink->active++; } ... } snd_soc_dai_link_event(...) { ... snd_soc_dapm_widget_for_each_source_path(w, path) { ... source->active--; ... } snd_soc_dapm_widget_for_each_sink_path(w, path) { ... sink->active--; ... } ... } Only these directly access to dai->active, *without* thinking stream_active. I think it should use snd_soc_runtime_activate() / snd_soc_runtime_deactivate(). My patch cares it... Oops !! I thought my patch cares it, but not enough (= [02/17]). Can you agree below ? 1) use runtime_activate()/deactivate() at above functions. 2) apply my original patches on top of 1) then, update git-log to explain above Thank you for your help !! Best regards --- Kuninori Morimoto
On Tue, 2020-05-12 at 08:53 +0900, Kuninori Morimoto wrote: > Hi Ranjani > > Thank you for reviewing > > > > - if (!cpu_dai->active) > > > + if (!snd_soc_dai_activity(cpu_dai)) > > > > I have a feeling this is probably incorrect. snd_soc_dai_activity() > > checks for stream_active count which is different from > > snd_soc_dai's > > active member, isnt it? > > OK, let me check. > The original code is like this > > static void snd_soc_runtime_action(struct snd_soc_pcm_runtime > *rtd, > int stream, int action) > { > ... > for_each_rtd_dais(rtd, i, dai) { > dai->stream_active[stream] += action; > dai->active += action; > ... > } > } > > void snd_soc_runtime_activate(...) > { > snd_soc_runtime_action(rtd, stream, 1); > } > > void snd_soc_runtime_deactivate(...) > { > snd_soc_runtime_action(rtd, stream, -1); > } > > > Basically, ASoC calls > snd_soc_runtime_activate() for activate count up, and, > snd_soc_runtime_deactivate() for activate count down. > > I think snd_soc_dai_activity() is correct, *so far*. > > Exceptions are > soc-dapm.c :: snd_soc_dai_link_event_pre_pmu() > soc-dapm.c :: snd_soc_dai_link_event() > > snd_soc_dai_link_event_pre_pmu(...) > { > ... > snd_soc_dapm_widget_for_each_source_path(w, path) { > ... > source->active++; > } > ... > snd_soc_dapm_widget_for_each_sink_path(w, path) { > ... > sink->active++; > } > ... > } > > snd_soc_dai_link_event(...) > { > ... > snd_soc_dapm_widget_for_each_source_path(w, path) { > ... > source->active--; > ... > } > > snd_soc_dapm_widget_for_each_sink_path(w, path) { > ... > sink->active--; > ... > } > ... > } > > Only these directly access to dai->active, *without* thinking > stream_active. > I think it should use snd_soc_runtime_activate() / > snd_soc_runtime_deactivate(). > My patch cares it... Oops !! > I thought my patch cares it, but not enough (= [02/17]). > > Can you agree below ? > 1) use runtime_activate()/deactivate() at above functions. I am wondering what the original intention for having separate stream_active and active members for snd_soc_dai was. With your proposal there wouldnt be a need for "active" anymore, isnt it? Thanks, Ranjani
Hi Ranjani Thank you for feedback > > I think snd_soc_dai_activity() is correct, *so far*. > > > > Exceptions are > > soc-dapm.c :: snd_soc_dai_link_event_pre_pmu() > > soc-dapm.c :: snd_soc_dai_link_event() (snip) > > Can you agree below ? > > 1) use runtime_activate()/deactivate() at above functions. > I am wondering what the original intention for having separate > stream_active and active members for snd_soc_dai was. I'm not sure... > With your proposal there wouldnt be a need for "active" anymore, isnt it? Yeah, thus, my patch will remove it. Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index def3ae78b4a7..92d70e75a5a4 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -231,10 +231,10 @@ static int soc_compr_free(struct snd_compr_stream *cstream) snd_soc_dai_digital_mute(codec_dai, 1, cstream->direction); - if (!cpu_dai->active) + if (!snd_soc_dai_activity(cpu_dai)) cpu_dai->rate = 0; - if (!codec_dai->active) + if (!snd_soc_dai_activity(codec_dai)) codec_dai->rate = 0; if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->shutdown) diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 95d8189e45ab..2cd88b9e8151 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -718,7 +718,7 @@ int snd_soc_resume(struct device *dev) /* activate pins from sleep state */ for_each_card_components(card, component) - if (component->active) + if (snd_soc_component_activity(component)) pinctrl_pm_select_default_state(component->dev); dev_dbg(dev, "ASoC: Scheduling resume work\n"); @@ -1943,7 +1943,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) /* deactivate pins to sleep state */ for_each_card_components(card, component) - if (!component->active) + if (!snd_soc_component_activity(component)) pinctrl_pm_select_sleep_state(component->dev); probe_end: diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 80658d13a855..94134878b320 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -3835,7 +3835,7 @@ snd_soc_dai_link_event_pre_pmu(struct snd_soc_dapm_widget *w, "ASoC: startup() failed: %d\n", ret); goto out; } - source->active++; + source->stream_active[substream->stream]++; } substream->stream = SNDRV_PCM_STREAM_PLAYBACK; @@ -3848,7 +3848,7 @@ snd_soc_dai_link_event_pre_pmu(struct snd_soc_dapm_widget *w, "ASoC: startup() failed: %d\n", ret); goto out; } - sink->active++; + sink->stream_active[substream->stream]++; } substream->hw_opened = 1; @@ -3978,14 +3978,14 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w, substream->stream = SNDRV_PCM_STREAM_CAPTURE; snd_soc_dapm_widget_for_each_source_path(w, path) { source = path->source->priv; - source->active--; + source->stream_active[substream->stream]--; snd_soc_dai_shutdown(source, substream); } substream->stream = SNDRV_PCM_STREAM_PLAYBACK; snd_soc_dapm_widget_for_each_sink_path(w, path) { sink = path->sink->priv; - sink->active--; + sink->stream_active[substream->stream]--; snd_soc_dai_shutdown(sink, substream); } break; diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 440c7e87829a..e923e3746fec 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -772,7 +772,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream) } for_each_rtd_components(rtd, i, component) - if (!component->active) + if (!snd_soc_component_activity(component)) pinctrl_pm_select_sleep_state(component->dev); return 0; @@ -866,7 +866,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) /* Symmetry only applies if we've already got an active stream. */ for_each_rtd_dais(rtd, i, dai) { - if (dai->active) { + if (snd_soc_dai_activity(dai)) { ret = soc_pcm_apply_symmetry(substream, dai); if (ret != 0) goto config_err; @@ -904,7 +904,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) } for_each_rtd_components(rtd, i, component) - if (!component->active) + if (!snd_soc_component_activity(component)) pinctrl_pm_select_sleep_state(component->dev); return ret; @@ -1160,7 +1160,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) for_each_rtd_dais(rtd, i, dai) { int active = dai->stream_active[substream->stream]; - if (dai->active == 1) { + if (snd_soc_dai_activity(dai) == 1) { dai->rate = 0; dai->channels = 0; dai->sample_bits = 0; @@ -1929,7 +1929,7 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream, for_each_rtd_cpu_dais (fe, i, fe_cpu_dai) { /* Symmetry only applies if we've got an active stream. */ - if (fe_cpu_dai->active) { + if (snd_soc_dai_activity(fe_cpu_dai)) { err = soc_pcm_apply_symmetry(fe_substream, fe_cpu_dai); if (err < 0) return err; @@ -1958,7 +1958,7 @@ static int dpcm_apply_symmetry(struct snd_pcm_substream *fe_substream, /* Symmetry only applies if we've got an active stream. */ for_each_rtd_dais(rtd, i, dai) { - if (dai->active) { + if (snd_soc_dai_activity(dai)) { err = soc_pcm_apply_symmetry(fe_substream, dai); if (err < 0) return err; @@ -2731,7 +2731,7 @@ static int soc_dpcm_fe_runtime_update(struct snd_soc_pcm_runtime *fe, int new) return 0; /* only check active links */ - if (!asoc_rtd_to_cpu(fe, 0)->active) + if (!snd_soc_dai_activity(asoc_rtd_to_cpu(fe, 0))) return 0; /* DAPM sync will call this to update DSP paths */