[PATCH v2 0/3] ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements

[PATCH v2 0/3] ASoC: sunxi: sun4i-spdif: Cleanup and runtime PM improvements

[phucduc.bui]

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

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/20260513105003.81880-1-phucduc.bui@gmail.com/

Change in v2: 
 - Switched from using guard() to scoped_guard()
 - Added runtime PM handling for kcontrol register accesses.
	
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(-)

-- 
2.43.0

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

[phucduc.bui]

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

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

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Change in v2: 
 - Switched from using guard() to scoped_guard()

 sound/soc/sunxi/sun4i-spdif.c | 62 ++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
index c2ec19437cd7..89eccc83a130 100644
--- a/sound/soc/sunxi/sun4i-spdif.c
+++ b/sound/soc/sunxi/sun4i-spdif.c
@@ -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;
 }
-- 
2.43.0

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

[phucduc.bui]

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

Accessing registers while the device is runtime-suspended
may lead to invalid hardware accesses on systems where the
APB bus clock is gated during runtime suspend.

Ensure the device is resumed before accessing registers.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/sunxi/sun4i-spdif.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
index 89eccc83a130..f54eb14c9ed8 100644
--- a/sound/soc/sunxi/sun4i-spdif.c
+++ b/sound/soc/sunxi/sun4i-spdif.c
@@ -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;
 }
 
-- 
2.43.0

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

[phucduc.bui]

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

Enable the APB bus clock before the SPDIF module clock
during runtime resume, as register accesses depend on the
bus clock being enabled first.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/sunxi/sun4i-spdif.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
index f54eb14c9ed8..102db1a2afbb 100644
--- a/sound/soc/sunxi/sun4i-spdif.c
+++ b/sound/soc/sunxi/sun4i-spdif.c
@@ -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;
 }
-- 
2.43.0

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Chen-Yu Tsai]

On Fri, May 22, 2026 at 12:54 PM <phucduc.bui@gmail.com> wrote:
>
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Accessing registers while the device is runtime-suspended
> may lead to invalid hardware accesses on systems where the
> APB bus clock is gated during runtime suspend.

Did you actually reproduce the issue, or did you add the patch simply
because Sashiko mentioned it?

On sunxi, either it will hang the system because the bus transaction
got ignored, or it won't as something else enabled the clock.

And when you do add patches due to Sashiko raising an issue, please
do mention it in the commit message.

> Ensure the device is resumed before accessing registers.
>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-spdif.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index 89eccc83a130..f54eb14c9ed8 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -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;
>  }
>
> --
> 2.43.0
>

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

[Chen-Yu Tsai]

On Fri, May 22, 2026 at 12:54 PM <phucduc.bui@gmail.com> wrote:
>
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> Enable the APB bus clock before the SPDIF module clock
> during runtime resume, as register accesses depend on the
> bus clock being enabled first.

That does not even matter here. Access will only happen once the runtime
PM callbacks return.

> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-spdif.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index f54eb14c9ed8..102db1a2afbb 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -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;
>  }
> --
> 2.43.0
>

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

[Bui Duc Phuc]

Hi Chen-yu,

Thanks for your feedback

On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > Enable the APB bus clock before the SPDIF module clock
> > during runtime resume, as register accesses depend on the
> > bus clock being enabled first.
>
> That does not even matter here. Access will only happen once the runtime
> PM callbacks return.
>

I understand your point that ⁠sun4i-spdif⁠ doesn't immediately access
registers within the current ⁠runtime_resume⁠ path, so the order might
not trigger a failure right now.

However, if we look at the peer driver for the same Sunxi SoC family,
⁠sun4i-i2s.c⁠:
Links:
https://elixir.bootlin.com/linux/v7.0-rc5/source/sound/soc/sunxi/sun4i-i2s.c#L1296
In ⁠sun4i_i2s_runtime_resume()⁠, the sequence is strictly enforced as:

1. Enable bus clock
2. Access and restore/sync I2S registers
3. Enable module clock

