| Message ID | 87k18dhkw2.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: soc-core: cleanup step5 | expand |
On Tue, Nov 5, 2019 at 5:14 PM Kuninori Morimoto < kuninori.morimoto.gx@renesas.com> wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > It is easy to read code if it is cleanly using paired function/naming, > like start <-> stop, register <-> unregister, etc, etc. > But, current ALSA SoC code is very random, unbalance, not paired, etc. > It is easy to create bug at the such code, and it will be difficult to > debug. > > snd_soc_bind_card() is calling snd_soc_dapm_init() for both > card and component. > Let's call paired snd_soc_dapm_shutdown() at paired > soc_cleanup_card_resources(). > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 927b9c9..0bed63e 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -1940,6 +1940,8 @@ static void soc_cleanup_card_resources(struct > snd_soc_card *card) > card->snd_card = NULL; > } > > + snd_soc_dapm_shutdown(card); > + > /* remove and free each DAI */ > soc_remove_link_dais(card); > soc_remove_link_components(card); > @@ -2377,7 +2379,6 @@ static void snd_soc_unbind_card(struct snd_soc_card > *card, bool unregister) > Morimoto-san, You removed snd_soc_bind_card in one of the patches but then leaving snd_soc_unbind_card() will be unbalanced isnt it? Why not just have instantiate_card() and cleanup_card_resources()? Thanks, Ranjani > { > if (card->instantiated) { > card->instantiated = false; > - snd_soc_dapm_shutdown(card); > snd_soc_flush_all_delayed_work(card); > > soc_cleanup_card_resources(card); > -- > 2.7.4 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
Hi Ranjani Thank you for your review > It is easy to read code if it is cleanly using paired function/naming, > like start <-> stop, register <-> unregister, etc, etc. > But, current ALSA SoC code is very random, unbalance, not paired, etc. > It is easy to create bug at the such code, and it will be difficult to > debug. > > snd_soc_bind_card() is calling snd_soc_dapm_init() for both > card and component. > Let's call paired snd_soc_dapm_shutdown() at paired > soc_cleanup_card_resources(). > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- (snip) > You removed snd_soc_bind_card in one of the patches but then leaving snd_soc_unbind_card() will be unbalanced isnt it? > > Why not just have instantiate_card() and cleanup_card_resources()? Do you mean [7/9] patch ? It merges snd_soc_instantiate_card() and snd_soc_bind_card(). Thus, snd_soc_bind_card() is still exist. Or am I misunderstanding ? Thank you for your help !! Best regards --- Kuninori Morimoto
On Tue, Nov 5, 2019 at 6:40 PM Kuninori Morimoto < kuninori.morimoto.gx@renesas.com> wrote: > > Hi Ranjani > > Thank you for your review > > > It is easy to read code if it is cleanly using paired > function/naming, > > like start <-> stop, register <-> unregister, etc, etc. > > But, current ALSA SoC code is very random, unbalance, not paired, > etc. > > It is easy to create bug at the such code, and it will be difficult > to > > debug. > > > > snd_soc_bind_card() is calling snd_soc_dapm_init() for both > > card and component. > > Let's call paired snd_soc_dapm_shutdown() at paired > > soc_cleanup_card_resources(). > > > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > --- > (snip) > > You removed snd_soc_bind_card in one of the patches but then leaving > snd_soc_unbind_card() will be unbalanced isnt it? > > > > Why not just have instantiate_card() and cleanup_card_resources()? > > Do you mean [7/9] patch ? > It merges snd_soc_instantiate_card() and snd_soc_bind_card(). > Thus, snd_soc_bind_card() is still exist. > Or am I misunderstanding ? > Oh yes, sorry I misread that. So why not remove cleanup_card_resources and move everything to snd_soc_unbind_card()? Thanks, Ranjani > > Thank you for your help !! > Best regards > --- > Kuninori Morimoto >
Hi Ranjani Thank you for your feedback > Do you mean [7/9] patch ? > It merges snd_soc_instantiate_card() and snd_soc_bind_card(). > Thus, snd_soc_bind_card() is still exist. > Or am I misunderstanding ? > > Oh yes, sorry I misread that. So why not remove cleanup_card_resources and move everything to snd_soc_unbind_card()? Good question :) Indeed snd_soc_bind_card() and snd_soc_unbind_card() are paired function. We want to merge cleanup_card_resources() and snd_soc_unbind_card(). But, can you check snd_soc_unbind_card() ? unbind() is caring - card->instantiated - snd_soc_flush_all_delayed_work(card); - unbind_card_list Actually I tried to merge cleanup() and unbind() into one, but then, the code became not simple. So I gave up this time. But, we might can do it in the future if soc-core is more cleanuped/simpled. Thank you for your help !! Best regards --- Kuninori Morimoto
On Tue, Nov 5, 2019 at 6:56 PM Kuninori Morimoto < kuninori.morimoto.gx@renesas.com> wrote: > > Hi Ranjani > > Thank you for your feedback > > > Do you mean [7/9] patch ? > > It merges snd_soc_instantiate_card() and snd_soc_bind_card(). > > Thus, snd_soc_bind_card() is still exist. > > Or am I misunderstanding ? > > > > Oh yes, sorry I misread that. So why not remove cleanup_card_resources > and move everything to snd_soc_unbind_card()? > > Good question :) > > Indeed snd_soc_bind_card() and snd_soc_unbind_card() are paired function. > We want to merge cleanup_card_resources() and snd_soc_unbind_card(). > But, can you check snd_soc_unbind_card() ? > unbind() is caring > - card->instantiated > - snd_soc_flush_all_delayed_work(card); > - unbind_card_list > > Actually I tried to merge cleanup() and unbind() into one, > but then, the code became not simple. > So I gave up this time. > But, we might can do it in the future if soc-core is more > cleanuped/simpled. > OK, makes sense. In that case, this series looks good to go. Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> > > Thank you for your help !! > Best regards > --- > Kuninori Morimoto >
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 927b9c9..0bed63e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1940,6 +1940,8 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card) card->snd_card = NULL; } + snd_soc_dapm_shutdown(card); + /* remove and free each DAI */ soc_remove_link_dais(card); soc_remove_link_components(card); @@ -2377,7 +2379,6 @@ static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister) { if (card->instantiated) { card->instantiated = false; - snd_soc_dapm_shutdown(card); snd_soc_flush_all_delayed_work(card); soc_cleanup_card_resources(card);