This patch adds DT support to the audio subsystem of the mvebu family
(Kirkwood, Dove, Armada 370).
Signed-off-by: Jean-Francois Moine <[email protected]>
---
.../devicetree/bindings/sound/mvebu-audio.txt | 29 ++++++++++++++++++++++
sound/soc/kirkwood/kirkwood-i2s.c | 26 +++++++++++++++++-----
2 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
new file mode 100644
index 0000000..7e5fd37
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
@@ -0,0 +1,29 @@
+* mvebu (Kirkwood, Dove, Armada 370) audio controller
+
+Required properties:
+
+- compatible: "mrvl,mvebu-audio"
+
+- reg: physical base address of the controller and length of memory mapped
+ region.
+
+- interrupts: list of two irq numbers.
+ The first irq is used for data flow and the second one is used for errors.
+
+- clocks: one or two phandles.
+ The first one is mandatory and defines the internal clock.
+ The second one is optional and defines an external clock.
+
+- clock-names: names associated to the clocks:
+ "internal" for the internal clock
+ "extclk" for the external clock
+
+Example:
+
+i2s1: audio-controller@b4000 {
+ compatible = "mrvl,mvebu-audio";
+ reg = <0xb4000 0x2210>;
+ interrupts = <21>, <22>;
+ clocks = <&gate_clk 13>;
+ clock-names = "internal";
+};
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c.next b/sound/soc/kirkwood/kirkwood-i2s.c
index e5f3f7a..a4170b4 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c.next
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -22,6 +22,8 @@
#include <sound/pcm_params.h>
#include <sound/soc.h>
#include <linux/platform_data/asoc-kirkwood.h>
+#include <linux/of.h>
+
#include "kirkwood.h"
#define DRV_NAME "mvebu-audio"
@@ -453,6 +455,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
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;
int err;
priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -473,14 +476,16 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
return -ENXIO;
}
- if (!data) {
- dev_err(&pdev->dev, "no platform data ?!\n");
+ if (np) {
+ priv->burst = 128; /* might be 32 or 128 */
+ } else if (data) {
+ priv->burst = data->burst;
+ } else {
+ dev_err(&pdev->dev, "no DT nor platform data ?!\n");
return -EINVAL;
}
- priv->burst = data->burst;
-
- priv->clk = devm_clk_get(&pdev->dev, NULL);
+ priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);
if (IS_ERR(priv->clk)) {
dev_err(&pdev->dev, "no clock\n");
return PTR_ERR(priv->clk);
@@ -507,7 +512,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
priv->ctl_rec = KIRKWOOD_RECCTL_SIZE_24;
/* Select the burst size */
- if (data->burst == 32) {
+ if (priv->burst == 32) {
priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_32;
priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_32;
} else {
@@ -552,12 +557,21 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_OF
+static struct of_device_id kirkwood_i2s_of_match[] = {
+ { .compatible = "mrvl,mvebu-audio" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, kirkwood_i2s_of_match);
+#endif
+
static struct platform_driver kirkwood_i2s_driver = {
.probe = kirkwood_i2s_dev_probe,
.remove = kirkwood_i2s_dev_remove,
.driver = {
.name = DRV_NAME,
.owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(kirkwood_i2s_of_match),
},
};
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
On 08/08/2013 01:22 PM, Jean-Francois Moine wrote:
> This patch adds DT support to the audio subsystem of the mvebu family
> (Kirkwood, Dove, Armada 370).
>
> Signed-off-by: Jean-Francois Moine <[email protected]>
> ---
> .../devicetree/bindings/sound/mvebu-audio.txt | 29 ++++++++++++++++++++++
> sound/soc/kirkwood/kirkwood-i2s.c | 26 +++++++++++++++++-----
> 2 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
> new file mode 100644
> index 0000000..7e5fd37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
> @@ -0,0 +1,29 @@
> +* mvebu (Kirkwood, Dove, Armada 370) audio controller
> +
> +Required properties:
> +
> +- compatible: "mrvl,mvebu-audio"
Jean-Francois,
we need at least two more compatibles for the audio controller found on
Dove and Kirkwood respectively. This is how we are going to distinguish
those two, e.g. Kirkwood has SPDIF in which Dove hasn't.
Also, we have used "marvell" as prefix for a long time. I know there has
been discussion about the stock ticker appreviation, has there been any
decision on that already?
> +- reg: physical base address of the controller and length of memory mapped
> + region.
> +
> +- interrupts: list of two irq numbers.
> + The first irq is used for data flow and the second one is used for errors.
> +
> +- clocks: one or two phandles.
> + The first one is mandatory and defines the internal clock.
> + The second one is optional and defines an external clock.
> +
> +- clock-names: names associated to the clocks:
> + "internal" for the internal clock
s/internal/dcoclk/
> + "extclk" for the external clock
> +
> +Example:
> +
> +i2s1: audio-controller@b4000 {
> + compatible = "mrvl,mvebu-audio";
> + reg = <0xb4000 0x2210>;
> + interrupts = <21>, <22>;
> + clocks = <&gate_clk 13>;
> + clock-names = "internal";
> +};
Also we will need some phandle reference to the audio codec here. As
this property is ongoing work in ASoC core, I suggest we wait for it
and propose a binding afterwards.
Sebastian
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c.next b/sound/soc/kirkwood/kirkwood-i2s.c
> index e5f3f7a..a4170b4 100644
> --- a/sound/soc/kirkwood/kirkwood-i2s.c.next
> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> @@ -22,6 +22,8 @@
> #include <sound/pcm_params.h>
> #include <sound/soc.h>
> #include <linux/platform_data/asoc-kirkwood.h>
> +#include <linux/of.h>
> +
> #include "kirkwood.h"
>
> #define DRV_NAME "mvebu-audio"
> @@ -453,6 +455,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
> 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;
> int err;
>
> priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> @@ -473,14 +476,16 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
> return -ENXIO;
> }
>
> - if (!data) {
> - dev_err(&pdev->dev, "no platform data ?!\n");
> + if (np) {
> + priv->burst = 128; /* might be 32 or 128 */
> + } else if (data) {
> + priv->burst = data->burst;
> + } else {
> + dev_err(&pdev->dev, "no DT nor platform data ?!\n");
> return -EINVAL;
> }
>
> - priv->burst = data->burst;
> -
> - priv->clk = devm_clk_get(&pdev->dev, NULL);
> + priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);
> if (IS_ERR(priv->clk)) {
> dev_err(&pdev->dev, "no clock\n");
> return PTR_ERR(priv->clk);
> @@ -507,7 +512,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
> priv->ctl_rec = KIRKWOOD_RECCTL_SIZE_24;
>
> /* Select the burst size */
> - if (data->burst == 32) {
> + if (priv->burst == 32) {
> priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_32;
> priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_32;
> } else {
> @@ -552,12 +557,21 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_OF
> +static struct of_device_id kirkwood_i2s_of_match[] = {
> + { .compatible = "mrvl,mvebu-audio" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, kirkwood_i2s_of_match);
> +#endif
> +
> static struct platform_driver kirkwood_i2s_driver = {
> .probe = kirkwood_i2s_dev_probe,
> .remove = kirkwood_i2s_dev_remove,
> .driver = {
> .name = DRV_NAME,
> .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(kirkwood_i2s_of_match),
> },
> };
>
>
On Fri, 09 Aug 2013 10:23:50 +0200
Sebastian Hesselbarth <[email protected]> wrote:
> On 08/08/2013 01:22 PM, Jean-Francois Moine wrote:
> > This patch adds DT support to the audio subsystem of the mvebu family
> > (Kirkwood, Dove, Armada 370).
> >
> > Signed-off-by: Jean-Francois Moine <[email protected]>
> > ---
> > .../devicetree/bindings/sound/mvebu-audio.txt | 29 ++++++++++++++++++++++
> > sound/soc/kirkwood/kirkwood-i2s.c | 26 +++++++++++++++++-----
> > 2 files changed, 49 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
> > new file mode 100644
> > index 0000000..7e5fd37
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
> > @@ -0,0 +1,29 @@
> > +* mvebu (Kirkwood, Dove, Armada 370) audio controller
> > +
> > +Required properties:
> > +
> > +- compatible: "mrvl,mvebu-audio"
>
> Jean-Francois,
>
> we need at least two more compatibles for the audio controller found on
> Dove and Kirkwood respectively. This is how we are going to distinguish
> those two, e.g. Kirkwood has SPDIF in which Dove hasn't.
Sebastian,
s/has/hasn't & s/hasn't/has
Are 2 compatibles enough, i.e. "mvebu-audio" and "mbevu-audio-spdif"?
> Also, we have used "marvell" as prefix for a long time. I know there has
> been discussion about the stock ticker appreviation, has there been any
> decision on that already?
Don't know, but, sure, there are still a lot of "mrvl".
> > +- reg: physical base address of the controller and length of memory mapped
> > + region.
> > +
> > +- interrupts: list of two irq numbers.
> > + The first irq is used for data flow and the second one is used for errors.
> > +
> > +- clocks: one or two phandles.
> > + The first one is mandatory and defines the internal clock.
> > + The second one is optional and defines an external clock.
> > +
> > +- clock-names: names associated to the clocks:
> > + "internal" for the internal clock
>
> s/internal/dcoclk/
Once, I got this message:
On Fri, 26 Jul 2013 10:21:56 +0100 Russell King - ARM Linux <[email protected]> wrote:
> On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote:
> > The A510 documentation uses the names "DCO PLL" for the internal clock
> > and "AU_EXTCLK" for the external clock. So, what about "dcopll" and
> > "extclk"?
>
> Stop naming them according to their source. Their _consumer_ names
> not _source_ names.
But Russell did not tell clearly which name could be the best.
BTW, as we are naming the clocks, the 'clk' in "xxxclk" seems redondant...
> > + "extclk" for the external clock
> > +
> > +Example:
> > +
> > +i2s1: audio-controller@b4000 {
> > + compatible = "mrvl,mvebu-audio";
> > + reg = <0xb4000 0x2210>;
> > + interrupts = <21>, <22>;
> > + clocks = <&gate_clk 13>;
> > + clock-names = "internal";
> > +};
>
> Also we will need some phandle reference to the audio codec here. As
> this property is ongoing work in ASoC core, I suggest we wait for it
> and propose a binding afterwards.
I don't think that we need any reference to the codec here. The glue is
done by the audio device. For example, using the (soon?) extended
simple audio card:
spdif: spdif {
compatible = "linux,spdif-dit";
};
sound {
compatible = "linux,simple-audio";
audio-controller = <&i2s1>;
audio-codec = <&spdif>;
codec-dai-name = "dit-hifi";
};
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
On Fri, Aug 09, 2013 at 10:23:50AM +0200, Sebastian Hesselbarth wrote:
> On 08/08/2013 01:22 PM, Jean-Francois Moine wrote:
> >+i2s1: audio-controller@b4000 {
> >+ compatible = "mrvl,mvebu-audio";
> >+ reg = <0xb4000 0x2210>;
> >+ interrupts = <21>, <22>;
> >+ clocks = <&gate_clk 13>;
> >+ clock-names = "internal";
> >+};
> Also we will need some phandle reference to the audio codec here. As
> this property is ongoing work in ASoC core, I suggest we wait for it
> and propose a binding afterwards.
No, as discussed this should be in the binding for the audio subsystem
not in the binding for an individual component in that subsystem.
On Fri, Aug 09, 2013 at 11:06:23AM +0200, Jean-Francois Moine wrote:
> On Fri, 09 Aug 2013 10:23:50 +0200
> Sebastian Hesselbarth <[email protected]> wrote:
> > we need at least two more compatibles for the audio controller found on
> > Dove and Kirkwood respectively. This is how we are going to distinguish
> > those two, e.g. Kirkwood has SPDIF in which Dove hasn't.
>
> Sebastian,
>
> s/has/hasn't & s/hasn't/has
No, you're both wrong.
Some Kirkwood has SPDIF recording and playback. There are those audio
blocks which have just I2S capture and playback. There are those which
have I2S capture and playback, and SPDIF playback. There are those which
have both I2S and SPDIF for both capture and playback. The only one I
haven't yet seen is SPDIF capture without playback.
> On Fri, 26 Jul 2013 10:21:56 +0100 Russell King - ARM Linux <[email protected]> wrote:
> > On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote:
> > > The A510 documentation uses the names "DCO PLL" for the internal clock
> > > and "AU_EXTCLK" for the external clock. So, what about "dcopll" and
> > > "extclk"?
> >
> > Stop naming them according to their source. Their _consumer_ names
> > not _source_ names.
>
> But Russell did not tell clearly which name could be the best.
>
> BTW, as we are naming the clocks, the 'clk' in "xxxclk" seems redondant...
"extclk" follows what's in the data sheet - the exact name of it is
"AU_EXTCLK". Since we already know that this is the audio unit from
the struct device, the "AU_" part is redundant. The input name for
the internal clock is not given, instead they talk about where it
comes from (it's producer) so your choice of "internal" is fine.
Take a moment to think about it: if you call it "dcoclk" then what
happens on a future SoC if it's not connected to the dcoclk anymore?
> I don't think that we need any reference to the codec here. The glue is
> done by the audio device. For example, using the (soon?) extended
> simple audio card:
>
> spdif: spdif {
> compatible = "linux,spdif-dit";
> };
> sound {
> compatible = "linux,simple-audio";
> audio-controller = <&i2s1>;
> audio-codec = <&spdif>;
> codec-dai-name = "dit-hifi";
> };
As I understand the way DPCM works, that isn't going to work for this
case. For DPCM as per Liam's example, it's going to require the audio
controller to be bound to the dummy codec, and a dummy platform/dai
bound to the SPDIF codec. You then use DAPM routing to connect the
SPDIF codec to the appropriate CPU DAI output stream.
So the above "simple" implementation won't do - it can't be used to
specify two DAI links, and it can't specify the DAPM routing between
them.
This will be another challenge to solve in DT!
On 08/09/13 11:19, Mark Brown wrote:
> On Fri, Aug 09, 2013 at 10:23:50AM +0200, Sebastian Hesselbarth wrote:
>> On 08/08/2013 01:22 PM, Jean-Francois Moine wrote:
>
>>> +i2s1: audio-controller@b4000 {
>>> + compatible = "mrvl,mvebu-audio";
>>> + reg = <0xb4000 0x2210>;
>>> + interrupts = <21>, <22>;
>>> + clocks = <&gate_clk 13>;
>>> + clock-names = "internal";
>>> +};
>
>> Also we will need some phandle reference to the audio codec here. As
>> this property is ongoing work in ASoC core, I suggest we wait for it
>> and propose a binding afterwards.
>
> No, as discussed this should be in the binding for the audio subsystem
> not in the binding for an individual component in that subsystem.
>
Mark,
I do understand there may be SoCs requiring sophisticated extra audio
nodes, but Marvell SoCs don't. I prefer having a single node for the
i2s controller *and* exploit the audio subsystem properties from that.
For Marvell audio, we only need a single node for all three ASoC
drivers. No other subsystem _requires_ you to have extra nodes for
it's needs. If you can provide interrupts, just have an interrupt-
controller property. If you can provide clocks, you can link to that
very node - no virtual device node required. Even for media they
do not insist on a virtual node but they do have generic properties
you can exploit.
If you insist on creating a virtual sound card node just because
ASoC wants it that way - okay, your call. But I don't see any value
in that.
Sebastian
On Fri, Aug 09, 2013 at 11:34:30AM +0200, Sebastian Hesselbarth wrote:
> Mark,
>
> I do understand there may be SoCs requiring sophisticated extra audio
> nodes, but Marvell SoCs don't. I prefer having a single node for the
> i2s controller *and* exploit the audio subsystem properties from that.
>
> For Marvell audio, we only need a single node for all three ASoC
> drivers. No other subsystem _requires_ you to have extra nodes for
> it's needs. If you can provide interrupts, just have an interrupt-
> controller property. If you can provide clocks, you can link to that
> very node - no virtual device node required. Even for media they
> do not insist on a virtual node but they do have generic properties
> you can exploit.
Certainly from the "DT is a hardware description" you are completely
correct. The audio controller _should_ link directly to the codec,
because that's how the hardware is wired. Remember though that there
are two outputs from the audio controller (please, call it audio
controller, not I2S controller) so you need to specify which of those
two outputs the "codec" is connected to.
I would say that's required irrespective of whether or not we have a
virtual node to aggregate the stuff together for ASoC's purposes (which
should not be part of the DT hardware description - it can be part of
the "software" configuration though.)
We may be able to have kirkwood-i2s.c (which I plan to rename to
mvebu-audio.c) automatically generate the snd_soc_card stuff from the
audio controller node. Given that we need a more complex description
than the "simple" thing for DPCM, that might be overall the best
solution in any case (maybe calling out to a library which can be
shared between CPU DAI drivers to do this.)
Note that we do have another case not yet in tree, which is DRM, but
this case is different from that, because ASoC can cope with components
with independent initialisation.
On 08/09/2013 11:34 AM, Sebastian Hesselbarth wrote:
> On 08/09/13 11:19, Mark Brown wrote:
>> On Fri, Aug 09, 2013 at 10:23:50AM +0200, Sebastian Hesselbarth wrote:
>>> On 08/08/2013 01:22 PM, Jean-Francois Moine wrote:
>>
>>>> +i2s1: audio-controller@b4000 {
>>>> + compatible = "mrvl,mvebu-audio";
>>>> + reg = <0xb4000 0x2210>;
>>>> + interrupts = <21>, <22>;
>>>> + clocks = <&gate_clk 13>;
>>>> + clock-names = "internal";
>>>> +};
>>
>>> Also we will need some phandle reference to the audio codec here. As
>>> this property is ongoing work in ASoC core, I suggest we wait for it
>>> and propose a binding afterwards.
>>
>> No, as discussed this should be in the binding for the audio subsystem
>> not in the binding for an individual component in that subsystem.
>>
>
> Mark,
>
> I do understand there may be SoCs requiring sophisticated extra audio
> nodes, but Marvell SoCs don't. I prefer having a single node for the
> i2s controller *and* exploit the audio subsystem properties from that.
It's not about SoCs, it's about the board. The audio fabric on a board can
easily get complex enough to require its own driver. Speakers, mics, jacks
and jack detection, external amplifiers, bluetooth, baseband, multiple
CODECs. That's what the audio node describes.
- Lars
On Fri, Aug 09, 2013 at 12:05:58PM +0200, Lars-Peter Clausen wrote:
> On 08/09/2013 11:34 AM, Sebastian Hesselbarth wrote:
> > I do understand there may be SoCs requiring sophisticated extra audio
> > nodes, but Marvell SoCs don't. I prefer having a single node for the
> > i2s controller *and* exploit the audio subsystem properties from that.
> It's not about SoCs, it's about the board. The audio fabric on a board can
> easily get complex enough to require its own driver. Speakers, mics, jacks
> and jack detection, external amplifiers, bluetooth, baseband, multiple
> CODECs. That's what the audio node describes.
Exactly - as I said earlier on this week the issue is that in many cases
there is enough going on in audio system design to make the PCB an
interesting bit of hardware that's worth describing in the device tree.
To repeat what I said before *please* go back and check the list
archives, we've been through this several times.
On Fri, 9 Aug 2013 10:43:40 +0100
Russell King - ARM Linux <[email protected]> wrote:
> Note that we do have another case not yet in tree, which is DRM, but
> this case is different from that, because ASoC can cope with components
> with independent initialisation.
No sure!
Sometimes, with the tda998x in HDMI mode, I get a black screen on my TV
set. And, surprise, when I start the audio (playing something with
vlc), the image appears!
I dumped all (known) registers in the audio and video subsystems
(tda998x, dove lcd, kirkwood audio), and there is no difference between
normal display and black screen. So, some audio event should interact
with the HDMI display.
I don't think that the problem comes from the TV set, because, when
there is a black screen, turning the TV off/on never makes it display
normally again. Rebooting the cubox also lets the black screen.
Only powering off/on the cubox could change the display back to normal.
Now, playing some audio stream gives me immediately a normal display.
Any idea?
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
On 08/09/13 11:43, Russell King - ARM Linux wrote:
> On Fri, Aug 09, 2013 at 11:34:30AM +0200, Sebastian Hesselbarth wrote:
>> I do understand there may be SoCs requiring sophisticated extra audio
>> nodes, but Marvell SoCs don't. I prefer having a single node for the
>> i2s controller *and* exploit the audio subsystem properties from that.
>>
>> For Marvell audio, we only need a single node for all three ASoC
>> drivers. No other subsystem _requires_ you to have extra nodes for
>> it's needs. If you can provide interrupts, just have an interrupt-
>> controller property. If you can provide clocks, you can link to that
>> very node - no virtual device node required. Even for media they
>> do not insist on a virtual node but they do have generic properties
>> you can exploit.
>
> Certainly from the "DT is a hardware description" you are completely
> correct. The audio controller _should_ link directly to the codec,
> because that's how the hardware is wired. Remember though that there
> are two outputs from the audio controller (please, call it audio
> controller, not I2S controller) so you need to specify which of those
> two outputs the "codec" is connected to.
Exactly, we can solve that with multiple phandle/args audio-codecs
property and audio-codec-names (or whatever property names are more
appropriate). That is the common set of properties, I am talking about.
But the properties are totally unrelated to what nodes you put them
into.
> I would say that's required irrespective of whether or not we have a
> virtual node to aggregate the stuff together for ASoC's purposes (which
> should not be part of the DT hardware description - it can be part of
> the "software" configuration though.)
And that is *the only thing* that keeps bugging me in Mark's replies -
he *insists* on having that virtual audio nodes. I have nothing against
it, except it should be *required* for every DT we have. DRM doesn't
_need_ it, media doesn't _need_ it, but audio is so very special that it
_requires_ you to have it described in DT?
I understand that it may be required on some boards, especially if you
create different sound-cards out of the IP available. Just like the
DRM discussion we had - have a virtual node if there is no other sane
way to describe it, but there is no strict requirement.
Anyway, Mark keeps insisting on it, I'll obey.
> We may be able to have kirkwood-i2s.c (which I plan to rename to
> mvebu-audio.c) automatically generate the snd_soc_card stuff from the
> audio controller node. Given that we need a more complex description
> than the "simple" thing for DPCM, that might be overall the best
> solution in any case (maybe calling out to a library which can be
> shared between CPU DAI drivers to do this.)
That is what I am doing on top of the audio-controller node and except
that there is no helper to determine the names, yet. If ASoC would
provide a snd_soc_simple_card_register_from_dt(...,device_node *), I
wouldn't even have to parse the properties myself.
> Note that we do have another case not yet in tree, which is DRM, but
> this case is different from that, because ASoC can cope with components
> with independent initialisation.
True, and those should also probe themselves independent of the
corresponding lcd driver. Now that we have the discussion on ASoC, that
will also allow us to have an audio codec driver for the TDA998x audio
part. We need that anyway to create correct AIF packets someday.
Sebastian
On Fri, Aug 09, 2013 at 01:01:24PM +0200, Sebastian Hesselbarth wrote:
> And that is *the only thing* that keeps bugging me in Mark's replies -
> he *insists* on having that virtual audio nodes. I have nothing against
> it, except it should be *required* for every DT we have. DRM doesn't
> _need_ it, media doesn't _need_ it, but audio is so very special that it
> _requires_ you to have it described in DT?
> I understand that it may be required on some boards, especially if you
> create different sound-cards out of the IP available. Just like the
> DRM discussion we had - have a virtual node if there is no other sane
> way to describe it, but there is no strict requirement.
The problem here is that the extra effort comes from the board, not from
the individual components - any component would have to optionally
support having a card binding hanging off it (or the properties for a
simple link) so it's more consistent just to say we link everything from
a board node all the time. By the time you start adding all the things
like jacks and connected pins I'd expect you'd find you'd want to have
at least a sub node to organise everything.
Even things like buses aren't that clear - an I2S link can be a
fairly simple point to point link but it can also be a multi-master bus
or share some signals with other links. Things like DRM generally have
interconnects which are much more regular and less open to interesting
board design decisions, though even for media the last time I looked at
the bindings the individual links were getting DT nodes which is another
way to try to deal with things.
Let me once more renew my request that you go and read the previous
discussions on this stuff, we've been through this loop repeatedly.
> That is what I am doing on top of the audio-controller node and except
> that there is no helper to determine the names, yet. If ASoC would
> provide a snd_soc_simple_card_register_from_dt(...,device_node *), I
> wouldn't even have to parse the properties myself.
So extend Morimoto-san's work on the simple card for this - that's what
it's there for, it's doing exactly this job for non-DT systems but it
just didn't get DT support added yet. All the trivial cards should end
up using this.
On Fri, Aug 09, 2013 at 12:39:40PM +0100, Mark Brown wrote:
> So extend Morimoto-san's work on the simple card for this - that's what
> it's there for, it's doing exactly this job for non-DT systems but it
> just didn't get DT support added yet. All the trivial cards should end
> up using this.
It's quite rediculous to request that the simple card stuff is expanded at
this time, when you're also telling us that we must use DPCM for Kirkwood,
but DPCM is not yet in a working state in mainline. We don't yet know
whether that's because of something the core is doing wrong or whether
that's because something that I'm doing wrong. The reason we don't know
that is because the _only_ person who knows anything about this is Liam,
and as you have said, Liam is away on vacation.
All that we presently know is that DPCM is supposed to be "something like
this". However, when we do that, it doesn't work, which means we can't
be certain what the end result is supposed to look like. It may be that
Liam has changed some of the design decisions with DPCM.
We know that various things are missing - like the .pcm_playback member in
the DAI link. That points to Liam having additional patches to the core
code in his tree to make DPCM work. That could impact our scenario here,
and how the "simple" card should be implemented.
So, what you're asking is for us to extend the design of something in a
way that we know very little about how to make work.
To that I say: No. Let's wait for Liam to return from vacation so that
this can be sorted out properly with someone who knows this code.
On Fri, Aug 09, 2013 at 02:09:32PM +0100, Russell King - ARM Linux wrote:
> On Fri, Aug 09, 2013 at 12:39:40PM +0100, Mark Brown wrote:
> > So extend Morimoto-san's work on the simple card for this - that's what
> > it's there for, it's doing exactly this job for non-DT systems but it
> > just didn't get DT support added yet. All the trivial cards should end
> > up using this.
> It's quite rediculous to request that the simple card stuff is expanded at
> this time, when you're also telling us that we must use DPCM for Kirkwood,
That's the place to put the sort of shared infrastructure for trivial
cards that Sebastian said he wanted to see - if you guys are saying that
the machines should all be able to use a trivial binding with shared
code that's where that code should be.
On Fri, Aug 09, 2013 at 07:00:58PM +0100, Mark Brown wrote:
> On Fri, Aug 09, 2013 at 02:09:32PM +0100, Russell King - ARM Linux wrote:
> > On Fri, Aug 09, 2013 at 12:39:40PM +0100, Mark Brown wrote:
>
> > > So extend Morimoto-san's work on the simple card for this - that's what
> > > it's there for, it's doing exactly this job for non-DT systems but it
> > > just didn't get DT support added yet. All the trivial cards should end
> > > up using this.
>
> > It's quite rediculous to request that the simple card stuff is expanded at
> > this time, when you're also telling us that we must use DPCM for Kirkwood,
>
> That's the place to put the sort of shared infrastructure for trivial
> cards that Sebastian said he wanted to see - if you guys are saying that
> the machines should all be able to use a trivial binding with shared
> code that's where that code should be.
Sigh, you completely miss the point.
What all three of us are ultimately after is a DT description for the
kirkwood stuff which covers all its use cases. The use case which all
three of us have in common is the Cubox, which is the one which needs
the spdif stuff to work.
Now, what you've said to date is:
1. you want kirkwood to use DPCM.
2. you want kirkwood using people to use the simple card stuff with the
kirkwood driver you want to use DPCM.
To make it work with DPCM, we first need to know what DPCM requires,
which means we either have to have the knowledge of DPCM and/or have a
working implementation. We don't have either of those yet.
So, I again state plainly that what you're asking is for people to come
up with a DT description for a DPCM implementation when we haven't yet
got a working DPCM implementation, even without DT.
It is this which I assert is a completely rediculous request at this
moment in time for the reasons stated in my previous email and repeated
in this email.
On Fri, Aug 09, 2013 at 07:25:16PM +0100, Russell King - ARM Linux wrote:
> Sigh, you completely miss the point.
> What all three of us are ultimately after is a DT description for the
> kirkwood stuff which covers all its use cases. The use case which all
> three of us have in common is the Cubox, which is the one which needs
> the spdif stuff to work.
I think I get what needs to be done well enough, I'm not sure that
matches up with what people are wanting the results to look like but
that's another story.
> Now, what you've said to date is:
> 1. you want kirkwood to use DPCM.
Yes.
> 2. you want kirkwood using people to use the simple card stuff with the
> kirkwood driver you want to use DPCM.
No. What I'm saying is that if your end goal is a binding for cards
that works for absolutely anything then it should be handled by the
simple card driver since that's what it's there for. There's no
requirement to do things that way though, certainly not in a first pass.
> To make it work with DPCM, we first need to know what DPCM requires,
> which means we either have to have the knowledge of DPCM and/or have a
> working implementation. We don't have either of those yet.
> So, I again state plainly that what you're asking is for people to come
> up with a DT description for a DPCM implementation when we haven't yet
> got a working DPCM implementation, even without DT.
> It is this which I assert is a completely rediculous request at this
> moment in time for the reasons stated in my previous email and repeated
> in this email.
I'm not sure I said that anyone need do anything right this very minute?
In any case since the binding that pulls everything together into the
audio subsystem is supposed to be separate to the bindings for the
bindings for individual components it should be possible to proceed with
those if someone wants to do that (as in the patch under discussion) and
add the bits to tie things together later.
If someone wants to it should also be possible to convert the existing
platforms without S/PDIF support over to DT, providing you don't mind
changing the code once the DPCM and S/PDIF support is added and a bit of
thought is put into where the S/PDIF output will fit into the bindings.
Since DPCM is a Linux internal thing it shouldn't have any impact on the
bindings.
But like I say there's no need to do anything in particular immediately.
The patch under discussion seems basically fine for what it covers,
modulo the fairly minor things Sebastian identified, and can be built on
later.
On Fri, Aug 09, 2013 at 08:44:34PM +0100, Mark Brown wrote:
> If someone wants to it should also be possible to convert the existing
> platforms without S/PDIF support over to DT, providing you don't mind
> changing the code once the DPCM and S/PDIF support is added and a bit of
> thought is put into where the S/PDIF output will fit into the bindings.
Okay, so you're thinking that the I2S output will be enabled in the
absence of DPCM? If so, that tells me that you don't understand my
patches, and this is getting *really* tiresome.
One more time:
- There are two outputs from the FIFO.
- There is an I2S output, and there is a SPDIF output.
- All hardware has an I2S output.
- Some hardware also has the SPDIF output.
- Each output is individually enable-able via separate bits.
- When both outputs are used, both must be enabled simultaneously.
- Otherwise, only one output must be enabled at any one time.
- At least one output must be enabled for there to be any activity
from the unit at all (that's obvious!)
So, what I'm doing is providing _two_ AIF connection points, one for I2S
and one for SPDIF. The appropriate AIF connection point must be attached
to for the appropriate output to be enabled.
That means that if you want to use the SPDIF output, the SPDIF AIF must
be linked to the codec. If you want to use the I2S output, the I2S AIF
must be linked to the codec.
Without this, there's no way for the CPU DAI to know which output(s) are
in use, and therefore which should be enabled.
What this means is that the conventional setup where you have _one_ DAI
link connecting the codec DAI to the CPU DAI won't work on Kirkwood
anymore, because there is no way to know which of the two outputs
should be enabled. I avoided that in my patches, but you've objected
to that saying that it must use DPCM. That makes the whole of kirkwood
entirely DPCM only, whether or not you have a single codec to connect.
Surely you realise this, because you must have read the patches properly
before you commented on them, and realised that one of spdifdo or i2sdo
must be powered up to set either enable bit?
On Fri, Aug 09, 2013 at 09:38:47PM +0100, Russell King - ARM Linux wrote:
> On Fri, Aug 09, 2013 at 08:44:34PM +0100, Mark Brown wrote:
> > If someone wants to it should also be possible to convert the existing
> > platforms without S/PDIF support over to DT, providing you don't mind
> > changing the code once the DPCM and S/PDIF support is added and a bit of
> > thought is put into where the S/PDIF output will fit into the bindings.
> Okay, so you're thinking that the I2S output will be enabled in the
> absence of DPCM? If so, that tells me that you don't understand my
When I say that it should be possible to convert the existing machines
over to DT I'm talking about the machines that are supported with the
drivers currently in mainline. These machines presumably work just fine
with the existing drivers which support the I2S only subset of the
possible functionality in the hardware without using DCPM. These are
the the machines that could have DT added now without implementing the
driver improvements to enable the extra hardware functionality.
When I say that they will need changing once DPCM and S/PDIF support is
added what I mean is that those changes to the CPU side drivers should,
as you correctly say, require all Kirkwood machine drivers to use DPCM
even if only due to the handling of simultaneous startup of the S/PDIF
and I2S interfaces.
The "you" in the above quote was intended to refer to people working on
Kirkwood in general, not you personally unless you want to.
> What this means is that the conventional setup where you have _one_ DAI
> link connecting the codec DAI to the CPU DAI won't work on Kirkwood
> anymore, because there is no way to know which of the two outputs
> should be enabled. I avoided that in my patches, but you've objected
> to that saying that it must use DPCM. That makes the whole of kirkwood
> entirely DPCM only, whether or not you have a single codec to connect.
Right, that's where the drivers should end up. In terms of the device
tree I would expect that it should be the case that there are distinct
I2S and S/PDIF interfaces visible, or at least that there's some way to
identify which interface on the SoC is being referred to - this is the
thought about where the S/PDIF output will fit into the bindings that I
mentioned being required.
So long as only I2S is used in the system (or at least any S/PDIF that
is present is ignored at runtime) it should be possible to continue
using the existing driver infrastructure until S/PDIF and DPCM support
is added and then transition the existing drivers (both non-DT and any
DT ones that get added or converted) over to that as part of the
conversion.
In other words I was talking about a possible intermediate state where
people are working with the DT bindings but the driver support for
systems with S/PDIF in use is not there yet. That's not required but it
should be possible if there's interest in progressing the DT bindings
while the driver is as it stands.
Dear Jean-Francois Moine,
On Fri, 9 Aug 2013 11:06:23 +0200, Jean-Francois Moine wrote:
> > we need at least two more compatibles for the audio controller found on
> > Dove and Kirkwood respectively. This is how we are going to distinguish
> > those two, e.g. Kirkwood has SPDIF in which Dove hasn't.
>
> Sebastian,
>
> s/has/hasn't & s/hasn't/has
>
> Are 2 compatibles enough, i.e. "mvebu-audio" and "mbevu-audio-spdif"?
Or:
marvell,kirkwood-audio
marvell,dove-audio
> > Also, we have used "marvell" as prefix for a long time. I know there has
> > been discussion about the stock ticker appreviation, has there been any
> > decision on that already?
>
> Don't know, but, sure, there are still a lot of "mrvl".
Yes, but they are being converted over to 'marvell', I've seen some
patches posted. Also, as per
Documentation/devicetree/bindings/vendor-prefixes.txt, 'marvell' is the
right prefix for Marvell compatible strings.
> > Also we will need some phandle reference to the audio codec here. As
> > this property is ongoing work in ASoC core, I suggest we wait for it
> > and propose a binding afterwards.
>
> I don't think that we need any reference to the codec here. The glue is
> done by the audio device. For example, using the (soon?) extended
> simple audio card:
Have you looked at the Samsung DT bindings for audio:
sound {
compatible = "samsung,smdk-wm8994";
samsung,i2s-controller = <&i2s0>;
samsung,audio-codec = <&wm8994>;
};
it definitely has a codec phandle reference here.
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Sat, Aug 10, 2013 at 12:42:09AM +0100, Mark Brown wrote:
> On Fri, Aug 09, 2013 at 09:38:47PM +0100, Russell King - ARM Linux wrote:
> > On Fri, Aug 09, 2013 at 08:44:34PM +0100, Mark Brown wrote:
>
> > > If someone wants to it should also be possible to convert the existing
> > > platforms without S/PDIF support over to DT, providing you don't mind
> > > changing the code once the DPCM and S/PDIF support is added and a bit of
> > > thought is put into where the S/PDIF output will fit into the bindings.
>
> > Okay, so you're thinking that the I2S output will be enabled in the
> > absence of DPCM? If so, that tells me that you don't understand my
>
> When I say that it should be possible to convert the existing machines
> over to DT I'm talking about the machines that are supported with the
> drivers currently in mainline. These machines presumably work just fine
> with the existing drivers which support the I2S only subset of the
> possible functionality in the hardware without using DCPM. These are
> the the machines that could have DT added now without implementing the
> driver improvements to enable the extra hardware functionality.
>
> When I say that they will need changing once DPCM and S/PDIF support is
> added what I mean is that those changes to the CPU side drivers should,
> as you correctly say, require all Kirkwood machine drivers to use DPCM
> even if only due to the handling of simultaneous startup of the S/PDIF
> and I2S interfaces.
>
> The "you" in the above quote was intended to refer to people working on
> Kirkwood in general, not you personally unless you want to.
>
> > What this means is that the conventional setup where you have _one_ DAI
> > link connecting the codec DAI to the CPU DAI won't work on Kirkwood
> > anymore, because there is no way to know which of the two outputs
> > should be enabled. I avoided that in my patches, but you've objected
> > to that saying that it must use DPCM. That makes the whole of kirkwood
> > entirely DPCM only, whether or not you have a single codec to connect.
>
> Right, that's where the drivers should end up. In terms of the device
> tree I would expect that it should be the case that there are distinct
> I2S and S/PDIF interfaces visible, or at least that there's some way to
> identify which interface on the SoC is being referred to - this is the
> thought about where the S/PDIF output will fit into the bindings that I
> mentioned being required.
>
> So long as only I2S is used in the system (or at least any S/PDIF that
> is present is ignored at runtime) it should be possible to continue
> using the existing driver infrastructure until S/PDIF and DPCM support
> is added and then transition the existing drivers (both non-DT and any
> DT ones that get added or converted) over to that as part of the
> conversion.
>
> In other words I was talking about a possible intermediate state where
> people are working with the DT bindings but the driver support for
> systems with S/PDIF in use is not there yet. That's not required but it
> should be possible if there's interest in progressing the DT bindings
> while the driver is as it stands.
Right, so what you're proposing is to come up with a DT description for
the existing stuff, and then have to change (or at the very least augment)
that description later when the DPCM stuff goes in.
What should be done is to sort out DPCM, get that working and then sort out
the DT side of it, so that everyone gets only one transition to DT, not a
transition to a half-baked stop-gap DT and then have to go through another
round of DT changes. "Because we can" is not a good enough reason not to
get this sorted properly.
On Sat, Aug 10, 2013 at 10:31:37AM +0100, Russell King - ARM Linux wrote:
> Right, so what you're proposing is to come up with a DT description for
> the existing stuff, and then have to change (or at the very least augment)
> that description later when the DPCM stuff goes in.
> What should be done is to sort out DPCM, get that working and then sort out
> the DT side of it, so that everyone gets only one transition to DT, not a
> transition to a half-baked stop-gap DT and then have to go through another
> round of DT changes. "Because we can" is not a good enough reason not to
> get this sorted properly.
Since we know what the hardware physically looks like we should be able
to write a DT binding which can be stable for both before and after the
DPCM transition. The bindings would have to be truly awful to require a
rewrite here and as with all the DMA wrapper drivers if parts of DPCM
that don't reflect actual hardware are appearing in the DT we're doing
something wrong.