[PATCH] ASoC: pcm3168a: Add option to force clock consumer

[PATCH] ASoC: pcm3168a: Add option to force clock consumer

[Stephen Gordon]

Scenario is that DAC's clock pins are tied to ADC's clock pins, with
codec acting as clock producer for the I2S bus, and the CPU has a single
pair of I2S clock pins used for both playback and capture.

Both DAI links will have codec acting as a clock producer, but we need
to configure the codec so that only one of ADC/DAC drives the lines.

Add DT options to support this configuration. adc-force-cons/dac-force-cons
cause MSAD/MSDA to be set to 0b000.

diff --git a/sound/soc/codecs/pcm3168a.c b/sound/soc/codecs/pcm3168a.c
index 9d6431338..5e3590f2d 100644
--- a/sound/soc/codecs/pcm3168a.c
+++ b/sound/soc/codecs/pcm3168a.c
@@ -61,6 +61,7 @@ struct pcm3168a_priv {
         struct clk *scki;
         struct gpio_desc *gpio_rst;
         unsigned long sysclk;
+       bool adc_fc, dac_fc; // Force clock consumer mode

         struct pcm3168a_io_params io_params[2];
         struct snd_soc_dai_driver dai_drv[2];
@@ -479,6 +480,12 @@ static int pcm3168a_hw_params(struct 
snd_pcm_substream *substream,
                 ms = 0;
         }

+       // Force clock consumer mode if needed
+       if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC)
+               ms = 0;
+       if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC)
+               ms = 0;
+
         format = io_params->format;

         if (io_params->slot_width)
@@ -757,6 +764,13 @@ int pcm3168a_probe(struct device *dev, struct 
regmap *regmap)

         pcm3168a->sysclk = clk_get_rate(pcm3168a->scki);

+       if (dev->of_node) {
+               pcm3168a->adc_fc = of_property_read_bool(dev->of_node,
+                               "adc-force-cons");
+               pcm3168a->dac_fc = of_property_read_bool(dev->of_node,
+                               "dac-force-cons");
+       }
+
         for (i = 0; i < ARRAY_SIZE(pcm3168a->supplies); i++)
                 pcm3168a->supplies[i].supply = pcm3168a_supply_names[i];



Signed-off-by: Stephen Gordon <gordoste@iinet.net.au>

[PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Stephen Gordon]

Hi,

Resending with signoff tag and including linux-sound on CC.

Scenario is that DAC's clock pins are tied to ADC's clock pins, with
codec acting as clock producer for the I2S bus, and the CPU has a single
pair of I2S clock pins used for both playback and capture.


                                  ,-------------------
---------            ------------| LRCK     DAC
      LRCK|-------+--/     -------| BCK   (producer)  C
CPU      |       |       /       |                   O
       BCK|-------|---+--/        | ----------------  D
consumer |       \   |           |                   E
---------         ---------------| LRCK     ADC      C
                      `-----------| BCK    (consumer)
                                  `--------------------

Both DAI links will have codec acting as a clock producer, but we need

to configure the codec so that only one of ADC/DAC drives the lines.


Add DT options to support this configuration. adc-force-cons/dac-force-cons

cause the corresponding component to be configured in consumer mode.


Signed-off-by: Stephen Gordon <gordoste@iinet.net.au>




diff --git a/sound/soc/codecs/pcm3168a.c b/sound/soc/codecs/pcm3168a.c
index 9d6431338..1a4cf8d63 100644
--- a/sound/soc/codecs/pcm3168a.c
+++ b/sound/soc/codecs/pcm3168a.c
@@ -61,6 +61,7 @@ struct pcm3168a_priv {
         struct clk *scki;
         struct gpio_desc *gpio_rst;
         unsigned long sysclk;
+       bool adc_fc, dac_fc; // Force clock consumer mode

         struct pcm3168a_io_params io_params[2];
         struct snd_soc_dai_driver dai_drv[2];
@@ -479,6 +480,12 @@ static int pcm3168a_hw_params(struct 
snd_pcm_substream *substream,
                 ms = 0;
         }

+       // Force clock consumer mode if needed
+       if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC)
+               ms = 0;
+       if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC)
+               ms = 0;
+
         format = io_params->format;

         if (io_params->slot_width)
@@ -757,6 +764,15 @@ int pcm3168a_probe(struct device *dev, struct 
regmap *regmap)

         pcm3168a->sysclk = clk_get_rate(pcm3168a->scki);

+       adc_fc = false;
+       dac_fc = false;
+       if (dev->of_node) {
+               pcm3168a->adc_fc = of_property_read_bool(dev->of_node,
+                               "adc-force-cons");
+               pcm3168a->dac_fc = of_property_read_bool(dev->of_node,
+                               "dac-force-cons");
+       }
+
         for (i = 0; i < ARRAY_SIZE(pcm3168a->supplies); i++)
                 pcm3168a->supplies[i].supply = pcm3168a_supply_names[i];

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Mark Brown]

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

On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote:
> Hi,
> 
> Resending with signoff tag and including linux-sound on CC.

As covered in submitting-patches.rst please put any administrative
information after the --- so that it is is automatically removed by
tooling.  There is no need to resubmit for this alone.

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

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Mark Brown]

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

On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote:

> Scenario is that DAC's clock pins are tied to ADC's clock pins, with
> codec acting as clock producer for the I2S bus, and the CPU has a single
> pair of I2S clock pins used for both playback and capture.

>                                  ,-------------------
> ---------            ------------| LRCK     DAC
>      LRCK|-------+--/     -------| BCK   (producer)  C
> CPU      |       |       /       |                   O
>       BCK|-------|---+--/        | ----------------  D
> consumer |       \   |           |                   E
> ---------         ---------------| LRCK     ADC      C
>                      `-----------| BCK    (consumer)
>                                  `--------------------

> Both DAI links will have codec acting as a clock producer, but we need
> 
> to configure the codec so that only one of ADC/DAC drives the lines.

This looks like something that should be done in the board
configuration, not hard coded in the CODEC driver.

> Add DT options to support this configuration. adc-force-cons/dac-force-cons
> 
> cause the corresponding component to be configured in consumer mode.

Any DT bindings should documented which should be done in a separate
patch.


> +       // Force clock consumer mode if needed
> +       if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC)
> +               ms = 0;
> +       if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC)
> +               ms = 0;

The clock consumer mode should just be configured via the standard
set_dai_fmt() operation.

> +       if (dev->of_node) {
> +               pcm3168a->adc_fc = of_property_read_bool(dev->of_node,
> +                               "adc-force-cons");
> +               pcm3168a->dac_fc = of_property_read_bool(dev->of_node,
> +                               "dac-force-cons");
> +       }

These properties are not documented, and you don't need to query to see
if there's an OF node to query if a property is present.

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

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Stephen Gordon]

On 6/12/2024 11:22 pm, Mark Brown wrote:
> On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote:
>
>> Scenario is that DAC's clock pins are tied to ADC's clock pins, with
>> codec acting as clock producer for the I2S bus, and the CPU has a single
>> pair of I2S clock pins used for both playback and capture.
>>                                   ,-------------------
>> ---------            ------------| LRCK     DAC
>>       LRCK|-------+--/     -------| BCK   (producer)  C
>> CPU      |       |       /       |                   O
>>        BCK|-------|---+--/        | ----------------  D
>> consumer |       \   |           |                   E
>> ---------         ---------------| LRCK     ADC      C
>>                       `-----------| BCK    (consumer)
>>                                   `--------------------
>> Both DAI links will have codec acting as a clock producer, but we need
>>
>> to configure the codec so that only one of ADC/DAC drives the lines.
> This looks like something that should be done in the board
> configuration, not hard coded in the CODEC driver.
>
>> Add DT options to support this configuration. adc-force-cons/dac-force-cons
>>
>> cause the corresponding component to be configured in consumer mode.
> Any DT bindings should documented which should be done in a separate
> patch.

