Re: [RFC PATCH 05/16] ASoC: Intel: sof-pcm512x: reconfigure sclk in hw_params if needed

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, Apr 09, 2020 at 02:58:30PM -0500, Pierre-Louis Bossart wrote:
> The SCLK is resumed by the codec driver. In case the rate specified in
> hw_params does not match the current configuration, disable, set the
> new rate and restart the clock.
> 
> There is no operation on hw_free, the codec suspend routine will
> disable/deprepare the clock.
> 
> Note that we don't change the DAI configuration when the DAC+ PRO is
> detected. All changes for the codec master mode are handled in the
> topology file (DAI configuration change and scheduling change)

...

> +	err = snd_interval_ratnum(hw_param_interval(params,
> +						    SNDRV_PCM_HW_PARAM_RATE),
> +				  1, &rats_no_pll, &num, &den);

> +	if (err >= 0 && den) {

Perhaps usual pattern, i.e.

	if (err < 0 || !den)
		return 0;
(so, above seems optional configuration)

	params...;
	return 0;

> +		params->rate_num = num;
> +		params->rate_den = den;
> +	}
> +
> +	return 0;

...

> +static int aif1_hw_params(struct snd_pcm_substream *substream,
> +			  struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct sof_card_private *ctx = snd_soc_card_get_drvdata(rtd->card);
> +	struct device *dev = rtd->card->dev;
> +	int current_rate;
> +	int sclk_rate;
> +	int channels;
> +	int width;
> +	int rate;
> +	int ret = 0;
> +

> +	if (ctx->is_dac_pro) {

	if (!...)
		return 0;

...and drop the redundant ret assignment above.

> +		rate = params_rate(params);
> +		channels = params_channels(params);
> +		width = snd_pcm_format_physical_width(params_format(params));
> +
> +		if (rate % 24000)
> +			sclk_rate = 22579200;
> +		else
> +			sclk_rate = 24576000;
> +
> +		current_rate = clk_get_rate(ctx->sclk);
> +		if (current_rate != sclk_rate) {
> +			/*
> +			 * The sclk clock is started and stopped by the codec
> +			 * resume/suspend functions. If the rate isn't correct,
> +			 * stop, set the new rate and restart the clock
> +			 */
> +
> +			dev_dbg(dev, "reconfiguring SCLK to rate %d\n",
> +				sclk_rate);
> +
> +			clk_disable_unprepare(ctx->sclk);
> +
> +			ret = clk_set_rate(ctx->sclk, sclk_rate);
> +			if (ret) {
> +				dev_err(dev, "Could not set SCLK rate %d\n",
> +					sclk_rate);
> +				return ret;
> +			}
> +
> +			ret = clk_prepare_enable(ctx->sclk);
> +			if (ret) {
> +				dev_err(dev, "Failed to enable SCLK: %d\n",
> +					ret);
> +				return ret;
> +			}
> +		}
> +
> +		ret = aif1_update_rate_den(substream, params);
> +		if (ret) {
> +			dev_err(dev, "Failed to update rate denominator: %d\n", ret);
> +			return ret;
> +		}

Do you still need below steps when current_rate == sclk_rate?

> +		ret = snd_soc_dai_set_bclk_ratio(rtd->codec_dai,
> +						 channels * width);
> +		if (ret) {
> +			dev_err(dev, "Failed to set bclk ratio : %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko