ASoC: mediatek: mt8189: Improve error handling

From: bui duc phuc <[email protected]>

mt8189_afe_enable_top_cg() currently returns success when the AFE regmap is unavailable.

Return -EINVAL so callers can handle the error.

Fixes dc637ffeed6c ("ASoC: mediatek: mt8189: support audio clock control")

Signed-off-by bui duc phuc <[email protected]>

From: bui duc phuc <[email protected]>

mt8189_apll1_enable() and mt8189_apll2_enable() currently ignore errors from regmap_update_bits() and do not clean up resources when clock enable operations fail.

Propagate these errors and roll back the clocks and tuner state on errors.

Fixes dc637ffeed6c ("ASoC: mediatek: mt8189: support audio clock control")

Signed-off-by bui duc phuc <[email protected]>

---

Changes in v2:
- Update the names of the goto labels.

  sound/soc/mediatek/mt8189/mt8189-afe-clk.c | 78 ++++++++++++++++------
  1 file changed, 56 insertions(+), 22 deletions(-)

sound/soc/mediatek/mt8189/mt8189-afe-clk.c +29 -12

index 63e03a40dbbe..aaf4f7921363 100644
@@ -454,30 +454,47 @@ int mt8189_apll1_enable(struct mtk_base_afe *afe)
 
  	ret = mt8189_afe_enable_top_cg(afe, MT8189_CG_APLL1_CK);
  	if (ret)
-		return ret;
+		goto err_clear_mux_setting;
 
  	ret = mt8189_afe_enable_top_cg(afe, MT8189_PDN_APLL_TUNER1);
  	if (ret)
-		return ret;
+		goto err_disable_apll1_ck;
 
  	/* sel 44.1kHz:1, apll_div:7, upper bound:3 */
-	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG,
-			   XTAL_EN_128FS_SEL_MASK_SFT | APLL_DIV_MASK_SFT |
-			   UPPER_BOUND_MASK_SFT,
-			   (0x1 << XTAL_EN_128FS_SEL_SFT) | (7 << APLL_DIV_SFT) |
-			   (3 << UPPER_BOUND_SFT));
+	ret = regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG,
+				 XTAL_EN_128FS_SEL_MASK_SFT | APLL_DIV_MASK_SFT |
+				 UPPER_BOUND_MASK_SFT,
+				 (0x1 << XTAL_EN_128FS_SEL_SFT) | (7 << APLL_DIV_SFT) |
+				 (3 << UPPER_BOUND_SFT));
+	if (ret)
+		goto err_disable_apll_tuner1;
 
  	/* apll1 freq tuner enable */
-	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG,
-			   FREQ_TUNER_EN_MASK_SFT,
-			   0x1 << FREQ_TUNER_EN_SFT);
+	ret = regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG,
+				 FREQ_TUNER_EN_MASK_SFT,
+				 0x1 << FREQ_TUNER_EN_SFT);
+	if (ret)
+		goto err_disable_apll_tuner1;
 
  	/* audio apll1 on */
  	ret = mt8189_afe_enable_top_cg(afe, MT8189_AUDIO_APLL1_EN_ON);
  	if (ret)
-		return ret;
+		goto err_clear_freq_tuner_en;
 
  	return 0;
+
+err_clear_freq_tuner_en:
+	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG,
+			   FREQ_TUNER_EN_MASK_SFT,
+			   0x0);
+err_disable_apll_tuner1:
+	mt8189_afe_disable_top_cg(afe, MT8189_PDN_APLL_TUNER1);
+err_disable_apll1_ck:
+	mt8189_afe_disable_top_cg(afe, MT8189_CG_APLL1_CK);
+err_clear_mux_setting:
+	apll1_mux_setting(afe, false);
+
+	return ret;
  }
 
  void mt8189_apll1_disable(struct mtk_base_afe *afe)

@@ -506,30 +523,47 @@ int mt8189_apll2_enable(struct mtk_base_afe *afe)
 
  	ret = mt8189_afe_enable_top_cg(afe, MT8189_CG_APLL2_CK);
  	if (ret)
-		return ret;
+		goto err_clear_mux_setting;
 
  	ret = mt8189_afe_enable_top_cg(afe, MT8189_PDN_APLL_TUNER2);
  	if (ret)

sound/soc/mediatek/mt8189/mt8189-afe-clk.c +6 -0

+				 XTAL_EN_128FS_SEL_MASK_SFT | APLL_DIV_MASK_SFT |
+				 UPPER_BOUND_MASK_SFT,
+				 (0x2 << XTAL_EN_128FS_SEL_SFT) | (7 << APLL_DIV_SFT) |
+				 (3 << UPPER_BOUND_SFT));
+	if (ret)
+		goto err_disable_apll_tuner2;

Review

Quoted 1 line · On 9/14/26 09:28, [email protected] wrote:

(quoted diff) +1 -0

+	ret = regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,

Well, this is a bit of defensive programming here.

The regmap pointer is already checked by the previous function call, and this is a regmap over MMIO... and MMIO writes can't fail.

Quoted 8 lines · AngeloGioacchino Del Regno

Review

Quoted 1 line · On 9/14/26 09:28, [email protected] wrote:

(quoted diff) +1 -0

+	ret = regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,

Well, this is a bit of defensive programming here.

The regmap pointer is already checked by the previous function call, and this is a regmap over MMIO... and MMIO writes can't fail.

Oh, you sweet summer child :) .

Though practically speaking the error
handling ends up being the same as if they couldn't fail since if something goes wrong it's generally catastrophic stuff like locking the core up completely so the end result is the same.

Quoted 4 lines · Mark Brown

Though practically speaking the error
handling ends up being the same as if they couldn't fail since if something goes wrong it's generally catastrophic stuff like locking the core up completely so the end result is the same.

That was also an implicit point (that should've been explicit from me): if anything goes horribly wrong here, it means that it already went horribly wrong "some function calls ago", and the platform likely already locked up as you suggested.

In any case, I'm not against doing error checking, it's just about not doing it when it's really useless (I'm sure you understand my reasons), and I believe this specific case is one of those.

That said, should you prefer having error checks in such places... it's not a performance path, so I don't really have strong opinions really.

Quoted 13 lines

Quoted 5 lines

Though practically speaking the error
handling ends up being the same as if they couldn't fail since if something goes wrong it's generally catastrophic stuff like locking the core up completely so the end result is the same.

That was also an implicit point (that should've been explicit from me): if anything goes horribly wrong here, it means that it already went horribly wrong "some function calls ago", and the platform likely already locked up as you suggested.

In any case, I'm not against doing error checking, it's just about not doing it when it's really useless (I'm sure you understand my reasons), and I believe this specific case is one of those.

Quoted 2 lines

That said, should you prefer having error checks in such places... it's not a performance path, so I don't really have strong opinions really.

Yeah, I think it's fine and sensible to skip the error checks in cases where we can't really do anything constructive about the error.

Hi Mark , Angelo

Thanks both for the review.

Quoted 14 lines

Quoted 13 lines

Quoted 5 lines

Though practically speaking the error
handling ends up being the same as if they couldn't fail since if something goes wrong it's generally catastrophic stuff like locking the core up completely so the end result is the same.

That was also an implicit point (that should've been explicit from me): if anything goes horribly wrong here, it means that it already went horribly wrong "some function calls ago", and the platform likely already locked up as you suggested.

In any case, I'm not against doing error checking, it's just about not doing it when it's really useless (I'm sure you understand my reasons), and I believe this specific case is one of those.

Quoted 5 lines

Quoted 2 lines

That said, should you prefer having error checks in such places... it's not a performance path, so I don't really have strong opinions really.

Yeah, I think it's fine and sensible to skip the error checks in cases where we can't really do anything constructive about the error.

As far as I understand it, regmap_update_bits() doesn't go straight to MMIO. It first passes through the regmap core, which has checks independent of the underlying bus:

https://elixir.bootlin.com/linux/v7.3-rc2/source/drivers/base/regmap/regmap.c#L2834

----------------------------------------------
if (map->cache_only)

         return -EBUSY;

if (!regmap_readable(map, reg))

         return -EIO;

----------------------------------------------

So, in my understanding, checking ret here isn't just unnecessary defensive programming
for an MMIO write, it guards an assumption about PM/cache ordering, namely that the regmap is already out of cache_only mode by the time this runs.

If that assumption were ever violated by a bug elsewhere (

a PM race,

wrong resume ordering,

a register missing from readable_reg, etc.),

this is where it would
get caught and reported,

rather than the driver silently assuming the enable succeeded when it didn't.

Best regards,
Phuc

You have reasons, of course, but have a wider view and check the full picture:
apll{1,2}_enable() is called only by mtk_apll_event(), which is a DAPM supply.

That call can only happen if the device is not in suspended state, so can happen only after a call to

        regcache_cache_only(afe->regmap, false);
        regcache_sync(afe->regmap);

is done in mt8189_afe_runtime_resume().

Quoted 4 lines

If that assumption were ever violated by a bug elsewhere (

a PM race,

That would have worst consequences, and your system won't be saved by this error check because it would crash way before reaching that...

Quoted 1 line

wrong resume ordering,

If limited to ASoC, that would break most (if not all) sound drivers, as that'd happen at the API level, and would get fixed immediately since that'd affect way too many platforms and architectures.

Quoted 1 line

a register missing from readable_reg, etc.),

..and that would be a driver bug, which is not the case here: the error check though would be useful during development where you definitely want to paranoidly check for literally everything, even "useless" things... but this driver definitely is not in development stage, is it? :-)

Quoted 2 lines

this is where it would
get caught and reported,

...but then you still wouldn't be able to do anything about it during runtime, as there would be no way to handle this and fix it: the driver would still act in a broken manner.

From: bui duc phuc <[email protected]>

mt8189_mck_disable() only checks for negative MCK IDs. Reject IDs outside the valid MCK range as well.

Fixes dc637ffeed6c ("ASoC: mediatek: mt8189: support audio clock control")

Signed-off-by bui duc phuc <[email protected]>

From: bui duc phuc <[email protected]>

Use dev_err_probe() when obtaining clocks to avoid redundant error messages, particularly for probe deferral.

Signed-off-by bui duc phuc <[email protected]>

From: bui duc phuc <[email protected]>

mt8189_afe_runtime_resume() currently ignores errors from regmap operations and mt8189_afe_enable_main_clock().

Propagate these errors and clean up the state before returning the error.

Fixes 7eb153585598 ("ASoC: mediatek: mt8189: add platform driver")

Signed-off-by bui duc phuc <[email protected]>

---

Changes in v2:
- Update the names of the goto labels.

  sound/soc/mediatek/mt8189/mt8189-afe-pcm.c | 36 +++++++++++++++++-----
  1 file changed, 29 insertions(+), 7 deletions(-)

sound/soc/mediatek/mt8189/mt8189-afe-pcm.c +0 -0

index 77cf2b604f6c..67fa40afdefa 100644

sound/soc/mediatek/mt8189/mt8189-afe-pcm.c +29 -7

-		return 0;
+		ret = -EINVAL;
+		goto err_disable_reg_rw_clk;
  	}
 
  	regcache_cache_only(afe->regmap, false);
-	regcache_sync(afe->regmap);
+	ret = regcache_sync(afe->regmap);
+	if (ret)
+		goto err_set_cache_only;
 
  	/* set audio 26M request */
-	regmap_update_bits(afe->regmap, AFE_SPM_CONTROL_REQ, 0x1, 0x1);
-	regmap_update_bits(afe->regmap, AFE_CBIP_CFG0, 0x1, 0x1);
+	ret = regmap_update_bits(afe->regmap, AFE_SPM_CONTROL_REQ, 0x1, 0x1);
+	if (ret)
+		goto err_set_cache_only;
+
+	ret = regmap_update_bits(afe->regmap, AFE_CBIP_CFG0, 0x1, 0x1);
+	if (ret)
+		goto err_clear_26m_req;
 
  	/* force cpu use 8_24 format when writing 32bit data */
-	regmap_update_bits(afe->regmap, AFE_MEMIF_CON0,
-			   CPU_HD_ALIGN_MASK_SFT, 0 << CPU_HD_ALIGN_SFT);
+	ret = regmap_update_bits(afe->regmap, AFE_MEMIF_CON0,
+				 CPU_HD_ALIGN_MASK_SFT, 0 << CPU_HD_ALIGN_SFT);
+	if (ret)
+		goto err_clear_26m_req;
 
  	/* enable AFE */
-	mt8189_afe_enable_main_clock(afe);
+	ret = mt8189_afe_enable_main_clock(afe);
+	if (ret)
+		goto err_clear_26m_req;
 
  	return 0;
+
+err_clear_26m_req:
+	regmap_update_bits(afe->regmap,
+			   AFE_SPM_CONTROL_REQ, 0x1, 0x0);
+err_set_cache_only:
+	regcache_cache_only(afe->regmap, true);
+err_disable_reg_rw_clk:
+	mt8189_afe_disable_reg_rw_clk(afe);
+
+	return ret;
  }
 
  static int mt8189_afe_component_probe(struct snd_soc_component *component)

Quoted 2 lines

      if (!afe->regmap) {
              dev_warn(afe->dev, "skip regmap\n");

In the probe function, there's a call to devm_regmap_init_mmio(), and that's being correctly checked for error as in, if any, probe will fail.

So... during suspend or resume or anywhere else in this driver really, the regmap pointer can't be NULL.
The right thing to do here would be to just remove the useless check.

Mind you, this comment applies to some other commits in this series as well.

Quoted 10 lines

In the probe function, there's a call to devm_regmap_init_mmio(), and that's being correctly checked for error as in, if any, probe will fail.

So... during suspend or resume or anywhere else in this driver really, the regmap pointer can't be NULL.
The right thing to do here would be to just remove the useless check.

Mind you, this comment applies to some other commits in this series as well.

Hi Angelo,

Quoted 10 lines

Quoted 10 lines

In the probe function, there's a call to devm_regmap_init_mmio(), and that's being correctly checked for error as in, if any, probe will fail.

So... during suspend or resume or anywhere else in this driver really, the regmap pointer can't be NULL.
The right thing to do here would be to just remove the useless check.

Mind you, this comment applies to some other commits in this series as well.

I found something interesting:

in mt8189_afe_pcm_dev_probe()

-------------------------------------
afe->runtime_resume = mt8189_afe_runtime_resume; afe->runtime_suspend = mt8189_afe_runtime_suspend;

ret = devm_pm_runtime_enable(dev);
if (ret)
return ret;

/*
* Audio device is part of genpd. Registering it as a syscore device
* ensure the proper power-on sequence of the AFE device. */
dev_pm_syscore_device(dev, true);

/* enable clock for regcache get default value from hw */ ret = pm_runtime_resume_and_get(dev);
if (ret)
return dev_err_probe(dev, ret, "failed to resume device\n");

afe->regmap = devm_regmap_init_mmio(dev, afe->base_addr,

    &mt8189_afe_regmap_config);

if (IS_ERR(afe->regmap)) {
ret = PTR_ERR(afe->regmap);
goto err_pm_put;
}
-----------------------------------------------

Here, it looks like mt8189_afe_runtime_resume() gets called before devm_regmap_init_mmio().

So if we remove this part:
---------------------

if (!afe->regmap) {
         dev_warn(afe->dev, "skip regmap\n");
         return 0;
     }

-----------------------
from mt8189_afe_runtime_resume(), that could actually trigger the bug we were just discussing.

Quoted 10 lines

So if we remove this part:
---------------------

if (!afe->regmap) {
         dev_warn(afe->dev, "skip regmap\n");
         return 0;
     }

-----------------------
from mt8189_afe_runtime_resume(), that could actually trigger the bug we were just discussing.

Well, then...

Reviewed-by AngeloGioacchino Del Regno <[email protected]>

...still, please check the other cases.

From: bui duc phuc <[email protected]>

The errors handled here are already reported by the called functions, either directly or deeper in the call chain. Therefore, the additional dev_err() and dev_err_probe() call is redundant and can be removed.

Signed-off-by bui duc phuc <[email protected]>

From: bui duc phuc <[email protected]>

mtk_mclk_en_event() currently ignores errors returned by the MCK enable and disable functions.

Propagate these errors.

Fixes 7eb153585598 ("ASoC: mediatek: mt8189: add platform driver")

Signed-off-by bui duc phuc <[email protected]>

---
  sound/soc/mediatek/mt8189/mt8189-dai-i2s.c | 7 ++++---
  1 file changed, 4 insertions(+), 3 deletions(-)

sound/soc/mediatek/mt8189/mt8189-dai-i2s.c +0 -0

index 4826ee250d70..f20dbb410223 100644

sound/soc/mediatek/mt8189/mt8189-dai-i2s.c +0 -0

  	dev_dbg(cmpnt->dev, "%s(), name %s, event 0x%x\n",
  		__func__, w->name, event);

@@ -529,17 +530,17 @@ static int mtk_mclk_en_event(struct snd_soc_dapm_widget *w,
 
  	switch (event) {

From: bui duc phuc <[email protected]>

A zero frequency causes a modulo-by-zero error when validating the I2S clock frequency. Reject it before performing the validation.

Fixes 7eb153585598 ("ASoC: mediatek: mt8189: add platform driver")

Signed-off-by bui duc phuc <[email protected]>

From: bui duc phuc <[email protected]>

mtk_dai_tdm_cal_mclk() accepts zero or negative frequencies. Reject invalid frequencies before configuring the TDM clock.

Fixes 9f202872ba04 ("ASoC: mediatek: mt8189: support TDM in platform driver")

Signed-off-by bui duc phuc <[email protected]>