[alsa-devel] [PATCH v2 1/4] ASoC: tlv320aic31xx: Add basic codec driver implementation

[Mark Brown]

On Tue, Mar 04, 2014 at 03:54:49PM +0200, Jyri Sarha wrote:

> +- ai31xx-micbias-vg - MicBias Voltage required.
> +        MICBIAS_OFF - MICBIAS output it not powered

Same comment as last time - why is this something which can be selected
in the binding?

> +        MICBIAS_2_0V - MICBIAS output is powered to 2.0V
> +        MICBIAS_2_5V - MICBIAS output is powered to 2.5V
> +        MICBIAS_AVDD - MICBIAS output is connected to AVDD

The numbers still need to be defined in the binding, the point with the
defines is to make both code and DTs more readable but we need to know
what is actually going to go into the binary.

> +	/* Mic Bias */
> +	SND_SOC_DAPM_SUPPLY("Mic Bias", SND_SOC_NOPM, 0, 0, mic_bias_event,
> +			    SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),

The idiomatic thing would be to use the pin name.

> +	/* Make ADC turn on when recording even if not mixed from any inputs */
> +	{"ADC", NULL, "Internal ADC Source"},

Don't do this (or the equivalent from the DACs) - we don't do this for
any other drivers, we shouldn't do it for this one.  If we're going to
do something like this it should be generic not per driver hacks.

> +	case SND_SOC_BIAS_STANDBY:
> +		if (codec->dapm.bias_level == SND_SOC_BIAS_OFF)
> +			aic31xx_set_power(codec, 1);
> +		break;
> +	case SND_SOC_BIAS_OFF:
> +		aic31xx_set_power(codec, 0);
> +		break;

Just inline the set power function, or at the very least split it into
separate on and off functions - there is zero shared code between the
power on and off paths.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140305/f8eb5e70/attachment.sig>