[PATCH 0/5] ASoC: cs35l56: Code improvements

[PATCH 0/5] ASoC: cs35l56: Code improvements

[Richard Fitzgerald]

Various code improvements. These remove redundant code and
clean up less-than-optimal original implementations.

Simon Trimmer (5):
  ASoC: cs35l56: Rework IRQ allocation
  ASoC: cs35l56: Allow a wider range for reset pulse width
  ASoC: cs35l56: Wait for init_complete in cs35l56_component_probe()
  ASoC: cs35l56: Remove redundant dsp_ready_completion
  ASoC: cs35l56: Don't return a value from cs35l56_remove()

 sound/soc/codecs/cs35l56-i2c.c |  3 +-
 sound/soc/codecs/cs35l56-sdw.c |  4 +-
 sound/soc/codecs/cs35l56-spi.c |  3 +-
 sound/soc/codecs/cs35l56.c     | 83 +++++++++++++---------------------
 sound/soc/codecs/cs35l56.h     |  5 +-
 5 files changed, 38 insertions(+), 60 deletions(-)

-- 
2.30.2

[PATCH 1/5] ASoC: cs35l56: Rework IRQ allocation

[Richard Fitzgerald]

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

The irq member was being set before calling the init function and then
cs35l56_irq_request() was called only when the init was successful.
However cs35l56_release() calls devm_free_irq() when the irq member is
set and therefore if init() fails then this will cause an attempted free
of an unallocated IRQ.

Instead pass the desired IRQ number to the cs35l56_irq_request()
function and set cs35l56->irq only when it has been successfully
allocated.

Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56-i2c.c |  3 +--
 sound/soc/codecs/cs35l56-spi.c |  3 +--
 sound/soc/codecs/cs35l56.c     | 11 ++++++-----
 sound/soc/codecs/cs35l56.h     |  2 +-
 4 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/sound/soc/codecs/cs35l56-i2c.c b/sound/soc/codecs/cs35l56-i2c.c
index 4b7f034a7670..295caad26224 100644
--- a/sound/soc/codecs/cs35l56-i2c.c
+++ b/sound/soc/codecs/cs35l56-i2c.c
@@ -27,7 +27,6 @@ static int cs35l56_i2c_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	cs35l56->dev = dev;
-	cs35l56->irq = client->irq;
 	cs35l56->can_hibernate = true;
 
 	i2c_set_clientdata(client, cs35l56);
@@ -43,7 +42,7 @@ static int cs35l56_i2c_probe(struct i2c_client *client)
 
 	ret = cs35l56_init(cs35l56);
 	if (ret == 0)
-		ret = cs35l56_irq_request(cs35l56);
+		ret = cs35l56_irq_request(cs35l56, client->irq);
 	if (ret < 0)
 		cs35l56_remove(cs35l56);
 
diff --git a/sound/soc/codecs/cs35l56-spi.c b/sound/soc/codecs/cs35l56-spi.c
index 4b2084e85f29..996aab10500e 100644
--- a/sound/soc/codecs/cs35l56-spi.c
+++ b/sound/soc/codecs/cs35l56-spi.c
@@ -32,7 +32,6 @@ static int cs35l56_spi_probe(struct spi_device *spi)
 	}
 
 	cs35l56->dev = &spi->dev;
-	cs35l56->irq = spi->irq;
 
 	ret = cs35l56_common_probe(cs35l56);
 	if (ret != 0)
@@ -40,7 +39,7 @@ static int cs35l56_spi_probe(struct spi_device *spi)
 
 	ret = cs35l56_init(cs35l56);
 	if (ret == 0)
-		ret = cs35l56_irq_request(cs35l56);
+		ret = cs35l56_irq_request(cs35l56, spi->irq);
 	if (ret < 0)
 		cs35l56_remove(cs35l56);
 
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 18e341744839..5ea7f419cda6 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -423,18 +423,19 @@ irqreturn_t cs35l56_irq(int irq, void *data)
 }
 EXPORT_SYMBOL_NS_GPL(cs35l56_irq, SND_SOC_CS35L56_CORE);
 
-int cs35l56_irq_request(struct cs35l56_private *cs35l56)
+int cs35l56_irq_request(struct cs35l56_private *cs35l56, int irq)
 {
 	int ret;
 
-	if (!cs35l56->irq)
+	if (!irq)
 		return 0;
 
-	ret = devm_request_threaded_irq(cs35l56->dev, cs35l56->irq, NULL,
-					cs35l56_irq,
+	ret = devm_request_threaded_irq(cs35l56->dev, irq, NULL, cs35l56_irq,
 					IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW,
 					"cs35l56", cs35l56);
-	if (ret < 0)
+	if (!ret)
+		cs35l56->irq = irq;
+	else
 		dev_err(cs35l56->dev, "Failed to get IRQ: %d\n", ret);
 
 	return ret;
diff --git a/sound/soc/codecs/cs35l56.h b/sound/soc/codecs/cs35l56.h
index 50278dafc9ca..ac2e9237c27d 100644
--- a/sound/soc/codecs/cs35l56.h
+++ b/sound/soc/codecs/cs35l56.h
@@ -74,7 +74,7 @@ int cs35l56_system_resume_no_irq(struct device *dev);
 int cs35l56_system_resume_early(struct device *dev);
 int cs35l56_system_resume(struct device *dev);
 irqreturn_t cs35l56_irq(int irq, void *data);
-int cs35l56_irq_request(struct cs35l56_private *cs35l56);
+int cs35l56_irq_request(struct cs35l56_private *cs35l56, int irq);
 int cs35l56_common_probe(struct cs35l56_private *cs35l56);
 int cs35l56_init(struct cs35l56_private *cs35l56);
 int cs35l56_remove(struct cs35l56_private *cs35l56);
-- 
2.30.2

[PATCH 2/5] ASoC: cs35l56: Allow a wider range for reset pulse width

[Richard Fitzgerald]

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

There is no reason to have such a tight usleep range of 400us and it is
acceptable to allow MIN_US * 2.

Also wrap the usleep in an inline function.

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

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 5ea7f419cda6..d60095162bfa 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -835,6 +835,12 @@ static int cs35l56_wait_for_firmware_boot(struct cs35l56_private *cs35l56)
 	return 0;
 }
 
+static inline void cs35l56_wait_min_reset_pulse(void)
+{
+	/* Satisfy minimum reset pulse width spec */
+	usleep_range(CS35L56_RESET_PULSE_MIN_US, 2 * CS35L56_RESET_PULSE_MIN_US);
+}
+
 static const struct reg_sequence cs35l56_system_reset_seq[] = {
 	REG_SEQ0(CS35L56_DSP_VIRTUAL1_MBOX_1, CS35L56_MBOX_CMD_SYSTEM_RESET),
 };
@@ -1236,7 +1242,7 @@ int cs35l56_system_suspend_late(struct device *dev)
 	 */
 	if (cs35l56->reset_gpio) {
 		gpiod_set_value_cansleep(cs35l56->reset_gpio, 0);
-		usleep_range(CS35L56_RESET_PULSE_MIN_US, CS35L56_RESET_PULSE_MIN_US + 400);
+		cs35l56_wait_min_reset_pulse();
 	}
 
 	regulator_bulk_disable(ARRAY_SIZE(cs35l56->supplies), cs35l56->supplies);
@@ -1289,7 +1295,7 @@ int cs35l56_system_resume_early(struct device *dev)
 	/* Ensure a spec-compliant RESET pulse. */
 	if (cs35l56->reset_gpio) {
 		gpiod_set_value_cansleep(cs35l56->reset_gpio, 0);
-		usleep_range(CS35L56_RESET_PULSE_MIN_US, CS35L56_RESET_PULSE_MIN_US + 400);
+		cs35l56_wait_min_reset_pulse();
 	}
 
 	/* Enable supplies before releasing RESET. */
@@ -1440,9 +1446,7 @@ int cs35l56_common_probe(struct cs35l56_private *cs35l56)
 		return dev_err_probe(cs35l56->dev, ret, "Failed to enable supplies\n");
 
 	if (cs35l56->reset_gpio) {
-		/* satisfy minimum reset pulse width spec */
-		usleep_range(CS35L56_RESET_PULSE_MIN_US,
-			     CS35L56_RESET_PULSE_MIN_US + 400);
+		cs35l56_wait_min_reset_pulse();
 		gpiod_set_value_cansleep(cs35l56->reset_gpio, 1);
 	}
 
-- 
2.30.2

[PATCH 3/5] ASoC: cs35l56: Wait for init_complete in cs35l56_component_probe()

[Richard Fitzgerald]

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

Moving the wait from the beginning of the cs35l56_dsp_work() into
cs35l56_component_probe() will prevent the limbo situation that is an
artifact of the two stage SoundWire driver probe and initialisation
where the card is all registered and shows in ALSA but doesn't actually
work because the hardware didn't enumerate.

The other bus drivers perform the probe and init sequentially and are
not susceptible to this issue.

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

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index d60095162bfa..ab2e663af6c2 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -875,13 +875,6 @@ static void cs35l56_dsp_work(struct work_struct *work)
 	unsigned int val;
 	int ret = 0;
 
-	if (!cs35l56->init_done &&
-	    !wait_for_completion_timeout(&cs35l56->init_completion,
-					 msecs_to_jiffies(5000))) {
-		dev_err(cs35l56->dev, "%s: init_completion timed out\n", __func__);
-		goto complete;
-	}
-
 	if (!cs35l56->init_done)
 		goto complete;
 
@@ -980,6 +973,12 @@ static int cs35l56_component_probe(struct snd_soc_component *component)
 
 	BUILD_BUG_ON(ARRAY_SIZE(cs35l56_tx_input_texts) != ARRAY_SIZE(cs35l56_tx_input_values));
 
+	if (!wait_for_completion_timeout(&cs35l56->init_completion,
+					 msecs_to_jiffies(5000))) {
+		dev_err(cs35l56->dev, "%s: init_completion timed out\n", __func__);
+		return -ENODEV;
+	}
+
 	cs35l56->component = component;
 	wm_adsp2_component_probe(&cs35l56->dsp, component);
 
-- 
2.30.2

[PATCH 4/5] ASoC: cs35l56: Remove redundant dsp_ready_completion

[Richard Fitzgerald]

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

dsp_ready_completion is redundant and can be replaced by a call
flush_work() to wait for cs35l56_dsp_work() to complete.

As the dsp_work is queued by component_probe() it must run before other
ASoC component callbacks and therefore there is no risk of calling
flush_work() before the dsp_work() has been queued.

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

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index ab2e663af6c2..5f66a8e20b2d 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -51,21 +51,10 @@ static int cs35l56_mbox_send(struct cs35l56_private *cs35l56, unsigned int comma
 	return 0;
 }
 
-static int cs35l56_wait_dsp_ready(struct cs35l56_private *cs35l56)
+static void cs35l56_wait_dsp_ready(struct cs35l56_private *cs35l56)
 {
-	int ret;
-
-	if (!cs35l56->fw_patched) {
-		/* block until firmware download completes */
-		ret = wait_for_completion_timeout(&cs35l56->dsp_ready_completion,
-						  msecs_to_jiffies(25000));
-		if (!ret) {
-			dev_err(cs35l56->dev, "dsp_ready_completion timeout\n");
-			return -ETIMEDOUT;
-		}
-	}
-
-	return 0;
+	/* Wait for patching to complete */
+	flush_work(&cs35l56->dsp_work);
 }
 
 static int cs35l56_dspwait_get_volsw(struct snd_kcontrol *kcontrol,
@@ -73,11 +62,8 @@ static int cs35l56_dspwait_get_volsw(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component);
-	int ret = cs35l56_wait_dsp_ready(cs35l56);
-
-	if (ret)
-		return ret;
 
+	cs35l56_wait_dsp_ready(cs35l56);
 	return snd_soc_get_volsw(kcontrol, ucontrol);
 }
 
@@ -86,11 +72,8 @@ static int cs35l56_dspwait_put_volsw(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
 	struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component);
-	int ret = cs35l56_wait_dsp_ready(cs35l56);
-
-	if (ret)
-		return ret;
 
+	cs35l56_wait_dsp_ready(cs35l56);
 	return snd_soc_put_volsw(kcontrol, ucontrol);
 }
 
