diff mbox series

[v4,11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'

Message ID 1593233625-14961-12-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
CPU/Codec in DPCM DAI links are connected as CPU<->Dummy and Dummy<->Codec.
Though mostly CPU won't use/require 'mclk-fs' property, looping over
'np' (current child node in a DAI link) can help in cases where multiple
Codecs are defined. This further helps to get rid of 'codec' argument
from simple_dai_link_of_dpcm() function, which gets called for DPCM links.

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

Comments

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

> CPU/Codec in DPCM DAI links are connected as CPU<->Dummy and Dummy<->Codec.
> Though mostly CPU won't use/require 'mclk-fs' property, looping over
> 'np' (current child node in a DAI link) can help in cases where multiple
> Codecs are defined. This further helps to get rid of 'codec' argument
> from simple_dai_link_of_dpcm() function, which gets called for DPCM links.
> 
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> ---
(snip)
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 39cdc71..02d6295 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -107,7 +107,9 @@ static void simple_parse_mclk_fs(struct device_node *top,
>  	snprintf(prop, sizeof(prop), "%smclk-fs", prefix);
>  	of_property_read_u32(node,	prop, &props->mclk_fs);
>  	of_property_read_u32(cpu,	prop, &props->mclk_fs);
> -	of_property_read_u32(codec,	prop, &props->mclk_fs);
> +
> +	if (cpu != codec)
> +		of_property_read_u32(codec, prop, &props->mclk_fs);

Maybe we want to have "cpu" in simple_dai_link_of_dpcm() side
without using magical code in simple_parse_mclk_fs() side ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar June 29, 2020, 4:32 p.m. UTC | #2
On 6/29/2020 6:35 AM, Kuninori Morimoto wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sameer
>
>> CPU/Codec in DPCM DAI links are connected as CPU<->Dummy and Dummy<->Codec.
>> Though mostly CPU won't use/require 'mclk-fs' property, looping over
>> 'np' (current child node in a DAI link) can help in cases where multiple
>> Codecs are defined. This further helps to get rid of 'codec' argument
>> from simple_dai_link_of_dpcm() function, which gets called for DPCM links.
>>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> ---
> (snip)
>> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
>> index 39cdc71..02d6295 100644
>> --- a/sound/soc/generic/simple-card.c
>> +++ b/sound/soc/generic/simple-card.c
>> @@ -107,7 +107,9 @@ static void simple_parse_mclk_fs(struct device_node *top,
>>        snprintf(prop, sizeof(prop), "%smclk-fs", prefix);
>>        of_property_read_u32(node,      prop, &props->mclk_fs);
>>        of_property_read_u32(cpu,       prop, &props->mclk_fs);
>> -     of_property_read_u32(codec,     prop, &props->mclk_fs);
>> +
>> +     if (cpu != codec)
>> +             of_property_read_u32(codec, prop, &props->mclk_fs);
> Maybe we want to have "cpu" in simple_dai_link_of_dpcm() side
> without using magical code in simple_parse_mclk_fs() side ?

Are you suggesting if we should simplify simple_parse_mclk_fs() by 
either passing 'cpu' or 'codec'?
>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
Kuninori Morimoto June 30, 2020, 2:08 a.m. UTC | #3
Hi Sameer

> >>        snprintf(prop, sizeof(prop), "%smclk-fs", prefix);
> >>        of_property_read_u32(node,      prop, &props->mclk_fs);
> >>        of_property_read_u32(cpu,       prop, &props->mclk_fs);
> >> -     of_property_read_u32(codec,     prop, &props->mclk_fs);
> >> +
> >> +     if (cpu != codec)
> >> +             of_property_read_u32(codec, prop, &props->mclk_fs);
> > Maybe we want to have "cpu" in simple_dai_link_of_dpcm() side
> > without using magical code in simple_parse_mclk_fs() side ?
> 
> Are you suggesting if we should simplify simple_parse_mclk_fs() by
> either passing 'cpu' or 'codec'?

Oops, sorry I was misunderstand.

But I still not 100% understand what do you want to do here.
Maybe 50% is my English skill, but in your code
 
(C)	 	of_property_read_u32(cpu,	prop, &props->mclk_fs);
	-	of_property_read_u32(codec,	prop, &props->mclk_fs);
	+
	+	if (cpu != codec)
(B)	+		of_property_read_u32(codec, prop, &props->mclk_fs);

and

	-	simple_parse_mclk_fs(top, np, codec, dai_props, prefix);
(A)	+	simple_parse_mclk_fs(top, np, np, dai_props, prefix);

Because of (A), cpu = codec = np in simple_parse_mclk_fs().
Do we have chance to call (B) ?
And it still have read_u32(cpu, ...) at (C),
this means all np will read mclk_fs anyway ?
For me, if you don't want/need mclk_fs, don't set it on DT
is the best answer, but am I misunderstanding ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Sameer Pujar June 30, 2020, 4:23 a.m. UTC | #4
On 6/30/2020 7:38 AM, Kuninori Morimoto wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sameer
>
>>>>         snprintf(prop, sizeof(prop), "%smclk-fs", prefix);
>>>>         of_property_read_u32(node,      prop, &props->mclk_fs);
>>>>         of_property_read_u32(cpu,       prop, &props->mclk_fs);
>>>> -     of_property_read_u32(codec,     prop, &props->mclk_fs);
>>>> +
>>>> +     if (cpu != codec)
>>>> +             of_property_read_u32(codec, prop, &props->mclk_fs);
>>> Maybe we want to have "cpu" in simple_dai_link_of_dpcm() side
>>> without using magical code in simple_parse_mclk_fs() side ?
>> Are you suggesting if we should simplify simple_parse_mclk_fs() by
>> either passing 'cpu' or 'codec'?
> Oops, sorry I was misunderstand.
>
> But I still not 100% understand what do you want to do here.
> Maybe 50% is my English skill, but in your code
>
> (C)             of_property_read_u32(cpu,       prop, &props->mclk_fs);
>          -       of_property_read_u32(codec,     prop, &props->mclk_fs);
>          +
>          +       if (cpu != codec)
> (B)     +               of_property_read_u32(codec, prop, &props->mclk_fs);
>
> and
>
>          -       simple_parse_mclk_fs(top, np, codec, dai_props, prefix);
> (A)     +       simple_parse_mclk_fs(top, np, np, dai_props, prefix);
>
> Because of (A), cpu = codec = np in simple_parse_mclk_fs().
> Do we have chance to call (B) ?
> And it still have read_u32(cpu, ...) at (C),
> this means all np will read mclk_fs anyway ?
> For me, if you don't want/need mclk_fs, don't set it on DT
> is the best answer, but am I misunderstanding ?
Sorry if I was not clear before.

My goal was to get rid of 'codec' argument from DPCM function 
simple_dai_link_of_dpcm(). Patches 10/23, 11/23 are preparations for 
12/23 to have multiple Codec support.

simple_parse_mclk_fs() is used by both simple_dai_link_of_dpcm() and 
simple_dai_link_of(). To make the same API work for both the cases, I 
had to use (A) in DPCM function. Now (B) does not get used for 
simple_dai_link_of_dpcm(), but will get used by simple_dai_link_of().

If it is simpler I will just avoid 'cpu != codec' check and keep the 
function simple_parse_mclk_fs() as it is.

>
> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto
Mark Brown June 30, 2020, 10:55 a.m. UTC | #5
On Tue, Jun 30, 2020 at 09:53:13AM +0530, Sameer Pujar wrote:
> On 6/30/2020 7:38 AM, Kuninori Morimoto wrote:
> > External email: Use caution opening links or attachments

> > > > > +     if (cpu != codec)
> > > > > +             of_property_read_u32(codec, prop, &props->mclk_fs);

> Sorry if I was not clear before.

I agree with Moromito-san that the new code (especially the above bit)
is very confusing, I'm not sure how the reader is supposed to figure out
what the purpose of the check is or how the CPU could ever be the CODEC.

> simple_parse_mclk_fs() is used by both simple_dai_link_of_dpcm() and
> simple_dai_link_of(). To make the same API work for both the cases, I had to
> use (A) in DPCM function. Now (B) does not get used for
> simple_dai_link_of_dpcm(), but will get used by simple_dai_link_of().

> If it is simpler I will just avoid 'cpu != codec' check and keep the
> function simple_parse_mclk_fs() as it is.

That'd definitely be simpler, or supporting this with a CPU node as
well.
Sameer Pujar June 30, 2020, 11:56 a.m. UTC | #6
On 6/30/2020 4:25 PM, Mark Brown wrote:
> On Tue, Jun 30, 2020 at 09:53:13AM +0530, Sameer Pujar wrote:
>> On 6/30/2020 7:38 AM, Kuninori Morimoto wrote:
>>> External email: Use caution opening links or attachments
>>>>>> +     if (cpu != codec)
>>>>>> +             of_property_read_u32(codec, prop, &props->mclk_fs);
>> Sorry if I was not clear before.
> I agree with Moromito-san that the new code (especially the above bit)
> is very confusing, I'm not sure how the reader is supposed to figure out
> what the purpose of the check is or how the CPU could ever be the CODEC.
>
>> simple_parse_mclk_fs() is used by both simple_dai_link_of_dpcm() and
>> simple_dai_link_of(). To make the same API work for both the cases, I had to
>> use (A) in DPCM function. Now (B) does not get used for
>> simple_dai_link_of_dpcm(), but will get used by simple_dai_link_of().
>> If it is simpler I will just avoid 'cpu != codec' check and keep the
>> function simple_parse_mclk_fs() as it is.
> That'd definitely be simpler, or supporting this with a CPU node as
> well.

OK. I will keep it simple.
diff mbox series

Patch

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 39cdc71..02d6295 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -107,7 +107,9 @@  static void simple_parse_mclk_fs(struct device_node *top,
 	snprintf(prop, sizeof(prop), "%smclk-fs", prefix);
 	of_property_read_u32(node,	prop, &props->mclk_fs);
 	of_property_read_u32(cpu,	prop, &props->mclk_fs);
-	of_property_read_u32(codec,	prop, &props->mclk_fs);
+
+	if (cpu != codec)
+		of_property_read_u32(codec, prop, &props->mclk_fs);
 
 	of_node_put(node);
 }
@@ -220,7 +222,7 @@  static int simple_dai_link_of_dpcm(struct asoc_simple_priv *priv,
 	}
 
 	simple_parse_convert(dev, np, &dai_props->adata);
-	simple_parse_mclk_fs(top, np, codec, dai_props, prefix);
+	simple_parse_mclk_fs(top, np, np, dai_props, prefix);
 
 	asoc_simple_canonicalize_platform(dai_link);