[alsa-devel] [PATCH RFC 0/3] asoc: uda1380 cleanup
This patch series cleanups uda1380 codec driver a bit: - it introduces callbacks instead of direct gpiolib usage, as some machines requires more actions than just plain gpio toggle to enable/disable/reset codec; - it makes driver more powersave-friendly and fixes suspend/resume
Some machines require some tricks to enable/disable codec, i.e. disable or enable i2s clock before enabling/disabling codec, and just configuring gpio is not enough; some machines have no reset pin (reset is performed on codec power on automatically). Fix that issue by using machine-specific callback to enable codec power.
Signed-off-by: Vasily Khoruzhick anarsoul@gmail.com --- include/sound/uda1380.h | 3 +-- sound/soc/codecs/uda1380.c | 25 +++---------------------- 2 files changed, 4 insertions(+), 24 deletions(-)
diff --git a/include/sound/uda1380.h b/include/sound/uda1380.h index 381319c..d2171dc 100644 --- a/include/sound/uda1380.h +++ b/include/sound/uda1380.h @@ -12,8 +12,7 @@ #define __UDA1380_H
struct uda1380_platform_data { - int gpio_power; - int gpio_reset; + void (*set_power)(int); int dac_clk; #define UDA1380_DAC_CLK_SYSCLK 0 #define UDA1380_DAC_CLK_WSPLL 1 diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c index 2f925a2..2b7f200 100644 --- a/sound/soc/codecs/uda1380.c +++ b/sound/soc/codecs/uda1380.c @@ -19,7 +19,6 @@ #include <linux/types.h> #include <linux/slab.h> #include <linux/errno.h> -#include <linux/gpio.h> #include <linux/delay.h> #include <linux/i2c.h> #include <linux/workqueue.h> @@ -752,22 +751,11 @@ static int uda1380_register(struct uda1380_priv *uda1380) return -EINVAL; }
- if (!pdata || !pdata->gpio_power || !pdata->gpio_reset) + if (!pdata || !pdata->set_power) return -EINVAL;
- ret = gpio_request(pdata->gpio_power, "uda1380 power"); - if (ret) - goto err_out; - ret = gpio_request(pdata->gpio_reset, "uda1380 reset"); - if (ret) - goto err_gpio; - - gpio_direction_output(pdata->gpio_power, 1); - /* we may need to have the clock running here - pH5 */ - gpio_direction_output(pdata->gpio_reset, 1); - udelay(5); - gpio_set_value(pdata->gpio_reset, 0); + pdata->set_power(1);
mutex_init(&codec->mutex); INIT_LIST_HEAD(&codec->dapm_widgets); @@ -818,11 +806,6 @@ static int uda1380_register(struct uda1380_priv *uda1380) err_dai: snd_soc_unregister_codec(codec); err_reset: - gpio_set_value(pdata->gpio_power, 0); - gpio_free(pdata->gpio_reset); -err_gpio: - gpio_free(pdata->gpio_power); -err_out: return ret; }
@@ -834,9 +817,7 @@ static void uda1380_unregister(struct uda1380_priv *uda1380) snd_soc_unregister_dais(uda1380_dai, ARRAY_SIZE(uda1380_dai)); snd_soc_unregister_codec(&uda1380->codec);
- gpio_set_value(pdata->gpio_power, 0); - gpio_free(pdata->gpio_reset); - gpio_free(pdata->gpio_power); + pdata->set_power(0);
kfree(uda1380); uda1380_codec = NULL;
On 26 Jun 2010, at 16:14, Vasily Khoruzhick anarsoul@gmail.com wrote:
Some machines require some tricks to enable/disable codec, i.e. disable or enable i2s clock before enabling/disabling codec, and just configuring gpio is not enough; some machines have no reset pin (reset is performed on codec power on automatically). Fix that issue by using machine-specific callback to enable codec power.
Signed-off-by: Vasily Khoruzhick anarsoul@gmail.com
This is fine but it'd be really nice to preserve the use of GPIOs since that will cover the majority of machines - for example, by providing a default callback if none is provided and GPIOs are. This will also avoid the need to update existing machine drivers (which needs to be done otherwise).
However, I do wonder if the more complex set_power() callbacks might not just end up as regulator API consumers?
include/sound/uda1380.h | 3 +-- sound/soc/codecs/uda1380.c | 25 +++--------------------- 2 files changed, 4 insertions(+), 24 deletions(-)
diff --git a/include/sound/uda1380.h b/include/sound/uda1380.h index 381319c..d2171dc 100644 --- a/include/sound/uda1380.h +++ b/include/sound/uda1380.h @@ -12,8 +12,7 @@ #define __UDA1380_H
struct uda1380_platform_data {
- int gpio_power;
- int gpio_reset;
- void (*set_power)(int); int dac_clk;
#define UDA1380_DAC_CLK_SYSCLK 0 #define UDA1380_DAC_CLK_WSPLL 1 diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c index 2f925a2..2b7f200 100644 --- a/sound/soc/codecs/uda1380.c +++ b/sound/soc/codecs/uda1380.c @@ -19,7 +19,6 @@ #include <linux/types.h> #include <linux/slab.h> #include <linux/errno.h> -#include <linux/gpio.h> #include <linux/delay.h> #include <linux/i2c.h> #include <linux/workqueue.h> @@ -752,22 +751,11 @@ static int uda1380_register(struct uda1380_priv *uda1380) return -EINVAL; }
- if (!pdata || !pdata->gpio_power || !pdata->gpio_reset)
- if (!pdata || !pdata->set_power) return -EINVAL;
- ret = gpio_request(pdata->gpio_power, "uda1380 power");
- if (ret)
goto err_out;- ret = gpio_request(pdata->gpio_reset, "uda1380 reset");
- if (ret)
goto err_gpio;- gpio_direction_output(pdata->gpio_power, 1);
- /* we may need to have the clock running here - pH5 */
- gpio_direction_output(pdata->gpio_reset, 1);
- udelay(5);
- gpio_set_value(pdata->gpio_reset, 0);
pdata->set_power(1);
mutex_init(&codec->mutex); INIT_LIST_HEAD(&codec->dapm_widgets);
@@ -818,11 +806,6 @@ static int uda1380_register(struct uda1380_priv *uda1380) err_dai: snd_soc_unregister_codec(codec); err_reset:
- gpio_set_value(pdata->gpio_power, 0);
- gpio_free(pdata->gpio_reset);
-err_gpio:
- gpio_free(pdata->gpio_power);
-err_out: return ret; }
@@ -834,9 +817,7 @@ static void uda1380_unregister(struct uda1380_priv *uda1380) snd_soc_unregister_dais(uda1380_dai, ARRAY_SIZE(uda1380_dai)); snd_soc_unregister_codec(&uda1380->codec);
- gpio_set_value(pdata->gpio_power, 0);
- gpio_free(pdata->gpio_reset);
- gpio_free(pdata->gpio_power);
pdata->set_power(0);
kfree(uda1380); uda1380_codec = NULL;
-- 1.7.1
В сообщении от 26 июня 2010 19:40:37 автор Mark Brown написал:
On 26 Jun 2010, at 16:14, Vasily Khoruzhick anarsoul@gmail.com wrote:
Some machines require some tricks to enable/disable codec, i.e. disable or enable i2s clock before enabling/disabling codec, and just configuring gpio is not enough; some machines have no reset pin (reset is performed on codec power on automatically). Fix that issue by using machine-specific callback to enable codec power.
Signed-off-by: Vasily Khoruzhick anarsoul@gmail.com
This is fine but it'd be really nice to preserve the use of GPIOs since that will cover the majority of machines - for example, by providing a default callback if none is provided and GPIOs are. This will also avoid the need to update existing machine drivers (which needs to be done otherwise).
The only machine that uses uda1380 and supported by mainline kernel is magician, rx1950 and h1940 sound support is not merged yet, so that's not a problem to perform that change.
However, I do wonder if the more complex set_power() callbacks might not just end up as regulator API consumers?
Is it really necessary? Plain callback perfectly fits here, and same approach is used for s3cmci driver. For example, rx1950_uda1380_set_power is not complex and looks like this:
static void rx1950_uda1380_set_power(int enable) { clk_disable(i2c_clk); gpio_direction_output(S3C2410_GPD(0), 0); gpio_direction_output(S3C2410_GPJ(0), enable); if (enable) { gpio_set_value(S3C2410_GPD(0), 1); mdelay(1); gpio_set_value(S3C2410_GPD(0), 0); } clk_enable(i2c_clk); }
Regards Vasily
On 26 Jun 2010, at 17:53, Vasily Khoruzhick wrote:
В сообщении от 26 июня 2010 19:40:37 автор Mark Brown написал:
This is fine but it'd be really nice to preserve the use of GPIOs since that will cover the majority of machines - for example, by providing a default callback if none is provided and GPIOs are. This will also avoid the need to update existing machine drivers (which needs to be done otherwise).
The only machine that uses uda1380 and supported by mainline kernel is magician, rx1950 and h1940 sound support is not merged yet, so that's not a problem to perform that change.
It's not just the hassle, it's also the fact that all these machine drivers will end up duplicating the code. This feels like a step back for machines other than yours.
However, I do wonder if the more complex set_power() callbacks might not just end up as regulator API consumers?
Is it really necessary? Plain callback perfectly fits here, and same approach is used for s3cmci driver. For example, rx1950_uda1380_set_power is not complex and looks like this:
The regulator API isn't massively complex either and it does provide a standard abstraction for controlling power which seems like it might be useful here, though given the (frankly rather odd) fiddling with clocks your callback does perhaps it won't help.
В сообщении от 26 июня 2010 23:09:39 автор Mark Brown написал:
The regulator API isn't massively complex either and it does provide a standard abstraction for controlling power which seems like it might be useful here, though given the (frankly rather odd) fiddling with clocks your callback does perhaps it won't help.
Ok, but I still don't like idea about keeping both regulator and gpio stuff. Btw, this weird clock stuff can be merged to i2c driver, i.e. keep i2c module clock enabled only on i2c transfer (it's not scl line, it's module clock) (I've sent patch for i2c-s3c2410 ~month ago, can resend it), and we can avoid modyfing power/reset controlling part of uda1380 driver. What do you think about that?
Regards Vasily
On 26 Jun 2010, at 21:53, Vasily Khoruzhick wrote:
В сообщении от 26 июня 2010 23:09:39 автор Mark Brown написал:
The regulator API isn't massively complex either and it does provide a standard abstraction for controlling power which seems like it might be useful here, though given the (frankly rather odd) fiddling with clocks your callback does perhaps it won't help.
Ok, but I still don't like idea about keeping both regulator and gpio stuff.
If you're going to the regulator API it's probably OK to remove the GPIO stuff since the regulator API supports GPIO controlled regulators already. Does mean transitioning the existing drivers but it means everyone shares the same code.
Btw, this weird clock stuff can be merged to i2c driver, i.e. keep i2c module clock enabled only on i2c transfer (it's not scl line, it's module clock) (I've sent patch for i2c-s3c2410 ~month ago, can resend it), and we can avoid modyfing power/reset controlling part of uda1380 driver. What do you think about that?
You definitely should be doing this in the I2C driver not the CODEC driver, you're peering inside the internals of the I2C driver as things stand and are likely to cause problems if someone comes along and implements this in the driver. There's also the fact that all users of the I2C controller will benefit if the code is implemented there, not just this one audio driver.
В сообщении от 26 июня 2010 23:57:42 автор Mark Brown написал:
You definitely should be doing this in the I2C driver not the CODEC driver, you're peering inside the internals of the I2C driver as things stand and are likely to cause problems if someone comes along and implements this in the driver. There's also the fact that all users of the I2C controller will benefit if the code is implemented there, not just this one audio driver.
I'm afraid that it can cause problems with other i2c-controlled devices, and I can test i2c modification only on h1940 and rx1950 with same codec (uda1380).
On Sun, Jun 27, 2010 at 12:12:05AM +0300, Vasily Khoruzhick wrote:
В сообщении от 26 июня 2010 23:57:42 автор Mark Brown написал:
You definitely should be doing this in the I2C driver not the CODEC driver, you're peering inside the internals of the I2C driver as things stand and are likely to cause problems if someone comes along and implements this in the driver. There's also the fact that all users of the I2C controller will benefit if the code is implemented there, not just this one audio driver.
I'm afraid that it can cause problems with other i2c-controlled devices, and I
That's still not dealing with the problems that will arise if someone does implement this in the core. To be honest I'm having a hard time seeing why devices would care - the I2C controller only clocks the bus during a transfer and otherwise produces no output - so it should just be an issue for the controller IP.
can test i2c modification only on h1940 and rx1950 with same codec (uda1380).
Nobody is ever going to be able to test on all possible hardware, testing is a community effort.
В сообщении от 26 июня 2010 23:57:42 автор Mark Brown написал:
On 26 Jun 2010, at 21:53, Vasily Khoruzhick wrote:
В сообщении от 26 июня 2010 23:09:39 автор Mark Brown написал:
The regulator API isn't massively complex either and it does provide a standard abstraction for controlling power which seems like it might be useful here, though given the (frankly rather odd) fiddling with clocks your callback does perhaps it won't help.
Ok, but I still don't like idea about keeping both regulator and gpio stuff.
If you're going to the regulator API it's probably OK to remove the GPIO stuff since the regulator API supports GPIO controlled regulators already. Does mean transitioning the existing drivers but it means everyone shares the same code.
Actually, power on sequence on rx1950 (and on magician) looks like following:
1. set power pin to 1 2. set reset pin to 1 3. wait a bit (1ms or so) 4. set reset pin to 0
Reset right after power on is necessary at least on rx1950 (otherwise codec doesn't respond on i2c).
Fixed regulator doesn't support that logic now, and I don't want to mix gpio/regulator stuff in uda1380 driver. Does it makes sense to implement another fixed-like regulator just for uda1380?
Regards Vasily
On Mon, Jun 28, 2010 at 03:00:13PM +0300, Vasily Khoruzhick wrote:
Actually, power on sequence on rx1950 (and on magician) looks like following:
- set power pin to 1
Is this a feature of the chip or an external power control?
В сообщении от 28 июня 2010 16:41:45 автор Mark Brown написал:
On Mon, Jun 28, 2010 at 03:00:13PM +0300, Vasily Khoruzhick wrote:
Actually, power on sequence on rx1950 (and on magician) looks like following:
- set power pin to 1
Is this a feature of the chip or an external power control?
External power control, power and reset pins are routed to s3c24xx gpio pins on rx1950
On Mon, Jun 28, 2010 at 04:49:33PM +0300, Vasily Khoruzhick wrote:
В сообщении от 28 июня 2010 16:41:45 автор Mark Brown написал:
Is this a feature of the chip or an external power control?
External power control, power and reset pins are routed to s3c24xx gpio pins on rx1950
So this is an on-board power source that's being controlled by the GPIO rather than an external device?
В сообщении от 28 июня 2010 16:50:46 автор Mark Brown написал:
So this is an on-board power source that's being controlled by the GPIO rather than an external device?
That's just gpio-based power control for uda1380 (not sure about schematics, it's not available). Same approach is used in h1940 and magician.
Regards Vasily
On Mon, Jun 28, 2010 at 05:05:51PM +0300, Vasily Khoruzhick wrote:
В сообщении от 28 июня 2010 16:50:46 автор Mark Brown написал:
So this is an on-board power source that's being controlled by the GPIO rather than an external device?
That's just gpio-based power control for uda1380 (not sure about schematics, it's not available). Same approach is used in h1940 and magician.
What I'm trying to figure out is if the GPIO is wired directly to the UDA1380 or if this is controlling something external to the UDA1380 which these designs just happen to have copied. If it's a direct connection to the UDA1380 then using GPIOs directly is perfectly sensible.
В сообщении от 28 июня 2010 17:15:40 автор Mark Brown написал:
What I'm trying to figure out is if the GPIO is wired directly to the UDA1380 or if this is controlling something external to the UDA1380 which these designs just happen to have copied. If it's a direct connection to the UDA1380 then using GPIOs directly is perfectly sensible.
I'm not sure if s3c24xx GPIOs can drive UDA1380 directly. I suspect it's wired to the gate of some transistor (FET - is it right term to use here? :)) to handle higher current (sorry, English is not my native language, and it's hard to explain schematic-specific things, that's not my field :)). Btw, I'm sure that there's no any external power-control chip, maybe just single transistor.
Regards Vasily
Signed-off-by: Vasily Khoruzhick anarsoul@gmail.com --- sound/soc/pxa/magician.c | 25 +++++++++++++++++++++++-- 1 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/sound/soc/pxa/magician.c b/sound/soc/pxa/magician.c index 4c8d99a..f2ad1b6 100644 --- a/sound/soc/pxa/magician.c +++ b/sound/soc/pxa/magician.c @@ -457,12 +457,21 @@ static struct snd_soc_device magician_snd_devdata = {
static struct platform_device *magician_snd_device;
+static void set_power(int enable) +{ + gpio_direction_output(EGPIO_MAGICIAN_CODEC_POWER, enable); + if (enable) { + gpio_direction_output(EGPIO_MAGICIAN_CODEC_RESET, 1); + udelay(5); + gpio_set_value(EGPIO_MAGICIAN_CODEC_RESET, 0); + } +} + /* * FIXME: move into magician board file once merged into the pxa tree */ static struct uda1380_platform_data uda1380_info = { - .gpio_power = EGPIO_MAGICIAN_CODEC_POWER, - .gpio_reset = EGPIO_MAGICIAN_CODEC_RESET, + .set_power = set_power, .dac_clk = UDA1380_DAC_CLK_WSPLL, };
@@ -490,6 +499,12 @@ static int __init magician_init(void) if (!client) return -ENODEV;
+ ret = gpio_request(EGPIO_MAGICIAN_CODEC_POWER, "CODEC_POWER"); + if (ret) + goto err_request_codec_pwr; + ret = gpio_request(EGPIO_MAGICIAN_CODEC_RESET, "CODEC_RESET"); + if (ret) + goto err_request_codec_reset; ret = gpio_request(EGPIO_MAGICIAN_SPK_POWER, "SPK_POWER"); if (ret) goto err_request_spk; @@ -535,6 +550,10 @@ err_request_mic: err_request_ep: gpio_free(EGPIO_MAGICIAN_SPK_POWER); err_request_spk: + gpio_free(EGPIO_MAGICIAN_CODEC_RESET); +err_request_codec_reset: + gpio_free(EGPIO_MAGICIAN_CODEC_POWER); +err_request_codec_pwr: return ret; }
@@ -551,6 +570,8 @@ static void __exit magician_exit(void) gpio_free(EGPIO_MAGICIAN_MIC_POWER); gpio_free(EGPIO_MAGICIAN_EP_POWER); gpio_free(EGPIO_MAGICIAN_SPK_POWER); + gpio_free(EGPIO_MAGICIAN_CODEC_RESET); + gpio_free(EGPIO_MAGICIAN_CODEC_POWER); }
module_init(magician_init);
Disable some codec modules in standby mode, completely disable codec in off mode to save some power. Fix suspend/resume: mark mixer regs as dirty on resume to restore mixer values, otherwise driver produces no sound (master is muted by default).
Signed-off-by: Vasily Khoruzhick anarsoul@gmail.com --- sound/soc/codecs/uda1380.c | 92 ++++++++++++++++++++++++++++--------------- 1 files changed, 60 insertions(+), 32 deletions(-)
diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c index 2b7f200..a30f809 100644 --- a/sound/soc/codecs/uda1380.c +++ b/sound/soc/codecs/uda1380.c @@ -130,7 +130,41 @@ static int uda1380_write(struct snd_soc_codec *codec, unsigned int reg, return -EIO; }
-#define uda1380_reset(c) uda1380_write(c, UDA1380_RESET, 0) +static void uda1380_sync_cache(struct snd_soc_codec *codec) +{ + int reg; + u8 data[3]; + u16 *cache = codec->reg_cache; + + /* Sync reg_cache with the hardware */ + for (reg = 0; reg < UDA1380_MVOL; reg++) { + data[0] = reg; + data[1] = (cache[reg] & 0xff00) >> 8; + data[2] = cache[reg] & 0x00ff; + if (codec->hw_write(codec->control_data, data, 3) != 3) + dev_err(codec->dev, "%s: write to reg 0x%x failed\n", + __func__, reg); + } + + for (reg = UDA1380_MVOL; reg < UDA1380_CACHEREGNUM; reg++) + set_bit(reg - 0x10, &uda1380_cache_dirty); +} + +static int uda1380_reset(struct snd_soc_codec *codec) +{ + u8 data[3]; + + data[0] = UDA1380_RESET; + data[1] = 0; + data[2] = 0; + + if (codec->hw_write(codec->control_data, data, 3) != 3) { + dev_err(codec->dev, "%s: failed\n", __func__); + return -EIO; + } + + return 0; +}
static void uda1380_flush_work(struct work_struct *work) { @@ -481,13 +515,13 @@ static int uda1380_trigger(struct snd_pcm_substream *substream, int cmd, switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: - uda1380_write_reg_cache(codec, UDA1380_MIXER, + uda1380_write(codec, UDA1380_MIXER, mixer & ~R14_SILENCE); schedule_work(&uda1380->work); break; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - uda1380_write_reg_cache(codec, UDA1380_MIXER, + uda1380_write(codec, UDA1380_MIXER, mixer | R14_SILENCE); schedule_work(&uda1380->work); break; @@ -561,17 +595,38 @@ static int uda1380_set_bias_level(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { int pm = uda1380_read_reg_cache(codec, UDA1380_PM); + struct uda1380_platform_data *pdata = codec->dev->platform_data; + + if (codec->bias_level == level) + return 0;
switch (level) { case SND_SOC_BIAS_ON: case SND_SOC_BIAS_PREPARE: + /* ADC, DAC on */ uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm); break; case SND_SOC_BIAS_STANDBY: - uda1380_write(codec, UDA1380_PM, R02_PON_BIAS); + if (codec->bias_level == SND_SOC_BIAS_OFF) { + pdata->set_power(1); + uda1380_reset(codec); + + switch (pdata->dac_clk) { + case UDA1380_DAC_CLK_SYSCLK: + uda1380_write_reg_cache(codec, UDA1380_CLK, 0); + break; + case UDA1380_DAC_CLK_WSPLL: + uda1380_write_reg_cache(codec, UDA1380_CLK, + R00_DAC_CLK); + break; + } + + uda1380_sync_cache(codec); + } + uda1380_write(codec, UDA1380_PM, 0x0); break; case SND_SOC_BIAS_OFF: - uda1380_write(codec, UDA1380_PM, 0x0); + pdata->set_power(0); break; } codec->bias_level = level; @@ -658,16 +713,7 @@ static int uda1380_resume(struct platform_device *pdev) { struct snd_soc_device *socdev = platform_get_drvdata(pdev); struct snd_soc_codec *codec = socdev->card->codec; - int i; - u8 data[2]; - u16 *cache = codec->reg_cache;
- /* Sync reg_cache with the hardware */ - for (i = 0; i < ARRAY_SIZE(uda1380_reg); i++) { - data[0] = (i << 1) | ((cache[i] >> 8) & 0x0001); - data[1] = cache[i] & 0x00ff; - codec->hw_write(codec->control_data, data, 2); - } uda1380_set_bias_level(codec, SND_SOC_BIAS_STANDBY); return 0; } @@ -697,15 +743,6 @@ static int uda1380_probe(struct platform_device *pdev)
/* power on device */ uda1380_set_bias_level(codec, SND_SOC_BIAS_STANDBY); - /* set clock input */ - switch (pdata->dac_clk) { - case UDA1380_DAC_CLK_SYSCLK: - uda1380_write(codec, UDA1380_CLK, 0); - break; - case UDA1380_DAC_CLK_WSPLL: - uda1380_write(codec, UDA1380_CLK, R00_DAC_CLK); - break; - }
snd_soc_add_controls(codec, uda1380_snd_controls, ARRAY_SIZE(uda1380_snd_controls)); @@ -754,9 +791,6 @@ static int uda1380_register(struct uda1380_priv *uda1380) if (!pdata || !pdata->set_power) return -EINVAL;
- /* we may need to have the clock running here - pH5 */ - pdata->set_power(1); - mutex_init(&codec->mutex); INIT_LIST_HEAD(&codec->dapm_widgets); INIT_LIST_HEAD(&codec->dapm_paths); @@ -776,12 +810,6 @@ static int uda1380_register(struct uda1380_priv *uda1380)
memcpy(codec->reg_cache, uda1380_reg, sizeof(uda1380_reg));
- ret = uda1380_reset(codec); - if (ret < 0) { - dev_err(codec->dev, "Failed to issue reset\n"); - goto err_reset; - } - INIT_WORK(&uda1380->work, uda1380_flush_work);
for (i = 0; i < ARRAY_SIZE(uda1380_dai); i++)
On 26 Jun 2010, at 16:14, Vasily Khoruzhick wrote:
Disable some codec modules in standby mode, completely disable codec in off mode to save some power. Fix suspend/resume: mark mixer regs as dirty on resume to restore mixer values, otherwise driver produces no sound (master is muted by default).
Please remember to CC Liam on ASoC patches.
- for (reg = UDA1380_MVOL; reg < UDA1380_CACHEREGNUM; reg++)
set_bit(reg - 0x10, &uda1380_cache_dirty);
This seems odd, I'd expect the cache to be being marked as clean immediately after sync?
@@ -561,17 +595,38 @@ static int uda1380_set_bias_level(struct snd_soc_codec *codec, enum snd_soc_bias_level level) { int pm = uda1380_read_reg_cache(codec, UDA1380_PM);
struct uda1380_platform_data *pdata = codec->dev->platform_data;
if (codec->bias_level == level)
return 0;switch (level) { case SND_SOC_BIAS_ON: case SND_SOC_BIAS_PREPARE:
/* ADC, DAC on */uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);
Like I said previously you really should look at using DAPM here, this should make the code simpler and will let you
You might also want to consider snd_soc_update_bits().
break;case SND_SOC_BIAS_STANDBY:
uda1380_write(codec, UDA1380_PM, R02_PON_BIAS);
if (codec->bias_level == SND_SOC_BIAS_OFF) {pdata->set_power(1);uda1380_reset(codec);
The reset here seems unneeded and possibly wasteful if there is no power control on the system. It'd seem better to do something like just power up, flagging the cache as dirty if there was a callback. I'd strongly expect that if you are actually controlling power the device will be in the default state anyway.
switch (pdata->dac_clk) {case UDA1380_DAC_CLK_SYSCLK:uda1380_write_reg_cache(codec, UDA1380_CLK, 0);break;case UDA1380_DAC_CLK_WSPLL:uda1380_write_reg_cache(codec, UDA1380_CLK,R00_DAC_CLK);break;}
Why is this being managed every time the device is enabled? Surely the setting can be done once at startup.
- ret = uda1380_reset(codec);
- if (ret < 0) {
dev_err(codec->dev, "Failed to issue reset\n");goto err_reset;- }
The reason for the reset at startup is that we don't know what state the device is in when Linux gets control.
В сообщении от 26 июня 2010 23:45:12 автор Mark Brown написал:
Please remember to CC Liam on ASoC patches.
Ok, I'll do
- for (reg = UDA1380_MVOL; reg < UDA1380_CACHEREGNUM; reg++)
set_bit(reg - 0x10, &uda1380_cache_dirty);This seems odd, I'd expect the cache to be being marked as clean immediately after sync?
Nope, it is not. Only i2c and clock related regs of uda1380 can be modified when there's no i2s clock, i.e. mixer regs should be updated right after i2s clock was enabled, so we marking mixer-related regs cache as dirty, to make sure they'll be updated when possible.
uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);Like I said previously you really should look at using DAPM here, this should make the code simpler and will let you
You might also want to consider snd_soc_update_bits().
Hmm, uda134x driver does pretty same things as in my patch... And it seems part of your sentence is lost :(
break;case SND_SOC_BIAS_STANDBY:
uda1380_write(codec, UDA1380_PM, R02_PON_BIAS);
if (codec->bias_level == SND_SOC_BIAS_OFF) {pdata->set_power(1);uda1380_reset(codec);The reset here seems unneeded and possibly wasteful if there is no power control on the system. It'd seem better to do something like just power up, flagging the cache as dirty if there was a callback. I'd strongly expect that if you are actually controlling power the device will be in the default state anyway.
Ok
switch (pdata->dac_clk) {case UDA1380_DAC_CLK_SYSCLK:uda1380_write_reg_cache(codec, UDA1380_CLK, 0);break;case UDA1380_DAC_CLK_WSPLL:uda1380_write_reg_cache(codec, UDA1380_CLK,R00_DAC_CLK);break;}Why is this being managed every time the device is enabled? Surely the setting can be done once at startup.
Yep, just need to mark cache of this reg as dirty here...
- ret = uda1380_reset(codec);
- if (ret < 0) {
dev_err(codec->dev, "Failed to issue reset\n");goto err_reset;- }
The reason for the reset at startup is that we don't know what state the device is in when Linux gets control.
It's softreset and it's performed by writing some value to some reg. i2c xfers is not possible when codec is not enabled (it is not at this point)
Regards Vasily
On Sun, Jun 27, 2010 at 12:07:26AM +0300, Vasily Khoruzhick wrote:
В сообщении от 26 июня 2010 23:45:12 автор Mark Brown написал:
This seems odd, I'd expect the cache to be being marked as clean immediately after sync?
Nope, it is not. Only i2c and clock related regs of uda1380 can be modified when there's no i2s clock, i.e. mixer regs should be updated right after i2s clock was enabled, so we marking mixer-related regs cache as dirty, to make sure they'll be updated when possible.
This is very much non-obvious from your code - it really needs comments explaining why the cache sync function didn't actually manage to sync the cache. I'd also really expect that the dirtying of the cache would be done when the operation that invalidates it happens, not later on when restoring the cache. Otherwise there's a window where the cache is flagged as valid but is not actually so which doesn't seem robust.
uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);
Like I said previously you really should look at using DAPM here, this should make the code simpler and will let you
Hmm, uda134x driver does pretty same things as in my patch... And it seems part of your sentence is lost :(
Old drivers for fairly obscure chips aren't always a good guide to best practices for things.
- ret = uda1380_reset(codec);
- if (ret < 0) {
dev_err(codec->dev, "Failed to issue reset\n");goto err_reset;- }
The reason for the reset at startup is that we don't know what state the device is in when Linux gets control.
It's softreset and it's performed by writing some value to some reg. i2c xfers is not possible when codec is not enabled (it is not at this point)
It needs to happen at some point.
В сообщении от 27 июня 2010 13:10:54 автор Mark Brown написал:
Nope, it is not. Only i2c and clock related regs of uda1380 can be modified when there's no i2s clock, i.e. mixer regs should be updated right after i2s clock was enabled, so we marking mixer-related regs cache as dirty, to make sure they'll be updated when possible.
This is very much non-obvious from your code - it really needs comments explaining why the cache sync function didn't actually manage to sync the cache. I'd also really expect that the dirtying of the cache would be done when the operation that invalidates it happens, not later on when restoring the cache. Otherwise there's a window where the cache is flagged as valid but is not actually so which doesn't seem robust.
Ok I'll add comments, but there's no window where cache is marked as valid but is no consistent with hardware - resume callback marks cache as dirty immediately :)
Old drivers for fairly obscure chips aren't always a good guide to best practices for things.
Ok, what driver should I use as reference?
It's softreset and it's performed by writing some value to some reg. i2c xfers is not possible when codec is not enabled (it is not at this point)
It needs to happen at some point.
Softreset is performed right after codec power on, it can't be performed earlier.
On 27 Jun 2010, at 11:43, Vasily Khoruzhick wrote:
В сообщении от 27 июня 2010 13:10:54 автор Mark Brown написал:
Old drivers for fairly obscure chips aren't always a good guide to best practices for things.
Ok, what driver should I use as reference?
Off the top of my head WM8904 does regulator based power management.
It's softreset and it's performed by writing some value to some reg. i2c xfers is not possible when codec is not enabled (it is not at this point)
It needs to happen at some point.
Softreset is performed right after codec power on, it can't be performed earlier.
After power on it's not needed, the device should be in the default state already. It's only required on startup when there's no power control for the device.
В сообщении от 27 июня 2010 23:55:29 автор Mark Brown написал:
After power on it's not needed, the device should be in the default state already. It's only required on startup when there's no power control for the device.
But that's not possible to reset codec if it is not powered on :(
On 27 Jun 2010, at 22:15, Vasily Khoruzhick anarsoul@gmail.com wrote:
В сообщении от 27 июня 2010 23:55:29 автор Mark Brown написал:
After power on it's not needed, the device should be in the default state already. It's only required on startup when there's no power control for the device.
But that's not possible to reset codec if it is not powered on :(
So don't reset it if you have control of the power? Remember, not all systems will have soft control of the power for the device.
participants (2)
-
Mark Brown -
Vasily Khoruzhick