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

[AngeloGioacchino Del Regno]

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