Re: [PATCH v3 2/7] ASoC: codec: pcm1681: Add optional SCK clock and runtime PM support

[Wang, Sen]

On 10/9/2026 8:12 AM, Mohammad Rafi Shaik wrote:
> The PCM1681 requires its SCK system clock to be running before register
> access. On platforms where SCK is provided by a gateable clock, register
> access may fail when the clock is disabled.
>   
> Add support for an optional "sck" clock and enable it before accessing the
> device. After enabling SCK, wait for the required 65536 system clock cycles
> to allow the device to complete its internal reset sequence.
>   
> Use runtime PM to manage SCK instead of keeping it enabled for the lifetime
> of the device. Disable the clock during runtime suspend and restore it on
> runtime resume. Use autosuspend to avoid unnecessary clock toggling between
> closely spaced accesses.
>   
> Since the device register state may be lost when SCK is disabled, enable
> the regmap cache. Switch regmap to cache-only mode and mark the cache
> dirty on runtime suspend, then synchronize the cached register state after
> SCK is restored on runtime resume. Mark the zero-detect status register
> volatile since it is updated by hardware.
>   
> Drop idle_bias_on so that the component can reach SND_SOC_BIAS_OFF and
> runtime suspend can gate SCK.
>
> Assisted-by: LLM
> Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@xxxxxxxxxxxxxxxx>

Hi Mohammd, thanks for your patches.

> ---
>   sound/soc/codecs/pcm1681.c | 137 +++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 131 insertions(+), 6 deletions(-)
>
> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c
> index 60fdbe5c4..a1ccf1edf 100644
> --- a/sound/soc/codecs/pcm1681.c
> +++ b/sound/soc/codecs/pcm1681.c
>   
>   
> -	return devm_snd_soc_register_component(&client->dev,
> -		&soc_component_dev_pcm1681,
> -		&pcm1681_dai, 1);
> +	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 */
> +	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);

For the sake of consistency let's add mark_last_busy first, so that 
first suspend won't skip the delay.

> +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);
> +}
> +

Sashiko reported something valid here so please take a look at this.

> +static int pcm1681_runtime_resume(struct device *dev)
> +{
> +	struct pcm1681_private *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->sck);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable sck: %d\n", ret);
> +		return ret;
> +	}
> +
> +	pcm1681_sck_settle(priv);
> +
> +	regcache_cache_only(priv->regmap, false);
> +	ret = regcache_sync(priv->regmap);

I'm wondering if this would cause regression on existing boards without 
SCK clock, since now regcache_sync writes from runtime resume at stream 
open, before machine/CPU DAI startup or machine hw_params that may 
enable SCK. Rather than writing in codec hw_params as before. Did/can 
you try running your board with runtime PM but without the SCK and see 
what it does?

The SCK dt might not be optional afterall with runtime PM 
enabled.Overall I think it's a cramped patch with runtime PM, regcache 
enable and SCK clock support which are all major features and shall be 
bisected individually, therefore can you decouple and have separate 
patches instead?

Best,
Sen Wang