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

[rajeev]

Hi Mark
Please find my answer inlined

On 3/30/2011 4:57 AM, Mark Brown wrote:
> 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.
>
ok
 
> 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?

master control provide a separate control.

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

> 
>> +	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.
> 
OK,Macro name in in-correct. 

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


>> +MODULE_DESCRIPTION("ASoC STA529 codec driver");
>> +MODULE_AUTHOR("Rajeev Kumar <rajeev-dlh.kumar at st.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:sta529-codec");
> 
> This isn't a platform driver, remove the MODULE_ALIAS.
> .
Ooops, 
>
Best Regards
Rajeev