[PATCH 0/2] Add CS35L41 shared boost feature

[PATCH 0/2] 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.

Lucas Tanure (2):
  ALSA: cs35l41: Add shared boost feature
  Documentation: cs35l41: Shared boost properties

 .../bindings/sound/cirrus,cs35l41.yaml        | 11 +++-
 include/sound/cs35l41.h                       | 10 +++-
 sound/pci/hda/cs35l41_hda.c                   |  6 +-
 sound/soc/codecs/cs35l41-lib.c                | 56 ++++++++++++++++++-
 sound/soc/codecs/cs35l41.c                    | 21 ++++++-
 sound/soc/codecs/cs35l41.h                    |  1 +
 6 files changed, 97 insertions(+), 8 deletions(-)

-- 
2.39.1

[PATCH 1/2] 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.

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

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index 9ac5918269a5..a034ebe03a0e 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
@@ -743,6 +748,8 @@ enum cs35l41_boost_type {
 	CS35L41_INT_BOOST,
 	CS35L41_EXT_BOOST,
 	CS35L41_EXT_BOOST_NO_VSPK_SWITCH,
+	CS35L41_SHD_BOOST_ACTV,
+	CS35L41_SHD_BOOST_PASS,
 };
 
 enum cs35l41_clk_ids {
@@ -891,6 +898,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..aedd81a766a7 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,9 @@ 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:
+		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 +1187,43 @@ 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)
+static const struct reg_sequence cs35l41_shd_boost_seq[] = {
+	{CS35L41_PWR_CTRL3,	0x01000110},
+	{CS35L41_PWR_CTRL1,	0x00000000, 3000 },
+	{CS35L41_PWR_CTRL1,	0x00000001, 3000 },
+};
+
+int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
+			  struct completion *pll_lock)
 {
 	int ret;
+	unsigned int gpio1;
 
 	switch (b_type) {
+	case CS35L41_SHD_BOOST_ACTV:
+	case CS35L41_SHD_BOOST_PASS:
+		regmap_update_bits(regmap, CS35L41_PWR_CTRL3, CS35L41_SYNC_EN_MASK, 0);
+
+		gpio1 = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
+		regmap_update_bits(regmap, CS35L41_GPIO_PAD_CONTROL, CS35L41_GPIO1_CTRL_MASK,
+				   gpio1 << CS35L41_GPIO1_CTRL_SHIFT);
+
+		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
+					 enable << CS35L41_GLOBAL_EN_SHIFT);
+		usleep_range(3000, 3100);
+		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_multi_reg_write(regmap, cs35l41_shd_boost_seq,
+					       ARRAY_SIZE(cs35l41_shd_boost_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 c223d83e02cf..40c0620d8a7a 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -483,6 +483,11 @@ 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);
+	}
+
 done:
 	pm_runtime_mark_last_busy(cs35l41->dev);
 	pm_runtime_put_autosuspend(cs35l41->dev);
@@ -520,10 +525,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,
@@ -1280,6 +1287,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,
@@ -1303,6 +1314,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);
@@ -1345,6 +1358,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 2/2] Documentation: cs35l41: Shared boost properties

[Lucas Tanure]

Describe the properties used for shared boost
configuration.

Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
---
 .../devicetree/bindings/sound/cirrus,cs35l41.yaml     | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
index 18fb471aa891..6f5f01bec6f1 100644
--- a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
+++ b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
@@ -85,11 +85,20 @@ 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 = Reserved
+      3 = Shared Boost Active
+      4 = Shared Boost Passive
     $ref: /schemas/types.yaml#/definitions/uint32
     minimum: 0
-    maximum: 1
+    maximum: 4
 
   cirrus,gpio1-polarity-invert:
     description:
-- 
2.39.1

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Krzysztof Kozlowski]

On 07/02/2023 11:40, Lucas Tanure wrote:
> Describe the properties used for shared boost
> configuration.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

> 
> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
> ---
>  .../devicetree/bindings/sound/cirrus,cs35l41.yaml     | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> index 18fb471aa891..6f5f01bec6f1 100644
> --- a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> +++ b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> @@ -85,11 +85,20 @@ 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 = Reserved

How binding can be reserved? For what and why? Drop. 2 is shared active,
3 is shared passive.

Best regards,
Krzysztof

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Krzysztof Kozlowski]

On 07/02/2023 11:40, Lucas Tanure wrote:
> Describe the properties used for shared boost
> configuration.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

> 
> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
> ---
>  .../devicetree/bindings/sound/cirrus,cs35l41.yaml     | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> index 18fb471aa891..6f5f01bec6f1 100644
> --- a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> +++ b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
> @@ -85,11 +85,20 @@ 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 = Reserved

How binding can be reserved? For what and why? Drop. 2 is shared active,
3 is shared passive.

Best regards,
Krzysztof

Re: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

On Tue, Feb 07, 2023 at 10:40:20AM +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.
> 

Probably would be nice to put at least a note to say based on
David's patches.

> +static const struct reg_sequence cs35l41_shd_boost_seq[] = {
> +	{CS35L41_PWR_CTRL3,	0x01000110},

This will blat whatever the user set in the DRE switch.
Technically blats the CLASS H enable from the DAPM widget too,
but as that always turns on should be a no-op. Probably should
either not register the DRE switch or have setting it return an
error for these boost modes.

> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
> +			  struct completion *pll_lock)
>  {
>  	int ret;
> +	unsigned int gpio1;
>  
>  	switch (b_type) {
> +	case CS35L41_SHD_BOOST_ACTV:
> +	case CS35L41_SHD_BOOST_PASS:
> +		regmap_update_bits(regmap, CS35L41_PWR_CTRL3, CS35L41_SYNC_EN_MASK, 0);
> +
> +		gpio1 = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
> +		regmap_update_bits(regmap, CS35L41_GPIO_PAD_CONTROL, CS35L41_GPIO1_CTRL_MASK,
> +				   gpio1 << CS35L41_GPIO1_CTRL_SHIFT);
> +
> +		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
> +					 enable << CS35L41_GLOBAL_EN_SHIFT);
> +		usleep_range(3000, 3100);
> +		if (!enable)
> +			break;
> +
> +		if (!pll_lock)
> +			return -EINVAL;
> +
> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> +		if (ret == 0)
> +			ret = -ETIMEDOUT;

This feels kinda scary, in that you are relying on a 1 to 1
correspondence between this code running and getting a PLL lock
signal. The datasheet is helpfully completely vague on when PLL
locks are triggered.

The PLL enable seems to be set through set_sysclk, which could
be called multiple times, per DAPM power up.  Does the PLL
lock only go once global enable has been set? Can't help
but wonder if a reinit_completion should probably go somewhere
to ensure we are getting this lock of the PLL not a past one.

> @@ -483,6 +483,11 @@ 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);
> +	}
> +

If you fall into any of the error cases in this IRQ handler above
this, it will blat values you don't want into BST_EN although, to
be fair that does look currently broken for external boost as
well.

Thanks,
Charles

Re: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

On Tue, Feb 07, 2023 at 10:40:20AM +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.
> 

Probably would be nice to put at least a note to say based on
David's patches.

> +static const struct reg_sequence cs35l41_shd_boost_seq[] = {
> +	{CS35L41_PWR_CTRL3,	0x01000110},

This will blat whatever the user set in the DRE switch.
Technically blats the CLASS H enable from the DAPM widget too,
but as that always turns on should be a no-op. Probably should
either not register the DRE switch or have setting it return an
error for these boost modes.

> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
> +			  struct completion *pll_lock)
>  {
>  	int ret;
> +	unsigned int gpio1;
>  
>  	switch (b_type) {
> +	case CS35L41_SHD_BOOST_ACTV:
> +	case CS35L41_SHD_BOOST_PASS:
> +		regmap_update_bits(regmap, CS35L41_PWR_CTRL3, CS35L41_SYNC_EN_MASK, 0);
> +
> +		gpio1 = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
> +		regmap_update_bits(regmap, CS35L41_GPIO_PAD_CONTROL, CS35L41_GPIO1_CTRL_MASK,
> +				   gpio1 << CS35L41_GPIO1_CTRL_SHIFT);
> +
> +		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
> +					 enable << CS35L41_GLOBAL_EN_SHIFT);
> +		usleep_range(3000, 3100);
> +		if (!enable)
> +			break;
> +
> +		if (!pll_lock)
> +			return -EINVAL;
> +
> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> +		if (ret == 0)
> +			ret = -ETIMEDOUT;

This feels kinda scary, in that you are relying on a 1 to 1
correspondence between this code running and getting a PLL lock
signal. The datasheet is helpfully completely vague on when PLL
locks are triggered.

The PLL enable seems to be set through set_sysclk, which could
be called multiple times, per DAPM power up.  Does the PLL
lock only go once global enable has been set? Can't help
but wonder if a reinit_completion should probably go somewhere
to ensure we are getting this lock of the PLL not a past one.

> @@ -483,6 +483,11 @@ 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);
> +	}
> +

If you fall into any of the error cases in this IRQ handler above
this, it will blat values you don't want into BST_EN although, to
be fair that does look currently broken for external boost as
well.

Thanks,
Charles

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Lucas Tanure]

On 07-02-2023 10:42, Krzysztof Kozlowski wrote:
> On 07/02/2023 11:40, Lucas Tanure wrote:
>> Describe the properties used for shared boost
>> configuration.
> 
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
ack
> 
>>
>> Signed-off-by: Lucas Tanure <lucas.tanure@collabora.com>
>> ---
>>   .../devicetree/bindings/sound/cirrus,cs35l41.yaml     | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
>> index 18fb471aa891..6f5f01bec6f1 100644
>> --- a/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
>> +++ b/Documentation/devicetree/bindings/sound/cirrus,cs35l41.yaml
>> @@ -85,11 +85,20 @@ 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 = Reserved
> 
> How binding can be reserved? For what and why? Drop. 2 is shared active,
> 3 is shared passive.
2 Is shared boost without VSPK switch, a mode not supported for new 
system designs. But there is laptops using it, so we need to keep 
supporting in the driver.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature

[Lucas Tanure]

On 07-02-2023 11:48, Charles Keepax wrote:
> On Tue, Feb 07, 2023 at 10:40:20AM +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.
>>
> 
> Probably would be nice to put at least a note to say based on
> David's patches.
ack
> 
>> +static const struct reg_sequence cs35l41_shd_boost_seq[] = {
>> +	{CS35L41_PWR_CTRL3,	0x01000110},
> 
> This will blat whatever the user set in the DRE switch.
> Technically blats the CLASS H enable from the DAPM widget too,
> but as that always turns on should be a no-op. Probably should
> either not register the DRE switch or have setting it return an
> error for these boost modes.
Fixed in v2.
Changed to regmap_update_bits.
> 
>> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
>> +			  struct completion *pll_lock)
>>   {
>>   	int ret;
>> +	unsigned int gpio1;
>>   
>>   	switch (b_type) {
>> +	case CS35L41_SHD_BOOST_ACTV:
>> +	case CS35L41_SHD_BOOST_PASS:
>> +		regmap_update_bits(regmap, CS35L41_PWR_CTRL3, CS35L41_SYNC_EN_MASK, 0);
>> +
>> +		gpio1 = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
>> +		regmap_update_bits(regmap, CS35L41_GPIO_PAD_CONTROL, CS35L41_GPIO1_CTRL_MASK,
>> +				   gpio1 << CS35L41_GPIO1_CTRL_SHIFT);
>> +
>> +		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
>> +					 enable << CS35L41_GLOBAL_EN_SHIFT);
>> +		usleep_range(3000, 3100);
>> +		if (!enable)
>> +			break;
>> +
>> +		if (!pll_lock)
>> +			return -EINVAL;
>> +
>> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
>> +		if (ret == 0)
>> +			ret = -ETIMEDOUT;
> 
> This feels kinda scary, in that you are relying on a 1 to 1
> correspondence between this code running and getting a PLL lock
> signal. The datasheet is helpfully completely vague on when PLL
> locks are triggered.
> 
> The PLL enable seems to be set through set_sysclk, which could
> be called multiple times, per DAPM power up.  Does the PLL
> lock only go once global enable has been set? Can't help
> but wonder if a reinit_completion should probably go somewhere
> to ensure we are getting this lock of the PLL not a past one.
Added a reinit_completion at cs35l41_pcm_startup

> 
>> @@ -483,6 +483,11 @@ 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);
>> +	}
>> +
> 
> If you fall into any of the error cases in this IRQ handler above
> this, it will blat values you don't want into BST_EN although, to
> be fair that does look currently broken for external boost as
> well.
Fixed with a new patch in v2 series.

> 
> Thanks,
> Charles
> 

Re: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature

[Lucas Tanure]

On 07-02-2023 11:48, Charles Keepax wrote:
> On Tue, Feb 07, 2023 at 10:40:20AM +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.
>>
> 
> Probably would be nice to put at least a note to say based on
> David's patches.
ack
> 
>> +static const struct reg_sequence cs35l41_shd_boost_seq[] = {
>> +	{CS35L41_PWR_CTRL3,	0x01000110},
> 
> This will blat whatever the user set in the DRE switch.
> Technically blats the CLASS H enable from the DAPM widget too,
> but as that always turns on should be a no-op. Probably should
> either not register the DRE switch or have setting it return an
> error for these boost modes.
Fixed in v2.
Changed to regmap_update_bits.
> 
>> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
>> +			  struct completion *pll_lock)
>>   {
>>   	int ret;
>> +	unsigned int gpio1;
>>   
>>   	switch (b_type) {
>> +	case CS35L41_SHD_BOOST_ACTV:
>> +	case CS35L41_SHD_BOOST_PASS:
>> +		regmap_update_bits(regmap, CS35L41_PWR_CTRL3, CS35L41_SYNC_EN_MASK, 0);
>> +
>> +		gpio1 = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
>> +		regmap_update_bits(regmap, CS35L41_GPIO_PAD_CONTROL, CS35L41_GPIO1_CTRL_MASK,
>> +				   gpio1 << CS35L41_GPIO1_CTRL_SHIFT);
>> +
>> +		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
>> +					 enable << CS35L41_GLOBAL_EN_SHIFT);
>> +		usleep_range(3000, 3100);
>> +		if (!enable)
>> +			break;
>> +
>> +		if (!pll_lock)
>> +			return -EINVAL;
>> +
>> +		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
>> +		if (ret == 0)
>> +			ret = -ETIMEDOUT;
> 
> This feels kinda scary, in that you are relying on a 1 to 1
> correspondence between this code running and getting a PLL lock
> signal. The datasheet is helpfully completely vague on when PLL
> locks are triggered.
> 
> The PLL enable seems to be set through set_sysclk, which could
> be called multiple times, per DAPM power up.  Does the PLL
> lock only go once global enable has been set? Can't help
> but wonder if a reinit_completion should probably go somewhere
> to ensure we are getting this lock of the PLL not a past one.
Added a reinit_completion at cs35l41_pcm_startup

> 
>> @@ -483,6 +483,11 @@ 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);
>> +	}
>> +
> 
> If you fall into any of the error cases in this IRQ handler above
> this, it will blat values you don't want into BST_EN although, to
> be fair that does look currently broken for external boost as
> well.
Fixed with a new patch in v2 series.

> 
> Thanks,
> Charles
> 

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Krzysztof Kozlowski]

On 07/02/2023 16:46, 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
>>> +      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 = Reserved
>>
>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>> 3 is shared passive.
> 2 Is shared boost without VSPK switch, a mode not supported for new 
> system designs. But there is laptops using it, so we need to keep 
> supporting in the driver.

That's not the answer. 2 is nothing here, so it cannot be reserved.
Aren't you mixing now some register value with bindings?

Best regards,
Krzysztof

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Krzysztof Kozlowski]

On 07/02/2023 16:46, 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
>>> +      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 = Reserved
>>
>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>> 3 is shared passive.
> 2 Is shared boost without VSPK switch, a mode not supported for new 
> system designs. But there is laptops using it, so we need to keep 
> supporting in the driver.

That's not the answer. 2 is nothing here, so it cannot be reserved.
Aren't you mixing now some register value with bindings?

Best regards,
Krzysztof

_______________________________________________
Alsa-devel mailing list -- alsa-devel@alsa-project.org
To unsubscribe send an email to alsa-devel-leave@alsa-project.org

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Lucas Tanure]

On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
> On 07/02/2023 16:46, 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
>>>> +      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 = Reserved
>>>
>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>>> 3 is shared passive.
>> 2 Is shared boost without VSPK switch, a mode not supported for new
>> system designs. But there is laptops using it, so we need to keep
>> supporting in the driver.
> 
> That's not the answer. 2 is nothing here, so it cannot be reserved.
> Aren't you mixing now some register value with bindings?
> 
> Best regards,
> Krzysztof
> 
> 
I have added a new patch with propper documentation.
And I would like to use 3 and 4 for shared boost as 
CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the 
current driver.
The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the 
property "cirrus,boost-type", but to make everything consistent I would 
prefer to use 3 and 4 for the new boost types.
Is that ok with you?

Thanks
Lucas

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Krzysztof Kozlowski]

On 07/02/2023 17:34, Lucas Tanure wrote:
> On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
>> On 07/02/2023 16:46, 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
>>>>> +      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 = Reserved
>>>>
>>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>>>> 3 is shared passive.
>>> 2 Is shared boost without VSPK switch, a mode not supported for new
>>> system designs. But there is laptops using it, so we need to keep
>>> supporting in the driver.
>>
>> That's not the answer. 2 is nothing here, so it cannot be reserved.
>> Aren't you mixing now some register value with bindings?
>>
>> Best regards,
>> Krzysztof
>>
>>
> I have added a new patch with propper documentation.
> And I would like to use 3 and 4 for shared boost as 
> CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the 
> current driver.

I don't see CS35L41_EXT_BOOST_NO_VSPK_SWITCH in the bindings.

> The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the 
> property "cirrus,boost-type", but to make everything consistent I would 
> prefer to use 3 and 4 for the new boost types.
> Is that ok with you?

I don't see how it is related. The value does not exist, so whether
laptop has that property or not, is not really related, right?

Best regards,
Krzysztof

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Krzysztof Kozlowski]

On 07/02/2023 17:34, Lucas Tanure wrote:
> On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
>> On 07/02/2023 16:46, 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
>>>>> +      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 = Reserved
>>>>
>>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>>>> 3 is shared passive.
>>> 2 Is shared boost without VSPK switch, a mode not supported for new
>>> system designs. But there is laptops using it, so we need to keep
>>> supporting in the driver.
>>
>> That's not the answer. 2 is nothing here, so it cannot be reserved.
>> Aren't you mixing now some register value with bindings?
>>
>> Best regards,
>> Krzysztof
>>
>>
> I have added a new patch with propper documentation.
> And I would like to use 3 and 4 for shared boost as 
> CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the 
> current driver.

I don't see CS35L41_EXT_BOOST_NO_VSPK_SWITCH in the bindings.

> The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the 
> property "cirrus,boost-type", but to make everything consistent I would 
> prefer to use 3 and 4 for the new boost types.
> Is that ok with you?

I don't see how it is related. The value does not exist, so whether
laptop has that property or not, is not really related, right?

Best regards,
Krzysztof

_______________________________________________
Alsa-devel mailing list -- alsa-devel@alsa-project.org
To unsubscribe send an email to alsa-devel-leave@alsa-project.org

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[lucas.tanure]

On 2/7/23 4:48 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> On 07/02/2023 17:34, Lucas Tanure wrote:
> > On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
> >> On 07/02/2023 16:46, 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
> >>>>> +      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 = Reserved
> >>>>
> >>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
> >>>> 3 is shared passive.
> >>> 2 Is shared boost without VSPK switch, a mode not supported for new
> >>> system designs. But there is laptops using it, so we need to keep
> >>> supporting in the driver.
> >>
> >> That's not the answer. 2 is nothing here, so it cannot be reserved.
> >> Aren't you mixing now some register value with bindings?
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >>
> > I have added a new patch with propper documentation.
> > And I would like to use 3 and 4 for shared boost as
> > CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the
> > current driver.
> 
> I don't see CS35L41_EXT_BOOST_NO_VSPK_SWITCH in the bindings.
> 
> > The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the
> > property "cirrus,boost-type", but to make everything consistent I would
> > prefer to use 3 and 4 for the new boost types.
> > Is that ok with you?
> 
> I don't see how it is related. The value does not exist, so whether
> laptop has that property or not, is not really related, right?
> 
> Best regards,
> Krzysztof
> 
> 
The value does exist in the code, but no device should have that in ACPI/DTB, so yes the value doesn't exist for ACPI/DTB purposes.
I can change CS35L41_EXT_BOOST_NO_VSPK_SWITCH to another value, like 99, and use 2 and 3 for shared boost.
I will re-submit that with v3.
Is that ok with you?