@@ -876,13 +859,13 @@ static void cs35l56_dsp_work(struct work_struct *work)
 	int ret = 0;
 
 	if (!cs35l56->init_done)
-		goto complete;
+		return;
 
 	cs35l56->dsp.part = devm_kasprintf(cs35l56->dev, GFP_KERNEL, "cs35l56%s-%02x",
 					   cs35l56->secured ? "s" : "", cs35l56->rev);
 
 	if (!cs35l56->dsp.part)
-		goto complete;
+		return;
 
 	pm_runtime_get_sync(cs35l56->dev);
 
@@ -961,9 +944,6 @@ static void cs35l56_dsp_work(struct work_struct *work)
 		sdw_write_no_pm(cs35l56->sdw_peripheral, CS35L56_SDW_GEN_INT_MASK_1,
 				CS35L56_SDW_INT_MASK_CODEC_IRQ);
 	}
-
-complete:
-	complete_all(&cs35l56->dsp_ready_completion);
 }
 
 static int cs35l56_component_probe(struct snd_soc_component *component)
@@ -1002,7 +982,6 @@ static int cs35l56_set_bias_level(struct snd_soc_component *component,
 				  enum snd_soc_bias_level level)
 {
 	struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component);
-	int ret = 0;
 
 	switch (level) {
 	case SND_SOC_BIAS_STANDBY:
@@ -1011,14 +990,14 @@ static int cs35l56_set_bias_level(struct snd_soc_component *component,
 		 * BIAS_OFF to BIAS_STANDBY
 		 */
 		if (snd_soc_component_get_bias_level(component) == SND_SOC_BIAS_OFF)
-			ret = cs35l56_wait_dsp_ready(cs35l56);
+			cs35l56_wait_dsp_ready(cs35l56);
 
 		break;
 	default:
 		break;
 	}
 
-	return ret;
+	return 0;
 }
 
 static const struct snd_soc_component_driver soc_component_dev_cs35l56 = {
@@ -1336,7 +1315,6 @@ int cs35l56_system_resume(struct device *dev)
 		return ret;
 
 	cs35l56->fw_patched = false;
-	init_completion(&cs35l56->dsp_ready_completion);
 	queue_work(cs35l56->dsp_wq, &cs35l56->dsp_work);
 
 	/*
@@ -1358,7 +1336,6 @@ static int cs35l56_dsp_init(struct cs35l56_private *cs35l56)
 		return -ENOMEM;
 
 	INIT_WORK(&cs35l56->dsp_work, cs35l56_dsp_work);
-	init_completion(&cs35l56->dsp_ready_completion);
 
 	dsp = &cs35l56->dsp;
 	dsp->part = "cs35l56";
diff --git a/sound/soc/codecs/cs35l56.h b/sound/soc/codecs/cs35l56.h
index ac2e9237c27d..09762e70ce81 100644
--- a/sound/soc/codecs/cs35l56.h
+++ b/sound/soc/codecs/cs35l56.h
@@ -34,7 +34,6 @@ struct cs35l56_private {
 	struct wm_adsp dsp; /* must be first member */
 	struct work_struct dsp_work;
 	struct workqueue_struct *dsp_wq;
-	struct completion dsp_ready_completion;
 	struct mutex irq_lock;
 	struct snd_soc_component *component;
 	struct device *dev;
-- 
2.30.2

[PATCH 5/5] ASoC: cs35l56: Don't return a value from cs35l56_remove()

[Richard Fitzgerald]

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

cs35l56_remove() always returns 0. Two of the functions that call
it are void and the other one should only return 0. So there's no
point returning anything from cs35l56_remove().

Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l56-sdw.c | 4 +++-
 sound/soc/codecs/cs35l56.c     | 4 +---
 sound/soc/codecs/cs35l56.h     | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/cs35l56-sdw.c b/sound/soc/codecs/cs35l56-sdw.c
index e759347423cf..2cde78605ba9 100644
--- a/sound/soc/codecs/cs35l56-sdw.c
+++ b/sound/soc/codecs/cs35l56-sdw.c
@@ -527,7 +527,9 @@ static int cs35l56_sdw_remove(struct sdw_slave *peripheral)
 	sdw_read_no_pm(peripheral, CS35L56_SDW_GEN_INT_STAT_1);
 	sdw_write_no_pm(peripheral, CS35L56_SDW_GEN_INT_STAT_1, 0xFF);
 
-	return cs35l56_remove(cs35l56);
+	cs35l56_remove(cs35l56);
+
+	return 0;
 }
 
 static const struct dev_pm_ops cs35l56_sdw_pm = {
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 5f66a8e20b2d..0f4a94b02ef8 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -1590,7 +1590,7 @@ int cs35l56_init(struct cs35l56_private *cs35l56)
 }
 EXPORT_SYMBOL_NS_GPL(cs35l56_init, SND_SOC_CS35L56_CORE);
 
-int cs35l56_remove(struct cs35l56_private *cs35l56)
+void cs35l56_remove(struct cs35l56_private *cs35l56)
 {
 	cs35l56->init_done = false;
 
@@ -1613,8 +1613,6 @@ int cs35l56_remove(struct cs35l56_private *cs35l56)
 
 	gpiod_set_value_cansleep(cs35l56->reset_gpio, 0);
 	regulator_bulk_disable(ARRAY_SIZE(cs35l56->supplies), cs35l56->supplies);
-
-	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cs35l56_remove, SND_SOC_CS35L56_CORE);
 
diff --git a/sound/soc/codecs/cs35l56.h b/sound/soc/codecs/cs35l56.h
index 09762e70ce81..1f7894662fcb 100644
--- a/sound/soc/codecs/cs35l56.h
+++ b/sound/soc/codecs/cs35l56.h
@@ -76,6 +76,6 @@ irqreturn_t cs35l56_irq(int irq, void *data);
 int cs35l56_irq_request(struct cs35l56_private *cs35l56, int irq);
 int cs35l56_common_probe(struct cs35l56_private *cs35l56);
 int cs35l56_init(struct cs35l56_private *cs35l56);
-int cs35l56_remove(struct cs35l56_private *cs35l56);
+void cs35l56_remove(struct cs35l56_private *cs35l56);
 
 #endif /* ifndef CS35L56_H */
-- 
2.30.2

Re: [PATCH 0/5] ASoC: cs35l56: Code improvements

[Mark Brown]

On Fri, 14 Apr 2023 14:37:48 +0100, Richard Fitzgerald wrote:
> Various code improvements. These remove redundant code and
> clean up less-than-optimal original implementations.
> 
> Simon Trimmer (5):
>   ASoC: cs35l56: Rework IRQ allocation
>   ASoC: cs35l56: Allow a wider range for reset pulse width
>   ASoC: cs35l56: Wait for init_complete in cs35l56_component_probe()
>   ASoC: cs35l56: Remove redundant dsp_ready_completion
>   ASoC: cs35l56: Don't return a value from cs35l56_remove()
> 
> [...]

Applied to

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

Thanks!

[1/5] ASoC: cs35l56: Rework IRQ allocation
      commit: 7d72351a4ef6e1e763bccc24d43c44ffbe1a1555
[2/5] ASoC: cs35l56: Allow a wider range for reset pulse width
      commit: b82396122b028a07ce2977e3f33123d7ab526d91
[3/5] ASoC: cs35l56: Wait for init_complete in cs35l56_component_probe()
      commit: 440c2d38950f738d7a35d3d29533728e74586d54
[4/5] ASoC: cs35l56: Remove redundant dsp_ready_completion
      commit: 4be476830bf96c0095e4e1acdfc12081aa0fb165
[5/5] ASoC: cs35l56: Don't return a value from cs35l56_remove()
      commit: 9ed4c762e214a6592184d0dd65b6db86ad30e6d5

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