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

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

[Jie Yang]

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@intel.com>
---
 include/sound/soc.h     | 7 +++++++
 sound/soc/Kconfig       | 5 ++++-
 sound/soc/Makefile      | 3 ++-
 sound/soc/intel/Kconfig | 1 +
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 93df8bf..cc1cd4f 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -440,7 +440,14 @@ 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);
+#else
+static inline int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
+{
+       return -EPERM;
+}
+#endif
 
 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 e2828e1..a124759 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,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM
 	bool
 	select SND_DMAENGINE_PCM
 
+config SND_SOC_COMPRESS
+	tristate
+	select SND_COMPRESS_OFFLOAD
+
 # All the supported SoCs
 source "sound/soc/adi/Kconfig"
 source "sound/soc/atmel/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index a0e1ee6..184c1e6 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -1,6 +1,7 @@
 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
 snd-soc-core-objs += soc-topology.o
+snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
 
 ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),)
 snd-soc-core-objs += soc-generic-dmaengine-pcm.o
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 791953f..e559174 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
 
 config SND_SST_MFLD_PLATFORM
 	tristate
+	select SND_SOC_COMPRESS
 
 config SND_SST_IPC
 	tristate
-- 
1.9.1

[PATCH v2 2/2] ASoC: soc-compress: split soc-compress to a module

[Jie Yang]

Split soc-compress into a separate module so we only compile/load
it when it's needed.

Signed-off-by: Jie Yang <yang.jie@intel.com>
---
 sound/soc/Makefile       | 4 +++-
 sound/soc/soc-compress.c | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 184c1e6..2936088 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -1,7 +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-io.o soc-devres.o soc-ops.o
 snd-soc-core-objs += soc-topology.o
-snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
+snd-soc-compress-objs := soc-compress.o
+
+obj-$(CONFIG_SND_SOC_COMPRESS) += snd-soc-compress.o
 
 ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),)
 snd-soc-core-objs += soc-generic-dmaengine-pcm.o
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 025c38f..a93ac8a 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -19,6 +19,7 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
+#include <linux/module.h>
 #include <sound/core.h>
 #include <sound/compress_params.h>
 #include <sound/compress_driver.h>
@@ -703,3 +704,10 @@ compr_err:
 	kfree(compr);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(soc_new_compress);
+
+/* Module information */
+MODULE_AUTHOR("Ramesh Babu K V <ramesh.babu@linux.intel.com>");
+MODULE_AUTHOR("Vinod Koul <vinod.koul@linux.intel.com>");
+MODULE_DESCRIPTION("ALSA SoC Compress");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Takashi Iwai]

At Mon, 15 Jun 2015 11:20:44 +0800,
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@intel.com>
> ---
>  include/sound/soc.h     | 7 +++++++
>  sound/soc/Kconfig       | 5 ++++-
>  sound/soc/Makefile      | 3 ++-
>  sound/soc/intel/Kconfig | 1 +
>  4 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 93df8bf..cc1cd4f 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -440,7 +440,14 @@ 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);
> +#else
> +static inline int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
> +{
> +       return -EPERM;
> +}
> +#endif

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

>  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 e2828e1..a124759 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,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM
>  	bool
>  	select SND_DMAENGINE_PCM
>  
> +config SND_SOC_COMPRESS
> +	tristate
> +	select SND_COMPRESS_OFFLOAD
> +
>  # All the supported SoCs
>  source "sound/soc/adi/Kconfig"
>  source "sound/soc/atmel/Kconfig"
> diff --git a/sound/soc/Makefile b/sound/soc/Makefile
> index a0e1ee6..184c1e6 100644
> --- a/sound/soc/Makefile
> +++ b/sound/soc/Makefile
> @@ -1,6 +1,7 @@
>  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
>  snd-soc-core-objs += soc-topology.o
> +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
>  
>  ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),)
>  snd-soc-core-objs += soc-generic-dmaengine-pcm.o
> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index 791953f..e559174 100644
> --- a/sound/soc/intel/Kconfig
> +++ b/sound/soc/intel/Kconfig
> @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
>  
>  config SND_SST_MFLD_PLATFORM
>  	tristate
> +	select SND_SOC_COMPRESS
>  
>  config SND_SST_IPC
>  	tristate
> -- 
> 1.9.1
> 

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, June 15, 2015 7:34 PM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R;
> vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> At Mon, 15 Jun 2015 11:20:44 +0800,
> 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@intel.com>
> > ---
> >  include/sound/soc.h     | 7 +++++++
> >  sound/soc/Kconfig       | 5 ++++-
> >  sound/soc/Makefile      | 3 ++-
> >  sound/soc/intel/Kconfig | 1 +
> >  4 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/sound/soc.h b/include/sound/soc.h index
> > 93df8bf..cc1cd4f 100644
> > --- a/include/sound/soc.h
> > +++ b/include/sound/soc.h
> > @@ -440,7 +440,14 @@ 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);
> > +#else
> > +static inline int soc_new_compress(struct snd_soc_pcm_runtime *rtd,
> > +int num) {
> > +       return -EPERM;
> > +}
> > +#endif
> 
> 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.
 
Here I used -EPERM to return and tell caller that compress operation is not
permitted, does it make sense, Takashi?

Or can you suggest a better solution, according to this specific case?

Thanks,
~Keyon
> 
> 
> Takashi
> 
> >  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
> > e2828e1..a124759 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,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM
> >  	bool
> >  	select SND_DMAENGINE_PCM
> >
> > +config SND_SOC_COMPRESS
> > +	tristate
> > +	select SND_COMPRESS_OFFLOAD
> > +
> >  # All the supported SoCs
> >  source "sound/soc/adi/Kconfig"
> >  source "sound/soc/atmel/Kconfig"
> > diff --git a/sound/soc/Makefile b/sound/soc/Makefile index
> > a0e1ee6..184c1e6 100644
> > --- a/sound/soc/Makefile
> > +++ b/sound/soc/Makefile
> > @@ -1,6 +1,7 @@
> >  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
> >  snd-soc-core-objs += soc-topology.o
> > +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
> >
> >  ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),)
> >  snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git
> > a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index
> > 791953f..e559174 100644
> > --- a/sound/soc/intel/Kconfig
> > +++ b/sound/soc/intel/Kconfig
> > @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
> >
> >  config SND_SST_MFLD_PLATFORM
> >  	tristate
> > +	select SND_SOC_COMPRESS
> >
> >  config SND_SST_IPC
> >  	tristate
> > --
> > 1.9.1
> >

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Takashi Iwai]

At Mon, 15 Jun 2015 14:15:40 +0000,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Monday, June 15, 2015 7:34 PM
> > To: Jie, Yang
> > Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R;
> > vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> > Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-
> > compress
> > 
> > At Mon, 15 Jun 2015 11:20:44 +0800,
> > 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@intel.com>
> > > ---
> > >  include/sound/soc.h     | 7 +++++++
> > >  sound/soc/Kconfig       | 5 ++++-
> > >  sound/soc/Makefile      | 3 ++-
> > >  sound/soc/intel/Kconfig | 1 +
> > >  4 files changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/sound/soc.h b/include/sound/soc.h index
> > > 93df8bf..cc1cd4f 100644
> > > --- a/include/sound/soc.h
> > > +++ b/include/sound/soc.h
> > > @@ -440,7 +440,14 @@ 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);
> > > +#else
> > > +static inline int soc_new_compress(struct snd_soc_pcm_runtime *rtd,
> > > +int num) {
> > > +       return -EPERM;
> > > +}
> > > +#endif
> > 
> > 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.
>  
> Here I used -EPERM to return and tell caller that compress operation is not
> permitted, does it make sense, Takashi?

The question is whether a runtime error is the best option.  A runtime
error won't be caught by build tests but only when actually running by
a user.

> Or can you suggest a better solution, according to this specific case?

Well, there are several ways, obviously (e.g. give the preprocessor
error if CONFIG_SND_SOC_COMPRESS is enabled).  But I'm not sure what
is the best to fit, too.  It's rather a matter of taste.


Takashi

> Thanks,
> ~Keyon
> > 
> > 
> > Takashi
> > 
> > >  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
> > > e2828e1..a124759 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,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM
> > >  	bool
> > >  	select SND_DMAENGINE_PCM
> > >
> > > +config SND_SOC_COMPRESS
> > > +	tristate
> > > +	select SND_COMPRESS_OFFLOAD
> > > +
> > >  # All the supported SoCs
> > >  source "sound/soc/adi/Kconfig"
> > >  source "sound/soc/atmel/Kconfig"
> > > diff --git a/sound/soc/Makefile b/sound/soc/Makefile index
> > > a0e1ee6..184c1e6 100644
> > > --- a/sound/soc/Makefile
> > > +++ b/sound/soc/Makefile
> > > @@ -1,6 +1,7 @@
> > >  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
> > >  snd-soc-core-objs += soc-topology.o
> > > +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
> > >
> > >  ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),)
> > >  snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git
> > > a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index
> > > 791953f..e559174 100644
> > > --- a/sound/soc/intel/Kconfig
> > > +++ b/sound/soc/intel/Kconfig
> > > @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
> > >
> > >  config SND_SST_MFLD_PLATFORM
> > >  	tristate
> > > +	select SND_SOC_COMPRESS
> > >
> > >  config SND_SST_IPC
> > >  	tristate
> > > --
> > > 1.9.1
> > >
> 

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, June 15, 2015 10:27 PM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam R;
> vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> At Mon, 15 Jun 2015 14:15:40 +0000,
> Jie, Yang wrote:
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Monday, June 15, 2015 7:34 PM
> > > To: Jie, Yang
> > > Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood, Liam
> > > R; vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang,
> > > Vivian
> > > Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item
> > > for soc- compress
> > >
> > > At Mon, 15 Jun 2015 11:20:44 +0800,
> > > 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@intel.com>
> > > > ---
> > > >  include/sound/soc.h     | 7 +++++++
> > > >  sound/soc/Kconfig       | 5 ++++-
> > > >  sound/soc/Makefile      | 3 ++-
> > > >  sound/soc/intel/Kconfig | 1 +
> > > >  4 files changed, 14 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/include/sound/soc.h b/include/sound/soc.h index
> > > > 93df8bf..cc1cd4f 100644
> > > > --- a/include/sound/soc.h
> > > > +++ b/include/sound/soc.h
> > > > @@ -440,7 +440,14 @@ 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);
> > > > +#else
> > > > +static inline int soc_new_compress(struct snd_soc_pcm_runtime
> > > > +*rtd, int num) {
> > > > +       return -EPERM;
> > > > +}
> > > > +#endif
> > >
> > > 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.
> >
> > Here I used -EPERM to return and tell caller that compress operation
> > is not permitted, does it make sense, Takashi?
> 
> The question is whether a runtime error is the best option.  A runtime error
> won't be caught by build tests but only when actually running by a user.
 
Unfortunately, here cpu_dai->driver->compress_dai is a runtime value,
which means we need compress API when it is true. Seems it is not easy
to decide it at compile stage?

> 
> > Or can you suggest a better solution, according to this specific case?
> 
> Well, there are several ways, obviously (e.g. give the preprocessor error if
> CONFIG_SND_SOC_COMPRESS is enabled).  But I'm not sure what is the best
 
Do you mean giving error, when CONFIG_SND_SOC_COMPRESS is disabled but
the driver wants the compress support at the same time?

As I stated above, it's not easy to detect whether the driver wants the compress
support, at the preprocessor/compile stage. :(

~Keyon

> to fit, too.  It's rather a matter of taste.
> 
> 
> Takashi
> 
> > Thanks,
> > ~Keyon
> > >
> > >
> > > Takashi
> > >
> > > >  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
> > > > e2828e1..a124759 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,10 @@ config SND_SOC_GENERIC_DMAENGINE_PCM
> > > >  	bool
> > > >  	select SND_DMAENGINE_PCM
> > > >
> > > > +config SND_SOC_COMPRESS
> > > > +	tristate
> > > > +	select SND_COMPRESS_OFFLOAD
> > > > +
> > > >  # All the supported SoCs
> > > >  source "sound/soc/adi/Kconfig"
> > > >  source "sound/soc/atmel/Kconfig"
> > > > diff --git a/sound/soc/Makefile b/sound/soc/Makefile index
> > > > a0e1ee6..184c1e6 100644
> > > > --- a/sound/soc/Makefile
> > > > +++ b/sound/soc/Makefile
> > > > @@ -1,6 +1,7 @@
> > > >  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
> > > >  snd-soc-core-objs += soc-topology.o
> > > > +snd-soc-core-$(CONFIG_SND_SOC_COMPRESS) += soc-compress.o
> > > >
> > > >  ifneq ($(CONFIG_SND_SOC_GENERIC_DMAENGINE_PCM),)
> > > >  snd-soc-core-objs += soc-generic-dmaengine-pcm.o diff --git
> > > > a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index
> > > > 791953f..e559174 100644
> > > > --- a/sound/soc/intel/Kconfig
> > > > +++ b/sound/soc/intel/Kconfig
> > > > @@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
> > > >
> > > >  config SND_SST_MFLD_PLATFORM
> > > >  	tristate
> > > > +	select SND_SOC_COMPRESS
> > > >
> > > >  config SND_SST_IPC
> > > >  	tristate
> > > > --
> > > > 1.9.1
> > > >
> >

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 951 bytes --]

On Mon, Jun 15, 2015 at 02:46:23PM +0000, Jie, Yang wrote:

> > > Here I used -EPERM to return and tell caller that compress operation
> > > is not permitted, does it make sense, Takashi?

> > The question is whether a runtime error is the best option.  A runtime error
> > won't be caught by build tests but only when actually running by a user.

> Unfortunately, here cpu_dai->driver->compress_dai is a runtime value,
> which means we need compress API when it is true. Seems it is not easy
> to decide it at compile stage?

The machine driver (which is presumably the thing that should be doing
the select here, it's not user visible) really ought to know if the DAI
links it is creating are for compressed audio.  This is why I'm still
surprised there's no driver updates as part of this patch.  If the Intel
drivers referencing compressed don't actually implement it then I'd
expect to see patches cleaning up the references to compressed audio.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/2] ASoC: soc-compress: split soc-compress to a module

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 320 bytes --]

