[PATCH] ASoC: WM8962: Enable start-up and normal bias after reset in runtime resume

[PATCH] ASoC: WM8962: Enable start-up and normal bias after reset in runtime resume

[Nicolin Chen]

The analogue circuits in WM8962 require a mid-rail analogue reference
voltage, VMID, which generator is enabled by STARTUP_BIAS_ENA bit,
while VMID_BUF_ENA bit can enable buffered VMID to avoid audible pops.

Besides, these circuits also need a bias current enabled by BIAS_ENA.

Then set VMID_SEL to 2x5k divider, the fast start-up mode, to power it
up. And let set_bias_level() handle its 2x250k for low power standby
and 2x50k for normal operation.

This patch might be kinda reverting operation against commit 52c0eee3
but after testing with WM8962 CODEC, these configurations during power
up should be essential and soc-core would not do any of them. So the
codec drvier need to handle it.

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
 sound/soc/codecs/wm8962.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index 7a7a056..c63b58e 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -3773,6 +3773,17 @@ static int wm8962_runtime_resume(struct device *dev)
 
 	regcache_sync(wm8962->regmap);
 
+	regmap_update_bits(wm8962->regmap, WM8962_ANTI_POP,
+			   WM8962_STARTUP_BIAS_ENA | WM8962_VMID_BUF_ENA,
+			   WM8962_STARTUP_BIAS_ENA | WM8962_VMID_BUF_ENA);
+
+	/* Bias enable at 2*5k (fast start-up) */
+	regmap_update_bits(wm8962->regmap, WM8962_PWR_MGMT_1,
+			   WM8962_BIAS_ENA | WM8962_VMID_SEL_MASK,
+			   WM8962_BIAS_ENA | 0x180);
+
+	msleep(5);
+
 	return 0;
 }
 
-- 
1.7.1

Re: [PATCH] ASoC: WM8962: Enable start-up and normal bias after reset in runtime resume

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 939 bytes --]

On Fri, Jun 14, 2013 at 04:52:20PM +0800, Nicolin Chen wrote:
> The analogue circuits in WM8962 require a mid-rail analogue reference
> voltage, VMID, which generator is enabled by STARTUP_BIAS_ENA bit,
> while VMID_BUF_ENA bit can enable buffered VMID to avoid audible pops.
> 
> Besides, these circuits also need a bias current enabled by BIAS_ENA.
> 
> Then set VMID_SEL to 2x5k divider, the fast start-up mode, to power it
> up. And let set_bias_level() handle its 2x250k for low power standby
> and 2x50k for normal operation.
> 
> This patch might be kinda reverting operation against commit 52c0eee3
> but after testing with WM8962 CODEC, these configurations during power
> up should be essential and soc-core would not do any of them. So the
> codec drvier need to handle it.

The biases should be being managed using set_bias_level() which will get
called automatically when audio is being started and stopped.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: WM8962: Enable start-up and normal bias after reset in runtime resume

[Nicolin Chen]

Hi,

On Fri, Jun 14, 2013 at 10:59:17AM +0100, Mark Brown wrote:
> The biases should be being managed using set_bias_level() which will get
> called automatically when audio is being started and stopped.

I understand your point because I also found other wm89xx drivers do this way.
But the truth is, originally this 8962 codec driver did put biases settings
into set_bias_level() as the others. But after commit d23031a4d they're put
into pm_resume().
So I can also see the point that it's reasonable to handle these settings in
pm_resume() since DAPM would call pm_runtime_get_sync(), so as snd_soc_open()
would though, pm_resume() would then finish the settings before the bias level
being set to STANDBY, according to dapm_pre_sequence_async() func.

As far as I can understand, I think these two approaches should be running
into a same consequence, while I'm not sure which one's better.

Sir, do you still think it's better to put them back into set_bias_level()?
Could you please help me understand the reason?

Thank you and looking forward to your reply.
Nicolin Chen

Re: [PATCH] ASoC: WM8962: Enable start-up and normal bias after reset in runtime resume

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1238 bytes --]

