Re: [PATCH v2 1/3] ASoC: rt5645: Simplify rt5645_enable_push_button_irq

From: Mark Brown <broonie@kernel.org>
To: Nicolas Boichat <drinkcat@chromium.org>
Cc: Oder Chiou <oder_chiou@realtek.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	Takashi Iwai <tiwai@suse.de>, Liam Girdwood <lgirdwood@gmail.com>,
	"koro.chen@mediatek.com" <koro.chen@mediatek.com>,
	Bard Liao <bardliao@realtek.com>
Subject: Re: [PATCH v2 1/3] ASoC: rt5645: Simplify rt5645_enable_push_button_irq
Date: Wed, 15 Jul 2015 09:50:12 +0100	[thread overview]
Message-ID: <20150715085012.GJ11162@sirena.org.uk> (raw)
In-Reply-To: <20150715010530.GA10273@google.com>

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

On Wed, Jul 15, 2015 at 09:05:30AM +0800, Nicolas Boichat wrote:
> On Tue, Jul 14, 2015 at 6:28 PM, Mark Brown <broonie@kernel.org> wrote:

> if (dapm)
> 	snd_soc_dapm_force_enable_pin(dapm, ...)
> else
> 	regmap_update_bits(...)

> If guess something like this might be preferable:
> if (rt5645->codec) {
> 	dapm = snd_soc_codec_get_dapm(rt5645->codec);
> }

> and then:

> if (dapm)
> 	snd_soc_dapm_force_enable_pin(dapm, ...)

> regmap_update_bits(...)

> Does that make sense?

No, that still has the problem that you don't handle the !dapm case
properly since as soon as DAPM kicks in it'll power everything off.

> Is there a better way to communicate my intent in this series? Maybe
> patch 1/3 should convert everyhing to this pattern:
> snd_soc_dapm_force_enable_pin(dapm, ...)
> regmap_update_bits(...)

Your intent is clear, the problem is that the code doesn't actually do
what it's supposed to do - see previous e-mail.

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

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