ASoC: SOF: Intel: reset SPIB in hda_data_stream_cleanup#5718
ASoC: SOF: Intel: reset SPIB in hda_data_stream_cleanup#5718bardliao wants to merge 1 commit intothesofproject:topic/sof-devfrom
Conversation
The register will indicate to the host DMA where the position is in the buffer currently processed by host SW. Reset it to 0 in hda_data_stream_cleanup. The register is ignored by the host DMA if SPIB is disabled. That's why the "enable" parameter needs to be HDA_DSP_SPIB_ENABLE. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the Intel SOF HDA stream cleanup path to reset the SPIB (Software Position In Buffer) register to 0 during hda_data_stream_cleanup(), based on the requirement that the SPIB value is only honored by host DMA when SPIB is enabled.
Changes:
- Add an SPIB “reset to 0” step in
hda_data_stream_cleanup()usingHDA_DSP_SPIB_ENABLE. - Keep the existing playback-specific SPIB disable call after cleanup.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* | ||
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled, | ||
| * That's why we need to set the value with HDA_DSP_SPIB_ENABLE | ||
| */ | ||
| hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_ENABLE, 0); | ||
|
|
||
| if (hstream->direction == SNDRV_PCM_STREAM_PLAYBACK) | ||
| ret = hda_dsp_stream_spib_config(sdev, hext_stream, HDA_DSP_SPIB_DISABLE, 0); |
There was a problem hiding this comment.
The new SPIB reset call enables SPIB unconditionally, but SPIB is only disabled again for playback streams. For capture streams (notably the ICCMAX capture stream path, which doesn’t enable SPIB in hda_data_stream_prepare()), this will leave SPIB enabled after cleanup and changes the stream’s register state unexpectedly. Consider limiting the reset to the playback case (where SPIB is disabled right after), or preserving/restoring the previous SPIB enable state (or explicitly disabling again for capture) so cleanup doesn’t introduce a persistent SPIB enable side effect.
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled, | ||
| * That's why we need to set the value with HDA_DSP_SPIB_ENABLE |
There was a problem hiding this comment.
Nit: comment grammar/casing — the sentence after the comma starts with "That's"; consider splitting into two sentences and avoiding the contraction (e.g., "That is why...") to match typical kernel comment style and improve readability.
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled, | |
| * That's why we need to set the value with HDA_DSP_SPIB_ENABLE | |
| * Reset SPIB. The SPIB value will only take effect when SPIB is enabled. | |
| * That is why we need to set the value with HDA_DSP_SPIB_ENABLE. |
The register will indicate to the host DMA where the position is in the buffer currently processed by host SW. Reset it to 0 in hda_data_stream_cleanup. The register is ignored by the host DMA if SPIB is disabled. That's why the "enable" parameter needs to be HDA_DSP_SPIB_ENABLE.