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

Lopez Cruz, Misael
2009-09-15T17:59:52Z

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.