[PATCH] ASoC:simple-card: Add multi-CODEC support

[PATCH] ASoC:simple-card: Add multi-CODEC support

[Jean-Francois Moine]

This patch adds multi-CODEC support to the simple-card.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/sound/simple-card.txt      | 28 +++++++--------
 sound/soc/generic/simple-card.c                    | 41 +++++++++++++++++++++-
 2 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index c2e9841..c217687 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -37,6 +37,8 @@ Required dai-link subnodes:
 
 - cpu					: CPU   sub-node
 - codec					: CODEC sub-node
+					  In case of multi-CODECs, there may
+					  be many of such sub-nodes.
 
 Optional dai-link subnode properties:
 
@@ -115,37 +117,31 @@ sh_fsi2: sh_fsi2@ec230000 {
 	interrupts = <0 146 0x4>;
 };
 
-Example 2 - many DAI links:
+Example 2 - many DAI links and multi-CODECs:
 
 sound {
 	compatible = "simple-audio-card";
 	simple-audio-card,name = "Cubox Audio";
 
-	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
+	simple-audio-card,dai-link@0 {		/* S/PDIF - HDMI & S/PDIF */
 		format = "i2s";
 		cpu {
-			sound-dai = <&audio1 0>;
-		};
-		codec {
-			sound-dai = <&tda998x 0>;
-		};
-	};
-
-	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
-		cpu {
 			sound-dai = <&audio1 1>;
 		};
-		codec {
-			sound-dai = <&tda998x 1>;
+		codec@0 {
+			sound-dai = <&hdmi 0>;
+		};
+		codec@1 {
+			sound-dai = <&spdif_codec>;
 		};
 	};
 
-	simple-audio-card,dai-link@2 {		/* S/PDIF - S/PDIF */
+	simple-audio-card,dai-link@1 {		/* I2S - HDMI */
 		cpu {
-			sound-dai = <&audio1 1>;
+			sound-dai = <&audio1 0>;
 		};
 		codec {
-			sound-dai = <&spdif_codec>;
+			sound-dai = <&hdmi 1>;
 		};
 	};
 };
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 4053152..bf0ce08 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -179,11 +179,12 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	struct device_node *np = NULL;
 	struct device_node *bitclkmaster = NULL;
 	struct device_node *framemaster = NULL;
+	struct snd_soc_dai_link_component *component;
 	unsigned int daifmt;
 	char *name;
 	char prop[128];
 	char *prefix = "";
-	int ret, cpu_args;
+	int ret, cpu_args, num_codec_dais;
 
 	/* For single DAI link & old style of DT node */
 	if (is_top_level_node)
@@ -225,7 +226,16 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	}
 
 	of_node_put(np);
+
+	/* count the number of codec DAIs */
 	snprintf(prop, sizeof(prop), "%scodec", prefix);
+	num_codec_dais = 0;
+	for_each_child_of_node(node, np) {
+		if (strcmp(np->name, prop) == 0)
+			num_codec_dais++;
+	}
+
+	/* treat the first DAI */
 	np = of_get_child_by_name(node, prop);
 	if (!np) {
 		ret = -EINVAL;
@@ -307,6 +317,35 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	if (!cpu_args)
 		dai_link->cpu_dai_name = NULL;
 
+	/* handle multi-codec DAIs */
+	if (num_codec_dais == 1)
+		goto out;
+	dai_link->codecs = component =
+			   devm_kzalloc(dev,
+					sizeof *component * num_codec_dais,
+					GFP_KERNEL);
+	dai_link->num_codecs = num_codec_dais;
+	component->of_node = dai_link->codec_of_node;
+	dai_link->codec_of_node = NULL;
+	component->dai_name = dai_link->codec_dai_name;
+	dai_link->codec_dai_name = NULL;
+	for (;;) {
+		np = of_get_next_child(node, np);
+		if (!np)
+			break;
+		component++;
+		component->of_node = of_parse_phandle(np, "sound-dai", 0);
+		if (!component->of_node) {
+			ret = -ENODEV;
+			dev_err(dev, "Bad sound-dai\n");
+			goto dai_link_of_err;
+		}
+		ret = snd_soc_of_get_dai_name(np, &component->dai_name);
+		if (ret < 0)
+			goto dai_link_of_err;
+	}
+
+out:
 dai_link_of_err:
 	if (np)
 		of_node_put(np);
-- 
2.1.0

Re: [PATCH] ASoC:simple-card: Add multi-CODEC support

[Benoit Cousson]

Hi Jean-Francois,

On 10/09/2014 13:28, Jean-Francois Moine wrote:
> This patch adds multi-CODEC support to the simple-card.
>
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>   .../devicetree/bindings/sound/simple-card.txt      | 28 +++++++--------
>   sound/soc/generic/simple-card.c                    | 41 +++++++++++++++++++++-
>   2 files changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> index c2e9841..c217687 100644
> --- a/Documentation/devicetree/bindings/sound/simple-card.txt
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -37,6 +37,8 @@ Required dai-link subnodes:
>
>   - cpu					: CPU   sub-node
>   - codec					: CODEC sub-node
> +					  In case of multi-CODECs, there may
> +					  be many of such sub-nodes.
>
>   Optional dai-link subnode properties:
>
> @@ -115,37 +117,31 @@ sh_fsi2: sh_fsi2@ec230000 {
>   	interrupts = <0 146 0x4>;
>   };
>
> -Example 2 - many DAI links:
> +Example 2 - many DAI links and multi-CODECs:
>
>   sound {
>   	compatible = "simple-audio-card";
>   	simple-audio-card,name = "Cubox Audio";
>
> -	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
> +	simple-audio-card,dai-link@0 {		/* S/PDIF - HDMI & S/PDIF */
>   		format = "i2s";
>   		cpu {
> -			sound-dai = <&audio1 0>;
> -		};
> -		codec {
> -			sound-dai = <&tda998x 0>;
> -		};
> -	};
> -
> -	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
> -		cpu {
>   			sound-dai = <&audio1 1>;
>   		};
> -		codec {
> -			sound-dai = <&tda998x 1>;
> +		codec@0 {
> +			sound-dai = <&hdmi 0>;
> +		};
> +		codec@1 {
> +			sound-dai = <&spdif_codec>;
>   		};

I don't have strong opinion on that, but in my case, I was considering 
using a simple list instead of several nodes.
I don't like having to add fake address just to ensure uniqueness.

Something like that:

  sound-dais = <&spdif_codec 1>, <&hdmi 0>;

That being said, it will require changing the name with a plural form, 
and ensuring we have the same number of parameters for each codec.

That was just my .2 cents.

Regards,
Benoit



>   	};
>
> -	simple-audio-card,dai-link@2 {		/* S/PDIF - S/PDIF */
> +	simple-audio-card,dai-link@1 {		/* I2S - HDMI */
>   		cpu {
> -			sound-dai = <&audio1 1>;
> +			sound-dai = <&audio1 0>;
>   		};
>   		codec {
> -			sound-dai = <&spdif_codec>;
> +			sound-dai = <&hdmi 1>;
>   		};
>   	};
>   };
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 4053152..bf0ce08 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -179,11 +179,12 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
>   	struct device_node *np = NULL;
>   	struct device_node *bitclkmaster = NULL;
>   	struct device_node *framemaster = NULL;
> +	struct snd_soc_dai_link_component *component;
>   	unsigned int daifmt;
>   	char *name;
>   	char prop[128];
>   	char *prefix = "";
> -	int ret, cpu_args;
> +	int ret, cpu_args, num_codec_dais;
>
>   	/* For single DAI link & old style of DT node */
>   	if (is_top_level_node)
> @@ -225,7 +226,16 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
>   	}
>
>   	of_node_put(np);
> +
> +	/* count the number of codec DAIs */
>   	snprintf(prop, sizeof(prop), "%scodec", prefix);
> +	num_codec_dais = 0;
> +	for_each_child_of_node(node, np) {
> +		if (strcmp(np->name, prop) == 0)
> +			num_codec_dais++;
> +	}
> +
> +	/* treat the first DAI */
>   	np = of_get_child_by_name(node, prop);
>   	if (!np) {
>   		ret = -EINVAL;
> @@ -307,6 +317,35 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
>   	if (!cpu_args)
>   		dai_link->cpu_dai_name = NULL;
>
> +	/* handle multi-codec DAIs */
> +	if (num_codec_dais == 1)
> +		goto out;
> +	dai_link->codecs = component =
> +			   devm_kzalloc(dev,
> +					sizeof *component * num_codec_dais,
> +					GFP_KERNEL);
> +	dai_link->num_codecs = num_codec_dais;
> +	component->of_node = dai_link->codec_of_node;
> +	dai_link->codec_of_node = NULL;
> +	component->dai_name = dai_link->codec_dai_name;
> +	dai_link->codec_dai_name = NULL;
> +	for (;;) {
> +		np = of_get_next_child(node, np);
> +		if (!np)
> +			break;
> +		component++;
> +		component->of_node = of_parse_phandle(np, "sound-dai", 0);
> +		if (!component->of_node) {
> +			ret = -ENODEV;
> +			dev_err(dev, "Bad sound-dai\n");
> +			goto dai_link_of_err;
> +		}
> +		ret = snd_soc_of_get_dai_name(np, &component->dai_name);
> +		if (ret < 0)
> +			goto dai_link_of_err;
> +	}
> +
> +out:
>   dai_link_of_err:
>   	if (np)
>   		of_node_put(np);
>


-- 
Benoît Cousson
BayLibre
Embedded Linux Technology Lab
www.baylibre.com

Re: [PATCH] ASoC:simple-card: Add multi-CODEC support

[Jean-Francois Moine]

On Wed, 10 Sep 2014 13:43:06 +0200
Benoit Cousson <bcousson@baylibre.com> wrote:

> > +		codec@0 {
> > +			sound-dai = <&hdmi 0>;
> > +		};
> > +		codec@1 {
> > +			sound-dai = <&spdif_codec>;
> >   		};  
> 
> I don't have strong opinion on that, but in my case, I was considering 
> using a simple list instead of several nodes.
> I don't like having to add fake address just to ensure uniqueness.
> 
> Something like that:
> 
>   sound-dais = <&spdif_codec 1>, <&hdmi 0>;

You are right, this would be simpler (I did not see that there could be
phandle's with different cell sizes in a list).

> That being said, it will require changing the name with a plural form, 
> and ensuring we have the same number of parameters for each codec.

But I don't think that changing the name could be useful: the treatment
is exactly the same and, as a result, all CODEC DAIs go to the DAI
list of the DAI link.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC:simple-card: Add multi-CODEC support

[Benoit Cousson]

On 10/09/2014 18:06, Jean-Francois Moine wrote:
> On Wed, 10 Sep 2014 13:43:06 +0200
> Benoit Cousson <bcousson@baylibre.com> wrote:
>
>>> +		codec@0 {
>>> +			sound-dai = <&hdmi 0>;
>>> +		};
>>> +		codec@1 {
>>> +			sound-dai = <&spdif_codec>;
>>>    		};
>>
>> I don't have strong opinion on that, but in my case, I was considering
>> using a simple list instead of several nodes.
>> I don't like having to add fake address just to ensure uniqueness.
>>
>> Something like that:
>>
>>    sound-dais = <&spdif_codec 1>, <&hdmi 0>;
>
> You are right, this would be simpler (I did not see that there could be
> phandle's with different cell sizes in a list).

In fact you cannot, that why I added the same number of parameters for 
both.

For DT, it is just a list of elements, it could be encoded like that:
<&spdif_codec 1 &hdmi 0>;

At the end it will be a list of u32 inside the dtb.

BTW, what is that extra parameter after the phandle? I cannot find any 
reference of that inside the Documentation.

If we need to have a random number of parameters after the code-dai 
phandle, we couldn't use that list.

>> That being said, it will require changing the name with a plural form,
>> and ensuring we have the same number of parameters for each codec.
>
> But I don't think that changing the name could be useful: the treatment
> is exactly the same and, as a result, all CODEC DAIs go to the DAI
> list of the DAI link.

Yeah, maybe not that useful, but that was just to be aligned with 
similar cases in DT bindings. Like gpios, interrupts...


Regards,
Benoit


-- 
Benoît Cousson
BayLibre
Embedded Linux Technology Lab
www.baylibre.com
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC:simple-card: Add multi-CODEC support

[Jean-Francois Moine]

On Wed, 10 Sep 2014 18:27:26 +0200
Benoit Cousson <bcousson@baylibre.com> wrote:

> >> I don't have strong opinion on that, but in my case, I was considering
> >> using a simple list instead of several nodes.
> >> I don't like having to add fake address just to ensure uniqueness.
> >>
> >> Something like that:
> >>
> >>    sound-dais = <&spdif_codec 1>, <&hdmi 0>;  
> >
> > You are right, this would be simpler (I did not see that there could be
> > phandle's with different cell sizes in a list).  
> 
> In fact you cannot, that why I added the same number of parameters for 
> both.
> 
> For DT, it is just a list of elements, it could be encoded like that:
> <&spdif_codec 1 &hdmi 0>;
> 
> At the end it will be a list of u32 inside the dtb.

Yes, but __of_parse_phandle_with_args() handles the number of cells of the phandle.

> BTW, what is that extra parameter after the phandle? I cannot find any 
> reference of that inside the Documentation.

It is the DAI number inside the CODEC. The TDA998x CODEC has two DAIs,
S/PDIF and I2S.

> If we need to have a random number of parameters after the code-dai 
> phandle, we couldn't use that list.
> 
> >> That being said, it will require changing the name with a plural form,
> >> and ensuring we have the same number of parameters for each codec.  
> >
> > But I don't think that changing the name could be useful: the treatment
> > is exactly the same and, as a result, all CODEC DAIs go to the DAI
> > list of the DAI link.  
> 
> Yeah, maybe not that useful, but that was just to be aligned with 
> similar cases in DT bindings. Like gpios, interrupts...

Not easy! "sound-dai" is used for both the CPU and CODEC DAIs.
Adding "sound-dais" for just the CODEC DAIs would complexify the
simple-card logic...

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel