Re: [PATCH RFC] ASoC: cs43130: Take the frame layout from set_tdm_slot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: cs43130: broken channel fixup and mask handling in cs43130_set_tdm_slot()
--

commit 4794a3406868264ed704b7b8312f7714c82ad623
Author: Olivier Louvignes <olivier@mgcrea.io>

ASoC: cs43130: Take the frame layout from set_tdm_slot

This commit adds a set_tdm_slot() callback to the cs43130 codec driver so
that generic sound cards can configure the frame layout directly as two slots
of 16 to 32 bits instead of deriving the slot width from set_sysclk().

> diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
> index 48d88dad4ceb0..8450794654c60 100644
> --- a/sound/soc/codecs/cs43130.c
> +++ b/sound/soc/codecs/cs43130.c
[ ... ]
> @@ -1609,12 +1615,36 @@ static int cs43130_set_sysclk(struct snd_soc_dai *codec_dai,
>  	return 0;
>  }
>  
> +/*
> + * Sets the frame layout directly: two slots of slot_width bits, which then
> + * takes precedence over the layout derived from the set_sysclk() frequency.
> + * Generic sound cards pass MCLK to set_sysclk(), not SCLK. Zero slots go
> + * back to the derived layout.
> +*/
> +static int cs43130_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
> +				unsigned int rx_mask, int slots, int slot_width)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct cs43130_private *cs43130 = snd_soc_component_get_drvdata(component);
> +
> +	if (!slots) {
> +		cs43130->dais[dai->id].slot_width = 0;
> +		return 0;
> +	}
> +	if (slots != 2 || slot_width < 16 || slot_width > 32)
> +		return -EINVAL;
> +
> +	cs43130->dais[dai->id].slot_width = slot_width;
> +	return 0;
> +}

[Severity: Medium]
Does configuring the layout through snd_soc_dai_set_tdm_slot() break mono
playback and allow invalid slot masks?

When simple-audio-card parses dai-tdm-slot-num = <2> and dai-tdm-slot-width =
<32> without dai-tdm-slot-tx-mask, simple_init_dai() calls
snd_soc_dai_set_tdm_slot(dai, 0, 0, 2, 32). Because cs43130_pcm_ops does not
implement xlate_tdm_slot_mask, snd_soc_xlate_tdm_slot_mask() sets tx_mask to
0x3, and snd_soc_dai_set_tdm_slot() stores 0x3 in
dai->stream[SNDRV_PCM_STREAM_PLAYBACK].tdm_mask.

During __soc_pcm_hw_params(), a non-zero tdm_mask causes
soc_pcm_codec_params_fixup() to overwrite params_channels() on the codec copy
of hw_params with hweight_long(tdm_mask) = 2 before invoking
cs43130_hw_params()->cs43130_set_sp_fmt():

sound/soc/codecs/cs43130.c:cs43130_set_sp_fmt() {
	...
	frm_size = bitwidth_sclk * params_channels(params);
	loc_ch1 = 0;
	loc_ch2 = bitwidth_sclk * (params_channels(params) - 1);
	...
}

For a 1-channel stream (permitted by cs43130_dai[0].playback.channels_min =
1), the CPU DAI receives 1 channel while cs43130_set_sp_fmt() sees 2
channels, setting loc_ch2 to bitwidth_sclk instead of 0 (where both channels
previously sampled slot 0 in mono) and programming frm_size for 2 channels.

Conversely, cs43130_set_tdm_slot() ignores tx_mask and rx_mask and accepts
any slot_width in 16..32. If a caller passes slots = 2 with a 1-bit tx_mask
such as 0x1, soc_pcm_codec_params_fixup() forces params_channels(params) to
1, so cs43130_set_sp_fmt() computes frm_size = bitwidth_sclk * 1 instead of
2 * slot_width. In I2S or LEFT_J mode with slot_width = 32, that programs
LRCK_HI_TIME equal to LRCK_PERIOD (31), or with slot_width = 24 in DSP mode,
causes switch (frm_size) in cs43130_set_sp_fmt() to fail with -EINVAL (whose
return value is ignored by cs43130_hw_params()).

Would set_bclk_ratio() avoid the TDM channel mask fixup in the ASoC core, or
if set_tdm_slot() is kept, should cs43130_set_tdm_slot() validate tx_mask and
slot_width while cs43130_set_sp_fmt() decouples the 2-slot frame size from
the active channel count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023081457.44579-1-olivier@mgcrea.io?part=1