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.