Date:	Wed, 4 Mar 2015 20:35:52 +0800
From:	Chih-Chiang Chang <ccchang12@...oton.com>
To:	Mark Brown <broonie@...nel.org>
CC:	"mcuos.com@...il.com" <mcuos.com@...il.com>,
	"tiwai@...e.de" <tiwai@...e.de>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"lgirdwood@...il.com" <lgirdwood@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	<liam.r.girdwood@...el.com>
Subject: Re: [PATCH] ASoC: Add support for NAU8824 codec to ASoC



On 2015/2/24 下午 10:13, Mark Brown wrote:
> 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.
The nau8824 related registers which control left/right volume are located
in different addresses and different shift bits. Since there is no available
preprocessor macro to meet our requirements, the driver consists of left/right
volume control separately.
>
>> +    /* 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...
The above controls have been done with DAPM in next patch submit.
>
>> +    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.
The naming has been changed.
>
>> +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.
Modified done in next patch submit.
>
>> +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.
Modified done in next patch submit.
>
>> +static int nau8824_hp_event(struct snd_soc_dapm_widget *w,
>> +                            struct snd_kcontrol *kcontrol, int event)
>> +{
>> +    return 0;
>> +}
>
> Remove empty functions.
Modified done in next patch submit.
>
>> +    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.
Modified done in next patch submit.
>
>> +static int nau8824_FLL_freerun_mode(struct snd_soc_codec *codec, int IsEnable)
>> +{
>> +    if (IsEnable == true) {
>
> This doesn't look like Linux coding style...
Remove this function
>
>> +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)>
Remove "nau8824_FLL_freerun_mode()" function
>
>> +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.
Modified done in next patch submit.
>
>> +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?
Remove it
>
>> +    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...
In nau8824, VMID_EN is to enable Vreff pin
>
>> +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.
Remove suspend function
>
>> +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.
Initial settings are arranged in order
>
>> +#define NAU8824_INIT_REG_LEN ARRAY_SIZE(init_list)
>
> Just use ARRAY_SIZE().
Modified done in next patch submit.
>
>> +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...
Modified done in next patch submit.
>
>> +    /* 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.
The above initial settings are for jack detection. As for other jack
detection flow, it will be implemented in machine driver but not be included in
this application.
>
>> +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?
Yes, there are ACPI ID table and registered in the dedicated platform BIOS.
>

===========================================================================================
The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists