Re: [PATCH 1/3] ASoC: TWL6030: Add twl6030 codec driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Mon, Sep 14, 2009 at 12:00:25PM -0500, Lopez Cruz, Misael wrote:

> 
> +/* propietary formats */
> +#define SND_SOC_DAIFMT_MCPDM           0x10 /* Texas Instruments McPDM */

This should really be split out into a separate patch.  Are you
absolutely positive that this is a proprietary interface that won't
interoperate with standard PDM?

Also note that your format doesn't match up with the existing numbering
scheme - they're all just 0, 1, 2, 3, ...

> +#define TWL6030_RATES   (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000)
> +#define TWL6030_FORMATS         (SNDRV_PCM_FMTBIT_S32_LE)

> +static void twl6030_power_up(struct snd_soc_codec *codec)
> +{
> +       struct snd_soc_device *socdev = codec->socdev;
> +       struct twl6030_setup_data *setup = socdev->codec_data;
> +
> +       setup->codec_enable(1);

That's interesting...?

> +       /* Capture gains */
> +       SOC_DOUBLE_TLV("Capture Preamplifier (Attenuator) Volume",
> +               TWL6030_REG_MICGAIN, 6, 7, 1, 1, mic_preamp_tlv),

No need to mention that it's an attenuator.

> +       SOC_DOUBLE_TLV("Capture Amplifier Volume",
> +               TWL6030_REG_MICGAIN, 0, 3, 4, 0, mic_amp_tlv),

Just "Capture Volume", probably.

> +static int twl6030_set_bias_level(struct snd_soc_codec *codec,
> +                               enum snd_soc_bias_level level)
> +{
> +       switch (level) {
> +       case SND_SOC_BIAS_ON:
> +               twl6030_power_up(codec);
> +               break;
> +       case SND_SOC_BIAS_PREPARE:
> +               twl6030_power_up(codec);
> +               break;
> +       case SND_SOC_BIAS_STANDBY:
> +               twl6030_power_up(codec);
> +               break;
> +       case SND_SOC_BIAS_OFF:
> +               twl6030_power_down(codec);
> +               break;

Is there any reason not to just fold these functions into the bias
management?  It looks like the only caller and it'd save jumping around
the file to find stuff.

> +static int twl6030_init(struct snd_soc_device *socdev)
> +{
> +       struct snd_soc_codec *codec = socdev->card->codec;
> +       int ret = 0;
> +
> +       dev_info(codec->dev, "TWL6030 Audio Codec init\n");

The driver should be probing as a platform device rather than using old
style registration - see wm8350 and wm8400 for examples.

> +       /* power on device */
> +       twl6030_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> +
> +       twl6030_init_chip(codec);

Is the the right ordering?  I'd have expected to see the one time init
stuff done prior to bringing up the power for the first time.