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

[Daniel Baluta]

On Wed, 2020-01-29 at 10:24 +0100, Cezary Rojewski wrote:
> 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 :)
> 

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.