Re: [alsa-devel] [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver
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?
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.
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
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.
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
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);
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
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.
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.
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" };
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.
participants (3)
-
Daniel Mack -
Mark Brown -
Timur Tabi