Message ID | 1405704668-8038-2-git-send-email-dmurphy@ti.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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.
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:
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(-)