[PATCH 0/5] ASoC: cs35l56: Code improvements
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(-)
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);
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); }
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);
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;
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 */
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
participants (2)
-
Mark Brown -
Richard Fitzgerald