Since both IP blocks belong to the same Sunxi platform and share similar
bus/module clock relationships, shouldn't we maintain architectural
consistency across these drivers?

Enforcing the "bus clock before module clock" order keeps the dependency
ordering aligned with the actual hardware roles, where the bus clock is
required for register access while the module clock drives the functional
audio path.

Wouldn't keeping this order also make the runtime PM behavior more
consistent and easier to follow across the Sunxi audio drivers?

Best Regards,
Phuc

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Bui Duc Phuc]

Hi Chen-Yu,

On Sat, May 23, 2026 at 2:19 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> And when you do add patches due to Sashiko raising an issue, please
> do mention it in the commit message.
>

As mentioned in the v1 discussion , this issue was originally reported
by Sashiko.
I'll add the Reported-by tag in the next revision.
v1 links:
https://lore.kernel.org/all/20260513105003.81880-1-phucduc.bui@gmail.com/

> Did you actually reproduce the issue, or did you add the patch simply
> because Sashiko mentioned it?
>
Since I lack Sunxi hardware, I couldn't reproduce it or perform runtime testing.
But I did compile-test the patch.
The patch aims to fix unsafe register accesses that occur before ensuring the
device is runtime-resumed.

> On sunxi, either it will hang the system because the bus transaction
> got ignored, or it won't as something else enabled the clock.
>

If Sunxi's PM design already guarantees safe access here,
feel free to reject the patch.

Best Regards,
Phuc

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Chen-Yu Tsai]

On Sat, May 23, 2026 at 4:55 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
>
> Hi Chen-Yu,
>
> On Sat, May 23, 2026 at 2:19 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > And when you do add patches due to Sashiko raising an issue, please
> > do mention it in the commit message.
> >
>
> As mentioned in the v1 discussion , this issue was originally reported
> by Sashiko.
> I'll add the Reported-by tag in the next revision.
> v1 links:
> https://lore.kernel.org/all/20260513105003.81880-1-phucduc.bui@gmail.com/
>
> > Did you actually reproduce the issue, or did you add the patch simply
> > because Sashiko mentioned it?
> >
> Since I lack Sunxi hardware, I couldn't reproduce it or perform runtime testing.
> But I did compile-test the patch.
> The patch aims to fix unsafe register accesses that occur before ensuring the
> device is runtime-resumed.

When you submit a patch, it is expected that you already tested it.
If you only compile tested it, please remember to say so in the
footer (or mark the patch as RFT) so that others can test for you
and the maintainer knows the status.

And if possible, provide a scheme to test it.

> > On sunxi, either it will hang the system because the bus transaction
> > got ignored, or it won't as something else enabled the clock.
> >
>
> If Sunxi's PM design already guarantees safe access here,
> feel free to reject the patch.

I can't say that it does. But since the only control that SPDIF gives
is the IEC958 status, and that doesn't appear in the standard mixer apps,
it's unlikely that a _user_ will trigger it. Plus the control was added
after the basic structure of the driver was done, so there is definitely
some possibility of a crash.

But what you wrote in the commit message doesn't match the actual hardware
behavior, like I wrote.


ChenYu

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

[Chen-Yu Tsai]

On Sat, May 23, 2026 at 3:11 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
>
> Hi Chen-yu,
>
> Thanks for your feedback
>
> On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > Enable the APB bus clock before the SPDIF module clock
> > > during runtime resume, as register accesses depend on the
> > > bus clock being enabled first.
> >
> > That does not even matter here. Access will only happen once the runtime
> > PM callbacks return.
> >
>
> I understand your point that ⁠sun4i-spdif⁠ doesn't immediately access
> registers within the current ⁠runtime_resume⁠ path, so the order might
> not trigger a failure right now.
>
> However, if we look at the peer driver for the same Sunxi SoC family,
> ⁠sun4i-i2s.c⁠:
> Links:
> https://elixir.bootlin.com/linux/v7.0-rc5/source/sound/soc/sunxi/sun4i-i2s.c#L1296
> In ⁠sun4i_i2s_runtime_resume()⁠, the sequence is strictly enforced as:
>
> 1. Enable bus clock
> 2. Access and restore/sync I2S registers
> 3. Enable module clock
>
> Since both IP blocks belong to the same Sunxi platform and share similar
> bus/module clock relationships, shouldn't we maintain architectural
> consistency across these drivers?
>
> Enforcing the "bus clock before module clock" order keeps the dependency
> ordering aligned with the actual hardware roles, where the bus clock is
> required for register access while the module clock drives the functional
> audio path.
>
> Wouldn't keeping this order also make the runtime PM behavior more
> consistent and easier to follow across the Sunxi audio drivers?