I will most certainly do this in v3 if I convince you that the DT 
bindings should be

added in the first place :)

>
>
>> +       // Force clock consumer mode if needed
>> +       if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC)
>> +               ms = 0;
>> +       if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC)
>> +               ms = 0;
> The clock consumer mode should just be configured via the standard
> set_dai_fmt() operation.

I did try to do this using simple_card and I documented my attempt and 
why it seems to

fail in this message:

https://lore.kernel.org/linux-sound/b64a630f-d71f-49ee-a5e7-ea1e3a7f8de5@iinet.net.au/

Basically, simple_card appears to set the CPU as producer if you don't 
specify a

producer. I am not sure whether this is a bug.

Also, there aren't any examples I could find in the documentation of DAI 
links with

clock consumer on both ends, so I was wondering if it is even valid.

There is some SoC code that seems like it might not work properly in 
this scenario:

https://github.com/torvalds/linux/blob/b8f52214c61a5b99a54168145378e91b40d10c90/sound/soc/soc-core.c#L1465


>> +       if (dev->of_node) {
>> +               pcm3168a->adc_fc = of_property_read_bool(dev->of_node,
>> +                               "adc-force-cons");
>> +               pcm3168a->dac_fc = of_property_read_bool(dev->of_node,
>> +                               "dac-force-cons");
>> +       }
> These properties are not documented, and you don't need to query to see
> if there's an OF node to query if a property is present.

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Mark Brown]

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

On Sat, Dec 07, 2024 at 12:16:28AM +1100, Stephen Gordon wrote:
> On 6/12/2024 11:22 pm, Mark Brown wrote:
> > On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote:

> > > +       // Force clock consumer mode if needed
> > > +       if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC)
> > > +               ms = 0;
> > > +       if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC)
> > > +               ms = 0;
> > The clock consumer mode should just be configured via the standard
> > set_dai_fmt() operation.

> I did try to do this using simple_card and I documented my attempt and why
> it seems to
> 
> fail in this message:

You should use one of the audio-graph-card bindings for anything new.

You probably want to fix the formatting in your mail client, it's
doing somet very weird line wrapping and inserting spurious blank lines.

> https://lore.kernel.org/linux-sound/b64a630f-d71f-49ee-a5e7-ea1e3a7f8de5@iinet.net.au/

> Basically, simple_card appears to set the CPU as producer if you don't
> specify a

> producer. I am not sure whether this is a bug.

Well, if nothing is configured it's got to pick a default?

> Also, there aren't any examples I could find in the documentation of DAI
> links with
> 
> clock consumer on both ends, so I was wondering if it is even valid.

No, that's not valid.  The CODEC is the clock provider here and should
understand that.  I *think* the audio graph cards can handle this case
by having both CODEC DAIs on a single PCM but I could be misremembering
and it may only be the DPCM cases.

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

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Stephen Gordon]

On Fri, 6 Dec 2024 14:13:14 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Sat, Dec 07, 2024 at 12:16:28AM +1100, Stephen Gordon wrote:
> > On 6/12/2024 11:22 pm, Mark Brown wrote:  
> > > On Fri, Dec 06, 2024 at 10:54:43PM +1100, Stephen Gordon wrote:  
> 
> > > > +       // Force clock consumer mode if needed
> > > > +       if (pcm3168a->adc_fc && dai->id == PCM3168A_DAI_ADC)
> > > > +               ms = 0;
> > > > +       if (pcm3168a->dac_fc && dai->id == PCM3168A_DAI_DAC)
> > > > +               ms = 0;  
> > > The clock consumer mode should just be configured via the standard
> > > set_dai_fmt() operation.  
> 
> > I did try to do this using simple_card and I documented my attempt
> > and why it seems to
> > 
> > fail in this message:  
> 
> You should use one of the audio-graph-card bindings for anything new.
> 
> You probably want to fix the formatting in your mail client, it's
> doing somet very weird line wrapping and inserting spurious blank
> lines.
> 
> > https://lore.kernel.org/linux-sound/b64a630f-d71f-49ee-a5e7-ea1e3a7f8de5@iinet.net.au/
> >  
> 
> > Basically, simple_card appears to set the CPU as producer if you
> > don't specify a  
> 
> > producer. I am not sure whether this is a bug.  
> 
> Well, if nothing is configured it's got to pick a default?
> 
> > Also, there aren't any examples I could find in the documentation
> > of DAI links with
> > 
> > clock consumer on both ends, so I was wondering if it is even
> > valid.  
> 
> No, that's not valid.  The CODEC is the clock provider here and should
> understand that.  I *think* the audio graph cards can handle this case
> by having both CODEC DAIs on a single PCM but I could be
> misremembering and it may only be the DPCM cases.

Thanks for confirming that a link must have a producer on one end. The
challenge is to configure it so that the codec end of the DAI link is
a producer, but the dai_fmt on the codec's DAI gets set to consumer.

I'll try to do this using the audio graph cards.

Hopefully the formatting is better, I have changed mail clients.

Stephen

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Kuninori Morimoto]

Hi Stephen

> > You should use one of the audio-graph-card bindings for anything new.

Using audio-graph-card is good idea, but all
simple_card/audio-graph-card/audio-graph-card2 are using same logic around here.
So, you will have same issue on audio-graph-card too.

> > > Basically, simple_card appears to set the CPU as producer if you
> > > don't specify a producer. I am not sure whether this is a bug.  
> > 
> > Well, if nothing is configured it's got to pick a default?

If my understand was correct, your issue can be solved...

	dailink_out_master: simple-audio-card,dai-link@0 {
		...
=>		pcm3168_playback: codec {
			...
		};
	};
	dailink_in_slave: simple-audio-card,dai-link@1 {
=>		bitclock-master = <&pcm3168_playback>;
=>		frame-master    = <&pcm3168_playback>;
		...
	};

asoc_simple_parse_daifmt() is just checking where the node was codec node
or not. So, if bitclock-master/frame-master were produced, but was not codec,
both CPU/Codec can be consumer ?

# Clock producer/consumer settings is very confusable, because it was
# Codec base, and has flip, etc...

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Stephen Gordon]

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

On Mon, 9 Dec 2024 01:58:59 +0000
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Stephen
> 
> > > You should use one of the audio-graph-card bindings for anything
> > > new.  
> 
> Using audio-graph-card is good idea, but all
> simple_card/audio-graph-card/audio-graph-card2 are using same logic
> around here. So, you will have same issue on audio-graph-card too.
> 
> > > > Basically, simple_card appears to set the CPU as producer if you
> > > > don't specify a producer. I am not sure whether this is a bug.
> > > >   
> > > 
> > > Well, if nothing is configured it's got to pick a default?  
> 
> If my understand was correct, your issue can be solved...
> 
> 	dailink_out_master: simple-audio-card,dai-link@0 {
> 		...
> =>		pcm3168_playback: codec {  
> 			...
> 		};
> 	};
> 	dailink_in_slave: simple-audio-card,dai-link@1 {
> =>		bitclock-master = <&pcm3168_playback>;
> =>		frame-master    = <&pcm3168_playback>;  
> 		...
> 	};
> 
> asoc_simple_parse_daifmt() is just checking where the node was codec
> node or not. So, if bitclock-master/frame-master were produced, but
> was not codec, both CPU/Codec can be consumer ?
> 
> # Clock producer/consumer settings is very confusable, because it was
> # Codec base, and has flip, etc...
> 
> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto

Hi Morimoto-san,

This is one of the things I tried initially, but I wasn't sure if it
was a valid configuration. 

I just tried it again with debug enabled (see dt_snippet.txt) and I get
the attached (see dbgout.txt) in dmesg.

It is trying to set the CPU end as producer. I think it's because
asoc_simple_parse_daifmt() checks whether the bit/frame phandles passed
match the codec phandle - since it doesn't (it's a different codec),
it sets the DAI link as "consumer mode" (i.e. clocks come from CPU).
Therefore, the CPU side gets configured as producer.

I am using the  version from 6.12.3 as that is the latest I can
build.

Regards
Stephen







[-- Attachment #2: dbgout.txt --]
[-- Type: text/plain, Size: 2692 bytes --]

[  688.239375] asoc-simple-card soc@107c000000:sound: link 2, dais 4, ccnf 0
[  688.239394] asoc-simple-card soc@107c000000:sound: link_of (/soc@107c000000/sound/simple-audio-card,dai-link@1)
[  688.267041] asoc-simple-card soc@107c000000:sound: link 2, dais 4, ccnf 0
[  688.267057] asoc-simple-card soc@107c000000:sound: link_of (/soc@107c000000/sound/simple-audio-card,dai-link@1)
[  688.267103] asoc-simple-card soc@107c000000:sound: link_of (/soc@107c000000/sound/simple-audio-card,dai-link@0)
[  688.267128] asoc-simple-card soc@107c000000:sound: Card Name: i2smulti
[  688.267131] asoc-simple-card soc@107c000000:sound: DAI0
[  688.267133] asoc-simple-card soc@107c000000:sound: cpu num = 1
[  688.267136] asoc-simple-card soc@107c000000:sound: cpu slots = 2
[  688.267138] asoc-simple-card soc@107c000000:sound: cpu slot width = 32
[  688.267141] asoc-simple-card soc@107c000000:sound: cpu sysclk = 50000000Hz
[  688.267143] asoc-simple-card soc@107c000000:sound: cpu direction = IN
[  688.267145] asoc-simple-card soc@107c000000:sound: codec num = 1
[  688.267147] asoc-simple-card soc@107c000000:sound: codec slots = 2
[  688.267149] asoc-simple-card soc@107c000000:sound: codec slot width = 32
[  688.267151] asoc-simple-card soc@107c000000:sound: codec sysclk = 24576000Hz
[  688.267153] asoc-simple-card soc@107c000000:sound: codec direction = IN
[  688.267155] asoc-simple-card soc@107c000000:sound: dai name = 1f000a4000.i2s-pcm3168a-adc
[  688.267157] asoc-simple-card soc@107c000000:sound: dai format = 4001
[  688.267160] asoc-simple-card soc@107c000000:sound: DAI1
[  688.267162] asoc-simple-card soc@107c000000:sound: cpu num = 1
[  688.267164] asoc-simple-card soc@107c000000:sound: cpu slots = 2
[  688.267166] asoc-simple-card soc@107c000000:sound: cpu slot width = 32
[  688.267168] asoc-simple-card soc@107c000000:sound: cpu sysclk = 50000000Hz
[  688.267170] asoc-simple-card soc@107c000000:sound: cpu direction = IN
[  688.267172] asoc-simple-card soc@107c000000:sound: codec num = 1
[  688.267174] asoc-simple-card soc@107c000000:sound: codec slots = 2
[  688.267175] asoc-simple-card soc@107c000000:sound: codec slot width = 32
[  688.267177] asoc-simple-card soc@107c000000:sound: codec sysclk = 24576000Hz
[  688.267179] asoc-simple-card soc@107c000000:sound: codec direction = IN
[  688.267181] asoc-simple-card soc@107c000000:sound: dai name = 1f000a4000.i2s-pcm3168a-dac
[  688.267183] asoc-simple-card soc@107c000000:sound: dai format = 1001
[  688.267424] designware-i2s 1f000a4000.i2s: ASoC: error at snd_soc_dai_set_fmt on 1f000a4000.i2s: -22
[  688.267596] asoc-simple-card soc@107c000000:sound: probe with driver asoc-simple-card failed with error -22

[-- Attachment #3: dt_snippet.txt --]
[-- Type: text/plain, Size: 1626 bytes --]

    fragment@2 {
        target = <&sound>;
        __overlay__ {
            compatible = "simple-audio-card";
            #address-cells = <1>;
            #size-cells = <0>;
            i2s-controller = <&i2s_clk_consumer>;
            status="okay";

            simple-audio-card,name = "i2smulti";
            simple-audio-card,format = "i2s";

            dailink_out_master: simple-audio-card,dai-link@0 {
                reg = <0>;
                format = "i2s";
                bitclock-master = <&pcm3168_playback>;
                frame-master = <&pcm3168_playback>;
                cpu  {
                    sound-dai = <&i2s_clk_consumer>;
                    dai-tdm-slot-num = <2>;
                    dai-tdm-slot-width = <32>;
                };
                pcm3168_playback: codec {
                    sound-dai = <&pcm3168a 0>;
                    dai-tdm-slot-num = <2>;
                    dai-tdm-slot-width = <32>;
                };
            };
            dailink_in_slave: simple-audio-card,dai-link@1 {
                reg = <1>;
                format = "i2s";
                bitclock-master = <&pcm3168_playback>;
                frame-master = <&pcm3168_playback>;
                cpu  {
                    sound-dai = <&i2s_clk_consumer>;
                    dai-tdm-slot-num = <2>;
                    dai-tdm-slot-width = <32>;
                };
                pcm3168_capture: codec {
                    sound-dai = <&pcm3168a 1>;
                    dai-tdm-slot-num = <2>;
                    dai-tdm-slot-width = <32>;
                };
             };
        };
    };

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Kuninori Morimoto]

Hi Mark
Cc Stephen

> It is trying to set the CPU end as producer. I think it's because
> asoc_simple_parse_daifmt() checks whether the bit/frame phandles passed
> match the codec phandle - since it doesn't (it's a different codec),
> it sets the DAI link as "consumer mode" (i.e. clocks come from CPU).
> Therefore, the CPU side gets configured as producer.
(snip)
> > # Clock producer/consumer settings is very confusable, because it was
> > # Codec base, and has flip, etc...

