| Message ID | 877eyx6uiu.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 25, 2017 at 04:00:48AM +0000, Kuninori Morimoto wrote: > ALSA SoC platform/codec will be replaced to component soon. > This means 1 device might have multiple components. But current > unregister component function only checks "dev" to find it. > This means, unexpected component might be unregistered by current > function. > But, it is no problem if driver registered only 1 component. Isn't this just a convenience for drivers though? It means they can just clean things up with one call and I'm having a hard time thinking of any use cases for only unregistering some components. > To avoid this issue, this patch adds new component > unregister_exp/lookup/remove functions. "lookup" function finds > component by "dev" and "driver name", and "remove" function removes it. > "unregister_exp" will use these functions. The exp name here seems a bit confusing... perhaps just _internal() or something? > +#define snd_soc_unregister_component(dev) snd_soc_unregister_component_exp(dev, NULL) A static inline is better, it's got better type safety.
Hi Mark > > ALSA SoC platform/codec will be replaced to component soon. > > This means 1 device might have multiple components. But current > > unregister component function only checks "dev" to find it. > > This means, unexpected component might be unregistered by current > > function. > > But, it is no problem if driver registered only 1 component. > > Isn't this just a convenience for drivers though? It means they can > just clean things up with one call and I'm having a hard time thinking > of any use cases for only unregistering some components. This means like this OK case register_component(dev, driver_A); unregister_component(dev); /* driver_A will be unregister */ not OK case register_component(dev, driver_A); register_component(dev, driver_B); unregister_component(dev); /* it can't specify driver_A */ unregister_component(dev); /* it can't specify driver_B */ or do we want to have like this ? register_component(dev, driver_A); register_component(dev, driver_B); unregister_component(dev); /* unregister both driver_A/driver_B */ > > To avoid this issue, this patch adds new component > > unregister_exp/lookup/remove functions. "lookup" function finds > > component by "dev" and "driver name", and "remove" function removes it. > > "unregister_exp" will use these functions. > > The exp name here seems a bit confusing... perhaps just _internal() or > something? OK, will fix in v2 > > +#define snd_soc_unregister_component(dev) snd_soc_unregister_component_exp(dev, NULL) > > A static inline is better, it's got better type safety. OK, will do Best regards --- Kuninori Morimoto
On Wed, Aug 02, 2017 at 11:35:38AM +0000, Kuninori Morimoto wrote: > > Isn't this just a convenience for drivers though? It means they can > > just clean things up with one call and I'm having a hard time thinking > > of any use cases for only unregistering some components. > This means like this > or do we want to have like this ? > register_component(dev, driver_A); > register_component(dev, driver_B); > unregister_component(dev); /* unregister both driver_A/driver_B */ This was what I was thinking of. It might not actually be the most sensible thing but I'm pretty sure it's what I was thinking when things were set up originally, if there's other use cases we might need to change.
Hi Mark > > > Isn't this just a convenience for drivers though? It means they can > > > just clean things up with one call and I'm having a hard time thinking > > > of any use cases for only unregistering some components. > > > This means like this > > > or do we want to have like this ? > > > register_component(dev, driver_A); > > register_component(dev, driver_B); > > > unregister_component(dev); /* unregister both driver_A/driver_B */ > > This was what I was thinking of. It might not actually be the most > sensible thing but I'm pretty sure it's what I was thinking when things > were set up originally, if there's other use cases we might need to > change. OK, I see. The main headache of "platform" replacement to "component" was dmaengine. (If my memory was correct) It only uses "lookup", "remove" kind of function on unregister. But, if we can unregister all component by 1 calls, these kind of headache will gone. Best regards --- Kuninori Morimoto
diff --git a/include/sound/soc.h b/include/sound/soc.h index c4a8b19..82209be 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -474,7 +474,12 @@ int snd_soc_register_component(struct device *dev, int devm_snd_soc_register_component(struct device *dev, const struct snd_soc_component_driver *cmpnt_drv, struct snd_soc_dai_driver *dai_drv, int num_dai); -void snd_soc_unregister_component(struct device *dev); +#define snd_soc_unregister_component(dev) snd_soc_unregister_component_exp(dev, NULL) +void snd_soc_unregister_component_exp(struct device *dev, + const char *driver_name); +void snd_soc_remove_component(struct snd_soc_component *component); +struct snd_soc_component *snd_soc_lookup_component(struct device *dev, + const char *driver_name); int snd_soc_cache_init(struct snd_soc_codec *codec); int snd_soc_cache_exit(struct snd_soc_codec *codec); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index c240e13..0bd7764 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3323,26 +3323,56 @@ int snd_soc_register_component(struct device *dev, * * @dev: The device to unregister */ -void snd_soc_unregister_component(struct device *dev) +void snd_soc_unregister_component_exp(struct device *dev, + const char *driver_name) { - struct snd_soc_component *cmpnt; + struct snd_soc_component *component; + + component = snd_soc_lookup_component(dev, driver_name); + if (!component || !component->registered_as_component) + return; + snd_soc_remove_component(component); +} +EXPORT_SYMBOL_GPL(snd_soc_unregister_component_exp); + +void snd_soc_remove_component(struct snd_soc_component *component) +{ mutex_lock(&client_mutex); - list_for_each_entry(cmpnt, &component_list, list) { - if (dev == cmpnt->dev && cmpnt->registered_as_component) - goto found; - } + snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL); + snd_soc_component_del_unlocked(component); mutex_unlock(&client_mutex); - return; -found: - snd_soc_tplg_component_remove(cmpnt, SND_SOC_TPLG_INDEX_ALL); - snd_soc_component_del_unlocked(cmpnt); + snd_soc_component_cleanup(component); + kfree(component); +} +EXPORT_SYMBOL_GPL(snd_soc_remove_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); + list_for_each_entry(component, &component_list, list) { + 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); - snd_soc_component_cleanup(cmpnt); - kfree(cmpnt); + + return ret; } -EXPORT_SYMBOL_GPL(snd_soc_unregister_component); +EXPORT_SYMBOL_GPL(snd_soc_lookup_component); static int snd_soc_platform_drv_probe(struct snd_soc_component *component) {