Fix handling of platform_component in sof_sdw#5383
Fix handling of platform_component in sof_sdw#5383charleskeepax wants to merge 2 commits intothesofproject:topic/sof-devfrom
Conversation
charleskeepax
requested review from
bardliao,
dbaluta,
kv2019i,
lgirdwood,
plbossart,
ranj063 and
ujfalusi
as code owners
| asoc_sdw_init_dai_link(dev, dai_links, be_id, name, playback, capture, | ||
| &dlc[0], 1, &dlc[1], num_platforms, | ||
| &dlc[2], 1, no_pcm, init, ops); | ||
| &dlc[0], 1, &dlc[1], 1, &dlc[2], 1, |
There was a problem hiding this comment.
@charleskeepax did you mean to remove num_platforms from this one too?
There was a problem hiding this comment.
While sound/soc/amd/acp/acp-sdw-sof-mach.c appears to use ARRAY_SIZE(platform_component), the platform_component there also limited to a single entry.
There was a problem hiding this comment.
Sorry struggling with the github, can't seem to figure out if these comments are showing in two places or if they are two different comments. But this is attempting to comment on the asoc_sdw_init_simple_dai_link part. Yeah doesn't make sense for asoc_sdw_init_simple_dai_link to set multiple platforms, the input string is "const char *platform_comp_name", which can only pass a single platform in to the function, and if you have multiple platforms its not really a simple dai you the full helper.
| asoc_sdw_init_dai_link(dev, dai_links, be_id, name, playback, capture, | ||
| &dlc[0], 1, &dlc[1], num_platforms, | ||
| &dlc[2], 1, no_pcm, init, ops); | ||
| &dlc[0], 1, &dlc[1], 1, &dlc[2], 1, |
There was a problem hiding this comment.
While sound/soc/amd/acp/acp-sdw-sof-mach.c appears to use ARRAY_SIZE(platform_component), the platform_component there also limited to a single entry.
sound/soc/intel/boards/sof_sdw.c
Outdated
| return -ENOMEM; | ||
|
|
||
| intel_ctx->platform_component = platform_component; | ||
|
|
There was a problem hiding this comment.
I don't think this is needed at all, the path via asoc_sdw_init_dai_link() ignores the name and just sets it.
There was a problem hiding this comment.
I don't think there is a set on that path it passes the full platform component in, on the path through create_sdw_dailinks. Could you point to where the set happens?
There was a problem hiding this comment.
Since something is modifying it (the whole point of this PR) I just removed this saving and audio probed and worked fine ;)
There was a problem hiding this comment.
Hmm... that is very strange, it should get cleared on tear down but I don't think it should be getting set on creation. I guess I will go dig further, hopefully I can find some time.
There was a problem hiding this comment.
diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 7413978bcf14..241faeb64a41 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1303,7 +1303,7 @@ static int mc_probe(struct platform_device *pdev)
if (!intel_ctx)
return -ENOMEM;
- intel_ctx->platform_component = platform_component;
+ intel_ctx->platform_component.name = "This is a nice day";
ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)and it still works :o
There was a problem hiding this comment.
Ok yeah it seems it comes from soc_check_tplg_fes in soc-core, not really sure what that function does but it seems to go in and monkey around with all the platforms kinda makes me wonder if there is any point setting any of the platform stuff from the machine driver.
| sof_sdw_quirk = quirk_entry->value; | ||
| } | ||
|
|
||
| static struct snd_soc_dai_link_component platform_component[] = { |
There was a problem hiding this comment.
Looks good @charleskeepax except for the typo in the commit title "don't both to set platform string"
There is no point in passing num_platforms into asoc_sdw_init_simple_dai_link(). Firstly, as a single pointer for the component name is passed in only a single string can be passed and secondly if it is a complex DAI with multiple platforms it would make more sense to use asoc_sdw_init_dai_link(). Fixes: 59f8b62 ("ASoC: intel/sdw_utils: refactor init_dai_link() and init_simple_dai_link()") Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
The static platform_component name string is overwritten on card tear down which will cause a NULL check on it to fail when the driver is reprobed. However, it turns out that the ASoC core sets this string for all topology systems anyway (after the aforementioned NULL check). So there is no need for the machine driver to set the platform at all, replace all the platform_component stuff with some simple place holders. Fixes: 52db12d ("ASoC: Intel: boards: add sof_sdw machine driver") Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
|
Anything else we need on these? Do you want me to upstream them? |
@charleskeepax yes, please submit the patches upstream. Thanks! |
@charleskeepax Please go ahead to upstream them. I will close this PR. |
4 participants
No description provided.