diff mbox

[2/2] ASoC: max98090: Enforce correct device sequencing when configuring a new

Message ID 1438902946-49633-2-git-send-email-yang.a.fang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

yang.a.fang@intel.com Aug. 6, 2015, 11:15 p.m. UTC 
From: "Fang, Yang A" <yang.a.fang@intel.com>

audio route when there is an existing active audio route(s).

This patch fixed recording noise issue while playback is active.

We have some registers which require the device to be in full shutdown
or to enter full shutdown before the register settings will take effect.
Currently the driver is not shutting down the device when a new audio
route is created. If a new audio route is made active while there is
already an active audio route, then the required register sequencing is
violated. A hardware shutdown toggle when creating a new audio route
corrects the sequencing error. The device must remain in hardware
shutdown for 40ms to allow the internal hardware core to fully shutdown.

Signed-off-by: Fang, Yang A <yang.a.fang@intel.com>
Signed-off-by: Sathyanarayana Nujella <sathyanarayana.nujella@intel.com>
---
 sound/soc/codecs/max98090.c |   46 +++++++++++++++++++++++++++++++++++++------
 sound/soc/codecs/max98090.h |    1 +
 2 files changed, 41 insertions(+), 6 deletions(-)

Comments

Mark Brown Aug. 7, 2015, 10:43 a.m. UTC | #1 
On Thu, Aug 06, 2015 at 04:15:45PM -0700, yang.a.fang@intel.com wrote:

> +static int max98090_shdn_event(struct snd_soc_dapm_widget *w,
> +				 struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	struct max98090_priv *max98090 = snd_soc_codec_get_drvdata(codec);
> +
> +	if (event & SND_SOC_DAPM_POST_PMU)
> +		max98090->shdn_pending = TRUE;

TRUE?  Please use normal C99 booleans like we normally do in the kernel.

> +static void max98090_seq_notifier(struct snd_soc_dapm_context *dapm,
> +	enum snd_soc_dapm_type event, int subseq, bool power_up)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(dapm);
> +	struct max98090_priv *max98090 = snd_soc_codec_get_drvdata(codec);
> +
> +	if (max98090->shdn_pending && power_up) {
> +		snd_soc_update_bits(codec, M98090_REG_DEVICE_SHUTDOWN,
> +				M98090_SHDNN_MASK, 0);
> +		msleep(40);
> +		snd_soc_update_bits(codec, M98090_REG_DEVICE_SHUTDOWN,
> +				M98090_SHDNN_MASK, M98090_SHDNN_MASK);
> +		max98090->shdn_pending = FALSE;
> +	}
> +}

Why did you change the API for this?  As far as I can tell power_up is
redundant here, we already know we're powering things up because we got
a _PMU event, the main effect here appears to be to just run this at the
next sequence point after we've powered up a widget with a shutdown
event.  The general concept with the seq_notifier is that we record
things we need to do in the per-widget events and then implement them in
the seq_notifier and I'm not seeing anything else going on here.
yang.a.fang@intel.com Aug. 7, 2015, 8:50 p.m. UTC | #2 
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Friday, August 07, 2015 3:43 AM
> To: Fang, Yang A
> Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org;
> dgreid@chromium.org; Nujella, Sathyanarayana;
> kevin.strasser@linux.intel.com; Sripathi, Srinivas; Iriawan, Denny; Jain,
> Praveen K; Anish.Kumar@maximintegrated.com; Eoff, Ullysses A
> Subject: Re: [PATCH 2/2] ASoC: max98090: Enforce correct device sequencing
> when configuring a new
> 
> On Thu, Aug 06, 2015 at 04:15:45PM -0700, yang.a.fang@intel.com wrote:
> 
> > +static int max98090_shdn_event(struct snd_soc_dapm_widget *w,
> > +				 struct snd_kcontrol *kcontrol, int event) {
> > +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> > +	struct max98090_priv *max98090 =
> snd_soc_codec_get_drvdata(codec);
> > +
> > +	if (event & SND_SOC_DAPM_POST_PMU)
> > +		max98090->shdn_pending = TRUE;
> 
> TRUE?  Please use normal C99 booleans like we normally do in the kernel.
I will fix this
> 
> > +static void max98090_seq_notifier(struct snd_soc_dapm_context *dapm,
> > +	enum snd_soc_dapm_type event, int subseq, bool power_up) {
> > +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(dapm);
> > +	struct max98090_priv *max98090 =
> snd_soc_codec_get_drvdata(codec);
> > +
> > +	if (max98090->shdn_pending && power_up) {
> > +		snd_soc_update_bits(codec,
> M98090_REG_DEVICE_SHUTDOWN,
> > +				M98090_SHDNN_MASK, 0);
> > +		msleep(40);
> > +		snd_soc_update_bits(codec,
> M98090_REG_DEVICE_SHUTDOWN,
> > +				M98090_SHDNN_MASK,
> M98090_SHDNN_MASK);
> > +		max98090->shdn_pending = FALSE;
> > +	}
> > +}
> 
> Why did you change the API for this?  As far as I can tell power_up is
> redundant here, we already know we're powering things up because we got
> a _PMU event, the main effect here appears to be to just run this at the next
> sequence point after we've powered up a widget with a shutdown event.
> The general concept with the seq_notifier is that we record things we need
> to do in the per-widget events and then implement them in the seq_notifier
> and I'm not seeing anything else going on here.
Thanks Mark.   Right , _PMU already indicates it is power_up event.. I will drop
This API change.
diff mbox

Patch

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 91c9f9f..1516c6b 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -850,6 +850,19 @@  static int max98090_micinput_event(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int max98090_shdn_event(struct snd_soc_dapm_widget *w,
+				 struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct max98090_priv *max98090 = snd_soc_codec_get_drvdata(codec);
+
+	if (event & SND_SOC_DAPM_POST_PMU)
+		max98090->shdn_pending = TRUE;
+
+	return 0;
+
+}
+
 static const char *mic1_mux_text[] = { "IN12", "IN56" };
 
 static SOC_ENUM_SINGLE_DECL(mic1_mux_enum,
@@ -1158,9 +1171,11 @@  static const struct snd_soc_dapm_widget max98090_dapm_widgets[] = {
 	SND_SOC_DAPM_SUPPLY("SDOEN", M98090_REG_IO_CONFIGURATION,
 		M98090_SDOEN_SHIFT, 0, NULL, 0),
 	SND_SOC_DAPM_SUPPLY("DMICL_ENA", M98090_REG_DIGITAL_MIC_ENABLE,
-		 M98090_DIGMICL_SHIFT, 0, NULL, 0),
+		 M98090_DIGMICL_SHIFT, 0, max98090_shdn_event,
+			SND_SOC_DAPM_POST_PMU),
 	SND_SOC_DAPM_SUPPLY("DMICR_ENA", M98090_REG_DIGITAL_MIC_ENABLE,
-		 M98090_DIGMICR_SHIFT, 0, NULL, 0),
+		 M98090_DIGMICR_SHIFT, 0, max98090_shdn_event,
+			 SND_SOC_DAPM_POST_PMU),
 	SND_SOC_DAPM_SUPPLY("AHPF", M98090_REG_FILTER_CONFIG,
 		M98090_AHPF_SHIFT, 0, NULL, 0),
 
@@ -1205,10 +1220,12 @@  static const struct snd_soc_dapm_widget max98090_dapm_widgets[] = {
 		&max98090_right_adc_mixer_controls[0],
 		ARRAY_SIZE(max98090_right_adc_mixer_controls)),
 
-	SND_SOC_DAPM_ADC("ADCL", NULL, M98090_REG_INPUT_ENABLE,
-		M98090_ADLEN_SHIFT, 0),
-	SND_SOC_DAPM_ADC("ADCR", NULL, M98090_REG_INPUT_ENABLE,
-		M98090_ADREN_SHIFT, 0),
+	SND_SOC_DAPM_ADC_E("ADCL", NULL, M98090_REG_INPUT_ENABLE,
+		M98090_ADLEN_SHIFT, 0, max98090_shdn_event,
+		SND_SOC_DAPM_POST_PMU),
+	SND_SOC_DAPM_ADC_E("ADCR", NULL, M98090_REG_INPUT_ENABLE,
+		M98090_ADREN_SHIFT, 0, max98090_shdn_event,
+		SND_SOC_DAPM_POST_PMU),
 
 	SND_SOC_DAPM_AIF_OUT("AIFOUTL", "HiFi Capture", 0,
 		SND_SOC_NOPM, 0, 0),
@@ -2539,9 +2556,26 @@  static int max98090_remove(struct snd_soc_codec *codec)
 	return 0;
 }
 
+static void max98090_seq_notifier(struct snd_soc_dapm_context *dapm,
+	enum snd_soc_dapm_type event, int subseq, bool power_up)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(dapm);
+	struct max98090_priv *max98090 = snd_soc_codec_get_drvdata(codec);
+
+	if (max98090->shdn_pending && power_up) {
+		snd_soc_update_bits(codec, M98090_REG_DEVICE_SHUTDOWN,
+				M98090_SHDNN_MASK, 0);
+		msleep(40);
+		snd_soc_update_bits(codec, M98090_REG_DEVICE_SHUTDOWN,
+				M98090_SHDNN_MASK, M98090_SHDNN_MASK);
+		max98090->shdn_pending = FALSE;
+	}
+}
+
 static struct snd_soc_codec_driver soc_codec_dev_max98090 = {
 	.probe   = max98090_probe,
 	.remove  = max98090_remove,
+	.seq_notifier = max98090_seq_notifier,
 	.set_bias_level = max98090_set_bias_level,
 };
 
diff --git a/sound/soc/codecs/max98090.h b/sound/soc/codecs/max98090.h
index 21ff743..bc610d9 100644
--- a/sound/soc/codecs/max98090.h
+++ b/sound/soc/codecs/max98090.h
@@ -1543,6 +1543,7 @@  struct max98090_priv {
 	unsigned int pa2en;
 	unsigned int sidetone;
 	bool master;
+	bool shdn_pending;
 };
 
 int max98090_mic_detect(struct snd_soc_codec *codec,