[PATCH 0/3] ASoC: core: Two step component registration

[PATCH 0/3] ASoC: core: Two step component registration

[Cezary Rojewski]

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(-)

-- 
2.17.1

[PATCH 1/3] ASoC: core: Relocate and expose snd_soc_component_initialize

[Cezary Rojewski]

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,
-- 
2.17.1

[PATCH 2/3] ASoC: core: Simplify snd_soc_component_initialize declaration

[Cezary Rojewski]

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;
 
-- 
2.17.1

[PATCH 3/3] ASoC: core: Two step component registration

[Cezary Rojewski]

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__);
-- 
2.17.1

Re: [PATCH 0/3] ASoC: core: Two step component registration

[Pierre-Louis Bossart]

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(-)
> 

Re: [PATCH 0/3] ASoC: core: Two step component registration

[Cezary Rojewski]

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.

Re: [PATCH 0/3] ASoC: core: Two step component registration

[Lars-Peter Clausen]

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

Re: [PATCH 0/3] ASoC: core: Two step component registration

[Cezary Rojewski]

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

Re: [PATCH 0/3] ASoC: core: Two step component registration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] ASoC: core: Two step component registration

[Mark Brown]

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