[PATCH] ASoC: TWL4030: PM fix for output amplifiers

[PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Peter Ujfalusi]

Gain controls on outputs affect the power consumption
when the gain is set to non 0 value.
To prevent amps beeing enabled, when they are not
needed, introduce the following workaround:
Track the state of each of this type of output.
In twl4030_write only allow actual write, when the
given output is enabled, otherwise only update
the reg_cache.
The PGA event handlers on power up will write the cached
value to the chip (restoring gain, routing selection).
On power down 0 is written to the register (disabling
the amp, and also just in case clearing the routing).

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/codecs/twl4030.c |   72 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 6f5d4af..bf59b8a 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -135,9 +135,11 @@ struct twl4030_priv {
 
 	unsigned int sysclk;
 
-	/* Headset output state handling */
-	unsigned int hsl_enabled;
-	unsigned int hsr_enabled;
+	/* Output (with associated amp) states */
+	u8 hsl_enabled, hsr_enabled;
+	u8 earpiece_enabled;
+	u8 predrivel_enabled, predriver_enabled;
+	u8 carkitl_enabled, carkitr_enabled;
 };
 
 /*
@@ -173,12 +175,47 @@ static inline void twl4030_write_reg_cache(struct snd_soc_codec *codec,
 static int twl4030_write(struct snd_soc_codec *codec,
 			unsigned int reg, unsigned int value)
 {
+	struct twl4030_priv *twl4030 = codec->private_data;
+	int write_to_reg = 0;
+
 	twl4030_write_reg_cache(codec, reg, value);
-	if (likely(reg < TWL4030_REG_SW_SHADOW))
-		return twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE, value,
-					    reg);
-	else
-		return 0;
+	if (likely(reg < TWL4030_REG_SW_SHADOW)) {
+		/* Decide if the given register can be written */
+		switch (reg) {
+		case TWL4030_REG_EAR_CTL:
+			if (twl4030->earpiece_enabled)
+				write_to_reg = 1;
+			break;
+		case TWL4030_REG_PREDL_CTL:
+			if (twl4030->predrivel_enabled)
+				write_to_reg = 1;
+			break;
+		case TWL4030_REG_PREDR_CTL:
+			if (twl4030->predriver_enabled)
+				write_to_reg = 1;
+			break;
+		case TWL4030_REG_PRECKL_CTL:
+			if (twl4030->carkitl_enabled)
+				write_to_reg = 1;
+			break;
+		case TWL4030_REG_PRECKR_CTL:
+			if (twl4030->carkitr_enabled)
+				write_to_reg = 1;
+			break;
+		case TWL4030_REG_HS_GAIN_SET:
+			if (twl4030->hsl_enabled || twl4030->hsr_enabled)
+				write_to_reg = 1;
+			break;
+		default:
+			/* All other register can be written */
+			write_to_reg = 1;
+			break;
+		}
+		if (write_to_reg)
+			return twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
+						    value, reg);
+	}
+	return 0;
 }
 
 static void twl4030_codec_enable(struct snd_soc_codec *codec, int enable)
@@ -525,26 +562,26 @@ static int micpath_event(struct snd_soc_dapm_widget *w,
  * Output PGA builder:
  * Handle the muting and unmuting of the given output (turning off the
  * amplifier associated with the output pin)
- * On mute bypass the reg_cache and mute the volume
- * On unmute: restore the register content
+ * On mute bypass the reg_cache and write 0 to the register
+ * On unmute: restore the register content from the reg_cache
  * Outputs handled in this way:  Earpiece, PreDrivL/R, CarkitL/R
  */
 #define TWL4030_OUTPUT_PGA(pin_name, reg, mask)				\
 static int pin_name##pga_event(struct snd_soc_dapm_widget *w,		\
 		struct snd_kcontrol *kcontrol, int event)		\
 {									\
-	u8 reg_val;							\
+	struct twl4030_priv *twl4030 = w->codec->private_data;		\
 									\
 	switch (event) {						\
 	case SND_SOC_DAPM_POST_PMU:					\
+		twl4030->pin_name##_enabled = 1;			\
 		twl4030_write(w->codec, reg,				\
 			twl4030_read_reg_cache(w->codec, reg));		\
 		break;							\
 	case SND_SOC_DAPM_POST_PMD:					\
-		reg_val = twl4030_read_reg_cache(w->codec, reg);	\
-		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,	\
-					reg_val & (~mask),		\
-					reg);				\
+		twl4030->pin_name##_enabled = 0;			\
+		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,		\
+					0, reg);			\
 		break;							\
 	}								\
 	return 0;							\
@@ -664,7 +701,10 @@ static void headset_ramp(struct snd_soc_codec *codec, int ramp)
 		/* Headset ramp-up according to the TRM */
 		hs_pop |= TWL4030_VMID_EN;
 		twl4030_write(codec, TWL4030_REG_HS_POPN_SET, hs_pop);
-		twl4030_write(codec, TWL4030_REG_HS_GAIN_SET, hs_gain);
+		/* Actually write to the register */
+		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
+					hs_gain,
+					TWL4030_REG_HS_GAIN_SET);
 		hs_pop |= TWL4030_RAMP_EN;
 		twl4030_write(codec, TWL4030_REG_HS_POPN_SET, hs_pop);
 		/* Wait ramp delay time + 1, so the VMID can settle */
-- 
1.7.0.2

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Mark Brown]

On Mon, Mar 22, 2010 at 03:36:07PM +0200, Peter Ujfalusi wrote:
> Gain controls on outputs affect the power consumption
> when the gain is set to non 0 value.
> To prevent amps beeing enabled, when they are not
> needed, introduce the following workaround:
> Track the state of each of this type of output.
> In twl4030_write only allow actual write, when the
> given output is enabled, otherwise only update
> the reg_cache.
> The PGA event handlers on power up will write the cached
> value to the chip (restoring gain, routing selection).
> On power down 0 is written to the register (disabling
> the amp, and also just in case clearing the routing).

I'm not 100% clear on what the existing code is supposed to be doing so
this explanation isn't entirely clear to me, sorry.

If it's supposed to be holding the controls at a mute value while the
PGA is powered down then this is something that ASoC could benefit from
in general - it'd be much better if we could keep amplifiers muted while
not in active use and sequence the unmute into the power management at
the end since this is good for pop/click management in general.  I'd
started to look at this but not yet got enough time to finish off
implementing it.

What I'd been thinking of doing was introducing a new control type which
would be the inverse of supply - something that's switched on whenever
its inputs are switched on, sequenced to power on at the end of the
sequence and power off at the start.  This would support both events and
a set of controls, with the controls doing what this looks like and only
writing to the device when the widget is powered.  I've not actually
tried implementing this properly yet, though - it'd involve adding a
new bunch of control types, sadly.

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Peter Ujfalusi]

On Monday 22 March 2010 15:47:01 ext Mark Brown wrote:
> On Mon, Mar 22, 2010 at 03:36:07PM +0200, Peter Ujfalusi wrote:
> > Gain controls on outputs affect the power consumption
> > when the gain is set to non 0 value.
> > To prevent amps beeing enabled, when they are not
> > needed, introduce the following workaround:
> > Track the state of each of this type of output.
> > In twl4030_write only allow actual write, when the
> > given output is enabled, otherwise only update
> > the reg_cache.
> > The PGA event handlers on power up will write the cached
> > value to the chip (restoring gain, routing selection).
> > On power down 0 is written to the register (disabling
> > the amp, and also just in case clearing the routing).
> 
> I'm not 100% clear on what the existing code is supposed to be doing so
> this explanation isn't entirely clear to me, sorry.

Sorry for that. So the purpose of this:
Most of the output register on the TWL looks like this (take PREDRIVEL):
PREDL_CTL (0x25):
bit 0: Voice enable
bit 1: Audio L1 enable
bit 2: Audio L2 enable
bit 3: Audio R2 enable
bit 4-5: Gain (0x0 - power down, 0x1 - 6dB, 0x2 - 0dB, 0x3 - -6dB)

Now bits 0 - 3 are part of the DAPM route implementation.
bit 4-5 (gain) has simple volume control.

If there is no audio activity, and user changes the routing, than the gain value 
will be also written to the chip, which causes the amp to be enabled.
Same goes, if the user changes the volume during inactive, the amp will be 
enabled.
The existing code did not handle this situation, it only muted the gain when the 
DAPM route got disabled.
So if the user modifies a gain, which is not part of the DAPM route, than that 
is going to be enabled all the time, since the DAPM off will not hit it.

In this way I can be sure, that no other gains are actually enabled, than the 
ones which is needed.

Oh, and if these gain values are other than 0, than it means that the amp is on, 
and consumes power. So I need to make sure, that they are off, when they are not 
needed.

> If it's supposed to be holding the controls at a mute value while the
> PGA is powered down then this is something that ASoC could benefit from
> in general - it'd be much better if we could keep amplifiers muted while
> not in active use and sequence the unmute into the power management at
> the end since this is good for pop/click management in general.  I'd
> started to look at this but not yet got enough time to finish off
> implementing it.

Yes, I have been also thinking about that, but I really don't know how it can be 
done:

For the DAPM part it is kind of easy, since we can decide in the core when to 
write to the chip, and when only to the reg_cache.
But since I have the gain in the same register, when the user changing the 
volume, than that control don't have information about the associated DAPM 
widget's state.

> What I'd been thinking of doing was introducing a new control type which
> would be the inverse of supply - something that's switched on whenever
> its inputs are switched on, sequenced to power on at the end of the
> sequence and power off at the start.  This would support both events and
> a set of controls, with the controls doing what this looks like and only
> writing to the device when the widget is powered.  I've not actually
> tried implementing this properly yet, though - it'd involve adding a
> new bunch of control types, sadly.

Well, I think the core could have some basic support for similar cases, but I'm 
not sure if it is possible to cover all cases in the core.

I do think, that at least the TWL codec has to be handled in a custom way. For 
now at least.

Should I add more explanation to the commit message to make the intention more 
clear?

-- 
Péter

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Mark Brown]

On Mon, Mar 22, 2010 at 04:04:56PM +0200, Peter Ujfalusi wrote:

> bit 4-5: Gain (0x0 - power down, 0x1 - 6dB, 0x2 - 0dB, 0x3 - -6dB)

> If there is no audio activity, and user changes the routing, than the gain value 
> will be also written to the chip, which causes the amp to be enabled.

So it's shared power and volume in a single bitfield.

> > If it's supposed to be holding the controls at a mute value while the
> > PGA is powered down then this is something that ASoC could benefit from
> > in general - it'd be much better if we could keep amplifiers muted while
> > not in active use and sequence the unmute into the power management at
> > the end since this is good for pop/click management in general.  I'd
> > started to look at this but not yet got enough time to finish off
> > implementing it.

> Yes, I have been also thinking about that, but I really don't know how it can be 
> done:

> For the DAPM part it is kind of easy, since we can decide in the core when to 
> write to the chip, and when only to the reg_cache.
> But since I have the gain in the same register, when the user changing the 
> volume, than that control don't have information about the associated DAPM 
> widget's state.

That's why I'm saying you'll need a new control type - it'd need to know
how to look at the state of a DAPM widget and decide if it should update
the hardware based on that.  The PGA handling code that's in current
mainline (not for-2.6.35, it got removed) is an example of this.

> Well, I think the core could have some basic support for similar cases, but I'm 
> not sure if it is possible to cover all cases in the core.

I don't see any fundamental problem here - mostly this just maps on to
"if the widget is powered on write this value, otherwise write the
value configured by the control.".

> I do think, that at least the TWL codec has to be handled in a custom way. For 
> now at least.

> Should I add more explanation to the commit message to make the intention more 
> clear?

Well, I was rather hoping I could convince you to make this more
generic.  But if that's not possible then yes, please do make the commit
message clearer.

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Peter Ujfalusi]

On Monday 22 March 2010 16:15:44 ext Mark Brown wrote:
> On Mon, Mar 22, 2010 at 04:04:56PM +0200, Peter Ujfalusi wrote:
> > bit 4-5: Gain (0x0 - power down, 0x1 - 6dB, 0x2 - 0dB, 0x3 - -6dB)
> > 
> > If there is no audio activity, and user changes the routing, than the
> > gain value will be also written to the chip, which causes the amp to be
> > enabled.
> 
> So it's shared power and volume in a single bitfield.

Yes.

> > > If it's supposed to be holding the controls at a mute value while the
> > > PGA is powered down then this is something that ASoC could benefit from
> > > in general - it'd be much better if we could keep amplifiers muted
> > > while not in active use and sequence the unmute into the power
> > > management at the end since this is good for pop/click management in
> > > general.  I'd started to look at this but not yet got enough time to
> > > finish off implementing it.
> > 
> > Yes, I have been also thinking about that, but I really don't know how it
> > can be done:
> > 
> > For the DAPM part it is kind of easy, since we can decide in the core
> > when to write to the chip, and when only to the reg_cache.
> > But since I have the gain in the same register, when the user changing
> > the volume, than that control don't have information about the
> > associated DAPM widget's state.
> 
> That's why I'm saying you'll need a new control type - it'd need to know
> how to look at the state of a DAPM widget and decide if it should update
> the hardware based on that.  The PGA handling code that's in current
> mainline (not for-2.6.35, it got removed) is an example of this.
> 
> > Well, I think the core could have some basic support for similar cases,
> > but I'm not sure if it is possible to cover all cases in the core.
> 
> I don't see any fundamental problem here - mostly this just maps on to
> "if the widget is powered on write this value, otherwise write the
> value configured by the control.".

Hmm, I suppose it could be possible to pass the name of the DAPM widget, a 
bitmask to a control...
Than in the handler in the core we could walk through the DAPM widgets, and find 
the one, check the status, and if it is on, we allow the write, otherwise we 
could use the mask to write only the things, that it is needed.

But, in TWL case I'd like to filter out also the writes which is done by the 
DAPM widgets (which is visible from the user space, the mixers).
As it is now, if user changes the mixer, which is associated with DAPM widget 
(and it is different than the PGA, they are DAPM_MIXER), than that write goes 
directly to the register, and this write takes the cached value, makes the 
changes and than writes it to register. This also enables the gain (which 
enables the amp, which takes power).

Even if we can filter out the volume control writes, we still have the problem 
with the DAPM_MIXER writes to the same register.

> > I do think, that at least the TWL codec has to be handled in a custom
> > way. For now at least.
> > 
> > Should I add more explanation to the commit message to make the intention
> > more clear?
> 
> Well, I was rather hoping I could convince you to make this more
> generic.  But if that's not possible then yes, please do make the commit
> message clearer.

I need to study other codes as well, before I could start doing something more 
generic, which I totally agree would be beneficial for others as well.
But for now, I would like to update the commit message, and come back later, and 
revisit the possibility of similar, and more generic ways of doing this.

-- 
Péter

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Mark Brown]

On Mon, Mar 22, 2010 at 04:46:08PM +0200, Peter Ujfalusi wrote:
> On Monday 22 March 2010 16:15:44 ext Mark Brown wrote:

> > I don't see any fundamental problem here - mostly this just maps on to
> > "if the widget is powered on write this value, otherwise write the
> > value configured by the control.".

> Hmm, I suppose it could be possible to pass the name of the DAPM widget, a 
> bitmask to a control...
> Than in the handler in the core we could walk through the DAPM widgets, and find 
> the one, check the status, and if it is on, we allow the write, otherwise we 
> could use the mask to write only the things, that it is needed.

Something like that, yes - the walk could happen at startup time to
avoid having to do it repeatedly.

> But, in TWL case I'd like to filter out also the writes which is done by the 
> DAPM widgets (which is visible from the user space, the mixers).
> As it is now, if user changes the mixer, which is associated with DAPM widget 
> (and it is different than the PGA, they are DAPM_MIXER), than that write goes 
> directly to the register, and this write takes the cached value, makes the 
> changes and than writes it to register. This also enables the gain (which 
> enables the amp, which takes power).

Remember, the register cache is below the controls and transparent to
them - if the controls haven't written a value to the chip then it will
not appear in the register cache so other controls will not be affected.

> > Well, I was rather hoping I could convince you to make this more
> > generic.  But if that's not possible then yes, please do make the commit
> > message clearer.

> I need to study other codes as well, before I could start doing something more 
> generic, which I totally agree would be beneficial for others as well.
> But for now, I would like to update the commit message, and come back later, and 
> revisit the possibility of similar, and more generic ways of doing this.

I guess.

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Peter Ujfalusi]

On Monday 22 March 2010 17:06:03 ext Mark Brown wrote:
> On Mon, Mar 22, 2010 at 04:46:08PM +0200, Peter Ujfalusi wrote:
> > On Monday 22 March 2010 16:15:44 ext Mark Brown wrote:
> > > I don't see any fundamental problem here - mostly this just maps on to
> > > "if the widget is powered on write this value, otherwise write the
> > > value configured by the control.".
> > 
> > Hmm, I suppose it could be possible to pass the name of the DAPM widget,
> > a bitmask to a control...
> > Than in the handler in the core we could walk through the DAPM widgets,
> > and find the one, check the status, and if it is on, we allow the write,
> > otherwise we could use the mask to write only the things, that it is
> > needed.
> 
> Something like that, yes - the walk could happen at startup time to
> avoid having to do it repeatedly.

Make sense.

> 
> > But, in TWL case I'd like to filter out also the writes which is done by
> > the DAPM widgets (which is visible from the user space, the mixers). As
> > it is now, if user changes the mixer, which is associated with DAPM
> > widget (and it is different than the PGA, they are DAPM_MIXER), than
> > that write goes directly to the register, and this write takes the
> > cached value, makes the changes and than writes it to register. This
> > also enables the gain (which enables the amp, which takes power).
> 
> Remember, the register cache is below the controls and transparent to
> them - if the controls haven't written a value to the chip then it will
> not appear in the register cache so other controls will not be affected.

Yes, but it also means, that the change will be not visible at all.
I mean, if we did not update the reg_cache (when we should not write to the HW), 
than that change will be gone.
So you will only be able to change the gain on the output, when the path is 
active?

Also, I think, if we can somehow prevent the gain change on the HW, when the 
associated DAPM widget is not active, we would still have the problem of the 
DAPM_MIXER. That operates from the cache, and if the cache has no 0 for the 
gain, than that will be written to the chip.

How I see this: we can go and introduce new controls, DAPM widgets for all type 
just for the sake of the TWL codec. But at the end it will be still TWL 
specific.

I'm not saying that other codecs could not use some generic way (in a way, that 
you have described) to make their amps muted when they are not needed.
What I'm saying, is that I don't see a generic way which can handle the TWL 
codec.

> > > Well, I was rather hoping I could convince you to make this more
> > > generic.  But if that's not possible then yes, please do make the
> > > commit message clearer.
> > 
> > I need to study other codes as well, before I could start doing something
> > more generic, which I totally agree would be beneficial for others as
> > well. But for now, I would like to update the commit message, and come
> > back later, and revisit the possibility of similar, and more generic
> > ways of doing this.
> 
> I guess.

I hope that I can convince you, that - again - the TWL codec is so special, it 
has to handled a bit differently.

-- 
Péter

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Mark Brown]

On Mon, Mar 22, 2010 at 05:31:14PM +0200, Peter Ujfalusi wrote:
> On Monday 22 March 2010 17:06:03 ext Mark Brown wrote:

> > Remember, the register cache is below the controls and transparent to
> > them - if the controls haven't written a value to the chip then it will
> > not appear in the register cache so other controls will not be affected.

> Yes, but it also means, that the change will be not visible at all.
> I mean, if we did not update the reg_cache (when we should not write to the HW), 
> than that change will be gone.
> So you will only be able to change the gain on the output, when the path is 
> active?

No, the control will have to take care of only writing to the chip when
it knows it's safe to do so.

> Also, I think, if we can somehow prevent the gain change on the HW, when the 
> associated DAPM widget is not active, we would still have the problem of the 
> DAPM_MIXER. That operates from the cache, and if the cache has no 0 for the 
> gain, than that will be written to the chip.

Once again, the cache is operating below the controls transparently to
them.  If the controls haven't written a value to the chip then the
write won't have gone to the register cache either and so other parts of
the system won't be affected.  As far as the control layer is concerned
the register cache and the hardware are identical - I'm not sure why you
believe that they are separate.

Please take a look at the old PGA code I mentioned, it implements this
without any problem.  The control suppresses the write unless the
widget is powered, storing the value separately.  If the widget is
powered down then the registers aren't referred to at all when the
control is accessed.

> How I see this: we can go and introduce new controls, DAPM widgets for all type 
> just for the sake of the TWL codec. But at the end it will be still TWL 
> specific.

> I'm not saying that other codecs could not use some generic way (in a way, that 
> you have described) to make their amps muted when they are not needed.
> What I'm saying, is that I don't see a generic way which can handle the TWL 
> codec.

Could you please be more specific about what you believe is going on
with TWL4030 here?  It would really help if you could be more specific
about how you see the register cache getting updated.

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Liam Girdwood]

On Mon, 2010-03-22 at 16:19 +0000, Mark Brown wrote:
> On Mon, Mar 22, 2010 at 05:31:14PM +0200, Peter Ujfalusi wrote:
> 
> > How I see this: we can go and introduce new controls, DAPM widgets for all type 
> > just for the sake of the TWL codec. But at the end it will be still TWL 
> > specific.
> 
> > I'm not saying that other codecs could not use some generic way (in a way, that 
> > you have described) to make their amps muted when they are not needed.
> > What I'm saying, is that I don't see a generic way which can handle the TWL 
> > codec.
> 
> Could you please be more specific about what you believe is going on
> with TWL4030 here?  It would really help if you could be more specific
> about how you see the register cache getting updated.

I think Peter means that without making changes to the current IO model
or reverting the removal of the old PGA code there is no generic way of
handling this atm.

So we should probably look at bringing back the PGA code (now that there
is a user) so that the patch can be updated to use it.

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Mark Brown]

On Mon, Mar 22, 2010 at 04:48:34PM +0000, Liam Girdwood wrote:

> I think Peter means that without making changes to the current IO model
> or reverting the removal of the old PGA code there is no generic way of
> handling this atm.

Well, you need to add new control types anyway so they know not to do
the writes so making them DTRT is fairly trivial.

> So we should probably look at bringing back the PGA code (now that there
> is a user) so that the patch can be updated to use it.

The issue with the old PGA code is that it's fundamentally mono but a
substantial proportion of the output paths really are stereo and want to
export stereo control to userspace but DAPM is pretty much mono all the
way.  Decoupling the mute from the PGA solves that, and lets us use this
with other widget types if that's useful (though virtual PGAs could
cover that too).

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Peter Ujfalusi]

On Monday 22 March 2010 18:19:28 ext Mark Brown wrote:
> > Yes, but it also means, that the change will be not visible at all.
> > I mean, if we did not update the reg_cache (when we should not write to
> > the HW), than that change will be gone.
> > So you will only be able to change the gain on the output, when the path
> > is active?
> 
> No, the control will have to take care of only writing to the chip when
> it knows it's safe to do so.

Now I start to understand what you are actually trying to say.
 
> > Also, I think, if we can somehow prevent the gain change on the HW, when
> > the associated DAPM widget is not active, we would still have the
> > problem of the DAPM_MIXER. That operates from the cache, and if the
> > cache has no 0 for the gain, than that will be written to the chip.
> 
> Once again, the cache is operating below the controls transparently to
> them.  If the controls haven't written a value to the chip then the
> write won't have gone to the register cache either and so other parts of
> the system won't be affected.  As far as the control layer is concerned
> the register cache and the hardware are identical - I'm not sure why you
> believe that they are separate.

OK. I know that the concept is that the cache should be equal to the actual 
register content.
What I'm doing is kind of misuse the fact, that the upper layers are only 
reading from the cache. So I intentionally 'lie' (in a controlled manner) to the 
control and DAPM layer about the HW status to achieve my goal.
Or I can put it like I made an abstraction at the lower level.

Your proposal is to make about similar abstraction, but in higher level, so the 
cache will actually equals to the HW status.

And you are right, that this shall be handled in the core in the future for the 
benefit of other codecs.

> Please take a look at the old PGA code I mentioned, it implements this
> without any problem.  The control suppresses the write unless the
> widget is powered, storing the value separately.  If the widget is
> powered down then the registers aren't referred to at all when the
> control is accessed.

I did, and I did looked at that when I started to work with the twl4030 codec.
It turned out not that simple for these outputs.
I ended up with custom handled volume controls (stereo and one mono).

> > I'm not saying that other codecs could not use some generic way (in a
> > way, that you have described) to make their amps muted when they are not
> > needed. What I'm saying, is that I don't see a generic way which can
> > handle the TWL codec.
> 
> Could you please be more specific about what you believe is going on
> with TWL4030 here?  It would really help if you could be more specific
> about how you see the register cache getting updated.

In any ways the output gains has to be handled with custom code no matter if the 
core has support for binding the volume controls to DAPM widget or not.
The root of the problem is how the amplifiers are working:
bit 4-5: Gain
0x0 - power down (mute)
0x1 - 6dB
0x2 - 0dB
0x3 - -6dB

I don't have separate power bit.
I can not just invert the range.
I need to do the following:
If 0 is coming, than I write that (mute, power down)
If 2, than write two
For 1 I write 3
For 3 I write 1
This gives nice gain range for the user space.
0x0 - power down (mute)
0x1 - -6dB
0x2 - 0dB
0x3 - 6dB

When I have done these there were no other way, but to have my own custom calls 
to handle the situation, and going for DAPM PGA would also mean that I actually 
need to duplicate the PGA code partially in the twl4030 codec.

Yes, this is a workaround, or a controlled hack to achieve what I believe is a 
good thing for this codec, and since I did not found a way to handle these 
particular registers in the core (in a way that it would have been beneficial 
for others as well), this was the result.

I have thought about the things, that you wrote, and you are right, if the 
register for controlling the gain is 'sane', than there is no problem to have 
generic core level handling such situations.

When my time allows, I can take a look at that.

Thanks for taking the v2 patch!

Thank you,
Péter

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Peter Ujfalusi]

On Tuesday 23 March 2010 09:05:39 Ujfalusi Peter (Nokia-D/Tampere) wrote:
> In any ways the output gains has to be handled with custom code no matter
> if the core has support for binding the volume controls to DAPM widget or
> not. The root of the problem is how the amplifiers are working:
> bit 4-5: Gain
> 0x0 - power down (mute)
> 0x1 - 6dB
> 0x2 - 0dB
> 0x3 - -6dB
> 
> I don't have separate power bit.
> I can not just invert the range.
> I need to do the following:
> If 0 is coming, than I write that (mute, power down)
> If 2, than write two
> For 1 I write 3
> For 3 I write 1
> This gives nice gain range for the user space.
> 0x0 - power down (mute)
> 0x1 - -6dB
> 0x2 - 0dB
> 0x3 - 6dB

I just wonder, would something like this work:

Instead of the custom SOC_DOUBLE_R_TLV_TWL4030, which handles the output gains:

/*
 * Gain controls tied to outputs
 * -6 dB to 6 dB in 6 dB steps (mute instead of -12)
 */
static const unsigned int output_tlv[] = {
	TLV_DB_RANGE_HEAD(4),
	0, 0, TLV_DB_SCALE_ITEM(-1200, 0,  1),
	3, 3, TLV_DB_SCALE_ITEM(-600, 0, 0),
	2, 2, TLV_DB_SCALE_ITEM(0, 0, 0),
	1, 1, TLV_DB_SCALE_ITEM(600, 0, 0),
};

SOC_DOUBLE_R_TLV("PreDriv Playback Volume",
	TWL4030_REG_PREDL_CTL, TWL4030_REG_PREDR_CTL,
	4, 3, 0, output_tvl),

I'm not sure about the tlv declaration, but is there a way to actually map the 
gain/power control in the TWL in a standard way?

-- 
Péter

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Mark Brown]

On Tue, Mar 23, 2010 at 09:59:13AM +0200, Peter Ujfalusi wrote:

> /*
>  * Gain controls tied to outputs
>  * -6 dB to 6 dB in 6 dB steps (mute instead of -12)
>  */
> static const unsigned int output_tlv[] = {
> 	TLV_DB_RANGE_HEAD(4),
> 	0, 0, TLV_DB_SCALE_ITEM(-1200, 0,  1),
> 	3, 3, TLV_DB_SCALE_ITEM(-600, 0, 0),
> 	2, 2, TLV_DB_SCALE_ITEM(0, 0, 0),
> 	1, 1, TLV_DB_SCALE_ITEM(600, 0, 0),
> };

> I'm not sure about the tlv declaration, but is there a way to actually map the 
> gain/power control in the TWL in a standard way?

I'd expect that to work in that userspace will see the various values
with the expected gains but I'd not expect that applications would
reorder the values so it'll look odd in the UI.  But I've not actually
tried it.

Re: [PATCH] ASoC: TWL4030: PM fix for output amplifiers

[Peter Ujfalusi]

On Tuesday 23 March 2010 12:02:10 ext Mark Brown wrote:
> On Tue, Mar 23, 2010 at 09:59:13AM +0200, Peter Ujfalusi wrote:
> > /*
> > 
> >  * Gain controls tied to outputs
> >  * -6 dB to 6 dB in 6 dB steps (mute instead of -12)
> >  */
> > 
> > static const unsigned int output_tlv[] = {
> > 
> > 	TLV_DB_RANGE_HEAD(4),
> > 	0, 0, TLV_DB_SCALE_ITEM(-1200, 0,  1),
> > 	3, 3, TLV_DB_SCALE_ITEM(-600, 0, 0),
> > 	2, 2, TLV_DB_SCALE_ITEM(0, 0, 0),
> > 	1, 1, TLV_DB_SCALE_ITEM(600, 0, 0),
> > 
> > };
> > 
> > I'm not sure about the tlv declaration, but is there a way to actually
> > map the gain/power control in the TWL in a standard way?
> 
> I'd expect that to work in that userspace will see the various values
> with the expected gains but I'd not expect that applications would
> reorder the values so it'll look odd in the UI.  But I've not actually
> tried it.

Indeed it look odd at the end:

amixer sset 'PreDriv' 0
  Front Left: Playback 0 [0%] [-99999.99dB]
  Front Right: Playback 0 [0%] [-99999.99dB]

amixer sset 'PreDriv' 1
  Front Left: Playback 1 [33%] [6.00dB]
  Front Right: Playback 1 [33%] [6.00dB]

amixer sset 'PreDriv' 2
  Front Left: Playback 2 [67%] [0.00dB]
  Front Right: Playback 2 [67%] [0.00dB]

amixer sset 'PreDriv' 3
  Front Left: Playback 3 [100%] [-6.00dB]
  Front Right: Playback 3 [100%] [-6.00dB

It worth a try anyway.

-- 
Péter