On Fri, Feb 25, 2011 at 02:42:47PM +0100, Javier Martin wrote:

+#define aic32x4_reset(c) snd_soc_write(c, AIC32X4_RESET, 0x01);

Make this a proper function.

+	SOC_DOUBLE_R_AIC32X4("HP Driver Gain", AIC32X4_HPLGAIN,
+			AIC32X4_HPRGAIN, 0, 0x23, 0),
+	SOC_DOUBLE_R_AIC32X4("LO Driver Gain", AIC32X4_LOLGAIN,
+			AIC32X4_LORGAIN, 0, 0x23, 0),

These should be Volume controls, ideally with TLV information.

+	SOC_DOUBLE_R("ADC Volume Level", AIC32X4_LADCVOL,
+			AIC32X4_RADCVOL, 0, 0x28, 0),
+	SOC_DOUBLE_R("PGA Gain Level", AIC32X4_LMICPGAVOL,
+			AIC32X4_RMICPGAVOL, 0, 0x5f, 0),

Again, Volume controls.  All gain controls are Volume controls.

+	SOC_SINGLE("Auto-mute", AIC32X4_DACMUTE, 4, 7, 0),

... Switch.

+	SOC_SINGLE("MIC BIAS", AIC32X4_MICBIAS, 6, 1, 0),

This should not be user visible, this should be a DAPM widget.

+static const struct aic32x4_configs aic32x4_reg_init[] = {
+	{AIC32X4_PWRCFG, AIC32X4_AVDDWEAKDISABLE},

What are these "aic32x4_configs" all about?  They look like you're
setting non-default configurations...

+static const struct snd_kcontrol_new hpl_output_mixer_controls[] = {
+	SOC_DAPM_SINGLE("L_DAC switch", AIC32X4_HPLROUTE, 3, 1, 0),
+	SOC_DAPM_SINGLE("IN1_L switch", AIC32X4_HPLROUTE, 2, 1, 0),

Switch (similarly for other DAPM controls).

+	if (codec->hw_write(codec->control_data, data, 2) == 2)
+		return 0;
+	else
+		return -EIO;

Ideally if this returned an error code you'd return that.

+int snd_soc_get_volsw_2r_aic32x4(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	int reg = kcontrol->private_value & 0xff;
+	int reg2 = (kcontrol->private_value >> 24) & 0xff;
+	int mask;
+	int shift;
+	unsigned short val, val2;
+
+	if (!strcmp(kcontrol->id.name, "PCM Playback Volume")) {
+		mask = 0xff;
+		shift = 0;
+	} else if ((!strcmp(kcontrol->id.name, "HP Driver Gain")) ||
+		   (!strcmp(kcontrol->id.name, "LO Driver Gain"))) {
+		mask = 0x3F;
+		shift = 0;
+	} else if (!strcmp(kcontrol->id.name, "PGA Capture Volume")) {
+		mask = 0x7F;
+		shift = 0;
+	} else {
+		printk(KERN_ERR "aic32x4: invalid kcontrol name\n");
+		return -1;
+	}

According to the info callback you're actually passing the mask in
through the private data and the shift is always zero so there's no need
to do these strcmp().

+	if (!strcmp(kcontrol->id.name, "PCM Playback Volume")) {
+		ucontrol->value.integer.value[0] =
+		    (val <= 48) ? (val + 127) : (val - 129);
+		ucontrol->value.integer.value[1] =
+		    (val2 <= 48) ? (val2 + 127) : (val2 - 129);

This looks awfully like the code that was added in the SGTL5000 driver
for volume controls there - this really needs to be factored out into a
standard macro.

+static struct i2c_driver aic32x4_i2c_driver = {
+	.driver = {
+		.name = "tlv320aic32x4-codec",
+		.owner = THIS_MODULE,

Drop the -codec from the driver name, it's redundant.

+{
+	int ret = 0;
+#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
+	ret = i2c_add_driver(&aic32x4_i2c_driver);
+	if (ret != 0) {
+		printk(KERN_ERR "Failed to register aic32x4 I2C driver: %d\n",
+		       ret);
+	}
+#endif

Remove the ifdefs if the driver doesn't support non-I2C buses.