[PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
---
 sound/soc/codecs/cs4270.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index 8069023..b1d4b5d 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -28,6 +28,7 @@
 #include <sound/initval.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 #include "cs4270.h"
 
@@ -114,6 +115,7 @@ struct cs4270_private {
 	unsigned int mode; /* The mode (I2S or left-justified) */
 	unsigned int slave_mode;
 	unsigned int manual_mute;
+	struct regulator *va_reg;
 };
 
 /**
@@ -462,27 +464,38 @@ static int cs4270_hw_params(struct snd_pcm_substream *substream,
  * @dai: the SOC DAI
  * @mute: 0 = disable mute, 1 = enable mute
  *
- * This function toggles the mute bits in the MUTE register.  The CS4270's
+ * This function toggles the mute bits in the MUTE register and switches
+ * the registered VA power regulator, if it was registered. The CS4270's
  * mute capability is intended for external muting circuitry, so if the
  * board does not have the MUTEA or MUTEB pins connected to such circuitry,
- * then this function will do nothing.
+ * and no power regulator was specified, then this function will do nothing.
  */
 static int cs4270_dai_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_codec *codec = dai->codec;
 	struct cs4270_private *cs4270 = codec->private_data;
-	int reg6;
+	int reg6, err;
 
 	reg6 = snd_soc_read(codec, CS4270_MUTE);
 
 	if (mute)
 		reg6 |= CS4270_MUTE_DAC_A | CS4270_MUTE_DAC_B;
 	else {
+		if (cs4270->va_reg)
+			regulator_enable(cs4270->va_reg);
+
 		reg6 &= ~(CS4270_MUTE_DAC_A | CS4270_MUTE_DAC_B);
 		reg6 |= cs4270->manual_mute;
 	}
 
-	return snd_soc_write(codec, CS4270_MUTE, reg6);
+	err = snd_soc_write(codec, CS4270_MUTE, reg6);
+	if (err)
+		return err;
+
+	if (cs4270->va_reg && mute)
+		regulator_disable(cs4270->va_reg);
+
+	return 0;
 }
 
 /**
@@ -579,6 +592,7 @@ static int cs4270_probe(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 	int ret;
 
 	/* Connect the codec to the socdev.  snd_soc_new_pcms() needs this. */
@@ -606,6 +620,14 @@ static int cs4270_probe(struct platform_device *pdev)
 		goto error_free_pcms;
 	}
 
+#ifdef CONFIG_REGULATOR
+	/* get the regulator if there is one read<y for us */
+	cs4270->va_reg = regulator_get(&pdev->dev, "va");
+
+	if (IS_ERR(cs4270->va_reg))
+		cs4270->va_reg = NULL;
+#endif
+
 	return 0;
 
 error_free_pcms:
@@ -623,9 +645,14 @@ error_free_pcms:
 static int cs4270_remove(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 
 	snd_soc_free_pcms(socdev);
 
+	if (cs4270->va_reg)
+		regulator_put(cs4270->va_reg);
+
 	return 0;
 };
 
-- 
1.6.5.2

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Wed, Nov 25, 2009 at 03:36:27PM +0100, Daniel Mack wrote:

> +	struct regulator *va_reg;

This should be three different supplies - there's separate digital and
control supplies, as well as a buffer supply.  Board designs frequently
split the analog and digital supplies.  The regulator API has bulk_
variants intended to make using multiple supplies en masse easier.