Thanks
Lucas

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[lucas.tanure]

On 2/7/23 4:48 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> On 07/02/2023 17:34, Lucas Tanure wrote:
> > On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
> >> On 07/02/2023 16:46, 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
> >>>>> +      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 = Reserved
> >>>>
> >>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
> >>>> 3 is shared passive.
> >>> 2 Is shared boost without VSPK switch, a mode not supported for new
> >>> system designs. But there is laptops using it, so we need to keep
> >>> supporting in the driver.
> >>
> >> That's not the answer. 2 is nothing here, so it cannot be reserved.
> >> Aren't you mixing now some register value with bindings?
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >>
> > I have added a new patch with propper documentation.
> > And I would like to use 3 and 4 for shared boost as
> > CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the
> > current driver.
> 
> I don't see CS35L41_EXT_BOOST_NO_VSPK_SWITCH in the bindings.
> 
> > The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the
> > property "cirrus,boost-type", but to make everything consistent I would
> > prefer to use 3 and 4 for the new boost types.
> > Is that ok with you?
> 
> I don't see how it is related. The value does not exist, so whether
> laptop has that property or not, is not really related, right?
> 
> Best regards,
> Krzysztof
> 
> 
The value does exist in the code, but no device should have that in ACPI/DTB, so yes the value doesn't exist for ACPI/DTB purposes.
I can change CS35L41_EXT_BOOST_NO_VSPK_SWITCH to another value, like 99, and use 2 and 3 for shared boost.
I will re-submit that with v3.
Is that ok with you?

Thanks
Lucas

_______________________________________________
Alsa-devel mailing list -- alsa-devel@alsa-project.org
To unsubscribe send an email to alsa-devel-leave@alsa-project.org

Re: [PATCH 2/2] Documentation: cs35l41: Shared boost properties

[Krzysztof Kozlowski]

On 07/02/2023 18:03, lucas.tanure@collabora.com wrote:
> On 2/7/23 4:48 PM, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>> On 07/02/2023 17:34, Lucas Tanure wrote:
>>> On 07-02-2023 16:13, Krzysztof Kozlowski wrote:
>>>> On 07/02/2023 16:46, 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
>>>>>>> +      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 = Reserved
>>>>>>
>>>>>> How binding can be reserved? For what and why? Drop. 2 is shared active,
>>>>>> 3 is shared passive.
>>>>> 2 Is shared boost without VSPK switch, a mode not supported for new
>>>>> system designs. But there is laptops using it, so we need to keep
>>>>> supporting in the driver.
>>>>
>>>> That's not the answer. 2 is nothing here, so it cannot be reserved.
>>>> Aren't you mixing now some register value with bindings?
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>>
>>> I have added a new patch with propper documentation.
>>> And I would like to use 3 and 4 for shared boost as
>>> CS35L41_EXT_BOOST_NO_VSPK_SWITCH already exist as 2 and is used in the
>>> current driver.
>>
>> I don't see CS35L41_EXT_BOOST_NO_VSPK_SWITCH in the bindings.
>>
>>> The laptop that uses CS35L41_EXT_BOOST_NO_VSPK_SWITCH doesn't have the
>>> property "cirrus,boost-type", but to make everything consistent I would
>>> prefer to use 3 and 4 for the new boost types.
>>> Is that ok with you?
>>
>> I don't see how it is related. The value does not exist, so whether
>> laptop has that property or not, is not really related, right?
>>
>> Best regards,
>> Krzysztof
>>
>>
> The value does exist in the code, but no device should have that in ACPI/DTB, so yes the value doesn't exist for ACPI/DTB purposes.
> I can change CS35L41_EXT_BOOST_NO_VSPK_SWITCH to another value, like 99, and use 2 and 3 for shared boost.
> I will re-submit that with v3.
> Is that ok with you?

