ASoC: cs35l41: Fix broken shared boost activation

Enabling the active/passive shared boosts requires setting SYNC_EN, but
*not* before receiving the PLL Lock signal.

Due to improper error handling, it was not obvious that waiting for the
completion operation times out and, consequently, the shared boost is
never activated.

Further investigations revealed the signal is triggered while
snd_pcm_start() is executed, right after receiving the
SNDRV_PCM_TRIGGER_START command, which happens long after the
SND_SOC_DAPM_PRE_PMU event handler is invoked as part of
snd_pcm_prepare(). That is where cs35l41_global_enable() is called
from.

Increasing the wait duration doesn't help, as it only causes an
unnecessary delay in the invocation of snd_pcm_start(). Moving the wait
and the subsequent regmap operations to the SNDRV_PCM_TRIGGER_START
callback is not a solution either, since they would be executed in an
IRQ-off atomic context.

Solve the issue by setting the SYNC_EN bit in PWR_CTRL3 register right
after receiving the PLL Lock interrupt.

Additionally, drop the unnecessary writes to PWR_CTRL1 register, part of
the original mdsync_up_seq, which would have toggled GLOBAL_EN with
unwanted consequences on PLL locking behavior.

Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
Signed-off-by: Cristian Ciocaltea <[e-mail]>
Reviewed-by: David Rhodes <[e-mail]>
Reviewed-by: Takashi Iwai <[e-mail]>
Link: https://lore.kernel.org/r/[e-mail]
Signed-off-by: Mark Brown <[e-mail]>

include/sound/cs35l41.h

···
 #define __CS35L41_H
 
 #include <linux/regmap.h>
-#include <linux/completion.h>
 #include <linux/firmware/cirrus/cs_dsp.h>
 
 #define CS35L41_FIRSTREG		0x00000000
···
 int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
 		       struct cs35l41_hw_cfg *hw_cfg);
 bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type);
+int cs35l41_mdsync_up(struct regmap *regmap);
 int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type,
-			  int enable, struct completion *pll_lock, bool firmware_running);
+			  int enable, bool firmware_running);
 
 #endif /* __CS35L41_H */

sound/pci/hda/cs35l41_hda.c

···
 
 	dev_dbg(dev, "Play (Complete)\n");
 
-	cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL,
+	cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1,
 			      cs35l41->firmware_running);
 	if (cs35l41->firmware_running) {
 		regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp,
···
 	dev_dbg(dev, "Pause (Start)\n");
 
 	regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute));
-	cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, NULL,
+	cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0,
 			      cs35l41->firmware_running);
 }
 

sound/soc/codecs/cs35l41-lib.c

···
 }
 EXPORT_SYMBOL_GPL(cs35l41_safe_reset);
 
+/*
+ * Enabling the CS35L41_SHD_BOOST_ACTV and CS35L41_SHD_BOOST_PASS shared boosts
+ * does also require a call to cs35l41_mdsync_up(), but not before getting the
+ * PLL Lock signal.
+ *
+ * PLL Lock seems to be triggered soon after snd_pcm_start() is executed and
+ * SNDRV_PCM_TRIGGER_START command is processed, which happens (long) after the
+ * SND_SOC_DAPM_PRE_PMU event handler is invoked as part of snd_pcm_prepare().
+ *
+ * This event handler is where cs35l41_global_enable() is normally called from,
+ * but waiting for PLL Lock here will time out. Increasing the wait duration
+ * will not help, as the only consequence of it would be to add an unnecessary
+ * delay in the invocation of snd_pcm_start().
+ *
+ * Trying to move the wait in the SNDRV_PCM_TRIGGER_START callback is not a
+ * solution either, as the trigger is executed in an IRQ-off atomic context.
+ *
+ * The current approach is to invoke cs35l41_mdsync_up() right after receiving
+ * the PLL Lock interrupt, in the IRQ handler.
+ */
 int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type,
-			  int enable, struct completion *pll_lock, bool firmware_running)
+			  int enable, bool firmware_running)
 {
 	int ret;
 	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3, int_status, pup_pdn_mask;
···
 		{CS35L41_PWR_CTRL3,		0},
 		{CS35L41_GPIO_PAD_CONTROL,	0},
 		{CS35L41_PWR_CTRL1,		0, 3000},
-	};
-	struct reg_sequence cs35l41_mdsync_up_seq[] = {
-		{CS35L41_PWR_CTRL3,	0},
-		{CS35L41_PWR_CTRL1,	0x00000000, 3000},
-		{CS35L41_PWR_CTRL1,	0x00000001, 3000},
 	};
 
 	pup_pdn_mask = enable ? CS35L41_PUP_DONE_MASK : CS35L41_PDN_DONE_MASK;
