Re: [PATCH v3 5/6] ASoC: Intel: add BYTCR machine driver with RT5640

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2113 bytes --]

On Thu, Nov 06, 2014 at 12:48:54PM +0000, Mark Brown wrote:
> On Tue, Nov 04, 2014 at 04:25:19PM +0530, Vinod Koul wrote:
> 
> > +static int byt_aif1_hw_params(struct snd_pcm_substream *substream,
> > +					struct snd_pcm_hw_params *params)
> > +{
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> > +	int ret;
> > +
> > +	if (strncmp(codec_dai->name, "rt5640-aif1", 11))
> > +		return 0;
> 
> This looks wrong...  fairly sure I queried this on an earlier version of
> the patch and was told it wasn't required.
This was supposed to be removed, not sure why it creeped back in, will fix
now

> 
> > +	ret = snd_soc_dai_set_sysclk(codec_dai, RT5640_SCLK_S_PLL1,
> > +				     params_rate(params) * 512,
> > +				     SND_SOC_CLOCK_IN);
> > +	if (ret < 0) {
> > +		dev_err(rtd->dev, "can't set codec clock %d\n", ret);
> > +		return ret;
> > +	}
> > +	ret = snd_soc_dai_set_pll(codec_dai, 0, RT5640_PLL1_S_BCLK1,
> 
> Missing blank line here and in several places throughout the file.  I'd
> expect the PLL to be enabled before the sysclk is told to use it, error
> checking might kick in otherwise.
ok

> 
> > +#ifdef CONFIG_PM_SLEEP
> > +static int snd_byt_prepare(struct device *dev)
> > +{
> > +	return snd_soc_suspend(dev);
> > +}
> > +
> > +static void snd_byt_complete(struct device *dev)
> > +{
> > +	snd_soc_resume(dev);
> > +}
> > +
> > +static int snd_byt_poweroff(struct device *dev)
> > +{
> > +	return snd_soc_poweroff(dev);
> > +}
> > +#else
> > +#define snd_byt_prepare NULL
> > +#define snd_byt_complete NULL
> > +#define snd_byt_poweroff NULL
> > +#endif
> 
> Don't bother with the wrapper functions, they're not adding anything.
> Why are we using prepare() and complete() here - other machine drivers
> don't need to do that?  Comments might be helpful...
due to I2C. We have seen that codec is resumed but I2C is still
not ready causing i2c failures, so moving to complete and prepare helps. I
will add this comment. Will remove wrappers.

Thanks
-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]