[PATCH 00/11] Improve SOF support for Steam Deck OLED

[PATCH 00/11] Improve SOF support for Steam Deck OLED

[Cristian Ciocaltea]

This patch series is a continuation of [1] to provide fixes and improvements to
SOF drivers targeting the Vangogh platform, as found on the Valve's Steam Deck
OLED.

The previous series only handled the legacy ACP drivers.

[1]: https://lore.kernel.org/all/20231209203229.878730-1-cristian.ciocaltea@collabora.com/

Cristian Ciocaltea (11):
  ASoC: amd: acp: Drop redundant initialization of machine driver data
  ASoC: amd: acp: Make use of existing *_CODEC_DAI macros
  ASoC: amd: acp: Add missing error handling in sof-mach
  ASoC: amd: acp: Update MODULE_DESCRIPTION for sof-mach
  ASoC: SOF: amd: Fix memory leak in amd_sof_acp_probe()
  ASoC: SOF: amd: Optimize quirk for Valve Galileo
  ASoC: SOF: core: Skip firmware test for undefined fw_name
  ASoC: SOF: amd: Override default fw name for Valve Galileo
  ASoC: SOF: amd: Compute file paths on firmware load
  ASoC: amd: acp: Use correct DAI link ID for BT codec
  ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

 sound/soc/amd/acp/acp-mach-common.c |  6 +++---
 sound/soc/amd/acp/acp-mach.h        |  2 +-
 sound/soc/amd/acp/acp-sof-mach.c    | 26 +++++++----------------
 sound/soc/sof/amd/acp-loader.c      | 32 +++++++++++++++++++++++------
 sound/soc/sof/amd/acp.c             | 30 ++++++++++++++++-----------
 sound/soc/sof/amd/vangogh.c         |  8 +++++++-
 sound/soc/sof/fw-file-profile.c     |  3 +++
 sound/soc/sof/topology.c            |  1 +
 8 files changed, 66 insertions(+), 42 deletions(-)

-- 
2.43.0

[PATCH 01/11] ASoC: amd: acp: Drop redundant initialization of machine driver data

[Cristian Ciocaltea]

Simplify driver data configuration by removing redundant initialization
of members in static structs.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/amd/acp/acp-sof-mach.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c
index 2a9fd3275e42..1d313fcb5f2d 100644
--- a/sound/soc/amd/acp/acp-sof-mach.c
+++ b/sound/soc/amd/acp/acp-sof-mach.c
@@ -28,7 +28,6 @@ static struct acp_card_drvdata sof_rt5682_rt1019_data = {
 	.hs_codec_id = RT5682,
 	.amp_codec_id = RT1019,
 	.dmic_codec_id = DMIC,
-	.tdm_mode = false,
 };
 
 static struct acp_card_drvdata sof_rt5682_max_data = {
@@ -38,7 +37,6 @@ static struct acp_card_drvdata sof_rt5682_max_data = {
 	.hs_codec_id = RT5682,
 	.amp_codec_id = MAX98360A,
 	.dmic_codec_id = DMIC,
-	.tdm_mode = false,
 };
 
 static struct acp_card_drvdata sof_rt5682s_rt1019_data = {
@@ -48,7 +46,6 @@ static struct acp_card_drvdata sof_rt5682s_rt1019_data = {
 	.hs_codec_id = RT5682S,
 	.amp_codec_id = RT1019,
 	.dmic_codec_id = DMIC,
-	.tdm_mode = false,
 };
 
 static struct acp_card_drvdata sof_rt5682s_max_data = {
@@ -58,7 +55,6 @@ static struct acp_card_drvdata sof_rt5682s_max_data = {
 	.hs_codec_id = RT5682S,
 	.amp_codec_id = MAX98360A,
 	.dmic_codec_id = DMIC,
-	.tdm_mode = false,
 };
 
 static struct acp_card_drvdata sof_nau8825_data = {
@@ -69,7 +65,6 @@ static struct acp_card_drvdata sof_nau8825_data = {
 	.amp_codec_id = MAX98360A,
 	.dmic_codec_id = DMIC,
 	.soc_mclk = true,
-	.tdm_mode = false,
 };
 
 static struct acp_card_drvdata sof_rt5682s_hs_rt1019_data = {
@@ -80,20 +75,15 @@ static struct acp_card_drvdata sof_rt5682s_hs_rt1019_data = {
 	.amp_codec_id = RT1019,
 	.dmic_codec_id = DMIC,
 	.soc_mclk = true,
-	.tdm_mode = false,
 };
 
 static struct acp_card_drvdata sof_nau8821_max98388_data = {
 	.hs_cpu_id = I2S_SP,
 	.amp_cpu_id = I2S_HS,
 	.bt_cpu_id = I2S_BT,
-	.dmic_cpu_id = NONE,
 	.hs_codec_id = NAU8821,
 	.amp_codec_id = MAX98388,
-	.bt_codec_id = NONE,
-	.dmic_codec_id = NONE,
 	.soc_mclk = true,
-	.tdm_mode = false,
 };
 
 static int acp_sof_probe(struct platform_device *pdev)
-- 
2.43.0

[PATCH 02/11] ASoC: amd: acp: Make use of existing *_CODEC_DAI macros

[Cristian Ciocaltea]

The generic ACP machine driver provides macros for NAU88221 and MAX98388
codec DAI names, but in places it is still using directly the related
strings.

For consistency, replace all strings with the equivalent macros.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/amd/acp/acp-mach-common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/amd/acp/acp-mach-common.c b/sound/soc/amd/acp/acp-mach-common.c
index c90ec3419247..346f7514c81a 100644
--- a/sound/soc/amd/acp/acp-mach-common.c
+++ b/sound/soc/amd/acp/acp-mach-common.c
@@ -821,8 +821,8 @@ static const struct snd_soc_ops acp_card_maxim_ops = {
 };
 
 SND_SOC_DAILINK_DEF(max98388,
-		    DAILINK_COMP_ARRAY(COMP_CODEC("i2c-ADS8388:00", "max98388-aif1"),
-				       COMP_CODEC("i2c-ADS8388:01", "max98388-aif1")));
+		    DAILINK_COMP_ARRAY(COMP_CODEC("i2c-ADS8388:00", MAX98388_CODEC_DAI),
+				       COMP_CODEC("i2c-ADS8388:01", MAX98388_CODEC_DAI)));
 
 static const struct snd_kcontrol_new max98388_controls[] = {
 	SOC_DAPM_PIN_SWITCH("Left Spk"),
@@ -1273,7 +1273,7 @@ static const struct snd_soc_ops acp_8821_ops = {
 
 SND_SOC_DAILINK_DEF(nau8821,
 		    DAILINK_COMP_ARRAY(COMP_CODEC("i2c-NVTN2020:00",
-						  "nau8821-hifi")));
+						  NAU8821_CODEC_DAI)));
 
 /* Declare DMIC codec components */
 SND_SOC_DAILINK_DEF(dmic_codec,
-- 
2.43.0

[PATCH 03/11] ASoC: amd: acp: Add missing error handling in sof-mach

[Cristian Ciocaltea]

Handle potential acp_sofdsp_dai_links_create() errors in ACP SOF machine
driver's probe function.  Additionally, switch to dev_err_probe().

Fixes: 9f84940f5004 ("ASoC: amd: acp: Add SOF audio support on Chrome board")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/amd/acp/acp-sof-mach.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c
index 1d313fcb5f2d..6f0ca23638af 100644
--- a/sound/soc/amd/acp/acp-sof-mach.c
+++ b/sound/soc/amd/acp/acp-sof-mach.c
@@ -112,16 +112,14 @@ static int acp_sof_probe(struct platform_device *pdev)
 	if (dmi_id && dmi_id->driver_data)
 		acp_card_drvdata->tdm_mode = dmi_id->driver_data;
 
-	acp_sofdsp_dai_links_create(card);
+	ret = acp_sofdsp_dai_links_create(card);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to create DAI links\n");
 
 	ret = devm_snd_soc_register_card(&pdev->dev, card);
-	if (ret) {
-		dev_err(&pdev->dev,
-				"devm_snd_soc_register_card(%s) failed: %d\n",
-				card->name, ret);
-		return ret;
-	}
-
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "Failed to register card(%s)\n", card->name);
 	return 0;
 }
 
-- 
2.43.0

[PATCH 04/11] ASoC: amd: acp: Update MODULE_DESCRIPTION for sof-mach

[Cristian Ciocaltea]

The current MODULE_DESCRIPTION relates to a Chrome board, as that was
what the driver initially supported.

Nonetheless, it has since progressed incrementally and evolved into a
more comprehensive machine driver. Hence, update MODULE_DESCRIPTION to
better reflect this.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/amd/acp/acp-sof-mach.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c
index 6f0ca23638af..19ff4fe5b1ea 100644
--- a/sound/soc/amd/acp/acp-sof-mach.c
+++ b/sound/soc/amd/acp/acp-sof-mach.c
@@ -166,7 +166,7 @@ static struct platform_driver acp_asoc_audio = {
 module_platform_driver(acp_asoc_audio);
 
 MODULE_IMPORT_NS(SND_SOC_AMD_MACH);
-MODULE_DESCRIPTION("ACP chrome SOF audio support");
+MODULE_DESCRIPTION("ACP SOF Machine Driver");
 MODULE_ALIAS("platform:rt5682-rt1019");
 MODULE_ALIAS("platform:rt5682-max");
 MODULE_ALIAS("platform:rt5682s-max");
-- 
2.43.0

[PATCH 05/11] ASoC: SOF: amd: Fix memory leak in amd_sof_acp_probe()

[Cristian Ciocaltea]

Driver uses kasprintf() to initialize fw_{code,data}_bin members of
struct acp_dev_data, but kfree() is never called to deallocate the
memory, which results in a memory leak.

Fix the issue by switching to devm_kasprintf(). Additionally, ensure the
allocation was successful by checking the pointer validity.

Fixes: f7da88003c53 ("ASoC: SOF: amd: Enable signed firmware image loading for Vangogh platform")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/amd/acp.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
index 603ea5fc0d0d..c6f637f29847 100644
--- a/sound/soc/sof/amd/acp.c
+++ b/sound/soc/sof/amd/acp.c
@@ -547,17 +547,27 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
 	adata->signed_fw_image = false;
 	dmi_id = dmi_first_match(acp_sof_quirk_table);
 	if (dmi_id && dmi_id->driver_data) {
-		adata->fw_code_bin = kasprintf(GFP_KERNEL, "%s/sof-%s-code.bin",
-					       plat_data->fw_filename_prefix,
-					       chip->name);
-		adata->fw_data_bin = kasprintf(GFP_KERNEL, "%s/sof-%s-data.bin",
-					       plat_data->fw_filename_prefix,
-					       chip->name);
-		adata->signed_fw_image = dmi_id->driver_data;
+		adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
+						    "%s/sof-%s-code.bin",
+						    plat_data->fw_filename_prefix,
+						    chip->name);
+		if (!adata->fw_code_bin) {
+			ret = -ENOMEM;
+			goto free_ipc_irq;
+		}
+
+		adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
+						    "%s/sof-%s-data.bin",
+						    plat_data->fw_filename_prefix,
+						    chip->name);
+		if (!adata->fw_data_bin) {
+			ret = -ENOMEM;
+			goto free_ipc_irq;
+		}
 
-		dev_dbg(sdev->dev, "fw_code_bin:%s, fw_data_bin:%s\n", adata->fw_code_bin,
-			adata->fw_data_bin);
+		adata->signed_fw_image = dmi_id->driver_data;
 	}
+
 	adata->enable_fw_debug = enable_fw_debug;
 	acp_memory_init(sdev);
 
-- 
2.43.0

[PATCH 06/11] ASoC: SOF: amd: Optimize quirk for Valve Galileo

[Cristian Ciocaltea]

Valve's Steam Deck OLED is uniquely identified by vendor and product
name (Galileo) DMI fields.

Simplify the quirk by removing the unnecessary match on product family.

Additionally, fix the related comment as it points to the old product
variant.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/amd/acp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
index c6f637f29847..1e9840ae8938 100644
--- a/sound/soc/sof/amd/acp.c
+++ b/sound/soc/sof/amd/acp.c
@@ -28,11 +28,10 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug");
 
 const struct dmi_system_id acp_sof_quirk_table[] = {
 	{
-		/* Valve Jupiter device */
+		/* Steam Deck OLED device */
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Valve"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"),
-			DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"),
 		},
 		.driver_data = (void *)SECURED_FIRMWARE,
 	},
-- 
2.43.0

[PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Cristian Ciocaltea]

Some SOF drivers like AMD ACP do not always rely on a single static
firmware file, but may require multiple files having their names
dynamically computed on probe time, e.g. based on chip name.

In those cases, providing an invalid default_fw_filename in their
sof_dev_desc struct will prevent probing due to 'SOF firmware
and/or topology file not found' error.

Fix the issue by allowing drivers to omit initialization for this member
(or alternatively provide a dynamic override via ipc_file_profile_base)
and update sof_test_firmware_file() to verify the given profile data and
skip firmware testing if either fw_path or fw_name is not defined.

Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/fw-file-profile.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
index 138a1ca2c4a8..e63700234df0 100644
--- a/sound/soc/sof/fw-file-profile.c
+++ b/sound/soc/sof/fw-file-profile.c
@@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev,
 	const u32 *magic;
 	int ret;
 
+	if (!profile->fw_path || !profile->fw_name || !*profile->fw_name)
+		return 0;
+
 	fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
 				profile->fw_name);
 	if (!fw_filename)
-- 
2.43.0

[PATCH 08/11] ASoC: SOF: amd: Override default fw name for Valve Galileo

[Cristian Ciocaltea]

The ACP driver for Vangogh platform uses a quirk for Valve Galileo
device to setup a custom firmware loader, which neither requires nor
uses the firmware file indicated via the default_fw_filename member of
struct sof_dev_desc.

Since commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version
fallback if firmware files are missing"), the provided filename gets
verified and triggers a fatal error on probe:

[ 7.719337] snd_sof_amd_vangogh 0000:04:00.5: enabling device (0000 -> 0002)
[ 7.721486] snd_sof_amd_vangogh 0000:04:00.5: SOF firmware and/or topology file not found.
[ 7.721565] snd_sof_amd_vangogh 0000:04:00.5: Supported default profiles
[ 7.721569] snd_sof_amd_vangogh 0000:04:00.5: - ipc type 0 (Requested):
[ 7.721573] snd_sof_amd_vangogh 0000:04:00.5:  Firmware file: amd/sof/sof-vangogh.ri
[ 7.721577] snd_sof_amd_vangogh 0000:04:00.5:  Topology file: amd/sof-tplg/sof-vangogh-nau8821-max.tplg
[ 7.721582] snd_sof_amd_vangogh 0000:04:00.5: Check if you have 'sof-firmware' package installed.
[ 7.721585] snd_sof_amd_vangogh 0000:04:00.5: Optionally it can be manually downloaded from:
[ 7.721589] snd_sof_amd_vangogh 0000:04:00.5:    https://github.com/thesofproject/sof-bin/
[ 7.721997] snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2

Skip testing the default firmware by overriding fw_name in
sof_vangogh_ops_init().

Fixes: d0dab6b76a9f ("ASoC: SOF: amd: Add sof support for vangogh platform")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/amd/vangogh.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c
index de15d21aa6d9..5843ff8a8b40 100644
--- a/sound/soc/sof/amd/vangogh.c
+++ b/sound/soc/sof/amd/vangogh.c
@@ -151,8 +151,14 @@ int sof_vangogh_ops_init(struct snd_sof_dev *sdev)
 	sof_vangogh_ops.num_drv = ARRAY_SIZE(vangogh_sof_dai);
 
 	dmi_id = dmi_first_match(acp_sof_quirk_table);
-	if (dmi_id && dmi_id->driver_data)
+	if (dmi_id && dmi_id->driver_data) {
 		sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware;
+		/*
+		 * Board doesn't use the default firmware, hence override
+		 * its name to prevent probe error due to fw validation.
+		 */
+		sdev->pdata->ipc_file_profile_base.fw_name = "";
+	}
 
 	return 0;
 }
-- 
2.43.0

[PATCH 09/11] ASoC: SOF: amd: Compute file paths on firmware load

[Cristian Ciocaltea]

Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if
firmware files are missing") changed the order of some operations and
the firmware paths are not available anymore at snd_sof_probe() time.

Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via

  plat_data->fw_filename_prefix = out_profile.fw_path;

but sof_init_environment() which calls this function was moved from
snd_sof_device_probe() to sof_probe_continue(). Moreover,
snd_sof_probe() was moved from sof_probe_continue() to
sof_init_environment(), but before the call to
sof_select_ipc_and_paths().

The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to
compute fw_code_bin and fw_data_bin paths, and because the field is not
yet initialized, the paths end up containing (null):

snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for (null)/sof-vangogh-code.bin failed with error -2
snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing
snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2
snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2

Move usage of fw_filename_prefix right before request_firmware() calls
in acp_sof_load_signed_firmware().

Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------
 sound/soc/sof/amd/acp.c        |  7 ++-----
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c
index e05eb7a86dd4..d2d21478399e 100644
--- a/sound/soc/sof/amd/acp-loader.c
+++ b/sound/soc/sof/amd/acp-loader.c
@@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct snd_sof_dev *sdev)
 {
 	struct snd_sof_pdata *plat_data = sdev->pdata;
 	struct acp_dev_data *adata = plat_data->hw_pdata;
+	const char *fw_filename;
 	int ret;
 
-	ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin, sdev->dev);
+	fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
+				plat_data->fw_filename_prefix,
+				adata->fw_code_bin);
+	if (!fw_filename)
+		return -ENOMEM;
+
+	ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev);
 	if (ret < 0) {
+		kfree(fw_filename);
 		dev_err(sdev->dev, "sof signed firmware code bin is missing\n");
 		return ret;
 	} else {
-		dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_code_bin);
+		dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename);
 	}
+	kfree(fw_filename);
+
 	ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0,
-				      (void *)sdev->basefw.fw->data, sdev->basefw.fw->size);
+				      (void *)sdev->basefw.fw->data,
+				      sdev->basefw.fw->size);
+
+	fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
+				plat_data->fw_filename_prefix,
+				adata->fw_data_bin);
+	if (!fw_filename)
+		return -ENOMEM;
 
-	ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin, sdev->dev);
+	ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev);
 	if (ret < 0) {
+		kfree(fw_filename);
 		dev_err(sdev->dev, "sof signed firmware data bin is missing\n");
 		return ret;
 
 	} else {
-		dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_data_bin);
+		dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename);
 	}
+	kfree(fw_filename);
 
 	ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0,
-				      (void *)adata->fw_dbin->data, adata->fw_dbin->size);
+				      (void *)adata->fw_dbin->data,
+				      adata->fw_dbin->size);
 	return ret;
 }
 EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON);
diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
index 1e9840ae8938..87c5c71eac68 100644
--- a/sound/soc/sof/amd/acp.c
+++ b/sound/soc/sof/amd/acp.c
@@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume, SND_SOC_SOF_AMD_COMMON);
 int amd_sof_acp_probe(struct snd_sof_dev *sdev)
 {
 	struct pci_dev *pci = to_pci_dev(sdev->dev);
-	struct snd_sof_pdata *plat_data = sdev->pdata;
 	struct acp_dev_data *adata;
 	const struct sof_amd_acp_desc *chip;
 	const struct dmi_system_id *dmi_id;
@@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
 	dmi_id = dmi_first_match(acp_sof_quirk_table);
 	if (dmi_id && dmi_id->driver_data) {
 		adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
-						    "%s/sof-%s-code.bin",
-						    plat_data->fw_filename_prefix,
+						    "sof-%s-code.bin",
 						    chip->name);
 		if (!adata->fw_code_bin) {
 			ret = -ENOMEM;
@@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
 		}
 
 		adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
-						    "%s/sof-%s-data.bin",
-						    plat_data->fw_filename_prefix,
+						    "sof-%s-data.bin",
 						    chip->name);
 		if (!adata->fw_data_bin) {
 			ret = -ENOMEM;
-- 
2.43.0

[PATCH 10/11] ASoC: amd: acp: Use correct DAI link ID for BT codec

[Cristian Ciocaltea]

Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
creation for I2S BT instance") added I2S BT support in ACP common
machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:

[ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d
[ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0
[ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0
[ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist
[ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22
[ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22
[ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22
[ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22
[ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
[ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max)
[ 8.545022] sof_mach: probe of nau8821-max failed with error -22

Move BT_BE_ID to the correct position in the enum.

Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/amd/acp/acp-mach.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
index a48546d8d407..0c18ccd29305 100644
--- a/sound/soc/amd/acp/acp-mach.h
+++ b/sound/soc/amd/acp/acp-mach.h
@@ -27,8 +27,8 @@
 enum be_id {
 	HEADSET_BE_ID = 0,
 	AMP_BE_ID,
-	DMIC_BE_ID,
 	BT_BE_ID,
+	DMIC_BE_ID,
 };
 
 enum cpu_endpoints {
-- 
2.43.0

[PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Cristian Ciocaltea]

Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type.  However, some
boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:

[  467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22
[  467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed
[  467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed
[  467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22
[  467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22
[  467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22
[  467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22
[  467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max)
[  467.514861] sof_mach: probe of nau8821-max failed with error -22

Add "ACPBT" alias for "ACP" SOF DAI type.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/sof/topology.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index e3e7fbe40fa6..73bf791e7520 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
 	{"SAI", SOF_DAI_IMX_SAI},
 	{"ESAI", SOF_DAI_IMX_ESAI},
 	{"ACP", SOF_DAI_AMD_BT},
+	{"ACPBT", SOF_DAI_AMD_BT},
 	{"ACPSP", SOF_DAI_AMD_SP},
 	{"ACPDMIC", SOF_DAI_AMD_DMIC},
 	{"ACPHS", SOF_DAI_AMD_HS},
-- 
2.43.0

Re: [PATCH 10/11] ASoC: amd: acp: Use correct DAI link ID for BT codec

[Venkata Prasad Potturu]

On 12/10/23 02:23, Cristian Ciocaltea wrote:
> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
> creation for I2S BT instance") added I2S BT support in ACP common
> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>
> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d
> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0
> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0
> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist
> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22
> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22
> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22
> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22
> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max)
> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>
> Move BT_BE_ID to the correct position in the enum.
>
> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>   sound/soc/amd/acp/acp-mach.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
> index a48546d8d407..0c18ccd29305 100644
> --- a/sound/soc/amd/acp/acp-mach.h
> +++ b/sound/soc/amd/acp/acp-mach.h
> @@ -27,8 +27,8 @@
>   enum be_id {
>   	HEADSET_BE_ID = 0,
>   	AMP_BE_ID,
> -	DMIC_BE_ID,
>   	BT_BE_ID,
> +	DMIC_BE_ID,
This will break the other platforms as this same enum used in topology 
to create dailink.
>   };
>   
>   enum cpu_endpoints {

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Venkata Prasad Potturu]

On 12/10/23 02:23, Cristian Ciocaltea wrote:
> Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
> DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type.  However, some
> boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
>
> [  467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22
> [  467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed
> [  467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed
> [  467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22
> [  467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22
> [  467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22
> [  467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22
> [  467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max)
> [  467.514861] sof_mach: probe of nau8821-max failed with error -22
>
> Add "ACPBT" alias for "ACP" SOF DAI type.
>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>   sound/soc/sof/topology.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
> index e3e7fbe40fa6..73bf791e7520 100644
> --- a/sound/soc/sof/topology.c
> +++ b/sound/soc/sof/topology.c
> @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
>   	{"SAI", SOF_DAI_IMX_SAI},
>   	{"ESAI", SOF_DAI_IMX_ESAI},
>   	{"ACP", SOF_DAI_AMD_BT},
> +	{"ACPBT", SOF_DAI_AMD_BT},
No need to create the alias name, we can directly modify ACP to ACPBT as 
ACP is not using anywhere.
>   	{"ACPSP", SOF_DAI_AMD_SP},
>   	{"ACPDMIC", SOF_DAI_AMD_DMIC},
>   	{"ACPHS", SOF_DAI_AMD_HS},

Re: [PATCH 06/11] ASoC: SOF: amd: Optimize quirk for Valve Galileo

[Venkata Prasad Potturu]

On 12/10/23 02:23, Cristian Ciocaltea wrote:
> Valve's Steam Deck OLED is uniquely identified by vendor and product
> name (Galileo) DMI fields.
>
> Simplify the quirk by removing the unnecessary match on product family.
>
> Additionally, fix the related comment as it points to the old product
> variant.
>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>   sound/soc/sof/amd/acp.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
> index c6f637f29847..1e9840ae8938 100644
> --- a/sound/soc/sof/amd/acp.c
> +++ b/sound/soc/sof/amd/acp.c
> @@ -28,11 +28,10 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug");
>   
>   const struct dmi_system_id acp_sof_quirk_table[] = {
>   	{
> -		/* Valve Jupiter device */
> +		/* Steam Deck OLED device */
If any changes in SOF drivers, first need to create a PR in SOF github.
>   		.matches = {
>   			DMI_MATCH(DMI_SYS_VENDOR, "Valve"),
>   			DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"),
> -			DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"),
>   		},
>   		.driver_data = (void *)SECURED_FIRMWARE,
>   	},

Re: [PATCH 09/11] ASoC: SOF: amd: Compute file paths on firmware load

[Venkata Prasad Potturu]

On 12/10/23 02:23, Cristian Ciocaltea wrote:
> Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if
> firmware files are missing") changed the order of some operations and
> the firmware paths are not available anymore at snd_sof_probe() time.
>
> Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via
>
>    plat_data->fw_filename_prefix = out_profile.fw_path;
>
> but sof_init_environment() which calls this function was moved from
> snd_sof_device_probe() to sof_probe_continue(). Moreover,
> snd_sof_probe() was moved from sof_probe_continue() to
> sof_init_environment(), but before the call to
> sof_select_ipc_and_paths().
>
> The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to
> compute fw_code_bin and fw_data_bin paths, and because the field is not
> yet initialized, the paths end up containing (null):
>
> snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for (null)/sof-vangogh-code.bin failed with error -2
> snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing
> snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2
> snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
>
> Move usage of fw_filename_prefix right before request_firmware() calls
> in acp_sof_load_signed_firmware().
>
> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>   sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------
>   sound/soc/sof/amd/acp.c        |  7 ++-----
>   2 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c
> index e05eb7a86dd4..d2d21478399e 100644
> --- a/sound/soc/sof/amd/acp-loader.c
> +++ b/sound/soc/sof/amd/acp-loader.c
> @@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct snd_sof_dev *sdev)
>   {
>   	struct snd_sof_pdata *plat_data = sdev->pdata;
>   	struct acp_dev_data *adata = plat_data->hw_pdata;
> +	const char *fw_filename;
>   	int ret;
>   
> -	ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin, sdev->dev);
> +	fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
> +				plat_data->fw_filename_prefix,
> +				adata->fw_code_bin);
File path already saved in adata->fw_code_bin in amd_sof_acp_probe function.
No need to get it again.
> +	if (!fw_filename)
> +		return -ENOMEM;
> +
> +	ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev);
>   	if (ret < 0) {
> +		kfree(fw_filename);
>   		dev_err(sdev->dev, "sof signed firmware code bin is missing\n");
>   		return ret;
>   	} else {
> -		dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_code_bin);
> +		dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename);
>   	}
> +	kfree(fw_filename);
> +
>   	ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0,
> -				      (void *)sdev->basefw.fw->data, sdev->basefw.fw->size);
> +				      (void *)sdev->basefw.fw->data,
> +				      sdev->basefw.fw->size);
> +
> +	fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
> +				plat_data->fw_filename_prefix,
> +				adata->fw_data_bin);
> +	if (!fw_filename)
> +		return -ENOMEM;
>   
> -	ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin, sdev->dev);
> +	ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev);
>   	if (ret < 0) {
> +		kfree(fw_filename);
>   		dev_err(sdev->dev, "sof signed firmware data bin is missing\n");
>   		return ret;
>   
>   	} else {
> -		dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_data_bin);
> +		dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename);
>   	}
> +	kfree(fw_filename);
>   
>   	ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0,
> -				      (void *)adata->fw_dbin->data, adata->fw_dbin->size);
> +				      (void *)adata->fw_dbin->data,
> +				      adata->fw_dbin->size);
>   	return ret;
>   }
>   EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON);
> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
> index 1e9840ae8938..87c5c71eac68 100644
> --- a/sound/soc/sof/amd/acp.c
> +++ b/sound/soc/sof/amd/acp.c
> @@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume, SND_SOC_SOF_AMD_COMMON);
>   int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>   {
>   	struct pci_dev *pci = to_pci_dev(sdev->dev);
> -	struct snd_sof_pdata *plat_data = sdev->pdata;
>   	struct acp_dev_data *adata;
>   	const struct sof_amd_acp_desc *chip;
>   	const struct dmi_system_id *dmi_id;
> @@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>   	dmi_id = dmi_first_match(acp_sof_quirk_table);
>   	if (dmi_id && dmi_id->driver_data) {
>   		adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
> -						    "%s/sof-%s-code.bin",
> -						    plat_data->fw_filename_prefix,
> +						    "sof-%s-code.bin",
>   						    chip->name);
>   		if (!adata->fw_code_bin) {
>   			ret = -ENOMEM;
> @@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>   		}
>   
>   		adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
> -						    "%s/sof-%s-data.bin",
> -						    plat_data->fw_filename_prefix,
> +						    "sof-%s-data.bin",
>   						    chip->name);
>   		if (!adata->fw_data_bin) {
>   			ret = -ENOMEM;

Re: [PATCH 06/11] ASoC: SOF: amd: Optimize quirk for Valve Galileo

[Cristian Ciocaltea]

On 12/10/23 05:33, Venkata Prasad Potturu wrote:
> 
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> Valve's Steam Deck OLED is uniquely identified by vendor and product
>> name (Galileo) DMI fields.
>>
>> Simplify the quirk by removing the unnecessary match on product family.
>>
>> Additionally, fix the related comment as it points to the old product
>> variant.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   sound/soc/sof/amd/acp.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
>> index c6f637f29847..1e9840ae8938 100644
>> --- a/sound/soc/sof/amd/acp.c
>> +++ b/sound/soc/sof/amd/acp.c
>> @@ -28,11 +28,10 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware
>> debug");
>>     const struct dmi_system_id acp_sof_quirk_table[] = {
>>       {
>> -        /* Valve Jupiter device */
>> +        /* Steam Deck OLED device */
> If any changes in SOF drivers, first need to create a PR in SOF github.

This is just an optimization for the driver, no need to change anything
on the firmware side.  The product family remains as is, but it's not
really required to match the board, i.e. the previous board was
"Jupiter", the next one will have a different product name.

>>           .matches = {
>>               DMI_MATCH(DMI_SYS_VENDOR, "Valve"),
>>               DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"),
>> -            DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"),
>>           },
>>           .driver_data = (void *)SECURED_FIRMWARE,
>>       },

Re: [PATCH 09/11] ASoC: SOF: amd: Compute file paths on firmware load

[Cristian Ciocaltea]

On 12/10/23 05:50, Venkata Prasad Potturu wrote:
> 
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if
>> firmware files are missing") changed the order of some operations and
>> the firmware paths are not available anymore at snd_sof_probe() time.
>>
>> Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via
>>
>>    plat_data->fw_filename_prefix = out_profile.fw_path;
>>
>> but sof_init_environment() which calls this function was moved from
>> snd_sof_device_probe() to sof_probe_continue(). Moreover,
>> snd_sof_probe() was moved from sof_probe_continue() to
>> sof_init_environment(), but before the call to
>> sof_select_ipc_and_paths().
>>
>> The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to
>> compute fw_code_bin and fw_data_bin paths, and because the field is not
>> yet initialized, the paths end up containing (null):
>>
>> snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for
>> (null)/sof-vangogh-code.bin failed with error -2
>> snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing
>> snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2
>> snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
>>
>> Move usage of fw_filename_prefix right before request_firmware() calls
>> in acp_sof_load_signed_firmware().
>>
>> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback
>> if firmware files are missing")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------
>>   sound/soc/sof/amd/acp.c        |  7 ++-----
>>   2 files changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/sound/soc/sof/amd/acp-loader.c
>> b/sound/soc/sof/amd/acp-loader.c
>> index e05eb7a86dd4..d2d21478399e 100644
>> --- a/sound/soc/sof/amd/acp-loader.c
>> +++ b/sound/soc/sof/amd/acp-loader.c
>> @@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct
>> snd_sof_dev *sdev)
>>   {
>>       struct snd_sof_pdata *plat_data = sdev->pdata;
>>       struct acp_dev_data *adata = plat_data->hw_pdata;
>> +    const char *fw_filename;
>>       int ret;
>>   -    ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin,
>> sdev->dev);
>> +    fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
>> +                plat_data->fw_filename_prefix,
>> +                adata->fw_code_bin);
> File path already saved in adata->fw_code_bin in amd_sof_acp_probe
> function.
> No need to get it again.

As already stated in the patch description, fw_filename_prefix is not
available anymore when amd_sof_acp_probe() gets invoked, and that is the
root cause of ending up with (null) in the computed file path.

Hence, the patch ensures amd_sof_acp_probe() computes the file name,
without prefix, while acp_sof_load_signed_firmware() adds the prefix.

>> +    if (!fw_filename)
>> +        return -ENOMEM;
>> +
>> +    ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev);
>>       if (ret < 0) {
>> +        kfree(fw_filename);
>>           dev_err(sdev->dev, "sof signed firmware code bin is
>> missing\n");
>>           return ret;
>>       } else {
>> -        dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> adata->fw_code_bin);
>> +        dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> fw_filename);
>>       }
>> +    kfree(fw_filename);
>> +
>>       ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0,
>> -                      (void *)sdev->basefw.fw->data,
>> sdev->basefw.fw->size);
>> +                      (void *)sdev->basefw.fw->data,
>> +                      sdev->basefw.fw->size);
>> +
>> +    fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
>> +                plat_data->fw_filename_prefix,
>> +                adata->fw_data_bin);
>> +    if (!fw_filename)
>> +        return -ENOMEM;
>>   -    ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin,
>> sdev->dev);
>> +    ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev);
>>       if (ret < 0) {
>> +        kfree(fw_filename);
>>           dev_err(sdev->dev, "sof signed firmware data bin is
>> missing\n");
>>           return ret;
>>         } else {
>> -        dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> adata->fw_data_bin);
>> +        dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> fw_filename);
>>       }
>> +    kfree(fw_filename);
>>         ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0,
>> -                      (void *)adata->fw_dbin->data,
>> adata->fw_dbin->size);
>> +                      (void *)adata->fw_dbin->data,
>> +                      adata->fw_dbin->size);
>>       return ret;
>>   }
>>   EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON);
>> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
>> index 1e9840ae8938..87c5c71eac68 100644
>> --- a/sound/soc/sof/amd/acp.c
>> +++ b/sound/soc/sof/amd/acp.c
>> @@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume,
>> SND_SOC_SOF_AMD_COMMON);
>>   int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>>   {
>>       struct pci_dev *pci = to_pci_dev(sdev->dev);
>> -    struct snd_sof_pdata *plat_data = sdev->pdata;
>>       struct acp_dev_data *adata;
>>       const struct sof_amd_acp_desc *chip;
>>       const struct dmi_system_id *dmi_id;
>> @@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>>       dmi_id = dmi_first_match(acp_sof_quirk_table);
>>       if (dmi_id && dmi_id->driver_data) {
>>           adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
>> -                            "%s/sof-%s-code.bin",
>> -                            plat_data->fw_filename_prefix,
>> +                            "sof-%s-code.bin",
>>                               chip->name);
>>           if (!adata->fw_code_bin) {
>>               ret = -ENOMEM;
>> @@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>>           }
>>             adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
>> -                            "%s/sof-%s-data.bin",
>> -                            plat_data->fw_filename_prefix,
>> +                            "sof-%s-data.bin",
>>                               chip->name);
>>           if (!adata->fw_data_bin) {
>>               ret = -ENOMEM;

Re: [PATCH 10/11] ASoC: amd: acp: Use correct DAI link ID for BT codec

[Cristian Ciocaltea]

On 12/10/23 05:24, Venkata Prasad Potturu wrote:
> 
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>> creation for I2S BT instance") added I2S BT support in ACP common
>> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>>
>> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version
>> 0:0:0-7863d
>> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0
>> Kernel ABI 3:23:0
>> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0
>> Kernel ABI 3:23:0
>> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id
>> 2) not exist
>> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load
>> header: -22
>> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>> load failed -22
>> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>> DSP topology -22
>> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>> snd_soc_component_probe on 0000:04:00.5: -22
>> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register
>> card(sof-nau8821-max)
>> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>>
>> Move BT_BE_ID to the correct position in the enum.
>>
>> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>> creation for I2S BT instance")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   sound/soc/amd/acp/acp-mach.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
>> index a48546d8d407..0c18ccd29305 100644
>> --- a/sound/soc/amd/acp/acp-mach.h
>> +++ b/sound/soc/amd/acp/acp-mach.h
>> @@ -27,8 +27,8 @@
>>   enum be_id {
>>       HEADSET_BE_ID = 0,
>>       AMP_BE_ID,
>> -    DMIC_BE_ID,
>>       BT_BE_ID,
>> +    DMIC_BE_ID,
> This will break the other platforms as this same enum used in topology
> to create dailink.

If I understand this correctly, there is no consistency across firmware
regarding the IDs used for DAI link identification.  What would be the
suggested solution in this case?

Thanks,
Cristian

>>   };
>>     enum cpu_endpoints {

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Cristian Ciocaltea]

