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

From: "N, Harshapriya" <harshapriya.n@intel.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	"Gopal, Vamshi Krishna" <vamshi.krishna.gopal@intel.com>,
	"lma@semihalf.com" <lma@semihalf.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>
Cc: "M R, Sathya Prakash" <sathya.prakash.m.r@intel.com>,
	"brndt@google.com" <brndt@google.com>
Subject: RE: [PATCH] ASoC: Intel: boards: eve: Fix DMIC records zero
Date: Thu, 23 Jul 2020 23:37:04 +0000	[thread overview]
Message-ID: <BY5PR11MB430737AAF999E8308B3AD065FD760@BY5PR11MB4307.namprd11.prod.outlook.com> (raw)
In-Reply-To: <79866874-1ec3-50a1-1034-39bc116264fd@linux.intel.com>

> >   .../intel/boards/kbl_rt5663_rt5514_max98927.c | 150 ++++++++++++------
> >   1 file changed, 102 insertions(+), 48 deletions(-)
> >
> > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > index b34cf6cf1139..584e4f9cedc2 100644
> > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > @@ -53,8 +53,10 @@ struct kbl_codec_private {
> >   	struct snd_soc_jack kabylake_headset;
> >   	struct list_head hdmi_pcm_list;
> >   	struct snd_soc_jack kabylake_hdmi[2];
> > -	struct clk *mclk;
> > -	struct clk *sclk;
> > +	struct clk *ssp0_mclk;
> > +	struct clk *ssp0_sclk;
> > +	struct clk *ssp1_mclk;
> > +	struct clk *ssp1_sclk;
> 
> The definition of a per-SSP MCLK is just wrong. there are 2 MCLKs regardless of
> the number of SSPs, this should be MCLK0 and MCLK1.
> 
> It probably works in your case since you have 2 SSPs, but the Skylake driver
> exposes ssp2..5_mclk clocks that don't exist in hardware. Oh well.
> 
> If you don't mind I'd prefer it if you used mclk0 and mclk1 and drop the ssp_
> prefix. You can still use the "ssp0_mclk" and "ssp1_mclk" strings when calling
> devm_clk_get(), but that way if the Skylake driver is fixed at some point we will
> not have to change the code in this driver, only the clock names.
There is actually just one mclk in this platform. Only the sclk rate to be applied is different for different codecs.
The names are misleading. It looks like there are different clocks for different SSP. 
we shall post a new patch fixing this.
> 
> [...]
> 
> > @@ -757,6 +800,29 @@ static struct snd_soc_card kabylake_audio_card = {
> >   	.late_probe = kabylake_card_late_probe,
> >   };
> >
> > +static int kabylake_audio_clk_get(struct device *dev, const char *id,
> > +	struct clk **clk)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!clk)
> > +		return -EINVAL;
> > +
> > +	*clk = devm_clk_get(dev, id);
> > +	if (IS_ERR(*clk)) {
> > +		ret = PTR_ERR(*clk);
> > +		if (ret == -ENOENT) {
> > +			dev_info(dev, "Failed to get %s, defer probe\n", id);
> > +			return -EPROBE_DEFER;
> > +		}
> > +
> > +		dev_err(dev, "Failed to get %s with err:%d\n", id, ret);
> > +		return ret;
> 
> nit-pick: you can remove this return since you already have one two lines
> below.
ack
> 
> > +	}
> > +
> > +	return ret;
> > +}
> > +