Re: [alsa-devel] [PATCH 07/22] ASoC: Ux500: Initialise PCM from MSP probe rather than as a device
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.
participants (2)
-
Lee Jones -
Mark Brown