[PATCH v2 0/4] ASoC: mediatek: mt8183: Fix clock error handling

[PATCH v2 0/4] ASoC: mediatek: mt8183: Fix clock error handling

[phucduc.bui]

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

Hi all,

This series fixes and cleans up clock error handling in the MT8183 ASoC 
driver.

Compile tested only.

Changes in v2:
 - Remove the redundant error log for apll1_mux_setting error handling.
 - Add Fixes tags.
 - Add a cover letter.

Best regards,
Phuc

bui duc phuc (4):
  ASoC: mediatek: mt8183: Fix wrong clock cleanup on clk_set_parent()
    failure
  ASoC: mediatek: mt8183: Fix APLL enable error handling
  ASoC: mediatek: mt8183: Use dev_err_probe() for error handling
  ASoC: mediatek: mt8183: Drop redundant probe error messages

 sound/soc/mediatek/mt8183/mt8183-afe-clk.c | 25 +++++++++++++---------
 sound/soc/mediatek/mt8183/mt8183-afe-pcm.c |  8 ++-----
 2 files changed, 17 insertions(+), 16 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] ASoC: mediatek: mt8183: Fix wrong clock cleanup on clk_set_parent() failure

[phucduc.bui]

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

In mt8183_afe_enable_clock(), when clk_set_parent() fails, the current
error path incorrectly cleans up the previously enabled clock instead of
the clock used by clk_set_parent().

Fix the error path to clean up the correct clock when clk_set_parent()
fails.

Fixes: a94aec035a12 ("ASoC: mediatek: mt8183: add platform driver")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8183/mt8183-afe-clk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
index cc4f8f4d3dab..6ab0734ad136 100644
--- a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
+++ b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
@@ -137,7 +137,7 @@ int mt8183_afe_enable_clock(struct mtk_base_afe *afe)
 		dev_err(afe->dev, "%s(), clk_set_parent %s-%s fail %d\n",
 			__func__, aud_clks[CLK_MUX_AUDIO],
 			aud_clks[CLK_CLK26M], ret);
-		goto CLK_MUX_AUDIO_ERR;
+		goto CLK_MUX_AUDIO_INTBUS_ERR;
 	}
 
 	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
@@ -153,7 +153,7 @@ int mt8183_afe_enable_clock(struct mtk_base_afe *afe)
 		dev_err(afe->dev, "%s(), clk_set_parent %s-%s fail %d\n",
 			__func__, aud_clks[CLK_MUX_AUDIOINTBUS],
 			aud_clks[CLK_TOP_SYSPLL_D2_D4], ret);
-		goto CLK_MUX_AUDIO_INTBUS_ERR;
+		goto CLK_AFE_ERR;
 	}
 
 	ret = clk_prepare_enable(afe_priv->clk[CLK_AFE]);
-- 
2.43.0

[PATCH v2 2/4] ASoC: mediatek: mt8183: Fix APLL enable error handling

[phucduc.bui]

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

Currently, the mt8183_apll*_enable() functions call mux_setting(afe, true)
but do not check its return value to handle failures.

In addition, the cleanup paths of mt8183_apll*_enable() do not call
mux_setting(afe, false) when the enable operation fails, while the
mt8183_apll*_disable() functions do.

Add error handling for apll*_mux_setting() and call mux_setting(afe, false)
in the cleanup paths when mt8183_apll*_enable() fails.

Fixes: a94aec035a12 ("ASoC: mediatek: mt8183: add platform driver")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8183/mt8183-afe-clk.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
index 6ab0734ad136..0790d8123179 100644
--- a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
+++ b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
@@ -382,7 +382,9 @@ int mt8183_apll1_enable(struct mtk_base_afe *afe)
 	int ret;
 
 	/* setting for APLL */
-	apll1_mux_setting(afe, true);
+	ret = apll1_mux_setting(afe, true);
+	if (ret)
+		goto ERR_APLL1_MUX_SETTING;
 
 	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL22M]);
 	if (ret) {
@@ -411,6 +413,8 @@ int mt8183_apll1_enable(struct mtk_base_afe *afe)
 ERR_CLK_APLL1_TUNER:
 	clk_disable_unprepare(afe_priv->clk[CLK_APLL22M]);
 ERR_CLK_APLL22M:
+	apll1_mux_setting(afe, false);
+ERR_APLL1_MUX_SETTING:
 	return ret;
 }
 
@@ -436,7 +440,9 @@ int mt8183_apll2_enable(struct mtk_base_afe *afe)
 	int ret;
 
 	/* setting for APLL */
-	apll2_mux_setting(afe, true);
+	ret = apll2_mux_setting(afe, true);
+	if (ret)
+		goto ERR_APLL2_MUX_SETTING;
 
 	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL24M]);
 	if (ret) {
@@ -465,6 +471,8 @@ int mt8183_apll2_enable(struct mtk_base_afe *afe)
 ERR_CLK_APLL2_TUNER:
 	clk_disable_unprepare(afe_priv->clk[CLK_APLL24M]);
 ERR_CLK_APLL24M:
+	apll2_mux_setting(afe, false);
+ERR_APLL2_MUX_SETTING:
 	return ret;
 }
 
-- 
2.43.0

[PATCH v2 3/4] ASoC: mediatek: mt8183: Use dev_err_probe() for error handling

[phucduc.bui]

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

Replace dev_err() with dev_err_probe() to prevent log spam when probe
returns -EPROBE_DEFER.

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

diff --git a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
index 0790d8123179..46cab62220d9 100644
--- a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
+++ b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
@@ -101,12 +101,9 @@ int mt8183_init_clock(struct mtk_base_afe *afe)
 
 	for (i = 0; i < 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, "%s(), devm_clk_get %s fail, ret %ld\n",
-				__func__, aud_clks[i],
-				PTR_ERR(afe_priv->clk[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]);
 	}
 
 	return 0;
-- 
2.43.0

[PATCH v2 4/4] ASoC: mediatek: mt8183: Drop redundant probe error messages

[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() calls are redundant and can be removed.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/mediatek/mt8183/mt8183-afe-pcm.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/sound/soc/mediatek/mt8183/mt8183-afe-pcm.c b/sound/soc/mediatek/mt8183/mt8183-afe-pcm.c
index 2634699534db..46b7a2bb6aaa 100644
--- a/sound/soc/mediatek/mt8183/mt8183-afe-pcm.c
+++ b/sound/soc/mediatek/mt8183/mt8183-afe-pcm.c
@@ -809,10 +809,8 @@ static int mt8183_afe_pcm_dev_probe(struct platform_device *pdev)
 
 	/* initial audio related clock */
 	ret = mt8183_init_clock(afe);
-	if (ret) {
-		dev_err(dev, "init clock error\n");
+	if (ret)
 		return ret;
-	}
 
 	pm_runtime_enable(dev);
 
@@ -903,10 +901,8 @@ static int mt8183_afe_pcm_dev_probe(struct platform_device *pdev)
 
 	ret = devm_request_irq(dev, irq_id, mt8183_afe_irq_handler,
 			       IRQF_TRIGGER_NONE, "asys-isr", (void *)afe);
-	if (ret) {
-		dev_err(dev, "could not request_irq for asys-isr\n");
+	if (ret)
 		goto err_pm_disable;
-	}
 
 	/* init sub_dais */
 	INIT_LIST_HEAD(&afe->sub_dais);
-- 
2.43.0

Re: [PATCH v2 2/4] ASoC: mediatek: mt8183: Fix APLL enable error handling

[Cezary Rojewski]

On 8/21/2026 2:29 PM, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Currently, the mt8183_apll*_enable() functions call mux_setting(afe, true)
> but do not check its return value to handle failures.
> 
> In addition, the cleanup paths of mt8183_apll*_enable() do not call
> mux_setting(afe, false) when the enable operation fails, while the
> mt8183_apll*_disable() functions do.
> 
> Add error handling for apll*_mux_setting() and call mux_setting(afe, false)
> in the cleanup paths when mt8183_apll*_enable() fails.


I have mixed feelings about appl*_mux_setting().  Take a look at its 
disable-path: if clk_set_parent() fails the follow up 
clk_disable_unprepare() is skipped possibly leaving one of the clks 
hanging.  I'd expect error paths of callers (of said mux_setting() 
function) to ensure all the clks are disabled and unprepared 
unconditionally.

> Fixes: a94aec035a12 ("ASoC: mediatek: mt8183: add platform driver")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>   sound/soc/mediatek/mt8183/mt8183-afe-clk.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
> index 6ab0734ad136..0790d8123179 100644
> --- a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
> +++ b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
> @@ -382,7 +382,9 @@ int mt8183_apll1_enable(struct mtk_base_afe *afe)
>   	int ret;
>   
>   	/* setting for APLL */
> -	apll1_mux_setting(afe, true);
> +	ret = apll1_mux_setting(afe, true);
> +	if (ret)
> +		goto ERR_APLL1_MUX_SETTING;

Why goto? The check is valid but the label in my opinion is unnecessary.

>   
>   	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL22M]);
>   	if (ret) {
> @@ -411,6 +413,8 @@ int mt8183_apll1_enable(struct mtk_base_afe *afe)
>   ERR_CLK_APLL1_TUNER:
>   	clk_disable_unprepare(afe_priv->clk[CLK_APLL22M]);
>   ERR_CLK_APLL22M:
> +	apll1_mux_setting(afe, false);
> +ERR_APLL1_MUX_SETTING:

While UPPER case for goto-labels is not part of the coding style I see 
why you did it - to be cohesive with the rest of the file.

Re: [PATCH v2 3/4] ASoC: mediatek: mt8183: Use dev_err_probe() for error handling

[Cezary Rojewski]

On 8/21/2026 2:29 PM, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Replace dev_err() with dev_err_probe() to prevent log spam when probe
> returns -EPROBE_DEFER.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>   sound/soc/mediatek/mt8183/mt8183-afe-clk.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
> index 0790d8123179..46cab62220d9 100644
> --- a/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
> +++ b/sound/soc/mediatek/mt8183/mt8183-afe-clk.c
> @@ -101,12 +101,9 @@ int mt8183_init_clock(struct mtk_base_afe *afe)
>   
>   	for (i = 0; i < 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, "%s(), devm_clk_get %s fail, ret %ld\n",
> -				__func__, aud_clks[i],
> -				PTR_ERR(afe_priv->clk[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]);
>   	}
>   
>   	return 0;

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>

Re: [PATCH v2 4/4] ASoC: mediatek: mt8183: Drop redundant probe error messages

[Cezary Rojewski]

On 8/21/2026 2:29 PM, 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() calls are redundant and can be removed.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>

Re: [PATCH v2 1/4] ASoC: mediatek: mt8183: Fix wrong clock cleanup on clk_set_parent() failure

[Cezary Rojewski]

On 8/21/2026 2:29 PM, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> In mt8183_afe_enable_clock(), when clk_set_parent() fails, the current
> error path incorrectly cleans up the previously enabled clock instead of
> the clock used by clk_set_parent().
> 
> Fix the error path to clean up the correct clock when clk_set_parent()
> fails.
> 
> Fixes: a94aec035a12 ("ASoC: mediatek: mt8183: add platform driver")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
Very good finding!

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>

Re: [PATCH v2 2/4] ASoC: mediatek: mt8183: Fix APLL enable error handling

[Bui Duc Phuc]

Hi Cezary,

Thank you for your review.


> I have mixed feelings about appl*_mux_setting().  Take a look at its
> disable-path: if clk_set_parent() fails the follow up
> clk_disable_unprepare() is skipped possibly leaving one of the clks
> hanging.  I'd expect error paths of callers (of said mux_setting()
> function) to ensure all the clks are disabled and unprepared
> unconditionally.
>

Perhaps the primary purpose of the appl*_mux_setting() functions is to
configure the mux, as their names suggest, which may explain why the disable
path is currently structured this way. However, I think your point is valid.

As far as I understand, calling clk_disable_unprepare() before
clk_set_parent() should not cause any issues. Therefore, we could move both
clk_disable_unprepare() calls before changing the parent in the disable path:

------------------------------------
clk_disable_unprepare(afe_priv->clk[CLK_TOP_MUX_AUD_ENG2]);
clk_disable_unprepare(afe_priv->clk[CLK_TOP_MUX_AUD_2]);
ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_ENG2],
                                 afe_priv->clk[CLK_CLK26M]);
if (ret) {
...
goto EXIT;
}
ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_2],
                                afe_priv->clk[CLK_CLK26M]);
if (ret) {
...
goto EXIT;
}
-------------------------------------

To be honest, after looking at the code for other MediaTek SoCs, I found quite
a few logic issues and inconsistent clock handling. This, along with
the git log history,
makes me wonder whether these code paths were ever properly tested on
real hardware.
Therefore, I have stopped at mt8186 for now.


> >       /* setting for APLL */
> > -     apll1_mux_setting(afe, true);
> > +     ret = apll1_mux_setting(afe, true);
> > +     if (ret)
> > +             goto ERR_APLL1_MUX_SETTING;
>
> Why goto? The check is valid but the label in my opinion is unnecessary.
>

The function could return directly here, but I used goto to keep it
consistent with
the existing error handling in this function.

> > +     apll1_mux_setting(afe, false);
> > +ERR_APLL1_MUX_SETTING:
>
> While UPPER case for goto-labels is not part of the coding style I see
> why you did it - to be cohesive with the rest of the file.

Yes, that's right. However, I noticed that the mt8186 code uses
lowercase names for goto labels,
so the style is not consistent across the file.

https://elixir.bootlin.com/linux/v7.2/source/sound/soc/mediatek/mt8186/mt8186-afe-clk.c#L303

Best regards,
Phuc

Re: [PATCH v2 2/4] ASoC: mediatek: mt8183: Fix APLL enable error handling

[Mark Brown]

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

On Thu, Aug 27, 2026 at 02:58:04PM +0700, Bui Duc Phuc wrote:

> To be honest, after looking at the code for other MediaTek SoCs, I found quite
> a few logic issues and inconsistent clock handling. This, along with
> the git log history,
> makes me wonder whether these code paths were ever properly tested on
> real hardware.
> Therefore, I have stopped at mt8186 for now.

There's a lot of code there with shaky error handling; probably the
default case works but things blow up relatively easily on error.

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

Re: [PATCH v2 2/4] ASoC: mediatek: mt8183: Fix APLL enable error handling

[Bui Duc Phuc]

Hi Mark, Cezary.

Thank you for your feedback.

>
> > To be honest, after looking at the code for other MediaTek SoCs, I found quite
> > a few logic issues and inconsistent clock handling. This, along with
> > the git log history,
> > makes me wonder whether these code paths were ever properly tested on
> > real hardware.
> > Therefore, I have stopped at mt8186 for now.
>
> There's a lot of code there with shaky error handling; probably the
> default case works but things blow up relatively easily on error.

If you think these code paths are generally functional on real hardware and
the main issue is the shaky error handling,
I’m happy to continue reviewing the remaining SoCs.

I’ll also apply Cezary’s review comments regarding the clock handling in
the mux disable path and returning directly from apllx_mux_setting()
instead of using a goto label, and incorporate these changes into the
next patches.

There are also quite a few uppercase goto labels.
I think this cleanup can be addressed separately later.

Best regards,
Phuc