Hi Ranjani
Thank you for your review
> > void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
> > - struct snd_pcm_substream *substream)
> > + struct snd_pcm_substream *substream,
> > + int rollback)
> > {
> > + if (rollback && !soc_dai_mark_match(dai, substream, startup))
> > + return;
> Morimoto-san,
> Im having a hard time undersdtanding why we need the second check? When
> will it ever be the case that this match will fail?
for_each_rtd_dais(...) {
ret = snd_soc_dai_startup(dai, substream);
if (ret < 0)
goto config_err;
...
}
...
config_err:
for_each_rtd_dais(rtd, i, dai)
snd_soc_dai_shutdown(dai, substream, 1);
For example, if rtd had 10 DAIs, and .startup() failed at 3rd DAI,
the mark will be
/* xxx here means unknown */
dai[0].mark_startup = substream;
dai[1].mark_startup = substream;
dai[2].mark_startup = substream;
dai[3].mark_startup = xxx;
...
dai[7].mark_startup = xxx;
dai[8].mark_startup = xxx;
dai[9].mark_startup = xxx;
Then, we need to call .shutdown() is for dai[0] - dai[2],
In such case, .shutdown() will be called with rollback flag.
if's second check will be failed on dai[3] - dai[9].
But please double check.
> I think if we want to roll-back in case of errors , we could simply
> have a bool in snd_soc_dai to indicate that the dai has been started up
> already and check that here to decide whether we should shut it down or
> not.
Yes, my v1 patch was that.
But someone (I don't remember who was he) indicated that
it is not enough if same DAI was called many times.
In my understanding, for example, if same DAI is used for 2xPlaybacks,
and 1st Playback was successed, 2nd Playback was failed,
and if it was using bool instead of pointer,
2nd Playback rollback doesn't need to call shutdown,
but it has successed bit of 1st Playback.
Then, 2nd Playback rollback will call unneeded shutdown,
and 1st Playback's necessary shutdown will not be called.
We can avoid such things if we use pointer instead of bool.
But please double check.
> Also, push/pop are associated with stacks and we're only really dealing
> with one object here. So it is a bit misleading nomemclature-wise.
Yes maybe.
The reason why I used push/pop was that it might be updated to handle
pointer list instead of single pointer (and maybe Pierre-Louis want it :).
I think I indicated it on git log and comment.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
