[alsa-devel] [PATCH 0/2] ASoC: dmaengine_pcm: support generic DMA binding users
The series adds a snd_dmaengine_generic_pcm_open() for users that adopt generic DMA binding and helplers.
Vinod,
AS per the request from Mark, I add the first patch to mark name parameter of dmaengine helpers as const, so that clients can mark const on their side.
We need to have these two patches on a topic branch, as I've heard a few people converting their ASoC driver needing the patches. Can you ack the first patch, so that Mark can maintain them in a branch for anyone who needs it to pull?
Shawn Guo (2): dmaengine: add const for name parameter ASoC: dmaengine_pcm: add snd_dmaengine_generic_pcm_open()
drivers/dma/dmaengine.c | 2 +- drivers/dma/of-dma.c | 6 +++--- include/linux/dmaengine.h | 7 ++++--- include/linux/of_dma.h | 2 +- include/sound/dmaengine_pcm.h | 2 ++ sound/soc/soc-dmaengine-pcm.c | 39 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 50 insertions(+), 8 deletions(-)
Mark name parameter of generic device tree helpers as const, as it shouldn't be changed anyway.
Signed-off-by: Shawn Guo shawn.guo@linaro.org --- drivers/dma/dmaengine.c | 2 +- drivers/dma/of-dma.c | 6 +++--- include/linux/dmaengine.h | 7 ++++--- include/linux/of_dma.h | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index b2728d6..2cbfefe 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -555,7 +555,7 @@ EXPORT_SYMBOL_GPL(__dma_request_channel); * @dev: pointer to client device structure * @name: slave channel name */ -struct dma_chan *dma_request_slave_channel(struct device *dev, char *name) +struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name) { /* If device-tree is present get slave info from here */ if (dev->of_node) diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 69d04d2..6036cd0 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -172,8 +172,8 @@ EXPORT_SYMBOL_GPL(of_dma_controller_free); * specifiers, matches the name provided. Returns 0 if the name matches and * a valid pointer to the DMA specifier is found. Otherwise returns -ENODEV. */ -static int of_dma_match_channel(struct device_node *np, char *name, int index, - struct of_phandle_args *dma_spec) +static int of_dma_match_channel(struct device_node *np, const char *name, + int index, struct of_phandle_args *dma_spec) { const char *s;
@@ -198,7 +198,7 @@ static int of_dma_match_channel(struct device_node *np, char *name, int index, * Returns pointer to appropriate dma channel on success or NULL on error. */ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, - char *name) + const char *name) { struct of_phandle_args dma_spec; struct of_dma *ofdma; diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 91ac8da..68b4252 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -968,7 +968,8 @@ enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie); enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx); void dma_issue_pending_all(void); struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param); -struct dma_chan *dma_request_slave_channel(struct device *dev, char *name); +struct dma_chan *dma_request_slave_channel(struct device *dev, + const char *name); void dma_release_channel(struct dma_chan *chan); #else static inline enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx) @@ -984,7 +985,7 @@ static inline struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, return NULL; } static inline struct dma_chan *dma_request_slave_channel(struct device *dev, - char *name) + const char *name) { return NULL; } @@ -1007,7 +1008,7 @@ struct dma_chan *net_dma_find_channel(void); static inline struct dma_chan *__dma_request_slave_channel_compat(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param, struct device *dev, - char *name) + const char *name) { struct dma_chan *chan;
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h index d15073e..0c4a82e 100644 --- a/include/linux/of_dma.h +++ b/include/linux/of_dma.h @@ -40,7 +40,7 @@ extern int of_dma_controller_register(struct device_node *np, void *data); extern int of_dma_controller_free(struct device_node *np); extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np, - char *name); + const char *name); extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec, struct of_dma *ofdma); #else
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 */
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
*/
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
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
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
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
On Fri, Mar 15, 2013 at 11:36:39AM +0800, Shawn Guo wrote:
The series adds a snd_dmaengine_generic_pcm_open() for users that adopt generic DMA binding and helplers.
Vinod,
Ping?
Shawn
AS per the request from Mark, I add the first patch to mark name parameter of dmaengine helpers as const, so that clients can mark const on their side.
We need to have these two patches on a topic branch, as I've heard a few people converting their ASoC driver needing the patches. Can you ack the first patch, so that Mark can maintain them in a branch for anyone who needs it to pull?
Shawn Guo (2): dmaengine: add const for name parameter ASoC: dmaengine_pcm: add snd_dmaengine_generic_pcm_open()
drivers/dma/dmaengine.c | 2 +- drivers/dma/of-dma.c | 6 +++--- include/linux/dmaengine.h | 7 ++++--- include/linux/of_dma.h | 2 +- include/sound/dmaengine_pcm.h | 2 ++ sound/soc/soc-dmaengine-pcm.c | 39 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 50 insertions(+), 8 deletions(-)
-- 1.7.9.5
On 03/21/2013 03:39 AM, Shawn Guo wrote:
On Fri, Mar 15, 2013 at 11:36:39AM +0800, Shawn Guo wrote:
The series adds a snd_dmaengine_generic_pcm_open() for users that adopt generic DMA binding and helplers.
Vinod,
Ping?
Hm, I only saw this series today would have been good to be on Cc. I've been working on something very similar. My series goes a bit further though, it implements an (almost generic) dmaengine based PCM driver using the of bindings. So you need almost no platform code. The only things that are platform specific at the moment is the pcm_hardware struct, but I'd like to replace that in the future with something that queries the pcm hardware parameter like max_period from the DMA engine driver. And another bit that is still driver specific is a callback that fills the dma_slave_config struct.
In my series the channels are requested at probe time, so it is possible to handle -EPROBE_DEFER properly and also we can allocate the audio buffers with the dma device instead of the sound device, so stupid hacks like
card->dev->dma_mask = &dma_mask; card->dev->coherent_dma_mask = DMA_BIT_MASK(32);
anymore. I'll try to post the series tomorrow.
- Lars
Shawn
AS per the request from Mark, I add the first patch to mark name parameter of dmaengine helpers as const, so that clients can mark const on their side.
We need to have these two patches on a topic branch, as I've heard a few people converting their ASoC driver needing the patches. Can you ack the first patch, so that Mark can maintain them in a branch for anyone who needs it to pull?
Shawn Guo (2): dmaengine: add const for name parameter ASoC: dmaengine_pcm: add snd_dmaengine_generic_pcm_open()
drivers/dma/dmaengine.c | 2 +- drivers/dma/of-dma.c | 6 +++--- include/linux/dmaengine.h | 7 ++++--- include/linux/of_dma.h | 2 +- include/sound/dmaengine_pcm.h | 2 ++ sound/soc/soc-dmaengine-pcm.c | 39 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 50 insertions(+), 8 deletions(-)
-- 1.7.9.5
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Mar 21, 2013 at 04:06:27PM +0100, Lars-Peter Clausen wrote:
Hm, I only saw this series today would have been good to be on Cc. I've been working on something very similar. My series goes a bit further though, it implements an (almost generic) dmaengine based PCM driver using the of bindings. So you need almost no platform code. The only things that are platform specific at the moment is the pcm_hardware struct, but I'd like to replace that in the future with something that queries the pcm hardware parameter like max_period from the DMA engine driver. And another bit that is still driver specific is a callback that fills the dma_slave_config struct.
FWIW it might be worth looking at the one rmk wrote but has never wanted to submit for whatever reason.
In my series the channels are requested at probe time, so it is possible to handle -EPROBE_DEFER properly and also we can allocate the audio buffers with the dma device instead of the sound device, so stupid hacks like
card->dev->dma_mask = &dma_mask; card->dev->coherent_dma_mask = DMA_BIT_MASK(32);
anymore. I'll try to post the series tomorrow.
This is the main thing his code has got that the library hasn't, it's certainly the only issue he keeps mentioning.
On 03/21/2013 04:22 PM, Mark Brown wrote:
On Thu, Mar 21, 2013 at 04:06:27PM +0100, Lars-Peter Clausen wrote:
Hm, I only saw this series today would have been good to be on Cc. I've been working on something very similar. My series goes a bit further though, it implements an (almost generic) dmaengine based PCM driver using the of bindings. So you need almost no platform code. The only things that are platform specific at the moment is the pcm_hardware struct, but I'd like to replace that in the future with something that queries the pcm hardware parameter like max_period from the DMA engine driver. And another bit that is still driver specific is a callback that fills the dma_slave_config struct.
FWIW it might be worth looking at the one rmk wrote but has never wanted to submit for whatever reason.
Ideally I'd like to eventually move this 'fill in slave config' callback to the DAI driver, since this is almost always DAI specific data, like for example the FIFO address, the burst length, the bus width, etc. But I'd like to do that in a second step. The first step should already help us to get rid of a large portion of redundant code we see in PCM drivers today.
In my series the channels are requested at probe time, so it is possible to handle -EPROBE_DEFER properly and also we can allocate the audio buffers with the dma device instead of the sound device, so stupid hacks like
card->dev->dma_mask = &dma_mask; card->dev->coherent_dma_mask = DMA_BIT_MASK(32);anymore. I'll try to post the series tomorrow.
This is the main thing his code has got that the library hasn't, it's certainly the only issue he keeps mentioning.
Yes, he definitely deserves credit for this, I probably wouldn't have implemented it, if he hadn't point this out.
The problem with the current library is that we don't know yet which dmaengine device is going to be used by the time we pre-allocate the audio buffers, since the DMA filter parameters often are provided by the DAI driver. When using devicetree on the other hand the handle to the dmaengine comes from the devicetree itself and so it is available at the time we probe the PCM driver.
There are some other drivers which don't really depend on runtime filter parameters, e.g. tegra, which doesn't use a filter function. So tegra can also easily make use of this new generic dmaengine based PCM driver.
- Lars
On Thu, Mar 21, 2013 at 05:26:19PM +0100, Lars-Peter Clausen wrote:
a second step. The first step should already help us to get rid of a large portion of redundant code we see in PCM drivers today.
Indeed, factor everything we can out into common code and then improve the common code in a single place.
There are some other drivers which don't really depend on runtime filter parameters, e.g. tegra, which doesn't use a filter function. So tegra can also easily make use of this new generic dmaengine based PCM driver.
Tegra's DT only these days anyway of course.
On Thu, Mar 21, 2013 at 05:26:19PM +0100, Lars-Peter Clausen wrote:
The problem with the current library is that we don't know yet which dmaengine device is going to be used by the time we pre-allocate the audio buffers, since the DMA filter parameters often are provided by the DAI driver.
That's not really a problem. The CPU DAI can provide the data via the playback_dma_data and capture_dma_data pointers, which the ASoC DMA engine driver can pick up on when it initializes. This data can contain everything that the ASoC DMA engine driver needs.
Where my driver falls down is if you need to reconfigure the DMA engine later - it doesn't provide an API for that, but that's trivially easy to augment the API.
If you want to look at what I did for sa11x0 Assabet platforms, it's still all here:
http://ftp.arm.linux.org.uk/git/gitweb.cgi?p=linux-arm.git;a=shortlog;h=2f03...
As you will see from the dates, it's over a year old now - the only thing I do with it is occasionally rebase it to bring it up to a more modern kernel.
On 03/22/2013 12:30 PM, Russell King - ARM Linux wrote:
On Thu, Mar 21, 2013 at 05:26:19PM +0100, Lars-Peter Clausen wrote:
The problem with the current library is that we don't know yet which dmaengine device is going to be used by the time we pre-allocate the audio buffers, since the DMA filter parameters often are provided by the DAI driver.
That's not really a problem. The CPU DAI can provide the data via the playback_dma_data and capture_dma_data pointers, which the ASoC DMA engine driver can pick up on when it initializes. This data can contain everything that the ASoC DMA engine driver needs.
Yes and no. Unfortunately it doesn't work for all drivers. But yes that's the way I want to move forward for the non dt case. Let the channels be requested in the pcm_new callback.
Where my driver falls down is if you need to reconfigure the DMA engine later - it doesn't provide an API for that, but that's trivially easy to augment the API.
If you want to look at what I did for sa11x0 Assabet platforms, it's still all here:
http://ftp.arm.linux.org.uk/git/gitweb.cgi?p=linux-arm.git;a=shortlog;h=2f03...
As you will see from the dates, it's over a year old now - the only thing I do with it is occasionally rebase it to bring it up to a more modern kernel.
On Thu, Mar 21, 2013 at 04:22:06PM +0100, Mark Brown wrote:
On Thu, Mar 21, 2013 at 04:06:27PM +0100, Lars-Peter Clausen wrote:
Hm, I only saw this series today would have been good to be on Cc. I've been working on something very similar. My series goes a bit further though, it implements an (almost generic) dmaengine based PCM driver using the of bindings. So you need almost no platform code. The only things that are platform specific at the moment is the pcm_hardware struct, but I'd like to replace that in the future with something that queries the pcm hardware parameter like max_period from the DMA engine driver. And another bit that is still driver specific is a callback that fills the dma_slave_config struct.
FWIW it might be worth looking at the one rmk wrote but has never wanted to submit for whatever reason.
Err no, stop twisting the facts. I know nothing is ever your fault. You rejected it because it was providing support for non-cyclic supporting DMA engine drivers.
I've since added support to it for cyclic DMA engines, but I've retained the non-cyclic support in it because I don't see why I should remove it when it works for me, especially given the difficulties with getting anything in sound/soc changed once its been merged.
Plus, as I've already said to you, I no longer develop and test it because the platform I was using is now doing service as my firewall, and you'll forgive me for not wanting to take the whole of *.arm.linux.org.uk offline to mess around with ASoC stuff. But that's not to say I don't care about the issue.
On Fri, Mar 22, 2013 at 11:17:04AM +0000, Russell King - ARM Linux wrote:
On Thu, Mar 21, 2013 at 04:22:06PM +0100, Mark Brown wrote:
FWIW it might be worth looking at the one rmk wrote but has never wanted to submit for whatever reason.
Err no, stop twisting the facts. I know nothing is ever your fault. You rejected it because it was providing support for non-cyclic supporting DMA engine drivers.
That might well have been one of the reasons now you mention it, I was just remembering that the code has never been posted to the list, I've only ever viewed it in your git trees. IIRC it was only mentioned on the list after the currently merged library was posted and you weren't keen on submitting at that point due as you weren't happy with the platform code using due to that code having to jump through hoops to support some unusual hardware design decisions.
I've since added support to it for cyclic DMA engines, but I've retained the non-cyclic support in it because I don't see why I should remove it when it works for me, especially given the difficulties with getting
We need support for both cyclic and non-cyclic devices, one doesn't preclude the other and both kinds of hardware exist. Cyclic is much more common and generally more desirable but that doesn't mean that hardware designers always provide it.
anything in sound/soc changed once its been merged.
I'm sorry, what difficulties are those?
Plus, as I've already said to you, I no longer develop and test it because the platform I was using is now doing service as my firewall, and you'll forgive me for not wanting to take the whole of *.arm.linux.org.uk offline to mess around with ASoC stuff. But that's not to say I don't care about the issue.
Indeed, I understand why you've stopped now.
On 03/22/2013 12:28 PM, Mark Brown wrote:
On Fri, Mar 22, 2013 at 11:17:04AM +0000, Russell King - ARM Linux wrote:
On Thu, Mar 21, 2013 at 04:22:06PM +0100, Mark Brown wrote:
FWIW it might be worth looking at the one rmk wrote but has never wanted to submit for whatever reason.
Err no, stop twisting the facts. I know nothing is ever your fault. You rejected it because it was providing support for non-cyclic supporting DMA engine drivers.
That might well have been one of the reasons now you mention it, I was just remembering that the code has never been posted to the list, I've only ever viewed it in your git trees. IIRC it was only mentioned on the list after the currently merged library was posted and you weren't keen on submitting at that point due as you weren't happy with the platform code using due to that code having to jump through hoops to support some unusual hardware design decisions.
I've since added support to it for cyclic DMA engines, but I've retained the non-cyclic support in it because I don't see why I should remove it when it works for me, especially given the difficulties with getting
We need support for both cyclic and non-cyclic devices, one doesn't preclude the other and both kinds of hardware exist. Cyclic is much more common and generally more desirable but that doesn't mean that hardware designers always provide it.
The idea was to only have support for the cyclic dmaengine API in the ASoC PCM driver and deal with the emulation of cyclic transfers for hardware, which does not implement it, at the dmaengine layer.
anything in sound/soc changed once its been merged.
I'm sorry, what difficulties are those?
Plus, as I've already said to you, I no longer develop and test it because the platform I was using is now doing service as my firewall, and you'll forgive me for not wanting to take the whole of *.arm.linux.org.uk offline to mess around with ASoC stuff. But that's not to say I don't care about the issue.
Indeed, I understand why you've stopped now.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Fri, Mar 22, 2013 at 12:42:44PM +0100, Lars-Peter Clausen wrote:
On 03/22/2013 12:28 PM, Mark Brown wrote:
We need support for both cyclic and non-cyclic devices, one doesn't preclude the other and both kinds of hardware exist. Cyclic is much more common and generally more desirable but that doesn't mean that hardware designers always provide it.
The idea was to only have support for the cyclic dmaengine API in the ASoC PCM driver and deal with the emulation of cyclic transfers for hardware, which does not implement it, at the dmaengine layer.
Perhaps I'm misremembering but I thought there was pushback on that on the basis that there are few if any other users of cyclic DMA so nobody else cares or wants to deal with the hassle? A quick grep seems not to turn up any other users.
participants (6)
-
Lars-Peter Clausen -
Mark Brown -
Russell King - ARM Linux -
Sebastien Guiriec -
Shawn Guo -
Vinod Koul