On 2015/4/20 下午 10:57, Lars-Peter Clausen wrote:
On 04/16/2015 11:56 AM, Chih-Chiang Chang wrote:
The NAU88L25 is an ultra-low power high performance audio codec designed for smartphone, tablet PC, and other portable devices by Nuvoton, now add linux driver support for it.
Signed-off-by: Chih-Chiang Chang ccchang12@nuvoton.com
Looks pretty good now.
v2->v1: - fixes according to Lars-Peter Clausen's review comments - removes unused platform data file - corrects the naming of DAPM input widget - fixes some wrong coding of SOC widgets and other codes - adds definition and remark for config FLL clock - moves the code of reset hardware registers from codec_probe() to i2c_probe() - removes unused codes
[...]
+static const struct snd_kcontrol_new nau8825_snd_controls[] = {
Here and a few other places you leave the first line in the struct declaration empty. No need for that.
Removed the space line in declarations.
- SOC_SINGLE_TLV("MIC Volume", NAU8825_ADC_DGAIN_CTRL,
NAU8825_ADC_DGAIN_SFT,NAU8825_ADC_VOL_RSCL_RANGE, 0, adc_vol_tlv),- SOC_DOUBLE_TLV("HP Volume", NAU8825_HSVOL_CTRL,
NAU8825_L_HSVOL_SFT, NAU8825_R_HSVOL_SFT,NAU8825_VOL_RSCL_RANGE, 1, out_hp_vol_tlv),+};
[...]
+static void config_fll_clk_12m(struct snd_soc_codec *codec) +{
- struct nau8825_priv *nau8825 = snd_soc_codec_get_drvdata(codec);
- regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
NAU8825_CLK_MCLK_SRC_MASK, 0x0003);- regmap_update_bits(nau8825->regmap, NAU8825_FLL_1,
NAU8825_FLL_RATIO_MASK, 0x0001);- /* FLL 16-bit fractional input */
- regmap_write(nau8825->regmap, NAU8825_FLL_2, 0xC49B);
- /* FLL 10-bit integer input */
- regmap_update_bits(nau8825->regmap, NAU8825_FLL_3,
NAU8825_FLL_INTEGER_MASK, 0x0020);- /* FLL pre-scaler */
- regmap_update_bits(nau8825->regmap, NAU8825_FLL_4,
NAU8825_FLL_REF_DIV_MASK, 0x0800);This seems to use some constant dividers and multipliers for the FLL. Does this expect a specific reference clock from somewhere?
Yes, current code expects ASoC will send 12 MHz MCLK to the codec. But finally, we will support all possible reference clock frequencies.
- /* select divied VCO input */
- regmap_update_bits(nau8825->regmap, NAU8825_FLL_5,
NAU8825_FLL_FILTER_SW_MASK, 0x0000);- /* FLL sigma delta modulator enable */
- regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,
NAU8825_SDM_EN_MASK, 0x4000);+}
+static void set_sys_clk(struct snd_soc_codec *codec, int sys_clk) +{
- struct nau8825_priv *nau8825 = snd_soc_codec_get_drvdata(codec);
- pr_debug("%s :: sys_clk=%x\n", __func__, sys_clk);
- switch (sys_clk) {
- case NAU8825_INTERNALCLOCK:
regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_DIS);regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,NAU8825_DCO_EN_MASK, NAU8825_DCO_EN);regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_EN);break;- case NAU8825_MCLK:
- default:
regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_DIS);regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,NAU8825_DCO_EN_MASK, NAU8825_DCO_DIS);regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_EN);break;- }
+}
+static int nau8825_dai_set_sysclk(struct snd_soc_dai *dai,
int clk_id, unsigned int freq, int dir)+{
- struct snd_soc_codec *codec = dai->codec;
- switch (clk_id) {
- case NAU8825_MCLK:
config_fll_clk_12m(codec);set_sys_clk(codec, clk_id);Maybe just inline the contents of set_sys_clk here ...
Since another code as below still uses set_sys_clk(), we reserve function calling here.
case SND_SOC_BIAS_OFF: dev_dbg(codec->dev, "###nau8825_set_bias_level OFF\n"); set_sys_clk(codec, NAU8825_INTERNALCLOCK); regmap_update_bits(nau8825->regmap, NAU8825_BIAS_ADJ, NAU8825_VMID_MASK, NAU8825_VMID_DIS);
break;- case NAU8825_INTERNALCLOCK:
set_sys_clk(codec, clk_id);... and here
The same answer to previous.
break;- default:
dev_err(codec->dev, "Wrong clock src\n");return -EINVAL;- }
- return 0;
+}
[...]
+static const struct reg_default nau8825_reg[] = {
- /* enable clock source */
- {0x0001, 0x07FF},
- /* enable VMID and Bias */
- {0x0076, 0x3140},
- /* setup clock divider */
- {0x0003, 0x0050},
- /* jack detection configuration */
- {0x000C, 0x0004},
- {0x000D, 0x00E0},
- {0x000F, 0x0801},
- {0x0012, 0x0010},
- /* keypad detection configuration */
- {0x0013, 0x0280},
- {0x0014, 0x7310},
- {0x0015, 0x050E},
- {0x0016, 0x1B2A},
- /* audio format configuration */
- {0x001A, 0x0800},
- {0x001C, 0x000E},
- {0x001D, 0x0010},
- /* sampling rate control */
- {0x002B, 0x0012},
- /* DAC sampling rate control */
- {0x002C, 0x0082},
- /* ADC and DAC mixer control */
- {0x0030, 0x00D2},
- {0x0033, 0x00CF},
- {0x0034, 0x02CF},
- /* DAC class-G control */
- {0x006A, 0x1003},
- {0x0050, 0x2007},
- /* ADC PGA control */
- {0x0072, 0x0260},
- /* DAC power down enabled */
- {0x0080, 0x03A0},
- /* enable DAC clock */
- {0x0073, 0x336C},
- /* enable MIC Bias */
- {0x0074, 0x5502},
- /* powerup output driver */
- {0x007F, 0x473C},
- {0x007F, 0x473F},
- /* DAC power down disabled */
- {0x0080, 0x00A0},
- /* adjust MIC Bias */
- {0x0066, 0x2060},
- {0x0066, 0x2060},
- {0x0066, 0x0060},
+};
The register defaults should hold the content of the registers in the power-on reset state of the chip. This is not meant to be used as a initialization sequence.
Modified code to make register defaults hold the register values in the power-on reset state.
static const struct reg_default nau8825_reg[] = { {0x000, 0x0000}, {0x001, 0x00ff}, {0x003, 0x0050}, {0x004, 0x0000}, {0x005, 0x3126}, {0x006, 0x0008}, {0x007, 0x0010}, {0x008, 0x0000}, {0x009, 0x6000}, {0x00a, 0xf13c}, {0x00c, 0x000c}, {0x00d, 0x0000}, {0x00f, 0x0800}, {0x010, 0x0000}, {0x011, 0x0000}, {0x012, 0x0010}, {0x013, 0x0015}, {0x014, 0x0110}, {0x015, 0x0000}, ...
[...]
+static struct snd_soc_dai_driver nau8825_dai_driver[] = {
- {
.name = "nau8825-aif1",.playback = {.stream_name = "AIF1 Playback",.channels_min = 1,.channels_max = 2,.rates = NAU8825_RATES,.formats = NAU8825_FORMATS,},.capture = {.stream_name = "AIF1 Capture",.channels_min = 1,.channels_max = 2,.rates = NAU8825_RATES,.formats = NAU8825_FORMATS,},The indent is a bit strange above.
Fixed the code as below.
static struct snd_soc_dai_driver nau8825_dai_driver[] = { { .name = "nau8825-aif1", .playback = { .stream_name = "AIF1 Playback", .channels_min = 1, .channels_max = 2, .rates = NAU8825_RATES, .formats = NAU8825_FORMATS, }, .capture = { .stream_name = "AIF1 Capture", .channels_min = 1, .channels_max = 2, .rates = NAU8825_RATES, .formats = NAU8825_FORMATS, }, .ops = &nau8825_dai_ops, } };
.ops = &nau8825_dai_ops,- }
+};
+static int nau8825_i2c_probe(struct i2c_client *i2c,
const struct i2c_device_id *i2c_id)+{
- struct nau8825_priv *nau8825;
- int i, ret;
- nau8825 = devm_kzalloc(&i2c->dev, sizeof(*nau8825),
GFP_KERNEL);- if (nau8825 == NULL)
return -ENOMEM;- nau8825->i2c = i2c;
- i2c_set_clientdata(i2c, nau8825);
- nau8825->regmap = devm_regmap_init_i2c(i2c, &nau8825_regmap);
- if (IS_ERR(nau8825->regmap)) {
ret = PTR_ERR(nau8825->regmap);dev_err(&i2c->dev, "Failed to allocate regmap: %d\n", ret);goto err_enable;Since there is nothing to do at err_enable, just return ret directly and get rid of the label.
Modified the code as below.
i2c_set_clientdata(i2c, nau8825); nau8825->regmap = devm_regmap_init_i2c(i2c, &nau8825_regmap); if (IS_ERR(nau8825->regmap)) { ret = PTR_ERR(nau8825->regmap); dev_err(&i2c->dev, "Failed to allocate regmap: %d\n", ret); return ret; } /* software reset */ regmap_write(nau8825->regmap, NAU8825_RESET, 0x00); regmap_write(nau8825->regmap, NAU8825_RESET, 0x00);
/* register sound card */ ret = snd_soc_register_codec(&i2c->dev, &soc_codec_driver_nau8825, nau8825_dai_driver, ARRAY_SIZE(nau8825_dai_driver)); return ret; }
- }
- /* software reset */
- regmap_write(nau8825->regmap, NAU8825_RESET, 0x01);
- regmap_write(nau8825->regmap, NAU8825_RESET, 0x02);
- /*writing initial register values to the codec*/
- for (i = 0; i < ARRAY_SIZE(nau8825_reg); i++)
regmap_write(nau8825->regmap, nau8825_reg[i].reg,nau8825_reg[i].def);- ret = snd_soc_register_codec(&i2c->dev, &soc_codec_driver_nau8825,
nau8825_dai_driver,ARRAY_SIZE(nau8825_dai_driver));+err_enable:
- return ret;
+}
[...]
+#define NAU8825_RATES SNDRV_PCM_RATE_8000_192000 +#define NAU8825_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE \
| SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)These defines should probably be moved to the C file close to their users.
Move the defines to C file as below.
#define NAU8825_RATES SNDRV_PCM_RATE_8000_192000 #define NAU8825_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE \ | SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
static const struct snd_soc_dai_ops nau8825_dai_ops = { .trigger = nau8825_trigger, .hw_params = nau8825_hw_params, .set_sysclk = nau8825_dai_set_sysclk, .set_fmt = nau8825_set_dai_fmt, .digital_mute = nau8825_dac_mute, };
static struct snd_soc_dai_driver nau8825_dai_driver[] = { { .name = "nau8825-aif1",
+struct nau8825_priv {
- struct snd_soc_codec *codec;
- struct regmap *regmap;
- struct i2c_client *i2c;
- struct snd_soc_jack *jack;
- struct delayed_work jack_detect_work;
All fields in here except for the regmap field are still unused.
Remove some unused field. But the i2c field is used in nau8825_i2c_probe().
struct nau8825_priv { struct regmap *regmap; struct i2c_client *i2c; };
static int nau8825_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *i2c_id) { struct nau8825_priv *nau8825; int ret;
nau8825 = devm_kzalloc(&i2c->dev, sizeof(*nau8825), GFP_KERNEL); if (nau8825 == NULL) return -ENOMEM; nau8825->i2c = i2c; i2c_set_clientdata(i2c, nau8825); nau8825->regmap = devm_regmap_init_i2c(i2c, &nau8825_regmap);
+}; +#endif /* _NAU8825_H */