[alsa-devel] [PATCH 002/002] ASoC: Replace max98090 Device Driver

[Jerry Wong]

Mark,

I was able to make changes for each of your comments except the following...

Jerry

-----

> +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.

This is an odd control that is a combination of an enum and a power
management widget. The 0 value of the enum  turns power off.
Not sure what is another choice. The enum value needs to be
saved so it can be restored when DAPM turns the control on.

> +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?

It is not controlled by DAPM. The low power function is taken care of by
/SHDN\, so this retains the user choice of bandgap or divider reference.

> +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 really need to reset the device here?  If anything I'd expect it
> on suspend since the reset state is probably the lowest power state.

Yes, for a soft reset where the device is not power cycled, DAPM has
the device go in and out of the low power /SHDN\ state, and the old
register values are retained. Without this, we would need to specify
that the PMIC power cycle the chip.