[PATCH v2 00/13] ASoC: mediatek: mt8189: Improve error handling

[PATCH v2 00/13] ASoC: mediatek: mt8189: Improve error handling

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Hi all,

This series addresses part of the error handling in the MT8189 AFE driver.

It propagates errors from clock operations and runtime resume, validates
clock parameters, and removes redundant error messages.

Compile-tested only.

Link v1 : 
https://lore.kernel.org/all/20260910125742.150902-1-phucduc.bui@gmail.com/

Changes in v2:
 - Update the names of the goto labels.
 - Check for zero frequency since freq is unsigned. 

Best regards,
Phuc

bui duc phuc (13):
  ASoC: mediatek: mt8189: Return error for missing regmap
  ASoC: mediatek: mt8189: Propagate APLL enable errors
  ASoC: mediatek: mt8189: Propagate MCK enable errors
  ASoC: mediatek: mt8189: Validate MCK ID
  ASoC: mediatek: mt8189: Propagate reg_rw clock errors
  ASoC: mediatek: mt8189: Use dev_err_probe() for clock errors
  ASoC: mediatek: mt8189: Propagate runtime resume errors
  ASoC: mediatek: mt8189: Remove redundant error message
  ASoC: mediatek: mt8189: Propagate APLL errors
  ASoC: mediatek: mt8189: Propagate MCLK errors
  ASoC: mediatek: mt8189: Validate sysclk frequency
  ASoC: mediatek: mt8189: Propagate TDM clock errors
  ASoC: mediatek: mt8189: Validate TDM MCLK frequency

 sound/soc/mediatek/mt8189/mt8189-afe-clk.c | 136 +++++++++++++++------
 sound/soc/mediatek/mt8189/mt8189-afe-pcm.c |  49 +++++---
 sound/soc/mediatek/mt8189/mt8189-dai-i2s.c |  17 ++-
 sound/soc/mediatek/mt8189/mt8189-dai-tdm.c |  17 ++-
 4 files changed, 155 insertions(+), 64 deletions(-)

-- 
2.43.0

[PATCH v2 01/13] ASoC: mediatek: mt8189: Return error for missing regmap

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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 <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8189/mt8189-afe-clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
index fc7a7a73b0cf..63e03a40dbbe 100644
--- a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
+++ b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
@@ -236,7 +236,7 @@ static int mt8189_afe_enable_top_cg(struct mtk_base_afe *afe, unsigned int cg_ty
 
 	if (!afe->regmap) {
 		dev_err(afe->dev, "afe regmap is null !!!\n");
-		return 0;
+		return -EINVAL;
 	}
 
 	dev_dbg(afe->dev, "reg: 0x%x, mask: 0x%x, val: 0x%x\n", reg, mask, val);
-- 
2.43.0

Re: [PATCH v2 01/13] ASoC: mediatek: mt8189: Return error for missing regmap

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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 <phucduc.bui@gmail.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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 <phucduc.bui@gmail.com>
---

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(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
index 63e03a40dbbe..aaf4f7921363 100644
--- a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
+++ b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
@@ -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)
-		return ret;
+		goto err_disable_apll2_ck;
 
 	/* sel 48kHz: 2, apll_div: 7, upper bound: 3*/
-	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
-			   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));
+	ret = regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
+				 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;
 
 	/* apll2 freq tuner enable */
-	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
-			   FREQ_TUNER_EN_MASK_SFT,
-			   0x1 << FREQ_TUNER_EN_SFT);
+	ret = regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
+				 FREQ_TUNER_EN_MASK_SFT,
+				 0x1 << FREQ_TUNER_EN_SFT);
+	if (ret)
+		goto err_disable_apll_tuner2;
 
 	/* audio apll2 on */
 	ret = mt8189_afe_enable_top_cg(afe, MT8189_AUDIO_APLL2_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_APLL2_TUNER_CFG,
+			   FREQ_TUNER_EN_MASK_SFT,
+			   0x0);
+err_disable_apll_tuner2:
+	mt8189_afe_disable_top_cg(afe, MT8189_PDN_APLL_TUNER2);
+err_disable_apll2_ck:
+	mt8189_afe_disable_top_cg(afe, MT8189_CG_APLL2_CK);
+err_clear_mux_setting:
+	apll2_mux_setting(afe, false);
+
+	return ret;
 }
 
 void mt8189_apll2_disable(struct mtk_base_afe *afe)
-- 
2.43.0

Re: [PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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 <phucduc.bui@gmail.com>
> ---
> 
> 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(-)
> 
> diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
> index 63e03a40dbbe..aaf4f7921363 100644
> --- a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
> +++ b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
> @@ -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)
> -		return ret;
> +		goto err_disable_apll2_ck;
>   
>   	/* sel 48kHz: 2, apll_div: 7, upper bound: 3*/
> -	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
> -			   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));
> +	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.

> +				 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;
>   
>   	/* apll2 freq tuner enable */
> -	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
> -			   FREQ_TUNER_EN_MASK_SFT,
> -			   0x1 << FREQ_TUNER_EN_SFT);
> +	ret = regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
> +				 FREQ_TUNER_EN_MASK_SFT,
> +				 0x1 << FREQ_TUNER_EN_SFT);
> +	if (ret)
> +		goto err_disable_apll_tuner2;

Same here.
...and it's the same in some other commits of this series.

Though, good job about fixing the failure paths, that's good stuff.

Cheers,
Angelo

Re: [PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Mon, Sep 14, 2026 at 03:22:21PM +0200, AngeloGioacchino Del Regno wrote:
> On 9/14/26 09:28, phucduc.bui@gmail.com wrote:

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[AngeloGioacchino Del Regno]

On 9/14/26 15:47, Mark Brown wrote:
> On Mon, Sep 14, 2026 at 03:22:21PM +0200, AngeloGioacchino Del Regno wrote:
>> On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> 
>>> +	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 :) .

lmao

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

Cheers!
Angelo

Re: [PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On Mon, Sep 14, 2026 at 03:58:28PM +0200, AngeloGioacchino Del Regno wrote:
> On 9/14/26 15:47, Mark Brown wrote:

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

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[Bui Duc Phuc]

Hi Mark , Angelo

Thanks both for the review.

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

Re: [PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[AngeloGioacchino Del Regno]

On 9/15/26 08:55, Bui Duc Phuc wrote:
> Hi Mark , Angelo
> 
> Thanks both for the review.
> 
>>
>>>>   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.
>>
>> 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.

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

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

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

> 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? :-)

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

Cheers,
Angelo

> rather than the driver silently assuming the enable succeeded when it didn't.
> 
> Best regards,
> Phuc

Re: [PATCH v2 02/13] ASoC: mediatek: mt8189: Propagate APLL enable errors

[Bui Duc 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().
>

I don't think runtime resume always succeeds.
However, with the code as it is now, it can indeed return success as
you described.

> > 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...
>
> > 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.
>
> > 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? :-)
>

Oh, being upstream means it's already past the development stage now? :-)

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

I agree it may not be recoverable at runtime, but returning the error prevents
the driver from continuing as if it succeeded and makes the failure
visible to the caller.

Best regards,
Phuc

[PATCH v2 03/13] ASoC: mediatek: mt8189: Propagate MCK enable errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

mt8189_mck_enable() currently returns without restoring the clock
state when setting the clock parent, enabling the divider, or setting
the divider rate fails.

Propagate the error and disable clocks enabled by the function.

Fixes: dc637ffeed6c ("ASoC: mediatek: mt8189: support audio clock control")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

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

 sound/soc/mediatek/mt8189/mt8189-afe-clk.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
index aaf4f7921363..8f6241d47a65 100644
--- a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
+++ b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
@@ -639,7 +639,7 @@ int mt8189_mck_enable(struct mtk_base_afe *afe, int mck_id, int rate)
 		ret = mt8189_afe_set_clk_parent(afe, afe_priv->clk[m_sel_id],
 						afe_priv->clk[apll_clk_id]);
 		if (ret)
-			return ret;
+			goto err_disable_m_sel_clk;
 	}
 
 	/* enable div, set rate */
@@ -650,13 +650,21 @@ int mt8189_mck_enable(struct mtk_base_afe *afe, int mck_id, int rate)
 
 	ret = mt8189_afe_enable_clk(afe, afe_priv->clk[div_clk_id]);
 	if (ret)
-		return ret;
+		goto err_disable_m_sel_clk;
 
 	ret = mt8189_afe_set_clk_rate(afe, afe_priv->clk[div_clk_id], rate);
 	if (ret)
-		return ret;
+		goto err_disable_div_clk;
 
 	return 0;
+
+err_disable_div_clk:
+	mt8189_afe_disable_clk(afe, afe_priv->clk[div_clk_id]);
+err_disable_m_sel_clk:
+	if (m_sel_id >= 0)
+		mt8189_afe_disable_clk(afe, afe_priv->clk[m_sel_id]);
+
+	return ret;
 }
 
 int mt8189_mck_disable(struct mtk_base_afe *afe, int mck_id)
-- 
2.43.0

[PATCH v2 04/13] ASoC: mediatek: mt8189: Validate MCK ID

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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 <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8189/mt8189-afe-clk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
index 8f6241d47a65..69aa3715c19e 100644
--- a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
+++ b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
@@ -675,8 +675,8 @@ int mt8189_mck_disable(struct mtk_base_afe *afe, int mck_id)
 
 	dev_dbg(afe->dev, "mck_id: %d.\n", mck_id);
 
-	if (mck_id < 0) {
-		dev_err(afe->dev, "mck_id = %d < 0\n", mck_id);
+	if (mck_id >= MT8189_MCK_NUM || mck_id < 0) {
+		dev_err(afe->dev, "mck_id = %d\n", mck_id);
 		return -EINVAL;
 	}
 
-- 
2.43.0

Re: [PATCH v2 04/13] ASoC: mediatek: mt8189: Validate MCK ID

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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 <phucduc.bui@gmail.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH v2 05/13] ASoC: mediatek: mt8189: Propagate reg_rw clock errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

mt8189_afe_enable_reg_rw_clk() currently ignores errors from clock
enable and parent operations.

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

Fixes: dc637ffeed6c ("ASoC: mediatek: mt8189: support audio clock control")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

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

 sound/soc/mediatek/mt8189/mt8189-afe-clk.c | 31 +++++++++++++++++-----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
index 69aa3715c19e..173ee0709b16 100644
--- a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
+++ b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
@@ -700,17 +700,36 @@ int mt8189_mck_disable(struct mtk_base_afe *afe, int mck_id)
 int mt8189_afe_enable_reg_rw_clk(struct mtk_base_afe *afe)
 {
 	struct mt8189_afe_private *afe_priv = afe->platform_priv;
+	int ret;
 
 	/* bus clock for AFE internal access, like AFE SRAM */
-	mt8189_afe_enable_clk(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIOINTBUS]);
-	mt8189_afe_set_clk_parent(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIOINTBUS],
-				  afe_priv->clk[MT8189_CLK_TOP_CLK26M]);
+	ret = mt8189_afe_enable_clk(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIOINTBUS]);
+	if (ret)
+		return ret;
+
+	ret = mt8189_afe_set_clk_parent(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIOINTBUS],
+					afe_priv->clk[MT8189_CLK_TOP_CLK26M]);
+	if (ret)
+		goto err_disable_audiointbus_clk;
+
 	/* enable audio clock source */
-	mt8189_afe_enable_clk(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIO_H]);
-	mt8189_afe_set_clk_parent(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIO_H],
-				  afe_priv->clk[MT8189_CLK_TOP_CLK26M]);
+	ret = mt8189_afe_enable_clk(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIO_H]);
+	if (ret)
+		goto err_disable_audiointbus_clk;
+
+	ret = mt8189_afe_set_clk_parent(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIO_H],
+					afe_priv->clk[MT8189_CLK_TOP_CLK26M]);
+	if (ret)
+		goto err_disable_audio_h_clk;
 
 	return 0;
+
+err_disable_audio_h_clk:
+	mt8189_afe_disable_clk(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIO_H]);
+err_disable_audiointbus_clk:
+	mt8189_afe_disable_clk(afe, afe_priv->clk[MT8189_CLK_TOP_MUX_AUDIOINTBUS]);
+
+	return ret;
 }
 
 int mt8189_afe_disable_reg_rw_clk(struct mtk_base_afe *afe)
-- 
2.43.0

[PATCH v2 06/13] ASoC: mediatek: mt8189: Use dev_err_probe() for clock errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8189/mt8189-afe-clk.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
index 173ee0709b16..3e7465c5b277 100644
--- a/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
+++ b/sound/soc/mediatek/mt8189/mt8189-afe-clk.c
@@ -793,10 +793,9 @@ int mt8189_init_clock(struct mtk_base_afe *afe)
 
 	for (i = 0; i < MT8189_CLK_NUM; i++) {
 		afe_priv->clk[i] = devm_clk_get(afe->dev, aud_clks[i]);
-		if (IS_ERR(afe_priv->clk[i])) {
-			dev_err(afe->dev, "devm_clk_get %s fail\n", aud_clks[i]);
-			return PTR_ERR(afe_priv->clk[i]);
-		}
+		if (IS_ERR(afe_priv->clk[i]))
+			return dev_err_probe(afe->dev, PTR_ERR(afe_priv->clk[i]),
+					     "failed to get clock %s\n", aud_clks[i]);
 	}
 
 	ret = mt8189_afe_disable_apll(afe);
-- 
2.43.0

Re: [PATCH v2 06/13] ASoC: mediatek: mt8189: Use dev_err_probe() for clock errors

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Use dev_err_probe() when obtaining clocks to avoid redundant error
> messages, particularly for probe deferral.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH v2 07/13] ASoC: mediatek: mt8189: Propagate runtime resume errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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 <phucduc.bui@gmail.com>
---

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(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c b/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
index 77cf2b604f6c..67fa40afdefa 100644
--- a/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
+++ b/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
@@ -2328,24 +2328,46 @@ static int mt8189_afe_runtime_resume(struct device *dev)
 
 	if (!afe->regmap) {
 		dev_warn(afe->dev, "skip regmap\n");
-		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)
-- 
2.43.0

Re: [PATCH v2 07/13] ASoC: mediatek: mt8189: Propagate runtime resume errors

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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 <phucduc.bui@gmail.com>
> ---
> 
> 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(-)
> 
> diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c b/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
> index 77cf2b604f6c..67fa40afdefa 100644
> --- a/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
> +++ b/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
> @@ -2328,24 +2328,46 @@ static int mt8189_afe_runtime_resume(struct device *dev)
>   
>   	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.

Cheers,
Angelo

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


-- 
AngeloGioacchino Del Regno
Senior Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Re: [PATCH v2 07/13] ASoC: mediatek: mt8189: Propagate runtime resume errors

[Bui Duc Phuc]

Hi Angelo,

Thank you for your reviews.


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

You are right. devm_regmap_init_mmio() returns an ERR_PTR() on error
or a valid pointer to a struct regmap on success,
so checking for NULL here is not correct.

Also, if devm_regmap_init_mmio() fails, the probe will already fail.
Therefore, I will remove the if (!afe->regmap) check here.


Best regards,
Phuc

Re: [PATCH v2 07/13] ASoC: mediatek: mt8189: Propagate runtime resume errors

[Bui Duc Phuc]

Hi Angelo,


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

Best regards,
Phuc

Re: [PATCH v2 07/13] ASoC: mediatek: mt8189: Propagate runtime resume errors

[AngeloGioacchino Del Regno]

On 9/15/26 10:23, Bui Duc Phuc wrote:
> Hi Angelo,
> 
> 
>>>
>>> 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.
> 
Well, then...

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

...still, please check the other cases.

Re: [PATCH v2 07/13] ASoC: mediatek: mt8189: Propagate runtime resume errors

[Bui Duc Phuc]

> > 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 <angelogioacchino.delregno@collabora.com>
>
> ...still, please check the other cases.


Thank you. I'll keep this part of the code as-is:

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

Best regards,
Phuc

[PATCH v2 08/13] ASoC: mediatek: mt8189: Remove redundant error message

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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 <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8189/mt8189-afe-pcm.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c b/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
index 67fa40afdefa..981e660981e3 100644
--- a/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
+++ b/sound/soc/mediatek/mt8189/mt8189-afe-pcm.c
@@ -2488,13 +2488,12 @@ static int mt8189_afe_pcm_dev_probe(struct platform_device *pdev)
 
 	afe->base_addr = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(afe->base_addr))
-		return dev_err_probe(dev, PTR_ERR(afe->base_addr),
-				     "AFE base_addr not found\n");
+		return PTR_ERR(afe->base_addr);
 
 	/* init audio related clock */
 	ret = mt8189_init_clock(afe);
 	if (ret)
-		return dev_err_probe(dev, ret, "init clock error.\n");
+		return ret;
 
 	/* init memif */
 	/* IPM2.0 no need banding */
@@ -2528,13 +2527,13 @@ static int mt8189_afe_pcm_dev_probe(struct platform_device *pdev)
 	/* request irq */
 	irq_id = platform_get_irq(pdev, 0);
 	if (irq_id < 0)
-		return dev_err_probe(dev, irq_id, "no irq found");
+		return irq_id;
 
 	ret = devm_request_irq(dev, irq_id, mt8189_afe_irq_handler,
 			       IRQF_TRIGGER_NONE,
 			       "Afe_ISR_Handle", afe);
 	if (ret)
-		return dev_err_probe(dev, ret, "could not request_irq for Afe_ISR_Handle\n");
+		return ret;
 
 	/* init sub_dais */
 	INIT_LIST_HEAD(&afe->sub_dais);
@@ -2603,10 +2602,8 @@ static int mt8189_afe_pcm_dev_probe(struct platform_device *pdev)
 					      &mt8189_afe_component,
 					      afe->dai_drivers,
 					      afe->num_dai_drivers);
-	if (ret) {
-		dev_err(dev, "afe component err: %d\n", ret);
+	if (ret)
 		return ret;
-	}
 
 	return 0;
 
-- 
2.43.0

Re: [PATCH v2 08/13] ASoC: mediatek: mt8189: Remove redundant error message

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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 <phucduc.bui@gmail.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH v2 09/13] ASoC: mediatek: mt8189: Propagate APLL errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

mtk_apll_event() currently ignores errors returned by the APLL
enable functions.

Propagate these errors.

Fixes: 7eb153585598 ("ASoC: mediatek: mt8189: add platform driver")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8189/mt8189-dai-i2s.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c b/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
index 94c49a662e2d..4826ee250d70 100644
--- a/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
+++ b/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
@@ -485,6 +485,7 @@ static int mtk_apll_event(struct snd_soc_dapm_widget *w,
 {
 	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
 	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
+	int ret;
 
 	dev_dbg(cmpnt->dev, "%s(), name %s, event 0x%x\n",
 		__func__, w->name, event);
@@ -492,9 +493,11 @@ static int mtk_apll_event(struct snd_soc_dapm_widget *w,
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
 		if (strcmp(w->name, APLL1_W_NAME) == 0)
-			mt8189_apll1_enable(afe);
+			ret = mt8189_apll1_enable(afe);
 		else
-			mt8189_apll2_enable(afe);
+			ret = mt8189_apll2_enable(afe);
+		if (ret)
+			return ret;
 		break;
 	case SND_SOC_DAPM_POST_PMD:
 		if (strcmp(w->name, APLL1_W_NAME) == 0)
-- 
2.43.0

[PATCH v2 10/13] ASoC: mediatek: mt8189: Propagate MCLK errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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 <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8189/mt8189-dai-i2s.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c b/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
index 4826ee250d70..f20dbb410223 100644
--- a/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
+++ b/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
@@ -519,6 +519,7 @@ static int mtk_mclk_en_event(struct snd_soc_dapm_widget *w,
 	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
 	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
 	struct mtk_afe_i2s_priv *i2s_priv;
+	int ret = 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) {
 	case SND_SOC_DAPM_PRE_PMU:
-		mt8189_mck_enable(afe, i2s_priv->mclk_id, i2s_priv->mclk_rate);
+		ret = mt8189_mck_enable(afe, i2s_priv->mclk_id, i2s_priv->mclk_rate);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
 		i2s_priv->mclk_rate = 0;
-		mt8189_mck_disable(afe, i2s_priv->mclk_id);
+		ret = mt8189_mck_disable(afe, i2s_priv->mclk_id);
 		break;
 	default:
 		break;
 	}
 
-	return 0;
+	return ret;
 }
 
 static const struct snd_soc_dapm_widget mtk_dai_i2s_widgets[] = {
-- 
2.43.0

Re: [PATCH v2 10/13] ASoC: mediatek: mt8189: Propagate MCLK errors

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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 <phucduc.bui@gmail.com>
> ---
>   sound/soc/mediatek/mt8189/mt8189-dai-i2s.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c b/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
> index 4826ee250d70..f20dbb410223 100644
> --- a/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
> +++ b/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
> @@ -519,6 +519,7 @@ static int mtk_mclk_en_event(struct snd_soc_dapm_widget *w,
>   	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
>   	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
>   	struct mtk_afe_i2s_priv *i2s_priv;
> +	int ret = 0;

int ret;

>   
>   	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) {
>   	case SND_SOC_DAPM_PRE_PMU:
> -		mt8189_mck_enable(afe, i2s_priv->mclk_id, i2s_priv->mclk_rate);
> +		ret = mt8189_mck_enable(afe, i2s_priv->mclk_id, i2s_priv->mclk_rate);
>   		break;
>   	case SND_SOC_DAPM_POST_PMD:
>   		i2s_priv->mclk_rate = 0;
> -		mt8189_mck_disable(afe, i2s_priv->mclk_id);
> +		ret = mt8189_mck_disable(afe, i2s_priv->mclk_id);
>   		break;
>   	default:

ret = 0;

>   		break;
>   	}
>   
> -	return 0;
> +	return ret;
>   }
>   
>   static const struct snd_soc_dapm_widget mtk_dai_i2s_widgets[] = {

...after which:
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers,
Angelo

[PATCH v2 11/13] ASoC: mediatek: mt8189: Validate sysclk frequency

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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 <phucduc.bui@gmail.com>
---

Changes in v2:
 - Check for zero frequency since freq is unsigned. 

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

diff --git a/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c b/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
index f20dbb410223..2202fed63fc9 100644
--- a/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
+++ b/sound/soc/mediatek/mt8189/mt8189-dai-i2s.c
@@ -1286,6 +1286,9 @@ static int mtk_dai_i2s_set_sysclk(struct snd_soc_dai *dai,
 	    dir != SND_SOC_CLOCK_OUT)
 		return -EINVAL;
 
+	if (!freq)
+		return -EINVAL;
+
 	i2s_priv = afe_priv->dai_priv[dai->id];
 	if (!i2s_priv)
 		return -EINVAL;
-- 
2.43.0

Re: [PATCH v2 11/13] ASoC: mediatek: mt8189: Validate sysclk frequency

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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 <phucduc.bui@gmail.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH v2 12/13] ASoC: mediatek: mt8189: Propagate TDM clock errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

mtk_tdm_bck_en_event() and mtk_tdm_mck_en_event() currently ignore
errors returned by the MCK enable and disable functions.

Propagate these errors.

Fixes: 9f202872ba04 ("ASoC: mediatek: mt8189: support TDM in platform driver")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8189/mt8189-dai-tdm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sound/soc/mediatek/mt8189/mt8189-dai-tdm.c b/sound/soc/mediatek/mt8189/mt8189-dai-tdm.c
index 5d68a55ccc45..1b51027c6cf4 100644
--- a/sound/soc/mediatek/mt8189/mt8189-dai-tdm.c
+++ b/sound/soc/mediatek/mt8189/mt8189-dai-tdm.c
@@ -261,22 +261,23 @@ static int mtk_tdm_bck_en_event(struct snd_soc_dapm_widget *w,
 	struct mt8189_afe_private *afe_priv = afe->platform_priv;
 	int dai_id = get_tdm_id_by_name(w->name);
 	struct mtk_afe_tdm_priv *tdm_priv = afe_priv->dai_priv[dai_id];
+	int ret = 0;
 
 	dev_dbg(cmpnt->dev, "name %s, event 0x%x, dai_id %d, bck: %d\n",
 		w->name, event, dai_id, tdm_priv->bck_rate);
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		mt8189_mck_enable(afe, tdm_priv->bck_id, tdm_priv->bck_rate);
+		ret = mt8189_mck_enable(afe, tdm_priv->bck_id, tdm_priv->bck_rate);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
-		mt8189_mck_disable(afe, tdm_priv->bck_id);
+		ret = mt8189_mck_disable(afe, tdm_priv->bck_id);
 		break;
 	default:
 		break;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int mtk_tdm_mck_en_event(struct snd_soc_dapm_widget *w,
@@ -288,23 +289,24 @@ static int mtk_tdm_mck_en_event(struct snd_soc_dapm_widget *w,
 	struct mt8189_afe_private *afe_priv = afe->platform_priv;
 	int dai_id = get_tdm_id_by_name(w->name);
 	struct mtk_afe_tdm_priv *tdm_priv = afe_priv->dai_priv[dai_id];
+	int ret = 0;
 
 	dev_dbg(cmpnt->dev, "name %s, event 0x%x, dai_id %d, mclk %d\n",
 		w->name, event, dai_id, tdm_priv->mclk_rate);
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		mt8189_mck_enable(afe, tdm_priv->mclk_id, tdm_priv->mclk_rate);
+		ret = mt8189_mck_enable(afe, tdm_priv->mclk_id, tdm_priv->mclk_rate);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
 		tdm_priv->mclk_rate = 0;
-		mt8189_mck_disable(afe, tdm_priv->mclk_id);
+		ret = mt8189_mck_disable(afe, tdm_priv->mclk_id);
 		break;
 	default:
 		break;
 	}
 
-	return 0;
+	return ret;
 }
 
 static const struct snd_soc_dapm_widget mtk_dai_tdm_widgets[] = {
-- 
2.43.0

[PATCH v2 13/13] ASoC: mediatek: mt8189: Validate TDM MCLK frequency

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

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 <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8189/mt8189-dai-tdm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/mediatek/mt8189/mt8189-dai-tdm.c b/sound/soc/mediatek/mt8189/mt8189-dai-tdm.c
index 1b51027c6cf4..7748b4ff2c72 100644
--- a/sound/soc/mediatek/mt8189/mt8189-dai-tdm.c
+++ b/sound/soc/mediatek/mt8189/mt8189-dai-tdm.c
@@ -381,6 +381,9 @@ static int mtk_dai_tdm_cal_mclk(struct mtk_base_afe *afe,
 	int apll;
 	int apll_rate;
 
+	if (freq <= 0)
+		return -EINVAL;
+
 	apll = mt8189_get_apll_by_rate(afe, freq);
 	apll_rate = mt8189_get_apll_rate(afe, apll);
 
-- 
2.43.0

Re: [PATCH v2 13/13] ASoC: mediatek: mt8189: Validate TDM MCLK frequency

[AngeloGioacchino Del Regno]

On 9/14/26 09:28, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> 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 <phucduc.bui@gmail.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>