| Message ID | 20130831210518.GK6617@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 31, 2013 at 10:05:18PM +0100, Russell King - ARM Linux wrote: > On Sat, Aug 31, 2013 at 10:46:35PM +0200, Lars-Peter Clausen wrote: > > On 08/31/2013 09:19 PM, Russell King - ARM Linux wrote: > > > On Sat, Aug 31, 2013 at 06:28:53PM +0100, Mark Brown wrote: > > >> On Sat, Aug 31, 2013 at 05:28:26PM +0200, Lars-Peter Clausen wrote: > > >> > > >>> Then connect the SPDIF AIF widgets to the SPDIF streams and the I2S AIF > > >>> widgets to the I2S streams. In your machine driver setup the link config to > > >>> use DAI 0 if the board uses I2S and DAI 1 if the board uses SPDIF. The ASoC > > >>> framework will then create the necessary routes all by itself. Of course > > >>> this won't work on a platform where both the SPDIF and I2S controller are > > >>> used at the same time, but neither does your current solution. > > >> > > >> This is the either/or approach I've been suggesting. > > > > > > Ah, here we go again. This is precisely why I can't work with you. > > > Nothing I say seems to stick in your mind. I've been telling you for the > > > last four weeks that it doesn't work - and I've shown you the oopses and > > > warnings I get from trying it. Your response to date: nothing of any > > > real use. > > > > > > If you're not going to provide any useful responses on bug reports, it is > > > totally unreasonable that you even suggest that _that_ is how it should > > > be done when I've already proved that it's impossible at present. > > > > Russell, please stop being such a bitch. You seem to be the only one who has > > any problems working with Mark, which maybe should make you wonder if the > > problem is not on your side. Insulting and screaming at people all the time > > won't exactly motivate them to help you with your problems. Please try to be > > constructive, it will makes things much easier for everyone. > > Consider that Mark's been saying the same old stuff for the last four > weeks, and I've been showing that it doesn't work, and the only thing > that Mark says is the same old same old - maybe in the hope that if he > says it enough times the bugs will mysteriously vanish. Unfortunately, > that isn't happening. > > It's also not constructive or helpful, and it's extremely frustrating. > I have very little patience left with ASoC at this point, having spent > a month trying to get this stuff working. > > I'm not the only one: I've heard other people complain about the _exact_ > same issue with ASoC: ASoC is difficult to work with, and many people > just seem to give up with it - or keep their stuff in their own trees > locally. I can very well see why that happens. > > As for "This is the either/or approach I've been suggesting." No, that's > another false statement from Mark. What Mark has been suggesting all > along is to use DPCM - and that's something which I tried for ages to > get working, and it just doesn't work (as evidenced by the oopses and > warnings that get spat out of the kernel.) > > Mark suggested it _before_ he suggested DPCM, and I tried that approach. > The problem is that it doesn't fit the hardware. They aren't two > separate streams - they're a single stream with two output paths, and > there's restrictions in the hardware to do with how they're controlled. > > There are two choices in how the hardware is used: either one output > only is used, or if both outputs are used, they must be used "as one" - > both outputs must be simultaneously enabled and disabled. As far as > I know, that's not possible with two DAIs. > > And here's the patch I tried. > > See - I've been down this route before. I've tried it, and I know > what it's problems are. I'm not making up _anything_ here - I really > have tried all these "suggestions" and I'm just going round in circles > because Mark isn't listening to what I've been reporting back. And this is how I ended up going down the DPCM route - this is from private mails, so I'm only including those bits which I authored. This is from an email exchange back in May (yes, this has been going on since May!) > > > So, I have this Armada 510 on the Cubox, which is wired up to use mainly > > > the SPDIF output from the device. > > > > > The structure of the device (if you look at the page I point out in the > > > PDF below) is: > > > > > Tx DMA --> Tx FIFO ----> I2S formatter ----> I2S output > > > `--> SPDIF formatter --> SPDIF output > > > > > There are separate mute and enable bits for the I2S and SPDIF. The mute > > > bits can be set and cleared independently at any time (but it's > > > recommended > > > to set the mute bits before pausing the entire block). However, the > > > enable bits must be set and cleared simultaneously if both SPDIF and I2S > > > output are required - setting one then the other is not permitted by > > > the spec. > > > > > Now, Mark describes below about using two front ends and a single back > > > end to describe this. So, I've created two DAIs in kirkwood-i2s.c, > > > one for I2S and one for SPDIF. I've since come to the conclusion that > > > this is wrong. I'm now wondering how all this front end/back end stuff > > > hangs together, and so forth. (Liam asks why) > It's wrong because there's no way to tie the two DAIs together and tell > ASoC that the second DAI must not be started if the first one is already > running. > > You can a card module come along, bind to the SPDIF one, then have the > SPDIF output be used (so the transmission starts), then the new card > module binds separately to the I2S one, at which point that automatically > starts up - which then violates the conditions layed down that "both > shall be started together or just one". > > There is no way in the CPU DAI backend to tell ALSA "these two DAIs are > inherently linked and must be either started together or only one." (Liam includes his relevant parts of a driver he is working on using DPCM.) So, it seems that you and Mark disagree with Liam. That's just great. I'm getting really tired of being told "you should do it this way" by various people where their way is different. I'm just going round and round in circles with this.
On Sat, Aug 31, 2013 at 10:05:18PM +0100, Russell King - ARM Linux wrote: > I'm not the only one: I've heard other people complain about the _exact_ > same issue with ASoC: ASoC is difficult to work with, and many people > just seem to give up with it - or keep their stuff in their own trees > locally. I can very well see why that happens. We do appear to have a fairly good success rate with systems working with mainline; I can only think of one driver that didn't make it in and that was one where we had a bit of an issue getting the code to visually resemble Linux code so I don't feel too bad about it. I am aware of some people who didn't work upstream, we've generally had some useful discussions with them once we've found each other. > As for "This is the either/or approach I've been suggesting." No, that's > another false statement from Mark. What Mark has been suggesting all > along is to use DPCM - and that's something which I tried for ages to > get working, and it just doesn't work (as evidenced by the oopses and > warnings that get spat out of the kernel.) While it is correct that I have been saying the final result should use DPCM (since this is what the hardware looks like) you will recall that I did recently suggest either/or as a stepping stone towards a full implementation, allowing systems with only S/PDIF to be supported while the other issues are still being worked on. > There are two choices in how the hardware is used: either one output > only is used, or if both outputs are used, they must be used "as one" - > both outputs must be simultaneously enabled and disabled. As far as > I know, that's not possible with two DAIs. That is correct, this is why I call it an either/or approach - a system would be able to use either I2S or S/PDIF but not both. For systems with both I2S and S/PDIF connected one or the other would have to be chosen by the machine driver. > And here's the patch I tried. Ah, I'm not sure that I have seen this before (it's certainly not been submitted). Just looking at the diff this all seems fine for an either/or approach though... > index 2c459c1..62e5b62 100644 > --- a/sound/soc/kirkwood/kirkwood-spdif.c > +++ b/sound/soc/kirkwood/kirkwood-spdif.c > @@ -61,7 +61,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { > .name = "S/PDIF0", > .stream_name = "S/PDIF0 PCM Playback", > .platform_name = "kirkwood-pcm-audio.0", > - .cpu_dai_name = "kirkwood-i2s.0", > + .cpu_dai_name = "kirkwood-spdif.0", > .codec_dai_name = "dit-hifi", > .codec_name = "spdif-dit", > .ops = &kirkwood_spdif_ops, > @@ -73,7 +73,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai1[] = { > .name = "S/PDIF1", > .stream_name = "IEC958 Playback", > .platform_name = "kirkwood-pcm-audio.1", > - .cpu_dai_name = "kirkwood-i2s.1", > + .cpu_dai_name = "kirkwood-spdif.1", > .codec_dai_name = "dit-hifi", > .codec_name = "spdif-dit", > .ops = &kirkwood_spdif_ops, ...this file does not appear to be in mainline and some of the rest of the context suggested this was based off something old.
On Sun, Sep 01, 2013 at 01:19:28PM +0100, Mark Brown wrote: > On Sat, Aug 31, 2013 at 10:05:18PM +0100, Russell King - ARM Linux wrote: > > > I'm not the only one: I've heard other people complain about the _exact_ > > same issue with ASoC: ASoC is difficult to work with, and many people > > just seem to give up with it - or keep their stuff in their own trees > > locally. I can very well see why that happens. > > We do appear to have a fairly good success rate with systems working > with mainline; I can only think of one driver that didn't make it in and > that was one where we had a bit of an issue getting the code to visually > resemble Linux code so I don't feel too bad about it. I am aware of > some people who didn't work upstream, we've generally had some useful > discussions with them once we've found each other. I wonder whether you include my ASoC sa11x0 assabet driver in that, which I gave up trying to get into mainline, because it didn't fit ASoC with its requirement to have the SSP transmit DMA running in order to capture, and didn't use the latest soc-dmaengine stuff. That driver remains in my tree, and is continually forward-ported, and built in my nightly ARM builds. > > As for "This is the either/or approach I've been suggesting." No, that's > > another false statement from Mark. What Mark has been suggesting all > > along is to use DPCM - and that's something which I tried for ages to > > get working, and it just doesn't work (as evidenced by the oopses and > > warnings that get spat out of the kernel.) > > While it is correct that I have been saying the final result should use > DPCM (since this is what the hardware looks like) you will recall that I > did recently suggest either/or as a stepping stone towards a full > implementation, allowing systems with only S/PDIF to be supported while > the other issues are still being worked on. Yes, and when I asked for details how you'd like that done, you conveniently decided that you would not reply to that. > > There are two choices in how the hardware is used: either one output > > only is used, or if both outputs are used, they must be used "as one" - > > both outputs must be simultaneously enabled and disabled. As far as > > I know, that's not possible with two DAIs. > > That is correct, this is why I call it an either/or approach - a system > would be able to use either I2S or S/PDIF but not both. For systems > with both I2S and S/PDIF connected one or the other would have to be > chosen by the machine driver. > > > And here's the patch I tried. > > Ah, I'm not sure that I have seen this before (it's certainly not been > submitted). Just looking at the diff this all seems fine for an > either/or approach though... You wouldn't have seen this exact patch before, because it's a delta between _this_ series and the one which I just built using the patch from back in May 2013. However, I believe you have seen the May 2013 patch at some point along the way. > > index 2c459c1..62e5b62 100644 > > --- a/sound/soc/kirkwood/kirkwood-spdif.c > > +++ b/sound/soc/kirkwood/kirkwood-spdif.c > > @@ -61,7 +61,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { > > .name = "S/PDIF0", > > .stream_name = "S/PDIF0 PCM Playback", > > .platform_name = "kirkwood-pcm-audio.0", > > - .cpu_dai_name = "kirkwood-i2s.0", > > + .cpu_dai_name = "kirkwood-spdif.0", > > .codec_dai_name = "dit-hifi", > > .codec_name = "spdif-dit", > > .ops = &kirkwood_spdif_ops, > > @@ -73,7 +73,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai1[] = { > > .name = "S/PDIF1", > > .stream_name = "IEC958 Playback", > > .platform_name = "kirkwood-pcm-audio.1", > > - .cpu_dai_name = "kirkwood-i2s.1", > > + .cpu_dai_name = "kirkwood-spdif.1", > > .codec_dai_name = "dit-hifi", > > .codec_name = "spdif-dit", > > .ops = &kirkwood_spdif_ops, > > ...this file does not appear to be in mainline and some of the rest of > the context suggested this was based off something old. Correct - and I have no intention of submitting this file, because it presupposes mainline non-DT support for Dove. Non-DT support for Dove is being removed from mainline at present. The mainline SIS5531 clock driver which provides the external clock for this on the Cubox does not support non-DT. The non-DT support I run has full support for everything on Dove, which means I can do much more in-depth testing than just running madplay or something similar to check that there's audio out - which is about the limit of what can be done with DT-only setups at the moment, such as the DPCM bug triggered by vlc. This is where those who are using mainline kernels with DT on Dove platforms (like Jean-Francois and Sebastian) need a working solution to this in a form which they can come up with a representative DT solution. This is not a one-person effort - there's multiple efforts working on this, and it's all inter-linked.
On Sun, Sep 01, 2013 at 01:34:33PM +0100, Russell King - ARM Linux wrote: > The non-DT support I run has full support for everything on Dove, which > means I can do much more in-depth testing than just running madplay or > something similar to check that there's audio out - which is about the > limit of what can be done with DT-only setups at the moment, such as > the DPCM bug triggered by vlc. Sorry, that was a bit unclear: I meant that I can do testing with vlc on my setup, which I don't believe to be possible yet on DT setups.
On Sun, Sep 01, 2013 at 01:34:33PM +0100, Russell King - ARM Linux wrote: > On Sun, Sep 01, 2013 at 01:19:28PM +0100, Mark Brown wrote: > > We do appear to have a fairly good success rate with systems working > > with mainline; I can only think of one driver that didn't make it in and > > that was one where we had a bit of an issue getting the code to visually > > resemble Linux code so I don't feel too bad about it. I am aware of > > some people who didn't work upstream, we've generally had some useful > > discussions with them once we've found each other. > I wonder whether you include my ASoC sa11x0 assabet driver in that, > which I gave up trying to get into mainline, because it didn't fit > ASoC with its requirement to have the SSP transmit DMA running in > order to capture, and didn't use the latest soc-dmaengine stuff. > That driver remains in my tree, and is continually forward-ported, > and built in my nightly ARM builds. To be honest not really, you've only mentioned it by providing links to the git repository (it's never been posted) and there has been no real discussion of the issues or other interest in getting the code integrated. For the capture DMA requirement you mention I'd expect a driver local fix should be OK so long as it's well abstracted - it's not likely to be a common issue. The approach I think I have suggested before is to get playback only support merged then worry about dealing with the fun stuff needed for capture support, that still seems like the most obvious way forwards if there is a desire to get the code merged. > > While it is correct that I have been saying the final result should use > > DPCM (since this is what the hardware looks like) you will recall that I > > did recently suggest either/or as a stepping stone towards a full > > implementation, allowing systems with only S/PDIF to be supported while > > the other issues are still being worked on. > Yes, and when I asked for details how you'd like that done, you > conveniently decided that you would not reply to that. Indeed. This was due to the way you asked: http://thread.gmane.org/gmane.linux.ports.arm.kernel/257542/focus=111909 which didn't indicate a great deal of interest in pursing this, suggesting that it wasn't worth the effort going into more detail. Something more like "that sounds like a useful idea, I've looked but I'm not quite sure how to implement it" would have done so. > This is where those who are using mainline kernels with DT on Dove > platforms (like Jean-Francois and Sebastian) need a working solution to > this in a form which they can come up with a representative DT solution. > This is not a one-person effort - there's multiple efforts working on > this, and it's all inter-linked. Yes, and this is one of the reasons for suggesting getting either/or support merged - it will help things like the binding definition progress (as well as being useful for any users with a S/PDIF only system).
On Mon, Sep 02, 2013 at 03:06:32PM +0100, Mark Brown wrote: > On Sun, Sep 01, 2013 at 01:34:33PM +0100, Russell King - ARM Linux wrote: > > This is where those who are using mainline kernels with DT on Dove > > platforms (like Jean-Francois and Sebastian) need a working solution to > > this in a form which they can come up with a representative DT solution. > > This is not a one-person effort - there's multiple efforts working on > > this, and it's all inter-linked. > > Yes, and this is one of the reasons for suggesting getting either/or > support merged - it will help things like the binding definition > progress (as well as being useful for any users with a S/PDIF only > system). Sorry, but I believe the exact opposite: 1. The DAI link binding created for a dual-DAI driver is completely different from the DAI link binding for a DPCM driver. The dual DAI link binding will have to be completely rewritten when the driver is converted to DPCM. 2. When the driver is converted to DPCM, it must use DPCM for everything, otherwise it has no way to know which of SPDIF or I2S to enable. The only way I know to work around that is to add additional routes to link up the AIF widgets, and that's the solution you're all telling me is not acceptable, as per the patch set at the start of this thread.
On Mon, Sep 02, 2013 at 03:16:31PM +0100, Russell King - ARM Linux wrote: > On Mon, Sep 02, 2013 at 03:06:32PM +0100, Mark Brown wrote: > > Yes, and this is one of the reasons for suggesting getting either/or > > support merged - it will help things like the binding definition > > progress (as well as being useful for any users with a S/PDIF only > > system). > Sorry, but I believe the exact opposite: > 1. The DAI link binding created for a dual-DAI driver is completely > different from the DAI link binding for a DPCM driver. The dual > DAI link binding will have to be completely rewritten when the > driver is converted to DPCM. That seems like overstating the difficulties. The updates for any new S/PDIF drivers will be pretty much the same as those for the existing I2S drivers and should just be mechanical changes, nothing too taxing. > 2. When the driver is converted to DPCM, it must use DPCM for > everything, otherwise it has no way to know which of SPDIF or I2S to > enable. The only way I know to work around that is to add additional > routes to link up the AIF widgets, and that's the solution you're all > telling me is not acceptable, as per the patch set at the start of > this thread. What we have been telling you is that if there is a DAI link present (there should be one for each physical DAI link) then this should be enough information for the framework to know that the two DAIs are linked and if any routes are needed in DAPM these should be added automatically in the same way that we add links for CODEC<->CODEC links at present. It's been said to you off-list that having the links manually added but marking them for removal when the framework figures out how to do that should be OK.
On Mon, Sep 02, 2013 at 05:27:29PM +0100, Mark Brown wrote: > On Mon, Sep 02, 2013 at 03:16:31PM +0100, Russell King - ARM Linux wrote: > > On Mon, Sep 02, 2013 at 03:06:32PM +0100, Mark Brown wrote: > > > > Yes, and this is one of the reasons for suggesting getting either/or > > > support merged - it will help things like the binding definition > > > progress (as well as being useful for any users with a S/PDIF only > > > system). > > > Sorry, but I believe the exact opposite: > > > 1. The DAI link binding created for a dual-DAI driver is completely > > different from the DAI link binding for a DPCM driver. The dual > > DAI link binding will have to be completely rewritten when the > > driver is converted to DPCM. > > That seems like overstating the difficulties. The updates for any new > S/PDIF drivers will be pretty much the same as those for the existing > I2S drivers and should just be mechanical changes, nothing too taxing. Sorry, you need to explain more. Here's the dual-DAI version: .name = "S/PDIF1", .stream_name = "IEC958 Playback", .platform_name = "mvebu-audio.1", .cpu_dai_name = "kirkwood-spdif.1", .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", Here's the DPCM version: { .name = "S/PDIF1", .stream_name = "Audio Playback", .platform_name = "mvebu-audio.1", .cpu_dai_name = "mvebu-audio.1", .codec_name = "snd-soc-dummy", .codec_dai_name = "snd-soc-dummy-dai", .dynamic = 1, }, { .name = "Codec", .stream_name = "IEC958 Playback", .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "snd-soc-dummy", .no_pcm = 1, .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", }, The above are two completely different beasts. Please explain how the DT representation for those DAI links can be the same. > > 2. When the driver is converted to DPCM, it must use DPCM for > > everything, otherwise it has no way to know which of SPDIF or I2S to > > enable. The only way I know to work around that is to add additional > > routes to link up the AIF widgets, and that's the solution you're all > > telling me is not acceptable, as per the patch set at the start of > > this thread. > > What we have been telling you is that if there is a DAI link present > (there should be one for each physical DAI link) then this should be > enough information for the framework to know that the two DAIs are > linked and if any routes are needed in DAPM these should be added > automatically in the same way that we add links for CODEC<->CODEC links > at present. > > It's been said to you off-list that having the links manually added > but marking them for removal when the framework figures out how to do > that should be OK. Sorry, I just don't get this. What you seem to be telling me here is to forget DPCM, and just go with the dual DAI solution. If I have separate CPU DAI links, then I can't do simultaneous operation, because both DAI links are entirely separate entities which are activated entirely separately. The only way I know how to handle this is with the patch set I've already posted. We've been through this before: this is also not how Liam's example DPCM driver works - please go back and look at those diagrams I drew you of how Liam's driver is setup. I've also said to you that from what I can see, the routes are still required for DPCM - those routes are in Liam's DPCM driver as well.
On Mon, Sep 02, 2013 at 05:59:33PM +0100, Russell King - ARM Linux wrote: > On Mon, Sep 02, 2013 at 05:27:29PM +0100, Mark Brown wrote: > > That seems like overstating the difficulties. The updates for any new > > S/PDIF drivers will be pretty much the same as those for the existing > > I2S drivers and should just be mechanical changes, nothing too taxing. > Sorry, you need to explain more. > Here's the dual-DAI version: > .name = "S/PDIF1", > .stream_name = "IEC958 Playback", > .platform_name = "mvebu-audio.1", > .cpu_dai_name = "kirkwood-spdif.1", > .codec_dai_name = "dit-hifi", > .codec_name = "spdif-dit", > Here's the DPCM version: > > { > .name = "S/PDIF1", > .stream_name = "Audio Playback", > .platform_name = "mvebu-audio.1", > .cpu_dai_name = "mvebu-audio.1", > .codec_name = "snd-soc-dummy", > .codec_dai_name = "snd-soc-dummy-dai", > .dynamic = 1, > }, { > .name = "Codec", > .stream_name = "IEC958 Playback", > .cpu_dai_name = "snd-soc-dummy-dai", > .platform_name = "snd-soc-dummy", > .no_pcm = 1, > .codec_dai_name = "dit-hifi", > .codec_name = "spdif-dit", > }, > The above are two completely different beasts. Please explain how the DT > representation for those DAI links can be the same. If the binding is representing the differences between those two in the DT then the DT binding is clearly not good since it is exposing Linux implementation details - all the binding needs to say is that there's something connected to the S/PDIF output of the audio controller. > > > 2. When the driver is converted to DPCM, it must use DPCM for > > > everything, otherwise it has no way to know which of SPDIF or I2S to > > > enable. The only way I know to work around that is to add additional > > > routes to link up the AIF widgets, and that's the solution you're all > > > telling me is not acceptable, as per the patch set at the start of > > > this thread. > > > > What we have been telling you is that if there is a DAI link present > > (there should be one for each physical DAI link) then this should be > > enough information for the framework to know that the two DAIs are > > linked and if any routes are needed in DAPM these should be added > > automatically in the same way that we add links for CODEC<->CODEC links > > at present. > > It's been said to you off-list that having the links manually added > > but marking them for removal when the framework figures out how to do > > that should be OK. > Sorry, I just don't get this. What you seem to be telling me here is to > forget DPCM, and just go with the dual DAI solution. No, it's not clear to me why you would think that. If we are talking about adding DAPM routes matching DAI links then we are talking about DPCM. To reiterate the dual DAI implementation would merely be a stepping stone to a DPCM based implementation. > If I have separate CPU DAI links, then I can't do simultaneous operation, > because both DAI links are entirely separate entities which are activated > entirely separately. The only way I know how to handle this is with the > patch set I've already posted. As you are aware DPCM supports multiple DAIs and a good DPCM implementation for this hardware will have one front end DAI for the DMA and two back end DAIs, one for the S/PDIF interface and one for the I2S interface. > We've been through this before: this is also not how Liam's example DPCM > driver works - please go back and look at those diagrams I drew you of > how Liam's driver is setup. I've also said to you that from what I can > see, the routes are still required for DPCM - those routes are in Liam's > DPCM driver as well. This is why in the above message I reminded you of the suggestion that until the framework does automatically figure out that those routes should be there the routes are manually added in the driver clearly marked so they can easily be removed when the framework does the right thing here.
On Mon, Sep 02, 2013 at 09:44:21PM +0100, Mark Brown wrote: > On Mon, Sep 02, 2013 at 05:59:33PM +0100, Russell King - ARM Linux wrote: > > On Mon, Sep 02, 2013 at 05:27:29PM +0100, Mark Brown wrote: > > > > That seems like overstating the difficulties. The updates for any new > > > S/PDIF drivers will be pretty much the same as those for the existing > > > I2S drivers and should just be mechanical changes, nothing too taxing. > > > Sorry, you need to explain more. > > > Here's the dual-DAI version: > > > .name = "S/PDIF1", > > .stream_name = "IEC958 Playback", > > .platform_name = "mvebu-audio.1", > > .cpu_dai_name = "kirkwood-spdif.1", > > .codec_dai_name = "dit-hifi", > > .codec_name = "spdif-dit", > > > Here's the DPCM version: > > > > { > > .name = "S/PDIF1", > > .stream_name = "Audio Playback", > > .platform_name = "mvebu-audio.1", > > .cpu_dai_name = "mvebu-audio.1", > > .codec_name = "snd-soc-dummy", > > .codec_dai_name = "snd-soc-dummy-dai", > > .dynamic = 1, > > }, { > > .name = "Codec", > > .stream_name = "IEC958 Playback", > > .cpu_dai_name = "snd-soc-dummy-dai", > > .platform_name = "snd-soc-dummy", > > .no_pcm = 1, > > .codec_dai_name = "dit-hifi", > > .codec_name = "spdif-dit", > > }, > > > The above are two completely different beasts. Please explain how the DT > > representation for those DAI links can be the same. > > If the binding is representing the differences between those two in the > DT then the DT binding is clearly not good since it is exposing Linux > implementation details - all the binding needs to say is that there's > something connected to the S/PDIF output of the audio controller. Can you explain how the kernel would know the difference between a DPCM and a non-DPCM setup from just the information in the DT please? > > > > 2. When the driver is converted to DPCM, it must use DPCM for > > > > everything, otherwise it has no way to know which of SPDIF or I2S to > > > > enable. The only way I know to work around that is to add additional > > > > routes to link up the AIF widgets, and that's the solution you're all > > > > telling me is not acceptable, as per the patch set at the start of > > > > this thread. > > > > > > What we have been telling you is that if there is a DAI link present > > > (there should be one for each physical DAI link) then this should be > > > enough information for the framework to know that the two DAIs are > > > linked and if any routes are needed in DAPM these should be added > > > automatically in the same way that we add links for CODEC<->CODEC links > > > at present. > > > > It's been said to you off-list that having the links manually added > > > but marking them for removal when the framework figures out how to do > > > that should be OK. > > > Sorry, I just don't get this. What you seem to be telling me here is to > > forget DPCM, and just go with the dual DAI solution. > > No, it's not clear to me why you would think that. If we are talking > about adding DAPM routes matching DAI links then we are talking about > DPCM. To reiterate the dual DAI implementation would merely be a > stepping stone to a DPCM based implementation. Your reply to my question about DPCM seems to be talking about the dual-DAI solution. Maybe you need to be more specific about what "two DAIs" you are referring to. > > If I have separate CPU DAI links, then I can't do simultaneous operation, > > because both DAI links are entirely separate entities which are activated > > entirely separately. The only way I know how to handle this is with the > > patch set I've already posted. > > As you are aware DPCM supports multiple DAIs and a good DPCM > implementation for this hardware will have one front end DAI for the DMA > and two back end DAIs, one for the S/PDIF interface and one for the I2S > interface. Isn't that what this patch series creates - the front end DAI is the CPU DAI, and the backend DAIs are provided by the snd-soc-dummy-dai. I refer you to this in Liam's driver: /* Back End DAI links */ { /* SSP0 - Codec */ .name = "Codec", .be_id = 0, .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "snd-soc-dummy", .no_pcm = 1, .codec_name = "...0-001c", .codec_dai_name = "...-aif1", .ignore_suspend = 1, .ignore_pmdown_time = 1, .be_hw_params_fixup = ..., .ops = &haswell_ops, .dpcm_playback = 1, .dpcm_capture = 1, }, { /* SSP1 - BT */ .name = "SSP1-Codec", .be_id = 1, .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "snd-soc-dummy", .no_pcm = 1, .codec_name = "snd-soc-dummy", .codec_dai_name = "snd-soc-dummy-dai", .ignore_suspend = 1, .ignore_pmdown_time = 1, }, Notice that for the DPCM BE, there is no CPU-layer involvement here. The difference here is that at the moment, with this patch series plus the DPCM add-on patch, I am only creating one BE, but that BE is being created in exactly the same way as the above is doing. > > We've been through this before: this is also not how Liam's example DPCM > > driver works - please go back and look at those diagrams I drew you of > > how Liam's driver is setup. I've also said to you that from what I can > > see, the routes are still required for DPCM - those routes are in Liam's > > DPCM driver as well. > > This is why in the above message I reminded you of the suggestion that > until the framework does automatically figure out that those routes > should be there the routes are manually added in the driver clearly > marked so they can easily be removed when the framework does the right > thing here. I'm not sure how the framework could figure out these things automatically. If you go back to the diagram which I drew you for Liam's driver, it's not a simple CPU-AIF to Codec linkage - there's a mixer in the middle of it as well. There's also feedback from that mixer (which is on the output side) to an input stream to the CPU DAI side. Liam also suggested that there could be DAPM routing which can control how the FE and BEs are linked together. So, I still don't see that there is anything wrong with my patch series plus DPCM-enabling patch, apart from the issues which I've reported. Maybe you could review it and provide specific comments against the patches highlighting the code which you have issues with. As for providing ALSA with a set of PCM operations, I'm not sure what they should be for a DPCM backend.
On Mon, Sep 02, 2013 at 10:18:30PM +0100, Russell King - ARM Linux wrote: > On Mon, Sep 02, 2013 at 09:44:21PM +0100, Mark Brown wrote: > > If the binding is representing the differences between those two in the > > DT then the DT binding is clearly not good since it is exposing Linux > > implementation details - all the binding needs to say is that there's > > something connected to the S/PDIF output of the audio controller. > Can you explain how the kernel would know the difference between a DPCM > and a non-DPCM setup from just the information in the DT please? In the short term from the machine driver of course - linking things together is what it's there for. In the longer term I'd expect that the compatible string could be moved over to a generic machine driver which can understand the way we're representing the device internally, or perhaps we'll change that internal representation and it'd do something totally different anyway, but no need to worry about that for now. > > > > What we have been telling you is that if there is a DAI link present > > > > (there should be one for each physical DAI link) then this should be > > > > enough information for the framework to know that the two DAIs are > > > > linked and if any routes are needed in DAPM these should be added > > > > automatically in the same way that we add links for CODEC<->CODEC links > > > > at present. > Your reply to my question about DPCM seems to be talking about the > dual-DAI solution. Maybe you need to be more specific about what > "two DAIs" you are referring to. The two DAIs that are linked by a DAI link are the two DAIs at either end of the link, for example the I2S DAI on the SoC and the I2S DAI on the CODEC. > > As you are aware DPCM supports multiple DAIs and a good DPCM > > implementation for this hardware will have one front end DAI for the DMA > > and two back end DAIs, one for the S/PDIF interface and one for the I2S > > interface. > Isn't that what this patch series creates - the front end DAI is the CPU > DAI, and the backend DAIs are provided by the snd-soc-dummy-dai. I Not in this patch series as far as I can see, there appear to be no references to the dummy DAI in it. From a comment later on in your mail I suspect you're thinking of the result of adding some additional changes to the series, please squash those into the existing patches appropriately and resubmit. > Notice that for the DPCM BE, there is no CPU-layer involvement here. > The difference here is that at the moment, with this patch series plus > the DPCM add-on patch, I am only creating one BE, but that BE is being > created in exactly the same way as the above is doing. You should have one back end DAI per physical DAI. > > This is why in the above message I reminded you of the suggestion that > > until the framework does automatically figure out that those routes > > should be there the routes are manually added in the driver clearly > > marked so they can easily be removed when the framework does the right > > thing here. > I'm not sure how the framework could figure out these things > automatically. If you go back to the diagram which I drew you for > Liam's driver, it's not a simple CPU-AIF to Codec linkage - there's > a mixer in the middle of it as well. There's also feedback from that This is not correct, there is no mixer in the link between the back end and the CODEC. > mixer (which is on the output side) to an input stream to the CPU DAI > side. > Liam also suggested that there could be DAPM routing which can control > how the FE and BEs are linked together. This is correct, it is in fact required for operation - you are of course well aware that front ends and back ends are not connected using DAI links but are instead connected using normal DAPM links. This should all be very simple for Kirkwood. > So, I still don't see that there is anything wrong with my patch series > plus DPCM-enabling patch, apart from the issues which I've reported. > Maybe you could review it and provide specific comments against the > patches highlighting the code which you have issues with. I think the most serious issues with the current series were already covered by Lars-Peter and the discussion in these subtreads, no need to repeat things. > As for providing ALSA with a set of PCM operations, I'm not sure what > they should be for a DPCM backend. Unless there is a need to actually take some action you can, naturally, provide stubs. You should provide the minimal set of stubs required for operation in your testing.
On Mon, Sep 02, 2013 at 11:35:00PM +0100, Mark Brown wrote: > Not in this patch series as far as I can see, there appear to be no > references to the dummy DAI in it. From a comment later on in your mail > I suspect you're thinking of the result of adding some additional > changes to the series, please squash those into the existing patches > appropriately and resubmit. I'm thinking of making _no_ changes to this series at present, because I fail to see anything wrong with it. As for "no references to the dummy DAI in it" - I did mention the additional patch which is _not_ part of this series which contains the changes to enable DPCM. Here's an extract which I posted just two messages before this one of the resulting DAI link structure: Here's the DPCM version: { .name = "S/PDIF1", .stream_name = "Audio Playback", .platform_name = "mvebu-audio.1", .cpu_dai_name = "mvebu-audio.1", .codec_name = "snd-soc-dummy", .codec_dai_name = "snd-soc-dummy-dai", .dynamic = 1, }, { .name = "Codec", .stream_name = "IEC958 Playback", .cpu_dai_name = "snd-soc-dummy-dai", .platform_name = "snd-soc-dummy", .no_pcm = 1, .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", }, Please see the second entry. This is the backend DAI setup. This refers to the dummy DAI for the CPU, and has 'no_pcm' set - both of which I believe are required to indicate that this is representing a backend DAI. As I've said before, the reason I haven't included this file is that it is unclear that it is useful for non-DT setups. As there is no non-DT Cubox support in the kernel and never will be, I don't see any point submitting it as part of this series. Secondly, I don't see the point of it being part of this series, because if we merge the changes for DPCM support, the thing falls apart - I'm not going to list the problems yet again. > > Notice that for the DPCM BE, there is no CPU-layer involvement here. > > > The difference here is that at the moment, with this patch series plus > > the DPCM add-on patch, I am only creating one BE, but that BE is being > > created in exactly the same way as the above is doing. > > You should have one back end DAI per physical DAI. What do you mean "physical DAI"? Do you mean "CPU DAI" which would be a front end DAI? Liam told me that it was possible to have multiple backends for a single front end. You've also told me on IRC: 16:29 < broonie> I have a pretty good idea of how I'd expect to see the hardware represented - two BEs for the DAIs connected to a FE for the DMA. That isn't two FEs. You explicitly state there "two BEs" for a single FE. > > > This is why in the above message I reminded you of the suggestion that > > > until the framework does automatically figure out that those routes > > > should be there the routes are manually added in the driver clearly > > > marked so they can easily be removed when the framework does the right > > > thing here. > > > I'm not sure how the framework could figure out these things > > automatically. If you go back to the diagram which I drew you for > > Liam's driver, it's not a simple CPU-AIF to Codec linkage - there's > > a mixer in the middle of it as well. There's also feedback from that > > This is not correct, there is no mixer in the link between the back end > and the CODEC. Let me re-post the structure of the widgets linking the codecs streams and CPU streams in Liam's driver then. CPU DAI drivers: DAI stream names: -------------------------------------- System Pin System Playback Offload0 Pin Offload0 Playback Offload1 Pin Offload1 Playback Loopback Pin Loopback Capture Capture Pin Analog Capture DAPM routes: [s]System Playback --------v .----> [aif]SSP0 CODEC OUT [s]Offload0 Playback ---> [w]Playback VMixer [s]Offload1 Playback --------^ `----> [s]Loopback Capture [aif]SSP0 CODEC IN ---> [s]Analog Capture where [s] is a stream widget, [w] is a DAPM widget, and [aif] is an AIF widget. Looks like there's a mixer in there to me. It may not be a bit of hardware which actually does mixing or anything (I don't know what it does) but that appears to be DAPM structure which Liam creates between the CPU streams and the codecs. > > mixer (which is on the output side) to an input stream to the CPU DAI > > side. > > > Liam also suggested that there could be DAPM routing which can control > > how the FE and BEs are linked together. > > This is correct, it is in fact required for operation - you are of > course well aware that front ends and back ends are not connected using > DAI links but are instead connected using normal DAPM links. This > should all be very simple for Kirkwood. And these are the DAPM routes which you're telling me to put a comment against because they will need to be removed. I can't see why they'd ever need to be removed, and I can't see how ASoC could possibly know the structure between the CPU and codec in DPCM solutions. > > So, I still don't see that there is anything wrong with my patch series > > plus DPCM-enabling patch, apart from the issues which I've reported. > > Maybe you could review it and provide specific comments against the > > patches highlighting the code which you have issues with. > > I think the most serious issues with the current series were already > covered by Lars-Peter and the discussion in these subtreads, no need to > repeat things. Sorry, I thought Lars-Peter's comments were about getting something that would be acceptable to go in _before_ DPCM was in a working state. So, I'll re-state again. I see nothing wrong with my patch series plus the DPCM-enabling patch. I see this as a complete and proper DPCM solution with no flaws in the driver code what so ever. I don't see Lars-Peter's comments being relevant to the DPCM solution, but only to a stop-gap solution. > > As for providing ALSA with a set of PCM operations, I'm not sure what > > they should be for a DPCM backend. > > Unless there is a need to actually take some action you can, naturally, > provide stubs. You should provide the minimal set of stubs required for > operation in your testing. That needs to go in the ASoC code though - I don't think I have access to the ALSA PCM which has been created for the backend DAI(s). Remember, the backend DAIs are "owned" by the dummy DAI driver, not the CPU driver, so the CPU driver doesn't get to see any operations for that.
On Tue, Sep 03, 2013 at 12:00:11AM +0100, Russell King - ARM Linux wrote: > > You should have one back end DAI per physical DAI. > What do you mean "physical DAI"? Do you mean "CPU DAI" which would be a > front end DAI? The same question got asked and answered in another subthread but for the record I'm speaking here about the back end DAIs - there should be one for S/PDIF and one for I2S. With DPCM there should also be a front end DAI for the DMA. I've cut a bunch of other discussion which was duplicated in that subthread. > > Unless there is a need to actually take some action you can, naturally, > > provide stubs. You should provide the minimal set of stubs required for > > operation in your testing. > That needs to go in the ASoC code though - I don't think I have access > to the ALSA PCM which has been created for the backend DAI(s). Remember, > the backend DAIs are "owned" by the dummy DAI driver, not the CPU driver, > so the CPU driver doesn't get to see any operations for that. If you need to define operations for the dummy driver then modify the dummy driver. However as indicated in the other thread you should not be using the dummy DAI driver for the CPU side back end.
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index cc733d1..8ef4241 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -35,9 +35,7 @@ #define KIRKWOOD_I2S_FORMATS \ (SNDRV_PCM_FMTBIT_S16_LE | \ SNDRV_PCM_FMTBIT_S24_LE | \ - SNDRV_PCM_FMTBIT_S32_LE | \ - SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE | \ - SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE) + SNDRV_PCM_FMTBIT_S32_LE) static void kirkwood_i2s_dump_spdif(struct device *dev, struct kirkwood_dma_data *priv) @@ -258,10 +256,22 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai); - uint32_t ctl_play, ctl_rec; + uint32_t ctl_play, ctl_rec, ctl_play_mask, ctl_rec_mask; unsigned int i2s_reg; unsigned long i2s_value; + /* + * Select the playback/record enable masks according to which + * DAI is being used. + */ + if (dai->id == 0) { + ctl_play_mask = KIRKWOOD_PLAYCTL_I2S_EN; + ctl_rec_mask = KIRKWOOD_RECCTL_I2S_EN; + } else { + ctl_play_mask = KIRKWOOD_PLAYCTL_SPDIF_EN; + ctl_rec_mask = KIRKWOOD_RECCTL_SPDIF_EN; + } + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { i2s_reg = KIRKWOOD_I2S_PLAYCTL; } else { @@ -279,48 +289,30 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, */ switch (params_format(params)) { case SNDRV_PCM_FORMAT_S16_LE: + case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE: + case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16; - ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C | - KIRKWOOD_PLAYCTL_I2S_EN | - KIRKWOOD_PLAYCTL_SPDIF_EN; - ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C | - KIRKWOOD_RECCTL_I2S_EN; + ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C | ctl_play_mask; + ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C | ctl_rec_mask; break; /* * doesn't work... S20_3LE != kirkwood 20bit format ? * case SNDRV_PCM_FORMAT_S20_3LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20; - ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 | - KIRKWOOD_PLAYCTL_I2S_EN | - KIRKWOOD_PLAYCTL_SPDIF_EN; - ctl_rec = KIRKWOOD_RECCTL_SIZE_20 | - KIRKWOOD_RECCTL_I2S_EN; + ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 | ctl_play_mask; + ctl_rec = KIRKWOOD_RECCTL_SIZE_20 | ctl_rec_mask; break; */ case SNDRV_PCM_FORMAT_S24_LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24; - ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 | - KIRKWOOD_PLAYCTL_I2S_EN | - KIRKWOOD_PLAYCTL_SPDIF_EN; - ctl_rec = KIRKWOOD_RECCTL_SIZE_24 | - KIRKWOOD_RECCTL_I2S_EN; + ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 | ctl_play_mask; + ctl_rec = KIRKWOOD_RECCTL_SIZE_24 | ctl_rec_mask; break; case SNDRV_PCM_FORMAT_S32_LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_32; - ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 | - KIRKWOOD_PLAYCTL_I2S_EN; - ctl_rec = KIRKWOOD_RECCTL_SIZE_32 | - KIRKWOOD_RECCTL_I2S_EN; - break; - case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE: - case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE: - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) - return -EINVAL; - i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16; - ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C | - KIRKWOOD_PLAYCTL_SPDIF_EN; - ctl_rec = 0; + ctl_play = KIRKWOOD_PLAYCTL_SIZE_32 | ctl_play_mask; + ctl_rec = KIRKWOOD_RECCTL_SIZE_32 | ctl_rec_mask; break; default: return -EINVAL; @@ -333,12 +325,12 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, ctl_play |= KIRKWOOD_PLAYCTL_MONO_OFF; priv->ctl_play &= ~(KIRKWOOD_PLAYCTL_MONO_MASK | - KIRKWOOD_PLAYCTL_I2S_EN | - KIRKWOOD_PLAYCTL_SPDIF_EN | + ctl_play_mask | KIRKWOOD_PLAYCTL_SIZE_MASK); priv->ctl_play |= ctl_play; } else { - priv->ctl_rec &= ~KIRKWOOD_RECCTL_SIZE_MASK; + priv->ctl_rec &= ~(KIRKWOOD_RECCTL_SIZE_MASK | + ctl_rec_mask); priv->ctl_rec |= ctl_rec; } @@ -351,7 +343,13 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai); - uint32_t ctl, value; + uint32_t ctl, value, mute_mask; + + if (dai->id == 0) { + mute_mask = KIRKWOOD_PLAYCTL_I2S_MUTE; + } else { + mute_mask = KIRKWOOD_PLAYCTL_SPDIF_MUTE; + } ctl = readl(priv->io + KIRKWOOD_PLAYCTL); if (ctl & KIRKWOOD_PLAYCTL_PAUSE) { @@ -396,7 +394,7 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_STOP: /* stop audio, disable interrupts */ - ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE; + ctl |= KIRKWOOD_PLAYCTL_PAUSE | mute_mask; writel(ctl, priv->io + KIRKWOOD_PLAYCTL); value = readl(priv->io + KIRKWOOD_INT_MASK); @@ -410,13 +408,13 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_PAUSE_PUSH: case SNDRV_PCM_TRIGGER_SUSPEND: - ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE; + ctl |= KIRKWOOD_PLAYCTL_PAUSE | mute_mask; writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break; case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE); + ctl &= ~(KIRKWOOD_PLAYCTL_PAUSE | mute_mask); writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break; @@ -499,56 +497,6 @@ static int kirkwood_i2s_trigger(struct snd_pcm_substream *substream, int cmd, return 0; } -static int kirkwood_i2s_probe(struct snd_soc_dai *dai) -{ - struct kirkwood_dma_data *priv = snd_soc_dai_get_drvdata(dai); - unsigned long value; - unsigned int reg_data; - int ret; - - ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls, - ARRAY_SIZE(kirkwood_i2s_iec958_controls)); - if (ret) { - dev_err(dai->dev, "unable to add soc card controls\n"); - return ret; - } - /* put system in a "safe" state : */ - /* disable audio interrupts */ - writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE); - writel(0, priv->io + KIRKWOOD_INT_MASK); - - reg_data = readl(priv->io + 0x120c); - reg_data = readl(priv->io + 0x1200); - reg_data &= (~(0x333FF8)); - reg_data |= 0x111D18; - writel(reg_data, priv->io + 0x1200); - - msleep(500); - - reg_data = readl(priv->io + 0x1200); - reg_data &= (~(0x333FF8)); - reg_data |= 0x111D18; - msleep(500); - writel(reg_data, priv->io + 0x1200); - - /* disable playback/record */ - value = readl(priv->io + KIRKWOOD_PLAYCTL); - value &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN); - writel(value, priv->io + KIRKWOOD_PLAYCTL); - - value = readl(priv->io + KIRKWOOD_RECCTL); - value &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN); - writel(value, priv->io + KIRKWOOD_RECCTL); - - return 0; - -} - -static int kirkwood_i2s_remove(struct snd_soc_dai *dai) -{ - return 0; -} - static const struct snd_soc_dai_ops kirkwood_i2s_dai_ops = { .startup = kirkwood_i2s_startup, .trigger = kirkwood_i2s_trigger, @@ -556,54 +504,57 @@ static const struct snd_soc_dai_ops kirkwood_i2s_dai_ops = { .set_fmt = kirkwood_i2s_set_fmt, }; +static int kirkwood_spdif_probe(struct snd_soc_dai *dai) +{ + int ret = snd_soc_add_dai_controls(dai, kirkwood_i2s_iec958_controls, + ARRAY_SIZE(kirkwood_i2s_iec958_controls)); + if (ret) + dev_err(dai->dev, "unable to add soc card controls\n"); -static struct snd_soc_dai_driver kirkwood_i2s_dai = { - .probe = kirkwood_i2s_probe, - .remove = kirkwood_i2s_remove, - .playback = { - .channels_min = 1, - .channels_max = 2, - .rates = KIRKWOOD_I2S_RATES, - .formats = KIRKWOOD_I2S_FORMATS, - }, - .capture = { - .channels_min = 1, - .channels_max = 2, - .rates = KIRKWOOD_I2S_RATES, - .formats = KIRKWOOD_I2S_FORMATS, - }, - .ops = &kirkwood_i2s_dai_ops, -}; + return ret; +} -static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk = { - .probe = kirkwood_i2s_probe, - .remove = kirkwood_i2s_remove, - .playback = { - .channels_min = 1, - .channels_max = 2, - .rates = SNDRV_PCM_RATE_8000_192000 | - SNDRV_PCM_RATE_CONTINUOUS | - SNDRV_PCM_RATE_KNOT, - .formats = KIRKWOOD_I2S_FORMATS, - }, - .capture = { - .channels_min = 1, - .channels_max = 2, - .rates = SNDRV_PCM_RATE_8000_192000 | - SNDRV_PCM_RATE_CONTINUOUS | - SNDRV_PCM_RATE_KNOT, - .formats = KIRKWOOD_I2S_FORMATS, +static struct snd_soc_dai_driver kirkwood_dais[KIRKWOOD_NUM_DAIS] = { + { + .name = "kirkwood-i2s.%d", + .ops = &kirkwood_i2s_dai_ops, + .playback = { + .channels_min = 1, + .channels_max = 2, + .rates = KIRKWOOD_I2S_RATES, + .formats = KIRKWOOD_I2S_FORMATS, + }, + .capture = { + .channels_min = 1, + .channels_max = 2, + .rates = KIRKWOOD_I2S_RATES, + .formats = KIRKWOOD_I2S_FORMATS, + }, + .symmetric_rates = 1, + }, { + .name = "kirkwood-spdif.%d", + .probe = kirkwood_spdif_probe, + .ops = &kirkwood_i2s_dai_ops, + .playback = { + .channels_min = 1, + .channels_max = 2, + .rates = KIRKWOOD_I2S_RATES, + .formats = SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S32_LE | + SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE | + SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE, + }, }, - .ops = &kirkwood_i2s_dai_ops, }; static int kirkwood_i2s_dev_probe(struct platform_device *pdev) { struct kirkwood_asoc_platform_data *data = pdev->dev.platform_data; - struct snd_soc_dai_driver *soc_dai = &kirkwood_i2s_dai; struct kirkwood_dma_data *priv; struct resource *mem; - int err; + int i, err; + u32 val; priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); if (!priv) { @@ -612,6 +563,24 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) } dev_set_drvdata(&pdev->dev, priv); + memcpy(priv->dai_driver, kirkwood_dais, sizeof(priv->dai_driver)); + + /* format the DAI names according to the platform device ID */ + for (i = 0; i < KIRKWOOD_NUM_DAIS; i++) { + const char *fmt = priv->dai_driver[i].name; + char *name, *p; + + if (pdev->id < 0) { + name = kstrdup(fmt, GFP_KERNEL); + p = strchr(name, '.'); + if (p) + *p = '\0'; + } else { + name = kasprintf(GFP_KERNEL, fmt, pdev->id); + } + priv->dai_driver[i].name = name; + } + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!mem) { dev_err(&pdev->dev, "platform_get_resource failed\n"); @@ -651,9 +620,23 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) clk_put(priv->extclk); priv->extclk = ERR_PTR(-EINVAL); } else { + int i; + dev_info(&pdev->dev, "found external clock\n"); clk_prepare_enable(priv->extclk); - soc_dai = &kirkwood_i2s_dai_extclk; + + for (i = 0; i < KIRKWOOD_NUM_DAIS; i++) { + if (priv->dai_driver[i].playback.rates) + priv->dai_driver[i].playback.rates = + SNDRV_PCM_RATE_8000_192000 | + SNDRV_PCM_RATE_CONTINUOUS | + SNDRV_PCM_RATE_KNOT; + if (priv->dai_driver[i].capture.rates) + priv->dai_driver[i].capture.rates = + SNDRV_PCM_RATE_8000_192000 | + SNDRV_PCM_RATE_CONTINUOUS | + SNDRV_PCM_RATE_KNOT; + } } } @@ -673,7 +656,35 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) writel(KIRKWOOD_MCLK_SOURCE_DCO, priv->io+KIRKWOOD_CLOCKS_CTRL); - err = snd_soc_register_dai(&pdev->dev, soc_dai); + /* put system in a "safe" state : disable audio interrupts */ + writel(0xffffffff, priv->io + KIRKWOOD_INT_CAUSE); + writel(0, priv->io + KIRKWOOD_INT_MASK); + + val = readl(priv->io + 0x120c); + val = readl(priv->io + 0x1200); + val &= (~(0x333FF8)); + val |= 0x111D18; + writel(val, priv->io + 0x1200); + + msleep(500); + + val = readl(priv->io + 0x1200); + val &= (~(0x333FF8)); + val |= 0x111D18; + msleep(500); + writel(val, priv->io + 0x1200); + + /* disable playback/record */ + val = readl(priv->io + KIRKWOOD_PLAYCTL); + val &= ~(KIRKWOOD_PLAYCTL_I2S_EN|KIRKWOOD_PLAYCTL_SPDIF_EN); + writel(val, priv->io + KIRKWOOD_PLAYCTL); + + val = readl(priv->io + KIRKWOOD_RECCTL); + val &= ~(KIRKWOOD_RECCTL_I2S_EN | KIRKWOOD_RECCTL_SPDIF_EN); + writel(val, priv->io + KIRKWOOD_RECCTL); + + err = snd_soc_register_dais(&pdev->dev, priv->dai_driver, + KIRKWOOD_NUM_DAIS); if (!err) return 0; dev_err(&pdev->dev, "snd_soc_register_dai failed\n"); diff --git a/sound/soc/kirkwood/kirkwood-spdif.c b/sound/soc/kirkwood/kirkwood-spdif.c index 2c459c1..62e5b62 100644 --- a/sound/soc/kirkwood/kirkwood-spdif.c +++ b/sound/soc/kirkwood/kirkwood-spdif.c @@ -61,7 +61,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai0[] = { .name = "S/PDIF0", .stream_name = "S/PDIF0 PCM Playback", .platform_name = "kirkwood-pcm-audio.0", - .cpu_dai_name = "kirkwood-i2s.0", + .cpu_dai_name = "kirkwood-spdif.0", .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", .ops = &kirkwood_spdif_ops, @@ -73,7 +73,7 @@ static struct snd_soc_dai_link kirkwood_spdif_dai1[] = { .name = "S/PDIF1", .stream_name = "IEC958 Playback", .platform_name = "kirkwood-pcm-audio.1", - .cpu_dai_name = "kirkwood-i2s.1", + .cpu_dai_name = "kirkwood-spdif.1", .codec_dai_name = "dit-hifi", .codec_name = "spdif-dit", .ops = &kirkwood_spdif_ops, diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index b7cf25c..c7ca27e 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -157,12 +157,15 @@ #define KIRKWOOD_SND_MAX_PERIOD_BYTES 0x800000 #define KIRKWOOD_SND_MAX_BUFFER_BYTES 0x100000 +#define KIRKWOOD_NUM_DAIS 2 + struct kirkwood_dma_data { void __iomem *io; struct clk *clk; struct clk *extclk; uint32_t ctl_play; uint32_t ctl_rec; + struct snd_soc_dai_driver dai_driver[KIRKWOOD_NUM_DAIS]; struct snd_pcm_substream *substream_play; struct snd_pcm_substream *substream_rec; int irq;