On Sun, Feb 15, 2015 at 03:49:30PM +0800, Wan Zongshun wrote:
- /* SP Class-D mute control */
- SOC_DOUBLE("HP Playback Switch", NAU8824_HP_MUTE, NAU8824_L_MUTE_SFT,
NAU8824_R_MUTE_SFT, 1, 1),- SOC_SINGLE_TLV("HP Left Volume", NAU8824_HP_VOL, NAU8824_L_VOL_SFT,
NAU8824_VOL_RSCL_RANGE, 1, out_hp_vol_tlv),- SOC_SINGLE_TLV("HP Right Volume", NAU8824_HP_VOL, NAU8824_R_VOL_SFT,
NAU8824_VOL_RSCL_RANGE, 1, out_hp_vol_tlv),
I would have expected the headphone volume control to be a stereo (double) control - same for speakers.
- /* DMIC control */
- SOC_DOUBLE("DMIC L R Switch", NAU8824_ENA_CTRL, NAU8824_DMIC_CH0_SFT,
NAU8824_DMIC_CH1_SFT, 1, 0),- SOC_SINGLE("DMIC Enable", NAU8824_BIAS_ADJ, NAU8824_DMIC1_SFT, 1, 0),
- SOC_SINGLE("VMID Switch", NAU8824_BIAS_ADJ, NAU8824_VMID_SFT, 1, 0),
- SOC_SINGLE("BIAS Switch", NAU8824_BOOST, NAU8824_G_BIAS_SFT, 1, 0),
This all looks like stuff that should be done with DAPM...
- SOC_DOUBLE_R_TLV("MIC L R Gain", NAU8824_ADC_CH0_DGAIN_CTRL,
NAU8824_ADC_CH1_DGAIN_CTRL, 0,NAU8824_ADC_VOL_RSCL_RANGE, 0, adc_vol_tlv),
All volume controls should be called Volume.
+static int set_dmic_clk(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
- struct snd_soc_codec *codec = w->codec;
w->codec is going away, use snd_soc_dapm_to_codec(). You should always submit against current code.
+static const struct snd_kcontrol_new nau8824_rec_l_mix[] = {
SOC_DAPM_SINGLE("BST1 Switch", SND_SOC_NOPM,0, 0, 0),+};
Please use normal indentation, a single tab is enough.
+static int nau8824_hp_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
- return 0;
+}
Remove empty functions.
- switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
- case SND_SOC_DAIFMT_CBM_CFM:
reg_val_2 |= NAU8824_I2S_MS_M;break;- case SND_SOC_DAIFMT_CBS_CFS:
break;- case SND_SOC_DAIFMT_CBS_CFM:
break;
These two clocking configurations appear not to differ in terms of what we do to the device - this suggests that only one of them is actually supported.
+static int nau8824_FLL_freerun_mode(struct snd_soc_codec *codec, int IsEnable) +{
- if (IsEnable == true) {
This doesn't look like Linux coding style...
+void set_sys_clk(struct snd_soc_codec *codec, int sys_clk) +{
- pr_debug("%s sys_clk=%x\n", __func__, sys_clk);
- switch (sys_clk) {
- case NAU8824_INTERNALCLOCK:
nau8824_FLL_freerun_mode(codec, true);break;- case NAU8824_MCLK:
- default:
nau8824_FLL_freerun_mode(codec, false);break;- }
+}
...and I don't entirely see why it's a separate function to this (which is itself an internal wrapper function which possibly shouldn't exist)>
+static int nau8824_set_sysclk(struct snd_soc_codec *codec,
int clk_id, int source, unsigned int freq, int dir)+{
- int divider = 1;
- if (clk_id == NAU8824_MCLK) {
set_sys_clk(codec, NAU8824_MCLK);dev_dbg(codec->dev, "%s: input freq = %d divider = %d",__func__, freq, divider);- } else if (clk_id == NAU8824_INTERNALCLOCK) {
set_sys_clk(codec, NAU8824_INTERNALCLOCK);- } else {
dev_err(codec->dev, "Wrong clock src\n");return -EINVAL;- }
Use switch statements rather than if trees like other drivers. It looks like clk_id should actually be source since we're selecting the clock to use rather than selecting which clock to configure.
+static int nau8824_set_bias_level(struct snd_soc_codec *codec,
enum snd_soc_bias_level level)+{
- dev_dbg(codec->dev, "## nau8824_set_bias_level %d\n", level);
- if (level == codec->dapm.bias_level) {
dev_dbg(codec->dev, "##set_bias_level: level returning...\r\n");return -EINVAL;- }
Why is this here - other drivers don't do this and it doesn't look device specific?
- switch (level) {
- case SND_SOC_BIAS_ON:
/* All power is driven by DAPM system*/dev_dbg(codec->dev, "###nau8824_set_bias_level BIAS_ON\n");snd_soc_update_bits(codec, NAU8824_BIAS_ADJ,NAU8824_VMID_MASK, NAU8824_VMID_EN);snd_soc_update_bits(codec, NAU8824_BOOST,NAU8824_G_BIAS_MASK, NAU8824_G_BIAS_EN);
We turn on VMID last? That's a strange thing to do...
+static int nau8824_suspend(struct snd_soc_codec *codec) +{
- dev_dbg(codec->dev, "%s: Entered\n", __func__);
- nau8824_set_bias_level(codec, SND_SOC_BIAS_OFF);
- dev_dbg(codec->dev, "%s: Exiting\n", __func__);
- return 0;
+}
Set idle_bias_off (which it looks like you should be doing) and this becomes redundant. If you're *not* using idle_bias_off for some reason then the set_bias_level() work done in _ON should be undone in at least _SUSPEND rather than _OFF so we save power on idle.
+struct nau8824_init_reg {
- u8 reg;
- u16 val;
+};
This looks like you're reimplementing regmap's register sequence stuff... It's also a *very* large sequence you have, are you sure it's all required? It seems like this may be doing a bunch of machine specific configuration but since it's all magic numbers it's hard to tell.
+#define NAU8824_INIT_REG_LEN ARRAY_SIZE(init_list)
Just use ARRAY_SIZE().
+static int nau8824_reg_init(struct snd_soc_codec *codec) +{
- int i;
- mdelay(1);
- for (i = 0; i < NAU8824_INIT_REG_LEN; i++) {
if (init_list[i].reg == 0xFFFF)mdelay(1);elsesnd_soc_write(codec, init_list[i].reg,init_list[i].val);- }
Use the standard regmap patch stuff. If you need the delays in the sequence implement that in the core, though I guess you don't since reg is a u8 and you're looking for 0xffff in it...
- /* Dynamic Headset detection enabled */
- snd_soc_update_bits(codec, 0x01, 0x400, 0x400);
- snd_soc_update_bits(codec, 0x02, 0x0008, 0x0008);
- snd_soc_update_bits(codec, 0x0f, 0x0300, 0x0100);
- snd_soc_write(codec, 0x09, 0xE000);
- snd_soc_write(codec, NAU8824_IRQ_SETTING, 0x1006);
- snd_soc_write(codec, 0x13, 0x1615);
- snd_soc_write(codec, 0x15, 0x0414);
- snd_soc_update_bits(codec, 0x16, 0xFF00, 0x5900);
- snd_soc_update_bits(codec, 0x66, 0x0070, 0x0060);
Too many magic numbers here I think and this looks like it should be configured using platform data and/or the machine driver (what if the headphone detection/IRQ aren't wired up?). I'd also expect to see reporting via the standard interfaces for jack reporting.
+static const struct i2c_device_id nau8824_i2c_id[] = {
- { "nau8824", 0},
- {"10508824:00", 0},
- {"10508824", 0},
- { }
+};
It looks like you've put some ACPI IDs in the I2C ID table here. At least I hope that's what the last two entries are... if they are ACPI IDs they should be registered as such. If they're something else then what are they?