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

[AngeloGioacchino Del Regno]

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