[alsa-devel] [PATCH 00/21] ASoC: soc-core cleanup - step 4
Hi Mark Cc Pierre-Louis
These are step4 of soc-core cleanup. These related to soc-topology. If my understanding and my code are correct, there is no effect to topology side, but I can't test.
I'm not sure who can test these, but I guess Pierre-Louis knows ? Pierre-Louis, can you help it ?
Kuninori Morimoto (21): ASoC: soc-core: remove for_each_rtdcom_safe() ASoC: soc-core: add for_each_rtd_components() and replace ASoC: soc-core: move soc_init_dai_link() ASoC: soc-core: rename soc_init_dai_link() to soc_dai_link_sanity_check() ASoC: soc-core: remove duplicated soc_is_dai_link_bound() ASoC: soc-core: call soc_bind_dai_link() under snd_soc_add_dai_link() ASoC: soc-core: add soc_unbind_dai_link() ASoC: soc-core: snd_soc_unbind_card() cleanup ASoC: soc-core: remove unneeded snd_soc_tplg_component_remove() ASoC: soc-core: move snd_soc_lookup_component() ASoC: soc-core: add snd_soc_del_component() ASoC: soc-core: use snd_soc_lookup_component() at snd_soc_unregister_component() ASoC: soc-core: move snd_soc_register_dai() ASoC: soc-core: have legacy_dai_naming at snd_soc_register_dai() ASoC: soc-core: move snd_soc_unregister_dais() ASoC: soc-core: add snd_soc_unregister_dai() ASoC: soc-core: don't call snd_soc_dapm_new_dai_widgets() at snd_soc_register_dai() ASoC: soc-core: use mutex_lock() at snd_soc_add_component() ASoC: soc-core: call snd_soc_register_dai() from snd_soc_register_dais() ASoC: soc-core: remove topology specific operation ASoC: soc.h: dobj is used only when SND_SOC_TOPOLOGY
include/sound/soc.h | 25 ++- sound/soc/soc-component.c | 43 +--- sound/soc/soc-compress.c | 52 ++--- sound/soc/soc-core.c | 542 ++++++++++++++++++++-------------------------- sound/soc/soc-pcm.c | 49 ++--- sound/soc/soc-topology.c | 32 ++- 6 files changed, 320 insertions(+), 423 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
There is no user of for_each_rtdcom(). Let's remove it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 2 -- 1 file changed, 2 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 32748f7..d730883 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -741,8 +741,6 @@ snd_soc_rtdcom_lookup(struct snd_soc_pcm_runtime *rtd, const char *driver_name); #define for_each_rtdcom(rtd, rtdcom) \ list_for_each_entry(rtdcom, &(rtd)->component_list, list) -#define for_each_rtdcom_safe(rtd, rtdcom1, rtdcom2) \ - list_for_each_entry_safe(rtdcom1, rtdcom2, &(rtd)->component_list, list)
struct snd_soc_dai_link_component { const char *name;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
ALSA SoC has for_each_rtdcom() which is link list for rtd-component which is called as rtdcom. The relationship image is like below
rtdcom rtdcom rtdcom component component component rtd->component_list -> list -> list -> list ...
Here, the pointer get via normal link list is rtdcom, Thus, current for_each loop is like below, and need to get component via rtdcom->component
for_each_rtdcom(rtd, rtdcom) { component = rtdcom->component; ... }
but usually, user want to get pointer from for_each_xxx is component directly, like below.
for_each_rtd_component(rtd, rtdcom, component) { ... }
This patch expands list_for_each_entry manually, and enable to get component directly from for_each macro. Because of it, the macro becoming difficult to read, but macro itself becoming useful.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 8 ++++++-- sound/soc/soc-component.c | 43 +++++++++++---------------------------- sound/soc/soc-compress.c | 52 ++++++++++++----------------------------------- sound/soc/soc-core.c | 22 ++++++++------------ sound/soc/soc-pcm.c | 49 +++++++++++++------------------------------- 5 files changed, 54 insertions(+), 120 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index d730883..320dcd4 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -739,8 +739,12 @@ struct snd_soc_rtdcom_list { struct snd_soc_component* snd_soc_rtdcom_lookup(struct snd_soc_pcm_runtime *rtd, const char *driver_name); -#define for_each_rtdcom(rtd, rtdcom) \ - list_for_each_entry(rtdcom, &(rtd)->component_list, list) +#define for_each_rtd_components(rtd, rtdcom, _component) \ + for (rtdcom = list_first_entry(&(rtd)->component_list, \ + typeof(*rtdcom), list); \ + (&rtdcom->list != &(rtd)->component_list) && \ + (_component = rtdcom->component); \ + rtdcom = list_next_entry(rtdcom, list))
struct snd_soc_dai_link_component { const char *name; diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index debaf1f..98ef066 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -420,13 +420,10 @@ int snd_soc_pcm_component_pointer(struct snd_pcm_substream *substream) struct snd_soc_component *component; struct snd_soc_rtdcom_list *rtdcom;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - - /* FIXME: use 1st pointer */ + /* FIXME: use 1st pointer */ + for_each_rtd_components(rtd, rtdcom, component) if (component->driver->pointer) return component->driver->pointer(component, substream); - }
return 0; } @@ -438,14 +435,11 @@ int snd_soc_pcm_component_ioctl(struct snd_pcm_substream *substream, struct snd_soc_component *component; struct snd_soc_rtdcom_list *rtdcom;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - - /* FIXME: use 1st ioctl */ + /* FIXME: use 1st ioctl */ + for_each_rtd_components(rtd, rtdcom, component) if (component->driver->ioctl) return component->driver->ioctl(component, substream, cmd, arg); - }
return snd_pcm_lib_ioctl(substream, cmd, arg); } @@ -458,14 +452,11 @@ int snd_soc_pcm_component_copy_user(struct snd_pcm_substream *substream, struct snd_soc_rtdcom_list *rtdcom; struct snd_soc_component *component;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - - /* FIXME. it returns 1st copy now */ + /* FIXME. it returns 1st copy now */ + for_each_rtd_components(rtd, rtdcom, component) if (component->driver->copy_user) return component->driver->copy_user( component, substream, channel, pos, buf, bytes); - }
return -EINVAL; } @@ -478,10 +469,8 @@ struct page *snd_soc_pcm_component_page(struct snd_pcm_substream *substream, struct snd_soc_component *component; struct page *page;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - - /* FIXME. it returns 1st page now */ + /* FIXME. it returns 1st page now */ + for_each_rtd_components(rtd, rtdcom, component) { if (component->driver->page) { page = component->driver->page(component, substream, offset); @@ -500,14 +489,11 @@ int snd_soc_pcm_component_mmap(struct snd_pcm_substream *substream, struct snd_soc_rtdcom_list *rtdcom; struct snd_soc_component *component;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - - /* FIXME. it returns 1st mmap now */ + /* FIXME. it returns 1st mmap now */ + for_each_rtd_components(rtd, rtdcom, component) if (component->driver->mmap) return component->driver->mmap(component, substream, vma); - }
return -EINVAL; } @@ -519,9 +505,7 @@ int snd_soc_pcm_component_new(struct snd_pcm *pcm) struct snd_soc_component *component; int ret;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (component->driver->pcm_construct) { ret = component->driver->pcm_construct(component, rtd); if (ret < 0) @@ -538,10 +522,7 @@ void snd_soc_pcm_component_free(struct snd_pcm *pcm) struct snd_soc_rtdcom_list *rtdcom; struct snd_soc_component *component;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) if (component->driver->pcm_destruct) component->driver->pcm_destruct(component, pcm); - } } diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index 9e54d8a..61f2303 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -28,9 +28,7 @@ static int soc_compr_components_open(struct snd_compr_stream *cstream, struct snd_soc_rtdcom_list *rtdcom; int ret;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->open) continue; @@ -57,9 +55,7 @@ static int soc_compr_components_free(struct snd_compr_stream *cstream, struct snd_soc_component *component; struct snd_soc_rtdcom_list *rtdcom;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (component == last) break;
@@ -353,9 +349,7 @@ static int soc_compr_components_trigger(struct snd_compr_stream *cstream, struct snd_soc_rtdcom_list *rtdcom; int ret;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->trigger) continue; @@ -458,9 +452,7 @@ static int soc_compr_components_set_params(struct snd_compr_stream *cstream, struct snd_soc_rtdcom_list *rtdcom; int ret;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->set_params) continue; @@ -601,9 +593,7 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream, goto err; }
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->get_params) continue; @@ -627,9 +617,7 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->get_caps) continue; @@ -652,9 +640,7 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->get_codec_caps) continue; @@ -684,9 +670,7 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes) goto err; }
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->ack) continue; @@ -715,9 +699,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream, if (cpu_dai->driver->cops && cpu_dai->driver->cops->pointer) cpu_dai->driver->cops->pointer(cstream, tstamp, cpu_dai);
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->pointer) continue; @@ -740,9 +722,7 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->copy) continue; @@ -770,9 +750,7 @@ static int soc_compr_set_metadata(struct snd_compr_stream *cstream, return ret; }
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->set_metadata) continue; @@ -801,9 +779,7 @@ static int soc_compr_get_metadata(struct snd_compr_stream *cstream, return ret; }
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->get_metadata) continue; @@ -932,9 +908,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num) memcpy(compr->ops, &soc_compr_ops, sizeof(soc_compr_ops)); }
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->compr_ops || !component->driver->compr_ops->copy) continue; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index bd2ac19..0603702 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -293,10 +293,11 @@ static int snd_soc_rtdcom_add(struct snd_soc_pcm_runtime *rtd, struct snd_soc_component *component) { struct snd_soc_rtdcom_list *rtdcom; + struct snd_soc_component *comp;
- for_each_rtdcom(rtd, rtdcom) { + for_each_rtd_components(rtd, rtdcom, comp) { /* already connected */ - if (rtdcom->component == component) + if (comp == component) return 0; }
@@ -327,6 +328,7 @@ struct snd_soc_component *snd_soc_rtdcom_lookup(struct snd_soc_pcm_runtime *rtd, const char *driver_name) { struct snd_soc_rtdcom_list *rtdcom; + struct snd_soc_component *component;
if (!driver_name) return NULL; @@ -339,8 +341,8 @@ struct snd_soc_component *snd_soc_rtdcom_lookup(struct snd_soc_pcm_runtime *rtd, * But, if many components which have same driver name are connected * to 1 rtd, this function will return 1st found component. */ - for_each_rtdcom(rtd, rtdcom) { - const char *component_name = rtdcom->component->driver->name; + for_each_rtd_components(rtd, rtdcom, component) { + const char *component_name = component->driver->name;
if (!component_name) continue; @@ -1248,9 +1250,7 @@ static void soc_remove_link_components(struct snd_soc_card *card)
for_each_comp_order(order) { for_each_card_rtds(card, rtd) { - for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (component->driver->remove_order != order) continue;
@@ -1269,9 +1269,7 @@ static int soc_probe_link_components(struct snd_soc_card *card)
for_each_comp_order(order) { for_each_card_rtds(card, rtd) { - for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (component->driver->probe_order != order) continue;
@@ -1520,9 +1518,7 @@ static int soc_link_init(struct snd_soc_card *card, * topology based drivers can use the DAI link id field to set PCM * device number and then use rtd + a base offset of the BEs. */ - for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (!component->driver->use_dai_pcm_id) continue;
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 5d973ac..b890481 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -118,11 +118,8 @@ bool snd_soc_runtime_ignore_pmdown_time(struct snd_soc_pcm_runtime *rtd) if (!rtd->pmdown_time || rtd->dai_link->ignore_pmdown_time) return true;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) ignore &= !component->driver->use_pmdown_time; - }
return ignore; } @@ -435,8 +432,7 @@ static int soc_pcm_components_open(struct snd_pcm_substream *substream, struct snd_soc_component *component; int ret = 0;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; + for_each_rtd_components(rtd, rtdcom, component) { *last = component;
ret = snd_soc_component_module_get_when_open(component); @@ -467,9 +463,7 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream, struct snd_soc_component *component; int ret = 0;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (component == last) break;
@@ -500,9 +494,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) for_each_rtd_codec_dai(rtd, i, codec_dai) pinctrl_pm_select_default_state(codec_dai->dev);
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { pm_runtime_get_sync(component->dev); }
@@ -625,9 +617,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) out: mutex_unlock(&rtd->card->pcm_mutex);
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { pm_runtime_mark_last_busy(component->dev); pm_runtime_put_autosuspend(component->dev); } @@ -740,9 +730,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
mutex_unlock(&rtd->card->pcm_mutex);
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { pm_runtime_mark_last_busy(component->dev); pm_runtime_put_autosuspend(component->dev); } @@ -782,9 +770,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream) } }
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { ret = snd_soc_component_prepare(component, substream); if (ret < 0) { dev_err(component->dev, @@ -849,9 +835,7 @@ static int soc_pcm_components_hw_free(struct snd_pcm_substream *substream, struct snd_soc_component *component; int ret = 0;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { if (component == last) break;
@@ -945,9 +929,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
snd_soc_dapm_update_dai(substream, params, cpu_dai);
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { ret = snd_soc_component_hw_params(component, substream, params); if (ret < 0) { dev_err(component->dev, @@ -1062,9 +1044,7 @@ static int soc_pcm_trigger_start(struct snd_pcm_substream *substream, int cmd) return ret; }
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { ret = snd_soc_component_trigger(component, substream, cmd); if (ret < 0) return ret; @@ -1102,9 +1082,7 @@ static int soc_pcm_trigger_stop(struct snd_pcm_substream *substream, int cmd) if (ret < 0) return ret;
- for_each_rtdcom(rtd, rtdcom) { - component = rtdcom->component; - + for_each_rtd_components(rtd, rtdcom, component) { ret = snd_soc_component_trigger(component, substream, cmd); if (ret < 0) return ret; @@ -2886,6 +2864,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) struct snd_soc_dai *codec_dai; struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_rtdcom_list *rtdcom; + struct snd_soc_component *component; struct snd_pcm *pcm; char new_name[64]; int ret = 0, playback = 0, capture = 0; @@ -2997,8 +2976,8 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) rtd->ops.ioctl = snd_soc_pcm_component_ioctl; }
- for_each_rtdcom(rtd, rtdcom) { - const struct snd_soc_component_driver *drv = rtdcom->component->driver; + for_each_rtd_components(rtd, rtdcom, component) { + const struct snd_soc_component_driver *drv = component->driver;
if (drv->copy_user) rtd->ops.copy_user = snd_soc_pcm_component_copy_user;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch moves soc_init_dai_link() next to soc_bind_dai_link(). This is prepare for soc_bind_dai_link() cleanup.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 192 +++++++++++++++++++++++++-------------------------- 1 file changed, 96 insertions(+), 96 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 0603702..335dc8f 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -941,6 +941,102 @@ 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) +{ + int i; + struct snd_soc_dai_link_component *codec, *platform; + + for_each_link_codecs(link, i, codec) { + /* + * Codec must be specified by 1 of name or OF node, + * not both or neither. + */ + if (!!codec->name == !!codec->of_node) { + dev_err(card->dev, "ASoC: Neither/both codec name/of_node are set for %s\n", + link->name); + return -EINVAL; + } + + /* Codec DAI name must be specified */ + if (!codec->dai_name) { + dev_err(card->dev, "ASoC: codec_dai_name not set for %s\n", + link->name); + return -EINVAL; + } + + /* + * Defer card registration if codec component is not added to + * component list. + */ + if (!soc_find_component(codec)) + return -EPROBE_DEFER; + } + + for_each_link_platforms(link, i, platform) { + /* + * Platform may be specified by either name or OF node, but it + * can be left unspecified, then no components will be inserted + * in the rtdcom list + */ + if (!!platform->name == !!platform->of_node) { + dev_err(card->dev, + "ASoC: Neither/both platform name/of_node are set for %s\n", + link->name); + return -EINVAL; + } + + /* + * Defer card registration if platform component is not added to + * component list. + */ + if (!soc_find_component(platform)) + return -EPROBE_DEFER; + } + + /* FIXME */ + if (link->num_cpus > 1) { + dev_err(card->dev, + "ASoC: multi cpu is not yet supported %s\n", + link->name); + return -EINVAL; + } + + /* + * CPU device may be specified by either name or OF node, but + * can be left unspecified, and will be matched based on DAI + * name alone.. + */ + if (link->cpus->name && link->cpus->of_node) { + dev_err(card->dev, + "ASoC: Neither/both cpu name/of_node are set for %s\n", + link->name); + return -EINVAL; + } + + /* + * Defer card registartion if cpu dai component is not added to + * component list. + */ + if ((link->cpus->of_node || link->cpus->name) && + !soc_find_component(link->cpus)) + return -EPROBE_DEFER; + + /* + * At least one of CPU DAI name or CPU device name/node must be + * specified + */ + if (!link->cpus->dai_name && + !(link->cpus->name || link->cpus->of_node)) { + dev_err(card->dev, + "ASoC: Neither cpu_dai_name nor cpu_name/of_node are set for %s\n", + link->name); + return -EINVAL; + } + + return 0; +} + static int soc_bind_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { @@ -1283,102 +1379,6 @@ static int soc_probe_link_components(struct snd_soc_card *card) return 0; }
-static int soc_init_dai_link(struct snd_soc_card *card, - struct snd_soc_dai_link *link) -{ - int i; - struct snd_soc_dai_link_component *codec, *platform; - - for_each_link_codecs(link, i, codec) { - /* - * Codec must be specified by 1 of name or OF node, - * not both or neither. - */ - if (!!codec->name == !!codec->of_node) { - dev_err(card->dev, "ASoC: Neither/both codec name/of_node are set for %s\n", - link->name); - return -EINVAL; - } - - /* Codec DAI name must be specified */ - if (!codec->dai_name) { - dev_err(card->dev, "ASoC: codec_dai_name not set for %s\n", - link->name); - return -EINVAL; - } - - /* - * Defer card registration if codec component is not added to - * component list. - */ - if (!soc_find_component(codec)) - return -EPROBE_DEFER; - } - - for_each_link_platforms(link, i, platform) { - /* - * Platform may be specified by either name or OF node, but it - * can be left unspecified, then no components will be inserted - * in the rtdcom list - */ - if (!!platform->name == !!platform->of_node) { - dev_err(card->dev, - "ASoC: Neither/both platform name/of_node are set for %s\n", - link->name); - return -EINVAL; - } - - /* - * Defer card registration if platform component is not added to - * component list. - */ - if (!soc_find_component(platform)) - return -EPROBE_DEFER; - } - - /* FIXME */ - if (link->num_cpus > 1) { - dev_err(card->dev, - "ASoC: multi cpu is not yet supported %s\n", - link->name); - return -EINVAL; - } - - /* - * CPU device may be specified by either name or OF node, but - * can be left unspecified, and will be matched based on DAI - * name alone.. - */ - if (link->cpus->name && link->cpus->of_node) { - dev_err(card->dev, - "ASoC: Neither/both cpu name/of_node are set for %s\n", - link->name); - return -EINVAL; - } - - /* - * Defer card registartion if cpu dai component is not added to - * component list. - */ - if ((link->cpus->of_node || link->cpus->name) && - !soc_find_component(link->cpus)) - return -EPROBE_DEFER; - - /* - * At least one of CPU DAI name or CPU device name/node must be - * specified - */ - if (!link->cpus->dai_name && - !(link->cpus->name || link->cpus->of_node)) { - dev_err(card->dev, - "ASoC: Neither cpu_dai_name nor cpu_name/of_node are set for %s\n", - link->name); - return -EINVAL; - } - - return 0; -} - void snd_soc_disconnect_sync(struct device *dev) { struct snd_soc_component *component =
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; - } - } 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;
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)@@ -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(...); }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_is_dai_link_bound() check will be called both *before* soc_bind_dai_link() (A), and *under* soc_bind_dai_link() (B). These are very verboqse code. Let's remove one of them.
* static int soc_bind_dai_link(...) { ... (B) if (soc_is_dai_link_bound(...)) { ... return 0; } ... }
static int snd_soc_instantiate_card(...) { ... for_each_card_links(...) { (A) if (soc_is_dai_link_bound(...)) continue;
* ret = soc_bind_dai_link(...); if (ret) goto probe_end; } ... }
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index f440022..4edac93 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2065,9 +2065,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) * Components with topology may bring new DAIs and DAI links. */ for_each_card_links(card, dai_link) { - if (soc_is_dai_link_bound(card, dai_link)) - continue; - ret = soc_bind_dai_link(card, dai_link); if (ret) goto probe_end;
On 10/8/19 11:30 PM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_is_dai_link_bound() check will be called both *before* soc_bind_dai_link() (A), and *under* soc_bind_dai_link() (B). These are very verboqse code. Let's remove one of them.
typo: verbose.
- static int soc_bind_dai_link(...) { ...
(B) if (soc_is_dai_link_bound(...)) { ... return 0; } ... }
static int snd_soc_instantiate_card(...) { ... for_each_card_links(...) { (A) if (soc_is_dai_link_bound(...)) continue;
ret = soc_bind_dai_link(...); if (ret) goto probe_end;Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-core.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index f440022..4edac93 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2065,9 +2065,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) * Components with topology may bring new DAIs and DAI links. */ for_each_card_links(card, dai_link) {
if (soc_is_dai_link_bound(card, dai_link))continue;- ret = soc_bind_dai_link(card, dai_link); if (ret) goto probe_end;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
If we focus to soc_bind_dai_link() at snd_soc_instantiate_card(), we will notice very complex operation.
static int snd_soc_instantiate_card(...) { ... /* * (1) find component via card pre-linked dai_link * * find component (CPU/Codec/Platform) via card pre-linked * dai_link, and connect found component to *rtd*. * for_each_card_prelinks() is dai_link loop for card pre-linked */ for_each_card_prelinks(card, i, dai_link) { ret = soc_bind_dai_link(card, dai_link); ... } ... /* * (2) connect card pre-linked dai_link to card list * * connect all card pre-linked dai_link to *card list*. */ for_each_card_prelinks(card, i, dai_link) { ret = snd_soc_add_dai_link(card, dai_link); ... } ... /* * (3) probe binded component * * probe *rtd* connected component. This means, connected * component at (1) is the probe target. * * At this component probe, topology may add new dai_link to * *card list* by using snd_soc_add_dai_link() which is * used at (2). */ ret = soc_probe_link_components(card); ...
/* * (4) find component again * * find component again. card pre-linked dai_link listed components * are already found at (1), but topology added one via (3) is not * yet found. Here try to find component for it. * * for_each_card_links() here means *card list* loop, * which is connected via (2) and (3). * It ignores already added one, this means it ignores component * which is connected at (1). * Thus, find target here is new added one at (3). */ for_each_card_links(card, dai_link) { ret = soc_bind_dai_link(card, dai_link); ... } ... }
It is doing very complex method. The problem is finding component for "card pre-linked" (= (1)) and "topology added dai_link" (= (3)) are separated. If we can "find component" when dai_link is connected to "card list", the code will be very simple. This patch fixup it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 4edac93..a0c80f7 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1409,6 +1409,8 @@ EXPORT_SYMBOL_GPL(snd_soc_disconnect_sync); int snd_soc_add_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { + int ret; + if (dai_link->dobj.type && dai_link->dobj.type != SND_SOC_DOBJ_DAI_LINK) { dev_err(card->dev, "Invalid dai link type %d\n", @@ -1424,6 +1426,10 @@ int snd_soc_add_dai_link(struct snd_soc_card *card, if (dai_link->dobj.type && card->add_dai_link) card->add_dai_link(card, dai_link);
+ ret = soc_bind_dai_link(card, dai_link); + if (ret < 0) + return ret; + /* see for_each_card_links */ list_add_tail(&dai_link->list, &card->dai_link_list);
@@ -1996,13 +2002,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) /* check whether any platform is ignore machine FE and using topology */ soc_check_tplg_fes(card);
- /* bind DAIs */ - for_each_card_prelinks(card, i, dai_link) { - ret = soc_bind_dai_link(card, dai_link); - if (ret != 0) - goto probe_end; - } - /* bind aux_devs too */ ret = soc_bind_aux_dev(card); if (ret < 0) @@ -2060,16 +2059,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card) if (ret < 0) goto probe_end;
- /* - * Find new DAI links added during probing components and bind them. - * Components with topology may bring new DAIs and DAI links. - */ - for_each_card_links(card, dai_link) { - ret = soc_bind_dai_link(card, dai_link); - if (ret) - goto probe_end; - } - /* probe all DAI links on this card */ ret = soc_probe_link_dais(card); if (ret < 0) {
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.
ALSA SoC has soc_bind_dai_link(), but its paired soc_unbind_dai_link() is not implemented. More confusable is that soc_remove_pcm_runtimes() which should be soc_unbind_dai_link() is implemented without synchronised to soc_bind_dai_link().
This patch cleanup this unbalance.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index a0c80f7..7f445b9 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -470,14 +470,6 @@ static struct snd_soc_pcm_runtime *soc_new_pcm_runtime( return NULL; }
-static void soc_remove_pcm_runtimes(struct snd_soc_card *card) -{ - struct snd_soc_pcm_runtime *rtd, *_rtd; - - for_each_card_rtds_safe(card, rtd, _rtd) - soc_free_pcm_runtime(rtd); -} - struct snd_soc_pcm_runtime *snd_soc_get_pcm_runtime(struct snd_soc_card *card, const char *dai_link) { @@ -1037,6 +1029,16 @@ static int soc_dai_link_sanity_check(struct snd_soc_card *card, return 0; }
+static void soc_unbind_dai_link(struct snd_soc_card *card, + struct snd_soc_dai_link *dai_link) +{ + struct snd_soc_pcm_runtime *rtd; + + rtd = snd_soc_get_pcm_runtime(card, dai_link->name); + if (rtd) + soc_free_pcm_runtime(rtd); +} + static int soc_bind_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { @@ -1466,6 +1468,8 @@ void snd_soc_remove_dai_link(struct snd_soc_card *card, card->remove_dai_link(card, dai_link);
list_del(&dai_link->list); + + soc_unbind_dai_link(card, dai_link); } EXPORT_SYMBOL_GPL(snd_soc_remove_dai_link);
@@ -1974,8 +1978,6 @@ static void soc_cleanup_card_resources(struct snd_soc_card *card) for_each_card_links_safe(card, link, _link) snd_soc_remove_dai_link(card, link);
- soc_remove_pcm_runtimes(card); - /* remove auxiliary devices */ soc_remove_aux_devices(card); soc_unbind_aux_dev(card);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_remove_link_components() will be called from soc_cleanup_card_resources(). This patch removes duplicate call.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 7f445b9..283ac63 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2422,9 +2422,6 @@ static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister) snd_soc_dapm_shutdown(card); snd_soc_flush_all_delayed_work(card);
- /* remove all components used by DAI links on this card */ - soc_remove_link_components(card); - soc_cleanup_card_resources(card); if (!unregister) list_add(&card->list, &unbind_card_list);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_tplg_component_remove() is topology related cleanup function. The driver which added topology needed cleanup it, not by soc-core. Only topology user skl-pcm is calling it, there is no effect by this patch.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 283ac63..fa837c0 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2870,8 +2870,6 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue;
- snd_soc_tplg_component_remove(component, - SND_SOC_TPLG_INDEX_ALL); snd_soc_component_del_unlocked(component); found = 1; break;
On 10/8/19 11:30 PM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_tplg_component_remove() is topology related cleanup function. The driver which added topology needed cleanup it, not by soc-core. Only topology user skl-pcm is calling it, there is no effect by this patch.
the SOF driver also calls snd_soc_tplg_component_remove(), so not sure what you meant by the comment?
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-core.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 283ac63..fa837c0 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2870,8 +2870,6 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue;
snd_soc_tplg_component_remove(component,SND_SOC_TPLG_INDEX_ALL);
Hi Pierre-Louis
snd_soc_tplg_component_remove() is topology related cleanup function. The driver which added topology needed cleanup it, not by soc-core. Only topology user skl-pcm is calling it, there is no effect by this patch.
(snip)
--- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2870,8 +2870,6 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue;
snd_soc_tplg_component_remove(component,SND_SOC_TPLG_INDEX_ALL);
(snip)
the SOF driver also calls snd_soc_tplg_component_remove(), so not sure what you meant by the comment?
Ahh, yes indeed.
My opinion is that driver who called _load() need to call _remove() under his responsibility.
Today, skl-pcm and topology are the user. They are calling both _load() and_remove(). Thus, I think soc-core don't need to call it ?
If we want to keep it as robustness, I want to have this comment, otherwise very confusable, because soc-core never call _load() but calling _remove()
/* For framework level robustness */ snd_soc_tplg_component_remove(...);
Thank you for your help !! Best regards --- Kuninori Morimoto
snd_soc_tplg_component_remove() is topology related cleanup function. The driver which added topology needed cleanup it, not by soc-core. Only topology user skl-pcm is calling it, there is no effect by this patch.
(snip)
--- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2870,8 +2870,6 @@ static int __snd_soc_unregister_component(struct device *dev) if (dev != component->dev) continue;
snd_soc_tplg_component_remove(component,SND_SOC_TPLG_INDEX_ALL); snd_soc_component_del_unlocked(component);(snip)
the SOF driver also calls snd_soc_tplg_component_remove(), so not sure what you meant by the comment?
Ahh, yes indeed.
My opinion is that driver who called _load() need to call _remove() under his responsibility.
Today, skl-pcm and topology are the user. They are calling both _load() and_remove(). Thus, I think soc-core don't need to call it ?
If we want to keep it as robustness, I want to have this comment, otherwise very confusable, because soc-core never call _load() but calling _remove()
/* For framework level robustness */ snd_soc_tplg_component_remove(...);
I would need Ranjani's help here. I vaguely remember that at some point we relied on the topology being removed by the framework, then we did it on our own but can't recall the reason.
Ranjani, if you've got power now, can you chime in?
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch moves snd_soc_lookup_component() to upper side. This is prepare for snd_soc_unregister_component()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 52 ++++++++++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index fa837c0..72eb59c 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -356,6 +356,32 @@ struct snd_soc_component *snd_soc_rtdcom_lookup(struct snd_soc_pcm_runtime *rtd, } EXPORT_SYMBOL_GPL(snd_soc_rtdcom_lookup);
+struct snd_soc_component *snd_soc_lookup_component(struct device *dev, + const char *driver_name) +{ + struct snd_soc_component *component; + struct snd_soc_component *ret; + + ret = NULL; + mutex_lock(&client_mutex); + for_each_component(component) { + if (dev != component->dev) + continue; + + if (driver_name && + (driver_name != component->driver->name) && + (strcmp(component->driver->name, driver_name) != 0)) + continue; + + ret = component; + break; + } + mutex_unlock(&client_mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(snd_soc_lookup_component); + struct snd_pcm_substream *snd_soc_get_dai_substream(struct snd_soc_card *card, const char *dai_link, int stream) { @@ -2889,32 +2915,6 @@ void snd_soc_unregister_component(struct device *dev) } EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
-struct snd_soc_component *snd_soc_lookup_component(struct device *dev, - const char *driver_name) -{ - struct snd_soc_component *component; - struct snd_soc_component *ret; - - ret = NULL; - mutex_lock(&client_mutex); - for_each_component(component) { - if (dev != component->dev) - continue; - - if (driver_name && - (driver_name != component->driver->name) && - (strcmp(component->driver->name, driver_name) != 0)) - continue; - - ret = component; - break; - } - mutex_unlock(&client_mutex); - - return ret; -} -EXPORT_SYMBOL_GPL(snd_soc_lookup_component); - /* Retrieve a card's name from device tree */ int snd_soc_of_parse_card_name(struct snd_soc_card *card, const char *propname)
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.
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; }
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);
- 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
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_unregister_component() is now finding component manually, but we already have snd_soc_lookup_component() to find component; Let's use existing function.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 7c0bb32..3e8ed4f 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2823,8 +2823,10 @@ static void snd_soc_try_rebind_card(void)
static void snd_soc_del_component(struct snd_soc_component *component) { + mutex_lock(&client_mutex); snd_soc_unregister_dais(component); snd_soc_component_del(component); + mutex_unlock(&client_mutex); }
int snd_soc_add_component(struct device *dev, @@ -2887,29 +2889,16 @@ EXPORT_SYMBOL_GPL(snd_soc_register_component); * * @dev: The device to unregister */ -static int __snd_soc_unregister_component(struct device *dev) +void snd_soc_unregister_component(struct device *dev) { struct snd_soc_component *component; - int found = 0; - - mutex_lock(&client_mutex); - for_each_component(component) { - if (dev != component->dev) - continue;
+ while (1) { + component = snd_soc_lookup_component(dev, NULL); + if (!component) + break; snd_soc_del_component(component); - found = 1; - break; } - mutex_unlock(&client_mutex); - - return found; -} - -void snd_soc_unregister_component(struct device *dev) -{ - while (__snd_soc_unregister_component(dev)) - ; } EXPORT_SYMBOL_GPL(snd_soc_unregister_component);
On 10/8/19 11:30 PM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_unregister_component() is now finding component manually, but we already have snd_soc_lookup_component() to find component; Let's use existing function.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-core.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 7c0bb32..3e8ed4f 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2823,8 +2823,10 @@ static void snd_soc_try_rebind_card(void)
static void snd_soc_del_component(struct snd_soc_component *component) {
mutex_lock(&client_mutex); snd_soc_unregister_dais(component); snd_soc_component_del(component);
mutex_unlock(&client_mutex); }
int snd_soc_add_component(struct device *dev,
@@ -2887,29 +2889,16 @@ EXPORT_SYMBOL_GPL(snd_soc_register_component);
- @dev: The device to unregister
*/ -static int __snd_soc_unregister_component(struct device *dev) +void snd_soc_unregister_component(struct device *dev) { struct snd_soc_component *component;
- int found = 0;
- mutex_lock(&client_mutex);
- for_each_component(component) {
if (dev != component->dev)continue;
- while (1) {
component = snd_soc_lookup_component(dev, NULL);if (!component)break;
is it ok/intended that the mutex lock is now taken *after* looking up the component and after each iteration ?
found = 1;break;- mutex_unlock(&client_mutex);
- return found;
-}
-void snd_soc_unregister_component(struct device *dev) -{
- while (__snd_soc_unregister_component(dev))
;
Hi Pierre-Louis
+void snd_soc_unregister_component(struct device *dev) { struct snd_soc_component *component;
- int found = 0;
- mutex_lock(&client_mutex);
- for_each_component(component) {
if (dev != component->dev)continue;
- while (1) {
component = snd_soc_lookup_component(dev, NULL);if (!component)break;is it ok/intended that the mutex lock is now taken *after* looking up the component and after each iteration ?
Hmm.. maybe not good. I will keep _unlocked() naming and mutex
Thank you for your help !! Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch moves snd_soc_register_dai() next to snd_soc_register_dais(). This is prepare for snd_soc_register_dais() cleanup.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 74 ++++++++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 3e8ed4f..be4e1b5 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2601,42 +2601,6 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, }
/** - * snd_soc_register_dais - Register a DAI with the ASoC core - * - * @component: The component the DAIs are registered for - * @dai_drv: DAI driver to use for the DAIs - * @count: Number of DAIs - */ -static int snd_soc_register_dais(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv, - size_t count) -{ - struct device *dev = component->dev; - struct snd_soc_dai *dai; - unsigned int i; - int ret; - - dev_dbg(dev, "ASoC: dai register %s #%zu\n", dev_name(dev), count); - - for (i = 0; i < count; i++) { - - dai = soc_add_dai(component, dai_drv + i, count == 1 && - !component->driver->non_legacy_dai_naming); - if (dai == NULL) { - ret = -ENOMEM; - goto err; - } - } - - return 0; - -err: - snd_soc_unregister_dais(component); - - return ret; -} - -/** * snd_soc_register_dai - Register a DAI dynamically & create its widgets * * @component: The component the DAIs are registered for @@ -2647,7 +2611,7 @@ static int snd_soc_register_dais(struct snd_soc_component *component, * will be freed in the component cleanup. */ int snd_soc_register_dai(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv) + struct snd_soc_dai_driver *dai_drv) { struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); @@ -2679,6 +2643,42 @@ int snd_soc_register_dai(struct snd_soc_component *component, } EXPORT_SYMBOL_GPL(snd_soc_register_dai);
+/** + * snd_soc_register_dais - Register a DAI with the ASoC core + * + * @component: The component the DAIs are registered for + * @dai_drv: DAI driver to use for the DAIs + * @count: Number of DAIs + */ +static int snd_soc_register_dais(struct snd_soc_component *component, + struct snd_soc_dai_driver *dai_drv, + size_t count) +{ + struct device *dev = component->dev; + struct snd_soc_dai *dai; + unsigned int i; + int ret; + + dev_dbg(dev, "ASoC: dai register %s #%zu\n", dev_name(dev), count); + + for (i = 0; i < count; i++) { + + dai = soc_add_dai(component, dai_drv + i, count == 1 && + !component->driver->non_legacy_dai_naming); + if (dai == NULL) { + ret = -ENOMEM; + goto err; + } + } + + return 0; + +err: + snd_soc_unregister_dais(component); + + return ret; +} + static int snd_soc_component_initialize(struct snd_soc_component *component, const struct snd_soc_component_driver *driver, struct device *dev) {
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
ALSA SoC has 2 similar but diffarent implementation. snd_soc_register_dai() is used from topology snd_soc_register_dais() is used from snd_soc_add_component()
Because of this kind of confusable naming, and duplicated implementation, code is very unreadale. We shouldn't have duplicated and confusable implementation.
snd_soc_register_dai() is now used from topology. But, to reduce duplicated code, it will be used from soc-core, too. To prepare for it, this patch adds missing parameter legacy_dai_naming to snd_soc_register_dai().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 3 ++- sound/soc/soc-core.c | 5 +++-- sound/soc/soc-topology.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 320dcd4..827322a 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1327,7 +1327,8 @@ struct snd_soc_dai_link *snd_soc_find_dai_link(struct snd_soc_card *card, const char *stream_name);
int snd_soc_register_dai(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv); + struct snd_soc_dai_driver *dai_drv, + bool legacy_dai_naming);
struct snd_soc_dai *snd_soc_find_dai( const struct snd_soc_dai_link_component *dlc); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index be4e1b5..3a16868 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2611,7 +2611,8 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, * will be freed in the component cleanup. */ int snd_soc_register_dai(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv) + struct snd_soc_dai_driver *dai_drv, + bool legacy_dai_naming) { struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); @@ -2625,7 +2626,7 @@ int snd_soc_register_dai(struct snd_soc_component *component, }
lockdep_assert_held(&client_mutex); - dai = soc_add_dai(component, dai_drv, false); + dai = soc_add_dai(component, dai_drv, legacy_dai_naming); if (!dai) return -ENOMEM;
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 0fd0329..b6e1456 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1842,7 +1842,7 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg, list_add(&dai_drv->dobj.list, &tplg->comp->dobj_list);
/* register the DAI to the component */ - return snd_soc_register_dai(tplg->comp, dai_drv); + return snd_soc_register_dai(tplg->comp, dai_drv, false); }
static void set_link_flags(struct snd_soc_dai_link *link,
On 10/8/19 11:30 PM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
ALSA SoC has 2 similar but diffarent implementation.
different
snd_soc_register_dai() is used from topology snd_soc_register_dais() is used from snd_soc_add_component()
Because of this kind of confusable naming, and duplicated
confusing
implementation, code is very unreadale.
unreadable
We shouldn't have duplicated and confusable implementation.
confusing
snd_soc_register_dai() is now used from topology. But, to reduce duplicated code, it will be used from soc-core, too. To prepare for it, this patch adds missing parameter legacy_dai_naming to snd_soc_register_dai().
It doesn't look like this series reduces the confusion between snd_soc_register_dai() and snd_soc_register_dais() ?
maybe for the latter case since it's a static function we don't want the entire prefix then?
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
include/sound/soc.h | 3 ++- sound/soc/soc-core.c | 5 +++-- sound/soc/soc-topology.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 320dcd4..827322a 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1327,7 +1327,8 @@ struct snd_soc_dai_link *snd_soc_find_dai_link(struct snd_soc_card *card, const char *stream_name);
int snd_soc_register_dai(struct snd_soc_component *component,
- struct snd_soc_dai_driver *dai_drv);
struct snd_soc_dai_driver *dai_drv,bool legacy_dai_naming);struct snd_soc_dai *snd_soc_find_dai( const struct snd_soc_dai_link_component *dlc);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index be4e1b5..3a16868 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2611,7 +2611,8 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component,
- will be freed in the component cleanup.
*/ int snd_soc_register_dai(struct snd_soc_component *component,
struct snd_soc_dai_driver *dai_drv)
struct snd_soc_dai_driver *dai_drv,bool legacy_dai_naming)@@ -2625,7 +2626,7 @@ int snd_soc_register_dai(struct snd_soc_component *component, }
lockdep_assert_held(&client_mutex);
- dai = soc_add_dai(component, dai_drv, false);
- dai = soc_add_dai(component, dai_drv, legacy_dai_naming); if (!dai) return -ENOMEM;
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 0fd0329..b6e1456 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1842,7 +1842,7 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg, list_add(&dai_drv->dobj.list, &tplg->comp->dobj_list);
/* register the DAI to the component */
- return snd_soc_register_dai(tplg->comp, dai_drv);
return snd_soc_register_dai(tplg->comp, dai_drv, false); }
static void set_link_flags(struct snd_soc_dai_link *link,
Hi Pierre-Louis
Thank you for your feedback
snd_soc_register_dai() is now used from topology. But, to reduce duplicated code, it will be used from soc-core, too. To prepare for it, this patch adds missing parameter legacy_dai_naming to snd_soc_register_dai().
It doesn't look like this series reduces the confusion between snd_soc_register_dai() and snd_soc_register_dais() ?
maybe for the latter case since it's a static function we don't want the entire prefix then?
Maybe my explain is not so good... The point is that, in general people think like below from naming. Other functions are this style.
=> int snd_soc_register_dai() { ... }
int snd_soc_register_dais() { for(..) { => snd_soc_register_dai() } }
But in reality is like this
int snd_soc_register_dai() { /* almost same but different code */ }
snd_soc_register_dais() { /* almost same but different code */ }
To avoid duplicate code and confusion, this patchset try to implement "general" style. But needs some preparation.
I will fix log and English.
Thank you for your help !! Best regards --- Kuninori Morimoto
On 10/10/19 8:07 PM, Kuninori Morimoto wrote:
Hi Pierre-Louis
Thank you for your feedback
snd_soc_register_dai() is now used from topology. But, to reduce duplicated code, it will be used from soc-core, too. To prepare for it, this patch adds missing parameter legacy_dai_naming to snd_soc_register_dai().
It doesn't look like this series reduces the confusion between snd_soc_register_dai() and snd_soc_register_dais() ?
maybe for the latter case since it's a static function we don't want the entire prefix then?
Maybe my explain is not so good... The point is that, in general people think like below from naming. Other functions are this style.
=> int snd_soc_register_dai() { ... }
int snd_soc_register_dais() { for(..) { => snd_soc_register_dai() } }
But in reality is like this
int snd_soc_register_dai() { /* almost same but different code */ }
snd_soc_register_dais() { /* almost same but different code */ }
To avoid duplicate code and confusion, this patchset try to implement "general" style. But needs some preparation.
I will fix log and English.
I get your point, what I was trying to suggest is that we only use the full snd_soc prefix for non-static functions that can be called by other files, for static functions we can use just the soc prefix. It's a convention that helps the reader understand what is local and what is common/shared.
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch moves snd_soc_unregister_dais() next to snd_soc_register_dais(). This is prepare for snd_soc_register_dais() cleanup
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 3a16868..12cb1a0 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2535,22 +2535,6 @@ static inline char *fmt_multiple_name(struct device *dev, return devm_kstrdup(dev, dai_drv->name, GFP_KERNEL); }
-/** - * snd_soc_unregister_dai - Unregister DAIs from the ASoC core - * - * @component: The component for which the DAIs should be unregistered - */ -static void snd_soc_unregister_dais(struct snd_soc_component *component) -{ - struct snd_soc_dai *dai, *_dai; - - for_each_component_dais_safe(component, dai, _dai) { - dev_dbg(component->dev, "ASoC: Unregistered DAI '%s'\n", - dai->name); - list_del(&dai->list); - } -} - /* Create a DAI and add it to the component's DAI list */ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv, @@ -2645,6 +2629,22 @@ int snd_soc_register_dai(struct snd_soc_component *component, EXPORT_SYMBOL_GPL(snd_soc_register_dai);
/** + * snd_soc_unregister_dai - Unregister DAIs from the ASoC core + * + * @component: The component for which the DAIs should be unregistered + */ +static void snd_soc_unregister_dais(struct snd_soc_component *component) +{ + struct snd_soc_dai *dai, *_dai; + + for_each_component_dais_safe(component, dai, _dai) { + dev_dbg(component->dev, "ASoC: Unregistered DAI '%s'\n", + dai->name); + list_del(&dai->list); + } +} + +/** * snd_soc_register_dais - Register a DAI with the ASoC core * * @component: The component the DAIs are registered for
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.
This patch adds missing soc_del_dai() and snd_soc_unregister_dai().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 1 + sound/soc/soc-core.c | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 827322a..766fa81 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1329,6 +1329,7 @@ struct snd_soc_dai_link *snd_soc_find_dai_link(struct snd_soc_card *card, int snd_soc_register_dai(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv, bool legacy_dai_naming); +void snd_soc_unregister_dai(struct snd_soc_dai *dai);
struct snd_soc_dai *snd_soc_find_dai( const struct snd_soc_dai_link_component *dlc); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 12cb1a0..d9f968c 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2535,6 +2535,12 @@ static inline char *fmt_multiple_name(struct device *dev, return devm_kstrdup(dev, dai_drv->name, GFP_KERNEL); }
+static void soc_del_dai(struct snd_soc_dai *dai) +{ + dev_dbg(dai->dev, "ASoC: Unregistered DAI '%s'\n", dai->name); + list_del(&dai->list); +} + /* Create a DAI and add it to the component's DAI list */ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv, @@ -2584,6 +2590,12 @@ static struct snd_soc_dai *soc_add_dai(struct snd_soc_component *component, return dai; }
+void snd_soc_unregister_dai(struct snd_soc_dai *dai) +{ + soc_del_dai(dai); +} +EXPORT_SYMBOL_GPL(snd_soc_unregister_dai); + /** * snd_soc_register_dai - Register a DAI dynamically & create its widgets * @@ -2637,11 +2649,8 @@ static void snd_soc_unregister_dais(struct snd_soc_component *component) { struct snd_soc_dai *dai, *_dai;
- for_each_component_dais_safe(component, dai, _dai) { - dev_dbg(component->dev, "ASoC: Unregistered DAI '%s'\n", - dai->name); - list_del(&dai->list); - } + for_each_component_dais_safe(component, dai, _dai) + snd_soc_unregister_dai(dai); }
/**
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
ALSA SoC has 2 similar but diffarent implementation. snd_soc_register_dai() is used from topology snd_soc_register_dais() is used from snd_soc_add_component()
Because of this kind of confusable naming, and duplicated implementation, code is very unreadale. We shouldn't have duplicated and confusable implementation.
snd_soc_register_dai() is called from soc-topology, but it should be used from soc-core, too.
Because of topology side specific reason, it is calling snd_soc_dapm_new_dai_widgets(), but it is not needed soc-core side.
This patch factorizes snd_soc_register_dai() to soc-topology / soc-core common part, and soc-topology specific part. And do topology specific part at soc-topology.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 6 +++--- sound/soc/soc-core.c | 25 ++++++------------------- sound/soc/soc-topology.c | 17 ++++++++++++++++- 3 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 766fa81..4e38ee6 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1326,9 +1326,9 @@ struct snd_soc_dai_link *snd_soc_find_dai_link(struct snd_soc_card *card, int id, const char *name, const char *stream_name);
-int snd_soc_register_dai(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv, - bool legacy_dai_naming); +struct snd_soc_dai *snd_soc_register_dai(struct snd_soc_component *component, + struct snd_soc_dai_driver *dai_drv, + bool legacy_dai_naming); void snd_soc_unregister_dai(struct snd_soc_dai *dai);
struct snd_soc_dai *snd_soc_find_dai( diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index d9f968c..5d96ffa 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2606,37 +2606,24 @@ EXPORT_SYMBOL_GPL(snd_soc_unregister_dai); * These DAIs's widgets will be freed in the card cleanup and the DAIs * will be freed in the component cleanup. */ -int snd_soc_register_dai(struct snd_soc_component *component, - struct snd_soc_dai_driver *dai_drv, - bool legacy_dai_naming) +struct snd_soc_dai *snd_soc_register_dai(struct snd_soc_component *component, + struct snd_soc_dai_driver *dai_drv, + bool legacy_dai_naming) { - struct snd_soc_dapm_context *dapm = - snd_soc_component_get_dapm(component); struct snd_soc_dai *dai; - int ret;
if (dai_drv->dobj.type != SND_SOC_DOBJ_PCM) { dev_err(component->dev, "Invalid dai type %d\n", dai_drv->dobj.type); - return -EINVAL; + return NULL; }
lockdep_assert_held(&client_mutex); dai = soc_add_dai(component, dai_drv, legacy_dai_naming); if (!dai) - return -ENOMEM; - - /* - * Create the DAI widgets here. After adding DAIs, topology may - * also add routes that need these widgets as source or sink. - */ - ret = snd_soc_dapm_new_dai_widgets(dapm, dai); - if (ret != 0) { - dev_err(component->dev, - "Failed to create DAI widgets %d\n", ret); - } + return NULL;
- return ret; + return dai; } EXPORT_SYMBOL_GPL(snd_soc_register_dai);
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index b6e1456..81d2af0 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1800,6 +1800,9 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg, struct snd_soc_dai_driver *dai_drv; struct snd_soc_pcm_stream *stream; struct snd_soc_tplg_stream_caps *caps; + struct snd_soc_dai *dai; + struct snd_soc_dapm_context *dapm = + snd_soc_component_get_dapm(tplg->comp); int ret;
dai_drv = kzalloc(sizeof(struct snd_soc_dai_driver), GFP_KERNEL); @@ -1842,7 +1845,19 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg, list_add(&dai_drv->dobj.list, &tplg->comp->dobj_list);
/* register the DAI to the component */ - return snd_soc_register_dai(tplg->comp, dai_drv, false); + dai = snd_soc_register_dai(tplg->comp, dai_drv, false); + if (!dai) + return -ENOMEM; + + /* Create the DAI widgets here */ + ret = snd_soc_dapm_new_dai_widgets(dapm, dai); + if (ret != 0) { + dev_err(dai->dev, "Failed to create DAI widgets %d\n", ret); + snd_soc_unregister_dai(dai); + return ret; + } + + return ret; }
static void set_link_flags(struct snd_soc_dai_link *link,
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_component_add() is using mutex_lock(), but it should be used at snd_soc_add_component(). This patch tidyup it. This is prepare for snd_soc_register_dais() cleanup
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 5d96ffa..d4f80c8 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2748,8 +2748,6 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
static void snd_soc_component_add(struct snd_soc_component *component) { - mutex_lock(&client_mutex); - if (!component->driver->write && !component->driver->read) { if (!component->regmap) component->regmap = dev_get_regmap(component->dev, @@ -2760,8 +2758,6 @@ static void snd_soc_component_add(struct snd_soc_component *component)
/* see for_each_component */ list_add(&component->list, &component_list); - - mutex_unlock(&client_mutex); }
static void snd_soc_component_del(struct snd_soc_component *component) @@ -2835,9 +2831,11 @@ int snd_soc_add_component(struct device *dev, int ret; int i;
+ mutex_lock(&client_mutex); + ret = snd_soc_component_initialize(component, component_driver, dev); if (ret) - goto err_free; + goto err_del;
if (component_driver->endianness) { for (i = 0; i < num_dai; i++) { @@ -2849,17 +2847,23 @@ int snd_soc_add_component(struct device *dev, ret = snd_soc_register_dais(component, dai_drv, num_dai); if (ret < 0) { dev_err(dev, "ASoC: Failed to register DAIs: %d\n", ret); - goto err_cleanup; + goto err_del; }
snd_soc_component_add(component); - snd_soc_try_rebind_card();
- return 0; +err_del: + mutex_unlock(&client_mutex); + + /* + * these should be called without + * mutex_unlock(client_mutex) + */ + if (ret < 0) + snd_soc_del_component(component); + else + snd_soc_try_rebind_card();
-err_cleanup: - snd_soc_del_component(component); -err_free: return ret; } EXPORT_SYMBOL_GPL(snd_soc_add_component);
On 10/8/19 11:31 PM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_component_add() is using mutex_lock(), but it should be used at snd_soc_add_component(). This patch tidyup it. This is prepare for snd_soc_register_dais() cleanup
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-core.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 5d96ffa..d4f80c8 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2748,8 +2748,6 @@ EXPORT_SYMBOL_GPL(snd_soc_component_exit_regmap);
static void snd_soc_component_add(struct snd_soc_component *component) {
- mutex_lock(&client_mutex);
- if (!component->driver->write && !component->driver->read) { if (!component->regmap) component->regmap = dev_get_regmap(component->dev,
@@ -2760,8 +2758,6 @@ static void snd_soc_component_add(struct snd_soc_component *component)
/* see for_each_component */ list_add(&component->list, &component_list);
mutex_unlock(&client_mutex); }
static void snd_soc_component_del(struct snd_soc_component *component)
@@ -2835,9 +2831,11 @@ int snd_soc_add_component(struct device *dev, int ret; int i;
- mutex_lock(&client_mutex);
- ret = snd_soc_component_initialize(component, component_driver, dev); if (ret)
goto err_free;
goto err_del;if (component_driver->endianness) { for (i = 0; i < num_dai; i++) {
@@ -2849,17 +2847,23 @@ int snd_soc_add_component(struct device *dev, ret = snd_soc_register_dais(component, dai_drv, num_dai); if (ret < 0) { dev_err(dev, "ASoC: Failed to register DAIs: %d\n", ret);
goto err_cleanup;
goto err_del;}
snd_soc_component_add(component);
snd_soc_try_rebind_card();
return 0;
+err_del:
- mutex_unlock(&client_mutex);
- /*
* these should be called without* mutex_unlock(client_mutex)*/- if (ret < 0)
snd_soc_del_component(component);
the first thing done in snd_soc_del_component is to take the same mutex.
- else
snd_soc_try_rebind_card();-err_cleanup:
- snd_soc_del_component(component);
-err_free: return ret; } EXPORT_SYMBOL_GPL(snd_soc_add_component);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
ALSA SoC has 2 similar but diffarent implementation. snd_soc_register_dai() is used from topology snd_soc_register_dais() is used from snd_soc_add_component()
Because of this kind of confusable naming, and duplicated implementation, code is very unreadale. We shouldn't have duplicated and confusable implementation. This patch calls snd_soc_register_dai() from snd_soc_register_dais()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index d4f80c8..bbcaac5 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2610,14 +2610,17 @@ struct snd_soc_dai *snd_soc_register_dai(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv, bool legacy_dai_naming) { + struct device *dev = component->dev; struct snd_soc_dai *dai;
- if (dai_drv->dobj.type != SND_SOC_DOBJ_PCM) { - dev_err(component->dev, "Invalid dai type %d\n", - dai_drv->dobj.type); + if (dai_drv->dobj.type && + dai_drv->dobj.type != SND_SOC_DOBJ_PCM) { + dev_err(dev, "Invalid dai type %d\n", dai_drv->dobj.type); return NULL; }
+ dev_dbg(dev, "ASoC: dai register %s\n", dai_drv->name); + lockdep_assert_held(&client_mutex); dai = soc_add_dai(component, dai_drv, legacy_dai_naming); if (!dai) @@ -2651,16 +2654,12 @@ static int snd_soc_register_dais(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv, size_t count) { - struct device *dev = component->dev; struct snd_soc_dai *dai; unsigned int i; int ret;
- dev_dbg(dev, "ASoC: dai register %s #%zu\n", dev_name(dev), count); - for (i = 0; i < count; i++) { - - dai = soc_add_dai(component, dai_drv + i, count == 1 && + dai = snd_soc_register_dai(component, dai_drv + i, count == 1 && !component->driver->non_legacy_dai_naming); if (dai == NULL) { ret = -ENOMEM;
On 10/8/19 11:31 PM, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
ALSA SoC has 2 similar but diffarent implementation. snd_soc_register_dai() is used from topology snd_soc_register_dais() is used from snd_soc_add_component()
Because of this kind of confusable naming, and duplicated implementation, code is very unreadale. We shouldn't have duplicated and confusable implementation. This patch calls snd_soc_register_dai() from snd_soc_register_dais()
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-core.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index d4f80c8..bbcaac5 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2610,14 +2610,17 @@ struct snd_soc_dai *snd_soc_register_dai(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv, bool legacy_dai_naming) {
- struct device *dev = component->dev; struct snd_soc_dai *dai;
- if (dai_drv->dobj.type != SND_SOC_DOBJ_PCM) {
dev_err(component->dev, "Invalid dai type %d\n",dai_drv->dobj.type);
- if (dai_drv->dobj.type &&
dai_drv->dobj.type != SND_SOC_DOBJ_PCM) {dev_err(dev, "Invalid dai type %d\n", dai_drv->dobj.type);
this block of code is removed in patch 20, so do we need the modification here?
- dev_dbg(dev, "ASoC: dai register %s\n", dai_drv->name);
- lockdep_assert_held(&client_mutex); dai = soc_add_dai(component, dai_drv, legacy_dai_naming); if (!dai)
@@ -2651,16 +2654,12 @@ static int snd_soc_register_dais(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv, size_t count) {
struct device *dev = component->dev; struct snd_soc_dai *dai; unsigned int i; int ret;
dev_dbg(dev, "ASoC: dai register %s #%zu\n", dev_name(dev), count);
for (i = 0; i < count; i++) {
dai = soc_add_dai(component, dai_drv + i, count == 1 &&
dai = snd_soc_register_dai(component, dai_drv + i, count == 1 && !component->driver->non_legacy_dai_naming);
Hi Pierre-Louis
@@ -2610,14 +2610,17 @@ struct snd_soc_dai *snd_soc_register_dai(struct snd_soc_component *component, struct snd_soc_dai_driver *dai_drv, bool legacy_dai_naming) {
- struct device *dev = component->dev; struct snd_soc_dai *dai;
- if (dai_drv->dobj.type != SND_SOC_DOBJ_PCM) {
dev_err(component->dev, "Invalid dai type %d\n",dai_drv->dobj.type);
- if (dai_drv->dobj.type &&
dai_drv->dobj.type != SND_SOC_DOBJ_PCM) {dev_err(dev, "Invalid dai type %d\n", dai_drv->dobj.type);this block of code is removed in patch 20, so do we need the modification here?
Yes, it will be removed 20, but it is still exist until then. Because of this patch 19, snd_soc_register_dai() is called from soc-core which doesn't have dobj.type. Without this modification, *all* sound will get error, unfortunately.
Or apply 20 first, 19 next can be more simple patch.
Thank you for your help !! Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc-core has some API which is used from topology, but it is doing topology specific operation at soc-core. soc-core should care about core things, and topology should care about topology things, otherwise, it is very confusable.
For example topology type is not related to soc-core, it is topology side issue.
This patch removes meaningless check from soc-core, and moves topology specific operation to soc-topology.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 32 -------------------------------- sound/soc/soc-topology.c | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 34 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index bbcaac5..7904e839 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1439,20 +1439,7 @@ int snd_soc_add_dai_link(struct snd_soc_card *card, { int ret;
- if (dai_link->dobj.type - && dai_link->dobj.type != SND_SOC_DOBJ_DAI_LINK) { - dev_err(card->dev, "Invalid dai link type %d\n", - dai_link->dobj.type); - return -EINVAL; - } - lockdep_assert_held(&client_mutex); - /* - * Notify the machine driver for extra initialization - * on the link created by topology. - */ - if (dai_link->dobj.type && card->add_dai_link) - card->add_dai_link(card, dai_link);
ret = soc_bind_dai_link(card, dai_link); if (ret < 0) @@ -1478,20 +1465,7 @@ EXPORT_SYMBOL_GPL(snd_soc_add_dai_link); void snd_soc_remove_dai_link(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link) { - if (dai_link->dobj.type - && dai_link->dobj.type != SND_SOC_DOBJ_DAI_LINK) { - dev_err(card->dev, "Invalid dai link type %d\n", - dai_link->dobj.type); - return; - } - lockdep_assert_held(&client_mutex); - /* - * Notify the machine driver for extra destruction - * on the link created by topology. - */ - if (dai_link->dobj.type && card->remove_dai_link) - card->remove_dai_link(card, dai_link);
list_del(&dai_link->list);
@@ -2613,12 +2587,6 @@ struct snd_soc_dai *snd_soc_register_dai(struct snd_soc_component *component, struct device *dev = component->dev; struct snd_soc_dai *dai;
- if (dai_drv->dobj.type && - dai_drv->dobj.type != SND_SOC_DOBJ_PCM) { - dev_err(dev, "Invalid dai type %d\n", dai_drv->dobj.type); - return NULL; - } - dev_dbg(dev, "ASoC: dai register %s\n", dai_drv->name);
lockdep_assert_held(&client_mutex); diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 81d2af0..92ada84 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -541,6 +541,7 @@ static void remove_link(struct snd_soc_component *comp, { struct snd_soc_dai_link *link = container_of(dobj, struct snd_soc_dai_link, dobj); + struct snd_soc_card *card = comp->card;
if (pass != SOC_TPLG_PASS_PCM_DAI) return; @@ -553,7 +554,12 @@ static void remove_link(struct snd_soc_component *comp, kfree(link->cpus->dai_name);
list_del(&dobj->list); - snd_soc_remove_dai_link(comp->card, link); + + /* Notify the machine driver */ + if (card->remove_dai_link) + card->remove_dai_link(card, link); + + snd_soc_remove_dai_link(card, link); kfree(link); }
@@ -1889,6 +1895,7 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, { struct snd_soc_dai_link *link; struct snd_soc_dai_link_component *dlc; + struct snd_soc_card *card = tplg->comp->card; int ret;
/* link + cpu + codec + platform */ @@ -1945,7 +1952,11 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, link->dobj.type = SND_SOC_DOBJ_DAI_LINK; list_add(&link->dobj.list, &tplg->comp->dobj_list);
- snd_soc_add_dai_link(tplg->comp->card, link); + /* Notify the machine driver */ + if (card->add_dai_link) + card->add_dai_link(card, link); + + snd_soc_add_dai_link(card, link); return 0; }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_dobj is used only when SND_SOC_TOPOLOGY was selected. Let's enable it under SND_SOC_TOPOLOGY.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 4e38ee6..e0855dc 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -847,7 +847,9 @@ struct snd_soc_dai_link { unsigned int ignore:1;
struct list_head list; /* DAI link list of the soc card */ +#ifdef CONFIG_SND_SOC_TOPOLOGY struct snd_soc_dobj dobj; /* For topology */ +#endif }; #define for_each_link_codecs(link, i, codec) \ for ((i) = 0; \ @@ -1169,7 +1171,9 @@ struct soc_mixer_control { unsigned int sign_bit; unsigned int invert:1; unsigned int autodisable:1; +#ifdef CONFIG_SND_SOC_TOPOLOGY struct snd_soc_dobj dobj; +#endif };
struct soc_bytes { @@ -1180,8 +1184,9 @@ struct soc_bytes {
struct soc_bytes_ext { int max; +#ifdef CONFIG_SND_SOC_TOPOLOGY struct snd_soc_dobj dobj; - +#endif /* used for TLV byte control */ int (*get)(struct snd_kcontrol *kcontrol, unsigned int __user *bytes, unsigned int size); @@ -1205,7 +1210,9 @@ struct soc_enum { const char * const *texts; const unsigned int *values; unsigned int autodisable:1; +#ifdef CONFIG_SND_SOC_TOPOLOGY struct snd_soc_dobj dobj; +#endif };
/* device driver data */
On 10/8/19 11:29 PM, Kuninori Morimoto wrote:
Hi Mark Cc Pierre-Louis
These are step4 of soc-core cleanup. These related to soc-topology. If my understanding and my code are correct, there is no effect to topology side, but I can't test.
I'm not sure who can test these, but I guess Pierre-Louis knows ? Pierre-Louis, can you help it ?
we can use GitHub and create a pull request with these patches, that will be tested on a variety of platforms. We unfortunately have a random IPC error at the moment which sometimes makes it difficult to judge if errors reported are due to the patch being tested or just bad timing.
I tried to apply the series but I get errors on the 2nd patch. I first need to rebase on Mark's tree.
Kuninori Morimoto (21): ASoC: soc-core: remove for_each_rtdcom_safe() ASoC: soc-core: add for_each_rtd_components() and replace ASoC: soc-core: move soc_init_dai_link() ASoC: soc-core: rename soc_init_dai_link() to soc_dai_link_sanity_check() ASoC: soc-core: remove duplicated soc_is_dai_link_bound() ASoC: soc-core: call soc_bind_dai_link() under snd_soc_add_dai_link() ASoC: soc-core: add soc_unbind_dai_link() ASoC: soc-core: snd_soc_unbind_card() cleanup ASoC: soc-core: remove unneeded snd_soc_tplg_component_remove() ASoC: soc-core: move snd_soc_lookup_component() ASoC: soc-core: add snd_soc_del_component() ASoC: soc-core: use snd_soc_lookup_component() at snd_soc_unregister_component() ASoC: soc-core: move snd_soc_register_dai() ASoC: soc-core: have legacy_dai_naming at snd_soc_register_dai() ASoC: soc-core: move snd_soc_unregister_dais() ASoC: soc-core: add snd_soc_unregister_dai() ASoC: soc-core: don't call snd_soc_dapm_new_dai_widgets() at snd_soc_register_dai() ASoC: soc-core: use mutex_lock() at snd_soc_add_component() ASoC: soc-core: call snd_soc_register_dai() from snd_soc_register_dais() ASoC: soc-core: remove topology specific operation ASoC: soc.h: dobj is used only when SND_SOC_TOPOLOGY
include/sound/soc.h | 25 ++- sound/soc/soc-component.c | 43 +--- sound/soc/soc-compress.c | 52 ++--- sound/soc/soc-core.c | 542 ++++++++++++++++++++-------------------------- sound/soc/soc-pcm.c | 49 ++--- sound/soc/soc-topology.c | 32 ++- 6 files changed, 320 insertions(+), 423 deletions(-)
On 10/8/19 11:29 PM, Kuninori Morimoto wrote:
Hi Mark Cc Pierre-Louis
These are step4 of soc-core cleanup. These related to soc-topology. If my understanding and my code are correct, there is no effect to topology side, but I can't test.
I'm not sure who can test these, but I guess Pierre-Louis knows ? Pierre-Louis, can you help it ?
This patchset was tested with the Intel SOF CI, and no regressions were seen.
Full results here: https://github.com/thesofproject/linux/pull/1298
I added a couple of comments to help improve commit messages or code. Nothing major, more about fixing inconsistencies than about broken code.
I could however not figure out if Patch 6 was correct or not, this is by far the most complicated in this series and additional folks more familiar with the code should look into it.
Kuninori Morimoto (21): ASoC: soc-core: remove for_each_rtdcom_safe() ASoC: soc-core: add for_each_rtd_components() and replace ASoC: soc-core: move soc_init_dai_link() ASoC: soc-core: rename soc_init_dai_link() to soc_dai_link_sanity_check() ASoC: soc-core: remove duplicated soc_is_dai_link_bound() ASoC: soc-core: call soc_bind_dai_link() under snd_soc_add_dai_link() ASoC: soc-core: add soc_unbind_dai_link() ASoC: soc-core: snd_soc_unbind_card() cleanup ASoC: soc-core: remove unneeded snd_soc_tplg_component_remove() ASoC: soc-core: move snd_soc_lookup_component() ASoC: soc-core: add snd_soc_del_component() ASoC: soc-core: use snd_soc_lookup_component() at snd_soc_unregister_component() ASoC: soc-core: move snd_soc_register_dai() ASoC: soc-core: have legacy_dai_naming at snd_soc_register_dai() ASoC: soc-core: move snd_soc_unregister_dais() ASoC: soc-core: add snd_soc_unregister_dai() ASoC: soc-core: don't call snd_soc_dapm_new_dai_widgets() at snd_soc_register_dai() ASoC: soc-core: use mutex_lock() at snd_soc_add_component() ASoC: soc-core: call snd_soc_register_dai() from snd_soc_register_dais() ASoC: soc-core: remove topology specific operation ASoC: soc.h: dobj is used only when SND_SOC_TOPOLOGY
include/sound/soc.h | 25 ++- sound/soc/soc-component.c | 43 +--- sound/soc/soc-compress.c | 52 ++--- sound/soc/soc-core.c | 542 ++++++++++++++++++++-------------------------- sound/soc/soc-pcm.c | 49 ++--- sound/soc/soc-topology.c | 32 ++- 6 files changed, 320 insertions(+), 423 deletions(-)
Hi Pierre-Louis
Thank you for your review.
I added a couple of comments to help improve commit messages or code. Nothing major, more about fixing inconsistencies than about broken code.
Thanks !! I will fixup these and post v2
I could however not figure out if Patch 6 was correct or not, this is by far the most complicated in this series and additional folks more familiar with the code should look into it.
It is the main patch of this patchset. If it was not correct, topology can't probe, I guess. But yes, more review is very welcome.
Thank you for your help !! Best regards --- Kuninori Morimoto
participants (2)
-
Kuninori Morimoto -
Pierre-Louis Bossart