[Sound-open-firmware] [alsa-devel] [PATCH v5 05/14] ASoC: SOF: Add PCM operations support

[Pierre-Louis Bossart]

On 4/9/19 10:48 AM, Takashi Iwai wrote:
> On Tue, 09 Apr 2019 16:23:17 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>> ok, thanks for confirming. we'll remove the INFO_RESUME flag in SOF
>>> and follow-up with the removal on all other Intel drivers. Thanks
>>> for enlightening us on this.
>>
>> Actually one more question related to the documentation, which reads
>>
>> "Note that the trigger with SUSPEND can always be called when
>> snd_pcm_suspend_all() is called, regardless of the
>> SNDRV_PCM_INFO_RESUME flag. The RESUME flag affects only the behavior
>> of snd_pcm_resume(). (Thus, in theory, SNDRV_PCM_TRIGGER_RESUME isn’t
>> needed to be handled in the trigger callback when no
>> SNDRV_PCM_INFO_RESUME flag is set. But, it’s better to keep it for
>> compatibility reasons.)"
>>
>> I could not figure out what the last sentence means. It's my
>> understanding that the resume_trigger will never be called with the
>> code flow below when INFO_RESUME isn't declared. Would you mind
>> clarifying what this compatibility might be? Thanks!
> 
> Well, in the above "better to keep it" text -- here "it" was meant as
> SNDRV_PCM_TRIGGER_RESUME case handling in the trigger callback, not as
> SNDRV_PCM_INFO_RESUME flag.  That is, the above recommends a trigger
> callback like below would keep SNDRV_PCM_TRIGGER_RESUME although it
> won't be called practically:

That's the part that I find odd. we keep the TRIGGER_RESUME but it will 
never be called, that's an unreachable/untestable switch case, no? Or we 
should trap it as an error case.

> static int foo_trigger(....)
> {
> 	switch (cmd) {
> 	case SNDRV_PCM_TRIGGER_START:
> 	case SNDRV_PCM_TRIGGER_RESUME:
> 		do_start();
> 		break;
> 	case SNDRV_PCM_TRIGGER_STOP:
> 	case SNDRV_PCM_TRIGGER_SUSPEND:
> 		do_stop();
> 		break;
> 	....
> 	}
> 	....
> }
> 
> 
> This text needs clearly a better rephrasing...
> 
> 
> Takashi
>