[PATCH] ASoC: generic: add generic compound card with DT support

[PATCH] ASoC: generic: add generic compound card with DT support

[Jean-Francois Moine]

Some audio cards are built from different hardware components.
When such compound cards don't need specific code, this driver creates
them with the required DAI links and routes from a DT.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
This code was first developped on the generic simple card, but its
recent DT extension cannot be easily extended again to support compound
cards as the one in the Cubox.
Note also that the example relies on a proposed patch of mine aiming to
render the codec name / OF node optional in DAI links
(http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/070082.html).
---
 .../devicetree/bindings/sound/compound-card.txt |  95 ++++++++++++
 sound/soc/generic/Kconfig                       |   6 +
 sound/soc/generic/Makefile                      |   2 +
 sound/soc/generic/compound-card.c               | 247 +++++++++++++++++++++++++
 4 file changed, 350 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/compound-card.txt b/Documentation/devicetree/bindings/sound/compound-card.txt
new file mode 100644
index 0000000..554a796
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/compound-card.txt
@@ -0,0 +1,95 @@
+Device-Tree bindings for compound audio card
+
+Compound audio card describes the links between the different parts
+of an audio card built from different hardware components.
+
+Required properties:
+  - compatible: should be "compound-audio-card"
+  - audio-controller: phandle of the audio controller
+
+Optional properties:
+  - routes: list of couple of strings (sink, source)
+
+Required subnodes:
+  - link: DAI link subnode
+	At least one link must be specified.
+
+Required link subnode properties:
+  - link-name: names of the DAI link and of the stream
+  - cpu-dai-name: name of the CPU or CODEC DAI
+		An empty string indicates that the CPU DAI is
+		the same as the audio controller.
+  - codec-dai-name: name of the CODEC DAI
+
+Optional link subnode properties:
+  - audio-codec or codec-name: phandle or name of the CODEC
+		in case the codec-dai-name is not unique
+  - format: DAI format. One of:
+		"i2s", "right_j", "left_j" , "dsp_a"
+		"dsp_b", "ac97", "pdm", "msb", "lsb"
+  - front-end or back-end: present if the DAI link describes resp.
+		a front-end CPU DAI or a back-end CODEC DAI
+  - playback or capture: present if the DAI link is used for
+		playback or capture only
+
+Example node:
+
+The audio subsystem found in the SolidRun Cubox is composed of:
+
+- an audio controller which outputs playback streams on either one or both
+  of I2S and S/PDIF,
+
+- a HDMI transmitter which can get its input from either I2S or S/PDIF,
+
+- an optical S/PDIF connector.
+
+	sound {
+		compatible = "compound-audio-card";
+		audio-controller = <&audio1>;
+		routes =
+			"I2S Playback",   "System Playback",
+			"SPDIF Playback", "System Playback",
+			"hdmi-out",	"I2S Playback",
+			"hdmi-out",	"SPDIF Playback",
+			"spdif-out",	"SPDIF Playback";
+		link@0 {
+			link-name = "System audio";
+			front-end;
+			cpu-dai-name = "mvebu-audio";
+			codec-dai-name = "snd-soc-dummy-dai";
+			format = "i2s";
+			playback;
+		};
+		link@1 {
+			link-name = "hdmi-i2s";
+			back-end;
+			cpu-dai-name = "i2s";
+			codec-dai-name = "i2s-hifi";
+			playback;
+		};
+		link@2 {
+			link-name = "hdmi-spdif";
+			back-end;
+			cpu-dai-name = "spdif";
+			codec-dai-name = "spdif-hifi";
+			playback;
+		};
+		link@3 {
+			link-name = "spdif";
+			back-end;
+			cpu-dai-name = "spdif";
+			codec-dai-name = "dit-hifi";
+			playback;
+		};
+	};
+
+	hdmi_codec: hdmi-codec {
+		compatible = "nxp,tda998x-codec";
+		audio-ports = <0x03>, <0x04>;
+	};
+
+	spdif_codec: spdif-codec {
+		compatible = "linux,spdif-dit";
+	};
+
+
diff --git a/sound/soc/generic/Kconfig b/sound/soc/generic/Kconfig
index 610f612..f2678ae 100644
--- a/sound/soc/generic/Kconfig
+++ b/sound/soc/generic/Kconfig
@@ -2,3 +2,9 @@ config SND_SIMPLE_CARD
 	tristate "ASoC Simple sound card support"
 	help
 	  This option enables generic simple sound card support
+
+config SND_COMPOUND_CARD
+	tristate "ASoC Compound sound card support"
+	depends on OF
+	help
+	  This option enables the generic compound sound card support.
diff --git a/sound/soc/generic/Makefile b/sound/soc/generic/Makefile
index 9c3b246..434cb79 100644
--- a/sound/soc/generic/Makefile
+++ b/sound/soc/generic/Makefile
@@ -1,3 +1,5 @@
 snd-soc-simple-card-objs	:= simple-card.o
+snd-soc-compound-card-objs	:= compound-card.o
 
 obj-$(CONFIG_SND_SIMPLE_CARD)	+= snd-soc-simple-card.o
+obj-$(CONFIG_SND_COMPOUND_CARD)	+= snd-soc-compound-card.o
diff --git a/sound/soc/generic/compound-card.c b/sound/soc/generic/compound-card.c
new file mode 100644
index 0000000..f57e159
--- /dev/null
+++ b/sound/soc/generic/compound-card.c
@@ -0,0 +1,247 @@
+/*
+ * ASoC compound sound card support
+ *
+ * Copyright (C) 2013 Jean-Francois Moine <moinejf@free.fr>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <sound/soc.h>
+
+// link types
+#define LINK_FE 0	/* front-end */
+#define LINK_BE 1	/* back-end */
+#define LINK_CC 2	/* (controller or codec) to codec */
+
+struct compound_card_data {
+	struct snd_soc_card snd_card;
+	struct snd_soc_dai_link *snd_links;
+};
+
+/* get a DAI link */
+static int get_link(struct compound_card_data *priv,
+			struct snd_soc_dai_link *link,
+			struct device_node *ctrl_np,
+			struct device_node *np)
+{
+	struct device_node *codec_np;
+	const char *cpu_dai_name;
+	int type, ret;
+
+	/* optional link-name */
+	of_property_read_string(np, "link-name", &link->name);
+	link->stream_name = link->name;
+
+	/* link type */
+	if (of_property_read_bool(np, "front-end"))
+		type = LINK_FE;
+	else if (of_property_read_bool(np, "back-end"))
+		type = LINK_BE;
+	else
+		type = LINK_CC;
+
+	/* cpu-dai-name */
+	ret = of_property_read_string(np, "cpu-dai-name",
+					&cpu_dai_name);
+	if (ret) {
+		dev_err(priv->snd_card.dev,
+			"No valid cpu-dai-name found\n");
+		return ret;
+	}
+
+	/* an empty cpu-dai-name forces the audio-controller */
+	if (cpu_dai_name[0] == '\0')
+		link->cpu_of_node = ctrl_np;
+	else
+		link->cpu_dai_name = cpu_dai_name;
+
+	/* the codec may be omitted or specified by name or phandle */
+	codec_np = of_parse_phandle(np, "audio-codec", 0);
+	if (codec_np)
+		link->codec_of_node = codec_np;
+	else
+		of_property_read_string(np, "codec-name",
+					&link->codec_name);
+
+	/* codec-dai-name */
+	ret = of_property_read_string(np, "codec-dai-name",
+					&link->codec_dai_name);
+	if (ret) {
+		dev_err(priv->snd_card.dev,
+			"Failed to parse codec-dai-name\n");
+		return ret;
+	}
+
+	if (type == LINK_BE) {
+		link->no_pcm = 1;
+		/* don't set the platform, otherwise, the platform functions
+		 * are called many times */
+	} else {
+		link->platform_of_node = ctrl_np;
+		if (type == LINK_FE)
+			link->dynamic = 1;
+	}
+
+	/* DAI format */
+	link->dai_fmt = snd_soc_of_parse_daifmt(np, NULL) &
+				SND_SOC_DAIFMT_FORMAT_MASK;
+
+	if (of_property_read_bool(np, "playback"))
+		link->playback_only = 1;
+	else if (of_property_read_bool(np, "capture"))
+		link->capture_only = 1;
+
+	return 0;
+}
+
+static int compound_card_dt_probe(struct platform_device *pdev,
+		struct compound_card_data *priv)
+{
+	struct device_node *np, *ctrl_np, *link_np;
+	struct snd_soc_dai_link *link;
+	int ret, num_links;
+
+	np = pdev->dev.of_node;
+
+	ctrl_np = of_parse_phandle(np, "audio-controller", 0);
+	if (!ctrl_np) {
+		dev_err(&pdev->dev, "No valid audio-controller phandle found\n");
+		return -EINVAL;
+	}
+
+	/* get the optional routes */
+	if (of_property_count_strings(np, "routes"))
+		snd_soc_of_parse_audio_routing(&priv->snd_card, "routes");
+
+	priv->snd_card.name = "ASoC compound sound card";
+
+	/* check the number of links */
+	num_links = 0;
+	for_each_child_of_node(np, link_np)
+		num_links++;
+
+	if (num_links == 0) {
+		dev_err(&pdev->dev, "No DAI link\n");
+		return -EINVAL;
+	}
+
+	link = devm_kzalloc(&pdev->dev,
+				sizeof(struct snd_soc_dai_link) * num_links,
+				GFP_KERNEL);
+	if (!link) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	priv->snd_links = link;
+	priv->snd_card.num_links = num_links;
+	for_each_child_of_node(np, link_np) {
+		ret = get_link(priv, link, ctrl_np, link_np);
+		if (ret)
+			goto err;
+		link++;
+	}
+
+	return 0;
+
+err:
+	if (link_np)
+		of_node_put(link_np);
+	if (ctrl_np)
+		of_node_put(ctrl_np);
+	for (num_links = 0, link = priv->snd_links;
+	     num_links < priv->snd_card.num_links;
+	     num_links++, link++) {
+		struct device_node *codec_np;
+
+		codec_np = (struct device_node *) link->codec_of_node;
+		if (!codec_np)
+			break;
+		of_node_put(codec_np);
+	}
+	return ret;
+}
+
+static int compound_card_dt_remove(struct platform_device *pdev,
+		struct compound_card_data *priv)
+{
+	struct device_node *codec_np;
+	struct snd_soc_dai_link *link;
+	int num_links;
+
+	if (!priv->snd_links)
+		return 0;
+	if (priv->snd_links->platform_of_node)
+		of_node_put((struct device_node *)
+				priv->snd_links->platform_of_node);
+	for (num_links = 0, link = priv->snd_links;
+	     num_links < priv->snd_card.num_links;
+	     num_links++, link++) {
+		codec_np = (struct device_node *) link->codec_of_node;
+		if (!codec_np)
+			break;
+		of_node_put(codec_np);
+	}
+	return 0;
+}
+
+static int compound_card_probe(struct platform_device *pdev)
+{
+	struct compound_card_data *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof *priv, GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->snd_card.owner = THIS_MODULE;
+	priv->snd_card.dev = &pdev->dev;
+
+	ret = compound_card_dt_probe(pdev, priv);
+
+	if (ret)
+		return ret;
+
+	priv->snd_card.dai_link = priv->snd_links;
+
+	dev_set_drvdata(&pdev->dev, priv);
+
+	return snd_soc_register_card(&priv->snd_card);
+}
+
+static int compound_card_remove(struct platform_device *pdev)
+{
+	struct compound_card_data *priv = dev_get_drvdata(&pdev->dev);
+ 
+	snd_soc_unregister_card(&priv->snd_card);
+
+	compound_card_dt_remove(pdev, priv);
+
+	return 0;
+}
+
+static const struct of_device_id compound_card_of_dev_ids[] = {
+	{ .compatible = "compound-audio-card", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, compound_card_of_dev_ids);
+
+static struct platform_driver compound_card = {
+	.driver = {
+		.name	= "asoc-compound-card",
+		.owner = THIS_MODULE,
+		.of_match_table = compound_card_of_dev_ids,
+	},
+	.probe		= compound_card_probe,
+	.remove		= compound_card_remove,
+};
+
+module_platform_driver(compound_card);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ASoC Compound Sound Card");
+MODULE_AUTHOR("Jean-Francois Moine <moinejf@free.fr>");

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] ASoC: generic: add generic compound card with DT support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

On Tue, Dec 31, 2013 at 11:31:38AM +0100, Jean-Francois Moine wrote:

> This code was first developped on the generic simple card, but its
> recent DT extension cannot be easily extended again to support compound
> cards as the one in the Cubox.

It would have been useful to have provided that feedback at the time
rather than waiting until after it had been merged - it was in review
for long enough.  It would also be good to articulate the issues with
the binding rather than simply stating they exist, and to consider
adding a second binding to the existing generic card rather than adding
a totally new card.

Please also remember that all DT patches need to be reviewed by the DT
people, you've not CCed either them or the list.

> +  - front-end or back-end: present if the DAI link describes resp.
> +		a front-end CPU DAI or a back-end CODEC DAI

These are Linux-internal concepts which shouldn't appear in a DT binding
or at the very least need definition.  One thing to consider here is
that these things are all about the internals of a SoC and you'd
therefore expect that they would be defined separately from the card so
as to avoid having to replicate information in every card using a given
SoC.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ASoC: generic: add generic compound card with DT support

[Jean-Francois Moine]

On Tue, 31 Dec 2013 11:59:27 +0000
Mark Brown <broonie@kernel.org> wrote:

> > This code was first developped on the generic simple card, but its
> > recent DT extension cannot be easily extended again to support compound
> > cards as the one in the Cubox.  
> 
> It would have been useful to have provided that feedback at the time
> rather than waiting until after it had been merged - it was in review
> for long enough.  It would also be good to articulate the issues with
> the binding rather than simply stating they exist, and to consider
> adding a second binding to the existing generic card rather than adding
> a totally new card.

Sorry, I spent a lot of time on DPCM, and I was not yet ready to propose
something when you accepted Kuninori's patch.

> Please also remember that all DT patches need to be reviewed by the DT
> people, you've not CCed either them or the list.
> 
> > +  - front-end or back-end: present if the DAI link describes resp.
> > +		a front-end CPU DAI or a back-end CODEC DAI  
> 
> These are Linux-internal concepts which shouldn't appear in a DT binding
> or at the very least need definition.  One thing to consider here is
> that these things are all about the internals of a SoC and you'd
> therefore expect that they would be defined separately from the card so
> as to avoid having to replicate information in every card using a given
> SoC.

Do you mean that, as DPCM cannot be in the DT, there should be a
specific driver for the Cubox audio card (Marvell Armada 510 + NXP HDMI
transmitter)?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] ASoC: generic: add generic compound card with DT support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1741 bytes --]

On Tue, Dec 31, 2013 at 01:36:10PM +0100, Jean-Francois Moine wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > It would have been useful to have provided that feedback at the time
> > rather than waiting until after it had been merged - it was in review
> > for long enough.  It would also be good to articulate the issues with

> Sorry, I spent a lot of time on DPCM, and I was not yet ready to propose
> something when you accepted Kuninori's patch.

It'd still have been worthwhile to say that you weren't sure it'd work
even if you didn't have an alternative; you could perhaps have worked on
it together.

> > These are Linux-internal concepts which shouldn't appear in a DT binding
> > or at the very least need definition.  One thing to consider here is
> > that these things are all about the internals of a SoC and you'd
> > therefore expect that they would be defined separately from the card so
> > as to avoid having to replicate information in every card using a given
> > SoC.

> Do you mean that, as DPCM cannot be in the DT, there should be a
> specific driver for the Cubox audio card (Marvell Armada 510 + NXP HDMI
> transmitter)?

I'm not saying it can't be in the DT, I'm saying that this doesn't look
like the right way to do things.  For example we could have a way of
specifying parts of the card separately so the SoC can be referenced as
a whole by the card, or we could have the internals behind the DAI
hidden from the DT entirely so the DT just links the DAIs together and
the SoC internals come from the implementation of the DAI devices.

If you can't figure something out a more specific binding is fine, but
if you're trying to do a generic binding for everything then these
things need to be considered.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Lars-Peter Clausen]

On 12/31/2013 11:31 AM, Jean-Francois Moine wrote:
> Some audio cards are built from different hardware components.
> When such compound cards don't need specific code, this driver creates
> them with the required DAI links and routes from a DT.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
> This code was first developped on the generic simple card, but its
> recent DT extension cannot be easily extended again to support compound
> cards as the one in the Cubox.
> Note also that the example relies on a proposed patch of mine aiming to
> render the codec name / OF node optional in DAI links
> (http://mailman.alsa-project.org/pipermail/alsa-devel/2013-December/070082.html).
> ---
>  .../devicetree/bindings/sound/compound-card.txt |  95 ++++++++++++
>  sound/soc/generic/Kconfig                       |   6 +
>  sound/soc/generic/Makefile                      |   2 +
>  sound/soc/generic/compound-card.c               | 247 +++++++++++++++++++++++++
>  4 file changed, 350 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/compound-card.txt b/Documentation/devicetree/bindings/sound/compound-card.txt
> new file mode 100644
> index 0000000..554a796
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/compound-card.txt
> @@ -0,0 +1,95 @@
> +Device-Tree bindings for compound audio card
> +
> +Compound audio card describes the links between the different parts
> +of an audio card built from different hardware components.
> +
> +Required properties:
> +  - compatible: should be "compound-audio-card"
> +  - audio-controller: phandle of the audio controller
> +
> +Optional properties:
> +  - routes: list of couple of strings (sink, source)
> +
> +Required subnodes:
> +  - link: DAI link subnode
> +	At least one link must be specified.
> +
> +Required link subnode properties:
> +  - link-name: names of the DAI link and of the stream
> +  - cpu-dai-name: name of the CPU or CODEC DAI
> +		An empty string indicates that the CPU DAI is
> +		the same as the audio controller.
> +  - codec-dai-name: name of the CODEC DAI
> +
> +Optional link subnode properties:
> +  - audio-codec or codec-name: phandle or name of the CODEC
> +		in case the codec-dai-name is not unique
> +  - format: DAI format. One of:
> +		"i2s", "right_j", "left_j" , "dsp_a"
> +		"dsp_b", "ac97", "pdm", "msb", "lsb"
> +  - front-end or back-end: present if the DAI link describes resp.
> +		a front-end CPU DAI or a back-end CODEC DAI
> +  - playback or capture: present if the DAI link is used for
> +		playback or capture only


As Mark also said, this binding definitely leaks way too much internals of
the current ASoC implementation. In my opinion the way forward for ASoC is
to stop to distinguish between different types of components. This is on one
hand CODECS and CPU-DAIs and on the other hand also front-end and beck-end
DAIs. The first steps in this direction have already been take by the start
of the component-fication, but its still a long way to go. Exposing those
concepts via the devicetree will only make it harder to get rid of them
later. The bindings for a compound card should essentially describe which
components are involved and how the fabric between and around them looks
like. If the type of the component is needed in the ASoC implementation it
should be possible to auto-discover it. Also I think we want to align the
devicetree bindings with what the media people have been doing[1]. Audio and
video are not that different in this regard and there will also be boards
where the audio and video fabric will be intermingled  (e.g. like on your
board with HDMI).

- Lars

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Jean-Francois Moine]

On Wed, 01 Jan 2014 20:05:05 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> As Mark also said, this binding definitely leaks way too much internals of
> the current ASoC implementation. In my opinion the way forward for ASoC is
> to stop to distinguish between different types of components. This is on one
> hand CODECS and CPU-DAIs and on the other hand also front-end and beck-end
> DAIs. The first steps in this direction have already been take by the start
> of the component-fication, but its still a long way to go. Exposing those
> concepts via the devicetree will only make it harder to get rid of them
> later. The bindings for a compound card should essentially describe which
> components are involved and how the fabric between and around them looks
> like. If the type of the component is needed in the ASoC implementation it
> should be possible to auto-discover it. Also I think we want to align the
> devicetree bindings with what the media people have been doing[1].

(you forgot the [1] reference)

> Audio and
> video are not that different in this regard and there will also be boards
> where the audio and video fabric will be intermingled  (e.g. like on your
> board with HDMI).

I found a way to discover the DAI link types for my system: when simple
DAPM, the kirkwood CPU DAIs have an ID != 0. For the Cubox, the CPU DAI
of the first link (system playback) has the ID 0, so I can move to DPCM
setting the 'dynamic' and 'no_pcm' flags in the DAI links at snd
platform probe time.

Then, after some extensions of the simple-card, the DT would look like
(not tested yet):

	sound {
		compatible = "simple-audio-card";
		simple-audio-routing =
			"HDMI I2S Playback",	"System Playback",
			"HDMI SPDIF Playback",	"System Playback",
			"SPDIF Playback",	"System Playback",

			"hdmi-out-i2s",		"HDMI I2S Playback",
			"hdmi-out-spdif",	"HDMI SPDIF Playback",
			"spdif-out",		"SPDIF Playback";

		simple-audio-card,cpu@0 {
			link-name = "System audio";	/* extension */
			sound-dai = <&audio1 0>;
			playback;			/* extension */
			format = "i2s";
		};
		simple-audio-card,codec@0 {
			sound-dai-name = "snd-soc-dummy-dai";
		};

							/* multi-links extension */
		simple-audio-card,cpu@1 {
			link-name = "hdmi-i2s";
			platform-name = "snd-soc-dummy"; /* extension */
			sound-dai = <&audio1 1>;
			playback;
		};
		simple-audio-card,codec@1 {
			sound-dai = <&hdmi_codec 0>;
		};

		simple-audio-card,cpu@2 {
			link-name = "hdmi-spdif";
			platform-name = "snd-soc-dummy";
			sound-dai = <&audio1 2>;
			playback;
		};
		simple-audio-card,codec@2 {
			sound-dai = <&hdmi_codec 1>;
		};

		simple-audio-card,cpu@3 {
			link-name = "spdif";
			platform-name = "snd-soc-dummy";
			sound-dai = <&audio1 1>;
			playback;
		};
		simple-audio-card,codec@3 {
			sound-dai = <&spdif_codec>;
		};
	};

May I go to this direction?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Lars-Peter Clausen]

On 01/01/2014 09:08 PM, Jean-Francois Moine wrote:
> On Wed, 01 Jan 2014 20:05:05 +0100
> Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
>> As Mark also said, this binding definitely leaks way too much internals of
>> the current ASoC implementation. In my opinion the way forward for ASoC is
>> to stop to distinguish between different types of components. This is on one
>> hand CODECS and CPU-DAIs and on the other hand also front-end and beck-end
>> DAIs. The first steps in this direction have already been take by the start
>> of the component-fication, but its still a long way to go. Exposing those
>> concepts via the devicetree will only make it harder to get rid of them
>> later. The bindings for a compound card should essentially describe which
>> components are involved and how the fabric between and around them looks
>> like. If the type of the component is needed in the ASoC implementation it
>> should be possible to auto-discover it. Also I think we want to align the
>> devicetree bindings with what the media people have been doing[1].
> 
> (you forgot the [1] reference)

http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/media/video-interfaces.txt

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Jean-Francois Moine]

On Wed, 01 Jan 2014 21:11:23 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 01/01/2014 09:08 PM, Jean-Francois Moine wrote:
> > On Wed, 01 Jan 2014 20:05:05 +0100
> > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > 
> >> As Mark also said, this binding definitely leaks way too much internals of
> >> the current ASoC implementation. In my opinion the way forward for ASoC is
> >> to stop to distinguish between different types of components. This is on one
> >> hand CODECS and CPU-DAIs and on the other hand also front-end and beck-end
> >> DAIs. The first steps in this direction have already been take by the start
> >> of the component-fication, but its still a long way to go. Exposing those
> >> concepts via the devicetree will only make it harder to get rid of them
> >> later. The bindings for a compound card should essentially describe which
> >> components are involved and how the fabric between and around them looks
> >> like. If the type of the component is needed in the ASoC implementation it
> >> should be possible to auto-discover it. Also I think we want to align the
> >> devicetree bindings with what the media people have been doing[1].
> > 
> > (you forgot the [1] reference)
> 
> http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/media/video-interfaces.txt

I see the idea. So here is a proposal of DT for the Cubox (audio + video)
which should work without too many difficulties.

/* video / audio transmitter */

	tda998x: hdmi-encoder {
		compatible = "nxp,tda998x";
		...

		/* video */
		port@0 {
			tda998x_0: endpoint {
				reg = <0x230145>;
				remote-endpoint = <&lcd0_0>;
			};
		};

		/* audio */
		port@1 {
			hdmi_i2s_audio: endpoint@0 {
				reg = <0x03>;
				remote-endpoint = <&audio_hdmi_i2s>;
			};
			hdmi_spdif_audio: endpoint@1 {
				reg = <0x04>;
				remote-endpoint = <&audio_hdmi_spdif>;
			};
		};
	};

	toslink: spdif {
		compatible = "linux,spdif-dit";
		port {
			spdif_audio: endpoint {
				remote-endpoint = <&audio_spdif>;
			};
		};
	};

