Skip to contentSkip to conversation

ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements

PATCH In review [email protected] 3 participants 6 messages Jun 2 – Nov 7

Subsystems (3)
Sound ASoC Arm/Allwinner sunXi SoC support
lore
Share & tools
lore

[email protected]

author
#0 lore raw

v3 cover ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements

From: bui duc phuc <[email protected]>

Hi,

This series contains a few improvements for the sun4i-spdif driver, including guard() conversions and ensuring the device is resumed via runtime PM before kcontrol register accesses.

Link v1:
https://lore.kernel.org/all/[email protected]/

Link v2:
https://lore.kernel.org/all/[email protected]/

Change in v2:
- Switched from using guard() to scoped_guard()
- Added runtime PM handling for kcontrol register accesses.
Change in v3:
- Clarify in the commit message that the resume sequence becomes the reverse of the suspend sequence.
- Add Reported-by tag from Sashiko AI review.

Best Regards,
Phuc

bui duc phuc (3):
ASoC: sunxi: sun4i-spdif: Use guard() for spin locks
ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register
access
ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

 sound/soc/sunxi/sun4i-spdif.c | 76 +++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 34 deletions(-)
Signature · 1 line
2.43.0
author
#1 lore raw

PATCH v3 1/3 ASoC: sunxi: sun4i-spdif: Use guard() for spin locks

From: bui duc phuc <[email protected]>

Clean up the code using guard() for spin locks.
Merely code refactoring, and no behavior change.

Signed-off-by bui duc phuc <[email protected]>

---

NOTE: This patch is compile-tested only.

 sound/soc/sunxi/sun4i-spdif.c | 62 ++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 34 deletions(-)
sound/soc/sunxi/sun4i-spdif.c +28 -34
index c2ec19437cd7..89eccc83a130 100644
@@ -427,24 +427,21 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
-	unsigned long flags;
 	unsigned int reg;
 
-	spin_lock_irqsave(&host->lock, flags);
+	scoped_guard(spinlock_irqsave, &host->lock) {
+		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
 
-	regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);
+		status[0] = reg & 0xff;
+		status[1] = (reg >> 8) & 0xff;
+		status[2] = (reg >> 16) & 0xff;
+		status[3] = (reg >> 24) & 0xff;
 
-	status[0] = reg & 0xff;
-	status[1] = (reg >> 8) & 0xff;
-	status[2] = (reg >> 16) & 0xff;
-	status[3] = (reg >> 24) & 0xff;
+		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA1, &reg);
 
-	regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA1, &reg);
-
-	status[4] = reg & 0xff;
-	status[5] = (reg >> 8) & 0x3;
-
-	spin_unlock_irqrestore(&host->lock, flags);
+		status[4] = reg & 0xff;
+		status[5] = (reg >> 8) & 0x3;
+	}
 
 	return 0;
 }

@@ -455,35 +452,32 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
-	unsigned long flags;
 	unsigned int reg;
 	bool chg0, chg1;
 
-	spin_lock_irqsave(&host->lock, flags);
-
-	reg = (u32)status[3] << 24;
-	reg |= (u32)status[2] << 16;
-	reg |= (u32)status[1] << 8;
-	reg |= (u32)status[0];
+	scoped_guard(spinlock_irqsave, &host->lock) {
+		reg = (u32)status[3] << 24;
+		reg |= (u32)status[2] << 16;
+		reg |= (u32)status[1] << 8;
+		reg |= (u32)status[0];
 
-	regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA0,
-				 GENMASK(31,0), reg, &chg0);
+		regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA0,
+					 GENMASK(31, 0), reg, &chg0);
 
-	reg = (u32)status[5] << 8;
-	reg |= (u32)status[4];
+		reg = (u32)status[5] << 8;
+		reg |= (u32)status[4];
 
-	regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA1,
-				 GENMASK(9,0), reg, &chg1);
+		regmap_update_bits_check(host->regmap, SUN4I_SPDIF_TXCHSTA1,
+					 GENMASK(9, 0), reg, &chg1);
 
-	reg = SUN4I_SPDIF_TXCFG_CHSTMODE;
-	if (status[0] & IEC958_AES0_NONAUDIO)
-		reg |= SUN4I_SPDIF_TXCFG_NONAUDIO;
+		reg = SUN4I_SPDIF_TXCFG_CHSTMODE;
+		if (status[0] & IEC958_AES0_NONAUDIO)
+			reg |= SUN4I_SPDIF_TXCFG_NONAUDIO;
 
-	regmap_update_bits(host->regmap, SUN4I_SPDIF_TXCFG,
-			   SUN4I_SPDIF_TXCFG_CHSTMODE |
-			   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
-
-	spin_unlock_irqrestore(&host->lock, flags);
+		regmap_update_bits(host->regmap, SUN4I_SPDIF_TXCFG,
+				   SUN4I_SPDIF_TXCFG_CHSTMODE |
+				   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
+	}
 
 	return chg0 || chg1;
 }
Signature · 1 line
2.43.0
author
#2 lore raw

PATCH v3 2/3 ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

In reply to [email protected]

From: bui duc phuc <[email protected]>

The IEC958 status kcontrols access hardware registers directly. Take a runtime PM reference while accessing the registers so that the device remains active for the duration of the operation.

Reported-by Sashiko AI Review <[email protected]>
Closes https://lore.kernel.org/all/[email protected]/ open here
Signed-off-by bui duc phuc <[email protected]>

---

NOTE: This patch is compile-tested only.
Changes in v3:
- Add Reported-by tag from Sashiko AI review.

 sound/soc/sunxi/sun4i-spdif.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
sound/soc/sunxi/sun4i-spdif.c +14 -0
index 89eccc83a130..f54eb14c9ed8 100644
@@ -428,6 +428,11 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
 	unsigned int reg;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(cpu_dai->dev);
+	if (ret)
+		return ret;
 
 	scoped_guard(spinlock_irqsave, &host->lock) {
 		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);

@@ -443,6 +448,8 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
 		status[5] = (reg >> 8) & 0x3;
 	}
 
+	pm_runtime_put(cpu_dai->dev);
+
 	return 0;
 }
 

@@ -453,8 +460,13 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
 	u8 *status = ucontrol->value.iec958.status;
 	unsigned int reg;
+	int ret;
 	bool chg0, chg1;
 
+	ret = pm_runtime_resume_and_get(cpu_dai->dev);
+	if (ret)
+		return ret;
+
 	scoped_guard(spinlock_irqsave, &host->lock) {
 		reg = (u32)status[3] << 24;
 		reg |= (u32)status[2] << 16;

@@ -479,6 +491,8 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
 				   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
 	}
 
+	pm_runtime_put(cpu_dai->dev);
+
 	return chg0 || chg1;
 }
Signature · 1 line
2.43.0
author
#3 lore raw

PATCH v3 3/3 ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

In reply to [email protected]

From: bui duc phuc <[email protected]>

Reorder the runtime resume clock enable sequence to match the hardware dependency and ensure symmetry with the suspend path.
The APB bus clock drives the register interface and must be enabled before the functional module clock. This aligns the resume sequence to be the exact reverse of the suspend sequence.

Signed-off-by bui duc phuc <[email protected]>

---

NOTE: This patch is compile-tested only.
Changes in v3:
- Clarify in the commit message that the resume sequence becomes the reverse of the suspend sequence.

 sound/soc/sunxi/sun4i-spdif.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
sound/soc/sunxi/sun4i-spdif.c +4 -4
index f54eb14c9ed8..102db1a2afbb 100644
@@ -643,15 +643,15 @@ static int sun4i_spdif_runtime_suspend(struct device *dev)
 
 static int sun4i_spdif_runtime_resume(struct device *dev)
 {
-	struct sun4i_spdif_dev *host  = dev_get_drvdata(dev);
+	struct sun4i_spdif_dev *host = dev_get_drvdata(dev);
 	int ret;
 
-	ret = clk_prepare_enable(host->spdif_clk);
+	ret = clk_prepare_enable(host->apb_clk);
 	if (ret)
 		return ret;
-	ret = clk_prepare_enable(host->apb_clk);
+	ret = clk_prepare_enable(host->spdif_clk);
 	if (ret)
-		clk_disable_unprepare(host->spdif_clk);
+		clk_disable_unprepare(host->apb_clk);
 
 	return ret;
 }
Signature · 1 line
2.43.0
AI review
#4 lore raw

Re: [PATCH v3 3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

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

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

Pre-existing issues:
- [High] Unbinding the driver while an audio stream is active will cause a fatal bus fault because hardware is suspended before the ASoC component is unregistered. --

commit 811edbc583bdf36ef4938bd44bba42ed394e19bd
Author: bui duc phuc <[email protected]>

ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence

Reorder the runtime resume clock enable sequence to match the hardware dependency and ensure symmetry with the suspend path.
The APB bus clock drives the register interface and must be enabled before the functional module clock. This aligns the resume sequence to be the exact reverse of the suspend sequence.

Quoted 8 lines
sound/soc/sunxi/sun4i-spdif.c +0 -0
index f54eb14c9ed86..102db1a2afbb8 100644
@@ -643,15 +643,15 @@ static int sun4i_spdif_runtime_suspend(struct device *dev)
 
 static int sun4i_spdif_runtime_resume(struct device *dev)
 {

[ ... ]

sound/soc/sunxi/sun4i-spdif.c static int sun4i_spdif_runtime_suspend(struct device *dev) · line 657

Quoted 13 lines
sound/soc/sunxi/sun4i-spdif.c +4 -4
-	ret = clk_prepare_enable(host->spdif_clk);
-	if (ret)
-		return ret;
 	ret = clk_prepare_enable(host->apb_clk);
 	if (ret)
-		clk_disable_unprepare(host->spdif_clk);
+		return ret;
+	ret = clk_prepare_enable(host->spdif_clk);
+	if (ret)
+		clk_disable_unprepare(host->apb_clk);
 
 	return ret;
 }

[Severity: High]
This isn't a bug introduced by this patch, but unbinding the driver while an audio stream is active can cause a fatal bus fault.

Looking at sun4i_spdif_remove(), it manually disables PM and suspends the hardware:

sun4i_spdif_remove() {
    pm_runtime_disable(&pdev->dev);
    if (!pm_runtime_status_suspended(&pdev->dev))
        sun4i_spdif_runtime_suspend(&pdev->dev);
}

Because devres unregisters the ASoC component after sun4i_spdif_remove() returns, the ALSA core responds to the unregistration by tearing down the active stream. This invokes callbacks like sun4i_spdif_trigger() with SNDRV_PCM_TRIGGER_STOP:

sun4i_spdif_trigger() {
    case SNDRV_PCM_TRIGGER_STOP:
    ...
        sun4i_snd_txctrl_off(substream, host);
        break;
}

This tries to write to the device registers via regmap to disable transmission. Since the APB bus clock was already disabled in sun4i_spdif_remove(), accessing the registers causes a Data Abort and panics the kernel.

Could the PM teardown be managed using devm_add_action_or_reset(), registered immediately after pm_runtime_enable() and before component registration, to ensure the ASoC component is fully unregistered before the clocks are turned off?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
maintainer
#5 lore raw
In reply to [email protected]
Quoted 11 lines · On Tue, 02 Jun 2026 18:44:34 +0700, [email protected] wrote:

ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements

From: bui duc phuc <[email protected]>

Hi,

This series contains a few improvements for the sun4i-spdif driver, including guard() conversions and ensuring the device is resumed via runtime PM before kcontrol register accesses.

[...]

Applied to

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

Thanks!

[1/3] ASoC: sunxi: sun4i-spdif: Use guard() for spin locks

      https://git.kernel.org/broonie/sound/c/c5f9ddc7d2ec
[2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access
      https://git.kernel.org/broonie/sound/c/f003dfe0842a
[3/3] ASoC: sunxi: sun4i-spdif: Reorder clock enable sequence
      https://git.kernel.org/broonie/sound/c/a4a9384fdf2c

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