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

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

[Cássio Gabriel]

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>
---
 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>

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

[Sebastian Reichel]

[-- 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 --]

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

[Mark Brown]

On Thu, 21 May 2026 23:30:07 -0300, Cássio Gabriel wrote:
> ASoC: rockchip: i2s: Use managed hclk and runtime PM cleanup

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-7.2

Thanks!

[1/1] ASoC: rockchip: i2s: Use managed hclk and runtime PM cleanup
      https://git.kernel.org/broonie/sound/c/2d90ecdfa326

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark