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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux