| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <AANLkTinGyn8tujw-TrLibuDuj_Xqb4_YEPfLPFUy4WdO@mail.gmail.com>
Date: Sun, 6 Mar 2011 21:44:49 -0500
From: Mike Frysinger <vapier.adi@...il.com>
To: cliff.cai@...log.com
Cc: broonie@...nsource.wolfsonmicro.com, alsa-devel@...a-project.org,
linux-kernel@...r.kernel.org,
device-drivers-devel@...ckfin.uclinux.org,
akpm@...ux-foundation.org, lrg@...mlogic.co.uk
Subject: Re: [Device-drivers-devel] [PATCH] Add driver for Analog Devices
ADAU1701 SigmaDSP
On Sun, Mar 6, 2011 at 20:11, <cliff.cai@...log.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists