[PATCH 0/3] ASoC: core: Two step component registration
Provide a mechanism for true two-step component registration. This mimics device registration flow where initialization is the first step while addition goes as second in line. Drivers may choose to modify component's fields before registering component to ASoC subsystem via snd_soc_add_component.
Patchset achieves status quo - behavior of snd_soc_register_component remains unchanged.
Cezary Rojewski (3): ASoC: core: Relocate and expose snd_soc_component_initialize ASoC: core: Simplify snd_soc_component_initialize declaration ASoC: core: Two step component registration
include/sound/soc-component.h | 3 -- include/sound/soc.h | 11 +++--- sound/soc/soc-component.c | 16 --------- sound/soc/soc-core.c | 52 +++++++++++++++++---------- sound/soc/soc-generic-dmaengine-pcm.c | 14 +++++--- sound/soc/stm/stm32_adfsdm.c | 9 +++-- 6 files changed, 55 insertions(+), 50 deletions(-)
To allow for two-step component registration, expose snd_soc_component_initialize function and move it back to soc-core.c.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/soc-component.h | 3 --- include/sound/soc.h | 3 +++ sound/soc/soc-component.c | 16 ---------------- sound/soc/soc-core.c | 17 +++++++++++++++++ 4 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 8917b15eccae..089ea9441fd1 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -325,9 +325,6 @@ static inline int snd_soc_component_cache_sync( return regcache_sync(component->regmap); }
-int snd_soc_component_initialize(struct snd_soc_component *component, - const struct snd_soc_component_driver *driver, - struct device *dev, const char *name); void snd_soc_component_set_aux(struct snd_soc_component *component, struct snd_soc_aux_dev *aux); int snd_soc_component_init(struct snd_soc_component *component); diff --git a/include/sound/soc.h b/include/sound/soc.h index acbb5efb28ef..77a304d36c61 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -414,6 +414,9 @@ static inline int snd_soc_resume(struct device *dev) } #endif int snd_soc_poweroff(struct device *dev); +int snd_soc_component_initialize(struct snd_soc_component *component, + const struct snd_soc_component_driver *driver, + struct device *dev, const char *name); int snd_soc_add_component(struct device *dev, struct snd_soc_component *component, const struct snd_soc_component_driver *component_driver, diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c index dcc89fa8913a..f0b4f4bc44a4 100644 --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -33,22 +33,6 @@ static inline int _soc_component_ret(struct snd_soc_component *component, return ret; }
-int snd_soc_component_initialize(struct snd_soc_component *component, - const struct snd_soc_component_driver *driver, - struct device *dev, const char *name) -{ - INIT_LIST_HEAD(&component->dai_list); - INIT_LIST_HEAD(&component->dobj_list); - INIT_LIST_HEAD(&component->card_list); - mutex_init(&component->io_mutex); - - component->name = name; - component->dev = dev; - component->driver = driver; - - return 0; -} - void snd_soc_component_set_aux(struct snd_soc_component *component, struct snd_soc_aux_dev *aux) { diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index df4c7116f308..236755c7a79e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2441,6 +2441,23 @@ static void snd_soc_del_component_unlocked(struct snd_soc_component *component) list_del(&component->list); }
+int snd_soc_component_initialize(struct snd_soc_component *component, + const struct snd_soc_component_driver *driver, + struct device *dev, const char *name) +{ + INIT_LIST_HEAD(&component->dai_list); + INIT_LIST_HEAD(&component->dobj_list); + INIT_LIST_HEAD(&component->card_list); + mutex_init(&component->io_mutex); + + component->name = name; + component->dev = dev; + component->driver = driver; + + return 0; +} +EXPORT_SYMBOL_GPL(snd_soc_component_initialize); + int snd_soc_add_component(struct device *dev, struct snd_soc_component *component, const struct snd_soc_component_driver *component_driver,
Move 'name' field initialization responsibility back to snd_soc_component_initialize to prepare snd_soc_add_component function for being called separatelly as a second registration step.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/soc.h | 2 +- sound/soc/soc-core.c | 18 ++++++++---------- 2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 77a304d36c61..787374362f83 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -416,7 +416,7 @@ static inline int snd_soc_resume(struct device *dev) int snd_soc_poweroff(struct device *dev); int snd_soc_component_initialize(struct snd_soc_component *component, const struct snd_soc_component_driver *driver, - struct device *dev, const char *name); + struct device *dev); int snd_soc_add_component(struct device *dev, struct snd_soc_component *component, const struct snd_soc_component_driver *component_driver, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 236755c7a79e..f528367bc3be 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2443,14 +2443,19 @@ static void snd_soc_del_component_unlocked(struct snd_soc_component *component)
int snd_soc_component_initialize(struct snd_soc_component *component, const struct snd_soc_component_driver *driver, - struct device *dev, const char *name) + struct device *dev) { INIT_LIST_HEAD(&component->dai_list); INIT_LIST_HEAD(&component->dobj_list); INIT_LIST_HEAD(&component->card_list); mutex_init(&component->io_mutex);
- component->name = name; + component->name = fmt_single_name(dev, &component->id); + if (!component->name) { + dev_err(dev, "ASoC: Failed to allocate name\n"); + return -ENOMEM; + } + component->dev = dev; component->driver = driver;
@@ -2464,19 +2469,12 @@ int snd_soc_add_component(struct device *dev, struct snd_soc_dai_driver *dai_drv, int num_dai) { - const char *name = fmt_single_name(dev, &component->id); int ret; int i;
- if (!name) { - dev_err(dev, "ASoC: Failed to allocate name\n"); - return -ENOMEM; - } - mutex_lock(&client_mutex);
- ret = snd_soc_component_initialize(component, component_driver, - dev, name); + ret = snd_soc_component_initialize(component, component_driver, dev); if (ret) goto err_free;
Modify snd_soc_add_component so it calls snd_soc_component_initialize no longer and thus providing true two-step registration. Drivers may choose to change component's fields before actually adding it to ASoC subsystem.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/soc.h | 8 +++----- sound/soc/soc-core.c | 27 +++++++++++++-------------- sound/soc/soc-generic-dmaengine-pcm.c | 14 +++++++++----- sound/soc/stm/stm32_adfsdm.c | 9 +++++++-- 4 files changed, 32 insertions(+), 26 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 787374362f83..5e3919ffb00c 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -417,11 +417,9 @@ int snd_soc_poweroff(struct device *dev); int snd_soc_component_initialize(struct snd_soc_component *component, const struct snd_soc_component_driver *driver, struct device *dev); -int snd_soc_add_component(struct device *dev, - struct snd_soc_component *component, - const struct snd_soc_component_driver *component_driver, - struct snd_soc_dai_driver *dai_drv, - int num_dai); +int snd_soc_add_component(struct snd_soc_component *component, + struct snd_soc_dai_driver *dai_drv, + int num_dai); int snd_soc_register_component(struct device *dev, const struct snd_soc_component_driver *component_driver, struct snd_soc_dai_driver *dai_drv, int num_dai); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index f528367bc3be..2fe1b2ec7c8f 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2463,22 +2463,16 @@ int snd_soc_component_initialize(struct snd_soc_component *component, } EXPORT_SYMBOL_GPL(snd_soc_component_initialize);
-int snd_soc_add_component(struct device *dev, - struct snd_soc_component *component, - const struct snd_soc_component_driver *component_driver, - struct snd_soc_dai_driver *dai_drv, - int num_dai) +int snd_soc_add_component(struct snd_soc_component *component, + struct snd_soc_dai_driver *dai_drv, + int num_dai) { int ret; int i;
mutex_lock(&client_mutex);
- ret = snd_soc_component_initialize(component, component_driver, dev); - if (ret) - goto err_free; - - if (component_driver->endianness) { + if (component->driver->endianness) { for (i = 0; i < num_dai; i++) { convert_endianness_formats(&dai_drv[i].playback); convert_endianness_formats(&dai_drv[i].capture); @@ -2487,7 +2481,8 @@ 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); + dev_err(component->dev, "ASoC: Failed to register DAIs: %d\n", + ret); goto err_cleanup; }
@@ -2505,7 +2500,7 @@ int snd_soc_add_component(struct device *dev, err_cleanup: if (ret < 0) snd_soc_del_component_unlocked(component); -err_free: + mutex_unlock(&client_mutex);
if (ret == 0) @@ -2521,13 +2516,17 @@ int snd_soc_register_component(struct device *dev, int num_dai) { struct snd_soc_component *component; + int ret;
component = devm_kzalloc(dev, sizeof(*component), GFP_KERNEL); if (!component) return -ENOMEM;
- return snd_soc_add_component(dev, component, component_driver, - dai_drv, num_dai); + ret = snd_soc_component_initialize(component, component_driver, dev); + if (ret < 0) + return ret; + + return snd_soc_add_component(component, dai_drv, num_dai); } EXPORT_SYMBOL_GPL(snd_soc_register_component);
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index d17b4bf1dbe3..fb95c1464e66 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -424,6 +424,7 @@ static void dmaengine_pcm_release_chan(struct dmaengine_pcm *pcm) int snd_dmaengine_pcm_register(struct device *dev, const struct snd_dmaengine_pcm_config *config, unsigned int flags) { + const struct snd_soc_component_driver *driver; struct dmaengine_pcm *pcm; int ret;
@@ -442,12 +443,15 @@ int snd_dmaengine_pcm_register(struct device *dev, goto err_free_dma;
if (config && config->process) - ret = snd_soc_add_component(dev, &pcm->component, - &dmaengine_pcm_component_process, - NULL, 0); + driver = &dmaengine_pcm_component_process; else - ret = snd_soc_add_component(dev, &pcm->component, - &dmaengine_pcm_component, NULL, 0); + driver = &dmaengine_pcm_component; + + ret = snd_soc_component_initialize(&pcm->component, driver, dev); + if (ret) + goto err_free_dma; + + ret = snd_soc_add_component(&pcm->component, NULL, 0); if (ret) goto err_free_dma;
diff --git a/sound/soc/stm/stm32_adfsdm.c b/sound/soc/stm/stm32_adfsdm.c index c1433c20b08b..ec27c13af04f 100644 --- a/sound/soc/stm/stm32_adfsdm.c +++ b/sound/soc/stm/stm32_adfsdm.c @@ -344,12 +344,17 @@ static int stm32_adfsdm_probe(struct platform_device *pdev) component = devm_kzalloc(&pdev->dev, sizeof(*component), GFP_KERNEL); if (!component) return -ENOMEM; + + ret = snd_soc_component_initialize(component, + &stm32_adfsdm_soc_platform, + &pdev->dev); + if (ret < 0) + return ret; #ifdef CONFIG_DEBUG_FS component->debugfs_prefix = "pcm"; #endif
- ret = snd_soc_add_component(&pdev->dev, component, - &stm32_adfsdm_soc_platform, NULL, 0); + ret = snd_soc_add_component(component, NULL, 0); if (ret < 0) dev_err(&pdev->dev, "%s: Failed to register PCM platform\n", __func__);
On 7/31/20 9:41 AM, Cezary Rojewski wrote:
Provide a mechanism for true two-step component registration. This mimics device registration flow where initialization is the first step while addition goes as second in line. Drivers may choose to modify component's fields before registering component to ASoC subsystem via snd_soc_add_component.
I must admit I don't see where this might be used for Intel platforms, we've been happily using snd_soc_register_component() without hitting limitations.
Also the only two uses of snd_soc_add_component() seem mainly driven by memory allocation - and avoiding a devm_kzalloc in snd_soc_register_component().
Out of curiosity, can you provide an example where this two-step would be required or beneficial? Thanks!
Patchset achieves status quo - behavior of snd_soc_register_component remains unchanged.
Cezary Rojewski (3): ASoC: core: Relocate and expose snd_soc_component_initialize ASoC: core: Simplify snd_soc_component_initialize declaration ASoC: core: Two step component registration
include/sound/soc-component.h | 3 -- include/sound/soc.h | 11 +++--- sound/soc/soc-component.c | 16 --------- sound/soc/soc-core.c | 52 +++++++++++++++++---------- sound/soc/soc-generic-dmaengine-pcm.c | 14 +++++--- sound/soc/stm/stm32_adfsdm.c | 9 +++-- 6 files changed, 55 insertions(+), 50 deletions(-)
On 2020-07-31 5:07 PM, Pierre-Louis Bossart wrote:
On 7/31/20 9:41 AM, Cezary Rojewski wrote:
Provide a mechanism for true two-step component registration. This mimics device registration flow where initialization is the first step while addition goes as second in line. Drivers may choose to modify component's fields before registering component to ASoC subsystem via snd_soc_add_component.
I must admit I don't see where this might be used for Intel platforms, we've been happily using snd_soc_register_component() without hitting limitations.
Patchset targets entire ASoC framework, not Intel catalog. As _initialize and _add are already in place, having a two-step registration is cohesive with other "frameworks" e.g. device one.
New to ASoC? Trying to learn soc-components? Guess what, creation/registration procedure is exactly the same as one you're used to already!
Also the only two uses of snd_soc_add_component() seem mainly driven by memory allocation - and avoiding a devm_kzalloc in snd_soc_register_component().
In general, code quality and improvements to its flow should not require ton of usages. But hey, you got two already.
Out of curiosity, can you provide an example where this two-step would be required or beneficial? Thanks!
Overridding component->name which is currently always tied to fmt_single_name so you may choose a different name than the ->dev one.
On 7/31/20 5:47 PM, Cezary Rojewski wrote:
On 2020-07-31 5:07 PM, Pierre-Louis Bossart wrote:
On 7/31/20 9:41 AM, Cezary Rojewski wrote:
Provide a mechanism for true two-step component registration. This mimics device registration flow where initialization is the first step while addition goes as second in line. Drivers may choose to modify component's fields before registering component to ASoC subsystem via snd_soc_add_component.
I must admit I don't see where this might be used for Intel platforms, we've been happily using snd_soc_register_component() without hitting limitations.
Patchset targets entire ASoC framework, not Intel catalog. As _initialize and _add are already in place, having a two-step registration is cohesive with other "frameworks" e.g. device one.
New to ASoC? Trying to learn soc-components? Guess what, creation/registration procedure is exactly the same as one you're used to already!
Also the only two uses of snd_soc_add_component() seem mainly driven by memory allocation - and avoiding a devm_kzalloc in snd_soc_register_component().
In general, code quality and improvements to its flow should not require ton of usages. But hey, you got two already.
Out of curiosity, can you provide an example where this two-step would be required or beneficial? Thanks!
Overridding component->name which is currently always tied to fmt_single_name so you may choose a different name than the ->dev one.
For what it is worth, I think this is a sensible change for the outlined reasons. It's something I've always had in the back of my mind, but there was never enough of a need to actually do it.
One change I'd like to see is the addition of snd_soc_component_alloc(), which combines the step of kzalloc() and snd_soc_component_init() as these will be done pretty much always in lockstep. And it also matches the APIs that other frameworks offer.
- Lars
On 2020-07-31 5:58 PM, Lars-Peter Clausen wrote:
On 7/31/20 5:47 PM, Cezary Rojewski wrote:
On 2020-07-31 5:07 PM, Pierre-Louis Bossart wrote:
On 7/31/20 9:41 AM, Cezary Rojewski wrote:
Provide a mechanism for true two-step component registration. This mimics device registration flow where initialization is the first step while addition goes as second in line. Drivers may choose to modify component's fields before registering component to ASoC subsystem via snd_soc_add_component.
I must admit I don't see where this might be used for Intel platforms, we've been happily using snd_soc_register_component() without hitting limitations.
Patchset targets entire ASoC framework, not Intel catalog. As _initialize and _add are already in place, having a two-step registration is cohesive with other "frameworks" e.g. device one.
New to ASoC? Trying to learn soc-components? Guess what, creation/registration procedure is exactly the same as one you're used to already!
Also the only two uses of snd_soc_add_component() seem mainly driven by memory allocation - and avoiding a devm_kzalloc in snd_soc_register_component().
In general, code quality and improvements to its flow should not require ton of usages. But hey, you got two already.
Out of curiosity, can you provide an example where this two-step would be required or beneficial? Thanks!
Overridding component->name which is currently always tied to fmt_single_name so you may choose a different name than the ->dev one.
For what it is worth, I think this is a sensible change for the outlined reasons. It's something I've always had in the back of my mind, but there was never enough of a need to actually do it.
One change I'd like to see is the addition of snd_soc_component_alloc(), which combines the step of kzalloc() and snd_soc_component_init() as these will be done pretty much always in lockstep. And it also matches the APIs that other frameworks offer.
- Lars
Nice, so it's not just me imagining things : D
In general granular registration is robust and scales well into the future. Components functionality will only grow in time so I bet usecases don't end on my example.
I'd suggest transition to _alloc/_init/other being separated from this patchset - let it serve as a middle step.
Czarek
On Fri, Jul 31, 2020 at 05:58:00PM +0200, Lars-Peter Clausen wrote:
On 7/31/20 5:47 PM, Cezary Rojewski wrote:
Patchset targets entire ASoC framework, not Intel catalog. As _initialize and _add are already in place, having a two-step registration is cohesive with other "frameworks" e.g. device one.
For what it is worth, I think this is a sensible change for the outlined reasons. It's something I've always had in the back of my mind, but there was never enough of a need to actually do it.
Yeah, it's a common pattern in the kernel and someone might want it so there's no great reason not to do it.
One change I'd like to see is the addition of snd_soc_component_alloc(), which combines the step of kzalloc() and snd_soc_component_init() as these will be done pretty much always in lockstep. And it also matches the APIs that other frameworks offer.
That'd be good.
On Fri, 31 Jul 2020 16:41:43 +0200, Cezary Rojewski wrote:
Provide a mechanism for true two-step component registration. This mimics device registration flow where initialization is the first step while addition goes as second in line. Drivers may choose to modify component's fields before registering component to ASoC subsystem via snd_soc_add_component.
Patchset achieves status quo - behavior of snd_soc_register_component remains unchanged.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: core: Relocate and expose snd_soc_component_initialize commit: 08ff7209faf21daa01bf66c91c321ce52d4b4bdb [2/3] ASoC: core: Simplify snd_soc_component_initialize declaration commit: 7274d4cd8506bbff9bf2d7c2f73b2febff99abef [3/3] ASoC: core: Two step component registration commit: ea029dd8d0124fcd5db1c7003e87a7bd4ddb3bad
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
participants (4)
-
Cezary Rojewski -
Lars-Peter Clausen -
Mark Brown -
Pierre-Louis Bossart