alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ASoC: Make aux_dev more like a generic component
@ 2016-01-06  5:29 mengdong.lin
  2016-01-10 12:50 ` Applied "ASoC: Make aux_dev more like a generic component" to the asoc tree Mark Brown
  0 siblings, 1 reply; 3+ messages in thread
From: mengdong.lin @ 2016-01-06  5:29 UTC (permalink / raw)
  To: alsa-devel, broonie
  Cc: Mengdong Lin, vinod.koul, mengdong.lin, liam.r.girdwood, jeeja.kp,
	subhransu.s.prusty

From: Mengdong Lin <mengdong.lin@linux.intel.com>

aux_dev is mainly used by the machine driver to specify analog devices,
which are registered as codecs. Making it more like a generic component
can help the machine driver to use it to specify any component with
topology info by name.

Details:
- Remove the stub 'rtd_aux' array from the soc card.
- Add a list 'aux_comp_list' to store the components of aux_devs.
  And add a list head 'list_aux' to struct snd_soc_component, for adding
  such components to the above list.
- Add a 'init' ops to a component for machine specific init.
  soc_bind_aux_dev() will set it to be aux_dev's init. And it will be
  called when probing the component.
- soc_bind_aux_dev() will also search components by name of an aux_dev,
  since it may not be a codec.
- Move probing of aux_devs before checking new DAI links brought by
  topology.
- Move removal of aux_devs later than removal of links. Because topology
  of aux components may register DAIs and the DAI drivers will go with
  removal of the aux components, we want soc_remove_link_dais() to remove
  the DAIs at first.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 49011c3..c03e07a 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -798,6 +798,7 @@ struct snd_soc_component {
 	unsigned int registered_as_component:1;
 
 	struct list_head list;
+	struct list_head list_aux; /* for auxiliary component of the card */
 
 	struct snd_soc_dai_driver *dai_drv;
 	int num_dai;
@@ -841,6 +842,9 @@ struct snd_soc_component {
 	int (*probe)(struct snd_soc_component *);
 	void (*remove)(struct snd_soc_component *);
 
+	/* machine specific init */
+	int (*init)(struct snd_soc_component *component);
+
 #ifdef CONFIG_DEBUG_FS
 	void (*init_debugfs)(struct snd_soc_component *component);
 	const char *debugfs_prefix;
@@ -1144,8 +1148,7 @@ struct snd_soc_card {
 	 */
 	struct snd_soc_aux_dev *aux_dev;
 	int num_aux_devs;
-	struct snd_soc_pcm_runtime *rtd_aux;
-	int num_aux_rtd;
+	struct list_head aux_comp_list;
 
 	const struct snd_kcontrol_new *controls;
 	int num_controls;
@@ -1553,6 +1556,7 @@ static inline void snd_soc_initialize_card_lists(struct snd_soc_card *card)
 	INIT_LIST_HEAD(&card->widgets);
 	INIT_LIST_HEAD(&card->paths);
 	INIT_LIST_HEAD(&card->dapm_list);
+	INIT_LIST_HEAD(&card->aux_comp_list);
 }
 
 static inline bool snd_soc_volsw_is_stereo(struct soc_mixer_control *mc)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4e03e82..790ee2b 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1413,6 +1413,16 @@ static int soc_probe_component(struct snd_soc_card *card,
 			component->name);
 	}
 
+	/* machine specific init */
+	if (component->init) {
+		ret = component->init(component);
+		if (ret < 0) {
+			dev_err(component->dev,
+				"Failed to do machine specific init %d\n", ret);
+			goto err_probe;
+		}
+	}
+
 	if (component->controls)
 		snd_soc_add_component_controls(component, component->controls,
 				     component->num_controls);
@@ -1657,65 +1667,81 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 
 static int soc_bind_aux_dev(struct snd_soc_card *card, int num)
 {
-	struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
 	struct snd_soc_aux_dev *aux_dev = &card->aux_dev[num];
-	const char *name = aux_dev->codec_name;
-
-	rtd->component = soc_find_component(aux_dev->codec_of_node, name);
-	if (!rtd->component) {
-		if (aux_dev->codec_of_node)
-			name = of_node_full_name(aux_dev->codec_of_node);
-
-		dev_err(card->dev, "ASoC: %s not registered\n", name);
-		return -EPROBE_DEFER;
+	struct snd_soc_component *component;
+	const char *name;
+	struct device_node *codec_of_node;
+
+	if (aux_dev->codec_of_node || aux_dev->codec_name) {
+		/* codecs, usually analog devices */
+		name = aux_dev->codec_name;
+		codec_of_node = aux_dev->codec_of_node;
+		component = soc_find_component(codec_of_node, name);
+		if (!component) {
+			if (codec_of_node)
+				name = of_node_full_name(codec_of_node);
+			goto err_defer;
+		}
+	} else if (aux_dev->name) {
+		/* generic components */
+		name = aux_dev->name;
+		component = soc_find_component(NULL, name);
+		if (!component)
+			goto err_defer;
+	} else {
+		dev_err(card->dev, "ASoC: Invalid auxiliary device\n");
+		return -EINVAL;
 	}
 
-	/*
-	 * Some places still reference rtd->codec, so we have to keep that
-	 * initialized if the component is a CODEC. Once all those references
-	 * have been removed, this code can be removed as well.
-	 */
-	 rtd->codec = rtd->component->codec;
-
+	component->init = aux_dev->init;
+	list_add(&component->list_aux, &card->aux_comp_list);
 	return 0;
+
+err_defer:
+	dev_err(card->dev, "ASoC: %s not registered\n", name);
+	return -EPROBE_DEFER;
 }
 
-static int soc_probe_aux_dev(struct snd_soc_card *card, int num)
+static int soc_probe_aux_devices(struct snd_soc_card *card)
 {
-	struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
-	struct snd_soc_aux_dev *aux_dev = &card->aux_dev[num];
+	struct snd_soc_component *comp;
+	int order;
 	int ret;
 
-	ret = soc_probe_component(card, rtd->component);
-	if (ret < 0)
-		return ret;
-
-	/* do machine specific initialization */
-	if (aux_dev->init) {
-		ret = aux_dev->init(rtd->component);
-		if (ret < 0) {
-			dev_err(card->dev, "ASoC: failed to init %s: %d\n",
-				aux_dev->name, ret);
-			return ret;
+	for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST;
+		order++) {
+		list_for_each_entry(comp, &card->aux_comp_list, list_aux) {
+			if (comp->driver->probe_order == order) {
+				ret = soc_probe_component(card,	comp);
+				if (ret < 0) {
+					dev_err(card->dev,
+						"ASoC: failed to probe aux component %s %d\n",
+						comp->name, ret);
+					return ret;
+				}
+			}
 		}
 	}
 
-	return soc_post_component_init(rtd, aux_dev->name);
+	return 0;
 }
 
-static void soc_remove_aux_dev(struct snd_soc_card *card, int num)
+static void soc_remove_aux_devices(struct snd_soc_card *card)
 {
-	struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
-	struct snd_soc_component *component = rtd->component;
+	struct snd_soc_component *comp, *_comp;
+	int order;
 
-	/* unregister the rtd device */
-	if (rtd->dev_registered) {
-		device_unregister(rtd->dev);
-		rtd->dev_registered = 0;
+	for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST;
+		order++) {
+		list_for_each_entry_safe(comp, _comp,
+			&card->aux_comp_list, list_aux) {
+			if (comp->driver->remove_order == order) {
+				soc_remove_component(comp);
+				/* remove it from the card's aux_comp_list */
+				list_del(&comp->list_aux);
+			}
+		}
 	}
-
-	if (component)
-		soc_remove_component(component);
 }
 
 static int snd_soc_init_codec_cache(struct snd_soc_codec *codec)
@@ -1894,6 +1920,11 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 		}
 	}
 
+	/* probe auxiliary components */
+	ret = soc_probe_aux_devices(card);
+	if (ret < 0)
+		goto probe_dai_err;
+
 	/* Find new DAI links added during probing components and bind them.
 	 * Components with topology may bring new DAIs and DAI links.
 	 */
@@ -1923,16 +1954,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 		}
 	}
 
-	for (i = 0; i < card->num_aux_devs; i++) {
-		ret = soc_probe_aux_dev(card, i);
-		if (ret < 0) {
-			dev_err(card->dev,
-				"ASoC: failed to add auxiliary devices %d\n",
-				ret);
-			goto probe_aux_dev_err;
-		}
-	}
-
 	snd_soc_dapm_link_dai_widgets(card);
 	snd_soc_dapm_connect_dai_link_widgets(card);
 
@@ -1992,8 +2013,7 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 	return 0;
 
 probe_aux_dev_err:
-	for (i = 0; i < card->num_aux_devs; i++)
-		soc_remove_aux_dev(card, i);
+	soc_remove_aux_devices(card);
 
 probe_dai_err:
 	soc_remove_dai_links(card);
@@ -2039,20 +2059,18 @@ static int soc_probe(struct platform_device *pdev)
 static int soc_cleanup_card_resources(struct snd_soc_card *card)
 {
 	struct snd_soc_pcm_runtime *rtd;
-	int i;
 
 	/* make sure any delayed work runs */
 	list_for_each_entry(rtd, &card->rtd_list, list)
 		flush_delayed_work(&rtd->delayed_work);
 
-	/* remove auxiliary devices */
-	for (i = 0; i < card->num_aux_devs; i++)
-		soc_remove_aux_dev(card, i);
-
 	/* remove and free each DAI */
 	soc_remove_dai_links(card);
 	soc_remove_pcm_runtimes(card);
 
+	/* remove auxiliary devices */
+	soc_remove_aux_devices(card);
+
 	soc_cleanup_card_debugfs(card);
 
 	/* remove the card */
@@ -2608,16 +2626,6 @@ int snd_soc_register_card(struct snd_soc_card *card)
 	INIT_LIST_HEAD(&card->rtd_list);
 	card->num_rtd = 0;
 
-	card->rtd_aux = devm_kzalloc(card->dev,
-				 sizeof(struct snd_soc_pcm_runtime) *
-				 card->num_aux_devs,
-				 GFP_KERNEL);
-	if (card->rtd_aux == NULL)
-		return -ENOMEM;
-
-	for (i = 0; i < card->num_aux_devs; i++)
-		card->rtd_aux[i].card = card;
-
 	INIT_LIST_HEAD(&card->dapm_dirty);
 	INIT_LIST_HEAD(&card->dobj_list);
 	card->instantiated = 0;
-- 
2.5.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Applied "ASoC: Make aux_dev more like a generic component" to the asoc tree
  2016-01-06  5:29 [PATCH] ASoC: Make aux_dev more like a generic component mengdong.lin
@ 2016-01-10 12:50 ` Mark Brown
  2016-04-25 14:43   ` Enric Balletbo Serra
  0 siblings, 1 reply; 3+ messages in thread
From: Mark Brown @ 2016-01-10 12:50 UTC (permalink / raw)
  To: Mengdong Lin, Mark Brown; +Cc: alsa-devel

The patch

   ASoC: Make aux_dev more like a generic component

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From f2ed6b07645ed29c1e090ead2e41066385cba3ea Mon Sep 17 00:00:00 2001
From: Mengdong Lin <mengdong.lin@linux.intel.com>
Date: Wed, 6 Jan 2016 13:29:31 +0800
Subject: [PATCH] ASoC: Make aux_dev more like a generic component

aux_dev is mainly used by the machine driver to specify analog devices,
which are registered as codecs. Making it more like a generic component
can help the machine driver to use it to specify any component with
topology info by name.

Details:
- Remove the stub 'rtd_aux' array from the soc card.
- Add a list 'aux_comp_list' to store the components of aux_devs.
  And add a list head 'list_aux' to struct snd_soc_component, for adding
  such components to the above list.
- Add a 'init' ops to a component for machine specific init.
  soc_bind_aux_dev() will set it to be aux_dev's init. And it will be
  called when probing the component.
- soc_bind_aux_dev() will also search components by name of an aux_dev,
  since it may not be a codec.
- Move probing of aux_devs before checking new DAI links brought by
  topology.
- Move removal of aux_devs later than removal of links. Because topology
  of aux components may register DAIs and the DAI drivers will go with
  removal of the aux components, we want soc_remove_link_dais() to remove
  the DAIs at first.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/sound/soc.h  |   8 ++-
 sound/soc/soc-core.c | 146 +++++++++++++++++++++++++++------------------------
 2 files changed, 83 insertions(+), 71 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 9d1383e8d039..5bc5def6af02 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -787,6 +787,7 @@ struct snd_soc_component {
 	unsigned int registered_as_component:1;
 
 	struct list_head list;
+	struct list_head list_aux; /* for auxiliary component of the card */
 
 	struct snd_soc_dai_driver *dai_drv;
 	int num_dai;
@@ -830,6 +831,9 @@ struct snd_soc_component {
 	int (*probe)(struct snd_soc_component *);
 	void (*remove)(struct snd_soc_component *);
 
+	/* machine specific init */
+	int (*init)(struct snd_soc_component *component);
+
 #ifdef CONFIG_DEBUG_FS
 	void (*init_debugfs)(struct snd_soc_component *component);
 	const char *debugfs_prefix;
@@ -1130,8 +1134,7 @@ struct snd_soc_card {
 	 */
 	struct snd_soc_aux_dev *aux_dev;
 	int num_aux_devs;
-	struct snd_soc_pcm_runtime *rtd_aux;
-	int num_aux_rtd;
+	struct list_head aux_comp_list;
 
 	const struct snd_kcontrol_new *controls;
 	int num_controls;
@@ -1537,6 +1540,7 @@ static inline void snd_soc_initialize_card_lists(struct snd_soc_card *card)
 	INIT_LIST_HEAD(&card->widgets);
 	INIT_LIST_HEAD(&card->paths);
 	INIT_LIST_HEAD(&card->dapm_list);
+	INIT_LIST_HEAD(&card->aux_comp_list);
 }
 
 static inline bool snd_soc_volsw_is_stereo(struct soc_mixer_control *mc)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index c572673a5a24..c10bd668659c 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1413,6 +1413,16 @@ static int soc_probe_component(struct snd_soc_card *card,
 			component->name);
 	}
 
+	/* machine specific init */
+	if (component->init) {
+		ret = component->init(component);
+		if (ret < 0) {
+			dev_err(component->dev,
+				"Failed to do machine specific init %d\n", ret);
+			goto err_probe;
+		}
+	}
+
 	if (component->controls)
 		snd_soc_add_component_controls(component, component->controls,
 				     component->num_controls);
@@ -1657,65 +1667,81 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
 
 static int soc_bind_aux_dev(struct snd_soc_card *card, int num)
 {
-	struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
 	struct snd_soc_aux_dev *aux_dev = &card->aux_dev[num];
-	const char *name = aux_dev->codec_name;
-
-	rtd->component = soc_find_component(aux_dev->codec_of_node, name);
-	if (!rtd->component) {
-		if (aux_dev->codec_of_node)
-			name = of_node_full_name(aux_dev->codec_of_node);
-
-		dev_err(card->dev, "ASoC: %s not registered\n", name);
-		return -EPROBE_DEFER;
+	struct snd_soc_component *component;
+	const char *name;
+	struct device_node *codec_of_node;
+
+	if (aux_dev->codec_of_node || aux_dev->codec_name) {
+		/* codecs, usually analog devices */
+		name = aux_dev->codec_name;
+		codec_of_node = aux_dev->codec_of_node;
+		component = soc_find_component(codec_of_node, name);
+		if (!component) {
+			if (codec_of_node)
+				name = of_node_full_name(codec_of_node);
+			goto err_defer;
+		}
+	} else if (aux_dev->name) {
+		/* generic components */
+		name = aux_dev->name;
+		component = soc_find_component(NULL, name);
+		if (!component)
+			goto err_defer;
+	} else {
+		dev_err(card->dev, "ASoC: Invalid auxiliary device\n");
+		return -EINVAL;
 	}
 
-	/*
-	 * Some places still reference rtd->codec, so we have to keep that
-	 * initialized if the component is a CODEC. Once all those references
-	 * have been removed, this code can be removed as well.
-	 */
-	 rtd->codec = rtd->component->codec;
-
+	component->init = aux_dev->init;
+	list_add(&component->list_aux, &card->aux_comp_list);
 	return 0;
+
+err_defer:
+	dev_err(card->dev, "ASoC: %s not registered\n", name);
+	return -EPROBE_DEFER;
 }
 
-static int soc_probe_aux_dev(struct snd_soc_card *card, int num)
+static int soc_probe_aux_devices(struct snd_soc_card *card)
 {
-	struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
-	struct snd_soc_aux_dev *aux_dev = &card->aux_dev[num];
+	struct snd_soc_component *comp;
+	int order;
 	int ret;
 
-	ret = soc_probe_component(card, rtd->component);
-	if (ret < 0)
-		return ret;
-
-	/* do machine specific initialization */
-	if (aux_dev->init) {
-		ret = aux_dev->init(rtd->component);
-		if (ret < 0) {
-			dev_err(card->dev, "ASoC: failed to init %s: %d\n",
-				aux_dev->name, ret);
-			return ret;
+	for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST;
+		order++) {
+		list_for_each_entry(comp, &card->aux_comp_list, list_aux) {
+			if (comp->driver->probe_order == order) {
+				ret = soc_probe_component(card,	comp);
+				if (ret < 0) {
+					dev_err(card->dev,
+						"ASoC: failed to probe aux component %s %d\n",
+						comp->name, ret);
+					return ret;
+				}
+			}
 		}
 	}
 
-	return soc_post_component_init(rtd, aux_dev->name);
+	return 0;
 }
 
-static void soc_remove_aux_dev(struct snd_soc_card *card, int num)
+static void soc_remove_aux_devices(struct snd_soc_card *card)
 {
-	struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
-	struct snd_soc_component *component = rtd->component;
+	struct snd_soc_component *comp, *_comp;
+	int order;
 
-	/* unregister the rtd device */
-	if (rtd->dev_registered) {
-		device_unregister(rtd->dev);
-		rtd->dev_registered = 0;
+	for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST;
+		order++) {
+		list_for_each_entry_safe(comp, _comp,
+			&card->aux_comp_list, list_aux) {
+			if (comp->driver->remove_order == order) {
+				soc_remove_component(comp);
+				/* remove it from the card's aux_comp_list */
+				list_del(&comp->list_aux);
+			}
+		}
 	}
-
-	if (component)
-		soc_remove_component(component);
 }
 
 static int snd_soc_init_codec_cache(struct snd_soc_codec *codec)
@@ -1894,6 +1920,11 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 		}
 	}
 
+	/* probe auxiliary components */
+	ret = soc_probe_aux_devices(card);
+	if (ret < 0)
+		goto probe_dai_err;
+
 	/* Find new DAI links added during probing components and bind them.
 	 * Components with topology may bring new DAIs and DAI links.
 	 */
@@ -1923,16 +1954,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 		}
 	}
 
-	for (i = 0; i < card->num_aux_devs; i++) {
-		ret = soc_probe_aux_dev(card, i);
-		if (ret < 0) {
-			dev_err(card->dev,
-				"ASoC: failed to add auxiliary devices %d\n",
-				ret);
-			goto probe_aux_dev_err;
-		}
-	}
-
 	snd_soc_dapm_link_dai_widgets(card);
 	snd_soc_dapm_connect_dai_link_widgets(card);
 
@@ -1992,8 +2013,7 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
 	return 0;
 
 probe_aux_dev_err:
-	for (i = 0; i < card->num_aux_devs; i++)
-		soc_remove_aux_dev(card, i);
+	soc_remove_aux_devices(card);
 
 probe_dai_err:
 	soc_remove_dai_links(card);
@@ -2039,20 +2059,18 @@ static int soc_probe(struct platform_device *pdev)
 static int soc_cleanup_card_resources(struct snd_soc_card *card)
 {
 	struct snd_soc_pcm_runtime *rtd;
-	int i;
 
 	/* make sure any delayed work runs */
 	list_for_each_entry(rtd, &card->rtd_list, list)
 		flush_delayed_work(&rtd->delayed_work);
 
-	/* remove auxiliary devices */
-	for (i = 0; i < card->num_aux_devs; i++)
-		soc_remove_aux_dev(card, i);
-
 	/* remove and free each DAI */
 	soc_remove_dai_links(card);
 	soc_remove_pcm_runtimes(card);
 
+	/* remove auxiliary devices */
+	soc_remove_aux_devices(card);
+
 	soc_cleanup_card_debugfs(card);
 
 	/* remove the card */
@@ -2608,16 +2626,6 @@ int snd_soc_register_card(struct snd_soc_card *card)
 	INIT_LIST_HEAD(&card->rtd_list);
 	card->num_rtd = 0;
 
-	card->rtd_aux = devm_kzalloc(card->dev,
-				 sizeof(struct snd_soc_pcm_runtime) *
-				 card->num_aux_devs,
-				 GFP_KERNEL);
-	if (card->rtd_aux == NULL)
-		return -ENOMEM;
-
-	for (i = 0; i < card->num_aux_devs; i++)
-		card->rtd_aux[i].card = card;
-
 	INIT_LIST_HEAD(&card->dapm_dirty);
 	INIT_LIST_HEAD(&card->dobj_list);
 	card->instantiated = 0;
-- 
2.7.0.rc3

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: Applied "ASoC: Make aux_dev more like a generic component" to the asoc tree
  2016-01-10 12:50 ` Applied "ASoC: Make aux_dev more like a generic component" to the asoc tree Mark Brown
@ 2016-04-25 14:43   ` Enric Balletbo Serra
  0 siblings, 0 replies; 3+ messages in thread
From: Enric Balletbo Serra @ 2016-04-25 14:43 UTC (permalink / raw)
  To: Mark Brown; +Cc: alsa-devel, Mengdong Lin

Hi,

2016-01-10 13:50 GMT+01:00 Mark Brown <broonie@kernel.org>:
> The patch
>
>    ASoC: Make aux_dev more like a generic component
>
> has been applied to the asoc tree at
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
>
> All being well this means that it will be integrated into the linux-next
> tree (usually sometime in the next 24 hours) and sent to Linus during
> the next merge window (or sooner if it is a bug fix), however if
> problems are discovered then the patch may be dropped or reverted.
>
> You may get further e-mails resulting from automated or manual testing
> and review of the tree, please engage with people reporting problems and
> send followup patches addressing any issues that are reported if needed.
>
> If any updates are required or you are submitting further changes they
> should be sent as incremental updates against current git, existing
> patches will not be replaced.
>
> Please add any relevant lists and maintainers to the CCs when replying
> to this mail.
>
> Thanks,
> Mark
>
> From f2ed6b07645ed29c1e090ead2e41066385cba3ea Mon Sep 17 00:00:00 2001
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
> Date: Wed, 6 Jan 2016 13:29:31 +0800
> Subject: [PATCH] ASoC: Make aux_dev more like a generic component
>
> aux_dev is mainly used by the machine driver to specify analog devices,
> which are registered as codecs. Making it more like a generic component
> can help the machine driver to use it to specify any component with
> topology info by name.
>
> Details:
> - Remove the stub 'rtd_aux' array from the soc card.
> - Add a list 'aux_comp_list' to store the components of aux_devs.
>   And add a list head 'list_aux' to struct snd_soc_component, for adding
>   such components to the above list.
> - Add a 'init' ops to a component for machine specific init.
>   soc_bind_aux_dev() will set it to be aux_dev's init. And it will be
>   called when probing the component.
> - soc_bind_aux_dev() will also search components by name of an aux_dev,
>   since it may not be a codec.
> - Move probing of aux_devs before checking new DAI links brought by
>   topology.
> - Move removal of aux_devs later than removal of links. Because topology
>   of aux components may register DAIs and the DAI drivers will go with
>   removal of the aux components, we want soc_remove_link_dais() to remove
>   the DAIs at first.
>
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  include/sound/soc.h  |   8 ++-
>  sound/soc/soc-core.c | 146 +++++++++++++++++++++++++++------------------------
>  2 files changed, 83 insertions(+), 71 deletions(-)
>
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 9d1383e8d039..5bc5def6af02 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -787,6 +787,7 @@ struct snd_soc_component {
>         unsigned int registered_as_component:1;
>
>         struct list_head list;
> +       struct list_head list_aux; /* for auxiliary component of the card */
>
>         struct snd_soc_dai_driver *dai_drv;
>         int num_dai;
> @@ -830,6 +831,9 @@ struct snd_soc_component {
>         int (*probe)(struct snd_soc_component *);
>         void (*remove)(struct snd_soc_component *);
>
> +       /* machine specific init */
> +       int (*init)(struct snd_soc_component *component);
> +
>  #ifdef CONFIG_DEBUG_FS
>         void (*init_debugfs)(struct snd_soc_component *component);
>         const char *debugfs_prefix;
> @@ -1130,8 +1134,7 @@ struct snd_soc_card {
>          */
>         struct snd_soc_aux_dev *aux_dev;
>         int num_aux_devs;
> -       struct snd_soc_pcm_runtime *rtd_aux;
> -       int num_aux_rtd;
> +       struct list_head aux_comp_list;
>
>         const struct snd_kcontrol_new *controls;
>         int num_controls;
> @@ -1537,6 +1540,7 @@ static inline void snd_soc_initialize_card_lists(struct snd_soc_card *card)
>         INIT_LIST_HEAD(&card->widgets);
>         INIT_LIST_HEAD(&card->paths);
>         INIT_LIST_HEAD(&card->dapm_list);
> +       INIT_LIST_HEAD(&card->aux_comp_list);
>  }
>
>  static inline bool snd_soc_volsw_is_stereo(struct soc_mixer_control *mc)
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index c572673a5a24..c10bd668659c 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1413,6 +1413,16 @@ static int soc_probe_component(struct snd_soc_card *card,
>                         component->name);
>         }
>
> +       /* machine specific init */
> +       if (component->init) {
> +               ret = component->init(component);
> +               if (ret < 0) {
> +                       dev_err(component->dev,
> +                               "Failed to do machine specific init %d\n", ret);
> +                       goto err_probe;
> +               }
> +       }
> +
>         if (component->controls)
>                 snd_soc_add_component_controls(component, component->controls,
>                                      component->num_controls);
> @@ -1657,65 +1667,81 @@ static int soc_probe_link_dais(struct snd_soc_card *card,
>
>  static int soc_bind_aux_dev(struct snd_soc_card *card, int num)
>  {
> -       struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
>         struct snd_soc_aux_dev *aux_dev = &card->aux_dev[num];
> -       const char *name = aux_dev->codec_name;
> -
> -       rtd->component = soc_find_component(aux_dev->codec_of_node, name);
> -       if (!rtd->component) {
> -               if (aux_dev->codec_of_node)
> -                       name = of_node_full_name(aux_dev->codec_of_node);
> -
> -               dev_err(card->dev, "ASoC: %s not registered\n", name);
> -               return -EPROBE_DEFER;
> +       struct snd_soc_component *component;
> +       const char *name;
> +       struct device_node *codec_of_node;
> +
> +       if (aux_dev->codec_of_node || aux_dev->codec_name) {
> +               /* codecs, usually analog devices */
> +               name = aux_dev->codec_name;
> +               codec_of_node = aux_dev->codec_of_node;
> +               component = soc_find_component(codec_of_node, name);
> +               if (!component) {
> +                       if (codec_of_node)
> +                               name = of_node_full_name(codec_of_node);
> +                       goto err_defer;
> +               }
> +       } else if (aux_dev->name) {
> +               /* generic components */
> +               name = aux_dev->name;
> +               component = soc_find_component(NULL, name);
> +               if (!component)
> +                       goto err_defer;
> +       } else {
> +               dev_err(card->dev, "ASoC: Invalid auxiliary device\n");
> +               return -EINVAL;
>         }
>
> -       /*
> -        * Some places still reference rtd->codec, so we have to keep that
> -        * initialized if the component is a CODEC. Once all those references
> -        * have been removed, this code can be removed as well.
> -        */
> -        rtd->codec = rtd->component->codec;
> -
> +       component->init = aux_dev->init;
> +       list_add(&component->list_aux, &card->aux_comp_list);
>         return 0;
> +
> +err_defer:
> +       dev_err(card->dev, "ASoC: %s not registered\n", name);
> +       return -EPROBE_DEFER;
>  }
>
> -static int soc_probe_aux_dev(struct snd_soc_card *card, int num)
> +static int soc_probe_aux_devices(struct snd_soc_card *card)
>  {
> -       struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
> -       struct snd_soc_aux_dev *aux_dev = &card->aux_dev[num];
> +       struct snd_soc_component *comp;
> +       int order;
>         int ret;
>
> -       ret = soc_probe_component(card, rtd->component);
> -       if (ret < 0)
> -               return ret;
> -
> -       /* do machine specific initialization */
> -       if (aux_dev->init) {
> -               ret = aux_dev->init(rtd->component);
> -               if (ret < 0) {
> -                       dev_err(card->dev, "ASoC: failed to init %s: %d\n",
> -                               aux_dev->name, ret);
> -                       return ret;
> +       for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST;
> +               order++) {
> +               list_for_each_entry(comp, &card->aux_comp_list, list_aux) {
> +                       if (comp->driver->probe_order == order) {
> +                               ret = soc_probe_component(card, comp);
> +                               if (ret < 0) {
> +                                       dev_err(card->dev,
> +                                               "ASoC: failed to probe aux component %s %d\n",
> +                                               comp->name, ret);
> +                                       return ret;
> +                               }
> +                       }
>                 }
>         }
>
> -       return soc_post_component_init(rtd, aux_dev->name);
> +       return 0;
>  }
>
> -static void soc_remove_aux_dev(struct snd_soc_card *card, int num)
> +static void soc_remove_aux_devices(struct snd_soc_card *card)
>  {
> -       struct snd_soc_pcm_runtime *rtd = &card->rtd_aux[num];
> -       struct snd_soc_component *component = rtd->component;
> +       struct snd_soc_component *comp, *_comp;
> +       int order;
>
> -       /* unregister the rtd device */
> -       if (rtd->dev_registered) {
> -               device_unregister(rtd->dev);
> -               rtd->dev_registered = 0;
> +       for (order = SND_SOC_COMP_ORDER_FIRST; order <= SND_SOC_COMP_ORDER_LAST;
> +               order++) {
> +               list_for_each_entry_safe(comp, _comp,
> +                       &card->aux_comp_list, list_aux) {
> +                       if (comp->driver->remove_order == order) {
> +                               soc_remove_component(comp);
> +                               /* remove it from the card's aux_comp_list */
> +                               list_del(&comp->list_aux);
> +                       }
> +               }
>         }
> -
> -       if (component)
> -               soc_remove_component(component);
>  }
>
>  static int snd_soc_init_codec_cache(struct snd_soc_codec *codec)
> @@ -1894,6 +1920,11 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
>                 }
>         }
>
> +       /* probe auxiliary components */
> +       ret = soc_probe_aux_devices(card);
> +       if (ret < 0)
> +               goto probe_dai_err;
> +
>         /* Find new DAI links added during probing components and bind them.
>          * Components with topology may bring new DAIs and DAI links.
>          */
> @@ -1923,16 +1954,6 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
>                 }
>         }
>
> -       for (i = 0; i < card->num_aux_devs; i++) {
> -               ret = soc_probe_aux_dev(card, i);
> -               if (ret < 0) {
> -                       dev_err(card->dev,
> -                               "ASoC: failed to add auxiliary devices %d\n",
> -                               ret);
> -                       goto probe_aux_dev_err;
> -               }
> -       }
> -
>         snd_soc_dapm_link_dai_widgets(card);
>         snd_soc_dapm_connect_dai_link_widgets(card);
>
> @@ -1992,8 +2013,7 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
>         return 0;
>
>  probe_aux_dev_err:
> -       for (i = 0; i < card->num_aux_devs; i++)
> -               soc_remove_aux_dev(card, i);
> +       soc_remove_aux_devices(card);
>
>  probe_dai_err:
>         soc_remove_dai_links(card);
> @@ -2039,20 +2059,18 @@ static int soc_probe(struct platform_device *pdev)
>  static int soc_cleanup_card_resources(struct snd_soc_card *card)
>  {
>         struct snd_soc_pcm_runtime *rtd;
> -       int i;
>
>         /* make sure any delayed work runs */
>         list_for_each_entry(rtd, &card->rtd_list, list)
>                 flush_delayed_work(&rtd->delayed_work);
>
> -       /* remove auxiliary devices */
> -       for (i = 0; i < card->num_aux_devs; i++)
> -               soc_remove_aux_dev(card, i);
> -
>         /* remove and free each DAI */
>         soc_remove_dai_links(card);
>         soc_remove_pcm_runtimes(card);
>
> +       /* remove auxiliary devices */
> +       soc_remove_aux_devices(card);
> +
>         soc_cleanup_card_debugfs(card);
>
>         /* remove the card */
> @@ -2608,16 +2626,6 @@ int snd_soc_register_card(struct snd_soc_card *card)
>         INIT_LIST_HEAD(&card->rtd_list);
>         card->num_rtd = 0;
>
> -       card->rtd_aux = devm_kzalloc(card->dev,
> -                                sizeof(struct snd_soc_pcm_runtime) *
> -                                card->num_aux_devs,
> -                                GFP_KERNEL);
> -       if (card->rtd_aux == NULL)
> -               return -ENOMEM;
> -
> -       for (i = 0; i < card->num_aux_devs; i++)
> -               card->rtd_aux[i].card = card;
> -
>         INIT_LIST_HEAD(&card->dapm_dirty);
>         INIT_LIST_HEAD(&card->dobj_list);
>         card->instantiated = 0;
> --
> 2.7.0.rc3
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

I just found that this change produces a kernel NULL pointer
dereference [1]. Reverting the patch avoids the kernel oops. From a
quick look seems that moving the probing of aux_devs before checking
new DAI links is what causes the problem. I'm not really familiar with
the API so before send a patch I prefer share my thoughts.

The sound/soc/rockchip/rockchip_max98090.c registers a headset chip as
a snd_soc_aux_dev, and the problem is that the init function calls
ts3a227e_enable_jack_detect and snd_jack_set_key before
snd_soc_card_jack_new is called. I think similar problem can happen
with this driver: sound/soc/intel/boards/cht_bsw_max98090_ti.c

Makes sense what I'm saying for you? And is the proper fix delay the
call of ts3a227e_enable_jack_detect to be later?

[1] dmesg.log

[  132.091648] max98090 2-0010: MAX98090 REVID=0x43
[  132.094658] max98090 2-0010: use default 2.8v micbias
[  132.103997] Unable to handle kernel NULL pointer dereference at
virtual address 00000014
[  132.104396] pgd = eba34000
[  132.104552] [00000014] *pgd=00000000
[  132.104780] Internal error: Oops: 5 [#1] SMP ARM
[  132.105005] Modules linked in: snd_soc_rockchip_max98090(+)
snd_soc_ts3a227e smsc95xx mwifiex_sdio mwifiex btmrv
l_sdio btmrvl bluetooth rk_crypto sha256_generic nvmem_rockchip_efuse
snd_soc_max98090 snd_soc_rockchip_i2s nvmem_c
ore sha1_generic joydev rockchip_thermal [last unloaded: snd_soc_ts3a227e]
[  132.106579] CPU: 0 PID: 708 Comm: insmod Tainted: G        W
4.6.0-rc5+ #1
[  132.106920] Hardware name: Rockchip (Device Tree)
[  132.107147] task: ec1c8d80 ti: ec3e8000 task.ti: ec3e8000
[  132.107411] PC is at snd_jack_set_key+0x14/0x78
[  132.107643] LR is at ts3a227e_enable_jack_detect+0x38/0x8c [snd_soc_ts3a227e]
[  132.107979] pc : [<c06a3c10>]    lr : [<bf166150>]    psr: 60010013
sp : ec3e9bb0  ip : ec3e9bd0  fp : ec3e9bcc
[  132.108494] r10: 00000000  r9 : ec56a038  r8 : ec56a0a0
[  132.108742] r7 : ec569ff0  r6 : bf16bb30  r5 : eb8a7c58  r4 : bf16c080
[  132.109051] r3 : 00000000  r2 : 000000e2  r1 : 00004000  r0 : 00000000
[  132.109357] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[  132.109688] Control: 10c5387d  Table: 2ba3406a  DAC: 00000051
[  132.109958] Process insmod (pid: 708, stack limit = 0xec3e8218)
[  132.110236] Stack: (0xec3e9bb0 to 0xec3ea000)
[  132.110450] 9ba0:                                     bf16c080
eb8a7c58 bf16bb30 ec569ff0
[  132.110832] 9bc0: ec3e9be4 ec3e9bd0 bf166150 c06a3c08 ec56a000
00000000 ec3e9bf4 ec3e9be8

...

[  132.290280] 9fe0: bee024f0 bee024e0 7f6546c3 b6f57b42 80010030
00000003 00000000 00000000
[  132.299958] [<c06a3c10>] (snd_jack_set_key) from [<bf166150>]
(ts3a227e_enable_jack_detect+0x38/0x8c [snd_soc_ts
3a227e])
[  132.309961] [<bf166150>] (ts3a227e_enable_jack_detect
[snd_soc_ts3a227e]) from [<bf16b124>] (rk_98090_headset_in
it+0x1c/0x24 [snd_soc_rockchip_max98090])
[  132.330170] [<bf16b124>] (rk_98090_headset_init
[snd_soc_rockchip_max98090]) from [<c06b771c>] (soc_probe_compon
ent+0x22c/0x334)
[  132.351063] [<c06b771c>] (soc_probe_component) from [<c06ba6a0>]
(snd_soc_register_card+0x560/0xd14)
[  132.362015] [<c06ba6a0>] (snd_soc_register_card) from [<c06c7e60>]
(devm_snd_soc_register_card+0x50/0x8c)
[  132.373129] [<c06c7e60>] (devm_snd_soc_register_card) from
[<bf16b0c0>] (snd_rk_mc_probe+0xc0/0x108 [snd_soc_roc
kchip_max98090])
[  132.395594] [<bf16b0c0>] (snd_rk_mc_probe
[snd_soc_rockchip_max98090]) from [<c04af5bc>] (platform_drv_probe+0x6
0/0xb0)
[  132.407335] [<c04af5bc>] (platform_drv_probe) from [<c04ad03c>]
(driver_probe_device+0x1dc/0x41c)
[  132.419192] [<c04ad03c>] (driver_probe_device) from [<c04ad364>]
(__driver_attach+0xe8/0x11c)
[  132.431112] [<c04ad364>] (__driver_attach) from [<c04aaf24>]
(bus_for_each_dev+0x7c/0xa0)
[  132.443033] [<c04aaf24>] (bus_for_each_dev) from [<c04ac900>]
(driver_attach+0x28/0x30)
[  132.454945] [<c04ac900>] (driver_attach) from [<c04ac340>]
(bus_add_driver+0x128/0x254)
[  132.466853] [<c04ac340>] (bus_add_driver) from [<c04ae3a4>]
(driver_register+0xac/0xf0)
[  132.478765] [<c04ae3a4>] (driver_register) from [<c04af4fc>]
(__platform_driver_register+0x40/0x54)
[  132.490793] [<c04af4fc>] (__platform_driver_register) from
[<bf16e018>] (snd_rk_mc_driver_init+0x18/0x24 [snd_so
c_rockchip_max98090])
[  132.515020] [<bf16e018>] (snd_rk_mc_driver_init
[snd_soc_rockchip_max98090]) from [<c0101de8>] (do_one_initcall+
0x128/0x1e4)
[  132.527685] [<c0101de8>] (do_one_initcall) from [<c02172f4>]
(do_init_module+0x6c/0x3ac)
[  132.540430] [<c02172f4>] (do_init_module) from [<c01bb1b4>]
(load_module+0x1a40/0x1f64)
[  132.553282] [<c01bb1b4>] (load_module) from [<c01bb93c>]
(SyS_finit_module+0xc4/0xd4)
[  132.566158] [<c01bb93c>] (SyS_finit_module) from [<c0108540>]
(ret_fast_syscall+0x0/0x1c)
[  132.579069] Code: e92dd8f0 e24cb004 e52de004 e8bd4000 (e5903014)
[  132.592242] ---[ end trace ca16e398efcae7d0 ]---

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-04-25 14:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-06  5:29 [PATCH] ASoC: Make aux_dev more like a generic component mengdong.lin
2016-01-10 12:50 ` Applied "ASoC: Make aux_dev more like a generic component" to the asoc tree Mark Brown
2016-04-25 14:43   ` Enric Balletbo Serra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).