[PATCH v3] ASoC: cs43130: Take the frame layout from set_tdm_slot()

[PATCH v3] 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. The card cannot simply leave mclk-fs out: simple-audio-card then
sets no clock on the I2S controller either, and the codec, as clock
slave, fails hw_params() with "SCLK freq is not set".

Add a set_tdm_slot() callback so the card can give the layout instead.
The PCM DAI receives what it plays, so the two channels go to the first
and second slots set in rx_mask, within frames of 16 to 64 bits, the
sizes the serial port clock tables cover. When set, the layout takes
precedence over the one 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>
---
Changes in v3:
 - The slots come from rx_mask instead of tx_mask (Charles Keepax):
   set_tdm_slot() is device-centric, and the codec receives what it
   plays. A card that gives no masks, like the R1's, gets both filled
   with the slot count's mask by the core, so its layout is unchanged.
 - Rebased on broonie/sound for-7.5; no longer an RFC, since the
   approach is the one Charles found fine.

Changes in v2, after Charles Keepax and the review bot:
 - Full TDM: the two channels go to the first and second slots set in
   tx_mask, with the frame length from the slot count, instead of only
   accepting two slots with mask 0x3. Limited to frames of 16 to 64 bits,
   the sizes cs43130_set_sp_fmt() has clock tables for.
 - The callback stays: on the R1, without mclk-fs simple-audio-card sets
   no clock on the I2S controller, and the codec as clock slave fails with
   "SCLK freq is not set".

Tested on v7.3 on a HiBy R1 with two 32-bit slots and no masks in the
device tree, the only layout this board can run: the DAC gets a 64-bit
frame with channel 2 at bit 32, in stereo at 44.1 kHz and mono at 48 kHz
(with v2). v3 was run on the same board (v7.3, and the device tree
unchanged): the core passes 0x3 for both masks, and ASP_CH_1_LOC and
ASP_CH_2_LOC read 0x00 and 0x20 in stereo at 44.1 and 48 kHz, as with
v2. Other layouts are build-tested only, with W=1.

v2: https://lore.kernel.org/all/20261023163206.81891-1-olivier@mgcrea.io/
v1: https://lore.kernel.org/all/20261023081457.44579-1-olivier@mgcrea.io/

 sound/soc/codecs/cs43130.c | 76 +++++++++++++++++++++++++++++++-------
 sound/soc/codecs/cs43130.h |  3 ++
 2 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
index 7fb23522f081..3a1588645b40 100644
--- a/sound/soc/codecs/cs43130.c
+++ b/sound/soc/codecs/cs43130.c
@@ -651,9 +651,16 @@ static int cs43130_set_sp_fmt(int dai_id, unsigned int bitwidth_sclk,
 		return -EINVAL;
 	}
 
-	frm_size = bitwidth_sclk * params_channels(params);
-	loc_ch1 = 0;
-	loc_ch2 = bitwidth_sclk * (params_channels(params) - 1);
+	if (cs43130->dais[dai_id].slots) {
+		/* The layout set with set_tdm_slot() */
+		frm_size = bitwidth_sclk * cs43130->dais[dai_id].slots;
+		loc_ch1 = cs43130->dais[dai_id].slot_loc[0];
+		loc_ch2 = cs43130->dais[dai_id].slot_loc[1];
+	} else {
+		frm_size = bitwidth_sclk * params_channels(params);
+		loc_ch1 = 0;
+		loc_ch2 = bitwidth_sclk * (params_channels(params) - 1);
+	}
 
 	frm_data = frm_delay & CS43130_SP_FSD_MASK;
 	frm_data |= (frm_phase << CS43130_SP_STP_SHIFT) & CS43130_SP_STP_MASK;
@@ -960,18 +967,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;
@@ -1624,6 +1637,42 @@ static int cs43130_set_sysclk(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+/*
+ * Sets the frame layout directly, which then takes precedence over the one
+ * derived from the set_sysclk() frequency: generic sound cards pass MCLK to
+ * set_sysclk(), not SCLK. The PCM DAI only plays, so it receives: the two
+ * channels go to the first and second slots set in rx_mask (both to the same
+ * slot for one), and tx_mask is not used. Frames are 16 to 64 bits, the sizes
+ * the serial port clock tables cover. 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);
+	struct cs43130_dai *cs_dai = &cs43130->dais[dai->id];
+	unsigned int frm_size = slots * slot_width;
+
+	if (!slots) {
+		cs_dai->slots = 0;
+		cs_dai->slot_width = 0;
+		return 0;
+	}
+	if (slots < 0 || slot_width < 16 || slot_width > 32 ||
+	    frm_size > 64 || frm_size % 16)
+		return -EINVAL;
+	if (!rx_mask || hweight32(rx_mask) > 2 || rx_mask >= BIT(slots))
+		return -EINVAL;
+
+	cs_dai->slot_loc[0] = __ffs(rx_mask) * slot_width;
+	rx_mask &= rx_mask - 1;
+	cs_dai->slot_loc[1] = rx_mask ? __ffs(rx_mask) * slot_width :
+					cs_dai->slot_loc[0];
+	cs_dai->slots = slots;
+	cs_dai->slot_width = slot_width;
+	return 0;
+}
+
 static const u64 cs43130_selectable_formats =
 	SND_SOC_POSSIBLE_DAIFMT_I2S	|
 	SND_SOC_POSSIBLE_DAIFMT_LEFT_J	|
@@ -1640,6 +1689,7 @@ static const struct snd_soc_dai_ops cs43130_pcm_ops = {
 	.hw_free	= cs43130_hw_free,
 	.set_sysclk	= cs43130_set_sysclk,
 	.set_fmt	= cs43130_pcm_set_fmt,
+	.set_tdm_slot	= cs43130_set_tdm_slot,
 	.auto_selectable_formats	= &cs43130_selectable_formats,
 	.num_auto_selectable_formats	= 1,
 };
diff --git a/sound/soc/codecs/cs43130.h b/sound/soc/codecs/cs43130.h
index b0dba903403f..d94ce32e9571 100644
--- a/sound/soc/codecs/cs43130.h
+++ b/sound/soc/codecs/cs43130.h
@@ -495,6 +495,9 @@ static const char *const cs43130_supply_names[CS43130_NUM_SUPPLIES] = {
 
 struct cs43130_dai {
 	unsigned int			sclk;
+	unsigned int			slots;
+	unsigned int			slot_width;
+	u8				slot_loc[2];
 	unsigned int			dai_format;
 	unsigned int			dai_mode;
 	unsigned int			dai_invert;

base-commit: edd3a027969c7609a8738230f07e607852377bef
-- 
2.39.5

Re: [PATCH v3] ASoC: cs43130: Take the frame layout from set_tdm_slot()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] ASoC: cs43130: unchecked error returns in cs43130_hw_params()
  [1]
--

commit 7ef1cbcfb870223ab41a2a71fc03df6192352302
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 PCM DAI so
cards can specify the TDM frame layout directly instead of deriving the
slot width from the set_sysclk() frequency.

> diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
> index 7fb23522f081c..3a1588645b405 100644
> --- a/sound/soc/codecs/cs43130.c
> +++ b/sound/soc/codecs/cs43130.c

[ ... ]

> @@ -960,18 +967,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;

[Severity: Medium]
This problem wasn't introduced by this patch, but should cs43130_hw_params()
check the return values of its configuration helpers [1]?

sound/soc/codecs/cs43130.c:cs43130_hw_params() {
	...
			cs43130_set_pll(component, 0, 0, cs43130->mclk, required_clk);
			if (cs43130->pll_bypass)
				cs43130_change_clksrc(component, CS43130_MCLK_SRC_EXT);
			else
				cs43130_change_clksrc(component, CS43130_MCLK_SRC_PLL);
	...
	cs43130_set_bitwidth(dai->id, bitwidth_dai, cs43130->regmap);
	cs43130_set_sp_fmt(dai->id, bitwidth_sclk, params, cs43130);

	return 0;
}

If cs43130_set_sp_fmt() rejects frm_size or fails to find a matching
cs43130_get_clk_gen() entry, it returns -EINVAL after writing the frame and
clock configuration registers but before programming the clock generator.
Similarly, cs43130_set_pll(), cs43130_change_clksrc(), and
cs43130_set_bitwidth() can fail with -EINVAL or -ETIMEDOUT, yet
cs43130_hw_params() still returns 0 and leaves the codec unclocked or
partially configured.

[1] https://sashiko.dev/#/bug/linux-d87cb6e4-e153-44f3-bf43-c8028f4ce12f

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