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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
