Re: [PATCH] ASoC: rockchip: i2s: Use managed hclk and runtime PM cleanup

[Sebastian Reichel <sebastian.reichel@collabora.com>]

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

Hi,

On Thu, May 21, 2026 at 11:30:07PM -0300, Cássio Gabriel wrote:
> The Rockchip I2S driver mixes devm-managed probe resources with manual
> runtime PM and hclk cleanup.  This leaves the remove path doing runtime PM
> shutdown and clock disable before devm-managed ASoC and PCM resources are
> released.
> 
> Keep the bus clock enabled for the device lifetime with
> devm_clk_get_enabled(), and move the runtime PM teardown into devres so the
> unwind order matches the managed registrations.  This also removes the
> remove callback, which only existed for cleanup.
> 
> Use a devm action for the final runtime suspend and register it before the
> managed runtime PM action, so teardown disables runtime PM before forcing
> the device into the suspended state.
> 
> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

Greetings,

-- Sebastian

>  sound/soc/rockchip/rockchip_i2s.c | 68 +++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 42 deletions(-)
> 
> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> index 49ff86b35ef1..4e3af0f37941 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c
> @@ -664,6 +664,14 @@ static const struct of_device_id rockchip_i2s_match[] __maybe_unused = {
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_i2s_match);
>  
> +static void rockchip_i2s_suspend(void *data)
> +{
> +	struct device *dev = data;
> +
> +	if (!pm_runtime_status_suspended(dev))
> +		i2s_runtime_suspend(dev);
> +}
> +
>  static int rockchip_i2s_init_dai(struct rk_i2s_dev *i2s, struct resource *res,
>  				 struct snd_soc_dai_driver **dp)
>  {
> @@ -758,37 +766,28 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
>  	}
>  
>  	/* try to prepare related clocks */
> -	i2s->hclk = devm_clk_get(&pdev->dev, "i2s_hclk");
> +	i2s->hclk = devm_clk_get_enabled(&pdev->dev, "i2s_hclk");
>  	if (IS_ERR(i2s->hclk)) {
>  		dev_err(&pdev->dev, "Can't retrieve i2s bus clock\n");
>  		return PTR_ERR(i2s->hclk);
>  	}
> -	ret = clk_prepare_enable(i2s->hclk);
> -	if (ret) {
> -		dev_err(i2s->dev, "hclock enable failed %d\n", ret);
> -		return ret;
> -	}
>  
>  	i2s->mclk = devm_clk_get(&pdev->dev, "i2s_clk");
>  	if (IS_ERR(i2s->mclk)) {
>  		dev_err(&pdev->dev, "Can't retrieve i2s master clock\n");
> -		ret = PTR_ERR(i2s->mclk);
> -		goto err_clk;
> +		return PTR_ERR(i2s->mclk);
>  	}
>  
>  	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> -	if (IS_ERR(regs)) {
> -		ret = PTR_ERR(regs);
> -		goto err_clk;
> -	}
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
>  
>  	i2s->regmap = devm_regmap_init_mmio(&pdev->dev, regs,
>  					    &rockchip_i2s_regmap_config);
>  	if (IS_ERR(i2s->regmap)) {
>  		dev_err(&pdev->dev,
>  			"Failed to initialise managed register map\n");
> -		ret = PTR_ERR(i2s->regmap);
> -		goto err_clk;
> +		return PTR_ERR(i2s->regmap);
>  	}
>  
>  	i2s->bclk_ratio = 64;
> @@ -799,8 +798,7 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
>  			i2s->bclk_off = pinctrl_lookup_state(i2s->pinctrl, "bclk_off");
>  			if (IS_ERR_OR_NULL(i2s->bclk_off)) {
>  				dev_err(&pdev->dev, "failed to find i2s bclk_off\n");
> -				ret = -EINVAL;
> -				goto err_clk;
> +				return -EINVAL;
>  			}
>  		}
>  	} else {
> @@ -811,16 +809,23 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
>  
>  	dev_set_drvdata(&pdev->dev, i2s);
>  
> -	pm_runtime_enable(&pdev->dev);
> +	ret = devm_add_action(&pdev->dev, rockchip_i2s_suspend, &pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
>  	if (!pm_runtime_enabled(&pdev->dev)) {
>  		ret = i2s_runtime_resume(&pdev->dev);
>  		if (ret)
> -			goto err_pm_disable;
> +			return ret;
>  	}
>  
>  	ret = rockchip_i2s_init_dai(i2s, res, &dai);
>  	if (ret)
> -		goto err_pm_disable;
> +		return ret;
>  
>  	ret = devm_snd_soc_register_component(&pdev->dev,
>  					      &rockchip_i2s_component,
> @@ -828,36 +833,16 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
>  
>  	if (ret) {
>  		dev_err(&pdev->dev, "Could not register DAI\n");
> -		goto err_suspend;
> +		return ret;
>  	}
>  
>  	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Could not register PCM\n");
> -		goto err_suspend;
> +		return ret;
>  	}
>  
>  	return 0;
> -
> -err_suspend:
> -	if (!pm_runtime_status_suspended(&pdev->dev))
> -		i2s_runtime_suspend(&pdev->dev);
> -err_pm_disable:
> -	pm_runtime_disable(&pdev->dev);
> -err_clk:
> -	clk_disable_unprepare(i2s->hclk);
> -	return ret;
> -}
> -
> -static void rockchip_i2s_remove(struct platform_device *pdev)
> -{
> -	struct rk_i2s_dev *i2s = dev_get_drvdata(&pdev->dev);
> -
> -	pm_runtime_disable(&pdev->dev);
> -	if (!pm_runtime_status_suspended(&pdev->dev))
> -		i2s_runtime_suspend(&pdev->dev);
> -
> -	clk_disable_unprepare(i2s->hclk);
>  }
>  
>  static const struct dev_pm_ops rockchip_i2s_pm_ops = {
> @@ -866,7 +851,6 @@ static const struct dev_pm_ops rockchip_i2s_pm_ops = {
>  
>  static struct platform_driver rockchip_i2s_driver = {
>  	.probe = rockchip_i2s_probe,
> -	.remove = rockchip_i2s_remove,
>  	.driver = {
>  		.name = DRV_NAME,
>  		.of_match_table = of_match_ptr(rockchip_i2s_match),
> 
> ---
> base-commit: 40cc9602caf2539369bd3dd7d66ee67e204e75ef
> change-id: 20260521-asoc-rockchip-i2s-devm-cleanup-6cade5f495e5
> 
> Best regards,
> --  
> Cássio Gabriel <cassiogabrielcontato@gmail.com>
> 
> 

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