Fix simple-card *-master DT parameter handling

RFC PATCH In review Jyri Sarha 4 participants 13 messages Mar 10, 2014 – Apr 15, 2014

Subsystems (3)

Sound ASoC Sound - generic sound card (Simple-Audio-Card, Audio-Graph-Card)

lore

Share & tools

lore

Jyri Sarha

author

2014-03-10 11:41 UTC #0 lore raw

v2 cover Fix simple-card *-master DT parameter handling

Since RFC:
- fixed commit msg typo
- added include/sound/soc.h changes too

The sematics of bitclock-master and frame-master DT parameters should depend on whether they are found from a cpu-dai or codec sub-node.

- bitclock-master in cpu-dai node means Codec-Bitclock-Slave
- frame-master in cpu-dai node means Codec-Frame-Slave
- bitclock-master in codec node means Codec-Bitclock-Master
- frame-master in codec node means Codec-Frame-Master

For example in a cpu-dai mode bitclock-master parameter should produce SND_SOC_DAIFMT_CBS_* daifmt flags and a codec node SND_SOC_DAIFMT_CBM_* flags.

Best regards,
Jyri

Jyri Sarha (2):
ASoC: core: Add is_cpu_dai_node-parameter to
snd_soc_of_parse_daifmt()
ASoC: simple-card: Take snd_soc_of_parse_daifmt() change in to
account

 include/sound/soc.h             |    3 ++-
 sound/soc/generic/simple-card.c |   13 ++++++++-----
 sound/soc/soc-core.c            |    8 +++++++-
 3 files changed, 17 insertions(+), 7 deletions(-)

Signature · 1 line

1.7.9.5

Jyri Sarha

author

2014-03-10 11:41 UTC #1 lore raw

RFC PATCH v2 1/2 ASoC: core: Add is_cpu_dai_node-parameter to snd_soc_of_parse_daifmt()

The sematics of bitclock-master and frame-master DT parameters should be inversed when parsing a cpu-dai node.

Signed-off-by Jyri Sarha <[email protected]>

---
 include/sound/soc.h  |    3 ++-
 sound/soc/soc-core.c |    8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

include/sound/soc.h +2 -1

index c0b6656..ecd0745 100644
@@ -1231,7 +1231,8 @@ int snd_soc_of_parse_tdm_slot(struct device_node *np,
 int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 				   const char *propname);
 unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
-				     const char *prefix);
+				     const char *prefix,
+				     bool is_cpu_dai_node);
 int snd_soc_of_get_dai_name(struct device_node *of_node,
 			    const char **dai_name);

sound/soc/soc-core.c +7 -1

index 8ddb15c..dfff75f 100644
@@ -4613,7 +4613,8 @@ int snd_soc_of_parse_audio_routing(struct snd_soc_card *card,
 EXPORT_SYMBOL_GPL(snd_soc_of_parse_audio_routing);
 
 unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
-				     const char *prefix)
+				     const char *prefix,
+				     bool is_cpu_dai_node)
 {
 	int ret, i;
 	char prop[128];

@@ -4700,6 +4701,11 @@ unsigned int snd_soc_of_parse_daifmt(struct device_node *np,
 	snprintf(prop, sizeof(prop), "%sframe-master", prefix);
 	frame = !!of_get_property(np, prop, NULL);
 
+	if (is_cpu_dai_node) {
+		bit = !bit;
+		frame = !frame;
+	}
+
 	switch ((bit << 4) + frame) {
 	case 0x11:
 		format |= SND_SOC_DAIFMT_CBM_CFM;

Signature · 1 line

1.7.9.5

Jyri Sarha

author

2014-03-10 11:41 UTC #2 lore raw

RFC PATCH v2 2/2 ASoC: simple-card: Take snd_soc_of_parse_daifmt() change in to account

In reply to Jyri Sarha

snd_soc_of_parse_daifmt() needs to know if it is parsing a cpu-dai node.

Signed-off-by Jyri Sarha <[email protected]>

---
 sound/soc/generic/simple-card.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

sound/soc/generic/simple-card.c +8 -5

index 7cabcc5..7cbbf44 100644
@@ -87,7 +87,8 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 			      unsigned int daifmt,
 			      struct asoc_simple_dai *dai,
 			      const struct device_node **p_node,
-			      const char **name)
+			      const char **name,
+			      bool is_cpu_dai_node)
 {
 	struct device_node *node;
 	struct clk *clk;

@@ -117,7 +118,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	 * bitclock-master,    frame-master
 	 * and specific "format" if it has
 	 */
-	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
+	dai->fmt = snd_soc_of_parse_daifmt(np, NULL, is_cpu_dai_node);
 	dai->fmt |= daifmt;
 
 	/*

@@ -165,7 +166,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
 
 	/* get CPU/CODEC common format via simple-audio-card,format */
-	priv->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
+	priv->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,", 0) &
 		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
 
 	/* off-codec widgets */

@@ -191,7 +192,8 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->cpu_dai,
 						  &dai_link->cpu_of_node,
-						  &dai_link->cpu_dai_name);
+						  &dai_link->cpu_dai_name,
+						  true);
 	if (ret < 0)
 		return ret;

@@ -202,7 +204,8 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 		ret = asoc_simple_card_sub_parse_of(np, priv->daifmt,
 						  &priv->codec_dai,
 						  &dai_link->codec_of_node,
-						  &dai_link->codec_dai_name);
+						  &dai_link->codec_dai_name,
+						  false);
 	if (ret < 0)
 		return ret;

Signature · 1 line

1.7.9.5

Mark Brown

maintainer

2014-03-11 10:50 UTC #3 lore raw

In reply to Jyri Sarha

Quoted 8 lines · Jyri Sarha

Since RFC:
- fixed commit msg typo
- added include/sound/soc.h changes too
The sematics of bitclock-master and frame-master DT parameters should depend on whether they are found from a cpu-dai or codec sub-node.

Quoted 8 lines

- bitclock-master in cpu-dai node means Codec-Bitclock-Slave
- frame-master in cpu-dai node means Codec-Frame-Slave
- bitclock-master in codec node means Codec-Bitclock-Master
- frame-master in codec node means Codec-Frame-Master
For example in a cpu-dai mode bitclock-master parameter should produce SND_SOC_DAIFMT_CBS_* daifmt flags and a codec node SND_SOC_DAIFMT_CBM_* flags.

I've added Morimoto-san and Xiubo to the CCs - can you please have a look at this? I can't see any in tree users but presumably there are some existing out of tree users of the binding that use it without current problems.

Kuninori Morimoto

2014-03-12 01:13 UTC #4 lore raw

In reply to Jyri Sarha

Hi Jyri

Quoted 8 lines

Since RFC:
- fixed commit msg typo
- added include/sound/soc.h changes too
The sematics of bitclock-master and frame-master DT parameters should depend on whether they are found from a cpu-dai or codec sub-node.

Quoted 8 lines

- bitclock-master in cpu-dai node means Codec-Bitclock-Slave
- frame-master in cpu-dai node means Codec-Frame-Slave
- bitclock-master in codec node means Codec-Bitclock-Master
- frame-master in codec node means Codec-Frame-Master
For example in a cpu-dai mode bitclock-master parameter should produce SND_SOC_DAIFMT_CBS_* daifmt flags and a codec node SND_SOC_DAIFMT_CBM_* flags.

SND_SOC_DAIFMT_xxx comment indicates "codec clk/FRM" indeed. but does this "codec" means "codec chip" ??
I'm not sure.

but anyway, if my understanding is correct,

	simple-audio-card,cpu {
		...
		bitclock-master;
		frame-master;
	};

	simple-audio-card,codec {
		...
		bitclock-master;
		frame-master;
	};

This will be
cpu : SND_SOC_DAIFMT_CBS_CFS
codec : SND_SOC_DAIFMT_CBM_CFM

but, it is un-understandable/confusable for me,
and it breaks our sound card.

	${LINUX}/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
        ${LINUX}/arch/arm/boot/dts/r8a7740-armadillo800eva-reference.dts

I guess you want like this ?

	codec-bitclock-master;
	codec-frame-master;

	simple-audio-card,cpu {
		...
	};

	simple-audio-card,codec {
		...
	};

# And I guess [1/2] and [2/2] should be 1 patch.
# otherwise, it breaks git-bisect :P

Best regards
---
Kuninori Morimoto

Kuninori Morimoto

2014-03-12 04:07 UTC #5 lore raw

Hi Mark, Jyri

Quoted 8 lines

Quoted 8 lines
Since RFC:
- fixed commit msg typo
- added include/sound/soc.h changes too
The sematics of bitclock-master and frame-master DT parameters should depend on whether they are found from a cpu-dai or codec sub-node.

Quoted 8 lines

Quoted 8 lines
- bitclock-master in cpu-dai node means Codec-Bitclock-Slave
- frame-master in cpu-dai node means Codec-Frame-Slave
- bitclock-master in codec node means Codec-Bitclock-Master
- frame-master in codec node means Codec-Frame-Master
For example in a cpu-dai mode bitclock-master parameter should produce SND_SOC_DAIFMT_CBS_* daifmt flags and a codec node SND_SOC_DAIFMT_CBM_* flags.

I had misunderstood about SND_SOC_DAIFMT_xxx
So, please ignore my previous comment.

But, I wounder, if cpu/codec use identical format flags, then, asoc_simple_dai don't need fmt ?

struct asoc_simple_dai {
	const char *name;
	unsigned int fmt;       <= we can/should remove this ?
	unsigned int sysclk;
};

Richard Lee

2014-03-12 05:00 UTC #6 lore raw

In reply to Kuninori Morimoto

Quoted 38 lines · Kuninori Morimoto

Hi Jyri
Quoted 8 lines
Since RFC:
- fixed commit msg typo
- added include/sound/soc.h changes too
The sematics of bitclock-master and frame-master DT parameters should depend on whether they are found from a cpu-dai or codec sub-node.
Quoted 8 lines
- bitclock-master in cpu-dai node means Codec-Bitclock-Slave
- frame-master in cpu-dai node means Codec-Frame-Slave
- bitclock-master in codec node means Codec-Bitclock-Master
- frame-master in codec node means Codec-Frame-Master
For example in a cpu-dai mode bitclock-master parameter should produce SND_SOC_DAIFMT_CBS_* daifmt flags and a codec node SND_SOC_DAIFMT_CBM_* flags.
SND_SOC_DAIFMT_xxx comment indicates "codec clk/FRM" indeed. but does this "codec" means "codec chip" ??
I'm not sure.
but anyway, if my understanding is correct,
        simple-audio-card,cpu {
                ...
                bitclock-master;
                frame-master;
        };

        simple-audio-card,codec {
                ...
                bitclock-master;
                frame-master;
        };

Quoted 4 lines

This will be
cpu : SND_SOC_DAIFMT_CBS_CFS
codec : SND_SOC_DAIFMT_CBM_CFM

Yes, That's also what my understanding of this patches.

But, IMO, if you want the CPU DAI be CBS_CFS and CODEC be CBM_CFM, you could just do it like this:

         simple-audio-card,cpu {
                 ...
         };

         simple-audio-card,codec {
                 ...
                 bitclock-master;
                 frame-master;
         };

and vice versa.

Thanks,

(I could find this mails in my Freescale acount, so I will reply it here.)

--
Best Regards,
Xiubo

Quoted 30 lines

but, it is un-understandable/confusable for me,
and it breaks our sound card.
        ${LINUX}/arch/arm/boot/dts/sh73a0-kzm9g-reference.dts
        ${LINUX}/arch/arm/boot/dts/r8a7740-armadillo800eva-reference.dts
I guess you want like this ?
        codec-bitclock-master;
        codec-frame-master;

        simple-audio-card,cpu {
                ...
        };

        simple-audio-card,codec {
                ...
        };
# And I guess [1/2] and [2/2] should be 1 patch.
# otherwise, it breaks git-bisect :P
Best regards
---
Kuninori Morimoto
--
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

Kuninori Morimoto

2014-03-12 06:11 UTC #7 lore raw

Hi Richard

Quoted 14 lines

but anyway, if my understanding is correct,

        simple-audio-card,cpu {
                ...
                bitclock-master;
                frame-master;
        };

        simple-audio-card,codec {
                ...
                bitclock-master;
                frame-master;
        };

Quoted 6 lines

Quoted 4 lines
This will be
cpu : SND_SOC_DAIFMT_CBS_CFS
codec : SND_SOC_DAIFMT_CBM_CFM
Yes, That's also what my understanding of this patches.

Thank you. I understand.
(and I need to send fixup patches :)

# "codec-is-bitclock-master" seems understandable than

# current "bitclock-master"...

Jyri Sarha

author

2014-03-12 09:27 UTC #8 lore raw

Quoted 22 lines · Kuninori Morimoto

Hi Richard
Quoted 19 lines
Quoted 18 lines
but anyway, if my understanding is correct,
         simple-audio-card,cpu {
                 ...
                 bitclock-master;
                 frame-master;
         };

         simple-audio-card,codec {
                 ...
                 bitclock-master;
                 frame-master;
         };
This will be
cpu : SND_SOC_DAIFMT_CBS_CFS
codec : SND_SOC_DAIFMT_CBM_CFM

Quoted 8 lines

Quoted 1 line
Yes, That's also what my understanding of this patches.
Thank you. I understand.
(and I need to send fixup patches :)
# "codec-is-bitclock-master" seems understandable than
# current "bitclock-master"...

Sounds great to me!

Best regards,
Jyri

Kuninori Morimoto

2014-04-14 05:07 UTC #9 lore raw

Hi Jyri, Mark

Quoted 21 lines

Quoted 21 lines
Quoted 20 lines
Quoted 14 lines
but anyway, if my understanding is correct,
         simple-audio-card,cpu {
                 ...
                 bitclock-master;
                 frame-master;
         };

         simple-audio-card,codec {
                 ...
                 bitclock-master;
                 frame-master;
         };
Quoted 4 lines
This will be
cpu : SND_SOC_DAIFMT_CBS_CFS
codec : SND_SOC_DAIFMT_CBM_CFM
Yes, That's also what my understanding of this patches.

Quoted 8 lines

Quoted 6 lines
Thank you. I understand.
(and I need to send fixup patches :)
# "codec-is-bitclock-master" seems understandable than
# current "bitclock-master"...
Sounds great to me!

Could you please teach me current status of this patch ?

Jyri Sarha

author

2014-04-14 09:53 UTC #10 lore raw

Quoted 3 lines · Kuninori Morimoto

Hi Jyri, Mark

...

Quoted 1 line

Could you please teach me current status of this patch ?

Hi,
This patch-set was never merged as such. Instead there is another patch-set that has been applied by Mark to cover this issue [1]. The patch maintains backward compatibility, with the Nicolin Chen's patch [2]. In other words, old style "bitclock-master" and "frame-master" boolean parameters are read from the codec node if they can not be found from the top level node. The backward compatibility does not apply to multi-link [3] configurations.

Best regards,
Jyri

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-March/074717.html

[2] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-March/074219.html

[3] http://mailman.alsa-project.org/pipermail/alsa-devel/2014-March/074592.html

ps. The discussions that lead to the current solution can be found here:
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-March/074388.html

Kuninori Morimoto

2014-04-15 00:23 UTC #11 lore raw

Hi Jyri

Quoted 7 lines

This patch-set was never merged as such. Instead there is another patch-set that has been applied by Mark to cover this issue [1]. The patch maintains backward compatibility, with the Nicolin Chen's patch [2]. In other words, old style "bitclock-master" and "frame-master" boolean parameters are read from the codec node if they can not be found from the top level node. The backward compatibility does not apply to multi-link [3] configurations.

Thank you !
I could find them

Mark Brown

maintainer

2014-03-12 12:30 UTC #12 lore raw

In reply to Richard Lee

Quoted 12 lines · Richard Lee

But, IMO, if you want the CPU DAI be CBS_CFS and CODEC be CBM_CFM, you could just do it like this:
         simple-audio-card,cpu {
                 ...
         };

         simple-audio-card,codec {
                 ...
                 bitclock-master;
                 frame-master;
         };

Quoted 5 lines

and vice versa.
Thanks,
(I could find this mails in my Freescale acount, so I will reply it here.)

Yes, that'd been what I'd thought the binding did (and it's what Jyri's patch makes it do).