Re: [PATCH v4 1/8] ASoC: qcom: sdm845: Demystify TDM masks a bit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] ASoC: qcom: Incorrect RX suffix for microphone TDM masks
--

commit 58ae8e5a0a62726195e8056775500ca69d67f4a4
Author: David Heidelberg <david@ixit.cz>

ASoC: qcom: sdm845: Demystify TDM masks a bit

This commit describes the bits used for each RX/TX TDM mask instead of
using hardcoded hex values.

> diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
> index 6843ab8ba017e..0ce8265ab1c1f 100644
> --- a/sound/soc/qcom/sdm845.c
> +++ b/sound/soc/qcom/sdm845.c
> @@ -23,9 +23,14 @@
>  #define DEFAULT_MCLK_RATE		24576000
>  #define TDM_BCLK_RATE		6144000
>  #define MI2S_BCLK_RATE		1536000
> -#define LEFT_SPK_TDM_TX_MASK    0x30
> -#define RIGHT_SPK_TDM_TX_MASK   0xC0
> -#define SPK_TDM_RX_MASK         0x03
> +#define LEFT_SPK_TDM_RX_MASK	BIT(0)
> +#define RIGHT_SPK_TDM_RX_MASK	BIT(1)
> +#define SPK_TDM_RX_MASK		(LEFT_SPK_TDM_RX_MASK | RIGHT_SPK_TDM_RX_MASK)
> +#define MIC1_TDM_RX_MASK	BIT(2)
> +#define MIC2_TDM_RX_MASK	BIT(3)
> +#define MIC_TDM_RX_MASK		(MIC1_TDM_RX_MASK | MIC2_TDM_RX_MASK)

[Severity: Low]
Are these microphone masks named correctly?

Microphones are capture devices that transmit data on the bus to the CPU,
meaning they utilize TX slots from the codec's perspective. 

While the bitwise values remain correct and do not introduce a functional
regression, naming these slots with an RX suffix appears to contradict
their semantic role. This is particularly noticeable since the stated goal
of this patch is to clarify the masks by describing their RX/TX directions.

> +#define LEFT_SPK_TDM_TX_MASK	(BIT(4) | BIT(5))
> +#define RIGHT_SPK_TDM_TX_MASK	(BIT(6) | BIT(7))
>  #define NUM_TDM_SLOTS           8
>  #define SLIM_MAX_TX_PORTS 16
>  #define SLIM_MAX_RX_PORTS 13

[ ... ]

> @@ -128,8 +133,10 @@ static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
>  			goto end;
>  		}
>  	} else {
> -		ret = snd_soc_dai_set_tdm_slot(cpu_dai, 0xf, 0,
> -				8, slot_width);
> +		ret = snd_soc_dai_set_tdm_slot(cpu_dai,
> +					       SPK_TDM_RX_MASK |
> +					       MIC_TDM_RX_MASK, 0,
> +					       NUM_TDM_SLOTS, slot_width);

[Severity: Low]
Does passing MIC_TDM_RX_MASK here create confusion in the capture path?

This passes the RX-suffixed microphone masks into the tx_mask parameter of
snd_soc_dai_set_tdm_slot() for the CPU capture DAI, since qdsp6 AFE expects
tx_mask for capture. 

Could this be renamed to MIC_TDM_TX_MASK to properly reflect its transmit
role and align with the parameter it is passed into?

>  		if (ret < 0) {
>  			dev_err(rtd->dev, "%s: failed to set tdm slot, err:%d\n",
>  					__func__, ret);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-pixel3-audio-v4-0-85b5d133aa5e@ixit.cz?part=1