On Thu, Sep 01, 2011 at 08:21:14PM +0800, johnnyhsu@realtek.com wrote:

Subject: [PATCH] ASoC: rt5631: first commit

"Add driver for..." or something.

+static int rt5631_volatile_register(struct snd_soc_codec *codec,

• 		    unsigned int reg)
  

+{

• switch (reg) {
• case RT5631_VENDOR_ID:
• case RT5631_VENDOR_ID1:
• case RT5631_VENDOR_ID2:

• return 0;
  

• default:

• return 1;
  

• }

+}

This looks to be exactly the wrong way round - the only registers we can cache are the ID registers.

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

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

TLV for volume information.

• rt5631->bclk_rate = snd_soc_params_to_bclk(params);
• if (rt5631->bclk_rate < 0) {

• dev_err(codec->dev, "Unsupported BCLK rate: %d\n",
  

• 			rt5631->bclk_rate);
  

• return rt5631->bclk_rate;
  

• }

The error message here is inacurate, there was an error obtaining the BCLK rate rather than a problem supporting it.

+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;
• dev_dbg(codec->dev, "enter %s\n", __func__);
• if (!freq_in || !freq_out) {

• dev_dbg(codec->dev, "PLL disabled\n");
  

• return -EINVAL;
  

• }

This shouldn't be an error condition, disabling the PLL is a perfectly normal thing to do.

• if (rt5631->master) {

• for (i = 0; i < ARRAY_SIZE(codec_master_pll_div); i++)
  

• 	if (freq_in == codec_master_pll_div[i].pll_in &&
  

Since the PLL configuration depends on the master/slave configuration you should either warn or reconfigure if the PLL is set up and the user changes master/slave configuration.

+/**

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

+static ssize_t rt5631_index_reg_show(struct device *dev,

• struct device_attribute *attr, char *buf)

+{

No, use the standard facilities. Please don't ignore review comments, it's not helpful.