Re: [PATCH] ASoC: rockchip: rockchip_sai: Hand over hclk control exclusively to Runtime PM

[Mark Brown]

On Mon, Jun 22, 2026 at 07:56:13AM +0700, phucduc.bui@xxxxxxxxx wrote:

> Although switching to devm_clk_get_enabled() in a previous patch was
> tested successfully, it introduces overlapping ownership of the clock
> lifecycle. Since the driver requires early register access to read the
> device version during probe(), enabling hclk at that point is mandatory.

> Clean up the design by:

>    1 Reverting back to devm_clk_get() to remove the implicit devres
>      enable/disable behavior.
>    2 Manually enabling and disabling hclk explicitly only around the
>      early register access before Runtime PM takes over.
>    3 Dropping the stray clk_disable_unprepare() at the end of probe()
>      so Runtime PM solely owns hclk afterward.

Note that runtime PM can be disabled at build time so we might not have
runtime PM at all...

> Links: 
> 1 This change is based on the discussion around manual hclk handing during probe(),
>   as raised by Krysztof:
>   https://lore.kernel.org/all/20e4754b-ea9a-404d-b529-ec44a7263cbf@xxxxxxxxxx/#t
> 2 Background for the earlier devm_clk_get_enbabled() conversion:
>   https://lore.kernel.org/all/2818018.CQOukoFCf9@workhorse/

> An alternative approach would be use devm_regmap_init_mmio_clk() and let regmap
> manage clock enablement around register accesses. If preferred, I can rework the 
> driver accordingly.

> -	sai->hclk = devm_clk_get_enabled(&pdev->dev, "hclk");
> +	sai->hclk = devm_clk_get(&pdev->dev, "hclk");
>  	if (IS_ERR(sai->hclk))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(sai->hclk),
>  				     "Failed to get hclk\n");
>  
> +	ret = clk_prepare_enable(sai->hclk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to enable hclk\n");
> +

> @@ -1482,8 +1492,6 @@ static int rockchip_sai_probe(struct platform_device *pdev)
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_put(&pdev->dev);
>  
> -	clk_disable_unprepare(sai->hclk);
> -
>  	return 0;

Are you sure that the runtime PM state there is such that it knows a
reference is held?  The driver used pm_runtime_get_noresume() so the
device didn't have RPM_ACTIVE set I think?

The runtime PM API really is a miserable collection of landmines :(

Attachment: signature.asc
Description: PGP signature