[9/9] ASoC: soc-core: call snd_soc_dapm_shutdown() at soc_cleanup_card_resources()
diff mbox series

Message ID 87k18dhkw2.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series
  • ASoC: soc-core: cleanup step5
Related show

Commit Message

Kuninori Morimoto Nov. 6, 2019, 1:08 a.m. UTC
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(-)

Comments

Sridharan, Ranjani Nov. 6, 2019, 2:34 a.m. UTC | #1
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
>
Kuninori Morimoto Nov. 6, 2019, 2:40 a.m. UTC | #2
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
Sridharan, Ranjani Nov. 6, 2019, 2:45 a.m. UTC | #3
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
>
Kuninori Morimoto Nov. 6, 2019, 2:56 a.m. UTC | #4
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
Sridharan, Ranjani Nov. 6, 2019, 3:09 a.m. UTC | #5
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
>

Patch
diff mbox series

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);