[PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Nicolin Chen]

Each implementation of gerneric pcm dmaengine has been naturally
limited by the pre-defined gerneric functions. Thus add an extra
interface for user to create a backdoor so that they can override
those fixed functions for some uncommon requirements: using on-chip
memory for DMA buffers and accordingly different mmap function for
example, while at the meantime, they can continue to benifit from
the concise and wise generic pcm dmaengine.

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 include/sound/dmaengine_pcm.h         | 13 +++++++++++++
 sound/core/pcm_dmaengine.c            | 25 +++++++++++++++++++++++++
 sound/soc/soc-generic-dmaengine-pcm.c | 30 ++++++++++++++++++------------
 3 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index f11c35c..a6e9d04 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -33,6 +33,18 @@ snd_pcm_substream_to_dma_direction(const struct snd_pcm_substream *substream)
 		return DMA_DEV_TO_MEM;
 }
 
+struct dmaengine_pcm {
+	struct dma_chan *chan[SNDRV_PCM_STREAM_CAPTURE + 1];
+	const struct snd_dmaengine_pcm_config *config;
+	struct snd_soc_platform platform;
+	unsigned int flags;
+};
+
+static inline struct dmaengine_pcm *soc_platform_to_pcm(struct snd_soc_platform *p)
+{
+	return container_of(p, struct dmaengine_pcm, platform);
+}
+
 int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
 	const struct snd_pcm_hw_params *params, struct dma_slave_config *slave_config);
 int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd);
@@ -123,6 +135,7 @@ struct snd_dmaengine_pcm_config {
 			struct snd_pcm_substream *substream);
 	dma_filter_fn compat_filter_fn;
 
+	const struct snd_soc_platform_driver *driver;
 	const struct snd_pcm_hardware *pcm_hardware;
 	unsigned int prealloc_buffer_size;
 };
diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index aa924d9..d78a67e 100644
--- a/sound/core/pcm_dmaengine.c
+++ b/sound/core/pcm_dmaengine.c
@@ -186,8 +186,14 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream)
 int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 {
 	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
+	const struct snd_dmaengine_pcm_config *config = pcm->config;
 	int ret;
 
+	if (config->driver->ops->trigger)
+		return config->driver->ops->trigger(substream, cmd);
+
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		ret = dmaengine_pcm_prepare_and_submit(substream);
@@ -224,6 +230,13 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
 snd_pcm_uframes_t snd_dmaengine_pcm_pointer_no_residue(struct snd_pcm_substream *substream)
 {
 	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
+	const struct snd_dmaengine_pcm_config *config = pcm->config;
+
+	if (config->driver->ops->pointer)
+		return config->driver->ops->pointer(substream);
+
 	return bytes_to_frames(substream->runtime, prtd->pos);
 }
 EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer_no_residue);
@@ -238,11 +251,17 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer_no_residue);
 snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream)
 {
 	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
+	const struct snd_dmaengine_pcm_config *config = pcm->config;
 	struct dma_tx_state state;
 	enum dma_status status;
 	unsigned int buf_size;
 	unsigned int pos = 0;
 
+	if (config->driver->ops->pointer)
+		return config->driver->ops->pointer(substream);
+
 	status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state);
 	if (status == DMA_IN_PROGRESS || status == DMA_PAUSED) {
 		buf_size = snd_pcm_lib_buffer_bytes(substream);
@@ -341,6 +360,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open_request_chan);
 int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream)
 {
 	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
+	const struct snd_dmaengine_pcm_config *config = pcm->config;
+
+	if (config->driver->ops->close)
+		return config->driver->ops->close(substream);
 
 	kfree(prtd);
 
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index e29ec3c..6e66c53 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -24,18 +24,6 @@
 
 #include <sound/dmaengine_pcm.h>
 
-struct dmaengine_pcm {
-	struct dma_chan *chan[SNDRV_PCM_STREAM_CAPTURE + 1];
-	const struct snd_dmaengine_pcm_config *config;
-	struct snd_soc_platform platform;
-	unsigned int flags;
-};
-
-static struct dmaengine_pcm *soc_platform_to_pcm(struct snd_soc_platform *p)
-{
-	return container_of(p, struct dmaengine_pcm, platform);
-}
-
 /**
  * snd_dmaengine_pcm_prepare_slave_config() - Generic prepare_slave_config callback
  * @substream: PCM substream
@@ -75,9 +63,13 @@ static int dmaengine_pcm_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
 	struct dma_chan *chan = snd_dmaengine_pcm_get_chan(substream);
+	const struct snd_dmaengine_pcm_config *config = pcm->config;
 	struct dma_slave_config slave_config;
 	int ret;
 
+	if (config->driver->ops->hw_params)
+		return config->driver->ops->hw_params(substream, params);
+
 	if (pcm->config->prepare_slave_config) {
 		ret = pcm->config->prepare_slave_config(substream, params,
 				&slave_config);
@@ -96,9 +88,13 @@ static int dmaengine_pcm_open(struct snd_pcm_substream *substream)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
+	const struct snd_dmaengine_pcm_config *config = pcm->config;
 	struct dma_chan *chan = pcm->chan[substream->stream];
 	int ret;
 
+	if (config->driver->ops->open)
+		return config->driver->ops->open(substream);
+
 	ret = snd_soc_set_runtime_hwparams(substream,
 				pcm->config->pcm_hardware);
 	if (ret)
@@ -118,6 +114,13 @@ static struct device *dmaengine_dma_dev(struct dmaengine_pcm *pcm,
 
 static void dmaengine_pcm_free(struct snd_pcm *pcm)
 {
+	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
+	struct dmaengine_pcm *dma_pcm = soc_platform_to_pcm(rtd->platform);
+	const struct snd_dmaengine_pcm_config *config = dma_pcm->config;
+
+	if (config->driver->pcm_free)
+		return config->driver->pcm_free(pcm);
+
 	snd_pcm_lib_preallocate_free_for_all(pcm);
 }
 
@@ -145,6 +148,9 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd)
 	unsigned int i;
 	int ret;
 
+	if (config->driver->pcm_new)
+		return config->driver->pcm_new(rtd);
+
 	for (i = SNDRV_PCM_STREAM_PLAYBACK; i <= SNDRV_PCM_STREAM_CAPTURE; i++) {
 		substream = rtd->pcm->streams[i].substream;
 		if (!substream)
-- 
1.8.4

Re: [PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Lars-Peter Clausen]

On 10/15/2013 09:49 AM, Nicolin Chen wrote:
> Each implementation of gerneric pcm dmaengine has been naturally
> limited by the pre-defined gerneric functions. Thus add an extra
> interface for user to create a backdoor so that they can override
> those fixed functions for some uncommon requirements: using on-chip
> memory for DMA buffers and accordingly different mmap function for
> example, while at the meantime, they can continue to benifit from
> the concise and wise generic pcm dmaengine.
> 
> Signed-off-by: Nicolin Chen <b42378@freescale.com>

We do have the dmaengine pcm helper functions (sound/core/pcm_dmaengine.c)
and the generic dmaengine pcm ASoC driver
(sound/soc/soc-generic-dmaengine-pcm.c). The generic dmaengine pcm ASoC
driver uses the dmaengine pcm helper functions, but it is not the only user
of them. So your patch breaks all other users of the helper functions.

The helper functions are designed in a way that you can either wrap them in
your own pcm driver driver or not use them at all. E.g. take a look at
sound/soc/omap/omap-pcm.c on how to overwrite specific functions.

- Lars

> ---
>  include/sound/dmaengine_pcm.h         | 13 +++++++++++++
>  sound/core/pcm_dmaengine.c            | 25 +++++++++++++++++++++++++
>  sound/soc/soc-generic-dmaengine-pcm.c | 30 ++++++++++++++++++------------
>  3 files changed, 56 insertions(+), 12 deletions(-)
> 
> diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
> index f11c35c..a6e9d04 100644
> --- a/include/sound/dmaengine_pcm.h
> +++ b/include/sound/dmaengine_pcm.h
> @@ -33,6 +33,18 @@ snd_pcm_substream_to_dma_direction(const struct snd_pcm_substream *substream)
>  		return DMA_DEV_TO_MEM;
>  }
>  
> +struct dmaengine_pcm {
> +	struct dma_chan *chan[SNDRV_PCM_STREAM_CAPTURE + 1];
> +	const struct snd_dmaengine_pcm_config *config;
> +	struct snd_soc_platform platform;
> +	unsigned int flags;
> +};
> +
> +static inline struct dmaengine_pcm *soc_platform_to_pcm(struct snd_soc_platform *p)
> +{
> +	return container_of(p, struct dmaengine_pcm, platform);
> +}
> +
>  int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
>  	const struct snd_pcm_hw_params *params, struct dma_slave_config *slave_config);
>  int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd);
> @@ -123,6 +135,7 @@ struct snd_dmaengine_pcm_config {
>  			struct snd_pcm_substream *substream);
>  	dma_filter_fn compat_filter_fn;
>  
> +	const struct snd_soc_platform_driver *driver;
>  	const struct snd_pcm_hardware *pcm_hardware;
>  	unsigned int prealloc_buffer_size;
>  };
> diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> index aa924d9..d78a67e 100644
> --- a/sound/core/pcm_dmaengine.c
> +++ b/sound/core/pcm_dmaengine.c
> @@ -186,8 +186,14 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream)
>  int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>  {
>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> +	const struct snd_dmaengine_pcm_config *config = pcm->config;
>  	int ret;
>  
> +	if (config->driver->ops->trigger)
> +		return config->driver->ops->trigger(substream, cmd);
> +
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
>  		ret = dmaengine_pcm_prepare_and_submit(substream);
> @@ -224,6 +230,13 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
>  snd_pcm_uframes_t snd_dmaengine_pcm_pointer_no_residue(struct snd_pcm_substream *substream)
>  {
>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> +	const struct snd_dmaengine_pcm_config *config = pcm->config;
> +
> +	if (config->driver->ops->pointer)
> +		return config->driver->ops->pointer(substream);
> +
>  	return bytes_to_frames(substream->runtime, prtd->pos);
>  }
>  EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer_no_residue);
> @@ -238,11 +251,17 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer_no_residue);
>  snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream)
>  {
>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> +	const struct snd_dmaengine_pcm_config *config = pcm->config;
>  	struct dma_tx_state state;
>  	enum dma_status status;
>  	unsigned int buf_size;
>  	unsigned int pos = 0;
>  
> +	if (config->driver->ops->pointer)
> +		return config->driver->ops->pointer(substream);
> +
>  	status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state);
>  	if (status == DMA_IN_PROGRESS || status == DMA_PAUSED) {
>  		buf_size = snd_pcm_lib_buffer_bytes(substream);
> @@ -341,6 +360,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open_request_chan);
>  int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream)
>  {
>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> +	const struct snd_dmaengine_pcm_config *config = pcm->config;
> +
> +	if (config->driver->ops->close)
> +		return config->driver->ops->close(substream);
>  
>  	kfree(prtd);
>  
> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
> index e29ec3c..6e66c53 100644
> --- a/sound/soc/soc-generic-dmaengine-pcm.c
> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> @@ -24,18 +24,6 @@
>  
>  #include <sound/dmaengine_pcm.h>
>  
> -struct dmaengine_pcm {
> -	struct dma_chan *chan[SNDRV_PCM_STREAM_CAPTURE + 1];
> -	const struct snd_dmaengine_pcm_config *config;
> -	struct snd_soc_platform platform;
> -	unsigned int flags;
> -};
> -
> -static struct dmaengine_pcm *soc_platform_to_pcm(struct snd_soc_platform *p)
> -{
> -	return container_of(p, struct dmaengine_pcm, platform);
> -}
> -
>  /**
>   * snd_dmaengine_pcm_prepare_slave_config() - Generic prepare_slave_config callback
>   * @substream: PCM substream
> @@ -75,9 +63,13 @@ static int dmaengine_pcm_hw_params(struct snd_pcm_substream *substream,
>  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>  	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
>  	struct dma_chan *chan = snd_dmaengine_pcm_get_chan(substream);
> +	const struct snd_dmaengine_pcm_config *config = pcm->config;
>  	struct dma_slave_config slave_config;
>  	int ret;
>  
> +	if (config->driver->ops->hw_params)
> +		return config->driver->ops->hw_params(substream, params);
> +
>  	if (pcm->config->prepare_slave_config) {
>  		ret = pcm->config->prepare_slave_config(substream, params,
>  				&slave_config);
> @@ -96,9 +88,13 @@ static int dmaengine_pcm_open(struct snd_pcm_substream *substream)
>  {
>  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
>  	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> +	const struct snd_dmaengine_pcm_config *config = pcm->config;
>  	struct dma_chan *chan = pcm->chan[substream->stream];
>  	int ret;
>  
> +	if (config->driver->ops->open)
> +		return config->driver->ops->open(substream);
> +
>  	ret = snd_soc_set_runtime_hwparams(substream,
>  				pcm->config->pcm_hardware);
>  	if (ret)
> @@ -118,6 +114,13 @@ static struct device *dmaengine_dma_dev(struct dmaengine_pcm *pcm,
>  
>  static void dmaengine_pcm_free(struct snd_pcm *pcm)
>  {
> +	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
> +	struct dmaengine_pcm *dma_pcm = soc_platform_to_pcm(rtd->platform);
> +	const struct snd_dmaengine_pcm_config *config = dma_pcm->config;
> +
> +	if (config->driver->pcm_free)
> +		return config->driver->pcm_free(pcm);
> +
>  	snd_pcm_lib_preallocate_free_for_all(pcm);
>  }
>  
> @@ -145,6 +148,9 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd)
>  	unsigned int i;
>  	int ret;
>  
> +	if (config->driver->pcm_new)
> +		return config->driver->pcm_new(rtd);
> +
>  	for (i = SNDRV_PCM_STREAM_PLAYBACK; i <= SNDRV_PCM_STREAM_CAPTURE; i++) {
>  		substream = rtd->pcm->streams[i].substream;
>  		if (!substream)
> 

Re: [PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Nicolin Chen]

Hi Lars,

On Tue, Oct 15, 2013 at 10:08:52AM +0200, Lars-Peter Clausen wrote:
> On 10/15/2013 09:49 AM, Nicolin Chen wrote:
> > Each implementation of gerneric pcm dmaengine has been naturally
> > limited by the pre-defined gerneric functions. Thus add an extra
> > interface for user to create a backdoor so that they can override
> > those fixed functions for some uncommon requirements: using on-chip
> > memory for DMA buffers and accordingly different mmap function for
> > example, while at the meantime, they can continue to benifit from
> > the concise and wise generic pcm dmaengine.
> > 
> > Signed-off-by: Nicolin Chen <b42378@freescale.com>
> 
> We do have the dmaengine pcm helper functions (sound/core/pcm_dmaengine.c)
> and the generic dmaengine pcm ASoC driver
> (sound/soc/soc-generic-dmaengine-pcm.c). The generic dmaengine pcm ASoC
> driver uses the dmaengine pcm helper functions, but it is not the only user
> of them. So your patch breaks all other users of the helper functions.
> 
> The helper functions are designed in a way that you can either wrap them in
> your own pcm driver driver or not use them at all. E.g. take a look at
> sound/soc/omap/omap-pcm.c on how to overwrite specific functions.
> 
> - Lars

If this modification to sound/core/pcm_dmaengine.c is not fair,
what about moving the modification to soc-gerneric-dmaengine-pcm.c?

For example, instead of using snd_dmaengine_pcm_trigger() directly
in gerneric soc dmaengine driver, we create a new one inside gerneric
soc dmaengine driver and check the override:

static int snd_soc_dmaengine_pcm_trigger(substream, cmd) {
	if (config->driver->ops->trigger)
		return config->driver->ops->trigger(substream, cmd);
		
	return snd_dmaengine_pcm_trigger(substream, cmd);
}

Since it happens inside generic dmaengine pcm ASoC driver, it won't break
those helper functions.

I'm trying this just because I hope generic dmaengine can be more flexible
Admittedly, using helper functions might be more plausible way in current
ASoC structure. However, there might be so much change for an generic soc
dmaengine implementation even if it just wants to override one single func.

Thank you,
Nicolin Chen


> 
> > ---
> >  include/sound/dmaengine_pcm.h         | 13 +++++++++++++
> >  sound/core/pcm_dmaengine.c            | 25 +++++++++++++++++++++++++
> >  sound/soc/soc-generic-dmaengine-pcm.c | 30 ++++++++++++++++++------------
> >  3 files changed, 56 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
> > index f11c35c..a6e9d04 100644
> > --- a/include/sound/dmaengine_pcm.h
> > +++ b/include/sound/dmaengine_pcm.h
> > @@ -33,6 +33,18 @@ snd_pcm_substream_to_dma_direction(const struct snd_pcm_substream *substream)
> >  		return DMA_DEV_TO_MEM;
> >  }
> >  
> > +struct dmaengine_pcm {
> > +	struct dma_chan *chan[SNDRV_PCM_STREAM_CAPTURE + 1];
> > +	const struct snd_dmaengine_pcm_config *config;
> > +	struct snd_soc_platform platform;
> > +	unsigned int flags;
> > +};
> > +
> > +static inline struct dmaengine_pcm *soc_platform_to_pcm(struct snd_soc_platform *p)
> > +{
> > +	return container_of(p, struct dmaengine_pcm, platform);
> > +}
> > +
> >  int snd_hwparams_to_dma_slave_config(const struct snd_pcm_substream *substream,
> >  	const struct snd_pcm_hw_params *params, struct dma_slave_config *slave_config);
> >  int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd);
> > @@ -123,6 +135,7 @@ struct snd_dmaengine_pcm_config {
> >  			struct snd_pcm_substream *substream);
> >  	dma_filter_fn compat_filter_fn;
> >  
> > +	const struct snd_soc_platform_driver *driver;
> >  	const struct snd_pcm_hardware *pcm_hardware;
> >  	unsigned int prealloc_buffer_size;
> >  };
> > diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
> > index aa924d9..d78a67e 100644
> > --- a/sound/core/pcm_dmaengine.c
> > +++ b/sound/core/pcm_dmaengine.c
> > @@ -186,8 +186,14 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream)
> >  int snd_dmaengine_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> >  {
> >  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> > +	const struct snd_dmaengine_pcm_config *config = pcm->config;
> >  	int ret;
> >  
> > +	if (config->driver->ops->trigger)
> > +		return config->driver->ops->trigger(substream, cmd);
> > +
> >  	switch (cmd) {
> >  	case SNDRV_PCM_TRIGGER_START:
> >  		ret = dmaengine_pcm_prepare_and_submit(substream);
> > @@ -224,6 +230,13 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
> >  snd_pcm_uframes_t snd_dmaengine_pcm_pointer_no_residue(struct snd_pcm_substream *substream)
> >  {
> >  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> > +	const struct snd_dmaengine_pcm_config *config = pcm->config;
> > +
> > +	if (config->driver->ops->pointer)
> > +		return config->driver->ops->pointer(substream);
> > +
> >  	return bytes_to_frames(substream->runtime, prtd->pos);
> >  }
> >  EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer_no_residue);
> > @@ -238,11 +251,17 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer_no_residue);
> >  snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream)
> >  {
> >  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> > +	const struct snd_dmaengine_pcm_config *config = pcm->config;
> >  	struct dma_tx_state state;
> >  	enum dma_status status;
> >  	unsigned int buf_size;
> >  	unsigned int pos = 0;
> >  
> > +	if (config->driver->ops->pointer)
> > +		return config->driver->ops->pointer(substream);
> > +
> >  	status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state);
> >  	if (status == DMA_IN_PROGRESS || status == DMA_PAUSED) {
> >  		buf_size = snd_pcm_lib_buffer_bytes(substream);
> > @@ -341,6 +360,12 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open_request_chan);
> >  int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream)
> >  {
> >  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> > +	const struct snd_dmaengine_pcm_config *config = pcm->config;
> > +
> > +	if (config->driver->ops->close)
> > +		return config->driver->ops->close(substream);
> >  
> >  	kfree(prtd);
> >  
> > diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
> > index e29ec3c..6e66c53 100644
> > --- a/sound/soc/soc-generic-dmaengine-pcm.c
> > +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> > @@ -24,18 +24,6 @@
> >  
> >  #include <sound/dmaengine_pcm.h>
> >  
> > -struct dmaengine_pcm {
> > -	struct dma_chan *chan[SNDRV_PCM_STREAM_CAPTURE + 1];
> > -	const struct snd_dmaengine_pcm_config *config;
> > -	struct snd_soc_platform platform;
> > -	unsigned int flags;
> > -};
> > -
> > -static struct dmaengine_pcm *soc_platform_to_pcm(struct snd_soc_platform *p)
> > -{
> > -	return container_of(p, struct dmaengine_pcm, platform);
> > -}
> > -
> >  /**
> >   * snd_dmaengine_pcm_prepare_slave_config() - Generic prepare_slave_config callback
> >   * @substream: PCM substream
> > @@ -75,9 +63,13 @@ static int dmaengine_pcm_hw_params(struct snd_pcm_substream *substream,
> >  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >  	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> >  	struct dma_chan *chan = snd_dmaengine_pcm_get_chan(substream);
> > +	const struct snd_dmaengine_pcm_config *config = pcm->config;
> >  	struct dma_slave_config slave_config;
> >  	int ret;
> >  
> > +	if (config->driver->ops->hw_params)
> > +		return config->driver->ops->hw_params(substream, params);
> > +
> >  	if (pcm->config->prepare_slave_config) {
> >  		ret = pcm->config->prepare_slave_config(substream, params,
> >  				&slave_config);
> > @@ -96,9 +88,13 @@ static int dmaengine_pcm_open(struct snd_pcm_substream *substream)
> >  {
> >  	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >  	struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform);
> > +	const struct snd_dmaengine_pcm_config *config = pcm->config;
> >  	struct dma_chan *chan = pcm->chan[substream->stream];
> >  	int ret;
> >  
> > +	if (config->driver->ops->open)
> > +		return config->driver->ops->open(substream);
> > +
> >  	ret = snd_soc_set_runtime_hwparams(substream,
> >  				pcm->config->pcm_hardware);
> >  	if (ret)
> > @@ -118,6 +114,13 @@ static struct device *dmaengine_dma_dev(struct dmaengine_pcm *pcm,
> >  
> >  static void dmaengine_pcm_free(struct snd_pcm *pcm)
> >  {
> > +	struct snd_soc_pcm_runtime *rtd = pcm->private_data;
> > +	struct dmaengine_pcm *dma_pcm = soc_platform_to_pcm(rtd->platform);
> > +	const struct snd_dmaengine_pcm_config *config = dma_pcm->config;
> > +
> > +	if (config->driver->pcm_free)
> > +		return config->driver->pcm_free(pcm);
> > +
> >  	snd_pcm_lib_preallocate_free_for_all(pcm);
> >  }
> >  
> > @@ -145,6 +148,9 @@ static int dmaengine_pcm_new(struct snd_soc_pcm_runtime *rtd)
> >  	unsigned int i;
> >  	int ret;
> >  
> > +	if (config->driver->pcm_new)
> > +		return config->driver->pcm_new(rtd);
> > +
> >  	for (i = SNDRV_PCM_STREAM_PLAYBACK; i <= SNDRV_PCM_STREAM_CAPTURE; i++) {
> >  		substream = rtd->pcm->streams[i].substream;
> >  		if (!substream)
> > 
> 
> 

Re: [PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Lars-Peter Clausen]

On 10/15/2013 10:22 AM, Nicolin Chen wrote:
> Hi Lars,
> 
> On Tue, Oct 15, 2013 at 10:08:52AM +0200, Lars-Peter Clausen wrote:
>> On 10/15/2013 09:49 AM, Nicolin Chen wrote:
>>> Each implementation of gerneric pcm dmaengine has been naturally
>>> limited by the pre-defined gerneric functions. Thus add an extra
>>> interface for user to create a backdoor so that they can override
>>> those fixed functions for some uncommon requirements: using on-chip
>>> memory for DMA buffers and accordingly different mmap function for
>>> example, while at the meantime, they can continue to benifit from
>>> the concise and wise generic pcm dmaengine.
>>>
>>> Signed-off-by: Nicolin Chen <b42378@freescale.com>
>>
>> We do have the dmaengine pcm helper functions (sound/core/pcm_dmaengine.c)
>> and the generic dmaengine pcm ASoC driver
>> (sound/soc/soc-generic-dmaengine-pcm.c). The generic dmaengine pcm ASoC
>> driver uses the dmaengine pcm helper functions, but it is not the only user
>> of them. So your patch breaks all other users of the helper functions.
>>
>> The helper functions are designed in a way that you can either wrap them in
>> your own pcm driver driver or not use them at all. E.g. take a look at
>> sound/soc/omap/omap-pcm.c on how to overwrite specific functions.
>>
>> - Lars
> 
> If this modification to sound/core/pcm_dmaengine.c is not fair,
> what about moving the modification to soc-gerneric-dmaengine-pcm.c?
> 
> For example, instead of using snd_dmaengine_pcm_trigger() directly
> in gerneric soc dmaengine driver, we create a new one inside gerneric
> soc dmaengine driver and check the override:
> 
> static int snd_soc_dmaengine_pcm_trigger(substream, cmd) {
> 	if (config->driver->ops->trigger)
> 		return config->driver->ops->trigger(substream, cmd);
> 		
> 	return snd_dmaengine_pcm_trigger(substream, cmd);
> }
> 
> Since it happens inside generic dmaengine pcm ASoC driver, it won't break
> those helper functions.
> 
> I'm trying this just because I hope generic dmaengine can be more flexible
> Admittedly, using helper functions might be more plausible way in current
> ASoC structure. However, there might be so much change for an generic soc
> dmaengine implementation even if it just wants to override one single func.

Well the idea of the generic dmaengine driver is to be generic and not
require SoC specific hacks since those should not be necessary if things are
done right. What exactly do you want to implement in the overwritten ops?

- Lars

Re: [PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Nicolin Chen]

On Tue, Oct 15, 2013 at 11:02:44AM +0200, Lars-Peter Clausen wrote:
> > If this modification to sound/core/pcm_dmaengine.c is not fair,
> > what about moving the modification to soc-gerneric-dmaengine-pcm.c?
> > 
> > For example, instead of using snd_dmaengine_pcm_trigger() directly
> > in gerneric soc dmaengine driver, we create a new one inside gerneric
> > soc dmaengine driver and check the override:
> > 
> > static int snd_soc_dmaengine_pcm_trigger(substream, cmd) {
> > 	if (config->driver->ops->trigger)
> > 		return config->driver->ops->trigger(substream, cmd);
> > 		
> > 	return snd_dmaengine_pcm_trigger(substream, cmd);
> > }
> > 
> > Since it happens inside generic dmaengine pcm ASoC driver, it won't break
> > those helper functions.
> > 
> > I'm trying this just because I hope generic dmaengine can be more flexible
> > Admittedly, using helper functions might be more plausible way in current
> > ASoC structure. However, there might be so much change for an generic soc
> > dmaengine implementation even if it just wants to override one single func.
> 
> Well the idea of the generic dmaengine driver is to be generic and not
> require SoC specific hacks since those should not be necessary if things are
> done right. What exactly do you want to implement in the overwritten ops?
> 
> - Lars

Just like I mentioned in the commit comments, using on-chip memory would
need an alternative memory allocating function to replace the default one
from external DDR. imx-pcm-dma.c is now using ASoC generic dmaengine, so
if we turn it back to helper functions way, all cpu dai drivers might have
to be turned back as well.

ASoC generic dmaengine is quite concise and well-developed for us to use.
But after using it, its own limitation might make user waver to choose the
old fashion helper functions. So I just wish it could provide a back door
for an easy trick. Then we can continue to enjoy the convenience from it
and also be able to tackle these uncommon cases.

Thank you,
Nicolin Chen

Re: [PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Nicolin Chen]

On Tue, Oct 15, 2013 at 11:40:10AM +0200, Lars-Peter Clausen wrote:
> On 10/15/2013 11:23 AM, Nicolin Chen wrote:
> > On Tue, Oct 15, 2013 at 11:02:44AM +0200, Lars-Peter Clausen wrote:
> >>> If this modification to sound/core/pcm_dmaengine.c is not fair,
> >>> what about moving the modification to soc-gerneric-dmaengine-pcm.c?
> >>>
> >>> For example, instead of using snd_dmaengine_pcm_trigger() directly
> >>> in gerneric soc dmaengine driver, we create a new one inside gerneric
> >>> soc dmaengine driver and check the override:
> >>>
> >>> static int snd_soc_dmaengine_pcm_trigger(substream, cmd) {
> >>> 	if (config->driver->ops->trigger)
> >>> 		return config->driver->ops->trigger(substream, cmd);
> >>> 		
> >>> 	return snd_dmaengine_pcm_trigger(substream, cmd);
> >>> }
> >>>
> >>> Since it happens inside generic dmaengine pcm ASoC driver, it won't break
> >>> those helper functions.
> >>>
> >>> I'm trying this just because I hope generic dmaengine can be more flexible
> >>> Admittedly, using helper functions might be more plausible way in current
> >>> ASoC structure. However, there might be so much change for an generic soc
> >>> dmaengine implementation even if it just wants to override one single func.
> >>
> >> Well the idea of the generic dmaengine driver is to be generic and not
> >> require SoC specific hacks since those should not be necessary if things are
> >> done right. What exactly do you want to implement in the overwritten ops?
> >>
> >> - Lars
> > 
> > Just like I mentioned in the commit comments, using on-chip memory would
> > need an alternative memory allocating function to replace the default one
> > from external DDR. imx-pcm-dma.c is now using ASoC generic dmaengine, so
> > if we turn it back to helper functions way, all cpu dai drivers might have
> > to be turned back as well.
> > 
> > ASoC generic dmaengine is quite concise and well-developed for us to use.
> > But after using it, its own limitation might make user waver to choose the
> > old fashion helper functions. So I just wish it could provide a back door
> > for an easy trick. Then we can continue to enjoy the convenience from it
> > and also be able to tackle these uncommon cases.
> 
> Wanting to use on-chip memory for the audio buffers is not that uncommon.
> I'd rather see this implemented generically in the generic driver instead of
> each SoC implementing its own version.
> 
> - Lars

Is that so? Actually, I did as you said in my internal branch, but I just
fear that might not be easy to upstream, so I tried to figure out another
plausible way. It looks like I took a detour :)

Thank you for the advice.
Nicolin Chen

Re: [PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Lars-Peter Clausen]

On 10/15/2013 11:23 AM, Nicolin Chen wrote:
> On Tue, Oct 15, 2013 at 11:02:44AM +0200, Lars-Peter Clausen wrote:
>>> If this modification to sound/core/pcm_dmaengine.c is not fair,
>>> what about moving the modification to soc-gerneric-dmaengine-pcm.c?
>>>
>>> For example, instead of using snd_dmaengine_pcm_trigger() directly
>>> in gerneric soc dmaengine driver, we create a new one inside gerneric
>>> soc dmaengine driver and check the override:
>>>
>>> static int snd_soc_dmaengine_pcm_trigger(substream, cmd) {
>>> 	if (config->driver->ops->trigger)
>>> 		return config->driver->ops->trigger(substream, cmd);
>>> 		
>>> 	return snd_dmaengine_pcm_trigger(substream, cmd);
>>> }
>>>
>>> Since it happens inside generic dmaengine pcm ASoC driver, it won't break
>>> those helper functions.
>>>
>>> I'm trying this just because I hope generic dmaengine can be more flexible
>>> Admittedly, using helper functions might be more plausible way in current
>>> ASoC structure. However, there might be so much change for an generic soc
>>> dmaengine implementation even if it just wants to override one single func.
>>
>> Well the idea of the generic dmaengine driver is to be generic and not
>> require SoC specific hacks since those should not be necessary if things are
>> done right. What exactly do you want to implement in the overwritten ops?
>>
>> - Lars
> 
> Just like I mentioned in the commit comments, using on-chip memory would
> need an alternative memory allocating function to replace the default one
> from external DDR. imx-pcm-dma.c is now using ASoC generic dmaengine, so
> if we turn it back to helper functions way, all cpu dai drivers might have
> to be turned back as well.
> 
> ASoC generic dmaengine is quite concise and well-developed for us to use.
> But after using it, its own limitation might make user waver to choose the
> old fashion helper functions. So I just wish it could provide a back door
> for an easy trick. Then we can continue to enjoy the convenience from it
> and also be able to tackle these uncommon cases.

Wanting to use on-chip memory for the audio buffers is not that uncommon.
I'd rather see this implemented generically in the generic driver instead of
each SoC implementing its own version.

- Lars

Re: [PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Mark Brown]

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

On Tue, Oct 15, 2013 at 11:40:10AM +0200, Lars-Peter Clausen wrote:

> Wanting to use on-chip memory for the audio buffers is not that uncommon.
> I'd rather see this implemented generically in the generic driver instead of
> each SoC implementing its own version.

I agree - to me it looks like the major problm here is that the handling
of the on chip memory tends to vary widely between devices rather than
that there are different problems.

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

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

Re: [PATCH] ASoC: generic-dmaengine-pcm: Add an interface to override its functions

[Mark Brown]

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

On Tue, Oct 15, 2013 at 05:37:17PM +0800, Nicolin Chen wrote:
> On Tue, Oct 15, 2013 at 11:40:10AM +0200, Lars-Peter Clausen wrote:

> > Wanting to use on-chip memory for the audio buffers is not that uncommon.
> > I'd rather see this implemented generically in the generic driver instead of
> > each SoC implementing its own version.

> Is that so? Actually, I did as you said in my internal branch, but I just
> fear that might not be easy to upstream, so I tried to figure out another
> plausible way. It looks like I took a detour :)

Sounds like your first idea was the best one!  I don't know about anyone
else but personally I'd be really happy to see the framework support for
these on chip mmemories improved in Linux, there's quite a few chips
aren't taking advantage of them at the minute due to the lack of a clear
way to do so.

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

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