[PATCH] ASoC: atmel-i2s: Use devm_clk_get_enabled() for pclk

[PATCH] ASoC: atmel-i2s: Use devm_clk_get_enabled() for pclk

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The peripheral clock is only acquired and enabled once during
probe. Use devm_clk_get_enabled() to combine clock acquisition
and enabling into a single call.

This also removes the need for manual clock disable handling
on probe failures and in the remove callback, as the devm API
automatically handles clock cleanup.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/atmel/atmel-i2s.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/sound/soc/atmel/atmel-i2s.c b/sound/soc/atmel/atmel-i2s.c
index e1d8af056234..a3aeb8e607f6 100644
--- a/sound/soc/atmel/atmel-i2s.c
+++ b/sound/soc/atmel/atmel-i2s.c
@@ -660,8 +660,8 @@ static int atmel_i2s_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	/* Get the peripheral clock. */
-	dev->pclk = devm_clk_get(&pdev->dev, "pclk");
+	/* Get and enable the peripheral clock. */
+	dev->pclk = devm_clk_get_enabled(&pdev->dev, "pclk");
 	if (IS_ERR(dev->pclk))
 		return dev_err_probe(&pdev->dev, PTR_ERR(dev->pclk),
 				     "failed to get the peripheral clock\n");
@@ -682,11 +682,6 @@ static int atmel_i2s_probe(struct platform_device *pdev)
 			return err;
 	}
 
-	/* Enable the peripheral clock. */
-	err = clk_prepare_enable(dev->pclk);
-	if (err)
-		return err;
-
 	/* Get IP version. */
 	regmap_read(dev->regmap, ATMEL_I2SC_VERSION, &version);
 	dev_info(&pdev->dev, "hw version: %#x\n", version);
@@ -698,10 +693,8 @@ static int atmel_i2s_probe(struct platform_device *pdev)
 	err = devm_snd_soc_register_component(&pdev->dev,
 					      &atmel_i2s_component,
 					      &atmel_i2s_dai, 1);
-	if (err) {
-		clk_disable_unprepare(dev->pclk);
+	if (err)
 		return err;
-	}
 
 	/* Prepare DMA config. */
 	dev->playback.addr	= (dma_addr_t)mem->start + ATMEL_I2SC_THR;
@@ -712,28 +705,18 @@ static int atmel_i2s_probe(struct platform_device *pdev)
 	if (of_property_match_string(np, "dma-names", "rx-tx") == 0)
 		pcm_flags |= SND_DMAENGINE_PCM_FLAG_HALF_DUPLEX;
 	err = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, pcm_flags);
-	if (err) {
-		clk_disable_unprepare(dev->pclk);
+	if (err)
 		return err;
-	}
 
 	return 0;
 }
 
-static void atmel_i2s_remove(struct platform_device *pdev)
-{
-	struct atmel_i2s_dev *dev = platform_get_drvdata(pdev);
-
-	clk_disable_unprepare(dev->pclk);
-}
-
 static struct platform_driver atmel_i2s_driver = {
 	.driver		= {
 		.name	= "atmel_i2s",
 		.of_match_table	= atmel_i2s_dt_ids,
 	},
 	.probe		= atmel_i2s_probe,
-	.remove		= atmel_i2s_remove,
 };
 module_platform_driver(atmel_i2s_driver);
 
-- 
2.43.0

Re: [PATCH] ASoC: atmel-i2s: Use devm_clk_get_enabled() for pclk

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ASoC: atmel-i2s: wrong IRQ and pclk ordering in
  atmel_i2s_probe()
--

commit 722d5e2f3add0fa840d1f0e68c8baa8331615c85
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: atmel-i2s: Use devm_clk_get_enabled() for pclk

This commit replaces devm_clk_get() and clk_prepare_enable() for the
peripheral clock with devm_clk_get_enabled(), removing the manual clock
cleanup paths and the driver remove callback.

> diff --git a/sound/soc/atmel/atmel-i2s.c b/sound/soc/atmel/atmel-i2s.c
> index e1d8af0562341..a3aeb8e607f61 100644
> --- a/sound/soc/atmel/atmel-i2s.c
> +++ b/sound/soc/atmel/atmel-i2s.c
> @@ -660,8 +660,8 @@ static int atmel_i2s_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> -	/* Get the peripheral clock. */
> -	dev->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	/* Get and enable the peripheral clock. */
> +	dev->pclk = devm_clk_get_enabled(&pdev->dev, "pclk");
>  	if (IS_ERR(dev->pclk))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(dev->pclk),
>  				     "failed to get the peripheral clock\n");

[Severity: Medium]
Can enabling dev->pclk here cause atmel_i2s_interrupt() to dereference a
NULL dev->regmap or access registers while dev->pclk is disabled during
teardown?

atmel_i2s_probe() registers atmel_i2s_interrupt() via devm_request_irq()
just before this call, while dev->regmap and dev->dev are only assigned
after devm_clk_get_optional():

sound/soc/atmel/atmel-i2s.c:atmel_i2s_probe() {
	...
	err = devm_request_irq(&pdev->dev, irq, atmel_i2s_interrupt, 0,
			       dev_name(&pdev->dev), dev);
	if (err)
		return err;

	/* Get and enable the peripheral clock. */
	dev->pclk = devm_clk_get_enabled(&pdev->dev, "pclk");
	...
	dev->gclk = devm_clk_get_optional(&pdev->dev, "gclk");
	if (IS_ERR(dev->gclk))
		return PTR_ERR(dev->gclk);

	dev->dev = &pdev->dev;
	dev->regmap = regmap;
	...
}

Previously, clk_prepare_enable(dev->pclk) was called after dev->regmap
and dev->dev were initialized.  With devm_clk_get_enabled(), the
peripheral clock is enabled while dev->regmap is still NULL, so a pending
interrupt (for example across a driver rebind, as ATMEL_I2SC_IER is never
masked on unbind) will hit a NULL pointer dereference in
regmap_read(dev->regmap, ATMEL_I2SC_SR, &sr).

In addition, because devm_clk_get_enabled() is registered after
devm_request_irq(), devres teardown on probe failure or driver unbind now
disables dev->pclk before freeing the IRQ, leaving a window where
atmel_i2s_interrupt() can read I2S registers while the peripheral clock
is gated.  Initializing dev->dev and dev->regmap and calling
devm_clk_get_enabled() before devm_request_irq() would avoid both issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111042746.14714-1-phucduc.bui@gmail.com?part=1

Re: [PATCH] ASoC: atmel-i2s: Use devm_clk_get_enabled() for pclk

[Bui Duc Phuc]

> [Severity: Medium]
> Can enabling dev->pclk here cause atmel_i2s_interrupt() to dereference a
> NULL dev->regmap or access registers while dev->pclk is disabled during
> teardown?
>

This scenario is unlikely, but it is possible.
To be safe, I'll move the devm_request_irq() call below the clock
initialization
and dev->regmap setup.