Skip to contentSkip to conversation

ASoC: simple-card: Add multi-CODEC support

PATCH Superseded Jean-Francois Moine 4 participants 9 messages Nov 3, 2014 – Nov 9, 2014 5 messages not in the archive

Subsystems (4)

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v3 cover ASoC: simple-card: Add multi-CODEC support

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

v3: rebase on broonie git 2014/11/03
v2: accept list of CODECs in 'sound-dais' (Benoit Cousson)

Jean-Francois Moine (2):
ASoC: core: add multi-codec support in DT
ASoC: simple-card: add multi-CODECs in DT

 .../devicetree/bindings/sound/simple-card.txt      |  28 ++---
 include/sound/soc.h                                |   3 +
 sound/soc/generic/simple-card.c                    | 135 +++++++++++++--------
 sound/soc/soc-core.c                               |  75 +++++++++---
 4 files changed, 157 insertions(+), 84 deletions(-)
Signature · 1 line
2.1.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

PATCH v3 1/2 ASoC: core: add multi-codec support in DT

SoC audio cards may have many CODECs per audio link. This patch exports a core function to handle such links when they are described in the DT.

---
 include/sound/soc.h  |  3 +++
 sound/soc/soc-core.c | 75 +++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 63 insertions(+), 15 deletions(-)
include/sound/soc.h +3 -0
index 7ba7130..a255f8b 100644
@@ -1451,6 +1451,9 @@ unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
 				     struct device_node **framemaster);
 int snd_soc_of_get_dai_name(struct device_node *of_node,
 			    const char **dai_name);
+int snd_soc_of_get_dai_link_codecs(struct device_node *of_node,
+				   const char *list_name,
+				   struct snd_soc_dai_link *dai_link);
 
 #include <sound/soc-dai.h>
 
sound/soc/soc-core.c +60 -15

108 lines not on this page · raw message

Signature · 6 lines
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 6b04d690576a0036b401d00784299ee576051c0d.1415096705.git.moinejf-GANU6spQydw@public.gmane.org

#4 lore raw

Re: [PATCH v3 1/2] ASoC: core: add multi-codec support in DT

Quoted 3 lines · Jean-Francois Moine

SoC audio cards may have many CODECs per audio link. This patch exports a core function to handle such links when they are described in the DT.

It would be a lot easier to review this change if the changelog said what the description this was intended to parse looks like...

sound/soc/soc-core.c int snd_soc_of_get_dai_name(struct device_node *of_node, · line 3796

Quoted 3 lines
sound/soc/soc-core.c +3 -0
+int snd_soc_of_get_dai_link_codecs(struct device_node *of_node,
+				   const char *list_name,
+				   struct snd_soc_dai_link *dai_link)

This is a newly exported function, please add kerneldoc for it.

Quoted 8 lines
sound/soc/soc-core.c +8 -0
+{
+	struct of_phandle_args args;
+	struct snd_soc_dai_link_component *component;
+	int index, ret;
+
+	for (index = 0, component = dai_link->codecs;
+	     index < dai_link->num_codecs;
+	     index++, component++) {

sound/soc/soc-core.c int snd_soc_of_get_dai_name(struct device_node *of_node, · line 3814

Quoted 10 lines
sound/soc/soc-core.c +10 -0
+		ret = of_parse_phandle_with_args(of_node, list_name,
+						 "#sound-dai-cells",
+						  index, &args);
+		if (ret)
+			goto err;
+		component->of_node = args.np;
+		ret = snd_soc_get_dai_name(&args, &component->dai_name);
+		if (ret < 0)
+			goto err;
+	}

We're relying on the caller to fill in the number of CODECs and not checking that there aren't extra things in the list in the DT that we're ignoring. I'd expect us to either get the number of CODECs as part of this function or check that there isn't anything extra in the list.

I'd also expect some sort of error message on parse failures to help people writing DTs.

#5 lore raw

PATCH v3 2/2 ASoC: simple-card: add multi-CODECs in DT

In reply to a message not in the archive

This patch allows many CODECs per link to be defined in the device tree.

---
 .../devicetree/bindings/sound/simple-card.txt      |  28 ++---
 sound/soc/generic/simple-card.c                    | 135 +++++++++++++--------
 2 files changed, 94 insertions(+), 69 deletions(-)
Documentation/devicetree/bindings/sound/simple-card.txt +12 -16
index c3cba60..6cb1816 100644
@@ -65,7 +65,11 @@ properties should also be placed in the codec node if needed.
 
 Required CPU/CODEC subnodes properties:
 
-- sound-dai				: phandle and port of CPU/CODEC
+either
+  - sound-dai				: phandle and port of CPU/CODEC
+or
+  - sound-dais				: list of phandle and port of CODECs
+					  
 
 Optional CPU/CODEC subnodes properties:
 
@@ -119,37 +123,29 @@ 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>;
+			sound-dais = <&hdmi 0>, <&spdif_codec>;
 		};
 	};
 
-	simple-audio-card,dai-link@2 {		/* S/PDIF - S/PDIF */
+	simple-audio-card,dai-link@1 {		/* I2S - HDMI */
+		format = "i2s";
 		cpu {
-			sound-dai = <&audio1 1>;
+			sound-dai = <&audio1 0>;
 		};
 		codec {
-			sound-dai = <&spdif_codec>;
+			sound-dai = <&hdmi 1>;
 		};
 	};
 };
sound/soc/generic/simple-card.c +82 -53

226 lines not on this page · raw message

Signature · 6 lines
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 53ef077d2174f8eb5b861d6cebe7c9e732f06d52.1415096705.git.moinejf-GANU6spQydw@public.gmane.org

#7 lore raw

Re: [PATCH v3 2/2] ASoC: simple-card: add multi-CODECs in DT

On Tue, Nov 04, 2014 at 11:21:46AM +0100, Jean-Francois Moine wrote:

This looks like it combines some refactoring to split out some of the DAI node parsing code with some new code. It's quite hard to read what's going on here, separating the code motion and the new code would make life a lot easier, I've stared at it for a bit and I'm still not 100% sure it's doing everything it's supposed to be.

Documentation/devicetree/bindings/sound/simple-card.txt sh_fsi2: sh_fsi2@ec230000 { · line 126

Quoted 2 lines
Documentation/devicetree/bindings/sound/simple-card.txt +1 -1
-Example 2 - many DAI links:
+Example 2 - many DAI links and multi-CODECs:

This would be a lot easier to read if the change didn't completely rewrite the example but just added the thing it wants to add an example of; the example may be drawn from a real system but that's not the point.

sound/soc/generic/simple-card.c asoc_simple_card_sub_parse_of(struct device_node *np, · line 203

Quoted 3 lines
sound/soc/generic/simple-card.c +1 -1
 	} else {
-		clk = of_clk_get(args.np, 0);
+		clk = of_clk_get((struct device_node *) dai_np, 0);

Adding this cast looks suspicous - why? As far as I can tell the original code didn't need one.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#9 lore raw

Re: [PATCH v3 2/2] ASoC: simple-card: add multi-CODECs in DT

Quoted 6 lines · On Fri, 7 Nov 2014 12:30:35 +0000 Mark Brown…

sound/soc/generic/simple-card.c asoc_simple_card_sub_parse_of(struct device_node *np, · line 203

Quoted 3 lines
sound/soc/generic/simple-card.c +1 -1
 	} else {
-		clk = of_clk_get(args.np, 0);
+		clk = of_clk_get((struct device_node *) dai_np, 0);  

Adding this cast looks suspicous - why? As far as I can tell the original code didn't need one.

Right, 'args.np' was (struct device_node *), but, now, 'dai_np' is (const struct device_node *).

Changing 'dai_np' to (struct device_node *) in the asoc_simple_card_sub_parse_of() argument asks for a cast in calling this function because the field 'of_node' of the struct snd_soc_dai_link_component is (const struct device_node *).

Do you better like this last cast?

Signature · 6 lines
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#10 lore raw

Re: [alsa-devel] [PATCH v3 2/2] ASoC: simple-card: add multi-CODECs in DT

Quoted 13 lines · Jean-Francois Moine
Quoted 6 lines · On Fri, 7 Nov 2014 12:30:35 +0000 Mark Brown…

sound/soc/generic/simple-card.c asoc_simple_card_sub_parse_of(struct device_node *np, · line 203

Quoted 3 lines
  	} else {
sound/soc/generic/simple-card.c +1 -1
-		clk = of_clk_get(args.np, 0);
+		clk = of_clk_get((struct device_node *) dai_np, 0);

Adding this cast looks suspicous - why? As far as I can tell the original code didn't need one.

Right, 'args.np' was (struct device_node *), but, now, 'dai_np' is (const struct device_node *).

Quoted 6 lines

Changing 'dai_np' to (struct device_node *) in the asoc_simple_card_sub_parse_of() argument asks for a cast in calling this function because the field 'of_node' of the struct snd_soc_dai_link_component is (const struct device_node *).

Do you better like this last cast?

Casting const away is usually a sign that something is wrong and has the potential to result in undefined behavior. The correct fix in this case is probably to make the np argument for of_clk_get() const.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 545D2667.4070707-Qo5EllUWu/[email protected]

#12 lore raw

Re: [alsa-devel] [PATCH v3 2/2] ASoC: simple-card: add multi-CODECs in DT

Quoted 3 lines · On Fri, 07 Nov 2014 21:07:03 +0100 Lars-Peter Clausen…

Casting const away is usually a sign that something is wrong and has the potential to result in undefined behavior. The correct fix in this case is probably to make the np argument for of_clk_get() const.

Thanks. I am sending a patch for that.

Signature · 6 lines
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#13 lore raw

Re: [PATCH v3 2/2] ASoC: simple-card: add multi-CODECs in DT

In reply to Jean-Francois Moine
Quoted 12 lines · Jean-Francois Moine
Quoted 6 lines · Mark Brown

sound/soc/generic/simple-card.c asoc_simple_card_sub_parse_of(struct device_node *np, · line 203

Quoted 3 lines
sound/soc/generic/simple-card.c +1 -1
 	} else {
-		clk = of_clk_get(args.np, 0);
+		clk = of_clk_get((struct device_node *) dai_np, 0);  

Adding this cast looks suspicous - why? As far as I can tell the original code didn't need one.

Right, 'args.np' was (struct device_node *), but, now, 'dai_np' is (const struct device_node *).

Quoted 6 lines

Changing 'dai_np' to (struct device_node *) in the asoc_simple_card_sub_parse_of() argument asks for a cast in calling this function because the field 'of_node' of the struct snd_soc_dai_link_component is (const struct device_node *).

Do you better like this last cast?

No. As Lars says having a cast in the first place is almost always a sign that you're doing something wrong outside of passing things through some of the generic callback interfaces.