[PATCH v2 1/2] ASoC: cs43130: Drop only the clock reference hw_params() took

[PATCH v2 1/2] ASoC: cs43130: Drop only the clock reference hw_params() took

[Olivier Louvignes]

cs43130_hw_params() and cs43130_dsd_hw_params() take a reference on the
internal clock (clk_req) and cs43130_hw_free() drops one, but hw_free()
can run twice for one hw_params(). When a stream's hw_params() fails,
soc_pcm_hw_params() rolls back the DAIs it had set up, the codec DAIs
included since they come before the CPU DAIs, and snd_pcm_hw_params()
then calls hw_free() once more, which reaches every DAI of the link
whether its hw_params() ran or not. Each failed hw_params() so left
clk_req one lower than it should be. An application that tries a format
the CPU DAI refuses before one it accepts then gets clk_req back to 0
after its successful hw_params(), so the next stream skips the switch
from the RC oscillator to the external MCLK.

Seen on a HiBy R1 (CS43131 on an Ingenic X1600 I2S controller): after
an S32_LE attempt the I2S controller refuses, then S24_LE, SYS_CLK_CTL_1
reads 0x06 instead of 0x04 while playing and every signal plays with
loud white noise mixed in.

Keep the DAIs holding a reference in a bitmap: a DAI takes one at most
once in hw_params() and hw_free() drops it only if held. A second
hw_params() without hw_free() no longer takes a second reference either.

Fixes: 8f1e5bf9b440 ("ASoC: cs43130: Add support for CS43130 codec")
Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
Changes in v2: rebased on broonie/sound for-next (8cf9607d2d85), where
v1 did not apply. The conflict was only context: the
.auto_selectable_formats lines now in the DAI ops. No change to the fix.

The double hw_free() comes from the core: soc_pcm_hw_params() rolls back
with rollback = 1, then snd_pcm_hw_params() calls hw_free() again and
soc_pcm_hw_clean() runs with rollback = 0, which reaches every DAI. Other
drivers that count references in hw_params() may see the same. If you
would rather that be fixed in soc-pcm, say so and I will look at it
instead.

Tested on v7.3 on a HiBy R1 (CS43131); build-tested with W=1 on
for-next.

 sound/soc/codecs/cs43130.c | 15 +++++++++++++++
 sound/soc/codecs/cs43130.h |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
index 7bbb2e83a1ff..7fb23522f081 100644
--- a/sound/soc/codecs/cs43130.c
+++ b/sound/soc/codecs/cs43130.c
@@ -820,6 +820,9 @@ static int cs43130_dsd_hw_params(struct snd_pcm_substream *substream,
 	u8 dsd_speed;
 
 	scoped_guard(mutex, &cs43130->clk_mutex) {
+		if (test_and_set_bit(dai->id, &cs43130->clk_dais))
+			goto clk_held;
+
 		if (!cs43130->clk_req) {
 			/* no DAI is currently using clk */
 			if (!(CS43130_MCLK_22M % params_rate(params)))
@@ -838,6 +841,7 @@ static int cs43130_dsd_hw_params(struct snd_pcm_substream *substream,
 		if (cs43130->clk_req == 2)
 			cs43130_pcm_dsd_mix(true, cs43130->regmap);
 	}
+clk_held:
 
 	switch (params_rate(params)) {
 	case 176400:
@@ -883,6 +887,9 @@ static int cs43130_hw_params(struct snd_pcm_substream *substream,
 	u8 dsd_speed;
 
 	scoped_guard(mutex, &cs43130->clk_mutex) {
+		if (test_and_set_bit(dai->id, &cs43130->clk_dais))
+			goto clk_held;
+
 		if (!cs43130->clk_req) {
 			/* no DAI is currently using clk */
 			if (!(CS43130_MCLK_22M % params_rate(params)))
@@ -901,6 +908,7 @@ static int cs43130_hw_params(struct snd_pcm_substream *substream,
 		if (cs43130->clk_req == 2)
 			cs43130_pcm_dsd_mix(true, cs43130->regmap);
 	}
+clk_held:
 
 	switch (dai->id) {
 	case CS43130_ASP_DOP_DAI:
@@ -990,6 +998,13 @@ static int cs43130_hw_free(struct snd_pcm_substream *substream,
 	struct cs43130_private *cs43130 = snd_soc_component_get_drvdata(component);
 
 	guard(mutex)(&cs43130->clk_mutex);
+	/*
+	 * ALSA calls hw_free() after a failed hw_params(), also when this DAI's
+	 * hw_params() never ran: drop only a reference it took.
+	 */
+	if (!test_and_clear_bit(dai->id, &cs43130->clk_dais))
+		return 0;
+
 	cs43130->clk_req--;
 	if (!cs43130->clk_req) {
 		/* no DAI is currently using clk */
diff --git a/sound/soc/codecs/cs43130.h b/sound/soc/codecs/cs43130.h
index dbdb5b262f1b..b0dba903403f 100644
--- a/sound/soc/codecs/cs43130.h
+++ b/sound/soc/codecs/cs43130.h
@@ -513,6 +513,8 @@ struct	cs43130_private {
 	/* shared by both DAIs */
 	struct mutex			clk_mutex;
 	int				clk_req;
+	/* DAIs holding one of clk_req's references, by DAI id */
+	unsigned long			clk_dais;
 	bool				pll_bypass;
 	struct completion		xtal_rdy;
 	struct completion		pll_rdy;

base-commit: 8cf9607d2d855629260fc78e6b54ea7a98525708
-- 
2.39.5

[PATCH v2 2/2] ASoC: cs43130: Mute the PCM path from hw_free() to prepare()

[Olivier Louvignes]

The PCM path is unmuted when DAPM powers it up and muted again only when
DAPM powers it down, pmdown_time after the stream closes. Meanwhile the
DAC plays whatever its serial port carries: on a HiBy R1 (CS43131 on an
Ingenic X1600 I2S controller), loud white noise while a stream is open
but stopped, as when paused, and until DAPM powers down after it closes.

Add a mute_stream() op, so that ASoC mutes the path at hw_free() and
unmutes it at prepare(). The op is not run at trigger time, since the
codec sits on I2C.

Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
Changes in v2: rebased on broonie/sound for-next, the ops now sit next to
.auto_selectable_formats. No other change.

Tested on v7.3 on a HiBy R1 (CS43131), by ear: no hiss while paused or
after the stream stops. Build-tested with W=1 on for-next.

 sound/soc/codecs/cs43130.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
index 7fb23522f081..919280711f8d 100644
--- a/sound/soc/codecs/cs43130.c
+++ b/sound/soc/codecs/cs43130.c
@@ -1624,6 +1624,20 @@ static int cs43130_set_sysclk(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+/*
+ * Mutes the PCM path from hw_free() to the next prepare(). Otherwise it stays
+ * unmuted until DAPM powers it down, pmdown_time after the stream closes, and
+ * plays what the serial port carries meanwhile: white noise on the HiBy R1.
+ */
+static int cs43130_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
+{
+	struct snd_soc_component *component = dai->component;
+	struct cs43130_private *cs43130 = snd_soc_component_get_drvdata(component);
+
+	return regmap_update_bits(cs43130->regmap, CS43130_PCM_PATH_CTL_1,
+				  CS43130_MUTE_MASK, mute ? CS43130_MUTE_EN : 0);
+}
+
 static const u64 cs43130_selectable_formats =
 	SND_SOC_POSSIBLE_DAIFMT_I2S	|
 	SND_SOC_POSSIBLE_DAIFMT_LEFT_J	|
@@ -1640,6 +1654,8 @@ 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,
+	.mute_stream	= cs43130_mute_stream,
+	.no_capture_mute = 1,
 	.auto_selectable_formats	= &cs43130_selectable_formats,
 	.num_auto_selectable_formats	= 1,
 };
-- 
2.39.5

Re: [PATCH v2 2/2] ASoC: cs43130: Mute the PCM path from hw_free() to prepare()

[Charles Keepax]

On Fri, Oct 23, 2026 at 10:29:41PM +0200, Olivier Louvignes wrote:
> The PCM path is unmuted when DAPM powers it up and muted again only when
> DAPM powers it down, pmdown_time after the stream closes. Meanwhile the
> DAC plays whatever its serial port carries: on a HiBy R1 (CS43131 on an
> Ingenic X1600 I2S controller), loud white noise while a stream is open
> but stopped, as when paused, and until DAPM powers down after it closes.
> 
> Add a mute_stream() op, so that ASoC mutes the path at hw_free() and
> unmutes it at prepare(). The op is not run at trigger time, since the
> codec sits on I2C.
> 
> Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
> ---
> Changes in v2: rebased on broonie/sound for-next, the ops now sit next to
> .auto_selectable_formats. No other change.
> 
> Tested on v7.3 on a HiBy R1 (CS43131), by ear: no hiss while paused or
> after the stream stops. Build-tested with W=1 on for-next.
> 
>  sound/soc/codecs/cs43130.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c
> index 7fb23522f081..919280711f8d 100644
> --- a/sound/soc/codecs/cs43130.c
> +++ b/sound/soc/codecs/cs43130.c
> @@ -1624,6 +1624,20 @@ static int cs43130_set_sysclk(struct snd_soc_dai *codec_dai,
>  	return 0;
>  }
>  
> +/*
> + * Mutes the PCM path from hw_free() to the next prepare(). Otherwise it stays
> + * unmuted until DAPM powers it down, pmdown_time after the stream closes, and
> + * plays what the serial port carries meanwhile: white noise on the HiBy R1.
> + */
> +static int cs43130_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct cs43130_private *cs43130 = snd_soc_component_get_drvdata(component);
> +
> +	return regmap_update_bits(cs43130->regmap, CS43130_PCM_PATH_CTL_1,
> +				  CS43130_MUTE_MASK, mute ? CS43130_MUTE_EN : 0);
> +}

I am slightly concerned about some variants of the chips
running additional register sequences around the mute, I
was not involved in the work on this driver so little unsure
how critical those are. Would it perhaps be slightly safer to
just remove use_pmdown_time? Thereby forcing the chip to
power down immediately.

Thanks,
Charles

> +
>  static const u64 cs43130_selectable_formats =
>  	SND_SOC_POSSIBLE_DAIFMT_I2S	|
>  	SND_SOC_POSSIBLE_DAIFMT_LEFT_J	|
> @@ -1640,6 +1654,8 @@ 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,
> +	.mute_stream	= cs43130_mute_stream,
> +	.no_capture_mute = 1,
>  	.auto_selectable_formats	= &cs43130_selectable_formats,
>  	.num_auto_selectable_formats	= 1,
>  };
> -- 
> 2.39.5
> 

Re: [PATCH v2 1/2] ASoC: cs43130: Drop only the clock reference hw_params() took

[Charles Keepax]

On Fri, Oct 23, 2026 at 10:29:40PM +0200, Olivier Louvignes wrote:
> cs43130_hw_params() and cs43130_dsd_hw_params() take a reference on the
> internal clock (clk_req) and cs43130_hw_free() drops one, but hw_free()
> can run twice for one hw_params(). When a stream's hw_params() fails,
> soc_pcm_hw_params() rolls back the DAIs it had set up, the codec DAIs
> included since they come before the CPU DAIs, and snd_pcm_hw_params()
> then calls hw_free() once more, which reaches every DAI of the link
> whether its hw_params() ran or not. Each failed hw_params() so left
> clk_req one lower than it should be. An application that tries a format
> the CPU DAI refuses before one it accepts then gets clk_req back to 0
> after its successful hw_params(), so the next stream skips the switch
> from the RC oscillator to the external MCLK.
> 
> Seen on a HiBy R1 (CS43131 on an Ingenic X1600 I2S controller): after
> an S32_LE attempt the I2S controller refuses, then S24_LE, SYS_CLK_CTL_1
> reads 0x06 instead of 0x04 while playing and every signal plays with
> loud white noise mixed in.
> 
> Keep the DAIs holding a reference in a bitmap: a DAI takes one at most
> once in hw_params() and hw_free() drops it only if held. A second
> hw_params() without hw_free() no longer takes a second reference either.
> 
> Fixes: 8f1e5bf9b440 ("ASoC: cs43130: Add support for CS43130 codec")
> Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
> ---

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles