Re: Re: [PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
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.
Re: [PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
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. signature.asc Description: PGP signature
Re: [PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
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
Re: [PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
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
Re: [PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
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 --- (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
Re: [PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
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 > --- (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
[PATCH v4 11/23] ASoC: simple-card: Loop over all children for 'mclk-fs'
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 --- sound/soc/generic/simple-card.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) 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); -- 2.7.4
