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

From: Nicolas Boichat <drinkcat@chromium.org>
To: Mark Brown <broonie@kernel.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:05:30 +0800	[thread overview]
Message-ID: <20150715010530.GA10273@google.com> (raw)
In-Reply-To: <20150714102833.GP11162@sirena.org.uk>

On Tue, Jul 14, 2015 at 6:28 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 14, 2015 at 10:09:44AM +0000, Bard Liao wrote:
>
>> Thanks for the review. I think what we need is something like
>> +             snd_soc_dapm_force_enable_pin(dapm, "ADC L power");
>> +             snd_soc_dapm_force_enable_pin(dapm, "ADC R power");
>> +             snd_soc_dapm_sync(dapm);
>> +             if (!codec->component.card->instantiated) {
>> +                     regmap_update_bits(rt5645->regmap, RT5645_PWR_DIG1,
>> +                             RT5645_PWR_ADC_L_BIT | RT5645_PWR_ADC_R_BIT,
>> +                             RT5645_PWR_ADC_L_BIT | RT5645_PWR_ADC_R_BIT);
>> +             }
>
> Yes, that's more what I'd expect.  You could probably just do the regmap
> update unconditionally since it shouldn't make any difference but it's a
> bit neater this way.

rt5645_enable_push_button_irq (where this code is added), is only called
from rt5645_jack_detect, where this kind of pattern is currently common:

if (codec->component.card->instantiated)
	snd_soc_dapm_force_enable_pin(dapm, ...)
else
	regmap_update_bits(...)

Not saying this is right, but if we fix this one we should fix them all.

The problem that I'm trying to solve with this series, is that rt5645->codec
might still be null when rt5645_jack_detect and rt5645_enable_push_button_irq
are first called, so in some cases we do not have a valid dapm pointer yet,
and the test above is modified in 3/3 of the series...

If you look at patch 3/3 of the series, I do something like this, early in
the function:
+       struct snd_soc_dapm_context *dapm = NULL;
+
+       if (rt5645->codec && rt5645->codec->component.card->instantiated) {
+               dapm = snd_soc_codec_get_dapm(rt5645->codec);
+       }

and then use this pattern:

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?

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(...)

And then 3/3 would add the if (dapm) tests?

Thanks for the feedback.