On 12/10/23 05:30, Venkata Prasad Potturu wrote:
> 
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
>> DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type.  However, some
>> boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
>>
>> [  467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for
>> 0x30030000 (msg/reply size: 16/0): -22
>> [  467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup:
>> route ACPBT2.IN -> BUF5.0 failed
>> [  467.495839] snd_sof_amd_vangogh 0000:04:00.5:
>> sof_ipc3_set_up_all_pipelines: route set up failed
>> [  467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>> load failed -22
>> [  467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>> DSP topology -22
>> [  467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>> snd_soc_component_probe on 0000:04:00.5: -22
>> [  467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>> [  467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register
>> card(sof-nau8821-max)
>> [  467.514861] sof_mach: probe of nau8821-max failed with error -22
>>
>> Add "ACPBT" alias for "ACP" SOF DAI type.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   sound/soc/sof/topology.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
>> index e3e7fbe40fa6..73bf791e7520 100644
>> --- a/sound/soc/sof/topology.c
>> +++ b/sound/soc/sof/topology.c
>> @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
>>       {"SAI", SOF_DAI_IMX_SAI},
>>       {"ESAI", SOF_DAI_IMX_ESAI},
>>       {"ACP", SOF_DAI_AMD_BT},
>> +    {"ACPBT", SOF_DAI_AMD_BT},
> No need to create the alias name, we can directly modify ACP to ACPBT as
> ACP is not using anywhere.

Great, thanks, will do in v2.

>>       {"ACPSP", SOF_DAI_AMD_SP},
>>       {"ACPDMIC", SOF_DAI_AMD_DMIC},
>>       {"ACPHS", SOF_DAI_AMD_HS},

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Venkata Prasad Potturu]

On 12/10/23 14:38, Cristian Ciocaltea wrote:
> On 12/10/23 05:30, Venkata Prasad Potturu wrote:
>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>> Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
>>> DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type.  However, some
>>> boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
>>>
>>> [  467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for
>>> 0x30030000 (msg/reply size: 16/0): -22
>>> [  467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup:
>>> route ACPBT2.IN -> BUF5.0 failed
>>> [  467.495839] snd_sof_amd_vangogh 0000:04:00.5:
>>> sof_ipc3_set_up_all_pipelines: route set up failed
>>> [  467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>> load failed -22
>>> [  467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>> DSP topology -22
>>> [  467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>> snd_soc_component_probe on 0000:04:00.5: -22
>>> [  467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>>> [  467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register
>>> card(sof-nau8821-max)
>>> [  467.514861] sof_mach: probe of nau8821-max failed with error -22
>>>
>>> Add "ACPBT" alias for "ACP" SOF DAI type.
>>>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>    sound/soc/sof/topology.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
>>> index e3e7fbe40fa6..73bf791e7520 100644
>>> --- a/sound/soc/sof/topology.c
>>> +++ b/sound/soc/sof/topology.c
>>> @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
>>>        {"SAI", SOF_DAI_IMX_SAI},
>>>        {"ESAI", SOF_DAI_IMX_ESAI},
>>>        {"ACP", SOF_DAI_AMD_BT},
>>> +    {"ACPBT", SOF_DAI_AMD_BT},
>> No need to create the alias name, we can directly modify ACP to ACPBT as
>> ACP is not using anywhere.
> Great, thanks, will do in v2.
This should send to SOF git repo for rewiew, once SOF reviewers approved 
this, again need to send to broonie git.
All the changes in sound/soc/sof/ path should go to SOF git.
>
>>>        {"ACPSP", SOF_DAI_AMD_SP},
>>>        {"ACPDMIC", SOF_DAI_AMD_DMIC},
>>>        {"ACPHS", SOF_DAI_AMD_HS},

Re: [PATCH 10/11] ASoC: amd: acp: Use correct DAI link ID for BT codec

[Venkata Prasad Potturu]

On 12/10/23 14:36, Cristian Ciocaltea wrote:
> On 12/10/23 05:24, Venkata Prasad Potturu wrote:
>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>> creation for I2S BT instance") added I2S BT support in ACP common
>>> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>>>
>>> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version
>>> 0:0:0-7863d
>>> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0
>>> Kernel ABI 3:23:0
>>> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0
>>> Kernel ABI 3:23:0
>>> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id
>>> 2) not exist
>>> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load
>>> header: -22
>>> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>> load failed -22
>>> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>> DSP topology -22
>>> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>> snd_soc_component_probe on 0000:04:00.5: -22
>>> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>>> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register
>>> card(sof-nau8821-max)
>>> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>>>
>>> Move BT_BE_ID to the correct position in the enum.
>>>
>>> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>> creation for I2S BT instance")
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>    sound/soc/amd/acp/acp-mach.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h
>>> index a48546d8d407..0c18ccd29305 100644
>>> --- a/sound/soc/amd/acp/acp-mach.h
>>> +++ b/sound/soc/amd/acp/acp-mach.h
>>> @@ -27,8 +27,8 @@
>>>    enum be_id {
>>>        HEADSET_BE_ID = 0,
>>>        AMP_BE_ID,
>>> -    DMIC_BE_ID,
>>>        BT_BE_ID,
>>> +    DMIC_BE_ID,
>> This will break the other platforms as this same enum used in topology
>> to create dailink.
> If I understand this correctly, there is no consistency across firmware
> regarding the IDs used for DAI link identification.  What would be the
> suggested solution in this case?

These id values should be same in machine driver and topology file, then 
only dailink can create without an error.
Always new be_id should add at the end only.

In this case BT_BE_ID should be at the end.

   enum be_id {
       HEADSET_BE_ID = 0,
       AMP_BE_ID,
       DMIC_BE_ID,
       BT_BE_ID,
   }




>
> Thanks,
> Cristian
>
>>>    };
>>>      enum cpu_endpoints {

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Cristian Ciocaltea]

On 12/10/23 11:51, Venkata Prasad Potturu wrote:
> 
> On 12/10/23 14:38, Cristian Ciocaltea wrote:
>> On 12/10/23 05:30, Venkata Prasad Potturu wrote:
>>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>>> Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP
>>>> DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type.  However,
>>>> some
>>>> boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same
>>>> type:
>>>>
>>>> [  467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for
>>>> 0x30030000 (msg/reply size: 16/0): -22
>>>> [  467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup:
>>>> route ACPBT2.IN -> BUF5.0 failed
>>>> [  467.495839] snd_sof_amd_vangogh 0000:04:00.5:
>>>> sof_ipc3_set_up_all_pipelines: route set up failed
>>>> [  467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>>> load failed -22
>>>> [  467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>>> DSP topology -22
>>>> [  467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>>> snd_soc_component_probe on 0000:04:00.5: -22
>>>> [  467.508430] sof_mach nau8821-max: ASoC: failed to instantiate
>>>> card -22
>>>> [  467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register
>>>> card(sof-nau8821-max)
>>>> [  467.514861] sof_mach: probe of nau8821-max failed with error -22
>>>>
>>>> Add "ACPBT" alias for "ACP" SOF DAI type.
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>    sound/soc/sof/topology.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
>>>> index e3e7fbe40fa6..73bf791e7520 100644
>>>> --- a/sound/soc/sof/topology.c
>>>> +++ b/sound/soc/sof/topology.c
>>>> @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = {
>>>>        {"SAI", SOF_DAI_IMX_SAI},
>>>>        {"ESAI", SOF_DAI_IMX_ESAI},
>>>>        {"ACP", SOF_DAI_AMD_BT},
>>>> +    {"ACPBT", SOF_DAI_AMD_BT},
>>> No need to create the alias name, we can directly modify ACP to ACPBT as
>>> ACP is not using anywhere.
>> Great, thanks, will do in v2.
> This should send to SOF git repo for rewiew, once SOF reviewers approved
> this, again need to send to broonie git.
> All the changes in sound/soc/sof/ path should go to SOF git.

Unfortunately I'm not familiar with the SOF dev workflow. So it's not
enough to have this patch cc-ed to sound-open-firmware@alsa-project.org?

>>>>        {"ACPSP", SOF_DAI_AMD_SP},
>>>>        {"ACPDMIC", SOF_DAI_AMD_DMIC},
>>>>        {"ACPHS", SOF_DAI_AMD_HS},

Re: [PATCH 10/11] ASoC: amd: acp: Use correct DAI link ID for BT codec

[Cristian Ciocaltea]

On 12/10/23 12:05, Venkata Prasad Potturu wrote:
> 
> On 12/10/23 14:36, Cristian Ciocaltea wrote:
>> On 12/10/23 05:24, Venkata Prasad Potturu wrote:
>>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>>> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>>> creation for I2S BT instance") added I2S BT support in ACP common
>>>> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>>>>
>>>> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version
>>>> 0:0:0-7863d
>>>> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0
>>>> Kernel ABI 3:23:0
>>>> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0
>>>> Kernel ABI 3:23:0
>>>> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id
>>>> 2) not exist
>>>> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load
>>>> header: -22
>>>> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>>> load failed -22
>>>> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>>> DSP topology -22
>>>> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>>> snd_soc_component_probe on 0000:04:00.5: -22
>>>> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>>>> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register
>>>> card(sof-nau8821-max)
>>>> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>>>>
>>>> Move BT_BE_ID to the correct position in the enum.
>>>>
>>>> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>>> creation for I2S BT instance")
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>    sound/soc/amd/acp/acp-mach.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/sound/soc/amd/acp/acp-mach.h
>>>> b/sound/soc/amd/acp/acp-mach.h
>>>> index a48546d8d407..0c18ccd29305 100644
>>>> --- a/sound/soc/amd/acp/acp-mach.h
>>>> +++ b/sound/soc/amd/acp/acp-mach.h
>>>> @@ -27,8 +27,8 @@
>>>>    enum be_id {
>>>>        HEADSET_BE_ID = 0,
>>>>        AMP_BE_ID,
>>>> -    DMIC_BE_ID,
>>>>        BT_BE_ID,
>>>> +    DMIC_BE_ID,
>>> This will break the other platforms as this same enum used in topology
>>> to create dailink.
>> If I understand this correctly, there is no consistency across firmware
>> regarding the IDs used for DAI link identification.  What would be the
>> suggested solution in this case?
> 
> These id values should be same in machine driver and topology file, then
> only dailink can create without an error.

Yes, my point was that some topology files seem to require different IDs
for the same DAI link types.  In this case the topology expects ID 2 for
BT, but other topologies would interpret that as DMIC.

> Always new be_id should add at the end only.
> 
> In this case BT_BE_ID should be at the end.
> 
>   enum be_id {
>       HEADSET_BE_ID = 0,
>       AMP_BE_ID,
>       DMIC_BE_ID,
>       BT_BE_ID,
>   }

So you are basically stating the firmware is broken and needs an update
to use ID 3 for BT, and there is nothing we can do about it on driver's
side.  Is that correct?

> 
> 
>>
>> Thanks,
>> Cristian
>>
>>>>    };
>>>>      enum cpu_endpoints {

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Mark Brown]

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

On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
> On 12/10/23 11:51, Venkata Prasad Potturu wrote:

> > This should send to SOF git repo for rewiew, once SOF reviewers approved
> > this, again need to send to broonie git.
> > All the changes in sound/soc/sof/ path should go to SOF git.

> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
> enough to have this patch cc-ed to sound-open-firmware@alsa-project.org?

The SOF people basically do their own thing in github at

   https://github.com/thesofproject/linux

with a github workflow and submit their patches upstream in batches a
few times a release, however my understanding is that their workflow can
cope with things going in directly upstream as well.

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

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Cristian Ciocaltea]

On 12/10/23 16:01, Mark Brown wrote:
> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
> 
>>> This should send to SOF git repo for rewiew, once SOF reviewers approved
>>> this, again need to send to broonie git.
>>> All the changes in sound/soc/sof/ path should go to SOF git.
> 
>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>> enough to have this patch cc-ed to sound-open-firmware@alsa-project.org?
> 
> The SOF people basically do their own thing in github at
> 
>    https://github.com/thesofproject/linux
> 
> with a github workflow and submit their patches upstream in batches a
> few times a release, however my understanding is that their workflow can
> cope with things going in directly upstream as well.

Thanks for clarifying, Mark!  That would greatly simplify and speedup
the whole process, at least for trivial patches like this one.

Re: [PATCH 10/11] ASoC: amd: acp: Use correct DAI link ID for BT codec

[Venkata Prasad Potturu]

On 12/10/23 16:02, Cristian Ciocaltea wrote:
> On 12/10/23 12:05, Venkata Prasad Potturu wrote:
>> On 12/10/23 14:36, Cristian Ciocaltea wrote:
>>> On 12/10/23 05:24, Venkata Prasad Potturu wrote:
>>>> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>>>>> Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>>>> creation for I2S BT instance") added I2S BT support in ACP common
>>>>> machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
>>>>>
>>>>> [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version
>>>>> 0:0:0-7863d
>>>>> [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0
>>>>> Kernel ABI 3:23:0
>>>>> [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0
>>>>> Kernel ABI 3:23:0
>>>>> [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id
>>>>> 2) not exist
>>>>> [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load
>>>>> header: -22
>>>>> [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component
>>>>> load failed -22
>>>>> [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load
>>>>> DSP topology -22
>>>>> [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at
>>>>> snd_soc_component_probe on 0000:04:00.5: -22
>>>>> [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22
>>>>> [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register
>>>>> card(sof-nau8821-max)
>>>>> [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
>>>>>
>>>>> Move BT_BE_ID to the correct position in the enum.
>>>>>
>>>>> Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink
>>>>> creation for I2S BT instance")
>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>> ---
>>>>>     sound/soc/amd/acp/acp-mach.h | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/sound/soc/amd/acp/acp-mach.h
>>>>> b/sound/soc/amd/acp/acp-mach.h
>>>>> index a48546d8d407..0c18ccd29305 100644
>>>>> --- a/sound/soc/amd/acp/acp-mach.h
>>>>> +++ b/sound/soc/amd/acp/acp-mach.h
>>>>> @@ -27,8 +27,8 @@
>>>>>     enum be_id {
>>>>>         HEADSET_BE_ID = 0,
>>>>>         AMP_BE_ID,
>>>>> -    DMIC_BE_ID,
>>>>>         BT_BE_ID,
>>>>> +    DMIC_BE_ID,
>>>> This will break the other platforms as this same enum used in topology
>>>> to create dailink.
>>> If I understand this correctly, there is no consistency across firmware
>>> regarding the IDs used for DAI link identification.  What would be the
>>> suggested solution in this case?
>> These id values should be same in machine driver and topology file, then
>> only dailink can create without an error.
> Yes, my point was that some topology files seem to require different IDs
> for the same DAI link types.  In this case the topology expects ID 2 for
> BT, but other topologies would interpret that as DMIC.
>
>> Always new be_id should add at the end only.
>>
>> In this case BT_BE_ID should be at the end.
>>
>>    enum be_id {
>>        HEADSET_BE_ID = 0,
>>        AMP_BE_ID,
>>        DMIC_BE_ID,
>>        BT_BE_ID,
>>    }
> So you are basically stating the firmware is broken and needs an update
> to use ID 3 for BT, and there is nothing we can do about it on driver's
> side.  Is that correct?
Yes, id 3 should be used for BT_BE_ID in topology file.
>
>>
>>> Thanks,
>>> Cristian
>>>
>>>>>     };
>>>>>       enum cpu_endpoints {

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Venkata Prasad Potturu]

On 12/10/23 21:20, Cristian Ciocaltea wrote:
> On 12/10/23 16:01, Mark Brown wrote:
>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>> This should send to SOF git repo for rewiew, once SOF reviewers approved
>>>> this, again need to send to broonie git.
>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>> enough to have this patch cc-ed to sound-open-firmware@alsa-project.org?
>> The SOF people basically do their own thing in github at
>>
>>     https://github.com/thesofproject/linux
>>
>> with a github workflow and submit their patches upstream in batches a
>> few times a release, however my understanding is that their workflow can
>> cope with things going in directly upstream as well.
> Thanks for clarifying, Mark!  That would greatly simplify and speedup
> the whole process, at least for trivial patches like this one.

Hi Cristian,

We have created a Pull request in SOF git hub for I2S BT support.
please hold v2 version SOF patches till below PR get's merged.
PR:- https://github.com/thesofproject/linux/pull/4742

Re: [PATCH 03/11] ASoC: amd: acp: Add missing error handling in sof-mach

[Emil Velikov]

On 2023/12/09, Cristian Ciocaltea wrote:
> Handle potential acp_sofdsp_dai_links_create() errors in ACP SOF machine
> driver's probe function.  Additionally, switch to dev_err_probe().
> 
> Fixes: 9f84940f5004 ("ASoC: amd: acp: Add SOF audio support on Chrome board")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  sound/soc/amd/acp/acp-sof-mach.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c
> index 1d313fcb5f2d..6f0ca23638af 100644
> --- a/sound/soc/amd/acp/acp-sof-mach.c
> +++ b/sound/soc/amd/acp/acp-sof-mach.c
> @@ -112,16 +112,14 @@ static int acp_sof_probe(struct platform_device *pdev)
>  	if (dmi_id && dmi_id->driver_data)
>  		acp_card_drvdata->tdm_mode = dmi_id->driver_data;
>  
> -	acp_sofdsp_dai_links_create(card);
> +	ret = acp_sofdsp_dai_links_create(card);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to create DAI links\n");
>  
>  	ret = devm_snd_soc_register_card(&pdev->dev, card);
> -	if (ret) {
> -		dev_err(&pdev->dev,
> -				"devm_snd_soc_register_card(%s) failed: %d\n",
> -				card->name, ret);
> -		return ret;
> -	}
> -
> +	if (ret)

Do we need to undo acp_sofdsp_dai_links_create() in here? If not, please
add a trivial note in the commit message.

With that the series is:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

HTH o/
-Emil

Re: [PATCH 03/11] ASoC: amd: acp: Add missing error handling in sof-mach

[Cristian Ciocaltea]

On 12/11/23 15:31, Emil Velikov wrote:
> On 2023/12/09, Cristian Ciocaltea wrote:
>> Handle potential acp_sofdsp_dai_links_create() errors in ACP SOF machine
>> driver's probe function.  Additionally, switch to dev_err_probe().
>>
>> Fixes: 9f84940f5004 ("ASoC: amd: acp: Add SOF audio support on Chrome board")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  sound/soc/amd/acp/acp-sof-mach.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c
>> index 1d313fcb5f2d..6f0ca23638af 100644
>> --- a/sound/soc/amd/acp/acp-sof-mach.c
>> +++ b/sound/soc/amd/acp/acp-sof-mach.c
>> @@ -112,16 +112,14 @@ static int acp_sof_probe(struct platform_device *pdev)
>>  	if (dmi_id && dmi_id->driver_data)
>>  		acp_card_drvdata->tdm_mode = dmi_id->driver_data;
>>  
>> -	acp_sofdsp_dai_links_create(card);
>> +	ret = acp_sofdsp_dai_links_create(card);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "Failed to create DAI links\n");
>>  
>>  	ret = devm_snd_soc_register_card(&pdev->dev, card);
>> -	if (ret) {
>> -		dev_err(&pdev->dev,
>> -				"devm_snd_soc_register_card(%s) failed: %d\n",
>> -				card->name, ret);
>> -		return ret;
>> -	}
>> -
>> +	if (ret)
> 
> Do we need to undo acp_sofdsp_dai_links_create() in here? If not, please
> add a trivial note in the commit message.

No need to undo, will update the commit as suggested.

> With that the series is:
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Thanks for reviewing,
Cristian

> HTH o/
> -Emil

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Mukunda,Vijendar]

On 10/12/23 19:31, Mark Brown wrote:
> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>> This should send to SOF git repo for rewiew, once SOF reviewers approved
>>> this, again need to send to broonie git.
>>> All the changes in sound/soc/sof/ path should go to SOF git.
>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>> enough to have this patch cc-ed to sound-open-firmware@alsa-project.org?
> The SOF people basically do their own thing in github at
>
>    https://github.com/thesofproject/linux
>
> with a github workflow and submit their patches upstream in batches a
> few times a release, however my understanding is that their workflow can
> cope with things going in directly upstream as well.
If patches are directly pushed to alsa devel list instead of creating pull request for SOF patches
It will break SOF github work flow.
Validation across all the platforms is a potential challenge, and it will also create an overhead
to pull the patches which got merged in to for-next branch, before the all the patches pulled in to SOF
GitHub.

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Péter Ujfalusi]

On 09/12/2023 22:53, Cristian Ciocaltea wrote:
> Some SOF drivers like AMD ACP do not always rely on a single static
> firmware file, but may require multiple files having their names
> dynamically computed on probe time, e.g. based on chip name.

I see, AMD vangogh needs two binary files to be loaded sometimes, it is not using the default name as it hints via the sof_dev_desc ("sof-vangogh.ri").

The constructed names for the two files are just using different pattern:
sof-%PLAT%.ri
vs
sof-%PLAT%-code.bin
sof-%PLAT%-data.bin

iow, instead of the combined .ri file which includes the code and data segment it has 'raw' bin files and cannot use the core for loading the firmware.

What is the reason for this? an .ri file can have two 'modules' one to be written to IRAM the other to DRAM.
sof_ipc3_load_fw_to_dsp()

> In those cases, providing an invalid default_fw_filename in their
> sof_dev_desc struct will prevent probing due to 'SOF firmware
> and/or topology file not found' error.
 
> Fix the issue by allowing drivers to omit initialization for this member
> (or alternatively provide a dynamic override via ipc_file_profile_base)
> and update sof_test_firmware_file() to verify the given profile data and
> skip firmware testing if either fw_path or fw_name is not defined.
> 
> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  sound/soc/sof/fw-file-profile.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
> index 138a1ca2c4a8..e63700234df0 100644
> --- a/sound/soc/sof/fw-file-profile.c
> +++ b/sound/soc/sof/fw-file-profile.c
> @@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev,
>  	const u32 *magic;
>  	int ret;
>  
> +	if (!profile->fw_path || !profile->fw_name || !*profile->fw_name)
> +		return 0;

I would rather make the firmware file skipping based on custom loader use and print a dev_dbg() alongside:

diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
index 138a1ca2c4a8..7b91c9551ada 100644
--- a/sound/soc/sof/fw-file-profile.c
+++ b/sound/soc/sof/fw-file-profile.c
@@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
 	return ret;
 }
 
+static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
+{
+	if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
+	    sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
+		return true;
+
+	return false;
+}
+
 static int
 sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
 			      enum sof_ipc_type ipc_type,
@@ -130,7 +139,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
 	 * For default path and firmware name do a verification before
 	 * continuing further.
 	 */
-	if (base_profile->fw_path || base_profile->fw_name) {
+	if ((base_profile->fw_path || base_profile->fw_name) &&
+	    sof_platform_uses_generic_loader(sdev)) {
 		ret = sof_test_firmware_file(dev, out_profile, &ipc_type);
 		if (ret)
 			return ret;
@@ -181,7 +191,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
 	out_profile->ipc_type = ipc_type;
 
 	/* Test only default firmware file */
-	if (!base_profile->fw_path && !base_profile->fw_name)
+	if ((!base_profile->fw_path && !base_profile->fw_name) &&
+	    sof_platform_uses_generic_loader(sdev))
 		ret = sof_test_firmware_file(dev, out_profile, NULL);
 
 	if (!ret)
@@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
 			 "Using fallback IPC type %d (requested type was %d)\n",
 			 profile->ipc_type, ipc_type);
 
-	dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
+	/* The firmware path only valid when generic loader is used */
+	if (sof_platform_uses_generic_loader(sdev))
+		dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
 
 	dev_info(dev, " Firmware file:     %s/%s\n", profile->fw_path, profile->fw_name);
 	if (profile->fw_lib_path)

> +
>  	fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
>  				profile->fw_name);
>  	if (!fw_filename)

checking for custom loader allows you to drop the next patch.
I would also skip the fw path printing in case of a custom loader.

-- 
Péter

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Venkata Prasad Potturu]

On 12/14/23 16:18, Péter Ujfalusi wrote:
Thanks for your time Peter!
>
> On 09/12/2023 22:53, Cristian Ciocaltea wrote:
>> Some SOF drivers like AMD ACP do not always rely on a single static
>> firmware file, but may require multiple files having their names
>> dynamically computed on probe time, e.g. based on chip name.
> I see, AMD vangogh needs two binary files to be loaded sometimes, it is not using the default name as it hints via the sof_dev_desc ("sof-vangogh.ri").
>
> The constructed names for the two files are just using different pattern:
> sof-%PLAT%.ri
> vs
> sof-%PLAT%-code.bin
> sof-%PLAT%-data.bin
>
> iow, instead of the combined .ri file which includes the code and data segment it has 'raw' bin files and cannot use the core for loading the firmware.
>
> What is the reason for this? an .ri file can have two 'modules' one to be written to IRAM the other to DRAM.
> sof_ipc3_load_fw_to_dsp()

For AMD Vangogh platform devices signed firmware image is required, so 
split .ri image into code and data images.

Only Code.bin will be signed and loaded into corresponding IRAM location.

>
>> In those cases, providing an invalid default_fw_filename in their
>> sof_dev_desc struct will prevent probing due to 'SOF firmware
>> and/or topology file not found' error.
>   
>> Fix the issue by allowing drivers to omit initialization for this member
>> (or alternatively provide a dynamic override via ipc_file_profile_base)
>> and update sof_test_firmware_file() to verify the given profile data and
>> skip firmware testing if either fw_path or fw_name is not defined.
>>
>> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   sound/soc/sof/fw-file-profile.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>> index 138a1ca2c4a8..e63700234df0 100644
>> --- a/sound/soc/sof/fw-file-profile.c
>> +++ b/sound/soc/sof/fw-file-profile.c
>> @@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev,
>>   	const u32 *magic;
>>   	int ret;
>>   
>> +	if (!profile->fw_path || !profile->fw_name || !*profile->fw_name)
>> +		return 0;
> I would rather make the firmware file skipping based on custom loader use and print a dev_dbg() alongside:
>
> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
> index 138a1ca2c4a8..7b91c9551ada 100644
> --- a/sound/soc/sof/fw-file-profile.c
> +++ b/sound/soc/sof/fw-file-profile.c
> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
>   	return ret;
>   }
>   
> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
> +{
> +	if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
> +	    sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
> +		return true;
> +
> +	return false;
> +}
> +
>   static int
>   sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
>   			      enum sof_ipc_type ipc_type,
> @@ -130,7 +139,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
>   	 * For default path and firmware name do a verification before
>   	 * continuing further.
>   	 */
> -	if (base_profile->fw_path || base_profile->fw_name) {
> +	if ((base_profile->fw_path || base_profile->fw_name) &&
> +	    sof_platform_uses_generic_loader(sdev)) {
>   		ret = sof_test_firmware_file(dev, out_profile, &ipc_type);
>   		if (ret)
>   			return ret;
> @@ -181,7 +191,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
>   	out_profile->ipc_type = ipc_type;
>   
>   	/* Test only default firmware file */
> -	if (!base_profile->fw_path && !base_profile->fw_name)
> +	if ((!base_profile->fw_path && !base_profile->fw_name) &&
> +	    sof_platform_uses_generic_loader(sdev))
>   		ret = sof_test_firmware_file(dev, out_profile, NULL);
>   
>   	if (!ret)
> @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
>   			 "Using fallback IPC type %d (requested type was %d)\n",
>   			 profile->ipc_type, ipc_type);
>   
> -	dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
> +	/* The firmware path only valid when generic loader is used */
> +	if (sof_platform_uses_generic_loader(sdev))
> +		dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>   
>   	dev_info(dev, " Firmware file:     %s/%s\n", profile->fw_path, profile->fw_name);
>   	if (profile->fw_lib_path)
>
>> +
>>   	fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
>>   				profile->fw_name);
>>   	if (!fw_filename)
> checking for custom loader allows you to drop the next patch.
> I would also skip the fw path printing in case of a custom loader.
>

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Cristian Ciocaltea]

On 12/14/23 12:48, Péter Ujfalusi wrote:
> 
> 
> On 09/12/2023 22:53, Cristian Ciocaltea wrote:
>> Some SOF drivers like AMD ACP do not always rely on a single static
>> firmware file, but may require multiple files having their names
>> dynamically computed on probe time, e.g. based on chip name.
> 
> I see, AMD vangogh needs two binary files to be loaded sometimes, it is not using the default name as it hints via the sof_dev_desc ("sof-vangogh.ri").
> 
> The constructed names for the two files are just using different pattern:
> sof-%PLAT%.ri
> vs
> sof-%PLAT%-code.bin
> sof-%PLAT%-data.bin
> 
> iow, instead of the combined .ri file which includes the code and data segment it has 'raw' bin files and cannot use the core for loading the firmware.
> 
> What is the reason for this? an .ri file can have two 'modules' one to be written to IRAM the other to DRAM.
> sof_ipc3_load_fw_to_dsp()
> 
>> In those cases, providing an invalid default_fw_filename in their
>> sof_dev_desc struct will prevent probing due to 'SOF firmware
>> and/or topology file not found' error.
>  
>> Fix the issue by allowing drivers to omit initialization for this member
>> (or alternatively provide a dynamic override via ipc_file_profile_base)
>> and update sof_test_firmware_file() to verify the given profile data and
>> skip firmware testing if either fw_path or fw_name is not defined.
>>
>> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  sound/soc/sof/fw-file-profile.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>> index 138a1ca2c4a8..e63700234df0 100644
>> --- a/sound/soc/sof/fw-file-profile.c
>> +++ b/sound/soc/sof/fw-file-profile.c
>> @@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev,
>>  	const u32 *magic;
>>  	int ret;
>>  
>> +	if (!profile->fw_path || !profile->fw_name || !*profile->fw_name)
>> +		return 0;
> 
> I would rather make the firmware file skipping based on custom loader use and print a dev_dbg() alongside:

Agree, that's a better approach. Thanks for the hint!

> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
> index 138a1ca2c4a8..7b91c9551ada 100644
> --- a/sound/soc/sof/fw-file-profile.c
> +++ b/sound/soc/sof/fw-file-profile.c
> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
>  	return ret;
>  }
>  
> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
> +{
> +	if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
> +	    sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
> +		return true;
> +
> +	return false;
> +}

I would drop the conditional and simply return.

>  static int
>  sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
>  			      enum sof_ipc_type ipc_type,
> @@ -130,7 +139,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
>  	 * For default path and firmware name do a verification before
>  	 * continuing further.
>  	 */
> -	if (base_profile->fw_path || base_profile->fw_name) {
> +	if ((base_profile->fw_path || base_profile->fw_name) &&
> +	    sof_platform_uses_generic_loader(sdev)) {
>  		ret = sof_test_firmware_file(dev, out_profile, &ipc_type);
>  		if (ret)
>  			return ret;
> @@ -181,7 +191,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev,
>  	out_profile->ipc_type = ipc_type;
>  
>  	/* Test only default firmware file */
> -	if (!base_profile->fw_path && !base_profile->fw_name)
> +	if ((!base_profile->fw_path && !base_profile->fw_name) &&
> +	    sof_platform_uses_generic_loader(sdev))
>  		ret = sof_test_firmware_file(dev, out_profile, NULL);
>  
>  	if (!ret)
> @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
>  			 "Using fallback IPC type %d (requested type was %d)\n",
>  			 profile->ipc_type, ipc_type);
>  
> -	dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
> +	/* The firmware path only valid when generic loader is used */
> +	if (sof_platform_uses_generic_loader(sdev))
> +		dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>  
>  	dev_info(dev, " Firmware file:     %s/%s\n", profile->fw_path, profile->fw_name);
>  	if (profile->fw_lib_path)
> 
>> +
>>  	fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
>>  				profile->fw_name);
>>  	if (!fw_filename)
> 
> checking for custom loader allows you to drop the next patch.

Yes, I will take only the context information and use it to update the
current patch description.

> I would also skip the fw path printing in case of a custom loader.

Sure, makes sense.

Thanks for the review,
Cristian

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Péter Ujfalusi]

On 14/12/2023 13:29, Cristian Ciocaltea wrote:
>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>> index 138a1ca2c4a8..7b91c9551ada 100644
>> --- a/sound/soc/sof/fw-file-profile.c
>> +++ b/sound/soc/sof/fw-file-profile.c
>> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
>>  	return ret;
>>  }
>>  
>> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
>> +{
>> +	if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
>> +	    sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
>> +		return true;
>> +
>> +	return false;
>> +}
> 
> I would drop the conditional and simply return.

What do you mean? We need to check if the platform is using either type
of the generic load_firmware helper (the _memcpy is calling the _raw to
load the file).

-- 
Péter

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Cristian Ciocaltea]

On 12/14/23 13:35, Péter Ujfalusi wrote:
> 
> 
> On 14/12/2023 13:29, Cristian Ciocaltea wrote:
>>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>>> index 138a1ca2c4a8..7b91c9551ada 100644
>>> --- a/sound/soc/sof/fw-file-profile.c
>>> +++ b/sound/soc/sof/fw-file-profile.c
>>> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
>>>  	return ret;
>>>  }
>>>  
>>> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
>>> +{
>>> +	if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
>>> +	    sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
>>> +		return true;
>>> +
>>> +	return false;
>>> +}
>>
>> I would drop the conditional and simply return.
> 
> What do you mean? We need to check if the platform is using either type
> of the generic load_firmware helper (the _memcpy is calling the _raw to
> load the file).

I mean to simply replace the if statement with:

static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
{
    return (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
	    sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy);
}

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Péter Ujfalusi]

On 14/12/2023 12:48, Péter Ujfalusi wrote:
> @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
>  			 "Using fallback IPC type %d (requested type was %d)\n",
>  			 profile->ipc_type, ipc_type);
>  
> -	dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
> +	/* The firmware path only valid when generic loader is used */
> +	if (sof_platform_uses_generic_loader(sdev))
> +		dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>  

This is the correct section in here, sorry:

-	dev_info(dev, " Firmware file:     %s/%s\n", profile->fw_path, profile->fw_name);
+	/* The firmware path only valid when generic loader is used */
+	if (sof_platform_uses_generic_loader(sdev))
+		dev_info(dev, " Firmware file:     %s/%s\n", profile->fw_path, profile->fw_name);
+
 	if (profile->fw_lib_path)


-- 
Péter

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Péter Ujfalusi]

On 14/12/2023 13:40, Cristian Ciocaltea wrote:
> On 12/14/23 13:35, Péter Ujfalusi wrote:
>>
>>
>> On 14/12/2023 13:29, Cristian Ciocaltea wrote:
>>>> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
>>>> index 138a1ca2c4a8..7b91c9551ada 100644
>>>> --- a/sound/soc/sof/fw-file-profile.c
>>>> +++ b/sound/soc/sof/fw-file-profile.c
>>>> @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
>>>> +{
>>>> +	if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
>>>> +	    sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy)
>>>> +		return true;
>>>> +
>>>> +	return false;
>>>> +}
>>>
>>> I would drop the conditional and simply return.
>>
>> What do you mean? We need to check if the platform is using either type
>> of the generic load_firmware helper (the _memcpy is calling the _raw to
>> load the file).
> 
> I mean to simply replace the if statement with:
> 
> static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev)
> {
>     return (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw ||
> 	    sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy);
> }

ah, OK.

-- 
Péter

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Péter Ujfalusi]

On 14/12/2023 12:58, Venkata Prasad Potturu wrote:
> 
> On 12/14/23 16:18, Péter Ujfalusi wrote:
> Thanks for your time Peter!
>>
>> On 09/12/2023 22:53, Cristian Ciocaltea wrote:
>>> Some SOF drivers like AMD ACP do not always rely on a single static
>>> firmware file, but may require multiple files having their names
>>> dynamically computed on probe time, e.g. based on chip name.
>> I see, AMD vangogh needs two binary files to be loaded sometimes, it
>> is not using the default name as it hints via the sof_dev_desc
>> ("sof-vangogh.ri").
>>
>> The constructed names for the two files are just using different pattern:
>> sof-%PLAT%.ri
>> vs
>> sof-%PLAT%-code.bin
>> sof-%PLAT%-data.bin
>>
>> iow, instead of the combined .ri file which includes the code and data
>> segment it has 'raw' bin files and cannot use the core for loading the
>> firmware.
>>
>> What is the reason for this? an .ri file can have two 'modules' one to
>> be written to IRAM the other to DRAM.
>> sof_ipc3_load_fw_to_dsp()
> 
> For AMD Vangogh platform devices signed firmware image is required, so
> split .ri image into code and data images.
> 
> Only Code.bin will be signed and loaded into corresponding IRAM location.

This is not different than what the Intel .ri files are made of. The
module which is to be loaded to IRAM is signed code the module which
goes to DRAM is not signed.
The loader itself is not looking into the sections of the .ri image, it
just parses the header and copies them where they belong.

if the issue is name collision then you could try to put the signed
firmware file under 'signed' folder (fw_path_postfix) of the platform
like Intel does with the community signed ones?

It would be great if somehow we can handle all of these in core, have
shared code and familiar prints among vendors, platforms..

Fwiw, I'm planning the path, filename creation to be moved to core for
the current platforms, but it implies that they do use single firmware file.
struct sof_dev_desc would only have two strings:
vendor - AMD / iMX / Intel / Mediatek
platform - tgl, vaggogh, etc

I need to adjust it based on what I have learned today about vangogh.

-- 
Péter

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Cristian Ciocaltea]

On 12/14/23 13:51, Péter Ujfalusi wrote:
> 
> 
> On 14/12/2023 12:48, Péter Ujfalusi wrote:
>> @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev,
>>  			 "Using fallback IPC type %d (requested type was %d)\n",
>>  			 profile->ipc_type, ipc_type);
>>  
>> -	dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>> +	/* The firmware path only valid when generic loader is used */
>> +	if (sof_platform_uses_generic_loader(sdev))
>> +		dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
>>  
> 
> This is the correct section in here, sorry:
> 
> -	dev_info(dev, " Firmware file:     %s/%s\n", profile->fw_path, profile->fw_name);
> +	/* The firmware path only valid when generic loader is used */
> +	if (sof_platform_uses_generic_loader(sdev))
> +		dev_info(dev, " Firmware file:     %s/%s\n", profile->fw_path, profile->fw_name);
> +
>  	if (profile->fw_lib_path)

No problem, thanks for the update!

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Cristian Ciocaltea]

On 12/11/23 07:58, Venkata Prasad Potturu wrote:
> 
> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>> On 12/10/23 16:01, Mark Brown wrote:
>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>> approved
>>>>> this, again need to send to broonie git.
>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>> enough to have this patch cc-ed to
>>>> sound-open-firmware@alsa-project.org?
>>> The SOF people basically do their own thing in github at
>>>
>>>     https://github.com/thesofproject/linux
>>>
>>> with a github workflow and submit their patches upstream in batches a
>>> few times a release, however my understanding is that their workflow can
>>> cope with things going in directly upstream as well.
>> Thanks for clarifying, Mark!  That would greatly simplify and speedup
>> the whole process, at least for trivial patches like this one.
> 
> Hi Cristian,
> 
> We have created a Pull request in SOF git hub for I2S BT support.
> please hold v2 version SOF patches till below PR get's merged.
> PR:- https://github.com/thesofproject/linux/pull/4742

Hi Venkata,

If this is going to be handled via the github workflow, this patch
should be removed from the series.  Since there is no dependency on it,
I cannot see a reason to put v2 on hold.

Do I miss something?

Thanks,
Cristian

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Venkata Prasad Potturu]

On 12/14/23 17:53, Cristian Ciocaltea wrote:
> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>> On 12/10/23 16:01, Mark Brown wrote:
>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>> approved
>>>>>> this, again need to send to broonie git.
>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>>> enough to have this patch cc-ed to
>>>>> sound-open-firmware@alsa-project.org?
>>>> The SOF people basically do their own thing in github at
>>>>
>>>>      https://github.com/thesofproject/linux
>>>>
>>>> with a github workflow and submit their patches upstream in batches a
>>>> few times a release, however my understanding is that their workflow can
>>>> cope with things going in directly upstream as well.
>>> Thanks for clarifying, Mark!  That would greatly simplify and speedup
>>> the whole process, at least for trivial patches like this one.
>> Hi Cristian,
>>
>> We have created a Pull request in SOF git hub for I2S BT support.
>> please hold v2 version SOF patches till below PR get's merged.
>> PR:- https://github.com/thesofproject/linux/pull/4742
> Hi Venkata,
>
> If this is going to be handled via the github workflow, this patch
> should be removed from the series.  Since there is no dependency on it,
> I cannot see a reason to put v2 on hold.
>
> Do I miss something?
Yes, need to drop this patch.

And it's better to follow the github workflow by creating a Pull request 
in SOF github

  for all sof driver related patches, rather than sending patches to 
broonie git directly.

>
> Thanks,
> Cristian

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Venkata Prasad Potturu]

On 12/14/23 17:53, Cristian Ciocaltea wrote:
> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>> On 12/10/23 16:01, Mark Brown wrote:
>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>> approved
>>>>>> this, again need to send to broonie git.
>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>>> enough to have this patch cc-ed to
>>>>> sound-open-firmware@alsa-project.org?
>>>> The SOF people basically do their own thing in github at
>>>>
>>>>      https://github.com/thesofproject/linux
>>>>
>>>> with a github workflow and submit their patches upstream in batches a
>>>> few times a release, however my understanding is that their workflow can
>>>> cope with things going in directly upstream as well.
>>> Thanks for clarifying, Mark!  That would greatly simplify and speedup
>>> the whole process, at least for trivial patches like this one.
>> Hi Cristian,
>>
>> We have created a Pull request in SOF git hub for I2S BT support.
>> please hold v2 version SOF patches till below PR get's merged.
>> PR:- https://github.com/thesofproject/linux/pull/4742
> Hi Venkata,
>
> If this is going to be handled via the github workflow, this patch
> should be removed from the series.  Since there is no dependency on it,
> I cannot see a reason to put v2 on hold.
>
> Do I miss something?
Non-sof driver related patches can directly send to broonie git ad v2 
series.
SOF driver patches should send to SOF github to avoid merge conflicts 
as  per guidelines of SOF community.
>
> Thanks,
> Cristian

Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined fw_name

[Venkata Prasad Potturu]

On 12/14/23 17:27, Péter Ujfalusi wrote:
>
> On 14/12/2023 12:58, Venkata Prasad Potturu wrote:
>> On 12/14/23 16:18, Péter Ujfalusi wrote:
>> Thanks for your time Peter!
>>> On 09/12/2023 22:53, Cristian Ciocaltea wrote:
>>>> Some SOF drivers like AMD ACP do not always rely on a single static
>>>> firmware file, but may require multiple files having their names
>>>> dynamically computed on probe time, e.g. based on chip name.
>>> I see, AMD vangogh needs two binary files to be loaded sometimes, it
>>> is not using the default name as it hints via the sof_dev_desc
>>> ("sof-vangogh.ri").
>>>
>>> The constructed names for the two files are just using different pattern:
>>> sof-%PLAT%.ri
>>> vs
>>> sof-%PLAT%-code.bin
>>> sof-%PLAT%-data.bin
>>>
>>> iow, instead of the combined .ri file which includes the code and data
>>> segment it has 'raw' bin files and cannot use the core for loading the
>>> firmware.
>>>
>>> What is the reason for this? an .ri file can have two 'modules' one to
>>> be written to IRAM the other to DRAM.
>>> sof_ipc3_load_fw_to_dsp()
>> For AMD Vangogh platform devices signed firmware image is required, so
>> split .ri image into code and data images.
>>
>> Only Code.bin will be signed and loaded into corresponding IRAM location.
> This is not different than what the Intel .ri files are made of. The
> module which is to be loaded to IRAM is signed code the module which
> goes to DRAM is not signed.
> The loader itself is not looking into the sections of the .ri image, it
> just parses the header and copies them where they belong.
>
> if the issue is name collision then you could try to put the signed
> firmware file under 'signed' folder (fw_path_postfix) of the platform
> like Intel does with the community signed ones?

We have a limitation that code image can't be signed during compilation.

So splitting the .ri image into code and data bin and sign the code bin
and load into IRAM.

>
> It would be great if somehow we can handle all of these in core, have
> shared code and familiar prints among vendors, platforms..
>
> Fwiw, I'm planning the path, filename creation to be moved to core for
> the current platforms, but it implies that they do use single firmware file.
> struct sof_dev_desc would only have two strings:
> vendor - AMD / iMX / Intel / Mediatek
> platform - tgl, vaggogh, etc
>
> I need to adjust it based on what I have learned today about vangogh.
>

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Cristian Ciocaltea]

On 12/14/23 15:15, Venkata Prasad Potturu wrote:
> 
> On 12/14/23 17:53, Cristian Ciocaltea wrote:
>> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>>> On 12/10/23 16:01, Mark Brown wrote:
>>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>>> approved
>>>>>>> this, again need to send to broonie git.
>>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>>>> enough to have this patch cc-ed to
>>>>>> sound-open-firmware@alsa-project.org?
>>>>> The SOF people basically do their own thing in github at
>>>>>
>>>>>      https://github.com/thesofproject/linux
>>>>>
>>>>> with a github workflow and submit their patches upstream in batches a
>>>>> few times a release, however my understanding is that their
>>>>> workflow can
>>>>> cope with things going in directly upstream as well.
>>>> Thanks for clarifying, Mark!  That would greatly simplify and speedup
>>>> the whole process, at least for trivial patches like this one.
>>> Hi Cristian,
>>>
>>> We have created a Pull request in SOF git hub for I2S BT support.
>>> please hold v2 version SOF patches till below PR get's merged.
>>> PR:- https://github.com/thesofproject/linux/pull/4742
>> Hi Venkata,
>>
>> If this is going to be handled via the github workflow, this patch
>> should be removed from the series.  Since there is no dependency on it,
>> I cannot see a reason to put v2 on hold.
>>
>> Do I miss something?
> Non-sof driver related patches can directly send to broonie git ad v2
> series.
> SOF driver patches should send to SOF github to avoid merge conflicts
> as  per guidelines of SOF community.

Honestly, I don't really see a high risk of conflicts, the patches are
not that complex and can be simply cherry-picked when needed.  Moreover,
as we already had people reviewing this, splitting this up will only add
confusion and unnecessary burden.

Are there any specific changes you are concerned about and cannot be
really handled here?

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Venkata Prasad Potturu]

On 12/14/23 22:12, Cristian Ciocaltea wrote:
> On 12/14/23 15:15, Venkata Prasad Potturu wrote:
>> On 12/14/23 17:53, Cristian Ciocaltea wrote:
>>> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>>>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>>>> On 12/10/23 16:01, Mark Brown wrote:
>>>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>>>> approved
>>>>>>>> this, again need to send to broonie git.
>>>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So it's not
>>>>>>> enough to have this patch cc-ed to
>>>>>>> sound-open-firmware@alsa-project.org?
>>>>>> The SOF people basically do their own thing in github at
>>>>>>
>>>>>>       https://github.com/thesofproject/linux
>>>>>>
>>>>>> with a github workflow and submit their patches upstream in batches a
>>>>>> few times a release, however my understanding is that their
>>>>>> workflow can
>>>>>> cope with things going in directly upstream as well.
>>>>> Thanks for clarifying, Mark!  That would greatly simplify and speedup
>>>>> the whole process, at least for trivial patches like this one.
>>>> Hi Cristian,
>>>>
>>>> We have created a Pull request in SOF git hub for I2S BT support.
>>>> please hold v2 version SOF patches till below PR get's merged.
>>>> PR:- https://github.com/thesofproject/linux/pull/4742
>>> Hi Venkata,
>>>
>>> If this is going to be handled via the github workflow, this patch
>>> should be removed from the series.  Since there is no dependency on it,
>>> I cannot see a reason to put v2 on hold.
>>>
>>> Do I miss something?
>> Non-sof driver related patches can directly send to broonie git ad v2
>> series.
>> SOF driver patches should send to SOF github to avoid merge conflicts
>> as  per guidelines of SOF community.
> Honestly, I don't really see a high risk of conflicts, the patches are
> not that complex and can be simply cherry-picked when needed.  Moreover,
> as we already had people reviewing this, splitting this up will only add
> confusion and unnecessary burden.
>
> Are there any specific changes you are concerned about and cannot be
> really handled here?
This is not the concern about this patch series,
Generally sof driver patches sends to SOF git hub as a PR, these are the 
guidelines by SOF maintainers.
If you still want to send alsa devel list directly, please discuss with 
SOF maintainers.

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Cristian Ciocaltea]

On 12/15/23 11:58, Venkata Prasad Potturu wrote:
> 
> On 12/14/23 22:12, Cristian Ciocaltea wrote:
>> On 12/14/23 15:15, Venkata Prasad Potturu wrote:
>>> On 12/14/23 17:53, Cristian Ciocaltea wrote:
>>>> On 12/11/23 07:58, Venkata Prasad Potturu wrote:
>>>>> On 12/10/23 21:20, Cristian Ciocaltea wrote:
>>>>>> On 12/10/23 16:01, Mark Brown wrote:
>>>>>>> On Sun, Dec 10, 2023 at 12:12:53PM +0200, Cristian Ciocaltea wrote:
>>>>>>>> On 12/10/23 11:51, Venkata Prasad Potturu wrote:
>>>>>>>>> This should send to SOF git repo for rewiew, once SOF reviewers
>>>>>>>>> approved
>>>>>>>>> this, again need to send to broonie git.
>>>>>>>>> All the changes in sound/soc/sof/ path should go to SOF git.
>>>>>>>> Unfortunately I'm not familiar with the SOF dev workflow. So
>>>>>>>> it's not
>>>>>>>> enough to have this patch cc-ed to
>>>>>>>> sound-open-firmware@alsa-project.org?
>>>>>>> The SOF people basically do their own thing in github at
>>>>>>>
>>>>>>>       https://github.com/thesofproject/linux
>>>>>>>
>>>>>>> with a github workflow and submit their patches upstream in
>>>>>>> batches a
>>>>>>> few times a release, however my understanding is that their
>>>>>>> workflow can
>>>>>>> cope with things going in directly upstream as well.
>>>>>> Thanks for clarifying, Mark!  That would greatly simplify and speedup
>>>>>> the whole process, at least for trivial patches like this one.
>>>>> Hi Cristian,
>>>>>
>>>>> We have created a Pull request in SOF git hub for I2S BT support.
>>>>> please hold v2 version SOF patches till below PR get's merged.
>>>>> PR:- https://github.com/thesofproject/linux/pull/4742
>>>> Hi Venkata,
>>>>
>>>> If this is going to be handled via the github workflow, this patch
>>>> should be removed from the series.  Since there is no dependency on it,
>>>> I cannot see a reason to put v2 on hold.
>>>>
>>>> Do I miss something?
>>> Non-sof driver related patches can directly send to broonie git ad v2
>>> series.
>>> SOF driver patches should send to SOF github to avoid merge conflicts
>>> as  per guidelines of SOF community.
>> Honestly, I don't really see a high risk of conflicts, the patches are
>> not that complex and can be simply cherry-picked when needed.  Moreover,
>> as we already had people reviewing this, splitting this up will only add
>> confusion and unnecessary burden.
>>
>> Are there any specific changes you are concerned about and cannot be
>> really handled here?
> This is not the concern about this patch series,
> Generally sof driver patches sends to SOF git hub as a PR, these are the
> guidelines by SOF maintainers.
> If you still want to send alsa devel list directly, please discuss with
> SOF maintainers.

I think this series makes sense as a whole and it's best to be handled
here, as it only provides trivial fixes to issues found on mainline.

If the SOF workflow is unable to integrate fixes submitted upstream, I
would perceive that as a significant drawback of adhering to that
process.  It is hard to believe, though, that this is really the case.

Hence, I kindly ask everyone here to shed some light and help move this
forward.

Thank you,
Cristian

Re: [PATCH 11/11] ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT

[Mark Brown]

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

On Fri, Dec 15, 2023 at 12:57:34PM +0200, Cristian Ciocaltea wrote:

> If the SOF workflow is unable to integrate fixes submitted upstream, I
> would perceive that as a significant drawback of adhering to that
> process.  It is hard to believe, though, that this is really the case.

As far as I'm aware they can cope fine with that, though it does help if
people try to avoid needless collisions.  It *does* cause trouble to use
both github and upstream flows simultaneously and there's a preference
for pushing anything substantial through github but picking one or the
other works as far as I know.

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