diff mbox series

[v4,10/23] ASoC: simple-card: Wrong daifmt for CPU end of DPCM DAI link

Message ID 1593233625-14961-11-git-send-email-spujar@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Add support for Tegra210 Audio | expand

Commit Message

Sameer Pujar June 27, 2020, 4:53 a.m. UTC
Consider following DPCM DAI link for example:

 simple-audio-card,dai-link@xxx {
     format = "i2s";
     bitclock-master=<&cpu1>;
     frame-master=<&cpu1>;

     cpu1: cpu@0 {
         ...
     };

     codec@0 {
         ...
     };

     ...
 };

In above case CPU is expected to be configured as a master and Codec as
a slave device. But both CPU/Codec are being configured as slave devices.
This happens because asoc_simple_parse_daifmt() uses Codec reference and
sets up the 'dai_link->dai_fmt' accordingly while parsing both CPU and
Codec. Though populating 'non_legacy_dai_naming' flag as true for CPU
component can address above issue in simple cases, but with multiple
CPU/Codecs with DPCM DAI link it becomes tricky because right now the
first Codec in the DAI link is used as reference.

This is fixed by passing current DAI link child node reference to
asoc_simple_parse_daifmt(). It parses a CPU/Codec node independently and
sets daifmt as per 'bitcloclk/frame-master' property.

Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 sound/soc/generic/simple-card.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kuninori Morimoto June 29, 2020, 12:56 a.m. UTC | #1
Hi Sameer

>  simple-audio-card,dai-link@xxx {
>      format = "i2s";
>      bitclock-master=<&cpu1>;
>      frame-master=<&cpu1>;
> 
>      cpu1: cpu@0 {
>          ...
>      };
> 
>      codec@0 {
>          ...
>      };
> 
>      ...
>  };
> 
> In above case CPU is expected to be configured as a master and Codec as
> a slave device. But both CPU/Codec are being configured as slave devices.
> This happens because asoc_simple_parse_daifmt() uses Codec reference and
> sets up the 'dai_link->dai_fmt' accordingly while parsing both CPU and
> Codec.

I'm sorry but I don't 100% understand about this case...
asoc_simple_parse_daifmt() should work in this case

The reason why it needs codec node is that
SND_SOC_DAIFMT_CBx_CFx are "Codec" base Master/Slave.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar June 29, 2020, 4:06 p.m. UTC | #2
On 6/29/2020 6:26 AM, Kuninori Morimoto wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sameer
>
>>   simple-audio-card,dai-link@xxx {
>>       format = "i2s";
>>       bitclock-master=<&cpu1>;
>>       frame-master=<&cpu1>;
>>
>>       cpu1: cpu@0 {
>>           ...
>>       };
>>
>>       codec@0 {
>>           ...
>>       };
>>
>>       ...
>>   };
>>
>> In above case CPU is expected to be configured as a master and Codec as
>> a slave device. But both CPU/Codec are being configured as slave devices.
>> This happens because asoc_simple_parse_daifmt() uses Codec reference and
>> sets up the 'dai_link->dai_fmt' accordingly while parsing both CPU and
>> Codec.
> I'm sorry but I don't 100% understand about this case...
> asoc_simple_parse_daifmt() should work in this case
>
> The reason why it needs codec node is that
> SND_SOC_DAIFMT_CBx_CFx are "Codec" base Master/Slave.

Currently soc-core has following code snippet,

/snd_soc_runtime_set_dai_fmt() {//
//     ...//
//
//     if (cpu_dai->component->driver->non_legacy_dai_naming)//
//         fmt = inv_dai_fmt;//
//
//     ...//
// }/

Above flips polarity for 'cpu_dai' if 'non_legacy_dai_naming' flag is set.

1. Hence example mentioned in the commit message does not work if my 
'cpu_dai' driver does not have this flag set.

2. While it is true that we consider reference of 'Codec' mode for 
simple CPU<->Codec DAI links, for DPCM this does not seem flexible. For 
DPCM links CPU and Codec are not directly connected (CPU<->Dummy or 
Dummy<->Codec). Please consider, for example, if the DAI link has 
multiple CPU/Codecs. Which 'Codec' reference needs to be considered? 
Isn't it better if we explicitly mention which DAI we want to operate as 
'Master'?

>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
Kuninori Morimoto June 30, 2020, 12:56 a.m. UTC | #3
Hi Sameer

>  snd_soc_runtime_set_dai_fmt() {
>      ...
> 
>      if (cpu_dai->component->driver->non_legacy_dai_naming)
>          fmt = inv_dai_fmt;
> 
>      ...
>  }
> 
> Above flips polarity for 'cpu_dai' if 'non_legacy_dai_naming' flag is set.
> 
> 1. Hence example mentioned in the commit message does not work if my 'cpu_dai'
> driver does not have this flag set.

?
Do you want fo flip it ? or don't flip?
It is for Codec <-> Codec connection.

> 2. While it is true that we consider reference of 'Codec' mode for simple CPU<->
> Codec DAI links, for DPCM this does not seem flexible. For DPCM links CPU and
> Codec are not directly connected (CPU<->Dummy or Dummy<->Codec). Please
> consider, for example, if the DAI link has multiple CPU/Codecs. Which 'Codec'
> reference needs to be considered? Isn't it better if we explicitly mention which
> DAI we want to operate as 'Master'?

I think Lars-Peter has (had ?) plan for this SND_SOC_DAIFMT_CBx_CFx
flag flexibility ? Yes maybe it is needed for multi CPU/Codec system.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar June 30, 2020, 3:47 a.m. UTC | #4
On 6/30/2020 6:26 AM, Kuninori Morimoto wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sameer
>
>>   snd_soc_runtime_set_dai_fmt() {
>>       ...
>>
>>       if (cpu_dai->component->driver->non_legacy_dai_naming)
>>           fmt = inv_dai_fmt;
>>
>>       ...
>>   }
>>
>> Above flips polarity for 'cpu_dai' if 'non_legacy_dai_naming' flag is set.
>>
>> 1. Hence example mentioned in the commit message does not work if my 'cpu_dai'
>> driver does not have this flag set.
> ?
> Do you want fo flip it ? or don't flip?
> It is for Codec <-> Codec connection.

For DPCM links I don't want to flip based on one Codec reference. My 
goal was to make the binding work for multiple CPU/Codec link. Hence I 
thought it would be better to explicitly describe the 'Master' DAI. We 
can eventually get rid of 'codec' argument from simple_dai_link_of_dpcm().
>> 2. While it is true that we consider reference of 'Codec' mode for simple CPU<->
>> Codec DAI links, for DPCM this does not seem flexible. For DPCM links CPU and
>> Codec are not directly connected (CPU<->Dummy or Dummy<->Codec). Please
>> consider, for example, if the DAI link has multiple CPU/Codecs. Which 'Codec'
>> reference needs to be considered? Isn't it better if we explicitly mention which
>> DAI we want to operate as 'Master'?
> I think Lars-Peter has (had ?) plan for this SND_SOC_DAIFMT_CBx_CFx
> flag flexibility ? Yes maybe it is needed for multi CPU/Codec system.
>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
Kuninori Morimoto June 30, 2020, 5:07 a.m. UTC | #5
Hi Sameer

> For DPCM links I don't want to flip based on one Codec reference. My
> goal was to make the binding work for multiple CPU/Codec link. Hence I
> thought it would be better to explicitly describe the 'Master' DAI. We
> can eventually get rid of 'codec' argument from
> simple_dai_link_of_dpcm().

Yes. 'codec' argument on current simple_dai_link_of_dpcm()
is not good match for multi Codec support.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 0f443c0..39cdc71 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -228,7 +228,7 @@  static int simple_dai_link_of_dpcm(struct asoc_simple_priv *priv,
 	if (ret)
 		goto out_put_node;
 
-	ret = asoc_simple_parse_daifmt(dev, node, codec,
+	ret = asoc_simple_parse_daifmt(dev, node, np,
 				       prefix, &dai_link->dai_fmt);
 	if (ret < 0)
 		goto out_put_node;