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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
