[v2,1/3] ASoC: rt5645: Simplify rt5645_enable_push_button_irq
diff mbox

Message ID 1436856687-31550-2-git-send-email-drinkcat@chromium.org
State New
Headers show

Commit Message

Nicolas Boichat July 14, 2015, 6:51 a.m. UTC
LDO2/Mic Det Power pins are already enabled/disabled in rt5645_jack_detect
(the jack out code path previously did not disable those: modify it to make
it so).

Also, provide an alternative if dapm is not ready yet.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---
 sound/soc/codecs/rt5645.c | 55 +++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

Comments

Bard Liao July 14, 2015, 9:48 a.m. UTC | #1
> -----Original Message-----
> From: Nicolas Boichat [mailto:drinkcat@chromium.org]
> Sent: Tuesday, July 14, 2015 2:51 PM
> To: Mark Brown
> Cc: Bard Liao; Oder Chiou; Liam Girdwood; Jaroslav Kysela; Takashi Iwai;
> alsa-devel@alsa-project.org; koro.chen@mediatek.com
> Subject: [PATCH v2 1/3] ASoC: rt5645: Simplify
> rt5645_enable_push_button_irq
> 
> LDO2/Mic Det Power pins are already enabled/disabled in
> rt5645_jack_detect (the jack out code path previously did not disable
> those: modify it to make it so).
> 
> Also, provide an alternative if dapm is not ready yet.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>

Acked-by: Bard Liao <bardliao@realtek.com>
Mark Brown July 14, 2015, 9:52 a.m. UTC | #2
On Tue, Jul 14, 2015 at 02:51:25PM +0800, Nicolas Boichat wrote:

> +		if (codec->component.card->instantiated) {
> +			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);
> +		} else {
> +			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);
> +		}

I don't understand why this isn't updating the DAPM state if the device
is not yet instantiated - this means that when the card instantiates the
pins will be turned off which presumably isn't what you want given the
manual register map futzing in the else case.  What's going on here?
Bard Liao July 14, 2015, 10:09 a.m. UTC | #3
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Tuesday, July 14, 2015 5:53 PM
> To: Nicolas Boichat
> Cc: Bard Liao; Oder Chiou; Liam Girdwood; Jaroslav Kysela; Takashi Iwai;
> alsa-devel@alsa-project.org; koro.chen@mediatek.com
> Subject: Re: [PATCH v2 1/3] ASoC: rt5645: Simplify
> rt5645_enable_push_button_irq
> 
> On Tue, Jul 14, 2015 at 02:51:25PM +0800, Nicolas Boichat wrote:
> 
> > +		if (codec->component.card->instantiated) {
> > +			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);
> > +		} else {
> > +			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);
> > +		}
> 
> I don't understand why this isn't updating the DAPM state if the device is
> not yet instantiated - this means that when the card instantiates the pins
> will be turned off which presumably isn't what you want given the manual
> register map futzing in the else case.  What's going on here?

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);
+		}

> 
> ------Please consider the environment before printing this e-mail.
Mark Brown July 14, 2015, 10:28 a.m. UTC | #4
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.
Nicolas Boichat July 15, 2015, 1:05 a.m. UTC | #5
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.
Mark Brown July 15, 2015, 8:50 a.m. UTC | #6
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.
Nicolas Boichat July 15, 2015, 11:50 a.m. UTC | #7
On Tue, Jul 14, 2015 at 6:09 PM, Bard Liao <bardliao@realtek.com> wrote:
>> -----Original Message-----
>> From: Mark Brown [mailto:broonie@kernel.org]
>> Sent: Tuesday, July 14, 2015 5:53 PM
>> To: Nicolas Boichat
>> Cc: Bard Liao; Oder Chiou; Liam Girdwood; Jaroslav Kysela; Takashi Iwai;
>> alsa-devel@alsa-project.org; koro.chen@mediatek.com
>> Subject: Re: [PATCH v2 1/3] ASoC: rt5645: Simplify
>> rt5645_enable_push_button_irq
>>
>> On Tue, Jul 14, 2015 at 02:51:25PM +0800, Nicolas Boichat wrote:
>>
>> > +           if (codec->component.card->instantiated) {
>> > +                   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);
>> > +           } else {
>> > +                   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);
>> > +           }
>>
>> I don't understand why this isn't updating the DAPM state if the device is
>> not yet instantiated - this means that when the card instantiates the pins
>> will be turned off which presumably isn't what you want given the manual
>> register map futzing in the else case.  What's going on here?
>
> 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);
> +               }

Just to make sure I understand... With the code above, the dapm state
is consistent. However, DAPM will only set the regmap bits when the
card is instantiated. So why do we still need to update the regmap? To
make sure we do not miss an early jack/button event? Or would we still
get jack irq if the pins are enabled a little later? (I guess we can
live with missing a button event at that stage, but we need the jack
state to be correct)

Also, I'm going to update rt5645_irq_detection to do nothing if the
codec is not initialized yet (just like rt286.c does). That should be
ok as we call rt5645_irq from rt5645_set_jack_detect, after the codec
is ready, which will update the initial jack status, and setup the
DAPM pins. Does that sound ok?

Thanks!
Mark Brown July 15, 2015, 11:56 a.m. UTC | #8
On Wed, Jul 15, 2015 at 07:50:50PM +0800, Nicolas Boichat 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);
> > +               }

> Just to make sure I understand... With the code above, the dapm state
> is consistent. However, DAPM will only set the regmap bits when the
> card is instantiated. So why do we still need to update the regmap? To
> make sure we do not miss an early jack/button event? Or would we still
> get jack irq if the pins are enabled a little later? (I guess we can
> live with missing a button event at that stage, but we need the jack
> state to be correct)

I'm assuming it's something to do with early detection, I don't really
know though.

> Also, I'm going to update rt5645_irq_detection to do nothing if the
> codec is not initialized yet (just like rt286.c does). That should be
> ok as we call rt5645_irq from rt5645_set_jack_detect, after the codec
> is ready, which will update the initial jack status, and setup the
> DAPM pins. Does that sound ok?

Yes.
Bard Liao July 15, 2015, 1:03 p.m. UTC | #9
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Wednesday, July 15, 2015 7:57 PM
> To: Nicolas Boichat
> Cc: Bard Liao; Oder Chiou; Liam Girdwood; Jaroslav Kysela; Takashi Iwai;
> alsa-devel@alsa-project.org; koro.chen@mediatek.com
> Subject: Re: [PATCH v2 1/3] ASoC: rt5645: Simplify
> rt5645_enable_push_button_irq
> 
> On Wed, Jul 15, 2015 at 07:50:50PM +0800, Nicolas Boichat 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);
> > > +               }
> 
> > Just to make sure I understand... With the code above, the dapm state
> > is consistent. However, DAPM will only set the regmap bits when the
> > card is instantiated. So why do we still need to update the regmap? To
> > make sure we do not miss an early jack/button event? Or would we still
> > get jack irq if the pins are enabled a little later? (I guess we can
> > live with missing a button event at that stage, but we need the jack
> > state to be correct)
> 
> I'm assuming it's something to do with early detection, I don't really know
> though.
> 

I think the problem is dapm won't update the bits if card is not
instantiated. And those bits are necessary for the jack or button
detect functions. Without these bits, we may not get irq properly.
That's why we need to update those bits manually.

> 
> ------Please consider the environment before printing this e-mail.
Nicolas Boichat July 16, 2015, 3:05 a.m. UTC | #10
On Wed, Jul 15, 2015 at 9:03 PM, Bard Liao <bardliao@realtek.com> wrote:
>> -----Original Message-----
>> From: Mark Brown [mailto:broonie@kernel.org]
>> Sent: Wednesday, July 15, 2015 7:57 PM
>> To: Nicolas Boichat
>> Cc: Bard Liao; Oder Chiou; Liam Girdwood; Jaroslav Kysela; Takashi Iwai;
>> alsa-devel@alsa-project.org; koro.chen@mediatek.com
>> Subject: Re: [PATCH v2 1/3] ASoC: rt5645: Simplify
>> rt5645_enable_push_button_irq
>>
>> On Wed, Jul 15, 2015 at 07:50:50PM +0800, Nicolas Boichat 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);
>> > > +               }
>>
>> > Just to make sure I understand... With the code above, the dapm state
>> > is consistent. However, DAPM will only set the regmap bits when the
>> > card is instantiated. So why do we still need to update the regmap? To
>> > make sure we do not miss an early jack/button event? Or would we still
>> > get jack irq if the pins are enabled a little later? (I guess we can
>> > live with missing a button event at that stage, but we need the jack
>> > state to be correct)
>>
>> I'm assuming it's something to do with early detection, I don't really know
>> though.
>>
>
> I think the problem is dapm won't update the bits if card is not
> instantiated. And those bits are necessary for the jack or button
> detect functions. Without these bits, we may not get irq properly.
> That's why we need to update those bits manually.

Understood: when !card->instantiated, snc_soc_dapm_sync is a noop.
However, state of the pins set by snd_soc_dapm_force_enable_pin are
still recorded.
Then, when the card is instantiated
(snd-core.c:snd_soc_instantiate_card), snd_soc_dapm_sync is called,
and the regmap bits get updated.

For the push button case, we can afford to wait until the card is
instantiated (we might lose very early button detection, probably not
a big deal...), so we do not need to change anything in
rt5645_enable_push_button_irq.

However, for the mic detection case, in the jack-in case, we need to
set these bits immediately, as the driver wants to report immediately
(e.g. after ~550ms) whether we detected a headset or just a headphone
(in rt5645_jack_detect).

But the jack-out case, and disabling "Mic Det Power" after jack type
detection, can probably wait until the card is instantiated.

Good thing is, this problem is now independent from the kernel panic
issue, so I'll send separate patches.

Patch
diff mbox

diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c
index 9dfa431..45651f4 100644
--- a/sound/soc/codecs/rt5645.c
+++ b/sound/soc/codecs/rt5645.c
@@ -2766,13 +2766,15 @@  static void rt5645_enable_push_button_irq(struct snd_soc_codec *codec,
 	struct rt5645_priv *rt5645 = snd_soc_codec_get_drvdata(codec);
 
 	if (enable) {
-		snd_soc_dapm_mutex_lock(dapm);
-		snd_soc_dapm_force_enable_pin_unlocked(dapm, "ADC L power");
-		snd_soc_dapm_force_enable_pin_unlocked(dapm, "ADC R power");
-		snd_soc_dapm_force_enable_pin_unlocked(dapm, "LDO2");
-		snd_soc_dapm_force_enable_pin_unlocked(dapm, "Mic Det Power");
-		snd_soc_dapm_sync_unlocked(dapm);
-		snd_soc_dapm_mutex_unlock(dapm);
+		if (codec->component.card->instantiated) {
+			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);
+		} else {
+			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);
+		}
 
 		snd_soc_update_bits(codec,
 					RT5645_INT_IRQ_ST, 0x8, 0x8);
@@ -2785,14 +2787,15 @@  static void rt5645_enable_push_button_irq(struct snd_soc_codec *codec,
 		snd_soc_update_bits(codec, RT5650_4BTN_IL_CMD2, 0x8000, 0x0);
 		snd_soc_update_bits(codec, RT5645_INT_IRQ_ST, 0x8, 0x0);
 
-		snd_soc_dapm_mutex_lock(dapm);
-		snd_soc_dapm_disable_pin_unlocked(dapm, "ADC L power");
-		snd_soc_dapm_disable_pin_unlocked(dapm, "ADC R power");
-		if (rt5645->pdata.jd_mode == 0)
-			snd_soc_dapm_disable_pin_unlocked(dapm, "LDO2");
-		snd_soc_dapm_disable_pin_unlocked(dapm, "Mic Det Power");
-		snd_soc_dapm_sync_unlocked(dapm);
-		snd_soc_dapm_mutex_unlock(dapm);
+		if (codec->component.card->instantiated) {
+			snd_soc_dapm_disable_pin(dapm, "ADC L power");
+			snd_soc_dapm_disable_pin(dapm, "ADC R power");
+			snd_soc_dapm_sync(dapm);
+		} else {
+			regmap_update_bits(rt5645->regmap, RT5645_PWR_DIG1,
+				RT5645_PWR_ADC_L_BIT | RT5645_PWR_ADC_R_BIT,
+				0);
+		}
 	}
 }
 
@@ -2852,22 +2855,22 @@  static int rt5645_jack_detect(struct snd_soc_codec *codec, int jack_insert)
 
 	} else { /* jack out */
 		rt5645->jack_type = 0;
+
 		if (rt5645->en_button_func)
 			rt5645_enable_push_button_irq(codec, false);
-		else {
-			if (codec->component.card->instantiated) {
-				if (rt5645->pdata.jd_mode == 0)
-					snd_soc_dapm_disable_pin(dapm, "LDO2");
-				snd_soc_dapm_disable_pin(dapm, "Mic Det Power");
-				snd_soc_dapm_sync(dapm);
-			} else {
-				if (rt5645->pdata.jd_mode == 0)
-					regmap_update_bits(rt5645->regmap,
+
+		if (codec->component.card->instantiated) {
+			if (rt5645->pdata.jd_mode == 0)
+				snd_soc_dapm_disable_pin(dapm, "LDO2");
+			snd_soc_dapm_disable_pin(dapm, "Mic Det Power");
+			snd_soc_dapm_sync(dapm);
+		} else {
+			if (rt5645->pdata.jd_mode == 0)
+				regmap_update_bits(rt5645->regmap,
 						RT5645_PWR_MIXER,
 						RT5645_PWR_LDO2, 0);
-				regmap_update_bits(rt5645->regmap,
+			regmap_update_bits(rt5645->regmap,
 					RT5645_PWR_VOL, RT5645_PWR_MIC_DET, 0);
-			}
 		}
 	}