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

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

[Olivier Louvignes]

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.

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;
+
+	cs43130->dais[dai->id].slot_width = slot_width;
+	return 0;
+}
+
 static const struct snd_soc_dai_ops cs43130_pcm_ops = {
 	.startup	= cs43130_pcm_startup,
 	.hw_params	= cs43130_hw_params,
 	.hw_free	= cs43130_hw_free,
 	.set_sysclk	= cs43130_set_sysclk,
 	.set_fmt	= cs43130_pcm_set_fmt,
+	.set_tdm_slot	= cs43130_set_tdm_slot,
 };
 
 static const struct snd_soc_dai_ops cs43130_dop_ops = {
diff --git a/sound/soc/codecs/cs43130.h b/sound/soc/codecs/cs43130.h
index dbdb5b262f1b..6026c14514c0 100644
--- a/sound/soc/codecs/cs43130.h
+++ b/sound/soc/codecs/cs43130.h
@@ -495,6 +495,7 @@ static const char *const cs43130_supply_names[CS43130_NUM_SUPPLIES] = {
 
 struct cs43130_dai {
 	unsigned int			sclk;
+	unsigned int			slot_width;
 	unsigned int			dai_format;
 	unsigned int			dai_mode;
 	unsigned int			dai_invert;

base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.39.5

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

[sashiko-bot]

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

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

[Charles Keepax]

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

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

[Olivier Louvignes]

Hi Charles,

On Fri, Oct 23, 2026 at 10:57:21AM +0100, Charles Keepax wrote:
> 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?

It is needed on the host side, I tried it on the R1 without mclk-fs:

- simple-audio-card then never sets the I2S clock, so it keeps whatever
  the previous kernel left (22.5792 MHz on one boot, 12.288 MHz on the
  next). At 12.288 MHz the X1600 I2S driver refuses 44.1 kHz and only
  48 kHz plays.

- with the tdm slots dropped as well, hw_params fails with "SCLK freq is
  not set": the codec only derives SCLK itself when it is clock master,
  and here the X1600 is.

> 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.

Makes sense, v2 follows with full TDM support (the slot numbers saved and
used in cs43130_set_sp_fmt()), within the 16 to 64 bit frames the clock
tables cover. I can only test two slots on this board though.

Thanks,
Olivier