[PATCH v5 0/4] Add CS35L41 shared boost feature

[PATCH v5 0/4] Add CS35L41 shared boost feature

[Lucas Tanure]

Valve's Steam Deck uses CS35L41 in shared boost mode, where both speakers
share the boost circuit.
Add this support in the shared lib, but for now, shared boost is not
supported in HDA systems as would require BIOS changes.

Based on David Rhodes shared boost patches.

Also, fix boost config overwriting in IRQ found in the review and do a
small refactor of the code.

Changes from V4:
 - Fix Document subject

Changes from V3:
 - Fix wrong code sent
 - Fix ISO C90 mixed declarations and code 

Changes from V2:
 - Drop External boost without VSPK Documentation
 - Move Shared boost to use values 2 and 3
 - Revert back to reg_sequence but reading the value first and only update
the necessary bits
 - Fix bug found by Intel kernel Test Robot

Changes from V1:
 - Fix Documentation patch subject
 - New patch for External boost without VSPK Documentation
 - New patch to fix boost IRQ overwriting issue
 - New patch to refactor IRQ release error code
 - reinit_completion on pcm_startup
 - fix DRE switch overwriting
 - return IRQ_HANDLED in PLL_LOCK case

Lucas Tanure (4):
  ASoC: cs35l41: Only disable internal boost
  ASoC: cs35l41: Refactor error release code
  ALSA: cs35l41: Add shared boost feature
  ASoC: dt-bindings: cirrus,cs35l41: Document CS35l41 shared boost

 .../bindings/sound/cirrus,cs35l41.yaml        |  10 +-
 include/sound/cs35l41.h                       |  13 +-
 sound/pci/hda/cs35l41_hda.c                   |   6 +-
 sound/soc/codecs/cs35l41-lib.c                |  73 +++++++++-
 sound/soc/codecs/cs35l41.c                    | 125 +++++++++---------
 sound/soc/codecs/cs35l41.h                    |   1 +
 6 files changed, 157 insertions(+), 71 deletions(-)

-- 
2.39.1

[PATCH v5 1/4] ASoC: cs35l41: Only disable internal boost

[Lucas Tanure]

In error situations, only the internal boost case should be disabled and
re-enabled.
Also, for other boost cases re-enabling the boost to the default internal
boost config is incorrect.

Fixes: 6450ef559056 ("ASoC: cs35l41: CS35L41 Boosted Smart Amplifier")
Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l41.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index c223d83e02cf..f2b5032daa6a 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -356,6 +356,19 @@ static const struct snd_kcontrol_new cs35l41_aud_controls[] = {
 	WM_ADSP_FW_CONTROL("DSP1", 0),
 };
 
+static void cs35l41_boost_enable(struct cs35l41_private *cs35l41, unsigned int enable)
+{
+	switch (cs35l41->hw_cfg.bst_type) {
+	case CS35L41_INT_BOOST:
+		enable = enable ? CS35L41_BST_EN_DEFAULT : CS35L41_BST_DIS_FET_OFF;
+		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2, CS35L41_BST_EN_MASK,
+				enable << CS35L41_BST_EN_SHIFT);
+		break;
+	default:
+		break;
+	}
+}
+
 static irqreturn_t cs35l41_irq(int irq, void *data)
 {
 	struct cs35l41_private *cs35l41 = data;
@@ -431,8 +444,7 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 
 	if (status[0] & CS35L41_BST_OVP_ERR) {
 		dev_crit_ratelimited(cs35l41->dev, "VBST Over Voltage error\n");
-		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-				   CS35L41_BST_EN_MASK, 0);
+		cs35l41_boost_enable(cs35l41, 0);
 		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
 			     CS35L41_BST_OVP_ERR);
 		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
@@ -441,16 +453,13 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 				   CS35L41_BST_OVP_ERR_RLS);
 		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
 				   CS35L41_BST_OVP_ERR_RLS, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-				   CS35L41_BST_EN_MASK,
-				   CS35L41_BST_EN_DEFAULT << CS35L41_BST_EN_SHIFT);
+		cs35l41_boost_enable(cs35l41, 1);
 		ret = IRQ_HANDLED;
 	}
 
 	if (status[0] & CS35L41_BST_DCM_UVP_ERR) {
 		dev_crit_ratelimited(cs35l41->dev, "DCM VBST Under Voltage Error\n");
-		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-				   CS35L41_BST_EN_MASK, 0);
+		cs35l41_boost_enable(cs35l41, 0);
 		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
 			     CS35L41_BST_DCM_UVP_ERR);
 		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
@@ -459,16 +468,13 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 				   CS35L41_BST_UVP_ERR_RLS);
 		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
 				   CS35L41_BST_UVP_ERR_RLS, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-				   CS35L41_BST_EN_MASK,
-				   CS35L41_BST_EN_DEFAULT << CS35L41_BST_EN_SHIFT);
+		cs35l41_boost_enable(cs35l41, 1);
 		ret = IRQ_HANDLED;
 	}
 
 	if (status[0] & CS35L41_BST_SHORT_ERR) {
 		dev_crit_ratelimited(cs35l41->dev, "LBST error: powering off!\n");
-		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-				   CS35L41_BST_EN_MASK, 0);
+		cs35l41_boost_enable(cs35l41, 0);
 		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
 			     CS35L41_BST_SHORT_ERR);
 		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
@@ -477,9 +483,7 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 				   CS35L41_BST_SHORT_ERR_RLS);
 		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
 				   CS35L41_BST_SHORT_ERR_RLS, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-				   CS35L41_BST_EN_MASK,
-				   CS35L41_BST_EN_DEFAULT << CS35L41_BST_EN_SHIFT);
+		cs35l41_boost_enable(cs35l41, 1);
 		ret = IRQ_HANDLED;
 	}
 
-- 
2.39.1

[PATCH v5 2/4] ASoC: cs35l41: Refactor error release code

[Lucas Tanure]

Add cs35l41_error_release function to handle error release sequences.

Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l41.c | 64 ++++++++++----------------------------
 1 file changed, 16 insertions(+), 48 deletions(-)

diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index f2b5032daa6a..c006364e5335 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -369,6 +369,16 @@ static void cs35l41_boost_enable(struct cs35l41_private *cs35l41, unsigned int e
 	}
 }
 
+
+static void cs35l41_error_release(struct cs35l41_private *cs35l41, unsigned int irq_err_bit,
+				  unsigned int rel_err_bit)
+{
+	regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1, irq_err_bit);
+	regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
+	regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, rel_err_bit, rel_err_bit);
+	regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, rel_err_bit, 0);
+}
+
 static irqreturn_t cs35l41_irq(int irq, void *data)
 {
 	struct cs35l41_private *cs35l41 = data;
@@ -405,54 +415,26 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 	 */
 	if (status[0] & CS35L41_AMP_SHORT_ERR) {
 		dev_crit_ratelimited(cs35l41->dev, "Amp short error\n");
-		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
-			     CS35L41_AMP_SHORT_ERR);
-		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_AMP_SHORT_ERR_RLS,
-				   CS35L41_AMP_SHORT_ERR_RLS);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_AMP_SHORT_ERR_RLS, 0);
+		cs35l41_error_release(cs35l41, CS35L41_AMP_SHORT_ERR, CS35L41_AMP_SHORT_ERR_RLS);
 		ret = IRQ_HANDLED;
 	}
 
 	if (status[0] & CS35L41_TEMP_WARN) {
 		dev_crit_ratelimited(cs35l41->dev, "Over temperature warning\n");
-		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
-			     CS35L41_TEMP_WARN);
-		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_TEMP_WARN_ERR_RLS,
-				   CS35L41_TEMP_WARN_ERR_RLS);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_TEMP_WARN_ERR_RLS, 0);
+		cs35l41_error_release(cs35l41, CS35L41_TEMP_WARN, CS35L41_TEMP_WARN_ERR_RLS);
 		ret = IRQ_HANDLED;
 	}
 
 	if (status[0] & CS35L41_TEMP_ERR) {
 		dev_crit_ratelimited(cs35l41->dev, "Over temperature error\n");
-		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
-			     CS35L41_TEMP_ERR);
-		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_TEMP_ERR_RLS,
-				   CS35L41_TEMP_ERR_RLS);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_TEMP_ERR_RLS, 0);
+		cs35l41_error_release(cs35l41, CS35L41_TEMP_ERR, CS35L41_TEMP_ERR_RLS);
 		ret = IRQ_HANDLED;
 	}
 
 	if (status[0] & CS35L41_BST_OVP_ERR) {
 		dev_crit_ratelimited(cs35l41->dev, "VBST Over Voltage error\n");
 		cs35l41_boost_enable(cs35l41, 0);
-		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
-			     CS35L41_BST_OVP_ERR);
-		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_BST_OVP_ERR_RLS,
-				   CS35L41_BST_OVP_ERR_RLS);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_BST_OVP_ERR_RLS, 0);
+		cs35l41_error_release(cs35l41, CS35L41_BST_OVP_ERR, CS35L41_BST_OVP_ERR_RLS);
 		cs35l41_boost_enable(cs35l41, 1);
 		ret = IRQ_HANDLED;
 	}
@@ -460,14 +442,7 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 	if (status[0] & CS35L41_BST_DCM_UVP_ERR) {
 		dev_crit_ratelimited(cs35l41->dev, "DCM VBST Under Voltage Error\n");
 		cs35l41_boost_enable(cs35l41, 0);
-		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
-			     CS35L41_BST_DCM_UVP_ERR);
-		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_BST_UVP_ERR_RLS,
-				   CS35L41_BST_UVP_ERR_RLS);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_BST_UVP_ERR_RLS, 0);
+		cs35l41_error_release(cs35l41, CS35L41_BST_DCM_UVP_ERR, CS35L41_BST_UVP_ERR_RLS);
 		cs35l41_boost_enable(cs35l41, 1);
 		ret = IRQ_HANDLED;
 	}
@@ -475,14 +450,7 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 	if (status[0] & CS35L41_BST_SHORT_ERR) {
 		dev_crit_ratelimited(cs35l41->dev, "LBST error: powering off!\n");
 		cs35l41_boost_enable(cs35l41, 0);
-		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
-			     CS35L41_BST_SHORT_ERR);
-		regmap_write(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN, 0);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_BST_SHORT_ERR_RLS,
-				   CS35L41_BST_SHORT_ERR_RLS);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PROTECT_REL_ERR_IGN,
-				   CS35L41_BST_SHORT_ERR_RLS, 0);
+		cs35l41_error_release(cs35l41, CS35L41_BST_SHORT_ERR, CS35L41_BST_SHORT_ERR_RLS);
 		cs35l41_boost_enable(cs35l41, 1);
 		ret = IRQ_HANDLED;
 	}
-- 
2.39.1

[PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Lucas Tanure]

Shared boost allows two amplifiers to share a single boost circuit by
communicating on the MDSYNC bus.
The passive amplifier does not control the boost and receives data from
the active amplifier.

Shared Boost is not supported in HDA Systems.
Based on David Rhodes shared boost patches.

Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
---
 include/sound/cs35l41.h        | 13 +++++-
 sound/pci/hda/cs35l41_hda.c    |  6 +--
 sound/soc/codecs/cs35l41-lib.c | 73 +++++++++++++++++++++++++++++++++-
 sound/soc/codecs/cs35l41.c     | 27 ++++++++++++-
 sound/soc/codecs/cs35l41.h     |  1 +
 5 files changed, 113 insertions(+), 7 deletions(-)

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index 9ac5918269a5..7239d943942c 100644
--- a/include/sound/cs35l41.h
+++ b/include/sound/cs35l41.h
@@ -11,6 +11,7 @@
 #define __CS35L41_H
 
 #include <linux/regmap.h>
+#include <linux/completion.h>
 #include <linux/firmware/cirrus/cs_dsp.h>
 
 #define CS35L41_FIRSTREG		0x00000000
@@ -677,6 +678,7 @@
 
 #define CS35L36_PUP_DONE_IRQ_UNMASK	0x5F
 #define CS35L36_PUP_DONE_IRQ_MASK	0xBF
+#define CS35L41_SYNC_EN_MASK		BIT(8)
 
 #define CS35L41_AMP_SHORT_ERR		0x80000000
 #define CS35L41_BST_SHORT_ERR		0x0100
@@ -686,6 +688,7 @@
 #define CS35L41_BST_DCM_UVP_ERR		0x80
 #define CS35L41_OTP_BOOT_DONE		0x02
 #define CS35L41_PLL_UNLOCK		0x10
+#define CS35L41_PLL_LOCK		BIT(1)
 #define CS35L41_OTP_BOOT_ERR		0x80000000
 
 #define CS35L41_AMP_SHORT_ERR_RLS	0x02
@@ -705,6 +708,8 @@
 #define CS35L41_INT1_MASK_DEFAULT	0x7FFCFE3F
 #define CS35L41_INT1_UNMASK_PUP		0xFEFFFFFF
 #define CS35L41_INT1_UNMASK_PDN		0xFF7FFFFF
+#define CS35L41_INT3_PLL_LOCK_SHIFT	1
+#define CS35L41_INT3_PLL_LOCK_MASK	BIT(CS35L41_INT3_PLL_LOCK_SHIFT)
 
 #define CS35L41_GPIO_DIR_MASK		0x80000000
 #define CS35L41_GPIO_DIR_SHIFT		31
@@ -742,6 +747,11 @@
 enum cs35l41_boost_type {
 	CS35L41_INT_BOOST,
 	CS35L41_EXT_BOOST,
+	CS35L41_SHD_BOOST_ACTV,
+	CS35L41_SHD_BOOST_PASS,
+
+	// Not present in Binding Documentation, so no system should use this value.
+	// This value is only used in CLSA0100 Laptop
 	CS35L41_EXT_BOOST_NO_VSPK_SWITCH,
 };
 
@@ -891,6 +901,7 @@ int cs35l41_exit_hibernate(struct device *dev, struct regmap *regmap);
 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_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable);
+int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
+			  struct completion *pll_lock);
 
 #endif /* __CS35L41_H */
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index f7815ee24f83..38c0079ef303 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -515,13 +515,13 @@ static void cs35l41_hda_playback_hook(struct device *dev, int action)
 		break;
 	case HDA_GEN_PCM_ACT_PREPARE:
 		mutex_lock(&cs35l41->fw_mutex);
-		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 1);
+		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 1, NULL);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLEANUP:
 		mutex_lock(&cs35l41->fw_mutex);
 		regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute));
-		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 0);
+		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 0, NULL);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLOSE:
@@ -673,7 +673,7 @@ static int cs35l41_runtime_suspend(struct device *dev)
 	if (cs35l41->playback_started) {
 		regmap_multi_reg_write(cs35l41->regmap, cs35l41_hda_mute,
 				       ARRAY_SIZE(cs35l41_hda_mute));
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0);
+		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0, NULL);
 		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
 				   CS35L41_AMP_EN_MASK, 0 << CS35L41_AMP_EN_SHIFT);
 		if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index 04be71435491..4d6417ce70bb 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -1114,12 +1114,31 @@ static const struct reg_sequence cs35l41_reset_to_safe[] = {
 	{ 0x00000040,			0x00000033 },
 };
 
+static const struct reg_sequence cs35l41_actv_seq[] = {
+	/* SYNC_BST_CTL_RX_EN = 0; SYNC_BST_CTL_TX_EN = 1 */
+	{CS35L41_MDSYNC_EN,        0x00001000},
+	/* BST_CTL_SEL = CLASSH */
+	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},
+};
+
+static const struct reg_sequence cs35l41_pass_seq[] = {
+	/* SYNC_BST_CTL_RX_EN = 1; SYNC_BST_CTL_TX_EN = 0 */
+	{CS35L41_MDSYNC_EN,        0x00002000},
+	/* BST_EN = 0 */
+	{CS35L41_PWR_CTRL2,        0x00003300},
+	/* BST_CTL_SEL = MDSYNC */
+	{CS35L41_BSTCVRT_VCTRL2,    0x00000002},
+};
+
 int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
 		       struct cs35l41_hw_cfg *hw_cfg)
 {
 	int ret;
 
 	switch (hw_cfg->bst_type) {
+	case CS35L41_SHD_BOOST_ACTV:
+		regmap_multi_reg_write(regmap, cs35l41_actv_seq, ARRAY_SIZE(cs35l41_actv_seq));
+		fallthrough;
 	case CS35L41_INT_BOOST:
 		ret = cs35l41_boost_config(dev, regmap, hw_cfg->bst_ind,
 					   hw_cfg->bst_cap, hw_cfg->bst_ipk);
@@ -1138,6 +1157,10 @@ int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
 		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL2, CS35L41_BST_EN_MASK,
 					 CS35L41_BST_DIS_FET_OFF << CS35L41_BST_EN_SHIFT);
 		break;
+	case CS35L41_SHD_BOOST_PASS:
+		ret = regmap_multi_reg_write(regmap, cs35l41_pass_seq,
+					     ARRAY_SIZE(cs35l41_pass_seq));
+		break;
 	default:
 		dev_err(dev, "Boost type %d not supported\n", hw_cfg->bst_type);
 		ret = -EINVAL;
@@ -1165,11 +1188,59 @@ bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type)
 }
 EXPORT_SYMBOL_GPL(cs35l41_safe_reset);
 
-int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable)
+int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
+			  struct completion *pll_lock)
 {
 	int ret;
+	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3;
+	struct reg_sequence cs35l41_mdsync_down_seq[] = {
+		{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},
+	};
 
 	switch (b_type) {
+	case CS35L41_SHD_BOOST_ACTV:
+	case CS35L41_SHD_BOOST_PASS:
+		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
+		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
+
+		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
+		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;
+
+		gpio1_func = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
+		gpio1_func <<= CS35L41_GPIO1_CTRL_SHIFT;
+
+		pad_control &= ~CS35L41_GPIO1_CTRL_MASK;
+		pad_control |= gpio1_func & CS35L41_GPIO1_CTRL_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 (!enable)
+			break;
+
+		if (!pll_lock)
+			return -EINVAL;
+
+		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
+		if (ret == 0) {
+			ret = -ETIMEDOUT;
+		} else {
+			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));
+		}
+		break;
 	case CS35L41_INT_BOOST:
 		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
 					 enable << CS35L41_GLOBAL_EN_SHIFT);
diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index c006364e5335..1624510d09c0 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -360,6 +360,7 @@ static void cs35l41_boost_enable(struct cs35l41_private *cs35l41, unsigned int e
 {
 	switch (cs35l41->hw_cfg.bst_type) {
 	case CS35L41_INT_BOOST:
+	case CS35L41_SHD_BOOST_ACTV:
 		enable = enable ? CS35L41_BST_EN_DEFAULT : CS35L41_BST_DIS_FET_OFF;
 		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2, CS35L41_BST_EN_MASK,
 				enable << CS35L41_BST_EN_SHIFT);
@@ -455,6 +456,12 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 		ret = IRQ_HANDLED;
 	}
 
+	if (status[2] & CS35L41_PLL_LOCK) {
+		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS3, CS35L41_PLL_LOCK);
+		complete(&cs35l41->pll_lock);
+		ret = IRQ_HANDLED;
+	}
+
 done:
 	pm_runtime_mark_last_busy(cs35l41->dev);
 	pm_runtime_put_autosuspend(cs35l41->dev);
@@ -492,10 +499,12 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w,
 						cs35l41_pup_patch,
 						ARRAY_SIZE(cs35l41_pup_patch));
 
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 1);
+		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 1,
+				      &cs35l41->pll_lock);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0);
+		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0,
+				      &cs35l41->pll_lock);
 
 		ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
 					       val, val &  CS35L41_PDN_DONE_MASK,
@@ -802,6 +811,10 @@ static const struct snd_pcm_hw_constraint_list cs35l41_constraints = {
 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,
@@ -1252,6 +1265,10 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 	/* Set interrupt masks for critical errors */
 	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1,
 		     CS35L41_INT1_MASK_DEFAULT);
+	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||
+	    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);
 
 	ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL, cs35l41_irq,
 					IRQF_ONESHOT | IRQF_SHARED | irq_pol,
@@ -1275,6 +1292,8 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 	if (ret < 0)
 		goto err;
 
+	init_completion(&cs35l41->pll_lock);
+
 	pm_runtime_set_autosuspend_delay(cs35l41->dev, 3000);
 	pm_runtime_use_autosuspend(cs35l41->dev);
 	pm_runtime_mark_last_busy(cs35l41->dev);
@@ -1317,6 +1336,10 @@ void cs35l41_remove(struct cs35l41_private *cs35l41)
 	pm_runtime_disable(cs35l41->dev);
 
 	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1, 0xFFFFFFFF);
+	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||
+	    cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_ACTV)
+		regmap_update_bits(cs35l41->regmap, CS35L41_IRQ1_MASK3, CS35L41_INT3_PLL_LOCK_MASK,
+				   1 << CS35L41_INT3_PLL_LOCK_SHIFT);
 	kfree(cs35l41->dsp.system_name);
 	wm_adsp2_remove(&cs35l41->dsp);
 	cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type);
diff --git a/sound/soc/codecs/cs35l41.h b/sound/soc/codecs/cs35l41.h
index c85cbc1dd333..34d967d4372b 100644
--- a/sound/soc/codecs/cs35l41.h
+++ b/sound/soc/codecs/cs35l41.h
@@ -33,6 +33,7 @@ struct cs35l41_private {
 	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);
-- 
2.39.1

[PATCH v5 4/4] ASoC: dt-bindings: cirrus,cs35l41: Document CS35l41 shared boost

[Lucas Tanure]

Describe the properties used for shared boost configuration.
Based on David Rhodes shared boost patches.

Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/sound/cirrus,cs35l41.yaml      | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
index 18fb471aa891..f3c0a66f3474 100644
--- a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
+++ b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
@@ -85,11 +85,19 @@ properties:
       boost-cap-microfarad.
       External Boost must have GPIO1 as GPIO output. GPIO1 will be set high to
       enable boost voltage.
+      Shared boost allows two amplifiers to share a single boost circuit by
+      communicating on the MDSYNC bus. The passive amplifier does not control
+      the boost and receives data from the active amplifier. GPIO1 should be
+      configured for Sync when shared boost is used. Shared boost is not
+      compatible with External boost. Active amplifier requires
+      boost-peak-milliamp, boost-ind-nanohenry and boost-cap-microfarad.
       0 = Internal Boost
       1 = External Boost
+      2 = Shared Boost Active
+      3 = Shared Boost Passive
     $ref: /schemas/types.yaml#/definitions/uint32
     minimum: 0
-    maximum: 1
+    maximum: 3
 
   cirrus,gpio1-polarity-invert:
     description:
-- 
2.39.1

Re: [PATCH v5 0/4] Add CS35L41 shared boost feature

[Takashi Iwai]

On Fri, 10 Feb 2023 10:19:38 +0100,
Lucas Tanure wrote:
> 
> Valve's Steam Deck uses CS35L41 in shared boost mode, where both speakers
> share the boost circuit.
> Add this support in the shared lib, but for now, shared boost is not
> supported in HDA systems as would require BIOS changes.
> 
> Based on David Rhodes shared boost patches.
> 
> Also, fix boost config overwriting in IRQ found in the review and do a
> small refactor of the code.
> 
> Changes from V4:
>  - Fix Document subject
> 
> Changes from V3:
>  - Fix wrong code sent
>  - Fix ISO C90 mixed declarations and code 
> 
> Changes from V2:
>  - Drop External boost without VSPK Documentation
>  - Move Shared boost to use values 2 and 3
>  - Revert back to reg_sequence but reading the value first and only update
> the necessary bits
>  - Fix bug found by Intel kernel Test Robot
> 
> Changes from V1:
>  - Fix Documentation patch subject
>  - New patch for External boost without VSPK Documentation
>  - New patch to fix boost IRQ overwriting issue
>  - New patch to refactor IRQ release error code
>  - reinit_completion on pcm_startup
>  - fix DRE switch overwriting
>  - return IRQ_HANDLED in PLL_LOCK case
> 
> Lucas Tanure (4):
>   ASoC: cs35l41: Only disable internal boost
>   ASoC: cs35l41: Refactor error release code
>   ALSA: cs35l41: Add shared boost feature
>   ASoC: dt-bindings: cirrus,cs35l41: Document CS35l41 shared boost

In case the series going through Mark's tree:

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH v5 0/4] Add CS35L41 shared boost feature

[Takashi Iwai]

On Fri, 10 Feb 2023 10:19:38 +0100,
Lucas Tanure wrote:
> 
> Valve's Steam Deck uses CS35L41 in shared boost mode, where both speakers
> share the boost circuit.
> Add this support in the shared lib, but for now, shared boost is not
> supported in HDA systems as would require BIOS changes.
> 
> Based on David Rhodes shared boost patches.
> 
> Also, fix boost config overwriting in IRQ found in the review and do a
> small refactor of the code.
> 
> Changes from V4:
>  - Fix Document subject
> 
> Changes from V3:
>  - Fix wrong code sent
>  - Fix ISO C90 mixed declarations and code 
> 
> Changes from V2:
>  - Drop External boost without VSPK Documentation
>  - Move Shared boost to use values 2 and 3
>  - Revert back to reg_sequence but reading the value first and only update
> the necessary bits
>  - Fix bug found by Intel kernel Test Robot
> 
> Changes from V1:
>  - Fix Documentation patch subject
>  - New patch for External boost without VSPK Documentation
>  - New patch to fix boost IRQ overwriting issue
>  - New patch to refactor IRQ release error code
>  - reinit_completion on pcm_startup
>  - fix DRE switch overwriting
>  - return IRQ_HANDLED in PLL_LOCK case
> 
> Lucas Tanure (4):
>   ASoC: cs35l41: Only disable internal boost
>   ASoC: cs35l41: Refactor error release code
>   ALSA: cs35l41: Add shared boost feature
>   ASoC: dt-bindings: cirrus,cs35l41: Document CS35l41 shared boost

In case the series going through Mark's tree:

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
> Shared boost allows two amplifiers to share a single boost circuit by
> communicating on the MDSYNC bus.
> The passive amplifier does not control the boost and receives data from
> the active amplifier.
> 
> Shared Boost is not supported in HDA Systems.
> Based on David Rhodes shared boost patches.
> 
> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
> ---

Ok I found a copy of David's internal patch which helps a litte.

> --- a/sound/soc/codecs/cs35l41-lib.c
> +++ b/sound/soc/codecs/cs35l41-lib.c
> @@ -1114,12 +1114,31 @@ static const struct reg_sequence cs35l41_reset_to_safe[] = {
>  	{ 0x00000040,			0x00000033 },
>  };
>  
> +static const struct reg_sequence cs35l41_actv_seq[] = {
> +	/* SYNC_BST_CTL_RX_EN = 0; SYNC_BST_CTL_TX_EN = 1 */
> +	{CS35L41_MDSYNC_EN,        0x00001000},

David's internal patch appears to set 0x3000 on the active side,
not sure where that difference snuck in, or which is the correct
value. Your settings appear to make logical sense to me though, TX
on the active side, RX on the passive side.

> +	/* BST_CTL_SEL = CLASSH */
> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},

BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
is called in the datasheet, yay us for using the same names).
That does not mean this write is wrong, could just be the
comment, but what this does write is a bit odd so I would like
David to confirm this isn't some typo in his original patch.

> +};
> +
> +static const struct reg_sequence cs35l41_pass_seq[] = {
> +	/* SYNC_BST_CTL_RX_EN = 1; SYNC_BST_CTL_TX_EN = 0 */
> +	{CS35L41_MDSYNC_EN,        0x00002000},
> +	/* BST_EN = 0 */
> +	{CS35L41_PWR_CTRL2,        0x00003300},
> +	/* BST_CTL_SEL = MDSYNC */
> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000002},

Ditto here, comment doesn't match the write.

> -int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable)
> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
> +			  struct completion *pll_lock)
>  {
>  	int ret;
> +	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3;
> +	struct reg_sequence cs35l41_mdsync_down_seq[] = {
> +		{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},
> +	};
>  
>  	switch (b_type) {
> +	case CS35L41_SHD_BOOST_ACTV:
> +	case CS35L41_SHD_BOOST_PASS:
> +		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
> +		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
> +
> +		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
> +		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;

Are you sure this is what you want? In the case of powering up,
the sequence would end up being:

mdsync_down
 -> sets GLOBAL_EN on
mdsync_up
 -> sets GLOBAL_EN off
 -> sets GLOBAL_EN on

Feels like mdsync_down should always turn global_enable off? But
again I don't know for sure. But then I guess why is there the
extra write to turn it off in mdsync_up? I can't see any sign of
GLOBAL_EN bouncing in David's internal patch.

> +
> +		gpio1_func = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
> +		gpio1_func <<= CS35L41_GPIO1_CTRL_SHIFT;

Hm... this is a good point, probably would be nice to return an
error if the user sets a shared boost mode and a specific function
for the GPIO1 pin.

> +		pad_control &= ~CS35L41_GPIO1_CTRL_MASK;
> +		pad_control |= gpio1_func & CS35L41_GPIO1_CTRL_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 (!enable)
> +			break;
> +
> +		if (!pll_lock)
> +			return -EINVAL;
> +
> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> +		if (ret == 0) {
> +			ret = -ETIMEDOUT;
> +		} else {
> +			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));
> +		}
> +		break;

Thanks,
Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
> Shared boost allows two amplifiers to share a single boost circuit by
> communicating on the MDSYNC bus.
> The passive amplifier does not control the boost and receives data from
> the active amplifier.
> 
> Shared Boost is not supported in HDA Systems.
> Based on David Rhodes shared boost patches.
> 
> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
> ---

Ok I found a copy of David's internal patch which helps a litte.

> --- a/sound/soc/codecs/cs35l41-lib.c
> +++ b/sound/soc/codecs/cs35l41-lib.c
> @@ -1114,12 +1114,31 @@ static const struct reg_sequence cs35l41_reset_to_safe[] = {
>  	{ 0x00000040,			0x00000033 },
>  };
>  
> +static const struct reg_sequence cs35l41_actv_seq[] = {
> +	/* SYNC_BST_CTL_RX_EN = 0; SYNC_BST_CTL_TX_EN = 1 */
> +	{CS35L41_MDSYNC_EN,        0x00001000},

David's internal patch appears to set 0x3000 on the active side,
not sure where that difference snuck in, or which is the correct
value. Your settings appear to make logical sense to me though, TX
on the active side, RX on the passive side.

> +	/* BST_CTL_SEL = CLASSH */
> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},

BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
is called in the datasheet, yay us for using the same names).
That does not mean this write is wrong, could just be the
comment, but what this does write is a bit odd so I would like
David to confirm this isn't some typo in his original patch.

> +};
> +
> +static const struct reg_sequence cs35l41_pass_seq[] = {
> +	/* SYNC_BST_CTL_RX_EN = 1; SYNC_BST_CTL_TX_EN = 0 */
> +	{CS35L41_MDSYNC_EN,        0x00002000},
> +	/* BST_EN = 0 */
> +	{CS35L41_PWR_CTRL2,        0x00003300},
> +	/* BST_CTL_SEL = MDSYNC */
> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000002},

Ditto here, comment doesn't match the write.

> -int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable)
> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
> +			  struct completion *pll_lock)
>  {
>  	int ret;
> +	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3;
> +	struct reg_sequence cs35l41_mdsync_down_seq[] = {
> +		{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},
> +	};
>  
>  	switch (b_type) {
> +	case CS35L41_SHD_BOOST_ACTV:
> +	case CS35L41_SHD_BOOST_PASS:
> +		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
> +		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
> +
> +		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
> +		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;

Are you sure this is what you want? In the case of powering up,
the sequence would end up being:

mdsync_down
 -> sets GLOBAL_EN on
mdsync_up
 -> sets GLOBAL_EN off
 -> sets GLOBAL_EN on

Feels like mdsync_down should always turn global_enable off? But
again I don't know for sure. But then I guess why is there the
extra write to turn it off in mdsync_up? I can't see any sign of
GLOBAL_EN bouncing in David's internal patch.

> +
> +		gpio1_func = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
> +		gpio1_func <<= CS35L41_GPIO1_CTRL_SHIFT;

Hm... this is a good point, probably would be nice to return an
error if the user sets a shared boost mode and a specific function
for the GPIO1 pin.

> +		pad_control &= ~CS35L41_GPIO1_CTRL_MASK;
> +		pad_control |= gpio1_func & CS35L41_GPIO1_CTRL_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 (!enable)
> +			break;
> +
> +		if (!pll_lock)
> +			return -EINVAL;
> +
> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> +		if (ret == 0) {
> +			ret = -ETIMEDOUT;
> +		} else {
> +			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));
> +		}
> +		break;

Thanks,
Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Lucas Tanure]

On 10-02-2023 13:43, Charles Keepax wrote:
> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
>> Shared boost allows two amplifiers to share a single boost circuit by
>> communicating on the MDSYNC bus.
>> The passive amplifier does not control the boost and receives data from
>> the active amplifier.
>>
>> Shared Boost is not supported in HDA Systems.
>> Based on David Rhodes shared boost patches.
>>
>> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
>> ---
> 
> Ok I found a copy of David's internal patch which helps a litte.
> 
>> --- a/sound/soc/codecs/cs35l41-lib.c
>> +++ b/sound/soc/codecs/cs35l41-lib.c
>> @@ -1114,12 +1114,31 @@ static const struct reg_sequence cs35l41_reset_to_safe[] = {
>>   	{ 0x00000040,			0x00000033 },
>>   };
>>   
>> +static const struct reg_sequence cs35l41_actv_seq[] = {
>> +	/* SYNC_BST_CTL_RX_EN = 0; SYNC_BST_CTL_TX_EN = 1 */
>> +	{CS35L41_MDSYNC_EN,        0x00001000},
> 
> David's internal patch appears to set 0x3000 on the active side,
> not sure where that difference snuck in, or which is the correct
> value. Your settings appear to make logical sense to me though, TX
> on the active side, RX on the passive side.
Yes, I an e-mail David said that the passive was controlling the boost 
and out putting the value to the active one, but in the Documentation 
patch he sent the explanation was exactly the opposite.

Quote:
" The passive amplifier does not control the boost and receives data 
from the active amplifier."

And as the patch sets TX and RX in the same chip I changed to follow the 
documentation.

> 
>> +	/* BST_CTL_SEL = CLASSH */
>> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},
> 
> BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
> is called in the datasheet, yay us for using the same names).
> That does not mean this write is wrong, could just be the
> comment, but what this does write is a bit odd so I would like
> David to confirm this isn't some typo in his original patch.

I can't find BOOST_VOLTAGE_CFG on my datasheet, but BST_CTL_SEL is at 
0x00003804 ( BSTCVRT_VCTRL2 / VBST_CTL_2 ).
This write here is to select the boost control source, which for the 
active should be "Class H tracking value".
So I still think this is correct.

> 
>> +};
>> +
>> +static const struct reg_sequence cs35l41_pass_seq[] = {
>> +	/* SYNC_BST_CTL_RX_EN = 1; SYNC_BST_CTL_TX_EN = 0 */
>> +	{CS35L41_MDSYNC_EN,        0x00002000},
>> +	/* BST_EN = 0 */
>> +	{CS35L41_PWR_CTRL2,        0x00003300},
>> +	/* BST_CTL_SEL = MDSYNC */
>> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000002},
> 
> Ditto here, comment doesn't match the write.

Same as above, re-write to be the passive, with RX not TX.

> 
>> -int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable)
>> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
>> +			  struct completion *pll_lock)
>>   {
>>   	int ret;
>> +	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3;
>> +	struct reg_sequence cs35l41_mdsync_down_seq[] = {
>> +		{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},
>> +	};
>>   
>>   	switch (b_type) {
>> +	case CS35L41_SHD_BOOST_ACTV:
>> +	case CS35L41_SHD_BOOST_PASS:
>> +		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
>> +		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
>> +
>> +		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
>> +		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;
> 
> Are you sure this is what you want? In the case of powering up,
> the sequence would end up being:
> 
> mdsync_down
>   -> sets GLOBAL_EN on
> mdsync_up
>   -> sets GLOBAL_EN off
>   -> sets GLOBAL_EN on
> 
> Feels like mdsync_down should always turn global_enable off? But
> again I don't know for sure. But then I guess why is there the
> extra write to turn it off in mdsync_up? 

For the disable case (DAPM turning everything off) SYNC and Global 
enable are off and the code hits

if (!enable)
	break;

But for for enable case (DAPM turning everything On) the code continues 
enabling SYNC_EN, and turning off Global enable, as requested by
"4.10.1 Multidevice Synchronization Enable" page 70.
But as it is a enable path Global should be enabled again.

I can't see any sign of
> GLOBAL_EN bouncing in David's internal patch.

Yes, but it is required by :
"4.10.1 Multidevice Synchronization Enable" page 70.

Thanks
Lucas

> 
>> +
>> +		gpio1_func = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
>> +		gpio1_func <<= CS35L41_GPIO1_CTRL_SHIFT;
> 
> Hm... this is a good point, probably would be nice to return an
> error if the user sets a shared boost mode and a specific function
> for the GPIO1 pin.
> 
>> +		pad_control &= ~CS35L41_GPIO1_CTRL_MASK;
>> +		pad_control |= gpio1_func & CS35L41_GPIO1_CTRL_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 (!enable)
>> +			break;
>> +
>> +		if (!pll_lock)
>> +			return -EINVAL;
>> +
>> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
>> +		if (ret == 0) {
>> +			ret = -ETIMEDOUT;
>> +		} else {
>> +			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));
>> +		}
>> +		break;
> 
> Thanks,
> Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Lucas Tanure]

On 10-02-2023 13:43, Charles Keepax wrote:
> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
>> Shared boost allows two amplifiers to share a single boost circuit by
>> communicating on the MDSYNC bus.
>> The passive amplifier does not control the boost and receives data from
>> the active amplifier.
>>
>> Shared Boost is not supported in HDA Systems.
>> Based on David Rhodes shared boost patches.
>>
>> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
>> ---
> 
> Ok I found a copy of David's internal patch which helps a litte.
> 
>> --- a/sound/soc/codecs/cs35l41-lib.c
>> +++ b/sound/soc/codecs/cs35l41-lib.c
>> @@ -1114,12 +1114,31 @@ static const struct reg_sequence cs35l41_reset_to_safe[] = {
>>   	{ 0x00000040,			0x00000033 },
>>   };
>>   
>> +static const struct reg_sequence cs35l41_actv_seq[] = {
>> +	/* SYNC_BST_CTL_RX_EN = 0; SYNC_BST_CTL_TX_EN = 1 */
>> +	{CS35L41_MDSYNC_EN,        0x00001000},
> 
> David's internal patch appears to set 0x3000 on the active side,
> not sure where that difference snuck in, or which is the correct
> value. Your settings appear to make logical sense to me though, TX
> on the active side, RX on the passive side.
Yes, I an e-mail David said that the passive was controlling the boost 
and out putting the value to the active one, but in the Documentation 
patch he sent the explanation was exactly the opposite.

Quote:
" The passive amplifier does not control the boost and receives data 
from the active amplifier."

And as the patch sets TX and RX in the same chip I changed to follow the 
documentation.

> 
>> +	/* BST_CTL_SEL = CLASSH */
>> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},
> 
> BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
> is called in the datasheet, yay us for using the same names).
> That does not mean this write is wrong, could just be the
> comment, but what this does write is a bit odd so I would like
> David to confirm this isn't some typo in his original patch.

I can't find BOOST_VOLTAGE_CFG on my datasheet, but BST_CTL_SEL is at 
0x00003804 ( BSTCVRT_VCTRL2 / VBST_CTL_2 ).
This write here is to select the boost control source, which for the 
active should be "Class H tracking value".
So I still think this is correct.

> 
>> +};
>> +
>> +static const struct reg_sequence cs35l41_pass_seq[] = {
>> +	/* SYNC_BST_CTL_RX_EN = 1; SYNC_BST_CTL_TX_EN = 0 */
>> +	{CS35L41_MDSYNC_EN,        0x00002000},
>> +	/* BST_EN = 0 */
>> +	{CS35L41_PWR_CTRL2,        0x00003300},
>> +	/* BST_CTL_SEL = MDSYNC */
>> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000002},
> 
> Ditto here, comment doesn't match the write.

Same as above, re-write to be the passive, with RX not TX.

> 
>> -int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable)
>> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
>> +			  struct completion *pll_lock)
>>   {
>>   	int ret;
>> +	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3;
>> +	struct reg_sequence cs35l41_mdsync_down_seq[] = {
>> +		{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},
>> +	};
>>   
>>   	switch (b_type) {
>> +	case CS35L41_SHD_BOOST_ACTV:
>> +	case CS35L41_SHD_BOOST_PASS:
>> +		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
>> +		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
>> +
>> +		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
>> +		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;
> 
> Are you sure this is what you want? In the case of powering up,
> the sequence would end up being:
> 
> mdsync_down
>   -> sets GLOBAL_EN on
> mdsync_up
>   -> sets GLOBAL_EN off
>   -> sets GLOBAL_EN on
> 
> Feels like mdsync_down should always turn global_enable off? But
> again I don't know for sure. But then I guess why is there the
> extra write to turn it off in mdsync_up? 

For the disable case (DAPM turning everything off) SYNC and Global 
enable are off and the code hits

if (!enable)
	break;

But for for enable case (DAPM turning everything On) the code continues 
enabling SYNC_EN, and turning off Global enable, as requested by
"4.10.1 Multidevice Synchronization Enable" page 70.
But as it is a enable path Global should be enabled again.

I can't see any sign of
> GLOBAL_EN bouncing in David's internal patch.

Yes, but it is required by :
"4.10.1 Multidevice Synchronization Enable" page 70.

Thanks
Lucas

> 
>> +
>> +		gpio1_func = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
>> +		gpio1_func <<= CS35L41_GPIO1_CTRL_SHIFT;
> 
> Hm... this is a good point, probably would be nice to return an
> error if the user sets a shared boost mode and a specific function
> for the GPIO1 pin.
> 
>> +		pad_control &= ~CS35L41_GPIO1_CTRL_MASK;
>> +		pad_control |= gpio1_func & CS35L41_GPIO1_CTRL_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 (!enable)
>> +			break;
>> +
>> +		if (!pll_lock)
>> +			return -EINVAL;
>> +
>> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
>> +		if (ret == 0) {
>> +			ret = -ETIMEDOUT;
>> +		} else {
>> +			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));
>> +		}
>> +		break;
> 
> Thanks,
> Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
> On 10-02-2023 13:43, Charles Keepax wrote:
> >On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
> >>+	{CS35L41_MDSYNC_EN,        0x00001000},
> >David's internal patch appears to set 0x3000 on the active side,
> >not sure where that difference snuck in, or which is the correct
> >value. Your settings appear to make logical sense to me though, TX
> >on the active side, RX on the passive side.
> And as the patch sets TX and RX in the same chip I changed to follow
> the documentation.

Yeah I mean I suspect this is sensible, unless there is some
reason the controller side also needs to have RX enabled. Perhaps
for feedback or something from the passive side, but I imagine
this is just a typo in the original patch.

> >>+	/* BST_CTL_SEL = CLASSH */
> >>+	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},
> >BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
> >is called in the datasheet, yay us for using the same names).
> >That does not mean this write is wrong, could just be the
> >comment, but what this does write is a bit odd so I would like
> >David to confirm this isn't some typo in his original patch.
> I can't find BOOST_VOLTAGE_CFG on my datasheet, but BST_CTL_SEL is
> at 0x00003804 ( BSTCVRT_VCTRL2 / VBST_CTL_2 ).
> This write here is to select the boost control source, which for the
> active should be "Class H tracking value".
> So I still think this is correct.

Yeah this one is a mistake on my part, I was reviewing some
patches on another amp just before I think I have looked at the
wrong datasheet here. You are correct those bits are infact
BST_CTL_SEL. So ignore this one.

> >>+		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
> >>+		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
> >>+
> >>+		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
> >>+		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;
> >
> >Are you sure this is what you want? In the case of powering up,
> >the sequence would end up being:
> >
> >mdsync_down
> >  -> sets GLOBAL_EN on
> >mdsync_up
> >  -> sets GLOBAL_EN off
> >  -> sets GLOBAL_EN on
> >
> >Feels like mdsync_down should always turn global_enable off? But
> >again I don't know for sure. But then I guess why is there the
> >extra write to turn it off in mdsync_up?
> 
> For the disable case (DAPM turning everything off) SYNC and Global
> enable are off and the code hits
> 
> if (!enable)
> 	break;

Yes, so the disable flow makes perfect sense here it is the
enable flow that seemed odd.

> But for for enable case (DAPM turning everything On) the code
> continues enabling SYNC_EN, and turning off Global enable, as
> requested by
> "4.10.1 Multidevice Synchronization Enable" page 70.
> But as it is a enable path Global should be enabled again.
> 
> I can't see any sign of
> >GLOBAL_EN bouncing in David's internal patch.
> 
> Yes, but it is required by :
> "4.10.1 Multidevice Synchronization Enable" page 70.

Hmm... yes that does appear to suggest bouncing the global
enable. Kinda weird, I can't help but wonder if the turning
global enable off is actually needed, but I guess it does say
that so probably safest. It is also rather unclear on who that
sequence should be performed on it says:

"When powering up a second (and each subsequent) CS35L41B onto a
shared MDSYNC bus, the following protocol must
be followed"

But very unclear if that sequence should be followed on only the
new device, the master device, or on all devices. I will try to
find some time to chase some apps guys next week see if anyone
has any ideas.

Thanks,
Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
> On 10-02-2023 13:43, Charles Keepax wrote:
> >On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
> >>+	{CS35L41_MDSYNC_EN,        0x00001000},
> >David's internal patch appears to set 0x3000 on the active side,
> >not sure where that difference snuck in, or which is the correct
> >value. Your settings appear to make logical sense to me though, TX
> >on the active side, RX on the passive side.
> And as the patch sets TX and RX in the same chip I changed to follow
> the documentation.

Yeah I mean I suspect this is sensible, unless there is some
reason the controller side also needs to have RX enabled. Perhaps
for feedback or something from the passive side, but I imagine
this is just a typo in the original patch.

> >>+	/* BST_CTL_SEL = CLASSH */
> >>+	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},
> >BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
> >is called in the datasheet, yay us for using the same names).
> >That does not mean this write is wrong, could just be the
> >comment, but what this does write is a bit odd so I would like
> >David to confirm this isn't some typo in his original patch.
> I can't find BOOST_VOLTAGE_CFG on my datasheet, but BST_CTL_SEL is
> at 0x00003804 ( BSTCVRT_VCTRL2 / VBST_CTL_2 ).
> This write here is to select the boost control source, which for the
> active should be "Class H tracking value".
> So I still think this is correct.

Yeah this one is a mistake on my part, I was reviewing some
patches on another amp just before I think I have looked at the
wrong datasheet here. You are correct those bits are infact
BST_CTL_SEL. So ignore this one.

> >>+		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
> >>+		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
> >>+
> >>+		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
> >>+		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;
> >
> >Are you sure this is what you want? In the case of powering up,
> >the sequence would end up being:
> >
> >mdsync_down
> >  -> sets GLOBAL_EN on
> >mdsync_up
> >  -> sets GLOBAL_EN off
> >  -> sets GLOBAL_EN on
> >
> >Feels like mdsync_down should always turn global_enable off? But
> >again I don't know for sure. But then I guess why is there the
> >extra write to turn it off in mdsync_up?
> 
> For the disable case (DAPM turning everything off) SYNC and Global
> enable are off and the code hits
> 
> if (!enable)
> 	break;

Yes, so the disable flow makes perfect sense here it is the
enable flow that seemed odd.

> But for for enable case (DAPM turning everything On) the code
> continues enabling SYNC_EN, and turning off Global enable, as
> requested by
> "4.10.1 Multidevice Synchronization Enable" page 70.
> But as it is a enable path Global should be enabled again.
> 
> I can't see any sign of
> >GLOBAL_EN bouncing in David's internal patch.
> 
> Yes, but it is required by :
> "4.10.1 Multidevice Synchronization Enable" page 70.

Hmm... yes that does appear to suggest bouncing the global
enable. Kinda weird, I can't help but wonder if the turning
global enable off is actually needed, but I guess it does say
that so probably safest. It is also rather unclear on who that
sequence should be performed on it says:

"When powering up a second (and each subsequent) CS35L41B onto a
shared MDSYNC bus, the following protocol must
be followed"

But very unclear if that sequence should be followed on only the
new device, the master device, or on all devices. I will try to
find some time to chase some apps guys next week see if anyone
has any ideas.

Thanks,
Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Lucas Tanure]

On 11-02-2023 17:06, Charles Keepax wrote:
> On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
>> On 10-02-2023 13:43, Charles Keepax wrote:
>>> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
>>>> +	{CS35L41_MDSYNC_EN,        0x00001000},
>>> David's internal patch appears to set 0x3000 on the active side,
>>> not sure where that difference snuck in, or which is the correct
>>> value. Your settings appear to make logical sense to me though, TX
>>> on the active side, RX on the passive side.
>> And as the patch sets TX and RX in the same chip I changed to follow
>> the documentation.
> 
> Yeah I mean I suspect this is sensible, unless there is some
> reason the controller side also needs to have RX enabled. Perhaps
> for feedback or something from the passive side, but I imagine
> this is just a typo in the original patch.

Ok, but the other side doesn't have both RX and TX enabled.
If the active side needed RX to receive information for the other side, 
the passive one would need TX enabled too.
So if a feedback is necessary, both channels on both sides would be 
enabled, not one channel in one side and both on the other.


> 
>>>> +	/* BST_CTL_SEL = CLASSH */
>>>> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},
>>> BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
>>> is called in the datasheet, yay us for using the same names).
>>> That does not mean this write is wrong, could just be the
>>> comment, but what this does write is a bit odd so I would like
>>> David to confirm this isn't some typo in his original patch.
>> I can't find BOOST_VOLTAGE_CFG on my datasheet, but BST_CTL_SEL is
>> at 0x00003804 ( BSTCVRT_VCTRL2 / VBST_CTL_2 ).
>> This write here is to select the boost control source, which for the
>> active should be "Class H tracking value".
>> So I still think this is correct.
> 
> Yeah this one is a mistake on my part, I was reviewing some
> patches on another amp just before I think I have looked at the
> wrong datasheet here. You are correct those bits are infact
> BST_CTL_SEL. So ignore this one.
> 
>>>> +		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
>>>> +		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
>>>> +
>>>> +		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
>>>> +		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;
>>>
>>> Are you sure this is what you want? In the case of powering up,
>>> the sequence would end up being:
>>>
>>> mdsync_down
>>>   -> sets GLOBAL_EN on
>>> mdsync_up
>>>   -> sets GLOBAL_EN off
>>>   -> sets GLOBAL_EN on
>>>
>>> Feels like mdsync_down should always turn global_enable off? But
>>> again I don't know for sure. But then I guess why is there the
>>> extra write to turn it off in mdsync_up?
>>
>> For the disable case (DAPM turning everything off) SYNC and Global
>> enable are off and the code hits
>>
>> if (!enable)
>> 	break;
> 
> Yes, so the disable flow makes perfect sense here it is the
> enable flow that seemed odd.
> 
>> But for for enable case (DAPM turning everything On) the code
>> continues enabling SYNC_EN, and turning off Global enable, as
>> requested by
>> "4.10.1 Multidevice Synchronization Enable" page 70.
>> But as it is a enable path Global should be enabled again.
>>
>> I can't see any sign of
>>> GLOBAL_EN bouncing in David's internal patch.
>>
>> Yes, but it is required by :
>> "4.10.1 Multidevice Synchronization Enable" page 70.
> 
> Hmm... yes that does appear to suggest bouncing the global
> enable. Kinda weird, I can't help but wonder if the turning
> global enable off is actually needed, but I guess it does say
> that so probably safest. It is also rather unclear on who that
> sequence should be performed on it says:
> 
> "When powering up a second (and each subsequent) CS35L41B onto a
> shared MDSYNC bus, the following protocol must
> be followed"
> 
> But very unclear if that sequence should be followed on only the
> new device, the master device, or on all devices. I will try to
> find some time to chase some apps guys next week see if anyone
> has any ideas.
I had long talks with apps guys on this when I was at Cirrus.
And my understanding is:
A new CS35L41 can misunderstand the information on MDSYNC bus if a 
communication is already in place, between another pair of CS35L41, so 
to avoid that, any CS35L41 being turn on in a already active MDSYNC bus, 
must execute those steps.

We could move the active amp up in DAPM graph so its enabled before all 
passive ones, but we would still need to execute that for all passive 
amps. So there is not much point in that.

Here I can see that if I enable SYNC_EN during probe without clocks the 
device becomes unresponsive, at least with the current code.
So following the datasheet and enabling SYNC_EN only after clocks seems 
to resolve Steam decks issue.

Questions I never got an answer from APPS guys:

- Can we enable SYNC_EN during probe if we know there is no playback
happening, no clocks and Global enable off? Steam decks seem to answer 
no here. If yes, why having pm_runtime features makes the device become 
unresponsive?

- Can we leave SYNC_EN enabled during Global enable off and no clocks?

- If SYNC_EN is enabled and we only set Global enable on after the PLL 
lock happened, do we still need to execute those steps? I mean, if the 
driver only deals with Global enable and leaves shared boost configured 
since boost, will MDSYNC bus work?

Thanks
Lucas

> 
> Thanks,
> Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Lucas Tanure]

On 11-02-2023 17:06, Charles Keepax wrote:
> On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
>> On 10-02-2023 13:43, Charles Keepax wrote:
>>> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
>>>> +	{CS35L41_MDSYNC_EN,        0x00001000},
>>> David's internal patch appears to set 0x3000 on the active side,
>>> not sure where that difference snuck in, or which is the correct
>>> value. Your settings appear to make logical sense to me though, TX
>>> on the active side, RX on the passive side.
>> And as the patch sets TX and RX in the same chip I changed to follow
>> the documentation.
> 
> Yeah I mean I suspect this is sensible, unless there is some
> reason the controller side also needs to have RX enabled. Perhaps
> for feedback or something from the passive side, but I imagine
> this is just a typo in the original patch.

Ok, but the other side doesn't have both RX and TX enabled.
If the active side needed RX to receive information for the other side, 
the passive one would need TX enabled too.
So if a feedback is necessary, both channels on both sides would be 
enabled, not one channel in one side and both on the other.


> 
>>>> +	/* BST_CTL_SEL = CLASSH */
>>>> +	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},
>>> BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
>>> is called in the datasheet, yay us for using the same names).
>>> That does not mean this write is wrong, could just be the
>>> comment, but what this does write is a bit odd so I would like
>>> David to confirm this isn't some typo in his original patch.
>> I can't find BOOST_VOLTAGE_CFG on my datasheet, but BST_CTL_SEL is
>> at 0x00003804 ( BSTCVRT_VCTRL2 / VBST_CTL_2 ).
>> This write here is to select the boost control source, which for the
>> active should be "Class H tracking value".
>> So I still think this is correct.
> 
> Yeah this one is a mistake on my part, I was reviewing some
> patches on another amp just before I think I have looked at the
> wrong datasheet here. You are correct those bits are infact
> BST_CTL_SEL. So ignore this one.
> 
>>>> +		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
>>>> +		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
>>>> +
>>>> +		pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
>>>> +		pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;
>>>
>>> Are you sure this is what you want? In the case of powering up,
>>> the sequence would end up being:
>>>
>>> mdsync_down
>>>   -> sets GLOBAL_EN on
>>> mdsync_up
>>>   -> sets GLOBAL_EN off
>>>   -> sets GLOBAL_EN on
>>>
>>> Feels like mdsync_down should always turn global_enable off? But
>>> again I don't know for sure. But then I guess why is there the
>>> extra write to turn it off in mdsync_up?
>>
>> For the disable case (DAPM turning everything off) SYNC and Global
>> enable are off and the code hits
>>
>> if (!enable)
>> 	break;
> 
> Yes, so the disable flow makes perfect sense here it is the
> enable flow that seemed odd.
> 
>> But for for enable case (DAPM turning everything On) the code
>> continues enabling SYNC_EN, and turning off Global enable, as
>> requested by
>> "4.10.1 Multidevice Synchronization Enable" page 70.
>> But as it is a enable path Global should be enabled again.
>>
>> I can't see any sign of
>>> GLOBAL_EN bouncing in David's internal patch.
>>
>> Yes, but it is required by :
>> "4.10.1 Multidevice Synchronization Enable" page 70.
> 
> Hmm... yes that does appear to suggest bouncing the global
> enable. Kinda weird, I can't help but wonder if the turning
> global enable off is actually needed, but I guess it does say
> that so probably safest. It is also rather unclear on who that
> sequence should be performed on it says:
> 
> "When powering up a second (and each subsequent) CS35L41B onto a
> shared MDSYNC bus, the following protocol must
> be followed"
> 
> But very unclear if that sequence should be followed on only the
> new device, the master device, or on all devices. I will try to
> find some time to chase some apps guys next week see if anyone
> has any ideas.
I had long talks with apps guys on this when I was at Cirrus.
And my understanding is:
A new CS35L41 can misunderstand the information on MDSYNC bus if a 
communication is already in place, between another pair of CS35L41, so 
to avoid that, any CS35L41 being turn on in a already active MDSYNC bus, 
must execute those steps.

We could move the active amp up in DAPM graph so its enabled before all 
passive ones, but we would still need to execute that for all passive 
amps. So there is not much point in that.

Here I can see that if I enable SYNC_EN during probe without clocks the 
device becomes unresponsive, at least with the current code.
So following the datasheet and enabling SYNC_EN only after clocks seems 
to resolve Steam decks issue.

Questions I never got an answer from APPS guys:

- Can we enable SYNC_EN during probe if we know there is no playback
happening, no clocks and Global enable off? Steam decks seem to answer 
no here. If yes, why having pm_runtime features makes the device become 
unresponsive?

- Can we leave SYNC_EN enabled during Global enable off and no clocks?

- If SYNC_EN is enabled and we only set Global enable on after the PLL 
lock happened, do we still need to execute those steps? I mean, if the 
driver only deals with Global enable and leaves shared boost configured 
since boost, will MDSYNC bus work?

Thanks
Lucas

> 
> Thanks,
> Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

On Sun, Feb 12, 2023 at 09:28:39AM +0000, Lucas Tanure wrote:
> On 11-02-2023 17:06, Charles Keepax wrote:
> >On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
> >>On 10-02-2023 13:43, Charles Keepax wrote:
> >>>On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
> Ok, but the other side doesn't have both RX and TX enabled.
> If the active side needed RX to receive information for the other
> side, the passive one would need TX enabled too.
> So if a feedback is necessary, both channels on both sides would be
> enabled, not one channel in one side and both on the other.

A very good point :-)

> >"When powering up a second (and each subsequent) CS35L41B onto a
> >shared MDSYNC bus, the following protocol must
> >be followed"
> >
> >But very unclear if that sequence should be followed on only the
> >new device, the master device, or on all devices. I will try to
> >find some time to chase some apps guys next week see if anyone
> >has any ideas.
> I had long talks with apps guys on this when I was at Cirrus.
> And my understanding is:
> A new CS35L41 can misunderstand the information on MDSYNC bus if a
> communication is already in place, between another pair of CS35L41,
> so to avoid that, any CS35L41 being turn on in a already active
> MDSYNC bus, must execute those steps.

Ok so that implies we are ok, since that suggests we are
saying that only the new amp to the bus needs to execute the
sequence, not the amps already on the bus.

> We could move the active amp up in DAPM graph so its enabled before
> all passive ones, but we would still need to execute that for all
> passive amps. So there is not much point in that.

Agree, fine as is.

> 
> Here I can see that if I enable SYNC_EN during probe without clocks
> the device becomes unresponsive, at least with the current code.
> So following the datasheet and enabling SYNC_EN only after clocks
> seems to resolve Steam decks issue.
> 
> Questions I never got an answer from APPS guys:
> 
> - Can we enable SYNC_EN during probe if we know there is no playback
> happening, no clocks and Global enable off? Steam decks seem to
> answer no here. If yes, why having pm_runtime features makes the
> device become unresponsive?
> 
> - Can we leave SYNC_EN enabled during Global enable off and no clocks?

These two I think are not too much of a concern, turning sync on as
part of powering up the amps doesn't seem to be a big concern,
its not a lot of writes.

> - If SYNC_EN is enabled and we only set Global enable on after the
> PLL lock happened, do we still need to execute those steps? I mean,
> if the driver only deals with Global enable and leaves shared boost
> configured since boost, will MDSYNC bus work?

Yeah I think here it is also probably safer to just do it anyway.

I would still like David to do a quick review, unfortunately he
is off at the moment but should be back Monday next week. But
from my side I think this is probably all good:

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

On Sun, Feb 12, 2023 at 09:28:39AM +0000, Lucas Tanure wrote:
> On 11-02-2023 17:06, Charles Keepax wrote:
> >On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
> >>On 10-02-2023 13:43, Charles Keepax wrote:
> >>>On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
> Ok, but the other side doesn't have both RX and TX enabled.
> If the active side needed RX to receive information for the other
> side, the passive one would need TX enabled too.
> So if a feedback is necessary, both channels on both sides would be
> enabled, not one channel in one side and both on the other.

A very good point :-)

> >"When powering up a second (and each subsequent) CS35L41B onto a
> >shared MDSYNC bus, the following protocol must
> >be followed"
> >
> >But very unclear if that sequence should be followed on only the
> >new device, the master device, or on all devices. I will try to
> >find some time to chase some apps guys next week see if anyone
> >has any ideas.
> I had long talks with apps guys on this when I was at Cirrus.
> And my understanding is:
> A new CS35L41 can misunderstand the information on MDSYNC bus if a
> communication is already in place, between another pair of CS35L41,
> so to avoid that, any CS35L41 being turn on in a already active
> MDSYNC bus, must execute those steps.

Ok so that implies we are ok, since that suggests we are
saying that only the new amp to the bus needs to execute the
sequence, not the amps already on the bus.

> We could move the active amp up in DAPM graph so its enabled before
> all passive ones, but we would still need to execute that for all
> passive amps. So there is not much point in that.

Agree, fine as is.

> 
> Here I can see that if I enable SYNC_EN during probe without clocks
> the device becomes unresponsive, at least with the current code.
> So following the datasheet and enabling SYNC_EN only after clocks
> seems to resolve Steam decks issue.
> 
> Questions I never got an answer from APPS guys:
> 
> - Can we enable SYNC_EN during probe if we know there is no playback
> happening, no clocks and Global enable off? Steam decks seem to
> answer no here. If yes, why having pm_runtime features makes the
> device become unresponsive?
> 
> - Can we leave SYNC_EN enabled during Global enable off and no clocks?

These two I think are not too much of a concern, turning sync on as
part of powering up the amps doesn't seem to be a big concern,
its not a lot of writes.

> - If SYNC_EN is enabled and we only set Global enable on after the
> PLL lock happened, do we still need to execute those steps? I mean,
> if the driver only deals with Global enable and leaves shared boost
> configured since boost, will MDSYNC bus work?

Yeah I think here it is also probably safer to just do it anyway.

I would still like David to do a quick review, unfortunately he
is off at the moment but should be back Monday next week. But
from my side I think this is probably all good:

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[David Rhodes]

On 2/12/23 03:28, Lucas Tanure wrote:
> On 11-02-2023 17:06, Charles Keepax wrote:
>> On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
>>> On 10-02-2023 13:43, Charles Keepax wrote:
>>>> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
>>>>> + {CS35L41_MDSYNC_EN,        0x00001000},
>>>> David's internal patch appears to set 0x3000 on the active side,
>>>> not sure where that difference snuck in, or which is the correct
>>>> value. Your settings appear to make logical sense to me though, TX
>>>> on the active side, RX on the passive side.
>>> And as the patch sets TX and RX in the same chip I changed to follow
>>> the documentation.
>>
>> Yeah I mean I suspect this is sensible, unless there is some
>> reason the controller side also needs to have RX enabled. Perhaps
>> for feedback or something from the passive side, but I imagine
>> this is just a typo in the original patch.
>
> Ok, but the other side doesn't have both RX and TX enabled.
> If the active side needed RX to receive information for the other 
> side, the passive one would need TX enabled too.
> So if a feedback is necessary, both channels on both sides would be 
> enabled, not one channel in one side and both on the other.
>
Both amps need to transmit their boost targets to the MDSYNC bus. The 
active amp needs to receive the combined boost target from the MDSYNC 
bus. That is why the active amp should enable both RX and TX, and the 
passive amp only needs to enable TX. It is not simply a unidirectional 
flow of data from one amp to the other.

Sorry if the documentation has been mismatched or confusing at times. 
It's taken me a while to gather the right understanding of how this all 
works.


On 2/10/23 03:19, Lucas Tanure wrote:
> +      Shared boost allows two amplifiers to share a single boost circuit by
> +      communicating on the MDSYNC bus. The passive amplifier does not control
> +      the boost and receives data from the active amplifier. GPIO1 should be

Not quite correct. I would suggest: "Shared boost allows two amplifiers 
to share a single boost circuit by communicating on the MDSYNC bus. The 
active amplifier controls the boost circuit using combined data from 
both amplifiers."


On 2/10/23 08:39, Lucas Tanure wrote:
>
> This write here is to select the boost control source, which for the 
> active should be "Class H tracking value". 

Active should use the MDSYNC value. Otherwise it will not provide boost 
to the passive amp when there is only audio on the passive amp's channel.


I believe there is another change needed for the Deck, to handle the 
'legacy' property names instead of bst-type?

Other than the changes needed to the reg_sequences this looks good.


Thanks,

David

Re: [PATCH v5 0/4] Add CS35L41 shared boost feature

[David Rhodes]

Apologies for the formatting mistake. Resending the previous reply.

On 2/12/23 03:28, Lucas Tanure wrote:
 > On 11-02-2023 17:06, Charles Keepax wrote:
 >> On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
 >>> On 10-02-2023 13:43, Charles Keepax wrote:
 >>>> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
 >>>>> +    {CS35L41_MDSYNC_EN,        0x00001000},
 >>>> David's internal patch appears to set 0x3000 on the active side,
 >>>> not sure where that difference snuck in, or which is the correct
 >>>> value. Your settings appear to make logical sense to me though, TX
 >>>> on the active side, RX on the passive side.
 >>> And as the patch sets TX and RX in the same chip I changed to follow
 >>> the documentation.
 >>
 >> Yeah I mean I suspect this is sensible, unless there is some
 >> reason the controller side also needs to have RX enabled. Perhaps
 >> for feedback or something from the passive side, but I imagine
 >> this is just a typo in the original patch.
 >
 > Ok, but the other side doesn't have both RX and TX enabled.
 > If the active side needed RX to receive information for the other 
side, the passive one would need TX enabled too.
 > So if a feedback is necessary, both channels on both sides would be 
enabled, not one channel in one side and both on the other.
 >
Both amps need to transmit their boost targets to the MDSYNC bus. The 
active amp needs to receive the combined boost target from the MDSYNC 
bus. That is why the active amp should enable both RX and TX, and the 
passive amp only needs to enable TX. It is not simply a unidirectional 
flow of data from one amp to the other.

Sorry if the documentation has been mismatched or confusing at times. 
It's taken me a while to gather the right understanding of how this all 
works.


On 2/10/23 03:19, Lucas Tanure wrote:
 > +      Shared boost allows two amplifiers to share a single boost 
circuit by
 > +      communicating on the MDSYNC bus. The passive amplifier does 
not control
 > +      the boost and receives data from the active amplifier. GPIO1 
should be

Not quite correct. I would suggest: "Shared boost allows two amplifiers 
to share a single boost circuit by communicating on the MDSYNC bus. The 
active amplifier controls the boost circuit using combined data from 
both amplifiers."


On 2/10/23 08:39, Lucas Tanure wrote:
 >
 > This write here is to select the boost control source, which for the 
active should be "Class H tracking value".

Active should use the MDSYNC value. Otherwise it will not provide boost 
to the passive amp when there is only audio on the passive amp's channel.


I believe there is another change needed for the Deck, to handle the 
'legacy' property names instead of bst-type?

Other than the changes needed to the reg_sequences this looks good.


Thanks,

David

Re: [PATCH v5 0/4] Add CS35L41 shared boost feature

[David Rhodes]

Apologies for the formatting mistake. Resending the previous reply.

On 2/12/23 03:28, Lucas Tanure wrote:
 > On 11-02-2023 17:06, Charles Keepax wrote:
 >> On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
 >>> On 10-02-2023 13:43, Charles Keepax wrote:
 >>>> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
 >>>>> +    {CS35L41_MDSYNC_EN,        0x00001000},
 >>>> David's internal patch appears to set 0x3000 on the active side,
 >>>> not sure where that difference snuck in, or which is the correct
 >>>> value. Your settings appear to make logical sense to me though, TX
 >>>> on the active side, RX on the passive side.
 >>> And as the patch sets TX and RX in the same chip I changed to follow
 >>> the documentation.
 >>
 >> Yeah I mean I suspect this is sensible, unless there is some
 >> reason the controller side also needs to have RX enabled. Perhaps
 >> for feedback or something from the passive side, but I imagine
 >> this is just a typo in the original patch.
 >
 > Ok, but the other side doesn't have both RX and TX enabled.
 > If the active side needed RX to receive information for the other 
side, the passive one would need TX enabled too.
 > So if a feedback is necessary, both channels on both sides would be 
enabled, not one channel in one side and both on the other.
 >
Both amps need to transmit their boost targets to the MDSYNC bus. The 
active amp needs to receive the combined boost target from the MDSYNC 
bus. That is why the active amp should enable both RX and TX, and the 
passive amp only needs to enable TX. It is not simply a unidirectional 
flow of data from one amp to the other.

Sorry if the documentation has been mismatched or confusing at times. 
It's taken me a while to gather the right understanding of how this all 
works.


On 2/10/23 03:19, Lucas Tanure wrote:
 > +      Shared boost allows two amplifiers to share a single boost 
circuit by
 > +      communicating on the MDSYNC bus. The passive amplifier does 
not control
 > +      the boost and receives data from the active amplifier. GPIO1 
should be

Not quite correct. I would suggest: "Shared boost allows two amplifiers 
to share a single boost circuit by communicating on the MDSYNC bus. The 
active amplifier controls the boost circuit using combined data from 
both amplifiers."


On 2/10/23 08:39, Lucas Tanure wrote:
 >
 > This write here is to select the boost control source, which for the 
active should be "Class H tracking value".

Active should use the MDSYNC value. Otherwise it will not provide boost 
to the passive amp when there is only audio on the passive amp's channel.


I believe there is another change needed for the Deck, to handle the 
'legacy' property names instead of bst-type?

Other than the changes needed to the reg_sequences this looks good.


Thanks,

David

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Lucas Tanure]

On 20-02-2023 19:25, David Rhodes wrote:
> 
> On 2/12/23 03:28, Lucas Tanure wrote:
>> On 11-02-2023 17:06, Charles Keepax wrote:
>>> On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
>>>> On 10-02-2023 13:43, Charles Keepax wrote:
>>>>> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
>>>>>> + {CS35L41_MDSYNC_EN,        0x00001000},
>>>>> David's internal patch appears to set 0x3000 on the active side,
>>>>> not sure where that difference snuck in, or which is the correct
>>>>> value. Your settings appear to make logical sense to me though, TX
>>>>> on the active side, RX on the passive side.
>>>> And as the patch sets TX and RX in the same chip I changed to follow
>>>> the documentation.
>>>
>>> Yeah I mean I suspect this is sensible, unless there is some
>>> reason the controller side also needs to have RX enabled. Perhaps
>>> for feedback or something from the passive side, but I imagine
>>> this is just a typo in the original patch.
>>
>> Ok, but the other side doesn't have both RX and TX enabled.
>> If the active side needed RX to receive information for the other 
>> side, the passive one would need TX enabled too.
>> So if a feedback is necessary, both channels on both sides would be 
>> enabled, not one channel in one side and both on the other.
>>
> Both amps need to transmit their boost targets to the MDSYNC bus. The 
> active amp needs to receive the combined boost target from the MDSYNC 
> bus. That is why the active amp should enable both RX and TX, and the 
> passive amp only needs to enable TX. It is not simply a unidirectional 
> flow of data from one amp to the other.
> 
> Sorry if the documentation has been mismatched or confusing at times. 
> It's taken me a while to gather the right understanding of how this all 
> works.
> 
> 
> On 2/10/23 03:19, Lucas Tanure wrote:
>> +      Shared boost allows two amplifiers to share a single boost circuit by
>> +      communicating on the MDSYNC bus. The passive amplifier does not control
>> +      the boost and receives data from the active amplifier. GPIO1 should be
> 
> Not quite correct. I would suggest: "Shared boost allows two amplifiers 
> to share a single boost circuit by communicating on the MDSYNC bus. The 
> active amplifier controls the boost circuit using combined data from 
> both amplifiers."
Ok

> 
> 
> On 2/10/23 08:39, Lucas Tanure wrote:
>>
>> This write here is to select the boost control source, which for the 
>> active should be "Class H tracking value". 
> 
> Active should use the MDSYNC value. Otherwise it will not provide boost 
> to the passive amp when there is only audio on the passive amp's channel.
David can you confirm that both sides should use MDSYNC for boost 
control source?


> 
> 
> I believe there is another change needed for the Deck, to handle the 
> 'legacy' property names instead of bst-type?
I am working with valve to update their bios.

> 
> Other than the changes needed to the reg_sequences this looks good.
> 
> 
> Thanks,
> 
> David
> 

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[David Rhodes]

On 2/21/23 02:28, Lucas Tanure wrote:
> David can you confirm that both sides should use MDSYNC for boost 
> control source?

Both amps can use the value 'MDSYNC' for BST_CTL_SEL.
The value for the passive amp does not affect the behavior because BST_EN=0.


On 2/21/23 02:28, Lucas Tanure wrote:
 >> I believe there is another change needed for the Deck, to handle the
 >> 'legacy' property names instead of bst-type?
 > I am working with valve to update their bios.

Great, I think that's a better solution.

Thanks,
David

Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[David Rhodes]

On 2/21/23 02:28, Lucas Tanure wrote:
> David can you confirm that both sides should use MDSYNC for boost 
> control source?

Both amps can use the value 'MDSYNC' for BST_CTL_SEL.
The value for the passive amp does not affect the behavior because BST_EN=0.


On 2/21/23 02:28, Lucas Tanure wrote:
 >> I believe there is another change needed for the Deck, to handle the
 >> 'legacy' property names instead of bst-type?
 > I am working with valve to update their bios.

Great, I think that's a better solution.

Thanks,
David