Re: [PATCH 002/002] ASoC: Replace max98090 Device Driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Mon, Jan 14, 2013 at 07:13:30AM -0800, Jerry Wong wrote:

> I have updated most per the review comments, except...
> "External MIC Mux" is a mux with 0=power down, and exists in DAPM as well
> "Digital Sidetone Source", with a master SHDN has no power impact, so was left out of DAPM

Sidetone should have an impact on routing though?  DAPM needs routing to
do power.

> "Quick Setup System Clock" was just a question what they do... they override the
>  clock settings for common use cases.
> "VCM bandgap reference", with a master SHDN has no power impact, so was left out of DAPM
> 

Please have this sort of discussion in line with the previous review -
discuss why you don't agree with the feedback rather than just sending a
new patch.  That tends to be less time consuming and it's easier to
follow the discusssion.

> +static const char * max98090_extmic_mux_text[] = { "MIC1", "MIC2" };
> +
> +static const struct soc_enum max98090_extmic_mux_enum =
> +       SOC_ENUM_SINGLE(M98090_REG_CFG_LINE, M98090_EXTMIC_SHIFT,
> +               ARRAY_SIZE(max98090_extmic_mux_text),
> +               max98090_extmic_mux_text);

This looks like routing control which I'd expect to be in DAPM - I'd not
expect duplication between DAPM and non-DAPM to work very well.

> +static const char * max98090_mixg_text[] = { "Normal", "6dB" };

> +static const struct soc_enum max98090_mixg135_enum =
> +       SOC_ENUM_SINGLE(M98090_REG_LVL_LINE, M98090_MIXG135_SHIFT,
> +               ARRAY_SIZE(max98090_mixg_text), max98090_mixg_text);
> +
> +static const struct soc_enum max98090_mixg246_enum =
> +       SOC_ENUM_SINGLE(M98090_REG_LVL_LINE, M98090_MIXT246_SHIFT,
> +               ARRAY_SIZE(max98090_mixg_text), max98090_mixg_text);

This looks like it should be a volume control with two steps of 0dB and
6dB?

> +static const char * max98090_enableddisabled_text[] =
> +       { "Disabled", "Enabled" };
> +static const char * max98090_enableddisabled_inv_text[] =
> +       { "Enabled", "Disabled" };
> +/* Note: Inverted Logic (0 = Enabled) */
> +
> +static const struct soc_enum max98090_eqclpn_enum =
> +       SOC_ENUM_SINGLE(M98090_REG_DAI_LVL_EQ, M98090_DAI_LVL_EQCLPN_SHIFT,
> +               ARRAY_SIZE(max98090_enableddisabled_inv_text),
> +               max98090_enableddisabled_inv_text);

These look like they should be Switch controls (which will help UIs
render them).

> +static const struct snd_kcontrol_new max98090_snd_controls[] = {
> +       SOC_SINGLE("MIC Bias VCM Bandgap", M98090_REG_BIAS_CNTL,
> +               M98090_VCM_MODE_SHIFT, M98090_VCM_MODE_NUM - 1, 0),

Why is this a user visible control?

> +       SOC_SINGLE("Quick Setup Sample Rate", M98090_REG_QCFG_RATE,
> +               M98090_SR_ALL_SHIFT, M98090_SR_ALL_NUM - 1, 0),
> +       SOC_SINGLE("Quick Setup DAI Interface", M98090_REG_QCFG_DAI,
> +               M98090_DAI_ALL_SHIFT, M98090_DAI_ALL_NUM - 1, 0),
> +       SOC_SINGLE("Quick Setup DAC Path", M98090_REG_QCFG_DAC,
> +               M98090_DIG2_ALL_SHIFT, M98090_DIG2_ALL_NUM - 1, 0),
> +       SOC_SINGLE("Quick Setup MIC/Direct ADC", M98090_REG_QCFG_MIC_PATH,
> +               M98090_MIC_ALL_SHIFT, M98090_MIC_ALL_NUM - 1, 0),
> +       SOC_SINGLE("Quick Setup Line ADC", M98090_REG_QCFG_LINE_PATH,
> +               M98090_LINE_ALL_SHIFT, M98090_LINE_ALL_NUM - 1, 0),
> +       SOC_SINGLE("Quick Setup Analog MIC Loop", M98090_REG_QCFG_MIC_LOOP,
> +               M98090_AMIC_ALL_SHIFT, M98090_AMIC_ALL_NUM - 1, 0),
> +       SOC_SINGLE("Quick Setup Analog Line Loop",
> +               M98090_REG_QCFG_LINE_LOOP, M98090_ALIN_ALL_SHIFT,
> +               M98090_ALIN_ALL_NUM - 1, 0),

What do these controls do?

> +       SOC_SINGLE("Rev ID", M98090_REG_REV_ID,
> +               M98090_REVID_SHIFT, M98090_REVID_NUM - 1, 0),

Hrm, this needs to be read only I think but we don't support that via
standard macros.  I'd expect exposing via a sysfs file rather than an
ALSA control, or just logging on startup - it'll be exposed via the
regmap debugfs interface anyway.

> +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"},

Is this really an internal connection in the device and not something
that's done in the system?

> +/*
> + * 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, your comment is misaligned.

> +
> +       switch (reg & (M98090_LSNS_MASK | M98090_JKSNS_MASK)) {
> +               case M98090_LSNS_MASK | M98090_JKSNS_MASK:
> +               {

You don't need the { } and they make the code look unusual.

> +                       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);

The issues I mentioned last time with the lack of sync between this code
and DAPM still apply - DAPM is likely to just come along and override
you here.

> +       if (active & M98090_IRQ_ULK_MASK) {
> +               dev_dbg(codec->dev, "M98090_IRQ_ULK_MASK\n");
> +       }

Should things like this be dev_err() - they look like real error
reports from the device?

> +                                const struct i2c_device_id *id)
> +{
> +       struct max98090_priv *max98090;
> +       int ret;
> +
> +       pr_info("max98090_i2c_probe\n");

No need for log messages like this - dev_dbg() at most.

> +       max98090 = kzalloc(sizeof(struct max98090_priv), GFP_KERNEL);
> +       if (max98090 == NULL)
> +               return -ENOMEM;

devm_kzalloc() then you don't need to explicitly free() which will fix
leaks on error.

> +static int max98090_runtime_resume(struct device *dev)
> +{
> +       struct max98090_priv *max98090 = dev_get_drvdata(dev);
> +
> +       regcache_cache_only(max98090->regmap, false);
> +
> +       max98090_reset(max98090);
> +
> +       regcache_sync(max98090->regmap);

Do you realy need to reset the device here?  If anything I'd expect it
on suspend since the reset state is probably the lowest power state.