[alsa-devel] [PATCH v3 11/11] ASoC: SOF: Intel: Add Probe compress CPU DAIs

[Cezary Rojewski]

On 2020-01-29 09:04, Daniel Baluta wrote:
> I'm not really happy with the changes inside pcm.c
> 
> 
> On Tue, 2020-01-28 at 11:43 +0100, Cezary Rojewski wrote:
>> --- a/sound/soc/sof/pcm.c
>> +++ b/sound/soc/sof/pcm.c
>> @@ -756,6 +756,15 @@ static void sof_pcm_remove(struct
>> snd_soc_component *component)
>>          snd_soc_tplg_component_remove(component,
>> SND_SOC_TPLG_INDEX_ALL);
>>   }
>>   
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>> +#include "compress.h"
>> +
>> struct snd_compr_ops sof_compressed_ops = {+
>> +       .copy           = sof_probe_compr_copy,
>> +};
>> +EXPORT_SYMBOL(sof_compressed_ops);
>> +#endif
>> +
> 
> Maybe call this structure sof_probe_compressed ops. Othwerwise you will
> conflict with the real sof_compressed_ops.
> 
> 
>>   void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
>>   {
>>          struct snd_soc_component_driver *pd = &sdev->plat_drv;
>> @@ -775,7 +784,7 @@ void snd_sof_new_platform_drv(struct snd_sof_dev
>> *sdev)
>>          pd->trigger = sof_pcm_trigger;
>>          pd->pointer = sof_pcm_pointer;
>>   
>> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>>          pd->compr_ops = &sof_compressed_ops;
>>   #endif
>>          pd->pcm_construct = sof_pcm_new;
>>
> 
> Here you are breaking the non-existent yet compressed support. I would
> leave:
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>           pd->compr_ops = &sof_compressed_ops;
> #endif
> 
> and only override compr_ops if SND_SOC_SOF_DEBUG_PROBES is set like
> this:
> 
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>           pd->compr_ops = &sof_probe_compressed_ops;
> #endif
> 
> Also does this mean we cannot support both "real" compressed sound card
> and probes in the same time?
> 
> 

Thanks for the review Daniel!

Tthe example you provided is not very clear to me - same condition is 
added for both assignments, but I'll try to answer your question.

Existing "sof_compressed_ops" don't even exist, as you said it yourself 
so nothing is lost. Changes within pcm.c are gated by _DEBUG_PROBES 
anyway so the solution is actually very clean.

While DAI can have different cops, platform driver cannot. I'm aware of 
the problem but till actual compress support for SOF comes out, minimal, 
probe-only changes were a better choice IMHO. After all, that's not a 
problem to make this code smarter in the future - not a farseer though, 
can't predict what you're going to add for SOF-compr :)

Czarek