diff mbox

[2/2] ASoC: dmaengine_pcm: add snd_dmaengine_generic_pcm_open()

Message ID 1363318601-31505-3-git-send-email-shawn.guo@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Shawn Guo March 15, 2013, 3:36 a.m. UTC 
With generic DMA device tree binding and helper function
dma_request_slave_channel() in place, dmaengine_pcm should support
that in requesting DMA channel for users that support generic DMA
device tree binding.

Add a new API snd_dmaengine_generic_pcm_open() as the variant of
snd_dmaengine_pcm_open().  It takes DMA client struct device pointer
and slave channel name as arguments and calls generic DMA helper
dma_request_slave_channel() to request DMA channel from dmaengine.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: alsa-devel@alsa-project.org
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
---
 include/sound/dmaengine_pcm.h |    2 ++
 sound/soc/soc-dmaengine-pcm.c |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

Comments

Sebastien Guiriec March 15, 2013, 10 a.m. UTC | #1 
Test on OMAP with patches
  http://www.spinics.net/lists/linux-omap/msg87893.html

Tested-by: Sebastien Guiriec <s-guiriec@ti.com>

On 03/15/2013 04:36 AM, Shawn Guo wrote:
> With generic DMA device tree binding and helper function
> dma_request_slave_channel() in place, dmaengine_pcm should support
> that in requesting DMA channel for users that support generic DMA
> device tree binding.
>
> Add a new API snd_dmaengine_generic_pcm_open() as the variant of
> snd_dmaengine_pcm_open().  It takes DMA client struct device pointer
> and slave channel name as arguments and calls generic DMA helper
> dma_request_slave_channel() to request DMA channel from dmaengine.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: alsa-devel@alsa-project.org
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   include/sound/dmaengine_pcm.h |    2 ++
>   sound/soc/soc-dmaengine-pcm.c |   39 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 41 insertions(+)
>
> diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
> index b877334..452df15 100644
> --- a/include/sound/dmaengine_pcm.h
> +++ b/include/sound/dmaengine_pcm.h
> @@ -43,6 +43,8 @@ snd_pcm_uframes_t snd_dmaengine_pcm_pointer_no_residue(struct snd_pcm_substream
>
>   int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>   	dma_filter_fn filter_fn, void *filter_data);
> +int snd_dmaengine_generic_pcm_open(struct snd_pcm_substream *substream,
> +				   struct device *dev, const char *name);
>   int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream);
>
>   struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream);
> diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c
> index 111b7d92..970eb2b 100644
> --- a/sound/soc/soc-dmaengine-pcm.c
> +++ b/sound/soc/soc-dmaengine-pcm.c
> @@ -304,6 +304,45 @@ int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>   EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open);
>
>   /**
> + * snd_dmaengine_generic_pcm_open - Open a dmaengine based PCM substream
> + * @substream: PCM substream
> + * @dev: pointer to DMA client device structure
> + * @name: DMA slave channel name
> + *
> + * Returns 0 on success, a negative error code otherwise.
> + *
> + * This function is a variant of snd_dmaengine_pcm_open(). It takes different
> + * parameters and calls generic DMA helper dma_request_slave_channel() to
> + * request a DMA channel from dmaengine.
> + */
> +int snd_dmaengine_generic_pcm_open(struct snd_pcm_substream *substream,
> +				   struct device *dev, const char *name)
> +{
> +	struct dmaengine_pcm_runtime_data *prtd;
> +	int ret;
> +
> +	ret = snd_pcm_hw_constraint_integer(substream->runtime,
> +					    SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0)
> +		return ret;
> +
> +	prtd = kzalloc(sizeof(*prtd), GFP_KERNEL);
> +	if (!prtd)
> +		return -ENOMEM;
> +
> +	prtd->dma_chan = dma_request_slave_channel(dev, name);
> +	if (!prtd->dma_chan) {
> +		kfree(prtd);
> +		return -ENXIO;
> +	}
> +
> +	substream->runtime->private_data = prtd;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_dmaengine_generic_pcm_open);
> +
> +/**
>    * snd_dmaengine_pcm_close - Close a dmaengine based PCM substream
>    * @substream: PCM substream
>    */
>
Vinod Koul March 21, 2013, 9:57 a.m. UTC | #2 
On Fri, Mar 15, 2013 at 11:36:41AM +0800, Shawn Guo wrote:
> With generic DMA device tree binding and helper function
> dma_request_slave_channel() in place, dmaengine_pcm should support
> that in requesting DMA channel for users that support generic DMA
> device tree binding.
> 
> Add a new API snd_dmaengine_generic_pcm_open() as the variant of
> snd_dmaengine_pcm_open().  It takes DMA client struct device pointer
> and slave channel name as arguments and calls generic DMA helper
> dma_request_slave_channel() to request DMA channel from dmaengine.
NAK

This is why we have dma_request_slave_channel_compat() API. You dont need to
write two open handlers here, just pass all the arguments and if DT is set it
will allocate a channel using dma_request_slave_channel() otherwise will do
dma_request_channel which is what you need.

> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: alsa-devel@alsa-project.org
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  include/sound/dmaengine_pcm.h |    2 ++
>  sound/soc/soc-dmaengine-pcm.c |   39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
> index b877334..452df15 100644
> --- a/include/sound/dmaengine_pcm.h
> +++ b/include/sound/dmaengine_pcm.h
> @@ -43,6 +43,8 @@ snd_pcm_uframes_t snd_dmaengine_pcm_pointer_no_residue(struct snd_pcm_substream
>  
>  int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>  	dma_filter_fn filter_fn, void *filter_data);
> +int snd_dmaengine_generic_pcm_open(struct snd_pcm_substream *substream,
> +				   struct device *dev, const char *name);
>  int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream);
>  
>  struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream);
> diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c
> index 111b7d92..970eb2b 100644
> --- a/sound/soc/soc-dmaengine-pcm.c
> +++ b/sound/soc/soc-dmaengine-pcm.c
> @@ -304,6 +304,45 @@ int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
>  EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open);
>  
>  /**
> + * snd_dmaengine_generic_pcm_open - Open a dmaengine based PCM substream
> + * @substream: PCM substream
> + * @dev: pointer to DMA client device structure
> + * @name: DMA slave channel name
> + *
> + * Returns 0 on success, a negative error code otherwise.
> + *
> + * This function is a variant of snd_dmaengine_pcm_open(). It takes different
> + * parameters and calls generic DMA helper dma_request_slave_channel() to
> + * request a DMA channel from dmaengine.
> + */
> +int snd_dmaengine_generic_pcm_open(struct snd_pcm_substream *substream,
> +				   struct device *dev, const char *name)
> +{
> +	struct dmaengine_pcm_runtime_data *prtd;
> +	int ret;
> +
> +	ret = snd_pcm_hw_constraint_integer(substream->runtime,
> +					    SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0)
> +		return ret;
> +
> +	prtd = kzalloc(sizeof(*prtd), GFP_KERNEL);
> +	if (!prtd)
> +		return -ENOMEM;
> +
> +	prtd->dma_chan = dma_request_slave_channel(dev, name);
> +	if (!prtd->dma_chan) {
> +		kfree(prtd);
> +		return -ENXIO;
> +	}
> +
> +	substream->runtime->private_data = prtd;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_dmaengine_generic_pcm_open);
> +
> +/**
>   * snd_dmaengine_pcm_close - Close a dmaengine based PCM substream
>   * @substream: PCM substream
>   */
> -- 
> 1.7.9.5
> 
>
Shawn Guo March 21, 2013, 2:53 p.m. UTC | #3 
On Thu, Mar 21, 2013 at 03:27:18PM +0530, Vinod Koul wrote:
> On Fri, Mar 15, 2013 at 11:36:41AM +0800, Shawn Guo wrote:
> > With generic DMA device tree binding and helper function
> > dma_request_slave_channel() in place, dmaengine_pcm should support
> > that in requesting DMA channel for users that support generic DMA
> > device tree binding.
> > 
> > Add a new API snd_dmaengine_generic_pcm_open() as the variant of
> > snd_dmaengine_pcm_open().  It takes DMA client struct device pointer
> > and slave channel name as arguments and calls generic DMA helper
> > dma_request_slave_channel() to request DMA channel from dmaengine.
> NAK
> 
> This is why we have dma_request_slave_channel_compat() API. You dont need to
> write two open handlers here, just pass all the arguments and if DT is set it
> will allocate a channel using dma_request_slave_channel() otherwise will do
> dma_request_channel which is what you need.

I do not quite follow your comment.  Let me try to understand it.  Are
you suggesting that instead of the current call path:

	snd_dmaengine_pcm_open()
		dmaengine_pcm_request_channel()
			dma_request_channel()

we should change it as below?

	snd_dmaengine_pcm_open()
		dmaengine_pcm_request_channel()
			dma_request_slave_channel_compat()

If that's the case, you are fundamentally requesting to change the
fingerprint of API snd_dmaengine_pcm_open() from the existing:

int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
			   dma_filter_fn filter_fn, void *filter_data)

to something like:

int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
			   dma_filter_fn filter_fn, void *filter_data,
			   struct device *dev, const char *name)

So every single user of snd_dmaengine_pcm_open() needs to adapt to the
interface change.

Is my understanding correct?  Or have I misunderstood your comment?