Ah.. indeed, but hmm...

Current ASoC provider/consumer settings for clock/frame is set via
dai_link->dai_fmt.

	static int soc_init_pcm_runtime(...)
	{
		...
=>		ret = snd_soc_runtime_set_dai_fmt(..., dai_link->dai_fmt);
		...
	}

And use it for both Codec (A) and CPU (B) with flipping (C)

	int snd_soc_runtime_set_dai_fmt(..., dai_fmt)
	{
		...
 ^		for_each_rtd_codec_dais(rtd, i, codec_dai) {
(A)			...
 v		}

		/* Flip the polarity for the "CPU" end of link */
(C)		dai_fmt = snd_soc_daifmt_clock_provider_flipped(dai_fmt);

 ^		for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
(B)			...
 v		}
		...
	}

So, I think we can't use both CPU/Codec as "consumer" (or "provider")
on current ASoC, but what do you think, Mark ?

Because of ASoC history, the clock/frame provider/consumer settings was
from Codec point of view (CBx_CFx), and "Sound Card" is still based on this.
OTOH, each CPU/Codec driver is now using own base (Bx_Fx).
Because of it we need flipping (C).

From "Sound Card" point of view, setup directly Bx_Fx base instead of
CBx_CFx base is not a big deal. But if we do it, we need to have such
setting for each DAI, instead of common dai_link.

Now dai_link->dai_fmt is including 4 type of info

	1. DAI hardware audio formats
	2. DAI Clock gating
	3. DAI hardware signal polarity
	4. DAI hardware clock providers/consumers

I think we want to separate 4 (and maybe 2 too ?) from it to support
more flexible system ?

Legacy system
	dai_link->dai_fmt		// include all 1, 2, 3, 4

New system
	dai_link->dai_fmt		// include 1, 2, 3 (or 1, 3)
	dai_link->cpus[idx].fmt		// include 4       (or 2, 4)

	fmt = dai_link->dai_fmt | dai_link->cpus[i].fmt

"flip" has effect to 4 only, so New system has no issue with flipping
on dai_link->dai_fmt.


