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

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Fri, Oct 23, 2026 at 10:14:57AM +0200, Olivier Louvignes wrote:
> hw_params() derives the slot width from the frequency given to the DAI's
> set_sysclk(), which it takes for SCLK. Generic cards pass MCLK there: the
> simple-audio-card of a HiBy R1 (Ingenic X1600 as I2S master, 64 bit
> clocks per frame, mclk-fs = 512) hands over 22.5792 MHz, so the driver
> sets up 256-bit slots for a 44.1 kHz stream and the CS43131 plays white
> noise.
> 
> Add a set_tdm_slot() callback so the card can say the layout instead:
> two slots of 16 to 32 bits. When set, it takes precedence over the width
> derived from set_sysclk(); zero slots go back to deriving it.
> 
> Tested on a HiBy R1, on v7.3, with dai-tdm-slot-num = <2> and
> dai-tdm-slot-width = <32> on the codec node.
> 
> Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
> ---
> RFC because the way to set the layout is a choice: set_tdm_slot() is what
> simple-audio-card can express from the device tree (dai-tdm-slot-num and
> dai-tdm-slot-width on the codec node), without a board-specific machine
> driver. I would also be fine with the codec deriving the layout from
> the sample width, or from a bclk ratio set with set_bclk_ratio(), if you
> prefer one of those.

Adding TDM support to cs43130 seems fine, so more than happy to
work through the patch to add that.

However, using it to work around the machine driver setting the
wrong clocks feels kinda fragile. I feel like a more robust
solution might be to have the machine driver not call the
set_sysclk callbacks, then I think the code in the codec driver
already does what you want.

From a quick poke around the generic machine driver stuff it
looks like it keys all the set_sysclk stuff off mclk-fs, is that
needed on the host side or could you just skip setting that and
avoid needing to do the tdm workaround?

> Tested on v7.3 on a HiBy R1; build-tested on 734bef548a96 (v7.3-7363).
> Only the CS43131 is covered, as PCM at 44.1 and 48 kHz.
> 
>  sound/soc/codecs/cs43130.c | 50 ++++++++++++++++++++++++++++++--------
>  sound/soc/codecs/cs43130.h |  1 +
>  2 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
> index 48d88dad4ceb..8450794654c6 100644
> --- a/sound/soc/codecs/cs43130.c
> +++ b/sound/soc/codecs/cs43130.c
> @@ -952,18 +952,24 @@ static int cs43130_hw_params(struct snd_pcm_substream *substream,
>  		break;
>  	}
>  
> -	if (!sclk && cs43130->dais[dai->id].dai_mode == SND_SOC_DAIFMT_CBP_CFP)
> -		/* Calculate SCLK in master mode if unassigned */
> -		sclk = params_rate(params) * bitwidth_dai *
> -		       params_channels(params);
> +	if (cs43130->dais[dai->id].slot_width) {
> +		/* The slot width set with set_tdm_slot() */
> +		bitwidth_sclk = cs43130->dais[dai->id].slot_width;
> +	} else {
> +		if (!sclk && cs43130->dais[dai->id].dai_mode == SND_SOC_DAIFMT_CBP_CFP)
> +			/* Calculate SCLK in master mode if unassigned */
> +			sclk = params_rate(params) * bitwidth_dai *
> +			       params_channels(params);
> +
> +		if (!sclk) {
> +			/* at this point, SCLK must be set */
> +			dev_err(cs43130->dev, "SCLK freq is not set\n");
> +			return -EINVAL;
> +		}
>  
> -	if (!sclk) {
> -		/* at this point, SCLK must be set */
> -		dev_err(cs43130->dev, "SCLK freq is not set\n");
> -		return -EINVAL;
> +		bitwidth_sclk = (sclk / params_rate(params)) /
> +				params_channels(params);
>  	}
> -
> -	bitwidth_sclk = (sclk / params_rate(params)) / params_channels(params);
>  	if (bitwidth_sclk < bitwidth_dai) {
>  		dev_err(cs43130->dev, "Format not supported: SCLK freq is too low\n");
>  		return -EINVAL;
> @@ -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;

If we are adding the set TDM callback we might as well add full
support looks like it should be pretty simple for cs43130. The
positions are set pretty intuitively in cs43130_set_sp_fmt(),
so just need to save the slot numbers and swap them in for the
params_channels() stuff. But at the very least if we are adding
partial support we should check the masks == 0x3 here as well.

Thanks,
Charles