I guess we still talk about different things. The code does not have a
binding for the boost, therefore it does not use boost binding. Whatever
it does with CS35L41_EXT_BOOST_NO_VSPK_SWITCH outside of DT, is not my
topic and I don't care.

That's why I asked folks to use strings for such enumerations, not
register values - to avoid any confusion between the code and bindings
(and also make it more readable for humans).

Best regards,
Krzysztof

Re: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature

[kernel test robot]

Hi Lucas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on tiwai-sound/for-next tiwai-sound/for-linus linus/master v6.2-rc7 next-20230208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Lucas-Tanure/ALSA-cs35l41-Add-shared-boost-feature/20230207-184238
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
patch link:    https://lore.kernel.org/r/20230207104021.2842-2-lucas.tanure%40collabora.com
patch subject: [PATCH 1/2] ALSA: cs35l41: Add shared boost feature
config: i386-randconfig-a011 (https://download.01.org/0day-ci/archive/20230208/202302081911.MDwfUTfx-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/c1726800667180cd46986c3578e635bafa8bf01a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Lucas-Tanure/ALSA-cs35l41-Add-shared-boost-feature/20230207-184238
        git checkout c1726800667180cd46986c3578e635bafa8bf01a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash sound/soc/codecs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> sound/soc/codecs/cs35l41-lib.c:1160:7: warning: variable 'ret' is used uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
           case CS35L41_SHD_BOOST_PASS:
                ^~~~~~~~~~~~~~~~~~~~~~
   sound/soc/codecs/cs35l41-lib.c:1169:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   sound/soc/codecs/cs35l41-lib.c:1136:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +/ret +1160 sound/soc/codecs/cs35l41-lib.c

  1132	
  1133	int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
  1134			       struct cs35l41_hw_cfg *hw_cfg)
  1135	{
  1136		int ret;
  1137	
  1138		switch (hw_cfg->bst_type) {
  1139		case CS35L41_SHD_BOOST_ACTV:
  1140			regmap_multi_reg_write(regmap, cs35l41_actv_seq, ARRAY_SIZE(cs35l41_actv_seq));
  1141			fallthrough;
  1142		case CS35L41_INT_BOOST:
  1143			ret = cs35l41_boost_config(dev, regmap, hw_cfg->bst_ind,
  1144						   hw_cfg->bst_cap, hw_cfg->bst_ipk);
  1145			if (ret)
  1146				dev_err(dev, "Error in Boost DT config: %d\n", ret);
  1147			break;
  1148		case CS35L41_EXT_BOOST:
  1149		case CS35L41_EXT_BOOST_NO_VSPK_SWITCH:
  1150			/* Only CLSA0100 doesn't use GPIO as VSPK switch, but even on that laptop we can
  1151			 * toggle GPIO1 as is not connected to anything.
  1152			 * There will be no other device without VSPK switch.
  1153			 */
  1154			regmap_write(regmap, CS35L41_GPIO1_CTRL1, 0x00000001);
  1155			regmap_multi_reg_write(regmap, cs35l41_reset_to_safe,
  1156					       ARRAY_SIZE(cs35l41_reset_to_safe));
  1157			ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL2, CS35L41_BST_EN_MASK,
  1158						 CS35L41_BST_DIS_FET_OFF << CS35L41_BST_EN_SHIFT);
  1159			break;
> 1160		case CS35L41_SHD_BOOST_PASS:
  1161			regmap_multi_reg_write(regmap, cs35l41_pass_seq, ARRAY_SIZE(cs35l41_pass_seq));
  1162			break;
  1163		default:
  1164			dev_err(dev, "Boost type %d not supported\n", hw_cfg->bst_type);
  1165			ret = -EINVAL;
  1166			break;
  1167		}
  1168	
  1169		return ret;
  1170	}
  1171	EXPORT_SYMBOL_GPL(cs35l41_init_boost);
  1172	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests