[alsa-devel] [PATCH 1/3] ASoC: TWL6030: Add twl6030 codec driver

[Lopez Cruz, Misael]

Mark Brown wrote:
> 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?

I think channel slot definition won't make it able to interoperate with
other PDM interfaces. But I may be wrong.

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

The codec is turned on/off through an external line (i.e. with a gpio).
Then, codec enable is board-dependent.

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

For the moment, there is no reason. I thought it was more clear to have
separate power_up/power_down functions, but I can merge them in bias
management function.

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

Yes, it's the right order. codec chip cannot be initialized if the codec
is not already power-up, registers are not accesible before that.