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

[Zeng Zhaoming]

On Wed 2011-02-16 16:20:37, Timur Tabi wrote:
> 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.

thanks for the hint.
> 
> > +
> > +       /* revert it */
> > +       l = 0xfc - l;
> > +       r = 0xfc - r;
> 
> I think you mean "invert it".  "revert" means to undo a change.
>
yes, invert it. ;)
> > +       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);
>
Yes, this one is better. I will change to this style in the whole file.

> > +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.
>
This is not common on ASoC codec drivers, many drivers have their own header file.
I think it make the code more clear.

> > + *
> > + * 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.
>
Ok, I will change it to GPL v2 only, thanks to point it out.
> -- 
> Timur Tabi
> Linux kernel developer at Freescale
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel