[alsa-devel] [PATCH v3 1/2] ASoC: soc-compress: add a config item for soc-compress

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Tuesday, October 13, 2015 5:33 PM
> To: Jie, Yang
> Cc: broonie at kernel.org; alsa-devel at alsa-project.org; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH v3 1/2] ASoC: soc-compress: add a config
> item for soc-compress
> 
> On Tue, 13 Oct 2015 11:11:03 +0200,
> Jie Yang wrote:
> >
> > We don't always need soc-compress in soc, here add a config item
> > SND_SOC_COMPRESS, please add 'select SND_SOC_COMPRESS' to driver
> > Kconfig when it is needed.
> >
> > Signed-off-by: Jie Yang <yang.jie at intel.com>
> > ---
> >  include/sound/soc-dai.h                      | 2 +-
> >  include/sound/soc.h                          | 2 ++
> >  sound/soc/Kconfig                            | 6 +++++-
> >  sound/soc/Makefile                           | 6 +++++-
> >  sound/soc/intel/Kconfig                      | 1 +
> >  sound/soc/intel/atom/sst-mfld-platform-pcm.c | 2 +-
> >  sound/soc/soc-compress.c                     | 1 +
> >  sound/soc/soc-core.c                         | 4 ++--
> >  8 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index
> > 2df96b1..238200f 100644
> > --- a/include/sound/soc-dai.h
> > +++ b/include/sound/soc-dai.h
> > @@ -214,7 +214,7 @@ struct snd_soc_dai_driver {
> >  	int (*suspend)(struct snd_soc_dai *dai);
> >  	int (*resume)(struct snd_soc_dai *dai);
> >  	/* compress dai */
> > -	bool compress_dai;
> > +	int (*compress_new)(struct snd_soc_pcm_runtime *rtd, int num);
> >  	/* DAI is also used for the control bus */
> >  	bool bus_control;
> >
> > diff --git a/include/sound/soc.h b/include/sound/soc.h index
> > 470f208..623ce0c 100644
> > --- a/include/sound/soc.h
> > +++ b/include/sound/soc.h
> > @@ -452,7 +452,9 @@ int snd_soc_platform_read(struct
> snd_soc_platform
> > *platform,  int snd_soc_platform_write(struct snd_soc_platform *platform,
> >  					unsigned int reg, unsigned int val);
> int soc_new_pcm(struct
> > snd_soc_pcm_runtime *rtd, int num);
> > +#if IS_ENABLED(CONFIG_SND_SOC_COMPRESS)
> >  int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num);
> > +#endif
> 
> Maybe better to provide a dummy function returning an error when this isn't
> enabled.

But I remember that you mentioned(comment to v2 on June 15th) that you 
don't like dummy function:

'A dummy function in such a case has both merit and demerit.  The demerit is
that you won't get errors if the driver really wants the compress support but
just forgot to select the dependency.

Takashi'

> 
> Also, as a namespace issue, we usually keep snd_ prefix for exported
> functions.
 
OK, let me change it to snd_xxx.

> 
> 
> >
> >  struct snd_pcm_substream *snd_soc_get_dai_substream(struct
> snd_soc_card *card,
> >  		const char *dai_link, int stream);
> > diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index
> > 7de792b..20b3069 100644
> > --- a/sound/soc/Kconfig
> > +++ b/sound/soc/Kconfig
> > @@ -9,7 +9,6 @@ menuconfig SND_SOC
> >  	select SND_JACK if INPUT=y || INPUT=SND
> >  	select REGMAP_I2C if I2C
> >  	select REGMAP_SPI if SPI_MASTER
> > -	select SND_COMPRESS_OFFLOAD
> >  	---help---
> >
> >  	  If you want ASoC support, you should say Y here and also to the @@
> > -30,6 +29,11 @@ config SND_SOC_GENERIC_DMAENGINE_PCM
> >  	bool
> >  	select SND_DMAENGINE_PCM
> >
> > +config SND_SOC_COMPRESS
> > +	tristate
> > +	select SND_COMPRESS_OFFLOAD
> > +	default n
> 
> No need for default n.  It's default.
 
Okay.

> 
> 
> > +
> >  config SND_SOC_TOPOLOGY
> >  	bool
> >
> > diff --git a/sound/soc/Makefile b/sound/soc/Makefile index
> > af0a571..a3a1505 100644
> > --- a/sound/soc/Makefile
> > +++ b/sound/soc/Makefile
> > @@ -1,5 +1,9 @@
> >  snd-soc-core-objs := soc-core.o soc-dapm.o soc-jack.o soc-cache.o
> > soc-utils.o -snd-soc-core-objs += soc-pcm.o soc-compress.o soc-io.o
> > soc-devres.o soc-ops.o
> > +snd-soc-core-objs += soc-pcm.o soc-io.o soc-devres.o soc-ops.o
> > +
> > +ifneq ($(CONFIG_SND_SOC_COMPRESS),)
> > +snd-soc-core-objs += soc-compress.o
> > +endif
> 
> This can be simplified like
> 
> snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
 
I tried this as you have mentioned before, but it seems can't work properly
when CONFIG_SND_SOC_COMPRESS is set to 'm'.
Not clear why it happen yet. :(

> 
> 
> >  ifneq ($(CONFIG_SND_SOC_TOPOLOGY),)
> >  snd-soc-core-objs += soc-topology.o
> 
> Oh, this is also... Make another patch :)
> 
> 
> > diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index
> > 025c38f..a672d9c 100644
> > --- a/sound/soc/soc-compress.c
> > +++ b/sound/soc/soc-compress.c
> > @@ -703,3 +703,4 @@ compr_err:
> >  	kfree(compr);
> >  	return ret;
> >  }
> > +EXPORT_SYMBOL_GPL(soc_new_compress);
> 
> When you're creating a new exported function, don't forget to give a proper
> documentation in the function comment.

Vinod also comment/mention this, OK, will add it. 

~Keyon

> 
> 
> thanks,
> 
> Takashi