[PATCH 0/3] ASoC: remove component->id

[PATCH 0/3] ASoC: remove component->id

[Kuninori Morimoto]

Hi Mark, Qcom member

snd_soc_component has "id", but no one is using it except Qcom. It is
initialized at snd_soc_component_initialize(), but Qcom overwrites it.
Qcom can use own private data for it. So, we don't need to have
component->id anymore. Let's remove it.

Kuninori Morimoto (3):
  ASoC: qcom: use drvdata instead of component to keep id
  ASoC: soc-core: save ID if param was set in fmt_single_name()
  ASoC: remove component->id

 include/sound/soc-component.h   |  1 -
 sound/soc/qcom/lpass-platform.c | 10 ++++++----
 sound/soc/qcom/lpass.h          |  1 +
 sound/soc/soc-core.c            | 14 +++++++++-----
 4 files changed, 16 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH 1/3] ASoC: qcom: use drvdata instead of component to keep id

[Kuninori Morimoto]

qcom lpass is using component->id to keep DAI ID (A).

(S)	static int lpass_platform_pcmops_open(
				sruct snd_soc_component *component,
				struct snd_pcm_substream *substream)
	{			                          ^^^^^^^^^(B0)
		...
(B1)		struct snd_soc_pcm_runtime *soc_runtime = snd_soc_substream_to_rtd(substream);
(B2)		struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
		...
(B3)		unsigned int dai_id = cpu_dai->driver->id;

(A)		component->id = dai_id;
		...
	}

This driver can get dai_id from substream (B0 - B3).
In this driver, below functions get dai_id from component->id (A).

(X)	lpass_platform_pcmops_suspend()
(Y)	lpass_platform_pcmops_resume()
(Z)	lpass_platform_copy()

Here, (Z) can get it from substream (B0 - B3), don't need to use
component->id (A). On suspend/resume (X)(Y), dai_id can only be obtained
from component->id (A), because there is no substream (B0) in function
parameter.

But, component->id (A) itself should not be used for such purpose.
It is intilialized at snd_soc_component_initialize(), and parsed its ID
(= component->id) from device name (a).

	int snd_soc_component_initialize(...)
	{
		...
		if (!component->name) {
(a)			component->name = fmt_single_name(dev, &component->id);
			...                                     ^^^^^^^^^^^^^
		}
		...
	}

On this driver, drvdata : component = 1 : 1 relatationship (b)

(b)	struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);

drvdata can be used on lpass_platform_pcmops_open() (S),
lpass_platform_pcmops_suspend()/lpass_platform_pcmops_resume() (X)(Y).

We can keep dai_id on drvdata->id instead of component->id (A). Let's do it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/qcom/lpass-platform.c | 10 ++++++----
 sound/soc/qcom/lpass.h          |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
index 9946f12254b3..676018b8134a 100644
--- a/sound/soc/qcom/lpass-platform.c
+++ b/sound/soc/qcom/lpass-platform.c
@@ -202,7 +202,7 @@ static int lpass_platform_pcmops_open(struct snd_soc_component *component,
 	struct regmap *map;
 	unsigned int dai_id = cpu_dai->driver->id;
 
-	component->id = dai_id;
+	drvdata->id = dai_id;
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -1190,7 +1190,7 @@ static int lpass_platform_pcmops_suspend(struct snd_soc_component *component)
 {
 	struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
 	struct regmap *map;
-	unsigned int dai_id = component->id;
+	unsigned int dai_id = drvdata->id;
 
 	if (dai_id == LPASS_DP_RX)
 		map = drvdata->hdmiif_map;
@@ -1207,7 +1207,7 @@ static int lpass_platform_pcmops_resume(struct snd_soc_component *component)
 {
 	struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
 	struct regmap *map;
-	unsigned int dai_id = component->id;
+	unsigned int dai_id = drvdata->id;
 
 	if (dai_id == LPASS_DP_RX)
 		map = drvdata->hdmiif_map;
@@ -1224,7 +1224,9 @@ static int lpass_platform_copy(struct snd_soc_component *component,
 			       unsigned long bytes)
 {
 	struct snd_pcm_runtime *rt = substream->runtime;
-	unsigned int dai_id = component->id;
+	struct snd_soc_pcm_runtime *soc_runtime = snd_soc_substream_to_rtd(substream);
+	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
+	unsigned int dai_id = cpu_dai->driver->id;
 	int ret = 0;
 
 	void __iomem *dma_buf = (void __iomem *) (rt->dma_area + pos +
diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
index de3ec6f594c1..7663dafef18a 100644
--- a/sound/soc/qcom/lpass.h
+++ b/sound/soc/qcom/lpass.h
@@ -93,6 +93,7 @@ struct lpaif_dmactl {
 
 /* Both the CPU DAI and platform drivers will access this data */
 struct lpass_data {
+	int id;
 
 	/* AHB-I/X bus clocks inside the low-power audio subsystem (LPASS) */
 	struct clk *ahbix_clk;
-- 
2.43.0

Re: [PATCH 1/3] ASoC: qcom: use drvdata instead of component to keep id

[Srinivas Kandagatla]

Thanks for the patch,

On 5/8/25 01:46, Kuninori Morimoto wrote:
> qcom lpass is using component->id to keep DAI ID (A).
> 
> (S)	static int lpass_platform_pcmops_open(
> 				sruct snd_soc_component *component,
> 				struct snd_pcm_substream *substream)
> 	{			                          ^^^^^^^^^(B0)
> 		...
> (B1)		struct snd_soc_pcm_runtime *soc_runtime = snd_soc_substream_to_rtd(substream);
> (B2)		struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
> 		...
> (B3)		unsigned int dai_id = cpu_dai->driver->id;
> 
> (A)		component->id = dai_id;
> 		...
> 	}
> 
> This driver can get dai_id from substream (B0 - B3).
> In this driver, below functions get dai_id from component->id (A).
> 
> (X)	lpass_platform_pcmops_suspend()
> (Y)	lpass_platform_pcmops_resume()
> (Z)	lpass_platform_copy()
> 
> Here, (Z) can get it from substream (B0 - B3), don't need to use
> component->id (A). On suspend/resume (X)(Y), dai_id can only be obtained
> from component->id (A), because there is no substream (B0) in function
> parameter.
> 
> But, component->id (A) itself should not be used for such purpose.
> It is intilialized at snd_soc_component_initialize(), and parsed its ID
> (= component->id) from device name (a).
> 
> 	int snd_soc_component_initialize(...)
> 	{
> 		...
> 		if (!component->name) {
> (a)			component->name = fmt_single_name(dev, &component->id);
> 			...                                     ^^^^^^^^^^^^^
> 		}
> 		...
> 	}
> 
> On this driver, drvdata : component = 1 : 1 relatationship (b)
> 
> (b)	struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
> 
> drvdata can be used on lpass_platform_pcmops_open() (S),
> lpass_platform_pcmops_suspend()/lpass_platform_pcmops_resume() (X)(Y).
> 
> We can keep dai_id on drvdata->id instead of component->id (A). Let's do it.
> 
Current code seems to be broken to start with.
May be we can fix that and also get rid of usage of component->id together.

From what I see that there are many regmaps that the driver cares about
however its only managing one(either dp or i2s) in component suspend
resume-path.

i2s regmap is mandatory however other regmaps are setup based on flags
like hdmi_port_enable and codec_dma_enable.

correct thing for suspend resume path to handle is by checking these
flags, instead of using component->id.

------------------>cut<----------------------------------
diff --git a/sound/soc/qcom/lpass-platform.c
b/sound/soc/qcom/lpass-platform.c
index 9946f12254b3..d8b0a637b1c6 100644
--- a/sound/soc/qcom/lpass-platform.c
+++ b/sound/soc/qcom/lpass-platform.c
@@ -202,7 +202,6 @@ static int lpass_platform_pcmops_open(struct
snd_soc_component *component,
        struct regmap *map;
        unsigned int dai_id = cpu_dai->driver->id;

-       component->id = dai_id;
        data = kzalloc(sizeof(*data), GFP_KERNEL);
        if (!data)
                return -ENOMEM;
@@ -1190,13 +1189,14 @@ static int lpass_platform_pcmops_suspend(struct
snd_soc_component *component)
 {
        struct lpass_data *drvdata =
snd_soc_component_get_drvdata(component);
        struct regmap *map;
-       unsigned int dai_id = component->id;

-       if (dai_id == LPASS_DP_RX)
+       if (drvdata->hdmi_port_enable) {
                map = drvdata->hdmiif_map;
-       else
-               map = drvdata->lpaif_map;
+               regcache_cache_only(map, true);
+               regcache_mark_dirty(map);
+       }

+       map = drvdata->lpaif_map;
        regcache_cache_only(map, true);
        regcache_mark_dirty(map);

@@ -1207,15 +1207,21 @@ static int lpass_platform_pcmops_resume(struct
snd_soc_component *component)
 {
        struct lpass_data *drvdata =
snd_soc_component_get_drvdata(component);
        struct regmap *map;
-       unsigned int dai_id = component->id;
+       int ret;

-       if (dai_id == LPASS_DP_RX)
+       if (drvdata->hdmi_port_enable) {
                map = drvdata->hdmiif_map;
-       else
-               map = drvdata->lpaif_map;
+               regcache_cache_only(map, false);
+               ret = regcache_sync(map);
+               if (ret)
+                       return ret;
+       }

+       map = drvdata->lpaif_map;
        regcache_cache_only(map, false);
+
        return regcache_sync(map);
 }

 static int lpass_platform_copy(struct snd_soc_component *component,
@@ -1224,7 +1230,9 @@ static int lpass_platform_copy(struct
snd_soc_component *component,
                               unsigned long bytes)
 {
        struct snd_pcm_runtime *rt = substream->runtime;
-       unsigned int dai_id = component->id;
+       struct snd_soc_pcm_runtime *soc_runtime =
snd_soc_substream_to_rtd(substream);
+       struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
+       unsigned int dai_id = cpu_dai->driver->id;
        int ret = 0;

        void __iomem *dma_buf = (void __iomem *) (rt->dma_area + pos +
-------------------->cut<-----------------------

--srini	
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  sound/soc/qcom/lpass-platform.c | 10 ++++++----
>  sound/soc/qcom/lpass.h          |  1 +
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
> index 9946f12254b3..676018b8134a 100644
> --- a/sound/soc/qcom/lpass-platform.c
> +++ b/sound/soc/qcom/lpass-platform.c
> @@ -202,7 +202,7 @@ static int lpass_platform_pcmops_open(struct snd_soc_component *component,
>  	struct regmap *map;
>  	unsigned int dai_id = cpu_dai->driver->id;
>  
> -	component->id = dai_id;
> +	drvdata->id = dai_id;
>  	data = kzalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
> @@ -1190,7 +1190,7 @@ static int lpass_platform_pcmops_suspend(struct snd_soc_component *component)
>  {
>  	struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
>  	struct regmap *map;
> -	unsigned int dai_id = component->id;
> +	unsigned int dai_id = drvdata->id;
>  
>  	if (dai_id == LPASS_DP_RX)
>  		map = drvdata->hdmiif_map;
> @@ -1207,7 +1207,7 @@ static int lpass_platform_pcmops_resume(struct snd_soc_component *component)
>  {
>  	struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
>  	struct regmap *map;
> -	unsigned int dai_id = component->id;
> +	unsigned int dai_id = drvdata->id;
>  
>  	if (dai_id == LPASS_DP_RX)
>  		map = drvdata->hdmiif_map;
> @@ -1224,7 +1224,9 @@ static int lpass_platform_copy(struct snd_soc_component *component,
>  			       unsigned long bytes)
>  {
>  	struct snd_pcm_runtime *rt = substream->runtime;
> -	unsigned int dai_id = component->id;
> +	struct snd_soc_pcm_runtime *soc_runtime = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
> +	unsigned int dai_id = cpu_dai->driver->id;
>  	int ret = 0;
>  
>  	void __iomem *dma_buf = (void __iomem *) (rt->dma_area + pos +
> diff --git a/sound/soc/qcom/lpass.h b/sound/soc/qcom/lpass.h
> index de3ec6f594c1..7663dafef18a 100644
> --- a/sound/soc/qcom/lpass.h
> +++ b/sound/soc/qcom/lpass.h
> @@ -93,6 +93,7 @@ struct lpaif_dmactl {
>  
>  /* Both the CPU DAI and platform drivers will access this data */
>  struct lpass_data {
> +	int id;
>  
>  	/* AHB-I/X bus clocks inside the low-power audio subsystem (LPASS) */
>  	struct clk *ahbix_clk;

Re: [PATCH 1/3] ASoC: qcom: use drvdata instead of component to keep id

[Kuninori Morimoto]

Hi Srinivas

Thank you for the feedback

> > qcom lpass is using component->id to keep DAI ID (A).
> > 
> > (S)	static int lpass_platform_pcmops_open(
> > 				sruct snd_soc_component *component,
> > 				struct snd_pcm_substream *substream)
> > 	{			                          ^^^^^^^^^(B0)
> > 		...
> > (B1)		struct snd_soc_pcm_runtime *soc_runtime = snd_soc_substream_to_rtd(substream);
> > (B2)		struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
> > 		...
> > (B3)		unsigned int dai_id = cpu_dai->driver->id;
> > 
> > (A)		component->id = dai_id;
> > 		...
> > 	}
> > 
> > This driver can get dai_id from substream (B0 - B3).
> > In this driver, below functions get dai_id from component->id (A).
> > 
> > (X)	lpass_platform_pcmops_suspend()
> > (Y)	lpass_platform_pcmops_resume()
> > (Z)	lpass_platform_copy()
> > 
> > Here, (Z) can get it from substream (B0 - B3), don't need to use
> > component->id (A). On suspend/resume (X)(Y), dai_id can only be obtained
> > from component->id (A), because there is no substream (B0) in function
> > parameter.
> > 
> > But, component->id (A) itself should not be used for such purpose.
> > It is intilialized at snd_soc_component_initialize(), and parsed its ID
> > (= component->id) from device name (a).
> > 
> > 	int snd_soc_component_initialize(...)
> > 	{
> > 		...
> > 		if (!component->name) {
> > (a)			component->name = fmt_single_name(dev, &component->id);
> > 			...                                     ^^^^^^^^^^^^^
> > 		}
> > 		...
> > 	}
> > 
> > On this driver, drvdata : component = 1 : 1 relatationship (b)
> > 
> > (b)	struct lpass_data *drvdata = snd_soc_component_get_drvdata(component);
> > 
> > drvdata can be used on lpass_platform_pcmops_open() (S),
> > lpass_platform_pcmops_suspend()/lpass_platform_pcmops_resume() (X)(Y).
> > 
> > We can keep dai_id on drvdata->id instead of component->id (A). Let's do it.
> > 
> Current code seems to be broken to start with.
> May be we can fix that and also get rid of usage of component->id together.
> 
> From what I see that there are many regmaps that the driver cares about
> however its only managing one(either dp or i2s) in component suspend
> resume-path.
> 
> i2s regmap is mandatory however other regmaps are setup based on flags
> like hdmi_port_enable and codec_dma_enable.
> 
> correct thing for suspend resume path to handle is by checking these
> flags, instead of using component->id.

Thanks. I have merged your code.
I will post it as v2

Thank you for your help !!

Best regards
---
Kuninori Morimoto

[PATCH 2/3] ASoC: soc-core: save ID if param was set in fmt_single_name()

[Kuninori Morimoto]

fmt_single_name() requests "ind *id" and not allow NULL for it.
But sometimes we don't need it. Allow NULL.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index ab615ec113d2..80569209ce05 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2604,6 +2604,7 @@ static char *fmt_single_name(struct device *dev, int *id)
 	const char *devname = dev_name(dev);
 	char *found, *name;
 	unsigned int id1, id2;
+	int __id;
 
 	if (devname == NULL)
 		return NULL;
@@ -2616,10 +2617,10 @@ static char *fmt_single_name(struct device *dev, int *id)
 	found = strstr(name, dev->driver->name);
 	if (found) {
 		/* get ID */
-		if (sscanf(&found[strlen(dev->driver->name)], ".%d", id) == 1) {
+		if (sscanf(&found[strlen(dev->driver->name)], ".%d", &__id) == 1) {
 
 			/* discard ID from name if ID == -1 */
-			if (*id == -1)
+			if (__id == -1)
 				found[strlen(dev->driver->name)] = '\0';
 		}
 
@@ -2627,16 +2628,19 @@ static char *fmt_single_name(struct device *dev, int *id)
 	} else if (sscanf(name, "%x-%x", &id1, &id2) == 2) {
 
 		/* create unique ID number from I2C addr and bus */
-		*id = ((id1 & 0xffff) << 16) + id2;
+		__id = ((id1 & 0xffff) << 16) + id2;
 
 		devm_kfree(dev, name);
 
 		/* sanitize component name for DAI link creation */
 		name = devm_kasprintf(dev, GFP_KERNEL, "%s.%s", dev->driver->name, devname);
 	} else {
-		*id = 0;
+		__id = 0;
 	}
 
+	if (id)
+		*id = __id;
+
 	return name;
 }
 
-- 
2.43.0

[PATCH 3/3] ASoC: remove component->id

[Kuninori Morimoto]

No one is using component->id.
One idea is we can re-use it as serial number for component.
But we have no usage, so far. Let's just remove it for now.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc-component.h | 1 -
 sound/soc/soc-core.c          | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 61534ac0edd1..2caa807c6249 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -206,7 +206,6 @@ struct snd_soc_component_driver {
 
 struct snd_soc_component {
 	const char *name;
-	int id;
 	const char *name_prefix;
 	struct device *dev;
 	struct snd_soc_card *card;
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 80569209ce05..39ecf0123a44 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2835,7 +2835,7 @@ int snd_soc_component_initialize(struct snd_soc_component *component,
 	mutex_init(&component->io_mutex);
 
 	if (!component->name) {
-		component->name = fmt_single_name(dev, &component->id);
+		component->name = fmt_single_name(dev, NULL);
 		if (!component->name) {
 			dev_err(dev, "ASoC: Failed to allocate name\n");
 			return -ENOMEM;
-- 
2.43.0