[alsa-devel] [PATCH] ASoC: davinci-mcasp: Choose PCM driver based on configured DMA controller
Find the configured DMA controller by asking for a DMA channel in the probe phase and releasing it right after. The controller device can be found via the dma_chan struct and the controller is recognized from the compatible property of its device node. The patch assumes EDMA if there is no device node.
Signed-off-by: Jyri Sarha jsarha@ti.com --- sound/soc/davinci/davinci-mcasp.c | 61 ++++++++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 10 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index d793494..5bc2712 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -1565,6 +1565,46 @@ static int davinci_mcasp_init_ch_constraints(struct davinci_mcasp *mcasp) return ret; }
+enum { + MCASP_EDMA = 1, + MCASP_SDMA, +}; +static const char *sdma_prefix = "ti,omap"; + +static int davinci_mcasp_get_dma_type(struct davinci_mcasp *mcasp) +{ + struct dma_chan *chan; + const char *tmp; + int ret = MCASP_EDMA; + + tmp = mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data; + chan = dma_request_slave_channel_reason(mcasp->dev, tmp); + if (IS_ERR(chan)) { + if (PTR_ERR(chan) != -EPROBE_DEFER) + dev_err(mcasp->dev, + "Can't verify DMA configuration (%ld)\n", + PTR_ERR(chan)); + return PTR_ERR(chan); + } + BUG_ON(!chan->device || !chan->device->dev); + + if (chan->device->dev->of_node) + ret = of_property_read_string(chan->device->dev->of_node, + "compatible", &tmp); + else + dev_dbg(mcasp->dev, "DMA controller has no of-node\n"); + + dma_release_channel(chan); + if (ret) + return ret; + + dev_dbg(mcasp->dev, "DMA controller compatible = "%s"\n", tmp); + if (!strncmp(tmp, sdma_prefix, strlen(sdma_prefix))) + return MCASP_SDMA; + + return MCASP_EDMA; +} + static int davinci_mcasp_probe(struct platform_device *pdev) { struct snd_dmaengine_dai_dma_data *dma_data; @@ -1763,27 +1803,28 @@ static int davinci_mcasp_probe(struct platform_device *pdev) if (ret != 0) goto err;
- switch (mcasp->version) { + ret = -EINVAL; + switch (davinci_mcasp_get_dma_type(mcasp)) { + case MCASP_EDMA: #if IS_BUILTIN(CONFIG_SND_EDMA_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_EDMA_SOC)) - case MCASP_VERSION_1: - case MCASP_VERSION_2: - case MCASP_VERSION_3: ret = edma_pcm_platform_register(&pdev->dev); - break; +#else + dev_err(&pdev->dev, "Missing SND_EDMA_SOC\n"); #endif + break; + case MCASP_SDMA: #if IS_BUILTIN(CONFIG_SND_OMAP_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_OMAP_SOC)) - case MCASP_VERSION_4: ret = omap_pcm_platform_register(&pdev->dev); - break; +#else + dev_err(&pdev->dev, "Missing SND_SDMA_SOC\n"); #endif + break; default: - dev_err(&pdev->dev, "Invalid McASP version: %d\n", - mcasp->version); - ret = -EINVAL; + dev_err(&pdev->dev, "No DMA controller found\n"); break; }
On 06/02/2015 11:58 AM, Jyri Sarha wrote:
Find the configured DMA controller by asking for a DMA channel in the probe phase and releasing it right after. The controller device can be found via the dma_chan struct and the controller is recognized from the compatible property of its device node. The patch assumes EDMA if there is no device node.
Signed-off-by: Jyri Sarha jsarha@ti.com
sound/soc/davinci/davinci-mcasp.c | 61 ++++++++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 10 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index d793494..5bc2712 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -1565,6 +1565,46 @@ static int davinci_mcasp_init_ch_constraints(struct davinci_mcasp *mcasp) return ret; }
+enum {
- MCASP_EDMA = 1,
Why start from 1? I'm not sure about the name. It somehow implies that McASP has something to do with the dma engine. MCASP_SERVICED_BY_E/SDMA PLATFORM_USES_E/SDMA
I can not come up with a decent name :(
- MCASP_SDMA,
+}; +static const char *sdma_prefix = "ti,omap";
+static int davinci_mcasp_get_dma_type(struct davinci_mcasp *mcasp) +{
- struct dma_chan *chan;
- const char *tmp;
- int ret = MCASP_EDMA;
- tmp = mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data;
- chan = dma_request_slave_channel_reason(mcasp->dev, tmp);
this will return with error in case of !dev->of_node. The first check should be: if (!mcasp->dev->of_node) return MCASP_EDMA;
since we can only boot in legacy mode with daVinci devices and they are using eDMA.
- if (IS_ERR(chan)) {
if (PTR_ERR(chan) != -EPROBE_DEFER)dev_err(mcasp->dev,"Can't verify DMA configuration (%ld)\n",PTR_ERR(chan));return PTR_ERR(chan);- }
- BUG_ON(!chan->device || !chan->device->dev);
- if (chan->device->dev->of_node)
ret = of_property_read_string(chan->device->dev->of_node,"compatible", &tmp);- else
dev_dbg(mcasp->dev, "DMA controller has no of-node\n");
Now this is not likely to happen...
- dma_release_channel(chan);
- if (ret)
return ret;- dev_dbg(mcasp->dev, "DMA controller compatible = "%s"\n", tmp);
- if (!strncmp(tmp, sdma_prefix, strlen(sdma_prefix)))
return MCASP_SDMA;- return MCASP_EDMA;
+}
static int davinci_mcasp_probe(struct platform_device *pdev) { struct snd_dmaengine_dai_dma_data *dma_data; @@ -1763,27 +1803,28 @@ static int davinci_mcasp_probe(struct platform_device *pdev) if (ret != 0) goto err;
- switch (mcasp->version) {
- ret = -EINVAL;
Why? What was the problem with setting the ret in the default case?
- switch (davinci_mcasp_get_dma_type(mcasp)) {
- case MCASP_EDMA:
#if IS_BUILTIN(CONFIG_SND_EDMA_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_EDMA_SOC))
- case MCASP_VERSION_1:
- case MCASP_VERSION_2:
- case MCASP_VERSION_3: ret = edma_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_EDMA_SOC\n");#endif
break;- case MCASP_SDMA:
#if IS_BUILTIN(CONFIG_SND_OMAP_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_OMAP_SOC))
- case MCASP_VERSION_4: ret = omap_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_SDMA_SOC\n");#endif
break;
dev_err(&pdev->dev, "Invalid McASP version: %d\n",mcasp->version);ret = -EINVAL;
dev_err(&pdev->dev, "No DMA controller found\n");
On 06/02/15 13:31, Peter Ujfalusi wrote:
On 06/02/2015 11:58 AM, Jyri Sarha wrote:
Find the configured DMA controller by asking for a DMA channel in the probe phase and releasing it right after. The controller device can be found via the dma_chan struct and the controller is recognized from the compatible property of its device node. The patch assumes EDMA if there is no device node.
Signed-off-by: Jyri Sarha jsarha@ti.com
sound/soc/davinci/davinci-mcasp.c | 61 ++++++++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 10 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index d793494..5bc2712 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -1565,6 +1565,46 @@ static int davinci_mcasp_init_ch_constraints(struct davinci_mcasp *mcasp) return ret; }
+enum {
- MCASP_EDMA = 1,
Why start from 1?
This is just an implementation detail, for "if" statement below. Maybe I should restucture for better readability.
I'm not sure about the name. It somehow implies that McASP has something to do with the dma engine. MCASP_SERVICED_BY_E/SDMA PLATFORM_USES_E/SDMA
I can not come up with a decent name :(
What about PCM_EDMA/PCM_SDMA? This is a local definition after all.
- MCASP_SDMA,
+}; +static const char *sdma_prefix = "ti,omap";
+static int davinci_mcasp_get_dma_type(struct davinci_mcasp *mcasp) +{
- struct dma_chan *chan;
- const char *tmp;
- int ret = MCASP_EDMA;
- tmp = mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data;
- chan = dma_request_slave_channel_reason(mcasp->dev, tmp);
this will return with error in case of !dev->of_node. The first check should be: if (!mcasp->dev->of_node) return MCASP_EDMA;
since we can only boot in legacy mode with daVinci devices and they are using eDMA.
- if (IS_ERR(chan)) {
if (PTR_ERR(chan) != -EPROBE_DEFER)dev_err(mcasp->dev,"Can't verify DMA configuration (%ld)\n",PTR_ERR(chan));return PTR_ERR(chan);- }
- BUG_ON(!chan->device || !chan->device->dev);
- if (chan->device->dev->of_node)
ret = of_property_read_string(chan->device->dev->of_node,"compatible", &tmp);- else
dev_dbg(mcasp->dev, "DMA controller has no of-node\n");Now this is not likely to happen...
Not having of_node happens on am335x, but not having compatible property is unlikely...
- dma_release_channel(chan);
- if (ret)
return ret;- dev_dbg(mcasp->dev, "DMA controller compatible = "%s"\n", tmp);
- if (!strncmp(tmp, sdma_prefix, strlen(sdma_prefix)))
return MCASP_SDMA;- return MCASP_EDMA;
+}
- static int davinci_mcasp_probe(struct platform_device *pdev) { struct snd_dmaengine_dai_dma_data *dma_data;
@@ -1763,27 +1803,28 @@ static int davinci_mcasp_probe(struct platform_device *pdev) if (ret != 0) goto err;
- switch (mcasp->version) {
- ret = -EINVAL;
Why? What was the problem with setting the ret in the default case?
- switch (davinci_mcasp_get_dma_type(mcasp)) {
- case MCASP_EDMA: #if IS_BUILTIN(CONFIG_SND_EDMA_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_EDMA_SOC))
- case MCASP_VERSION_1:
- case MCASP_VERSION_2:
- case MCASP_VERSION_3: ret = edma_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_EDMA_SOC\n");break;- case MCASP_SDMA: #if IS_BUILTIN(CONFIG_SND_OMAP_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_OMAP_SOC))
- case MCASP_VERSION_4: ret = omap_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_SDMA_SOC\n");break;
dev_err(&pdev->dev, "Invalid McASP version: %d\n",mcasp->version);ret = -EINVAL;
dev_err(&pdev->dev, "No DMA controller found\n");
On 06/02/2015 01:40 PM, Jyri Sarha wrote:
+enum {
- MCASP_EDMA = 1,
Why start from 1?
This is just an implementation detail, for "if" statement below. Maybe I should restucture for better readability.
No need to change if it makes the code flow better. I was just asking.
I'm not sure about the name. It somehow implies that McASP has something to do with the dma engine. MCASP_SERVICED_BY_E/SDMA PLATFORM_USES_E/SDMA
I can not come up with a decent name :(
What about PCM_EDMA/PCM_SDMA? This is a local definition after all.
Sounds much better!
- MCASP_SDMA,
+}; +static const char *sdma_prefix = "ti,omap";
+static int davinci_mcasp_get_dma_type(struct davinci_mcasp *mcasp) +{
- struct dma_chan *chan;
- const char *tmp;
- int ret = MCASP_EDMA;
- tmp = mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data;
- chan = dma_request_slave_channel_reason(mcasp->dev, tmp);
this will return with error in case of !dev->of_node. The first check should be: if (!mcasp->dev->of_node) return MCASP_EDMA;
since we can only boot in legacy mode with daVinci devices and they are using eDMA.
- if (IS_ERR(chan)) {
if (PTR_ERR(chan) != -EPROBE_DEFER)dev_err(mcasp->dev,"Can't verify DMA configuration (%ld)\n",PTR_ERR(chan));return PTR_ERR(chan);- }
- BUG_ON(!chan->device || !chan->device->dev);
- if (chan->device->dev->of_node)
ret = of_property_read_string(chan->device->dev->of_node,"compatible", &tmp);- else
dev_dbg(mcasp->dev, "DMA controller has no of-node\n");Now this is not likely to happen...
Not having of_node happens on am335x, but not having compatible property is unlikely...
Yes, you are right. The edma dmaengine driver is not probed via DT, but the arch/arm/common/edma.c will create a platform device for it to load, so it does not have of_node. On the other hand omap-dma is probed via DT.
- dma_release_channel(chan);
- if (ret)
return ret;- dev_dbg(mcasp->dev, "DMA controller compatible = "%s"\n", tmp);
- if (!strncmp(tmp, sdma_prefix, strlen(sdma_prefix)))
return MCASP_SDMA;- return MCASP_EDMA;
+}
- static int davinci_mcasp_probe(struct platform_device *pdev) { struct snd_dmaengine_dai_dma_data *dma_data;
@@ -1763,27 +1803,28 @@ static int davinci_mcasp_probe(struct platform_device *pdev) if (ret != 0) goto err;
- switch (mcasp->version) {
- ret = -EINVAL;
Why? What was the problem with setting the ret in the default case?
- switch (davinci_mcasp_get_dma_type(mcasp)) {
- case MCASP_EDMA: #if IS_BUILTIN(CONFIG_SND_EDMA_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_EDMA_SOC))
- case MCASP_VERSION_1:
- case MCASP_VERSION_2:
- case MCASP_VERSION_3: ret = edma_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_EDMA_SOC\n");break;- case MCASP_SDMA: #if IS_BUILTIN(CONFIG_SND_OMAP_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_OMAP_SOC))
- case MCASP_VERSION_4: ret = omap_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_SDMA_SOC\n");break; default:
dev_err(&pdev->dev, "Invalid McASP version: %d\n",mcasp->version);ret = -EINVAL;
dev_err(&pdev->dev, "No DMA controller found\n"); break; }
On Tue, Jun 02, 2015 at 01:31:28PM +0300, Peter Ujfalusi wrote:
On 06/02/2015 11:58 AM, Jyri Sarha wrote:
+enum {
- MCASP_EDMA = 1,
Why start from 1?
It's a fairly common style to ensure that zero initialised structures don't have a default value. Don't know if that's the intent here or not.
I missed two comments first time around...
On 06/02/15 13:31, Peter Ujfalusi wrote:
On 06/02/2015 11:58 AM, Jyri Sarha wrote:
Find the configured DMA controller by asking for a DMA channel in the probe phase and releasing it right after. The controller device can be found via the dma_chan struct and the controller is recognized from the compatible property of its device node. The patch assumes EDMA if there is no device node.
Signed-off-by: Jyri Sarha jsarha@ti.com
sound/soc/davinci/davinci-mcasp.c | 61 ++++++++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 10 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index d793494..5bc2712 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -1565,6 +1565,46 @@ static int davinci_mcasp_init_ch_constraints(struct davinci_mcasp *mcasp) return ret; }
+enum {
- MCASP_EDMA = 1,
Why start from 1? I'm not sure about the name. It somehow implies that McASP has something to do with the dma engine. MCASP_SERVICED_BY_E/SDMA PLATFORM_USES_E/SDMA
I can not come up with a decent name :(
- MCASP_SDMA,
+}; +static const char *sdma_prefix = "ti,omap";
+static int davinci_mcasp_get_dma_type(struct davinci_mcasp *mcasp) +{
- struct dma_chan *chan;
- const char *tmp;
- int ret = MCASP_EDMA;
- tmp = mcasp->dma_data[SNDRV_PCM_STREAM_PLAYBACK].filter_data;
- chan = dma_request_slave_channel_reason(mcasp->dev, tmp);
this will return with error in case of !dev->of_node. The first check should be: if (!mcasp->dev->of_node) return MCASP_EDMA;
since we can only boot in legacy mode with daVinci devices and they are using eDMA.
Yep, I'll add that back. I tought that I would not need it, but am not so familiar with the generic dmaengine pcm...
- if (IS_ERR(chan)) {
if (PTR_ERR(chan) != -EPROBE_DEFER)dev_err(mcasp->dev,"Can't verify DMA configuration (%ld)\n",PTR_ERR(chan));return PTR_ERR(chan);- }
- BUG_ON(!chan->device || !chan->device->dev);
- if (chan->device->dev->of_node)
ret = of_property_read_string(chan->device->dev->of_node,"compatible", &tmp);- else
dev_dbg(mcasp->dev, "DMA controller has no of-node\n");Now this is not likely to happen...
- dma_release_channel(chan);
- if (ret)
return ret;- dev_dbg(mcasp->dev, "DMA controller compatible = "%s"\n", tmp);
- if (!strncmp(tmp, sdma_prefix, strlen(sdma_prefix)))
return MCASP_SDMA;- return MCASP_EDMA;
+}
- static int davinci_mcasp_probe(struct platform_device *pdev) { struct snd_dmaengine_dai_dma_data *dma_data;
@@ -1763,27 +1803,28 @@ static int davinci_mcasp_probe(struct platform_device *pdev) if (ret != 0) goto err;
- switch (mcasp->version) {
- ret = -EINVAL;
Why? What was the problem with setting the ret in the default case?
I would need to have that I three places now for the #else cases, when devicetree tries to configure pcm driver that has not been compiled.
- switch (davinci_mcasp_get_dma_type(mcasp)) {
- case MCASP_EDMA: #if IS_BUILTIN(CONFIG_SND_EDMA_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_EDMA_SOC))
- case MCASP_VERSION_1:
- case MCASP_VERSION_2:
- case MCASP_VERSION_3: ret = edma_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_EDMA_SOC\n");break;- case MCASP_SDMA: #if IS_BUILTIN(CONFIG_SND_OMAP_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_OMAP_SOC))
- case MCASP_VERSION_4: ret = omap_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_SDMA_SOC\n");break;
dev_err(&pdev->dev, "Invalid McASP version: %d\n",mcasp->version);ret = -EINVAL;
dev_err(&pdev->dev, "No DMA controller found\n");
On 06/02/2015 02:11 PM, Jyri Sarha wrote:
- switch (mcasp->version) {
- ret = -EINVAL;
Why? What was the problem with setting the ret in the default case?
I would need to have that I three places now for the #else cases, when devicetree tries to configure pcm driver that has not been compiled.
I see now, thanks for pointing out ;)
- switch (davinci_mcasp_get_dma_type(mcasp)) {
- case MCASP_EDMA: #if IS_BUILTIN(CONFIG_SND_EDMA_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_EDMA_SOC))
- case MCASP_VERSION_1:
- case MCASP_VERSION_2:
- case MCASP_VERSION_3: ret = edma_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_EDMA_SOC\n");break;- case MCASP_SDMA: #if IS_BUILTIN(CONFIG_SND_OMAP_SOC) || \ (IS_MODULE(CONFIG_SND_DAVINCI_SOC_MCASP) && \ IS_MODULE(CONFIG_SND_OMAP_SOC))
- case MCASP_VERSION_4: ret = omap_pcm_platform_register(&pdev->dev);
break;+#else
dev_err(&pdev->dev, "Missing SND_SDMA_SOC\n");break; default:
dev_err(&pdev->dev, "Invalid McASP version: %d\n",mcasp->version);ret = -EINVAL;
dev_err(&pdev->dev, "No DMA controller found\n"); break; }
participants (3)
-
Jyri Sarha -
Mark Brown -
Peter Ujfalusi