| Message ID | 87d0f6ilqq.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> > > 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 is difficult to debug. > > Now ALSA SoC has snd_soc_add_component(), but there is no paired > snd_soc_del_component(). Thus, snd_soc_unregister_component() is > calling cleanup function randomly. it is difficult to read. > This patch adds missing snd_soc_del_component() and balance up code. the problem now is that the naming is confusing we have snd_soc_component_del and snd_soc_del_component. we already had snd_soc_component_add and snd_soc_add_component. Also I find it useful to keep the _unlocked suffix when relevant, it adds value that is lost otherwise. Can we avoid this pretty please? > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-core.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 72eb59c..7c0bb32 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -2767,12 +2767,7 @@ static void snd_soc_component_add(struct snd_soc_component *component) > mutex_unlock(&client_mutex); > } > > -static void snd_soc_component_cleanup(struct snd_soc_component *component) > -{ > - snd_soc_unregister_dais(component); > -} > - > -static void snd_soc_component_del_unlocked(struct snd_soc_component *component) > +static void snd_soc_component_del(struct snd_soc_component *component) > { > struct snd_soc_card *card = component->card; > > @@ -2826,6 +2821,12 @@ static void snd_soc_try_rebind_card(void) > list_del(&card->list); > } > > +static void snd_soc_del_component(struct snd_soc_component *component) > +{ > + snd_soc_unregister_dais(component); > + snd_soc_component_del(component); > +} > + > int snd_soc_add_component(struct device *dev, > struct snd_soc_component *component, > const struct snd_soc_component_driver *component_driver, > @@ -2858,7 +2859,7 @@ int snd_soc_add_component(struct device *dev, > return 0; > > err_cleanup: > - snd_soc_component_cleanup(component); > + snd_soc_del_component(component); > err_free: > return ret; > } > @@ -2896,15 +2897,12 @@ static int __snd_soc_unregister_component(struct device *dev) > if (dev != component->dev) > continue; > > - snd_soc_component_del_unlocked(component); > + snd_soc_del_component(component); > found = 1; > break; > } > mutex_unlock(&client_mutex); > > - if (found) > - snd_soc_component_cleanup(component); > - > return found; > } > >
Hi Pierre-Louis > > 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 is difficult to debug. > > > > Now ALSA SoC has snd_soc_add_component(), but there is no paired > > snd_soc_del_component(). Thus, snd_soc_unregister_component() is > > calling cleanup function randomly. it is difficult to read. > > This patch adds missing snd_soc_del_component() and balance up code. > > > the problem now is that the naming is confusing > > we have snd_soc_component_del and snd_soc_del_component. > we already had snd_soc_component_add and snd_soc_add_component. Yes, very confusing. > Also I find it useful to keep the _unlocked suffix when relevant, it > adds value that is lost otherwise. > > Can we avoid this pretty please? Yeah, thanks. The only issue for it is that my English naming sense/skill ;P Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 72eb59c..7c0bb32 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2767,12 +2767,7 @@ static void snd_soc_component_add(struct snd_soc_component *component) mutex_unlock(&client_mutex); } -static void snd_soc_component_cleanup(struct snd_soc_component *component) -{ - snd_soc_unregister_dais(component); -} - -static void snd_soc_component_del_unlocked(struct snd_soc_component *component) +static void snd_soc_component_del(struct snd_soc_component *component) { struct snd_soc_card *card = component->card; @@ -2826,6 +2821,12 @@ static void snd_soc_try_rebind_card(void) list_del(&card->list); } +static void snd_soc_del_component(struct snd_soc_component *component) +{ + snd_soc_unregister_dais(component); + snd_soc_component_del(component); +} + int snd_soc_add_component(struct device *dev, struct snd_soc_component *component, const struct snd_soc_component_driver *component_driver, @@ -2858,7 +2859,7 @@ int snd_soc_add_component(struct device *dev, return 0; err_cleanup: - snd_soc_component_cleanup(component); + snd_soc_del_component(component); err_free: return ret; } @@ -2896,15 +2897,12 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue; - snd_soc_component_del_unlocked(component); + snd_soc_del_component(component); found = 1; break; } mutex_unlock(&client_mutex); - if (found) - snd_soc_component_cleanup(component); - return found; }