Re: [PATCH v8 07/10] ASoC: qcom: Add QAIF PCM operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] QAIF driver fails to restore hardware registers or SHRAM state on system resume
- [Low] Mismatch between register constant naming and the register being updated (`QAIF_DMACTL_WM_5` used for `DMACFG`)
--

commit 26a7723ec5d620bcd1ab1da632a18d127c2fe623
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF PCM operations

This commit adds the PCM platform callbacks needed to use QAIF DAIs for
playback and capture. It handles DMA buffer pre-allocation, stream
configuration, and one-time hardware initialization. It also adds support
for mmap on CIF and AIF paths.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> new file mode 100644
> index 0000000000000..e24c4252825e2
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-platform.c

[ ... ]

> @@ -0,0 +1,464 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + *
> + * qaif-platform.c -- ALSA SoC PCM platform driver for the Qualcomm Audio Interface (QAIF)
> + */

[ ... ]

> +static const struct snd_pcm_hardware qaif_platform_hardware = {
> +	.info			=	SNDRV_PCM_INFO_MMAP |
> +					SNDRV_PCM_INFO_MMAP_VALID |
> +					SNDRV_PCM_INFO_INTERLEAVED |
> +					SNDRV_PCM_INFO_PAUSE |
> +					SNDRV_PCM_INFO_RESUME,
> +	.formats		=	SNDRV_PCM_FMTBIT_S16 |

[Severity: High]
This structure advertises the hardware resume capability by setting
SNDRV_PCM_INFO_RESUME. Does this create a regression during system resume
where the hardware state is lost, because the driver does not provide the
required callbacks and state restoration logic to support it?

[ ... ]

> +static int qaif_init(struct snd_soc_component *component)
> +{
> +	struct qaif_drv_data *drvdata = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	if (drvdata->qaif_hw_configured)
> +		return 0;
> +
> +	ret = qaif_config_shram(drvdata);

[Severity: High]
During system suspend, the hardware typically loses its SHRAM configuration
and EE mapping when the power domain collapses. Since qaif_hw_configured is
never cleared during suspend, will qaif_init() return early without
re-applying the required one-time hardware configuration on resume?

Without restoring these registers, could this lead to a regression where
the hardware remains uninitialized and fails to operate correctly after a
system sleep cycle?

[ ... ]

> +static int qaif_platform_pcmops_hw_params(struct snd_soc_component *component,
> +					  struct snd_pcm_substream *substream,
> +					  struct snd_pcm_hw_params *params)
> +{

[ ... ]

> +	ret = regmap_update_bits(drvdata->audio_qaif_map,
> +				 qaif_dmacfg_reg(v, idx, substream->stream, dai_id),
> +				 QAIF_DMACFG_BURST4_BIT | QAIF_DMACFG_SHRAM_WM_MASK,
> +				 QAIF_DMACFG_BURST4_BIT | QAIF_DMACTL_WM_5);
> +	if (ret)
> +		dev_err(soc_runtime->dev, "error updating DMA CFG: %d\n", ret);

[Severity: Low]
Is the use of QAIF_DMACTL_WM_5 intentional here? It appears to be a constant
meant for a DMACTL register, but it is being supplied to a DMACFG register
operation (qaif_dmacfg_reg).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-b4-qaif-shikra-audio-v8-0-ac618057fe85@oss.qualcomm.com?part=7