[alsa-devel] [PATCH v2 0/3] ASoC: rt5645: Remove codec dependency in workqueue handler
Same issue as https://patchwork.kernel.org/patch/6694351/, but reworked fix.
The first 2 patches are preparatory work for the 3rd one, which fixes occasional kernel panics on boot.
Nicolas Boichat (3): ASoC: rt5645: Simplify rt5645_enable_push_button_irq ASoC: rt5645: Remove irq_jack_detection function ASoC: rt5645: Remove codec dependency in workqueue handler
sound/soc/codecs/rt5645.c | 185 +++++++++++++++++++++++----------------------- sound/soc/codecs/rt5645.h | 1 + 2 files changed, 95 insertions(+), 91 deletions(-)
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(-)
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); - } } }
-----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 Lpower");
snd_soc_dapm_force_enable_pin(dapm, "ADC Rpower");
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 Lpower");
snd_soc_dapm_force_enable_pin(dapm, "ADC Rpower");
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.
irq_jack_detection is only called from rt5645_jack_detect_work: remove the function to simplify the code.
Signed-off-by: Nicolas Boichat drinkcat@chromium.org --- sound/soc/codecs/rt5645.c | 53 ++++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 31 deletions(-)
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index 45651f4..ef6d3ad 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -2877,7 +2877,18 @@ static int rt5645_jack_detect(struct snd_soc_codec *codec, int jack_insert) return rt5645->jack_type; }
-static int rt5645_irq_detection(struct rt5645_priv *rt5645); +static int rt5645_button_detect(struct snd_soc_codec *codec) +{ + int btn_type, val; + + val = snd_soc_read(codec, RT5650_4BTN_IL_CMD1); + pr_debug("val=0x%x\n", val); + btn_type = val & 0xfff0; + snd_soc_write(codec, RT5650_4BTN_IL_CMD1, val); + + return btn_type; +} + static irqreturn_t rt5645_irq(int irq, void *data);
int rt5645_set_jack_detect(struct snd_soc_codec *codec, @@ -2908,34 +2919,6 @@ static void rt5645_jack_detect_work(struct work_struct *work) { struct rt5645_priv *rt5645 = container_of(work, struct rt5645_priv, jack_detect_work.work); - - rt5645_irq_detection(rt5645); -} - -static irqreturn_t rt5645_irq(int irq, void *data) -{ - struct rt5645_priv *rt5645 = data; - - queue_delayed_work(system_power_efficient_wq, - &rt5645->jack_detect_work, msecs_to_jiffies(250)); - - return IRQ_HANDLED; -} - -static int rt5645_button_detect(struct snd_soc_codec *codec) -{ - int btn_type, val; - - val = snd_soc_read(codec, RT5650_4BTN_IL_CMD1); - pr_debug("val=0x%x\n", val); - btn_type = val & 0xfff0; - snd_soc_write(codec, RT5650_4BTN_IL_CMD1, val); - - return btn_type; -} - -static int rt5645_irq_detection(struct rt5645_priv *rt5645) -{ int val, btn_type, gpio_state = 0, report = 0;
switch (rt5645->pdata.jd_mode) { @@ -2950,7 +2933,7 @@ static int rt5645_irq_detection(struct rt5645_priv *rt5645) report, SND_JACK_HEADPHONE); snd_soc_jack_report(rt5645->mic_jack, report, SND_JACK_MICROPHONE); - return report; + return; case 1: /* 2 port */ val = snd_soc_read(rt5645->codec, RT5645_A_JD_CTRL1) & 0x0070; break; @@ -3032,8 +3015,16 @@ static int rt5645_irq_detection(struct rt5645_priv *rt5645) snd_soc_jack_report(rt5645->btn_jack, report, SND_JACK_BTN_0 | SND_JACK_BTN_1 | SND_JACK_BTN_2 | SND_JACK_BTN_3); +}
- return report; +static irqreturn_t rt5645_irq(int irq, void *data) +{ + struct rt5645_priv *rt5645 = data; + + queue_delayed_work(system_power_efficient_wq, + &rt5645->jack_detect_work, msecs_to_jiffies(250)); + + return IRQ_HANDLED; }
static int rt5645_probe(struct snd_soc_codec *codec)
The patch
ASoC: rt5645: Remove irq_jack_detection function
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From f312bc59d21bed7593199a1921468868150954fa Mon Sep 17 00:00:00 2001
From: Nicolas Boichat drinkcat@chromium.org Date: Tue, 14 Jul 2015 14:51:26 +0800 Subject: [PATCH] ASoC: rt5645: Remove irq_jack_detection function
irq_jack_detection is only called from rt5645_jack_detect_work: remove the function to simplify the code.
Signed-off-by: Nicolas Boichat drinkcat@chromium.org Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/rt5645.c | 53 ++++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 31 deletions(-)
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index 9dfa431b337d..8693a25830d3 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -2874,7 +2874,18 @@ static int rt5645_jack_detect(struct snd_soc_codec *codec, int jack_insert) return rt5645->jack_type; }
-static int rt5645_irq_detection(struct rt5645_priv *rt5645); +static int rt5645_button_detect(struct snd_soc_codec *codec) +{ + int btn_type, val; + + val = snd_soc_read(codec, RT5650_4BTN_IL_CMD1); + pr_debug("val=0x%x\n", val); + btn_type = val & 0xfff0; + snd_soc_write(codec, RT5650_4BTN_IL_CMD1, val); + + return btn_type; +} + static irqreturn_t rt5645_irq(int irq, void *data);
int rt5645_set_jack_detect(struct snd_soc_codec *codec, @@ -2905,34 +2916,6 @@ static void rt5645_jack_detect_work(struct work_struct *work) { struct rt5645_priv *rt5645 = container_of(work, struct rt5645_priv, jack_detect_work.work); - - rt5645_irq_detection(rt5645); -} - -static irqreturn_t rt5645_irq(int irq, void *data) -{ - struct rt5645_priv *rt5645 = data; - - queue_delayed_work(system_power_efficient_wq, - &rt5645->jack_detect_work, msecs_to_jiffies(250)); - - return IRQ_HANDLED; -} - -static int rt5645_button_detect(struct snd_soc_codec *codec) -{ - int btn_type, val; - - val = snd_soc_read(codec, RT5650_4BTN_IL_CMD1); - pr_debug("val=0x%x\n", val); - btn_type = val & 0xfff0; - snd_soc_write(codec, RT5650_4BTN_IL_CMD1, val); - - return btn_type; -} - -static int rt5645_irq_detection(struct rt5645_priv *rt5645) -{ int val, btn_type, gpio_state = 0, report = 0;
switch (rt5645->pdata.jd_mode) { @@ -2947,7 +2930,7 @@ static int rt5645_irq_detection(struct rt5645_priv *rt5645) report, SND_JACK_HEADPHONE); snd_soc_jack_report(rt5645->mic_jack, report, SND_JACK_MICROPHONE); - return report; + return; case 1: /* 2 port */ val = snd_soc_read(rt5645->codec, RT5645_A_JD_CTRL1) & 0x0070; break; @@ -3029,8 +3012,16 @@ static int rt5645_irq_detection(struct rt5645_priv *rt5645) snd_soc_jack_report(rt5645->btn_jack, report, SND_JACK_BTN_0 | SND_JACK_BTN_1 | SND_JACK_BTN_2 | SND_JACK_BTN_3); +}
- return report; +static irqreturn_t rt5645_irq(int irq, void *data) +{ + struct rt5645_priv *rt5645 = data; + + queue_delayed_work(system_power_efficient_wq, + &rt5645->jack_detect_work, msecs_to_jiffies(250)); + + return IRQ_HANDLED; }
static int rt5645_probe(struct snd_soc_codec *codec)
The workqueue handler rt5645_jack_detect_work (and functions it calls) uses rt5645->codec, which may be uninitialized when the workqueue is first executed.
Actually, rt5645->codec is never required, and regmap functions can be used instead of snd_soc functions.
Signed-off-by: Nicolas Boichat drinkcat@chromium.org --- sound/soc/codecs/rt5645.c | 87 ++++++++++++++++++++++++++--------------------- sound/soc/codecs/rt5645.h | 1 + 2 files changed, 49 insertions(+), 39 deletions(-)
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index ef6d3ad..4f62988 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -2759,14 +2759,14 @@ static int rt5650_calibration(struct rt5645_priv *rt5645) return ret; }
-static void rt5645_enable_push_button_irq(struct snd_soc_codec *codec, - bool enable) +static void rt5645_enable_push_button_irq(struct rt5645_priv *rt5645, + struct snd_soc_dapm_context *dapm, + bool enable) { - struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); - struct rt5645_priv *rt5645 = snd_soc_codec_get_drvdata(codec); + unsigned int val;
if (enable) { - if (codec->component.card->instantiated) { + if (dapm) { 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); @@ -2776,18 +2776,18 @@ static void rt5645_enable_push_button_irq(struct snd_soc_codec *codec, RT5645_PWR_ADC_L_BIT | RT5645_PWR_ADC_R_BIT); }
- snd_soc_update_bits(codec, - RT5645_INT_IRQ_ST, 0x8, 0x8); - snd_soc_update_bits(codec, - RT5650_4BTN_IL_CMD2, 0x8000, 0x8000); - snd_soc_read(codec, RT5650_4BTN_IL_CMD1); - pr_debug("%s read %x = %x\n", __func__, RT5650_4BTN_IL_CMD1, - snd_soc_read(codec, RT5650_4BTN_IL_CMD1)); + regmap_update_bits(rt5645->regmap, RT5645_INT_IRQ_ST, 0x8, 0x8); + regmap_update_bits(rt5645->regmap, RT5650_4BTN_IL_CMD2, + 0x8000, 0x8000); + regmap_read(rt5645->regmap, RT5650_4BTN_IL_CMD1, &val); + dev_dbg(rt5645->dev, "%s read %x = %x\n", __func__, + RT5650_4BTN_IL_CMD1, val); } else { - snd_soc_update_bits(codec, RT5650_4BTN_IL_CMD2, 0x8000, 0x0); - snd_soc_update_bits(codec, RT5645_INT_IRQ_ST, 0x8, 0x0); + regmap_update_bits(rt5645->regmap, RT5650_4BTN_IL_CMD2, + 0x8000, 0x0); + regmap_update_bits(rt5645->regmap, RT5645_INT_IRQ_ST, 0x8, 0x0);
- if (codec->component.card->instantiated) { + if (dapm) { snd_soc_dapm_disable_pin(dapm, "ADC L power"); snd_soc_dapm_disable_pin(dapm, "ADC R power"); snd_soc_dapm_sync(dapm); @@ -2799,16 +2799,19 @@ static void rt5645_enable_push_button_irq(struct snd_soc_codec *codec, } }
-static int rt5645_jack_detect(struct snd_soc_codec *codec, int jack_insert) +static int rt5645_jack_detect(struct rt5645_priv *rt5645, int jack_insert) { - struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); - struct rt5645_priv *rt5645 = snd_soc_codec_get_drvdata(codec); unsigned int val; + struct snd_soc_dapm_context *dapm = NULL; + + if (rt5645->codec && rt5645->codec->component.card->instantiated) { + dapm = snd_soc_codec_get_dapm(rt5645->codec); + }
if (jack_insert) { regmap_write(rt5645->regmap, RT5645_CHARGE_PUMP, 0x0006);
- if (codec->component.card->instantiated) { + if (dapm) { /* for jack type detect */ snd_soc_dapm_force_enable_pin(dapm, "LDO2"); snd_soc_dapm_force_enable_pin(dapm, "Mic Det Power"); @@ -2836,15 +2839,16 @@ static int rt5645_jack_detect(struct snd_soc_codec *codec, int jack_insert) msleep(450); regmap_read(rt5645->regmap, RT5645_IN1_CTRL3, &val); val &= 0x7; - dev_dbg(codec->dev, "val = %d\n", val); + dev_dbg(rt5645->dev, "val = %d\n", val);
if (val == 1 || val == 2) { rt5645->jack_type = SND_JACK_HEADSET; if (rt5645->en_button_func) { - rt5645_enable_push_button_irq(codec, true); + rt5645_enable_push_button_irq(rt5645, dapm, + true); } } else { - if (codec->component.card->instantiated) { + if (dapm) { snd_soc_dapm_disable_pin(dapm, "Mic Det Power"); snd_soc_dapm_sync(dapm); } else @@ -2857,9 +2861,9 @@ static int rt5645_jack_detect(struct snd_soc_codec *codec, int jack_insert) rt5645->jack_type = 0;
if (rt5645->en_button_func) - rt5645_enable_push_button_irq(codec, false); + rt5645_enable_push_button_irq(rt5645, dapm, false);
- if (codec->component.card->instantiated) { + if (dapm) { if (rt5645->pdata.jd_mode == 0) snd_soc_dapm_disable_pin(dapm, "LDO2"); snd_soc_dapm_disable_pin(dapm, "Mic Det Power"); @@ -2877,14 +2881,14 @@ static int rt5645_jack_detect(struct snd_soc_codec *codec, int jack_insert) return rt5645->jack_type; }
-static int rt5645_button_detect(struct snd_soc_codec *codec) +static int rt5645_button_detect(struct rt5645_priv *rt5645) { int btn_type, val;
- val = snd_soc_read(codec, RT5650_4BTN_IL_CMD1); - pr_debug("val=0x%x\n", val); + regmap_read(rt5645->regmap, RT5650_4BTN_IL_CMD1, &val); + dev_dbg(rt5645->dev, "val=0x%x\n", val); btn_type = val & 0xfff0; - snd_soc_write(codec, RT5650_4BTN_IL_CMD1, val); + regmap_write(rt5645->regmap, RT5650_4BTN_IL_CMD1, val);
return btn_type; } @@ -2925,9 +2929,9 @@ static void rt5645_jack_detect_work(struct work_struct *work) case 0: /* Not using rt5645 JD */ if (rt5645->gpiod_hp_det) { gpio_state = gpiod_get_value(rt5645->gpiod_hp_det); - dev_dbg(rt5645->codec->dev, "gpio_state = %d\n", + dev_dbg(rt5645->dev, "gpio_state = %d\n", gpio_state); - report = rt5645_jack_detect(rt5645->codec, gpio_state); + report = rt5645_jack_detect(rt5645, gpio_state); } snd_soc_jack_report(rt5645->hp_jack, report, SND_JACK_HEADPHONE); @@ -2935,10 +2939,12 @@ static void rt5645_jack_detect_work(struct work_struct *work) report, SND_JACK_MICROPHONE); return; case 1: /* 2 port */ - val = snd_soc_read(rt5645->codec, RT5645_A_JD_CTRL1) & 0x0070; + regmap_read(rt5645->regmap, RT5645_A_JD_CTRL1, &val); + val &= 0x0070; break; default: /* 1 port */ - val = snd_soc_read(rt5645->codec, RT5645_A_JD_CTRL1) & 0x0020; + regmap_read(rt5645->regmap, RT5645_A_JD_CTRL1, &val); + val &= 0x0020; break;
} @@ -2948,15 +2954,16 @@ static void rt5645_jack_detect_work(struct work_struct *work) case 0x30: /* 2 port */ case 0x0: /* 1 port or 2 port */ if (rt5645->jack_type == 0) { - report = rt5645_jack_detect(rt5645->codec, 1); + report = rt5645_jack_detect(rt5645, 1); /* for push button and jack out */ break; } btn_type = 0; - if (snd_soc_read(rt5645->codec, RT5645_INT_IRQ_ST) & 0x4) { + regmap_read(rt5645->regmap, RT5645_INT_IRQ_ST, &val); + if (val & 0x4) { /* button pressed */ report = SND_JACK_HEADSET; - btn_type = rt5645_button_detect(rt5645->codec); + btn_type = rt5645_button_detect(rt5645); /* rt5650 can report three kinds of button behavior, one click, double click and hold. However, currently we will report button pressed/released @@ -2986,7 +2993,7 @@ static void rt5645_jack_detect_work(struct work_struct *work) case 0x0000: /* unpressed */ break; default: - dev_err(rt5645->codec->dev, + dev_err(rt5645->dev, "Unexpected button code 0x%04x\n", btn_type); break; @@ -3001,9 +3008,9 @@ static void rt5645_jack_detect_work(struct work_struct *work) case 0x10: /* 2 port */ case 0x20: /* 1 port */ report = 0; - snd_soc_update_bits(rt5645->codec, - RT5645_INT_IRQ_ST, 0x1, 0x0); - rt5645_jack_detect(rt5645->codec, 0); + regmap_update_bits(rt5645->regmap, + RT5645_INT_IRQ_ST, 0x1, 0x0); + rt5645_jack_detect(rt5645, 0); break; default: break; @@ -3252,6 +3259,8 @@ static int rt5645_i2c_probe(struct i2c_client *i2c, rt5645->i2c = i2c; i2c_set_clientdata(i2c, rt5645);
+ rt5645->dev = &i2c->dev; + if (pdata) rt5645->pdata = *pdata; else if (dmi_check_system(dmi_platform_intel_braswell)) diff --git a/sound/soc/codecs/rt5645.h b/sound/soc/codecs/rt5645.h index 0353a6a..8517ac6 100644 --- a/sound/soc/codecs/rt5645.h +++ b/sound/soc/codecs/rt5645.h @@ -2179,6 +2179,7 @@ int rt5645_sel_asrc_clk_src(struct snd_soc_codec *codec,
struct rt5645_priv { struct snd_soc_codec *codec; + struct device *dev; struct rt5645_platform_data pdata; struct regmap *regmap; struct i2c_client *i2c;
participants (3)
-
Bard Liao -
Mark Brown -
Nicolas Boichat