lkml.org 
[lkml]   [2011]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [Device-drivers-devel] [PATCH] Add driver for Analog Devices ADAU1701 SigmaDSP
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-03-07 03:47    [W:0.045 / U:29.028 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site