Re: [PATCH 07/10] ASoC: soc-dai: rename snd_soc_dai_stream_active()

["Alvin Šipraga" <alvin.sipraga@analog.com>]

On Tue, Sep 15, 2026 at 05:20:50AM +0000, Kuninori Morimoto wrote:
> 
> Hi Alvin
> 
> Thank you for your review
> 
> > Normally I read a function like snd_soc_dai_steam_active(x, y) to mean
> > "is stream y of dai x active"? We also have snd_soc_dai_stream_valid()
> > which has similar semantics.
> > 
> > OTOH snd_soc_dai_active_stream() sounds like "what's the active stream
> > of the DAI?" (which of course doesn't make sense; could be that both or
> > neither are active).
> > 
> > I sense you're trying to "namespace" all the dai->stream[dir].active
> > logic under functions named snd_soc_dai_active_xxx(), which is
> > understandable, but don't you think the following is cleaner?
> > 
> > snd_soc_dai_action()          <=>   similar to snd_soc_runtime_action()
> > snd_soc_dai_active()          <=>   is (any) DAI stream active?
> > snd_soc_dai_stream_active()   <=>   is (given) DAI stream active?
> 
> Yeah, agree.
> This is maybe because of my English-skill, but "active" / "action" are
> makes it more confusable / non-intuitive.

I agree _active() <=> _action() is not an obvious relationship. But the
_action() form is just an internal thing to avoid code duplication in
snd_soc_runtime_activate()/deactivate():


   snd_soc_runtime_activate()               snd_soc_runtime_deactivate()
            |                                           |
            +----------------------+--------------------+
                                   |
                                   v
                           snd_soc_runtime_action()
                                   |
            +----------------------+--------------------+
            |                                           |
            v                                           v
(A) snd_soc_dai_action(+1)                    snd_soc_dai_action(-1)
              `-> snd_soc_dai_activate()                `-> snd_soc_dai_deactivate()
                  component->active++;                      component->active--;
        or                                          or
(B) snd_soc_component_action(+1) *            snd_soc_component_action(-1) *
              `-> component->active++;                  `-> component->active--;
                           

[*] snd_soc_component_action() doesn't exist yet but I guess it will if
    you proceed with your series this way.

(B) this path is only relevant for PLATFORM components associated with
    the runtime, all CPU/CODEC components' active update go via path (A)


Since it's internal, I wouldn't care too much about the naming. It could
also be snd_soc_dai_active_update(dai, +/-1). Or maybe _push/_pop? But
even as a native English speaker, I think _action() is quite fine.

> 
> How about this ?
> 
> 	snd_soc_dai_stream_count()		(= snd_soc_dai_active_stream())
> 	snd_soc_dai_stream_count_update()	(= snd_soc_dai_active_action())
> 
> 	snd_soc_dai_count()			(= snd_soc_dai_active())
> 	// no _count_update()
> 
> Not only DAI, but Component/runtime too
> 
> 	snd_soc_component_count()		(= snd_soc_component_active())
> 	snd_soc_component_count_update()	(= snd_soc_component_active_action())
> 
> 	// no _count()
> 	snd_soc_runtime_count_update()		(= snd_soc_runtime_action())
> 
> Then, we want to rename .active to .count

I would look at it more from the API consumer side, since naming can
confuse driver authors.

Most users of snd_soc_dai_active() just treat it as a boolean. By
changing it from "active" to "count", we would be advertising the
internal reference counter, which suggests to the author that he/she
should maybe care about it. I don't think drivers should care about it
though. Especially for more niche things like DPCM, the reference
counter value can be surprising.

There's a few cases where the actual integer value of
snd_soc_dai_active() is tested, but they all seem a bit wrong to me.

What I mean to say is, maybe better to keep the consumer API the same
(snd_soc_dai_active()), and instead consider whether it can be removed
from the public API altogether later on? I think when you are done with
your series it will make it much easier to analyze the codebase and see
what can be simplified!

...

Just a few observations on this snd_soc_dai_active() stuff to explain
what I mean.

As best as I can tell, it's a marker saying that the DAI's .startup op
has been called successfully. If it goes from nonzero to zero, then it's
guaranteed that .shutdown has also been called. Nonzero is also
equivalent to PCM .open having been called, except for codec2codec where
there's no PCM.

snd_soc_component_active() always follows snd_soc_dai_active(). That is,
the following condition is always true:

  !!snd_soc_component_active(dai->component) == !!snd_soc_dai_active(dai)

snd_soc_dai_active() is interesting for drivers' DAI ops. It's used as a
guard sometimes in .startup/shutdown to check if substream-independent
work has been done already. Another common pattern is to guard against
.set_sysclk if a substream is already active [codecs/arizona.c].

snd_soc_component_active() is interesting for components drivers' power
management. You see it being checked in .suspend/resume component
ops. I'm not sure if that's actually necessary?

Personally I always found it surprising that DAI .startup can get called
multiple times on the same substream. (I think this is just a DPCM
thing though?) Unlike .hw_params, here is no new info to act on I think.
So maybe the core can guarantee balanced calls of .startup/.shutdown
per-substream, and the guards can be removed. Equally for .set_sysclk,
it's mostly the machine driver's fault if it does that when the PCM is
open. Along the same lines, maybe the core can help here.

Some of the substream-independent work of DAI drivers' .startup/shutdown
probably belong in the component_driver .open/close ops instead. This
would also remove a few of the guards.

Just some thoughts here. I know we were only talking about naming and
don't mean to derail things. But if you have some input then feel free
to share!

Kind regards,
Alvin