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

[Mark Brown]

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.

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

> +#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...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20141106/dd6c6086/attachment-0001.sig>