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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ASoC: devres lifecycle ordering bug causing I2C access after clock is gated
--

commit c3c4668080e08de3f73de6e53cea7fdcb695945c
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: codec: pcm1681: Add optional SCK clock and runtime PM support

The PCM1681 requires its SCK system clock to be running before register
access. This commit adds support for an optional sck clock and uses
runtime PM to manage it instead of keeping it enabled permanently.

> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c
> index 60fdbe5c4e054..a1ccf1edf4819 100644
> --- a/sound/soc/codecs/pcm1681.c
> +++ b/sound/soc/codecs/pcm1681.c
[ ... ]
> @@ -304,34 +335,128 @@ MODULE_DEVICE_TABLE(i2c, pcm1681_i2c_id);
>  
>  static int pcm1681_i2c_probe(struct i2c_client *client)
>  {
[ ... ]
> +	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;
> +	}
> +
> +	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);

[Severity: High]
Does this devres lifecycle ordering introduce a regression where I2C bus
timeouts occur during teardown?

Since the component is registered using devm_snd_soc_register_component() in
pcm1681_i2c_probe(), it will be unregistered during the devres phase, which
runs after pcm1681_i2c_remove() completes.

By manually disabling runtime PM and gating the hardware SCK clock here, the
clock is turned off while the ALSA component is still fully registered. The
subsequent devres unregistration phase will invoke ASoC DAPM teardown
sequences, which perform regmap writes to power down widgets and lower the
bias level.

Because regcache_cache_only() is not enabled here in pcm1681_i2c_remove(),
wouldn't these regmap accesses attempt physical I2C transactions to the
device with a gated SCK clock, resulting in I2C timeouts or hangs?

Could this be fixed by using snd_soc_register_component() and unregistering
it explicitly at the beginning of remove(), or by using
devm_add_action_or_reset() to manage the PM and clock teardown after the
component is unregistered?

> +	pm_runtime_set_suspended(dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-nord-asoc-driver-support-v2-v2-0-0b6f7e94060d@oss.qualcomm.com?part=2