[PATCH 0/4] Yet another round of SDCA fixes

[PATCH 0/4] Yet another round of SDCA fixes

[Charles Keepax]

Another round of SDCA fixes a couple of fix to the IRQ cleanup
from Richard, and a minor tweak to the IRQ handling from me and
an update to the comment on a fixme.

Thanks,
Charles

Changes since v1:
 - Move cancel_work_sync() on boot_work to bus remove.
 - Add fix for kernel doc error
 - Add patch to update text of FIXME

Charles Keepax (3):
  ASoC: SDCA: Correct kernel doc for sdca_irq_cleanup()
  ASoC: SDCA: Tidy up irq_enable_flags()/sdca_irq_disable()
  ASoC: SDCA: Update text of FIXME

Richard Fitzgerald (1):
  ASoC: SDCA: Fix cleanup inversion in class driver

 sound/soc/sdca/sdca_class.c      | 34 ++++++++++++++++++++------------
 sound/soc/sdca/sdca_interrupts.c | 24 ++++++++++++++--------
 2 files changed, 37 insertions(+), 21 deletions(-)

-- 
2.47.3

[PATCH v2 1/4] ASoC: SDCA: Correct kernel doc for sdca_irq_cleanup()

[Charles Keepax]

Fix typo of function argument name in the kernel doc.

Fixes: 0b8757b220f9 ("ASoC: SDCA: Unregister IRQ handlers on module remove")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202604090800.koxM6j6O-lkp@intel.com/
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

New since v1.

 sound/soc/sdca/sdca_interrupts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 5cdabf8ae9da3..475aebcafe3f1 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -555,7 +555,7 @@ EXPORT_SYMBOL_NS_GPL(sdca_irq_populate, "SND_SOC_SDCA");
 
 /**
  * sdca_irq_cleanup - Free all the individual IRQs for an SDCA Function
- * @sdev: Device pointer against which the sdca_interrupt_info was allocated.
+ * @dev: Device pointer against which the sdca_interrupt_info was allocated.
  * @function: Pointer to the SDCA Function.
  * @info: Pointer to the SDCA interrupt info for this device.
  *
-- 
2.47.3

[PATCH v2 2/4] ASoC: SDCA: Fix cleanup inversion in class driver

[Charles Keepax]

From: Richard Fitzgerald <rf@opensource.cirrus.com>

Fix inverted cleanup of the SoundWire IRQ and the function drivers
that use it.

The devm cleanup function to call sdca_dev_unregister_functions() was
being registered at the end of class_sdw_probe(). The bus core
creates the parent SoundWire IRQ handler after class_sdw_probe() has
returned, and it registers a devm cleanup handler at the same time.

This led to a cleanup inversion where the devm cleanup for the parent
Soundwire IRQ runs before the handler that removes the function drivers.
So the parent IRQ is destroyed before the function drivers had a chance
to do any cleanup and remove their IRQ handlers.

Move the registrations of the function driver cleanup into
class_boot_work() after the function drivers are registered, so that it
runs before the cleanup of the parent SoundWire IRQ handler.

Fixes: 2d877d0659cb ("ASoC: SDCA: Add basic SDCA class driver")
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Move cancel_work_sync() on boot_work to bus remove.

 sound/soc/sdca/sdca_class.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
index 7af4e5d1b347f..6e9b66f718019 100644
--- a/sound/soc/sdca/sdca_class.c
+++ b/sound/soc/sdca/sdca_class.c
@@ -137,6 +137,13 @@ static const struct regmap_config class_dev_regmap_config = {
 	.unlock			= class_regmap_unlock,
 };
 
+static void class_remove_functions(void *data)
+{
+	struct sdca_class_drv *drv = data;
+
+	sdca_dev_unregister_functions(drv->sdw);
+}
+
 static void class_boot_work(struct work_struct *work)
 {
 	struct sdca_class_drv *drv = container_of(work,
@@ -157,6 +164,11 @@ static void class_boot_work(struct work_struct *work)
 	if (ret)
 		goto err;
 
+	/* Ensure function drivers are removed before the IRQ is destroyed */
+	ret = devm_add_action_or_reset(drv->dev, class_remove_functions, drv);
+	if (ret)
+		goto err;
+
 	dev_dbg(drv->dev, "boot work complete\n");
 
 	pm_runtime_mark_last_busy(drv->dev);
@@ -168,15 +180,6 @@ static void class_boot_work(struct work_struct *work)
 	pm_runtime_put_sync(drv->dev);
 }
 
-static void class_dev_remove(void *data)
-{
-	struct sdca_class_drv *drv = data;
-
-	cancel_work_sync(&drv->boot_work);
-
-	sdca_dev_unregister_functions(drv->sdw);
-}
-
 static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id)
 {
 	struct device *dev = &sdw->dev;
@@ -230,15 +233,19 @@ static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id
 	if (ret)
 		return ret;
 
-	ret = devm_add_action_or_reset(dev, class_dev_remove, drv);
-	if (ret)
-		return ret;
-
 	queue_work(system_long_wq, &drv->boot_work);
 
 	return 0;
 }
 
+static void class_sdw_remove(struct sdw_slave *sdw)
+{
+	struct device *dev = &sdw->dev;
+	struct sdca_class_drv *drv = dev_get_drvdata(dev);
+
+	cancel_work_sync(&drv->boot_work);
+}
+
 static int class_suspend(struct device *dev)
 {
 	struct sdca_class_drv *drv = dev_get_drvdata(dev);
@@ -330,6 +337,7 @@ static struct sdw_driver class_sdw_driver = {
 	},
 
 	.probe		= class_sdw_probe,
+	.remove		= class_sdw_remove,
 	.id_table	= class_sdw_id,
 	.ops		= &class_sdw_ops,
 };
-- 
2.47.3

[PATCH v2 3/4] ASoC: SDCA: Tidy up irq_enable_flags()/sdca_irq_disable()

[Charles Keepax]

In irq_enable_flags() and sdca_irq_disable() there is a NULL
check on the interrupt data pointer, however this is just pulled
from an array so can never be NULL. This was likely left over
from an earlier version that looked up the data in a different
way. Replace the check with checking for the IRQ itself being
non-zero.

Whilst here also drop the sdca_interrupt structure down into
the loop within the function to better match the style of the
rest of the code in this file.

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

No changes since v1.

 sound/soc/sdca/sdca_interrupts.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 475aebcafe3f1..3d7eb19967243 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -630,13 +630,12 @@ EXPORT_SYMBOL_NS_GPL(sdca_irq_allocate, "SND_SOC_SDCA");
 static void irq_enable_flags(struct sdca_function_data *function,
 			     struct sdca_interrupt_info *info, bool early)
 {
-	struct sdca_interrupt *interrupt;
 	int i;
 
 	for (i = 0; i < SDCA_MAX_INTERRUPTS; i++) {
-		interrupt = &info->irqs[i];
+		struct sdca_interrupt *interrupt = &info->irqs[i];
 
-		if (!interrupt || interrupt->function != function)
+		if (!interrupt->irq || interrupt->function != function)
 			continue;
 
 		switch (SDCA_CTL_TYPE(interrupt->entity->type,
@@ -689,13 +688,12 @@ EXPORT_SYMBOL_NS_GPL(sdca_irq_enable, "SND_SOC_SDCA");
 void sdca_irq_disable(struct sdca_function_data *function,
 		      struct sdca_interrupt_info *info)
 {
-	struct sdca_interrupt *interrupt;
 	int i;
 
 	for (i = 0; i < SDCA_MAX_INTERRUPTS; i++) {
-		interrupt = &info->irqs[i];
+		struct sdca_interrupt *interrupt = &info->irqs[i];
 
-		if (!interrupt || interrupt->function != function)
+		if (!interrupt->irq || interrupt->function != function)
 			continue;
 
 		disable_irq(interrupt->irq);
-- 
2.47.3

[PATCH 4/4] ASoC: SDCA: Update text of FIXME

[Charles Keepax]

A couple of attempts to correct this FIXME have been sent upstream but
the situation is not quite a simple as the FIXME implies. Update the
FIXME to include a better description of the situation.

Link: https://lore.kernel.org/linux-sound/20260408085607.3813488-1-shumingf@realtek.com/
Link: https://lore.kernel.org/linux-sound/20260324-sdca-function-status-init-irq-v1-1-bba49417a4e0@gmail.com/
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

New since v1.

 sound/soc/sdca/sdca_interrupts.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 3d7eb19967243..6e10b4e660d96 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -119,7 +119,17 @@ static irqreturn_t function_status_handler(int irq, void *data)
 	for_each_set_bit(mask, &status, BITS_PER_BYTE) {
 		switch (BIT(mask)) {
 		case SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION:
-			//FIXME: Add init writes
+/*
+ * FIXME: Should this do init writes?
+ *
+ * Currently init writes/cache sync are done from the suspend/resume
+ * infrastructure. It is unclear in what situations one would receive this
+ * IRQ outside of that flow. Presumably it would be something like the chip
+ * crashing. In that case however doing the init writes and a cache sync might
+ * not be sufficient, for example if the failure was during audio playback
+ * there could be ordering constraints on the register writes to restore the
+ * state that are not handled by a simple cache sync.
+ */
 			break;
 		case SDCA_CTL_ENTITY_0_FUNCTION_FAULT:
 			dev_err(dev, "function fault\n");
-- 
2.47.3

Re: [PATCH 4/4] ASoC: SDCA: Update text of FIXME

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On Thu, Apr 09, 2026 at 05:43:28PM +0100, Charles Keepax wrote:
> A couple of attempts to correct this FIXME have been sent upstream but
> the situation is not quite a simple as the FIXME implies. Update the
> FIXME to include a better description of the situation.

Please don't try to mix versions in the middle of a series, that is just
going to confuse both people and tooling.  The versioning applies to the
series as a whole.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] ASoC: SDCA: Update text of FIXME

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 692 bytes --]

On Thu, Apr 09, 2026 at 06:08:10PM +0100, Mark Brown wrote:
> On Thu, Apr 09, 2026 at 05:43:28PM +0100, Charles Keepax wrote:
> > A couple of attempts to correct this FIXME have been sent upstream but
> > the situation is not quite a simple as the FIXME implies. Update the
> > FIXME to include a better description of the situation.
> 
> Please don't try to mix versions in the middle of a series, that is just
> going to confuse both people and tooling.  The versioning applies to the
> series as a whole.

Yeah, I actually can't convince things to interpret this as part of the
series but the threading means the actual series gets in the way of
handling the patch by itself.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/4] ASoC: SDCA: Update text of FIXME

[Charles Keepax]

On Thu, Apr 09, 2026 at 07:39:12PM +0100, Mark Brown wrote:
> On Thu, Apr 09, 2026 at 06:08:10PM +0100, Mark Brown wrote:
> > On Thu, Apr 09, 2026 at 05:43:28PM +0100, Charles Keepax wrote:
> > > A couple of attempts to correct this FIXME have been sent upstream but
> > > the situation is not quite a simple as the FIXME implies. Update the
> > > FIXME to include a better description of the situation.
> > 
> > Please don't try to mix versions in the middle of a series, that is just
> > going to confuse both people and tooling.  The versioning applies to the
> > series as a whole.
> 
> Yeah, I actually can't convince things to interpret this as part of the
> series but the threading means the actual series gets in the way of
> handling the patch by itself.

Sorry I didn't actually mean to do that. I will resend the extra
patch.

Thanks,
Charles

Re: [PATCH 0/4] Yet another round of SDCA fixes

[Mark Brown]

On Thu, 09 Apr 2026 17:43:24 +0100, Charles Keepax wrote:
> Yet another round of SDCA fixes
> 
> Another round of SDCA fixes a couple of fix to the IRQ cleanup
> from Richard, and a minor tweak to the IRQ handling from me and
> an update to the comment on a fixme.
> 
> Thanks,
> Charles
> 
> [...]

Applied to

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

Thanks!

[1/4] ASoC: SDCA: Correct kernel doc for sdca_irq_cleanup()
      https://git.kernel.org/broonie/sound/c/c271b0815f45
[2/4] ASoC: SDCA: Fix cleanup inversion in class driver
      https://git.kernel.org/broonie/sound/c/7936490e0473
[3/4] ASoC: SDCA: Tidy up irq_enable_flags()/sdca_irq_disable()
      https://git.kernel.org/broonie/sound/c/87ceac0a98e9

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