···
 		cs35l41_mdsync_down_seq[0].def = pwr_ctrl3;
 		cs35l41_mdsync_down_seq[1].def = pad_control;
 		cs35l41_mdsync_down_seq[2].def = pwr_ctrl1;
+
 		ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_down_seq,
 					     ARRAY_SIZE(cs35l41_mdsync_down_seq));
-		if (ret || !enable)
-			break;
-
-		if (!pll_lock)
-			return -EINVAL;
-
-		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
-		if (ret == 0) {
-			dev_err(dev, "Timed out waiting for pll_lock\n");
-			return -ETIMEDOUT;
-		}
-
-		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
-		pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
-		cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
-		ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
-					     ARRAY_SIZE(cs35l41_mdsync_up_seq));
-		if (ret)
+		/* Activation to be completed later via cs35l41_mdsync_up() */
+		if (ret || enable)
 			return ret;
 
 		ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1,
···
 		if (ret)
 			dev_err(dev, "Enable(%d) failed: %d\n", enable, ret);
 
-		// Clear PUP/PDN status
+		/* Clear PUP/PDN status */
 		regmap_write(regmap, CS35L41_IRQ1_STATUS1, pup_pdn_mask);
 		break;
 	case CS35L41_INT_BOOST:
···
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cs35l41_global_enable);
+
+/*
+ * To be called after receiving the IRQ Lock interrupt, in order to complete
+ * any shared boost activation initiated by cs35l41_global_enable().
+ */
+int cs35l41_mdsync_up(struct regmap *regmap)
+{
+	return regmap_update_bits(regmap, CS35L41_PWR_CTRL3,
+				  CS35L41_SYNC_EN_MASK, CS35L41_SYNC_EN_MASK);
+}
+EXPORT_SYMBOL_GPL(cs35l41_mdsync_up);
 
 int cs35l41_gpio_config(struct regmap *regmap, struct cs35l41_hw_cfg *hw_cfg)
 {

sound/soc/codecs/cs35l41.c

···
 
 	if (status[2] & CS35L41_PLL_LOCK) {
 		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS3, CS35L41_PLL_LOCK);
-		complete(&cs35l41->pll_lock);
+
+		if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_ACTV ||
+		    cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS) {
+			ret = cs35l41_mdsync_up(cs35l41->regmap);
+			if (ret)
+				dev_err(cs35l41->dev, "MDSYNC-up failed: %d\n", ret);
+			else
+				dev_dbg(cs35l41->dev, "MDSYNC-up done\n");
+
+			dev_dbg(cs35l41->dev, "PUP-done status: %d\n",
+				!!(status[0] & CS35L41_PUP_DONE_MASK));
+		}
+
 		ret = IRQ_HANDLED;
 	}
 
···
 						ARRAY_SIZE(cs35l41_pup_patch));
 
 		ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
-					    1, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
+					    1, cs35l41->dsp.cs_dsp.running);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
 		ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
-					    0, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
+					    0, cs35l41->dsp.cs_dsp.running);
 
 		regmap_multi_reg_write_bypassed(cs35l41->regmap,
 						cs35l41_pdn_patch,
···
 static int cs35l41_pcm_startup(struct snd_pcm_substream *substream,
 			       struct snd_soc_dai *dai)
 {
-	struct cs35l41_private *cs35l41 = snd_soc_component_get_drvdata(dai->component);
-
-	reinit_completion(&cs35l41->pll_lock);
-
 	if (substream->runtime)
 		return snd_pcm_hw_constraint_list(substream->runtime, 0,
 						  SNDRV_PCM_HW_PARAM_RATE,
···
 	    cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_ACTV)
 		regmap_update_bits(cs35l41->regmap, CS35L41_IRQ1_MASK3, CS35L41_INT3_PLL_LOCK_MASK,
 				   0 << CS35L41_INT3_PLL_LOCK_SHIFT);
-
-	init_completion(&cs35l41->pll_lock);
 
 	ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL, cs35l41_irq,
 					IRQF_ONESHOT | IRQF_SHARED | irq_pol,

sound/soc/codecs/cs35l41.h

···
 	int irq;
 	/* GPIO for /RST */
 	struct gpio_desc *reset_gpio;
-	struct completion pll_lock;
 };
 
 int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *hw_cfg);