On Fri, May 31, 2013 at 03:04:59PM +0800, bardliao@realtek.com wrote:

From: Bard Liao bardliao@realtek.com

This looks pretty good, a few small things below and probably one actual bug.

• Use regmap_range_cfg to replace index read/write function.
• Remove I2S3 related code since there is no I2S3 in ALC5640.
• Remove Voice DSP related code since there is no Voice DSP in ALC5640.
• Remove MICBIAS2 since there is no MICBIAS2 in ALC5640.
• Change DMIC1/2 CLK to DMIC1/2 Power since it is for enable/disable DMIC1/2
• Modify some texts for consistent coding style.
• Merge sto adc l/r mux since it shares the same control bits.
• Other minor changes.

Put stuff like this after the --- so it doesn't end up in the commit log.

• switch (event) {
• case SND_SOC_DAPM_POST_PMU:

• snd_soc_update_bits(codec, RT5640_PWR_DIG1, 0x0001, 0x0001);
  

• regmap_update_bits(rt5640->regmap, 0x1c, 0xf000, 0xf000);
  

• break;
  

It's a bit weird to be using snd_soc_update_bits() and regmap_update_bits()...

• SND_SOC_DAPM_SUPPLY("DMIC1 Power", SND_SOC_NOPM, 0, 0,

• rt5640_set_dmic1_event,
  

• SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
  

• SND_SOC_DAPM_SUPPLY("DMIC2 Power", SND_SOC_NOPM, 0, 0,

• rt5640_set_dmic2_event,
  

• SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
  

The enable bits could be listed here - this would be a bit more idiomatic, would save the PMD event and would enable the writes to be combined.

• case SND_SOC_BIAS_STANDBY:

• if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
  

• 	snd_soc_update_bits(codec, RT5640_PWR_ANLG1,
  

• 		RT5640_PWR_VREF1 | RT5640_PWR_MB |
  

• 		RT5640_PWR_BG | RT5640_PWR_VREF2,
  

• 		RT5640_PWR_VREF1 | RT5640_PWR_MB |
  

• 		RT5640_PWR_BG | RT5640_PWR_VREF2);
  

• 	mdelay(10);
  

• 	snd_soc_update_bits(codec, RT5640_PWR_ANLG1,
  

• 		RT5640_PWR_FV1 | RT5640_PWR_FV2,
  

• 		RT5640_PWR_FV1 | RT5640_PWR_FV2);
  

• 	regcache_cache_only(rt5640->regmap, false);
  

• 	regcache_sync(rt5640->regmap);
  

This looks wrong - you're writing to the device before you take it out of cache only mode so your delay in the above sequence will have no effect.

• val = snd_soc_read(codec, RT5640_VENDOR_ID2);
• if ((val != RT5640_DEVICE_ID)) {

• dev_err(codec->dev,
  

• 	"Device with ID register %x is not rt5640/39\n", val);
  

• return -ENODEV;
  

• }
• rt5640_reset(codec);

Do these in the I2C probe function before you register the CODEC, that way we don't try to start the sound card if we can't talk to the device.

• ret = regmap_register_patch(rt5640->regmap, init_list,

• 		    ARRAY_SIZE(init_list));
  

• if (ret != 0)

• dev_warn(codec->dev, "Failed to apply regmap patch: %d\n",
  

• 	 ret);
  

This should also be in the I2C probe.