Re: [PATCH 3/3] ASoC: cs4271: Add support for the external mclk

[Alexander Sverdlin <alexander.sverdlin@gmail.com>]

Hello Herve,

On Thu, 2025-10-16 at 15:03 +0200, Herve Codina wrote:
> The mclk (master clock) of the cs4271 codec can be an input clock.
> 
> In this case the connected clock needs to be enabled outside of any
> audio stream. Indeed, this clock is needed for i2c communication.
> 
> Add support of this clock and enable it before the first i2c transfer.

what about suspend/resume, would it make sense to disable/enable the clock
there?

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  sound/soc/codecs/cs4271.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c
> index ff9c6628224c..481a2c20b7cf 100644
> --- a/sound/soc/codecs/cs4271.c
> +++ b/sound/soc/codecs/cs4271.c
> @@ -10,6 +10,7 @@
>   * DAPM support not implemented.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/delay.h>
> @@ -163,6 +164,7 @@ struct cs4271_private {
>  	/* enable soft reset workaround */
>  	bool				enable_soft_reset;
>  	struct regulator_bulk_data      supplies[ARRAY_SIZE(supply_names)];
> +	struct clk *clk;
>  };
>  
>  static const struct snd_soc_dapm_widget cs4271_dapm_widgets[] = {
> @@ -567,22 +569,36 @@ static int cs4271_component_probe(struct snd_soc_component *component)
>  		cs4271->enable_soft_reset = cs4271plat->enable_soft_reset;
>  	}
>  
> +	ret = clk_prepare_enable(cs4271->clk);
> +	if (ret) {
> +		dev_err(component->dev, "Failed to enable clk: %d\n", ret);
> +		goto err_disable_regulators;
> +	}
> +
> +	/*
> +	 * Be sure to have the clock and power-supplies stable before releasing
> +	 * the reset.
> +	 */

Would "if (cs4271->clk)" make sense for the fsleep() call?

> +	fsleep(1000);
> +
>  	/* Reset codec */
>  	cs4271_reset(component);
>  
>  	ret = regcache_sync(cs4271->regmap);
>  	if (ret < 0)
> -		return ret;
> +		goto err_force_reset;
>  
>  	ret = regmap_update_bits(cs4271->regmap, CS4271_MODE2,
>  				 CS4271_MODE2_PDN | CS4271_MODE2_CPEN,
>  				 CS4271_MODE2_PDN | CS4271_MODE2_CPEN);
>  	if (ret < 0)
> -		return ret;
> +		goto err_force_reset;
> +
>  	ret = regmap_update_bits(cs4271->regmap, CS4271_MODE2,
>  				 CS4271_MODE2_PDN, 0);
>  	if (ret < 0)
> -		return ret;
> +		goto err_force_reset;
> +
>  	/* Power-up sequence requires 85 uS */
>  	udelay(85);
>  
> @@ -592,6 +608,13 @@ static int cs4271_component_probe(struct snd_soc_component *component)
>  				   CS4271_MODE2_MUTECAEQUB);
>  
>  	return 0;
> +
> +err_force_reset:
> +	gpiod_set_value(cs4271->reset, 1);

Is the reset line assertion really required here?

> +	clk_disable_unprepare(cs4271->clk);
> +err_disable_regulators:
> +	regulator_bulk_disable(ARRAY_SIZE(cs4271->supplies), cs4271->supplies);

Would it make sense to fix the error path regarding regulators handling
in a separate patch (especially if you decide to extend suspend/resume)?

> +	return ret;
>  }
>  
>  static void cs4271_component_remove(struct snd_soc_component *component)
> @@ -603,6 +626,7 @@ static void cs4271_component_remove(struct snd_soc_component *component)
>  
>  	regcache_mark_dirty(cs4271->regmap);
>  	regulator_bulk_disable(ARRAY_SIZE(cs4271->supplies), cs4271->supplies);
> +	clk_disable_unprepare(cs4271->clk);
>  };
>  
>  static const struct snd_soc_component_driver soc_component_dev_cs4271 = {
> @@ -637,6 +661,10 @@ static int cs4271_common_probe(struct device *dev,
>  				     "error retrieving RESET GPIO\n");
>  	gpiod_set_consumer_name(cs4271->reset, "CS4271 Reset");
>  
> +	cs4271->clk = devm_clk_get_optional(dev, "mclk");
> +	if (IS_ERR(cs4271->clk))
> +		return dev_err_probe(dev, PTR_ERR(cs4271->clk), "Failed to get mclk\n");
> +
>  	for (i = 0; i < ARRAY_SIZE(supply_names); i++)
>  		cs4271->supplies[i].supply = supply_names[i];
>  

-- 
Alexander Sverdlin.