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

From: Mark Brown <broonie@kernel.org>
To: yang.a.fang@intel.com
Cc: alsa-devel@alsa-project.org, srinivas.sripathi@intel.com,
	Anish.Kumar@maximintegrated.com, praveen.k.jain@intel.com,
	lgirdwood@gmail.com, denny.iriawan@intel.com,
	sathyanarayana.nujella@intel.com, ullysses.a.eoff@intel.com,
	kevin.strasser@linux.intel.com, dgreid@chromium.org
Subject: Re: [PATCH 2/2] ASoC: max98090: Enforce correct device sequencing when configuring a new
Date: Fri, 7 Aug 2015 11:43:11 +0100	[thread overview]
Message-ID: <20150807104311.GM20873@sirena.org.uk> (raw)
In-Reply-To: <1438902946-49633-2-git-send-email-yang.a.fang@intel.com>

[-- Attachment #1.1: Type: text/plain, Size: 1557 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]