Use sof sdw as default sdw machine #5417

bardliao · 2025-05-16T07:55:49Z

Use sof_sdw as default Intel SoundWire machine driver when there is no link adr matches in the acpi match table.

Copilot

Pull Request Overview

This PR updates the default SoundWire machine driver configuration to use "sof_sdw" when no matching link ADR is found in the ACPI table. Key changes include new endpoint discovery functions in the Intel HDA driver, updates to symbol exports and cast assignments for link structures, and modifications to ACPI endpoint definitions across multiple files (including header changes to support a new "name_prefix" field).

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
sound/soc/sof/intel/hda.c	Adds new helper functions and creates a default SDW machine driver with updated cast assignments.
sound/soc/sof/amd/acp-common.c	Updates the assignment of mach_params.links with an explicit cast.
sound/soc/sdw_utils/soc_sdw_utils.c	Updates codec info structures with new "name_prefix" attributes and makes asoc_sdw_get_dai_type public.
Various ACPI match files	Removes const qualifiers from endpoint data and updates symbol exports.
include/sound/soc_sdw_utils.h & include/sound/soc-acpi.h	Updates structure definitions to support mutable link pointers and the new "name_prefix" field.

Comments suppressed due to low confidence (1)

include/sound/soc-acpi.h:84

The update from a const pointer to a mutable pointer for the 'links' field in struct snd_soc_acpi_mach_params should be reviewed and documented to ensure it is consistent with the intended immutability or mutability of endpoint data across the codebase.

struct snd_soc_acpi_link_adr *links;

Copilot · 2025-05-16T07:56:37Z

sound/soc/sof/intel/hda.c

@@ -1185,7 +1301,7 @@ static struct snd_soc_acpi_mach *hda_sdw_machine_select(struct snd_sof_dev *sdev
 				break;
 	}
 	if (mach && mach->link_mask) {
-		mach->mach_params.links = mach->links;
+		mach->mach_params.links = (struct snd_soc_acpi_link_adr *)mach->links;


The explicit cast from a const pointer to a mutable pointer for 'mach->mach_params.links' suggests an underlying type mismatch. Consider updating the structure definitions to align the types, which would avoid the need for the cast.

Copilot · 2025-05-16T07:56:37Z

include/sound/soc_sdw_utils.h

@@ -67,6 +67,7 @@ struct asoc_sdw_codec_info {
 	const int part_id;
 	const int version_id;
 	const char *codec_name;
+	const char *name_prefix;


A new field 'name_prefix' has been added to asoc_sdw_codec_info without accompanying documentation. Please add a descriptive comment to clarify its intended purpose and usage.

lgirdwood

Only minor questions. @charleskeepax good for you ?

lgirdwood · 2025-05-26T16:33:37Z

sound/soc/sof/intel/hda.c

+		return NULL;
+
+	for (i = 0; i < asoc_sdw_get_codec_info_list_count(); i++) {
+		if (sdw_device->id.part_id == codec_info_list[i].part_id) {


if (!) continue would help with indentation.

sound/soc/sof/intel/hda.c

bardliao · 2025-05-27T06:00:15Z

@lgirdwood comments added

charleskeepax · 2025-05-27T15:19:38Z

sound/soc/amd/acp/amd-acp63-acpi-match.c

@@ -9,21 +9,21 @@
 #include <sound/soc-acpi.h>
 #include "../mach-config.h"

-static const struct snd_soc_acpi_endpoint single_endpoint = {
+static struct snd_soc_acpi_endpoint single_endpoint = {


Do we actually need to remove the const on all of these hard coded entries? At least from the description where we are only creating a match if one doesn't exist that doesn't imply we would be changing an existing hard coded entry?

The problem is that I don't know how to convert a const struct to a non-const struct. For example, I will get below compile error if I don't remove the const of cs42l43_endpoints and try to force cast the type when set the .endpoints.

sound/soc/intel/common/soc-acpi-intel-arl-match.c:200:38: error: conversion to non-scalar type requested 200 | .endpoints = (struct snd_soc_acpi_endpoint)cs42l43_endpoints, | ^~~~~~~~~~~~~~~~~~~~~

It would be great if you could provide me some advice. I am more than happy if we don't need to remove the const on all of those hard coded entries.

charleskeepax · 2025-05-27T15:27:10Z

sound/soc/sof/intel/hda.c

+
+	links = devm_kcalloc(sdev->dev, peripherals->num_peripherals, sizeof(*links), GFP_KERNEL);
+	if (!links)
+		return NULL;


Is this right? I feel like num_peripherals is the number of devices but links are the soundwire links in usage? Is the naming slightly confusing or are we allocating the right sizes here?

My idea was that as we don't know the link numbers at this moment. Allocating the number of devices can guarantee that we allocate enough space for the links array. But, I use a loop to get the number of the links now. Thanks for pointing it out.

bardliao · 2025-05-28T08:24:22Z

SOFCI TEST

charleskeepax · 2025-05-28T10:27:12Z

sound/soc/sof/intel/hda.c

+
+		adr_dev[index].endpoints =
+			devm_kzalloc(dev, codec_info_list[i].dai_num *
+				     sizeof(struct snd_soc_acpi_endpoint), GFP_KERNEL);


Is the problem here? If instead of assigning the endpoint pointer in adr_dev directly what if you made a local non-const pointer, then once you have filled in the endpoints you can then assign that to the const pointer in the adr_dev structure.

No, the problem is

struct snd_soc_acpi_adr_device { const u64 adr; const u8 num_endpoints; const struct snd_soc_acpi_endpoint *endpoints; const char *name_prefix; };

and

struct snd_soc_acpi_link_adr { const u32 mask; const u32 num_adr; const struct snd_soc_acpi_adr_device *adr_d; };

I will get assignment of read-only member error even if the adr_device and the link_adr themselves are not const.

Pretty sure the problem is there, this seems to build, although I haven't actually runtime tested it:

charleskeepax@ce250fb
https://github.com/charleskeepax/linux/commits/sof/fix/

And feel free squash that in where ever it goes.

Oh, yes. Just make required variables non const and keep snd_soc_acpi_endpoint and snd_soc_acpi_adr_device const. Thanks for the advice. I will revise my PR, thanks.

lgirdwood

LGTM

charleskeepax · 2025-05-30T12:24:47Z

sound/soc/sof/intel/hda.c

+		 */
+		if (i < peripherals->num_peripherals - 1 &&
+		    peripherals->array[i + 1]->bus->link_id != peripherals->array[i]->bus->link_id)
+			link_index++;


Rather than doing this which depends on the links being ordered, would it be reasonable to do something like:

link_index = link_mask & (BIT(peripherals->array[i]->bus->link_id) - 1);

Pretty sure that works.

But the link_index may exceed the array size if the link_id is not continued or it doesn't start from 0. Like link 2 and 3 are used.

Apologies it seems I forgot an hweight in my comment there:

link_index = hweight(link_mask & (BIT(peripherals->array[i]->bus->link_id) - 1));

For example a system using links 1,3 would look like:

link_index = hweight(link_mask & (BIT(link_id) - 1))) = hweight(0xA & (0x2 - 1)) = hweight(0xA & 0x1) = 0 link_index = hweight(link_mask & (BIT(link_id) - 1))) = hweight(0xA & (0x8 - 1)) = hweight(0xA & 0x3) = 1

It basically counts the number of used links below the current link to get the link_index.

Wow, that is clever. Thanks for the suggestion.

charleskeepax · 2025-05-30T12:25:47Z

sound/soc/sof/intel/hda.c

+				"link_index %d exceeds the link number %d link_mask #%x\n",
+				link_index, link_num, link_mask);
+			return NULL;
+		}


Then one can probably drop this check too, since link_mask is generated from the link_id's the only way you could hit this was if the peripheral list was modified in between.

charleskeepax · 2025-05-30T12:41:36Z

sound/soc/sof/intel/hda.c

+		if (sdw_device->id.part_id != codec_info_list[i].part_id)
+			continue;
+
+		int amp_group_id = 1;


would be nicer to put this before the if, with the endpoints variable.

charleskeepax · 2025-05-30T12:49:36Z

Couple of last minor comments from me, but otherwise looks good to me.

ujfalusi · 2025-06-02T08:14:25Z

include/sound/soc-acpi.h

-	const u32 mask;
-	const u32 num_adr;
+	u32 mask;
+	u32 num_adr;


nitpick: the commit message can mention that to achieve what is described, we will drop the const for some members.

ujfalusi · 2025-06-02T08:16:38Z

sound/soc/intel/common/sof-function-topology-lib.c

@@ -132,4 +132,4 @@ int sof_sdw_get_tplg_files(struct snd_soc_card *card, const struct snd_soc_acpi_

 	return tplg_num;
 }


commit message last sentence needs rewording

ujfalusi · 2025-06-02T08:22:02Z

sound/soc/sof/intel/hda.c

+		if (dai_type == dai_info->dais[i].dai_type)
+			return true;
+	}
+	dev_dbg(&sdw_device->dev, "Endpoint DAI type %d not found\n", dai_type);


this is not an error, right?

do we need to print this and would we want to print if it is found also?

No, it is not an error. I only print when the dai_type is not fond because that the dai_type will be found in most cases. And that is expected. Printing it makes users know exactly the reason why the endpoint doesn't show up.

ujfalusi · 2025-06-02T08:24:10Z

sound/soc/sof/intel/hda.c

+			continue;
+
+		endpoints = devm_kzalloc(dev, codec_info_list[i].dai_num *
+					 sizeof(struct snd_soc_acpi_endpoint), GFP_KERNEL);


devm_kcalloc() ?

check allocation fail

fixed, thanks.

ujfalusi · 2025-06-02T08:25:19Z

sound/soc/sof/intel/hda.c

+
+		/* Get name_prefix from codec_info_list[] */
+		adr_dev[index].name_prefix = devm_kasprintf(dev, GFP_KERNEL, "%s",
+							    codec_info_list[i].name_prefix);


check adr_dev[index].name_prefix == NULL ?

fixed, thanks.

ujfalusi · 2025-06-02T08:29:20Z

sound/soc/sof/intel/hda.c

+	 */
+	if (!strcmp(adr_dev[index].name_prefix, "AMP")) {
+		adr_dev[index].name_prefix = devm_kasprintf(dev, GFP_KERNEL, "%s%d",
+							    adr_dev[index].name_prefix,


we might want to free up the old adr_dev[index].name_prefix after this?

ujfalusi · 2025-06-02T08:31:10Z

sound/soc/sof/intel/hda.c

+			adr_dev[index].name_prefix = devm_kasprintf(dev, GFP_KERNEL, "%s%d",
+								    "AMP", *amp_index);
+			if (!adr_dev[index].name_prefix)
+				return NULL;


we seams to keep some overshadowed allocation around while the module is loaded, can this be optimized somehow, or do we care?

Move the common name_pfefix allocation after the special cases. So that we will allocate the name_prefix just once.

ujfalusi · 2025-06-02T08:34:59Z

sound/soc/sof/intel/hda.c

+	}
+
+	mach->drv_name = "sof_sdw";
+	mach->sof_tplg_filename = "sof-sdw-generic.tplg";


we do have this topology file already, right?

Not yet, I will submit it soon.

thesofproject/sof#10040

not clear what this generic topology is because it's only there to "avoid errors"....

Currently, we still test the existence of the topology file. That is the main reason of the tplg. But the topology still works for generic SDCA codecs.

ujfalusi · 2025-06-02T08:38:29Z

sound/soc/sof/intel/hda.c

+	mach->mach_params.platform = dev_name(sdev->dev);
+	mach->get_function_tplg_files = sof_sdw_get_tplg_files;
+	dev_info(sdev->dev, "Use default SDW machine driver %s topology: %s\n",
+		 mach->drv_name, mach->sof_tplg_filename);


Do we print when we are not using the 'default SDW machine driver sof_sdw toopology: sof-sdw-generic.tplg'?
Do we need to print this in info level?

Currently, we use predefined snd_soc_acpi_link_adr tables to match the link adr from ACPI table to select the machine driver and the topology. However, with the mechanism, we need to create the snd_soc_acpi_link_adr table for each audio config. The sof_sdw machine driver is used by almost all Intel platforms with SOF and we can load required topology file dynamically today. In other words, we can use sof_sdw machine driver as the default machine driver for Intel SOF SoundWire codecs and no need to create snd_soc_acpi_link_adr table for every new audio configs. To achieve it, we need to drop the const for some members and edit the link adr and acpi adr data to match the data from the ACPI table. Suggested-by: Charles Keepax <ckeepax@opensource.cirrus.com> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Currently, the codec name_prefix of Intel SoundWire machine driver is from the ACPI match table. We can have it in the asoc_sdw_codec_info struct as a default name_prefix of a codec if there is no corresponding audio config found in the ACPI match table. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

The sof_sdw_get_tplg_files function is a callback of snd_soc_acpi_mach. Export it to allow other modules to use it. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

asoc_sdw_get_dai_type() is quite useful to convert SDCA function types to SDW DAI types. It can be used by other drivers. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

plbossart

the problem statement is not clear, you are referring to 'common' uses, but we have lots of specific platforms with little to no SDCA information. I think this code is well-intended but is too optimistic for legacy platforms between CNL and MTL, where most of the ACPI information for SoundWire is garbage at best - as measured by the quirks to modify the deviceID...

plbossart · 2025-06-04T09:43:24Z

sound/soc/sof/intel/hda.c

+	}
+
+	mach->drv_name = "sof_sdw";
+	mach->sof_tplg_filename = "sof-sdw-generic.tplg";


not clear what this generic topology is because it's only there to "avoid errors"....

plbossart · 2025-06-04T09:45:22Z

sound/soc/sof/intel/hda.c

+{
+	int i;
+
+	for (i = 0; i < sdw_device->sdca_data.num_functions; i++) {


clearly this function is only intended to work on platforms which do have information on SDCA. I think what you are trying to do is simplify integration for NEW platforms where SDCA is well supported on Windows, and I think you'd want to place limits on when this code is used, i.e. don't use it before LNL.

Thanks @plbossart for pointing this out. Add if (chip->hw_ip_version < SOF_INTEL_ACE_2_0) test now.

If there is no SoundWire machine matches the existing acpi match table, get the required machine date from the acpi table and construct the link adrs and endpoints. Pass the data to the default Intel SoundWire machine driver. And we don't need to add new item to the acpi match table in common cases. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2025-06-05T06:22:16Z

the problem statement is not clear, you are referring to 'common' uses, but we have lots of specific platforms with little to no SDCA information. I think this code is well-intended but is too optimistic for legacy platforms between CNL and MTL, where most of the ACPI information for SoundWire is garbage at best - as measured by the quirks to modify the deviceID...

The intention is to give the machine driver selection function a second chance to use the default SDW machine driver when it can't find any matched machine driver from the ACPI match table. Then we don't need to add new match to the ACPI match table for new audio configs. I.e. We still need to add the match table in some special cases. But in general, we don't have to do so.

bardliao requested review from lgirdwood, plbossart, ranj063, kv2019i, dbaluta and ujfalusi as code owners May 16, 2025 07:55

bardliao requested review from Copilot, charleskeepax and ujfalusi and removed request for dbaluta, plbossart, ranj063, lgirdwood, ujfalusi and kv2019i May 16, 2025 07:56

Copilot AI reviewed May 16, 2025

View reviewed changes

bardliao force-pushed the use_sof_sdw_as_default_sdw_machine branch from 63ea027 to 7c63353 Compare May 16, 2025 10:17

lgirdwood reviewed May 26, 2025

View reviewed changes

bardliao force-pushed the use_sof_sdw_as_default_sdw_machine branch 2 times, most recently from 3ca3ebc to 630629d Compare May 27, 2025 05:29

bardliao requested a review from lgirdwood May 27, 2025 06:00

charleskeepax reviewed May 27, 2025

View reviewed changes

bardliao force-pushed the use_sof_sdw_as_default_sdw_machine branch from 630629d to 133b10f Compare May 28, 2025 06:48

charleskeepax reviewed May 28, 2025

View reviewed changes

lgirdwood reviewed May 28, 2025

View reviewed changes

bardliao force-pushed the use_sof_sdw_as_default_sdw_machine branch from 133b10f to 06ad996 Compare May 29, 2025 12:22

charleskeepax reviewed May 30, 2025

View reviewed changes

bardliao force-pushed the use_sof_sdw_as_default_sdw_machine branch from 06ad996 to d07b49e Compare June 2, 2025 07:43

ujfalusi reviewed Jun 2, 2025

View reviewed changes

bardliao added 4 commits June 2, 2025 16:57

ASoC: Intel: export sof_sdw_get_tplg_files

adf7143

The sof_sdw_get_tplg_files function is a callback of snd_soc_acpi_mach. Export it to allow other modules to use it. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ASoC: soc_sdw_utils: export asoc_sdw_get_dai_type

5b31d21

asoc_sdw_get_dai_type() is quite useful to convert SDCA function types to SDW DAI types. It can be used by other drivers. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the use_sof_sdw_as_default_sdw_machine branch 3 times, most recently from 05a280a to 0bff9ef Compare June 3, 2025 02:06

plbossart reviewed Jun 4, 2025

View reviewed changes

bardliao force-pushed the use_sof_sdw_as_default_sdw_machine branch from 0bff9ef to 781e7b1 Compare June 5, 2025 06:11

		@@ -132,4 +132,4 @@ int sof_sdw_get_tplg_files(struct snd_soc_card *card, const struct snd_soc_acpi_

		return tplg_num;
		}

Use sof sdw as default sdw machine #5417

Are you sure you want to change the base?

Use sof sdw as default sdw machine #5417

Uh oh!

Conversation

bardliao commented May 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 16, 2025

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bardliao commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bardliao commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charleskeepax May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charleskeepax commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

charleskeepax May 29, 2025 •

edited

Loading