[i2c] [PATCH] add TI TLV320AIC33 audio codec support

[Jean Delvare]

Hi Vitaly,

On Wed, 11 Apr 2007 11:20:55 +0400, Vitaly Wool wrote:
> please find the patch that adds Texas Instruments TLV320AIC33 audio codec support attached.

No need to send every new driver twice, you won't get a faster answer
by doing so ;)

I wonder why you are posting an audio codec driver to the i2c list. It
sounds (ah ah) like something for the sound subsystem maintainer? I see
a bunch of audio codec drivers in the linux/sound subdirectory. And I
have no idea what these drivers do and how they work, so I cannot
really review this one.

> 
>  drivers/i2c/chips/Kconfig       |    7 +
>  drivers/i2c/chips/Makefile      |    1
>  drivers/i2c/chips/tlv320aic33.c |  187 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 195 insertions(+)
> 
> Signed-off-by: Vitaly Wool <vitalywool at gmail.com>
> 
> diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
> index 87ee3ce..289f5c8 100644
> --- a/drivers/i2c/chips/Kconfig
> +++ b/drivers/i2c/chips/Kconfig
> @@ -125,4 +125,11 @@ config SENSORS_MAX6875
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called max6875.
>  
> +config SENSORS_TLV320AIC33
> +	tristate "Texas Instruments TLV320AIC33 Codec"
> +	depends on I2C && I2C_DAVINCI
> +	help
> +	  If you say yes here you get support for the I2C control
> +	  interface for Texas Instruments TLV320AIC33 audio codec.
> +
>  endmenu
> diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
> index 779868e..346956a 100644
> --- a/drivers/i2c/chips/Makefile
> +++ b/drivers/i2c/chips/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
>  obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
>  obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574.o
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> +obj-$(CONFIG_SENSORS_TLV320AIC33) += tlv320aic33.o

That's definitely not a sensor chip.

>  obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>  
> diff --git a/drivers/i2c/chips/tlv320aic33.c b/drivers/i2c/chips/tlv320aic33.c
> new file mode 100644
> index 0000000..8232b0c
> --- /dev/null
> +++ b/drivers/i2c/chips/tlv320aic33.c
> @@ -0,0 +1,187 @@
> +/*
> + * Copyright (C) 2005 Texas Instruments Inc
> + * Copyright (C) 2007 MontaVista Software Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <linux/init.h>
> +#include <linux/fs.h>
> +#include <linux/vmalloc.h>
> +#include <linux/slab.h>
> +#include <linux/proc_fs.h>
> +#include <linux/ctype.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +
> +
> +#include <linux/proc_fs.h>
> +#include <linux/sysctl.h>

This list of includes needs cleanups. There are duplicates, headers you
don't seem to actually need, and missing ones.

> +
> +
> +/* Number of registers in AIC33                                           */
> +#define I2C_AIC33_REG_SIZE                102

Not used anywhere?

> +
> +struct aic33_serial_bus_ops {
> +	unsigned char version;
> +	int (*init)(void);
> +	void (*cleanup)(void);
> +	int (*read)(u8 reg, u8 *val);
> +	int (*write)(u8 reg, u8 val);
> +};
> +
> +struct aic33_i2c_param {
> +	struct i2c_client client;
> +	struct i2c_driver driver;
> +};
> +
> +static int aic33_i2c_probe_adapter(struct i2c_adapter *adap);
> +static int aic33_i2c_detach_client(struct i2c_client *client);
> +
> +/* Global structure to store the i2c driver info */
> +static struct aic33_i2c_param aic33_i2c_dev = {
> +	.driver = {
> +		.id = I2C_DRIVERID_I2CDEV,

Certainly not :p

> +		.driver = {
> +			.name = "Audio Codec I2C driver",

Very bad name. For one thing it's too generic, for another that string
will be used in sysfs, you want something short. In fact it should
match the source file name.

> +		},
> +		.attach_adapter = aic33_i2c_probe_adapter,
> +		.detach_client = aic33_i2c_detach_client,
> +	},
> +};
> +
> +static int aic33_i2c_read_reg(u8 reg, u8 *val)
> +{
> +	int err;
> +	struct i2c_client *client = &aic33_i2c_dev.client;
> +
> +	struct i2c_msg msg[1];
> +	unsigned char data[1];
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	data[0]=reg;
> +	msg->addr = client->addr;
> +	msg->flags = 0;
> +	msg->len = 1;
> +	msg->buf = data;
> +	err = i2c_transfer(client->adapter, msg, 1);
> +	if (err >= 0) {
> +		msg->flags = I2C_M_RD;
> +		err = i2c_transfer(client->adapter, msg, 1);
> +	}
> +	if (err >= 0) {
> +		*val = *data;
> +		return 0;
> +	}
> +
> +	return err;
> +}
> +
> +static int aic33_i2c_write_reg(u8 reg, u8  val)
> +{
> +	struct i2c_client *client = &aic33_i2c_dev.client;
> +	struct i2c_msg msg[1];
> +	unsigned char data[2];
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	msg->addr = client->addr;
> +	msg->flags = 0;
> +	msg->len = 2;
> +	msg->buf = data;
> +	data[0] = reg;
> +	data[1] = val;
> +	return i2c_transfer(client->adapter, msg, 1);
> +}
> +
> +static int aic33_i2c_attach_client(struct i2c_adapter *adap, int addr)
> +{
> +	struct aic33_i2c_param *aic33_i2c_if = &aic33_i2c_dev;
> +	struct i2c_client *client = &aic33_i2c_if->client;
> +	int err = -EBUSY;
> +
> +	if (client->adapter)
> +		goto out; /* our client is already attached */

How could this ever happen?

> +
> +	client->addr = addr;
> +	client->flags = 0;
> +	client->driver = &aic33_i2c_if->driver;
> +	client->adapter = adap;
> +
> +	err = i2c_attach_client(client);
> +	if (err)
> +		client->adapter = NULL;
> +out:
> +	return err;
> +}
> +
> +static int aic33_i2c_detach_client(struct i2c_client *client)
> +{
> +	int err = -ENODEV;
> +
> +	if (!client->adapter)
> +		goto out;

How could this ever happen?

> +
> +	err = i2c_detach_client(client);
> +	client->adapter = NULL;
> +
> +out:
> +	return err;
> +}
> +
> +static int aic33_i2c_probe_adapter(struct i2c_adapter *adap)
> +{
> +	/* 
> +	 * I2C client can be up to 4 devices with device addresses 
> +	 * 0x18, 0x19, 0x1A, 0x1B 
> +	 */
> +	return aic33_i2c_attach_client(adap, 0x1B);
> +}
> +
> +static int aic33_i2c_init(void)

Should be tagged __init.

> +{
> +	int err;
> +	struct i2c_driver *driver = &aic33_i2c_dev.driver;
> +
> +	err = i2c_add_driver(driver);
> +	if (err)
> +		printk(KERN_ERR "Failed to register Audio Codec I2C client.\n");
> +	return err;
> +}
> +
> +static void aic33_i2c_cleanup (void)

Should be tagged __exit.

> +{
> +	struct i2c_driver *driver = &aic33_i2c_dev.driver;
> +
> +	i2c_detach_client(&aic33_i2c_dev.client);
> +	i2c_del_driver(driver);
> +	aic33_i2c_dev.client.adapter = NULL;
> +
> +	return;

Useless return.

> +}
> +
> +struct aic33_serial_bus_ops aic33_i2c_fops = {
> +	version : 0x01,
> +	init    : aic33_i2c_init,
> +	cleanup : aic33_i2c_cleanup,
> +	read    : aic33_i2c_read_reg,
> +	write   : aic33_i2c_write_reg,
> +};

Please switch to C99-style structure initialization.

> +
> +module_init(aic33_i2c_init);
> +module_exit(aic33_i2c_cleanup);


-- 
Jean Delvare