[alsa-devel] [PATCH 1/2] ASoC: tas2552: Fix PM sequencing
In the pm suspend/resume it is better to disable the GPIO after the regmap_cache setting calls so that if the call is interrupted the new reg values will be cached and set on resume.
Also add pm_runtime_put in the remove call.
Signed-off-by: Dan Murphy dmurphy@ti.com --- sound/soc/codecs/tas2552.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/tas2552.c b/sound/soc/codecs/tas2552.c index f0760af..a3ae394 100644 --- a/sound/soc/codecs/tas2552.c +++ b/sound/soc/codecs/tas2552.c @@ -239,12 +239,12 @@ static int tas2552_runtime_suspend(struct device *dev)
tas2552_sw_shutdown(tas2552, 0);
- if (tas2552->enable_gpio) - gpiod_set_value(tas2552->enable_gpio, 0); - regcache_cache_only(tas2552->regmap, true); regcache_mark_dirty(tas2552->regmap);
+ if (tas2552->enable_gpio) + gpiod_set_value(tas2552->enable_gpio, 0); + return 0; }
@@ -382,6 +382,8 @@ static int tas2552_codec_remove(struct snd_soc_codec *codec) { struct tas2552_data *tas2552 = snd_soc_codec_get_drvdata(codec);
+ pm_runtime_put(codec->dev); + if (tas2552->enable_gpio) gpiod_set_value(tas2552->enable_gpio, 0);
Add DAPM calls to enable/disable the Class D amp. Also add a DAPM call to turn off the PLL upon the stream completing.
Signed-off-by: Dan Murphy dmurphy@ti.com --- sound/soc/codecs/tas2552.c | 58 +++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 17 deletions(-)
diff --git a/sound/soc/codecs/tas2552.c b/sound/soc/codecs/tas2552.c index a3ae394..3fdb173 100644 --- a/sound/soc/codecs/tas2552.c +++ b/sound/soc/codecs/tas2552.c @@ -78,6 +78,40 @@ struct tas2552_data { unsigned int mclk; };
+ +static int tas2552_pll_disable(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + if (event == SND_SOC_DAPM_POST_PMD) + snd_soc_update_bits(w->codec, TAS2552_CFG_2, TAS2552_PLL_ENABLE, 0); + + return 0; +} + +static int tas2552_class_d_en(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + snd_soc_update_bits(w->codec, TAS2552_CFG_2, + TAS2552_CLASSD_EN_MASK, TAS2552_CLASSD_EN_MASK); + break; + case SND_SOC_DAPM_POST_PMD: + snd_soc_update_bits(w->codec, TAS2552_CFG_2, + TAS2552_CLASSD_EN_MASK, 0); + break; + } + + return 0; +} + +static const struct snd_soc_dapm_widget tas2552_dapm_widgets[] = +{ +SND_SOC_DAPM_PRE("Class D Enable", tas2552_class_d_en), +SND_SOC_DAPM_POST("Class D Disable", tas2552_class_d_en), +SND_SOC_DAPM_POST("PLL Disable", tas2552_pll_disable), +}; + static void tas2552_sw_shutdown(struct tas2552_data *tas_data, int sw_shutdown) { u8 cfg1_reg; @@ -101,10 +135,6 @@ static int tas2552_hw_params(struct snd_pcm_substream *substream, int d; u8 p, j;
- /* Turn on Class D amplifier */ - snd_soc_update_bits(codec, TAS2552_CFG_2, TAS2552_CLASSD_EN_MASK, - TAS2552_CLASSD_EN); - if (!tas2552->mclk) return -EINVAL;
@@ -149,7 +179,6 @@ static int tas2552_hw_params(struct snd_pcm_substream *substream,
snd_soc_update_bits(codec, TAS2552_CFG_2, TAS2552_PLL_ENABLE, TAS2552_PLL_ENABLE); - return 0; }
@@ -269,19 +298,10 @@ static const struct dev_pm_ops tas2552_pm = { NULL) };
-static void tas2552_shutdown(struct snd_pcm_substream *substream, - struct snd_soc_dai *dai) -{ - struct snd_soc_codec *codec = dai->codec; - - snd_soc_update_bits(codec, TAS2552_CFG_2, TAS2552_PLL_ENABLE, 0); -} - static struct snd_soc_dai_ops tas2552_speaker_dai_ops = { .hw_params = tas2552_hw_params, .set_sysclk = tas2552_set_dai_sysclk, .set_fmt = tas2552_set_dai_fmt, - .shutdown = tas2552_shutdown, .digital_mute = tas2552_mute, };
@@ -321,6 +341,7 @@ static const struct reg_default tas2552_init_regs[] = { static int tas2552_codec_probe(struct snd_soc_codec *codec) { struct tas2552_data *tas2552 = snd_soc_codec_get_drvdata(codec); + struct snd_soc_dapm_context *dapm = &codec->dapm; int ret;
tas2552->codec = codec; @@ -362,9 +383,12 @@ static int tas2552_codec_probe(struct snd_soc_codec *codec) goto patch_fail; }
- snd_soc_write(codec, TAS2552_CFG_2, TAS2552_CLASSD_EN | - TAS2552_BOOST_EN | TAS2552_APT_EN | - TAS2552_LIM_EN); + snd_soc_write(codec, TAS2552_CFG_2, TAS2552_BOOST_EN | + TAS2552_APT_EN | TAS2552_LIM_EN); + + snd_soc_dapm_new_controls(dapm, tas2552_dapm_widgets, + ARRAY_SIZE(tas2552_dapm_widgets)); + return 0;
patch_fail:
On Fri, Jul 18, 2014 at 12:31:08PM -0500, Dan Murphy wrote:
+static int tas2552_class_d_en(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
- switch (event) {
- case SND_SOC_DAPM_PRE_PMU:
snd_soc_update_bits(w->codec, TAS2552_CFG_2,TAS2552_CLASSD_EN_MASK, TAS2552_CLASSD_EN_MASK);break;- case SND_SOC_DAPM_POST_PMD:
snd_soc_update_bits(w->codec, TAS2552_CFG_2,TAS2552_CLASSD_EN_MASK, 0);break;- }
- return 0;
+}
+static const struct snd_soc_dapm_widget tas2552_dapm_widgets[] = +{ +SND_SOC_DAPM_PRE("Class D Enable", tas2552_class_d_en), +SND_SOC_DAPM_POST("Class D Disable", tas2552_class_d_en), +SND_SOC_DAPM_POST("PLL Disable", tas2552_pll_disable), +};
This seems broken, having to use _PRE or _POST widgets for simple register writes (or almost anything really) should never be required and error prone - what is this actually trying to do? I'd expect the class D to be a PGA or OUTPUT widget and the PLL to be a SUPPLY widget.
Mark On 07/21/2014 06:54 AM, Mark Brown wrote:
On Fri, Jul 18, 2014 at 12:31:08PM -0500, Dan Murphy wrote:
+static int tas2552_class_d_en(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *kcontrol, int event) +{ + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + snd_soc_update_bits(w->codec, TAS2552_CFG_2, + TAS2552_CLASSD_EN_MASK, TAS2552_CLASSD_EN_MASK); + break; + case SND_SOC_DAPM_POST_PMD: + snd_soc_update_bits(w->codec, TAS2552_CFG_2, + TAS2552_CLASSD_EN_MASK, 0); + break; + } + + return 0; +} + +static const struct snd_soc_dapm_widget tas2552_dapm_widgets[] = +{ +SND_SOC_DAPM_PRE("Class D Enable", tas2552_class_d_en), +SND_SOC_DAPM_POST("Class D Disable", tas2552_class_d_en), +SND_SOC_DAPM_POST("PLL Disable", tas2552_pll_disable), +};
This seems broken, having to use _PRE or _POST widgets for simple register writes (or almost anything really) should never be required and error prone - what is this actually trying to do? I'd expect the class D to be a PGA or OUTPUT widget and the PLL to be a SUPPLY widget.
I need a little help here. I am not seeing the PLL being disabled or Class D being disabled when I am using the DAPM calls
My implementation is like this. And I use set these widgets and audio map in the call back functions in the struct snd_soc_codec_driver.
When I look at the registers after stream playback I see that the TAS2552_CFG_2 register is untouched
static const struct snd_soc_dapm_widget tas2552_dapm_widgets[] = { SND_SOC_DAPM_AIF_IN("DAC IN", NULL, 0, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_DAC("DAC", NULL, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_OUT_DRV("ClassD Enable", TAS2552_CFG_2, 7, 0, NULL, 0), SND_SOC_DAPM_OUT_DRV("ClassD Disable", TAS2552_CFG_2, 7, 1, NULL, 0), SND_SOC_DAPM_SUPPLY("PLL Disable", TAS2552_CFG_2, 3, 1, NULL, 0), SND_SOC_DAPM_OUTPUT("OUT") };
static const struct snd_soc_dapm_route tas2552_audio_map[] = { {"DAC", NULL, "DAC IN"}, {"ClassD Enable", NULL, "DAC"}, {"OUT", NULL, "ClassD Enable"}, {"ClassD Disable", NULL, "OUT"}, {"PLL Disable", NULL, "ClassD Disable"} };
Any information would help here
Thanks in advance
-- ------------------ Dan Murphy
On 07/21/2014 06:54 AM, Mark Brown wrote:
On Fri, Jul 18, 2014 at 12:31:08PM -0500, Dan Murphy wrote:
+static int tas2552_class_d_en(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
- switch (event) {
- case SND_SOC_DAPM_PRE_PMU:
snd_soc_update_bits(w->codec, TAS2552_CFG_2,TAS2552_CLASSD_EN_MASK, TAS2552_CLASSD_EN_MASK);break;- case SND_SOC_DAPM_POST_PMD:
snd_soc_update_bits(w->codec, TAS2552_CFG_2,TAS2552_CLASSD_EN_MASK, 0);break;- }
- return 0;
+}
+static const struct snd_soc_dapm_widget tas2552_dapm_widgets[] = +{ +SND_SOC_DAPM_PRE("Class D Enable", tas2552_class_d_en), +SND_SOC_DAPM_POST("Class D Disable", tas2552_class_d_en), +SND_SOC_DAPM_POST("PLL Disable", tas2552_pll_disable), +};
This seems broken, having to use _PRE or _POST widgets for simple register writes (or almost anything really) should never be required and error prone - what is this actually trying to do? I'd expect the class D to be a PGA or OUTPUT widget and the PLL to be a SUPPLY widget.
I need a little help here. I am not seeing the PLL being disabled or Class D being disabled when I am using the DAPM calls
My implementation is like this. And I use set these widgets and audio map in the call back functions in the struct snd_soc_codec_driver.
When I look at the registers after stream playback I see that the TAS2552_CFG_2 register is untouched
static const struct snd_soc_dapm_widget tas2552_dapm_widgets[] = { SND_SOC_DAPM_AIF_IN("DAC IN", NULL, 0, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_DAC("DAC", NULL, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_OUT_DRV("ClassD Enable", TAS2552_CFG_2, 7, 0, NULL, 0), SND_SOC_DAPM_OUT_DRV("ClassD Disable", TAS2552_CFG_2, 7, 1, NULL, 0), SND_SOC_DAPM_SUPPLY("PLL Disable", TAS2552_CFG_2, 3, 1, NULL, 0), SND_SOC_DAPM_OUTPUT("OUT") };
static const struct snd_soc_dapm_route tas2552_audio_map[] = { {"DAC", NULL, "DAC IN"}, {"ClassD Enable", NULL, "DAC"}, {"OUT", NULL, "ClassD Enable"}, {"ClassD Disable", NULL, "OUT"}, {"PLL Disable", NULL, "ClassD Disable"} };
Any information would help here
Thanks in advance
On Fri, Jul 25, 2014 at 06:54:19PM +0000, Murphy, Dan wrote:
On 07/21/2014 06:54 AM, Mark Brown wrote:
On Fri, Jul 18, 2014 at 12:31:08PM -0500, Dan Murphy wrote:
+static const struct snd_soc_dapm_widget tas2552_dapm_widgets[] = +{ +SND_SOC_DAPM_PRE("Class D Enable", tas2552_class_d_en), +SND_SOC_DAPM_POST("Class D Disable", tas2552_class_d_en), +SND_SOC_DAPM_POST("PLL Disable", tas2552_pll_disable), +};
This seems broken, having to use _PRE or _POST widgets for simple register writes (or almost anything really) should never be required and error prone - what is this actually trying to do? I'd expect the class D to be a PGA or OUTPUT widget and the PLL to be a SUPPLY widget.
I need a little help here. I am not seeing the PLL being disabled or Class D being disabled when I am using the DAPM calls
My implementation is like this. And I use set these widgets and audio map in the call back functions in the struct snd_soc_codec_driver.
When I look at the registers after stream playback I see that the TAS2552_CFG_2 register is untouched
static const struct snd_soc_dapm_widget tas2552_dapm_widgets[] = { SND_SOC_DAPM_AIF_IN("DAC IN", NULL, 0, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_DAC("DAC", NULL, SND_SOC_NOPM, 0, 0), SND_SOC_DAPM_OUT_DRV("ClassD Enable", TAS2552_CFG_2, 7, 0, NULL, 0), SND_SOC_DAPM_OUT_DRV("ClassD Disable", TAS2552_CFG_2, 7, 1, NULL, 0), SND_SOC_DAPM_SUPPLY("PLL Disable", TAS2552_CFG_2, 3, 1, NULL, 0), SND_SOC_DAPM_OUTPUT("OUT") };
static const struct snd_soc_dapm_route tas2552_audio_map[] = { {"DAC", NULL, "DAC IN"}, {"ClassD Enable", NULL, "DAC"}, {"OUT", NULL, "ClassD Enable"}, {"ClassD Disable", NULL, "OUT"}, {"PLL Disable", NULL, "ClassD Disable"} };
This looks very confused, why do you have separate enable and disable things - what do you think the effect of the above should be? That's probably what's going wrong. I'd expect to see one DAPM widget for the class D and one for the PLL.
On Fri, Jul 18, 2014 at 12:31:07PM -0500, Dan Murphy wrote:
In the pm suspend/resume it is better to disable the GPIO after the regmap_cache setting calls so that if the call is interrupted the new reg values will be cached and set on resume.
Also add pm_runtime_put in the remove call.
Applied, thanks, but please don't mix multiple changes into one commit like this especially when there's no textual overlap.
Mark
On 07/21/2014 07:19 AM, Mark Brown wrote:
On Fri, Jul 18, 2014 at 12:31:07PM -0500, Dan Murphy wrote:
In the pm suspend/resume it is better to disable the GPIO after the regmap_cache setting calls so that if the call is interrupted the new reg values will be cached and set on resume.
Also add pm_runtime_put in the remove call.
Applied, thanks, but please don't mix multiple changes into one commit like this especially when there's no textual overlap.
I usually don't mix changes like this as I like to have a single change per commit.
But I find each maintainer is a little different but now I know for next time.
Thanks
participants (3)
-
Dan Murphy -
Mark Brown -
Murphy, Dan