On Mon, Aug 22, 2011 at 11:31:30AM +0800, johnnyhsu wrote:

+/* MIC Input Type */ +static const char *rt5631_input_mode[] = {

• "Single-end", "Differential"};

Single ended.

+/* MIC Boost Volume */ +static const char *rt5631_mic_boost[] = {"Bypass", "+20db", "+24db", "+30db",

• 	"+35db", "+40db", "+44db", "+50db", "+52db"};
  

TLV please. I'm fairly sure I mentioned this last time.

+/* Record Data Select */ +static const char *rt5631_rec_sel[] = {"Disable", "MIC1", "MIC2", "Stereo"};

+static const SOC_ENUM_SINGLE_DECL(

• rt5631_rec_sel_enum, RT5631_INT_ST_IRQ_CTRL_2,
• RT5631_ADC_DATA_SEL_SHIFT, rt5631_rec_sel);

This looks like DAPM...

+/* SPK Channel Volume Input Select */ +static const char *rt5631_spkvol_sel[] = {"VMID", "SPKMIX"};

+static const SOC_ENUM_DOUBLE_DECL(

• rt5631_spkvol_enum, RT5631_SPK_OUT_VOL,
• RT5631_L_EN_SHIFT, RT5631_R_EN_SHIFT, rt5631_spkvol_sel);

This too, and similarly for the other mixers.

+static void rt5631_set_dmic_params(struct snd_soc_codec *codec,

• struct snd_pcm_hw_params *params)

+{

• int rate;
• snd_soc_update_bits(codec, RT5631_GPIO_CTRL,

• RT5631_GPIO_PIN_FUN_SEL_MASK |
  

• RT5631_GPIO_DMIC_FUN_SEL_MASK,
  

• RT5631_GPIO_PIN_FUN_SEL_GPIO_DIMC |
  

• RT5631_GPIO_DMIC_FUN_SEL_DIMC);
  

Shouldn't this be done once in th eprobe function? DMICs aren't usually hotplugged...

• snd_soc_update_bits(codec, RT5631_DIG_MIC_CTRL,

• RT5631_DMIC_ENA_MASK, RT5631_DMIC_ENA);
  

This looks like DAPM.

• snd_soc_update_bits(codec, RT5631_DIG_MIC_CTRL,

• RT5631_DMIC_L_CH_LATCH_MASK |
  

• RT5631_DMIC_R_CH_LATCH_MASK,
  

• RT5631_DMIC_L_CH_LATCH_FALLING |
  

• RT5631_DMIC_R_CH_LATCH_RISING);
  

This also looks like probe work.

• default:

• break;
  

• }

Should return an error if you don't know how to configure.

• snd_soc_update_bits(codec, RT5631_SDP_CTRL,

• RT5631_SDP_I2S_DL_MASK, iface);
  

• if (coeff >= 0)

• snd_soc_write(codec, RT5631_STEREO_AD_DA_CLK_CTRL,
  

• 			coeff_div[coeff].reg_val);
  

What if you didn't find coefficients?

+static int rt5631_hifi_codec_set_dai_sysclk(struct snd_soc_dai *codec_dai,

• 		int clk_id, unsigned int freq, int dir)
  

+{

• struct snd_soc_codec *codec = codec_dai->codec;
• struct rt5631_priv *rt5631 = snd_soc_codec_get_drvdata(codec);
• pr_info("enter %s, syclk=%d\n", __func__, freq);
• if ((freq >= (256 * 8000)) && (freq <= (512 * 96000))) {

• rt5631->sysclk = freq;
  

• return 0;
  

• }
• pr_info("unsupported sysclk freq %u for audio i2s\n", freq);
• pr_info("set sysclk to 24.576Mhz by default\n");
• rt5631->sysclk = 24576000;
• return 0;

No, return an error if the user specified something you don't understand. If that's the sysclk the user can always specify it directly.

+static int rt5631_codec_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,

• int source, unsigned int freq_in, unsigned int freq_out)
  

+{

• struct snd_soc_codec *codec = codec_dai->codec;
• struct rt5631_priv *rt5631 = snd_soc_codec_get_drvdata(codec);
• int i, ret = -EINVAL;
• printk(KERN_DEBUG "enter %s\n", __func__);
• if (!freq_in || !freq_out)

• return 0;
  

Should stop the PLL if a zero output is requested.

+/**

• • rt5631_index_reg_show - sysfs file for dumping index registers of 2nd layer
• */

+#define IDX_REG_FMT "%02x: %04x\n" +#define IDX_REG_LEN 9 +static ssize_t rt5631_index_reg_show(struct device *dev,

• struct device_attribute *attr, char *buf)

You didn't answer any of my queries about this stuff on the prior posting...

• ret = snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_I2C);
• if (ret != 0) {

• dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
  

• return ret;
  

• }
• codec->cache_bypass = 1;

Why cache_bypass?

• /* speaker ratio is 1.44x */
• snd_soc_write(codec, RT5631_GEN_PUR_CTRL_REG, 0x4e00);

Hrm?