Shawn
Sebastien Guiriec March 22, 2013, 8:07 a.m. UTC | #4 
On 03/21/2013 03:53 PM, Shawn Guo wrote:
> On Thu, Mar 21, 2013 at 03:27:18PM +0530, Vinod Koul wrote:
>> On Fri, Mar 15, 2013 at 11:36:41AM +0800, Shawn Guo wrote:
>>> With generic DMA device tree binding and helper function
>>> dma_request_slave_channel() in place, dmaengine_pcm should support
>>> that in requesting DMA channel for users that support generic DMA
>>> device tree binding.
>>>
>>> Add a new API snd_dmaengine_generic_pcm_open() as the variant of
>>> snd_dmaengine_pcm_open().  It takes DMA client struct device pointer
>>> and slave channel name as arguments and calls generic DMA helper
>>> dma_request_slave_channel() to request DMA channel from dmaengine.
>> NAK
>>
>> This is why we have dma_request_slave_channel_compat() API. You dont need to
>> write two open handlers here, just pass all the arguments and if DT is set it
>> will allocate a channel using dma_request_slave_channel() otherwise will do
>> dma_request_channel which is what you need.
>
> I do not quite follow your comment.  Let me try to understand it.  Are
> you suggesting that instead of the current call path:
>
> 	snd_dmaengine_pcm_open()
> 		dmaengine_pcm_request_channel()
> 			dma_request_channel()
>
> we should change it as below?
>
> 	snd_dmaengine_pcm_open()
> 		dmaengine_pcm_request_channel()
> 			dma_request_slave_channel_compat()
>
> If that's the case, you are fundamentally requesting to change the
> fingerprint of API snd_dmaengine_pcm_open() from the existing:
>
> int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
> 			   dma_filter_fn filter_fn, void *filter_data)
>
> to something like:
>
> int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
> 			   dma_filter_fn filter_fn, void *filter_data,
> 			   struct device *dev, const char *name)
>
> So every single user of snd_dmaengine_pcm_open() needs to adapt to the
> interface change.
>
> Is my understanding correct?  Or have I misunderstood your comment?

This is what has been done on OMAP for other IPs. I have tested this 
solution for Audio before you submit your first series and it is working.

Now I will wait Lars serie to check again on OMAP and let Mark comment 
on the best approach. Whereas update current API like you propose or 
wait Lars series and move on according to the different comments done on 
open function. As OMAP audio DMA binding are depending on the chosen 
solution.

Sebastien.
>
> Shawn
>
>
Shawn Guo March 22, 2013, 8:39 a.m. UTC | #5 
On Fri, Mar 22, 2013 at 09:07:21AM +0100, Sebastien Guiriec wrote:
> Now I will wait Lars serie to check again on OMAP and let Mark
> comment on the best approach. Whereas update current API like you
> propose or wait Lars series and move on according to the different
> comments done on open function. As OMAP audio DMA binding are
> depending on the chosen solution.

This the case for everyone else who is moving their audio driver to
generic DMA bindings.  For mxs platform that I'm working on, audio
becomes the only one not moving to generic DMA bindings.

Shawn
diff mbox

Patch

diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index b877334..452df15 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -43,6 +43,8 @@  snd_pcm_uframes_t snd_dmaengine_pcm_pointer_no_residue(struct snd_pcm_substream
 
 int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
 	dma_filter_fn filter_fn, void *filter_data);
+int snd_dmaengine_generic_pcm_open(struct snd_pcm_substream *substream,
+				   struct device *dev, const char *name);
 int snd_dmaengine_pcm_close(struct snd_pcm_substream *substream);
 
 struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream);
diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c
index 111b7d92..970eb2b 100644
--- a/sound/soc/soc-dmaengine-pcm.c
+++ b/sound/soc/soc-dmaengine-pcm.c
@@ -304,6 +304,45 @@  int snd_dmaengine_pcm_open(struct snd_pcm_substream *substream,
 EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_open);
 
 /**
+ * snd_dmaengine_generic_pcm_open - Open a dmaengine based PCM substream
+ * @substream: PCM substream
+ * @dev: pointer to DMA client device structure
+ * @name: DMA slave channel name
+ *
+ * Returns 0 on success, a negative error code otherwise.
+ *
+ * This function is a variant of snd_dmaengine_pcm_open(). It takes different
+ * parameters and calls generic DMA helper dma_request_slave_channel() to
+ * request a DMA channel from dmaengine.
+ */
+int snd_dmaengine_generic_pcm_open(struct snd_pcm_substream *substream,
+				   struct device *dev, const char *name)
+{
+	struct dmaengine_pcm_runtime_data *prtd;
+	int ret;
+
+	ret = snd_pcm_hw_constraint_integer(substream->runtime,
+					    SNDRV_PCM_HW_PARAM_PERIODS);
+	if (ret < 0)
+		return ret;
+
+	prtd = kzalloc(sizeof(*prtd), GFP_KERNEL);
+	if (!prtd)
+		return -ENOMEM;
+
+	prtd->dma_chan = dma_request_slave_channel(dev, name);
+	if (!prtd->dma_chan) {
+		kfree(prtd);
+		return -ENXIO;
+	}
+
+	substream->runtime->private_data = prtd;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_dmaengine_generic_pcm_open);
+
+/**
  * snd_dmaengine_pcm_close - Close a dmaengine based PCM substream
  * @substream: PCM substream
  */