On Sun, Mar 6, 2011 at 20:11, cliff.cai@analog.com wrote:
+#define ADAU1701_FIRMWARE "SigmaDSP_fw.bin"
this probably needs to be more specific. like "adau1701.bin". otherwise it makes it a pain to work with diff codecs in the same system.
+static int adau1701_write_register(struct snd_soc_codec *codec,
- u16 reg_address, u8 length, u32 value)
+{
- int ret;
- int count = length + 2; /*data plus 16bit register address*/
- u8 buf[8] = {0, 0, 0, 0, 0, 0, 0, 0};
- if (length == 0)
- return -1;
- buf[0] = (reg_address >> 8) & 0xFF;
should be a blank line after that return
- buf[1] = reg_address & 0xFF;
- if (length == 1)
- buf[2] = value & 0xFF;
- else if (length == 2) {
- buf[2] = (value >> 8) & 0xFF;
- buf[3] = value & 0xFF;
- } else if (length == 3) {
- buf[2] = (value >> 16) & 0xFF;
- buf[3] = (value >> 8) & 0xFF;
- buf[4] = value & 0xFF;
- }
i think the buf[8] init forces gcc to generate bad code, and seems to be useless. shouldnt the code have an "else" brace to return -1 if length is greater than 3, and then change the decl to "u8 buf[5];" ?
also, all these 0xFF masks are useless -- you're assigning it to a u8 which implies bit truncation
ret = i2c_master_send(codec->control_data, buf, count);- return ret;
+}
the whole "int ret" seems kind of useless in this func. just return the i2c_master_send and drop the ret var completely. and drop the blank link after the return.
+/*
- read ADAU1701 hw register
- */
+static u32 adau1701_read_register(struct snd_soc_codec *codec,
- u16 reg_address, u8 length)
+{
- u8 addr[2];
- u8 buf[2];
- u32 value = 0;
- int ret;
- if (reg_address < ADAU1701_FIRSTREG)
- reg_address = reg_address + ADAU1701_FIRSTREG;
- if ((reg_address < ADAU1701_FIRSTREG) || (reg_address > ADAU1701_LASTREG))
- return -EIO;
this func has "u32" for ret, and you return the raw register value below. but here you try returning -EIO. shouldnt it be "int" so the caller can check for "< 0" ?
- addr[0] = (reg_address >> 8) & 0xFF;
- addr[1] = reg_address & 0xFF;
- /* write the 2byte read address */
- ret = i2c_master_send(codec->control_data, addr, 2);
- if (ret)
- return ret;
- if (length == 1) {
- if (i2c_master_recv(codec->control_data, buf, 1) != 1)
- return -EIO;
- value = buf[0];
- } else if (length == 2) {
- if (i2c_master_recv(codec->control_data, buf, 2) != 2)
- return -EIO;
seems like this can be combined into one simple code block where you replace "1" and '2" with "length"
+static int adau1701_setprogram(struct snd_soc_codec *codec) +{
- int ret = 0;
- ret = process_sigma_firmware(codec->control_data, ADAU1701_FIRMWARE);
- return ret;
+}
seems like this should be one statement -- a simple return
+static int adau1701_set_bias_level(struct snd_soc_codec *codec,
- enum snd_soc_bias_level level)
+{
- u16 reg;
- switch (level) {
- case SND_SOC_BIAS_ON:
- reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);
- reg &= ~(AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |
- AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD);
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);
- break;
- case SND_SOC_BIAS_PREPARE:
- break;
- case SND_SOC_BIAS_STANDBY:
- break;
- case SND_SOC_BIAS_OFF:
- /* everything off, dac mute, inactive */
- reg = adau1701_read_register(codec, ADAU1701_AUXNPOW, 2);
- reg |= AUXNPOW_AAPD | AUXNPOW_D0PD | AUXNPOW_D1PD | AUXNPOW_D2PD |
- AUXNPOW_D3PD | AUXNPOW_VBPD | AUXNPOW_VRPD;
- adau1701_write_register(codec, ADAU1701_AUXNPOW, 2, reg);
error checking missing on the read_register() call
- break;
- }
drop the blank line before the brace
+struct snd_soc_dai_driver adau1701_dai = {
- .name = "ADAU1701",
i think the standard is to use all lower case
+static int adau1701_suspend(struct snd_soc_codec *codec, pm_message_t state) +{
- adau1701_set_bias_level(codec, SND_SOC_BIAS_OFF);
- return 0;
+}
+static int adau1701_resume(struct snd_soc_codec *codec) +{
- adau1701_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
- return 0;
+} ... +static int adau1701_remove(struct snd_soc_codec *codec) +{
adau1701_set_bias_level(codec, SND_SOC_BIAS_OFF);return 0;+}
why not return set_bias_level ?
+static int adau1701_reg_init(struct snd_soc_codec *codec) ...
- printk(KERN_ERR "Loading program data failed\n");
dev_err
+struct snd_soc_codec_driver soc_codec_dev_adau1701 = {
static
+static __devinit int adau1701_i2c_probe(struct i2c_client *i2c,
- const struct i2c_device_id *id)
+{
- struct adau1701_priv *adau1701;
- int ret = 0;
- adau1701 = kzalloc(sizeof(struct adau1701_priv), GFP_KERNEL);
sizeof(*adau1701)
+static int __init adau1701_modinit(void) +{
- int ret;
- ret = i2c_add_driver(&adau1701_i2c_driver);
- if (ret != 0) {
- printk(KERN_ERR "Failed to register adau1701 I2C driver: %d\n",
- ret);
- }
uesless braces, and should be pr_err
+MODULE_DESCRIPTION("ASoC ADAU1701 SigmaDSP driver"); +MODULE_AUTHOR("Cliff Cai"); +MODULE_LICENSE("GPL");
MODULE_ALIAS() ?
+++ b/sound/soc/codecs/adau1701.h ... +#define AUXADCE_AAEN (1 << 15) +#define OSCIPOW_OPD (0x04) +#define DACSET_DACEN (1)
looks like you got "#define<tab>" junk in here -mike