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

Mark Brown
2009-09-14T17:21:34Z

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.