This should be your primary motivation for the patch, i.e. what you
put in the commit message as the reason for this patch. What you
currently have doesn't make sense, as I already mentioned.

Some background though, sunxi is done mostly by volunteers, so we're
not overly concerned with rigidness or aligning different drivers,
unless they share a common library, such as all the clk or pinctrl
drivers.


ChenYu

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

[Chen-Yu Tsai]

On Sun, May 24, 2026 at 9:41 AM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> On Sat, May 23, 2026 at 3:11 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
> >
> > Hi Chen-yu,
> >
> > Thanks for your feedback
> >
> > On Sat, May 23, 2026 at 2:20 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > > Enable the APB bus clock before the SPDIF module clock
> > > > during runtime resume, as register accesses depend on the
> > > > bus clock being enabled first.
> > >
> > > That does not even matter here. Access will only happen once the runtime
> > > PM callbacks return.
> > >
> >
> > I understand your point that ⁠sun4i-spdif⁠ doesn't immediately access
> > registers within the current ⁠runtime_resume⁠ path, so the order might
> > not trigger a failure right now.
> >
> > However, if we look at the peer driver for the same Sunxi SoC family,
> > ⁠sun4i-i2s.c⁠:
> > Links:
> > https://elixir.bootlin.com/linux/v7.0-rc5/source/sound/soc/sunxi/sun4i-i2s.c#L1296
> > In ⁠sun4i_i2s_runtime_resume()⁠, the sequence is strictly enforced as:
> >
> > 1. Enable bus clock
> > 2. Access and restore/sync I2S registers
> > 3. Enable module clock
> >
> > Since both IP blocks belong to the same Sunxi platform and share similar
> > bus/module clock relationships, shouldn't we maintain architectural
> > consistency across these drivers?
> >
> > Enforcing the "bus clock before module clock" order keeps the dependency
> > ordering aligned with the actual hardware roles, where the bus clock is
> > required for register access while the module clock drives the functional
> > audio path.
> >
> > Wouldn't keeping this order also make the runtime PM behavior more
> > consistent and easier to follow across the Sunxi audio drivers?
>
> This should be your primary motivation for the patch, i.e. what you
> put in the commit message as the reason for this patch. What you
> currently have doesn't make sense, as I already mentioned.
>
> Some background though, sunxi is done mostly by volunteers, so we're
> not overly concerned with rigidness or aligning different drivers,
> unless they share a common library, such as all the clk or pinctrl
> drivers.

Oh, and you could also add that the resume order should (normally) be
the reverse of the suspend order.

ChenYu

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

[Bui Duc Phuc]

Hi Chen-yu,

> Oh, and you could also add that the resume order should (normally) be
> the reverse of the suspend order.

Thank you for the feedback and clarification.

I can certainly rewrite the commit message around the idea that the
resume ordering should normally mirror the suspend ordering.

However, I am still slightly unsure about which ordering should be
considered the preferred one in this case. If the reasoning is purely
based on suspend/resume symmetry, I wonder if changing the suspend
path itself to disable the APB clock before the SPDIF module clock
would be an option, which would then naturally lead to enabling the
SPDIF module clock first during resume.

From the hardware perspective, I originally assumed the APB clock acts
as the prerequisite for register access while the module clock mainly
drives the functional audio logic, which is why the "bus before module"
ordering initially seemed more intuitive to me.

So I would like to better understand which ordering is considered the
preferred model for the Sunxi audio blocks here: preserving the existing
suspend/resume symmetry, or enforcing the bus/module dependency ordering.

Thanks,
Phuc

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Bui Duc Phuc]

Hi Chenyu,

> When you submit a patch, it is expected that you already tested it.
> If you only compile tested it, please remember to say so in the
> footer (or mark the patch as RFT) so that others can test for you
> and the maintainer knows the status.
>
> And if possible, provide a scheme to test it.
>

Thanks for the guidance.
I’ll clearly mention the test status next time.

>
> I can't say that it does. But since the only control that SPDIF gives
> is the IEC958 status, and that doesn't appear in the standard mixer apps,
> it's unlikely that a _user_ will trigger it. Plus the control was added
> after the basic structure of the driver was done, so there is definitely
> some possibility of a crash.
>
> But what you wrote in the commit message doesn't match the actual hardware
> behavior, like I wrote.

Thanks for the clarification.
I'll update the commit message to something like:
"The kcontrols may access hardware registers while the
device is runtime-suspended.
Ensure the device is resumed before touching the registers."

Best Regards,
Phuc

Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[Bui Duc Phuc]

Hi ,

I did some additional investigation into the regmap core functions
currently used by this driver, such as _regmap_update_bits():

https://elixir.bootlin.com/linux/v7.1-rc5/source/drivers/base/regmap/regmap.c#L3249

and _regmap_read():
https://elixir.bootlin.com/linux/v7.1-rc5/source/drivers/base/regmap/regmap.c#L2822

as well as the current sun4i_spdif_regmap_config definition:
https://elixir.bootlin.com/linux/v7.1-rc5/source/sound/soc/sunxi/sun4i-spdif.c#L526

Based on the current regmap core implementation, I think it might be
cleaner to leverage the existing regmap suspend protection mechanism:

if (map->cache_only)
return -EBUSY;

This would inherently prevent direct hardware register access and help
avoid potential crashes during suspend. To support this properly, we would
likely also need to enable a basic cache type (e.g. REGCACHE_FLAT) in
sun4i_spdif_regmap_config, since the driver currently
does not define a .cache_type.

A patch in this direction would look roughly like this:

static int sun4i_spdif_runtime_suspend(struct device *dev)
{
  struct sun4i_spdif_dev *host = dev_get_drvdata(dev);

  clk_disable_unprepare(host->spdif_clk);
+regcache_cache_only(host->regmap, true);
 clk_disable_unprepare(host->apb_clk);

}

static int sun4i_spdif_runtime_resume(struct device *dev)
{
   struct sun4i_spdif_dev *host = dev_get_drvdata(dev);

   clk_prepare_enable(host->apb_clk);
+ regcache_cache_only(host->regmap, false);

   clk_prepare_enable(host->spdif_clk);
}

This approach would not only address the current kcontrol-related issue,
but could also provide a more generic safeguard against other runtime
suspend race conditions that we may not have considered yet.

However, one downside is that this solution depends on the current internal
behavior of the regmap core. In addition, if this driver starts using
.volatile_reg in the future, the current regmap implementation may no
longer provide sufficient protection during suspend.

For example, in the first branch of _regmap_update_bits(),
volatile register accesses bypass the cache_only check entirely and can
still hit the hardware bus directly, which could still lead to system crashes.

Because of that, the explicit pm_runtime resume handling proposed
in the current patch may still be the more robust solution in the long term.

Given our previous discussion about making the pm_runtime handling
more consistent between I2S and SPDIF:

https://lore.kernel.org/all/CAABR9nEFGOX5GnQ9qpJY-T-92dA_kDcVS+qBz1ior590G_x6gw@mail.gmail.com/

What are your thoughts on this direction?
Or would it be safer and simpler to keep the current patch as-is?

Best Regards,
Phuc