| Message ID | 87mueailrn.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: soc-core cleanup - step 4 | expand |
On 10/8/19 11:30 PM, Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > soc_init_dai_link() is checking dai_link only, not initializing today. > Therefore, we can rename it as sanity_check. > > And this check is for soc_bind_dai_link(). > Thus, we can check it under soc_bind_dai_link() to more clear code. > This patch rename it, and call it from soc_bind_dai_link(). > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-core.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 335dc8f..f440022 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -941,8 +941,8 @@ static bool soc_is_dai_link_bound(struct snd_soc_card *card, > return false; > } > > -static int soc_init_dai_link(struct snd_soc_card *card, > - struct snd_soc_dai_link *link) > +static int soc_dai_link_sanity_check(struct snd_soc_card *card, > + struct snd_soc_dai_link *link) > { > int i; > struct snd_soc_dai_link_component *codec, *platform; > @@ -1043,11 +1043,15 @@ static int soc_bind_dai_link(struct snd_soc_card *card, > struct snd_soc_pcm_runtime *rtd; > struct snd_soc_dai_link_component *codec, *platform; > struct snd_soc_component *component; > - int i; > + int i, ret; > > if (dai_link->ignore) > return 0; > > + ret = soc_dai_link_sanity_check(card, dai_link); > + if (ret < 0) > + return ret; > + > dev_dbg(card->dev, "ASoC: binding %s\n", dai_link->name); > > if (soc_is_dai_link_bound(card, dai_link)) { > @@ -1985,15 +1989,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) > int ret, i; > > mutex_lock(&client_mutex); > - for_each_card_prelinks(card, i, dai_link) { > - ret = soc_init_dai_link(card, dai_link); > - if (ret) { > - dev_err(card->dev, "ASoC: failed to init link %s: %d\n", > - dai_link->name, ret); > - mutex_unlock(&client_mutex); > - return ret; > - } > - } This part is difficult to understand. There were two calls to soc_init_dai_link(), here and [2] below. Your patch removes the first call and the for loop, is this intentional and needed? > mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_INIT); > > snd_soc_dapm_init(&card->dapm, card, NULL); > @@ -2073,9 +2068,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) > if (soc_is_dai_link_bound(card, dai_link)) > continue; > > - ret = soc_init_dai_link(card, dai_link); > - if (ret) > - goto probe_end; [2] > ret = soc_bind_dai_link(card, dai_link); > if (ret) > goto probe_end; >
Hi Pierre-Louis > > - for_each_card_prelinks(card, i, dai_link) { > > - ret = soc_init_dai_link(card, dai_link); > > - if (ret) { > > - dev_err(card->dev, "ASoC: failed to init link %s: %d\n", > > - dai_link->name, ret); > > - mutex_unlock(&client_mutex); > > - return ret; > > - } > > - } > > This part is difficult to understand. > > There were two calls to soc_init_dai_link(), here and [2] below. > Your patch removes the first call and the for loop, is this > intentional and needed? soc_init_dai_link() is just sanity_check now. In my understanding, it is needed before soc_bind_dai_link(). Current code is like below. (1) and (2) are for care prelink:ed dai_link. (A) and (B) are for topology added new dai_link. and (1) is for (2) (A) is for (B) int snd_soc_instantiate_card() { for_each_card_prelinks(...) { (1) ret = soc_init_dai_link(...); ... } ... for_each_card_prelinks(...) { (2) ret = soc_bind_dai_link(...); ... } ... for_each_card_links(...) { ... (A) ret = soc_init_dai_link(...); ... (B) ret = soc_bind_dai_link(...); } } It is very confusing/verbose code for me. It can be more simple if we can call soc_init_dai_link() from soc_bind_dai_link(). Thank you for your help !! Best regards --- Kuninori Morimoto
On 10/10/19 8:19 PM, Kuninori Morimoto wrote: > > Hi Pierre-Louis > >>> - for_each_card_prelinks(card, i, dai_link) { >>> - ret = soc_init_dai_link(card, dai_link); >>> - if (ret) { >>> - dev_err(card->dev, "ASoC: failed to init link %s: %d\n", >>> - dai_link->name, ret); >>> - mutex_unlock(&client_mutex); >>> - return ret; >>> - } >>> - } >> >> This part is difficult to understand. >> >> There were two calls to soc_init_dai_link(), here and [2] below. >> Your patch removes the first call and the for loop, is this >> intentional and needed? > > soc_init_dai_link() is just sanity_check now. > In my understanding, it is needed before soc_bind_dai_link(). > > Current code is like below. > (1) and (2) are for care prelink:ed dai_link. > (A) and (B) are for topology added new dai_link. > and > (1) is for (2) > (A) is for (B) > > int snd_soc_instantiate_card() > { > for_each_card_prelinks(...) { > (1) ret = soc_init_dai_link(...); > ... > } > ... > for_each_card_prelinks(...) { > (2) ret = soc_bind_dai_link(...); > ... > } > ... > for_each_card_links(...) { > ... > (A) ret = soc_init_dai_link(...); > ... > (B) ret = soc_bind_dai_link(...); > } > } > > It is very confusing/verbose code for me. > It can be more simple if we can call soc_init_dai_link() > from soc_bind_dai_link(). ok, the explanations help, maye you can add them to the commit message to help explain the intent, e.g. Current code is like below. (1) and (2) are for care prelink:ed dai_link. (A) and (B) are for topology added new dai_link. and (1) is for (2) (A) is for (B) for_each_card_prelinks(...) { (2) int snd_soc_instantiate_card() { for_each_card_prelinks(...) { (1) ret = soc_init_dai_link(...); ... } ... ret = soc_bind_dai_link(...); ... } ... for_each_card_links(...) { ... (A) ret = soc_init_dai_link(...); ... (B) ret = soc_bind_dai_link(...); } and the new code keeps the same flow/steps but collapses the two calls into one for_each_card_prelinks(...) { (2) int snd_soc_instantiate_card() { for_each_card_prelinks(...) { (1) ret = soc_bind_dai_link(...); ... } ... for_each_card_links(...) { (A) (B) ret = soc_bind_dai_link(...); }
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 335dc8f..f440022 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -941,8 +941,8 @@ static bool soc_is_dai_link_bound(struct snd_soc_card *card, return false; } -static int soc_init_dai_link(struct snd_soc_card *card, - struct snd_soc_dai_link *link) +static int soc_dai_link_sanity_check(struct snd_soc_card *card, + struct snd_soc_dai_link *link) { int i; struct snd_soc_dai_link_component *codec, *platform; @@ -1043,11 +1043,15 @@ static int soc_bind_dai_link(struct snd_soc_card *card, struct snd_soc_pcm_runtime *rtd; struct snd_soc_dai_link_component *codec, *platform; struct snd_soc_component *component; - int i; + int i, ret; if (dai_link->ignore) return 0; + ret = soc_dai_link_sanity_check(card, dai_link); + if (ret < 0) + return ret; + dev_dbg(card->dev, "ASoC: binding %s\n", dai_link->name); if (soc_is_dai_link_bound(card, dai_link)) { @@ -1985,15 +1989,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) int ret, i; mutex_lock(&client_mutex); - for_each_card_prelinks(card, i, dai_link) { - ret = soc_init_dai_link(card, dai_link); - if (ret) { - dev_err(card->dev, "ASoC: failed to init link %s: %d\n", - dai_link->name, ret); - mutex_unlock(&client_mutex); - return ret; - } - } mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_INIT); snd_soc_dapm_init(&card->dapm, card, NULL); @@ -2073,9 +2068,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) if (soc_is_dai_link_bound(card, dai_link)) continue; - ret = soc_init_dai_link(card, dai_link); - if (ret) - goto probe_end; ret = soc_bind_dai_link(card, dai_link); if (ret) goto probe_end;