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

[Pierre-Louis Bossart]

>> +	/* container size */
>> +	switch (params_width(params)) {
>> +	case 16:
>> +		pcm.params.sample_container_bytes = 2;
>> +		break;
>> +	case 24:
>> +		pcm.params.sample_container_bytes = 4;
>> +		break;
>> +	case 32:
>> +		pcm.params.sample_container_bytes = 4;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
> 
> This can be simply snd_pcm_format_physical_width() / 8.

ok

> 
>> +static int sof_pcm_open(struct snd_pcm_substream *substream)
>> +{
> ....
>> +	/* set any runtime constraints based on topology */
>> +	snd_pcm_hw_constraint_step(substream->runtime, 0,
>> +				   SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
>> +				   le32_to_cpu(caps->period_size_min));
>> +	snd_pcm_hw_constraint_step(substream->runtime, 0,
>> +				   SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
>> +				   le32_to_cpu(caps->period_size_min));
> 
> Is period_size_min in frames or in bytes?  The code below refers as
> byte size while this refers as frames, so they look inconsistent.

need to check, the topology files always mention frames not bytes.

> 
>> +
>> +	/* set runtime config */
>> +	runtime->hw.info = SNDRV_PCM_INFO_MMAP |
>> +			  SNDRV_PCM_INFO_MMAP_VALID |
>> +			  SNDRV_PCM_INFO_INTERLEAVED |
>> +			  SNDRV_PCM_INFO_PAUSE |
>> +			  SNDRV_PCM_INFO_RESUME |
>> +			  SNDRV_PCM_INFO_NO_PERIOD_WAKEUP;
> 
> Does SOF support the full resume?  That is, the stream gets resumed at
> the exact position that was suspended.  Most devices can't, hence they
> don't set *_INFO_RESUME flag and let user-space restarting.

Ah this is interesting. All previous Intel drivers set this flag, 
baytrail/haswell/broadwell, atom-sst and skylake. Only Skylake+ devices 
can restart at exactly the right position, but it's my understanding 
that no one ever enabled this mode.

Now I see an awful amount of devices that set this INFO_RESUME flag, and 
I'd be surprised if they all supported a full resume, could it be that 
there's a requirement that fell between the cracks on this one?

> 
>> +	runtime->hw.period_bytes_min = le32_to_cpu(caps->period_size_min);
>> +	runtime->hw.period_bytes_max = le32_to_cpu(caps->period_size_max);
>> +	runtime->hw.periods_min = le32_to_cpu(caps->periods_min);
>> +	runtime->hw.periods_max = le32_to_cpu(caps->periods_max);
>> +	runtime->hw.buffer_bytes_max = le32_to_cpu(caps->buffer_size_max);
> 
> These refer as bytes...

will check as well.