From 7d72351a4ef6e1e763bccc24d43c44ffbe1a1555 Mon Sep 17 00:00:00 2001 From: Simon Trimmer Date: Fri, 14 Apr 2023 14:37:49 +0100 Subject: [PATCH 1/5] ASoC: cs35l56: Rework IRQ allocation 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 Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/168147949598.26.711670799488943454@mailman-core.alsa-project.org Signed-off-by: Mark Brown --- 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 b82396122b028a07ce2977e3f33123d7ab526d91 Mon Sep 17 00:00:00 2001 From: Simon Trimmer Date: Fri, 14 Apr 2023 14:37:50 +0100 Subject: [PATCH 2/5] ASoC: cs35l56: Allow a wider range for reset pulse width 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 Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/168147949455.26.3401634900657387799@mailman-core.alsa-project.org Signed-off-by: Mark Brown --- 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 440c2d38950f738d7a35d3d29533728e74586d54 Mon Sep 17 00:00:00 2001 From: Simon Trimmer Date: Fri, 14 Apr 2023 14:37:51 +0100 Subject: [PATCH 3/5] ASoC: cs35l56: Wait for init_complete in cs35l56_component_probe() 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 Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/20230414133753.653139-4-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- 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 4be476830bf96c0095e4e1acdfc12081aa0fb165 Mon Sep 17 00:00:00 2001 From: Simon Trimmer Date: Fri, 14 Apr 2023 14:37:52 +0100 Subject: [PATCH 4/5] ASoC: cs35l56: Remove redundant dsp_ready_completion 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 Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/20230414133753.653139-5-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- 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 9ed4c762e214a6592184d0dd65b6db86ad30e6d5 Mon Sep 17 00:00:00 2001 From: Simon Trimmer Date: Fri, 14 Apr 2023 14:37:53 +0100 Subject: [PATCH 5/5] ASoC: cs35l56: Don't return a value from cs35l56_remove() 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 Signed-off-by: Richard Fitzgerald Link: https://lore.kernel.org/r/20230414133753.653139-6-rf@opensource.cirrus.com Signed-off-by: Mark Brown --- 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 */