[PATCH 0/5] ASoC: cs35l56: Bugfixes

[PATCH 0/5] ASoC: cs35l56: Bugfixes

[Richard Fitzgerald]

Miscellaneous bugfixes for the cs35l56 codec driver.

Richard Fitzgerald (3):
  ASoC: cs35l56: Avoid uninitialized variable in
    cs35l56_set_asp_slot_positions()
  ASoC: cs35l56: Don't rely on GPIOD_OUT_LOW to set RESET initially low
  ASoC: cs35l56: Wait for control port ready during system-resume

Simon Trimmer (2):
  ASoC: wm_adsp: Expose the DSP power down actions as
    wm_adsp_power_down()
  ASoC: cs35l56: Call wm_adsp_power_down() before reloading firmware

 sound/soc/codecs/cs35l56.c | 33 +++++++++++++++++----------------
 sound/soc/codecs/wm_adsp.c |  8 +++++++-
 sound/soc/codecs/wm_adsp.h |  1 +
 3 files changed, 25 insertions(+), 17 deletions(-)

-- 
2.30.2

[PATCH 1/5] ASoC: cs35l56: Avoid uninitialized variable in cs35l56_set_asp_slot_positions()

[Richard Fitzgerald]

Re-implement setting of ASP TDM slots so that only the common loop to
build the register word is factored out.

The original cs35l56_set_asp_slot_positions() had an apparent
uninitialized variable if the passed register address was neither of the
ASP slot registers. In fact this would never happen because the calling
code passed valid registers.

While it's trivial to initialize the variable or add a default case,
actually the only common code was the loop at the end of the function,
which simply manipulates some mask values and is identical for either
register. Factoring out the regmap_write() didn't really gain anything.

So instead re-implement the code to replace the original function with
cs35l56_make_tdm_config_word() that only does the loop, and change the
calling code to call regmap_write() directly.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 19b6b4fbe5de..be400208205a 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -358,22 +358,11 @@ static int cs35l56_asp_dai_set_fmt(struct snd_soc_dai *codec_dai, unsigned int f
 	return 0;
 }
 
-static void cs35l56_set_asp_slot_positions(struct cs35l56_private *cs35l56,
-					   unsigned int reg, unsigned long mask)
+static unsigned int cs35l56_make_tdm_config_word(unsigned int reg_val, unsigned long mask)
 {
-	unsigned int reg_val, channel_shift;
+	unsigned int channel_shift;
 	int bit_num;
 
-	/* Init all slots to 63 */
-	switch (reg) {
-	case CS35L56_ASP1_FRAME_CONTROL1:
-		reg_val = 0x3f3f3f3f;
-		break;
-	case CS35L56_ASP1_FRAME_CONTROL5:
-		reg_val = 0x3f3f3f;
-		break;
-	}
-
 	/* Enable consecutive TX1..TXn for each of the slots set in mask */
 	channel_shift = 0;
 	for_each_set_bit(bit_num, &mask, 32) {
@@ -382,7 +371,7 @@ static void cs35l56_set_asp_slot_positions(struct cs35l56_private *cs35l56,
 		channel_shift += 8;
 	}
 
-	regmap_write(cs35l56->base.regmap, reg, reg_val);
+	return reg_val;
 }
 
 static int cs35l56_asp_dai_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
@@ -418,8 +407,11 @@ static int cs35l56_asp_dai_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx
 	if (rx_mask == 0)
 		rx_mask = 0xf;	// ASPTX1..TX4 in slots 0..3
 
-	cs35l56_set_asp_slot_positions(cs35l56, CS35L56_ASP1_FRAME_CONTROL1, rx_mask);
-	cs35l56_set_asp_slot_positions(cs35l56, CS35L56_ASP1_FRAME_CONTROL5, tx_mask);
+	/* Default unused slots to 63 */
+	regmap_write(cs35l56->base.regmap, CS35L56_ASP1_FRAME_CONTROL1,
+		     cs35l56_make_tdm_config_word(0x3f3f3f3f, rx_mask));
+	regmap_write(cs35l56->base.regmap, CS35L56_ASP1_FRAME_CONTROL5,
+		     cs35l56_make_tdm_config_word(0x3f3f3f, tx_mask));
 
 	dev_dbg(cs35l56->base.dev, "tdm slot width: %u count: %u tx_mask: %#x rx_mask: %#x\n",
 		cs35l56->asp_slot_width, cs35l56->asp_slot_count, tx_mask, rx_mask);
-- 
2.30.2

[PATCH 2/5] ASoC: cs35l56: Don't rely on GPIOD_OUT_LOW to set RESET initially low

[Richard Fitzgerald]

The ACPI setting for a GPIO default state has higher priority than the
flag passed to devm_gpiod_get_optional() so ACPI can override the
GPIOD_OUT_LOW. Explicitly set the GPIO low when hard resetting.

Although GPIOD_OUT_LOW can't be relied on this doesn't seem like a
reason to stop passing it to devm_gpiod_get_optional(). So we still pass
it to state our intent, but can deal with it having no effect.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index be400208205a..9560059c867b 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -1069,6 +1069,8 @@ int cs35l56_common_probe(struct cs35l56_private *cs35l56)
 		return dev_err_probe(cs35l56->base.dev, ret, "Failed to enable supplies\n");
 
 	if (cs35l56->base.reset_gpio) {
+		/* ACPI can override GPIOD_OUT_LOW flag so force it to start low */
+		gpiod_set_value_cansleep(cs35l56->base.reset_gpio, 0);
 		cs35l56_wait_min_reset_pulse();
 		gpiod_set_value_cansleep(cs35l56->base.reset_gpio, 1);
 	}
-- 
2.30.2

[PATCH 3/5] ASoC: cs35l56: Wait for control port ready during system-resume

[Richard Fitzgerald]

The CS35L56 could be hard-reset during a system suspend-resume cycle,
either by the codec driver, in cs35l56_system_resume_early(), or by ACPI.
After a hard reset the driver must wait for the control port to be ready
(datasheet tIRS time) before attempting to access the CS35L56.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 9560059c867b..094bcbd0a174 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -952,6 +952,12 @@ int cs35l56_system_resume(struct device *dev)
 
 	dev_dbg(dev, "system_resume\n");
 
+	/*
+	 * We might have done a hard reset or the CS35L56 was power-cycled
+	 * so wait for control port to be ready.
+	 */
+	cs35l56_wait_control_port_ready();
+
 	/* Undo pm_runtime_force_suspend() before re-enabling the irq */
 	ret = pm_runtime_force_resume(dev);
 	if (cs35l56->base.irq)
-- 
2.30.2

[PATCH 4/5] ASoC: wm_adsp: Expose the DSP power down actions as wm_adsp_power_down()

[Richard Fitzgerald]

From: Simon Trimmer <simont@opensource.cirrus.com>

To support self-booting DSPs that operate outside of a conventional DAPM
event life cycle expose a companion function to wm_adsp_power_up() so
that the correct state of the DSP firmware and controls can be recorded.

Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/wm_adsp.c | 8 +++++++-
 sound/soc/codecs/wm_adsp.h | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 5a89abfe8784..13f500fa9a5f 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -1025,6 +1025,12 @@ int wm_adsp_power_up(struct wm_adsp *dsp)
 }
 EXPORT_SYMBOL_GPL(wm_adsp_power_up);
 
