From: Alvin Šipraga <[email protected]>
Currently the ASoC core always assumes that one end of a dai-link is a
clock provider and the other a consumer. This series adds support for
configuring dai-links where both ends are actually clock consumers.
Alvin Šipraga (4):
ASoC: dt-bindings: document new symmetric-clock-role flag
ASoC: core: add support for dai-links with symmetric clock roles
ASoC: audio-graph-card2: parse symmetric-clock-roles property
ASoC: simple-card: parse symmetric-clock-roles property
.../devicetree/bindings/sound/simple-card.yaml | 11 +++++++++++
include/sound/soc.h | 3 +++
sound/soc/generic/audio-graph-card2.c | 7 ++++++-
sound/soc/generic/simple-card.c | 4 ++++
sound/soc/soc-core.c | 4 +++-
5 files changed, 27 insertions(+), 2 deletions(-)
--
2.40.0
From: Alvin Šipraga <[email protected]>
The property, when set, specifies that both ends of the dai-link should
have the same clock consumer/provider roles. Like with parsing of DAI
format, the property can be specified in multiple places:
ports {
(A)
port {
(B)
endpoint {
(C)
};
};
};
So each place has to be checked. In case the clock roles are symmetric,
there is then no need to flip the role when parsing the DAI format on
the CPU side, as it should then be the same on the Codec side.
Signed-off-by: Alvin Šipraga <[email protected]>
---
sound/soc/generic/audio-graph-card2.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/sound/soc/generic/audio-graph-card2.c b/sound/soc/generic/audio-graph-card2.c
index 25aa79dd55b3..9b4ebfd0c0b6 100644
--- a/sound/soc/generic/audio-graph-card2.c
+++ b/sound/soc/generic/audio-graph-card2.c
@@ -721,13 +721,18 @@ static void graph_link_init(struct asoc_simple_priv *priv,
if (of_node_name_eq(ports, "ports"))
graph_parse_daifmt(ports, &daifmt, &bit_frame); /* (A) */
+ if (of_property_read_bool(ep, "symmetric-clock-roles") ||
+ of_property_read_bool(port, "symmetric-clock-roles") ||
+ of_property_read_bool(ports, "symmetric-clock-roles"))
+ dai_link->symmetric_clock_roles = 1;
+
/*
* convert bit_frame
* We need to flip clock_provider if it was CPU node,
* because it is Codec base.
*/
daiclk = snd_soc_daifmt_clock_provider_from_bitmap(bit_frame);
- if (is_cpu_node)
+ if (is_cpu_node && !dai_link->symmetric_clock_roles)
daiclk = snd_soc_daifmt_clock_provider_flipped(daiclk);
dai_link->dai_fmt = daifmt | daiclk;
--
2.40.0
From: Alvin Šipraga <[email protected]>
The property, when set, specifies that both ends of the dai-link should
have the same clock consumer/provider roles. As with other simple-card
properties, a prefix can be specified.
Signed-off-by: Alvin Šipraga <[email protected]>
---
sound/soc/generic/simple-card.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 5a5e4ecd0f61..4513e30948b7 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -181,6 +181,7 @@ static int simple_link_init(struct asoc_simple_priv *priv,
{
struct device *dev = simple_priv_to_dev(priv);
struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, li->link);
+ char prop[128];
int ret;
ret = asoc_simple_parse_daifmt(dev, node, codec,
@@ -188,6 +189,9 @@ static int simple_link_init(struct asoc_simple_priv *priv,
if (ret < 0)
return 0;
+ snprintf(prop, sizeof(prop), "%ssymmetric-clock-roles", prefix);
+ dai_link->symmetric_clock_roles = of_property_read_bool(node, prop);
+
dai_link->init = asoc_simple_dai_init;
dai_link->ops = &simple_ops;
--
2.40.0
Hi Alvin
> --- a/sound/soc/generic/audio-graph-card2.c
> +++ b/sound/soc/generic/audio-graph-card2.c
(snip)
> /*
> * convert bit_frame
> * We need to flip clock_provider if it was CPU node,
> * because it is Codec base.
> */
> daiclk = snd_soc_daifmt_clock_provider_from_bitmap(bit_frame);
> - if (is_cpu_node)
> + if (is_cpu_node && !dai_link->symmetric_clock_roles)
> daiclk = snd_soc_daifmt_clock_provider_flipped(daiclk);
>
> dai_link->dai_fmt = daifmt | daiclk;
Hmm ? I'm confusing
[2/4] patch handling fliping, and [3/4] also handling fliping.
Nothing changed ?
Current SND_SOC_DAIFMT_xx_xx are very confusable,
framework and driver are different (flipped).
and also, audio-graph-card2 is using intuitive DT settings
(need flip for CPU).
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Hi Alvin
Thank you for the patch
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -181,6 +181,7 @@ static int simple_link_init(struct asoc_simple_priv *priv,
> {
> struct device *dev = simple_priv_to_dev(priv);
> struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, li->link);
> + char prop[128];
> int ret;
>
> ret = asoc_simple_parse_daifmt(dev, node, codec,
> @@ -188,6 +189,9 @@ static int simple_link_init(struct asoc_simple_priv *priv,
> if (ret < 0)
> return 0;
>
> + snprintf(prop, sizeof(prop), "%ssymmetric-clock-roles", prefix);
> + dai_link->symmetric_clock_roles = of_property_read_bool(node, prop);
> +
> dai_link->init = asoc_simple_dai_init;
> dai_link->ops = &simple_ops;
looks good to me.
simple-card / audio-graph-card / audio-graph-card2 want to support same settings
(But unfortunately it is not completely synchronized...).
Could you please add same settings or indicate it on the comment
(like /* FIXME support symmetric-clock-roles here */, etc)
on audio-graph-card, if you create v2 patch ?
Thank you for your help !!
Best regards
---
Kuninori Morimoto
Hi Kuninori,
On Mon, Jun 05, 2023 at 12:28:14AM +0000, Kuninori Morimoto wrote:
>
> Hi Alvin
>
> Thank you for the patch
>
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c
> > @@ -181,6 +181,7 @@ static int simple_link_init(struct asoc_simple_priv *priv,
> > {
> > struct device *dev = simple_priv_to_dev(priv);
> > struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, li->link);
> > + char prop[128];
> > int ret;
> >
> > ret = asoc_simple_parse_daifmt(dev, node, codec,
> > @@ -188,6 +189,9 @@ static int simple_link_init(struct asoc_simple_priv *priv,
> > if (ret < 0)
> > return 0;
> >
> > + snprintf(prop, sizeof(prop), "%ssymmetric-clock-roles", prefix);
> > + dai_link->symmetric_clock_roles = of_property_read_bool(node, prop);
> > +
> > dai_link->init = asoc_simple_dai_init;
> > dai_link->ops = &simple_ops;
>
> looks good to me.
>
> simple-card / audio-graph-card / audio-graph-card2 want to support same settings
> (But unfortunately it is not completely synchronized...).
>
> Could you please add same settings or indicate it on the comment
> (like /* FIXME support symmetric-clock-roles here */, etc)
> on audio-graph-card, if you create v2 patch ?
>
> Thank you for your help !!
Sure. If I send a v2, I will add a patch for audio-graph-card as well. :)
Kind regards,
Alvin
Hi Kuninori,
On Mon, Jun 05, 2023 at 12:35:56AM +0000, Kuninori Morimoto wrote:
>
> Hi Alvin
>
> > --- a/sound/soc/generic/audio-graph-card2.c
> > +++ b/sound/soc/generic/audio-graph-card2.c
> (snip)
> > /*
> > * convert bit_frame
> > * We need to flip clock_provider if it was CPU node,
> > * because it is Codec base.
> > */
> > daiclk = snd_soc_daifmt_clock_provider_from_bitmap(bit_frame);
> > - if (is_cpu_node)
> > + if (is_cpu_node && !dai_link->symmetric_clock_roles)
> > daiclk = snd_soc_daifmt_clock_provider_flipped(daiclk);
> >
> > dai_link->dai_fmt = daifmt | daiclk;
>
> Hmm ? I'm confusing
> [2/4] patch handling fliping, and [3/4] also handling fliping.
> Nothing changed ?
Yes, I agree it seems wrong. Let me try and explain what is going on.
First take a look at the original snd_soc_runtime_set_dai_fmt:
int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
unsigned int dai_fmt)
{
struct snd_soc_dai_link *dai_link = rtd->dai_link;
struct snd_soc_dai *cpu_dai;
struct snd_soc_dai *codec_dai;
unsigned int i;
int ret;
if (!dai_fmt)
return 0;
for_each_rtd_codec_dais(rtd, i, codec_dai) {
ret = snd_soc_dai_set_fmt(codec_dai, dai_fmt);
if (ret != 0 && ret != -ENOTSUPP)
return ret;
}
/* Flip the polarity for the "CPU" end of link */
dai_fmt = snd_soc_daifmt_clock_provider_flipped(dai_fmt);
for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt);
if (ret != 0 && ret != -ENOTSUPP)
return ret;
}
return 0;
}
... which is called by the core in soc_init_pcm_runtime:
static int soc_init_pcm_runtime(struct snd_soc_card *card,
struct snd_soc_pcm_runtime *rtd)
{
struct snd_soc_dai_link *dai_link = rtd->dai_link;
/* ... */
snd_soc_runtime_get_dai_fmt(rtd);
ret = snd_soc_runtime_set_dai_fmt(rtd, dai_link->dai_fmt);
if (ret)
return ret;
/* ... */
}
From the above I conclude that the clock role expressed by dai_link->dai_fmt is
"as applied to the codec", i.e. snd_soc_runtime_set_dai_fmt does not flip the
value before applying it on the codec side. When applying it to the CPU side,
the roles are flipped.
>
> Current SND_SOC_DAIFMT_xx_xx are very confusable,
> framework and driver are different (flipped).
> and also, audio-graph-card2 is using intuitive DT settings
> (need flip for CPU).
Now consider audio-graph-card2. Except for DPCM backends, it always parses the
DAI format on the CPU side. Since dai_link->dai_fmt is flipped by the ASoC core
when applying format to the CPU, card2 is flipping the parsed value before
storing it in dai_link->dai_fmt so that it is correct.
audio-graph-card2 -.
v
-1 * -1 = 1
^
'- ASoC core
But with patch 2/4 of this series, symmetric_clock_roles == 1 means that the
ASoC core doesn't flip it. In fact, it means that the role is the same both "as
applied to the codec" and "as applied to the CPU". So no flipping should happen,
neither in card2 nor in ASoC core. CPU and codec clock consumer/provider roles
are symmetric. e.g. if CPU is a bitclock consumer then so is the codec, and
nothing needs flipping.
I hope that is clear now.
Kind regards,
Alvin