[PATCH 0/3] ASoC: rockchip: Reorder clock enable sequence

[PATCH 0/3] ASoC: rockchip: Reorder clock enable sequence

[phucduc.bui]

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

Hi all,

This series reorders the runtime resume clock enable
sequence in the Rockchip SPDIF and PDM drivers to enable
the bus clock before the functional controller clock.

It also updates the SPDIF DT binding clock descriptions to
match the actual clock usage in the driver.

Best Regards,
Phuc

bui duc phuc (3):
  ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions
  ASoC: rockchip: spdif: Reorder clock enable sequence
  ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

 .../devicetree/bindings/sound/rockchip-spdif.yaml      |  2 +-
 sound/soc/rockchip/rockchip_pdm.c                      | 10 +++++-----
 sound/soc/rockchip/rockchip_spdif.c                    | 10 +++++-----
 3 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH 0/3] ASoC: rockchip: Reorder clock enable sequence

[phucduc.bui]

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

Hi all,

This series reorders the runtime resume clock enable
sequence in the Rockchip SPDIF and PDM drivers to enable
the bus clock before the functional controller clock.

It also updates the SPDIF DT binding clock descriptions to
match the actual clock usage in the driver.

Best Regards,
Phuc

bui duc phuc (3):
  ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions
  ASoC: rockchip: spdif: Reorder clock enable sequence
  ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

 .../devicetree/bindings/sound/rockchip-spdif.yaml      |  2 +-
 sound/soc/rockchip/rockchip_pdm.c                      | 10 +++++-----
 sound/soc/rockchip/rockchip_spdif.c                    | 10 +++++-----
 3 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 1/3] ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions

[phucduc.bui]

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

The clock descriptions are currently swapped relative to the
clock names used by the driver.

Update the binding descriptions to match the actual clock
usage, where 'mclk' is the controller clock and 'hclk' is
the bus clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 Documentation/devicetree/bindings/sound/rockchip-spdif.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
index 502907dd28b3..b174d7498029 100644
--- a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
+++ b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
@@ -45,8 +45,8 @@ properties:
 
   clocks:
     items:
-      - description: clock for SPDIF bus
       - description: clock for SPDIF controller
+      - description: clock for SPDIF bus
 
   clock-names:
     items:
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 1/3] ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions

[phucduc.bui]

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

The clock descriptions are currently swapped relative to the
clock names used by the driver.

Update the binding descriptions to match the actual clock
usage, where 'mclk' is the controller clock and 'hclk' is
the bus clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 Documentation/devicetree/bindings/sound/rockchip-spdif.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
index 502907dd28b3..b174d7498029 100644
--- a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
+++ b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
@@ -45,8 +45,8 @@ properties:
 
   clocks:
     items:
-      - description: clock for SPDIF bus
       - description: clock for SPDIF controller
+      - description: clock for SPDIF bus
 
   clock-names:
     items:
-- 
2.43.0

[PATCH 2/3] ASoC: rockchip: spdif: Reorder clock enable sequence

[phucduc.bui]

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

Enable the 'hclk' bus clock before the 'mclk' controller
clock during runtime resume.
The bus clock provides the register access interface and
should be enabled before the controller clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/rockchip/rockchip_spdif.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
index 581624f2682e..8de5b76cfe79 100644
--- a/sound/soc/rockchip/rockchip_spdif.c
+++ b/sound/soc/rockchip/rockchip_spdif.c
@@ -76,16 +76,16 @@ static int rk_spdif_runtime_resume(struct device *dev)
 	struct rk_spdif_dev *spdif = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(spdif->mclk);
+	ret = clk_prepare_enable(spdif->hclk);
 	if (ret) {
-		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
+		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(spdif->hclk);
+	ret = clk_prepare_enable(spdif->mclk);
 	if (ret) {
-		clk_disable_unprepare(spdif->mclk);
-		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
+		clk_disable_unprepare(spdif->hclk);
+		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.43.0

[PATCH 2/3] ASoC: rockchip: spdif: Reorder clock enable sequence

[phucduc.bui]

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

Enable the 'hclk' bus clock before the 'mclk' controller
clock during runtime resume.
The bus clock provides the register access interface and
should be enabled before the controller clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/rockchip/rockchip_spdif.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
index 581624f2682e..8de5b76cfe79 100644
--- a/sound/soc/rockchip/rockchip_spdif.c
+++ b/sound/soc/rockchip/rockchip_spdif.c
@@ -76,16 +76,16 @@ static int rk_spdif_runtime_resume(struct device *dev)
 	struct rk_spdif_dev *spdif = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(spdif->mclk);
+	ret = clk_prepare_enable(spdif->hclk);
 	if (ret) {
-		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
+		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(spdif->hclk);
+	ret = clk_prepare_enable(spdif->mclk);
 	if (ret) {
-		clk_disable_unprepare(spdif->mclk);
-		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
+		clk_disable_unprepare(spdif->hclk);
+		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 3/3] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[phucduc.bui]

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

Enable the 'hclk' bus clock before the 'clk' controller
clock during runtime resume.
The bus clock provides the register access interface and
should be enabled before the controller clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/rockchip/rockchip_pdm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
index c69cdd6f2499..8f78f7bc1806 100644
--- a/sound/soc/rockchip/rockchip_pdm.c
+++ b/sound/soc/rockchip/rockchip_pdm.c
@@ -422,16 +422,16 @@ static int rockchip_pdm_runtime_resume(struct device *dev)
 	struct rk_pdm_dev *pdm = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(pdm->clk);
+	ret = clk_prepare_enable(pdm->hclk);
 	if (ret) {
-		dev_err(pdm->dev, "clock enable failed %d\n", ret);
+		dev_err(pdm->dev, "hclock enable failed %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pdm->hclk);
+	ret = clk_prepare_enable(pdm->clk);
 	if (ret) {
-		clk_disable_unprepare(pdm->clk);
-		dev_err(pdm->dev, "hclock enable failed %d\n", ret);
+		clk_disable_unprepare(pdm->hclk);
+		dev_err(pdm->dev, "clock enable failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.43.0

[PATCH 3/3] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[phucduc.bui]

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

Enable the 'hclk' bus clock before the 'clk' controller
clock during runtime resume.
The bus clock provides the register access interface and
should be enabled before the controller clock.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/rockchip/rockchip_pdm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
index c69cdd6f2499..8f78f7bc1806 100644
--- a/sound/soc/rockchip/rockchip_pdm.c
+++ b/sound/soc/rockchip/rockchip_pdm.c
@@ -422,16 +422,16 @@ static int rockchip_pdm_runtime_resume(struct device *dev)
 	struct rk_pdm_dev *pdm = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(pdm->clk);
+	ret = clk_prepare_enable(pdm->hclk);
 	if (ret) {
-		dev_err(pdm->dev, "clock enable failed %d\n", ret);
+		dev_err(pdm->dev, "hclock enable failed %d\n", ret);
 		return ret;
 	}
 
-	ret = clk_prepare_enable(pdm->hclk);
+	ret = clk_prepare_enable(pdm->clk);
 	if (ret) {
-		clk_disable_unprepare(pdm->clk);
-		dev_err(pdm->dev, "hclock enable failed %d\n", ret);
+		clk_disable_unprepare(pdm->hclk);
+		dev_err(pdm->dev, "clock enable failed %d\n", ret);
 		return ret;
 	}
 
-- 
2.43.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/3] ASoC: rockchip: spdif: Reorder clock enable sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Failure to restore regmap cache-only state if `regcache_sync()` fails during runtime resume.
--

commit 517eeaafec0deb4d7a25eb1c2641316c2b2e2b72
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: rockchip: spdif: Reorder clock enable sequence

This commit reorders the clock enablement sequence during runtime resume.
It ensures the bus clock (hclk) is prepared and enabled before the
controller clock (mclk) to guarantee proper register access.

> diff --git a/sound/soc/rockchip/rockchip_spdif.c b/sound/soc/rockchip/rockchip_spdif.c
> --- a/sound/soc/rockchip/rockchip_spdif.c
> +++ b/sound/soc/rockchip/rockchip_spdif.c
[ ... ]
> @@ -76,16 +76,16 @@ static int rk_spdif_runtime_resume(struct device *dev)
[ ... ]
> -	ret = clk_prepare_enable(spdif->hclk);
> +	ret = clk_prepare_enable(spdif->mclk);
>  	if (ret) {
> -		clk_disable_unprepare(spdif->mclk);
> -		dev_err(spdif->dev, "hclk clock enable failed %d\n", ret);
> +		clk_disable_unprepare(spdif->hclk);
> +		dev_err(spdif->dev, "mclk clock enable failed %d\n", ret);
>  		return ret;
>  	}
>  
>  	regcache_cache_only(spdif->regmap, false);
>  	regcache_mark_dirty(spdif->regmap);
>  
>  	ret = regcache_sync(spdif->regmap);
>  	if (ret) {
>  		clk_disable_unprepare(spdif->mclk);
>  		clk_disable_unprepare(spdif->hclk);

[Severity: High]
This is a pre-existing issue, but is the regmap cache-only state left
unrestored on this error path?

If regcache_sync() fails, the driver disables the clocks and returns an error.
This causes the runtime power management core to leave the device in the
suspended state.

However, the error path doesn't appear to restore the cache-only mode via
regcache_cache_only(spdif->regmap, true).

If a subsequent register read or write occurs while the device is in this
suspended state, could the regmap bypass the cache and attempt a physical
memory-mapped I/O access?

Since the generic power domain would be powered off when suspended,
attempting this access might trigger an asynchronous system error or
synchronous data abort.

>  	}
>  
>  	return ret;
>  }

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

Re: [PATCH 3/3] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Unchecked return value of `pm_runtime_get_sync()` in `rockchip_pdm_set_fmt()` can lead to hardware access while suspended.
- [High] Manual disable of hardware clocks and PM runtime before devm teardown can cause bus faults.
--

commit 8f19ac4b34f49c4a08218121167b705580c4469a
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

Enable the 'hclk' bus clock before the 'clk' controller clock during runtime
resume. The bus clock provides the register access interface and should be
enabled before the controller clock.

> diff --git a/sound/soc/rockchip/rockchip_pdm.c b/sound/soc/rockchip/rockchip_pdm.c
> index c69cdd6f24994..8f78f7bc1806c 100644
> --- a/sound/soc/rockchip/rockchip_pdm.c
> +++ b/sound/soc/rockchip/rockchip_pdm.c

[ ... ]

While reviewing this patch, I noticed a couple of pre-existing issues in the
same file:

sound/soc/rockchip/rockchip_pdm.c:rockchip_pdm_set_fmt() {
...
	pm_runtime_get_sync(cpu_dai->dev);
	regmap_update_bits(pdm->regmap, PDM_CLK_CTRL, mask, val);
	pm_runtime_put(cpu_dai->dev);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to ignore the
return value of pm_runtime_get_sync() here?

If the runtime resume fails (for example, if clock enablement or power
domain restoration fails), the function ignores the negative error code
and proceeds to call regmap_update_bits().

Accessing hardware registers when the hardware is powered down or unclocked
can lead to a synchronous external abort (bus fault) and a kernel panic.

Should pm_runtime_resume_and_get() be used instead, and the error handled?


sound/soc/rockchip/rockchip_pdm.c:rockchip_pdm_remove() {
...
static void rockchip_pdm_remove(struct platform_device *pdev)
{
	struct rk_pdm_dev *pdm = dev_get_drvdata(&pdev->dev);

	pm_runtime_disable(&pdev->dev);
	if (!pm_runtime_status_suspended(&pdev->dev))
		rockchip_pdm_runtime_suspend(&pdev->dev);

	clk_disable_unprepare(pdm->clk);
	clk_disable_unprepare(pdm->hclk);
}
...
}

[Severity: High]
This is a pre-existing issue, but can manually disabling the clocks and PM
runtime before the devm teardown cause bus faults?

The driver registers the ASoC component using
devm_snd_soc_register_component() in rockchip_pdm_probe(), which defers
unregistration until after the remove() callback returns.

If the device is unbound via sysfs while an audio stream is active, ALSA
will attempt to tear down the stream during the deferred devres
unregistration. This triggers operations like
rockchip_pdm_trigger(SNDRV_PCM_TRIGGER_STOP), which writes to the
hardware registers.

Since the clocks and power domains were already forcibly disabled in
remove(), this could result in a bus fault and kernel panic.

Should the driver use devm_pm_runtime_enable() with a devres action for
clock disable, or unregister the component manually before turning off
resources?

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

Re: [PATCH 1/3] ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions

[Krzysztof Kozlowski]

On Fri, May 22, 2026 at 05:03:16PM +0700, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> The clock descriptions are currently swapped relative to the
> clock names used by the driver.

Why would order of clock names in the driver matter here? I do not
understand that explanation.

> 
> Update the binding descriptions to match the actual clock
> usage, where 'mclk' is the controller clock and 'hclk' is
> the bus clock.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  Documentation/devicetree/bindings/sound/rockchip-spdif.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
> index 502907dd28b3..b174d7498029 100644
> --- a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
> +++ b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
> @@ -45,8 +45,8 @@ properties:
>  
>    clocks:
>      items:
> -      - description: clock for SPDIF bus
>        - description: clock for SPDIF controller
> +      - description: clock for SPDIF bus

So example is wrong?

What about all the users?

Best regards,
Krzysztof


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions

[Krzysztof Kozlowski]

On Fri, May 22, 2026 at 05:03:16PM +0700, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> The clock descriptions are currently swapped relative to the
> clock names used by the driver.

Why would order of clock names in the driver matter here? I do not
understand that explanation.

> 
> Update the binding descriptions to match the actual clock
> usage, where 'mclk' is the controller clock and 'hclk' is
> the bus clock.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  Documentation/devicetree/bindings/sound/rockchip-spdif.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
> index 502907dd28b3..b174d7498029 100644
> --- a/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
> +++ b/Documentation/devicetree/bindings/sound/rockchip-spdif.yaml
> @@ -45,8 +45,8 @@ properties:
>  
>    clocks:
>      items:
> -      - description: clock for SPDIF bus
>        - description: clock for SPDIF controller
> +      - description: clock for SPDIF bus

So example is wrong?

What about all the users?

Best regards,
Krzysztof

Re: [PATCH 1/3] ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions

[Bui Duc Phuc]

Hi Krzysztof,

Thank you for your review .

> So example is wrong?
>
> What about all the users?

I only updated the internal description text to match the existing clock-names.
The clock ordering (mclk followed by hclk) remains unchanged,
so external .dts files and the example section at the end of the
binding are unaffected.

> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>

Thank you for the guidance. I will address this in v2.

> > The clock descriptions are currently swapped relative to the
> > clock names used by the driver.
>
> Why would order of clock names in the driver matter here? I do not
> understand that explanation.
>

I inferred that 'hclk' is the bus clock from the driver, which uses
devm_regmap_init_mmio_clk() with 'hclk',
and this matches the description found in several Rockchip datasheets.
However, I understand that the binding should be described
independently of the driver implementation.
Therefore, I'll drop this description in the next revision.

Best Regards,
Phuc

Re: [PATCH 1/3] ASoC: dt-bindings: rockchip-spdif: Correct SPDIF clock descriptions

[Bui Duc Phuc]

Hi Krzysztof,

Thank you for your review .

> So example is wrong?
>
> What about all the users?

I only updated the internal description text to match the existing clock-names.
The clock ordering (mclk followed by hclk) remains unchanged,
so external .dts files and the example section at the end of the
binding are unaffected.

> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>

Thank you for the guidance. I will address this in v2.

> > The clock descriptions are currently swapped relative to the
> > clock names used by the driver.
>
> Why would order of clock names in the driver matter here? I do not
> understand that explanation.
>

I inferred that 'hclk' is the bus clock from the driver, which uses
devm_regmap_init_mmio_clk() with 'hclk',
and this matches the description found in several Rockchip datasheets.
However, I understand that the binding should be described
independently of the driver implementation.
Therefore, I'll drop this description in the next revision.

Best Regards,
Phuc

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/3] ASoC: rockchip: spdif: Reorder clock enable sequence

[Bui Duc Phuc]

Thank you for the review.
I agree with your analysis. This appears to be a valid issue, and the
fix should
be relatively small and localized. I do not have the hardware on hand
for runtime
 testing, but I'll prepare a follow-up patch.

> [Severity: High]
> This is a pre-existing issue, but is the regmap cache-only state left
> unrestored on this error path?
>
> If regcache_sync() fails, the driver disables the clocks and returns an error.
> This causes the runtime power management core to leave the device in the
> suspended state.
>
> However, the error path doesn't appear to restore the cache-only mode via
> regcache_cache_only(spdif->regmap, true).
>
> If a subsequent register read or write occurs while the device is in this
> suspended state, could the regmap bypass the cache and attempt a physical
> memory-mapped I/O access?
>
> Since the generic power domain would be powered off when suspended,
> attempting this access might trigger an asynchronous system error or
> synchronous data abort.
>
> >       }
> >
> >       return ret;
> >  }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260522100318.73474-1-phucduc.bui@gmail.com?part=2

Re: [PATCH 3/3] ASoC: rockchip: rockchip_pdm: Reorder clock enable sequence

[Bui Duc Phuc]

>
> [Severity: High]
> This is a pre-existing issue, but can manually disabling the clocks and PM
> runtime before the devm teardown cause bus faults?
>
> The driver registers the ASoC component using
> devm_snd_soc_register_component() in rockchip_pdm_probe(), which defers
> unregistration until after the remove() callback returns.
>
> If the device is unbound via sysfs while an audio stream is active, ALSA
> will attempt to tear down the stream during the deferred devres
> unregistration. This triggers operations like
> rockchip_pdm_trigger(SNDRV_PCM_TRIGGER_STOP), which writes to the
> hardware registers.
>
> Since the clocks and power domains were already forcibly disabled in
> remove(), this could result in a bus fault and kernel panic.
>
> Should the driver use devm_pm_runtime_enable() with a devres action for
> clock disable, or unregister the component manually before turning off
> resources?
>

Thank you for pointing this out.

I agree that your concern makes sense, and the teardown ordering may
indeed deserve further investigation.
However, addressing it appears to require a relatively significant change
to the driver's resource management and lifecycle handling. Since I do
not have the hardware on hand, I am not able to validate either the reported
scenario or a potential fix.
Given that, I don't feel comfortable submitting a speculative patch that
I cannot test properly. For now, I'll leave this issue to someone with the
hardware or with more familiarity with the expected teardown sequence
in this driver.


>
> [Severity: High]
> This isn't a bug introduced by this patch, but is it safe to ignore the
> return value of pm_runtime_get_sync() here?
>
> If the runtime resume fails (for example, if clock enablement or power
> domain restoration fails), the function ignores the negative error code
> and proceeds to call regmap_update_bits().
>
> Accessing hardware registers when the hardware is powered down or unclocked
> can lead to a synchronous external abort (bus fault) and a kernel panic.
>
> Should pm_runtime_resume_and_get() be used instead, and the error handled?
>

I agree this looks like a valid issue. The proposed fix is relatively
 small and localized, and does not affect the overall resource lifetime
 handling of the driver. I'll prepare a follow-up patch for it, although
I can only perform compile testing since I don't have the hardware
on hand.