[alsa-devel] [PATCH] ASoC: codecs: Add DA732x codec driver

[Software Maintainer]

On Wed, 23 May, 2012 at 12:15, Mark Brown wrote:
> 
> You only ever enable this and it seems like it should be a widget - why
> is it not a widget?
> 

Makes sense. Will change this.

> This should be a switch not an enum.

Agreed.

> 
> > +static int da732x_hpf_set(struct snd_kcontrol *kcontrol,
> > +			  struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> > +	struct soc_enum *enum_ctrl = (struct soc_enum *)kcontrol->private_value;
> > +	unsigned int reg = enum_ctrl->reg;
> > +	unsigned int sel = ucontrol->value.integer.value[0];
> > +	unsigned int bits;
> > +
> > +	switch (sel) {
> > +	case DA732X_HPF_DISABLED:
> > +		bits = DA732X_HPF_DIS;
> > +		break;
> > +	case DA732X_HPF_VOICE:
> > +		bits = DA732X_HPF_VOICE_EN;
> > +		break;
> > +	case DA732X_HPF_MUSIC:
> > +		bits = DA732X_HPF_MUSIC_EN;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	snd_soc_update_bits(codec, reg, DA732X_HPF_MASK, bits);
> 
> Why is this not done using a normal enum?

> Similarly here, and this looks like a switch statement.

Agreed.

> This should be generic code, there's nothing device specific about this
> control type and it'd be useful for other devices.

Will look at this and see if we can create something generic.

> Just MIC1 Switch.

Agreed.

> All these should be Volumes, and really they should be stereo controls.

Agreed.

> Volume needs to be the last thing in the name.

Ok, will change this.

> This looks like you perhaps need some supply widgets.  Similarly for
> most of your events.

Ok, will update.

> Don't use MICBIAS widgets for new code, use supply widgets.

Will change it.

> The MICBIASes should be connected on the board.

Will look at this.

> Use dai->base.

Will change.

> Use snd_soc_update_bits, this will fix the issue where you don't
> currently clear the register.  You're also missing error handling in the
> default: case.

Agreed. Will change this.

> Don't make up private defines for generic things like this.  Though
> really a define like this is a bit silly...

Fair point, will update.

> This should either stop and restart the PLL or return an error if it's
> already enabled.

Will change this accordingly.
 
> This doesn't validate against the sysclk rate...

> Pass back the error code.
 
> This means there's no way to stop the PLL.

Will do some rework here to improve this area of the code.

> This appears to mute all the DACs but you have two audio interfaces and
> also had some other code which also managed the mutes...  If there's no
> mutes on the actual audio interfaces just don't implement this.

Will look at this. Some rework required.
 
> SNDRV_PCM_RATE_8000_48000

Will change to use SNDRV_PCM_RATE_8000_96000.

> Almost all of this sequence looks like it should be in set_bias_level(),
> it'll be needed every time the chip is powered up and sequencing looks
> important.

Makes sense. Will rearrange the code for this.

> Shouldn't this be a supply, or again in set_bias_level()?

Again, will look at this.

> Should be dynamically managed.

Will update.

> devm_regmap_init_i2c().

Yes, this is nicer. Will change.

> Remove the i2c, devices connected by I2C are generally i2c devices...
> 
> > +		.name	= "da732x-i2c",

Ok, no problem. Will do that.
 
> module_i2c_driver().

Ok, that's neater. Will change.

> Really not sure how helpful I found any of these reading the code.

Ok will remove.

Thanks

Adam