On Thu, Dec 13, 2012 at 11:19:29AM -0800, Jerry Wong wrote:

For legibility please submit future revisions as a two stage patch series, one removing the old driver and another adding the new one.

+/* codec platform data */ +struct max98090_pdata {

•   int irq;
  

This should be passed in using the normal mechanism provided by the bus, not custom implemented.

•   int power_over_performance;
  

This doesn't look like platform data to me, it looks like something that should be configured dynamically at runtime.

•   /* Equalizers for DAC */
  

•   struct max98090_eq_cfg *eq_cfg;
  

•   unsigned int eq_cfgcnt;
  

Coefficients should be configured using SND_SOC_BYTES(). Some older drivers use this method but it's deprecated.

•   { 0xE0, 0x00 }, /* E0 */
  

•   { 0xE1, 0x00 }, /* E1 */
  

•   { 0xE2, 0x00 }, /* E2 */
  

Do these number-only registers actually exist in the published register map or are they undefined? If they're undefined and not used by software there's no need to include them here.

•   switch (reg) {
  

•   case M98090_REG_01_IRQ_STATUS:
  

•   case M98090_REG_02_JACK_STATUS:
  

•   case M98090_REG_FF_REV_ID:
  

Including the register number in the constants seems to rather defeat the point of having symbolic names for the registers; it means people still have to remember the register numbers.

+static int max98090_get_enab_tlv(struct snd_kcontrol *kcontrol,

•                           struct snd_ctl_elem_value *ucontrol)
  

What's the relevance of _tlv here? It looks like these are normal get and sets.

+static const char * max98090_dsts_text[] =

•   { "None", "Left", "Right", "Left and Right" };
  

+static const struct soc_enum max98090_dsts_enum =

•   SOC_ENUM_SINGLE(M98090_REG_1A_LVL_SIDETONE, M98090_DSTS_SHIFT,
  

•           ARRAY_SIZE(max98090_dsts_text), max98090_dsts_text);
  

+static const struct snd_kcontrol_new max98090_snd_controls[] = {

•   SOC_SINGLE("DMIC MIC Left Enable", M98090_REG_13_MIC_CFG1,
  

•           M98090_DIGMICL_SHIFT, M98090_DIGMICL_NUM - 1, 0),
  

•   SOC_SINGLE("DMIC MIC Right Enable", M98090_REG_13_MIC_CFG1,
  

•           M98090_DIGMICR_SHIFT, M98090_DIGMICR_NUM - 1, 0),
  

This looks like it should be DAPM.

•   SOC_ENUM_EXT("External MIC Mux", max98090_extmic_mux_enum,
  

•           max98090_extmic_mux_get, max98090_extmic_mux_set),
  

Likewise.

•   SOC_ENUM("Digital Sidetone Source", max98090_dsts_enum),
  

DAPM.

•   SOC_SINGLE_EXT_TLV("Digital Sidetone Level Volume",
  

•           M98090_REG_1A_LVL_SIDETONE, M98090_DVST_SHIFT,
  

•           M98090_DVST_NUM - 1, 1, max98090_get_enab_tlv,
  

•           max98090_put_enab_tlv, max98090_micboost_tlv),
  

Level Volume?

•   SOC_SINGLE_TLV("Digital Gain Volume", M98090_REG_27_DAI_LVL,
  

•           M98090_DAI_LVL_DVG_SHIFT, M98090_DAI_LVL_DVG_NUM - 1, 0,
  

•           max98090_dvg_tlv),
  

Gain Volume?

•   SOC_SINGLE("Digital EQ 3 Band Enable", M98090_REG_41_DSP_EQ_EN,
  

•           M98090_EQ3BANDEN_SHIFT, M98090_EQ3BANDEN_NUM - 1, 0),
  

Switch.

•   SOC_SINGLE("DAC HP Playback Performance Mode", M98090_REG_43_DAC_CFG,
  

•           M98090_DAC_PERFMODE_SHIFT, M98090_DAC_PERFMODE_NUM - 1, 0),
  

•   SOC_SINGLE("DAC High Performance Mode", M98090_REG_43_DAC_CFG,
  

•           M98090_DACHP_SHIFT, M98090_DACHP_NUM - 1, 0),
  

These should either be enumerations with real names of Switches if it's just an on/off (looks like the former).

•   SOC_SINGLE_TLV("Headphone Left Volume", M98090_REG_2C_LVL_HP_LEFT,
  

•           M98090_HPVOLL_SHIFT, M98090_HPVOLL_NUM - 1, 0,
  

•           max98090_hp_tlv),
  

•   SOC_SINGLE_TLV("Headphone Right Volume", M98090_REG_2D_LVL_HP_RIGHT,
  

•           M98090_HPVOLR_SHIFT, M98090_HPVOLR_NUM - 1, 0,
  

•           max98090_hp_tlv),
  

Should be a single stereo control (and similarly for the other output controls).

•   SOC_SINGLE("Quick Setup System Clock", M98090_REG_04_QCFG_SYS_CLK,
  

•           M98090_CLK_ALL_SHIFT, M98090_CLK_ALL_NUM - 1, 0),
  

What do these controls do?

•   if (val >= 1) {
  

•           if (w->reg == M98090_REG_10_LVL_MIC1)
  

•                   max98090->mic1pre = val - 1; /* Update for volatile */
  

•           else
  

•                   max98090->mic2pre = val - 1; /* Update for volatile */
  

•   }
  

Use braces consistently within an if statemeent - if you use them for the outer one use them for the inner one too.

+static const struct snd_soc_dapm_route max98090_dapm_routes[] = {

•   {"MIC1 Input", NULL, "MIC1"},
  

•   {"MIC2 Input", NULL, "MIC2"},
  

•   {"MIC1 Input", NULL, "MICBIAS"},
  

•   {"MIC2 Input", NULL, "MICBIAS"},
  

Unless this chip is *very* unusual I expect the bias is something that's connected externally and therefore these connections should be being made by the machine driver.

•   /* Check for user calculated MI and NI ratios */
  

•   for (i = 0; i < ARRAY_SIZE(user_pclk_rates); i++) {
  

•           if ((user_pclk_rates[i] == max98090->sysclk) &&
  

•                   (user_lrclk_rates[i] == max98090->lrclk)) {
  

•                   dev_info(codec->dev,
  

•                           "Found user supported PCLK to LRCLK rates\n");
  

•                   dev_info(codec->dev, "i %d ni %lld mi %lld\n",
  

•                           i, ni_value[i], mi_value[i]);
  

dev_dbg() would be fine but dev_info() is too loud.

+/*

• • This accommodates an inverted logic in the MAX98090 chip
• • for Bit Clock Invert (BCI). The inverted logic is only seen
• • for the case of TDM mode. The remaining cases have normal logic.
• */

•           if (max98090->tdm_slots > 1) {
  

•                   regval ^= M98090_DAI_BCI_MASK;
  

•           }
  

Coding style, the indentation is inconsistent here and no braces for single line if () statements.

•           if (max98090->jack_state == M98090_JACK_STATE_HEADSET) {
  

•                   /*
  

•                    * Set to normal bias level.
  

•                    */
  

•                   snd_soc_update_bits(codec, M98090_REG_12_MIC_BIAS,
  

•                           M98090_MBVSEL_MASK, M98090_MBVSEL_2V4);
  

•                   snd_soc_update_bits(codec, M98090_REG_3E_PWR_EN_IN,
  

•                           M98090_PWR_MBEN_MASK, M98090_PWR_MBEN_MASK);
  

•           }
  

This looks suspicous, why does the headset state have any bearing here?

+static int max98090_dai_digital_mute(struct snd_soc_dai *codec_dai, int mute) +{

•   struct snd_soc_codec *codec = codec_dai->codec;
  

•   int regval;
  

•   dev_info(codec->dev, "max98090_dai_digital_mute\n");
  

Drop this, it's not conveying any useful information.

+static void max98090_dmic_switch(struct snd_soc_codec *codec, int state) +{

I have no idea what this function is supposed to do...

+static void max98090_headset_button_event(struct snd_soc_codec *codec) +{

•   dev_info(codec->dev, "max98090_headset_button_event\n");
  

+}

Delete this, it doesn't do anything except log.

•   switch (reg & (M98090_LSNS_MASK | M98090_JKSNS_MASK)) {
  

•           case M98090_LSNS_MASK | M98090_JKSNS_MASK:
  

•           {
  

•                   dev_info(codec->dev, "No Headset Detected\n");
  

•                   max98090->jack_state = M98090_JACK_STATE_NO_HEADSET;
  

•                   max98090_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
  

•                   snd_soc_dapm_disable_pin(&codec->dapm, "HPL");
  

•                   snd_soc_dapm_disable_pin(&codec->dapm, "HPR");
  

•                   snd_soc_dapm_force_enable_pin(&codec->dapm, "SPKL");
  

•                   snd_soc_dapm_force_enable_pin(&codec->dapm, "SPKR");
  

•                   snd_soc_dapm_disable_pin(&codec->dapm, "MIC1");
  

•                   snd_soc_dapm_disable_pin(&codec->dapm, "MIC2");
  

•                   snd_soc_dapm_force_enable_pin(&codec->dapm, "DMIC1");
  

•                   snd_soc_dapm_force_enable_pin(&codec->dapm, "DMIC2");
  

•                   max98090_dmic_switch(codec, 1);
  

•                   break;
  

•           }
  

This doesn't look obviously sensible... calling set_bias_level() outside of DAPM is going to get overridden in pretty short order, none of the DAPM pin setting looks like a good idea (especially not forcing on the speakers whenever there's no headset) and there's no DAPM sync to make any of this take effect. I'm a bit confused about the intended effect. I also can't see any interaction with soc-jack...

What is this code actually intended to do?

•   dev_info(codec->dev, "***** max98090_interrupt *****\n");
  

There's lots of these noisy, uninformative prints at info level throughout the driver.

•   /* Send work to be scheduled */
  

•   if (active & M98090_IRQ_CLD_MASK) {
  

•           dev_info(codec->dev, "M98090_IRQ_CLD_MASK\n");
  

•   }
  

This doesn't actually schedule anything...

•   ret = snd_soc_codec_set_cache_io(codec, 8, 8, SND_SOC_I2C);
  

•   if (ret != 0) {
  

•           dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
  

•           return ret;
  

•   }
  

SND_SOC_REGMAP.

•   if ( (request_threaded_irq(pdata->irq, NULL,
  

•           max98090_interrupt, IRQF_TRIGGER_FALLING,
  

•           "max98090_interrupt", codec)) < 0) {
  

•           dev_info(codec->dev, "request_irq failed\n");
  

•   }
  

•   /*
  

•    * Clear any old interrupts.
  

Coding style and it's not terribly helpful to hide the return code from users.

•   /* Turn on VCM bandgap reference */
  

•   snd_soc_write(codec, M98090_REG_42_BIAS_CNTL,
  

•           M98090_VCM_MODE_MASK);
  

This shouldn't be managed at runtime?

+/*

• • Driver revision
• */

+#define MAX98090_REVISION "0.01.00"

The kernel already has perfectly good versioning.