[PATCH 0/3] Improve pop performance of cs42l43 input path

[PATCH 0/3] Improve pop performance of cs42l43 input path

[Charles Keepax]

Some rework to improve the pop noises associated with bring up of both
the analogue and the digital input paths on cs42l43. As part of that
there is also a little refactoring of the DAPM mic event handler to
switch over to look tables rather than switch statements.

Charles Keepax (3):
  ASoC: cs42l43: Improve analogue mic pop performance
  ASoC: cs42l43: Factor out decimator startup data to an array
  ASoC: cs42l43: Improve pop performance of the input power up

 sound/soc/codecs/cs42l43.c | 187 +++++++++++++++++++++----------------
 sound/soc/codecs/cs42l43.h |   3 +-
 2 files changed, 107 insertions(+), 83 deletions(-)

-- 
2.47.3

[PATCH 1/3] ASoC: cs42l43: Improve analogue mic pop performance

[Charles Keepax]

Enable the quick charge feature of the micbias to ensure the bias is
more likely to be up to voltage before the recording starts. This
greatly improves the startup pop of the analogue microphones.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs42l43.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
index b6238f89c789c..90c83bb33d69b 100644
--- a/sound/soc/codecs/cs42l43.c
+++ b/sound/soc/codecs/cs42l43.c
@@ -2822,6 +2822,10 @@ static int cs42l43_codec_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_pm;
 
+	// Enable quick charge to improve analogue microphone startup
+	regmap_update_bits(cs42l43->regmap, CS42L43_ADC_B_CTRL1, 0x20, 0x20);
+	regmap_update_bits(cs42l43->regmap, CS42L43_ADC_B_CTRL2, 0x20, 0x20);
+
 	for (i = 0; i < ARRAY_SIZE(cs42l43_irqs); i++) {
 		ret = cs42l43_request_irq(priv, cs42l43_irqs[i].name,
 					  cs42l43_irqs[i].irq,
-- 
2.47.3

[PATCH 2/3] ASoC: cs42l43: Factor out decimator startup data to an array

[Charles Keepax]

Rather than large switches factor out an array with all the data
necessary for the decimator startup. This makes the code slightly more
readable and prepares for some incoming improvements.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs42l43.c | 150 +++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 82 deletions(-)

diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
index 90c83bb33d69b..17d55a3de9de3 100644
--- a/sound/soc/codecs/cs42l43.c
+++ b/sound/soc/codecs/cs42l43.c
@@ -1736,97 +1736,82 @@ static int cs42l43_hp_ev(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+struct decim_info {
+	unsigned int vreg;
+	unsigned int vmask;
+};
+
+struct decim_info cs42l43_decim_info[] = {
+	{
+		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
+		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
+		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
+		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
+		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43_DECIM_VOL_CTRL_CH3_CH4,
+		.vmask = CS42L43_DECIM3_VD_RAMP_MASK | CS42L43_DECIM3_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43_DECIM_VOL_CTRL_CH3_CH4,
+		.vmask = CS42L43_DECIM4_VD_RAMP_MASK | CS42L43_DECIM4_MUTE_MASK,
+	}
+};
+
+struct decim_info cs42l43b_decim_info[] = {
+	{
+		.vreg = CS42L43B_DECIM_VOL_CTRL_CH1_CH2,
+		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43B_DECIM_VOL_CTRL_CH1_CH2,
+		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43B_DECIM_VOL_CTRL_CH3_CH4,
+		.vmask = CS42L43_DECIM3_VD_RAMP_MASK | CS42L43_DECIM3_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43B_DECIM_VOL_CTRL_CH3_CH4,
+		.vmask = CS42L43_DECIM4_VD_RAMP_MASK | CS42L43_DECIM4_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43B_DECIM_VOL_CTRL_CH5_CH6,
+		.vmask = CS42L43B_DECIM5_PATH1_VOL_FALL_RATE_MASK | CS42L43B_DECIM5_MUTE_MASK,
+	},
+	{
+		.vreg = CS42L43B_DECIM_VOL_CTRL_CH5_CH6,
+		.vmask = CS42L43B_DECIM6_PATH1_VOL_FALL_RATE_MASK | CS42L43B_DECIM6_MUTE_MASK,
+	}
+};
+
 static int cs42l43_mic_ev(struct snd_soc_dapm_widget *w,
 			  struct snd_kcontrol *kcontrol, int event)
 {
 	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct cs42l43_codec *priv = snd_soc_component_get_drvdata(component);
+	struct decim_info *info = &cs42l43_decim_info[w->shift];
 	struct cs42l43 *cs42l43 = priv->core;
-	unsigned int reg, ramp, mute;
-	unsigned int *val;
+	unsigned int vval = CS42L43_DECIM1_MUTE_MASK | CS42L43_DECIM2_MUTE_MASK;
 	int ret;
 
-	if (cs42l43->variant_id == CS42L43_DEVID_VAL) {
-		switch (w->shift) {
-		case CS42L43_ADC1_EN_SHIFT:
-		case CS42L43_PDM1_DIN_L_EN_SHIFT:
-			reg = CS42L43_DECIM_VOL_CTRL_CH1_CH2;
-			ramp = CS42L43_DECIM1_VD_RAMP_MASK;
-			mute = CS42L43_DECIM1_MUTE_MASK;
-			val = &priv->decim_cache[0];
-			break;
-		case CS42L43_ADC2_EN_SHIFT:
-		case CS42L43_PDM1_DIN_R_EN_SHIFT:
-			reg = CS42L43_DECIM_VOL_CTRL_CH1_CH2;
-			ramp = CS42L43_DECIM2_VD_RAMP_MASK;
-			mute = CS42L43_DECIM2_MUTE_MASK;
-			val = &priv->decim_cache[1];
-			break;
-		case CS42L43_PDM2_DIN_L_EN_SHIFT:
-			reg = CS42L43_DECIM_VOL_CTRL_CH3_CH4;
-			ramp  = CS42L43_DECIM3_VD_RAMP_MASK;
-			mute = CS42L43_DECIM3_MUTE_MASK;
-			val = &priv->decim_cache[2];
-			break;
-		case CS42L43_PDM2_DIN_R_EN_SHIFT:
-			reg = CS42L43_DECIM_VOL_CTRL_CH3_CH4;
-			ramp = CS42L43_DECIM4_VD_RAMP_MASK;
-			mute = CS42L43_DECIM4_MUTE_MASK;
-			val = &priv->decim_cache[3];
-			break;
-		default:
-			dev_err(priv->dev, "Invalid microphone shift: %d\n", w->shift);
-			return -EINVAL;
-		}
-	} else if (cs42l43->variant_id == CS42L43B_DEVID_VAL) {
-		switch (w->shift) {
-		case CS42L43_ADC1_EN_SHIFT:
-			reg = CS42L43B_DECIM_VOL_CTRL_CH1_CH2;
-			ramp = CS42L43_DECIM1_VD_RAMP_MASK;
-			mute = CS42L43_DECIM1_MUTE_MASK;
-			val = &priv->decim_cache[0];
-			break;
-		case CS42L43_ADC2_EN_SHIFT:
-			reg = CS42L43B_DECIM_VOL_CTRL_CH1_CH2;
-			ramp = CS42L43_DECIM2_VD_RAMP_MASK;
-			mute = CS42L43_DECIM2_MUTE_MASK;
-			val = &priv->decim_cache[1];
-			break;
-		case CS42L43_PDM1_DIN_L_EN_SHIFT:
-			reg = CS42L43B_DECIM_VOL_CTRL_CH3_CH4;
-			ramp  = CS42L43_DECIM3_VD_RAMP_MASK;
-			mute = CS42L43_DECIM3_MUTE_MASK;
-			val = &priv->decim_cache[2];
-			break;
-		case CS42L43_PDM1_DIN_R_EN_SHIFT:
-			reg = CS42L43B_DECIM_VOL_CTRL_CH3_CH4;
-			ramp = CS42L43_DECIM4_VD_RAMP_MASK;
-			mute = CS42L43_DECIM4_MUTE_MASK;
-			val = &priv->decim_cache[3];
-			break;
-		case CS42L43_PDM2_DIN_L_EN_SHIFT:
-			reg = CS42L43B_DECIM_VOL_CTRL_CH5_CH6;
-			ramp  = CS42L43B_DECIM5_PATH1_VOL_FALL_RATE_MASK;
-			mute = CS42L43B_DECIM5_MUTE_MASK;
-			val = &priv->decim_cache[4];
-			break;
-		case CS42L43_PDM2_DIN_R_EN_SHIFT:
-			reg = CS42L43B_DECIM_VOL_CTRL_CH5_CH6;
-			ramp = CS42L43B_DECIM6_PATH1_VOL_FALL_RATE_MASK;
-			mute = CS42L43B_DECIM6_MUTE_MASK;
-			val = &priv->decim_cache[5];
-			break;
-		default:
-			dev_err(priv->dev, "Invalid microphone shift: %d\n", w->shift);
-			return -EINVAL;
-		}
-	} else {
-		return -EINVAL;
-	}
+	if (cs42l43->variant_id == CS42L43B_DEVID_VAL)
+		info = &cs42l43b_decim_info[w->shift];
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		ret = regmap_read(cs42l43->regmap, reg, val);
+		ret = regmap_read(cs42l43->regmap, info->vreg,
+				  &priv->decim_cache[w->shift]);
 		if (ret) {
 			dev_err(priv->dev,
 				"Failed to cache decimator settings: %d\n",
@@ -1834,10 +1819,11 @@ static int cs42l43_mic_ev(struct snd_soc_dapm_widget *w,
 			return ret;
 		}
 
-		regmap_update_bits(cs42l43->regmap, reg, mute | ramp, mute);
+		regmap_update_bits(cs42l43->regmap, info->vreg,  info->vmask, vval);
 		break;
 	case SND_SOC_DAPM_POST_PMU:
-		regmap_update_bits(cs42l43->regmap, reg, mute | ramp, *val);
+		regmap_update_bits(cs42l43->regmap, info->vreg, info->vmask,
+				   priv->decim_cache[w->shift]);
 		break;
 	default:
 		break;
-- 
2.47.3

[PATCH 3/3] ASoC: cs42l43: Improve pop performance of the input power up

[Charles Keepax]

cs42l43's decimator is particularly susceptible to discontinuities
during startup. To help reduce the pop noises adjust the high
pass filter settings whilst powering up the microphones and add
a sizable delay to let that filter settle, then restore the user
settings.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs42l43.c | 37 +++++++++++++++++++++++++++++++++++--
 sound/soc/codecs/cs42l43.h |  3 ++-
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
index 17d55a3de9de3..9576f7abba97f 100644
--- a/sound/soc/codecs/cs42l43.c
+++ b/sound/soc/codecs/cs42l43.c
@@ -1737,32 +1737,39 @@ static int cs42l43_hp_ev(struct snd_soc_dapm_widget *w,
 }
 
 struct decim_info {
+	unsigned int freg;
 	unsigned int vreg;
 	unsigned int vmask;
 };
 
 struct decim_info cs42l43_decim_info[] = {
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
 		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
 		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
 		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
 		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL2,
 		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
 		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL2,
 		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
 		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL3,
 		.vreg = CS42L43_DECIM_VOL_CTRL_CH3_CH4,
 		.vmask = CS42L43_DECIM3_VD_RAMP_MASK | CS42L43_DECIM3_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL4,
 		.vreg = CS42L43_DECIM_VOL_CTRL_CH3_CH4,
 		.vmask = CS42L43_DECIM4_VD_RAMP_MASK | CS42L43_DECIM4_MUTE_MASK,
 	}
@@ -1770,26 +1777,32 @@ struct decim_info cs42l43_decim_info[] = {
 
 struct decim_info cs42l43b_decim_info[] = {
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
 		.vreg = CS42L43B_DECIM_VOL_CTRL_CH1_CH2,
 		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL2,
 		.vreg = CS42L43B_DECIM_VOL_CTRL_CH1_CH2,
 		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL3,
 		.vreg = CS42L43B_DECIM_VOL_CTRL_CH3_CH4,
 		.vmask = CS42L43_DECIM3_VD_RAMP_MASK | CS42L43_DECIM3_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43_DECIM_HPF_WNF_CTRL4,
 		.vreg = CS42L43B_DECIM_VOL_CTRL_CH3_CH4,
 		.vmask = CS42L43_DECIM4_VD_RAMP_MASK | CS42L43_DECIM4_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43B_DECIM_HPF_WNF_CTRL5,
 		.vreg = CS42L43B_DECIM_VOL_CTRL_CH5_CH6,
 		.vmask = CS42L43B_DECIM5_PATH1_VOL_FALL_RATE_MASK | CS42L43B_DECIM5_MUTE_MASK,
 	},
 	{
+		.freg = CS42L43B_DECIM_HPF_WNF_CTRL6,
 		.vreg = CS42L43B_DECIM_VOL_CTRL_CH5_CH6,
 		.vmask = CS42L43B_DECIM6_PATH1_VOL_FALL_RATE_MASK | CS42L43B_DECIM6_MUTE_MASK,
 	}
@@ -1802,6 +1815,8 @@ static int cs42l43_mic_ev(struct snd_soc_dapm_widget *w,
 	struct cs42l43_codec *priv = snd_soc_component_get_drvdata(component);
 	struct decim_info *info = &cs42l43_decim_info[w->shift];
 	struct cs42l43 *cs42l43 = priv->core;
+	unsigned int fmask = CS42L43_DECIM_HPF_EN_MASK | CS42L43_DECIM_HPF_CF_MASK;
+	unsigned int fval = CS42L43_DECIM_HPF_EN_MASK | 3 << CS42L43_DECIM_HPF_CF_SHIFT;
 	unsigned int vval = CS42L43_DECIM1_MUTE_MASK | CS42L43_DECIM2_MUTE_MASK;
 	int ret;
 
@@ -1810,8 +1825,10 @@ static int cs42l43_mic_ev(struct snd_soc_dapm_widget *w,
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
+		priv->decim_pre_pmu = true;
+
 		ret = regmap_read(cs42l43->regmap, info->vreg,
-				  &priv->decim_cache[w->shift]);
+				  &priv->decim_cache[w->shift][0]);
 		if (ret) {
 			dev_err(priv->dev,
 				"Failed to cache decimator settings: %d\n",
@@ -1819,11 +1836,27 @@ static int cs42l43_mic_ev(struct snd_soc_dapm_widget *w,
 			return ret;
 		}
 
+		ret = regmap_read(cs42l43->regmap, info->freg,
+				  &priv->decim_cache[w->shift][1]);
+		if (ret) {
+			dev_err(priv->dev,
+				"Failed to cache decimator filter settings: %d\n", ret);
+			return ret;
+		}
+
 		regmap_update_bits(cs42l43->regmap, info->vreg,  info->vmask, vval);
+		regmap_update_bits(cs42l43->regmap, info->freg, fmask, fval);
 		break;
 	case SND_SOC_DAPM_POST_PMU:
+		if (priv->decim_pre_pmu) {
+			msleep(150);
+			priv->decim_pre_pmu = false;
+		}
+
+		regmap_update_bits(cs42l43->regmap, info->freg, fmask,
+				   priv->decim_cache[w->shift][1]);
 		regmap_update_bits(cs42l43->regmap, info->vreg, info->vmask,
-				   priv->decim_cache[w->shift]);
+				   priv->decim_cache[w->shift][0]);
 		break;
 	default:
 		break;
diff --git a/sound/soc/codecs/cs42l43.h b/sound/soc/codecs/cs42l43.h
index fd50feb26ae99..2dc8d530a1fc0 100644
--- a/sound/soc/codecs/cs42l43.h
+++ b/sound/soc/codecs/cs42l43.h
@@ -61,7 +61,8 @@ struct cs42l43_codec {
 	unsigned int refclk_freq;
 	struct completion pll_ready;
 
-	unsigned int decim_cache[6];
+	bool decim_pre_pmu;
+	unsigned int decim_cache[6][2];
 	unsigned int adc_ena;
 	unsigned int hp_ena;
 
-- 
2.47.3

Re: [PATCH 2/3] ASoC: cs42l43: Factor out decimator startup data to an array

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] ASoC: cs42l43: missing static const on decim_info lookup tables
--

commit aad56061661af8b4127504548c049c31c4e54dca
Author: Charles Keepax <ckeepax@opensource.cirrus.com>

ASoC: cs42l43: Factor out decimator startup data to an array

This commit replaces the variant-specific switch statements in
cs42l43_mic_ev() with the cs42l43_decim_info and cs42l43b_decim_info lookup
arrays indexed by w->shift.

> diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
> index 90c83bb33d69b..17d55a3de9de3 100644
> --- a/sound/soc/codecs/cs42l43.c
> +++ b/sound/soc/codecs/cs42l43.c
> @@ -1736,97 +1736,82 @@ static int cs42l43_hp_ev(struct snd_soc_dapm_widget *w,
>  	return 0;
>  }
>  
> +struct decim_info {
> +	unsigned int vreg;
> +	unsigned int vmask;
> +};
> +
> +struct decim_info cs42l43_decim_info[] = {
> +	{
> +		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> +		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> +		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> +		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> +		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43_DECIM_VOL_CTRL_CH3_CH4,
> +		.vmask = CS42L43_DECIM3_VD_RAMP_MASK | CS42L43_DECIM3_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43_DECIM_VOL_CTRL_CH3_CH4,
> +		.vmask = CS42L43_DECIM4_VD_RAMP_MASK | CS42L43_DECIM4_MUTE_MASK,
> +	}
> +};
> +
> +struct decim_info cs42l43b_decim_info[] = {
> +	{
> +		.vreg = CS42L43B_DECIM_VOL_CTRL_CH1_CH2,
> +		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43B_DECIM_VOL_CTRL_CH1_CH2,
> +		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43B_DECIM_VOL_CTRL_CH3_CH4,
> +		.vmask = CS42L43_DECIM3_VD_RAMP_MASK | CS42L43_DECIM3_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43B_DECIM_VOL_CTRL_CH3_CH4,
> +		.vmask = CS42L43_DECIM4_VD_RAMP_MASK | CS42L43_DECIM4_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43B_DECIM_VOL_CTRL_CH5_CH6,
> +		.vmask = CS42L43B_DECIM5_PATH1_VOL_FALL_RATE_MASK | CS42L43B_DECIM5_MUTE_MASK,
> +	},
> +	{
> +		.vreg = CS42L43B_DECIM_VOL_CTRL_CH5_CH6,
> +		.vmask = CS42L43B_DECIM6_PATH1_VOL_FALL_RATE_MASK | CS42L43B_DECIM6_MUTE_MASK,
> +	}
> +};
> +
>  static int cs42l43_mic_ev(struct snd_soc_dapm_widget *w,
>  			  struct snd_kcontrol *kcontrol, int event)
>  {
>  	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>  	struct cs42l43_codec *priv = snd_soc_component_get_drvdata(component);
> +	struct decim_info *info = &cs42l43_decim_info[w->shift];

[Severity: Low]
Should cs42l43_decim_info and cs42l43b_decim_info in
sound/soc/codecs/cs42l43.c be declared static const (and info in
cs42l43_mic_ev() be a const struct decim_info *)?

Both tables are only used within sound/soc/codecs/cs42l43.c and are never
modified. Without static const, they are placed in writable .data instead of
.rodata and trigger -Wmissing-variable-declarations and Sparse warnings when
building with W=1 or C=1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102155440.3995136-1-ckeepax@opensource.cirrus.com?part=2

Re: [PATCH 3/3] ASoC: cs42l43: Improve pop performance of the input power up

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: cs42l43: swapped filter registers for ADC2 and PDM1L in cs42l43_decim_info
- [Medium] ASoC: cs42l43: missing DAPM lock on Decimator HPF controls racing with cs42l43_mic_ev()
- [Low] ASoC: cs42l43: missing static const on cs42l43_decim_info tables
--

commit 09aa11a71081b83cbebd343e4e4609f427672725
Author: Charles Keepax <ckeepax@opensource.cirrus.com>

ASoC: cs42l43: Improve pop performance of the input power up

This commit adjusts the decimator high pass filter settings during
microphone startup in cs42l43_mic_ev(), waits 150ms for the filter to
settle, and then restores the cached user settings.

> diff --git a/sound/soc/codecs/cs42l43.c b/sound/soc/codecs/cs42l43.c
> index 17d55a3de9de3..9576f7abba97f 100644
> --- a/sound/soc/codecs/cs42l43.c
> +++ b/sound/soc/codecs/cs42l43.c
> @@ -1737,32 +1737,39 @@ static int cs42l43_hp_ev(struct snd_soc_dapm_widget *w,
>  }
>  
>  struct decim_info {
> +	unsigned int freg;
>  	unsigned int vreg;
>  	unsigned int vmask;
>  };
>  
>  struct decim_info cs42l43_decim_info[] = {
>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
>  		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
>  	},
>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
>  		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
>  	},
>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL2,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
>  		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
>  	},

[Severity: High]
Are the .freg assignments for entries 1 and 2 in cs42l43_decim_info[]
swapped?

cs42l43_mic_ev() indexes cs42l43_decim_info[] by w->shift from
CS42L43_BLOCK_EN3:

  0: CS42L43_ADC1_EN_SHIFT       -> Decimator 1
  1: CS42L43_ADC2_EN_SHIFT       -> Decimator 2 (.vmask uses DECIM2)
  2: CS42L43_PDM1_DIN_L_EN_SHIFT -> Decimator 1 (.vmask uses DECIM1)
  3: CS42L43_PDM1_DIN_R_EN_SHIFT -> Decimator 2 (.vmask uses DECIM2)

Entry 1 (ADC2, Decimator 2) sets .freg = CS42L43_DECIM_HPF_WNF_CTRL1, and
entry 2 (PDM1L, Decimator 1) sets .freg = CS42L43_DECIM_HPF_WNF_CTRL2.

When ADC2 or PDM1L powers up individually on CS42L43, cs42l43_mic_ev()
adjusts the HPF register of the wrong decimator channel.

Furthermore, when stereo ADC1 + ADC2 (shifts 0 and 1) or PDM1L + PDM1R
(shifts 2 and 3) power up together in a coalesced DAPM sequence via
dapm_seq_run_coalesced(), SND_SOC_DAPM_PRE_PMU runs for both widgets
before SND_SOC_DAPM_POST_PMU runs for either. Because both widgets in the
pair point info->freg to the same register, the first PRE_PMU caches the
user setting and writes the temporary fval, and the second PRE_PMU then
reads that temporary fval into priv->decim_cache[w->shift][1]. When
SND_SOC_DAPM_POST_PMU runs, the second widget writes fval back to
info->freg, permanently overwriting the user's HPF configuration while the
other decimator channel's HPF was never adjusted.

>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL2,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
>  		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
>  	},
>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL3,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH3_CH4,
>  		.vmask = CS42L43_DECIM3_VD_RAMP_MASK | CS42L43_DECIM3_MUTE_MASK,
>  	},
>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL4,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH3_CH4,
>  		.vmask = CS42L43_DECIM4_VD_RAMP_MASK | CS42L43_DECIM4_MUTE_MASK,
>  	}
> @@ -1770,26 +1777,32 @@ struct decim_info cs42l43_decim_info[] = {
>  
>  struct decim_info cs42l43b_decim_info[] = {

[Severity: Low]
This isn't a bug, but should cs42l43_decim_info[] and
cs42l43b_decim_info[] (and the info pointer in cs42l43_mic_ev()) be
marked static const since they are only used inside sound/soc/codecs/cs42l43.c
and have no external declarations?

[ ... ]

> @@ -1819,11 +1836,27 @@ static int cs42l43_mic_ev(struct snd_soc_dapm_widget *w,
>  			return ret;
>  		}
>  
> +		ret = regmap_read(cs42l43->regmap, info->freg,
> +				  &priv->decim_cache[w->shift][1]);
> +		if (ret) {
> +			dev_err(priv->dev,
> +				"Failed to cache decimator filter settings: %d\n", ret);
> +			return ret;
> +		}
> +
>  		regmap_update_bits(cs42l43->regmap, info->vreg,  info->vmask, vval);
> +		regmap_update_bits(cs42l43->regmap, info->freg, fmask, fval);
>  		break;
>  	case SND_SOC_DAPM_POST_PMU:
> +		if (priv->decim_pre_pmu) {
> +			msleep(150);
> +			priv->decim_pre_pmu = false;
> +		}
> +
> +		regmap_update_bits(cs42l43->regmap, info->freg, fmask,
> +				   priv->decim_cache[w->shift][1]);
>  		regmap_update_bits(cs42l43->regmap, info->vreg, info->vmask,
> -				   priv->decim_cache[w->shift]);
> +				   priv->decim_cache[w->shift][0]);
>  		break;

[Severity: Medium]
Should the Decimator 1..6 HPF Switch and Decimator 1..6 HPF Corner
Frequency controls in cs42l43_controls[] and cs42l43_b_controls[] also use
cs42l43_dapm_get_volsw() / cs42l43_dapm_put_volsw() and
cs42l43_dapm_get_enum() / cs42l43_dapm_put_enum() to hold the DAPM mutex?

The other controls whose register bits are cached in SND_SOC_DAPM_PRE_PMU
and restored in SND_SOC_DAPM_POST_PMU (Decimator 1..6 Switch, Decimator
1..6 Ramp Up/Down, and EQ Switch) serialize kcontrol access under
snd_soc_dapm_mutex_lock(dapm).

Because the HPF controls in cs42l43_controls[] and cs42l43_b_controls[]
use plain SOC_SINGLE and SOC_ENUM:

	SOC_SINGLE("Decimator 1 HPF Switch", CS42L43_DECIM_HPF_WNF_CTRL1,
		   CS42L43_DECIM_HPF_EN_SHIFT, 1, 0),
	...
	SOC_ENUM("Decimator 1 HPF Corner Frequency", cs42l43_dec1_hpf_corner),

can a userspace read or write during the 150ms msleep() window in
cs42l43_mic_ev() observe the temporary fval override or have a newly
written HPF setting clobbered when SND_SOC_DAPM_POST_PMU restores
priv->decim_cache[w->shift][1]?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102155440.3995136-1-ckeepax@opensource.cirrus.com?part=3

Re: [PATCH 3/3] ASoC: cs42l43: Improve pop performance of the input power up

[Charles Keepax]

On Mon, Nov 02, 2026 at 04:15:47PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] ASoC: cs42l43: swapped filter registers for ADC2 and PDM1L in cs42l43_decim_info
> - [Medium] ASoC: cs42l43: missing DAPM lock on Decimator HPF controls racing with cs42l43_mic_ev()
> - [Low] ASoC: cs42l43: missing static const on cs42l43_decim_info tables
> --
> [Severity: High]
> Are the .freg assignments for entries 1 and 2 in cs42l43_decim_info[]
> swapped?
> 
> cs42l43_mic_ev() indexes cs42l43_decim_info[] by w->shift from
> CS42L43_BLOCK_EN3:
> 
>   0: CS42L43_ADC1_EN_SHIFT       -> Decimator 1
>   1: CS42L43_ADC2_EN_SHIFT       -> Decimator 2 (.vmask uses DECIM2)
>   2: CS42L43_PDM1_DIN_L_EN_SHIFT -> Decimator 1 (.vmask uses DECIM1)
>   3: CS42L43_PDM1_DIN_R_EN_SHIFT -> Decimator 2 (.vmask uses DECIM2)
> 
> Entry 1 (ADC2, Decimator 2) sets .freg = CS42L43_DECIM_HPF_WNF_CTRL1, and
> entry 2 (PDM1L, Decimator 1) sets .freg = CS42L43_DECIM_HPF_WNF_CTRL2.
> 
> When ADC2 or PDM1L powers up individually on CS42L43, cs42l43_mic_ev()
> adjusts the HPF register of the wrong decimator channel.
> 
> Furthermore, when stereo ADC1 + ADC2 (shifts 0 and 1) or PDM1L + PDM1R
> (shifts 2 and 3) power up together in a coalesced DAPM sequence via
> dapm_seq_run_coalesced(), SND_SOC_DAPM_PRE_PMU runs for both widgets
> before SND_SOC_DAPM_POST_PMU runs for either. Because both widgets in the
> pair point info->freg to the same register, the first PRE_PMU caches the
> user setting and writes the temporary fval, and the second PRE_PMU then
> reads that temporary fval into priv->decim_cache[w->shift][1]. When
> SND_SOC_DAPM_POST_PMU runs, the second widget writes fval back to
> info->freg, permanently overwriting the user's HPF configuration while the
> other decimator channel's HPF was never adjusted.

No, on cs42l43 the ADC and PDM are muxed so both can't be powered
up together and share the same decimator control bits.

> [Severity: Low]
> This isn't a bug, but should cs42l43_decim_info[] and
> cs42l43b_decim_info[] (and the info pointer in cs42l43_mic_ev()) be
> marked static const since they are only used inside sound/soc/codecs/cs42l43.c
> and have no external declarations?

Yeah these should be static const will fixup in a v2.

> [Severity: Medium]
> Should the Decimator 1..6 HPF Switch and Decimator 1..6 HPF Corner
> Frequency controls in cs42l43_controls[] and cs42l43_b_controls[] also use
> cs42l43_dapm_get_volsw() / cs42l43_dapm_put_volsw() and
> cs42l43_dapm_get_enum() / cs42l43_dapm_put_enum() to hold the DAPM mutex?
> 
> The other controls whose register bits are cached in SND_SOC_DAPM_PRE_PMU
> and restored in SND_SOC_DAPM_POST_PMU (Decimator 1..6 Switch, Decimator
> 1..6 Ramp Up/Down, and EQ Switch) serialize kcontrol access under
> snd_soc_dapm_mutex_lock(dapm).
> 
> Because the HPF controls in cs42l43_controls[] and cs42l43_b_controls[]
> use plain SOC_SINGLE and SOC_ENUM:
> 
> 	SOC_SINGLE("Decimator 1 HPF Switch", CS42L43_DECIM_HPF_WNF_CTRL1,
> 		   CS42L43_DECIM_HPF_EN_SHIFT, 1, 0),
> 	...
> 	SOC_ENUM("Decimator 1 HPF Corner Frequency", cs42l43_dec1_hpf_corner),
> 
> can a userspace read or write during the 150ms msleep() window in
> cs42l43_mic_ev() observe the temporary fval override or have a newly
> written HPF setting clobbered when SND_SOC_DAPM_POST_PMU restores
> priv->decim_cache[w->shift][1]?

Yeah this is also a good shout I will move these over to the dapm
version of the helper.

Thanks,
Charles

Re: [PATCH 3/3] ASoC: cs42l43: Improve pop performance of the input power up

[Mark Brown]

> cs42l43's decimator is particularly susceptible to discontinuities
> during startup. To help reduce the pop noises adjust the high
> pass filter settings whilst powering up the microphones and add
> a sizable delay to let that filter settle, then restore the user
> settings.

>  struct decim_info cs42l43_decim_info[] = {
>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
>  		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
>  	},
>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
>  		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
>  	},
>  	{
> +		.freg = CS42L43_DECIM_HPF_WNF_CTRL2,
>  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
>  		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
>  	},

Sashiko is flagging a bunch of concerns about the accuracy of this
table:

   https://sashiko.dev/#/patchset/20261102155440.3995136-1-ckeepax%40opensource.cirrus.com

-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH 2/3] ASoC: cs42l43: Factor out decimator startup data to an array

[Mark Brown]

> Rather than large switches factor out an array with all the data
> necessary for the decimator startup. This makes the code slightly more
> readable and prepares for some incoming improvements.

> +struct decim_info cs42l43_decim_info[] = {
> +	{

> +struct decim_info cs42l43b_decim_info[] = {
> +	{

These could be static.

-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH 3/3] ASoC: cs42l43: Improve pop performance of the input power up

[Charles Keepax]

On Mon, Nov 02, 2026 at 05:07:01PM +0000, Mark Brown wrote:
> > cs42l43's decimator is particularly susceptible to discontinuities
> > during startup. To help reduce the pop noises adjust the high
> > pass filter settings whilst powering up the microphones and add
> > a sizable delay to let that filter settle, then restore the user
> > settings.
> 
> >  struct decim_info cs42l43_decim_info[] = {
> >  	{
> > +		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
> >  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> >  		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
> >  	},
> >  	{
> > +		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
> >  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> >  		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
> >  	},
> >  	{
> > +		.freg = CS42L43_DECIM_HPF_WNF_CTRL2,
> >  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> >  		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
> >  	},
> 
> Sashiko is flagging a bunch of concerns about the accuracy of this
> table:
> 
>    https://sashiko.dev/#/patchset/20261102155440.3995136-1-ckeepax%40opensource.cirrus.com

Yeah replied on the list about that, the table is fine, as the
input is muxed. But the other two comments are good so will send
a v2 with those fixed up shortly.

Thanks,
Charles

Re: [PATCH 3/3] ASoC: cs42l43: Improve pop performance of the input power up

[Charles Keepax]

On Tue, Nov 03, 2026 at 09:39:43AM +0000, Charles Keepax wrote:
> On Mon, Nov 02, 2026 at 05:07:01PM +0000, Mark Brown wrote:
> > > cs42l43's decimator is particularly susceptible to discontinuities
> > > during startup. To help reduce the pop noises adjust the high
> > > pass filter settings whilst powering up the microphones and add
> > > a sizable delay to let that filter settle, then restore the user
> > > settings.
> > 
> > >  struct decim_info cs42l43_decim_info[] = {
> > >  	{
> > > +		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
> > >  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> > >  		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
> > >  	},
> > >  	{
> > > +		.freg = CS42L43_DECIM_HPF_WNF_CTRL1,
> > >  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> > >  		.vmask = CS42L43_DECIM2_VD_RAMP_MASK | CS42L43_DECIM2_MUTE_MASK,
> > >  	},
> > >  	{
> > > +		.freg = CS42L43_DECIM_HPF_WNF_CTRL2,
> > >  		.vreg = CS42L43_DECIM_VOL_CTRL_CH1_CH2,
> > >  		.vmask = CS42L43_DECIM1_VD_RAMP_MASK | CS42L43_DECIM1_MUTE_MASK,
> > >  	},
> > 
> > Sashiko is flagging a bunch of concerns about the accuracy of this
> > table:
> > 
> >    https://sashiko.dev/#/patchset/20261102155440.3995136-1-ckeepax%40opensource.cirrus.com
> 
> Yeah replied on the list about that, the table is fine, as the
> input is muxed. But the other two comments are good so will send
> a v2 with those fixed up shortly.

Although looking more closely maybe not, will check in a bit more
detail :-)

Thanks,
Charles