Re: [PATCH v1 3/5] ASoC: codec: pcm1681: Enable system clock before regmap access

[Shawn Guo <shengchao.guo@oss.qualcomm.com>]

On Mon, Sep 07, 2026 at 11:39:44PM +0530, Mohammad Rafi Shaik wrote:
> The PCM1681 DAC requires the SCK (system clock) input to be
> present for proper device operation. On platforms where SCK
> is provided by a controllable clock source, register accesses
> over I2C may fail when the clock is not enabled.
> 
> Add optional clock support to the PCM1681 driver by acquiring
> the "sck" clock, enabling it during probe, and registering a
> managed cleanup action to disable it during device removal or
> probe failure.
> 
> This allows platforms to model the PCM1681 system clock through
> the common clock framework and ensures the device is operational
> before regmap initialization and register accesses occur.
> 
> Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
> ---
>  sound/soc/codecs/pcm1681.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/sound/soc/codecs/pcm1681.c b/sound/soc/codecs/pcm1681.c
> index 60fdbe5c4e05..cfe549176d14 100644
> --- a/sound/soc/codecs/pcm1681.c
> +++ b/sound/soc/codecs/pcm1681.c
> @@ -12,6 +12,7 @@
>  #include <linux/i2c.h>
>  #include <linux/regmap.h>
>  #include <linux/of.h>
> +#include <linux/clk.h>
>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>
> @@ -74,6 +75,7 @@ struct pcm1681_private {
>  	unsigned int deemph;
>  	/* Current rate for deemphasis control */
>  	unsigned int rate;
> +	struct clk *sclk;
>  };
>  
>  static const int pcm1681_deemph[] = { 44100, 48000, 32000 };
> @@ -311,6 +313,22 @@ static int pcm1681_i2c_probe(struct i2c_client *client)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	priv->sclk = devm_clk_get_optional(&client->dev, "sck");
> +	if (IS_ERR(priv->sclk))
> +		return dev_err_probe(&client->dev, PTR_ERR(priv->sclk),
> +				     "Failed to get sck\n");
> +
> +	ret = clk_prepare_enable(priv->sclk);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "Failed to enable sck\n");
> +
> +	ret = devm_add_action_or_reset(&client->dev,
> +				       (void (*)(void *))clk_disable_unprepare,
> +				       priv->sclk);
> +	if (ret)
> +		return ret;
> +

All three calls can collapse into one?

	priv->sclk = devm_clk_get_optional_enabled(&client->dev, "sck");
	if (IS_ERR(priv->sclk))
		return dev_err_probe(&client->dev, PTR_ERR(priv->sclk), "Failed to get sck\n");

Also, priv->sclk is never used after probe, so drop the struct member
and use a local.

Shawn

>  	priv->regmap = devm_regmap_init_i2c(client, &pcm1681_regmap);
>  	if (IS_ERR(priv->regmap)) {
>  		ret = PTR_ERR(priv->regmap);
> 
> -- 
> 2.34.1
> 
>