[PATCH v2 0/3] ASoC: rt5645: Remove codec dependency in workqueue handler

[PATCH v2 0/3] ASoC: rt5645: Remove codec dependency in workqueue handler

[Nicolas Boichat]

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

-- 
2.4.3.573.g4eafbef

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

[Nicolas Boichat]

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);
-			}
 		}
 	}
 
-- 
2.4.3.573.g4eafbef

[PATCH v2 2/3] ASoC: rt5645: Remove irq_jack_detection function

[Nicolas Boichat]

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)
-- 
2.4.3.573.g4eafbef

[PATCH v2 3/3] ASoC: rt5645: Remove codec dependency in workqueue handler

[Nicolas Boichat]

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;
-- 
2.4.3.573.g4eafbef

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

[Bard Liao]

> -----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>

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

[Mark Brown]

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

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?

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

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

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

[Bard Liao]

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

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

[Mark Brown]

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

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.

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

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

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

[Nicolas Boichat]

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.

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

[Mark Brown]

[-- 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 --]

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

[Nicolas Boichat]

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!

Applied "ASoC: rt5645: Remove irq_jack_detection function" to the asoc tree

[Mark Brown]

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)
-- 
2.1.4

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

[Mark Brown]

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

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.

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

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

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

[Bard Liao]

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

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

[Nicolas Boichat]

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.