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

[PATCH 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   | 6 +++++-
 sound/soc/Makefile  | 3 ++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 93df8bf..03c53a9 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);
+#ifdef 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..de743d9 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
+
 # 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
-- 
1.9.1

[PATCH 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 | 20 ++++++++++++++++++++
 2 files changed, 23 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..9683f53 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,22 @@ compr_err:
 	kfree(compr);
 	return ret;
 }
+
+static int __init soc_compress_init(void)
+{
+       return 0;
+}
+module_init(soc_compress_init);
+
+static void __exit soc_compress_exit(void)
+{
+}
+module_exit(soc_compress_exit);
+
+/* 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");
-- 
1.9.1

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

[Mark Brown]

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

On Fri, Jun 12, 2015 at 07:59:15PM +0800, Jie Yang wrote:

> +config SND_SOC_COMPRESS
> +	tristate
> +	select SND_COMPRESS_OFFLOAD
> +	default n
> +

No need to specify default n, it is the default.  I'm not seeing any
change here to make the drivers that do use compressed support select
this symbol and since there's no config text there's no way for users to
enable this on a per system basis so this breaks all existing compressed
audio support.

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

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

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

[Mark Brown]

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

On Fri, Jun 12, 2015 at 07:59:16PM +0800, Jie Yang wrote:

> +static int __init soc_compress_init(void)
> +{
> +       return 0;
> +}
> +module_init(soc_compress_init);
> +
> +static void __exit soc_compress_exit(void)
> +{
> +}
> +module_exit(soc_compress_exit);

These are completely empty, why are they being added?

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

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

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

[Mark Brown]

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

On Fri, Jun 12, 2015 at 07:59:16PM +0800, Jie Yang wrote:

> +MODULE_ALIAS("platform:soc-compress");

Oh, and this is incorrect - there is no platform device driver in this
file.

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

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

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

[Jie, Yang]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Friday, June 12, 2015 8:15 PM
> To: Jie, Yang
> Cc: tiwai@suse.de; alsa-devel@alsa-project.org; Girdwood, Liam R;
> vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> On Fri, Jun 12, 2015 at 07:59:15PM +0800, Jie Yang wrote:
> 
> > +config SND_SOC_COMPRESS
> > +	tristate
> > +	select SND_COMPRESS_OFFLOAD
> > +	default n
> > +
> 
> No need to specify default n, it is the default.  I'm not seeing any change

OK, will remove this default setting.

> here to make the drivers that do use compressed support select this symbol
 
As mentioned in commit message, we need add 'select SND_SOC_COMPRESS'
to driver  Kconfig(e.g. sound/soc/intel/Kconfig) when it is needed.

> and since there's no config text there's no way for users to enable this on a
> per system basis so this breaks all existing compressed audio support.
 
Do you mean I should add this message to 'Help' for this config item?
Per my understanding, no existing driver really uses this compressed audio
support currently, is that true?

~Keyon

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

[Mark Brown]

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

On Fri, Jun 12, 2015 at 12:31:25PM +0000, Jie, Yang wrote:

> > here to make the drivers that do use compressed support select this symbol

> As mentioned in commit message, we need add 'select SND_SOC_COMPRESS'
> to driver  Kconfig(e.g. sound/soc/intel/Kconfig) when it is needed.
> 
> > and since there's no config text there's no way for users to enable this on a
> > per system basis so this breaks all existing compressed audio support.
>  
> Do you mean I should add this message to 'Help' for this config item?
> Per my understanding, no existing driver really uses this compressed audio
> support currently, is that true?

Some of the existing Intel machine drivers have references to compressed
audio, it certainly looks like they're trying to use it.  Are you saying
that this is not actually the case?

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

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

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

[Jie, Yang]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Friday, June 12, 2015 9:00 PM
> To: Jie, Yang
> Cc: tiwai@suse.de; alsa-devel@alsa-project.org; Girdwood, Liam R;
> vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH 1/2] ASoC: soc-compress: add a config item for soc-
> compress
> 
> On Fri, Jun 12, 2015 at 12:31:25PM +0000, Jie, Yang wrote:
> 
> > > here to make the drivers that do use compressed support select this
> > > symbol
> 
> > As mentioned in commit message, we need add 'select
> SND_SOC_COMPRESS'
> > to driver  Kconfig(e.g. sound/soc/intel/Kconfig) when it is needed.
> >
> > > and since there's no config text there's no way for users to enable
> > > this on a per system basis so this breaks all existing compressed audio
> support.
> >
> > Do you mean I should add this message to 'Help' for this config item?
> > Per my understanding, no existing driver really uses this compressed
> > audio support currently, is that true?
> 
> Some of the existing Intel machine drivers have references to compressed
> audio, it certainly looks like they're trying to use it.  Are you saying that this is
> not actually the case?
 
Let me double confirm if the atom drivers are using it. Seems mfld platform is
using that.

~Keyon

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

[Jie, Yang]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Friday, June 12, 2015 8:16 PM
> To: Jie, Yang
> Cc: tiwai@suse.de; alsa-devel@alsa-project.org; Girdwood, Liam R;
> vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH 2/2] ASoC: soc-compress: split soc-compress to a
> module
> 
> On Fri, Jun 12, 2015 at 07:59:16PM +0800, Jie Yang wrote:
> 
> > +MODULE_ALIAS("platform:soc-compress");
> 
> Oh, and this is incorrect - there is no platform device driver in this file.
 
OK, will remove this line.

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

[Jie, Yang]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Friday, June 12, 2015 8:15 PM
> To: Jie, Yang
> Cc: tiwai@suse.de; alsa-devel@alsa-project.org; Girdwood, Liam R;
> vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> Subject: Re: [PATCH 2/2] ASoC: soc-compress: split soc-compress to a
> module
> 
> On Fri, Jun 12, 2015 at 07:59:16PM +0800, Jie Yang wrote:
> 
> > +static int __init soc_compress_init(void) {
> > +       return 0;
> > +}
> > +module_init(soc_compress_init);
> > +
> > +static void __exit soc_compress_exit(void) { }
> > +module_exit(soc_compress_exit);
> 
> These are completely empty, why are they being added?
 
I imitated that from sound/core/compress_offload.c, let me remove
them.

~Keyon

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

[Vinod Koul]

On Fri, Jun 12, 2015 at 01:33:25PM +0000, Jie, Yang wrote:
> > -----Original Message-----
> > From: Mark Brown [mailto:broonie@kernel.org]
> > Sent: Friday, June 12, 2015 9:00 PM
> > To: Jie, Yang
> > Cc: tiwai@suse.de; alsa-devel@alsa-project.org; Girdwood, Liam R;
> > vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang, Vivian
> > Subject: Re: [PATCH 1/2] ASoC: soc-compress: add a config item for soc-
> > compress
Okay didnt realise that its posted to alsa :( I need to remove all ref to
@linux id as am not using that anymore

> > 
> > On Fri, Jun 12, 2015 at 12:31:25PM +0000, Jie, Yang wrote:
> > 
> > > > here to make the drivers that do use compressed support select this
> > > > symbol
> > 
> > > As mentioned in commit message, we need add 'select
> > SND_SOC_COMPRESS'
> > > to driver  Kconfig(e.g. sound/soc/intel/Kconfig) when it is needed.
> > >
> > > > and since there's no config text there's no way for users to enable
> > > > this on a per system basis so this breaks all existing compressed audio
> > support.
> > >
> > > Do you mean I should add this message to 'Help' for this config item?
> > > Per my understanding, no existing driver really uses this compressed
> > > audio support currently, is that true?
> > 
> > Some of the existing Intel machine drivers have references to compressed
> > audio, it certainly looks like they're trying to use it.  Are you saying that this is
> > not actually the case?
>  
> Let me double confirm if the atom drivers are using it. Seems mfld platform is
> using that.
Yes and we will get runtime errors on all mfld-atom based drivers so please
add that on all atom machines

Since the dummy symbols are present so we wont get build errors. If a driver is
using the compressed API maybe we should not add dummy so that we catch ones
which are not including this config option, so for that point I would say
removal of dummay handlers looks best to me.

-- 
~Vinod

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

[Jie, Yang]

> -----Original Message-----
> From: Koul, Vinod
> Sent: Tuesday, June 16, 2015 4:14 PM
> To: Jie, Yang
> Cc: Mark Brown; ramesh.babu@linux.intel.com; alsa-devel@alsa-project.org;
> tiwai@suse.de; Zhang, Vivian; vinod.koul@linux.intel.com; Girdwood, Liam R
> Subject: Re: [alsa-devel] [PATCH 1/2] ASoC: soc-compress: add a config item
> for soc-compress
> 
> On Fri, Jun 12, 2015 at 01:33:25PM +0000, Jie, Yang wrote:
> > > -----Original Message-----
> > > From: Mark Brown [mailto:broonie@kernel.org]
> > > Sent: Friday, June 12, 2015 9:00 PM
> > > To: Jie, Yang
> > > Cc: tiwai@suse.de; alsa-devel@alsa-project.org; Girdwood, Liam R;
> > > vinod.koul@linux.intel.com; ramesh.babu@linux.intel.com; Zhang,
> > > Vivian
> > > Subject: Re: [PATCH 1/2] ASoC: soc-compress: add a config item for
> > > soc- compress
> Okay didnt realise that its posted to alsa :( I need to remove all ref to @linux
> id as am not using that anymore
 
Seems that's a big patch for replacing. :)

> 
> > >
> > > On Fri, Jun 12, 2015 at 12:31:25PM +0000, Jie, Yang wrote:
> > >
> > > > > here to make the drivers that do use compressed support select
> > > > > this symbol
> > >
> > > > As mentioned in commit message, we need add 'select
> > > SND_SOC_COMPRESS'
> > > > to driver  Kconfig(e.g. sound/soc/intel/Kconfig) when it is needed.
> > > >
> > > > > and since there's no config text there's no way for users to
> > > > > enable this on a per system basis so this breaks all existing
> > > > > compressed audio
> > > support.
> > > >
> > > > Do you mean I should add this message to 'Help' for this config item?
> > > > Per my understanding, no existing driver really uses this
> > > > compressed audio support currently, is that true?
> > >
> > > Some of the existing Intel machine drivers have references to
> > > compressed audio, it certainly looks like they're trying to use it.
> > > Are you saying that this is not actually the case?
> >
> > Let me double confirm if the atom drivers are using it. Seems mfld
> > platform is using that.
> Yes and we will get runtime errors on all mfld-atom based drivers so please
> add that on all atom machines
> 
> Since the dummy symbols are present so we wont get build errors. If a driver
> is using the compressed API maybe we should not add dummy so that we
> catch ones which are not including this config option, so for that point I would
> say removal of dummay handlers looks best to me.
 
Hi, Vinod, as I mentioned in replying to v2 patch in another thread,  here
cpu_dai->driver->compress_dai is a runtime value, which means we don't know
if we need compress API(for our driver) or not at compiling stage. 
So, reporting error at compiling stage looks not easy to be implemented.

At the same time, we return -EPERM to return and tell caller that compress 
operation is not permitted.

Anyway, let's discuss further on v2 patches thread.

Thanks,
~Keyon

> 
> --
> ~Vinod