[PATCH v3 0/2] make it possible to not compile compress APIs

[PATCH v3 0/2] make it possible to not compile compress APIs

[Jie Yang]

Compress APIs are not always needed, but they are always built currentlly,
in both ALSA core and ASoC core. 

This series make it possible to not compile it when unneeded.

Hi, Takashi & all,

I am not sure you will like the patch 2/2, it split soc-compress to a
module, but introduced several extra 'EXPORT_SYMBOL_GPL' there. If don't
like it, you can just apply the first one(1/2) and skip 2/2.

thanks,
~Keyon

Changes in this version:
1. change to use a callback for compress_new, and remove compress_dai
bool.
2. remove the empty module_init/exit for soc-compress module.

Jie Yang (2):
  ASoC: soc-compress: add a config item for soc-compress
  ASoC: soc-compress: split soc-compress to a module

 include/sound/soc-dai.h                      |  2 +-
 include/sound/soc.h                          |  2 ++
 sound/soc/Kconfig                            |  6 +++++-
 sound/soc/Makefile                           |  5 ++++-
 sound/soc/intel/Kconfig                      |  1 +
 sound/soc/intel/atom/sst-mfld-platform-pcm.c |  2 +-
 sound/soc/soc-compress.c                     | 10 ++++++++++
 sound/soc/soc-core.c                         |  4 ++--
 sound/soc/soc-dapm.c                         |  1 +
 sound/soc/soc-pcm.c                          | 13 +++++++++++++
 10 files changed, 40 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH v3 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-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
 
 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
+
 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
 
 ifneq ($(CONFIG_SND_SOC_TOPOLOGY),)
 snd-soc-core-objs += soc-topology.o
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 05fde5e6e..221e3bd 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
diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index 5e9c316..cb1dd50 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -512,7 +512,7 @@ static struct snd_soc_dai_driver sst_platform_dai[] = {
 },
 {
 	.name = "compress-cpu-dai",
-	.compress_dai = 1,
+	.compress_new = soc_new_compress,
 	.ops = &sst_compr_dai_ops,
 	.playback = {
 		.stream_name = "Compress Playback",
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);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 3b471f9..24b0960 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1370,9 +1370,9 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
 		soc_dpcm_debugfs_add(rtd);
 #endif
 
-	if (cpu_dai->driver->compress_dai) {
+	if (cpu_dai->driver->compress_new) {
 		/*create compress_device"*/
-		ret = soc_new_compress(rtd, num);
+		ret = cpu_dai->driver->compress_new(rtd, num);
 		if (ret < 0) {
 			dev_err(card->dev, "ASoC: can't create compress %s\n",
 					 dai_link->stream_name);
-- 
1.9.1

[PATCH v3 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       |  5 ++---
 sound/soc/soc-compress.c |  9 +++++++++
 sound/soc/soc-dapm.c     |  1 +
 sound/soc/soc-pcm.c      | 13 +++++++++++++
 4 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index a3a1505..dacddf4 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -1,9 +1,8 @@
 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
 
-ifneq ($(CONFIG_SND_SOC_COMPRESS),)
-snd-soc-core-objs += soc-compress.o
-endif
+snd-soc-compress-objs := soc-compress.o
+obj-$(CONFIG_SND_SOC_COMPRESS) += snd-soc-compress.o
 
 ifneq ($(CONFIG_SND_SOC_TOPOLOGY),)
 snd-soc-core-objs += soc-topology.o
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index a672d9c..28bb1b2 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>
@@ -704,3 +705,11 @@ compr_err:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(soc_new_compress);
+
+/* Module information */
+MODULE_AUTHOR("Namarta Kohli <namartax.kohli@intel.com>");
+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");
+MODULE_ALIAS("platform:soc-compress");
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index ff8bda4..9e32151 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3896,6 +3896,7 @@ void snd_soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream,
 	soc_dapm_stream_event(rtd, stream, event);
 	mutex_unlock(&card->dapm_mutex);
 }
+EXPORT_SYMBOL_GPL(snd_soc_dapm_stream_event);
 
 /**
  * snd_soc_dapm_enable_pin_unlocked - enable pin.
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 3173958..517a9d4 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -86,6 +86,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
 		rtd->codec_dais[i]->component->active++;
 	}
 }
+EXPORT_SYMBOL_GPL(snd_soc_runtime_activate);
 
 /**
  * snd_soc_runtime_deactivate() - Decrement active count for PCM runtime components
@@ -121,6 +122,7 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
 		rtd->codec_dais[i]->active--;
 	}
 }
+EXPORT_SYMBOL_GPL(snd_soc_runtime_deactivate);
 
 /**
  * snd_soc_runtime_ignore_pmdown_time() - Check whether to ignore the power down delay
@@ -144,6 +146,7 @@ bool snd_soc_runtime_ignore_pmdown_time(struct snd_soc_pcm_runtime *rtd)
 
 	return rtd->cpu_dai->component->ignore_pmdown_time && ignore;
 }
+EXPORT_SYMBOL_GPL(snd_soc_runtime_ignore_pmdown_time);
 
 /**
  * snd_soc_set_runtime_hwparams - set the runtime hardware parameters
@@ -188,6 +191,7 @@ int dpcm_dapm_stream_event(struct snd_soc_pcm_runtime *fe, int dir,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(dpcm_dapm_stream_event);
 
 static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream,
 					struct snd_soc_dai *soc_dai)
@@ -1209,6 +1213,7 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
 		kfree(dpcm);
 	}
 }
+EXPORT_SYMBOL_GPL(dpcm_be_disconnect);
 
 /* get BE for DAI widget and stream */
 static struct snd_soc_pcm_runtime *dpcm_get_be(struct snd_soc_card *card,
@@ -1293,6 +1298,7 @@ int dpcm_path_get(struct snd_soc_pcm_runtime *fe,
 
 	return paths;
 }
+EXPORT_SYMBOL_GPL(dpcm_path_get);
 
 static int dpcm_prune_paths(struct snd_soc_pcm_runtime *fe, int stream,
 	struct snd_soc_dapm_widget_list **list_)
@@ -1405,6 +1411,7 @@ int dpcm_process_paths(struct snd_soc_pcm_runtime *fe,
 	else
 		return dpcm_prune_paths(fe, stream, list);
 }
+EXPORT_SYMBOL_GPL(dpcm_process_paths);
 
 void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream)
 {
@@ -1414,6 +1421,7 @@ void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream)
 		dpcm->be->dpcm[stream].runtime_update =
 						SND_SOC_DPCM_UPDATE_NO;
 }
+EXPORT_SYMBOL_GPL(dpcm_clear_pending_state);
 
 static void dpcm_be_dai_startup_unwind(struct snd_soc_pcm_runtime *fe,
 	int stream)
@@ -1530,6 +1538,7 @@ unwind:
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(dpcm_be_dai_startup);
 
 static void dpcm_init_runtime_hw(struct snd_pcm_runtime *runtime,
 				 struct snd_soc_pcm_stream *stream,
@@ -1693,6 +1702,7 @@ int dpcm_be_dai_shutdown(struct snd_soc_pcm_runtime *fe, int stream)
 	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(dpcm_be_dai_shutdown);
 
 static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
 {
@@ -1758,6 +1768,7 @@ int dpcm_be_dai_hw_free(struct snd_soc_pcm_runtime *fe, int stream)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(dpcm_be_dai_hw_free);
 
 static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
 {
@@ -1865,6 +1876,7 @@ unwind:
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(dpcm_be_dai_hw_params);
 
 static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
 				 struct snd_pcm_hw_params *params)
@@ -2134,6 +2146,7 @@ int dpcm_be_dai_prepare(struct snd_soc_pcm_runtime *fe, int stream)
 	}
 	return ret;
 }
+EXPORT_SYMBOL_GPL(dpcm_be_dai_prepare);
 
 static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
 {
-- 
1.9.1

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

[Liam Girdwood]

+ Vinod,

On Tue, 2015-10-13 at 17:11 +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.

We will need to identify the existing upstream compress users and add
this option to their Kconfig as part of this patch.

Liam 

> 
> Signed-off-by: Jie Yang <yang.jie@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
>  
>  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
> +
>  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
>  
>  ifneq ($(CONFIG_SND_SOC_TOPOLOGY),)
>  snd-soc-core-objs += soc-topology.o
> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index 05fde5e6e..221e3bd 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
> diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> index 5e9c316..cb1dd50 100644
> --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> @@ -512,7 +512,7 @@ static struct snd_soc_dai_driver sst_platform_dai[] = {
>  },
>  {
>  	.name = "compress-cpu-dai",
> -	.compress_dai = 1,
> +	.compress_new = soc_new_compress,
>  	.ops = &sst_compr_dai_ops,
>  	.playback = {
>  		.stream_name = "Compress Playback",
> 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);
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 3b471f9..24b0960 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1370,9 +1370,9 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
>  		soc_dpcm_debugfs_add(rtd);
>  #endif
>  
> -	if (cpu_dai->driver->compress_dai) {
> +	if (cpu_dai->driver->compress_new) {
>  		/*create compress_device"*/
> -		ret = soc_new_compress(rtd, num);
> +		ret = cpu_dai->driver->compress_new(rtd, num);
>  		if (ret < 0) {
>  			dev_err(card->dev, "ASoC: can't create compress %s\n",
>  					 dai_link->stream_name);


---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

[Takashi Iwai]

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@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.

Also, as a namespace issue, we usually keep snd_ prefix for exported
functions.


>  
>  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.


> +
>  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


>  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.


thanks,

Takashi

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

[Koul, Vinod]

On Tue, 2015-10-13 at 17:11 +0800, Jie Yang wrote:
> +/* Module information */
> +MODULE_AUTHOR("Namarta Kohli <namartax.kohli@intel.com>");
> +MODULE_AUTHOR("Ramesh Babu K V <ramesh.babu@linux.intel.com>");
> +MODULE_AUTHOR("Vinod Koul <vinod.koul@linux.intel.com>");

Vinod Koul <vinod.koul@intel.com> please

I need to move other instances too :(


> +MODULE_DESCRIPTION("ALSA SoC Compress");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:soc-compress");
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index ff8bda4..9e32151 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -3896,6 +3896,7 @@ void snd_soc_dapm_stream_event(struct snd_soc_pcm_runtime
> *rtd, int stream,
>  	soc_dapm_stream_event(rtd, stream, event);
>  	mutex_unlock(&card->dapm_mutex);
>  }
> +EXPORT_SYMBOL_GPL(snd_soc_dapm_stream_event);

These exports should be a new patch

And as we agreed this should have Documentation if not already done :)

-- 
~Vinod

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

[Jie, Yang]

> -----Original Message-----
> From: Girdwood, Liam R
> Sent: Tuesday, October 13, 2015 5:25 PM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org; Koul, Vinod
> Subject: Re: [PATCH v3 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> + Vinod,
> 
> On Tue, 2015-10-13 at 17:11 +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.
> 
> We will need to identify the existing upstream compress users and add this
> option to their Kconfig as part of this patch.

Have checked that there is only one user(sst-mfld-platform-pcm.c ) for it,
and it's Kconfig change already in this patch.

~Keyon

> 
> Liam
> 
> >
> > Signed-off-by: Jie Yang <yang.jie@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
> >
> >  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
> > +
> >  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
> >
> >  ifneq ($(CONFIG_SND_SOC_TOPOLOGY),)
> >  snd-soc-core-objs += soc-topology.o
> > diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index
> > 05fde5e6e..221e3bd 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
> > diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> > b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> > index 5e9c316..cb1dd50 100644
> > --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> > +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> > @@ -512,7 +512,7 @@ static struct snd_soc_dai_driver
> > sst_platform_dai[] = {  },  {
> >  	.name = "compress-cpu-dai",
> > -	.compress_dai = 1,
> > +	.compress_new = soc_new_compress,
> >  	.ops = &sst_compr_dai_ops,
> >  	.playback = {
> >  		.stream_name = "Compress Playback", 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);
> > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index
> > 3b471f9..24b0960 100644
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> > @@ -1370,9 +1370,9 @@ static int soc_probe_link_dais(struct
> snd_soc_card *card, int num, int order)
> >  		soc_dpcm_debugfs_add(rtd);
> >  #endif
> >
> > -	if (cpu_dai->driver->compress_dai) {
> > +	if (cpu_dai->driver->compress_new) {
> >  		/*create compress_device"*/
> > -		ret = soc_new_compress(rtd, num);
> > +		ret = cpu_dai->driver->compress_new(rtd, num);
> >  		if (ret < 0) {
> >  			dev_err(card->dev, "ASoC: can't create
> compress %s\n",
> >  					 dai_link->stream_name);
> 

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

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, October 13, 2015 5:33 PM
> To: Jie, Yang
> Cc: broonie@kernel.org; alsa-devel@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@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

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

[Jie, Yang]

> -----Original Message-----
> From: Koul, Vinod
> Sent: Tuesday, October 13, 2015 7:50 PM
> To: Jie, Yang; broonie@kernel.org
> Cc: Girdwood, Liam R; alsa-devel@alsa-project.org
> Subject: Re: [alsa-devel] [PATCH v3 2/2] ASoC: soc-compress: split soc-
> compress to a module
> 
> On Tue, 2015-10-13 at 17:11 +0800, Jie Yang wrote:
> > +/* Module information */
> > +MODULE_AUTHOR("Namarta Kohli <namartax.kohli@intel.com>");
> > +MODULE_AUTHOR("Ramesh Babu K V <ramesh.babu@linux.intel.com>");
> > +MODULE_AUTHOR("Vinod Koul <vinod.koul@linux.intel.com>");
> 
> Vinod Koul <vinod.koul@intel.com> please
> 

OK, I will change this one. 

> I need to move other instances too :(
> 
> 
> > +MODULE_DESCRIPTION("ALSA SoC Compress");
> MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:soc-compress");
> > diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index
> > ff8bda4..9e32151 100644
> > --- a/sound/soc/soc-dapm.c
> > +++ b/sound/soc/soc-dapm.c
> > @@ -3896,6 +3896,7 @@ void snd_soc_dapm_stream_event(struct
> > snd_soc_pcm_runtime *rtd, int stream,
> >  	soc_dapm_stream_event(rtd, stream, event);
> >  	mutex_unlock(&card->dapm_mutex);
> >  }
> > +EXPORT_SYMBOL_GPL(snd_soc_dapm_stream_event);
> 
> These exports should be a new patch
> 
> And as we agreed this should have Documentation if not already done :)

It's not what I really want to do, it introduce 13 exports
(EXPORT_SYMBOL_GPL) when we want split soc-compress.c to a separate
Module, and adding documentation to all of these 13 functions(only for
soc-compress.c to use them) looks somewhat ungraceful :(

Hi Takashi, what's your opinion? Maybe we should keep soc-compress.c
in snd-soc-core.ko, which means skip/ignore this patch and don’t split it?

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

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

[Liam Girdwood]

On Tue, 2015-10-13 at 13:42 +0100, Jie, Yang wrote:
> > -----Original Message-----
> > From: Koul, Vinod
> > Sent: Tuesday, October 13, 2015 7:50 PM
> > To: Jie, Yang; broonie@kernel.org
> > Cc: Girdwood, Liam R; alsa-devel@alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH v3 2/2] ASoC: soc-compress: split soc-
> > compress to a module
> > 
> > On Tue, 2015-10-13 at 17:11 +0800, Jie Yang wrote:
> > > +/* Module information */
> > > +MODULE_AUTHOR("Namarta Kohli <namartax.kohli@intel.com>");
> > > +MODULE_AUTHOR("Ramesh Babu K V <ramesh.babu@linux.intel.com>");
> > > +MODULE_AUTHOR("Vinod Koul <vinod.koul@linux.intel.com>");
> > 
> > Vinod Koul <vinod.koul@intel.com> please
> > 
> 
> OK, I will change this one. 
> 
> > I need to move other instances too :(
> > 
> > 
> > > +MODULE_DESCRIPTION("ALSA SoC Compress");
> > MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:soc-compress");
> > > diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index
> > > ff8bda4..9e32151 100644
> > > --- a/sound/soc/soc-dapm.c
> > > +++ b/sound/soc/soc-dapm.c
> > > @@ -3896,6 +3896,7 @@ void snd_soc_dapm_stream_event(struct
> > > snd_soc_pcm_runtime *rtd, int stream,
> > >  	soc_dapm_stream_event(rtd, stream, event);
> > >  	mutex_unlock(&card->dapm_mutex);
> > >  }
> > > +EXPORT_SYMBOL_GPL(snd_soc_dapm_stream_event);
> > 
> > These exports should be a new patch
> > 
> > And as we agreed this should have Documentation if not already done :)
> 
> It's not what I really want to do, it introduce 13 exports
> (EXPORT_SYMBOL_GPL) when we want split soc-compress.c to a separate
> Module, and adding documentation to all of these 13 functions(only for
> soc-compress.c to use them) looks somewhat ungraceful :(
> 

If these are internal functions only (and used only by one user) then
adding some small kernel doc comments above each function should be
enough.

Liam

> Hi Takashi, what's your opinion? Maybe we should keep soc-compress.c
> in snd-soc-core.ko, which means skip/ignore this patch and don’t split it?
> 
> ~Keyon
> > 
> > --
> > ~Vinod


---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

On Tue, 13 Oct 2015 14:31:39 +0200,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Tuesday, October 13, 2015 5:33 PM
> > To: Jie, Yang
> > Cc: broonie@kernel.org; alsa-devel@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@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'

Ah, indeed, I'm now convinced by old me ;)


> > 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. :(

Hm, strange.  But this change itself isn't too serious as you'll
change again in the next patch.


Takashi

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

[Takashi Iwai]

On Tue, 13 Oct 2015 14:42:11 +0200,
Jie, Yang wrote:
> 
> > -----Original Message-----
> > From: Koul, Vinod
> > Sent: Tuesday, October 13, 2015 7:50 PM
> > To: Jie, Yang; broonie@kernel.org
> > Cc: Girdwood, Liam R; alsa-devel@alsa-project.org
> > Subject: Re: [alsa-devel] [PATCH v3 2/2] ASoC: soc-compress: split soc-
> > compress to a module
> > 
> > On Tue, 2015-10-13 at 17:11 +0800, Jie Yang wrote:
> > > +/* Module information */
> > > +MODULE_AUTHOR("Namarta Kohli <namartax.kohli@intel.com>");
> > > +MODULE_AUTHOR("Ramesh Babu K V <ramesh.babu@linux.intel.com>");
> > > +MODULE_AUTHOR("Vinod Koul <vinod.koul@linux.intel.com>");
> > 
> > Vinod Koul <vinod.koul@intel.com> please
> > 
> 
> OK, I will change this one. 
> 
> > I need to move other instances too :(
> > 
> > 
> > > +MODULE_DESCRIPTION("ALSA SoC Compress");
> > MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:soc-compress");
> > > diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index
> > > ff8bda4..9e32151 100644
> > > --- a/sound/soc/soc-dapm.c
> > > +++ b/sound/soc/soc-dapm.c
> > > @@ -3896,6 +3896,7 @@ void snd_soc_dapm_stream_event(struct
> > > snd_soc_pcm_runtime *rtd, int stream,
> > >  	soc_dapm_stream_event(rtd, stream, event);
> > >  	mutex_unlock(&card->dapm_mutex);
> > >  }
> > > +EXPORT_SYMBOL_GPL(snd_soc_dapm_stream_event);
> > 
> > These exports should be a new patch
> > 
> > And as we agreed this should have Documentation if not already done :)
> 
> It's not what I really want to do, it introduce 13 exports
> (EXPORT_SYMBOL_GPL) when we want split soc-compress.c to a separate
> Module, and adding documentation to all of these 13 functions(only for
> soc-compress.c to use them) looks somewhat ungraceful :(
> 
> Hi Takashi, what's your opinion? Maybe we should keep soc-compress.c
> in snd-soc-core.ko, which means skip/ignore this patch and don’t split it?

Well, I'm open about it.  Of course, making it as a module would make
sense, even for normal use cases.  But allowing the selective build of
soc-compress.c should be enough for most cases.  (In that case, the
new kconfig should be a boolean.)

As Liam suggested, if the exported functions are supposed to be
internal use only, it's also the very reason you have to write
documentation, too.  The function description itself can be concise,
then.

Another possibility is to refactor soc-compress.c.  If you compare the
code with soc-pcm.c, you see very much similarity.  But it's a path
that takes much longer, although it might be cleaner.


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

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

[Jie, Yang]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Tuesday, October 13, 2015 10:19 PM
> To: Jie, Yang
> Cc: Koul, Vinod; broonie@kernel.org; alsa-devel@alsa-project.org; Girdwood,
> Liam R
> Subject: Re: [alsa-devel] [PATCH v3 2/2] ASoC: soc-compress: split soc-
> compress to a module
> 
> On Tue, 13 Oct 2015 14:42:11 +0200,
> Jie, Yang wrote:
> >
> > > -----Original Message-----
> > > From: Koul, Vinod
> > > Sent: Tuesday, October 13, 2015 7:50 PM
> > > To: Jie, Yang; broonie@kernel.org
> > > Cc: Girdwood, Liam R; alsa-devel@alsa-project.org
> > > Subject: Re: [alsa-devel] [PATCH v3 2/2] ASoC: soc-compress: split
> > > soc- compress to a module
> > >
> > > On Tue, 2015-10-13 at 17:11 +0800, Jie Yang wrote:
> > > > +/* Module information */
> > > > +MODULE_AUTHOR("Namarta Kohli <namartax.kohli@intel.com>");
> > > > +MODULE_AUTHOR("Ramesh Babu K V
> <ramesh.babu@linux.intel.com>");
> > > > +MODULE_AUTHOR("Vinod Koul <vinod.koul@linux.intel.com>");
> > >
> > > Vinod Koul <vinod.koul@intel.com> please
> > >
> >
> > OK, I will change this one.
> >
> > > I need to move other instances too :(
> > >
> > >
> > > > +MODULE_DESCRIPTION("ALSA SoC Compress");
> > > MODULE_LICENSE("GPL");
> > > > +MODULE_ALIAS("platform:soc-compress");
> > > > diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index
> > > > ff8bda4..9e32151 100644
> > > > --- a/sound/soc/soc-dapm.c
> > > > +++ b/sound/soc/soc-dapm.c
> > > > @@ -3896,6 +3896,7 @@ void snd_soc_dapm_stream_event(struct
> > > > snd_soc_pcm_runtime *rtd, int stream,
> > > >  	soc_dapm_stream_event(rtd, stream, event);
> > > >  	mutex_unlock(&card->dapm_mutex);  }
> > > > +EXPORT_SYMBOL_GPL(snd_soc_dapm_stream_event);
> > >
> > > These exports should be a new patch
> > >
> > > And as we agreed this should have Documentation if not already done :)
> >
> > It's not what I really want to do, it introduce 13 exports
> > (EXPORT_SYMBOL_GPL) when we want split soc-compress.c to a separate
> > Module, and adding documentation to all of these 13 functions(only for
> > soc-compress.c to use them) looks somewhat ungraceful :(
> >
> > Hi Takashi, what's your opinion? Maybe we should keep soc-compress.c
> > in snd-soc-core.ko, which means skip/ignore this patch and don’t split it?
> 
> Well, I'm open about it.  Of course, making it as a module would make
> sense, even for normal use cases.  But allowing the selective build of
> soc-compress.c should be enough for most cases.  (In that case, the
> new kconfig should be a boolean.)
> 
> As Liam suggested, if the exported functions are supposed to be
> internal use only, it's also the very reason you have to write
> documentation, too.  The function description itself can be concise,
> then.
> 
> Another possibility is to refactor soc-compress.c.  If you compare the
> code with soc-pcm.c, you see very much similarity.  But it's a path
> that takes much longer, although it might be cleaner.
 
Thank you, Takashi, then I prefer only allowing the selective build of
soc-compress.c here.

Will update patch soon.

Thanks,
~Keyon

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