[PATCH 1/1] Add AIC3106 support.

[PATCH 1/1] Add AIC3106 support.

[Ambrose, Martin]

Signed-off-by: Steve Chen <schen@mvista.com>
Signed-off-by: Martin Ambrose <martin@ti.com>
---
 sound/soc/codecs/tlv320aic3x.c |  185 ++++++++++++++++++++++++++++++++++++++-
 sound/soc/codecs/tlv320aic3x.h |   11 ++-
 2 files changed, 190 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index ab099f4..5720cad 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -57,13 +57,17 @@ struct aic3x_priv {
 	int master;
 };
 
+static u8 pg0_end = AIC3X_GEN_PG0_END;
+static u8 pg1_end = AIC3X_GEN_PG1_END;
+static enum aic3x_codec_variant codec_variant;
+
 /*
  * AIC3X register cache
  * We can't read the AIC3X register space when we are
  * using 2 wire for device control, so we cache them instead.
  * There is no point in caching the reset register
  */
-static const u8 aic3x_reg[AIC3X_CACHEREGNUM] = {
+static const u8 aic3x_reg[] = {
 	0x00, 0x00, 0x00, 0x10,	/* 0 */
 	0x04, 0x00, 0x00, 0x00,	/* 4 */
 	0x00, 0x00, 0x00, 0x01,	/* 8 */
@@ -90,6 +94,35 @@ static const u8 aic3x_reg[AIC3X_CACHEREGNUM] = {
 	0x00, 0x00, 0x00, 0x00,	/* 92 */
 	0x00, 0x00, 0x00, 0x00,	/* 96 */
 	0x00, 0x00, 0x02,	/* 100 */
+
+	0x00, 0x00, 0x00, 0x00, /* 103-127 unused */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00,
+
+	0x00, 0x6b, 0xe3, 0x96, /* 0 */
+	0x66, 0x67, 0x5d, 0x6b, /* 4 */
+	0xe3, 0x96, 0x66, 0x67, /* 8 */
+	0x5d, 0x7d, 0x83, 0x84, /* 12 */
+	0xee, 0x7d, 0x83, 0x84, /* 16 */
+	0xee, 0x39, 0x55, 0xf3, /* 20 */
+	0x2d, 0x53, 0x7e, 0x6b, /* 24 */
+	0xe3, 0x96, 0x66, 0x67, /* 28 */
+	0x5d, 0x6b, 0xe3, 0x96, /* 32 */
+	0x66, 0x67, 0x5d, 0x7d, /* 36 */
+	0x83, 0x84, 0xee, 0x7d, /* 40 */
+	0x83, 0x84, 0xee, 0x39, /* 44 */
+	0x55, 0xf3, 0x2d, 0x53, /* 48 */
+	0x7e, 0x7f, 0xff, 0x00, /* 52 */
+	0x00, 0x00, 0x00, 0x00, /* 56 */
+	0x00, 0x00, 0x00, 0x00, /* 60 */
+	0x00, 0x39, 0x55, 0xf3, /* 64 */
+	0x2d, 0x53, 0x7e, 0x39, /* 68 */
+	0x55, 0xf3, 0x2d, 0x53, /* 72 */
+	0x7e,			/* 76 */
 };
 
 /*
@@ -99,7 +132,8 @@ static inline unsigned int aic3x_read_reg_cache(struct snd_soc_codec *codec,
 						unsigned int reg)
 {
 	u8 *cache = codec->reg_cache;
-	if (reg >= AIC3X_CACHEREGNUM)
+	if ((reg >= pg1_end) ||
+		((reg >= pg0_end) && (reg < AIC3X_GEN_PG1_BEG)))
 		return -1;
 	return cache[reg];
 }
@@ -111,7 +145,8 @@ static inline void aic3x_write_reg_cache(struct snd_soc_codec *codec,
 					 u8 reg, u8 value)
 {
 	u8 *cache = codec->reg_cache;
-	if (reg >= AIC3X_CACHEREGNUM)
+	if ((reg >= pg1_end) ||
+		((reg >= pg0_end) && (reg < AIC3X_GEN_PG1_BEG)))
 		return;
 	cache[reg] = value;
 }
@@ -119,8 +154,8 @@ static inline void aic3x_write_reg_cache(struct snd_soc_codec *codec,
 /*
  * write to the aic3x register space
  */
-static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
-		       unsigned int value)
+static int _aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
+			unsigned int value)
 {
 	u8 data[2];
 
@@ -132,12 +167,41 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 	data[1] = value & 0xff;
 
 	aic3x_write_reg_cache(codec, data[0], data[1]);
+
+	/* adjust for page 1 before updating hardware if necessary */
+	if (data[0] >= AIC3X_GEN_PG1_BEG)
+		data[0] -= AIC3X_GEN_PG1_BEG;
+
 	if (codec->hw_write(codec->control_data, data, 2) == 2)
 		return 0;
 	else
 		return -EIO;
 }
 
+static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
+		       unsigned int value)
+{
+	u8 cur_pg;
+	u8 reg_pg;
+	int ret = 0;
+
+	cur_pg = aic3x_read_reg_cache(codec, 0);
+	if (reg < pg0_end)
+		reg_pg = 0;
+	else if ((reg >= AIC3X_GEN_PG1_BEG) && (reg < pg1_end))
+		reg_pg = 1;
+	else
+		return -EIO;
+
+	if (cur_pg != reg_pg)
+		ret = _aic3x_write(codec, 0, reg_pg);
+
+	if (ret == 0)
+		ret = _aic3x_write(codec, reg, value);
+
+	return ret;
+}
+
 /*
  * read from the aic3x register space
  */
@@ -218,6 +282,65 @@ static int snd_soc_dapm_put_volsw_aic3x(struct snd_kcontrol *kcontrol,
 	return ret;
 }
 
+static int tlv320aic3x_dual_reg_info(struct snd_kcontrol *kcontrol,
+				     struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 0xffff;
+	return 0;
+}
+
+static int tlv320aic3x_dual_reg_get(struct snd_kcontrol *kcontrol,
+				    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	int reg_msb = kcontrol->private_value & 0xff;
+	int reg_lsb = (kcontrol->private_value >> 8) & 0xff;
+	int val = aic3x_read_reg_cache(codec, reg_msb) << 8;
+
+	val |= aic3x_read_reg_cache(codec, reg_lsb);
+
+	/* convert 2's complement to unsigned int */
+	val ^= 0x8000;
+
+	ucontrol->value.integer.value[0] = val;
+
+	return 0;
+}
+
+static int tlv320aic3x_dual_reg_put(struct snd_kcontrol *kcontrol,
+				    struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+	int err;
+	int reg_msb = kcontrol->private_value & 0xff;
+	int reg_lsb = (kcontrol->private_value >> 8) & 0xff;
+	int val_msb, val_lsb;
+
+	val_msb = (ucontrol->value.integer.value[0] >> 8) & 0xff;
+	val_lsb = ucontrol->value.integer.value[0] & 0xff;
+
+	/* convert unsigned int to 2's complement */
+	val_msb ^= 0x80;
+
+	err = snd_soc_update_bits(codec, reg_msb, 0xff, val_msb);
+	if (err < 0)
+		return err;
+	err = snd_soc_update_bits(codec, reg_lsb, 0xff, val_lsb);
+	return err;
+}
+
+#define TLV320AIC3X_DUAL_R(xname, page, reg_msb, reg_lsb) \
+{      .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
+	.info = tlv320aic3x_dual_reg_info, \
+	.get = tlv320aic3x_dual_reg_get, .put = tlv320aic3x_dual_reg_put, \
+	.private_value = ((reg_msb) + page) | (((reg_lsb) + page) << 8) }
+
+#define TLV320AIC3X_PG1_DUAL_R(xname, reg_msb, reg_lsb) \
+	TLV320AIC3X_DUAL_R(xname, AIC3X_GEN_PG1_BEG, reg_msb, reg_lsb)
+
 static const char *aic3x_left_dac_mux[] = { "DAC_L1", "DAC_L3", "DAC_L2" };
 static const char *aic3x_right_dac_mux[] = { "DAC_R1", "DAC_R3", "DAC_R2" };
 static const char *aic3x_left_hpcom_mux[] =
@@ -344,6 +467,36 @@ static const struct snd_kcontrol_new aic3x_snd_controls[] = {
 	SOC_DOUBLE_R("PGA Capture Switch", LADC_VOL, RADC_VOL, 7, 0x01, 1),
 
 	SOC_ENUM("ADC HPF Cut-off", aic3x_enum[ADC_HPF_ENUM]),
+
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient N0", 1, 2),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient N1", 3, 4),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient N2", 5, 6),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient N3", 7, 8),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient N4", 9, 10),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient N5", 11, 12),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient D1", 13, 14),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient D2", 15, 16),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient D4", 17, 18),
+	TLV320AIC3X_PG1_DUAL_R("Left Effects Coefficient D5", 19, 20),
+	TLV320AIC3X_PG1_DUAL_R("Left De-Emphasis Coefficient N0", 21, 22),
+	TLV320AIC3X_PG1_DUAL_R("Left De-Emphasis Coefficient N1", 23, 24),
+	TLV320AIC3X_PG1_DUAL_R("Left De-Emphasis Coefficient D1", 25, 26),
+
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient N0", 27, 28),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient N1", 29, 30),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient N2", 31, 32),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient N3", 33, 34),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient N4", 35, 36),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient N5", 37, 38),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient D1", 39, 40),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient D2", 41, 42),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient D4", 43, 44),
+	TLV320AIC3X_PG1_DUAL_R("Right Effects Coefficient D5", 45, 46),
+	TLV320AIC3X_PG1_DUAL_R("Right De-Emphasis Coefficient N0", 47, 48),
+	TLV320AIC3X_PG1_DUAL_R("Right De-Emphasis Coefficient N1", 49, 50),
+	TLV320AIC3X_PG1_DUAL_R("Right De-Emphasis Coefficient D1", 51, 52),
+
+	TLV320AIC3X_PG1_DUAL_R("3-D Attenuation Coefficient", 53, 54),
 };
 
 /* Left DAC Mux */
@@ -454,6 +607,18 @@ static const struct snd_kcontrol_new aic3x_right_line2_bp_mixer_controls[] = {
 	SOC_DAPM_SINGLE("HPRCOM Switch", LINE2R_2_HPRCOM_VOL, 7, 1, 0),
 };
 
+static const struct snd_kcontrol_new aic3106_snd_controls[] = {
+	TLV320AIC3X_PG1_DUAL_R("Left Capture High Pass Coefficient N0", 65, 66),
+	TLV320AIC3X_PG1_DUAL_R("Left Capture High Pass Coefficient N1", 67, 68),
+	TLV320AIC3X_PG1_DUAL_R("Left Capture High Pass Coefficient D1", 69, 70),
+	TLV320AIC3X_PG1_DUAL_R("Right Capture High Pass Coefficient N0",
+			     71, 72),
+	TLV320AIC3X_PG1_DUAL_R("Right Capture High Pass Coefficient N1",
+			     73, 74),
+	TLV320AIC3X_PG1_DUAL_R("Right Capture High Pass Coefficient D1",
+			     75, 76),
+};
+
 static const struct snd_soc_dapm_widget aic3x_dapm_widgets[] = {
 	/* Left DAC to Left Outputs */
 	SND_SOC_DAPM_DAC("Left DAC", "Left Playback", DAC_PWR, 7, 0),
@@ -1165,6 +1330,10 @@ static int aic3x_init(struct snd_soc_device *socdev)
 	if (codec->reg_cache == NULL)
 		return -ENOMEM;
 
+	/* setup register cache sizes */
+	if (codec_variant == AIC3106_CODEC)
+		pg1_end = AIC3106_PG1_END;
+
 	aic3x_write(codec, AIC3X_PAGE_SELECT, PAGE0_SELECT);
 	aic3x_write(codec, AIC3X_RESET, SOFT_RESET);
 
@@ -1247,6 +1416,11 @@ static int aic3x_init(struct snd_soc_device *socdev)
 
 	snd_soc_add_controls(codec, aic3x_snd_controls,
 				ARRAY_SIZE(aic3x_snd_controls));
+
+	if (codec_variant == AIC3106_CODEC)
+		snd_soc_add_controls(codec, aic3106_snd_controls,
+				ARRAY_SIZE(aic3106_snd_controls));
+
 	aic3x_add_widgets(codec);
 	ret = snd_soc_init_card(socdev);
 	if (ret < 0) {
@@ -1374,6 +1548,7 @@ static int aic3x_probe(struct platform_device *pdev)
 	printk(KERN_INFO "AIC3X Audio Codec %s\n", AIC3X_VERSION);
 
 	setup = socdev->codec_data;
+	codec_variant = setup->variant;
 	codec = kzalloc(sizeof(struct snd_soc_codec), GFP_KERNEL);
 	if (codec == NULL)
 		return -ENOMEM;
diff --git a/sound/soc/codecs/tlv320aic3x.h b/sound/soc/codecs/tlv320aic3x.h
index ac827e5..ee40e9c 100644
--- a/sound/soc/codecs/tlv320aic3x.h
+++ b/sound/soc/codecs/tlv320aic3x.h
@@ -13,7 +13,10 @@
 #define _AIC3X_H
 
 /* AIC3X register space */
-#define AIC3X_CACHEREGNUM		103
+#define AIC3X_GEN_PG0_END		103
+#define AIC3X_GEN_PG1_BEG		128
+#define AIC3X_GEN_PG1_END		183
+#define AIC3106_PG1_END 		205
 
 /* Page select register */
 #define AIC3X_PAGE_SELECT		0
@@ -199,6 +202,11 @@
 /* Default input volume */
 #define DEFAULT_GAIN    0x20
 
+enum aic3x_codec_variant {
+	AIC3X_GENERIC_CODEC,
+	AIC3106_CODEC,
+};
+
 /* GPIO API */
 enum {
 	AIC3X_GPIO1_FUNC_DISABLED		= 0,
@@ -284,6 +292,7 @@ int aic3x_button_pressed(struct snd_soc_codec *codec);
 struct aic3x_setup_data {
 	int i2c_bus;
 	unsigned short i2c_address;
+	enum aic3x_codec_variant variant;
 	unsigned int gpio_func[2];
 };
 
-- 
1.6.0.6

Re: [PATCH 1/1] Add AIC3106 support.

[Mark Brown]

On Wed, May 13, 2009 at 10:28:23AM -0500, Ambrose, Martin wrote:

> Signed-off-by: Steve Chen <schen@mvista.com>
> Signed-off-by: Martin Ambrose <martin@ti.com>

You should really provide a changelog explaining what's going on with
the chip here, especially with regard to the effect on the other devices
that are supported.

Please also always try to remember to CC the maintainers for things on
patch submissions.

> +static enum aic3x_codec_variant codec_variant;

This shouldn't be static data, it should be stored in the private data
for the codec in order to account for future support for multiple codecs.

> -static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
> -		       unsigned int value)
> +static int _aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
> +			unsigned int value)

Please come up with a better name for this.

> +
> +	/* adjust for page 1 before updating hardware if necessary */
> +	if (data[0] >= AIC3X_GEN_PG1_BEG)
> +		data[0] -= AIC3X_GEN_PG1_BEG;
> +

You really need some comments explaining what's going on with the
register paging stuff here.

> +	cur_pg = aic3x_read_reg_cache(codec, 0);
> +	if (reg < pg0_end)
> +		reg_pg = 0;
> +	else if ((reg >= AIC3X_GEN_PG1_BEG) && (reg < pg1_end))
> +		reg_pg = 1;
> +	else
> +		return -EIO;

Should be a different return code, probably -EINVAL or something.

> +static int tlv320aic3x_dual_reg_info(struct snd_kcontrol *kcontrol,
> +				     struct snd_ctl_elem_info *uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +	uinfo->count = 1;
> +	uinfo->value.integer.min = 0;
> +	uinfo->value.integer.max = 0xffff;
> +	return 0;
> +}

Is this true for all of these registers?

> +static int tlv320aic3x_dual_reg_put(struct snd_kcontrol *kcontrol,
> +				    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	int err;
> +	int reg_msb = kcontrol->private_value & 0xff;
> +	int reg_lsb = (kcontrol->private_value >> 8) & 0xff;
> +	int val_msb, val_lsb;
> +
> +	val_msb = (ucontrol->value.integer.value[0] >> 8) & 0xff;
> +	val_lsb = ucontrol->value.integer.value[0] & 0xff;
> +
> +	/* convert unsigned int to 2's complement */
> +	val_msb ^= 0x80;
> +
> +	err = snd_soc_update_bits(codec, reg_msb, 0xff, val_msb);
> +	if (err < 0)
> +		return err;

Just do the register write?

> +#define TLV320AIC3X_DUAL_R(xname, page, reg_msb, reg_lsb) \
> +{      .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
> +	.info = tlv320aic3x_dual_reg_info, \
> +	.get = tlv320aic3x_dual_reg_get, .put = tlv320aic3x_dual_reg_put, \
> +	.private_value = ((reg_msb) + page) | (((reg_lsb) + page) << 8) }
> +
> +#define TLV320AIC3X_PG1_DUAL_R(xname, reg_msb, reg_lsb) \
> +	TLV320AIC3X_DUAL_R(xname, AIC3X_GEN_PG1_BEG, reg_msb, reg_lsb)

Given that your register write code hides the register pages is there
any need for this to know anything about them?

> +	/* setup register cache sizes */
> +	if (codec_variant == AIC3106_CODEC)
> +		pg1_end = AIC3106_PG1_END;
> +

This doesn't feel very well joined up with the way you've supported this
in the register cache - I'd expect the register code to know about the
maximum register address and be able to check that attempts aren't being
made to use the second page on the less complex codecs.

It'd also be better to write all of these as switch statements so that
any further devices can be supported more readily.

> @@ -284,6 +292,7 @@ int aic3x_button_pressed(struct snd_soc_codec *codec);
>  struct aic3x_setup_data {
>  	int i2c_bus;
>  	unsigned short i2c_address;
> +	enum aic3x_codec_variant variant;
>  	unsigned int gpio_func[2];
>  };

It'd be nice to convert the driver to use the standard I2C probing
stuff, though this is not essential to the patch.

Re: [PATCH 1/1] Add AIC3106 support.

[Ambrose, Martin]

On Wed, May 13, 2009 at 11:18:30, Mark Brown wrote:

Thanks for the review comments -- sorry for the delay in responding.

 
> > Signed-off-by: Steve Chen <schen@mvista.com>
> > Signed-off-by: Martin Ambrose <martin@ti.com>
> 
> You should really provide a changelog explaining what's going on with
> the chip here, especially with regard to the effect on the 
> other devices
> that are supported.

This is an area I'm struggling with a bit and could use your advice.

My primary intent is to enable all AIC3106 functionality since my current 
focus is on platforms which use this codec. My first patch set, which 
admittedly was basically just a diff with the MV5 tree, added more than just 
AIC3106 specifics. It added controls for effects, de-emphasis, and 
attenuation coefficients which are common between the AIC33 and AIC3106 and 
lie in the page 1 register set. To digress, I'm curious why these don't 
already exist for the AIC33 since the source file indicates " It supports 
full aic33 codec functionality." 

In addition the patch provided controls for those page 1 registers (high 
pass coefficients) which are on the AIC3106 but not the AIC33. But did not 
add controls for the new AIC3106 page 0 registers. So, to get to my point, I 
propose three sets of patches. The first would add page 1 register support 
to AIC33 then a follow on patch would add the AIC3106 high pass coeff 
support. The concept/variable of a codec variant would only be introduced in 
the second patch set. A third patch, further out in time when I understand 
better ALSA AGC and power management, would add support for the new page 0 
registers.

Regarding the firs patch set, one concern I have is that this file is also 
compatible with AIC31 and AIC32. The addition of controls without 
qualification would result in these appearing in amixer but not actually 
present. On the other hand this seems to be the general strategy of this 
file. For example the AIC32 does not have register 73 but the 
LINE2L_2_MONOLOPM_VOL control is added for all devices since it is on the 
AIC33. Of course everything could be restructured to be more accurate on a 
device by device basis but this seems too severe and far reaching.

  
> Please also always try to remember to CC the maintainers for things on
> patch submissions.

Ack.


> > +static enum aic3x_codec_variant codec_variant;
> 
> This shouldn't be static data, it should be stored in the private data
> for the codec in order to account for future support for 
> multiple codecs.

Ack.

 
> > -static int aic3x_write(struct snd_soc_codec *codec, 
> unsigned int reg,
> > -		       unsigned int value)
> > +static int _aic3x_write(struct snd_soc_codec *codec, 
> unsigned int reg,
> > +			unsigned int value)
> 
> Please come up with a better name for this.

I may subsume the new function into the old. However I do like the convention of leading underscore (_) to represent local/private helper functions. I guess this is frowned upon in the kernel source.


> > +
> > +	/* adjust for page 1 before updating hardware if necessary */
> > +	if (data[0] >= AIC3X_GEN_PG1_BEG)
> > +		data[0] -= AIC3X_GEN_PG1_BEG;
> > +
> 
> You really need some comments explaining what's going on with the
> register paging stuff here.
>
> > +	cur_pg = aic3x_read_reg_cache(codec, 0);
> > +	if (reg < pg0_end)
> > +		reg_pg = 0;
> > +	else if ((reg >= AIC3X_GEN_PG1_BEG) && (reg < pg1_end))
> > +		reg_pg = 1;
> > +	else
> > +		return -EIO;
>
> Should be a different return code, probably -EINVAL or something.

Ack.
 
> > +static int tlv320aic3x_dual_reg_info(struct snd_kcontrol *kcontrol,
> > +				     struct snd_ctl_elem_info *uinfo)
> > +{
> > +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> > +	uinfo->count = 1;
> > +	uinfo->value.integer.min = 0;
> > +	uinfo->value.integer.max = 0xffff;
> > +	return 0;
> > +}
> 
> Is this true for all of these registers?

I believe so but will double check.

 
> > +static int tlv320aic3x_dual_reg_put(struct snd_kcontrol *kcontrol,
> > +				    struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> > +	int err;
> > +	int reg_msb = kcontrol->private_value & 0xff;
> > +	int reg_lsb = (kcontrol->private_value >> 8) & 0xff;
> > +	int val_msb, val_lsb;
> > +
> > +	val_msb = (ucontrol->value.integer.value[0] >> 8) & 0xff;
> > +	val_lsb = ucontrol->value.integer.value[0] & 0xff;
> > +
> > +	/* convert unsigned int to 2's complement */
> > +	val_msb ^= 0x80;
> > +
> > +	err = snd_soc_update_bits(codec, reg_msb, 0xff, val_msb);
> > +	if (err < 0)
> > +		return err;
> 
> Just do the register write?

Steve's response but I need to also review:
-- Hardware values are in 2's complement and software values are in
unsigned int.  When a value of '0' is passed in, it translated to the
smallest possible value that should be written to hardware (which is
0x80000000).

 
> > +#define TLV320AIC3X_DUAL_R(xname, page, reg_msb, reg_lsb) \
> > +{      .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
> > +	.info = tlv320aic3x_dual_reg_info, \
> > +	.get = tlv320aic3x_dual_reg_get, .put = 
> tlv320aic3x_dual_reg_put, \
> > +	.private_value = ((reg_msb) + page) | (((reg_lsb) + 
> page) << 8) }
> > +
> > +#define TLV320AIC3X_PG1_DUAL_R(xname, reg_msb, reg_lsb) \
> > +	TLV320AIC3X_DUAL_R(xname, AIC3X_GEN_PG1_BEG, reg_msb, reg_lsb)
> 
> Given that your register write code hides the register pages is there
> any need for this to know anything about them?

Steve's response but I need to also review:
-- The registers 0-100 are mapped to register 0-100 of page 0, and
registers 128 - 204 are mapped to registers 0-76 of page 1.  This macro
adds the page 1 offset so that the write functions can tell the
registers are for page 1.

 
> > +	/* setup register cache sizes */
> > +	if (codec_variant == AIC3106_CODEC)
> > +		pg1_end = AIC3106_PG1_END;
> > +
> 
> This doesn't feel very well joined up with the way you've 
> supported this
> in the register cache - I'd expect the register code to know about the
> maximum register address and be able to check that attempts 
> aren't being
> made to use the second page on the less complex codecs.
> 
> It'd also be better to write all of these as switch statements so that
> any further devices can be supported more readily.

Ack.


> > @@ -284,6 +292,7 @@ int aic3x_button_pressed(struct 
> snd_soc_codec *codec);
> >  struct aic3x_setup_data {
> >  	int i2c_bus;
> >  	unsigned short i2c_address;
> > +	enum aic3x_codec_variant variant;
> >  	unsigned int gpio_func[2];
> >  };
> 
> It'd be nice to convert the driver to use the standard I2C probing
> stuff, though this is not essential to the patch.

Agreed and think this should be a separate stand-alone patch for othogonality purposes.


Regards, 
Martin

Re: [PATCH 1/1] Add AIC3106 support.

[Mark Brown]

On Tue, May 19, 2009 at 03:17:19PM -0500, Ambrose, Martin wrote:
> On Wed, May 13, 2009 at 11:18:30, Mark Brown wrote:

> > You should really provide a changelog explaining what's going on with
> > the chip here, especially with regard to the effect on the 
> > other devices
> > that are supported.

> My primary intent is to enable all AIC3106 functionality since my current 
> focus is on platforms which use this codec. My first patch set, which 
> admittedly was basically just a diff with the MV5 tree, added more than just 
> AIC3106 specifics. It added controls for effects, de-emphasis, and 
> attenuation coefficients which are common between the AIC33 and AIC3106 and 
> lie in the page 1 register set. To digress, I'm curious why these don't 
> already exist for the AIC33 since the source file indicates " It supports 
> full aic33 codec functionality." 

I suspect the comment is just wrong or is a non-native English error and
that the intention is to say that the driver supports everything up to
the 33 rather than everything that the 33 supports.

> add controls for the new AIC3106 page 0 registers. So, to get to my point, I 
> propose three sets of patches. The first would add page 1 register support 
> to AIC33 then a follow on patch would add the AIC3106 high pass coeff 
> support. The concept/variable of a codec variant would only be introduced in 
> the second patch set. A third patch, further out in time when I understand 
> better ALSA AGC and power management, would add support for the new page 0 
> registers.

This sounds like a good approach.

> Regarding the firs patch set, one concern I have is that this file is also 
> compatible with AIC31 and AIC32. The addition of controls without 
> qualification would result in these appearing in amixer but not actually 
> present. On the other hand this seems to be the general strategy of this 
> file. For example the AIC32 does not have register 73 but the 
> LINE2L_2_MONOLOPM_VOL control is added for all devices since it is on the 
> AIC33. Of course everything could be restructured to be more accurate on a 
> device by device basis but this seems too severe and far reaching.

Once you add support for structuring things by device variant you could
start putting the hooks in there for handling the currently supported
variants but not migrate the controls.  The controls could then be
handled in subsequent patches as people have time.

If you do convert the driver to use normal device probes (see wm8731 for
a simple example of a completed transition) then you can use the I2C
support for device variants to look up the device you're running on.

> I may subsume the new function into the old. However I do like the
> convention of leading underscore (_) to represent local/private helper
> functions. I guess this is frowned upon in the kernel source.

I can't think of many other examples - it's part of one of the reserved
namespaces in non-freestanding implementations too.

> > > +	err = snd_soc_update_bits(codec, reg_msb, 0xff, val_msb);
> > > +	if (err < 0)
> > > +		return err;

> > Just do the register write?

> Steve's response but I need to also review:
> -- Hardware values are in 2's complement and software values are in
> unsigned int.  When a value of '0' is passed in, it translated to the
> smallest possible value that should be written to hardware (which is
> 0x80000000).

I was referring to the use of snd_soc_update_bits() rather than the
value that was being written - snd_soc_update_bits() makes it look like
you're updating a bitfield when you are in fact writing the entire
register.  It seemed a bit odd, especially given the assumption about
register contents that the info function made.  Normally drivers would
just call the register write function directly, bypassing the core
(there's no benefit from going through it and the cache is internal to
the codec driver anyway).

> > > +#define TLV320AIC3X_PG1_DUAL_R(xname, reg_msb, reg_lsb) \
> > > +	TLV320AIC3X_DUAL_R(xname, AIC3X_GEN_PG1_BEG, reg_msb, reg_lsb)

> > Given that your register write code hides the register pages is there
> > any need for this to know anything about them?

> Steve's response but I need to also review:
> -- The registers 0-100 are mapped to register 0-100 of page 0, and
> registers 128 - 204 are mapped to registers 0-76 of page 1.  This macro
> adds the page 1 offset so that the write functions can tell the
> registers are for page 1.

Given that you're going to have to manually add the offset for anything
not handled by these controls are you sure that it adds any clarity to
have a special way of doing it for this one control type?

> > It'd be nice to convert the driver to use the standard I2C probing
> > stuff, though this is not essential to the patch.

> Agreed and think this should be a separate stand-alone patch for othogonality purposes.

Yes, it should be a separate patch.