[PATCH] asoc tlv320aic3x: set power bits correctly

[PATCH] asoc tlv320aic3x: set power bits correctly

[Daniel Mack]

[-- Attachment #1: Type: text/plain, Size: 183 bytes --]

Set power bits in output stage control registers to reflect the "!muted"
bits. The codec needs them set in order to operate correctly.

Signed-off-by: Daniel Mack <daniel@caiaq.de>



[-- Attachment #2: alsa-tlv320aic33-pwrbits.diff --]
[-- Type: text/x-diff, Size: 1187 bytes --]

From: Daniel Mack <daniel@caiaq.de>

Set power bits in output stage control registers to reflect the "!muted"
bits. The codec needs them set in order to operate correctly.

Signed-off-by: Daniel Mack <daniel@caiaq.de>

diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
index 6e4bc69..c4c50b7 100644
--- a/sound/soc/codecs/tlv320aic3x.c
+++ b/sound/soc/codecs/tlv320aic3x.c
@@ -93,6 +93,28 @@ static const u8 aic3x_reg[AIC3X_CACHEREGNUM] = {
 };
 
 /*
+ * power up/down aic3x functions
+ */
+static inline void aic3x_handle_pwr_bits(unsigned int reg, unsigned int *val)
+{
+	switch (reg) {
+	case HPLOUT_CTRL:
+	case HPROUT_CTRL:
+	case HPLCOM_CTRL:
+	case HPRCOM_CTRL:
+	case LLOPM_CTRL:
+	case RLOPM_CTRL:
+	case MONOLOPM_CTRL:
+		if (*val & 0x08)
+			*val |= 0x01;
+		else
+			*val &= ~0x01;
+
+		break;
+	}
+}
+
+/*
  * read aic3x register cache
  */
 static inline unsigned int aic3x_read_reg_cache(struct snd_soc_codec *codec,
@@ -124,6 +146,8 @@ static int aic3x_write(struct snd_soc_codec *codec, unsigned int reg,
 {
 	u8 data[2];
 
+	aic3x_handle_pwr_bits(reg, &value);
+
 	/* data is
 	 *   D15..D8 aic3x register offset
 	 *   D7...D0 register data

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] asoc tlv320aic3x: set power bits correctly

[Mark Brown]

On Wed, Apr 30, 2008 at 11:37:19AM +0200, Daniel Mack wrote:
> Set power bits in output stage control registers to reflect the "!muted"
> bits. The codec needs them set in order to operate correctly.

Could you explain in more detail what this patch is doing, please
(ideally in comments in the code or at least the commit message)?

> +	case MONOLOPM_CTRL:
> +		if (*val & 0x08)
> +			*val |= 0x01;
> +		else
> +			*val &= ~0x01;
> +
> +		break;
> +	}

It would be good if the code were clearer about what these register bits
mean and why this isn't being handled via DAPM.

If what you're trying to do is avoid powering on amplifiers when they
are muted then this is best avoided since it often creates problems with
pops and clicks due to things getting powered on and off out of the
sequence generated by DAPM.  If the amplifier is powered on before its
inputs then it will amplify any noise generaetd by the inputs when they
change power state.  This is why ASoC mute controls for amplifiers are
normally independent of their power controls.

Re: [PATCH] asoc tlv320aic3x: set power bits correctly

[Jarkko Nikula]

On Wed, Apr 30, 2008 at 12:57 PM, Mark Brown <
broonie@opensource.wolfsonmicro.com> wrote:

>
> If what you're trying to do is avoid powering on amplifiers when they
> are muted then this is best avoided since it often creates problems with
> pops and clicks due to things getting powered on and off out of the
> sequence generated by DAPM.  If the amplifier is powered on before its
> inputs then it will amplify any noise generaetd by the inputs when they
> change power state.  This is why ASoC mute controls for amplifiers are
> normally independent of their power controls.
>

I was looking do we have possible driver bug here and answer is no. Driver
keeps output state powered during playback if associated endpoint is enabled
with snd_soc_dapm_set_endpoint and unpowered otherwise.

So as with Mark's comment, the patch is unnecessary.


Jarkko

Re: [PATCH] asoc tlv320aic3x: set power bits correctly

[Daniel Mack]

Hi Jarkko,

On 30.04.2008, at 13:50, Jarkko Nikula wrote:

> I was looking do we have possible driver bug here and answer is no.  
> Driver keeps output state powered during playback if associated  
> endpoint is enabled with snd_soc_dapm_set_endpoint and unpowered  
> otherwise.
>
> So as with Mark's comment, the patch is unnecessary.

Hmm, ok - I might have a lack of knowledge here. All I was seeing with  
an I2C hardware analyzer is that those registers in question were  
written without the power bit set which makes them remain muted.  
What's the way to power up this entity properly?

And as we're on it - aic3x_dapm_event(SNDRV_CTL_POWER_D3cold) is  
called shortly after PCM playback has finished which clears the power  
bits of many registers. This in turn also makes the current PGA mixer  
setting become unfunctional as the outputs are switched off. Is there  
a proper way of preventing the glue layer to power down the chip in  
this case?

Thanks,
Daniel

Re: [PATCH] asoc tlv320aic3x: set power bits correctly

[Mark Brown]

On Wed, Apr 30, 2008 at 03:11:06PM +0200, Daniel Mack wrote:

> Hmm, ok - I might have a lack of knowledge here. All I was seeing with  
> an I2C hardware analyzer is that those registers in question were  
> written without the power bit set which makes them remain muted.  
> What's the way to power up this entity properly?

You need to ensure that the entire DAPM path from the DAC to the output
is powered by ensuring that the final output endpoint is marked as
active.  Your machine driver should be using snd_soc_dapm_set_endpoint()
to mark either the output pin of the codec or any external device (eg, a
jack) that you've told DAPM about as enabled.  If you do not do this
then DAPM will notice that none output paths are connected and not power
on any of the components in the output path in order to save power.

This should be integrated with any jack detection you are doing so that,
for example, the headphone output will be marked as disabled when there
is no headphone present.  The core will automatically manage the power
state for DACs and ADCs but the machine driver needs to manage the power
state for everything else.

> And as we're on it - aic3x_dapm_event(SNDRV_CTL_POWER_D3cold) is  
> called shortly after PCM playback has finished which clears the power  
> bits of many registers. This in turn also makes the current PGA mixer  
> setting become unfunctional as the outputs are switched off. Is there  
> a proper way of preventing the glue layer to power down the chip in  
> this case?

Are you sure about that?  The chip should only be being placed into
D3cold when the system is being suspended or powered off.  The chip will
be placed into D3hot when the DACs and ADCs go idle but not D3cold.
You will see similar behaviour if you haven't marked appropriate
endpoints as active (since DAPM will power off anything it thinks is not
in use, triggered when the DAC or ADC goes idle) but it shouldn't be
done by the driver D3cold.