+void wm_adsp_power_down(struct wm_adsp *dsp)
+{
+	cs_dsp_power_down(&dsp->cs_dsp);
+}
+EXPORT_SYMBOL_GPL(wm_adsp_power_down);
+
 static void wm_adsp_boot_work(struct work_struct *work)
 {
 	struct wm_adsp *dsp = container_of(work,
@@ -1046,7 +1052,7 @@ int wm_adsp_early_event(struct snd_soc_dapm_widget *w,
 		queue_work(system_unbound_wq, &dsp->boot_work);
 		break;
 	case SND_SOC_DAPM_PRE_PMD:
-		cs_dsp_power_down(&dsp->cs_dsp);
+		wm_adsp_power_down(dsp);
 		break;
 	default:
 		break;
diff --git a/sound/soc/codecs/wm_adsp.h b/sound/soc/codecs/wm_adsp.h
index 769904d34a87..3044f964ac14 100644
--- a/sound/soc/codecs/wm_adsp.h
+++ b/sound/soc/codecs/wm_adsp.h
@@ -92,6 +92,7 @@ int wm_adsp_early_event(struct snd_soc_dapm_widget *w,
 			struct snd_kcontrol *kcontrol, int event);
 
 int wm_adsp_power_up(struct wm_adsp *dsp);
+void wm_adsp_power_down(struct wm_adsp *dsp);
 
 irqreturn_t wm_adsp2_bus_error(int irq, void *data);
 irqreturn_t wm_halo_bus_error(int irq, void *data);
-- 
2.30.2

[PATCH 5/5] ASoC: cs35l56: Call wm_adsp_power_down() before reloading firmware

[Richard Fitzgerald]

From: Simon Trimmer <simont@opensource.cirrus.com>

When cs35l56_system_resume() needs to reload firmware it should call
wm_adsp_power_down() to put cs_dsp into a powered-down state before
cs35l56_secure_patch() or cs35l56_patch() calls wm_adsp_power_up().

Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 094bcbd0a174..80e7fddae926 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -976,6 +976,7 @@ int cs35l56_system_resume(struct device *dev)
 		return ret;
 
 	cs35l56->base.fw_patched = false;
+	wm_adsp_power_down(&cs35l56->dsp);
 	queue_work(cs35l56->dsp_wq, &cs35l56->dsp_work);
 
 	/*
-- 
2.30.2

Re: [PATCH 0/5] ASoC: cs35l56: Bugfixes

[Mark Brown]

On Tue, 08 Aug 2023 17:46:57 +0100, Richard Fitzgerald wrote:
> Miscellaneous bugfixes for the cs35l56 codec driver.
> 
> Richard Fitzgerald (3):
>   ASoC: cs35l56: Avoid uninitialized variable in
>     cs35l56_set_asp_slot_positions()
>   ASoC: cs35l56: Don't rely on GPIOD_OUT_LOW to set RESET initially low
>   ASoC: cs35l56: Wait for control port ready during system-resume
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/5] ASoC: cs35l56: Avoid uninitialized variable in cs35l56_set_asp_slot_positions()
      commit: ebd0f7b08e032900e5327962f7da6bed6f37feb6
[2/5] ASoC: cs35l56: Don't rely on GPIOD_OUT_LOW to set RESET initially low
      commit: 853734588dcb1bf4c41a17e4d9df231965e559db
[3/5] ASoC: cs35l56: Wait for control port ready during system-resume
      commit: f5eb9503e80e70c22e3d3f73a5d3c149c132407f
[4/5] ASoC: wm_adsp: Expose the DSP power down actions as wm_adsp_power_down()
      commit: d0a3a6ad0d3b24578f1b3832ad1d7fbdb20f7a20
[5/5] ASoC: cs35l56: Call wm_adsp_power_down() before reloading firmware
      commit: e24ef967c735bf7272099610e422f964c0a4258b

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark