Re: [PATCH v2] tlv320aic3x: disable ADC/DAC while changing clock

["Daniel Glöckner" <dg@emlix.com>]

On 04/01/2009 02:10 PM, Mark Brown wrote:
> On Wed, Apr 01, 2009 at 12:56:55PM +0200, Daniel Glöckner wrote:
> 
>> +static int aic3x_trigger(struct snd_pcm_substream *substream, int cmd,
>> +                        struct snd_soc_dai *codec_dai)
>> +{
>> +       struct snd_soc_codec *codec = codec_dai->codec;
>> +       struct aic3x_priv *aic3x = codec->private_data;
>> +
>> +       switch (cmd) {
>> +       case SNDRV_PCM_TRIGGER_START:
>> +       case SNDRV_PCM_TRIGGER_RESUME:
>> +       case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +               aic3x->running[substream->stream] = 1;
>> +               break;
> 
> Hrm, does the chip support asymmetric configurations for playback and
> capture?

No, why do you ask?

>> +/*
>> + * Enables or disables the left/right ADC/DAC according to new.
>> + * Bits in new:
>> + *	0 left ADC
>> + *	1 right ADC
>> + *	2 left DAC
>> + *	4 right DAC
>> + * A set bit enables the component.
>> + * Returns the previous state of those components encoded in the same way.
>> + */
> 
> Is this really required?  I can't see it being a good idea to bounce the
> power of the DAC or ADC while they are live since that will impact the
> audio stream noticably.  It looks like it'd be better to do a check
> which refuses to do the reconfiguration when it's not possible.

Powering off is required in case the stream has been closed previously but the
close_delayed_work has not yet been run. That was the case where I observed the
problems this patch tries to fix.

> It's especially suspicious since the power of the DAC and ADC is managed
> via DAPM; the only current user should be safe since it saves then
> restores the state but it does raise alarm bells.

How about making it a function that always disables the four components and
referring to snd_soc_dapm_sync to restore the state?

In this version of the patch I removed the line that restores the state when
setting the sampling rate failed in-between. Is it safe to assume that DAPM will
not enable the ADC/DAC on other occasions before a valid sampling rate has been
configured? One could reorder aic3x_hw_params to detect the error before the
first registers are written..

>>  	case SND_SOC_BIAS_OFF:
>> -		/* force all power off */
>> -		reg = aic3x_read_reg_cache(codec, LINE1L_2_LADC_CTRL);
>> -		aic3x_write(codec, LINE1L_2_LADC_CTRL, reg & ~LADC_PWR_ON);
> 
> This should be split into a separate patch; it's a separate change.

Ok

  Daniel

-- 
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax -11, Bahnhofsallee 1b, 37081 Göttingen, Germany
Geschäftsführung: Dr. Uwe Kracke, Dr. Cord Seele, Ust-IdNr.: DE 205 198 055
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160

emlix - your embedded linux partner