Re: [PATCH RFC 10/23] ASoC: qcom: lpass-cdc-dma: add support for WSA codec DMA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: qcom: missing WSA_TX4 case in
  __lpass_get_codec_dma_intf_type()
- [Medium] ASoC: qcom: unchecked clk_prepare_enable() in
  lpass_cdc_dma_daiops_startup()
--

commit fe9e37da583fba7a4cfa98118783d3ccffae7524
Author: Neil Armstrong <neil.armstrong@linaro.org>

ASoC: qcom: lpass-cdc-dma: add support for WSA codec DMA

This commit adds WSA codec DMA interface plumbing and clock handling to
the LPASS codec DMA driver.

> diff --git a/sound/soc/qcom/lpass-cdc-dma.c b/sound/soc/qcom/lpass-cdc-dma.c
> index 2dc8c75c4bf08..903055e3188d8 100644
> --- a/sound/soc/qcom/lpass-cdc-dma.c
> +++ b/sound/soc/qcom/lpass-cdc-dma.c

[ ... ]

> @@ -68,49 +76,67 @@ static int __lpass_get_codec_dma_intf_type(int dai_id)
>  	case LPASS_CDC_DMA_RX0:
>  	case LPASS_CDC_DMA_TX0:
>  	case LPASS_CDC_DMA_VA_TX0:
> +	case LPASS_CDC_DMA_WSA_TX0:
> +	case LPASS_CDC_DMA_WSA_RX0:
>  		ret = LPASS_CDC_DMA_INTERFACE1;
>  		break;
>  	case LPASS_CDC_DMA_RX1:
>  	case LPASS_CDC_DMA_TX1:
>  	case LPASS_CDC_DMA_VA_TX1:
> +	case LPASS_CDC_DMA_WSA_RX1:
> +	case LPASS_CDC_DMA_WSA_TX1:
>  		ret = LPASS_CDC_DMA_INTERFACE2;
>  		break;
>  	case LPASS_CDC_DMA_RX2:
>  	case LPASS_CDC_DMA_TX2:
>  	case LPASS_CDC_DMA_VA_TX2:
> +	case LPASS_CDC_DMA_WSA_RX2:
> +	case LPASS_CDC_DMA_WSA_TX2:
>  		ret = LPASS_CDC_DMA_INTERFACE3;
>  		break;
>  	case LPASS_CDC_DMA_RX3:
>  	case LPASS_CDC_DMA_TX3:
>  	case LPASS_CDC_DMA_VA_TX3:
> +	case LPASS_CDC_DMA_WSA_RX3:
> +	case LPASS_CDC_DMA_WSA_TX3:
>  		ret = LPASS_CDC_DMA_INTERFACE4;
>  		break;
>  	case LPASS_CDC_DMA_RX4:
>  	case LPASS_CDC_DMA_TX4:
>  	case LPASS_CDC_DMA_VA_TX4:
> +	case LPASS_CDC_DMA_WSA_RX4:
>  		ret = LPASS_CDC_DMA_INTERFACE5;
>  		break;

[Severity: Medium]
Should LPASS_CDC_DMA_WSA_TX4 also be included in the
LPASS_CDC_DMA_INTERFACE5 case?

__lpass_get_dmactl_handle(), lpass_cdc_dma_daiops_startup(), and
lpass_cdc_dma_daiops_shutdown() all handle the full
LPASS_CDC_DMA_WSA_TX0 ... LPASS_CDC_DMA_WSA_TX8 range, but omitting
LPASS_CDC_DMA_WSA_TX4 here causes __lpass_get_codec_dma_intf_type() to
return -EINVAL and __lpass_platform_codec_intf_init() to abort without
configuring or enabling the DMA interface.

[ ... ]

> @@ -184,6 +210,17 @@ static int lpass_cdc_dma_daiops_startup(struct snd_pcm_substream *substream,
>  		clk_set_rate(drvdata->va_mem0, CODEC_MEM_HZ_NORMAL);
>  		clk_prepare_enable(drvdata->va_mem0);
>  		break;
> +	case LPASS_CDC_DMA_WSA_RX0 ... LPASS_CDC_DMA_WSA_RX9:
> +	case LPASS_CDC_DMA_WSA_TX0 ... LPASS_CDC_DMA_WSA_TX8:
> +		clk_set_rate(drvdata->codec_mem0, CODEC_MEM_HZ_NORMAL);	//??
> +		clk_set_rate(drvdata->codec_mem1, CODEC_MEM_HZ_NORMAL);	//??
> +		clk_set_rate(drvdata->codec_mem2, CODEC_MEM_HZ_NORMAL);	//??
> +		clk_set_rate(drvdata->codec_mem3, CODEC_MEM_HZ_NORMAL);	//??
> +		clk_prepare_enable(drvdata->codec_mem0);		//??
> +		clk_prepare_enable(drvdata->codec_mem1);		//??
> +		clk_prepare_enable(drvdata->codec_mem2);		//??
> +		clk_prepare_enable(drvdata->codec_mem3);		//??
> +		break;
>  	default:
>  		dev_err(soc_runtime->dev, "%s: invalid  interface: %d\n", __func__, dai->id);
>  		break;
> @@ -205,6 +242,13 @@ static void lpass_cdc_dma_daiops_shutdown(struct snd_pcm_substream *substream,
>  	case LPASS_CDC_DMA_VA_TX0 ... LPASS_CDC_DMA_VA_TX0:
>  		clk_disable_unprepare(drvdata->va_mem0);
>  		break;
> +	case LPASS_CDC_DMA_WSA_RX0 ... LPASS_CDC_DMA_WSA_RX9:
> +	case LPASS_CDC_DMA_WSA_TX0 ... LPASS_CDC_DMA_WSA_TX8:
> +		clk_disable_unprepare(drvdata->codec_mem3);
> +		clk_disable_unprepare(drvdata->codec_mem2);
> +		clk_disable_unprepare(drvdata->codec_mem1);
> +		clk_disable_unprepare(drvdata->codec_mem0);
> +		break;

[Severity: Medium]
What happens if clk_set_rate() or clk_prepare_enable() fails in
lpass_cdc_dma_daiops_startup()?

Ignoring errors here lets the stream proceed with unclocked LPM memory,
leaves any earlier codec_mem clocks enabled on partial failure, and
causes lpass_cdc_dma_daiops_shutdown() to call clk_disable_unprepare()
on clocks whose enable failed.  Later commit "ASoC: qcom: lpass: enable
the right codec_mem clock when usin LPM memory" removes the "//??"
comments and selects a single clock via lpass_cdc_dma_mem_clk(), but
still leaves clk_set_rate() and clk_prepare_enable() unchecked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=10