[PATCH -next] ASoC: pandora: Add DAC regulator support

[PATCH -next] ASoC: pandora: Add DAC regulator support

[Grazvydas Ignotas]

Pandora's external DAC is connected to VSIM TWL4030 supply, so let's
start switching it too to save more power.

Also DAC got it's own DAPM handler and the delay was removed
(it gave on audible improvement).

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 sound/soc/omap/omap3pandora.c |   37 +++++++++++++++++++++++++++++++------
 1 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/sound/soc/omap/omap3pandora.c b/sound/soc/omap/omap3pandora.c
index 68980c1..b978411 100644
--- a/sound/soc/omap/omap3pandora.c
+++ b/sound/soc/omap/omap3pandora.c
@@ -22,7 +22,7 @@
 #include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
-#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 #include <sound/core.h>
 #include <sound/pcm.h>
@@ -40,6 +40,8 @@
 
 #define PREFIX "ASoC omap3pandora: "
 
+static struct regulator *omap3pandora_dac_reg;
+
 static int omap3pandora_cmn_hw_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_params *params, unsigned int fmt)
 {
@@ -106,21 +108,31 @@ static int omap3pandora_in_hw_params(struct snd_pcm_substream *substream,
 					  SND_SOC_DAIFMT_CBS_CFS);
 }
 
-static int omap3pandora_hp_event(struct snd_soc_dapm_widget *w,
+static int omap3pandora_dac_event(struct snd_soc_dapm_widget *w,
 	struct snd_kcontrol *k, int event)
 {
 	if (SND_SOC_DAPM_EVENT_ON(event)) {
+		regulator_enable(omap3pandora_dac_reg);
 		gpio_set_value(OMAP3_PANDORA_DAC_POWER_GPIO, 1);
-		gpio_set_value(OMAP3_PANDORA_AMP_POWER_GPIO, 1);
 	} else {
-		gpio_set_value(OMAP3_PANDORA_AMP_POWER_GPIO, 0);
-		mdelay(1);
 		gpio_set_value(OMAP3_PANDORA_DAC_POWER_GPIO, 0);
+		regulator_disable(omap3pandora_dac_reg);
 	}
 
 	return 0;
 }
 
+static int omap3pandora_hp_event(struct snd_soc_dapm_widget *w,
+	struct snd_kcontrol *k, int event)
+{
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		gpio_set_value(OMAP3_PANDORA_AMP_POWER_GPIO, 1);
+	else
+		gpio_set_value(OMAP3_PANDORA_AMP_POWER_GPIO, 0);
+
+	return 0;
+}
+
 /*
  * Audio paths on Pandora board:
  *
@@ -130,7 +142,9 @@ static int omap3pandora_hp_event(struct snd_soc_dapm_widget *w,
  *  |P| <--- TWL4030 <--------- Line In and MICs
  */
 static const struct snd_soc_dapm_widget omap3pandora_out_dapm_widgets[] = {
-	SND_SOC_DAPM_DAC("PCM DAC", "HiFi Playback", SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_DAC_E("PCM DAC", "HiFi Playback", SND_SOC_NOPM,
+			   0, 0, omap3pandora_dac_event,
+			   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
 	SND_SOC_DAPM_PGA_E("Headphone Amplifier", SND_SOC_NOPM,
 			   0, 0, NULL, 0, omap3pandora_hp_event,
 			   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
@@ -306,8 +320,18 @@ static int __init omap3pandora_soc_init(void)
 		goto fail2;
 	}
 
+	omap3pandora_dac_reg = regulator_get(&omap3pandora_snd_device->dev, "vcc");
+	if (IS_ERR(omap3pandora_dac_reg)) {
+		pr_err(PREFIX "Failed to get DAC regulator from %s: %ld\n",
+			dev_name(&omap3pandora_snd_device->dev),
+			PTR_ERR(omap3pandora_dac_reg));
+		goto fail3;
+	}
+
 	return 0;
 
+fail3:
+	platform_device_del(omap3pandora_snd_device);
 fail2:
 	platform_device_put(omap3pandora_snd_device);
 fail1:
@@ -320,6 +344,7 @@ module_init(omap3pandora_soc_init);
 
 static void __exit omap3pandora_soc_exit(void)
 {
+	regulator_put(omap3pandora_dac_reg);
 	platform_device_unregister(omap3pandora_snd_device);
 	gpio_free(OMAP3_PANDORA_AMP_POWER_GPIO);
 	gpio_free(OMAP3_PANDORA_DAC_POWER_GPIO);
-- 
1.6.3.3

Re: [PATCH -next] ASoC: pandora: Add DAC regulator support

[Liam Girdwood]

On Fri, 2010-02-05 at 15:13 +0200, Grazvydas Ignotas wrote:
> Pandora's external DAC is connected to VSIM TWL4030 supply, so let's
> start switching it too to save more power.
> 
> Also DAC got it's own DAPM handler and the delay was removed
> (it gave on audible improvement).
> 

That's good to hear (pun intended).

> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
>  sound/soc/omap/omap3pandora.c |   37 +++++++++++++++++++++++++++++++------
>  1 files changed, 31 insertions(+), 6 deletions(-)

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>

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

Re: [PATCH -next] ASoC: pandora: Add DAC regulator support

[Grazvydas Ignotas]

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

On Fri, Feb 5, 2010 at 4:08 PM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> On Fri, 2010-02-05 at 15:13 +0200, Grazvydas Ignotas wrote:
>> Pandora's external DAC is connected to VSIM TWL4030 supply, so let's
>> start switching it too to save more power.
>>
>> Also DAC got it's own DAPM handler and the delay was removed
>> (it gave on audible improvement).
>>
>
> That's good to hear (pun intended).

Uh, I meant "no audible improvement". However I've just checked the
DAC datasheet again and it requires 1ms delay between turning on/off
the supply and switching /PD pin (connected to
OMAP3_PANDORA_DAC_POWER_GPIO). Even though I hear no difference,
adding the delays back just in case. Updated patch attached.

[-- Attachment #2: 0001-ASoC-pandora-add-DAC-regulator-support.patch --]
[-- Type: text/x-patch, Size: 3718 bytes --]

From 72cdf36e432a9929baff8841df979f13492dc813 Mon Sep 17 00:00:00 2001
From: Grazvydas Ignotas <notasas@gmail.com>
Date: Fri, 5 Feb 2010 12:57:37 +0200
Subject: [PATCH -next] ASoC: pandora: Add DAC regulator support

Pandora's external DAC is connected to VSIM TWL4030 supply, so let's
start switching it too to save more power.
Also DAC got it's own DAPM handler.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 sound/soc/omap/omap3pandora.c |   42 ++++++++++++++++++++++++++++++++++++----
 1 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/sound/soc/omap/omap3pandora.c b/sound/soc/omap/omap3pandora.c
index 68980c1..de10f76 100644
--- a/sound/soc/omap/omap3pandora.c
+++ b/sound/soc/omap/omap3pandora.c
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
 #include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 #include <sound/core.h>
 #include <sound/pcm.h>
@@ -40,6 +41,8 @@
 
 #define PREFIX "ASoC omap3pandora: "
 
+static struct regulator *omap3pandora_dac_reg;
+
 static int omap3pandora_cmn_hw_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_params *params, unsigned int fmt)
 {
@@ -106,21 +109,37 @@ static int omap3pandora_in_hw_params(struct snd_pcm_substream *substream,
 					  SND_SOC_DAIFMT_CBS_CFS);
 }
 
-static int omap3pandora_hp_event(struct snd_soc_dapm_widget *w,
+static int omap3pandora_dac_event(struct snd_soc_dapm_widget *w,
 	struct snd_kcontrol *k, int event)
 {
+	/*
+	 * The PCM1773 DAC datasheet requires 1ms delay between switching
+	 * VCC power on/off and /PD pin high/low
+	 */
 	if (SND_SOC_DAPM_EVENT_ON(event)) {
+		regulator_enable(omap3pandora_dac_reg);
+		mdelay(1);
 		gpio_set_value(OMAP3_PANDORA_DAC_POWER_GPIO, 1);
-		gpio_set_value(OMAP3_PANDORA_AMP_POWER_GPIO, 1);
 	} else {
-		gpio_set_value(OMAP3_PANDORA_AMP_POWER_GPIO, 0);
-		mdelay(1);
 		gpio_set_value(OMAP3_PANDORA_DAC_POWER_GPIO, 0);
+		mdelay(1);
+		regulator_disable(omap3pandora_dac_reg);
 	}
 
 	return 0;
 }
 
+static int omap3pandora_hp_event(struct snd_soc_dapm_widget *w,
+	struct snd_kcontrol *k, int event)
+{
+	if (SND_SOC_DAPM_EVENT_ON(event))
+		gpio_set_value(OMAP3_PANDORA_AMP_POWER_GPIO, 1);
+	else
+		gpio_set_value(OMAP3_PANDORA_AMP_POWER_GPIO, 0);
+
+	return 0;
+}
+
 /*
  * Audio paths on Pandora board:
  *
@@ -130,7 +149,9 @@ static int omap3pandora_hp_event(struct snd_soc_dapm_widget *w,
  *  |P| <--- TWL4030 <--------- Line In and MICs
  */
 static const struct snd_soc_dapm_widget omap3pandora_out_dapm_widgets[] = {
-	SND_SOC_DAPM_DAC("PCM DAC", "HiFi Playback", SND_SOC_NOPM, 0, 0),
+	SND_SOC_DAPM_DAC_E("PCM DAC", "HiFi Playback", SND_SOC_NOPM,
+			   0, 0, omap3pandora_dac_event,
+			   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
 	SND_SOC_DAPM_PGA_E("Headphone Amplifier", SND_SOC_NOPM,
 			   0, 0, NULL, 0, omap3pandora_hp_event,
 			   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
@@ -306,8 +327,18 @@ static int __init omap3pandora_soc_init(void)
 		goto fail2;
 	}
 
+	omap3pandora_dac_reg = regulator_get(&omap3pandora_snd_device->dev, "vcc");
+	if (IS_ERR(omap3pandora_dac_reg)) {
+		pr_err(PREFIX "Failed to get DAC regulator from %s: %ld\n",
+			dev_name(&omap3pandora_snd_device->dev),
+			PTR_ERR(omap3pandora_dac_reg));
+		goto fail3;
+	}
+
 	return 0;
 
+fail3:
+	platform_device_del(omap3pandora_snd_device);
 fail2:
 	platform_device_put(omap3pandora_snd_device);
 fail1:
@@ -320,6 +351,7 @@ module_init(omap3pandora_soc_init);
 
 static void __exit omap3pandora_soc_exit(void)
 {
+	regulator_put(omap3pandora_dac_reg);
 	platform_device_unregister(omap3pandora_snd_device);
 	gpio_free(OMAP3_PANDORA_AMP_POWER_GPIO);
 	gpio_free(OMAP3_PANDORA_DAC_POWER_GPIO);
-- 
1.6.3.3


[-- 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 -next] ASoC: pandora: Add DAC regulator support

[Liam Girdwood]

On Fri, 2010-02-05 at 16:29 +0200, Grazvydas Ignotas wrote:
> On Fri, Feb 5, 2010 at 4:08 PM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> > On Fri, 2010-02-05 at 15:13 +0200, Grazvydas Ignotas wrote:
> >> Pandora's external DAC is connected to VSIM TWL4030 supply, so let's
> >> start switching it too to save more power.
> >>
> >> Also DAC got it's own DAPM handler and the delay was removed
> >> (it gave on audible improvement).
> >>
> >
> > That's good to hear (pun intended).
> 
> Uh, I meant "no audible improvement". However I've just checked the
> DAC datasheet again and it requires 1ms delay between turning on/off
> the supply and switching /PD pin (connected to
> OMAP3_PANDORA_DAC_POWER_GPIO). Even though I hear no difference,
> adding the delays back just in case. Updated patch attached.
> _______________________________________________

Btw, have you tried turning the volume to mute when the DAC is enabled.
It may lessen the pop at switch on.

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>

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

Re: [PATCH -next] ASoC: pandora: Add DAC regulator support

[Mark Brown]

On Fri, Feb 05, 2010 at 04:29:53PM +0200, Grazvydas Ignotas wrote:

> Uh, I meant "no audible improvement". However I've just checked the
> DAC datasheet again and it requires 1ms delay between turning on/off
> the supply and switching /PD pin (connected to
> OMAP3_PANDORA_DAC_POWER_GPIO). Even though I hear no difference,
> adding the delays back just in case. Updated patch attached.

Which step of the powerup is the one that generates the pop?  As Liam
says if you can mute the DAC while powering up might help if that's
possible, though looking I don't think it is.

> From 72cdf36e432a9929baff8841df979f13492dc813 Mon Sep 17 00:00:00 2001
> From: Grazvydas Ignotas <notasas@gmail.com>
> Date: Fri, 5 Feb 2010 12:57:37 +0200
> Subject: [PATCH -next] ASoC: pandora: Add DAC regulator support

Please don't append patches to messages like this, it makes them harder
to apply since the e-mail text ends up as part of the commit message as
does the patch header.

Applied, thanks.

Re: [PATCH -next] ASoC: pandora: Add DAC regulator support

[Grazvydas Ignotas]

On Fri, Feb 5, 2010 at 7:20 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Feb 05, 2010 at 04:29:53PM +0200, Grazvydas Ignotas wrote:
>
>> Uh, I meant "no audible improvement". However I've just checked the
>> DAC datasheet again and it requires 1ms delay between turning on/off
>> the supply and switching /PD pin (connected to
>> OMAP3_PANDORA_DAC_POWER_GPIO). Even though I hear no difference,
>> adding the delays back just in case. Updated patch attached.
>
> Which step of the powerup is the one that generates the pop?  As Liam
> says if you can mute the DAC while powering up might help if that's
> possible, though looking I don't think it is.

It's on powerdown, there is a faint pop. It appears its the amp (final
part in the path), I tried to toggle it while playing /dev/zero
manually and it always pops on powerdown. On powerup there is a small
sound cutoff too, probably because of amp capacitors charging, but we
decided to leave it instead of halting every sound playing program
until the amp gets ready.

>
>> From 72cdf36e432a9929baff8841df979f13492dc813 Mon Sep 17 00:00:00 2001
>> From: Grazvydas Ignotas <notasas@gmail.com>
>> Date: Fri, 5 Feb 2010 12:57:37 +0200
>> Subject: [PATCH -next] ASoC: pandora: Add DAC regulator support
>
> Please don't append patches to messages like this, it makes them harder
> to apply since the e-mail text ends up as part of the commit message as
> does the patch header.
>
> Applied, thanks.

noted, thanks.

Re: [PATCH -next] ASoC: pandora: Add DAC regulator support

[Jarkko Nikula]

On Sat, 6 Feb 2010 19:33:08 +0200
Grazvydas Ignotas <notasas@gmail.com> wrote:

> It's on powerdown, there is a faint pop. It appears its the amp (final
> part in the path), I tried to toggle it while playing /dev/zero
> manually and it always pops on powerdown. On powerup there is a small
> sound cutoff too, probably because of amp capacitors charging, but we
> decided to leave it instead of halting every sound playing program
> until the amp gets ready.
> 
My side comment for the /dev/zero test. You may hear pops also
if /dev/zero is played as unsigned 8-bit samples on signed DAC since
then samples gets converted to maximum negative value.

So there is a difference how the /dev/zero is played. First command
below produces a pop because it's playing a pop and second produces a
pop only if HW is producing it.

aplay /dev/zero
aplay -f S16_LE /dev/zero


-- 
Jarkko

Re: [PATCH -next] ASoC: pandora: Add DAC regulator support

[Mark Brown]

On Sat, Feb 06, 2010 at 07:33:08PM +0200, Grazvydas Ignotas wrote:
> On Fri, Feb 5, 2010 at 7:20 PM, Mark Brown

> > Which step of the powerup is the one that generates the pop?  As Liam
> > says if you can mute the DAC while powering up might help if that's
> > possible, though looking I don't think it is.

> It's on powerdown, there is a faint pop. It appears its the amp (final
> part in the path), I tried to toggle it while playing /dev/zero
> manually and it always pops on powerdown. On powerup there is a small

Like Jarkko says be careful what you're actually playing there -
especially given the ramp you appear to have at startup which would mask
any sudden DC offset in the samples at startup.

> sound cutoff too, probably because of amp capacitors charging, but we
> decided to leave it instead of halting every sound playing program
> until the amp gets ready.

If this goes off too much you might want to tweak the delay ASoC uses to
defer power down of DAPM widgets after streams stop.  There's a command
line option pmdown_time, it really ought to have a sysfs file to tweak
but there's not been any demand.

Re: [PATCH -next] ASoC: pandora: Add DAC regulator support

[Grazvydas Ignotas]

On Sun, Feb 7, 2010 at 11:07 AM, Jarkko Nikula <jhnikula@gmail.com> wrote:
> On Sat, 6 Feb 2010 19:33:08 +0200
> Grazvydas Ignotas <notasas@gmail.com> wrote:
>
>> It's on powerdown, there is a faint pop. It appears its the amp (final
>> part in the path), I tried to toggle it while playing /dev/zero
>> manually and it always pops on powerdown. On powerup there is a small
>> sound cutoff too, probably because of amp capacitors charging, but we
>> decided to leave it instead of halting every sound playing program
>> until the amp gets ready.
>>
> My side comment for the /dev/zero test. You may hear pops also
> if /dev/zero is played as unsigned 8-bit samples on signed DAC since
> then samples gets converted to maximum negative value.
>
> So there is a difference how the /dev/zero is played. First command
> below produces a pop because it's playing a pop and second produces a
> pop only if HW is producing it.
>
> aplay /dev/zero
> aplay -f S16_LE /dev/zero

I was testing with '-f cd' which should also be signed. Actually doing
just 'aplay /dev/zero' makes matters worse, there is some additional
'squeek' noise audible when I turn the volume up.

On Mon, Feb 8, 2010 at 1:39 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> If this goes off too much you might want to tweak the delay ASoC uses to
> defer power down of DAPM widgets after streams stop.  There's a command
> line option pmdown_time, it really ought to have a sysfs file to tweak
> but there's not been any demand.

Yes we are already using that (set to 1 minute), and the sysfs file
would be useful for us (we have a hack for that in our tree).