[PATCH 0/4] ASoC: support dai-links with symmetric clock roles

[PATCH 0/4] ASoC: support dai-links with symmetric clock roles

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

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

[PATCH 1/4] ASoC: dt-bindings: document new symmetric-clock-role flag

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

The new flag specifies that both ends of the dai-link have the same
clock consumer/provider role. This should be used to describe hardware
where e.g. the CPU and codec both receive their bit- and frame-clocks
from an external source.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 .../devicetree/bindings/sound/simple-card.yaml        | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml
index b05e05c81cc4..ce738d1a394d 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.yaml
+++ b/Documentation/devicetree/bindings/sound/simple-card.yaml
@@ -27,6 +27,11 @@ definitions:
     description: dai-link uses bit clock inversion
     $ref: /schemas/types.yaml#/definitions/flag
 
+  symmetric-clock-roles:
+    description: |
+    dai-link uses same clock consumer/provider role for both CPU and Codec
+    $ref: /schemas/types.yaml#/definitions/flag
+
   dai-tdm-slot-num:
     description: see tdm-slot.txt.
     $ref: /schemas/types.yaml#/definitions/uint32
@@ -128,6 +133,8 @@ definitions:
         $ref: "#/definitions/frame-inversion"
       bitclock-inversion:
         $ref: "#/definitions/bitclock-inversion"
+      symmetric-clock-roles:
+        $ref: "#/definitions/symmetric-clock-roles"
       frame-master:
         $ref: /schemas/types.yaml#/definitions/flag
       bitclock-master:
@@ -181,6 +188,8 @@ properties:
     $ref: "#/definitions/frame-inversion"
   simple-audio-card,bitclock-inversion:
     $ref: "#/definitions/bitclock-inversion"
+  simple-audio-card,symmetric-clock-roles:
+    $ref: "#/definitions/symmetric-clock-roles"
   simple-audio-card,format:
     $ref: "#/definitions/format"
   simple-audio-card,mclk-fs:
@@ -230,6 +239,8 @@ patternProperties:
         $ref: "#/definitions/frame-inversion"
       bitclock-inversion:
         $ref: "#/definitions/bitclock-inversion"
+      symmetric-clock-roles:
+        $ref: "#/definitions/symmetric-clock-roles"
       format:
         $ref: "#/definitions/format"
       mclk-fs:
-- 
2.40.0

[PATCH 2/4] ASoC: core: add support for dai-links with symmetric clock roles

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

The snd_soc_dai_link::dai_fmt field contains the nominal bit- and
frame-clock consumer/provider role from the point of view of the codec.
The ASoC core then assumes that it should flip the roles when
initializing the format on the CPU. But in cases where both the CPU and
codec are clock consumers, e.g. because of an external clock source,
this assumption breaks down.

To allow for proper configuration of the backing CPU/codec drivers for
consumer roles, introduce support for a symmetric_clock_roles flag. The
role flipping will be skipped when this flag is set.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 include/sound/soc.h  | 3 +++
 sound/soc/soc-core.c | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 533e553a343f..87f4fb3d4b20 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -718,6 +718,9 @@ struct snd_soc_dai_link {
 	/* Keep DAI active over suspend */
 	unsigned int ignore_suspend:1;
 
+	/* Assume CPU/Codec have the same clock consumer/provider role */
+	unsigned int symmetric_clock_roles:1;
+
 	/* Symmetry requirements */
 	unsigned int symmetric_rate:1;
 	unsigned int symmetric_channels:1;
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b48efc3a08d2..7817b86dd93d 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1216,6 +1216,7 @@ static void snd_soc_runtime_get_dai_fmt(struct snd_soc_pcm_runtime *rtd)
 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;
@@ -1231,7 +1232,8 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd,
 	}
 
 	/* Flip the polarity for the "CPU" end of link */
-	dai_fmt = snd_soc_daifmt_clock_provider_flipped(dai_fmt);
+	if (!dai_link->symmetric_clock_roles)
+		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);
-- 
2.40.0

[PATCH 3/4] ASoC: audio-graph-card2: parse symmetric-clock-roles property

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

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 <alsi@bang-olufsen.dk>
---
 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

[PATCH 4/4] ASoC: simple-card: parse symmetric-clock-roles property

[Alvin Šipraga]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

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 <alsi@bang-olufsen.dk>
---
 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

Re: [PATCH 1/4] ASoC: dt-bindings: document new symmetric-clock-role flag

[Rob Herring]

On Fri, 02 Jun 2023 11:03:18 +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> The new flag specifies that both ends of the dai-link have the same
> clock consumer/provider role. This should be used to describe hardware
> where e.g. the CPU and codec both receive their bit- and frame-clocks
> from an external source.
> 
> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> ---
>  .../devicetree/bindings/sound/simple-card.yaml        | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/sound/simple-card.yaml:33:5: [error] syntax error: could not find expected ':' (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/sound/simple-card.example.dts'
Documentation/devicetree/bindings/sound/simple-card.yaml:33:5: could not find expected ':'
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/sound/simple-card.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/sound/simple-card.yaml:33:5: could not find expected ':'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/sound/simple-card.yaml: ignoring, error parsing file
make: *** [Makefile:1512: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230602090322.1876359-2-alvin@pqrs.dk

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/4] ASoC: dt-bindings: document new symmetric-clock-role flag

[Mark Brown]

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

On Fri, Jun 02, 2023 at 11:03:18AM +0200, Alvin Šipraga wrote:
> From: Alvin Šipraga <alsi@bang-olufsen.dk>
> 
> The new flag specifies that both ends of the dai-link have the same
> clock consumer/provider role. This should be used to describe hardware
> where e.g. the CPU and codec both receive their bit- and frame-clocks
> from an external source.

Why would we have a property for this and not just describe whatever the
actual clocking arrangement is?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] ASoC: dt-bindings: document new symmetric-clock-role flag

[Alvin Šipraga]

Hi Mark,

On Fri, Jun 02, 2023 at 12:43:51PM +0100, Mark Brown wrote:
> On Fri, Jun 02, 2023 at 11:03:18AM +0200, Alvin Šipraga wrote:
> > From: Alvin Šipraga <alsi@bang-olufsen.dk>
> > 
> > The new flag specifies that both ends of the dai-link have the same
> > clock consumer/provider role. This should be used to describe hardware
> > where e.g. the CPU and codec both receive their bit- and frame-clocks
> > from an external source.
> 
> Why would we have a property for this and not just describe whatever the
> actual clocking arrangement is?

Sure - let me just elaborate on my thinking and maybe you can help me with a
better approach:

The clocking arrangement is encoded in the dai_fmt field of snd_soc_dai_link,
but this is a single value that describes the format on both ends. The current
behaviour of ASoC is to flip the clock roles encoded in dai_fmt when applying it
to the CPU side of the link.

Looking from a DT perspective, if I do not specify e.g. bitclock-master on
either side of the link, then the dai_fmt will describe the codec as a bitclock
consumer and (after flipping) the CPU as a provider. That's the default
implication of the DT bindings and I can't break compatibility there.

The other issue is that for the simple-card the DAI format is only parsed in one
place and applied to the whole link. Are you proposing that it be modified to
explicitly try and parse both ends in order to determine if both sides want to
be clock consumers? In that case I'd have to also introduce bitclock-consumer
and frameclock-consumer properties to mirror the existing bitclock-master and
frameclock-master properties, as an explicit absence of the *-master property on
both sides would have to default to the original ASoC behaviour described above.

Or did you have something else in mind?

Thanks for your review.

Kind regards,
Alvin

Re: [PATCH 1/4] ASoC: dt-bindings: document new symmetric-clock-role flag

[Mark Brown]

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

On Fri, Jun 02, 2023 at 12:12:52PM +0000, Alvin Šipraga wrote:
> On Fri, Jun 02, 2023 at 12:43:51PM +0100, Mark Brown wrote:

> > Why would we have a property for this and not just describe whatever the
> > actual clocking arrangement is?

> Sure - let me just elaborate on my thinking and maybe you can help me with a
> better approach:

> The clocking arrangement is encoded in the dai_fmt field of snd_soc_dai_link,
> but this is a single value that describes the format on both ends. The current
> behaviour of ASoC is to flip the clock roles encoded in dai_fmt when applying it
> to the CPU side of the link.

> Looking from a DT perspective, if I do not specify e.g. bitclock-master on
> either side of the link, then the dai_fmt will describe the codec as a bitclock
> consumer and (after flipping) the CPU as a provider. That's the default
> implication of the DT bindings and I can't break compatibility there.

None of this addresses my question.  To repeat why would we not just
describe the actual clocking arrangement here - this property does not
specify where the clock actually comes from at all, we're still going to
need additional information for that and if we've described that clock
then we already know it's there without having to specify any more
properties.

> The other issue is that for the simple-card the DAI format is only parsed in one
> place and applied to the whole link. Are you proposing that it be modified to
> explicitly try and parse both ends in order to determine if both sides want to
> be clock consumers? In that case I'd have to also introduce bitclock-consumer
> and frameclock-consumer properties to mirror the existing bitclock-master and
> frameclock-master properties, as an explicit absence of the *-master property on
> both sides would have to default to the original ASoC behaviour described above.

If simple-card can't be made to work that's fine, it's deprecated
anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] ASoC: dt-bindings: document new symmetric-clock-role flag

[Alvin Šipraga]

On Fri, Jun 02, 2023 at 01:19:08PM +0100, Mark Brown wrote:
> On Fri, Jun 02, 2023 at 12:12:52PM +0000, Alvin Šipraga wrote:
> > On Fri, Jun 02, 2023 at 12:43:51PM +0100, Mark Brown wrote:
> 
> > > Why would we have a property for this and not just describe whatever the
> > > actual clocking arrangement is?
> 
> > Sure - let me just elaborate on my thinking and maybe you can help me with a
> > better approach:
> 
> > The clocking arrangement is encoded in the dai_fmt field of snd_soc_dai_link,
> > but this is a single value that describes the format on both ends. The current
> > behaviour of ASoC is to flip the clock roles encoded in dai_fmt when applying it
> > to the CPU side of the link.
> 
> > Looking from a DT perspective, if I do not specify e.g. bitclock-master on
> > either side of the link, then the dai_fmt will describe the codec as a bitclock
> > consumer and (after flipping) the CPU as a provider. That's the default
> > implication of the DT bindings and I can't break compatibility there.
> 
> None of this addresses my question.  To repeat why would we not just
> describe the actual clocking arrangement here - this property does not
> specify where the clock actually comes from at all, we're still going to
> need additional information for that and if we've described that clock
> then we already know it's there without having to specify any more
> properties.

Yes I see what you mean. On my platform the clock source is actually described
by the common clock framework, so I would want to use that. If it were a
component driver then it would most likely be a codec that is part of the
dai-link anyway. So what about having two struct clk pointers in struct
snd_soc_dai?

    struct snd_soc_dai {
        /* ... */
        struct clk *bitclock_provider;
        struct clk *frameclock_provider;
        /* ... */
    };

If non-NULL I could then have the ASoC core enable/disable the clocks on demand?
I would say in hw_params/hw_free, albeit that runs after set_fmt.

Having said that, I see ASoC doesn't really use the CCF much... am I way off?

I don't think it's feasible to modify every component driver to explicitly
handle this and then ignore any CBP_CFP bits set in its call to set_fmt - this
is why I want help from the ASoC core.

> 
> > The other issue is that for the simple-card the DAI format is only parsed in one
> > place and applied to the whole link. Are you proposing that it be modified to
> > explicitly try and parse both ends in order to determine if both sides want to
> > be clock consumers? In that case I'd have to also introduce bitclock-consumer
> > and frameclock-consumer properties to mirror the existing bitclock-master and
> > frameclock-master properties, as an explicit absence of the *-master property on
> > both sides would have to default to the original ASoC behaviour described above.
> 
> If simple-card can't be made to work that's fine, it's deprecated
> anyway.

