Re: [PATCH 1/4] ASoC: sn95031: add capture support

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

On Wed, Jan 19, 2011 at 06:16:09PM +0530, Harsha Priya wrote:

> +static int amic1_mic_bias(struct snd_soc_dapm_widget *w,
> +				struct snd_kcontrol *k, int event)
> +{
> +	unsigned int value = 0;
> +
> +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> +		pr_debug("AMIC1 SND_SOC_DAPM_EVENT_ON doing\n");
> +		value = BIT(2);
> +	}
> +	snd_soc_update_bits(w->codec, SN95031_MICBIAS, BIT(2), value);
> +	return 0;
> +}

I'm not clear why this is being done using an event?  It's updating a
single register bit which is what the standard MICBIAS widget does.
There's also another example of debug logging which replicates the
standard feature facilities.

> +static int dmic12_mic_bias(struct snd_soc_dapm_widget *w,
> +				struct snd_kcontrol *k, int event)
> +{
> +	unsigned int ldo = 0, clk = 0, out = 0;
> +
> +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> +		pr_debug("DMIC12 SND_SOC_DAPM_EVENT_ON doing\n");
> +		ldo = BIT(5)|BIT(4);
> +		clk = BIT(0);
> +		out = BIT(3);
> +	}
> +	/* program DMIC LDO */
> +	snd_soc_update_bits(w->codec, SN95031_MICBIAS, BIT(5)|BIT(4), ldo);
> +	msleep(1);
> +	/* enable/disable DMIC clock and o/p */
> +	snd_soc_update_bits(w->codec, SN95031_DMICLK, BIT(0), clk);
> +	snd_soc_update_bits(w->codec, SN95031_DMICMUX, BIT(3), out);

How about using a supply widget for the LDO and the clock?  This would
also ensure that...

> +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> +		pr_debug("DMIC34 SND_SOC_DAPM_EVENT_ON doing\n");
> +		ldo = BIT(5)|BIT(4);
> +		clk = BIT(1);
> +		out = BIT(4);

...we don't end up disabling them when one of the DMICs is shut off but
the other is left on.

> +	SND_SOC_DAPM_MIC("AMIC1", amic1_mic_bias), /* headset mic */
> +	SND_SOC_DAPM_MIC("AMIC2", amic2_mic_bias),
> +	SND_SOC_DAPM_MIC("DMIC1", dmic12_mic_bias),
> +	SND_SOC_DAPM_MIC("DMIC2", dmic12_mic_bias),
> +	SND_SOC_DAPM_MIC("DMIC3", dmic34_mic_bias),
> +	SND_SOC_DAPM_MIC("DMIC4", dmic34_mic_bias),
> +	SND_SOC_DAPM_MIC("DMIC5", dmic56_mic_bias),
> +	SND_SOC_DAPM_MIC("DMIC6", dmic56_mic_bias),

These widgets are for use externally to the device in machine drivers.
The pins on the CODEC should be _INPUT() widgets and the biases should
be MICBIAS widgets.

> +	SND_SOC_DAPM_REG(snd_soc_dapm_micbias, "MIC1 Enable", SN95031_MICAMP1,
> +			0, 1, 1, 0),
> +	SND_SOC_DAPM_REG(snd_soc_dapm_micbias, "MIC2 Enable", SN95031_MICAMP2,
> +			0, 1, 1, 0),

These look awfully like they should just be normal PGA widgets?