Re: [PATCH V2 1/5] sound: asoc: Adding support for STA529 Audio Codec

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

On Tue, Mar 29, 2011 at 04:46:59PM +0530, Rajeev Kumar wrote:

> +static const struct snd_kcontrol_new sta529_snd_controls[] = {
> +	SOC_SINGLE("Master Playback Volume", STA529_MVOL, 0, 127, 1),
> +	SOC_SINGLE("Left Playback Volume", STA529_LVOL, 0, 127, 1),
> +	SOC_SINGLE("Right Playback Volume", STA529_RVOL, 0, 127, 1),
> +};

Left and Right should be a single stereo control (usually done with
SOC_DOUBLE).  It'd also be better to provide gain information with the
_TLV variants of the controls.

Does the master control actually provide a separate control or does it
update both left and right channels simultaneously?  If the latter then
normally you'd just omit it from the driver as the driver can easily do
the stereo updates?

> +	val = snd_soc_read(codec, STA529_S2PCFG0);
> +	val |= mode;
> +	/*this setting will be used with actual h/w */
> +	snd_soc_write(codec, STA529_S2PCFG0, val);

snd_soc_update_bits() - you're not clearing any bits here so if you
change modes things will go wrong.

> +	case SND_SOC_BIAS_STANDBY:
> +	case SND_SOC_BIAS_OFF:
> +		snd_soc_update_bits(codec, STA529_FFXCFG0, POWER_CNTLMSAK,
> +				POWER_STDBY);
> +		snd_soc_update_bits(codec, STA529_FFXCFG0, MUTE_ON_MASK,
> +				MUTE_OFF);

Managing the mute in these states seems odd - could do with some
comments in the code if nothing else.

> +static void sta529_init(struct snd_soc_codec *codec)
> +{
> +	/* DAC default master volume */
> +	snd_soc_write(codec, STA529_MVOL, DEFAULT_MASTER_VOL);
> +	/* DAC default left volume */
> +	snd_soc_write(codec, STA529_LVOL, DEFAULT_VOL);
> +	/* DAC default right volume */
> +	snd_soc_write(codec, STA529_RVOL, DEFAULT_VOL);
> +	/* By default route to binary headphones */
> +	snd_soc_write(codec, STA529_FFXCFG1, DEFAULT_BIN_HEADPHONE);
> +	/* default value for Serial-to-parallel audio interface configuration */

> +	/* select microphone input by default*/
> +	snd_soc_write(codec, STA529_ADCCFG, DEFAULT_MIC_SEL);

None of the above should be configured here, leave them at the default
values (and offer them as runtime controls if they're not there
already).  The settings appropriate for one machine may not be
appropriate for another.

> +static int sta529_resume(struct snd_soc_codec *codec)
> +{
> +
> +	sta529_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> +	sta529_set_bias_level(codec, codec->suspend_bias_level);
> +	return 0;
> +}

I'd expect this to restore the register cache somewhere.

> +MODULE_DESCRIPTION("ASoC STA529 codec driver");
> +MODULE_AUTHOR("Rajeev Kumar <rajeev-dlh.kumar@st.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:sta529-codec");

This isn't a platform driver, remove the MODULE_ALIAS.