[alsa-devel] [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 | 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; };
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).
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
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...
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
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
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.
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
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;} elseregulator_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).
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
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.
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.
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.
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
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?
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
participants (3)
-
Daniel Mack -
Mark Brown -
Timur Tabi