diff mbox series

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

Message ID 878qn82alv.wl-kuninori.morimoto.gx@renesas.com
State New
Headers show
Series ASoC: remove component->id | expand

Commit Message

Kuninori Morimoto May 8, 2025, 12:46 a.m. UTC 
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(-)

Comments

Srinivas Kandagatla May 9, 2025, 11 a.m. UTC | #1 
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.
Kuninori Morimoto May 12, 2025, 12:44 a.m. UTC | #2 
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
diff mbox series

Patch

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;