On Fri, Jun 14, 2013 at 06:08:39PM +0800, Nicolin Chen wrote:

> I understand your point because I also found other wm89xx drivers do this way.
> But the truth is, originally this 8962 codec driver did put biases settings
> into set_bias_level() as the others. But after commit d23031a4d they're put
> into pm_resume().

Don't just quote commit IDs, use the subject lines as well so humans can
read your e-mails.  The commit you're referring to moves some code out
of set_bias_level() into runtime PM.

> As far as I can understand, I think these two approaches should be running
> into a same consequence, while I'm not sure which one's better.

> Sir, do you still think it's better to put them back into set_bias_level()?
> Could you please help me understand the reason?

So, if that's the case why are you changing this...  it may be that what
you need to do is to write a better commit message so it's clear why
you're making the change, your message just described what the code
change does but not why it's being done in that way.

That said the general concept is that runtime PM would be powering on
the device for digital operation (eg, for the GPIOs) and then
set_bias_level() powering up the analogue portions of the device.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: WM8962: Enable start-up and normal bias after reset in runtime resume

[Nicolin Chen]

On Fri, Jun 14, 2013 at 11:51:18AM +0100, Mark Brown wrote:
> Don't just quote commit IDs, use the subject lines as well so humans can
> read your e-mails.  The commit you're referring to moves some code out
> of set_bias_level() into runtime PM.

I'm sorry, I'll obey the rule from now on.

> So, if that's the case why are you changing this...  it may be that what
> you need to do is to write a better commit message so it's clear why
> you're making the change, your message just described what the code
> change does but not why it's being done in that way.

You're right. I'll revise it.

> That said the general concept is that runtime PM would be powering on
> the device for digital operation (eg, for the GPIOs) and then
> set_bias_level() powering up the analogue portions of the device.

Understood. But I just tested the set_bias_level() way to handle BIAS,
it prompts "wm8962 0-001a: DC servo timed out" when I start to play a
wav file. There should be still some parts of code not right.
So I'm gonna just revise the comment to continue the resume() methods.
It should be appropriate since pm_suspend() is handling bias-off in
the current code.

If you still can't accept it, I will then trace the code next week.
It's Friday night here, and I'm starving now :(

Thank you,
Nicolin Chen

Re: [PATCH] ASoC: WM8962: Enable start-up and normal bias after reset in runtime resume

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1009 bytes --]

On Fri, Jun 14, 2013 at 07:07:43PM +0800, Nicolin Chen wrote:
> On Fri, Jun 14, 2013 at 11:51:18AM +0100, Mark Brown wrote:

> > That said the general concept is that runtime PM would be powering on
> > the device for digital operation (eg, for the GPIOs) and then
> > set_bias_level() powering up the analogue portions of the device.

> Understood. But I just tested the set_bias_level() way to handle BIAS,
> it prompts "wm8962 0-001a: DC servo timed out" when I start to play a
> wav file. There should be still some parts of code not right.
> So I'm gonna just revise the comment to continue the resume() methods.
> It should be appropriate since pm_suspend() is handling bias-off in
> the current code.

If the DC servo is timing out that'll be a problem with the clocking...
the FLL is partially analogue so if you're using the FLL that makes
sense.  Looking at the code we're also bringing the bias down in the
suspend path so it needs to be powered up in the resume path for
symmetry if nothing else.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: WM8962: Enable start-up and normal bias after reset in runtime resume

[Nicolin Chen]

On Fri, Jun 14, 2013 at 12:21:10PM +0100, Mark Brown wrote:
> If the DC servo is timing out that'll be a problem with the clocking...
> the FLL is partially analogue so if you're using the FLL that makes
> sense.  Looking at the code we're also bringing the bias down in the
> suspend path so it needs to be powered up in the resume path for
> symmetry if nothing else.

Thank you for helping me understand the case. And just sent the v2.

Hope you can apply it.
Nicolin Chen