| Message ID | 20130924124122.64834b2a@armhf |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 24, 2013 at 12:41:22PM +0200, Jean-Francois Moine wrote: > This patch adds S/PDIF input/output for mvebu DT boards. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> Right, so if this goes in, then we're completely fucked for doing this the way that Mark apparantly wants it to be done, because once these DT properties go in, they're there to stay.
On Tue, Sep 24, 2013 at 11:51:27AM +0100, Russell King - ARM Linux wrote: > On Tue, Sep 24, 2013 at 12:41:22PM +0200, Jean-Francois Moine wrote: > > This patch adds S/PDIF input/output for mvebu DT boards. > Right, so if this goes in, then we're completely fucked for doing this > the way that Mark apparantly wants it to be done, because once these > DT properties go in, they're there to stay. I'm not sure that this is in particular conflict with any other way of writing things, though it would be redundant. The best thing would be to key this off connecting to DAIs so that people just need to make the connections that exist in their system in the device tree.
On Tue, 24 Sep 2013 11:51:27 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Sep 24, 2013 at 12:41:22PM +0200, Jean-Francois Moine wrote: > > This patch adds S/PDIF input/output for mvebu DT boards. > > > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > > Right, so if this goes in, then we're completely fucked for doing this > the way that Mark apparantly wants it to be done, because once these > DT properties go in, they're there to stay. Indeed! The DT properties I propose just reflect the hardware connections of the board. There are only i2s-out and spdif-out in the Cubox, while there may be kirkwood boards with, say, only spdif-in and spdif-out. These definitions do not interfere with your DPCM problem: you assume i2s-out and spdif-out for the i2s1 dove device, and that is what I assume too, but from the DT. Then, the driver does not need to know the type of the audio device (Kirkwood or Dove), nor to know if it is the first or the second i2s device of the Dove chip.
On Tue, Sep 24, 2013 at 12:41:22PM +0200, Jean-Francois Moine wrote:
> This patch adds S/PDIF input/output for mvebu DT boards.
This got less discussion than I was expecting and then slipped under a
lot of other patches in my queue while that was timing out, sorry... in
any case, this still has the issue that it used a single DAI and a
single DAI link to represent two physical DAIs and their links.
Please as covered in the prior threads on this either implement DPCM or
implement two traditional DAIs which can't be used simultaneously (which
should be simpler and can then be built on incrementally).
The last set of DPCMish patches was reported to work so should be a
fairly small refactoring away from being OK - the main thing is to
create back end DAIs for the physical links and then put the widgets
that manage the enable of the S/PDIF and I2S interfaces between the FE
and BE DAIs instead of directly connecting them to widgets on the CODEC.
For two traditional DAIs it should just be a case of duplicating the DAI
with a differnet ID then setting the enable bits based on the ID of the
DAI in use and ideally adding some code to check that they're not both
active (or that limit can be enforced by only having machine drivers
that use one or the other).
On Fri, Oct 18, 2013 at 01:34:39AM +0100, Mark Brown wrote: > The last set of DPCMish patches was reported to work so should be a > fairly small refactoring away from being OK - the main thing is to > create back end DAIs for the physical links and then put the widgets > that manage the enable of the S/PDIF and I2S interfaces between the FE > and BE DAIs instead of directly connecting them to widgets on the CODEC. ROTFL. You're now telling JF to do exactly what I did in my last patch. Your two-faced-ness is utterly astounding. "widgets that manage the enable of the S/PDIF and I2S interfaces between the FE and BE DAIs" is exactly what I did and you told me that was wrong. Make up your fscking mind.
On Fri, Oct 18, 2013 at 11:30:19AM +0100, Russell King - ARM Linux wrote: > On Fri, Oct 18, 2013 at 01:34:39AM +0100, Mark Brown wrote: > > The last set of DPCMish patches was reported to work so should be a > > fairly small refactoring away from being OK - the main thing is to > > create back end DAIs for the physical links and then put the widgets > > that manage the enable of the S/PDIF and I2S interfaces between the FE > > and BE DAIs instead of directly connecting them to widgets on the CODEC. > ROTFL. You're now telling JF to do exactly what I did in my last patch. > Your two-faced-ness is utterly astounding. > "widgets that manage the enable of the S/PDIF and I2S interfaces between > the FE and BE DAIs" is exactly what I did and you told me that was wrong. > Make up your fscking mind. No, this is not the case. The major problem with your last set of patches was that they did not create any back end DAIs, instead they created purely DAPM routes from the single traditional DAI out to widgets in the CODEC. What I'm asking Jean-Francois to do above is create and use back end DAIs. Please let me also remind you that confrontational behaviour such as the ad hominem remarks above is not at all constructive or helpful, please keep the discussion civil.
On Fri, Oct 18, 2013 at 01:48:32PM +0100, Mark Brown wrote: > On Fri, Oct 18, 2013 at 11:30:19AM +0100, Russell King - ARM Linux wrote: > > On Fri, Oct 18, 2013 at 01:34:39AM +0100, Mark Brown wrote: > > > > The last set of DPCMish patches was reported to work so should be a > > > fairly small refactoring away from being OK - the main thing is to > > > create back end DAIs for the physical links and then put the widgets > > > that manage the enable of the S/PDIF and I2S interfaces between the FE > > > and BE DAIs instead of directly connecting them to widgets on the CODEC. > > > ROTFL. You're now telling JF to do exactly what I did in my last patch. > > Your two-faced-ness is utterly astounding. > > > "widgets that manage the enable of the S/PDIF and I2S interfaces between > > the FE and BE DAIs" is exactly what I did and you told me that was wrong. > > Make up your fscking mind. > > No, this is not the case. The major problem with your last set of > patches was that they did not create any back end DAIs, instead they > created purely DAPM routes from the single traditional DAI out to > widgets in the CODEC. What I'm asking Jean-Francois to do above is > create and use back end DAIs. > > Please let me also remind you that confrontational behaviour such as the > ad hominem remarks above is not at all constructive or helpful, please > keep the discussion civil. Let me remind you that I had a definition of what a front end and a back end DAI was from Liam, and your definition is at odds with that. I am still of the opinion that you don't know what you're talking about most of the time, and nothing has changed in that regard, and I still regard you as a completely obstructive and unhelpful person. And I think you're just wrong on what you've said above. But... I've basically given up any hope of progressing that code after months of pissing around with your obstructive attitude, and I've elected (and others) to maintain it completely out of mainline for the forseeable future - hopefully until you get replaced by someone who can be more assistive and helpful.
On Fri, Oct 18, 2013 at 01:56:38PM +0100, Russell King - ARM Linux wrote: > Let me remind you that I had a definition of what a front end and a back > end DAI was from Liam, and your definition is at odds with that. Could you please provide some detail on this? I was aware that you were for a time confused about this but had not been aware of the source of the confusion. > I am still of the opinion that you don't know what you're talking about > most of the time, and nothing has changed in that regard, and I still > regard you as a completely obstructive and unhelpful person. > And I think you're just wrong on what you've said above. But... I've > basically given up any hope of progressing that code after months of > pissing around with your obstructive attitude, and I've elected (and > others) to maintain it completely out of mainline for the forseeable > future - hopefully until you get replaced by someone who can be more > assistive and helpful. Once more, please be civil and constructive - a confrontational approach has not been helping here. If you are intent on behaving in this fashion please at least try to avoid disrupting others who are interested in working in mainline.
On Fri, Oct 18, 2013 at 02:48:29PM +0100, Mark Brown wrote: > On Fri, Oct 18, 2013 at 01:56:38PM +0100, Russell King - ARM Linux wrote: > > > Let me remind you that I had a definition of what a front end and a back > > end DAI was from Liam, and your definition is at odds with that. > > Could you please provide some detail on this? I was aware that you were > for a time confused about this but had not been aware of the source of > the confusion. Let's see: 16:10 < rmk> broonie: can you explain this pcm frontend/backend stuff in asoc. what exactly is a frontend and backend pcm, and how do they relate to the CPU hardware and codec links? Which are the ones which are given ALSA PCMs? 17:22 < broonie> rmk: A front end is the bit that does DMA from memory, a back end is a physical interface. 17:22 < broonie> The bit in the middle is usually a DSP. 17:23 < broonie> To be frank I've not actually *used* this stuff myself except in bolting a new CODEC on the edge of it during system integrations - going to be rectifying that very soon with the Samsung drivers I hope. 17:24 < broonie> But the general idea is that the front end is what the application sees, the back end is what comes out of the hardware at the other end. 17:25 < broonie> And the bit in the middle does any routing an rewriting of formats between the two. 17:26 < broonie> (The Samsung picture should be very similar to Kirkwood but the opposite way around - two front ends mixed togther to a single back end) ... 16:49 < rmk> oh, another question 16:49 < rmk> .dynamic in the dai link structure, that's for backends only, right? 16:50 < broonie> Think so, yes. 16:52 < rmk> hmm 16:52 < rmk> if (rtd->dai_link->dynamic) { 16:52 < rmk> rtd->ops.open = dpcm_fe_dai_open; 16:52 < rmk> probably needed for frontends
On Fri, Oct 18, 2013 at 02:48:29PM +0100, Mark Brown wrote: > On Fri, Oct 18, 2013 at 01:56:38PM +0100, Russell King - ARM Linux wrote: > > And I think you're just wrong on what you've said above. But... I've > > basically given up any hope of progressing that code after months of > > pissing around with your obstructive attitude, and I've elected (and > > others) to maintain it completely out of mainline for the forseeable > > future - hopefully until you get replaced by someone who can be more > > assistive and helpful. > > Once more, please be civil and constructive - a confrontational approach > has not been helping here. If you are intent on behaving in this > fashion please at least try to avoid disrupting others who are > interested in working in mainline. Well, you say that but that's actually a total lie. It is you who have done far more than anyone else to disrupt progress on this issue through your purposely misleading and unhelpful comments. Many I remind you that it was _me_ who spent months implementing the suggestions you came out with only to be told time and time again that it was wrong.
On Fri, 18 Oct 2013 13:48:32 +0100 Mark Brown <broonie@kernel.org> wrote: > > "widgets that manage the enable of the S/PDIF and I2S interfaces between > > the FE and BE DAIs" is exactly what I did and you told me that was wrong. > > Make up your fscking mind. > > No, this is not the case. The major problem with your last set of > patches was that they did not create any back end DAIs, instead they > created purely DAPM routes from the single traditional DAI out to > widgets in the CODEC. What I'm asking Jean-Francois to do above is > create and use back end DAIs. Mark, I think I understood what you mean. I am testing my system with this new approach and I will submit a new patch as soon as it works.
diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt index f0062c5..c19a5d3 100644 --- a/Documentation/devicetree/bindings/sound/mvebu-audio.txt +++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt @@ -22,6 +22,27 @@ Required properties: "internal" for the internal clock "extclk" for the external clock +Optional properties: + +- i2s-out: This is a boolean property. If present, the transmitting + function of I2S will be enabled, indicating the I2S is connected + to some IP block, such as an HDMI encoder/display-controller. + +- i2s-in: This is a boolean property. If present, the receiving + function of I2S will be enabled, indicating the I2S is connected + to some IP block, such as an HDMI encoder/display-controller. + +- spdif-out: This is a boolean property. If present, the transmitting + function of S/PDIF will be enabled, indicating there's a physical + S/PDIF out connector/jack on the board or it's connecting to some + other IP block, such as an HDMI encoder/display-controller. + +- spdif-in: This is a boolean property. If present, the receiving + function of S/PDIF will be enabled, indicating there's a physical + S/PDIF in connector/jack on the board. + +* Note: At least one of these four properties should be set in the DT binding. + Example: i2s1: audio-controller@b4000 { @@ -30,4 +51,6 @@ i2s1: audio-controller@b4000 { interrupts = <21>, <22>; clocks = <&gate_clk 13>; clock-names = "internal"; + i2s-out; + spdif-out; }; diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c index 0f3d73d..c62c7fa 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -160,9 +160,11 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, case SNDRV_PCM_FORMAT_S16_LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16; ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C | - KIRKWOOD_PLAYCTL_I2S_EN; + KIRKWOOD_PLAYCTL_I2S_EN | + KIRKWOOD_PLAYCTL_SPDIF_EN; ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C | - KIRKWOOD_RECCTL_I2S_EN; + KIRKWOOD_RECCTL_I2S_EN | + KIRKWOOD_RECCTL_SPDIF_EN; break; /* * doesn't work... S20_3LE != kirkwood 20bit format ? @@ -178,9 +180,11 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream, 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_I2S_EN | + KIRKWOOD_PLAYCTL_SPDIF_EN; ctl_rec = KIRKWOOD_RECCTL_SIZE_24 | - KIRKWOOD_RECCTL_I2S_EN; + KIRKWOOD_RECCTL_I2S_EN | + KIRKWOOD_PLAYCTL_I2S_EN; break; case SNDRV_PCM_FORMAT_S32_LE: i2s_value |= KIRKWOOD_I2S_CTL_SIZE_32; @@ -253,12 +257,14 @@ static int kirkwood_i2s_play_trigger(struct snd_pcm_substream *substream, writel(value, priv->io + KIRKWOOD_INT_MASK); /* enable playback */ + ctl &= priv->ctl_play_mask; writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break; case SNDRV_PCM_TRIGGER_STOP: /* stop audio, disable interrupts */ - ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE; + ctl |= KIRKWOOD_PLAYCTL_PAUSE | KIRKWOOD_PLAYCTL_I2S_MUTE | + KIRKWOOD_PLAYCTL_SPDIF_MUTE; writel(ctl, priv->io + KIRKWOOD_PLAYCTL); value = readl(priv->io + KIRKWOOD_INT_MASK); @@ -272,13 +278,15 @@ 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 | KIRKWOOD_PLAYCTL_I2S_MUTE | + KIRKWOOD_PLAYCTL_SPDIF_MUTE; 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 | KIRKWOOD_PLAYCTL_I2S_MUTE | + KIRKWOOD_PLAYCTL_SPDIF_MUTE); writel(ctl, priv->io + KIRKWOOD_PLAYCTL); break; @@ -301,7 +309,8 @@ static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_START: /* configure */ ctl = priv->ctl_rec; - value = ctl & ~KIRKWOOD_RECCTL_I2S_EN; + value = ctl & ~(KIRKWOOD_RECCTL_I2S_EN | + KIRKWOOD_RECCTL_SPDIF_EN); writel(value, priv->io + KIRKWOOD_RECCTL); /* enable interrupts */ @@ -310,6 +319,7 @@ static int kirkwood_i2s_rec_trigger(struct snd_pcm_substream *substream, writel(value, priv->io + KIRKWOOD_INT_MASK); /* enable record */ + ctl &= priv->ctl_rec_mask; writel(ctl, priv->io + KIRKWOOD_RECCTL); break; @@ -404,55 +414,75 @@ static const struct snd_soc_dai_ops kirkwood_i2s_dai_ops = { .set_fmt = kirkwood_i2s_set_fmt, }; +static const struct snd_soc_component_driver kirkwood_i2s_component = { + .name = DRV_NAME, +}; -static struct snd_soc_dai_driver kirkwood_i2s_dai = { - .probe = kirkwood_i2s_probe, - .playback = { - .channels_min = 1, - .channels_max = 2, - .rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | - SNDRV_PCM_RATE_96000, - .formats = KIRKWOOD_I2S_FORMATS, - }, - .capture = { +static int kirkwood_register_component(struct device *dev, + struct kirkwood_dma_data *priv) +{ + struct device_node *np = dev->of_node; + uint32_t ctl_play_mask = ~(KIRKWOOD_PLAYCTL_I2S_EN | + KIRKWOOD_PLAYCTL_SPDIF_EN); + uint32_t ctl_rec_mask = ~(KIRKWOOD_RECCTL_I2S_EN | + KIRKWOOD_RECCTL_SPDIF_EN); + int ret; + + static const struct snd_soc_pcm_stream stream = { .channels_min = 1, .channels_max = 2, .rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000, .formats = KIRKWOOD_I2S_FORMATS, - }, - .ops = &kirkwood_i2s_dai_ops, -}; + }; -static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk = { - .probe = kirkwood_i2s_probe, - .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, - }, - .ops = &kirkwood_i2s_dai_ops, -}; + if (np) { + if (of_property_read_bool(np, "i2s-in")) + ctl_rec_mask |= KIRKWOOD_RECCTL_I2S_EN; + if (of_property_read_bool(np, "i2s-out")) + ctl_play_mask |= KIRKWOOD_PLAYCTL_I2S_EN; + if (of_property_read_bool(np, "spdif-in")) + ctl_rec_mask |= KIRKWOOD_RECCTL_SPDIF_EN; + if (of_property_read_bool(np, "spdif-out")) + ctl_play_mask |= KIRKWOOD_PLAYCTL_SPDIF_EN; + } else { + ctl_rec_mask |= KIRKWOOD_RECCTL_I2S_EN; + ctl_play_mask |= KIRKWOOD_PLAYCTL_I2S_EN; + } + priv->ctl_play_mask = ctl_play_mask; + priv->ctl_rec_mask = ctl_rec_mask; + + priv->soc_dai.probe = kirkwood_i2s_probe; + priv->soc_dai.ops = &kirkwood_i2s_dai_ops; + if (ctl_play_mask & (KIRKWOOD_PLAYCTL_I2S_EN | + KIRKWOOD_PLAYCTL_SPDIF_EN)) { + memcpy(&priv->soc_dai.playback, &stream, + sizeof priv->soc_dai.playback); + if (!IS_ERR(priv->extclk)) + priv->soc_dai.playback.rates = SNDRV_PCM_RATE_8000_192000 | + SNDRV_PCM_RATE_CONTINUOUS | + SNDRV_PCM_RATE_KNOT; + } + if (ctl_rec_mask & (KIRKWOOD_RECCTL_I2S_EN | + KIRKWOOD_RECCTL_SPDIF_EN)) { + memcpy(&priv->soc_dai.capture, &stream, + sizeof priv->soc_dai.capture); + if (!IS_ERR(priv->extclk)) + priv->soc_dai.capture.rates = SNDRV_PCM_RATE_8000_192000 | + SNDRV_PCM_RATE_CONTINUOUS | + SNDRV_PCM_RATE_KNOT; + } -static const struct snd_soc_component_driver kirkwood_i2s_component = { - .name = DRV_NAME, -}; + ret = snd_soc_register_component(dev, &kirkwood_i2s_component, + &priv->soc_dai, 1); + if (ret) + dev_err(dev, "snd_soc_register_component failed\n"); + return ret; +} 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; struct device_node *np = pdev->dev.of_node; @@ -503,7 +533,6 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) } else { dev_info(&pdev->dev, "found external clock\n"); clk_prepare_enable(priv->extclk); - soc_dai = &kirkwood_i2s_dai_extclk; } } @@ -520,12 +549,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_128; } - err = snd_soc_register_component(&pdev->dev, &kirkwood_i2s_component, - soc_dai, 1); - if (err) { - dev_err(&pdev->dev, "snd_soc_register_component failed\n"); + err = kirkwood_register_component(&pdev->dev, priv); + if (err) goto err_component; - } err = snd_soc_register_platform(&pdev->dev, &kirkwood_soc_platform); if (err) { diff --git a/sound/soc/kirkwood/kirkwood.h b/sound/soc/kirkwood/kirkwood.h index f8e1ccc..8f11e03 100644 --- a/sound/soc/kirkwood/kirkwood.h +++ b/sound/soc/kirkwood/kirkwood.h @@ -134,6 +134,8 @@ struct kirkwood_dma_data { struct clk *extclk; uint32_t ctl_play; uint32_t ctl_rec; + uint32_t ctl_play_mask, ctl_rec_mask; + struct snd_soc_dai_driver soc_dai; struct snd_pcm_substream *substream_play; struct snd_pcm_substream *substream_rec; int irq;
This patch adds S/PDIF input/output for mvebu DT boards. Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- Documentation/devicetree/bindings/sound/mvebu-audio.txt | 23 +++++ sound/soc/kirkwood/kirkwood-i2s.c | 128 ++++++++++++++++---------- sound/soc/kirkwood/kirkwood.h | 2 + 3 files changed, 102 insertions(+), 51 deletions(-)