>  static int cs4270_dai_mute(struct snd_soc_dai *dai, int mute)
>  {
>  	struct snd_soc_codec *codec = dai->codec;
>  	struct cs4270_private *cs4270 = codec->private_data;
> -	int reg6;
> +	int reg6, err;
>  
>  	reg6 = snd_soc_read(codec, CS4270_MUTE);
>  
>  	if (mute)
>  		reg6 |= CS4270_MUTE_DAC_A | CS4270_MUTE_DAC_B;
>  	else {
> +		if (cs4270->va_reg)
> +			regulator_enable(cs4270->va_reg);
> +

This looks wrong - why is the power being controlled in the mute
function?  If nothing else this is going to break recording since the
CODEC will only be unmuted during playback which means power will be cut
during record.

> +#ifdef CONFIG_REGULATOR
> +	/* get the regulator if there is one read<y for us */
> +	cs4270->va_reg = regulator_get(&pdev->dev, "va");
> +
> +	if (IS_ERR(cs4270->va_reg))
> +		cs4270->va_reg = NULL;
> +#endif

This isn't needed, the regulator API stubs itself out so you can just
unconditionally use it in consumer drivers.  The same applies to the
enable/disable calls, a simple driver like this should never need to
check to see if the API is enabled (this is why you don't need to ifdef
out the calls in the mute function).

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Wed, Nov 25, 2009 at 03:01:39PM +0000, Mark Brown wrote:
> On Wed, Nov 25, 2009 at 03:36:27PM +0100, Daniel Mack wrote:
> 
> >  static int cs4270_dai_mute(struct snd_soc_dai *dai, int mute)
> >  {
> >  	struct snd_soc_codec *codec = dai->codec;
> >  	struct cs4270_private *cs4270 = codec->private_data;
> > -	int reg6;
> > +	int reg6, err;
> >  
> >  	reg6 = snd_soc_read(codec, CS4270_MUTE);
> >  
> >  	if (mute)
> >  		reg6 |= CS4270_MUTE_DAC_A | CS4270_MUTE_DAC_B;
> >  	else {
> > +		if (cs4270->va_reg)
> > +			regulator_enable(cs4270->va_reg);
> > +
> 
> This looks wrong - why is the power being controlled in the mute
> function?  If nothing else this is going to break recording since the
> CODEC will only be unmuted during playback which means power will be cut
> during record.

Ok - which place would you suggest for it? Is there an ASoC callback I
can hook on to tell me when the whole codec isn't used anymore? I can
only see startup/shutdown, but I would need to my own snd_pc_substream
handling login in there. Other drivers do that in the probe/remove
functions, but that won't suffice for my board as we want VA disabled
whenever possible.

Daniel

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Wed, Nov 25, 2009 at 04:20:52PM +0100, Daniel Mack wrote:

> Ok - which place would you suggest for it? Is there an ASoC callback I
> can hook on to tell me when the whole codec isn't used anymore? I can
> only see startup/shutdown, but I would need to my own snd_pc_substream
> handling login in there. Other drivers do that in the probe/remove

set_bias_level().  If the device is idle then the bias will be held in
STANDBY (or OFF in future).  If you are happy switching off the analogue
supply separately to the others then turn that one on when in PREPARE or
ON, and kill the others only in OFF.

> functions, but that won't suffice for my board as we want VA disabled
> whenever possible.

Are the power savings really that great?  If the power saving really is
that critical it seems like a low power focused part might be a more
natural choice...

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Wed, Nov 25, 2009 at 03:38:04PM +0000, Mark Brown wrote:
> On Wed, Nov 25, 2009 at 04:20:52PM +0100, Daniel Mack wrote:
> 
> > Ok - which place would you suggest for it? Is there an ASoC callback I
> > can hook on to tell me when the whole codec isn't used anymore? I can
> > only see startup/shutdown, but I would need to my own snd_pc_substream
> > handling login in there. Other drivers do that in the probe/remove
> 
> set_bias_level().  If the device is idle then the bias will be held in
> STANDBY (or OFF in future).  If you are happy switching off the analogue
> supply separately to the others then turn that one on when in PREPARE or
> ON, and kill the others only in OFF.

Ok, will do and resend later. Thanks!

> > functions, but that won't suffice for my board as we want VA disabled
> > whenever possible.
> 
> Are the power savings really that great?  If the power saving really is
> that critical it seems like a low power focused part might be a more
> natural choice...

The VA suplpy domain not only powers the codec but a bunch of OPAMPs and
the like as well. And yes, for all these, the power saving is
significant.

Daniel

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

Hi Mark,

On Wed, Nov 25, 2009 at 03:38:04PM +0000, Mark Brown wrote:
> On Wed, Nov 25, 2009 at 04:20:52PM +0100, Daniel Mack wrote:
> 
> > Ok - which place would you suggest for it? Is there an ASoC callback I
> > can hook on to tell me when the whole codec isn't used anymore? I can
> > only see startup/shutdown, but I would need to my own snd_pc_substream
> > handling login in there. Other drivers do that in the probe/remove
> 
> set_bias_level().  If the device is idle then the bias will be held in
> STANDBY (or OFF in future).  If you are happy switching off the analogue
> supply separately to the others then turn that one on when in PREPARE or
> ON, and kill the others only in OFF.

I implemented that now and it seems to work fine. However, I'm unsure
about two details:

- I only added VA for now because when VD is disabled, we need to
  restore all codec information, and I can't test that here. We can
  still add that later, right?

- I'm tracking the va_reg enable state inside the driver to take anyone
  else's reference. Is that the way do do it?

Thanks,
Daniel


>From ab10605ccad8d4409e541deb530b03e418435b8f Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Wed, 25 Nov 2009 15:31:25 +0100
Subject: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
---
 sound/soc/codecs/cs4270.c |   62 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index 8069023..acbe961 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -28,6 +28,7 @@
 #include <sound/initval.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 #include "cs4270.h"
 
@@ -114,6 +115,10 @@ struct cs4270_private {
 	unsigned int mode; /* The mode (I2S or left-justified) */
 	unsigned int slave_mode;
 	unsigned int manual_mute;
+
+	/* power domain regulators (optional) */
+	struct regulator *va_reg;
+	bool va_reg_enabled;
 };
 
 /**
@@ -458,6 +463,42 @@ static int cs4270_hw_params(struct snd_pcm_substream *substream,
 }
 
 /**
+ * cs4270_set_bias_level - catches BIAS level changes
+ * @dai: the SOC DAI
+ * @level: the new BIAS level
+ *
+ * This function controls the voltage regulators to properly
+ * power up/down the voltage regulator domains
+ */
+
+static int cs4270_set_bias_level(struct snd_soc_codec *codec,
+				 enum snd_soc_bias_level level)
+{
+	struct cs4270_private *cs4270 = codec->private_data;
+
+	switch (level) {
+	case SND_SOC_BIAS_ON:
+	case SND_SOC_BIAS_PREPARE:
+		/* Full power on */
+		if (cs4270->va_reg && !cs4270->va_reg_enabled) {
+			regulator_enable(cs4270->va_reg);
+			cs4270->va_reg_enabled = true;
+		}
+		break;
+	case SND_SOC_BIAS_STANDBY:
+	case SND_SOC_BIAS_OFF:
+		if (cs4270->va_reg && cs4270->va_reg_enabled) {
+			regulator_disable(cs4270->va_reg);
+			cs4270->va_reg_enabled = false;
+		}
+		break;
+	}
+
+	codec->bias_level = level;
+	return 0;
+}
+
+/**
  * cs4270_dai_mute - enable/disable the CS4270 external mute
  * @dai: the SOC DAI
  * @mute: 0 = disable mute, 1 = enable mute
@@ -579,6 +620,7 @@ static int cs4270_probe(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 	int ret;
 
 	/* Connect the codec to the socdev.  snd_soc_new_pcms() needs this. */
@@ -599,6 +641,13 @@ static int cs4270_probe(struct platform_device *pdev)
 		goto error_free_pcms;
 	}
 
+	/* get the power supply regulators (optional) */
+	cs4270->va_reg = regulator_get(&pdev->dev, "va");
+	if (IS_ERR(cs4270->va_reg))
+		cs4270->va_reg = NULL;
+
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
 	/* And finally, register the socdev */
 	ret = snd_soc_init_card(socdev);
 	if (ret < 0) {
@@ -623,9 +672,13 @@ error_free_pcms:
 static int cs4270_remove(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 
 	snd_soc_free_pcms(socdev);
 
+	regulator_put(cs4270->va_reg);
+
 	return 0;
 };
 
@@ -699,6 +752,8 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client,
 	codec->write = cs4270_i2c_write;
 	codec->reg_cache = cs4270->reg_cache;
 	codec->reg_cache_size = CS4270_NUMREGS;
+	codec->bias_level = SND_SOC_BIAS_OFF;
+	codec->set_bias_level = cs4270_set_bias_level;
 
 	/* The I2C interface is set up, so pre-fill our register cache */
 
@@ -808,6 +863,8 @@ static int cs4270_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
 	struct cs4270_private *cs4270 = i2c_get_clientdata(client);
 	struct snd_soc_codec *codec = &cs4270->codec;
 
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_OFF);
+
 	return snd_soc_suspend_device(codec->dev);
 }
 
@@ -816,6 +873,11 @@ static int cs4270_i2c_resume(struct i2c_client *client)
 	struct cs4270_private *cs4270 = i2c_get_clientdata(client);
 	struct snd_soc_codec *codec = &cs4270->codec;
 
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
+	if (codec->suspend_bias_level == SND_SOC_BIAS_ON)
+		cs4270_set_bias_level(codec, SND_SOC_BIAS_ON);
+
 	return snd_soc_resume_device(codec->dev);
 }
 
-- 
1.6.5.2

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Thu, Nov 26, 2009 at 04:48:07PM +0100, Daniel Mack wrote:

> - I only added VA for now because when VD is disabled, we need to
>   restore all codec information, and I can't test that here. We can
>   still add that later, right?

I'd suggest just adding it blind.  The code is very simple and it's much
easier to fix if there's a problem than it is to go through retrofit the
additional regulator hookups to boards if the other supplies are added
later.

You can test the flow by defining a fixed voltage regulator with no soft
control, obviously it won't actually turn the power on or off but the
code will run.

> - I'm tracking the va_reg enable state inside the driver to take anyone
>   else's reference. Is that the way do do it?

Yes.  You can also check if you're doing a PREPARE<->STANDBY transition
but either way works.  Existing drivers only do the enables/disables in
off which makes this really easy.

> +	/* get the power supply regulators (optional) */
> +	cs4270->va_reg = regulator_get(&pdev->dev, "va");
> +	if (IS_ERR(cs4270->va_reg))
> +		cs4270->va_reg = NULL;

Just fail if you can't get the supply.  If you can't get the regulator
then you've no analogue supply and the CODEC isn't going to work
terribly well.  Currently the assumption is that if you've built in the
regulator API you intend to use it, otherwise it's very hard to tell if
the operation failed and broke something or failed because the API isn't
in use.

>  	struct cs4270_private *cs4270 = i2c_get_clientdata(client);
>  	struct snd_soc_codec *codec = &cs4270->codec;
>  
> +	cs4270_set_bias_level(codec, SND_SOC_BIAS_OFF);
> +
>  	return snd_soc_suspend_device(codec->dev);
>  }

This patch isn't against current for-2.6.33 - those functions have been
removed.  Better to do the bias management in the ASoC suspend/resume
callbacks anyway so that it's joined up with the register save/restore.
Until we get the pm_link stuff in the next merge window there's no link
between the I2C bus suspend and the platform bus suspend for the ASoC
core.

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Thu, Nov 26, 2009 at 04:03:51PM +0000, Mark Brown wrote:
> On Thu, Nov 26, 2009 at 04:48:07PM +0100, Daniel Mack wrote:
> 
> > - I only added VA for now because when VD is disabled, we need to
> >   restore all codec information, and I can't test that here. We can
> >   still add that later, right?
> 
> I'd suggest just adding it blind.  The code is very simple and it's much
> easier to fix if there's a problem than it is to go through retrofit the
> additional regulator hookups to boards if the other supplies are added
> later.
> 
> You can test the flow by defining a fixed voltage regulator with no soft
> control, obviously it won't actually turn the power on or off but the
> code will run.

Ok. I couldn't find a dummy framework for such cases, and registering a
full regulator just to satisfy the codec code seems a litte cumbersome.
Is there anything like that? Or should there be?

> Just fail if you can't get the supply.  If you can't get the regulator
> then you've no analogue supply and the CODEC isn't going to work
> terribly well.  Currently the assumption is that if you've built in the
> regulator API you intend to use it, otherwise it's very hard to tell if
> the operation failed and broke something or failed because the API isn't
> in use.

Hmm - that will break all existing platforms that use this codec and
need regulators for other drivers. But ok, I'm fine with forcing
everyone to innovation :)

> >  	struct cs4270_private *cs4270 = i2c_get_clientdata(client);
> >  	struct snd_soc_codec *codec = &cs4270->codec;
> >  
> > +	cs4270_set_bias_level(codec, SND_SOC_BIAS_OFF);
> > +
> >  	return snd_soc_suspend_device(codec->dev);
> >  }
> 
> This patch isn't against current for-2.6.33 - those functions have been
> removed.  Better to do the bias management in the ASoC suspend/resume
> callbacks anyway so that it's joined up with the register save/restore.
> Until we get the pm_link stuff in the next merge window there's no link
> between the I2C bus suspend and the platform bus suspend for the ASoC
> core.

Ok, done - see the new patch below. I did use the regulator_bulk
functions in the first place, but due to the VA special case, I had to
extract the single regulators again from the bulk struct which turned
out to mess the code quite a bit. Looks better this way.

Thanks,
Daniel


>From dedbbb7ee101f635e9682559d31e48b4c57722a3 Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Wed, 25 Nov 2009 15:31:25 +0100
Subject: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
---
 sound/soc/codecs/cs4270.c |   90 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index 8069023..e79d30d 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -28,6 +28,7 @@
 #include <sound/initval.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 #include "cs4270.h"
 
@@ -114,6 +115,11 @@ struct cs4270_private {
 	unsigned int mode; /* The mode (I2S or left-justified) */
 	unsigned int slave_mode;
 	unsigned int manual_mute;
+
+	/* power domain regulators (optional) */
+	struct regulator *va_reg;
+	struct regulator *vd_reg;
+	struct regulator *vlc_reg;
 };
 
 /**
@@ -458,6 +464,56 @@ static int cs4270_hw_params(struct snd_pcm_substream *substream,
 }
 
 /**
+ * cs4270_set_bias_level - catches BIAS level changes
+ * @dai: the SOC DAI
+ * @level: the new BIAS level
+ *
+ * This function controls the voltage regulators to properly
+ * power up/down the voltage regulator domains
+ */
+
+static int cs4270_set_bias_level(struct snd_soc_codec *codec,
+				 enum snd_soc_bias_level level)
+{
+	int ret;
+	struct cs4270_private *cs4270 = codec->private_data;
+
+	switch (level) {
+	case SND_SOC_BIAS_ON:
+		ret = regulator_enable(cs4270->vd_reg);
+		if (ret < 0)
+			return ret;
+
+		ret = regulator_enable(cs4270->vlc_reg);
+		if (ret < 0)
+			return ret;
+		/* fall thru */
+	case SND_SOC_BIAS_PREPARE:
+		ret = regulator_enable(cs4270->va_reg);
+		if (ret < 0)
+			return ret;
+		break;
+	case SND_SOC_BIAS_STANDBY:
+		if (codec->bias_level == SND_SOC_BIAS_OFF) {
+			/* OFF -> STANDBY */
+			ret = regulator_enable(cs4270->va_reg);
+			if (ret < 0)
+				return ret;
+		} else
+			regulator_disable(cs4270->va_reg);
+		break;
+	case SND_SOC_BIAS_OFF:
+		regulator_disable(cs4270->va_reg);
+		regulator_disable(cs4270->vd_reg);
+		regulator_disable(cs4270->vlc_reg);
+		break;
+	}
+
+	codec->bias_level = level;
+	return 0;
+}
+
+/**
  * cs4270_dai_mute - enable/disable the CS4270 external mute
  * @dai: the SOC DAI
  * @mute: 0 = disable mute, 1 = enable mute
@@ -579,6 +635,7 @@ static int cs4270_probe(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 	int ret;
 
 	/* Connect the codec to the socdev.  snd_soc_new_pcms() needs this. */
@@ -599,6 +656,20 @@ static int cs4270_probe(struct platform_device *pdev)
 		goto error_free_pcms;
 	}
 
+	/* get the power supply regulators */
+	cs4270->va_reg = regulator_get(&pdev->dev, "va");
+	cs4270->vd_reg = regulator_get(&pdev->dev, "vd");
+	cs4270->vlc_reg = regulator_get(&pdev->dev, "vlc");
+
+	if (IS_ERR(cs4270->va_reg) ||
+	    IS_ERR(cs4270->vd_reg) ||
+	    IS_ERR(cs4270->vlc_reg)) {
+		dev_err(codec->dev, "failed to get supplies %s\n", dev_name(codec->dev));
+		goto error_free_pcms;
+	}
+
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
 	/* And finally, register the socdev */
 	ret = snd_soc_init_card(socdev);
 	if (ret < 0) {
@@ -609,6 +680,10 @@ static int cs4270_probe(struct platform_device *pdev)
 	return 0;
 
 error_free_pcms:
+	regulator_put(cs4270->va_reg);
+	regulator_put(cs4270->vd_reg);
+	regulator_put(cs4270->vlc_reg);
+
 	snd_soc_free_pcms(socdev);
 
 	return ret;
@@ -623,9 +698,15 @@ error_free_pcms:
 static int cs4270_remove(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 
 	snd_soc_free_pcms(socdev);
 
+	regulator_put(cs4270->va_reg);
+	regulator_put(cs4270->vd_reg);
+	regulator_put(cs4270->vlc_reg);
+
 	return 0;
 };
 
@@ -699,6 +780,8 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client,
 	codec->write = cs4270_i2c_write;
 	codec->reg_cache = cs4270->reg_cache;
 	codec->reg_cache_size = CS4270_NUMREGS;
+	codec->bias_level = SND_SOC_BIAS_OFF;
+	codec->set_bias_level = cs4270_set_bias_level;
 
 	/* The I2C interface is set up, so pre-fill our register cache */
 
@@ -824,6 +907,8 @@ static int cs4270_soc_suspend(struct platform_device *pdev, pm_message_t mesg)
 	struct snd_soc_codec *codec = cs4270_codec;
 	int reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
 
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_OFF);
+
 	return snd_soc_write(codec, CS4270_PWRCTL, reg);
 }
 
@@ -833,6 +918,11 @@ static int cs4270_soc_resume(struct platform_device *pdev)
 	struct i2c_client *i2c_client = codec->control_data;
 	int reg;
 
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
+	if (codec->suspend_bias_level == SND_SOC_BIAS_ON)
+		cs4270_set_bias_level(codec, SND_SOC_BIAS_ON);
+
 	/* In case the device was put to hard reset during sleep, we need to
 	 * wait 500ns here before any I2C communication. */
 	ndelay(500);
-- 
1.6.5.2

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Thu, Nov 26, 2009 at 06:42:45PM +0100, Daniel Mack wrote:
> On Thu, Nov 26, 2009 at 04:03:51PM +0000, Mark Brown wrote:

> > You can test the flow by defining a fixed voltage regulator with no soft
> > control, obviously it won't actually turn the power on or off but the
> > code will run.

> Ok. I couldn't find a dummy framework for such cases, and registering a
> full regulator just to satisfy the codec code seems a litte cumbersome.
> Is there anything like that? Or should there be?

That's what the fixed voltage regulator was there for originally - the
GPIO is a later additional and is optional.

> > terribly well.  Currently the assumption is that if you've built in the
> > regulator API you intend to use it, otherwise it's very hard to tell if
> > the operation failed and broke something or failed because the API isn't
> > in use.

> Hmm - that will break all existing platforms that use this codec and
> need regulators for other drivers. But ok, I'm fine with forcing
> everyone to innovation :)

It's difficult to do much more without making driver code using
regulators more cumbersome - the need to figure out of the error is a
real one or due to partial API usage makes things painful.  If this were
supported it'd be better to do it with a fudge in the API rather than in
driver code, we really want to keep the drivers as simple as possible to
reduce the burden on them.

> Ok, done - see the new patch below. I did use the regulator_bulk
> functions in the first place, but due to the VA special case, I had to
> extract the single regulators again from the bulk struct which turned
> out to mess the code quite a bit. Looks better this way.

Yes, due to the one special regulator you have the bulk functions don't
help here.

> +
> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +		ret = regulator_enable(cs4270->vd_reg);
> +		if (ret < 0)
> +			return ret;
> +		ret = regulator_enable(cs4270->vlc_reg);
> +		if (ret < 0)
> +			return ret;
> +		/* fall thru */

I'd expect these to just be enabled on OFF->STANDBY (or in the suspend
code indeed) - you'll need to restore the register cache after the
digital comes up anyway.

Also, you'll leak references since you only turn them off when going
into OFF but enable them every time you go to ON.

> +	case SND_SOC_BIAS_STANDBY:
> +		if (codec->bias_level == SND_SOC_BIAS_OFF) {
> +			/* OFF -> STANDBY */
> +			ret = regulator_enable(cs4270->va_reg);
> +			if (ret < 0)
> +				return ret;
> +		} else
> +			regulator_disable(cs4270->va_reg);
> +		break;

Moving this into PREPARE and checking if the previous level was STANDBY
is probably more what you mean, otherwise the audio will be burning
power from startup until the first audio has played.

>  
> +	cs4270_set_bias_level(codec, SND_SOC_BIAS_OFF);
> +
>  	return snd_soc_write(codec, CS4270_PWRCTL, reg);

I think you need to reverse these so that you turn off the supplies
after you've done the soft power off.

> @@ -833,6 +918,11 @@ static int cs4270_soc_resume(struct platform_device *pdev)
>  	struct i2c_client *i2c_client = codec->control_data;
>  	int reg;
>  
> +	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> +
> +	if (codec->suspend_bias_level == SND_SOC_BIAS_ON)
> +		cs4270_set_bias_level(codec, SND_SOC_BIAS_ON);
> +

The core should take care of the BIAS_ON bit for you these days (some
drivers do do it by hand for historical reasons or because they're being
a bit naughty with the bias levels).

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Fri, Nov 27, 2009 at 11:25:51AM +0000, Mark Brown wrote:
> On Thu, Nov 26, 2009 at 06:42:45PM +0100, Daniel Mack wrote:
> > +
> > +	switch (level) {
> > +	case SND_SOC_BIAS_ON:
> > +		ret = regulator_enable(cs4270->vd_reg);
> > +		if (ret < 0)
> > +			return ret;
> > +		ret = regulator_enable(cs4270->vlc_reg);
> > +		if (ret < 0)
> > +			return ret;
> > +		/* fall thru */
> 
> I'd expect these to just be enabled on OFF->STANDBY (or in the suspend
> code indeed) - you'll need to restore the register cache after the
> digital comes up anyway.
> 
> Also, you'll leak references since you only turn them off when going
> into OFF but enable them every time you go to ON.

Ok, agreed. Next try below.

Thanks,
Daniel


>From 2a36f34232b060ad6a12e4260e24d70d491e7dcb Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Wed, 25 Nov 2009 15:31:25 +0100
Subject: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
---
 sound/soc/codecs/cs4270.c |  114 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index 8069023..34cd97d 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -28,6 +28,7 @@
 #include <sound/initval.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 #include "cs4270.h"
 
@@ -114,6 +115,12 @@ struct cs4270_private {
 	unsigned int mode; /* The mode (I2S or left-justified) */
 	unsigned int slave_mode;
 	unsigned int manual_mute;
+
+	/* power domain regulators */
+	struct regulator *va_reg;
+	struct regulator *vd_reg;
+	struct regulator *vlc_reg;
+	bool va_reg_enabled;
 };
 
 /**
@@ -458,6 +465,50 @@ static int cs4270_hw_params(struct snd_pcm_substream *substream,
 }
 
 /**
+ * cs4270_set_bias_level - catches BIAS level changes
+ * @dai: the SOC DAI
+ * @level: the new BIAS level
+ *
+ * This function controls the voltage regulators to properly
+ * power up/down the voltage regulator domains
+ */
+
+static int cs4270_set_bias_level(struct snd_soc_codec *codec,
+				 enum snd_soc_bias_level level)
+{
+	int ret;
+	struct cs4270_private *cs4270 = codec->private_data;
+
+	switch (level) {
+	case SND_SOC_BIAS_ON:
+	case SND_SOC_BIAS_PREPARE:
+		if (!cs4270->va_reg_enabled) {
+			ret = regulator_enable(cs4270->va_reg);
+			if (ret < 0)
+				return ret;
+
+			cs4270->va_reg_enabled = true;
+		}
+
+		break;
+	case SND_SOC_BIAS_STANDBY:
+	case SND_SOC_BIAS_OFF:
+		if (cs4270->va_reg_enabled) {
+			ret = regulator_disable(cs4270->va_reg);
+			if (ret < 0)
+				return ret;
+
+			cs4270->va_reg_enabled = false;
+		}
+
+		break;
+	}
+
+	codec->bias_level = level;
+	return 0;
+}
+
+/**
  * cs4270_dai_mute - enable/disable the CS4270 external mute
  * @dai: the SOC DAI
  * @mute: 0 = disable mute, 1 = enable mute
@@ -579,6 +630,7 @@ static int cs4270_probe(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 	int ret;
 
 	/* Connect the codec to the socdev.  snd_soc_new_pcms() needs this. */
