RE: [PATCH v2] ASoC: Intel: boards: eve: Fix DMIC records zero

["N, Harshapriya" <harshapriya.n@intel.com>]

> 
> >   	/*
> >   	 * MCLK/SCLK need to be ON early for a successful synchronization of
> >   	 * codec internal clock. And the clocks are turned off during @@
> > -91,38 +108,48 @@ static int platform_clock_control(struct
> snd_soc_dapm_widget *w,
> >   	 */
> >   	switch (event) {
> >   	case SND_SOC_DAPM_PRE_PMU:
> > -		/* 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;
> > +			dev_err(card->dev, "Can't set rate for mclk for ssp%d,
> err: %d\n",
> > +				ssp_num, ret);
> > +				return ret;
> 
> nit-pick: alignment is off for the 'return ret'.
my bad... will change that
> 
> >   		}
> >
> > -		ret = clk_prepare_enable(priv->mclk);
> > -		if (ret < 0) {
> > -			dev_err(card->dev, "Can't enable mclk, err: %d\n",
> ret);
> > -			return ret;
> > +		if (!__clk_is_enabled(priv->mclk)) {
> > +			/* Enable MCLK */
> > +			ret = clk_prepare_enable(priv->mclk);
> 
> That seems correct since you share the mclk between two resources but see [1]
> below
> 
> > +			if (ret < 0) {
> > +				dev_err(card->dev, "Can't enable mclk for
> ssp%d, err: %d\n",
> > +					ssp_num, ret);
> > +				return ret;
> > +			}
> >   		}
> >
> > -		/* Enable SCLK */
> > -		ret = clk_set_rate(priv->sclk, 3072000);
> > +		ret = clk_set_rate(sclk, sclk_rate);
> >   		if (ret < 0) {
> > -			dev_err(card->dev, "Can't set rate for sclk, err: %d\n",
> > -				ret);
> > +			dev_err(card->dev, "Can't set rate for sclk for ssp%d,
> err: %d\n",
> > +				ssp_num, 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);
> > +		if (!__clk_is_enabled(sclk)) {
> 
> Why do you need this test? the sclocks are not shared? see also [2] below
My thought process was if the clock is already enabled, then we don't have to enable it. 
Isee your point, I can skip this check. This check will always be true.
> 
> > +			/* Enable SCLK */
> > +			ret = clk_prepare_enable(sclk);
> > +			if (ret < 0) {
> > +				dev_err(card->dev, "Can't enable sclk for
> ssp%d, err: %d\n",
> > +					ssp_num, ret);
> > +				clk_disable_unprepare(priv->mclk);
> > +				return ret;
> > +			}
> >   		}
> >   		break;
> >   	case SND_SOC_DAPM_POST_PMD:
> > -		clk_disable_unprepare(priv->mclk);
> > -		clk_disable_unprepare(priv->sclk);
> > +		if (__clk_is_enabled(priv->mclk))
> > +			clk_disable_unprepare(priv->mclk);
> > +
> 
> [1] this seems wrong in case you have two SSPs working, and stop one.
> This would turn off the mclk while one of the two SSPs is still working.
For this platform we use either headset or dmic. 
There is no way we can record simultaneously using different devices.
So disabling mclk might not be harmful here. But this case will always be true too :). 
> 
> > +		if (__clk_is_enabled(sclk))
> 
> [2] Again is this test needed since sclk is not shared between SSPs
Same thought process to check if its enabled or not. Will remove that.
> 
> > +			clk_disable_unprepare(sclk);