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);
> + else
> + snd_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?
signature.asc
Description: Digital signature
