[alsa-devel] [PATCH 1/5] [RFC] ALSA ASoC driver for TLVaic23b audio codec

[Mark Brown]

On Mon, Sep 29, 2008 at 03:15:04PM +0530, Arun KS wrote:
> ASOC Audio driver for TLVaic23b audio codec

This looks good overall, thanks!  There's quite a few comments below but
they're mostly minor things at the coding style issues.

The only issue I've got overall is that the name of the codec seems to
vary through the driver - sometimes it's referred to as the tlv320aic23,
sometimes as aic23 and sometimes as tlvaic23.  It'd be good if the
naming scheme were more consistent.  My inclination would be to go with
tlv320aic23 since it will reduce the risk of collisions and the general
style is to use the full part number (then again, I know the tlv320aic3x
driver didn't do that).

> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -55,3 +55,7 @@ config SND_SOC_TWL4030
>  	tristate
>  	depends on SND_SOC && TWL4030_CORE
> 

This patch is against the OMAP tree - please generate all submitted
patches against either the ASoC dev tree or the topic/asoc branch of:

	git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git

Even if other patches in the series depend on the OMAP tree there should
be no reason not to merge an I2C codec driver like this with no external
dependencies immediately.

> +config SND_SOC_TLV320AIC23
> +	tristate
> +	depends on I2C
> +

Please also add this to the SND_SOC_ALL_CODECS Kconfig option.

> +static int aic23_write(struct snd_soc_codec *codec, unsigned int reg,
> +		       unsigned int value)
> +{
> +
> +	u8 data;
> +
> +	/* TLV320AIC23 has 7 bit address and 9 bits of data
> +	 * so we need to switch one data bit into reg and rest
> +	 * of data into val
> +	 */
> +
> +	if ((reg < 0 || reg > 9) && (reg != 15)) {
> +		printk(KERN_WARNING "Invalid register R%d\n", reg);
> +		return -1;
> +	}

It'd be nice if this said which device was being written to - adding in
the driver name to the printk(), for example.  It'd make it easier for
someone seeing the message in their logs to work out where it came from.

> +	data = (reg << 1) | (value >> 8 & 0x01);
> +
> +	aic23_write_reg_cache(codec, reg, value);
> +
> +	if (codec->hw_write(codec->control_data, (u8) data,
> +			    (u8) (value & 0xff)) == 0)
> +		return 0;
> +
> +	printk(KERN_ERR "I2C: cannot write %03x to register R%d\n", value, reg);

Again, saying where the error came from would be nice.

> +       SOC_SINGLE("Mic Mute", ANALOG_AUDIO_CONTROL_ADDR, 1, 0x01, 0),

This should be "Mic Switch" to help UIs display the control correctly.

> +/* aic23 related */
> +static const struct aic23_samplerate_reg_info
> + rate_reg_info[NUMBER_SAMPLE_RATES_SUPPORTED] = {
> +	{4000, 0x06, 1},	/*  4000 */

You could just use [] for the size of the array and let the compiler
figure it out - you actually use ARRAY_SIZE() rather than the #define
for the array size later anyway.

> +static int aic23_set_bias_level(struct snd_soc_codec *codec,
> +				enum snd_soc_bias_level level)
> +{
> +	u16 reg;
> +
> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +		reg = aic23_read_reg_cache(codec, POWER_DOWN_CONTROL_ADDR);
> +		aic23_write(codec, POWER_DOWN_CONTROL_ADDR,
> +			    reg & (~DEVICE_POWER_OFF));
> +		/* Activate interface */
> +		reg = aic23_read_reg_cache(codec, DIGITAL_INTERFACE_ACT_ADDR);
> +		aic23_write(codec, DIGITAL_INTERFACE_ACT_ADDR, reg | ACT_ON);
> +		break;
> +	case SND_SOC_BIAS_PREPARE:
> +		break;
> +	case SND_SOC_BIAS_STANDBY:
> +		reg = aic23_read_reg_cache(codec, POWER_DOWN_CONTROL_ADDR);
> +		aic23_write(codec, POWER_DOWN_CONTROL_ADDR,
> +			    reg | DEVICE_POWER_OFF);
> +		/* Deactivate interface */
> +		reg = aic23_read_reg_cache(codec, DIGITAL_INTERFACE_ACT_ADDR);
> +		aic23_write(codec, DIGITAL_INTERFACE_ACT_ADDR, reg & (~ACT_ON));

Hrm.  This probably deserves a comment about what DEVICE_POWER_OFF does
that the normal DAPM control doesn't.  Given that there are no bypass
paths there should be no risk of interfering with DAPM but it raises a
bit of an eyebrow.

> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> +	codec->hw_write = (hw_write_t) i2c_smbus_write_byte_data;
> +	codec->hw_read = NULL;
> +	ret = i2c_add_driver(&aic23b_i2c_driver);
> +	if (ret != 0)
> +		printk(KERN_ERR "can't add i2c driver");
> +#else
> +	/* Add other interfaces here */
> +#endif

If the device doesn't support anything except I2C then please remove all
the conditionals on I2C support from the driver (they don't add
anything).  If the device does support other control methods then please
change this to be something like:

   #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
   ...
   #endif
   /* Add other interfaces here */

That way if someone adds support for the other control interfaces
they'll be encouraged to do it in a way that allows kernels to be built
supporting multiple systems simultaneously.

> +#define AIC23_CACHEREGNUM		16
> +/* Codec TLV320AIC23 */
> +#define LEFT_LINE_VOLUME_ADDR		0x00
> +#define RIGHT_LINE_VOLUME_ADDR		0x01
> +#define LEFT_CHANNEL_VOLUME_ADDR	0x02
> +#define RIGHT_CHANNEL_VOLUME_ADDR	0x03
> +#define ANALOG_AUDIO_CONTROL_ADDR	0x04
> +#define DIGITAL_AUDIO_CONTROL_ADDR	0x05
> +#define POWER_DOWN_CONTROL_ADDR		0x06
> +#define DIGITAL_AUDIO_FORMAT_ADDR	0x07
> +#define SAMPLE_RATE_CONTROL_ADDR	0x08
> +#define DIGITAL_INTERFACE_ACT_ADDR	0x09
> +#define RESET_CONTROL_ADDR		0x0F

These (and most of the rest of the defines in the header) should all be
namespaced - there's a very good chance of them colliding with other
things and this header is going to be included in the machine drivers.

> +/* Define to set the AIC23 as the master w.r.t McBSP */
> +#define AIC23_MASTER

> +#ifndef DEFAULT_BITPERSAMPLE
> +#define DEFAULT_BITPERSAMPLE		16
> +#endif

> +#define DEFAULT_SAMPLE_RATE		44100
> +#define CODEC_CLOCK			12000000
> +#define AUDIO_MCBSP			OMAP_MCBSP1

None of these should be in this driver - they should all be part of the
machine driver.

> +struct aic23_samplerate_reg_info {
> +	u32 sample_rate;
> +	u8 control;		/* SR3, SR2, SR1, SR0 and BOSR */
> +	u8 divider		/* if 0 CLKIN = MCLK, if 1 CLKIN = MCLK/2 */
> +};

This could just be pushed into the driver.