@@ -599,6 +651,25 @@ static int cs4270_probe(struct platform_device *pdev)
 		goto error_free_pcms;
 	}
 
+	/* get the power supply regulators */
+	cs4270->va_reg = regulator_get(codec->dev, "va");
+	cs4270->vd_reg = regulator_get(codec->dev, "vd");
+	cs4270->vlc_reg = regulator_get(codec->dev, "vlc");
+
+	if (IS_ERR(cs4270->va_reg) ||
+	    IS_ERR(cs4270->vd_reg) ||
+	    IS_ERR(cs4270->vlc_reg)) {
+		dev_err(codec->dev,
+			"failed to get supplies for device %s\n",
+			dev_name(codec->dev));
+		goto error_free_pcms;
+	}
+
+	regulator_enable(cs4270->vd_reg);
+	regulator_enable(cs4270->vlc_reg);
+
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
 	/* And finally, register the socdev */
 	ret = snd_soc_init_card(socdev);
 	if (ret < 0) {
@@ -609,6 +680,10 @@ static int cs4270_probe(struct platform_device *pdev)
 	return 0;
 
 error_free_pcms:
+	regulator_put(cs4270->va_reg);
+	regulator_put(cs4270->vd_reg);
+	regulator_put(cs4270->vlc_reg);
+
 	snd_soc_free_pcms(socdev);
 
 	return ret;
@@ -623,9 +698,21 @@ error_free_pcms:
 static int cs4270_remove(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 
 	snd_soc_free_pcms(socdev);
 
+	if (cs4270->va_reg_enabled)
+		regulator_disable(cs4270->va_reg);
+
+	regulator_disable(cs4270->vd_reg);
+	regulator_disable(cs4270->vlc_reg);
+
+	regulator_put(cs4270->va_reg);
+	regulator_put(cs4270->vd_reg);
+	regulator_put(cs4270->vlc_reg);
+
 	return 0;
 };
 
@@ -699,6 +786,8 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client,
 	codec->write = cs4270_i2c_write;
 	codec->reg_cache = cs4270->reg_cache;
 	codec->reg_cache_size = CS4270_NUMREGS;
+	codec->bias_level = SND_SOC_BIAS_OFF;
+	codec->set_bias_level = cs4270_set_bias_level;
 
 	/* The I2C interface is set up, so pre-fill our register cache */
 
@@ -822,17 +911,38 @@ static int cs4270_i2c_resume(struct i2c_client *client)
 static int cs4270_soc_suspend(struct platform_device *pdev, pm_message_t mesg)
 {
 	struct snd_soc_codec *codec = cs4270_codec;
-	int reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
+	struct cs4270_private *cs4270 = codec->private_data;
+	int reg, ret;
 
-	return snd_soc_write(codec, CS4270_PWRCTL, reg);
+	reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
+	if (reg < 0)
+		return reg;
+
+	ret = snd_soc_write(codec, CS4270_PWRCTL, reg);
+	if (ret < 0)
+		return ret;
+
+	ret = cs4270_set_bias_level(codec, SND_SOC_BIAS_OFF);
+	if (ret < 0)
+		return ret;
+
+	regulator_disable(cs4270->vd_reg);
+	regulator_disable(cs4270->vlc_reg);
+
+	return 0;
 }
 
 static int cs4270_soc_resume(struct platform_device *pdev)
 {
 	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 	struct i2c_client *i2c_client = codec->control_data;
 	int reg;
 
+	regulator_enable(cs4270->vd_reg);
+	regulator_enable(cs4270->vlc_reg);
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
 	/* In case the device was put to hard reset during sleep, we need to
 	 * wait 500ns here before any I2C communication. */
 	ndelay(500);
-- 
1.6.5.2

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Fri, Nov 27, 2009 at 01:41:39PM +0100, Daniel Mack wrote:

> Ok, agreed. Next try below.

Looks good to me, though I suspect Timur will want error checking for
those enable calls - Timur?

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Timur Tabi]

Mark Brown wrote:
> Looks good to me, though I suspect Timur will want error checking for
> those enable calls - Timur?

Yes, I'd like to see error checking on those calls, but I'm more concerned 
that I don't see "#ifdef CONFIG_REGULATOR" anywhere in this patch.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On 28 Nov 2009, at 20:23, Timur Tabi <timur@freescale.com> wrote:

> Mark Brown wrote:
>> Looks good to me, though I suspect Timur will want error checking for
>> those enable calls - Timur?
>
> Yes, I'd like to see error checking on those calls, but I'm more  
> concerned that I don't see "#ifdef CONFIG_REGULATOR" anywhere in  
> this patch.

The regulator header file handles that -it provides stub versions of  
the core consumer API calls to save all the drivers having to have  
conditional code in them.

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Timur Tabi]

Mark Brown wrote:
> The regulator header file handles that -it provides stub versions of the
> core consumer API calls to save all the drivers having to have
> conditional code in them.

Ok, that works for me.

However, when I tried to apply the patch to the for-2.6.33 branch of 
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git, it fails:

$ patch -p 1 < "/home/b04825/.mozilla/seamonkey/fl00b35q.default/Mail/Local 
Folders/patches"
patching file sound/soc/codecs/cs4270.c
Hunk #5 FAILED at 651.
Hunk #6 succeeded at 673 (offset -7 lines).
Hunk #8 succeeded at 779 (offset -7 lines).
Hunk #9 succeeded at 895 (offset -16 lines).
1 out of 9 hunks FAILED -- saving rejects to file sound/soc/codecs/cs4270.c.rej

I'm on vacation until the end of the year, so I can't test this driver 
thoroughly, but I'd like to make sure it at least compiles and boots before I 
ack it.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Sat, Nov 28, 2009 at 04:18:05PM -0600, Timur Tabi wrote:
> Mark Brown wrote:
> >The regulator header file handles that -it provides stub versions of the
> >core consumer API calls to save all the drivers having to have
> >conditional code in them.
> 
> Ok, that works for me.
> 
> However, when I tried to apply the patch to the for-2.6.33 branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git,
> it fails:
> 
> $ patch -p 1 <
> "/home/b04825/.mozilla/seamonkey/fl00b35q.default/Mail/Local
> Folders/patches"
> patching file sound/soc/codecs/cs4270.c
> Hunk #5 FAILED at 651.
> Hunk #6 succeeded at 673 (offset -7 lines).
> Hunk #8 succeeded at 779 (offset -7 lines).
> Hunk #9 succeeded at 895 (offset -16 lines).
> 1 out of 9 hunks FAILED -- saving rejects to file sound/soc/codecs/cs4270.c.rej
> 
> I'm on vacation until the end of the year, so I can't test this
> driver thoroughly, but I'd like to make sure it at least compiles
> and boots before I ack it.

It does compile and does boot, we're testing this already on mayn
devices and it works fine. However, there have been updates to that
driver due to changes in the suspend/resume paths.

Mark, can you resolve those conflicts easily or do you want me to do it?

Thanks,
Daniel

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Timur Tabi]

On Sat, Nov 28, 2009 at 6:44 PM, Daniel Mack <daniel@caiaq.de> wrote:

> It does compile and does boot,

Does it compile and boot on a Freescale MPC8610 HPCD?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Sat, Nov 28, 2009 at 10:34:12PM -0600, Timur Tabi wrote:
> On Sat, Nov 28, 2009 at 6:44 PM, Daniel Mack <daniel@caiaq.de> wrote:
> 
> > It does compile and does boot,
> 
> Does it compile and boot on a Freescale MPC8610 HPCD?

No, I don't have such a board. But I see no reason why it shouldn't
compile as no part of the patch is platform depended.

And for configurations with CONFIG_REGULATOR=n, all the new calls are
NOPs. The only heads-up is that for CONFIG_REGULATOR=y, you now need to
provide the regulators the codec driver needs.

Anyway - I'll rebase the driver to 'for-2.6.33' so you can give it a
try.

Thanks,
Daniel

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Sun, Nov 29, 2009 at 09:35:54AM +0100, Daniel Mack wrote:
> Anyway - I'll rebase the driver to 'for-2.6.33' so you can give it a
> try.

See below.

Daniel


>From 40f036c11cf36d6832431a4fda4e1ab1a2cf5b2e Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Wed, 25 Nov 2009 15:31:25 +0100
Subject: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
---
 sound/soc/codecs/cs4270.c |  114 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index ffe122d..279a644 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -28,6 +28,7 @@
 #include <sound/initval.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 #include "cs4270.h"
 
@@ -114,6 +115,12 @@ struct cs4270_private {
 	unsigned int mode; /* The mode (I2S or left-justified) */
 	unsigned int slave_mode;
 	unsigned int manual_mute;
+
+	/* power domain regulators */
+	struct regulator *va_reg;
+	struct regulator *vd_reg;
+	struct regulator *vlc_reg;
+	bool va_reg_enabled;
 };
 
 /**
@@ -458,6 +465,50 @@ static int cs4270_hw_params(struct snd_pcm_substream *substream,
 }
 
 /**
+ * cs4270_set_bias_level - catches BIAS level changes
+ * @dai: the SOC DAI
+ * @level: the new BIAS level
+ *
+ * This function controls the voltage regulators to properly
+ * power up/down the voltage regulator domains
+ */
+
+static int cs4270_set_bias_level(struct snd_soc_codec *codec,
+				 enum snd_soc_bias_level level)
+{
+	int ret;
+	struct cs4270_private *cs4270 = codec->private_data;
+
+	switch (level) {
+	case SND_SOC_BIAS_ON:
+	case SND_SOC_BIAS_PREPARE:
+		if (!cs4270->va_reg_enabled) {
+			ret = regulator_enable(cs4270->va_reg);
+			if (ret < 0)
+				return ret;
+
+			cs4270->va_reg_enabled = true;
+		}
+
+		break;
+	case SND_SOC_BIAS_STANDBY:
+	case SND_SOC_BIAS_OFF:
+		if (cs4270->va_reg_enabled) {
+			ret = regulator_disable(cs4270->va_reg);
+			if (ret < 0)
+				return ret;
+
+			cs4270->va_reg_enabled = false;
+		}
+
+		break;
+	}
+
+	codec->bias_level = level;
+	return 0;
+}
+
+/**
  * cs4270_dai_mute - enable/disable the CS4270 external mute
  * @dai: the SOC DAI
  * @mute: 0 = disable mute, 1 = enable mute
@@ -579,6 +630,7 @@ static int cs4270_probe(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 	int ret;
 
 	/* Connect the codec to the socdev.  snd_soc_new_pcms() needs this. */
@@ -599,9 +651,32 @@ static int cs4270_probe(struct platform_device *pdev)
 		goto error_free_pcms;
 	}
 
+	/* get the power supply regulators */
+	cs4270->va_reg = regulator_get(codec->dev, "va");
+	cs4270->vd_reg = regulator_get(codec->dev, "vd");
+	cs4270->vlc_reg = regulator_get(codec->dev, "vlc");
+
+	if (IS_ERR(cs4270->va_reg) ||
+	    IS_ERR(cs4270->vd_reg) ||
+	    IS_ERR(cs4270->vlc_reg)) {
+		dev_err(codec->dev,
+			"failed to get supplies for device %s\n",
+			dev_name(codec->dev));
+		goto error_free_pcms;
+	}
+
+	regulator_enable(cs4270->vd_reg);
+	regulator_enable(cs4270->vlc_reg);
+
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
 	return 0;
 
 error_free_pcms:
+	regulator_put(cs4270->va_reg);
+	regulator_put(cs4270->vd_reg);
+	regulator_put(cs4270->vlc_reg);
+
 	snd_soc_free_pcms(socdev);
 
 	return ret;
@@ -616,9 +691,21 @@ error_free_pcms:
 static int cs4270_remove(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 
 	snd_soc_free_pcms(socdev);
 
+	if (cs4270->va_reg_enabled)
+		regulator_disable(cs4270->va_reg);
+
+	regulator_disable(cs4270->vd_reg);
+	regulator_disable(cs4270->vlc_reg);
+
+	regulator_put(cs4270->va_reg);
+	regulator_put(cs4270->vd_reg);
+	regulator_put(cs4270->vlc_reg);
+
 	return 0;
 };
 
@@ -692,6 +779,8 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client,
 	codec->write = cs4270_i2c_write;
 	codec->reg_cache = cs4270->reg_cache;
 	codec->reg_cache_size = CS4270_NUMREGS;
+	codec->bias_level = SND_SOC_BIAS_OFF;
+	codec->set_bias_level = cs4270_set_bias_level;
 
 	/* The I2C interface is set up, so pre-fill our register cache */
 
@@ -799,17 +888,38 @@ MODULE_DEVICE_TABLE(i2c, cs4270_id);
 static int cs4270_soc_suspend(struct platform_device *pdev, pm_message_t mesg)
 {
 	struct snd_soc_codec *codec = cs4270_codec;
-	int reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
+	struct cs4270_private *cs4270 = codec->private_data;
+	int reg, ret;
 
-	return snd_soc_write(codec, CS4270_PWRCTL, reg);
+	reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
+	if (reg < 0)
+		return reg;
+
+	ret = snd_soc_write(codec, CS4270_PWRCTL, reg);
+	if (ret < 0)
+		return ret;
+
+	ret = cs4270_set_bias_level(codec, SND_SOC_BIAS_OFF);
+	if (ret < 0)
+		return ret;
+
+	regulator_disable(cs4270->vd_reg);
+	regulator_disable(cs4270->vlc_reg);
+
+	return 0;
 }
 
 static int cs4270_soc_resume(struct platform_device *pdev)
 {
 	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 	struct i2c_client *i2c_client = codec->control_data;
 	int reg;
 
+	regulator_enable(cs4270->vd_reg);
+	regulator_enable(cs4270->vlc_reg);
+	cs4270_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+
 	/* In case the device was put to hard reset during sleep, we need to
 	 * wait 500ns here before any I2C communication. */
 	ndelay(500);
-- 
1.6.5.2

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Sat, Nov 28, 2009 at 07:46:22PM -0700, Tabi Timur-B04825 wrote:

> Have you tested it on a Freescale MPC8610 HPCD?  Are you sure it still
> compiles and builds on any PowerPC system?

I can do a compile test on PowerPC (and that's one of the platforms that
gets tested with linux-next too) but I don't have hardware to do runtime
tests with.  Timur, would you be OK with compile tests for now - I'd not
expect to see 2.6.33 go out this year so if there are any runtime issues
it should be possible to fix them once you're back in the office?

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Mon, Nov 30, 2009 at 12:01:59PM +0000, Mark Brown wrote:
> On Sat, Nov 28, 2009 at 07:46:22PM -0700, Tabi Timur-B04825 wrote:
> 
> > Have you tested it on a Freescale MPC8610 HPCD?  Are you sure it still
> > compiles and builds on any PowerPC system?
> 
> I can do a compile test on PowerPC (and that's one of the platforms that
> gets tested with linux-next too) but I don't have hardware to do runtime
> tests with.  Timur, would you be OK with compile tests for now - I'd not
> expect to see 2.6.33 go out this year so if there are any runtime issues
> it should be possible to fix them once you're back in the office?

Wait a sec before applying this, please. We might have caused a
regression, I'm still investiating.

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Mon, Nov 30, 2009 at 01:36:17PM +0100, Daniel Mack wrote:
> On Mon, Nov 30, 2009 at 12:01:59PM +0000, Mark Brown wrote:
> > On Sat, Nov 28, 2009 at 07:46:22PM -0700, Tabi Timur-B04825 wrote:
> > 
> > > Have you tested it on a Freescale MPC8610 HPCD?  Are you sure it still
> > > compiles and builds on any PowerPC system?
> > 
> > I can do a compile test on PowerPC (and that's one of the platforms that
> > gets tested with linux-next too) but I don't have hardware to do runtime
> > tests with.  Timur, would you be OK with compile tests for now - I'd not
> > expect to see 2.6.33 go out this year so if there are any runtime issues
> > it should be possible to fix them once you're back in the office?
> 
> Wait a sec before applying this, please. We might have caused a
> regression, I'm still investiating.

The current aproach does not work reliably, unfortunately. It turns out
that the codec necessarily needs its analogue supply to maintain its
state. For all other operations (iow, any of the volatges not applied),
it has to be held in reset mode. I did that manually by driving the GPIO
directly before, but with the current regulator framework support,
that's not possible anymore.

Hence, we can only power down the codec when we're in system suspend,
which is a pitty. But even for that, we should still drive the RESET
signal low. Can we make the GPIO pin number available to the codec
driver?

In any case, the current patch is plainly wrong and can be dropped.
Sorry for the noise caused.

Daniel

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Mon, Nov 30, 2009 at 04:57:42PM +0100, Daniel Mack wrote:

> The current aproach does not work reliably, unfortunately. It turns out
> that the codec necessarily needs its analogue supply to maintain its
> state. For all other operations (iow, any of the volatges not applied),
> it has to be held in reset mode. I did that manually by driving the GPIO
> directly before, but with the current regulator framework support,
> that's not possible anymore.

This is one of the reasons why your previous code looked suspect to me,
actually - since the CODEC driver had no idea that you'd suspended the
device it'd do things like try to write volume updates to the device
while you were keeping the power off.

> Hence, we can only power down the codec when we're in system suspend,

When runtime PM hits in 2.6.33 I intend to try to enable support for it
in ASoC, it's relatively straightforward I think.  Unfortunately it'll
end up depending on the platform bus on the relevant platform being
capable of runtime pm but that should become more widely supported as
things progress.

Given this I would suggest doing the same thing the other drivers are
doing and only turning things off when the device is suspended (either
via the explicit functions or via set_bias_level()) and then let the
ASoC core deal with actually doing the suspend - it's probably less
hassle for you that way.  Converting to use the shared register I/O code
in soc-cache.c might also help here.

> which is a pitty. But even for that, we should still drive the RESET
> signal low. Can we make the GPIO pin number available to the codec
> driver?

Of course, use platform data.

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

On Mon, Nov 30, 2009 at 04:12:09PM +0000, Mark Brown wrote:
> On Mon, Nov 30, 2009 at 04:57:42PM +0100, Daniel Mack wrote:
> 
> > The current aproach does not work reliably, unfortunately. It turns out
> > that the codec necessarily needs its analogue supply to maintain its
> > state. For all other operations (iow, any of the volatges not applied),
> > it has to be held in reset mode. I did that manually by driving the GPIO
> > directly before, but with the current regulator framework support,
> > that's not possible anymore.
> 
> This is one of the reasons why your previous code looked suspect to me,
> actually - since the CODEC driver had no idea that you'd suspended the
> device it'd do things like try to write volume updates to the device
> while you were keeping the power off.

Yes, I totally agree. It's just that the whole power regulator framework
was new to me, and hence I didn't use and fully understand it. Once you
know what it is designed for, it all makes sense.

> When runtime PM hits in 2.6.33 I intend to try to enable support for it
> in ASoC, it's relatively straightforward I think.  Unfortunately it'll
> end up depending on the platform bus on the relevant platform being
> capable of runtime pm but that should become more widely supported as
> things progress.
> 
> Given this I would suggest doing the same thing the other drivers are
> doing and only turning things off when the device is suspended (either
> via the explicit functions or via set_bias_level()) and then let the
> ASoC core deal with actually doing the suspend - it's probably less
> hassle for you that way.  Converting to use the shared register I/O code
> in soc-cache.c might also help here.

Ok, I'll first post a new version for the regulators again and will care
for the rest later :)

Daniel

[PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
---
 sound/soc/codecs/cs4270.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index ffe122d..8b54575 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -28,6 +28,7 @@
 #include <sound/initval.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 
 #include "cs4270.h"
 
@@ -106,6 +107,10 @@
 #define CS4270_MUTE_DAC_A	0x01
 #define CS4270_MUTE_DAC_B	0x02
 
+static const char *supply_names[] = {
+	"va", "vd", "vlc"
+};
+
 /* Private data for the CS4270 */
 struct cs4270_private {
 	struct snd_soc_codec codec;
@@ -114,6 +119,9 @@ struct cs4270_private {
 	unsigned int mode; /* The mode (I2S or left-justified) */
 	unsigned int slave_mode;
 	unsigned int manual_mute;
+
+	/* power domain regulators */
+	struct regulator_bulk_data supplies[ARRAY_SIZE(supply_names)];
 };
 
 /**
@@ -579,7 +587,8 @@ static int cs4270_probe(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = cs4270_codec;
-	int ret;
+	struct cs4270_private *cs4270 = codec->private_data;
+	int i, ret;
 
 	/* Connect the codec to the socdev.  snd_soc_new_pcms() needs this. */
 	socdev->card->codec = codec;
@@ -599,6 +608,15 @@ static int cs4270_probe(struct platform_device *pdev)
 		goto error_free_pcms;
 	}
 
+	/* get the power supply regulators */
+	for (i = 0; i < ARRAY_SIZE(supply_names); i++)
+		cs4270->supplies[i].supply = supply_names[i];
+
+	ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(cs4270->supplies),
+				 cs4270->supplies);
+	if (ret < 0)
+		goto error_free_pcms;
+
 	return 0;
 
 error_free_pcms:
@@ -616,8 +634,11 @@ error_free_pcms:
 static int cs4270_remove(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
+	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 
 	snd_soc_free_pcms(socdev);
+	regulator_bulk_free(ARRAY_SIZE(cs4270->supplies), cs4270->supplies);
 
 	return 0;
 };
