ASoC: soc_sdw_utils: skip the endpoint that doesn't present by bardliao · Pull Request #5351 · thesofproject/linux

bardliao · 2025-03-06T12:43:02Z

We can check the DisCo table and know what functions are used by the codec. Then skip the unused endpoints. And we can remove the dai quirk in codec_info_list[].

ranj063 · 2025-03-06T16:17:07Z

sound/soc/sdw_utils/soc_sdw_utils.c

+				      class_id, adr_index))
 		return devm_kasprintf(dev, GFP_KERNEL, "sdw:0:%01x:%04x:%04x:%02x",
 				      link_id, mfg_id, part_id, class_id);
 	else


the else is redundant here

ranj063 · 2025-03-06T16:18:28Z

sound/soc/sdw_utils/soc_sdw_utils.c

+				if (!slave)
+					return -EINVAL;
+
+				/* Mack sure BIOS provides SDCA properties */


ranj063 · 2025-03-06T16:20:58Z

sound/soc/sdw_utils/soc_sdw_utils.c

+				for (k = 0; k < slave->sdca_data.num_functions; k++) {
+					int dai_type = asoc_sdw_get_dai_type(
+							slave->sdca_data.function[k].type);
+					dev_dbg(&slave->dev, "function: %s dai_type %d\n",


you need a newline here

And won't you be printing this debug for every endpoint match?

Yes, will remove.

ranj063 · 2025-03-06T16:22:52Z

sound/soc/sdw_utils/soc_sdw_utils.c

+					dev_dbg(&slave->dev, "function: %s dai_type %d\n",
+						slave->sdca_data.function[k].name, dai_type);
+					if (dai_type == dai_info->dai_type) {
+						dev_dbg(&slave->dev, "DAI type %d found\n",


Maybe say something along the lines of SDCA device function found ?

ranj063 · 2025-03-06T16:23:54Z

sound/soc/sdw_utils/soc_sdw_utils.c

+					}
+				}
+				if (k == slave->sdca_data.num_functions) {
+					dev_dbg(&slave->dev, "DAI type %d not found, skip endpoint\n",


Also here, maybe "SDCA device function for DAI type %d not supported, skip endpoint"

lgirdwood

May need a little more in the logs

lgirdwood · 2025-03-07T18:52:43Z

sound/soc/sdw_utils/soc_sdw_utils.c

 					continue;

+				/* No need to check SDCA functions if there is only 1 endpoint present */
+				if (adr_dev->num_endpoints == 1)


do we need a log entry here to state that only 1 endpoint is found and we assume it to be type X

sound/soc/sdw_utils/soc_sdw_utils.c

bardliao · 2025-03-10T01:17:42Z

Not sure what happened to PTLP_RVP_SDW tests. All playback/capture tests failed. And there is no dmesg. But, I run the test manually on the same DUT (ba-ptlh-rvp-01) and they passed. Also, the dmesg of https://sof-ci.01.org/linuxpr/PR5351/build6503/devicetest/index.html?model=PTLP_RVP_SDW&testcase=verify-kernel-boot-log looks good to me.

Mar 07 07:54:02 ba-ptlh-rvp-01 kernel: soundwire_bus:sdw_extract_slave_id: soundwire sdw-master-0-3: SDW Slave Addr: 330025d072201
Mar 07 07:54:02 ba-ptlh-rvp-01 kernel: soundwire_bus:sdw_extract_slave_id: soundwire sdw-master-0-3: SDW Slave class_id 0x01, mfg_id 0x025d, part_id 0x0722, unique_id 0x0, version 0x3
Mar 07 07:54:02 ba-ptlh-rvp-01 kernel: acpi device:1f: find_sdca_function: SDCA function UAJ (type 6) at 0x1
Mar 07 07:54:02 ba-ptlh-rvp-01 kernel: acpi device:20: find_sdca_function: SDCA function SmartMic (type 3) at 0x2
Mar 07 07:54:02 ba-ptlh-rvp-01 kernel: acpi device:21: find_sdca_function: SDCA function HID (type 10) at 0x3
Mar 07 07:54:02 ba-ptlh-rvp-01 kernel: acpi device:22: find_sdca_function: SDCA function SmartAmp (type 1) at 0x4

bardliao · 2025-03-10T01:17:51Z

SOFCI TEST

bardliao · 2025-03-10T06:50:10Z

SOFCI TEST

Currently asoc_sdw_get_codec_name will return codec_info->codec_name if it is set. However, in some case we need the sdw codec name no matter if codec_info->codec_name is set or not. _asoc_sdw_get_codec_name() will be used in the follow up commit. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2025-03-10T11:41:58Z

PTLP_RVP_SDW is not tested. Trigger CI test again.

bardliao · 2025-03-10T11:42:06Z

SOFCI TEST

ujfalusi

@bardliao, will this work with already release topology files (where split ones are not available)?

ujfalusi · 2025-03-10T12:22:57Z

sound/soc/sdw_utils/soc_sdw_utils.c

+				/* Make sure BIOS provides SDCA properties */
+				if (!slave->sdca_data.interface_revision) {
+					dev_warn(&slave->dev,
+						 "SDCA properties not found in the BIOS\n");


Is every sdw device is SDCA?
I think this should be at most in info or even dbg level.

Or check is it a SDCA codec first?

ujfalusi · 2025-03-10T12:24:25Z

sound/soc/sdw_utils/soc_sdw_utils.c

+						dai_info->dai_type);
+					continue;
+				}
+skip_sdca_function_check:


I don't know, but jumping within a loop does not sound right.
It might be better to move this new code into a sdca helper function.

bardliao · 2025-03-10T14:14:18Z

@bardliao, will this work with already release topology files (where split ones are not available)?

In theory, yes. We use DMI quirk to set optional SDCA DAI types for all existing. We should get the same result with this PR is the BIOS is configured properly.

ujfalusi · 2025-03-10T15:14:02Z

sound/soc/sdw_utils/soc_sdw_utils.c

+				if (ret < 0)
+					return ret;
+
+				if (!ret)


is_sdca_function_present()
ret < 0 is error
ret == 0 is true
ret == 1 is false
?
I would swap the true/false with a comment.

or not, I'm not sure what is going on ;)

My first thought wat let is_sdca_function_present() return true/false only and it will be Intuitive. However, I would like to include the error checking in is_sdca_function_present(), so I changed the return type to int. Maybe we should rename is_sdca_function_present() as it is not just checking is SDCA function present. The purpose of the helper is to decide whether we should skip the endpoint. We should only skip the endpoint if there are more than 1 endpoints and we can get the SDCA supported functions and none of the supported functions matches the DAI type.

ujfalusi · 2025-03-11T08:49:47Z

sound/soc/sdw_utils/soc_sdw_utils.c


+				/*
+				 * Don't check is sdca endpoint present if machine quirk is set
+				 * In other works, quirk should have higher priority than the sdca


typo: s/works/words

charleskeepax · 2025-03-11T13:41:11Z

sound/soc/sdw_utils/soc_sdw_utils.c

 					continue;

+				/*
+				 * Don't check is sdca endpoint present if machine quirk is set


*Don't check if

*Don't check if

Changed

ujfalusi · 2025-03-11T14:01:35Z

sound/soc/sdw_utils/soc_sdw_utils.c

 					continue;

+				/*
+				 * Don't check if sdca endpoint present if machine quirk is set


Don't check the sdca endpoint presence if machine quirk is set.

Or
Skip the sdca endpoint presence check if machine quirk is set.

A codec endpoint may not be used. We could check the present SDCA functions to know if the endpoint is used or not. Skip the endpoint which is not used. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ranj063 · 2025-03-12T15:09:13Z

@charleskeepax are you good with this PR?

ranj063 · 2025-03-14T15:31:08Z

@bardliao I see some failures on PTL SDW that I do not see in other PRs. Do you think it is related to the changes here?

bardliao · 2025-03-17T02:35:25Z

SOFCI TEST

bardliao · 2025-03-17T02:36:09Z

@bardliao I see some failures on PTL SDW that I do not see in other PRs. Do you think it is related to the changes here?

No, it is related to display audio

bardliao requested review from charleskeepax, ranj063 and ujfalusi March 6, 2025 12:43

bardliao requested review from dbaluta, kv2019i, lgirdwood and plbossart as code owners March 6, 2025 12:43

ranj063 reviewed Mar 6, 2025

View reviewed changes

bardliao force-pushed the sdca-endpoint-test branch 2 times, most recently from 5167bbe to b153c6f Compare March 7, 2025 07:00

lgirdwood reviewed Mar 7, 2025

View reviewed changes

bardliao force-pushed the sdca-endpoint-test branch from b153c6f to fc371ba Compare March 10, 2025 02:08

bardliao force-pushed the sdca-endpoint-test branch from fc371ba to 76db426 Compare March 10, 2025 06:57

lgirdwood previously approved these changes Mar 10, 2025

View reviewed changes

ujfalusi reviewed Mar 10, 2025

View reviewed changes

bardliao dismissed lgirdwood’s stale review via 8074dec March 10, 2025 14:40

bardliao force-pushed the sdca-endpoint-test branch from 76db426 to 8074dec Compare March 10, 2025 14:40

ujfalusi reviewed Mar 10, 2025

View reviewed changes

bardliao mentioned this pull request Mar 11, 2025

[DNM] Trace for dai not found #5356

Closed

bardliao force-pushed the sdca-endpoint-test branch 2 times, most recently from e3ee7ee to 5756cb1 Compare March 11, 2025 08:35

ujfalusi reviewed Mar 11, 2025

View reviewed changes

bardliao force-pushed the sdca-endpoint-test branch from 5756cb1 to bc0821a Compare March 11, 2025 08:55

ujfalusi previously approved these changes Mar 11, 2025

View reviewed changes

bardliao dismissed ujfalusi’s stale review via 9348f77 March 11, 2025 09:53

bardliao force-pushed the sdca-endpoint-test branch 2 times, most recently from 9348f77 to dc21a44 Compare March 11, 2025 10:43

charleskeepax reviewed Mar 11, 2025

View reviewed changes

bardliao force-pushed the sdca-endpoint-test branch from dc21a44 to 32d8a10 Compare March 11, 2025 13:54

ujfalusi reviewed Mar 11, 2025

View reviewed changes

ASoC: soc_sdw_utils: skip the endpoint that doesn't present

eae45e1

A codec endpoint may not be used. We could check the present SDCA functions to know if the endpoint is used or not. Skip the endpoint which is not used. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the sdca-endpoint-test branch from 32d8a10 to eae45e1 Compare March 12, 2025 01:16

ujfalusi approved these changes Mar 12, 2025

View reviewed changes

ranj063 approved these changes Mar 12, 2025

View reviewed changes

bardliao merged commit 7209bcb into thesofproject:topic/sof-dev Mar 17, 2025
12 of 15 checks passed

Conversation

bardliao commented Mar 6, 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!

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!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bardliao commented Mar 10, 2025

Uh oh!

bardliao commented Mar 10, 2025

Uh oh!

bardliao commented Mar 10, 2025

Uh oh!

bardliao commented Mar 10, 2025

Uh oh!

bardliao commented Mar 10, 2025

Uh oh!

ujfalusi 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!

bardliao commented Mar 10, 2025

Uh oh!

ujfalusi Mar 10, 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!

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!

ranj063 commented Mar 12, 2025

Uh oh!

ranj063 commented Mar 14, 2025

Uh oh!

bardliao commented Mar 17, 2025

Uh oh!

bardliao commented Mar 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ujfalusi Mar 10, 2025 •

edited

Loading