ASoC: SOF: add SOF_DBG_CHECK_SDW_PERIPHERAL debug flag#5741
ASoC: SOF: add SOF_DBG_CHECK_SDW_PERIPHERAL debug flag#5741bardliao wants to merge 1 commit intothesofproject:topic/sof-devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new SOF debug flag to optionally wait for SoundWire enumeration and filter out “ghost” peripherals listed in ACPI, avoiding machine-driver selection based on non-existent devices.
Changes:
- Introduces
SOF_DBG_CHECK_SDW_PERIPHERALdebug flag. - Adds an optional enumeration wait/verification step in
hda_sdw_machine_select()before generating ACPI link/ADR structures. - Adds a local enumeration timeout constant.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sound/soc/sof/sof-priv.h | Adds a new SOF debug flag definition controlling SoundWire peripheral presence checks. |
| sound/soc/sof/intel/hda.c | Implements the optional wait-for-enumeration logic during SoundWire default machine selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| time = wait_for_completion_timeout(&slave->enumeration_complete, | ||
| msecs_to_jiffies(ENUMERATION_TIMEOUT)); | ||
| if (!time) { | ||
| dev_warn(&slave->dev, "SoundWire peripheral is not present\n"); |
There was a problem hiding this comment.
The warning message is too generic to diagnose which ACPI-described peripheral was missing. Include identifying information (e.g. link_id, mfg_id/part_id/unique_id or dev_name) so users can correlate the warning with the ACPI table entry.
| dev_warn(&slave->dev, "SoundWire peripheral is not present\n"); | |
| dev_warn(&slave->dev, | |
| "SoundWire peripheral is not present on link %d (dev %s)\n", | |
| slave->bus->link_id, dev_name(&slave->dev)); |
There was a problem hiding this comment.
The device information is already included in slave->dev.
| for (i = 0; i < peripherals->num_peripherals; i++) { | ||
| slave = peripherals->array[i]; | ||
|
|
||
| /* Check is the peripheral is present */ |
There was a problem hiding this comment.
Typo/grammar in the new comment: "Check is the peripheral is present" should be "Check if the peripheral is present" (and keep SoundWire capitalization consistent).
| /* Check is the peripheral is present */ | |
| /* Check if the SoundWire peripheral is present */ |
| * DSPLESS_MODE is not set. | ||
| * No audio functionality when enabled. | ||
| */ | ||
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) /* Check is Soundwire peripherals are |
There was a problem hiding this comment.
Grammar/capitalization in the new flag description: "Check is Soundwire peripherals are" should be "Check if SoundWire peripherals are".
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) /* Check is Soundwire peripherals are | |
| #define SOF_DBG_CHECK_SDW_PERIPHERAL BIT(17) /* Check if SoundWire peripherals are |
| /* Check is the peripheral is present */ | ||
| if (sof_debug_check_flag(SOF_DBG_CHECK_SDW_PERIPHERAL) && !slave->dev_num_sticky) { | ||
| /* Wait for the peripheral to enumerate */ | ||
| time = wait_for_completion_timeout(&slave->enumeration_complete, | ||
| msecs_to_jiffies(ENUMERATION_TIMEOUT)); | ||
| if (!time) { | ||
| dev_warn(&slave->dev, "SoundWire peripheral is not present\n"); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The new continue when a SoundWire peripheral fails to enumerate can leave a link entry with num_adr == 0 while link_mask still includes that link. Several SoundWire helper paths iterate mach_params->links until num_adr == 0 (null-terminated contract), so an empty entry can prematurely terminate parsing and ignore later valid links/devices. Consider building a present_link_mask first (after the optional wait), allocating links based on that mask (+1 for a terminating entry), and only emitting link entries for links that have at least one present peripheral (and update mach->mach_params.link_mask accordingly).
| return adr_dev; | ||
| } | ||
|
|
||
| #define ENUMERATION_TIMEOUT 3000 |
There was a problem hiding this comment.
ENUMERATION_TIMEOUT is a very generic macro name and doesn’t document the unit. Rename it to something scoped (e.g. SOF_SDW_ENUMERATION_TIMEOUT_MS) and/or add a _MS suffix so it’s clear this value is in milliseconds.
| #define ENUMERATION_TIMEOUT 3000 | |
| #define SOF_SDW_ENUMERATION_TIMEOUT_MS 3000 |
If the flag is set, the driver waits for and verifies the presence of SoundWire peripherals listed in the ACPI table. This prevents the system from probing non-existent (ghost) SoundWire devices. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return adr_dev; | ||
| } | ||
|
|
||
| #define ENUMERATION_TIMEOUT 3000 |
There was a problem hiding this comment.
ENUMERATION_TIMEOUT is a very generic macro name and the units aren’t obvious at the definition site. Since it’s passed to msecs_to_jiffies(), consider renaming to something like HDA_SDW_ENUMERATION_TIMEOUT_MS (or adding a short comment) to make the units and scope clear and reduce the risk of collisions.
| #define ENUMERATION_TIMEOUT 3000 | |
| #define ENUMERATION_TIMEOUT 3000 /* milliseconds, used with msecs_to_jiffies() for HDA SoundWire enumeration */ |
If the flag is set, the driver waits for and verifies the presence of SoundWire peripherals listed in the ACPI table. This prevents the system from probing non-existent (ghost) SoundWire devices.