@@ -799,17 +820,33 @@ MODULE_DEVICE_TABLE(i2c, cs4270_id);
 static int cs4270_soc_suspend(struct platform_device *pdev, pm_message_t mesg)
 {
 	struct snd_soc_codec *codec = cs4270_codec;
-	int reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
+	struct cs4270_private *cs4270 = codec->private_data;
+	int reg, ret;
 
-	return snd_soc_write(codec, CS4270_PWRCTL, reg);
+	reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
+	if (reg < 0)
+		return reg;
+
+	ret = snd_soc_write(codec, CS4270_PWRCTL, reg);
+	if (ret < 0)
+		return ret;
+
+	regulator_bulk_disable(ARRAY_SIZE(cs4270->supplies),
+			       cs4270->supplies);
+
+	return 0;
 }
 
 static int cs4270_soc_resume(struct platform_device *pdev)
 {
 	struct snd_soc_codec *codec = cs4270_codec;
+	struct cs4270_private *cs4270 = codec->private_data;
 	struct i2c_client *i2c_client = codec->control_data;
 	int reg;
 
+	regulator_bulk_enable(ARRAY_SIZE(cs4270->supplies),
+			      cs4270->supplies);
+
 	/* In case the device was put to hard reset during sleep, we need to
 	 * wait 500ns here before any I2C communication. */
 	ndelay(500);
-- 
1.6.5.2

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Daniel Mack]

Was that one applied?

On Mon, Nov 30, 2009 at 05:56:11PM +0100, Daniel Mack wrote:
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Timur Tabi <timur@freescale.com>
> Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> ---
>  sound/soc/codecs/cs4270.c |   43 ++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
> index ffe122d..8b54575 100644
> --- a/sound/soc/codecs/cs4270.c
> +++ b/sound/soc/codecs/cs4270.c
> @@ -28,6 +28,7 @@
>  #include <sound/initval.h>
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include "cs4270.h"
>  
> @@ -106,6 +107,10 @@
>  #define CS4270_MUTE_DAC_A	0x01
>  #define CS4270_MUTE_DAC_B	0x02
>  
> +static const char *supply_names[] = {
> +	"va", "vd", "vlc"
> +};
> +
>  /* Private data for the CS4270 */
>  struct cs4270_private {
>  	struct snd_soc_codec codec;
> @@ -114,6 +119,9 @@ struct cs4270_private {
>  	unsigned int mode; /* The mode (I2S or left-justified) */
>  	unsigned int slave_mode;
>  	unsigned int manual_mute;
> +
> +	/* power domain regulators */
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(supply_names)];
>  };
>  
>  /**
> @@ -579,7 +587,8 @@ static int cs4270_probe(struct platform_device *pdev)
>  {
>  	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
>  	struct snd_soc_codec *codec = cs4270_codec;
> -	int ret;
> +	struct cs4270_private *cs4270 = codec->private_data;
> +	int i, ret;
>  
>  	/* Connect the codec to the socdev.  snd_soc_new_pcms() needs this. */
>  	socdev->card->codec = codec;
> @@ -599,6 +608,15 @@ static int cs4270_probe(struct platform_device *pdev)
>  		goto error_free_pcms;
>  	}
>  
> +	/* get the power supply regulators */
> +	for (i = 0; i < ARRAY_SIZE(supply_names); i++)
> +		cs4270->supplies[i].supply = supply_names[i];
> +
> +	ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(cs4270->supplies),
> +				 cs4270->supplies);
> +	if (ret < 0)
> +		goto error_free_pcms;
> +
>  	return 0;
>  
>  error_free_pcms:
> @@ -616,8 +634,11 @@ error_free_pcms:
>  static int cs4270_remove(struct platform_device *pdev)
>  {
>  	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
> +	struct snd_soc_codec *codec = cs4270_codec;
> +	struct cs4270_private *cs4270 = codec->private_data;
>  
>  	snd_soc_free_pcms(socdev);
> +	regulator_bulk_free(ARRAY_SIZE(cs4270->supplies), cs4270->supplies);
>  
>  	return 0;
>  };
> @@ -799,17 +820,33 @@ MODULE_DEVICE_TABLE(i2c, cs4270_id);
>  static int cs4270_soc_suspend(struct platform_device *pdev, pm_message_t mesg)
>  {
>  	struct snd_soc_codec *codec = cs4270_codec;
> -	int reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
> +	struct cs4270_private *cs4270 = codec->private_data;
> +	int reg, ret;
>  
> -	return snd_soc_write(codec, CS4270_PWRCTL, reg);
> +	reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
> +	if (reg < 0)
> +		return reg;
> +
> +	ret = snd_soc_write(codec, CS4270_PWRCTL, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	regulator_bulk_disable(ARRAY_SIZE(cs4270->supplies),
> +			       cs4270->supplies);
> +
> +	return 0;
>  }
>  
>  static int cs4270_soc_resume(struct platform_device *pdev)
>  {
>  	struct snd_soc_codec *codec = cs4270_codec;
> +	struct cs4270_private *cs4270 = codec->private_data;
>  	struct i2c_client *i2c_client = codec->control_data;
>  	int reg;
>  
> +	regulator_bulk_enable(ARRAY_SIZE(cs4270->supplies),
> +			      cs4270->supplies);
> +
>  	/* In case the device was put to hard reset during sleep, we need to
>  	 * wait 500ns here before any I2C communication. */
>  	ndelay(500);
> -- 
> 1.6.5.2
> 

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Fri, Dec 04, 2009 at 01:00:07PM +0100, Daniel Mack wrote:
> Was that one applied?

No, need review from Timur still.

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Timur Tabi]

On Fri, Dec 4, 2009 at 6:07 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Dec 04, 2009 at 01:00:07PM +0100, Daniel Mack wrote:
>> Was that one applied?
>
> No, need review from Timur still.

Wait, I'm confused.  I thought the patch didn't work at all and Daniel
rescinded it.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On 5 Dec 2009, at 02:54, Timur Tabi <timur@freescale.com> wrote:

> On Fri, Dec 4, 2009 at 6:07 AM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Fri, Dec 04, 2009 at 01:00:07PM +0100, Daniel Mack wrote:
>>> Was that one applied?
>>
>> No, need review from Timur still.
>
> Wait, I'm confused.  I thought the patch didn't work at all and Daniel
> rescinded it.

Yes, but then he posted a new revision which didn't try to turn off  
the analogue supply seperately which ought to work.

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Timur Tabi]

On Mon, Nov 30, 2009 at 10:56 AM, Daniel Mack <daniel@caiaq.de> wrote:
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Timur Tabi <timur@freescale.com>
> Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> ---

Ack

> +static const char *supply_names[] = {
> +       "va", "vd", "vlc"
> +};

Minor nit -- this array would take up less space if it were an array
of strings instead of an array of pointers to strings.

static const char supply_names[4][] = {
       "va", "vd", "vlc"
};

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Sat, Dec 05, 2009 at 11:03:49AM -0600, Timur Tabi wrote:
> On Mon, Nov 30, 2009 at 10:56 AM, Daniel Mack <daniel@caiaq.de> wrote:
> > Signed-off-by: Daniel Mack <daniel@caiaq.de>
> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > Cc: Timur Tabi <timur@freescale.com>
> > Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> > ---

> Ack

Applied, thanks.