[alsa-devel] [PATCH 07/22] ASoC: Ux500: Initialise PCM from MSP probe rather than as a device
The PCM is a pseudo-device. It doesn't have any of it's own registers or hardware. It rather acts as a layer of abstraction for DMA transfers. Hence, instead of classifying it as a device in its own right, we call the initialisation from the MSP driver.
CC: alsa-devel@alsa-project.org Signed-off-by: Lee Jones lee.jones@linaro.org --- sound/soc/ux500/mop500.c | 4 ++-- sound/soc/ux500/ux500_msp_dai.c | 14 +++++++++++++- sound/soc/ux500/ux500_pcm.c | 19 ++++--------------- sound/soc/ux500/ux500_pcm.h | 3 +++ 4 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/sound/soc/ux500/mop500.c b/sound/soc/ux500/mop500.c index 6840df7..fe6223a 100644 --- a/sound/soc/ux500/mop500.c +++ b/sound/soc/ux500/mop500.c @@ -33,7 +33,7 @@ struct snd_soc_dai_link mop500_dai_links[] = { .stream_name = "ab8500_0", .cpu_dai_name = "ux500-msp-i2s.1", .codec_dai_name = "ab8500-codec-dai.0", - .platform_name = "ux500-pcm.0", + .platform_name = "ux500-msp-i2s.1", .codec_name = "ab8500-codec.0", .init = mop500_ab8500_machine_init, .ops = mop500_ab8500_ops, @@ -43,7 +43,7 @@ struct snd_soc_dai_link mop500_dai_links[] = { .stream_name = "ab8500_1", .cpu_dai_name = "ux500-msp-i2s.3", .codec_dai_name = "ab8500-codec-dai.1", - .platform_name = "ux500-pcm.0", + .platform_name = "ux500-msp-i2s.3", .codec_name = "ab8500-codec.0", .init = NULL, .ops = mop500_ab8500_ops, diff --git a/sound/soc/ux500/ux500_msp_dai.c b/sound/soc/ux500/ux500_msp_dai.c index 0f7dd49..3476d6a 100644 --- a/sound/soc/ux500/ux500_msp_dai.c +++ b/sound/soc/ux500/ux500_msp_dai.c @@ -28,6 +28,7 @@
#include "ux500_msp_i2s.h" #include "ux500_msp_dai.h" +#include "ux500_pcm.h"
static int setup_pcm_multichan(struct snd_soc_dai *dai, struct ux500_msp_config *msp_config) @@ -806,11 +807,20 @@ static int __devinit ux500_msp_drv_probe(struct platform_device *pdev) goto err_init_msp; }
+ ret = ux500_pcm_register_platform(pdev); + if (ret < 0) { + dev_err(&pdev->dev, + "Error: %s: Failed to register PCM platform device!\n", + __func__); + goto err_reg_plat; + } + return 0;
+err_reg_plat: + snd_soc_unregister_dais(&pdev->dev, ARRAY_SIZE(ux500_msp_dai_drv)); err_init_msp: clk_put(drvdata->clk); - err_clk: devm_regulator_put(drvdata->reg_vape);
@@ -821,6 +831,8 @@ static int __devexit ux500_msp_drv_remove(struct platform_device *pdev) { struct ux500_msp_i2s_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+ ux500_pcm_unregister_platform(pdev); + snd_soc_unregister_dais(&pdev->dev, ARRAY_SIZE(ux500_msp_dai_drv));
devm_regulator_put(drvdata->reg_vape); diff --git a/sound/soc/ux500/ux500_pcm.c b/sound/soc/ux500/ux500_pcm.c index 1a04e24..894c9f4 100644 --- a/sound/soc/ux500/ux500_pcm.c +++ b/sound/soc/ux500/ux500_pcm.c @@ -282,7 +282,7 @@ static struct snd_soc_platform_driver ux500_pcm_soc_drv = { .pcm_new = ux500_pcm_new, };
-static int __devexit ux500_pcm_drv_probe(struct platform_device *pdev) +int __devinit ux500_pcm_register_platform(struct platform_device *pdev) { int ret;
@@ -296,23 +296,12 @@ static int __devexit ux500_pcm_drv_probe(struct platform_device *pdev)
return 0; } +EXPORT_SYMBOL_GPL(ux500_pcm_register_platform);
-static int __devinit ux500_pcm_drv_remove(struct platform_device *pdev) +int __devexit ux500_pcm_unregister_platform(struct platform_device *pdev) { snd_soc_unregister_platform(&pdev->dev);
return 0; } - -static struct platform_driver ux500_pcm_driver = { - .driver = { - .name = "ux500-pcm", - .owner = THIS_MODULE, - }, - - .probe = ux500_pcm_drv_probe, - .remove = __devexit_p(ux500_pcm_drv_remove), -}; -module_platform_driver(ux500_pcm_driver); - -MODULE_LICENSE("GPL v2"); +EXPORT_SYMBOL_GPL(ux500_pcm_unregister_platform); diff --git a/sound/soc/ux500/ux500_pcm.h b/sound/soc/ux500/ux500_pcm.h index 77ed44d..76d3444 100644 --- a/sound/soc/ux500/ux500_pcm.h +++ b/sound/soc/ux500/ux500_pcm.h @@ -32,4 +32,7 @@ #define UX500_PLATFORM_PERIODS_MAX 48 #define UX500_PLATFORM_BUFFER_BYTES_MAX (2048 * PAGE_SIZE)
+int ux500_pcm_register_platform(struct platform_device *pdev); +int ux500_pcm_unregister_platform(struct platform_device *pdev); + #endif
On Thu, Aug 9, 2012 at 5:47 PM, Lee Jones lee.jones@linaro.org wrote:
The PCM is a pseudo-device. It doesn't have any of it's own registers or hardware. It rather acts as a layer of abstraction for DMA transfers. Hence, instead of classifying it as a device in its own right, we call the initialisation from the MSP driver.
CC: alsa-devel@alsa-project.org Signed-off-by: Lee Jones lee.jones@linaro.org
I'd prefer to have Ola LILJA review this patch, but it looks alright to me. Acked-by: Linus Walleij linus.walleij@linaro.org
Yours, Linus Walleij
Hi Ola,
As requested:
Author: Lee Jones lee.jones@linaro.org Date: Tue Jul 31 14:06:19 2012 +0100
ASoC: Ux500: Initialise PCM from MSP probe rather than as a device
The PCM is a pseudo-device. It doesn't have any of it's own registers or hardware. It rather acts as a layer of abstraction for DMA transfers. Hence, instead of classifying it as a device in its own right, we call the initialisation from the MSP driver.
Signed-off-by: Lee Jones lee.jones@linaro.org
diff --git a/sound/soc/ux500/mop500.c b/sound/soc/ux500/mop500.c index 31c4d26..9707f4a 100644 --- a/sound/soc/ux500/mop500.c +++ b/sound/soc/ux500/mop500.c @@ -32,7 +32,7 @@ struct snd_soc_dai_link mop500_dai_links[] = { .stream_name = "ab8500_0", .cpu_dai_name = "ux500-msp-i2s.1", .codec_dai_name = "ab8500-codec-dai.0", - .platform_name = "ux500-pcm.0", + .platform_name = "ux500-msp-i2s.1", .codec_name = "ab8500-codec.0", .init = mop500_ab8500_machine_init, .ops = mop500_ab8500_ops, @@ -42,7 +42,7 @@ struct snd_soc_dai_link mop500_dai_links[] = { .stream_name = "ab8500_1", .cpu_dai_name = "ux500-msp-i2s.3", .codec_dai_name = "ab8500-codec-dai.1", - .platform_name = "ux500-pcm.0", + .platform_name = "ux500-msp-i2s.3", .codec_name = "ab8500-codec.0", .init = NULL, .ops = mop500_ab8500_ops, diff --git a/sound/soc/ux500/ux500_msp_dai.c b/sound/soc/ux500/ux500_msp_dai.c index 772cb19..6afab6c 100644 --- a/sound/soc/ux500/ux500_msp_dai.c +++ b/sound/soc/ux500/ux500_msp_dai.c @@ -28,6 +28,7 @@
#include "ux500_msp_i2s.h" #include "ux500_msp_dai.h" +#include "ux500_pcm.h"
static int setup_pcm_multichan(struct snd_soc_dai *dai, struct ux500_msp_config *msp_config) @@ -806,11 +807,20 @@ static int __devinit ux500_msp_drv_probe(struct platform_device *pdev) goto err_init_msp; }
+ ret = ux500_pcm_register_platform(pdev); + if (ret < 0) { + dev_err(&pdev->dev, + "Error: %s: Failed to register PCM platform device!\n", + __func__); + goto err_reg_plat; + } + return 0;
+err_reg_plat: + snd_soc_unregister_dais(&pdev->dev, ARRAY_SIZE(ux500_msp_dai_drv)); err_init_msp: clk_put(drvdata->clk); - err_clk: devm_regulator_put(drvdata->reg_vape);
@@ -821,6 +831,8 @@ static int __devexit ux500_msp_drv_remove(struct platform_device *pdev) { struct ux500_msp_i2s_drvdata *drvdata = dev_get_drvdata(&pdev->dev);
+ ux500_pcm_unregister_platform(pdev); + snd_soc_unregister_dais(&pdev->dev, ARRAY_SIZE(ux500_msp_dai_drv));
devm_regulator_put(drvdata->reg_vape); diff --git a/sound/soc/ux500/ux500_pcm.c b/sound/soc/ux500/ux500_pcm.c index 1a04e24..894c9f4 100644 --- a/sound/soc/ux500/ux500_pcm.c +++ b/sound/soc/ux500/ux500_pcm.c @@ -282,7 +282,7 @@ static struct snd_soc_platform_driver ux500_pcm_soc_drv = { .pcm_new = ux500_pcm_new, };
-static int __devexit ux500_pcm_drv_probe(struct platform_device *pdev) +int __devinit ux500_pcm_register_platform(struct platform_device *pdev) { int ret;
@@ -296,23 +296,12 @@ static int __devexit ux500_pcm_drv_probe(struct platform_device *pdev)
return 0; } +EXPORT_SYMBOL_GPL(ux500_pcm_register_platform);
-static int __devinit ux500_pcm_drv_remove(struct platform_device *pdev) +int __devexit ux500_pcm_unregister_platform(struct platform_device *pdev) { snd_soc_unregister_platform(&pdev->dev);
return 0; } - -static struct platform_driver ux500_pcm_driver = { - .driver = { - .name = "ux500-pcm", - .owner = THIS_MODULE, - }, - - .probe = ux500_pcm_drv_probe, - .remove = __devexit_p(ux500_pcm_drv_remove), -}; -module_platform_driver(ux500_pcm_driver); - -MODULE_LICENSE("GPL v2"); +EXPORT_SYMBOL_GPL(ux500_pcm_unregister_platform); diff --git a/sound/soc/ux500/ux500_pcm.h b/sound/soc/ux500/ux500_pcm.h index 77ed44d..76d3444 100644 --- a/sound/soc/ux500/ux500_pcm.h +++ b/sound/soc/ux500/ux500_pcm.h @@ -32,4 +32,7 @@ #define UX500_PLATFORM_PERIODS_MAX 48 #define UX500_PLATFORM_BUFFER_BYTES_MAX (2048 * PAGE_SIZE)
+int ux500_pcm_register_platform(struct platform_device *pdev); +int ux500_pcm_unregister_platform(struct platform_device *pdev); + #endif
Hi Ola,
As requested:
Author: Lee Jones lee.jones@linaro.org Date: Tue Jul 31 14:06:19 2012 +0100
ASoC: Ux500: Initialise PCM from MSP probe rather than as a device The PCM is a pseudo-device. It doesn't have any of it's own registers or hardware. It rather acts as a layer of abstraction for DMA transfers. Hence, instead of classifying it as a device in its own right, we call the initialisation from the MSP driver. Signed-off-by: Lee Jones <lee.jones@linaro.org>diff --git a/sound/soc/ux500/mop500.c b/sound/soc/ux500/mop500.c index 31c4d26..9707f4a 100644 --- a/sound/soc/ux500/mop500.c +++ b/sound/soc/ux500/mop500.c @@ -32,7 +32,7 @@ struct snd_soc_dai_link mop500_dai_links[] = { .stream_name = "ab8500_0", .cpu_dai_name = "ux500-msp-i2s.1", .codec_dai_name = "ab8500-codec-dai.0",
.platform_name = "ux500-pcm.0",
.platform_name = "ux500-msp-i2s.1",@@ -42,7 +42,7 @@ struct snd_soc_dai_link mop500_dai_links[] = { .stream_name = "ab8500_1", .cpu_dai_name = "ux500-msp-i2s.3", .codec_dai_name = "ab8500-codec-dai.1",
.platform_name = "ux500-pcm.0",
.platform_name = "ux500-msp-i2s.3",diff --git a/sound/soc/ux500/ux500_msp_dai.c b/sound/soc/ux500/ux500_msp_dai.c
Lee Jones - Sept. 19, 2012, 12:29 p.m. On Thu, Aug 23, 2012 at 01:59:04PM +0100, Mark Brown wrote:
On Thu, Aug 23, 2012 at 01:20:03PM +0100, Lee Jones wrote:
I say I don't understand the motivation for this change. All the modern DT bindings are perfectly happy handling this without an explicit shim in the device tree to bodge things for Linux, adding them in seems like it'd be a retrograde step. What benefit do you believe this brings?
How do the all the other DT:ed audio drivers handle the PCM then? More importantly, how would you like to see it handled? Ola has NACKed this patch and explained why:
They instantiate the PCM driver dynamically from the DAI when it's probed which is pretty much what you're patch is doing.
Can we have some closure on this patch please, as it's blocking the patch-set? I'm fairly sure the patch is doing the correct thing, as seconded by Mark.
I still don't like this. It is the dai_link-struct that bothers me. We have "ux500-msp-i2s.1" as name of the platform AND the cpu_dai. The MSP I2S-block is not the platform and it is certainly not both platform and cpu-DAI at the same time. Mark: Did you have a solution for this? Couldn't we just put NULL on the platform_name instead?
On Thu, Sep 20, 2012 at 11:03:34AM +0200, Ola Lilja wrote:
I still don't like this. It is the dai_link-struct that bothers me. We have "ux500-msp-i2s.1" as name of the platform AND the cpu_dai. The MSP I2S-block is not the platform and it is certainly not both platform and cpu-DAI at the same time.
Mark: Did you have a solution for this? Couldn't we just put NULL on the platform_name instead?
No, having no platform driver means a direct device<->device link (like baseband<->CODEC). You could always register the platform with a -dma added in the name or something, it'd be a bit more code but it'd work, but the framework doesn't particularly care what you call these things.
Can we have some closure on this patch please, as it's blocking the patch-set? I'm fairly sure the patch is doing the correct thing, as seconded by Mark.
I still don't like this. It is the dai_link-struct that bothers me. We have "ux500-msp-i2s.1" as name of the platform AND the cpu_dai. The MSP I2S-block is not the platform and it is certainly not both platform and cpu-DAI at the same time. Mark: Did you have a solution for this? Couldn't we just put NULL on the platform_name instead?
There are other drivers which do this already.
I don't think it's an issue to do this.
participants (4)
-
Lee Jones -
Linus Walleij -
Mark Brown -
Ola Lilja