| Message ID | 1436856687-31550-2-git-send-email-drinkcat@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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>
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?
> -----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.
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.
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.
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.
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!
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.
> -----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.
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.
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); - } } }
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(-)