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
