Re: [PATCH RFC v2 1/6] 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] Incorrect semantic definition and conflation of TDM RX/TX masks for the CPU capture path.
--

commit 14c4efbbf7408318c5b2824f99db089826af6b11
Author: David Heidelberg <david@ixit.cz>

ASoC: qcom: sdm845: Demystify TDM masks a bit

This commit attempts to clarify the TDM masks by defining individual bits
for left/right speakers and microphones 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]
Is this naming convention accurate for the hardware routing?

Microphones capture audio, meaning they transmit data to the CPU rather than
receive it. Naming this an RX mask seems to misrepresent the physical data
flow and makes the definitions confusing.

> +#define LEFT_SPK_TDM_TX_MASK	(BIT(4) | BIT(5))
> +#define RIGHT_SPK_TDM_TX_MASK	(BIT(6) | BIT(7))

[ ... ]

> @@ -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 this combination of masks correctly reflect the TDM slot usage for
capture in sdm845_tdm_snd_hw_params()?

It appears the speaker mask, which defines playback slots on the DOUT wire,
is being combined with the new microphone mask merely to mathematically
reconstruct the previous 0xf value.

Using an RX mask to configure the CPU transmit mask, which reads from the
DIN wire, makes the capture path configuration difficult to follow. Could
we define a proper set of TX masks for the capture path instead?

>  		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/20260903-pixel3-audio-v2-0-7c22ed915291@ixit.cz?part=1