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

[Lu, Brent]

> > 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
> 
> 

Will fix it.

> > +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.
> 

This function call prevents the clk_set_rate() from being called when clock is 
enabled. It's removed during the upstream review and use the flag 
CLK_SET_RATE_GATE instead. Will fix it.

https://patchwork.kernel.org/patch/10070383/
https://patchwork.kernel.org/patch/10133179/

> > +
> > +		/* 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?
> 

Will fix it.

> > +
> > +		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?

Actually I don't know the purpose to use ignore_pmdown_time flag in the first
place but I think they are not related since this patch is to turn on ssp clocks earlier
while the ignore_pmdown_time flag is used to delay the DAPM power down for
playback stream in the soc_pcm_close(). Just my two cents and thank you for the
review.


Regards,
Brent