| Message ID | 1344527268-5964-8-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Mark, > > > I'm sorry but this patch is breaking the design of ASoC. > > > The ASoC-platform is the DMA-block (in combination with the > > > MSP-block), and there should be a platform-driver for the DMA/PCM. > > > The > > > platform-driver then has a DAI which is the MSP. The ASoC > > > DAI-link-struct should have one driver for each of these, so the > > > dummy-driver for PCM should be there. > > > > Just thinking about this now. I converted it to the current format at > > the request of Mark. If this isn't the correct method I'm not quite > > sure what is. If you want it to be registered as a device, then it > > needs to go into the Device Tree, but Mark doesn't want it in there > > because it doesn't actually represent hardware. I've just taken a closer look at this with a view to finding the most suitable solution. My conclusion is that although the PCM doesn't contain any registers, or represent hardware it should be a device and therefore be present in the Device Tree. These are my findings: PCM devices already represented in DTs: fsl,mpc5200-pcm - written by Grant Likely, the author of Device Tree phytec,pcm030 - written by Grant Likely, the author of Device Tree PCM devices which register as actual devices (should be in DT): samsung - samsung-pcm sh - siu-pcm-audio omap - omap-pcm-audio pxa - pxa-pcm-audio jz4740 - jz4740-pcm-audio kirkwood - kirkwood-pcm-audio ep93xx - ep93xx-pcm-audio ... The later was basically every PCM device bar one I think. The Open Firmware used to stipulate that each device represented in the Device Tree to own registers and therefore be an actual hardware device, but that has since been lifted as it didn't make sense. I propose to represent the PCM in the Device Tree again and have it probe just like all the other PCM devices in sound/soc. If you give me the nod, I'll revert this patch, enable the PCM for DT again and resent the patch-set in full.
On Thu, Aug 23, 2012 at 10:22:17AM +0100, Lee Jones wrote: > > > Just thinking about this now. I converted it to the current format at > > > the request of Mark. If this isn't the correct method I'm not quite > > > sure what is. If you want it to be registered as a device, then it > > > needs to go into the Device Tree, but Mark doesn't want it in there > > > because it doesn't actually represent hardware. > I've just taken a closer look at this with a view to finding the most > suitable solution. My conclusion is that although the PCM doesn't > contain any registers, or represent hardware it should be a device and > therefore be present in the Device Tree. Your assumption that being a device in Linux means that something should appear in the device tree definitely doesn't follow. > PCM devices already represented in DTs: > fsl,mpc5200-pcm - written by Grant Likely, the author of Device Tree > phytec,pcm030 - written by Grant Likely, the author of Device Tree I suspect Mitch might have a word or two to say about the above... in any case, these are *really* old PowerPC bindings which means they're not always a good model. Though in this case if you look at the code you'll also see that the driver is actually directly managing hardware with register I/O rather than just purely proxying an underlying DMA API. Clearly if there's actual hardware control involved a device is appropriate. > PCM devices which register as actual devices (should be in DT): These are all Linux platform devices, things that are OK for purely internal Linux usage which we can rewrite at will aren't always appropriate for a cross platform DT. > I propose to represent the PCM in the Device Tree again and have it > probe just like all the other PCM devices in sound/soc. > If you give me the nod, I'll revert this patch, enable the PCM for DT > again and resent the patch-set in full. 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?
> 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: "I'm sorry but this patch is breaking the design of ASoC. The ASoC- platform is the DMA-block (in combination with the MSP-block), and there should be a platform-driver for the DMA/PCM. The platform-driver then has a DAI which is the MSP. The ASoC DAI-link-struct should have one driver for each of these, so the dummy-driver for PCM should be there." So I don't really know where to go with it. Any ideas?
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. > "I'm sorry but this patch is breaking the design of ASoC. The ASoC- > platform is the DMA-block (in combination with the MSP-block), and > there should be a platform-driver for the DMA/PCM. The platform-driver > then has a DAI which is the MSP. The ASoC DAI-link-struct should have > one driver for each of these, so the dummy-driver for PCM should be > there." > So I don't really know where to go with it. Any ideas? I think Ola is suggesting probing the DMA driver from the machine which will also work though I'm not 100% sure if I'm parsing the above correctly. The issue in DT terms is that if the DMA controller is shared with a bunch of other IPs then it should have one node shared between them all and not a bunch of shim nodes inserted in the middle which only exists due to the way Linux instantiates stuff.
> > "I'm sorry but this patch is breaking the design of ASoC. The ASoC- > > platform is the DMA-block (in combination with the MSP-block), and > > there should be a platform-driver for the DMA/PCM. The platform-driver > > then has a DAI which is the MSP. The ASoC DAI-link-struct should have > > one driver for each of these, so the dummy-driver for PCM should be > > there." > > > So I don't really know where to go with it. Any ideas? > > I think Ola is suggesting probing the DMA driver from the machine which > will also work though I'm not 100% sure if I'm parsing the above > correctly. The issue in DT terms is that if the DMA controller is > shared with a bunch of other IPs then it should have one node shared > between them all and not a bunch of shim nodes inserted in the middle > which only exists due to the way Linux instantiates stuff. When you say 'machine', do you mean from arch/<arch>/mach-*? If so, I'm keen for that not to happen. > > 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. So they do it in the same why I have with this patch? Do you know why Ola might think this is a bad idea?
On Thu, Aug 23, 2012 at 02:26:19PM +0100, Lee Jones wrote: > > I think Ola is suggesting probing the DMA driver from the machine which > > will also work though I'm not 100% sure if I'm parsing the above > > correctly. The issue in DT terms is that if the DMA controller is > > shared with a bunch of other IPs then it should have one node shared > > between them all and not a bunch of shim nodes inserted in the middle > > which only exists due to the way Linux instantiates stuff. > When you say 'machine', do you mean from arch/<arch>/mach-*? If so, I'm > keen for that not to happen. No, sound/soc/ux500/snowball.c or whatever. At least that's my guess. > > They instantiate the PCM driver dynamically from the DAI when it's > > probed which is pretty much what you're patch is doing. > So they do it in the same why I have with this patch? Do you know why > Ola might think this is a bad idea? I'm not 100% sure, I'm guessing it might be down to the fact that you end up with multiple PCM drivers. We could avoid that with refcounting but nobody's really worried about it.
On Thu, Aug 23, 2012 at 03:37:57PM +0100, Mark Brown wrote: > On Thu, Aug 23, 2012 at 02:26:19PM +0100, Lee Jones wrote: > > > > I think Ola is suggesting probing the DMA driver from the machine which > > > will also work though I'm not 100% sure if I'm parsing the above > > > correctly. The issue in DT terms is that if the DMA controller is > > > shared with a bunch of other IPs then it should have one node shared > > > between them all and not a bunch of shim nodes inserted in the middle > > > which only exists due to the way Linux instantiates stuff. > > > When you say 'machine', do you mean from arch/<arch>/mach-*? If so, I'm > > keen for that not to happen. > > No, sound/soc/ux500/snowball.c or whatever. At least that's my guess. Ah, I see. Maybe the mop500.c file then. > > > They instantiate the PCM driver dynamically from the DAI when it's > > > probed which is pretty much what you're patch is doing. > > > So they do it in the same why I have with this patch? Do you know why > > Ola might think this is a bad idea? > > I'm not 100% sure, I'm guessing it might be down to the fact that you > end up with multiple PCM drivers. We could avoid that with refcounting > but nobody's really worried about it. I think I'll wait for Ola to get back, as he's the expert on this stuff. I'll attempt to re-jig the patch-set, as this is a blocker atm.
On Thu, Aug 23, 2012 at 03:59:06PM +0100, Lee Jones wrote:
> I'll attempt to re-jig the patch-set, as this is a blocker atm.
OK, if we could get at least the DAI driver and arch/arm changes in
that'd at least make the series smaller.
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.
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
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(-)