> simple_card/audio-graph-card/audio-graph-card2 are using same logic around here.

I'm sorry, this was wrong.
simple_card/audio-graph-card are using same function/method, but
audio-graph-card2 is using own method to handing dai_link->dai_fmt

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Stephen Gordon]

>  From "Sound Card" point of view, setup directly Bx_Fx base instead of
> CBx_CFx base is not a big deal. But if we do it, we need to have such
> setting for each DAI, instead of common dai_link.
> 
> Now dai_link->dai_fmt is including 4 type of info
> 
> 	1. DAI hardware audio formats
> 	2. DAI Clock gating
> 	3. DAI hardware signal polarity
> 	4. DAI hardware clock providers/consumers
> 
> I think we want to separate 4 (and maybe 2 too ?) from it to support
> more flexible system ?
> 
> Legacy system
> 	dai_link->dai_fmt		// include all 1, 2, 3, 4
> 
> New system
> 	dai_link->dai_fmt		// include 1, 2, 3 (or 1, 3)
> 	dai_link->cpus[idx].fmt		// include 4       (or 2, 4)
> 
> 	fmt = dai_link->dai_fmt | dai_link->cpus[i].fmt
> 
> "flip" has effect to 4 only, so New system has no issue with flipping
> on dai_link->dai_fmt.

Hi Morimoto-san,

This is a great idea. I'd add that #4 could still be in 
dai_link->dai_fmt, but the new field in CPU/codec 
(snd_soc_dai_link_component.fmt) should override the setting (not a 
simple OR). This would preserve backward compatibility (I think). It 
greatly improves support for multiple codecs sharing clock lines, where 
1 codec is the producer and others are consumers. The bit/frame logic in 
simple/graph cards should be able to be changed to handle this properly too.

Without this type of override, the original patch is needed (or the card 
driver needs to write registers directly, which seems bad).

I will wait to hear Mark's opinion.

Stephen

Re: [PATCH v2] ASoC: pcm3168a: Add option to force clock consumer

[Kuninori Morimoto]

Hi Stephen, Mark

> >  From "Sound Card" point of view, setup directly Bx_Fx base instead of
> > CBx_CFx base is not a big deal. But if we do it, we need to have such
> > setting for each DAI, instead of common dai_link.
> > 
> > Now dai_link->dai_fmt is including 4 type of info
> > 
> > 	1. DAI hardware audio formats
> > 	2. DAI Clock gating
> > 	3. DAI hardware signal polarity
> > 	4. DAI hardware clock providers/consumers
> > 
> > I think we want to separate 4 (and maybe 2 too ?) from it to support
> > more flexible system ?
> > 
> > Legacy system
> > 	dai_link->dai_fmt		// include all 1, 2, 3, 4
> > 
> > New system
> > 	dai_link->dai_fmt		// include 1, 2, 3 (or 1, 3)
> > 	dai_link->cpus[idx].fmt		// include 4       (or 2, 4)
> > 
> > 	fmt = dai_link->dai_fmt | dai_link->cpus[i].fmt
> > 
> > "flip" has effect to 4 only, so New system has no issue with flipping
> > on dai_link->dai_fmt.
> 
> Hi Morimoto-san,
> 
> This is a great idea. I'd add that #4 could still be in 
> dai_link->dai_fmt, but the new field in CPU/codec 
> (snd_soc_dai_link_component.fmt) should override the setting (not a 
> simple OR). This would preserve backward compatibility (I think). It 
> greatly improves support for multiple codecs sharing clock lines, where 
> 1 codec is the producer and others are consumers. The bit/frame logic in 
> simple/graph cards should be able to be changed to handle this properly too.
> 
> Without this type of override, the original patch is needed (or the card 
> driver needs to write registers directly, which seems bad).
> 
> I will wait to hear Mark's opinion.

I'm now preparing the patch for it.
I want to more test it. I think I can post it next week if Mark is OK for it.


Thank you for your help !!

Best regards
---
Kuninori Morimoto