> > 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
