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

[Cezary Rojewski]

On 2020-01-29 10:46, Daniel Baluta wrote:
> On Wed, 2020-01-29 at 10:24 +0100, Cezary Rojewski wrote:
>>
>> 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 :)
>>
> 
> Indeed, we can make the code smarter later but I want to do the code
> cleaner now.
> 
> I think I had a copy paste error when providing the example.
> 
> So, my proposal is to override the platform driver compr_ops field
> with probe compressed ops when CONFIG_SND_SOC_SOF_DEBUG_PROBES is set.
> 
> The code could look like this in the end:
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
>            pd->compr_ops = &sof_compressed_ops;
> #endif
> 
> /* Add a comment explaining that we are doing override in case of
> probes */
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>            pd->compr_ops = &sof_probe_compressed_ops;
> #endif
> 
> Also, I think there is no need to define the probe compressed ops
> inside pcm.c
> 
> So, instead of
> 
> #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
> 
> We can do:
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
> extern snd_compr_ops sof_probe_compressed_ops;
> #endif
> 
> or even better include a header with the declaration.
> 
> And add the definition of sof_probe_compressed_ops would be somewhere
> in the compressed probe file.
> 
> 

Your comments have been addressed in v4. Non-existant cops usage have 
been re-added and is now overridden by probes when enabled. 
sof_probe_compressed_ops declaration moved to compress.c file as requested.

Czarek