Re: [alsa-devel] [PATCH v2] ASoC: Intel: Add Cherrytrail & Braswell machine driver cht_bsw_rt5672

On Tue, Oct 28, 2014 at 07:30:32PM +0800, mengdong.lin@intel.com wrote:

+config SND_SOC_INTEL_CHT_BSW_RT5672_MACH

•    tristate "ASoC Audio driver for Intel Cherrytrail & Braswell with RT5672 codec"
  

•    depends on X86_INTEL_LPSS
  

•    select SND_SOC_RT5670
  

•    select SND_SST_MFLD_PLATFORM
  

•    select SND_SOC_INTEL_SST
  

•    select SND_SST_IPC
  

•    select SND_SST_MACHINE
  

•    default n
  

n is the default anyway.

+obj-$(CONFIG_SND_SST_MACHINE) += board/

It's a bit weird to have the Kconfig in the root Intel directory if the boards are getting moved into a subdirectory - why not just source a Kconfig in the board directory?

+static int cht_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, "rt5670-aif1", 11))

• return 0;
  

11 is a magic number! Why not use strlen() here (and why do we do nothing in the AIF1 hw_params() if we're working with AIF1)?

• ret = snd_soc_dai_set_pll(codec_dai, 0, RT5670_PLL1_S_MCLK,

• 		  CHT_PLAT_CLK_3_HZ, params_rate(params) * 512);
  

• if (ret < 0) {

• dev_err(rtd->dev, "can't set codec pll: %d\n", ret);
  

• return ret;
  

• }
• ret = snd_soc_dai_set_sysclk(codec_dai, RT5670_SCLK_S_PLL1,

• 		     params_rate(params) * 512,
  

• 		     SND_SOC_CLOCK_IN);
  

• if (ret < 0) {

• dev_err(rtd->dev, "can't set codec sysclk: %d\n", ret);
  

• return ret;
  

• }

Nothing ever stops the PLL - a set_bias_level() callback with a handler for _OFF. Otherwise we're wasting power.

• snd_soc_card_cht.dev = &pdev->dev;
• ret_val = snd_soc_register_card(&snd_soc_card_cht);
• if (ret_val) {

devm_snd_soc_register_card() and the removal function can go entirely.