/* video */

	lcd0: video-controller {
		compatible = "marvell,dove-lcd";
		...
		port {
			lcd0_0: endpoint {
				remote-endpoint = <&tda998x_0>;
			};
		};
	};

	video {
		compatible = "media-video";
		video-controller = <&lcd0>;
	};

/* audio */

	audio1: audio-controller {
		compatible = "marvell,dove-audio";
		...
		port@0 {
			audio_hdmi_i2s: endpoint {
				remote-endpoint = <&hdmi_i2s_audio>;
			};
		};

		port@1 {
			audio_hdmi_spdif: endpoint@0 {
				remote-endpoint = <&hdmi_spdif_audio>;
			};
			audio_spdif: endpoint@1 {
				remote-endpoint = <&spdif_audio>;
			};
		};
	};

	sound {
		compatible = "media-audio";
		audio-controller = <&audio1>;
	};

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Thu, Jan 02, 2014 at 10:26:47AM +0100, Jean-Francois Moine wrote:

> 		/* audio */
> 		port@1 {
> 			hdmi_i2s_audio: endpoint@0 {
> 				reg = <0x03>;
> 				remote-endpoint = <&audio_hdmi_i2s>;
> 			};

I think we want an explicit object in the card representing the DAIs.
This will both be useful for making it easy to find the configuration
for the link and will be more extensible for the cases where multiple
devices are connected, you can't just assume there's just two.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Jean-Francois Moine]

On Thu, 2 Jan 2014 11:10:56 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jan 02, 2014 at 10:26:47AM +0100, Jean-Francois Moine wrote:
> 
> > 		/* audio */
> > 		port@1 {
> > 			hdmi_i2s_audio: endpoint@0 {
> > 				reg = <0x03>;
> > 				remote-endpoint = <&audio_hdmi_i2s>;
> > 			};
> 
> I think we want an explicit object in the card representing the DAIs.
> This will both be useful for making it easy to find the configuration
> for the link and will be more extensible for the cases where multiple
> devices are connected, you can't just assume there's just two.

I don't see the problem: the 'port' is the DAI. The associated
endpoints give the DAI links and the routing information.

As the DT definition has been done for video, some properties may be
added at will for audio.

What kind of object were you thinking of?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Thu, Jan 02, 2014 at 12:43:31PM +0100, Jean-Francois Moine wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > I think we want an explicit object in the card representing the DAIs.
> > This will both be useful for making it easy to find the configuration
> > for the link and will be more extensible for the cases where multiple
> > devices are connected, you can't just assume there's just two.

> I don't see the problem: the 'port' is the DAI. The associated
> endpoints give the DAI links and the routing information.

> As the DT definition has been done for video, some properties may be
> added at will for audio.

> What kind of object were you thinking of?

Like I say multiple devices on the same link - if you're just listing a
single remote device there can't be more than one.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Jean-Francois Moine]

On Thu, 2 Jan 2014 11:56:18 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jan 02, 2014 at 12:43:31PM +0100, Jean-Francois Moine wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > I think we want an explicit object in the card representing the DAIs.
> > > This will both be useful for making it easy to find the configuration
> > > for the link and will be more extensible for the cases where multiple
> > > devices are connected, you can't just assume there's just two.
> 
> > I don't see the problem: the 'port' is the DAI. The associated
> > endpoints give the DAI links and the routing information.
> 
> > As the DT definition has been done for video, some properties may be
> > added at will for audio.
> 
> > What kind of object were you thinking of?
> 
> Like I say multiple devices on the same link - if you're just listing a
> single remote device there can't be more than one.

I still don't understand. There is already such cases in the Cubox:
the S/PDIF output from the kirkwood audio controller is connected to
both the HDMI transmitter and the S/PDIF TOSLINK. So, in the audio
controller, the port @1 defines the S/PDIF DAI and the endpoints @0 and
@1 point to the remote DAIs, creating 2 snd DAI links:

	port@1 {
		audio_hdmi_spdif: endpoint@0 {
			remote-endpoint = <&hdmi_spdif_audio>;
		};
		audio_spdif: endpoint@1 {
			remote-endpoint = <&spdif_audio>;
		};
	};

in the snd card:

	- DAI link 1 = 'audio controller spdif out' <=> 'hdmi spdif'
	- DAI link 2 = 'audio controller spdif out' <=> 'spdif'

If I am wrong, may you give us an example for which such a DT would not
work?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Thu, Jan 02, 2014 at 01:44:37PM +0100, Jean-Francois Moine wrote:

> I still don't understand. There is already such cases in the Cubox:
> the S/PDIF output from the kirkwood audio controller is connected to
> both the HDMI transmitter and the S/PDIF TOSLINK. So, in the audio
> controller, the port @1 defines the S/PDIF DAI and the endpoints @0 and
> @1 point to the remote DAIs, creating 2 snd DAI links:

> 	port@1 {
> 		audio_hdmi_spdif: endpoint@0 {
> 			remote-endpoint = <&hdmi_spdif_audio>;
> 		};
> 		audio_spdif: endpoint@1 {
> 			remote-endpoint = <&spdif_audio>;
> 		};
> 	};

Oh, so the endpoints are virtual and that's supposed to be three things
wired together rather than a single device with multiple links?  That's
really not very clear from reading the above and seems cumbersome -
every device will want to explicitly identify every other device on the
link and any configuration is going to either need to be replicated on
every device or we'll need to check lots of places for the configuation.
It seems like this will be hard to work with.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Jean-Francois Moine]

On Thu, 2 Jan 2014 13:10:45 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Jan 02, 2014 at 01:44:37PM +0100, Jean-Francois Moine wrote:
> 
> > I still don't understand. There is already such cases in the Cubox:
> > the S/PDIF output from the kirkwood audio controller is connected to
> > both the HDMI transmitter and the S/PDIF TOSLINK. So, in the audio
> > controller, the port @1 defines the S/PDIF DAI and the endpoints @0 and
> > @1 point to the remote DAIs, creating 2 snd DAI links:
> 
> > 	port@1 {
> > 		audio_hdmi_spdif: endpoint@0 {
> > 			remote-endpoint = <&hdmi_spdif_audio>;
> > 		};
> > 		audio_spdif: endpoint@1 {
> > 			remote-endpoint = <&spdif_audio>;
> > 		};
> > 	};
> 
> Oh, so the endpoints are virtual and that's supposed to be three things
> wired together rather than a single device with multiple links?  That's
> really not very clear from reading the above and seems cumbersome -
> every device will want to explicitly identify every other device on the
> link and any configuration is going to either need to be replicated on
> every device or we'll need to check lots of places for the configuation.
> It seems like this will be hard to work with.

No, the 'endpoint' <=> 'remote-endpoint' is a point to point relation.
Even if the sources and sinks are not explicitly defined, the way
the stream flows is easy to find: the main source is always in the
'audio-controller' node

	sound {
		compatible = "media-audio";
		audio-controller = <&audio1>;
	};

and, then, the controller ports are sources (CPU DAIs) and the
associated remote ports are sinks (CODEC DAIs).

With many levels, once the remote (sink) ports are identified, in the
devices where such sinks exist, the remaining ports are sources.

Usually, the devices don't have to know to which other device they are
connected, and, yes, the reverse pointer sink to source is not useful.

But the way (link) the audio stream comes from may be important to
know. This is the case for the HDMI CODEC which must tell the HDMI
transmitter from which hardware port(s) ('reg') it may get the audio
stream. That's why, the HDMI encoder has two endpoints in its audio
port, each endpoint being a different CODEC DAI.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [alsa-devel] [PATCH] ASoC: generic: add generic compound card with DT support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

On Thu, Jan 02, 2014 at 06:50:55PM +0100, Jean-Francois Moine wrote:

> No, the 'endpoint' <=> 'remote-endpoint' is a point to point relation.
> Even if the sources and sinks are not explicitly defined, the way
> the stream flows is easy to find: the main source is always in the
> 'audio-controller' node

But the links in question aren't point to point links and may be
bidirectional...

> Usually, the devices don't have to know to which other device they are
> connected, and, yes, the reverse pointer sink to source is not useful.

This may be the case for the systems you've looked at but other designs
are quite common.

> But the way (link) the audio stream comes from may be important to
> know. This is the case for the HDMI CODEC which must tell the HDMI
> transmitter from which hardware port(s) ('reg') it may get the audio
> stream. That's why, the HDMI encoder has two endpoints in its audio
> port, each endpoint being a different CODEC DAI.

Obviously if there are multiple DAIs on a device then it needs to be
possible to represent them separately but that seems orthogonal to the
rest of the discussion (and resolved already)?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]