Re: [alsa-devel] [PATCH] ASoC: core: add support to card re-bind/unbind using component framework

[Takashi Iwai]

On Thu, 12 Jul 2018 17:26:00 +0200,
Pierre-Louis Bossart wrote:
> 
> On 7/11/18 3:43 AM, Srinivas Kandagatla wrote:
> > This patch aims at add achieving dynamic behaviour of audio card when
> > the dependent components disappear and reappear.
> >
> > With this patch the card is removed if any of the dependent component
> > is removed and card is added back if the dependent component comes back.
> > All this is done using component framework and matching based on
> > component name.
> 
> Humm, no real comment on this patch proper but rather on how userspace
> would deal with this dynamic behavior?
> We had similar opens when we worked on the BYT/CHT HDMI stuff and
> ended-up with quite a few issues in userspace (PulseAudio mainly)
> related to dynamic behavior at the kernel level as a result of
> plug/unplug. Also not sure how an Android HAL would deal with a card
> disappearing temporarily if DSP resources become unavailable or
> unresponsive. Any thoughts or guidance you might think of?

Was the card-level register / unregister really problematic with PA?
It's basically similar as the hotplug like USB, so I thought it would
work as is.


Takashi

> 
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
> >
> > During discussion regarding card re-binding when components unregister
> > and register back at https://lkml.org/lkml/2018/7/9/785 it was suggested
> > that component framework can be added into core to provide this feature.
> >
> > With this new changes the card will re-bind once the dependent component
> > re-registers after unregistering. This works based on the match done
> > from component name using component framework.
> >
> > I have tested this patch with qdsp start-stop usecase for more than 10000
> > times in loop on Qcom platforms.
> >
> > I will send qdsp side cleanup patches once I get some feedback on this patch.
> >
> >
> >   include/sound/soc.h  |  5 ++++
> >   sound/soc/soc-core.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >   2 files changed, 70 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/sound/soc.h b/include/sound/soc.h
> > index 870ba6b64817..b94149d29c0d 100644
> > --- a/include/sound/soc.h
> > +++ b/include/sound/soc.h
> > @@ -17,6 +17,7 @@
> >   #include <linux/workqueue.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/kernel.h>
> > +#include <linux/component.h>
> >   #include <linux/regmap.h>
> >   #include <linux/log2.h>
> >   #include <sound/core.h>
> > @@ -1088,6 +1089,10 @@ struct snd_soc_card {
> >     	struct work_struct deferred_resume_work;
> >   +	/* component framework related */
> > +	bool components_added;
> > +	struct component_match *match;
> > +
> >   	/* lists of probed devices belonging to this card */
> >   	struct list_head component_dev_list;
> >   diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> > index 6d33634b934b..377ed8e67686 100644
> > --- a/sound/soc/soc-core.c
> > +++ b/sound/soc/soc-core.c
> > @@ -279,11 +279,31 @@ static inline void snd_soc_debugfs_exit(void)
> >     #endif
> >   +static int snd_soc_card_comp_compare(struct device *dev, void
> > *data)
> > +{
> > +	struct snd_soc_component *component = NULL;
> > +	struct snd_soc_component *t;
> > +
> > +	lockdep_assert_held(&client_mutex);
> > +	list_for_each_entry(t, &component_list, list) {
> > +		if (dev == t->dev) {
> > +			component = t;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (component && !strcmp(component->name, data))
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> >   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_rtdcom_list *new_rtdcom;
> > +	char *cname;
> >     	for_each_rtdcom(rtd, rtdcom) {
> >   		/* already connected */
> > @@ -300,6 +320,13 @@ static int snd_soc_rtdcom_add(struct snd_soc_pcm_runtime *rtd,
> >     	list_add_tail(&new_rtdcom->list, &rtd->component_list);
> >   +	if (!rtd->card->components_added) {
> > +		cname = devm_kasprintf(rtd->card->dev, GFP_KERNEL,
> > +				       "%s", component->name);
> > +		component_match_add(rtd->card->dev, &rtd->card->match,
> > +				    snd_soc_card_comp_compare, cname);
> > +	}
> > +
> >   	return 0;
> >   }
> >   @@ -835,6 +862,28 @@ static bool soc_is_dai_link_bound(struct
> > snd_soc_card *card,
> >   	return false;
> >   }
> >   +static int snd_soc_card_comp_bind(struct device *dev)
> > +{
> > +	struct snd_soc_card *card = dev_get_drvdata(dev);
> > +
> > +	if (card->instantiated)
> > +		return 0;
> > +
> > +	return snd_soc_register_card(card);
> > +}
> > +
> > +static void snd_soc_card_comp_unbind(struct device *dev)
> > +{
> > +	struct snd_soc_card *card = dev_get_drvdata(dev);
> > +
> > +	snd_soc_unregister_card(card);
> > +}
> > +
> > +static const struct component_master_ops snd_soc_card_comp_ops = {
> > +	.bind = snd_soc_card_comp_bind,
> > +	.unbind = snd_soc_card_comp_unbind,
> > +};
> > +
> >   static int soc_bind_dai_link(struct snd_soc_card *card,
> >   	struct snd_soc_dai_link *dai_link)
> >   {
> > @@ -2108,6 +2157,12 @@ static int snd_soc_instantiate_card(struct snd_soc_card *card)
> >     	card->instantiated = 1;
> >   	snd_soc_dapm_sync(&card->dapm);
> > +	if (!card->components_added) {
> > +		component_master_add_with_match(card->dev,
> > +						&snd_soc_card_comp_ops,
> > +						card->match);
> > +		card->components_added = true;
> > +	}
> >   	mutex_unlock(&card->mutex);
> >   	mutex_unlock(&client_mutex);
> >   @@ -3098,11 +3153,6 @@ static void
> > snd_soc_component_cleanup(struct snd_soc_component *component)
> >     static void snd_soc_component_del_unlocked(struct
> > snd_soc_component *component)
> >   {
> > -	struct snd_soc_card *card = component->card;
> > -
> > -	if (card)
> > -		snd_soc_unregister_card(card);
> > -
> >   	list_del(&component->list);
> >   }
> >   @@ -3169,8 +3219,17 @@ int snd_soc_add_component(struct device
> > *dev,
> >     	snd_soc_component_add(component);
> >   +	ret = component_add(dev, NULL);
> > +	if (ret < 0) {
> > +		dev_err(dev, "ASoC: Failed to add Component: %d\n", ret);
> > +		goto err_comp;
> > +	}
> > +
> >   	return 0;
> >   +err_comp:
> > +	soc_remove_component(component);
> > +	snd_soc_unregister_dais(component);
> >   err_cleanup:
> >   	snd_soc_component_cleanup(component);
> >   err_free:
> > @@ -3218,6 +3277,7 @@ static int __snd_soc_unregister_component(struct device *dev)
> >   	mutex_unlock(&client_mutex);
> >     	if (found) {
> > +		component_del(dev, NULL);
> >   		snd_soc_component_cleanup(component);
> >   	}
> >   
> >
> 
>