[alsa-devel] [PATCH] Add ASoC TLV320 Codec driver.

[Takashi Iwai]

At Tue, 20 Nov 2007 09:25:39 +0000,
Mark Brown wrote:
> 
> diff --git a/include/linux/i2c-id.h b/include/linux/i2c-id.h
> index 5ced329..f6a1ddf 100644
> --- a/include/linux/i2c-id.h
> +++ b/include/linux/i2c-id.h
> @@ -122,6 +122,7 @@
>  #define I2C_DRIVERID_VP27SMPX	93	/* Panasonic VP27s tuner internal MPX */
>  #define I2C_DRIVERID_CS4270	94	/* Cirrus Logic 4270 audio codec */
>  #define I2C_DRIVERID_AK4535	95	/* AK4525 audio codec */
> +#define I2C_DRIVERID_TLV320	97	/* TLV 320 audio codec */

Where is 96? :-)

> --- /dev/null
> +++ b/sound/soc/codecs/tlv320.c
(snip)
> +//#define TLV320_DEBUG 0

Avoid C++ style comments.  Use C-style /**/ instead.

> +#define TLV320_VOICE_BITS \
> +	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
> +	SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)

Double-check if it's really S24_LE or S24_3LE.

> +
> +
> +static int caps_charge = 2000;
> +static int setting = 1;
> +module_param(caps_charge, int, 0);
> +module_param(setting, int, 0);

No description for setting variable?  Also, this should be bool
instead of int.

> +/* ADDR table */
> +static const unsigned char tlv320_reg_addr[] = {
> +	0x00,	/* CONTROL REG 0 No Operation */
> +	0x01,	/* CONTROL REG 1  */
> +        0x02,	/* CONTROL REG 2  */
> +        0x03,	/* CONTROL REG 3A */

Spaces and tabs are mixed up.  Use tabs please.

> +static inline unsigned int tlv320_read_reg_cache(struct snd_soc_codec *codec,
> +	unsigned int reg)
> +{
> +	u8 *cache = codec->reg_cache;
> +	if (reg > ARRAY_SIZE(tlv320_reg_addr))

Shouldn't it be "reg >= ARRAY_SIZE(tlv_320_reg_addr)" ?

> +static inline void tlv320_write_reg_cache(struct snd_soc_codec *codec,
> +	unsigned int reg, unsigned int value)
> +{
> +	u8 *cache = codec->reg_cache;
> +	if (reg > ARRAY_SIZE(tlv320_reg_addr))
> +		return;

Ditto.

> +static int tlv320_write(struct snd_soc_codec *codec, unsigned int reg,
> +	unsigned int value)
> +{
> +	if (tlv320_reg_addr[reg] > 0x06)
> +		return -1;
> +
> +	tlv320_write_reg_cache (codec, reg, value);
> +
> +	return i2c_smbus_write_byte_data(codec->control_data, tlv320_reg_addr[reg], value);


> +/*
> + * write block tlv320
> + */
> +static int tlv320_write_block (struct snd_soc_codec *codec,
> +	const u8 *data, unsigned int len)
> +{
> +	int ret = -1;
> +	int i;
> +
> +	for (i=0; i<len; i++) {
> +		dbg("addr = 0x%02x, data = 0x%02x", tlv320_reg_addr[i], data[i]);
> +		if ((ret = tlv320_write(codec, i, data[i])) < 0)

Avoid if ((xxx = yyy()) < 0) style.  This isn't recommended any more.
Split to two lines,
	xxx = yyy();
	if (xxx < 0)
		...

> +static int tlv320_init(struct snd_soc_device *socdev)
> +{
...
> +	/* register pcms */
> +	ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);

SNDRV_DEFAULT_IDX1 and SNDRV_DEFAULT_STR1 are for card index and name
string, not for PCM instance.  Pass 0 and any suitable string here.

> +	queue_delayed_work(tlv320_workq,
> +		&codec->delayed_work, msecs_to_jiffies(caps_charge));

Remove this unless you really implement the workq stuff.
(Ditto for workq creation.)  Creation of a workq isn't no cost even if
it's not used.

Also, there are some other coding style issues.
Try checkpatch.pl to find out them.

Furthermore, if this driver supports only I2C right now, it'd be
better to add kconfig dependency to CONFIG_I2C.


Takashi