[PATCH 0/9] Improve CS35l41 ALSA SoC audio driver

[PATCH 0/9] Improve CS35l41 ALSA SoC audio driver

[Cristian Ciocaltea]

This patch series contains several fixes and improvements to 
the CS35l41 audio codec driver.

It has been verified on Valve's Steam Deck.

Cristian Ciocaltea (9):
  ASoC: cs35l41: Handle mdsync_down reg write errors
  ASoC: cs35l41: Handle mdsync_up reg write errors
  ASoC: cs35l41: Initialize completion object before requesting IRQ
  ASoC: cs35l41: Fix broken shared boost activation
  ASoC: cs35l41: Rename pll_lock to pll_lock_done
  ASoC: cs35l41: Make use of dev_err_probe()
  ASoC: cs35l41: Verify PM runtime resume errors in IRQ handler
  ASoC: cs35l41: Use modern pm_ops
  ASoC: cs35l41: Use devm_pm_runtime_enable()

 include/sound/cs35l41.h        |   5 +-
 sound/soc/codecs/cs35l41-i2c.c |  11 ++-
 sound/soc/codecs/cs35l41-lib.c |  83 ++++++++++++++++------
 sound/soc/codecs/cs35l41-spi.c |  11 ++-
 sound/soc/codecs/cs35l41.c     | 121 ++++++++++++++++++++++-----------
 sound/soc/codecs/cs35l41.h     |   4 +-
 6 files changed, 158 insertions(+), 77 deletions(-)

-- 
2.41.0

[PATCH 1/9] ASoC: cs35l41: Handle mdsync_down reg write errors

[Cristian Ciocaltea]

The return code of regmap_multi_reg_write() call related to "MDSYNC
down" sequence is shadowed by the subsequent
wait_for_completion_timeout() invocation, which is expected to time
timeout in case the write operation failed.

Let cs35l41_global_enable() return the correct error code instead of
-ETIMEDOUT.

Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/codecs/cs35l41-lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index 4ec306cd2f47..a018f1d98428 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -1243,7 +1243,7 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 		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)
+		if (ret || !enable)
 			break;
 
 		if (!pll_lock)
-- 
2.41.0

Re: [PATCH 1/9] ASoC: cs35l41: Handle mdsync_down reg write errors

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:13AM +0300, Cristian Ciocaltea wrote:
> The return code of regmap_multi_reg_write() call related to "MDSYNC
> down" sequence is shadowed by the subsequent
> wait_for_completion_timeout() invocation, which is expected to time
> timeout in case the write operation failed.
> 
> Let cs35l41_global_enable() return the correct error code instead of
> -ETIMEDOUT.
> 
> Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  sound/soc/codecs/cs35l41-lib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
> index 4ec306cd2f47..a018f1d98428 100644
> --- a/sound/soc/codecs/cs35l41-lib.c
> +++ b/sound/soc/codecs/cs35l41-lib.c
> @@ -1243,7 +1243,7 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
>  		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)
> +		if (ret || !enable)
>  			break;

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

Thanks,
Charles

[PATCH 2/9] ASoC: cs35l41: Handle mdsync_up reg write errors

[Cristian Ciocaltea]

The return code of regmap_multi_reg_write() call related to "MDSYNC up"
sequence is shadowed by the subsequent regmap_read_poll_timeout()
invocation, which will hit a timeout in case the write operation above
fails.

Make sure cs35l41_global_enable() returns the correct error code instead
of -ETIMEDOUT.

Additionally, to be able to distinguish between the timeouts of
wait_for_completion_timeout() and regmap_read_poll_timeout(), print an
error message for the former and return immediately.  This also avoids
having to wait unnecessarily for the second time.

Fixes: f8264c759208 ("ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed delay")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/codecs/cs35l41-lib.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index a018f1d98428..a6c6bb23b957 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -1251,15 +1251,18 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 
 		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));
+			dev_err(dev, "Timed out waiting for pll_lock\n");
+			return -ETIMEDOUT;
 		}
 
+		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
+		pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
+		cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
+		ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
+					     ARRAY_SIZE(cs35l41_mdsync_up_seq));
+		if (ret)
+			return ret;
+
 		ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1,
 					int_status, int_status & pup_pdn_mask,
 					1000, 100000);
-- 
2.41.0

Re: [PATCH 2/9] ASoC: cs35l41: Handle mdsync_up reg write errors

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:14AM +0300, Cristian Ciocaltea wrote:
> The return code of regmap_multi_reg_write() call related to "MDSYNC up"
> sequence is shadowed by the subsequent regmap_read_poll_timeout()
> invocation, which will hit a timeout in case the write operation above
> fails.
> 
> Make sure cs35l41_global_enable() returns the correct error code instead
> of -ETIMEDOUT.
> 
> Additionally, to be able to distinguish between the timeouts of
> wait_for_completion_timeout() and regmap_read_poll_timeout(), print an
> error message for the former and return immediately.  This also avoids
> having to wait unnecessarily for the second time.
> 
> Fixes: f8264c759208 ("ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed delay")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

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

Thanks,
Charles

[PATCH 3/9] ASoC: cs35l41: Initialize completion object before requesting IRQ

[Cristian Ciocaltea]

Technically, an interrupt handler can be called before probe() finishes
its execution, hence ensure the pll_lock completion object is always
initialized before being accessed in cs35l41_irq().

Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/codecs/cs35l41.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index 722b69a6de26..fe5376b3e01b 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -1273,6 +1273,8 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 		regmap_update_bits(cs35l41->regmap, CS35L41_IRQ1_MASK3, CS35L41_INT3_PLL_LOCK_MASK,
 				   0 << CS35L41_INT3_PLL_LOCK_SHIFT);
 
+	init_completion(&cs35l41->pll_lock);
+
 	ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL, cs35l41_irq,
 					IRQF_ONESHOT | IRQF_SHARED | irq_pol,
 					"cs35l41", cs35l41);
@@ -1295,8 +1297,6 @@ 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);
-- 
2.41.0

Re: [PATCH 3/9] ASoC: cs35l41: Initialize completion object before requesting IRQ

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:15AM +0300, Cristian Ciocaltea wrote:
> Technically, an interrupt handler can be called before probe() finishes
> its execution, hence ensure the pll_lock completion object is always
> initialized before being accessed in cs35l41_irq().
> 
> Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

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

Thanks,
Charles

[PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

[Cristian Ciocaltea]

Enabling the active/passive shared boosts involves writing the MDSYNC UP
register sequence, which cannot be performed before receiving the PLL
lock signal.

Due to improper error handling, it was not obvious the wait operation
times out and, consequently, the shared boost gets never enabled.

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

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

Solve the issue by deferring the processing to a workqueue task, which
allows to correctly wait for the signal and then safely proceed with
the required regmap operations.

Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 include/sound/cs35l41.h        |  5 +-
 sound/soc/codecs/cs35l41-lib.c | 86 +++++++++++++++++++++++++---------
 sound/soc/codecs/cs35l41.c     | 38 ++++++++++++++-
 sound/soc/codecs/cs35l41.h     |  2 +
 4 files changed, 104 insertions(+), 27 deletions(-)

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index 1bf757901d02..91024411f8a1 100644
--- a/include/sound/cs35l41.h
+++ b/include/sound/cs35l41.h
@@ -11,7 +11,7 @@
 #define __CS35L41_H
 
 #include <linux/regmap.h>
-#include <linux/completion.h>
+#include <linux/workqueue.h>
 #include <linux/firmware/cirrus/cs_dsp.h>
 
 #define CS35L41_FIRSTREG		0x00000000
@@ -902,7 +902,8 @@ 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_mdsync_up(struct regmap *regmap);
 int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type,
-			  int enable, struct completion *pll_lock, bool firmware_running);
+			  int enable, struct work_struct *mdsync_up_work, bool firmware_running);
 
 #endif /* __CS35L41_H */
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index a6c6bb23b957..16e7f46d3853 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -1192,8 +1192,56 @@ bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type)
 }
 EXPORT_SYMBOL_GPL(cs35l41_safe_reset);
 
+int cs35l41_mdsync_up(struct regmap *regmap)
+{
+	struct reg_sequence cs35l41_mdsync_up_seq[] = {
+		{CS35L41_PWR_CTRL3, 0},
+		{CS35L41_PWR_CTRL1, 0x00000000, 3000},
+		{CS35L41_PWR_CTRL1, 0x00000001, 3000},
+	};
+	unsigned int pwr_ctrl3, int_status;
+	int ret;
+
+	regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
+	pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
+	cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
+
+	ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
+				     ARRAY_SIZE(cs35l41_mdsync_up_seq));
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1, int_status,
+				       int_status & CS35L41_PUP_DONE_MASK,
+				       1000, 100000);
+
+	/* Clear PUP/PDN status */
+	regmap_write(regmap, CS35L41_IRQ1_STATUS1, CS35L41_PUP_DONE_MASK);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cs35l41_mdsync_up);
+
+/*
+ * NOTE: Enabling the CS35L41_SHD_BOOST_ACTV and CS35L41_SHD_BOOST_PASS shared
+ * boosts involves writing the MDSYNC UP reg sequence, which must be performed
+ * only after getting the PLL lock signal. This signal seems to be triggered
+ * soon after snd_pcm_start() is executed and SNDRV_PCM_TRIGGER_START command
+ * is processed, which happens (long) after the SND_SOC_DAPM_PRE_PMU event
+ * handler is invoked as part of snd_pcm_prepare().
+ *
+ * The event handler is where cs35l41_global_enable() should be normally called
+ * from, but waiting for the PLL lock signal here will time out. Increasing the
+ * wait duration will not help, as the only consequence of that would be to add
+ * an unnecessary delay in the invocation of snd_pcm_start().
+ *
+ * Trying to move the wait in the SNDRV_PCM_TRIGGER_START callback is not a
+ * solution either, as the trigger is executed in an IRQ-off atomic context,
+ * hence the current approach is to defer the processing to a workqueue task.
+ * This allows to properly wait for the signal and then safely write the
+ * necessary MDSYNC sequence by calling cs35l41_mdsync_up() above.
+ */
 int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type,
-			  int enable, struct completion *pll_lock, bool firmware_running)
+			  int enable, struct work_struct *mdsync_up_work, bool firmware_running)
 {
 	int ret;
 	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3, int_status, pup_pdn_mask;
@@ -1203,11 +1251,6 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 		{CS35L41_GPIO_PAD_CONTROL,	0},
 		{CS35L41_PWR_CTRL1,		0, 3000},
 	};
-	struct reg_sequence cs35l41_mdsync_up_seq[] = {
-		{CS35L41_PWR_CTRL3,	0},
-		{CS35L41_PWR_CTRL1,	0x00000000, 3000},
-		{CS35L41_PWR_CTRL1,	0x00000001, 3000},
-	};
 
 	pup_pdn_mask = enable ? CS35L41_PUP_DONE_MASK : CS35L41_PDN_DONE_MASK;
 
@@ -1226,6 +1269,9 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 	switch (b_type) {
 	case CS35L41_SHD_BOOST_ACTV:
 	case CS35L41_SHD_BOOST_PASS:
+		if (mdsync_up_work)
+			cancel_work_sync(mdsync_up_work);
+
 		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
 		regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
 
@@ -1243,33 +1289,27 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 		cs35l41_mdsync_down_seq[2].def = pwr_ctrl1;
 		ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_down_seq,
 					     ARRAY_SIZE(cs35l41_mdsync_down_seq));
-		if (ret || !enable)
+		if (ret)
 			break;
 
-		if (!pll_lock)
-			return -EINVAL;
-
-		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
-		if (ret == 0) {
-			dev_err(dev, "Timed out waiting for pll_lock\n");
-			return -ETIMEDOUT;
+		if (enable) {
+			if (mdsync_up_work) {
+				/* Call cs35l41_mdsync_up() after receiving PLL lock signal */
+				schedule_work(mdsync_up_work);
+			} else {
+				dev_err(dev, "MDSYNC UP work not provided\n");
+				ret = -EINVAL;
+			}
+			break;
 		}
 
-		regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
-		pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
-		cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
-		ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
-					     ARRAY_SIZE(cs35l41_mdsync_up_seq));
-		if (ret)
-			return ret;
-
 		ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1,
 					int_status, int_status & pup_pdn_mask,
 					1000, 100000);
 		if (ret)
 			dev_err(dev, "Enable(%d) failed: %d\n", enable, ret);
 
-		// Clear PUP/PDN status
+		/* Clear PUP/PDN status */
 		regmap_write(regmap, CS35L41_IRQ1_STATUS1, pup_pdn_mask);
 		break;
 	case CS35L41_INT_BOOST:
diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index fe5376b3e01b..9bf70da03972 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -500,11 +500,11 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w,
 						ARRAY_SIZE(cs35l41_pup_patch));
 
 		ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
-					    1, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
+					    1, &cs35l41->mdsync_up_work, cs35l41->dsp.cs_dsp.running);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
 		ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
-					    0, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
+					    0, &cs35l41->mdsync_up_work, cs35l41->dsp.cs_dsp.running);
 
 		regmap_multi_reg_write_bypassed(cs35l41->regmap,
 						cs35l41_pdn_patch,
@@ -1155,6 +1155,36 @@ static int cs35l41_acpi_get_name(struct cs35l41_private *cs35l41)
 	return 0;
 }
 
+static void cs35l41_mdsync_up_work(struct work_struct *work)
+{
+	struct cs35l41_private *cs35l41 = container_of(work,
+						       struct cs35l41_private,
+						       mdsync_up_work);
+	int ret = wait_for_completion_timeout(&cs35l41->pll_lock,
+					      msecs_to_jiffies(100));
+	if (ret == 0) {
+		dev_err(cs35l41->dev, "Timed out waiting for pll_lock signal\n");
+		return;
+	}
+
+	dev_dbg(cs35l41->dev, "Received pll_lock signal\n");
+
+	ret = pm_runtime_resume_and_get(cs35l41->dev);
+	if (ret < 0) {
+		dev_err(cs35l41->dev,
+			"pm_runtime_resume_and_get failed in %s: %d\n",
+			__func__, ret);
+		return;
+	}
+
+	ret = cs35l41_mdsync_up(cs35l41->regmap);
+	if (ret < 0)
+		dev_err(cs35l41->dev, "MDSYNC UP failed: %d\n", ret);
+
+	pm_runtime_mark_last_busy(cs35l41->dev);
+	pm_runtime_put_autosuspend(cs35l41->dev);
+}
+
 int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *hw_cfg)
 {
 	u32 regid, reg_revid, i, mtl_revid, int_status, chipid_match;
@@ -1297,6 +1327,8 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 	if (ret < 0)
 		goto err;
 
+	INIT_WORK(&cs35l41->mdsync_up_work, cs35l41_mdsync_up_work);
+
 	pm_runtime_set_autosuspend_delay(cs35l41->dev, 3000);
 	pm_runtime_use_autosuspend(cs35l41->dev);
 	pm_runtime_mark_last_busy(cs35l41->dev);
@@ -1335,6 +1367,8 @@ EXPORT_SYMBOL_GPL(cs35l41_probe);
 
 void cs35l41_remove(struct cs35l41_private *cs35l41)
 {
+	cancel_work_sync(&cs35l41->mdsync_up_work);
+
 	pm_runtime_get_sync(cs35l41->dev);
 	pm_runtime_disable(cs35l41->dev);
 
diff --git a/sound/soc/codecs/cs35l41.h b/sound/soc/codecs/cs35l41.h
index 34d967d4372b..f9f85796a13a 100644
--- a/sound/soc/codecs/cs35l41.h
+++ b/sound/soc/codecs/cs35l41.h
@@ -13,6 +13,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/firmware.h>
+#include <linux/completion.h>
 #include <sound/core.h>
 #include <sound/cs35l41.h>
 
@@ -34,6 +35,7 @@ struct cs35l41_private {
 	/* GPIO for /RST */
 	struct gpio_desc *reset_gpio;
 	struct completion pll_lock;
+	struct work_struct mdsync_up_work;
 };
 
 int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *hw_cfg);
-- 
2.41.0

Re: [PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:
> Enabling the active/passive shared boosts involves writing the MDSYNC UP
> register sequence, which cannot be performed before receiving the PLL
> lock signal.
> 
> Due to improper error handling, it was not obvious the wait operation
> times out and, consequently, the shared boost gets never enabled.
> 
> Further investigations revealed the signal is triggered while
> snd_pcm_start() is executed, right after receiving the
> SNDRV_PCM_TRIGGER_START command, which happens long after the
> SND_SOC_DAPM_PRE_PMU event handler is invoked as part of
> snd_pcm_prepare().  That is where cs35l41_global_enable() is called
> from.
> 
> Increasing the wait duration doesn't help, as it only causes an
> unnecessary delay in the invocation of snd_pcm_start().  Moving the wait
> and the subsequent regmap operations to the SNDRV_PCM_TRIGGER_START
> callback is not a solution either, since they would be executed in an
> IRQ-off atomic context.
> 
> Solve the issue by deferring the processing to a workqueue task, which
> allows to correctly wait for the signal and then safely proceed with
> the required regmap operations.
> 
> Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

Thanks for looking at this apologies this was missed in the
initial review of the patch.

> +int cs35l41_mdsync_up(struct regmap *regmap)
> +{
> +	struct reg_sequence cs35l41_mdsync_up_seq[] = {
> +		{CS35L41_PWR_CTRL3, 0},
> +		{CS35L41_PWR_CTRL1, 0x00000000, 3000},
> +		{CS35L41_PWR_CTRL1, 0x00000001, 3000},
> +	};
> +	unsigned int pwr_ctrl3, int_status;
> +	int ret;
> +
> +	regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
> +	pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
> +	cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
> +
> +	ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
> +				     ARRAY_SIZE(cs35l41_mdsync_up_seq));
> +	if (ret < 0)
> +		return ret;

Is this now safe? By pulling this out into a worker thread, it is
no longer under the DAPM lock, which makes me worry this can race
with the other uses of PWR_CTRL3 which could theoretically change
state between when you read the reg and when you write it.

> @@ -1243,33 +1289,27 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
>  		cs35l41_mdsync_down_seq[2].def = pwr_ctrl1;
>  		ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_down_seq,
>  					     ARRAY_SIZE(cs35l41_mdsync_down_seq));
> -		if (ret || !enable)
> +		if (ret)
>  			break;
>  
> -		if (!pll_lock)
> -			return -EINVAL;
> -
> -		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> -		if (ret == 0) {
> -			dev_err(dev, "Timed out waiting for pll_lock\n");
> -			return -ETIMEDOUT;
> +		if (enable) {
> +			if (mdsync_up_work) {
> +				/* Call cs35l41_mdsync_up() after receiving PLL lock signal */
> +				schedule_work(mdsync_up_work);
> +			} else {
> +				dev_err(dev, "MDSYNC UP work not provided\n");
> +				ret = -EINVAL;
> +			}
> +			break;

One question I might also have would be does a worker thread make
more sense or would it be simpler to do the mdsync power up
directly in response to the PLL lock IRQ?

Thanks,
Charles

Re: [PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

[Rhodes, David]

On 9/5/23 5:29 AM, Charles Keepax wrote:
> On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:
>> Enabling the active/passive shared boosts involves writing the MDSYNC UP
>> register sequence, which cannot be performed before receiving the PLL
>> lock signal.
>>

> 
> Thanks for looking at this apologies this was missed in the
> initial review of the patch.
> 

Thanks Cristian, I agree with the intent of your patch.
We do not expect that clocks are always available before the DAPM PMU 
event and shared boost should still be configured if they are not.

>> +int cs35l41_mdsync_up(struct regmap *regmap)
>> +{
>> +	struct reg_sequence cs35l41_mdsync_up_seq[] = {
>> +		{CS35L41_PWR_CTRL3, 0},
>> +		{CS35L41_PWR_CTRL1, 0x00000000, 3000},
>> +		{CS35L41_PWR_CTRL1, 0x00000001, 3000},
>> +	};

I don't know why PWR_CTRL1 is included in the up sequence here.
This toggles GLOBAL_EN, which will cause the PLL to unlock and lock 
again. Doing this defeats the purpose of setting SYNC_EN in a separate 
operation, which is to only do so when the amp is powered on and has 
locked the PLL. GLOBAL_EN is set by the mdsync_down_seq, so all that is 
needed when the PLL is locked is to set SYNC_EN.

> 
> Is this now safe? By pulling this out into a worker thread, it is
> no longer under the DAPM lock, which makes me worry this can race
> with the other uses of PWR_CTRL3 which could theoretically change
> state between when you read the reg and when you write it.
> 

The Class-H DAPM widget also uses the PWR_CTRL3 register.

> 
> One question I might also have would be does a worker thread make
> more sense or would it be simpler to do the mdsync power up
> directly in response to the PLL lock IRQ?
> 

I  agree with implementing this in the PLL lock IRQ.
As I described above, all that would need to be done is to set SYNC_EN 
in the PLL Lock IRQ handler.

Thanks,
David

Re: [PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

[Cristian Ciocaltea]

On 9/5/23 21:11, Rhodes, David wrote:
> On 9/5/23 5:29 AM, Charles Keepax wrote:
>> On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:
>>> Enabling the active/passive shared boosts involves writing the MDSYNC UP
>>> register sequence, which cannot be performed before receiving the PLL
>>> lock signal.
>>>
> 
>>
>> Thanks for looking at this apologies this was missed in the
>> initial review of the patch.
>>
> 
> Thanks Cristian, I agree with the intent of your patch.
> We do not expect that clocks are always available before the DAPM PMU
> event and shared boost should still be configured if they are not.
> 
>>> +int cs35l41_mdsync_up(struct regmap *regmap)
>>> +{
>>> +    struct reg_sequence cs35l41_mdsync_up_seq[] = {
>>> +        {CS35L41_PWR_CTRL3, 0},
>>> +        {CS35L41_PWR_CTRL1, 0x00000000, 3000},
>>> +        {CS35L41_PWR_CTRL1, 0x00000001, 3000},
>>> +    };
> 
> I don't know why PWR_CTRL1 is included in the up sequence here.
> This toggles GLOBAL_EN, which will cause the PLL to unlock and lock
> again. Doing this defeats the purpose of setting SYNC_EN in a separate
> operation, which is to only do so when the amp is powered on and has
> locked the PLL. GLOBAL_EN is set by the mdsync_down_seq, so all that is
> needed when the PLL is locked is to set SYNC_EN.

Unfortunately I had to rely on the existing implementation as I don't 
have access to the datasheet.

If I got it right, we should drop all write operations on PWR_CTRL1, 
and simply set the CS35L41_SYNC_EN_MASK bit in PWR_CTRL3.
 
>>
>> Is this now safe? By pulling this out into a worker thread, it is
>> no longer under the DAPM lock, which makes me worry this can race
>> with the other uses of PWR_CTRL3 which could theoretically change
>> state between when you read the reg and when you write it.

That's a good point, it should be fixed implicitly by replacing the 
read/write operations with a single regmap_update_bits() call, which 
is protected by regmap's internal lock.

> 
> The Class-H DAPM widget also uses the PWR_CTRL3 register.
> 
>>
>> One question I might also have would be does a worker thread make
>> more sense or would it be simpler to do the mdsync power up
>> directly in response to the PLL lock IRQ?
>>
> 
> I  agree with implementing this in the PLL lock IRQ.
> As I described above, all that would need to be done is to set SYNC_EN
> in the PLL Lock IRQ handler.

As a matter of fact I initially considered doing this in the IRQ handler, 
but I also wanted to understand the reasoning behind current solution. 
Therefore I searched the ML archive for any relevant review comments, 
and I came across [1] which raises some concerns regarding the PLL lock 
signal, e.g. lack of documentation regarding trigger time & frequency.

If going with the IRQ handler approach is still the recommended approach,
I will handle it in v2.

Thanks for reviewing,
Cristian

[1] https://lore.kernel.org/all/20230207114855.GC36097@ediswmail.ad.cirrus.com/

Re: [PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

[Rhodes, David]

On 9/5/23 3:05 PM, Cristian Ciocaltea wrote:
> If I got it right, we should drop all write operations on PWR_CTRL1,
> and simply set the CS35L41_SYNC_EN_MASK bit in PWR_CTRL3.

> That's a good point, it should be fixed implicitly by replacing the
> read/write operations with a single regmap_update_bits() call, which
> is protected by regmap's internal lock.
> 

Yes, my recommendation is to replace the mdsync_up_sequence and 
completion handling with a single regmap_update_bits() call to set the 
CS35L41_SYNC_EN_MASK bit in PWR_CTRL3, which occurs in response to the 
PLL Lock interrupt.

Thanks,
David

[PATCH 5/9] ASoC: cs35l41: Rename pll_lock to pll_lock_done

[Cristian Ciocaltea]

Use a more intuitive name for 'pll_lock' completion, which helps
improving code readability a bit.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/codecs/cs35l41.c | 8 ++++----
 sound/soc/codecs/cs35l41.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index 9bf70da03972..e143b0e306b1 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -459,7 +459,7 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 
 	if (status[2] & CS35L41_PLL_LOCK) {
 		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS3, CS35L41_PLL_LOCK);
-		complete(&cs35l41->pll_lock);
+		complete(&cs35l41->pll_lock_done);
 		ret = IRQ_HANDLED;
 	}
 
@@ -804,7 +804,7 @@ static int cs35l41_pcm_startup(struct snd_pcm_substream *substream,
 {
 	struct cs35l41_private *cs35l41 = snd_soc_component_get_drvdata(dai->component);
 
-	reinit_completion(&cs35l41->pll_lock);
+	reinit_completion(&cs35l41->pll_lock_done);
 
 	if (substream->runtime)
 		return snd_pcm_hw_constraint_list(substream->runtime, 0,
@@ -1160,7 +1160,7 @@ static void cs35l41_mdsync_up_work(struct work_struct *work)
 	struct cs35l41_private *cs35l41 = container_of(work,
 						       struct cs35l41_private,
 						       mdsync_up_work);
-	int ret = wait_for_completion_timeout(&cs35l41->pll_lock,
+	int ret = wait_for_completion_timeout(&cs35l41->pll_lock_done,
 					      msecs_to_jiffies(100));
 	if (ret == 0) {
 		dev_err(cs35l41->dev, "Timed out waiting for pll_lock signal\n");
@@ -1303,7 +1303,7 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 		regmap_update_bits(cs35l41->regmap, CS35L41_IRQ1_MASK3, CS35L41_INT3_PLL_LOCK_MASK,
 				   0 << CS35L41_INT3_PLL_LOCK_SHIFT);
 
-	init_completion(&cs35l41->pll_lock);
+	init_completion(&cs35l41->pll_lock_done);
 
 	ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL, cs35l41_irq,
 					IRQF_ONESHOT | IRQF_SHARED | irq_pol,
diff --git a/sound/soc/codecs/cs35l41.h b/sound/soc/codecs/cs35l41.h
index f9f85796a13a..fe61c11404e7 100644
--- a/sound/soc/codecs/cs35l41.h
+++ b/sound/soc/codecs/cs35l41.h
@@ -34,7 +34,7 @@ struct cs35l41_private {
 	int irq;
 	/* GPIO for /RST */
 	struct gpio_desc *reset_gpio;
-	struct completion pll_lock;
+	struct completion pll_lock_done;
 	struct work_struct mdsync_up_work;
 };
 
-- 
2.41.0

Re: [PATCH 5/9] ASoC: cs35l41: Rename pll_lock to pll_lock_done

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:17AM +0300, Cristian Ciocaltea wrote:
> Use a more intuitive name for 'pll_lock' completion, which helps
> improving code readability a bit.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

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

Thanks,
Charles

[PATCH 6/9] ASoC: cs35l41: Make use of dev_err_probe()

[Cristian Ciocaltea]

Use dev_err_probe() helper where possible, to simplify error handling
during probe.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/codecs/cs35l41-i2c.c |  9 +++------
 sound/soc/codecs/cs35l41-spi.c |  9 +++------
 sound/soc/codecs/cs35l41.c     | 34 ++++++++++++++++------------------
 3 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/sound/soc/codecs/cs35l41-i2c.c b/sound/soc/codecs/cs35l41-i2c.c
index 7ea890d7d387..9109203a7f25 100644
--- a/sound/soc/codecs/cs35l41-i2c.c
+++ b/sound/soc/codecs/cs35l41-i2c.c
@@ -35,7 +35,6 @@ static int cs35l41_i2c_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct cs35l41_hw_cfg *hw_cfg = dev_get_platdata(dev);
 	const struct regmap_config *regmap_config = &cs35l41_regmap_i2c;
-	int ret;
 
 	cs35l41 = devm_kzalloc(dev, sizeof(struct cs35l41_private), GFP_KERNEL);
 
@@ -47,11 +46,9 @@ static int cs35l41_i2c_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, cs35l41);
 	cs35l41->regmap = devm_regmap_init_i2c(client, regmap_config);
-	if (IS_ERR(cs35l41->regmap)) {
-		ret = PTR_ERR(cs35l41->regmap);
-		dev_err(cs35l41->dev, "Failed to allocate register map: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(cs35l41->regmap))
+		return dev_err_probe(cs35l41->dev, PTR_ERR(cs35l41->regmap),
+				     "Failed to allocate register map\n");
 
 	return cs35l41_probe(cs35l41, hw_cfg);
 }
diff --git a/sound/soc/codecs/cs35l41-spi.c b/sound/soc/codecs/cs35l41-spi.c
index 5c8bb24909eb..28e9c9473e60 100644
--- a/sound/soc/codecs/cs35l41-spi.c
+++ b/sound/soc/codecs/cs35l41-spi.c
@@ -32,7 +32,6 @@ static int cs35l41_spi_probe(struct spi_device *spi)
 	const struct regmap_config *regmap_config = &cs35l41_regmap_spi;
 	struct cs35l41_hw_cfg *hw_cfg = dev_get_platdata(&spi->dev);
 	struct cs35l41_private *cs35l41;
-	int ret;
 
 	cs35l41 = devm_kzalloc(&spi->dev, sizeof(struct cs35l41_private), GFP_KERNEL);
 	if (!cs35l41)
@@ -43,11 +42,9 @@ static int cs35l41_spi_probe(struct spi_device *spi)
 
 	spi_set_drvdata(spi, cs35l41);
 	cs35l41->regmap = devm_regmap_init_spi(spi, regmap_config);
-	if (IS_ERR(cs35l41->regmap)) {
-		ret = PTR_ERR(cs35l41->regmap);
-		dev_err(&spi->dev, "Failed to allocate register map: %d\n", ret);
-		return ret;
-	}
+	if (IS_ERR(cs35l41->regmap))
+		return dev_err_probe(cs35l41->dev, PTR_ERR(cs35l41->regmap),
+				     "Failed to allocate register map\n");
 
 	cs35l41->dev = &spi->dev;
 	cs35l41->irq = spi->irq;
diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index e143b0e306b1..6f2ad0d3a75c 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -1204,16 +1204,14 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 
 	ret = devm_regulator_bulk_get(cs35l41->dev, CS35L41_NUM_SUPPLIES,
 				      cs35l41->supplies);
-	if (ret != 0) {
-		dev_err(cs35l41->dev, "Failed to request core supplies: %d\n", ret);
-		return ret;
-	}
+	if (ret != 0)
+		return dev_err_probe(cs35l41->dev, ret,
+				     "Failed to request core supplies\n");
 
 	ret = regulator_bulk_enable(CS35L41_NUM_SUPPLIES, cs35l41->supplies);
-	if (ret != 0) {
-		dev_err(cs35l41->dev, "Failed to enable core supplies: %d\n", ret);
-		return ret;
-	}
+	if (ret != 0)
+		return dev_err_probe(cs35l41->dev, ret,
+				     "Failed to enable core supplies\n");
 
 	/* returning NULL can be an option if in stereo mode */
 	cs35l41->reset_gpio = devm_gpiod_get_optional(cs35l41->dev, "reset",
@@ -1225,8 +1223,8 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 			dev_info(cs35l41->dev,
 				 "Reset line busy, assuming shared reset\n");
 		} else {
-			dev_err(cs35l41->dev,
-				"Failed to get reset GPIO: %d\n", ret);
+			dev_err_probe(cs35l41->dev, ret,
+				      "Failed to get reset GPIO\n");
 			goto err;
 		}
 	}
@@ -1242,8 +1240,8 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 				       int_status, int_status & CS35L41_OTP_BOOT_DONE,
 				       1000, 100000);
 	if (ret) {
-		dev_err(cs35l41->dev,
-			"Failed waiting for OTP_BOOT_DONE: %d\n", ret);
+		dev_err_probe(cs35l41->dev, ret,
+			      "Failed waiting for OTP_BOOT_DONE\n");
 		goto err;
 	}
 
@@ -1256,13 +1254,13 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 
 	ret = regmap_read(cs35l41->regmap, CS35L41_DEVID, &regid);
 	if (ret < 0) {
-		dev_err(cs35l41->dev, "Get Device ID failed: %d\n", ret);
+		dev_err_probe(cs35l41->dev, ret, "Get Device ID failed\n");
 		goto err;
 	}
 
 	ret = regmap_read(cs35l41->regmap, CS35L41_REVID, &reg_revid);
 	if (ret < 0) {
-		dev_err(cs35l41->dev, "Get Revision ID failed: %d\n", ret);
+		dev_err_probe(cs35l41->dev, ret, "Get Revision ID failed\n");
 		goto err;
 	}
 
@@ -1287,7 +1285,7 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 
 	ret = cs35l41_otp_unpack(cs35l41->dev, cs35l41->regmap);
 	if (ret < 0) {
-		dev_err(cs35l41->dev, "OTP Unpack failed: %d\n", ret);
+		dev_err_probe(cs35l41->dev, ret, "OTP Unpack failed\n");
 		goto err;
 	}
 
@@ -1309,13 +1307,13 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 					IRQF_ONESHOT | IRQF_SHARED | irq_pol,
 					"cs35l41", cs35l41);
 	if (ret != 0) {
-		dev_err(cs35l41->dev, "Failed to request IRQ: %d\n", ret);
+		dev_err_probe(cs35l41->dev, ret, "Failed to request IRQ\n");
 		goto err;
 	}
 
 	ret = cs35l41_set_pdata(cs35l41);
 	if (ret < 0) {
-		dev_err(cs35l41->dev, "Set pdata failed: %d\n", ret);
+		dev_err_probe(cs35l41->dev, ret, "Set pdata failed\n");
 		goto err;
 	}
 
@@ -1340,7 +1338,7 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 					      &soc_component_dev_cs35l41,
 					      cs35l41_dai, ARRAY_SIZE(cs35l41_dai));
 	if (ret < 0) {
-		dev_err(cs35l41->dev, "Register codec failed: %d\n", ret);
+		dev_err_probe(cs35l41->dev, ret, "Register codec failed\n");
 		goto err_pm;
 	}
 
-- 
2.41.0

Re: [PATCH 6/9] ASoC: cs35l41: Make use of dev_err_probe()

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:18AM +0300, Cristian Ciocaltea wrote:
> Use dev_err_probe() helper where possible, to simplify error handling
> during probe.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

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

Thanks,
Charles

[PATCH 7/9] ASoC: cs35l41: Verify PM runtime resume errors in IRQ handler

[Cristian Ciocaltea]

The interrupt handler invokes pm_runtime_get_sync() without checking the
returned error code.

Add a proper verification and switch to pm_runtime_resume_and_get(), to
avoid the need to call pm_runtime_put_noidle() for decrementing the PM
usage counter before returning from the error condition.

Fixes: f517ba4924ad ("ASoC: cs35l41: Add support for hibernate memory retention mode")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/codecs/cs35l41.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index 6f2ad0d3a75c..66418547a4dd 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -386,10 +386,18 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 	struct cs35l41_private *cs35l41 = data;
 	unsigned int status[4] = { 0, 0, 0, 0 };
 	unsigned int masks[4] = { 0, 0, 0, 0 };
-	int ret = IRQ_NONE;
 	unsigned int i;
+	int ret;
 
-	pm_runtime_get_sync(cs35l41->dev);
+	ret = pm_runtime_resume_and_get(cs35l41->dev);
+	if (ret < 0) {
+		dev_err(cs35l41->dev,
+			"pm_runtime_resume_and_get failed in %s: %d\n",
+			__func__, ret);
+		return IRQ_NONE;
+	}
+
+	ret = IRQ_NONE;
 
 	for (i = 0; i < ARRAY_SIZE(status); i++) {
 		regmap_read(cs35l41->regmap,
-- 
2.41.0

Re: [PATCH 7/9] ASoC: cs35l41: Verify PM runtime resume errors in IRQ handler

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:19AM +0300, Cristian Ciocaltea wrote:
> The interrupt handler invokes pm_runtime_get_sync() without checking the
> returned error code.
> 
> Add a proper verification and switch to pm_runtime_resume_and_get(), to
> avoid the need to call pm_runtime_put_noidle() for decrementing the PM
> usage counter before returning from the error condition.
> 
> Fixes: f517ba4924ad ("ASoC: cs35l41: Add support for hibernate memory retention mode")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

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

Thanks,
Charles

[PATCH 8/9] ASoC: cs35l41: Use modern pm_ops

[Cristian Ciocaltea]

Make use of the recently introduced EXPORT_GPL_DEV_PM_OPS() macro, to
conditionally export the runtime/system PM functions.

Replace the old SET_{RUNTIME,SYSTEM_SLEEP,NOIRQ_SYSTEM_SLEEP}_PM_OPS()
helpers with their modern alternatives and get rid of the now
unnecessary '__maybe_unused' annotations on all PM functions.

Additionally, use the pm_ptr() macro to fix the following errors when
building with CONFIG_PM disabled:

ERROR: modpost: "cs35l41_pm_ops" [sound/soc/codecs/snd-soc-cs35l41-spi.ko] undefined!
ERROR: modpost: "cs35l41_pm_ops" [sound/soc/codecs/snd-soc-cs35l41-i2c.ko] undefined!

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/codecs/cs35l41-i2c.c |  2 +-
 sound/soc/codecs/cs35l41-spi.c |  2 +-
 sound/soc/codecs/cs35l41.c     | 21 ++++++++++-----------
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/cs35l41-i2c.c b/sound/soc/codecs/cs35l41-i2c.c
index 9109203a7f25..96414ee35285 100644
--- a/sound/soc/codecs/cs35l41-i2c.c
+++ b/sound/soc/codecs/cs35l41-i2c.c
@@ -80,7 +80,7 @@ MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_match);
 static struct i2c_driver cs35l41_i2c_driver = {
 	.driver = {
 		.name		= "cs35l41",
-		.pm		= &cs35l41_pm_ops,
+		.pm		= pm_ptr(&cs35l41_pm_ops),
 		.of_match_table = of_match_ptr(cs35l41_of_match),
 		.acpi_match_table = ACPI_PTR(cs35l41_acpi_match),
 	},
diff --git a/sound/soc/codecs/cs35l41-spi.c b/sound/soc/codecs/cs35l41-spi.c
index 28e9c9473e60..a6db44520c06 100644
--- a/sound/soc/codecs/cs35l41-spi.c
+++ b/sound/soc/codecs/cs35l41-spi.c
@@ -80,7 +80,7 @@ MODULE_DEVICE_TABLE(acpi, cs35l41_acpi_match);
 static struct spi_driver cs35l41_spi_driver = {
 	.driver = {
 		.name		= "cs35l41",
-		.pm		= &cs35l41_pm_ops,
+		.pm		= pm_ptr(&cs35l41_pm_ops),
 		.of_match_table = of_match_ptr(cs35l41_of_match),
 		.acpi_match_table = ACPI_PTR(cs35l41_acpi_match),
 	},
diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index 66418547a4dd..5655758063ae 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -1394,7 +1394,7 @@ void cs35l41_remove(struct cs35l41_private *cs35l41)
 }
 EXPORT_SYMBOL_GPL(cs35l41_remove);
 
-static int __maybe_unused cs35l41_runtime_suspend(struct device *dev)
+static int cs35l41_runtime_suspend(struct device *dev)
 {
 	struct cs35l41_private *cs35l41 = dev_get_drvdata(dev);
 
@@ -1411,7 +1411,7 @@ static int __maybe_unused cs35l41_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused cs35l41_runtime_resume(struct device *dev)
+static int cs35l41_runtime_resume(struct device *dev)
 {
 	struct cs35l41_private *cs35l41 = dev_get_drvdata(dev);
 	int ret;
@@ -1440,7 +1440,7 @@ static int __maybe_unused cs35l41_runtime_resume(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused cs35l41_sys_suspend(struct device *dev)
+static int cs35l41_sys_suspend(struct device *dev)
 {
 	struct cs35l41_private *cs35l41 = dev_get_drvdata(dev);
 
@@ -1450,7 +1450,7 @@ static int __maybe_unused cs35l41_sys_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused cs35l41_sys_suspend_noirq(struct device *dev)
+static int cs35l41_sys_suspend_noirq(struct device *dev)
 {
 	struct cs35l41_private *cs35l41 = dev_get_drvdata(dev);
 
@@ -1460,7 +1460,7 @@ static int __maybe_unused cs35l41_sys_suspend_noirq(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused cs35l41_sys_resume_noirq(struct device *dev)
+static int cs35l41_sys_resume_noirq(struct device *dev)
 {
 	struct cs35l41_private *cs35l41 = dev_get_drvdata(dev);
 
@@ -1470,7 +1470,7 @@ static int __maybe_unused cs35l41_sys_resume_noirq(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused cs35l41_sys_resume(struct device *dev)
+static int cs35l41_sys_resume(struct device *dev)
 {
 	struct cs35l41_private *cs35l41 = dev_get_drvdata(dev);
 
@@ -1480,13 +1480,12 @@ static int __maybe_unused cs35l41_sys_resume(struct device *dev)
 	return 0;
 }
 
-const struct dev_pm_ops cs35l41_pm_ops = {
-	SET_RUNTIME_PM_OPS(cs35l41_runtime_suspend, cs35l41_runtime_resume, NULL)
+EXPORT_GPL_DEV_PM_OPS(cs35l41_pm_ops) = {
+	RUNTIME_PM_OPS(cs35l41_runtime_suspend, cs35l41_runtime_resume, NULL)
 
-	SET_SYSTEM_SLEEP_PM_OPS(cs35l41_sys_suspend, cs35l41_sys_resume)
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cs35l41_sys_suspend_noirq, cs35l41_sys_resume_noirq)
+	SYSTEM_SLEEP_PM_OPS(cs35l41_sys_suspend, cs35l41_sys_resume)
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(cs35l41_sys_suspend_noirq, cs35l41_sys_resume_noirq)
 };
-EXPORT_SYMBOL_GPL(cs35l41_pm_ops);
 
 MODULE_DESCRIPTION("ASoC CS35L41 driver");
 MODULE_AUTHOR("David Rhodes, Cirrus Logic Inc, <david.rhodes@cirrus.com>");
-- 
2.41.0

Re: [PATCH 8/9] ASoC: cs35l41: Use modern pm_ops

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:20AM +0300, Cristian Ciocaltea wrote:
> Make use of the recently introduced EXPORT_GPL_DEV_PM_OPS() macro, to
> conditionally export the runtime/system PM functions.
> 
> Replace the old SET_{RUNTIME,SYSTEM_SLEEP,NOIRQ_SYSTEM_SLEEP}_PM_OPS()
> helpers with their modern alternatives and get rid of the now
> unnecessary '__maybe_unused' annotations on all PM functions.
> 
> Additionally, use the pm_ptr() macro to fix the following errors when
> building with CONFIG_PM disabled:
> 
> ERROR: modpost: "cs35l41_pm_ops" [sound/soc/codecs/snd-soc-cs35l41-spi.ko] undefined!
> ERROR: modpost: "cs35l41_pm_ops" [sound/soc/codecs/snd-soc-cs35l41-i2c.ko] undefined!
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

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

Thanks,
Charles

[PATCH 9/9] ASoC: cs35l41: Use devm_pm_runtime_enable()

[Cristian Ciocaltea]

Simplify runtime PM during probe by converting pm_runtime_enable() to
the managed version.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 sound/soc/codecs/cs35l41.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index 5655758063ae..2e5b4633e98d 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -1340,7 +1340,12 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 	pm_runtime_mark_last_busy(cs35l41->dev);
 	pm_runtime_set_active(cs35l41->dev);
 	pm_runtime_get_noresume(cs35l41->dev);
-	pm_runtime_enable(cs35l41->dev);
+
+	ret = devm_pm_runtime_enable(cs35l41->dev);
+	if (ret < 0) {
+		dev_err_probe(cs35l41->dev, ret, "Failed to enable PM runtime\n");
+		goto err_pm;
+	}
 
 	ret = devm_snd_soc_register_component(cs35l41->dev,
 					      &soc_component_dev_cs35l41,
@@ -1358,9 +1363,7 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 	return 0;
 
 err_pm:
-	pm_runtime_disable(cs35l41->dev);
 	pm_runtime_put_noidle(cs35l41->dev);
-
 	wm_adsp2_remove(&cs35l41->dsp);
 err:
 	cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type);
@@ -1376,7 +1379,6 @@ void cs35l41_remove(struct cs35l41_private *cs35l41)
 	cancel_work_sync(&cs35l41->mdsync_up_work);
 
 	pm_runtime_get_sync(cs35l41->dev);
-	pm_runtime_disable(cs35l41->dev);
 
 	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1, 0xFFFFFFFF);
 	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||
-- 
2.41.0

Re: [PATCH 9/9] ASoC: cs35l41: Use devm_pm_runtime_enable()

[Charles Keepax]

On Sun, Sep 03, 2023 at 12:06:21AM +0300, Cristian Ciocaltea wrote:
> Simplify runtime PM during probe by converting pm_runtime_enable() to
> the managed version.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> @@ -1376,7 +1379,6 @@ void cs35l41_remove(struct cs35l41_private *cs35l41)
>  	cancel_work_sync(&cs35l41->mdsync_up_work);
>  
>  	pm_runtime_get_sync(cs35l41->dev);
> -	pm_runtime_disable(cs35l41->dev);
>  
>  	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1, 0xFFFFFFFF);
>  	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||

Are we sure this is safe? The remove handler appears to be
written to disable pm_runtime at the start presumably to stop the
resume/suspend handler running during the remove callback.
Whereas after this change the pm_runtime isn't disabled until
after the remove callback has run. Does this open a window were
we could get an erroneous pm_runtime suspend after the
pm_runtime_put_noidle?

Thanks,
Charles

Re: [PATCH 9/9] ASoC: cs35l41: Use devm_pm_runtime_enable()

[Cristian Ciocaltea]

On 9/5/23 12:45, Charles Keepax wrote:
> On Sun, Sep 03, 2023 at 12:06:21AM +0300, Cristian Ciocaltea wrote:
>> Simplify runtime PM during probe by converting pm_runtime_enable() to
>> the managed version.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>> @@ -1376,7 +1379,6 @@ void cs35l41_remove(struct cs35l41_private *cs35l41)
>>  	cancel_work_sync(&cs35l41->mdsync_up_work);
>>  
>>  	pm_runtime_get_sync(cs35l41->dev);
>> -	pm_runtime_disable(cs35l41->dev);
>>  
>>  	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1, 0xFFFFFFFF);
>>  	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||
> 
> Are we sure this is safe? The remove handler appears to be
> written to disable pm_runtime at the start presumably to stop the
> resume/suspend handler running during the remove callback.
> Whereas after this change the pm_runtime isn't disabled until
> after the remove callback has run. Does this open a window were
> we could get an erroneous pm_runtime suspend after the
> pm_runtime_put_noidle?

I've just made a test adding a 6s sleep before returning from the remove 
handler: 

[14444.894316] cs35l41 spi-VLV1776:00: Runtime resume
[14444.894469] cs35l41 spi-VLV1776:00: sleep 6s before return of cs35l41_remove()
[14448.338994] cs35l41 spi-VLV1776:00: Runtime suspend
[14451.079649] cs35l41 spi-VLV1776:00: return from cs35l41_remove()
[14451.080129] cs35l41 spi-VLV1776:00: Runtime resume
[14451.080165] cs35l41 spi-VLV1776:00: ASoC: Unregistered DAI 'cs35l41-pcm'
[14451.080181] cs35l41 spi-VLV1776:00: Runtime suspend
[14451.813639] acp5x_i2s_playcap acp5x_i2s_playcap.0: ASoC: Unregistered DAI 'acp5x_i2s_playcap.0'

As expected, suspend triggered, but a resume was issued later, before DAI
got unregistered.

I didn't notice any issues while repeating the test several times, hence 
I wonder what would be the reason to prevent getting suspend/resume events 
at this point?

Thanks,
Cristian

Re: [PATCH 9/9] ASoC: cs35l41: Use devm_pm_runtime_enable()

[Charles Keepax]

On Tue, Sep 05, 2023 at 10:15:46PM +0300, Cristian Ciocaltea wrote:
> On 9/5/23 12:45, Charles Keepax wrote:
> > On Sun, Sep 03, 2023 at 12:06:21AM +0300, Cristian Ciocaltea wrote:
> >> Simplify runtime PM during probe by converting pm_runtime_enable() to
> >> the managed version.
> >>
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> ---
> >> @@ -1376,7 +1379,6 @@ void cs35l41_remove(struct cs35l41_private *cs35l41)
> >>  	cancel_work_sync(&cs35l41->mdsync_up_work);
> >>  
> >>  	pm_runtime_get_sync(cs35l41->dev);
> >> -	pm_runtime_disable(cs35l41->dev);
> >>  
> >>  	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1, 0xFFFFFFFF);
> >>  	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||
> > 
> > Are we sure this is safe? The remove handler appears to be
> > written to disable pm_runtime at the start presumably to stop the
> > resume/suspend handler running during the remove callback.
> > Whereas after this change the pm_runtime isn't disabled until
> > after the remove callback has run. Does this open a window were
> > we could get an erroneous pm_runtime suspend after the
> > pm_runtime_put_noidle?
> 
> I've just made a test adding a 6s sleep before returning from the remove 
> handler: 
> 
> [14444.894316] cs35l41 spi-VLV1776:00: Runtime resume
> [14444.894469] cs35l41 spi-VLV1776:00: sleep 6s before return of cs35l41_remove()
> [14448.338994] cs35l41 spi-VLV1776:00: Runtime suspend
> [14451.079649] cs35l41 spi-VLV1776:00: return from cs35l41_remove()
> [14451.080129] cs35l41 spi-VLV1776:00: Runtime resume
> [14451.080165] cs35l41 spi-VLV1776:00: ASoC: Unregistered DAI 'cs35l41-pcm'
> [14451.080181] cs35l41 spi-VLV1776:00: Runtime suspend
> [14451.813639] acp5x_i2s_playcap acp5x_i2s_playcap.0: ASoC: Unregistered DAI 'acp5x_i2s_playcap.0'
> 
> As expected, suspend triggered, but a resume was issued later, before DAI
> got unregistered.
> 
> I didn't notice any issues while repeating the test several times, hence 
> I wonder what would be the reason to prevent getting suspend/resume events 
> at this point?

The enter/exit hibernate code might run, which at the very
least might result in a bunch of unexpected and failing bus
traffic. Having a bit of a poke through the code, I guess the
most dangerous thing would if you actually got as far as an
extra runtime resume. This might cause cs35l41_init_boost
to run which would undo the work done by the call to
cs35l41_safe_reset in remove, which could leave the boost in a
dangerous state when we enable reset/power down the supplies,
which I think was not considered good. But its just likely
simpler/cleaner if we don't have to think about all the
possible implications of such things by just not allowing
it to happen.

Thanks,
Charles

Re: [PATCH 9/9] ASoC: cs35l41: Use devm_pm_runtime_enable()

[Cristian Ciocaltea]

On 9/6/23 19:37, Charles Keepax wrote:
> On Tue, Sep 05, 2023 at 10:15:46PM +0300, Cristian Ciocaltea wrote:
>> On 9/5/23 12:45, Charles Keepax wrote:
>>> On Sun, Sep 03, 2023 at 12:06:21AM +0300, Cristian Ciocaltea wrote:
>>>> Simplify runtime PM during probe by converting pm_runtime_enable() to
>>>> the managed version.
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>> @@ -1376,7 +1379,6 @@ void cs35l41_remove(struct cs35l41_private *cs35l41)
>>>>  	cancel_work_sync(&cs35l41->mdsync_up_work);
>>>>  
>>>>  	pm_runtime_get_sync(cs35l41->dev);
>>>> -	pm_runtime_disable(cs35l41->dev);
>>>>  
>>>>  	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1, 0xFFFFFFFF);
>>>>  	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||
>>>
>>> Are we sure this is safe? The remove handler appears to be
>>> written to disable pm_runtime at the start presumably to stop the
>>> resume/suspend handler running during the remove callback.
>>> Whereas after this change the pm_runtime isn't disabled until
>>> after the remove callback has run. Does this open a window were
>>> we could get an erroneous pm_runtime suspend after the
>>> pm_runtime_put_noidle?
>>
>> I've just made a test adding a 6s sleep before returning from the remove 
>> handler: 
>>
>> [14444.894316] cs35l41 spi-VLV1776:00: Runtime resume
>> [14444.894469] cs35l41 spi-VLV1776:00: sleep 6s before return of cs35l41_remove()
>> [14448.338994] cs35l41 spi-VLV1776:00: Runtime suspend
>> [14451.079649] cs35l41 spi-VLV1776:00: return from cs35l41_remove()
>> [14451.080129] cs35l41 spi-VLV1776:00: Runtime resume
>> [14451.080165] cs35l41 spi-VLV1776:00: ASoC: Unregistered DAI 'cs35l41-pcm'
>> [14451.080181] cs35l41 spi-VLV1776:00: Runtime suspend
>> [14451.813639] acp5x_i2s_playcap acp5x_i2s_playcap.0: ASoC: Unregistered DAI 'acp5x_i2s_playcap.0'
>>
>> As expected, suspend triggered, but a resume was issued later, before DAI
>> got unregistered.
>>
>> I didn't notice any issues while repeating the test several times, hence 
>> I wonder what would be the reason to prevent getting suspend/resume events 
>> at this point?
> 
> The enter/exit hibernate code might run, which at the very
> least might result in a bunch of unexpected and failing bus
> traffic. Having a bit of a poke through the code, I guess the
> most dangerous thing would if you actually got as far as an
> extra runtime resume. This might cause cs35l41_init_boost
> to run which would undo the work done by the call to
> cs35l41_safe_reset in remove, which could leave the boost in a
> dangerous state when we enable reset/power down the supplies,
> which I think was not considered good. But its just likely
> simpler/cleaner if we don't have to think about all the
> possible implications of such things by just not allowing
> it to happen.

Agree, let's keep it simple. I will revert the change and instead ensure
a proper cleanup of pm_runtime_use_autosuspend(), according to the
documentation:

"It's important to undo this with pm_runtime_dont_use_autosuspend() at
driver exit time unless your driver initially enabled pm_runtime with
devm_pm_runtime_enable() (which handles it for you)."

Thanks for the clarifications,
Cristian

Re: [PATCH 9/9] ASoC: cs35l41: Use devm_pm_runtime_enable()

[Charles Keepax]

On Wed, Sep 06, 2023 at 10:55:28PM +0300, Cristian Ciocaltea wrote:
> On 9/6/23 19:37, Charles Keepax wrote:
> > On Tue, Sep 05, 2023 at 10:15:46PM +0300, Cristian Ciocaltea wrote:
> >> On 9/5/23 12:45, Charles Keepax wrote:
> >>> On Sun, Sep 03, 2023 at 12:06:21AM +0300, Cristian Ciocaltea wrote:
> Agree, let's keep it simple. I will revert the change and instead ensure
> a proper cleanup of pm_runtime_use_autosuspend(), according to the
> documentation:
> 
> "It's important to undo this with pm_runtime_dont_use_autosuspend() at
> driver exit time unless your driver initially enabled pm_runtime with
> devm_pm_runtime_enable() (which handles it for you)."
> 

Good spot was not aware of that, thank you.

Thanks,
Charles