[alsa-devel] [PATCH v3] ASoC: Add Freescale SGTL5000 codec support

[Timur Tabi]

On Tue, Feb 15, 2011 at 4:56 PM,  <zhaoming.zeng at freescale.com> wrote:
> From: Zeng Zhaoming <zhaoming.zeng at freescale.com>
>
> Add Freescale SGTL5000 codec support

Please provide a description of this part.  What is it, what features
does it have, etc.

> @@ -176,6 +177,9 @@ config SND_SOC_MAX98088
>  config SND_SOC_PCM3008
>        tristate
>
> +config SND_SOC_SGTL5000
> +       tristate
> +

Please provide some kind of descriptive text here.

> +       l = l < 0x3c ? 0x3c : l;
> +       l = l > 0xfc ? 0xfc : l;
> +       r = r < 0x3c ? 0x3c : r;
> +       r = r > 0xfc ? 0xfc : r;

Use the clamp() macro instead.

> +
> +       /* revert it */
> +       l = 0xfc - l;
> +       r = 0xfc - r;

I think you mean "invert it".  "revert" means to undo a change.

> +       if (mute)
> +               snd_soc_update_bits(codec, SGTL5000_CHIP_ADCDAC_CTRL,
> +                               adcdac_ctrl, adcdac_ctrl);
> +       else
> +               snd_soc_update_bits(codec, SGTL5000_CHIP_ADCDAC_CTRL,
> +                               adcdac_ctrl, 0);

How about:

snd_soc_update_bits(codec, SGTL5000_CHIP_ADCDAC_CTRL, adcdac_ctrl,
mute ? adcdac_ctrl : 0);

> +static int sgtl5000_suspend(struct snd_soc_codec *codec, pm_message_t state)
> +{
> +       sgtl5000_set_bias_level(codec, SND_SOC_BIAS_OFF);
> +
> +       return 0;
> +}

#ifdef CONFIG_PM ?


> +       /*
> +        * copy DAP default values to default value array.
> +        * sgtl5000 register space has a big hole, merge it
> +        * at init phase makes life easy.
> +        * FIXME: should we drop 'const' of sgtl5000_regs?
> +        */
> +       memcpy((void *)(&sgtl5000_regs[0] + (SGTL5000_DAP_REG_OFFSET >> 1)),
> +                       sgtl5000_dap_regs,
> +                       0x3a);

I don't think you need the "(void *)"

And what is 0x3a?

> +MODULE_DESCRIPTION("ASoC sgtl5000 driver");
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");

Please consider putting your name here, so that people know who the
real author is.

> diff --git a/sound/soc/codecs/sgtl5000.h b/sound/soc/codecs/sgtl5000.h
> new file mode 100644
> index 0000000..7f24655
> --- /dev/null
> +++ b/sound/soc/codecs/sgtl5000.h
> @@ -0,0 +1,403 @@
> +/*
> + * sgtl5000.h - SGTL5000 audio codec interface

This whole file should probably be merged into sgtl5000.c, since that
file is the only file that #includes sgtl5000.h.  No point in having a
header file that is only included by one other file.

> + *
> + * Copyright 2008-2009 Freescale Semiconductor, Inc.

So the header file is copyright 2009, but the source file is copyright 2011?

> + *
> + *  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.

Also, the header file is "GPL v2 or later", but the source file is
"GPL v2 only".  Pick one or the other, but don't use both.  I'm
surprised your patch was approved by the PPP legal review.

-- 
Timur Tabi
Linux kernel developer at Freescale