Re: [alsa-devel] [PATCH] ASoC: Intel: eve: Enable mclk and ssp sclk early

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> rt5663 and rt5514 needs mclk/sclk early to synchronize its internal
> clocks.

There are a couple of differences with similar code used in 
kbl_rt5663_max98927, see below


> diff --git a/sound/soc/intel/boards/Kconfig b/sound/soc/intel/boards/Kconfig
> index 5c27f7a..d5f167e 100644
> --- a/sound/soc/intel/boards/Kconfig
> +++ b/sound/soc/intel/boards/Kconfig
> @@ -315,6 +315,7 @@ config SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH
>   	depends on I2C && ACPI
>   	depends on MFD_INTEL_LPSS || COMPILE_TEST
>           depends on SPI
> +	select SND_SOC_INTEL_SKYLAKE_SSP_CLK

It would be nicer to follow the same order as for kbl_rt5663_max98927, 
with this SKYLAKE_SSP_CLK added last after HDAC_HDMI


> +static int platform_clock_control(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *k, int  event)
> +{
> +	struct snd_soc_dapm_context *dapm = w->dapm;
> +	struct snd_soc_card *card = dapm->card;
> +	struct kbl_codec_private *priv = snd_soc_card_get_drvdata(card);
> +	int ret = 0;
> +
> +	/*
> +	 * MCLK/SCLK need to be ON early for a successful synchronization of
> +	 * codec internal clock. And the clocks are turned off during
> +	 * POST_PMD after the stream is stopped.
> +	 */
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		if (__clk_is_enabled(priv->mclk))
> +			return 0;

Is this if() test needed? it's not part of the code for 
kbl_rt5663_max98927, despite all the comments and code structure being 
identical.

> +
> +		/* Enable MCLK */
> +		ret = clk_set_rate(priv->mclk, 24000000);
> +		if (ret < 0) {
> +			dev_err(card->dev, "Can't set rate for mclk, err: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = clk_prepare_enable(priv->mclk);
> +		if (ret < 0) {
> +			dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
> +			return ret;
> +		}
> +
> +		/* Enable SCLK */
> +		ret = clk_set_rate(priv->sclk, 3072000);
> +		if (ret < 0) {
> +			dev_err(card->dev, "Can't set rate for sclk, err: %d\n",
> +				ret);
> +			clk_disable_unprepare(priv->mclk);
> +			return ret;
> +		}
> +
> +		ret = clk_prepare_enable(priv->sclk);
> +		if (ret < 0) {
> +			dev_err(card->dev, "Can't enable sclk, err: %d\n", ret);
> +			clk_disable_unprepare(priv->mclk);
> +		}
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		if (!__clk_is_enabled(priv->mclk))
> +			return 0;

same here, is this if() test needed? If yes, isn't it needed in 
kbl_rt5663_max98927?

> +
> +		clk_disable_unprepare(priv->mclk);
> +		clk_disable_unprepare(priv->sclk);
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +

While I am at it, this machine driver uses .ignore_pmdown_time, which is 
typically used to avoid clock issues. Now that you have an explicit 
control on clocks, is .ignore_pmdown_time actually required?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel