[PATCH] ASoC: codec: pcm1681: Acquire sck when bound to a card

[PATCH] ASoC: codec: pcm1681: Acquire sck when bound to a card

[Shawn Guo]

The sck clock is taken once at i2c probe and held for the lifetime of
the device. On Qualcomm boards it is an LPASS clock exported by the
q6prm clock controller, which lives on the DSP and is unregistered and
registered anew whenever the ADSP is stopped and restarted. fw_devlink
cannot tie pcm1681 to that provider, since the provider node only gets
a device once the DSP is up, so it falls back to the remoteproc device,
which stays bound across a DSP restart. pcm1681 is therefore never
unbound and keeps a clk handle to the dead provider.

After an ADSP restart the first runtime resume fails with:

  pcm1681 0-004c: Failed to enable sck: -6

and there is no playback through the DAC. Even with a fresh handle the
DAC stays silent: the new provider registers its clocks at their
default rate (19.2 MHz), while assigned-clock-rates (12.288 MHz) was
only applied by the i2c core at probe time.

The sound card, however, is torn down and re-created on a DSP restart.
Move sck handling into the component probe/remove callbacks: get the
clock, re-apply the assigned clock rates and enable it each time the
component is bound, and release it on removal. The i2c probe now only
sets up supplies and the regmap, keeping the cache in cache-only mode
until the clock is running, and the cache is synced once sck is up.
Runtime PM is enabled and disabled together with the clock.

Fixes: 8987e9a24f45 ("ASoC: codec: pcm1681: Add optional SCK clock and runtime PM support")
Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>
---
 sound/soc/codecs/pcm1681.c | 122 ++++++++++++++++++++++++++-----------
 1 file changed, 88 insertions(+), 34 deletions(-)

diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c
index 6002da3a5666..2cc34561aa83 100644
--- a/sound/soc/codecs/pcm1681.c
+++ b/sound/soc/codecs/pcm1681.c
@@ -13,6 +13,7 @@
 #include <linux/regmap.h>
 #include <linux/of.h>
 #include <linux/clk.h>
+#include <linux/clk/clk-conf.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <sound/pcm.h>
@@ -321,7 +322,87 @@ static const struct regmap_config pcm1681_regmap = {
 	.cache_type		= REGCACHE_MAPLE,
 };
 
+/*
+ * sck may come from a DSP-hosted clock controller that is torn down and
+ * re-registered when the DSP restarts, which also rebinds the card. A clk
+ * handle kept across that would point at the dead provider, so only hold
+ * sck while the component is bound to a card.
+ */
+static int pcm1681_component_probe(struct snd_soc_component *component)
+{
+	struct pcm1681_private *priv = snd_soc_component_get_drvdata(component);
+	struct device *dev = component->dev;
+	int ret;
+
+	priv->sck = clk_get_optional(dev, "sck");
+	if (IS_ERR(priv->sck)) {
+		ret = PTR_ERR(priv->sck);
+		priv->sck = NULL;
+		return dev_err_probe(dev, ret, "Failed to get sck\n");
+	}
+
+	/*
+	 * A re-registered provider comes back at its default rate, and the
+	 * driver core applied assigned-clock-rates only once, at i2c probe.
+	 */
+	ret = of_clk_set_defaults(dev->of_node, false);
+	if (ret) {
+		dev_err(dev, "Failed to set assigned clock rates: %d\n", ret);
+		goto err_put;
+	}
+
+	ret = clk_prepare_enable(priv->sck);
+	if (ret) {
+		dev_err(dev, "Failed to enable sck: %d\n", ret);
+		goto err_put;
+	}
+
+	pcm1681_sck_settle(priv);
+
+	/* The DAC lost its state if sck went away while unbound */
+	regcache_cache_only(priv->regmap, false);
+	regcache_mark_dirty(priv->regmap);
+	ret = regcache_sync(priv->regmap);
+	if (ret) {
+		dev_err(dev, "Failed to sync regcache: %d\n", ret);
+		goto err_disable;
+	}
+
+	/* The clock is on, so hand the now-active device over to runtime PM */
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_idle(dev);
+
+	return 0;
+
+err_disable:
+	clk_disable_unprepare(priv->sck);
+err_put:
+	clk_put(priv->sck);
+	priv->sck = NULL;
+	return ret;
+}
+
+static void pcm1681_component_remove(struct snd_soc_component *component)
+{
+	struct pcm1681_private *priv = snd_soc_component_get_drvdata(component);
+	struct device *dev = component->dev;
+
+	pm_runtime_disable(dev);
+	/* Runtime PM may already have gated the clock */
+	if (!pm_runtime_status_suspended(dev)) {
+		regcache_cache_only(priv->regmap, true);
+		clk_disable_unprepare(priv->sck);
+	}
+	pm_runtime_set_suspended(dev);
+
+	clk_put(priv->sck);
+	priv->sck = NULL;
+}
+
 static const struct snd_soc_component_driver soc_component_dev_pcm1681 = {
+	.probe			= pcm1681_component_probe,
+	.remove			= pcm1681_component_remove,
 	.controls		= pcm1681_controls,
 	.num_controls		= ARRAY_SIZE(pcm1681_controls),
 	.dapm_widgets		= pcm1681_dapm_widgets,
@@ -359,11 +440,6 @@ static int pcm1681_i2c_probe(struct i2c_client *client)
 		return dev_err_probe(dev, ret,
 				     "Failed to enable supplies\n");
 
-	priv->sck = devm_clk_get_optional(dev, "sck");
-	if (IS_ERR(priv->sck))
-		return dev_err_probe(dev, PTR_ERR(priv->sck),
-				     "Failed to get sck\n");
-
 	priv->regmap = devm_regmap_init_i2c(client, &pcm1681_regmap);
 	if (IS_ERR(priv->regmap)) {
 		ret = PTR_ERR(priv->regmap);
@@ -371,53 +447,31 @@ static int pcm1681_i2c_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	/* No sck until the component is bound, serve access from the cache */
+	regcache_cache_only(priv->regmap, true);
+
 	/* Must be set before runtime PM is enabled, the callbacks use it */
 	i2c_set_clientdata(client, priv);
 
-	ret = clk_prepare_enable(priv->sck);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to enable sck\n");
-
-	pcm1681_sck_settle(priv);
-
-	/* The clock is on, so hand the now-active device over to runtime PM */
+	/* Runtime PM is enabled from component probe, once sck is running */
 	pm_runtime_set_autosuspend_delay(dev, 100);
 	pm_runtime_use_autosuspend(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-	pm_runtime_idle(dev);
 
 	ret = devm_snd_soc_register_component(dev,
 					      &soc_component_dev_pcm1681,
 					      &pcm1681_dai, 1);
 	if (ret) {
 		dev_err(dev, "Failed to register component: %d\n", ret);
-		goto err_pm;
+		pm_runtime_dont_use_autosuspend(dev);
+		return ret;
 	}
 
 	return 0;
-
-err_pm:
-	pm_runtime_dont_use_autosuspend(dev);
-	pm_runtime_disable(dev);
-	if (!pm_runtime_status_suspended(dev))
-		clk_disable_unprepare(priv->sck);
-	pm_runtime_set_suspended(dev);
-
-	return ret;
 }
 
 static void pcm1681_i2c_remove(struct i2c_client *client)
 {
-	struct pcm1681_private *priv = i2c_get_clientdata(client);
-	struct device *dev = &client->dev;
-
-	pm_runtime_dont_use_autosuspend(dev);
-	pm_runtime_disable(dev);
-	/* Runtime PM may already have gated the clock */
-	if (!pm_runtime_status_suspended(dev))
-		clk_disable_unprepare(priv->sck);
-	pm_runtime_set_suspended(dev);
+	pm_runtime_dont_use_autosuspend(&client->dev);
 }
 
 static int pcm1681_runtime_suspend(struct device *dev)
-- 
2.43.0

Re: [PATCH] ASoC: codec: pcm1681: Acquire sck when bound to a card

[Mohammad Rafi Shaik]

On 10/21/2026 8:42 AM, Shawn Guo wrote:
> The sck clock is taken once at i2c probe and held for the lifetime of
> the device. On Qualcomm boards it is an LPASS clock exported by the
> q6prm clock controller, which lives on the DSP and is unregistered and
> registered anew whenever the ADSP is stopped and restarted. fw_devlink
> cannot tie pcm1681 to that provider, since the provider node only gets
> a device once the DSP is up, so it falls back to the remoteproc device,
> which stays bound across a DSP restart. pcm1681 is therefore never
> unbound and keeps a clk handle to the dead provider.
> 
> After an ADSP restart the first runtime resume fails with:
> 
>    pcm1681 0-004c: Failed to enable sck: -6
> 
> and there is no playback through the DAC. Even with a fresh handle the
> DAC stays silent: the new provider registers its clocks at their
> default rate (19.2 MHz), while assigned-clock-rates (12.288 MHz) was
> only applied by the i2c core at probe time.
> 
> The sound card, however, is torn down and re-created on a DSP restart.
> Move sck handling into the component probe/remove callbacks: get the
> clock, re-apply the assigned clock rates and enable it each time the
> component is bound, and release it on removal. The i2c probe now only
> sets up supplies and the regmap, keeping the cache in cache-only mode
> until the clock is running, and the cache is synced once sck is up.
> Runtime PM is enabled and disabled together with the clock.
> 
> Fixes: 8987e9a24f45 ("ASoC: codec: pcm1681: Add optional SCK clock and runtime PM support")
> Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>

LGTM,

Reviewed-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

> ---
>   sound/soc/codecs/pcm1681.c | 122 ++++++++++++++++++++++++++-----------
>   1 file changed, 88 insertions(+), 34 deletions(-)
> 
> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c
> index 6002da3a5666..2cc34561aa83 100644
> --- a/sound/soc/codecs/pcm1681.c
> +++ b/sound/soc/codecs/pcm1681.c

Re: [PATCH] ASoC: codec: pcm1681: Acquire sck when bound to a card

[Bartosz Golaszewski]

On Wed, Oct 21, 2026 at 11:12:28AM +0800, Shawn Guo wrote:
> The sck clock is taken once at i2c probe and held for the lifetime of
> the device. On Qualcomm boards it is an LPASS clock exported by the
> q6prm clock controller, which lives on the DSP and is unregistered and
> registered anew whenever the ADSP is stopped and restarted. fw_devlink
> cannot tie pcm1681 to that provider, since the provider node only gets
> a device once the DSP is up, so it falls back to the remoteproc device,
> which stays bound across a DSP restart. pcm1681 is therefore never
> unbound and keeps a clk handle to the dead provider.
> 
> After an ADSP restart the first runtime resume fails with:
> 
>   pcm1681 0-004c: Failed to enable sck: -6
> 
> and there is no playback through the DAC. Even with a fresh handle the
> DAC stays silent: the new provider registers its clocks at their
> default rate (19.2 MHz), while assigned-clock-rates (12.288 MHz) was
> only applied by the i2c core at probe time.
> 
> The sound card, however, is torn down and re-created on a DSP restart.
> Move sck handling into the component probe/remove callbacks: get the
> clock, re-apply the assigned clock rates and enable it each time the
> component is bound, and release it on removal. The i2c probe now only
> sets up supplies and the regmap, keeping the cache in cache-only mode
> until the clock is running, and the cache is synced once sck is up.
> Runtime PM is enabled and disabled together with the clock.
> 
> Fixes: 8987e9a24f45 ("ASoC: codec: pcm1681: Add optional SCK clock and runtime PM support")
> Signed-off-by: Shawn Guo <shengchao.guo@oss.qualcomm.com>
> ---
 
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH] ASoC: codec: pcm1681: Acquire sck when bound to a card

[Mark Brown]

> The sck clock is taken once at i2c probe and held for the lifetime of
> the device. On Qualcomm boards it is an LPASS clock exported by the
> q6prm clock controller, which lives on the DSP and is unregistered and
> registered anew whenever the ADSP is stopped and restarted. fw_devlink
> cannot tie pcm1681 to that provider, since the provider node only gets
> a device once the DSP is up, so it falls back to the remoteproc device,
> which stays bound across a DSP restart. pcm1681 is therefore never
> unbound and keeps a clk handle to the dead provider.

This seems like something that ought to be dealt with on the Qualcomm
or fw_devlink side TBH, registering the clocks and returning -EBUSY on
attempts to use them would be less disruptive to other components in the
system.  I don't understand what fw_devlink would be doing with
reparenting clocks...

> +static int pcm1681_component_probe(struct snd_soc_component *component)
> +{
> +	struct pcm1681_private *priv = snd_soc_component_get_drvdata(component);
> +	struct device *dev = component->dev;
> +	int ret;
> +
> +	priv->sck = clk_get_optional(dev, "sck");
> +	if (IS_ERR(priv->sck)) {
> +		ret = PTR_ERR(priv->sck);
> +		priv->sck = NULL;
> +		return dev_err_probe(dev, ret, "Failed to get sck\n");
> +	}

This is the oppositie of the standard advice for drivers, and we don't
deal well with probe deferral from component probes - in particular,
nothing will trigger a rebind of the card if we don't probe a new
component and that's not going to happen if the CODEC already bound and
isn't waiting for the clock.

Also dev_err_probe() will only DTRT from device model probe.

-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH] ASoC: codec: pcm1681: Acquire sck when bound to a card

[Shawn Guo]

+ Srini

On Thu, Nov 05, 2026 at 05:42:03PM +0000, Mark Brown wrote:
> > The sck clock is taken once at i2c probe and held for the lifetime of
> > the device. On Qualcomm boards it is an LPASS clock exported by the
> > q6prm clock controller, which lives on the DSP and is unregistered and
> > registered anew whenever the ADSP is stopped and restarted. fw_devlink
> > cannot tie pcm1681 to that provider, since the provider node only gets
> > a device once the DSP is up, so it falls back to the remoteproc device,
> > which stays bound across a DSP restart. pcm1681 is therefore never
> > unbound and keeps a clk handle to the dead provider.
> 
> This seems like something that ought to be dealt with on the Qualcomm
> or fw_devlink side TBH, registering the clocks and returning -EBUSY on
> attempts to use them would be less disruptive to other components in the
> system.  I don't understand what fw_devlink would be doing with
> reparenting clocks...

Thanks for the review, Mark! I agree this belongs in the Qualcomm clock
provider, not in pcm1681, so I'll drop this patch.

Sorry, my wording about fw_devlink was misleading. No clock gets
reparented. My point was only that pcm1681 is never unbound when the DSP
restarts. The q6prm clock-controller device only exists while the DSP is
up. So fw_devlink links pcm1681 to the remoteproc device instead, which
stays bound across the restart.

Today the clocks can't stay registered across a DSP restart. When the DSP
stops, the GPR bus unregisters the q6prm device and its clock-controller
child, and that unregisters the clocks. The clk core then switches them
to clk_nodrv_ops, which return -ENXIO. That's where the "Failed to enable
sck: -6" in my report comes from. The consumer's struct clk handle can't
be reconnected after that.

So I'd like to change q6prm along the lines you suggested:

- Keep the clocks registered, on a device that is not removed when
  the DSP stops.
- While the DSP is down, make prepare/enable return -EBUSY.
- When the DSP comes back, re-send the requests for clocks that are still
  enabled, at their last set rate. This also fixes the rate being reset
  to the 19.2 MHz default.

The part I'm unsure about is which device should own the clocks:

a) A platform device that q6prm creates on its first probe and keeps
   until the module is unloaded. This needs no DT change.

b) The ADSP remoteproc device, which already survives restarts.
   It's cleaner, but probably needs a binding change, since the clock
   controller node is a child of qcom,q6prm today.

Srini, Mark, do you prefer a), b), or something else?

Shawn

> 
> > +static int pcm1681_component_probe(struct snd_soc_component *component)
> > +{
> > +	struct pcm1681_private *priv = snd_soc_component_get_drvdata(component);
> > +	struct device *dev = component->dev;
> > +	int ret;
> > +
> > +	priv->sck = clk_get_optional(dev, "sck");
> > +	if (IS_ERR(priv->sck)) {
> > +		ret = PTR_ERR(priv->sck);
> > +		priv->sck = NULL;
> > +		return dev_err_probe(dev, ret, "Failed to get sck\n");
> > +	}
> 
> This is the oppositie of the standard advice for drivers, and we don't
> deal well with probe deferral from component probes - in particular,
> nothing will trigger a rebind of the card if we don't probe a new
> component and that's not going to happen if the CODEC already bound and
> isn't waiting for the clock.
> 
> Also dev_err_probe() will only DTRT from device model probe.
> 
> -- 
> Mark Brown <broonie@kernel.org>