Ah OK, I didn't know that. Right now I'm using graph-card2, that's not
deprecated, right?

Kind regards,
Alvin

Re: [PATCH 4/4] ASoC: simple-card: parse symmetric-clock-roles property

[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

Re: [PATCH 3/4] ASoC: audio-graph-card2: parse symmetric-clock-roles property

[Kuninori Morimoto]

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

Re: [PATCH 3/4] ASoC: audio-graph-card2: parse symmetric-clock-roles property

[Alvin Šipraga]

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

Re: [PATCH 4/4] ASoC: simple-card: parse symmetric-clock-roles property

[Alvin Šipraga]

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

Re: [PATCH 1/4] ASoC: dt-bindings: document new symmetric-clock-role flag

[Alvin Šipraga]

On Fri, Jun 02, 2023 at 01:19:08PM +0100, Mark Brown wrote:
> On Fri, Jun 02, 2023 at 12:12:52PM +0000, Alvin Šipraga wrote:
> > On Fri, Jun 02, 2023 at 12:43:51PM +0100, Mark Brown wrote:
> 
> > > Why would we have a property for this and not just describe whatever the
> > > actual clocking arrangement is?
> 
> > Sure - let me just elaborate on my thinking and maybe you can help me with a
> > better approach:
> 
> > The clocking arrangement is encoded in the dai_fmt field of snd_soc_dai_link,
> > but this is a single value that describes the format on both ends. The current
> > behaviour of ASoC is to flip the clock roles encoded in dai_fmt when applying it
> > to the CPU side of the link.
> 
> > Looking from a DT perspective, if I do not specify e.g. bitclock-master on
> > either side of the link, then the dai_fmt will describe the codec as a bitclock
> > consumer and (after flipping) the CPU as a provider. That's the default
> > implication of the DT bindings and I can't break compatibility there.
> 
> None of this addresses my question.  To repeat why would we not just
> describe the actual clocking arrangement here - this property does not
> specify where the clock actually comes from at all, we're still going to
> need additional information for that and if we've described that clock
> then we already know it's there without having to specify any more
> properties.

Maybe I overcomplicated your point with my previous reply. Some questions to
clarify:

1. You don't like the DT property because it should be inferrable by other
means. Correct?

2. As for the flag added to snd_soc_dai_link, do you think that is an OK
approach?

Just want to understand which direction you would like me to focus the
effort.

Kind regards,
Alvin

Re: [PATCH 1/4] ASoC: dt-bindings: document new symmetric-clock-role flag

[Mark Brown]

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

On Fri, Jun 02, 2023 at 12:42:49PM +0000, Alvin Šipraga wrote:

> Yes I see what you mean. On my platform the clock source is actually described
> by the common clock framework, so I would want to use that. If it were a
> component driver then it would most likely be a codec that is part of the
> dai-link anyway. So what about having two struct clk pointers in struct
> snd_soc_dai?
> 
>     struct snd_soc_dai {
>         /* ... */
>         struct clk *bitclock_provider;
>         struct clk *frameclock_provider;
>         /* ... */
>     };

> If non-NULL I could then have the ASoC core enable/disable the clocks on demand?
> I would say in hw_params/hw_free, albeit that runs after set_fmt.

hw_params() can be called repeatedly so that's not a good fit but
broadly yes.

> Having said that, I see ASoC doesn't really use the CCF much... am I way off?

Ideally we'd be representing more of the clocking via the clock
framework but at present yes.

> I don't think it's feasible to modify every component driver to explicitly
> handle this and then ignore any CBP_CFP bits set in its call to set_fmt - this
> is why I want help from the ASoC core.

Sure, but that's not going to impact the DT bindings.  All these things
are driven from the machine driver.

> > If simple-card can't be made to work that's fine, it's deprecated
> > anyway.

> Ah OK, I didn't know that. Right now I'm using graph-card2, that's not
> deprecated, right?

Yes, audio-graph-card replaces simple-card.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]