On Mon, Jun 15, 2015 at 11:20:45AM +0800, Jie Yang wrote:
> Split soc-compress into a separate module so we only compile/load
> it when it's needed.

Given that this patch is so much simpler than the previous one it would
seem better to make it the first patch - I'd be OK to apply it but it
depends on the prior patch.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Jie, Yang]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Monday, June 15, 2015 11:06 PM
> To: Jie, Yang
> Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R;
> vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> On Mon, Jun 15, 2015 at 02:46:23PM +0000, Jie, Yang wrote:
> 
> > > > Here I used -EPERM to return and tell caller that compress
> > > > operation is not permitted, does it make sense, Takashi?
> 
> > > The question is whether a runtime error is the best option.  A
> > > runtime error won't be caught by build tests but only when actually
> running by a user.
> 
> > Unfortunately, here cpu_dai->driver->compress_dai is a runtime value,
> > which means we need compress API when it is true. Seems it is not easy
> > to decide it at compile stage?
> 
> The machine driver (which is presumably the thing that should be doing the
> select here, it's not user visible) really ought to know if the DAI links it is
> creating are for compressed audio.  This is why I'm still surprised there's no
> driver updates as part of this patch.  If the Intel drivers referencing
> compressed don't actually implement it then I'd expect to see patches
> cleaning up the references to compressed audio.
 
Mark, as you may see in this patch(1/2), I have added this selection to sound/
Soc/intel/Kconfig, we can find .compress_dai =1 in sst-mfld-platform-pcm.c,
the compressed is referenced in sst-mfld-platform-pcm.c:sst_platform_dai, 
here copy the related part from the patch:

b/sound/soc/intel/Kconfig index 791953f..e559174 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -12,6 +12,7 @@ config SND_MFLD_MACHINE
 
 config SND_SST_MFLD_PLATFORM
 	tristate
+	select SND_SOC_COMPRESS
 
 config SND_SST_IPC
 	tristate

~Keyon

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 399 bytes --]

On Tue, Jun 16, 2015 at 12:49:37AM +0000, Jie, Yang wrote:

> Mark, as you may see in this patch(1/2), I have added this selection to sound/
> Soc/intel/Kconfig, we can find .compress_dai =1 in sst-mfld-platform-pcm.c,
> the compressed is referenced in sst-mfld-platform-pcm.c:sst_platform_dai, 
> here copy the related part from the patch:

OK, that should have been in the commit message.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Jie, Yang]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Tuesday, June 16, 2015 6:54 PM
> To: Jie, Yang
> Cc: Takashi Iwai; alsa-devel@alsa-project.org; Girdwood, Liam R;
> vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> On Tue, Jun 16, 2015 at 12:49:37AM +0000, Jie, Yang wrote:
> 
> > Mark, as you may see in this patch(1/2), I have added this selection
> > to sound/ Soc/intel/Kconfig, we can find .compress_dai =1 in
> > sst-mfld-platform-pcm.c, the compressed is referenced in
> > sst-mfld-platform-pcm.c:sst_platform_dai,
> > here copy the related part from the patch:
> 
> OK, that should have been in the commit message.
 
OK, so let me add it to the commit message and resend?

~Keyon

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 555 bytes --]

On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:

> > OK, that should have been in the commit message.

> OK, so let me add it to the commit message and resend?

Well, there's still the stubs to consider.  Should we really be
returning an error or should we silently ignore the error and hide the
DAI if the user deconfigured compressed audio?  Or rearrange things so
we don't need stubs?  Given that the machine driver has to select
compressed support it's not something that should ever really hit the
stubs, it's not truly a runtime thing...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Takashi Iwai]

At Tue, 16 Jun 2015 17:06:07 +0100,
Mark Brown wrote:
> 
> On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> 
> > > OK, that should have been in the commit message.
> 
> > OK, so let me add it to the commit message and resend?
> 
> Well, there's still the stubs to consider.  Should we really be
> returning an error or should we silently ignore the error and hide the
> DAI if the user deconfigured compressed audio?  Or rearrange things so
> we don't need stubs?  Given that the machine driver has to select
> compressed support it's not something that should ever really hit the
> stubs, it's not truly a runtime thing...

Yes, I guess that leaving without dummy function will give unresolved
symbol errors at module link time, so the user can catch before
actually running it.  Of course, this should be checked actually.


Takashi

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Takashi Iwai]

At Tue, 16 Jun 2015 18:14:40 +0200,
Takashi Iwai wrote:
> 
> At Tue, 16 Jun 2015 17:06:07 +0100,
> Mark Brown wrote:
> > 
> > On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> > 
> > > > OK, that should have been in the commit message.
> > 
> > > OK, so let me add it to the commit message and resend?
> > 
> > Well, there's still the stubs to consider.  Should we really be
> > returning an error or should we silently ignore the error and hide the
> > DAI if the user deconfigured compressed audio?  Or rearrange things so
> > we don't need stubs?  Given that the machine driver has to select
> > compressed support it's not something that should ever really hit the
> > stubs, it's not truly a runtime thing...
> 
> Yes, I guess that leaving without dummy function will give unresolved
> symbol errors at module link time, so the user can catch before
> actually running it.  Of course, this should be checked actually.

Oh, scratch this.  It won't work well in the current code.
Possibly with weak linking, but...


Takashi

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Vinod Koul]

On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
> At Tue, 16 Jun 2015 18:14:40 +0200,
> Takashi Iwai wrote:
> > 
> > At Tue, 16 Jun 2015 17:06:07 +0100,
> > Mark Brown wrote:
> > > 
> > > On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> > > 
> > > > > OK, that should have been in the commit message.
> > > 
> > > > OK, so let me add it to the commit message and resend?
> > > 
> > > Well, there's still the stubs to consider.  Should we really be
> > > returning an error or should we silently ignore the error and hide the
> > > DAI if the user deconfigured compressed audio?  Or rearrange things so
> > > we don't need stubs?  Given that the machine driver has to select
> > > compressed support it's not something that should ever really hit the
> > > stubs, it's not truly a runtime thing...
> > 
> > Yes, I guess that leaving without dummy function will give unresolved
> > symbol errors at module link time, so the user can catch before
> > actually running it.  Of course, this should be checked actually.
> 
> Oh, scratch this.  It won't work well in the current code.
> Possibly with weak linking, but...
Right as the soc_new_compress() is called from soc-core for compressed dai
links. Since ASoC drivers don't use any of compress APIs directly, it would
be difficult to add a compile time failure so we are stuck with silent
runtime failures :(

-- 
~Vinod

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Qais Yousef]

On 06/16/2015 05:17 PM, Takashi Iwai wrote:
> At Tue, 16 Jun 2015 18:14:40 +0200,
> Takashi Iwai wrote:
>> At Tue, 16 Jun 2015 17:06:07 +0100,
>> Mark Brown wrote:
>>> On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
>>>
>>>>> OK, that should have been in the commit message.
>>>> OK, so let me add it to the commit message and resend?
>>> Well, there's still the stubs to consider.  Should we really be
>>> returning an error or should we silently ignore the error and hide the
>>> DAI if the user deconfigured compressed audio?  Or rearrange things so
>>> we don't need stubs?  Given that the machine driver has to select
>>> compressed support it's not something that should ever really hit the
>>> stubs, it's not truly a runtime thing...
>> Yes, I guess that leaving without dummy function will give unresolved
>> symbol errors at module link time, so the user can catch before
>> actually running it.  Of course, this should be checked actually.
> Oh, scratch this.  It won't work well in the current code.
> Possibly with weak linking, but...

I don't know if I am missing something, but am I the only one who thinks 
these patches and the proposed changes are too complex for what seems to 
be little gain? Why is it that bad to always compile in soc-compress?

Qais

>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Takashi Iwai]

At Tue, 16 Jun 2015 21:55:07 +0530,
Vinod Koul wrote:
> 
> On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
> > At Tue, 16 Jun 2015 18:14:40 +0200,
> > Takashi Iwai wrote:
> > > 
> > > At Tue, 16 Jun 2015 17:06:07 +0100,
> > > Mark Brown wrote:
> > > > 
> > > > On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> > > > 
> > > > > > OK, that should have been in the commit message.
> > > > 
> > > > > OK, so let me add it to the commit message and resend?
> > > > 
> > > > Well, there's still the stubs to consider.  Should we really be
> > > > returning an error or should we silently ignore the error and hide the
> > > > DAI if the user deconfigured compressed audio?  Or rearrange things so
> > > > we don't need stubs?  Given that the machine driver has to select
> > > > compressed support it's not something that should ever really hit the
> > > > stubs, it's not truly a runtime thing...
> > > 
> > > Yes, I guess that leaving without dummy function will give unresolved
> > > symbol errors at module link time, so the user can catch before
> > > actually running it.  Of course, this should be checked actually.
> > 
> > Oh, scratch this.  It won't work well in the current code.
> > Possibly with weak linking, but...
> Right as the soc_new_compress() is called from soc-core for compressed dai
> links. Since ASoC drivers don't use any of compress APIs directly, it would
> be difficult to add a compile time failure so we are stuck with silent
> runtime failures :(

One feasible option would be to make the driver's dai creation a
function pointer to the generic constructor.  That is,
soc_probe_link_dais() will run like:

	if (cpu_dai->driver->create)
		ret = cpu_dai->driver->create(rtd, num);

and each driver passes soc_new_compress (or whatever) there.  This is
flexible and can be extended if any new stuff has to be handled.
Even PCM creation or dai widget links can be passed in that form,
too.


Takashi

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Vinod Koul]

On Tue, Jun 16, 2015 at 06:36:22PM +0200, Takashi Iwai wrote:
> At Tue, 16 Jun 2015 21:55:07 +0530,
> Vinod Koul wrote:
> > 
> > On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
> > > At Tue, 16 Jun 2015 18:14:40 +0200,
> > > Takashi Iwai wrote:
> > > > 
> > > > At Tue, 16 Jun 2015 17:06:07 +0100,
> > > > Mark Brown wrote:
> > > > > 
> > > > > On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> > > > > 
> > > > > > > OK, that should have been in the commit message.
> > > > > 
> > > > > > OK, so let me add it to the commit message and resend?
> > > > > 
> > > > > Well, there's still the stubs to consider.  Should we really be
> > > > > returning an error or should we silently ignore the error and hide the
> > > > > DAI if the user deconfigured compressed audio?  Or rearrange things so
> > > > > we don't need stubs?  Given that the machine driver has to select
> > > > > compressed support it's not something that should ever really hit the
> > > > > stubs, it's not truly a runtime thing...
> > > > 
> > > > Yes, I guess that leaving without dummy function will give unresolved
> > > > symbol errors at module link time, so the user can catch before
> > > > actually running it.  Of course, this should be checked actually.
> > > 
> > > Oh, scratch this.  It won't work well in the current code.
> > > Possibly with weak linking, but...
> > Right as the soc_new_compress() is called from soc-core for compressed dai
> > links. Since ASoC drivers don't use any of compress APIs directly, it would
> > be difficult to add a compile time failure so we are stuck with silent
> > runtime failures :(
> 
> One feasible option would be to make the driver's dai creation a
> function pointer to the generic constructor.  That is,
> soc_probe_link_dais() will run like:
> 
> 	if (cpu_dai->driver->create)
> 		ret = cpu_dai->driver->create(rtd, num);
> 
> and each driver passes soc_new_compress (or whatever) there.  This is
> flexible and can be extended if any new stuff has to be handled.
> Even PCM creation or dai widget links can be passed in that form,
> too.
and the default would be snd_pcm_new so that *most* drivers dont need to add
this. Only Intel atom driver should add soc_new_compress() as create ops,
hence direct reference so no gaurds required

Looks good to me then, Mark you okay with this approach?

-- 
~Vinod

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, June 17, 2015 12:36 AM
> To: Koul, Vinod
> Cc: Mark Brown; Jie, Yang; alsa-devel@alsa-project.org; Girdwood, Liam R;
> ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> At Tue, 16 Jun 2015 21:55:07 +0530,
> Vinod Koul wrote:
> >
> > On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
> > > At Tue, 16 Jun 2015 18:14:40 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
> > > > >
> > > > > On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> > > > >
> > > > > > > OK, that should have been in the commit message.
> > > > >
> > > > > > OK, so let me add it to the commit message and resend?
> > > > >
> > > > > Well, there's still the stubs to consider.  Should we really be
> > > > > returning an error or should we silently ignore the error and
> > > > > hide the DAI if the user deconfigured compressed audio?  Or
> > > > > rearrange things so we don't need stubs?  Given that the machine
> > > > > driver has to select compressed support it's not something that
> > > > > should ever really hit the stubs, it's not truly a runtime thing...
> > > >
> > > > Yes, I guess that leaving without dummy function will give
> > > > unresolved symbol errors at module link time, so the user can
> > > > catch before actually running it.  Of course, this should be checked
> actually.
> > >
> > > Oh, scratch this.  It won't work well in the current code.
> > > Possibly with weak linking, but...
> > Right as the soc_new_compress() is called from soc-core for compressed
> > dai links. Since ASoC drivers don't use any of compress APIs directly,
> > it would be difficult to add a compile time failure so we are stuck
> > with silent runtime failures :(
> 
> One feasible option would be to make the driver's dai creation a function
> pointer to the generic constructor.  That is,
> soc_probe_link_dais() will run like:
> 
> 	if (cpu_dai->driver->create)
> 		ret = cpu_dai->driver->create(rtd, num);
> 
> and each driver passes soc_new_compress (or whatever) there.  This is
> flexible and can be extended if any new stuff has to be handled.
> Even PCM creation or dai widget links can be passed in that form, too.
 
Then we may need this in driver code:

static struct snd_soc_dai_driver sst_platform_dai[] = {
...,
{
	.name = "compress-cpu-dai",
	.create = soc_new_compress, //.compress_dai = 1,
//	.create = NULL, //.compress_dai = 0,
	.ops = &sst_compr_dai_ops,
	.playback = {
		.stream_name = "Compress Playback",
		.channels_min = SST_STEREO,
		.channels_max = SST_STEREO,
		.rates = SNDRV_PCM_RATE_48000,
		.formats = SNDRV_PCM_FMTBIT_S16_LE,
	},
},
}

With  this implementation, for driver which needed compress,
if we forget to add 'select SND_SOC_COMPRESS' in Kconfig of the
driver, compiler will check and report error(no NULL inline function
defined).

Sounds this feasible for me, any extra comment for it?

~Keyon

> 
> 
> Takashi

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1400 bytes --]

On Wed, Jun 17, 2015 at 08:24:48AM +0530, Vinod Koul wrote:
> On Tue, Jun 16, 2015 at 06:36:22PM +0200, Takashi Iwai wrote:

Please remember to delete unneeded context from replies. 

> > 	if (cpu_dai->driver->create)
> > 		ret = cpu_dai->driver->create(rtd, num);


> > and each driver passes soc_new_compress (or whatever) there.  This is
> > flexible and can be extended if any new stuff has to be handled.
> > Even PCM creation or dai widget links can be passed in that form,
> > too.

> and the default would be snd_pcm_new so that *most* drivers dont need to add
> this. Only Intel atom driver should add soc_new_compress() as create ops,
> hence direct reference so no gaurds required

> Looks good to me then, Mark you okay with this approach?

I'm not sure this fully helps.  This will put the reference in the CPU
driver which if there's sharing with non-compressed DAIs means that any
machine driver using the platform needs to select the compressed audio
code.  It also means that we're still in the situation whre if a machine
*can* support compressed audio it *must* support compressed audio, it's
not clear to me that people doing this sort of memory optimisation are
always going to be doing it on hardware that doesn't have the capacity
for compressed audio (or won't in the future).  But then I don't know
exactly how much memory is being saved here...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Vinod Koul]

On Wed, Jun 17, 2015 at 01:23:40PM +0100, Mark Brown wrote:
> On Wed, Jun 17, 2015 at 08:24:48AM +0530, Vinod Koul wrote:
> > > 	if (cpu_dai->driver->create)
> > > 		ret = cpu_dai->driver->create(rtd, num);
> 
> 
> > > and each driver passes soc_new_compress (or whatever) there.  This is
> > > flexible and can be extended if any new stuff has to be handled.
> > > Even PCM creation or dai widget links can be passed in that form,
> > > too.
> 
> > and the default would be snd_pcm_new so that *most* drivers dont need to add
> > this. Only Intel atom driver should add soc_new_compress() as create ops,
> > hence direct reference so no gaurds required
> 
> > Looks good to me then, Mark you okay with this approach?
> 
> I'm not sure this fully helps.  This will put the reference in the CPU
> driver which if there's sharing with non-compressed DAIs means that any
> machine driver using the platform needs to select the compressed audio
> code.  It also means that we're still in the situation whre if a machine
> *can* support compressed audio it *must* support compressed audio, it's
> not clear to me that people doing this sort of memory optimisation are
> always going to be doing it on hardware that doesn't have the capacity
> for compressed audio (or won't in the future).  But then I don't know
> exactly how much memory is being saved here...
Yes this is a good point but then driver can define the compressed dai only
when SND_SOC_COMPRESS is enabled. So if machine has enabled this symbol then
only dai get added and machine can create dai-link

Also for the size question, in sound-next we have 248KB soc-compress.o

Thanks
-- 
~Vinod

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1332 bytes --]

On Wed, Jun 17, 2015 at 09:52:56PM +0530, Vinod Koul wrote:
> On Wed, Jun 17, 2015 at 01:23:40PM +0100, Mark Brown wrote:

> > I'm not sure this fully helps.  This will put the reference in the CPU
> > driver which if there's sharing with non-compressed DAIs means that any
> > machine driver using the platform needs to select the compressed audio
> > code.  It also means that we're still in the situation whre if a machine
> > *can* support compressed audio it *must* support compressed audio, it's
> > not clear to me that people doing this sort of memory optimisation are
> > always going to be doing it on hardware that doesn't have the capacity
> > for compressed audio (or won't in the future).  But then I don't know
> > exactly how much memory is being saved here...

> Yes this is a good point but then driver can define the compressed dai only
> when SND_SOC_COMPRESS is enabled. So if machine has enabled this symbol then
> only dai get added and machine can create dai-link

Well, if it's a user configurable thing then the stubs start to make
sense - the driver defines the DAI and then the core doesn't bother to
instantiate it.

> Also for the size question, in sound-next we have 248KB soc-compress.o

How big is the actual code - that sounds like it's got the debug
symbols?  I suppose I should go look myself...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Jie, Yang]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-
> bounces@alsa-project.org] On Behalf Of Mark Brown
> Sent: Wednesday, June 17, 2015 8:24 PM
> To: Koul, Vinod
> Cc: Takashi Iwai; alsa-devel@alsa-project.org; Zhang, Vivian; Babu, Ramesh;
> Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: soc-compress: add a config
> item for soc-compress
> 
> On Wed, Jun 17, 2015 at 08:24:48AM +0530, Vinod Koul wrote:
> > On Tue, Jun 16, 2015 at 06:36:22PM +0200, Takashi Iwai wrote:
> 
> Please remember to delete unneeded context from replies.
> 
> > > 	if (cpu_dai->driver->create)
> > > 		ret = cpu_dai->driver->create(rtd, num);
> 
> 
> > > and each driver passes soc_new_compress (or whatever) there.  This
> > > is flexible and can be extended if any new stuff has to be handled.
> > > Even PCM creation or dai widget links can be passed in that form,
> > > too.
> 
> > and the default would be snd_pcm_new so that *most* drivers dont need
> > to add this. Only Intel atom driver should add soc_new_compress() as
> > create ops, hence direct reference so no gaurds required
> 
> > Looks good to me then, Mark you okay with this approach?
> 
> I'm not sure this fully helps.  This will put the reference in the CPU driver
> which if there's sharing with non-compressed DAIs means that any machine
> driver using the platform needs to select the compressed audio code.  It also
> means that we're still in the situation whre if a machine
> *can* support compressed audio it *must* support compressed audio, it's
> not clear to me that people doing this sort of memory optimisation are
> always going to be doing it on hardware that doesn't have the capacity for
> compressed audio (or won't in the future).  But then I don't know exactly
> how much memory is being saved here...

It saves ~12KB memory usage on i386 platform on my side.
 
~Keyon

Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-compress

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, June 17, 2015 12:36 AM
> To: Koul, Vinod
> Cc: Mark Brown; Jie, Yang; alsa-devel@alsa-project.org; Girdwood, Liam R;
> ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH v2 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> At Tue, 16 Jun 2015 21:55:07 +0530,
> Vinod Koul wrote:
> >
> > On Tue, Jun 16, 2015 at 06:17:12PM +0200, Takashi Iwai wrote:
> > > At Tue, 16 Jun 2015 18:14:40 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > At Tue, 16 Jun 2015 17:06:07 +0100, Mark Brown wrote:
> > > > >
> > > > > On Tue, Jun 16, 2015 at 12:36:16PM +0000, Jie, Yang wrote:
> > > > >
> > > > > > > OK, that should have been in the commit message.
> > > > >
> > > > > > OK, so let me add it to the commit message and resend?
> > > > >
> > > > > Well, there's still the stubs to consider.  Should we really be
> > > > > returning an error or should we silently ignore the error and
> > > > > hide the DAI if the user deconfigured compressed audio?  Or
> > > > > rearrange things so we don't need stubs?  Given that the machine
> > > > > driver has to select compressed support it's not something that
> > > > > should ever really hit the stubs, it's not truly a runtime thing...
> > > >
> > > > Yes, I guess that leaving without dummy function will give
> > > > unresolved symbol errors at module link time, so the user can
> > > > catch before actually running it.  Of course, this should be checked
> actually.
> > >
> > > Oh, scratch this.  It won't work well in the current code.
> > > Possibly with weak linking, but...
> > Right as the soc_new_compress() is called from soc-core for compressed
> > dai links. Since ASoC drivers don't use any of compress APIs directly,
> > it would be difficult to add a compile time failure so we are stuck
> > with silent runtime failures :(
> 
> One feasible option would be to make the driver's dai creation a function
> pointer to the generic constructor.  That is,
> soc_probe_link_dais() will run like:
> 
> 	if (cpu_dai->driver->create)
> 		ret = cpu_dai->driver->create(rtd, num);
> 
> and each driver passes soc_new_compress (or whatever) there.  This is
> flexible and can be extended if any new stuff has to be handled.
> Even PCM creation or dai widget links can be passed in that form, too.
 
How time flies! About 3 months passed away when I have chance to resume back
to this thread.

Takashi, so as your suggestion, I will add a member to struct snd_soc_dai_driver:

struct snd_soc_dai_driver {
...
+	int (*create)(struct snd_soc_dai *dai);
-	bool compress_dai;
...
}

And then change sst_platform_dai[] in sst-mfld-platform-pcm.c such as:

{
        .name = "compress-cpu-dai",
-        .compress_dai = 1,
+     .create = soc_new_compress,
        .ops = &sst_compr_dai_ops,
        .playback = {
                .stream_name = "Compress Playback",
                .channels_min = SST_STEREO,
                .channels_max = SST_STEREO,
                .rates = SNDRV_PCM_RATE_48000,
                .formats = SNDRV_PCM_FMTBIT_S16_LE,
        },
},

And then, we can remove the dummy soc_new_compress() inline definition,
is that right?

~Keyon

> 
> 
> Takashi