| Message ID | 87jzz7jczp.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Accepted |
| Commit | e7098ba9b3785d626326040d300f95fec79aa765 |
| Headers | show |
| Series | ASoC: remove unnecessary dai_link->platform | expand |
Morimoto-san, On 24/03/2023 00:53, Kuninori Morimoto wrote: > dai_link->platform is no longer needed if CPU and Platform are > same Component. This patch removes unnecessary dai_link->platform. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/soc-topology.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c > index 07421f5d4ebd..7f6424fa59ab 100644 > --- a/sound/soc/soc-topology.c > +++ b/sound/soc/soc-topology.c > @@ -1685,8 +1685,8 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > struct snd_soc_dai_link_component *dlc; > int ret; > > - /* link + cpu + codec + platform */ > - link = devm_kzalloc(tplg->dev, sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); > + /* link + cpu + codec */ > + link = devm_kzalloc(tplg->dev, sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); > if (link == NULL) > return -ENOMEM; > > @@ -1694,11 +1694,9 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > link->cpus = &dlc[0]; > link->codecs = &dlc[1]; > - link->platforms = &dlc[2]; > > link->num_cpus = 1; > link->num_codecs = 1; > - link->num_platforms = 1; > > link->dobj.index = tplg->index; > link->dobj.type = SND_SOC_DOBJ_DAI_LINK; > @@ -1726,8 +1724,6 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > link->codecs->name = "snd-soc-dummy"; > link->codecs->dai_name = "snd-soc-dummy-dai"; > > - link->platforms->name = "snd-soc-dummy"; > - I think this patch is incorrect and should be reverted (I have received a note from a SOF developer). The link->cpus->dai_name is not necessarily "snd-soc-dummy", it is set earlier: if (strlen(pcm->dai_name)) { link->cpus->dai_name = devm_kstrdup(tplg->dev, pcm->dai_name, GFP_KERNEL); if (!link->cpus->dai_name) { ret = -ENOMEM; goto err; } } We cannot be sure that it is the same component, in fact it is most like not. > /* enable DPCM */ > link->dynamic = 1; > link->ignore_pmdown_time = 1;
Hi Peter Thank you for your feedback. > > @@ -1694,11 +1694,9 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > > > link->cpus = &dlc[0]; > > link->codecs = &dlc[1]; > > - link->platforms = &dlc[2]; > > > > link->num_cpus = 1; > > link->num_codecs = 1; > > - link->num_platforms = 1; > > > > link->dobj.index = tplg->index; > > link->dobj.type = SND_SOC_DOBJ_DAI_LINK; > > @@ -1726,8 +1724,6 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > link->codecs->name = "snd-soc-dummy"; > > link->codecs->dai_name = "snd-soc-dummy-dai"; > > > > - link->platforms->name = "snd-soc-dummy"; > > - > > I think this patch is incorrect and should be reverted (I have received > a note from a SOF developer). > The link->cpus->dai_name is not necessarily "snd-soc-dummy", it is set > earlier: > if (strlen(pcm->dai_name)) { > link->cpus->dai_name = devm_kstrdup(tplg->dev, pcm->dai_name, > GFP_KERNEL); > if (!link->cpus->dai_name) { > ret = -ENOMEM; > goto err; > } > } > > We cannot be sure that it is the same component, in fact it is most like > not. But sorry, I don't understand the point. This patch removed dummy PLATFORM which is not necessary I think, but you are talking about CPU. Could you please explain more detail what is the problem ? Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Morimoto-san, On 28/03/2023 02:20, Kuninori Morimoto wrote: > > Hi Peter > > Thank you for your feedback. > >>> @@ -1694,11 +1694,9 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, >>> >>> link->cpus = &dlc[0]; >>> link->codecs = &dlc[1]; >>> - link->platforms = &dlc[2]; >>> >>> link->num_cpus = 1; >>> link->num_codecs = 1; >>> - link->num_platforms = 1; >>> >>> link->dobj.index = tplg->index; >>> link->dobj.type = SND_SOC_DOBJ_DAI_LINK; >>> @@ -1726,8 +1724,6 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, >>> link->codecs->name = "snd-soc-dummy"; >>> link->codecs->dai_name = "snd-soc-dummy-dai"; >>> >>> - link->platforms->name = "snd-soc-dummy"; >>> - >> >> I think this patch is incorrect and should be reverted (I have received >> a note from a SOF developer). >> The link->cpus->dai_name is not necessarily "snd-soc-dummy", it is set >> earlier: >> if (strlen(pcm->dai_name)) { >> link->cpus->dai_name = devm_kstrdup(tplg->dev, pcm->dai_name, >> GFP_KERNEL); >> if (!link->cpus->dai_name) { >> ret = -ENOMEM; >> goto err; >> } >> } >> >> We cannot be sure that it is the same component, in fact it is most like >> not. > > But sorry, I don't understand the point. > This patch removed dummy PLATFORM which is not necessary I think, > but you are talking about CPU. The patch removes the dummy platform with a justification that if the CPU name and the platform name is identical, then the platform name does not need to be set. But in this case the CPU name and the platform name is _not_ identical, they are different and we need to have the dummy platform on the FE side unconditionally. > Could you please explain more detail what is the problem ? snd_sof:sof_dai_load: sof-audio-pci-intel-tgl 0000:00:1f.3: tplg: load pcm HDMI1 sof-audio-pci-intel-tgl 0000:00:1f.3: error: no platforms skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: FE link loading failed skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: topology: could not load header: -22 sof-audio-pci-intel-tgl 0000:00:1f.3: error: tplg component load failed -22 sof-audio-pci-intel-tgl 0000:00:1f.3: error: failed to load DSP topology -22 sof-audio-pci-intel-tgl 0000:00:1f.3: ASoC: error at snd_soc_component_probe on 0000:00:1f.3: -22 skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: failed to instantiate card -22 skl_hda_dsp_generic: probe of skl_hda_dsp_generic failed with error -22 which should look like this ideally: snd_sof:sof_dai_load: sof-audio-pci-intel-tgl 0000:00:1f.3: tplg: load pcm HDMI1 snd_sof:sof_dai_load: sof-audio-pci-intel-tgl 0000:00:1f.3: tplg: load pcm HDMI2 snd_sof:sof_dai_load: sof-audio-pci-intel-tgl 0000:00:1f.3: tplg: load pcm HDMI3 snd_sof:sof_dai_load: sof-audio-pci-intel-tgl 0000:00:1f.3: tplg: load pcm HDA Analog snd_sof:sof_dai_load: sof-audio-pci-intel-tgl 0000:00:1f.3: tplg: load pcm DeepBuffer > > > Thank you for your help !! > > Best regards > --- > Kuninori Morimoto
Hi Peter Thank you for your feedback > But in this case the CPU name and the platform name is _not_ identical, > they are different and we need to have the dummy platform on the FE side > unconditionally. (snip) > sof-audio-pci-intel-tgl 0000:00:1f.3: error: no platforms Oh, I didn't know that platform is needed on SOF. Thank you for explaining detail, now I could understand your issue. In my quick check, each SOF topology might overwrites link->platforms->name and/or link->platforms->of_node... I'm not 100% sure about SOF, but I looks strange for me... I'm not sure which one is better idea, soc-topology.c has dummy platforms and allow to be overwritten, or each topology alloc own platform if needed. But I will post the fixup/revert patch for it. Could you please check it ? Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 07421f5d4ebd..7f6424fa59ab 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1685,8 +1685,8 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, struct snd_soc_dai_link_component *dlc; int ret; - /* link + cpu + codec + platform */ - link = devm_kzalloc(tplg->dev, sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); + /* link + cpu + codec */ + link = devm_kzalloc(tplg->dev, sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); if (link == NULL) return -ENOMEM; @@ -1694,11 +1694,9 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, link->cpus = &dlc[0]; link->codecs = &dlc[1]; - link->platforms = &dlc[2]; link->num_cpus = 1; link->num_codecs = 1; - link->num_platforms = 1; link->dobj.index = tplg->index; link->dobj.type = SND_SOC_DOBJ_DAI_LINK; @@ -1726,8 +1724,6 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, link->codecs->name = "snd-soc-dummy"; link->codecs->dai_name = "snd-soc-dummy-dai"; - link->platforms->name = "snd-soc-dummy"; - /* enable DPCM */ link->dynamic = 1; link->ignore_pmdown_time = 1;
dai_link->platform is no longer needed if CPU and Platform are same Component. This patch removes unnecessary dai_link->platform. Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- sound/soc/soc-topology.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)