ASoC/soundwire: Intel: reset the PCMSyCM registers in hda_sdw_bpt_close#5715
ASoC/soundwire: Intel: reset the PCMSyCM registers in hda_sdw_bpt_close#5715bardliao wants to merge 1 commit intothesofproject:topic/sof-devfrom
Conversation
bardliao
requested review from
dbaluta,
kv2019i,
lgirdwood and
shumingfan
as code owners
There was a problem hiding this comment.
Pull request overview
This PR ensures Intel SoundWire BPT cleanup resets the PCMSyCM registers on stream close, matching the reset behavior already present for “normal” SoundWire streams.
Changes:
- Extend
hda_sdw_bpt_close()API to accept alink_id. - Reset PCMSyCM registers for TX/RX PDIs in
hda_sdw_bpt_close(). - Update in-tree callers to pass the SoundWire instance/link identifier.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sound/soc/sof/intel/hda-sdw-bpt.c | Adds PCMSyCM reset in close path and threads link_id into the close call. |
| include/sound/hda-sdw-bpt.h | Updates the exported/stub prototype for hda_sdw_bpt_close() with link_id. |
| drivers/soundwire/intel_ace2x.c | Updates callers to pass sdw->instance as the link_id argument. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sound/soc/sof/intel/hda-sdw-bpt.c
Outdated
| if (ret) | ||
| dev_err(dev, "%s: PCMSyCM PCMSyCM failed for TX: %d\n", | ||
| __func__, ret); | ||
|
|
||
| ret = hdac_bus_eml_sdw_map_stream_ch(sof_to_bus(sdev), link_id, | ||
| 1, /* cpu_dai->id -> PDI1 */ | ||
| 0, 0, SNDRV_PCM_STREAM_CAPTURE); | ||
| if (ret) |
There was a problem hiding this comment.
The return value from hdac_bus_eml_sdw_map_stream_ch() is checked with if (ret) here, but the rest of this file (and e.g. sound/soc/sof/intel/hda-dai.c) consistently treats errors as ret < 0. Please switch to ret < 0 to avoid flagging any non-negative return as an error and to keep error handling consistent.
| if (ret) | |
| dev_err(dev, "%s: PCMSyCM PCMSyCM failed for TX: %d\n", | |
| __func__, ret); | |
| ret = hdac_bus_eml_sdw_map_stream_ch(sof_to_bus(sdev), link_id, | |
| 1, /* cpu_dai->id -> PDI1 */ | |
| 0, 0, SNDRV_PCM_STREAM_CAPTURE); | |
| if (ret) | |
| if (ret < 0) | |
| dev_err(dev, "%s: PCMSyCM PCMSyCM failed for TX: %d\n", | |
| __func__, ret); | |
| ret = hdac_bus_eml_sdw_map_stream_ch(sof_to_bus(sdev), link_id, | |
| 1, /* cpu_dai->id -> PDI1 */ | |
| 0, 0, SNDRV_PCM_STREAM_CAPTURE); | |
| if (ret < 0) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sound/soc/sof/intel/hda-sdw-bpt.c
Outdated
| ret = hda_sdw_bpt_dma_deprepare(dev, bpt_rx_stream, dmab_rx_bdl); | ||
| /* | ||
| * In the case of SoundWire we need to reset the PCMSyCM registers. | ||
| * Need to continue deprepareing even if this fails. |
There was a problem hiding this comment.
Typo in comment: “deprepareing” should be corrected (e.g., “depreparing” / “depreparing the DMA buffers”).
| * Need to continue deprepareing even if this fails. | |
| * Need to continue depreparing even if this fails. |
sound/soc/sof/intel/hda-sdw-bpt.c
Outdated
| dev_err(dev, "%s: hdac_bus_eml_sdw_map_stream_ch failed %d\n", | ||
| __func__, ret); | ||
|
|
||
| ret1 = hdac_bus_eml_sdw_map_stream_ch(sof_to_bus(sdev), link_id, | ||
| 1, /* PDI1 */ | ||
| 0, 0, SNDRV_PCM_STREAM_CAPTURE); | ||
| if (ret1 < 0) { | ||
| dev_err(dev, "%s: hdac_bus_eml_sdw_map_stream_ch failed %d\n", |
There was a problem hiding this comment.
Both dev_err() messages are identical for the PDI0/playback and PDI1/capture reset calls, which makes logs ambiguous. Consider including the PDI index and/or direction in the error message so failures can be attributed to the correct stream.
| dev_err(dev, "%s: hdac_bus_eml_sdw_map_stream_ch failed %d\n", | |
| __func__, ret); | |
| ret1 = hdac_bus_eml_sdw_map_stream_ch(sof_to_bus(sdev), link_id, | |
| 1, /* PDI1 */ | |
| 0, 0, SNDRV_PCM_STREAM_CAPTURE); | |
| if (ret1 < 0) { | |
| dev_err(dev, "%s: hdac_bus_eml_sdw_map_stream_ch failed %d\n", | |
| dev_err(dev, "%s: hdac_bus_eml_sdw_map_stream_ch failed for PDI0/playback: %d\n", | |
| __func__, ret); | |
| ret1 = hdac_bus_eml_sdw_map_stream_ch(sof_to_bus(sdev), link_id, | |
| 1, /* PDI1 */ | |
| 0, 0, SNDRV_PCM_STREAM_CAPTURE); | |
| if (ret1 < 0) { | |
| dev_err(dev, "%s: hdac_bus_eml_sdw_map_stream_ch failed for PDI1/capture: %d\n", |
Resetting the PCMSyCM registers is required for Intel SoundWire stream. The same procedure is done in sdw_hda_dai_hw_params() for the normal SoundWire stream, too. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Resetting the PCMSyCM registers is required for Intel SoundWire stream. The same procedure is done in sdw_hda_dai_hw_params() for the normal SoundWire stream, too.